Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DX] Log potential redirect loops caused by forced HTTPS#27605
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
[DX] Log potential redirect loops caused by forced HTTPS#27605
Uh oh!
There was an error while loading.Please reload this page.
Conversation
colinodell commentedJun 14, 2018
I have also submittedsymfony/symfony-docs#9924 to help point developers in the right direction via documentation.That PR does not document this change, it just provides a different avenue to notify the developer about the potential issue. Feel free to merge that instead of this (or merge both! 😃) |
| if (null !==$this->logger) { | ||
| $this->logger->info('Redirecting to HTTPS.'); | ||
| if ('https' ===$request->headers->get('X-Forwarded-Proto')) { | ||
| $this->logger->debug('Possible redirect loop - did you set "trusted_proxies" correctly?'); |
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.
What aboutwarn?
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 log message could be triggered by anybody who sends HTTP requests to the server with that header:
curl http://www.example.com -H'X-Forwarded-Proto: https'If the site is configured to log messages with a minimum severity ofwarn or higher then their logs could potentially be flooded with "false positives". Usingdebug() would prevent that from happening unless the site is configured to logdebug messages (which I'd assume somebody might do if they had an unexpected redirect loop and were looking for clues).
That being said, I'm not strongly opposed to using a different log level if others feel that it is more appropriate.
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.
since there is already an info level message, we could merge the new with the existing one:$this->logger->info('Redirecting to HTTPS ("X-Forwarded-Proto" is set to "https" - did you set "trusted_proxies" correctly?).');
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.
Good idea! I've made it so that only one message will ever be logged.
| if ('https' ===$channel && !$request->isSecure()) { | ||
| if (null !==$this->logger) { | ||
| $this->logger->info('Redirecting to HTTPS.'); | ||
| if ('https' ===$request->headers->get('X-Forwarded-Proto')) { |
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 could also handle rfc7239's "proto" 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.
I've added an additional check for this parameter.
15748c7 to7714655CompareIf the developer forgets/fails to set "trusted_proxies" properly, forcing thehttps channel can cause infinite redirect loops. This change will hopefullyhelp them identify the problem faster.Seesymfony#27603
7714655 to53048ceComparefabpot commentedJun 20, 2018
Thank you@colinodell. |
…PS (colinodell)This PR was merged into the 4.2-dev branch.Discussion----------[DX] Log potential redirect loops caused by forced HTTPS| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27603| License | MIT| Doc PR | n/aIf the developer forgets/fails to set "trusted_proxies" properly, forcing thehttps channel can cause infinite redirect loops. This change will hopefullyhelp them identify the problem faster.See#27603Commits-------53048ce Log potential redirect loops caused by forced HTTPS
If the developer forgets/fails to set "trusted_proxies" properly, forcing the
https channel can cause infinite redirect loops. This change will hopefully
help them identify the problem faster.
See#27603