Skip to content

fix(decompress): treat inherited Object.prototype names as unsupported encodings - #5807

Open
NgoQuocViet2001 wants to merge 1 commit into
nodejs:mainfrom
NgoQuocViet2001:fix-decompress-inherited-encoding
Open

fix(decompress): treat inherited Object.prototype names as unsupported encodings#5807
NgoQuocViet2001 wants to merge 1 commit into
nodejs:mainfrom
NgoQuocViet2001:fix-decompress-inherited-encoding

Conversation

@NgoQuocViet2001

Copy link
Copy Markdown

Problem

supportedEncodings is a plain object literal, so the unsupported-encoding guard resolves inherited names through the prototype chain:

const supportedEncodings = {
  gzip: createGunzip,
  ...
}
...
if (!supportedEncodings[encoding]) {
  decompressors.length = 0
  return decompressors           // unsupported -> pass through
}
decompressors.push(supportedEncodings[encoding]())

onResponseStart lowercases the header before the lookup, and exactly two Object.prototype own-property names survive lowercasing — constructor and __proto__. Both are truthy, so both walk past the guard and get called.

Effect

Measured against main with a plain node:http origin and a client composed with interceptors.decompress():

content-encoding: "identity"          -> ok: status=200 body=hello
content-encoding: "bogus-encoding"    -> ok: status=200 body=hello
content-encoding: "constructor"       -> HUNG - promise never settled
content-encoding: "__proto__"         -> threw: TypeError: supportedEncodings[encoding] is not a function
content-encoding: "gzip, constructor" -> HUNG - promise never settled

constructor is the bad one: Object() returns a plain object, that object is pushed into the decompressor chain as if it were a stream, and client.request() then neither resolves nor rejects. No uncaughtException, no teardown — the caller's await and the socket both leak. A remote origin (or any intermediary that rewrites Content-Encoding) can wedge a client with a single response header.

An unrecognised encoding is supposed to pass through untouched, which is what bogus-encoding shows.

Fix

Give the lookup table a null prototype, so only its own entries can match. This matches the existing style in lib/core/util.js:973, lib/util/runtime-features.js:7 and lib/web/eventsource/eventsource.js:458.

const supportedEncodings = {
  __proto__: null,
  gzip: createGunzip,
  ...
}

All five cases above then pass through cleanly.

Test plan

  • Added three cases to test/interceptors/decompress.js beside the existing should pass through unsupported encoding, covering constructor, __proto__ and gzip, constructor.
  • Ran: node --test test/interceptors/decompress.js38 passing.
  • Checked: reverting only the __proto__: null line makes the constructor case hang rather than fail — the test run never terminates, which is the defect itself.
  • Ran: npx standard on both touched files → clean.

…d encodings

supportedEncodings is a plain object literal, so the guard

  if (!supportedEncodings[encoding])

resolves inherited names through the prototype chain instead of rejecting
them. onResponseStart lowercases the header first, which leaves exactly two
reachable from the wire: constructor and __proto__.

A response carrying Content-Encoding: constructor calls Object(), pushes the
plain object it returns into the decompressor chain, and the request then
never settles; __proto__ throws TypeError out of the interceptor. Give the
table a null prototype so only its own entries can match.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.50%. Comparing base (cf3e133) to head (f563277).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5807      +/-   ##
==========================================
- Coverage   93.50%   93.50%   -0.01%     
==========================================
  Files         110      110              
  Lines       39555    39556       +1     
==========================================
  Hits        36987    36987              
- Misses       2568     2569       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants