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

destroySession doesn't delete cookie if max-age is set #5150

Closed
AlexErrant opened this issue Jan 19, 2023 · 6 comments · Fixed by #7252
Closed

destroySession doesn't delete cookie if max-age is set #5150

AlexErrant opened this issue Jan 19, 2023 · 6 comments · Fixed by #7252
Assignees
Labels
bug Something isn't working feat:sessions Sessions related issues

Comments

@AlexErrant
Copy link

AlexErrant commented Jan 19, 2023

What version of Remix are you using?

v1.11.0

Steps to Reproduce

  1. Create a cookie session storage with maxAge set
  storage = createCookieSessionStorage({
    cookie: {
      name: "__Host-session",
      secure: true,
      sameSite: "lax",
      path: "/",
      maxAge: 60 * 60 * 24 * 30, // 30 days
      httpOnly: true,
    },
  })
  1. Destroy it
  const session = await storage.getSession(request.headers.get("Cookie"))
  const headers = new Headers()
  headers.append("Set-Cookie", await storage.destroySession(session))

Expected Behavior

I expected the cookie to be deleted.

Actual Behavior

Instead, I get a set-cookie response header that looks like

__Host-session=; Max-Age=2592000; Path=/; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Secure; SameSite=Lax

Remix blanks out the session, which is great, and causes logouts to work as expected.

Remix also sets the Expires set to the epoch, which is a spec-compliant way of deleting the cookie:

servers can delete cookies by sending the user agent a new cookie with an Expires attribute with a value in the past.

However, the spec also states:

If a cookie has both the Max-Age and the Expires attribute, the Max-Age attribute has precedence and controls the expiration date of the cookie.

My interpretation is that if Max-Age is set, setting the Expires to the epoch doesn't change the cookie's expiration. This is reflected in Chrome, where the above response header does not result in the cookie being deleted.

Code_YLatP5tpkB

May I suggest implementing this code as

        return cookie.serialize("", {
          ...options,
          maxAge: undefined,
          expires: new Date(0)
        });
@github-actions
Copy link
Contributor

This issue has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Apr 18, 2023
@AlexErrant
Copy link
Author

Ulgh.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 19, 2023
@brophdawg11 brophdawg11 added the feat:sessions Sessions related issues label May 5, 2023
@mmacdonaldOCP
Copy link

Having this issue as well

@brophdawg11 brophdawg11 self-assigned this Aug 3, 2023
@brophdawg11 brophdawg11 added this to v2 Aug 3, 2023
@brophdawg11 brophdawg11 assigned mjackson and unassigned brophdawg11 Aug 3, 2023
@brophdawg11 brophdawg11 moved this to Backlog in v2 Aug 3, 2023
@brophdawg11 brophdawg11 self-assigned this Aug 24, 2023
@brophdawg11 brophdawg11 moved this from Backlog to In progress in v2 Aug 24, 2023
@brophdawg11 brophdawg11 added bug Something isn't working and removed bug:unverified labels Aug 24, 2023
@brophdawg11 brophdawg11 moved this from In progress to PR in v2 Aug 24, 2023
@brophdawg11
Copy link
Contributor

This is fixed by #7252 and will be available in Remix v2

@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Aug 25, 2023
@brophdawg11 brophdawg11 moved this from PR to Merged in v2 Aug 25, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-f77588e-20230826 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat:sessions Sessions related issues
Projects
No open projects
Status: Merged
Development

Successfully merging a pull request may close this issue.

4 participants