Skip to content
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

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

Merged
merged 10 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ jobs:
run: pnpm build

- name: Node.js tests
run: pnpm test:node
run: pnpm test:node test/modules/http/compliance/http-res-raw-headers.test.ts

- name: Install Playwright browsers
run: npx playwright install
Expand Down
36 changes: 21 additions & 15 deletions src/interceptors/ClientRequest/MockHttpSocket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
type RequestHeadersCompleteCallback,
type ResponseHeadersCompleteCallback,
} from '_http_common'
import { IncomingMessage, ServerResponse } from 'node:http'
import { STATUS_CODES, IncomingMessage, ServerResponse } from 'node:http'
import { Readable } from 'node:stream'
import { invariant } from 'outvariant'
import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor'
Expand All @@ -13,12 +13,12 @@ import type { NormalizedSocketWriteArgs } from '../Socket/utils/normalizeSocketW
import { isPropertyAccessible } from '../../utils/isPropertyAccessible'
import { baseUrlFromConnectionOptions } from '../Socket/utils/baseUrlFromConnectionOptions'
import { parseRawHeaders } from '../Socket/utils/parseRawHeaders'
import { getRawFetchHeaders } from '../../utils/getRawFetchHeaders'
import {
createServerErrorResponse,
RESPONSE_STATUS_CODES_WITHOUT_BODY,
} from '../../utils/responseUtils'
import { createRequestId } from '../../createRequestId'
import { getRawFetchHeaders } from './utils/recordRawHeaders'

type HttpConnectionOptions = any

Expand Down Expand Up @@ -185,11 +185,12 @@ export class MockHttpSocket extends MockSocket {
const chunkAfterRequestHeaders = chunkString.slice(
chunk.indexOf('\r\n\r\n')
)
const requestHeaders =
getRawFetchHeaders(this.request!.headers) || this.request!.headers
const requestHeadersString = Array.from(requestHeaders.entries())
const rawRequestHeaders = getRawFetchHeaders(this.request!.headers)
const requestHeadersString = rawRequestHeaders
// Skip the internal request ID deduplication header.
.filter(([name]) => name !== INTERNAL_REQUEST_ID_HEADER_NAME)
.filter(([name]) => {
return name.toLowerCase() !== INTERNAL_REQUEST_ID_HEADER_NAME
})
.map(([name, value]) => `${name}: ${value}`)
.join('\r\n')

Expand Down Expand Up @@ -304,8 +305,6 @@ export class MockHttpSocket extends MockSocket {
read() {},
})
)
serverResponse.statusCode = response.status
serverResponse.statusMessage = response.statusText

/**
* @note Remove the `Connection` and `Date` response headers
Expand All @@ -319,17 +318,24 @@ export class MockHttpSocket extends MockSocket {
serverResponse.removeHeader('connection')
serverResponse.removeHeader('date')

const rawResponseHeaders = getRawFetchHeaders(response.headers)

/**
* @note Call `.writeHead` in order to set the raw response headers
* in the same case as they were provided by the developer. Using
* `.setHeader()`/`.appendHeader()` normalizes header names.
*/
serverResponse.writeHead(
response.status,
response.statusText || STATUS_CODES[response.status],
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

rawResponseHeaders
)

// If the developer destroy the socket, gracefully destroy the response.
this.once('error', () => {
serverResponse.destroy()
})

// Get the raw headers stored behind the symbol to preserve name casing.
const headers = getRawFetchHeaders(response.headers) || response.headers
for (const [name, value] of headers) {
serverResponse.setHeader(name, value)
}

if (response.body) {
try {
const reader = response.body.getReader()
Expand Down Expand Up @@ -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() {} })
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

const response = new Response(
Expand Down
11 changes: 11 additions & 0 deletions src/interceptors/ClientRequest/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import { toInteractiveRequest } from '../../utils/toInteractiveRequest'
import { normalizeClientRequestArgs } from './utils/normalizeClientRequestArgs'
import { isNodeLikeError } from '../../utils/isNodeLikeError'
import { createServerErrorResponse } from '../../utils/responseUtils'
import {
recordRawFetchHeaders,
restoreHeadersPrototype,
} from './utils/recordRawHeaders'

export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> {
static symbol = Symbol('client-request-interceptor')
Expand Down Expand Up @@ -104,12 +108,19 @@ export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> {
},
})

// 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()
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What about unmocked requests?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mikicho mikicho Jul 9, 2024

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@mikicho mikicho Jul 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mikicho mikicho Jul 10, 2024

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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


this.subscriptions.push(() => {
http.get = originalGet
http.request = originalRequest

https.get = originalHttpsGet
https.request = originalHttpsRequest

restoreHeadersPrototype()
})
}

Expand Down
170 changes: 170 additions & 0 deletions src/interceptors/ClientRequest/utils/recordRawHeaders.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
type HeaderTuple = [string, string]
type RawHeaders = Array<HeaderTuple>

const kRawHeaders = Symbol('kRawHeaders')
const kRestorePatches = Symbol('kRestorePatches')

function recordRawHeader(headers: Headers, args: HeaderTuple) {
if (Reflect.get(headers, kRawHeaders) == null) {
Object.defineProperty(headers, kRawHeaders, {
value: [],
enumerable: false,
})
}
const rawHeaders = Reflect.get(headers, kRawHeaders) as RawHeaders
rawHeaders.push(args)
}

/**
* Patch the global `Headers` class to store raw headers.
* This is for compatibility with `IncomingMessage.prototype.rawHeaders`.
*
* @note Node.js has their own raw headers symbol but it
* only records the first header name in case of multi-value headers.
* Any other headers are normalized before comparing. This makes it
* incompatible with the `rawHeaders` format.
*
* let h = new Headers()
* h.append('X-Custom', 'one')
* h.append('x-custom', 'two')
* h[Symbol('headers map')] // Map { 'X-Custom' => 'one, two' }
*/
export function recordRawFetchHeaders() {
// Prevent patching the Headers prototype multiple times.
if (Reflect.get(Headers, kRestorePatches)) {
return Reflect.get(Headers, kRestorePatches)
}

const { Request: OriginalRequest, Response: OriginalResponse } = globalThis
const { set, append, delete: headersDeleteMethod } = Headers.prototype

Object.defineProperty(Headers, kRestorePatches, {
value: () => {
Headers.prototype.set = set
Headers.prototype.append = append
Headers.prototype.delete = headersDeleteMethod

globalThis.Request = OriginalRequest
globalThis.Response = OriginalResponse
},
enumerable: false,
})

Headers = new Proxy(Headers, {
construct(target, args, newTarget) {
const headers = Reflect.construct(target, args, newTarget)
const initialHeaders = args[0] || []
const initialRawHeaders = Array.isArray(initialHeaders)
? initialHeaders
: Object.entries(initialHeaders)

// Request/Response constructors will set the symbol
// upon creating a new instance, using the raw developer
// input as the raw headers. Skip the symbol altogether
// in those cases because the input to Headers will be normalized.
if (!Reflect.has(headers, kRawHeaders)) {
Object.defineProperty(headers, kRawHeaders, {
value: initialRawHeaders,
enumerable: false,
})
}

return headers
},
})

Headers.prototype.set = new Proxy(Headers.prototype.set, {
apply(target, thisArg, args: HeaderTuple) {
recordRawHeader(thisArg, args)
return Reflect.apply(target, thisArg, args)
},
})

Headers.prototype.append = new Proxy(Headers.prototype.append, {
apply(target, thisArg, args: HeaderTuple) {
recordRawHeader(thisArg, args)
return Reflect.apply(target, thisArg, args)
},
})

Headers.prototype.delete = new Proxy(Headers.prototype.delete, {
apply(target, thisArg, args: [string]) {
const rawHeaders = Reflect.get(thisArg, kRawHeaders) as RawHeaders

if (rawHeaders) {
for (let index = rawHeaders.length - 1; index >= 0; index--) {
if (rawHeaders[index][0].toLowerCase() === args[0].toLowerCase()) {
rawHeaders.splice(index, 1)
}
}
}

return Reflect.apply(target, thisArg, args)
},
})

Request = new Proxy(Request, {
construct(target, args, newTarget) {
const request = Reflect.construct(target, args, newTarget)

if (
typeof args[1] === 'object' &&
args[1].headers != null &&
!request.headers[kRawHeaders]
) {
request.headers[kRawHeaders] = inferRawHeaders(args[1].headers)
}

return request
},
})

Response = new Proxy(Response, {
construct(target, args, newTarget) {
const response = Reflect.construct(target, args, newTarget)

if (typeof args[1] === 'object' && args[1].headers != null) {
/**
* @note Pass the init argument directly because it gets
* transformed into a normalized Headers instance once it
* passes the Response constructor.
*/
response.headers[kRawHeaders] = inferRawHeaders(args[1].headers)
}

return response
},
})
}

export function restoreHeadersPrototype() {
if (!Reflect.get(Headers, kRestorePatches)) {
return
}

Reflect.get(Headers, kRestorePatches)()
}

export function getRawFetchHeaders(headers: Headers): RawHeaders {
// Return the raw headers, if recorded (i.e. `.set()` or `.append()` was called).
// If no raw headers were recorded, return all the headers.
return Reflect.get(headers, kRawHeaders) || Array.from(headers.entries())
}

/**
* Infers the raw headers from the given `HeadersInit` provided
* to the Request/Response constructor.
*
* If the `init.headers` is a Headers instance, use it directly.
* That means the headers were created standalone and already have
* the raw headers stored.
* If the `init.headers` is a HeadersInit, create a new Headers
* instace out of it.
*/
function inferRawHeaders(headers: HeadersInit): RawHeaders {
if (headers instanceof Headers) {
return Reflect.get(headers, kRawHeaders)
}

return Reflect.get(new Headers(headers), kRawHeaders)
}
50 changes: 0 additions & 50 deletions src/utils/getRawFetchHeaders.test.ts

This file was deleted.

Loading
Loading