WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.45 KB, patch)
2022-03-03 14:09 PST
,
Kate Cheney
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(6.08 KB, patch)
2022-03-03 14:32 PST
,
Kate Cheney
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Kate Cheney
Comment 1
2022-03-03 11:53:46 PST
Created
attachment 453771
[details]
Patch
Kate Cheney
Comment 2
2022-03-03 11:57:52 PST
rdar://89081463
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
Created
attachment 453785
[details]
Patch
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
Created
attachment 453786
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug