Turn Propshaft::Server into a proper middleware

Fix: rails/rails#55508

Mounting Propshaft::Server in the Rails router isn't suitable
because it end up executed after a long list of middlewares that
may set cookies or do other transformation we don't want.

In production, propshaft assets are served by `ActionDispatch::Static`
(or a reverse proxy), so we should try to be as close to that as possible.

Author: byroot

Gemfile.lock

@@ -212,4 +212,4 @@ DEPENDENCIES
   rake
 
 BUNDLED WITH
-   2.5.3
+   2.7.1

lib/propshaft/assembly.rb

@@ -34,8 +34,11 @@ def resolver
     end
   end
 
-  def server
-    Propshaft::Server.new(self)
+  def prefix
+    @prefix ||= begin
+      prefix = config.prefix || "/"
+      prefix.end_with?("/") ? prefix : "#{prefix}/"
+    end
   end
 
   def processor

lib/propshaft/railtie.rb

@@ -42,14 +42,6 @@ class Railtie < ::Rails::Railtie
         Pathname.new(File.join(app.config.paths["public"].first, app.config.assets.prefix))
       config.assets.manifest_path ||= config.assets.output_path.join(".manifest.json")
 
-      app.assets = Propshaft::Assembly.new(app.config.assets)
-
-      if config.assets.server
-        app.routes.prepend do
-          mount app.assets.server, at: app.assets.config.prefix
-        end
-      end
-
       ActiveSupport.on_load(:action_view) do
         include Propshaft::Helper
       end
@@ -71,6 +63,13 @@ class Railtie < ::Rails::Railtie
       end
     end
 
+    initializer "propshaft.assets_middleware", group: :all do |app|
+      app.assets = Propshaft::Assembly.new(app.config.assets)
+      if config.assets.server
+        app.middleware.insert_after ::ActionDispatch::Static, Propshaft::Server, app.assets
+      end
+    end
+
     rake_tasks do
       load "propshaft/railties/assets.rake"
     end

lib/propshaft/server.rb

@@ -2,30 +2,39 @@
 require "rack/version"
 
 class Propshaft::Server
-  def initialize(assembly)
+  def initialize(app, assembly)
+    @app = app
     @assembly = assembly
   end
 
   def call(env)
     execute_cache_sweeper_if_updated
-    path, digest = extract_path_and_digest(env)
-
-    if (asset = @assembly.load_path.find(path)) && asset.fresh?(digest)
-      compiled_content = asset.compiled_content
-
-      [
-        200,
-        {
-          Rack::CONTENT_LENGTH  => compiled_content.length.to_s,
-          Rack::CONTENT_TYPE    => asset.content_type.to_s,
-          VARY                  => "Accept-Encoding",
-          Rack::ETAG            => "\"#{asset.digest}\"",
-          Rack::CACHE_CONTROL   => "public, max-age=31536000, immutable"
-        },
-        [ compiled_content ]
-      ]
+
+    path = env["PATH_INFO"]
+    method = env["REQUEST_METHOD"]
+
+    if (method == "GET" || method == "HEAD") && path.start_with?(@assembly.prefix)
+      path, digest = extract_path_and_digest(path)
+
+      if (asset = @assembly.load_path.find(path)) && asset.fresh?(digest)
+        compiled_content = asset.compiled_content
+
+        [
+          200,
+          {
+            Rack::CONTENT_LENGTH  => compiled_content.length.to_s,
+            Rack::CONTENT_TYPE    => asset.content_type.to_s,
+            VARY                  => "Accept-Encoding",
+            Rack::ETAG            => "\"#{asset.digest}\"",
+            Rack::CACHE_CONTROL   => "public, max-age=31536000, immutable"
+          },
+          method == "HEAD" ? [] : [ compiled_content ]
+        ]
+      else
+        [ 404, { Rack::CONTENT_TYPE => "text/plain", Rack::CONTENT_LENGTH => "9" }, [ "Not found" ] ]
+      end
     else
-      [ 404, { Rack::CONTENT_TYPE => "text/plain", Rack::CONTENT_LENGTH => "9" }, [ "Not found" ] ]
+      @app.call(env)
     end
   end
 
@@ -34,10 +43,11 @@ def inspect
   end
 
   private
-    def extract_path_and_digest(env)
-      full_path = Rack::Utils.unescape(env["PATH_INFO"].to_s.sub(/^\//, ""))
+    def extract_path_and_digest(path)
+      path = path.delete_prefix(@assembly.prefix)
+      path = Rack::Utils.unescape(path)
 
-      Propshaft::Asset.extract_path_and_digest(full_path)
+      Propshaft::Asset.extract_path_and_digest(path)
     end
 
     if Gem::Version.new(Rack::RELEASE) < Gem::Version.new("3")

test/propshaft/server_test.rb

@@ -5,44 +5,70 @@
 class Propshaft::ServerTest < ActiveSupport::TestCase
   include Rack::Test::Methods
 
+  class RackApp
+    attr_reader :calls
+
+    def initialize
+      @calls = []
+    end
+
+    def call(env)
+      @calls << env
+      [200, {}, ["OK"]]
+    end
+  end
+
   setup do
     @assembly = Propshaft::Assembly.new(ActiveSupport::OrderedOptions.new.tap { |config|
       config.paths = [Pathname.new("#{__dir__}/../fixtures/assets/vendor"), Pathname.new("#{__dir__}/../fixtures/assets/first_path")]
       config.output_path = Pathname.new("#{__dir__}../fixtures/output")
+      config.prefix = "/assets"
     })
 
+    @rack_app = RackApp.new
     @assembly.compilers.register "text/css", Propshaft::Compiler::CssAssetUrls
-    @server = Propshaft::Server.new(@assembly)
+    @server = Propshaft::Server.new(@rack_app, @assembly)
+  end
+
+  test "forward requests not under prefix" do
+    get "/test"
+    assert_not_empty @rack_app.calls
+  end
+
+  test "forward requests that aren't GET or HEAD" do
+    asset = @assembly.load_path.find("foobar/source/test.css")
+    post "/assets/#{asset.digested_path}"
+    assert_not_empty @rack_app.calls
   end
 
   test "serve a compiled file" do
     asset = @assembly.load_path.find("foobar/source/test.css")
-    get "/#{asset.digested_path}"
+    get "/assets/#{asset.digested_path}"
 
     assert_equal 200, last_response.status
-    assert_equal "62", last_response.headers['content-length']
+    assert_equal last_response.body.bytesize.to_s, last_response.headers['content-length']
     assert_equal "text/css", last_response.headers['content-type']
     assert_equal "Accept-Encoding", last_response.headers['vary']
     assert_equal "\"#{asset.digest}\"", last_response.headers['etag']
     assert_equal "public, max-age=31536000, immutable", last_response.headers['cache-control']
-    assert_equal ".hero { background: url(\"/foobar/source/file-3e6a1297.jpg\") }\n",
+    assert_equal ".hero { background: url(\"/assets/foobar/source/file-3e6a1297.jpg\") }\n",
                  last_response.body
   end
 
   test "serve a predigested file" do
     asset = @assembly.load_path.find("file-already-abcdefVWXYZ0123456789_-.digested.css")
-    get "/#{asset.digested_path}"
+    get "/assets/#{asset.digested_path}"
     assert_equal 200, last_response.status
   end
 
   test "serve a sourcemap" do
     asset = @assembly.load_path.find("file-is-a-sourcemap.js.map")
-    get "/#{asset.digested_path}"
+    get "/assets/#{asset.digested_path}"
     assert_equal 200, last_response.status
   end
 
   test "not found" do
-    get "/not-found.js"
+    get "/assets/not-found.js"
 
     assert_equal 404, last_response.status
     assert_equal "9", last_response.headers['content-length']
@@ -55,7 +81,7 @@ class Propshaft::ServerTest < ActiveSupport::TestCase
 
   test "not found if digest does not match" do
     asset = @assembly.load_path.find("foobar/source/test.css")
-    get "/#{asset.logical_path}"
+    get "/assets/#{asset.logical_path}"
     assert_equal 404, last_response.status
   end