fix(db): file-deletion

[alperozturk96]

Issue

Child of directory is not gets deleted. getContentProviderClient() is always null and getContentResolver().delete(folderUri, where, whereArgs); not removing the childs just deletes folder itself see where logic

String where = ProviderTableMeta.FILE_ACCOUNT_OWNER + AND + ProviderTableMeta.FILE_PATH + "=?";
String[] whereArgs = new String[]{user.getAccountName(), folder.getRemotePath()};

.

Changes

Replaces content resolver functions with DAO functions.
Applies fail fast princible and adds logs to follow execution flow more clearly.
Adds tests.

[alperozturk96]

/backport to stable-3.36

In general, to fix uncontrolled path usage you either validate the incoming path against a safe base directory or ensure that only application-generated paths (not arbitrary user input) are used. Here, we can keep the existing behavior but add a safety check before deleting a file based on OCFile.getStoragePath(). The key idea is: compute the canonical path of the candidate file and of the application’s configured storage root, and only proceed with deletion if the file path is within (a descendant of) that root. This directly mitigates the risk of a malicious or corrupted storagePath pointing outside the intended storage area.

The best minimal fix, without changing higher-level behavior, is in FileDataStorageManager.removeLocalCopyIfNeeded. Right before constructing new File(localPath) and deleting it, we should:

1. Obtain the current configured storage root from MainApp.getStoragePath().
2. If that storage root is empty/null, we bail out (no deletion).
3. Resolve both the storage root and the target file to canonical paths (getCanonicalFile()).
4. Check fileCanonical.getPath().startsWith(rootCanonical.getPath() + File.separator).
5. If the check fails, log a warning and skip deletion.

This ensures that even if localPath has been tainted (for example, via a compromised database entry or weird storage configuration), we will not delete anything outside the app’s own storage tree. We do not need extra imports since java.io.File is already in use. All changes are confined to the existing method body in FileDataStorageManager and do not alter public APIs or other behavior except for refusing to delete files outside the configured storage root.

---

In general, the problem should be fixed by validating and constraining any user-controllable storage path before it is persisted or used to construct file operation paths. For a “root storage location” setting, the safe approach is to ensure the chosen directory is within a set of allowed base directories (e.g., the app’s internal files dir, app-specific external storage, or other app-managed roots), and that the path is normalized (no .. traversal, no control characters). Storing only validated paths in MainApp.storagePath ensures all downstream uses (like FileStorageUtils.getSavePath and getDefaultSavePathFor) are safe.

The best fix with minimal functional change is:

1. Introduce a reusable validator in FileStorageUtils that:

   • Rejects null/empty inputs.
   • Normalizes the candidate path (canonicalPath).
   • Ensures it is inside one of the allowed roots for the app (for example, the app’s internal files directory and/or an app-specific external files directory).
   • Optionally enforces a maximum length and disallows control characters.
     The method should return a boolean indicating whether the path is acceptable.

2. Use this validator at the point where the path first becomes persistent application state:

   • In SettingsActivity.saveStoragePath(String newStoragePath), check FileStorageUtils.isValidStorageRoot(getApplicationContext(), newStoragePath) (or similar).
   • If invalid, do not persist it and show an error to the user (or silently fall back to the previous/default storage path). This prevents MainApp.setStoragePath from ever seeing an unsafe value.

3. Optionally, add a defensive check in MainApp.setStoragePath(String path) to avoid storing obviously invalid values (e.g., null/empty). However, the core security fix is at the UI/settings layer.

4. No changes are required in FileDataStorageManager.removeLocalFolder itself, because once MainApp.storagePath is guaranteed to be valid, FileStorageUtils.getDefaultSavePathFor and the paths it builds become safe by construction.

Concretely:

• Edit FileStorageUtils.java to add:

  • An import for android.os.Environment is already present; we can also safely use Context which is already imported.
  • A new public static method isValidStorageRoot(Context context, String path) that performs normalization and containment checks using new File(path).getCanonicalPath() and compares it to allowed base directories obtained from context.getFilesDir() and context.getExternalFilesDir(null) (if non-null).

• Edit SettingsActivity.java:

  • In saveStoragePath(String newStoragePath), before assigning to storagePath and calling MainApp.setStoragePath, call FileStorageUtils.isValidStorageRoot(getApplicationContext(), newStoragePath).
  • If validation fails, log and either keep the existing storagePath or fall back to the default (getFilesDir().getAbsolutePath()); do not save the unvalidated path.

These changes keep existing behavior for normal, valid inputs but prevent arbitrary or malicious paths from being used for storage or deletion.

---

In general, to fix “uncontrolled data used in path expression” issues, we must ensure that any path derived from user-controllable input is either (a) validated/normalized against an allowlist of safe locations or (b) constrained to a specific directory tree, rejecting anything that escapes that tree. For this code path, the user controls the storage root path through settings; that root then influences where per-account files are stored and what storagePath the OCFile model holds. Deletion in removeLocalFolder must not operate on files outside the current storage root.

The best minimal fix without changing behaviour is to: (1) validate and normalize the storage root when it is saved (in SettingsActivity.saveStoragePath), and (2) guard removeLocalFolder so that it only deletes files whose storagePath is under the configured storage root. We will add a static helper to FileStorageUtils that encapsulates “safe storage root validation” and reuse it in both places.

Concrete changes:

1. Add validation helper in FileStorageUtils

   • Implement a public static method, e.g. ensureValidStoragePath(String requestedPath), that:
     • Returns the default internal storage path if requestedPath is null/empty.
     • Attempts to canonicalize the requested path (new File(requestedPath).getCanonicalFile()).
     • Canonicalizes the app’s internal files directory root (getAppContext().getFilesDir()).
     • If the requested path is not under some acceptable base (here, at least not under the root path / alone), we still accept it but ensure we store the canonical absolute path (to remove .., symlinks, etc.).
       To satisfy the CodeQL rule more robustly and avoid directory traversal, we check that the canonical requested path is an absolute path and not equal to / and not containing .. segments; otherwise we fall back to default internal storage.
   • This helper only uses standard java.io.File and MainApp.getAppContext(), which are already present, so no new dependencies are needed.

2. Use validation when saving storage path in SettingsActivity

   • In saveStoragePath(String newStoragePath), instead of directly assigning storagePath = newStoragePath, we call FileStorageUtils.ensureValidStoragePath(newStoragePath) and store that safe/normalized result. This ensures that MainApp.setStoragePath and subsequent FileStorageUtils.getSavePath calls operate on a sanitized root path.

3. Restrict deletions in FileDataStorageManager.removeLocalFolder

   • Before deleting per-file paths, we ensure that the ocFile.getStoragePath() is non-null and lies under the current storage root (MainApp.getStoragePath()), using canonical path comparison (startsWith on canonical paths). If the file is outside the storage root, we skip deletion and log a warning instead of deleting it.
   • This prevents accidental deletion of arbitrary files if a storagePath in the DB is inconsistent with the configured storage root, and clearly constrains deletion to the intended data tree.

These changes leave the functional behaviour unchanged for valid, normal configurations, but add guardrails and satisfy the requirement to validate user-influenced paths before using them for file operations.

---

In general, to fix uncontrolled path usage you must validate and constrain any user-provided path before it is stored or used. For a storage root, the usual pattern is: (1) normalize the candidate path, (2) ensure it resides under one of a small set of allowed base directories (e.g., app-internal files directory and possibly app-specific external storage), and (3) reject or ignore values that are empty, invalid, or point outside those bases.

For this concrete case, the most robust and least invasive fix is to validate the storage path when it is (a) received from the Intent result, and (b) persisted in preferences and assigned to MainApp.storagePath. That ensures all downstream uses (including FileStorageUtils.getSavePath and removeLocalFolder(File)) only ever see safe values. We can implement a dedicated validator method in SettingsActivity to check the new storage location against the app’s internal files directory (and possibly other allowed roots if desired). The validator will use java.io.File’s getCanonicalFile to normalize paths and then check that the candidate path starts with the allowed base. If validation fails, the migration will not be started, and we can optionally show a message or just ignore the change. Additionally, we should guard MainApp.setStoragePath against null or obviously invalid strings (e.g., empty) so that a bad value cannot be propagated globally, even if some call path accidentally bypassed saveStoragePath.

Concretely:

• In SettingsActivity, add a private method isValidStoragePath(String candidate) that:
  • Returns false on null/blank.
  • Resolves the candidate to a canonical file.
  • Resolves the app’s internal files directory to a canonical file.
  • Checks candidateCanonical.getPath().startsWith(internalCanonical.getPath()).
• In onActivityResult, when handling ExtendedSettingsActivityDialog.StorageLocation, only construct StorageMigration and call migrate() if isValidStoragePath(newPath) returns true.
• In saveStoragePath, validate the newStoragePath before writing it to preferences and before calling MainApp.setStoragePath; if invalid, log and return without changing the stored path.
• In MainApp.setStoragePath, add a simple null/blank guard to prevent obviously bogus values, without changing any existing semantics for valid strings.

This keeps existing functionality for normal, UI-generated paths (which should already be under the app’s internal directory), while blocking crafted or malicious paths from being used as storage roots.

In general, the problem should be fixed by validating and constraining the storage base path before it is accepted and persisted, and by ensuring that any deletion routines only operate within a known-safe area. The safest approach is to restrict storage locations to a small set of app-controlled roots (for example, app-internal storage and possibly app-private external directories), and reject any path that falls outside these roots. At minimum, the path should be normalized and checked to be an absolute path under one of those roots, without any .. components or other tricks.

For this codebase, the least intrusive and most robust fix is:

1. Introduce a validation helper in FileStorageUtils (which already hosts filesystem-related helpers) that takes a candidate storage path and returns either a normalized, validated version or null if invalid. This helper can:

   • Reject empty/null strings.
   • Normalize the path (using new File(path).getCanonicalFile()).
   • Require it to be absolute.
   • Require it to start with one of the allowed base directories: the app’s internal files directory (context.getFilesDir()) and, if desired, its getExternalFilesDir(null). Because FileStorageUtils is static, it can obtain a Context via MainApp.getAppContext().

2. Use this helper in SettingsActivity.saveStoragePath to validate any new storage path before persisting it and before calling MainApp.setStoragePath. If invalid, log and either:

   • fall back to the current stored path (no change), or
   • fall back to the default internal storage path.
     To avoid changing user-visible behavior too much, the best compromise is to keep the old path if the requested new one is invalid.

3. Ensure that MainApp.storagePath is always non-null and points to something sensible. We can add a small guard in MainApp.getStoragePath() that falls back to the default internal storage directory if storagePath is null or empty, using MainApp.getAppContext().

With these changes, any localFolder passed into removeLocalFolder(File) that is derived from MainApp.getStoragePath() will be anchored under validated directories only, eliminating the possibility of deleting arbitrary, attacker-chosen filesystem locations.

[tobiasKaminsky]

This "only" tests first folder hierarchy.
Please add a real hierarchy / recursive test.

[alperozturk96]

[github-actions]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/16621.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[github-actions]

Codacy

SpotBugs

Category	Base	New
Bad practice	42	42
Correctness	75	75
Dodgy code	253	253
Experimental	1	1
Internationalization	7	7
Malicious code vulnerability	2	2
Multithreaded correctness	34	34
Performance	43	43
Security	18	18
Total	475	475

[github-actions]

blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.