From 1f84949215b73c15407466a10895b36503079286 Mon Sep 17 00:00:00 2001 From: Teo Calin Date: Thu, 1 Oct 2026 17:11:57 +0300 Subject: [PATCH] fix(appstore): `uninstall .` no longer removes every installed app `pilotctl appstore uninstall . --yes` resolved "." to the install root itself: resolveUnder only checks containment, and the root contains itself. The whole root was removed, and since #473 every app's leftover processes were stopped too. App ids are single directory names, so uninstall, caps and the next-steps loaders now go through resolveAppDir, which rejects ".", ".." and anything with a path separator before the containment check. install keeps resolveUnder: its argument may be a bundle directory. Also drop two references to the removed managed-fleet reconcile from the rename comments added in #473, and name the release in the catalogue README. Co-Authored-By: Claude Opus 5.5 --- catalogue/README.md | 4 ++-- cmd/pilotctl/appstore.go | 15 ++++++++++++-- cmd/pilotctl/appstore_catalogue.go | 4 ++-- cmd/pilotctl/appstore_nextsteps.go | 4 ++-- cmd/pilotctl/zz_subprocess_test.go | 32 ++++++++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/catalogue/README.md b/catalogue/README.md index 19a56203..e890b3e2 100644 --- a/catalogue/README.md +++ b/catalogue/README.md @@ -81,11 +81,11 @@ and `publisher` (so existing installs keep their pin and keep running), set `metadata_url` (the tombstone is not installable), and delete the old `apps//` detail dir. The full new entry lives under the new id, and the catalogue is re-signed. A `pilotctl` that knows the fields (this repo's, from -the release after v1.13.10) then: +v1.14.1) then: - omits the old id from `catalogue` (text and `--json`); - on `install `, warns and installs `renamed_to` instead. With - `--version` (a pin, as the managed-fleet reconcile passes) it refuses, naming + `--version` (a pin) it refuses, naming the new id: a pin names a release of one app, and the old id has none; - reports an installed old id in `outdated` as `renamed` (AVAILABLE is the new id). `upgrade --all` skips it, since the hourly updater runs it and the new id diff --git a/cmd/pilotctl/appstore.go b/cmd/pilotctl/appstore.go index 51c921b8..beececeb 100644 --- a/cmd/pilotctl/appstore.go +++ b/cmd/pilotctl/appstore.go @@ -779,7 +779,7 @@ func cmdAppStoreUninstall(args []string) { // crafted id (e.g. "../../something") can't RemoveAll outside the // install tree. Same guard the install/verify paths use. root := appStoreRoot() - dir, err := resolveUnder(root, appID) + dir, err := resolveAppDir(root, appID) if err != nil { fatalHint("invalid_argument", "app ids are single directory names; try `pilotctl appstore list`", @@ -1132,6 +1132,17 @@ func resolveUnder(base, rel string) (string, error) { return joined, nil } +// resolveAppDir confines a user-supplied app id to one directory directly +// under root. resolveUnder alone accepts "." (root itself) and nested paths, +// so `appstore uninstall .` would resolve to the install root and remove +// every installed app. +func resolveAppDir(root, appID string) (string, error) { + if appID == "." || appID == ".." || strings.ContainsAny(appID, `/\`) { + return "", fmt.Errorf("app id must be a single directory name") + } + return resolveUnder(root, appID) +} + func cmdAppStoreInstall(args []string) { if len(args) < 1 { fatalHint("invalid_argument", @@ -1914,7 +1925,7 @@ func cmdAppStoreCaps(args []string) { // appID is user input; confine it to a single entry under the app // store root so a crafted id (e.g. "../../etc") can't read or open // files outside the install tree. - appDir, err := resolveUnder(appStoreRoot(), appID) + appDir, err := resolveAppDir(appStoreRoot(), appID) if err != nil { fatalHint("invalid_argument", "app ids are single directory names; try `pilotctl appstore list`", diff --git a/cmd/pilotctl/appstore_catalogue.go b/cmd/pilotctl/appstore_catalogue.go index 271c6c50..131a83af 100644 --- a/cmd/pilotctl/appstore_catalogue.go +++ b/cmd/pilotctl/appstore_catalogue.go @@ -418,8 +418,8 @@ func resolveInstallTargetVersion(target, wantVersion string) (string, installSou // A pin names a release of one app. The old id has no releases, // and following the rename would install a different app id (with // its own publisher key) than the one the caller checks for - // afterwards — the managed-fleet reconcile would reinstall it - // every cycle. Fail closed and name the new id instead. + // afterwards, so a caller that re-checks would reinstall it every + // time. Fail closed and name the new id instead. return "", installSourceCatalogue, fmt.Errorf("%w: %s was renamed to %s and has no releases of its own; pin %s instead", ErrCatalogueVersionUnavailable, target, e.RenamedTo, e.RenamedTo) } diff --git a/cmd/pilotctl/appstore_nextsteps.go b/cmd/pilotctl/appstore_nextsteps.go index 4a0b67e8..5887cea5 100644 --- a/cmd/pilotctl/appstore_nextsteps.go +++ b/cmd/pilotctl/appstore_nextsteps.go @@ -138,7 +138,7 @@ func loadNextStepsGraph(appID string) *nextStepsGraph { // `pilotctl appstore call ../../etc/x ...` would resolve outside the install // root and we would happily read (and print) whatever we found. resolveUnder // is the same containment guard install and the supervisor use. - dir, err := resolveUnder(appStoreRoot(), appID) + dir, err := resolveAppDir(appStoreRoot(), appID) if err != nil { return nil } @@ -451,7 +451,7 @@ func ensureNextStepsFresh(appID string) { if nextStepsDisabled() { return } - dir, err := resolveUnder(appStoreRoot(), appID) + dir, err := resolveAppDir(appStoreRoot(), appID) if err != nil { return } diff --git a/cmd/pilotctl/zz_subprocess_test.go b/cmd/pilotctl/zz_subprocess_test.go index 2b5e754b..6f1e7801 100644 --- a/cmd/pilotctl/zz_subprocess_test.go +++ b/cmd/pilotctl/zz_subprocess_test.go @@ -279,6 +279,38 @@ func TestCLIAppStoreUninstallRequiresYes(t *testing.T) { } } +// `uninstall .` used to resolve to the install root itself, so --yes removed +// every installed app. An app id is a single directory name. +func TestCLIAppStoreUninstallRejectsRootAndNestedIDs(t *testing.T) { + t.Parallel() + for _, id := range []string{".", "..", "victim/..", "a/b"} { + id := id + t.Run(id, func(t *testing.T) { + t.Parallel() + root := filepath.Join(t.TempDir(), "apps-root") + for _, app := range []string{"victim", "bystander"} { + if err := os.MkdirAll(filepath.Join(root, app), 0o755); err != nil { + t.Fatal(err) + } + } + _, stderr, code := runCLI(t, []string{"appstore", "uninstall", id, "--yes"}, map[string]string{ + "PILOT_APPSTORE_ROOT": root, + }) + if code == 0 { + t.Errorf("uninstall %q --yes succeeded", id) + } + if !strings.Contains(stderr, "invalid app id") { + t.Errorf("expected invalid app id error: %s", stderr) + } + for _, app := range []string{"victim", "bystander"} { + if _, err := os.Stat(filepath.Join(root, app)); err != nil { + t.Errorf("%s should still exist: %v", app, err) + } + } + }) + } +} + func TestCLIAppStoreVerifyMissingBundle(t *testing.T) { t.Parallel() _, stderr, code := runCLI(t, []string{"appstore", "verify", "/no/such/bundle"}, nil)