node compat: fix tty prototype pollution, MessagePort listener aliases, fs.read position validation#29093
Open
markimbriaco wants to merge 3 commits intooven-sh:mainfrom
Open
Conversation
…fault Fixes oven-sh#29019. The `tty.WriteStream.prototype` getter was assigning its cursor/color helpers directly onto `fs.WriteStream.prototype`, which (1) leaked those methods onto plain `fs.WriteStream` instances and (2) meant the tty subclass had nowhere to store its own default `isTTY = true`. As a result, `tty.WriteStream.prototype.isTTY` was `undefined`, breaking packages (e.g. chalk, supports-color, ink) that probe the prototype directly to decide whether to emit color/control sequences. This matches the existing `ReadStream` pattern: create a dedicated `Object.create(fs.WriteStream.prototype)`, install the tty methods on it, and seal it via `Object.defineProperty`. Also set `isTTY = true` on both `ReadStream.prototype` and `WriteStream.prototype`, matching Node.js `lib/tty.js`. Instance values set by the constructors still override this for fds that are not actually a TTY. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes oven-sh#29022. `injectFakeEmitter()` installs EventEmitter-shaped methods on `MessagePort.prototype` (`on`, `off`, `once`, `emit`, `prependListener`, `prependOnceListener`), but was missing the `addListener`/`removeListener` aliases that Node's EventEmitter exposes. Code that feature-detects the EventEmitter surface (e.g. undici, node-fetch, abort-controller polyfills) does `if ("removeListener" in port) port.removeListener(...)`, which silently skipped the cleanup path under Bun and leaked listeners. Add them as live aliases of `on`/`off` so a handler attached via either name can be removed via either name, matching Node. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes oven-sh#29016. Node's `fs.read`/`fs.readSync` call `validatePosition` on `position` (see `lib/internal/fs/utils.js`), which only accepts `null`, `undefined`, a finite integer, or a bigint. Passing anything else (an object, string, boolean) throws `ERR_INVALID_ARG_TYPE` with `position` as the argument name. Bun's JS wrapper was forwarding arbitrary `position` values straight into the native call, which in one reported case (oven-sh#29016) caused a confusing downstream error or silently used the wrong offset. Mirror Node's validation in the JS layer so the user gets the same `TypeError` with the same argument name, before we touch the fd. Applied symmetrically to async `read` and `readSync`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Author
|
Also sharing this on X: https://x.com/PreviewOps |
Contributor
WalkthroughThe PR adds runtime validation for Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/fs.ts`:
- Around line 375-380: The numeric `position` validation currently permits
non-integer and non-finite numbers; update the checks so that when typeof
position === "number" you also require Number.isInteger(position) and
isFinite(position), and if that fails throw ERR_OUT_OF_RANGE (not
ERR_INVALID_ARG_TYPE); extract a shared helper (e.g.,
validatePosition(position)) in src/js/node/fs.ts to encapsulate the
null/undefined, bigint, number, integer and finite checks and replace the
duplicated blocks used for fs.read()/fs.readSync() validation so both use the
new helper and preserve existing use of $ERR_INVALID_ARG_TYPE for wrong types
and ERR_OUT_OF_RANGE for out-of-range numeric values.
In `@src/js/node/worker_threads.ts`:
- Around line 112-116: The instance methods defined by fakeParentPort are
shadowing the prototype aliases (Class.prototype.addListener and
Class.prototype.removeListener), so parentPort.addListener/removeListener still
use DOM semantics and removeListener can't remove handlers added via on/off;
update fakeParentPort() to either stop defining its own
addListener/removeListener properties or replace them with functions that
delegate to the on/off methods (e.g., addListener -> this.on, removeListener ->
this.off), ensuring parentPort.addListener/removeListener use the same behavior
as Class.prototype.addListener/removeListener and that removeListener correctly
removes listeners added via on().
In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 135-184: The test currently uses setTimeout callbacks after
postMessage to wait for delivery which makes it timing-dependent; replace both
timer-based waits with awaiting the "message" event via once(port1, "message")
(import once from "events") after each postMessage so the assertions run after
the awaited event, then perform removeListener and postMessage again and await
once to verify no additional delivery; keep the port1.close()/port2.close()
cleanup (still in the catch or finally) and convert the test to async so you can
await the once() calls directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d9f0b7e-4e1d-4f04-84de-7e8b620a7d1f
📒 Files selected for processing (6)
src/js/node/fs.tssrc/js/node/tty.tssrc/js/node/worker_threads.tstest/js/node/fs/fs.test.tstest/js/node/tty.test.tstest/js/node/worker_threads/worker_threads.test.ts
Comment on lines
+375
to
+380
| // Validate `position`: must be null/undefined, a number, or a bigint. | ||
| // Mirrors Node.js `validatePosition` (lib/internal/fs/utils.js), which | ||
| // rejects objects, strings, booleans, etc. with TypeError. | ||
| if (position != null && typeof position !== "number" && typeof position !== "bigint") { | ||
| throw $ERR_INVALID_ARG_TYPE("position", ["integer", "bigint"], position); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current Node.js, how do fs.read()andfs.readSync()validatepositionwhen it is1.5, NaN, Infinity, or -Infinity?
💡 Result:
In current Node.js (e.g., v25.x), both fs.read() and fs.readSync() run the same check:
- If
position == null(nullorundefined), Node uses-1(meaning “use & advance the current file position”). - Otherwise Node calls
validatePosition(position, 'position', length), which for number positions callsvalidateInteger(position, 'position', -1). (raw.githubusercontent.com) validateInteger()throwsERR_OUT_OF_RANGEif the value is not a JavaScript integer (Number.isInteger(value)must be true). (raw.githubusercontent.com)
So for the specific inputs (for both fs.read() and fs.readSync()):
position value |
Result |
|---|---|
1.5 |
throws RangeError [ERR_OUT_OF_RANGE] (“position” must be an integer) |
NaN |
throws RangeError [ERR_OUT_OF_RANGE] (“position” must be an integer) |
Infinity |
throws RangeError [ERR_OUT_OF_RANGE] (“position” must be an integer) |
-Infinity |
throws RangeError [ERR_OUT_OF_RANGE] (“position” must be an integer) |
Citations:
- 1: https://raw.githubusercontent.com/nodejs/node/v25.8.1/lib/fs.js
- 2: https://raw.githubusercontent.com/nodejs/node/v25.8.1/lib/internal/validators.js
Add integer and finite-value checks for numeric positions.
The validation currently accepts any number or bigint, but Node.js fs.read() and fs.readSync() reject fractional and non-finite numbers. For numeric position, add a check equivalent to Number.isInteger(position) to reject 1.5, NaN, Infinity, and -Infinity with ERR_OUT_OF_RANGE. Both paths (lines 375–380 and 583–588) share this gap; consider extracting a shared validatePosition helper to avoid duplication and ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/fs.ts` around lines 375 - 380, The numeric `position` validation
currently permits non-integer and non-finite numbers; update the checks so that
when typeof position === "number" you also require Number.isInteger(position)
and isFinite(position), and if that fails throw ERR_OUT_OF_RANGE (not
ERR_INVALID_ARG_TYPE); extract a shared helper (e.g.,
validatePosition(position)) in src/js/node/fs.ts to encapsulate the
null/undefined, bigint, number, integer and finite checks and replace the
duplicated blocks used for fs.read()/fs.readSync() validation so both use the
new helper and preserve existing use of $ERR_INVALID_ARG_TYPE for wrong types
and ERR_OUT_OF_RANGE for out-of-range numeric values.
Comment on lines
+112
to
+116
| // Node.js EventEmitter exposes `addListener`/`removeListener` as aliases of | ||
| // `on`/`off`. MessagePort in node:worker_threads is expected to implement | ||
| // the same surface, so code that checks `"removeListener" in port` works. | ||
| Class.prototype.addListener = Class.prototype.on; | ||
| Class.prototype.removeListener = Class.prototype.off; |
Contributor
There was a problem hiding this comment.
parentPort still shadows these aliases with DOM-style methods.
These prototype aliases fix real MessagePort instances, but fakeParentPort() later defines its own addListener / removeListener properties on the instance. Inside workers, that shadows the new aliases, so parentPort.addListener("message", fn) still uses raw DOM listener semantics and removeListener() will not remove handlers added via on().
Suggested follow-up
- Object.defineProperty(fake, "removeListener", {
- value: self.removeEventListener.bind(self),
- enumerable: false,
- });
-
- Object.defineProperty(fake, "addListener", {
- value: self.addEventListener.bind(self),
- enumerable: false,
- });
+ Object.defineProperty(fake, "removeListener", {
+ value: MessagePort.prototype.removeListener,
+ enumerable: false,
+ });
+
+ Object.defineProperty(fake, "addListener", {
+ value: MessagePort.prototype.addListener,
+ enumerable: false,
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/worker_threads.ts` around lines 112 - 116, The instance methods
defined by fakeParentPort are shadowing the prototype aliases
(Class.prototype.addListener and Class.prototype.removeListener), so
parentPort.addListener/removeListener still use DOM semantics and removeListener
can't remove handlers added via on/off; update fakeParentPort() to either stop
defining its own addListener/removeListener properties or replace them with
functions that delegate to the on/off methods (e.g., addListener -> this.on,
removeListener -> this.off), ensuring parentPort.addListener/removeListener use
the same behavior as Class.prototype.addListener/removeListener and that
removeListener correctly removes listeners added via on().
Comment on lines
+135
to
+184
| test("MessagePort exposes full EventEmitter alias surface", () => { | ||
| // Regression test for oven-sh/bun#29022. MessagePort.prototype was missing | ||
| // `addListener`/`removeListener` aliases, so code that does | ||
| // `if ("removeListener" in port)` (e.g. undici, node-fetch) would | ||
| // incorrectly skip the cleanup path. | ||
| const { port1, port2 } = new MessageChannel(); | ||
| try { | ||
| for (const name of [ | ||
| "on", | ||
| "off", | ||
| "once", | ||
| "emit", | ||
| "addListener", | ||
| "removeListener", | ||
| "prependListener", | ||
| "prependOnceListener", | ||
| ] as const) { | ||
| expect(name in port1).toBe(true); | ||
| expect(typeof port1[name]).toBe("function"); | ||
| } | ||
|
|
||
| // `addListener`/`removeListener` must be live aliases of `on`/`off`, as in | ||
| // Node.js EventEmitter, so a listener attached via one name can be removed | ||
| // via the other. | ||
| let received = 0; | ||
| const handler = () => { | ||
| received++; | ||
| }; | ||
| port1.addListener("message", handler); | ||
| port2.postMessage("hello"); | ||
| // Give the event loop a tick so the message is delivered. | ||
| return new Promise<void>(resolve => { | ||
| setTimeout(() => { | ||
| expect(received).toBe(1); | ||
| port1.removeListener("message", handler); | ||
| port2.postMessage("world"); | ||
| setTimeout(() => { | ||
| expect(received).toBe(1); | ||
| port1.close(); | ||
| port2.close(); | ||
| resolve(); | ||
| }, 10); | ||
| }, 10); | ||
| }); | ||
| } catch (err) { | ||
| port1.close(); | ||
| port2.close(); | ||
| throw err; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Replace the timer-based waits with event awaits.
The two setTimeout() calls make this regression test timing-dependent, and a failed expect() inside those callbacks will not fail the returned promise as cleanly as an awaited event path. await once(port1, "message") for each postMessage() would make the test deterministic and keep failures attached to the test itself.
Proposed rewrite
-test("MessagePort exposes full EventEmitter alias surface", () => {
+test("MessagePort exposes full EventEmitter alias surface", async () => {
const { port1, port2 } = new MessageChannel();
try {
for (const name of [
"on",
"off",
"once",
"emit",
"addListener",
"removeListener",
"prependListener",
"prependOnceListener",
] as const) {
expect(name in port1).toBe(true);
expect(typeof port1[name]).toBe("function");
}
let received = 0;
const handler = () => {
received++;
};
port1.addListener("message", handler);
- port2.postMessage("hello");
- // Give the event loop a tick so the message is delivered.
- return new Promise<void>(resolve => {
- setTimeout(() => {
- expect(received).toBe(1);
- port1.removeListener("message", handler);
- port2.postMessage("world");
- setTimeout(() => {
- expect(received).toBe(1);
- port1.close();
- port2.close();
- resolve();
- }, 10);
- }, 10);
- });
- } catch (err) {
+ const firstMessage = once(port1, "message");
+ port2.postMessage("hello");
+ await firstMessage;
+ expect(received).toBe(1);
+
+ port1.removeListener("message", handler);
+
+ const secondMessage = once(port1, "message");
+ port2.postMessage("world");
+ await secondMessage;
+ expect(received).toBe(1);
+ } finally {
port1.close();
port2.close();
- throw err;
}
});As per coding guidelines: Do not use setTimeout in tests; instead await the condition to be met as you are testing the CONDITION, not TIME PASSING.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("MessagePort exposes full EventEmitter alias surface", () => { | |
| // Regression test for oven-sh/bun#29022. MessagePort.prototype was missing | |
| // `addListener`/`removeListener` aliases, so code that does | |
| // `if ("removeListener" in port)` (e.g. undici, node-fetch) would | |
| // incorrectly skip the cleanup path. | |
| const { port1, port2 } = new MessageChannel(); | |
| try { | |
| for (const name of [ | |
| "on", | |
| "off", | |
| "once", | |
| "emit", | |
| "addListener", | |
| "removeListener", | |
| "prependListener", | |
| "prependOnceListener", | |
| ] as const) { | |
| expect(name in port1).toBe(true); | |
| expect(typeof port1[name]).toBe("function"); | |
| } | |
| // `addListener`/`removeListener` must be live aliases of `on`/`off`, as in | |
| // Node.js EventEmitter, so a listener attached via one name can be removed | |
| // via the other. | |
| let received = 0; | |
| const handler = () => { | |
| received++; | |
| }; | |
| port1.addListener("message", handler); | |
| port2.postMessage("hello"); | |
| // Give the event loop a tick so the message is delivered. | |
| return new Promise<void>(resolve => { | |
| setTimeout(() => { | |
| expect(received).toBe(1); | |
| port1.removeListener("message", handler); | |
| port2.postMessage("world"); | |
| setTimeout(() => { | |
| expect(received).toBe(1); | |
| port1.close(); | |
| port2.close(); | |
| resolve(); | |
| }, 10); | |
| }, 10); | |
| }); | |
| } catch (err) { | |
| port1.close(); | |
| port2.close(); | |
| throw err; | |
| } | |
| }); | |
| test("MessagePort exposes full EventEmitter alias surface", async () => { | |
| // Regression test for oven-sh/bun#29022. MessagePort.prototype was missing | |
| // `addListener`/`removeListener` aliases, so code that does | |
| // `if ("removeListener" in port)` (e.g. undici, node-fetch) would | |
| // incorrectly skip the cleanup path. | |
| const { port1, port2 } = new MessageChannel(); | |
| try { | |
| for (const name of [ | |
| "on", | |
| "off", | |
| "once", | |
| "emit", | |
| "addListener", | |
| "removeListener", | |
| "prependListener", | |
| "prependOnceListener", | |
| ] as const) { | |
| expect(name in port1).toBe(true); | |
| expect(typeof port1[name]).toBe("function"); | |
| } | |
| // `addListener`/`removeListener` must be live aliases of `on`/`off`, as in | |
| // Node.js EventEmitter, so a listener attached via one name can be removed | |
| // via the other. | |
| let received = 0; | |
| const handler = () => { | |
| received++; | |
| }; | |
| port1.addListener("message", handler); | |
| const firstMessage = once(port1, "message"); | |
| port2.postMessage("hello"); | |
| await firstMessage; | |
| expect(received).toBe(1); | |
| port1.removeListener("message", handler); | |
| const secondMessage = once(port1, "message"); | |
| port2.postMessage("world"); | |
| await secondMessage; | |
| expect(received).toBe(1); | |
| } finally { | |
| port1.close(); | |
| port2.close(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/worker_threads/worker_threads.test.ts` around lines 135 - 184,
The test currently uses setTimeout callbacks after postMessage to wait for
delivery which makes it timing-dependent; replace both timer-based waits with
awaiting the "message" event via once(port1, "message") (import once from
"events") after each postMessage so the assertions run after the awaited event,
then perform removeListener and postMessage again and await once to verify no
additional delivery; keep the port1.close()/port2.close() cleanup (still in the
catch or finally) and convert the test to async so you can await the once()
calls directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three independent Node.js compat fixes that came up while triaging
node:-tagged issues. Each is small, has tests, and corresponds to an open issue.1.
node:tty—WriteStream.prototype.isTTY+ prototype pollution — fixes #29019tty.WriteStream.prototypewas doingconst Real = fs.WriteStream.prototypeand hangingclearLine,cursorTo,getColorDepth,hasColors,moveCursor, etc. directly onto it. Two consequences:fs.WriteStream.prototype(the plain fs one) ended up with tty-only methods leaking onto it.isTTYdefault, sotty.WriteStream.prototype.isTTYwasundefined, whereas Node'slib/tty.jsguarantees it to betrue. Packages like chalk / supports-color / ink probe the prototype directly, so the wrong answer disables colors.Fixed by switching to
Object.create(fs.WriteStream.prototype)(matching the existingReadStreamshape in this same file) and settingPrototype.isTTY = trueon bothReadStream.prototypeandWriteStream.prototype. Instance values set by the constructors still override for fds that are not actually a TTY.2.
node:worker_threads—MessagePort.addListener/removeListener— fixes #29022injectFakeEmitter()was installingon,off,once,emit,prependListener,prependOnceListener— but not theaddListener/removeListeneraliases that Node's EventEmitter exposes. Code that feature-detects the EventEmitter surface (undici, node-fetch, various abort-controller polyfills) doesif ("removeListener" in port) port.removeListener(...)and silently skipped the cleanup path under Bun, leaking listeners. Added them as live aliases ofon/off.3.
node:fs—read/readSyncposition type validation — fixes #29016Node's
validatePosition(lib/internal/fs/utils.js) only acceptsnull/undefined/ a finite integer / a bigint forposition; anything else throwsERR_INVALID_ARG_TYPEwithpositionas the argument name. Bun's JS wrapper was forwarding arbitrary values straight into the native call. Mirrored Node's validation in the JS layer (asyncread+readSync) so users get the sameTypeErrorwith the same argument name before we touch the fd.Test plan
test/js/node/tty.test.ts: newtty.{ReadStream,WriteStream}.prototype.isTTYblock covering both prototype defaults, the distinct-prototype relationship, and explicitObject.hasOwnchecks thatfs.WriteStream.prototypeis no longer polluted with tty-only keys (isTTY,clearLine,clearScreenDown,cursorTo,getColorDepth,hasColors,moveCursor).test/js/node/worker_threads/worker_threads.test.ts: newMessagePort exposes full EventEmitter alias surfacetest that (a) asserts all 8 listener names are present and are functions, and (b) verifiesaddListener/removeListenerare live aliases ofon/offby attaching via one and removing via the other.test/js/node/fs/fs.test.ts: two new cases inside the existingdescribe("readSync", ...)— one assertingTypeErrorfor object / string / booleanposition, one assertingnull/undefined/ number / bigint are accepted.Happy to split into three PRs if you'd prefer — they're already in three separate commits on the branch. cc @Jarred-Sumner
🤖 Generated with Claude Code