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
Support CSP reporting for a configurable proportion of page requests #14453
base: main
Are you sure you want to change the base?
Support CSP reporting for a configurable proportion of page requests #14453
Conversation
…viour is off/no reporting
… a CSP header - this is handy for local testing and also testing on demo
@pmac @robhudson Do you think this'll have any significant downside (eg performance or CDN-related pain points)? I don't think so, but happy to be corrected |
@stephaniehobson @alexgibson I'm not sure if you'll be able to see the PR for the extra env var set in the infra repo, so to check here instead:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14453 +/- ##
=======================================
Coverage 75.72% 75.72%
=======================================
Files 144 144
Lines 7876 7877 +1
=======================================
+ Hits 5964 5965 +1
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.
🕸️ 🚫
except IndexError: | ||
sample_size = 100 | ||
|
||
check_for_report_uri(url, sample_size) |
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.
We have click in our requirements already b/c of glean-parser. We could make it a main dependency instead and use that here to avoid some of this boilerplate for arguments.
"CSP_REPORT_PERCENTAGE", | ||
default="0", | ||
parser=float, | ||
) |
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.
Maybe include the expected range of the value here? 0.0 to 1.0
?
I'm very happy with 0.5%. I think you could go even lower initially if you wanted. |
Note that to avoid this triggering loads of unnecessary false positives during integration tests (which hit the origin hostnames, not the CDNed hostname) we'll need to update the CSP_DEFAULT_SRC value in our infra config. Handily, #14466 means that extending that one item will now also cover |
@stephaniehobson I've taken it down to 0.2% for initial try-out on Prod |
One-line summary
This changeset adds support for including a CSP report-uri in a certain percentage of pages, so that we can get an idea of whether pages have inappropriate CSP set, but without saturating our logging service, Sentry.
NB: the relevant config to enable this on Dev, Stage and Prod will be in a separate PR, and that work needs to be merged to make this changeset do anything.
Issue / Bugzilla link
Resolves #14451
Testing
I've added a script called
seek_csp_report_uri.py
which we can use to check the proportion of headers being set, either locally or on an actual server.Here's some testing I've done with it:
With
CSP_REPORT_PERCENTAGE
not set as an env varWith
CSP_REPORT_PERCENTAGE
set to 0.005 (which is what we'd start with in production)On mozorg-demo-8 with
CSP_REPORT_PERCENTAGE
set to 0.75:On pocket-demo-4 with
CSP_REPORT_PERCENTAGE
not set:If you want to do the same
kent-server
(a fake Sentry from @willkg) withpipx install kent
) and run it in it own shell:kent-server run -p 8011
.env
file set:./bin/manual_qa/seek_csp_report_uri.py http://localhost:8000/en-US/ 200
and look at the results