Cookie Expiration Age Fix

[jonathandunne]

Fixes #7301
Fixes #5437

[code-asher]

Thank you for the contribution! Looks good to me so far. We will have to add it to src/node/cli.ts as well.

The . syntax is intriguing, but I want to say we call it auth-cookie-max-age for now.

[code-asher]

Oh wait is 0 the right default? This will delete the cookie instantly, I think? We may want -1.

[code-asher]

Hmm actually zero or negative immediately expires the cookie it seems.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#max-agenumber
https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.2

Should it just be undefined? Also we may want ?? instead of || in case someone does explicitly set it to zero for some reason.

[nb-programmer]

I'd read it in the Express JS docs setting it to 0 makes it session
https://expressjs.com/en/api.html#res.cookie

They gave this explanation to the expires parameter, but also said maxAge is a "Convenient option for setting the expiry time relative to the current time in milliseconds". They didn't specify what to put here to make it session, nor what the default is.

[code-asher]

Yeah I also thought maybe it was an Express thing, but when I checked the source it seems to just immediately expire.

If maxAge is zero, it passes expires set to now and maxAge set to zero into cookie.serialize():

https://github.com/expressjs/express/blob/f9954dd317a1b534e62a5ae3aa7f52f3582b8881/lib/response.js#L764-L771

cookie.serialize() simply adds Max-Age=0 and Expires=<the now date> to the cookie:

https://github.com/jshttp/cookie/blob/2a1a4d8b2679b208f354e848e711dc8471fb83af/src/index.ts#L268-L274

Now, if expires is zero (although this is actually a type error), and there is no max age it does indeed skip setting Expires in that case, which would result in a session cookie. The difference is they do if (options.maxAge !== undefined) versus if (options.expires), so zero is not ignored in the max age case.

[nb-programmer]

Yeah I also thought maybe it was an Express thing, but when I checked the source it seems to just immediately expire.

If maxAge is zero, it passes expires set to now and maxAge set to zero into cookie.serialize():

https://github.com/expressjs/express/blob/f9954dd317a1b534e62a5ae3aa7f52f3582b8881/lib/response.js#L764-L771

cookie.serialize() simply adds Max-Age=0 and Expires=<the now date> to the cookie:

https://github.com/jshttp/cookie/blob/2a1a4d8b2679b208f354e848e711dc8471fb83af/src/index.ts#L268-L274

Now, if expires is zero (although this is actually a type error), and there is no max age it does indeed skip setting Expires in that case, which would result in a session cookie. The difference is they do if (options.maxAge !== undefined) versus if (options.expires), so zero is not ignored in the max age case.

Hmm, what would be a good way to ensure it is properly a session cookie? Sending null seems illogical, but would work, or we can just skip the field if there is no config for it.

Edit: null won't work since the serialize method just checks for undefined, and raises an error if its not an integer.

[code-asher]

Yeah I think something like maxAge: getConfigCookieMaxAgeAsMilliseconds(req) ?? undefined would do the trick.

[code-asher]

Ah wait getConfigCookieMaxAgeAsMilliseconds always returns a number, so we would have to do it in there. We could return number | undefined.

[code-asher]

Closing for now but happy to re-review if you come back to this.