1276048 - 302 response fails to redirect to moz-extension:// page

Status: RESOLVED DUPLICATE of bug 1380186

(WebExtensions :: Request Handling, defect, P1)

Description

[Edouard Oger [:eoger]]

I'm making a request on a 3rd party website (trello.com authorization) that replies with a 302 code, with the location header set to "moz-extension://00b68b4c.../settings.html".

Steps to reproduce:
You can check the extension code here
https://github.com/eoger/bug2trello/tree/firefox (the xpi is at the root of the project)
You need to click on the button to trigger the Trello authorization page that does this redirection and then click "Authorize".

Expected behavior:
Being redirected to my extension settings page

Actual behavior:
Getting a "Corrupted Content Error" error page

This is probably because the 302 is trying to redirect to the moz-extension:// protocol.
This settings.html page is listed in web_accessible_resources.

Comment 1

[Andy McKay]

Looking for help if this is something in Necko or elsewhere. I'll note that the reporter got bounced around irc channels between #addons and #necko. In a triage meeting for us this week, it was noted that this was more likely a Necko issue... but that's a big topic area it self. Any advice Jason?

Comment 2

[Jason Duell]

Honza, can you look at this?

Comment 3

[Honza Bambas (:mayhemer)]

Edouard, can you reproduce when you turn of e10s (multiprocess)?  If not, then this is duplicate of bug 1277028.

Comment 4

[Edouard Oger [:eoger]]

Same error without e10s

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

worth a notice that the moz-extensions url should be able to accept a redirect only if they are listed in the manifest as web_accessible_resources (reference link: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-extensions/xmRWA61Dj3c)

I took the chance to look a bit deeper in the WebExtensions related code that lives in the "netwerk/" dir and so I can now add some additional details about this:

First of all, it looks like that the redirection is first blocked by nsContentSecurityManager::AsyncOnChannelRedirect (and in my experiment I've tried to permit redirections to moz-extensions url if the url is one of the web_accessible_resources, using the AddonPolicyService)

Once I successfully tweaked the nsContentSecurityManager to permit the redirection (as described above), I met another issue with the ExtensionProtocolHandler (which is based on the SubstitutionProtocolHandler): 

the redirection end up into the corresponding "resolved" jar/file urls, and the oauth popup is pointed to the "resolved" jar/file urls instead of a "moz-extension://" url. 

By looking deeper I find out that in the past we solved similar issues for the "app://" url and the OpenWebApps:

- nsIJARChannel has a setAppURI method which permit to override the above behavior and get the JAR Channel to return the "app://" url on GetURI calls (useful references: https://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#948 and https://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#604)

- nsDocShell has a nsDocShell::DoAppRedirectIfNeeded method to help to handle the redirection to the "app://" urls (useful references: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6674)

I've tried to make similar changes for the "moz-extension://" urls (by defining a new SetMozExtensionURI on nsIJARChannel and a new DoAddonRedirectIfNeeded on nsDocShell) and I've been able to get the trello example chrome addon (https://github.com/trello/chromello) to successfully complete the oauth flow as a WebExtension on Firefox.

Unfortunately the WebExtension resources can be loaded from a "file://" url as well (besides the jar urls as described above), and so the experiment that I described above solves only half of the issue.

Given my limited experience in this part of our sources I've probably done a tons of wrong things in my experimental patch, but anyway, I'm going to attach it here because it can be helpful to better debug and plan this issue.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 1-Experiments_on_Bug_1276048___Support_redirect_to_moz_extension____URLs_.patch

The patch from the experiment described above.

Comment 7

[Andy McKay]

I note that bug 1277028 has been closed. Wondering, if based on comment 3, this is now fixed, or we need to do more. Edouard, would you have chance to test in Nightly again?

Comment 8

[Edouard Oger [:eoger]]

Not working in today's nightly, sorry.

Comment 9

[Andy McKay]

Honza, could you give some feedback on Luca's proposed patch, we aren't sure if its the right strategy.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Andy McKay [:andym] from comment #7)
> I note that bug 1277028 has been closed. Wondering, if based on comment 3,
> this is now fixed, or we need to do more. Edouard, would you have chance to
> test in Nightly again?

It has nothing to do with this.  External protocols are something else than moz-extension.  But anyway, good to know it's not fixed ;)

(In reply to Andy McKay [:andym] from comment #9)
> Honza, could you give some feedback on Luca's proposed patch, we aren't sure
> if its the right strategy.

I'll forward to Jason Duell, that was behind the jar/app channel magic.  I'm not the best person for this, my area are more just (from) HTTP redirects.

Comment 11

[Jason Duell]

I've started looking at this.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Jason, is there any update on this?

Comment 13

[Valentin Gosu [:valentin] (he/him)]

I don't know the code very well but I will try to take a look tomorrow.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Support redirect to moz-extension:// URLs

MozReview-Commit-ID: 3sJvFmk7tG7

Comment 15

[Valentin Gosu [:valentin] (he/him)]

I rebased the patch on top of m-c, and tried the extension in comment 0, and it works!
It just needs to be reviewed by someone who is aware of the security implications.

Comment 16

[(no longer active)]

Comment on attachment 8803187 [details] [diff] [review]
Support redirect to moz-extension:// URLs

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

You should add tests for this, both for the redirection issue, and also for the docshell load change you're implementing.

::: docshell/base/nsDocShell.cpp
@@ +6745,5 @@
> +      rv = aps->ExtensionURILoadableByAnyone(aURI, &loadableByAnyone);
> +
> +      if (NS_SUCCEEDED(rv) && loadableByAnyone) {
> +        rv = LoadURI(aURI, aLoadInfo, nsIWebNavigation::LOAD_FLAGS_NONE,
> +                     aFirstParty);

I'm not sure if I understand the docshell changes.  If these checks pass, you're permitting the *original* load to proceed (since you're just passing aURI) so you aren't really redirecting anything.  Also, you're clobbering the load flags passed in and forcing LOAD_FLAGS_NONE, which sounds wrong.

But even more importantly, what's this trying to do?  If I read this code correctly, we'll happily also load moz-extension URLs that aren't loadable by anyone, since we won't enter this branch, end up returning false, which makes the caller proceed as normal.

I guess this page describes the behavior we want to implement here? <https://developer.chrome.com/extensions/manifest/web_accessible_resources>

That page is talking about "navigation from a web origin to an extension resource" but your change here affects *all* docshell loads, including navigating from places such as about:preferences, and also the case where the user manually types in the moz-extension:// URI.

I think it's best to break this part off into a separate patch, with some explanation about what this code is trying to do.

::: docshell/base/nsDocShell.h
@@ +727,5 @@
>    // Convenience method for getting our parent docshell. Can return null
>    already_AddRefed<nsDocShell> GetParentDocshell();
>  
> +  // Check if the url is a valid addon redirect url, it it is listend in the
> +  // addon web_accesible_resources returns true, false otherwise.

Looks like the wording of this sentence got scrambled somehow?

::: dom/security/nsContentSecurityManager.cpp
@@ +491,5 @@
>    aNewChannel->GetOriginalURI(getter_AddRefs(newOriginalURI));
>  
>    NS_ENSURE_STATE(oldPrincipal && newURI && newOriginalURI);
>  
> +  bool equals;

Nit: please initialize to false.

@@ +492,5 @@
>  
>    NS_ENSURE_STATE(oldPrincipal && newURI && newOriginalURI);
>  
> +  bool equals;
> +  bool loadableByAnyone = false;

Nit: this can be hoisted down into the |if (aps)| block.

::: modules/libjar/nsJARChannel.cpp
@@ +965,5 @@
> +    NS_ENSURE_ARG_POINTER(aURI);
> +
> +    nsAutoCString scheme;
> +    aURI->GetScheme(scheme);
> +    if (!scheme.EqualsLiteral("moz-extension")) {

Nit: please use nsIURI::SchemeIs() instead.  That's a tiny bit more efficient, and also more grep'able.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp
@@ +18,5 @@
>  #include "nsIStreamConverterService.h"
>  #include "nsIPipe.h"
>  #include "nsNetUtil.h"
>  #include "LoadInfo.h"
> +#include "nsJARChannel.h"

You can just #include "nsIJARChannel.h" instead.

@@ +122,5 @@
> +  nsCOMPtr<nsIURI> newURI;
> +  (*result)->GetURI(getter_AddRefs(newURI));
> +
> +  nsAutoCString scheme;
> +  newURI->GetScheme(scheme);

Nit: you can also use SchemeIs() here.

@@ +130,5 @@
> +  if (scheme.EqualsLiteral("jar")) {
> +    nsCOMPtr<nsIURI> originalURI;
> +    (*result)->GetOriginalURI(getter_AddRefs(originalURI));
> +
> +    nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(*result, &rv);

Hmm, I'm not sure why you wrote the code this way.

Wouldn't it be simpler to QI to nsIJARChannel first, and then just call SetMozExtensionURI?  Do we actually need to check the scheme as well?

@@ +132,5 @@
> +    (*result)->GetOriginalURI(getter_AddRefs(originalURI));
> +
> +    nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(*result, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    jarChannel->SetMozExtensionURI(originalURI);

Nit: please check the return value as this method is fallible.

@@ +135,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    jarChannel->SetMozExtensionURI(originalURI);
> +  } else if (scheme.EqualsLiteral("file")) {
> +    // TODO: how to support the override of the channel URI when
> +    // the resolved scheme is file?

I'm guessing you're not planing to do this here.

But should we add something similar to nsIFileChannel as well?  And if yes, why not move the SetMozExtensionURI() method to a new interface, nsIChannelWithMozExtensionURI and have both types of channels implement that?

::: netwerk/protocol/res/moz.build
@@ +21,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
>  LOCAL_INCLUDES += [
> +    '/modules/libjar',

And you shouldn't need this then.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Attachment: Support redirect to moz-extension:// URLs

MozReview-Commit-ID: 3sJvFmk7tG7

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Thanks for the review Ehsan. It turns out changes to the docshell and nsContentSecurityManager weren't even needed, so that's great :)

Luca, do you think you could help me with a few things?
1. How do I write a test for this? Do we have something similar in the tree? Or could you write up something really quick?
2. In what circumstances would a moz-extension URL map to a file URI/channel? It's easy to implement those changes as well, but I'd like to first have some tests to make sure it actually does what it's supposed to.

As it stands, the patch fixes the issue in comment 0.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8803572 [details] [diff] [review]
Support redirect to moz-extension:// URLs

CC Patrick for necko review.

Is there any interest in getting this landed with just manual testing?

Comment 20

[Andy McKay]

(In reply to Valentin Gosu [:valentin] from comment #19)
> Is there any interest in getting this landed with just manual testing?

Would much rather have a test, even if it's in the webextensions part of the code base to be sure this doesn't regress.

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 8803572 [details] [diff] [review]
Support redirect to moz-extension:// URLs

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

I'm ok with the actual necko code here.. but I'm going to ask christoph to look at this whole thing from a security perspective. I don't feel like I'm comfortable enough to just make the call.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

Here's a test that fails without the fix for moz-extension.  I haven't applied that patch yet to make sure it succeeds.

Comment 23

[Shane Caraveo (:mixedpuppy)]

I'm doing a full build to make sure the above test patch works.

Comment 24

[Adam Pieniazek]

I have an extension that exhibits the same behavior with the Pocket API that Edouard reported with the Trello API. If someone could point me at instructions on how to make a build with the patch I can test against my extension as well to confirm patch works as expected.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Adam, once I verify my test works I'll run this through the try server.  You'll be able to grab builds from that and verify with your addon.

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Valentin Gosu [:valentin] from comment #18)
> Luca, do you think you could help me with a few things?
> 1. How do I write a test for this? Do we have something similar in the tree?
> Or could you write up something really quick?
> 2. In what circumstances would a moz-extension URL map to a file
> URI/channel? It's easy to implement those changes as well, but I'd like to
> first have some tests to make sure it actually does what it's supposed to.
> 
> As it stands, the patch fixes the issue in comment 0.

Hi Valentin,
sorry for the late response (and thanks a lot to Shane for having prepared and attached the initial test file to this issue):

1. the test that Shane has attached is a great start (I can't say much about tests written in the netwerk/ dir, but I totally agree with Andy and Shane that we should at least add a test for it in the webextension tests), I would add to that test file the following additional test cases:
  - a redirect to a extension url that is not in the web_accessible_resources should fail
  - query params should be preserved (because they are usually used to pass the token to the target extension pages)
  - the final URL schema should be "moz-extension" (and not a jar or file schema)

2. a moz-extension URL maps to a file when the extension is installed as a directory instead of a packed xpi file (and so if possible, the above test should run in both modes: when the addon is installed as an xpi and when it is installed as a directory)

Comment 27

[Shane Caraveo (:mixedpuppy)]

I'm not certain yet if its the test environment, or something I'm not doing right in a test, but I cannot get the redirect to work with the patch applied.  I still get the "Corrupted Content Error" page.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

Ok, here is a test patch that can be applied.  It has two tests, nearly identical.  The first expects a failure since web_accessible_resources is not set (it passes).  The second sets web_accessible_resources (and also adds the file that should be landed on), however it still fails the redirect.

I haven't bothered covering more test cases as the simplest is not working.

rpl: do the tests look correct?  The sjs file is doing a 302 to the provided url, which in this case is a moz-extension url.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Addendum. I patched in the CSP/DocShell parts that were removed, and the above test passes with a redirect.  So the reviewed patch is not enough, but I'm not certain if the other parts are the right way to do things or not.

Comment 30

[Shane Caraveo (:mixedpuppy)]

Valentin, you mentioned that the changes in docshell weren't necessary, however my testing last night I could not get a passing test with out it.  Could you verify and give me STR so I can see what might be different?

Comment 31

[Valentin Gosu [:valentin] (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #30)
> Valentin, you mentioned that the changes in docshell weren't necessary,
> however my testing last night I could not get a passing test with out it. 
> Could you verify and give me STR so I can see what might be different?

Since I didn't have a test, I used the extension in comment 0 to verify, and the redirect worked with just the necko bits of the patch.

Comment 32

[Shane Caraveo (:mixedpuppy)]

I tried that addon but the panel wouldn't stay open for me to interact with it.

Comment 33

[Valentin Gosu [:valentin] (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> I tried that addon but the panel wouldn't stay open for me to interact with
> it.

It doesn't work until you go to the settings page in about:addons, and login to trello. That's the part that needs a redirect.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Ok, I fixed the addon by swapping the order of tab creation and window close here:
https://github.com/eoger/bug2trello/blob/firefox/js/popup.js#L307
When I click the toolbarbutton it now opens a tab for me to continue authorizing.

I still get the corrupted content error (after removing the docshell/csp patches and rebuilding).

With the addon installed via about:debugging (not using xpi) I end up with a file uri, so I think we'll need to handle that.  This is the error in console.

Security Error: Content at https://trello.com/1/token/approve may not load or link to file:///Users/shanec
/tmp/bug2trello/settings.html#token=....

If I install the xpi normally the error happens but no message in the console.  I do see the 302 response with location header moz-extension://d2b09eaa-6a06-5248-bb7d-f6a6bb059607/settings.html#token=...

Comment 35

[Boris Zbarsky [:bzbarsky]]

> and the oauth popup is pointed to the "resolved" jar/file urls

Is that because someone is using GetURI on a channel when they should be looking at the "final channel URI" (which takes the originalURI into account as needed)?

Or is the issue that in the case of a redirect target the "final channel URI" can no longer take into account the originalURI set on the post-redirect channel because HTTP clobbers it with the pre-redirect URI and sets the "redirected channel" flag?

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8803572 [details] [diff] [review]
Support redirect to moz-extension:// URLs

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

Sorry for the lag of response and even more sorry for letting you wait to now tell you I don't feel comfortable enough reviewing that code. Code itself looks good but my background on jarChannels and moz-extensions is not solid enough to factor the security implications of such a change. Sorry :-(

Comment 37

[Shane Caraveo (:mixedpuppy)]

After reading through bug 1256122 a couple times I'm pretty certain this will end up being duped to it.  The primary fix is likely bug 1319111.

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: testredirect

This patch is updated to add a redirect test that happens from a webrequest listener.

The first test passes as expected (no redirect allowed).
The second fails.
The third also fails, with a security error unless nsContentSecurityManager.cpp is patched (next attachment), but even then it simply hangs without a redirect.

The jar patch as-is and previously reviewed has no affect on redirects.

Comment 39

[Shane Caraveo (:mixedpuppy)]

Attachment: nsContentSecurityManager support for moz-ext

This patch adds a check for moz-ext and allows the redirect if the addon manifest has specified the final url in the web_accessible_resources list.

Comment 40

[Jason Duell]

Shane: does this still look like a dupe of bug 1256122 to you?  If so, would your patch here be helpful?

Comment 41

[Shane Caraveo (:mixedpuppy)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #40)
> Shane: does this still look like a dupe of bug 1256122 to you?  If so, would
> your patch here be helpful?

A test in attachment 8863958 [details] [diff] [review] should cover the issue from bug 1256122 (I included it on purpose for that reason).  The change (or other/better fix if someone has one) for nsContentSecurityManager would probably also be necessary for bug 1256122.

Comment 42

[Boris Zbarsky [:bzbarsky]]

The right fix here is probably just to fix bug 1256122 comment 47 last paragraph in nsScriptSecurityManager::AsyncOnChannelRedirect.

Comment 43

[Shane Caraveo (:mixedpuppy)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #42)
> The right fix here is probably just to fix bug 1256122 comment 47 last
> paragraph in nsScriptSecurityManager::AsyncOnChannelRedirect.

Do you mean in nsContentSecurityManager?  I modified it to handle web accessible moz-ext urls in attachment 8863960 [details] [diff] [review], that only gets past that particular issue, but is not a full fix.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> Do you mean in nsContentSecurityManager?

Yes, I do.

> I modified it to handle web accessible moz-ext urls in attachment 8863960 [details] [diff] [review]

Right, I'm saying I think that modification is wrong and the right fix is what's described in bug 1256122 comment 47 towards the end.

Comment 45

[Edouard Oger [:eoger]]

Is the bug blocking webextensions? I'm a bit concerned it may fall under the radar.

Comment 46

[Andy McKay]

No worries, its not falling off the radar at all. Just a complicated set of dependencies underneath it.

Comment 47

[Shane Caraveo (:mixedpuppy)]

I'm just going to dup this since the fix for this issue is in bug 1380186