From bca855f2396998943d52545fd9d59dc0d3da3183 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 24 Apr 2024 09:40:13 +0100 Subject: [PATCH] FIX: Improve handling of 'PublicExceptions' when bootstrap_error_pages enabled (#26700) - Run the CSP-nonce-related middlewares on the generated response - Fix the readonly mode checking to avoid empty strings being passed (the `check_readonly_mode` before_action will not execute in the case of these re-dispatched exceptions) - Move the BlockRequestsMiddleware cookie-setting to the middleware, so that it is included even for unusual HTML responses like these exceptions --- app/controllers/application_controller.rb | 4 ++-- config/initializers/200-first_middlewares.rb | 15 +++++++++++---- lib/middleware/discourse_public_exceptions.rb | 11 ++++++++++- lib/read_only_mixin.rb | 10 ++++++++++ spec/rails_helper.rb | 9 --------- spec/system/bootstrap_error_pages_spec.rb | 19 +++++++++++++++++++ spec/system/content_security_policy_spec.rb | 13 +++++++++++++ 7 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 spec/system/bootstrap_error_pages_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 86d614abad7..16ace3d8bb3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -641,8 +641,8 @@ class ApplicationController < ActionController::Base store_preloaded("customHTML", custom_html_json) store_preloaded("banner", banner_json) store_preloaded("customEmoji", custom_emoji) - store_preloaded("isReadOnly", @readonly_mode.to_s) - store_preloaded("isStaffWritesOnly", @staff_writes_only_mode.to_s) + store_preloaded("isReadOnly", get_or_check_readonly_mode.to_json) + store_preloaded("isStaffWritesOnly", get_or_check_staff_writes_only_mode.to_json) store_preloaded("activatedThemes", activated_themes_json) end diff --git a/config/initializers/200-first_middlewares.rb b/config/initializers/200-first_middlewares.rb index a8390498cd8..55c2ac05102 100644 --- a/config/initializers/200-first_middlewares.rb +++ b/config/initializers/200-first_middlewares.rb @@ -62,16 +62,23 @@ if Rails.env.test? request.cookies[RSPEC_CURRENT_EXAMPLE_COOKIE_STRING] ) ) - [ + return [ 503, { "Content-Type" => "text/plain" }, [ "Blocked by BlockRequestsMiddleware for requests initiated by #{request.cookies[RSPEC_CURRENT_EXAMPLE_COOKIE_STRING]} when running #{self.class.current_example_location}", - ], + ] ] - else - @app.call(env) end + + status, headers, body = @app.call(env) + if headers["Content-Type"]&.match?(/html/) && BlockRequestsMiddleware.current_example_location + headers["Set-Cookie"] = [ + headers["Set-Cookie"], + "#{RSPEC_CURRENT_EXAMPLE_COOKIE_STRING}=#{BlockRequestsMiddleware.current_example_location}; path=/;", + ].compact.join("\n") + end + [status, headers, body] end end diff --git a/lib/middleware/discourse_public_exceptions.rb b/lib/middleware/discourse_public_exceptions.rb index 1c3ec1b0204..d3666201608 100644 --- a/lib/middleware/discourse_public_exceptions.rb +++ b/lib/middleware/discourse_public_exceptions.rb @@ -4,6 +4,12 @@ # we need to handle certain exceptions here module Middleware class DiscoursePublicExceptions < ::ActionDispatch::PublicExceptions + # These middlewares will be re-run when the exception response is generated + EXCEPTION_RESPONSE_MIDDLEWARES = [ + ContentSecurityPolicy::Middleware, + Middleware::CspScriptNonceInjector, + ] + INVALID_REQUEST_ERRORS = Set.new( [ @@ -60,7 +66,10 @@ module Middleware if ApplicationController.rescue_with_handler(exception, object: fake_controller) body = response.body body = [body] if String === body - return response.status, response.headers, body + rack_response = [response.status, response.headers, body] + app = lambda { |env| rack_response } + EXCEPTION_RESPONSE_MIDDLEWARES.each { |middleware| app = middleware.new(app) } + return app.call(env) end rescue => e return super if INVALID_REQUEST_ERRORS.include?(e.class) diff --git a/lib/read_only_mixin.rb b/lib/read_only_mixin.rb index 3c393d1b84a..7775040aff4 100644 --- a/lib/read_only_mixin.rb +++ b/lib/read_only_mixin.rb @@ -32,6 +32,16 @@ module ReadOnlyMixin end end + def get_or_check_readonly_mode + check_readonly_mode if @readonly_mode.nil? + @readonly_mode + end + + def get_or_check_staff_writes_only_mode + check_readonly_mode if @readonly_mode.nil? + @readonly_mode + end + def add_readonly_header response.headers["Discourse-Readonly"] = "true" if @readonly_mode end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 16a2ec5866c..5db7091bd3f 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -580,15 +580,6 @@ RSpec.configure do |config| ActiveRecord::Base.connection.schema_cache.add(table) end - ApplicationController.before_action(prepend: true) do - if BlockRequestsMiddleware.current_example_location && !request.xhr? && - request.format == "html" - cookies[ - BlockRequestsMiddleware::RSPEC_CURRENT_EXAMPLE_COOKIE_STRING - ] = BlockRequestsMiddleware.current_example_location - end - end - system_tests_initialized = true end diff --git a/spec/system/bootstrap_error_pages_spec.rb b/spec/system/bootstrap_error_pages_spec.rb new file mode 100644 index 00000000000..766c62c0d4c --- /dev/null +++ b/spec/system/bootstrap_error_pages_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +describe "bootstrap_error_pages", type: :system do + before { SiteSetting.bootstrap_error_pages = true } + + it "boots ember for non-existent route" do + visit "/foobar" + expect(page).not_to have_css("body.no-ember") + expect(page).to have_css("#site-logo") + expect(page).to have_css("div.page-not-found") + end + + it "boots ember for non-existent topic" do + visit "/t/999999999999" + expect(page).not_to have_css("body.no-ember") + expect(page).to have_css("#site-logo") + expect(page).to have_css("div.page-not-found") + end +end diff --git a/spec/system/content_security_policy_spec.rb b/spec/system/content_security_policy_spec.rb index 01c4d8669ca..935a6aba911 100644 --- a/spec/system/content_security_policy_spec.rb +++ b/spec/system/content_security_policy_spec.rb @@ -9,6 +9,19 @@ describe "Content security policy", type: :system do expect(page).to have_css("#site-logo") end + it "works for 'public exceptions' like RoutingError" do + expect(SiteSetting.content_security_policy).to eq(true) + SiteSetting.content_security_policy_strict_dynamic = true + SiteSetting.bootstrap_error_pages = true + + get "/nonexistent" + expect(response.headers["Content-Security-Policy"]).to include("'strict-dynamic'") + + visit "/nonexistent" + expect(page).not_to have_css("body.no-ember") + expect(page).to have_css("#site-logo") + end + it "can boot logster in strict_dynamic mode" do expect(SiteSetting.content_security_policy).to eq(true) sign_in Fabricate(:admin)