Sid Stamm [:geekboy or :sstamm] Reporter
	Description • 10 years ago

When sites send the reflected-xss directive with their CSP, we should put a warning in the developer/web console about how Firefox interprets it may not line up with the spec.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 1 • 10 years ago

Currently the csp-parser would log a warning to the console if it encounters the 'reflected-xss' directive during parsing [1]:
> "Couldn't process unknown directive ..."
basically the parser logs the same warning for all unknown directives; every directive not listed here [2].

Everything following 'reflected-xss' will be ignored till we hit the ending ";" for that directive. I am not sure if we want anything else than what we already have.

Or do you want a special warning, because 'reflected-xss' is in the spec, but we do *not* support it? At least not support it as of now.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPParser.cpp#780
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.h#78
[2] http://www.w3.org/TR/2014/WD-CSP11-20140211/#reflected-xss

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 2 • 10 years ago

That should do what we want for this bug.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 3 • 10 years ago

Facepalm - copy/paste error and fixed only one site and not the other - should be: notSupportingDirective :-)

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 4 • 10 years ago

Comment on attachment 8465961 [details] [diff] [review]
bug_1045902_wip.patch

Garrett, wanna review this?

	Garrett Robinson [:grobinson]
	Comment 5 • 10 years ago

Comment on attachment 8465961 [details] [diff] [review]
bug_1045902_wip.patch

Review of attachment 8465961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/locales/en-US/chrome/security/csp.properties
@@ +66,5 @@
>  # %1$S is the hostname in question and %2$S is the keyword
>  hostNameMightBeKeyword = Interpreting %1$S as a hostname, not a keyword. If you intended this to be a keyword, use '%2$S' (wrapped in single quotes).
> +# LOCALIZATION NOTE (notSupportingDirective):
> +# directive is not supported (e.g. 'reflected-xss')
> +notSupportingDirective = Not supporting directive '%1$S'. Directive and values will be ignored.

A thought: people might wonder *why* we are ignoring reflected-xss, and this messages does not explain that. However, I think it is sufficiently clear and for the average user, and it is usefully generic. People who want to know that should be able to find the answer online (i.e. in this bug).

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 6 • 10 years ago

Garrett, I couldn't resist to write a testcase for that as well. Can you have a look? Should be pretty straight forward. Thanks!

	Garrett Robinson [:grobinson]
	Comment 7 • 10 years ago

Comment on attachment 8472731 [details] [diff] [review]
bug_1045902_tests.patch

Review of attachment 8472731 [details] [diff] [review]:
-----------------------------------------------------------------

I think you can get rid of a lot of boilerplate if you use SimpleTest.expectConsoleMessages.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 8 • 10 years ago

(In reply to Garrett Robinson [:grobinson] from comment #7)
> Comment on attachment 8472731 [details] [diff] [review]
> bug_1045902_tests.patch
> 
> Review of attachment 8472731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you can get rid of a lot of boilerplate if you use
> SimpleTest.expectConsoleMessages.

In general, I totally agree with you, but it seems that there is a problem related to the 'SENTINEL' on line
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#964
and hence it's not working correctly which causes my testcase to fail.

At the moment I don't want to invest the time to figure out what's causing the problem in detail. Since literally all other tests in devtools use the approach I am using to test console messages, I think you agree that we can accept the test the way it is right now. Can we agree on that?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 9 • 10 years ago

Added reviewer to the comment. Carrying over r+ from grobinson.

	Garrett Robinson [:grobinson]
	Comment 10 • 10 years ago

Comment on attachment 8474761 [details] [diff] [review]
bug_1045902_tests_v2.patch

Review of attachment 8474761 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed that you shouldn't waste time on SimpleTest.expectConsoleMessages if it's buggy. Other than the boilerplate, this test is good!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cffcf3a6c904
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25aeae7a346

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/cffcf3a6c904
https://hg.mozilla.org/mozilla-central/rev/d25aeae7a346

	Eric Shepherd [:sheppy]
	Comment 13 • 9 years ago

I've added dev-doc-needed since it may make sense to add a note somewhere in the CSP docs explaining what the deal is here. If it doesn't, we can remove the keyword.

	Jean-Yves Perrier [:teoli]
	Comment 14 • 9 years ago

I added a comment in: https://developer.mozilla.org/en-US/Firefox/Releases/34#Security_and_Networking

	u561511
	Comment 15 • 8 years ago

Is there any update on this? It doesn't appear any documentation has been modified as to why firefox returns this way, only that it does.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 16 • 8 years ago

(In reply to Jacob Taylor from comment #15)
> Is there any update on this? It doesn't appear any documentation has been
> modified as to why firefox returns this way, only that it does.

We are working on a blogpost to cover that topic. Once posted, I will paste a link to the post within this bug.