Closed
Bug 1207394
Opened 9 years ago
Closed 8 years ago
Make sure web_accessible_resources work with CSP/mixed content blocking
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
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.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(wmccloskey)
Updated•9 years ago
|
Flags: blocking-webextensions+
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(wmccloskey)
Updated•8 years ago
|
Whiteboard: triaged
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Assignee | ||
Comment 1•8 years 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•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
I'm using img.addEventListener("load") to verify that the images were able to load properly.
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
||
FYI, I am working on some improvements as suggested by Luca. I should have a new version up sometime today.
Assignee | ||
Comment 13•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Answering my own question, I think we should fix this for Chrome parity. I have opened bug 1264623.
Flags: needinfo?(kmaglione+bmo)
Comment 19•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8740475 -
Flags: review?(kmaglione+bmo)
Comment 20•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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.
Comment 24•8 years ago
|
||
(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 25•8 years ago
|
||
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•8 years 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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af60403a5041
Assignee | ||
Comment 28•8 years 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•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Sorry about that. It should be good to merge now.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Comment 32•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/85616706a29b
Keywords: checkin-needed
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/85616706a29b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•