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
This commit is contained in:
David Taylor 2020-03-19 19:54:42 +00:00 committed by GitHub
parent e9a3639b10
commit 19814c5e81
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 122 additions and 21 deletions

View File

@ -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 }

View File

@ -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)

View File

@ -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]

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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")
end
end
context "with different protocols" do
it "forces https when the site setting is enabled" do
SiteSetting.force_https = true
get "/"
expect(response.headers["Content-Security-Policy"]).to include("https://test.localhost")
end
it "uses https when the site setting is disabled, but request is ssl" do
SiteSetting.force_https = false
https!
get "/"
expect(response.headers["Content-Security-Policy"]).to include("https://test.localhost")
end
end
end

View File

@ -2,7 +2,7 @@
require 'rails_helper'
describe ContentSecurityPolicy::Builder do
let(:builder) { described_class.new }
let(:builder) { described_class.new(base_url: Discourse.base_url) }
describe '#<<' do
it 'normalizes directive name' do

View File

@ -2,8 +2,6 @@
require 'rails_helper'
describe ContentSecurityPolicy do
before { ContentSecurityPolicy.base_url = nil }
after do
DiscoursePluginRegistry.reset!
end