Implement support for manifest-src CSP directive, which controls where manifests can be d/l from.

@tanvi, Ehsan told me you implemented other parts of CSP. I'm wondering if you can help me with adding `manifest-src` as per: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-manifest-src

Actually, Sid and Christoph are the people to ask for Content Security Policy. Sid's out for a bit; switching needinfo to Christoph.

You basically just have to add "manifest-src" to: > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPUtils.h#80 and update > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPUtils.h#61 then create a CSP object and call appendPolicy() adding your policy to CSP: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentSecurityPolicy.idl#51 and finally call shouldLoad(): http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentSecurityPolicy.idl#217 That should be it!

Thanks Christoph, I'll give that a go! \O/. Can you mentor the bug? I'm still a bit of a noob.

(In reply to Marcos Caceres [:marcosc] from comment #4) > Thanks Christoph, I'll give that a go! \O/. Can you mentor the bug? Sure thing. I am observing pretty much everything that happens with CSP anyway.

Just added enum values... not very exciting:) I'm trying to figure out what would need to change in HTMLLinkElement in order for this to kick in.

Comment on attachment 8528570 [details] [diff] [review] 0001-added-CSP-keywords-to-CSP-utils.patch Review of attachment 8528570 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marcos Caceres [:marcosc] from comment #6) > Just added enum values... not very exciting:) Yep, that looks good. > I'm trying to figure out what would need to change in HTMLLinkElement in > order for this to kick in. That sounds great. Clearing review until then, because I would also like to see how it's linked into Gecko.

@ckerschb, I made the changes as you suggested. I'm blocked on something else before I can test this properly tho. Would appreciate any feedback. In my own code, then I have the following which seems to work: ```JS try { var contentPolicy = Cc["@mozilla.org/layout/content-policy;1"]. getService(Ci.nsIContentPolicy); } catch(e) { //Abort if we can't do the check return Promise.reject(e); } if(contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, toNsURI(elem), elem.ownerDocument.documentURIObject, elem, elem.type, null) !== Ci.nsIContentPolicy.ACCEPT){ let msg = "Blocked by CSP."; Promise.reject(new Error(msg)); } ```

Comment on attachment 8558915 [details] [diff] [review] 0002-Added-manifest_src-to-CSP.patch Review of attachment 8558915 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marcos Caceres [:marcosc] from comment #8) > try { > var contentPolicy = Cc["@mozilla.org/layout/content-policy;1"]. > getService(Ci.nsIContentPolicy); > } catch(e) { > //Abort if we can't do the check > return Promise.reject(e); > } > > if(contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, > toNsURI(elem), > elem.ownerDocument.documentURIObject, > elem, elem.type, null) !== > Ci.nsIContentPolicy.ACCEPT){ > let msg = "Blocked by CSP."; > Promise.reject(new Error(msg)); > } Marcos, the code in the patch looks good as far as I can tell from a quick inspection. I am not sure I understand your question. The JS code above also looks ok from a first glance, what is the problem with it exactly? In case you are asking what the best way of testing your feature is, then you can have a look at all the tests in "dom/base/test/csp/". You can also ping me on IRC for a quicker turnaround time.

Also marking this bug as blocking the main CSP bug (Bug 493857) and I think the dependency should be the other way round. Once bug 997779 (manifest support) has landed - we should land the CSP directive, no?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9) > The JS code above also looks ok from a first glance, what is the problem with it exactly? Apologies, there is no problem. I'm just trying to make sure I have everything in place to be able to land this. > In case you are asking what the best way of testing your feature is, then > you can have a look at all the tests in "dom/base/test/csp/". You can also > ping me on IRC for a quicker turnaround time. Thanks! I'll take a look there and see if I can copy some of the tests. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #10) > Also marking this bug as blocking the main CSP bug (Bug 493857) Thanks! > and I think > the dependency should be the other way round. Once bug 997779 (manifest > support) has landed - we should land the CSP directive, no? It's possible, yes. I can try to land them in that order. Right now, I have running and tested as if CSP support was already in place. I'll split this up and send something later today. I also spotted some nice tools in the CSP test above that I can use :) Thanks again for your guidance and patience! Will send a new patch soon and hopefully we can land it :D

While I wait on bug 1083410 to land, are these tests sufficient?

Is this the correct way to test if manifest-src data: works within a page? <meta http-equiv="Content-Security-Policy" content="manifest-src data:"> The "manifest" attribute of the <html> tag contains a URI beginning with "data:text/cache-manifest …"

Fixed 2 validation errors. validator.w3.org still says: "Line 7, Column 74: Bad value Content-Security-Policy for attribute http-equiv on element meta."

(In reply to Mardeg from comment #13) > Created attachment 8561938 [details] > data: manifest testcase v1 > > Is this the correct way to test if manifest-src data: works within a page? > <meta http-equiv="Content-Security-Policy" content="manifest-src data:"> I think so, but I thought that we didn't support http-equiv yet? It's great if we now have support for this! > The "manifest" attribute of the <html> tag contains a URI beginning with > "data:text/cache-manifest …" Please note that this is not a "cache manifest", it's a "web manifest" ^_^ I know, the name is a problem but I lost that battle. So, you instead want the following data URL: data:application/manifest+json;charset=utf-8,{"name"%3A "pass"} Or: data:application/manifest+json;charset=utf-8;base64,eyJuYW1lIjogInBhc3MifQ==

Are you saying the manifest-src directive applies *only* to web manifests and no other manifests? If so, that would be manifestly unfair :( https://developer.mozilla.org/en-US/docs/Web/HTML/Using_the_application_cache#The_cache_manifest_file is where I obtained the info for the cache manifest.

(In reply to Mardeg from comment #16) > Are you saying the manifest-src directive applies *only* to web manifests > and no other manifests? > If so, that would be manifestly unfair :( Heh, I see what you did there:) But yes, it only applies to this one kind of manifest. Please see: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-manifest-src > https://developer.mozilla.org/en-US/docs/Web/HTML/ > Using_the_application_cache#The_cache_manifest_file is where I obtained the > info for the cache manifest. yeah, we are going to deprecate that stuff after Service Workers ship.

(In reply to Marcos Caceres [:marcosc] from comment #17) > > https://developer.mozilla.org/en-US/docs/Web/HTML/ > > Using_the_application_cache#The_cache_manifest_file is where I obtained the > > info for the cache manifest. > > yeah, we are going to deprecate that stuff after Service Workers ship. I see. Thanks for the heads up on that! (In reply to Marcos Caceres [:marcosc] from comment #15) > I thought that we didn't support http-equiv yet? You're right, we don't - bug 663570

Comment on attachment 8561919 [details] [diff] [review] 0001-Bug-1089255-Implement-manifest-src-CSP-directive.patch Review of attachment 8561919 [details] [diff] [review]: ----------------------------------------------------------------- Looks good as far as I can tell. Now, any chance you could split the patch into 2 changesets: a) the actual changes to CSP, and b) the tests. I can review the tests but I would like :dveditz to confirm the nsIContentPolicy.idl changes; the bits about adding a new ContentType. As far as tests, they look good, but we do have a file_csp_testserver.sjs file. Any chance you could reuse that instead of adding a new *.sjs file? ::: dom/base/nsIContentPolicy.idl @@ +174,5 @@ > > + /** > + * Indicates a web manifest request. > + */ > + const nsContentPolicyType TYPE_MANIFEST = 22; Since we do have manifests in firefox os, maybe it makes more sense to actaully rename this to TYPE_WEB_MANIFEST. ::: dom/base/test/csp/browser.ini @@ +1,2 @@ > +[DEFAULT] > +[browser_bug1089255.js] \ No newline at end of file Nit: We are trying to get away from browser_bug* and try to name files based on the testcase within csp/. Maybe [browser_test_web_manifest.js] would be more appropriate. ::: dom/base/test/csp/browser_bug1089255.js @@ +1,2 @@ > +//Used by JSLint: > +/*global SpecialPowers, gBrowser, Components, ise, finish, waitForExplicitFinish*/ In all mochitests within CSP we have a small description of the test. See. e.g: http://mxr.mozilla.org/mozilla-central/source/dom/base/test/csp/test_csp_path_matching_redirect.html?force=1#19 Maybe you could also add a small description how the test works. @@ +11,5 @@ > + 'http://example.org/tests/dom/base/test/csp/file_bug1089255.sjs'; > + const remoteURL = > + 'http://mochi.test:8888/tests/dom/base/test/csp/file_bug1089255.sjs'; > + const tests = [ > + { Maybe you can even add a small description (one line) to each individual test and explain what it's doing, what it's testing and the expected output. ::: dom/base/test/csp/mochitest.ini @@ +1,3 @@ > [DEFAULT] > support-files = > + file_bug1089255.sjs As before, please also rename to match the name of the test rather then the bug number. ::: dom/base/test/moz.build @@ +38,5 @@ > ] > > +BROWSER_CHROME_MANIFESTS += [ > + 'csp/browser.ini' > +] I think you can add 'csp/browser.ini' to the BROWSER_CHROME_MANIFEST array defined underneath. ::: dom/interfaces/security/nsIContentSecurityPolicy.idl @@ +48,4 @@ > const unsigned short BASE_URI_DIRECTIVE = 13; > const unsigned short FORM_ACTION_DIRECTIVE = 14; > const unsigned short REFERRER_DIRECTIVE = 15; > + const unsigned short MANIFEST_SRC_DIRECTIVE = 16; To be consistent, also rename this one to WEB_MANIFEST_SRC_DIRECTIVE; ::: extensions/permissions/nsContentBlocker.cpp @@ +39,5 @@ > "media", > "websocket", > "csp_report", > + "xslt", > + "manifest"}; web_manifest.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19) > Comment on attachment 8561919 [details] [diff] [review] First, thank you for your time, guidance, and the feedback so far! It's been very helpful. > 0001-Bug-1089255-Implement-manifest-src-CSP-directive.patch > > Review of attachment 8561919 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good as far as I can tell. Now, any chance you could split the patch > into 2 changesets: a) the actual changes to CSP, and b) the tests. Will do. > I can > review the tests but I would like :dveditz to confirm the > nsIContentPolicy.idl changes; the bits about adding a new ContentType. > > As far as tests, they look good, but we do have a file_csp_testserver.sjs > file. Any chance you could reuse that instead of adding a new *.sjs file? I'm thinking of redoing the tests, tbh. They are indirectly testing CSP, and I'm worried that this could lead to false positives. I will move these tests back to my Manifest Obtainer API and craft some new ones that actually check directly if CSP rules are applied (and make use of `file_csp_testserver.sjs` in the process!). > ::: dom/base/nsIContentPolicy.idl > @@ +174,5 @@ > > > > + /** > > + * Indicates a web manifest request. > > + */ > > + const nsContentPolicyType TYPE_MANIFEST = 22; > > Since we do have manifests in firefox os, maybe it makes more sense to > actaully rename this to TYPE_WEB_MANIFEST. Done. > ::: dom/base/test/csp/browser.ini > @@ +1,2 @@ > > +[DEFAULT] > > +[browser_bug1089255.js] > \ No newline at end of file > > Nit: We are trying to get away from browser_bug* and try to name files based > on the testcase within csp/. Maybe [browser_test_web_manifest.js] would be > more appropriate. Heh, I had something like that initially, but then saw all the other files named after bug :) I'll do as you suggest. > ::: dom/base/test/csp/browser_bug1089255.js > @@ +1,2 @@ > > +//Used by JSLint: > > +/*global SpecialPowers, gBrowser, Components, ise, finish, waitForExplicitFinish*/ > > In all mochitests within CSP we have a small description of the test. See. > e.g: > http://mxr.mozilla.org/mozilla-central/source/dom/base/test/csp/ > test_csp_path_matching_redirect.html?force=1#19 > Maybe you could also add a small description how the test works. Will do. > @@ +11,5 @@ > > + 'http://example.org/tests/dom/base/test/csp/file_bug1089255.sjs'; > > + const remoteURL = > > + 'http://mochi.test:8888/tests/dom/base/test/csp/file_bug1089255.sjs'; > > + const tests = [ > > + { > > Maybe you can even add a small description (one line) to each individual > test and explain what it's doing, what it's testing and the expected output. Will do. > ::: dom/base/test/csp/mochitest.ini > @@ +1,3 @@ > > [DEFAULT] > > support-files = > > + file_bug1089255.sjs > > As before, please also rename to match the name of the test rather then the > bug number. > > ::: dom/base/test/moz.build > @@ +38,5 @@ > > ] > > > > +BROWSER_CHROME_MANIFESTS += [ > > + 'csp/browser.ini' > > +] > > I think you can add 'csp/browser.ini' to the BROWSER_CHROME_MANIFEST array > defined underneath. Ha! good catch. Totally missed that other one being there :) > ::: dom/interfaces/security/nsIContentSecurityPolicy.idl > @@ +48,4 @@ > > const unsigned short BASE_URI_DIRECTIVE = 13; > > const unsigned short FORM_ACTION_DIRECTIVE = 14; > > const unsigned short REFERRER_DIRECTIVE = 15; > > + const unsigned short MANIFEST_SRC_DIRECTIVE = 16; > > To be consistent, also rename this one to WEB_MANIFEST_SRC_DIRECTIVE; Fixed. > ::: extensions/permissions/nsContentBlocker.cpp > @@ +39,5 @@ > > "media", > > "websocket", > > "csp_report", > > + "xslt", > > + "manifest"}; > > web_manifest. Fixed.

Hi Christoph, I've split out the implementation from the tests. I will redo the tests over the next few days, but in the mean time, perhaps :dveditz can check the nsIContentPolicy.idl changes?

Made a mistake in the prev. patch. so replacing...

Comment on attachment 8567793 [details] [diff] [review] 0001-Bug-1089255-Implement-manifest-src-CSP-directive.patch Looks good to me. Clearing the needinfo and flagging Dan for feedback. Please flag me/us for review once you have the tests ready. Thanks for working on this!

Hi Christoph, For some reason, I don't seem to be having much joy getting the underlying CSP machinery to work with the manifest-src directive. In my code, I have the following check: ```JS var contentPolicy = Cc['@mozilla.org/layout/content-policy;1'].getService(Ci.nsIContentPolicy); //Can it load? var shouldLoad = contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST, toNsURI(elem), elem.ownerDocument.documentURIObject, elem, elem.type, null); if (shouldLoad !== Ci.nsIContentPolicy.ACCEPT) { let msg = 'Blocked by CSP.'; return reject(new Error(msg)); } ``` The above catches all violations for the "default-src" (e.g., default-src 'none' etc.) and notifies `SpecialPowers` as per: ``` SpecialPowers.addObserver(observer, 'csp-on-violate-policy', false); ``` However, it doesn't seem to pick up any for "manifest-src" directives :( The CSP parser seems happy to accept the "manifest-src" directives (it doesn't complain that it's unknown), but just doesn't know how to block "manifest" things (in my case, in the above "elem" is a HTMLLinkElement). Any ideas what I be missing?

(In reply to Marcos Caceres [:marcosc] from comment #24) > Hi Christoph, > For some reason, I don't seem to be having much joy getting the underlying > CSP machinery to work with the manifest-src directive. > > In my code, I have the following check: > > ```JS > var contentPolicy = > Cc['@mozilla.org/layout/content-policy;1'].getService(Ci.nsIContentPolicy); > > //Can it load? > var shouldLoad = > contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST, > toNsURI(elem), > > elem.ownerDocument.documentURIObject, > elem, > elem.type, > null); > > if (shouldLoad !== Ci.nsIContentPolicy.ACCEPT) { > let msg = 'Blocked by CSP.'; > return reject(new Error(msg)); > } > ``` > > The above catches all violations for the "default-src" (e.g., default-src > 'none' etc.) and notifies `SpecialPowers` as per: > > ``` > SpecialPowers.addObserver(observer, 'csp-on-violate-policy', false); > ``` > > However, it doesn't seem to pick up any for "manifest-src" directives :( The > CSP parser seems happy to accept the "manifest-src" directives (it doesn't > complain that it's unknown), but just doesn't know how to block "manifest" > things (in my case, in the above "elem" is a HTMLLinkElement). > > Any ideas what I be missing? It's hard to tell given only the small code snipped provided (which seems fine at a first glance). What does the policy look like for your setup? Can you attach the whole testcase, then it would be easier for me to reason what's wrong with it.

Hi Christoph, Apologies, I've attached both the tests and the code which invokes nsIContentPolicy's .shouldLoad(). The "Obtainer" code is fairly short, so hopefully it should be easy to follow. Basically, given a HTMLLinkElement, it checks if CSP can load the URL it points to. If not, it rejects a promise with a "Blocked by CSP" error. All the "default-src" tests work as expected, but any with "manifest-src" fail :( Hope that all makes sense.

(please note that I generated that attachment from my working directory, so it contains some bugs - however, it should give you an idea of what I'm trying to test)

Still hard to evaluate :-) but what I think might be going on is that all the tests that only include either > default-src 'none' or > default-src mochi.test allow some additional JS to be loaded or blocked (remember script-src falls back to default-src if not defined ecplicitly within a policy). To check, you could add 'script-src * unsafe-inline unsafe-eval; to your policy. The better approach to figure what's going on is to use PR_LOGGING, which you can enable by using the following two commands: > export NSPR_LOG_MODULES=CSPContext:5,CSPUtils:5,CSPParser:5 > export NSPR_LOG_FILE=/home/ckerschb/Desktop/log.txt If that all doesn't work, please attach a full test-patch (one that I can run on my machine, e.g. by adding the tests to csp/mochitest.ini and such). All in all, I think manifest-src itself works correctly, probably there is just some problem with your test setup.

Comment on attachment 8567793 [details] [diff] [review] 0001-Bug-1089255-Implement-manifest-src-CSP-directive.patch Review of attachment 8567793 [details] [diff] [review]: ----------------------------------------------------------------- Is there more to this patch? I see where you parse the new CSP directive in a CSP header, but I don't see where you do anything with it in the CSP implementation. At some point people call ShouldLoad() and a new case needs to be added for this content type to compare those requests against the newly parsed source list. Speaking of which... if this is the patch that adds TYPE_WEB_MANIFEST to nsIContentPolicy (base) then you can't already be using it in a load check anywhere. That means there's no way CSP could regulate that content type even if there were an implementation that tried to. ::: dom/base/nsIContentPolicyBase.idl @@ +174,5 @@ > > + /** > + * Indicates a web manifest. > + */ > + const nsContentPolicyType TYPE_WEB_MANIFEST = 22; You've missed several of our built-in nsIContentPolicy implementations. For instance the mixed-content blocker needs to know about this new type (I would consider this "active" content when you get there). Basically everything that implements nsIContentPolicy. Several add-ons have nsIContentPolicy implementations (notably ad blockers & NoScript) so we should also get this addition published in whatever developer notes we have for a release. Might be the release notes, but I think we have something more developer oriented.

I started this etherpad on Gecko Content Policies. I looked into a few and documented what they appeared to be about. Perhaps it will help when updating them all to handle TYPE_WEB__MANIFEST. https://etherpad.mozilla.org/ContentPolicies

I started trying to work on this again, but I'm still stuck. I've attached a patch that can be applied. From nsIContentPolicy.shouldLoad(), I'm getting a "-2" - which is a Ci.nsIContentPolicy.REJECT_TYPE. I don't know enough C++ to debug the thing, unfortunately - so once it leaves JS land it becomes a black box to me. Would appreciate some guidance.

The core of what I'm trying to do is in dom/ipc/manifestMessages.js. Basically, I want to check the CSP there to see if the manifest should be loaded.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e02f14b20

(oops, try above was for a different bug!)

Ok, I got it working locally with a bit of c++ help from @eshan. I'm sending this in a few parts as, as there are quite a few test files, etc. Starting with just the c++ implementation. From my testing, this will now respect the manifest-src directives and mixed content blocking.

Comment on attachment 8603025 [details] [diff] [review] C++ part of implementation Review of attachment 8603025 [details] [diff] [review]: ----------------------------------------------------------------- Going to have to r- for concerns over the dbschema thing, but this is going in the right direction (assuming something actually checks manifest loads against the content policies?). ::: docshell/base/nsIDocShell.idl @@ +267,5 @@ > > /** > + * Attribute stating whether or not web manifests should be loaded. > + */ > + attribute boolean allowWebManifest; Don't we require updating the UUID when we change an interface? (adding a constant, as you do in later .idl files, doesn't require a UUID change since it doesn't affect binary compatibility.) ::: dom/base/nsIContentPolicyBase.idl @@ +180,3 @@ > /* When adding new content types, please update nsContentBlocker, > * NS_CP_ContentTypeName, nsCSPContext, all nsIContentPolicy > * implementations, and other things that are not listed here that are You're adding a type, but missed places that need to be updated to know about the new type per the comment. dom/fetch/InternalRequest.cpp The comment here is worrying: https://mxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#121 You'll get failures from that assert with this patch, but you can't just add to that assertion without (apparently) updating the cache schema. That will involve talking to networking folks. It's possible it should be rejected in dom/base/nsDataDocumentContentPolicy.cpp ::: extensions/permissions/nsContentBlocker.cpp @@ +39,5 @@ > "media", > "websocket", > "csp_report", > + "xslt", > + "manifest"}; This routine fails if the passed-in content type is bigger than the number of known types in this array, but fails benignly by returning NS_OK. You should update the check at https://dxr.mozilla.org/mozilla-central/source/extensions/permissions/nsContentBlocker.cpp#139 to assert or at least NS_WARNING in that case. Sadly when ShouldProcess() is called rather than ShouldLoad() then the array lookup is going to be out of bounds and possibly crash before ShouldProcess calls ShouldLoad and checks the array size. So thank you for updating nsContentBlocker as the comment in nsIContentPolicyBase.idl says, but clearly since others don't/haven't we need to have more aggressive checking here. You should add "beacon","fetch","imageset", between xslt and manifest.

(In reply to Daniel Veditz [:dveditz] from comment #37) > Comment on attachment 8603025 [details] [diff] [review] > C++ part of implementation > > Review of attachment 8603025 [details] [diff] [review]: > ----------------------------------------------------------------- > > Going to have to r- for concerns over the dbschema thing, but this is going > in the right direction (assuming something actually checks manifest loads > against the content policies?). Talked to Ehsan who said that the dbschema thing should be fine by adding web manifest to the list there. > ::: docshell/base/nsIDocShell.idl > @@ +267,5 @@ > > > > /** > > + * Attribute stating whether or not web manifests should be loaded. > > + */ > > + attribute boolean allowWebManifest; > > Don't we require updating the UUID when we change an interface? (adding a > constant, as you do in later .idl files, doesn't require a UUID change since > it doesn't affect binary compatibility.) Done. Was not aware of this, so good to know. :) > ::: dom/base/nsIContentPolicyBase.idl > @@ +180,3 @@ > > /* When adding new content types, please update nsContentBlocker, > > * NS_CP_ContentTypeName, nsCSPContext, all nsIContentPolicy > > * implementations, and other things that are not listed here that are > > You're adding a type, but missed places that need to be updated to know > about the new type per the comment. > > dom/fetch/InternalRequest.cpp Added check here. > The comment here is worrying: > https://mxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#121 > You'll get failures from that assert with this patch, but you can't just add > to that assertion without (apparently) updating the cache schema. That will > involve talking to networking folks. > > It's possible it should be rejected in > dom/base/nsDataDocumentContentPolicy.cpp Ok, have not added any check here yet (but makes sense)... will add something. > ::: extensions/permissions/nsContentBlocker.cpp > @@ +39,5 @@ > > "media", > > "websocket", > > "csp_report", > > + "xslt", > > + "manifest"}; > > This routine fails if the passed-in content type is bigger than the number > of known types in this array, but fails benignly by returning NS_OK. You > should update the check at > https://dxr.mozilla.org/mozilla-central/source/extensions/permissions/ > nsContentBlocker.cpp#139 to assert or at least NS_WARNING in that case. > Sadly when ShouldProcess() is called rather than ShouldLoad() then the array > lookup is going to be out of bounds and possibly crash before ShouldProcess > calls ShouldLoad and checks the array size. > > So thank you for updating nsContentBlocker as the comment in > nsIContentPolicyBase.idl says, but clearly since others don't/haven't we > need to have more aggressive checking here. > > You should add "beacon","fetch","imageset", between xslt and manifest. Added the three above.

> dom/base/nsDataDocumentContentPolicy.cpp About the above, should I just add WEB_MANIFEST to?: https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDataDocumentContentPolicy.cpp#121 I'm a bit lost in there :(

(In reply to Marcos Caceres [:marcosc] from comment #39) > > dom/base/nsDataDocumentContentPolicy.cpp > > About the above, should I just add WEB_MANIFEST to?: > https://mxr.mozilla.org/mozilla-central/source/dom/base/ > nsDataDocumentContentPolicy.cpp#121 > > I'm a bit lost in there :( Since manifests cannot be loaded by documents similar to the way we load images/scripts/etc, there is no point in checking for this type in nsDataDocumentContentPolicy, but I guess we might as well do that for hygiene reasons.

When through this patch with Ehsan. We dropped the "allowWebManifest" from nsIDocShell.idl, as it didn't have any utility in practice. I think all the feedback has been addressed - hopefully we didn't miss anything.

Comment on attachment 8609039 [details] [diff] [review] Updated patch Review of attachment 8609039 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz

Comment on attachment 8609039 [details] [diff] [review] Updated patch Review of attachment 8609039 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/nsCSPUtils.h @@ +77,5 @@ > "reflected-xss", // REFLECTED_XSS_DIRECTIVE > "base-uri", // BASE_URI_DIRECTIVE > "form-action", // FORM_ACTION_DIRECTIVE > + "referrer", // REFERRER_DIRECTIVE > + "manifest-src" // MANIFEST_SRC_DIRECTIVE Drive by note, please not only add manifest-src here but also add it to nsCSPDirective::toDomCSPStruct() so the devtool keeps working correctly. Thank you!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #43) > Comment on attachment 8609039 [details] [diff] [review] > Updated patch > > Review of attachment 8609039 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/security/nsCSPUtils.h > @@ +77,5 @@ > > "reflected-xss", // REFLECTED_XSS_DIRECTIVE > > "base-uri", // BASE_URI_DIRECTIVE > > "form-action", // FORM_ACTION_DIRECTIVE > > + "referrer", // REFERRER_DIRECTIVE > > + "manifest-src" // MANIFEST_SRC_DIRECTIVE > > Drive by note, please not only add manifest-src here but also add it to > nsCSPDirective::toDomCSPStruct() so the devtool keeps working correctly. > Thank you! Yeah, I spotted that yesterday when rebasing :) I've added it - updated patch coming soon. Can you point me to some example tests for that?

(In reply to Marcos Caceres [:marcosc] from comment #44) > > Drive by note, please not only add manifest-src here but also add it to > > nsCSPDirective::toDomCSPStruct() so the devtool keeps working correctly. > > Thank you! > > Can you point me to some example tests for that? There are no automated tests - only manual testing. If you open the developer console and type 'security csp' you should see feedback about the CSP of the page. If you have a page that uses your newly added 'manifest' it should be displayed/included in the report.

@ckerschb, can you please double check that I've added the stuff to nsCSPDirective::toDomCSPStruct(), and to CSPDictionaries.webidl, correctly.

Comment on attachment 8612873 [details] [diff] [review] Added code for nsCSPDirective::toDomCSPStruct() + enhanced comment about how to add that Review of attachment 8612873 [details] [diff] [review]: ----------------------------------------------------------------- I think Dan already reviewed and r+ed that patch, no? The changes to ::toDomCSPStruct() are ok, but you need review from a dom-peer for the webidl changes otherwise you can not land the patch. (Please make sure the name of the reviewer for the dom changes also appears in your commit message, .e.g. [Bug - bla bla r=(dveditz,bholley]). You could ask bholley to accept the webidl changes - he also reviewed the initial bits for those. r=me on the /security/csp changes. ::: dom/base/nsIContentPolicyBase.idl @@ +24,4 @@ > * by launching a dialog to prompt the user for something). > */ > > +[scriptable,uuid(0aed0785-d5f9-4afe-86fb-8d2f5a544813)] No need to change the uuid if you are just adding a const variable.

Comment on attachment 8612868 [details] [diff] [review] Updated tests for manifest-src based on previous review feedback. Review of attachment 8612868 [details] [diff] [review]: ----------------------------------------------------------------- Didn't dveditz already take a look at that patch? If not, please re-flag me for review and I can take a look.

Comment on attachment 8613011 [details] [diff] [review] Reverted UUID change to idl file. Bobby, can you please review the IDL/WebIDL? Review of attachment 8613011 [details] [diff] [review]: ----------------------------------------------------------------- rs=me on the idl/webidl changes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=278a138feef3

Christoph, sorry to bother you again but I'm getting test failures in e10s because the 'csp-on-violate-policy' message doesn't seems to call back the observer [1]. i.e., the following "observer" function never gets called in e10s - while it works perfectly in non-e10s mode (again see [1], "bc2" tests, where they ran in both modes): ``` ... this.observe = function observer(subject, topic) { SpecialPowers.removeObserver(this, 'csp-on-violate-policy'); test.run(topic); finishedTest(); success = true; }; SpecialPowers.addObserver(this, 'csp-on-violate-policy', false); ... ``` Is e10s compat with this stuff a known issue (or is it something I'm doing wrong)? In my code, I can see that everything else works as expected (CSP is actually blocking requests, as it should), just the 'csp-on-violate-policy' message is never sent back - so my tests are timing out waiting for the message. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8735168b26bb

(In reply to Marcos Caceres [:marcosc] from comment #53) > Is e10s compat with this stuff a known issue (or is it something I'm doing > wrong)? In my opinion it shouldn't make a difference, but all the CSP tests that use 'csp-on-violate-policy' are plain mochitests and not browser tests. The only thing I am aware of is that 'http-on-opening-request' observers do not work in e10s [1]. Other than that we use 'csp-on-violate-policy' quite often and none of the tests that's using it is disabled for e10s, so it should work. At the moment, I can't tell why it's not working. Maybe some folks from the browser team can be of more help here than me. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/test/csp/mochitest.ini#142

Hmm... I can get 'http-on-modify-request' to respond in e10s, but 'csp-on-violate-policy' doesn't seem to at all :( Maybe, as you say, it is a problem related to being a browser test. > Maybe some folks from the browser team can be of more help here than me. Not sure who to ask about this?

I think I'm just going to disable or TODO these tests in e10s, as it's only the tests that are affected. The actual functionality works fine in e10s, so I would prefer not to block on this.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #54) > (In reply to Marcos Caceres [:marcosc] from comment #53) > > Is e10s compat with this stuff a known issue (or is it something I'm doing > > wrong)? > > In my opinion it shouldn't make a difference, but all the CSP tests that use > 'csp-on-violate-policy' are plain mochitests and not browser tests. > > The only > thing I am aware of is that 'http-on-opening-request' observers do not work > in e10s [1]. Other than that we use 'csp-on-violate-policy' quite often and > none of the tests that's using it is disabled for e10s, so it should work. Ok, so talking to Ehsan, the problem is that 'csp-on-violate-policy' is only sent to the child process, not the parent process. Browser tests run in the parent process so can't receive this message.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e949fe7daa8

Good news, "csp-on-violate-policy" can be easily enabled in e10s + browser tests. Prepared patch already and works locally: https://bugzilla.mozilla.org/show_bug.cgi?id=1170385

Documentation updated: https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives and https://developer.mozilla.org/en-US/Firefox/Releases/41#Security

The change to nsIContentPolicy was missed in the previous documentation pass. I have now updated https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPolicy and https://developer.mozilla.org/en-US/Firefox/Releases/41.