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

registerContentScripts misleadingly names page scripts as content scripts #313

Closed
tophf opened this issue Oct 30, 2022 · 17 comments
Closed
Labels
opposed: chrome Opposed by Chrome opposed: firefox Opposed by Firefox opposed: safari Opposed by Safari

Comments

@tophf
Copy link

tophf commented Oct 30, 2022

chrome.scripting.registerContentScripts, getRegisteredContentScripts, updateContentScripts, unregisterContentScripts all misleadingly describe the scripts injected into the MAIN world as content scripts - both in the name of the method and in their synopsis. The API and the documentation misleads developers into thinking that the MAIN-world script has the same security guarantees as a normal content script, which it doesn't, and exposes the same chrome API, which it doesn't either.

A script in MAIN world is not a content script.
A content script is a script running in the isolated world both historically and by design for the extra layer of security.

Suggesting to drop Content from the names and ensure "register" is present in all names (updateContentScripts was missing it). It will also align with the main executeScript method, which never contained "content" even though it always injected only content scripts (there was no world parameter in MV2).

current suggested
registerContentScripts registerScripts
getRegisteredContentScripts getRegisteredScripts
updateContentScripts updateRegisteredScripts
unregisterContentScripts unregisterScripts

The browser should probably print a deprecation message each time the old names are used, and remove them in ~1 year.

Additionally, each browser's documentation repository is encouraged to add a short notice about the distinction and security implications of running code in a possibly poisoned/spoofed environment (see also https://crbug.com/1261964), maybe with a link to a more detailed article.

@xeenon
Copy link
Collaborator

xeenon commented Dec 8, 2022

I don't think this is a meaningful change to make at this point. "ContentScript" is slightly inaccurate now, but it is still "content".

@tophf
Copy link
Author

tophf commented Dec 8, 2022

It's completely inaccurate, not slightly. MV3 is still in the early stages, it's bugged all around, its API is evolving, now is the time to fix it. It's a meaningful change that fixes a mistake made by developers who apparently weren't working on the extension API previously and which wasn't caught by a more experienced developer.

A script in the main page world is not a content script in the terms of the extension API. Content scripts are processed differently in many aspects e.g. network requests when making a POST/PUT have Origin header, only the isolated world can access blob:chrome-extension:// URLs created in the context of an extension origin via URL.createObjectURL, only isolated world provides guarantees for the JS environment being un-spoofed/un-poisoned. There's a number of other differences.

Note that the problem is not semantics per se, but the fact that the name "Content" in the methods creates false expectations because for ~10 years it meant a script in the isolated world that has quite a few critical differences to the main world.

A content script still means a "script in the isolated world".

@EmiliaPaz
Copy link
Contributor

"content scripts" do not mean "isolated world scripts"; they mean "scripts running on the context of a page" (which is true for both isolated world and main world scripts). ISOLATED is still the default world, so anyone injecting in the MAIN world needs to explicitly select that. Additionally, Chrome is likely to support the world specification in the manifest-provided content scripts (and we shouldn't separate those or just name the field "scripts").

We should provide better documentation on this, specifically fix the link you mentioned

@tophf
Copy link
Author

tophf commented Dec 8, 2022

"content scripts" do not mean "isolated world scripts";

It's what this term meant for ~10 years.

"scripts running on the context of a page"

This is a generic introduction, not the definition.

The problem is not semantics but false expectations and misleading developers into making their extensions unsafe.

@tophf
Copy link
Author

tophf commented Dec 8, 2022

I guess if you fix every piece of the documentation to link to an article that lists dangers of running code in the MAIN world it'll be fine. Primarily the danger of spoofing of standard JS methods and prototypes to intercept and exfiltrate data from such scripts.

@Rob--W Rob--W added opposed: chrome Opposed by Chrome opposed: firefox Opposed by Firefox and removed agenda Discuss in future meetings labels Dec 8, 2022
@Rob--W
Copy link
Member

Rob--W commented Dec 8, 2022

We discussed this topic in the meeting today (https://github.com/w3c/webextensions/blob/main/_minutes/2022-12-08-wecg.md) and did not see value in the churn associated with renaming the API.

I agree that there are dangers associated with running code in the page's context, but that issue is not due to the name of the API. The scripting API documentation should call out the danger of running code in the main world.

Side note: Renaming the scripts to be more generic, from registerContentScripts to registerScripts would add more confusion than to reduce it. At least with "content scripts", it's more clear that the script runs on top of web content. With generic "registerScripts", there is a lot more ambiguity over what script is being registered.

@Rob--W Rob--W closed this as completed Dec 8, 2022
@tophf
Copy link
Author

tophf commented Dec 8, 2022

Indeed, looking back I see the underlying problem that I tried to convey is the undocumented change of the behavior not the name per se. Every browser's documentation should clearly and prominently emphasize the dangers of MAIN world injection and do it more convincingly than the current MDN article to frighten the developers properly e.g. show or at least mention how trivial it is to hack into any webpack-produced bundle by overriding Object.defineProperty and Function.prototype.call.

@Rob--W
Copy link
Member

Rob--W commented Dec 8, 2022

emphasize the dangers of MAIN world injection and do it more convincingly than the current MDN article to frighten the developers properly

That page on MDN features the following prominent warning. If that's not sufficiently convincing, then I wonder what you're looking for.

Warning: As an extension developer you should consider that scripts running in arbitrary web pages are hostile code whose aim is to steal the user's personal information, damage their computer, or attack them in some other way.

The isolation between content scripts and scripts loaded by web pages is intended to make it more difficult for hostile web pages to do this.

Since the techniques described in this section break down that isolation, they are inherently dangerous and should be used with great care.

@tophf
Copy link
Author

tophf commented Dec 8, 2022

This warning isn't frightening at all because it looks like a generic "don't do this do this" proclamation on the same level of urgency as a warning about an experimental API. I guessed this problem may be solved by showing an example that instantly conveys how trivial it is for a web page to hook into almost any JS code.

@Rob--W
Copy link
Member

Rob--W commented Dec 8, 2022

This warning isn't frightening at all because it looks like a generic "don't do this do this" proclamation on the same level of urgency as a warning about an experimental API.

With that mindset no amount of documentation is sufficient to stop the development of insecurely implemented extensions.

I guessed this problem may be solved by showing an example that instantly conveys how trivial it is for a web page to hook into almost any JS code.

There are numerous mentions on that page that explicitly describes why working with page objects is unreliable. If someone ignores all these warnings and caveats, then I think that a more likely outcome of an "example" is for an unskilled reader to at most write one counter-defense against that specific example and have a false sense of security while being ignorant about other aspects.

There is hardly a way to run code in an untrusted environment and get fully reliable results.

@tophf
Copy link
Author

tophf commented Dec 8, 2022

With that mindset
If someone ignores all these warnings and caveats

It's a practical mindset. I'd say it's shared by the majority of programmers. People who solve practical focused tasks and don't work on web platform specifications with caveats.

As a practical programmer who has seen thousands of cases where people make mistakes regardless of how many times the documentation warns against it verbally, I hope that adding an example of how spoofing/interception works would help people understand the problem better.

An example/MCVE/repro always helps.

@tophf
Copy link
Author

tophf commented Dec 8, 2022

There is hardly a way to run code in an untrusted environment and get fully reliable results.

This is what Violentmonkey/Tampemonkey do currently by extracting safe methods from an iframe and any extension can do the same. In the future ShadowRealm might help, assuming its constructor is unforgeable.

This is impossible in MV3 though as its CSP for the isolated world doesn't allow creating script elements with inline code.

@tophf
Copy link
Author

tophf commented Dec 9, 2022

After reading the meeting notes it seems to me everyone considered this problem to be about naming/bikeshedding whereas it's actually about a much bigger and more dangerous change of security guarantees that subverts established behavior expectations of developers for the past 10 years. I'll probably open a new issue...

@tophf
Copy link
Author

tophf commented Dec 9, 2022

  • Chrome devtools shows "Content scripts" panel only for scripts in the isolated world.

  • chrome.devtools.inspectedWindow uses useContentScriptContext parameter that indicates the isolated world.
    How do you propose to fix it?

This is yet another evidence that the term "content script" has an established meaning of a script in the isolated world. Over the last ~10 years it's been used in like a million of places in code, forums and sites like stackoverflow, apps, UI, so the destructive impact of this change is enormous and the benefits are non-existent. The only benefit of diminishing/disregarding the problem I see is that Chromium team doesn't have to lose face for not thinking this through.

@Sxderp
Copy link

Sxderp commented Jan 10, 2023

I just want to point out that I've been confused in some messages from Chrome devs when they've said "content scripts". Coming form Firefox I've always thought that meant "privileged". Apparently that's not the case in the Chrome world as that term can mean "running in the page context / access level" (MAIN ?)

Common terminology would be nice..

@Rob--W
Copy link
Member

Rob--W commented Jan 15, 2023

Prior to the introduction of world: "MAIN", content scripts were unambiguously referring to extension scripts in an isolated world (in Chromium). With the support for world: "MAIN" (in scripting.registerContentScripts, scripting.executeScript and content_scripts in manifest.json), this is no longer true. The issue is not with "content scripts" as a term, but the overloading of it with the introduction of world: "MAIN". The alternatives would have been to rename content_scripts or scripting.registerContentScripts, but that was not ideal either. So at this point I suppose that the best that we can do is to emphasize the lack of separation / privileges (in documentation) when world: "MAIN" is used in the content script APIs.

@Rob--W
Copy link
Member

Rob--W commented Feb 10, 2023

FYI: I've submitted a PR to document the world parameter of the scripting API on MDN at mdn/content#24316 . This includes an unambiguous and specific warning about the danger of running scripts in the MAIN world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
opposed: chrome Opposed by Chrome opposed: firefox Opposed by Firefox opposed: safari Opposed by Safari
Projects
None yet
Development

No branches or pull requests

6 participants