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.

Sign up for GitHub

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

Jump to bottom

clearCookie should set maxAge not expires #3856

Closed
chrsmith opened this issue Jan 15, 2019 · 3 comments
Closed

clearCookie should set maxAge not expires #3856

chrsmith opened this issue Jan 15, 2019 · 3 comments

Comments

@chrsmith
Copy link

Calling res.clearCookie will fail to delete a cookie if a maxAge value is passed in the cookie's options. This is because of a specific detail in how the Set-Cookie header works and what I believe to be a bug in the Express implementation. (This may be the root cause for some users who came across #691.)

Repro

const cookieSettings = {
    httpOnly: false,
    maxAge: 1000 * 60 * 60 * 24 * 14,  // 14 days.
    secure: false,
};

// Testing cookie issues. In a browser make the following requests:
//
// /cookie-test?add=foo
// /cookie-test?delete=foo
// /cookie-test
//
// Expected: Don't see "foo" in the list of cookies.
// Observed: "foo" is in the list of cookies.
app.get("/cookie-test", (req, res) => {
    let body = "<html><pre>";
    if (req.query["add"]) {
        body += `Adding cookie: ${req.query["add"]}\n`;
        res.cookie(req.query["add"], (new Date()).toString(), cookieSettings);
    }

    if (req.query["delete"]) {
        body += `Deleting cookie: ${req.query["delete"]}\n`;
        res.clearCookie(req.query["delete"], cookieSettings);
    }

    body += `Cookies:${JSON.stringify(req.cookies, null, 2)}`;
    body += "</pre>";

    console.log(`=============\n${body}\n\n`);
    res.status(200).send(body);
});

Cause
The problem is rooted in the implementation of clearCookie:

res.clearCookie = function clearCookie(name, options) {
  var opts = merge({ expires: new Date(1), path: '/' }, options);

  return this.cookie(name, '', opts);
};

The expires field is set, which if that timestamp has passed, the cookie will be deleted. However, there is a separate cookie setting maxAge which can be used as well. And from the RFC, the maxAge setting takes precedence.

https://tools.ietf.org/html/rfc6265#section-4.1
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

So if you call clearCookie with the cookie settings that contains a maxAge, the result will be both the expires and maxAge properties will be set. And since the maxAge property will take precedence, the cookie will not be deleted. (It's value will be cleared out, however.)

I believe the right fix would just to be explicitly set maxAge on the merged cookie options, intentionally overwriting whatever settings were passed by the user. Since obviously the intent is to clear the cookie and not persist it. e.g.

res.clearCookie = function clearCookie(name, options) {
-  var opts = merge({ expires: new Date(1), path: '/' }, options);
+  var opts = merge({ path: '/' }, options);
+  opts.maxAge = 0;
  return this.cookie(name, '', opts);
};

Assuming this all sounds right, I'm happy to submit a PR to fix it.
@dougwilson
Copy link
Contributor

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?

@chrsmith
Copy link
Author

  1. Does the documentation clearly explain this

Assuming this is the right place to look, IMHO there are some ways to make the docs clearer. (This is less about express and more about the sometimes surprising semantics of cookies.)
https://expressjs.com/en/api.html#res.clearCookie

For example, having an example specifically for deleting cookies, and mentioning that clearCookie may just clear the value of the cookie depending on if maxAge or expires is present in the cookie options, etc.

  1. Should the behavior be changed in the next major release to not allow users to keep the cookie?

Perhaps not? If clearCookie isn't intended to delete cookies, then making a breaking change is probably too high of a bar. But having a separate deleteCookie method might be reasonable; but with better docs perhaps that wouldn't be necessary.

Anyways, thanks for the additional detail. I guess it makes sense to close this out as working as intended, and I'll try to find the time to submit a PR to clarify the documentation to hopefully prevent other folk from making the same faulty assumptions that I did.

@rvolgers
Copy link

rvolgers commented Jun 15, 2019 •
edited
Loading

Anyways, thanks for the additional detail. I guess it makes sense to close this out as working as intended, and I'll try to find the time to submit a PR to clarify the documentation to hopefully prevent other folk from making the same faulty assumptions that I did.

For what it's worth I just ran into this and think the current logic is not really defensible.

The clearCookie can be used to just clear the value and not actually delete the cookie if the user doesn't want to.

A much more obvious way to do that would be to use res.cookie() with an empty value. Being able to do this with clearCookie as well does not seem valuable to me. At least not enough to justify this API wart.

Also note that the clearCookie documentation does not mention the "setting the cookie to an empty string" use case at all. The documentation does however say that passing expires or maxAge will result in the cookie not actually being cleared, blaming "standards".

I don't feel like we can blame standards here. The standards don't have a clearCookie API. ExpressJS came up with that, and I think it should try harder to make that API live up to its name.

Now I recognize this is probably a pointless rant as it probably should be considered a breaking change. Just wanted to put this out there. Carry on.

@jonchurch jonchurch mentioned this issue May 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dougwilson @chrsmith @rvolgers