Repository navigation
node:util: create aborted()'s listener without Function.prototype.bind - #42283
Conversation
…e.bind aborted() created its listener with onAbortedCallback.bind(undefined, promise). `bind` is looked up on Function.prototype, so a replaced bind received the internal onAbortedCallback as its receiver. Create the listener with a small closure instead, which does no user-visible lookups and still keeps `resource` out of the listener's scope.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe ChangesAbort listener handling
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The abort-listener update has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, correct primordial-safety hardening.
What was reviewed:
- Confirmed
createAbortedListenerbeing its own function keepsresourceout of the closure's scope, so theFinalizationRegistryonresourcestill fires (an inline arrow insideaborted()would have retained it and regressed the leak fix). - The arrow closure discards the
abortevent argument just as the oldbind(undefined, promise)path effectively did;onAbortedCallbackonly readspromise. - Test restores
Function.prototype.bindinfinally, asserts an exact empty receivers array, and lives in the existingtest-aborted.test.ts.
Extended reasoning...
Overview
This PR touches two files: src/js/node/util.ts replaces the onAbortedCallback.bind(undefined, promise) call in util.aborted() with a new createAbortedListener(promise) helper that returns () => onAbortedCallback(promise), and test/js/node/util/test-aborted.test.ts gains one test that patches Function.prototype.bind and asserts aborted() never routes through it. No native code, no API surface change, no behavioral change for well-behaved callers.
Security risks
The change is itself a security/robustness hardening — it removes a user-observable Function.prototype.bind lookup from a builtin, which per REVIEW.md and src/js/CLAUDE.md is exactly the class of tamper-vector builtins must avoid. The main correctness risk to check was whether swapping .bind() for a closure could re-introduce the resource retention the original bind was there to avoid: it does not, because the closure is created in a separate top-level function whose only parameter is promise. The two-line comment states both non-obvious "why"s, satisfying the load-bearing-comment rule.
Level of scrutiny
Low-to-moderate. The diff is ~15 lines, mechanical, and follows the sanctioned pattern for primordial-safe builtins (there is no $bind intrinsic, so a closure is the right substitute). The listener's call signature change (arrow ignores the event arg vs. bind appending it) is immaterial since onAbortedCallback reads only its first parameter. The addEventListener options object and resistStopPropagation wrapper are unchanged.
Other factors
Test hygiene is good: added to the existing module test file, mutated global restored in finally, exact-value assertion that would fail on the pre-fix code (author confirmed USE_SYSTEM_BUN=1 captures onAbortedCallback). CODEOWNERS only covers *.d.ts, so no owner gate applies. No prior reviewer objections in the timeline, and the bug hunt exited on dry_streak with no findings.
|
Updated 9:53 AM PT - Sep 16th, 2026
@dylan-conway, your commit d2dd875 is building: |
2 participants
What does this PR do?
util.aborted()built its abort listener withonAbortedCallback.bind(undefined, promise).bindis looked up onFunction.prototype, so if user code has replacedFunction.prototype.bind, that replacement is called with the internalonAbortedCallbackas its receiver.onAbortedCallbackis a private helper that assumes its argument is the promiseaborted()created, and it is not meant to be reachable from user code.The listener is now created by a small
createAbortedListener(promise)helper that returns a closure. It does no user-visible property lookups, and being its own function it still keepsresourceout of the listener's scope, which is what thebindwas there for (the listener must not keepresourcealive).Behavior of
aborted()is otherwise unchanged.How did you verify your code works?
Added a test to
test/js/node/util/test-aborted.test.tsthat replacesFunction.prototype.bindaround a call toaborted()and asserts it is never invoked.USE_SYSTEM_BUN=1 bun test test/js/node/util/test-aborted.test.ts(Bun 1.4.2): the new test fails with[Function: onAbortedCallback]captured, the other 7 pass.