content/base/test/csp/test_csp_regexp_parsing.html includes some "blocked" tests for host/port with trailing junk after the port. According to the ABNF in CSP 1.0, these should be treated as invalid sources because of the junk after the port (that is not proper ext-host-src since it is missing a "/"). http://hg.mozilla.org/users/sstamm_mozilla.com/csp-rewrite-patches/file/30822debf637/new_backend_mochitests#l1139 The patch in bug 993477 disables these tests via comments, but ckerschb and I think the new parser should actually block those sources and the tests should not be disabled so it is CSP 1.0 compliant (and also 1.1 compliant here when we start parsing paths).

The invalid sources that should be rejected are: "test1.example.com:88path-1/" "test1.example.com:80.js" "test1.example.com:*.js" "test1.example.com:*."

The current parser silently ignores invalid paths after the successful parsing of [scheme] host [port]. The attached patch logs an error to the console and does *not* accept parsing a host-src if the path is invalid.

Comment on attachment 8433878 [details] [diff] [review] bug_1019984.patch Review of attachment 8433878 [details] [diff] [review]: ----------------------------------------------------------------- Did you confirm this fixes the problem by running the failed test? (I'm thinking it will, but want to be sure.) ::: content/base/src/nsCSPParser.cpp @@ +290,5 @@ > > if (!accept(SLASH)) { > + const char16_t* params[] = { mCurValue.get() }; > + logWarningErrorToConsole(nsIScriptError::warningFlag, "policyURIParseError", > + params, ArrayLength(params)); Why is this "policyURIParseError"? @@ +469,5 @@ > // Calling path() to see if there is a path to parse, if an error > // occurs, path() reports the error; handing cspHost as an argument > // which simplifies parsing of several paths. > if (!path(cspHost)) { > + // Error was reported in path() Please put a comment here explaining why nullptr is returned when path() fails.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3) > Comment on attachment 8433878 [details] [diff] [review] > bug_1019984.patch > > Review of attachment 8433878 [details] [diff] [review]: > ----------------------------------------------------------------- > > Did you confirm this fixes the problem by running the failed test? (I'm > thinking it will, but want to be sure.) I verified that the test succeeds. > ::: content/base/src/nsCSPParser.cpp > @@ +290,5 @@ > > > > if (!accept(SLASH)) { > > + const char16_t* params[] = { mCurValue.get() }; > > + logWarningErrorToConsole(nsIScriptError::warningFlag, "policyURIParseError", > > + params, ArrayLength(params)); > > Why is this "policyURIParseError"? Isn't that the most appropriate error if we can't parse a URI of a policy? http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/security/csp.properties#73 If there is a better one, let me know, and I'll update. > > @@ +469,5 @@ > > // Calling path() to see if there is a path to parse, if an error > > // occurs, path() reports the error; handing cspHost as an argument > > // which simplifies parsing of several paths. > > if (!path(cspHost)) { > > + // Error was reported in path() > > Please put a comment here explaining why nullptr is returned when path() > fails. Definitely can, just used the same syntax and comment which we used further up in that function: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPParser.cpp#442

As discussed in person, we should rather use 'couldntParseInvalidSource' to report the error in case we are encountering an invalid path while parsing. I took a closer look at what valid paths are, and I encountered that double slashes are invalid (http://tools.ietf.org/html/rfc3986#section-3.3) and should cause an error while parsing. Formerly we were accepting double slashes even though the parser should reject such cases. I incorporated that change which also causes tests in TestCSPParser to fail. I updated the parser as well as TestCSPParser.cpp to completely follow the spec. Looking through the parser errors, I encountered other places where we are incorrectly using 'policyURIParseError' - I filed bug 1020407 to update those cases.

Does policyURIParseError refer specifically to the (non-standard) policy-uri directive?

(In reply to Garrett Robinson [:grobinson] from comment #6) > Does policyURIParseError refer specifically to the (non-standard) policy-uri > directive? I was not aware of that, but Sid told me that that is the case. In the end 'couldntParseInvalidSource' is a better fit anyway even if policyURIParseError would not refer specifically to policy-uri.

Comment on attachment 8434269 [details] [diff] [review] bug_1019984_v2.patch Review of attachment 8434269 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.