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

req.session.save() overrides existing Set-Cookie header values #112

Closed
mwisner opened this issue May 19, 2020 · 5 comments · Fixed by #117
Closed

req.session.save() overrides existing Set-Cookie header values #112

mwisner opened this issue May 19, 2020 · 5 comments · Fixed by #117
Labels

Comments

@mwisner
Copy link

mwisner commented May 19, 2020

https://github.com/vvo/next-iron-session/blob/master/lib/index.js#L62-L65

I "think" this line: res.setHeader("set-cookie", [cookieValue] is assuming that there were no other attempts to set cookies before calling req.session.save()

It looks like destroy does something similar: https://github.com/vvo/next-iron-session/blob/master/lib/index.js#L73

I think these calls don't seem to keep any existing values in the Set-Cookie header?

This doesn't seem to work:


const postHandler = async (req, res) => {
  destroyCookie({ req, res }, 'token');
  req.session.destroy();
  res.status(200).json({ message: 'logged out' }).end();
};

But this works fine:

const postHandler = async (req, res) => {
  req.session.destroy();
  destroyCookie({ req, res }, 'token');
  res.status(200).json({ message: 'logged out' }).end();
};

*destoyCookie comes from this package: https://github.com/maticzav/nookies which I am using to manage other cookies.

This kind of hung me up a little bit and required a fair bit of debugging and moving code around trying to understand why my extra cookies weren't being set.

vvo added a commit that referenced this issue May 26, 2020
This commit fixes a case where previous set-cookie headers were ignored through
the request lifecycle. This is now fixed and handles both single and multiple
set-cookie header values. Since in Node.js you can do:

res.setHeader("set-cookie", "name=value"); and
res.setHeader("set-cookie", ["name=value", "name2=value2"])

fixes #112
@vvo
Copy link
Owner

vvo commented May 26, 2020

@mwisner thanks for the report, I have been working on a fix for this. When this is released can you try again? Thanks!

@vvo vvo closed this as completed in #117 May 26, 2020
vvo added a commit that referenced this issue May 26, 2020
This commit fixes a case where previous set-cookie headers were ignored through
the request lifecycle. This is now fixed and handles both single and multiple
set-cookie header values. Since in Node.js you can do:

res.setHeader("set-cookie", "name=value"); and
res.setHeader("set-cookie", ["name=value", "name2=value2"])

fixes #112
@vvo
Copy link
Owner

vvo commented May 26, 2020

🎉 This issue has been resolved in version 4.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vvo vvo added the released label May 26, 2020
@lkbr
Copy link

lkbr commented Sep 9, 2020

Hi there, I think #117 fixed req.session.save() but the change was never applied to req.session.destroy()which still seems to override attempts to set a cookies. Atleast for me.

@vvo
Copy link
Owner

vvo commented Oct 1, 2020

(handling this in another issue @lkbr, solved)

@lkbr
Copy link

lkbr commented Oct 1, 2020

🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants