[2.8][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context #18434
Conversation
b2ff331 to
20b4c7f
Compare
|
|
||
| $headers = $this->getCSPHeaders($response); | ||
|
|
||
| foreach($headers as $header => $directives) { |
There was a problem hiding this comment.
missing space after foreach.
|
IMO, this should be considered as a new feature rather than a bugfix |
| * | ||
| * @return string The Content-Security-Policy header | ||
| */ | ||
| private function generateCSPHeader(array $directives) |
There was a problem hiding this comment.
Given that this method just reduces an array to a string, maybe we could use the array_reduce() method.
|
thus, another reason to consider it a new feature is that any bundle extending the profiler will have to consider it too. |
| { | ||
| $response = new Response('', $code, $headers); | ||
|
|
||
| $nonces = $this->cspHandler ? $this->cspHandler->getNonces($request, $response) : []; |
There was a problem hiding this comment.
Forbidden short array notation in this line
|
I have a global concern about this code. As usually happens with acronyms, it's hard to name methods and variables. But right now there are different criteria: updateCSPheaders()
getCSPHeaders()
$cspHandler;I propose to treat updateCspHeaders()
getCspHeaders()
$cspHandler; |
| @@ -32,6 +34,9 @@ | |||
| <argument>%kernel.debug%</argument> | |||
| </service> | |||
|
|
|||
| <service id="web_profiler.csp_handler" class="%web_profiler.csp_handler.class%"> | |||
| </service> | |||
There was a problem hiding this comment.
Can be removed and replaced with a single <service/> monotag.
There was a problem hiding this comment.
@hhamon I think they are called "empty tags" ... but I absolutely love the "monotag" word 😁
There was a problem hiding this comment.
I never knew how to call them and this is why I also end up calling them "monotag" :D
f81a05c to
455cbd5
Compare
|
@stof I proposed this as a bug fix because it prevents usage of Symfony in a content-security-policy context. This is fully BC with other usage (Silex and any bundle extending it) |
|
For the record, I updated the PR according to your comments |
8de3cdf to
ff1cccd
Compare
| @@ -397,4 +400,18 @@ protected function getTemplateManager() | |||
|
|
|||
| return $this->templateManager; | |||
| } | |||
|
|
|||
| private function renderWithCspNonces(Request $request, $template, $variables, $code = 200, $headers = array('Content-Type' => 'text/html')) | |||
There was a problem hiding this comment.
according to your talk at the SymfonyLive right now, wouldn't it be better to use signatures instead ?
There was a problem hiding this comment.
Actually this PR provides CSP support for setups where unsafe-inline is disabled in development environment (because it only affects the WebProfilerBundle that should not be enabled in prod in any case)
So, even if it's recommended to use signature instead of nonce, it's a recommendation for production or staging. In development, it enables user to fully uses test with their CSP and uses the WebProfilerBundle.
So, no, it's not a problem :)
ff1cccd to
e1b2404
Compare
e1b2404 to
0c61a7b
Compare
|
I just amended the PR according to Javier's comment about |
|
A side note argument about "new feature" vs "bug fix": Considering this a "new feature" would disable the WebProfilerBundle to any user that uses Content Security Policy in their Symfony Application until 3.2. Moreover, it would mean that Symfony would not provide a LTS version compatible with Content Security Policy before the end of 2017 (3.4 if I'm not wrong). |
|
Replaced by #18568 that proposes this has a new feature in 3.2 |
…ecurity-Policy context without unsafe-inline (romainneutron) This PR was merged into the 3.2-dev branch. Discussion ---------- [3.2][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15397 | License | MIT | Doc PR | N/A Hello, this PR fixes the compatibility of the WebprofilerBundle in a context where Content-Security-Policy headers are could prevent `unsafe-inline` of `script-src` or `style-src` directives. This PR has been originally proposed in 2.8 in #18434 Commits ------- 571a1f2 [WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline
Hello, this PR fixes the compatibility of the WebprofilerBundle in a context where Content-Security-Policy headers are could prevent
unsafe-inlineofscript-srcorstyle-srcdirectives.I intentionally open this PR against 2.8 instead of 2.3. Fixing this in 2.3/2.7 is a PITA due to the legacy codebase of the WebProfiler. It would imply doing the job twice. Moreover, doing this is not trivial at all. I hope you all understand.
Job is done at 80%, so I open the PR to get feedback.
ContentSecurityPolicyHandlerandWebDebugToolbarListener