diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 1120d9e8a1442558fb3972255a5bfe51ccfbcd12..8026dfc4357a0210ca2182a9ec7b0b2bae2e1172 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -2398,7 +2398,6 @@ RSpec/ContextWording: - 'spec/requests/api/usage_data_spec.rb' - 'spec/requests/api/users_preferences_spec.rb' - 'spec/requests/api/users_spec.rb' - - 'spec/requests/content_security_policy_spec.rb' - 'spec/requests/dashboard/projects_controller_spec.rb' - 'spec/requests/dashboard_controller_spec.rb' - 'spec/requests/git_http_spec.rb' diff --git a/app/controllers/acme_challenges_controller.rb b/app/controllers/acme_challenges_controller.rb index 4a7706db94e6b06074eb51382fb8e955b1f328e7..a187e43b3dfd217d258de4f3d5dd7654a63f6aa4 100644 --- a/app/controllers/acme_challenges_controller.rb +++ b/app/controllers/acme_challenges_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# rubocop:disable Rails/ApplicationController -class AcmeChallengesController < ActionController::Base +class AcmeChallengesController < BaseActionController def show if acme_order render plain: acme_order.challenge_file_content, content_type: 'text/plain' @@ -16,4 +15,3 @@ def acme_order @acme_order ||= PagesDomainAcmeOrder.find_by_domain_and_token(params[:domain], params[:token]) end end -# rubocop:enable Rails/ApplicationController diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f4d9d616851cfeed6a95b2e5cb81a8ad8ae94099..8156cf8e165a1895385f753576241357e1bbea3b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,7 +3,7 @@ require 'gon' require 'fogbugz' -class ApplicationController < ActionController::Base +class ApplicationController < BaseActionController include Gitlab::GonHelper include Gitlab::NoCacheHeaders include GitlabRoutingHelper @@ -25,7 +25,6 @@ class ApplicationController < ActionController::Base include FlocOptOut include CheckRateLimit include RequestPayloadLogger - extend ContentSecurityPolicyPatch before_action :limit_session_time, if: -> { !current_user } before_action :authenticate_user!, except: [:route_not_found] @@ -113,33 +112,6 @@ def self.endpoint_id_for_action(action_name) render plain: e.message, status: :service_unavailable end - content_security_policy do |p| - next if p.directives.blank? - - if helpers.vite_enabled? - vite_host = ViteRuby.instance.config.host - vite_port = ViteRuby.instance.config.port - vite_origin = "#{vite_host}:#{vite_port}" - http_origin = "http://#{vite_origin}" - ws_origin = "ws://#{vite_origin}" - wss_origin = "wss://#{vite_origin}" - gitlab_ws_origin = Gitlab::Utils.append_path(Gitlab.config.gitlab.url, 'vite-dev/') - http_path = Gitlab::Utils.append_path(http_origin, 'vite-dev/') - - connect_sources = p.directives['connect-src'] - p.connect_src(*(Array.wrap(connect_sources) | [ws_origin, wss_origin, http_path])) - - worker_sources = p.directives['worker-src'] - p.worker_src(*(Array.wrap(worker_sources) | [gitlab_ws_origin, http_path])) - end - - next unless Gitlab::CurrentSettings.snowplow_enabled? && !Gitlab::CurrentSettings.snowplow_collector_hostname.blank? - - default_connect_src = p.directives['connect-src'] || p.directives['default-src'] - connect_src_values = Array.wrap(default_connect_src) | [Gitlab::CurrentSettings.snowplow_collector_hostname] - p.connect_src(*connect_src_values) - end - def redirect_back_or_default(default: root_path, options: {}) redirect_back(fallback_location: default, **options) end diff --git a/app/controllers/base_action_controller.rb b/app/controllers/base_action_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..05ba00426c27898c9d33bcf2c34aa9c7cfdc798e --- /dev/null +++ b/app/controllers/base_action_controller.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# GitLab lightweight base action controller +# +# This class should be limited to content that +# is desired/required for *all* controllers in +# GitLab. +# +# Most controllers inherit from `ApplicationController`. +# Some controllers don't want or need all of that +# logic and instead inherit from `ActionController::Base`. +# This makes it difficult to set security headers and +# handle other critical logic across *all* controllers. +# +# Between this controller and `ApplicationController` +# no controller should ever inherit directly from +# `ActionController::Base` +# +# rubocop:disable Rails/ApplicationController -- This class is specifically meant as a base class for controllers that +# don't inherit from ApplicationController +# rubocop:disable Gitlab/NamespacedClass -- Base controllers live in the global namespace +class BaseActionController < ActionController::Base + extend ContentSecurityPolicyPatch + + content_security_policy do |p| + next if p.directives.blank? + + if helpers.vite_enabled? + vite_host = ViteRuby.instance.config.host + vite_port = ViteRuby.instance.config.port + vite_origin = "#{vite_host}:#{vite_port}" + http_origin = "http://#{vite_origin}" + ws_origin = "ws://#{vite_origin}" + wss_origin = "wss://#{vite_origin}" + gitlab_ws_origin = Gitlab::Utils.append_path(Gitlab.config.gitlab.url, 'vite-dev/') + http_path = Gitlab::Utils.append_path(http_origin, 'vite-dev/') + + connect_sources = p.directives['connect-src'] + p.connect_src(*(Array.wrap(connect_sources) | [ws_origin, wss_origin, http_path])) + + worker_sources = p.directives['worker-src'] + p.worker_src(*(Array.wrap(worker_sources) | [gitlab_ws_origin, http_path])) + end + + next unless Gitlab::CurrentSettings.snowplow_enabled? && !Gitlab::CurrentSettings.snowplow_collector_hostname.blank? + + default_connect_src = p.directives['connect-src'] || p.directives['default-src'] + connect_src_values = Array.wrap(default_connect_src) | [Gitlab::CurrentSettings.snowplow_collector_hostname] + p.connect_src(*connect_src_values) + end +end +# rubocop:enable Gitlab/NamespacedClass +# rubocop:enable Rails/ApplicationController diff --git a/app/controllers/chaos_controller.rb b/app/controllers/chaos_controller.rb index 7328b793b09650010fc4e9b6c9489a122b611cff..b61a8c5ff1288895cfae850d7eb70ddae4149f52 100644 --- a/app/controllers/chaos_controller.rb +++ b/app/controllers/chaos_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# rubocop:disable Rails/ApplicationController -class ChaosController < ActionController::Base +class ChaosController < BaseActionController before_action :validate_chaos_secret, unless: :development_or_test? def leakmem @@ -95,4 +94,3 @@ def development_or_test? Rails.env.development? || Rails.env.test? end end -# rubocop:enable Rails/ApplicationController diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb index 1381999ab4c8836e1f79593be17b4411eb351aa3..2b2db2f950cfb9018a2ae2529100fbc72d4d9189 100644 --- a/app/controllers/health_controller.rb +++ b/app/controllers/health_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# rubocop:disable Rails/ApplicationController -class HealthController < ActionController::Base +class HealthController < BaseActionController protect_from_forgery with: :exception, prepend: true include RequiresAllowlistedMonitoringClient @@ -40,4 +39,3 @@ def render_checks(checks = []) render json: result.json, status: result.http_status end end -# rubocop:enable Rails/ApplicationController diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb index 9f41c092fa0a107cbec291b33721f03ee154f564..61851fd1c6096b8e3443abfe2b6ba3d80aae20af 100644 --- a/app/controllers/metrics_controller.rb +++ b/app/controllers/metrics_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# rubocop:disable Rails/ApplicationController -class MetricsController < ActionController::Base +class MetricsController < BaseActionController include RequiresAllowlistedMonitoringClient protect_from_forgery with: :exception, prepend: true @@ -36,4 +35,3 @@ def system_metrics ) end end -# rubocop:enable Rails/ApplicationController diff --git a/ee/app/controllers/oauth/geo_auth_controller.rb b/ee/app/controllers/oauth/geo_auth_controller.rb index 040884104b504c2f153e1d798ae5596cc0a7ebc5..11808da03950b9863878c8bc819982f14307d4b9 100644 --- a/ee/app/controllers/oauth/geo_auth_controller.rb +++ b/ee/app/controllers/oauth/geo_auth_controller.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# rubocop:disable Rails/ApplicationController -class Oauth::GeoAuthController < ActionController::Base +class Oauth::GeoAuthController < BaseActionController rescue_from Gitlab::Geo::OauthApplicationUndefinedError, with: :undefined_oauth_application rescue_from OAuth2::Error, with: :auth @@ -86,4 +85,3 @@ def invalid_access_token(token) render :error, layout: 'errors' end end -# rubocop:enable Rails/ApplicationController diff --git a/ee/spec/requests/groups/sso_controller_spec.rb b/ee/spec/requests/groups/sso_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..103af1cf0ffc72873a63909f44f96e26e1d172df --- /dev/null +++ b/ee/spec/requests/groups/sso_controller_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::SsoController, feature_category: :system_access do + let_it_be(:group) { create(:group) } + let_it_be(:saml_provider) { create(:saml_provider, group: group) } + + before do + stub_licensed_features(group_saml: true) + allow(Devise).to receive(:omniauth_providers).and_return(['group_saml']) + end + + it_behaves_like 'Base action controller' do + subject(:request) { get sso_group_saml_providers_path(group, token: group.saml_discovery_token) } + end +end diff --git a/ee/spec/requests/oauth/geo_auth_controller_spec.rb b/ee/spec/requests/oauth/geo_auth_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..beef439999b87d96a23ff4b419de3d77bbfd9986 --- /dev/null +++ b/ee/spec/requests/oauth/geo_auth_controller_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Oauth::GeoAuthController, type: :request, feature_category: :geo_replication do + it_behaves_like 'Base action controller' do + subject(:request) { get metrics_path } + end +end diff --git a/lib/gitlab/base_doorkeeper_controller.rb b/lib/gitlab/base_doorkeeper_controller.rb index c8520993b8e4e20acbc1f77b677f66718026f8bb..91994c2fa9535b933d2cd527d63b3b497a615bc0 100644 --- a/lib/gitlab/base_doorkeeper_controller.rb +++ b/lib/gitlab/base_doorkeeper_controller.rb @@ -3,8 +3,7 @@ # This is a base controller for doorkeeper. # It adds the `can?` helper used in the views. module Gitlab - # rubocop:disable Rails/ApplicationController - class BaseDoorkeeperController < ActionController::Base + class BaseDoorkeeperController < BaseActionController include Gitlab::Allowable include EnforcesTwoFactorAuthentication include SessionsHelper @@ -13,5 +12,4 @@ class BaseDoorkeeperController < ActionController::Base helper_method :can? end - # rubocop:enable Rails/ApplicationController end diff --git a/lib/gitlab/request_forgery_protection.rb b/lib/gitlab/request_forgery_protection.rb index d5e80053772b91a79be7a79930ef70f40339d13d..3a389d3363f8fde01771ac5ab5a1b1f01f0cc10a 100644 --- a/lib/gitlab/request_forgery_protection.rb +++ b/lib/gitlab/request_forgery_protection.rb @@ -6,8 +6,7 @@ module Gitlab module RequestForgeryProtection - # rubocop:disable Rails/ApplicationController - class Controller < ActionController::Base + class Controller < BaseActionController protect_from_forgery with: :exception, prepend: true def initialize @@ -40,6 +39,5 @@ def self.verified?(env) rescue ActionController::InvalidAuthenticityToken false end - # rubocop:enable Rails/ApplicationController end end diff --git a/spec/requests/acme_challenges_controller_spec.rb b/spec/requests/acme_challenges_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f37aefed488d4246a2237e0d50f77ad4cd32237d --- /dev/null +++ b/spec/requests/acme_challenges_controller_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AcmeChallengesController, type: :request, feature_category: :pages do + it_behaves_like 'Base action controller' do + subject(:request) { get acme_challenge_path } + end +end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..52fdf6bc69e09b36788576c0284dc1ed5c52d271 --- /dev/null +++ b/spec/requests/application_controller_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ApplicationController, type: :request, feature_category: :shared do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + it_behaves_like 'Base action controller' do + subject(:request) { get root_path } + end +end diff --git a/spec/requests/chaos_controller_spec.rb b/spec/requests/chaos_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d2ce618b041ad0a10a2e1a5d172183fc9924af9e --- /dev/null +++ b/spec/requests/chaos_controller_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ChaosController, type: :request, feature_category: :tooling do + it_behaves_like 'Base action controller' do + before do + # Stub leak_mem so we don't actually leak memory for the base action controller tests. + allow(Gitlab::Chaos).to receive(:leak_mem).with(100, 30.seconds) + end + + subject(:request) { get leakmem_chaos_path } + end +end diff --git a/spec/requests/content_security_policy_spec.rb b/spec/requests/content_security_policy_spec.rb deleted file mode 100644 index 3ce7e33d88a28ff05eb56bb81ba693af1c5fa0b3..0000000000000000000000000000000000000000 --- a/spec/requests/content_security_policy_spec.rb +++ /dev/null @@ -1,79 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -# The AnonymousController doesn't support setting the CSP -# This is why an arbitrary test request was chosen instead -# of testing in application_controller_spec. -RSpec.describe 'Content Security Policy', feature_category: :application_instrumentation do - let(:snowplow_host) { 'snowplow.example.com' } - let(:vite_origin) { "#{ViteRuby.instance.config.host}:#{ViteRuby.instance.config.port}" } - - shared_examples 'snowplow is not in the CSP' do - it 'does not add the snowplow collector hostname to the CSP' do - get explore_root_url - - expect(response).to have_gitlab_http_status(:ok) - expect(response.headers['Content-Security-Policy']).not_to include(snowplow_host) - end - end - - describe 'GET #explore' do - context 'snowplow is enabled' do - before do - stub_application_setting(snowplow_enabled: true, snowplow_collector_hostname: snowplow_host) - end - - it 'adds the snowplow collector hostname to the CSP' do - get explore_root_url - - expect(response).to have_gitlab_http_status(:ok) - expect(response.headers['Content-Security-Policy']).to include(snowplow_host) - end - end - - context 'snowplow is enabled but host is not configured' do - before do - stub_application_setting(snowplow_enabled: true) - end - - it_behaves_like 'snowplow is not in the CSP' - end - - context 'snowplow is disabled' do - before do - stub_application_setting(snowplow_enabled: false, snowplow_collector_hostname: snowplow_host) - end - - it_behaves_like 'snowplow is not in the CSP' - end - - context 'when vite enabled during development', - quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/424334' do - before do - stub_rails_env('development') - stub_feature_flags(vite: true) - - get explore_root_url - end - - it 'adds vite csp' do - expect(response).to have_gitlab_http_status(:ok) - expect(response.headers['Content-Security-Policy']).to include(vite_origin) - end - end - - context 'when vite disabled' do - before do - stub_feature_flags(vite: false) - - get explore_root_url - end - - it "doesn't add vite csp" do - expect(response).to have_gitlab_http_status(:ok) - expect(response.headers['Content-Security-Policy']).not_to include(vite_origin) - end - end - end -end diff --git a/spec/requests/health_controller_spec.rb b/spec/requests/health_controller_spec.rb index 639f6194af97c8297e0c29fead044f7722aedcf3..5fb2115aac34671aa38c5827c1bf347bb6e593b4 100644 --- a/spec/requests/health_controller_spec.rb +++ b/spec/requests/health_controller_spec.rb @@ -73,7 +73,9 @@ end describe 'GET /-/readiness' do - subject { get '/-/readiness', params: params, headers: headers } + subject(:request) { get readiness_path, params: params, headers: headers } + + it_behaves_like 'Base action controller' shared_context 'endpoint responding with readiness data' do context 'when requesting instance-checks' do @@ -219,7 +221,6 @@ stub_remote_addr(whitelisted_ip) end - it_behaves_like 'endpoint not querying database' it_behaves_like 'endpoint responding with readiness data' context 'when requesting all checks' do @@ -236,7 +237,6 @@ stub_remote_addr(not_whitelisted_ip) end - it_behaves_like 'endpoint not querying database' it_behaves_like 'endpoint not found' end @@ -273,7 +273,6 @@ stub_remote_addr(whitelisted_ip) end - it_behaves_like 'endpoint not querying database' it_behaves_like 'endpoint responding with liveness data' end @@ -282,7 +281,6 @@ stub_remote_addr(not_whitelisted_ip) end - it_behaves_like 'endpoint not querying database' it_behaves_like 'endpoint not found' context 'accessed with valid token' do diff --git a/spec/requests/metrics_controller_spec.rb b/spec/requests/metrics_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ce96906e02040b7ba5d5a3917f9d19eff25c06b6 --- /dev/null +++ b/spec/requests/metrics_controller_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MetricsController, type: :request, feature_category: :metrics do + it_behaves_like 'Base action controller' do + subject(:request) { get metrics_path } + end +end diff --git a/spec/requests/oauth/authorizations_controller_spec.rb b/spec/requests/oauth/authorizations_controller_spec.rb index 257f238d9ef784863379ae10df2507e182a43aba..7887bf52542e26f6bbff659519eaee5ed4c327d0 100644 --- a/spec/requests/oauth/authorizations_controller_spec.rb +++ b/spec/requests/oauth/authorizations_controller_spec.rb @@ -20,6 +20,10 @@ end describe 'GET #new' do + it_behaves_like 'Base action controller' do + subject(:request) { get oauth_authorization_path } + end + context 'when application redirect URI has a custom scheme' do context 'when CSP is disabled' do before do diff --git a/spec/requests/registrations_controller_spec.rb b/spec/requests/registrations_controller_spec.rb index 8b857046a4db68bb2560edaf7ae7e51bc77dd979..71f2f347f0d3f105d9f7046e836e8841dd9a2f01 100644 --- a/spec/requests/registrations_controller_spec.rb +++ b/spec/requests/registrations_controller_spec.rb @@ -6,7 +6,9 @@ describe 'POST #create' do let_it_be(:user_attrs) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) } - subject(:create_user) { post user_registration_path, params: { user: user_attrs } } + subject(:request) { post user_registration_path, params: { user: user_attrs } } + + it_behaves_like 'Base action controller' context 'when email confirmation is required' do before do @@ -15,7 +17,7 @@ end it 'redirects to the `users_almost_there_path`', unless: Gitlab.ee? do - create_user + request expect(response).to redirect_to(users_almost_there_path(email: user_attrs[:email])) end diff --git a/spec/requests/sessions_spec.rb b/spec/requests/sessions_spec.rb index 12939acdd914977f2e9252b69a569379c3bdbd3e..337f358d394217f74d293651e253616746a09b5f 100644 --- a/spec/requests/sessions_spec.rb +++ b/spec/requests/sessions_spec.rb @@ -7,6 +7,10 @@ let(:user) { create(:user) } + it_behaves_like 'Base action controller' do + subject(:request) { get new_user_session_path } + end + context 'for authentication', :allow_forgery_protection do it 'logout does not require a csrf token' do login_as(user) diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index d6fa1d60dad2728e7624cef958a5e21380d7b138..fe84a80dae693fcecdbff452e71c8c7f198a03bd 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -8148,7 +8148,6 @@ - './spec/requests/api/users_spec.rb' - './spec/requests/api/wikis_spec.rb' - './spec/requests/concerns/planning_hierarchy_spec.rb' -- './spec/requests/content_security_policy_spec.rb' - './spec/requests/dashboard_controller_spec.rb' - './spec/requests/dashboard/projects_controller_spec.rb' - './spec/requests/git_http_spec.rb' diff --git a/spec/support/shared_examples/controllers/base_action_controller_shared_examples.rb b/spec/support/shared_examples/controllers/base_action_controller_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..2eab533ef7fd80b3b855e0e9a166d4587cbcf0c3 --- /dev/null +++ b/spec/support/shared_examples/controllers/base_action_controller_shared_examples.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +# Requires `request` subject to be defined +# +# subject(:request) { get root_path } +RSpec.shared_examples 'Base action controller' do + describe 'security headers' do + describe 'Cross-Security-Policy' do + context 'when configuring snowplow' do + let(:snowplow_host) { 'snowplow.example.com' } + + shared_examples 'snowplow is not in the CSP' do + it 'does not add the snowplow collector hostname to the CSP' do + request + + expect(response.headers['Content-Security-Policy']).not_to include(snowplow_host) + end + end + + context 'when snowplow is enabled' do + before do + stub_application_setting(snowplow_enabled: true, snowplow_collector_hostname: snowplow_host) + end + + it 'adds snowplow to the csp' do + request + + expect(response.headers['Content-Security-Policy']).to include(snowplow_host) + end + end + + context 'when snowplow is enabled but host is not configured' do + before do + stub_application_setting(snowplow_enabled: true, snowplow_collector_hostname: nil) + end + + it_behaves_like 'snowplow is not in the CSP' + end + + context 'when snowplow is disabled' do + before do + stub_application_setting(snowplow_enabled: false, snowplow_collector_hostname: snowplow_host) + end + + it_behaves_like 'snowplow is not in the CSP' + end + end + + context 'when configuring vite' do + let(:vite_origin) { "#{ViteRuby.instance.config.host}:#{ViteRuby.instance.config.port}" } + + context 'when vite enabled during development', + skip: 'https://gitlab.com/gitlab-org/gitlab/-/issues/424334' do + before do + stub_rails_env('development') + stub_feature_flags(vite: true) + end + + it 'adds vite csp' do + request + + expect(response.headers['Content-Security-Policy']).to include(vite_origin) + end + end + + context 'when vite disabled' do + before do + stub_feature_flags(vite: false) + end + + it "doesn't add vite csp" do + request + + expect(response.headers['Content-Security-Policy']).not_to include(vite_origin) + end + end + end + end + end +end