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

credentials config not supported in node? #189

Closed
kentcdodds opened this issue Jun 1, 2020 · 12 comments
Closed

credentials config not supported in node? #189

kentcdodds opened this issue Jun 1, 2020 · 12 comments
Labels
bug Something isn't working scope:node Related to MSW running in Node

Comments

@kentcdodds
Copy link
Contributor

Describe the bug

When I set the credentials config in fetch to 'omit' that isn't passed along to the req.

Environment

  • msw: 0.0.18
  • nodejs: 12
  • npm: 6.14.5

To Reproduce

kentcdodds/bookshelf@b733434#diff-47664a78a0147eab6cc64883365d1ef4R75-R76

test('allows for config overrides', async () => {
  let request
  const endpoint = 'test-endpoint'
  const mockResult = {mockValue: 'VALUE'}
  server.use(
    rest.get(`${apiURL}/${endpoint}`, async (req, res, ctx) => {
      request = req
      return res(ctx.json(mockResult))
    }),
  )

  const customConfig = {
    credentials: 'omit',
    headers: {'Content-Type': 'fake-type'},
  }

  await client(endpoint, customConfig)

  // TODO: this should pass but it's not...
  // expect(request.credentials).toBe(customConfig.credentials)
  expect(request.headers.get('Content-Type')).toBe(
    customConfig.headers['Content-Type'],
  )
})

Expected behavior

I expect the request.credentials to bet set to what is set in the window.fetch call.

@kentcdodds kentcdodds added the bug Something isn't working label Jun 1, 2020
@kettanaito
Copy link
Member

Hey, @kentcdodds! Thank you for raising this.

I think the culprit here is that an intercepted request in Node doesn't have the same properties as the one issued in a browser. To comply with the promised req object, those missing properties are hard-coded:

const mockedRequest: MockedRequest = {
url: req.url,
method: req.method,
// Parse the request's body based on the "Content-Type" header.
body: parseRequestBody(req.body, requestHeaders),
headers: requestHeaders,
params: {},
redirect: 'manual',
referrer: '',
keepalive: false,
cache: 'default',
mode: 'cors',
referrerPolicy: 'no-referrer',
integrity: '',
destination: 'document',
bodyUsed: false,
credentials: 'same-origin',
}

You can see a high-level overview on what issues requests in NodeJS depending on the context.

Since you use fetch, I can assume either jsdom, or something like node-fetch. In case of jsdom, window.fetch is a wrapper around XMLHttpRequest, so I could try to investigate what it does with the given credentials options.

@kettanaito kettanaito added the scope:node Related to MSW running in Node label Jun 2, 2020
@kentcdodds
Copy link
Contributor Author

JSDOM does not support fetch so I'm using whatwg-fetch.

@kettanaito
Copy link
Member

I'm pretty sure it also relies on XMLHttpRequest. Taking a look at whatwg-fetch source code, we can see how it handlers the credentials request option:

https://github.com/github/fetch/blob/74e4fe8a4731b4e233ac7da42c960a351fd3400f/fetch.js#L507-L511

I'd have to research what XHR does with .withCredentials, and how I can retrieve that as a part of interception.

@kettanaito
Copy link
Member

After some investigation, I don't think there's a reliable way to propagate the credentials value to the intercepted req instance. See more technical details below.

Request issuer

Depending on the request issuing library you use to polyfill window.fetch, the handling of the credentials property will be different.

node-fetch

node-fetch doesn't support RequestInit.credentials, so you cannot set them.

isomorphic-fetch

isomorphic-fetch uses node-fetch for the execution in a Node environment, so the handling of credentials is identical to the one in node-fetch.

whatwg-fetch

whatwg-fetch propagates the credentials value to the XMLHttpRequest request instance by setting the xhrRequest.withCredentials: boolean property.

Resolution

What we can do is to support those request issuing libraries that use XMLHttpRequest, since it's the only client-side instance that handlers credentials. However, XMLHttpRequest request doesn't store the credentials value, only the flag whether credentials were used (boolean).

The optimal solution would be something like this:

// `this` refers to a `XMLHttpRequest` instance
const requestCredentials: RequestCredentials = this.withCredentials
  ? 'same-origin'
  : 'omit'

This will not give you a full I/O compliance between what you provide to fetch and what you assert in the intercepted req instance.

@kentcdodds
Copy link
Contributor Author

That's too bad. Well, I suggest do the best we can and document the limitation. I wonder if whatwg-fetch would be open to a contribution to handle this considering this is not compliant to the spec and compliance is the goal.

@kettanaito
Copy link
Member

That is not the issue with whatwg-fetch, however. They do a great job on mapping credentials: X into xhrRequest.withCredentials. The issue is that it cannot be mapped backwards. Consider:

if (['same-origin', 'include'].includes(options.credentials) {
  xhrRequest.withCrendetials = true
}
// in a different universe, that knows only about `xhrRequest`:
if (xhrRequest.withCredentials) {
  interceptedRequest.credentials = 'same-origin' // but can also be 'include'
}

The original input to fetch is lost when working with the underlying XMLHttpRequest.

@kentcdodds
Copy link
Contributor Author

Hmmm... But in a real scenario, if I make a fetch request to a backend that expects the credentials option to be a string on the request object, that won't work with the polyfill but it would work with a spec-complient implementation right?

@kimroen
Copy link

kimroen commented Jun 4, 2020

But in a real scenario, if I make a fetch request to a backend that expects the credentials option to be a string on the request object, that won't work with the polyfill but it would work with a spec-complient implementation right?

Sorry for jumping in to an ongoing discussion like this, but in a real scenario, wouldn't you receive cookies or not receive cookies on the server? I don't think you can tell from the request what setting it had initially.

@kentcdodds
Copy link
Contributor Author

Yeah, I've personally never used the credentials field, so I really don't know. I'm just thinking that if they're trying to simulate real fetch, they're missing something here. But perhaps it's not possible for some reason?

@kimroen
Copy link

kimroen commented Jun 4, 2020

So just thinking out loud here:

The options for credentials are:

  • 'omit': Credentials should not be sent along with the request in any case
  • 'same-origin' (default): Send the credentials along with the request, but only if the host we're making a request to is on the same origin as the client
  • 'include': Always include any credentials with the requests, no matter the origins

Now, in node you don't really have the concept of a host origin (unless I'm misunderstanding something, or jsdom has implemented something for this!), so it seems to me that the behavior whatwg-fetch uses of mapping 'omit' to "don't include credentials" and 'include' to "include credentials" really is the best they can do.

In msw however, I would expect the behavior to look like what it would look like on the server, for instance including the cookies in the request if credentials is set to 'include'. To do this and also support same-origin in node would be quite tricky for the reasons @kettanaito mentioned above though.

@kimroen
Copy link

kimroen commented Jun 4, 2020

What they are doing that might be debated though, is assuming same-origin (the default) means "don't include the credentials ever", but that has already been discussed fairly extensively over here: whatwg/fetch#585

@kentcdodds
Copy link
Contributor Author

Interesting! In any case, it seems that this is out of scope for this module and it's actually up to the polyfill to deal with this.

Also, it's not critically important to me anyway so I'll go ahead and just close this.

Thanks for the discussion everyone :)

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

No branches or pull requests

3 participants