mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
fix(tool): close three sandbox follow-up gaps surfaced by review
1. Relative parent traversal in shell commands (HIGH) validateShellCommand only scanned absolute path tokens, so commands like `cat ../mateclaw/CLAUDE.md`, `cd .. && cat foo`, or `ln -sf ../bar breakout` had no absolute path to trip the check. From a workspace cwd that's a real escape — `..` segments resolve against the JVM cwd at file-tool time and reach anywhere the user can read. Add a second pass: any token containing `..` as a path segment is resolved against the workspace root via root.resolve(token). normalize(); reject when the result falls outside. In-workspace traversal like `subdir/../sibling` normalizes back inside and passes. Identifiers without slashes (e.g. version strings with `1.2..3`) are not treated as paths. 2. Shell validation and process working directory used different context sources (MEDIUM) execute_shell_command validated with the explicit ToolContext, but buildShellProcess called WorkspacePathGuard.getWorkingDirectory() (no-arg), which only sees the ThreadLocal fallback. Today the ToolExecutionExecutor sets both so the discrepancy is latent, but a future direct Spring AI invocation passing only ToolContext would validate against one basePath and exec against another. Thread ctx through buildShellProcess and call getWorkingDirectory(ctx) so validation and execution agree on a single source of truth. 3. Absolute agent override could disable workspace scoping (MEDIUM) resolveAgentBasePath accepted an absolute override verbatim, even when it pointed outside the workspace root. An admin (or any account with agent-edit permission) could set workspaceBasePath="/" or another team's repo and bypass workspace boundaries entirely. When a workspace has its own basePath, require absolute overrides to sit underneath it. The caller in build() catches the rejection, logs WARN, and falls back to the workspace basePath so chat stays available rather than crashing agent construction. When the workspace has no basePath there's no boundary to enforce, so legacy behavior is preserved. Test coverage: WorkspacePathGuardShellTest grows from 17 to 23 (six new cases for `cd ..`, relative parent traversal, relative symlink escape, deeper traversal, in-workspace normalization, and the identifier false-positive guard). AgentGraphBuilderBasePathResolutionTest grows from 7 to 10 (three new cases for in-workspace absolute, outside-workspace absolute rejection, and no-workspace legacy behavior). All 45 sandbox-area tests pass with no regressions.
This commit is contained in:
matevip
parent
7272d8f633
commit
a37074a9a6
@ -375,11 +375,23 @@ public class AgentGraphBuilder {
|
||||
entity.getName(), e.getMessage());
|
||||
}
|
||||
}
|
||||
String resolvedBase = resolveAgentBasePath(entity.getWorkspaceBasePath(), workspaceBase);
|
||||
String resolvedBase;
|
||||
try {
|
||||
resolvedBase = resolveAgentBasePath(entity.getWorkspaceBasePath(), workspaceBase);
|
||||
} catch (IllegalArgumentException e) {
|
||||
// Override violates the workspace-scoping rule (e.g. admin tried to
|
||||
// set an absolute path outside the workspace root). Fall back to
|
||||
// inheriting the workspace basePath so chat stays available, but
|
||||
// surface the violation in logs so the admin can fix it.
|
||||
log.warn("Agent {} workspaceBasePath override rejected, falling back to workspace: {}",
|
||||
entity.getName(), e.getMessage());
|
||||
resolvedBase = workspaceBase;
|
||||
}
|
||||
if (resolvedBase != null && !resolvedBase.isBlank()) {
|
||||
agent.workspaceBasePath = resolvedBase;
|
||||
boolean fromOverride = entity.getWorkspaceBasePath() != null
|
||||
&& !entity.getWorkspaceBasePath().isBlank();
|
||||
&& !entity.getWorkspaceBasePath().isBlank()
|
||||
&& resolvedBase.equals(entity.getWorkspaceBasePath());
|
||||
log.info("Agent {} basePath = {} (source: {})",
|
||||
entity.getName(), resolvedBase, fromOverride ? "agent-override" : "workspace");
|
||||
}
|
||||
@ -1165,7 +1177,12 @@ public class AgentGraphBuilder {
|
||||
* <p>Precedence:
|
||||
* <ol>
|
||||
* <li>When the agent-level override is set, it wins.</li>
|
||||
* <li>An absolute override is used verbatim.</li>
|
||||
* <li>An absolute override is used verbatim, but only when it sits
|
||||
* inside the workspace basePath (or when the workspace has no
|
||||
* basePath of its own). An absolute path that points outside a
|
||||
* configured workspace root is rejected — otherwise a less-trusted
|
||||
* user with agent-edit access could set
|
||||
* {@code workspaceBasePath="/"} and bypass workspace scoping.</li>
|
||||
* <li>A relative override is resolved <em>under</em> the workspace basePath
|
||||
* when the workspace has one, matching the UI hint that agent paths
|
||||
* are relative to the workspace root.</li>
|
||||
@ -1174,6 +1191,9 @@ public class AgentGraphBuilder {
|
||||
* <li>With no override, the workspace basePath is inherited verbatim;
|
||||
* returns {@code null} when neither side has a value.</li>
|
||||
* </ol>
|
||||
*
|
||||
* @throws IllegalArgumentException when an absolute override escapes the
|
||||
* workspace root
|
||||
*/
|
||||
static String resolveAgentBasePath(String agentOverride, String workspaceBase) {
|
||||
boolean hasOverride = agentOverride != null && !agentOverride.isBlank();
|
||||
@ -1183,6 +1203,15 @@ public class AgentGraphBuilder {
|
||||
}
|
||||
Path overridePath = Paths.get(agentOverride);
|
||||
if (overridePath.isAbsolute()) {
|
||||
if (hasWorkspace) {
|
||||
Path wsRoot = Paths.get(workspaceBase).toAbsolutePath().normalize();
|
||||
Path absOverride = overridePath.toAbsolutePath().normalize();
|
||||
if (!absOverride.startsWith(wsRoot)) {
|
||||
throw new IllegalArgumentException(
|
||||
"Agent workspaceBasePath override must be inside the workspace root: "
|
||||
+ absOverride + " is not under " + wsRoot);
|
||||
}
|
||||
}
|
||||
return agentOverride;
|
||||
}
|
||||
if (hasWorkspace) {
|
||||
|
||||
@ -91,7 +91,7 @@ public class ShellExecuteTool {
|
||||
// Windows cmd.exe 会在第一个换行处截断命令，Unix sh 也可能误解
|
||||
String sanitizedCommand = collapseEmbeddedNewlines(command);
|
||||
|
||||
ProcessBuilder pb = buildShellProcess(sanitizedCommand);
|
||||
ProcessBuilder pb = buildShellProcess(sanitizedCommand, ctx);
|
||||
// 不继承环境变量中的敏感信息
|
||||
pb.environment().keySet().removeIf(key ->
|
||||
key.contains("KEY") || key.contains("SECRET") || key.contains("TOKEN")
|
||||
@ -157,7 +157,7 @@ public class ShellExecuteTool {
|
||||
* from the calling environment still apply; falls back to /bin/sh
|
||||
* when $SHELL is unset or points at a non-executable path.
|
||||
*/
|
||||
private static ProcessBuilder buildShellProcess(String command) {
|
||||
private static ProcessBuilder buildShellProcess(String command, @Nullable ToolContext ctx) {
|
||||
ProcessBuilder pb;
|
||||
if (IS_WINDOWS) {
|
||||
String winCommand = sanitizeWindowsCommand(command);
|
||||
@ -167,8 +167,13 @@ public class ShellExecuteTool {
|
||||
pb = new ProcessBuilder(shell, "-c", command);
|
||||
}
|
||||
|
||||
// 设置工作区活动目录
|
||||
java.nio.file.Path workingDir = vip.mate.tool.guard.WorkspacePathGuard.getWorkingDirectory();
|
||||
// Pin the process cwd to the same workspace basePath the validator
|
||||
// checked against. Using getWorkingDirectory(ctx) (not the no-arg
|
||||
// ThreadLocal-only overload) keeps validation and execution on a
|
||||
// single source of truth — otherwise a caller that only sets
|
||||
// ToolContext could validate against one basePath and run with the
|
||||
// ThreadLocal fallback's basePath.
|
||||
java.nio.file.Path workingDir = vip.mate.tool.guard.WorkspacePathGuard.getWorkingDirectory(ctx);
|
||||
if (workingDir != null && java.nio.file.Files.isDirectory(workingDir)) {
|
||||
pb.directory(workingDir.toFile());
|
||||
log.info("[ShellExecute] Working directory set to: {}", workingDir);
|
||||
|
||||
@ -114,6 +114,11 @@ public final class WorkspacePathGuard {
|
||||
* {@code cd /var}) whose normalized form is not under the workspace
|
||||
* root — even when nested inside command substitution {@code $(...)}
|
||||
* or backticks;</li>
|
||||
* <li>relative tokens containing {@code ..} as a directory segment
|
||||
* (e.g. {@code cd ..}, {@code cat ../foo}, {@code ln -s ../bar baz})
|
||||
* when the resolved path falls outside the workspace root —
|
||||
* in-workspace traversal like {@code subdir/../sibling} is allowed
|
||||
* because it normalizes back inside;</li>
|
||||
* <li>tilde expansion ({@code ~}, {@code ~/...}) — always resolves to
|
||||
* {@code $HOME}, which sits outside the workspace;</li>
|
||||
* <li>references to environment variables ({@code $HOME}, {@code ${USER}},
|
||||
@ -193,6 +198,28 @@ public final class WorkspacePathGuard {
|
||||
+ normalized + ", allowed root: " + root);
|
||||
}
|
||||
}
|
||||
|
||||
// 4. Relative tokens containing ".." — must resolve inside the workspace.
|
||||
// Catches `cd ..`, `cat ../foo`, `ln -s ../bar baz`, `mv foo/../bar dst`,
|
||||
// etc. In-workspace traversal (`subdir/../sibling`) normalizes back
|
||||
// inside and passes.
|
||||
Matcher traversalMatch = RELATIVE_TRAVERSAL_TOKEN.matcher(command);
|
||||
while (traversalMatch.find()) {
|
||||
String candidate = traversalMatch.group(1);
|
||||
Path resolved;
|
||||
try {
|
||||
resolved = root.resolve(candidate).normalize();
|
||||
} catch (Exception ex) {
|
||||
continue;
|
||||
}
|
||||
if (isAllowedDeviceNode(resolved)) continue;
|
||||
if (!resolved.startsWith(root)) {
|
||||
throw new IllegalArgumentException(
|
||||
"Shell command uses parent-directory traversal that escapes the workspace: '"
|
||||
+ candidate + "' would resolve to " + resolved
|
||||
+ ", allowed root: " + root);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@ -206,6 +233,28 @@ public final class WorkspacePathGuard {
|
||||
private static final Pattern ABS_PATH_TOKEN = Pattern.compile(
|
||||
"(?:^|[\\s|&;<>(`\"'={}])(?<!:)(/[^\\s|&;<>()\"'`{}=]+)");
|
||||
|
||||
/**
|
||||
* Match relative tokens that contain {@code ..} as a path segment. Captures
|
||||
* the whole token (prefix + {@code ..} + optional suffix) so the caller
|
||||
* can resolve it against the workspace root and decide whether it escapes.
|
||||
*
|
||||
* <p>Matches:
|
||||
* <ul>
|
||||
* <li>{@code ..} ({@code cd ..}, bare arg)</li>
|
||||
* <li>{@code ../foo/bar} (relative parent traversal)</li>
|
||||
* <li>{@code ./..} ({@code cd ./..})</li>
|
||||
* <li>{@code foo/..} ({@code rm foo/..})</li>
|
||||
* <li>{@code foo/../bar} (in-workspace normalization)</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>Does NOT match {@code abc..xyz} (no slash before/after the {@code ..} —
|
||||
* not a path segment) or absolute {@code /foo/../bar} (handled by
|
||||
* {@link #ABS_PATH_TOKEN}). The token must be bounded by a shell separator
|
||||
* or end-of-string on both sides.
|
||||
*/
|
||||
private static final Pattern RELATIVE_TRAVERSAL_TOKEN = Pattern.compile(
|
||||
"(?:^|[\\s|&;<>(`\"'={}])((?:[^\\s|&;<>()\"'`{}=/]+/)*\\.\\.(?:/[^\\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|&;<>)`\"'$]|$)");
|
||||
|
||||
@ -39,21 +39,21 @@ class AgentGraphBuilderBasePathResolutionTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Agent override is absolute → used verbatim, workspace ignored")
|
||||
@DisplayName("Agent override is absolute and inside workspace → used verbatim")
|
||||
@DisabledOnOs(OS.WINDOWS)
|
||||
void absoluteOverride_usedAsIs_unix() {
|
||||
assertEquals("/opt/agents/code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/opt/agents/code-review", "/srv/ws-root"));
|
||||
void absoluteOverride_insideWs_usedAsIs_unix() {
|
||||
assertEquals("/srv/ws-root/agents/code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/srv/ws-root/agents/code-review", "/srv/ws-root"));
|
||||
assertEquals("/opt/agents/code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/opt/agents/code-review", null));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Agent override is absolute (Windows) → used verbatim")
|
||||
@DisplayName("Agent override is absolute (Windows) and inside workspace → used verbatim")
|
||||
@EnabledOnOs(OS.WINDOWS)
|
||||
void absoluteOverride_usedAsIs_windows() {
|
||||
assertEquals("C:\\agents\\code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("C:\\agents\\code-review", "C:\\ws-root"));
|
||||
void absoluteOverride_insideWs_usedAsIs_windows() {
|
||||
assertEquals("C:\\ws-root\\agents\\code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("C:\\ws-root\\agents\\code-review", "C:\\ws-root"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -79,4 +79,44 @@ class AgentGraphBuilderBasePathResolutionTest {
|
||||
assertEquals("agent-dir",
|
||||
AgentGraphBuilder.resolveAgentBasePath("agent-dir", " "));
|
||||
}
|
||||
|
||||
// ==================== Absolute override scoped to workspace root ====================
|
||||
|
||||
@Test
|
||||
@DisplayName("Absolute override inside workspace root → allowed verbatim")
|
||||
@DisabledOnOs(OS.WINDOWS)
|
||||
void absoluteOverride_insideWorkspace_allowed() {
|
||||
// /srv/ws-root/agents/code-review starts with /srv/ws-root → fine.
|
||||
assertEquals("/srv/ws-root/agents/code-review",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/srv/ws-root/agents/code-review", "/srv/ws-root"));
|
||||
// Identical to workspace root → trivially allowed.
|
||||
assertEquals("/srv/ws-root",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/srv/ws-root", "/srv/ws-root"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Absolute override outside workspace root → rejected (workspace-scoping bypass)")
|
||||
@DisabledOnOs(OS.WINDOWS)
|
||||
void absoluteOverride_outsideWorkspace_rejected() {
|
||||
// A less-trusted admin could set / or another repo and bypass scoping —
|
||||
// reject it so the override stays inside the team workspace.
|
||||
org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException.class, () ->
|
||||
AgentGraphBuilder.resolveAgentBasePath("/etc", "/srv/ws-root"));
|
||||
org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException.class, () ->
|
||||
AgentGraphBuilder.resolveAgentBasePath("/", "/srv/ws-root"));
|
||||
org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException.class, () ->
|
||||
AgentGraphBuilder.resolveAgentBasePath("/Users/admin/other-project",
|
||||
"/srv/ws-root"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Absolute override with no workspace basePath → used verbatim (legacy)")
|
||||
@DisabledOnOs(OS.WINDOWS)
|
||||
void absoluteOverride_noWorkspace_allowed() {
|
||||
// No workspace boundary to enforce — fall back to legacy behavior.
|
||||
assertEquals("/Users/admin/scratch",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/Users/admin/scratch", null));
|
||||
assertEquals("/Users/admin/scratch",
|
||||
AgentGraphBuilder.resolveAgentBasePath("/Users/admin/scratch", ""));
|
||||
}
|
||||
}
|
||||
|
||||
@ -205,6 +205,65 @@ class WorkspacePathGuardShellTest {
|
||||
WorkspacePathGuard.validateShellCommand("read line < /dev/fd/3"));
|
||||
}
|
||||
|
||||
// ==================== Relative parent-directory traversal ====================
|
||||
|
||||
@Test
|
||||
@DisplayName("Bare `cd ..` → rejected")
|
||||
void cdDotDot_blocked() {
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("cd .. && ls"));
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("cd .."));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Relative parent traversal `../foo` → rejected")
|
||||
void relativeParentTraversal_blocked() {
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("cat ../mateclaw/CLAUDE.md"));
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("head -3 ../README.md"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Symlink creation with relative outside-pointing target → rejected")
|
||||
void relativeSymlinkEscape_blocked() {
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("ln -sf ../mateclaw breakout"));
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("ln -s ../../etc shortcut"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Deeper relative traversal `foo/../../bar` → rejected")
|
||||
void deepRelativeTraversal_blocked() {
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("cat subdir/../../other/file.txt"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("In-workspace `..` traversal that normalizes back inside → allowed")
|
||||
void inWorkspaceTraversal_pass() {
|
||||
// subdir/../sibling → workspace/sibling, still inside.
|
||||
assertDoesNotThrow(() ->
|
||||
WorkspacePathGuard.validateShellCommand("cat subdir/../sibling.txt"));
|
||||
// ./.. is at workspace root after normalize — still inside? No: ./.. is parent of cwd.
|
||||
// We do want to reject that, which the regex will catch as escape.
|
||||
assertDoesNotThrow(() ->
|
||||
WorkspacePathGuard.validateShellCommand("ls foo/..")); // resolves to workspace root
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Identifier with double-dot but no slash (e.g. `abc..xyz`) is not a path → allowed")
|
||||
void doubleDotInIdentifier_pass() {
|
||||
// "..foo" / "abc..xyz" should not be confused with parent traversal.
|
||||
// These appear in version strings, env var values, etc.
|
||||
assertDoesNotThrow(() ->
|
||||
WorkspacePathGuard.validateShellCommand("echo version=1.2..3"));
|
||||
}
|
||||
|
||||
// ==================== Device-node negative cases ====================
|
||||
|
||||
@Test
|
||||
@DisplayName("Non-allowlisted /dev/* paths still rejected")
|
||||
void devOther_blocked() {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user