SECURITY: Error responses missing Cache-Control header

tgxworld · tgxworld · commit fd567af7bf5a · 2025-10-28T14:41:01.000+08:00
By default, we want all responses to have the `Cache-Control` header set
to `no-cache, no-store`. Individual controller actions can override the
header when need be.
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -30,6 +30,7 @@ def handle_unverified_request
     end
   end
 
+  around_action :ensure_dont_cache_page
   before_action :check_readonly_mode
   before_action :handle_theme
   before_action :set_current_user_for_logs
@@ -47,7 +48,6 @@ def handle_unverified_request
   before_action :check_xhr
   after_action :add_readonly_header
   after_action :perform_refresh_session
-  after_action :dont_cache_page
   after_action :conditionally_allow_site_embedding
   after_action :ensure_vary_header
   after_action :add_noindex_header,
@@ -157,6 +157,11 @@ class PluginDisabled < StandardError
   end
 
   rescue_from ActionController::RoutingError, PluginDisabled do
+    # This error is raised outside of the normal request response cycle and is called via the
+    # `DiscoursePublicExceptions` middleware which creates a new instance of the ApplicationController.
+    # As a result, controller actions hooks are not called and we need to explicitly call `dont_cache_page` here.
+    dont_cache_page
+
     rescue_discourse_actions(:not_found, 404)
   end
 
@@ -1055,4 +1060,10 @@ def clean_xml
   def service_params
     { params: params.to_unsafe_h, guardian: }
   end
+
+  def ensure_dont_cache_page
+    yield
+  ensure
+    dont_cache_page
+  end
 end
diff --git a/lib/middleware/discourse_public_exceptions.rb b/lib/middleware/discourse_public_exceptions.rb
@@ -65,6 +65,8 @@ def call(env)
           end
 
           if ApplicationController.rescue_with_handler(exception, object: fake_controller)
+            # Calling `ActionDispatch::Response#to_a` here to ensure that cache control headers are set.
+            response.to_a
             body = response.body
             body = [body] if String === body
             rack_response = [response.status, response.headers, body]
diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb
@@ -1,6 +1,31 @@
 # frozen_string_literal: true
 
 RSpec.describe ApplicationController do
+  fab!(:user)
+
+  context "for cache control headers" do
+    it "sets the `no-cache, no-store` cache control response header when no error is raised" do
+      get "/latest"
+
+      expect(response.status).to eq(200)
+      expect(response.headers["Cache-Control"]).to eq("no-cache, no-store")
+    end
+
+    it "sets the `no-cache, no-store` cache control response header when ActionController::RoutingError is raised" do
+      get "/invalid-urlllllllllll"
+
+      expect(response.status).to eq(404)
+      expect(response.headers["Cache-Control"]).to eq("no-cache, no-store")
+    end
+
+    it "sets the `no-cache, no-store` cache control response header when Discourse::InvalidAccess is raised" do
+      get "/latest.json", headers: { HTTP_API_KEY: "invalid-api-key" }
+
+      expect(response.status).to eq(403)
+      expect(response.headers["Cache-Control"]).to eq("no-cache, no-store")
+    end
+  end
+
   describe "#redirect_to_login_if_required" do
     let(:admin) { Fabricate(:admin) }
 
