From c5e6e271a582b2627b6ba3caeba070cfc72974da Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 6 Nov 2023 15:57:00 +0000 Subject: [PATCH] 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. --- app/controllers/static_controller.rb | 21 ++---- app/helpers/application_helper.rb | 3 - config/routes.rb | 5 -- lib/content_security_policy/default.rb | 1 - spec/helpers/application_helper_spec.rb | 9 --- spec/lib/content_security_policy_spec.rb | 7 -- spec/requests/static_controller_spec.rb | 83 ------------------------ 7 files changed, 4 insertions(+), 125 deletions(-) diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 1eb75bd8101..945b7866e7c 100644 --- 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 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 54582775217..ffcb3ee1586 100644 --- 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 diff --git a/config/routes.rb b/config/routes.rb index 267ca86dad3..f014203b4a5 100644 --- 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 diff --git a/lib/content_security_policy/default.rb b/lib/content_security_policy/default.rb index a4e6f70aefc..a606ae2c89a 100644 --- 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], diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 396dff62dda..de661a67fbe 100644 --- 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" diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index 338a2e1e085..c6c6cdc5b39 100644 --- 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/ diff --git a/spec/requests/static_controller_spec.rb b/spec/requests/static_controller_spec.rb index 102ddfa0e6a..de45c497253 100644 --- 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