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

clearCookie should probably only set one header per cookie name test case #4542

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StefanWallin
Copy link

While developing a session termination feature in our apps I noticed that a cookie set by our session middleware to maintain cookie expiration was kept in the response even though the clearCookie was set at a later request handler. This test case attempts to replicate this circumstances.

According to https://tools.ietf.org/html/rfc6265#section-4.1.1:

Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name. (See Section 5.2 for
how user agents handle this case.)

Do you want filtration of setCookie-headers like this case to be handled within the expressjs framework, if so I could create a PR to fix that, or is this something that should be handled by expressjs users?

@dougwilson
Copy link
Contributor

Hi @StefanWallin that is correct, all the cookie-setting functions just add to the outbound set-cookie header correctly. res.clearCookie is just a res.cookie with an expiration in the past (the http cookie spec doesn't have a special "clear cookie" command).

Unfortunately it is quite hard to actually filter them out and thankfully the spec is SHOULD NOT and not MUST NOT. Web browsers out there currently have no issues receiving them like this and processing them in order. Part of what makes it hard to filter out is that you cannot just filter on the cookie's name -- you have to implement the full cookie-matching algorithm just to know if two of the cookies are the same cookie.

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

Successfully merging this pull request may close these issues.

2 participants