Closed
Bug 1208756
Opened 9 years ago
Closed 9 years ago
Files enumerated in web_accessible_resources are not accessible
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox44 fixed)
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: callahad, Unassigned)
References
Details
(Keywords: DevAdvocacy)
Attachments
(3 files, 1 obsolete file)
3.53 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
15.33 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We state that we support web_accessible_resources in https://wiki.mozilla.org/WebExtensions However, they don't work at all in my experience. Steps to Reproduce: 1. List "resource.json" in your manifest's web_accessible_resources stanza 2. Attempt to fetch("moz-extension://{uuid}/resource.json") 3. Attempt to XHR the same What should happen: - The content of resource.json should be retrieved. What actually happens: - Fetch fails with "TypeError: NetworkError when attempting to fetch resource." - XHR fails with "NS_ERROR_DOM_BAD_URI: Access to restricted URI denied"
What context are you doing the fetch/XHR from? Presumably a web page?
Reporter | ||
Comment 2•9 years ago
|
||
Both webpages and content_scripts fail Testcase in the "web_accessible_resources folder at https://github.com/callahad/webextension-tests
The behavior we do support (and I just checked that it works) is loading content through an <iframe>. It looks like XHR doesn't work though. Bobby, can you look into this? It looks like we go through nsXMLHttpRequest::CheckChannelForCrossSiteRequest and then nsPrincipal::CheckMayLoad, which don't consider the URI_LOADABLE_BY_ANYONE flag.
Flags: needinfo?(bobbyholley)
Reporter | ||
Updated•9 years ago
|
Comment 5•9 years ago
|
||
This matches the behavior described in https://developer.chrome.com/extensions/manifest/web_accessible_resources
Attachment #8668231 -
Flags: review?(bzbarsky)
Comment 6•9 years ago
|
||
Attachment #8668232 -
Flags: review?(wmccloskey)
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fcff15dd877
Flags: needinfo?(bobbyholley)
Updated•9 years ago
|
Attachment #8668231 -
Attachment description: Part 2 - Introduce URI_FETCHABLY_BY_ANYONE and use it for moz-extension. v1 → Part 2 - Introduce URI_FETCHABLE_BY_ANYONE and use it for moz-extension. v1
Comment 8•9 years ago
|
||
Comment on attachment 8668230 [details] [diff] [review] Part 1 - Hoist shared CheckMayLoad logic into BasePrincipal. v1 >+ virtual bool CheckMayLoadInternal(nsIURI* aURI) = 0; Please document. This will make things a bit slower in the system principal case when aAllowIfInheritsPrincipal. Is it worth it to continue overriding CheckMayLoad in system principal? r=me otherwise
Attachment #8668230 -
Flags: review?(bzbarsky) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8668231 [details] [diff] [review] Part 2 - Introduce URI_FETCHABLE_BY_ANYONE and use it for moz-extension. v1 So I'd like to understand the desired semantics here better. Is the idea that we just want to allow the fetches? Or that the data should really be considered as same-origin with whoever fetched it (effectively inheriting origin)? One reason I ask is that I think it matters for the case of an <img src="web_accessible_resource"> (note lack of "crossorigin" attribute) being painted to a canvas. Does that canvas become tainted?
Flags: needinfo?(bobbyholley)
Comment 10•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #9) > Is the idea that we just want to allow the fetches? Or that the data should > really be considered as same-origin with whoever fetched it (effectively > inheriting origin)? The docs from google (see comment 5) describe it in terms of CORS, though that's probably hand-wavigin. I think we almost certainly want to do the 'inherit origin' thing. > One reason I ask is that I think it matters for the case of an <img > src="web_accessible_resource"> (note lack of "crossorigin" attribute) being > painted to a canvas. Does that canvas become tainted? Dan, can you test what Chrome does here?
Flags: needinfo?(bobbyholley) → needinfo?(dan.callahan)
Comment 11•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #8) > Comment on attachment 8668230 [details] [diff] [review] > Part 1 - Hoist shared CheckMayLoad logic into BasePrincipal. v1 > > >+ virtual bool CheckMayLoadInternal(nsIURI* aURI) = 0; > > Please document. > > This will make things a bit slower in the system principal case when > aAllowIfInheritsPrincipal. Is it worth it to continue overriding > CheckMayLoad in system principal? I think that's likely to be confusing given how the rest of the BasePrincipal::*Internal methods are |final|, and also weird because we still need a CheckMayLoadInternal for nsExpandedPrincipal to call into. However, we could instead simply invert the order of the checks, which should work fine given that the implementations are now side-effect-free. I'll attach a patch.
Comment 12•9 years ago
|
||
Attachment #8668540 -
Flags: review?(bzbarsky)
Updated•9 years ago
|
Attachment #8668230 -
Attachment is obsolete: true
Comment 13•9 years ago
|
||
Comment on attachment 8668540 [details] [diff] [review] Part 1 - Hoist shared CheckMayLoad logic into BasePrincipal. v2 r=me
Attachment #8668540 -
Flags: review?(bzbarsky) → review+
Comment on attachment 8668232 [details] [diff] [review] Part 3 - Tests. v1 Review of attachment 8668232 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this.
Attachment #8668232 -
Flags: review?(wmccloskey) → review+
Reporter | ||
Comment 15•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #10) > (In reply to Boris Zbarsky [:bz] from comment #9) > > Is the idea that we just want to allow the fetches? Or that the data should > > really be considered as same-origin with whoever fetched it (effectively > > inheriting origin)? > > The docs from google (see comment 5) describe it in terms of CORS, though > that's probably hand-wavigin. I think we almost certainly want to do the > 'inherit origin' thing. > > > One reason I ask is that I think it matters for the case of an <img > > src="web_accessible_resource"> (note lack of "crossorigin" attribute) being > > painted to a canvas. Does that canvas become tainted? > > Dan, can you test what Chrome does here? In Chrome, painting a web_accessible_resource into a canvas *does* taint the canvas.
Flags: needinfo?(dan.callahan)
Comment 16•9 years ago
|
||
Ok. So that suggests that they're probably leveraging their CORS machinery behind the scenes. This might be more complicated to do with our architecture, and it probably doesn't matter if this is the only way that the difference is observable. Boris, what do you think?
Flags: needinfo?(bzbarsky)
Comment 17•9 years ago
|
||
I'm not sure I follow. Doesn't the attached patch to whitelist these URIs through CheckMayLoad give us precisely that behavior, where the load succeeds and is treated as CORS-same-origin for people who care about CORS, but for cases when CORS isn't used (like my <img> example) the principal of the result is whatever it was loaded from, not that of the page, and the "CORS was used" bit is off, so will cause tainting for the drawImage case, no? Worth checking what Chrome does with a web-accessible resource in <img crossorigin>, assuming they support @crossorigin on <img>.
Flags: needinfo?(bzbarsky)
Comment 18•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #17) > I'm not sure I follow. Doesn't the attached patch to whitelist these URIs > through CheckMayLoad give us precisely that behavior, where the load > succeeds and is treated as CORS-same-origin for people who care about CORS, > but for cases when CORS isn't used (like my <img> example) the principal of > the result is whatever it was loaded from, not that of the page, and the > "CORS was used" bit is off, so will cause tainting for the drawImage case, > no? Why yes in fact. Ignore comment 16. In that case, I think we're good to go on this patch. > Worth checking what Chrome does with a web-accessible resource in <img > crossorigin>, assuming they support @crossorigin on <img>. Ok. Dan, can you test this as well?
Flags: needinfo?(dan.callahan)
Comment 19•9 years ago
|
||
Comment on attachment 8668231 [details] [diff] [review] Part 2 - Introduce URI_FETCHABLE_BY_ANYONE and use it for moz-extension. v1 r=me
Attachment #8668231 -
Flags: review?(bzbarsky) → review+
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb7068b07a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/06a3886ffdc9 https://hg.mozilla.org/integration/mozilla-inbound/rev/db60203ceb0c
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ecb7068b07a1 https://hg.mozilla.org/mozilla-central/rev/06a3886ffdc9 https://hg.mozilla.org/mozilla-central/rev/db60203ceb0c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Reporter | ||
Comment 22•9 years ago
|
||
Sorry for the late response on the needinfo. I can confirm that our behavior successfully matches Chrome's: 1. Painting *does not* taint the canvas if a web_accessible_resource is loaded from an img tag *with* a crossorigin attribute. 2. Painting *does* taint the canvas if a web_accessible_resource is loaded from an img tag *without* a crossorigin attribute.
Flags: needinfo?(dan.callahan)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•