mirror of
https://github.com/mozilla/gecko-dev.git
synced 2024-10-19 08:15:31 +00:00
d6b900089c
resource://android URIs always point to a "jar:" URI so it doesn't make sense that the returned URL object implements nsIFileURL. This also makes it so extensions can be loaded from a resource://android URI. Audited all places where we use `nsIJARURI` and check for places where we assume it looks like `jar:`: the only (!) place where we do that is here: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/dom/xhr/XMLHttpRequestMainThread.cpp#1852 Where we have special handling for `jar:` URIs. It looks like we would have less special handling for such a request to a `resource://android` and it could be fixed by just checking for the interface instead, but that's what's already happening today so it should work. That code is never reached for `resource://android` URIs as `mIsMappedArrayBuffer` is false for those URIs (see #2822). And the code is consistent in checking for the scheme instead of the interface (the other check is here: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/dom/xhr/XMLHttpRequestMainThread.cpp#2822) Audited all places where we do `EqualsLiteral("jar")`: https://searchfox.org/mozilla-central/search?q=.EqualsLiteral%28%22jar%22%29&path= `SubstituteRemoteChannel`: looks interesting, but uses `nsISubstitutingProtocolHandler::ResolveURI` to first get the real URI (which is a `jar:`) so it works for our case. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/protocol/res/ExtensionProtocolHandler.cpp#414 `SubstitutingProtocolHandler.cpp` This case is explicitly fixed by this change, making it account for both `"jar"` and `"resource"`. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#299 `ReadSourceFromFileName`: this also looks interesting, but uses the channel to get the URI which returns the real `"jar"` URI and not the mapped `"resource"`. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/js/xpconnect/src/XPCJSRuntime.cpp#2837 `nsStringBundle.cpp` Accounts for both `"jar"` and `"resource"`, so it should work the same. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/intl/strres/nsStringBundle.cpp#437 Audited all places where we use `nsINestedURI` to make sure they would work for a `nsIJARURI` which does not implement `nsINestedURI`. `BrowserContentHandler.jsm` Uses `nsINestedURI` to figure out if a URI is local or not and then it checks whether it's a `"file"`, `"resource"` or `"chrome"` URI, for a `nsIJARURI & nsINestedURI` it would return a `"file"` which passes the test, for a `nsIJARURI` alone it would return `"resource"` which is also considered local by this code, so the result wouldn't change. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/browser/components/BrowserContentHandler.jsm#395-399 `nsScriptSecurityManager.cpp` `GetOriginFromURI`: This is the reason why `SubstitutingJARURI` doesn't implement `nsINestedURI`, the origin is computed starting from the innermost URI, which in our case would be a file. The behavior in our case is that the origin from a `resource://android` URI behaves like other `resource://` URIs, which is what we expect. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/caps/nsScriptSecurityManager.cpp#169 `CheckLoadURIWithPrincipal`: for `nsIJARURI & nsINestedURI` this code will only allow pages from the same jar to be in the same origin (which is correct), for `nsIJARURI` this code is unreachable and it would allow every `resource://android` to load every other `resource://android` URI (which is essentially the same thing). https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/caps/nsScriptSecurityManager.cpp#874-876 `nsDocShell.cpp` `DisplayLoadError`: Just looping through the nested URI chain to build an error message, no concerns here (it would just ignore the `jar:` part, which is fine). https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/docshell/base/nsDocShell.cpp#3986 `DoURILoad`: Looking for `view-source`, no concerns for `resource://android`. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/docshell/base/nsDocShell.cpp#9645 `nsObjectLoadingContent.cpp` Also looking for `view-source`, no concerns. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/dom/base/nsObjectLoadingContent.cpp#2004 `nsIconURI.cpp`/`nsIconURI.h` Exposing `nsINestedURI`. No concerns. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/image/decoders/icon/nsIconURI.cpp#58 `nsJARURI.cpp`/`nsJARURI.h` Exposing `nsINestedURI`, the subject of this audit. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/modules/libjar/nsJARURI.cpp#45 `nsIOService.cpp` `URIChainHasFlags`: This code looks at the chain of nested URIs to figure out if the chain of protocols has a certain flags. for `nsIJARURI & nsINestedURI` it would look at both `jar:` and `file:` protocols, while in `nsIJARURI` it would only look at the `resource:` protocol, which is our intention, since we want this URI to be treated like a `resource:` URI and nothing else. Note the `resource:` URI is always local (enforced by `NewURI`), so this should be ok. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/base/nsIOService.cpp#1494 `nsNetUtil.cpp`/`nsNetUtil.h` Implementation of `NS_ImplGetInnermostURI`, which wouldn't work for `nsIJARURI` alone, as expected. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/base/nsNetUtil.h#704 `nsSimpleNestedURI.cpp`/`nsSimpleNestedURI.h` Implementing `nsINestedURI` for `nsSimpleNestedURI`, no concerns. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/base/nsSimpleNestedURI.cpp#19 `nsViewSourceHandler.cpp` Looking at `view-source` inner URI to get the flags, no concerns. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/protocol/viewsource/nsViewSourceHandler.cpp#49 `nsHtml5StreamParser.cpp`/`nsHtml5TreeOpExecutor.cpp` More `view-source` handling code. No concerns. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/parser/html/nsHtml5StreamParser.cpp#310 https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/parser/html/nsHtml5TreeOpExecutor.cpp#884 `DownloadPlatform.cpp` `IsURLPossiblyFromWeb`: This line is unreachable as the method would return true because resource has `URI_IS_UI_RESOURCE`. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/components/downloads/DownloadPlatform.cpp#314 `getAnonymizedPath`: This code looks a little scary, and it's the first one that seems to conflate the fact that `jar: == nsINestedURI`. On the android case (line 2130) this code manually replaces the `resource://android` URI with a `jar:` URI, so it wouldn't matter whether `resource://android` implements `nsINestedURI` or not. Actually this code could be a lot easier by using `nsISubstitutingURL::resolveURI`, maybe I should open a bug. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/components/search/SearchService.jsm#2148 `getRemoteTypeForURIObject`: This also looks interesting. The switch at line 157 would go straight to line 218 for `resource:` URIs (irrespective of `nsINestedURI`) and then for `nsINestedURI` we would look at the `jar:` URI (and eventually the `file:` URI). While for not `nsINestedURI` we would call straight to `validatedWebRemoteType`. This might return `WEB_REMOTE_TYPE` instead of `FILE_REMOTE_TYPE`, but since it's already happening it should be ok (`resource://android` maps _today_ to a `jar:` file that return `WEB_RETURN_TYPE`) https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/modules/E10SUtils.jsm#150 `getURIHost`: This is another piece of code that benefits from not implementing `nsINestedURI` as the host would be correctly `"android"` instead of the apk path. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/mozapps/downloads/DownloadUtils.jsm#390 `initDialog`: Download dialog would show the `resource://android` URI instead of the actual `jar:` URI, kind of expected. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/toolkit/mozapps/downloads/HelperAppDlg.jsm#423 There are no places in the codebase where we do `== "jar"`. Looks like I've already looked at all hits for "jar" too. Checked for `"jar:` too and It looks like they are all from code that tries to build a `jar:` URI manually which is fine for this change. Audited all `schemeIs("jar")` occurrences: https://searchfox.org/mozilla-central/search?q=schemeIs(%22jar%22)&path= `browser-identity.js` Uses the channel URI which is always resolved to `jar:`, so that works regardless. See also: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#229 https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/browser/base/content/browser-siteIdentity.js#812 `tabbrowser.js` Only for `scheme == "about"` URIs. https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/browser/base/content/tabbrowser.js#789 `HelperAppDialog.js` Treats "jar:" and "resource" the same way. https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/mobile/android/components/HelperAppDialog.js#63 `WebNavigationContent.js` This code checks if the scheme is "jar" and if the original URI is "resource" it will use that instead, so in our case it will use the "resource" URI either way. https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/toolkit/modules/addons/WebNavigationContent.js#158 Depends on D18740 Differential Revision: https://phabricator.services.mozilla.com/D16913 --HG-- extra : moz-landing-system : lando |
||
|---|---|---|
| .. | ||
| components.conf | ||
| moz.build | ||
| nsNetCID.h | ||
| nsNetModule.cpp | ||
| nsNetModule.h | ||