Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Check UserInterface::getPassword is not null before calling needsRehash #34802

Merged
merged 1 commit into from Dec 6, 2019
Merged

[Security] Check UserInterface::getPassword is not null before calling needsRehash #34802

merged 1 commit into from Dec 6, 2019

Conversation

dbrekelmans
Copy link
Contributor

@dbrekelmans dbrekelmans commented Dec 4, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface::needsRehash() expects a string as the input argument. In some cases Symfony\Component\Security\Core\User\UserInterface::getPassword() is used as the input argument, but this function can return null resulting in a potential type error.

@nicolas-grekas nicolas-grekas changed the title [Security] Check UserInterface::getPassword is not null before callin… [Security] Check UserInterface::getPassword is not null before calling needsRehash Dec 4, 2019
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Dec 4, 2019
@dbrekelmans
Copy link
Contributor Author

@xabbuh Based on #34824 (comment) I could make some additional changes in this PR if you agree.

Which option would have your preference?

  1. Check if $user->getPassword() is null before call needsRehash() (as are the current changes in this PR, I'd just have to add it to isPasswordValid() as well)
  2. throw BadCredentialsException (or another exception) when $user->getPassword() is null

@xabbuh
Copy link
Member

xabbuh commented Dec 6, 2019

@dbrekelmans I think needsRehash() should return false when the user's password is null.

@xabbuh
Copy link
Member

xabbuh commented Dec 6, 2019

By the way, #34779 has been merged up to 4.4. You can rebase your changes now. :)

@dbrekelmans
Copy link
Contributor Author

@xabbuh I rebased on 4.4. I think the PR is good to go :)

@chalasr
Copy link
Member

chalasr commented Dec 6, 2019

Thank you @dbrekelmans.

chalasr added a commit that referenced this pull request Dec 6, 2019
…fore calling needsRehash (dbrekelmans)

This PR was squashed before being merged into the 4.4 branch (closes #34802).

Discussion
----------

[Security] Check UserInterface::getPassword is not null before calling needsRehash

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

`Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface::needsRehash()` expects a string as the input argument. In some cases `Symfony\Component\Security\Core\User\UserInterface::getPassword()` is used as the input argument, but this function can return `null` resulting in a potential type error.

Commits
-------

8e4cf49 [Security] Check UserInterface::getPassword is not null before calling needsRehash
@chalasr chalasr merged commit 8e4cf49 into symfony:4.4 Dec 6, 2019
@chalasr
Copy link
Member

chalasr commented Dec 6, 2019

And congratz for your first contrib!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants