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

Tighten fingerprint detection regex to only accept hexadecimal. #795

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

Conversation

botandrose
Copy link
Contributor

Simple fix for #749

Unless, of course, some fingerprints are not hexadecimal? Is that even a thing? If it is, do we want to support it?

@brenogazzola
Copy link
Contributor

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

@botandrose
Copy link
Contributor Author

@brenogazzola Ah, I see. I couldn't find specific documentation on esbuild's hash format, but I did find this: evanw/esbuild#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:

  1. This wasn't actually a bugfix, but a feature addition. And as it turns out, a backwards-incompatible feature addition.
  2. Supporting people using Sprockets directly should take priority over playing nice with 3rd party alternatives.

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.

@botandrose
Copy link
Contributor Author

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!

@botandrose
Copy link
Contributor Author

botandrose commented Nov 14, 2023

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
}

@botandrose
Copy link
Contributor Author

botandrose commented Nov 14, 2023

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.

@nubigram
Copy link

nubigram commented Apr 2, 2024

Hello, also the same problem, all the js files are broken. some help? I got lost from searching so much

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.

3 participants