Split Set-Cookie header correctly by karaggeorge · Pull Request #30560 · vercel/next.js

karaggeorge · 2021-10-28T15:47:44Z

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

There's some more discussion in the issue, but in summary:

web Headers implementation combines all header values with ', '
For Set-Cookie headers, you're supposed to set them as separate values, not combine them
web Headers forbids the use of Cookie, Set-Cookie and some more headers, so they don't have custom implementation for those, and still joins them with ,
We currently just split them using split(','), but this breaks when the header contains a date (expires, max-age) that also includes a ,

I used this method to split the Set-Cookie header properly: https://www.npmjs.com/package/set-cookie-parser#splitcookiestringcombinedsetcookieheader as suggested here

I didn't add it as a dependency, since we only needed that one method and I wasn't sure what the process is for adding dependencies, so I just added the method in the middleware utils

styfle · 2021-10-28T15:55:10Z

+  Based on: https://github.com/google/j2objc/commit/16820fdbc8f76ca0c33472810ce0cb03d20efe25
+  Credits to: https://github.com/tomball for original and https://github.com/chrusart for JavaScript implementation
+*/
+export function splitCookiesString(cookiesString: string) {


Can we add a bunch of unit tests for this function so we can refactor in the future?

ijjk · 2021-10-28T16:15:14Z

Failing test suites

Commit: faf8427

test/integration/middleware-core/test/index.test.js

Middleware base tests > dev mode > should respond with top level headers and append deep headers
Middleware base tests > dev mode > /fr should respond with top level headers and append deep headers
Middleware base tests > production mode > should respond with top level headers and append deep headers
Middleware base tests > production mode > /fr should respond with top level headers and append deep headers

Expand output

● Middleware base tests › dev mode › should respond with top level headers and append deep headers

expect(received).toEqual(expected) // deep equality

- Expected  - 2
+ Received  + 1

  Array [
-   "chocochip",
-   "oatmeal",
+   "chocochip, oatmeal",
  ]

  337 |     expect(res.headers.get('x-deep-header')).toBe('valid')
  338 |     expect(res.headers.get('x-append-me')).toBe('top, deep')
> 339 |     expect(res.headers.raw()['set-cookie']).toEqual(['chocochip', 'oatmeal'])
      |                                             ^
  340 |   })
  341 | }
  342 |

  at Object.<anonymous> (integration/middleware-core/test/index.test.js:339:45)
      at runMicrotasks (<anonymous>)

● Middleware base tests › dev mode › /fr should respond with top level headers and append deep headers

expect(received).toEqual(expected) // deep equality

- Expected  - 2
+ Received  + 1

  Array [
-   "chocochip",
-   "oatmeal",
+   "chocochip, oatmeal",
  ]

  337 |     expect(res.headers.get('x-deep-header')).toBe('valid')
  338 |     expect(res.headers.get('x-append-me')).toBe('top, deep')
> 339 |     expect(res.headers.raw()['set-cookie']).toEqual(['chocochip', 'oatmeal'])
      |                                             ^
  340 |   })
  341 | }
  342 |

  at Object.<anonymous> (integration/middleware-core/test/index.test.js:339:45)
      at runMicrotasks (<anonymous>)

● Middleware base tests › production mode › should respond with top level headers and append deep headers

expect(received).toEqual(expected) // deep equality

- Expected  - 2
+ Received  + 1

  Array [
-   "chocochip",
-   "oatmeal",
+   "chocochip, oatmeal",
  ]

  337 |     expect(res.headers.get('x-deep-header')).toBe('valid')
  338 |     expect(res.headers.get('x-append-me')).toBe('top, deep')
> 339 |     expect(res.headers.raw()['set-cookie']).toEqual(['chocochip', 'oatmeal'])
      |                                             ^
  340 |   })
  341 | }
  342 |

  at Object.<anonymous> (integration/middleware-core/test/index.test.js:339:45)
      at runMicrotasks (<anonymous>)

● Middleware base tests › production mode › /fr should respond with top level headers and append deep headers

expect(received).toEqual(expected) // deep equality

- Expected  - 2
+ Received  + 1

  Array [
-   "chocochip",
-   "oatmeal",
+   "chocochip, oatmeal",
  ]

  337 |     expect(res.headers.get('x-deep-header')).toBe('valid')
  338 |     expect(res.headers.get('x-append-me')).toBe('top, deep')
> 339 |     expect(res.headers.raw()['set-cookie']).toEqual(['chocochip', 'oatmeal'])
      |                                             ^
  340 |   })
  341 | }
  342 |

  at Object.<anonymous> (integration/middleware-core/test/index.test.js:339:45)
      at runMicrotasks (<anonymous>)

styfle

Great work, thanks! 🎉

styfle · 2021-10-28T16:31:12Z

            if (key !== 'content-encoding') {
              if (key.toLowerCase() === 'set-cookie') {
-                res.setHeader(key, value.split(', '))
+                res.setHeader(key, splitCookiesString(value))


We need this added to toNodeHeaders() too:

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

Lines 41 to 43 in 3828ebe

if (key.toLowerCase() === 'set-cookie') {

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

}

…t.js into fix-set-cookie-header

ijjk · 2021-10-28T17:46:51Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
buildDuration	22.7s	22.9s	⚠️ +162ms
buildDurationCached	4.9s	4.8s	-53ms
nodeModulesSize	198 MB	198 MB	⚠️ +3.72 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.01	3.969	-0.04
/ avg req/sec	623.43	629.94	+6.51
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.133	2.105	-0.03
/error-in-render avg req/sec	1172.19	1187.68	+15.49

Client Bundles (main, webpack, commons)

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28 kB	28 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	71.9 kB	71.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
_app-HASH.js gzip	1.23 kB	1.23 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	635 B	635 B	✓
image-HASH.js gzip	4.44 kB	4.44 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.87 kB	1.87 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
334f979574ae..6f4.css gzip	106 B	106 B	✓
Overall change	13.1 kB	13.1 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
index.html gzip	534 B	534 B	✓
link.html gzip	545 B	545 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.61 kB	1.61 kB	✓

Default Build with SWC (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
buildDuration	19.4s	19.6s	⚠️ +165ms
buildDurationCached	4.6s	4.8s	⚠️ +253ms
nodeModulesSize	198 MB	198 MB	⚠️ +3.72 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.86	3.802	-0.06
/ avg req/sec	647.6	657.49	+9.89
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.99	2.11	⚠️ +0.12
/error-in-render avg req/sec	1256.24	1184.97	⚠️ -71.27

Client Bundles (main, webpack, commons)

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.2 kB	28.2 kB	✓
webpack-HASH.js gzip	1.43 kB	1.43 kB	✓
Overall change	72.1 kB	72.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
_app-HASH.js gzip	1.22 kB	1.22 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.46 kB	4.46 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.91 kB	1.91 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
334f979574ae..6f4.css gzip	106 B	106 B	✓
Overall change	13.1 kB	13.1 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js fix-set-cookie-header	Change
index.html gzip	535 B	535 B	✓
link.html gzip	547 B	547 B	✓
withRouter.html gzip	529 B	529 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 40f729e

Next js PR w/ the header fix: vercel/next.js#30560

* Add websandbox from next.js codebase. * Use node-fetch instead of next's polyfilled fetch. * Handle middleware rewrites. * Add response, headers, and request to websandbox context. * Move websandbox dependency to middleware plugin. * Add integration tests, update websandbox to support ts files and json imports. * commit yarn.lock changes after rebasing * Clean up left over console.logs, fix some tsc issues, and rebase issue. * Fix failing test and eslint. * Fix middleware test on windows. * [examples] Update Vercel Next.js example template to 12.0.1 (#6905) * Mark the Plugins as external to CLI's ncc build * [cli] Improve tracing in vc build (#6898) * [cli] Fix tracing for `vc build` * Ignore object when there are no changes * Make Next < 12 work with FS API w/ nft * Update packages/cli/src/commands/build.ts Co-authored-by: Nathan Rajlich <n@n8.io> * Document how Next.js processing works in build * [cli] Fix static assets (#6906) * Make sure output path is .next * Fix up require-server-files for processing * Fix typo * Move static * Update static rename Co-authored-by: Andy Bitz <artzbitz@gmail.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Andy <AndyBitz@users.noreply.github.com> * Publish Canary - vercel@23.1.3-canary.17 - @vercel/client@10.2.3-canary.15 - @vercel/static-config@0.0.1-canary.0 * [cli] Ignore `.env` and `.gitignore` in "vc build" (#6910) * Publish Canary - vercel@23.1.3-canary.18 * Pass workPath to plugins. The new plugin architecture doesn't pass a full BuildOptions object, previous to this commit it wasn't passing any options at all. I've added workingPath to support running dev/build from directories other than the project root. * Remove error state when package.json exists, but no build script This allows vercel build to continue working for projects that are not using frameworks, but use package.json to manage dependencies. * Fix types, pull in middleware header fix from next.js Next js PR w/ the header fix: vercel/next.js#30560 * Fix missing entries file for vc build. * Update call signature of middleware when using vc build. Co-authored-by: Drew Bredvick <dbredvick@gmail.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Jared Palmer <jared@jaredpalmer.com> Co-authored-by: Andy Bitz <artzbitz@gmail.com> Co-authored-by: Andy <AndyBitz@users.noreply.github.com>

* Add initial `vercel-plugin-middleware` * Ignore `entries.js` from ESLint * Add `runDevMiddleware()` stub * Add test * Add support for "_middleware.{js,ts}" to `vercel dev` (#6880) * Add websandbox from next.js codebase. * Use node-fetch instead of next's polyfilled fetch. * Handle middleware rewrites. * Add response, headers, and request to websandbox context. * Move websandbox dependency to middleware plugin. * Add integration tests, update websandbox to support ts files and json imports. * commit yarn.lock changes after rebasing * Clean up left over console.logs, fix some tsc issues, and rebase issue. * Fix failing test and eslint. * Fix middleware test on windows. * [examples] Update Vercel Next.js example template to 12.0.1 (#6905) * Mark the Plugins as external to CLI's ncc build * [cli] Improve tracing in vc build (#6898) * [cli] Fix tracing for `vc build` * Ignore object when there are no changes * Make Next < 12 work with FS API w/ nft * Update packages/cli/src/commands/build.ts Co-authored-by: Nathan Rajlich <n@n8.io> * Document how Next.js processing works in build * [cli] Fix static assets (#6906) * Make sure output path is .next * Fix up require-server-files for processing * Fix typo * Move static * Update static rename Co-authored-by: Andy Bitz <artzbitz@gmail.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Andy <AndyBitz@users.noreply.github.com> * Publish Canary - vercel@23.1.3-canary.17 - @vercel/client@10.2.3-canary.15 - @vercel/static-config@0.0.1-canary.0 * [cli] Ignore `.env` and `.gitignore` in "vc build" (#6910) * Publish Canary - vercel@23.1.3-canary.18 * Pass workPath to plugins. The new plugin architecture doesn't pass a full BuildOptions object, previous to this commit it wasn't passing any options at all. I've added workingPath to support running dev/build from directories other than the project root. * Remove error state when package.json exists, but no build script This allows vercel build to continue working for projects that are not using frameworks, but use package.json to manage dependencies. * Fix types, pull in middleware header fix from next.js Next js PR w/ the header fix: vercel/next.js#30560 * Fix missing entries file for vc build. * Update call signature of middleware when using vc build. Co-authored-by: Drew Bredvick <dbredvick@gmail.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Jared Palmer <jared@jaredpalmer.com> Co-authored-by: Andy Bitz <artzbitz@gmail.com> Co-authored-by: Andy <AndyBitz@users.noreply.github.com> Co-authored-by: Gary Borton <gdborton@gmail.com> Co-authored-by: Drew Bredvick <dbredvick@gmail.com> Co-authored-by: Jared Palmer <jared@jaredpalmer.com> Co-authored-by: Andy Bitz <artzbitz@gmail.com> Co-authored-by: Andy <AndyBitz@users.noreply.github.com>

Split Set-Cookie header correctly

faf8427

karaggeorge requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners October 28, 2021 15:47

ijjk added the type: next label Oct 28, 2021

styfle reviewed Oct 28, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

karaggeorge added 2 commits October 28, 2021 12:27

Add unit tests for splitCookiesString

b6f5d2d

Merge branch 'canary' into fix-set-cookie-header

741aea0

styfle previously approved these changes Oct 28, 2021

View reviewed changes

styfle reviewed Oct 28, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

karaggeorge added 2 commits October 28, 2021 12:49

Fix failing test and re-use toNodeHeaders

056ca42

Merge branch 'fix-set-cookie-header' of https://github.com/vercel/nex…

dae9a97

…t.js into fix-set-cookie-header

karaggeorge dismissed styfle’s stale review via dae9a97 October 28, 2021 16:50

styfle approved these changes Oct 28, 2021

View reviewed changes

Merge branch 'canary' into fix-set-cookie-header

40f729e

kodiakhq Bot merged commit 450552d into canary Oct 28, 2021

kodiakhq Bot deleted the fix-set-cookie-header branch October 28, 2021 17:47

gdborton added a commit to vercel/vercel that referenced this pull request Oct 29, 2021

Fix types, pull in middleware header fix from next.js

a38b0bc

Next js PR w/ the header fix: vercel/next.js#30560

styfle added the hacktoberfest-accepted label Oct 31, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split Set-Cookie header correctly#30560

Split Set-Cookie header correctly#30560
kodiakhq[bot] merged 6 commits intocanaryfrom
fix-set-cookie-header

karaggeorge commented Oct 28, 2021

Uh oh!

styfle Oct 28, 2021 •

edited

Loading

Uh oh!

karaggeorge Oct 28, 2021

Uh oh!

This comment has been minimized.

ijjk commented Oct 28, 2021

Uh oh!

styfle left a comment

Uh oh!

styfle Oct 28, 2021 •

edited

Loading

Uh oh!

This comment has been minimized.

ijjk commented Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (key.toLowerCase() === 'set-cookie') {
	result[key] = value.split(', ')
	}

Conversation

karaggeorge commented Oct 28, 2021

Bug

Uh oh!

styfle Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karaggeorge Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

ijjk commented Oct 28, 2021

Failing test suites

Uh oh!

styfle left a comment

Choose a reason for hiding this comment

Uh oh!

styfle Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

ijjk commented Oct 28, 2021

Stats from current PR

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

styfle Oct 28, 2021 •

edited

Loading

styfle Oct 28, 2021 •

edited

Loading