mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 00:16:38 +00:00
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.
This commit is contained in:
1 parent
f0f39ce31f
commit
4ed4eb9509
17 files changed
+794
-39
No files matched your search
@@ -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**
|
||||
|
||||
+32
-10
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
+10
-5
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
+19
-9
@@ -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))
|
||||
|
||||
+70
@@ -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.
|
||||
+79
@@ -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.
|
||||
+63
@@ -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.
|
||||
@@ -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/<relative-subtree>`.
|
||||
- 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?
|
||||
+28
@@ -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.
|
||||
Reference in new issue
Block a user