Bug 97538 - CSP paths: Ignore invalid path components, rather than dropping the source completely.
Summary: CSP paths: Ignore invalid path components, rather than dropping the source co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on:
Blocks: 85558
  Show dependency treegraph
 
Reported: 2012-09-25 01:43 PDT by Mike West
Modified: 2012-09-25 10:24 PDT (History)
2 users (show)

See Also:


Attachments
Patch (15.69 KB, patch)
2012-09-25 01:50 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (15.71 KB, patch)
2012-09-25 09:35 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.