feat: Encryption security hardening #6668
Conversation
…note for future testing
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughImplements server-versioned E2EE (v1/v2) across JS/TS and native layers, adds prefixed‑base64 payloads, BIP‑39 passphrase generation, per‑room refactor and async push decryption, updates message/content models and chat.update to accept 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(plaintext)
Room->>Room: determine algorithm (server/version)
alt v2 (rc.v2.aes-sha2)
Room->>Native: aesGcmEncrypt(key, iv, plaintext)
Native-->>Room: {ciphertext, iv}
Room-->>UI: content {algorithm, ciphertext, kid, iv}
else v1 (rc.v1.aes-sha2)
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 React context until available or 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 Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)app/lib/encryption/encryption.ts (4)
🔇 Additional comments (7)
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
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
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: 1
📜 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 (1)
app/lib/encryption/encryption.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/encryption/encryption.ts (7)
app/lib/encryption/room.ts (1)
EncryptionRoom(71-823)app/lib/encryption/index.ts (1)
EncryptionRoom(6-6)app/lib/store/auxStore.ts (1)
store(6-6)app/lib/methods/helpers/compareServerVersion.ts (1)
compareServerVersion(10-15)app/sagas/login.js (6)
userId(125-125)userId(338-338)server(89-89)server(221-221)server(296-296)server(372-372)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/sagas/encryption.js (3)
privateKey(102-102)server(20-20)server(93-93)
🔇 Additional comments (7)
app/lib/encryption/encryption.ts (7)
228-243: LGTM: Clean v1/v2 decode path.The decode logic correctly delegates version detection to
parsePrivateKey, then routes to the appropriate decryption algorithm. The flow is clear and handles both legacy v1 and new v2 formats transparently.
258-262: LGTM: BIP-39 passphrase generation.Switching to
generatePassphrasealigns with ESH-23 and ESH-44 objectives to use BIP-39 wordlist for stronger, human-memorable passphrases.
400-439: LGTM: Efficient subscription decryption with filtering.The filtering logic (lines 412-414) correctly skips already-decrypted messages, reducing redundant work. Delegation to per-room
decryptSubscriptionaligns with the new architecture.
536-540: LGTM: Clean delegation to per-room instance.The refactored method correctly delegates subscription decryption to the per-room instance, simplifying the flow and aligning with the new architecture.
573-587: LGTM: Simplified message decryption.The refactored method efficiently short-circuits non-E2E or already-decrypted messages (line 577), then delegates to the per-room instance. Clean and maintainable.
364-375: LGTM: Improved type annotations.The explicit type annotation at line 364 and the cast at line 375 improve type safety and clarity.
245-255: Remove this review comment—generateMasterKey is a private class method with no external callers.The search confirms that
generateMasterKeyis called only withinapp/lib/encryption/encryption.ts(lines 211 and 233), and both internal calls have been properly updated with the newiterationsparameter. The method is not exported individually; only the Encryption class is exported as default. Therefore, there are no external callers that could break, and this concern does not apply.Likely an incorrect or invalid review comment.
| // TODO: get the appropriate server version | ||
| const { version } = store.getState().server; | ||
| const isV2 = compareServerVersion(version, 'greaterThanOrEqualTo', '7.13.0'); | ||
|
|
||
| const ivArrayBuffer = b64ToBuffer(await randomBytes(16)); | ||
| const keyBase64 = await this.generateMasterKey(password, isV2 ? `v2:${userId}:mobile` : userId, isV2 ? 100000 : 1000); | ||
| const ivB64 = isV2 ? await randomBytes(12) : await randomBytes(16); | ||
| const ivArrayBuffer = b64ToBuffer(ivB64); | ||
| const keyHex = bufferToHex(b64ToBuffer(keyBase64)); | ||
| const ivHex = bufferToHex(ivArrayBuffer); | ||
|
|
||
| if (isV2) { | ||
| const ciphertextB64 = await aesGcmEncrypt(bufferToB64(utf8ToBuffer(privateKey)), keyHex, ivHex); | ||
| return EJSON.stringify({ iv: ivB64, ciphertext: ciphertextB64, salt: userId, iterations: 100000 }); | ||
| } | ||
|
|
||
| // v1 | ||
| const data = b64ToBuffer(await aesEncrypt(bufferToB64(utf8ToBuffer(privateKey)), keyHex, ivHex)); | ||
| return EJSON.stringify(new Uint8Array(joinVectorData(ivArrayBuffer, data))); | ||
| }; |
There was a problem hiding this comment.
Enforce server.version presence and prefer JSON.stringify for v2 encoding.
This addresses an unresolved past review comment and a maintainer's feedback:
-
Critical: The TODO at line 207 and past review remain unaddressed. If
store.getState().server.versionis undefined,compareServerVersionreturns false and silently falls back to v1 (weaker encryption). Replace the TODO with an explicit null check that throws or defers if version is missing:const { version } = store.getState().server; if (!version) { throw new Error('server.version not set: encryption requires server version'); } const isV2 = compareServerVersion(version, 'greaterThanOrEqualTo', '7.13.0');
-
Recommended refactor: At line 219, use
JSON.stringifyinstead ofEJSON.stringifyfor the v2 payload, as the object contains only plain properties (base64 strings and numbers). EJSON is intended for MongoDB-specific types and adds unnecessary overhead here.
Based on learnings from past reviews and maintainer feedback.
🤖 Prompt for AI Agents
In app/lib/encryption/encryption.ts around lines 207 to 225, add an explicit
check that store.getState().server.version is present and throw a clear Error if
missing before calling compareServerVersion so we don't silently fall back to
v1; then, for the v2 branch return a JSON.stringify of the plain object ({ iv,
ciphertext, salt, iterations }) instead of EJSON.stringify to avoid unnecessary
EJSON overhead. Ensure the thrown error message clearly states "server.version
not set: encryption requires server version" (or similar) and that
JSON.stringify is used only in the v2 path.
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.67.0.107684 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTLWD301NKEfS7cYerwXTIsDsXorWuDVlowRNPzCK4WLWbfnDy-9_EEbioZP2S8PyV2c16yHrZhBNmNMy7w |
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107685 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/lib/encryption/room.ts (2)
196-222: Unresolved: Back-compat risk with fixed RSA ciphertext length.Line 203's
decodePrefixedBase64call enforces a fixed 256-byte ciphertext length (2048-bit RSA). Different key sizes will throwRangeErrorand break room key imports.Add fallback handling for legacy key formats:
): Promise<{ sessionKeyExportedString: string; roomKey: ArrayBuffer; keyID: string; algorithm: TAlgorithm }> => { try { - // Parse the encrypted key using prefixed base64 - const [kid, encryptedData] = decodePrefixedBase64(E2EKey); + // Try strict prefixed-base64 (v2/modern) + let kid: string; + let encryptedData: Uint8Array; + try { + [kid, encryptedData] = decodePrefixedBase64(E2EKey); + } catch (err) { + // Fallback: legacy v1 uses 12-char keyID prefix + base64 ciphertext + kid = E2EKey.slice(0, 12); + encryptedData = new Uint8Array(b64ToBuffer(E2EKey.slice(12))); + } // Decrypt the session key const decryptedKey = await rsaDecrypt(bufferToB64(encryptedData.buffer), privateKey);Based on learnings from past reviews.
352-361: Unresolved: Handle non-2048-bit RSA keys when sharing room keys.Line 357's
encodePrefixedBase64requires exactly 256-byte ciphertext (2048-bit RSA). Users with 4096-bit or other key sizes will fail to receive room keys.Add fallback handling:
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 (error) { + if (error instanceof RangeError) { + // Fallback for non-2048-bit RSA keys + return `${this.keyID}${encryptedUserKey}`; + } + throw error; + } } catch (e) {Based on learnings from past reviews.
app/lib/encryption/encryption.ts (1)
207-225: Address unresolved past review concerns: version validation and v2 serialization.Two issues from previous reviews remain unaddressed:
Critical: Line 207 TODO and missing validation — if
store.getState().server.versionis undefined,compareServerVersionreturns false and silently falls back to v1 (weaker encryption). Add an explicit null check before line 209:const { version } = store.getState().server; if (!version) { throw new Error('server.version not set: encryption requires server version'); } const isV2 = compareServerVersion(version, 'greaterThanOrEqualTo', '7.13.0');Recommended refactor: Line 219 uses
EJSON.stringifyfor the v2 payload. Since the object contains only plain properties (base64 strings and numbers), useJSON.stringifyinstead to avoid unnecessary EJSON overhead.Based on learnings from past reviews.
🧹 Nitpick comments (1)
app/lib/encryption/room.ts (1)
447-474: Consider explicit algorithm validation to prevent silent v1 fallback.When
this.algorithmis uninitialized (empty string) or has an unexpected value, execution silently falls through to the v1 encryption path (line 467). WhilecreateNewRoomKeynow properly initializesthis.algorithm, explicit validation would prevent subtle edge cases:): 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(`Invalid encryption algorithm: ${this.algorithm}. Room encryption not properly initialized.`); + } if (this.algorithm === 'A256GCM') {Based on learnings from past reviews.
📜 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 (3)
app/lib/encryption/encryption.ts(10 hunks)app/lib/encryption/room.ts(15 hunks)app/lib/methods/subscriptions/rooms.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/encryption/room.ts (5)
app/lib/encryption/helpers/deferred.ts (1)
Deferred(1-39)app/lib/services/restApi.ts (2)
e2eAcceptSuggestedGroupKey(84-86)e2eRejectSuggestedGroupKey(88-90)app/lib/encryption/utils.ts (9)
decodePrefixedBase64(200-219)bufferToB64(17-17)b64ToBuffer(15-15)bufferToB64URI(21-40)encodePrefixedBase64(221-237)utf8ToBuffer(42-59)joinVectorData(73-78)splitVectorData(67-71)bufferToUtf8(61-65)app/definitions/IMessage.ts (1)
IMessage(147-178)app/definitions/ISubscription.ts (1)
ISubscription(40-119)
app/lib/encryption/encryption.ts (4)
app/lib/encryption/room.ts (1)
EncryptionRoom(71-822)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)
🔇 Additional comments (13)
app/lib/methods/subscriptions/rooms.ts (2)
223-225: LGTM: Decryption deferred to after persistence.Moving decryption out of the database transaction to explicit pending calls is a sound architectural improvement. This aligns with the per-room decryption pipeline introduced in this PR.
389-389: LGTM: Supports v2 encrypted message notifications.Adding
message.contentto the condition correctly handles v2 encrypted messages that use thecontentfield instead ofmsg.app/lib/encryption/encryption.ts (4)
228-243: LGTM: Version-aware private key decoding.The refactored decode path correctly delegates parsing to
parsePrivateKeyand selects the appropriate decryption algorithm based on version. Clean separation of concerns.
246-255: LGTM: Iteration count parameterized for v2 hardening.Adding the
iterationsparameter enables v2's higher iteration count (100000 vs v1's 1000), strengthening key derivation as required by the security hardening objectives.
258-262: LGTM: BIP-39 passphrase generation implemented.Switching to
generatePassphrasecorrectly implements ESH-23 (BIP-39 wordlist) and ESH-24 (longer passphrase enforcement).
391-422: LGTM: Optimized subscription decryption with filtering.The new filtering logic (lines 391-397) prevents redundant decryption of already-decrypted lastMessages, improving performance. The simplified delegation to per-room instances (line 401) aligns well with the refactored per-room architecture.
app/lib/encryption/room.ts (7)
122-133: LGTM: Race condition guard and private key validation.The redundant establishing check (lines 122-125) prevents race conditions during async subscription fetch, and the private key check (lines 127-132) correctly guards against proceeding before the E2EE password is entered.
226-272: LGTM: Algorithm properly initialized for v2 and v1.Both encryption paths now correctly set
this.algorithm('A256GCM' for v2 at line 245, 'A128CBC' for v1 at line 265), resolving the previously flagged initialization issue.
477-498: LGTM: Message encryption updated for v2 content structure.The refactored
encryptmethod correctly uses the new structured content format fromencryptTextand assigns it tomessage.content, aligning with the v2 encryption architecture.
646-663: LGTM: Clean abstraction for dual-format payload parsing.The new
parsehelper cleanly abstracts v2 JSON format (lines 654-656) and v1 kid+base64 format (lines 658-663), providing good separation of concerns.
665-678: LGTM: Algorithm-aware decryption helper.The new
doDecrypthelper cleanly selects the appropriate decryption function based on algorithm (A256GCM vs A128CBC) and handles parsing. Good separation of concerns.
680-702: LGTM: Refactored content decryption with key rotation support.The refactored
decryptContentcleanly delegates to the newparseanddoDecrypthelpers, and properly handles old room keys (lines 688-695) for key rotation scenarios.
776-821: LGTM: New subscription decryption with DB optimization.The new
decryptSubscriptionmethod supports the per-room architecture with appropriate guard clauses and includes a smart optimization (lines 803-814) to reuse already-decrypted messages from the DB, avoiding redundant work.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/encryption/room.ts (2)
477-498: Encryption failure silently returns unencrypted message.If
encryptTextthrows (line 483), the catch block logs the error but returns the original unencrypted message (line 497). This fail-open behavior could allow sensitive content to be sent unencrypted. Consider either throwing the error to prevent sending or marking the message with an error state.Apply this diff to prevent unencrypted sends:
} catch (e) { - // Do nothing console.error(e); + throw new Error('Failed to encrypt message: ' + (e instanceof Error ? e.message : String(e))); }
501-528: Same fail-open behavior in upload encryption.Like the
encryptmethod,encryptUploadreturns the original unencrypted upload if encryption fails (line 527). This creates the same security risk of unencrypted content being sent.Consider applying the same fix as suggested for the
encryptmethod.
♻️ Duplicate comments (2)
app/lib/encryption/room.ts (2)
196-222: Back-compat risk: decodePrefixedBase64 assumes fixed RSA ciphertext length.
decodePrefixedBase64at line 203 enforces a fixed 256-byte Base64 length (2048-bit RSA). Older v1 keys using 12-char prefix + variable-length base64, or users with 4096-bit RSA keys, will throwRangeErrorand break key imports.Wrap the call in a try-catch with fallback to legacy parsing:
): Promise<{ sessionKeyExportedString: string; roomKey: ArrayBuffer; keyID: string; algorithm: TAlgorithm }> => { try { - // Parse the encrypted key using prefixed base64 - const [kid, encryptedData] = decodePrefixedBase64(E2EKey); + let kid: string; + let encryptedData: ArrayBuffer; + try { + [kid, encryptedData] = decodePrefixedBase64(E2EKey); + } catch (err) { + // Fallback: legacy v1 uses 12-char keyID prefix + base64 ciphertext of variable length + if (err instanceof RangeError) { + kid = E2EKey.slice(0, 12); + encryptedData = b64ToBuffer(E2EKey.slice(12)); + } else { + throw err; + } + } // Decrypt the session key const decryptedKey = await rsaDecrypt(bufferToB64(encryptedData), privateKey);
352-361: Handle >2048-bit RSA ciphertext when exporting room keys.
encodePrefixedBase64at line 357 requires exactly 256 bytes. If a recipient has a 4096-bit RSA key (or any non-2048-bit key),rsaEncryptproduces a different-sized ciphertext,encodePrefixedBase64throws, and you cannot share the room key with that user.Add fallback to legacy format on size mismatch:
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 (error) { + if (error instanceof RangeError) { + // Fallback to legacy format for non-2048-bit keys + return `${this.keyID}${encryptedUserKey}`; + } + throw error; + }
🧹 Nitpick comments (2)
app/lib/encryption/room.ts (2)
447-474: Consider explicit algorithm validation to prevent silent v1 fallback.When
this.algorithm === ''(uninitialized) or has an unexpected value, execution silently falls through to the v1 encryption path at line 467. Whilehandshake()should setthis.algorithmviacreateNewRoomKeyorimportRoomKey, explicit validation would catch edge cases where the room is not properly initialized.Add validation before branching:
): 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) { + throw new Error('Encryption algorithm not initialized. Room handshake may have failed.'); + } if (this.algorithm === 'A256GCM') {
776-821: Well-structured subscription decryption with smart caching.The method includes appropriate guard clauses for ready state, message type, and encryption status. The optimization at lines 803-814 avoids re-decrypting identical messages by comparing
_updatedAttimestamps, which is a good performance enhancement for frequently accessed subscriptions.Consider adding a type guard for lastMessage to improve type safety:
decryptSubscription = async (subscription: Partial<ISubscription>) => { if (!this.ready) { return subscription; } // If the subscription doesn't have a lastMessage just return const { rid, lastMessage } = subscription; - if (!lastMessage) { + if (!lastMessage || typeof lastMessage !== 'object') { return subscription; }
📜 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(15 hunks)app/lib/methods/subscriptions/rooms.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/methods/subscriptions/rooms.ts
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/encryption/room.ts (4)
app/lib/encryption/helpers/deferred.ts (1)
Deferred(1-39)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(108-454)app/lib/services/restApi.ts (2)
e2eAcceptSuggestedGroupKey(84-86)e2eRejectSuggestedGroupKey(88-90)app/lib/encryption/utils.ts (9)
decodePrefixedBase64(200-219)bufferToB64(17-17)b64ToBuffer(15-15)bufferToB64URI(21-40)encodePrefixedBase64(221-237)utf8ToBuffer(42-59)joinVectorData(73-78)splitVectorData(67-71)bufferToUtf8(61-65)
🔇 Additional comments (7)
app/lib/encryption/room.ts (7)
226-272: Version-aware key creation implemented correctly.The method properly branches by server version (>= 7.13.0 for v2) and sets
this.algorithmto'A256GCM'(line 245) for v2 or'A128CBC'(line 265) for v1. Key sizes, key ID generation, and session key formats are appropriate for each version.
646-663: Parse method correctly handles v1/v2 payload formats.The method properly distinguishes v2 structured payloads (with
algorithm: 'rc.v2.aes-sha2') from v1 string-based payloads. The v1 path correctly extracts the 12-character key ID and splits the vector from ciphertext. Defensive handling withpayload?.ciphertext || ''prevents null reference errors.
665-678: Clean algorithm-aware decryption dispatcher.The method correctly routes decryption to
aesGcmDecryptfor v2 (A256GCM) oraesDecryptfor v1 (A128CBC), gracefully handles null decryption results, and parses the final EJSON payload. Acceptingalgorithmas a parameter rather than usingthis.algorithmprovides flexibility for decrypting old keys.
680-702: Robust v1/v2 decryption with key rotation support.The method correctly handles both encryption versions via the
parsehelper, supports key rotation by searchingoldRoomKeyswhen the key ID doesn't match, and uses the appropriate algorithm for each key. Error handling returns null rather than throwing, which aligns with the graceful degradation pattern used elsewhere.
705-741: Message decryption properly handles v1/v2 formats and nested content.The flow correctly decrypts message content (with v1 fallback to
msgfield), spreads the decrypted structured data (attachments, files, text) into the message, and processes quote attachments. Marking attachments ase2e: 'pending'appropriately signals that file content requires separate decryption.
530-636: File encryption consistently uses structured encryption results.The
encryptFilemethod properly returns structured encryption results fromencryptTextfor both thegetContentcallback (line 607-608) andfileContent(line 624). This maintains consistency with the versioned encryption approach across message and file flows.
638-644: LGTM!File content decryption correctly checks for v1/v2 algorithm markers and delegates to the versioned
decryptContentmethod.
| if (E2ESuggestedKey) { | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString, algorithm } = await this.importRoomKey( | ||
| E2ESuggestedKey, | ||
| Encryption.privateKey | ||
| ); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.algorithm = algorithm; | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString } = await this.importRoomKey(E2ESuggestedKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| await e2eAcceptSuggestedGroupKey(this.roomId); | ||
| Encryption.deleteRoomInstance(this.roomId); | ||
| return; | ||
| } catch (error) { | ||
| await e2eRejectSuggestedGroupKey(this.roomId); | ||
| } | ||
| await e2eAcceptSuggestedGroupKey(this.roomId); | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| } catch (e) { | ||
| log(e); | ||
| } | ||
| } | ||
|
|
||
| // If this room has a E2EKey, we import it | ||
| if (E2EKey && Encryption.privateKey) { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString } = await this.importRoomKey(E2EKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| if (E2EKey) { | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString, algorithm } = await this.importRoomKey(E2EKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.algorithm = algorithm; | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| } catch (error) { | ||
| this.establishing = false; | ||
| log(error); | ||
| // Fall through to try other options | ||
| } | ||
| } |
There was a problem hiding this comment.
Ready promise not rejected on key import failure, can leave room stuck.
When importRoomKey fails for E2ESuggestedKey (lines 153-155) or E2EKey (lines 169-173), the code sets establishing = false and logs the error but never rejects this.readyPromise. Callers awaiting handshake() will hang indefinitely. The room remains in a not-ready state with no path to recovery until the subscription updates externally.
Consider rejecting the promise to signal failure:
} catch (error) {
await e2eRejectSuggestedGroupKey(this.roomId);
+ this.establishing = false;
+ this.readyPromise.reject(error);
+ return;
} } catch (error) {
this.establishing = false;
+ this.readyPromise.reject(error);
log(error);
- // Fall through to try other options
+ return;
}
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.67.0.107687 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTUEipTZV9RaVOUtE0NSFeLqHqbTyUPzqNTigUDW2gt8bFHzkXPaE7_bzXE8x90LmybFQI5C99zM-BjerKo |
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
upload_experimental_android
GitHub Actions
Waiting
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107690 |
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
https://rocketchat.atlassian.net/browse/ESH-44
https://rocketchat.atlassian.net/browse/ESH-47
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