Closed Bug 1208756 Opened 9 years ago Closed 9 years ago

Files enumerated in web_accessible_resources are not accessible

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: callahad, Unassigned)

References

Details

(Keywords: DevAdvocacy)

Attachments

(3 files, 1 obsolete file)

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?
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)
Blocks: 1209184
This is a pure refactoring.
Attachment #8668230 - Flags: review?(bzbarsky)
Attachment #8668232 - Flags: review?(wmccloskey)
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 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 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)
(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)
(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.
Attachment #8668230 - Attachment is obsolete: true
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+
(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)
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)
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)
(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 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+
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)
Blocks: 1256122
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: