diff --git a/docs/tool-execution-toctou.md b/docs/tool-execution-toctou.md new file mode 100644 index 00000000..f4ce8347 --- /dev/null +++ b/docs/tool-execution-toctou.md @@ -0,0 +1,50 @@ +# 工具执行期 TOCTOU 防护设计 + +## 背景 +`WorkspaceSandbox` 在此前版本主要完成“路径边界校验”，执行链为： + +`Permission + Sandbox Check -> Tool.Execute(path string)` + +这会留下检查与执行之间的 TOCTOU（Time-of-Check to Time-of-Use）窗口：路径在校验后被替换，工具仍可能访问到非预期对象。 + +## 本次实现 +本次将链路升级为： + +`Permission + Sandbox Check -> WorkspaceExecutionPlan -> Tool.Execute(plan + args)` + +核心变化如下： + +1. `WorkspaceSandbox.Check` 不再只返回 `error`，而是返回 `*WorkspaceExecutionPlan`。 +2. `ToolManager` 将 plan 透传到 `ToolCallInput.WorkspacePlan`。 +3. `filesystem_read_file` / `filesystem_write_file` / `filesystem_edit` / `bash` 在真实执行前调用 `plan.ValidateForExecution()` 复验锚点状态。 +4. 工具使用 `tools.ResolveWorkspaceTarget` 统一消费 plan，避免再次仅依赖字符串路径解析。 + +## 执行期绑定机制 +`WorkspaceExecutionPlan` 在 sandbox 阶段记录： + +- 规范化后的 workspace root +- 规范化后的最终 target +- 最近存在路径锚点（anchor） +- 锚点快照（模式、大小、修改时间、符号链接目标） + +工具执行前会复验： + +1. 当前锚点是否仍为同一路径； +2. 锚点快照是否与校验阶段一致； +3. 锚点解析后的真实路径是否仍在 workspace 内。 + +若任一步失败，返回稳定错误：`workspace target changed before execution` 或 `escapes workspace root via symlink`。 + +## 当前防护边界 +已覆盖： + +- 校验后 symlink 被替换导致 read 越界 +- 校验后父目录被替换导致 write 越界 +- 校验后 bash workdir 被替换导致 cwd 漂移 + +仍存在限制（已显式记录）： + +- 未实现跨平台 `openat/no-follow/dirfd` 级别的系统调用原子封装 +- 仍非容器级隔离，不替代系统沙箱 + +本次目标是把“校验结果”显式带入执行期，显著缩小 TOCTOU 窗口，并为后续更强执行器（含 MCP）预留统一接口。 diff --git a/internal/security/workspace.go b/internal/security/workspace.go index a0c6198a..7af09964 100644 --- a/internal/security/workspace.go +++ b/internal/security/workspace.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "runtime" @@ -22,28 +23,49 @@ func NewWorkspaceSandbox() *WorkspaceSandbox { } // Check validates that the action stays within the configured workspace root. -func (s *WorkspaceSandbox) Check(ctx context.Context, action Action) error { +func (s *WorkspaceSandbox) Check(ctx context.Context, action Action) (*WorkspaceExecutionPlan, error) { if err := ctx.Err(); err != nil { - return err + return nil, err } if err := action.Validate(); err != nil { - return err + return nil, err } plan, ok, err := buildWorkspacePlan(action) if err != nil { - return err + return nil, err } if !ok { - return nil + return nil, nil } return s.validateWorkspacePlan(plan) } type workspacePlan struct { - root string - target string + root string + target string + targetType TargetType + actionType ActionType +} + +// WorkspaceExecutionPlan binds a validated workspace target to the later +// execution phase. +type WorkspaceExecutionPlan struct { + Root string + Target string + RequestedTarget string + TargetType TargetType + ActionType ActionType + anchorPath string + anchorSnapshot pathSnapshot +} + +type pathSnapshot struct { + mode fs.FileMode + size int64 + modUnix int64 + linkTarget string } // buildWorkspacePlan extracts the workspace root and the sandbox-specific target @@ -64,9 +86,16 @@ func buildWorkspacePlan(action Action) (workspacePlan, bool, error) { return workspacePlan{}, false, nil } + targetType := action.Payload.SandboxTargetType + if targetType == "" { + targetType = action.Payload.TargetType + } + return workspacePlan{ - root: root, - target: target, + root: root, + target: target, + targetType: targetType, + actionType: action.Type, }, true, nil } @@ -121,21 +150,71 @@ func sandboxTarget(action Action) (string, bool) { // validateWorkspacePlan resolves the canonical workspace root, expands the // requested target to an absolute path, verifies it stays inside the workspace, // and then checks the nearest existing path ancestor for symlink escape. -func (s *WorkspaceSandbox) validateWorkspacePlan(plan workspacePlan) error { +func (s *WorkspaceSandbox) validateWorkspacePlan(plan workspacePlan) (*WorkspaceExecutionPlan, error) { root, err := s.canonicalWorkspaceRoot(plan.root) if err != nil { - return err + return nil, err } target, err := absoluteWorkspaceTarget(root, plan.target) if err != nil { - return err + return nil, err } if !isWithinWorkspace(root, target) { - return fmt.Errorf("security: path %q escapes workspace root", plan.target) + return nil, fmt.Errorf("security: path %q escapes workspace root", plan.target) + } + + anchorPath, err := ensureNoSymlinkEscape(root, target, plan.target) + if err != nil { + return nil, err + } + anchorSnapshot, err := capturePathSnapshot(anchorPath) + if err != nil { + return nil, err + } + + return &WorkspaceExecutionPlan{ + Root: root, + Target: target, + RequestedTarget: plan.target, + TargetType: plan.targetType, + ActionType: plan.actionType, + anchorPath: anchorPath, + anchorSnapshot: anchorSnapshot, + }, nil +} + +// ValidateForExecution rechecks the validated workspace anchor before a tool +// touches the filesystem to narrow the TOCTOU window between sandbox check and +// execution. +func (p *WorkspaceExecutionPlan) ValidateForExecution() error { + if p == nil { + return nil + } + if strings.TrimSpace(p.Root) == "" { + return errors.New("security: workspace plan root is empty") + } + if strings.TrimSpace(p.Target) == "" { + return errors.New("security: workspace plan target is empty") + } + + currentAnchor, err := nearestExistingPath(p.Root, p.Target) + if err != nil { + return err + } + if !samePathKey(currentAnchor, p.anchorPath) { + return fmt.Errorf("security: workspace target changed before execution: %q", p.RequestedTarget) } - return ensureNoSymlinkEscape(root, target, plan.target) + currentSnapshot, err := capturePathSnapshot(currentAnchor) + if err != nil { + return err + } + if !p.anchorSnapshot.Equal(currentSnapshot) { + return fmt.Errorf("security: workspace target changed before execution: %q", p.RequestedTarget) + } + + return ensureResolvedPathWithinWorkspace(p.Root, currentAnchor, p.RequestedTarget) } // canonicalWorkspaceRoot resolves the configured workspace root to a canonical @@ -218,22 +297,29 @@ func absoluteWorkspaceTarget(root string, target string) (string, error) { // sandbox check and the later filesystem operation in the executor. Eliminating // that window requires executor-level changes such as descriptor-based access or // no-follow file opening, which are outside this lightweight sandbox layer. -func ensureNoSymlinkEscape(root string, target string, original string) error { +func ensureNoSymlinkEscape(root string, target string, original string) (string, error) { existingPath, err := nearestExistingPath(root, target) if err != nil { - return err + return "", err } if existingPath == root { - return nil + return root, nil + } + + if err := ensureResolvedPathWithinWorkspace(root, existingPath, original); err != nil { + return "", err } + return existingPath, nil +} - resolved, err := filepath.EvalSymlinks(existingPath) +func ensureResolvedPathWithinWorkspace(root string, candidate string, original string) error { + resolved, err := filepath.EvalSymlinks(candidate) if err != nil { - return fmt.Errorf("security: resolve symlink %q: %w", existingPath, err) + return fmt.Errorf("security: resolve symlink %q: %w", candidate, err) } resolved, err = filepath.Abs(resolved) if err != nil { - return fmt.Errorf("security: resolve symlink %q: %w", existingPath, err) + return fmt.Errorf("security: resolve symlink %q: %w", candidate, err) } if !isWithinWorkspace(root, resolved) { return fmt.Errorf("security: path %q escapes workspace root via symlink", original) @@ -241,6 +327,35 @@ func ensureNoSymlinkEscape(root string, target string, original string) error { return nil } +func capturePathSnapshot(path string) (pathSnapshot, error) { + info, err := os.Lstat(path) + if err != nil { + return pathSnapshot{}, fmt.Errorf("security: inspect path %q: %w", path, err) + } + + snapshot := pathSnapshot{ + mode: info.Mode(), + size: info.Size(), + modUnix: info.ModTime().UnixNano(), + } + if info.Mode()&os.ModeSymlink != 0 { + linkTarget, readErr := os.Readlink(path) + if readErr != nil { + return pathSnapshot{}, fmt.Errorf("security: read symlink %q: %w", path, readErr) + } + snapshot.linkTarget = filepath.Clean(linkTarget) + } + return snapshot, nil +} + +// Equal reports whether two path snapshots represent the same path identity. +func (s pathSnapshot) Equal(other pathSnapshot) bool { + return s.mode == other.mode && + s.size == other.size && + s.modUnix == other.modUnix && + s.linkTarget == other.linkTarget +} + func validateTargetVolume(root string, target string) error { rootVolume := normalizeVolumeName(root) targetVolume := normalizeVolumeName(target) diff --git a/internal/security/workspace_test.go b/internal/security/workspace_test.go index 4cb75c14..37fb49fa 100644 --- a/internal/security/workspace_test.go +++ b/internal/security/workspace_test.go @@ -155,7 +155,7 @@ func TestWorkspaceSandboxCheck(t *testing.T) { } sandbox := NewWorkspaceSandbox() - err := sandbox.Check(context.Background(), tt.action(root, outsideFile)) + _, err := sandbox.Check(context.Background(), tt.action(root, outsideFile)) if tt.expectErr != "" { if err == nil || !strings.Contains(err.Error(), tt.expectErr) { t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) @@ -179,12 +179,12 @@ func TestWorkspaceSandboxCheckShortCircuits(t *testing.T) { canceledCtx, cancel := context.WithCancel(context.Background()) cancel() - err := sandbox.Check(canceledCtx, action) + _, err := sandbox.Check(canceledCtx, action) if !errors.Is(err, context.Canceled) { t.Fatalf("expected context canceled error, got %v", err) } - err = sandbox.Check(context.Background(), Action{ + _, err = sandbox.Check(context.Background(), Action{ Type: ActionTypeRead, Payload: ActionPayload{ Resource: "filesystem_read_file", @@ -638,7 +638,7 @@ func TestEnsureNoSymlinkEscape(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() root, target, original := tt.setup(t) - err := ensureNoSymlinkEscape(root, target, original) + _, err := ensureNoSymlinkEscape(root, target, original) if tt.expectErr != "" { if err == nil || !strings.Contains(err.Error(), tt.expectErr) { t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) @@ -652,6 +652,197 @@ func TestEnsureNoSymlinkEscape(t *testing.T) { } } +func TestWorkspaceExecutionPlanValidateForExecution(t *testing.T) { + t.Parallel() + + t.Run("nil plan is allowed", func(t *testing.T) { + t.Parallel() + var plan *WorkspaceExecutionPlan + if err := plan.ValidateForExecution(); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + }) + + t.Run("empty root is rejected", func(t *testing.T) { + t.Parallel() + plan := &WorkspaceExecutionPlan{Target: "target.txt"} + err := plan.ValidateForExecution() + if err == nil || !strings.Contains(err.Error(), "workspace plan root is empty") { + t.Fatalf("expected empty root error, got %v", err) + } + }) + + t.Run("empty target is rejected", func(t *testing.T) { + t.Parallel() + plan := &WorkspaceExecutionPlan{Root: t.TempDir()} + err := plan.ValidateForExecution() + if err == nil || !strings.Contains(err.Error(), "workspace plan target is empty") { + t.Fatalf("expected empty target error, got %v", err) + } + }) + + t.Run("anchor path change is rejected", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + insideDir := filepath.Join(root, "inside") + if err := os.MkdirAll(insideDir, 0o755); err != nil { + t.Fatalf("mkdir inside: %v", err) + } + outsideDir := t.TempDir() + + linkPath := filepath.Join(root, "swap-link") + if err := os.Symlink(insideDir, linkPath); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + plan, err := NewWorkspaceSandbox().Check(context.Background(), fileAction( + ActionTypeRead, + "filesystem_read_file", + "read_file", + root, + filepath.Join("swap-link", "a.txt"), + )) + if err != nil { + t.Fatalf("build plan: %v", err) + } + if plan == nil { + t.Fatalf("expected non-nil plan") + } + + if err := os.Remove(linkPath); err != nil { + t.Fatalf("remove link: %v", err) + } + if err := os.Symlink(outsideDir, linkPath); err != nil { + t.Fatalf("replace link: %v", err) + } + + err = plan.ValidateForExecution() + if err == nil || !strings.Contains(err.Error(), "changed before execution") { + t.Fatalf("expected changed-before-execution error, got %v", err) + } + }) + + t.Run("anchor snapshot change is rejected", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + file := filepath.Join(root, "main.go") + mustWriteWorkspaceFile(t, file, "short") + + plan, err := NewWorkspaceSandbox().Check(context.Background(), fileAction( + ActionTypeRead, + "filesystem_read_file", + "read_file", + root, + "main.go", + )) + if err != nil { + t.Fatalf("build plan: %v", err) + } + if plan == nil { + t.Fatalf("expected non-nil plan") + } + + if err := os.WriteFile(file, []byte("longer-content"), 0o644); err != nil { + t.Fatalf("mutate file: %v", err) + } + + err = plan.ValidateForExecution() + if err == nil || !strings.Contains(err.Error(), "changed before execution") { + t.Fatalf("expected changed-before-execution error, got %v", err) + } + }) + + t.Run("valid unchanged plan passes", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + mustWriteWorkspaceFile(t, filepath.Join(root, "main.go"), "package main\n") + plan, err := NewWorkspaceSandbox().Check(context.Background(), fileAction( + ActionTypeRead, + "filesystem_read_file", + "read_file", + root, + "main.go", + )) + if err != nil { + t.Fatalf("build plan: %v", err) + } + if plan == nil { + t.Fatalf("expected non-nil plan") + } + if err := plan.ValidateForExecution(); err != nil { + t.Fatalf("expected valid plan, got %v", err) + } + }) +} + +func TestCapturePathSnapshotAndEqual(t *testing.T) { + t.Parallel() + + t.Run("regular file snapshot and equality", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + file := filepath.Join(root, "a.txt") + mustWriteWorkspaceFile(t, file, "hello") + + left, err := capturePathSnapshot(file) + if err != nil { + t.Fatalf("capturePathSnapshot(left): %v", err) + } + right, err := capturePathSnapshot(file) + if err != nil { + t.Fatalf("capturePathSnapshot(right): %v", err) + } + if !left.Equal(right) { + t.Fatalf("expected equal snapshots: %#v vs %#v", left, right) + } + }) + + t.Run("symlink snapshot captures link target", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + targetDir := filepath.Join(root, "target") + if err := os.MkdirAll(targetDir, 0o755); err != nil { + t.Fatalf("mkdir target: %v", err) + } + linkPath := filepath.Join(root, "link") + if err := os.Symlink(targetDir, linkPath); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + snapshot, err := capturePathSnapshot(linkPath) + if err != nil { + t.Fatalf("capturePathSnapshot(link): %v", err) + } + if snapshot.linkTarget == "" { + t.Fatalf("expected link target in snapshot, got %#v", snapshot) + } + }) + + t.Run("missing path returns inspect error", func(t *testing.T) { + t.Parallel() + + _, err := capturePathSnapshot(filepath.Join(t.TempDir(), "missing.txt")) + if err == nil || !strings.Contains(err.Error(), "inspect path") { + t.Fatalf("expected inspect error, got %v", err) + } + }) + + t.Run("different snapshots are not equal", func(t *testing.T) { + t.Parallel() + + a := pathSnapshot{mode: 0o644, size: 10, modUnix: 1, linkTarget: "a"} + b := pathSnapshot{mode: 0o755, size: 10, modUnix: 1, linkTarget: "a"} + if a.Equal(b) { + t.Fatalf("expected snapshots to differ: %#v vs %#v", a, b) + } + }) +} + func TestNearestExistingPath(t *testing.T) { t.Parallel() diff --git a/internal/tools/bash/tool.go b/internal/tools/bash/tool.go index d8243122..9facbb88 100644 --- a/internal/tools/bash/tool.go +++ b/internal/tools/bash/tool.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "neo-code/internal/security" "neo-code/internal/tools" ) @@ -68,7 +69,17 @@ func (t *Tool) Execute(ctx context.Context, call tools.ToolCallInput) (tools.Too return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } - workdir, err := resolveWorkdir(call.Workdir, in.Workdir) + base := strings.TrimSpace(call.Workdir) + if base == "" { + base = t.root + } + _, workdir, err := tools.ResolveWorkspaceTarget( + call, + security.TargetTypeDirectory, + base, + in.Workdir, + resolveWorkdir, + ) if err != nil { return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } diff --git a/internal/tools/bash/tool_test.go b/internal/tools/bash/tool_test.go index ad3a0402..e2202ca4 100644 --- a/internal/tools/bash/tool_test.go +++ b/internal/tools/bash/tool_test.go @@ -24,31 +24,42 @@ func TestToolExecute(t *testing.T) { name string command string workdir string + callWorkdir string expectErr string expectContent string }{ { name: "captures stdout", command: safeEchoCommand(), + callWorkdir: workspace, expectContent: "hello", }, { - name: "rejects workdir escape", - command: safeEchoCommand(), - workdir: "..", - expectErr: "workdir escapes workspace root", + name: "rejects workdir escape", + command: safeEchoCommand(), + workdir: "..", + callWorkdir: workspace, + expectErr: "workdir escapes workspace root", }, { - name: "rejects empty command", - command: "", - expectErr: "command is empty", + name: "rejects empty command", + command: "", + callWorkdir: workspace, + expectErr: "command is empty", }, { name: "runs inside nested workdir", command: safePwdCommand(), workdir: "sub", + callWorkdir: workspace, expectContent: normalizeOutputPath(subdir), }, + { + name: "uses tool root when call workdir is empty", + command: safePwdCommand(), + callWorkdir: "", + expectContent: normalizeOutputPath(workspace), + }, } tool := New(workspace, defaultShell(), 3*time.Second) @@ -66,7 +77,7 @@ func TestToolExecute(t *testing.T) { result, execErr := tool.Execute(context.Background(), tools.ToolCallInput{ Name: tool.Name(), Arguments: args, - Workdir: workspace, + Workdir: tt.callWorkdir, }) if tt.expectErr != "" { diff --git a/internal/tools/filesystem/edit.go b/internal/tools/filesystem/edit.go index f878062a..069830f7 100644 --- a/internal/tools/filesystem/edit.go +++ b/internal/tools/filesystem/edit.go @@ -8,6 +8,7 @@ import ( "os" "strings" + "neo-code/internal/security" "neo-code/internal/tools" ) @@ -72,7 +73,13 @@ func (t *EditTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool } root := effectiveRoot(t.root, input.Workdir) - target, err := resolvePath(root, args.Path) + root, target, err := tools.ResolveWorkspaceTarget( + input, + security.TargetTypePath, + root, + args.Path, + resolvePath, + ) if err != nil { return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } diff --git a/internal/tools/filesystem/read_file.go b/internal/tools/filesystem/read_file.go index 612501a0..e27706e5 100644 --- a/internal/tools/filesystem/read_file.go +++ b/internal/tools/filesystem/read_file.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "neo-code/internal/security" "neo-code/internal/tools" ) @@ -58,7 +59,13 @@ func (t *ReadFileTool) Execute(ctx context.Context, input tools.ToolCallInput) ( base := effectiveRoot(t.root, input.Workdir) - target, err := resolvePath(base, args.Path) + base, target, err := tools.ResolveWorkspaceTarget( + input, + security.TargetTypePath, + base, + args.Path, + resolvePath, + ) if err != nil { return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } diff --git a/internal/tools/filesystem/write_file.go b/internal/tools/filesystem/write_file.go index bdc9bc3c..1b5069f6 100644 --- a/internal/tools/filesystem/write_file.go +++ b/internal/tools/filesystem/write_file.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "neo-code/internal/security" "neo-code/internal/tools" ) @@ -64,7 +65,13 @@ func (t *WriteFileTool) Execute(ctx context.Context, input tools.ToolCallInput) base := effectiveRoot(t.root, input.Workdir) - target, err := resolvePath(base, args.Path) + _, target, err := tools.ResolveWorkspaceTarget( + input, + security.TargetTypePath, + base, + args.Path, + resolvePath, + ) if err != nil { return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } diff --git a/internal/tools/manager.go b/internal/tools/manager.go index fea81eb7..a0f88b36 100644 --- a/internal/tools/manager.go +++ b/internal/tools/manager.go @@ -31,7 +31,7 @@ type Executor interface { // WorkspaceSandbox enforces workspace-oriented constraints before execution. type WorkspaceSandbox interface { - Check(ctx context.Context, action security.Action) error + Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) } // NoopWorkspaceSandbox keeps the explicit sandbox stage in the execution chain @@ -39,8 +39,8 @@ type WorkspaceSandbox interface { type NoopWorkspaceSandbox struct{} // Check implements WorkspaceSandbox. -func (NoopWorkspaceSandbox) Check(ctx context.Context, action security.Action) error { - return ctx.Err() +func (NoopWorkspaceSandbox) Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) { + return nil, ctx.Err() } // PermissionDecisionError reports a non-allow permission decision. @@ -155,12 +155,17 @@ func (m *DefaultManager) Execute(ctx context.Context, input ToolCallInput) (Tool return result, permissionErrorFromDecision(decision) } - if err := m.sandbox.Check(ctx, action); err != nil { + plan, err := m.sandbox.Check(ctx, action) + if err != nil { result := NewErrorResult(input.Name, "workspace sandbox rejected action", err.Error(), actionMetadata(action)) result.ToolCallID = input.ID return result, err } + if plan != nil { + input.WorkspacePlan = plan + } + return m.executor.Execute(ctx, input) } diff --git a/internal/tools/manager_test.go b/internal/tools/manager_test.go index 9f2615ec..c33f405d 100644 --- a/internal/tools/manager_test.go +++ b/internal/tools/manager_test.go @@ -16,6 +16,7 @@ type managerStubTool struct { content string err error callCount int + lastCall ToolCallInput } func (t *managerStubTool) Name() string { return t.name } @@ -26,6 +27,7 @@ func (t *managerStubTool) Schema() map[string]any { return map[string]any{"type" func (t *managerStubTool) Execute(ctx context.Context, call ToolCallInput) (ToolResult, error) { t.callCount++ + t.lastCall = call return ToolResult{ Name: t.name, Content: t.content, @@ -38,13 +40,13 @@ type stubSandbox struct { lastAction security.Action } -func (s *stubSandbox) Check(ctx context.Context, action security.Action) error { +func (s *stubSandbox) Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) { s.callCount++ s.lastAction = action if err := ctx.Err(); err != nil { - return err + return nil, err } - return s.err + return nil, s.err } func TestDefaultManagerListAvailableSpecs(t *testing.T) { @@ -599,13 +601,18 @@ func TestNoopWorkspaceSandbox(t *testing.T) { t.Parallel() sandbox := NoopWorkspaceSandbox{} - if err := sandbox.Check(context.Background(), security.Action{}); err != nil { + plan, err := sandbox.Check(context.Background(), security.Action{}) + if err != nil { t.Fatalf("expected nil error, got %v", err) } + if plan != nil { + t.Fatalf("expected nil workspace plan, got %#v", plan) + } ctx, cancel := context.WithCancel(context.Background()) cancel() - if err := sandbox.Check(ctx, security.Action{}); !errors.Is(err, context.Canceled) { + _, err = sandbox.Check(ctx, security.Action{}) + if !errors.Is(err, context.Canceled) { t.Fatalf("expected context canceled, got %v", err) } } diff --git a/internal/tools/manager_toctou_test.go b/internal/tools/manager_toctou_test.go new file mode 100644 index 00000000..459d70c2 --- /dev/null +++ b/internal/tools/manager_toctou_test.go @@ -0,0 +1,242 @@ +package tools_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + "neo-code/internal/security" + "neo-code/internal/tools" + "neo-code/internal/tools/bash" + "neo-code/internal/tools/filesystem" +) + +type mutatingWorkspaceSandbox struct { + base *security.WorkspaceSandbox + mutate func(plan *security.WorkspaceExecutionPlan) error +} + +func (s *mutatingWorkspaceSandbox) Check( + ctx context.Context, + action security.Action, +) (*security.WorkspaceExecutionPlan, error) { + plan, err := s.base.Check(ctx, action) + if err != nil { + return nil, err + } + if plan == nil || s.mutate == nil { + return plan, nil + } + if err := s.mutate(plan); err != nil { + return nil, err + } + return plan, nil +} + +func TestDefaultManagerTOCTOUScenarios(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T, workspace string) (*tools.Registry, tools.ToolCallInput, func(plan *security.WorkspaceExecutionPlan) error) + asserts func(t *testing.T, workspace string, result tools.ToolResult, err error) + }{ + { + name: "read file blocks symlink swap after sandbox check", + setup: func(t *testing.T, workspace string) (*tools.Registry, tools.ToolCallInput, func(plan *security.WorkspaceExecutionPlan) error) { + t.Helper() + + safePath := filepath.Join(workspace, "safe.txt") + if err := os.WriteFile(safePath, []byte("safe"), 0o644); err != nil { + t.Fatalf("write safe file: %v", err) + } + outsideDir := t.TempDir() + outsidePath := filepath.Join(outsideDir, "outside.txt") + if err := os.WriteFile(outsidePath, []byte("outside-secret"), 0o644); err != nil { + t.Fatalf("write outside file: %v", err) + } + + linkPath := filepath.Join(workspace, "swap-link.txt") + if err := os.Symlink(safePath, linkPath); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + registry := tools.NewRegistry() + registry.Register(filesystem.New(workspace)) + + args, err := json.Marshal(map[string]string{"path": "swap-link.txt"}) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + + mutate := func(plan *security.WorkspaceExecutionPlan) error { + if err := os.Remove(linkPath); err != nil { + return err + } + return os.Symlink(outsidePath, linkPath) + } + + return registry, tools.ToolCallInput{ + Name: "filesystem_read_file", + Arguments: args, + Workdir: workspace, + }, mutate + }, + asserts: func(t *testing.T, workspace string, result tools.ToolResult, err error) { + t.Helper() + if err == nil || !strings.Contains(err.Error(), "changed before execution") { + t.Fatalf("expected TOCTOU change error, got %v", err) + } + if strings.Contains(result.Content, "outside-secret") { + t.Fatalf("expected outside content to stay unread, got %q", result.Content) + } + }, + }, + { + name: "write file blocks parent replacement after sandbox check", + setup: func(t *testing.T, workspace string) (*tools.Registry, tools.ToolCallInput, func(plan *security.WorkspaceExecutionPlan) error) { + t.Helper() + + parent := filepath.Join(workspace, "data") + if err := os.MkdirAll(parent, 0o755); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + outsideDir := t.TempDir() + requireSymlinkSupport(t, outsideDir, filepath.Join(workspace, "_write_link_probe")) + swapped := filepath.Join(workspace, "data") + + registry := tools.NewRegistry() + registry.Register(filesystem.NewWrite(workspace)) + + args, err := json.Marshal(map[string]string{ + "path": filepath.Join("data", "new.txt"), + "content": "hello", + }) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + + mutate := func(plan *security.WorkspaceExecutionPlan) error { + if err := os.Remove(swapped); err != nil { + return err + } + return os.Symlink(outsideDir, swapped) + } + + return registry, tools.ToolCallInput{ + Name: "filesystem_write_file", + Arguments: args, + Workdir: workspace, + }, mutate + }, + asserts: func(t *testing.T, workspace string, result tools.ToolResult, err error) { + t.Helper() + if err == nil || !strings.Contains(err.Error(), "changed before execution") { + t.Fatalf("expected TOCTOU change error, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(workspace, "data", "new.txt")); statErr == nil { + t.Fatalf("expected write to be blocked before file creation") + } + }, + }, + { + name: "bash blocks workdir replacement after sandbox check", + setup: func(t *testing.T, workspace string) (*tools.Registry, tools.ToolCallInput, func(plan *security.WorkspaceExecutionPlan) error) { + t.Helper() + + scriptsDir := filepath.Join(workspace, "scripts") + if err := os.MkdirAll(scriptsDir, 0o755); err != nil { + t.Fatalf("mkdir scripts dir: %v", err) + } + outsideDir := t.TempDir() + requireSymlinkSupport(t, outsideDir, filepath.Join(workspace, "_bash_link_probe")) + + registry := tools.NewRegistry() + registry.Register(bash.New(workspace, shellForTOCTOUTest(), 3*time.Second)) + + args, err := json.Marshal(map[string]string{ + "command": safeEchoCommandForTOCTOUTest(), + "workdir": "scripts", + }) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + + mutate := func(plan *security.WorkspaceExecutionPlan) error { + if err := os.Remove(scriptsDir); err != nil { + return err + } + return os.Symlink(outsideDir, scriptsDir) + } + + return registry, tools.ToolCallInput{ + Name: "bash", + Arguments: args, + Workdir: workspace, + }, mutate + }, + asserts: func(t *testing.T, workspace string, result tools.ToolResult, err error) { + t.Helper() + if err == nil || !strings.Contains(err.Error(), "changed before execution") { + t.Fatalf("expected TOCTOU change error, got %v", err) + } + if strings.Contains(result.Content, "ok") { + t.Fatalf("expected command not to run, got %q", result.Content) + } + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + registry, input, mutate := tt.setup(t, workspace) + + engine, err := security.NewStaticGateway(security.DecisionAllow, nil) + if err != nil { + t.Fatalf("new static gateway: %v", err) + } + sandbox := &mutatingWorkspaceSandbox{ + base: security.NewWorkspaceSandbox(), + mutate: mutate, + } + manager, err := tools.NewManager(registry, engine, sandbox) + if err != nil { + t.Fatalf("new manager: %v", err) + } + + result, execErr := manager.Execute(context.Background(), input) + tt.asserts(t, workspace, result, execErr) + }) + } +} + +func shellForTOCTOUTest() string { + if runtime.GOOS == "windows" { + return "powershell" + } + return "sh" +} + +func safeEchoCommandForTOCTOUTest() string { + if runtime.GOOS == "windows" { + return "Write-Output 'ok'" + } + return "printf 'ok'" +} + +func requireSymlinkSupport(t *testing.T, target string, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + _ = os.Remove(link) +} diff --git a/internal/tools/types.go b/internal/tools/types.go index b09726f2..39147ee9 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -4,6 +4,7 @@ import ( "context" "neo-code/internal/provider" + "neo-code/internal/security" ) type Tool interface { @@ -16,12 +17,13 @@ type Tool interface { type ChunkEmitter func(chunk []byte) type ToolCallInput struct { - ID string - Name string - Arguments []byte - SessionID string - Workdir string - EmitChunk ChunkEmitter + ID string + Name string + Arguments []byte + SessionID string + Workdir string + WorkspacePlan *security.WorkspaceExecutionPlan + EmitChunk ChunkEmitter } type ToolResult struct { diff --git a/internal/tools/workspace_plan.go b/internal/tools/workspace_plan.go new file mode 100644 index 00000000..e9ec2aad --- /dev/null +++ b/internal/tools/workspace_plan.go @@ -0,0 +1,66 @@ +package tools + +import ( + "fmt" + "path/filepath" + "runtime" + "strings" + + "neo-code/internal/security" +) + +type workspaceResolver func(root string, requested string) (string, error) + +// ResolveWorkspaceTarget resolves the effective execution target for one tool +// call. When a workspace execution plan exists it is validated and preferred, +// which binds sandbox validation to execution-time use. +func ResolveWorkspaceTarget( + call ToolCallInput, + expectedType security.TargetType, + root string, + requested string, + fallback workspaceResolver, +) (resolvedRoot string, target string, err error) { + if call.WorkspacePlan == nil { + target, err = fallback(root, requested) + if err != nil { + return "", "", err + } + return root, target, nil + } + + plan := call.WorkspacePlan + if err := plan.ValidateForExecution(); err != nil { + return "", "", err + } + if expectedType != "" && plan.TargetType != expectedType { + return "", "", fmt.Errorf( + "tools: workspace plan target type %q does not match expected %q", + plan.TargetType, + expectedType, + ) + } + + expectedTarget, err := fallback(plan.Root, requested) + if err != nil { + return "", "", err + } + if !sameWorkspacePath(expectedTarget, plan.Target) { + return "", "", fmt.Errorf( + "tools: workspace plan target mismatch: expected %q, got %q", + expectedTarget, + plan.Target, + ) + } + + return plan.Root, plan.Target, nil +} + +func sameWorkspacePath(a string, b string) bool { + cleanA := filepath.Clean(a) + cleanB := filepath.Clean(b) + if runtime.GOOS == "windows" { + return strings.EqualFold(cleanA, cleanB) + } + return cleanA == cleanB +} diff --git a/internal/tools/workspace_plan_test.go b/internal/tools/workspace_plan_test.go new file mode 100644 index 00000000..efc65ee5 --- /dev/null +++ b/internal/tools/workspace_plan_test.go @@ -0,0 +1,219 @@ +package tools + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "neo-code/internal/security" +) + +func TestResolveWorkspaceTarget(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T) (ToolCallInput, string, string) + wantErr string + assertion func(t *testing.T, root string, target string) + }{ + { + name: "falls back when workspace plan is missing", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + return ToolCallInput{}, root, "a.txt" + }, + assertion: func(t *testing.T, root string, target string) { + t.Helper() + if !strings.HasSuffix(target, filepath.Join(root, "a.txt")) { + t.Fatalf("expected target inside root, got %q", target) + } + }, + }, + { + name: "fallback resolver error bubbles up", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + return ToolCallInput{}, t.TempDir(), "a.txt" + }, + wantErr: "resolver failed", + }, + { + name: "uses validated workspace plan target", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + path := filepath.Join(root, "main.go") + if err := os.WriteFile(path, []byte("package main\n"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + plan := mustBuildWorkspacePlan(t, root, "main.go", security.TargetTypePath, security.ActionTypeRead) + return ToolCallInput{WorkspacePlan: plan}, root, "main.go" + }, + assertion: func(t *testing.T, root string, target string) { + t.Helper() + if !strings.HasSuffix(target, filepath.Join(root, "main.go")) { + t.Fatalf("expected planned target, got %q", target) + } + }, + }, + { + name: "rejects mismatched requested target", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + path := filepath.Join(root, "a.txt") + if err := os.WriteFile(path, []byte("a"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + plan := mustBuildWorkspacePlan(t, root, "a.txt", security.TargetTypePath, security.ActionTypeRead) + return ToolCallInput{WorkspacePlan: plan}, root, "b.txt" + }, + wantErr: "workspace plan target mismatch", + }, + { + name: "rejects changed anchor before execution", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + path := filepath.Join(root, "a.txt") + if err := os.WriteFile(path, []byte("short"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + plan := mustBuildWorkspacePlan(t, root, "a.txt", security.TargetTypePath, security.ActionTypeRead) + if err := os.WriteFile(path, []byte("longer-content"), 0o644); err != nil { + t.Fatalf("mutate file: %v", err) + } + return ToolCallInput{WorkspacePlan: plan}, root, "a.txt" + }, + wantErr: "changed before execution", + }, + { + name: "rejects target type mismatch", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "scripts"), 0o755); err != nil { + t.Fatalf("mkdir scripts: %v", err) + } + plan := mustBuildWorkspacePlan(t, root, "scripts", security.TargetTypeDirectory, security.ActionTypeBash) + return ToolCallInput{WorkspacePlan: plan}, root, "scripts" + }, + wantErr: "target type", + }, + { + name: "allows type mismatch when expected type is empty", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "scripts"), 0o755); err != nil { + t.Fatalf("mkdir scripts: %v", err) + } + plan := mustBuildWorkspacePlan(t, root, "scripts", security.TargetTypeDirectory, security.ActionTypeBash) + return ToolCallInput{WorkspacePlan: plan}, root, "scripts" + }, + assertion: func(t *testing.T, root string, target string) { + t.Helper() + if !strings.HasSuffix(target, filepath.Join(root, "scripts")) { + t.Fatalf("expected planned scripts target, got %q", target) + } + }, + }, + { + name: "invalid plan root is rejected from validate", + setup: func(t *testing.T) (ToolCallInput, string, string) { + t.Helper() + return ToolCallInput{ + WorkspacePlan: &security.WorkspaceExecutionPlan{ + Target: "a.txt", + }, + }, t.TempDir(), "a.txt" + }, + wantErr: "workspace plan root is empty", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + call, root, requested := tt.setup(t) + expectedType := security.TargetTypePath + if tt.name == "allows type mismatch when expected type is empty" { + expectedType = "" + } + + resolvedRoot, target, err := ResolveWorkspaceTarget( + call, + expectedType, + root, + requested, + func(resolveRoot string, resolveRequested string) (string, error) { + if tt.name == "fallback resolver error bubbles up" { + return "", errors.New("resolver failed") + } + return testPathResolver(resolveRoot, resolveRequested) + }, + ) + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("expected error containing %q, got %v", tt.wantErr, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resolvedRoot == "" { + t.Fatalf("expected non-empty resolved root") + } + if tt.assertion != nil { + tt.assertion(t, resolvedRoot, target) + } + }) + } +} + +func mustBuildWorkspacePlan( + t *testing.T, + root string, + target string, + targetType security.TargetType, + actionType security.ActionType, +) *security.WorkspaceExecutionPlan { + t.Helper() + + plan, err := security.NewWorkspaceSandbox().Check(context.Background(), security.Action{ + Type: actionType, + Payload: security.ActionPayload{ + ToolName: "test_tool", + Resource: "test_tool", + Operation: "test_op", + Workdir: root, + TargetType: targetType, + Target: target, + SandboxTargetType: targetType, + SandboxTarget: target, + }, + }) + if err != nil { + t.Fatalf("build workspace plan: %v", err) + } + if plan == nil { + t.Fatalf("expected non-nil workspace plan") + } + return plan +} + +func testPathResolver(root string, requested string) (string, error) { + path := requested + if !filepath.IsAbs(path) { + path = filepath.Join(root, path) + } + return filepath.Abs(path) +}