diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb index f8f7425b06..ba8c19c7f8 100644 --- a/app/controllers/oauth_clients_controller.rb +++ b/app/controllers/oauth_clients_controller.rb @@ -128,7 +128,7 @@ class OAuthClientsController < ApplicationController # This needs to be modified as soon as we support more integration types. if User.current.admin && state && nextcloud? yield - elsif state + elsif ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin? state flash[:error] = [t(:'oauth_client.errors.oauth_issue_contact_admin')] redirect_to state else diff --git a/app/services/oauth_clients/connection_manager.rb b/app/services/oauth_clients/connection_manager.rb index 8e1c737984..6e54275be0 100644 --- a/app/services/oauth_clients/connection_manager.rb +++ b/app/services/oauth_clients/connection_manager.rb @@ -87,6 +87,10 @@ module OAuthClients # In the current implementation "state" just consists of the URL of # the initial page, possibly with "&var=value" added parameters. # So we can just return this URI. + unless ::API::V3::Utilities::PathHelper::ApiV3Path::same_origin?(state) + return ::API::V3::Utilities::PathHelper::ApiV3Path::root_url + end + state end @@ -137,9 +141,9 @@ module OAuthClients # Localize the error message def i18n_rack_oauth2_error_message(rack_oauth2_client_exception) - l10n_key = "oauth_client.errors.rack_oauth2.#{rack_oauth2_client_exception.message}" - if I18n.exists? l10n_key - I18n.t(l10n_key) + i18n_key = "oauth_client.errors.rack_oauth2.#{rack_oauth2_client_exception.message}" + if I18n.exists? i18n_key + I18n.t(i18n_key) else "#{I18n.t('oauth_client.errors.oauth_returned_error')}: #{rack_oauth2_client_exception.message.to_html}" end @@ -148,12 +152,22 @@ module OAuthClients # Return a fully configured RackOAuth2Client. # This client does all the heavy lifting with the OAuth2 protocol. def rack_oauth_client(options = {}) + rack_oauth_client = build_basic_rack_oauth_client + + # Write options, for example authorization_code and refresh_token + rack_oauth_client.refresh_token = options[:refresh_token] if options[:refresh_token] + rack_oauth_client.authorization_code = options[:authorization_code] if options[:authorization_code] + + rack_oauth_client + end + + def build_basic_rack_oauth_client oauth_client_uri = URI.parse(@oauth_client.integration.host) oauth_client_scheme = oauth_client_uri.scheme oauth_client_host = oauth_client_uri.host oauth_client_port = oauth_client_uri.port - client = Rack::OAuth2::Client.new( + Rack::OAuth2::Client.new( identifier: @oauth_client.client_id, secret: @oauth_client.client_secret, scheme: oauth_client_scheme, @@ -162,11 +176,6 @@ module OAuthClients authorization_endpoint: "/apps/oauth2/authorize", token_endpoint: "/apps/oauth2/api/v1/token" ) - - # Write options, for example authorization_code and refresh_token - client.refresh_token = options[:refresh_token] if options[:refresh_token] - client.authorization_code = options[:authorization_code] if options[:authorization_code] - client end # Create a new OpenProject token object based on the return values diff --git a/lib/api/v3/utilities/path_helper.rb b/lib/api/v3/utilities/path_helper.rb index 79176c373f..499ec95eac 100644 --- a/lib/api/v3/utilities/path_helper.rb +++ b/lib/api/v3/utilities/path_helper.rb @@ -92,6 +92,10 @@ module API "#{root_path}api/v3" end + def self.same_origin?(url) + url.to_s.start_with? root_url + end + index :action show :action diff --git a/spec/requests/oauth_clients/callback_flow_spec.rb b/spec/requests/oauth_clients/callback_flow_spec.rb index 47053ea0b9..a7261567e6 100644 --- a/spec/requests/oauth_clients/callback_flow_spec.rb +++ b/spec/requests/oauth_clients/callback_flow_spec.rb @@ -39,7 +39,9 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do "mBf4v9hNA6hXXCWHd5mZggsAa2FSOXinx9jKx1yjSoDwOPOX4k6zGEgM2radqgg1nRwXCqvIe5xZsfwqMIaTdL" + "jYnl0OpYOc6ePblzQTmnlp7RYiHW09assYEJjv9zps" end - let(:state) { "https://example.org/my-path?and=some&query=params" } + let(:state) do + File.join(::API::V3::Utilities::PathHelper::ApiV3Path::root_url, "/my-path?and=some&query=params") + end let(:oauth_client_token) { create :oauth_client_token } let(:oauth_client) do create :oauth_client, @@ -57,7 +59,6 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do subject(:response) { last_response } before do - host! 'https://my-example.org' login_as current_user allow(::Rack::OAuth2::Client).to receive(:new).and_return(rack_oauth2_client) @@ -84,6 +85,13 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do end end + shared_examples 'fallback redirect' do + it 'redirects to home' do + expect(response.status).to eq 302 + expect(URI(response.location).path).to eq home_path + end + end + context 'with valid params' do context 'without errors' do before do @@ -103,6 +111,19 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do end end + context 'with a state param containing a URL pointing to a different host' do + let(:state) { "https://some-other-domain.com/foo/bar" } + + before do + uri.query = URI.encode_www_form([['code', code], ['state', state]]) + get uri.to_s + + subject + end + + it_behaves_like 'fallback redirect' + end + context 'with some other error, having a state param' do before do allow(::OAuthClients::ConnectionManager) @@ -136,7 +157,7 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do subject end - context 'with current_user being not being an admin' do + context 'with current_user not being an admin' do it_behaves_like 'with errors and state param, not being admin' end @@ -155,9 +176,6 @@ describe 'OAuthClient callback endpoint', :enable_storages, type: :request do subject end - it 'redirects to home' do - expect(response.status).to eq 302 - expect(URI(response.location).path).to eq home_path - end + it_behaves_like 'fallback redirect' end end