From a4a7a59b6a03af7171f4e9801de2ad5e08b90bde Mon Sep 17 00:00:00 2001 From: viettranx Date: Fri, 20 Mar 2026 07:32:23 +0700 Subject: [PATCH] fix(sandbox): path isolation, DooD volume mounting, hints, and nginx DNS (#129, #136) Address issues identified in PR #137 with a cleaner approach: - Sandbox isolation: add SandboxCwd/ResolveSandboxPath helpers to map filesystem tool paths to agent-scoped container subdirectories, preventing cross-agent file access via read/write/edit/list tools - DooD volume mounting: add resolveHostWorkspacePath with multi-strategy container ID detection (/proc/self/mountinfo, HOSTNAME, os.Hostname) and 5s timeout on docker inspect - Sandbox hints: expand from 1 pattern (binary not found) to 6 patterns (permission denied, network disabled, read-only FS, missing file, resource limits) with MaybeFsBridgeHint for filesystem tools - Nginx DNS: add Docker resolver (127.0.0.11) with dynamic upstream variable to handle backend container IP changes - MCP args: switch from comma-separated to space-separated parsing with quote support for --flag="value with spaces" patterns - Refactor: rename ExecTool.workingDir to workspace for consistency, extract sandbox.DefaultContainerWorkdir constant --- internal/sandbox/docker.go | 5 +- internal/sandbox/docker_resolve.go | 136 +++++++++++++++++++++++ internal/sandbox/sandbox.go | 6 +- internal/tools/credentialed_exec.go | 4 +- internal/tools/edit.go | 16 ++- internal/tools/filesystem.go | 12 +- internal/tools/filesystem_list.go | 12 +- internal/tools/filesystem_write.go | 12 +- internal/tools/sandbox_hints.go | 103 +++++++++++++++-- internal/tools/sandbox_hints_test.go | 69 +++++++++--- internal/tools/sandbox_utils.go | 45 ++++++++ internal/tools/sandbox_utils_test.go | 133 ++++++++++++++++++++++ internal/tools/shell.go | 32 +++--- ui/web/nginx.conf | 12 +- ui/web/src/i18n/locales/en/mcp.json | 4 +- ui/web/src/i18n/locales/vi/mcp.json | 4 +- ui/web/src/i18n/locales/zh/mcp.json | 4 +- ui/web/src/pages/mcp/mcp-form-dialog.tsx | 17 ++- 18 files changed, 554 insertions(+), 72 deletions(-) create mode 100644 internal/sandbox/docker_resolve.go create mode 100644 internal/tools/sandbox_utils.go create mode 100644 internal/tools/sandbox_utils_test.go diff --git a/internal/sandbox/docker.go b/internal/sandbox/docker.go index be236075..bb9e3bae 100644 --- a/internal/sandbox/docker.go +++ b/internal/sandbox/docker.go @@ -81,14 +81,15 @@ func newDockerSandbox(ctx context.Context, name string, cfg Config, workspace st args = append(args, "--network", "none") } - // Workspace mount + // Workspace mount — resolve host path for DooD (Docker-out-of-Docker) setups. containerWorkdir := cfg.ContainerWorkdir() if workspace != "" && cfg.WorkspaceAccess != AccessNone { mountOpt := "rw" if cfg.WorkspaceAccess == AccessRO { mountOpt = "ro" } - args = append(args, "-v", fmt.Sprintf("%s:%s:%s", workspace, containerWorkdir, mountOpt)) + hostPath := resolveHostWorkspacePath(ctx, workspace) + args = append(args, "-v", fmt.Sprintf("%s:%s:%s", hostPath, containerWorkdir, mountOpt)) } args = append(args, "-w", containerWorkdir) diff --git a/internal/sandbox/docker_resolve.go b/internal/sandbox/docker_resolve.go new file mode 100644 index 00000000..c90cf4cb --- /dev/null +++ b/internal/sandbox/docker_resolve.go @@ -0,0 +1,136 @@ +package sandbox + +import ( + "context" + "encoding/json" + "log/slog" + "os" + "os/exec" + "path/filepath" + "strings" + "time" +) + +// resolveHostWorkspacePath maps a container-local path to its host-side +// equivalent for Docker-out-of-Docker (DooD) sibling container mounts. +// When GoClaw runs inside a container and spawns sandbox containers, the +// workspace path (e.g., /app/workspace) only exists inside the GoClaw +// container — the sandbox needs the corresponding host path or volume name +// to mount it correctly. +// +// If not running in Docker (no /.dockerenv), returns localPath as-is. +func resolveHostWorkspacePath(ctx context.Context, localPath string) string { + // Not in a container — local path is the host path. + if _, err := os.Stat("/.dockerenv"); err != nil { + return localPath + } + + containerID := detectContainerID() + if containerID == "" { + slog.Error("sandbox.resolve: cannot determine container ID — DooD volume mounts will fail", "path", localPath) + return localPath + } + + // Short timeout to avoid blocking on a stuck Docker daemon. + inspectCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + + out, err := exec.CommandContext(inspectCtx, "docker", "inspect", "--format", "{{json .Mounts}}", containerID).Output() + if err != nil { + slog.Warn("sandbox.resolve: docker inspect failed", "container", containerID, "error", err) + return localPath + } + + var mounts []struct { + Type string `json:"Type"` + Source string `json:"Source"` + Destination string `json:"Destination"` + Name string `json:"Name"` + } + if err := json.Unmarshal(out, &mounts); err != nil { + slog.Warn("sandbox.resolve: failed to parse mounts", "error", err) + return localPath + } + + targetDir := filepath.Clean(localPath) + var bestDest string + var bestSource string + var bestRel string + var bestType string + var bestName string + + for _, m := range mounts { + dest := filepath.Clean(m.Destination) + if targetDir == dest || strings.HasPrefix(targetDir, dest+string(filepath.Separator)) { + if len(dest) > len(bestDest) { + bestDest = dest + bestSource = m.Source + bestType = m.Type + bestName = m.Name + bestRel, _ = filepath.Rel(dest, targetDir) + } + } + } + + if bestDest == "" { + slog.Warn("sandbox.resolve: no matching mount found", "path", localPath, "container", containerID) + return localPath + } + + // Named volume: return volume name if it's the exact mount, otherwise + // use the host source path (assumes local volume driver). + if bestType == "volume" && bestName != "" { + if bestRel == "." { + slog.Debug("sandbox.resolve: resolved to named volume", "path", localPath, "volume", bestName) + return bestName + } + if bestSource != "" { + return filepath.Join(bestSource, bestRel) + } + } + + // Bind mount: join source with relative path. + if bestSource != "" { + resolved := filepath.Join(bestSource, bestRel) + slog.Debug("sandbox.resolve: resolved to host path", "path", localPath, "host", resolved) + return resolved + } + + slog.Warn("sandbox.resolve: mount found but no source path", "path", localPath, "mount", bestDest) + return localPath +} + +// detectContainerID returns the current Docker container ID using multiple +// strategies for reliability: +// 1. /proc/self/mountinfo — parse the container ID from cgroup mount paths +// 2. HOSTNAME env var — usually the short container ID (but can be overridden) +// 3. os.Hostname() — fallback +func detectContainerID() string { + // Strategy 1: Parse /proc/self/mountinfo for docker container ID. + // Lines contain paths like /docker/containers//... + if data, err := os.ReadFile("/proc/self/mountinfo"); err == nil { + for _, line := range strings.Split(string(data), "\n") { + if idx := strings.Index(line, "/docker/containers/"); idx != -1 { + rest := line[idx+len("/docker/containers/"):] + if slashIdx := strings.IndexByte(rest, '/'); slashIdx > 0 { + id := rest[:slashIdx] + if len(id) >= 12 { // Docker IDs are 64 hex chars, short form 12 + return id + } + } + } + } + } + + // Strategy 2: HOSTNAME env var (common Docker default). + if h := os.Getenv("HOSTNAME"); h != "" { + return h + } + + // Strategy 3: os.Hostname() fallback. + if h, err := os.Hostname(); err == nil && h != "" { + return h + } + + return "" +} diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 5f5c517e..c286260f 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -117,12 +117,16 @@ func (c Config) ShouldSandbox(agentID string) bool { } } +// DefaultContainerWorkdir is the default container-side working directory +// used when no custom Workdir is configured. +const DefaultContainerWorkdir = "/workspace" + // ContainerWorkdir returns the container-side working directory. func (c Config) ContainerWorkdir() string { if c.Workdir != "" { return c.Workdir } - return "/workspace" + return DefaultContainerWorkdir } // ResolveScopeKey maps a session key to a sandbox scope key. diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 7fb78657..3a6dbf26 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -114,7 +114,7 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL if err != nil { r := credentialedPathError(binary, err) if t.sandboxMgr != nil && sandboxKey != "" { - r.ForLLM += sandboxHint + r.ForLLM += hintBinaryNotFound } return r } @@ -181,7 +181,7 @@ 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.workingDir, SandboxConfigFromCtx(ctx)) + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) if err != nil { slog.Warn("security.credentialed_exec_sandbox_unavailable", "binary", absPath, "error", err) diff --git a/internal/tools/edit.go b/internal/tools/edit.go index aa2fc76a..f7afeb26 100644 --- a/internal/tools/edit.go +++ b/internal/tools/edit.go @@ -198,10 +198,16 @@ func (t *EditTool) executeInSandbox(ctx context.Context, path, oldStr, newStr st return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } - bridge := sandbox.NewFsBridge(sb.ID(), "/workspace") - content, err := bridge.ReadFile(ctx, path) + 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(), sandbox.DefaultContainerWorkdir) + content, err := bridge.ReadFile(ctx, containerPath) if err != nil { - return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) + return ErrorResult(fmt.Sprintf("failed to read file: %v", err) + MaybeFsBridgeHint(err)) } newContent, result := applyEdit(content, oldStr, newStr, replaceAll) @@ -209,8 +215,8 @@ func (t *EditTool) executeInSandbox(ctx context.Context, path, oldStr, newStr st return result } - if err := bridge.WriteFile(ctx, path, newContent); err != nil { - return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) + if err := bridge.WriteFile(ctx, containerPath, newContent); err != nil { + return ErrorResult(fmt.Sprintf("failed to write file: %v", err) + MaybeFsBridgeHint(err)) } count := strings.Count(content, oldStr) diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index e71bbb3a..10d0b849 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -180,9 +180,15 @@ func (t *ReadFileTool) executeInSandbox(ctx context.Context, path, sandboxKey st return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } - data, err := bridge.ReadFile(ctx, path) + containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) + if cwdErr != nil { + return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) + } + containerPath := ResolveSandboxPath(path, containerCwd) + + data, err := bridge.ReadFile(ctx, containerPath) if err != nil { - return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) + return ErrorResult(fmt.Sprintf("failed to read file: %v", err) + MaybeFsBridgeHint(err)) } return SilentResult(data) @@ -193,7 +199,7 @@ func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey string) (*san if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), "/workspace"), nil + return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil } // allowedWithTeamWorkspace returns the allowed prefixes with team workspace appended diff --git a/internal/tools/filesystem_list.go b/internal/tools/filesystem_list.go index d96414db..36ebea91 100644 --- a/internal/tools/filesystem_list.go +++ b/internal/tools/filesystem_list.go @@ -138,9 +138,15 @@ func (t *ListFilesTool) executeInSandbox(ctx context.Context, path, sandboxKey s return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } - output, err := bridge.ListDir(ctx, path) + containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) + if cwdErr != nil { + return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) + } + containerPath := ResolveSandboxPath(path, containerCwd) + + output, err := bridge.ListDir(ctx, containerPath) if err != nil { - return ErrorResult(fmt.Sprintf("failed to list directory: %v", err)) + return ErrorResult(fmt.Sprintf("failed to list directory: %v", err) + MaybeFsBridgeHint(err)) } return SilentResult(output) @@ -151,5 +157,5 @@ func (t *ListFilesTool) getFsBridge(ctx context.Context, sandboxKey string) (*sa if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), "/workspace"), nil + return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil } diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index 17bb1d62..f061148f 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -224,8 +224,14 @@ func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, san return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) } - if err := bridge.WriteFile(ctx, path, content); err != nil { - return ErrorResult(fmt.Sprintf("failed to write file: %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) + + if err := bridge.WriteFile(ctx, containerPath, content); err != nil { + return ErrorResult(fmt.Sprintf("failed to write file: %v", err) + MaybeFsBridgeHint(err)) } msg := fmt.Sprintf("File written: %s (%d bytes)", path, len(content)) @@ -251,5 +257,5 @@ func (t *WriteFileTool) getFsBridge(ctx context.Context, sandboxKey string) (*sa if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), "/workspace"), nil + return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil } diff --git a/internal/tools/sandbox_hints.go b/internal/tools/sandbox_hints.go index 51b81256..1230e268 100644 --- a/internal/tools/sandbox_hints.go +++ b/internal/tools/sandbox_hints.go @@ -2,31 +2,114 @@ package tools import "strings" -// sandboxHint is appended to tool error output when a command fails inside -// a Docker sandbox container due to a missing binary/tool (exit code 127). -const sandboxHint = "\n\n[SANDBOX] This command ran inside a Docker sandbox container. " + +// --- Hint messages for LLM consumption --- + +const hintBinaryNotFound = "\n\n[SANDBOX] This command ran inside a Docker sandbox container. " + "The required tool/binary is not installed in the sandbox image. " + "Tell the user this failed due to sandbox environment limitations — " + "they can install the binary in the sandbox image or disable sandbox mode for this agent." -// isBinaryNotFound returns true if the error output and exit code indicate -// a missing binary. Exit code 127 is the POSIX convention for "command not found". +const hintPermissionDenied = "\n\n[SANDBOX] Permission denied inside sandbox container. " + + "The workspace may be mounted as read-only (workspace_access: ro). " + + "Check the agent's sandbox configuration or tell the user to change workspace_access to rw." + +const hintNetworkDisabled = "\n\n[SANDBOX] Network operation failed — sandbox networking is disabled (--network none). " + + "If this agent needs internet access, tell the user to enable network_enabled in the agent's sandbox configuration." + +const hintReadOnlyFS = "\n\n[SANDBOX] Write failed — target path is outside the mounted workspace volume. " + + "The sandbox filesystem is read-only except for the workspace mount. " + + "Ensure all file operations use paths within the workspace directory." + +const hintNoSuchFile = "\n\n[SANDBOX] File or directory not found inside sandbox container. " + + "The sandbox has a minimal filesystem — only the workspace mount and installed packages are available. " + + "Verify the path exists within the workspace, or install required files in the sandbox image." + +const hintResourceLimit = "\n\n[SANDBOX] Sandbox resource limit reached (disk space or memory). " + + "The container has restricted resources. Tell the user to increase sandbox limits " + + "or clean up temporary files in the workspace." + +// --- Detection functions --- + +// isBinaryNotFound returns true if the error indicates a missing binary. +// Exit code 127 is the POSIX convention for "command not found". func isBinaryNotFound(exitCode int, output string) bool { if exitCode == 127 { return true } - // Fallback: match stderr patterns from non-standard shells. lower := strings.ToLower(output) return strings.Contains(lower, "not found") && (strings.Contains(lower, "command") || strings.Contains(lower, "sh:")) } -// MaybeSandboxHint returns the sandbox hint suffix if the error looks like -// a missing binary. Returns empty string otherwise. -// Only call this from sandbox execution paths — never from host execution. +func isPermissionDenied(output string) bool { + lower := strings.ToLower(output) + return strings.Contains(lower, "permission denied") || strings.Contains(lower, "eacces") +} + +func isNetworkDisabled(output string) bool { + lower := strings.ToLower(output) + return strings.Contains(lower, "network is unreachable") || + strings.Contains(lower, "name resolution") + // Note: "connection refused" is NOT matched here — it indicates a service + // not running, not network disabled. --network none produces "network is + // unreachable" or DNS resolution failures, not "connection refused". +} + +func isReadOnlyFS(output string) bool { + lower := strings.ToLower(output) + return strings.Contains(lower, "read-only file system") || strings.Contains(lower, "erofs") +} + +func isResourceLimit(output string) bool { + lower := strings.ToLower(output) + return strings.Contains(lower, "no space left") || + strings.Contains(lower, "enospc") || + strings.Contains(lower, "cannot allocate memory") +} + +// --- Public API --- + +// MaybeSandboxHint returns an LLM-actionable hint suffix for sandbox exec errors. +// Checks multiple error patterns in priority order. Returns empty string if no +// pattern matches. Only call from sandbox execution paths. func MaybeSandboxHint(exitCode int, output string) string { if isBinaryNotFound(exitCode, output) { - return sandboxHint + return hintBinaryNotFound + } + if isPermissionDenied(output) { + return hintPermissionDenied + } + if isNetworkDisabled(output) { + return hintNetworkDisabled + } + if isReadOnlyFS(output) { + return hintReadOnlyFS + } + if isResourceLimit(output) { + return hintResourceLimit + } + return "" +} + +// MaybeFsBridgeHint returns an LLM-actionable hint for FsBridge errors +// (filesystem tool sandbox operations). FsBridge errors are Go errors, +// not exit codes, so we match against err.Error() text. +func MaybeFsBridgeHint(err error) string { + if err == nil { + return "" + } + msg := err.Error() + if isPermissionDenied(msg) { + return hintPermissionDenied + } + if isReadOnlyFS(msg) { + return hintReadOnlyFS + } + if strings.Contains(strings.ToLower(msg), "no such file") { + return hintNoSuchFile + } + if isResourceLimit(msg) { + return hintResourceLimit } return "" } diff --git a/internal/tools/sandbox_hints_test.go b/internal/tools/sandbox_hints_test.go index 711fb0aa..b4b005bb 100644 --- a/internal/tools/sandbox_hints_test.go +++ b/internal/tools/sandbox_hints_test.go @@ -1,6 +1,7 @@ package tools import ( + "fmt" "strings" "testing" ) @@ -35,28 +36,70 @@ func TestIsBinaryNotFound(t *testing.T) { func TestMaybeSandboxHint(t *testing.T) { tests := []struct { - name string - exitCode int - output string - wantHint bool + name string + exitCode int + output string + wantTag string // expected [SANDBOX] substring, or "" for no hint }{ - {"binary not found returns hint", 127, "sh: git: not found", true}, - {"success returns empty", 0, "ok", false}, - {"non-binary error returns empty", 1, "segfault", false}, - {"sh pattern returns hint", 1, "sh: python3: not found", true}, + {"binary not found", 127, "sh: git: not found", "not installed in the sandbox image"}, + {"success no hint", 0, "ok", ""}, + {"non-binary error no hint", 1, "segfault", ""}, + {"sh pattern", 1, "sh: python3: not found", "not installed in the sandbox image"}, + {"permission denied", 1, "bash: /workspace/file: Permission denied", "read-only"}, + {"network unreachable", 1, "connect: Network is unreachable", "networking is disabled"}, + {"dns failure", 1, "Temporary failure in name resolution", "networking is disabled"}, + {"read-only fs", 1, "mkdir: cannot create directory: Read-only file system", "outside the mounted workspace"}, + {"no space left", 1, "write: No space left on device", "resource limit"}, + {"cannot allocate memory", 1, "Cannot allocate memory", "resource limit"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := MaybeSandboxHint(tt.exitCode, tt.output) - if tt.wantHint { - if !strings.Contains(got, "[SANDBOX]") { - t.Errorf("MaybeSandboxHint(%d, %q) = %q, want hint containing [SANDBOX]", tt.exitCode, tt.output, got) - } - } else { + if tt.wantTag == "" { if got != "" { t.Errorf("MaybeSandboxHint(%d, %q) = %q, want empty", tt.exitCode, tt.output, got) } + } else { + if !strings.Contains(got, "[SANDBOX]") { + t.Errorf("MaybeSandboxHint(%d, %q) missing [SANDBOX] tag, got %q", tt.exitCode, tt.output, got) + } + if !strings.Contains(got, tt.wantTag) { + t.Errorf("MaybeSandboxHint(%d, %q) missing %q, got %q", tt.exitCode, tt.output, tt.wantTag, got) + } + } + }) + } +} + +func TestMaybeFsBridgeHint(t *testing.T) { + tests := []struct { + name string + err error + wantTag string + }{ + {"nil error", nil, ""}, + {"permission denied", fmt.Errorf("docker exec: permission denied"), "read-only"}, + {"read-only filesystem", fmt.Errorf("write /workspace/file: read-only file system"), "outside the mounted workspace"}, + {"no such file", fmt.Errorf("stat /workspace/missing.txt: no such file or directory"), "not found inside sandbox"}, + {"no space", fmt.Errorf("write: no space left on device"), "resource limit"}, + {"generic error", fmt.Errorf("unexpected EOF"), ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := MaybeFsBridgeHint(tt.err) + if tt.wantTag == "" { + if got != "" { + t.Errorf("MaybeFsBridgeHint(%v) = %q, want empty", tt.err, got) + } + } else { + if !strings.Contains(got, "[SANDBOX]") { + t.Errorf("MaybeFsBridgeHint(%v) missing [SANDBOX] tag, got %q", tt.err, got) + } + if !strings.Contains(got, tt.wantTag) { + t.Errorf("MaybeFsBridgeHint(%v) missing %q, got %q", tt.err, tt.wantTag, got) + } } }) } diff --git a/internal/tools/sandbox_utils.go b/internal/tools/sandbox_utils.go new file mode 100644 index 00000000..e1ccd1a1 --- /dev/null +++ b/internal/tools/sandbox_utils.go @@ -0,0 +1,45 @@ +package tools + +import ( + "context" + "fmt" + "path/filepath" + "strings" +) + +// SandboxCwd maps the current effective workspace (from context) to its +// corresponding path inside the sandbox container. The sandbox mounts the +// global workspace root at containerBase (usually "/workspace"). This function +// computes the relative path from globalWorkspace to the context workspace +// and joins it with containerBase. +// +// Example: globalWorkspace="/app/workspace", ctx workspace="/app/workspace/agent-a/user-123" +// → returns "/workspace/agent-a/user-123" +func SandboxCwd(ctx context.Context, globalWorkspace, containerBase string) (string, error) { + ws := ToolWorkspaceFromCtx(ctx) + if ws == "" { + // No per-request workspace — fall back to container root. + return containerBase, nil + } + + rel, err := filepath.Rel(globalWorkspace, ws) + if err != nil || strings.HasPrefix(filepath.Clean(rel), "..") { + return "", fmt.Errorf("workspace %q is outside global mount %q", ws, globalWorkspace) + } + + if rel == "." { + return containerBase, nil + } + return filepath.Join(containerBase, rel), nil +} + +// ResolveSandboxPath resolves a tool-provided path (relative or absolute) +// against the sandbox container CWD. If the path is relative, it is joined +// with containerCwd. Absolute paths are returned as-is (the sandbox +// filesystem already restricts access to the mounted volume). +func ResolveSandboxPath(path, containerCwd string) string { + if filepath.IsAbs(path) { + return path + } + return filepath.Join(containerCwd, path) +} diff --git a/internal/tools/sandbox_utils_test.go b/internal/tools/sandbox_utils_test.go new file mode 100644 index 00000000..0fb9d4f9 --- /dev/null +++ b/internal/tools/sandbox_utils_test.go @@ -0,0 +1,133 @@ +package tools + +import ( + "context" + "testing" +) + +func TestSandboxCwd(t *testing.T) { + tests := []struct { + name string + ctxWorkspace string // empty = no workspace in context + globalWorkspace string + containerBase string + want string + wantErr bool + }{ + { + name: "no workspace in context — fallback to container base", + ctxWorkspace: "", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + want: "/workspace", + }, + { + name: "workspace equals global mount", + ctxWorkspace: "/app/workspace", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + want: "/workspace", + }, + { + name: "per-agent workspace", + ctxWorkspace: "/app/workspace/agent-a-workspace", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + want: "/workspace/agent-a-workspace", + }, + { + name: "per-user workspace", + ctxWorkspace: "/app/workspace/agent-a/user-123", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + want: "/workspace/agent-a/user-123", + }, + { + name: "team workspace", + ctxWorkspace: "/app/workspace/teams/team-uuid/chat-123", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + want: "/workspace/teams/team-uuid/chat-123", + }, + { + name: "workspace outside global mount — error", + ctxWorkspace: "/other/path/agent-a", + globalWorkspace: "/app/workspace", + containerBase: "/workspace", + wantErr: true, + }, + { + name: "custom container base", + ctxWorkspace: "/app/workspace/agent-a", + globalWorkspace: "/app/workspace", + containerBase: "/home/sandbox", + want: "/home/sandbox/agent-a", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + if tt.ctxWorkspace != "" { + ctx = WithToolWorkspace(ctx, tt.ctxWorkspace) + } + got, err := SandboxCwd(ctx, tt.globalWorkspace, tt.containerBase) + if tt.wantErr { + if err == nil { + t.Errorf("SandboxCwd() = %q, want error", got) + } + return + } + if err != nil { + t.Errorf("SandboxCwd() error = %v", err) + return + } + if got != tt.want { + t.Errorf("SandboxCwd() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveSandboxPath(t *testing.T) { + tests := []struct { + name string + path string + containerCwd string + want string + }{ + { + name: "relative path joined with cwd", + path: "file.txt", + containerCwd: "/workspace/agent-a", + want: "/workspace/agent-a/file.txt", + }, + { + name: "relative subdirectory path", + path: "subdir/file.txt", + containerCwd: "/workspace/agent-a", + want: "/workspace/agent-a/subdir/file.txt", + }, + { + name: "absolute path passed through", + path: "/workspace/agent-a/file.txt", + containerCwd: "/workspace/agent-b", + want: "/workspace/agent-a/file.txt", + }, + { + name: "dot path", + path: ".", + containerCwd: "/workspace/agent-a", + want: "/workspace/agent-a", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ResolveSandboxPath(tt.path, tt.containerCwd) + if got != tt.want { + t.Errorf("ResolveSandboxPath(%q, %q) = %q, want %q", tt.path, tt.containerCwd, got, tt.want) + } + }) + } +} diff --git a/internal/tools/shell.go b/internal/tools/shell.go index f31dad44..706b5179 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -7,7 +7,6 @@ import ( "fmt" "log/slog" "os/exec" - "path/filepath" "regexp" "strings" "time" @@ -31,7 +30,7 @@ func DefaultDenyPatterns() []*regexp.Regexp { // ExecTool executes shell commands, optionally inside a sandbox container. type ExecTool struct { - workingDir string + workspace string timeout time.Duration pathDenyPatterns []*regexp.Regexp // always-on path-based denials (DenyPaths) denyExemptions []string // substrings that exempt a command from deny @@ -43,18 +42,18 @@ type ExecTool struct { } // NewExecTool creates an exec tool that runs commands directly on the host. -func NewExecTool(workingDir string, restrict bool) *ExecTool { +func NewExecTool(workspace string, restrict bool) *ExecTool { return &ExecTool{ - workingDir: workingDir, + workspace: workspace, timeout: 60 * time.Second, restrict: restrict, } } // NewSandboxedExecTool creates an exec tool that routes commands through a sandbox container. -func NewSandboxedExecTool(workingDir string, restrict bool, mgr sandbox.Manager) *ExecTool { +func NewSandboxedExecTool(workspace string, restrict bool, mgr sandbox.Manager) *ExecTool { return &ExecTool{ - workingDir: workingDir, + workspace: workspace, timeout: 300 * time.Second, // sandbox allows longer timeout restrict: restrict, sandboxMgr: mgr, @@ -168,11 +167,11 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { if cred, binary, cmdArgs := t.lookupCredentialedBinary(ctx, command); cred != nil { cwd := ToolWorkspaceFromCtx(ctx) if cwd == "" { - cwd = t.workingDir + cwd = t.workspace } if wd, _ := args["working_dir"].(string); wd != "" { if effectiveRestrict(ctx, t.restrict) { - if resolved, err := resolvePath(wd, t.workingDir, true); err == nil { + if resolved, err := resolvePath(wd, t.workspace, true); err == nil { cwd = resolved } } else { @@ -202,11 +201,11 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { // Use per-user workspace from context if available, fallback to struct field cwd := ToolWorkspaceFromCtx(ctx) if cwd == "" { - cwd = t.workingDir + cwd = t.workspace } if wd, _ := args["working_dir"].(string); wd != "" { if effectiveRestrict(ctx, t.restrict) { - resolved, err := resolvePath(wd, t.workingDir, true) + resolved, err := resolvePath(wd, t.workspace, true) if err != nil { return ErrorResult(err.Error()) } @@ -282,7 +281,7 @@ func (t *ExecTool) executeOnHost(ctx context.Context, command, cwd string) *Resu // 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.workingDir, SandboxConfigFromCtx(ctx)) + sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) if err != nil { if errors.Is(err, sandbox.ErrSandboxDisabled) { return t.executeOnHost(ctx, command, cwd) @@ -296,13 +295,10 @@ 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 - containerCwd := "/workspace" - if cwd != t.workingDir { - rel, relErr := filepath.Rel(t.workingDir, cwd) - if relErr == nil { - containerCwd = filepath.Join("/workspace", rel) - } + // 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 diff --git a/ui/web/nginx.conf b/ui/web/nginx.conf index 9fa0dc53..e2205f4c 100644 --- a/ui/web/nginx.conf +++ b/ui/web/nginx.conf @@ -10,6 +10,12 @@ server { gzip_types text/plain text/css application/json application/javascript text/xml application/xml text/javascript image/svg+xml; gzip_min_length 256; + # Docker internal DNS resolver — re-resolves upstream when backend + # container restarts with a new IP (prevents stale DNS cache). + # Note: valid=10s means up to 10s stale DNS on backend restart. + resolver 127.0.0.11 valid=10s ipv6=off; + set $upstream_backend "http://goclaw:18790"; + # Cache static assets location /assets/ { expires 1y; @@ -18,7 +24,7 @@ server { # WebSocket proxy location /ws { - proxy_pass http://goclaw:18790; + proxy_pass $upstream_backend; proxy_http_version 1.1; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection "upgrade"; @@ -30,7 +36,7 @@ server { # API proxy location /v1/ { - proxy_pass http://goclaw:18790; + proxy_pass $upstream_backend; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; @@ -38,7 +44,7 @@ server { # Health check proxy location /health { - proxy_pass http://goclaw:18790; + proxy_pass $upstream_backend; } # SPA fallback — serve index.html for all other routes diff --git a/ui/web/src/i18n/locales/en/mcp.json b/ui/web/src/i18n/locales/en/mcp.json index 087560c7..434261af 100644 --- a/ui/web/src/i18n/locales/en/mcp.json +++ b/ui/web/src/i18n/locales/en/mcp.json @@ -32,8 +32,8 @@ "displayNamePlaceholder": "My MCP Server", "transport": "Transport *", "command": "Command *", - "args": "Args (comma-separated)", - "argsPlaceholder": "--flag1, --flag2", + "args": "Args (space-separated)", + "argsPlaceholder": "--flag1 --flag2=\"value with spaces\"", "url": "URL *", "headers": "Headers", "headerKeyPlaceholder": "Header name", diff --git a/ui/web/src/i18n/locales/vi/mcp.json b/ui/web/src/i18n/locales/vi/mcp.json index 5f4e83ba..f6bbee65 100644 --- a/ui/web/src/i18n/locales/vi/mcp.json +++ b/ui/web/src/i18n/locales/vi/mcp.json @@ -32,8 +32,8 @@ "displayNamePlaceholder": "Máy chủ MCP của tôi", "transport": "Giao thức *", "command": "Lệnh *", - "args": "Tham số (phân cách bằng dấu phẩy)", - "argsPlaceholder": "--flag1, --flag2", + "args": "Tham số (phân cách bằng dấu cách)", + "argsPlaceholder": "--flag1 --flag2=\"giá trị có dấu cách\"", "url": "URL *", "headers": "Headers", "headerKeyPlaceholder": "Tên header", diff --git a/ui/web/src/i18n/locales/zh/mcp.json b/ui/web/src/i18n/locales/zh/mcp.json index 8bd3f5c3..92498f0b 100644 --- a/ui/web/src/i18n/locales/zh/mcp.json +++ b/ui/web/src/i18n/locales/zh/mcp.json @@ -32,8 +32,8 @@ "displayNamePlaceholder": "我的 MCP 服务器", "transport": "传输协议 *", "command": "命令 *", - "args": "参数（逗号分隔）", - "argsPlaceholder": "--flag1, --flag2", + "args": "参数（空格分隔）", + "argsPlaceholder": "--flag1 --flag2=\"带空格的值\"", "url": "URL *", "headers": "请求头", "headerKeyPlaceholder": "请求头名称", diff --git a/ui/web/src/pages/mcp/mcp-form-dialog.tsx b/ui/web/src/pages/mcp/mcp-form-dialog.tsx index 0f98d4db..af369e34 100644 --- a/ui/web/src/pages/mcp/mcp-form-dialog.tsx +++ b/ui/web/src/pages/mcp/mcp-form-dialog.tsx @@ -78,9 +78,20 @@ export function MCPFormDialog({ open, onOpenChange, server, onSubmit, onTest }: const isStdio = transport === "stdio"; const buildConnectionData = () => { - const parsedArgs = isStdio && args.trim() - ? args.split(",").map((a) => a.trim()).filter(Boolean) - : undefined; + let parsedArgs: string[] | undefined = undefined; + if (isStdio && args.trim()) { + // Split by spaces respecting quoted strings, including --flag="value" patterns. + const matches = args.match(/[^\s"']*"([^"]*)"|[^\s"']*'([^']*)'|[^\s]+/g); + if (matches) { + parsedArgs = matches.map((m) => { + // Strip surrounding quotes from standalone quoted args: "foo bar" → foo bar + if (m.startsWith('"') && m.endsWith('"')) return m.slice(1, -1); + if (m.startsWith("'") && m.endsWith("'")) return m.slice(1, -1); + // Strip quotes from --flag="value" patterns: --flag="foo bar" → --flag=foo bar + return m.replace(/"([^"]*)"/g, "$1").replace(/'([^']*)'/g, "$1"); + }); + } + } const parsedHeaders = !isStdio && Object.keys(headers).length > 0 ? headers : undefined; const parsedEnv = Object.keys(env).length > 0 ? env : undefined; return {