mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.98k stars 519 forks source link

The cookies object recevied in the handlers varies depending on the placement of the handler within the `setupServer` function. #2126

Closed NZepeda closed 4 months ago

NZepeda commented 7 months ago

Prerequisites

Environment check

Node.js version

18.12.1

Reproduction repository

https://github.com/NZepeda/msw-cookies-example

Reproduction steps

  1. git clone https://github.com/NZepeda/msw-cookies-example
  2. cd msw-cookies-example
  3. pnpm install
  4. pnpm test mock

Current behavior

In my test, I've set the cookie to be sent with the request via the store.add function of @mswjs/cookies. I have a few handlers for my server that check the received cookie from the request and test it against a static string authorizedSessionCookieValue.

The first test passes as the received cookie value is the same as authorizedSessionCookieValue, but the second test fails. This seems to be due to the placement of the handler within the setupServer function. It seems like the first handler (which the first test hits) receives the correct cookie value, in my case { MyCookie: 'ABCD1234' }, however, the third handler (which the second test hits) receives a cookie value of { MyCookie: 'ABCD1234, MyCookie=ABCD1234' }, which causes the test to fail.

Expected behavior

I would expect the received cookies object from each handler to be the same.

kettanaito commented 7 months ago

Hi, @NZepeda!

It may not be the best idea to use @mswjs/cookie that way. You are interfering with how MSW picks up cookies that way, it seems. If you want to have an initial cookie set, mutate document.cookie instead. In other words, do what your app would do.

I also have a concern that you aren't testing anything in your test: https://github.com/NZepeda/msw-cookies-example/blob/d7d900d6e0202c8b8a522b1c88b4958b8782c181/src/test/mock.test.ts. What the test cases assert right now is that MSW handles cookies correctly. If this is a trimmed test case you created to illustrate the issue then it's okay. But if it's not, don't test third-party libraries, test your code instead.

antoinetissier commented 7 months ago

Hi @kettanaito,

What we are trying to simulate is a prod setup with an Http-Only, Secure cookie. In a browser Http-Only are not readable/mutable through document.cookie.

Is there no way to set this up in msw + jsdom ?

This is related to https://github.com/mswjs/msw/issues/1586

kettanaito commented 7 months ago

@antoinetissier 👋

Since you cannot read the HttpOnly cookie in JavaScript, what would you test then? Nothing in your application can depend on it. I suspect you are talking about forwarding that cookie to the server request (i.e. credentials)? If that's the case, it's done by the browser for you.

Please tell me more about your test case.

antoinetissier commented 7 months ago

We use a cookie to authenticate a WebSocket connection to the server. We get that cookie by sending a request to the server with a JWT, and if it is valid the server responds with a Set-Cookie header where the cookie is HttpOnly and Secure. The cookie is stored in the browser securely (as it's not accessible through JavaScript with document.cookie), and sent upon further server requests (we adapt the credentials request property if necessary for cross-domain deployments). As mentioned above, this allows to then open a WebSocket in particular, but the cookie can also be used to authenticate any other further HTTP request to the server (our Java uses Spring security and this is the default behavior).

When the UI assets (JS, HTML, css, etc.) are served by the server itself (which is the deployment that we recommended), then when you try connecting to the UI, you must first pass the Spring security layer before the UI assets can be fetched. In that case, by the time any JavaScript is run, the session cookie is already set in the user's browser.

We are looking to test all this authentication flow with msw.

NZepeda commented 7 months ago

To add on to this, @kettanaito, as you recommended above, I've removed usage of the @mswjs/cookie package to set the cookie and instead, I'm directly mutating document.cookie. The same error persists. I've updated the linked repo to demonstrate the issue.

For the test, I've set up a server with three handlers, two of which depend on the cookies object given by the resolver function. These handlers check the cookie similarly by comparing cookies[sessionCookieName] to an expected cookie value.

Within the test suite itself, I set the session cookie once by mutating document.cookie within the beforeAll. There are two tests, each hitting a different handler. The first test passes because the cookie has the expected value. The second test fails because the cookie value is now unexpected.

This seems like a bug within msw because I expect that both tests should pass because the cookie was only mutated once in the beforeAll. I do not expect the cookies object given by the resolver to change between handlers simply due to their order within the setupServer function.

For more clarity: This is the value of the cookies object given by the resolver in the first handler:

{ MyCookie: 'ABCD1234' } <- This is good

This is the value of the cookies object given the resolver in the third handler:

{ MyCookie: 'ABCD1234, MyCookie=ABCD1234' } <- This is bad

Can you confirm whether or not this is expected?

kettanaito commented 4 months ago

I looked into this and found out that the cookie handling was broken since v2.0. After we've migrated to the Fetch API, we haven't accounted for the constrains of that API when it comes to storing response cookies.

I'm addressing all the issues I found in #2206. Let me see if I can put your scenario in a test, and whether we don't already have one. I'm certain the pull request fixes it.

kettanaito commented 4 months ago

So, there's a couple of things going on.

  1. Don't use @mswjs/cookies. It's not meant to be used as an API to affect cookies externally out of MSW. That won't work. We are moving away from that library as well as there's a semi-standard tough-cookie while we are waiting for the standard CookieStore to get full browser support.

  2. If you want to affect cookies, set Set-Cookie header on the mocked response. This will work in the browser and in Node.js (once #2206 is released). That is your primary way of setting a cookie.

  3. If you want to model a scenario for testing where some document cookies exist, use document.cookie directly. The browser has you covered, and in JSDOM you will be subjected to that environment's correctness of implementing the setter.

For now, wait for the next release, and give it a try.