Edge Functions: maxAge breaks Set-Cookie header

[dferber90]

What version of Next.js are you using?

12.0.0

What version of Node.js are you using?

16.3.0

What browser are you using?

Chrome, Firefox

What operating system are you using?

macOS

How are you deploying your application?

Vercel

Describe the Bug

When using _middleware.ts and setting a cookie via res.cookie(COOKIE_NAME, bucket, { maxAge: 10000 }), the response contains two broken Set-Cookie headers instead of one correct one.

Here is the response to the request in Chrome:

Notice how the first set-cookie header abruptly stops after "Wed" and the content that would follow there then follows in a separate Set-Cookie header, except that the comma that should be after "Wed" is missing.

Response headers as text

set-cookie: bucket-marketing=original; Max-Age=10; Path=/; Expires=Wed
set-cookie: 27 Oct 2021 11:43:14 GMT

The value of Set-Cookie is originally something like bucket-marketing=original; Max-Age=10; Path=/; Expires=Wed, 27 Oct 2021 11:43:14 GMT and it seems as if the , splits the header into two.

I tested, and this does not happen with other headers with the same value.

This seems to be a regression. The same code is working in ^11.1.3-canary.104 but not in 12.0.0.

Expected Behavior

Only one Set-Cookie header is added to the response.

To Reproduce

Clone the Simple AB Testing example and adapt:

git clone git@github.com:vercel/examples.git

Update the next.js version to v12 by editing examples/edge-functions/ab-testing-simple/package.json:

  "dependencies": {
-   "next": "^11.1.3-canary.104"
+   "next": "12.0.0",
  },

Run yarn in examples/edge-functions/ab-testing-simple.

Then edit examples/edge-functions/ab-testing-simple/pages/marketing/_middleware to:

import { NextRequest, NextResponse } from 'next/server'
import { getBucket } from '@lib/ab-testing'
import { MARKETING_BUCKETS } from '@lib/buckets'

const COOKIE_NAME = 'bucket-marketing'

export function middleware(req: NextRequest) {
  const bucket = req.cookies[COOKIE_NAME] || getBucket(MARKETING_BUCKETS)
  const res = NextResponse.rewrite(`/marketing/${bucket}`)

  // Removed the if-block to always set the cookie

  // Adding the maxAge option (this breaks the cookie)
  res.cookie(COOKIE_NAME, bucket, { maxAge: 10000 })

  return res
}

Run yarn dev to start Next.js.

Open http://localhost:3000/marketing and inspect the response to the marketing site. You will see two Set-Cookie headers. This only happens when maxAge or expires are set in res.cookie().

[dferber90]

It seems as if one of these is the root cause:

next.js/packages/next/server/next-server.ts

Line 1174 in 7902616

res.setHeader(key, value.split(', '))

next.js/packages/next/server/web/utils.ts

Line 42 in 82001f2

result[key] = value.split(', ')

[dferber90]

@styfle It seems as if this caused by #30288, specifically in d31615b. It's hard for me to follow why Set-Cookie needs the special treatment added there. I could undo it, but I don't know the side effects without knowing the reason why it was added.

Splitting based on , leads to wrong behaviour with cookies that have an Expires like
foo=bar; Max-Age=0; Path=/; Expires=Wed, 27 Oct 2021 11:17:49 GMT; SameSite=Lax as that contains a ,.

Since the middleware automatically sets Expires when maxAge is given, this is affecting cookies added through response.cookie() from the middleware that either have maxAge or expires set, like response.cookie(name, value, { maxAge: 100 }):

next.js/packages/next/server/web/spec-extension/response.ts

Lines 49 to 52 in 88131eb

	if (opts.maxAge) {
	opts.expires = new Date(Date.now() + opts.maxAge)
	opts.maxAge /= 1000
	}

Could you give some context of why it's necessary, or can we maybe just drop the .split(', ')?

---

Edit: Oh it's there because calling append with the same key multiple times leads to concatenation with , :

const headers = new Headers()
headers.append("set-cookie", "a")
headers.append("set-cookie", "b")
headers.get("set-cookie") // -> 'a, b'

Unfortunately we still can't distinguish between header values genuinely containing , and header values concatenated with , because the header contains multiple values :( Seems like a limitation of the Headers class itself.

---

References:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.
https://www.rfc-editor.org/rfc/rfc6265#section-6.1

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html

Note: When Header values are iterated over, they are automatically sorted in lexicographical order, and values from duplicate header names are combined.
https://developer.mozilla.org/en-US/docs/Web/API/Headers

Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
appears multiple times in a response message and does not use the
list syntax, violating the above requirements on multiple header
fields with the same name. Since it cannot be combined into a
single field-value, recipients ought to handle "Set-Cookie" as a
special case while processing header fields.
https://www.rfc-editor.org/rfc/rfc7230

[dferber90]

To sum up my findings: RFC 6265 says that the Set-Cookie header should not be folded (multiple values should not get concatenated with ,) and should instead persist as multiple Set-Cookie headers. Unfortunately Headers folds it, so we end up with a broken Set-Cookie header.

The header looks like a=1; Expires=Wed, 27 Oct 2021 11:17:49 GMT, b=2; Expires=Thu, 28 Oct 2021 11:17:49 GMT which then breaks when split on , .

This can also happen to other headers containing a , in their original value.

[karaggeorge]

Just to add some more context, the native web Headers implementation doesn't even allow setting/getting some forbidden header names:

• https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_response_header_name
• https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name

Which include Cookie, Cookie2, Set-Cookie, Set-Cookie2

My guess is that CF workers implement their own version of Headers to allow those values. It also adds some new methods on the Headers object like getAll.

They probably did not take the RFC above into consideration when "patching" those forbidden headers?

Relevant issue: whatwg/fetch#973

Seems like that's why they proposed adding getAll in the first place, and it was even suggested to just handle Set-Cookie separately

[karaggeorge]

We could also look into utilizing this library which handles the splitting correctly: https://www.npmjs.com/package/set-cookie-parser#splitcookiestringcombinedsetcookieheader

[balazsorban44]

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.