fix: prefix match for root check to empty folders in file driver - #1302
Conversation
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens FileBackend’s empty-directory cleanup logic to avoid accidental deletions when a target path shares a prefix with the storage root (e.g., /tmp/root vs /tmp/root-sibling). It replaces a string prefix check with a cross-platform “is dirPath inside filePath?” check using path.relative, and adds a regression test to cover the sibling-directory case.
Changes:
- Replace
startsWith(storageRoot)directory-boundary logic with apath.relative-based containment check. - Prevent cleanup of the storage root itself and any directory outside the storage root.
- Add a test ensuring sibling directories sharing a storage-root prefix are not removed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/storage/backend/file.ts | Uses path.relative() + ../absolute checks to ensure cleanup never escapes the storage root and never deletes the root itself. |
| src/storage/backend/file.test.ts | Adds regression coverage for the sibling-prefix scenario and cleans up the created sibling directory in teardown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — this is a small, well-tested defense-in-depth fix.
Extended reasoning...
Overview
The PR changes cleanupEmptyDirectories in src/storage/backend/file.ts to replace a string-prefix check (dirPath.startsWith(this.filePath)) with a proper path.relative-based containment check, so that a sibling directory sharing the storage root as a string prefix (e.g. /data/storage vs /data/storage-sibling) is no longer misidentified as being inside the storage root. A new test case explicitly verifies the sibling-directory scenario is not cleaned up.
Security risks
As the PR description notes, this path is already gated by resolveSecureFilesystemPath for all real callers (deleteObject, deleteObjects, abortMultipartUpload), so dirPath passed into cleanupEmptyDirectories is always already confined to the storage root in current usage. This change is purely defense-in-depth for subclasses/future callers that might invoke it with untrusted paths directly. No new attack surface is introduced; the new logic is strictly more conservative than the old one (it can only refuse to clean up cases the old code would have accepted, not the reverse) — I traced through the three call sites and confirmed all pass in an already-secure path derived from resolveSecurePath.
Level of scrutiny
This warrants light scrutiny: it's a small, self-contained change to non-critical cleanup logic (best-effort empty-directory removal, wrapped in try/catch and ignored on error), not to auth, data access, or request handling. The fix itself is a standard, idiomatic Node.js pattern for safe subpath containment checks (relative-path based rather than string-prefix based), and correctly handles the root-equals case, the ..-prefix case, and the cross-platform absolute-path case (relevant for Windows drive-letter edge cases).
Other factors
Test coverage is good — the new test directly exercises the bug being fixed (sibling directory sharing a prefix) and the existing tests for normal cleanup and root-boundary behavior still pass conceptually with the new logic. No CODEOWNER-restricted paths, no config changes, no external interfaces affected.
Coverage Report for CI Build 31019005254Coverage increased (+0.005%) to 80.653%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Empty check does prefix matching and it can match sibling directory.
It is guarded by resolve secure path so not exploitable as it is but error-prone if subclassed.
What is the new behavior?
Do cross platform relative check from the root.
This is just defense in depth.