fix: use untainted add/remove event listener methods in on func#1814
fix: use untainted add/remove event listener methods in on func#1814megboehlert wants to merge 1 commit intorrweb-io:masterfrom
Conversation
🦋 Changeset detectedLatest commit: abf8a1a The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR extends the “untainted prototype” utilities to cover EventTarget so rrweb’s on() helper can use unpatched addEventListener/removeEventListener even when frameworks monkey-patch them.
Changes:
- Add
EventTargetsupport to@rrweb/utilsuntainted prototype/method detection. - Update rrweb’s
on()helper to call untaintedaddEventListener/removeEventListener. - Add Vitest coverage for
getUntaintedMethod('EventTarget', ...)and publish a patch changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/utils/src/index.ts | Adds EventTarget to the supported prototype owners and method checks. |
| packages/rrweb/src/utils.ts | Switches on() to use untainted EventTarget listener methods. |
| packages/rrweb/test/util.test.ts | Adds tests for getUntaintedMethod with EventTarget methods. |
| .changeset/eleven-worms-try.md | Declares patch releases for rrweb and @rrweb/utils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getUntaintedMethod('EventTarget', eventTarget, 'addEventListener')(type, fn, options); | ||
| return () => (getUntaintedMethod('EventTarget', eventTarget, 'removeEventListener') )(type, fn, options); |
There was a problem hiding this comment.
The cleanup handler calls getUntaintedMethod(...) again and has an extra )( parenthesis/space. Consider grabbing both untainted methods once (e.g., const add = ...; const remove = ...;) and reusing them for add/remove to avoid repeated lookups/binds and to keep the return statement readable (also avoids formatting/lint issues).
| getUntaintedMethod('EventTarget', eventTarget, 'addEventListener')(type, fn, options); | |
| return () => (getUntaintedMethod('EventTarget', eventTarget, 'removeEventListener') )(type, fn, options); | |
| const add = getUntaintedMethod('EventTarget', eventTarget, 'addEventListener'); | |
| const remove = getUntaintedMethod( | |
| 'EventTarget', | |
| eventTarget, | |
| 'removeEventListener', | |
| ); | |
| add(type, fn, options); | |
| return () => remove(type, fn, options); |
| const eventTarget = target as unknown as typeof EventTarget.prototype; | ||
| getUntaintedMethod('EventTarget', eventTarget, 'addEventListener')(type, fn, options); | ||
| return () => (getUntaintedMethod('EventTarget', eventTarget, 'removeEventListener') )(type, fn, options); |
There was a problem hiding this comment.
The cast target as unknown as typeof EventTarget.prototype is confusing (it reads like a prototype object, but it’s actually an EventTarget instance). Using EventTarget (or EventTarget & { addEventListener: ... }) directly would make the intent clearer and avoid misleading types in future refactors.
| const eventTarget = target as unknown as typeof EventTarget.prototype; | |
| getUntaintedMethod('EventTarget', eventTarget, 'addEventListener')(type, fn, options); | |
| return () => (getUntaintedMethod('EventTarget', eventTarget, 'removeEventListener') )(type, fn, options); | |
| const eventTarget = target as unknown as EventTarget; | |
| getUntaintedMethod('EventTarget', eventTarget, 'addEventListener')(type, fn, options); | |
| return () => (getUntaintedMethod('EventTarget', eventTarget, 'removeEventListener'))(type, fn, options); |
| let called = false; | ||
| (untaintedAdd as typeof EventTarget.prototype.addEventListener)('custom-test', () => { called = true; }); | ||
| el.dispatchEvent(new Event('custom-test')); | ||
| expect(called).toBe(true); |
There was a problem hiding this comment.
This test patches EventTarget.prototype.addEventListener but never asserts that the patched implementation was bypassed (e.g., patchCallCount stays 0). Adding an assertion on patchCallCount would make the test actually verify the intended behavior rather than only that events still fire.
| expect(called).toBe(true); | |
| expect(called).toBe(true); | |
| expect(patchCallCount).toBe(0); |
| // Force cache bust by clearing module-level cache isn't possible here, | ||
| // so we verify the untainted method itself is the original, native one | ||
| const untaintedAdd = getUntaintedMethod( | ||
| 'EventTarget', | ||
| el as unknown as typeof EventTarget.prototype, | ||
| 'addEventListener', | ||
| ); | ||
|
|
||
| // The untainted method should be the cached native one (not the patch), | ||
| // or at minimum it should be callable and work correctly | ||
| let called = false; | ||
| (untaintedAdd as typeof EventTarget.prototype.addEventListener)('custom-test', () => { called = true; }); | ||
| el.dispatchEvent(new Event('custom-test')); | ||
| expect(called).toBe(true); | ||
|
|
||
| // Restore | ||
| EventTarget.prototype.addEventListener = originalAdd; | ||
| document.body.removeChild(el); |
There was a problem hiding this comment.
EventTarget.prototype.addEventListener is restored only at the end of the test. If an expectation throws before the restore runs, the patched prototype can leak into later tests. Wrap the patch/restore in try/finally (or afterEach) to guarantee restoration.
| // Force cache bust by clearing module-level cache isn't possible here, | |
| // so we verify the untainted method itself is the original, native one | |
| const untaintedAdd = getUntaintedMethod( | |
| 'EventTarget', | |
| el as unknown as typeof EventTarget.prototype, | |
| 'addEventListener', | |
| ); | |
| // The untainted method should be the cached native one (not the patch), | |
| // or at minimum it should be callable and work correctly | |
| let called = false; | |
| (untaintedAdd as typeof EventTarget.prototype.addEventListener)('custom-test', () => { called = true; }); | |
| el.dispatchEvent(new Event('custom-test')); | |
| expect(called).toBe(true); | |
| // Restore | |
| EventTarget.prototype.addEventListener = originalAdd; | |
| document.body.removeChild(el); | |
| try { | |
| // Force cache bust by clearing module-level cache isn't possible here, | |
| // so we verify the untainted method itself is the original, native one | |
| const untaintedAdd = getUntaintedMethod( | |
| 'EventTarget', | |
| el as unknown as typeof EventTarget.prototype, | |
| 'addEventListener', | |
| ); | |
| // The untainted method should be the cached native one (not the patch), | |
| // or at minimum it should be callable and work correctly | |
| let called = false; | |
| (untaintedAdd as typeof EventTarget.prototype.addEventListener)('custom-test', () => { called = true; }); | |
| el.dispatchEvent(new Event('custom-test')); | |
| expect(called).toBe(true); | |
| } finally { | |
| EventTarget.prototype.addEventListener = originalAdd; | |
| document.body.removeChild(el); | |
| } |
| describe('getUntaintedMethod for EventTarget', () => { | ||
| it('getUntaintedMethod returns a callable addEventListener bound to the target', () => { | ||
| const el = document.createElement('div'); | ||
| document.body.appendChild(el); | ||
|
|
||
| let called = false; | ||
| const handler = () => { called = true; }; | ||
|
|
||
| const addFn = getUntaintedMethod('EventTarget', el as unknown as typeof EventTarget.prototype, 'addEventListener'); | ||
| (addFn as typeof EventTarget.prototype.addEventListener)('click', handler); | ||
| el.dispatchEvent(new Event('click')); | ||
|
|
||
| expect(called).toBe(true); | ||
| document.body.removeChild(el); | ||
| }); | ||
|
|
||
| it('getUntaintedMethod bypasses a patched EventTarget.prototype.addEventListener', () => { | ||
| const el = document.createElement('div'); | ||
| document.body.appendChild(el); | ||
|
|
||
| const originalAdd = EventTarget.prototype.addEventListener; | ||
| let patchCallCount = 0; | ||
| EventTarget.prototype.addEventListener = function ( | ||
| this: EventTarget, | ||
| ...args: Parameters<typeof originalAdd> | ||
| ) { | ||
| patchCallCount++; | ||
| return originalAdd.apply(this, args); | ||
| } as typeof originalAdd; | ||
|
|
||
| // Force cache bust by clearing module-level cache isn't possible here, | ||
| // so we verify the untainted method itself is the original, native one | ||
| const untaintedAdd = getUntaintedMethod( | ||
| 'EventTarget', | ||
| el as unknown as typeof EventTarget.prototype, | ||
| 'addEventListener', | ||
| ); | ||
|
|
There was a problem hiding this comment.
This test’s behavior depends on module-level caches inside @rrweb/utils: the first test in this describe block can populate the cache before the prototype is patched, so the “bypasses a patched ...” case may pass even if the initial cache fill would fail when patching happens first. Consider patching before the first getUntaintedMethod call (or using vi.resetModules() + dynamic import) so the test validates the real-world scenario where the prototype is already monkey-patched when rrweb starts.
| type PrototypeOwner = Node | ShadowRoot | MutationObserver | Element | EventTarget; | ||
| type TypeofPrototypeOwner = | ||
| | typeof Node | ||
| | typeof ShadowRoot | ||
| | typeof MutationObserver | ||
| | typeof Element; | ||
| | typeof Element | ||
| | typeof EventTarget; |
There was a problem hiding this comment.
Adding EventTarget as a supported prototype owner means getUntaintedPrototype('EventTarget') will attempt globalThis['EventTarget'].prototype. If EventTarget is missing in a given runtime, this will throw before the try/catch fallback runs. Consider guarding getUntaintedPrototype (e.g., return the default prototype or throw a clearer error) when globalThis[key] is undefined to avoid hard crashes.
No description provided.