diff --git a/server/cmd/multica/cmd_agent_test.go b/server/cmd/multica/cmd_agent_test.go index 0a23c8cf87..bfb4eec523 100644 --- a/server/cmd/multica/cmd_agent_test.go +++ b/server/cmd/multica/cmd_agent_test.go @@ -63,6 +63,71 @@ func chdirWithDaemonTaskMarker(t *testing.T) { }) } +// TestNewAPIClient_WorkdirParentEscapeFailsClosed reproduces the confirmed +// impersonation escape: a sandbox fault strips every MULTICA_* env var from +// an agent subprocess, which then runs `multica` from the *parent* directory +// of its workdir. The per-workdir marker sits below cwd, so the upward walk +// used to find no daemon signal and silently fell back to the user's config +// PAT, posting agent writes as the workspace owner (author_type=member). +// +// The daemon now writes a persistent marker at the workspaces root +// (execenv.EnsureWorkspacesRootMarker), so every cwd inside the +// daemon-owned tree — task dir, workspace dir, sibling task dirs, the root +// itself — carries the fail-closed signal. This test builds that tree shape +// and asserts the CLI refuses the config-PAT fallback from the escaped cwd. +func TestNewAPIClient_WorkdirParentEscapeFailsClosed(t *testing.T) { + // Seed a user config with a mul_ PAT that must never be picked up. + t.Setenv("HOME", t.TempDir()) + if err := cli.SaveCLIConfig(cli.CLIConfig{Token: "mul_owner_pat"}); err != nil { + t.Fatalf("seed config: %v", err) + } + + // Daemon-owned tree: {root}/.multica marker + {root}/{ws}/{task}/workdir + // with its own per-workdir marker, exactly as the daemon lays it out. + root := t.TempDir() + if err := execenv.EnsureWorkspacesRootMarker(root); err != nil { + t.Fatalf("write root marker: %v", err) + } + taskDir := filepath.Join(root, "ws-1", "task-1") + workDir := filepath.Join(taskDir, "workdir") + workdirMarker := filepath.Join(workDir, execenv.TaskContextMarkerRelPath) + if err := os.MkdirAll(filepath.Dir(workdirMarker), 0o755); err != nil { + t.Fatalf("create workdir marker dir: %v", err) + } + data := []byte(`{"managed_by":"` + execenv.TaskContextMarkerManagedBy + `"}`) + if err := os.WriteFile(workdirMarker, data, 0o644); err != nil { + t.Fatalf("write workdir marker: %v", err) + } + + // The escape: cwd is the workdir's parent, all daemon env vars gone. + prev, err := os.Getwd() + if err != nil { + t.Fatalf("get cwd: %v", err) + } + if err := os.Chdir(taskDir); err != nil { + t.Fatalf("chdir task dir: %v", err) + } + t.Cleanup(func() { + if err := os.Chdir(prev); err != nil { + t.Fatalf("restore cwd: %v", err) + } + }) + t.Setenv("MULTICA_AGENT_ID", "") + t.Setenv("MULTICA_TASK_ID", "") + t.Setenv("MULTICA_DAEMON_PORT", "") + t.Setenv("MULTICA_TOKEN", "") + t.Setenv("MULTICA_SERVER_URL", "http://127.0.0.1:8080") + + if got := resolveToken(testCmd()); got != "" { + t.Fatalf("resolveToken() = %q, want empty (config PAT must not leak into an escaped daemon subprocess)", got) + } + if _, err := newAPIClient(testCmd()); err == nil { + t.Fatal("newAPIClient(): expected fail-closed error from workdir-parent escape, got nil") + } else if !strings.Contains(err.Error(), "mat_") { + t.Fatalf("error should demand a task-scoped mat_ token; got %q", err.Error()) + } +} + // TestNewAPIClient_LeftoverMarkerActionableError verifies that a stale // daemon-task marker with no daemon env (the local_directory crash-leftover // case) fails closed with an actionable message that names the marker file, diff --git a/server/internal/daemon/daemon.go b/server/internal/daemon/daemon.go index 727055736e..dba822ebdc 100644 --- a/server/internal/daemon/daemon.go +++ b/server/internal/daemon/daemon.go @@ -757,6 +757,16 @@ func (d *Daemon) Run(ctx context.Context) error { "launched_by", d.cfg.LaunchedBy, ) + // Mark the daemon-owned workspaces tree before any task runs. A sandbox + // fault can strip every MULTICA_* env var from an agent subprocess; the + // per-workdir marker then only protects cwds inside the workdir, and a + // subprocess that escaped to the workdir's parent would fall back to the + // user's config PAT. The root marker makes the CLI fail closed anywhere + // under the tree. Non-fatal: Prepare re-ensures it per task. + if err := execenv.EnsureWorkspacesRootMarker(d.cfg.WorkspacesRoot); err != nil { + d.logger.Warn("workspaces root marker not written; CLI fail-closed guard limited to task workdirs", "error", err) + } + // Load auth token from CLI config. if err := d.resolveAuth(); err != nil { return err @@ -3592,6 +3602,7 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i } if task.PriorWorkDir != "" && localAssignment == nil && !task.IsLeaderTask { env = execenv.Reuse(execenv.ReuseParams{ + WorkspacesRoot: d.cfg.WorkspacesRoot, WorkDir: task.PriorWorkDir, Provider: provider, CodexVersion: codexVersion, diff --git a/server/internal/daemon/execenv/context.go b/server/internal/daemon/execenv/context.go index 70c1529814..31dc31cb1d 100644 --- a/server/internal/daemon/execenv/context.go +++ b/server/internal/daemon/execenv/context.go @@ -28,6 +28,94 @@ type taskContextMarkerFile struct { IssueID string `json:"issue_id,omitempty"` } +// EnsureWorkspacesRootMarker writes a persistent daemon-task marker at +// {workspacesRoot}/.multica/daemon_task_context.json. +// +// The per-workdir marker only protects `multica` invocations whose cwd is +// inside the workdir, because the CLI discovers markers by walking *up* from +// cwd. A sandboxed subprocess that lost every MULTICA_* env var and escaped +// to the workdir's parent directory sits above that marker, finds no daemon +// signal, and would fall back to the user's config PAT — a confirmed +// impersonation path. Every directory under workspacesRoot is daemon-owned, +// so a marker at the root puts the entire tree back under the fail-closed +// guard without touching any directory a user works in. +// +// A pre-existing marker owned by the daemon is left untouched, so the shared, +// node-wide file is not rewritten on every task start. A truncated or otherwise +// unparseable file — the signature of a torn os.WriteFile from a daemon killed +// mid-write — is reclaimed and rewritten, so a crash can never brick the guard +// for the whole node; only a *parseable* marker owned by something else is +// treated as genuinely foreign and refused. The (re)write is atomic +// (temp file + rename): a concurrent CLI walking up from an escaped cwd sees +// either the old bytes or the complete new file, never a half-written marker it +// would misread as "no signal" and fall open on. +func EnsureWorkspacesRootMarker(workspacesRoot string) error { + if strings.TrimSpace(workspacesRoot) == "" { + return errors.New("execenv: workspaces root is required") + } + path := filepath.Join(workspacesRoot, TaskContextMarkerRelPath) + if existing, err := os.ReadFile(path); err == nil { + var marker taskContextMarkerFile + if json.Unmarshal(existing, &marker) == nil { + if marker.ManagedBy == TaskContextMarkerManagedBy { + return nil + } + // Parseable but owned by something else: never clobber it. + return fmt.Errorf("foreign file at workspaces root marker path %s; refusing to overwrite", path) + } + // Unparseable content is almost certainly a torn write of our own + // marker; fall through to reclaim it below. + } else if !os.IsNotExist(err) { + // A real read error (e.g. a directory at the path) is not a signal we + // can safely overwrite; surface it. Callers degrade non-fatally. + return fmt.Errorf("read workspaces root marker %s: %w", path, err) + } + payload := taskContextMarkerFile{ManagedBy: TaskContextMarkerManagedBy} + data, err := json.MarshalIndent(payload, "", " ") + if err != nil { + return fmt.Errorf("marshal workspaces root marker: %w", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return fmt.Errorf("create workspaces root marker dir: %w", err) + } + if err := writeWorkspacesRootMarkerAtomic(path, data); err != nil { + return fmt.Errorf("write workspaces root marker: %w", err) + } + return nil +} + +// writeWorkspacesRootMarkerAtomic writes data to path via a same-directory temp +// file plus a rename, so a concurrent reader observes either the old file or the +// complete new one — never a partial write. Mirrors the daemon-id / CLI-config +// write idiom (see writeDaemonIDFile). Perm is 0644 because the CLI's upward +// walk must be able to read the marker from a subprocess that may run under a +// different uid; the payload is non-secret. +func writeWorkspacesRootMarkerAtomic(path string, data []byte) error { + tmp, err := os.CreateTemp(filepath.Dir(path), ".daemon_task_context-*.json.tmp") + if err != nil { + return fmt.Errorf("create temp workspaces root marker: %w", err) + } + tmpPath := tmp.Name() + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpPath) + return fmt.Errorf("write temp workspaces root marker: %w", err) + } + if err := tmp.Close(); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("close temp workspaces root marker: %w", err) + } + if err := os.Chmod(tmpPath, 0o644); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("chmod temp workspaces root marker: %w", err) + } + if err := os.Rename(tmpPath, path); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("rename workspaces root marker: %w", err) + } + return nil +} + // writeContextFiles renders and writes .agent_context/issue_context.md and // skills into the appropriate provider-native location. // diff --git a/server/internal/daemon/execenv/context_marker_test.go b/server/internal/daemon/execenv/context_marker_test.go new file mode 100644 index 0000000000..50ffdb9b84 --- /dev/null +++ b/server/internal/daemon/execenv/context_marker_test.go @@ -0,0 +1,171 @@ +package execenv + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestEnsureWorkspacesRootMarker covers the root-level daemon marker that +// protects the whole workspaces tree. Regression for the confirmed escape +// where a sandboxed subprocess lost every MULTICA_* env var and ran +// `multica` from the workdir's *parent* directory: the per-workdir marker +// sits below cwd, so the CLI's upward walk found no daemon signal and fell +// back to the user's config PAT, misattributing agent writes to a member. +func TestEnsureWorkspacesRootMarker(t *testing.T) { + t.Run("writes a valid marker into an empty root", func(t *testing.T) { + root := t.TempDir() + if err := EnsureWorkspacesRootMarker(root); err != nil { + t.Fatalf("EnsureWorkspacesRootMarker: %v", err) + } + data, err := os.ReadFile(filepath.Join(root, TaskContextMarkerRelPath)) + if err != nil { + t.Fatalf("read root marker: %v", err) + } + var marker struct { + ManagedBy string `json:"managed_by"` + } + if err := json.Unmarshal(data, &marker); err != nil { + t.Fatalf("unmarshal root marker: %v\n%s", err, string(data)) + } + if marker.ManagedBy != TaskContextMarkerManagedBy { + t.Fatalf("managed_by = %q, want %q", marker.ManagedBy, TaskContextMarkerManagedBy) + } + }) + + t.Run("is idempotent when a matching marker exists", func(t *testing.T) { + root := t.TempDir() + if err := EnsureWorkspacesRootMarker(root); err != nil { + t.Fatalf("first EnsureWorkspacesRootMarker: %v", err) + } + if err := EnsureWorkspacesRootMarker(root); err != nil { + t.Fatalf("second EnsureWorkspacesRootMarker: %v", err) + } + }) + + t.Run("refuses to overwrite a foreign file", func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, TaskContextMarkerRelPath) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + foreign := []byte(`{"managed_by":"someone-else"}`) + if err := os.WriteFile(path, foreign, 0o644); err != nil { + t.Fatalf("write foreign file: %v", err) + } + if err := EnsureWorkspacesRootMarker(root); err == nil { + t.Fatal("expected error for foreign file at marker path, got nil") + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("re-read foreign file: %v", err) + } + if string(data) != string(foreign) { + t.Fatalf("foreign file was clobbered: %s", string(data)) + } + }) + + t.Run("self-heals a corrupt marker from a torn write", func(t *testing.T) { + // A daemon killed mid os.WriteFile leaves an empty or truncated marker. + // The old code treated any unparseable file like a foreign one and + // refused it forever, leaving the whole node without a root guard. The + // unparseable content is our own torn write, so it must be reclaimed. + for _, corrupt := range []string{"", "{", "not-json", `{"managed_by":`} { + root := t.TempDir() + path := filepath.Join(root, TaskContextMarkerRelPath) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, []byte(corrupt), 0o644); err != nil { + t.Fatalf("seed corrupt marker %q: %v", corrupt, err) + } + if err := EnsureWorkspacesRootMarker(root); err != nil { + t.Fatalf("reclaim corrupt marker %q: unexpected error %v", corrupt, err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read reclaimed marker: %v", err) + } + var marker struct { + ManagedBy string `json:"managed_by"` + } + if err := json.Unmarshal(data, &marker); err != nil { + t.Fatalf("reclaimed marker not valid JSON for input %q: %v\n%s", corrupt, err, string(data)) + } + if marker.ManagedBy != TaskContextMarkerManagedBy { + t.Fatalf("managed_by = %q, want %q (input %q)", marker.ManagedBy, TaskContextMarkerManagedBy, corrupt) + } + } + }) + + t.Run("rejects empty root", func(t *testing.T) { + if err := EnsureWorkspacesRootMarker(""); err == nil { + t.Fatal("expected error for empty workspaces root, got nil") + } + }) +} + +// TestPrepare_WritesWorkspacesRootMarker verifies Prepare self-heals the +// root-level marker on every task start, so a marker deleted while the +// daemon is running is restored before the next agent spawns. +func TestPrepare_WritesWorkspacesRootMarker(t *testing.T) { + root := t.TempDir() + env, err := Prepare(PrepareParams{ + WorkspacesRoot: root, + WorkspaceID: "ws-test-001", + TaskID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + AgentName: "Test Agent", + Task: TaskContextForEnv{ + IssueID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + }, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + data, err := os.ReadFile(filepath.Join(root, TaskContextMarkerRelPath)) + if err != nil { + t.Fatalf("read root marker after Prepare: %v", err) + } + if !strings.Contains(string(data), TaskContextMarkerManagedBy) { + t.Fatalf("root marker missing managed_by discriminator: %s", string(data)) + } +} + +// TestReuse_SelfHealsWorkspacesRootMarker verifies the root marker is restored +// on the reuse path too: a marker deleted while the daemon runs must be back +// before a reused task spawns, not only after the next fresh Prepare. +func TestReuse_SelfHealsWorkspacesRootMarker(t *testing.T) { + root := t.TempDir() + env, err := Prepare(PrepareParams{ + WorkspacesRoot: root, + WorkspaceID: "ws-reuse-001", + TaskID: "b2c3d4e5-f6a7-8901-bcde-f23456789012", + AgentName: "Reuse Agent", + Task: TaskContextForEnv{IssueID: "b2c3d4e5-f6a7-8901-bcde-f23456789012"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + rootMarker := filepath.Join(root, TaskContextMarkerRelPath) + if err := os.Remove(rootMarker); err != nil { + t.Fatalf("remove root marker: %v", err) + } + + reused := Reuse(ReuseParams{ + WorkspacesRoot: root, + WorkDir: env.WorkDir, + Task: TaskContextForEnv{IssueID: "b2c3d4e5-f6a7-8901-bcde-f23456789012"}, + }, testLogger()) + if reused == nil { + t.Fatal("Reuse returned nil for an existing workdir") + } + if _, err := os.Stat(rootMarker); err != nil { + t.Fatalf("Reuse did not restore the root marker: %v", err) + } +} diff --git a/server/internal/daemon/execenv/execenv.go b/server/internal/daemon/execenv/execenv.go index f565bcc214..db2225e740 100644 --- a/server/internal/daemon/execenv/execenv.go +++ b/server/internal/daemon/execenv/execenv.go @@ -196,6 +196,16 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { envRoot := filepath.Join(params.WorkspacesRoot, params.WorkspaceID, shortID(params.TaskID)) + // Self-heal the root-level daemon marker on every task start so a marker + // removed while the daemon runs is restored before the agent spawns. The + // per-workdir marker written below only covers cwds inside the workdir; + // the root marker keeps the CLI fail-closed guard active for subprocesses + // that lose all MULTICA_* env vars AND escape above the workdir. Non-fatal: + // without it the workdir marker still protects the common case. + if err := EnsureWorkspacesRootMarker(params.WorkspacesRoot); err != nil && logger != nil { + logger.Warn("execenv: workspaces root marker not written; fail-closed guard limited to the task workdir", "error", err) + } + // Remove existing env if present (defensive — task IDs are unique). if _, err := os.Stat(envRoot); err == nil { if err := os.RemoveAll(envRoot); err != nil { @@ -295,10 +305,15 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { // the per-provider knobs (CodexVersion, OpenclawBin) so callers can pass // the same resolved binary path on both first-run and reuse paths. type ReuseParams struct { - WorkDir string - Provider string - CodexVersion string // only used when Provider == "codex" - OpenclawBin string // only used when Provider == "openclaw"; empty = PATH lookup + // WorkspacesRoot is the daemon-owned root under which all task envs live. + // Passed on reuse so the root-level fail-closed marker is self-healed here + // too — a marker removed while the daemon runs is restored before a reused + // task spawns, not only on the fresh-Prepare path. + WorkspacesRoot string + WorkDir string + Provider string + CodexVersion string // only used when Provider == "codex" + OpenclawBin string // only used when Provider == "openclaw"; empty = PATH lookup // McpConfig is the agent's saved `mcp_config` JSON. Reused on reuse so a // freshly-saved managed set re-materialises into the wrapper before the // task starts — without this a stale wrapper from a prior run would keep @@ -323,6 +338,17 @@ func Reuse(params ReuseParams, logger *slog.Logger) *Environment { return nil } + // Self-heal the root-level daemon marker on the reuse path too, so a marker + // removed while the daemon runs is restored before a reused task spawns — + // otherwise reuse could run without the fail-closed guard until the next + // fresh Prepare. Non-fatal: the per-workdir marker still protects the common + // case, and an empty WorkspacesRoot (legacy callers) simply skips this. + if params.WorkspacesRoot != "" { + if err := EnsureWorkspacesRootMarker(params.WorkspacesRoot); err != nil && logger != nil { + logger.Warn("execenv: workspaces root marker not written on reuse; fail-closed guard limited to the task workdir", "error", err) + } + } + rootDir := filepath.Dir(params.WorkDir) if params.LocalDirectory { // For local_directory tasks the user's WorkDir is unrelated to