From 17e1bfe06971e9f508bc82e5053fdde7162e82ca Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Fri, 3 Jan 2025 12:21:17 +0800
Subject: [PATCH] SECURITY: Preload data only when rendering application layout

This commit drops the `before_action :preload_json` callback in `ApplicationController` as it adds unnecessary complexity to `ApplicationController` as well as other controllers which has to skip this callback. The source of the complexity comes mainly from the following two conditionals in the `preload_json` method:

```
    # We don't preload JSON on xhr or JSON request
    return if request.xhr? || request.format.json?

    # if we are posting in makes no sense to preload
    return if request.method != "GET"
```

Basically, the conditionals solely exists for optimization purposes to ensure that we don't run the preloading code when the request is not a GET request and the response is not expected to be HTML. The key problem here is that the conditionals are trying to expect what the content type of the response will be and this has proven to be hard to get right. Instead, we can simplify this problem by running the preloading code in a more deterministic way which is to preload only when the `application` layout is being rendered and this is main change that this commit introduces.
---
 app/controllers/application_controller.rb     | 167 +++---------------
 .../finish_installation_controller.rb         |   1 -
 app/controllers/site_controller.rb            |   6 +-
 app/helpers/application_helper.rb             |   8 +-
 app/models/topic.rb                           |   4 +-
 lib/application_layout_preloader.rb           | 146 +++++++++++++++
 lib/db_helper.rb                              |   2 +-
 spec/helpers/application_helper_spec.rb       |  18 +-
 .../requests/admin/backups_controller_spec.rb |   6 +-
 spec/requests/application_controller_spec.rb  |  35 ----
 10 files changed, 201 insertions(+), 192 deletions(-)
 create mode 100644 lib/application_layout_preloader.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 64810414534..84a6ec562bb 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -43,6 +43,7 @@ class ApplicationController < ActionController::Base
   before_action :block_if_requires_login
   before_action :redirect_to_profile_if_required
   before_action :preload_json
+  before_action :initialize_application_layout_preloader
   before_action :check_xhr
   after_action :add_readonly_header
   after_action :perform_refresh_session
@@ -277,6 +278,7 @@ class ApplicationController < ActionController::Base
 
   def rescue_discourse_actions(type, status_code, opts = nil)
     opts ||= {}
+
     show_json_errors =
       (request.format && request.format.json?) || (request.xhr?) ||
         ((params[:external_id] || "").ends_with? ".json")
@@ -339,8 +341,9 @@ class ApplicationController < ActionController::Base
       rescue Discourse::InvalidAccess
         return render plain: message, status: status_code
       end
+
       with_resolved_locale do
-        error_page_opts[:layout] = (opts[:include_ember] && @preloaded) ? set_layout : "no_ember"
+        error_page_opts[:layout] = opts[:include_ember] && @_preloaded ? set_layout : "no_ember"
         render html: build_not_found_page(error_page_opts)
       end
     end
@@ -424,31 +427,6 @@ class ApplicationController < ActionController::Base
     I18n.with_locale(locale) { yield }
   end
 
-  def store_preloaded(key, json)
-    @preloaded ||= {}
-    # I dislike that there is a gsub as opposed to a gsub!
-    #  but we can not be mucking with user input, I wonder if there is a way
-    #  to inject this safety deeper in the library or even in AM serializer
-    @preloaded[key] = json.gsub("</", "<\\/")
-  end
-
-  # If we are rendering HTML, preload the session data
-  def preload_json
-    # We don't preload JSON on xhr or JSON request
-    return if request.xhr? || request.format.json?
-
-    # if we are posting in makes no sense to preload
-    return if request.method != "GET"
-
-    # TODO should not be invoked on redirection so this should be further deferred
-    preload_anonymous_data
-
-    if current_user
-      current_user.sync_notification_channel_position
-      preload_current_user_data
-    end
-  end
-
   def set_mobile_view
     session[:mobile_view] = params[:mobile_view] if params.has_key?(:mobile_view)
   end
@@ -608,110 +586,36 @@ class ApplicationController < ActionController::Base
   end
 
   def login_method
-    return if current_user.anonymous?
+    return if !current_user || current_user.anonymous?
     current_user.authenticated_with_oauth ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL
   end
 
   private
 
-  def preload_anonymous_data
-    store_preloaded("site", Site.json_for(guardian))
-    store_preloaded("siteSettings", SiteSetting.client_settings_json)
-    store_preloaded("customHTML", custom_html_json)
-    store_preloaded("banner", banner_json)
-    store_preloaded("customEmoji", custom_emoji)
-    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)
+  # This method is intended to be a no-op.
+  # The only reason this `before_action` callback continues to exist is for backwards compatibility purposes which we cannot easily
+  # solve at this point. In the `rescue_discourse_actions` method, the `@_preloaded` instance variable is used to determine
+  # if the `no_ember` or `application` layout should be used. To use the `no_ember` layout, controllers have been
+  # setting `skip_before_action :preload_json`. This is however a flawed implementation as which layout is used for rendering
+  # errors should ideally not be set by skipping a `before_action` callback. To fix this properly will require some careful
+  # planning which we do not intend to tackle at this point.
+  def preload_json
+    return if request.format&.json? || request.xhr? || !request.get?
+    @_preloaded = {}
   end
 
-  def preload_current_user_data
-    store_preloaded(
-      "currentUser",
-      MultiJson.dump(
-        CurrentUserSerializer.new(
-          current_user,
-          scope: guardian,
-          root: false,
-          login_method: login_method,
-        ),
-      ),
-    )
-
-    report = TopicTrackingState.report(current_user)
-    serializer = TopicTrackingStateSerializer.new(report, scope: guardian, root: false)
-
-    hash = serializer.as_json
-
-    store_preloaded("topicTrackingStates", MultiJson.dump(hash[:data]))
-    store_preloaded("topicTrackingStateMeta", MultiJson.dump(hash[:meta]))
-
-    if current_user.admin?
-      # This is used in the wizard so we can preload fonts using the FontMap JS API.
-      store_preloaded("fontMap", MultiJson.dump(load_font_map))
-
-      # Used to show plugin-specific admin routes in the sidebar.
-      store_preloaded(
-        "visiblePlugins",
-        MultiJson.dump(
-          Discourse
-            .plugins_sorted_by_name(enabled_only: false)
-            .map do |plugin|
-              {
-                name: plugin.name.downcase,
-                humanized_name: plugin.humanized_name,
-                admin_route: plugin.full_admin_route,
-                enabled: plugin.enabled?,
-              }
-            end,
-        ),
+  def initialize_application_layout_preloader
+    @application_layout_preloader =
+      ApplicationLayoutPreloader.new(
+        guardian:,
+        theme_id: @theme_id,
+        theme_target: view_context.mobile_view? ? :mobile : :desktop,
+        login_method:,
       )
-    end
   end
 
-  def custom_html_json
-    target = view_context.mobile_view? ? :mobile : :desktop
-
-    data =
-      if @theme_id.present?
-        {
-          top: Theme.lookup_field(@theme_id, target, "after_header"),
-          footer: Theme.lookup_field(@theme_id, target, "footer"),
-        }
-      else
-        {}
-      end
-
-    data.merge! DiscoursePluginRegistry.custom_html if DiscoursePluginRegistry.custom_html
-
-    DiscoursePluginRegistry.html_builders.each do |name, _|
-      if name.start_with?("client:")
-        data[name.sub(/\Aclient:/, "")] = DiscoursePluginRegistry.build_html(name, self)
-      end
-    end
-
-    MultiJson.dump(data)
-  end
-
-  def self.banner_json_cache
-    @banner_json_cache ||= DistributedCache.new("banner_json")
-  end
-
-  def banner_json
-    return "{}" if !current_user && SiteSetting.login_required?
-
-    ApplicationController
-      .banner_json_cache
-      .defer_get_set("json") do
-        topic = Topic.where(archetype: Archetype.banner).first
-        banner = topic.present? ? topic.banner : {}
-        MultiJson.dump(banner)
-      end
-  end
-
-  def custom_emoji
-    serializer = ActiveModel::ArraySerializer.new(Emoji.custom, each_serializer: EmojiSerializer)
-    MultiJson.dump(serializer)
+  def store_preloaded(key, json)
+    @application_layout_preloader.store_preloaded(key, json)
   end
 
   # Render action for a JSON error.
@@ -947,7 +851,7 @@ class ApplicationController < ActionController::Base
 
   def build_not_found_page(opts = {})
     if SiteSetting.bootstrap_error_pages?
-      preload_json
+      initialize_application_layout_preloader
       opts[:layout] = "application" if opts[:layout] == "no_ember"
     end
 
@@ -1064,13 +968,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def activated_themes_json
-    id = @theme_id
-    return "{}" if id.blank?
-    ids = Theme.transform_ids(id)
-    Theme.where(id: ids).pluck(:id, :name).to_h.to_json
-  end
-
   def run_second_factor!(action_class, action_data: nil, target_user: current_user)
     if current_user && target_user != current_user
       # Anon can run 2fa against another target, but logged-in users should not.
@@ -1117,20 +1014,6 @@ class ApplicationController < ActionController::Base
     request.get? && !(request.format && request.format.json?) && !request.xhr?
   end
 
-  def load_font_map
-    DiscourseFonts
-      .fonts
-      .each_with_object({}) do |font, font_map|
-        next if !font[:variants]
-        font_map[font[:key]] = font[:variants].map do |v|
-          {
-            url: "#{Discourse.base_url}/fonts/#{v[:filename]}?v=#{DiscourseFonts::VERSION}",
-            weight: v[:weight],
-          }
-        end
-      end
-  end
-
   def fetch_limit_from_params(params: self.params, default:, max:)
     fetch_int_from_params(:limit, params: params, default: default, max: max)
   end
diff --git a/app/controllers/finish_installation_controller.rb b/app/controllers/finish_installation_controller.rb
index 6a4964aa411..09544afc85b 100644
--- a/app/controllers/finish_installation_controller.rb
+++ b/app/controllers/finish_installation_controller.rb
@@ -71,7 +71,6 @@ class FinishInstallationController < ApplicationController
   end
 
   def ensure_no_admins
-    preload_anonymous_data
     raise Discourse::InvalidAccess.new unless SiteSetting.has_login_hint?
   end
 end
diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb
index 9b6d2cb5ae4..99218675655 100644
--- a/app/controllers/site_controller.rb
+++ b/app/controllers/site_controller.rb
@@ -16,15 +16,15 @@ class SiteController < ApplicationController
   end
 
   def custom_html
-    render json: custom_html_json
+    render json: @application_layout_preloader.custom_html_json
   end
 
   def banner
-    render json: banner_json
+    render json: @application_layout_preloader.banner_json
   end
 
   def emoji
-    render json: custom_emoji
+    render json: @application_layout_preloader.custom_emoji
   end
 
   def basic_info
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index d10380f0d1c..5286856ddfb 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -704,8 +704,12 @@ module ApplicationHelper
   end
 
   def preloaded_json
-    return "{}" if @preloaded.blank?
-    @preloaded.transform_values { |value| escape_unicode(value) }.to_json
+    return "{}" if !@application_layout_preloader
+
+    @application_layout_preloader
+      .preloaded_data
+      .transform_values { |value| escape_unicode(value) }
+      .to_json
   end
 
   def client_side_setup_data
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 966aa87a01c..b6d232d7616 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -419,7 +419,7 @@ class Topic < ActiveRecord::Base
     banner = "banner"
 
     if archetype_before_last_save == banner || archetype == banner
-      ApplicationController.banner_json_cache.clear
+      ApplicationLayoutPreloader.banner_json_cache.clear
     end
 
     if tags_changed || saved_change_to_attribute?(:category_id) ||
@@ -1872,7 +1872,7 @@ class Topic < ActiveRecord::Base
 
   def update_excerpt(excerpt)
     update_column(:excerpt, excerpt)
-    ApplicationController.banner_json_cache.clear if archetype == "banner"
+    ApplicationLayoutPreloader.banner_json_cache.clear if archetype == "banner"
   end
 
   def pm_with_non_human_user?
diff --git a/lib/application_layout_preloader.rb b/lib/application_layout_preloader.rb
new file mode 100644
index 00000000000..466e0d21424
--- /dev/null
+++ b/lib/application_layout_preloader.rb
@@ -0,0 +1,146 @@
+# frozen_string_literal: true
+
+class ApplicationLayoutPreloader
+  include ReadOnlyMixin
+
+  def self.banner_json_cache
+    @banner_json_cache ||= DistributedCache.new("banner_json")
+  end
+
+  def initialize(guardian:, theme_id:, theme_target:, login_method:)
+    @guardian = guardian
+    @theme_id = theme_id
+    @theme_target = theme_target
+    @login_method = login_method
+    @preloaded = {}
+  end
+
+  def store_preloaded(key, json)
+    # I dislike that there is a gsub as opposed to a gsub!
+    #  but we can not be mucking with user input, I wonder if there is a way
+    #  to inject this safety deeper in the library or even in AM serializer
+    @preloaded[key] = json.gsub("</", "<\\/")
+  end
+
+  def preloaded_data
+    preload_anonymous_data
+
+    if @guardian.authenticated?
+      @guardian.user.sync_notification_channel_position
+      preload_current_user_data
+    end
+
+    @preloaded
+  end
+
+  def banner_json
+    return "{}" if !@guardian.authenticated? && SiteSetting.login_required?
+
+    self
+      .class
+      .banner_json_cache
+      .defer_get_set("json") do
+        topic = Topic.where(archetype: Archetype.banner).first
+        banner = topic.present? ? topic.banner : {}
+        MultiJson.dump(banner)
+      end
+  end
+
+  def custom_html_json
+    data =
+      if @theme_id.present?
+        {
+          top: Theme.lookup_field(@theme_id, @theme_target, "after_header"),
+          footer: Theme.lookup_field(@theme_id, @theme_target, "footer"),
+        }
+      else
+        {}
+      end
+
+    data.merge! DiscoursePluginRegistry.custom_html if DiscoursePluginRegistry.custom_html
+
+    DiscoursePluginRegistry.html_builders.each do |name, _|
+      if name.start_with?("client:")
+        data[name.sub(/\Aclient:/, "")] = DiscoursePluginRegistry.build_html(name, self)
+      end
+    end
+
+    MultiJson.dump(data)
+  end
+
+  private
+
+  def preload_current_user_data
+    @preloaded["currentUser"] = MultiJson.dump(
+      CurrentUserSerializer.new(
+        @guardian.user,
+        scope: @guardian,
+        root: false,
+        login_method: @login_method,
+      ),
+    )
+
+    report = TopicTrackingState.report(@guardian.user)
+    serializer = TopicTrackingStateSerializer.new(report, scope: @guardian, root: false)
+    hash = serializer.as_json
+
+    @preloaded["topicTrackingStates"] = MultiJson.dump(hash[:data])
+    @preloaded["topicTrackingStateMeta"] = MultiJson.dump(hash[:meta])
+
+    if @guardian.is_admin?
+      # This is used in the wizard so we can preload fonts using the FontMap JS API.
+      @preloaded["fontMap"] = MultiJson.dump(load_font_map)
+
+      # Used to show plugin-specific admin routes in the sidebar.
+      @preloaded["visiblePlugins"] = MultiJson.dump(
+        Discourse
+          .plugins_sorted_by_name(enabled_only: false)
+          .map do |plugin|
+            {
+              name: plugin.name.downcase,
+              humanized_name: plugin.humanized_name,
+              admin_route: plugin.full_admin_route,
+              enabled: plugin.enabled?,
+            }
+          end,
+      )
+    end
+  end
+
+  def preload_anonymous_data
+    @preloaded["site"] = Site.json_for(@guardian)
+    @preloaded["siteSettings"] = SiteSetting.client_settings_json
+    @preloaded["customHTML"] = custom_html_json
+    @preloaded["banner"] = banner_json
+    @preloaded["customEmoji"] = custom_emoji
+    @preloaded["isReadOnly"] = get_or_check_readonly_mode.to_json
+    @preloaded["isStaffWritesOnly"] = get_or_check_staff_writes_only_mode.to_json
+    @preloaded["activatedThemes"] = activated_themes_json
+  end
+
+  def activated_themes_json
+    id = @theme_id
+    return "{}" if id.blank?
+    ids = Theme.transform_ids(id)
+    Theme.where(id: ids).pluck(:id, :name).to_h.to_json
+  end
+
+  def load_font_map
+    DiscourseFonts
+      .fonts
+      .each_with_object({}) do |font, font_map|
+        next if !font[:variants]
+        font_map[font[:key]] = font[:variants].map do |v|
+          {
+            url: "#{Discourse.base_url}/fonts/#{v[:filename]}?v=#{DiscourseFonts::VERSION}",
+            weight: v[:weight],
+          }
+        end
+      end
+  end
+
+  def custom_emoji
+    serializer = ActiveModel::ArraySerializer.new(Emoji.custom, each_serializer: EmojiSerializer)
+    MultiJson.dump(serializer)
+  end
+end
diff --git a/lib/db_helper.rb b/lib/db_helper.rb
index c8e5d0691cd..86913a707ad 100644
--- a/lib/db_helper.rb
+++ b/lib/db_helper.rb
@@ -144,7 +144,7 @@ class DbHelper
     SiteSetting.refresh!
     Theme.expire_site_cache!
     SiteIconManager.ensure_optimized!
-    ApplicationController.banner_json_cache.clear
+    ApplicationLayoutPreloader.banner_json_cache.clear
   end
 
   def self.find_text_columns(excluded_tables)
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 89e1e8de879..1eff49dc8ea 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -565,14 +565,24 @@ RSpec.describe ApplicationHelper do
   end
 
   describe "preloaded_json" do
-    it "returns empty JSON if preloaded is empty" do
-      @preloaded = nil
+    fab!(:user)
+
+    it "returns empty JSON if preloader is not initialized" do
+      @application_layout_preloader = nil
       expect(helper.preloaded_json).to eq("{}")
     end
 
     it "escapes and strips invalid unicode and strips in json body" do
-      @preloaded = { test: %{["< \x80"]} }
-      expect(helper.preloaded_json).to eq(%{{"test":"[\\"\\u003c \uFFFD\\"]"}})
+      @application_layout_preloader =
+        ApplicationLayoutPreloader.new(
+          guardian: Guardian.new(user),
+          theme_id: nil,
+          theme_target: nil,
+          login_method: nil,
+        )
+
+      @application_layout_preloader.store_preloaded("test", %{["< \x80"]})
+      expect(helper.preloaded_json).to include(%{"test":"[\\"\\u003c \uFFFD\\"]"})
     end
   end
 
diff --git a/spec/requests/admin/backups_controller_spec.rb b/spec/requests/admin/backups_controller_spec.rb
index 031e2ed21b4..62c7c4b8769 100644
--- a/spec/requests/admin/backups_controller_spec.rb
+++ b/spec/requests/admin/backups_controller_spec.rb
@@ -22,8 +22,8 @@ RSpec.describe Admin::BackupsController do
   end
 
   def map_preloaded
-    controller
-      .instance_variable_get("@preloaded")
+    JSON
+      .parse(Nokogiri.HTML5(response.body).at_css("[data-preloaded]")["data-preloaded"])
       .map { |key, value| [key, JSON.parse(value)] }
       .to_h
   end
@@ -53,9 +53,11 @@ RSpec.describe Admin::BackupsController do
           expect(response.status).to eq(200)
 
           preloaded = map_preloaded
+
           expect(preloaded["operations_status"].symbolize_keys).to eq(
             BackupRestore.operations_status,
           )
+
           expect(preloaded["logs"].size).to eq(BackupRestore.logs.size)
         end
       end
diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb
index 0bd21a53cf0..0453a10c231 100644
--- a/spec/requests/application_controller_spec.rb
+++ b/spec/requests/application_controller_spec.rb
@@ -1295,41 +1295,6 @@ RSpec.describe ApplicationController do
         get "/", headers: { "HTTP_USER_AGENT" => "iam badcrawler" }
         expect(response.status).to eq(429)
       end
-
-      context "with XHR requests" do
-        before { global_setting :anon_cache_store_threshold, 1 }
-
-        def preloaded_data
-          response_html = Nokogiri::HTML5.fragment(response.body)
-          JSON.parse(response_html.css("#data-preloaded")[0]["data-preloaded"])
-        end
-
-        it "does not return the same preloaded data for XHR and non-XHR requests" do
-          # Request is stored in cache
-          get "/", headers: { "X-Requested-With" => "XMLHTTPrequest" }
-          expect(response.status).to eq(200)
-          expect(response.headers["X-Discourse-Cached"]).to eq("store")
-          expect(preloaded_data).not_to have_key("site")
-
-          # Request is served from cache
-          get "/", headers: { "X-Requested-With" => "xmlhttprequest" }
-          expect(response.status).to eq(200)
-          expect(response.headers["X-Discourse-Cached"]).to eq("true")
-          expect(preloaded_data).not_to have_key("site")
-
-          # Request is not served from cache because of different headers, but is stored
-          get "/"
-          expect(response.status).to eq(200)
-          expect(response.headers["X-Discourse-Cached"]).to eq("store")
-          expect(preloaded_data).to have_key("site")
-
-          # Request is served from cache
-          get "/", headers: { "X-Requested-With" => "xmlhttprequest" }
-          expect(response.status).to eq(200)
-          expect(response.headers["X-Discourse-Cached"]).to eq("true")
-          expect(preloaded_data).not_to have_key("site")
-        end
-      end
     end
   end