mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
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.
This commit is contained in:
matevip
parent
5ff58b00ad
commit
252a6fc425
@ -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 <workspace>/.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;
|
||||
}
|
||||
|
||||
|
||||
@ -87,12 +87,84 @@ public final class WorkspacePathGuard {
|
||||
return defaultRoot;
|
||||
}
|
||||
|
||||
/**
|
||||
* Additional always-trusted roots that sit <em>outside</em> 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<Path> 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
|
||||
|
||||
@ -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<String> SHELL_LANGUAGES = Set.of("bash", "sh", "shell");
|
||||
|
||||
/** File tools and their JSON path-parameter name. */
|
||||
private static final Map<String, String> 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<String, Object> params = objectMapper.readValue(rawArgs, new TypeReference<>() {});
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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<GuardFinding> 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<GuardFinding> 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();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user