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

CSP: Remove unsafe-eval & unsafe-inline from script-src #14831

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robhudson
Copy link
Member

@robhudson robhudson commented Jul 15, 2024

One-line summary

Remove unsafe-eval & unsafe-inline from script-src

NOTE
This also changes webpack to use devtool: 'source-map' which may result in slightly slower build times. This is needed to satisfy the CSP requirements locally (not use eval) so devs can spot any breaking CSP being added. On my machine it was a negligible difference but I'd be interested to hear about other experiences.

Issue / Bugzilla link

#14828

Testing

I tested locally by clicking around with the console open looking for CSP violations, including the wagtail admin in my checks.

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.34%. Comparing base (9f3ba2f) to head (0951a6f).
Report is 17 commits behind head on main.

Files Patch % Lines
bedrock/settings/__init__.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14831      +/-   ##
==========================================
- Coverage   77.34%   77.34%   -0.01%     
==========================================
  Files         161      161              
  Lines        8321     8334      +13     
==========================================
+ Hits         6436     6446      +10     
- Misses       1885     1888       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexgibson
Copy link
Member

Thanks for taking a look at this one @robhudson! It would really be great to make these improvements.

One area that might be worth investigating is GA4. In particular, I see in their CSP docs there is mention of needing 'unsafe-eval' if we use any custom JavaScript variables.

I might be mistaken (and maybe @stephaniehobson can confirm?), but we might use a couple still that need removing:

@stephaniehobson
Copy link
Contributor

stephaniehobson commented Jul 22, 2024

Yeah, we are still using those but they can be removed now that GA3 is gone. We just have to make some changes bedrock side first.

This tightens the CSP for the report-only policy. If all looks good we can migrate to the enforced policy.

This also adds exceptions for the wagtail admin.

fix formatting
This is to avoid CSP 'unsafe-eval' errors.
@robhudson
Copy link
Member Author

This updates the PR to only remove these from the report-only policy so we can see how it does there and we should get CSP reports in sentry, but it won't break anything since it's not enforced.

@alexgibson alexgibson added Backend Server stuff yo Frontend HTML, CSS, JS... client side stuff labels Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Server stuff yo Frontend HTML, CSS, JS... client side stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants