Bug 54799 - Teach Content Security Policy how to parse source-list
: Teach Content Security Policy how to parse source-list
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 53572
  Show dependency treegraph
 
Reported: 2011-02-19 00:06 PST by
Modified: 2011-03-26 05:00 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-02-19 00:06:59 PST
Teach Content Security Policy how to parse origin-patterns
------- Comment #1 From 2011-02-19 00:13:56 PST -------
Created an attachment (id=83064) [details]
Needs tests
------- Comment #2 From 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 From 2011-02-19 02:05:38 PST -------
Created an attachment (id=83066) [details]
Patch
------- Comment #4 From 2011-03-17 12:01:38 PST -------
(From update of attachment 83066 [details])
The grammar in the spec has been updated.
------- Comment #5 From 2011-03-20 23:44:30 PST -------
Created an attachment (id=86298) [details]
work in progress
------- Comment #6 From 2011-03-21 07:04:33 PST -------
(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.

> 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 From 2011-03-21 10:47:51 PST -------
(In reply to comment #6)
> (From update of attachment 83066 [details] [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 From 2011-03-21 10:53:20 PST -------
(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 From 2011-03-21 10:56:08 PST -------
> 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 From 2011-03-21 12:59:39 PST -------
Created an attachment (id=86356) [details]
work in progress
------- Comment #11 From 2011-03-24 22:48:12 PST -------
Created an attachment (id=86886) [details]
Patch
------- Comment #12 From 2011-03-25 16:16:56 PST -------
(From update of attachment 86886 [details])
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 From 2011-03-25 16:22:16 PST -------
(From update of attachment 86886 [details])
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 From 2011-03-25 21:53:32 PST -------
Created an attachment (id=87007) [details]
Patch
------- Comment #15 From 2011-03-25 22:09:28 PST -------
Attachment 87007 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8252404
------- Comment #16 From 2011-03-25 22:23:09 PST -------
Attachment 87007 [details] did not build on win:
Build output: http://queues.webkit.org/results/8252410
------- Comment #17 From 2011-03-25 22:33:00 PST -------
Created an attachment (id=87010) [details]
Patch
------- Comment #18 From 2011-03-25 22:36:16 PST -------
(From update of attachment 87010 [details])
This looks reasonable.
------- Comment #19 From 2011-03-25 22:54:36 PST -------
(From update of attachment 87010 [details])
Thanks!
------- Comment #20 From 2011-03-26 04:59:55 PST -------
(From update of attachment 87010 [details])
Clearing flags on attachment: 87010

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