Bun.serve: reject upgrade(), timeout() and requestIP() for a Request that another server received - #41811
Bun.serve: reject upgrade(), timeout() and requestIP() for a Request that another server received#41811robobun wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe server runtime now exposes the attached server pointer on ChangesServer Ownership Validation
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Cross-server upgrade, timeout and requestIP calls now throw as intended. One narrow edge case, a foreign request used while the server listens on a Unix socket or has been terminated, still returns null or false instead of throwing. That is a safe fallback and not worth blocking the merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on 1.4.3-canary ( The new test in CI: the only red lane is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 1:45 AM PT - Oct 3rd, 2026
✅ @robobun, your commit b93225c3a95555b369518856e2710342aed2c0e9 passed in 🧪 To try this PR locally: bunx bun-pr 41811That installs a local version of the PR into your bun-41811 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
I arrived at the same fix independently for the Two small things from that branch that this PR could pick up if wanted:
|
…t another server received Each of these resolved the Request to its RequestContext by context type only (ssl x debug), so a Request dispatched by server A was accepted by server B when both had the same type. upgrade() then built the ServerWebSocket from B's handlers on A's socket and A's uWS app: B's callbacks fired, but the topics, publish() and stop() stayed with A. timeout() changed A's connection timeout from B. The RequestContext already holds a backref to the server that created it. Compare it with the server the method was called on and throw a TypeError when they differ. A detached Request (no server) keeps the old false/null/undefined results.
6bd1905 to
b93225c
Compare
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/server/server_body.rs— nit: a server listening on a unix socket silently accepts another server's Request in timeout(), returning null instead of the new error. The is_unix early return at server_body.rs:1570-1572 runs before check_same_server at :1584, unlike requestIP() where the ownership check precedes request_ip's unix check. Fix: run check_same_server before the is_unix return (and before the TERMINATED / handler.server early returns in on_upgrade at :1671-1687) so a foreign Request throws on every variant.Why this was flagged
Bun.serve({ unix: path, ... }) as server B; inside server A's fetch handler call B.timeout(req, 5). timeout() at server_body.rs:1570 returns Ok(JSValue::NULL) because B.config.address.is_unix() before reaching the new check at :1584, so no error is thrown. B.requestIP(req) on the same unix server does throw, because do_request_ip at :1508 checks ownership before request_ip's unix return at :1543. on_upgrade similarly returns false at :1672 and :1686 for a terminated or finalizing B before the check at :1693/:1820. Base returned null in all these cases; the inconsistency is new and harmless (no cross-server mutation happens on these paths), so this is a nit about the variant matrix the PR claims to cover.
Verification: The server the method is called on is configured with
unix:and it is handed a Request owned by a different, still-attached server. In /home/claude/bun/src/runtime/server/server_body.rs,timeout()returns early at lines 1570-1572 beforecheck_same_serverat line 1584, so a unix-socket server B silently returns null forB.timeout(reqA, n)instead of throwing the PR's new error.
| self.check_same_server( | ||
| global, | ||
| "upgrade", | ||
| Some(node_http_response.server.ptr.cast_const()), | ||
| )?; |
There was a problem hiding this comment.
🔴 node:http users who close() and listen() again lose WebSocket upgrades on connections from the first listener: the upgrade now throws instead of succeeding. ws.js completeUpgrade calls socket.server[kBunInternals].upgrade(req), i.e. the http.Server's CURRENT Bun.serve, while req is a NodeHTTPResponse whose server is the generation that accepted the connection, so check_same_server at server_body.rs:1693-1697 rejects it. Fix: do not apply the ownership check on the NodeHTTPResponse branches of upgrade() and timeout() (both act on the response's own server/connection), or make ws.js call upgrade on the response's own server; same pattern at server_body.rs:1590.
Why this was flagged
An http.Server gets a keep-alive request that is in flight when the user calls server.close() and then server.listen() again. A later WebSocket upgrade request on that connection is dispatched by the first generation's Bun.serve; ws.js completeUpgrade does const server = socket.server[kBunInternals] (src/js/thirdparty/ws.js:1549), which is the NEW Bun.serve set at _http_server.ts:698, then server.upgrade(req, ...) (ws.js:1566) with req = the old generation's NodeHTTPResponse. In on_upgrade the new check at server_body.rs:1693-1697 compares node_http_response.server.ptr (old NewServer) with self (new NewServer) and throws 'upgrade() must be called on the same server that received the request'. On the base branch this call reached node_http_response.upgrade(), which builds the ServerWebSocket from self.server (NodeHTTPResponse.rs:625-641), so the upgrade succeeded. The TERMINATED and handler.server guards at server_body.rs:1671-1687 do not stop this because the old generation was stopped gracefully and the new generation is live.
Verification: src/js/thirdparty/ws.js:1549 const server = socket.server[kBunInternals]; then :1566 server.upgrade(req, {...}). New check at src/runtime/server/server_body.rs:1693-1697 compares node_http_response.server.ptr against self and throws upgrade() must be called on the same server that received the request.
| let request = arg.as_class_ref::<Request>().ok_or_else(|| { | ||
| global.throw_invalid_arguments(format_args!("Expected Request object")) | ||
| })?; | ||
| self.check_same_server(global, "requestIP", request.request_context.server_ptr())?; |
There was a problem hiding this comment.
🔴 Bun.serve users who call requestIP() or timeout() through a server instance other than the one that received the request now get a TypeError and a failed request where the base returned the correct value. This follows from the stated purpose, but for these two methods nothing was mis-wired on the base: request_ip at server_body.rs:1542-1548 and set_timeout at 1585 read only the Request's own context, and the called server contributes nothing but the is_unix check. Fix: either confine the ownership check to upgrade(), where the called server's websocket handlers really are used, or keep the throw and document it as a breaking change; the call is the author's.
Why this was flagged
Two Bun.serve instances exist in one process and a fetch handler shared by both calls A.requestIP(req) or A.timeout(req, n) for a Request that B received; this is ordinary code when one server object is module-level and a second listener (redirect, health, admin port) reuses the same handler. On the base branch do_request_ip resolves the address from request.request_context.get_remote_socket_info() (server_body.rs:1546), which is the request's own socket, and timeout() calls request_context.set_timeout(value) on the request's own connection (server_body.rs:1585); self is only consulted for self.config.address.is_unix(). Both calls therefore returned the right answer for the request. After this change the new lines at server_body.rs:1508 and 1584 throw 'requestIP() must be called on the same server that received the request' / 'timeout() must be called on the same server that received the request', so the handler rejects and the client receives an error response.
Verification: On the base, request_ip (server_body.rs:1542-1555) derives the address solely from request.request_context.get_remote_socket_info(), and the called server contributes only self.config.address.is_unix(); likewise timeout() (server_body.rs:1585) calls request_context.set_timeout(value) on the request's own connection. The new check_same_server (server_body.rs:1282-1297) is wired at :1508 and :1584.
| self.check_same_server( | ||
| global, | ||
| "upgrade", | ||
| Some(node_http_response.server.ptr.cast_const()), | ||
| )?; |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers get no regression coverage for the node:http half of this fix: the NodeHTTPResponse branches of upgrade() and timeout() gain check_same_server but the new test exercises only Request arguments. The NodeHTTPResponse check at server_body.rs:1693-1697 (and the timeout one at server_body.rs:1590) is reachable from JS via the ws shim path, so it can regress silently. Fix: add a sibling case alongside the new test that drives both NodeHTTPResponse call sites cross-server and same-server, e.g. two node:http servers where otherHttp[Symbol.for("::bunternal::")].upgrade(req.socket[Symbol.for("::bunternal::")]) must throw and the owning server's call must still succeed.
Why this was flagged
The diff adds check_same_server to four entry points: the Request branches of requestIP (server_body.rs:1508), timeout (server_body.rs:1584) and upgrade (server_body.rs:1820), and the NodeHTTPResponse branches of timeout (server_body.rs:1590) and upgrade (server_body.rs:1693-1697). The only test added, test/js/bun/http/bun-server.test.ts:112-163, passes a Bun.serve Request to every call; nothing in the PR passes a NodeHTTPResponse. A NodeHTTPResponse reaches these methods from user JS: _http_server.ts:2191 exposes it as socket[Symbol.for("::bunternal::")] and src/js/thirdparty/ws.js:1549-1566 calls socket.server[kBunInternals].upgrade(socket[kBunInternals], ...). If the NodeHTTPResponse argument expression or the comparison against response.server.ptr were later changed or dropped, no test would fail, so the two node:http sites can silently revert to the pre-PR behaviour of accepting another server's response. REVIEW.md asks that every sibling entry point receiving the same fix be covered.
Verification: Any future change to the NodeHTTPResponse branches of upgrade()/timeout() (server_body.rs:1693-1697 and :1590) can drop the cross-server rejection without any test failing. The only test added (test/js/bun/http/bun-server.test.ts:112-163) passes a Bun.serve Request; nothing passes a NodeHTTPResponse.
This branch has not been deployed
2 participants
Problem
serverB.upgrade(req)accepts aRequestthat anotherBun.serve()instance (A) received, when A and B have the same (TLS, debug) type. TheServerWebSocketis built from B'swebsockethandlers on A's socket and A's uWS app:B.open/B.messagefire, butA.subscriberCount()sees the socket,B.publish()reaches nobody, andB.stop(true)does not close it.serverB.timeout(reqA, n)changes A's connection timeout, andserverB.requestIP(reqA)answers, for the same reason.on_upgraderesolves theRequestwithrequest_context.get::<ServerRequestContext<SSL, DEBUG>>()(src/runtime/server/server_body.rs), which checks only the context type tag.timeout()andrequestIP()go through the type-erasedAnyRequestContextdispatch and never look atself.Fix
RequestContextalready stores a backref to the server that created it (ctx.server,Noneonce detached). AddAnyRequestContext::server_ptr()andNewServer::check_same_server(), and call it fromupgrade(),timeout()andrequestIP()(for anode:httpresponse object, through itsAnyServerfield).Requeststill attached to a different server throwsTypeError: upgrade() must be called on the same server that received the request. A detachedRequest(finished, aborted, already upgraded) keeps today's results:false,null, no-op.falsebecause passing another server'sRequestis always a programming error, while an aborted or finished request is a runtime condition thatfalsealready reports.test/js/bun/http/bun-server.test.ts("upgrade(), timeout() and requestIP() throw for a Request that another server received") fails on 1.4.3 and passes with the fix. Also ran theupgradetests inwebsocket-server.test.ts,websocket-server-upgrade-reentrant.test.ts, therequestIP/timeouttests inserve.test.ts, andserve-http2.test.ts.Background
Bun.serve()call creates oneNewServer<SSL, DEBUG>(four monomorphizations). Every request gets a pooledRequestContextwhoseserverfield points back at that instance.Requestcreated by the server holds anAnyRequestContext: a (type tag, pointer) pair over the eightRequestContextmonomorphizations (HTTP/1 plus the HTTP/2 and HTTP/3 variants).get::<T>()compares only the tag, so two plain-HTTP servers produce contexts that look the same.upgrade()takes the socket out of the HTTP parser of whichever uWS app owns it and registers the WebSocket there. So the app (topics,publish(),stop()) comes from the request, while the handlers come from the server the method was called on.Notes
B.upgrade(reqA) -> true,B.open,B.message hi, thenA.subscriberCount(room)=1 B.subscriberCount(room)=0 A.publish=1 B.publish=0,after B.stop(true) readyState=1,after A.stop(true) readyState=3.B.timeout(reqA, 2)cut A's connection at about 3 s with A'sidleTimeout: 30.upgrade()already returnedfalse(tag mismatch). The ownership check runs before the tag check, so both cases now throw the same error.server_ptr()dispatches over those too, sorequestIP()/timeout()on the owning server are unchanged (serve-http2.test.tsrequestIP/upgrade tests pass).Rebase notes
Rebased onto main (519963e). The commits are now one commit (the branch had a merge of main in it).
src/runtime/server/server_body.rs: main moved the ended-or-closed test ofupgrade()into a closure. Thecheck_same_servercall sits before it.src/change, on a debug build of main 519963e, the new test fails.src/change (debug build, ASAN), the new test passes. That build also had the change of bake: unregister Bake::SourceProvider from the source map table when it is destroyed #37444, which shares no file with this PR.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts