fix(wiki): close symlink TOCTOU and size-bypass in scan; single-read binary hash

This commit is contained in:
matevip 2026-05-31 07:58:17 +08:00
parent aac04cdfa4
commit 6583aa1e42
3 changed files with 108 additions and 9 deletions

View File

@ -148,14 +148,35 @@ public class WikiDirectoryScanService {
skipped++;
continue;
}
String absolutePath = file.toAbsolutePath().normalize().toString();
// From here on operate ONLY on the resolved real path, never the
// original entry. Reading `realFile` (a concrete, fully-resolved
// path) closes the TOCTOU window: swapping the symlink after
// resolution cannot redirect the read. The file name for the
// title still comes from the directory entry the user sees.
// Re-check the size on the resolved target walkFileTree does
// not follow links, so a symlink's attribute size (the link
// length) can slip an oversized target past the visitFile gate.
long realSize;
try {
realSize = Files.size(realFile);
} catch (IOException e) {
errors.add("Failed to stat: " + file.getFileName() + " (" + e.getMessage() + ")");
skipped++;
continue;
}
if (realSize > properties.getMaxScanFileSize()) {
errors.add("Skipped oversized file: " + file.getFileName() + " (" + realSize + " bytes)");
skipped++;
continue;
}
String absolutePath = realFile.toString();
String fileName = file.getFileName().toString();
String ext = getExtension(fileName);
if (TEXT_EXTENSIONS.contains(ext)) {
// Text files: dedup by content hash, so an unchanged file is
// skipped while a modified file (new hash) is re-ingested.
String content = Files.readString(file, StandardCharsets.UTF_8);
String content = Files.readString(realFile, StandardCharsets.UTF_8);
boolean fresh = rawService.ingestTextFileFromScan(kbId, fileName, absolutePath, content);
if (fresh) {
added++;
@ -177,7 +198,7 @@ public class WikiDirectoryScanService {
default -> "text";
};
boolean freshBinary = rawService.ingestBinaryFileFromScan(
kbId, fileName, sourceType, absolutePath, Files.size(file));
kbId, fileName, sourceType, absolutePath, realSize);
if (freshBinary) {
added++;
} else {

View File

@ -142,7 +142,8 @@ public class WikiRawMaterialService {
return false; // unchanged addFile not called, avoids a second read
}
}
addFile(kbId, title, sourceType, absolutePath, fileSize);
// Pass the hash we already computed so addFile does not re-read the file.
addFile(kbId, title, sourceType, null, absolutePath, fileSize, hash);
return true;
}
@ -227,6 +228,19 @@ public class WikiRawMaterialService {
@Transactional
public WikiRawMaterialEntity addFile(Long kbId, String title, String sourceType,
String mimeType, String sourcePath, long fileSize) {
return addFile(kbId, title, sourceType, mimeType, sourcePath, fileSize, null);
}
/**
* As {@link #addFile(Long, String, String, String, String, long)}, but with
* an optional precomputed content hash so a caller that already read the
* file (e.g. the directory scan's change detection) does not pay a second
* full-file read to dedup.
*/
@Transactional
public WikiRawMaterialEntity addFile(Long kbId, String title, String sourceType,
String mimeType, String sourcePath, long fileSize,
String precomputedHash) {
WikiRawMaterialEntity entity = new WikiRawMaterialEntity();
entity.setKbId(kbId);
entity.setTitle(title);
@ -240,11 +254,15 @@ public class WikiRawMaterialService {
// directly the previous `new String(bytes, UTF_8)` round-trip produced unstable
// hashes for binary files (PDF/Office) because invalid UTF-8 sequences become
// replacement characters, collapsing distinct files into the same hash.
try {
byte[] bytes = java.nio.file.Files.readAllBytes(java.nio.file.Paths.get(sourcePath));
entity.setContentHash(computeHashOfBytes(bytes));
} catch (Exception e) {
log.warn("[Wiki] Could not compute file hash for dedup: {}", e.getMessage());
if (precomputedHash != null) {
entity.setContentHash(precomputedHash);
} else {
try {
byte[] bytes = java.nio.file.Files.readAllBytes(java.nio.file.Paths.get(sourcePath));
entity.setContentHash(computeHashOfBytes(bytes));
} catch (Exception e) {
log.warn("[Wiki] Could not compute file hash for dedup: {}", e.getMessage());
}
}
// Dedup: reuse any existing row with the same hash in this KB (any status)

View File

@ -0,0 +1,60 @@
package vip.mate.wiki.service;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* Verifies the scan re-checks the resolved target's size, so an oversized file
* reached through a symlink (whose own attribute size is just the link length)
* cannot slip past the max-scan-file-size gate.
*/
@SpringBootTest(
webEnvironment = SpringBootTest.WebEnvironment.NONE,
properties = {
"spring.flyway.enabled=true",
"spring.flyway.locations=classpath:db/migration/h2",
"mateclaw.feature-flag.refresh-ms=999999",
"mate.wiki.auto-process-on-upload=false",
"mate.wiki.max-scan-file-size=200"
}
)
class WikiScanSizeGuardE2ETest {
@Autowired
private WikiDirectoryScanService scanService;
private static final java.util.concurrent.atomic.AtomicLong SEQ =
new java.util.concurrent.atomic.AtomicLong(System.nanoTime());
@Test
void oversizedTargetReachedViaSymlink_isSkipped(@TempDir Path dir) throws IOException {
// 5000-byte file far exceeds the 200-byte cap; a RELATIVE symlink to it
// has a tiny attribute size (the short link path) that passes the
// visitFile gate, so only the resolved-target re-check can stop it.
Path big = dir.resolve("big.pdf");
Files.write(big, new byte[5000]);
Path link = dir.resolve("link.pdf");
try {
Files.createSymbolicLink(link, big.getFileName()); // relative -> "big.pdf"
} catch (UnsupportedOperationException | IOException e) {
return; // no symlink support skip
}
WikiDirectoryScanService.ScanResult result =
scanService.scanDirectory(SEQ.incrementAndGet(), dir.toString());
// Neither the oversized file nor the symlink to it is ingested.
assertEquals(0, result.added(), "oversized target must not be ingested via a symlink");
assertTrue(result.errors().stream().anyMatch(e -> e.contains("oversized")),
"the resolved-target size check should report the oversized skip");
}
}