Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fingerprint fallback when filename collision #793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flivni
Copy link

@flivni flivni commented Sep 14, 2023

This is a fix for: #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.

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.
# URLs containing a `".."` are rejected for security reasons.
if forbidden_request?(path)
return forbidden_response(env)
return forbidden_response(env) if forbidden_request?(path)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should also repeat:

return bad_request_response(env) unless path.valid_encoding?

But the original code did not do this so I wasn't sure whether to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant