[webkit-reviews] review denied: [Bug 89750] Support paths in Content Security Policy directives. : [Attachment 150615] Slightly less 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 89750: Support paths in Content Security Policy directives.
https://bugs.webkit.org/show_bug.cgi?id=89750

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

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


> Source/WebCore/page/ContentSecurityPolicy.cpp:188
> +	   return path.startsWith(m_path, false);

This test is overly simplistic.  You need to do some sort of comparison that
ignores encoding differences.

> Source/WebCore/page/ContentSecurityPolicy.cpp:230
> +    String m_directiveName;
> +    ScriptExecutionContext* m_scriptExecutionContext;

I'd prefer not to store this state on CSPSourceList.  Perhaps the source list
can report its error to some higher level object that has a back pointer to
ScriptExecutionContext?

> Source/WebCore/page/ContentSecurityPolicy.cpp:486
> +    while (position < end) {
> +	 skipWhile<isPathComponentCharacter>(position, end);

four-space indent pls.

> Source/WebCore/page/ContentSecurityPolicy.cpp:496
> +	   logInvalidPath(String(begin, end - begin));

Let's worry about logging errors in an later patch.  This line is pushing us to
have this object keep a bunch of state that it doesn't really need or want.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:501
>> +	path = KURL(m_scriptExecutionContext->url(), String(begin, end -
begin)).path();
> 
> This seems like an expensive way of parsing the path, but it's the best I
could find (that didn't involve replicating huge swaths of code). Are there
better mechanisms for canonicalizing paths?

I'm not sure what you're trying to accomplish here.  We should just store
String(begin, end - begin) into path and use a comparison function that
understands %-escaping.  It's not the case that KURL canonicalizes paths
anyway, so this isn't really saving us any work.