[webkit-reviews] review denied: [Bug 54551] disable execution of inline scripts when a content security policy is present : [Attachment 82626] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 54551: disable execution of inline scripts when a content security policy
is present
https://bugs.webkit.org/show_bug.cgi?id=54551

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

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:468
> +bool HTMLDocumentParser::shouldRunInlineScripts()

I see that you've patterned this after shouldLoadExternalScriptFromSrc, but
shouldLoadExternalScriptFromSrc is very wrong.	It's nowhere need near the only
way to run external scripts.

> Source/WebCore/html/parser/HTMLDocumentParser.h:110
> +    virtual bool shouldRunInlineScripts();

This should be a method on ContentSecurityPolicy.  ContentSecurityPolicy should
encapsulate the semantics of the policy.  I'd call it something like
ContentSecurityPolicy::allowInlineScripts()

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:307
> +	   } else if (m_host->shouldRunInlineScripts()) {

This is the wrong layer.  We should do this work in ScriptController.  We
probably need to teach script controller some things it doesn't know yet.  It's
probably easier to do JavaScript URLs first because ScriptController has a
pretty good handle on whether it's trying to execute a JavaScript URL.

> Source/WebCore/page/ContentSecurityPolicy.h:42
> +    bool hasPolicy() const { return m_havePolicy; }

This should be private.