fix: decode URL-encoded characters in E2E file paths#13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix incorrect display/handling of non-ASCII E2E file names by decoding URL-encoded characters in file:// paths coming from mobile file systems.
Changes:
- Bump package version to
0.2.2. - iOS: decode percent-encoded characters when normalizing file paths in
FileUtils. - iOS/Android: add percent-decoding in AES file processing paths (with additional iOS behavior changes around output naming/return value).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| package.json | Version bump to ship the fix. |
| ios/algorithms/FileUtils.m | Normalize file:// URIs by stripping scheme and decoding percent-encoding. |
| ios/algorithms/AESCrypto.m | Decode percent-encoding for AES file processing; adds temp-file + sanitized-name handling. |
| android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt | Normalize file paths for file I/O by stripping file:// and decoding percent-encoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
Outdated
Show resolved
Hide resolved
Rohit3523
force-pushed
the
file-name-url-encoding
branch
3 times, most recently
from
a29a13e to
eaed78f
Compare
There was a problem hiding this comment.
Pull request overview
Fixes URL-encoded filenames showing up during E2E file decrypt/encrypt flows by normalizing file URIs/paths before they’re used by native file I/O.
Changes:
- Bump package version to
0.2.2. - iOS: decode percent-encoded characters in
FileUtils.normalizeFilePathand improve file processing path handling inAESCrypto. - Android: normalize
file://paths for decrypt overwrite and file input stream creation by decoding URL-encoded characters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| package.json | Version bump for the release containing the path-decoding fix. |
| ios/algorithms/FileUtils.m | normalizeFilePath now percent-decodes URL-encoded characters after stripping file://. |
| ios/algorithms/AESCrypto.m | Updates file-path normalization and adds additional stream/file handling around AES-CTR file processing. |
| android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt | Introduces a normalizeFilePath helper and uses it in decrypt overwrite + file stream creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
Outdated
Show resolved
Hide resolved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughNormalize file paths (strip Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FileUtils
participant AESCrypto
participant FileSystem
participant Cryptor
Caller->>AESCrypto: encryptFile/decryptFile(filePath, base64Key, base64Iv)
AESCrypto->>FileUtils: normalizeFilePath(filePath)
FileUtils-->>AESCrypto: decodedPath
AESCrypto->>FileSystem: check exists(decodedPath)
FileSystem-->>AESCrypto: exists / error
AESCrypto->>FileSystem: open inputStream(decodedPath), create temp output
FileSystem-->>AESCrypto: input/output handles
AESCrypto->>Cryptor: create/init cryptor(key, iv, operation)
Cryptor-->>AESCrypto: cryptor handle / error
loop read-chunks
AESCrypto->>FileSystem: read chunk
FileSystem-->>AESCrypto: chunk bytes
AESCrypto->>Cryptor: update(chunk)
Cryptor-->>AESCrypto: processed bytes
AESCrypto->>FileSystem: write bytes to output
FileSystem-->>AESCrypto: bytesWritten
AESCrypto-->>AESCrypto: verify bytesWritten == bytesOutMoved
end
AESCrypto->>Cryptor: finalize
Cryptor-->>AESCrypto: final bytes / status
alt success
AESCrypto->>FileSystem: if decrypt -> atomically replace original with output
FileSystem-->>Caller: return file://URL
else failure
AESCrypto->>FileSystem: remove temp output
FileSystem-->>Caller: return null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt`:
- Around line 210-224: The normalizeFilePath function is decoding every path
which corrupts plain filesystem filenames containing '%' characters; change it
to only decode when the input is a file:// URI. Specifically, in
normalizeFilePath(filePath: String) keep the existing removal of the "file://"
prefix and then call Uri.decode(...) only in that branch (or check
Uri.parse(filePath).scheme == "file"), otherwise return the original path
unchanged; ensure the catch block still returns the original path on decode
errors.
In `@ios/algorithms/AESCrypto.m`:
- Around line 90-97: The current normalization uses [NSURL
URLWithString:filePath] which treats '#' and '?' as URL syntax and truncates
filenames (variables: filePath, fileURL, normalizedFilePath); replace that block
to parse paths as file paths (use [NSURL fileURLWithPath:] or mirror the logic
from FileUtils.m that strips "file://" and decodes percent escapes) so filenames
containing '#' or '?' are preserved; update the import if needed to reuse
FileUtils helpers, and add a quick test/verification for filenames like
"report#1.pdf" and "file?name.txt" to confirm existence checks succeed before
merging.
In `@ios/algorithms/FileUtils.m`:
- Around line 223-231: The normalizeFilePath: helper currently percent-decodes
every input which corrupts paths containing literal '%' (e.g., "budget%20.pdf");
change normalizeFilePath: so it only calls stringByRemovingPercentEncoding when
the input begins with the "file://" scheme: detect and strip the "file://"
prefix from path (as currently done), then call stringByRemovingPercentEncoding
on that stripped path and return the decoded result; for inputs that do not
start with "file://", return the original path unchanged (no percent-decoding)
to preserve literal '%' characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f35a958-1971-4429-a304-9711659480a3
📒 Files selected for processing (4)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.ktios/algorithms/AESCrypto.mios/algorithms/FileUtils.mpackage.json
📜 Review details
🧰 Additional context used
🪛 detekt (1.23.8)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
[warning] 221-221: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (1)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt (1)
116-130: Verify the decrypt result is no longer leaking the encoded URI.Line 130 still returns
inputFileunchanged, andandroid/src/main/java/chat/rocket/mobilecrypto/MobileCryptoModule.kt:113-129resolves that exact string back to JS. If the caller derives the visible filename from the returned path,%20/%D0%9Dwill still survive even though the bytes were written totargetPath.💡 Possible adjustment
- outputFileObj.delete() - inputFile + outputFileObj.delete() + "file://$targetPath"
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt`:
- Around line 118-121: The decrypt path currently assumes file system paths by
calling FileOutputStream(targetPath) which breaks for content:// URIs; update
the write-back logic in AESCrypto (around normalizeFilePath, targetPath, and the
decrypt routine that currently uses FileOutputStream(targetPath)) to detect when
targetPath is a content URI and, in that case, open an OutputStream via the
Android ContentResolver (e.g.,
context.contentResolver.openOutputStream(Uri.parse(targetPath))) and copy bytes
from the decryption InputStream into that OutputStream; keep the existing
FileOutputStream(File(targetPath)) behavior for regular file paths so both
file:// and content:// outputs are supported.
In `@ios/algorithms/AESCrypto.m`:
- Around line 113-118: The code currently returns early after opening
outputStream or failing cryptor initialization and leaves the truncated
destination file and temp artifacts behind; update the failure branches that
check [inputStream streamStatus] and [outputStream streamStatus] and the cryptor
init error path (where CCCryptorRef is created) to perform cleanup: close
inputStream and outputStream, and delete the destination/processed file(s)
(e.g., processedFilePath / processedFileURL or whatever processed_* variables
are used) using NSFileManager before returning, ensuring the same cleanup logic
that exists at the later cleanup point (the code around the existing cleanup at
line 172) is applied in these early-return paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c424573-f937-4627-a801-23446b3a3d85
📒 Files selected for processing (3)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.ktios/algorithms/AESCrypto.mios/algorithms/FileUtils.m
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/algorithms/FileUtils.m
📜 Review details
🧰 Additional context used
🪛 detekt (1.23.8)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
[warning] 218-218: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (5)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt (2)
197-199: Nice fix: local/file input now uses normalized path before opening stream.This is a good change for
%-encoded filenames infile://paths.
210-224: Path normalization behavior is now scoped correctly.Decoding only in the
file://branch avoids altering plain filesystem names that legitimately contain%.ios/algorithms/AESCrypto.m (3)
3-3: Nice reuse of centralized path normalization.Pulling
FileUtils normalizeFilePath:intoprocessFilekeeps the existence and stream setup checks pointed at the decoded filesystem path instead of the URI form.Also applies to: 91-107
131-153: Good fail-closed handling in the streaming loop.Checking
bytesWrittenagainstdataOutMovedis a solid guard against silent truncation on the hot path.
157-195: The temp-file cleanup and atomic replace flow looks solid.Cleaning the temp output on failure and only swapping it into place after a full decrypt completes avoids in-place corruption of the source file.
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ios/algorithms/AESCrypto.m (2)
173-191: Consider reusing the existingfileManagerinstance.A
fileManagervariable is already defined at line 94, but lines 174, 183, and 191 create new instances via[NSFileManager defaultManager]. WhiledefaultManagerreturns a singleton and this is functionally correct, reusing the local variable improves consistency.♻️ Proposed fix
if (status != kCCSuccess || !loopSuccess) { - [[NSFileManager defaultManager] removeItemAtPath:outputFilePath error:nil]; + [fileManager removeItemAtPath:outputFilePath error:nil]; return nil; } if (operation == kCCDecrypt) { // For decrypt: atomically replace the original file with decrypted content NSURL *originalFileURL = [NSURL fileURLWithPath:normalizedFilePath]; NSURL *outputFileURL = [NSURL fileURLWithPath:outputFilePath]; NSError *error = nil; - NSURL *replacedURL = [[NSFileManager defaultManager] replaceItemAtURL:originalFileURL + NSURL *replacedURL = [fileManager replaceItemAtURL:originalFileURL withItemAtURL:outputFileURL backupItemName:nil options:NSFileManagerItemReplacementUsingNewMetadataOnly resultingItemURL:nil error:&error]; if (!replacedURL) { NSLog(@"Failed to replace original file: %@", error); - [[NSFileManager defaultManager] removeItemAtPath:outputFilePath error:nil]; + [fileManager removeItemAtPath:outputFilePath error:nil]; return nil; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/algorithms/AESCrypto.m` around lines 173 - 191, Replace the inline uses of [NSFileManager defaultManager] with the existing local fileManager variable to improve consistency: in the conditional after the status check (around the block that removes the temp file and the decrypt replacement logic), use fileManager for removeItemAtPath: and for replaceItemAtURL:withItemAtURL:... instead of calling [NSFileManager defaultManager] so the calls around normalizedFilePath, outputFilePath and the replaceItemAtURL invocation all reuse the same fileManager instance.
162-163: Consider verifying the final write for consistency.The write at line 163 doesn't verify
bytesWrittenagainstfinalBytesOut, unlike the check at lines 142-146. WhilefinalBytesOutis typically 0 for CTR mode (no padding), verifying the write would maintain consistent error handling.♻️ Proposed fix
if (status == kCCSuccess && finalBytesOut > 0) { - [outputStream write:buffer maxLength:finalBytesOut]; + NSInteger finalWritten = [outputStream write:buffer maxLength:finalBytesOut]; + if (finalWritten != (NSInteger)finalBytesOut) { + loopSuccess = NO; + } } else if (status != kCCSuccess) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/algorithms/AESCrypto.m` around lines 162 - 163, The final write in AESCrypto.m (inside the method performing encryption/decryption where it checks status == kCCSuccess && finalBytesOut > 0) should verify the return value of [outputStream write:buffer maxLength:finalBytesOut] just like the earlier write check; capture the bytesWritten, compare it to finalBytesOut, and if they differ, perform the same error handling/cleanup path (log the failure and return/propagate error) to ensure consistent error handling for the final write in methods like the one using CCCryptorFinal/stream writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/algorithms/AESCrypto.m`:
- Around line 173-191: Replace the inline uses of [NSFileManager defaultManager]
with the existing local fileManager variable to improve consistency: in the
conditional after the status check (around the block that removes the temp file
and the decrypt replacement logic), use fileManager for removeItemAtPath: and
for replaceItemAtURL:withItemAtURL:... instead of calling [NSFileManager
defaultManager] so the calls around normalizedFilePath, outputFilePath and the
replaceItemAtURL invocation all reuse the same fileManager instance.
- Around line 162-163: The final write in AESCrypto.m (inside the method
performing encryption/decryption where it checks status == kCCSuccess &&
finalBytesOut > 0) should verify the return value of [outputStream write:buffer
maxLength:finalBytesOut] just like the earlier write check; capture the
bytesWritten, compare it to finalBytesOut, and if they differ, perform the same
error handling/cleanup path (log the failure and return/propagate error) to
ensure consistent error handling for the final write in methods like the one
using CCCryptorFinal/stream writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fab81f3-5822-443b-82d7-1a5ab7b29496
📒 Files selected for processing (1)
ios/algorithms/AESCrypto.m
📜 Review details
🔇 Additional comments (5)
ios/algorithms/AESCrypto.m (5)
3-3: LGTM!The
FileUtils.himport is correctly added to support the path normalization at line 91.
52-58: LGTM!The
encryptFileanddecryptFilewrappers correctly delegate toprocessFilewith the appropriateCCOperationtype, matching the header declarations.
91-97: LGTM! Path normalization issue addressed.Using
[FileUtils normalizeFilePath:]correctly handles URL-encoded characters and thefile://prefix, addressing the previous issue withURLWithString:mishandling special characters like#and?in filenames.
113-131: LGTM! Stream and cryptor failure cleanup addressed.The cleanup logic at lines 117 and 129 properly removes the truncated output file on early failures, addressing the previous review concern about orphaned
processed_*files.
178-198: LGTM! Atomic file replacement and return path handling.The decrypt operation correctly uses
replaceItemAtURL:withItemAtURL:for atomic replacement, and both operations return properly formattedfile://URLs consistent with the MobileCrypto.mm caller expectations.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt (1)
119-136:⚠️ Potential issue | 🟡 MinorEnsure temp file cleanup also happens on decrypt write-back failure.
If copy-back to the target URI/path fails,
outputFileObjis not deleted (Line 135 is success-only), leaving cache artifacts behind.Proposed hardening
return if (mode == "decrypt") { // Overwrite the input file with the decrypted file val targetUri = Uri.parse(inputFile) - FileInputStream(outputFileObj).use { inputStream -> - val outputStream = if (targetUri.scheme == null || targetUri.scheme == "file") { - FileOutputStream(normalizeFilePath(inputFile)) - } else { - reactContext.contentResolver.openOutputStream(targetUri) - ?: throw IllegalArgumentException("Cannot open output stream for URI: $targetUri") - } - outputStream.use { fos -> - val buffer = ByteArray(BUFFER_SIZE) - var numBytesRead: Int - - while (inputStream.read(buffer).also { numBytesRead = it } != -1) { - fos.write(buffer, 0, numBytesRead) - } - } - } - outputFileObj.delete() - inputFile + try { + FileInputStream(outputFileObj).use { inputStream -> + val outputStream = if (targetUri.scheme == null || targetUri.scheme == "file") { + FileOutputStream(normalizeFilePath(inputFile)) + } else { + reactContext.contentResolver.openOutputStream(targetUri) + ?: throw IllegalArgumentException("Cannot open output stream for URI: $targetUri") + } + outputStream.use { fos -> + val buffer = ByteArray(BUFFER_SIZE) + var numBytesRead: Int + while (inputStream.read(buffer).also { numBytesRead = it } != -1) { + fos.write(buffer, 0, numBytesRead) + } + } + } + inputFile + } finally { + outputFileObj.delete() + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt` around lines 119 - 136, The copy-back block that writes from outputFileObj to the target (using FileInputStream(outputFileObj) and outputStream/fos) can throw and currently only deletes outputFileObj on the success path; wrap the whole copy operation in a try/finally (or use Kotlin's runCatching/also) so that outputFileObj.delete() is executed in the finally block regardless of success, rethrowing the original exception after cleanup; refer to the variables/output objects outputFileObj, inputFile, targetUri and the FileInputStream/FileOutputStream/reactContext.contentResolver.openOutputStream usage to locate and modify the copy-back section.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt`:
- Around line 119-136: The copy-back block that writes from outputFileObj to the
target (using FileInputStream(outputFileObj) and outputStream/fos) can throw and
currently only deletes outputFileObj on the success path; wrap the whole copy
operation in a try/finally (or use Kotlin's runCatching/also) so that
outputFileObj.delete() is executed in the finally block regardless of success,
rethrowing the original exception after cleanup; refer to the variables/output
objects outputFileObj, inputFile, targetUri and the
FileInputStream/FileOutputStream/reactContext.contentResolver.openOutputStream
usage to locate and modify the copy-back section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dc0e814-6d66-467f-9d30-027ee9491452
📒 Files selected for processing (1)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
📜 Review details
🧰 Additional context used
🪛 detekt (1.23.8)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt
[warning] 224-224: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (1)
android/src/main/java/chat/rocket/mobilecrypto/algorithms/AESCrypto.kt (1)
203-205: Good path-normalization scope and URI handling.Nice fix: decoding is now limited to
file://paths, and plain filesystem paths are preserved as-is. This directly addresses the encoded-filename issue without breaking literal%filenames.Also applies to: 216-230
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/algorithms/AESCrypto.m`:
- Around line 159-164: The final write in the CCCryptorFinal block (inside
function handling the cryptor/CCCryptorFinal call) currently ignores the return
value of [outputStream write:buffer maxLength:finalBytesOut]; update this to
validate the number of bytes written equals finalBytesOut (and detect
negative/error return), mirror the checks used in the main write loop (lines
handling bytesWritten and loopSuccess), and on short write or error set
loopSuccess = NO and set an appropriate non-kCCSuccess status (or return an
error) so the method does not silently succeed when the final write fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd1302b-05e7-4cd8-b1c9-8a0c7373a9e3
📒 Files selected for processing (1)
ios/algorithms/AESCrypto.m
📜 Review details
🔇 Additional comments (1)
ios/algorithms/AESCrypto.m (1)
91-97: Path normalization + early cleanup changes look correct.Using
FileUtilsnormalization before file access and deletingoutputFilePathon stream-open failure is a solid fix for encoded-path handling and temp-file hygiene.Also applies to: 113-119
Issue
When decrypting E2E (end-to-end encrypted) files, file names containing non-ASCII characters (spaces, Cyrillic, etc.) would display URL-encoded literals instead of the actual characters.
Example:
Наименование.txtНаименование.txt(or%D0%9D%D0%B0...)http://rocketchat.atlassian.net/browse/SUP-1007
Root Cause
File URIs (
file://) from the Android/iOS file system contain URL-encoded paths (e.g.,%20for spaces,%D0%9Dfor Cyrillic). The native decryption code was not decoding these paths before using them, resulting in the encoded literals appearing in file names.Summary by CodeRabbit
New Features
Bug Fixes
Chores