From 0fa8b4a77b9ebc5442e3e321293ba632633ec64f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 1 Mar 2021 22:18:03 +0100 Subject: [PATCH] Forward CSP extensions to login controller (#9047) * Forward CSP extensions to login controller * Extend spec for double auth code * stabilize spec On my machine, since I have an s3 bucket configured, the spec failed Co-authored-by: ulferts --- app/controllers/account_controller.rb | 8 + app/controllers/oauth/auth_base_controller.rb | 23 ++- config/initializers/secure_headers.rb | 6 - .../oauth/authorization_code_flow_spec.rb | 145 +++++++++++------- spec/requests/auth/oauth_login_csp_spec.rb | 52 +++++++ 5 files changed, 169 insertions(+), 65 deletions(-) create mode 100644 spec/requests/auth/oauth_login_csp_spec.rb diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index b41674b0f9e..438eb5a5e24 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -41,6 +41,7 @@ class AccountController < ApplicationController # prevents login action to be filtered by check_if_login_required application scope filter skip_before_action :check_if_login_required + before_action :apply_csp_appends, only: %i[login] before_action :disable_api before_action :check_auth_source_sso_failure, only: :auth_source_sso_failed @@ -577,4 +578,11 @@ class AccountController < ApplicationController redirect_to home_url end + + def apply_csp_appends + appends = flash[:_csp_appends] + return unless appends + + append_content_security_policy_directives(appends) + end end diff --git a/app/controllers/oauth/auth_base_controller.rb b/app/controllers/oauth/auth_base_controller.rb index c41935f0556..fe4dae3a395 100644 --- a/app/controllers/oauth/auth_base_controller.rb +++ b/app/controllers/oauth/auth_base_controller.rb @@ -34,18 +34,29 @@ module OAuth # because it needs to set a specific return URL # See config/initializers/doorkeeper.rb class AuthBaseController < ::ApplicationController + # Ensure we prepend the CSP extension + # before any other action is being performed + prepend_before_action :extend_content_security_policy + skip_before_action :check_if_login_required - after_action :extend_content_security_policy layout 'only_logo' def extend_content_security_policy - use_content_security_policy_named_append(:oauth) + return unless pre_auth&.authorizable? + + additional_form_actions = application_redirect_uris + return if additional_form_actions.empty? + + flash[:_csp_appends] = { form_action: additional_form_actions } + append_content_security_policy_directives flash[:_csp_appends] end - def allowed_forms - allowed_redirect_urls = pre_auth&.client&.application&.redirect_uri - urls = allowed_redirect_urls.to_s.split - urls.map { |url| URI.join(url, '/') }.map(&:to_s) + def application_redirect_uris + pre_auth&.client&.application&.redirect_uri + .to_s + .split + .select { |url| url.start_with?('http') } + .map { |url| URI.join(url, '/').to_s } end end end diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb index 6d3c231fe45..db1c6fbb287 100644 --- a/config/initializers/secure_headers.rb +++ b/config/initializers/secure_headers.rb @@ -73,9 +73,3 @@ SecureHeaders::Configuration.default do |config| connect_src: connect_src } end - -SecureHeaders::Configuration.named_append(:oauth) do |request| - hosts = request.controller_instance.try(:allowed_forms) || [] - - { form_action: hosts } -end diff --git a/spec/features/oauth/authorization_code_flow_spec.rb b/spec/features/oauth/authorization_code_flow_spec.rb index f22a00ca714..6bff64e8046 100644 --- a/spec/features/oauth/authorization_code_flow_spec.rb +++ b/spec/features/oauth/authorization_code_flow_spec.rb @@ -41,45 +41,7 @@ describe 'OAuth authorization code flow', "/oauth/authorize?response_type=code&client_id=#{client_id}&redirect_uri=#{CGI.escape(redirect_url)}&scope=api_v3" end - it 'can authorize and manage an OAuth application grant' do - visit oauth_path app.uid, redirect_uri - - # Expect we're guided to the login screen - login_with user.login, 'adminADMIN!', visit_signin_path: false - - # We get to the authorization screen - expect(page).to have_selector('h2', text: 'Authorize Cool API app!') - - # With the correct scope printed - expect(page).to have_selector('li strong', text: I18n.t('oauth.scopes.api_v3')) - expect(page).to have_selector('li', text: I18n.t('oauth.scopes.api_v3_text')) - - first = true - allow_any_instance_of(::OAuth::AuthBaseController) - .to receive(:allowed_forms).and_wrap_original do |m| - forms = m.call - - # Multiple requests end up here with one not containing the request url - if first - expect(forms).to include redirect_uri - first = false - end - - forms - end - - SeleniumHubWaiter.wait - # Authorize - find('input.button[value="Authorize"]').click - - # Expect auth token - code = find('#authorization_code').text - - # And also have a grant for this application - user.oauth_grants.reload - expect(user.oauth_grants.count).to eq 1 - expect(user.oauth_grants.first.application).to eq app - + def get_and_test_token(code) parameters = { client_id: app.uid, client_secret: client_secret, @@ -96,6 +58,34 @@ describe 'OAuth authorization code flow', expect(body['access_token']).to be_present expect(body['refresh_token']).to be_present expect(body['scope']).to eq 'api_v3' + end + + it 'can authorize and manage an OAuth application grant' do + visit oauth_path app.uid, redirect_uri + + # Expect we're guided to the login screen + login_with user.login, 'adminADMIN!', visit_signin_path: false + + # We get to the authorization screen + expect(page).to have_selector('h2', text: 'Authorize Cool API app!') + + # With the correct scope printed + expect(page).to have_selector('li strong', text: I18n.t('oauth.scopes.api_v3')) + expect(page).to have_selector('li', text: I18n.t('oauth.scopes.api_v3_text')) + + SeleniumHubWaiter.wait + # Authorize + find('input.button[value="Authorize"]').click + + # Expect auth token + code = find('#authorization_code').text + + # And also have a grant for this application + user.oauth_grants.reload + expect(user.oauth_grants.count).to eq 1 + expect(user.oauth_grants.first.application).to eq app + + get_and_test_token(code) # Should show that grant in my account visit my_account_path @@ -145,26 +135,75 @@ describe 'OAuth authorization code flow', end context 'with real urls as allowed redirect uris' do - let!(:redirect_uri) { "https://foo.com/foo " } + let!(:redirect_uri) { "https://foo.com/foo" } let!(:allowed_redirect_uri) { "#{redirect_uri} https://bar.com/bar" } it 'can authorize and manage an OAuth application grant' do visit oauth_path app.uid, redirect_uri - allow_any_instance_of(::OAuth::AuthBaseController) - .to receive(:allowed_forms).and_wrap_original do |m| - forms = m.call - - expect(forms).to include 'https://foo.com/' - expect(forms).to include 'https://bar.com/' - - forms - end - # Check that the hosts of allowed redirection urls are present in the content security policy - expect(page.response_headers['content-security-policy']).to( - include("form-action 'self' https://foo.com/ https://bar.com/;") - ) + + form_csp_header = page + .response_headers['content-security-policy'] + .split(';') + .find { |s| s.start_with?(' form-action') } + + expect(form_csp_header) + .to include("'self'") + + expect(form_csp_header) + .to include('foo.com') + + expect(form_csp_header) + .to include('bar.com') end end end + + context 'when redirecting to a stubbed foreign service', driver: :chrome_billy do + let!(:redirect_uri) { "https://oauth.example.com/callback" } + + before do + proxy + .stub("https://oauth.example.com:443/callback") + .and_return(code: 200, text: 'Welcome to stubbed response') + end + + it 'can be authorized twice (Regression #34554)' do + visit oauth_path app.uid, redirect_uri + + # Expect we're guided to the login screen + login_with user.login, 'adminADMIN!', visit_signin_path: false + + # We get to the authorization screen + expect(page).to have_selector('h2', text: 'Authorize Cool API app!') + + # Authorize + find('input.button[value="Authorize"]').click + + # Expect redirect to stubbed URL + expect(page).to have_current_path(/#{Regexp.escape(redirect_uri)}\?code\=.+$/, url: true) + expect(page).to have_text 'Welcome to stubbed response' + + # Get auth token from URL query + code = page.current_url.match(/\?code=(.+)$/)[1] + get_and_test_token(code) + + # Log out again + logout + + # Try to reauthorize with existing grant + visit oauth_path app.uid, redirect_uri + + # Expect we're guided to the login screen + login_with user.login, 'adminADMIN!', visit_signin_path: false + + # Expect redirect to stubbed URL + expect(page).to have_current_path(/#{Regexp.escape(redirect_uri)}\?code\=.+$/, url: true) + expect(page).to have_text 'Welcome to stubbed response' + + # Get auth token from URL query + new_code = page.current_url.match(/\?code=(.+)$/)[1] + get_and_test_token(new_code) + end + end end diff --git a/spec/requests/auth/oauth_login_csp_spec.rb b/spec/requests/auth/oauth_login_csp_spec.rb new file mode 100644 index 00000000000..ada6726bcc0 --- /dev/null +++ b/spec/requests/auth/oauth_login_csp_spec.rb @@ -0,0 +1,52 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe 'CSP appends on login form from oauth', + type: :rails_request do + let!(:redirect_uri) { 'https://foobar.com' } + let!(:oauth_app) { FactoryBot.create(:oauth_application, redirect_uri: redirect_uri) } + let(:oauth_path) do + "/oauth/authorize?response_type=code&client_id=#{oauth_app.uid}&redirect_uri=#{CGI.escape(redirect_uri)}&scope=api_v3" + end + + context 'when login required', with_settings: { login_required: true } do + it 'appends given CSP appends from flash' do + get oauth_path + + csp = response.headers['Content-Security-Policy'] + expect(csp).to include "form-action 'self' https://foobar.com/;" + + location = response.headers['Location'] + expect(location).to include("/login?back_url=#{CGI.escape(oauth_path)}") + end + end +end