Add special handling of `set-cookie` to Headers #1346

lucacasonato · 2021-10-29T15:46:35Z

Towards #973

At least two implementers are interested (and none opposed):
- Firefox (Use case for Headers getAll #973 (comment))
- Deno (Use case for Headers getAll #973 (comment))
- Chrome (Add special handling of set-cookie to Headers #1346 (comment))
- Cloudflare Workers
- Node.js
Tests are written and can be reviewed and commented upon at:
- Fetch: set-cookie special cases in Headers web-platform-tests/wpt#31442
Implementation bugs are filed:
- Chrome: https://crbug.com/1409512
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1812511
- Safari: https://bugs.webkit.org/show_bug.cgi?id=251194
- Deno: feat(ext/fetch): add Headers#getSetCookie denoland/deno#13542 (comment)
MDN
- Document Fetch's Headers's getSetCookie() method mdn/content#24266

Preview | Diff

fetch.bs

Co-authored-by: Steven <steven@ceriously.com>

fetch.bs

annevk · 2021-11-01T09:36:22Z

Thanks for working on this, this looks quite promising to me. @yutakahirano @ricea @youennf thoughts?

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

fetch.bs

Co-authored-by: Andreu Botella <andreu@andreubotella.com>

lucacasonato · 2021-11-04T10:34:47Z

@jasnell @kentonv thoughts?

yutakahirano

LGTM.

fetch.bs

jasnell

LGTM. For Workers we do currently take a different approach using getAll() that we'll need to maintain for a while but there's nothing here that should interfere with us being able to support this as well.

…tch into special_headers_handling

kentonv · 2021-11-04T17:09:04Z

I'm fine with this.

I have a weak opinion that, for server-side vendors, getAll() is slightly more flexible: If we have some big customer come along and say "We really need to access this legacy API, but it has a header that misuses commas, which we can't change," we can easily say "No problem, we'll just add your special weird header to the getAll() allow list." On the server side, implementing special requests for high-paying customers are not uncommon. Obviously, on the browser side, such special requests would be complete nonsense, since web sites don't get to choose which browser is used to open them.

But I have seen no evidence that any such headers exist in the wild, so it seems unlikely we'll ever get this exact sort of special request. So, whatever. :)

annevk · 2021-11-05T07:55:19Z

It's more flexible, but it would also violate HTTP, as would any header that needs it (with the exception of Set-Cookie).

jimmywarting · 2021-11-21T16:29:32Z

I'm not opposed to adding this into node-fetch either. I will only be glad that we can unite on one unified api that works the same across all platform

Refs: whatwg/fetch#1346

https://bugs.webkit.org/show_bug.cgi?id=251194 Reviewed by Youenn Fablet. This change implements the `getSetCookie` method of the fetch spec's `Headers` interface. This change allows storing `Set-Cookie` headers as a list of values, and retrieving the separate values, rather than their combined representation, which is ambiguous. This change also makes it so iterating through `Headers` objects will yield `Set-Cookie` headers separately rather than combined, which is part of the same change in the fetch spec. Given that `Set-Cookie` is a forbidden header name in both requests and responses, this will not be useful for any uses of `Headers` related to fetch. It does allows some non-fetch use cases, however, and enables further compatibility with server-side runtimes that do allow this header. Rather than refactoring `HTTPHeaderMap` to not combine (all) headers, this implementation makes use of the fact that `Set-Cookie` is forbidden for both requests and responses, to store the `Set-Cookie` values in a vector of strings directly in `FetchHeaders` instead. This needed such header name to be special-cased in almost every operation of this class. And in order to have `Set-Cookie` headers properly appear in their right order in the pair iterator, `m_keys` has been changed to be a pair where the second element is an index into the `Set-Cookie` values vector, which is ignored for other header names. Fetch spec PR: whatwg/fetch#1346 This commit also imports the corresponding WPT tests from web-platform-tests/wpt#31442, and adds some additional tests. * Source/WebCore/Modules/fetch/FetchHeaders.cpp: (WebCore::appendToHeaderMap): (WebCore::fillHeaderMap): (WebCore::FetchHeaders::create): (WebCore::FetchHeaders::fill): (WebCore::FetchHeaders::append): (WebCore::FetchHeaders::remove): (WebCore::FetchHeaders::get const): (WebCore::FetchHeaders::getSetCookie const): (WebCore::FetchHeaders::has const): (WebCore::FetchHeaders::set): (WebCore::FetchHeaders::Iterator::next): * Source/WebCore/Modules/fetch/FetchHeaders.h: (WebCore::FetchHeaders::create): (WebCore::FetchHeaders::fastGet const): (WebCore::FetchHeaders::fastHas const): (WebCore::FetchHeaders::fastSet): (WebCore::FetchHeaders::FetchHeaders): * Source/WebCore/Modules/fetch/FetchHeaders.idl: * Source/WebCore/platform/network/ResourceResponseBase.cpp: (WebCore::ResourceResponseBase::httpHeaderField const): Canonical link: https://commits.webkit.org/260533@main

…rs, a=testonly Automatic update from web-platform-tests Fetch: the Set-Cookie header, and Header's getSetCookie() For whatwg/fetch#1346. Co-authored-by: Andreu Botella <abotella@igalia.com> Co-authored-by: Andreu Botella <andreu@andreubotella.com> Co-authored-by: Anne van Kesteren <annevk@annevk.nl> -- wpt-commits: 14151646ae4684a32638d73187381fc0ed86e2c8 wpt-pr: 31442

Fixes #56 Relates to whatwg/fetch#1346

For whatwg/fetch#1346. Co-authored-by: Andreu Botella <abotella@igalia.com> Co-authored-by: Andreu Botella <andreu@andreubotella.com> Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

## What This fix serves to address issues where multiple `Set-Cookie` headers were combined in some runtimes. ## Why This is because `set-cookie` behaves differently than other headers in some cases. Eg. when iterating on a `Headers` instance, multiple set-cookie headers are folded. To set them correctly, we need to split them. But it'd not be enough to naively split on the first occurrence, because `,` is a valid cookie value when for example it's used in `Expires` in a date string. So we use a method to correctly detect where to split the cookie. This should fix all runtimes. Note, the spec now has `Headers#getSetCookie` which should be preferred if it's present. whatwg/fetch#1346. We are using the [`edge-runtime`](https://github.com/vercel/edge-runtime), so this should be fixed upstream and then reused in Next.js in the future. ## How Wherever we can, we reuse the `fromNodeHeaders` and `toNodeHeaders` methods that have the correct implementation. This should be preferred in the future in other parts of the codebase. We fixed some related TS issues as well. Fixes #46579, supersedes #40579 fix NEXT-735 ([link](https://linear.app/vercel/issue/NEXT-735)) --------- Co-authored-by: Balázs Orbán <info@balazsorban.com>

Spec change: whatwg/fetch#1346 Tests: web-platform-tests/wpt#31442 (ran against this PR and they all pass) --------- Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>

lucacasonato mentioned this pull request Oct 29, 2021

Fetch: set-cookie special cases in Headers web-platform-tests/wpt#31442

Merged

lucacasonato added 2 commits October 29, 2021 18:23

Add special handling of set-cookie to Headers

267d6d2

Add getSetCookie()

f837d20

lucacasonato force-pushed the special_headers_handling branch from 80fa156 to f837d20 Compare October 29, 2021 16:24

styfle reviewed Oct 29, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

Update fetch.bs

7c79f92

Co-authored-by: Steven <steven@ceriously.com>

annevk reviewed Nov 1, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

lucacasonato and others added 6 commits November 1, 2021 11:27

review comment

8176492

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

review comment

93e26a3

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

typo

07e6f8c

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

add empty list case

e82d215

update note regarding set-cookie

0394f8e

fix

538c360

andreubotella reviewed Nov 1, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

review comment

84a1afc

Co-authored-by: Andreu Botella <andreu@andreubotella.com>

yutakahirano approved these changes Nov 4, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

jasnell approved these changes Nov 4, 2021

View reviewed changes

lucacasonato added 2 commits November 4, 2021 14:38

fix formatting

e95ce29

Merge branch 'special_headers_handling' of github.com:lucacasonato/fe…

e3c7c38

…tch into special_headers_handling

youennf mentioned this pull request Nov 5, 2021

Use case for Headers getAll #973

Closed

lucacasonato mentioned this pull request Jan 30, 2022

feat(ext/fetch): add Headers#getSetCookie denoland/deno#13542

Merged

linting

3e7013c

jimmywarting mentioned this pull request Jan 30, 2022

[Feature request] Add a CookieStore option to Request & Response #1384

Open

GregBrimble mentioned this pull request May 3, 2022

Stop unnecessarily amalgamating duplicate headers in Functions cloudflare/workers-sdk#880

Merged

github-actions bot mentioned this pull request May 4, 2022

Version Packages cloudflare/workers-sdk#889

Merged

annevk mentioned this pull request Feb 8, 2023

Document Fetch's Headers's getSetCookie() method mdn/content#24266

Closed

nfriedly mentioned this pull request Feb 10, 2023

Add support for getSetCookie() method nfriedly/set-cookie-parser#56

Closed

jasnell added a commit to cloudflare/workerd that referenced this pull request Feb 13, 2023

Implement headers.getSetCookie()

7b7fd2e

Refs: whatwg/fetch#1346

CYBAI mentioned this pull request Feb 20, 2023

Implement Headers.prototype.getSetCookie servo/servo#29388

Closed

Rich-Harris mentioned this pull request Mar 3, 2023

Setting multiple Set-Cookie headers using the vercel adapter sveltejs/kit#9221

Open

yoseio mentioned this pull request Mar 6, 2023

Implement Headers.prototype.getSetCookie servo/servo#29465

Merged

5 tasks

nfriedly added a commit to nfriedly/set-cookie-parser that referenced this pull request Mar 18, 2023

add support for getSetCookie() method on fetch response headers objects.

bbfdc6e

Fixes #56 Relates to whatwg/fetch#1346

nfriedly added a commit to nfriedly/set-cookie-parser that referenced this pull request Mar 18, 2023

add support for getSetCookie() method on fetch response headers objects.

801ace8

Fixes #56 Relates to whatwg/fetch#1346

balazsorban44 mentioned this pull request Mar 30, 2023

fix: correctly handle Set-Cookie headers vercel/next.js#47718

Merged

timostamm mentioned this pull request Aug 8, 2023

Unable to set multiple cookies from within service method handler connectrpc/connect-es#745

Closed

lucacasonato deleted the special_headers_handling branch August 17, 2023 10:17

chicoxyzzy mentioned this pull request Aug 30, 2023

Setting a cookie to send to a server doesn't work socketsupply/socket#536

Open

ezzatron mentioned this pull request Sep 21, 2023

Set-Cookie headers should not be combined / Support Headers.getSetCookie() method JakeChampion/fetch#1384

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special handling of `set-cookie` to Headers #1346

Add special handling of `set-cookie` to Headers #1346

lucacasonato commented Oct 29, 2021 •

edited by annevk

Loading

annevk commented Nov 1, 2021

lucacasonato commented Nov 4, 2021

yutakahirano left a comment

jasnell left a comment

kentonv commented Nov 4, 2021

annevk commented Nov 5, 2021

jimmywarting commented Nov 21, 2021

Add special handling of set-cookie to Headers #1346

Add special handling of set-cookie to Headers #1346

Conversation

lucacasonato commented Oct 29, 2021 • edited by annevk Loading

annevk commented Nov 1, 2021

lucacasonato commented Nov 4, 2021

yutakahirano left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

kentonv commented Nov 4, 2021

annevk commented Nov 5, 2021

jimmywarting commented Nov 21, 2021

Add special handling of `set-cookie` to Headers #1346

Add special handling of `set-cookie` to Headers #1346

lucacasonato commented Oct 29, 2021 •

edited by annevk

Loading