Bug 54799 – Teach Content Security Policy how to parse source-list

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Teach Content Security Policy how to parse origin-patterns

[Adam Barth]

Created attachment 83064 [details]
Needs tests

[Adam Barth]

Spec (although I haven't filled in all the details yet):
http://www.w3.org/Security/wiki/Content_Security_Policies

[Adam Barth]

Created attachment 83066 [details]
Patch

[Adam Barth]

Comment on attachment 83066 [details]
Patch

The grammar in the spec has been updated.

[Adam Barth]

Created attachment 86298 [details]
work in progress

[Darin Adler]

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.

[Adam Barth]

(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.

[Eric Seidel (no email)]

(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.

[Adam Barth]

> 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.

[Adam Barth]

Created attachment 86356 [details]
work in progress

[Adam Barth]

Created attachment 86886 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

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.

[Adam Barth]

Created attachment 87007 [details]
Patch

[Early Warning System Bot]

Attachment 87007 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8252404

[Build Bot]

Attachment 87007 [details] did not build on win:
Build output: http://queues.webkit.org/results/8252410

[Adam Barth]

Created attachment 87010 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87010 [details]
Patch

This looks reasonable.

[Adam Barth]

Comment on attachment 87010 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 87010 [details]
Patch

Clearing flags on attachment: 87010

Committed r82028: <http://trac.webkit.org/changeset/82028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.