From 0e5927af280aadb7dad3fa56c9052c21c869033d Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Mon, 13 Jul 2026 03:55:48 +0000 Subject: [PATCH] fix: warn on writable provider-state ancestors --- internal/adapter/omp/omp_test.go | 10 +++--- internal/adapter/opencode/opencode_test.go | 36 ++++++++++++---------- internal/providerstate/root.go | 22 +++++++++---- internal/providerstate/root_test.go | 26 ++++++++++++++-- 4 files changed, 64 insertions(+), 30 deletions(-) diff --git a/internal/adapter/omp/omp_test.go b/internal/adapter/omp/omp_test.go index 7fb9d71..662ee01 100644 --- a/internal/adapter/omp/omp_test.go +++ b/internal/adapter/omp/omp_test.go @@ -142,18 +142,18 @@ func TestDispatchRejectsIntermediateStateSymlink(t *testing.T) { } } -func TestDispatchRejectsHostileStateBase(t *testing.T) { +func TestDispatchWarnsForWritableStateBase(t *testing.T) { ag, be := newTestAgent(t) base := os.Getenv("XDG_STATE_HOME") if err := os.Chmod(base, 0o772); err != nil { t.Fatal(err) } _, err := ag.Dispatch(context.Background(), adapter.DispatchRequest{Cwd: "/tmp", Mode: "safe"}) - if err == nil { - t.Fatal("group/other-writable XDG_STATE_HOME accepted") + if err != nil { + t.Fatalf("group/other-writable XDG_STATE_HOME blocked: %v", err) } - if len(be.CallsOf("create")) != 0 { - t.Fatal("hostile state base created backend session") + if len(be.CallsOf("create")) != 1 { + t.Fatal("writable state base did not create backend session") } } diff --git a/internal/adapter/opencode/opencode_test.go b/internal/adapter/opencode/opencode_test.go index 4353655..015bc5d 100644 --- a/internal/adapter/opencode/opencode_test.go +++ b/internal/adapter/opencode/opencode_test.go @@ -233,29 +233,19 @@ func TestProviderFilesRejectIntermediateSymlink(t *testing.T) { } } -func TestProviderFilesRejectHostileStateBase(t *testing.T) { +func TestProviderFilesWarnForWritableStateAncestry(t *testing.T) { t.Run("group writable", func(t *testing.T) { base := t.TempDir() if err := os.Chmod(base, 0o770); err != nil { t.Fatal(err) } t.Setenv("XDG_STATE_HOME", base) - if _, err := ensureProviderFiles(); err == nil { - t.Fatal("group-writable XDG_STATE_HOME accepted") + if _, err := ensureProviderFiles(); err != nil { + t.Fatalf("group-writable XDG_STATE_HOME blocked: %v", err) } }) - t.Run("symlink", func(t *testing.T) { - parent, target := t.TempDir(), t.TempDir() - base := filepath.Join(parent, "state") - if err := os.Symlink(target, base); err != nil { - t.Fatal(err) - } - t.Setenv("XDG_STATE_HOME", base) - if _, err := ensureProviderFiles(); err == nil { - t.Fatal("symlinked XDG_STATE_HOME accepted") - } - }) - t.Run("hostile parent", func(t *testing.T) { + + t.Run("writable parent", func(t *testing.T) { parent := filepath.Join(t.TempDir(), "shared") if err := os.Mkdir(parent, 0o777); err != nil { t.Fatal(err) @@ -264,8 +254,22 @@ func TestProviderFilesRejectHostileStateBase(t *testing.T) { t.Fatal(err) } t.Setenv("XDG_STATE_HOME", filepath.Join(parent, "state")) + if _, err := ensureProviderFiles(); err != nil { + t.Fatalf("state base under writable non-sticky parent blocked: %v", err) + } + }) +} + +func TestProviderFilesRejectSymlinkedStateBase(t *testing.T) { + t.Run("symlink", func(t *testing.T) { + parent, target := t.TempDir(), t.TempDir() + base := filepath.Join(parent, "state") + if err := os.Symlink(target, base); err != nil { + t.Fatal(err) + } + t.Setenv("XDG_STATE_HOME", base) if _, err := ensureProviderFiles(); err == nil { - t.Fatal("state base under writable non-sticky parent accepted") + t.Fatal("symlinked XDG_STATE_HOME accepted") } }) } diff --git a/internal/providerstate/root.go b/internal/providerstate/root.go index 91de4ed..bc6d4dd 100644 --- a/internal/providerstate/root.go +++ b/internal/providerstate/root.go @@ -1,7 +1,6 @@ -// Package providerstate secures the persistent root used for provider-owned -// launch metadata. Once the base and every controlled child are owned by the -// current user and not group/other-writable, another local user cannot swap a -// checked component between validation and use. +// Package providerstate validates the persistent root used for provider-owned +// launch metadata. Ownership, file type, and symlink violations fail closed; +// writable ancestors are reported while UAM-owned children remain owner-only. package providerstate import ( @@ -9,11 +8,17 @@ import ( "os" "path/filepath" "strings" + "sync" "syscall" + + uamlog "github.com/RandomCodeSpace/unified-agent-manager/internal/log" ) +var warnedWritableAncestors sync.Map + // EnsureTrustedBase creates base when absent and verifies that it is a real, -// current-user-owned directory with no group/other write permission. +// current-user-owned directory. Group/other-writable ancestors emit a warning +// because some managed systems do not allow users to change those modes. func EnsureTrustedBase(base string) error { return walkTrustedPath(base, true) } @@ -134,7 +139,12 @@ func verifyComponent(path string, info os.FileInfo) error { if info.Mode().Perm()&0o022 != 0 { rootSticky := uid == 0 && info.Mode()&os.ModeSticky != 0 if !rootSticky { - return fmt.Errorf("provider state ancestor %s is group/other-writable (%04o)", path, info.Mode().Perm()) + if _, loaded := warnedWritableAncestors.LoadOrStore(path, struct{}{}); !loaded { + uamlog.Warn("provider state ancestor is group/other-writable", + "path", path, + "mode", fmt.Sprintf("%04o", info.Mode().Perm()), + ) + } } } return nil diff --git a/internal/providerstate/root_test.go b/internal/providerstate/root_test.go index a9046a8..fa80d90 100644 --- a/internal/providerstate/root_test.go +++ b/internal/providerstate/root_test.go @@ -1,10 +1,15 @@ package providerstate import ( + "bytes" + "log/slog" "os" "path/filepath" + "strings" "syscall" "testing" + + uamlog "github.com/RandomCodeSpace/unified-agent-manager/internal/log" ) func TestRootOwnedSymlinkPolicyIsAncestorOnly(t *testing.T) { @@ -30,7 +35,7 @@ func TestTrustedBaseRejectsSymlinkAtBase(t *testing.T) { } } -func TestResolvedAncestryRejectsWritableComponent(t *testing.T) { +func TestResolvedAncestryWarnsForWritableComponent(t *testing.T) { unsafe := filepath.Join(t.TempDir(), "unsafe") if err := os.Mkdir(unsafe, 0o700); err != nil { t.Fatal(err) @@ -38,8 +43,23 @@ func TestResolvedAncestryRejectsWritableComponent(t *testing.T) { if err := os.Chmod(unsafe, 0o770); err != nil { t.Fatal(err) } - if err := verifyResolvedAncestry(unsafe); err == nil { - t.Fatal("writable resolved target ancestry accepted") + var output bytes.Buffer + previous := uamlog.SetLogger(slog.New(slog.NewTextHandler(&output, nil))) + t.Cleanup(func() { uamlog.SetLogger(previous) }) + + if err := verifyResolvedAncestry(unsafe); err != nil { + t.Fatalf("writable resolved target ancestry blocked: %v", err) + } + if err := verifyResolvedAncestry(unsafe); err != nil { + t.Fatalf("repeated writable ancestry check blocked: %v", err) + } + warning := output.String() + if !strings.Contains(warning, "provider state ancestor is group/other-writable") || + !strings.Contains(warning, unsafe) || !strings.Contains(warning, "0770") { + t.Fatalf("missing actionable writable-ancestor warning: %q", warning) + } + if count := strings.Count(warning, "provider state ancestor is group/other-writable"); count != 1 { + t.Fatalf("writable ancestor warning count = %d, want 1: %q", count, warning) } }