feat: Encryption security hardening #6668
Conversation
…note for future testing
WalkthroughAdds server-versioned E2E encryption (v1 AES‑CBC‑like and v2 AES‑GCM) across JS/TS and native layers, prefixed‑base64 payload handling, BIP‑39 passphrase generation and validation, message/content model updates, async notification decryption, UI/i18n updates, iOS/Android build changes, and new Maestro E2E test flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as RN UI
participant Room as EncryptionRoom (TS)
participant Native as Native Crypto
participant Server as Server
UI->>Room: encryptText(message)
Room->>Room: select algorithm by server version (v1 | v2)
alt rc.v2.aes-sha2 (v2)
Room->>Native: aesGcmEncrypt(key, iv, plaintext)
Native-->>Room: {ciphertext, iv}
Room-->>UI: content {algorithm, ciphertext, kid, iv}
else rc.v1.aes-sha2 (v1)
Room->>Native: legacy AES‑CBC-like encrypt
Native-->>Room: ciphertext
Room-->>UI: content {algorithm, ciphertext}
end
UI->>Server: sendMessage({ content, msg? })
sequenceDiagram
autonumber
participant Push as Push Receiver
participant Proc as E2ENotificationProcessor (Android)
participant Ctx as React Context Provider
participant Enc as Native Encryption
participant Sys as System Notification
Push->>Proc: processAsync(bundle, ejson)
loop poll for React context (until timeout)
Proc->>Ctx: getReactContext()
end
Proc->>Enc: decryptRoomKey(e2eKey)
Enc-->>Proc: RoomKeyResult { decryptedKey, algorithm }
Proc->>Enc: decryptContent(content/msg, decryptedKey, algorithm)
Enc-->>Proc: plaintext
Proc->>Sys: showNotification(plaintext)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…note for future testing
|
Android Build Available Rocket.Chat Experimental 4.65.0.107511 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTCR9OPP1tr5x7boVZvYY6m8o0GjrhvkwzQp8ZvfBFD89ZxXmRuCnDCHU4o0tlbdg7Y166tr8Yrxd1cmxH6 |
|
iOS Build Available Rocket.Chat Experimental 4.65.0.107512 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
app/lib/encryption/room.ts (2)
198-223: [DUPLICATE] Handle non-2048-bit RSA keys in importRoomKey.
decodePrefixedBase64enforces a fixed 256-byte ciphertext length (2048-bit RSA). Users with 4096-bit or other key sizes will encounterRangeErrorand fail to import room keys. Add a try-catch fallback to the legacy 12-character prefix format.Apply this diff to add fallback handling:
): Promise<{ sessionKeyExportedString: string; roomKey: ArrayBuffer; keyID: string; algorithm: TAlgorithm }> => { try { - // Parse the encrypted key using prefixed base64 - const [kid, encryptedData] = decodePrefixedBase64(E2EKey); + // Try modern prefixed base64 (v2/standard format) + let kid: string; + let encryptedData: ArrayBuffer; + try { + [kid, encryptedData] = decodePrefixedBase64(E2EKey); + } catch (err) { + // Fallback: legacy format uses 12-char keyID + base64 ciphertext + if (err instanceof RangeError) { + kid = E2EKey.slice(0, 1 55CE 2); + encryptedData = b64ToBuffer(E2EKey.slice(12)); + } else { + throw err; + } + } // Decrypt the session key const decryptedKey = await rsaDecrypt(bufferToB64(encryptedData), privateKey);
353-362: [DUPLICATE] Handle >2048-bit RSA keys when exporting.
encodePrefixedBase64requires exactly 256 bytes (2048-bit RSA). For users with 4096-bit keys,encryptedBuffer.byteLengthwill be 512 bytes, throwingRangeErrorand preventing room key sharing. Add a try-catch to fallback to legacykid + base64format.Apply this diff to add fallback:
const encryptedUserKey = await rsaEncrypt(this.sessionKeyExportedString as string, userKey); const encryptedBuffer = b64ToBuffer(encryptedUserKey as string); - return encodePrefixedBase64(this.keyID, encryptedBuffer); + try { + return encodePrefixedBase64(this.keyID, encryptedBuffer); + } catch (err) { + // Fallback for non-standard RSA key sizes (e.g., 4096-bit) + if (err instanceof RangeError) { + return `${this.keyID}${encryptedUserKey}`; + } + throw err; + } } catch (e) { log(e); }app/lib/encryption/encryption.ts (1)
207-225: [DUPLICATE] Enforce server.version before encryption.
compareServerVersion(undefined, …)returns false and silently falls back to v1 (weaker) ifserver.versionis missing. Replace the TODO with an explicit null check or assertion onversion, throwing or deferring until it's populated.Apply this diff to add explicit validation:
encodePrivateKey = async (privateKey: string, password: string, userId: string) => { - // TODO: get the appropriate server version const { version } = store.getState().server; + if (!version) { + throw new Error('Server version not available. Cannot determine encryption version.'); + } const isV2 = compareServerVersion(version, 'greaterThanOrEqualTo', '7.9.0');
🧹 Nitpick comments (1)
app/lib/encryption/room.ts (1)
448-475: Consider explicit algorithm validation.While
this.algorithmis always initialized beforeencryptTextruns (guarded by!this.readycheck inencrypt()), adding explicit validation at the start ofencryptTextwould provide defense-in-depth against future refactoring errors.Apply this diff to add validation:
): Promise< | { algorithm: 'rc.v2.aes-sha2'; kid: string; iv: string; ciphertext: string } | { algorithm: 'rc.v1.aes-sha2'; ciphertext: string } > => { const textBuffer = utf8ToBuffer(text); + if (!this.algorithm || (this.algorithm !== 'A256GCM' && this.algorithm !== 'A128CBC')) { + throw new Error(`Encryption algorithm not initialized: ${this.algorithm}`); + } if (this.algorithm === 'A256GCM') {
📜 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 (15)
.maestro/helpers/login-with-deeplink.yaml(1 hunks).maestro/helpers/open-deeplink.yaml(1 hunks).maestro/scripts/data.js(1 hunks).maestro/tests/e2ee/flows/accept-new-key-and-verify.yaml(1 hunks).maestro/tests/e2ee/flows/check-encrypted-room-without-key.yaml(1 hunks).maestro/tests/e2ee/flows/enter-key-read-and-send.yaml(1 hunks).maestro/tests/e2ee/flows/reset-keys-and-send.yaml(1 hunks).maestro/tests/e2ee/flows/verify-message-unread.yaml(1 hunks).maestro/tests/e2ee/flows/verify-messages-read.yaml(1 hunks)app/definitions/rest/v1/chat.ts(2 hunks)app/lib/encryption/encryption.ts(10 hunks)app/lib/encryption/room.ts(16 hunks)app/lib/services/restApi.ts(1 hunks)app/reducers/encryption.test.ts(2 hunks)app/reducers/encryption.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/reducers/encryption.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .maestro/tests/e2ee/flows/accept-new-key-and-verify.yaml
🧰 Additional context used
🧬 Code graph analysis (5)
app/definitions/rest/v1/chat.ts (2)
app/definitions/IRoom.ts (1)
IServerRoom(152-236)app/definitions/IMessage.ts (1)
EncryptedContent(107-107)
app/lib/services/restApi.ts (2)
app/definitions/IMessage.ts (1)
IMessage(147-178)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(108-454)
app/reducers/encryption.test.ts (2)
app/reducers/mockedStore.ts (1)
mockedStore(7-7)app/actions/encryption.ts (1)
encryptionSet(32-38)
app/lib/encryption/encryption.ts (6)
app/lib/encryption/room.ts (1)
EncryptionRoom(71-823)app/lib/store/auxStore.ts (1)
store(6-6)app/lib/methods/helpers/compareServerVersion.ts (1)
compareServerVersion(10-15)app/lib/encryption/utils.ts (8)
b64ToBuffer(15-15)bufferToHex(12-12)bufferToB64(17-17)utf8ToBuffer(42-59)joinVectorData(73-78)parsePrivateKey(239-276)bufferToUtf8(61-65)generatePassphrase(300-329)app/definitions/IMessage.ts (2)
TMessageModel(180-183)IMessage(147-178)app/lib/constants/keys.ts (2)
E2E_MESSAGE_TYPE(1-1)E2E_STATUS(5-8)
app/lib/encryption/room.ts (2)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(108-454)app/lib/encryption/utils.ts (10)
decodePrefixedBase64(200-219)bufferToB64(17-17)toString(80-86)b64ToBuffer(15-15)bufferToB64URI(21-40)encodePrefixedBase64(221-237)utf8ToBuffer(42-59)joinVectorData(73-78)splitVectorData(67-71)bufferToUtf8(61-65)
⏰ 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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (30)
.maestro/scripts/data.js (1)
14-15: Verify test password aligns with BIP-39 passphrase format.The test password
'Password1@abcdefghijklmnopqrst'uses a traditional alphanumeric+symbol format, but ESH-23 specifies adopting BIP-39 wordlist for passphrases. If the new implementation requires BIP-39 format (space-separated words from the 2048-word list), this test should use a BIP-39-compliant passphrase to validate the actual requirements.Confirm whether:
- This test intentionally uses the old/non-BIP-39 format (e.g., testing legacy compatibility)
- Or if it should use a BIP-39 formatted passphrase like
'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'Based on PR objectives ESH-23.
.maestro/tests/e2ee/flows/verify-message-unread.yaml (1)
1-19: LGTM!The test flow correctly composes authentication, navigation, and key entry helpers, then verifies the unread receipt indicator with an appropriate 60-second timeout.
.maestro/helpers/login-with-deeplink.yaml (1)
7-12: LGTM!Adding
stopAppbeforeclearStateensures a clean initialization sequence, and removing the platform condition from the clearState block simplifies the flow without affecting Android-specific permission handling..maestro/tests/e2ee/flows/check-encrypted-room-without-key.yaml (1)
1-18: LGTM!The flow correctly verifies encrypted room visibility when the user lacks the decryption key, supporting the v1/v2 auto-detection objective (ESH-28).
app/reducers/encryption.test.ts (1)
1-32: LGTM!Test updates correctly reflect the removal of
encryptionInitfrom the public API. The banner state expectation at line 20 correctly preserves theenabled: truestate from the prior dispatch while updating only the banner value..maestro/tests/e2ee/flows/enter-key-read-and-send.yaml (1)
1-44: LGTM!The flow comprehensively tests E2EE read and write paths: entering the key, verifying decryption of existing messages, and sending a new encrypted message. This aligns with the v2 write (ESH-26) and v1/v2 auto-detection (ESH-28) objectives.
.maestro/tests/e2ee/flows/reset-keys-and-send.yaml (1)
1-79: LGTM!The flow thoroughly tests key rotation scenarios: user key reset, key recreation, and room key reset, followed by message sending. The detailed UI verification ensures users see proper encryption status indicators during key rotation.
app/definitions/rest/v1/chat.ts (2)
1-1: LGTM!Import of
EncryptedContentenables versioned encryption payload support in the REST API type definitions.
78-78: Encrypted content handling verified
Callers ofchat.updateinapp/lib/services/restApi.tsbranch onresult.contentto postcontentor fallback totext. No further changes required.app/lib/services/restApi.ts (2)
966-970: LGTM!The encryption result validation correctly throws an error if encryption fails, preventing message updates from proceeding with unencrypted content.
972-979: LGTM!Correctly sends v2 encrypted content via the
contentfield when present, supporting the v2 write path (ESH-26).app/lib/encryption/encryption.ts (7)
13-16: LGTM: V2 encryption imports and type improvements.The new imports for AES-GCM operations, parsing utilities, and BIP-39 passphrase generation correctly support the v2 encryption flow. The type annotation on
roomInstancesalso improves type safety.Also applies to: 53-53, 63-65, 75-75
228-243: LGTM: Dual-format private key decoding.The updated
decodePrivateKeycorrectly handles both v1 and v2 encrypted private keys by parsing the format, selecting the appropriate AES algorithm (GCM for v2, CBC-like for v1), and using the parsed iteration count.
246-255: LGTM: Breaking API change for security improvement.Adding the
iterationsparameter enables different PBKDF2 iteration counts for v1 (1000) and v2 (100000), significantly strengthening key derivation. This breaking change aligns with ESH-24 requirements for enforcing longer passphrases through increased entropy.
258-262: LGTM: BIP-39 passphrase generation.Switching from
randomPassword()togeneratePassphrase()implements the ESH-23 requirement to use the standardized BIP-39 2048-word list for generating human-memorable, high-entropy passphrases.
407-414: LGTM: Performance optimization for subscription decryption.The filter that excludes already-decrypted subscriptions (
e2e === DONE) prevents redundant decryption work and potential corruption of already-processed messages. This optimization improves initialization performance when many subscriptions exist.
416-431: LGTM: Safer subscription update logic.The conditional assignment of
lastMessage(if (newSub?.lastMessage)) prevents accidental overwrites when decryption fails or returns null, improving robustness of the decryption flow.
536-540: LGTM: Safe optional chaining.Using optional chaining (
roomE2E?.decryptSubscription) prevents runtime errors when the room instance is unavailable while returningundefinedto signal decryption failure.app/lib/encryption/room.ts (12)
12-16: LGTM: Imports and algorithm property support v1/v2 encryption.The new imports (AES-GCM operations, prefixed base64 utilities) and the
TAlgorithmproperty enable version-aware encryption. The empty string inTAlgorithmtype serves as an "uninitialized" state, which is validated by the!this.readycheck inencrypt()(line 479).Also applies to: 25-26, 45-47, 69-79, 89-89
122-132: LGTM: Defensive handshake guards.The redundant
establishingcheck after async subscription fetch prevents race conditions, and theprivateKeycheck ensures the room won't attempt encryption before the user enters their E2E password.
136-194: LGTM: Improved handshake error handling.The added try-catch blocks with fallback logic allow the handshake to attempt multiple key sources (suggested key, existing key, create new key) gracefully, improving resilience when key exchange encounters issues.
227-273: LGTM: Algorithm property correctly set for v1/v2.Both encryption paths now properly set
this.algorithm('A256GCM' for v2, 'A128CBC' for v1), addressing the past review concern. The server version check ensures the correct path is taken.
478-499: LGTM: Safe encryption with ready check.The
!this.readyguard ensures encryption only proceeds after successful handshake, preventing calls toencryptTextwith uninitialized algorithm state.
502-529: LGTM: Upload encryption handles v1/v2 formats.Correctly handles the different return formats from
encryptText: v1 returns a stringciphertext, while v2 returns an object requiringEJSON.stringify.
608-625: LGTM: File encryption uses versioned encryptText.Both the content callback and file content encryption correctly use
encryptText, which returns the appropriate v1 or v2 format based onthis.algorithm.
639-645: LGTM: Dual-format file content decryption.Correctly checks for both v1 and v2 algorithm markers and delegates to
decryptContentfor format-aware decryption.
647-679: LGTM: Well-structured parse and decrypt helpers.The
parsemethod correctly handles v2 (object withalgorithm: 'rc.v2.aes-sha2') and v1 (string with 12-char prefix) formats. ThedoDecryptmethod properly branches on algorithm to use AES-GCM or AES-CBC decryption.
681-703: LGTM: Robust content decryption with key rotation.The refactored
decryptContentcorrectly handles both v1/v2 formats viaparse, supports decryption with old room keys whenkiddoesn't match the current key, and includes proper error handling.
706-742: LGTM: Complete message decryption flow.The updated
decryptmethod correctly decrypts the message content, merges decrypted fields, setse2e: 'done', and recursively decrypts quoted attachments for a complete decryption experience.
777-822: LGTM: New decryptSubscription method with optimization.The new
decryptSubscriptionmethod includes important optimizations: it checks if the message is already decrypted in the local DB (lines 804-815) and reuses that version, avoiding redundant decryption work. Multiple defensive early returns ensure robustness.
| - runFlow: | ||
| when: | ||
| visible: '.*Would like to send you notifications.*' | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: 'Allow' |
There was a problem hiding this comment.
Fix the regex casing for the notification prompt
iOS renders the system alert as "AppName" Would Like to Send You Notifications; the current pattern uses lowercase like, so Maestro never matches and the Allow tap never executes. Update the regex to match the actual casing, e.g. '.*Would Like to Send You Notifications.*'.
- visible: '.*Would like to send you notifications.*'
+ visible: '.*Would Like to Send You Notifications.*'🤖 Prompt for AI Agents
In .maestro/helpers/open-deeplink.yaml around lines 15 to 20, the regex for
matching the iOS notification alert uses lowercase "Would like" so it doesn't
match the system prompt; update the pattern to use correct casing (e.g. "Would
Like to Send You Notifications") so Maestro can match the alert and execute the
tap on "Allow".
| visible: | ||
| text: '.*${MESSAGE_1}.*' | ||
| timeout: 60000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| text: '.*${MESSAGE_2}.*' | ||
| timeout: 60000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| text: '.*${MESSAGE_3}.*' | ||
| timeout: 60000 | ||
| - extendedWaitUntil: | ||
| visible: | ||
| text: '.*${MESSAGE_4}.*' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use textMatches/textContains, not text.
extendedWaitUntil.visible.text performs an exact string comparison; it won’t treat .*${MESSAGE_X}.* as a regex. These waits will never succeed unless the UI literally renders those .*...* sequences. Switch to textContains (or textMatches with proper regex) so we actually detect the decrypted messages.
Apply this diff to fix all four checks:
- visible:
- text: '.*${MESSAGE_1}.*'
+ visible:
+ textContains: '${MESSAGE_1}'
@@
- visible:
- text: '.*${MESSAGE_2}.*'
+ visible:
+ textContains: '${MESSAGE_2}'
@@
- visible:
- text: '.*${MESSAGE_3}.*'
+ visible:
+ textContains: '${MESSAGE_3}'
@@
- visible:
- text: '.*${MESSAGE_4}.*'
+ visible:
+ textContains: '${MESSAGE_4}'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| visible: | |
| text: '.*${MESSAGE_1}.*' | |
| timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| text: '.*${MESSAGE_2}.*' | |
| < 55CE td class="border-0 px-2 py-1 blob-code-inner blob-code-deletion js-blob-code-deletion blob-code-marker-deletion"> timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| text: '.*${MESSAGE_3}.*' | |
| timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| text: '.*${MESSAGE_4}.*' | |
| visible: | |
| textContains: '${MESSAGE_1}' | |
| timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| textContains: '${MESSAGE_2}' | |
| timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| textContains: '${MESSAGE_3}' | |
| timeout: 60000 | |
| - extendedWaitUntil: | |
| visible: | |
| textContains: '${MESSAGE_4}' |
🤖 Prompt for AI Agents
In .maestro/tests/e2ee/flows/verify-messages-read.yaml around lines 24–37, the
checks use extendedWaitUntil.visible.text with regex-like strings ('
.*${MESSAGE_X}.*') which do exact-match and will fail; update each
extendedWaitUntil block to use visible.textContains (or visible.textMatches with
a proper regex) instead of visible.text so the waits detect the decrypted
message content — replace the four visible.text entries with
visible.textContains: '${MESSAGE_X}' (or visible.textMatches: '.*${MESSAGE_X}.*'
if you prefer regex).
| // RC 0.49.0 | ||
| return sdk.post('chat.update', { roomId: rid, msgId: message.id, text: msg }); | ||
| return sdk.post('chat.update', { | ||
| roomId: message.rid, | ||
| msgId: message.id, | ||
| text: message.msg || '' | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Use encryption result instead of original message.
The fallback path sends message.msg (original plaintext) instead of result.msg (encrypted or transformed content). For v1 encryption or any transformation where Encryption.encryptMessage returns a modified msg field without content, this sends plaintext to the server instead of ciphertext.
Apply this diff:
- return sdk.post('chat.update', {
- roomId: message.rid,
- msgId: message.id,
- text: message.msg || ''
- });
+ return sdk.post('chat.update', {
+ roomId: message.rid,
+ msgId: message.id,
+ text: result.msg || ''
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // RC 0.49.0 | |
| return sdk.post('chat.update', { roomId: rid, msgId: message.id, text: msg }); | |
| return sdk.post('chat.update', { | |
| roomId: message.rid, | |
| msgId: message.id, | |
| text: message.msg || '' | |
| }); | |
| }; | |
| // RC 0.49.0 | |
| return sdk.post('chat.update', { | |
| roomId: message.rid, | |
| msgId: message.id, | |
| text: result.msg || '' | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In app/lib/services/restApi.ts around lines 981 to 987, the fallback is posting
message.msg (original plaintext) instead of the encryption/transformation
result; change the payload to use result.msg (or result.msg || '') so the
encrypted/modified content is sent to the server; ensure roomId and msgId remain
the same and replace message.msg with result.msg in the sdk.post('chat.update',
...) call.
|
iOS Build Available Rocket.Chat Experimental 4.65.0.107531 |
|
Android Build Available Rocket.Chat Experimental 4.65.0.107530 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQro_QWqVtGtRIdmjax_xau4LsIm3Ba_k83xT7nOuvOKPk_PNmU7OkzdttVLtk8I0ixd6rGWkPTsU6O1_Ys |
Proposed changes
Issue(s)
Depends on:
https://rocketchat.atlassian.net/browse/ESH-23
https://rocketchat.atlassian.net/browse/ESH-24
https://rocketchat.atlassian.net/browse/ESH-30
https://rocketchat.atlassian.net/browse/ESH-26
https://rocketchat.atlassian.net/browse/ESH-28
How to test or reproduce
Screenshots
UI improvements on change e2ee password
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
UI/Style
Localization
Tests
Chores