[Security] add Request type json check in json_login #22477
Conversation
23a3293 to
b4d4500
Compare
| @@ -73,6 +73,9 @@ public function __construct(TokenStorageInterface $tokenStorage, AuthenticationM | |||
| public function handle(GetResponseEvent $event) | |||
| { | |||
| $request = $event->getRequest(); | |||
| if ('json' !== $request->getRequestFormat() || 'json' !== $request->getContentType()) { | |||
There was a problem hiding this comment.
AFAIK the checked content type should be application/json
There was a problem hiding this comment.
nevermind, it seems getContentType() does return json
| @@ -73,6 +73,9 @@ public function __construct(TokenStorageInterface $tokenStorage, AuthenticationM | |||
| public function handle(GetResponseEvent $event) | |||
| { | |||
| $request = $event->getRequest(); | |||
| if ('json' !== $request->getRequestFormat() || 'json' !== $request->getContentType()) { | |||
There was a problem hiding this comment.
I would use false !== strpos($format, 'json') instead of the strict test to allow to execute the listener for any JSON-based formats including JSON-LD and HAL.
772ef1d to
104cc9c
Compare
|
hmm .. will have to investigate why tests are partially passing with some configurations. any hints welcome. |
| || false !== strpos($request->getContentType(), 'json') | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think this should be:
if (false === strpos($request->getRequestFormat(), 'json') && false === strpos($request->getContentType(), 'json')) {
return;
}Skipping only if both request format and content type do not contain json. It makes your tests green.
If we want to skip if only one of them does not contain json, you have to set the _format in all the existing tests in addition to the content type (and still change false !== in false ===, actually we are skipping for json requests :))
There was a problem hiding this comment.
yeah .. logic mistake :)
cc5ff7c to
223d133
Compare
|
@lsmith77 some jobs are just skipping all tests for PRs (to limit the number of queued jobs due to the activity on the repo), which is why they cannot fail |
223d133 to
f62c7be
Compare
|
ok .. tests are fixed |
|
For next reviewers, here is the main reason for this PR:
|
f62c7be to
045a36b
Compare
|
rebased on master |
|
@dunglas ok for you? |
|
Thank you @lsmith77. |
…mith77) This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] add Request type json check in json_login | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no, unreleased feature | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | - follow up to #22425 to limit the `UsernamePasswordJsonAuthenticationListener` to only requests with appropriate JSON content type. I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered. Commits ------- 045a36b add Request type json check in json_login
…sent on bad request (ogizanagi)
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Security] json login listener: ensure a json response is sent on bad request
| Q | A
| ------------- | ---
| Branch? | master (3.3)
| Bug fix? | yesish
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | N/A
| License | MIT
| Doc PR | N/A
I would have simply recommended to set the proper format when declaring the route:
```yml
# routing.yml
api_login:
path: /login
defaults: { _format: json }
```
but, since #22477 has been merged, and considering #22477 (comment):
> my point above regarding checking the content type is so that one could use form_login and json_login in parallel on the same routes and within the same firewall
we may consider setting the request format to json when throwing the `BadRequestHttpException`, so used conjointly with the TwigBundle, the exception is rendered using the `exception.json.twig` template.
ping @lsmith77
(An alternative would be to check the Accept header to set the request format to json if it's the preferred one instead of doing it each time we throw the exception. But Symfony never used such content negotiation AFAIK, and I think it's safe enough to assume someone sending json is expecting json as ouput for exceptions.)
Commits
-------
4427cf9 [Security] json login listener: ensure a json response is sent on bad request
follow up to #22425 to limit the
UsernamePasswordJsonAuthenticationListenerto only requests with appropriate JSON content type.I am not entirely happy with this implementation but mostly because Symfony out of the box only provides very limited content type negotiation. I guess anyone that wants to tweak the content negotiation will simply need to ensure the Request::$format is set accordingly before the code is triggered.