refactor(file): improve path handling and validation#82
Open
balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
Open
refactor(file): improve path handling and validation#82balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces systematic path traversal protection and input validation across file operations. New Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java (1)
107-107:⚠️ Potential issue | 🟡 MinorInconsistent path validation:
downloadBookFilelacks containment check.Unlike
downloadBook(lines 61-62) anddownloadKoboBook(lines 261-263) which useFileUtils.requirePathWithinBase,downloadBookFileonly normalizes the path without validating it stays within the library root. For consistency and defense-in-depth, consider adding the same containment check.🛡️ Suggested fix for consistency
+ BookEntity bookEntity = bookFileEntity.getBook(); + Path libraryRoot = Path.of(bookEntity.getLibraryPath().getPath()); - Path file = bookFileEntity.getFullFilePath().toAbsolutePath().normalize(); + Path file = FileUtils.requirePathWithinBase(bookFileEntity.getFullFilePath(), libraryRoot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java` at line 107, The downloadBookFile method currently only normalizes the path returned by bookFileEntity.getFullFilePath() without verifying it remains inside the library root; update downloadBookFile to call FileUtils.requirePathWithinBase (same pattern used in downloadBook and downloadKoboBook) after normalization to enforce containment within the library root and throw/handle the same error if the check fails so path validation is consistent across methods.
🧹 Nitpick comments (3)
booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
34-44: Consider consistent null handling across path utilities.
containsParentTraversalreturnsfalsefornullinput (line 36), whilenormalizeAbsolutePaththrowsIllegalArgumentException. This inconsistency could mask null inputs in call sites that check traversal before normalization.♻️ Optional: throw for null to align with normalizeAbsolutePath
public boolean containsParentTraversal(Path path) { if (path == null) { - return false; + throw new IllegalArgumentException("Path cannot be null"); } for (Path part : path) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java` around lines 34 - 44, containsParentTraversal currently returns false for a null Path but normalizeAbsolutePath throws IllegalArgumentException for null; change containsParentTraversal(Path path) to validate input the same way (throw IllegalArgumentException when path is null) so callers get consistent null handling. Update the method (containsParentTraversal) to check for null at the start and throw IllegalArgumentException with a clear message, ensuring behavior aligns with normalizeAbsolutePath.booklore-api/src/main/java/org/booklore/controller/OpdsController.java (1)
140-146: Inconsistent response types across OPDS endpoints.Only
/catalogand/recentwere updated to returnbyte[]with explicit UTF-8 encoding, while other endpoints (/,/libraries,/shelves,/magic-shelves,/authors,/series,/surprise) still returnString. Since all endpoints declarecharset=utf-8in their content type, consider applying the same pattern consistently, or reverting toStringif the explicit byte conversion isn't strictly necessary.Also applies to: 151-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/controller/OpdsController.java` around lines 140 - 146, The OPDS endpoints are inconsistent: getCatalog (and recent) convert the feed String to byte[] with UTF-8 while other handlers (/, /libraries, /shelves, /magic-shelves, /authors, /series, /surprise) still return String; make them consistent by updating those handler methods to return ResponseEntity<byte[]> and change their bodies to call opdsFeedService.generateXFeed(...), convert the resulting String to byte[] using StandardCharsets.UTF_8, and set the same MediaType.parseMediaType(OPDS_ACQUISITION_MEDIA_TYPE) content type as in getCatalog (or alternatively change getCatalog/recent back to returning ResponseEntity<String> if you prefer Strings); pick one approach and apply it to all methods (refer to getCatalog, opdsFeedService.generateCatalogFeed, and the other endpoint handler method names) so all OPDS endpoints declare and deliver UTF-8 consistently.booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java (1)
352-354: Overly broad exception catch may mask bugs.Catching
RuntimeExceptioncould hide unexpected errors likeNullPointerException. Consider catching onlyInvalidPathExceptionwhich is whatPath.of()throws for malformed paths.♻️ Narrow exception type
+ } catch (java.nio.file.InvalidPathException e) { - } catch (RuntimeException e) { throw ApiError.GENERIC_BAD_REQUEST.createException("Invalid upload target path"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java` around lines 352 - 354, The catch block in FileUploadService that currently catches RuntimeException around the Path.of(...) call should be narrowed to catch java.nio.file.InvalidPathException only; replace "catch (RuntimeException e)" with "catch (InvalidPathException e)" (importing it) and pass the caught exception as the cause when creating the ApiError (e.g., ApiError.GENERIC_BAD_REQUEST.createException("Invalid upload target path", e)) so only malformed-path errors are handled while other runtime errors still surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java`:
- Around line 341-355: The toSafeRelativePath method currently calls
Path.of(relativePath).normalize() before checking
FileUtils.containsParentTraversal, which defeats the traversal detection; update
to check for parent-traversal on the raw parsed Path elements before normalizing
(or inspect the original string for ".."), e.g., obtain Path parsed =
Path.of(relativePath) then verify parsed.isAbsolute() is false and iterate
parsed.getName(i) (or call FileUtils.containsParentTraversal(parsed) if it
inspects raw segments) to detect ".." segments, only then call
parsed.normalize() and return it; keep throwing
ApiError.GENERIC_BAD_REQUEST.createException("Invalid upload target path") on
any detection or exception.
In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java`:
- Around line 70-73: The current check runs containsParentTraversal against
normalizedRelative which will hide parent-traversal like ".."; instead create a
raw Path from relativePath (e.g., rawRelative = Path.of(relativePath)), perform
the security checks against rawRelative (check rawRelative.isAbsolute() ||
containsParentTraversal(rawRelative)) and throw if invalid, then call
normalizedRelative = rawRelative.normalize() for further processing; update the
checks around containsParentTraversal, normalizedRelative, and relativePath in
FileUtils so traversal is detected before normalization.
In `@CHANGELOG.md`:
- Around line 1-11: The top-level release section "## [2.2.2] (2026-03-19)" is
placed above the document title; move that entire release block (the "##
[2.2.2]" heading and its following "### Bug Fixes" and "### Chores" entries) so
it appears below the main "# Changelog" heading instead, ensuring the file
starts with "# Changelog" and then lists releases like "## [2.2.2]" in
chronological order.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java`:
- Line 107: The downloadBookFile method currently only normalizes the path
returned by bookFileEntity.getFullFilePath() without verifying it remains inside
the library root; update downloadBookFile to call
FileUtils.requirePathWithinBase (same pattern used in downloadBook and
downloadKoboBook) after normalization to enforce containment within the library
root and throw/handle the same error if the check fails so path validation is
consistent across methods.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/controller/OpdsController.java`:
- Around line 140-146: The OPDS endpoints are inconsistent: getCatalog (and
recent) convert the feed String to byte[] with UTF-8 while other handlers (/,
/libraries, /shelves, /magic-shelves, /authors, /series, /surprise) still return
String; make them consistent by updating those handler methods to return
ResponseEntity<byte[]> and change their bodies to call
opdsFeedService.generateXFeed(...), convert the resulting String to byte[] using
StandardCharsets.UTF_8, and set the same
MediaType.parseMediaType(OPDS_ACQUISITION_MEDIA_TYPE) content type as in
getCatalog (or alternatively change getCatalog/recent back to returning
ResponseEntity<String> if you prefer Strings); pick one approach and apply it to
all methods (refer to getCatalog, opdsFeedService.generateCatalogFeed, and the
other endpoint handler method names) so all OPDS endpoints declare and deliver
UTF-8 consistently.
In
`@booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java`:
- Around line 352-354: The catch block in FileUploadService that currently
catches RuntimeException around the Path.of(...) call should be narrowed to
catch java.nio.file.InvalidPathException only; replace "catch (RuntimeException
e)" with "catch (InvalidPathException e)" (importing it) and pass the caught
exception as the cause when creating the ApiError (e.g.,
ApiError.GENERIC_BAD_REQUEST.createException("Invalid upload target path", e))
so only malformed-path errors are handled while other runtime errors still
surface.
In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java`:
- Around line 34-44: containsParentTraversal currently returns false for a null
Path but normalizeAbsolutePath throws IllegalArgumentException for null; change
containsParentTraversal(Path path) to validate input the same way (throw
IllegalArgumentException when path is null) so callers get consistent null
handling. Update the method (containsParentTraversal) to check for null at the
start and throw IllegalArgumentException with a clear message, ensuring behavior
aligns with normalizeAbsolutePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6175d295-d43f-4e84-9675-66d0842549ab
📒 Files selected for processing (9)
CHANGELOG.mdbooklore-api/src/main/java/org/booklore/controller/OpdsController.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/file/FileFingerprint.javabooklore-api/src/main/java/org/booklore/service/file/PathService.javabooklore-api/src/main/java/org/booklore/service/migration/migrations/MoveIconsToDataFolderMigration.javabooklore-api/src/main/java/org/booklore/service/opds/OpdsFeedService.javabooklore-api/src/main/java/org/booklore/service/upload/FileUploadService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.java
booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java
Show resolved
Hide resolved
imajes
force-pushed
the
develop
branch
2 times, most recently
from
89113d4 to
37ca101
Compare
# Conflicts: # booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java (1)
124-128: Consider reusingFileUtils.isAudioFileto reduce duplication.This method duplicates
FileUtils.isAudioFile(lines 235-239), andFileUtilsis already imported.♻️ Proposed refactor
- private static boolean isAudioFile(String fileName) { - if (fileName == null) return false; - String lower = fileName.toLowerCase(); - return lower.endsWith(".mp3") || lower.endsWith(".m4a") || lower.endsWith(".m4b") || lower.endsWith(".opus"); - }Then update line 63 to use
FileUtils.isAudioFile(...)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java` around lines 124 - 128, Replace the duplicated audio check in FileFingerprint by delegating to the existing utility: remove or collapse the private static boolean isAudioFile(String fileName) implementation and call FileUtils.isAudioFile(fileName) wherever FileFingerprint.isAudioFile is currently used; update callers (e.g., the place that currently invokes FileFingerprint.isAudioFile) to use FileUtils.isAudioFile(...) directly so the duplicated logic in FileFingerprint is eliminated and the shared FileUtils implementation is reused.booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
86-91: Consider walking up ancestors for real-path validation.When the candidate path doesn't exist, only its immediate parent is checked. If the parent also doesn't exist, validation is skipped. For deeper defense-in-depth against symlink attacks on non-existent intermediate directories, consider walking up the tree to find the first existing ancestor.
♻️ Proposed enhancement
- Path existingAnchor = Files.exists(normalizedCandidate) - ? normalizedCandidate - : normalizedCandidate.getParent(); - if (existingAnchor == null || !Files.exists(existingAnchor)) { - return; - } + Path existingAnchor = normalizedCandidate; + while (existingAnchor != null && !Files.exists(existingAnchor)) { + existingAnchor = existingAnchor.getParent(); + } + if (existingAnchor == null) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java` around lines 86 - 91, The current validation in FileUtils.java only checks the candidate or its immediate parent; update the logic in the method that computes existingAnchor so it walks up the ancestor chain from normalizedCandidate until it finds the first existing ancestor (loop using Path.getParent()), set existingAnchor to that path, and if none found return; ensure you still preserve normalization and use Files.exists(existingAnchor) for the check (adjust the existingAnchor variable and its use accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java`:
- Around line 55-57: Remove the redundant existence check that follows
validateReadableFolderPath: delete the if-block that tests
Files.exists(normalizedFolderPath) || Files.isDirectory(normalizedFolderPath)
and its RuntimeException throw, because validateReadableFolderPath(folderPath)
already validates existence and directory-ness; keep using normalizedFolderPath
afterwards and rely on validateReadableFolderPath and its RuntimeException for
error handling (references: validateReadableFolderPath and
normalizedFolderPath).
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java`:
- Around line 124-128: Replace the duplicated audio check in FileFingerprint by
delegating to the existing utility: remove or collapse the private static
boolean isAudioFile(String fileName) implementation and call
FileUtils.isAudioFile(fileName) wherever FileFingerprint.isAudioFile is
currently used; update callers (e.g., the place that currently invokes
FileFingerprint.isAudioFile) to use FileUtils.isAudioFile(...) directly so the
duplicated logic in FileFingerprint is eliminated and the shared FileUtils
implementation is reused.
In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java`:
- Around line 86-91: The current validation in FileUtils.java only checks the
candidate or its immediate parent; update the logic in the method that computes
existingAnchor so it walks up the ancestor chain from normalizedCandidate until
it finds the first existing ancestor (loop using Path.getParent()), set
existingAnchor to that path, and if none found return; ensure you still preserve
normalization and use Files.exists(existingAnchor) for the check (adjust the
existingAnchor variable and its use accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f454891-4df9-4d64-b644-76ecf6464bd3
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/file/FileFingerprint.javabooklore-api/src/main/java/org/booklore/util/FileUtils.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
Comment on lines
+55
to
57
| if (!Files.exists(normalizedFolderPath) || !Files.isDirectory(normalizedFolderPath)) { | ||
| throw new RuntimeException("Folder does not exist: " + normalizedFolderPath); | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant existence check — this code is unreachable.
validateReadableFolderPath(folderPath) at line 53 already verifies that the folder exists and is a directory (lines 117-118), throwing RuntimeException on failure. This check can never evaluate to true.
🧹 Proposed fix — remove dead code
public static String generateFolderHash(Path folderPath) {
Path normalizedFolderPath = validateReadableFolderPath(folderPath);
try {
- if (!Files.exists(normalizedFolderPath) || !Files.isDirectory(normalizedFolderPath)) {
- throw new RuntimeException("Folder does not exist: " + normalizedFolderPath);
- }
-
List<Path> audioFiles;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@booklore-api/src/main/java/org/booklore/service/file/FileFingerprint.java`
around lines 55 - 57, Remove the redundant existence check that follows
validateReadableFolderPath: delete the if-block that tests
Files.exists(normalizedFolderPath) || Files.isDirectory(normalizedFolderPath)
and its RuntimeException throw, because validateReadableFolderPath(folderPath)
already validates existence and directory-ness; keep using normalizedFolderPath
afterwards and rely on validateReadableFolderPath and its RuntimeException for
error handling (references: validateReadableFolderPath and
normalizedFolderPath).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description
This pull request focuses on improving file path validation and security throughout the codebase, especially to prevent directory traversal attacks and ensure safe file operations. It also includes some bug fixes and minor improvements to the OPDS feed and related services.
These changes collectively harden the application against path traversal vulnerabilities.
Linked Issue: Fixes #
🏷️ Type of Change
🔧 Changes
🧪 Testing (MANDATORY)
Manual testing steps you performed:
Regression testing:
Edge cases covered:
Test output:
Backend test output (
./gradlew test)Frontend test output (
ng test)📸 Screen Recording / Screenshots (MANDATORY)
✅ Pre-Submission Checklist
develop(merge conflicts resolved)🤖 AI-Assisted Contributions
TODOs, or unused scaffolding left behind by AI💬 Additional Context (optional)
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements