Skip to content

fix: keep a header named __proto__ across features - #5688

Open
luantaraschi wants to merge 1 commit into
nodejs:mainfrom
luantaraschi:fix/proto-header-util
Open

fix: keep a header named __proto__ across features#5688
luantaraschi wants to merge 1 commit into
nodejs:mainfrom
luantaraschi:fix/proto-header-util

Conversation

@luantaraschi

Copy link
Copy Markdown

This relates to...

Supersedes #5666, #5667, #5668 and #5669. This is the consolidation you asked for in #5666 (comment) and confirmed in #5667 (comment): one util, used across the features. Thanks for the steer, it turned four near-identical diffs into one.

Rationale

Writing a header named __proto__ onto a plain object reaches the Object.prototype setter instead of creating a property. A string value is dropped without a trace. An array value replaces the accumulator's prototype, so the object loses its own methods.

parseHeaders in lib/core/util.js already guards that with Object.defineProperty. Four other accumulators did not, and each of the superseded PRs carried its own copy of the guard. Three of them had the identical setHeader function written out three times, which is what you spotted.

Changes

The guard becomes setHeader in lib/core/util.js, exported next to parseHeaders, and the four sites call it:

  • lib/web/fetch/headers.js, the entries object built from the headers list
  • lib/util/cache.js, appendHeader and makeDeduplicationKey
  • lib/mock/snapshot-utils.js and lib/mock/snapshot-recorder.js, header normalizing and filtering
  • lib/dispatcher/client-h2.js, buildRequestHeaders

Three of those read a header back before writing it, so they now check with Object.hasOwn. A plain read resolves __proto__ through the prototype chain instead of reporting the header as absent, and that is what made two different requests share a deduplication key.

Two notes on scope, both easy to change if you see it differently:

parseHeaders keeps its inline guard rather than calling the util. It runs once per header on every response, so routing it through a function call is a hot path change this PR does not need to make. Happy to fold it in if you would rather have a single caller shape everywhere.

#5663 is not part of this and stays as it is. It gives getCookies a null prototype accumulator, so there is no header write to route through the util, and it builds a cookie record rather than a header record.

Features

N/A

Bug Fixes

A header named __proto__ survives the fetch Headers list, the cache key and deduplication key, the mock snapshot recorder, and the HTTP/2 request header builder, instead of being dropped or replacing the accumulator's prototype.

Breaking Changes and Deprecations

None. setHeader is a new export on lib/core/util.js, nothing else changed shape.

Testing

Five new tests, one per site plus the deduplication key collision. Each of them fails on main and passes here:

test/prototype-headers.js                pass=2 fail=2   (fetch, h2)
test/cache-interceptor/cache-utils.js    pass=11 fail=1
test/snapshot-recorder.js                pass=14 fail=1
test/interceptors/deduplicate.js         pass=34 fail=1

With the change, on Node 22.20.0 on Windows: test/prototype-headers.js 4 passing, test/cache-interceptor 77 passing, test/interceptors/deduplicate.js 35 passing, test/snapshot-recorder.js 15, test/snapshot-testing.js 32, test/snapshot-redirect-interceptor.js 1, test/fetch/headers.js 66, test/util.js 46 passing with 1 skipped, and every test/http2-*.js file green including http2-request-never-settles.js. eslint is clean.

Status

Writing a header named `__proto__` onto a plain object reaches the
Object.prototype setter instead of creating a property. A string value is
dropped without a trace, and an array value replaces the accumulator's
prototype, which takes every method of that object with it.

`parseHeaders` in lib/core/util.js already guarded the case with
Object.defineProperty. That guard becomes an exported `setHeader` util and the
four remaining accumulators use it: the entries object of the fetch Headers
list, the cache key and deduplication key builders, the mock snapshot header
normalizers, and the HTTP/2 request header builder.

The three accumulators that also read a header back before writing now check
with Object.hasOwn, because a plain read resolves `__proto__` through the
prototype chain rather than reporting the header as absent.
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