[webkit-reviews] review denied: [Bug 237440] CSP report does not get sent to the document in the case of a detached element : [Attachment 453771] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Kate Cheney
<katherine_cheney at apple.com>'s request for review:
Bug 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

Attachment 453771: Patch

https://bugs.webkit.org/attachment.cgi?id=453771&action=review




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 453771
  --> https://bugs.webkit.org/attachment.cgi?id=453771
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(event
Init));

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-docum
ent-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-docum
ent-after-element-has-been-detached.html:9
> +	   testRunner.waitUntilDone();

`jsTestIsAsync = true;` when using js-test.

>
LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-docum
ent-after-element-has-been-detached.html:10
> +	   testRunner.dumpAsText();

Unnecessary when using js-test.

>
LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-docum
ent-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-docum
ent-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-docum
ent-after-element-has-been-detached.html:14
> +	       testRunner.notifyDone();

finishJSTest() when using js-test

>
LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-docum
ent-after-element-has-been-detached.html:17
> +	   inlineScript = document.createElement("script");

let inlineScript = ...?

>
LayoutTests/http/tests/security/contentSecurityPolicy/report-violation-to-docum
ent-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-docum
ent-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