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

Add feature of regeneration for #34 #74

Closed
wants to merge 4 commits into from

Conversation

palmtale
Copy link

@palmtale palmtale commented May 5, 2017

Add feature of regeneration for #34

Signed-off-by: Cartoon Zhang <cartoon.zhang@zeofast.com>
Signed-off-by: Cartoon Zhang <cartoon.zhang@zeofast.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling dcede0c on so-glad:feature/regeneration into 777c8e8 on koajs:master.

@palmtale
Copy link
Author

palmtale commented May 5, 2017

Just await ctx.regenerateSession() to regen a new session. I've tested in an env of Mac koa^2.2.0 with redis store and cookie store.
But not write any new code of test module

@dead-horse
Copy link
Member

can't we just use ctx.session = {} instead of regeneration ?

@palmtale
Copy link
Author

@dead-horse
No, no, no, no, no.
1st, In real effect, I've tested, it's not OK, did not do regeneration, just clean the store of the correspond user session key, because of the set() function using this.create(val, externalKey) .

2nd, In the semantics, ctx.session={}, as well as set(), did correct logic. But what we want to do in 'regeneration', is to regenerate a new session key/code, for avoiding hack attack.

@dead-horse
Copy link
Member

@palmtale need to add test cases for this feature

index.js Outdated
@@ -105,6 +105,13 @@ function extendContext(context, opts) {
return this[_CONTEXT_SESSION];
},
},
regenerateSession: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to add too many properties in ctx, maybe we can move this method to ctx.session.regenerate(), and it should be an async function.

@coveralls
Copy link

coveralls commented Jun 18, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 919968a on so-glad:feature/regeneration into 777c8e8 on koajs:master.

@palmtale
Copy link
Author

@dead-horse How about this, set signal _requireRegenerate in session, and do it in commit as the same as save.
And also added the same test module in cookie and external store.
Actually, it's not required to clear the content of session, if really need, use ctx.session={};

@smh
Copy link

smh commented Sep 30, 2017

@dead-horse Any hope of getting this merged? As it is now, we cannot use this module as we need a way to regenerate the session key when we authenticate the user. As @palmtale says, setting ctx.session = {} will empty the session, but the session key remains the same, and this leaves us open to session fixation attacks.

@lehni
Copy link
Contributor

lehni commented Nov 8, 2022

Would be great to see this merged, also due to jaredhanson/passport#907

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

Successfully merging this pull request may close these issues.

None yet

6 participants