Closed Bug 1246125 Opened 8 years ago Closed 8 years ago

Fix "moz-extensions:" URLs reported as "jar:" URLs in webNavigation events

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Whiteboard: [webNavigation][berlin])

Attachments

(1 file)

frame details collected from the webNavigation events contain jar urls instead of webextension urls:

    "jar:file:///tmp/generated-extension-13.xpi!/subframe.html"

STR (using the test case from Bug 1190685):

- apply the patches from Bug 1190685
- uncomment the assertion which checks the URLs collected using getAllFrames with the URLs collected from the webNavigation events, in the test case file[1]:

     // Bug-XXX: moz-extension url are resolved as jar urls in webNavigation events
     is(getAllFramesDetail.url, collected.url, "url");

- run the test case:

     ./mach mochitest browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js

Expected behaviour:
- the two URLs should be equal and the assertion passes

Actual behaviour:
- assertion fails because the URLs collected from the navigation events are the corresponding "jar:" URLs of the original "moz-extension:" URLs

[1]: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js
Whiteboard: [webNavigation]
In the webNavigation events the url is retrieved using the nsIChannel[1], e.g.:

    onStateChange: function onStateChange(webProgress, request, stateFlags, status) {
      let data = {
        requestURL: request.QueryInterface(Ci.nsIChannel).URI.spec,

and the reason of this issue (and its solution) seems to be described in the nsIChannel.idl[2]

    /** 
     *... 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. ...
     * ...
     */
    attribute nsIURI originalURI;

So the solution seems to be simple, we should be able to use the originalURI to retrieve the original "moz-extension:" URI (e.g. using originalURI if originalURI is an extension URI and the URI is a jar URI)

[1]: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/toolkit/modules/addons/WebNavigationContent.js#52 and 
  https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebNavigationContent.js#67
[2]: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannel.idl#46
Whiteboard: [webNavigation] → [webNavigation][good first bug]
I think we're going to have to be careful about how we handle this...

We should handle it properly for internal resolution of things like resource: and moz-extension: URLs, but we need to be careful to use the final URL in HTTP redirections, rather than the initial URL.
Whiteboard: [webNavigation][good first bug] → [webNavigation]
(In reply to Kris Maglione [:kmag] from comment #2)
> I think we're going to have to be careful about how we handle this...
> 
> We should handle it properly for internal resolution of things like
> resource: and moz-extension: URLs, but we need to be careful to use the
> final URL in HTTP redirections, rather than the initial URL.

completely agree (and it is what I meant with "if originalURI is an extension URI" above)

another thing that I was thinking about is that an URL in the form "jar:file:///tmp/generated-extension-13.xpi!/subframe.html" is disclosing the local paths of the user (which is definitely something that we don't want to happen).

so, yeah, we definitely need to be extra extra careful and think about other URL schemas that could do the same (like "resource:", and "chrome:"?), and this is enough to turn it in a "not a very good first bug"

I'm not even completely sure that it would not be reasonable to completely filter out any webNavigation event with a "resource:", "chrome:" or "jar:" URL (and what I mean here is "completely drop any webNavigation events about pages that shouldn't be accessible or visible to addons"), would it be too much restrictive? (Probably this is something to discuss in a separate bugzilla issue.)
Whiteboard: [webNavigation] → [webNavigation][berlin]
Blocks: 1190329
Attachment #8738973 - Flags: review?(gkrizsanits)
Attachment #8738973 - Flags: feedback?(mwein)
The attached patch includes a new test file which has assertions only on the URLs which are supposed to be always reported with their original URI (currently "about", "chrome", "resource" and the "moz-extension" URI schemes) instead of the resolved one.

The HTTP redirect scenario (which is supposed to report the URI resolved during the redirect) is tested in Attachment 8739239 [details] in Bug 1256652, along side the server_redirect transitionQualifier.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/44995/#review41797

This looks good.  The comments I made are all stylistic.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webnavigation_resolved_urls.html:22
(Diff revision 1)
> +  function backgroundScript() {
> +    let checkURLs;
> +    let eventIdx = 0;
> +
> +    browser.webNavigation.onCompleted.addListener((msg) => {
> +      if (eventIdx < checkURLs.length) {

I think you could remove the need for the `eventIdx` variable:

if (checkURLs.length > 0) {
  let URL = checkURLs.shift();
}

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webnavigation_resolved_urls.html:45
(Diff revision 1)
> +    manifest: {
> +      permissions: [
> +        "webNavigation",
> +      ],
> +    },
> +    background: `(${backgroundScript})()`,

Stringifying the background script looks hacky to me - could we avoid doing it unless we need to pass information in from the outer scope?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webnavigation_resolved_urls.html:48
(Diff revision 1)
> +      ],
> +    },
> +    background: `(${backgroundScript})()`,
> +    files: {
> +      "tab.html": `<!DOCTYPE html>
> +      <html>

Please tab this under `<!DOCTYPE html>`.
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44995/diff/1-2/
Attachment #8738973 - Flags: review?(gkrizsanits)
Attachment #8738973 - Flags: feedback?(mwein)
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

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

> I think you could remove the need for the `eventIdx` variable:
> 
> if (checkURLs.length > 0) {
>   let URL = checkURLs.shift();
> }

I like it, tweak applied!

> Stringifying the background script looks hacky to me - could we avoid doing it unless we need to pass information in from the outer scope?

Unfortunately it seems that only tests in the browser.ini group can pass the background script as a function, if we try to do it in tests in the mochitest.ini or chrome.ini groups, the backgound function turns into an undefined when it reachs [generateXPI](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#937), for this reason all the background scripts in the "toolkit/components/tests/mochitest/" are stringified before the extension data goes through the SpecialPowers API:

- https://dxr.mozilla.org/mozilla-central/search?q=%22background%3A%22+path%3Atoolkit%2Fcomponents%2Fextensions%2F&redirect=false&case=false

Anyway, even if this group of tests needs to stringify the background function, it would be nice to always use the same convention, maybe the one with lighter syntax (how about the `"new " + backgroundScript` flavor?)

> Please tab this under `<!DOCTYPE html>`.

tabbed (not under the doctype, but like done in other existent tests, eg. in [test_ext_storage_tab.html](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_storage_tab.html#92))
Attachment #8738973 - Flags: review?(gkrizsanits)
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

https://reviewboard.mozilla.org/r/44995/#review42281

::: toolkit/modules/addons/WebNavigationContent.js:59
(Diff revision 3)
> +    if (originalURI.schemeIs("about") ||
> +        originalURI.schemeIs("chrome") ||
> +        originalURI.schemeIs("resource") ||
> +        originalURI.schemeIs("moz-extension")) {
> +      locationURI = originalURI;
> +    }

Could you please assert the scheme of the locationURI too here and add some comment about why are we doing this? Not really for security concerns just to make it obvious for readers what are we trying to achieve here.
Attachment #8738973 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44995/diff/3-4/
Attachment #8738973 - Attachment description: MozReview Request: Bug 1246125 - webNavigation events should not resolve local special schemes into file/jar URLs. → MozReview Request: Bug 1246125 - webNavigation events should not resolve local special schemes into file/jar URLs. r=gabor
https://reviewboard.mozilla.org/r/44995/#review42281

> Could you please assert the scheme of the locationURI too here and add some comment about why are we doing this? Not really for security concerns just to make it obvious for readers what are we trying to achieve here.

In the last updated version of this patch (besides adding a inline comment to explain this change):
the locationURI is first checked for "jar" or "file" URI schemes, and only in that case the originalURI scheme is evaluated and eventually assigned to the locationURI (if the originalURI scheme is one of the expected one).

is this what you mean above with the "assert the scheme of the locationuRI" part of the comment?
Iteration: --- → 48.3 - Apr 25
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44995/diff/4-5/
Attachment #8738973 - Attachment description: MozReview Request: Bug 1246125 - webNavigation events should not resolve local special schemes into file/jar URLs. r=gabor → MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa
The last push contains the previous patch rebased on the refactoring which is going to be introduced by the patch attached to Bug 1262794.
Depends on: 1262794
Comment on attachment 8738973 [details]
MozReview Request: Bug 1246125 - [webext] webNavigation events should not resolve local special schemes into file/jar URLs. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44995/diff/5-6/
(In reply to Luca Greco [:rpl] from comment #15)
> Comment on attachment 8738973 [details]
> MozReview Request: Bug 1246125 - [webext] webNavigation events should not
> resolve local special schemes into file/jar URLs. r=krizsa
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/44995/diff/5-6/

patch rebased on the last fx-team tip.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/170eb8db5f8c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.