-
-
Notifications
You must be signed in to change notification settings - Fork 155
fix(fetch): sanitize header tuples to fix Node.js 24 compatibility #766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,9 +113,15 @@ export function recordRawFetchHeaders() { | |
| headersInit instanceof Headers && | ||
| Reflect.has(headersInit, kRawHeaders) | ||
| ) { | ||
| // Ensure each header tuple has exactly 2 elements (name, value). | ||
| // Node.js 24+ may have stored tuples with extra internal arguments. | ||
| const rawHeadersFromInit = Reflect.get(headersInit, kRawHeaders) as RawHeaders | ||
| const sanitizedHeaders = rawHeadersFromInit.map( | ||
| (tuple): HeaderTuple => [tuple[0], tuple[1]] | ||
| ) | ||
| const headers = Reflect.construct( | ||
| target, | ||
| [Reflect.get(headersInit, kRawHeaders)], | ||
| [sanitizedHeaders], | ||
| newTarget | ||
| ) | ||
| ensureRawHeadersSymbol(headers, [ | ||
|
|
@@ -124,7 +130,7 @@ export function recordRawFetchHeaders() { | |
| * This prevents multiple Headers instances from pointing | ||
| * at the same internal "rawHeaders" array. | ||
| */ | ||
| ...Reflect.get(headersInit, kRawHeaders), | ||
| ...sanitizedHeaders, | ||
| ]) | ||
| return headers | ||
| } | ||
|
|
@@ -149,14 +155,20 @@ export function recordRawFetchHeaders() { | |
|
|
||
| Headers.prototype.set = new Proxy(Headers.prototype.set, { | ||
| apply(target, thisArg, args: HeaderTuple) { | ||
| recordRawHeader(thisArg, args, 'set') | ||
| // Use only the first two arguments (name, value) to record raw headers. | ||
| // Node.js 24+ may pass additional internal arguments that should not | ||
| // be included in the raw headers array. | ||
| recordRawHeader(thisArg, [args[0], args[1]], 'set') | ||
| return Reflect.apply(target, thisArg, args) | ||
| }, | ||
| }) | ||
|
|
||
| Headers.prototype.append = new Proxy(Headers.prototype.append, { | ||
| apply(target, thisArg, args: HeaderTuple) { | ||
| recordRawHeader(thisArg, args, 'append') | ||
| // Use only the first two arguments (name, value) to record raw headers. | ||
| // Node.js 24+ may pass additional internal arguments that should not | ||
| // be included in the raw headers array. | ||
| recordRawHeader(thisArg, [args[0], args[1]], 'append') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are your thoughts on doing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to change it to Performance-wise, Let me know your preference and I'll update the PR. |
||
| return Reflect.apply(target, thisArg, args) | ||
| }, | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a reference to Node.js implementation of those additional arguments to
Headers? What do they affect?By not recording them, are we risking recreating a
Headersinstance that's not a 1-1 representation of the one recorded?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 3rd boolean argument is passed internally during Headers operations — it's not part of the public Headers API spec which only defines
set(name, value)andappend(name, value)with 2 arguments.Regarding 1-1 representation: No risk here. The boolean is purely internal (likely for validation skipping or provenance tracking). It's never exposed through any public Headers API (
entries(),get(),forEach(), etc.), so it cannot affect actual header name/value data. The recorded raw headers will still be a 1-1 representation of the actual header data.