perf(node): fast header lookups and fewer per-request allocations#218
Conversation
Restores the request header get()/has() fast path that #217 removed (reading req.headers directly instead of always materializing a native Headers from rawHeaders), while keeping its WHATWG correctness: - Repeated single-value headers (authorization, content-type, ...) still fall back to the rawHeaders-materialized Headers for spec ", " joins. - A literal __proto__ header (invisible in req.headers) falls back too. - Prototype artifacts (toString, ...) are rejected via Object.hasOwn and typeof guards; invalid names and HTTP/2 pseudo-headers throw TypeError like native Headers. - Repeated cookie headers need no fallback: Node and the Fetch spec both join with "; ". Also trims response-path allocations: - FastResponse._toNodeResponse() now emits a flat rawHeaders-style list, removing the per-response .flat() in writeHead, the intermediate Object.entries().map() array, and a double toLowerCase per header. Header names are now always lowercased (matching native Response). - readBody() returns single-chunk bodies directly instead of copying through Buffer.concat. Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughThis PR adds Node-specific header collapsing and validation semantics to ChangesNode Request Header Semantics
Flat Header Array Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/node-headers.test.ts (1)
103-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
has(":path")pseudo-header branch too.Line 106 covers
get(":path"), buthas()has its own pseudo-header fallback path; add the matching assertion to lock that behavior down.🧪 Proposed test addition
expect(() => headers.get("foo bar")).toThrow(TypeError); expect(() => headers.has("foo bar")).toThrow(TypeError); expect(() => headers.get(":path")).toThrow(TypeError); + expect(() => headers.has(":path")).toThrow(TypeError);🤖 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 `@test/node-headers.test.ts` around lines 103 - 107, Add a missing assertion in the node-headers test to cover the pseudo-header fallback in Headers.has as well as Headers.get. In the test block around headers.get(":path"), include a matching expectation for headers.has(":path") throwing TypeError, using the existing headers variable and the same invalid-name case so both methods are locked down consistently.
🤖 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 `@src/adapters/_node/headers.ts`:
- Around line 92-118: The fast path in Headers.get and Headers.has is validating
too late, which can let invalid non-ASCII names alias ASCII keys and return the
wrong result. Update the logic in src/adapters/_node/headers.ts so header-name
validation happens before lowercasing and before any req.headers lookup, and
keep the native Headers fallback for invalid names or pseudo-headers. Make the
change in both get() and has() so their behavior stays consistent with native
Headers semantics.
In `@src/adapters/_node/response.ts`:
- Around line 183-188: The content-length handling in the response header
builder is dropping valid zero-length bodies because the check in the response
assembly logic treats 0 as falsy. Update the logic around the headers push in
the response adapter so it explicitly distinguishes “present” from “nonzero” and
still emits content-length: 0 for empty string, ArrayBuffer, Uint8Array,
DataView, and Blob responses, while keeping the existing hasContentLength guard.
---
Nitpick comments:
In `@test/node-headers.test.ts`:
- Around line 103-107: Add a missing assertion in the node-headers test to cover
the pseudo-header fallback in Headers.has as well as Headers.get. In the test
block around headers.get(":path"), include a matching expectation for
headers.has(":path") throwing TypeError, using the existing headers variable and
the same invalid-name case so both methods are locked down consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b60ac83-9bd2-4be2-92d8-cec611a3d792
📒 Files selected for processing (6)
src/adapters/_node/headers.tssrc/adapters/_node/request.tssrc/adapters/_node/response.tssrc/adapters/_node/send.tstest/node-adapters.test.tstest/node-headers.test.ts
| const lower = name.toLowerCase(); | ||
| if (lower.charCodeAt(0) === 58 /* : */) { | ||
| // HTTP/2 pseudo-header: invalid WHATWG name → native TypeError | ||
| return this._headers.get(name); | ||
| } | ||
| const value = this.#req.headers[lower]; | ||
| if (typeof value === "string") { | ||
| // Node collapses repeated single-value headers (authorization, | ||
| // content-type, …) to their first occurrence in `req.headers`, | ||
| // diverging from WHATWG ", " join semantics. Deopt to the | ||
| // rawHeaders-materialized Headers only when such a header actually | ||
| // repeats. | ||
| return _nonJoinedHeaders.has(lower) && _isRepeated(this.#req.rawHeaders, lower) | ||
| ? this._headers.get(name) | ||
| : value; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| // Only set-cookie is array-valued in `req.headers` | ||
| return value.join(", "); | ||
| } | ||
| // Absent, or a non-string artifact from `req.headers`'s prototype | ||
| // (`toString`, …). A real `__proto__` header never lands as an own key | ||
| // (the prototype accessor swallows Node's assignment) and invalid names | ||
| // need native error semantics — both read from the materialized Headers. | ||
| return lower !== "__proto__" && _validHeaderNameRE.test(name) | ||
| ? null | ||
| : this._headers.get(name); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the invalid-name alias case against native Headers semantics.
node - <<'NODE'
const invalidName = "\u212A"; // Kelvin sign; invalid HTTP field-name char
console.log("lowercase:", invalidName.toLowerCase());
try {
new Headers({ k: "v" }).get(invalidName);
console.log("unexpected: did not throw");
} catch (error) {
console.log("native throws:", error.constructor.name);
}
NODERepository: h3js/srvx
Length of output: 2025
🌐 Web query:
WHATWG Headers.get invalid header name TypeError non-ASCII ByteString lowercasing alias Kelvin sign
💡 Result:
In the WHATWG Fetch standard, the Headers.get method (and other Headers interface methods) requires that the provided header name be a valid HTTP header name [1][2]. If a name is passed that does not match the field-name production in the HTTP specification, the method throws a TypeError [1][2][3]. Regarding the specific technical concerns in your query: 1. ByteString and Non-ASCII Characters: The Headers interface operates using ByteStrings, which are defined as sequences of bytes where each byte is restricted to the range 0-255 [4]. Because HTTP header names and values are strictly defined as ASCII in their underlying protocol requirements, passing non-ASCII characters (or characters that cannot be represented in a ByteString) to Headers methods typically results in errors (such as TypeError) [4][5][6][7]. 2. Kelvin Sign (U+212A) and Lowercasing: The Kelvin sign (K) is a known Unicode character that can cause issues with case-folding and normalization [8]. Because it looks like the letter "K", some implementations may incorrectly normalize or lowercase it into an ASCII "k" [8]. This behavior has been exploited in security vulnerabilities, such as HTTP Request Smuggling, where an attacker crafts headers that are interpreted differently by frontend and backend systems due to inconsistent Unicode handling [8]. 3. Lowercasing of Header Names: While earlier versions of the Fetch specification experimented with forcing lowercase on header names within the API, the current specification maintains a byte-case-insensitive match for header names [9][10]. This allows developers to pass headers with mixed casing, which is preserved by the API while still permitting standard-compliant case-insensitive retrieval [2][10]. The interaction between these factors—specifically the requirement for strict ByteString compliance, the sensitivity of security parsers to Unicode normalization, and the case-insensitive nature of HTTP header matching—is critical for ensuring robustness and preventing protocol desynchronization attacks [8][7].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Headers
- 3: https://devdoc.net/web/developer.mozilla.org/en-US/docs/Web/API/Headers/get.html
- 4:
Argument is not a ByteStringwhen setting headers with non-ASCII characters nodejs/undici#1590 - 5: Can't correctly set
ByteStringheaders with non-ASCII chars fastify/fastify#3808 - 6: MCP server discovery failure: ByteString conversion error when using non-ASCII characters in headers google-gemini/gemini-cli#25668
- 7: Panics with non-ASCII header value http-rs/tide#853
- 8: https://dev.to/cverports/cve-2025-69224-absolute-zero-security-smuggling-requests-into-aiohttp-with-the-kelvin-sign-391f
- 9: Headers.get/getAll/has do not byte-lowercase the name whatwg/fetch#203
- 10: Stop lowercasing header names whatwg/fetch#476
Validate header names before the fast path. Lowercasing first lets invalid non-ASCII names alias ASCII keys and return a value/true instead of matching native Headers behavior. Move validation ahead of the req.headers lookup in both get() and has().
🤖 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 `@src/adapters/_node/headers.ts` around lines 92 - 118, The fast path in
Headers.get and Headers.has is validating too late, which can let invalid
non-ASCII names alias ASCII keys and return the wrong result. Update the logic
in src/adapters/_node/headers.ts so header-name validation happens before
lowercasing and before any req.headers lookup, and keep the native Headers
fallback for invalid names or pseudo-headers. Make the change in both get() and
has() so their behavior stays consistent with native Headers semantics.
| if (contentType && !hasContentTypeHeader) { | ||
| headers.push(["content-type", contentType]); | ||
| headers.push("content-type", contentType); | ||
| } | ||
| if (contentLength && !hasContentLength) { | ||
| headers.push(["content-length", String(contentLength)]); | ||
| headers.push("content-length", String(contentLength)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Falsy 0 content-length is silently dropped.
contentLength && !hasContentLength treats a zero-length body (empty string, empty ArrayBuffer/Uint8Array/DataView/Blob) as falsy, so the content-length: 0 header is never emitted for those responses.
🩹 Proposed fix
- if (contentLength && !hasContentLength) {
+ if (contentLength != null && !hasContentLength) {
headers.push("content-length", String(contentLength));
}📝 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.
| if (contentType && !hasContentTypeHeader) { | |
| headers.push(["content-type", contentType]); | |
| headers.push("content-type", contentType); | |
| } | |
| if (contentLength && !hasContentLength) { | |
| headers.push(["content-length", String(contentLength)]); | |
| headers.push("content-length", String(contentLength)); | |
| } | |
| if (contentType && !hasContentTypeHeader) { | |
| headers.push("content-type", contentType); | |
| } | |
| if (contentLength != null && !hasContentLength) { | |
| headers.push("content-length", String(contentLength)); | |
| } |
🤖 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 `@src/adapters/_node/response.ts` around lines 183 - 188, The content-length
handling in the response header builder is dropping valid zero-length bodies
because the check in the response assembly logic treats 0 as falsy. Update the
logic around the headers push in the response adapter so it explicitly
distinguishes “present” from “nonzero” and still emits content-length: 0 for
empty string, ArrayBuffer, Uint8Array, DataView, and Blob responses, while
keeping the existing hasContentLength guard.
2 participants
Fixes a Node adapter performance regression introduced by the recent security hardenings (#217) and further reduces per-request work to keep srvx fast.
Changes
get()/has()/getSetCookie()readreq.headersdirectly again instead of always materializing a nativeHeadersfromrawHeadersper call. The fix(node): read duplicate single-value headers from rawHeaders in get/has #217 correctness is kept: repeated single-value headers (authorization,content-type, …) and a literal__proto__header still fall back to the materializedHeaders, prototype artifacts (toString, …) are rejected, and invalid names / HTTP/2 pseudo-headers throwTypeErrorlike nativeHeaders. Repeatedcookieheaders need no fallback (Node and the Fetch spec both join with"; "). Verified with a differential test against the rawHeaders-materialized reference through Node's real parser.FastResponse._toNodeResponse()emits a flat rawHeaders-style list, removing the per-response.flat(), an intermediateObject.entries().map()array, and a doubletoLowerCaseper header. Header names are now always lowercased on the wire (matching nativeResponse).Buffer.concatalloc+copy.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Headersbehavior, including repeated headers, special header names, and__proto__edge cases.Tests