mozilla / bedrock

Making mozilla.org awesome, one pebble at a time
https://www.mozilla.org
Mozilla Public License 2.0
1.18k stars 919 forks source link

CSP needs a tweak? #15546

Closed stevejalim closed 36 minutes ago

stevejalim commented 8 hours ago

This Sentry alert suggests that we need to allow media on https://www.mozilla.org/de/firefox/built-for-you/?v=2

Also, in Devtools, I see this message Content-Security-Policy: This site (https://www.mozilla.org/) has a Report-Only policy without a report-uri directive nor a report-to directive. CSP will not block and cannot report violations of this policy. which is a surprise - has some config slipped somewhere?


Success Criteria

alexgibson commented 7 hours ago

The specific error is Content-Security-Policy: (Report-Only policy) The page’s settings would block the loading of a resource (media-src) at https://assets.mozilla.net/video/built-for-you/video-superhero-de.webm because it violates the following directive: “default-src 'self'”

So it sounds like we need to allow https://assets.mozilla.net/ as a media-src

janbrasna commented 6 hours ago

Ahem, that's from yesterday: trying out tightening undefined defaults #14897

(Is it too noisy to wait if something else crops up, or needs addressing asap? EDIT: 🐛 fixing in RO right away in #15548)

It would have seemed any report-only violation would end up in a separate Sentry project, but this feels like the missing reporting endpoint makes it trigger inside the main project, which is somewhat unfortunate for a report-only testdrive… 🙉

Also, in Devtools, I see this message Content-Security-Policy: This site has a Report-Only policy without a report-uri directive nor a report-to directive. which is a surprise - has some config slipped somewhere?

It should not send RO if no reporting endpoint is set:

https://github.com/mozilla/bedrock/blob/a70a4933dfa374501480e7b0a42187cbafe51db0/bedrock/settings/__init__.py#L101

https://github.com/mozilla/bedrock/blob/a70a4933dfa374501480e7b0a42187cbafe51db0/bedrock/settings/__init__.py#L120-L125

so this sounds like a bug?

janbrasna commented 5 hours ago

Thinking aloud: Isn't it that the report-uri endpoint is set, the RO policy constructed… but then at some later point the reporting percentage middleware removes the report-url but keeps the rest of the header intact? @mozilla/django-csp: contrib/rate_limiting.py

It would still be worth keeping the RO header if DEV even where it has nowhere to report to; but in prod the RO should be removed completely if it's outside of rate limiting — because at that point it seems to defy the whole idea of rate limiting when ending up in the main Sentry anyways… /cc @robhudson

robhudson commented 59 minutes ago

The missing report-uri for the report-only header doesn't trigger any Sentry error to be sent by the way. It's just a console warning.

I have wondered before if we should strip the whole header if the header is the report-only header because what's the point of a report-only header without a report-uri? But the rate limiting middleware should keep the enforced header regardless since that's enforced at the browser level.

robhudson commented 36 minutes ago

With those, this issue can be closed.