fix(ClientRequest): support "rawHeaders" in Fetch API Headers #598

kettanaito · 2024-07-06T13:39:02Z

Related to Nock compatibility #575
Follow-up from Fix response raw headers #487

Changes

We are now patching the global Headers class to record its raw headers as they come in from the developer. That's not ideal but our proxy is transparent and doesn't change the behavior in any way, just gathers the input (header fields).

The Fetch API disregards header fields case sensitivity on all layers:

When constructing a new Headers, raw headers are lost.
When constructing a new Request/Response and giving it a Headers instance, the original header fields are lost.

kettanaito · 2024-07-06T14:45:45Z

Hi, @mikicho. Can you please tell me more about this use case? We are effectively testing how Fetch API Headers behave, and they don't nest headers with the same name how we expect here.

Interceptors construct a correct Response instance. Maybe there's something else we should be testing on the IncomingMessage level?

mikicho · 2024-07-06T14:53:31Z

@kettanaito IIRC, the current response differs from what Node.js returns with http.get request.
I guess that Response logic is differs from the IncomingMessage logic

kettanaito · 2024-07-06T16:55:09Z

Got it.

It's a bit complicated. Headers don't store the headers in the same way. Besides, the class only stores the original raw header name of the first of the multi-value headers set:

let h = new Headers()
h.append('X-Custom', 'one')
h.append('x-custom', 'one')

This instance will only store the raw name as X-Custom (precisely that casing). And it will use the same casing for any subsequent header value assignments of the same name, even if the casing of those header names differs.

For us to prevent that, we need to spy on Headers.prototype.append and keep our own map of raw header names (or better, assignment calls). Not sure how much I like the idea of that.

mikicho · 2024-07-06T17:08:20Z

@kettanaito, I put some thought into this a few months ago.
Node.js rawHeaders and Response have some gaps.
IMHO, it isn't worth it to fill these gaps. Instead, I suggest we add a rawHeaders-like property to our Response and use this for the ClientRequest.
This has two advantages:

We won't rely on internal symbols and behavior as we do today.
This is a non-intrusive approach, and we can remove this property easily once (if..) the ClientRequest is deprecated, or Node.js will expose better API for HTTP mocking as Undici has.

kettanaito · 2024-07-06T17:22:52Z

I don't mind trying to breach the gap here but I see a problem: Node.js itself lowecases the header names before setting them:

https://github.com/nodejs/node/blob/3a456c6db802b5b25594d3a9d235d4989e9f7829/lib/_http_outgoing.js#L764

Even if I record .append() calls and provide the header names in the same casing as they were set on Response, all of them still get lowercased anyway, resulting in wrong rawHeaders. A bit confused.

What you are proposing means a completely new API just to set rawHeaders. We must not modify the Response global API with user-facing changes.

kettanaito · 2024-07-06T17:29:36Z

@mikicho, can you please show me the original Nock test behind this issue?

If you interface with OutgoingMessage, it will normalize the header names and store them that way in rawHeaders. The only way that's not the case is if the headers were received from the server (i.e. from the parsed HTTP response message). I wonder how you achieve that in Nock.

kettanaito · 2024-07-06T17:34:24Z

Okay, I found how one can set raw headers: using ServerResponse.prototype.writeHead. The test is now passing 🎉 Still wonder how Nock users are doing this.

mikicho · 2024-07-06T17:39:10Z

can you please show me the original Nock test behind this issue?

https://github.com/nock/nock/blob/ca9e61674a5e31086dd766f1f2d917b21ae4b358/tests/got/test_reply_headers.js#L58
https://github.com/nock/nock/blob/ca9e61674a5e31086dd766f1f2d917b21ae4b358/tests/got/test_reply_headers.js#L272

Also, I do some work around headers when creating the Response object: https://github.com/nock/nock/blob/dac5a26cf655e4b7fc6382f90ec883bd5538096b/lib/create_response.js#L37-L59

I wonder how you achieve that in Nock.

Nock returns IncomingMessage to the user.

kettanaito · 2024-07-07T12:23:53Z

The main difficult here is that in Fetch API, this is one header:

headers.append('X-Custom-Header', 'one')
headers.append('x-custom-header', 'two')

While in OutgoingMessage, these are two separate headers:

X-Custom-Header: one
x-custom-header: two

No matter what solution we ship, it will be incomplete due to the design of the Fetch API headers class.

Moreover, headers behavior across Node.js versions differs. The tests we have now pass on v18.19, fail on v18.20. Since by the spec all headers are lowecased, none of those changes are treated as breaking.

Edit: I can confirm that since v18.19, Node.js doesn't even keep the map of raw header names anymore. The header names are lowercased as soon as they enter Headers, and then stored like that both externally and internally.

Here's a Symbol('headers list') of a Headers instance:

new Headers({ 'X-Custom-Header': 'yes' })

HeadersList {
  cookies: null,
  [Symbol(headers map)]: Map(1) {
    'x-custom-header' => { name: 'x-custom-header', value: 'Yes' }
  },
  [Symbol(headers map sorted)]: [ [ 'x-custom-header', 'Yes' ] ]
}

mikicho · 2024-07-07T14:31:02Z

No matter what solution we ship, it will be incomplete due to the design of the Fetch API headers class.

This is an annoying gap, indeed. This is why I think we should create a separate flow between these two, if possible.

kettanaito · 2024-07-08T09:31:16Z

@mikicho, I somewhat siding with the fetch api though. Header fields are case-insensitive per spec. rawHeaders make very little sense to me. I can't think of a use case when knowing the original casing of a header would matter. Although, this may just be me. I'm not a backend developer, in the end.

Anyhow, I managed to record the raw headers with a bit of transparent proxying over the Headers constructor. That satisfies the headers test cases but breaks a few others. I suspect my implementation is just not correct. Working on fixing that.

mikicho · 2024-07-08T10:32:06Z

@kettanaito Thank you for working on this!

rawHeaders make very little sense to me.

AFAIK

Proxy servers.
Some servers do not respect case insensitivity.
it may be good for debugging.

kettanaito · 2024-07-09T17:03:24Z

@mikicho, makes sense.

I've pushed the last patch that should give us consistent raw headers recording and have the existing tests passing. Could you please give this PR a try before we merge this? I wonder how it complies with Nock. Thanks.

kettanaito · 2024-07-09T17:06:14Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+     */
+    serverResponse.writeHead(
+      response.status,
+      response.statusText || STATUS_CODES[response.status],


We've deleted this prematurely. Probably cached test results but this was failing. We guarantee to set a response status code even if none was explicitly provided. This isn't what Fetch API does but we do this.

kettanaito · 2024-07-09T17:06:29Z

src/interceptors/ClientRequest/MockHttpSocket.ts

@@ -535,7 +541,7 @@ export class MockHttpSocket extends MockSocket {

    // Similarly, create a new stream for each response.
    if (canHaveBody) {
-      this.responseStream = new Readable()
+      this.responseStream = new Readable({ read() {} })


Was also erroring. Likely an oversight. read() must always be implemented.

kettanaito · 2024-07-09T17:06:47Z

src/interceptors/ClientRequest/index.ts

+    // Spy on `Header.prototype.set` and `Header.prototype.append` calls
+    // and record the raw header names provided. This is to support
+    // `IncomingMessage.prototype.rawHeaders`.
+    recordRawFetchHeaders()


I'm enabling the raw headers recording as a part of the ClientRequest interceptor. This is the only place we need it for now.

What about unmocked requests?

The behavior should be the same for bypassed requests. Let me check tomorrow if I have tests to prove that.

@kettanaito Seems like it is working 🎉
I added a test. If you meant something else, feel free to remove this.
Also, I updated the minimum node version in the package.json to 18.20.0

@kettanaito For some reason, I get this error:

Exception has occurred: TypeError: Cannot assign to read only property 'Symbol(kRawHeaders)' of object '#<_Headers>'
at Object.construct (/..../mswjs-interceptors/lib/node/chunk-CFRXZJO4.js:192:39)
at createResponse (/..../nock/lib/create_response.js:43:20)
at OverriddenClientRequest. (/.../nock/lib/intercept.js:405:28)
at OverriddenClientRequest.emit (node:events:530:35)
at Timeout.respond [as _onTimeout] (/.../nock/lib/playback_interceptor.js:307:11)
at listOnTimeout (node:internal/timers:573:17)
at process.processTimers (node:internal/timers:514:7)

I'm wondering if I do something wrong:

function createResponse(message: IncomingMessage) { ... const rawHeaders = new Headers() for (let i = 0; i < message.rawHeaders.length; i += 2) { rawHeaders.append(message.rawHeaders[i], message.rawHeaders[i + 1]) } const response = new Response(responseBodyOrNull, { status: message.statusCode, statusText: message.statusMessage || STATUS_CODES[message.statusCode], headers: rawHeaders, }) return response }

The error is thrown from this line.

The errors disappear when I set writable: true for kRawHeaders.
I'm wondering why it works for interceptors tests.

I also see a problem with the apply and restore functionality. It could be how I implemented it in Nock. I'll continue to investigate this tomorrow.

@mikicho, thanks for the tests! I will look at them once I have a moment.

Also, I updated the minimum node version in the package.json to 18.20.0

Please, this would be best introduced on its own. Let's not add it as a part of this pull request.

The "cannot set symbol" error is caused because something tries to set it when it's already set. Interesting, I thought I put guards against that.

The errors disappear when I set writable: true for kRawHeaders.

This isn't a solution, it simply means one actor can override previously stored raw headers. This is actually quite dangerous because fetch api primitives repeatedly construct Headers instances.

const h = new Headers() // ^ This is one headers instance. const r = new Response(null, { headers: h }) r.headers // ^ And this will be a completely different // headers instance constructed from "h" as init.

Please, this would be best introduced on its own. Let's not add it as a part of this pull request.

Reverted the change

This isn't a solution...

Got you!

I will look at that test today, see what I can find.

kettanaito · 2024-07-11T10:55:09Z

test/modules/http/compliance/http-res-raw-headers.test.ts

+  expect(res.rawHeaders).toEqual(
+    expect.arrayContaining(['X-CustoM-HeadeR', 'Yes'])
+  )
+  expect(res.headers['x-custom-header']).toEqual('Yes')


This looks great!

kettanaito · 2024-07-11T10:56:38Z

All the tests are passing. @mikicho, let me know about that apply/restore issue you found. Otherwise, I think this is good to go.

kettanaito · 2024-07-11T16:58:05Z

Released: v0.32.2 🎉

This has been released in v0.32.2!

📄 Release notes
📦 npm package

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

test: add failing test

31db9ee

kettanaito mentioned this pull request Jul 6, 2024

Fix response raw headers #487

Closed

kettanaito changed the title ~~test: add failing test~~ fix(ClientRequest): fold response headers with the same name Jul 6, 2024

kettanaito added 2 commits July 7, 2024 12:46

fix(ClientRequest): record raw Headers

3e4fcf8

fix: patch and restore Headers in interceptor

50bdac0

kettanaito force-pushed the fix/response-headers-folding branch from 41ac23d to d2c93a0 Compare July 7, 2024 12:24

chore(wip): debug

6800c35

kettanaito force-pushed the fix/response-headers-folding branch from d2c93a0 to 6800c35 Compare July 7, 2024 12:26

fix: patch Request/Response constructors

c814337

Merge branch 'main' into fix/response-headers-folding

5d2f998

kettanaito marked this pull request as ready for review July 9, 2024 17:05

kettanaito requested a review from mikicho July 9, 2024 17:05

chore: run all node tests on ci

5d47f15

kettanaito commented Jul 9, 2024

View reviewed changes

kettanaito changed the title ~~fix(ClientRequest): fold response headers with the same name~~ fix(ClientRequest): support "rawHeaders" in Fetch API Headers Jul 9, 2024

mikicho added 3 commits July 10, 2024 00:00

add test for unmocked request

d0c4383

fix test name

37a5513

revert package.json

a295abd

kettanaito commented Jul 11, 2024

View reviewed changes

kettanaito added the release candidate label Jul 11, 2024

kettanaito merged commit 4660558 into main Jul 11, 2024
2 checks passed

kettanaito deleted the fix/response-headers-folding branch July 11, 2024 16:51

mikicho mentioned this pull request Jul 11, 2024

fix(ClientRequest): improve rawHeaders recording #602

Merged

mikicho mentioned this pull request Sep 30, 2024

feat: update the node.js version range #648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ClientRequest): support "rawHeaders" in Fetch API Headers #598

fix(ClientRequest): support "rawHeaders" in Fetch API Headers #598

kettanaito commented Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024

kettanaito commented Jul 7, 2024 •

edited

Loading

mikicho commented Jul 7, 2024

kettanaito commented Jul 8, 2024 •

edited

Loading

mikicho commented Jul 8, 2024 •

edited

Loading

kettanaito commented Jul 9, 2024

kettanaito Jul 9, 2024

kettanaito Jul 9, 2024

kettanaito Jul 9, 2024

mikicho Jul 9, 2024

kettanaito Jul 9, 2024

mikicho Jul 9, 2024 •

edited

Loading

mikicho Jul 9, 2024 •

edited

Loading

mikicho Jul 9, 2024

mikicho Jul 9, 2024

kettanaito Jul 10, 2024

mikicho Jul 10, 2024 •

edited

Loading

kettanaito Jul 11, 2024

kettanaito Jul 11, 2024

kettanaito commented Jul 11, 2024

kettanaito commented Jul 11, 2024

fix(ClientRequest): support "rawHeaders" in Fetch API Headers #598

fix(ClientRequest): support "rawHeaders" in Fetch API Headers #598

Conversation

kettanaito commented Jul 6, 2024 • edited Loading

Changes

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024 • edited Loading

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024

kettanaito commented Jul 7, 2024 • edited Loading

mikicho commented Jul 7, 2024

kettanaito commented Jul 8, 2024 • edited Loading

mikicho commented Jul 8, 2024 • edited Loading

kettanaito commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikicho Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

mikicho Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikicho Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito commented Jul 11, 2024

kettanaito commented Jul 11, 2024

Released: v0.32.2 🎉

kettanaito commented Jul 6, 2024 •

edited

Loading

mikicho commented Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 7, 2024 •

edited

Loading

kettanaito commented Jul 8, 2024 •

edited

Loading

mikicho commented Jul 8, 2024 •

edited

Loading

mikicho Jul 9, 2024 •

edited

Loading

mikicho Jul 9, 2024 •

edited

Loading

mikicho Jul 10, 2024 •

edited

Loading