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

Fix fonts CSP for staging #14466

Merged
merged 2 commits into from Apr 16, 2024
Merged

Fix fonts CSP for staging #14466

merged 2 commits into from Apr 16, 2024

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Apr 16, 2024

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:

  • mozorg:
    • dev: font-src 'self';
    • prod: font-src 'self';
  • pocket:
    • dev: font-src 'self' assets.getpocket.com;
    • prod: font-src 'self' assets.getpocket.com;

after:

  • mozorg:
    • dev: font-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com *.allizom.org;
    • prod: font-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com;
  • pocket:
    • dev: font-src 'self' *.getpocket.com *.allizom.org;
    • prod: font-src 'self' *.getpocket.com;

(basically leaving out font-src completely and falling back to default-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/

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 75.72%. Comparing base (75d3291) to head (616afaa).
Report is 1 commits behind head on main.

Files Patch % Lines
bedrock/settings/__init__.py 66.66% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@stevejalim stevejalim left a 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!

@stevejalim stevejalim merged commit 74779c3 into mozilla:main Apr 16, 2024
5 checks passed
stephaniehobson pushed a commit that referenced this pull request Apr 17, 2024
* Fix font-src for stage hosts

* Simplify font CSP to defaults

---------

Co-authored-by: Jan Brasna <janbrasna@users.noreply.github.com>
@janbrasna janbrasna deleted the patch-2 branch April 17, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSP errors loading fonts on stage deployments
3 participants