Skip to content

Verify release checksum before replacing lstk binary#410

Open
skyrpex wants to merge 3 commits into
mainfrom
claude/lstk-release-integrity-verify-00adda
Open

Verify release checksum before replacing lstk binary#410
skyrpex wants to merge 3 commits into
mainfrom
claude/lstk-release-integrity-verify-00adda

Conversation

@skyrpex

@skyrpex skyrpex commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What

The self-updater's direct-binary path (lstk update when not installed via Homebrew/npm) downloaded the release archive from GitHub and replaced the running executable without any integrity check. It now verifies the archive's SHA-256 against the release's published checksums.txt before extraction:

  • New parseChecksums (internal/update/checksum.go) parses the goreleaser manifest (tolerates CRLF, blank lines, * binary-mode marker; rejects malformed lines with line numbers).
  • updateBinary is now a binaryUpdater method with injectable download base URL and executable resolver, enabling unit tests against httptest servers. It fetches checksums.txt first (fail fast, 1 MiB read cap), requires an entry for the platform asset, stream-hashes the archive during download (io.MultiWriter), and aborts before extractAndReplace on mismatch.
  • Hard fail on missing checksums.txt (404), missing asset entry, malformed manifest, or hash mismatch — warn-and-continue would make the control bypassable by deleting one release asset.
  • Spinner text is now "Downloading and verifying update".

Why

Flagged on Slack (see DEVX-999): the downloaded binary was trusted without verification. goreleaser already publishes a SHA-256 checksums.txt on every release, so this needs no release-pipeline changes. Homebrew and npm update paths are unchanged — those package managers verify integrity themselves.

Notes for reviewers

  • Error surfacing reuses ErrInternal per the error-code policy in internal/output/error_code.go; the message text carries the security signal ("may be corrupted or tampered with; update aborted"). A dedicated integrity code would be an additive follow-up.
  • Signed releases (cosign keyless: goreleaser signs: block, id-token: write in the release job, sigstore verification in the updater) are deliberately out of scope — worth a separate ticket.
  • Tests: parser table tests plus an end-to-end binaryUpdater matrix (happy path, mismatch leaves the binary untouched with no temp leftovers, 404, missing entry, malformed manifest). The existing integration test TestUpdateBinaryInPlace does a real GitHub download and now exercises verification end-to-end (its spinner-text assertion was updated).

Review: security-relevant change to the self-update path — human review advisable.

Closes DEVX-999

Co-Authored-By: Claude noreply@anthropic.com

Co-Authored-By: Claude <noreply@anthropic.com>
@skyrpex skyrpex added semver: patch docs: skip Pull request does not require documentation changes labels Jul 24, 2026
@skyrpex
skyrpex marked this pull request as ready for review July 24, 2026 14:34
@skyrpex
skyrpex requested a review from a team as a code owner July 24, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant