From 252a6fc425c2afc55fd17bc60826d30a1246f17b Mon Sep 17 00:00:00 2001 From: matevip Date: Tue, 23 Jun 2026 10:11:54 +0800 Subject: [PATCH] fix(tool-guard): enforce workspace boundary for execute_code and trust spill roots (#403) execute_code (bash/sh/shell) bypassed the workspace boundary guard, so shell code run through it could read/write/delete paths outside the workspace sandbox (e.g. cat /etc/passwd) while the same paths were blocked for read_file and the shell tools. Bring execute_code under the guard (scan only shell-language code, report the code param), and trust the tool-result spill roots so a legitimate spilled result stays readable. Adds regression tests. --- .../graph/executor/ToolResultStorage.java | 38 +++++++++ .../mate/tool/guard/WorkspacePathGuard.java | 82 +++++++++++++++++-- .../guardian/WorkspaceBoundaryGuardian.java | 49 ++++++++++- .../guard/WorkspacePathGuardSandboxTest.java | 56 +++++++++++++ .../WorkspaceBoundaryGuardianTest.java | 73 ++++++++++++++++- 5 files changed, 291 insertions(+), 7 deletions(-) diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java index 426bf067..d27cf8fc 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java @@ -1,8 +1,10 @@ package vip.mate.agent.graph.executor; +import jakarta.annotation.PostConstruct; import lombok.extern.slf4j.Slf4j; import org.springframework.ai.chat.messages.ToolResponseMessage; import vip.mate.agent.context.StructuredTruncator; +import vip.mate.tool.guard.WorkspacePathGuard; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Configuration; import org.springframework.stereotype.Component; @@ -73,6 +75,30 @@ public class ToolResultStorage { this.excludedToolsSnapshot = props.excludedToolsSet(); } + /** + * Trust the deterministic spill roots with the workspace path guard at + * startup, before any spill happens in this JVM. Without this, a + * conversation that spilled in a previous run and is then resumed after a + * restart would have its {@code read_file} of the still-on-disk spill path + * rejected as a boundary escape until the next spill re-registers the root. + * The per-workspace branch ({@code /.mateclaw/tool-results}) is + * intentionally not registered here — it already sits inside its own + * workspace boundary. + */ + @PostConstruct + void registerSpillRootsAsTrusted() { + if (!props.isEnabled()) { + return; + } + if (!props.getStorageBaseDir().isEmpty()) { + WorkspacePathGuard.addTrustedRoot(props.getStorageBaseDir()); + } + String tmp = System.getProperty("java.io.tmpdir"); + if (tmp != null && !tmp.isEmpty()) { + WorkspacePathGuard.addTrustedRoot(Paths.get(tmp, "mateclaw", "tool-results").toString()); + } + } + /** D-6: current cumulative spill count (monotonically increasing). */ public long getSpillCount() { return spillCount.get(); @@ -283,18 +309,30 @@ public class ToolResultStorage { private Path resolveBaseDir(String workspaceBasePath) { Path base; + boolean outsideWorkspace; if (!props.getStorageBaseDir().isEmpty()) { base = Paths.get(props.getStorageBaseDir()); + outsideWorkspace = true; } else if (workspaceBasePath != null && !workspaceBasePath.isBlank()) { + // Inside the workspace boundary already — read_file of these spill + // files is permitted without an extra trusted-root registration. base = Paths.get(workspaceBasePath, ".mateclaw", "tool-results"); + outsideWorkspace = false; } else { String tmp = System.getProperty("java.io.tmpdir"); if (tmp == null || tmp.isEmpty()) return null; base = Paths.get(tmp, "mateclaw", "tool-results"); + outsideWorkspace = true; } // Register so the retention sweep and conversation-delete hook can // reach this root even when the workspace path is no longer in scope. observedRoots.add(base); + // A spill directory that lives outside the workspace must be trusted by + // the path guard; otherwise the read_file the spill preview tells the + // agent to perform is rejected as a workspace-boundary escape. + if (outsideWorkspace) { + WorkspacePathGuard.addTrustedRoot(base.toString()); + } return base; } 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 d0b329ec..59cb8346 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 @@ -87,12 +87,84 @@ public final class WorkspacePathGuard { return defaultRoot; } + /** + * Additional always-trusted roots that sit outside any workspace + * boundary yet must remain readable by the agent. The tool-result spill + * store registers its base directories here: when a tool produces an + * oversized result it is written to disk and the agent is handed back a + * path with the instruction to {@code read_file} it on demand. That spill + * directory may live outside the workspace (a central + * {@code storage-base-dir} or the {@code ${java.io.tmpdir}} fallback), so + * without this allow-list the very read the agent is told to perform would + * be rejected as a boundary escape. Registered roots are matched exactly + * like {@link #skillRoot} — by {@code startsWith} on the normalized path. + */ + private static final Set trustedRoots = java.util.concurrent.ConcurrentHashMap.newKeySet(); + + /** + * Register an additional always-trusted root (e.g. a tool-result spill + * directory). A {@code null} or blank path is ignored. Idempotent. + */ + public static void addTrustedRoot(@Nullable String path) { + if (path == null || path.isBlank()) { + return; + } + Path normalized = Paths.get(path).toAbsolutePath().normalize(); + if (trustedRoots.add(normalized)) { + log.info("[WorkspacePathGuard] Trusted root added: {}", normalized); + } + } + + /** Clear every registered trusted root. Intended for test teardown. */ + public static void clearTrustedRoots() { + trustedRoots.clear(); + } + /** True when {@code normalized} lives under the shared skill root (if one is set). */ private static boolean isUnderSkillRoot(Path normalized) { Path sr = skillRoot; return sr != null && normalized.startsWith(sr); } + /** + * True when {@code normalized} (or its symlink-resolved real path) lives + * under the shared skill root or any registered {@link #trustedRoots}. + * Bundles the skill-root and trusted-root checks so every boundary check + * site stays a single call. + */ + private static boolean isExempt(Path normalized) { + if (isUnderSkillRoot(normalized)) { + return true; + } + for (Path root : trustedRoots) { + if (normalized.startsWith(root)) { + return true; + } + } + return false; + } + + /** Symlink-resolved variant of {@link #isExempt}. */ + private static boolean isExemptReal(Path realPath) { + if (isUnderSkillRootReal(realPath)) { + return true; + } + for (Path root : trustedRoots) { + if (realPath.startsWith(root)) { + return true; + } + try { + Path realRoot = root.toFile().exists() ? root.toRealPath() : root; + if (realPath.startsWith(realRoot)) { + return true; + } + } catch (IOException e) { + // fall through — the plain startsWith above already ran + } + } + return false; + } + /** * Symlink-resolved variant of {@link #isUnderSkillRoot}. Resolves the skill * root's real path so a path whose real location lands inside the skill @@ -142,7 +214,7 @@ public final class WorkspacePathGuard { Path root = Paths.get(basePath).toAbsolutePath().normalize(); // 先用 normalize 检查，再尝试 toRealPath 防符号链接逃逸 - if (!normalized.startsWith(root) && !isUnderSkillRoot(normalized)) { + if (!normalized.startsWith(root) && !isExempt(normalized)) { throw new IllegalArgumentException( "Path is outside workspace boundary: " + normalized + ", allowed root: " + root); } @@ -152,7 +224,7 @@ public final class WorkspacePathGuard { if (normalized.toFile().exists()) { Path realPath = normalized.toRealPath(); Path realRoot = root.toFile().exists() ? root.toRealPath() : root; - if (!realPath.startsWith(realRoot) && !isUnderSkillRootReal(realPath)) { + if (!realPath.startsWith(realRoot) && !isExemptReal(realPath)) { throw new IllegalArgumentException( "Path escapes workspace via symlink: " + realPath + ", allowed root: " + realRoot); } @@ -262,7 +334,7 @@ public final class WorkspacePathGuard { Path root = basePathToRoot(basePath); if (root == null) return null; Path normalized = Paths.get(rawPath).toAbsolutePath().normalize(); - if (!normalized.startsWith(root) && !isUnderSkillRoot(normalized)) { + if (!normalized.startsWith(root) && !isExempt(normalized)) { return "Path is outside workspace boundary: " + normalized + ", allowed root: " + root; } return null; @@ -340,7 +412,7 @@ public final class WorkspacePathGuard { if (destructive && normalized.equals(root)) { throw rootDeletionError(root); } - if (!normalized.startsWith(root) && !isUnderSkillRoot(normalized)) { + if (!normalized.startsWith(root) && !isExempt(normalized)) { throw new IllegalArgumentException( "Shell command references path outside workspace boundary: " + normalized + ", allowed root: " + root); @@ -364,7 +436,7 @@ public final class WorkspacePathGuard { if (destructive && resolved.equals(root)) { throw rootDeletionError(root); } - if (!resolved.startsWith(root) && !isUnderSkillRoot(resolved)) { + if (!resolved.startsWith(root) && !isExempt(resolved)) { throw new IllegalArgumentException( "Shell command uses parent-directory traversal that escapes the workspace: '" + candidate + "' would resolve to " + resolved 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 index 8afb0c22..5cfcd920 100644 --- 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 @@ -8,6 +8,7 @@ import vip.mate.tool.guard.WorkspacePathGuard; import vip.mate.tool.guard.model.*; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; @@ -36,6 +37,25 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { "execute_shell_command", "shell_execute", "run_command" ); + /** + * Inline code-execution tool. Its shell content lives in the {@code code} + * JSON parameter (selected by a sibling {@code language} parameter), not in + * a {@code command} parameter, so it needs its own extraction path. Only + * shell-language code is screened for boundary escapes — see + * {@link #SHELL_LANGUAGES} and {@link #evaluate}. + */ + private static final String CODE_TOOL_NAME = "execute_code"; + + /** + * {@code language} values whose {@code code} is shell script and can be + * scanned with the shell-syntax boundary scanner. Mirrors the {@code .sh} + * aliases accepted by the code-execution runtime. Python/Node code is not + * scanned: the shell scanner would false-positive on absolute-path string + * literals while missing interpreter-specific file access, so applying it + * there is both noisy and incomplete. + */ + private static final Set SHELL_LANGUAGES = Set.of("bash", "sh", "shell"); + /** File tools and their JSON path-parameter name. */ private static final Map FILE_PATH_PARAMS = Map.of( "read_file", "filePath", @@ -48,7 +68,9 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { @Override public boolean supports(ToolInvocationContext context) { String tool = context.toolName(); - return tool != null && (SHELL_TOOL_NAMES.contains(tool) || FILE_PATH_PARAMS.containsKey(tool)); + return tool != null && (SHELL_TOOL_NAMES.contains(tool) + || CODE_TOOL_NAME.equals(tool) + || FILE_PATH_PARAMS.containsKey(tool)); } /** Run before the DB-rule guardians so a boundary escape blocks early. */ @@ -76,6 +98,26 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { return List.of(); } + if (CODE_TOOL_NAME.equals(tool)) { + // The shell content lives in the `code` param, gated by `language`. + // Scan only shell-language code, and extract `code` explicitly + // rather than scanning the whole rawArgs JSON — the latter would + // false-positive on Python/Node source that merely contains an + // absolute-path string literal. + if (!isShellLanguage(extractJsonParam(rawArgs, "language"))) { + return List.of(); + } + String code = extractJsonParam(rawArgs, "code"); + if (code == null) { + return List.of(); + } + String violation = WorkspacePathGuard.findShellBoundaryViolation(code, basePath); + if (violation != null) { + return List.of(boundaryFinding(tool, "code", code, violation)); + } + return List.of(); + } + String paramName = FILE_PATH_PARAMS.get(tool); if (paramName != null) { String path = extractJsonParam(rawArgs, paramName); @@ -102,6 +144,11 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { GuardDecision.BLOCK); } + /** True when {@code language} names a shell interpreter (bash/sh/shell). */ + private static boolean isShellLanguage(String language) { + return language != null && SHELL_LANGUAGES.contains(language.trim().toLowerCase(Locale.ROOT)); + } + private String extractJsonParam(String rawArgs, String paramName) { try { Map params = objectMapper.readValue(rawArgs, new TypeReference<>() {}); 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 index 1a4b4871..89d6f526 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/guard/WorkspacePathGuardSandboxTest.java @@ -38,6 +38,7 @@ class WorkspacePathGuardSandboxTest { ToolExecutionContext.clear(); WorkspacePathGuard.setDefaultRoot(null); WorkspacePathGuard.setSkillRoot(null); + WorkspacePathGuard.clearTrustedRoots(); } // ==================== Defect 1: fail-closed default root ==================== @@ -117,6 +118,61 @@ class WorkspacePathGuardSandboxTest { } } + // ============ Tool-result spill dir is trusted outside the boundary (issue #403) ============ + + @Nested + @DisplayName("A registered tool-result spill root is readable outside the workspace boundary") + class TrustedSpillRoot { + + private static final String SPILL_ROOT = "/tmp/mate-tool-result-spill/tool-results"; + + @BeforeEach + void setup() { + // A conversation bound to a workspace, with a central spill dir that + // lives outside that workspace — the production scenario from #403. + ToolExecutionContext.set("conv", "user", WORKSPACE); + WorkspacePathGuard.addTrustedRoot(SPILL_ROOT); + } + + @Test + @DisplayName("validatePath: reading a spilled tool result outside the workspace is allowed") + void validatePathSpill_pass() { + assertDoesNotThrow(() -> + WorkspacePathGuard.validatePath(SPILL_ROOT + "/conv/call_2.txt")); + } + + @Test + @DisplayName("findPathBoundaryViolation: spill path reports no violation") + void findPathBoundaryViolationSpill_null() { + org.junit.jupiter.api.Assertions.assertNull( + WorkspacePathGuard.findPathBoundaryViolation( + SPILL_ROOT + "/conv/call_2.txt", WORKSPACE)); + } + + @Test + @DisplayName("Shell: cat-ing a spilled tool result outside the workspace is allowed") + void shellSpill_pass() { + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("cat " + SPILL_ROOT + "/conv/call_2.txt")); + } + + @Test + @DisplayName("A non-spill path outside the workspace is still blocked") + void unrelatedOutside_stillBlocked() { + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validatePath("/etc/passwd")); + assertThrows(IllegalArgumentException.class, () -> + WorkspacePathGuard.validateShellCommand("cat /etc/passwd")); + } + + @Test + @DisplayName("Deleting inside the trusted spill root is not a root-deletion escape") + void deleteInsideSpillRoot_pass() { + assertDoesNotThrow(() -> + WorkspacePathGuard.validateShellCommand("rm -rf " + SPILL_ROOT + "/conv")); + } + } + // ==================== Defect 2: workspace-root deletion guard ==================== @Nested 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 index 8d527d05..42347e92 100644 --- 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 @@ -47,6 +47,13 @@ class WorkspaceBoundaryGuardianTest { .withWorkspaceBasePath(basePath); } + private ToolInvocationContext code(String language, String src, String basePath) { + String args = "{\"language\":\"" + language + "\",\"code\":\"" + + src.replace("\"", "\\\"") + "\"}"; + return ToolInvocationContext.of("execute_code", args, "conv", "agent") + .withWorkspaceBasePath(basePath); + } + private void assertBlocked(List findings) { assertFalse(findings.isEmpty(), "expected a boundary finding"); GuardFinding f = findings.get(0); @@ -79,6 +86,50 @@ class WorkspaceBoundaryGuardianTest { assertTrue(guardian.evaluate(shell("rm -rf " + WORKSPACE + "/subdir", WORKSPACE)).isEmpty()); } + // ==================== Inline code execution (execute_code) ==================== + + @Test + @DisplayName("execute_code bash escaping the workspace → CRITICAL BLOCK finding") + void codeBashEscape_blocked() { + // The #403 reproduction: `cat /tmp/...` and reading /etc/passwd from + // shell-language code must be blocked just like the shell tool. + assertBlocked(guardian.evaluate(code("bash", "cat /etc/passwd", WORKSPACE))); + assertBlocked(guardian.evaluate(code("sh", "cat /tmp/mate-tool-result-spill/x", WORKSPACE))); + assertBlocked(guardian.evaluate(code("shell", "ls ..", WORKSPACE))); + } + + @Test + @DisplayName("execute_code bash deleting the workspace root → CRITICAL BLOCK finding") + void codeBashRootDeletion_blocked() { + assertBlocked(guardian.evaluate(code("bash", "rm -rf " + WORKSPACE, WORKSPACE))); + } + + @Test + @DisplayName("execute_code bash inside the workspace → no finding") + void codeBashInBounds_pass() { + assertTrue(guardian.evaluate(code("bash", "ls -la", WORKSPACE)).isEmpty()); + assertTrue(guardian.evaluate(code("bash", "cat " + WORKSPACE + "/foo.txt", WORKSPACE)).isEmpty()); + } + + @Test + @DisplayName("execute_code reports the violating param as 'code'") + void codeViolation_paramName() { + List findings = guardian.evaluate(code("bash", "cat /etc/passwd", WORKSPACE)); + assertFalse(findings.isEmpty()); + assertEquals("code", findings.get(0).paramName()); + } + + @Test + @DisplayName("execute_code python/node is not path-scanned (avoids string-literal false positives)") + void codeNonShell_notScanned() { + // A Python/Node literal containing an absolute path must NOT be treated + // as a shell boundary escape — the static shell scan doesn't apply. + assertTrue(guardian.evaluate(code("python", "open('/etc/passwd')", WORKSPACE)).isEmpty()); + assertTrue(guardian.evaluate(code("node", "fs.readFileSync('/etc/passwd')", WORKSPACE)).isEmpty()); + // Unknown / missing language is likewise not scanned. + assertTrue(guardian.evaluate(code("ruby", "File.read('/etc/passwd')", WORKSPACE)).isEmpty()); + } + // ==================== File path tools ==================== @Test @@ -112,11 +163,31 @@ class WorkspaceBoundaryGuardianTest { } @Test - @DisplayName("supports() only fires for shell and file-path tools") + @DisplayName("supports() fires for shell, code, and file-path tools") void supports_scope() { assertTrue(guardian.supports(shell("ls", WORKSPACE))); + assertTrue(guardian.supports(code("bash", "ls", WORKSPACE))); assertTrue(guardian.supports(write("a.txt", WORKSPACE))); assertFalse(guardian.supports( ToolInvocationContext.of("web_search", "{}", "conv", "agent"))); } + + // ==================== Tool-result spill dir stays reachable (issue #403) ==================== + + @Test + @DisplayName("execute_code can still cat a legitimate spilled tool result outside the workspace") + void codeBashSpill_pass() { + String spillRoot = "/tmp/mate-tool-result-spill/tool-results"; + WorkspacePathGuard.addTrustedRoot(spillRoot); + try { + // The trusted-root mechanism added for #403 must keep working once + // execute_code is brought under the boundary guard: reading a real + // spill path is allowed, an unrelated outside path is still blocked. + assertTrue(guardian.evaluate( + code("bash", "cat " + spillRoot + "/conv/call_2.txt", WORKSPACE)).isEmpty()); + assertBlocked(guardian.evaluate(code("bash", "cat /etc/passwd", WORKSPACE))); + } finally { + WorkspacePathGuard.clearTrustedRoots(); + } + } }