Fix issue #40

[ebihara99999]

Fix Password reset token leak via HTTP referer.
This change requires to update the documentation accordingly, and I will explain how to fix it in implementation and the documentation (written below).

Implementation

I follow the step of the first direction mentioned in Is Your Site Leaking Password Reset Links?:

Update the passwords#edit controller action so it immediately invalidates the password reset token in the request and generates a new one that is used in the form action. The Referer will still contain the original password reset token, but the token is no longer valid.

• Add access_count_to_reset_password_page attribute to user model, responsible for the count how many times the password reset page is accessed.
  In lib/sorcery/model/submodules/reset_password.rb,
• Add #increment_password_reset_page_access_counter, which increments access_count_to_reset_password_page attribute.
• Add #reset_password_reset_page_access_counter. This resets access_count_to_reset_password_page attribute to 0

I will explain how these changes affect the password reset feature in a rails application below.

Documentation

The wiki is expected to update as follows only PasswordResetsController#create and PasswordResetsController#edit:

class PasswordResetsController < ApplicationController
  skip_before_filter :require_login
  
  def create
    @user = User.find_by_email(params[:email])
    
    #### Difference Start#####
    # Sends an email to the user with instructions on how to reset their password (a url with a random token)
    # Don't forget to reset password_reset_page_access_counter
    # For security reset password page can be accessed only once.
    if @user
      @user.deliver_reset_password_instructions!
      @user.reset_password_reset_page_access_counter
    end
    #### Difference End #####
    
    # Tell the user instructions have been sent whether or not email was found.
    # This is to not leak information to attackers about which emails exist in the system.
    redirect_to(root_path, :notice => 'Instructions have been sent to your email.')
  end
  
  # This is the reset password form.
  def edit
    @token = params[:id]
    @user = User.load_from_reset_password_token(params[:id])
    
    if @user.blank?
      not_authenticated
      return
    end
    
    #### Difference Start #####
    # Increment password reset page access counter
    @user.increment_password_reset_page_access_counter

    #  password reset page can't be accessed more than twice
    if @user.access_count_to_reset_password_page > 1
      # It's nice to override #not_authenticated to show a message of the reason not authenticated.
      not_authenticated
      return
    end
    #### Difference End #####
  end
end

Testing

Sorcery

All specs are green.

Documentation

Test the steps mentioned in Is Your Site Leaking Password Reset Links?, confirming the second access is invalid.

1. Request a password reset and click the link that is emailed to you.
2. Copy the URL from the resulting page.
3. Open a private browser window or a different web browser and paste the copied URL.
   If a working password reset form is rendered then you have proven that without any mitigating steps, any Referer generated by this page would contain a working password reset URL.

[ebihara99999]

@athix
I know you have a lot to do, thanks for letting me notice.
This is a security issue (though the risk is low) and needs to be released.
It's been long, I would like to explain the point.

The risk:

• the password reset url including a reset token, is set to refferer when a user accesses to the passoword reset page
• the url is leaked if the user accesses another link from the password reset page: considered the case of the access to external links from the footer

Considering these points, barring the leak is achieved by the restriction of one time access.
Because the report offers 2 solutions - the restriction of one time access and including a token in session - let me compare them.

The restriction of one time access is:

• safe because the token is invalidated even though it's leaked
• more simple, more understandable and easier to be tested

I think it is the better way to prevent the leak, and I would appreciate if you move on to reviewing my codes.

[ebihara99999]

@athix
And I can help you more with dealing with and closing issues, reviewing other PRs if you let me in a member. I would appreciate if you think about it.
This is misunderstanding. Without authority, can deal with them. I'm sorry.

[joshbuker]

@Ch4s3 do you have the time to review/merge this? I probably won't be able to get to it for at least a couple weeks.

[Ch4s3]

@athix and @ebihara99999 this looks good. I'll merge it once I have some time to update the docs.

[Ch4s3]

I'll try to merge this tonight