From 56493da31af114cac4d5954a78bab0331bf3f6e2 Mon Sep 17 00:00:00 2001 From: Felix Livni Date: Wed, 13 Sep 2023 17:12:22 -0700 Subject: [PATCH] Fix fingerprint fallback when filename collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a fix for: https://github.com/rails/sprockets/issues/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. --- lib/sprockets/server.rb | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/lib/sprockets/server.rb b/lib/sprockets/server.rb index c32ce2228..edd313c6f 100644 --- a/lib/sprockets/server.rb +++ b/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