Bug 54799

Created attachment 83064 [details] Needs tests

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

Created attachment 83066 [details] Patch

Comment on attachment 83066 [details] Patch The grammar in the spec has been updated.

Created attachment 86298 [details] work in progress

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.

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

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

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

Created attachment 86356 [details] work in progress

Created attachment 86886 [details] Patch

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

Created attachment 87007 [details] Patch

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

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

Created attachment 87010 [details] Patch

Comment on attachment 87010 [details] Patch This looks reasonable.

Comment on attachment 87010 [details] Patch Thanks!

Comment on attachment 87010 [details] Patch Clearing flags on attachment: 87010 Committed r82028: <http://trac.webkit.org/changeset/82028>

All reviewed patches have been landed. Closing bug.