Summary: | Send CSP violation reports to the DOM window | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kangil.han, mkwst, philipj, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Kate Cheney
2021-09-23 15:59:04 PDT
Created attachment 439103 [details]
Patch
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? (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. Created attachment 439202 [details]
Patch
Comment on attachment 439202 [details]
Patch
r=me. So nice to see these timeouts resolved!
(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. Windows might have its own result expectations. It looks like the test failures are specific to WebKitLegacy. Created attachment 439264 [details]
Patch
Created attachment 439265 [details]
Patch
(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. Created attachment 439275 [details]
Patch
Created attachment 439294 [details]
Patch
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. “I skipped” —> unskipped (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. 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]. |