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

reset session at sign_in for protect from session fixation attacks. #108

Merged

Conversation

madogiwa0124
Copy link
Contributor

@madogiwa0124 madogiwa0124 commented Dec 4, 2021

It is recommended to reset the session at sign in for protect from session fixation attacks.
So reseted session in Passwordless::ControllerHelpers#sign_in.

2.7 Session Fixation - Countermeasures
One line of code will protect you from session fixation.
The most effective countermeasure is to issue a new session identifier and declare the old one invalid after a successful login.
That way, an attacker cannot use the fixed session identifier.
This is a good countermeasure against session hijacking, as well.
Here is how to create a new session in Rails:
reset_session
https://guides.rubyonrails.org/security.html#session-fixation-countermeasures

ex. Sorcery::Controller::InstanceMethods#login in Sorcery

old_session = session.dup.to_hash
reset_sorcery_session
old_session.each_pair do |k, v|
  session[k.to_sym] = v
end

https://github.com/Sorcery/sorcery/blob/6fdc703416b3ff8d05708b05d5a8228ab39032a5/lib/sorcery/controller.rb#L52-L56

How about this PR?

fixed #107

It is recommended to reset the session at sign in for protect from session fixation attacks.
So  reseted session in `Passwordless::ControllerHelpers#sign_in`.

> 2.7 Session Fixation - Countermeasures
> One line of code will protect you from session fixation.
> The most effective countermeasure is to issue a new session identifier and declare the old one invalid after a successful login.
> That way, an attacker cannot use the fixed session identifier.
> This is a good countermeasure against session hijacking, as well.
> Here is how to create a new session in Rails:
> reset_session
> https://guides.rubyonrails.org/security.html#session-fixation-countermeasures

ex. `Sorcery::Controller::InstanceMethods#login` in Sorcery

``` ruby
old_session = session.dup.to_hash
reset_sorcery_session
old_session.each_pair do |k, v|
  session[k.to_sym] = v
end
```

https://github.com/Sorcery/sorcery/blob/6fdc703416b3ff8d05708b05d5a8228ab39032a5/lib/sorcery/controller.rb#L52-L56
@rickychilcott
Copy link
Collaborator

This seems reasonable to me. I'm apt to approve and merge. @madogiwa0124 have you tried your fork in one of your own applications to ensure there are no downsides to this approach?

Thank you for jumping on this.

@madogiwa0124
Copy link
Contributor Author

@rickychilcott

Thanks for your comment!

I just tried forking on my application and it seems to be working fine. Here is what I actually did.

  • Existing tests for my application passed.
  • URL login with the generated session worked.
  • The session id changed every time I do a URL login.

@mikker
Copy link
Owner

mikker commented Dec 7, 2021

Thank you both for helping out. Don't forget a changelog entry if you decide to include this, @rickychilcott 😊

@rickychilcott
Copy link
Collaborator

My review on this one is good, but let me spin up a test project here and merge/changlog/bump if all goes well.

@rickychilcott rickychilcott merged commit d5a80d1 into mikker:master Dec 19, 2021
@rickychilcott
Copy link
Collaborator

I bumped to version 0.11.0 and added a changelog entry in 74e7530. @mikker I'm not sure I have the ability to release the gem. If that's something you want to give to me, great. Otherwise, I wonder if we could work up a github action flow to possibly auto-publish.

@rickychilcott
Copy link
Collaborator

https://andrewm.codes/blog/automating-ruby-gem-releases-with-github-actions/ could be a good read and an hour of work to save pain for the future :)

@mikker
Copy link
Owner

mikker commented Dec 19, 2021

You can absolutely be a maintainer on RubyGems. Appreciate all your help here very much. Do I just use the e-mail address that I already have?

@rickychilcott
Copy link
Collaborator

Same email. I couldn't readily find that email communication, so... I commented here. Thanks!

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.

Potentially affected by Session Fixation attack
3 participants