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/Http] Ignore invalid URLs found in failure/success paths#46317
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
nicolas-grekas commentedMay 11, 2022
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix#43567 |
| License | MIT |
| Doc PR | - |
wouterj 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.
sorry, too quick
| if (\is_string($failureUrl) &&str_starts_with($failureUrl,'/')) { | ||
| $options['failure_path'] =$failureUrl; | ||
| }elseif ($this->logger &&$failureUrl) { | ||
| $this->logger->debug(sprintf('Ignoring query parameter "%s": not a valid URL.',$options['failure_path_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.
Should we include the value in the log message/context?
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 don't think so: this is user input, might be just garbage. The query parameter name is enough to me.
| } | ||
| if ($this->options['failure_forward']) { | ||
| $options['failure_path'] ??$options['failure_path'] =$options['login_path']; |
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 must confess that I'm not at all sure if I understand what is going on here. Is this a leftover?
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 is$options['failure_path'] ??= $options['login_path']; written for PHP 7.1+
fabpot commentedMay 13, 2022
Thank you@nicolas-grekas. |
crmpicco commentedJun 23, 2022
Is there a recommended way to pass through the |
…m Ward)This PR was merged into the 4.4 branch.Discussion----------[Security] Allow redirect after login to absolute URLs| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#46533| License | MIT| Doc PR |Fixes the regression introduced by#46317, and once again allows absolute URLs to be the target of authentication redirection, as specified in the documentation (https://symfony.com/doc/current/security/form_login.html#changing-the-default-page)Commits-------b9901aa [Security] Allow redirect after login to absolute URLs
- seesymfony/symfony#46317- _target_path 及び _failure_path はURLまたはパスを指定する必要がある- アプリケーション範囲外の URL を指定された場合は homepage へリダイレクトする
- seesymfony/symfony#46317- _target_path 及び _failure_path はURLまたはパスを指定する必要がある- アプリケーション範囲外の URL を指定された場合は homepage へリダイレクトする