Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make sure web_accessible_resources work with CSP/mixed content blocking

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: bsilverberg)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(wmccloskey)

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Flags: blocking-webextensions+
(Reporter)

Updated

2 years ago
Flags: needinfo?(wmccloskey)

Updated

2 years ago
Whiteboard: triaged

Updated

2 years ago
Priority: -- → P3
(Assignee)

Updated

a year ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
(Assignee)

Comment 1

a year ago
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
(Assignee)

Comment 2

a year ago
Created attachment 8740475 [details]
MozReview Request: Bug 1207394 - Make sure web_accessible_resources work with CSP/mixed content blocking, r?kmag

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)
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Comment 4

a year ago
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/
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
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/
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Comment 10

a year ago
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)
(Assignee)

Comment 11

a year ago
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!
(Assignee)

Comment 12

a year ago
FYI, I am working on some improvements as suggested by Luca. I should have a new version up sometime today.
(Assignee)

Comment 13

a year ago
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/
(Assignee)

Comment 14

a year ago
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/

Comment 15

a year ago
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
(Assignee)

Comment 16

a year ago
(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.
(Assignee)

Comment 17

a year ago
Kris, I can confirm that this does work in Chrome, with the leading slash. Should we consider this a bug?
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 18

a year ago
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.
(Assignee)

Comment 21

a year ago
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)
(Assignee)

Comment 22

a year ago
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.
(Assignee)

Comment 23

a year ago
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+
(Assignee)

Comment 26

a year ago
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/
(Assignee)

Comment 27

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af60403a5041
(Assignee)

Comment 28

a year ago
(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.
(Assignee)

Updated

a year ago
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
(Assignee)

Comment 30

a year ago
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/
(Assignee)

Comment 31

a year ago
Sorry about that. It should be good to merge now.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed

Comment 32

a year ago
https://hg.mozilla.org/integration/fx-team/rev/85616706a29b
Keywords: checkin-needed

Comment 33

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85616706a29b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1267027
You need to log in before you can comment on or make changes to this bug.