Verify self-update archives against GitHub release digests before install#168
Conversation
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
📝 WalkthroughWalkthroughThe PR adds cryptographic SHA256 verification to the self-update flow, fetches trusted release metadata per tag from GitHub API, parses checksum assets, verifies downloaded archives using constant-time digest comparison, and refactors all update tests to use reusable mock helpers and validate the complete verification path. ChangesUpdate Integrity Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aish/cli/update_manager.py (1)
371-386:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent partial-download cleanup on
httpx.HTTPError.The generic
Exceptionhandler unlinksdest_path(Line 385), but thehttpx.HTTPErrorhandler does not. If_download_with_progressopensdest_pathfor writing and then a network error is raised duringiter_bytesstreaming (afteropen(...)), a partial/corrupt archive will be left indest_dir. A subsequent retry would then operate against (or have to clean up) a stale file.🛠️ Proposed fix
except httpx.HTTPError as e: self.console.print(f"[red]Download failed from CDN: {e}[/red]") + dest_path.unlink(missing_ok=True) return None except Exception as e: self.console.print(f"[red]Unexpected error during download: {e}[/red]") dest_path.unlink(missing_ok=True) return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aish/cli/update_manager.py` around lines 371 - 386, The HTTPError except block in the download logic leaves partial files behind; update the httpx.HTTPError handler in the same method where _download_with_progress and verify_archive are used to unlink dest_path (use dest_path.unlink(missing_ok=True)) before returning None, matching the cleanup currently done in the generic Exception handler so partial/corrupt archives are removed on network failures.
🧹 Nitpick comments (1)
src/aish/cli/update_manager.py (1)
300-346: ⚡ Quick winAvoid the duplicate
get_release_by_taground-trip per update.For the stable update flow, the release metadata is fetched twice for the same tag: once in
get_latest_release(Line 177), then again here viaverify_archive→get_expected_archive_sha256→get_release_by_tag. That doubles GitHub API traffic (relevant for the 60/hour unauthenticated rate limit) and adds a second network round-trip on the critical install path, with no added security benefit since both calls hit the same endpoint.Consider plumbing the already-fetched
releasedict (or just itsassets) intoverify_archive/get_expected_archive_sha256, or memoizingget_release_by_tagpertag_nameon the instance. The pre-release path (Line 154-172) similarly already has the release data and could pass it through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aish/cli/update_manager.py` around lines 300 - 346, The code currently fetches the same GitHub release twice; fix by either (A) adding an optional release/assets parameter to get_expected_archive_sha256 and updating verify_archive (and the callers like get_latest_release/pre-release flow) to pass the already-fetched release dict (or release["assets"]) so no second network call is made, or (B) implement simple memoization on the instance (e.g., self._release_cache keyed by tag_name) and have get_release_by_tag consult/store this cache; update verify_archive/get_expected_archive_sha256 to use the cached release when available. Ensure symbols touched include get_expected_archive_sha256, verify_archive, get_release_by_tag, and callers such as get_latest_release/pre-release code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/aish/cli/update_manager.py`:
- Around line 371-386: The HTTPError except block in the download logic leaves
partial files behind; update the httpx.HTTPError handler in the same method
where _download_with_progress and verify_archive are used to unlink dest_path
(use dest_path.unlink(missing_ok=True)) before returning None, matching the
cleanup currently done in the generic Exception handler so partial/corrupt
archives are removed on network failures.
---
Nitpick comments:
In `@src/aish/cli/update_manager.py`:
- Around line 300-346: The code currently fetches the same GitHub release twice;
fix by either (A) adding an optional release/assets parameter to
get_expected_archive_sha256 and updating verify_archive (and the callers like
get_latest_release/pre-release flow) to pass the already-fetched release dict
(or release["assets"]) so no second network call is made, or (B) implement
simple memoization on the instance (e.g., self._release_cache keyed by tag_name)
and have get_release_by_tag consult/store this cache; update
verify_archive/get_expected_archive_sha256 to use the cached release when
available. Ensure symbols touched include get_expected_archive_sha256,
verify_archive, get_release_by_tag, and callers such as
get_latest_release/pre-release code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99fbe0a1-4fe8-4ab1-bde6-c1b3ff1b32fe
📒 Files selected for processing (2)
src/aish/cli/update_manager.pytests/test_update_manager.py
2 participants
Motivation
sudo.latestmetadata to the authoritative GitHub release for the matching tag and ensures the downloaded archive matches a trusted SHA256 digest before it can be installed.Description
get_release_by_tag, checksum discovery, and verification helpers (_sha256_file,_parse_checksum_text,get_expected_archive_sha256,verify_archive) tosrc/aish/cli/update_manager.pyand aSHA256_PATTERNconstant andGITHUB_API_RELEASE_TAGendpoint constant.latestversion, the code now fetches the corresponding GitHub release metadata and uses its assets/checksum files as the trust root instead of accepting a version-only CDN response alone.download_releasenow verifies the downloaded archive against the expected SHA256 digest (usinghmac.compare_digest) and removes/rejects the archive if verification fails, preventing unverified installers from being returned for installation.tests/test_update_manager.pyto exercise trusted release metadata lookup, checksum-parsed digests, download verification, download-base overrides, and rejection of checksum mismatches.Testing
uv run ruff check src/aish/cli/update_manager.py tests/test_update_manager.pyanduv run black --check src/aish/cli/update_manager.py tests/test_update_manager.py, both passed after applying formatting.uv run python -m pytest tests/test_update_manager.pywhich passed (12 passed).uv run python -m pytestwhich completed but reported one unrelated failing test (tests/terminal/pty/test_pty_control_protocol.py::test_pty_manager_execute_command_honors_explicit_timeout), with the rest of the suite passing (1 failed, 631 passed, 15 skipped); this failure is not related to the update manager changes.Codex Task
Summary by CodeRabbit
New Features
Tests