Merge branch 'remove-self-frame-src' into 'master'

Stop using 'self' in the CSP's frame-src directive

See merge request gitlab-org/gitlab!71345

lib/gitlab/content_security_policy/config_loader.rb

@@ -19,7 +19,7 @@ module Gitlab
           'font_src' => "'self'",
           'form_action' => "'self' https: http:",
           'frame_ancestors' => "'self'",
-          'frame_src' => "'self' https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com",
+          'frame_src' => "#{framed_gitlab_paths.join(' ')} https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com",
           'img_src' => "'self' data: blob: http: https:",
           'manifest_src' => "'self'",
           'media_src' => "'self'",
@@ -30,11 +30,6 @@ module Gitlab
           'report_uri' => nil
         }
 
-        # frame-src was deprecated in CSP level 2 in favor of child-src
-        # CSP level 3 "undeprecated" frame-src and browsers fall back on child-src if it's missing
-        # However Safari seems to read child-src first so we'll just keep both equal
-        directives['child_src'] = directives['frame_src']
-
         # connect_src with 'self' includes https/wss variations of the origin,
         # however, safari hasn't covered this yet and we need to explicitly add
         # support for websocket origins until Safari catches up with the specs
@@ -44,6 +39,11 @@ module Gitlab
         allow_customersdot(directives) if Rails.env.development? && ENV['CUSTOMER_PORTAL_URL'].present?
         allow_sentry(directives) if Gitlab.config.sentry&.enabled && Gitlab.config.sentry&.clientside_dsn
 
+        # frame-src was deprecated in CSP level 2 in favor of child-src
+        # CSP level 3 "undeprecated" frame-src and browsers fall back on child-src if it's missing
+        # However Safari seems to read child-src first so we'll just keep both equal
+        directives['child_src'] = directives['frame_src']
+
         directives
       end
 
@@ -119,6 +119,14 @@ module Gitlab
 
         append_to_directive(directives, 'connect_src', sentry_uri.to_s)
       end
+
+      # Using 'self' in the CSP introduces several CSP bypass opportunities
+      # for this reason we list the URLs where GitLab frames itself instead
+      def self.framed_gitlab_paths
+        ['/admin/sidekiq', '/-/speedscope/index.html'].map do |path|
+          Gitlab::Utils.append_path(Gitlab.config.gitlab.url, path)
+        end
+      end
     end
   end
 end

spec/lib/gitlab/content_security_policy/config_loader_spec.rb

@@ -109,7 +109,7 @@ RSpec.describe Gitlab::ContentSecurityPolicy::ConfigLoader do
         end
 
         it 'does not add CUSTOMER_PORTAL_URL to CSP' do
-          expect(directives['frame_src']).to eq("'self' https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com")
+          expect(directives['frame_src']).to eq("http://localhost/admin/sidekiq http://localhost/-/speedscope/index.html https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com")
         end
       end
 
@@ -119,12 +119,36 @@ RSpec.describe Gitlab::ContentSecurityPolicy::ConfigLoader do
         end
 
         it 'adds CUSTOMER_PORTAL_URL to CSP' do
-          expect(directives['frame_src']).to eq("'self' https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com https://customers.example.com")
+          expect(directives['frame_src']).to eq("http://localhost/admin/sidekiq http://localhost/-/speedscope/index.html https://www.google.com/recaptcha/ https://www.recaptcha.net/ https://content.googleapis.com https://content-compute.googleapis.com https://content-cloudbilling.googleapis.com https://content-cloudresourcemanager.googleapis.com https://customers.example.com")
         end
       end
     end
   end
 
+  describe '#framed_gitlab_paths' do
+    context 'generates URLs to be added to child-src' do
+      it 'with insecure domain' do
+        stub_config_setting(url: 'http://example.com')
+        expect(described_class.framed_gitlab_paths).to eq(%w[http://example.com/admin/sidekiq http://example.com/-/speedscope/index.html])
+      end
+
+      it 'with secure domain' do
+        stub_config_setting(url: 'https://example.com')
+        expect(described_class.framed_gitlab_paths).to eq(%w[https://example.com/admin/sidekiq https://example.com/-/speedscope/index.html])
+      end
+
+      it 'with custom port' do
+        stub_config_setting(url: 'http://example.com:1234')
+        expect(described_class.framed_gitlab_paths).to eq(%w[http://example.com:1234/admin/sidekiq http://example.com:1234/-/speedscope/index.html])
+      end
+
+      it 'with custom port and secure domain' do
+        stub_config_setting(url: 'https://example.com:1234')
+        expect(described_class.framed_gitlab_paths).to eq(%w[https://example.com:1234/admin/sidekiq https://example.com:1234/-/speedscope/index.html])
+      end
+    end
+  end
+
   describe '#load' do
     subject { described_class.new(csp_config[:directives]) }