Chunked cookies should not exceed browser max #237
Conversation
|
@davidpatrick - the tests are failing because the max header size for Node 12 is 8KB You could up it to 16kb on the circle CI environment Or rewrite the tests to keep the header <8KB |
| delete cookieOptions.transient; | ||
| const cookies = req[COOKIES]; | ||
| const { transient: cookieTransient , ...cookieOptions } = cookieConfig; | ||
| cookieOptions.expires = cookieTransient ? 0 : new Date(exp * 1000); |
There was a problem hiding this comment.
I know this was the existing logic, but I noticed it's not correct.
If you want a session cookie expires should not be defined (it shouldn't be set to 0)
If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes
See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271
| delete cookieOptions.transient; | ||
| const cookies = req[COOKIES]; | ||
| const { transient: cookieTransient , ...cookieOptions } = cookieConfig; | ||
| cookieOptions.expires = cookieTransient ? 0 : new Date(exp * 1000); |
There was a problem hiding this comment.
I know this was the existing logic, but I noticed it's not correct.
If you want a session cookie expires should not be defined (it shouldn't be set to 0)
If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes
See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271
| delete cookieOptions.transient; | ||
| const cookies = req[COOKIES]; | ||
| const { transient: cookieTransient , ...cookieOptions } = cookieConfig; | ||
| cookieOptions.expires = cookieTransient ? 0 : new Date(exp * 1000); |
There was a problem hiding this comment.
@davidpatrick I know this was the existing logic, but I noticed it's not correct.
If you want a session cookie expires should not be defined (it shouldn't be set to 0)
If (expires is) unspecified, the cookie becomes a session cookie. A session finishes when the client shuts down, and session cookies will be removed.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes
See nextjs-auth0 for reference https://github.com/auth0/nextjs-auth0/blob/main/src/auth0-session/cookie-store.ts#L44-L46 and https://github.com/auth0/nextjs-auth0/blob/main/tests/auth0-session/cookie-store.test.ts#L262-L271
Would you mind fixing it and adding a regression test? Either in this PR or another one before you do the release
There was a problem hiding this comment.
@davidpatrick - actually, aplogies, ignore me - I've just tested it and I get a sessoion cookie fine - so don't worry
We currently allow 96 characters leeway for cookie attribute size (browser cookie max size (4096 Bytes) minus CHUNK_BYTE_SIZE)
This is not enough for scenarios where the user provides many cookie options or longer cookie domain or path options.
Since the cookie attributes are dynamic and can be quite long, we can calculate the cookie attributes length from serializing an empty cookie with the same options and measuring it.
Same strategy as done over in auth0/nextjs-auth0#301