-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Detection of when target url is the same as the login_path is extremely brittle #18862
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
Labels
Comments
mrzard
pushed a commit
to mrzard/symfony
that referenced
this issue
Jun 10, 2016
…arget_url and login_path
Hi, I just submitted a PR that makes this a bit better, let me know if this useful :) |
fabpot
added a commit
that referenced
this issue
Mar 22, 2017
…n_path (mrzard) This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] Strengthen comparison of target_url vs login_path | Q | A | | --- | --- | | Branch? | "master" | | Bug fix? | no | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #18862 | | License | MIT | | Doc PR | | Commits ------- ac9d75a [Security] Strengthen comparison of target_url vs login_path
This was referenced Jun 5, 2017
The change breaks the login redirect. See comments here: |
Can you open a new issue with a description of your problem and how to reproduce it? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The DefaultAuthenticationSuccessHanlder class attempts to redirect you to the specified default_target_path if the referrer is equal to the specified login_path, however, this detection is extremely brittle. Simply adding a blank query string ( /login? instead of /login) when visiting the login_path directly causes the login to wrap back to the login_path rather than using the default target path.
Specifically, these lines (122 through 124) from Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler.php are the issue:
The last part of the condition in this if statement checks only if the Referer matches the login_path exactly, not taking into account negligible cases such as an empty query string.
I am new to Symfony, so I'm not sure how to accomplish a less brittle check directly using Symfony components, however, I have patched my custom AuthenticationSuccessHandler to split the url and remove any empty query string before comparing the two paths in order to catch this case.
The text was updated successfully, but these errors were encountered: