Security fix: redirection in google_api/authorizations_controller

5 files changed, 57 insertions, 13 deletions

diff --git a/app/controllers/google_api/authorizations_controller.rb b/app/controllers/google_api/authorizations_controller.rb
index e4f76fb493e..709d1d34796 100644
--- a/app/controllers/google_api/authorizations_controller.rb
+++ b/app/controllers/google_api/authorizations_controller.rb
@@ -9,8 +9,13 @@ module GoogleApi
       session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at] =
         expires_at.to_s
 
-      if params[:state].present?
-        redirect_to params[:state]
+      key, _ = GoogleApi::CloudPlatform::Client
+        .session_key_for_second_redirect_uri(secure: params[:state])
+
+      second_redirect_uri = session[key]
+
+      if second_redirect_uri.present?
+        redirect_to second_redirect_uri
       else
         redirect_to root_path
       end
diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb
index ce8e73392b8..2f7364f4abf 100644
--- a/app/controllers/projects/clusters_controller.rb
+++ b/app/controllers/projects/clusters_controller.rb
@@ -16,9 +16,13 @@ class Projects::ClustersController < Projects::ApplicationController
 
   def login
     begin
-      @authorize_url = GoogleApi::CloudPlatform::Client.new(
-        nil, callback_google_api_auth_url,
-        state: namespace_project_clusters_url.to_s).authorize_url
+      GoogleApi::CloudPlatform::Client.session_key_for_second_redirect_uri.tap do |key, secure|
+        session[key] = namespace_project_clusters_url.to_s
+
+        @authorize_url = GoogleApi::CloudPlatform::Client.new(
+          nil, callback_google_api_auth_url,
+          state: secure).authorize_url
+      end
     rescue GoogleApi::Auth::ConfigMissingError
       # no-op
     end
diff --git a/lib/google_api/cloud_platform/client.rb b/lib/google_api/cloud_platform/client.rb
index 5ec1fa37546..6d0c148b261 100644
--- a/lib/google_api/cloud_platform/client.rb
+++ b/lib/google_api/cloud_platform/client.rb
@@ -15,6 +15,11 @@ module GoogleApi
         def session_key_for_expires_at
           :cloud_platform_expires_at
         end
+
+        def session_key_for_second_redirect_uri(secure: nil)
+          secure = SecureRandom.hex unless secure
+          return "cloud_platform_second_redirect_uri_#{secure}", secure
+        end
       end
 
       def scope
diff --git a/spec/controllers/google_api/authorizations_controller_spec.rb b/spec/controllers/google_api/authorizations_controller_spec.rb
index 64c16af582f..5413f20c83c 100644
--- a/spec/controllers/google_api/authorizations_controller_spec.rb
+++ b/spec/controllers/google_api/authorizations_controller_spec.rb
@@ -3,12 +3,10 @@ require 'spec_helper'
 describe GoogleApi::AuthorizationsController do
   describe 'GET|POST #callback' do
     let(:user) { create(:user) }
-    let(:project) { create(:project) }
-    let(:state) { project_clusters_url(project).to_s }
     let(:token) { 'token' }
     let(:expires_at) { 1.hour.since.strftime('%s') }
 
-    subject { get :callback, code: 'xxx', state: state }
+    subject { get :callback, code: 'xxx', state: @state }
 
     before do
       sign_in(user)
@@ -17,7 +15,7 @@ describe GoogleApi::AuthorizationsController do
         .to receive(:get_token).and_return([token, expires_at])
     end
 
-    it 'sets token and expires_atin session' do
+    it 'sets token and expires_at in session' do
       subject
 
       expect(session[GoogleApi::CloudPlatform::Client.session_key_for_token])
@@ -26,15 +24,24 @@ describe GoogleApi::AuthorizationsController do
         .to eq(expires_at)
     end
 
-    context 'when redirection url is stored in state' do
+    context 'when second redirection url key is stored in state' do
+      set(:project) { create(:project) }
+      let(:second_redirect_uri) { namespace_project_clusters_url(project.namespace, project).to_s } # TODO: revrt
+
+      before do
+        GoogleApi::CloudPlatform::Client
+          .session_key_for_second_redirect_uri.tap do |key, secure|
+          @state = secure
+          session[key] = second_redirect_uri
+        end
+      end
+
       it 'redirects to the URL stored in state param' do
-        expect(subject).to redirect_to(state)
+        expect(subject).to redirect_to(second_redirect_uri)
       end
     end
 
     context 'when redirection url is not stored in state' do
-      let(:state) { '' }
-
       it 'redirects to root_path' do
         expect(subject).to redirect_to(root_path)
       end
diff --git a/spec/lib/google_api/cloud_platform/client_spec.rb b/spec/lib/google_api/cloud_platform/client_spec.rb
index 6538dc21d6f..e770f2e9edc 100644
--- a/spec/lib/google_api/cloud_platform/client_spec.rb
+++ b/spec/lib/google_api/cloud_platform/client_spec.rb
@@ -4,6 +4,29 @@ describe GoogleApi::CloudPlatform::Client do
   let(:token) { 'token' }
   let(:client) { described_class.new(token, nil) }
 
+  describe '.session_key_for_second_redirect_uri' do
+    subject { described_class.session_key_for_second_redirect_uri(secure: secure) }
+
+    context 'when pass a postfix' do
+      let(:secure) { SecureRandom.hex }
+
+      it 'creates a required session key' do
+        key, _ = described_class.session_key_for_second_redirect_uri(secure: secure)
+        expect(key).to eq("cloud_platform_second_redirect_uri_#{secure}")
+      end
+    end
+
+    context 'when pass a postfix' do
+      let(:secure) { nil }
+
+      it 'creates a new session key' do
+        key, secure = described_class.session_key_for_second_redirect_uri
+        expect(key).to include('cloud_platform_second_redirect_uri_')
+        expect(secure).not_to be_nil
+      end
+    end
+  end
+
   describe '#validate_token' do
     subject { client.validate_token(expires_at) }