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

Login session ends when browser session ends, cookie maxAge ignored #2365

Closed
4 tasks done
zivchen opened this issue Jan 22, 2023 · 7 comments · Fixed by #2366
Closed
4 tasks done

Login session ends when browser session ends, cookie maxAge ignored #2365

zivchen opened this issue Jan 22, 2023 · 7 comments · Fixed by #2366

Comments

@zivchen
Copy link
Contributor

zivchen commented Jan 22, 2023

New Issue Checklist

Issue Description

cookie-session - maxAge property has no effect, at the moment its in an object

Steps to reproduce

Just open chrome inspect elements and see that the cookie expires on session

Actual Outcome

cookie expires on session

Expected Outcome

cookie will expire in 2 weeks

Environment

Dashboard

  • Parse Dashboard version: 5.0.0
  • Browser (Safari, Chrome, Firefox, Edge, etc.): Chrome, Firefox (all...)
  • Browser version: ``

The problem is at Authentication.js row 59, maxAge should be a prop of the configuration object and not nested in cookie :{}

See actual API
https://github.com/expressjs/cookie-session

A feature request for this bug will be to have maxAge property via config

Would love to make a PR...

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jan 22, 2023

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

@mtrezza
Copy link
Member

mtrezza commented Jan 22, 2023

Could you provide more details about what the change is you're suggesting?

@zivchen
Copy link
Contributor Author

zivchen commented Jan 23, 2023

@mtrezza
Sure
The fix it self is easy (put maxAge in the root object of cookie-session initialization) but it will change the current behavior (session only cookie) that i assume people are use to.
I created a PR that adds a cookieMaxAge configuration option and chain it all the way similar to cookieSessionSecret so if not provided it will remain the way it is today.
PR:
#2366

@mtrezza
Copy link
Member

mtrezza commented Jan 23, 2023

the cookie expires on session

What do you mean by that?

it will change the current behavior (session only cookie)

Could you give more details what the current behavior is vs. the new behavior?

@zivchen
Copy link
Contributor Author

zivchen commented Jan 24, 2023

@mtrezza

What do you mean by that?

The current bug in maxAge definition makes the package cookie-session to add a login cookie with no maxAge / expires which means the browser will save it for the current session only (it doesn't do "remember me")

Could you give more details what the current behavior is vs. the new behavior?

Well the bug is easily fixed, but its very old which means i assume that the dashboard users are use to the "buggy" behavior which is session only cookie and not 2 weeks cookie as the code tried to make, so my PR keeps session only cookie by default but gives the ability to change by config to a longer cookie ( 2weeks or even more)

To be honest i just need my dashboard users not to do a login every time and that it will remember them

@mtrezza
Copy link
Member

mtrezza commented Jan 24, 2023

the browser will save it for the current session only

You are referring to the browser session, not to the dashboard login session, correct?

In other words, the current behavior is:

  1. user logs in to dashboard
  2. user closes browser and reopens browser
  3. user needs to re-log in to dashboard

The behavior you expect is:

  1. user logs in to dashboard
  2. user closes browser and reopens browser
  3. user only needs to re-log in to dashboard if the cookie has expired

This would be a breaking change that relates to security. It requires users to behave differently and manually log out to end the dashboard session.

We could:

  • Add a dashboard config option that allows to set the max cookie age.
  • Keep the current behavior as default in that a dashboard session is limited to the browser session.

@zivchen
Copy link
Contributor Author

zivchen commented Jan 24, 2023

@mtrezza
Yes and yes, my PR handle these steps the same way you wrote it.
The reason i opened this issue as a bug is because the code currently has this maxAge definition but just not in the correct place.

@mtrezza mtrezza changed the title cookie is session only , maxAge ignored Login session ends when browser session ends, cookie maxAge ignored Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants