search

expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.62k stars 16.21k forks source link

`res.clearCookie()` does not ignore `maxAge` #4851

Open tjarbo opened 2 years ago

tjarbo commented 2 years ago

Hi everyone! I just ran into a bug, where res.clearCookie() does not work properly.

What happen?

According to the typescript definitions, res.clearCookie() accepts CookieOptions as a second parameter (see here) which includes the maxAge attribute. But if the maxAge is set, the cookie won't be deleted.

What do I expect?

.clearCookie()should ignore (or delete) the maxAge attribute, because it is used to calculate the expire attribute afterwards in .cookie();

Research

I already located the bug and would like to provide a pr to fix this.

yagmurmutluer9 commented 2 years ago

I had the same problem a few days ago, thanks for the pr i want to try when it merged

dougwilson commented 2 years ago

Thank you for your work on this @tjarbo !

dougwilson commented 2 years ago

Please see #4252 for related discussion. This was original designed this way on purpose (ugh), and I see it being used in the wild this way. We can land such a change in the 5.0 branch, so I'm setting it to 5.0.

Segmentational commented 2 years ago

Added a review for tgarbo's PR

tjarbo commented 2 years ago

Thank you @Segmentational !

kaj commented 1 year ago

Since #4252 is closed, I'll continue the discussion here:

As all the other options (domain, sameSite, etc) needs to be the same when clearing the cookie as when setting it, the natural thing to do is use the same const OPTIONS when clearing the current cookie as when setting it. Anything that depends on the current behaviour is obviously broken. If a new major is needed to fix this, then a new major is needed asap.

Big thanks to @tjarbo for identifying the problem and provding a PR!

jonchurch commented 6 months ago

I don't think this is semver major, I think this is just a bug which we can fix in a patch

According to the v4 docs https://expressjs.com/en/api.html#res.clearCookie

Clears the cookie specified by name. For details about the options object, see res.cookie().

Web browsers and other compliant clients will only clear the cookie if the given options is identical to those given to res.cookie(), excluding expires and maxAge.

The implementation today does not reflect the documented behavior. The method itself does not have the ability to "clear cookies" at all, it relies on browser behavior. And if we allow maxAge or expires to be passed to the newly created cookie we return, then it does not accomplish the intent of the method.

This is the entire intent of the method, and it has a bug today.

jonchurch commented 6 months ago

I think this is an incorrect interpretation of the docs and intent behind the commits related that got us here:

Although, I think it would still be breaking to fix it in v4. So hands are tied here ughhh, see https://github.com/expressjs/express/pull/4852#issuecomment-2094520956

https://github.com/expressjs/express/issues/4252#issuecomment-619222441

Basically the res.clearCookie API will delete the cookie unless you provide a maxAge or an expires, in which case it only clears the value of the cookie. This is as designed currently.

and https://github.com/expressjs/express/issues/4252#issuecomment-619226285

A user-supplied expires to res.clearCookie should be what is used in the final Set-Cookie header (unless maxAge is also provided, in which case maxAge takes precedence).

And more here https://github.com/expressjs/express/issues/3856#issuecomment-454545957

Everything you said is right, except as you can see in the current implementation, a user is allowed to even override the expires date. The clearCookie can be used to just clear the value and not actually delete the cookie if the user doesn't want to. This is why explicitly passing in either expires or maxAge will cause the cookie to remain in the browser, as that is currently how res.clearCookie is designed.

This means that you are observing the correct behavior which is not a bug.

Now, that being said, perhaps the discussion is one (or both?) of the following:

  1. Does the documentation clearly explain this
  2. Should the behavior be changed in the next major release to not allow users to keep the cookie?

commit history

Support for options first came in in v2.3.0 https://github.com/expressjs/express/commit/dc02b0d5a

There was no indication in the change that it was meant to allow a cookie's value to be cleared by this method, while still persisting the cookie clientside via extended expires.

"Clearing" a cookie means deleting it in the browser by setting expires to a value in the far past. That is the understanding of the method as it came in w/ https://github.com/expressjs/express/commit/041b7e8a2 during v2 and when it was first conceived https://github.com/expressjs/express/issues/314

During the v4 release, there is no mention of supporting this alternative usecase for res.clearCookie, which would prevent setting expires to the proper value to ensure deletion from the client.

It's my opinion that the options feature introduced introduced a bug by allowing folks to prevent clearCookie from doing it's job.

@expressjs/express-tc thoughts here?

The question is, do we consider this a bug? Do we fix it in v4 or do we consider the fact that users can abuse this method an officially supported feature?