[webkit-reviews] review denied: [Bug 53867] Disable script elements when a CSP header is present : [Attachment 81409] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 53867: Disable script elements when a CSP header is present
https://bugs.webkit.org/show_bug.cgi?id=53867

Attachment 81409: Patch
https://bugs.webkit.org/attachment.cgi?id=81409&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81409&action=review

> LayoutTests/http/tests/security/contentSecurityPolicy/no-policy.html:7
> +if (window.layoutTestController) {
> +  layoutTestController.dumpAsText();
> +}

No brackets needed.

>
LayoutTests/http/tests/security/contentSecurityPolicy/script-src-in-iframe.html
:9
> +<meta http-equiv="refresh" content="0;
URL=http://localhost:8000/security/contentSecurityPolicy/resources/echo-iframe.
pl?q=http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script-src.
html&csp=allow%20*;%20script-src%20'none'" />

Why a meta refresh?  It would be easier to just open a new window or put the
test in a frame.

> Source/WebCore/dom/ScriptElement.cpp:161
> +    if
(!m_element->document()->contentSecurityPolicy()->allowScriptFromUrl(sourceUrl)
)
> +	   return;

Hum...	I'm not sure this will do the right thing if there's a redirect chain
involved.  This code is just checking the first URL in the redirect chain.  We
want to check the final URL in the chain...

> Source/WebCore/page/ContentSecurityPolicy.cpp:48
> +    if (m_document->parentDocument() == m_document)
> +	   return !m_isEnabled;
> +    return !m_isEnabled &&
m_document->parentDocument()->contentSecurityPolicy()->allowScriptFromUrl(sourc
eUrl);

What is all this business about the parent document?  We shouldn't be talking
to the parent document at all.	Each resource needs to have its own security
policy.

> Source/WebCore/page/ContentSecurityPolicy.h:42
> +    bool allowScriptFromUrl(const String&) const;

allowScriptFromUrl => allowScriptFromURL

> Source/WebCore/page/ContentSecurityPolicy.h:49
>      String m_header;
> +
> +    bool m_isEnabled;
> +
> +    Document* m_document;

A really minor style nit: I'd reverse the order here and remove the blank
lines.	Usually, I like to put the "parent object" first, which, in this case,
is the document.  Then isEnabled is the most important piece of state (because
it controls the behavior of the whole object).