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

Bypassed requests miss cookie headers with axios + axios-cookiejar-support #2338

Open
4 tasks done
aristofun opened this issue Oct 29, 2024 · 9 comments
Open
4 tasks done
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed scope:node Related to MSW running in Node

Comments

@aristofun
Copy link

aristofun commented Oct 29, 2024

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

Node.js version

20.17.0

Reproduction repository

https://github.com/aristofun/bugs-js-msw

Reproduction steps

Minimal example: https://github.com/aristofun/bugs-js-msw

Change the msw version to 2.4.3 reinstall and yarn start to see the difference.

Current behavior

After mswServer.listen({ onUnhandledRequest: "bypass" }); all localhost requests issued by axios in the same process are stripped off all cookie headers.

Reproduced on all msw versions starting with 2.4.4

Explicitly adding cookie header like this:

 const result2 = await axios.post("/mypath", {}, { headers: { 'Cookie': 'key=value' } });

Works. That means the problem is with interference with axios-cookiejar-support

Expected behavior

mswServer.listen({ onUnhandledRequest: "bypass" }); should not interfere with axios-cookiejar-support

@aristofun aristofun added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node labels Oct 29, 2024
@aristofun aristofun changed the title Bypassed requests miss cookie headers with axios + tough-cookie Bypassed requests miss cookie headers with axios + axios-cookiejar-support Oct 29, 2024
@kettanaito
Copy link
Member

I suspect this has to do with how axios-cookiejar-support works. If it hides the request headers, or adds them somehow at the wrong time, there will be little MSW should do about it. Given our XHR interception has been stable for years, I suspect that would be the case, sadly. But I will provide you with more details once I take a look at this properly.

@kettanaito
Copy link
Member

Root cause

This was a fun exercise to untangle.

First, axios-cookiejar-support uses a custom HTTP agent:

https://github.com/3846masa/axios-cookiejar-support/blob/a42f8a61de4fbf6af3be195633f94ded3837ff9e/src/index.ts#L36

Then, the custom HttpCookieAgent extends the default HTTP agent and overrides the addRequest method to provision its logic related to cookies:

https://github.com/3846masa/http-cookie-agent/blob/783654f06b54d285d13150c0a8e79c0380c27990/src/http/create_cookie_agent.ts#L126

There was the culprit. Interceptors tap into the createConnection() method of the mock agent but disregarded the addRequest() method on the custom agent.

I've added the fix in mswjs/interceptors#666, alongside the test for this use case.

@aristofun
Copy link
Author

Wow, amazing job! You're saving all those like me who want to have just 1 client (axios) evertywhere - server, client, tests

@aristofun
Copy link
Author

Just as a side note - curious what whas different in msw <2.4.4 in that area

@kettanaito
Copy link
Member

@aristofun, somewhere along those versions we've migrated to a new interception algorithm that's socket-based. That's the main difference.

The issue is non-trivial though. To fix it, we need to call .addRequest of custom agents. But that method likely calls super.addRequest of the default http.Agent, which, in turn, triggers the actual request connection all the time.

Obviously, that won't work when you want to mock a request.

I'm exploring what we can do to allow the .addRequest method of custom agents to run. For now, having thoughts of patching the default http.Agent to make its methods do nothing so that custom agents extending them can safely tap into super without triggering the request. That responsibility will be delegated to the MockAgent that's explicitly set on each request.

Any thoughts are welcome on this!

@kettanaito kettanaito added help wanted Extra attention is needed and removed needs:triage Issues that have not been investigated yet. labels Nov 3, 2024
@aristofun
Copy link
Author

Hmm, what do you think might be downsides of patching default http.Agent?

Sounds to me like you have to do it one way or another, if addRequest is a legitimate part of its API - to catch all possible surfaces of intercepting requests. Unless I miss something.

@kettanaito
Copy link
Member

Hmm, what do you think might be downsides of patching default http.Agent?

Apart from meddling with another object, not much. I would love not to do that if we can, but I don't see how. We have to make sure custom agents taping into super.addRequest() don't initiate the actual network connection, which that method does.

Sounds to me like you have to do it one way or another, if addRequest is a legitimate part of its API - to catch all possible surfaces of intercepting requests.

Kind of. I treat this as a niche use case but it would be great to support it anyway.

@aristofun
Copy link
Author

I'm happy to test alpha version of your solution on my personal project where this issue originally came from. If that helps.

@kettanaito
Copy link
Member

Keep an eye on this issue to know when there's an update. We publish preview releases now so you will have what to try, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed scope:node Related to MSW running in Node
Projects
None yet
Development

No branches or pull requests

2 participants