Open botandrose opened 8 months ago
I think (though I'm not sure) this has to remain like this because esbuild uses base32 when generating fingerprints. This line predates it, but we've had to change some other parts of sprockets/propshaft that handle fingerprint from hexa to base32 because of this
@brenogazzola Ah, I see. I couldn't find specific documentation on esbuild's hash format, but I did find this: https://github.com/evanw/esbuild/issues/1529 , which definitely fits with what you're describing. That more permissive regex makes sense now.
Looking a bit deeper, it appears this behavior was added in the 4.0.3 bugfix release: https://github.com/rails/sprockets/blob/main/CHANGELOG.md#403 described: "Allow assets already fingerprinted to be served through Sprockets::Server"
So, I'd like to make the case that:
That said, how would we feel about putting this behavior behind an off-by-default config flag? It could become the default in Sprockets 5.0, but as it stands, I believe its breaking semver guarantees.
For context, my motivation is that this seemingly innocuous feature addition resulted in broken images making it to production.
My designer likes to kebab-case his image filenames, and he added this to the homepage of a project <picture><source srcset="/assets/art-and-space-staging.webp"></picture>
, forgetting to use the asset_path
helper. This both worked in development mode and passed CI, because Sprockets::Server
runs in those environments, and with this new feature, it was happy to serve this file without the fingerprint, because its interpretting "staging" as a valid fingerprint. It was only in production, where config.assets.compile = false
, where the image correctly resulted in a 404.
I have a hotfix monkeypatch tightening the fingerprint regex and returning 404 if one doesn't exist, but I hope you can agree with me that this is not an ideal situation!
Oh, wow, and I just realized that this is the same issue that I've been running into with importmaps-rails, lately, as well!
Given the file app/javascripts/lib/utils.js
// app/javascripts/controllers/thing_controller.js
import Utils from "../lib/utils" // rather than from "lib/utils"
This works in dev and CI, but results in broken JS in production! I thought I was just holding it wrong because JavaScript is often quirky. But it was this same Sprockets bug all along!
This is particularly interesting, because the path doesn't contain anything that could be interpreted as a fingerprint (even by esbuild standards). However, its the same result: an HTTP request to /assets/lib/utils
yields the asset in dev and CI, but 404s in production. And additionally, this issue disappears when I include my same hotfix that 404s when a fingerprint is not present:
require "sprockets/server"
Sprockets::Server.prepend Module.new {
def call(env)
path = Rack::Utils.unescape(env['PATH_INFO'].to_s.sub(/^\//, ''))
return not_found_response(env) unless path_fingerprint(path)
super
end
private
def path_fingerprint(path)
path[/-([0-9a-f]{7,128})\.[^.]+\z/, 1]
end
}
So perhaps a conservative alternative could be a new configuration flag e.g. config.assets.compiler_require_fingerprint = true
that 404s when a fingerprint is absent. Although that wouldn't solve the issue with the kebab-cased image file names. Maybe true
for default fingerprint format, and also accept a Regex, e.g. config.assets.compiler_require_fingerprint = /\Aa-z0-f{64}\z/
.
Or maybe accept symbols as well for easy shorthand: config.assets.external_fingerprinters << :esbuild
. which would enable the more permissive regex.
Just spit-balling here. That's a lot out of me, so I look forward to hearing your responses.
Hello, also the same problem, all the js files are broken. some help? I got lost from searching so much
Simple fix for https://github.com/rails/sprockets/issues/749
Unless, of course, some fingerprints are not hexadecimal? Is that even a thing? If it is, do we want to support it?