Skip to content

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

Closed
unsane1047 opened this issue May 24, 2016 · 3 comments

Comments

@unsane1047
Copy link

unsane1047 commented May 24, 2016

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:

if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) {
    return $targetUrl;
}

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.

mrzard pushed a commit to mrzard/symfony that referenced this issue Jun 10, 2016
mrzard pushed a commit to mrzard/symfony that referenced this issue Jun 10, 2016
@mrzard
Copy link

mrzard commented Jun 10, 2016

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
@fabpot fabpot closed this as completed Mar 22, 2017
@develth
Copy link

develth commented Jun 30, 2017

The change breaks the login redirect. See comments here:

bafa8e2

@xabbuh
Copy link
Member

xabbuh commented Jun 30, 2017

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
Projects
None yet
Development

No branches or pull requests

6 participants