mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
fix(tool-guard): harden filesystem-root skip and chat-upload fallback in boundary checks
- Shell scan: a token normalizing to the filesystem root (//, /., /..) is only skipped when the command carries no destructive verb; with rm/rmdir/shred/srm present the scan fails closed, so 'rm -rf //' is refused while sed empty replacements (s/pattern//) stay allowed. - Chat-upload fallback: a boundary violation is waived only when the requested path itself normalizes inside one of the conversation's candidate upload directories; a basename match against a stored attachment no longer clears the violation. Resolver/DB failures keep the BLOCK finding. The unused candidate-roots resolve overload is removed. - Regression tests for destructive root tokens, sed allowance, the fail-closed compound case, stored-upload-path allowance, basename collisions, cross-conversation paths, and resolver failure.
This commit is contained in:
matevip
parent
bc9768b717
commit
4ae4731d54
@ -87,43 +87,6 @@ public final class ChatUploadResolver {
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve a raw path against a pre-computed set of candidate upload roots
|
||||
* (from {@link ChatUploadLocationResolver#resolveCandidateUploadRoots(String)}).
|
||||
* Each root is the conversation-scoped upload directory
|
||||
* ({@code {root}/{conversationId}/}); this overload avoids the
|
||||
* {@code workspaceBasePath} heuristic so the guardian can use DB-resolved
|
||||
* candidate roots even when the thread-local context carries {@code null}.
|
||||
*
|
||||
* @param rawPath user-supplied path (basename or relative)
|
||||
* @param conversationId business conversation id
|
||||
* @param candidateUploadRoots pre-resolved candidate upload roots,
|
||||
* each being an upload root <em>without</em>
|
||||
* the conversation-id subdirectory appended
|
||||
* @return absolute path of the matched attachment, or {@code null}
|
||||
*/
|
||||
public static Path resolve(String rawPath, String conversationId,
|
||||
List<Path> candidateUploadRoots) {
|
||||
if (rawPath == null || rawPath.isBlank()) {
|
||||
return null;
|
||||
}
|
||||
if (conversationId == null || conversationId.isBlank()) {
|
||||
return null;
|
||||
}
|
||||
if (candidateUploadRoots == null || candidateUploadRoots.isEmpty()) {
|
||||
return null;
|
||||
}
|
||||
for (Path uploadRoot : candidateUploadRoots) {
|
||||
Path uploadDir = uploadRoot.resolve(conversationId)
|
||||
.toAbsolutePath().normalize();
|
||||
Path matched = resolveIn(rawPath, uploadDir);
|
||||
if (matched != null) {
|
||||
return matched;
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Ordered candidate upload directories for a conversation: the
|
||||
* workspace-scoped dir first (when a base path is active), then the default
|
||||
|
||||
@ -432,10 +432,17 @@ public final class WorkspacePathGuard {
|
||||
continue;
|
||||
}
|
||||
if (isFilesystemRoot(normalized)) {
|
||||
// A normalized filesystem root `/` (or `//`) is almost always a
|
||||
// false positive from shell syntax — sed's s/pattern//, awk's
|
||||
// empty field, etc. Real commands never target the filesystem
|
||||
// root as a cat/ls/write operand.
|
||||
// A token normalizing to the filesystem root is usually shell
|
||||
// syntax misread as a path — sed's s/pattern//, awk's empty
|
||||
// field, etc. — so it is skipped for non-destructive commands.
|
||||
// When the command carries a destructive verb, fail closed:
|
||||
// `rm -rf //` (or `/.`, `/..`) targets the filesystem root and
|
||||
// must be refused. The verb flag is command-wide, so a compound
|
||||
// command mixing e.g. `rm` with a sed empty replacement is also
|
||||
// refused — the error tells the caller to split the command.
|
||||
if (destructive) {
|
||||
throw filesystemRootDeletionError();
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (destructive && normalized.equals(root)) {
|
||||
@ -564,9 +571,10 @@ public final class WorkspacePathGuard {
|
||||
|
||||
/**
|
||||
* True when {@code normalized} is the filesystem root ({@code /} or
|
||||
* {@code //}). These are almost always false positives from shell syntax
|
||||
* (sed's {@code s/pattern//}, awk's empty field, etc.) — real commands
|
||||
* never target the filesystem root as a cat/ls/write operand.
|
||||
* {@code //}). In non-destructive commands these are almost always false
|
||||
* positives from shell syntax (sed's {@code s/pattern//}, awk's empty
|
||||
* field, etc.) and are skipped; destructive commands are refused at the
|
||||
* call site because {@code rm -rf //} really does target the root.
|
||||
*/
|
||||
private static boolean isFilesystemRoot(Path normalized) {
|
||||
String s = normalized.toString();
|
||||
@ -583,6 +591,14 @@ public final class WorkspacePathGuard {
|
||||
+ ". Deleting the workspace root is refused — target a path inside it instead.");
|
||||
}
|
||||
|
||||
private static IllegalArgumentException filesystemRootDeletionError() {
|
||||
return new IllegalArgumentException(
|
||||
"Shell command combines a destructive verb (rm/rmdir/shred/srm) with a path that "
|
||||
+ "normalizes to the filesystem root (/), which is refused. If the root-like "
|
||||
+ "token comes from shell syntax (e.g. an empty sed replacement) rather than a "
|
||||
+ "delete target, run the delete and the text edit as separate commands.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the active workspace base path. Order of preference:
|
||||
* <ol>
|
||||
|
||||
@ -5,12 +5,12 @@ import com.fasterxml.jackson.databind.ObjectMapper;
|
||||
import lombok.extern.slf4j.Slf4j;
|
||||
import org.springframework.context.annotation.Lazy;
|
||||
import org.springframework.stereotype.Component;
|
||||
import vip.mate.tool.builtin.ChatUploadResolver;
|
||||
import vip.mate.tool.guard.WorkspacePathGuard;
|
||||
import vip.mate.tool.guard.model.*;
|
||||
import vip.mate.workspace.core.service.ChatUploadLocationResolver;
|
||||
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.Paths;
|
||||
import java.util.List;
|
||||
import java.util.Locale;
|
||||
import java.util.Map;
|
||||
@ -141,22 +141,20 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian {
|
||||
String path = extractJsonParam(rawArgs, paramName);
|
||||
String violation = WorkspacePathGuard.findPathBoundaryViolation(path, basePath);
|
||||
if (violation != null) {
|
||||
// Chat-upload fallback: a path like "chat-uploads/{convId}/..."
|
||||
// may sit outside the workspace root yet still be a legitimate
|
||||
// user attachment. Resolve candidate upload roots from the DB
|
||||
// (workspace-scoped + default) to find the file — this avoids
|
||||
// the workspaceBasePath heuristic that can be null when the
|
||||
// agent isn't configured with a workspace override.
|
||||
// Chat-upload fallback: an attachment path may sit outside the
|
||||
// workspace root yet still be a legitimate user upload. Resolve
|
||||
// candidate upload roots from the DB (workspace-scoped +
|
||||
// default) — this avoids the workspaceBasePath heuristic that
|
||||
// can be null when the agent isn't configured with a workspace
|
||||
// override. Any resolver failure keeps the BLOCK finding.
|
||||
String conversationId = context.conversationId();
|
||||
if (conversationId != null && !conversationId.isBlank()) {
|
||||
try {
|
||||
List<Path> candidateRoots = chatUploadLocationResolver
|
||||
.resolveCandidateUploadRoots(conversationId);
|
||||
Path resolved = ChatUploadResolver.resolve(
|
||||
path, conversationId, candidateRoots);
|
||||
if (resolved != null) {
|
||||
log.debug("[WorkspaceBoundaryGuardian] Path {} resolved to chat-upload: {}",
|
||||
path, resolved);
|
||||
if (isInsideUploadDir(path, conversationId, candidateRoots)) {
|
||||
log.debug("[WorkspaceBoundaryGuardian] Path {} is inside a chat-upload dir "
|
||||
+ "of conversation {}", path, conversationId);
|
||||
return List.of();
|
||||
}
|
||||
} catch (Exception e) {
|
||||
@ -170,6 +168,40 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian {
|
||||
return List.of();
|
||||
}
|
||||
|
||||
/**
|
||||
* True when {@code rawPath} itself normalizes to a location inside one of
|
||||
* the conversation's candidate upload directories
|
||||
* ({@code {root}/{conversationId}/}).
|
||||
* <p>
|
||||
* The check is a strict prefix match on the <em>requested</em> path — a
|
||||
* basename match against stored attachments is deliberately not enough to
|
||||
* clear a boundary violation, because an arbitrary outside path could share
|
||||
* a basename with an uploaded file and would then slip past the guard
|
||||
* whenever it exists under some other trusted root. Tool-level resolvers
|
||||
* may still redirect a basename-only request to the stored attachment; the
|
||||
* redirected path they read lands inside the upload directory and passes
|
||||
* this same check.
|
||||
*/
|
||||
private static boolean isInsideUploadDir(String rawPath, String conversationId,
|
||||
List<Path> candidateRoots) {
|
||||
if (rawPath == null || rawPath.isBlank() || candidateRoots == null) {
|
||||
return false;
|
||||
}
|
||||
Path normalized;
|
||||
try {
|
||||
normalized = Paths.get(rawPath).toAbsolutePath().normalize();
|
||||
} catch (Exception e) {
|
||||
return false;
|
||||
}
|
||||
for (Path root : candidateRoots) {
|
||||
Path uploadDir = root.resolve(conversationId).toAbsolutePath().normalize();
|
||||
if (normalized.startsWith(uploadDir)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
private GuardFinding boundaryFinding(String toolName, String paramName, String matchValue, String reason) {
|
||||
return new GuardFinding(
|
||||
"WORKSPACE_BOUNDARY_ESCAPE",
|
||||
|
||||
@ -311,6 +311,41 @@ class WorkspacePathGuardShellTest {
|
||||
WorkspacePathGuard.validateShellCommand("echo evil > /tmp/leak.txt"));
|
||||
}
|
||||
|
||||
// ==================== Filesystem-root tokens ====================
|
||||
|
||||
@Test
|
||||
@DisplayName("Destructive command targeting the filesystem root (`rm -rf //`, `/.`, `/..`) → rejected")
|
||||
void destructiveFilesystemRoot_blocked() {
|
||||
// "//", "/." and "/.." all normalize to "/" — a delete aimed there must
|
||||
// not be skipped by the root-token false-positive allowance.
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("rm -rf //"));
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("rm -rf /."));
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand("rm -rf /.."));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("sed empty replacement (s/pattern//) is not misread as a filesystem-root path")
|
||||
void sedEmptyReplacement_pass() {
|
||||
// The trailing "// inside the sed script produces a token that
|
||||
// normalizes to "/" — allowed because the command is non-destructive.
|
||||
assertDoesNotThrow(() ->
|
||||
WorkspacePathGuard.validateShellCommand("sed 's/\"text\": \"//' data.json"));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Compound command mixing a destructive verb with a root-normalizing token fails closed")
|
||||
void destructiveMixedWithRootToken_blocked() {
|
||||
// The destructive flag is command-wide by design: a compound command
|
||||
// that both deletes and carries a "/"-normalizing token is refused,
|
||||
// trading a rare false positive for never skipping `rm ... //`.
|
||||
assertThrows(IllegalArgumentException.class, () ->
|
||||
WorkspacePathGuard.validateShellCommand(
|
||||
"rm -f old.log && sed 's/\"x\": \"//' data.json"));
|
||||
}
|
||||
|
||||
// ==================== Device-node negative cases ====================
|
||||
|
||||
@Test
|
||||
|
||||
@ -5,17 +5,23 @@ 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 org.junit.jupiter.api.io.TempDir;
|
||||
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 vip.mate.workspace.core.service.ChatUploadLocationResolver;
|
||||
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
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;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.when;
|
||||
|
||||
/**
|
||||
* Verifies that {@link WorkspaceBoundaryGuardian} turns a workspace-boundary
|
||||
@ -47,6 +53,12 @@ class WorkspaceBoundaryGuardianTest {
|
||||
.withWorkspaceBasePath(basePath);
|
||||
}
|
||||
|
||||
private ToolInvocationContext read(String path, String basePath) {
|
||||
String args = "{\"filePath\":\"" + path + "\"}";
|
||||
return ToolInvocationContext.of("read_file", args, "conv", "agent")
|
||||
.withWorkspaceBasePath(basePath);
|
||||
}
|
||||
|
||||
private ToolInvocationContext code(String language, String src, String basePath) {
|
||||
String args = "{\"language\":\"" + language + "\",\"code\":\""
|
||||
+ src.replace("\"", "\\\"") + "\"}";
|
||||
@ -194,6 +206,68 @@ class WorkspaceBoundaryGuardianTest {
|
||||
ToolInvocationContext.of("web_search", "{}", "conv", "agent")));
|
||||
}
|
||||
|
||||
// ==================== Chat-upload fallback scope ====================
|
||||
|
||||
@Test
|
||||
@DisplayName("A stored chat-upload path outside the workspace is allowed via the DB-backed fallback")
|
||||
void chatUpload_storedPath_pass(@TempDir Path uploadRoot) throws Exception {
|
||||
Path stored = Files.createDirectories(uploadRoot.resolve("conv"))
|
||||
.resolve("1777391026594_secret.txt");
|
||||
Files.writeString(stored, "attachment");
|
||||
|
||||
ChatUploadLocationResolver resolver = mock(ChatUploadLocationResolver.class);
|
||||
when(resolver.resolveCandidateUploadRoots("conv")).thenReturn(List.of(uploadRoot));
|
||||
WorkspaceBoundaryGuardian g = new WorkspaceBoundaryGuardian(resolver);
|
||||
|
||||
// The upload root sits outside the workspace, so the boundary check
|
||||
// trips first; the fallback must clear it for the real stored path.
|
||||
assertTrue(g.evaluate(read(stored.toString(), WORKSPACE)).isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("An outside path merely sharing a basename with an attachment stays blocked")
|
||||
void chatUpload_basenameCollision_blocked(@TempDir Path uploadRoot, @TempDir Path elsewhere)
|
||||
throws Exception {
|
||||
// Store an attachment whose "{millis}_{safeName}" name would basename-match
|
||||
// a request for "secret.txt" — the fallback must not let that clear a
|
||||
// violation for a path pointing somewhere else entirely.
|
||||
Path uploadDir = Files.createDirectories(uploadRoot.resolve("conv"));
|
||||
Files.writeString(uploadDir.resolve("1777391026594_secret.txt"), "attachment");
|
||||
Path outside = elsewhere.resolve("secret.txt");
|
||||
Files.writeString(outside, "not an attachment");
|
||||
|
||||
ChatUploadLocationResolver resolver = mock(ChatUploadLocationResolver.class);
|
||||
when(resolver.resolveCandidateUploadRoots("conv")).thenReturn(List.of(uploadRoot));
|
||||
WorkspaceBoundaryGuardian g = new WorkspaceBoundaryGuardian(resolver);
|
||||
|
||||
assertBlocked(g.evaluate(read(outside.toString(), WORKSPACE)));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("A path inside a different conversation's upload dir stays blocked")
|
||||
void chatUpload_otherConversation_blocked(@TempDir Path uploadRoot) throws Exception {
|
||||
Path otherConvFile = Files.createDirectories(uploadRoot.resolve("other-conv"))
|
||||
.resolve("1777391026594_secret.txt");
|
||||
Files.writeString(otherConvFile, "someone else's attachment");
|
||||
|
||||
ChatUploadLocationResolver resolver = mock(ChatUploadLocationResolver.class);
|
||||
when(resolver.resolveCandidateUploadRoots("conv")).thenReturn(List.of(uploadRoot));
|
||||
WorkspaceBoundaryGuardian g = new WorkspaceBoundaryGuardian(resolver);
|
||||
|
||||
assertBlocked(g.evaluate(read(otherConvFile.toString(), WORKSPACE)));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Resolver failure keeps the BLOCK finding (fail closed)")
|
||||
void chatUpload_resolverFailure_blocked() {
|
||||
ChatUploadLocationResolver resolver = mock(ChatUploadLocationResolver.class);
|
||||
when(resolver.resolveCandidateUploadRoots("conv"))
|
||||
.thenThrow(new RuntimeException("db down"));
|
||||
WorkspaceBoundaryGuardian g = new WorkspaceBoundaryGuardian(resolver);
|
||||
|
||||
assertBlocked(g.evaluate(read("/somewhere/else/file.txt", WORKSPACE)));
|
||||
}
|
||||
|
||||
// ==================== Tool-result spill dir stays reachable (issue #403) ====================
|
||||
|
||||
@Test
|
||||
|
||||
Loading…
Reference in New Issue
Block a user