Closed Bug 1207394 Opened 5 years ago Closed 4 years ago

Make sure web_accessible_resources work with CSP/mixed content blocking

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: billm, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(1 file)

Based on the comments in bug 792479, I'm worried that resources listed in the web_accessible_resources section of the extension manifest still won't be loaded on pages with restrictive content security policies or due to mixed content blocking. We need to test this and fix any bugs that arise.

For mixed content blocking we have the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT flag. CSP will probably be difficult to fix, though.
Flags: needinfo?(wmccloskey)
Blocks: webext
Flags: blocking-webextensions+
Flags: needinfo?(wmccloskey)
Whiteboard: triaged
Priority: -- → P3
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
I manually verified that we can load an image which is a web_accessible_resource, into an `img` tag on a page with a restrictive CSP. Specifically, I used the page at [1], which blocks images from outside the page's domain, and then used the beastify example web extension to load a new image on the page, and it worked.

Now we need to have some tests that verify that this works. Specifically we need tests for:

* Load an img into a page with a CSP that would normally forbid loading images.
* Load and run a script on a page with a CSP that would normally forbid loading and running scripts.
* Load an img into a page with a CSP that would normally forbid passive mixed content.

[1] http://www.cspplayground.com/violation_examples?csp_mode=enable#image
This includes tests for loading web accessible images and scripts into a page with a restrictive CSP

Review commit: https://reviewboard.mozilla.org/r/45783/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45783/
Attachment #8740475 - Flags: review?(kmaglione+bmo)
The attached patch addresses two of the above cases, namely:

* Load an img into a page with a CSP that would normally forbid loading images.
* Load and run a script on a page with a CSP that would normally forbid loading and running scripts.

One thing that is missing from the test is an explicit verification that the image loaded successfully. Any suggestions for doing that would be welcome.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/1-2/
I added a test for mixed content as well, but it has the same issue as the test I originally added: I am not actually verifying that the image was loaded successfully.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/2-3/
I'm using img.addEventListener("load") to verify that the images were able to load properly.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

https://reviewboard.mozilla.org/r/45783/#review42419

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:18
(Diff revision 2)
>  <script type="text/javascript">
>  "use strict";
>  
>  /* eslint-disable mozilla/balanced-listeners */
>  
> +SimpleTest.registerCleanupFunction(() => { SpecialPowers.clearUserPref("security.mixed_content.block_display_content"); });

New line after the opening brace please.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:188
(Diff revision 2)
> +  function testScript() {
> +    window.postMessage("test-script-loaded", "*");
> +  }
> +
> +  const IMAGE = atob("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAA" +
> +  "ACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=");

Please align with the opening paren.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:221
(Diff revision 2)
> +  let cspEventCount = 0;
> +
> +  examiner.prototype = {
> +    observe: function(subject, topic, data) {
> +      cspEventCount++;
> +      let asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");

Please use `spec` rather than `asciiSpec`.

Also, this would probably be a bit clearer as:

    SpecialPowers.wrap(subject).QueryInterface(SpecialPowers.Ci.nsIURI).spec

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:223
(Diff revision 2)
> +  examiner.prototype = {
> +    observe: function(subject, topic, data) {
> +      cspEventCount++;
> +      let asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");
> +      ok(asciiSpec.includes("file_image_bad.png") || asciiSpec.includes("file_script_bad.js"),
> +         `Expected file: ${asciiSpec} rejected by CSP`);

Please align with opening paren.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:226
(Diff revision 2)
> +      let asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");
> +      ok(asciiSpec.includes("file_image_bad.png") || asciiSpec.includes("file_script_bad.js"),
> +         `Expected file: ${asciiSpec} rejected by CSP`);
> +    },
> +
> +    // must eventually call this to remove the listener,

Please capitalize.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:233
(Diff revision 2)
> +    remove: function() {
> +      SpecialPowers.removeObserver(this, "csp-on-violate-policy");
> +    },
> +  };
> +
> +  window.examiner = new examiner();

Why does this need to be assigned to a property of the window?

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:252
(Diff revision 2)
> +
> +add_task(function* test_web_accessible_resources_mixed_content() {
> +  function background() {
> +    browser.runtime.onMessage.addListener((msg, sender) => {
> +      if (msg === "content-ready") {
> +        browser.tabs.sendMessage(sender.tab.id, "load-resources");

Why is this necessary?

Also, you can just send a reply to the original message, rather than sending a new message.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:259
(Diff revision 2)
> +      }
> +      if (msg === "accessible-script-loaded") {
> +        browser.test.notifyPass("mixed-test");
> +        return;
> +      }
> +      browser.test.sendMessage(msg);

Can we make this an `if-else if-else`?

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:270
(Diff revision 2)
> +  function content() {
> +    browser.runtime.onMessage.addListener(msg => {
> +      let testImage = document.createElement("img");
> +      testImage.setAttribute("src", browser.extension.getURL("image.png"));
> +      document.body.appendChild(testImage);
> +      browser.runtime.sendMessage("image-loaded");

Can you add a bit more whitespace to this function?

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:279
(Diff revision 2)
> +      browser.runtime.sendMessage("script-loaded");
> +    });
> +
> +    window.addEventListener("message", function rcv(event) {
> +      browser.runtime.sendMessage(event.data);
> +    }, false);

No need for the `false`. It's the default.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:289
(Diff revision 2)
> +  function testScript() {
> +    window.postMessage("accessible-script-loaded", "*");
> +  }
> +
> +  const IMAGE = atob("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAA" +
> +  "ACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=");

Please align.
Attachment #8740475 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/45783/#review42425

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:169
(Diff revisions 2 - 3)
>      browser.runtime.onMessage.addListener(msg => {
>        let testImage = document.createElement("img");
>        testImage.setAttribute("src", browser.extension.getURL("image.png"));
> -      document.body.appendChild(testImage);
> +      testImage.addEventListener("load", () => {
> -      browser.runtime.sendMessage("image-loaded");
> +        browser.runtime.sendMessage("image-loaded");
> +      });

Please also check that this same test fails when you expect the image to be blocked.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/3-4/
Attachment #8740475 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/45783/#review42419

> Please align with opening paren.

I'm not understanding this one. Line 222 is "ok(....." and line 223 is "   `Expected...". Isn't that backtick lined up with the open paren above it?

> Why does this need to be assigned to a property of the window?

It doesn't seem to. That's what was done in the code I borrowed this from, so I assumed it needed to be.

> Why is this necessary?
> 
> Also, you can just send a reply to the original message, rather than sending a new message.

You're right, it's totally unnecessary. Fixed!
FYI, I am working on some improvements as suggested by Luca. I should have a new version up sometime today.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/4-5/
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/5-6/
Not sure if this is related but I have the following scenario (because of limited access I am not able to give a complete test case)

Basically what I am doing is injecting a script into a specific page as I need to read an object from the document
Based on the answer here: http://stackoverflow.com/a/20513730

This works as expected in Google Chrome, but Firefox gives the following 'error' message:

"Security Error: Content at https://sub.domain.tld/secure/RapidBoard.jspa?rapidView=134&view=planning.nodetail may not load or link to moz-extension://655db9c7-4cd2-9040-874d-e947241eadfc/content-jira.js"


My code,

manifest.json

  ...
  "web_accessible_resources" : ["/content-jira.js"],
  "content_scripts": [
    {
      "matches": ["https://sub.domain.tld/*"],
      "js": ["/content.js"]
    }
  ],
  ...


content.js

function injectScript( file, node ) {
    var th = document.getElementsByTagName( node )[0];
    var s = document.createElement( 'script' );
    s.setAttribute( 'type', 'text/javascript' );
    s.setAttribute( 'src', file );
    th.appendChild( s );
}
injectScript( chrome.extension.getURL( '/content-jira.js' ), 'body' );


content-jira.js
some standard DOM operations
(In reply to Thomas Andersen from comment #15)

I believe the problem is with the leading slash in your definition of `web_accessible_resources`. Try changing:

>   "web_accessible_resources" : ["/content-jira.js"],

to:

"web_accessible_resources" : ["content-jira.js"],

and please let me know if that fixes it.
Kris, I can confirm that this does work in Chrome, with the leading slash. Should we consider this a bug?
Flags: needinfo?(kmaglione+bmo)
Answering my own question, I think we should fix this for Chrome parity. I have opened bug 1264623.
Flags: needinfo?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/45783/#review42419

> I'm not understanding this one. Line 222 is "ok(....." and line 223 is "   `Expected...". Isn't that backtick lined up with the open paren above it?

Yeah. I'm guessing reviewboard did something weird the first time I looked at it.
Attachment #8740475 - Flags: review?(kmaglione+bmo)
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

https://reviewboard.mozilla.org/r/45783/#review43143

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:27
(Diff revision 6)
> +let image = atob("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAA" +
> +                 "ACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=");
> +const IMAGE_ARRAYBUFFER = Uint8Array.from(image, byte => byte.charCodeAt(0)).buffer;
> +
> +function contentScriptHelper() {
> +  this.testImageLoading = function(src, expectedAction) {

Please don't rely on `this` pointing to the global in a bare function call. That doesn't work in strict mode. I'm actually a bit surprised it works here, since the body of this function is in strict mode.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:188
(Diff revision 6)
>  });
> +
> +add_task(function* test_web_accessible_resources_csp() {
> +  function background() {
> +    browser.runtime.onMessage.addListener((msg, sender) => {
> +      if (msg === "content-ready") {

I still don't understand why this is necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:209
(Diff revision 6)
> +    browser.runtime.onMessage.addListener(msg => {
> +      if (msg !== "load-resources") {
> +        return;
> +      }
> +
> +      this.testImageLoading(browser.extension.getURL("image.png"), "loaded");

Again, please don't rely on `this` referring to the global here.

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:298
(Diff revision 6)
> +      if (msg.name === "image-loading") {
> +        browser.test.assertTrue(msg.success, `Image was ${msg.expectedAction}`);
> +        browser.test.sendMessage(`image-${msg.expectedAction}`);
> +      } else {
> +        if (msg === "accessible-script-loaded") {
> +          browser.test.notifyPass("mixed-test");

This should probably be after the `sendMessage` call.
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/6-7/
Attachment #8740475 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/45783/#review43143

> Please don't rely on `this` pointing to the global in a bare function call. That doesn't work in strict mode. I'm actually a bit surprised it works here, since the body of this function is in strict mode.

I thought I needed it to make it work, but it turns out I was just working around a different problem that I had. I now have it working without the use of `this`. I assume the new version is okay.

> I still don't understand why this is necessary.

And you are still correct. Last time I fixed the other test and didn't take a second look at this one. This must be a remnant of something different I was doing with message passing earlier in the development of the tests. I've removed this now.
The only issue left on this (unless you raise more in the next review) is that there are a couple of eslint errors due to circular dependencies of functions in the `testImageLoading` function. I'm not sure how to address those, or if I should just mark them to be ignored.
(In reply to Bob Silverberg [:bsilverberg] from comment #23)
> The only issue left on this (unless you raise more in the next review) is
> that there are a couple of eslint errors due to circular dependencies of
> functions in the `testImageLoading` function. I'm not sure how to address
> those, or if I should just mark them to be ignored.

Just declare them ahead of time:

   let bar;
   function foo() {
     bar()
   }
   bar = () => {
   };
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

https://reviewboard.mozilla.org/r/45783/#review43235
Attachment #8740475 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/7-8/
(In reply to Kris Maglione [:kmag] from comment #24)

> Just declare them ahead of time:
> 
>    let bar;
>    function foo() {
>      bar()
>    }
>    bar = () => {
>    };

So easy! Thanks Kris.
Keywords: checkin-needed
seems this needs rebasing 

applying 4688ee250c58
patching file toolkit/components/extensions/test/mochitest/mochitest.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/mochitest/mochitest.ini.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Comment on attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45783/diff/8-9/
Sorry about that. It should be good to merge now.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85616706a29b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1267027
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.