Fix integrity hash #555

[Fudoshiki]

#555
rails/sprockets-rails#393

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[rafaelfranca]

We need a test that show this is broken so it doesn't regress again

[Fudoshiki]

Hello @rafaelfranca , should I make changes in current PR or this work for rails team?

[Fudoshiki]

@schneems Check this please

[rafaelfranca]

Yes, we should apply the changes to this PR

[schneems]

Do you need help writing a test?

[Fudoshiki]

Hello, no, need help with revert commit

[schneems]

What do you need to revert? Generally I use

git rebase -i HEAD~3

This will open your last 3 commits in your $EDITOR. You can remove a commit by deleting it from the file and then saving it.

[schneems]

For now though don’t worry about adding extra commits. Just worry about getting the code like we want it and a regression test and we can squash all the changes later.

[Fudoshiki]

I will revert commit and write test tomorrow, I will need help with

We change the code to make version to change the cache key but not the digest and we add a test that fails when the version is added to the digest.

[rsmything]

Hello @rafaelfranca, @schneems,
i've added checking for generated digest to the text_environment.rb, what's next?

assert @env["gallery.js"].hexdigest == Digest::SHA256.new.update(File.binread(fixture_path('default/gallery.js'))).to_s

[Fudoshiki]

what next?

[rafaelfranca]

The support to change the cache key using the version. The way it is right now it is just reverting #404, but the idea is to fix the issue while keeping the feature that #404 added.

[rsmything]

Hello, any updates?

[schneems]

The way it is right now it is just reverting #404, but the idea is to fix the issue while keeping the feature that #404 added

I think the core problem here is that we fundamentally want to recompile all assets when the version changes, however that digest is being used for an integrity hash which is not correct. I don't know if there's a straightforward way to do that.

One option could be to store the cache version in each object, then before it gets read and used, the version gets checked to see if it is valid. This is somewhat similar to how rails' "recyclable cache keys" work https://www.schneems.com/2018/10/17/cache-invalidation-complexity-rails-52-and-dalli-cache-store/.

[Fudoshiki]

I think we need revert #404 to fix worked SRI feature

[airhorns]

I think either the version needs to not be included in the digest, or the digest and the integrity hash need to be two separate things.

I can think of a couple ways forward:

• remove the version from the digest as this PR does so the integrity hash can remain a pure function of the digest. Implement asset version cycling by putting the assets version in the manifest, and then have sprockets ignore/consider the manifest invalid if the configured version is different than it, which would force a full recompile?
• put both digests and integrity hashes into the manifest so that the digest can change as the assets version changes, but the integrity hash (which must be a pure function of the asset contents) can not change. I think it'd need to be computed ahead of time and stored in the manifest, which would mean a pretty major manifest format change IIRC

Thoughts? This is pretty broken right now if you ask me.

[rafaelfranca]

Closed by 5038ad5