Closed Bug 847081 Opened 7 years ago Closed 6 years ago
CSP: Throw a warning when a '*-report-only' header doesn't contain a 'report-uri' directive
(A mirror of webkit bug https://bugs.webkit.org/show_bug.cgi?id=111208 , inspired by Ashar Javed and Mike West) I am lazy, so I will just copy&paste the Description from Mike West: "Apparently people set report-only headers without a report URI, as evidenced by https://twitter.com/soaj1664ashar/status/307450782704365569. We should help them understand that they've basically created an expensive no-op machine."
Feedback on the warning msg? I say non-Report-Only header since the fix is used in both the prefixed and unprefixed headers. Try is green: https://tbpl.mozilla.org/?tree=Try&rev=4a2e39bf5e2f Thank you.
Attachment #8365642 - Flags: review?(sstamm)
Component: Developer Tools: Console → Security
Product: Firefox → Core
Attachment #8365642 - Flags: review?(sstamm) → review-
Matej: can you help with words here? We're trying to warn developers when they use a Report-Only CSP header (means tell me when the page violates my policy, but don't protect my page) without specifying a report-uri (where Firefox will send info when the policy gets violated). I made a suggestion above, but you know words better than I do.
Do we have to repeat "violations of this policy" twice? Here's what I would recommend: """ # %1$S is the site with the Report-Only HTTP header This site (%1$S) has a Report-Only policy without a report URI. CSP will not block and cannot report violations of this policy. """ The more detailed version looks fine as is.
1. Should I make a separate test for X-CSP-RO? 2. Saying "CSP will not block" in the context of missing report-uri directive sounds like "to enforce the policy you should add report-uri". I supposed if we mix the simple and verbose version: This site (%1$S) has a Report-Only policy without a report URI. To receive violation reports in a Report-Only policy, use the report-uri directive. To block violations, use the Content-Security-Policy header. 3. Should I use selfUri.host or selfUri.hostPort? The latter has port appended which seems to be more accurate (what if the site has multiple apps on different ports?) Maybe an edge case... Thanks.
Comment on attachment 8366258 [details] [diff] [review] report_uri_warning_v2.patch Review of attachment 8366258 [details] [diff] [review]: ----------------------------------------------------------------- r=me assuming all the CSP tests pass locally. (In reply to Yeuk Hon Wong [:yeukhon] from comment #6) > Created attachment 8366258 [details] [diff] [review] > report_uri_warning_v2.patch > > 1. Should I make a separate test for X-CSP-RO? No. We're deprecating the X- headers soon, and would be deleting the test. > > 2. Saying "CSP will not block" in the context of missing report-uri > directive sounds like "to enforce the policy you should add report-uri". I > supposed if we mix the simple and verbose version: <snip> Nah. Just add the "more detailed" sentence to the one in this patch and call it good. > 3. Should I use selfUri.host or selfUri.hostPort? The latter has port > appended which seems to be more accurate (what if the site has multiple apps > on different ports?) Maybe an edge case... Use prePath (will include scheme/host/port). ::: dom/locales/en-US/chrome/security/csp.properties @@ +29,5 @@ > # %1$S is the ETLD of the report URI that is not HTTP or HTTPS > reportURInotHttpsOrHttp = The report URI (%1$) should be an HTTP or HTTPS URI. > +# LOCALIZATION NOTE (reportURInotInReportOnlyHeader): > +# %1$S is the host of the page with the policy > +reportURInotInReportOnlyHeader = This site (%1$S) has a Report-Only policy without a report URI. CSP will not block and cannot report violations of this policy. Is the more detailed info we discussed in comment 4 helpful to developers? If so, feel free to append it here.
Attachment #8366258 - Flags: review?(sstamm) → review+
Which test case in particular is causing the exception?
My patch will fail any tests that tests XCSP without allow / default-src because selfUri. This means if you ever access a website whose XCSP does not have allow/default-src, Fx will crash. For example: https://bugzilla.mozilla.org/show_bug.cgi?id=916446 I can add an additional check, but I don't understand why it will continue after a return.
I don't think it actually continues after the return, rather a recursive call to fromString that doesn't hit that early return. The XCSP header requires allow/default-src to be present, and the parser replaces the policy with "default-src 'none'" if it's not. That's a recursive call to fromString, so what's probably happening is that the first call *does* return before your code, but the inner/second call does not. It actually creates a policy but without selfUri set. You could pass selfUri, but it's not required by fromString, so you should probably do the check anyway. CSPRep.fromString assumes that only calls to other functions need selfUri, so if you're accessing one of its members you need the check. Should be a simple change: 750 cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotInReportOnlyHeader", [selfUri ? selfUri.host : "undefined"]));
Added the addition checkin. Verified XCSP can pass as well. Try now looks good: https://tbpl.mozilla.org/?tree=Try&rev=9c6048e37168 Thank you Sid.
Comment on attachment 8375188 [details] [diff] [review] report_uri_warning_v3.patch Carrying over r+ from Sid.
Attachment #8375188 - Flags: review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.