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

Add nonce also to legacy CSP #1920

Merged
merged 2 commits into from
Oct 26, 2016

Conversation

LukasReschke
Copy link
Member

Pages that do not use the AppFramework have its CSP inherited from \OC_Response::addSecurityHeaders. While those are not many anymore, there are some examples such as the "Help" page.

To stay completely backwards-compatible we should also add the nonce to the legacy CSP response.

To test that open your browser console and open the help page. Without this you will get a JS error. With this you won't.

Signed-off-by: Lukas Reschke [email protected]

cc @rullzer

Pages that do not use the AppFramework have its CSP inherited from `\OC_Response::addSecurityHeaders`. While those are not many anymore, there are some examples such as the "Help" page.

To stay completely backwards-compatible we should also add the nonce to the legacy CSP response.

To test that open your browser console and open the help page. Without this you will get a JS error. With this you won't.

Signed-off-by: Lukas Reschke <[email protected]>
@LukasReschke LukasReschke added the 3. to review Waiting for reviews label Oct 26, 2016
@LukasReschke LukasReschke added this to the Nextcloud 11.0 milestone Oct 26, 2016
@mention-bot
Copy link

@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @bartv2 and @bantu to be potential reviewers.

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen
Copy link
Member

nickvergessen commented Oct 26, 2016

Works after fixing Chromium being identified as Chrome to make use of the feature.
Pushed it to this branch after talking to lukas

👍

@LukasReschke
Copy link
Member Author

Thanks, @nickvergessen 🚀

@codecov-io
Copy link

Current coverage is 57.18% (diff: 0.00%)

Merging #1920 into master will decrease coverage by 0.16%

@@             master      #1920   diff @@
==========================================
  Files          1078       1078          
  Lines         61498      61701   +203   
  Methods        6875       6900    +25   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35266      35282    +16   
- Misses        26232      26419   +187   
  Partials          0          0          

Sunburst

Diff Coverage File Path
0% ...e/Security/CSP/ContentSecurityPolicyNonceManager.php
0% lib/private/legacy/response.php
•••••••••• 100% lib/private/AppFramework/Http/Request.php

Powered by Codecov. Last update a973c1b...c20ab00

@rullzer
Copy link
Member

rullzer commented Oct 26, 2016

yes LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants