Undici strips out set-cookie headers, even when "credentials: 'include'" is set

[hansede]

Bug Description

Undici strips out set-cookie headers, which is expected per the fetch API spec. What is not expected though is that the set-cookie headers are stripped out in the presence of { credentials: 'include' } in a fetch request. Here is the relevant portion of the fetch API spec:
https://fetch.spec.whatwg.org/#example-cors-with-credentials

Reproducible By

server.js

const express = require('express')
const app = express()

app.get('/', (req, res) => {
  res.set({
    abc: '123',
    'set-cookie': 'hello=world',
  });
  res.send('Hello World!');
})

app.listen(3333, () => {
  console.log(`Example app listening on port ${port}`)
})

client.js

const { fetch } = require('undici');

async function get() {
  const headers = await fetch('http:https://localhost:3333/', { credentials: 'include' })
    .then(async res => {
      for await (const chunk of res.body) {}
      return res.headers
    });

  console.log(`abc: ${headers.get('abc')}`);
  console.log(`set-cookie: ${headers.get('set-cookie')}`);
}

get();

With the server script running, the client script returns:

abc: 123
set-cookie: null

Expected Behavior

In the presence of { credentials: 'include' } I would expect set-cookie headers to not be stripped from the response. The expected output of the client script should be:

abc: 123
set-cookie: hello=world

Environment

Mac OS 12.2.1
Node v16.13.1

Additional context

There is a TODO in Undici's fetch code where this behavior should be implemented:
https://github.com/nodejs/undici/blob/main/lib/fetch/index.js#L1369-L1375

[mcollina]

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[hansede]

I would like to create a pull request, but I'm going to need some help first with deciding how to handle this.

includeCredentials is being generated in httpNetworkOrCacheFetch, but I need to use it in filterResponse to determine whether forbidden headers should be included/excluded from the response. includeCredentials can be generated from properties on the request object, but the request object is unavailable in filterResponse. The request object is available in the context of most calls to filterResponse, but not all.

This leads me to wonder how best to make includeCredentials available to filterResponse. The response object is of course available, so I could potentially attach includeCredentials as a property of the response object, but that feels like a hack and I'm not even sure where the right place in code would be to make that attachment. I looked around for an overall "fetch context" object that is available for the full lifecycle, but I didn't find anything that perfectly matches that description. I found fetchParams, but I'm not sure that attaching includeCredentials to this object is the right place to put it, and fetchParams is not currently available in many contexts.

@mcollina: Do you have a suggestion for how to provide includeCredentials to filterResponse?

In general it feels like it could be helpful to have a context object that includes the fetchParams, request, and response objects that would be passed around to most functions in the call chain. There seems to be an assumption in some parts of the code right now that the request object is no longer needed when the response is available, but that is not always true. (Note that I've only been looking at Undici code for the past day, so I'm speaking from a place of limited exposure). Dealing with this would all be out of the scope of this issue though, so here I'm looking for a simpler solution.

[ronag]

Isn't it enough to just literally follow the steps in the spec?

If includeCredentials is true, then:

If the user agent is not configured to block cookies for httpRequest (see [section 7](https://datatracker.ietf.org/doc/html/rfc6265#section-7) of [[COOKIES]](https://fetch.spec.whatwg.org/#biblio-cookies)), then:

Let cookies be the result of running the "cookie-string" algorithm (see [section 5.4](https://datatracker.ietf.org/doc/html/rfc6265#section-5.4) of [[COOKIES]](https://fetch.spec.whatwg.org/#biblio-cookies)) with the user agent’s cookie store and httpRequest’s [current URL](https://fetch.spec.whatwg.org/#concept-request-current-url).

If cookies is not the empty string, then [append](https://fetch.spec.whatwg.org/#concept-header-list-append) (`Cookie`, cookies) to httpRequest’s [header list](https://fetch.spec.whatwg.org/#concept-request-header-list).
If httpRequest’s [header list](https://fetch.spec.whatwg.org/#concept-request-header-list) [does not contain](https://fetch.spec.whatwg.org/#header-list-contains) `Authorization`, then:

Let authorizationValue be null.

If there’s an [authentication entry](https://fetch.spec.whatwg.org/#authentication-entry) for httpRequest and either httpRequest’s [use-URL-credentials flag](https://fetch.spec.whatwg.org/#concept-request-use-url-credentials-flag) is unset or httpRequest’s [current URL](https://fetch.spec.whatwg.org/#concept-request-current-url) does not [include credentials](https://url.spec.whatwg.org/#include-credentials), then set authorizationValue to [authentication entry](https://fetch.spec.whatwg.org/#authentication-entry).

Otherwise, if httpRequest’s [current URL](https://fetch.spec.whatwg.org/#concept-request-current-url) does [include credentials](https://url.spec.whatwg.org/#include-credentials) and isAuthenticationFetch is true, set authorizationValue to httpRequest’s [current URL](https://fetch.spec.whatwg.org/#concept-request-current-url), converted to an `Authorization` value.

If authorizationValue is non-null, then [append](https://fetch.spec.whatwg.org/#concept-header-list-append) (`Authorization`, authorizationValue) to httpRequest’s [header list](https://fetch.spec.whatwg.org/#concept-request-header-list).

[hansede]

🤔 That mentions setting the Cookie header, but there's nothing there about allowing the Set-Cookie headers through.

[ronag]

@hansede I think we first need to understand the specification and how it imagines your use case to work.

[ronag]

Here is the relevant part https://github.com/nodejs/undici/blob/main/lib/fetch/index.js#L1850-L1858

[benmccann]

Perhaps the relevant portion of the spec would be:

If includeCredentials is true and the user agent is not configured to block cookies for request (see section 7 of [COOKIES]), then run the "set-cookie-string" parsing algorithm (see section 5.2 of [COOKIES]) on the value of each header whose name is a byte-case-insensitive match for Set-Cookie in response’s header list, if any, and request’s current URL.

[ronag]

I think we already fixed this. Can someone confirm?

[rmunn]

undici/lib/fetch/index.js

Lines 1806 to 1811 in 5478cdd

	// 3. If includeCredentials is true and the user agent is not configured
	// to block cookies for request (see section 7 of [COOKIES]), then run the
	// "set-cookie-string" parsing algorithm (see section 5.2 of [COOKIES]) on
	// the value of each header whose name is a byte-case-insensitive match for
	// `Set-Cookie` in response’s header list, if any, and request’s current URL.
	// TODO

doesn't look fixed to me; there's still a TODO comment but no code.

[ronag]

You are right. PR welcome!

[KhafraDev]

Cookie parsing compliant with the rfc linked is done with #1441. I'll probably strip out that implementation into a new PR to use for this issue.

edit: I found the issue and will make a PR soon to fix this. Didn't need to parse cookies.

[Rich-Harris]

#1454 was reverted following @ronag's comment:

I think this landed too early and would prefer we properly investigate and follow the spec

I don't think there is a spec to follow here though. The spec (referring to 4.7.17.3 of the fetch standard, which leads us 5.2 of RFC 6265) is clearly written for browsers; it assumes that the user agent has a cookie store, which obviously makes no sense for a server that can handle requests from multiple users.

I gather from @KhafraDev that there's a proposal to add cookieStore options to Request and Response constructors (though the comment doesn't link to the proposal, just to the proposed CookieStore spec?), and that could be a good solution one day. In the meantime though, being unable to read cookies from requests means that Undici is untenable for many real-world applications. (SvelteKit would like to use Node's built-in fetch when applications are deployed as Node servers, but for the time being we're reliant on node-fetch even with Node 18.)

Other implementations have chosen differently. This URL returns a set-cookie: hello=world header — if we fetch it with different implementations, this is what we see:

implementation	headers.get('set-cookie')
node-fetch	hello=world
Deno	hello=world
Cloudflare Workers	hello=world
Undici	null

Note that it fails in Cloudflare when you're using Wrangler (whether in --local mode or not), precisely because Miniflare uses Undici: cloudflare/miniflare#183

In the absence of a usable spec, I would argue that following the behaviour that a majority of implementations have agreed upon (and that users expect, as evidenced by the Miniflare issue among others) is the prudent course of action. (Even though headers.get('set-cookie') is problematic because it can smush together multiple headers, this is easily solved in userland with popular packages like set-cookie-parser.)

[Rich-Harris]

@mcollina any chance we can reopen this, since #1454 was reverted?

[KhafraDev]

I believe this is a bug that has nothing to do with cookie parsing (etc.) and #1454 presents a valid solution, just need to see what ronag thinks now. 😄

[ronag]

I'm ok with following cloudflare and deno. But if we land this change I would like that:

• A WinterCG issue.
• It's clear in the code that all related logic is outside of spec and a reference motivating it. Possibly an issue in the WinterCG repo?
• Clearly added to the README.
• Investigate what exactly deno, node-fetch and cloudflare does so that we don't introduce another variant with slight differences.

I think a clear motivation is what was primarly missing from the PR. Similar to #1262 (comment).

[KhafraDev]

• node-fetch does not have credentials. So I'm guessing that no filtering is occurring.
• Deno does not have credentials - however they still keep the typing. I would guess no filtering occurs either?
• Cloudflare, probably no credentials or filtering either, considering the example code works.

I would assume that no other platform besides undici actually does this filtering, especially since all the other platforms seem to work without an issue (that I could find) mentioning credentials: 'include'. I think a more generalized wintercg issue would be a better idea, ie. regarding which properties of Request and Response should be dropped because having them in a server environment is pointless.