Skip to content
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

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

Closed
Uzlopak opened this issue Mar 1, 2024 · 8 comments
Closed

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

Uzlopak opened this issue Mar 1, 2024 · 8 comments
Labels
feedback welcome We want community's feedback on this issue or PR http suggestion a suggestion yet to be agreed

Comments

@Uzlopak
Copy link

Uzlopak commented Mar 1, 2024

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.

nodejs/undici#2888

Expected behavior

Negative numbers should be allowed

@iuioiua
Copy link
Contributor

iuioiua commented Mar 4, 2024

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 iuioiua added good first issue Good for newcomers http PR welcome A pull request for this issue would be welcome and removed needs triage labels Mar 4, 2024
@iuioiua iuioiua changed the title cookie: max-age should allow negative numbers bug(http): missing docs and setCookie() allows Max-age to be 0 Mar 4, 2024
@Uzlopak
Copy link
Author

Uzlopak commented Mar 6, 2024

@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.

@iuioiua
Copy link
Contributor

iuioiua commented Mar 6, 2024

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.

@kt3k
Copy link
Member

kt3k commented Mar 10, 2024

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

@Uzlopak
Copy link
Author

Uzlopak commented Mar 10, 2024

@kt3k

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

@iuioiua iuioiua added the feedback welcome We want community's feedback on this issue or PR label Mar 10, 2024
@kt3k kt3k added suggestion a suggestion yet to be agreed and removed bug Something isn't working good first issue Good for newcomers PR welcome A pull request for this issue would be welcome labels Mar 12, 2024
@iuioiua
Copy link
Contributor

iuioiua commented Apr 17, 2024

@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().

@kt3k
Copy link
Member

kt3k commented Apr 17, 2024

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.

@iuioiua
Copy link
Contributor

iuioiua commented Jul 17, 2024

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 iuioiua closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback welcome We want community's feedback on this issue or PR http suggestion a suggestion yet to be agreed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants