[webkit-reviews] review granted: [Bug 179728] CSP: Hide nonce values from the DOM : [Attachment 443596] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 179728: CSP: Hide nonce values from the DOM
https://bugs.webkit.org/show_bug.cgi?id=179728

Attachment 443596: Patch

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




--- Comment #17 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 443596
  --> https://bugs.webkit.org/attachment.cgi?id=443596
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443596&action=review

r=me with a few minor fixes (mostly WebKit coding style).

> Source/WebCore/dom/Element.cpp:340
> +    // Retain previous IDL nonce

nit: WebKit comments need to end with a period, as per coding style.

> Source/WebCore/dom/Element.cpp:341
> +    const AtomString currentNonce = nonce();

nit: I don't think the "const" is really helpful here and we rarely use const
variables in WebKit.

> Source/WebCore/dom/Element.cpp:1896
> +	       if (is<HTMLElement>(this) || is<SVGElement>(this)) {

The following may be a little more efficient as it may avoid a null check:
`if (is<HTMLElement>(*this) || is<SVGElement>(*this))`

> Source/WebCore/dom/Element.cpp:1897
> +		   if (newValue.isNull())

nit: I'd recommend a ternary to make the code more concise:
setNonce(newValue.isNull() ? emptyAtom() : newValue);

> Source/WebCore/dom/Element.h:362
> +    // Used by the HTMLElement, HTMLScriptElement, and SVGElement IDLs

nit: I don't think this comment should still mention HTMLScriptElement.

> Source/WebCore/html/HTMLElement.cpp:505
> +    hideNonce();

Just curious, would this work?
```
hideNonce();
return Element::insertedIntoAncestor(insertionType, containerNode);
```

It would be simpler so I am wondering if there is a good reason to use the
ordering in your patch.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:198
> +    } else if (policyFrom == PolicyFrom::HTTPHeader) {

nit: Per WebKit coding style, no curly brackets for one-liners.

>
LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-misc-expected.t
xt:970
> +FAIL script.nonce: IDL set to "" assert_equals: getAttribute() expected ""
but got "test-valueOf"

I agree the test is wrong here. Would be nice to file a bug against WPT though.