From e0ff57ca750d4c41b4af7c3f45b4655c33b6fc07 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Mon, 19 Dec 2016 18:00:22 +1100
Subject: [PATCH] SECURITY: prevent reuse of password reset

---
 app/controllers/application_controller.rb |  7 +++++++
 app/controllers/users_controller.rb       | 16 +++++++++-------
 lib/secure_session.rb                     | 18 ++++++++++++++++++
 spec/components/secure_session_spec.rb    | 16 ++++++++++++++++
 4 files changed, 50 insertions(+), 7 deletions(-)
 create mode 100644 lib/secure_session.rb
 create mode 100644 spec/components/secure_session_spec.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 98cafcd2f24..cec61bf9d6f 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -9,6 +9,7 @@ require_dependency 'json_error'
 require_dependency 'letter_avatar'
 require_dependency 'distributed_cache'
 require_dependency 'global_path'
+require_dependency 'secure_session'
 
 class ApplicationController < ActionController::Base
   include CurrentUser
@@ -381,6 +382,11 @@ class ApplicationController < ActionController::Base
     end
   end
 
+
+  def secure_session
+    SecureSession.new(session["secure_session_id"] ||= SecureRandom.hex)
+  end
+
   private
 
     def locale_from_header
@@ -558,6 +564,7 @@ class ApplicationController < ActionController::Base
       render_to_string status: status, layout: layout, formats: [:html], template: '/exceptions/not_found'
     end
 
+
   protected
 
     def render_post_json(post, add_raw=true)
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d67c12455e1..153dae27d39 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -389,19 +389,21 @@ class UsersController < ApplicationController
   def password_reset
     expires_now
 
-    if EmailToken.valid_token_format?(params[:token])
+    token = params[:token]
+
+    if EmailToken.valid_token_format?(token)
       if request.put?
-        @user = EmailToken.confirm(params[:token])
+        @user = EmailToken.confirm(token)
       else
-        email_token = EmailToken.confirmable(params[:token])
+        email_token = EmailToken.confirmable(token)
         @user = email_token.try(:user)
       end
 
       if @user
-        session["password-#{params[:token]}"] = @user.id
+        secure_session["password-#{token}"] = @user.id
       else
-        user_id = session["password-#{params[:token]}"]
-        @user = User.find(user_id) if user_id
+        user_id = secure_session["password-#{token}"].to_i
+        @user = User.find(user_id) if user_id > 0
       end
     else
       @invalid_token = true
@@ -420,7 +422,7 @@ class UsersController < ApplicationController
         @user.auth_token = nil
         if @user.save
           Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore
-          session["password-#{params[:token]}"] = nil
+          secure_session["password-#{token}"] = nil
           logon_after_password_reset
 
           return redirect_to(wizard_path) if Wizard.user_requires_completion?(@user)
diff --git a/lib/secure_session.rb b/lib/secure_session.rb
new file mode 100644
index 00000000000..bbe9a71833e
--- /dev/null
+++ b/lib/secure_session.rb
@@ -0,0 +1,18 @@
+# session that is not stored in cookie, expires after 1.hour unconditionally
+class SecureSession
+  def initialize(prefix)
+    @prefix = prefix
+  end
+
+  def [](key)
+    $redis.get("#{@prefix}#{key}")
+  end
+
+  def []=(key,val)
+    if val == nil
+      $redis.del("#{@prefix}#{key}")
+    else
+      $redis.setex("#{@prefix}#{key}", 1.hour, val.to_s)
+    end
+  end
+end
diff --git a/spec/components/secure_session_spec.rb b/spec/components/secure_session_spec.rb
new file mode 100644
index 00000000000..df5f5532532
--- /dev/null
+++ b/spec/components/secure_session_spec.rb
@@ -0,0 +1,16 @@
+require 'rails_helper'
+require_dependency 'secure_session'
+
+describe SecureSession do
+  it "operates correctly" do
+    s = SecureSession.new("abc")
+
+    s["hello"] = "world"
+    s["foo"] = "bar"
+    expect(s["hello"]).to eq("world")
+    expect(s["foo"]).to eq("bar")
+
+    s["hello"] = nil
+    expect(s["hello"]).to eq(nil)
+  end
+end