I guess we can save some cycles by removing some code in ShouldProcess [1], which bascially turns into a no-op when calling [2]. The same thing is going to happen once the new implementation landed, so we should clean that up. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#207 [2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#878

Removal of dead code for CSP. We can save a function call by removing shouldProcess from nsIContentSecurityPolicy.idl and also nsCSPContext.cpp. Currently, shouldProcess always accepts the load, hence no need to have that function call. I think, the removal of the old CSP implementation [1] is blocking this bug - we could land this right away. Sid, what do you think? [1] https://bugzilla.mozilla.org/show_bug.cgi?id=994782

I agree with the shortcutting. ShouldProcess is used after load begins to determine if the content should be used (mostly for images). Since CSP is intended to block initial requests, I think this approach is fine. One lingering question (before I review the patch) is how this affects cached content. Is there an easy way to check to see if this will allow cached resources to be used despite a CSP? For example, if I have http://resources.com/script.js cached, and a page with CSP prohibiting scripts from resources.com loads, will it load?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2) > I agree with the shortcutting. ShouldProcess is used after load begins to > determine if the content should be used (mostly for images). Since CSP is > intended to block initial requests, I think this approach is fine. > > One lingering question (before I review the patch) is how this affects > cached content. Is there an easy way to check to see if this will allow > cached resources to be used despite a CSP? For example, if I have > http://resources.com/script.js cached, and a page with CSP prohibiting > scripts from resources.com loads, will it load? Shortcutting (removing) shouldProcess will not affect any of the current caching behavior. If you look at the current implmentation [1], all it does is, finding the correct CSP and then returning true (allow). Besides wasting cycles, shouldProcess is not performing any task and does not influence caching at all. To answer your question, if your example scenario is indeed a problem, it is a problem with or without that patch. The shortcutting will not influence the result of the load. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#218

I agree that this change won't make our implementation behave differently. The reason I asked was because it makes more sense to fix shouldProcess than to remove it if this is a bug in our implementation. Is there an easy way for you to test if our current impl does the right thing with cached content?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #4) > I agree that this change won't make our implementation behave differently. > The reason I asked was because it makes more sense to fix shouldProcess than > to remove it if this is a bug in our implementation. Is there an easy way > for you to test if our current impl does the right thing with cached content? Are you saying you are afraid that there probably is a bug in the current CSP implementation (JS and C++) and shouldProcess is probably not doing the right thing? Maybe you are right, shouldProcess should be called after the resource is loaded and hence it doesn't make sense that CSP always returns true. Interesting is that MCB shouldProcess internally calls shouldLoad. Unfortunately, I don't think there is an easy way to test/verify.

Yes, that's what I was worried about. I think it's working, though. I did a manual test: 1. Set up a web server with a page (x.html) that references a script (y.js). Server serves same content on a.local and b.local. 2. Set up CSP for x.html to allow only scripts from 'self'. 3. Edited x.html to reference absolute URI http://a.local/y.js 4. Set caching headers on y.js to be public and not expire for a few days. 5. Loaded a.local/x.html in Firefox (script ran) 6. Loaded a.local/x.html again, script ran, webserver log showed no connection to fetch y.js 7. Loaded b.local/x.html, script blocked by CSP. No connection to fetch a.local/y.js. So caching is working, and CSP is blocking the load even from cache, it seems.

Comment on attachment 8451750 [details] [diff] [review] bug_1005225.patch Review of attachment 8451750 [details] [diff] [review]: ----------------------------------------------------------------- This patch is reasonable given the test of cached loads I did, at least for scripts. It is still possible that _image_ or _object_ loads may bypass the check but that is less of a problem than scripts, and I think we should test that before landing this so we don't change the idl twice if we have to put shouldload back in. So DO NOT LAND this patch until after we've tested if it is a bypass via cache for other content types.

I've verified that images are also blocked by CSP from cache. I think the risk of other content types being allowed is minimal, but we need a follow-up bug to investigate (and perhaps to create automated tests for this). This patch is ok to land to remove that unnecessary shouldProcess implementation.

This is the test PHP script I used. It loads resources that send strong cache hints. alpha.local and bravo.local both point to the server on localhost. Since the URLs in the test refer to alpha.local, the resources should load on alpha.local/test.php but not bravo.local/test.php.

Unbitrotted the patch, pushed to try [1], just to make sure everything is fine. Carrying over r+ from sstamm. I will land this tomorrow morning. [1] https://tbpl.mozilla.org/?tree=Try&rev=0cf5cb4cd3b5

https://hg.mozilla.org/integration/mozilla-inbound/rev/61d41b381f30 Needs a follow-up, where we revert the uuid of the nsIContentPolicy again, will do that in a minute - shouldn't break anything.

As discussed with RyanVM on IRC, I pushed a follow up to revert the uuid in nsIContentPolicy.idl and changed the uuid for nsIContentSecurityPolicy.idl. https://hg.mozilla.org/integration/mozilla-inbound/rev/452d8502bea1

Both backed out (along with bug 1005225) in https://hg.mozilla.org/integration/mozilla-inbound/rev/28dfed913436 for turning B2G's Gu test red: https://tbpl.mozilla.org/php/getParsedLog.php?id=43859708&tree=Mozilla-Inbound

Since this got backed out I can combine the follow-up with the original patch. Carrying over r+ from Sid. I don't think this patch caused Gu to turn red, but rather bug [1]. Also what Sid wrote for bug 1030936 in Comment 15 [1] rather indicates that the other patch was responsbile for the perma-red. To make sure, I pushed my changes to try again [2], once everything turns green on B2G, I will reland that patch tomorrow and take a closer look at what causes the problem in bug 1030936. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1030936#c15 [2] https://tbpl.mozilla.org/?tree=Try&rev=209baf5aabf1

Re-landing that patch since TRY turned green (in particular Gu, which was in question). Also, the discussions Sid and I had in person indicate that bug 1030936 introduces the perma-red on Gu. https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd6bf6c57cb