mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-23 18:17:44 +02:00
fix(daemon): mark workspaces root so escaped subprocesses still fail closed (#5044)
Write a persistent daemon-task marker at the workspaces root so a subprocess that lost all MULTICA_* env vars and escaped above its workdir still fails closed instead of falling back to the user's config PAT. Includes daemon-startup pre-ensure, per-task and reuse-path self-heal, torn-marker reclaim, atomic write, and non-fatal degrade. Fixes #5043.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
//
|
||||
|
||||
171
server/internal/daemon/execenv/context_marker_test.go
Normal file
171
server/internal/daemon/execenv/context_marker_test.go
Normal file
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user