security(import): reject path-traversal filenames from ODS in MassImportService.processRows #530

New Issue

marcel · 2026-05-11T20:13:41+02:00

marcel commented

2026-05-11 20:13:41 +02:00

Type: Security hardening (defense-in-depth)
Priority: P2-medium — current code accidentally safe; codify the constraint so a future refactor doesn't silently break it
Source: review of #526 by Nora "NullX" Steiner — comment #8648, finding 6
Parent PR: #526 (mass-import bind mount → first prod-reachable use)

Summary

Reject ODS-supplied filenames that contain path separators, .. segments, or absolute paths before findFileRecursive runs. Today the impact is bounded by an implementation detail; this issue makes the constraint explicit and tested.

Context

The ODS spreadsheet contains filenames in cells. MassImportService.processRows (MassImportService.java:121) maps those cells to files via findFileRecursive(filename) which walks /import and matches on p.getFileName().toString().equals(filename). Currently no validation that the spreadsheet-supplied filename is a plain name without separators or traversal segments.

Today the impact is limited because findFileRecursive matches on getFileName().equals(...), so a cell value like ../etc/passwd would not match anything under /import (no file with that exact basename exists). But if anyone later refactors findFileRecursive to use a path-join (which is the natural-looking thing to do), the protection silently disappears.

With #526 making this prod-reachable, codifying the constraint now prevents that refactor from being a silent vulnerability.

Required

Add a validateImportFilename(String) guard at the top of processRows (or wherever the cell value is first read).
Reject if any of:
- null or blank
- contains / or \ (no path components)
- contains .. segments
- is . or ..
- is an absolute path (Paths.get(name).isAbsolute())
On reject, increment the same skipped counter introduced by #529 and log with sanitized SLF4J pattern.
Permanent regression tests for each rejection case + happy path.

Acceptance criteria

All rejection cases above have failing-then-passing tests
Guard runs before findFileRecursive, not inside it (cheaper, more explicit)
Status message lists rejected cell values
Shares the skipped counter with #529 — no duplicate field

Linked NFRs

Security: Filenames sourced from spreadsheet input MUST be plain basenames with no path components.
Maintainability: Validation MUST live at the input boundary, not inside lookup helpers, so future refactors can't silently bypass it.

Dependencies

Shares the ImportStatus.skipped field with #529 — pick whichever lands first; the second reuses it.

Definition of Ready

Validation rules enumerated
Insertion point identified (processRows, before findFileRecursive)
Dependency on #529 noted
Acceptance criteria testable

🤖 Generated with Claude Code during /implement on #526

**Type:** Security hardening (defense-in-depth) **Priority:** P2-medium — current code accidentally safe; codify the constraint so a future refactor doesn't silently break it **Source:** review of #526 by Nora "NullX" Steiner — comment [#8648](https://git.raddatz.cloud/marcel/familienarchiv/pulls/526#issuecomment-8648), finding 6 **Parent PR:** #526 (mass-import bind mount → first prod-reachable use) ## Summary Reject ODS-supplied filenames that contain path separators, `..` segments, or absolute paths *before* `findFileRecursive` runs. Today the impact is bounded by an implementation detail; this issue makes the constraint explicit and tested. ## Context The ODS spreadsheet contains filenames in cells. `MassImportService.processRows` (`MassImportService.java:121`) maps those cells to files via `findFileRecursive(filename)` which walks `/import` and matches on `p.getFileName().toString().equals(filename)`. Currently no validation that the spreadsheet-supplied filename is a plain name without separators or traversal segments. Today the impact is *limited* because `findFileRecursive` matches on `getFileName().equals(...)`, so a cell value like `../etc/passwd` would not match anything under `/import` (no file with that exact basename exists). **But** if anyone later refactors `findFileRecursive` to use a path-join (which is the natural-looking thing to do), the protection silently disappears. With #526 making this prod-reachable, codifying the constraint now prevents that refactor from being a silent vulnerability. ## Required 1. Add a `validateImportFilename(String)` guard at the top of `processRows` (or wherever the cell value is first read). 2. Reject if any of: - `null` or blank - contains `/` or `\` (no path components) - contains `..` segments - is `.` or `..` - is an absolute path (`Paths.get(name).isAbsolute()`) 3. On reject, increment the same `skipped` counter introduced by #529 and log with sanitized SLF4J pattern. 4. Permanent regression tests for each rejection case + happy path. ## Acceptance criteria - [ ] All rejection cases above have failing-then-passing tests - [ ] Guard runs *before* `findFileRecursive`, not inside it (cheaper, more explicit) - [ ] Status message lists rejected cell values - [ ] Shares the `skipped` counter with #529 — no duplicate field ## Linked NFRs - **Security:** Filenames sourced from spreadsheet input MUST be plain basenames with no path components. - **Maintainability:** Validation MUST live at the input boundary, not inside lookup helpers, so future refactors can't silently bypass it. ## Dependencies - Shares the `ImportStatus.skipped` field with #529 — pick whichever lands first; the second reuses it. ## Definition of Ready - [x] Validation rules enumerated - [x] Insertion point identified (`processRows`, before `findFileRecursive`) - [x] Dependency on #529 noted - [x] Acceptance criteria testable 🤖 Generated with [Claude Code](https://claude.com/claude-code) during /implement on #526

marcel referenced this issue

2026-05-11 20:14:42 +02:00

feat(infra): bind-mount /import for backend mass-import endpoint #526

marcel referenced this issue

2026-05-11 20:36:23 +02:00

ui(admin/system): improve mass-import status card (loading state, i18n, font size) #533

marcel added the P2-medium security labels 2026-05-11 20:36:41 +02:00

Sign in to join this conversation.