FIX: Allow CSP to work correctly for non-default hostnames/schemes (#9180)

FIX: Allow CSP to work correctly for non-default hostnames/schemes (#9180)

  • Define the CSP based on the requested domain / scheme (respecting force_https)
  • Update EnforceHostname middleware to allow secondary domains, add specs
  • Add URL scheme to anon cache key so that CSP headers are cached correctly
diff --git a/lib/content_security_policy.rb b/lib/content_security_policy.rb
index 3a6227b..76d2246 100644
--- a/lib/content_security_policy.rb
+++ b/lib/content_security_policy.rb
@@ -4,18 +4,13 @@ require 'content_security_policy/extension'
 
 class ContentSecurityPolicy
   class << self
-    def policy(theme_ids = [], path_info: "/")
-      new.build(theme_ids, path_info: path_info)
+    def policy(theme_ids = [], base_url: Discourse.base_url, path_info: "/")
+      new.build(theme_ids, base_url: base_url, path_info: path_info)
     end
-
-    def base_url
-      @base_url || Discourse.base_url
-    end
-    attr_writer :base_url
   end
 
-  def build(theme_ids, path_info: "/")
-    builder = Builder.new
+  def build(theme_ids, base_url:, path_info: "/")
+    builder = Builder.new(base_url: base_url)
 
     Extension.theme_extensions(theme_ids).each { |extension| builder << extension }
     Extension.plugin_extensions.each { |extension| builder << extension }
diff --git a/lib/content_security_policy/builder.rb b/lib/content_security_policy/builder.rb
index f7cf1b6..e59e8e2 100644
--- a/lib/content_security_policy/builder.rb
+++ b/lib/content_security_policy/builder.rb
@@ -25,8 +25,8 @@ class ContentSecurityPolicy
       style_src
     ].freeze
 
-    def initialize
-      @directives = Default.new.directives
+    def initialize(base_url:)
+      @directives = Default.new(base_url: base_url).directives
     end
 
     def <<(extension)
diff --git a/lib/content_security_policy/default.rb b/lib/content_security_policy/default.rb
index d1daff3..d2f92f2 100644
--- a/lib/content_security_policy/default.rb
+++ b/lib/content_security_policy/default.rb
@@ -5,7 +5,8 @@ class ContentSecurityPolicy
   class Default
     attr_reader :directives
 
-    def initialize
+    def initialize(base_url:)
+      @base_url = base_url
       @directives = {}.tap do |directives|
         directives[:base_uri] = [:none]
         directives[:object_src] = [:none]
@@ -17,7 +18,9 @@ class ContentSecurityPolicy
 
     private
 
-    delegate :base_url, to: :ContentSecurityPolicy
+    def base_url
+      @base_url
+    end
 
     SCRIPT_ASSET_DIRECTORIES = [
       # [dir, can_use_s3_cdn, can_use_cdn]
diff --git a/lib/content_security_policy/middleware.rb b/lib/content_security_policy/middleware.rb
index 3e10db2..d9b8ece 100644
--- a/lib/content_security_policy/middleware.rb
+++ b/lib/content_security_policy/middleware.rb
@@ -12,12 +12,15 @@ class ContentSecurityPolicy
       _, headers, _ = response = @app.call(env)
 
       return response unless html_response?(headers)
-      ContentSecurityPolicy.base_url = request.host_with_port if !Rails.env.production?
+
+      # The EnforceHostname middleware ensures request.host_with_port can be trusted
+      protocol = (SiteSetting.force_https || request.ssl?) ? "https://" : "http://"
+      base_url = protocol + request.host_with_port + Discourse.base_uri
 
       theme_ids = env[:resolved_theme_ids]
 
-      headers['Content-Security-Policy'] = policy(theme_ids, path_info: env["PATH_INFO"]) if SiteSetting.content_security_policy
-      headers['Content-Security-Policy-Report-Only'] = policy(theme_ids, path_info: env["PATH_INFO"]) if SiteSetting.content_security_policy_report_only
+      headers['Content-Security-Policy'] = policy(theme_ids, base_url: base_url, path_info: env["PATH_INFO"]) if SiteSetting.content_security_policy
+      headers['Content-Security-Policy-Report-Only'] = policy(theme_ids, base_url: base_url, path_info: env["PATH_INFO"]) if SiteSetting.content_security_policy_report_only
 
       response
     end
diff --git a/lib/middleware/anonymous_cache.rb b/lib/middleware/anonymous_cache.rb
index aa7dd7d..8f1e756 100644
--- a/lib/middleware/anonymous_cache.rb
+++ b/lib/middleware/anonymous_cache.rb
@@ -107,7 +107,7 @@ module Middleware
       def cache_key
         return @cache_key if defined?(@cache_key)
 
-        @cache_key = +"ANON_CACHE_#{@env["HTTP_ACCEPT"]}_#{@env["HTTP_HOST"]}#{@env["REQUEST_URI"]}"
+        @cache_key = +"ANON_CACHE_#{@env["HTTP_ACCEPT"]}_#{@env[Rack::RACK_URL_SCHEME]}_#{@env["HTTP_HOST"]}#{@env["REQUEST_URI"]}"
         @cache_key << AnonymousCache.build_cache_key(self)
         @cache_key
       end
diff --git a/lib/middleware/enforce_hostname.rb b/lib/middleware/enforce_hostname.rb
index 130cabb..00e8870 100644
--- a/lib/middleware/enforce_hostname.rb
+++ b/lib/middleware/enforce_hostname.rb
@@ -13,7 +13,12 @@ module Middleware
       # all Rails helpers are guarenteed to use it unconditionally and
       # never generate incorrect links
       env[Rack::Request::HTTP_X_FORWARDED_HOST] = nil
-      env[Rack::HTTP_HOST] = Discourse.current_hostname
+
+      allowed_hostnames = RailsMultisite::ConnectionManagement.current_db_hostnames
+      requested_hostname = env[Rack::HTTP_HOST]
+
+      env[Rack::HTTP_HOST] = allowed_hostnames.find { |h| h == requested_hostname } || Discourse.current_hostname
+
       @app.call(env)
     end
   end
diff --git a/spec/components/middleware/enforce_hostname_spec.rb b/spec/components/middleware/enforce_hostname_spec.rb
new file mode 100644
index 0000000..1a065eb
--- /dev/null
+++ b/spec/components/middleware/enforce_hostname_spec.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Middleware::EnforceHostname do
+
+  before do
+    RailsMultisite::ConnectionManagement.stubs(:current_db_hostnames).returns(['primary.example.com', 'secondary.example.com'])
+    RailsMultisite::ConnectionManagement.stubs(:current_hostname).returns('primary.example.com')
+  end
+
+  def check_returned_host(input_host)
+    resolved_host = nil
+
+    app = described_class.new(
+      lambda do |env|
+        resolved_host = env["HTTP_HOST"]
+        [200, {}, ["ok"]]
+      end
+    )
+
+    app.call({ "HTTP_HOST" => input_host })
+
+    resolved_host
+  end
+
+  it "works for the primary domain" do
+    expect(check_returned_host("primary.example.com")).to eq("primary.example.com")
+  end
+
+  it "works for the secondary domain" do
+    expect(check_returned_host("secondary.example.com")).to eq("secondary.example.com")
+  end
+
+  it "returns primary domain otherwise" do
+    expect(check_returned_host("other.example.com")).to eq("primary.example.com")
+    expect(check_returned_host(nil)).to eq("primary.example.com")
+  end
+end
diff --git a/spec/integration/content_security_policy_spec.rb b/spec/integration/content_security_policy_spec.rb
new file mode 100644
index 0000000..095e3c2
--- /dev/null
+++ b/spec/integration/content_security_policy_spec.rb
@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'content security policy integration' do
+
+  it "adds the csp headers correctly" do
+    SiteSetting.content_security_policy = false
+    get "/"
+    expect(response.headers["Content-Security-Policy"]).to eq(nil)
+
+    SiteSetting.content_security_policy = true
+    get "/"
+    expect(response.headers["Content-Security-Policy"]).to be_present
+  end
+
+  context "with different hostnames" do
+    before do
+      SiteSetting.content_security_policy = true
+      RailsMultisite::ConnectionManagement.stubs(:current_db_hostnames).returns(['primary.example.com', 'secondary.example.com'])
+      RailsMultisite::ConnectionManagement.stubs(:current_hostname).returns('primary.example.com')
+    end
+
+    it "works with the primary domain" do
+      host! "primary.example.com"
+      get "/"
+      expect(response.headers["Content-Security-Policy"]).to include("http://primary.example.com")
+    end
+
+    it "works with the secondary domain" do
+      host! "secondary.example.com"
+      get "/"
+      expect(response.headers["Content-Security-Policy"]).to include("http://secondary.example.com")
+    end
+
+    it "uses the primary domain for unknown hosts" do
+      host! "unknown.example.com"
+      get "/"
+      expect(response.headers["Content-Security-Policy"]).to include("http://primary.example.com")

[... diff too long, it was truncated ...]

GitHub sha: 19814c5e

This commit appears in #9180 which was approved by pmusaraj and ZogStriP. It was merged by davidtaylorhq.

This commit has been mentioned on Discourse Meta. There might be relevant details there: