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

XMLHttpRequest failing to construct Response when body status is null #379

Closed
disco-panda opened this issue May 25, 2023 · 11 comments · Fixed by #411
Closed

XMLHttpRequest failing to construct Response when body status is null #379

disco-panda opened this issue May 25, 2023 · 11 comments · Fixed by #411
Assignees

Comments

@disco-panda
Copy link

disco-panda commented May 25, 2023

Screenshot 2023-05-25 at 12 00 56 PM Screenshot 2023-05-25 at 12 05 54 PM Screenshot 2023-05-25 at 12 13 10 PM

When using the XMLHttpRequestInterceptor these errors are thrown, crashing the webpage.

It does not appear when using other interceptors.

This is the code I am using -

import { BatchInterceptor } from '@mswjs/interceptors'
import { FetchInterceptor } from '@mswjs/interceptors/fetch'
import { XMLHttpRequestInterceptor } from '@mswjs/interceptors/XMLHttpRequest'


const interceptor = new BatchInterceptor({
    name: 'my-interceptor',
    interceptors: [new FetchInterceptor(), new XMLHttpRequestInterceptor()],
})

interceptor.apply()

interceptor.on('request', async (e: any, request: any) => {
    console.log("request", await e.clone().json())
})

interceptor.on('response', async (res: any, request: any) => {
    console.log("response", await res.clone().json())
})
@disco-panda
Copy link
Author

Appears similar to #378

@kettanaito
Copy link
Member

Hi, @disco-panda. Thanks for reporting this.

So there seems to be two issues going on:

  1. The request gets terminated due to the forbidden request header (likely).
  2. While the request is terminated, the library still attempts to construct a Response with incorrect input data.

I would love to put this use case into an integration test. This should be fairly easy to emulate with our testing setup.

The actual bug here is that we should never construct the Response instance if the request has been terminated.

@kettanaito kettanaito self-assigned this May 25, 2023
@disco-panda
Copy link
Author

Thank you so much for the fast response and great explanation of what is going on!

@kettanaito kettanaito moved this to Interceptors in Roadmap Jul 30, 2023
@avivasyuta
Copy link
Contributor

Hello.

This issue is completely unrelated to passing headers from #378.
The essence of the problem is that the interceptor is trying to construct a new Response() with a body for responses that, according to the specification, cannot have a body.

Here body is equal empty string "", but for http statuses 101, 103, 204, 205, 304 it must be null.

@kettanaito
Copy link
Member

@avivasyuta, looks like we need to do a bit of response body transformations since XHR responses don't directly translate to Fetch API responses. Please, would you have a chance to open a pull request with a failing test for this? We can collaborate on it and ship the fix in a timely manner. I would be thankful!

@avivasyuta
Copy link
Contributor

Yes, I'm just trying to figure out how to write an test for this scenario. I'll open a PR when it is ready.

@kettanaito
Copy link
Member

I believe we need to model a two scenarios (translate to 2 tests):

  1. An XHR interceptor that has no request listener (no mocking) and instead requests an actual HTTP server that responds with one of the responses you mentioned that mustn't have bodies, like a 204 No Content for a GET request. We then add a response listener on the test and assert that it returns a valid response representation.
  2. An XHR interceptor that has a request listener that returns the same response but a mocked one. This is just to make sure nothing goes awry between the consumer and the actual response arriving at the XHR (since the validity of the constructed response is now on the consumer's shoulders we can still, potentially, have a bug in the interceptor that would not handle that response correctly). In this test, we'd instead assert the xhr.response returned on the request instance.

You can take inspiration from the existing tests under test/modules/XMLHttpRequest, which are plenty and feature both of the above scenarios.

@avivasyuta
Copy link
Contributor

avivasyuta commented Sep 1, 2023

The trick is that the original XMLHttpRequest will have xhr.response === '' even for responses with 204 HTTP status code. Also if we create new response with null body than it text variant will equal empty string ``.

const resp = new Response(null)
await resp.text() // ''

So I can't figure out how to track the changes in the test.
I made a simple test that always passes. But in the console we can see that an error occurs.

Screenshot 2023-09-01 at 21 53 46

@kettanaito
Copy link
Member

I think that's the expected behavior as we channel runtime exceptions from Playwright but don't treat them as test failures to allow us to test those exceptions as intended.

Looks good! Please open a pull request with that if you haven't already.

The trick is that the original XMLHttpRequest will have xhr.response === '' even for responses with 204 HTTP status codes.

I see. Our goal is to manually check the response status code and prevent anything but null as the response body in those special cases.

Also if we create new response with null body than it text variant will equal empty string ``.

This is also correct. The proper way to get null response body back is by response.body, which will be a ReadableStream if the response has a body to stream, or null if the response body was set to null when constructing the response.

@avivasyuta
Copy link
Contributor

It seems to me that I've realised how to fix and test this issue.
I've made a PR.

Please see if this is a good approach when you have time so I can proceed.

@kettanaito
Copy link
Member

Released: v0.24.1 🎉

This has been released in v0.24.1!

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.

@kettanaito kettanaito removed this from Roadmap Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants