1922967 - No CSP violation report sent when both report-uri and report-to directives are used, but Reporting API is disabled

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Daniel Matetaka]

Steps to reproduce:

1. Ensure experimental feature (Reporting API Support for CSP Violations) is disabled.
2. Visit a website (under your control) with a CSP that provides endpoints for both the report-uri and report-to directives.
3. Perform an action that will be blocked by the CSP.

Actual results:

A CSP violation will not reported on sites with CSPs containing both the report-to and report-uri directives.

Expected results:

If the experimental feature ((Reporting API Support for CSP Violations)) is disabled, then a CSP violation should be reported via report-uri, like previous versions of Firefox.

Comment 1

[Daniel Veditz [:dveditz]]

How are you disabling "Reporting API Support for CSP Violations"? The only control I found was the "dom.reporting.enabled" pref that disables the Reporting API entirely.

Assuming there's only that one global switch I'm not sure this is the wrong behavior: The CSP spec says report-to obsoletes report-uri (ignore the latter if the former is present) if it's supported. We support report-to so CSP now ignores report-uri. Separately from that, the user has flipped a pref saying "don't send out reports" and we're apparently honoring that? If you see the pref that way then you could say it was a bug that the pref didn't also disable CSP report-uri. But of course that's not why we have that pref, it's there so we could turn it ON to test an incomplete feature.

I'm unfamiliar with this code so I may be missing something. I could only find the pref used to disable some webidl but it would seem like there would have to be more involved than just that.

Comment 2

[Daniel Veditz [:dveditz]]

"dom.reporting.enabled" defaults to "false"?! So we shipped "support" for report-to in bug 1391243 that in effect disabled all reporting? That's much worse than what I thought you were reporting ("step 1: disable reporting").

Sean: This sounds like a quick fix, or would it be safer/simpler to back out 1391243 until we make sure the tests actually test the way we ship this?

Comment 3

[Daniel Matetaka]

(In reply to Daniel Veditz [:dveditz] from comment #2)

"dom.reporting.enabled" defaults to "false"?! So we shipped "support" for report-to in bug 1391243 that in effect disabled all reporting? That's much worse than what I thought you were reporting ("step 1: disable reporting").

Sean: This sounds like a quick fix, or would it be safer/simpler to back out 1391243 until we make sure the tests actually test the way we ship this?

Yeah, I should've mentioned the experimental feature is disabled by default, and requires end-users to explicitly enable the feature.

If the experimental feature is not enabled, then Firefox (130) effectively reports nothing to sites with CSPs containing both reporting directives.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 5

[Sean Feng [:sefeng]]

I think I see it in the code. If report-to is provided, we always use it https://searchfox.org/mozilla-central/rev/0cd8e314881fc6c216586d1adc13eb8a03f5d040/dom/security/nsCSPContext.cpp#1174-1175, which should only happen when reporting API is enabled.

I'll land a patch to fix this, so that we don't need to back bug 1391243 out.

Comment 6

[Sean Feng [:sefeng]]

Attachment: Bug 1922967 - Make `report-uri` directive is alwasy used for CSP when Reporting API is disabled r=dveditz

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd18aefcc523
Make `report-uri` directive is alwasy used for CSP when Reporting API is disabled r=dveditz

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/dd18aefcc523

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sefeng, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox132 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Sean Feng [:sefeng]]

Comment on attachment 9430393 [details]
Bug 1922967 - Make report-uri directive is alwasy used for CSP when Reporting API is disabled r=dveditz

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: This is an regression to site authors. They may not be able to receive CSP reports if they configure it in such a way. So I won't claim it's super urgent given it has no impact to our users, however it's kinda important for site authors.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch itself is trivial.
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9430393 [details]
Bug 1922967 - Make report-uri directive is alwasy used for CSP when Reporting API is disabled r=dveditz

Approved for 132.0b9.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/57709292334e