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

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

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.

(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.)

- chrome, resource, about and moz-extension URLs should not be resolved into file or jar URLs Review commit: https://reviewboard.mozilla.org/r/44995/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44995/

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.

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/

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))

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.

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/

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?

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/

The last push contains the previous patch rebased on the refactoring which is going to be introduced by the patch attached to Bug 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.