Skip to content

Commit

Permalink
SECURITY: Don't reuse CSP nonce between anonymous requests
Browse files Browse the repository at this point in the history
  • Loading branch information
OsamaSayegh authored and davidtaylorhq committed Jul 28, 2023
1 parent 672f3e7 commit 0976c8f
Show file tree
Hide file tree
Showing 15 changed files with 105 additions and 22 deletions.
6 changes: 4 additions & 2 deletions app/helpers/application_helper.rb
Expand Up @@ -64,8 +64,10 @@ def google_tag_manager_json
google_universal_analytics_json
end

def self.google_tag_manager_nonce(env)
env[:discourse_content_security_policy_nonce] ||= SecureRandom.hex
def google_tag_manager_nonce_placeholder
placeholder = "[[csp_nonce_placeholder_#{SecureRandom.hex}]]"
response.headers["Discourse-GTM-Nonce-Placeholder"] = placeholder
placeholder
end

def shared_session_key
Expand Down
2 changes: 1 addition & 1 deletion app/views/common/_google_tag_manager_head.html.erb
@@ -1,6 +1,6 @@
<meta id="data-google-tag-manager"
data-data-layer="<%= google_tag_manager_json %>"
data-nonce="<%= ApplicationHelper.google_tag_manager_nonce(request.env) %>"
data-nonce="<%= google_tag_manager_nonce_placeholder %>"
data-container-id="<%= SiteSetting.gtm_container_id %>" />

<%= preload_script 'google-tag-manager' %>
3 changes: 3 additions & 0 deletions config/application.rb
Expand Up @@ -167,6 +167,9 @@ def config.database_configuration
config.middleware.swap ActionDispatch::ContentSecurityPolicy::Middleware,
ContentSecurityPolicy::Middleware

require "middleware/gtm_script_nonce_injector"
config.middleware.insert_after(ActionDispatch::Flash, Middleware::GtmScriptNonceInjector)

require "middleware/discourse_public_exceptions"
config.exceptions_app = Middleware::DiscoursePublicExceptions.new(Rails.public_path)

Expand Down
5 changes: 3 additions & 2 deletions config/initializers/099-anon-cache.rb
Expand Up @@ -6,10 +6,11 @@
if Rails.configuration.respond_to?(:enable_anon_caching)
Rails.configuration.enable_anon_caching
else
Rails.env.production?
Rails.env.production? || Rails.env.test?
end

if !ENV["DISCOURSE_DISABLE_ANON_CACHE"] && enabled
# in an ideal world this is position 0, but mobile detection uses ... session and request and params
Rails.configuration.middleware.insert_after ActionDispatch::Flash, Middleware::AnonymousCache
Rails.configuration.middleware.insert_after Middleware::GtmScriptNonceInjector,
Middleware::AnonymousCache
end
8 changes: 4 additions & 4 deletions lib/content_security_policy.rb
Expand Up @@ -4,13 +4,13 @@

class ContentSecurityPolicy
class << self
def policy(theme_id = nil, env: {}, base_url: Discourse.base_url, path_info: "/")
new.build(theme_id, env: env, base_url: base_url, path_info: path_info)
def policy(theme_id = nil, base_url: Discourse.base_url, path_info: "/")
new.build(theme_id, base_url: base_url, path_info: path_info)
end
end

def build(theme_id, env: {}, base_url:, path_info: "/")
builder = Builder.new(base_url: base_url, env: env)
def build(theme_id, base_url:, path_info: "/")
builder = Builder.new(base_url: base_url)

Extension.theme_extensions(theme_id).each { |extension| builder << extension }
Extension.plugin_extensions.each { |extension| builder << extension }
Expand Down
4 changes: 2 additions & 2 deletions lib/content_security_policy/builder.rb
Expand Up @@ -25,8 +25,8 @@ class Builder
style_src
].freeze

def initialize(base_url:, env: {})
@directives = Default.new(base_url: base_url, env: env).directives
def initialize(base_url:)
@directives = Default.new(base_url: base_url).directives
@base_url = base_url
end

Expand Down
4 changes: 1 addition & 3 deletions lib/content_security_policy/default.rb
Expand Up @@ -5,9 +5,8 @@ class ContentSecurityPolicy
class Default
attr_reader :directives

def initialize(base_url:, env: {})
def initialize(base_url:)
@base_url = base_url
@env = env
@directives =
{}.tap do |directives|
directives[:upgrade_insecure_requests] = [] if SiteSetting.force_https
Expand Down Expand Up @@ -86,7 +85,6 @@ def script_src
end
if SiteSetting.gtm_container_id.present?
sources << "https://www.googletagmanager.com/gtm.js"
sources << "'nonce-#{ApplicationHelper.google_tag_manager_nonce(@env)}'"
end

sources << "'#{SplashScreenHelper.fingerprint}'" if SiteSetting.splash_screen
Expand Down
2 changes: 0 additions & 2 deletions lib/content_security_policy/middleware.rb
Expand Up @@ -21,13 +21,11 @@ def call(env)

headers["Content-Security-Policy"] = policy(
theme_id,
env: env,
base_url: base_url,
path_info: env["PATH_INFO"],
) if SiteSetting.content_security_policy
headers["Content-Security-Policy-Report-Only"] = policy(
theme_id,
env: env,
base_url: base_url,
path_info: env["PATH_INFO"],
) if SiteSetting.content_security_policy_report_only
Expand Down
22 changes: 21 additions & 1 deletion lib/middleware/anonymous_cache.rb
Expand Up @@ -43,6 +43,21 @@ def self.anon_cache(env, duration)
env["ANON_CACHE_DURATION"] = duration
end

def self.clear_all_cache!
if Rails.env.production?
raise "for perf reasons, clear_all_cache! cannot be used in production."
end
Discourse.redis.keys("ANON_CACHE_*").each { |k| Discourse.redis.del(k) }
end

def self.disable_anon_cache
@@disabled = true
end

def self.enable_anon_cache
@@disabled = false
end

# This gives us an API to insert anonymous cache segments
class Helper
RACK_SESSION = "rack.session"
Expand Down Expand Up @@ -232,7 +247,10 @@ def should_force_anonymous?
end

def cacheable?
!!(!has_auth_cookie? && get? && no_cache_bypass)
!!(
GlobalSetting.anon_cache_store_threshold > 0 && !has_auth_cookie? && get? &&
no_cache_bypass
)
end

def compress(val)
Expand Down Expand Up @@ -326,6 +344,8 @@ def initialize(app, settings = {})
PAYLOAD_INVALID_REQUEST_METHODS = %w[GET HEAD]

def call(env)
return @app.call(env) if defined?(@@disabled) && @@disabled

if PAYLOAD_INVALID_REQUEST_METHODS.include?(env[Rack::REQUEST_METHOD]) &&
env[Rack::RACK_INPUT].size > 0
return 413, { "Cache-Control" => "private, max-age=0, must-revalidate" }, []
Expand Down
26 changes: 26 additions & 0 deletions lib/middleware/gtm_script_nonce_injector.rb
@@ -0,0 +1,26 @@
# frozen_string_literal: true

module Middleware
class GtmScriptNonceInjector
def initialize(app, settings = {})
@app = app
end

def call(env)
status, headers, response = @app.call(env)

if nonce_placeholder = headers.delete("Discourse-GTM-Nonce-Placeholder")
nonce = SecureRandom.hex
parts = []
response.each { |part| parts << part.to_s.sub(nonce_placeholder, nonce) }
%w[Content-Security-Policy Content-Security-Policy-Report-Only].each do |name|
next if headers[name].blank?
headers[name] = headers[name].sub("script-src ", "script-src 'nonce-#{nonce}' ")
end
[status, headers, parts]
else
[status, headers, response]
end
end
end
end
4 changes: 3 additions & 1 deletion spec/lib/content_security_policy_spec.rb
Expand Up @@ -106,7 +106,9 @@

script_srcs = parse(policy)["script-src"]
expect(script_srcs).to include("https://www.googletagmanager.com/gtm.js")
expect(script_srcs.to_s).to include("nonce-")
# nonce is added by the GtmScriptNonceInjector middleware to prevent the
# nonce from getting cached by AnonymousCache
expect(script_srcs.to_s).not_to include("nonce-")
end

it "allowlists CDN assets when integrated" do
Expand Down
2 changes: 2 additions & 0 deletions spec/lib/middleware/anonymous_cache_spec.rb
Expand Up @@ -3,6 +3,8 @@
RSpec.describe Middleware::AnonymousCache do
let(:middleware) { Middleware::AnonymousCache.new(lambda { |_| [200, {}, []] }) }

before { Middleware::AnonymousCache.enable_anon_cache }

def env(opts = {})
create_request_env(path: opts.delete(:path) || "http://test.com/path?bla=1").merge(opts)
end
Expand Down
2 changes: 2 additions & 0 deletions spec/lib/middleware/request_tracker_spec.rb
Expand Up @@ -677,6 +677,8 @@ def app(result, sql_calls: 0, redis_calls: 0)
after { Middleware::RequestTracker.unregister_detailed_request_logger(logger) }

it "can report data from anon cache" do
Middleware::AnonymousCache.enable_anon_cache

cache = Middleware::AnonymousCache.new(app([200, {}, ["i am a thing"]]))
tracker = Middleware::RequestTracker.new(cache)

Expand Down
2 changes: 2 additions & 0 deletions spec/rails_helper.rb
Expand Up @@ -149,6 +149,8 @@ def self.test_setup(x = nil)
BadgeGranter.disable_queue

OmniAuth.config.test_mode = false

Middleware::AnonymousCache.disable_anon_cache
end
end

Expand Down
35 changes: 31 additions & 4 deletions spec/requests/application_controller_spec.rb
Expand Up @@ -637,36 +637,63 @@

it "when GTM is enabled it adds the same nonce to the policy and the GTM tag" do
SiteSetting.content_security_policy = true
SiteSetting.content_security_policy_report_only = true
SiteSetting.gtm_container_id = "GTM-ABCDEF"

get "/latest"

expect(response.headers).to include("Content-Security-Policy")
script_src = parse(response.headers["Content-Security-Policy"])["script-src"]
report_only_script_src =
parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"]

nonce = extract_nonce_from_script_src(script_src)
report_only_nonce = extract_nonce_from_script_src(report_only_script_src)

expect(nonce).to eq(report_only_nonce)

gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first
expect(gtm_meta_tag["data-nonce"]).to eq(nonce)
end

it "doesn't reuse CSP nonces between requests" do
it "doesn't reuse nonces between requests" do
global_setting :anon_cache_store_threshold, 1
Middleware::AnonymousCache.enable_anon_cache
Middleware::AnonymousCache.clear_all_cache!

SiteSetting.content_security_policy = true
SiteSetting.content_security_policy_report_only = true
SiteSetting.gtm_container_id = "GTM-ABCDEF"

get "/latest"

expect(response.headers).to include("Content-Security-Policy")
expect(response.headers["X-Discourse-Cached"]).to eq("store")
expect(response.headers).not_to include("Discourse-GTM-Nonce-Placeholder")

script_src = parse(response.headers["Content-Security-Policy"])["script-src"]
report_only_script_src =
parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"]

first_nonce = extract_nonce_from_script_src(script_src)
first_report_only_nonce = extract_nonce_from_script_src(report_only_script_src)

expect(first_nonce).to eq(first_report_only_nonce)

gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first
expect(gtm_meta_tag["data-nonce"]).to eq(first_nonce)

get "/latest"

expect(response.headers).to include("Content-Security-Policy")
expect(response.headers["X-Discourse-Cached"]).to eq("true")
expect(response.headers).not_to include("Discourse-GTM-Nonce-Placeholder")

script_src = parse(response.headers["Content-Security-Policy"])["script-src"]
report_only_script_src =
parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"]

second_nonce = extract_nonce_from_script_src(script_src)
second_report_only_nonce = extract_nonce_from_script_src(report_only_script_src)

expect(second_nonce).to eq(second_report_only_nonce)

expect(first_nonce).not_to eq(second_nonce)
gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first
Expand Down

0 comments on commit 0976c8f

Please sign in to comment.