Repository navigation
fix: verify binaries on disk against a pinned checksum and publish the digests (#63) - #262
Merged
Merged
Conversation
Checksum verification was fail-closed for downloads but nothing was ever pinned, so every native install was refused, and a binary already on disk was trusted without a checksum. - composer.json pins the published SHA-256 digests of 0.5.0 and 0.5.2 under extra.scanmephp.checksums, and the plugin reads them from the installed package (as issue #48 describes: checksums shipped with the released package), with the root project's own pins taking precedence. - ChecksumManager::existingBinaryIsValid() rejects a binary it cannot verify instead of accepting it, so an unknown file is re-downloaded through the fail-closed download path; the installer says whether it failed the digest or had no digest to check. - The release workflow publishes checksums.txt (sha256sum format) with every release, and docs/release-workflow.md documents pinning the digests of a new release after it is built.
Review round 1 was right: the digests of a release only exist after its build, so they can never be in that release's own composer.json. The docs said a verified native install needs no action from the consumer, which is not true and contradicted docs/release-workflow.md in the same change. - CHANGELOG.md and README.md now say that the digests of a version land in composer.json of the *next* release, that a fresh install of the newest one therefore still refuses the download and uses the pure PHP encoder, and how to pin the digests yourself from the release's checksums.txt. - Verify a manual download with --ignore-missing, and mention shasum for macOS, which has no sha256sum. - Rename the plugin test: its fixture exercises the lookup order, it is not a state any real release is in. - findings.md hands the rest of issue point (1) to the maintainer with the three ways out and what each one costs.
…63) Two reviews made the same point: a release's digests only exist after its build, so they can never sit in that release's own composer.json. The pins and the second lookup source therefore changed no real install, while the docs claimed an out-of-the-box native install. Both are gone. What ships instead: - ChecksumManager::existingBinaryIsValid() rejects a binary it has no pinned digest for, so an unverifiable file is re-downloaded through the fail-closed path instead of being accepted (the actual point 2 of the issue). - An empty or non-string value in extra.scanmephp.checksums is not a pin. - The installer says whether a binary failed the digest or had none to check, and reports it when the file cannot be deleted: the loaders probe those paths, so a file left behind would still be loaded. - Every release publishes checksums.txt, the published source a project pins its digests from. README, CHANGELOG and the release docs no longer claim a native install works without a pinned digest; the rest of the issue point is handed to the maintainer in findings.md with the three ways out. Tests: fail-closed verification, the pin reaching the downloader, and a read-only directory making unlink() fail. All three fail on the old code.
Round 3 findings. - The FFI branch ignored removeUnverifiedBinary()'s result while the extension branch honoured it, so a file that could not be deleted was truncated by the next download. Both branches stop now, and the read-only directory test covers each of them. - "Re-downloading the verified binary." is printed only when a download follows, and the "could not remove" message no longer promises the pure PHP encoder: installBinaries() still tries the FFI path after the extension path gives up. - Replaced testPinnedChecksumReachesTheVerifiedDownloadPath. It pinned the project root, which main already wires, so it passed on main and the commit message counted it as regression coverage it was not. The new testPackageInstallSurvivesANonStringPin does fail on main: there an int pin comes back out of getChecksum(): ?string as a TypeError, which is an Error and escapes catch(\Exception), aborting the install. Verified in a scratch copy with src/ restored from origin/main: all seven new and changed tests fail there. - README: verification runs when ScanMePHP is installed or updated, not on every composer install, and the pin example says to copy the digest column only - a whole sha256sum line is a pin that never matches. - findings.md: InstallScript::run() is dead code, not a live hole.
Round 4 finding. No published release has checksums.txt - the release job that writes it is still unreleased - so `gh release download vX.Y.Z` with the concrete "0.5.2" from the example 404s for every version that exists today. - The example pins "X.Y.Z", and both the README and the CHANGELOG say once that the file ships with the first release containing this change and that earlier releases have none. - CHANGELOG also states the behaviour change a user will hit first: `composer update` to a version with no pinned digest now deletes a previously verified binary and then refuses to replace it.
Round 5 findings, fixed in ChecksumManager rather than in the prose.
- A scalar extra.scanmephp.checksums (a string, int, bool or float instead of
a version map) was assigned to the ?array property and raised a TypeError.
That is an Error, so it escaped the plugin's catch (\Exception) and aborted
the whole composer install. A non-array section now reads as "nothing is
pinned", which is the fail-closed path it should always have taken.
- Only a 64-character lowercase hex digest is a pin. A whole `sha256sum` line
copied out of checksums.txt, an empty value or an uppercase digest can never
match: each of them downloaded the multi-megabyte binary and then threw
checksumMismatch on every install. They are refused before the transfer now,
which is what the README section already claimed.
- The three legacy fixtures that used 'abc123def456' as a stand-in digest now
use real hash('sha256', ...) values, since the shape is validated.
Tests at both levels - ChecksumManagerTest and PluginTest - and all eleven
new or changed tests fail on origin/main with src/ restored.
Round 6 finding. The assertion matched "refused" and "extra.scanmephp.checksums", which the FFI branch prints on its own because that branch has no pin either way - so the test passed with the 64-hex validation removed, and would have started doing real HTTP transfers the moment the behaviour regressed. Asserting "No SHA-256 checksum configured for binary <name>" is present on this branch and absent without the fix (verified in a scratch copy with only the shape validation removed). With that, all 12 new or changed test cases in this PR fail on origin/main; my previous commit message said eleven. This change was not reviewed by a seventh round, which the team lead ruled out.
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.
What this does
Fixes the second half of #63 and publishes the checksums for the first half.
ChecksumManager::existingBinaryIsValid()no longer trusts a binary already on disk when nochecksum is pinned for it. Such a file cannot be verified, so it is rejected and re-downloaded
through the verified (fail-closed) path, which refuses it too.
instead of downloading when the file cannot be deleted — the loaders probe these paths, so a
leftover file would still be loaded.
extra.scanmephp.checksumscan no longer abort acomposer install: a scalar sectionand a non-string digest both raised a
TypeError, which is an\Errorand escaped the plugin'scatch (\Exception).sha256sumline copied out ofchecksums.txt— the mistake the README warns about — can never match, so it is refused beforethe transfer instead of downloading the binary and then throwing
checksumMismatchon everyinstall.
checksums.txtwith the SHA-256 of all its binaries. It is thepublished source for the digests a project pins, and it verifies a manual download.
Behaviour change to be aware of
composer updateto a version whose binaries have no pinned digest now deletes a previouslyverified binary and then refuses to replace it, so native acceleration disappears until the digest
is pinned. That is the fail-closed behaviour #63 asks for and the whole point of the change, but it
is visible to anyone who updates without pinning, and it is the first thing users will report. It is
in the
CHANGELOG.mdentry too.Part 1 of #63 is NOT done
The issue asks for checksums "in
composer.json" as well as in the releases. Releases now publishthem;
composer.jsoncannot, and that is a structural problem, not an oversight:The installer trusts digests pinned in the consuming project's
composer.json, so they have totravel with the package. A release's digests only exist after that release is built, so they can
never be in that release's own
composer.json— a fresh install always looks up a version whoseentry does not exist yet. That design was implemented, reviewed twice and removed; the reasoning is
in
findings.mdand the decision that closes the gap is tracked in #260 (build before tagging /sign the assets / trust the published file as a fallback — a maintainer call).
Until then, a project pins the digest for its platform itself, from the release's
checksums.txt;the README shows how. So this PR does not close #63 and deliberately carries no
Closes #63.Tests
12 new or changed test cases, all of which fail on
origin/mainwithsrc/restored (verified in ascratch copy): fail-closed on-disk verification, unusable pins at the manager and the plugin, the
scalar section, the unremovable binary in both installer branches, and the refused
sha256sumline.The last commit tightens one assertion after the round-6 review; it was not reviewed by a seventh
round, which the team lead ruled out.
Follow-ups filed while working on this: #260 (checksum source decision), #261 (an unrelated
composer.jsonread that also aborted the install, filed separately on purpose).