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): prevent stacked proxies when intercepting node http(s) requests #697

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sam-super
Copy link
Contributor

@sam-super sam-super commented Jan 3, 2025

It was possible to end up with the http(s).request/get proxies wrapping existing proxies, which can cause odd behaviour:
nock/nock#2802

This PR attempts to fix the issue by only creating a proxy once, and just updating the callbacks the proxy uses when it's called a second time. it's not idea (since it could cause confusion with the previous instance, which will now no longer be invoked), but it seems like a preferable scenario.

instead we could error when an existing proxy is detected, but it seems like it would cause a lot of errors in practice (due to nodes module loader cache being inconsistent).

…ing node http(s) requests

It was possible to end up with the http.request (etc) proxies wrapping existing proxies, which can
cause odd behaviour.
@sam-super
Copy link
Contributor Author

Not sure why but some tests failed when running locally on macos (node 18). they all seem to be due to header being lowercase on my machine:
image
any idea why?

Add logging so we know when we update an existing proxy and fix some linting (remove semicolons)
@mikicho
Copy link

mikicho commented Jan 5, 2025

How does the code pass the check for an already applied interceptor? It seems like this is the problem we need to solve.

@sam-super
Copy link
Contributor Author

How does the code pass the check for an already applied interceptor? It seems like this is the problem we need to solve.

I'm doing some more digging, but the core problem/discrepancy seems to come from this scenario:

  1. Interceptors have been setup and they work
  2. MSW's apply is executed again
  3. globalThis state has been lost (so the global symbols are lost, and the interceptors are re-initialised).
  4. fetch isn't affected (presumably because it's been reset along with the symbols), so MSW just re-patches it
  5. however, the core node:http(s) modules are persisting the patch, which causes the Proxy-stacking.

How we end up with globalThis being reset I'm not sure. So we can narrow it down I'm trying to replicate it without including the jest/nock. Will update...

@mikicho
Copy link

mikicho commented Jan 6, 2025

@sam-super, I think Jest contributes to this by creating a test sandbox. It may wipe out globalThis but not the module cache.

@sam-super
Copy link
Contributor Author

@mikicho yup just tested it and globalThis is reset between each test.
I guess it's then node env:
https://github.com/jestjs/jest/blob/main/packages/jest-environment-node/src/index.ts

@sam-super
Copy link
Contributor Author

So MSW's attempt to use globalThis to maintain a single instance of each interceptor wont work in jest via nodejs.

Because the FetchInterceptor is bound to globalFetch, it currently behaves differently from ClientRequestInterceptor (which is bound to the http module cache).

Ideally maybe jest should reset the native-module cache between each test to remove any state that is attached (i.e. the interceptor proxy). Not sure how feasible it will be though. From my testing jest's own jest.resetModules(); method doesn't fix the issue (the proxy persists between the tests).

I know this PR isn't ideal, but i think preventing the stacking of proxies is probably still a good thing, and if MSW errors on detecting the proxy, it might break a lot of code that uses MSW in node/jest, so updating the proxy to the latest seems the least-bad way IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants