Skip to content

fix: e2ee attachment getting locked after interaction#6735

Merged
diegolmello merged 3 commits into
developfrom
e2e-attachment-fix
Dec 12, 2025
Merged

fix: e2ee attachment getting locked after interaction#6735
diegolmello merged 3 commits into
developfrom
e2e-attachment-fix

Conversation

@Rohit3523

@Rohit3523 Rohit3523 commented Oct 17, 2025

Copy link
Copy Markdown
Member

Proposed changes

Whenever we or someone try to interact with an attachment in e2ee channel like reacting to that message or read receipt event happen, we receive websocket event which just overwrite the attachment payload provided from backend which removes the e2ee done and cache property from the local database (this thing is done in the app), so before updating the message, i am just trying to get the attachment from the local db and add the e2ee key and local cache path to each object.

Issue(s)

Closes: #5977
https://rocketchat.atlassian.net/browse/COMM-86

How to test or reproduce

  1. Enable E2EE in DM or Private Channel
  2. Send an attachment
  3. React with any emoji
  4. Observe the same attachment

It will show overlay component with key icon if we use the code from develop

Screenshots

Before
https://github.com/user-attachments/assets/8b5c1e5a-8d7c-4d77-a79d-2b6aa506b168

After
https://github.com/user-attachments/assets/5d496425-4a95-4be4-a526-651cd8746bde

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Summary by CodeRabbit

  • Bug Fixes
    • Attachments in encrypted messages now inherit existing encryption metadata correctly; attachment title links are preserved only when encryption is fully completed, otherwise original links are kept.

✏️ Tip: You can customize this high-level summary in your review settings.

@Rohit3523 Rohit3523 requested a review from diegolmello October 17, 2025 22:22
@coderabbitai

coderabbitai Bot commented Oct 17, 2025

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

When updating messages, if an existing messageRecord is found and its type is e2e, attachments are transformed by merging each attachment with the existing record's e2e metadata; the existing title_link is preserved only when the existing e2e status is 'done'.

Changes

Cohort / File(s) Summary
E2E Attachment Metadata Handling
app/lib/methods/subscriptions/room.ts
In updateMessage, when an existing messageRecord is e2e, transform attachments by augmenting each attachment with the existing record's e2e metadata; preserve title_link from the existing record only if its e2e status is 'done', otherwise keep the incoming title_link.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Potential attention points:

  • app/lib/methods/subscriptions/room.ts — verify merging logic preserves required e2e fields and does not drop other attachment properties.
  • Ensure tests or manual checks cover both e2e === 'done' and other e2e states to confirm title_link selection.

Possibly related PRs

Poem

🐰 I hop through code to mend a thread,
Merging secrets where attachments tread,
Titles kept when locks are done,
Else fresh links race with the sun,
Hooray—encrypted files now tread.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "fix: e2ee attachment getting locked after interaction" directly relates to the main change in the changeset. The title accurately describes the core problem being fixed—preventing E2EE attachments from losing their metadata and becoming unusable after websocket-driven interactions like read receipts. The title is concise, clear, and specific enough that teammates reviewing history would understand the primary objective without ambiguity.
Linked Issues Check ✅ Passed The code changes in app/lib/methods/subscriptions/room.ts directly implement the solution to the objectives outlined in linked issue #5977. The modifications augment attachments with E2EE metadata retrieved from the local database before websocket payload updates overwrite them, which directly addresses the requirement to preserve client-side metadata (e2e flags and local cache paths). This approach prevents E2EE-encrypted attachments from becoming unusable after read receipts or similar interaction-triggered message updates, fulfilling all primary coding requirements specified in the linked issue.
Out of Scope Changes Check ✅ Passed The changeset modifies only app/lib/methods/subscriptions/room.ts with focused changes to the updateMessage function that preserve E2EE metadata during message updates. All modifications are directly scoped to the stated objective of preventing E2EE attachment metadata loss during websocket-driven updates, with no other behavioral changes or unrelated refactoring present. The changes are tightly aligned with the requirements in linked issue #5977 and do not introduce functionality outside the defined scope.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7418e4d and 1bc3615.

📒 Files selected for processing (1)
  • app/lib/methods/subscriptions/room.ts (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Rohit3523

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Rohit3523 Rohit3523 changed the title fix: e2e attachment getting locked after interaction fix: e2ee attachment getting locked after interaction Oct 17, 2025
@Rohit3523 Rohit3523 had a problem deploying to official_android_build October 17, 2025 22:26 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to experimental_ios_build October 17, 2025 22:26 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to experimental_android_build October 17, 2025 22:26 — with GitHub Actions Error

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/lib/methods/subscriptions/room.ts (1)

266-272: Consider optimizing attachment matching for better performance.

The current nested loop (mapping over new attachments while finding in existing attachments) has O(n*m) complexity. While acceptable for typical attachment counts, this could be optimized.

Consider creating a Map for O(1) lookups:

 if (messageRecord.t === 'e2e' && message.attachments) {
+  // Create lookup map keyed by URL for efficient matching
+  const existingAttachmentMap = new Map(
+    messageRecord.attachments?.map(att => {
+      const key = att.image_url || att.video_url || att.audio_url || att.thumb_url;
+      return [key, att];
+    }) ?? []
+  );
+
   message.attachments = message.attachments?.map(att => {
-    const existing = messageRecord.attachments?.find(
-      a =>
-        a.image_url === att.image_url ||
-        a.video_url === att.video_url ||
-        a.audio_url === att.audio_url ||
-        a.thumb_url === att.thumb_url
-    );
+    const key = att.image_url || att.video_url || att.audio_url || att.thumb_url;
+    const existing = existingAttachmentMap.get(key);

     return {
       ...att,
       e2e: existing?.e2e,
       title_link: existing?.e2e === 'done' ? existing?.title_link : att.title_link
     };
   });
 }
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 390dc39 and f774e82.

📒 Files selected for processing (1)
  • app/lib/methods/subscriptions/room.ts (1 hunks)
⏰ 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 (2)
app/lib/methods/subscriptions/room.ts (2)

277-277: Clarify the conditional title_link preservation logic.

The title_link is preserved only when existing?.e2e === 'done', otherwise the new title_link from the websocket payload is used. Please verify this is the intended behavior. If title_link contains the local decrypted file path when E2EE decryption is complete, this logic makes sense—but it should be documented with a comment.

Additionally, consider whether there are edge cases where e2e might have other values (e.g., 'pending', 'error') that should also preserve the title_link.


264-280: The PR fix is complete and correct—no verification issues found.

The code properly preserves all necessary E2EE metadata:

  • The e2e property is preserved from the existing attachment
  • The title_link (which stores the local cached file path) is conditionally preserved when e2e === 'done'

The title_link property confirmed as the local cache referenced in the PR description, used for offline access to downloaded media. The conditional preservation logic is sound—only preserve the cache when decryption is complete (e2e === 'done'), otherwise use the new URL from the websocket event.

Comment thread app/lib/methods/subscriptions/room.ts
@Rohit3523 Rohit3523 marked this pull request as ready for review October 18, 2025 19:35
@Rohit3523 Rohit3523 had a problem deploying to experimental_ios_build October 18, 2025 19:37 — with GitHub Actions Failure
@Rohit3523 Rohit3523 had a problem deploying to official_android_build October 18, 2025 19:37 — with GitHub Actions Failure
@Rohit3523 Rohit3523 had a problem deploying to experimental_android_build October 18, 2025 19:37 — with GitHub Actions Failure
@diegolmello diegolmello merged commit 5585f5d into develop Dec 12, 2025
4 of 6 checks passed
@diegolmello diegolmello deleted the e2e-attachment-fix branch December 12, 2025 12:38
@diegolmello diegolmello had a problem deploying to experimental_ios_build December 12, 2025 12:42 — with GitHub Actions Failure
@diegolmello diegolmello had a problem deploying to official_android_build December 12, 2025 12:42 — with GitHub Actions Failure
@diegolmello diegolmello had a problem deploying to experimental_android_build December 12, 2025 12:42 — with GitHub Actions Failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When read receipts works and E2E encryption photos become "whitelabeled " on iOS app

2 participants