Repository navigation
http-transport: safe cors defaults, request body size cap - #270
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe HTTP transport adds configurable request body size limits with HTTP 413 handling across Node, Bun, and the server layer. CORS handling now restricts credentials to explicitly allowed origins and preserves ChangesHTTP transport hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpTransportServer
participant BodySizeGuard
participant RPCHandler
Client->>HttpTransportServer: Send request body
HttpTransportServer->>BodySizeGuard: Stream body through configured limit
BodySizeGuard->>RPCHandler: Forward body within limit
BodySizeGuard-->>HttpTransportServer: Raise PayloadTooLargeError over limit
HttpTransportServer-->>Client: Return HTTP 413
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/http-transport/src/server.ts (1)
433-455: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against
nullfrom the CORS callback.typeof result === 'object'also matchesnull, so anullreturn will throw here and surface as a generic 500 instead of skipping CORS headers.Proposed fix
- } else if (typeof result === 'object') { + } else if (result && typeof result === 'object') {🤖 Prompt for 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. In `@packages/http-transport/src/server.ts` around lines 433 - 455, Update the CORS callback handling in the `#corsOptions` function branch to exclude null before treating result as an object. A null callback result must skip CORS header generation without throwing, while preserving the existing boolean and non-null object behavior.
🧹 Nitpick comments (2)
packages/http-transport/src/server.ts (1)
178-215: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBuffered branch skips the early declared-size rejection used by the blob branch.
The blob/cannotDecode branch rejects immediately when
Content-Lengthdeclares an oversized body (Lines 184-187). The buffered/decodable branch (Lines 200-211) has no equivalent early check — it only rejects once bytes accumulate past the cap, doing unneeded work for requests that are already known to be oversized. Also, a non-numericContent-LengthyieldsNaN(Line 182), which silently bypasses the early check entirely (NaN > capis alwaysfalse); worth guarding withNumber.isNaN.♻️ Suggested consolidation
+ const contentLength = request.headers.get('content-length') + const declaredSize = contentLength ? Number.parseInt(contentLength, 10) : undefined + if ( + typeof declaredSize === 'number' && + !Number.isNaN(declaredSize) && + declaredSize > this.#maxRequestBodySize + ) { + throw new PayloadTooLargeError() + } if (isBlob || cannotDecode) { - const type = contentType || 'application/octet-stream' - const contentLength = request.headers.get('content-length') - const size = contentLength - ? Number.parseInt(contentLength, 10) - : undefined - // Declared size over the cap: reject before reading anything - if (size !== undefined && size > this.#maxRequestBodySize) { - throw new PayloadTooLargeError() - } + const type = contentType || 'application/octet-stream' + const size = Number.isNaN(declaredSize as number) ? undefined : declaredSize const clientStream = new ProtocolClientStream(-1, { size, type }) ...🤖 Prompt for 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. In `@packages/http-transport/src/server.ts` around lines 178 - 215, Consolidate the Content-Length parsing and validation before the isBlob/cannotDecode branch so every request path, including the buffered decoder flow, rejects a declared body larger than `#maxRequestBodySize` before reading it. Treat a non-numeric Content-Length as invalid using Number.isNaN rather than allowing NaN to bypass the size check, while preserving the existing streaming received-size guard.packages/http-transport/src/runtimes/node.ts (1)
76-99: 🚀 Performance & Scalability | 🔵 TrivialCapped uploads still keep the socket busy. Later chunks are dropped in userland once
cappedis set, but the request isn’t aborted, so oversized bodies can keep consuming bandwidth until the client finishes sending them. If that waste matters, this needs a transport-level abort/pause path;res.close()alone won’t reliably stop in-flight chunks.🤖 Prompt for 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. In `@packages/http-transport/src/runtimes/node.ts` around lines 76 - 99, The oversized-body path in the ReadableStream start handler must stop the underlying uWS request, not merely set capped and discard later chunks. Update the maxBodySize overflow handling around res.onDataV2 to invoke the transport-level abort or pause mechanism that reliably halts in-flight upload delivery, while preserving PayloadTooLargeError propagation and avoiding reliance on res.close() alone.
🤖 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.
Outside diff comments:
In `@packages/http-transport/src/server.ts`:
- Around line 433-455: Update the CORS callback handling in the `#corsOptions`
function branch to exclude null before treating result as an object. A null
callback result must skip CORS header generation without throwing, while
preserving the existing boolean and non-null object behavior.
---
Nitpick comments:
In `@packages/http-transport/src/runtimes/node.ts`:
- Around line 76-99: The oversized-body path in the ReadableStream start handler
must stop the underlying uWS request, not merely set capped and discard later
chunks. Update the maxBodySize overflow handling around res.onDataV2 to invoke
the transport-level abort or pause mechanism that reliably halts in-flight
upload delivery, while preserving PayloadTooLargeError propagation and avoiding
reliance on res.close() alone.
In `@packages/http-transport/src/server.ts`:
- Around line 178-215: Consolidate the Content-Length parsing and validation
before the isBlob/cannotDecode branch so every request path, including the
buffered decoder flow, rejects a declared body larger than `#maxRequestBodySize`
before reading it. Treat a non-numeric Content-Length as invalid using
Number.isNaN rather than allowing NaN to bypass the size check, while preserving
the existing streaming received-size guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7586892-1c55-457a-819d-538842607241
📒 Files selected for processing (10)
packages/http-transport/src/constants.tspackages/http-transport/src/runtimes/bun.tspackages/http-transport/src/runtimes/node.tspackages/http-transport/src/server.tspackages/http-transport/src/types.tspackages/http-transport/src/utils.tspackages/http-transport/tests/_helpers/test-utils.tspackages/http-transport/tests/body-limit.spec.tspackages/http-transport/tests/cors.spec.tspackages/http-transport/vitest.config.ts
This was referenced Jul 15, 2026
This was referenced Jul 30, 2026
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.
Fixes the first two items of #214 (the WS-token-in-URL item needs coordinated transport/protocol work and stays open).
cors: trueno longer reflects arbitrary origins with credentials: the defaults sentAccess-Control-Allow-Credentials: truewhile reflecting the request origin verbatim, letting any website make credentialed requests against a cookie-authed API. Credentials are now granted only when the config supplies an explicit origin allowlist (string array, object form, or a vetting function whose returned allowlist actually contains the requesting origin — a returned list is matched, not trusted). The params type became a union that makesorigin: true+allowCredentialsa compile error while still accepting composedtrue | string[]values; a JS caller passing the vulnerable combo has it ignored at runtime. Origin-dependent responses now also emitVary: Origin(merged with existing Vary values) so shared caches can't serve one origin's CORS response to another.Buffer.concat(await bodyStream.toArray())) with no backpressure — a single large request could exhaust memory. A newmaxRequestBodySizeoption (default 128 MiB, matching Bun's native default so all runtimes behave identically) is enforced incrementally on every body path: declared oversizedContent-Lengthis rejected before reading, the buffering loop and a byte-counting transform on blob/undecodable streams abort with 413Payload Too Largethe moment the running total exceeds the cap, and stream errors propagate through an awaited pipeline so an oversized upload yields a catchable 413 instead of an uncaught stream error. The option is also wired intoBun.serve(runtime-specific setting wins).Tests cover: credentialed vs credential-less config shapes (through the public type, with compile-time fixtures), the vetting-function allowlist match and mismatch, Vary merging, and 413s for oversized buffered/blob/undecodable/declared bodies on both the direct handler and the real uWS adapter — asserting the server survives and serves the next request.
Summary by CodeRabbit
Vary: Originresponses.Varyresponse header values.