-
Notifications
You must be signed in to change notification settings - Fork 926
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
Fix fonts CSP for staging #14466
Fix fonts CSP for staging #14466
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14466 +/- ##
=======================================
Coverage 75.72% 75.72%
=======================================
Files 144 144
Lines 7876 7876
=======================================
Hits 5964 5964
Misses 1912 1912 ☔ View full report in Codecov by Sentry. |
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.
This makes sense to me and mirrors something I was just exploring myself. Merging it now as a focused, standalone changeset is better than me adding it to one of my branches, so it's an r+ from me. Thanks @janbrasna!
* Fix font-src for stage hosts * Simplify font CSP to defaults --------- Co-authored-by: Jan Brasna <[email protected]>
One-line summary
Expands
CSP_FONT_SRC
the same way addt'l hosts are added to e.g.CSP_STYLE_SRC
to avoid surprises when run from different hosts as moz.works or run.app… (loaded from the same absolute URIs as styles, so to match the hosts…)Significant changes and points to review
The other rules start with defaults and add more values. Fonts are the exception to this, only specifying its values. This change makes it more consistent, also starting out with defaults (thus also covering anything needed for both mozorg & pocket mode straight away), that make the effective rule a bit wider than used to be — but basically matches what's in
style-src
anyways so that should be okay~ish.This could be further optimised to only add
self
for prod, and the extra defaults only for dev or with extra defaults set only etc., but this is the least complicated change, even though the (prod) output is probably a little excessive:before:
font-src 'self';
font-src 'self';
font-src 'self' assets.getpocket.com;
font-src 'self' assets.getpocket.com;
after:
font-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com *.allizom.org;
font-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com;
font-src 'self' *.getpocket.com *.allizom.org;
font-src 'self' *.getpocket.com;
(basically leaving out
font-src
completely and falling back todefault-src
would have the same effect, but this still retains the possibility to add more rules to the policy in settings, that's why the result is so verbose.)@stevejalim You can either try this in staging alone, or base the PR to be part of #14453 branch if you want to test it together, should be clean merge one way or another.
Issue / Bugzilla link
Fixes #9869
Testing
https://bedrock-stage.gcp.moz.works/ vs. http://localhost:8000/en-GB/
https://dev.tekcopteg.com/en/ vs. http://localhost:8000/en/