From 83615593cdd9a573a6f48d0196a6bb6804a7c581 Mon Sep 17 00:00:00 2001 From: matevip Date: Wed, 10 Jun 2026 17:17:07 +0800 Subject: [PATCH] fix(tool/guard): harden workspace filesystem sandbox (#313) - Fail closed to a global fallback sandbox root when a conversation has no per-workspace base path, instead of leaving file/shell tools unconstrained - Refuse shell commands that delete the workspace root directory itself - Block workspace-boundary escapes at the policy layer before the approval prompt, not only at execution time - Approval bar now shows the actual command / target path being approved --- .../graph/executor/ToolExecutionExecutor.java | 5 +- .../mate/tool/guard/WorkspacePathGuard.java | 140 ++++++++++++- .../guardian/WorkspaceBoundaryGuardian.java | 114 +++++++++++ .../guard/model/ToolInvocationContext.java | 19 +- .../WorkspaceSandboxAutoConfiguration.java | 47 +++++ .../config/WorkspaceSandboxProperties.java | 36 ++++ .../src/main/resources/application.yml | 9 + .../grant/ApprovalGrantResolverTest.java | 5 +- .../guard/WorkspacePathGuardSandboxTest.java | 189 ++++++++++++++++++ .../WorkspaceBoundaryGuardianTest.java | 122 +++++++++++ mateclaw-ui/src/components/chat/ChatInput.vue | 53 ++++- 11 files changed, 728 insertions(+), 11 deletions(-) create mode 100644 mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java create mode 100644 mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxAutoConfiguration.java create mode 100644 mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxProperties.java create mode 100644 mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java create mode 100644 mateclaw-server/src/test/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardianTest.java diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java index 301b0ed7..71363c40 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java @@ -988,7 +988,10 @@ public class ToolExecutionExecutor { ToolInvocationContext guardCtx = ToolInvocationContext.of( toolName, java.util.Map.of(), arguments, conversationId, agentId, - /*channelType*/ null, requesterId, workspaceId); + /*channelType*/ null, requesterId, workspaceId) + // Carry the active workspace base path so a guardian can enforce + // the filesystem boundary before approval (issue #313). + .withWorkspaceBasePath(origin != null ? origin.workspaceBasePath() : null); if (toolGuardService != null) { GuardEvaluation evaluation = toolGuardService.evaluate(guardCtx); diff --git a/mateclaw-server/src/main/java/vip/mate/tool/guard/WorkspacePathGuard.java b/mateclaw-server/src/main/java/vip/mate/tool/guard/WorkspacePathGuard.java index 06cb3255..d0b329ec 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/guard/WorkspacePathGuard.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/guard/WorkspacePathGuard.java @@ -39,6 +39,19 @@ public final class WorkspacePathGuard { */ private static volatile Path skillRoot; + /** + * Global fallback sandbox root. When a conversation has no per-workspace + * base path configured, file and shell operations fall back to this root + * instead of running unconstrained against the whole filesystem. This is + * the fail-closed default: without it, a workspace whose {@code base_path} + * column is unset (the out-of-the-box state) leaves the agent able to read, + * write, and delete anywhere the server process can reach. Registered once + * at startup from the {@code mateclaw.workspace.sandbox.root} setting. + * {@code null} until set (then the legacy "no boundary when unconfigured" + * behaviour applies — used in tests and when the sandbox is disabled). + */ + private static volatile Path defaultRoot; + /** * Register the shared skill repository root. A {@code null} or blank path * clears it, restoring workspace-only enforcement. @@ -56,6 +69,24 @@ public final class WorkspacePathGuard { return skillRoot; } + /** + * Register the global fallback sandbox root. A {@code null} or blank path + * clears it, restoring the legacy unconstrained behaviour for conversations + * without a configured workspace base path. + */ + public static void setDefaultRoot(@Nullable String path) { + defaultRoot = (path == null || path.isBlank()) + ? null + : Paths.get(path).toAbsolutePath().normalize(); + log.info("[WorkspacePathGuard] Default sandbox root: {}", defaultRoot); + } + + /** The registered global fallback sandbox root, or {@code null} if none is set. */ + @Nullable + public static Path getDefaultRoot() { + return defaultRoot; + } + /** True when {@code normalized} lives under the shared skill root (if one is set). */ private static boolean isUnderSkillRoot(Path normalized) { Path sr = skillRoot; @@ -196,9 +227,71 @@ public final class WorkspacePathGuard { /** ToolContext-aware overload — see {@link #validateShellCommand(String)}. */ public static void validateShellCommand(String command, @Nullable ToolContext ctx) { if (command == null || command.isEmpty()) return; - String basePath = resolveBasePath(ctx); - if (basePath == null || basePath.isBlank()) return; - Path root = Paths.get(basePath).toAbsolutePath().normalize(); + scanShellCommand(command, basePathToRoot(resolveBasePath(ctx))); + } + + /** + * Non-throwing boundary check for the guard layer. Returns a human-readable + * violation reason when {@code command} escapes the workspace identified by + * {@code basePath} (or deletes its root), or {@code null} when it is in + * bounds / no boundary is configured. {@code basePath} may be blank, in + * which case the global fallback sandbox root applies (same semantics as + * {@link #validateShellCommand(String, ToolContext)}). + */ + @Nullable + public static String findShellBoundaryViolation(String command, @Nullable String basePath) { + if (command == null || command.isEmpty()) return null; + Path root = basePathToRoot(basePath); + if (root == null) return null; + try { + scanShellCommand(command, root); + return null; + } catch (IllegalArgumentException e) { + return e.getMessage(); + } + } + + /** + * Non-throwing boundary check for a single filesystem path argument (e.g. + * the {@code filePath} of write_file / edit_file). Returns a violation + * reason or {@code null} when in bounds / no boundary is configured. + */ + @Nullable + public static String findPathBoundaryViolation(String rawPath, @Nullable String basePath) { + if (rawPath == null || rawPath.isBlank()) return null; + Path root = basePathToRoot(basePath); + if (root == null) return null; + Path normalized = Paths.get(rawPath).toAbsolutePath().normalize(); + if (!normalized.startsWith(root) && !isUnderSkillRoot(normalized)) { + return "Path is outside workspace boundary: " + normalized + ", allowed root: " + root; + } + return null; + } + + /** + * Resolve a base-path string to a normalized root, falling back to the + * global sandbox root when blank. {@code null} only when neither is set. + */ + @Nullable + private static Path basePathToRoot(@Nullable String basePath) { + if (basePath != null && !basePath.isBlank()) { + return Paths.get(basePath).toAbsolutePath().normalize(); + } + return defaultRoot; + } + + private static void scanShellCommand(String command, @Nullable Path root) { + if (root == null) return; + + // A delete whose target resolves to the workspace root itself is an + // escape even though the root is "inside" its own boundary. Detected + // alongside the path scans below; this flag gates those equality checks + // so non-destructive references to the root (`ls`, `cd `) stay + // allowed. + boolean destructive = DESTRUCTIVE_VERB.matcher(command).find(); + if (destructive && DOT_ARG.matcher(command).find()) { + throw rootDeletionError(root); + } // 1. Tilde — expands to $HOME, always outside a non-$HOME workspace. if (TILDE_REF.matcher(command).find()) { @@ -244,6 +337,9 @@ public final class WorkspacePathGuard { // idioms (`2>/dev/null`, `cmd <(cat file)`) keep working. continue; } + if (destructive && normalized.equals(root)) { + throw rootDeletionError(root); + } if (!normalized.startsWith(root) && !isUnderSkillRoot(normalized)) { throw new IllegalArgumentException( "Shell command references path outside workspace boundary: " @@ -265,6 +361,9 @@ public final class WorkspacePathGuard { continue; } if (isAllowedDeviceNode(resolved)) continue; + if (destructive && resolved.equals(root)) { + throw rootDeletionError(root); + } if (!resolved.startsWith(root) && !isUnderSkillRoot(resolved)) { throw new IllegalArgumentException( "Shell command uses parent-directory traversal that escapes the workspace: '" @@ -307,6 +406,23 @@ public final class WorkspacePathGuard { private static final Pattern RELATIVE_TRAVERSAL_TOKEN = Pattern.compile( "(?:^|[\\s|&;<>(`\"'={}])((?:[^\\s|&;<>()\"'`{}=/]+/)*\\.\\.(?:/[^\\s|&;<>()\"'`{}=]*)?)(?=[\\s|&;<>)`\"'=}]|$)"); + /** + * Destructive verbs that erase whatever path follows. Used to escalate a + * delete aimed at the workspace root itself into a boundary violation: the + * normal boundary check is reflexive ({@code root startsWith root}), so a + * delete of the root would otherwise pass — yet it wipes the entire sandbox. + */ + private static final Pattern DESTRUCTIVE_VERB = Pattern.compile( + "(?:^|[\\s|&;(`])(rm|rmdir|shred|srm)(?=[\\s])"); + + /** + * A bare {@code .} or {@code ./} standalone argument — when the shell cwd is + * the workspace root, {@code rm -rf .} / {@code rm -rf ./} erases the root's + * contents just like targeting the root by absolute path. + */ + private static final Pattern DOT_ARG = Pattern.compile( + "(?:^|[\\s])(\\.|\\./)(?=[\\s]|$)"); + /** Bare tilde or tilde at the start of a path token: {@code ~}, {@code ~/foo}, {@code "~/bar"}. */ private static final Pattern TILDE_REF = Pattern.compile( "(?:^|[\\s|&;<>(`\"'={}])~(?=[/\\s|&;<>)`\"'$]|$)"); @@ -349,6 +465,12 @@ public final class WorkspacePathGuard { return s.length() > 200 ? s.substring(0, 200) + "..." : s; } + private static IllegalArgumentException rootDeletionError(Path root) { + return new IllegalArgumentException( + "Shell command would delete the workspace root directory itself: " + root + + ". Deleting the workspace root is refused — target a path inside it instead."); + } + /** * Resolve the active workspace base path. Order of preference: *
    @@ -363,6 +485,16 @@ public final class WorkspacePathGuard { return origin.workspaceBasePath(); } } - return ToolExecutionContext.workspaceBasePath(); + String legacy = ToolExecutionContext.workspaceBasePath(); + if (legacy != null && !legacy.isBlank()) { + return legacy; + } + // Fail closed: with no per-conversation workspace configured, confine + // operations to the global fallback root rather than leaving them + // unconstrained against the entire filesystem. Only null when no + // default root is registered (tests / sandbox explicitly disabled), + // in which case the legacy no-boundary behaviour is preserved. + Path dr = defaultRoot; + return dr != null ? dr.toString() : null; } } diff --git a/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java b/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java new file mode 100644 index 00000000..8afb0c22 --- /dev/null +++ b/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java @@ -0,0 +1,114 @@ +package vip.mate.tool.guard.guardian; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Component; +import vip.mate.tool.guard.WorkspacePathGuard; +import vip.mate.tool.guard.model.*; + +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * Workspace filesystem-boundary guard. + *

    + * Backstops {@link WorkspacePathGuard} at the policy layer: when a shell command + * escapes the active workspace (absolute path, {@code ..} traversal, tilde/env + * expansion) or deletes the workspace root itself, this guardian emits a + * {@code CRITICAL} / {@code BLOCK} finding so the call is rejected + * before the human-approval prompt — not merely refused at execution + * time. A delete of the workspace root passes the reflexive boundary check + * ({@code root startsWith root}) yet destroys the whole sandbox, so it is + * treated as an escape. (Issue #313.) + *

    + * The boundary is read from {@link ToolInvocationContext#workspaceBasePath()}; + * when unset, the global fallback sandbox root applies via + * {@code WorkspacePathGuard}. When neither is configured, the guardian is a + * no-op and the legacy unconstrained behaviour is preserved. + */ +@Slf4j +@Component +public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { + + private static final Set SHELL_TOOL_NAMES = Set.of( + "execute_shell_command", "shell_execute", "run_command" + ); + + /** File tools and their JSON path-parameter name. */ + private static final Map FILE_PATH_PARAMS = Map.of( + "read_file", "filePath", + "write_file", "filePath", + "edit_file", "filePath" + ); + + private final ObjectMapper objectMapper = new ObjectMapper(); + + @Override + public boolean supports(ToolInvocationContext context) { + String tool = context.toolName(); + return tool != null && (SHELL_TOOL_NAMES.contains(tool) || FILE_PATH_PARAMS.containsKey(tool)); + } + + /** Run before the DB-rule guardians so a boundary escape blocks early. */ + @Override + public int priority() { + return 400; + } + + @Override + public List evaluate(ToolInvocationContext context) { + String tool = context.toolName(); + String rawArgs = context.rawArguments(); + if (tool == null || rawArgs == null || rawArgs.isEmpty()) { + return List.of(); + } + String basePath = context.workspaceBasePath(); + + if (SHELL_TOOL_NAMES.contains(tool)) { + String command = extractJsonParam(rawArgs, "command"); + if (command == null) command = rawArgs; + String violation = WorkspacePathGuard.findShellBoundaryViolation(command, basePath); + if (violation != null) { + return List.of(boundaryFinding(tool, "command", command, violation)); + } + return List.of(); + } + + String paramName = FILE_PATH_PARAMS.get(tool); + if (paramName != null) { + String path = extractJsonParam(rawArgs, paramName); + String violation = WorkspacePathGuard.findPathBoundaryViolation(path, basePath); + if (violation != null) { + return List.of(boundaryFinding(tool, "path", path, violation)); + } + } + return List.of(); + } + + private GuardFinding boundaryFinding(String toolName, String paramName, String matchValue, String reason) { + return new GuardFinding( + "WORKSPACE_BOUNDARY_ESCAPE", + GuardSeverity.CRITICAL, + GuardCategory.SENSITIVE_FILE_ACCESS, + "工作区越界", + reason, + "仅在工作区目录内操作；不要访问上层目录或删除工作区根目录", + toolName, + paramName, + "workspace_boundary", + matchValue, + GuardDecision.BLOCK); + } + + private String extractJsonParam(String rawArgs, String paramName) { + try { + Map params = objectMapper.readValue(rawArgs, new TypeReference<>() {}); + Object val = params.get(paramName); + return val instanceof String s ? s : null; + } catch (Exception e) { + return null; + } + } +} diff --git a/mateclaw-server/src/main/java/vip/mate/tool/guard/model/ToolInvocationContext.java b/mateclaw-server/src/main/java/vip/mate/tool/guard/model/ToolInvocationContext.java index f33e6cc5..acee1055 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/guard/model/ToolInvocationContext.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/guard/model/ToolInvocationContext.java @@ -21,7 +21,8 @@ public record ToolInvocationContext( String agentId, String channelType, String userId, - Long workspaceId + Long workspaceId, + String workspaceBasePath ) { /** @@ -32,7 +33,7 @@ public record ToolInvocationContext( String conversationId, String agentId) { return new ToolInvocationContext( toolName, Map.of(), rawArguments, conversationId, agentId, - null, null, null); + null, null, null, null); } /** @@ -48,6 +49,18 @@ public record ToolInvocationContext( toolName, parameters != null ? parameters : Map.of(), rawArguments, conversationId, agentId, - channelType, userId, workspaceId); + channelType, userId, workspaceId, null); + } + + /** + * Return a copy carrying the active workspace base path. Used by the guard + * engine so a guardian can enforce the workspace filesystem boundary (e.g. + * refuse a shell command that escapes it or deletes its root) before + * the approval prompt, rather than only at execution time. + */ + public ToolInvocationContext withWorkspaceBasePath(String basePath) { + return new ToolInvocationContext( + toolName, parameters, rawArguments, conversationId, agentId, + channelType, userId, workspaceId, basePath); } } diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxAutoConfiguration.java b/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxAutoConfiguration.java new file mode 100644 index 00000000..a789bba7 --- /dev/null +++ b/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxAutoConfiguration.java @@ -0,0 +1,47 @@ +package vip.mate.workspace.core.config; + +import lombok.extern.slf4j.Slf4j; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.context.annotation.Configuration; +import vip.mate.tool.guard.WorkspacePathGuard; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +/** + * Registers the global fallback sandbox root with {@link WorkspacePathGuard}. + *

    + * Without this, a workspace whose {@code base_path} is unset (the default state) + * leaves the path guard a no-op, so the agent's file and shell tools can reach + * anywhere the server process can. Pinning a fallback root makes the sandbox + * fail closed: unconfigured conversations are confined to a single directory + * instead of the whole filesystem. + * + * @author MateClaw Team + */ +@Slf4j +@Configuration +@EnableConfigurationProperties(WorkspaceSandboxProperties.class) +public class WorkspaceSandboxAutoConfiguration { + + public WorkspaceSandboxAutoConfiguration(WorkspaceSandboxProperties properties) { + if (!properties.isEnabled()) { + WorkspacePathGuard.setDefaultRoot(null); + log.warn("[WorkspaceSandbox] Fallback sandbox root disabled — conversations " + + "without a configured workspace base path run unconstrained"); + return; + } + Path root = Paths.get(properties.getRoot()).toAbsolutePath().normalize(); + try { + Files.createDirectories(root); + } catch (Exception e) { + // Registering the root still tightens the boundary even if the + // directory can't be pre-created; the shell cwd just won't be pinned + // to it until it exists. Log and continue rather than fail startup. + log.warn("[WorkspaceSandbox] Failed to create fallback sandbox root {}: {}", + root, e.getMessage()); + } + WorkspacePathGuard.setDefaultRoot(root.toString()); + } +} diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxProperties.java b/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxProperties.java new file mode 100644 index 00000000..37b4a0de --- /dev/null +++ b/mateclaw-server/src/main/java/vip/mate/workspace/core/config/WorkspaceSandboxProperties.java @@ -0,0 +1,36 @@ +package vip.mate.workspace.core.config; + +import lombok.Data; +import org.springframework.boot.context.properties.ConfigurationProperties; + +/** + * Workspace filesystem sandbox configuration. + *

    + * Backs the global fallback boundary enforced by + * {@link vip.mate.tool.guard.WorkspacePathGuard}. When a conversation has no + * per-workspace base path configured, file and shell tools are confined to + * {@link #root} instead of running unconstrained against the whole filesystem. + * This is the fail-closed default for the common out-of-the-box state where a + * workspace's {@code base_path} column is unset. + * + * @author MateClaw Team + */ +@Data +@ConfigurationProperties(prefix = "mateclaw.workspace.sandbox") +public class WorkspaceSandboxProperties { + + /** + * Whether the global fallback sandbox root is enforced. When {@code false}, + * conversations without a configured workspace base path run unconstrained + * (the legacy behaviour) — an escape hatch for operators who deliberately + * want agents to reach outside any single directory. + */ + private boolean enabled = true; + + /** + * Global fallback sandbox root, used when no per-workspace base path is set. + * Defaults to {@code /data/workspace}, alongside the H2 data + * directory. The directory is created at startup if missing. + */ + private String root = System.getProperty("user.dir") + "/data/workspace"; +} diff --git a/mateclaw-server/src/main/resources/application.yml b/mateclaw-server/src/main/resources/application.yml index a547ece6..0abb58b1 100644 --- a/mateclaw-server/src/main/resources/application.yml +++ b/mateclaw-server/src/main/resources/application.yml @@ -146,6 +146,15 @@ mateclaw: # an admin can move a noisy one to extension per server. # legacy: advertise every bound tool up front (pre-disclosure behavior). mode: ${MATECLAW_TOOLS_DISCLOSURE_MODE:progressive} + workspace: + sandbox: + # Global fallback filesystem boundary for file/shell tools. When a + # conversation has no per-workspace base path configured, operations are + # confined to this root instead of running unconstrained against the whole + # filesystem (fail-closed default). Set enabled=false to restore the legacy + # unconstrained behaviour for unconfigured conversations. + enabled: ${MATECLAW_WORKSPACE_SANDBOX_ENABLED:true} + root: ${MATECLAW_WORKSPACE_SANDBOX_ROOT:${user.dir}/data/workspace} skill: workspace: # Skill workspace root. Override with MATECLAW_SKILL_WORKSPACE_ROOT to diff --git a/mateclaw-server/src/test/java/vip/mate/approval/grant/ApprovalGrantResolverTest.java b/mateclaw-server/src/test/java/vip/mate/approval/grant/ApprovalGrantResolverTest.java index a880e418..c7101c71 100644 --- a/mateclaw-server/src/test/java/vip/mate/approval/grant/ApprovalGrantResolverTest.java +++ b/mateclaw-server/src/test/java/vip/mate/approval/grant/ApprovalGrantResolverTest.java @@ -112,7 +112,7 @@ class ApprovalGrantResolverTest { void null_workspace_id_falls_back_to_human() { ToolInvocationContext ctx = new ToolInvocationContext( "tool", java.util.Map.of(), "touch /tmp/x", "conv-1", "agent-1", - null, "user-1", /* workspaceId */ null); + null, "user-1", /* workspaceId */ null, /* workspaceBasePath */ null); var r = resolver.tryAutoApprove(ctx, evaluationWith(GuardSeverity.MEDIUM, "shell.exec")); assertThat(r.isRequiresHuman()).isTrue(); @@ -229,7 +229,8 @@ class ApprovalGrantResolverTest { private static ToolInvocationContext ctxWithArgs(String args) { return new ToolInvocationContext( "execute_shell_command", java.util.Map.of(), args, - "conv-1", "agent-1", null, "user-1", /* workspaceId */ 100L); + "conv-1", "agent-1", null, "user-1", /* workspaceId */ 100L, + /* workspaceBasePath */ null); } /** Builds a minimal GuardFinding using the 10-arg constructor (no decision / metadata). */ diff --git a/mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java b/mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java new file mode 100644 index 00000000..1a4b4871 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java @@ -0,0 +1,189 @@ +package vip.mate.tool.guard; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; +import vip.mate.tool.builtin.ToolExecutionContext; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Regression coverage for two workspace-sandbox escape paths (issue #313): + * + *

      + *
    1. Fail-closed default root — when a conversation has no + * per-workspace base path, operations must fall back to the registered + * global sandbox root instead of running unconstrained. Without the + * fallback, a fresh install (workspace {@code base_path} unset) lets the + * agent read/write/delete anywhere the server process can reach.
      2. + *
    2. Workspace-root deletion guard — the boundary check is reflexive + * ({@code root startsWith root}), so a delete aimed at the workspace root + * itself would otherwise pass and wipe the whole sandbox. Destructive + * commands targeting the root are rejected as escapes.
      3. + *
    + */ +@DisabledOnOs(OS.WINDOWS) // POSIX-style absolute paths in these cases +class WorkspacePathGuardSandboxTest { + + private static final String DEFAULT_ROOT = "/tmp/ws-guard-default-root"; + private static final String WORKSPACE = "/tmp/ws-guard-root-del-test"; + + @AfterEach + void teardown() { + ToolExecutionContext.clear(); + WorkspacePathGuard.setDefaultRoot(null); + WorkspacePathGuard.setSkillRoot(null); + } + + // ==================== Defect 1: fail-closed default root ==================== + + @Nested + @DisplayName("Fail-closed fallback to the global sandbox root") + class FailClosed { + + @BeforeEach + void setup() { + // No per-conversation workspace configured — the out-of-the-box state. + ToolExecutionContext.clear(); + WorkspacePathGuard.setDefaultRoot(DEFAULT_ROOT); + } + + @Test + @DisplayName("Shell: outside-the-root reads are rejected, not waved through") + void shellOutside_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("cat /etc/passwd")); + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("ls ..")); + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf /tmp/somewhere-else")); + } + + @Test + @DisplayName("Shell: paths inside the default root still pass") + void shellInside_pass() { + assertDoesNotThrow(() -> WorkspacePathGuard.validateShellCommand("ls -la")); + assertDoesNotThrow(() -> WorkspacePathGuard.validateShellCommand( + "cat " + DEFAULT_ROOT + "/foo.txt")); + } + + @Test + @DisplayName("validatePath: outside the default root is rejected") + void validatePathOutside_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validatePath("/etc/passwd")); + } + + @Test + @DisplayName("validatePath: inside the default root is allowed") + void validatePathInside_pass() { + assertDoesNotThrow(() -> + WorkspacePathGuard.validatePath(DEFAULT_ROOT + "/notes/new-file.txt")); + } + + @Test + @DisplayName("Per-conversation workspace still takes precedence over the default root") + void conversationWorkspace_wins() { + ToolExecutionContext.set("conv", "user", WORKSPACE); + // Inside the conversation workspace passes even though it's outside DEFAULT_ROOT. + assertDoesNotThrow(() -> WorkspacePathGuard.validateShellCommand( + "cat " + WORKSPACE + "/foo.txt")); + // The default root is NOT additionally trusted when a workspace is set. + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("cat " + DEFAULT_ROOT + "/foo.txt")); + } + } + + @Nested + @DisplayName("Escape hatch: no default root registered → legacy no-op") + class Disabled { + + @BeforeEach + void setup() { + ToolExecutionContext.clear(); + WorkspacePathGuard.setDefaultRoot(null); + } + + @Test + @DisplayName("Without a default root, unconfigured conversations are unconstrained") + void noDefaultRoot_noop() { + assertDoesNotThrow(() -> WorkspacePathGuard.validateShellCommand("cat /etc/passwd")); + assertDoesNotThrow(() -> WorkspacePathGuard.validatePath("/etc/passwd")); + } + } + + // ==================== Defect 2: workspace-root deletion guard ==================== + + @Nested + @DisplayName("Deleting the workspace root itself is refused") + class RootDeletion { + + @BeforeEach + void setup() { + ToolExecutionContext.set("conv", "user", WORKSPACE); + } + + @Test + @DisplayName("rm -rf (absolute) → rejected") + void rmRootAbsolute_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf " + WORKSPACE)); + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf " + WORKSPACE + "/")); + } + + @Test + @DisplayName("rmdir → rejected") + void rmdirRoot_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rmdir " + WORKSPACE)); + } + + @Test + @DisplayName("rm -rf . and rm -rf ./ (cwd is root) → rejected") + void rmDotInRoot_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf .")); + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf ./")); + } + + @Test + @DisplayName("rm -rf foo/.. (resolves to root) → rejected") + void rmTraversalToRoot_blocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("rm -rf foo/..")); + } + + @Test + @DisplayName("Deleting a path INSIDE the root still passes the boundary check") + void rmInsideRoot_pass() { + // Destructive but in-bounds — the approval layer, not the path guard, + // gates this. The guard must not over-block legitimate cleanup. + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("rm -rf subdir")); + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("rm -rf " + WORKSPACE + "/subdir")); + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("rm -rf ./build/output")); + } + + @Test + @DisplayName("Non-destructive references to the root are still allowed") + void nonDestructiveRoot_pass() { + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("ls " + WORKSPACE)); + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("cd " + WORKSPACE + " && ls")); + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("ls foo/..")); + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("find . -name '*.md'")); + } + } +} diff --git a/mateclaw-server/src/test/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardianTest.java b/mateclaw-server/src/test/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardianTest.java new file mode 100644 index 00000000..8d527d05 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardianTest.java @@ -0,0 +1,122 @@ +package vip.mate.tool.guard.guardian; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; +import vip.mate.tool.guard.WorkspacePathGuard; +import vip.mate.tool.guard.model.GuardDecision; +import vip.mate.tool.guard.model.GuardFinding; +import vip.mate.tool.guard.model.GuardSeverity; +import vip.mate.tool.guard.model.ToolInvocationContext; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Verifies that {@link WorkspaceBoundaryGuardian} turns a workspace-boundary + * escape (or a delete of the workspace root) into a hard, un-approvable BLOCK + * at the policy layer — before the human-approval prompt (issue #313). + */ +@DisabledOnOs(OS.WINDOWS) // POSIX-style absolute paths in these cases +class WorkspaceBoundaryGuardianTest { + + private static final String WORKSPACE = "/tmp/ws-boundary-guardian-test"; + private static final String DEFAULT_ROOT = "/tmp/ws-boundary-default-root"; + + private final WorkspaceBoundaryGuardian guardian = new WorkspaceBoundaryGuardian(); + + @AfterEach + void teardown() { + WorkspacePathGuard.setDefaultRoot(null); + } + + private ToolInvocationContext shell(String command, String basePath) { + String args = "{\"command\":\"" + command.replace("\"", "\\\"") + "\"}"; + return ToolInvocationContext.of("execute_shell_command", args, "conv", "agent") + .withWorkspaceBasePath(basePath); + } + + private ToolInvocationContext write(String path, String basePath) { + String args = "{\"filePath\":\"" + path + "\",\"content\":\"x\"}"; + return ToolInvocationContext.of("write_file", args, "conv", "agent") + .withWorkspaceBasePath(basePath); + } + + private void assertBlocked(List findings) { + assertFalse(findings.isEmpty(), "expected a boundary finding"); + GuardFinding f = findings.get(0); + assertEquals(GuardSeverity.CRITICAL, f.severity()); + assertEquals(GuardDecision.BLOCK, f.decision()); + assertEquals("WORKSPACE_BOUNDARY_ESCAPE", f.ruleId()); + } + + // ==================== Shell ==================== + + @Test + @DisplayName("Shell command escaping the workspace → CRITICAL BLOCK finding") + void shellEscape_blocked() { + assertBlocked(guardian.evaluate(shell("cat /etc/passwd", WORKSPACE))); + assertBlocked(guardian.evaluate(shell("ls ..", WORKSPACE))); + } + + @Test + @DisplayName("Deleting the workspace root → CRITICAL BLOCK finding") + void rootDeletion_blocked() { + assertBlocked(guardian.evaluate(shell("rm -rf " + WORKSPACE, WORKSPACE))); + assertBlocked(guardian.evaluate(shell("rm -rf .", WORKSPACE))); + } + + @Test + @DisplayName("In-bounds shell command → no finding") + void shellInBounds_pass() { + assertTrue(guardian.evaluate(shell("ls -la", WORKSPACE)).isEmpty()); + assertTrue(guardian.evaluate(shell("cat " + WORKSPACE + "/foo.txt", WORKSPACE)).isEmpty()); + assertTrue(guardian.evaluate(shell("rm -rf " + WORKSPACE + "/subdir", WORKSPACE)).isEmpty()); + } + + // ==================== File path tools ==================== + + @Test + @DisplayName("write_file outside the workspace → CRITICAL BLOCK finding") + void writeOutside_blocked() { + assertBlocked(guardian.evaluate(write("/etc/evil.conf", WORKSPACE))); + } + + @Test + @DisplayName("write_file inside the workspace → no finding") + void writeInside_pass() { + assertTrue(guardian.evaluate(write(WORKSPACE + "/notes.txt", WORKSPACE)).isEmpty()); + } + + // ==================== Default-root fallback & escape hatch ==================== + + @Test + @DisplayName("No per-workspace base path → falls back to the global default root") + void defaultRootFallback_blocks() { + WorkspacePathGuard.setDefaultRoot(DEFAULT_ROOT); + // basePath null on the context, but the default root still confines. + assertBlocked(guardian.evaluate(shell("cat /etc/passwd", null))); + assertTrue(guardian.evaluate(shell("cat " + DEFAULT_ROOT + "/foo.txt", null)).isEmpty()); + } + + @Test + @DisplayName("No base path and no default root → guardian is a no-op") + void noBoundary_noop() { + assertTrue(guardian.evaluate(shell("cat /etc/passwd", null)).isEmpty()); + assertTrue(guardian.evaluate(write("/etc/passwd", null)).isEmpty()); + } + + @Test + @DisplayName("supports() only fires for shell and file-path tools") + void supports_scope() { + assertTrue(guardian.supports(shell("ls", WORKSPACE))); + assertTrue(guardian.supports(write("a.txt", WORKSPACE))); + assertFalse(guardian.supports( + ToolInvocationContext.of("web_search", "{}", "conv", "agent"))); + } +} diff --git a/mateclaw-ui/src/components/chat/ChatInput.vue b/mateclaw-ui/src/components/chat/ChatInput.vue index 4314d60e..afc847ed 100644 --- a/mateclaw-ui/src/components/chat/ChatInput.vue +++ b/mateclaw-ui/src/components/chat/ChatInput.vue @@ -67,6 +67,9 @@ {{ getToolLabel(pendingApproval.toolName) }} {{ t('chat.approvalExecute') }} + + {{ approvalDetail }}