CSP report does not get sent to the document in the case of a detached element
Created attachment 453771 [details] Patch
rdar://89081463
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
(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.
Created attachment 453785 [details] Patch
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.
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.
(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.
Created attachment 453786 [details] Patch
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 :)
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].