Summary: | Teach Content Security Policy how to parse source-list | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, eric, jochen, webkit-ews | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 53572 | ||||||||||||||||||
Attachments: |
|
Description
Adam Barth
2011-02-19 00:06:59 PST
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. |