Closed
Bug 1371551
Opened 7 years ago
Closed 7 years ago
Relative imports in WebExtensions use wrong url and cause security error
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: Fallen, Assigned: evilpie)
References
Details
(Keywords: triage-deferred)
Attachments
(3 files)
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
baku
:
review+
jonco
:
feedback+
|
Details | Diff | Splinter Review |
In a WebExtension, I have a background page that loads a <script type="module">. This works fine (pref is enabled). Now when importing a module in that script using:
import { check } from "./imported.js"
I get a security error because it is trying to load from a file:// url instead of a moz-extension:// url.
Security Error: Content at moz-extension://baf998c3-e9d6-a242-bd5c-afeac3192105/background/background.html may not load or link to file:///Users/kewisch/mozilla/extensions/test-es6-module/background/imported.js.
See attached extension for a test case.
Reporter | ||
Updated•7 years ago
|
Attachment #8876023 -
Attachment mime type: application/x-xpinstall → application/octet-stream
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
I am not totally sure if this correct, but it works. Instead of using file: we want to use the original moz-extension: protocol.
Assignee | ||
Updated•7 years ago
|
Attachment #8937437 -
Flags: review?(bugs)
Comment 2•7 years ago
|
||
Could you explain the patch?
Assignee | ||
Comment 3•7 years ago
|
||
I don't have any great explanation, but basically see https://searchfox.org/mozilla-central/source/netwerk/base/nsIChannel.idl#36.
> This is used in
> * the case of a redirect or URI "resolution" (e.g. resolving a
> * resource: URI to a file: URI) so that the original pre-redirect
> * URI can still be obtained. This is never null. Attempts to
> * set it to null must throw.
In this case nsIChannel.URI is "file:///Users/kewisch/mozilla/extensions/test-es6-module/background/imported.js", because that is how we resolved "moz-extension://baf998c3-e9d6-a242-bd5c-afeac3192105/background/background.html". Extension however may not load file:// URIs directly, and must go through moz-extension://. originalURI is still ""moz-extension://baf998c3-e9d6-a242-bd5c-afeac3192105/background/background.html", i.e. the URI before we resolved it to actual "file" URI where the extensions' file is stored.
Assignee | ||
Comment 4•7 years ago
|
||
I am not sure if the "redirect" case would be an issue here.
Assignee | ||
Comment 5•7 years ago
|
||
Maybe the safest option would be so something like:
if (!originalURI.SchemeIs("moz-extension"))
channel->GetURI(getter_AddRefs(request->mBaseURL));
Comment 6•7 years ago
|
||
Comment on attachment 8937437 [details] [diff] [review]
Use channel originalURI instead of URI
I think jonco should review this first.
Could someone point to the relevant spec defining what the base uri should be?
Is channel's uri the right, or should it be script element's document's base uri or something?
Attachment #8937437 -
Flags: review?(bugs) → review?(jcoppeard)
Comment 7•7 years ago
|
||
Comment on attachment 8937437 [details] [diff] [review]
Use channel originalURI instead of URI
Review of attachment 8937437 [details] [diff] [review]:
-----------------------------------------------------------------
The base URL is set to the response URL, so it's like this intentionally. The relevant part of the spec is:
https://html.spec.whatwg.org/#fetching-scripts:concept-script-base-url
Attachment #8937437 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 8•7 years ago
|
||
Thanks for linking the relevant spec. So what about this more targeted fix for WebExtensions, I don't think that should break anything else?
Assignee: nobody → evilpies
Attachment #8937757 -
Flags: review?(jcoppeard)
Comment 9•7 years ago
|
||
Comment on attachment 8937757 [details] [diff] [review]
Use channel originalURI instead of URI only for webextensions
Review of attachment 8937757 [details] [diff] [review]:
-----------------------------------------------------------------
Fine by me.
Attachment #8937757 -
Flags: review?(jcoppeard)
Attachment #8937757 -
Flags: review?(amarchesini)
Attachment #8937757 -
Flags: feedback+
Comment 11•7 years ago
|
||
Comment on attachment 8937757 [details] [diff] [review]
Use channel originalURI instead of URI only for webextensions
Review of attachment 8937757 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/script/ScriptLoader.cpp
@@ +3048,5 @@
> return NS_ERROR_FAILURE;
> }
>
> + nsCOMPtr<nsIURI> uri;
> + channel->GetOriginalURI(getter_AddRefs(uri));
in theory, this can return an error. just do:
rv = channel->GetOriginalURI(getter_AddRefs(uri));
NS_ENSURE_SUCCESS(rv, rv);
@@ +3052,5 @@
> + channel->GetOriginalURI(getter_AddRefs(uri));
> +
> + // Fixup moz-extension URIs, because the channel URI points to file:,
> + // which won't be allowed to load.
> + bool isWebExt;
isWebExt = false;
Attachment #8937757 -
Flags: review?(amarchesini) → review+
Comment 12•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c3034830d9
Make ES6 modules work for webextension URLs. r=baku
Comment 13•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•