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] Handle bad request format in json auth listener#22569
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
[Security] Handle bad request format in json auth listener#22569
Uh oh!
There was an error while loading.Please reload this page.
Conversation
chalasr left a comment
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.
👍 These exceptions are not about a failed authentication but a wrongly formatted request (and don't provide any sensitive info) thus should not trigger the authentication failure handler nor any authentication exception to be thrown.
dunglas commentedApr 28, 2017
Fair enough 👍 |
nicolas-grekas left a comment
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.
👍
fabpot commentedApr 29, 2017
Thank you@ogizanagi. |
… (ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[Security] Handle bad request format in json auth listener| Q | A| ------------- | ---| Branch? | master (3.3)| Bug fix? | yesish| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AIn#22034, I wondered myself if we shouldn't throw a dedicated exception to handle bad formatted requests and give more inputs to the client by returning a 400 response with an explicit message.~~Here is a suggestion, introducing a new `BadRequestFormatException` and using it in `UsernamePasswordJsonAuthenticationListener` whenever there is no custom failure handler set (but someone using its own handler should be able to treat the failure properly too).~~As discussed with@chalasr , it seems better to directly throw a `BadRequestHttpException` as it's actually out of the whole security process. PR updated.Commits-------93a8cb9 [Security] Handle bad request format in json auth listener
nicolas-grekas commentedApr 29, 2017
@ogizanagi master is red after this PR has been merged, would you mind looking at it please (or anyone else really?) |
ogizanagi commentedApr 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
chalasr commentedApr 29, 2017
See#22582 |
This PR was merged into the 3.3-dev branch.Discussion----------Fix tests| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22569 (comment)| License | MIT| Doc PR | n/aCommits-------b6948dd Fix tests
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
Uh oh!
There was an error while loading.Please reload this page.
In#22034, I wondered myself if we shouldn't throw a dedicated exception to handle bad formatted requests and give more inputs to the client by returning a 400 response with an explicit message.
Here is a suggestion, introducing a newBadRequestFormatExceptionand using it inUsernamePasswordJsonAuthenticationListenerwhenever there is no custom failure handler set (but someone using its own handler should be able to treat the failure properly too).As discussed with@chalasr , it seems better to directly throw a
BadRequestHttpExceptionas it's actually out of the whole security process. PR updated.