From 20bdbb05c32d6f93593974fbd37e811932841801 Mon Sep 17 00:00:00 2001
From: Tom Hughes <tom@compton.nu>
Date: Thu, 21 Mar 2024 20:32:12 +0000
Subject: [PATCH] Switch to using rails builtin content security policy support

---
 Gemfile                                       |  3 -
 Gemfile.lock                                  |  2 -
 app/controllers/accounts_controller.rb        | 12 +---
 app/controllers/application_controller.rb     | 50 ++++++++--------
 app/controllers/diary_entries_controller.rb   |  3 +-
 app/controllers/export_controller.rb          | 10 ++--
 app/controllers/messages_controller.rb        |  3 +-
 .../oauth2_authorizations_controller.rb       |  9 +--
 app/controllers/oauth_controller.rb           |  4 +-
 app/controllers/sessions_controller.rb        |  4 +-
 app/controllers/site_controller.rb            | 24 ++++----
 app/controllers/users_controller.rb           |  8 +--
 app/views/layouts/_head.html.erb              |  2 +-
 .../initializers/content_security_policy.rb   | 58 +++++++++++++------
 config/initializers/secure_headers.rb         | 50 ----------------
 config/initializers/session_store.rb          |  4 +-
 16 files changed, 101 insertions(+), 145 deletions(-)
 delete mode 100644 config/initializers/secure_headers.rb

diff --git a/Gemfile b/Gemfile
index 75387b5d5..77a9f1c97 100644
--- a/Gemfile
+++ b/Gemfile
@@ -116,9 +116,6 @@ gem "connection_pool"
 gem "dalli"
 gem "kgio"
 
-# Load secure_headers for Content-Security-Policy support
-gem "secure_headers"
-
 # Load canonical-rails to generate canonical URLs
 gem "canonical-rails"
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 23518339b..bf78a3d3d 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -529,7 +529,6 @@ GEM
     sass-embedded (1.64.2)
       google-protobuf (~> 3.23)
       rake (>= 13.0.0)
-    secure_headers (6.5.0)
     selenium-webdriver (4.21.1)
       base64 (~> 0.2)
       rexml (~> 3.2, >= 3.2.5)
@@ -690,7 +689,6 @@ DEPENDENCIES
   rubocop-rake
   sanitize
   sass-embedded (~> 1.64.0)
-  secure_headers
   selenium-webdriver
   simplecov
   simplecov-lcov
diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb
index 6b47ca666..d45dce66a 100644
--- a/app/controllers/accounts_controller.rb
+++ b/app/controllers/accounts_controller.rb
@@ -11,15 +11,13 @@ class AccountsController < ApplicationController
 
   before_action :check_database_readable
   before_action :check_database_writable, :only => [:update]
-  before_action :allow_thirdparty_images, :only => [:edit, :update]
+
+  allow_thirdparty_images :only => [:edit, :update]
+  allow_social_login :only => [:edit, :update]
 
   def edit
     @tokens = current_user.oauth_tokens.authorized
 
-    append_content_security_policy_directives(
-      :form_action => %w[accounts.google.com *.facebook.com login.microsoftonline.com github.com meta.wikimedia.org]
-    )
-
     if errors = session.delete(:user_errors)
       errors.each do |attribute, error|
         current_user.errors.add(attribute, error)
@@ -31,10 +29,6 @@ class AccountsController < ApplicationController
   def update
     @tokens = current_user.oauth_tokens.authorized
 
-    append_content_security_policy_directives(
-      :form_action => %w[accounts.google.com *.facebook.com login.microsoftonline.com github.com meta.wikimedia.org]
-    )
-
     user_params = params.require(:user).permit(:display_name, :new_email, :pass_crypt, :pass_crypt_confirmation, :auth_provider)
 
     if params[:user][:auth_provider].blank? ||
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index f5accc3c4..05fa76658 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -13,13 +13,30 @@ class ApplicationController < ActionController::Base
   rescue_from RailsParam::InvalidParameterError, :with => :invalid_parameter
 
   before_action :fetch_body
-  around_action :better_errors_allow_inline, :if => proc { Rails.env.development? }
 
   attr_accessor :current_user, :oauth_token
 
   helper_method :current_user
   helper_method :oauth_token
 
+  def self.allow_thirdparty_images(**options)
+    content_security_policy(options) do |policy|
+      policy.img_src("*")
+    end
+  end
+
+  def self.allow_social_login(**options)
+    content_security_policy(options) do |policy|
+      policy.form_action(*policy.form_action, "accounts.google.com", "*.facebook.com", "login.microsoftonline.com", "github.com", "meta.wikimedia.org")
+    end
+  end
+
+  def self.allow_all_form_action(**options)
+    content_security_policy(options) do |policy|
+      policy.form_action(nil)
+    end
+  end
+
   private
 
   def authorize_web
@@ -233,13 +250,15 @@ class ApplicationController < ActionController::Base
   end
 
   def map_layout
-    append_content_security_policy_directives(
-      :child_src => %w[http://127.0.0.1:8111 https://127.0.0.1:8112],
-      :frame_src => %w[http://127.0.0.1:8111 https://127.0.0.1:8112],
-      :connect_src => [Settings.nominatim_url, Settings.overpass_url, Settings.fossgis_osrm_url, Settings.graphhopper_url, Settings.fossgis_valhalla_url],
-      :form_action => %w[render.openstreetmap.org],
-      :style_src => %w['unsafe-inline']
-    )
+    policy = request.content_security_policy.clone
+
+    policy.child_src(*policy.child_src, "http://127.0.0.1:8111", "https://127.0.0.1:8112")
+    policy.frame_src(*policy.frame_src, "http://127.0.0.1:8111", "https://127.0.0.1:8112")
+    policy.connect_src(*policy.connect_src, Settings.nominatim_url, Settings.overpass_url, Settings.fossgis_osrm_url, Settings.graphhopper_url, Settings.fossgis_valhalla_url)
+    policy.form_action(*policy.form_action, "render.openstreetmap.org")
+    policy.style_src(*policy.style_src, :unsafe_inline)
+
+    request.content_security_policy = policy
 
     case Settings.status
     when "database_offline", "api_offline"
@@ -251,10 +270,6 @@ class ApplicationController < ActionController::Base
     request.xhr? ? "xhr" : "map"
   end
 
-  def allow_thirdparty_images
-    append_content_security_policy_directives(:img_src => %w[*])
-  end
-
   def preferred_editor
     if params[:editor]
       params[:editor]
@@ -277,17 +292,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def better_errors_allow_inline
-    yield
-  rescue StandardError
-    append_content_security_policy_directives(
-      :script_src => %w['unsafe-inline'],
-      :style_src => %w['unsafe-inline']
-    )
-
-    raise
-  end
-
   def current_ability
     Ability.new(current_user)
   end
diff --git a/app/controllers/diary_entries_controller.rb b/app/controllers/diary_entries_controller.rb
index 8da0842eb..9af36709e 100644
--- a/app/controllers/diary_entries_controller.rb
+++ b/app/controllers/diary_entries_controller.rb
@@ -12,7 +12,8 @@ class DiaryEntriesController < ApplicationController
 
   before_action :lookup_user, :only => [:show, :comments]
   before_action :check_database_writable, :only => [:new, :create, :edit, :update, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
-  before_action :allow_thirdparty_images, :only => [:new, :create, :edit, :update, :index, :show, :comments]
+
+  allow_thirdparty_images :only => [:new, :create, :edit, :update, :index, :show, :comments]
 
   def index
     if params[:display_name]
diff --git a/app/controllers/export_controller.rb b/app/controllers/export_controller.rb
index 94851de96..cddc97b68 100644
--- a/app/controllers/export_controller.rb
+++ b/app/controllers/export_controller.rb
@@ -4,6 +4,10 @@ class ExportController < ApplicationController
   before_action :update_totp, :only => [:finish]
   authorize_resource :class => false
 
+  content_security_policy(:only => :embed) do |policy|
+    policy.frame_ancestors("*")
+  end
+
   caches_page :embed
 
   # When the user clicks 'Export' we redirect to a URL which generates the export download
@@ -25,9 +29,5 @@ class ExportController < ApplicationController
     end
   end
 
-  def embed
-    append_content_security_policy_directives(
-      :frame_ancestors => %w[*]
-    )
-  end
+  def embed; end
 end
diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index adfbd9157..779174e25 100644
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -11,7 +11,8 @@ class MessagesController < ApplicationController
   before_action :lookup_user, :only => [:new, :create]
   before_action :check_database_readable
   before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
-  before_action :allow_thirdparty_images, :only => [:new, :create, :show]
+
+  allow_thirdparty_images :only => [:new, :create, :show]
 
   # Show a message
   def show
diff --git a/app/controllers/oauth2_authorizations_controller.rb b/app/controllers/oauth2_authorizations_controller.rb
index dca95de4e..415ab2775 100644
--- a/app/controllers/oauth2_authorizations_controller.rb
+++ b/app/controllers/oauth2_authorizations_controller.rb
@@ -3,13 +3,8 @@ class Oauth2AuthorizationsController < Doorkeeper::AuthorizationsController
 
   prepend_before_action :authorize_web
   before_action :set_locale
-  before_action :allow_all_form_action, :only => [:new]
 
-  authorize_resource :class => false
-
-  private
+  allow_all_form_action :only => :new
 
-  def allow_all_form_action
-    override_content_security_policy_directives(:form_action => []) if Settings.csp_enforce || Settings.key?(:csp_report_url)
-  end
+  authorize_resource :class => false
 end
diff --git a/app/controllers/oauth_controller.rb b/app/controllers/oauth_controller.rb
index 49af05b0d..62a68b533 100644
--- a/app/controllers/oauth_controller.rb
+++ b/app/controllers/oauth_controller.rb
@@ -9,6 +9,8 @@ class OauthController < ApplicationController
 
   layout "site"
 
+  allow_all_form_action :only => :oauth1_authorize
+
   def revoke
     @token = current_user.oauth_tokens.find_by :token => params[:token]
     if @token
@@ -41,8 +43,6 @@ class OauthController < ApplicationController
   end
 
   def oauth1_authorize
-    override_content_security_policy_directives(:form_action => []) if Settings.csp_enforce || Settings.key?(:csp_report_url)
-
     if @token.invalidated?
       @message = t "oauth.authorize_failure.invalid"
       render :action => "authorize_failure"
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 2b6905ebb..a3e6f42f0 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -11,9 +11,9 @@ class SessionsController < ApplicationController
 
   authorize_resource :class => false
 
-  def new
-    override_content_security_policy_directives(:form_action => []) if Settings.csp_enforce || Settings.key?(:csp_report_url)
+  allow_all_form_action :only => :new
 
+  def new
     referer = safe_referer(params[:referer]) if params[:referer]
 
     parse_oauth_referer referer
diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb
index 3d830c63f..172be5653 100644
--- a/app/controllers/site_controller.rb
+++ b/app/controllers/site_controller.rb
@@ -12,6 +12,17 @@ class SiteController < ApplicationController
 
   authorize_resource :class => false
 
+  content_security_policy(:only => :edit) do |policy|
+    policy.frame_src(*policy.frame_src, :blob)
+  end
+
+  content_security_policy(:only => :id) do |policy|
+    policy.connect_src("*")
+    policy.img_src("*", :blob)
+    policy.script_src(*policy.script_src, "dev.virtualearth.net", :unsafe_eval)
+    policy.style_src(*policy.style_src, :unsafe_inline)
+  end
+
   def index
     session[:location] ||= OSM.ip_location(request.env["REMOTE_ADDR"]) unless Settings.status == "database_readonly" || Settings.status == "database_offline"
   end
@@ -71,12 +82,6 @@ class SiteController < ApplicationController
       require_user
     end
 
-    if %w[id].include?(editor)
-      append_content_security_policy_directives(
-        :frame_src => %w[blob:]
-      )
-    end
-
     begin
       if params[:node]
         bbox = Node.visible.find(params[:node]).bbox.to_unscaled
@@ -136,13 +141,6 @@ class SiteController < ApplicationController
   end
 
   def id
-    append_content_security_policy_directives(
-      :connect_src => %w[*],
-      :img_src => %w[* blob:],
-      :script_src => %w[dev.virtualearth.net 'unsafe-eval'],
-      :style_src => %w['unsafe-inline']
-    )
-
     render :layout => false
   end
 
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f3ba5df2c..186e06120 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -17,7 +17,9 @@ class UsersController < ApplicationController
   before_action :check_database_writable, :only => [:new, :go_public]
   before_action :require_cookies, :only => [:new]
   before_action :lookup_user_by_name, :only => [:set_status, :destroy]
-  before_action :allow_thirdparty_images, :only => [:show]
+
+  allow_thirdparty_images :only => :show
+  allow_social_login :only => :new
 
   ##
   # display a list of users matching specified criteria
@@ -58,10 +60,6 @@ class UsersController < ApplicationController
 
     parse_oauth_referer @referer
 
-    append_content_security_policy_directives(
-      :form_action => %w[accounts.google.com *.facebook.com login.microsoftonline.com github.com meta.wikimedia.org]
-    )
-
     if current_user
       # The user is logged in already, so don't show them the signup
       # page, instead send them to the home page
diff --git a/app/views/layouts/_head.html.erb b/app/views/layouts/_head.html.erb
index 724ca5526..3c691612a 100644
--- a/app/views/layouts/_head.html.erb
+++ b/app/views/layouts/_head.html.erb
@@ -12,6 +12,6 @@
   <%= yield :head %>
   <%= yield :auto_discovery_link_tag %>
   <%= csrf_meta_tag %>
-  <meta name="csp-nonce" content="<%= content_security_policy_style_nonce %>" />
+  <%= csp_meta_tag %>
   <title><%= "#{@title} | " if @title %><%= t "layouts.project_name.title" %></title>
 <% end %>
diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb
index b3076b38f..5ce53e863 100644
--- a/config/initializers/content_security_policy.rb
+++ b/config/initializers/content_security_policy.rb
@@ -4,22 +4,42 @@
 # See the Securing Rails Applications Guide for more information:
 # https://guides.rubyonrails.org/security.html#content-security-policy-header
 
-# Rails.application.configure do
-#   config.content_security_policy do |policy|
-#     policy.default_src :self, :https
-#     policy.font_src    :self, :https, :data
-#     policy.img_src     :self, :https, :data
-#     policy.object_src  :none
-#     policy.script_src  :self, :https
-#     policy.style_src   :self, :https
-#     # Specify URI for violation reports
-#     # policy.report_uri "/csp-violation-report-endpoint"
-#   end
-#
-#   # Generate session nonces for permitted importmap, inline scripts, and inline styles.
-#   config.content_security_policy_nonce_generator = ->(request) { request.session.id.to_s }
-#   config.content_security_policy_nonce_directives = %w(script-src style-src)
-#
-#   # Report violations without enforcing the policy.
-#   # config.content_security_policy_report_only = true
-# end
+Rails.application.configure do
+  connect_src = [:self]
+  img_src = [:self, :data, "www.gravatar.com", "*.wp.com", "tile.openstreetmap.org", "*.tile.thunderforest.com", "tile.tracestrack.com", "*.openstreetmap.fr"]
+  script_src = [:self]
+
+  connect_src << Settings.matomo["location"] if defined?(Settings.matomo)
+  img_src << Settings.matomo["location"] if defined?(Settings.matomo)
+  script_src << Settings.matomo["location"] if defined?(Settings.matomo)
+
+  img_src << Settings.avatar_storage_url if Settings.key?(:avatar_storage_url)
+  img_src << Settings.trace_image_storage_url if Settings.key?(:trace_image_storage_url)
+
+  config.content_security_policy do |policy|
+    policy.default_src :self
+    policy.child_src(:self)
+    policy.connect_src(*connect_src)
+    policy.font_src(:none)
+    policy.form_action(:self)
+    policy.frame_ancestors(:self)
+    policy.frame_src(:self)
+    policy.img_src(*img_src)
+    policy.manifest_src(:self)
+    policy.media_src(:none)
+    policy.object_src(:self)
+    policy.plugin_types
+    policy.script_src(*script_src)
+    policy.style_src(:self)
+    policy.worker_src(:none)
+    policy.manifest_src(:self)
+    policy.report_uri(Settings.csp_report_url) if Settings.key?(:csp_report_url)
+  end
+
+  # Generate session nonces for permitted importmap and inline scripts
+  config.content_security_policy_nonce_generator = ->(_request) { SecureRandom.base64(24) }
+  config.content_security_policy_nonce_directives = %w[style-src]
+
+  # Report violations without enforcing the policy.
+  config.content_security_policy_report_only = true unless Settings.csp_enforce
+end
diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb
deleted file mode 100644
index 60f155139..000000000
--- a/config/initializers/secure_headers.rb
+++ /dev/null
@@ -1,50 +0,0 @@
-csp_policy = {
-  :preserve_schemes => true,
-  :default_src => %w['self'],
-  :child_src => %w['self'],
-  :connect_src => %w['self'],
-  :font_src => %w['none'],
-  :form_action => %w['self'],
-  :frame_ancestors => %w['self'],
-  :frame_src => %w['self'],
-  :img_src => %w['self' data: www.gravatar.com *.wp.com tile.openstreetmap.org *.tile.openstreetmap.org *.tile.thunderforest.com tile.tracestrack.com *.openstreetmap.fr],
-  :manifest_src => %w['self'],
-  :media_src => %w['none'],
-  :object_src => %w['self'],
-  :plugin_types => %w[],
-  :script_src => %w['self'],
-  :style_src => %w['self'],
-  :worker_src => %w['none'],
-  :report_uri => []
-}
-
-csp_policy[:connect_src] << Settings.matomo["location"] if defined?(Settings.matomo)
-csp_policy[:img_src] << Settings.matomo["location"] if defined?(Settings.matomo)
-csp_policy[:script_src] << Settings.matomo["location"] if defined?(Settings.matomo)
-
-csp_policy[:img_src] << Settings.avatar_storage_url if Settings.key?(:avatar_storage_url)
-csp_policy[:img_src] << Settings.trace_image_storage_url if Settings.key?(:trace_image_storage_url)
-
-csp_policy[:report_uri] << Settings.csp_report_url if Settings.key?(:csp_report_url)
-
-cookie_policy = {
-  :httponly => { :only => %w[_osm_session _osm_totp_token] }
-}
-
-SecureHeaders::Configuration.default do |config|
-  config.hsts = SecureHeaders::OPT_OUT
-  config.referrer_policy = "strict-origin-when-cross-origin"
-
-  if Settings.csp_enforce
-    config.csp = csp_policy
-    config.csp_report_only = SecureHeaders::OPT_OUT
-  elsif Settings.key?(:csp_report_url)
-    config.csp = SecureHeaders::OPT_OUT
-    config.csp_report_only = csp_policy
-  else
-    config.csp = SecureHeaders::OPT_OUT
-    config.csp_report_only = SecureHeaders::OPT_OUT
-  end
-
-  config.cookies = cookie_policy
-end
diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb
index a12284855..4119064c3 100644
--- a/config/initializers/session_store.rb
+++ b/config/initializers/session_store.rb
@@ -1,7 +1,7 @@
 # Be sure to restart your server when you modify this file.
 
 if Settings.key?(:memcache_servers)
-  Rails.application.config.session_store :mem_cache_store, :memcache_server => Settings.memcache_servers, :namespace => "rails:session", :key => "_osm_session"
+  Rails.application.config.session_store :mem_cache_store, :memcache_server => Settings.memcache_servers, :namespace => "rails:session", :key => "_osm_session", :same_site => :lax
 else
-  Rails.application.config.session_store :cache_store, :key => "_osm_session", :cache => ActiveSupport::Cache::MemoryStore.new
+  Rails.application.config.session_store :cache_store, :key => "_osm_session", :cache => ActiveSupport::Cache::MemoryStore.new, :same_site => :lax
 end
-- 
2.39.5