fix(plugin): verify install checksums against the artifact actually downloaded - #406
Merged
Merged
Conversation
…ownloaded A plugin with a binaryName has downloadPluginPackageForTier try its per-platform release asset first, falling back to the source tarball only when that download fails. But the registry carried exactly one checksum per plugin (the source tarball's), and installLocked always verified against it regardless of which artifact was actually on disk — so any binaryName plugin whose per-platform asset downloaded successfully had its platform tarball's bytes hashed and compared against the source tarball's checksum, which can never match. downloadPluginPackageForTier now reports which artifact it fetched (ArtifactKindSource, or a platform string from PlatformArch()) alongside the path. installLocked resolves the checksum that matches via the new resolveArtifactChecksum: the source artifact uses manifest.Checksum exactly as before (including its existing FIX-CLI-6 warn-and-proceed leniency when empty); a platform artifact uses the matching entry in the new PluginManifest.PlatformChecksums map, parsed from the registry's checksums.platforms object. A platform artifact with no matching registry checksum is refused unconditionally — it never falls through to the source-checksum leniency, and no env var (NSELF_PLUGIN_REQUIRE_CHECKSUM included) changes that. That leniency exists for the documented, tracked source-checksum coverage gap; it was never a license to install a downloaded executable with zero verification. A release that predates PlatformChecksums, or one platform whose checksum was never backfilled, is a registry data gap to fix upstream, not a flag to bypass here. PlatformChecksums round-trips through the registry cache (Registry.MarshalJSON) the same way every other field in this package must, per TestRegistryRoundTripLosesNoField.
… policy resolveArtifactChecksum: the source artifact always uses manifest.Checksum (including the empty-string case, left for verifyChecksum's own leniency to handle); a platform artifact resolves to its matching PlatformChecksums entry, never the source checksum or a different platform's; a platform artifact with no matching entry is refused regardless of NSELF_PLUGIN_REQUIRE_CHECKSUM, a present source checksum, or whether the map is nil versus just missing that one key. Three end-to-end tests exercise the same policy through verifyChecksum: a correct platform checksum passes, a mismatched one is rejected (mirroring the existing source-checksum mismatch test), and a missing one never reaches verifyChecksum's lenient empty-string branch at all.
…ile cap The per-artifact checksum change pushed this file to 309 lines, past the engineering-standard cap enforced by internal/repoqa's TestFileSizeBudgetNotExceeded (budget: 0 files allowed over). Tightened the new Step 5 comments and removed a pre-existing duplicated sentence in the Step 4 comment — no behavior change, same logic, back to 300 lines exactly.
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
1 participant
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.
Summary
A CLI plugin (one with a
binaryName) downloads its per-platform release asset first, falling back to the source tarball only on failure — but the registry carried exactly one checksum per plugin (the source tarball's), soinstallLockedwas hashing the downloaded platform tarball and comparing it against the source tarball's checksum. That comparison can never match, so every plugin whose per-platform asset downloads successfully would fail checksum verification at install time.downloadPluginPackageForTiernow returns which artifact it fetched (ArtifactKindSourceor a platform string fromPlatformArch()) alongside the download path.internal/plugin/registry_parse.goparses the registry's new nestedchecksums.platformsobject intoPluginManifest.PlatformChecksums(map[string]string, keyed by the exact platform stringsPlatformArch()returns).installLockedresolves the checksum that matches the artifact actually on disk via the newresolveArtifactChecksum: source usesmanifest.Checksumexactly as before (same FIX-CLI-6 leniency when empty); a platform artifact uses its matchingPlatformChecksumsentry.PlatformChecksumsround-trips through the registry cache (Registry.MarshalJSON), matching every other field in this package perTestRegistryRoundTripLosesNoField.Companion registry-side change (adds
checksums.platformsto the schema, backfills it for all 32 binaryName free plugins, and extends the CI checksum gate to verify it): nself-org/plugins branchp6/release-v1.0.1-repackaged-free-plugins(PR #81).Test plan
gofmt -l .empty (excluding pre-existing vendor findings)make vetcleango test ./internal/plugin/...— 412 passed (was 398 before this change)NSELF_PLUGIN_REQUIRE_CHECKSUM, a present source checksum, or nil vs. missing-key map shape)checksums.platforms(blocked on the companion plugins PR merging first)