Skip to content
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

Do not inject web debug toolbar on attachments #18971

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
7 participants
@peterrehm
Copy link
Contributor

commented Jun 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 -

@peterrehm peterrehm force-pushed the peterrehm:toolbar branch 2 times, most recently from f3960c8 to 0a39fc1 Jun 5, 2016

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

👍

Status: reviewed

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

👍

@@ -112,6 +112,11 @@ public function onKernelResponse(FilterResponseEvent $event)
*/
protected function injectToolbar(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')) {

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 6, 2016

Member

This should be moved in the condition starting at line 96 instead

This comment has been minimized.

Copy link
@peterrehm

peterrehm Jun 6, 2016

Author Contributor

Updated.

@javiereguiluz javiereguiluz added the Ready label Jun 6, 2016

@@ -98,6 +98,7 @@ public function onKernelResponse(FilterResponseEvent $event)
|| $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')

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 6, 2016

Member

The content-disposition types are case-insensitive according to https://tools.ietf.org/html/rfc2183
Also this current check will also trigger for Content-Disposition: <something-else>; filename=attachment which is probably not intended.

This comment has been minimized.

Copy link
@peterrehm

peterrehm Jun 7, 2016

Author Contributor

Updated accordingly. The question is now the check, I am now checking for attachment; instead.

@@ -98,6 +98,7 @@ public function onKernelResponse(FilterResponseEvent $event)
|| $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;')

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 8, 2016

Member

stripos instead of strpos+strtolower?

This comment has been minimized.

Copy link
@peterrehm

peterrehm Jun 8, 2016

Author Contributor

Updated

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

Thank you @peterrehm.

fabpot added a commit that referenced this pull request Jun 8, 2016

bug #18971 Do not inject web debug toolbar on attachments (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

@fabpot fabpot closed this Jun 8, 2016

@peterrehm peterrehm deleted the peterrehm:toolbar branch Jun 8, 2016

@fabpot fabpot referenced this pull request Jun 15, 2016

Merged

Release v3.1.1 #19055

This was referenced Jun 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.