Skip to content

fix(node:net): validate listen ports synchronously - #44555

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/w131-listen-port-validation-upstream
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/w131-listen-port-validation-upstream

Conversation

@steipete

@steipete steipete commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

http.Server.listen(65536) and https.Server.listen(65536) now throw RangeError with ERR_SOCKET_BAD_PORT synchronously, like Node 24. Real bind failures such as EADDRINUSE remain asynchronous error events.

HTTP currently lets Bun.serve() validate the port inside the bind-error catch. The fix validates before binding, preserves the original string until validation, and converts the complete numeric string instead of accepting a parseInt() prefix. Numeric ports take precedence over an accompanying path. The net portion preserves whitespace strings for validation and uses Node's pipe-name routing.

Adapts the net string-routing and U+3000 validation changes from #34083, which remains open; thanks @robobun. This also covers HTTP/HTTPS timing and the full port overload matrix.

Proof:

  • Node v24.19.0 passes a dependency-free matrix of 264 invalid-port cases and three asynchronous bind controls. The exact upstream candidate passes the same matrix.
  • All 75 added regressions pass on an exact Linux build of this branch. Exact unpatched upstream base aa8307619d8dccbda113a5a4aa1885e0cef7eaba fails 36 of them and passes 39.
  • The fix resolves OpenClaw's gateway listen consumer: Node 5/5, c999 4/5, patched fork 5/5, with isolated HOME/state/TMPDIR and umask 022.
  • Surrounding net/HTTP/TLS suites pass 484 tests with one existing skip on the exact upstream candidate.
  • Scoped P2 branch review is clean.

The fork counterpart is openclaw#101; Linux x64 and macOS arm64 native CI passed before its guarded squash merge.

Validate HTTP and HTTPS ports before binding, preserving asynchronous bind failures. Adapt the net string-routing and Unicode whitespace fixes from oven-sh#34083.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (3)
src/js/CLAUDE.md — configured
test/CLAUDE.md — configured
src/CLAUDE.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 8f21c339-d48f-4988-b67f-a44da06262d0
📥 Commits

Reviewing files that changed from the base of the PR and between aa83076 and c40812a.

📒 Files selected for processing (4)
  • src/js/node/_http_server.ts
  • src/js/node/net.ts
  • src/jsc/bindings/NodeValidator.cpp
  • test/js/node/http/node-http.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Server listen() argument routing and port validation changed for Node-compatible servers. Null and omitted ports can select an ephemeral port, and invalid port values are rejected synchronously. Tests cover net, http, and https servers.

Changes

Server listen port handling

Layer / File(s) Summary
Listen argument routing
src/js/node/net.ts
The pipe-path branch now uses isPipeName(port) without first converting numeric-looking strings to numbers.
Port validation and binding
src/js/node/_http_server.ts, src/jsc/bindings/NodeValidator.cpp, test/js/node/http/node-http.test.ts
listen() validates numeric and string ports before binding, treats null like an omitted port when no socket path is supplied, and coerces validated ports to signed 32-bit numbers. Port-string trimming includes U+3000. Tests cover invalid inputs, ephemeral-port forms, and asynchronous EADDRINUSE.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to c4081

This change makes invalid listen ports throw synchronously, as Node does, and keeps address-in-use failures asynchronous. The supplied evidence shows no outstanding merge-blocking risk.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c4081

Port validation becomes stricter without weakening the numeric range controls. The main security-relevant change is endpoint selection when port and path inputs overlap. No introduced authorization bypass was established, but downstream deployment exposure and cancellation-after-failure behavior remain incompletely assessed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The security-relevant exposure is listener placement for applications using these APIs. HTTP selects TCP when a valid port accompanies a path, while net accepts additional positional strings as pipe names. Deployment impact depends on caller configuration and endpoint access controls, which were not supplied.

Trust Boundaries and Controls

  • observed — Numeric port inputs are validated before native binding. Strings classified as pipe names use the existing Unix-socket binding path, which already accepted nonnumeric paths before this PR. The routing expansion does not itself establish additional binding authority or a numeric validation bypass.

Resilience and Maintainability Implications

  • observed — Successful net binding assigns handle ownership before scheduling listening, and close releases the handle. The added regressions check invalid-input error timing and asynchronous address conflicts, but do not exercise AbortSignal cleanup followed by retry. Test execution was not independently performed in this review.

Hardening Proposals

  • proposed — Applications relying on filesystem-restricted IPC should explicitly reject simultaneous port and path configuration, rather than treating a supplied path as proof that the listener remains IPC-only.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: synchronous validation of listen ports.
Description check ✅ Passed The description explains the change and provides detailed verification results. It covers both required template topics, although it does not use the template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant