From 4766af3b2345b29605f89f1d8de161da0a976714 Mon Sep 17 00:00:00 2001 From: naxty <9141879+naxty@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:39:38 +0200 Subject: [PATCH] fix(update): refuse to initialize an install root instead of wiping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `formae update` derived the orbital tree root from the running binary's location and, when that path carried no tree, offered to initialize one. orbital's Initialize is a force-init: it removes the root recursively before recreating it. A formae reached through a copy or symlink in a foreign prefix (a package manager's bin, /usr/local/bin, a build tree) resolved to that prefix, so confirming the prompt deleted an unrelated directory — and the update then reported success. update now refuses with the resolved path and the ways out; creating install roots stays with the installer, which is the only component that knows it owns the target. Root derivation also resolves symlinks first, so a shim on PATH resolves to the real install rather than making its own prefix the managed root. --- internal/cli/update/update.go | 52 +++++++---------------- internal/cli/update/update_test.go | 44 +++++-------------- internal/opsmgr/opsmgr.go | 16 ++++++- internal/opsmgr/treepath_internal_test.go | 40 +++++++++++++++++ 4 files changed, 82 insertions(+), 70 deletions(-) create mode 100644 internal/opsmgr/treepath_internal_test.go diff --git a/internal/cli/update/update.go b/internal/cli/update/update.go index 1f7b10bcb..85ee9ef0f 100644 --- a/internal/cli/update/update.go +++ b/internal/cli/update/update.go @@ -82,24 +82,20 @@ func ackLine(w io.Writer, tty bool, th *theme.Theme, m components.AckMarker, tex _, _ = fmt.Fprintln(w, components.AckLinePlain(m, text)) } -// runInitConfirmDecision asks the user whether to initialize the managed root -// when none is detected. Returns (true, nil) to proceed, (false, nil) to -// abort, or a non-nil error on D8 violation. -// -// D8 policy: non-TTY without --yes → error. -func runInitConfirmDecision(w io.Writer, th *theme.Theme, s updateSeams, path string, yes bool) (bool, error) { - if yes { - return true, nil - } - if !s.isInteractiveFn() { - return false, fmt.Errorf("interactive input requires a TTY — pass --yes to proceed non-interactively") - } - title := fmt.Sprintf("No managed installation root at %s. Initialize?", path) - ok, err := s.runConfirmFn(th, title, "") - if err != nil { - return false, err - } - return ok, nil +// errNoRoot is the message shown when the resolved tree path carries no +// orbital tree. `update` must never create one: the path is derived from the +// running binary's location, so a formae reached through a copy or symlink in +// a foreign prefix (a Homebrew bin, /usr/local/bin, a build tree) resolves to +// that prefix. Initializing there is destructive — orbital's force-init wipes +// the root — so the only safe answer is to refuse and let the installer own +// root creation. +func errNoRoot(path string) error { + return fmt.Errorf( + "no formae installation root at %s\n\n"+ + "formae update installs into the tree next to the running binary and never creates one.\n"+ + "Reinstall with the official installer, run the formae in your install root (e.g. /opt/pel/bin/formae),\n"+ + "or set %s to an existing install root.", + path, opsmgr.FormaePelRootEnv) } // runUpdateFlow is the testable core of the interactive update flow. @@ -187,25 +183,9 @@ func UpdateCmd() *cobra.Command { th := themeFor(a) - // Init root if needed — D8 gated confirm. + // Never initialize a root here — see errNoRoot. if !orb.Ready() { - seams := updateSeams{ - isInteractiveFn: isInteractive, - runConfirmFn: runConfirm, - // stopAgentFn and installFn are not used in the init path. - } - proceed, err := runInitConfirmDecision(os.Stdout, th, seams, orb.Path(), yes) - if err != nil { - return err - } - if !proceed { - return nil - } - - _, err = orb.Initialize() - if err != nil { - return err - } + return errNoRoot(orb.Path()) } err = orb.Refresh() diff --git a/internal/cli/update/update_test.go b/internal/cli/update/update_test.go index 55543922c..0296f51c9 100644 --- a/internal/cli/update/update_test.go +++ b/internal/cli/update/update_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/platform-engineering-labs/formae/internal/cli/tui/theme" + "github.com/platform-engineering-labs/formae/internal/opsmgr" "github.com/platform-engineering-labs/orbital/opm/records" "github.com/platform-engineering-labs/orbital/ops" ) @@ -131,42 +132,19 @@ func TestUpdateFlow_ConsequenceBeforeConfirm(t *testing.T) { } // ---------------------------------------------------------------------------- -// D8 gate tests — runInitConfirmDecision +// no-root policy: update must refuse, never initialize // ---------------------------------------------------------------------------- -// Non-TTY without --yes must error with "interactive input requires a TTY". -func TestInitConfirm_NonTTY_NoYes(t *testing.T) { - stub := &stubInstaller{} - var buf captureWriter - th := theme.New("formae") - - _, err := runInitConfirmDecision(&buf, th, seamsFor(stub, false, false), "/some/path", false) +// The resolved tree path is derived from the running binary's location, so a +// formae reached through a foreign prefix resolves to that prefix. `update` +// must refuse with actionable guidance instead of initializing a tree there — +// orbital's force-init wipes the root it is handed. +func TestErrNoRoot_RefusesAndGuides(t *testing.T) { + err := errNoRoot("/opt/homebrew") require.Error(t, err) - assert.Contains(t, err.Error(), "interactive input requires a TTY") -} - -// Non-TTY with --yes must proceed (return true, nil) without calling confirm. -func TestInitConfirm_NonTTY_WithYes(t *testing.T) { - stub := &stubInstaller{} - var confirmCalled bool - - seams := updateSeams{ - isInteractiveFn: func() bool { return false }, - runConfirmFn: func(_ *theme.Theme, _, _ string) (bool, error) { - confirmCalled = true - return true, nil - }, - stopAgentFn: stub.stop, - installFn: stub.install, - } - - var buf captureWriter - th := theme.New("formae") - - result, err := runInitConfirmDecision(&buf, th, seams, "/some/path", true) - require.NoError(t, err) - assert.True(t, result, "should proceed when --yes on non-TTY") - assert.False(t, confirmCalled, "confirm must not be called with --yes") + assert.Contains(t, err.Error(), "/opt/homebrew") + assert.Contains(t, err.Error(), "never creates one") + assert.Contains(t, err.Error(), opsmgr.FormaePelRootEnv) } // ---------------------------------------------------------------------------- diff --git a/internal/opsmgr/opsmgr.go b/internal/opsmgr/opsmgr.go index 5f98afeaf..e630ead44 100644 --- a/internal/opsmgr/opsmgr.go +++ b/internal/opsmgr/opsmgr.go @@ -109,6 +109,12 @@ func newManager(logger *slog.Logger, repos []pkgmodel.Repository, channel string // resolveTreePath returns FORMAE_PEL_ROOT when set, otherwise the // directory two levels above the running binary (matches the // /opt/pel/bin/formae → /opt/pel install layout). +// +// Symlinks are resolved first. os.Executable() reports the path the process +// was invoked with — on darwin symlinks are left intact — so a shim such as +// /opt/homebrew/bin/formae → /opt/pel/bin/formae would otherwise make the +// shim's own prefix the tree root, and every install and removal would target +// that foreign directory. func resolveTreePath() (string, error) { if root := os.Getenv(FormaePelRootEnv); root != "" { return root, nil @@ -117,5 +123,13 @@ func resolveTreePath() (string, error) { if err != nil { return "", fmt.Errorf("could not determine binary path: %w", err) } - return filepath.Dir(filepath.Dir(binPath)), nil + return treePathFrom(binPath), nil +} + +// treePathFrom derives the tree root from a binary path, resolving symlinks. +func treePathFrom(binPath string) string { + if resolved, err := filepath.EvalSymlinks(binPath); err == nil { + binPath = resolved + } + return filepath.Dir(filepath.Dir(binPath)) } diff --git a/internal/opsmgr/treepath_internal_test.go b/internal/opsmgr/treepath_internal_test.go new file mode 100644 index 000000000..b64be5d63 --- /dev/null +++ b/internal/opsmgr/treepath_internal_test.go @@ -0,0 +1,40 @@ +// © 2026 Platform Engineering Labs Inc. +// +// SPDX-License-Identifier: FSL-1.1-ALv2 + +//go:build unit + +package opsmgr + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// A formae reached through a shim in a foreign prefix (a Homebrew bin, a +// /usr/local/bin copy) must resolve to the real install root, not to the +// shim's prefix: the derived root is what update installs into and what +// orbital manages, so getting it wrong points package operations at an +// unrelated directory. +func TestTreePathFrom_ResolvesShimToRealRoot(t *testing.T) { + tmp := t.TempDir() + realRoot := filepath.Join(tmp, "pel") + shimRoot := filepath.Join(tmp, "homebrew") + require.NoError(t, os.MkdirAll(filepath.Join(realRoot, "bin"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(shimRoot, "bin"), 0o755)) + + realBin := filepath.Join(realRoot, "bin", "formae") + require.NoError(t, os.WriteFile(realBin, []byte("#!/bin/sh\n"), 0o755)) + shimBin := filepath.Join(shimRoot, "bin", "formae") + require.NoError(t, os.Symlink(realBin, shimBin)) + + // EvalSymlinks resolves /tmp on darwin; compare against the resolved form. + want, err := filepath.EvalSymlinks(realRoot) + require.NoError(t, err) + + require.Equal(t, want, treePathFrom(shimBin), "shim must resolve to the real root") + require.Equal(t, want, treePathFrom(realBin), "direct invocation unchanged") +}