Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[3.2][WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline#18568
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
| if (!$this->authorizesInline($directives, $type)) { | ||
| if (!isset($headers[$header][$type])) { | ||
| if (isset($headers[$header]['default-src'])) { | ||
| $headers[$header][$type] = $headers[$header]['default-src']; |
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.
As this is nested pretty deeply, it might be more readable if the first if becomes a guard clause with a continue
nicolas-grekas commentedApr 17, 2016 • 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.
How could we ensure that strict CSP compliancy is preserved on the long run? |
e305560 to3c75a1eCompareromainneutron commentedApr 17, 2016
Toggling to Need Work : I will disable CSP when browsing profiler pages and will introduce nonce only for toolbar injection. |
cda45ee toe5e4f73Compareromainneutron commentedApr 17, 2016
Work done, it can now be reviewed |
romainneutron commentedApr 17, 2016
@nicolas-grekas you asked this question. Checking if the injected script has the appropriate nonce, headers are included and rendered toolbar include nonce should be alright. |
0a024c0 to3083453Compare| {{dump.data|raw }} | ||
| </div> | ||
| {%endfor %} | ||
| <imgsrc="data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="onload="var h = this.parentNode.innerHTML, rx=/<script>(.*?)<\/script>/g, s; while (s = rx.exec(h)) {eval(s[1]);};" /> |
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.
@nicolas-grekas what was the reason for this one ?
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.
Toggle nested levels in the toolbar
| $this->cleanHeaders($response); | ||
| $this->updateCspHeaders($response, $nonces); | ||
| return $nonces; |
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.
returning something from an event listener looks weird to me
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.
OK, this method is actually not an event listener. So the phpdoc is wrong (and the naming is not that good either)
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.
fixed
romainneutron commentedJun 1, 2016
Thanks@stof for this review, I'm going to take care of this |
| /** | ||
| * @param Response $response | ||
| */ |
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.
to be removed
fabpot commentedJun 8, 2016
👍 |
e48e16e to52ae783Compareromainneutron commentedJun 9, 2016
PR updated, comments addressed |
…ron)This PR was merged into the 2.8 branch.Discussion----------[2.8][WebProfilerBundle] Fix invalid CSS style| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/ARelated to#18568 (comment)Commits-------aa35a16 [WebProfilerBundle] Fix invalid CSS style
52ae783 toef3827fCompare| * | ||
| * @return array | ||
| */ | ||
| private function updateCspHeaders(Response $response, array $nonces = array()) { |
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.
{ on a new line
ef3827f tobb94559Compare…xt without unsafe-inline
bb94559 to571a1f2Comparefabpot commentedJun 9, 2016
Thank you@romainneutron. |
…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/AHello, 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#18434Commits-------571a1f2 [WebProfilerBundle] Fix bundle usage in Content-Security-Policy context without unsafe-inline
herndlm commentedJun 4, 2018
Could this be broken since 4.1 in contexts where unsafe-eval is not allowed? I had problems after upgrading because of CSP violations, but need to retest tomorrow |
Hello, this PR fixes the compatibility of the WebprofilerBundle in a context where Content-Security-Policy headers are could prevent
unsafe-inlineofscript-srcorstyle-srcdirectives.This PR has been originally proposed in 2.8 in#18434