DEV: Remove legacy `/brotli_asset` workaround (#24243)

When Discourse first introduced brotli support, reverse-proxy/CDN support for passing through the accept-encoding header to our NGINX server was very poor. Therefore, a separate `/brotli_assets/...` path was introduced to serve the brotli assets. This worked well, but introduces additional complexity and inconsistencies. Nowadays, Brotli encoding is well supported, so we don't need the separate paths any more. Requests can be routed to the asset `.js` URLs, and NGINX will serve the brotli/gzip version of the asset automatically.
2023-11-06 15:57:00 +00:00 · 2023-11-06 15:57:00 +00:00 · c5e6e271a5
parent 90efdd7f9d
commit c5e6e271a5
7 changed files with 4 additions and 125 deletions
--- a/app/controllers/static_controller.rb
+++ b/app/controllers/static_controller.rb
@ -3,14 +3,11 @@
 class StaticController < ApplicationController
  skip_before_action :check_xhr, :redirect_to_login_if_required
  skip_before_action :verify_authenticity_token,
-                     only: %i[brotli_asset cdn_asset enter favicon service_worker_asset]
-  skip_before_action :preload_json,
-                     only: %i[brotli_asset cdn_asset enter favicon service_worker_asset]
-  skip_before_action :handle_theme,
-                     only: %i[brotli_asset cdn_asset enter favicon service_worker_asset]
+                     only: %i[cdn_asset enter favicon service_worker_asset]
+  skip_before_action :preload_json, only: %i[cdn_asset enter favicon service_worker_asset]
+  skip_before_action :handle_theme, only: %i[cdn_asset enter favicon service_worker_asset]

-  before_action :apply_cdn_headers,
-                only: %i[brotli_asset cdn_asset enter favicon service_worker_asset]
+  before_action :apply_cdn_headers, only: %i[cdn_asset enter favicon service_worker_asset]

  PAGES_WITH_EMAIL_PARAM = %w[login password_reset signup]
  MODAL_PAGES = %w[password_reset signup]
@ -190,16 +187,6 @@ class StaticController < ApplicationController
    end
  end

-  def brotli_asset
-    is_asset_path
-
-    if params[:path].end_with?(".map")
-      serve_asset
-    else
-      serve_asset(".br") { response.headers["Content-Encoding"] = "br" }
-    end
-  end
-
  def cdn_asset
    is_asset_path

--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@ -125,9 +125,6 @@ module ApplicationHelper
          path = path.gsub(/\.([^.]+)\z/, '.gz.\1')
        end
      end
-    elsif GlobalSetting.cdn_url&.start_with?("https") && is_brotli_req? &&
-          Rails.env != "development"
-      path = path.gsub("#{GlobalSetting.cdn_url}/assets/", "#{GlobalSetting.cdn_url}/brotli_asset/")
    end

    path
--- a/config/routes.rb
+++ b/config/routes.rb
@ -1469,11 +1469,6 @@ Discourse::Application.routes.draw do
        :constraints => {
          format: /.*/,
        }
-    get "brotli_asset/*path" => "static#brotli_asset",
-        :format => false,
-        :constraints => {
-          format: /.*/,
-        }

    get "favicon/proxied" => "static#favicon", :format => false

--- a/lib/content_security_policy/default.rb
+++ b/lib/content_security_policy/default.rb
@ -31,7 +31,6 @@ class ContentSecurityPolicy
    SCRIPT_ASSET_DIRECTORIES = [
      # [dir, can_use_s3_cdn, can_use_cdn, for_worker]
      ["/assets/", true, true, true],
-      ["/brotli_asset/", true, true, true],
      ["/extra-locales/", false, false, false],
      ["/highlight-js/", false, true, false],
      ["/javascripts/", false, true, true],
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@ -28,15 +28,6 @@ RSpec.describe ApplicationHelper do
      expect(helper.include_crawler_content?).to eq(false)
    end

-    it "provides brotli links to brotli cdn" do
-      set_cdn_url "https://awesome.com"
-
-      helper.request.env["HTTP_ACCEPT_ENCODING"] = "br"
-      link = helper.preload_script("start-discourse")
-
-      expect(link).to eq(script_tag("https://awesome.com/brotli_asset/start-discourse.js"))
-    end
-
    context "with s3 CDN" do
      before do
        global_setting :s3_bucket, "test_bucket"
--- a/spec/lib/content_security_policy_spec.rb
+++ b/spec/lib/content_security_policy_spec.rb
@ -47,7 +47,6 @@ RSpec.describe ContentSecurityPolicy do
        %w[
          'self'
          http://test.localhost/assets/
-          http://test.localhost/brotli_asset/
          http://test.localhost/javascripts/
          http://test.localhost/plugins/
        ],
@ -64,7 +63,6 @@ RSpec.describe ContentSecurityPolicy do
          http://test.localhost/sidekiq/
          http://test.localhost/mini-profiler-resources/
          http://test.localhost/assets/
-          http://test.localhost/brotli_asset/
          http://test.localhost/extra-locales/
          http://test.localhost/highlight-js/
          http://test.localhost/javascripts/
@ -118,7 +116,6 @@ RSpec.describe ContentSecurityPolicy do
      expect(script_srcs).to include(
        *%w[
          https://cdn.com/assets/
-          https://cdn.com/brotli_asset/
          https://cdn.com/highlight-js/
          https://cdn.com/javascripts/
          https://cdn.com/plugins/
@ -133,7 +130,6 @@ RSpec.describe ContentSecurityPolicy do
      expect(script_srcs).to include(
        *%w[
          https://s3-cdn.com/assets/
-          https://s3-cdn.com/brotli_asset/
          https://cdn.com/highlight-js/
          https://cdn.com/javascripts/
          https://cdn.com/plugins/
@ -148,7 +144,6 @@ RSpec.describe ContentSecurityPolicy do
      expect(script_srcs).to include(
        *%w[
          https://s3-asset-cdn.com/assets/
-          https://s3-asset-cdn.com/brotli_asset/
          https://cdn.com/highlight-js/
          https://cdn.com/javascripts/
          https://cdn.com/plugins/
@ -166,7 +161,6 @@ RSpec.describe ContentSecurityPolicy do
      expect(script_srcs).to include(
        *%w[
          https://cdn.com/forum/assets/
-          https://cdn.com/forum/brotli_asset/
          https://cdn.com/forum/highlight-js/
          https://cdn.com/forum/javascripts/
          https://cdn.com/forum/plugins/
@ -181,7 +175,6 @@ RSpec.describe ContentSecurityPolicy do
      expect(script_srcs).to include(
        *%w[
          https://s3-cdn.com/assets/
-          https://s3-cdn.com/brotli_asset/
          https://cdn.com/forum/highlight-js/
          https://cdn.com/forum/javascripts/
          https://cdn.com/forum/plugins/
--- a/spec/requests/static_controller_spec.rb
+++ b/spec/requests/static_controller_spec.rb
@ -57,89 +57,6 @@ RSpec.describe StaticController do
    end
  end

-  describe "#brotli_asset" do
-    it "returns a non brotli encoded 404 if asset is missing" do
-      get "/brotli_asset/missing.js"
-
-      expect(response.status).to eq(404)
-      expect(response.headers["Content-Encoding"]).not_to eq("br")
-      expect(response.headers["Cache-Control"]).to match(/max-age=1/)
-    end
-
-    it "can handle fallback brotli assets" do
-      begin
-        assets_path = Rails.root.join("tmp/backup_assets")
-
-        GlobalSetting.stubs(:fallback_assets_path).returns(assets_path.to_s)
-
-        FileUtils.mkdir_p(assets_path)
-
-        file_path = assets_path.join("test.js.br")
-        File.write(file_path, "fake brotli file")
-
-        get "/brotli_asset/test.js"
-
-        expect(response.status).to eq(200)
-        expect(response.headers["Cache-Control"]).to match(/public/)
-      ensure
-        File.delete(file_path)
-      end
-    end
-
-    it "has correct headers for brotli assets" do
-      begin
-        assets_path = Rails.root.join("public/assets")
-
-        FileUtils.mkdir_p(assets_path)
-
-        file_path = assets_path.join("test.js.br")
-        File.write(file_path, "fake brotli file")
-
-        get "/brotli_asset/test.js"
-
-        expect(response.status).to eq(200)
-        expect(response.headers["Cache-Control"]).to match(/public/)
-      ensure
-        File.delete(file_path)
-      end
-    end
-
-    it "has correct cors headers for brotli assets" do
-      begin
-        assets_path = Rails.root.join("public/assets")
-
-        FileUtils.mkdir_p(assets_path)
-
-        file_path = assets_path.join("test.js.br")
-        File.write(file_path, "fake brotli file")
-        GlobalSetting.stubs(:cdn_url).returns("https://www.example.com/")
-
-        get "/brotli_asset/test.js"
-
-        expect(response.status).to eq(200)
-        expect(response.headers["Access-Control-Allow-Origin"]).to match("*")
-      ensure
-        File.delete(file_path)
-      end
-    end
-
-    it "can serve sourcemaps on adjacent paths" do
-      assets_path = Rails.root.join("public/assets")
-
-      FileUtils.mkdir_p(assets_path)
-
-      file_path = assets_path.join("test.map")
-      File.write(file_path, "fake source map")
-      GlobalSetting.stubs(:cdn_url).returns("https://www.example.com/")
-
-      get "/brotli_asset/test.map"
-
-      expect(response.status).to eq(200)
-    ensure
-      File.delete(file_path)
-    end
-  end
-
  describe "#cdn_asset" do
    let (:site) {
      RailsMultisite::ConnectionManagement.current_db