Prevent file picker from importing BookPlayer's own Documents folder #1443
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a safeguard to prevent users from accidentally importing BookPlayer's own Documents folder through the file picker, which would cause circular references and corrupt the library. The implementation includes a new validation method in DataManager and integrates it into the file import flow.
Changes:
- Added
isAppOwnFoldermethod to detect if a URL points to the app's Documents folder or its subfolders - Integrated the check into the file picker selection handler to silently reject app folder imports
- Refactored ImportManager to use constant for inbox folder name instead of hardcoded string
- Added server error (5xx) handling to NetworkClient
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Shared/CoreData/DataManager.swift | Added isAppOwnFolder method to check if a URL is within the app's Documents folder |
| BookPlayer/Library/ItemList/ItemListViewModel.swift | Added check to prevent importing app's own folder in file picker handler |
| BookPlayer/Import/ImportManager.swift | Replaced hardcoded "Inbox" string with DataManager.inboxFolderName constant |
| Shared/Network/NetworkClient.swift | Added handling for 500-599 HTTP status codes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if DataManager.isAppOwnFolder(url) { | ||
| return | ||
| } |
There was a problem hiding this comment.
When the app's own folder is detected and rejected, there is no user feedback. The function silently returns without notifying the user why their selection was ignored. Consider providing user feedback (e.g., through loadingState.error) to inform the user that the selected folder cannot be imported because it would cause circular references.
|
|
||
| let destinationURL = documentsFolder.appendingPathComponent(url.lastPathComponent) | ||
| if !FileManager.default.fileExists(atPath: destinationURL.path) { | ||
| try! FileManager.default.copyItem(at: url, to: destinationURL) |
There was a problem hiding this comment.
Using force try (try!) when copying items can crash the app if the copy operation fails for any reason (e.g., insufficient disk space, permissions issues, file already exists and is locked). Consider using proper error handling with do-catch and reporting errors through loadingState.error, consistent with error handling patterns elsewhere in this file.
| try! FileManager.default.copyItem(at: url, to: destinationURL) | |
| do { | |
| try FileManager.default.copyItem(at: url, to: destinationURL) | |
| } catch { | |
| loadingState.error = error | |
| } |
| public class func isAppOwnFolder(_ url: URL) -> Bool { | ||
| let resolvedURL = url.resolvingSymlinksInPath() | ||
| let documentsPath = getDocumentsFolderURL().resolvingSymlinksInPath().path | ||
|
|
||
| // Check if the URL is the Documents folder itself or inside it | ||
| if resolvedURL.path == documentsPath || resolvedURL.path.hasPrefix(documentsPath + "/") { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
The new isAppOwnFolder method lacks test coverage. Given that the codebase has test files (e.g., DataManagerTests.swift) and this is a critical security feature to prevent circular references and library corruption, consider adding unit tests to verify that the method correctly identifies the app's Documents folder and its subfolders, as well as correctly allows other folders.
Bugfix
If a user 'Open's the BookPlayer's folder from the file picker in the import process, the app will try to import a copy of its own Documents folder, messing up the entire library, as there's a circular reference in there