[webkit-reviews] review denied: [Bug 89577] Implement the script-nonce Content Security Policy directive. : [Attachment 148588] WIP

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 89577: Implement the script-nonce Content Security Policy directive.
https://bugs.webkit.org/show_bug.cgi?id=89577

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

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


> Source/WebCore/html/HTMLAttributeNames.in:161
> +nonce

We should add this to HTMLScriptElement.idl (behind the ENABLE flag)

>> Source/WebCore/page/ContentSecurityPolicy.cpp:763
>> +	return checkNonceAndReportViolation(nonce, consoleMessage, contextURL,
contextLine);
> 
> I'm not sure this is correct: we only check `script-src` iff script-nonce
isn't set. This means that `script-src 'self'; script-nonce myawesomenonce`
allows script to run on the page. I _think_ this is correct behavior, but I'm
not sure if the spec's language precludes it. In particular, I'm not sure if
"Whenever the user agent would execute a script from a script element" means
"If CSP would otherwise allow a script element to execute" or "If there's a
script element on the page that the browser understands".
> 
> Adam? :)

I had in mind that you'd need to satisfy both script-src and script-nonce
independently.	So, script-src 'self'; script-nonce awesome would only allow a
script of it's URL both from a URL that's same-origin with 'self' and had an
appropriate nonce.  Maybe that's too restrictive?

> Source/WebCore/page/ContentSecurityPolicy.cpp:923
> +    if (!m_scriptNonce.isEmpty()) {

I wonder if we should distinguish between isNull and isEmpty.  We could use
isNull to represent "no script-nonce directive" and isEmpty to represent "a
script-nonce directive with an empty value".  That's basically what we do with
HTML attributes.

> Source/WebCore/page/ContentSecurityPolicy.h:71
> -    bool allowInlineScript(const String& contextURL, const
WTF::OrdinalNumber& contextLine) const;
> +    bool allowInlineScript(const String& nonce, const String& contextURL,
const WTF::OrdinalNumber& contextLine) const;

The script-nonce directive is supposed to apply to out-of-line script too.