mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
539 stars 123 forks source link

fix(xhr): clone the request before calculating its body size #632

Closed robbtraister closed 3 weeks ago

robbtraister commented 3 weeks ago

The root cause of the issue is that cloning the Request to calculate the Content-Length header occurs too late. At the point in which clone() is called, the body buffer has already been consumed, so cloning fails.

Since this kFetchRequest value is only used for this specific purpose of calculating content-length, there is no harm in creating and storing a clone immediately upon creation of the Request object.

ddolcimascolo commented 3 weeks ago

Hey all, can we get this merged and msw released?

Thx, David

robbtraister commented 3 weeks ago

@kettanaito I tried adding a test, but I can't seem to duplicate the behavior. In the test environment, reading the request buffer does not seem to lock it from future attempts to read/clone.

This is a simplification of what I tried:

  interceptor.once('request', async ({ controller, request }) => {
    await request.arrayBuffer() // <-- this is the addition to the existing test, along with changing to POST
    controller.respondWith(
      new UndiciResponse('Hello world') as unknown as Response
    )
  })
robbtraister commented 3 weeks ago

I think the issue is that the vite environment has its own Request implementation that does not block cloning after consumption.

I was able to figure out a hacky workaround by importing Request from "undici" and setting it to global.Request in the beforeAll hook, then reverting in the afterAll hook. Does that seem reasonable?

ddolcimascolo commented 3 weeks ago

@robbtraister I'm impacted by this one and I'm using Vite + Vitest. Please see https://github.com/mswjs/msw/issues/2273#issuecomment-2340935025

kettanaito commented 3 weeks ago

I was able to figure out a hacky workaround by importing Request from "undici" and setting it to global.Request in the beforeAll hook, then reverting in the afterAll hook. Does that seem reasonable?

Hmm, this seems a bit too much. We are using the Undici's Request. Let me take a look at what we can do to improve these tests, and how to drive out this problematic scenario.

kettanaito commented 3 weeks ago

I've added an isolated test and confirm it failing with the following error without the proposed fix:

TypeError: unusable
 ❯ Request.clone ../node:internal/deps/undici/undici:7289:17
 ❯ XMLHttpRequestController.respondWith ../src/interceptors/XMLHttpRequest/XMLHttpRequestController.ts:299:29
    297|     if (this[kFetchRequest]) {
    298|       const totalRequestBodyLength = await getBodyByteLength(
    299|         this[kFetchRequest].clone()
       |                             ^
    300|       )
    301| 
 ❯ Object.onResponse ../src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts:70:24
 ❯ handleResponse ../src/utils/handleRequest.ts:55:21
 ❯ Module.handleRequest ../src/utils/handleRequest.ts:207:12
 ❯ processTicksAndRejections ../node:internal/process/task_queues:95:5
 ❯ XMLHttpRequestController.xhrRequestController.onRequest ../src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts:64:34

With the fix, the test is passing.

robbtraister commented 3 weeks ago

It looks like the key difference is using the jsdom environment in lieu of happy-dom. Either way, thanks for getting it working.

kettanaito commented 3 weeks ago

Released: v0.35.3 🎉

This has been released in v0.35.3!

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.