[webkit-reviews] review denied: [Bug 116508] Implementation of W3C compliant CSP script-src nonce : [Attachment 239319] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 116508: Implementation of W3C compliant CSP script-src nonce
https://bugs.webkit.org/show_bug.cgi?id=116508

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239319&action=review


Doesn't build and needs a bit of work.

> Source/WebCore/page/ContentSecurityPolicy.cpp:107
> +bool isBase64EncodedCharacter(UChar c)

Please mark this function static to give it internal linkage.

> Source/WebCore/page/ContentSecurityPolicy.cpp:623
> +    if (!equalIgnoringCase(noncePrefix.characters8(), begin,
noncePrefix.length()))
> +	   return true;

This code is incorrect. We can’t call characters8() on a string without first
checking if it’s 8 bit or not. Instead this should call the String::startsWith
function. Unfortunately I see some mistakes in the implementation of that
function that probably need to be fixed.

I think there’s a second mistake here. I don't see any code checking that end -
begin is >= nonceprefix.length(). So there could be a buffer overrun.

> Source/WebCore/page/ContentSecurityPolicy.cpp:632
> +    if ((position + 1) != end || *position != '\'' || !(position -
nonceBegin))
> +	   return false;

No need for parentheses in the (position + 1) != end expression.

!(position - nonceBegin) seems like a strange way to write position !=
nonceBegin.

> Source/WebCore/page/ContentSecurityPolicy.cpp:807
> +    bool allowNonce(const String& nonce) const { return
m_sourceList.allowNonce(nonce.stripWhiteSpace()); }

Seems a little strange that the stripWhitespace is done here, rather than in
the CSPSourceList::allowNonce function or at the call site. Seems almost random
to do it at this level rather than the lowest level or the highest. Maybe
there's some rationale for this being the right level.

I think we probably want stripLeadingAndTrailingHTMLSpaces here, rather than
stripWhiteSpace. I don’t think we want to strip arbitrary Unicode whitespace,
rather just the usual HTML parser idiom whitespace characters.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1011
> +    String suffix = directive->allowInline() && directive->isNoncePresent()

Can we use const char* here to avoid creating a String multiple times? Should
require only a small amount of rearranging.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1016
> -	   suffix = makeString(" Note that '", (isScript ? "script" : "style"),
"-src' was not explicitly set, so 'default-src' is used as a fallback.");
> +	   suffix = suffix + " Note also that '" + String(isScript ? "script" :
"style") + "-src' was not explicitly set, so 'default-src' is used as a
fallback.";

Why switch away from makeString here? That just forces us to allocate an extra
string for the "script" and "style" for no good reason.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1516
> +    for (size_t i = 0; i < policies.size(); ++i) {
> +	   if (!(policies[i].get()->*allowed)(nonce))
> +	       return false;
> +    }

Should use a modern for loop:

    for (auto& policy : policies)