microcipcip / cookie-universal

Universal cookie plugin, perfect for SSR
526 stars 39 forks source link

removeAll() Doesn't Work Because Of Wrong Expire Date #89

Closed XiNiHa closed 3 years ago

XiNiHa commented 3 years ago

Calling removeAll() on the server side doesn't work(at least in Firefox) because of its invalid expire date value. The browser console message says that the cookies(with empty values) are being ignored because those are already invalidated. No idea how to fix this though, should it send some far future or very close future, like 5secs later to be invalidated ASAP?

microcipcip commented 3 years ago

I'll have to experiment with this, I think it was working when I tested it, but maybe the browser's behavior has changed since then. What I am currently doing is setting the expires value to new Date(0).

What I have experienced sometimes is that the cookie is removed but still visible in the Developers tools. A refresh of the page usually removes it from there too.

XiNiHa commented 3 years ago

The cookie was simply not being accepted with some warnings on the console, so it isn't a problem of Devtools. BTW I use Firefox - not sure what was the exact version - so it could be the problem

microcipcip commented 3 years ago

I tried to run the express demo in Firefox (with cookies.removeAll()) and I got the screenshot below back, which is expected I think. Is this what you got as well?

Capture

XiNiHa commented 3 years ago

Yeah exactly. Hope to see this fixed ASAP!

microcipcip commented 3 years ago

@XiNiHa isn't that expected though? I am expiring the cookie so that it becomes invalid. I think this is what other libraries do as well, it is equivalent to removing it. See for example this Stackoverflow question.

Edit: actually MDN states the following

You can delete a cookie by updating its expiration time to zero.

So maybe I just need to set it to 0 and see if the warning goes away. I have to test it.

XiNiHa commented 3 years ago

AFAIK that warnings are saying that the cookies that are trying to invalidate existing cookies are blocked so the existing cookies are not affected by that invalidation attempt. Not sure though. Setting the age directly seems good

XiNiHa commented 3 years ago

Weird, just tested with code below and everything worked well(With warnings, but the cookie was removed). Haven't checked for the library itself but it seems like the issue was not a thing. Looks good to close after verification.

const express = require('express')
const app = express()

app.get('/add', (req, res) => {
    res.cookie('test', '1234')
    res.sendStatus(200)
})

app.get('/remove', (req, res) => {
    res.cookie('test', '', { expires: new Date(0) })
    res.sendStatus(200)
})

app.get('/echo', (req, res) => {
    res.status(200)
    res.json(req.headers.cookie)
})

app.listen(8080, () => { console.log("server is listening") })
XiNiHa commented 3 years ago

Just tested with the library by tweaking the code above a bit and everything worked well - not sure why I thought it was not removing cookies. Sorry for opening an invalid issue ;(

microcipcip commented 3 years ago

@XiNiHa thanks for checking it.