From the CSP spec: "User-agents must not follow HTTP 3xx redirect responses when sending violation reports." We currently follow a 302 response to the report request by sending a GET to the redirected location. To reproduce: 1. Load http://hackmill.com/csp/tests/redir-report-uri-test.php 2. Note the requests: a. GET http://hackmill.com/csp/tests/redir-report-uri-test.php HTTP/1.1 b. POST http://hackmill.com/csp/tests/resources/csp-report.php HTTP/1.1 c. GET http://brandon.sternefamily.net/csp/resources/csp-report HTTP/1.1 c. definitely shouldn't happen.

should this test case still be working ? i don't see the requests in 2b. and 2c. happening - should i only see this in a nightly ?

I'm definitely still seeing the redirect using Nightly: 1 GET http://hackmill.com/csp/tests/redir-report-uri-test.php HTTP/1.1 => HTTP/1.1 200 OK [0.277 s] 5 POST http://hackmill.com/csp/tests/resources/csp-report.php HTTP/1.1 => HTTP/1.1 302 Moved Temporarily [0.149 s] 8 GET http://brandon.sternefamily.net/csp/resources/csp-report HTTP/1.1 => HTTP/1.1 404 Not Found [0.325 s] I'm using an external proxy, though. If you're just using Web Console, those requests won't show up due to bug 639533.

ah ok, i was just using web console, i'll set up an external proxy and check it out. thanks !

this is a first stab at a patch for this.. i'm still learning XPCOM/XPConnect so this might not be the sanest/correctest way of doing this. this bug should also probably have an associated test as well, which i will work on next.

i have a test mostly written for this but bsterne and i are going to rap with jst about new style mochitests to try and do this in the new style if possible and hopefully save an iteration

needs work to remove the use of EnablePrivilege, but the test works with it. will update with a non-EnablePrivilege version.

this patch includes tests modified to use SpecialPowers and has been updated, sending to bsterne for feedback

Comment on attachment 559580 [details] [diff] [review] latest patch >+ // we need to set an nsIChannelEventSink on the XHR object >+ // so we can tell it to not follow redirects when posting the report >+ req.channel.notificationCallbacks = reportChannelEventSink; >+ Can you line the comment up with the rest of the code there? >+var reportChannelEventSink = > ... >+ // nsIInterfaceRequestor >+ getInterface: function requestor_gi(iid) { >+ if (iid.equals(Ci.nsIChannelEventSink)) { >+ try { >+ return this; >+ } catch (e) { do_throw(e); } >+ } >+ throw Cr.NS_ERROR_NO_INTERFACE; >+ }, I'm definitely no authority here, but I haven't seen this pattern before. Why wrap the single return statement? Is there a chance return can throw? Also, do_throw only seems to be used in unit tests, which you appear to have modeled some of this code after. I'd just simplify it by removing the try/catch, more like: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginInstallerService.js#154 >+ // nsIChannelEventSink >+ asyncOnChannelRedirect: function channel_redirect(oldChannel, newChannel, >+ flags, callback) { >+ // cancel the old channel so XHR failure callback happens >+ oldChannel.cancel(Cr.NS_ERROR_ABORT); >+ >+ // notify an observer that we have blocked the report POST due to a redirect, >+ // used in testing, do this async since we're in an async call now to begin with >+ Services.tm.mainThread.dispatch( >+ function() { >+ observerSubject = Cc["@mozilla.org/supports-cstring;1"] >+ .createInstance(Ci.nsISupportsCString); >+ observerSubject.data = oldChannel.URI.asciiSpec; >+ >+ Services.obs.notifyObservers(observerSubject, >+ CSP_VIOLATION_TOPIC, >+ "denied redirect while sending violation report"); >+ }, Ci.nsIThread.DISPATCH_NORMAL); >+ >+ // throw to stop the redirect happening >+ throw Cr.NS_BINDING_REDIRECTED;; >+ } >+}; Looks good. Your test make sense too: 1) create a page with a built-in violation and a report-uri, 2) report-uri responds with a 302, 3) test harness listens for CSP_VIOLATION_TOPIC and the data is the one you expect (the thing about the redirect, not the one about the image being blocked).

patch updated with feedback from bsterne and adding changes from review of bug #674255 (SpecialPowers patch). will try to get 674255 landed and then have this reviewed and landed.

clean up some indentation, ready for review after 674255 lands

tweak the test a little bit based on review tweak to 674255

fix an assertion that crept in with last fix. going to give this a try run.

extra space removed ! waiting for try run to finish..

try run : https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1e5c09ff0cf7 no failures that look related to this change, imo.

unbitrot, 674255 is hopefully about to land so sending this to dveditz for review

Comment on attachment 566935 [details] [diff] [review] patch v9 Review of attachment 566935 [details] [diff] [review]: ----------------------------------------------------------------- This is really close but we need a couple small changes. The only substantive one is logging the blocked redirect to the error console. A reluctant r- General comments on the test: * Looks good for an explicit 302 test. * Doesn't the spec apply equally to 301,303, and 307 redirects? Unless we already have tests for those (seems unlikely or we wouldn't have missed 302) we should add those, too. Yes, this is the paranoia talking, but extra tests are cheap. * There's a push to have more-explicitly named tests rather than just bug numbers which run together. "test_bug650386_reporturi_redirects.html" for example. Or "...302redirect.html" if you put each redirect type in its own test. Separate tests might add clarity if there are failures but I could go either way. Either way you should be able to keep a single file_bug650386.html test file and call with "?redir=301" etc. and have the .sjs file do a conditional response. I hope anyway, haven't tried doing a test like that. Hm, if you combine the tests you can't just test violation==PASS/http-modify==FAIL, you'll have to check which request was which (both pass and fail). My gut feel is that you want a separate test_ host file for each type of redirect to keep the test code simple. * For similar help-the-future-maintainer reasons there's also a push to make sure every test has a comment at the top that explains what it's testing. The bug number can be a good reference but don't force everyone to make that lookup (and some bugs make a terrible reference, or the test only tests part of a bug). A single line "Test that CSP reportURI does not follow 3xx redirects as per spec" or something on those lines is fine. ::: content/base/src/contentSecurityPolicy.js @@ -496,0 +500,37 @@ > > +// the POST of the violation report (if it happens) should not follow > > +// redirects, per the spec. hence, we implement an nsIChannelEventSink here > > +// so we can tell XHR to abort if a redirect happens. > > +var reportChannelEventSink = NaN more ... This notification is good for testing but won't help a site author debug why they aren't getting any reports. We need to also log this error to the console. Consistent with the main CSP sendReports you probably want to use CSPWarning(). As Sid explained the problem is blocked due to correct interpretation of the CSP policy, not a problem with CSP itself or understanding the policy. As a completely separate bug we might want to reconsider which things are warnings and which errors, but for now CSPWarning is most consistent.

> > > +var reportChannelEventSink = > NaN more ... The context was supposed to be the notifyObservers call > Services.obs.notifyObservers(observerSubject, > CSP_VIOLATION_TOPIC, > "denied redirect while sending violation report");

New version of patch that addresses Dan's review feedback : * Adds tests for 301, 303 and 307 redirects as well as 302 redirects * Rename files to give more info about what they're testing * Add comment to test_* files explaining what they are testing * Add CSPWarning() call when a redirect is blocked

Comment on attachment 608152 [details] [diff] [review] patch v10 disregard that r? - i'm gonna try and do this with a test_ file per redirect and 2 .sjs files instead of the way it is now. will r? again when that's done :)

I like this version a lot more - less files, but still preserves the test case for each redirect value.

https://tbpl.mozilla.org/?tree=Try&rev=82bbde692032 the oranges don't look related to this change to me.

Comment on attachment 608215 [details] [diff] [review] patch v11 Review of attachment 608215 [details] [diff] [review]: ----------------------------------------------------------------- A few nits, but overall really good. Unless you've got a reason I'm unaware of, please use the constructor pattern for the reportChannelEventSink (function + prototype -- maybe call it "CSPReportRedirectSink) and I'd like to have another look. Clearing r? flag too, for now. ::: content/base/src/contentSecurityPolicy.js @@ +300,5 @@ > continue; > > var failure = function(aEvt) { > if (req.readyState == 4 && req.status != 200) { > + CSPError("Failed to send report to " + uris[i]); good catch. @@ +500,5 @@ > > +// the POST of the violation report (if it happens) should not follow > +// redirects, per the spec. hence, we implement an nsIChannelEventSink here > +// so we can tell XHR to abort if a redirect happens. > +var reportChannelEventSink = You may want to set this up as a class (function Foo() constructor and a prototype) and instantiate it here. @@ +504,5 @@ > +var reportChannelEventSink = > +{ > + QueryInterface: function requestor_qi(iid) { > + if (iid.equals(Ci.nsISupports) || > + iid.equals(Ci.nsIInterfaceRequestor)) you probably also want "|| iid.equals(Ci.nsIChannelEventSink)" here too since this object is one. @@ +521,5 @@ > + // nsIChannelEventSink > + asyncOnChannelRedirect: function channel_redirect(oldChannel, newChannel, > + flags, callback) { > + CSPWarning("Post of violation report to " + oldChannel.URI.asciiSpec + > + " failed, as a redirect occurred"); nit: please line up string parts (add nine spaces to the beginning of this line). @@ +531,5 @@ > + // used in testing, do this async since we're in an async call now to begin with > + Services.tm.mainThread.dispatch( > + function() { > + observerSubject = Cc["@mozilla.org/supports-cstring;1"] > + .createInstance(Ci.nsISupportsCString); nit: please unindent by one space to be consistent with the rest of the file (line up [ and .) @@ +537,5 @@ > + > + Services.obs.notifyObservers(observerSubject, > + CSP_VIOLATION_TOPIC, > + "denied redirect while sending violation report"); > + }, Ci.nsIThread.DISPATCH_NORMAL); nit: please line up arguments to notifyObservers() ::: content/base/test/file_bug650386_content.sjs @@ +10,5 @@ > +{ > + response.setHeader("Cache-Control", "no-cache", false); > + > + // this gets used in the CSP as part of the report URI. > + var redirect = request.queryString; For catching errors in future code, you may want to validate the queryString here... ensure it's a 30x with no additional cruft. Throw a really nasty error if it's invalid so that if we accidentally break this test in the future it'll be obvious.

Address Sid's feedback - change the nsIChannelEventSink implementor to be CSPReportRedirectSink, clean up the nits, do some general whitespace/indentation cleanup, and change the content .sjs file to do a redirect (which will fail the test) if it gets passed a redirect that isn't covered by these tests.

Makefile bit rotted :( pushed this to try.

Comment on attachment 608905 [details] [diff] [review] patch v13 Review of attachment 608905 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: content/base/src/contentSecurityPolicy.js @@ +547,5 @@ > + // throw to stop the redirect happening > + throw Cr.NS_BINDING_REDIRECTED; > + } > +}; > + Excellent. ::: content/base/test/file_bug650386_content.sjs @@ +6,5 @@ > +// returned/type of redirect to do comes from the query string > +// parameter passed in from the test_bug650386_* files and then also > +// uses that value in the report-uri parameter of the CSP > +function handleRequest(request, response) > +{ really minor nit: please put { on previous line ::: content/base/test/file_bug650386_report.sjs @@ +4,5 @@ > +// This handles 301, 302, 303 and 307 redirects. The HTTP status code > +// returned/type of redirect to do comes from the query string > +// parameter > +function handleRequest(request, response) > +{ same as previous nit: collapse line 7 and 8.

Thanks Sid. This revision addresses those nits and also cleans up more whitespace. r? to jst

Pushed to inbound. http://hg.mozilla.org/integration/mozilla-inbound/rev/2f79b816b0da