fix: race condition in E2EE rooms creation causing 'incorrect encryption key' error - #41169
Merged
Conversation
🦋 Changeset detectedLatest commit: 04e8f12 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
|
| Layer / File(s) | Summary |
|---|---|
Server-side atomic key assignment packages/model-typings/src/models/IRoomsModel.ts, packages/models/src/models/Rooms.ts |
setE2eKeyId now updates only rooms without an existing e2eKeyId and returns the updated room or null. |
Client race handling in E2ERoom apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts |
createGroupKey() detects existing-key responses, discards local key material when it loses, and makes handshake() enter WAITING_KEYS. |
Race outcome tests and release metadata apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts, .changeset/four-tigers-clap.md |
Tests cover winning and losing key-creation paths, and the changeset records patch releases and the concurrency fix. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Sequence Diagram(s)
sequenceDiagram
participant E2ERoom
participant REST API
participant RoomsRaw
E2ERoom->>REST API: POST setRoomKeyID(keyID)
REST API->>RoomsRaw: setE2eKeyId(roomId, keyID)
RoomsRaw-->>REST API: updated room or null
REST API-->>E2ERoom: success or already-exists error
E2ERoom->>E2ERoom: distribute key or discard local key and wait
Suggested labels: type: bug
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly describes the main fix: an E2EE room creation race condition causing incorrect encryption key errors. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Warning
Review ran into problems
🔥 Problems
Errors were encountered while retrieving linked issues.
Errors (1)
- SUP-1067: Request failed with status code 401
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 @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41169 +/- ##
===========================================
- Coverage 68.47% 68.45% -0.02%
===========================================
Files 4092 4092
Lines 158285 158362 +77
Branches 28622 28667 +45
===========================================
+ Hits 108379 108406 +27
- Misses 44874 44924 +50
Partials 5032 5032
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid unsafe type cast for
keyIDon discard; widen the field type instead.
this.keyID = undefined as unknown as string;forces an invalid value through the type system. SincediscardGroupKey()legitimately needs to clear the key, the underlying[KEY_ID]field should be typed asstring | undefined.As per coding guidelines, "Write concise, technical TypeScript/JavaScript with accurate typing" (
**/*.{ts,tsx,js}).♻️ Proposed fix
- [KEY_ID]: string; + [KEY_ID]: string | undefined;private discardGroupKey() { this.groupSessionKey = null; this.sessionKeyExportedString = undefined; - this.keyID = undefined as unknown as string; + this.keyID = undefined; }Also applies to: 437-456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts` at line 82, The `[KEY_ID]` field in `RocketchatE2ERoom` is typed too narrowly, which forces an unsafe cast when `discardGroupKey()` clears it. Widen the field declaration to allow `undefined`, and update the related `keyID` accessors/assignment in `discardGroupKey()` so the value can be cleared without any `as unknown as string` cast. Keep the typing consistent anywhere `KEY_ID` is read or written in this class.Source: Coding guidelines
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts (1)
38-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the rethrow path.
Only the "already exists" error path is tested. Consider adding a case where
sdk.rest.postrejects with aResponsecarrying a differenterrorType(or a non-Responseerror) to assertcreateGroupKey()rethrows rather than silently discarding the key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts` around lines 38 - 51, The current `createGroupKey()` test only covers the `error-room-e2e-key-already-exists` discard path, so add a separate spec in `rocketchat.e2e.room.spec.ts` that makes `sdk.rest.post` reject with a different `Response.errorType` or a non-`Response` error and asserts the `createGroupKey()` call rethrows. Use `makeRoom()` and `e2eRoom.createGroupKey()` to locate the behavior, and verify it does not return `false` or clear `groupSessionKey`/`keyID` when the error is not the race-lost case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts`:
- Around line 38-51: The current `createGroupKey()` test only covers the
`error-room-e2e-key-already-exists` discard path, so add a separate spec in
`rocketchat.e2e.room.spec.ts` that makes `sdk.rest.post` reject with a different
`Response.errorType` or a non-`Response` error and asserts the
`createGroupKey()` call rethrows. Use `makeRoom()` and
`e2eRoom.createGroupKey()` to locate the behavior, and verify it does not return
`false` or clear `groupSessionKey`/`keyID` when the error is not the race-lost
case.
In `@apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts`:
- Line 82: The `[KEY_ID]` field in `RocketchatE2ERoom` is typed too narrowly,
which forces an unsafe cast when `discardGroupKey()` clears it. Widen the field
declaration to allow `undefined`, and update the related `keyID`
accessors/assignment in `discardGroupKey()` so the value can be cleared without
any `as unknown as string` cast. Keep the typing consistent anywhere `KEY_ID` is
read or written in this class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 328e0ae3-8649-4ff4-9323-3c7fffd79b90
📒 Files selected for processing (5)
.changeset/four-tigers-clap.mdapps/meteor/app/e2e/server/methods/setRoomKeyID.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.tspackages/models/src/models/Rooms.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts
🧠 Learnings (8)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/four-tigers-clap.md
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts
🔇 Additional comments (8)
packages/models/src/models/Rooms.ts (2)
1218-1233: LGTM!
1218-1233: 🎯 Functional CorrectnessCheck whether any caller depends on
setE2eKeyIdoverwriting an existing value; this now no-ops whene2eKeyIdis already set.apps/meteor/app/e2e/server/methods/setRoomKeyID.ts (1)
22-34: LGTM!apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts (3)
37-45: LGTM!
457-467: LGTM!
343-348: 🎯 Functional CorrectnessEnsure the race-lost branch still requests the room key The
!createdpath only switches toWAITING_KEYS; if the proactive key push is missed, this client has no fallback request and can remain stuck.apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts (1)
31-64: LGTM!.changeset/four-tigers-clap.md (1)
1-7: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
KevLehman
reviewed
Jul 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/model-typings/src/models/IRoomsModel.ts`:
- Line 230: The `IRoomsModel.setE2eKeyId` signature is still typed with the
wrong options type, so update the interface to match `RoomsRaw.setE2eKeyId` by
replacing `FindOptions<IRoom>` with `FindOneAndUpdateOptions`. Also adjust the
imports in `IRoomsModel` so the matching option type is referenced consistently
with the implementation.
🪄 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: b230980f-4e87-4e29-ad1a-95dfbf18abf8
📒 Files selected for processing (5)
.changeset/four-tigers-clap.mdapps/meteor/app/e2e/server/methods/setRoomKeyID.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/Rooms.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/four-tigers-clap.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
⚠️ CI failures not shown inline (3)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ✅ **Has milestone or project**
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/model-typings/src/models/IRoomsModel.tsapps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/model-typings/src/models/IRoomsModel.tsapps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/model-typings/src/models/IRoomsModel.tsapps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/model-typings/src/models/IRoomsModel.tsapps/meteor/app/e2e/server/methods/setRoomKeyID.tspackages/models/src/models/Rooms.ts
🔇 Additional comments (2)
apps/meteor/app/e2e/server/methods/setRoomKeyID.ts (1)
22-30: This implements the previously requested change to usefindOneAndUpdateand pass the full room document to the notifier.packages/models/src/models/Rooms.ts (1)
1219-1233: LGTM!
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cardoso
previously approved these changes
Jul 7, 2026
KevLehman
previously approved these changes
Jul 7, 2026
tassoevan
previously approved these changes
Jul 9, 2026
tassoevan
left a comment
tassoevan
left a comment
Member
There was a problem hiding this comment.
Approved, but because I don't remember if there is any legit workflow where overriding the E2E key is a valid operation.
KevLehman
dismissed stale reviews from tassoevan, cardoso, and themself
via
July 13, 2026 13:14
88d05ef
KevLehman
force-pushed
the
fix/e2ee-rooms-race-cond
branch
from
July 13, 2026 13:14
d6c12c3 to
88d05ef
Compare
KevLehman
previously approved these changes
Jul 13, 2026
tassoevan
previously approved these changes
Jul 15, 2026
cardoso
previously approved these changes
Jul 15, 2026
KevLehman
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Jul 15, 2026
KevLehman
dismissed stale reviews from cardoso, tassoevan, and themself
via
July 16, 2026 18:36
04e8f12
KevLehman
force-pushed
the
fix/e2ee-rooms-race-cond
branch
from
July 16, 2026 18:36
88d05ef to
04e8f12
Compare
KevLehman
approved these changes
Jul 16, 2026
tassoevan
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
There was a race condition occurring when a E2EE room is created with several online users. It happens because when the room is created, every online added user triggers a request to the
e2e.setRoomKeyIDendpoint concurrently, which isn't atomic (it first reads the room, then checks if it hase2eKeyId, and if it doesn't have it, then another roundtrip to the database is performed to add the key). This two-phase behavior caused stale states for users who lost the race.To address this:
$existscondition to thee2eKeyIdproperty in the query of theRooms.setE2eKeyIdmodel method. This avoids overwriting the value if another user already won the race. Also theRooms.findOneByIdquery was removed, since we are already checking for room availability withcanAccessRoomIdAsync.WAITING_KEYS.apps/meteor/client/lib/e2ee/rocketchat.e2e.room.spec.ts) checking the local values are discarded if another user won the race.Issue(s)
SUP-1067 E2EE: rooms created with encryption enabled at creation leave members' messages undecryptable ("incorrect encryption key")
Steps to test or reproduce
To reproduce this even more consistently, I'd recommend to have five or more users online concurrently.
Further comments
Summary by CodeRabbit
Bug Fixes
Tests