Bug 54799 - Teach Content Security Policy how to parse source-list
Summary: Teach Content Security Policy how to parse source-list
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 53572
  Show dependency treegraph
 
Reported: 2011-02-19 00:06 PST by Adam Barth
Modified: 2011-03-26 05:00 PDT (History)
5 users (show)

See Also:


Attachments
Needs tests (6.68 KB, patch)
2011-02-19 00:13 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2011-02-19 02:05 PST, Adam Barth
no flags Details | Formatted Diff | Diff
work in progress (4.91 KB, patch)
2011-03-20 23:44 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
work in progress (11.62 KB, patch)
2011-03-21 12:59 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (27.14 KB, patch)
2011-03-24 22:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (27.16 KB, patch)
2011-03-25 21:53 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (28.10 KB, patch)
2011-03-25 22:33 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-19 00:06:59 PST
Teach Content Security Policy how to parse origin-patterns
Comment 1 Adam Barth 2011-02-19 00:13:56 PST
Created attachment 83064 [details]
Needs tests
Comment 2 Adam Barth 2011-02-19 00:19:54 PST
Spec (although I haven't filled in all the details yet):
http://www.w3.org/Security/wiki/Content_Security_Policies
Comment 3 Adam Barth 2011-02-19 02:05:38 PST
Created attachment 83066 [details]
Patch
Comment 4 Adam Barth 2011-03-17 12:01:38 PDT
Comment on attachment 83066 [details]
Patch

The grammar in the spec has been updated.
Comment 5 Adam Barth 2011-03-20 23:44:30 PDT
Created attachment 86298 [details]
work in progress
Comment 6 Darin Adler 2011-03-21 07:04:33 PDT
Comment on attachment 83066 [details]
Patch

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

Not sure the status of this patch since the review flag is gone from it. I read some of it and have a few comments.

> Source/WebCore/page/ContentSecurityPolicy.cpp:32
> +namespace {

In this project we have been using static to give functions internal linkage instead of using anonymous namespaces. You’ve mentioned a couple time that you can’t remember that. Is that really the issue or do you prefer doing it this way.

> Source/WebCore/page/ContentSecurityPolicy.cpp:36
> +    return isASCIIAlpha(c) || isASCIIDigit(c) || c == '+' || c == '-' || c == '.';

You can use isASCIIAlphanumeric here instead of isASCIIAlpha || isASCIIDigit.

> Source/WebCore/page/ContentSecurityPolicy.cpp:39
> +bool extractScheme(const UChar*& pos, const UChar* end, String& result)

I prefer words like "position" to abbreviations like "pos".

> Source/WebCore/page/ContentSecurityPolicy.cpp:55
> +    UChar currentCharater = *pos;
> +    while (isSchemeCharacter(currentCharater)) {
> +        scheme.append(toASCIILower(currentCharater));
> +        ++pos;
> +        if (pos == end)
> +            break;
> +        currentCharater = *pos;
> +    }

Typo here, current “charater”. The compiler will eliminate common subexpressions; I think you can write this in a way that reads better without the local variable:

    while (pos < end && isSchemeCharacter(*pos))
        scheme.append(toASCIILower(*pos++)

> Source/WebCore/page/ContentSecurityPolicy.cpp:112
> +        port *= 10;
> +        port += currentCharacter - '0';

What about overflow?

> Source/WebCore/page/ContentSecurityPolicy.cpp:156
> +            if (end - pos < 3
> +                || *pos++ != ':'
> +                || *pos++ != '/'
> +                || *pos++ != '/') {

I think this predicate would read better on a single line or with a helper function.

This will do the wrong thing if some characters match and others don't, advancing past some characters even if the string doesn’t match.

> Source/WebCore/page/ContentSecurityPolicy.cpp:170
> +            if (pos == end
> +                || *pos++ != '.'
> +                || pos == end)

I think this predicate would read better on a single line or with a helper function. It advances pos even if it does not match the '.', which I think is a bug.
Comment 7 Adam Barth 2011-03-21 10:47:51 PDT
(In reply to comment #6)
> (From update of attachment 83066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83066&action=review
> 
> Not sure the status of this patch since the review flag is gone from it. I read some of it and have a few comments.

Thanks for the comments.  The Mozilla folks have posted a new version of the spec that specifies the parsing rules more clearly, so I've scrapped this version of the patch and started on a new one (called work-in-progress above).

Many of your comments apply to the new patch as well, so they're useful and I'll apply them there.

> > Source/WebCore/page/ContentSecurityPolicy.cpp:32
> > +namespace {
> 
> In this project we have been using static to give functions internal linkage instead of using anonymous namespaces. You’ve mentioned a couple time that you can’t remember that. Is that really the issue or do you prefer doing it this way.

The problem is that different projects I contribute to have different opinions on this topic.  It doesn't come up that often, so I sometimes forget which is which.  In any case, the new version of the patch correctly uses static.

> > Source/WebCore/page/ContentSecurityPolicy.cpp:112
> > +        port *= 10;
> > +        port += currentCharacter - '0';
> 
> What about overflow?

I believe the correct thing to do here is to call String::toIntStrict, but I'll look into how that handles overflow.
Comment 8 Eric Seidel (no email) 2011-03-21 10:53:20 PDT
(In reply to comment #7)
> > > Source/WebCore/page/ContentSecurityPolicy.cpp:32
> > > +namespace {
> > 
> > In this project we have been using static to give functions internal linkage instead of using anonymous namespaces. You’ve mentioned a couple time that you can’t remember that. Is that really the issue or do you prefer doing it this way.
> 
> The problem is that different projects I contribute to have different opinions on this topic.  It doesn't come up that often, so I sometimes forget which is which.  In any case, the new version of the patch correctly uses static.

Would someone please update the style guide and check-webkit-style if we have consensus here?
http://www.webkit.org/coding/coding-style.html
It says nothing about anonymous namespaces as far as I can tell.
Comment 9 Adam Barth 2011-03-21 10:56:08 PDT
> Would someone please update the style guide and check-webkit-style if we have consensus here?
> http://www.webkit.org/coding/coding-style.html
> It says nothing about anonymous namespaces as far as I can tell.

Sure.  I'll write up a patch for that.
Comment 10 Adam Barth 2011-03-21 12:59:39 PDT
Created attachment 86356 [details]
work in progress
Comment 11 Adam Barth 2011-03-24 22:48:12 PDT
Created attachment 86886 [details]
Patch
Comment 12 Eric Seidel (no email) 2011-03-25 16:16:56 PDT
Comment on attachment 86886 [details]
Patch

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

Seems there is a lot of code and not very much testing of said code.  I think we need more than 10 tests for this new parser.

> Source/WebCore/page/ContentSecurityPolicy.cpp:37
> +// templete parameters.

templete :)

> Source/WebCore/page/ContentSecurityPolicy.cpp:45
> -static bool isDirectiveValueCharacter(UChar c)
> +bool isDirectiveValueCharacter(UChar c)

I doubt darin will like the removal of static, but i'm not sure.  Again, I don't really undertand the rule or style, etc.

> Source/WebCore/page/ContentSecurityPolicy.cpp:154
> +    explicit CSPSourceList(SecurityOrigin* self);

self?  Should just be removed.

> Source/WebCore/page/ContentSecurityPolicy.cpp:169
> +    SecurityOrigin* m_self;

I'm confused what "self" means here.

> Source/WebCore/page/ContentSecurityPolicy.cpp:226
> +//                   / "'self'"

I see, it's a spec term?

> Source/WebCore/page/ContentSecurityPolicy.cpp:283
> +        ASSERT_NOT_REACHED();

I take it we should never get here because we'll fall out somewhere earlier?
Comment 13 Adam Barth 2011-03-25 16:22:16 PDT
Comment on attachment 86886 [details]
Patch

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

>> Source/WebCore/page/ContentSecurityPolicy.cpp:45
>> -static bool isDirectiveValueCharacter(UChar c)
>> +bool isDirectiveValueCharacter(UChar c)
> 
> I doubt darin will like the removal of static, but i'm not sure.  Again, I don't really undertand the rule or style, etc.

Right, that's why I wrote a big comment explaining why.  The compiler doesn't allow static functions as template parameters but functions in an anonymous namespace are ok.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:154
>> +    explicit CSPSourceList(SecurityOrigin* self);
> 
> self?  Should just be removed.

yep

>> Source/WebCore/page/ContentSecurityPolicy.cpp:169
>> +    SecurityOrigin* m_self;
> 
> I'm confused what "self" means here.

I can just rename it to m_securityOrigin.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:226
>> +//                   / "'self'"
> 
> I see, it's a spec term?

Yes.  I agree that it's confusing to use that name for the variable names though.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:283
>> +        ASSERT_NOT_REACHED();
> 
> I take it we should never get here because we'll fall out somewhere earlier?

Yes, we've already verified that we're not at the end and that *position is ':'.  The ASSERT is just to catch future bugs.
Comment 14 Adam Barth 2011-03-25 21:53:32 PDT
Created attachment 87007 [details]
Patch
Comment 15 Early Warning System Bot 2011-03-25 22:09:28 PDT
Attachment 87007 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8252404
Comment 16 Build Bot 2011-03-25 22:23:09 PDT
Attachment 87007 [details] did not build on win:
Build output: http://queues.webkit.org/results/8252410
Comment 17 Adam Barth 2011-03-25 22:33:00 PDT
Created attachment 87010 [details]
Patch
Comment 18 Eric Seidel (no email) 2011-03-25 22:36:16 PDT
Comment on attachment 87010 [details]
Patch

This looks reasonable.
Comment 19 Adam Barth 2011-03-25 22:54:36 PDT
Comment on attachment 87010 [details]
Patch

Thanks!
Comment 20 WebKit Commit Bot 2011-03-26 04:59:55 PDT
Comment on attachment 87010 [details]
Patch

Clearing flags on attachment: 87010

Committed r82028: <http://trac.webkit.org/changeset/82028>
Comment 21 WebKit Commit Bot 2011-03-26 05:00:00 PDT
All reviewed patches have been landed.  Closing bug.