1256122 - webRequest.onHeadersReceived no redirect to extension page

Status: RESOLVED DUPLICATE of bug 1380186

(WebExtensions :: Request Handling, defect, P2)

Description

[testit]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160303134406

Steps to reproduce:

My extension registers as a listener to webRequest.onHeadersReceived and tries to redirect to the page "testit.html" which is included in my extension. "testit.html" is listed in the "web_accessible_resources" section in the "manifest.json".

browser.webRequest.onHeadersReceived.addListener
(
   function(data)
   {
      return {redirectUrl: browser.extension.getURL('testit.html')};
   },
   {
      urls: ["<all_urls>"],
      types: ["main_frame", "sub_frame"]
   },
   ["responseHeaders", "blocking"]
);



Actual results:

Redirecting does not work. In the browser console the following error is shown:

moz-extension://a22cf3ea-6db2-4b8c-83d2-f4c344c4461b/testit.html
Security Error: Content at http://<url> may not load or link to file:///C:/extension/testit.html.


Expected results:

The requested url should be redirected to the extension page "testit.html". I've tested it with Chrome where it worked without problems.

Comment 1

[Giorgio Maone [:ma1]]

Talked with Kris, we want to perform our own security checks on extension-triggered redirections, rather than rely on the platform. As far as I can tell, redirectTo() is meant to be called by privileged (add-on or browser) code only, so the best course of action seems be removing security checks from the native redirectTo() code. We'll perform them in our own WebExtension handler of redirectURL.
Honza: does it seems reasonable to you? Do you want to take on the native part (we'll open a spin-off bug for the WebExtensions-side security checks)? Thanks.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Giorgio Maone [:mao] from comment #1)
> Talked with Kris, we want to perform our own security checks on
> extension-triggered redirections, rather than rely on the platform. As far
> as I can tell, redirectTo() is meant to be called by privileged (add-on or
> browser) code only, so the best course of action seems be removing security
> checks from the native redirectTo() code. We'll perform them in our own
> WebExtension handler of redirectURL.
> Honza: does it seems reasonable to you? 

Not really regarding what necko does as part of the redirectTo() job now, no.  

I'm probably not the best person to ask about your needs.  I'm not part of the workers effort nor particularly involved in CSP.

Let's try what :jdm thinks, maybe he can lead us to the right people to figure this out.  I believe the changes should not happen in necko but in our redirect listeners that vetoes your redirect here.

Comment 3

[Josh Matthews [:jdm]]

Sorry, I don't think this is my ballpark either. This might be for sicking or ckerschb, since the code in question triggering the error lives in nsScriptSecurityManager::CheckLoadURIWithPrincipal.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

I suppose the code in comment 0 never worked within Firefox, or is that a regression? I don't think that asyncOpen2() or the newly added SecurityManager is responsible for that. Presumably we are lacking APIs for contentPolicies to handle redirects gracefully.

If you can post some more detailed STR I can take a closer look of course.

Comment 5

[Giorgio Maone [:ma1]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> I suppose the code in comment 0 never worked within Firefox

You're correct. But we need it to because redirectTo() is used only internally (not by content), and its main client in the future will be the WebExtensions API, whose minimum target is Chrome compatibility. The ability for a WebExtension to redirect a HTTP request to a file:// destination is a Chrome parity feature.

> If you can post some more detailed STR I can take a closer look of course.

1. Execute following snippet in a browser-privileged scratchpad:

<SNIPPET>
{
  try {
    Services.obs.removeObserver(window._testHttpObserver, "http-on-modify-request");
  } catch (e) {}
  let fileURL = Services.io.newURI("file://", null, null).nsIFileURL;
  fileURL.file = Services.dirsvc.get("Home", Ci.nsIFile);
  let destinations = {File: fileURL.spec, Http: "https://noscript.net/"};
  
  window._testHttpObserver = {

    observe(subject) {
      let channel = subject.QueryInterface(Ci.nsIHttpChannel);
      let m = channel.name.match(/\bredirectTo(File|Http)\b/);
      if (m) {
        channel.redirectTo(Services.io.newURI(destinations[m[1]], null, null));
      }
    }
  };
  Services.obs.addObserver(_testHttpObserver, "http-on-modify-request", false);
  for (let dest in destinations) {
    let url = `https://mozilla.org/?redirectTo${dest}`;
    fetch(url)
      .then(r => { console.log(`OK: ${url} => ${r.url}`)})
      .catch(e => { console.error(`${url} ${e}`)});
  }
  fetch(destinations.File).then(r => { console.log(`OK: direct ${r.url}`)})
      .catch(e => { console.error(`direct file ${e}`)});
}
</SNIPPET>

2. Watch the browser console (ctrl+shift+J)

Expected result (messages in the Browser console):
"OK: direct file:///..."
"OK: https://mozilla.org/?redirectToHttp => https://noscript.net/"
"OK: https://mozilla.org/?redirectToFile => file:///..."

Actual result:
"OK: direct file:///..."
"OK: https://mozilla.org/?redirectToHttp => https://noscript.net/"
Security Error: Content at https://mozilla.org/?redirectToFile may not load or link to file:///.
"https://mozilla.org/?redirectToFile TypeError: NetworkError when attempting to fetch resource."

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

I don't think that skipping Security checks within nsContentSecurityManager is the right path forward here. I ran the example from Comment 5 and I can see the error message in the console, but I don't fully understand what the question is. Are we missing any APIs so you can do additional security checks once the channel gets redirected? Or are you asking we should skip security checks in case of such web extensions? How would we be able to identify such web extensions? All I see at the moment within the contentSecurityManager is that we are loading a resource using TYPE_FETCH.

Jonas, maybe you have a better understanding of what the question is here. Can you help me out?

Comment 7

[Giorgio Maone [:ma1]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> I don't think that skipping Security checks within nsContentSecurityManager
> is the right path forward here. I ran the example from Comment 5 and I can
> see the error message in the console, but I don't fully understand what the
> question is. Are we missing any APIs so you can do additional security
> checks once the channel gets redirected? Or are you asking we should skip
> security checks in case of such web extensions? How would we be able to
> identify such web extensions? All I see at the moment within the
> contentSecurityManager is that we are loading a resource using TYPE_FETCH.
> 

As I said, redirectTo() is used only by add-ons and not exposed to content:
https://dxr.mozilla.org/mozilla-central/search?q=redirectTo%28&redirect=false&case=false

Since Chrome parity is a goal for WebExtensions (and it seems some existing Firefox extensions also need this feature to be ported), we need redirectTo() to be allowed to redirect a HTTP request to a file:/// URL.

I understand from your comment there's no easy way at this moment to tell a redirectTo() from a "normal" redirection, I think mainly because it passes nsIChannelEventSink::REDIRECT_TEMPORARY rather than nsIChannelEventSink::REDIRECT_INTERNAL ( https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5893 ).

We could add this contextual info either by passing a different flag (a new one, if REDIRECT_INTERNAL is still too ambiguous) from nsIHttpChannel::redirectTo(), or if we want to limit this feature to opt-in add-ons and the WebExtensions API, reserve a new loadFlags value to be set on the "old" channel before the redirectTo() call.

What do you think?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The basic problem here is that we just don't have the right infrastructure to internally deal with this type of redirect.

So we need to change our internals to support it somehow.

One question is what the redirect looks like to the world outside of the extension. For example, if a webpage contains

<iframe src="somepage.html">

and an extension uses redirectTo() to redirect that load to an extension-provided page 'extensionPage.html', what should the security properties be of the resulting page?

Is it considered same-origin with the webpage outside of the <iframe>? If the outer page does iframeElement.contentWindow.document, does it get access to the document? I.e. does it look to the website like somepage.html was loaded, or does it look like there was a redirect to some cross-origin page that it can't reach into?

What does document.location.href return? What does document.baseURI return?

If the thing being loaded is not an iframe, but an <img>, can that image be drawn into a canvas without the canvas getting tainted. I.e. is the image considered same-origin, or is it considered redirected to some cross-origin location?

Comment 9

[Giorgio Maone [:ma1]]

(In reply to Jonas Sicking (:sicking) from comment #8)
> The basic problem here is that we just don't have the right infrastructure
> to internally deal with this type of redirect.

Could you please elaborate a bit? Beside CheckLoadURI(), in what redirecting to a file:// URI would be different than redirecting to a cross-domain HTTP resource or a different, supported scheme, such as ftp://?

> So we need to change our internals to support it somehow.

Sure, we need a way to tell redirectTo() redirection from "organic" ones, and logic to skip some security checks when this is the case.

> One question is what the redirect looks like to the world outside of the
> extension. For example, if a webpage contains
> 
> <iframe src="somepage.html">
> 
> and an extension uses redirectTo() to redirect that load to an
> extension-provided page 'extensionPage.html', what should the security
> properties be of the resulting page?
> 

No matter what Chrome does, I'd always threat it as cross-domain. Messing things up and exposing local resources to remote web pages is just too easy to be allowed. And if an extension really needs to produce content on the fly which acts as same-site with a remote parent, it could always build a data: URI, couldn't it?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Giorgio Maone [:mao] from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #8)
> > The basic problem here is that we just don't have the right infrastructure
> > to internally deal with this type of redirect.
> 
> Could you please elaborate a bit? Beside CheckLoadURI(), in what redirecting
> to a file:// URI would be different than redirecting to a cross-domain HTTP
> resource or a different, supported scheme, such as ftp://?

It's not clear that "treat it as if it's a http redirect" is what we want to do.

For a few APIs we for example have a same-origin restriction. Which means that a lot of http URLs are also blocked.

In other cases we have CORS restrictions, which require that the http URL respond with certain headers.

> > So we need to change our internals to support it somehow.
> 
> Sure, we need a way to tell redirectTo() redirection from "organic" ones,
> and logic to skip some security checks when this is the case.

There's lots of security checks involved in the web platform. Not just during the network traffic, but also once the resulting resources have been parsed and/or executed.

To figure out how to implement the behavior that we want, we need to first figure out what behavior we want.

> > One question is what the redirect looks like to the world outside of the
> > extension. For example, if a webpage contains
> > 
> > <iframe src="somepage.html">
> > 
> > and an extension uses redirectTo() to redirect that load to an
> > extension-provided page 'extensionPage.html', what should the security
> > properties be of the resulting page?
> > 
> 
> No matter what Chrome does, I'd always threat it as cross-domain. Messing
> things up and exposing local resources to remote web pages is just too easy
> to be allowed. And if an extension really needs to produce content on the
> fly which acts as same-site with a remote parent, it could always build a
> data: URI, couldn't it?

It seems like there's a big risk involved in deviating from the Chrome behavior. Are we doing that elsewhere? Are we reaching out to developers and trying to get them to be compatible with Firefox?

This is clearly not my call though. But I'd also like to make sure that we make an intentional decision about what we want our behavior to be as to minimize risk of having to reimplement.

If you are certain that we want to have a particular behavior, then I'm happy to point you to how to implement that.

Comment 12

[Synzvato]

> It seems like there's a big risk involved in deviating from the Chrome behavior.
> Are we doing that elsewhere? Are we reaching out to developers and trying to get
> them to be compatible with Firefox?

I, for one, am a developer that would greatly benefit from complete feature parity here.

> And if an extension really needs to produce content on the fly which acts as same-
> site with a remote parent, it could always build a data: URI, couldn't it?

I believe this would have a severe impact extensions that rely on redirecting page requests "on before send". Not only will this requirement make these add-ons hard to port, but refusing to loosen aspects such as Content Security Policies (for feature parity with Chrome) will continue to cause problems.

From experience (and by design), it's pretty much impossible for an add-on to detect CSP errors. More and more websites are starting to use strict security policies that disallow script loads using the "data:" scheme. I vote for workable behavior, especially since the goal is feature parity.

So I say verified add-ons with granted elevated host privileges should be able to inject bundled sources into specified pages (through match patterns), and the host's CSP should not interfere.

Comment 13

[Giorgio Maone [:ma1]]

(In reply to Synzvato from comment #12)

> I, for one, am a developer that would greatly benefit from complete feature
> parity here.
> [...]
> I vote for workable behavior, especially since the goal is feature parity.
> [...]

This sounds like you've got experience with these specific Chrome behaviors. Could you please document them with test cases, since as far as I can tell the official documentation doesn't provide any detail about these security-sensitive edge cases we're trying to implement?

> So I say verified add-ons with granted elevated host privileges should be
> able to inject bundled sources into specified pages (through match
> patterns), and the host's CSP should not interfere.

This is probably the place where we want to go, at least looking at Mozilla's interpretation of https://www.w3.org/TR/html-design-principles/#priority-of-constituencies in the W3C Web Application Security Workgroup regarding CSP and extensions or even bookmarklets.

Comment 14

[Harsha A]

We are facing similar issues with redirect not working when our webpage is trying to call into our extension's web-accessible-resource.

Comment 15

[Matt Powers]

What is status and any sense of urgency to get this fix incorporated into FF48 release?

Comment 16

[Andy McKay]

(In reply to Matt Powers from comment #15)
> What is status and any sense of urgency to get this fix incorporated into
> FF48 release?

Given that no patches exist, the complexity of this bug and the fact that 48 is already in beta - it is unlikely to land in 48.

Comment 17

[Giorgio Maone [:ma1]]

(In reply to Synzvato from comment #12)

> So I say verified add-ons with granted elevated host privileges should be
> able to inject bundled sources into specified pages (through match
> patterns), and the host's CSP should not interfere.

Could you please specify as formally as possible whatever you know about Chrome's behavior in the context of this bug, even better if with tests we can use as a guideline?
Thank you!

Comment 18

[testit]

(In reply to Giorgio Maone [:mao] from comment #17)
> Could you please specify as formally as possible whatever you know about
> Chrome's behavior in the context of this bug, even better if with tests we
> can use as a guideline?
> Thank you!

Like I wrote in the user story, Chrome just redirects to the given page which is part of the extension. The only prerequisite is, that this page is listed in the "web_accessible_resources" section in the "manifest.json".

Comment 19

[Nerijus Ramanauskas]

Hey guys, any progress on this yet? Affects "onBeforeRequest" too.

Comment 20

[Andy McKay]

Bumping up priority as it sounds like a few people are asking for this and I noticed it was unprioritized.

Comment 21

[Synzvato]

(In reply to Giorgio Maone [:mao] from comment #17)
> Could you please specify as formally as possible whatever you know about
> Chrome's behavior in the context of this bug, even better if with tests we
> can use as a guideline?
> Thank you!
Sorry for the slow reply. Here are some details on Chrome's behaviour:

Performed Steps:
  - Created a test add-on and registered some local scripts in the manifest.
  - Visited pages with strict Content Security Policies and attempted to
    redirect page requests to these registered, local, scripts.

Observations:
  - The local scripts ran just fine (and no CSP errors were thrown).
  - The response status code was "307 - Internal Redirect".

I hope this is somewhat helpful. As soon as I get the chance, I will try to
return to this topic with additional details.

Comment 22

[Matt Powers]

Any fix plan/target FF release forecasts to share?  We are keen to have this issue fixed so we can do more straightforward API redirection handling directly by our extension.

Comment 23

[kazvorpal]

I am a professional web developer contracted to code extensions for a Fortune 100 company. I am currently trying to port to Firefox an extension I had developed for Chrome and successfully ported to Safari.

We had discarded the idea of writing it for — or porting it to — Firefox when faced with the development burden of normal Firefox extensions. But with the release of Webextensions, had decided to revisit the effort.

The extension's primary task is to warn the user when he tries to navigate to a website whose host name is on one of the widely-used "bad site" lists for malware or piracy. 

This involves setting up an onBeforeNavigate event watcher, comparing an MD5-encoded version of the hostname to an internally stored hashlist of bad sites, and if there's a match redirecting the browser to an internal HTML file alerting him to the match, and allowing him to either go on (ergo adding it to a whitelist) or back. 

Instead, Firefox is canceling the action with apparent feedback at all. The internal page does not get displayed, but there is no apparent error in the (awkward and buggy) console/debugger. At least none that I've noticed. As I mentioned, the Firefox debugging console much more difficult to use than in Chrome or Safari, and very inconsistent in its behavior: Sometimes Background stops posting console.log messages, while other times those messages are posted both there and in the main browser console, instead of just one or the other as intended.

I am guessing that the lack of redirect is caused by this bug, 1256122. 

I am able to instead redirect the browser to a different website, say http://www.myclient.com/file.html, just not to anything like chrome.extension.getURL("file.html").

As it is, we're discussing whether to put the port on hold, or to implement the obvious workaround of replacing the internal alert page with one hosted on the client's web server. 

Ironically, this workaround is far less secure for all involved, as well as slower for the user and burdensome for the client's web server, as they have millions of customers who might use this tool...in the unlikely event that all chose to install it. 

For the 99.99% of extensions developed and listed on the Firefox store that are not nefarious, an internal file is completely safe, while being forced to direct the user to an external website is clearly less so.

This paradoxical situation reminds me of security "experts" who talk companies into forcing employees to frequently change passwords, and set up a relatively insecure, awkward set of "one upper, one lower, one special character, one number" rules that are impossible to easily memorize. This either ends up costing the company more in helpdesk calls than is saved in already-rare security breaches, or more likely results in everyone's password being stored on a post-it note on their monitor, so that social hacking becomes a breeze.

Comment 24

[Giorgio Maone [:ma1]]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #10)
> If you are certain that we want to have a particular behavior, then I'm
> happy to point you to how to implement that.

It seems all we want is internal redirections (nsIHttpChannel.redirectTo()) to work for moz-extensions: URIs which are listed as web-accessible-resources.
I suppose fixing bug 1208756 should have removed most if not all of the obstacles to that. 
Actually I wonder why it didn't fix this issue outright. 
:bholley, :bz, any insight? Thank you!

Comment 25

[Boris Zbarsky [:bzbarsky]]

I have no idea.  Have you tried stepping through the code?

That said, note that the error message from comment 0 is about being unable to link to "file:///C:/extension/testit.html", whereas bug 1208756 was about URLs of the form "moz-extension:....".  What is the actual URL being redirected to here?

Comment 26

[testit]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #25)
> I have no idea.  Have you tried stepping through the code?
> 
> That said, note that the error message from comment 0 is about being unable
> to link to "file:///C:/extension/testit.html", whereas bug 1208756 was about
> URLs of the form "moz-extension:....".  What is the actual URL being
> redirected to here?

The URL is moz-extension://a22cf3ea-6db2-4b8c-83d2-f4c344c4461b/testit.html.

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK, so where does that file:// URL come from, exactly?  That seems like the relevant bug to me...

Comment 28

[Tom Zöhner]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #27)
> OK, so where does that file:// URL come from, exactly?  That seems like the
> relevant bug to me...

It seems this is the case, because during development all files are loaded directly from the file system. So the extension redirects correctly to the 'moz-extension://...' url, but internally this is resolved to a 'file://...' url, pointing to the actual file on the file system.

The resulting error message is:
"Security Error: Content at <original site url> may not load or link to file://<path to file on my file system>"

Comment 29

[Boris Zbarsky [:bzbarsky]]

> but internally this is resolved to a 'file://...' url

Well, whatever code is doing _that_ is obviously not doing it right, because those have different security properties!

Comment 30

[Synzvato]

I think that the problem is indeed the fact, that "moz-extension://" requests
are internally resolved to "file://" URIs. I have built a tiny prototype that
works on Opera and Chrome ("chrome-extension://"), but fails on Firefox.

In my "manifest.json", I have marked a specific resource as "web accessible":
> "web_accessible_resources": ["data/example.js"]

The prototype redirects particular requests to the "web accessible" resource:
> 'redirectUrl': chrome.extension.getURL('data/example.js')

Opera and Chrome truly make the resource web accessible. This means, that our
"chrome-extension://" URI can be used as a web page's script source. Firefox,
however does not load "moz-extension://" URIs as page sources, and says:

> XML Parsing Error: not well-formed
> Location: file:///home/user/add-on/data/example.js
> Line Number 1, Column 1

This (first) problem likely is that, as far as I know, "file://" URIs (unlike
e.g. "data://" URIs) cannot be a redirection target of an XMLHttpRequest. The
way the other browsers handle requests for web accessible resources, is quite
sophisticated. Internal redirects (307), and proper HTTP compatiblity.

On top of that, these browsers ignore Content Security Policies when requests
are redirected to web accessible extension resources. From what I can see, it
seems that Firefox places a website's CSPs above custom extension logic.

I personally need absolute feature parity here. Otherwise, it will simply not
be possible for me to make my WebExtensions port Firefox-compatible. So, this
is currently a blocking issue for me. I hope this helps!

Comment 31

[Boris Zbarsky [:bzbarsky]]

> that "moz-extension://" requests are internally resolved to "file://" URIs.

Well, more precisely _how_ that's done, and the fact that the file://-ness is leaking out of the moz-extension internals.  It shouldn't be!

> Firefox, however does not load "moz-extension://" URIs as page sources

The error message you quote sounds like you're doing an XHR load, not a <script> load, and it's actually _succeeding_.  The reporter location is a bit confusing, but the XHR load did in fact happen; otherwise we wouldn't have even tried parsing as XML.

I would really like us to avoid getting sidetracked in this bug; please file a separate bug on the CSP issue and cc me on it.  Please include detailed steps to reproduce (which extension to install, what to do after it's installed, etc).  Same for any other issue that's not actually the redirectTo with moz-extension target issue this bug is originally filed on.  It's possible that a single fix would fix all of them, but it's not obvious to me that it would.

I cannot emphasize enough the need for clear steps to reproduce, which this bug is also missing.  If I had those, I could just reproduce the problem and see what's going on instead of asking questions trying to get some information from _someone_ about what people are actually doing/seeing... which is a waste of both your time and mine.

Comment 32

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #31)
> > that "moz-extension://" requests are internally resolved to "file://" URIs.
>
> Well, more precisely _how_ that's done, and the fact that the file://-ness
> is leaking out of the moz-extension internals.  It shouldn't be!

I was surprised to hear that too, when I heard about the apparent solution for bug 1276048, but I haven't looked into it until now.

Apparently the issue is that SubstitutingProtocolHandler, which resource: and moz-extension: protocols are built on, creates file or JAR channels for the underlying URI directly, and those channels don't know about their original URIs. Since the security checks that happen from AsyncOnChannelRedirect handlers use the channel's URI to do security checks, that causes a failure.

The solution (for JAR channels, anyway) in bug 1276048 is to make the underlying channel aware of its original URL. Which should also fix this problem. The other option that I can think of would be to just unconditionally create a wrapper channel, which would also solve this for file: URLs (and seems a bit less hacky).

> > Firefox, however does not load "moz-extension://" URIs as page sources
>
> I would really like us to avoid getting sidetracked in this bug; please file
> a separate bug on the CSP issue and cc me on it.  Please include detailed
> steps to reproduce (which extension to install, what to do after it's
> installed, etc).

I agree with not side-tracking this but, but I'll add that moz-extension: resources are supposed to be immune to CSP, and we have tests to ensure that they are. So please provide as much detail as possible about where and how you're seeing it fail.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> and those channels don't know about their original URIs.

That is... simply not true.  SubstitutingProtocolHandler::NewChannel2 calls SetOriginalURI right there on the channel, plain as day, with the original URI.

If you override SubstituteChannel() and substitute another channel, then that's irrelevant, but the only thing that does that is moz-extension for localizable CSS, and that uses the original URI for the channel it substitutes anyway.

Note that one reason I needinfo'd you on this bug is that it sounded to me based on previous comments like you knew how to actually reproduce it, which is one of the things I would really like to be actually written down in the bug.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> SubstitutingProtocolHandler::NewChannel2 calls SetOriginalURI 

I guess in the specific case of moz-extension as a redirect target this would not be good enough because redirects would clobber the originalURI on the channel with the pre-redirect URI and moreover set it to the "ignore original URI" mode, because they clobbered it...

I wonder whether we can just change the semantics of originalURI so redirects do NOT mess with it on the post-redirect channels.  Does anyone actually rely on the current semantics?  I think people who keep track of something keyed on the channel URI also implement redirect listeners instead of consistently using originalURI as the key.

If we can't change/fix how originalURI works, we should add something to all channels that acts like originalURI but does NOT get clobbered on redirects and is always the canonical URI for all security checks and the like.  I suppose that's possible now with a wrapper channel, as you note, but that seems like an annoying overhead in general.

Comment 35

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33)
> > and those channels don't know about their original URIs.
> 
> That is... simply not true.  SubstitutingProtocolHandler::NewChannel2 calls
> SetOriginalURI right there on the channel, plain as day, with the original
> URI.

Sorry, rather, they don't know that their original URIs are meant to be treated as transparent substitutions. The redirect security checks are checking permissions for the original URI to load the current/final URI. Which means in this case, they're probably actually checking that the moz-extension: URL has permission to load the underlying file: URL (though I haven't confirmed this).

> If you override SubstituteChannel() and substitute another channel, then
> that's irrelevant, but the only thing that does that is moz-extension for
> localizable CSS, and that uses the original URI for the channel it
> substitutes anyway.
> 
> Note that one reason I needinfo'd you on this bug is that it sounded to me
> based on previous comments like you knew how to actually reproduce it, which
> is one of the things I would really like to be actually written down in the
> bug.

I don't have time to create a working testcase right now, but it would look something like this:

  <script>
    browser.webRequest.onBeforeRequest.addListener(
      details => {
        return {redirect: "moz-extension://.../foo.html"};
      },
      {urls: ["http://foo.com/"]},
      ["blocking"]);
  </script>

  <iframe src="http://foo.com/"></iframe>

where the iframe should be redirected to load a page from the extension instead.

Comment 36

[Kris Maglione [:kmag]]

In particular, I'm referring to checks like this one:

http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/netwerk/protocol/http/nsCORSListenerProxy.cpp#877

Comment 37

[Boris Zbarsky [:bzbarsky]]

> The redirect security checks are checking permissions for the original URI to load the current/final URI.

"current" and "final" URI are not the same thing for channels.  If the redirect checks are checking the permissions to load the return value of GetURI(), then I claim they are broken.  They should be checking the "final channel URI", which takes originalURI into account.  See NS_GetFinalChannelURI.

> but it would look something like this

So when I say "steps to reproduce" I mean "an actual extension I can install and steps to follow after that".  Assume I know a bunch about our network and security code, but nothing about webextensions (all of which is true!).  I can understand if you don't have time to create a testcase right now, nor am I asking for that; right now it's Saturday evening or Sunday morning!

Comment 38

[Boris Zbarsky [:bzbarsky]]

> In particular, I'm referring to checks like this one:

That's using NS_GetFinalChannelURI, which takes the originalURI into account.  Again, in the redirect target case things could be going awry depending on the exact timing of the UpdateChannel call and redirects clobbering the originalURI.  UpdateChannel is called from AsyncOnChannelRedirect and the HTTP code is careful to not clobber the originalURI until _after_ redirect listeners have been notified (e.g. see comments in HttpChannelChild::OnRedirectVerifyCallback and the various nsHttpChannel methods that call SetOriginalURI()).  The entire point of that ordering is that things like UpdateChannel should see a sane NS_GetFinalChannelURI on the channel they're getting.

Comment 39

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #37)
> > The redirect security checks are checking permissions for the original URI to load the current/final URI.
>
> "current" and "final" URI are not the same thing for channels.  If the
> redirect checks are checking the permissions to load the return value of
> GetURI(), then I claim they are broken.  They should be checking the "final
> channel URI", which takes originalURI into account.  See
> NS_GetFinalChannelURI.

They are checking the final channel URL, or at least trying to:

http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/netwerk/protocol/http/nsCORSListenerProxy.cpp#877
  nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));

But at this point the LOAD_REPLACE flag has already been set.

Oddly, it looks like the redirect code is already setup to handle this:

http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/netwerk/protocol/http/HttpChannelChild.cpp#1617-1618
    // Must not be called until after redirect observers called.
    newHttpChannel->SetOriginalURI(mOriginalURI);

http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/netwerk/base/nsBaseChannel.cpp#166-167
  // Make sure to do this _after_ making all the OnChannelRedirect calls
  mRedirectChannel->SetOriginalURI(OriginalURI());

but it sets the LOAD_REPLACE flag before it calls the observers, so the
original URI is still not used as the final URL.

It seems like maybe we should just store separate original and final URL
properties rather than using originalURL for two separate things...

> So when I say "steps to reproduce" I mean "an actual extension I can install
> and steps to follow after that".  Assume I know a bunch about our network
> and security code, but nothing about webextensions (all of which is true!). 
> I can understand if you don't have time to create a testcase right now, nor
> am I asking for that; right now it's Saturday evening or Sunday morning!

I was making both of those assumptions. :) I'll add a complete testcase when I
have more time.

Comment 40

[Kris Maglione [:kmag]]

I apparently can't post a comment to this thread without getting mid-aired...

Comment 41

[Boris Zbarsky [:bzbarsky]]

> But at this point the LOAD_REPLACE flag has already been set.

Aha!  Well, that's a bug right there, I think...  We should probably be setting it when we set the originalURI, even if we change nothing else.

Comment 42

[testit]

Attachment: Testcase

Comment 43

[testit]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #31)
> I cannot emphasize enough the need for clear steps to reproduce, which this
> bug is also missing.  If I had those, I could just reproduce the problem and
> see what's going on instead of asking questions trying to get some
> information from _someone_ about what people are actually doing/seeing...
> which is a waste of both your time and mine.

I just attached a testcase which can be used in Firefox and Chrome. Unzip the file and load the extension via "about:debugging" in Firefox. Enter any URL. If the redirect works, "Redirect was successful" should be displayed. Currently in Firefox an empty page is displayed and the error message "Security Error: Content at http://<url> may not load or link to file:///<extension location>/testit.html." is shown in the browser console.

Comment 44

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #41)
> > But at this point the LOAD_REPLACE flag has already been set.
> 
> Aha!  Well, that's a bug right there, I think...  We should probably be
> setting it when we set the originalURI, even if we change nothing else.

The NS_GetFinalChannelURI function is something that confuses me a long time.  We always set the LOAD_REPLACE flag on channels from the very moment we create them.  It was like this since introduction of that flag.  Hence, NS_GetFinalChannelURI can return original URL only on a channel that is the first in the redirect chain (where it's equal to the current URL) and then returns current URL for any later channel in the same redirect chain (where it's the target URL).

At least on nshttpchannel (both child/parent) the original URL is equal to current URL at the time of creation of the channel.

I'm not sure what the NS_GetFinalChannelURI helper has to do exactly.  Is searching for a "final channel" and then grab a URL (which one?) from it?  Or is it searching for "final url" (which is what exactly??) on the given channel?

From how I understand it, it should always return original URL on any channel regardless the replace flag.  We update the original URL after we agree on the redirect (after all the redirect observer have been called on).

But that's a bug change and we should make a good audit of all the current consumers.

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #44)
> We always set the LOAD_REPLACE flag on channels from the very moment
> we create them.  

I mean for channel that are handling the redirect target URL load.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> NS_GetFinalChannelURI can return original URL only on a channel that is the first in the redirect chain

That is a correct summary of the current state of things, and I believe it's a broken setup for the way we do security checks during redirects.  It's also somewhat broken for the way we get principals post-redirect, for cases that actually rely on the URI principal.  More on this below.

However, the brokenness can only be detected if we have a protocol handler that satisfies the following two conditions:

1) Sets originalURI on the channels it creates.
2) Can be the target of a redirect.

Condition #1 is satisfied by the about: (via the about redirector), chrome:, file: protocols, plus whatever uses the substituting protocol handler (resource: and moz-extension:, I believe).  chrome:, resource:, and file: can't be used as the target of a redirect.  Some about: URLs can, but no one does that in practice.  If someone _were_ to do it, it would be just as broken: I just tried a test HTTP response that does a 302 to about:rights and it doesn't work.

> I'm not sure what the NS_GetFinalChannelURI helper has to do exactly

It's meant to return "the URI of this channel for security purposes, like determining the origin of the resulting document".  For protocols that effectively do their own "internal" redirect by returning a channel whose URI doesn't match the originalURI, this is the originalURI.  But for the HTTP redirect case it should really be "the originalURI of the post-redirect channel, as of when we created it".  We don't have a way to get this right now (because that state is not stored anywhere), so we just use the URI of the post-redirect channel when LOAD_REPLACE is set.  This worked ok as long as there were no redirects to protocols whose handlers set an interesting originalURI, but no we do in fact want to support such redirects.

For purposes of the security checks during redirect, it would be enough to just set LOAD_REPLACE after we call the redirect observers.  That would cause the redirect observers to see the default (protocol-handler-set) originalURI on the post-redirect channel, which is what we really want to do security checks against.  I had thought this might be enough, but of course it's not, because the extension protocol handler doesn't do anything interesting with loadinfo or its channel owners/principals.  As a result, the results of those loads get a principal based on the "final channel URI", and that means we really do need some sort of setup that will produce the "moz-extension:" URI for the "final channel URI" there.

So what we really need to fix this bug is one of two things, I think:

A) Stop setting originalURI during redirects, audit all originalURI consumers to make sure they're OK with this change.

B) Add some other way of getting the "original" originalURI on the post-redirect channel, ensure it works correctly both while notifying redirect observers and after we get OnStartRequest, and update NS_GetFinalChannelURI to use that.

Does that make sense?

Comment 47

[Boris Zbarsky [:bzbarsky]]

(In reply to testit from comment #43)
> Unzip the file and load the extension via "about:debugging" in Firefox.

Thank you for the testcase and the steps to reproduce.

This testcase actually hits fatal assertions in networking code in a debug build (in non-e10s mode), for extra fun:

  Assertion failure: !(mTransactionPump && mCachePump) || mCachedContentIsPartial (If we have both pumps, the cache content must be partial), at /Users/bzbarsky/mozilla/vanilla/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:6429

or

  Assertion failure: mConcurrentCacheAccess, at /Users/bzbarsky/mozilla/inbound/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:1232


but before we ever get to any of those, we fail the security check for the redirect, even if I move where LOAD_REPLACE is set.  And the reason for that is that nsContentSecurityManager::AsyncOnChannelRedirect does checks on both the new channel's URI _and_ originalURI.  That's extra-safe, I guess, but isn't the semantics we need for working redirects to webextension urls, again unless we create wrapper channels in the webextension protocol handler...

It looks like that code in AsyncOnChannelRedirect just got moved from nsScriptSecurityManager::AsyncOnChannelRedirect and there we used to _just_ check the channel URI, until bug 460425 when we added checking of the originalURI.

If we want this to work with the existing webextension protocol handler setup, we should do a single check, on the "final channel URI".

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #47)
> If we want this to work with the existing webextension protocol handler
> setup, we should do a single check, on the "final channel URI".

Yes, that sounds reasonable, we should replace that whole block within AsyncOnChannelRedirect:

  nsresult rv = nsContentUtils::GetSecurityManager()->
    CheckLoadURIWithPrincipal(oldPrincipal, newURI, flags);
  if (NS_SUCCEEDED(rv) && newOriginalURI != newURI) {
      rv = nsContentUtils::GetSecurityManager()->
        CheckLoadURIWithPrincipal(oldPrincipal, newOriginalURI, flags);
  }

with just performing a CheckLoadURIWithPrincipal check using the finalChannelURI. (Assuming that does not break any testcase in our tree, but I suppose it shouldn't).

Comment 49

[Honza Bambas (:mayhemer)]

I had to read this like three times to catch up :)  answers (and some more questions) inlined.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> > NS_GetFinalChannelURI can return original URL only on a channel that is the first in the redirect chain
> 
> That is a correct summary of the current state of things, and I believe it's
> a broken setup for the way we do security checks during redirects.  It's
> also somewhat broken for the way we get principals post-redirect, for cases
> that actually rely on the URI principal.  More on this below.
> 
> However, the brokenness can only be detected if we have a protocol handler
> that satisfies the following two conditions:
> 
> 1) Sets originalURI on the channels it creates.

I presume you mean originalURI that is not the same as URI?

> 2) Can be the target of a redirect.

Because http channel overrides the custom originalURI set on the channel before when redirect proceeds, right?

> 
> Condition #1 is satisfied by the about: (via the about redirector), chrome:,
> file: protocols, plus whatever uses the substituting protocol handler
> (resource: and moz-extension:, I believe).  chrome:, resource:, and file:
> can't be used as the target of a redirect.  Some about: URLs can, but no one
> does that in practice.  If someone _were_ to do it, it would be just as
> broken: I just tried a test HTTP response that does a 302 to about:rights
> and it doesn't work.
> 
> > I'm not sure what the NS_GetFinalChannelURI helper has to do exactly
> 
> It's meant to return "the URI of this channel for security purposes, like
> determining the origin of the resulting document".  For protocols that
> effectively do their own "internal" redirect by returning a channel whose
> URI doesn't match the originalURI, this is the originalURI.  But for the
> HTTP redirect case it should really be "the originalURI of the post-redirect
> channel, as of when we created it".  

so, when we overwrite it with URL of the first channel in the redirect chain after redirect observers are called, we destroy that information, right?

> We don't have a way to get this right
> now (because that state is not stored anywhere), so we just use the URI of
> the post-redirect channel when LOAD_REPLACE is set.  

> This worked ok as long
> as there were no redirects to protocols whose handlers set an interesting
> originalURI, but no we do in fact want to support such redirects.
> 
> For purposes of the security checks during redirect, it would be enough to
> just set LOAD_REPLACE after we call the redirect observers.  

I'm afraid that will break few things...  I already took a look and there were places counting on it.

> That would
> cause the redirect observers to see the default (protocol-handler-set)
> originalURI on the post-redirect channel, which is what we really want to do
> security checks against.  


> I had thought this might be enough, but of course
> it's not, because the extension protocol handler doesn't do anything
> interesting with loadinfo or its channel owners/principals.  

This is the part I don't quite follow.

> As a result,
> the results of those loads get a principal based on the "final channel URI",
> and that means we really do need some sort of setup that will produce the
> "moz-extension:" URI for the "final channel URI" there.
> 
> So what we really need to fix this bug is one of two things, I think:
> 
> A) Stop setting originalURI during redirects, audit all originalURI
> consumers to make sure they're OK with this change.

Or, set it only under some conditions, like when targetchannel->URI == targerchannel->originURI?  but that may be too weak.  maybe channels should say explicitly "I don't want my original URI" be modified.

> 
> B) Add some other way of getting the "original" originalURI on the
> post-redirect channel, ensure it works correctly both while notifying
> redirect observers and after we get OnStartRequest, and update
> NS_GetFinalChannelURI to use that.

that sounds like more straightforward way - originalOriginalURI :)  but that is something channel implementation must implement and maintain..  

maybe rather have redirectSourceURI that is set by channels capable of doing a redirect (mostly just http) ; it would be set instead of how originalURI is being set now.  but then we must audit all consumers of the originalURI which would remain identical to the channel's URI..  still, sounds to me like the cleanest solution.  would have to check (will do tomorrow, leaving ni? on me then.)

> 
> Does that make sense?

I think yes, except the one part with principals I don't understand fully.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> I presume you mean originalURI that is not the same as URI?

Yes.

> Because http channel overrides the custom originalURI set on the channel before
> when redirect proceeds, right?

Indeed.  Note that BaseChannel's redirect does the same thing, so this is not, strictly speaking, HTTP-specific; it's just a property of how necko models redirects.  In practice, http redirects are the type of redirect web authors are most likely to run into, of course, but for extension authors there may be other cases too.

> so, when we overwrite it with URL of the first channel in the redirect chain after
> redirect observers are called, we destroy that information, right?

Yep.

> I'm afraid that will break few things...  I already took a look and there were places counting on it.

I would be somewhat interested in the details here.

> This is the part I don't quite follow.

Consider a load of "moz-extension:whatever".  Under the hood this redirects to a file://whatever-else URI, but the principal for the resulting page should have "moz-extension:whatever" as the URI, not file://whatever-else, because those are in fact quite different trust domains.  Normally (no redirects involved) this works because we base the principal on the return value of NS_GetFinalChannelURI unless the loadinfo for the channel explicitly says to use something else, and in this case that would be the originalURI, which is the "moz-extension:whatever".  

But in the redirect case, by the time we're computing the principal the channel has the pre-redirect http:// URI as originalURI.  And since we certainly don't want to use _that_ for the principal, it also has LOAD_REPLACE set, which tells the security manager to use the URI, not originalURI, of the post-redirect channel to produce a principal.  In this case that would be file://.  But really, there's no way to win here: the moz-extension: URI is not available from the channel any more by this point.

Note that we _could_ address this issue by having the extension protocol set the desired principal in the loadinfo, along with the right "just use the principal in the loadinfo" flags.  That would make the resulting principal come out right, independently of what NS_GetFinalChannelURI returns for the channel.  That won't be enough to make the redirect security checks, which test against the new URI, not a principal, pass...

> Or, set it only under some conditions, like when targetchannel->URI == targerchannel->originURI? 

But then what's the point of setting it at all on redirects?  I _think_ originally it was meant to be a way to not have to keep track of redirects if you want to use a URI as a hashtable key or something.  But if we only set it on redirect _sometimes_ it becomes just as useless as if we _never_ set it, right?

> that sounds like more straightforward way - originalOriginalURI :)  but that
> is something channel implementation must implement and maintain..  

Indeed.

Comment 51

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #50)
> I would be somewhat interested in the details here.

The bad thing is that we are using the LOAD_REPLACE flag for two things apparently: 

1) when the resource content is actually coming from a different location than the channel has originally be opened for and we want that 'real' location use for representation and also security checks (that's what NS_GetFinalChannelURI does)

2) when the resource content location has been redirected (internally or externally) and the channel having this flag is that redirect target


So, possible solutions:
- have a different flag identifying a redirected channel (LOAD_REDIRECTED, but we are out of flags, AFAIK)
- don't have any flag identifying a redirected channel (i.e. don't set LOAD_REPLACE on channels being targets of a redirect) 

with it, don't let http channels rewrite the originalURI on redirect target channels.  I didn't check consumers of originalURI yet.  but if there is a demand for the redirect source, these days there is the whole chain of principals redirect went through to examine available.



I was checking the usage of the flag and there are few places that seem sensitive:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7791

  we don't want to do uri fixup (like ttp:// -> http://) on channels that has this flag set, comment says "don't guess on URIs that are result of a redirect"

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5688

  we don't want to notify "http-on-opening-request" for http channels that are redirect targets (apparently, not sure exactly why, tho) ; anyway, could be fixed if needed

https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#437
  
  it's just an assertion, but it seems to be checking that the document load request is either null at that point or the onstartrequest comes from a channel that is a redirect target 
  (which is a bit odd, since channels that redirect to a different channel never call onstartrequest, i.e. only the final (last) channel from the redirect chain will even notify onstart/onstoprequest.)

Comment 52

[Boris Zbarsky [:bzbarsky]]

> The bad thing is that we are using the LOAD_REPLACE flag for two things apparently: 

OK, good point.  For what it's worth, I think the only user that cares about your use #1 (forcing the use of URI instead of originalURI for the principal, but not doing an actual "redirect" in the sense of sending redirect notification) is the about redirector.  And maybe multimixedconv, but it's not clear to me why that's messing with this flag...

> So, possible solutions:

A third solution is to change the about redirector to act more like a normal redirect, so there is really only one usage.  It's a bit annoying, because it requires an actual second channel, or having the originalURI for the about redirector case not match the URI passed to NS_NewURI...

> but we are out of flags, AFAIK

I think we have some on nsIRequest.  But also, we could put one of these flags (e.g. the "use the originalURI or URI for principal" boolean) on the LoadInfo.  

> https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7791

Not an issue if we set LOAD_REPLACE right after notifying redirect observers, but an issue if we want to keep it unset all the way through.

That said, if we want to support both modes ("use URI for principal" and "use originalURI for principal") then we would want to not set this flag for HTTP redirects...  We can use the loadinfo's redirect chain here, though, if all we care about is whether there were any redirects.

> we don't want to notify "http-on-opening-request" for http channels that are redirect targets

Again, could be done via the loadinfo if needed, right?

> https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#437
> which is a bit odd, since channels that redirect to a different channel never call onstartrequest

This is the nsIRequestObserver implementation for the groupObserver of a loadgroup.  It gets onstartrequest whenever a non-background channel is added to the loadgroup.  So the assert is saying that whenever a LOAD_DOCUMENT_URI request is added to the loadgroup either there isn't one already or we're dealing with a redirect (of the existing one, presumably).

OK.  So maybe the simplest thing is to change people who care about "is a redirect target" to use an accessor on loadinfo, which will presumably return something like !mRedirectChainIncludingInternalRedirects.IsEmpty().  And then we can repurpose the existing LOAD_REPLACE flag to just mean "use the URI instead of the originalURI for the final channel URI).

I looked into whether we can just change redirects to not change originalURI, and that would in fact break some consumers that expect to be able to get the "URL the page thinks it started loading".  That includes, for example, PerformanceMainThread::AddEntry and SheetLoadData::OnStreamComplete.  We could change the latter to work more like the scriptloader (which just saves the URI in the stream listener, effectively), but the performance API case is harder to fix.

So it seems like we may want some other "real URI to use for security purposes" thing.  Maybe we can put that in the loadinfo too?  If we did that, then we wouldn't need to do anything about LOAD_REPLACE; it could just have your meaning #2, and NS_GetFinalChannelURI would check loadinfo, and if that doesn't have a URI to use then it would fall back to the channel URI.  The about: protocol handler would change to simply not setting the thing in the loadinfo in some cases.

That means changing all our protocol handlers that set interesting originalURIs, but that seems ok....

Comment 53

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #52)
> > The bad thing is that we are using the LOAD_REPLACE flag for two things apparently: 
> 
> OK, good point.  For what it's worth, I think the only user that cares about
> your use #1 (forcing the use of URI instead of originalURI for the
> principal, but not doing an actual "redirect" in the sense of sending
> redirect notification) is the about redirector.  

there is one more:
https://dxr.mozilla.org/mozilla-central/rev/4f09d9469e73adf32c7db6720504fcbe580516b3/netwerk/protocol/file/nsFileChannel.cpp#285

here I'm not sure, if this is for redirects or orignalURL/URL switch:
https://dxr.mozilla.org/mozilla-central/rev/4f09d9469e73adf32c7db6720504fcbe580516b3/modules/libjar/nsJARChannel.cpp#903

> And maybe multimixedconv,
> but it's not clear to me why that's messing with this flag...

introduced 16 years ago: https://github.com/englehardt/gecko-dev/commit/87373a21f6a1f7e9b9dfa0f9557a922aab0841f2

+	/**
+	 * This flag is used to tell the webshell not to cancel the load in cases
+	 * when the channel is receiving multipart/replace document
+	 */
+	const unsigned long LOAD_REPLACE			   = 1 << 16;

Interesting... :)

> 
> > So, possible solutions:
> 
> A third solution is to change the about redirector to act more like a normal
> redirect, so there is really only one usage.  It's a bit annoying, because
> it requires an actual second channel,

there already is, I think:
https://dxr.mozilla.org/mozilla-central/rev/4f09d9469e73adf32c7db6720504fcbe580516b3/docshell/base/nsAboutRedirector.cpp#180

> or having the originalURI for the
> about redirector case not match the URI passed to NS_NewURI...

Not sure what you mean.

> 
> > but we are out of flags, AFAIK
> 
> I think we have some on nsIRequest.  But also, we could put one of these
> flags (e.g. the "use the originalURI or URI for principal" boolean) on the
> LoadInfo.  
> 
> > https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7791
> 
> Not an issue if we set LOAD_REPLACE right after notifying redirect
> observers, but an issue if we want to keep it unset all the way through.

I think the latter applies here, so it will need to migrate.

> 
> That said, if we want to support both modes ("use URI for principal" and
> "use originalURI for principal") then we would want to not set this flag for
> HTTP redirects...  We can use the loadinfo's redirect chain here, though, if
> all we care about is whether there were any redirects.

Yes.

> 
> > we don't want to notify "http-on-opening-request" for http channels that are redirect targets
> 
> Again, could be done via the loadinfo if needed, right?

Yes.

> 
> > https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#437
> > which is a bit odd, since channels that redirect to a different channel never call onstartrequest
> 
> This is the nsIRequestObserver implementation for the groupObserver of a
> loadgroup.  It gets onstartrequest whenever a non-background channel is
> added to the loadgroup.  So the assert is saying that whenever a
> LOAD_DOCUMENT_URI request is added to the loadgroup either there isn't one
> already or we're dealing with a redirect (of the existing one, presumably).

Got it.

> 
> OK.  So maybe the simplest thing is to change people who care about "is a
> redirect target" to use an accessor on loadinfo, which will presumably
> return something like !mRedirectChainIncludingInternalRedirects.IsEmpty(). 
> And then we can repurpose the existing LOAD_REPLACE flag to just mean "use
> the URI instead of the originalURI for the final channel URI).
> 
> I looked into whether we can just change redirects to not change
> originalURI, and that would in fact break some consumers that expect to be
> able to get the "URL the page thinks it started loading".  That includes,
> for example, PerformanceMainThread::AddEntry and
> SheetLoadData::OnStreamComplete.  

Both solvable by inspecting loadinfo, just take the first URL in the array of redirect principals.

> We could change the latter to work more
> like the scriptloader (which just saves the URI in the stream listener,
> effectively), but the performance API case is harder to fix.
> 
> So it seems like we may want some other "real URI to use for security
> purposes" thing.  Maybe we can put that in the loadinfo too?  If we did
> that, then we wouldn't need to do anything about LOAD_REPLACE; it could just
> have your meaning #2, and NS_GetFinalChannelURI would check loadinfo, and if
> that doesn't have a URI to use then it would fall back to the channel URI. 
> The about: protocol handler would change to simply not setting the thing in
> the loadinfo in some cases.
> 
> That means changing all our protocol handlers that set interesting
> originalURIs, but that seems ok....

So, it's about to decide which is better: get redirect info from load info (which is already there, btw) and not via LOAD_REPLACE/orignalURI, or put the security check URI on load info and only use LOAD_REPLACE to say "hey, I've been redirected".


or, maybe remove the LOAD_REPLACE flag altogether and migrate all consumers to loadinfo to explicitly see either the security check URI or the channel's redirect status.  and not abandon just one of the meaning of the flag and hope nothing's gonna break..  that flag has probably been abused not just once in the past already.

Comment 54

[Boris Zbarsky [:bzbarsky]]

> there is one more:

Good catch.

> here I'm not sure, if this is for redirects or orignalURL/URL switch:

This is for the jar:http://url-that-redirects!/something.  In that case, we want the security stuff to be based on what we mutate our "URI" to, which is jar:redirect-target!/something.  But yes, I agree this case also cares about the security aspect.

> there already is, I think:

No, this is just the channel that gets returned from the protocol handler newChannel.  To make it look like a "normall" redirect, we'd have to create a channel with about:whatever as URI, then create the channel we create now, then send redirect notifications, etc.  Lots of potential for breakage there.

> Not sure what you mean.

I mean create a channel whose URI is tempURI in nsAboutRedirector::NewChannel and NOT call SetOriginalURI(aURI) on it in the cases when !isUIResource.

That may not help, because there are other places like docshell that will do NewChannel(someURI) and then immediately SetOriginalURI(someURI) (same URI as it passed to NewChannel) on it, iirc.  :(

> Both solvable by inspecting loadinfo, just take the first URL in the array of redirect principals.

Hmm.  Do we trust it sufficiently?  I'm not sure I do: if we start off with some channel that has the "look at my URI, not originalURI, for my principal" flag already, and then redirect, this will pick up the wrong URI (that first channel's URI as opposed to its originalURI), no?

> maybe remove the LOAD_REPLACE flag altogether and migrate all consumers
> to loadinfo to explicitly see either the security check URI or the channel's redirect status.

That seems reasonable to me.  And keep the current behavior of originalURI, then, as the thing set at initial channel creation time and then propagated through redirects?

Comment 55

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #54)
> > maybe remove the LOAD_REPLACE flag altogether and migrate all consumers
> > to loadinfo to explicitly see either the security check URI or the channel's redirect status.
> 
> That seems reasonable to me.  And keep the current behavior of originalURI,
> then, as the thing set at initial channel creation time and then propagated
> through redirects?

(I'm sorry for such a late answer, I was pretty much distracted last few days.)

Yes!  So, to sum up what the plan may be:

- remove the LOAD_REPLACE flag
- let originalURI be the first URI in a redirect chain(, never changed to anything else through out the redirect chain lifetime)
- if the security check should be made against a different URL, say so by setting it on the channel's associated LoadInfo (a new property of it) ; migrate all consumers that do |if LOAD_REPLACE -> look at URL, else -> look at originalURL| look for this new property, if not set, use _URL_ of the channel
- with the previous bullet I think it might be wise to rename the NS_GetFinalChannelURL method to something more descriptive, but it's more up to you, Boris
- if we want to find out whether a channel is a redirect target channel, inspect the chain (array) of redirect principals on its associated LoadInfo (provide a helper API to make it more simple)


Makes sense?  I didn't verify if this is what we want on all the consumers, but I think it should cover everything.

Comment 56

[Boris Zbarsky [:bzbarsky]]

I think that approach makes sense, yes.

How do we want to split up the work here?  Seems like your last bullet and third bullet (moving away from using LOAD_REPLACE for detecting redirect targets, and adding a new loadinfo thing for "the thing to be used to derive the principal) should be done first, and in parallel; then we update/rename NS_GetFinalChannelURL and LOAD_REPLACE becomes dead code we can remove....  For purposes of fixing this bug, it's really the third and fourth bullets that are needed, right?

Comment 58

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #56)
> I think that approach makes sense, yes.
> 
> How do we want to split up the work here?  Seems like your last bullet and
> third bullet (moving away from using LOAD_REPLACE for detecting redirect
> targets, and adding a new loadinfo thing for "the thing to be used to derive
> the principal) should be done first, and in parallel; then we update/rename
> NS_GetFinalChannelURL and LOAD_REPLACE becomes dead code we can remove.... 
> For purposes of fixing this bug, it's really the third and fourth bullets
> that are needed, right?

Something like this, yes.  I'll file the bugs.

Comment 59

[Shane Caraveo (:mixedpuppy)]

Honza, I wonder if the fix for this may also address, or be part of the fix for bug 1276048.

Comment 60

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #59)
> Honza, I wonder if the fix for this may also address, or be part of the fix
> for bug 1276048.

I can't say for sure now, but it is possible.  I'm in the middle of writing a patch for bug 1319111 (that is a prerequisite or even the final fix for this bug).  But usage of OrignalURL is kinda unclear for me on few places, so it takes time to migrate everything correctly.

Comment 62

[testit]

I just tested bug 1319111 which should also solve this bug. I used the nightly from 2017-05-06 and tested the attached test case. Unfortunately, the redirect still does not work, in the console still the error message "Security Error: Content at http://<url> may not load or link to file:///C:/extension/testit.html." is displayed.

Comment 63

[Shane Caraveo (:mixedpuppy)]

I don't believe that bug 1319111 will fix this redirect issue without potentially several other minor fixes along the way.

Comment 64

[Boris Zbarsky [:bzbarsky]]

I actually expected bug 1319111 to fix this.

Specifically, after that bug HTTP redirection does not stomp on the resultPrincipalURI the newChannel call sets up, if any.  SubstitutingProtocolHandler::NewChannel2 sets resultPrincipalURI.  ExtensionProtocolHandler::SubstituteChannel doesn't mess with it.

So I would expect this to work correctly now.  If it's not, why is it not?

Comment 65

[Boris Zbarsky [:bzbarsky]]

Ah, comment 47 last paragraph, probably.

Comment 66

[Honza Bambas (:mayhemer)]

(In reply to testit from comment #62)
> I just tested bug 1319111 which should also solve this bug. I used the
> nightly from 2017-05-06 and tested the attached test case. Unfortunately,
> the redirect still does not work, in the console still the error message
> "Security Error: Content at http://<url> may not load or link to
> file:///C:/extension/testit.html." is displayed.

Please note that the I reopened bug 1319111 and there will be a different solution to it.  Let's wait until then, I think the new and better fix might fix this one "just that".

Comment 67

[Shane Caraveo (:mixedpuppy)]

You can apply the patch in bug 1276048 attachment 8863958 [details] [diff] [review] and run that, it has 3 tests, first currently and correctly passes, the other two fail to redirect.  I'm also happy to do that after the changes on bug 1319111 are ready.

Comment 68

[Honza Bambas (:mayhemer)]

The URI collected for CheckMayLoad are unaffected by the LOAD_REPLACE or resultPrincipalURI at all....  

We compare the old channel principal against new channel's inner most URI.  It's grabbed directly from the channel via GetURI and the patch for 1319111 has absolutely no effect on it.  The target channel URI is a jar: URI.  Hence, we end up comparing https://example.com/.../redirect.sjs?... against a file URI as mentioned in comment 0.


 	xul.dll!nsScriptSecurityManager::ReportError(0x00000000, {...}, 0x32b634a0, 0x2ea72f10) Line 1003	C++
 	xul.dll!nsScriptSecurityManager::CheckLoadURIFlags(0x32b634a0, 0x2ea72f10, 0x32b634a0, 0x32b63230, 9) Line 956	C++
>	xul.dll!nsScriptSecurityManager::CheckLoadURIWithPrincipal(0x20c9d1d0, 0x2ea72f10, 9) Line 807	C++
 	xul.dll!nsContentSecurityManager::AsyncOnChannelRedirect(0x332ea038, 0x20bee790, 1, 0x3345d314) Line 522	C++

See https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/dom/security/nsContentSecurityManager.cpp#511-527

Comment 69

[Boris Zbarsky [:bzbarsky]]

> We compare the old channel principal against new channel's inner most URI.

No, we compare against both the new channel's URI and the new channel's originalURI, in the block you quoted.  The point is that, in my opinion, we should be comparing against only one URI: the "final channel URI".  And that will be affected by the patch for bug 1319111.

Comment 70

[Shane Caraveo (:mixedpuppy)]

The one aspect that would be missing (assuming a final channel uri check works) is checking nsIProtocolHandler::URI_LOADABLE_BY_ANYONE (see what I did in attachment 8863960 [details] [diff] [review] in bug 1276048).

Comment 71

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #70)
> checking nsIProtocolHandler::URI_LOADABLE_BY_ANYONE

This is necessary *somewhere* to make sure that the extension uri is allowed to be accessed via redirects and other "external" access (e.g. links)

Comment 72

[Boris Zbarsky [:bzbarsky]]

> is checking nsIProtocolHandler::URI_LOADABLE_BY_ANYONE 

That's already checked by CheckLoadURIWithPrincipal.  The important part is to only call that on the moz-extension: URI in the moz-extension: case.

Comment 73

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #69)
> > We compare the old channel principal against new channel's inner most URI.
> 
> No, we compare against both the new channel's URI and the new channel's
> originalURI, in the block you quoted.  The point is that, in my opinion, we
> should be comparing against only one URI: the "final channel URI".  And that
> will be affected by the patch for bug 1319111.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a5f7d5b41845b8ea4af45aad56779919d2cd973

Comment 74

[Honza Bambas (:mayhemer)]

Attachment: wip1 (part1) as suggested in comment 69

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8dbdd3b69c1470bbe38e732e88d904d328677c

Comment 75

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d95516143f13276378f6e695dfbc1c2ab2449c

Comment 76

[Honza Bambas (:mayhemer)]

Attachment: wip1 (part2) force SubstitutingProtocolHandler::NewChannel2 set result principal URI to original URI, part of bug 1319111

part1 and part2 wips move the test (test_extRedirect_accessible) closer to passing, but there is apparently still more work to do, this time more on the moz-extension side.  

I've added logging to that test (comment 67) giving me the following output in the console:

2 INFO TEST-START | browser/components/extensions/test/browser/browser_ext_redirect.js
3 INFO Entering test bound test_extRedirect
4 INFO Extension loaded
GECKO(1600) | [1600] WARNING: Could not get disk status from nsIDiskSpaceWatcher: file c:/Mozilla/src/mozilla-central1/uriloader/prefetch/nsOfflineCacheUpdateService.cpp, line 284
5 INFO got browser.tabs.onUpdated.addListener, s=undefined u=undefined
6 INFO got browser.tabs.onUpdated.addListener, s=loading u=undefined
GECKO(1600) | [1600] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/Mozilla/src/mozilla-central1/dom/security/nsContentSecurityManager.cpp, line 520
7 INFO Console message: Security Error: Content at https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://95676119-9c1d-4e4a-ab89-f349f040f11a/finished.html may not load or link to moz-extension://95676119-9c1d-4e4a-ab89-f349f040f11a/finished.html.
GECKO(1600) | [1600] WARNING: attempt to modify an immutable nsStandardURL: file c:/Mozilla/src/mozilla-central1/netwerk/base/nsStandardURL.cpp, line 1717
8 INFO got browser.tabs.onUpdated.addListener, s=undefined u=undefined
9 INFO got browser.tabs.onUpdated.addListener, s=complete u=undefined
10 INFO got browser.tabs.onUpdated.addListener, s=complete u=https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://95676119-9c1d-4e4a-ab89-f349f040f11a/finished.html
11 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_redirect.js | tab should not redirect to moz-extension - Expected: https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://95676119-9c1d-4e4a-ab89-f349f040f11a/finished.html, Actual: https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://95676119-9c1d-4e4a-ab89-f349f040f11a/finished.html -
12 INFO Leaving test bound test_extRedirect
13 INFO Entering test bound test_extRedirect_accessible
14 INFO Extension loaded
15 INFO Console message: Ignoring response to aborted listener for 9-0
16 INFO Console message: Ignoring response to aborted listener for 10-0
17 INFO got browser.tabs.onUpdated.addListener, s=undefined u=undefined
18 INFO got browser.tabs.onUpdated.addListener, s=loading u=undefined
19 INFO got browser.tabs.onUpdated.addListener, s=loading u=moz-extension://a1bd09c2-e0c2-4622-8576-e0638083a304/finished.html
20 INFO got browser.tabs.onUpdated.addListener, s=undefined u=undefined
21 INFO got browser.tabs.onUpdated.addListener, s=complete u=undefined
Failed to retrieve MOZ_UPLOAD_DIR env var
22 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_redirect.js | Test timed out -


The line #21 shows that the onUpdated notification is missing the URL for the "complete" state.  This is rather erratic, since the first test (test_extRedirect) shows there are two notifications of "complete", only where the second has url.  There sometimes is and sometimes isn't the url property on the "changed" arg object.

How to proceed?  I would also be happier if someone more familiar with the moz-extension code took this.

Comment 77

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #76)
> I've added logging to that test (comment 67) giving me the following output
> in the console:

+++ b/browser/components/extensions/test/browser/browser_ext_redirect.js
@@ -23,16 +23,17 @@ const testExtension = {
 }

 function backgroundScript(expectFail) {
   let exturi = browser.extension.getURL("/finished.html");
   let uri = "https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs";
   let url = `${uri}?redirect_uri=${exturi}`;

   browser.tabs.onUpdated.addListener((changedTabId, changed) => {
+    browser.test.log("got browser.tabs.onUpdated.addListener, s=" + changed.status + " u=" + changed.url);
     if (changed.status === "complete" && changed.url) {
       if (expectFail) {
         browser.test.assertEq(url, changed.url, "tab should not redirect to moz-extension");
       } else {
         browser.test.assertEq(exturi, changed.url, "tab should redirect to moz-extension");
       }
       browser.tabs.remove(changedTabId);
       browser.test.sendMessage("done");

Comment 78

[Shane Caraveo (:mixedpuppy)]

"complete" comes from a progress listener.  It has a url only when called via onLocationChange.  When called via onStateChange for STATE_STOP it will not have a url.  Per docs here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated url is only included if the url has changed.  You can see the listener here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionTabs.jsm#1104

With patch from Bug 1319111 and the part 1 patch here (part 2 is obsolete, didn't/couldn't apply it):

During test_extRedirect_accessible I see AsyncOnChannelRedirect called twice, the first time with moz-extension, the second time with a jar:file:// url pointing to the extension file.  The second call fails CheckLoadURIWithPrincipal.

14 INFO Entering test bound test_extRedirect_accessible
15 INFO Extension loaded
16 INFO tabs.onUpdated.addListener, s=loading u=undefined
GECKO(15181) | finalChannelURI[moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/finished.html]
GECKO(15181) | finalChannelURI[jar:file:///private/var/folders/tf/rsv8z5mj4_n_qqxk4c48r9k00000gn/T/TemporaryItems/generated-extension-1.xpi!/finished.html]
17 INFO Console message: Security Error: Content at https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/finished.html may not load or link to jar:file:///private/var/folders/tf/rsv8z5mj4_n_qqxk4c48r9k00000gn/T/TemporaryItems/generated-extension-1.xpi!/finished.html.
18 INFO tabs.onUpdated.addListener, s=complete u=undefined
19 INFO tabs.onUpdated.addListener, s=complete u=https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/finished.html
20 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_redirect.js | tab should redirect to moz-extension - Expected: moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/finished.html, Actual: https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/finished.html -
Stack trace:
    @moz-extension://2d752164-dda1-f641-8877-0ed2df16b830/%7Ba45b1c7a-4fca-d04d-8546-6a4cfd1669ed%7D.js:12:9

21 INFO Console message: Ignoring response to aborted listener for 11-0
22 INFO Console message: Ignoring response to aborted listener for 12-0
23 INFO Console message: Ignoring response to aborted listener for 13-0
24 INFO Leaving test bound test_extRedirect_accessible

Comment 79

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #78)
> "complete" comes from a progress listener.  It has a url only when called
> via onLocationChange.  When called via onStateChange for STATE_STOP it will
> not have a url.  Per docs here:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
> url is only included if the url has changed.  You can see the listener here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionTabs.jsm#1104

And what is it you expect in the test?  For me it times out all the time because there is no uri on the object.

> 
> With patch from Bug 1319111 

Presuming you had your sources up to date and were testing with v11 (from 2017-05-15 15:28)

> and the part 1 patch here 

yes

> (part 2 is obsolete,
> didn't/couldn't apply it):

That one is dead, yes.

> 
> During test_extRedirect_accessible I see AsyncOnChannelRedirect called
> twice, the first time with moz-extension, the second time with a jar:file://
> url pointing to the extension file.  The second call fails
> CheckLoadURIWithPrincipal.

Which of them are expected and which not?  What are the expected results?  Where from are both called?

The problem I see is that in most cases (not always, what indicates a race condition!!) the test times out because the |changed| object is not what you expect (as you explain) and browser.test.sendMessage("done"); is then never called.

The redirect-to handling of result principal URI is correct.  Now we have to fix your code and the test.  Since I did not get a clear answer what is expected, what is not, I can't much help right now.  And I don't know the moz-ext code at all.

Comment 80

[Honza Bambas (:mayhemer)]

When I fix the test as:

  var changed_url = null;

  browser.tabs.onUpdated.addListener((changedTabId, changed) => {
    if (changed.url) changed_url = changed.url;
    if (changed.status === "complete" && changed_url) {


then it passes for me.

Comment 81

[Honza Bambas (:mayhemer)]

But still:

8 INFO Entering test bound test_extRedirect_webRequest
9 INFO Extension loaded
10 INFO onBeforeRequest url http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html?r=0.2539724607312579
11 INFO ... redirecting to moz-extension://49bb633b-0e30-4733-9eff-9ce30eeb1ff4/finished.html

and gets stuck (main thread responsive, tho).  I really don't have more time to investigate, sorry.

Comment 82

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

I've applied the latest patch, bug 1319111 Attachment #8870446 [details] [diff] and Attachment #8867322 [details] [diff] to a pull from m-c this morning.  Both tests, test_extRedirect_accessible and test_extRedirect_webRequest, get a security error and fail.

To avoid any discussion about how webextensions, webrequest or the hang are the problem here, I've rewritten the tests to not use any webextension api for testing the redirection.

There is an extension in the test, it exists only to provide a valid moz-extension landing page and otherwise has no involvement.

Apply the patch then:
./mach mochitest --tag remote-webextensions browser/components/extensions/test/browser/browser_ext_redirect_chrome.js


Both tests still fail due to a security error:

8 INFO Console message: Security Error: Content at https://example.com/browser/browser/components/extensions/test/browser/redirect.sjs?redirect_uri=moz-extension://3d86c6ac-157f-b44a-b161-37ba7a9f568b/finished.html may not load or link to jar:file:///private/var/folders/tf/rsv8z5mj4_n_qqxk4c48r9k00000gn/T/TemporaryItems/generated-extension.xpi!/finished.html

Comment 83

[testit]

I just tested bug 1319111 which should also solve this bug. I used the nightly from 2017-05-24 and tested the above attached test case (https://bugzilla.mozilla.org/attachment.cgi?id=8805828). Unfortunately, the redirect still does not work, in the console still the error message "Security Error: Content at http://<url> may not load or link to file:///C:/extension/testit.html." is displayed.

As far as I understand, bug 1319111 was created to fix this bug. So I think it would be a good idea to test the above mentioned testcase before setting the status of bug 1319111 to resolved fixed.

Comment 84

[Honza Bambas (:mayhemer)]

(In reply to testit from comment #83)
> As far as I understand, bug 1319111 was created to fix this bug. So I think
> it would be a good idea to test the above mentioned testcase before setting
> the status of bug 1319111 to resolved fixed.

bug 1319111 is just a basis for fixing this issue.  there is more to do.  the patch in that bug has landed, hence the bug is marked as fixed.

Comment 85

[Boris Zbarsky [:bzbarsky]]

Shane, thank you for the testcase!

So what happens in that testcase in e10s mode is that we get two calls to nsContentSecurityManager::AsyncOnChannelRedirect for each redirect.  The first call happens in the _parent_ process and is passed "moz-extension://25eea7ce-4788-0c46-ad71-1c6fe9d535cd/finished.html" as the final channel URI of the post-redirect channel.  The second call happens in the _child_ process and is passed "jar:file:///private/var/folders/cn/n4nn_b1x5zd4fs1gywhrjwr80000gn/T/TemporaryItems/generated-extension-3.xpi!/finished.html" as the final channel URI.

That second call is coming from HttpChannelChild::Redirect1Begin which is passed that jar: URI as the "newURI".  So the fact that it sets that as the channel principal URI in CloneLoadInfoForRedirect doesn't help.

I can't tell what happens in non-e10s mode, because the test hangs in that mode.  But I expect redirects to moz-extension: work correctly there.

Anyway, the jar: URI in Redirect1Begin is coming from HttpChannelParent::StartRedirect doing:

    nsCOMPtr<nsIURI> newURI;
    newChannel->GetURI(getter_AddRefs(newURI));

and then using that URI as the URI it sends.  Presumably it should send down more information to allow us to more correctly reconstruct the post-redirect state in the child process.  That said, it's not clear to me why we end up doing these redirect security checks both in the parent and the child process.  Do we really want to do that?

Comment 86

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

This patch works around the lack of channel.URI in the request observer when running non-e10s.

Redirects are in fact working if I run mochitest --disable-e10s.

Comment 87

[Honza Bambas (:mayhemer)]

Comment on attachment 8867322 [details] [diff] [review]
wip1 (part1) as suggested in comment 69

I moved this patch to a separate bug (seems appropriate)

Comment 88

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #82)
> Apply the patch then:
> ./mach mochitest --tag remote-webextensions
> browser/components/extensions/test/browser/browser_ext_redirect_chrome.js

Thanks!

So, with:
- bug 1319111 patch v11.1
- bug 1367586 patch v1
- bug 1367814 wip patch

and --disable-e10s the test works.

Additionally with
- bug 1368110 patch v1

and e10s enabled, the test fails because the moz-extension channel on the child process (created at HttpChannelParent::StartRedirect) doesn't implement nsIChildChannel interface.  We are not able to redirect to such channels and hence the redirect is technically vetoed.

Hence, now it turns back to you, you have to implement the child/parent redirect machinery.  The child channel has to implement nsIChildChannel and the parent channel must implement nsIParentChannel (which is trivial).  Good example (since it's simple) is our FtpChannelChild/Parent.


-- How the e10s redirects work --

The thing is that during an http redirect to a URI, the target channel is first created (NS_NewChannel) on the parent process (as there were no e10s involved).  After that this new parent 'physical' channel is registered under a unique id and the information about the redirect and this id goes to the child process.  There we create a child channel for the same URI.  We QI to nsIChildChannel (or hard-fail) and call ConnectParent(parent-channel-id) which, in most cases, creates a parent IPC channel that then connects with the 'physical' channel on the parent process through the given id.  Both parent and child processes do redirect security checks and when done and passed, the parent process opens (AsyncOpen) the 'physical' channel on the parent process.  At the same time we send a message to the child that the redirect got a green lite and the succeeded to AsyncOpen.  We again QI to nsIChildChannel and call CompleteRedirectSetup.  Meaning is similar to calling AsyncOpen, but the channel can decide what to actually do.  And now we are done, the machinery of the content data delivery should go on the same way as if the channel were normally initiated from the child process side.


(mayhemer thinks this is the shortest explanation of this mechanism ever!)

Comment 89

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #85)
> That said, it's not clear to me why we end up
> doing these redirect security checks both in the parent and the child
> process.  Do we really want to do that?

I believe we should do both.  redirect observation is a pretty generic thing, anyone can register to do it either globally or on a separate channel via callbacks.  When I was writing the e10s redirects years ago I was afraid we could miss some security check or simply a on-redirect update code (there is many! - better safe than sorry).  If these days we are sure there are no additional security checking or redirect update observers on the child process, then it's probably OK to remove the check calls.

But w/o a proper audit I would be worried a bit we could miss a veto check or simply break something hard to predict.

Comment 90

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #88)
> At the same time we send a message to the child that the
> redirect got a green *light* and the *parent 'physical' channel* succeeded to AsyncOpen. 

Two coffees not enough on friday evening.

Comment 91

[Honza Bambas (:mayhemer)]

I think bug 1334550 may be related to how to implement the redirection here.

Comment 92

[Shane Caraveo (:mixedpuppy)]

If we're proxying the protocol to the parent, do we also need parent/child for this?  This is all pretty muddy.

Comment 93

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #92)
> If we're proxying the protocol to the parent, do we also need parent/child
> for this?  This is all pretty muddy.

Yeah, you have to.  This is how the redirects work.  It's built for channels that send the data from the parent to the child via OnDataAvailable (sending down strings...)  That's how HTTP/FTP work.

Bug 1334550 is a way more effective implementation for your case then copying the data down to the child, so I would definitely go that way.

So, the better example of how to implement this for moz-extensions is https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/file which is using remote file opening as well.  It just cancels the parent 'physical channel' from its OnStartRequest.  Yes, way ineffective...

Note that the 'physical channel' on the parent, being created when the redirect request arrives, doesn't know it's being created for serving an e10s redirect request.  It would be a way to tell the parent 'physical channel' to 'do nothing' when eventually open.

I will think about some mechanism how to prevent this ineffectiveness.  In the mean time, please get inspired by how the file channel is implemented.

Comment 94

[testit]

Bug 1319111, which was created to solve this bug, is fixed now. It would be great, if somebody could fix now this bug, so the redirect to extensions pages works.

Comment 95

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

This fixes the tests to work again (non-e10s passes).  Note that there is now a check for nsIJARURI in WebRequest.jsm.  I believe this has leaked through due to bug 1334550.

Comment 96

[Shane Caraveo (:mixedpuppy)]

Duping to where the work is actually happening to fix this.

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13a9e2bbb96a
Let nsContentSecurityManager check if a redirect may load against the target channel's final URI, r=bz

Comment 98

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8dd724b3cdf
Backed out changeset 13a9e2bbb96a for landing with wrong bug number in commit message

Comment 99

[Shane Caraveo (:mixedpuppy)]

Last week bug 1380186 landed, which should resolve any redirect to web accessible resources/moz-ext urls.  I've ni? a couple people, but anyone who has been blocked by this issue can chime in.  If you can give Firefox Nightly (56) a spin and see if your particular use case is is now handled that would be appreciated.

Comment 100

[testit]

Thanks, works as expected.

Comment 101

[Paul Heil]

With FF57.0a1 my use case still causes "Security Error: Content at https://<PAGE WITH TIFF> may not load data from blob:moz-extension://6a919b25-9bf0-4d3e-ba99-680063c3932e/31f15a3f-7cd8-4ae0-a292-186a1efb755e."

I've published my fork on Github for reference if that's okay. https://github.com/Pheil/chrome-tiff-viewer-fork

Comment 102

[Shane Caraveo (:mixedpuppy)]

(In reply to Paul Heil from comment #101)
> With FF57.0a1 my use case still causes "Security Error: Content at
> https://<PAGE WITH TIFF> may not load data from
> blob:moz-extension://6a919b25-9bf0-4d3e-ba99-680063c3932e/31f15a3f-7cd8-4ae0-
> a292-186a1efb755e."
> 
> I've published my fork on Github for reference if that's okay.
> https://github.com/Pheil/chrome-tiff-viewer-fork

That is a blob url, not a moz-extension url.  e.g. See bug 1331176 (and others related to blob urls opening).

Comment 103

[Synzvato]

(In reply to Shane Caraveo (:mixedpuppy) from comment #99)
> Last week bug 1380186 landed, which should resolve any redirect to web
> accessible resources/moz-ext urls.  I've ni? a couple people, but anyone who
> has been blocked by this issue can chime in.  If you can give Firefox
> Nightly (56) a spin and see if your particular use case is is now handled
> that would be appreciated.

I'm experiencing issues with both Firefox 56 and 57. My extension redirects website requests for particular scripts to locally bundled resources. It can redirect said requests without any problems as long as it's loaded as an unpacked extension. After I add the files to an (unsigned) .xpi archive, however, resources fail to load. Hard refreshes expose the behavior.

Signature verification was disabled in the desktop Nightly build, and the extension installed successfully. I checked for, but could not find any extension or browser console errors (apart from the file load errors). I used a fresh user profile.

It is worth noting that the same .xpi archive works fine on "Fennec" 57.

Comment 104

[Synzvato]

The redirection problems stated in my previous comment #103 have since been submitted as a separate bug. So, for more information, minimal test cases, and detailed steps to reproduce, please see bug 1390346.