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

[BUG] FilesystemStore MaxAge==0 does set cookies that expire when browser closes #267

Closed
1 task done
HTechHQ opened this issue Aug 4, 2023 · 5 comments
Closed
1 task done
Labels

Comments

@HTechHQ
Copy link

HTechHQ commented Aug 4, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Using the file store sessions.NewFilesystemStore("", []byte("secret")) and
saving a new session with the Options.MaxAge of 0 will lead to
error: remove /tmp/session_36KLXHXBPO7WQCZCDAULFDR73W7ON6445Y2YCWYZMY26QPCYJQSA: no such file or directory

Expected Behavior

The session should be working as 0 as a value indicates to the browser to delete the cookie ones the browser is closed.

Steps To Reproduce

No response

Anything else?

The issue has been discussed in #96 and #103

The main objection seems to have been the breaking of depending projects.
As the current behaviour of the FilesystemStore is wildly different from the expected web standards, I think it is worth the step. Especially with the new revamp of the gorillas project, a new version could make this a a viable option for everyone.

@HTechHQ HTechHQ added the bug label Aug 4, 2023
@maragunde93
Copy link

Is there any update on this or a work around? We need to have 'Session' cookies too so it gets deleted when the client closes the browser.

@mengstr
Copy link

mengstr commented Feb 7, 2024

Yes please. Working session (temporary) cookies are sorely missed. If patching the obvious(?) bug would make existing usages break, then just add some other functions to handle session cookies.

Having to fork this project to change if session.Options.MaxAge <= 0 { into if session.Options.MaxAge < 0 { feels so silly. Doesn't most prjoject nowadays need session cookies to ease the issues with GDPR and et.al?

@jaitaiwan
Copy link
Member

Hey folks, feel free to open a PR with a failing test and we can prioritise a fix for this.

@jaitaiwan
Copy link
Member

jaitaiwan commented Jun 15, 2024

As far as I can tell folks, I looked at the http spec and this seems compliant:

If delta-seconds is less than or equal to zero (0), let expiry-time
be the earliest representable date and time.

From: https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.2

What's the behaviour that you're wanting to see happen? So far as I can tell, the only way to ensure the session cookie stays around in the browser is to ensure that the max-age is set to a positive integer.

I replied about the specific logic in #271

@jaitaiwan jaitaiwan changed the title [BUG] FilesystemStore MaxAge does not allow cookies until browser closes [BUG] FilesystemStore MaxAge==0 does set cookies that expire when browser closes Jun 15, 2024
@jaitaiwan
Copy link
Member

As to the error that's appearing, the if statement is supposed to cover that:

if err := s.erase(session); err != nil && !os.IsNotExist(err) {

The !os.IsNotExists(err) should ensure that there's no error returned when the session doesn't exist.

If you have a different perspective feel free to speak out and I'll do my best to work with you. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

No branches or pull requests

4 participants