863246 - resource:// URIs leak information (Tor 8725)

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Tom Adams]

Attachment: A copy of the page I linked to, in case of link rot

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.

Comment 1

[earthasa]

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.

Comment 2

[Loic]

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?

Comment 3

[Patrick McManus [:mcmanus]]

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

Comment 4

[Patrick McManus [:mcmanus]]

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?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 7

[Tom S [:evilpie]]

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

Comment 8

[François Marier [:francois]]

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/

Comment 9

[Dave Huseby [:huseby]]

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.

Comment 10

[Arthur Edelstein [:arthur]]

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

Comment 11

[Dave Huseby [:huseby]]

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

Comment 12

[Tom S [:evilpie]]

Attachment: Proof of concept patch

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?

Comment 13

[Tom S [:evilpie]]

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

Comment 14

[bugzilla-ng]

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

Any ideas?

Comment 15

[Selek Respa]

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

Comment 16

[Tom S [:evilpie]]

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)

Comment 17

[Selek Respa]

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

Comment 18

[Tom S [:evilpie]]

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.

Comment 19

[Tom S [:evilpie]]

Attachment: Introduce a contentaccessible flag for resource substitutions

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.

Comment 20

[Tom S [:evilpie]]

Attachment: Alias gre-res to /gre/res and make it contentaccessible

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.

Comment 21

[Mike Hommey [:glandium]]

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.

Comment 22

[Mike Hommey [:glandium]]

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.

Comment 23

[Tom S [:evilpie]]

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://.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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...

Comment 25

[Tom S [:evilpie]]

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 :)

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 27

[Tom S [:evilpie]]

Attachment: v2 - Introduce a contentaccessible flag for resource substitutions

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.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 29

[Tom S [:evilpie]]

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

Comment 30

[Tom S [:evilpie]]

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

Comment 31

[Tom S [:evilpie]]

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

Comment 32

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Use system principal to load debugger scripts

Review commit: https://reviewboard.mozilla.org/r/138314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138314/

Comment 33

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Add test case

Review commit: https://reviewboard.mozilla.org/r/138316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138316/

Comment 34

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/1-2/

Comment 35

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138316/diff/1-2/

Comment 36

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138316/diff/2-3/

Comment 37

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/2-3/

Comment 38

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138316/diff/3-4/

Comment 39

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Deny resource:// access to web content

The caps part

Review commit: https://reviewboard.mozilla.org/r/140412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140412/

Comment 40

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/3-4/

Comment 41

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138316/diff/4-5/

Comment 42

[Chung-Sheng Fu [:cfu]]

(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

Comment 43

[Bobby Holley (:bholley)]

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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

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.

Comment 45

[Arthur Edelstein [:arthur]]

(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.

Comment 46

[Chung-Sheng Fu [:cfu]]

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!

Comment 47

[Georg Koppen]

(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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 49

[Georg Koppen]

(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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

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....

Comment 51

[Arthur Edelstein [:arthur]]

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.

Comment 52

[Chung-Sheng Fu [:cfu]]

(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?

Comment 53

[Tom S [:evilpie]]

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.

Comment 54

[Boris Zbarsky [:bzbarsky]]

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.

Comment 55

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 56

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 57

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 58

[Chung-Sheng Fu [:cfu]]

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!

Comment 59

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 60

[Andy McKay]

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.

Comment 61

[Chung-Sheng Fu [:cfu]]

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

Comment 62

[Chung-Sheng Fu [:cfu]]

(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.

Comment 63

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> 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.

Comment 64

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/4-5/

Comment 65

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/5-6/

Comment 66

[Ethan Tseng [:ethan]]

Move from milestone 1 to 2 according to comment 60.

Comment 67

[Chung-Sheng Fu [:cfu]]

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?

Comment 68

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 69

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Content can only load resource:// URIs declared content-accessible in manifests

The caps part

Review commit: https://reviewboard.mozilla.org/r/147956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147956/

Comment 70

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Alias resource://gre-res to resource://gre/res and make it content-accessible

Review commit: https://reviewboard.mozilla.org/r/147958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147958/

Comment 71

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Add test cases

Review commit: https://reviewboard.mozilla.org/r/147960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147960/

Comment 72

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/6-7/

Comment 73

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/7-8/

Comment 74

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/1-2/

Comment 75

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147958/diff/1-2/

Comment 76

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/1-2/

Comment 77

[Chung-Sheng Fu [:cfu]]

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.

Comment 78

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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(...);
});

Comment 79

[Chung-Sheng Fu [:cfu]]

(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!

Comment 80

[Boris Zbarsky [:bzbarsky]]

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.

Comment 81

[Boris Zbarsky [:bzbarsky]]

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

Comment 82

[Chung-Sheng Fu [:cfu]]

(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!

Comment 83

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/8-9/

Comment 84

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/2-3/

Comment 85

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147958/diff/2-3/

Comment 86

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/2-3/

Comment 87

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/3-4/

Comment 88

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/4-5/

Comment 89

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Fix caps/tests/mochitest/test_bug292789.html

Review commit: https://reviewboard.mozilla.org/r/150738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150738/

Comment 90

[Chung-Sheng Fu [:cfu]]

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?

Comment 91

[Boris Zbarsky [:bzbarsky]]

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).

Comment 92

[Chung-Sheng Fu [:cfu]]

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

Comment 93

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/9-10/

Comment 94

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/3-4/

Comment 95

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147958/diff/3-4/

Comment 96

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/5-6/

Comment 97

[Chung-Sheng Fu [:cfu]]

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?

Comment 98

[Boris Zbarsky [:bzbarsky]]

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.

Comment 99

[Selek Respa]

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.

Comment 100

[Boris Zbarsky [:bzbarsky]]

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

Comment 101

[Boris Zbarsky [:bzbarsky]]

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.

Comment 102

[Chung-Sheng Fu [:cfu]]

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.

Comment 103

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Fix test failures

Review commit: https://reviewboard.mozilla.org/r/156768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156768/

Comment 104

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Move resources that need to be exposed to web content to locations that are marked as contentaccessible

Review commit: https://reviewboard.mozilla.org/r/156770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156770/

Comment 105

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/10-11/

Comment 106

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/4-5/

Comment 107

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/6-7/

Comment 108

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147958/diff/4-5/

Comment 109

[Chung-Sheng Fu [:cfu]]

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.

Comment 110

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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".

Comment 111

[Chung-Sheng Fu [:cfu]]

(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!

Comment 112

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/11-12/

Comment 113

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/5-6/

Comment 114

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/1-2/

Comment 115

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/1-2/

Comment 116

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/7-8/

Comment 117

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/12-13/

Comment 118

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/6-7/

Comment 119

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/2-3/

Comment 120

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/8-9/

Comment 121

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/2-3/

Comment 122

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/3-4/

Comment 123

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/3-4/

Comment 124

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/9-10/

Comment 125

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/4-5/

Comment 126

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/10-11/

Comment 127

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/5-6/

Comment 128

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Fix test failures

browser/base/content/test/static/browser_parsable_css.js

Review commit: https://reviewboard.mozilla.org/r/158774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158774/

Comment 129

[Chung-Sheng Fu [:cfu]]

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.

Comment 130

[:Gijs (out for now; he/him)]

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

Comment 131

[:Gijs (out for now; he/him)]

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.

Comment 132

[:Gijs (out for now; he/him)]

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.

Comment 133

[:Gijs (out for now; he/him)]

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?

Comment 134

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 135

[Chung-Sheng Fu [:cfu]]

(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.

Comment 136

[Chung-Sheng Fu [:cfu]]

(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?

Comment 137

[Chung-Sheng Fu [:cfu]]

(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.

Comment 138

[:Gijs (out for now; he/him)]

(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.

Comment 139

[Chung-Sheng Fu [:cfu]]

(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.

Comment 140

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156768/diff/6-7/

Comment 141

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158774/diff/1-2/

Comment 142

[Chung-Sheng Fu [:cfu]]

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.

Comment 143

[Chung-Sheng Fu [:cfu]]

(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.

Comment 144

[:Gijs (out for now; he/him)]

(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.

Comment 145

[:Gijs (out for now; he/him)]

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?

Comment 146

[:Gijs (out for now; he/him)]

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)

Comment 147

[:Gijs (out for now; he/him)]

(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.

Comment 148

[Chung-Sheng Fu [:cfu]]

(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.

Comment 149

[:Gijs (out for now; he/him)]

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

Comment 150

[Chung-Sheng Fu [:cfu]]

(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.

Comment 151

[:Gijs (out for now; he/him)]

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

Comment 152

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 153

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 863246 - Remove compatibility for add-on

Review commit: https://reviewboard.mozilla.org/r/160102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160102/

Comment 154

[Chung-Sheng Fu [:cfu]]

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.

Comment 155

[Chung-Sheng Fu [:cfu]]

(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?

Comment 156

[:Gijs (out for now; he/him)]

(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.

Comment 157

[:Gijs (out for now; he/him)]

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. :-)

Comment 158

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 159

[Chung-Sheng Fu [:cfu]]

(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?

Comment 160

[Jim Blandy :jimb]

(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.

Comment 161

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Bug-863246-Make-debugger-scripts-content-accessible.patch

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.

Comment 162

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 163

[Boris Zbarsky [:bzbarsky]]

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.

Comment 164

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Bug-863246-Use-system-principal-to-load-debugger-scr.patch

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?

Comment 165

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 166

[Chung-Sheng Fu [:cfu]]

(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?

Comment 167

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 168

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 169

[Boris Zbarsky [:bzbarsky]]

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

Comment 170

[Ben Kelly [:bkelly, not reviewing]]

(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?

Comment 171

[Chung-Sheng Fu [:cfu]]

(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?

Comment 172

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 173

[Chung-Sheng Fu [:cfu]]

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

Comment 174

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 175

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Bug-863246-Use-system-principal-to-load-debugger-scr.patch

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.

Comment 176

[Ben Kelly [:bkelly, not reviewing]]

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);

Comment 177

[Chung-Sheng Fu [:cfu]]

(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?

Comment 178

[Chung-Sheng Fu [:cfu]]

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?

Comment 179

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 180

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/7-8/

Comment 181

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/4-5/

Comment 182

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/13-14/

Comment 183

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158774/diff/2-3/

Comment 184

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/11-12/

Comment 185

[Chung-Sheng Fu [:cfu]]

I had some patches merged.  Please take a look again.

Sorry for taking your time.

Comment 186

[Ben Kelly [:bkelly, not reviewing]]

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

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

Comment 187

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

::: xpcom/components/ManifestParser.cpp:704
(Diff revision 8)
>          bool flag;
>          if (CheckFlag(kContentAccessible, wtoken, flag)) {
>            if (flag)
>              flags |= nsChromeRegistry::CONTENT_ACCESSIBLE;
> +          else
> +            flags |= nsChromeRegistry::CONTENT_INACCESSIBLE;

I don't think we're using CONTENT_INACCESSIBLE anymore, so let's just remove it.

Comment 188

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/8-9/

Comment 189

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/5-6/

Comment 190

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/14-15/

Comment 191

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158774/diff/3-4/

Comment 192

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/12-13/

Comment 193

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/9-10/

Comment 194

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/6-7/

Comment 195

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/15-16/

Comment 196

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158774/diff/4-5/

Comment 197

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/13-14/

Comment 199

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05fc8d2d7ca9
Content can only load resource:// URIs declared content-accessible in manifests r=billm,bz
https://hg.mozilla.org/integration/autoland/rev/1bfbae351dd7
Move resources that need to be exposed to web content to locations that are marked as contentaccessible r=billm
https://hg.mozilla.org/integration/autoland/rev/aba9324719be
Use system principal to load debugger scripts r=billm,bkelly
https://hg.mozilla.org/integration/autoland/rev/a7b0c6ddd812
Fix test failures r=Gijs
https://hg.mozilla.org/integration/autoland/rev/88eb4aebb63c
Add test cases r=billm

Comment 200

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing various reftests, e.g. parser/htmlparser/tests/reftest/bug535530-2.html:

https://hg.mozilla.org/integration/autoland/rev/4c7a7a93b2c35be2b42ba1f11e7cfb1f9603fe6b
https://hg.mozilla.org/integration/autoland/rev/ad0723ab1eb2c1407eeee924719220adb20433b2
https://hg.mozilla.org/integration/autoland/rev/149b351b2f16794f3a54d76645b0f6367531ae58
https://hg.mozilla.org/integration/autoland/rev/c3d76aac538550ba25ebd96fa4e8f8dda4d47abb
https://hg.mozilla.org/integration/autoland/rev/d6cf1a8cde2d0aa9d64d849c6c250879185b5ba0

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ed482ad5d31f987fa1ac8bab5989a5a54b2d2378&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=125891883&repo=autoland

[task 2017-08-25T12:52:53.854106Z] 12:52:53     INFO -  /home/worker/workspace/build/src/mobile/android/app/src/photon/res/drawable-hdpi/tab_indicator_selected.9.png: libpng warning: iCCP: Not recognizing known sRGB profile that has been edited
[task 2017-08-25T12:52:53.854485Z] 12:52:53     INFO -  /home/worker/workspace/build/src/mobile/android/app/src/photon/res/drawable-hdpi/tab_indicator_selected_focused.9.png: libpng warning: iCCP: Not recognizing known sRGB profile that has been edited
[task 2017-08-25T12:52:53.854838Z] 12:52:53     INFO -  /home/worker/workspace/build/src/mobile/android/app/src/photon/res/layout/toolbar_edit_layout.xml:25: error: Error parsing XML: unbound prefix
[task 2017-08-25T12:52:53.855154Z] 12:52:53     INFO -  tive.png, for flavor xxxhdpi-v4
[task 2017-08-25T12:52:53.855478Z] 12:52:53     INFO -  nothing matches overlay file url_bar_nav_button.xml, for flavor large-v11
[task 2017-08-25T12:52:53.855798Z] 12:52:53     INFO -  Makefile:487: recipe for target '.aapt.deps' failed
[task 2017-08-25T12:52:53.856110Z] 12:52:53     INFO -  gmake[5]: *** [.aapt.deps] Error 1

Comment 201

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, wrong push and log in the previous comment. Correct one:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=88eb4aebb63c84131f113fc4790de5f49dde5975&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=125906795&repo=autoland

REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug535530-2.html == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug535530-2-ref.html | image comparison, max difference: 255, number of differing pixels: 7744
REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug482921-1.html == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug482921-1-ref.html | image comparison, max difference: 255, number of differing pixels: 18383
REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug482921-2.xhtml == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug482921-2-ref.html | image comparison, max difference: 255, number of differing pixels: 20347
REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug704667-1.html == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug704667-1-ref.html | image comparison, max difference: 255, number of differing pixels: 424
REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug731234-1.html == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug731234-1-ref.html | image comparison, max difference: 255, number of differing pixels: 36128
REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug910588-1.html == file:///C:/slave/test/build/tests/reftest/tests/parser/htmlparser/tests/reftest/bug910588-1-ref.html | image comparison, max difference: 255, number of differing pixels: 2679

Comment 202

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147956/diff/10-11/

Comment 203

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156770/diff/7-8/

Comment 204

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138314/diff/16-17/

Comment 205

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158774/diff/5-6/

Comment 206

[Chung-Sheng Fu [:cfu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147960/diff/14-15/

Comment 207

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a676a1188572
Content can only load resource:// URIs declared content-accessible in manifests r=billm,bz
https://hg.mozilla.org/integration/autoland/rev/ac41228312b8
Move resources that need to be exposed to web content to locations that are marked as contentaccessible r=billm
https://hg.mozilla.org/integration/autoland/rev/b9baf84bb1db
Use system principal to load debugger scripts r=billm,bkelly
https://hg.mozilla.org/integration/autoland/rev/27ca9f2f8b61
Fix test failures r=Gijs
https://hg.mozilla.org/integration/autoland/rev/bd9c5474a5e4
Add test cases r=billm

Comment 209

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a676a1188572
https://hg.mozilla.org/mozilla-central/rev/ac41228312b8
https://hg.mozilla.org/mozilla-central/rev/b9baf84bb1db
https://hg.mozilla.org/mozilla-central/rev/27ca9f2f8b61
https://hg.mozilla.org/mozilla-central/rev/bd9c5474a5e4

Comment 210

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f86f48fda7bd
Port bug 863246 to TB/IB/SM: Move resources that need to be exposed to web content to locations that are marked as contentaccessible. r=jorgk

Comment 211

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/720e1638522cc348e4105f2b71b409aa5160648f
chore(build): Backport bug 863246 set contentaccessible=yes

https://github.com/mozilla/activity-stream/commit/dbdfa48f077593ea696e88784c18d44139b3946a
Merge pull request #3276 from Mardak/gh3275-jar

chore(build): Backport bug 863246 set contentaccessible=yes

Comment 212

[Tom S [:evilpie]]

Thank you Chung-Sheng Fu for driving this bug to completion!

Comment 213

[Chung-Sheng Fu [:cfu]]

(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #212)
> Thank you Chung-Sheng Fu for driving this bug to completion!

Thank you for your great start and all of your support :)

Comment 214

[Ethan Tseng [:ethan]]

This is a significant progress!
Thank you, CS, and all the reviewers!

Comment 215

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed (tested on https://browserleaks.com/firefox ), starting in Mozilla Firefox Nightly 57.0a1 (2017-08-29), so I'm marking this bug as VERIFIED.
Now, there is no need to use "No Resource URI Leak" extension (FYI-it stopped working in Firefox 55) as workaround .

Thank you very much! \o/

Comment 217

[Ethan Tseng [:ethan]]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #215)
> I'm confirming that bug is fixed (tested on https://browserleaks.com/firefox
> ), starting in Mozilla Firefox Nightly 57.0a1 (2017-08-29), so I'm marking
> this bug as VERIFIED.
> Now, there is no need to use "No Resource URI Leak" extension (FYI-it
> stopped working in Firefox 55) as workaround .
> Thank you very much! \o/

Great! Thanks for the verification!

Comment 218

[[github robot]]

Commits pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/a8e83624c9d285910d8a9f3dc17996a9aed05d7f
Bug 863246 - Declare content and vendor resources content accessible.

Fixes #1015

https://github.com/mozilla/normandy/commit/a1454c2c1cbef62b19a78535530cdf30e76cdb77
Merge pull request #1016 from mythmon/backport-863246

Bug 863246 - Declare content and vendor resources content accesible.

Comment 219

[Chris Mills [:cmills]]

I am not sure what to document here, so for now I've just added a note to the Fx57 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/57#Security

If there is more to do here, I'd be happy to document some more.

Comment 220

[Ethan Tseng [:ethan]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #219)
> I am not sure what to document here, so for now I've just added a note to
> the Fx57 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/57#Security

Thanks, Chris!
Adding this to the release notes for developers is great.

> If there is more to do here, I'd be happy to document some more.

Maybe we could add a single page for resource: URI scheme, similar to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs.

We had wrote this document to explain the change of resource: URI scheme.
https://docs.google.com/a/mozilla.com/document/d/1bUh0RP3O3q67_O0BaXtAr0FcfRExM39sdLZ3pGve294/edit?usp=sharing
It was also sent to dev-platform with the title "Changed the loading behavior of resource:// URI since Nightly 57".

What do you think, Chris?

Comment 221

[Chris Mills [:cmills]]

(In reply to Ethan Tseng [:ethan] - 57 Regression Engineering Support from comment #220)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #219)
> > I am not sure what to document here, so for now I've just added a note to
> > the Fx57 rel notes:
> > https://developer.mozilla.org/en-US/Firefox/Releases/57#Security
> 
> Thanks, Chris!
> Adding this to the release notes for developers is great.
> 
> > If there is more to do here, I'd be happy to document some more.
> 
> Maybe we could add a single page for resource: URI scheme, similar to
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs.
> 
> We had wrote this document to explain the change of resource: URI scheme.
> https://docs.google.com/a/mozilla.com/document/d/
> 1bUh0RP3O3q67_O0BaXtAr0FcfRExM39sdLZ3pGve294/edit?usp=sharing
> It was also sent to dev-platform with the title "Changed the loading
> behavior of resource:// URI since Nightly 57".
> 
> What do you think, Chris?

Sorry for being so slow to respond on this. I have started drafting a document to publish on MDN, but I need some input from you to make sure the content is complete:

https://docs.google.com/document/d/1rDNrzqAUDZ7WghSwENhjP5BrDjDx3NEUElU0Is6ytYo/edit#heading=h.15t6ciqwqy27

I've left some comments for you to answer, then you've got a spare moment ;-)

Comment 222

[Chris Mills [:cmills]]

OK, I've published a page covering this:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Resource_URLs

Let me know if you think this looks ok. Thanks!

Comment 223

[Ethan Tseng [:ethan]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #222)
> OK, I've published a page covering this:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/
> Resource_URLs
> Let me know if you think this looks ok. Thanks!

It is great!
Thank you, Chris and CS!