Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/web/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2110,7 +2110,15 @@ async function httpNetworkFetch (
const headersList = new HeadersList()

for (let i = 0; i < rawHeaders.length; i += 2) {
headersList.append(bufferToLowerCasedHeaderName(rawHeaders[i]), rawHeaders[i + 1].toString('latin1'), true)
const nameStr = bufferToLowerCasedHeaderName(rawHeaders[i])
Copy link
Member

Choose a reason for hiding this comment

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

As this only breaks given an interceptor, I'd prefer that we fix it within undici's rather than fetch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea is that fetch should properly support arrays of header values in rawHeaders, if such case is valid.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I'd prefer to cross check with the spec to see what it has to say regarding duplicated headers, if the implementation already aligns, most likely is an undici issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, RFC 7230 Section 3.2.2 means that:

  • A sender must not send multiple headers with the same name unless it is Set-Cookie,
  • A receiver may combine values of multiple headers with the same name into a comma-separated list, except for Set-Cookie.

I'd say it is fine to have them separated in Undici/interceptor APIs, and only join them in fetch.

Copy link
Contributor

Choose a reason for hiding this comment

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

You refer to RFC7230 but not the fetch spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that HTTP-network fetch doesn't really specify how headers should be treated while being received? It merely says:

Follow the relevant requirements from HTTP. [HTTP] [HTTP-CACHING]

Wait until all the HTTP response headers are transmitted.

in which "[HTTP]" refers to RFC9110. In that document, section 5.2 and 5.3 are basically a rephrase of RFC7230 section 3.2.2.

In undici, the relevant code is in httpNetworkFetch->dispatch->onHeaders, and I believe is an implementation detail. IIUC, changes to this part of the code should count as fixing the issue in undici.

const value = rawHeaders[i + 1]
if (Array.isArray(value) && !Buffer.isBuffer(rawHeaders[i + 1])) {
for (const val of value) {
headersList.append(nameStr, val.toString('latin1'), true)
}
} else {
headersList.append(nameStr, value.toString('latin1'), true)
}
}
const contentEncoding = headersList.get('content-encoding', true)
if (contentEncoding) {
Expand Down
45 changes: 45 additions & 0 deletions test/issue-4389.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
const { describe, test } = require('node:test')
const assert = require('node:assert')
const { once } = require('node:events')
const { createServer: createHttpServer } = require('node:http')
const { createServer: createSocketServer } = require('node:net')
const { fetch, Agent } = require('..')

describe('Interceptor should not break multi-value headers', () => {
test('Interceptor should not break Set-Cookie', async (t) => {
const server = createHttpServer({ joinDuplicateHeaders: true }, async (_req, res) => {
const headers = [
['set-cookie', 'a=1'],
['set-cookie', 'b=2'],
['set-cookie', 'c=3']
]
res.writeHead(200, headers)
res.end()
}).listen(0)

await once(server, 'listening')
t.after(() => server.close())

const dispatcher = new Agent().compose((dispatch) => dispatch)

const { headers } = await fetch(`http://localhost:${server.address().port}`, { dispatcher })
assert.deepStrictEqual(headers.getSetCookie(), ['a=1', 'b=2', 'c=3'])
})

test('Interceptor should not break other multi-value header', async (t) => {
const server = createSocketServer((socket) => {
socket.write('HTTP/1.0 204 No Content\r\n')
socket.write('X-Test-Header: 1\r\n')
socket.write('X-Test-Header: 2\r\n')
socket.write('X-Test-Header: 3\r\n')
socket.end('\r\n\r\n')
}).listen(0)
t.after(() => server.close())
await once(server, 'listening')

const dispatcher = new Agent().compose((dispatch) => dispatch)

const { headers } = await fetch(`http://localhost:${server.address().port}`, { dispatcher })
assert.deepStrictEqual(headers.get('x-test-header'), '1, 2, 3')
})
})