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

Password reset token leak via HTTP referer #40

Closed
williantenfen opened this issue Jan 20, 2017 · 6 comments · Fixed by #56
Closed

Password reset token leak via HTTP referer #40

williantenfen opened this issue Jan 20, 2017 · 6 comments · Fixed by #56
Labels
help wanted Community assistance requested

Comments

@williantenfen
Copy link

The reset password token is leaking through the HTTP referer header ... This happens when user clicks at the link sent to their email and when the page is rendered with the token at the URL, if there is some subsequent requests to CDN/analytics or malicious JS , the token will be present at the REFERER header of this requests... As reported at here: https://robots.thoughtbot.com/is-your-site-leaking-password-reset-links A solution will be to store the token to session and then redirect to the page without the token at the URL ...
Is there any chance to change this behavior to current versions?

@joshbuker
Copy link
Member

If you open a PR for this, we can definitely take a look and push out a new version. We'd also want to push to the 0.9.x branch and fix the security issue if possible.

@ebihara99999
Copy link
Contributor

Hi @athix
I looked over the issue, wondering which way is favorable, and I would be happy if you offer a suggestion.

The link offers 2 solutions: session based approach and immediate invalidation approach.
The first one is, as @williantenfen mentioned, is to store a token in session. This needs to modify documentation or templates because the password reset feature is implemented only in the model layer.
To embed a token to session instead of url, the following needs to be done:

  • Modify resources :password_resets to resource :password_resets in config/routes.rb.
  • Modify edit_password_reset_url(@user.reset_password_token) to edit_password_reset_url in app/mailers/user_mailer.rb
  • Add a token to session after @user.deliver_reset_password_instructions! succeeds in app/controllers/password_resets_controller.rb.
  • Modify params[:id], which stores a token, to session[:token] not to depend on url parameters including a token in app/controllers/password_resets_controller.rb.

To complete these instructions, it's required to modify documentation and generated templates.

The second approach is achieved by adding visited flag to database or session and modify #load_from_reset_password_token to set the flag. At the same time, it's necessary to remove implementions related to reset_password_token_expires_at and reset_password_email_sent_at because this way no longer needs them.

@ebihara99999
Copy link
Contributor

I'm sorry I don't intend to reference the commits from this issue. It's a mistake.

@ebihara99999
Copy link
Contributor

Hi @Ch4s3, would you suggest the way to go? I prefer the second way; because the most of the fix in the first way is about the documentation, it seems difficult to check if the modification is correct.

@ebihara99999
Copy link
Contributor

Hi @athix and @Ch4s3
I asked the direction about how to fix, the both are unclear and seems to be hard to answer...
To clarify I opened a PR. I appreciate if you check it.

@Ch4s3
Copy link
Contributor

Ch4s3 commented May 16, 2017

Sorry, I've been awol for a bit. I'm reviewing now.

@joshbuker joshbuker added the help wanted Community assistance requested label Aug 1, 2017
joshbuker pushed a commit that referenced this issue Apr 12, 2018
* Add access_count_to_reset_password_page to migration/reset_password.rb

* Add #increment_password_reset_page_access_counter and #reset_password_reset_page_access_counter.

* Add #increment_password_reset_page_access_counter and #reset_password_reset_page_access_counter

* Add test suites related to #increment_password_reset_page_access_counter and #reset_password_reset_page_access_counter.

* Refactor `#increment_password_reset_page_access_counter`

Change manual increment into using
Sorcery::Adapters::ActiveRecordAdapter.increment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Community assistance requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants