Bug 97538

Summary: CSP paths: Ignore invalid path components, rather than dropping the source completely.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85558    
Attachments:
Description Flags
Patch
none
Patch none

Description Mike West 2012-09-25 01:43:48 PDT 
Offlist, Tanvi expressed concern that WebKit currently drops source expressions on the floor if they contain '#' or '?'. I think this is a mistake in the implementation, as we'd apparently agreed to simply throw a warning in that case[1].

[1]: https://bugs.webkit.org/show_bug.cgi?id=89750#c4
Comment 1 Mike West 2012-09-25 01:50:30 PDT 
Created attachment 165557 [details]
Patch
Comment 2 Adam Barth 2012-09-25 09:15:04 PDT 
Comment on attachment 165557 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:1579
> +{

Can you ASSERT that invalidChar is either '#' or '?'

I know you have that assert above, but it's good to have it in this function because the correctness of this function depends on that fact.
Comment 3 Mike West 2012-09-25 09:29:59 PDT 
Comment on attachment 165557 [details]
Patch

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

>> Source/WebCore/page/ContentSecurityPolicy.cpp:1579
>> +{
> 
> Can you ASSERT that invalidChar is either '#' or '?'
> 
> I know you have that assert above, but it's good to have it in this function because the correctness of this function depends on that fact.

Sure. I'll spin a new patch in a moment, thanks!
Comment 4 Mike West 2012-09-25 09:35:07 PDT 
Created attachment 165629 [details]
Patch
Comment 5 Mike West 2012-09-25 09:37:16 PDT 
Comment on attachment 165629 [details]
Patch

CQ?, assuming the bots don't mind the extra ASSERT? :)
Comment 6 Adam Barth 2012-09-25 09:46:13 PDT 
> CQ?, assuming the bots don't mind the extra ASSERT? :)

The bots test in release, so they're not going to complain.  ;)
Comment 7 WebKit Review Bot 2012-09-25 10:24:01 PDT 
Comment on attachment 165629 [details]
Patch

Clearing flags on attachment: 165629

Committed r129525: <http://trac.webkit.org/changeset/129525>
Comment 8 WebKit Review Bot 2012-09-25 10:24:04 PDT 
All reviewed patches have been landed.  Closing bug.