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

Event listeners in Content Scripts and communication between main and isolated CS worlds #241

Open
bershanskiy opened this issue Jul 16, 2022 · 5 comments

Comments

@bershanskiy
Copy link
Member

bershanskiy commented Jul 16, 2022

Background

Currently, extension content scripts add event listeners on DOM nodes for two purposes:

  • actually handle DOM events (like button clicks)
  • communication of JS injected into main world JS with isolated world JS:
    isolated world adds and event listener, say on document.body and and main-world dispatches this listener, say with document.body.dispatchEvent(new CustomEvent('custom_message', {detail: 'Hello World!'}))

These are two fundamentally different use cases. Developers are probably already performing some validation on messages sent via the second method. However, I observed (anecdotal evidence) that some developers inject extension UIs into pages and do not realize that page can dispatch events automatically without user interaction. Lack of validation is, of course, mostly a failure on extension developers' side, but I believe user agents can also fix it.

Proposal

Proposal (breaking change): main-world JS should not be able to trigger event listeners added in isolated world. This will break the second use case, however developers can switch to "official" messaging methods like runtime.sendMessage and runtime.onMessage and make extensions "externally connectable".

I'm sure this proposal is possible to implement because Developer Tools already are able to distinguish between event listeners added in two worlds and display only main-world ones (if I recall correctly, I was able to see these event listeners before).

@bershanskiy bershanskiy changed the title Event listeners in Content Scripts and communica Event listeners in Content Scripts and communication between main and isolated CS worlds Jul 16, 2022
@bershanskiy
Copy link
Member Author

Here is a test extension which demonstrates the first use case and reacts to events created by the page itself:

manifest.json

{
  "name": "Test",
  "version": "1.0",
  "manifest_version": 3,
  "background": {
    "service_worker": "background.js"
  },
  "action": {},
  "content_scripts": [
    {
      "matches": [
        "<all_urls>"
      ],
      "js": [
        "cs.js"
      ],
      "run_at": "document_end",
      "match_about_blank": true
    }
  ]
}

background.js:

chrome.runtime.onMessage.addListener(console.log);

cs.js

function listener(message) {
  return () => chrome.runtime.sendMessage(`click ${message}`);
}

document.addEventListener('click', listener('regular'));

const button = document.createElement('button');
button.innerText = 'HELLO';
button.addEventListener('click', listener('button'));
document.body.appendChild(button);

@theseanl
Copy link

This will break the second use case, however developers can switch to "official" messaging methods like runtime.sendMessage and runtime.onMessage and make extensions "externally connectable".

Just want to add that this can't address all use cases, because adding a domain as externally connectable allows any scripts in the domain to communicate with the extension, but extensions mostly wants to only communicate to first-party scripts.

@bershanskiy
Copy link
Member Author

Just want to add that this can't address all use cases, because adding a domain as externally connectable allows any scripts in the domain to communicate with the extension, but extensions mostly wants to only communicate to first-party scripts.

This is a valid point. Also, extensions currently can not specify very generic patterns in externally connectable patterns. Also, in MV3 extensions can change content scripts (register/unregister them) while externally connectable patterns are static (specified only in manifest.

@Rob--W
Copy link
Member

Rob--W commented Jul 21, 2022

event.isTrusted can already be used if developers really want to distinguish user-initiated events from "artificially" synthesized events. But even with event.isTrusted, the extension cannot know for sure that the user intentionally triggered the event for the expected reason. Hostile pages could change the appearance of the UI element (e.g. button) to mislead the user.

Extensions that interface through web pages should be designed to work well with a hostile web page. Critical extension UI should not share the same DOM as the web page.

On the topic of communication between the web page and content scripts, several discussions have happened before in the WECG: see #57, #77, #78, #79 and the meeting notes linked to these issues (and also today's meeting notes linked to this issue).

This issue prescribed a solution that is a backwards-incompatible change, without compelling reasons to support such a big change. The mentioned scenario's are incomplete: besides the two use cases (DOM events and extension communication), extensions may be interested in subscribing to custom events from the web page itself (e.g. I have written extensions that subscribes to custom events on YouTube to detect website-specific state transitions).

@tophf
Copy link

tophf commented Sep 12, 2022

This proposal will also break Tampermonkey and all the other userscript engines, which use CustomEvent messages for synchronous communication, unachievable via chrome.runtime messaging. They also use MouseEvent to pass DOM nodes, which is otherwise impossible between worlds. On the other hand, it was a bad solution anyway, and a better one must be implemented, also synchronous, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants