Bug 237440 – CSP report does not get sent to the document in the case of a detached element

• Component: New Bugs
• Status: RESOLVED FIXED

[Kate Cheney]

CSP report does not get sent to the document in the case of a detached element

[Kate Cheney]

Created attachment 453771 [details]
Patch

[Kate Cheney]

rdar://89081463

[Chris Dumez]

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]

(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]

Created attachment 453785 [details]
Patch

[Chris Dumez]

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 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]

(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]

Created attachment 453786 [details]
Patch

[Chris Dumez]

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]

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].