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

@@ -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

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

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

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

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?

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

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)

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
 

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

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