Fix fingerprint fallback when filename collision

This is a fix for: rails#749

The bug is when you have two assets in the same path where one looks like the fingerprint-stripped version of the other, e.g. “landing.png” and “landing-splashimage.png”. In this case, if landing-splashimage.png is requested, a 404 is returned rather than the asset.

The old behavior:
The code had logic to first strip anything that looked like a fingerprint from the filename. Then, if the asset was not found with the stripped path, it would attempt to find it with the full_path. This “fallback” was to handle the case where the code mistakenly thought a filename with a dash actually had a fingerprint. But, if the stripped path had already matched a different asset, then a 404 would be returned since the matched asset’s etag wouldn’t match the fingerprint.

The new behavior:
This PR first looks up the asset using the original path. Only in the case where the asset is not found does the code strip what might be a fingerprint from the path and re-lookup.

This new behavior fixes the bug and I think is more true to the spirit of what this code should be doing.

Other changes / side effects:
In the case where both the original asset and the fingerprinted asset exist, the new code returns the fingerprinted-asset while the old code returns the original asset. This should never result in any actual difference of bytes served.

I think this is worth fixing because if the bug is encountered, it can be deeply confusing to the uninitiated and could waste hours of time.

Author: flivni

lib/sprockets/server.rb

@@ -48,22 +48,20 @@ def call(env)
       full_path = Rack::Utils.unescape(env['PATH_INFO'].to_s.sub(/^\//, ''))
       path = full_path
 
-      unless path.valid_encoding?
-        return bad_request_response(env)
-      end
+      return bad_request_response(env) unless path.valid_encoding?
+      return forbidden_response(env) if forbidden_request?(path)
+
+      asset = find_asset(path)
 
       # Strip fingerprint
-      if fingerprint = path_fingerprint(path)
+      if asset.nil? && fingerprint = path_fingerprint(path)
         path = path.sub("-#{fingerprint}", '')
-      end
-
-      # URLs containing a `".."` are rejected for security reasons.
-      if forbidden_request?(path)
-        return forbidden_response(env)
+        return forbidden_response(env) if forbidden_request?(path)
+        asset = find_asset(path)
       end
 
       if fingerprint
-        if_match = fingerprint
+        if_match = asset&.etag
       elsif env['HTTP_IF_MATCH']
         if_match = env['HTTP_IF_MATCH'][/"(\w+)"$/, 1]
       end
@@ -72,18 +70,6 @@ def call(env)
         if_none_match = env['HTTP_IF_NONE_MATCH'][/"(\w+)"$/, 1]
       end
 
-      # Look up the asset.
-      asset = find_asset(path)
-
-      # Fallback to looking up the asset with the full path.
-      # This will make assets that are hashed with webpack or
-      # other js bundlers work consistently between production
-      # and development pipelines.
-      if asset.nil? && (asset = find_asset(full_path))
-        if_match = asset.etag if fingerprint
-        fingerprint = asset.etag
-      end
-
       if asset.nil?
         status = :not_found
       elsif fingerprint && asset.etag != fingerprint