-
-
Notifications
You must be signed in to change notification settings - Fork 664
fix(dns): preserve header shape and ensure Host #4498
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?
Conversation
lib/interceptor/dns.js
Outdated
@@ -5,6 +5,43 @@ const DecoratorHandler = require('../handler/decorator-handler') | |||
const { InvalidArgumentError, InformationalError } = require('../core/errors') | |||
const maxInt = Math.pow(2, 31) - 1 | |||
|
|||
function addHostHeader (headers, host) { |
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.
Hmm, you don't need to parse the headers by yourself, this is done by undici automatically, we just need to standardise the way that these are forward to undici.
If Array, push host
to the headers array, if an object, attach host
as a new property
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.
I've simplified the code!
A header case I'm slightly worried about though is non-array iterables.
request({ origin, path: '/', method: 'GET', headers: new Set([['f0', 'b0'], ['f1', 'b1']]) })
.
These headers are valid under UndiciHeaders
and a request like this (though rare) is possible; but the proposed array/object check would break this.
Another issue I've noticed is [string,string][] headers always raise an error due to the way header processing is handled in Request.js
. Say headers = [['f0', 'b0']] or [['f0', 'b0'], ['f1', 'b1']]
if (Array.isArray(headers)) {
if (headers.length % 2 !== 0) {
throw new InvalidArgumentError('headers array must be even')
}
for (let i = 0; i < headers.length; i += 2) {
processHeader(this, headers[i], headers[i + 1])
}
}
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.
These headers are valid under UndiciHeaders and a request like this (though rare) is possible; but the proposed array/object check would break this.
Usually these are iterables, and we iterate over them, but not use utility functions; if that's the case, we can try to also check for Symbol.iterator
to decide what to do with them.
Say headers = [['f0', 'b0']] or [['f0', 'b0'], ['f1', 'b1']]
That's somehow expected, we only expect them in the format [string, string]
, anything outside should throw
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.
Handling iterators might mean we can't avoid some normalization. However this can be streamlined with the normalizeHeaders
function also used in cache.js
Line 36 in ad4d547
function normalizeHeaders (opts) { |
diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index 38287607..a878f39f 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -4,6 +4,15 @@ const { lookup } = require('node:dns')
const DecoratorHandler = require('../handler/decorator-handler')
const { InvalidArgumentError, InformationalError } = require('../core/errors')
const maxInt = Math.pow(2, 31) - 1
+const { normalizeHeaders } = require('../util/cache.js')
+
+function addHostHeader (opts, host) {
+ if (Array.isArray(opts.headers)) {
+ return ['host', host, ...opts.headers]
+ }
+ const headers = normalizeHeaders(opts)
+ return { host, ...headers }
+}
class DNSInstance {
#maxTTL = 0
@@ -411,10 +420,7 @@ module.exports = interceptorOpts => {
...origDispatchOpts,
servername: origin.hostname, // For SNI on TLS
origin: newOrigin.origin,
- headers: {
- host: origin.host,
- ...origDispatchOpts.headers
- }
+ headers: addHostHeader(origDispatchOpts, origin.host)
}
dispatch(
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.
Hmm... that's a good catch; I'd suggest to move the normalize headers to core/utils
and implement it within the DNS interceptor first, and we can check if we wanna do the same for the core request in a further issue
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.
Hmm... that's a good catch; I'd suggest to move the normalize headers to
core/utils
and implement it within the DNS interceptor first, and we can check if we wanna do the same for the core request in a further issue
This makes sense to me
lib/interceptor/dns.js
Outdated
function addHostHeader (headers = {}, host) { | ||
if (Array.isArray(headers)) { | ||
const header = ['host', host] | ||
if (Array.isArray(headers[0])) { |
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 we really support them? They should throw if request
receives a multi-dimensional array.
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.
They do throw for arrays, just not other iterables
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.
Got it, let's do this: #4498 (comment)
lib/interceptor/dns.js
Outdated
} | ||
return [...header, ...headers] | ||
function addHostHeader (opts, host) { | ||
if (Array.isArray(opts.headers)) { |
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 normalizeHeaders
will normalize it to a plain JS object, feel free to append it once normalized.
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.
Not for flat arrays for some reason. Refactored to accept types of [k1, v1, k2, v2, ...]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
- Coverage 94.17% 93.56% -0.61%
==========================================
Files 90 103 +13
Lines 24559 32366 +7807
==========================================
+ Hits 23128 30283 +7155
- Misses 1431 2083 +652 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lib/core/util.js
Outdated
if (typeof src[Symbol.iterator] === 'function') { | ||
const msg = 'opts.headers is not a valid header map' | ||
for (const s of src) { | ||
if (!Array.isArray(s)) throw new Error(msg) |
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.
If there's an Undici error that is usable, let's make usage of it
This relates to...
#4444
Rationale
Preserve header shape and correct Host after DNS resolution to avoid routing issues/header corruption
Changes
Added
addHostHeader
to dns interceptor and parameterized tests for different header types.Bug Fixes
Fixed DNS interceptor turning [string, string][]/iterables into numeric-key headers
Status