make the install script refuse what it cannot verify - #63
Merged
Merged
Conversation
From the review of the merged change, and the first of these was a fault in the line the README tells people to run. CERTREADER_INSTALL_DIR pointing somewhere that does not exist yet, which is the ordinary state of ~/.local/bin, failed. Worse, it failed saying the directory was not writable and to pick one that was, which is not what was wrong with it. The directory is created now, with sudo only if it cannot be created otherwise. The earlier test of this passed because it ran mkdir -p first, so it tested a case the README does not describe. Without sha256sum or shasum the download went unchecked and was installed anyway, on a shrug and a message to stderr. A script that people are told to pipe into a shell does not get to skip the one step that makes that defensible. It stops instead. Which then went wrong in its own way: the failure was raised inside a subshell, so what reached the user was the mismatch message from the caller rather than the missing tool. The command is settled before anything is downloaded now, and the three ways verification can fail say which one happened: no tool to check with, no entry in the sums file for this archive, or a file that does not match the entry. None of them install anything. Matching the archive in the sums file is by whole field rather than by grep, where the dots in a filename are a pattern that could match a longer name. The note about brew now spells macOS the way apple does, being a line somebody reads. The comments keep the lowercase this repo writes them in. Tested on amd64 and arm64: the README's own command with no directory there, no checksum tool, a sums file with no matching entry, and a tampered archive served through GITHUB_URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v
There was a problem hiding this comment.
🟡 Changes recommended
checksum_command can return non-zero under set -e, causing an early exit before the intended “missing checksum tool” error is shown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the install one-liner by making the script refuse installation when it cannot verify a downloaded release artifact, and by correctly handling install directories that don’t exist yet (e.g., ~/.local/bin).
Changes:
- Resolve and validate the checksum verification command before any downloads, and fail fast when no verifier is available.
- Verify checksums using an exact filename match (field-based) rather than a regex match.
- Create the install directory when missing, using
sudoonly when necessary.
File summaries
| File | Description |
|---|---|
| README.md | Updates the Linux install docs to reflect directory creation and stricter verification behavior. |
| install | Improves checksum verifier selection/handling, checksum line selection, and install-dir creation logic. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+51
to
+55
| checksum_command() { | ||
| if command -v sha256sum >/dev/null 2>&1; then | ||
| sha256sum -c "$1" | ||
| echo "sha256sum -c" | ||
| elif command -v shasum >/dev/null 2>&1; then | ||
| shasum -a 256 -c "$1" | ||
| else | ||
| echo "neither sha256sum nor shasum found, skipping checksum" >&2 | ||
| return 0 | ||
| echo "shasum -a 256 -c" |
| echo "${install_dir} does not exist, creating it" | ||
| if ! mkdir -p "${install_dir}" 2>/dev/null; then | ||
| command -v sudo >/dev/null 2>&1 || | ||
| fail "${install_dir} does not exist and sudo is not available to create it" |
The other install directory failures name the variable to set and this one did not, so it told somebody what was wrong without telling them what to do about it. The comment on checksum_command now says why finding neither tool returns zero rather than tripping set -e at the assignment, which is the other thing the review raised. An if whose conditions are all false and which has no else exits zero, so the empty answer reaches the caller's check, which is what reports it. Confirmed in sh, bash and dash, and by running the script with both tools hidden: it prints the missing tool message and exits 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v
The behaviour is unchanged: an if whose conditions are all false and which has no else already exits zero, so the empty answer reached the caller's check and the missing tool was reported there. Two passes of review read the function as though it could end the script at the assignment instead, which is reason enough to stop asking a reader to know the rule. The return states it. Still prints the missing tool message and exits 1 with both tools hidden, and installs as before with them present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From the Copilot review of #62, which landed after it was merged. Four of the five points were fair; the first was a fault in the line the README tells people to run.
the documented command failed
CERTREADER_INSTALL_DIRpointing somewhere that does not exist yet — the ordinary state of~/.local/bin— failed, and failed misleadingly:The directory simply was not there. It is created now, with
sudoonly if it cannot be created otherwise.My earlier test passed because it ran
mkdir -p ~/binfirst, so it tested a case the README does not describe. Testing the documented command verbatim is what found it.unverified downloads were installed
Without
sha256sumorshasumthe script skipped verification and installed anyway, on a message to stderr. A script people are told to pipe into a shell does not get to skip the step that makes that defensible. It now stops.Fixing that went wrong in its own way first: the failure was raised inside a subshell, so the user saw the mismatch message from the caller rather than the missing-tool one. The command is now settled before anything is downloaded, and the three ways verification can fail are distinct:
neither sha256sum nor shasum is available to check the download withchecksums_linux_arm64.txt has no entry for certreader_0.25.1_linux_arm64.tar.gzcertreader_0.25.1_linux_arm64.tar.gz does not match its published checksumNone of them install anything.
also
Matching the archive within the sums file is by whole field (
awk) rather thangrep, where the dots in a filename are a pattern that could match a longer name. Thebrewnote spells macOS the way Apple does, being a line somebody reads.Not taken: capitalising "github" in a code comment. This repo writes comments in lowercase throughout (
homebrew marks what a cask installs,goreleaser calls this deprecated), so the lowercase matches house style. The user-visible string was worth changing; the comment was not.testing
amd64 and arm64 Debian containers: the README's own command with no directory present, no checksum tool, a sums file with no matching entry, and a tampered archive served through
GITHUB_URL.🤖 Generated with Claude Code
https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v