[HttpKernel] Disable CSP header on exception pages #25532
Conversation
|
@romainneutron as our CSP expert, would you agree with this change? Thanks! @ostrolucky adding new listeners always impacts performance. Can't this header removal be done in the existing |
|
There must be CSP header removal, because we don't have control over not adding it, it's done in userspace. What can be done to avoid unnecessarily adding new listener is to add this listener from inside onKernelException, but that will require access to EventDispatcher in this listener. Let me know if you would like that more, or there is better way. |
|
@ostrolucky you can have access to the EventDispatcher. It is passed as third argument to the listener when calling it. |
|
However, if the listener is added dynamically, don't forget to remove it dynamically too (to reset state properly for reused kernel). |
|
@stof Unfortunately I can't do that. According to BC promise I can't add arguments to public method 🙁 edit: but added it via func_get_arg. Better? |
4b40940 to
187e429
Compare
|
Can anybody think of a simpler solution to this issue? The func_get_arg, the listeners, etc. look a bit complicated (but maybe this is the simplest solution possible). |
|
|
|
I would suggest to make the class final in 4.1 (can be done now) |
|
The code looks good to me. |
|
Sure. What kind of test? |
|
eg one that check that whe appropriate, the header is removed, and the event listener is not left in place? |
| @@ -41,6 +43,7 @@ public function onKernelException(GetResponseForExceptionEvent $event) | |||
| { | |||
| $exception = $event->getException(); | |||
| $request = $event->getRequest(); | |||
| $eventDispatcher = func_num_args() > 2 ? func_get_arg(2) : null; | |||
There was a problem hiding this comment.
just define the proper arguments instead
There was a problem hiding this comment.
Can I define them as non-optional?
There was a problem hiding this comment.
@stof that'd be a BC break...
There was a problem hiding this comment.
hmm, our listener methods are indeed not marked as @internal 😢
|
status: needs review |
| $cspRemovalListener = function (FilterResponseEvent $event) use (&$cspRemovalListener, $eventDispatcher) { | ||
| $event->getResponse()->headers->remove('Content-Security-Policy'); | ||
| $eventDispatcher->removeListener(KernelEvents::RESPONSE, $cspRemovalListener); | ||
| }; |
There was a problem hiding this comment.
This code should be move under the if as it's only used there.
|
Thank you @ostrolucky. |
…ucky) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Disable CSP header on exception pages | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24772 | License | MIT | Doc PR | - This makes exception pages styled normally when using CSP listener. Technically it's new feature, but from user POV it's bugfix, because it's really confusing to get a blank page. It takes a while to realize it is because of enabled CSP. Therefore I'm trying to push it as patch release. Commits ------- f33a383 [HttpKernel] Disable CSP header on exception pages
This makes exception pages styled normally when using CSP listener. Technically it's new feature, but from user POV it's bugfix, because it's really confusing to get a blank page. It takes a while to realize it is because of enabled CSP. Therefore I'm trying to push it as patch release.