Don't use return_to, but instead pass state with signed return_to parameter

6 files changed, 42 insertions, 36 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9852736..0d27134 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -1,4 +1,6 @@
 class ApplicationController < ActionController::Base
+  include UserSessionsHelper
+
   rescue_from Network::UnauthorizedError, with: :invalid_token
   before_filter :default_headers
   before_filter :check_config
@@ -39,7 +41,7 @@ class ApplicationController < ActionController::Base
   def authenticate_public_page!
     unless project.public
       unless current_user
-        redirect_to(new_user_sessions_path(return_to: request.fullpath)) and return
+        redirect_to(new_user_sessions_path(state: generate_oauth_state(request.fullpath))) and return
       end
 
       unless current_user.can_access_project?(project.gitlab_id)
diff --git a/app/controllers/user_sessions_controller.rb b/app/controllers/user_sessions_controller.rb
index e486b24..23e4182 100644
--- a/app/controllers/user_sessions_controller.rb
+++ b/app/controllers/user_sessions_controller.rb
@@ -1,6 +1,4 @@
 class UserSessionsController < ApplicationController
-  include UserSessionsHelper
-
   before_filter :authenticate_user!, except: [:new, :callback, :auth]
 
   def show
@@ -11,9 +9,14 @@ class UserSessionsController < ApplicationController
   end
 
   def auth
+    unless is_oauth_state_valid?(params[:state])
+      redirect_to new_user_sessions_path
+      return
+    end
+
     redirect_to client.auth_code.authorize_url({
       redirect_uri: callback_user_sessions_url,
-      state: generate_oauth_state(params[:return_to])
+      state: params[:state]
     })
   end
 
diff --git a/app/helpers/user_sessions_helper.rb b/app/helpers/user_sessions_helper.rb
index e5853b5..df158c6 100644
--- a/app/helpers/user_sessions_helper.rb
+++ b/app/helpers/user_sessions_helper.rb
@@ -3,17 +3,18 @@ module UserSessionsHelper
     SecureRandom.hex(16)
   end
 
-  def generate_oauth_secret(salt, return_to)
+  def generate_oauth_hmac(salt, return_to)
     return unless return_to
-    message = GitlabCi::Application.config.secret_key_base + salt + return_to
-    Digest::SHA256.hexdigest message
+    digest = OpenSSL::Digest.new('sha256')
+    key = GitlabCi::Application.config.secret_key_base + salt
+    OpenSSL::HMAC.hexdigest(digest, key, return_to)
   end
 
   def generate_oauth_state(return_to)
     return unless return_to
     salt = generate_oauth_salt
-    secret = generate_oauth_secret(salt, return_to)
-    "#{salt}:#{secret}:#{return_to}"
+    hmac = generate_oauth_hmac(salt, return_to)
+    "#{salt}:#{hmac}:#{return_to}"
   end
 
   def get_ouath_state_return_to(state)
@@ -22,8 +23,8 @@ module UserSessionsHelper
 
   def is_oauth_state_valid?(state)
     return true unless state
-    salt, secret, return_to = state.split(':', 3)
+    salt, hmac, return_to = state.split(':', 3)
     return false unless return_to
-    secret == generate_oauth_secret(salt, return_to)
+    hmac == generate_oauth_hmac(salt, return_to)
   end
 end
diff --git a/app/views/projects/_public.html.haml b/app/views/projects/_public.html.haml
index 11eccbd..9662cc9 100644
--- a/app/views/projects/_public.html.haml
+++ b/app/views/projects/_public.html.haml
@@ -3,7 +3,7 @@
     Public projects
 
 .bs-callout
-  = link_to new_user_sessions_path(return_to: request.fullpath) do
+  = link_to new_user_sessions_path(state: generate_oauth_state(request.fullpath)) do
     %strong Login with GitLab
   to see your private projects
 
diff --git a/app/views/user_sessions/new.html.haml b/app/views/user_sessions/new.html.haml
index b457e93..c5be95b 100644
--- a/app/views/user_sessions/new.html.haml
+++ b/app/views/user_sessions/new.html.haml
@@ -4,5 +4,5 @@
     Make sure you have account on GitLab server
     = link_to GitlabCi.config.gitlab_server.url, GitlabCi.config.gitlab_server.url, no_turbolink
   %hr
-  = link_to "Login with GitLab", auth_user_sessions_path(return_to: params[:return_to]), no_turbolink.merge( class: 'btn btn-login btn-success' )
+  = link_to "Login with GitLab", auth_user_sessions_path(state: params[:state]), no_turbolink.merge( class: 'btn btn-login btn-success' )
 
diff --git a/spec/helpers/user_sessions_helper_spec.rb b/spec/helpers/user_sessions_helper_spec.rb
index 1fbbad4..4af735c 100644
--- a/spec/helpers/user_sessions_helper_spec.rb
+++ b/spec/helpers/user_sessions_helper_spec.rb
@@ -1,34 +1,34 @@
 require 'spec_helper'
 
 describe UserSessionsHelper do
-  describe :generate_oauth_secret do
-    let (:salt) { "a" }
-    let (:salt2) { "b" }
-    let (:return_to) { "b" }
+  describe :generate_oauth_hmac do
+    let (:salt) { 'a' }
+    let (:salt2) { 'b' }
+    let (:return_to) { 'b' }
 
-    it "should return null if return_to is also null" do
-      generate_oauth_secret(salt, nil).should be_nil
+    it 'should return null if return_to is also null' do
+      generate_oauth_hmac(salt, nil).should be_nil
     end
 
-    it "should return not null if return_to is also not null" do
-      generate_oauth_secret(salt, return_to).should_not be_nil
+    it 'should return not null if return_to is also not null' do
+      generate_oauth_hmac(salt, return_to).should_not be_nil
     end
 
-    it "should return different secrets for different salts" do
-      secret1 = generate_oauth_secret(salt, return_to)
-      secret2 = generate_oauth_secret(salt, return_to)
+    it 'should return different hmacs for different salts' do
+      secret1 = generate_oauth_hmac(salt, return_to)
+      secret2 = generate_oauth_hmac(salt, return_to)
       secret1.should eq(secret2)
     end
   end
 
   describe :generate_oauth_state do
-    let (:return_to) { "b" }
+    let (:return_to) { 'b' }
 
-    it "should return null if return_to is also null" do
+    it 'should return null if return_to is also null' do
       generate_oauth_state(nil).should be_nil
     end
 
-    it "should return two different states for same return_to" do
+    it 'should return two different states for same return_to' do
       state1 = generate_oauth_state(return_to)
       state2 = generate_oauth_state(return_to)
       state1.should_not eq(state2)
@@ -36,31 +36,31 @@ describe UserSessionsHelper do
   end
 
   describe :get_ouath_state_return_to do
-    let (:return_to) { "a" }
+    let (:return_to) { 'a' }
     let (:state) { generate_oauth_state(return_to) }
 
-    it "should return return_to" do
+    it 'should return return_to' do
       get_ouath_state_return_to(state).should eq(return_to)
     end
   end
 
   describe :is_oauth_state_valid? do
-    let (:return_to) { "a" }
+    let (:return_to) { 'a' }
     let (:state) { generate_oauth_state(return_to) }
     let (:forged) { "forged#{state}" }
-    let (:invalid) { "aa" }
-    let (:invalid2) { "aa:bb" }
-    let (:invalid3) { "aa:bb:" }
+    let (:invalid) { 'aa' }
+    let (:invalid2) { 'aa:bb' }
+    let (:invalid3) { 'aa:bb:' }
 
-    it "should validate oauth state" do
+    it 'should validate oauth state' do
       is_oauth_state_valid?(state).should be_true
     end
 
-    it "should not validate forged state" do
+    it 'should not validate forged state' do
       is_oauth_state_valid?(forged).should be_false
     end
 
-    it "should not validate invalid state" do
+    it 'should not validate invalid state' do
       is_oauth_state_valid?(invalid).should be_false
       is_oauth_state_valid?(invalid2).should be_false
       is_oauth_state_valid?(invalid3).should be_false