User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1632.4 Safari/537.36 Steps to reproduce: Visit https://about.twitter.com/products (which sets Content-Security-Policy and X-Content-Security-Policy. Actual results: In the console: [10:15:19.680] The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead. @ https://about.twitter.com/products [10:15:19.680] This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored. @ https://about.twitter.com/products Expected results: Nothing? Just the second message? UA sniffing would be required for the developer to remove these error messages. I think supplying both headers shows intent. If an unexpected header was applied, the CSP reports would reflect this.

I could get behind only showing the 2nd message when both headers are sent. Then the first message would only be shown if the site ONLY specifies the old header. I think Neil's point about both headers showing intent to be backwards compatible until the X- header is deprecated (when it should just continue to be ignored as it is when both are sent) is a good one.

I agree that only showing the second message when both headers are present is a good idea. This is low priority, and would make a good mentored bug. freddyb, I know you're interested in ramping up to some C++, want to take this? :)

Sorry to disappoint but this must be a misunderstanding ;D I have never done any c++ before and I don't think it's a good use of my time to do so right now..

(In reply to Garrett Robinson [:grobinson] from comment #2) > I agree that only showing the second message when both headers are present > is a good idea. This is low priority, and would make a good mentored bug. > freddyb, I know you're interested in ramping up to some C++, want to take > this? :) Please have a look at this first version. Thought this would be an easy bug to fix too :) issues: (1) lack of test: I am not sure how to write a webconsole test that tests the number of expected warnings to be seen. (2) I initially wrote the patch without negating everything (that is, just && all the IsEmpty) - saves a couple negations. Though I think this patch is more readable than saving a couple negations... Screenshot (using twitter as demo): http://i.imgur.com/Z6oNrGO.png Let me know what to address in the next patch if needed. I was going to ping you but thought I could just write the patch first :) Thanks.

Comment on attachment 8333700 [details] [diff] [review] bug_918397_mutex_csp_warning_v1.patch Review of attachment 8333700 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good :) All you need to do now is write a test. Check out "7.patch" from bug 713980 for an example of test that listens to the Console Service (not specifically the web console, but the messages in the web console are a subset of message sent to nsIConsoleService). There are some helper functions in SimpleTest.js that would be good to use here. Check out SimpleTest.runTestExpectingConsoleMessages.

I have been playing around for a day and I have troubling writing the test. I have to DO "something" otherwise I always get "0 console message, expecting 1". The hack I use here is to load an iframe. My theory on why I see nothing is that the warning is sent to console before we invoke SimpleTest.runTestExpectingConsoleMessages. I sort of could do the iframe hack using SimpleTest.monitorConsole as follows: > var errorMsg = "...." > SimpleTest.waitForExplicitFinish(); > SimpleTest.monitorConsole(SimpleTest.finish, [{errorMessage: errorMsg}]); > function doTest() { > var cspframe = document.getElementById('cspframe'); > cspframe.src = "test_bug918397.html"; > }; > doTest(); However, I have trouble using the similar construction in SimpleTest.runTestExpectingConsoleMessages. I get zero console message. I have looked at other tests under content/base/tests and /tests/csp for inspirations (did a lot of digging on the Internet such as reading SimpleTest.js) I will try again and see if I could create a test that uses runTestExpectingConsoleMessages. But ultimately, I really am not sure if I had another way around the iframe hack. Do you think there is a better way than the current iframe hack I am using? Thanks.

Comment on attachment 8338089 [details] [diff] [review] bug_918397_mutex_csp_warning_v2.patch Review of attachment 8338089 [details] [diff] [review]: ----------------------------------------------------------------- Loading a document with a CSP set in a corresponding ^headers^ file in an iframe is the standard way we write the CSP tests. Rather than load the test itself with these headers, I'd recommend you create a separate file_bug918397.html and file_bug918397.html^headers^, and load that html file in the iframe. The html file can probably be empty, or just have the skeleton of an HTML document. The headers file should be the same as test_bug918397.html^headers^ (and get rid of that file). The test should still work without any other changes. This might make SimpleTest.runTestExpectingConsoleMessages work. But the way you're doing it now is just fine so don't change it if you don't want to. Put the test files in content/base/test/csp so they live with the rest of the CSP tests. And change the name to something more descriptive (there was an email thread about this fairly recently, people are trying to encourage this as a new trend). For exampe, test_dual_headers_warning.html. Just make sure the bug number is in a comment in the <head> of the test file. ::: content/base/test/test_bug918397.html @@ +34,5 @@ > + if (!aMsg.message || aMsg.message.indexOf(errorMsg) == -1) > + return; > + > + // only true if the indexOf value is NOT -1 > + isnot(aMsg.message.indexOf(errorMsg), -1, "Dual CSP header warning is not present.") You don't need this test. You already know this is true from the previous if statement. Do: ok(true, "Dual CSP header warning present.") You want the text to describe what is being tested, otherwise the test output will be confusing - this test is to test that the specified error msg *appears*, so seeing "TEST-PASS: Dual CSP header warning is not present." is confusing.

+ Fixed minor grammar in content/base/src/nsDocument.cpp + Moved test and support files to test/csp + Load empty html file in test + Assert ok during the test Question: 1. Do I need to update this patch to work with the latest? Or should I leave this to people whoever going to do a checkin? Thanks grobinson!

(In reply to yeukhon from comment #8) > Question: > > 1. Do I need to update this patch to work with the latest? Or should I leave > this to people whoever going to do a checkin? In general the patch author should do that (see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed) - the checkin-needed elves greatly appreciate it ! :)

Comment on attachment 8344033 [details] [diff] [review] bug_918397_mutex_csp_warning_v3.patch Review of attachment 8344033 [details] [diff] [review]: ----------------------------------------------------------------- You should also add a test that we are *only* seeing the correct errorMsg, and not any other messages (like the additional "X-Content-Security-Policy header is deprecated..." that is currently shown). r=me with that addition. Otherwise this looks great! And yes - you definitely want to "un-bitrot" your patch before asking for checkin-needed. Ping me if you need help. ::: content/base/test/csp/test_dual_headers_warning.html @@ +32,5 @@ > +SpecialPowers.registerConsoleListener(function ConsoleMsgListener(aMsg) { > + // if the expected error message is not present, try again later > + if (!aMsg.message || aMsg.message.indexOf(errorMsg) == -1) > + return; > + ok(true, "Dual CSP header warning present."); Should we also be testing that the "X-Content-Security-Policy headers are deprecated" message is *not* being displayed here? After all, that was the original complaint in comment 1.

+ Added a test case to assert failure when deprecation warning is present As the comment said in the test, our test is not perfect. We rely on the ordering of the console output. If the order is changed, the test will fail. At the moment I don't have a better workaround with this issue (has spoken this on IRC as well). One proposal was to use timeout due to the absence of the deprecation warning, but I don't have a way to invert failure due to timeout to passing. I think we can agree on this patch is simple and sane enough to for a checkin. I will seek additional help from people more familiar with mochitest. Once I receive a final approval, I will request a check-in. Thank you.

Comment on attachment 8345009 [details] [diff] [review] bug_918397_mutex_csp_warning_v4.patch Review of attachment 8345009 [details] [diff] [review]: ----------------------------------------------------------------- Since this patch is so straightforward, I don't think we need a robust test (this one should be just fine). The only thing that concerns me is that it may not work as intended if the ordering changes (which it will when we deprecate the X- header). Would you file a follow-up bug to remove this test (and logic) when we deprecate the X-Content-Security-Policy header? Once filed, please make it block bug 949533 (the bug for deprecating the old parser). You already have r=grobinson from before, but I'll pile on with an additional r=geekboy and you should be set to land. (It would not hurt to run it through try first to make sure the test works on many platforms, but I suspect it will work). ::: content/base/test/csp/test_dual_headers_warning.html @@ +18,5 @@ > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > + > +var depreHeaderMsg = "The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future." > +var dualHeaderMsg = "This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored." You should grab these out of the localizer in case we change them in the future. (Like you did in your patch for bug 587377!)

+ Use GetStringFromName instead of FormatStringFromName. The latter gets an exception when I run mochitest on a directory (or all mochitest) - - B2G tests will not pass because of this bug https://bugzilla.mozilla.org/show_bug.cgi?id=951895#c10 All tests are passing (with the exception of B2G builds): https://tbpl.mozilla.org/?tree=Try&rev=86c5c8431af0

Comment on attachment 8350881 [details] [diff] [review] bug_918397_mutex_csp_warning_v6.patch Test fails on B2G.

Do we absolutely have to wait for B2G tests to be fixed if the problem is known to exist?

We're not going to land it if it's going to fail tests. I suspect there are ways to disable the test on B2G if you don't think you need the coverage, but that's between you and the reviewer.

You can disable the test on B2G, but we don't want to encourage that. Have you tried rebasing this patch on the latest m-c and running the tests on B2G again? The fix for 951895 has landed, so if that really was the cause of the problem, as you say in Comment 13, then they should be working now.

I can't say for sure whether my test is actually affected by the problem describe in https://bugzilla.mozilla.org/show_bug.cgi?id=951895#c10 I did run the same against the latest mozilla-central and still fail. I will try again, maybe something has changed I am not aware of. I probably end up writing a chrome test. I think that might be the only way out to pass on both regular and B2G. smaug can you confirm my understanding of comment 10? Thank you.

comment 10 of this bug or bug 951895#c10 ? And what should I confirm ;) Do we know why it fails on b2g? Why do we get 'Deprecated CSP header warning' there? Or do we get that warning also on desktop but after 'Dual CSP header warning present'?

My mistake didn't realize the try result I linked above has a different error than other older try results where timeout was the only error. Sorry! On b2g builds I can still see the header we are trying to get rid of in the first place, but desktop build does not. https://tbpl.mozilla.org/?tree=Try&rev=7724536fcbed In this try push, I commented out the code where I assert the existence of the expected header (see this: https://hg.mozilla.org/try/rev/626e47345610) The desktop build timeout but on b2g it first detected the header we DON'T want to see and then timeout. I assume this behavior is due to b2g running on multi-

Okay seems like pushpref has to be set in test too. Looks good so far: https://tbpl.mozilla.org/?tree=Try&rev=e78d24195687 Waiting for full build: https://tbpl.mozilla.org/?tree=Try&rev=6c22b66cc8fb Once all green, check-in will be requested.

+ security.csp.specompliant added b2g tests now passed.

Comment on attachment 8360834 [details] [diff] [review] bug_918397_mutex_csp_warning_v7.patch Please use the checkin-needed bug keyword in the future.