marcel/familienarchiv
1
0
Fork 0
Code Issues 107 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

security(import): reject path-traversal filenames in MassImportService.processRows #650

Merged
marcel merged 6 commits from feat/issue-530-reject-path-traversal-filenames into main 2026-05-21 10:30:57 +02:00
Conversation 15 Commits 6 Files Changed 2 +175 -19
2 changed files with 175 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

57
backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
View File

@@ -56,9 +56,17 @@ public class MassImportService {
public enum State { IDLE, RUNNING, DONE, FAILED }
public enum SkipReason {
INVALID_FILENAME_PATH_TRAVERSAL,
INVALID_PDF_SIGNATURE,
FILE_READ_ERROR,
ALREADY_EXISTS,
S3_UPLOAD_FAILED
}
public record SkippedFile(
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) String filename,
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) String reason
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) SkipReason reason
) {}
public record ImportStatus(
@@ -291,6 +299,11 @@ public class MassImportService {
if (index.isBlank()) continue;
String filename = index.contains(".") ? index : index + ".pdf";
if (!isValidImportFilename(filename)) {
log.warn("Skipping import row {}: filename rejected — {}", i, filename);
skippedFiles.add(new SkippedFile(filename, SkipReason.INVALID_FILENAME_PATH_TRAVERSAL));
continue;
}
Optional<File> fileOnDisk = findFileRecursive(filename);
if (fileOnDisk.isEmpty()) {
log.warn("Datei nicht gefunden, importiere nur Metadaten: {}", filename);
@@ -300,17 +313,17 @@ public class MassImportService {
try {
if (!isPdfMagicBytes(fileOnDisk.get())) {
log.warn("Überspringe {}: Datei beginnt nicht mit %PDF-Signatur", filename);
skippedFiles.add(new SkippedFile(filename, "INVALID_PDF_SIGNATURE"));
skippedFiles.add(new SkippedFile(filename, SkipReason.INVALID_PDF_SIGNATURE));
continue;
}
} catch (IOException e) {
log.error("Fehler beim Prüfen der Magic-Bytes für {}", filename, e);
skippedFiles.add(new SkippedFile(filename, "FILE_READ_ERROR"));
skippedFiles.add(new SkippedFile(filename, SkipReason.FILE_READ_ERROR));
continue;
}
}
Optional<String> skipReason = importSingleDocument(cells, fileOnDisk, filename, index);
Optional<SkipReason> skipReason = importSingleDocument(cells, fileOnDisk, filename, index);
if (skipReason.isPresent()) {
skippedFiles.add(new SkippedFile(filename, skipReason.get()));
} else {
@@ -320,6 +333,23 @@ public class MassImportService {
return new ProcessResult(processed, skippedFiles);
}
private boolean isValidImportFilename(String filename) {
if (filename == null || filename.isBlank()) return false;
if (filename.contains("/")) return false;
if (filename.contains("\\")) return false;
if (filename.contains("∕")) return false; // U+2215 DIVISION SLASH
if (filename.contains("／")) return false; // U+FF0F FULLWIDTH SOLIDUS
if (filename.contains("⧵")) return false; // U+29F5 REVERSE SOLIDUS OPERATOR
if (filename.contains("..")) return false;
if (filename.equals(".")) return false;
if (filename.contains("\0")) return false;
// Paths.get() is safe here on Linux for all inputs that passed the checks above;
// it may throw InvalidPathException for OS-specific illegal chars on Windows,
// but those are not reachable in production.
if (Paths.get(filename).isAbsolute()) return false;
return true;
}
// package-private: Mockito spy in tests can override to inject IOException
InputStream openFileStream(File file) throws IOException {
return new FileInputStream(file);
@@ -342,11 +372,11 @@ public class MassImportService {
* @return empty Optional on success; an Optional containing the skip reason on failure/skip.
*/
@Transactional
protected Optional<String> importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) {
protected Optional<SkipReason> importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) {
Optional<Document> existing = documentService.findByOriginalFilename(originalFilename);
if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) {
log.info("Dokument {} existiert bereits, überspringe.", originalFilename);
return Optional.of("ALREADY_EXISTS");
return Optional.of(SkipReason.ALREADY_EXISTS);
}
String archiveBox = getCell(cells, colBox);
@@ -382,7 +412,7 @@ public class MassImportService {
status = DocumentStatus.UPLOADED;
} catch (Exception e) {
log.error("S3 Upload Fehler für {}", file.get().getName(), e);
return Optional.of("S3_UPLOAD_FAILED");
return Optional.of(SkipReason.S3_UPLOAD_FAILED);
}
}
@@ -460,11 +490,18 @@ public class MassImportService {
}
private Optional<File> findFileRecursive(String filename) {
try (Stream<Path> walk = Files.walk(Paths.get(importDir))) {
return walk.filter(p -> !Files.isDirectory(p))
File baseDir = new File(importDir);
try (Stream<Path> walk = Files.walk(baseDir.toPath())) {
Optional<Path> match = walk.filter(p -> !Files.isDirectory(p))
.filter(p -> p.getFileName().toString().equals(filename))
.map(Path::toFile)
.findFirst();
if (match.isEmpty()) return Optional.empty();
File candidate = match.get().toFile();
String baseDirCanonical = baseDir.getCanonicalPath();
if (!candidate.getCanonicalPath().startsWith(baseDirCanonical + File.separator)) {
throw DomainException.internal(ErrorCode.INTERNAL_ERROR, "Path escape detected: " + candidate);
}
return Optional.of(candidate);
} catch (IOException e) {
return Optional.empty();
}

137
backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
View File

@@ -154,10 +154,10 @@ class MassImportServiceTest {
.build();
when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing));
Optional<String> result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001");
Optional<MassImportService.SkipReason> result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001");
verify(documentService, never()).save(any());
assertThat(result).isPresent().contains("ALREADY_EXISTS");
assertThat(result).isPresent().contains(MassImportService.SkipReason.ALREADY_EXISTS);
}
// ─── importSingleDocument — already-exists guard fires before file I/O ─────
@@ -179,10 +179,10 @@ class MassImportServiceTest {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(physicalFile, pdfHeader);
Optional<String> result = service.importSingleDocument(
Optional<MassImportService.SkipReason> result = service.importSingleDocument(
minimalCells("present.pdf"), Optional.of(physicalFile.toFile()), "present.pdf", "present");
assertThat(result).isPresent().contains("ALREADY_EXISTS");
assertThat(result).isPresent().contains(MassImportService.SkipReason.ALREADY_EXISTS);
verify(s3Client, never()).putObject(any(PutObjectRequest.class), any(RequestBody.class));
verify(documentService, never()).save(any());
}
@@ -204,7 +204,7 @@ class MassImportServiceTest {
assertThat(service.getStatus().skipped()).isEqualTo(1);
assertThat(service.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::filename, MassImportService.SkippedFile::reason)
.containsExactly(org.assertj.core.groups.Tuple.tuple("upload_fail.pdf", "S3_UPLOAD_FAILED"));
.containsExactly(org.assertj.core.groups.Tuple.tuple("upload_fail.pdf", MassImportService.SkipReason.S3_UPLOAD_FAILED));
}
@Test
@@ -223,7 +223,7 @@ class MassImportServiceTest {
assertThat(service.getStatus().skipped()).isEqualTo(1);
assertThat(service.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::reason)
.containsExactly("ALREADY_EXISTS");
.containsExactly(MassImportService.SkipReason.ALREADY_EXISTS);
}
// ─── importSingleDocument — create new document (metadata only) ───────────
@@ -283,11 +283,11 @@ class MassImportServiceTest {
doThrow(new RuntimeException("S3 error"))
.when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class));
Optional<String> result = service.importSingleDocument(
Optional<MassImportService.SkipReason> result = service.importSingleDocument(
minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail");
verify(documentService, never()).save(any());
assertThat(result).isPresent().contains("S3_UPLOAD_FAILED");
assertThat(result).isPresent().contains(MassImportService.SkipReason.S3_UPLOAD_FAILED);
}
// ─── importSingleDocument — sender handling ───────────────────────────────
@@ -438,6 +438,110 @@ class MassImportServiceTest {
verify(documentService).findByOriginalFilename("doc002.pdf");
}
// ─── isValidImportFilename — security regression — do not remove ─────────
@Test
void isValidImportFilename_returnsFalse_whenFilenameIsNull() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", (String) null);
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameIsBlank() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", " ");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsForwardSlash() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "etc/passwd");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsBackslash() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "..\\etc\\passwd");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsDotDot() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "doc..evil.pdf");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameIsDotDot() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "..");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameIsAbsolutePath() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "/etc/passwd");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsNullByte() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "file\0.pdf");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsTrue_whenFilenameIsPlainBasename() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "document.pdf");
assertThat(result).isTrue();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsUnicodeDivisionSlash() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "foo∕bar.pdf");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsFullwidthSlash() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "foo／bar.pdf");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsFalse_whenFilenameContainsUnicodeReverseSolidus() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "foo⧵bar.pdf");
assertThat(result).isFalse();
}
@Test
void isValidImportFilename_returnsTrue_whenFilenameHasLeadingDot() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", ".hidden.pdf");
assertThat(result).isTrue();
}
@Test
void isValidImportFilename_returnsTrue_whenFilenameHasSpaces() {
boolean result = ReflectionTestUtils.invokeMethod(service, "isValidImportFilename", "Brief an Oma.pdf");
assertThat(result).isTrue();
}
@Test
void processRows_skipsRowAndContinues_whenFilenameIsPathTraversal() {
when(documentService.findByOriginalFilename("legitimate.pdf")).thenReturn(Optional.empty());
when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0));
List<List<String>> rows = List.of(
List.of("header"),
minimalCells("../evil"), // row 1: path traversal — should be skipped
minimalCells("legitimate.pdf") // row 2: valid — should be processed
);
MassImportService.ProcessResult result = ReflectionTestUtils.invokeMethod(service, "processRows", rows);
assertThat(result.processed()).isEqualTo(1);
assertThat(result.skippedFiles())
.extracting(MassImportService.SkippedFile::reason)
.containsExactly(MassImportService.SkipReason.INVALID_FILENAME_PATH_TRAVERSAL);
}
// ─── importSingleDocument — non-blank optional fields ────────────────────
@Test
@@ -651,7 +755,22 @@ class MassImportServiceTest {
assertThat(spyService.getStatus().skipped()).isEqualTo(1);
assertThat(spyService.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::reason)
.containsExactly("FILE_READ_ERROR");
.containsExactly(MassImportService.SkipReason.FILE_READ_ERROR);
}
// ─── findFileRecursive — symlink escape security regression — do not remove ─
@Test
void findFileRecursive_throwsDomainException_whenSymlinkEscapesImportDir(
@TempDir Path importDirPath, @TempDir Path outsideDir) throws Exception {
Path outsideFile = outsideDir.resolve("secret.pdf");
Files.writeString(outsideFile, "sensitive content");
Files.createSymbolicLink(importDirPath.resolve("secret.pdf"), outsideFile);
ReflectionTestUtils.setField(service, "importDir", importDirPath.toString());
assertThatThrownBy(() -> ReflectionTestUtils.invokeMethod(service, "findFileRecursive", "secret.pdf"))
.isInstanceOf(DomainException.class);
}
// ─── readOds — XXE security regression ───────────────────────────────────