From f293288589f24e1928b57dcd3428b762ae9ced79 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Oct 2017 21:28:40 +0900 Subject: Security fix: redirection in google_api/authorizations_controller --- .../google_api/authorizations_controller.rb | 9 +++++++-- app/controllers/projects/clusters_controller.rb | 10 +++++++--- lib/google_api/cloud_platform/client.rb | 5 +++++ .../google_api/authorizations_controller_spec.rb | 23 ++++++++++++++-------- spec/lib/google_api/cloud_platform/client_spec.rb | 23 ++++++++++++++++++++++ 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) } -- cgit v1.2.1