diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index f0344692..b16a3683 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -269,9 +269,16 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan } destDir := filepath.Join(dataDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir %s: %w", destDir, err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. This prevents stale + // binaries from surviving an upgrade when the tarball uses a differently- + // named executable (e.g. GoReleaser platform-suffix names). + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() // no-op if commitPluginStagingDir succeeds (staging renamed away) fmt.Fprintf(os.Stderr, "Downloading %s...\n", dl.URL) data, err := downloadURL(dl.URL) @@ -307,7 +314,7 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan // Emit install_verify hook after download and before extraction (opt-in via req.Verify). // Write tarball to disk so hook handlers can inspect it (e.g. sigstore cosign verify). if verify != nil { - tarballPath := filepath.Join(destDir, pluginName+".tar.gz") + tarballPath := filepath.Join(stagingDir, pluginName+".tar.gz") if writeErr := os.WriteFile(tarballPath, data, 0600); writeErr != nil { return fmt.Errorf("write tarball for verify hook: %w", writeErr) } @@ -318,30 +325,45 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan } fmt.Fprintf(os.Stderr, "Extracting to %s...\n", destDir) - if err := extractTarGz(data, destDir); err != nil { + if err := extractTarGz(data, stagingDir); err != nil { return fmt.Errorf("extract plugin: %w", err) } // Ensure the plugin binary is named to match the plugin name so that // ExternalPluginManager.DiscoverPlugins() can find it (expects //). - if err := ensurePluginBinary(destDir, pluginName); err != nil { + if err := ensurePluginBinary(stagingDir, pluginName); err != nil { fmt.Fprintf(os.Stderr, "warning: could not normalize binary name: %v\n", err) } // Write plugin.json from the registry manifest. This keeps the installed // version metadata in sync with the manifest. If the tarball already // extracted a plugin.json, this overwrites it with the registry version. - pluginJSONPath := filepath.Join(destDir, "plugin.json") + // Failure is a hard error: continuing with an archive-supplied plugin.json + // could silently drop registry-only metadata (capabilities, CLI commands, + // build hooks) and would let the archive version bypass verifyInstalledVersion. + pluginJSONPath := filepath.Join(stagingDir, "plugin.json") if writeErr := writeInstalledManifest(pluginJSONPath, manifest); writeErr != nil { - fmt.Fprintf(os.Stderr, "warning: could not write plugin.json: %v\n", writeErr) + return fmt.Errorf("write plugin.json: %w", writeErr) } - // Verify the installed plugin.json is valid for ExternalPluginManager. + // Verify the staged plugin.json is valid for ExternalPluginManager. fmt.Fprintf(os.Stderr, "Verifying plugin manifest...\n") - if verifyErr := verifyInstalledPlugin(destDir, pluginName); verifyErr != nil { + if verifyErr := verifyInstalledPlugin(stagingDir, pluginName); verifyErr != nil { return fmt.Errorf("post-install verification failed: %w", verifyErr) } + // Verify the installed version matches what the manifest declares. This + // catches cases where plugin.json could not be written above. + if verifyErr := verifyInstalledVersion(stagingDir, manifest.Version); verifyErr != nil { + return fmt.Errorf("post-install version check failed: %w", verifyErr) + } + + // Atomically replace any previous installation with the validated staging + // directory. If this step fails the old installation is left intact. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + // Strip any existing "v" prefix from the version before printing so that // manifests that store "v0.6.1" don't produce "Installed X vv0.6.1". fmt.Printf("Installed %s v%s to %s\n", manifest.Name, strings.TrimPrefix(manifest.Version, "v"), destDir) @@ -727,23 +749,33 @@ func installFromURL(rawURL, pluginDir, expectedSHA256 string, skipChecksum bool) pluginName := normalizePluginName(pj.Name) destDir := filepath.Join(pluginDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir: %w", err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() - if err := extractTarGz(data, destDir); err != nil { + if err := extractTarGz(data, stagingDir); err != nil { return fmt.Errorf("extract to dest: %w", err) } - if err := ensurePluginBinary(destDir, pluginName); err != nil { + if err := ensurePluginBinary(stagingDir, pluginName); err != nil { return fmt.Errorf("could not normalize binary name: %w", err) } - // Validate the installed plugin (same checks as registry installs). - if verifyErr := verifyInstalledPlugin(destDir, pluginName); verifyErr != nil { + // Validate the staged plugin (same checks as registry installs). + if verifyErr := verifyInstalledPlugin(stagingDir, pluginName); verifyErr != nil { return fmt.Errorf("post-install verification failed: %w", verifyErr) } + // Atomically replace any previous installation. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + // Hash the installed binary (not the archive) so that verifyInstalledChecksum matches. binaryPath := filepath.Join(destDir, pluginName) checksum, hashErr := hashFileSHA256(binaryPath) @@ -802,12 +834,17 @@ func installFromLocal(srcDir, pluginDir string) error { pluginName := normalizePluginName(pj.Name) destDir := filepath.Join(pluginDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir: %w", err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() // Copy plugin.json - if err := copyFile(pjPath, filepath.Join(destDir, "plugin.json"), 0640); err != nil { + if err := copyFile(pjPath, filepath.Join(stagingDir, "plugin.json"), 0640); err != nil { return err } @@ -820,10 +857,15 @@ func installFromLocal(srcDir, pluginDir string) error { return fmt.Errorf("no plugin binary found in %s (tried %s and %s)", srcDir, pluginName, fullName) } } - if err := copyFile(srcBinary, filepath.Join(destDir, pluginName), 0750); err != nil { + if err := copyFile(srcBinary, filepath.Join(stagingDir, pluginName), 0750); err != nil { return err } + // Atomically replace any previous installation. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + binaryChecksum, hashErr := hashFileSHA256(filepath.Join(destDir, pluginName)) if hashErr != nil { fmt.Fprintf(os.Stderr, "warning: could not compute binary checksum: %v\n", hashErr) @@ -1364,6 +1406,81 @@ func ensurePluginBinary(destDir, pluginName string) error { return os.Rename(filepath.Join(destDir, bestName), expectedPath) } +// preparePluginStagingDir removes any leftover staging directory and creates a +// fresh one alongside destDir (same filesystem). The caller must call +// commitPluginStagingDir on success. On failure the returned cleanup func +// removes the staging directory. +func preparePluginStagingDir(destDir string) (stagingDir string, cleanup func(), err error) { + stagingDir = destDir + ".installing" + if removeErr := os.RemoveAll(stagingDir); removeErr != nil { + return "", nil, fmt.Errorf("clean staging dir %s: %w", stagingDir, removeErr) + } + // Ensure the parent directory exists so MkdirAll only needs to create the + // staging leaf. + if mkErr := os.MkdirAll(filepath.Dir(destDir), 0750); mkErr != nil { + return "", nil, fmt.Errorf("create plugin base dir: %w", mkErr) + } + if mkErr := os.Mkdir(stagingDir, 0750); mkErr != nil { + return "", nil, fmt.Errorf("create staging dir %s: %w", stagingDir, mkErr) + } + return stagingDir, func() { os.RemoveAll(stagingDir) }, nil //nolint:errcheck +} + +// commitPluginStagingDir replaces destDir with stagingDir. To preserve the +// existing installation if the final rename fails, the old destDir is first +// renamed to a trash location on the same filesystem. Only after the new +// directory is successfully renamed into place is the trash removed. +// +// 1. Rename destDir → destDir+".uninstalling" (no-op if destDir absent) +// 2. Rename stagingDir → destDir +// 3. On step-2 failure: restore destDir+".uninstalling" → destDir +// 4. On step-2 success: remove destDir+".uninstalling" +// +// On success stagingDir no longer exists on disk; the deferred cleanup +// returned by preparePluginStagingDir becomes a harmless no-op. +func commitPluginStagingDir(stagingDir, destDir string) error { + trashDir := destDir + ".uninstalling" + // Remove any leftover trash from a previous interrupted commit. + if err := os.RemoveAll(trashDir); err != nil { + return fmt.Errorf("clean trash dir %s: %w", trashDir, err) + } + + // Move the existing install out of the way before installing the new one. + // If destDir does not exist yet (first install) we skip this step. + hasExisting := false + if _, statErr := os.Stat(destDir); statErr == nil { + hasExisting = true + if err := os.Rename(destDir, trashDir); err != nil { + return fmt.Errorf("preserve existing plugin dir %s: %w", destDir, err) + } + } + + // Move staging into the final location. + if err := os.Rename(stagingDir, destDir); err != nil { + // Best-effort restore: move the old install back if we preserved it. + if hasExisting { + _ = os.Rename(trashDir, destDir) //nolint:errcheck + } + return fmt.Errorf("install plugin dir %s: %w", destDir, err) + } + + // New install is in place — remove the old one (best effort). + _ = os.RemoveAll(trashDir) //nolint:errcheck + return nil +} + +// verifyInstalledVersion checks that the plugin.json in dir declares the +// expected version. It normalises "v" prefixes before comparing, so "v1.0.8" +// and "1.0.8" are treated as equal. +func verifyInstalledVersion(dir, expectedVersion string) error { + installedVersion := readInstalledVersion(dir) + norm := func(v string) string { return strings.TrimPrefix(v, "v") } + if norm(installedVersion) != norm(expectedVersion) { + return fmt.Errorf("installed plugin.json version %q does not match expected %q", installedVersion, expectedVersion) + } + return nil +} + // verifyInstalledPlugin validates the installed plugin.json using the engine's // manifest loader and checks that the binary exists and is executable. func verifyInstalledPlugin(destDir, pluginName string) error { diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index 05525dcb..16e686da 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -474,7 +474,282 @@ func makeTestManifest(name, downloadURL, sha256sum string) *RegistryManifest { } } -// ---- verifyChecksum error format ---- +// makeTestTarGzVersioned builds a minimal .tar.gz using the GoReleaser platform- +// suffix naming convention (e.g. myplugin-linux-amd64/myplugin-linux-amd64) so +// ensurePluginBinary must rename the binary. version and binaryContent allow +// callers to distinguish upgrades from fresh installs. +func makeTestTarGzVersioned(t *testing.T, pluginName, version string, binaryContent []byte) []byte { + t.Helper() + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + + // Use the GoReleaser platform-suffix convention so ensurePluginBinary must + // rename the file (rather than finding it already correctly named). + topDir := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + binaryName := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + pjContent := fmt.Sprintf(`{"name":%q,"version":%q,"author":"test","description":"test plugin"}`, pluginName, version) + addTestTarFile(t, tw, topDir+"/plugin.json", 0640, []byte(pjContent)) + addTestTarFile(t, tw, topDir+"/"+binaryName, 0750, binaryContent) + + if err := tw.Close(); err != nil { + t.Fatalf("close tar: %v", err) + } + if err := gw.Close(); err != nil { + t.Fatalf("close gzip: %v", err) + } + return buf.Bytes() +} + +// makeTestManifestVersioned is like makeTestManifest but accepts an explicit version. +func makeTestManifestVersioned(name, version, downloadURL, sha256sum string) *RegistryManifest { + m := makeTestManifest(name, downloadURL, sha256sum) + m.Version = version + return m +} + +// ---- stale-binary upgrade tests (issue: wfctl plugin install reports upgraded +// plugin but leaves stale executable) ---- + +// TestInstallPluginFromManifest_UpgradeReplacesStaleBinary verifies that +// upgrading a plugin via registry manifest atomically replaces the previous +// installation. The old binary (from a GoReleaser platform-suffix tarball) must +// be gone after the upgrade; only the new binary should exist. +func TestInstallPluginFromManifest_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "myplugin" + + oldBinary := []byte("#!/bin/sh\necho v1.0.6\n") + newBinary := []byte("#!/bin/sh\necho v1.0.8\n") + + oldTar := makeTestTarGzVersioned(t, pluginName, "1.0.6", oldBinary) + newTar := makeTestTarGzVersioned(t, pluginName, "1.0.8", newBinary) + + var serveData []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(serveData) + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // First install (v1.0.6). + serveData = oldTar + oldSum := sha256sum(oldTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifestVersioned(pluginName, "1.0.6", srv.URL+"/p.tar.gz", oldSum), nil, false); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after v1 install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade to v1.0.8 — the new tarball uses the GoReleaser suffix name. + serveData = newTar + newSum := sha256sum(newTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifestVersioned(pluginName, "1.0.8", srv.URL+"/p.tar.gz", newSum), nil, false); err != nil { + t.Fatalf("upgrade install: %v", err) + } + + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } + + // Installed version in plugin.json must match v1.0.8. + if got := readInstalledVersion(filepath.Join(pluginDir, pluginName)); got != "1.0.8" { + t.Errorf("installed version = %q, want 1.0.8", got) + } + + // The old platform-suffix binary must NOT be present (no stale files). + staleName := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + if _, err := os.Stat(filepath.Join(pluginDir, pluginName, staleName)); !os.IsNotExist(err) { + t.Errorf("stale GoReleaser-named binary %q still present after upgrade", staleName) + } +} + +// TestInstallFromURL_UpgradeReplacesStaleBinary verifies the same invariant for +// the --url install path. +func TestInstallFromURL_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "urlplugin" + + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + + oldBinary := []byte("#!/bin/sh\necho old-url\n") + newBinary := []byte("#!/bin/sh\necho new-url\n") + + oldTar := makeTestTarGzVersioned(t, pluginName, "2.0.0", oldBinary) + newTar := makeTestTarGzVersioned(t, pluginName, "3.0.0", newBinary) + + var serveData []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(serveData) + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // First install. + serveData = oldTar + if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(oldTar), false); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade. + serveData = newTar + if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(newTar), false); err != nil { + t.Fatalf("upgrade install: %v", err) + } + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after URL upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } +} + +// TestInstallFromLocal_UpgradeReplacesStaleBinary verifies the same invariant +// for the --local install path. +func TestInstallFromLocal_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "localplugin" + + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + + pluginDir := t.TempDir() + + makeLocalSrc := func(version string, binaryContent []byte) string { + src := t.TempDir() + pj := fmt.Sprintf(`{"name":%q,"version":%q,"author":"t","description":"t"}`, pluginName, version) + if err := os.WriteFile(filepath.Join(src, "plugin.json"), []byte(pj), 0640); err != nil { + t.Fatalf("write plugin.json: %v", err) + } + if err := os.WriteFile(filepath.Join(src, pluginName), binaryContent, 0750); err != nil { + t.Fatalf("write binary: %v", err) + } + return src + } + + oldBinary := []byte("#!/bin/sh\necho local-v1\n") + newBinary := []byte("#!/bin/sh\necho local-v2\n") + + // First install. + if err := installFromLocal(makeLocalSrc("1.0.0", oldBinary), pluginDir); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade. + if err := installFromLocal(makeLocalSrc("2.0.0", newBinary), pluginDir); err != nil { + t.Fatalf("upgrade install: %v", err) + } + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after local upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } +} + +// TestInstallPluginFromManifest_StagingCleanedUpOnFailure verifies that if the +// install fails (e.g. download error) the staging directory is removed and the +// original installation is left intact. +func TestInstallPluginFromManifest_StagingCleanedUpOnFailure(t *testing.T) { + const pluginName = "myplugin" + + oldBinary := []byte("#!/bin/sh\necho stable\n") + oldTar := makeTestTarGz(t, pluginName) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/old.tar.gz": + _, _ = w.Write(oldTar) + default: + http.Error(w, "not found", http.StatusNotFound) + } + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // Install stable version first. + oldSum := sha256sum(oldTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifest(pluginName, srv.URL+"/old.tar.gz", oldSum), nil, false); err != nil { + t.Fatalf("first install: %v", err) + } + // Overwrite binary with sentinel so we can verify it's preserved. + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + if err := os.WriteFile(binaryPath, oldBinary, 0750); err != nil { + t.Fatalf("write sentinel: %v", err) + } + + // Attempt upgrade with a failing URL. + badManifest := makeTestManifest(pluginName, srv.URL+"/nonexistent.tar.gz", strings.Repeat("0", 64)) + if err := installPluginFromManifest(pluginDir, pluginName, badManifest, nil, false); err == nil { + t.Fatal("expected upgrade with non-existent URL to fail, but it succeeded") + } + + // Staging directory must be cleaned up. + stagingDir := filepath.Join(pluginDir, pluginName+".installing") + if _, err := os.Stat(stagingDir); !os.IsNotExist(err) { + t.Errorf("staging dir %q was not cleaned up after failed install", stagingDir) + } + + // Original installation must be intact. + gotBinary, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("original binary missing after failed upgrade: %v", err) + } + if !bytes.Equal(gotBinary, oldBinary) { + t.Errorf("original binary modified by failed upgrade: got %q, want %q", gotBinary, oldBinary) + } +} + +// sha256sum is a test helper that returns the hex-encoded SHA256 of data. +func sha256sum(data []byte) string { + h := sha256.Sum256(data) + return hex.EncodeToString(h[:]) +} + + func TestVerifyChecksum_MismatchFormat(t *testing.T) { err := verifyChecksum([]byte("data"), strings.Repeat("0", 64)) @@ -597,6 +872,10 @@ func TestInstallFromURL_NonGitHubNoSHAFails(t *testing.T) { } func TestInstallFromURL_WithExpectedSHA256_Correct(t *testing.T) { + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + archiveData := makeTestTarGz(t, "myplugin") h := sha256.Sum256(archiveData) sha := hex.EncodeToString(h[:]) @@ -630,6 +909,10 @@ func TestInstallFromURL_WithExpectedSHA256_Wrong(t *testing.T) { } func TestInstallFromURL_SkipChecksum_NonGitHub(t *testing.T) { + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + archiveData := makeTestTarGz(t, "myplugin") srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(archiveData)