1839165 - Throttle the number of CSP reports that are sent

Status: RESOLVED FIXED

(Core :: DOM: Security, task)

Description

[Tom Schuster]

Coealising of reports like bug 1657519 suggests is much harder to implement, and for cases like Bug 1806276 (yahoo) we would need to do inexact matching, because every report has a different source "0.5", "-2.2" etc.

We probably need to tune our constants a bit, but a maximum of 100 reports in 2 seconds makes the yahoo page load without slowing down my browser to a crawl.

Comment 1

[Tom Schuster]

Attachment: Bug 1839165 - Throttle the number of CSP reports that are send. r?freddyb!

Comment 2

[Tom Schuster]

Attachment: Bug 1839165 - Test for too many CSP reports. r?freddyb!

Depends on D181392

Comment 3

[Tom Schuster]

Attachment: WIP: Bug 1839165 - WIP: Telemetry for number of CSP reports. (not working)

Depends on D181393

Comment 4

[Frederik Braun [:freddy]]

Can we dupe bug 1657519 against this?

Comment 5

[Tom Schuster]

(In reply to Frederik Braun [:freddy] from comment #4)

Can we dupe bug 1657519 against this?

I don't think we should. We probably want to do something like bug 1657519 suggests (maybe in combination with the reporting API?) in the long run.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ab35b57969d
Throttle the number of CSP reports that are send. r=freddyb
https://hg.mozilla.org/integration/autoland/rev/5e73732114f1
Test for too many CSP reports. r=freddyb,devtools-reviewers,nchevobbe

Comment 7

[Norisz Fay [:noriszfay]]

Backed out for causing failures on test_ext_contentscript_triggeringPrincipal.js

Backout link

Push with failures

Failure log

TV Failure also present here

Comment 8

[Tom Schuster]

Hi Nicolas! Do you have any idea why the test continues to time out? I left some references in phabricator.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: CleanShot 2023-07-20 at 17.33.36.png

Sorry for the delay Tom.
So looking at the logs, it does look like we do find the messages that we want. Maybe one of them could be displayed multiple times?

waitForMessagesByType (which is called by waitForMessageByType) will only resolve if there's only 1 matched messages.

https://searchfox.org/mozilla-central/rev/8d43262674d6c6d469b821cca579b1240ebb42a5/devtools/client/webconsole/test/browser/head.js#216-217,220,226-230

    info(
      `Matched a message with text: "${message.text}", ` +
...
          : `all messages received.`)
...
if (matchedMessages.length === messages.length) {
  hud.ui.off("new-messages", messagesReceived);
  resolve(matchedMessages);
  return;
}

If I wait at the end of the test, I can see that the "blocked" message can be shown twice, and depending on the timing, it might be the case that we have the 2 messages.

Maybe you can try to switch to a polling helper instead:

diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_csp_too_many_reports.js b/devtools/client/webconsole/test/browser/browser_webconsole_csp_too_many_reports.js
--- a/devtools/client/webconsole/test/browser/browser_webconsole_csp_too_many_reports.js
+++ b/devtools/client/webconsole/test/browser/browser_webconsole_csp_too_many_reports.js
@@ -27,11 +27,6 @@ add_task(async function () {
 
   const hud = await openNewTabAndConsole(TEST_URI);
 
-  const onCspViolationMessage = waitForMessageByType(
-    hud,
-    CSP_VIOLATION_MSG,
-    ".error"
-  );
   const onCspTooManyReportsMessage = waitForMessageByType(
     hud,
     CSP_TOO_MANY_REPORTS_MSG,
@@ -41,9 +36,8 @@ add_task(async function () {
   info("Load a page with CSP warnings.");
   await navigateTo(TEST_VIOLATIONS);
 
-  await onCspViolationMessage;
+  await waitFor(() => findMessageByType(hud, CSP_VIOLATION_MSG, ".error"));
   await onCspTooManyReportsMessage;
   ok(true, "Got error about too many reports");
-
   await clearOutput(hud);
 });

Let me know if this helps!

Comment 10

[Tom Schuster]

Thank you Nicolas. I didn't realize that this only matches one message. I think your suggestion works, because test-verify now seems green on try.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8417e4f779aa
Throttle the number of CSP reports that are send. r=freddyb
https://hg.mozilla.org/integration/autoland/rev/b1a1263e67ce
Test for too many CSP reports. r=freddyb,devtools-reviewers,nchevobbe

Comment 12

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8417e4f779aa
https://hg.mozilla.org/mozilla-central/rev/b1a1263e67ce

Comment 13

[Phabricator Automation]

Comment on attachment 9340735 [details]
WIP: Bug 1839165 - WIP: Telemetry for number of CSP reports. (not working)

Revision D181897 was moved to bug 1848303. Setting attachment 9340735 [details] to obsolete.