deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

bug(http): missing docs and `setCookie()` allows `Max-age` to be 0

Open Uzlopak opened this issue 1 year ago • 7 comments

Description

Regarding this line: https://github.com/denoland/deno_std/blob/45f7a6b46018e9f4d330943918299c68ae80f1bb/http/cookie.ts#L91

While working on the cookie implementation in undici/node, I realized that the max-age implementation is slightly off.

This is what I wrote to the corresponding code piece in the undici PR

Imho the spec rfc 6265 is not correctly implemented.

Either only positive integers excluding 0 are allowed (which the current implementation does not match) for MaxAge or all integer values are allowed (this PR)

I think the problem arises because imho https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1 talks about how the data in the useragent is stored while https://www.rfc-editor.org/rfc/rfc6265#section-5.2.2 talks about the data which the useragent receives and how it should be processed.

So a server can send negative values for max-age, while a client has to drop all cookies with negative values immediatly. If the maxage is set to 0, then it would actually mean to drop the cookie according to the rfc, but it seems most useragents keep the cookie till the end of the session.

Anyhow, this means that we must allow negative integers for validateMaxAge.

https://github.com/nodejs/undici/pull/2888

Expected behavior

Negative numbers should be allowed

Uzlopak avatar Mar 01 '24 23:03 Uzlopak

setCookie() sends non-expired cookies to the client. If you'd like to send cookies that expire before the current time, use deleteCookie() instead. We should include these facts in the documentation. You are correct that setCookie() should not allow cookies with a Max-age of 0. I'd happily take a look at PRs that fix these two issues.

iuioiua avatar Mar 04 '24 02:03 iuioiua

@iuioiua

What about #992 and #989 ?

ping to @asos-craigmorten @SimonRask @wperron

So this would be basically a regression?

I think we should allow negative numbers.

Uzlopak avatar Mar 06 '24 00:03 Uzlopak

While adding this isn't a big deal, deleteCookie() does precisely what setCookie() would do if it allowed Max-Age <= 0, so there's no need to make the change. Again, in my eyes, the only tweak that seems reasonable is not allowing setCookie() to set Max-Age >= 0 and documenting this clearly. Yes, let's undo #992.

iuioiua avatar Mar 06 '24 06:03 iuioiua

I think the fix #4459 needs more support from the community to be landed.

kt3k avatar Mar 10 '24 16:03 kt3k

@kt3k

who can make the decision? tbh. I still think that we should allow negativ integers.

Uzlopak avatar Mar 10 '24 16:03 Uzlopak

@Uzlopak, can you please explain why you can't just use deleteCookie()? Again, your suggestion extends the functionality of setCookie() so it overlaps with deleteCookie().

iuioiua avatar Apr 17 '24 06:04 iuioiua

I personally think the current state is a good compromise between what RFC encourage to do (max-age > 0) and what people believe to work (max-age >= 0).

I think the reason why people think max-age=0 should work is that it was encouraged by the older versions of RFCs (https://www.rfc-editor.org/rfc/rfc2109.html, https://www.rfc-editor.org/rfc/rfc2965) and some blog post / Q&As are still mentioning it.

kt3k avatar Apr 17 '24 07:04 kt3k

Closing this now. Let's keep the current behavior. @Uzlopak, please feel free to re-open if you're able to provide reasoning behind wanting to support negative expiries. Either way, thank you for the suggestion.

iuioiua avatar Jul 17 '24 23:07 iuioiua