fix: reject symlink artifacts and bound collection reads

This commit is contained in:
mateaix 2026-09-13 21:57:34 +08:00
parent 511b689387
commit c024e2d9f0
2 changed files with 72 additions and 6 deletions

View File

@ -4,7 +4,11 @@ import lombok.extern.slf4j.Slf4j;
import org.springframework.ai.chat.model.ToolContext;
import org.springframework.lang.Nullable;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
@ -50,7 +54,7 @@ public final class WorkspaceArtifactSurfacer {
long totalBytes = 0L;
try (Stream<Path> walk = Files.walk(workingDir, SCAN_DEPTH)) {
List<Path> candidates = walk
.filter(Files::isRegularFile)
.filter(p -> Files.isRegularFile(p, LinkOption.NOFOLLOW_LINKS))
.filter(p -> !isNoise(p))
.filter(p -> modifiedSince(p, sinceMillis))
.limit(MAX_SCAN_CANDIDATES)
@ -60,12 +64,18 @@ public final class WorkspaceArtifactSurfacer {
break;
}
try {
long size = Files.size(p);
if (size <= 0 || size > MAX_ARTIFACT_BYTES || totalBytes + size > MAX_TOTAL_ARTIFACT_BYTES) {
BasicFileAttributes attrs = Files.readAttributes(p, BasicFileAttributes.class,
LinkOption.NOFOLLOW_LINKS);
long size = attrs.size();
int budget = (int) Math.min(MAX_ARTIFACT_BYTES, MAX_TOTAL_ARTIFACT_BYTES - totalBytes);
if (!attrs.isRegularFile() || size <= 0 || size > budget) {
continue;
}
byte[] bytes = Files.readAllBytes(p);
totalBytes += size;
byte[] bytes = readArtifact(p, budget);
if (bytes.length == 0) {
continue;
}
totalBytes += bytes.length;
String name = p.getFileName().toString();
String id = cache.put(bytes, name, probeMime(p, name), ctx);
links.add("[" + name + "](" + cache.downloadUrl(id, ctx) + ")");
@ -79,9 +89,28 @@ public final class WorkspaceArtifactSurfacer {
return links;
}
/**
* Bound the actual read, including files that grow after the size check.
* NOFOLLOW_LINKS also rejects a leaf replaced with a symlink after scanning.
* This is best-effort collection, not managed-scope acceptance: ancestor
* replacement, hard links and concurrent writers still need custody fencing.
*/
static byte[] readArtifact(Path path, int budget) throws IOException {
if (budget < 0 || budget > MAX_ARTIFACT_BYTES) {
throw new IllegalArgumentException("Invalid artifact read budget");
}
try (var input = Files.newInputStream(path, StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)) {
byte[] bytes = input.readNBytes(budget + 1);
if (bytes.length > budget) {
throw new IOException("Artifact exceeds read budget");
}
return bytes;
}
}
private static boolean modifiedSince(Path p, long sinceMillis) {
try {
return Files.getLastModifiedTime(p).toMillis() >= sinceMillis;
return Files.getLastModifiedTime(p, LinkOption.NOFOLLOW_LINKS).toMillis() >= sinceMillis;
} catch (Exception e) {
return false;
}

View File

@ -4,6 +4,7 @@ import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.FileTime;
@ -13,6 +14,8 @@ import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
@ -96,6 +99,40 @@ class WorkspaceArtifactSurfacerTest {
assertTrue(links.isEmpty(), "pre-existing files from another run/workspace must not surface: " + links);
}
@Test
@DisplayName("A workspace symlink must not publish an outside file as an artifact")
void excludesSymbolicLinks() throws Exception {
tmp = Files.createTempDirectory("artifacts-");
cacheDir = Files.createTempDirectory("cache-");
Path outside = Files.writeString(cacheDir.resolve("secret.csv"), "private outside content");
Files.createSymbolicLink(tmp.resolve("report.csv"), outside);
Files.createSymbolicLink(tmp.resolve("nested"), cacheDir);
assertTrue(WorkspaceArtifactSurfacer.collect(new GeneratedFileCache(cacheDir), tmp, 0L, null).isEmpty());
}
@Test
void boundsActualReadAndRejectsLinkAtOpen() throws Exception {
tmp = Files.createTempDirectory("artifacts-");
Path artifact = Files.write(tmp.resolve("data.csv"), new byte[]{1, 2, 3, 4});
assertArrayEquals(new byte[]{1, 2, 3, 4}, WorkspaceArtifactSurfacer.readArtifact(artifact, 4));
assertThrows(IOException.class, () -> WorkspaceArtifactSurfacer.readArtifact(artifact, 3));
Files.delete(artifact);
Path target = Files.writeString(tmp.resolve("target.csv"), "secret");
Files.createSymbolicLink(artifact, target);
assertThrows(IOException.class, () -> WorkspaceArtifactSurfacer.readArtifact(artifact, 10));
}
@Test
void preservesRegularNestedArtifacts() throws Exception {
tmp = Files.createTempDirectory("artifacts-");
cacheDir = Files.createTempDirectory("cache-");
Files.writeString(Files.createDirectory(tmp.resolve("results")).resolve("report.csv"), "a,b");
List<String> links = WorkspaceArtifactSurfacer.collect(new GeneratedFileCache(cacheDir), tmp, 0L, null);
assertEquals(1, links.size());
assertTrue(links.getFirst().contains("report.csv"));
}
@Test
@DisplayName("Null / non-existent working dir and null cache are safe no-ops")
void edgeCasesAreSafe() throws Exception {