Repository navigation
Conversation
File names and paths delivered by the API (or by external apps through SAF / the content resolver) were joined onto local directories with a plain java.io.File, so '..' or absolute segments resolved outside the intended folder when creating, writing or reading local files. Port the toSafeFileName / resolveContainedFileName / resolveContainedPath hardening introduced in kMail (3ddc669) and apply it to every local join of an untrusted name: - File.getOfflineFile: sanitize remote path segments, containment-check the resolved folder against the per-user/per-drive root before mkdirs(), sanitize the file name; return null on escape - File.getPublicShareCache: sanitize the share file name - CloudStorageProvider.createTempFile: sanitize SAF display name - CloudStorageProvider.generateThumbnail: sanitize remote file name - SaveExternalFilesActivity.getOutputFile: sanitize external file name - convertToIOFile: fall back to the id-keyed cache instead of !! when the offline path is rejected
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Root-level offline files resolve incorrectly, and sanitized-name collisions can overwrite distinct files.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Hardens local file-path resolution against traversal from untrusted file names and paths.
Changes:
- Adds filename/path sanitization and containment utilities.
- Applies sanitization across offline files, shared files, thumbnails, and SAF temporary files.
- Adds JUnit 5 security-focused tests.
| File | Description |
|---|---|
FileNameUtils.kt |
Adds sanitization and containment helpers. |
File.kt |
Secures offline and public-share paths. |
CloudStorageProvider.kt |
Sanitizes SAF and thumbnail filenames. |
SaveExternalFilesActivity.kt |
Sanitizes externally supplied filenames. |
FileNameUtilsTest.kt |
Tests traversal, normalization, truncation, and containment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+33
to
+39
| val normalizedName = Normalizer.normalize(this, Normalizer.Form.NFKC) | ||
| .replace(invalidFileNameCharacters, "_") | ||
| .trim() | ||
| .takeUnless { it.isEmpty() || it == "." || it == ".." } | ||
| ?: DEFAULT_FILE_NAME | ||
|
|
||
| return if (normalizedName.utf8Size <= MAX_FILE_NAME_SIZE_BYTES) normalizedName else computeTruncatedFileName(normalizedName) |
Comment on lines
+310
to
+312
| val sanitizedPath = path.toSafeRelativePath() | ||
| val child = if (isFolder()) sanitizedPath else sanitizedPath.substringBeforeLast("/") | ||
| val folder = rootFolder.resolveContainedPath(child) ?: return null |
|
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
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.
Context
Path traversal via unsanitized file names (reported through the bug bounty program). File names and paths delivered by the API — or by external apps through SAF / the content resolver — were joined onto local directories with a plain
java.io.File, so..or absolute segments resolved outside the intended folder when the app created the folder, created the file, wrote downloaded content, or read the file back.This mirrors the hardening already shipped in kMail Android (commit
3ddc669,FileNameUtils.kt), adapted to kDrive.Vulnerable join sites fixed
File.getOfflineFilepath+nametoSafeRelativePath), canonical containment check against the per-user/per-drive root beforemkdirs(),toSafeFileName()for the final component; returnsnullon escape (function was already nullable)File.getPublicShareCachenametoSafeFileName()— output is a single safe component (no separators, never./..), so the write stays insidefilesDir/public_shareFile.convertToIOFilegetOfflineFile(...)!!with a graceful fallback to the id-keyedgetCacheFileso a rejected traversal degrades instead of crashingCloudStorageProvider.createTempFiledisplayName(external caller)toSafeFileName()CloudStorageProvider.generateThumbnailfile.nametoSafeFileName()(keeps thefileId_prefix)SaveExternalFilesActivity.getOutputFilefileNamevia content resolvertoSafeFileName()File.getCacheFile/getConvertedPdfCachealready join anIntid and needed no change. Sanitization applies only to local path resolution — thenameproperty itself, uploads and API metadata are untouched.New utility:
utils/FileNameUtils.ktPort of kMail's
FileNameUtils, with kDrive adaptations:String.toSafeFileName()— NFKC normalization, strips[\\/:*?"<>|%and control characters, empty/./..→"file"fallback, UTF-8 240-byte truncation preserving the extensionString.toSafeRelativePath()— sanitizes every segment of a relative path (new, kDrive-specific)File.resolveContainedFileName()/File.resolveContainedPath()— canonical-file +Path.startsWithcontainment check, reports to Sentry and returnsnullon escape. Kept non-suspend (unlike kMail's suspend variants): kDrive'sgetOfflineFilehas ~27 synchronous call sites including UI/adapter contexts, and those sites already performexists()/mkdirs()IO, so canonicalization adds no meaningful cost.Tests
app/src/test/kotlin/com/infomaniak/drive/utils/FileNameUtilsTest.kt(12 tests, JUnit 5): traversal names, control characters,./../empty fallback, fullwidth-separator normalization, UTF-8 truncation + extension preservation, containment accept/reject cases, sanitized-path round-trip.Verification
./gradlew compileStandardDebugKotlin— clean./gradlew app:testStandardDebugUnitTest— full suite green (3 classes, 0 failures)DB_VERSIONbump needed)