[RubyGemsGemInstaller] Validate checksums from the compact index #4851
Conversation
|
woo @homu r+ |
|
📌 Commit 9231ceb has been approved by |
|
Ugh, of course now that I've approved it I realized that we should add a setting to disable this check, because some people will have existing gems with invalid checksums that they want to keep using. |
|
@homu r- |
|
I am super happy with this, though! |
| rescue | ||
| nil | ||
| end | ||
| CompactIndex::GemVersion.new(spec.version.version, spec.platform.to_s, checksum, nil, |
There was a problem hiding this comment.
can we just checksum = Digest::SHA256.file("#{GEM_REPO}/gems/#{spec.original_name}.gem").base64digest rescue nil? lots of lines super far indented makes me feel gross. 😝
There was a problem hiding this comment.
I don't like the modifier rescue and rubocop agrees with me :P
There was a problem hiding this comment.
how about checksum = gem_checksum(spec) then?
|
@indirect added a setting to disable validation |
|
This is failing because |
| "the checksum given by the API. This means that the contents of the " \ | ||
| "gem appear to be different from what was uploaded, and could be an indicator of a security issue.\n" \ | ||
| "(The expected SHA256 checksum was #{checksum.inspect}, but the checksum for the downloaded gem was #{digest.inspect}.)\n" \ | ||
| "Bundler cannot continue installing #{spec.name} (#{spec.version})." |
There was a problem hiding this comment.
Based on the principle of empowering users to resolve their own errors, how do you feel about also printing the full path, suggesting deleting the gem with the bad checksum, and explaining that if they are sure they want to install this gem despite the checksum not matching, they can bundle config disable.checksum_validaiton true?
|
The config key should probably also to be added to the |
|
Running a data migration right now to replace all checksums in the bundler-api database with the longer hex versions. I'm torn about what we should put in the actual index files, though... using base64 checksums would decrease the file size. :/ |
|
☔ The latest upstream changes (presumably #4869) made this pull request unmergeable. Please resolve the merge conflicts. |
segiddins
force-pushed
the
seg-validate-checksums
branch
from
9660a1e to
12a3569
Compare
|
Rebased |
segiddins
force-pushed
the
seg-validate-checksums
branch
from
12a3569 to
54baa4e
Compare
|
This should be good now that the checksums in the database are all consistent. @homu r+ |
|
📌 Commit 54baa4e has been approved by |
|
⌛ Testing commit 54baa4e with merge a238b61... |
|
💔 Test failed - status |
segiddins
force-pushed
the
seg-validate-checksums
branch
from
54baa4e to
dc2a61c
Compare
|
This should pass now |
segiddins
force-pushed
the
seg-validate-checksums
branch
from
c4f89cd to
ae465eb
Compare
|
@homu retry |
|
oops, @homu r+ |
|
📌 Commit ae465eb has been approved by |
[RubyGemsGemInstaller] Validate checksums from the compact index Closes #4464
|
☀️ Test successful - status |
Closes #4464