See the single broken 'script-nonce' test at http://erlend.oftedal.no/blog/csp/readiness/version-1.1-old.php I've apparently not properly implemented the RFC 2616 definition of "token". I should do that.
Created attachment 158393 [details] Patch
This patch implements stricter checking against the RFC 2616 definition of "token". Honestly, it might make more sense to change the spec to VCHAR minus spaces, ';', and ','. :) Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter? WDYT, Adam?
Comment on attachment 158393 [details] Patch Attachment 158393 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13506025
Comment on attachment 158393 [details] Patch Attachment 158393 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13489829
Comment on attachment 158393 [details] Patch Attachment 158393 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13491709 New failing tests: http/tests/security/contentSecurityPolicy/1.1/scriptnonce-invalidnonce.html
Created attachment 158398 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158393 [details] Patch Attachment 158393 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13492680
Created attachment 158404 [details] Let's try that again.
Comment on attachment 158404 [details] Let's try that again. View in context: https://bugs.webkit.org/attachment.cgi?id=158404&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:81 > +bool isSeparatorCharacter(UChar c) > +{ > + return c == '(' || c == ')' || c == '<' || c == '>' || c == '@' || c == ',' > + || c == ';' || c == ':' || c == '\\' || c == '"' || c == '/' > + || c == '[' || c == ']' || c == '?' || c == '=' || c == '{' > + || c == '}' || c == ' ' || c == '\t'; > +} > + > +bool isControlCharacter(UChar c) > +{ > + return c <= 0x1f || c == 0x7f; > +} > + > +bool isNonceCharacter(UChar c) > +{ > + // RFC2616 'token': Any CHAR other than separators and CTLs. > + return !isASCIISpace(c) && !isSeparatorCharacter(c) && !isControlCharacter(c); > +} We should already have a function that checks whether something is a valid token. If we don't, we should add one to a more general location than ContentSecurityPolicy.cpp
> Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter? It's up to you. I'd be inclined to be liberal here and allow VCHAR - whatever delimiters we need to exclude.
(In reply to comment #10) > > Is there any good reason I couldn't use '@' in a nonce? Or '/' for that matter? > > It's up to you. I'd be inclined to be liberal here and allow VCHAR - whatever delimiters we need to exclude. Great. That's what I'd prefer as well. I'll rework the patch to allow VCHAR minus ; and ,. I don't think we really need to exclude anything else.
Created attachment 158419 [details] Liberalizing.
Renaming the bug to match the new patch. I'll change the draft csp 1.1 spec tomorrow to drop 'token' and replace it with VCHAR - stuff.
Comment on attachment 158419 [details] Liberalizing. Can you update the spec as well?
> I'll change the draft csp 1.1 spec tomorrow to drop 'token' and replace it with VCHAR - stuff. Ah, I didn't see this message. :)
Comment on attachment 158419 [details] Liberalizing. Clearing flags on attachment: 158419 Committed r125614: <http://trac.webkit.org/changeset/125614>
All reviewed patches have been landed. Closing bug.
Comment on attachment 158419 [details] Liberalizing. View in context: https://bugs.webkit.org/attachment.cgi?id=158419&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:61 > return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that.
Comment on attachment 158419 [details] Liberalizing. View in context: https://bugs.webkit.org/attachment.cgi?id=158419&action=review >> Source/WebCore/page/ContentSecurityPolicy.cpp:61 >> return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR > > This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that. Thanks Darin.
(In reply to comment #19) > (From update of attachment 158419 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158419&action=review > > >> Source/WebCore/page/ContentSecurityPolicy.cpp:61 > >> return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR > > > > This line of code does the same thing that isASCIIPrintable(c) does. Probably better to just call that. > > Thanks Darin. isASCIIPrintable seems to include ' '. Is that intentional? template<typename CharType> inline bool isASCIIPrintable(CharType c) { return c >= ' ' && c <= '~'; }
Looks like it: http://en.wikipedia.org/wiki/ASCII#ASCII_printable_characters So, there's a one-character difference.
https://dvcs.w3.org/hg/content-security-policy/rev/3d8307e7ced0 (and fixed in https://dvcs.w3.org/hg/content-security-policy/rev/4c7d1c445641)