Skip to content

fix: normalizeHeaders throws on flat header arrays from dns re-dispatch#5535

Open
GiHoon1123 wants to merge 1 commit into
nodejs:mainfrom
GiHoon1123:fix-issue-5522-normalize-headers-interceptor-order
Open

fix: normalizeHeaders throws on flat header arrays from dns re-dispatch#5535
GiHoon1123 wants to merge 1 commit into
nodejs:mainfrom
GiHoon1123:fix-issue-5522-normalize-headers-interceptor-order

Conversation

@GiHoon1123

Copy link
Copy Markdown

This relates to...

Fixes #5522

Rationale

normalizeHeaders() in lib/util/cache.js (shared by the cache and deduplicate interceptors) only recognized headers as a plain object or an iterable of [key, value] pairs. It didn't recognize the flat, alternating [key, value, key, value, ...] array that the dns interceptor's withHostHeader() produces when it re-dispatches after resolving an address -- the same low-level shape core/util.js and dispatcher/client-h1.js already consume on the way to the wire.

Because compose() wraps arguments innermost-first, .compose(cache(), dns()) makes dns the outer interceptor and cache its "next" -- so dns's post-resolution re-dispatch lands back in cache's (or deduplicate's) normalizeHeaders() with headers already in that shape, which got misread as a malformed pairs array and threw.

Changes

  • normalizeHeaders(): detect the flat-array shape (first element is not itself an array) before falling through to the existing pairs-array handling.
  • Widened the value check (both the new flat-array branch and the pre-existing pairs branch) to accept string[] as well as string, matching what client-h1.js already accepts for repeated headers like Set-Cookie -- a plain string-only check would have thrown again on that case.
  • Updated normalizeHeaders's JSDoc @param/@returns to reference the actual accepted input (DispatchOptions, whose .headers is UndiciHeaders -- already covers object/array/iterable) instead of the stale Record<string, string[] | string>.
  • Added unit tests for normalizeHeaders directly (test/cache-interceptor/cache-utils.js): flat array, empty flat array, flat array with an array value, tuple-style array with an array value, and two invalid-value cases that should still throw.
  • Added end-to-end regression tests in test/interceptors/dns.js reproducing the exact dns -> cache and dns -> deduplicate re-dispatch path with a local server and mocked lookup (no real DNS/network needed).

Features

N/A

Bug Fixes

Breaking Changes and Deprecations

N/A -- this only widens what normalizeHeaders() accepts; nothing that previously worked changes shape or behavior.

Status

normalizeHeaders() in lib/util/cache.js (shared by the cache and
deduplicate interceptors) only recognized headers as a plain object or
an iterable of [key, value] pairs. It didn't recognize the flat,
alternating [key, value, key, value, ...] array that the dns
interceptor's withHostHeader() produces when it re-dispatches after
resolving an address -- the same low-level shape core/util.js and
dispatcher/client-h1.js already consume.

Because compose() wraps arguments innermost-first, .compose(cache(),
dns()) makes dns the outer interceptor and cache its "next", so dns's
post-resolution re-dispatch lands back in cache's normalizeHeaders()
with headers already in that shape, misread as a broken pairs array.

Detect the flat-array shape by checking whether the first element is
itself an array. Also widened the value check (both the new branch and
the existing pairs branch) to accept string[] as well as string,
matching what client-h1.js already accepts for repeated headers like
Set-Cookie -- the stricter string-only check would have thrown again
on that case.
Comment thread lib/util/cache.js
return true
}

return Array.isArray(val) && val.every(v => typeof v === 'string')

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.

use a proper for loop, .every() is slow.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.45%. Comparing base (3c662a5) to head (9be4822).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5535   +/-   ##
=======================================
  Coverage   93.44%   93.45%           
=======================================
  Files         110      110           
  Lines       37329    37352   +23     
=======================================
+ Hits        34883    34908   +25     
+ Misses       2446     2444    -2     

☔ 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.

normalizeHeaders fails with opts.headers is not a valid header map depending on interceptor ordering

3 participants