Bug 89281 – Ignore paths in Content Security Policy sources rather than failing to parse them.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

`X-WebKit-CSP: script-src http://example.com/;` should work correctly. See http://crbug.com/128493 and the `ext-host-source` definition in the spec (https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-1.0-specification.html#source-list).

[Mike West]

Created attachment 147961 [details]
First pass: needs more tests.

[Mike West]

I'd like to take this in steps, if you don't mind Adam.

The attached patch is a first pass at adjusting the parseSource method to accept paths, but has a more or less no-op `parsePath` method, and throws away the path component. I think it's complex enough that it deserves it's own review. I think it also needs more tests... suggestions for cases I've missed are welcome. :)

The next step would be to save the path (after properly parsing it), and use it for comparisons.

[Adam Barth]

Comment on attachment 147961 [details]
First pass: needs more tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=147961&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:316
>          if (!parseHost(beginHost, position, host, hostHasWildcard))
>              return false;
>          return true;

Can't we just say "return parseHost(beginHost, position, host, hostHasWildcard);" in these cases?

> Source/WebCore/page/ContentSecurityPolicy.cpp:356
> +            skipWhile<isNotColonOrSlash>(position, end);

Why isNotColonOrSlash here?

> Source/WebCore/page/ContentSecurityPolicy.cpp:462
> +    ASSERT(!path);

Does that work?  I would have thought you'd need to say ASSERT(path.isEmpty())

> LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-05.html:17
> +    ['yes', 'script-src http://127.0.0.1:*/path', 'resources/script.js'],

I would add some tests with ? and # parts of the URL.  I'd also add some tests that include spaces and ; in the path to show that we're not too aggressive in eating up characters.

[Mike West]

Comment on attachment 147961 [details]
First pass: needs more tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=147961&action=review

I'll fix these up in a patch shortly. Thanks for the quick review!

>> Source/WebCore/page/ContentSecurityPolicy.cpp:316
>>          return true;
> 
> Can't we just say "return parseHost(beginHost, position, host, hostHasWildcard);" in these cases?

We can! I only did it this way because I thought it fit better stylistically, but I didn't like it. :)

>> Source/WebCore/page/ContentSecurityPolicy.cpp:356
>> +            skipWhile<isNotColonOrSlash>(position, end);
> 
> Why isNotColonOrSlash here?

Hrm. I guess I can replace this with skipUtil(position, end, '/').

Relatedly: is `skipUtil` intentional? I want to change it to skipU_n_til every time I see it.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:462
>> +    ASSERT(!path);
> 
> Does that work?  I would have thought you'd need to say ASSERT(path.isEmpty())

I think you're right. I'm not sure why it worked either.

>> LayoutTests/http/tests/security/contentSecurityPolicy/source-list-parsing-05.html:17
>> +    ['yes', 'script-src http://127.0.0.1:*/path', 'resources/script.js'],
> 
> I would add some tests with ? and # parts of the URL.  I'd also add some tests that include spaces and ; in the path to show that we're not too aggressive in eating up characters.

What's the expected behavior in those cases? I think we'd end up with a path of `/path?query=value`, as both '?' and '#' fall into `<VCHAR except ";" and ",">`. Is that accurate?

[Adam Barth]

> Relatedly: is `skipUtil` intentional? I want to change it to skipU_n_til every time I see it.

That's just a typo.  I fail at spelling.

[Adam Barth]

> What's the expected behavior in those cases? I think we'd end up with a path of `/path?query=value`, as both '?' and '#' fall into `<VCHAR except ";" and ",">`. Is that accurate?

Yeah, the "path" will eat up all those characters.  We'll probably end up ignoring everything after the ? or the # though.

[Mike West]

Created attachment 147982 [details]
abarth feedback.

[Mike West]

Created attachment 147983 [details]
adding abarth as reviewer.

[Mike West]

That was fast. :)

I'm renaming this bug to "Ignore paths in Content Security Policy sources rather than failing to parse them." and will cover any future work on paths in another.

This patch brings us up to 1.0, but the path behavior isn't really specced out yet... Should I set that next bug to block the CSP 1.1 meta bug?

[Adam Barth]

I commented on the next bug.

[WebKit Review Bot]

Comment on attachment 147983 [details]
adding abarth as reviewer.

Clearing flags on attachment: 147983

Committed r120540: <http://trac.webkit.org/changeset/120540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.