Closed
Bug 1276048
Opened 9 years ago
Closed 7 years ago
302 response fails to redirect to moz-extension:// page
Categories
(WebExtensions :: Request Handling, defect, P1)
WebExtensions
Request Handling
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1380186
webextensions | + |
People
(Reporter: eoger, Assigned: mixedpuppy)
References
Details
(Whiteboard: triaged)
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Priority: -- → P1
Whiteboard: triaged
Comment 1•9 years ago
|
||
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?
Flags: needinfo?(jduell.mcbugs)
Comment 2•8 years ago
|
||
Honza, can you look at this?
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Comment 3•8 years ago
|
||
Edouard, can you reproduce when you turn of e10s (multiprocess)? If not, then this is duplicate of bug 1277028.
Flags: needinfo?(honzab.moz) → needinfo?(edouard.oger)
Comment 5•8 years ago
|
||
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•8 years ago
|
||
The patch from the experiment described above.
Comment 7•8 years ago
|
||
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?
Flags: needinfo?(edouard.oger)
Reporter | ||
Comment 8•8 years ago
|
||
Not working in today's nightly, sorry.
Flags: needinfo?(edouard.oger)
Comment 9•8 years ago
|
||
Honza, could you give some feedback on Luca's proposed patch, we aren't sure if its the right strategy.
Flags: needinfo?(honzab.moz)
Comment 10•8 years ago
|
||
(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.
Flags: needinfo?(honzab.moz) → needinfo?(jduell.mcbugs)
Comment 11•8 years ago
|
||
I've started looking at this.
Assignee: nobody → jduell.mcbugs
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Comment 12•8 years ago
|
||
Jason, is there any update on this?
Flags: needinfo?(jduell.mcbugs)
Comment 13•8 years ago
|
||
I don't know the code very well but I will try to take a look tomorrow.
Assignee: jduell.mcbugs → valentin.gosu
Flags: needinfo?(jduell.mcbugs)
Whiteboard: triaged → [necko-active], triaged
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Comment 14•8 years ago
|
||
MozReview-Commit-ID: 3sJvFmk7tG7
Updated•8 years ago
|
Attachment #8760340 -
Attachment is obsolete: true
Comment 15•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8803187 -
Flags: review?(ehsan)
Comment 16•8 years ago
|
||
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.
Attachment #8803187 -
Flags: review?(ehsan) → review-
Comment 17•8 years ago
|
||
MozReview-Commit-ID: 3sJvFmk7tG7
Updated•8 years ago
|
Attachment #8803187 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
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.
Flags: needinfo?(lgreco)
Comment 19•8 years ago
|
||
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?
Attachment #8803572 -
Flags: review?(mcmanus)
Comment 20•8 years ago
|
||
(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•8 years ago
|
||
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.
Attachment #8803572 -
Flags: review?(mcmanus)
Attachment #8803572 -
Flags: review?(ckerschb)
Attachment #8803572 -
Flags: review+
Assignee | ||
Comment 22•8 years ago
|
||
Here's a test that fails without the fix for moz-extension. I haven't applied that patch yet to make sure it succeeds.
Assignee | ||
Comment 23•8 years ago
|
||
I'm doing a full build to make sure the above test patch works.
Flags: needinfo?(lgreco)
Comment 24•8 years ago
|
||
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.
Assignee | ||
Comment 25•8 years ago
|
||
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•8 years ago
|
||
(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)
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
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.
Attachment #8805230 -
Attachment is obsolete: true
Flags: needinfo?(lgreco)
Assignee | ||
Comment 29•8 years ago
|
||
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.
Assignee | ||
Comment 30•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(valentin.gosu)
Comment 31•8 years ago
|
||
(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.
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 32•8 years ago
|
||
I tried that addon but the panel wouldn't stay open for me to interact with it.
Comment 33•8 years ago
|
||
(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.
Assignee | ||
Comment 34•8 years ago
|
||
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•8 years ago
|
||
> 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•8 years ago
|
||
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 :-(
Attachment #8803572 -
Flags: review?(ckerschb)
Updated•8 years ago
|
Updated•8 years ago
|
webextensions: --- → +
Assignee | ||
Comment 37•8 years ago
|
||
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.
Depends on: 1256122
Updated•8 years ago
|
Assignee: valentin.gosu → nobody
Whiteboard: [necko-active], triaged → triaged
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mixedpuppy
Assignee | ||
Comment 38•8 years ago
|
||
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.
Attachment #8805397 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
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•8 years ago
|
||
Shane: does this still look like a dupe of bug 1256122 to you? If so, would your patch here be helpful?
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 41•8 years ago
|
||
(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.
Flags: needinfo?(mixedpuppy)
Assignee | ||
Updated•8 years ago
|
Comment 42•8 years ago
|
||
The right fix here is probably just to fix bug 1256122 comment 47 last paragraph in nsScriptSecurityManager::AsyncOnChannelRedirect.
Assignee | ||
Comment 43•8 years ago
|
||
(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.
Flags: needinfo?(bzbarsky)
Comment 44•8 years ago
|
||
> 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.
Flags: needinfo?(bzbarsky)
Updated•7 years ago
|
Flags: needinfo?(lgreco)
Reporter | ||
Comment 45•7 years ago
|
||
Is the bug blocking webextensions? I'm a bit concerned it may fall under the radar.
Comment 46•7 years ago
|
||
No worries, its not falling off the radar at all. Just a complicated set of dependencies underneath it.
Assignee | ||
Comment 47•7 years ago
|
||
I'm just going to dup this since the fix for this issue is in bug 1380186
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•