Repository navigation
node:net: throw ERR_SOCKET_BAD_PORT for Server.listen("") instead of binding an ephemeral port - #34083
node:net: throw ERR_SOCKET_BAD_PORT for Server.listen("") instead of binding an ephemeral port#34083robobun wants to merge 2 commits into
Conversation
…binding an ephemeral port
Server.prototype.listen had an early coercion that converted any string
argument via Number() before deciding whether it was a pipe path or a
port. Since Number("") === 0, an empty (or whitespace-only) string was
silently treated as port 0 and the server bound a dual-stack ephemeral
listener on ::, instead of throwing ERR_SOCKET_BAD_PORT as Node does.
Replace the early coercion with the same isPipeName() discrimination
Node's normalizeArgs uses: a positional string is a pipe path only when
isPipeName() is true, otherwise it is passed to validatePort() (which
already rejects empty and whitespace-only strings in C++).
WalkthroughChangesNode port validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:12 PM PT - Jul 13th, 2026
❌ @robobun, your commit 3cb104d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34083That installs a local version of the PR into your bun-34083 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix that swaps the hand-rolled Number() coercion for the existing isPipeName() helper, matching Node's normalizeArgs. Both inline notes are pre-existing sibling issues (in _http_server.ts and NodeValidator.cpp) that this PR doesn't touch; they're worth follow-ups but don't block here.
Extended reasoning...
Overview
The PR removes an early Number(port) coercion in Server.prototype.listen (src/js/node/net.ts) and replaces the typeof port === "string" pipe-path check with isPipeName(port), which is the same helper Node's normalizeArgs uses and which is already defined and used elsewhere in this file. Empty and whitespace-only strings now flow to the existing validatePort call at L3365-3369 and throw ERR_SOCKET_BAD_PORT instead of being coerced to port 0. A parametrized test in test/js/node/net/node-net-server.test.ts covers "", " ", " ", "\t".
I traced the other input classes through the new routing: numeric strings ("0", "8080") → isPipeName false → validatePort accepts → port | 0; real paths ("/tmp/sock") → isPipeName true → pipe branch; negative-number strings ("-1") → isPipeName true → pipe branch, which matches Node's toNumber semantics exactly; numbers/objects/functions/null/undefined are unaffected since isPipeName gates on typeof s === "string" first, same as the old check.
Security risks
None introduced. The change tightens validation — previously an empty string opened a world-reachable ephemeral listener on ::; now it throws synchronously. No new surface area.
Level of scrutiny
Low-to-medium. This is a ~7-line net change in the Node-compat argument-parsing path, reusing an existing in-file helper to match Node's reference implementation. No native code, no new state, no lifecycle changes.
Other factors
Both inline findings from the bug hunt are explicitly flagged pre-existing: (1) http.Server.listen("") has a separate listen() in _http_server.ts that never delegates here, and (2) validatePort's C++ trim omits U+3000. Neither is touched by this PR and both behave byte-identically before and after. They're reasonable follow-ups per the "fix the whole class" guidance but shouldn't block a correct, tested fix to the net.Server path.
The hand-rolled trim in jsFunction_validatePort enumerated the Unicode
Space_Separator range but omitted U+3000, so a string containing only
U+3000 was not considered empty-after-trim. Since JSC's ToNumber does
treat U+3000 as StrWhiteSpace, Number("\u3000") is 0 and validatePort
accepted it, so net.Server#listen("\u3000") bound an ephemeral port
instead of throwing ERR_SOCKET_BAD_PORT as Node does.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/net/node-net-server.test.ts`:
- Around line 474-495: Change the parameterized test from it.each() to
describe.each(), using each port value as the nested suite parameter and placing
the existing assertions and cleanup in a nested it(). Preserve the current test
behavior and error expectations for every port value.
🪄 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: b14d1593-8b24-482b-b117-0531ca914fca
📒 Files selected for processing (3)
src/js/node/net.tssrc/jsc/bindings/NodeValidator.cpptest/js/node/net/node-net-server.test.ts
There was a problem hiding this comment.
LGTM — small, well-scoped Node-compat fix with good coverage.
Checked: isPipeName() routing preserves behavior for numeric strings ("0", "8080"), pipe paths, numbers, null/undefined, and options objects — only empty/whitespace strings now correctly reach validatePort. The one incidental change ("-1" now treated as a pipe name instead of throwing) matches Node's normalizeArgs exactly. The U+3000 addition to validatePort's trim set completes the ES StrWhiteSpace Zs range. Both prior review threads are addressed (U+3000 folded in; http.Server sibling deferred to a follow-up).
Extended reasoning...
Overview
Three files touched: src/js/node/net.ts (removes an early Number() coercion of the positional port argument in Server.prototype.listen and replaces the typeof port === "string" pipe-path check with isPipeName(port), which is the same helper Node's normalizeArgs uses and is already defined and used elsewhere in this file), src/jsc/bindings/NodeValidator.cpp (adds U+3000 IDEOGRAPHIC SPACE to jsFunction_validatePort's hand-rolled whitespace trim, one line matching the surrounding range-check style), and a new it.each test covering "", " ", " ", "\\t", "\\u3000".
Security risks
None introduced; this is a net security improvement. Before, listen("") silently bound a dual-stack world-reachable ephemeral TCP listener on :: — now it throws ERR_SOCKET_BAD_PORT synchronously before any socket is created, matching Node. The C++ change closes the same gap for U+3000-only strings.
Level of scrutiny
Moderate — this is user-facing argument routing in node:net, so I traced every input class through the new branch: numbers, null, undefined, functions, options objects, numeric strings ("0", "8080", " 8080 ", "1.5", "Infinity"), non-numeric strings (pipe paths), and empty/whitespace strings. All reach the same branch as before except the intended fix (empty/whitespace → validatePort throws) and one incidental case: negative numeric strings like "-1" now route to the pipe-path branch instead of throwing ERR_SOCKET_BAD_PORT. That is exactly Node's behavior (Node's toNumber returns false for negatives, so isPipeName("-1") is true), so it is a compat improvement, not a regression.
Other factors
isPipeNameandtoNumberare pre-existing, already used bynormalizeArgsin the same file — this reuses the established helper rather than adding new logic.- Gate evidence in the PR description shows the new tests fail on main (both ASAN debug and release) and pass with the fix.
- Both of my prior inline notes are resolved: the U+3000 gap was folded into this PR with a test case; the
http.Serversibling in_http_server.tswas acknowledged and deferred to a follow-up (reasonable — that file has a separatelisten()backed byBun.servewith its own error-handling shape). - The 3-line comment in
net.tsexplains the routing intent and stays within the repo's comment-length guidance.
|
CI build #72526: 284/286 lanes passed. The two red lanes are unrelated to this diff:
The new Ready for review. |
Validate listen ports before DNS and binding so HTTP and HTTPS argument errors throw synchronously, while real bind failures remain asynchronous. Preserve numeric-string validation and Node pipe-name routing in net. Adapts oven-sh#34083; thanks @robobun. Covers the OpenClaw gateway listen regression. Node 24 oracle, Linux regression/consumer proof, scoped P2 review, and both native CI lanes passed on the guarded head.
This branch has not been deployed
1 participant
What does this PR do?
net.Server#listen("")(and any whitespace-only string) now throwsERR_SOCKET_BAD_PORTsynchronously, matching Node.js, instead of silently binding a dual-stack ephemeral TCP listener on::.Reproduction
Node.js throws synchronously:
Bun (before this change) binds a world-reachable ephemeral port:
Cause
Server.prototype.listenhad an early coercion that converted a string argument viaNumber()before deciding whether it was a pipe path or a port. SinceNumber("")is0, empty and whitespace-only strings were silently treated as port0, so the server bound an ephemeral listener andvalidatePortnever saw the bad value.Node's
normalizeArgsusesisPipeName()to make this distinction: a positional string is a pipe path only whenisPipeName()is true; otherwise it becomesoptions.portand is passed tovalidatePort, which rejects empty and whitespace-only strings.Fix
Remove the early
Number()coercion and useisPipeName(port)(already defined in the file and used bynormalizeArgs) to route the positional string argument. Empty and whitespace-only strings are not pipe names, so they reachvalidatePortwhich throwsERR_SOCKET_BAD_PORT. Numeric strings like"0"or"8080"continue to work (validatePortaccepts them and they are coerced viaport | 0).Verification
test/js/node/net/node-net-server.test.tscovers""," "," ","\\t"erris undefined, server is listening), passes with this changelisten({port: ""}),Socket#connect("")andlisten("0")were already correct and remain sotest-net-listen-invalid-port,test-net-server-listen-options,test-net-server-listen-pathpass[stamp-90s] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file