Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Fix fatal error on non string username#25657
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJan 2, 2018
I think we should do the same with the provided password. |
| $password =$requestBag->get($this->options['password_parameter'],null,true); | ||
| if (!is_string($username)) { | ||
| thrownewBadRequestHttpException(sprintf('The key "%s" must be a string.',$this->options['username_parameter'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This kind of messages in Symfony are usually like:
sprintf('The XXX must be a string, "%s" given.',gettype($XXX))
Is it necessary to do the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| $password =$requestBag->get($this->options['password_parameter'],null,true); | ||
| if (!is_string($username)) { | ||
| thrownewBadRequestHttpException(sprintf('The key "%s" must be a string.',$this->options['username_parameter'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
as javiere said I think we should change both message too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
got it :) needs work
6894e56 to4205e5eCompare4205e5e toa3e17daComparexabbuh commentedJan 11, 2018
@chalasr tests are failing |
chalasr commentedJan 11, 2018
@xabbuh done. Objects implementing |
a3e17da to15ecf24Comparexabbuh commentedJan 11, 2018
@chalasr I also wonder if not passing credentials at all must be supported here. |
e9e6818 tod7e615fCompareacasademont commentedJan 12, 2018
Thanks for the patch! I was going to submit the same issue ;) |
chalasr commentedJan 12, 2018
@xabbuh I think so, password part reverted and |
| array('require_previous_session' =>false) | ||
| ); | ||
| $event =$this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would not mock the event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
d7e615f to8f09568Comparechalasr commentedJan 15, 2018
Ready |
fabpot commentedJan 16, 2018
Thank you@chalasr. |
This PR was merged into the 2.7 branch.Discussion----------[Security] Fix fatal error on non string username| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25612| License | MIT| Doc PR | n/aThat's consistent with what#22569 did for the `json_login` listener.Commits-------8f09568 [Security] Fix fatal error on non string username
That's consistent with what#22569 did for the
json_loginlistener.