From 7e60ef7cc5f72174b5fd2767fe717051757e4894 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:13:29 -0400 Subject: [PATCH 1/3] fix(plugin): verify install checksums against the artifact actually downloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/plugin/download.go | 32 +++++++---- internal/plugin/download_platform_test.go | 15 +++++- internal/plugin/installer_locked.go | 25 ++++++--- internal/plugin/interfaces.go | 21 ++++++++ internal/plugin/registry_cache.go | 13 +++++ internal/plugin/registry_json.go | 50 +++++++++-------- internal/plugin/registry_parse.go | 65 ++++++++++++++++++----- internal/plugin/security.go | 38 +++++++++++++ 8 files changed, 205 insertions(+), 54 deletions(-) diff --git a/internal/plugin/download.go b/internal/plugin/download.go index a6042221f..9189fb4d9 100644 --- a/internal/plugin/download.go +++ b/internal/plugin/download.go @@ -24,6 +24,14 @@ import ( "github.com/nself-org/cli/internal/license" ) +// ArtifactKindSource is the artifact kind downloadPluginPackageForTier +// reports when it fetched the source tarball. Any other value it returns is +// one of PlatformArch()'s platform strings (darwin-arm64, darwin-amd64, +// linux-amd64, linux-arm64, windows-amd64), identifying a per-platform +// binary tarball instead. Callers use this to pick the checksum that +// actually matches the bytes on disk — see installer_locked.go's Step 5. +const ArtifactKindSource = "source" + // downloadPlugin fetches the plugin tarball to a temporary file. // For paid plugins, it sends the X-License-Key header required by ping.nself.org. // For free plugins, it tries the R2-backed worker URL first and falls back to @@ -35,18 +43,24 @@ import ( // see license.go's isPaidPluginManifest doc comment for why the name map // drifts and the registry fields do not. func downloadPlugin(ctx context.Context, name, version, repository string) (string, error) { //nolint:unused // kept: name-only fallback entry point retained deliberately; see qa/bugs/declared-but-never-wired-symbols.md - return downloadPluginPackage(ctx, name, version, repository, "") + path, _, err := downloadPluginPackage(ctx, name, version, repository, "") + return path, err } // downloadPluginPackage fetches a plugin package using the name-only // isPaidPlugin fallback. Prefer downloadPluginPackageForTier when a manifest // is available (see its doc comment). -func downloadPluginPackage(ctx context.Context, name, version, repository, binaryName string) (string, error) { +func downloadPluginPackage(ctx context.Context, name, version, repository, binaryName string) (string, string, error) { return downloadPluginPackageForTier(ctx, name, version, repository, binaryName, isPaidPlugin(name)) } // downloadPluginPackageForTier fetches a plugin package, preferring a build -// for the running platform when the plugin ships a command binary. +// for the running platform when the plugin ships a command binary. It +// returns the local temp file path and the kind of artifact it actually +// fetched (ArtifactKindSource or a platform string) — the caller needs the +// kind to know which registry checksum applies, since a source tarball and a +// platform tarball for the same plugin+version have different bytes and +// therefore different checksums (see PluginManifest.PlatformChecksums). // // paid must come from the registry manifest (isPaidPluginManifest), not the // static paidPlugins name map — that map only lists 59 of 127 registered paid @@ -61,7 +75,7 @@ func downloadPluginPackage(ctx context.Context, name, version, repository, binar // someone on macOS. Those live as per-platform release assets, so they are // tried first, with the generic package as the fallback for a plugin whose // release predates per-platform assets. -func downloadPluginPackageForTier(ctx context.Context, name, version, repository, binaryName string, paid bool) (string, error) { +func downloadPluginPackageForTier(ctx context.Context, name, version, repository, binaryName string, paid bool) (string, string, error) { // Platform-specific package first, for a plugin that provides a command. if binaryName != "" { if platform, err := PlatformArch(); err == nil { @@ -71,7 +85,7 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository } platformURL := binaryPluginDownloadURL(strings.TrimSuffix(repo, ".git"), name, version, platform) if tmp, err := downloadFromURL(ctx, platformURL, nil); err == nil { - return tmp, nil + return tmp, platform, nil } // Fall through: no per-platform asset for this release. } @@ -91,7 +105,7 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository tmp, err := downloadFromURL(ctx, primaryURL, extraHeaders) if err == nil { - return tmp, nil + return tmp, ArtifactKindSource, nil } // If not a paid plugin, attempt GitHub Releases fallback on primary failure. @@ -100,13 +114,13 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository if fallbackURL != primaryURL { tmp2, fallbackErr := downloadFromURL(ctx, fallbackURL, nil) if fallbackErr == nil { - return tmp2, nil + return tmp2, ArtifactKindSource, nil } - return "", fmt.Errorf("download failed: primary %s: %w; fallback %s: %v", primaryURL, err, fallbackURL, fallbackErr) + return "", "", fmt.Errorf("download failed: primary %s: %w; fallback %s: %v", primaryURL, err, fallbackURL, fallbackErr) } } - return "", err + return "", "", err } // downloadFromURL fetches a single URL to a temp file and returns the file path. diff --git a/internal/plugin/download_platform_test.go b/internal/plugin/download_platform_test.go index debef889d..0806fc12f 100644 --- a/internal/plugin/download_platform_test.go +++ b/internal/plugin/download_platform_test.go @@ -31,16 +31,19 @@ func TestDownloadPluginPackagePrefersPlatformAsset(t *testing.T) { name string binaryName string wantPath string + wantKind string }{ { name: "cli plugin asks for its platform build", binaryName: "nself-example", wantPath: "/releases/download/v1.0.0/example-1.0.0-" + platform + ".tar.gz", + wantKind: platform, }, { name: "service plugin uses the generic package", binaryName: "", wantPath: "/plugins/example/tarball", + wantKind: ArtifactKindSource, }, } @@ -62,12 +65,16 @@ func TestDownloadPluginPackagePrefersPlatformAsset(t *testing.T) { t.Setenv("NSELF_PLUGIN_REGISTRY", srv.URL) - path, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, tt.binaryName) + path, kind, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, tt.binaryName) if err != nil { t.Fatalf("download: %v", err) } _ = path + if kind != tt.wantKind { + t.Errorf("artifact kind = %q, want %q", kind, tt.wantKind) + } + mu.Lock() defer mu.Unlock() if len(seen) == 0 { @@ -106,9 +113,13 @@ func TestDownloadPluginPackageFallsBackWhenNoPlatformAsset(t *testing.T) { t.Setenv("NSELF_PLUGIN_REGISTRY", srv.URL) - if _, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, "nself-example"); err != nil { + _, kind, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, "nself-example") + if err != nil { t.Fatalf("expected fallback to succeed, got: %v", err) } + if kind != ArtifactKindSource { + t.Errorf("artifact kind = %q, want %q (fell back to the generic package)", kind, ArtifactKindSource) + } mu.Lock() defer mu.Unlock() diff --git a/internal/plugin/installer_locked.go b/internal/plugin/installer_locked.go index ecdb630da..1b98fca3c 100644 --- a/internal/plugin/installer_locked.go +++ b/internal/plugin/installer_locked.go @@ -166,20 +166,31 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD // Tier comes from the registry manifest, not the paidPlugins name map, // which has drifted — see isPaidPluginManifest in license.go. paid := isPaidPluginManifest(manifest) - archivePath, err := downloadPluginPackageForTier(ctx, name, manifest.Version, manifest.Repository, firstBinary, paid) + archivePath, artifactKind, err := downloadPluginPackageForTier(ctx, name, manifest.Version, manifest.Repository, firstBinary, paid) if err != nil { return fmt.Errorf("downloading plugin %q: %w", name, err) } defer func() { _ = os.Remove(archivePath) }() // Step 5: Verify checksum before extraction. + // + // Which checksum applies depends on WHICH artifact Step 4 actually + // downloaded — see resolveArtifactChecksum's doc comment. + expectedChecksum, err := resolveArtifactChecksum(*manifest, artifactKind) + if err != nil { + _ = os.Remove(archivePath) + return err + } + // A checksum that IS present and wrong always refuses the install. A - // MISSING checksum only refuses when NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is - // set (default: warn and proceed, for every publishStatus including an - // effectively-stable one — registry coverage is 47/177 as of 2026-09-04, - // see verifyChecksum's doc comment; FIX-CLI-6). - if manifest.Checksum != "" { - if err := verifyChecksum(archivePath, manifest.Checksum, manifest.PublishStatus); err != nil { + // MISSING checksum for the SOURCE artifact only refuses when + // NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is set (default: warn and proceed, for + // every publishStatus including an effectively-stable one — registry + // coverage is 47/177 as of 2026-09-04, see verifyChecksum's doc comment; + // FIX-CLI-6). A platform artifact never reaches this branch with an + // empty expectedChecksum — the block above already refused it. + if expectedChecksum != "" { + if err := verifyChecksum(archivePath, expectedChecksum, manifest.PublishStatus); err != nil { _ = os.Remove(archivePath) return fmt.Errorf("checksum verification for plugin %q: %w", name, err) } diff --git a/internal/plugin/interfaces.go b/internal/plugin/interfaces.go index 90eb9cc8f..55d3fd2a8 100644 --- a/internal/plugin/interfaces.go +++ b/internal/plugin/interfaces.go @@ -240,4 +240,25 @@ type PluginManifest struct { // --detailed` displays it when present (registry entry or local // plugin.json) and shows nothing invented when it is not. UpdatedAt string `json:"updated_at,omitempty"` + + // PlatformChecksums holds one SHA-256 checksum (lowercase hex, no + // "sha256:" prefix) per per-platform binary tarball, keyed by the exact + // platform string PlatformArch() returns (darwin-arm64, darwin-amd64, + // linux-amd64, linux-arm64, windows-amd64). Populated from the registry's + // nested `checksums.platforms` object. + // + // Checksum above is ALWAYS the source tarball's checksum, never a + // platform one — the two packages have different bytes, so one flat + // field can never validate both (FIX-CLI-plugins-83: a platform tarball + // downloaded via binaryPluginDownloadURL was being checked against + // Checksum, which could never match). downloadPluginPackageForTier + // reports which artifact it fetched; installLocked picks the matching + // entry from this map, or Checksum for the source artifact — see + // verifyChecksum's caller in installer_locked.go. + // + // Only present for a plugin with a binaryName. A plugin whose release + // predates this field (or a platform the release never built a checksum + // for) has no entry here — see downloadPluginPackageForTier's doc + // comment for what happens then. + PlatformChecksums map[string]string `json:"platform_checksums,omitempty"` } diff --git a/internal/plugin/registry_cache.go b/internal/plugin/registry_cache.go index 481bfa255..201043538 100644 --- a/internal/plugin/registry_cache.go +++ b/internal/plugin/registry_cache.go @@ -101,6 +101,18 @@ func (r Registry) MarshalJSON() ([]byte, error) { } rawDeps = b } + // Checksums re-serialises PlatformChecksums into the nested registry + // shape, matching how it was parsed in — see pluginChecksumsEntry. + // SHA256 is left empty: the cache's source-tarball checksum lives in + // the flat Checksum field below (and always has), so re-populating + // the nested duplicate here would just be inventing a value never + // read back by anything (entryToManifest sources PlatformChecksums + // from Checksums.Platforms only). + var checksums *pluginChecksumsEntry + if len(p.PlatformChecksums) > 0 { + checksums = &pluginChecksumsEntry{Platforms: p.PlatformChecksums} + } + entries = append(entries, pluginEntry{ Name: p.Name, Version: p.Version, @@ -112,6 +124,7 @@ func (r Registry) MarshalJSON() ([]byte, error) { Tier: p.Tier, Repository: p.Repository, Checksum: p.Checksum, + Checksums: checksums, Tags: p.Tags, Tables: p.Tables, Port: p.Port, diff --git a/internal/plugin/registry_json.go b/internal/plugin/registry_json.go index 0b6dc9cf8..8e6bbfa28 100644 --- a/internal/plugin/registry_json.go +++ b/internal/plugin/registry_json.go @@ -130,29 +130,35 @@ func entryToManifest(e pluginEntry) PluginManifest { implFramework = e.Implementation.Framework } + var platformChecksums map[string]string + if e.Checksums != nil { + platformChecksums = normalizePlatformChecksums(e.Checksums.Platforms) + } + return PluginManifest{ - Name: e.Name, - Version: e.Version, - Description: e.Description, - Category: e.Category, - License: e.License, - LicenseType: e.LicenseType, - Tier: tier, - Repository: e.Repository, - Checksum: e.Checksum, - Tags: e.Tags, - RequiresLicense: e.RequiresLicense, - Tables: e.Tables, - Port: port, - TierPair: e.TierPair, - Bundles: e.Bundles, - Dependencies: parseDependencies(e.Dependencies), - APIEndpoints: parseAPIEndpoints(e.APIEndpoints), - Language: language, - Runtime: runtime, - PluginType: pluginType, - BinaryName: binaryName, - CLICommands: e.CLICommands, + Name: e.Name, + Version: e.Version, + Description: e.Description, + Category: e.Category, + License: e.License, + LicenseType: e.LicenseType, + Tier: tier, + Repository: e.Repository, + Checksum: e.Checksum, + PlatformChecksums: platformChecksums, + Tags: e.Tags, + RequiresLicense: e.RequiresLicense, + Tables: e.Tables, + Port: port, + TierPair: e.TierPair, + Bundles: e.Bundles, + Dependencies: parseDependencies(e.Dependencies), + APIEndpoints: parseAPIEndpoints(e.APIEndpoints), + Language: language, + Runtime: runtime, + PluginType: pluginType, + BinaryName: binaryName, + CLICommands: e.CLICommands, Author: e.Author, Homepage: e.Homepage, diff --git a/internal/plugin/registry_parse.go b/internal/plugin/registry_parse.go index 5b936bc97..8748d2f4b 100644 --- a/internal/plugin/registry_parse.go +++ b/internal/plugin/registry_parse.go @@ -62,26 +62,42 @@ type pluginEndpointEntry struct { Description string `json:"description"` } +// pluginChecksumsEntry holds the registry's nested `checksums` object. +// `sha256` duplicates the flat `checksum` field (source tarball, kept for +// backwards compatibility with older registry readers); `platforms` carries +// one checksum per per-platform binary tarball, keyed by the exact platform +// string internal/plugin/arch.go's PlatformArch() returns. Only present for +// a plugin with a binaryName — see PluginManifest.PlatformChecksums. +type pluginChecksumsEntry struct { + SHA256 string `json:"sha256,omitempty"` + Platforms map[string]string `json:"platforms,omitempty"` +} + // pluginEntry matches the fields present in the array-format (pro) // registry as well as the object-format (free) registry. // APIEndpoints is kept as json.RawMessage because the live registry returns // it as an array of objects while older/local registries use an array of // strings. We normalise both into []string during entryToManifest conversion. type pluginEntry struct { - Name string `json:"name"` - Version string `json:"version"` - Description string `json:"description"` - Category string `json:"category"` - Tier string `json:"tier"` - License string `json:"license"` - LicenseType string `json:"licenseType"` - Repository string `json:"repository"` - Checksum string `json:"checksum"` - DownloadURL string `json:"download_url"` - RequiresLicense bool `json:"requires_license"` - Tags []string `json:"tags"` - Tables []string `json:"tables,omitempty"` - Port int `json:"port,omitempty"` + Name string `json:"name"` + Version string `json:"version"` + Description string `json:"description"` + Category string `json:"category"` + Tier string `json:"tier"` + License string `json:"license"` + LicenseType string `json:"licenseType"` + Repository string `json:"repository"` + Checksum string `json:"checksum"` + // Checksums is the nested form carrying per-platform binary tarball + // checksums (checksums.platforms) alongside the source-tarball checksum + // (checksums.sha256, a duplicate of the flat Checksum field above). See + // pluginChecksumsEntry and PluginManifest.PlatformChecksums. + Checksums *pluginChecksumsEntry `json:"checksums,omitempty"` + DownloadURL string `json:"download_url"` + RequiresLicense bool `json:"requires_license"` + Tags []string `json:"tags"` + Tables []string `json:"tables,omitempty"` + Port int `json:"port,omitempty"` // TierPair and Bundles support install-time tier resolution for a slug // served twice (free + pro) as one product — see PluginManifest's doc // comments (interfaces.go) and tier_resolve.go. @@ -205,6 +221,27 @@ func parseAPIEndpoints(raw json.RawMessage) []string { return out } +// normalizePlatformChecksums copies a registry's checksums.platforms map, +// stripping an optional "sha256:" prefix from each value. The plugins repo +// writes the sibling checksums.sha256 field with that prefix (see +// release-tarballs.yml's backfill step); platform checksums are documented +// to be written as plain hex, but stripping a stray prefix defensively here +// costs nothing and avoids a checksum that LOOKS present but can never +// match verifyChecksum's raw hex comparison — which would otherwise surface +// as an install-time checksum mismatch instead of the registry data error it +// actually is. Returns nil for an empty/nil input, matching the omitempty +// contract used throughout this package. +func normalizePlatformChecksums(raw map[string]string) map[string]string { + if len(raw) == 0 { + return nil + } + out := make(map[string]string, len(raw)) + for platform, checksum := range raw { + out[platform] = strings.TrimPrefix(strings.ToLower(strings.TrimSpace(checksum)), "sha256:") + } + return out +} + // parseDependencies extracts the list of plugin-name dependencies from the // raw `dependencies` JSON value. The registry has shipped several shapes over // time, so we accept all of them and ignore non-plugin keys (npm/system/python) diff --git a/internal/plugin/security.go b/internal/plugin/security.go index fd20afa2d..fd04bfccd 100644 --- a/internal/plugin/security.go +++ b/internal/plugin/security.go @@ -116,6 +116,44 @@ func verifyChecksum(filePath string, expectedHash string, publishStatus string) return nil } +// resolveArtifactChecksum picks the registry checksum that matches the +// artifact downloadPluginPackageForTier actually fetched, identified by +// artifactKind (ArtifactKindSource, or a platform string from PlatformArch()). +// +// The source tarball and each platform's binary tarball are different bytes +// for the same plugin+version, so a source checksum can never validate a +// platform download or vice versa — comparing the wrong one is not a +// stricter check, it is a guaranteed-wrong one (plugins#83, the bug this +// function exists to close). +// +// For the source artifact this returns manifest.Checksum verbatim, including +// when it is empty — verifyChecksum's own warn-and-proceed leniency for a +// missing SOURCE checksum (FIX-CLI-6, a documented and tracked registry +// coverage gap) still applies, unchanged, to whatever this returns. +// +// For a platform artifact, a missing checksum in +// manifest.PlatformChecksums[artifactKind] is refused HERE, unconditionally +// — it never reaches verifyChecksum's lenient empty-string path, and no env +// var (NSELF_PLUGIN_REQUIRE_CHECKSUM included) changes that. The FIX-CLI-6 +// leniency exists for the source-checksum coverage gap; it was never a +// license to install an actual downloaded EXECUTABLE with zero +// verification. A release that predates PlatformChecksums, or one platform's +// checksum that was never backfilled, are both registry data gaps — the fix +// is a registry checksum, not a flag that makes the install proceed anyway. +func resolveArtifactChecksum(manifest PluginManifest, artifactKind string) (string, error) { + if artifactKind == ArtifactKindSource || artifactKind == "" { + return manifest.Checksum, nil + } + + checksum, ok := manifest.PlatformChecksums[artifactKind] + if !ok || checksum == "" { + return "", fmt.Errorf( + "plugin %q: registry has no checksum for platform %q at version %s — refusing to install an unverified binary (report to nself-org/plugins)", + manifest.Name, artifactKind, manifest.Version) + } + return checksum, nil +} + // verifyPluginSignature verifies that the Ed25519 signature stored in the // plugin's registry manifest matches the SHA-256 hash of the downloaded // tarball. The public key is pinned in the registry (never fetched at verify From 2a2d99e3e642bf1af8085da367c65305b068426f Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:13:34 -0400 Subject: [PATCH 2/3] test(plugin): cover per-platform checksum verification and the absent 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. --- internal/plugin/checksum_artifact_test.go | 196 ++++++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 internal/plugin/checksum_artifact_test.go diff --git a/internal/plugin/checksum_artifact_test.go b/internal/plugin/checksum_artifact_test.go new file mode 100644 index 000000000..f8773c32d --- /dev/null +++ b/internal/plugin/checksum_artifact_test.go @@ -0,0 +1,196 @@ +package plugin + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "testing" +) + +// TestResolveArtifactChecksum_SourceUsesManifestChecksum verifies that for +// the source artifact (ArtifactKindSource, and the zero value "" — a +// download that never got as far as reporting a kind), resolveArtifactChecksum +// returns manifest.Checksum verbatim, unaffected by whatever is in +// PlatformChecksums. +func TestResolveArtifactChecksum_SourceUsesManifestChecksum(t *testing.T) { + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "deadbeef", + PlatformChecksums: map[string]string{"linux-amd64": "should-not-be-used"}, + } + + for _, kind := range []string{ArtifactKindSource, ""} { + t.Run("kind="+kind, func(t *testing.T) { + got, err := resolveArtifactChecksum(manifest, kind) + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "deadbeef" { + t.Errorf("got %q, want manifest.Checksum %q", got, manifest.Checksum) + } + }) + } +} + +// TestResolveArtifactChecksum_SourceEmptyChecksumPassesThrough verifies that +// an empty manifest.Checksum for the source artifact comes back as "", not +// an error — the FIX-CLI-6 warn-and-proceed leniency for a missing SOURCE +// checksum is verifyChecksum's decision to make, not this function's. +func TestResolveArtifactChecksum_SourceEmptyChecksumPassesThrough(t *testing.T) { + manifest := PluginManifest{Name: "example", Version: "1.0.1"} + + got, err := resolveArtifactChecksum(manifest, ArtifactKindSource) + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "" { + t.Errorf("got %q, want empty string", got) + } +} + +// TestResolveArtifactChecksum_PlatformUsesMatchingEntry verifies that a +// platform artifact resolves to PlatformChecksums[platform], the entry +// matching the artifact actually downloaded — not manifest.Checksum (the +// SOURCE tarball's checksum, different bytes entirely) and not a different +// platform's entry. +func TestResolveArtifactChecksum_PlatformUsesMatchingEntry(t *testing.T) { + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "source-checksum-must-not-be-used", + PlatformChecksums: map[string]string{ + "darwin-arm64": "darwin-arm64-checksum", + "linux-amd64": "linux-amd64-checksum", + }, + } + + got, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "linux-amd64-checksum" { + t.Errorf("got %q, want the linux-amd64 entry, not the source checksum or a different platform's", got) + } +} + +// TestResolveArtifactChecksum_PlatformMissingEntryRefusesUnconditionally +// verifies the absent-checksum policy (plugins#83 item 8): a platform +// artifact with NO matching registry entry is refused with an error — +// regardless of NSELF_PLUGIN_REQUIRE_CHECKSUM, regardless of whether +// manifest.Checksum (the source checksum) happens to be present, and +// regardless of whether PlatformChecksums is nil or just missing this one +// platform's key. This is what makes the policy impossible to bypass: there +// is no code path, env var, or registry shape that turns a missing platform +// checksum into an empty string reaching verifyChecksum's lenient branch. +func TestResolveArtifactChecksum_PlatformMissingEntryRefusesUnconditionally(t *testing.T) { + cases := []struct { + name string + platformChecksums map[string]string + requireChecksum bool + }{ + {name: "nil map, default mode", platformChecksums: nil, requireChecksum: false}, + {name: "nil map, hard mode", platformChecksums: nil, requireChecksum: true}, + {name: "other platforms present, default mode", platformChecksums: map[string]string{"darwin-arm64": "abc123"}, requireChecksum: false}, + {name: "other platforms present, hard mode", platformChecksums: map[string]string{"darwin-arm64": "abc123"}, requireChecksum: true}, + {name: "empty-string entry for this platform", platformChecksums: map[string]string{"linux-amd64": ""}, requireChecksum: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if tc.requireChecksum { + t.Setenv(pluginRequireChecksumEnv, "1") + } + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "source-checksum-present-but-irrelevant", + PlatformChecksums: tc.platformChecksums, + } + + _, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err == nil { + t.Fatal("expected an error refusing the install, got nil") + } + if !strings.Contains(err.Error(), "linux-amd64") { + t.Errorf("error should name the platform that has no checksum: %v", err) + } + }) + } +} + +// TestPlatformArtifact_VerifiesAgainstMatchingPlatformChecksum is the +// end-to-end shape of Step 5 for a platform download: resolve the checksum +// for the artifact actually downloaded, then verify the archive bytes +// against it. A correct platform checksum passes. +func TestPlatformArtifact_VerifiesAgainstMatchingPlatformChecksum(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + sum := sha256.Sum256(content) + platformChecksum := hex.EncodeToString(sum[:]) + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "unrelated-source-checksum", + PlatformChecksums: map[string]string{ + "linux-amd64": platformChecksum, + }, + } + + expected, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if err := verifyChecksum(archivePath, expected, "stable"); err != nil { + t.Fatalf("correct platform checksum should verify: %v", err) + } +} + +// TestPlatformArtifact_MismatchRejected verifies that a platform download +// whose bytes do not match its recorded platform checksum is rejected — the +// same as any other present-but-wrong checksum (mirrors +// TestVerifyChecksum_MismatchAlwaysRefusesRegardlessOfStatus, for the +// platform-checksum path specifically). +func TestPlatformArtifact_MismatchRejected(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + wrongChecksum := hex.EncodeToString(make([]byte, sha256.Size)) // all-zero, guaranteed wrong + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + PlatformChecksums: map[string]string{ + "linux-amd64": wrongChecksum, + }, + } + + expected, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if err := verifyChecksum(archivePath, expected, "stable"); err == nil { + t.Fatal("tampered/mismatched platform artifact should fail verification, got nil error") + } +} + +// TestPlatformArtifact_AbsentChecksumNeverReachesLenientPath verifies the +// absent-checksum policy end to end: a platform artifact with no registry +// checksum never reaches verifyChecksum's warn-and-proceed branch at all — +// resolveArtifactChecksum refuses first, before any checksum comparison +// (lenient or not) would run. +func TestPlatformArtifact_AbsentChecksumNeverReachesLenientPath(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + _ = archivePath // the archive is never reached; resolveArtifactChecksum fails first + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + PlatformChecksums: nil, + } + + if _, err := resolveArtifactChecksum(manifest, "linux-amd64"); err == nil { + t.Fatal("expected resolveArtifactChecksum to refuse before any verification would run") + } +} From db680d673f7265890108b5fc9d22b148c485f50d Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:21:54 -0400 Subject: [PATCH 3/3] refactor(plugin): trim installer_locked.go comments to the 300-line file cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/plugin/installer_locked.go | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/internal/plugin/installer_locked.go b/internal/plugin/installer_locked.go index 1b98fca3c..62d12ccb7 100644 --- a/internal/plugin/installer_locked.go +++ b/internal/plugin/installer_locked.go @@ -153,12 +153,9 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD fmt.Fprintf(os.Stderr, " ✓ %s installed\n", dep) } - // Step 4: Download the plugin archive. - // A plugin that provides a command needs a package built for this platform; - // one that does not is source and works anywhere. cliBinaryName returns "" - // for the latter, which is most plugins. - // A plugin providing a command needs a package built for this platform; one - // that does not is source and works anywhere. + // Step 4: Download the plugin archive. A plugin that provides a command + // needs a package built for this platform; one that does not is source + // and works anywhere. cliBinaryName returns "" for the latter (most plugins). var firstBinary string if names := cliBinaryNames(name, manifest); len(names) > 0 { firstBinary = names[0] @@ -172,23 +169,17 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD } defer func() { _ = os.Remove(archivePath) }() - // Step 5: Verify checksum before extraction. - // - // Which checksum applies depends on WHICH artifact Step 4 actually - // downloaded — see resolveArtifactChecksum's doc comment. + // Step 5: Verify checksum before extraction — resolveArtifactChecksum + // picks the checksum matching the artifact Step 4 downloaded. expectedChecksum, err := resolveArtifactChecksum(*manifest, artifactKind) if err != nil { _ = os.Remove(archivePath) return err } - // A checksum that IS present and wrong always refuses the install. A - // MISSING checksum for the SOURCE artifact only refuses when - // NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is set (default: warn and proceed, for - // every publishStatus including an effectively-stable one — registry - // coverage is 47/177 as of 2026-09-04, see verifyChecksum's doc comment; - // FIX-CLI-6). A platform artifact never reaches this branch with an - // empty expectedChecksum — the block above already refused it. + // A present-but-wrong checksum always refuses; an empty one here is + // always the SOURCE artifact's — see verifyChecksum's FIX-CLI-6 doc + // comment (a missing platform checksum already refused above). if expectedChecksum != "" { if err := verifyChecksum(archivePath, expectedChecksum, manifest.PublishStatus); err != nil { _ = os.Remove(archivePath)