4.0.1: New etag behavior can cause overlong URLs/tags and also nil crashes

[csuhta]

Expected behavior

Generated etags and production asset URLs should contain the file hash, but the hash should be some kind of sensible length like 64 characters.

Actual behavior

In Sprockets 4.0.1, this commit changed etag generation so that it involves the environment_version

But end-users control this variable (like with Rails.config.assets.version), and it can be any string or nil that probably won't .pack() properly.

For example, I discovered this bug because we were setting our config.assets.version to a hexadecimal string. A hash wasn't generated properly, so we got this final URL:

https://assets.scryfall.com/assets/scryfall-3164333166373436636163393336633539383766363730646563313338313633656563383138373164666330643764333837393639613665353837653932326477e737efcdb1484d1c2976acd6449d878e23b3ca3c6a3e5d66d03703376c0a8c.js

Also you can set the environment_version to nil downstream and it will blow up.

System configuration

• Sprockets version 4.0.1
• Ruby version 2.7.1p83

Example

require "sprockets"

x = Sprockets::Asset.new( 
  metadata: {
    # Example file digest
    digest: [178, 77, 101, 96, 39, 128, 253, 228, 97, 199, 237, 233, 109, 105, 141, 91, 136, 32, 238, 61, 209, 198, 92, 63, 119, 81, 200, 54, 196, 187, 5, 138].pack("c*"),
    # Our config.assets.version was a fingerprint of our own
    environment_version: "886472439e2c2da548ead23fde10451e78e590a14414cac4f79a74ec2fc125ef",
  }
)

puts x.etag
# => "38383634373234333965326332646135343865616432336664653130343531653738653539306131343431346361633466373961373465633266633132356566b24d65602780fde461c7ede96d698d5b8820ee3dd1c65c3f7751c836c4bb058a"
# This is too long, and causes very long URLs and HTTP headers downstream

x = Sprockets::Asset.new( 
  metadata: {
    # Example file digest
    digest: [178, 77, 101, 96, 39, 128, 253, 228, 97, 199, 237, 233, 109, 105, 141, 91, 136, 32, 238, 61, 209, 198, 92, 63, 119, 81, 200, 54, 196, 187, 5, 138].pack("c*"),
    # Empty environment
    environment_version: "",
  }
)

puts x.etag
# => "b24d65602780fde461c7ede96d698d5b8820ee3dd1c65c3f7751c836c4bb058a"
# This is a sensible etag

x = Sprockets::Asset.new( 
  metadata: {
    # Example file digest
    digest: [178, 77, 101, 96, 39, 128, 253, 228, 97, 199, 237, 233, 109, 105, 141, 91, 136, 32, 238, 61, 209, 198, 92, 63, 119, 81, 200, 54, 196, 187, 5, 138].pack("c*"),
    # nil environment, because I set Rails.config.assets.version = nil
    environment_version: nil,
  }
)
puts x.etag
# => NoMethodError: undefined method `+' for nil:NilClass
# => from /Users/Csuhta/.gem/ruby/2.7.1/gems/sprockets-4.0.1/lib/sprockets/asset.rb:140:in `etag'

Possible Solution

I'm not sure exactly what was desired from this change, but perhaps you should coerce the environment_version into a binary string before generating the etags?

[guillaumewrobel]

Heroku build log:

Running: rake assets:precompile
Skipping yarn:install
rake aborted!
NoMethodError: undefined method `+' for nil:NilClass
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/asset.rb:138:in `etag'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/asset.rb:67:in `block in digest_path'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/asset.rb:67:in `sub'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/asset.rb:67:in `digest_path'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/manifest.rb:175:in `block in compile'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/manifest.rb:173:in `each'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/sprockets/manifest.rb:173:in `compile'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-rails-3.2.1/lib/sprockets/rails/task.rb:68:in `block (3 levels) in define'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-4.0.1/lib/rake/sprocketstask.rb:148:in `with_logger'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/sprockets-rails-3.2.1/lib/sprockets/rails/task.rb:67:in `block (2 levels) in define'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/bugsnag-6.13.1/lib/bugsnag/integrations/rake.rb:19:in `execute'
/tmp/build_c7a3f0bf57d027565a792ff63c0334c6/vendor/bundle/ruby/2.7.0/gems/rake-13.0.1/exe/rake:27:in `<top (required)>'

[airled]

Confirm that bug. Can't run rails app after upgrading to 4.0.1. (Ruby 2.7.1, Rails 6.0.1.3)

undefined method `+' for nil:NilClass

[elv1ss]

app also crashes if Rails.application.config.assets.version = '1.0' is set

[MaximeRobion]

found this thread thanks got I was loosing my mind.
It makes my activeadmin panel crash as well, same error as above

[rafaelfranca]

Thank you for reporting. Could you please try the master branch to see if it is fixed? I'll release 4.0.2 when it is confirmed.

[rafaelfranca]

4.0.2 released

[csuhta]

Thank you for the fast turn-around on this!