DEV: Consolidate experimental 'Link' header implementations (#26377)

This commit removes the 'experimental_preconnect_link_header' site setting, and the 'preload_link_header' site setting, and introduces two new global settings: early_hint_header_mode and early_hint_header_name.

We don't actually send 103 Early Hint responses from Discourse. However, upstream proxies can be configured to cache a response header from the app and use that to send an Early Hint response to future clients.

- `early_hint_header_mode` specifies the mode for the early hint header. Can be nil (disabled), "preconnect" (lists just CDN domains) or "preload" (lists all assets).
- `early_hint_header_name` specifies which header name to use for the early hint. Defaults to "Link", but can be changed to support different proxy mechanisms.

app/controllers/application_controller.rb

@@ -53,7 +53,7 @@ class ApplicationController < ActionController::Base
   after_action :add_noindex_header_to_non_canonical, if: :spa_boot_request?
   after_action :set_cross_origin_opener_policy_header, if: :spa_boot_request?
   after_action :clean_xml, if: :is_feed_response?
-  around_action :add_link_header, if: -> { spa_boot_request? }
+  around_action :add_early_hint_header, if: -> { spa_boot_request? }
 
   HONEYPOT_KEY ||= "HONEYPOT_KEY"
   CHALLENGE_KEY ||= "CHALLENGE_KEY"
@@ -1096,29 +1096,29 @@ class ApplicationController < ActionController::Base
     result
   end
 
-  def add_link_header
+  # We don't actually send 103 Early Hint responses from Discourse. However, upstream proxies can be configured
-    @links_to_preload = [] if GlobalSetting.preload_link_header
+  # to cache a response header from the app and use that to send an Early Hint response to future clients.
+  # See 'early_hint_header_mode' and 'early_hint_header_name' Global Setting descriptions for more info.
+  def add_early_hint_header
+    return yield if GlobalSetting.early_hint_header_mode.nil?
+
+    @asset_preload_links = []
 
     yield
 
     links = []
 
-    if SiteSetting.experimental_preconnect_link_header
+    if GlobalSetting.early_hint_header_mode == "preconnect"
       [GlobalSetting.cdn_url, SiteSetting.s3_cdn_url].each do |url|
         next if url.blank?
         base_url = URI.join(url, "/").to_s.chomp("/")
-
         links.push("<#{base_url}>; rel=preconnect")
-        # Not all browsers support the preconnect resource hint so we are adding dns-prefetch as the fallback
-        links.push("<#{base_url}>; rel=dns-prefetch")
       end
+    elsif GlobalSetting.early_hint_header_mode == "preload"
+      links.push(*@asset_preload_links)
     end
 
-    if GlobalSetting.preload_link_header && !@links_to_preload.empty?
+    response.headers[GlobalSetting.early_hint_header_name] = links.join(", ") if links.present?
-      links = links.concat(@links_to_preload)
-    end
-
-    response.headers["Link"] = links.join(", ") if links.present?
   end
 
   def spa_boot_request?

app/helpers/application_helper.rb

@@ -158,8 +158,8 @@ module ApplicationHelper
   end
 
   def add_resource_preload_list(resource_url, type)
-    if !@links_to_preload.nil?
+    if !@asset_preload_links.nil?
-      @links_to_preload << %Q(<#{resource_url}>; rel="preload"; as="#{type}")
+      @asset_preload_links << %Q(<#{resource_url}>; rel="preload"; as="#{type}")
     end
   end
 

app/views/layouts/application.html.erb

@@ -7,7 +7,7 @@
     <meta name="discourse_theme_id" content="<%= theme_id %>">
     <meta name="discourse_current_homepage" content="<%= current_homepage %>">
 
-    <%- if GlobalSetting.preload_link_header %>
+    <%- if GlobalSetting.early_hint_header_mode == "prefetch" %>
       <%= render partial: "common/discourse_preload_stylesheet" %>
     <%- end %>
     <%= render partial: "layouts/head" %>
@@ -23,13 +23,11 @@
 
     <%= build_plugin_html 'server:before-script-load' %>
 
-    <%- if GlobalSetting.preload_link_header %>
+    <% add_resource_preload_list(script_asset_path("start-discourse"), "script") %>
-      <% add_resource_preload_list(script_asset_path("start-discourse"), "script") %>
+    <% add_resource_preload_list(script_asset_path("browser-update"), "script") %>
-      <% add_resource_preload_list(script_asset_path("browser-update"), "script") %>
+    <link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
-    <%- else %>
+    <link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
-      <link rel="preload" href="<%= script_asset_path "start-discourse" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
+    
-      <link rel="preload" href="<%= script_asset_path "browser-update" %>" as="script" nonce="<%= csp_nonce_placeholder %>">
-    <%- end %>  
     <%= preload_script 'browser-detect' %>
 
     <%= preload_script "vendor" %>

config/discourse_defaults.conf

@@ -366,8 +366,12 @@ enable_long_polling =
 # Length of time to hold open a long polling connection in milliseconds
 long_polling_interval =
 
-# Moves asset preloading from tags in the response document head to response headers
+# Specify the mode for the early hint header. Can be nil (disabled), "preconnect" (lists just CDN domains) or "preload" (lists all assets).
-preload_link_header = false
+# The 'preload' mode currently serves inconsistent headers for different pages/users, and is not recommended for production use.
+early_hint_header_mode =
+
+# Specify which header name to use for the early hint. Defaults to "Link", but can be changed to support different proxy mechanisms.
+early_hint_header_name = "Link"
 
 # When using an external upload store, redirect `user_avatar` requests instead of proxying
 redirect_avatar_requests = false

config/site_settings.yml

@@ -2368,9 +2368,6 @@ developer:
   experimental_objects_type_for_theme_settings:
     default: false
     hidden: true
-  experimental_preconnect_link_header:
-    default: false
-    hidden: true
 
 navigation:
   navigation_menu:

spec/helpers/application_helper_spec.rb

@@ -133,42 +133,42 @@ RSpec.describe ApplicationHelper do
 
   describe "add_resource_preload_list" do
     it "adds resources to the preload list when it's available" do
-      @links_to_preload = []
+      @asset_preload_links = []
       add_resource_preload_list("/assets/start-discourse.js", "script")
       add_resource_preload_list("/assets/discourse.css", "style")
 
-      expect(@links_to_preload.size).to eq(2)
+      expect(@asset_preload_links.size).to eq(2)
     end
 
     it "doesn't add resources to the preload list when it's not available" do
-      @links_to_preload = nil
+      @asset_preload_links = nil
       add_resource_preload_list("/assets/start-discourse.js", "script")
       add_resource_preload_list("/assets/discourse.css", "style")
 
-      expect(@links_to_preload).to eq(nil)
+      expect(@asset_preload_links).to eq(nil)
     end
 
     it "adds resources to the preload list when preload_script is called" do
-      @links_to_preload = []
+      @asset_preload_links = []
       helper.preload_script("start-discourse")
 
-      expect(@links_to_preload.size).to eq(1)
+      expect(@asset_preload_links.size).to eq(1)
     end
 
     it "adds resources to the preload list when discourse_stylesheet_link_tag is called" do
-      @links_to_preload = []
+      @asset_preload_links = []
       helper.discourse_stylesheet_link_tag(:desktop)
 
-      expect(@links_to_preload.size).to eq(1)
+      expect(@asset_preload_links.size).to eq(1)
     end
 
     it "adds resources as the correct type" do
-      @links_to_preload = []
+      @asset_preload_links = []
       helper.discourse_stylesheet_link_tag(:desktop)
       helper.preload_script("start-discourse")
 
-      expect(@links_to_preload[0]).to match(/as="style"/)
+      expect(@asset_preload_links[0]).to match(/as="style"/)
-      expect(@links_to_preload[1]).to match(/as="script"/)
+      expect(@asset_preload_links[1]).to match(/as="script"/)
     end
   end
 

spec/requests/application_controller_spec.rb

@@ -1188,60 +1188,50 @@ RSpec.describe ApplicationController do
     end
   end
 
-  describe "Link header" do
+  describe "Early hint header" do
-    describe "when `experimental_preconnect_link_header` site setting is enabled" do
+    before { global_setting :cdn_url, "https://cdn.example.com/something" }
-      before { SiteSetting.experimental_preconnect_link_header = true }
 
-      it "should include the `preconnect` and `dns-prefetch` resource hints in the Link header when `GlobalSetting.cdn_url is configured`" do
+    it "is not included by default" do
-        global_setting :cdn_url, "https://cdn.example.com/something"
+      get "/latest"
+      expect(response.status).to eq(200)
+      expect(response.headers["Link"]).to eq(nil)
+    end
 
+    context "when in preconnect mode" do
+      before { global_setting :early_hint_header_mode, "preconnect" }
+
+      it "includes the preconnect hint" do
         get "/latest"
-
         expect(response.status).to eq(200)
-
+        expect(response.headers["Link"]).to include("<https://cdn.example.com>; rel=preconnect")
-        expect(response.headers["Link"]).to include(
+        expect(response.headers["Link"]).not_to include("rel=preload")
-          "<https://cdn.example.com>; rel=preconnect, <https://cdn.example.com>; rel=dns-prefetch",
-        )
       end
 
-      it "should include the `preconnect` and `dns-prefetch` resource hints in the Link header when `SiteSetting.s3_cdn_url is configured`" do
+      it "can use a different header" do
-        SiteSetting.s3_cdn_url = "https://s3.some-cdn.com/something"
+        global_setting :early_hint_header_name, "X-Discourse-Early-Hint"
-
         get "/latest"
-
         expect(response.status).to eq(200)
-
+        expect(response.headers["X-Discourse-Early-Hint"]).to include(
-        expect(response.headers["Link"]).to include(
+          "<https://cdn.example.com>; rel=preconnect",
-          "<https://s3.some-cdn.com>; rel=preconnect, <https://s3.some-cdn.com>; rel=dns-prefetch",
         )
+        expect(response.headers["Link"]).to eq(nil)
+      end
+
+      it "is skipped for non-app URLs" do
+        get "/latest.json"
+        expect(response.status).to eq(200)
+        expect(response.headers["Link"]).to eq(nil)
       end
     end
 
-    context "when `GlobalSetting.preload_link_header` is enabled" do
+    context "when in preload mode" do
-      before { global_setting :preload_link_header, true }
+      before { global_setting :early_hint_header_mode, "preload" }
 
-      it "should have the Link header with assets on full page requests" do
+      it "includes the preload hint" do
-        get("/latest")
+        get "/latest"
-        expect(response.headers).to include("Link")
+        expect(response.status).to eq(200)
-      end
+        expect(response.headers["Link"]).to include('.js>; rel="preload"')
-
+        expect(response.headers["Link"]).to include('.css?__ws=test.localhost>; rel="preload"')
-      it "shouldn't have the Link header on xhr api requests" do
-        get("/latest.json")
-        expect(response.headers).not_to include("Link")
-      end
-    end
-
-    context "when `GlobalSetting.preload_link_header` is disabled" do
-      before { global_setting :preload_link_header, false }
-
-      it "shouldn't have the Link header with assets on full page requests" do
-        get("/latest")
-        expect(response.headers).not_to include("Link")
-      end
-
-      it "shouldn't have the Link header on xhr api requests" do
-        get("/latest.json")
-        expect(response.headers).not_to include("Link")
       end
     end
   end