Skip to content

Commit

Permalink
FEATURE: Add experimental option for strict-dynamic CSP (#25664)
Browse files Browse the repository at this point in the history
The strict-dynamic CSP directive is supported in all our target browsers, and makes for a much simpler configuration. Instead of allowlisting paths, we use a per-request nonce to authorize `<script>` tags, and then those scripts are allowed to load additional scripts (or add additional inline scripts) without restriction.

This becomes especially useful when admins want to add external scripts like Google Tag Manager, or advertising scripts, which then go on to load a ton of other scripts.

All script tags introduced via themes will automatically have the nonce attribute applied, so it should be zero-effort for theme developers. Plugins *may* need some changes if they are inserting their own script tags.

This commit introduces a strict-dynamic-based CSP behind an experimental `content_security_policy_strict_dynamic` site setting.
  • Loading branch information
davidtaylorhq committed Feb 16, 2024
1 parent 9329a53 commit b1f74ab
Show file tree
Hide file tree
Showing 25 changed files with 196 additions and 158 deletions.
19 changes: 17 additions & 2 deletions app/assets/javascripts/bootstrap-json/index.js
Expand Up @@ -80,8 +80,23 @@ function updateScriptReferences({
entrypointName === "discourse" &&
element.tagName.toLowerCase() === "script"
) {
let nonce = "";
for (const [attr, value] of element.attributes) {
if (attr === "nonce") {
nonce = value;
break;
}
}

if (!nonce) {
// eslint-disable-next-line no-console
console.error(
"Expected to find a nonce= attribute on the main discourse script tag, but none was found. ember-cli-live-reload may not work correctly."
);
}

newElements.unshift(
`<script async src="${baseURL}ember-cli-live-reload.js"></script>`
`<script async src="${baseURL}ember-cli-live-reload.js" nonce="${nonce}"></script>`
);
}

Expand Down Expand Up @@ -159,7 +174,7 @@ async function handleRequest(proxy, baseURL, req, res, outputPath) {
}

const csp = response.headers.get("content-security-policy");
if (csp) {
if (csp && !csp.includes("'strict-dynamic'")) {
const emberCliAdditions = [
`http://${originalHost}${baseURL}assets/`,
`http://${originalHost}${baseURL}ember-cli-live-reload.js`,
Expand Down
21 changes: 14 additions & 7 deletions app/helpers/application_helper.rb
Expand Up @@ -65,10 +65,13 @@ def google_tag_manager_json
google_universal_analytics_json
end

def google_tag_manager_nonce_placeholder
placeholder = "[[csp_nonce_placeholder_#{SecureRandom.hex}]]"
response.headers["Discourse-GTM-Nonce-Placeholder"] = placeholder
placeholder
def csp_nonce_placeholder
@csp_nonce_placeholder ||=
begin
placeholder = "[[csp_nonce_placeholder_#{SecureRandom.hex}]]"
response.headers["Discourse-CSP-Nonce-Placeholder"] = placeholder
placeholder
end
end

def shared_session_key
Expand Down Expand Up @@ -150,16 +153,17 @@ def preload_script(script)

def preload_script_url(url, entrypoint: nil)
entrypoint_attribute = entrypoint ? "data-discourse-entrypoint=\"#{entrypoint}\"" : ""
nonce_attribute = "nonce=\"#{csp_nonce_placeholder}\""

add_resource_preload_list(url, "script")
if GlobalSetting.preload_link_header
<<~HTML.html_safe
<script defer src="#{url}" #{entrypoint_attribute}></script>
<script defer src="#{url}" #{entrypoint_attribute} #{nonce_attribute}></script>
HTML
else
<<~HTML.html_safe
<link rel="preload" href="#{url}" as="script" #{entrypoint_attribute}>
<script defer src="#{url}" #{entrypoint_attribute}></script>
<link rel="preload" href="#{url}" as="script" #{entrypoint_attribute} #{nonce_attribute}>
<script defer src="#{url}" #{entrypoint_attribute} #{nonce_attribute}></script>
HTML
end
end
Expand Down Expand Up @@ -586,6 +590,7 @@ def theme_lookup(name)
mobile_view? ? :mobile : :desktop,
name,
skip_transformation: request.env[:skip_theme_ids_transformation].present?,
csp_nonce: csp_nonce_placeholder,
)
end

Expand All @@ -595,6 +600,7 @@ def theme_translations_lookup
:translations,
I18n.locale,
skip_transformation: request.env[:skip_theme_ids_transformation].present?,
csp_nonce: csp_nonce_placeholder,
)
end

Expand All @@ -604,6 +610,7 @@ def theme_js_lookup
:extra_js,
nil,
skip_transformation: request.env[:skip_theme_ids_transformation].present?,
csp_nonce: csp_nonce_placeholder,
)
end

Expand Down
26 changes: 0 additions & 26 deletions app/helpers/defer_script_helper.rb

This file was deleted.

26 changes: 4 additions & 22 deletions app/helpers/splash_screen_helper.rb
@@ -1,18 +1,12 @@
# frozen_string_literal: true

module SplashScreenHelper
def self.inline_splash_screen_script
<<~HTML.html_safe
<script>#{raw_js}</script>
HTML
end

def self.fingerprint
def self.raw_js
if Rails.env.development?
calculate_fingerprint
load_js
else
@fingerprint ||= calculate_fingerprint
end
@loaded_js ||= load_js
end.html_safe
end

private
Expand All @@ -26,16 +20,4 @@ def self.load_js
Rails.logger.error("Unable to load splash screen JS") if Rails.env.production?
"console.log('Unable to load splash screen JS')"
end

def self.raw_js
if Rails.env.development?
load_js
else
@loaded_js ||= load_js
end
end

def self.calculate_fingerprint
"sha256-#{Digest::SHA256.base64digest(raw_js)}"
end
end
12 changes: 7 additions & 5 deletions app/models/theme.rb
Expand Up @@ -6,7 +6,7 @@
class Theme < ActiveRecord::Base
include GlobalPath

BASE_COMPILER_VERSION = 78
BASE_COMPILER_VERSION = 80

class SettingsMigrationError < StandardError
end
Expand Down Expand Up @@ -356,11 +356,13 @@ def switch_to_theme!
end
end

def self.lookup_field(theme_id, target, field, skip_transformation: false)
def self.lookup_field(theme_id, target, field, skip_transformation: false, csp_nonce: nil)
return "" if theme_id.blank?

theme_ids = !skip_transformation ? transform_ids(theme_id) : [theme_id]
(resolve_baked_field(theme_ids, target.to_sym, field) || "").html_safe
resolved = (resolve_baked_field(theme_ids, target.to_sym, field) || "")
resolved = resolved.gsub(ThemeField::CSP_NONCE_PLACEHOLDER, csp_nonce) if csp_nonce
resolved.html_safe
end

def self.lookup_modifier(theme_ids, modifier_name)
Expand Down Expand Up @@ -469,8 +471,8 @@ def self.resolve_baked_field(theme_ids, target, name)
.compact

caches.map { |c| <<~HTML.html_safe }.join("\n")
<link rel="preload" href="#{c.url}" as="script">
<script defer src='#{c.url}' data-theme-id='#{c.theme_id}'></script>
<link rel="preload" href="#{c.url}" as="script" nonce="#{ThemeField::CSP_NONCE_PLACEHOLDER}">
<script defer src="#{c.url}" data-theme-id="#{c.theme_id}" nonce="#{ThemeField::CSP_NONCE_PLACEHOLDER}"></script>
HTML
end
when :translations
Expand Down
28 changes: 17 additions & 11 deletions app/models/theme_field.rb
Expand Up @@ -3,6 +3,9 @@
class ThemeField < ActiveRecord::Base
MIGRATION_NAME_PART_MAX_LENGTH = 150

# This string is not 'secret'. It's just randomized to avoid accidental clashes with genuine theme field content.
CSP_NONCE_PLACEHOLDER = "__CSP__NONCE__PLACEHOLDER__f72bff1b1768168a34ee092ce759f192__"

belongs_to :upload
has_one :javascript_cache, dependent: :destroy
has_one :upload_reference, as: :target, dependent: :destroy
Expand Down Expand Up @@ -168,12 +171,15 @@ def process_html(html)
doc
.css("script")
.each_with_index do |node, index|
next unless inline_javascript?(node)
js_compiler.append_raw_script(
"_html/#{Theme.targets[self.target_id]}/#{name}_#{index + 1}.js",
node.inner_html,
)
node.remove
if inline_javascript?(node)
js_compiler.append_raw_script(
"_html/#{Theme.targets[self.target_id]}/#{name}_#{index + 1}.js",
node.inner_html,
)
node.remove
else
node["nonce"] = CSP_NONCE_PLACEHOLDER
end
end

settings_hash = theme.build_settings_hash
Expand All @@ -185,9 +191,9 @@ def process_html(html)
javascript_cache.save!

doc.add_child(<<~HTML.html_safe) if javascript_cache.content.present?
<link rel="preload" href="#{javascript_cache.url}" as="script">
<script defer src='#{javascript_cache.url}' data-theme-id='#{theme_id}'></script>
HTML
<link rel="preload" href="#{javascript_cache.url}" as="script" nonce="#{CSP_NONCE_PLACEHOLDER}">
<script defer src='#{javascript_cache.url}' data-theme-id='#{theme_id}' nonce="#{CSP_NONCE_PLACEHOLDER}"></script>
HTML
[doc.to_s, errors&.join("\n")]
end

Expand Down Expand Up @@ -294,8 +300,8 @@ def process_translation
javascript_cache.save!
doc = ""
doc = <<~HTML.html_safe if javascript_cache.content.present?
<link rel="preload" href="#{javascript_cache.url}" as="script">
<script defer src='#{javascript_cache.url}' data-theme-id='#{theme_id}'></script>
<link rel="preload" href="#{javascript_cache.url}" as="script" nonce="#{ThemeField::CSP_NONCE_PLACEHOLDER}">
<script defer src="#{javascript_cache.url}" data-theme-id="#{theme_id}" nonce="#{ThemeField::CSP_NONCE_PLACEHOLDER}"></script>
HTML
[doc, errors&.join("\n")]
end
Expand Down
4 changes: 3 additions & 1 deletion app/views/common/_discourse_splash.html.erb
Expand Up @@ -246,6 +246,8 @@
</style>
</noscript>

<%= SplashScreenHelper.inline_splash_screen_script %>
<script nonce="<%= csp_nonce_placeholder %>">
<%= SplashScreenHelper.raw_js %>
</script>
</section>
<%- end %>
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="<%= google_tag_manager_nonce_placeholder %>"
data-nonce="<%= csp_nonce_placeholder %>"
data-container-id="<%= SiteSetting.gtm_container_id %>" />

<%= preload_script 'google-tag-manager' %>
2 changes: 1 addition & 1 deletion app/views/common/_google_universal_analytics.html.erb
Expand Up @@ -7,6 +7,6 @@
<% if SiteSetting.ga_version == "v3_analytics" %>
<%= preload_script "google-universal-analytics-v3" %>
<% elsif SiteSetting.ga_version == "v4_gtag" %>
<script async src="https://www.googletagmanager.com/gtag/js?id=<%= SiteSetting.ga_universal_tracking_code %>"></script>
<script async src="https://www.googletagmanager.com/gtag/js?id=<%= SiteSetting.ga_universal_tracking_code %>" nonce="<%= csp_nonce_placeholder %>"></script>
<%= preload_script "google-universal-analytics-v4" %>
<% end %>
10 changes: 5 additions & 5 deletions app/views/layouts/application.html.erb
Expand Up @@ -27,8 +27,8 @@
<% add_resource_preload_list(script_asset_path("start-discourse"), "script") %>
<% add_resource_preload_list(script_asset_path("browser-update"), "script") %>
<%- else %>
<link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script">
<link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script">
<link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
<link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
<%- end %>
<%= preload_script 'browser-detect' %>
Expand Down Expand Up @@ -132,11 +132,11 @@
</form>
<% end %>

<script defer src="<%= script_asset_path "start-discourse" %>"></script>
<script defer src="<%= script_asset_path "start-discourse" %>" nonce="<%= csp_nonce_placeholder %>"></script>

<%= yield :data %>

<script defer src="<%= script_asset_path "browser-update" %>"></script>
<script defer src="<%= script_asset_path "browser-update" %>" nonce="<%= csp_nonce_placeholder %>"></script>

<%- unless customization_disabled? %>
<%= theme_lookup("body_tag") %>
Expand All @@ -146,6 +146,6 @@
<%= build_plugin_html 'server:before-body-close' %>
<%- end %>

<%= DeferScriptHelper.safari_workaround_script %>
<script nonce="<%= csp_nonce_placeholder %>">/* Workaround for https://bugs.webkit.org/show_bug.cgi?id=209261 */</script>
</body>
</html>
4 changes: 2 additions & 2 deletions config/application.rb
Expand Up @@ -167,8 +167,8 @@ 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/csp_script_nonce_injector"
config.middleware.insert_after(ActionDispatch::Flash, Middleware::CspScriptNonceInjector)

require "middleware/discourse_public_exceptions"
config.exceptions_app = Middleware::DiscoursePublicExceptions.new(Rails.public_path)
Expand Down
7 changes: 7 additions & 0 deletions config/initializers/006-mini_profiler.rb
Expand Up @@ -88,6 +88,13 @@

Rack::MiniProfiler.config.max_traces_to_show = 100 if Rails.env.development?

Rack::MiniProfiler.config.content_security_policy_nonce =
Proc.new do |env, headers|
if csp = headers["Content-Security-Policy"]
csp[/script-src[^;]+'nonce-([^']+)'/, 1]
end
end

Rack::MiniProfiler.counter_method(Redis::Client, :call) { "redis" }
# Rack::MiniProfiler.counter_method(ActiveRecord::QueryMethods, 'build_arel')
# Rack::MiniProfiler.counter_method(Array, 'uniq')
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/099-anon-cache.rb
Expand Up @@ -11,6 +11,6 @@

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 Middleware::GtmScriptNonceInjector,
Rails.configuration.middleware.insert_after Middleware::CspScriptNonceInjector,
Middleware::AnonymousCache
end
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Expand Up @@ -1710,6 +1710,7 @@ en:
content_security_policy_collect_reports: "Enable CSP violation report collection at /csp_reports"
content_security_policy_frame_ancestors: "Restrict who can embed this site in iframes via CSP. Control allowed hosts on <a href='%{base_path}/admin/customize/embedding'>Embedding</a>"
content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See <a href='https://meta.discourse.org/t/mitigate-xss-attacks-with-content-security-policy/104243' target='_blank'>Mitigate XSS Attacks with Content Security Policy.</a> (CSP)"
content_security_policy_strict_dynamic: "EXPERIMENTAL: Use a strict-dynamic content security policy. This is more modern and more flexible than our default CSP configuration. This is fully functional, but not yet tested with all themes and plugins."
invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable."
include_secure_categories_in_tag_counts: "When enabled, count of topics for a tag will include topics that are in read restricted categories for all users. When disabled, normal users are only shown a count of topics for a tag where all the topics are in public categories."
display_personal_messages_tag_counts: "When enabled, count of personal messages tagged with a given tag will be displayed."
Expand Down
2 changes: 2 additions & 0 deletions config/site_settings.yml
Expand Up @@ -1985,6 +1985,8 @@ security:
content_security_policy_script_src:
type: simple_list
default: ""
content_security_policy_strict_dynamic:
default: false
invalidate_inactive_admin_email_after_days:
default: 365
min: 0
Expand Down
9 changes: 9 additions & 0 deletions lib/content_security_policy/builder.rb
Expand Up @@ -74,6 +74,15 @@ def extend_directive(directive, sources)
@directives[directive] ||= []

sources = Array(sources).map { |s| normalize_source(s) }

if SiteSetting.content_security_policy_strict_dynamic
# Strip any sources which are ignored under strict-dynamic
# If/when we make strict-dynamic the only option, we could print deprecation warnings
# asking plugin/theme authors to remove the unnecessary config
sources =
sources.reject { |s| s == "'unsafe-inline'" || s == "'self'" || !s.start_with?("'") }
end

@directives[directive].concat(sources)

@directives[directive].delete(:none) if @directives[directive].count > 1
Expand Down

1 comment on commit b1f74ab

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://meta.discourse.org/t/experimenting-with-a-strict-dynamic-content-security-policy-csp/295603/1

Please sign in to comment.