fix(e2ee): stop sending msg when there's content#6818
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Caution Review failedThe pull request is closed. WalkthroughConsolidates upload encryption into Changes
Sequence Diagram(s)(omitted — changes are refactor/payload tweaks without new multi-component control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
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 |
diegolmello
force-pushed
the
fix.e2ee-msg
branch
from
76a9d4b to
feb29c7
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/encryption/room.ts (1)
18-27: Remove unusedIUploadimport to fix lint failure
IUploadis still imported from../../definitionsbut no longer used (per the pipelineno-unused-varserror). This will keep CI red until removed.Suggested diff:
-import type { - IAttachment, - IMessage, - IUpload, - TSendFileMessageFileInfo, - IServerAttachment, - TSubscriptionModel, - ISubscription -} from '../../definitions'; +import type { + IAttachment, + IMessage, + TSendFileMessageFileInfo, + IServerAttachment, + TSubscriptionModel, + ISubscription +} from '../../definitions';
🧹 Nitpick comments (1)
app/lib/methods/sendFileMessage/sendFileMessageV2.ts (1)
65-81: Clarifymsgomission logic whencontentis present
(content ? '' : file.msg) || undefineddoes achieve “nomsgwhen there’s content” (since'' || undefinedevaluates toundefined), and only sendsmsgwhencontentis falsy andfile.msgis non‑empty. The behaviour looks correct for the PR goal, but the intent is a bit opaque and also dropsmsgwhenfile.msg === ''.You could make this clearer and equivalent with:
msg: content ? undefined : file.msg || undefined,which reads more directly as “if we have encrypted content, don’t send msg; otherwise send a non‑empty caption if present.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/lib/encryption/room.ts(1 hunks)app/lib/methods/sendFileMessage/sendFileMessageV2.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/encryption/room.ts (3)
app/lib/constants/keys.ts (2)
E2E_MESSAGE_TYPE(1-1)E2E_STATUS(5-8)app/lib/encryption/utils.ts (1)
getE2EEMentions(92-103)app/definitions/IMessage.ts (1)
IMessage(147-178)
🪛 GitHub Actions: Format Code with Prettier
app/lib/encryption/room.ts
[error] 22-22: 'IUpload' is defined but never used. (no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (1)
app/lib/encryption/room.ts (1)
476-502: Encrypted message shape now consistently usescontentand dropsmsgThe new
encrypt()implementation looks sound:
- It wraps the plaintext in
contentviaencryptText(EJSON.stringify({ msg: message.msg || '' })).- It marks the message as E2E (
t: E2E_MESSAGE_TYPE,e2e: E2E_STATUS.PENDING) and computese2eMentionsfrom the originalmsg.- It deletes
encryptedMessage.msg, so we no longer send plaintextmsgalongside encryptedcontent.This remains compatible with
decrypt(), which readsmessage.content || message.msgand spreads the decrypted payload back, so new and legacy messages should both decrypt correctly.I’d keep this as‑is; just ensure server‑side E2E handling is aligned with the “content only, no msg” contract for all relevant versions.
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/methods/sendFileMessage/sendFileMessageV2.ts (1)
76-76: Consider simplifying the conditional expression.The logic correctly omits
msgwhencontentis present, but the expression(content ? '' : file.msg) || undefinedrelies on empty string's falsiness, making it less immediately clear.Apply this diff for improved readability:
- msg: (content ? '' : file.msg) || undefined, + msg: content ? undefined : file.msg,Both expressions produce the same result, but the simplified version more directly expresses the intent: when
contentexists, omitmsg; otherwise, includefile.msg(which may itself be undefined).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/lib/encryption/room.ts(1 hunks)app/lib/methods/sendFileMessage/sendFileMessageV2.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/encryption/room.ts (3)
app/lib/constants/keys.ts (2)
E2E_MESSAGE_TYPE(1-1)E2E_STATUS(5-8)app/lib/encryption/utils.ts (1)
getE2EEMentions(92-103)app/definitions/IMessage.ts (1)
IMessage(147-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (1)
app/lib/encryption/room.ts (1)
484-494: LGTM: Encryption logic correctly constructs content-only encrypted messages.The refactored encryption flow properly:
- Encrypts the message text into a structured
contentobject (line 482)- Constructs an encrypted message with e2e metadata and the encrypted content (lines 484-490)
- Explicitly deletes the
msgfield to prevent sending plaintext alongside encrypted content (line 492)This directly addresses the PR objective of preventing msg from being sent when encrypted content is present.
| } catch (e) { | ||
| // Do nothing | ||
| console.error(e); | ||
| } | ||
|
|
||
| return message; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all calls to .encrypt() and examine their context
echo "=== Finding .encrypt() method calls ==="
rg -n -C4 '\.encrypt\s*\(' --type=ts | head -100
echo ""
echo "=== Finding Encryption class references ==="
rg -n -C3 'Encryption\.(encrypt|encryptMessage)' --type=ts | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2738
🏁 Script executed:
#!/bin/bash
# Examine Encryption.encryptMessage() implementation
echo "=== Examining Encryption class encryptMessage method ==="
rg -n -A20 'encryptMessage\s*\(' app/lib/encryption/encryption.ts | head -80
echo ""
echo "=== Examining sendMessage.ts context around Encryption.encryptMessage calls ==="
sed -n '60,120p' app/lib/methods/sendMessage.ts
echo ""
echo "=== Checking if there's message validation after encryption ==="
rg -n -C5 'content' app/lib/methods/sendMessage.ts | head -60Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1717
🏁 Script executed:
#!/bin/bash
# Get full context of Encryption class
echo "=== Finding Encryption.encryptMessage full implementation ==="
rg -n 'encryptMessage' app/lib/encryption/encryption.ts -A30 | head -100
echo ""
echo "=== Finding sendMessageCall implementation ==="
rg -n -B3 -A15 'sendMessageCall\s*\(' app/lib/methods/sendMessage.ts | head -80
echo ""
echo "=== Full context of EncryptionRoom.encrypt() in room.ts ==="
sed -n '470,510p' app/lib/encryption/room.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3288
🏁 Script executed:
#!/bin/bash
# Check if there's any server-side validation or other client guards
echo "=== Search for any validation of encrypted content before sending ==="
rg -n 'chat\.sendMessage' app/lib --type=ts -B5 -A5
echo ""
echo "=== Check for any guards on message.content or message.msg ==="
rg -n -C3 'message\.content\|message\.msg' app/lib/methods/sendMessage.ts
echo ""
echo "=== Check what happens with undefined return from encryptMessage ==="
rg -n -C5 'Encryption\.encryptMessage' app/lib/methods/sendMessage.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1629
🏁 Script executed:
#!/bin/bash
# Final check: Look at what properties the encrypted message should have
echo "=== Check IMessage type to understand expected structure ==="
fd -type f -name "*.ts" | xargs grep -l "interface IMessage\|type IMessage" | head -5
echo ""
echo "=== Check E2E_MESSAGE_TYPE to understand encrypted messages ==="
rg -n "E2E_MESSAGE_TYPE\|E2E_STATUS" app/lib/encryption --type=ts -B2 -A2 | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 391
Based on my verification, I have found critical security issues with the error handling in the encryption code. Let me provide the rewritten review comment:
Encryption failure silently sends plaintext messages without validation.
When encryption fails (lines 495-500), EncryptionRoom.encrypt() returns the original unencrypted message. This fallback is problematic in encrypted rooms because:
-
Encryption.encryptMessage()can return unencrypted messages or undefined:- Line 542: Returns
undefinedif room has no session key - Line 551: Returns unencrypted message if catch block executes
- Line 537: Returns unencrypted message for non-encrypted subscriptions (intentional but indistinguishable from failures)
- Line 542: Returns
-
Calling code has no validation:
sendMessage()andresendMessage()(app/lib/methods/sendMessage.ts) don't validate the return value- They pass whatever is returned directly to
sendMessageCall()which sends it to the API - No check for undefined, no check for plaintext
msgfield vs encryptedcontentfield
-
Inconsistent with
editMessage():editMessage()(app/lib/services/restApi.ts:967-969) explicitly checks:if (!result) throw new Error('Failed to encrypt message')- Message send paths lack this guard, creating different security guarantees
Required action: Add explicit null/undefined check in sendMessage() and resendMessage() before calling sendMessageCall(), and/or differentiate between intentional fallback (non-encrypted room) vs error cases. Users expect E2E encryption; silent plaintext fallback is a security risk.
🤖 Prompt for AI Agents
In app/lib/encryption/room.ts around lines 495-500, EncryptionRoom.encrypt()
currently catches errors and returns the original plaintext message, which can
cause silent plaintext sends; change behavior so encryption failures do not
return the original plaintext (either return undefined/null or throw) and ensure
the catch logs error details but does not fall back to plaintext. Then update
callers app/lib/methods/sendMessage.ts (sendMessage and resendMessage) to
explicitly check the encrypt() result before calling sendMessageCall(): if
encrypt() returns undefined/null or an error is thrown, abort sending and
surface/throw a clear "Failed to encrypt message" error (or return a failure
result) rather than sending plaintext; alternatively, make encrypt() return a
distinct sentinel for non-encrypted rooms and have callers treat that
differently from an encryption failure.
|
Android Build Available Rocket.Chat Experimental 4.67.0.107750 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQt__e0fa4oT7lN2awbuhsKv2Sa5uCqjfSEzh8VwafFq9pU1pLMttQh1pkjP3DkDqA8076-JHVim5d4BsWM |
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107751 |
diegolmello
force-pushed
the
fix.e2ee-msg
branch
from
feb29c7 to
51fb3b1
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
2 participants
Proposed changes
Removed
msgwhen there'scontent.Although I was not able to reproduce it, that was the reason it was breaking web app.
Tested both on v1 (7.12.0) and v2 (7.13.0).
Issue(s)
Closes #6817
https://rocketchat.atlassian.net/browse/CORE-1525
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.