feat(bundler): checksum validation for bundler - #1655
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
This comment was marked as resolved.
This comment was marked as resolved.
183d763 to
22adc46
Compare
|
@Harsh-2005d Could you verify the integration test is passing? By temporary changing the repo-url for bundler/e2e. |
there are assertion errors in test data for some packages, and if i regenerate test data from my repo url it introduces some unintended changes . "pkg:gem/quux@0.0.1?vcs_url=git%2Bhttps://github.com/Harsh-2005d/integration-tests.git%40515cd9a5f1a688fdaeda58b50822ce266fe19025#quux", |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7ddc213-1f87-4d68-9139-35e3fe507209
📒 Files selected for processing (4)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/parser.pyhermeto/core/package_managers/bundler/scripts/lockfile_parser.rbtests/unit/package_managers/bundler/test_parser.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hermeto/core/package_managers/bundler/main.py (2)
71-74: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake comments state rationale.
These comments restate aliases, types, or assignments. Remove them when the code is self-explanatory. Otherwise, state why the alias, environment variable, or tuple list is necessary.
Also applies to: 269-272, 290-291
Source: Coding guidelines
167-167: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the local variable annotation.
Annotate
git_pathsaslist[tuple[DepName, FSDepName, DepURL]]. This is new Python code and must include type annotations.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90cefa8b-910c-48ad-86af-25b2ed3aa515
⛔ Files ignored due to path filters (2)
tests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfile.lockis excluded by!**/*.locktests/integration/bundler/scenarios/bundler_e2e_ruby33/in/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/parser.pyhermeto/core/package_managers/bundler/scripts/lockfile_parser.rbtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Containerfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/README.mdtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/tmp.gemspectests/integration/bundler/test_bundler.py
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a2612b2-949d-4c0d-b3bc-b41882580e8d
⛔ Files ignored due to path filters (2)
tests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfile.lockis excluded by!**/*.locktests/integration/bundler/scenarios/bundler_e2e_ruby33/in/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
hermeto/core/package_managers/bundler/main.pytests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Containerfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/README.mdtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/tmp.gemspectests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.jsontests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.jsontests/integration/bundler/test_bundler.py
9ffa1cb to
bc70ad0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.json (1)
47-47:⚠️ Potential issue | 🟠 MajorRevert the temporary VCS references in both generated BOMs.
The checksum change must not change the
quuxortmpcommit identities. Restore the commits expected by each scenario lockfile, then regenerate both snapshots. This repeats the previous review finding.
tests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.json#L47-L47: restore thequuxannotation subject.tests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.json#L60-L60: restore thetmpannotation subject.tests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.json#L558-L566: restore thequuxcomponent PURL.tests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.json#L727-L735: restore thetmpcomponent PURL.tests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.json#L47-L47: restore thequuxannotation subject.tests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.json#L60-L60: restore thetmpannotation subject.tests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.json#L558-L566: restore thequuxcomponent PURL.tests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.json#L727-L735: restore thetmpcomponent PURL.hermeto/core/package_managers/bundler/scripts/lockfile_parser.rb (1)
14-14:⚠️ Potential issue | 🟠 MajorKeep optional checksum parsing nil-safe.
checksum_store.to_lock(spec)can returnnilwhenGemfile.lockhas noCHECKSUMSentry. The[]call executes before&.sub, sonil[/sha[^, ]*/]raisesNoMethodErrorand prevents the parser from emitting an empty checksum map. The line also assumes that the running Bundler exposeschecksum_store. Guard both cases, or enforce and document the minimum Bundler version.Proposed fix
- checksum = spec.source.checksum_store.to_lock(spec)[/sha[^, ]*/]&.sub("=",":") + lock_checksum = + if spec.source.respond_to?(:checksum_store) + spec.source.checksum_store&.to_lock(spec) + end + checksum = lock_checksum&.slice(/sha[^, ]*/)&.sub("=", ":")tests/unit/package_managers/bundler/test_parser.py (1)
154-154:⚠️ Potential issue | 🟠 MajorAdd checksum propagation test cases.
The empty
"checksums": {}values only keep existing fixtures parseable. They do not verify thatparse_lockfileselects the"ruby"checksum or the binary platform checksum and passes it to the correct model. Add new test cases with asha256:<hexdigest>value and assert bothGemDependency.checksumandGemPlatformSpecificDependency.checksum.As per coding guidelines, add new test cases instead of modifying existing ones and aim for near-full coverage of new code.
Also applies to: 303-303
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4f2e191-83b2-4267-b416-ece57d61db1b
⛔ Files ignored due to path filters (2)
tests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfile.lockis excluded by!**/*.locktests/integration/bundler/scenarios/bundler_e2e_ruby33/in/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/parser.pyhermeto/core/package_managers/bundler/scripts/lockfile_parser.rbtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Containerfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/README.mdtests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.jsontests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.jsontests/integration/bundler/test_bundler.pytests/unit/package_managers/bundler/test_parser.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
hermeto/core/package_managers/bundler/parser.py (1)
62-75:⚠️ Potential issue | 🟠 MajorPersist the checksum in the component PURL.
These constructors set
checksum, butGemDependency.purlcurrently creates a barePackageURL. Generated components therefore omit the checksum qualifier even when the lockfile provides a checksum.hermeto/core/package_managers/bundler/scripts/lockfile_parser.rb (1)
14-14:⚠️ Potential issue | 🟠 MajorRestore the Bundler compatibility guard.
Line 14 calls
checksum_storeunconditionally. A Bundler runtime without this API, or a missing lock entry, can stop lockfile parsing withNoMethodError.#!/bin/bash set -euo pipefail rg -n -C 3 'checksum_store|BUNDLED WITH|bundler' \ hermeto tests \ -g '*.rb' -g 'Gemfile.lock' -g 'Containerfile'hermeto/core/package_managers/bundler/main.py (1)
156-158:⚠️ Potential issue | 🟠 MajorMark unchecked dependencies in the SBOM.
This branch only logs that verification was skipped. The later component conversion does not receive any missing-checksum state, so consumers cannot distinguish unchecked dependencies.
As per coding guidelines, “mark dependencies without user-provided checksums in the SBOM.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70640966-5844-4df7-bc9d-a532d8bcc8fd
⛔ Files ignored due to path filters (2)
tests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfile.lockis excluded by!**/*.locktests/integration/bundler/scenarios/bundler_e2e_ruby33/in/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/parser.pyhermeto/core/package_managers/bundler/scripts/lockfile_parser.rbtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Containerfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Gemfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/README.mdtests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.jsontests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.jsontests/integration/bundler/test_bundler.pytests/unit/package_managers/bundler/test_parser.py
|
@slimreaper35 @eskultety ready for review. Thanks. |
|
@Harsh-2005d Functionally, it looks good to me, but I am sure @eskultety will point out that commit 2/3 does not pass integration tests because of SBOM mismatch that is fixed in commit 3/3. |
Yes, I will :) . |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hermeto/core/package_managers/bundler/main.py (1)
155-157: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMark unchecked dependencies in the SBOM.
This branch only logs and skips verification. The supplied
GemDependency.purlimplementation has no checksum qualifier, and the visible flow does not show a missing-checksum SBOM property. Add the required marker or verify thatto_component()adds it.As per coding guidelines, “mark dependencies without user-provided checksums in the SBOM.”
#!/bin/bash set -euo pipefail rg -n -C 6 'def to_component|def purl|checksum|properties|hashes' hermeto/core/package_managers/bundler hermeto/coreSource: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3153bcb5-5151-43a4-a029-8e4df0ab0fe8
📒 Files selected for processing (7)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/scripts/lockfile_parser.rbtests/integration/bundler/scenarios/bundler_checksum_mismatch/in/Containerfiletests/integration/bundler/scenarios/bundler_checksum_mismatch/in/README.mdtests/integration/bundler/scenarios/bundler_e2e_ruby33/out/bom.jsontests/integration/bundler/scenarios/bundler_e2e_ruby40/out/bom.jsontests/unit/package_managers/bundler/test_parser.py
|
New changes update the regex matching and add support for multiple checksums for Bundler hash verification. Although Bundler currently does not configure multiple checksum algorithms, this keeps the implementation future-proof if support is added later. |
ea3e415 to
4d3c207
Compare
|
@Harsh-2005d Can you rebase on main to pick-up a change to the CI? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/package_managers/bundler/test_parser.py (2)
157-212: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the no-checksum parser path.
The new assertions cover only populated checksum values. Add a dedicated case with an empty
checksumsmapping. Assertchecksum is Nonefor regular and platform-specific dependencies. This verifies optionalCHECKSUMSsupport before download verification.As per coding guidelines, “Aim for near-full coverage of new code.”
Also applies to: 322-354
Source: Coding guidelines
73-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the new Ruff warnings from the mock callback.
Ruff reports
argsas unused and rejectsAnyin*argsand**kwargs. Rename the unused capture and useobjector the project-approved callback protocol.Proposed fix
- def _assert_bundle_is_hidden(*args: Any, **kwargs: Any) -> subprocess.CompletedProcess[str]: + def _assert_bundle_is_hidden( + *_args: object, **kwargs: object + ) -> subprocess.CompletedProcess[str]:Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7dfc5a32-02f0-422d-9b6e-74f2b945e9fd
📒 Files selected for processing (3)
hermeto/core/package_managers/bundler/main.pyhermeto/core/package_managers/bundler/parser.pytests/unit/package_managers/bundler/test_parser.py
eskultety
left a comment
eskultety
left a comment
There was a problem hiding this comment.
A couple more nitpicks, but ACK.
Update lockfile_parser.rb to extract checksums Verify downloaded .gem files against lockfile checksums warn and skip verification when no checksum is present Assisted-by: Claude Signed-off-by: Harsh Dahiya <dahiyaharsh2005@gmail.com>
The test verifies that Hermeto correctly fails when given a malformed checksum for a gem. Signed-off-by: Harsh Dahiya <dahiyaharsh2005@gmail.com>
Updated the e2e test to verify checksum handling Signed-off-by: Harsh Dahiya <dahiyaharsh2005@gmail.com>
5 participants
Description:
Closes #1652
Summary
The
checksumfield onGemDependencywas alwaysNone— the lockfile parser never populated it and it was never used for verification. This PR wires up the full checksum pipeline for RubyGems dependencies.Changes
lockfile_parser.rbCHECKSUMSsection viaspec.source.checksum_store.to_lock(spec)sha256:<hexdigest>format in the JSON outputgem_models.pyGemDependency.purlfor SBOM outputmain.py.gemfiles, verify each against its lockfile checksum usingmust_match_any_checksum