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 <jens.ulferts@googlemail.com>

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

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

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

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

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