Bug 230728 – Send CSP violation reports to the DOM window

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

Many imported CSP tests are timing out because they wait for security violation reports using a window event listener that are never sent.

[Kate Cheney]

Created attachment 439103 [details]
Patch

[Brent Fulgham]

Comment on attachment 439103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439103&action=review

> Source/WebCore/dom/Document.cpp:6728
> +    queueTaskToDispatchEventOnWindow(TaskSource::DOMManipulation, SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes));

Does the spec state whether the events should be sent to the Window, the Document, or both? Or is there some kind of bubbling rule where we start with the document, then bubble to the Window or something?

[Kate Cheney]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 439103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439103&action=review
> 
> > Source/WebCore/dom/Document.cpp:6728
> > +    queueTaskToDispatchEventOnWindow(TaskSource::DOMManipulation, SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes));
> 
> Does the spec state whether the events should be sent to the Window, the
> Document, or both? Or is there some kind of bubbling rule where we start
> with the document, then bubble to the Window or something?

That would probably be a good idea. Here's what it says:

Given a violation (violation), this algorithm reports it to the endpoint specified in violation’s policy, and fires a SecurityPolicyViolationEvent at violation’s element, or at violation’s global object as described below:

1. Let global be violation’s global object.
2. Let target be violation’s element.

Queue a task to run the following steps:

1. If target is not null, and global is a Window, and target’s shadow-including root is not global’s associated Document, set target to null.


If target is null:

1. Set target be violation’s global object.
2. If target is a Window, set target to target’s associated Document.


So it seems like I need a few more tweaks.

[Kate Cheney]

Created attachment 439202 [details]
Patch

[Brent Fulgham]

Comment on attachment 439202 [details]
Patch

r=me. So nice to see these timeouts resolved!

[Kate Cheney]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 439202 [details]
> Patch
> 
> r=me. So nice to see these timeouts resolved!

Thanks, I'll wait for EWS to make sure I didn't miss any expectations.

[Brent Fulgham]

Windows might have its own result expectations.

[Brent Fulgham]

It looks like the test failures are specific to WebKitLegacy.

[Kate Cheney]

Created attachment 439264 [details]
Patch

[Kate Cheney]

Created attachment 439265 [details]
Patch

[Kate Cheney]

(In reply to Kate Cheney from comment #10)
> Created attachment 439265 [details]
> Patch

Ok I think they should be fixed now. One test was skipped on macOS so my local tests missed it but it still runs on macwk1 and win. Another has iOS-specific expectations that I updated, and the last one was failing on macOS and iOS but with different error messages, so I skipped that one because I thought that was better than making a platform-specific expected text file for a failing test.

[Kate Cheney]

Created attachment 439275 [details]
Patch

[Kate Cheney]

Created attachment 439294 [details]
Patch

[Brent Fulgham]

Comment on attachment 439294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439294&action=review

R=me

> LayoutTests/TestExpectations:-938
> -imported/w3c/web-platform-tests/content-security-policy/reporting/report-original-url.sub.html [ Skip ]

This makes me so happy!

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt:8
> +{"csp-report":{"document-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.py%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","referrer":"http://127.0.0.1:8000/","violated-directive":"frame-ancestors","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.py?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html","blocked-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.py%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","status-code":200}}

Out of curiosity: Does this revised format with only the name now match the spec? Seems silly that we failed some tests just for that small fix (wish I had looked at it before)!

So great to see all of these I skipped tests.

[Brent Fulgham]

“I skipped” —> unskipped

[Kate Cheney]

(In reply to Brent Fulgham from comment #14)
> Comment on attachment 439294 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439294&action=review
> 
> R=me
> 
> > LayoutTests/TestExpectations:-938
> > -imported/w3c/web-platform-tests/content-security-policy/reporting/report-original-url.sub.html [ Skip ]
> 
> This makes me so happy!
> 
> > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt:8
> > +{"csp-report":{"document-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.py%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","referrer":"http://127.0.0.1:8000/","violated-directive":"frame-ancestors","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.py?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html","blocked-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.py%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","status-code":200}}
> 
> Out of curiosity: Does this revised format with only the name now match the
> spec? Seems silly that we failed some tests just for that small fix (wish I
> had looked at it before)!
> 

The spec is a bit vague, it says the violatedDirective should be "a non-empty string representing the directive whose enforcement caused the violation." This seems like it could refer either to the violated directive name only, or also the values. But to get imported tests to pass, it needs to be the name only, and this also matches Chrome's behavior from my testing. So I think this is a correct change to make.

> So great to see all of these I skipped tests.

[EWS]

Committed r283111 (242169@main): <https://commits.webkit.org/242169@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439294 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83574710>