-
Notifications
You must be signed in to change notification settings - Fork 525
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
cookies: fix validateMaxAge allow negative numbers #2888
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2888 +/- ##
=======================================
Coverage 93.81% 93.81%
=======================================
Files 85 85
Lines 23410 23422 +12
=======================================
+ Hits 21961 21973 +12
Misses 1449 1449 ☔ View full report in Codecov by Sentry. |
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.
lgtm
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.
Just because the test comes from deno, doesnt mean that it makes sense or is even correct. I argued with the spec and imho the spec clearly states that sending a negative value means removes the cookie from the user agent. Do I have to first upstream the change on deno, before i can propose the change on undici? |
The logic is sound on deno's end. A cookie cannot expire on the server, therefore a negative value is invalid.
|
First of all, the logic on deno is not following the spec if we follow your argument. according to the spec 0 is not allowed but deno allows it. second of all, not allowing negative numbers makes it impossible to delete immediatly cookies in the browser. Following case:
This behaviour is described in the spec: |
You can set the value to 0 which will instantly delete the cookie. I don't know if it's a bug in Deno's implementation, but it does make sense that you wouldn't be allowed to set an already expired cookie. The max-age is implementation specific, and in an environment that doesn't expire cookies automatically, it doesn't make sense for them to be negative. |
This does not convince me. :( |
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.
Giving my two cents here, I do actually see the point @Uzlopak is making, given the arguments of the spec, but the point from @KhafraDev is pretty valid.
I do believe that the use case that handling -1
gives over max-age
on the set-cookie
header does not work in an environment that does not do automatic cookie management like Node, everything goes upon the implementation to decide how to manage it, it is of no use case for the Node at all (given how the set-cookie
is designed for).
On the other hand, surfacing this implementation by allowing this might not be of value for the implementer as cookies
are often of no importance in service-side communications.
My take is to keep it is as is, but won't block the PR from being merged
I don't have a strong opinion on this, but at the minimum an issue should be reported in the denostd repo. |
Seems like deno wants to restrict the maxAge to be a positive number |
I checked again deno and they purposely changed it because of the same reason I am argueing to allow negative numbers. |
@KhafraDev Will adapt this PR if deno merges my corresponding 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.