Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Do not inject web debug toolbar on attachments#18971
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
peterrehm commentedJun 5, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #18965 |
| License | MIT |
| Doc PR | - |
javiereguiluz commentedJun 5, 2016
👍 Status: reviewed |
xabbuh commentedJun 5, 2016
👍 |
| protectedfunctioninjectToolbar(Response$response,Request$request) | ||
| { | ||
| // The toolbar shall not be injected if the header enforces a download of the content | ||
| if (false !==strpos($response->headers->get('Content-Disposition'),'attachment')) { |
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 should be moved in the condition starting at line 96 instead
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.
Updated.
| ||$response->isRedirection() | ||
| || ($response->headers->has('Content-Type') &&false ===strpos($response->headers->get('Content-Type'),'html')) | ||
| ||'html' !==$request->getRequestFormat() | ||
| ||false !==strpos($response->headers->get('Content-Disposition'),'attachment') |
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.
The content-disposition types are case-insensitive according tohttps://tools.ietf.org/html/rfc2183
Also this current check will also trigger forContent-Disposition: <something-else>; filename=attachment which is probably not intended.
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.
Updated accordingly. The question is now the check, I am now checking forattachment; instead.
| ||$response->isRedirection() | ||
| || ($response->headers->has('Content-Type') &&false ===strpos($response->headers->get('Content-Type'),'html')) | ||
| ||'html' !==$request->getRequestFormat() | ||
| ||false !==strpos(strtolower($response->headers->get('Content-Disposition')),'attachment;') |
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.
stripos instead of strpos+strtolower?
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.
Updated
fabpot commentedJun 8, 2016
Thank you@peterrehm. |
This PR was squashed before being merged into the 2.7 branch (closes#18971).Discussion----------Do not inject web debug toolbar on attachments| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18965| License | MIT| Doc PR | -Commits-------4a7d836 Do not inject web debug toolbar on attachments