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
Split Set-Cookie header correctly #30560
Conversation
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a bunch of unit tests for this function so we can refactor in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 👍
This comment has been minimized.
This comment has been minimized.
Failing test suitesCommit: faf8427 test/integration/middleware-core/test/index.test.js
Expand output● 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks! 🎉
packages/next/server/next-server.ts
Outdated
@@ -1171,7 +1172,7 @@ export default class Server { | |||
for (const [key, value] of result.response.headers.entries()) { | |||
if (key !== 'content-encoding') { | |||
if (key.toLowerCase() === 'set-cookie') { | |||
res.setHeader(key, value.split(', ')) | |||
res.setHeader(key, splitCookiesString(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(', ') | |
} |
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js fix-set-cookie-header | Change | |
---|---|---|---|
buildDuration | 22.7s | 22.9s | |
buildDurationCached | 4.9s | 4.8s | -53ms |
nodeModulesSize | 198 MB | 198 MB |
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 | |
buildDurationCached | 4.6s | 4.8s | |
nodeModulesSize | 198 MB | 198 MB |
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 | |
/error-in-render avg req/sec | 1256.24 | 1184.97 |
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 | ✓ |
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>
Bug
fixes #number
contributing.md
Fixes #30430
There's some more discussion in the issue, but in summary:
Headers
implementation combines all header values with', '
Set-Cookie
headers, you're supposed to set them as separate values, not combine themHeaders
forbids the use ofCookie
,Set-Cookie
and some more headers, so they don't have custom implementation for those, and still joins them with,
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