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

Allow specific session keys to bypass reset_session #63

Merged
merged 14 commits into from
Jan 17, 2024

Conversation

grega
Copy link
Member

@grega grega commented Jan 11, 2024

Adds session_keys_to_persist config option in order to carry over specific session keys from pre-log in to post-log in

(since reset_session is called post-log in in order to prevent session fixation: https://guides.rubyonrails.org/security.html#session-fixation-countermeasures)

@grega
Copy link
Member Author

grega commented Jan 11, 2024

spec/dummy/spec/requests/auth_request_spec.rb:185 is failing due to the session apparently being stripped 🤷‍♂️

Test puts debug output below:

Initial session
{"session_id"=>"932565ccbd6f22ac33af628c6f5b6d8f", "omniauth.params"=>{}, "foo"=>"bar"}

Old / pre-reset_session session:
{"session_id"=>"932565ccbd6f22ac33af628c6f5b6d8f"}

Logged in (/rpi_auth/auth/callback) session:
{"session_id"=>"e39d3637a2f1256107fd8efc33d73d0d", "current_user"=>{"user_id"=>"3ed9b57a-1eb9-42e1-ae54-9a11c930f035", "country"=>nil, "country_code"=>"GB", "email"=>"[email protected]", "email_verified"=>nil, "name"=>"Sylvester McBean", "nickname"=>"Sylvie", "picture"=>"https://placecage.com/100/100", "postcode"=>nil, "profile"=>"https://my.raspberry.pi/profile/edit", "roles"=>nil}}

The session resetting after /rpi_auth/auth/callback is expected, due to reset_session being called, but I can't figure out why the session params are stripped between the first two instances. It's perhaps suspicious that omniauth.params is removed also, have dug around OmniAuth source but not found anything of note as yet.


Fixed in: 8ddfe50

@grega grega marked this pull request as ready for review January 16, 2024 14:17
@grega
Copy link
Member Author

grega commented Jan 16, 2024

Moved CI / Bundle faff to: #66

@patch0
Copy link
Contributor

patch0 commented Jan 16, 2024

This all looks fine to me :)

Copy link

github-actions bot commented Jan 17, 2024

Test coverage: 100%

README.md Show resolved Hide resolved
@grega grega merged commit abda12d into main Jan 17, 2024
10 checks passed
@grega grega deleted the bypass-reset-session-for-certain-keys branch January 17, 2024 15:20
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.

3 participants