Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Uses cookies to track the requests redirection#25719
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
stof commentedJan 8, 2018
Well, solving a complain about one page being uncachable by keeping it uncachable for a different reason does not seem to solve it. |
nicolas-grekas commentedJan 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof at least this would make it explicit aboutwhy this is not cached, and also would prevent messing up with users' Cache-Control headers. So far better IMHO. |
sroze commentedJan 8, 2018
Yup,@nicolas-grekas' point was my reasoning behind it. Note that Travis' error is unrelated. |
stof commentedJan 8, 2018
Looks good to me then |
fabpot commentedJan 8, 2018
Thank you@sroze. |
…n (sroze)This PR was merged into the 3.4 branch.Discussion----------[HttpKernel] Uses cookies to track the requests redirection| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25698| License | MIT| Doc PR | øIn order to track the redirections across requests, we need to have some state. So far, we've been using the session but some users have complained about it (#24774,#24730). The idea is that we don't actually need the session, we can use cookies.It's a tradeoff: using a cookie would mean that both the redirection and the target page will not be cachable (because of the Set-Cookie to set the sf_redirect and the one to clear it).As it's only on dev, it seems fair to say that having no cache (because of `Set-Cookie`s) is a better side effect than starting the session.Commits-------83f2579 Uses cookies to track the requests redirection
…redirection (sroze)This PR was merged into the 3.4 branch.Discussion----------[HttpKernel] Add tests for request collector and cookie redirection| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes (#25719)| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR | øNot that I felt bad doing a PR without tests (#25719) but this one adds tests to be sure we stabilize this cookie-based redirection.Commits-------7b4f5a1 Add tests for the HttpKernel request collector and redirection via cookies
lookitsjonno commentedSep 21, 2018
This PR discloses the file system structure to the browser which isn't great in terms of security. |
nicolas-grekas commentedSep 21, 2018
The profiler discloses way more info than just this cookie. It's always been flagged as FOR DEV ONLY. |
lookitsjonno commentedSep 21, 2018
The folder structure is disclosed in production with profiler disabled. |
In order to track the redirections across requests, we need to have some state. So far, we've been using the session but some users have complained about it (#24774,#24730). The idea is that we don't actually need the session, we can use cookies.
It's a tradeoff: using a cookie would mean that both the redirection and the target page will not be cachable (because of the Set-Cookie to set the sf_redirect and the one to clear it).
As it's only on dev, it seems fair to say that having no cache (because of
Set-Cookies) is a better side effect than starting the session.