Tom Adams Reporter
	Description • 11 years ago

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130329030352

Steps to reproduce:

Visit this URI: http://marcorondini.eu/research/resource_uri/


Actual results:

Information from resource:// is leaked, including the fact that I'm using Firefox, the platform, the browser's language (not just the content of the Accept-Language header), and whether or not I was using Tor Browser Bundle.


Expected results:

No information in resource:// should be leaked.

	earthasa
	Comment 1 • 11 years ago

I verified this under Windows using the Tor Browser Bundle (reports version Firefox ESR 17.0.4, not sure if this is accurate or not).

However the exploit failed in Aurora (Firefox 22.0a2 2013-4-12) where the javascript was unable to access resource URIs (although I could from the address bar).

This leads me to believe that at least in the 22 branch the security issue has been plugged.

	Loic
	Comment 2 • 8 years ago

I saw there is an add-on against that:
https://addons.mozilla.org/fr/firefox/addon/no-resource-uri-leak/

Is it still a real issue in FF46?

	Patrick McManus [:mcmanus]
	Comment 3 • 8 years ago

rehosted attachment at
https://www.ducksong.com/misc/b2.html

	Patrick McManus [:mcmanus]
	Comment 4 • 8 years ago

I really have no idea if this is a problem to be fixed, and if so its unlikely the networking layer would do the enforcement (we need to be able to access resource uris in some contexts for sure).. christoph maybe?

	Christoph Kerschbaumer [:ckerschb]
	Comment 5 • 8 years ago

Wow, this POC has been around for quite some time. Anyway, I suppose this is something worth fixing. Dave is our TOR specialist, what do you think? Check out the link in Comment 3.

	Christoph Kerschbaumer [:ckerschb]
	Comment 6 • 8 years ago

I suppose Pat is right and this is a dup of Bug 903959.

	Tom S [:evilpie]
	Comment 7 • 8 years ago

This problem is also shown on https://www.browserleaks.com/firefox

	François Marier [:francois]
	Comment 8 • 8 years ago

This bugs was mentioned on http://www.ghacks.net/2016/06/12/firefox-resource-leak/ and someone created an add-on to disable these loads until we fix it: https://addons.mozilla.org/fr/firefox/addon/no-resource-uri-leak/

	Dave Huseby [:huseby]
	Comment 9 • 8 years ago

This is very interesting.  I brought it up with Arthur Edelstein and we're investigating.  I'm taking this bug to investigate and fix if needed.

	Arthur Edelstein [:arthur]
	Comment 10 • 8 years ago

For reference, here is the Tor Browser ticket: https://trac.torproject.org/projects/tor/ticket/8725

	Dave Huseby [:huseby]
	Comment 11 • 8 years ago

the related tor browser ticket: https://trac.torproject.org/projects/tor/ticket/8725

	Tom S [:evilpie]
	Comment 12 • 8 years ago

This patch explicitly disallows all resource hosts defined by Firefox itself. We should definitely add telemetry to see how often resouce URIs are actually loaded in content.

Instead of blacklisting certain resource hosts we obviously need something different. Maybe a new flag for chrome manifests where most substitutions are defined and adding a flag to SetSubstitution if it is save to load?

	Tom S [:evilpie]
	Comment 13 • 8 years ago

It might be better to do this in nsScriptSecurityManager::CheckLoadURIFlags instead, which already has similar code for chrome://.

	bugzilla-ng
	Comment 14 • 8 years ago

Please look at this:
Isolated content environment proposal
https://discourse.mozilla-community.org/t/proposal-isolated-content-environment/9866?u=desktopd

Any ideas?

	Selek Respa
	Comment 15 • 8 years ago

I want this problem fixed so FF gives less opportunities for fingerprinting but keep in mind that you could harm Webmail encryption via mailvelope if you do. https://github.com/mailvelope/mailvelope/issues/406

	Tom S [:evilpie]
	Comment 16 • 8 years ago

Comment on attachment 8776322 [details] [diff] [review]
Proof of concept patch

Hey Dave. This is not a real patch yet. I am not sure yet how to decide which hosts to block. We probably need some kind of flag in the ComponentsRegistry. I also don't quite understand how the empty host protocols work, is it just everything in the Firefox directory?

We might actually decide to just block everything that is defined by Firefox and have a new version of SetSubstitution that allows to specify content accessibility. (So that add-ons using the old version of SetSubstitution would still be exposed, but they can opt-into not exposing)

	Selek Respa
	Comment 17 • 8 years ago

Be carefull with the audio&videoplayer: TorBrowser caused https://trac.torproject.org/projects/tor/ticket/19837 when trying to fix it.

	Tom S [:evilpie]
	Comment 18 • 8 years ago

Good catch, seems like we really need to implement AllowContentToAccess, I am going to look into extending the code in nsScriptSecurityManager::CheckLoadURIFlags to work with resource URIs.

	Tom S [:evilpie]
	Comment 19 • 8 years ago

These flags allows us to whitelist a specific host substitution in the manifest. It is also used to keep old "SetSubstitution" function working, so that for example extensions that use it to provide web-accessible resources don't break.

	Tom S [:evilpie]
	Comment 20 • 8 years ago

It seems like mostly everything that should be web-accessible is already in /gre/res/, but I have no idea if that is actually true. So we alias resource://gre-res to resource://gre/res and make it contentaccessible.

	Mike Hommey [:glandium]
	Comment 21 • 8 years ago

Comment on attachment 8783218 [details] [diff] [review]
Alias gre-res to /gre/res and make it contentaccessible

Review of attachment 8783218 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure why you're seeking my feedback here. If it's on the build system side, then it looks fine. If it's for part creating resource://gre-res and making it contentaccessible, I'm not the right person to ask whether it's the right solution to this bug.

	Mike Hommey [:glandium]
	Comment 22 • 8 years ago

Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

Review of attachment 8783214 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not peer of this code.

	Tom S [:evilpie]
	Comment 23 • 8 years ago

Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

Review of attachment 8783214 [details] [diff] [review]:
-----------------------------------------------------------------

Mhm I must have misread some of the blame. Maybe bz can take a look, seems like you reviewed some of the AllowContentToAccess stuff we have for chrome://.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 8 years ago

Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

The caps bits look fine.  The necko bits, I have no idea how that stuff works nowadays...

	Tom S [:evilpie]
	Comment 25 • 8 years ago

Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

Not that easy to find out who can review this code. I hope you can have a look :)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 26 • 8 years ago

Comment on attachment 8783214 [details] [diff] [review]
Introduce a contentaccessible flag for resource substitutions

Review of attachment 8783214 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to r- this for now because of the add-on issues we talked about. It looks good aside from that though, although I haven't looked to thoroughly.

::: caps/nsScriptSecurityManager.cpp
@@ +948,5 @@
> +
> +              bool accessAllowed = false;
> +              rph->AllowContentToAccess(aTargetBaseURI, &accessAllowed);
> +              if (accessAllowed) {
> +                  return NS_OK;

Indentation is off here.

@@ +958,5 @@
> +              if (reg) {
> +                  bool accessAllowed = false;
> +                  reg->AllowContentToAccess(aTargetBaseURI, &accessAllowed);
> +                  if (accessAllowed) {
> +                      return NS_OK;

Here too.

::: chrome/nsChromeRegistryChrome.cpp
@@ +980,5 @@
>      return;
>    }
>  
> +  uint32_t substitutionFlags = 0;
> +  if (flags & nsChromeRegistry::CONTENT_ACCESSIBLE)

Should have braces here.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +338,5 @@
>  
> +  if (SubstitutionEntry* entry = mSubstitutions.Get(root)) {
> +    // How does this work?
> +    *result = entry->baseURI.get();
> +    NS_ADDREF(*result);

It's cleaner to assign to a local nsCOMPtr and then .forget that into result.

	Tom S [:evilpie]
	Comment 27 • 8 years ago

This patch makes extension resource contentaccessible by default, but they can explicitly opt-out with contentaccessible=no.

I still haven't gotten a green try run, seems like there are still more resource to track down and whitelist.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 28 • 8 years ago

Comment on attachment 8786306 [details] [diff] [review]
v2 - Introduce a contentaccessible flag for resource substitutions

Review of attachment 8786306 [details] [diff] [review]:
-----------------------------------------------------------------

This patch really needs some tests! It looks pretty good though. However, I want to look at it again before r+ing.

I didn't look at the caps part. You'll need review from bz or bholley on that.

::: chrome/nsChromeRegistryChrome.cpp
@@ +982,5 @@
>  
> +  uint32_t substitutionFlags = 0;
> +  if (flags & nsChromeRegistry::CONTENT_ACCESSIBLE) {
> +    substitutionFlags |= nsIResProtocolHandler::ALLOW_CONTENT_ACCESS;
> +  } else {

This might be cleaner as:

if (flags & CONTENT_ACCESSIBLE) {
  |= ALLOW_...;
} else if (flags & CONTENT_INACCESSIBLE) {
  // Don't set any flags.
} else {
  // Default for extensions is to allow content access. Default for browser code is not to allow it.
  if (type == ...) {
    |= ALLOW...;
  }
}

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +149,4 @@
>      aBaseURI->GetSpec(mapping.resolvedURI.spec);
>      aBaseURI->GetOriginCharset(mapping.resolvedURI.charset);
>    }
> +  mapping.flags = flags;

I think you need code to use the extra flags here:
http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/chrome/nsChromeRegistryContent.cpp#121

Maybe other places too.

@@ +330,4 @@
>    return NS_OK;
>  }
>  
> +

Extra newline.

@@ +336,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(result);
>  
> +  if (SubstitutionEntry* entry = mSubstitutions.Get(root)) {
> +    // How does this work?

What is this comment for?

::: netwerk/protocol/res/SubstitutingProtocolHandler.h
@@ +76,4 @@
>    nsIIOService* IOService() { return mIOService; }
>  
>  private:
> +  struct SubstitutionEntry : public PLDHashEntryHdr

I don't think you need the inheritance here.

@@ +89,3 @@
>    nsCString mScheme;
>    Maybe<uint32_t> mFlags;
> +  nsClassHashtable<nsCStringHashKey, SubstitutionEntry> mSubstitutions;

You might be able to use nsDataHashtable here to avoid allocating SubstitutionEntrys in the heap.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl
@@ +12,4 @@
>  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
>  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
>  {
> +  const short ALLOW_CONTENT_ACCESS = 1;

Please add a comment describing what this means. Also, can we put this flag on nsIResProtocolHandler.idl? It doesn't apply to ExtensionProtocolHandler (and is confusing there).

@@ +25,4 @@
>     */
>    void setSubstitution(in ACString root, in nsIURI baseURI);
>  
> +  void setSubstitutionWithFlags(in ACString root, in nsIURI baseURI, in uint32_t flags);

Need a comment here too.

::: xpcom/components/ManifestParser.cpp
@@ +122,4 @@
>      nullptr, &nsChromeRegistry::ManifestOverride, nullptr
>    },
>    {
> +    "resource",         2, false, true, true, true, true,

Please fix the comment on the contentflags field of ManifestDirective.

	Tom S [:evilpie]
	Comment 29 • 7 years ago

Sorry for holding this up, I will try to get back to this soon, probably this week.

	Tom S [:evilpie]
	Comment 30 • 7 years ago

If anyone wants to pick this up feel free, I am currently working on some other things.

	Tom S [:evilpie]
	Comment 31 • 7 years ago

Sorry, I won't be working on this in the near future.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 42 • 7 years ago

(In reply to Bill McCloskey (:billm) from comment #28)
Hi Bill,

I took this bug and made a new patch, please take a look again.
This patch is based on the one Tom introduced and followed your instruction to modify, except

> ::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl
> @@ +12,4 @@
> >  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
> >  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
> >  {
> > +  const short ALLOW_CONTENT_ACCESS = 1;
> 
> Please add a comment describing what this means. Also, can we put this flag
> on nsIResProtocolHandler.idl? It doesn't apply to ExtensionProtocolHandler
> (and is confusing there).

I think you are right that it should be more reasonable to put the flag ALLOW_CONTENT_ACCESS on nsIResProtocolHandler.
However, I found that the flag is also used by SubstitutingProtocolHandler.
If the flag is put on nsIResProtocolHandler, the dependency will get complex (SubstitutingProtocolHandler will depend on nsIResProtocolHandler, just like parent depends on child).
Therefore, I decided to keep it in nsISubstitutingProtocolHandler and need your comment.

Besides, this patch fails some tests because they try to load resource:// URIs in web pages, and I am not sure if this patch effects other functionalities.
So I use a preference "security.block_resource_uri" to determine whether resource:// URIs should be blocked instead of blocking them by default.

I also added some test cases that resource:// URIs are only blocked with the preference enabled.

For the caps part, I asked bholley to review it.

Thanks

	Bobby Holley (:bholley)
	Comment 43 • 7 years ago

Comment on attachment 8868827 [details]
Bug 863246 - Deny resource:// access to web content

Bouncing to Boris. Sorry for doing so Boris, I'm just incredibly swamped.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 7 years ago
mozreview-review

Comment on attachment 8868827 [details]
Bug 863246 - Deny resource:// access to web content

https://reviewboard.mozilla.org/r/140412/#review145704

::: commit-message-043ad:1
(Diff revision 1)
> +Bug 863246 - Deny resource:// access to web content

This commit message is wrong.  That's not what the patches are doing.  Please write a reasonable commit message.

::: caps/nsScriptSecurityManager.cpp:80
(Diff revision 1)
>  nsIStringBundle *nsScriptSecurityManager::sStrBundle = nullptr;
>  JSContext       *nsScriptSecurityManager::sContext   = nullptr;
>  bool nsScriptSecurityManager::sStrictFileOriginPolicy = true;
>  
> +// Determines whether resource:// URIs should be blocked
> +static const char * const kPrefBlockResourceUri = "security.block_resource_uri";

Does anything actually set this pref?  If not, why not?

That is, why are we adding a pref here, not changing the default behavior?

::: caps/nsScriptSecurityManager.cpp:883
(Diff revision 1)
>          if (aFlags & nsIScriptSecurityManager::ALLOW_CHROME) {
>  
> -            // For now, don't change behavior for resource:// or moz-icon:// and
> -            // just allow them.
> -            if (!targetScheme.EqualsLiteral("chrome")) {
> +            // For now, don't change behavior for moz-icon:// and just allow it.
> +            if (!targetScheme.EqualsLiteral("chrome") &&
> +                    // resource:// is simply allowed if the preference kPrefBlockResourceUri is false
> +                    (!Preferences::GetBool(kPrefBlockResourceUri) || !targetScheme.EqualsLiteral("resource"))) {

Why is there an uncached pref lookup on a hot path here?

Please use a bool var cache in the usual way, assuming we need the pref at all.  Is the pref just to make this easy to back out?

::: caps/nsScriptSecurityManager.cpp:908
(Diff revision 1)
>  
> +            if (targetScheme.EqualsLiteral("resource")) {
> +                nsCOMPtr<nsIProtocolHandler> ph;
> +                rv = sIOService->GetProtocolHandler("resource", getter_AddRefs(ph));
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);

What guarantees that this does not return null?  We should probably block the load if null is returned.

	Arthur Edelstein [:arthur]
	Comment 45 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #42)

Hi Chung-Sheng -- thank you for working on this ticket. Disabling resource:// (and chrome://) URIs completely causes breakage of a number of browser features, including video controls, image display, text-box resizing, and directory listing.

So in Tor Browser, my colleague wrote some stopgap JavaScript extension code that blocks resource://, chrome:// and about: URIs from being loaded by content, except that some specific resource: and chrome: URIs are whitelisted, so that they can be loaded and no browser features are broken.

You can see the whitelisted addresses here:
https://gitweb.torproject.org/torbutton.git/tree/src/components/content-policy.js#n33

Could this whitelist (and any additional URIs need to past unit tests) be included in your C++ patch? I think that would eliminate the need for the "security.block_resource_uri" preference altogether.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 46 • 7 years ago

Hi Arthur,

Thank you very much for your comment.

Hi Boris,

Does the whitelist approach in Tor Browser seem reasonable to you?
If we are going to implement the whitelist, should the accessibility flag of substitution mapping be kept?
Perhaps it is better to keep the flag and related functions because maybe it can be used to temporarily allow URIs in unit tests in order not to fail the tests without adding them to the whitelist.
Besides, should chrome:// URIs be block as well like Tor Browser does?

Thanks!

	Georg Koppen
	Comment 47 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #46)
> Does the whitelist approach in Tor Browser seem reasonable to you?

I don't think the whitelist approach should get uplifted. It is a nasty workaround which only works for us because we don't need to take care of extensions getting installed (apart from the ones we ship). In fact, later on we had to add a way to disable that whitelist approach for users with additional extensions installed (https://trac.torproject.org/projects/tor/ticket/21396). I have not checked whether resource:// URIs are affected by that but chances are high that they are. But even if not, then there is the issue of keeping the whitelist up-to-date.

Besides that, yes, we'd need a fix for chrome:// URIs as well but, again, without whitelisting particular chrome URLs.

	Boris Zbarsky [:bzbarsky]
	Comment 48 • 7 years ago

> Does the whitelist approach in Tor Browser seem reasonable to you?

Um... I assumed that it was already implemented in the patches, via the contentaccessible thing.

I would much prefer we handle this via chrome registration instead of a hardcoded whitelist, obviously.

If we have a problem wherein things that should and shouldn't be exposed are in the same package, the right solution, as for chrome://, is to split them up into separate packages.

> Besides that, yes, we'd need a fix for chrome:// URIs as well

What's the problem with chrome:// URIs?  They are already not exposed unless you explicitly opt in to exposing them in your manifest.

	Georg Koppen
	Comment 49 • 7 years ago

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #48)
> > Besides that, yes, we'd need a fix for chrome:// URIs as well
> 
> What's the problem with chrome:// URIs?  They are already not exposed unless
> you explicitly opt in to exposing them in your manifest.

It seemed to me there is a bit uncertainty involved in how to get the patch for this bug done. I just wanted to highlight that there is no real difference between resource:// and chrome:// URIs regarding fingerprinting issues. Thus, however this bug gets fixed chrome:// URIs should be kept in mind, so that chrome:// URIs have/get a similar treatment.

	Boris Zbarsky [:bzbarsky]
	Comment 50 • 7 years ago

Right, I think we should fix this the same way as chrome:// URIs: don't expose by default, allow opt-in to exposure via the chrome manifest.

Whether that's viable for backporting is a separate issue.  For chrome:// URIs when we first introduced the contentaccessible flag we defaulted to "yes" for a while until we flipped the default, to give extensions time to update....

	Arthur Edelstein [:arthur]
	Comment 51 • 7 years ago

I think Boris and Georg are right. A `contentaccessible` flag approach is probably much better than the whitelist approach I was suggesting in comment 45.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 52 • 7 years ago

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #50)
Hi Arthur and Georg,

Thank you very much for your comments.

Hi Boris,

> Right, I think we should fix this the same way as chrome:// URIs: don't
> expose by default, allow opt-in to exposure via the chrome manifest.

Yes, this patch exactly does what you mean, and I totally agree with you that the limitation of loading resource:// URIs should be default behavior.
However, this patch fails some unit tests and I am not sure if it also breaks any living functionalities.
That's why I added a preference to determine whether the check of the contentaccessible flag should be performed when content is trying to load resource:// URIs.
Therefore, I would like to ask if I can put the patch (without the preference) on nightly builds and let users test if there is any breakage of functionalities?

	Tom S [:evilpie]
	Comment 53 • 7 years ago

This isn't a very good idea. Please first start looking into fixing the failures on try, because we already know various resources need to be loadable by content. I think with my patch " Alias gre-res to /gre/res and make it contentaccessible" a lot of these things actually work. Add-ons also need to load resource:// in the content, for example Adblock Plus.

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 7 years ago

Right, what comment 53 says.  You want to fix the unit test failures by exposing the relevant things as needed, or changing the tests to not depend on them if they really shouldn't be exposed.  You want to talk to Jorge about addon compat fallout, though I see that the patch makes addons opt-in instead of opt-out to the new blocking behavior, so maybe there isn't any.

	Jorge Villalobos [:jorgev] (he/him)
	Comment 55 • 7 years ago

What's the expected impact for add-ons? resource:// URLs were meant to be content-accessible, so changing that assumption could cause a lot of breakage. Like with other add-on compat issues, I'd like to ask if it's necessary to fix before 57, since most legacy add-ons at this point are either migrating to WebExtensions or abandoned.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 56 • 7 years ago
mozreview-review

Comment on attachment 8866700 [details]
Bug 863246 - Use system principal to load debugger scripts

https://reviewboard.mozilla.org/r/138314/#review146700

::: commit-message-d8762:1
(Diff revision 4)
> +Bug 863246 - Deny resource:// access to web content

The commit message should say something about how you're marking resources based on whether they should be content accessible. This patch doesn't actually deny anything.

::: chrome/nsChromeRegistryChrome.cpp:931
(Diff revision 4)
> +  if ((flags & nsChromeRegistry::CONTENT_ACCESSIBLE)
> +      // Allow content access to extension resources by default.
> +      || (!(flags & nsChromeRegistry::CONTENT_INACCESSIBLE)
> +          && ((cx.mType == NS_EXTENSION_LOCATION) || (cx.mType == NS_BOOTSTRAPPED_LOCATION)))) {
> +    substitutionFlags |= nsIResProtocolHandler::ALLOW_CONTENT_ACCESS;
> +  }

This really should be broken into multiple statements, with comments for each case. Something like:

bool contentAccessible = false;
if (cx.mType == NS_EXTENSION_LOCATION || cx.mType == NS_BOOTSTRAPPED_LOCATION) {
  // This is a browser extension. By default, extension resources are
  // content-accessible unless the manifest opts out.
  contentAccessible = !(flags & nsChromeRegistry::CONTENT_INACCESSIBLE);
} else {
  // This manifest is part of Firefox. By default, Firefox resources are
  // not content-accessible unless the manifests opts in.
  contentAccessible = (flags & nsChromeRegistry::CONTENT_ACCESSIBLE);
}

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:136
(Diff revision 4)
>  
>    return NS_OK;
>  }
>  
>  nsresult
> -SubstitutingProtocolHandler::SendSubstitution(const nsACString& aRoot, nsIURI* aBaseURI)
> +SubstitutingProtocolHandler::SendSubstitution(const nsACString& aRoot, nsIURI* aBaseURI, uint32_t flags)

This should be aFlags.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl:16
(Diff revision 4)
>   */
>  [scriptable, uuid(154c64fd-a69e-4105-89f8-bd7dfe621372)]
>  interface nsISubstitutingProtocolHandler : nsIProtocolHandler
>  {
>    /**
> +   * Content script may access files in this package

This should end with a period.

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl:32
(Diff revision 4)
>     * enforced.
>     */
>    [must_use] void setSubstitution(in ACString root, in nsIURI baseURI);
>  
>    /**
> +   * Same as setSubstitution, but with specific flags

Also should end with a period.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8866701 [details]
Bug 863246 - Add test case

https://reviewboard.mozilla.org/r/138316/#review146708

::: browser/components/resistfingerprinting/test/mochitest/test_uri_resource.html:37
(Diff revision 5)
> +      });
> +      dom = document.createElement("script");
> +      dom.onload = listener.bind(dom, !aPrefVal);
> +      dom.onerror = listener.bind(dom, aPrefVal);
> +      document.head.appendChild(dom);
> +      dom.src = "resource:///defaults/preferences/firefox.js";

This is a nice start, but you really need to test other cases. You should test both the addon case and the non-addon case. And for each of those, you should test resources that don't set contentaccessible as well as those that explicitly set it to both possible values.

Testing add-ons is a pain, but I think we actually need to do it here. Maybe ask rhelmer or Mossop how to do it.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 58 • 7 years ago

Hi Tom and Boris,

I did not apply the manifest patch because I am afraid that it still exposes too much.
If the allowance of resource://gre/res seems fine to you, I will apply it.

Hi Jorge,

OK, I will check our milestones about this issue.
Besides, does the migration to WebExtensions imply add-ons will not depend on the use of resource:// URIs?
(I am sorry if this is a silly question but I am really not familiar with that)

Thank you very much!

	Jorge Villalobos [:jorgev] (he/him)
	Comment 59 • 7 years ago

WebExtensions shouldn't depend on the behavior of resource://, as I understand it. ni Andy for confirmation.

	Andy McKay
	Comment 60 • 7 years ago

An add-on developer writing a webextension will no longer be using resource:// URIs, so they don't need to worry about this change. Since billm is on the thread, I'm pretty sure any underlying WebExtensions issues are covered.

Agree with Jorge, landing in 57 would be nice to reduce breakage.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 61 • 7 years ago

We will review the landing schedule.
Thank you very much!

	Chung-Sheng Fu [:cfu] Assignee
	Comment 62 • 7 years ago

(In reply to Bill McCloskey (:billm) from comment #57)
> Comment on attachment 8866701 [details]
> Bug 863246 - Add test case
> 
> https://reviewboard.mozilla.org/r/138316/#review146708
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_uri_resource.
> html:37
> (Diff revision 5)
> > +      });
> > +      dom = document.createElement("script");
> > +      dom.onload = listener.bind(dom, !aPrefVal);
> > +      dom.onerror = listener.bind(dom, aPrefVal);
> > +      document.head.appendChild(dom);
> > +      dom.src = "resource:///defaults/preferences/firefox.js";
> 
> This is a nice start, but you really need to test other cases. You should
> test both the addon case and the non-addon case. And for each of those, you
> should test resources that don't set contentaccessible as well as those that
> explicitly set it to both possible values.

We decided to follow Jorge and Andy's suggestion and will land the patch in 57.
Should I still test the add-on case?

> Testing add-ons is a pain, but I think we actually need to do it here. Maybe
> ask rhelmer or Mossop how to do it.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 63 • 7 years ago

> We decided to follow Jorge and Andy's suggestion and will land the patch in 57.
> Should I still test the add-on case?

All right, I guess you don't need an add-on test if you don't plan to land until 57.

	Ethan Tseng [:ethan]
	Comment 66 • 7 years ago

Move from milestone 1 to 2 according to comment 60.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 67 • 7 years ago

I am trying to fix failed tests due to this change of policy of loading resource:// URIs by setting substitutions for specific hosts so that content can temporarily load required resources.
Take this test for example http://searchfox.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html#44
It tries to load resources under resource://gre and I add the following code to make it accessible

    SpecialPowers.Cc["@mozilla.org/network/protocol;1?name=resource"]
        .getService(SpecialPowers.Ci.nsIResProtocolHandler)
        .setSubstitution("gre", SpecialPowers.Services.io.newURI("resource://gre/"));

However, some hosts cannot be substituted according to nsResProtocolHandler::SetSubstitution http://searchfox.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.cpp#98
So, is it possible to remove the host filter in nsResProtocolHandler::SetSubstitution?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 68 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #67)
> Take this test for example
> http://searchfox.org/mozilla-central/source/caps/tests/mochitest/
> test_bug292789.html#44

For this test, I think it would be best to have a preference that allows loads from resource URLs (like the "security.block_resource_uri" pref you added. To be clear though, we want this pref to be enabled by default. You would just turn if off using SpecialPowers.pushPrefEnv around the part of the test where the load should be allowed.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 77 • 7 years ago

There is still a preference named "security.all_resource_uri_content_accessible" now.
By default, it is false and content can only load resource:// URIs declared content-accessible in manifests.
Mochitests can call SpecialPowers.pushPrefEnv to set the preference true and load resource:// URIs they need.

Tom's patch that aliases resource://gre-res to resource://gre/res and sets it content-accessible is also applied.

The unit test for this patch is updated.
First it tries to load an image from resource://gre-res, which is content-accessible because of Tom's aliasing patch.
Then it tries to load an image from resource://gre and should be blocked.
At last, it calls SpecialPowers.pushPrefEnv to set "security.all_resource_uri_content_accessible" true, trying to load image from resource://gre again, and should be allowed this time.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 78 • 7 years ago
mozreview-review

Comment on attachment 8876626 [details]
Bug 863246 - Add test cases

https://reviewboard.mozilla.org/r/147960/#review153110

::: browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:5
(Diff revision 2)
> +<!DOCTYPE html>
> +<meta charset="utf8">
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<script>
> +function test(aTest, aUri, aContentAccessible, aResolve) {

This function should return a promise.

::: browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:21
(Diff revision 2)
> +  document.body.appendChild(img);
> +}
> +
> +SimpleTest.waitForExplicitFinish();
> +document.addEventListener("DOMContentLoaded", function() {
> +  Promise.resolve(new Promise(function(aResolve) {

This logic is pretty complicated. Instead, please use add_task and async/await. It should look like this:

add_task(async function() {
  await test(...);
  await test(...);
  await SpecialPowers.pushPrefEnv(...);
  await test(...);
});

	Chung-Sheng Fu [:cfu] Assignee
	Comment 79 • 7 years ago

(In reply to Bill McCloskey (:billm) from comment #78)
> Comment on attachment 8876626 [details]
> Bug 863246 - Add test cases
> 
> https://reviewboard.mozilla.org/r/147960/#review153110
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:5
> (Diff revision 2)
> > +<!DOCTYPE html>
> > +<meta charset="utf8">
> > +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> > +<script>
> > +function test(aTest, aUri, aContentAccessible, aResolve) {
> 
> This function should return a promise.
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/test_bug863246.html:21
> (Diff revision 2)
> > +  document.body.appendChild(img);
> > +}
> > +
> > +SimpleTest.waitForExplicitFinish();
> > +document.addEventListener("DOMContentLoaded", function() {
> > +  Promise.resolve(new Promise(function(aResolve) {
> 
> This logic is pretty complicated. Instead, please use add_task and
> async/await. It should look like this:
> 
> add_task(async function() {
>   await test(...);
>   await test(...);
>   await SpecialPowers.pushPrefEnv(...);
>   await test(...);
> });

OK I will learn how to use add_task.
Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 80 • 7 years ago
mozreview-review

Comment on attachment 8876624 [details]
Bug 863246 - Content can only load resource:// URIs declared content-accessible in manifests

https://reviewboard.mozilla.org/r/147956/#review154102

r=me

::: caps/nsScriptSecurityManager.cpp:923
(Diff revision 2)
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                if (!ph) {
> +                    return NS_ERROR_DOM_BAD_URI;
> +                }
> +
> +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);

It might be a good idea to cache this, here or on nsContentUtils or something...  Followup is OK for that.

	Boris Zbarsky [:bzbarsky]
	Comment 81 • 7 years ago
mozreview-review

Comment on attachment 8876625 [details]
Bug 863246 - Alias resource://gre-res to resource://gre/res and make it content-accessible

https://reviewboard.mozilla.org/r/147958/#review154104

r=me

	Chung-Sheng Fu [:cfu] Assignee
	Comment 82 • 7 years ago

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #80)
> Comment on attachment 8876624 [details]
> Bug 863246 - Content can only load resource:// URIs declared
> content-accessible in manifests
> 
> https://reviewboard.mozilla.org/r/147956/#review154102
> 
> r=me
> 
> ::: caps/nsScriptSecurityManager.cpp:923
> (Diff revision 2)
> > +                NS_ENSURE_SUCCESS(rv, rv);
> > +                if (!ph) {
> > +                    return NS_ERROR_DOM_BAD_URI;
> > +                }
> > +
> > +                nsCOMPtr<nsIResProtocolHandler> rph = do_QueryInterface(ph);
> 
> It might be a good idea to cache this, here or on nsContentUtils or
> something...  Followup is OK for that.

I will learn how to cache the result of querying interface.
And I am going to fix test failures by setting the pref security.all_resource_uri_content_accessible true.

Thank you very much for your help!

	Chung-Sheng Fu [:cfu] Assignee
	Comment 90 • 7 years ago

After reviewing several test failures, it seems better to me to have a general solution.
Is it a good idea to add security.all_resource_uri_content_accessible = true to testing/profiles/prefs_general.js?

	Boris Zbarsky [:bzbarsky]
	Comment 91 • 7 years ago

It seems like it would be too easy to create tests that pass in the test harness but fail in an actual browser, and not because they're explicitly loading res:// bits that are not web-exposed (e.g. a plaintext test that fails because plaintext.css fails to get loaded outside the test harness).

	Chung-Sheng Fu [:cfu] Assignee
	Comment 92 • 7 years ago

OK, then I will keep fixing individual failures.
Thanks!

	Chung-Sheng Fu [:cfu] Assignee
	Comment 97 • 7 years ago

Sorry again.
I found that the "View Source" page requires style sheet from resource://gre-resources.
The folder contains only images and style sheets, and a hiddenWindow.html.
Can I make resource://gre-resources content-accessible in manifest?

	Boris Zbarsky [:bzbarsky]
	Comment 98 • 7 years ago

I have no idea.  In particular, I don't know whether other things like extensions can place stuff in that folder.

If not, then this folder doesn't leak any information that is not available from the UA string version or object-detection techniques to determine the Firefox version, so probably OK to expose.

	Selek Respa
	Comment 99 • 7 years ago

Keep in mind though that in https://bugzilla.mozilla.org/show_bug.cgi?id=1333651 the User Agent is spoofed from time to time.
Not sure if this wouldn't break it by revealing the original.

	Boris Zbarsky [:bzbarsky]
	Comment 100 • 7 years ago

Yes, that's why I said "object-detection techniques".

	Boris Zbarsky [:bzbarsky]
	Comment 101 • 7 years ago

I mean, the principled and safe thing to do is to add a new expose-gre-resources thing, move stuff that should be exposed there, etc.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 102 • 7 years ago

Here is a brief conclusion after reviewing test failures.


Must be exposed:

Image document
- resource://gre/res/ImageDocument.css
- resource://gre/res/TopLevelImageDocument.css

Json view
- resource://devtools/client/jsonview/css/main.css
- resource://devtools/client/jsonview/lib/require.js

Video document
- resource://gre/res/TopLevelVideoDocument.css

View source
- resource://gre-resources/viewsource.css


Break tests, but not sure if need to be exposed:

caps/tests/mochitest/test_bug292789.html
- resource://gre/chrome/toolkit/content/mozapps/xpinstall/xpinstallConfirm.js

devtools/client/debugger/test/mochitest/
- resource://devtools/server/worker.js

devtools/client/shared/shim/test/
- resource://devtools/client/shared/shim/Services.js

devtools/shared/platform/content/test/test_clipboard.html
- resource://devtools/shared/platform/content/clipboard.js

dom/security/test/mixedcontentblocker/test_bug803225.html
- resource://gre/modules/XPCOMUtils.jsm

Besides, the test browser/base/content/test/static/browser_all_files_referenced.js accesses many resources like resource://app/ resource://devtools/ in web content.
I think this case can be simply ignored and fixed with the preference security.all_resource_uri_content_accessible.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 109 • 7 years ago

This patch creates a new content-accessible folder and copies necessary files to it.
Now the files required by image document, video document, and view source page are handled.

For the jsonview related files:
1. almost the whole module need to be exposed
2. it requires React.js library, whose js files are everywhere in devtools/client/shared
3. jsonview module and React.js library are referenced by not only web content but also the browser internally
So I just make devtools/client/jsonview and devtools/client/shared content-accessible without changing their paths.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 110 • 7 years ago
mozreview-review

Comment on attachment 8885981 [details]
Bug 863246 - Move resources that need to be exposed to web content to locations that are marked as contentaccessible

https://reviewboard.mozilla.org/r/156770/#review162172

This looks nice, but I'm worried that it's based on top of the "Alias resource://gre-res to resource://gre/res and make it content-accessible" patch, but it doesn't seem to remove the line that makes all of resource://gre/res/ content-accessible. Isn't that the whole point of this patch? r+ if you remove that line (as well as chrome-resources.manifest).

I'm also a little worried that people could add stuff to the devtools directory that could be used for fingerprinting. But I can't think of a good way to avoid that.

::: commit-message-a34dc:1
(Diff revision 1)
> +Bug 863246 - Move stuff that should be exposed

This needs a better commit message. Perhaps "Move resources that need to be exposed to web content to locations that are marked as contentaccessible".

	Chung-Sheng Fu [:cfu] Assignee
	Comment 111 • 7 years ago

(In reply to Bill McCloskey (:billm) from comment #110)
> Comment on attachment 8885981 [details]
> Bug 863246 - Move stuff that should be exposed
> 
> https://reviewboard.mozilla.org/r/156770/#review162172
> 
> This looks nice, but I'm worried that it's based on top of the "Alias
> resource://gre-res to resource://gre/res and make it content-accessible"
> patch, but it doesn't seem to remove the line that makes all of
> resource://gre/res/ content-accessible. Isn't that the whole point of this
> patch? r+ if you remove that line (as well as chrome-resources.manifest).

Yes, if this new patch seems fine to you, I will remove the resource://gre-res patch.
I should have explained it before, sorry.
 
> I'm also a little worried that people could add stuff to the devtools
> directory that could be used for fingerprinting. But I can't think of a good
> way to avoid that.

I have no idea how to verify it either.
Should I find others to help us clarify the risk?

> ::: commit-message-a34dc:1
> (Diff revision 1)
> > +Bug 863246 - Move stuff that should be exposed
> 
> This needs a better commit message. Perhaps "Move resources that need to be
> exposed to web content to locations that are marked as contentaccessible".

I will fix it.
Thank you very much!

	Chung-Sheng Fu [:cfu] Assignee
	Comment 129 • 7 years ago

The test browser/base/content/test/static/browser_parsable_css.js fails because there are some --in-content variables cannot be referenced.
https://treeherder.mozilla.org/logviewer.html#?job_id=115177078&repo=try&lineNumber=2462
I found they are defined in chrome://global/skin/in-content/common.css but it is loaded after those stylesheets that reference the --in-content variables.
I'm not sure how my patch breaks this test but I think it is better to load chrome://global/skin/in-content/common.css first, in order to guarantee the --in-content variables can be correctly referenced by other stylesheets.

	:Gijs (he/him)
	Comment 130 • 7 years ago

Does this fix bug 1120398, or do we need to do more work for that?

	:Gijs (he/him)
	Comment 131 • 7 years ago
mozreview-review

Comment on attachment 8887849 [details]
Bug 863246 - Fix test failures

https://reviewboard.mozilla.org/r/158774/#review164582

r=me with the comments addressed.

::: browser/base/content/test/static/browser_parsable_css.js:300
(Diff revision 1)
>    // filter out either the devtools paths or the non-devtools paths:
>    let isDevtools = SimpleTest.harnessParameters.subsuite == "devtools";
>    let devtoolsPathBits = ["webide", "devtools"];
>    uris = uris.filter(uri => isDevtools == devtoolsPathBits.some(path => uri.spec.includes(path)));
>  
> -  for (let uri of uris) {
> +  let loadCss = (chromeUri) => new Promise((resolve) => {

Nit: no () around the argument of a single-argument arrow function

Nit: The other functions here also use camelcase and consistently capitalize all of "CSS", so it should be "loadCSS".

::: browser/base/content/test/static/browser_parsable_css.js
(Diff revision 1)
> -      };
> +    };
> +    linkEl = doc.createElement("link");
> +    linkEl.setAttribute("rel", "stylesheet");
> -      linkEl.addEventListener("load", onLoad);
> +    linkEl.addEventListener("load", onLoad);
> -      linkEl.addEventListener("error", onError);
> +    linkEl.addEventListener("error", onError);
> -      linkEl.setAttribute("type", "text/css");

Why did you remove the setting of the type attribute?

::: browser/base/content/test/static/browser_parsable_css.js:322
(Diff revision 1)
> +  // Make sure chrome://global/skin/in-content/common.css is loaded before other
> +  // stylesheets in order to guarantee the --in-content variables can be
> +  // correctly referenced.
> +  const kCommonCss = "chrome://global/skin/in-content/common.css";

Ugh, we really need a more scalable solution for variable errors, but if this reliably fixes the problem and unblocks these patches, I guess I can stamp this patch for now. I believe the correct solution is for this test to ignore variable errors, and to simultaneously fix bug 1375126. But that's not work we need to do here.

Nit: please rename to kInContentCommonCSS.

::: browser/base/content/test/static/browser_parsable_css.js:326
(Diff revision 1)
> +
> +  // Make sure chrome://global/skin/in-content/common.css is loaded before other
> +  // stylesheets in order to guarantee the --in-content variables can be
> +  // correctly referenced.
> +  const kCommonCss = "chrome://global/skin/in-content/common.css";
> +  allPromises = uris.map((uri) => convertToCodeURI(uri.spec))

Declare allPromises here, and remove the earlier declaration of this that assigns an empty array (which is now always being overwritten) on line 293. The comment will want to be adapted and moved as well.

	:Gijs (he/him)
	Comment 132 • 7 years ago
mozreview-review

Comment on attachment 8885980 [details]
Bug 863246 - Fix test failures

https://reviewboard.mozilla.org/r/156768/#review164588

::: browser/base/content/test/static/browser_all_files_referenced.js:29
(Diff revision 6)
> +  // Bug 863246
> +  // Files in this path are moved from different locations.
> +  "resource://content-accessible/",

This doesn't look right. Why do we need this? The bug number shouldn't be this bug, it should be a bug that is filed to deal with the fact that we need to make an exception for these files.

	:Gijs (he/him)
	Comment 133 • 7 years ago

Some general comments:

- did someone review the system add-ons we ship? On the assumption that they count as NS_BOOTSTRAPPED_LOCATION, we should make sure that we mark their stuff as non-content-accessible in their respective chrome manifests (unless it needs to be).
- is there a bug on file to remove the add-on exception for 57? Is it even helpful to have that right now, given 56 has only 2 weeks of trunk life left?
- has someone talked with the webextension folks and checked how the moz-extension protocol currently behaves?
- if we keep the exception for add-ons, could we create a testcase that checks the extension side of things works?

	Boris Zbarsky [:bzbarsky]
	Comment 134 • 7 years ago

> but I think it is better to load chrome://global/skin/in-content/common.css first

There is no "first" or "second" with stylesheets in terms of referencing.  It's more like a constraint system than a procedural system.  Any time a new sheet is loaded everything that might be affected by it needs to be recomputed.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 135 • 7 years ago

(In reply to :Gijs from comment #130)
> Does this fix bug 1120398, or do we need to do more work for that?

No, we still need some work to solve the whole problem of bug 1120398.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 136 • 7 years ago

(In reply to :Gijs from comment #132)
> Comment on attachment 8885980 [details]
> Bug 863246 - Fix test failures
> 
> https://reviewboard.mozilla.org/r/156768/#review164588
> 
> ::: browser/base/content/test/static/browser_all_files_referenced.js:29
> (Diff revision 6)
> > +  // Bug 863246
> > +  // Files in this path are moved from different locations.
> > +  "resource://content-accessible/",
> 
> This doesn't look right. Why do we need this? The bug number shouldn't be
> this bug, it should be a bug that is filed to deal with the fact that we
> need to make an exception for these files.

There are 4 files moved to resource://content-accessible

resource://gre/res/ImageDocument.css
resource://gre/res/TopLevelImageDocument.css
resource://gre/res/TopLevelVideoDocument.css
resource://gre-resources/viewsource.css

They now break this test with new URIs.
I'm not sure how they could pass so I added that new exception rule.
Should I add them to the whitelist array and comment them with file paths where they are referenced?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 137 • 7 years ago

(In reply to :Gijs from comment #133)
> Some general comments:
> 
> - did someone review the system add-ons we ship? On the assumption that they
> count as NS_BOOTSTRAPPED_LOCATION, we should make sure that we mark their
> stuff as non-content-accessible in their respective chrome manifests (unless
> it needs to be).

resource:// URIs are non-content-accessible by default (if not marked contentaccessible=yes in manifests).
Currently the only content-accessible resource:// URIs are:
resource://content-accessible
resource://devtools-client-jsonview
resource://devtools-client-shared

> - is there a bug on file to remove the add-on exception for 57? Is it even
> helpful to have that right now, given 56 has only 2 weeks of trunk life left?
> - has someone talked with the webextension folks and checked how the
> moz-extension protocol currently behaves?
> - if we keep the exception for add-ons, could we create a testcase that
> checks the extension side of things works?

I will ask someone about add-on and WebExtension.
Thank you for the comments.

	:Gijs (he/him)
	Comment 138 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #137)
> (In reply to :Gijs from comment #133)
> > Some general comments:
> > 
> > - did someone review the system add-ons we ship? On the assumption that they
> > count as NS_BOOTSTRAPPED_LOCATION, we should make sure that we mark their
> > stuff as non-content-accessible in their respective chrome manifests (unless
> > it needs to be).
> 
> resource:// URIs are non-content-accessible by default (if not marked
> contentaccessible=yes in manifests).

This is the case for firefox code, but not for extension/add-on code, in the current version of your patch, right? But system add-ons are technically add-ons, they are just (sometimes) in the app bundle, but they can also be updated out-of-band from the main app, so I kind of assume that their manifest locations will (sometimes?) be considered as add-on manifests, and thus get the "content-accessible by default" treatment. If true, we will need to set contentaccessible=no explicitly in those manifests. This looks like it might be true at least for mortar flash/pdfium as well as pdfjs, and soon ( https://docs.google.com/document/d/1_cl1ZtcS7zFy4JTmh4ShqaVl0HHnXrN2oHBcJMgN6lU/edit ) maybe also devtools.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 139 • 7 years ago

(In reply to :Gijs from comment #138)
> (In reply to Chung-Sheng Fu [:cfu] from comment #137)
> > (In reply to :Gijs from comment #133)
> > > Some general comments:
> > > 
> > > - did someone review the system add-ons we ship? On the assumption that they
> > > count as NS_BOOTSTRAPPED_LOCATION, we should make sure that we mark their
> > > stuff as non-content-accessible in their respective chrome manifests (unless
> > > it needs to be).
> > 
> > resource:// URIs are non-content-accessible by default (if not marked
> > contentaccessible=yes in manifests).
> 
> This is the case for firefox code, but not for extension/add-on code, in the
> current version of your patch, right? But system add-ons are technically
> add-ons, they are just (sometimes) in the app bundle, but they can also be
> updated out-of-band from the main app, so I kind of assume that their
> manifest locations will (sometimes?) be considered as add-on manifests, and
> thus get the "content-accessible by default" treatment. If true, we will
> need to set contentaccessible=no explicitly in those manifests. This looks
> like it might be true at least for mortar flash/pdfium as well as pdfjs, and
> soon (
> https://docs.google.com/document/d/
> 1_cl1ZtcS7zFy4JTmh4ShqaVl0HHnXrN2oHBcJMgN6lU/edit ) maybe also devtools.

I believe add-ons also follow the rule that their stuff are content-accessible only if their locations are explicitly marked contentaccessible=yes.
Take pdfjs for example, consider a image element

<img src="resource://pdf.js/web/images/loading-icon.gif">

under the conditions:

1. without this patch
- can load the image
2. with this patch
2.1 security.all_resource_uri_content_accessible = false
- cannot load the image
2.2 security.all_resource_uri_content_accessible = true
- can load the image

Hope I did not misunderstand your concern.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 142 • 7 years ago

Comment on attachment 8885980 [details]
Bug 863246 - Fix test failures

Could you also please help review this patch?
It fixes other test failures by setting the preference security.all_resource_uri_content_accessible = true, using SpecialPowers.pushPrefEnv, in order to temporarily allow web content to access all resource:// URIs.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 143 • 7 years ago

(In reply to :Gijs from comment #133)
> Some general comments:
> 
> - did someone review the system add-ons we ship? On the assumption that they
> count as NS_BOOTSTRAPPED_LOCATION, we should make sure that we mark their
> stuff as non-content-accessible in their respective chrome manifests (unless
> it needs to be).
> - is there a bug on file to remove the add-on exception for 57? Is it even
> helpful to have that right now, given 56 has only 2 weeks of trunk life left?
> - has someone talked with the webextension folks and checked how the
> moz-extension protocol currently behaves?
> - if we keep the exception for add-ons, could we create a testcase that
> checks the extension side of things works?

We need your expertise to help with Gijs' concerns about add-on and WebExtension.
Thanks.

	:Gijs (he/him)
	Comment 144 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #139)
> I believe add-ons also follow the rule that their stuff are
> content-accessible only if their locations are explicitly marked
> contentaccessible=yes.
> Take pdfjs for example, consider a image element
> 
> <img src="resource://pdf.js/web/images/loading-icon.gif">
> 
> under the conditions:
> 
> 1. without this patch
> - can load the image
> 2. with this patch
> 2.1 security.all_resource_uri_content_accessible = false
> - cannot load the image
> 2.2 security.all_resource_uri_content_accessible = true
> - can load the image
> 
> Hope I did not misunderstand your concern.

This is the opposite of what this comment said:

(In reply to Boris Zbarsky [:bz] from comment #54)
> You want to talk to Jorge
> about addon compat fallout, though I see that the patch makes addons opt-in
> instead of opt-out to the new blocking behavior, so maybe there isn't any.

and the opposite of what a naive reading of your patches suggests, so then your patches aren't doing what they suggest they're doing, which is concerning... There is *explicit* logic in your patches to make an exception for add-ons (such that they are content-accessible by default unless they use contentaccessible=no in the manifest), so at this point I'm completely lost at what you're trying to do, and what is actually happening, and whether those two things match or not.

Specifically, your patch says:

    // This is a browser extension. By default, extension resources are
    // content-accessible unless the manifest opts out.

	:Gijs (he/him)
	Comment 145 • 7 years ago

Ugh, so, re-reading more comments here, it seems you already plan to land this in 57? In which case, why do we need to specialcase add-ons at all?

	:Gijs (he/him)
	Comment 146 • 7 years ago
mozreview-review

Comment on attachment 8885980 [details]
Bug 863246 - Fix test failures

https://reviewboard.mozilla.org/r/156768/#review165098

::: browser/base/content/test/static/browser_all_files_referenced.js:171
(Diff revision 7)
> -
> +  // dom/html/ImageDocument.cpp
> +  {file: "resource://content-accessible/ImageDocument.css"},
> +  {file: "resource://content-accessible/TopLevelImageDocument.css"},
> +  // dom/html/VideoDocument.cpp
> +  {file: "resource://content-accessible/TopLevelVideoDocument.css"},

This wasn't necessary before, so I don't understand why it is necessary after your patches. Please actually check why this is failing in the test. If you don't want to do that in this bug, file a followup and reference it in a comment.

::: devtools/client/debugger/test/mochitest/browser_dbg_worker-source-map.js:23
(Diff revision 7)
> +    // Temporarily allow content to access all resource:// URIs.
> +    yield pushPrefs(["security.all_resource_uri_content_accessible", true]);
> +

This is only for resource://devtools/server/worker.js, right (based on comment 102)? But that file is not used from the test, it's used from the implementation (https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/devtools/server/main.js#817 ). I have 2 concerns about this:

1) I suspect this would wallpaper the test but the feature (ie worker debugging) would be broken.
2) Why is that breaking? Is it not being loaded by privileged code, but by content code? Uhhh... In Dutch, no doubt due to our history of flooding, we would say "I feel a wet patch!" - either we're not using the right triggering principal for the load, or worse, we load debugger infrastructure into a content context, which would suggest... bad things. It turns out this stuff is turned off by default and you'd need to open the debugger, and potentially switch to the old debugger frontend, but this feels like priv-esc stuff waiting to happen.

::: devtools/shared/platform/content/test/test_clipboard.html:39
(Diff revision 7)
> +      ["security.all_resource_uri_content_accessible", true]
> +    ]
> +  });
> +
> +  // Load script.
> +  await (() => new Promise((aResolve) => {

Nit: no 'aArgs' in most new JS code (definitely not in devtools)

	:Gijs (he/him)
	Comment 147 • 7 years ago

(In reply to :Gijs from comment #146)
> :::
> devtools/client/debugger/test/mochitest/browser_dbg_worker-source-map.js:23
> (Diff revision 7)
> > +    // Temporarily allow content to access all resource:// URIs.
> > +    yield pushPrefs(["security.all_resource_uri_content_accessible", true]);
> > +
> 
> This is only for resource://devtools/server/worker.js, right (based on
> comment 102)? But that file is not used from the test, it's used from the
> implementation
> (https://dxr.mozilla.org/mozilla-central/rev/
> 1b065ffd8a535a0ad4c39a912af18e948e6a42c1/devtools/server/main.js#817 ). I
> have 2 concerns about this:
> 
> 1) I suspect this would wallpaper the test but the feature (ie worker
> debugging) would be broken.
> 2) Why is that breaking? Is it not being loaded by privileged code, but by
> content code? Uhhh... In Dutch, no doubt due to our history of flooding, we
> would say "I feel a wet patch!" - either we're not using the right
> triggering principal for the load, or worse, we load debugger infrastructure
> into a content context, which would suggest... bad things. It turns out this
> stuff is turned off by default and you'd need to open the debugger, and
> potentially switch to the old debugger frontend, but this feels like
> priv-esc stuff waiting to happen.


Yulia, I think you know the debugger stuff here? Can you clarify how we use resource://devtools/server/worker.js , and why tests might break if we no longer allow loading resource:// URIs from web content-type contexts? I am worried that the way that script works would allow workers to access the debugger in a similar way.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 148 • 7 years ago

(In reply to comment #144)
Oh you are right, there are some browser extensions whose locations are content-accessible by default.
In my local test, they are activity-stream, formautofill, onboarding, shield-recipe-client.
I'm not sure why not all browser extensions are regarded as NS_EXTENSION_LOCATION or NS_BOOTSTRAPPED_LOCATION (e.g., pdfjs).
Should I mark all browser extensions non-content-accessible?
But it will lead to another question that if all browser extensions have to be non-content-accessible, do we still need the condition?

> if ((cx.mType == NS_EXTENSION_LOCATION) || (cx.mType == NS_BOOTSTRAPPED_LOCATION)) {
>   // This is a browser extension. By default, extension resources are
>   // content-accessible unless the manifest opts out.
>   contentAccessible = !(flags & nsChromeRegistry::CONTENT_INACCESSIBLE);
> } else {

(In reply to comment #145)
Yes, we decided to follow Jorge (Comment 55) and Andy's (Comment 60) suggestion land this in 57.
But I'm not sure if we still have to track add-on exceptions.

	:Gijs (he/him)
	Comment 149 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #148)
> (In reply to comment #144)
> Oh you are right, there are some browser extensions whose locations are
> content-accessible by default.
> In my local test, they are activity-stream, formautofill, onboarding,
> shield-recipe-client.
> I'm not sure why not all browser extensions are regarded as
> NS_EXTENSION_LOCATION or NS_BOOTSTRAPPED_LOCATION (e.g., pdfjs).
> Should I mark all browser extensions non-content-accessible?
> But it will lead to another question that if all browser extensions have to
> be non-content-accessible, do we still need the condition?

The condition is meant for third-party add-ons who may expect their resource: stuff to be content-accessible.

> > if ((cx.mType == NS_EXTENSION_LOCATION) || (cx.mType == NS_BOOTSTRAPPED_LOCATION)) {
> >   // This is a browser extension. By default, extension resources are
> >   // content-accessible unless the manifest opts out.
> >   contentAccessible = !(flags & nsChromeRegistry::CONTENT_INACCESSIBLE);
> > } else {
> 
> (In reply to comment #145)
> Yes, we decided to follow Jorge (Comment 55) and Andy's (Comment 60)
> suggestion land this in 57.
> But I'm not sure if we still have to track add-on exceptions.

If we land this in 57 then I think we should remove the exception for add-ons, as there will be no more extensions that use resource: that we have to keep compatibility for. I think this also applies to updating the APIs on the resource: protocol handler to include the flags (that is, we then don't need to keep backward compat).

	Chung-Sheng Fu [:cfu] Assignee
	Comment 150 • 7 years ago

(In reply to :Gijs from comment #149)
Thank you very much for the instruction.
I will modify the patches and ask you to review them again.

	:Gijs (he/him)
	Comment 151 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #150)
> (In reply to :Gijs from comment #149)
> Thank you very much for the instruction.
> I will modify the patches and ask you to review them again.

Thank you! Note that for the core patches, you will need review from relevant peers (ie not me, more like bz/billm like before).

	Jorge Villalobos [:jorgev] (he/him)
	Comment 152 • 7 years ago

Yes, if the target is 57 and there's no plan to uplift, legacy add-ons are no longer a concern.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 154 • 7 years ago

This patch slightly modifies attachment 8866700 [details], removing add-on compatibility code.
Browser extension resources will be non-content-accessible by default, and add-on will not be able to make anything content-accessible by calling setSubstitution.
But I found that some resources of the browser extension "onboarding" are referenced in about:home and about:newtab, so I add the contentaccessible=yes flag in its manifest and make resource://onboarding/ content-accessible.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 155 • 7 years ago

(In reply to :Gijs from comment #146)
> Comment on attachment 8885980 [details]
> Bug 863246 - Fix test failures
> 
> https://reviewboard.mozilla.org/r/156768/#review165098
> 
> ::: browser/base/content/test/static/browser_all_files_referenced.js:171
> (Diff revision 7)
> > -
> > +  // dom/html/ImageDocument.cpp
> > +  {file: "resource://content-accessible/ImageDocument.css"},
> > +  {file: "resource://content-accessible/TopLevelImageDocument.css"},
> > +  // dom/html/VideoDocument.cpp
> > +  {file: "resource://content-accessible/TopLevelVideoDocument.css"},
> 
> This wasn't necessary before, so I don't understand why it is necessary
> after your patches. Please actually check why this is failing in the test.
> If you don't want to do that in this bug, file a followup and reference it
> in a comment.

It extracts constant strings from libxul and store them here:
http://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#495
So the new URIs resource://content-accessible/ of the 3 files are ignored.
If I don't add them to whitelist, I may add one more condition in order to store them in gReferencesFromCode (like string.startsWith("resource://content-accessible/")).
Which solution seems better to you?

	:Gijs (he/him)
	Comment 156 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #155)
> If I don't add them to whitelist, I may add one more condition in order to
> store them in gReferencesFromCode (like
> string.startsWith("resource://content-accessible/")).
> Which solution seems better to you?

Adding the condition to add them into gReferencesFromCode sounds better, but please also file a followup bug. I think the libxul reading code should go after the manifest parsing code, and the libxul reading code should accept resource:// URLs for all registered resource packages.

	:Gijs (he/him)
	Comment 157 • 7 years ago
mozreview-review

Comment on attachment 8889057 [details]
Bug 863246 - Remove compatibility for add-on

https://reviewboard.mozilla.org/r/160102/#review165650

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:300
(Diff revision 1)
> -  // Because add-ons use this API to register new substitutions we have to allow it :/
> -  return SetSubstitutionWithFlags(root, baseURI, nsISubstitutingProtocolHandler::ALLOW_CONTENT_ACCESS);
> +  // Add-ons use this API but they should not be able to make anything
> +  // content-accessible.
> +  return SetSubstitutionWithFlags(root, baseURI, 0);

I think this works, but I also think you could probably just alter `SetSubstitution` to take the extra flags parameter, and make the extra param optional in the IDL file, to avoid adding the extra `SetSubstitutionWithFlags` method. I'm assuming you audited and updated in-tree callers of SetSubstitution() in the earlier patches. :-)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 158 • 7 years ago
mozreview-review

Comment on attachment 8889057 [details]
Bug 863246 - Remove compatibility for add-on

https://reviewboard.mozilla.org/r/160102/#review166372

I think it would be a good idea to squash all the non-test patches together. This patch doesn't really stand on its own.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 159 • 7 years ago

(In reply to comment #156)
I filed a bug 1384389.
Please take a look if the description is clear.

(In reply to comment #157)
We added a new interface in order to make it easier to distinguish add-on calls and internal uses.
If we don't have to worry about it, I will modify the prototype of the original method and remove the new one.

(In reply to comment #158)
I also tend to do this.
But should I merge the patch reviewed by bz together?

	Jim Blandy :jimb
	Comment 160 • 7 years ago

(In reply to :Gijs from comment #146)
> :::
> devtools/client/debugger/test/mochitest/browser_dbg_worker-source-map.js:23
> (Diff revision 7)
> > +    // Temporarily allow content to access all resource:// URIs.
> > +    yield pushPrefs(["security.all_resource_uri_content_accessible", true]);
> > +
> 
> This is only for resource://devtools/server/worker.js, right (based on
> comment 102)? But that file is not used from the test, it's used from the
> implementation
> (https://dxr.mozilla.org/mozilla-central/rev/
> 1b065ffd8a535a0ad4c39a912af18e948e6a42c1/devtools/server/main.js#817 ). I
> have 2 concerns about this:
> 
> 1) I suspect this would wallpaper the test but the feature (ie worker
> debugging) would be broken.
> 2) Why is that breaking? Is it not being loaded by privileged code, but by
> content code? Uhhh... In Dutch, no doubt due to our history of flooding, we
> would say "I feel a wet patch!" - either we're not using the right
> triggering principal for the load, or worse, we load debugger infrastructure
> into a content context, which would suggest... bad things. It turns out this
> stuff is turned off by default and you'd need to open the debugger, and
> potentially switch to the old debugger frontend, but this feels like
> priv-esc stuff waiting to happen.

Our worker debugging arrangement is complicated. For JavaScript debugging, the devtools server must run on the same thread as the debuggee. Since our server is privileged JavaScript code, this requires the worker thread to have several JavaScript globals: one unprivileged global for the worker content, and several privileged globals for the server and the JS modules that it uses.

I believe the worker.js script is the initial script that is loaded into a privileged global on the worker thread. That call to `dbg.initialize(".../worker.js")` is a call to nsIWorkerDebugger::initialize:

http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/workers/nsIWorkerDebugger.idl#42

It seems to me that a resource: URL should be completely appropriate to pass to that method. If the changes here are preventing that, then either they've missed something, or the implementation of nsIWorkerDebugger is not setting its privileges properly.

Worker threads are an unusual environment, lacking some facilities available to the main thread. I would make sure that the way the patch decides whether to permit a resource: scheme works properly in worker threads.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 161 • 7 years ago

This patch is created according to Jim's introduction in comment 160.
An additional security flag will be added to the load info of any channel established to load debugger scripts.
It can solve the resource://devtools/server/worker.js problem.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 162 • 7 years ago

I'm afraid I don't understand what is going on here. Reading Jim's comment, it that like a worker thread has both privileged and unprivileged globals. One of the privileged globals is trying to load a resource: URI and that is failing. The patch here seems to allow someone to set a channel flag that will allow all resource: URIs to be loaded, even if they're not content-accessible.

What I don't understand here is why we're getting to this point. If the worker global is privileged, why are we doing this check at all?

Anyway, I'm going to forward the needinfo to bz since he knows a lot more about the security manager.

	Boris Zbarsky [:bzbarsky]
	Comment 163 • 7 years ago

The security manager doesn't even run on the worker thread.  And necko activity doesn't happen there either.  So all the real work here is on the main thread.

Looking at WorkerDebugger::Initialize(), it does AssertIsOnMainThread().  That said, it then proceeds to send a runnable to the worker to do the load, which sends a runnable back to the main thread to actually do the load.  The runnable going to the worker is CompileDebuggerScriptRunnable.  It then calls workers::scriptloader::LoadMainScript with DebuggerScript as the aWorkerScriptType.  This bounces through ScriptLoaderRunnable::RunInternal, thence to ScriptLoaderRunnable::LoadScript which passes the principal of the WorkerPrivate to ChannelFromScriptURL.  That, of course, is the untrusted principal of the worker.  

In ChannelFromScriptURL we check aWorkerScriptType and if it's DebuggerScript ensure that the URI being loaded is URI_IS_UI_RESOURCE, and set the SEC_ALLOW_CHROME flag.  So the intent is certainly that the DebuggerScript case should be able to load only things like chrome:// and resource://, and presumably all of them.

But we're using the worker's principal for the security checks, so the load gets disallowed.

I think the right thing to do here is to use the system principal for the load when the script type is DebuggerScript.  So change ScriptLoaderRunnable::LoadScript to check mWorkerScriptType and adjust the principal accordingly.  That's assuming that we control the urls of all loads of type DebuggerScript and can't end up with some sort of confused-deputy attack.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 164 • 7 years ago

Only one line added :p
I still put it in ChannelFromScriptURL instead of ScriptLoaderRunnable::LoadScript.
Is nsContentUtils::GetSystemPrincipal the right way to use system principal?

Besides, I would like to merge some patches as Bill mentioned in comment 158.
But the patches are not reviewed by the same reviewer.
Would you mind if I merge them together?

	Boris Zbarsky [:bzbarsky]
	Comment 165 • 7 years ago

> I still put it in ChannelFromScriptURL instead of ScriptLoaderRunnable::LoadScript.

That seems really odd to me.  We should just get the right principal up front instead of passing in the wrong thing and then munging it in the callee.  That's just asking for trouble if anyone else does anything with that principal in here.  Minimizing the line count is _not_ the right metric to optimize for here.

> Is nsContentUtils::GetSystemPrincipal the right way to use system principal?

It's the right thing in this case, yes.

Merging the patches is fine; just list all the reviewers in the commit message.  It might be nice to say who reviewed what in the non-first-line parts of the commit message.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 166 • 7 years ago

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #165)
> > I still put it in ChannelFromScriptURL instead of ScriptLoaderRunnable::LoadScript.
> 
> That seems really odd to me.  We should just get the right principal up
> front instead of passing in the wrong thing and then munging it in the
> callee.  That's just asking for trouble if anyone else does anything with
> that principal in here.  Minimizing the line count is _not_ the right metric
> to optimize for here.

I am going to start from here

http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#895

But I noticed there is a check NS_LoadGroupMatchesPrincipal (also found in ChannelFromScriptURL)

Is it necessary to take care about it?

If we have to, my plan is to create a new load group here, using system principal and load context from mWorkerPrivate.

Is this a reasonable approach?

	Boris Zbarsky [:bzbarsky]
	Comment 167 • 7 years ago

> But I noticed there is a check NS_LoadGroupMatchesPrincipal (also found in ChannelFromScriptURL)

That sounds like a question for bkelly.

> If we have to, my plan is to create a new load group here

That doesn't sound like the right approach at first glance.  Better would be to skip the assert for system principal or something, but again check with bkelly.

	Ben Kelly [:bkelly, not reviewing]
	Comment 168 • 7 years ago

First, let me say I don't completely understand our worker debugger code.  Unfortunately all the people who knew this code have moved on to other things.

Let me see if I understand, though:

a. We are creating a worker for a document with a content principal
b. The worker is a "DebuggerScript" type which means the worker URL is actually chrome: or resource:
c. "DebuggerScript" scripts are loaded on top of a WorkerPrivate that already exists and could be running a content script
d. You are proposing to trigger the network load of the DebuggerScript with system principal instead of the content principal

So I have a few questions:

1. Does this change the result principal from the channel at all?  I thought chrome: and resource: URLs always ended up as system principal, but I'm not sure.

2. If this does change the result principal coming out of the channel, why is that safe?

3. What happens to the overall principal of the WorkerPrivate?

I suspect that perhaps this will not be safe.  It seems we run these debugger scripts on the same WorkerPrivate that is already running a content script.  I worry that this will change the entire WorkerPrivate over to system principal.

Can you please add some instrumentation to see what the principal of the WorkerPrivate is before and after this line?

http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#674

	Boris Zbarsky [:bzbarsky]
	Comment 169 • 7 years ago

> a. We are creating a worker for a document with a content principal

That's correct.  And the worker itself is also content principal.

Then we are loading a debugger script for the worker, into the worker debugger global.  This is NOT the load of the worker script itself; that's a separate load that's already happened.

> "DebuggerScript" scripts are loaded on top of a WorkerPrivate that already exists and could be running a content script

"on top of" in some sense yes, but into a separate global.

> Does this change the result principal from the channel at all?

It shouldn't, afaik.

> 3. What happens to the overall principal of the WorkerPrivate?

I don't believe it changes, but it's worth double-checking.  In particular, IsMainWorkerScript() will be false for this debugger load (because it checks the type).  And OnStreamCompleteInternal guards the principal-changing bits on IsMainWorkerScript().

	Ben Kelly [:bkelly, not reviewing]
	Comment 170 • 7 years ago

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #169)
> > Does this change the result principal from the channel at all?
> 
> It shouldn't, afaik.
> 
> > 3. What happens to the overall principal of the WorkerPrivate?
> 
> I don't believe it changes, but it's worth double-checking.  In particular,
> IsMainWorkerScript() will be false for this debugger load (because it checks
> the type).  And OnStreamCompleteInternal guards the principal-changing bits
> on IsMainWorkerScript().

Ok, that makes sense.

I guess the next question is why does the NS_LoadGroupMatchesPrincipal() fail?  It only checks that some origin attributes haven't been lost AFAICT:

http://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#810

It used to check appId, but only seems to check IsInIsolatedMozBrowserElement now.  What is failing in here for you?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 171 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #170)
> (In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #169)
> > > Does this change the result principal from the channel at all?
> > 
> > It shouldn't, afaik.
> > 
> > > 3. What happens to the overall principal of the WorkerPrivate?
> > 
> > I don't believe it changes, but it's worth double-checking.  In particular,
> > IsMainWorkerScript() will be false for this debugger load (because it checks
> > the type).  And OnStreamCompleteInternal guards the principal-changing bits
> > on IsMainWorkerScript().
> 
> Ok, that makes sense.
> 
> I guess the next question is why does the NS_LoadGroupMatchesPrincipal()
> fail?  It only checks that some origin attributes haven't been lost AFAICT:
> 
> http://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#810
> 
> It used to check appId, but only seems to check
> IsInIsolatedMozBrowserElement now.  What is failing in here for you?

I'm changing this line http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#895 by

> diff --git a/dom/workers/ScriptLoader.cpp b/dom/workers/ScriptLoader.cpp
> index 3a29d0137050..079c8d5f37e1 100644
> --- a/dom/workers/ScriptLoader.cpp
> +++ b/dom/workers/ScriptLoader.cpp
> @@ -892,7 +892,7 @@ private:
> 
>      WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
> 
> -    nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
> +    nsIPrincipal* principal = (mWorkerScriptType == DebuggerScript) ? nsContentUtils::GetSystemPrincipal() : mWorkerPrivate->GetPrincipal();
>      nsCOMPtr<nsILoadGroup> loadGroup = mWorkerPrivate->GetLoadGroup();
>      MOZ_DIAGNOSTIC_ASSERT(principal);

So I'm not going to change the principal of the WorkerPrivate.

I also added some log to observe the principal of the WorkerPrivate before and after calling scriptloader::LoadMainScript as you mentioned in comment 168.

> diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
> index 5b497a08c6e3..21c0e5961b81 100644
> --- a/dom/workers/WorkerPrivate.cpp
> +++ b/dom/workers/WorkerPrivate.cpp
> @@ -671,8 +671,16 @@ private:
> 
>      ErrorResult rv;
>      JSAutoCompartment ac(aCx, global);
> +    do {
> +      auto principal = static_cast<BasePrincipal*>(aWorkerPrivate->GetPrincipalDontAssertMainThread());
> +      printf("_(:3_|/_)_ 1 %p:%d\n", principal, principal->Kind());
> +    } while (false);
>      scriptloader::LoadMainScript(aWorkerPrivate, mScriptURL,
>                                   DebuggerScript, rv);
> +    do {
> +      auto principal = static_cast<BasePrincipal*>(aWorkerPrivate->GetPrincipalDontAssertMainThread());
> +      printf("_(:3_|/_)_ 2 %p:%d\n", principal, principal->Kind());
> +    } while (false);
>      rv.WouldReportJSException();
>      // Explicitly ignore NS_BINDING_ABORTED on rv.  Or more precisely, still
>      // return false and don't SetWorkerScriptExecutedSuccessfully() in that

and the log shows

> 3 INFO Attaching to worker with url 'code_WorkerActor.attachThread-worker.js'.
> 24 INFO Attaching to thread.
> GECKO(39316) | _(:3_|/_)_ 1 0x11b1edde0:1
> GECKO(39316) | _(:3_|/_)_ 2 0x11b1edde0:1
> GECKO(39316) | EMITTING: emit(connectionchange, opened, [object Object]) from
> GECKO(39316) | EMITTING: emit(newGlobal, [object Object]) from

The principal of the WorkerPrivate doesn't change even system principal is used to load debugger scripts.

So far the assertion of NS_LoadGroupMatchesPrincipal never fails, but I'm not sure if there is any case that the load group in the WorkerPrivate doesn't match system principal.

Considering bz's suggestion in comment 167, I may change this line http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#899 (and also line 196, in ChannelFromScriptURL) to

> NS_ENSURE_TRUE(principal->GetIsSystemPrincipal() || NS_LoadGroupMatchesPrincipal(loadGroup, principal), NS_ERROR_FAILURE);

Does it look fine?

	Ben Kelly [:bkelly, not reviewing]
	Comment 172 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #171)
> So far the assertion of NS_LoadGroupMatchesPrincipal never fails, but I'm
> not sure if there is any case that the load group in the WorkerPrivate
> doesn't match system principal.
> 
> Considering bz's suggestion in comment 167, I may change this line
> http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#899
> (and also line 196, in ChannelFromScriptURL) to
> 
> > NS_ENSURE_TRUE(principal->GetIsSystemPrincipal() || NS_LoadGroupMatchesPrincipal(loadGroup, principal), NS_ERROR_FAILURE);
> 
> Does it look fine?

I think you should leave the NS_LoadGroupMatchesPrincipal() in for now.  If we don't think the checks makes sense for system principal then I would recommend adding the check inside NS_LoadGroupMatchesPrincipal() where we check for null principal.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 173 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #172)
> (In reply to Chung-Sheng Fu [:cfu] from comment #171)
> I think you should leave the NS_LoadGroupMatchesPrincipal() in for now.  If
> we don't think the checks makes sense for system principal then I would
> recommend adding the check inside NS_LoadGroupMatchesPrincipal() where we
> check for null principal.

I'm sorry I'm not sure whether the checks make sense for system principal or not.

What if we skip the assertion for debugger scripts instead of system principal, like

> NS_ENSURE_TRUE((mWorkerScriptType == DebuggerScript) || NS_LoadGroupMatchesPrincipal(loadGroup, principal), NS_ERROR_FAILURE);

	Ben Kelly [:bkelly, not reviewing]
	Comment 174 • 7 years ago

I don't understand why you want to remove the check if its not failing.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 175 • 7 years ago

I will just change this line http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#900.

Debugger scripts will not use principal from the WorkerPrivate.

Instead, system principal will be assigned.

The NS_LoadGroupMatchesPrincipal checks will be kept.

	Ben Kelly [:bkelly, not reviewing]
	Comment 176 • 7 years ago

Comment on attachment 8896830 [details] [diff] [review]
0001-Bug-863246-Use-system-principal-to-load-debugger-scr.patch

Review of attachment 8896830 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/workers/ScriptLoader.cpp
@@ +893,5 @@
>      WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
>  
> +    nsIPrincipal* principal = (mWorkerScriptType == DebuggerScript) ?
> +                              nsContentUtils::GetSystemPrincipal() :
> +                              mWorkerPrivate->GetPrincipal();

Please add a comment here about why we need to use the system principal for debugger scripts.  Also, note that its safe to do since we never load the top level script as a debugger script.

Also, please add an assert like:

  MOZ_ASSERT_IF(mIsMainScript, mWorkerScriptType != DebuggerScript);

	Chung-Sheng Fu [:cfu] Assignee
	Comment 177 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #176)
> Comment on attachment 8896830 [details] [diff] [review]
> 0001-Bug-863246-Use-system-principal-to-load-debugger-scr.patch
> 
> Review of attachment 8896830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/workers/ScriptLoader.cpp
> @@ +893,5 @@
> >      WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
> >  
> > +    nsIPrincipal* principal = (mWorkerScriptType == DebuggerScript) ?
> > +                              nsContentUtils::GetSystemPrincipal() :
> > +                              mWorkerPrivate->GetPrincipal();
> 
> Please add a comment here about why we need to use the system principal for
> debugger scripts.  Also, note that its safe to do since we never load the
> top level script as a debugger script.
> 
> Also, please add an assert like:
> 
>   MOZ_ASSERT_IF(mIsMainScript, mWorkerScriptType != DebuggerScript);

I have a question about this assertion.

Since the load starts from WorkerDebugger::Initialize
http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/workers/WorkerPrivate.cpp#4342

The runnable calls scriptloader::LoadMainScript with aWorkerScriptType = DebuggerScript
http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/workers/WorkerPrivate.cpp#674

Then LoadMainScript calls LoadAllScripts with aIsMainScript = true
http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/workers/ScriptLoader.cpp#2255

So we finally have mIsMainScript = true and mWorkerScriptType = DebuggerScript in the loader runnable
http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/workers/ScriptLoader.cpp#2118

And the assertion fails.

Is there still something that needs changes?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 178 • 7 years ago

Besides, I commented the change with

// For JavaScript debugging, the devtools server must run on the same
// thread as the debuggee, indicating the worker uses content principal.
// However, in Bug 863246, web content will no longer be able to load
// resource:// URIs by default, so we need system principal to load
// debugger scripts.

Is it correct?

	Ben Kelly [:bkelly, not reviewing]
	Comment 179 • 7 years ago

So, I think I partly misunderstood comment 169.  The mIsMainScript will be true, but we expect all code to use IsMainWorkerScript() instead, which filters on the type.

So maybe make the assertion:

  MOZ_ASSERT_IF(IsMainWorkerScript(), mWorkerScriptType != DebuggerScript);

The comment looks good.  Thanks!