Closed Bug 1089255 Opened 5 years ago Closed 4 years ago

Implement manifest-src CSP directive

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 15 obsolete files)

28.13 KB, patch
Details | Diff | Splinter Review
Implement support for manifest-src CSP directive, which controls where manifests can be d/l from.
Blocks: webmanifest
@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
Flags: needinfo?(tanvi)
Actually, Sid and Christoph are the people to ask for Content Security Policy.  Sid's out for a bit; switching needinfo to Christoph.
Flags: needinfo?(tanvi) → needinfo?(mozilla)
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.
Assignee: nobody → mcaceres
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.
Attachment #8528570 - Flags: review?(mozilla)
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.
Attachment #8528570 - Flags: review?(mozilla)
@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));
    }
```
Attachment #8528570 - Attachment is obsolete: true
Attachment #8558915 - Flags: feedback?(mozilla)
Blocks: 1124638
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.
Attachment #8558915 - Flags: feedback?(mozilla) → feedback+
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?
Blocks: CSP
(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
No longer blocks: webmanifest
No longer blocks: 1124638
Depends on: 1083410
While I wait on bug 1083410 to land, are these tests sufficient?
Attachment #8558915 - Attachment is obsolete: true
Attachment #8561919 - Flags: feedback?(mozilla)
Attached file data: manifest testcase v1 (obsolete) —
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 …"
Attached file data: manifest testcase v1.1 (obsolete) —
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."
Attachment #8561938 - Attachment is obsolete: true
(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.
Attachment #8561919 - Flags: feedback?(mozilla) → feedback+
(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.
No longer depends on: 1083410
Blocks: 1083410
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?
Attachment #8561919 - Attachment is obsolete: true
Attachment #8561946 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Flags: needinfo?(dveditz)
Made a mistake in the prev. patch. so replacing...
Attachment #8566394 - Attachment is obsolete: true
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!
Flags: needinfo?(mozilla)
Flags: needinfo?(dveditz)
Attachment #8567793 - Flags: feedback?(dveditz)
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?
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
Attached patch 0001-Obtainer-and-tests.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
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.
Attachment #8567793 - Flags: feedback?(dveditz) → feedback-
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
Duplicate of this bug: 1143201
No longer blocks: 1083410
Depends on: 1083410
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.
Attachment #8567793 - Attachment is obsolete: true
Attachment #8570364 - Attachment is obsolete: true
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.
Depends on: 1162729
(oops, try above was for a different bug!)
Attached patch C++ part of implementation (obsolete) — Splinter Review
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.
Attachment #8600092 - Attachment is obsolete: true
Attachment #8603025 - Flags: review?(dveditz)
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.
Attachment #8603025 - Flags: review?(dveditz) → review-
Component: DOM → DOM: Security
(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.
Attachment #8603025 - Attachment is obsolete: true
Attachment #8606543 - Flags: feedback?(dveditz)
> 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.
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #8606543 - Attachment is obsolete: true
Attachment #8606543 - Flags: feedback?(dveditz)
Attachment #8609039 - Flags: review?(dveditz)
Comment on attachment 8609039 [details] [diff] [review]
Updated patch

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

r=dveditz
Attachment #8609039 - Flags: review?(dveditz) → review+
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?
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
Blocks: webmanifest
@ckerschb, can you please double check that I've added the stuff to nsCSPDirective::toDomCSPStruct(), and to CSPDictionaries.webidl, correctly.
Attachment #8609039 - Attachment is obsolete: true
Attachment #8612873 - Flags: review?(mozilla)
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.
Attachment #8612873 - Flags: review?(mozilla) → review+
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.
Attachment #8612868 - Flags: review?(mozilla)
Attachment #8612873 - Attachment is obsolete: true
Attachment #8613011 - Flags: review?(bobbyholley)
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.
Attachment #8613011 - Flags: review?(bobbyholley) → review+
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
Flags: needinfo?(mozilla)
(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
Flags: needinfo?(mozilla)
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.
Attachment #8612868 - Attachment is obsolete: true
Attachment #8613011 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/6e5683d96cbb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Duplicate of this bug: 1095461
You need to log in before you can comment on or make changes to this bug.