Skip to content

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

Open
Bornoz wants to merge 2 commits into
circlefin:mainfrom
Bornoz:fix/arcup-uppercase-checksum
Open

fix(arcup): accept a valid checksum written in uppercase hex#302
Bornoz wants to merge 2 commits 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.

The script header requires incrementing ARCUP_INSTALLER_VERSION for any
modification to arcup, and the self-update check compares it against the
copy on main to tell a user their installer is stale. This change edits
arcup, so it owes the bump.

Test suite stays at 26/26: the self-update fixture serves 0.2.0 as the
remote version, so a local 0.2.1 is simply not older and nothing is
advertised as an update.
@Bornoz

Bornoz commented Sep 10, 2026

Copy link
Copy Markdown
Author

Thanks for the merge test, that is more than I checked myself.

Bump added in 3a07773: ARCUP_INSTALLER_VERSION 0.2.0 -> 0.2.1. You are right that skipping it is exactly what kept this branch conflict-free with everything, and that is not a property worth buying at the cost of the header's contract.

On renumbering: #300, #243 and #262 all claim 0.2.1, so whichever lands first takes it and the rest have to move. I have no stake in the ordering, so treat 0.2.1 here as a placeholder and I will rebase onto whatever number is free when this is ready to land.

Suite is still 26/26, exit 0, on Linux with GNU coreutils. The self-update fixture serves 0.2.0 as the remote version, so a local 0.2.1 reads as not-older and nothing is advertised as an update.

Agreed on the severity framing too. Every sha256sum pipeline emits lowercase, so nothing breaks on the default path today. The case for the change is the one in the PR body: the format the validator accepts and the format the comparator requires should be the same.

@osr21

osr21 commented Sep 10, 2026

Copy link
Copy Markdown

Re-ran the merge test at the new head (3a07773), since adding the bump changes exactly what the earlier test measured. My prediction was wrong, so correcting it here.

All six pairwise merges are clean, in both orders. Tested at current heads (#300 c6f48d9, #243 363ded4, #262 4989cb0):

pr300 + pr302 : CLEAN      pr302 + pr300 : CLEAN
pr243 + pr302 : CLEAN      pr302 + pr243 : CLEAN
pr262 + pr302 : CLEAN      pr302 + pr262 : CLEAN

Adding the bump did not cost this branch the conflict-free property. I claimed the four PRs "will conflict pairwise on that line" — they do not, because all four set the identical value 0.2.1, and git merges an identical change on both sides without conflict. A version-line conflict would require two PRs picking different numbers.

The real hazard is the opposite of the one I described: silence, not conflict. Merging #243 + #302 produces a tree whose header reads ARCUP_INSTALLER_VERSION="0.2.1" — two independent modifications under one version string. Adding #300 on top: still 0.2.1, three modifications, still clean, nothing raised.

That has a concrete consequence in the self-update path. Both check_installer_up_to_date and update_arcup gate on version_gt "$remote_version" "$ARCUP_INSTALLER_VERSION", and version_gt returns 1 on string equality (the [ "$1" = "$2" ] && return 1 guard at line 262). Extracting the function verbatim and exercising it:

remote=0.2.1 local=0.2.1 -> no update
remote=0.2.2 local=0.2.1 -> UPDATE ADVERTISED
remote=0.2.0 local=0.2.1 -> no update

So a user holding the first-landed 0.2.1 is never told a second 0.2.1 exists, and arcup --self-update prints Arcup is already up to date (version 0.2.1) and exits 0 without updating. The fix silently fails to reach anyone already on that version — for a checksum-verification change, the wrong direction to fail in.

That makes your "treat 0.2.1 as a placeholder and rebase onto whatever is free" plan the right call, with one caveat worth flagging to whoever lands these: nothing will remind them. No conflict fires, no test asserts the bump (the suite exercises version_gt directly at lines 58/63 and pins the remote fixture at 0.2.0), and the header's "please increment the version number" note is a comment, not a check. The renumber has to happen from memory at merge time, and if it is missed the failure is invisible.

Your other claims check out at 3a07773 (Linux, GNU coreutils): the suite is 26/26, exit 0, with no non-ok lines — including uppercase checksum digest passes. And the self-update fixture does serve 0.2.0 as the remote version (test_arcup.sh:714), so a local 0.2.1 reads as not-older and nothing is advertised, exactly as you said. CI still has not run on this PR — external fork PRs sit behind the workflow-approval gate — so all of the above is local.

I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only.

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