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

Fix MaxAge check. #103

Closed
wants to merge 1 commit into from
Closed

Fix MaxAge check. #103

wants to merge 1 commit into from

Conversation

jmcarp
Copy link

@jmcarp jmcarp commented Jan 14, 2017

Since MaxAge of 0 means cookies shouldn't be expired, only delete cookies with MaxAge < 0. Plus, update tests to verify that session files have been deleted or not, as appropriate.

[Resolves #96]

cc @adborden

@kisielk
Copy link
Contributor

kisielk commented Jan 27, 2017

This seems like the right thing to do but I'm a bit concerned about the change in semantics. If people have been using the package and setting MaxAge to 0 to delete sessions, now these session will no longer be deleted. That could be a surprising / harmful change in behaviour.

@elithrar what do you think?

@elithrar
Copy link
Contributor

elithrar commented Jan 27, 2017

https://godoc.org/github.com/gorilla/sessions?importers -> 661 importers. That's not going to show all the non-OSS repos out there that import us, but we could scan quickly to see how common MaxAge 0 vs. -1 is and understand the context in those cases.

(Otherwise, this change is API breaking and the side-effect - not deleting a session - is fairly severe)

@kisielk
Copy link
Contributor

kisielk commented Mar 21, 2018

Now that there's some kind of versioning possible, maybe we should look at merging this?

@stale
Copy link

stale bot commented Dec 9, 2018

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

@stale stale bot added the stale label Dec 9, 2018
@stale stale bot removed the stale label Dec 9, 2018
@stale
Copy link

stale bot commented Jun 30, 2019

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

@stale stale bot added the stale label Jun 30, 2019
@stale stale bot closed this Jul 7, 2019
@mengstr
Copy link

mengstr commented Apr 15, 2021

This is a really annoying limitation as most end-users expect a session to go away when he closes the browser tab (if not checked a "keep me logged in" box at the site). This especially on sites with a bit higher stakes of keeping a login lingering, like banking, healthcare and the likes.

Could this be fixed by use a "magical" value instead of 0 for producing a session cookie? Like 1 second (which would have no real use), or maybe -4242 seconds which no sane developer would use anyways.

Or maybe simply a new option like SessionCookie:true that overrides whatever the MaxAge is set to?

@jaitaiwan
Copy link
Member

Even though a bot closed this out well before I became a maintainer I wanted to follow this up to say that in #271 I discuss the specific logic that's happening and from what I can tell this is expected behaviour. The fact that in some browsers the cookie stays around until the user closes the browser seems to me to be browser-specific behaviour and not spec-compliance.

If folks disagree, and they may well, we can talk about what possible changes should be made.

Further discussion should occur in #271.

@gorilla gorilla locked as resolved and limited conversation to collaborators Jun 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MaxAge checks against wrong value in Save func
5 participants