Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency: runtime.getURL() #281

Open
carlosjeurissen opened this issue Sep 26, 2022 · 7 comments
Open

Inconsistency: runtime.getURL() #281

carlosjeurissen opened this issue Sep 26, 2022 · 7 comments
Labels
implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified

Comments

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Sep 26, 2022

Background

During TPAC we discussed our intentions to start writing spec and tackle browser inconsistencies. We proposed to tackle a small chunks of API hoping that after some time all the APIs which are out there right now have been discussed and documented properly.

To start, off, inspired by #270 and #273, I want to tackle runtime.getURL() as it is a very basic API, has simple syntax and is supposed to be non ambiguous. This API is / was previously (also) available as extension.getURL().

Definition

In short, the idea behind runtime.getURL() is to get the URL of a resource which sits within the extension.

Defined by the Chrome Docs:
Converts a relative path within an app/extension install directory to a fully-qualified URL.

And by MDN: Given a relative path from the manifest.json to a resource packaged with the extension, return a fully-qualified URL.

The API takes a single argument being the path, which is supposed to be a string. Generally speaking the API returns the origin of the extension and adds the path to it. If the argument starts with a slash, the slash is excluded in the response.

from runtime.json:

{
  "name": "getURL",
  "type": "function",
  "nocompile": true,
  "description": "Converts a relative path within an app/extension install directory to a fully-qualified URL.",
  "parameters": [
    {
      "type": "string",
      "name": "path",
      "description": "A path to a resource within an app/extension expressed relative to its install directory."
    }
  ],
  "returns": {
    "type": "string",
    "description": "The fully-qualified URL to the resource."
  }
}

Going forward

We should determine what the right behaviour for each inconsistency listed below is. And start writing down an exact description on how the algorithm should handle edge cases.

In addition, we should determine when to ship those resolutions. Can they be updated for manifest v2 and v3, or do some need to wait till v4. Some could lead to breaking changes, while others do not have real use cases right now and can be shipped without much trouble.

Inconsistencies

As simple as the API might look, after doing intensive testing with more than 1000 test cases, it turns out there exists many inconsistencies across browsers. To make it easy to go over them one by one, I have grouped the inconsistencies and described how each browser is behaving and my proposed preferred behaviour. In below examples, sometimes the protocol browser-extension is being used as generic protocol. Read it as chrome-extension in Chrome, moz-extension in Firefox and safari-web-extension in Safari. This makes it easier to compare what is returned by the APIs.

Inconsistency 1: passing non-string parameter (Safari will match others)

Example: runtime.getURL(200);
Chrome and Firefox currently throw an exception when a non-string parameter is passed to getURL.
Safari currently attempts to call .toString() and proceeds without throwing an exception. If however .toString() fails, for example when passing null or undefined, Safari will throw an exception just like Chrome and Firefox.

Inconsistency 2: passing lesser known utf-8 characters (Safari will match others)

Example: runtime.getURL('Ω');
Safari currently returns null when the string passed contains characters like emoji and the greek alphabet.
Chrome and Firefox handle these characters just fine.

Seems this is a Safari issue and should be solved.

Inconsistency 3: passing a full external URL

Example: runtime.getURL('https://www.example.com');
Chrome prefixes the URL with the extension origin as such:
browser-extension://$extension_uuid/https://www.example.com/
Firefox and Safari just return the full URL:
https://www.example.com/

Prefixing the URL with the extension origin seems more true to the definition of the API and can potentially reduce attack surfaces. An exception and edge-case to this is inconsistency #4, see below.

Inconsistency 4: passing a full extension-owned URL

Example: runtime.getURL('browser-extension://$extension_uuid/');
Chrome prefixes the URL with the extension origin:
browser-extension://$extension_uuid/browser-extension://$extension_uuid/
Firefox and Safari just return the full URL:
browser-extension://$extension_uuid/

The behaviour of Firefox and Safari can be more useful. Yet the Chrome behaviour would align more with how full external URLs are also handled.

Inconsistency 5: passing URL which is exactly //

Example: runtime.getURL('//');
Chrome returns browser-extension://$extension_uuid//
Firefox returns browser-extension://
Safari returns browser-extension://$extension_uuid/

Inconsistency 6: passing URL which starts with or is three slashes ///

Example: runtime.getURL('///');
Chrome will prefix with extension origin except trailing slash: browser-extension://$extension_uuid///
Firefox will prefix with browser-extension:: browser-extension:///
Safari has same behaviour as Chrome yet removes two slashes from the start: browser-extension://$extension_uuid/

Inconsistency 7: passing URL which starts with exactly two slashes and at least one non-slash character

Example: runtime.getURL('//example');
Chrome will prefix with extension origin except trailing slash: browser-extension://$extension_uuid//example
Firefox and Safari will prefix with browser-extension:: browser-extension://example

Inconsistency 8: passing URL which is exactly .

Example: runtime.getURL('.');
Chrome returns browser-extension://$extension_uuid/.
Firefox and Safari returns browser-extension://$extension_uuid/

Inconsistency 9: passing URL which starts with ./

Background: ./ in URLs generally starts resolving URLs
Example: runtime.getURL('././/example');
Chrome will prefix with extension origin: browser-extension://$extension_uuid/././/example
Firefox and Safari will remove as many ./ prefixes as exists, skips the removal of the first slash, and prefixes it with the extension origin: browser-extension://$extension_uuid//example"

Inconsistency 10: passing URL which contains ../

Background: ../ in URLs generally lets you go up one directory
Example: runtime.getURL('../../example/..//test/');
Chrome will not do anything special: browser-extension://$extension_uuid/../../example/..//test/
Firefox and Safari will resolve the ../, which in the above example means: browser-extension://$extension_uuid//test/

Inconsistency 11: Safari ../ artifact edge-cases (Safari will match Firefox)

When resolving the ../ in URLs Safari sometimes returns exactly browser-extension://$extension_uuid/../. Example input paths are:
../, /../, ../././, ./.././, .././, ..//../, ././../, /./../, /../../, ../../, ./../, .././., ../../., ./../., ../., ../a/.., ..//..

And sometimes it returns exactly browser-extension://$extension_uuid/... Example input paths are:
.., /.., ./.., ../.., /../.., ./../.., a/../.., /./.., ././.., .././.., /././..

While some just return browser-extension://$extension_uuid/. Example paths are:
/.././, /.././., /./../., /../.

Bonus for those who can decode the logic for inconsistency 11.

@carlosjeurissen carlosjeurissen added inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified agenda Discuss in future meetings labels Sep 26, 2022
@xeenon
Copy link
Collaborator

xeenon commented Sep 27, 2022

This is great @carlosjeurissen! A lot of the Safari (and Firefox) behaviors boil down to us returning an absolute resolved URL, that tries to proactively resolve .. and . in the path and stay inside the extension's allowed resources. For example ../../example/..//test/ would break out of the extension's resources and try to access other files on the system. Chrome must handle this at another layer, hence the inconsistencies.

I agree case 11 is odd!

@Jack-Works
Copy link

Thanks for your investigation!

Inconsistency 1: Agree to throw on non-string arguments.

Inconsistency 2: I agree this looks like a Safari issue.

Inconsistency 3: passing a full external URL
Inconsistency 4: passing a full extension-owned URL
Inconsistency 5: passing URL which is exactly //
Inconsistency 6: passing URL which starts with or is three slashes ///
Inconsistency 7: passing URL which starts with exactly two slashes and at least one non-slash character

For those 5 cases, I believe it should act as the following:

  • keep the URL if it is already full-qualified.
  • check if the result is of the same origin as the current extension, if not, throw an error.

Example:

getURL('/') // browser-extension://extension1/
getURL('http://example.com') // error
getURL('browser-extension://extension1/file') // return same string
getURL('browser-extension://extension2/file') // error
getURL('//') // error
getURL('///') // error
getURL('//extension1/file') // browser-extension://extension1/file
getURL('//extension2/file') // error

Inconsistency 8: Is . a valid file name in an extension?
Inconsistency 9 and 10: this API should normalize the URL (Firefox and Safari does), and make sure the origin is kept.

@carlosjeurissen
Copy link
Contributor Author

@xeenon You mentioned you will follow up with the Safari related items. What inconsistencies would this cover? I assume 1, 2 and 11?

This leaves the other ones to be discussed on how to handle them.

@Jack-Works thanks for your proposals! Seems like this approach makes a lot of sense. You have my support! As for the ., not sure, probably not. Yet same can be asked about ///. Seems removing the dot makes most sense here as it can be read as ./.

@xeenon
Copy link
Collaborator

xeenon commented Oct 7, 2022

Yes, I am filing internal bugs about issue 1, 2, and 11.

@dotproto
Copy link
Member

In today's meeting there was consensus among browser vendors that our main concern with the return value of runtime.getURL() is whether the resulting URL string is processed as expected by the browser. If there are situations where inconsistencies in how different browsers process input strings results in a material challenge for extension developers targeting cross-browser use cases, we would like to dig more into those situations to understand the problems this causes for developers and whether browser changes are necessary and appropriate.

Additionally, @Rob--W and @xeenon expressed a desire to investigate Inconsistency 3. I think we may also want to align on how to handle Inconsistency 4, as the differences in this case may cause an implementation to work in one browser but not another.

@Rob--W
Copy link
Member

Rob--W commented Oct 13, 2022

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1795082 for Firefox's handling of getURL with regards to inconsistency 3.

@carlosjeurissen carlosjeurissen removed the agenda Discuss in future meetings label Oct 17, 2022
@Rob--W
Copy link
Member

Rob--W commented Aug 7, 2024

Firefox has fixed the inconsistencies in Firefox 130 (https://bugzilla.mozilla.org/show_bug.cgi?id=1795082). The URL normalization has been removed, and I included almost every test case here in unit tests: https://searchfox.org/mozilla-central/rev/c4966e1c1e6a8cb38c15a27693e8ecd63082055e/toolkit/components/extensions/test/xpcshell/test_ext_geturl.js#21-70

@Rob--W Rob--W added the implemented: firefox Implemented in Firefox label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified
Projects
None yet
Development

No branches or pull requests

5 participants