From 4ed4eb9509543984dcd6f5192069e893957772b5 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Sun, 31 May 2026 11:55:10 +0700 Subject: [PATCH] fix(sandbox): isolate tenant workspace mounts Squash merge PR #100 after resolving changelog conflict with current dev. PR CI run 26703558537 passed release-versioning, go, and web. --- docs/project-changelog.md | 15 + internal/sandbox/docker.go | 42 ++- internal/sandbox/docker_cache_key_test.go | 30 ++ internal/sandbox/sandbox.go | 4 +- internal/tools/credentialed_exec.go | 13 +- internal/tools/edit.go | 15 +- internal/tools/filesystem.go | 12 +- internal/tools/filesystem_list.go | 12 +- internal/tools/filesystem_write.go | 12 +- internal/tools/sandbox_mount_test.go | 308 ++++++++++++++++++ internal/tools/sandbox_utils.go | 44 +++ internal/tools/shell.go | 28 +- ...se-01-characterize-risk-and-write-tests.md | 70 ++++ ...-implement-tenant-scoped-sandbox-mounts.md | 79 +++++ .../phase-03-validate-security-and-ship.md | 63 ++++ .../plan.md | 58 ++++ .../reports/scout-debug-summary.md | 28 ++ 17 files changed, 794 insertions(+), 39 deletions(-) create mode 100644 internal/sandbox/docker_cache_key_test.go create mode 100644 internal/tools/sandbox_mount_test.go create mode 100644 plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-01-characterize-risk-and-write-tests.md create mode 100644 plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-02-implement-tenant-scoped-sandbox-mounts.md create mode 100644 plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-03-validate-security-and-ship.md create mode 100644 plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/plan.md create mode 100644 plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/reports/scout-debug-summary.md diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 07e4d1fb..0f0c1d65 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,21 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-29 +### Sandbox tenant workspace isolation (issue #68) + +**Security** + +- Scoped Docker sandbox workspace mounts to the effective tenant/session workspace from tool context instead of the global workspace root. +- Kept the in-container UX stable: the effective workspace is mounted at `/workspace`. +- Made Docker sandbox reuse workspace/config-aware so shared or agent-scoped containers cannot cross workspace boundaries. +- Added fail-closed behavior when tenant-scoped sandbox execution has no effective workspace. + +**Tests** + +- Added unit coverage for effective sandbox workspace selection, `/workspace` cwd mapping, normal and credentialed sandbox exec, file-tool sandbox bridge mount selection, and Docker cache-key isolation. + +--- + ### Human-like channel delivery MVP (issue #67) **New** diff --git a/internal/sandbox/docker.go b/internal/sandbox/docker.go index 4347053d..345d79a3 100644 --- a/internal/sandbox/docker.go +++ b/internal/sandbox/docker.go @@ -3,6 +3,8 @@ package sandbox import ( "bytes" "context" + "crypto/sha256" + "encoding/hex" "fmt" "log/slog" "maps" @@ -254,9 +256,10 @@ func (m *DockerManager) Get(ctx context.Context, key string, workspace string, c if cfg.Mode == ModeOff { return nil, ErrSandboxDisabled } + cacheKey := dockerCacheKey(key, workspace, cfg) m.mu.RLock() - if sb, ok := m.sandboxes[key]; ok { + if sb, ok := m.sandboxes[cacheKey]; ok { m.mu.RUnlock() return sb, nil } @@ -266,7 +269,7 @@ func (m *DockerManager) Get(ctx context.Context, key string, workspace string, c defer m.mu.Unlock() // Double-check - if sb, ok := m.sandboxes[key]; ok { + if sb, ok := m.sandboxes[cacheKey]; ok { return sb, nil } @@ -274,29 +277,48 @@ func (m *DockerManager) Get(ctx context.Context, key string, workspace string, c if prefix == "" { prefix = "goclaw-sbx-" } - name := prefix + sanitizeKey(key) + name := prefix + sanitizeKey(cacheKey) sb, err := newDockerSandbox(ctx, name, cfg, workspace) if err != nil { return nil, err } - m.sandboxes[key] = sb + m.sandboxes[cacheKey] = sb return sb, nil } +func dockerCacheKey(key, workspace string, cfg Config) string { + if workspace == "" { + return key + } + h := sha256.Sum256([]byte(strings.Join([]string{ + workspace, + string(cfg.WorkspaceAccess), + cfg.ContainerWorkdir(), + cfg.Image, + }, "\x00"))) + return "w" + hex.EncodeToString(h[:])[:16] + ":" + key +} + // Release destroys a sandbox by key. func (m *DockerManager) Release(ctx context.Context, key string) error { m.mu.Lock() - sb, ok := m.sandboxes[key] - if ok { - delete(m.sandboxes, key) + sbs := make([]*DockerSandbox, 0, 1) + for cacheKey, sb := range m.sandboxes { + if cacheKey == key || strings.HasSuffix(cacheKey, ":"+key) { + delete(m.sandboxes, cacheKey) + sbs = append(sbs, sb) + } } m.mu.Unlock() - if ok { - return sb.Destroy(ctx) + var firstErr error + for _, sb := range sbs { + if err := sb.Destroy(ctx); err != nil && firstErr == nil { + firstErr = err + } } - return nil + return firstErr } // ReleaseAll destroys all active sandboxes. diff --git a/internal/sandbox/docker_cache_key_test.go b/internal/sandbox/docker_cache_key_test.go new file mode 100644 index 00000000..c993c86c --- /dev/null +++ b/internal/sandbox/docker_cache_key_test.go @@ -0,0 +1,30 @@ +package sandbox + +import "testing" + +func TestDockerCacheKeyIncludesWorkspaceAndConfig(t *testing.T) { + cfg := DefaultConfig() + key := "agent:default:session" + + first := dockerCacheKey(key, "/srv/goclaw/workspace/tenant-a", cfg) + second := dockerCacheKey(key, "/srv/goclaw/workspace/tenant-b", cfg) + if first == second { + t.Fatalf("dockerCacheKey reused key for different workspaces: %q", first) + } + + ro := cfg + ro.WorkspaceAccess = AccessRO + third := dockerCacheKey(key, "/srv/goclaw/workspace/tenant-a", ro) + if first == third { + t.Fatalf("dockerCacheKey reused key for different workspace access: %q", first) + } +} + +func TestDockerCacheKeyPreservesEmptyWorkspaceCompatibility(t *testing.T) { + cfg := DefaultConfig() + key := "agent:default:session" + + if got := dockerCacheKey(key, "", cfg); got != key { + t.Fatalf("dockerCacheKey empty workspace = %q, want original key %q", got, key) + } +} diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 523f5f51..f97e5078 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -196,7 +196,9 @@ type Sandbox interface { // Manager manages sandbox lifecycle based on scope. type Manager interface { - // Get returns (or creates) a sandbox for the given scope key. + // Get returns (or creates) a sandbox for the given scope key, workspace mount, + // and effective config. Implementations must not reuse one container across + // different mounted workspaces or workspace access settings. // For session scope: key = sessionKey // For agent scope: key = agentID // For shared scope: key = "shared" diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 187b716c..6d630b2b 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -623,7 +623,16 @@ func (t *ExecTool) executeCredentialedHost(ctx context.Context, absPath string, func (t *ExecTool) executeCredentialedSandbox(ctx context.Context, absPath string, args []string, cwd string, sandboxKey string, envMap map[string]string, timeout time.Duration) *Result { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(cwd, mountWorkspace, sandbox.DefaultContainerWorkdir) + if cwdErr != nil { + return ErrorResult(fmt.Sprintf("credentialed sandbox path mapping: %v", cwdErr)) + } + + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { slog.Warn("security.credentialed_exec_sandbox_unavailable", "binary", absPath, "error", err) @@ -632,7 +641,7 @@ func (t *ExecTool) executeCredentialedSandbox(ctx context.Context, absPath strin // Direct exec inside sandbox: [absPath, args...] with env injection command := append([]string{absPath}, args...) - result, err := sb.Exec(ctx, command, cwd, sandbox.WithEnv(envMap)) + result, err := sb.Exec(ctx, command, containerCwd, sandbox.WithEnv(envMap)) if err != nil { return ErrorResult(fmt.Sprintf("credentialed sandbox exec: %v", err)) } diff --git a/internal/tools/edit.go b/internal/tools/edit.go index c9a5ac90..3d1b8646 100644 --- a/internal/tools/edit.go +++ b/internal/tools/edit.go @@ -207,15 +207,20 @@ func (t *EditTool) Execute(ctx context.Context, args map[string]any) *Result { } func (t *EditTool) executeInSandbox(ctx context.Context, path, oldStr, newStr string, replaceAll bool, sandboxKey string) *Result { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(mountWorkspace, mountWorkspace, sandbox.DefaultContainerWorkdir) + if cwdErr != nil { + return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) + } + + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) - if cwdErr != nil { - return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) - } containerPath := ResolveSandboxPath(path, containerCwd) bridge := sandbox.NewFsBridge(sb.ID(), containerCwd) diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index 20e2fe2b..e257f600 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -196,11 +196,15 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *Result } func (t *ReadFileTool) executeInSandbox(ctx context.Context, path, sandboxKey string, args map[string]any) *Result { - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(mountWorkspace, mountWorkspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } - bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + bridge, err := t.getFsBridge(ctx, sandboxKey, mountWorkspace, containerCwd) if err != nil { return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } @@ -214,8 +218,8 @@ func (t *ReadFileTool) executeInSandbox(ctx context.Context, path, sandboxKey st return t.paginateOutput(data, args) } -func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) +func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey, mountWorkspace, containerCwd string) (*sandbox.FsBridge, error) { + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } diff --git a/internal/tools/filesystem_list.go b/internal/tools/filesystem_list.go index 02c20969..52d1cdcd 100644 --- a/internal/tools/filesystem_list.go +++ b/internal/tools/filesystem_list.go @@ -140,11 +140,15 @@ func (t *ListFilesTool) Execute(ctx context.Context, args map[string]any) *Resul } func (t *ListFilesTool) executeInSandbox(ctx context.Context, path, sandboxKey string) *Result { - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(mountWorkspace, mountWorkspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } - bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + bridge, err := t.getFsBridge(ctx, sandboxKey, mountWorkspace, containerCwd) if err != nil { return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } @@ -158,8 +162,8 @@ func (t *ListFilesTool) executeInSandbox(ctx context.Context, path, sandboxKey s return SilentResult(output) } -func (t *ListFilesTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) +func (t *ListFilesTool) getFsBridge(ctx context.Context, sandboxKey, mountWorkspace, containerCwd string) (*sandbox.FsBridge, error) { + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index 8f0dff2b..ce7ed448 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -240,11 +240,15 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul } func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, sandboxKey string, deliver, appendMode bool) *Result { - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(mountWorkspace, mountWorkspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } - bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + bridge, err := t.getFsBridge(ctx, sandboxKey, mountWorkspace, containerCwd) if err != nil { return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } @@ -283,8 +287,8 @@ func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, san return result } -func (t *WriteFileTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) +func (t *WriteFileTool) getFsBridge(ctx context.Context, sandboxKey, mountWorkspace, containerCwd string) (*sandbox.FsBridge, error) { + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } diff --git a/internal/tools/sandbox_mount_test.go b/internal/tools/sandbox_mount_test.go new file mode 100644 index 00000000..1232c358 --- /dev/null +++ b/internal/tools/sandbox_mount_test.go @@ -0,0 +1,308 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/sandbox" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type recordingSandboxManager struct { + sandbox *recordingSandbox + key string + workspace string + cfg *sandbox.Config +} + +func (m *recordingSandboxManager) Get(ctx context.Context, key string, workspace string, cfg *sandbox.Config) (sandbox.Sandbox, error) { + m.key = key + m.workspace = workspace + m.cfg = cfg + if m.sandbox == nil { + m.sandbox = &recordingSandbox{} + } + return m.sandbox, nil +} + +func (m *recordingSandboxManager) Release(context.Context, string) error { return nil } +func (m *recordingSandboxManager) ReleaseAll(context.Context) error { return nil } +func (m *recordingSandboxManager) Stop() {} +func (m *recordingSandboxManager) Stats() map[string]any { return nil } + +type recordingSandbox struct { + workDir string + command []string +} + +func (s *recordingSandbox) Exec(ctx context.Context, command []string, workDir string, opts ...sandbox.ExecOption) (*sandbox.ExecResult, error) { + s.command = append([]string(nil), command...) + s.workDir = workDir + return &sandbox.ExecResult{Stdout: "ok"}, nil +} + +func (s *recordingSandbox) Destroy(context.Context) error { return nil } +func (s *recordingSandbox) ID() string { return "recording-sandbox" } + +func TestEffectiveSandboxWorkspacePrefersTenantWorkspace(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + tenantWorkspace := "/srv/goclaw/workspace/tenants/acme/sessions/direct" + + got, err := effectiveSandboxWorkspace(WithToolWorkspace(context.Background(), tenantWorkspace), globalWorkspace) + if err != nil { + t.Fatalf("effectiveSandboxWorkspace returned error: %v", err) + } + if got != tenantWorkspace { + t.Fatalf("effectiveSandboxWorkspace = %q, want tenant workspace %q", got, tenantWorkspace) + } +} + +func TestEffectiveSandboxWorkspaceFailsClosedWithoutTenantWorkspace(t *testing.T) { + ctx := store.WithTenantID(context.Background(), uuid.New()) + + if got, err := effectiveSandboxWorkspace(ctx, "/srv/goclaw/workspace"); err == nil { + t.Fatalf("effectiveSandboxWorkspace = %q, want fail-closed error for tenant context without workspace", got) + } +} + +func TestEffectiveSandboxWorkspaceAllowsMasterFallback(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + ctx := store.WithTenantID(context.Background(), store.MasterTenantID) + + got, err := effectiveSandboxWorkspace(ctx, globalWorkspace) + if err != nil { + t.Fatalf("effectiveSandboxWorkspace returned error: %v", err) + } + if got != globalWorkspace { + t.Fatalf("effectiveSandboxWorkspace = %q, want global workspace %q", got, globalWorkspace) + } +} + +func TestSandboxCwdForHostPathMapsMountRootToWorkspace(t *testing.T) { + mountWorkspace := "/srv/goclaw/workspace/tenants/acme" + + got, err := sandboxCwdForHostPath(mountWorkspace, mountWorkspace, sandbox.DefaultContainerWorkdir) + if err != nil { + t.Fatalf("sandboxCwdForHostPath returned error: %v", err) + } + if got != sandbox.DefaultContainerWorkdir { + t.Fatalf("sandboxCwdForHostPath = %q, want %q", got, sandbox.DefaultContainerWorkdir) + } +} + +func TestSandboxCwdForHostPathMapsChildPathUnderWorkspace(t *testing.T) { + mountWorkspace := "/srv/goclaw/workspace/tenants/acme" + cwd := "/srv/goclaw/workspace/tenants/acme/project" + + got, err := sandboxCwdForHostPath(cwd, mountWorkspace, sandbox.DefaultContainerWorkdir) + if err != nil { + t.Fatalf("sandboxCwdForHostPath returned error: %v", err) + } + if got != "/workspace/project" { + t.Fatalf("sandboxCwdForHostPath = %q, want /workspace/project", got) + } +} + +func TestSandboxCwdForHostPathRejectsWorkspaceEscape(t *testing.T) { + mountWorkspace := "/srv/goclaw/workspace/tenants/acme" + cwd := "/srv/goclaw/workspace/tenants/other" + + if got, err := sandboxCwdForHostPath(cwd, mountWorkspace, sandbox.DefaultContainerWorkdir); err == nil { + t.Fatalf("sandboxCwdForHostPath = %q, want escape error", got) + } +} + +func TestExecSandboxUsesEffectiveWorkspaceMountAndContainerCwd(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + tenantWorkspace := "/srv/goclaw/workspace/tenants/acme" + mgr := &recordingSandboxManager{} + tool := NewSandboxedExecTool(globalWorkspace, true, mgr) + + result := tool.executeInSandbox(WithToolWorkspace(context.Background(), tenantWorkspace), "pwd", tenantWorkspace, "session-1") + if result.IsError { + t.Fatalf("executeInSandbox returned error: %s", result.ForLLM) + } + if mgr.workspace != tenantWorkspace { + t.Fatalf("sandbox manager workspace = %q, want tenant workspace %q", mgr.workspace, tenantWorkspace) + } + if mgr.sandbox.workDir != sandbox.DefaultContainerWorkdir { + t.Fatalf("sandbox exec workDir = %q, want %q", mgr.sandbox.workDir, sandbox.DefaultContainerWorkdir) + } +} + +func TestCredentialedExecSandboxUsesEffectiveWorkspaceMountAndContainerCwd(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + tenantWorkspace := "/srv/goclaw/workspace/tenants/acme" + mgr := &recordingSandboxManager{} + tool := NewSandboxedExecTool(globalWorkspace, true, mgr) + + result := tool.executeCredentialedSandbox(WithToolWorkspace(context.Background(), tenantWorkspace), "/usr/bin/gh", []string{"api", "user"}, tenantWorkspace, "session-1", map[string]string{"GOCLAW_TEST_ENV": "value"}, 30*time.Second) + if result.IsError { + t.Fatalf("executeCredentialedSandbox returned error: %s", result.ForLLM) + } + if mgr.workspace != tenantWorkspace { + t.Fatalf("sandbox manager workspace = %q, want tenant workspace %q", mgr.workspace, tenantWorkspace) + } + if mgr.sandbox.workDir != sandbox.DefaultContainerWorkdir { + t.Fatalf("sandbox exec workDir = %q, want %q", mgr.sandbox.workDir, sandbox.DefaultContainerWorkdir) + } +} + +func TestCredentialedExecSandboxWorkingDirResolvesInsideTenantWorkspace(t *testing.T) { + globalWorkspace := t.TempDir() + tenantWorkspace := filepath.Join(globalWorkspace, "tenants", "acme") + subdir := filepath.Join(tenantWorkspace, "repo") + if err := os.MkdirAll(subdir, 0o755); err != nil { + t.Fatalf("mkdir subdir: %v", err) + } + binaryPath := filepath.Join(tenantWorkspace, "fake-cli") + if err := os.WriteFile(binaryPath, []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatalf("write binary: %v", err) + } + wantMountWorkspace, err := filepath.EvalSymlinks(tenantWorkspace) + if err != nil { + t.Fatalf("canonicalize tenant workspace: %v", err) + } + + mgr := &recordingSandboxManager{} + tool := NewSandboxedExecTool(globalWorkspace, true, mgr) + tool.SetSecureCLIStore(&sandboxMountSecureCLIStore{binary: &store.SecureCLIBinary{ + BinaryName: "fake-cli", + BinaryPath: &binaryPath, + TimeoutSeconds: 30, + Enabled: true, + IsGlobal: true, + }}) + + ctx := WithToolWorkspace(context.Background(), tenantWorkspace) + ctx = WithToolSandboxKey(ctx, "session-1") + result := tool.Execute(ctx, map[string]any{ + "command": "fake-cli status", + "working_dir": "repo", + }) + if result.IsError { + t.Fatalf("Execute returned error: %s", result.ForLLM) + } + if mgr.workspace != wantMountWorkspace { + t.Fatalf("sandbox manager workspace = %q, want tenant workspace %q", mgr.workspace, wantMountWorkspace) + } + if mgr.sandbox.workDir != "/workspace/repo" { + t.Fatalf("sandbox exec workDir = %q, want /workspace/repo", mgr.sandbox.workDir) + } +} + +func TestSandboxFileToolsUseEffectiveWorkspaceMount(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + tenantWorkspace := "/srv/goclaw/workspace/tenants/acme" + ctx := WithToolWorkspace(context.Background(), tenantWorkspace) + + tests := []struct { + name string + run func(*recordingSandboxManager) error + }{ + { + name: "read_file", + run: func(mgr *recordingSandboxManager) error { + tool := NewSandboxedReadFileTool(globalWorkspace, true, mgr) + _, err := tool.getFsBridge(ctx, "session-1", tenantWorkspace, sandbox.DefaultContainerWorkdir) + return err + }, + }, + { + name: "write_file", + run: func(mgr *recordingSandboxManager) error { + tool := NewSandboxedWriteFileTool(globalWorkspace, true, mgr) + _, err := tool.getFsBridge(ctx, "session-1", tenantWorkspace, sandbox.DefaultContainerWorkdir) + return err + }, + }, + { + name: "list_files", + run: func(mgr *recordingSandboxManager) error { + tool := NewSandboxedListFilesTool(globalWorkspace, true, mgr) + _, err := tool.getFsBridge(ctx, "session-1", tenantWorkspace, sandbox.DefaultContainerWorkdir) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mgr := &recordingSandboxManager{} + if err := tt.run(mgr); err != nil { + t.Fatalf("getFsBridge returned error: %v", err) + } + if mgr.workspace != tenantWorkspace { + t.Fatalf("sandbox manager workspace = %q, want tenant workspace %q", mgr.workspace, tenantWorkspace) + } + }) + } +} + +type sandboxMountSecureCLIStore struct { + binary *store.SecureCLIBinary +} + +func (s *sandboxMountSecureCLIStore) Create(context.Context, *store.SecureCLIBinary) error { + return nil +} +func (s *sandboxMountSecureCLIStore) Get(context.Context, uuid.UUID) (*store.SecureCLIBinary, error) { + return nil, nil +} +func (s *sandboxMountSecureCLIStore) Update(context.Context, uuid.UUID, map[string]any) error { + return nil +} +func (s *sandboxMountSecureCLIStore) Delete(context.Context, uuid.UUID) error { return nil } +func (s *sandboxMountSecureCLIStore) List(context.Context) ([]store.SecureCLIBinary, error) { + return nil, nil +} +func (s *sandboxMountSecureCLIStore) LookupByBinary(context.Context, string, *uuid.UUID, string) (*store.SecureCLIBinary, error) { + return s.binary, nil +} +func (s *sandboxMountSecureCLIStore) ListEnabled(context.Context) ([]store.SecureCLIBinary, error) { + return nil, nil +} +func (s *sandboxMountSecureCLIStore) ListForAgent(context.Context, uuid.UUID) ([]store.SecureCLIBinary, error) { + return nil, nil +} +func (s *sandboxMountSecureCLIStore) IsRegisteredBinary(context.Context, string) (bool, error) { + return false, nil +} +func (s *sandboxMountSecureCLIStore) GetUserCredentials(context.Context, uuid.UUID, string) (*store.SecureCLIUserCredential, error) { + return nil, nil +} +func (s *sandboxMountSecureCLIStore) SetUserCredentials(context.Context, uuid.UUID, string, []byte) error { + return nil +} +func (s *sandboxMountSecureCLIStore) SetUserCredentialsTyped(context.Context, uuid.UUID, string, []byte, *string, *string) error { + return nil +} +func (s *sandboxMountSecureCLIStore) DeleteUserCredentials(context.Context, uuid.UUID, string) error { + return nil +} +func (s *sandboxMountSecureCLIStore) ListUserCredentials(context.Context, uuid.UUID) ([]store.SecureCLIUserCredential, error) { + return nil, nil +} + +func TestExecSandboxFailsClosedWhenTenantWorkspaceMissing(t *testing.T) { + globalWorkspace := "/srv/goclaw/workspace" + mgr := &recordingSandboxManager{} + tool := NewSandboxedExecTool(globalWorkspace, true, mgr) + ctx := store.WithTenantID(context.Background(), uuid.New()) + + result := tool.executeInSandbox(ctx, "pwd", globalWorkspace, "session-1") + if !result.IsError { + t.Fatalf("executeInSandbox succeeded, want fail-closed error") + } + if result.ForLLM == "" { + t.Fatalf("executeInSandbox returned empty error result") + } + if mgr.workspace != "" { + t.Fatalf("sandbox manager should not be called, got workspace %q", mgr.workspace) + } +} diff --git a/internal/tools/sandbox_utils.go b/internal/tools/sandbox_utils.go index 18885326..068e442a 100644 --- a/internal/tools/sandbox_utils.go +++ b/internal/tools/sandbox_utils.go @@ -3,9 +3,12 @@ package tools import ( "context" "fmt" + "log/slog" "path" "path/filepath" "strings" + + "github.com/nextlevelbuilder/goclaw/internal/store" ) // SandboxCwd maps the current effective workspace (from context) to its @@ -34,6 +37,47 @@ func SandboxCwd(ctx context.Context, globalWorkspace, containerBase string) (str return path.Join(filepath.ToSlash(containerBase), filepath.ToSlash(rel)), nil } +func effectiveSandboxWorkspace(ctx context.Context, globalWorkspace string) (string, error) { + if ws := ToolWorkspaceFromCtx(ctx); ws != "" { + return canonicalSandboxWorkspace(ws), nil + } + if globalWorkspace != "" && store.IsMasterScope(ctx) { + slog.Warn("security.sandbox_global_workspace_fallback", + "workspace", globalWorkspace, + "tenant_id", store.TenantIDFromContext(ctx), + "agent_id", store.AgentIDFromContext(ctx)) + return canonicalSandboxWorkspace(globalWorkspace), nil + } + return "", fmt.Errorf("sandbox workspace unavailable for tenant-scoped execution") +} + +func canonicalSandboxWorkspace(workspace string) string { + clean := filepath.Clean(workspace) + if real, err := filepath.EvalSymlinks(clean); err == nil { + return real + } + return clean +} + +func sandboxCwdForHostPath(hostCwd, mountWorkspace, containerBase string) (string, error) { + if hostCwd == "" { + hostCwd = mountWorkspace + } + if containerBase == "" { + containerBase = "/workspace" + } + cleanMount := filepath.Clean(mountWorkspace) + cleanCwd := filepath.Clean(hostCwd) + rel, err := filepath.Rel(cleanMount, cleanCwd) + if err != nil || strings.HasPrefix(filepath.Clean(rel), "..") { + return "", fmt.Errorf("working directory %q is outside sandbox mount %q", hostCwd, mountWorkspace) + } + if rel == "." { + return filepath.ToSlash(containerBase), nil + } + return path.Join(filepath.ToSlash(containerBase), filepath.ToSlash(rel)), nil +} + // ResolveSandboxPath resolves a tool-provided path (relative or absolute) // against the sandbox container CWD. Escapes are rejected to containerCwd so a // tool scoped to /workspace/agent-a cannot address /workspace/agent-b. diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 16b5e70f..f0885cd0 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -260,9 +260,16 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { } if wd, _ := args["working_dir"].(string); wd != "" { if effectiveRestrict(ctx, t.restrict) { - if resolved, err := resolvePath(wd, t.workspace, true); err == nil { - cwd = resolved + wsBase := ToolWorkspaceFromCtx(ctx) + if wsBase == "" { + wsBase = t.workspace } + allowed := allowedWriteWithTeamWorkspace(ctx, nil) + resolved, err := resolvePathWithAllowed(wd, wsBase, true, allowed) + if err != nil { + return ErrorResult(err.Error()) + } + cwd = resolved } else { cwd = wd } @@ -644,7 +651,16 @@ func buildHostResult(err error, stdout, stderr *limitedBuffer, ctx context.Conte // executeInSandbox routes a command through a Docker sandbox container. func (t *ExecTool) executeInSandbox(ctx context.Context, command, cwd, sandboxKey string) *Result { - sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) + mountWorkspace, err := effectiveSandboxWorkspace(ctx, t.workspace) + if err != nil { + return ErrorResult(err.Error()) + } + containerCwd, cwdErr := sandboxCwdForHostPath(cwd, mountWorkspace, sandbox.DefaultContainerWorkdir) + if cwdErr != nil { + return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) + } + + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, mountWorkspace, SandboxConfigFromCtx(ctx)) if err != nil { if errors.Is(err, sandbox.ErrSandboxDisabled) { return t.executeOnHost(ctx, command, cwd) @@ -658,12 +674,6 @@ func (t *ExecTool) executeInSandbox(ctx context.Context, command, cwd, sandboxKe return ErrorResult(fmt.Sprintf("sandbox unavailable: %v (will not fall back to unsandboxed host execution)", err)) } - // Map host workdir to container workdir via SandboxCwd helper. - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) - if cwdErr != nil { - return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) - } - result, err := sb.Exec(ctx, []string{"sh", "-c", command}, containerCwd) //nolint: no ExecOption for normal exec if err != nil { return ErrorResult(fmt.Sprintf("sandbox exec: %v", err)) diff --git a/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-01-characterize-risk-and-write-tests.md b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-01-characterize-risk-and-write-tests.md new file mode 100644 index 00000000..b9641a95 --- /dev/null +++ b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-01-characterize-risk-and-write-tests.md @@ -0,0 +1,70 @@ +--- +phase: 1 +title: "Characterize Risk and Write Tests" +status: complete +effort: "2h" +--- + +# Phase 1: Characterize Risk and Write Tests + +## Context Links + +- Issue: https://github.com/digitopvn/goclaw/issues/68 +- Upstream issue: https://github.com/nextlevelbuilder/goclaw/issues/1163 +- Docker mount creation: `internal/sandbox/docker.go` +- Sandbox path helpers: `internal/tools/sandbox_utils.go` +- Exec sandbox path: `internal/tools/shell.go` +- File tools: `internal/tools/filesystem.go`, `internal/tools/filesystem_write.go`, `internal/tools/filesystem_list.go`, `internal/tools/edit.go` +- FsBridge: `internal/sandbox/fsbridge.go` + +## Overview + +Priority P0. First lock current behavior with tests so implementation fixes the mount namespace, not just tool-layer symptoms. + +## Key Insights + +- `newDockerSandbox()` mounts the `workspace` argument at the container workdir when workspace access is enabled. +- `ExecTool.executeInSandbox()` calls `sandboxMgr.Get(ctx, sandboxKey, t.workspace, ...)`, where `t.workspace` is the global workspace root. +- `SandboxCwd()` maps effective request workspace to a subdirectory under `/workspace`, but shell commands can still address `/workspace` directly because the global tree is mounted. +- `ResolveSandboxPath()` and `FsBridge` already clamp file-tool paths to the effective container cwd; keep and test these protections. + +## Requirements + +- Add tests proving sandbox creation uses the effective workspace from context when present. +- Add tests proving `sandboxKey` or manager reuse cannot return a container for a different effective workspace. +- Add a regression test showing `/workspace` should represent only the effective tenant/session workspace. +- Keep tests deterministic; avoid real Docker dependency unless an existing integration harness already provides it. + +## Related Code Files + +- Modify tests: `internal/tools/sandbox_utils_test.go` +- Modify tests: `internal/tools/shell_test.go` or a new focused test file in `internal/tools/` +- Modify tests: `internal/sandbox/docker_test.go` +- Read only: `internal/sandbox/docker.go`, `internal/tools/shell.go`, file-tool sandbox callers + +## Implementation Steps + +1. Add a manager fake that records the `workspace` argument passed to `Get`. +2. Exercise `ExecTool.executeInSandbox()` with `WithToolWorkspace(ctx, tenantWorkspace)` and assert `Get` receives `tenantWorkspace`, not the global root. +3. Cover read/list/write/edit sandbox callers or extract a shared helper so one test protects all callers. +4. Add a config/key test proving two effective workspaces do not reuse the same sandbox container key. +5. Re-run existing sandbox path tests to keep absolute path clamp and symlink realpath behavior intact. + +## Success Criteria + +- Complete: new tests failed before implementation because `effectiveSandboxWorkspace`, `sandboxCwdForHostPath`, and `dockerCacheKey` did not exist yet. +- Complete: tests cover effective workspace selection, fail-closed missing tenant workspace, `/workspace` root mapping, exec/credentialed exec mount/cwd use, file-tool bridge mount use, and Docker cache-key separation. +- Complete: tests are unit-only and require no production Docker daemon. + +## Risk Assessment + +- Fake manager can under-test Docker args. Mitigate with a small `newDockerSandbox` argument-builder seam if needed. +- If tests reach private methods only through exported tools, keep package-level tests in `internal/tools` rather than adding production hooks. + +## Security Considerations + +- Tests must model malicious absolute `/workspace/...` shell paths and sibling tenant directory enumeration. + +## Unresolved Questions + +- None for characterization. diff --git a/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-02-implement-tenant-scoped-sandbox-mounts.md b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-02-implement-tenant-scoped-sandbox-mounts.md new file mode 100644 index 00000000..02cfb453 --- /dev/null +++ b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-02-implement-tenant-scoped-sandbox-mounts.md @@ -0,0 +1,79 @@ +--- +phase: 2 +title: "Implement Tenant-Scoped Sandbox Mounts" +status: complete +effort: "3h" +--- + +# Phase 2: Implement Tenant-Scoped Sandbox Mounts + +## Context Links + +- Phase 1 tests: `./phase-01-characterize-risk-and-write-tests.md` +- Context workspace source: `internal/tools/context_keys.go` +- Sandbox manager: `internal/sandbox/docker.go` +- Exec sandbox caller: `internal/tools/shell.go` +- Credentialed sandbox caller: `internal/tools/credentialed_exec.go` +- File tool sandbox callers: `internal/tools/filesystem*.go`, `internal/tools/edit.go` + +## Overview + +Mount only the effective workspace subtree into the sandbox container while preserving the inside-container path convention: the effective workspace appears as `/workspace`. + +## Requirements + +- Functional: tenant/session workspace is the only workspace mounted for sandboxed exec and file tools. +- Backward compatible UX: existing relative paths and `/workspace/...` paths inside the effective workspace continue to work. +- Safety: if no effective workspace exists in context, either fail closed for non-master requests or explicitly fall back only for verified master/global scope. +- Reuse: sandbox manager key must include enough scope to avoid container reuse across workspaces/config. + +## Architecture + +Add one shared helper in `internal/tools` that resolves the sandbox mount workspace: + +1. Prefer `ToolWorkspaceFromCtx(ctx)`. +2. Fall back to `t.workspace` only when no request workspace is available. +3. Return both mount workspace and container cwd mapping. + +When the mount workspace is already the effective workspace, `SandboxCwd()` should return `/workspace`. If a team/shared workspace is intentionally the effective workspace, mount that path directly. + +## Related Code Files + +- Modify: `internal/tools/shell.go` +- Modify: `internal/tools/credentialed_exec.go` +- Modify: `internal/tools/filesystem.go` +- Modify: `internal/tools/filesystem_write.go` +- Modify: `internal/tools/filesystem_list.go` +- Modify: `internal/tools/edit.go` +- Modify if needed: `internal/tools/sandbox_utils.go` +- Modify if needed: `internal/sandbox/docker.go` + +## Implementation Steps + +1. Introduce a small `effectiveSandboxWorkspace(ctx, globalWorkspace)` helper. +2. Replace sandbox `Get(ctx, sandboxKey, t.workspace, ...)` calls with the effective workspace for exec, credentialed exec, read, write, list, and edit. +3. Update `SandboxCwd()` usage so the effective workspace maps to `/workspace` after scoped mounting. +4. Ensure sandbox reuse key differentiates effective workspace; prefer appending a stable hash of effective mount path and workspace access mode. +5. Keep `ResolveSandboxPath()` and `FsBridge` validations unchanged unless Phase 1 exposes a real gap. +6. Add concise security log on fallback to global workspace, if fallback remains. + +## Success Criteria + +- Complete: sandboxed exec and file tools now pass the effective tenant/session workspace to `sandbox.Manager.Get`. +- Complete: sandboxed normal exec and credentialed exec map host cwd under the effective mount to `/workspace`. +- Complete: Docker sandbox cache identity now includes workspace, workspace access, workdir, and image. +- Complete: host-mode workspace restrictions were not changed. + +## Risk Assessment + +- Relative path delivery for sandboxed `write_file` currently joins `ToolWorkspaceFromCtx(ctx)` with user path; verify absolute path inputs do not produce bad host delivery paths. +- Sandbox containers may be reused longer than expected. Include effective workspace in key or manager lookup identity. + +## Security Considerations + +- Do not whitelist sibling tenant paths as allowed mounts. +- Treat shell as untrusted arbitrary code; mount isolation is mandatory even if deny patterns remain. + +## Unresolved Questions + +- Whether master-scope maintenance agents should retain a global sandbox mode needs product decision; default implementation should prefer fail closed or explicit opt-in. diff --git a/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-03-validate-security-and-ship.md b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-03-validate-security-and-ship.md new file mode 100644 index 00000000..7d2e5846 --- /dev/null +++ b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/phase-03-validate-security-and-ship.md @@ -0,0 +1,63 @@ +--- +phase: 3 +title: "Validate Security and Ship" +status: complete +effort: "2h" +--- + +# Phase 3: Validate Security and Ship + +## Context Links + +- Phase 2 implementation: `./phase-02-implement-tenant-scoped-sandbox-mounts.md` +- Post-implementation checklist: `CLAUDE.md` +- Related issue: https://github.com/digitopvn/goclaw/issues/68 + +## Overview + +Validate the P0 isolation fix locally, review with security focus, update changelog, then ship through the normal `dev` PR path. + +## Requirements + +- Fresh verification only; no completion claim from stale test output. +- No load/stress/benchmark tests. +- No dotenv or secret files staged. +- Both PostgreSQL and SQLite builds must compile because sandbox code is shared by standard and desktop builds. + +## Implementation Steps + +1. Run focused tests: + - `go test ./internal/tools -run 'Sandbox|ExecTool|ReadFile|WriteFile|ListFiles|Edit'` + - `go test ./internal/sandbox -run 'Docker|FsBridge|Workspace|Path'` +2. Run compile/static checks: + - `go build ./...` + - `go build -tags sqliteonly ./...` + - `go vet ./...` +3. If implementation changes schema or user-facing docs, update both roadmap/changelog docs per repo rules. +4. Run security review focused on tenant isolation, sandbox reuse, symlink/absolute-path escapes, and fail-open fallback. +5. Stage only relevant code, tests, and docs; run: + - `git diff --cached --check` + - `git diff --cached | grep -iE '(api[_-]?key|token|password|secret|credential)'` +6. Commit with conventional message and push branch. +7. Create PR to `digitopvn/goclaw:dev`, link issue #68, and monitor CI. + +## Success Criteria + +- Complete: focused sandbox tests pass and cover effective mount selection plus cache-key isolation. +- Complete: `go build ./...`, `go build -tags sqliteonly ./...`, `go vet ./...`, and `go test -race -tags integration ./tests/integration/` pass locally. +- Complete: changelog security note added in `docs/project-changelog.md`. +- Pending ship actions: PR creation and issue #68 comment/label after push. + +## Risk Assessment + +- CI may show pre-existing flaky `TestShellAbort_ProcessGroupKilled`; only treat as unrelated after local reproduction and check log proof. +- Docker-dependent tests can flake on CI. Prefer unit tests around args/workspace selection plus one optional integration test if harness exists. + +## Security Considerations + +- Verify no mount path falls back to global workspace for tenant-scoped runs. +- Verify sandbox `WorkspaceAccess=rw` cannot modify sibling tenant data because sibling data is absent from the mount. + +## Unresolved Questions + +- None for validation. diff --git a/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/plan.md b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/plan.md new file mode 100644 index 00000000..d04fbfed --- /dev/null +++ b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/plan.md @@ -0,0 +1,58 @@ +--- +title: "Issue 68 Sandbox Tenant Workspace Isolation" +description: "Plan P0 fix for Docker sandbox global workspace mount visibility across tenants." +status: complete +priority: P0 +issue: 68 +branch: "codex/issue-68-sandbox-tenant-workspace-plan" +tags: [sandbox, security, tenant-isolation, issue-68] +blockedBy: [] +blocks: [] +created: "2026-05-28T18:37:00+07:00" +createdBy: "codex" +source: github-issue +--- + +# Issue 68 Sandbox Tenant Workspace Isolation + +## Overview + +Fix the sandbox isolation gap where Docker containers receive the global workspace root as `/workspace`. Current file tools already clamp sandbox file paths to the effective container cwd, but shell/exec still runs inside a container that can enumerate the full mounted tree. + +## Evidence + +- Issue: digitopvn/goclaw#68, upstream nextlevelbuilder/goclaw#1163 +- Mount source: `internal/sandbox/docker.go:91-100` +- Shell sandbox caller passes global workspace: `internal/tools/shell.go:645-667` +- File-tool sandbox callers also request sandbox by global workspace: `internal/tools/filesystem.go:198-222`, `internal/tools/filesystem_write.go:242-291`, `internal/tools/filesystem_list.go:142-166`, `internal/tools/edit.go:209-221` +- Existing mitigations: `internal/tools/sandbox_utils.go:37-55`, `internal/sandbox/fsbridge.go:143-236`, `internal/tools/sandbox_utils_test.go:92-145`, `internal/sandbox/docker_test.go:130-174` + +## Phases + +| Phase | Name | Status | +|-------|------|--------| +| 1 | [Characterize Risk and Write Tests](./phase-01-characterize-risk-and-write-tests.md) | Complete | +| 2 | [Implement Tenant-Scoped Sandbox Mounts](./phase-02-implement-tenant-scoped-sandbox-mounts.md) | Complete | +| 3 | [Validate Security and Ship](./phase-03-validate-security-and-ship.md) | Complete | + +## Key Decisions + +- Do not rely on cwd-only isolation for shell. The mount namespace must not include sibling tenant workspaces. +- Keep existing `ResolveSandboxPath` and `FsBridge` realpath validation as defense in depth. +- Preserve current `/workspace` UX inside the sandbox by mounting the effective workspace subtree at `/workspace`, not by exposing `/workspace/`. +- Reuse `ToolWorkspaceFromCtx(ctx)` / `store.RunContext.Workspace` as the effective tenant/session workspace source. + +## Success Criteria + +- A sandboxed exec for tenant A cannot list or read tenant B workspace paths. +- Sandboxed file tools still read/write/list/edit inside tenant A workspace. +- Sandbox reuse key cannot reuse a container created for a different effective workspace/config. +- Focused tests pass, plus `go build ./...`, `go build -tags sqliteonly ./...`, and `go vet ./...`. + +## Docs Impact + +Minor. Add changelog/security note after implementation lands. + +## Unresolved Questions + +- Should team shared workspace be a separate allowed mount, or should team runs set `ToolWorkspaceFromCtx` to the team/session workspace before sandbox creation? diff --git a/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/reports/scout-debug-summary.md b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/reports/scout-debug-summary.md new file mode 100644 index 00000000..b8b48022 --- /dev/null +++ b/plans/260528-1837-issue-68-sandbox-tenant-workspace-isolation/reports/scout-debug-summary.md @@ -0,0 +1,28 @@ +# Scout Debug Summary + +## Issue + +`digitopvn/goclaw#68` reports P0 cross-tenant workspace exposure through Docker sandbox global workspace mounts. Upstream `nextlevelbuilder/goclaw#1163` is still open and describes the same mount-layer gap. + +## Verified Code Evidence + +- `internal/sandbox/docker.go:91-100` mounts the `workspace` argument into `cfg.ContainerWorkdir()` as `/workspace` by default. +- `internal/tools/shell.go:645-667` calls `sandboxMgr.Get(ctx, sandboxKey, t.workspace, ...)` and then executes `sh -c` in the mapped container cwd. +- `internal/tools/credentialed_exec.go:621-635` also calls sandbox manager with `t.workspace`. +- `internal/tools/filesystem.go:198-222`, `internal/tools/filesystem_write.go:242-291`, `internal/tools/filesystem_list.go:142-166`, and `internal/tools/edit.go:209-221` use the same global `t.workspace` for sandbox manager lookup. +- `internal/tools/context_keys.go:120-131` already provides effective request workspace via `ToolWorkspaceFromCtx(ctx)` / `store.RunContext.Workspace`. +- `internal/tools/sandbox_utils.go:37-55` clamps sandbox file-tool paths to `containerCwd`. +- `internal/sandbox/fsbridge.go:143-236` adds realpath validation inside the container. +- `internal/tools/sandbox_utils_test.go:92-145` and `internal/sandbox/docker_test.go:130-174` already test path-boundary behavior. + +## Root Cause + +Current mount isolation relies on setting the container cwd below `/workspace`, but the bind mount still contains the global workspace tree. File tools have defense-in-depth path clamps; shell/exec remains arbitrary code and can directly inspect `/workspace` unless the mount itself is scoped. + +## Recommended Fix + +Mount the effective tenant/session/team workspace, not the global workspace root, for every sandboxed tool path. Preserve `/workspace` as the container-visible root for that effective workspace. + +## Unresolved Questions + +- Master-scope global sandbox mode needs explicit product decision. Default P0 fix should not expose global workspace to tenant-scoped runs.