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

[Proposal] Add EventTarget.getEventListeners() #412

Open
LeaVerou opened this issue Feb 16, 2017 · 90 comments
Open

[Proposal] Add EventTarget.getEventListeners() #412

LeaVerou opened this issue Feb 16, 2017 · 90 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events

Comments

@LeaVerou
Copy link

Originally posted at WICG: https://discourse.wicg.io/t/proposal-eventtarget-prototype-geteventlisteners/2015

Purpose: This would provide a way to get all event listeners added to an element via addEventListener.

Signature: element.getEventListeners([type])

Returns: Array of objects. Each object contains properties for type, callback, options for the arguments that registered the corresponding event listener. Events registered via inline event handlers are not included.

Use cases: This would enable removing events based on arbitrary criteria, instead of requiring a reference to the callback, which causes unnecessary couplings. Typically libraries deal with this by providing their own abstractions for adding events that track the listeners manually. However, this is fragile, as it means listeners not registered via the library cannot be retrieved or removed. Some libraries deal with this by hijacking addEventListener to keep track of listeners, but this is very intrusive for a library and it doesn't help with any listeners registered before the library was included. Browsers already keep track of event listeners, so it should be relatively easy to expose them, and is on par with the Extensible Web Manifesto principle of exposing browser "magic" via JS APIs.

@domenic
Copy link
Member

domenic commented Feb 16, 2017

On balance I think this is probably worth it, but we should note the solid objections to this on a few grounds:

  • This opens up the ability for APIs (e.g. custom element APIs) to change their behavior based on the presence or absence of an event listener, which is very bad design. Adding event listeners should be side-effect free.
    • On the other hand, this opens up the ability for libraries to perform unobservable optimizations as side effects of listeners being present, if they are careful.
  • This opens up the ability to unregister listeners which you did not register, and thus interfere with other parts of your codebase or other code running on the same page in hard-to-debug ways. The current model has an "object capability" design wherein you can more easily reason about the system. (Note: this is not a security boundary, just a reason-about-code-better boundary, analogous to how you can't resolve or reject a promise unless you are the one that created it.)

I think both of these objections are solid, but as I said, on balance I think they are overcome. The fact that people are hacking around the current design so much, and that this abstraction appears in other platforms (e.g. jQuery's system, or Node.js's EventEmitter) implies to me it's probably better to expose.

It's probable that this subject has been discussed before, maybe pre-GitHub; I wonder if we could find those threads to see how the discussion went then and if anything has changed.

Events registered via inline event handlers are not included.

This is weird, but I see the issue; currently the actual listener function is not reified, and it would be unclear what removeEventListener("event", reifiedListener) does (does the content attribute or IDL attribute stick around?).

Browsers already keep track of event listeners, so it should be relatively easy to expose them, and is on par with the Extensible Web Manifesto principle of exposing browser "magic" via JS APIs.

I think this is a misinterpretation of the situation. Browsers don't have the magic ability to look at all event listeners; just like user code, they only have the ability to dispatch events. The actual event listener list is usually stored as a private variable in the EventTarget backing class, with only equivalents of addEventListener/dispatchEvent exposed to the rest of the codebase. You could always modify a browser to make that private variable public, but that's the same as what we're discussing here for the web. So there's no browser-only magic we're exposing here; this would be a purely new capability, not a piece of bedrock we're unearthing.

@LeaVerou
Copy link
Author

LeaVerou commented Feb 16, 2017

All good points there, thanks for the thoughtful response @domenic!

On another note, another possible design would be to have the same signature as addEventListener with all three arguments (type, callback, options) which would act as filters for which listeners to return. I suspect if the only supported filter is type, the rest will be emulated by developers via .filter() on the returned array.

@annevk
Copy link
Member

annevk commented Feb 17, 2017

Someone should probably research https://lists.w3.org/Archives/Public/www-dom/ as I recall this coming up quite a few times in the past and being dismissed each time. I don't recall much else unfortunately.

@LeaVerou
Copy link
Author

This is what I could find:
https://lists.w3.org/Archives/Public/www-dom/2012AprJun/0131.html
https://lists.w3.org/Archives/Public/public-whatwg-archive/2012Jun/0157.html
https://lists.w3.org/Archives/Public/www-dom/2014JulSep/0029.html

At first glance, it appears the pushback is entirely comprised of the theoretical purity arguments that @domenic mentioned would come up. No implementation difficulties or anything of the like.

Tab mentioned an alternative in the last thread: Allowing an option for namespacing and removing events via namespaces, with both strings and symbols allowed. This is a good idea but does not solve all use cases. For example, a major use case is cloning an element with its events. In that case you legitimately want access to listeners you did not bind.

@annevk
Copy link
Member

annevk commented Feb 17, 2017

I also found https://lists.w3.org/Archives/Public/public-webapi/2008Apr/thread.html#msg66 that isn't very conclusive either way, mostly folks not convinced by the use cases given at the time. (Although allowing iteration over them is somewhat novel I suppose.)

(The identifier grouping idea is #208.)

@domenic
Copy link
Member

domenic commented Feb 17, 2017

I don't think "theoretical purity" is a fair characterization of those arguments. They are practical arguments from real developers who want to be able to develop components without being afraid that any other piece of code on the page could interfere. The lack of ability to reason about program control flow is an actual issue that affects developers every day, not a theoretical one.

@domenic
Copy link
Member

domenic commented Feb 17, 2017

For example, given the code for TacoButton here, you'd no longer be able to guarantee that all taco-buttons on the page properly forward enter/space keydowns to click events, or that they stop firing click events when disabled. Being unable to reason about your program in this way is a real burden, and it changes your story as a component author from "include my component and it'll work" to "include my component but make sure all your third-party scripts aren't doing some over-aggressive cleanup action or you'll see strange behavior".

I still lean toward exposing the functionality, and then you'll just have to accept that you can no longer write encapsulated components in that way, but I think it's a real tradeoff between two different types of developer convenience, not one between theoretical purity and developer convenience.

@ljharb
Copy link

ljharb commented Feb 17, 2017

It is an important security property that nobody without a reference to my listener function can remove my listeners.

If a use case is cloning a node with all listeners, perhaps this could be handled with a "copyListeners" API (taking event names as filters, eg) without the need to expose the listener references directly?

What about adding a new "addEventListener" option that opts the listener in to being unprotected? That way the default would remain the same - secure - but users who wanted to waive a bit of security for the convenience of jQuery-like event manipulation could do so?

@nilssolanki
Copy link

nilssolanki commented Feb 17, 2017

I like the idea of namespacing events; this way, I can remove all my handlers later on without needing to keep their references (which is very helpful when binding functions). As far as I understand, when using a Symbol as a namespace one could create a private namespace which could not be intercepted by third parties (which might be important for developers of third-party code).

Example:

(() => {
  const namespace = Symbol('eventNamespace');

  const myObj = {
    a: 'a',
    myFn() { console.log(this.a); },
  };

  function add() {
    document.addEventListener('click', myFn.bind(myObj), { namespace });
  }

  function remove() {
    document.removeEventListener('click', null, { namespace });
  }
})();

Combined with lhjarbs idea of a copyListeners API, this should probably cover most of the use-cases?

@domenic
Copy link
Member

domenic commented Feb 17, 2017

It is an important security property that nobody without a reference to my listener function can remove my listeners.

To be clear, it is not a security property, because on the web our threat model is not meant to defend against malicious code running on the same page. It is purely a developer-reasoning-about-code property.

@LeaVerou
Copy link
Author

Regardless of the characterization of these arguments, it's a false sense of security, since this is possible today by wrapping addEventListener. There are libraries that do this. However, I think we both agree that there's a multitude of reasons why this is better done natively than having a library hijack addEventListener, right?

@ljharb
Copy link

ljharb commented Feb 17, 2017

It's certainly not an intentional security property, but that does not mean it is not an emergent one.

I can defend against wrapping addEventListener by caching it in first-run code.

I definitely agree it should be done natively - i'm simply suggesting that it be opt in, not by default.

@LeaVerou
Copy link
Author

I can defend against wrapping addEventListener by caching it in first-run code.

If you're writing a library, you don't control whether your library is first-run code, so tough luck. If you're not writing a library, then what's the point of defending anything? You control exactly what runs.

@ljharb
Copy link

ljharb commented Feb 17, 2017

I can extract an untainted addEventListener from an iframe, I can include in the documentation that it needs to be first-run, and often ad network code is ran - last - that I don't control. I don't think it's productive to try to make a claim that making code more robust is a fool's errand; whether it's part of the intentional security model of the web or not.

Is there a reason that an opt-in mechanism (at the addEventListener callsite, like how passive: true works) wouldn't be a good middle ground, where you could get the functionality you want with zero risk of breaking existing axioms, and zero risk of developers having difficulty reasoning about their code?

@LeaVerou
Copy link
Author

LeaVerou commented Feb 17, 2017

Is there a reason that an opt-in mechanism (at the addEventListener callsite, like how passive: true works) wouldn't be a good middle ground, where you could get the functionality you want with zero risk of breaking existing axioms, and zero risk of developers having difficulty reasoning about their code?

One of the most basic HCI (or human psychology in general) principles is that when something is opt-in, few do it. Most of the time because they simply never considered it. Therefore, it renders the feature useless for libraries, and only helps in reducing coupling in one's own code.

Another basic HCI principle is sensible defaults. Most listeners have no reason to be hidden, so the default should be non-private.

Opt-out might work, like a stealth option for listeners.

@markandrus
Copy link

markandrus commented Feb 17, 2017

@LeaVerou

Regardless of the characterization of these arguments, it's a false sense of security, since this is possible today by wrapping addEventListener. There are libraries that do this. However, I think we both agree that there's a multitude of reasons why this is better done natively than having a library hijack addEventListener, right?

Just my two cents here, but I disagree. As has been pointed out, once you can enumerate event listeners, you can implement something like Node's removeAllListeners, which warns

Note that it is bad practice to remove listeners added elsewhere in the code, particularly when the EventEmitter instance was created by some other component or module (e.g. sockets or file streams).

In practice, I've found this means that, as a library author, if you vend event-emitting objects to your users whose events you also use to drive internal functionality, you need to separate the EventEmitter interface your users will depend on from the one your internal functionality depends on (otherwise, a removeAllListeners call could break you, and it's annoying to document around…). I would not like to see the DOM APIs follow suit, and I would prefer encapsulation over convenience in this case.

EDIT: Sorry, @LeaVerou re-reading what you wrote, I too agree hijacking addEventListener is bad practice.

@LeaVerou
Copy link
Author

LeaVerou commented Feb 17, 2017

@markandrus The stealth option I mentioned above would solve this. Or namespaces, with a default namespace so that listeners bound without a namespace can still be retrieved. I assume UAs would also need this functionality, to protect their own internal listeners. Although I'm not sure why anyone would call removeAllListeners without wanting to, you know, remove all listeners.

@markandrus
Copy link

@LeaVerou a mechanism like stealth would be useful 👍 I am happy so long as there is at least an opt-out.

Although I'm not sure why anyone would call removeAllListeners without wanting to, you know, remove all listeners.

There are many different types of developers... I've heard bug reports from some who called this function expecting it to remove only "their" event listeners.

@ibash
Copy link

ibash commented Feb 17, 2017

Having overridden window.addEventListener to work around a browser bug, I would welcome this change as a way to make my code more obvious (get event listeners and remove where it makes sense) and more robust (don't have to have my code run first thing to have it work).

@FremyCompany
Copy link

If you want to copy your event listeners, you can redispatch your events on the old node. That doesn't require destroying the currently existing abstraction.

function redirect(event) {
  var ne = event.constructor(event.type, event);
  document.querySelector('input').dispatchEvent(ne);
}

https://jsfiddle.net/yrewbnes/

@annevk
Copy link
Member

annevk commented Feb 18, 2017

@FremyCompany that fiddle doesn't seem to work in Chrome/Firefox/Safari. You'll also lose trustedness of the event that way and target/currentTarget and such.

@phistuck
Copy link

@annevk - it seems to just be missing a new (new event.constructor(event.type, event);).

@dominiccooney
Copy link

From a purely implementation point of view I don't think this would be a terrible hassle to implement. Blink internally uses the same EventListener machinery for some internal use cases, like IndexedDB index population and media controls and things. We'd just need some level of filtering or segregating for them.

I'm not sure about window. Are there any times you have cross origin iframes and different domains get to stick event listeners on the same window through the window proxy or anything like that?

As a recovering? wannabe? web developer I'm mildly anxious about the code that's out there assuming their event listeners, once registered, won't be messed with. On the other hand if you can get a reference to the EventTarget anyway you probably can already cause interesting headaches.

@rianby64
Copy link

IMHO the only reason to getEventListeners() is because you want to clean up your own listeners after leaving. But, you can put a little effort in order to track manually your listeners. Also, @domenic said

... and it changes your story as a component author from "include my component and it'll work" to "include my component but make sure all your third-party scripts aren't doing some over-aggressive cleanup action or you'll see strange behavior"

For me, this statement has a lot of sense... So, getEventListeners() doesn't add a significant value to DOM.

@naugtur
Copy link

naugtur commented Feb 21, 2017

I've heard it many times. "If it's in the standard we're supposed to use it because it's the way to go, right?"

I think that's why everyone above is worried about it breaking abstractions.

Calling this method should not seen an obvious choice for a novice developer so that they put in some effort into managing their listeners instead of looping through the list and removing each one when they clean up.

How about making it a "static" method of the DOM node, so that it looks like Object.keys? I'm not sure if hurting discoverability of this feature is really helpful, but noone brought it up before.

Additionally, instead of taking this listing function as a solution for cloning nodes, it'd be cleaner to just have a method for cloning with events.
Having that available removes the main use case for getting all events. The other major use case is inspection, but that is already available on Dev tools. Might need improvements.

OK. Final thought.
If this goes in, please group the output by some categories that users could set, like "click.myown" to help people use it the right way.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 16, 2017
@LeaVerou
Copy link
Author

Another use case I came across yesterday: I was in a talk about an accessibility tool that automatically adds keyboard shortcuts to existing web apps. If they cannot read whether there are existing key* events on each command, they cannot avoid controls that already have keyboard shortcuts. (although this argues more for a declarative way to specify keyboard shortcuts than for this…)

@phistuck
Copy link

@LeaVerou - plus, having an event listener does not mean it supports keyboard shortcuts. accesskey exists for a declarative way.

@domenic
Copy link
Member

domenic commented Jun 25, 2017

Small thread hijack: I'd like to draw the attention of people in this thread to the related, but distinct, proposal in #208. Although it doesn't cover all use cases of this thread, it covers some of them, so the audience may be similar. If you work on a library or app that would use the proposal in #208, let us know over in that thread, as implementers are currently trying to judge developer interest.

@LeaVerou
Copy link
Author

LeaVerou commented Mar 9, 2022

Sorry if this has been mentioned already, but re:security and encapsulation concerns: but developers can already intercept addEventListener by doing something like

let addEventListener = EventTarget.prototype.addEventListener;
let removeEventListener = EventTarget.prototype.removeEventListener;

EventTarget.prototype.addEventListener = function(type, callback, options) {
	/* store args and then… */
	addEventListener.call(this, type, callback, options);
};

EventTarget.prototype.removeEventListener = function(type, callback, options) {
	/* remove from stored args and then… */
	removeEventListener.call(this, type, callback, options);
};

The problem is just that a) it's flimsy, since listeners may have been attached before the code was run and b) it's suboptimal to be overwriting built-ins like that.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

developers can already intercept addEventListener by doing something like

yes, it was mentioned, but on the other side, developers can secure nothing will ever interfere with their intent if your code runs earlier (see polyfills)

const {bind: b, call: c} = Function;
const {addEventListener} = EventTarget.prototype;
const addListener = b.call(c, addEventListener);

// any time later on ...
addListener(document.body, 'click', () => { console.log('safe'); });

We do this already in explicit circumstances to avoid evil scripts intercepting, removing, or mutating our listeners behaviors, but extensions can guarantee (in some browser) their code executes before anything else.

Again, I think this feature would be a good one for Web Extensions, at least to pave the path, but I also think user land can have a centralized library authors can use to retrieve all listeners, if added/remove through such centralized reference.

That would be also fine, like:

EventTarget.addPublicEventListener(element, listener, options);

Now, that would be cool ... changing a very long time expected behavior where many based their libraries around looks like an unnecessary, imho undesired, breaking change I hope won't land as amend of the current methods.

I would be more than OK making it land as new method, but I guess that will also fall into the "opt-in" is less effective than "opt-out", but then again, if this lands as default behavior, we (where I work) need to be sure we patch that default behavior and exclude any of our listeners from being exposed in the wild to undesired evil scripts.

@domenic
Copy link
Member

domenic commented Mar 9, 2022

Any data in the same process can be intercepted with Spectre; there is no security concern. If this is not something you're familiar with then it might be best to read up on that instead of sidetracking this issue. Early running scripts are not protection against that; there is simply no ability to keep secrets within the same process.

The issue is only about encapsulation, not security.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

I’m not sidetracking the issue, I’m talking about real world concern our product, and every similar product, has around leaked listeners.

I wasn’t aware Spectre can be exploited through JavaScript in a browser page and leak every part of the code, but if that’s the only concern around security then fine, let’s talk encapsulation, no reason to be aggressive though.

@LeaVerou
Copy link
Author

LeaVerou commented Mar 9, 2022

I don't think an opt-in new method would work (if the listener-adding code can follow the same "contract" as the listener-reading code they can just use a helper, most use cases for getEventListeners() are about doing stuff with listeners you did not attach yourself), but what about an opt-out? I'd wager that most event listeners exist to enable functionality and don't care one iota about encapsulation. For the ones that do, there could be something like addEventListener(eventName, callback, {closed: true}) (closed concept loosely inspired from open/closed shadow roots)

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

@LeaVerou like I've said, I love the stealth (or closed) idea, but I respectfully disagree on this part:

most event listeners exist to enable functionality and don't care one iota about encapsulation

and the reason is the opposite of:

most use cases for getEventListeners() are about doing stuff with listeners you did not attach yourself

with NodeJS you "own" the server that runs the code (or the lambda, or whatever) and you know for sure (™️) what script gets in and what not ... you don't have this open world where everyone can do whatever they wont because YOLO or because evil scripts (ads industry is full of these) you cannot control, because you ship one bit of a front end you might never control, as library author, while code used in a server has a different story/use-case for all listeners.

That being said, none of my libraries welcome a method that exposes internal listeners, mostly because I use extensively the event listener interface with hadnleEvent which is about attaching a whole state handler to an element, not just a callback, and that's proven to be extremely effective and handy.

lit-element, to name others, use this pattern too ... and so do tons of other frameworks. I never want my attached states/behaviors to ever leak to undesired scripts I didn't authorize, so I hope instead of telling me I am sidetracking the issue somebody would understand what is the issue I am talking about.

Yet, if this changes, which is a breaking expectations change to me, I need to add that third option everywhere, and say goodbye to legacy compatibility because once you add an object, old browsers take that as a truthy value and set the handler as capturing instead. This is a minor gotcha in 2022, yet something else some of my code needs to take care about (or polyfills around this requested braking change).

@benjamingr
Copy link
Member

benjamingr commented Mar 9, 2022

Let me make sure I am understanding the current situation:

  • There are encapsulation concerns which people agrees with. Adding this API does remove a current (implicit) guarantee that "regular" code cannot access the already registered event listeners.
  • Domenic also raised another very good concern early on - this enables building APIs that rely on an event listener being registered already which is typically not a good design.
  • There seems to be consensus about the use cases Lea raised in the initial post being valid and common. There is also agreement that the current way to accomplish this by altering EventTarget.prototype is hacky and imperfect.
  • The idea of having an opt in or opt out for listeners being discoverable by getEventListeners has been raised. People seem to agree that having an opt-in reduces the usability of the feature.

What about an alternative "enable encapsulation" options like the ones browsers typically use to enable breaking features like eval e.g. a header that restores the "no getEventListeners" behavior or an opt-in behavior so most websites can benefit from this but ones who care about this encapsulation cannot?

@benjamingr
Copy link
Member

Also @WebReflection note in Node.js we already shipped an API that lets you get event listeners to our native EventTarget implementation which is also the one we use for our AbortSignal and other APIs. It's a static one, to not violate the spec.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

@benjamingr

What about an alternative "enable encapsulation" options like the ones browsers typically use to enable breaking features like eval e.g. a header that restores the "no getEventListeners" behavior or an opt-in behavior so most websites can benefit from this but ones who care about this encapsulation cannot?

if this is about special CSP flag, then maybe it makes sense ... but I don't see CSP less cumbersome than opt-in, to be honest, yet it's close to my idea of "let's bring this to Web Extensions and see how it goes", so to me that's an option.

note in Node.js ...

as mentioned, Node.js is a different use case. It doesn't get evil scripts out of the box and it doesn't suffer by design XSS.

It's a static one, to not violate the spec.

If it's a new method I am not sure how that would violate the specs but again, in node you don't have DOM listeners to deal with, I am not fully sure why Node.js is so relevant in this breaking change for the Web (or the current state).

Let me make sure I am understanding the current situation:

Yes, all points reflect my understanding too.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

alternative how about an explicit API that allows to retrieve all listeners from X time on?

element.addEventListener('click', myOwnBusiness);

const listeners = element.observeListeners();
[...listeners]; // []

element.addEventListener('click', publicAffair);
[...listeners]; // [publicAffair]

or dare I say, something to implement through a mutation observer when listeners are added/removed? still with a possibility to opt-out from certain listeners (as in filterOut property) ... would this work better? (all ideas rushed just to have feedback)

@benjamingr
Copy link
Member

CSP can also be opt-out it doesn't have to be opt in (right?).

alternative how about an explicit API that allows to retrieve all listeners from X time on?

I think anything that doesn't intercept listeners that were there before my code is run is significantly diminished in value.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

CSP can also be opt-out it doesn't have to be opt in (right?).

that breaks everything by default, if you want to enable this by default and we should ask everyone to add a CSP rule in every site that used some library that most of them didn't even know was relying on non-leaking listeners.

At that point I'd personally think {closed} or {stealth} are more reasonable as JS dependencies are easier to update /patch than HTML layouts or server side headers ... yet, this is a breaking change.

@ljharb
Copy link

ljharb commented Mar 9, 2022

@benjamingr it is absolutely critical for encapsulation that listeners added before your code runs are not interceptable, for reasons discussed at length upthread.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

I guess I’m echoing @ljharb and yet another reason this feature scares me is custom elements suddenly unable to safely register their listeners in the constructor to properly work because "cowboy.js" might suddenly remove all listeners from a node … encapsulation and ownership should be preserved by default as its always been to date. The more I think about this, the more I think opt-in is the best way to move forward, as @ljharb suggested ages ago.

@strongholdmedia
Copy link

Me, personally, a won't fix, not because I don't understand the use case, simply because it's been too many years that libraries trust the fact listeners set privately or ASAP will never be reachaed or removed from the node.

This is simply not true.
As @LeaVerou noted above, it is well possible since ages to do this.

In fact, I also mentioned my "hijack library" (see @dorgaren/usable at npm) for this and similar uses above last year.
(It has a few issues remaining, that are to be fixed soon, the primary one being multiple registration, but as this not being self-promotion, this is not my focus here.)
Loading that at the beginning of a page (either by embedding it into a script on the very same page, or creating a 20-line browser addon that guarantees to load it before any page is loaded).

It makes it facile to retrieve and remove (that is the primary scope) event listeners attached by anyone to anything.

This very ability / behavior, I think, makes most points above by everyone about expectations around some sort of encapsulation, as well as security moot.

Any page relying on third party scripts could load something similar (either intentionally or "inadvertently", via hijacking, sandbox vulnerabilities, or any other method) to make this possible for such (or any other) third party scripts (or, as my intention was, for the "JS app" belonging to / being the "page" itself).

Would it be better if this has been implemented natively with some configurable security? Sure, I think.
Is relying on any such stuff for privacy, security, or expectation of actions to be taken / lack thereof unreasonable? Sure, I also think.

@ljharb
Copy link

ljharb commented Mar 9, 2022

@strongholdmedia that's only the case if your hijacking happens before the code meant to be encapsulated; being "first-run" code is the only way to have any sort of guarantees in JS. If event listeners are aded before your hijacking code runs, then it must remain impossible for you to observe them. It's totally fine (and impossible to prevent anyways) if later-running code is unable to prevent observation of its event listeners.

@WebReflection
Copy link

WebReflection commented Mar 9, 2022

@strongholdmedia what @ljharb said, but I also do that for a living, I wasn’t here commenting my concerns out of the blue, I use secured code and run it before others daily.

* secured as in JS secured, not Spectre

@bathos
Copy link

bathos commented Mar 10, 2022

wanna echo @ljharb cause his last comment hits on an extremely important general principle of how js "virtualization" works. that other code running first can do this is (kinda confusingly) a key part of the foundation of encapsulation rather than evidence that it's unavailable. but the observability river only runs in one direction. reversing the river on untold amounts of existing code is not okay, but a new method or opt-in flag does not have that problem.

@WilcoFiers
Copy link

Instead of returning a function that might potentially include secrets in its string, could some representation of that function be returned instead?

@benjamingr
Copy link
Member

Just another point I thought about: Even with getEventListeners() it is still possible for code interested in doing so to hide event handlers and prevent removing them from consumers by overriding EventTarget.prototype.getEventListeners() itself before any external code can get a reference.

const origGetEventListeners = EventTarget.prototype.getEventListeners;
Object.defineProperty(EventTarget.prototype, 'getEventListeners', {
  value: function myGetEventListeners(eventType) {
    return origGetEventListeners.apply(this, arguments).filter(handler => myCustomFilterLogic(handler));
  }
});

So encapsulation is still possible just not the default with Lea's proposal.

@ljharb
Copy link

ljharb commented Apr 1, 2022

@benjamingr youd probably also have to prevent/hijack creation of new iframes and opened windows, all of which would have a fresh copy of getAllEventListeners. The scope of the problem changes pretty drastically if this method is available.

@benjamingr
Copy link
Member

youd probably also have to prevent/hijack creation of new iframes and opened windows, all of which would have a fresh copy of getAllEventListeners.

It's probably(?) fine to make getEventListeners throw an error if it was used on a different realm which would resolve that.

@ljharb
Copy link

ljharb commented Apr 1, 2022

Do any other web methods do that? That seems like it’d be a pretty big inconsistency to introduce.

@bathos
Copy link

bathos commented Apr 2, 2022

So encapsulation is still possible just not the default with Lea's proposal.

It would be ... for code that’s yet to be written.

@WebReflection
Copy link

@benjamingr I think every “just pollute native prototype” argument could be used, and has been used, already to indeed solve the original issue. For the same reason that doesn’t work one way, it doesn’t any other way and it will also defeat the intended goal of this proposal. Now imagine every library trusting secured events doing that trick …

@dmethvin
Copy link

dmethvin commented Apr 3, 2022

Assuming there is a link in the document, what is the value of x in this example?

document.body.addEventListener("click", (event) => console.log("I am listening"));
var x = document.getElementsByTagName("a")[0].getEventListeners("click");

@subhanahmed047
Copy link

Sorry if this has been mentioned already, but re:security and encapsulation concerns: but developers can already intercept addEventListener by doing something like

let addEventListener = EventTarget.prototype.addEventListener;
let removeEventListener = EventTarget.prototype.removeEventListener;

EventTarget.prototype.addEventListener = function(type, callback, options) {
	/* store args and then… */
	addEventListener.call(this, type, callback, options);
};

EventTarget.prototype.removeEventListener = function(type, callback, options) {
	/* remove from stored args and then… */
	removeEventListener.call(this, type, callback, options);
};

The problem is just that a) it's flimsy, since listeners may have been attached before the code was run and b) it's suboptimal to be overwriting built-ins like that.

I don't think the /* remove from stored args and then… */ part works when you are using the abort controller to remove an event listener. i.e.

const controller = new AbortController();
document.addEventListener('clicl', () => {}, { signal: controller.signal });

controller.abort();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events
Development

No branches or pull requests