Skip to content

fix(arcup): accept a valid checksum written in uppercase hex - #302

Open
Bornoz wants to merge 1 commit into
circlefin:mainfrom
Bornoz:fix/arcup-uppercase-checksum
Open

fix(arcup): accept a valid checksum written in uppercase hex#302
Bornoz wants to merge 1 commit into
circlefin:mainfrom
Bornoz:fix/arcup-uppercase-checksum

Conversation

@Bornoz

@Bornoz Bornoz commented Sep 1, 2026

Copy link
Copy Markdown

verify_checksum_file accepts a checksum whose hash matches ^[0-9A-Fa-f]{64}$ — either case — but then compares it against compute_sha256, which always returns lowercase (sha256sum and shasum both do). The comparison is case-sensitive, so a correct digest written in uppercase fails as Checksum verification failed and the install is refused for a genuine release.

Fold the expected checksum to lowercase before comparing, the same normalization detect_platform already applies to uname output. Adds an uppercase case to test_checksum_validation; it fails on main and passes with the fix. shellcheck arcup/arcup is clean and the suite is 26/26.

This is a separate defect from the open trailing-newline fixes (#243, #262) and the self-update verification work (#204/#223) — same function, different failure. It applies after the format check and touches only the comparison, so it composes with those rather than competing.

verify_checksum_file accepts a checksum whose hash matches ^[0-9A-Fa-f]{64}$ —
either case — but then compares it against compute_sha256, which always returns
lowercase (sha256sum and shasum both do). The comparison is case-sensitive, so a
correct digest written in uppercase fails as "Checksum verification failed" and
the install is refused for a genuine release.

Fold the expected checksum to lowercase before comparing, the same normalization
detect_platform already applies to uname output. Adds an uppercase case to
test_checksum_validation, which fails on main and passes with the fix.
@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Verified this independently at 0936d76 — the defect is real, the fix is correctly placed, and the composition claim survives an actual merge test. One gap worth fixing before this lands, at the end.

Confirmed on main:

  • The inconsistency is exactly as described: the format check at line 663 admits ^[0-9A-Fa-f]{64}$ (either case), compute_sha256 returns lowercase on both branches (sha256sum and shasum -a 256), and the comparison at 677 is a case-sensitive !=. An uppercase digest therefore passes validation and then fails verification with a misleading "Checksum verification failed" — the validator accepts what the comparator rejects.
  • The new test does fail on main: with main's arcup and this branch's test file, the suite aborts at the uppercase case (error exits the script), and on this branch the full run is 26/26, exit 0 (Linux, GNU coreutils).
  • The composition claim holds under merge test, not just by inspection (disclosure: fix(arcup): accept checksum files without a trailing newline #243 is mine, so I tested it directly): fix(arcup): accept checksum files without a trailing newline #243 ↔ this PR merge cleanly in both orders — the trailing-newline fix rewrites the read above the format check, this one inserts after it, and the hunks stay disjoint. Also merge-tested against fix(arcup): a release is newer than its own pre-release #300 (the version_gt fix, same two files): clean.

Severity, honestly framed: I pulled a real .sha256 asset from the v0.8.0 release — lowercase, as every sha256sum-based pipeline produces. So nothing bites today on the default path; the exposure is ARC_REPO overrides or any future release-tooling change that uppercases digests (PowerShell Get-FileHash, for one, emits uppercase). That makes this a consistency fix rather than a live-path bug — the right justification is the one the PR already gives: the accepted format and the compared format should agree.

The gap: no ARCUP_INSTALLER_VERSION bump. The script header requires incrementing it for any modification, and the self-update check depends on it. Ironically, skipping it is why this PR merges cleanly with everything — #300, #243, and #262 all claim the same 0.2.0 → 0.2.1 bump and will conflict pairwise on that line. That coordination problem exists regardless; this PR still needs its increment, with the concrete number picked at land time depending on what has already gone in. Worth adding now with a note to renumber, so it doesn't land as the one modification that broke the header's contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants