RESOLVED FIXED 237440
CSP report does not get sent to the document in the case of a detached element
https://bugs.webkit.org/show_bug.cgi?id=237440
Summary CSP report does not get sent to the document in the case of a detached element
Kate Cheney
Reported 2022-03-03 11:51:37 PST
CSP report does not get sent to the document in the case of a detached element
Attachments
Patch (7.49 KB, patch)
2022-03-03 11:53 PST, Kate Cheney
no flags
Patch (6.45 KB, patch)
2022-03-03 14:09 PST, Kate Cheney
ews-feeder: commit-queue-
Patch (6.08 KB, patch)
2022-03-03 14:32 PST, Kate Cheney
no flags
Kate Cheney
Comment 1 2022-03-03 11:53:46 PST
Kate Cheney
Comment 2 2022-03-03 11:57:52 PST
Chris Dumez
Comment 3 2022-03-03 12:02:24 PST
Comment on attachment 453771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453771&action=review > Source/WebCore/dom/Element.cpp:3255 > + document().eventLoop().queueTask(TaskSource::DOMManipulation, [this, protectedThis = Ref { *this }, &contentSecurityPolicyDocument, eventInit = WTFMove(eventInit)] () mutable { This is a security bug waiting to happen. What guarantees that contentSecurityPolicyDocument is still alive by the time that the task runs asynchronously. > Source/WebCore/dom/Element.cpp:3257 > + contentSecurityPolicyDocument.enqueueSecurityPolicyViolationEvent(WTFMove(eventInit)); Why do we need contentSecurityPolicyDocument? Why can't we simply use this->document()? A disconnected Node/Element still has a Document. > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:824 > + if (element && element->isConnected() && element->document() == document) Do we really need this check here now the Element::enqueueSecurityPolicyViolationEvent() does the forwarding to the Document by itself? Can't we just call `element->enqueueSecurityPolicyViolationEvent(WTFMove(violationEventInit));` here no matter what? > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:5 > + <script src="/js-test-resources/js-test-pre.js"></script> In modern tests, please use js-test.js and no js-test-post.js at the end of the test. > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:9 > + testRunner.waitUntilDone(); `jsTestIsAsync = true;` when using js-test. > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:10 > + testRunner.dumpAsText(); Unnecessary when using js-test. > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:11 > + Please add a description("my description...") call here with a suitable description. It will make the test output look much nicer. > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:13 > + testPassed("Successfully received violation event"); May be good to validate e.composed since it seems you tweaked that in the same patch? > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:14 > + testRunner.notifyDone(); finishJSTest() when using js-test > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:17 > + inlineScript = document.createElement("script"); let inlineScript = ...? > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:22 > +</script> What's this </script> doing here? Doesn't seem to match a <script>. > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:23 > +<script src="/js-test-resources/js-test-post.js"></script> Not necessary if you use the modern js-test.js
Kate Cheney
Comment 4 2022-03-03 12:58:04 PST
(In reply to Chris Dumez from comment #3) > Comment on attachment 453771 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453771&action=review > > > Source/WebCore/dom/Element.cpp:3255 > > + document().eventLoop().queueTask(TaskSource::DOMManipulation, [this, protectedThis = Ref { *this }, &contentSecurityPolicyDocument, eventInit = WTFMove(eventInit)] () mutable { > > This is a security bug waiting to happen. What guarantees that > contentSecurityPolicyDocument is still alive by the time that the task runs > asynchronously. > ouch, yeah true. I don't think I need this anyways based on your next comment. > > Source/WebCore/dom/Element.cpp:3257 > > + contentSecurityPolicyDocument.enqueueSecurityPolicyViolationEvent(WTFMove(eventInit)); > > Why do we need contentSecurityPolicyDocument? Why can't we simply use > this->document()? > > A disconnected Node/Element still has a Document. > Oh, that makes it simpler. I assumed a disconnected element would not have access to the same document. > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:824 > > + if (element && element->isConnected() && element->document() == document) > > Do we really need this check here now the > Element::enqueueSecurityPolicyViolationEvent() does the forwarding to the > Document by itself? > > Can't we just call > `element->enqueueSecurityPolicyViolationEvent(WTFMove(violationEventInit));` > here no matter what? > I think we can remove the isConnected() check because we do it later, but should keep the others checks because the element might be null or have the wrong document. Not sure if that's what you meant. > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:5 > > + <script src="/js-test-resources/js-test-pre.js"></script> > > In modern tests, please use js-test.js and no js-test-post.js at the end of > the test. > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:9 > > + testRunner.waitUntilDone(); > > `jsTestIsAsync = true;` when using js-test. > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:10 > > + testRunner.dumpAsText(); > > Unnecessary when using js-test. > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:11 > > + > > Please add a description("my description...") call here with a suitable > description. It will make the test output look much nicer. > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:13 > > + testPassed("Successfully received violation event"); > > May be good to validate e.composed since it seems you tweaked that in the > same patch? > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:14 > > + testRunner.notifyDone(); > > finishJSTest() when using js-test > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:17 > > + inlineScript = document.createElement("script"); > > let inlineScript = ...? > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:22 > > +</script> > > What's this </script> doing here? Doesn't seem to match a <script>. > > > LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-document-after-element-has-been-detached.html:23 > > +<script src="/js-test-resources/js-test-post.js"></script> > > Not necessary if you use the modern js-test.js Will fix all of these test changes.
Kate Cheney
Comment 5 2022-03-03 14:09:10 PST
Chris Dumez
Comment 6 2022-03-03 14:22:34 PST
Comment on attachment 453785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453785&action=review r=me with suggestions. > Source/WebCore/dom/Element.cpp:3257 > + document().enqueueSecurityPolicyViolationEvent(WTFMove(eventInit)); can we just do this here? document().dispatchEvent(SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes)); We've already queued a task to get here. Seems odd to re-queue again inside enqueueSecurityPolicyViolationEvent(). > Source/WebCore/dom/Element.cpp:3259 > + auto event = SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes); nit: I don't think this local variable is very useful. I personally feel it would look nicer if we just did this on one line without curly brackets.
Kate Cheney
Comment 7 2022-03-03 14:26:12 PST
Comment on attachment 453785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453785&action=review thanks. I accidentally removed the mutable keyword from the lambda which is causing EWS failures. Will fix that too. >> Source/WebCore/dom/Element.cpp:3257 >> + document().enqueueSecurityPolicyViolationEvent(WTFMove(eventInit)); > > can we just do this here? > document().dispatchEvent(SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes)); > > We've already queued a task to get here. Seems odd to re-queue again inside enqueueSecurityPolicyViolationEvent(). Oh, yes. Will fix. >> Source/WebCore/dom/Element.cpp:3259 >> + auto event = SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes); > > nit: I don't think this local variable is very useful. I personally feel it would look nicer if we just did this on one line without curly brackets. agree, will remove.
Kate Cheney
Comment 8 2022-03-03 14:28:25 PST
(In reply to Kate Cheney from comment #7) > Comment on attachment 453785 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453785&action=review > > thanks. I accidentally removed the mutable keyword from the lambda which is > causing EWS failures. Will fix that too. > > >> Source/WebCore/dom/Element.cpp:3257 > >> + document().enqueueSecurityPolicyViolationEvent(WTFMove(eventInit)); > > > > can we just do this here? > > document().dispatchEvent(SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, WTFMove(eventInit), Event::IsTrusted::Yes)); > > > > We've already queued a task to get here. Seems odd to re-queue again inside enqueueSecurityPolicyViolationEvent(). > > Oh, yes. Will fix. > Or, actually I can create the event in the lambda capture again to avoid having to duplicate the creation.
Kate Cheney
Comment 9 2022-03-03 14:32:52 PST
Chris Dumez
Comment 10 2022-03-03 14:33:47 PST
Comment on attachment 453786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453786&action=review > Source/WebCore/dom/Element.cpp:3256 > + if (!isConnected()) Well, that ended up looking quite nice :)
EWS
Comment 11 2022-03-04 07:12:53 PST
Committed r290830 (248066@main): <https://commits.webkit.org/248066@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453786 [details].
Note You need to log in before you can comment on or make changes to this bug.