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

nginx: add Content-Security-Policy-Report-Only header to all non-wordpress content sites #57

Merged
merged 3 commits into from
Aug 24, 2024

Conversation

timmywil
Copy link
Member

This PR adds the "Report-Only" version of the CSP header to all content sites, which has no real effect other than to send reports of violations to an API. I've set up a temporary API on cloudflare to accept those reports. We can watch the logs to determine if there are any issues that need to be addressed before switching to real CSP headers.

Fixes gh-54

@timmywil timmywil requested a review from Krinkle August 12, 2024 14:52
@timmywil timmywil self-assigned this Aug 12, 2024
@Krinkle
Copy link
Member

Krinkle commented Aug 12, 2024

I suggest adding unsafe-inline to gruntjs.com and WordPress from the start. gruntjs.com does do other things as well, but I'll work with Grunt folks to see which ones of those we can improve.

Are inline scripts "unsafe" and a major problem for us? At least on static sites, I think inline scripts and styles tend to be idiomatic and don't pose much risk, assuming you do disallow connections of course. (And using nonce tokens is hard to reconcile with a static site.) For sites that want to allow arbitrary connections, I suppose disabling inline scripts can form an interesting compromise, but afaik we don't have such sites at the moment.

WordPress requires and commonly uses inline <style> tags. jQuery API use inline style and inline scripts. For blogs, WordPress admin requires inline scripts.

Eliminating inline tags would make abuse much much harder, but also significantly limits what the web platform can do. For us that might not be worth trying to satisfy and workaround. What do you think?

@Krinkle
Copy link
Member

Krinkle commented Aug 12, 2024

I've set up a temporary API on cloudflare to accept those reports.

What kind of traffic does this accept? Is there a sample rate by default?

Unfortunately, neither CSP nor Report-To header support a sampling rate by default. The NEL (Network Error Logging) spec, which uses the same reporting API, does have sampling. There is w3c/webappsec-csp#227 and w3c/reporting#45 for bringing this to the Reporting spec more generally, so that it can benefit CSP.

The only option we have today is to sample in the policy itself. That is, to conditionally emit the header (or, when enforced, conditonally include the report-to portion). If we need this, we can do that in Nginx using the split_clients directive to randomly assign one of two two strings to a variable, and then add_header using that variable.

@timmywil
Copy link
Member Author

There's no sample rate, but logs are not written to a file by default. You have to live stream logs to view them, which means any logs when not live streaming are disregarded. If the logs are crazy when streaming, we can turn it off and address the most common ones and keep going from there until there are no logs and we've addressed all reports. At that point, we can shut down the API.

That said, if you think it's necessary to build in sampling we can do that. Ideally, we get the header close enough that logs are few even without sampling.

@timmywil
Copy link
Member Author

Perhaps it's best to focus on one site at a time, and build in sampling.

@Krinkle
Copy link
Member

Krinkle commented Aug 13, 2024

@timmywil You could push this to staging only, to get early information without live traffic. That way we can get a glimpse of anything that's easily reproduced by us first.

Once done testing, you can force push production over staging to roll it back (we currently allow force push on the staging branch).

@timmywil
Copy link
Member Author

I figured out a way to test each site locally. I'll document what I find here.

@timmywil
Copy link
Member Author

timmywil commented Aug 15, 2024

Will update as I find more.

The CSP header for the API sites, and all wordpress sites, will not be handled in this PR, but instead set in jquery/jquery-wp-content#463

Site Violation Previous Rule Proposed Rule Update
api.jquery.com 2 Inline SVGs from typesense CSS img-src 'self' img-src 'self' 'data:'
api.jquery.com inline style added in typesense JS style-src 'self' style-src 'self' 'sha256-biLFinpqYMtWHmXfkA1BPeCY0/fNt46SAZ+BBk5YUog=' 'unsafe-hashes'
api.jquery.com content pages inline scripts from demos script-src 'self' script-src 'self' 'unsafe-inline' code.jquery.com
api.jquery.com content pages inline styles from demos style-src 'self' style-src 'self' 'unsafe-inline'

Notes

@timmywil
Copy link
Member Author

Rebased and removed the header for wordpress sites, which will be handled by jquery-wp-content

@timmywil timmywil changed the title nginx: add Content-Security-Policy-Report-Only header to all content sites nginx: add Content-Security-Policy-Report-Only header to all non-wordpress content sites Aug 20, 2024
@@ -51,6 +51,12 @@ server {
include /etc/nginx/fastcgi_params;
}
<%- end -%>

location / {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik location blocks are mutually exclusive, so this means this won't apply to .php URLs.

It seems add_header is valid in server blocks directly as well, if you want to do that for miscweb sites (i.e. above before the first location block). https://nginx.org/en/docs/http/ngx_http_headers_module.html

This is fine to me either way as an incremental start. I am making the assumption that this new catch-all location block will inherit the index and try_files directives from the server block.

Copy link
Member

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of these three, only gruntjs has a staging site. So we can confirm overall catalogue validation and workings of the nginx config snippet there on stage, before prod if you like.

@timmywil timmywil merged commit 83d3101 into jquery:staging Aug 24, 2024
2 checks passed
@timmywil timmywil deleted the csp branch August 24, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Add Content Security Policy headers to all jQuery content sites
2 participants