-
Notifications
You must be signed in to change notification settings - Fork 13k
chore!: remove ecdh
#37123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore!: remove ecdh
#37123
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 4ee8726 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
|
Caution Review failedThe pull request is closed. WalkthroughRemoves ECDH encryption functionality and related infrastructure across the codebase, including ECDH session classes, proxy service, client-side integration, settings, and the rocketchat-ddp Meteor package. Adds a database migration to clean up the ECDH_Enabled setting and updates type definitions to remove references to ECDH endpoints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (24)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37123 +/- ##
==============================================
Coverage 70.24% 70.24%
==============================================
Files 2999 2999
Lines 102301 102297 -4
Branches 18221 18224 +3
==============================================
Hits 71861 71861
+ Misses 28576 28569 -7
- Partials 1864 1867 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
b58f2a5 to
e265ab7
Compare
dd959e2 to
589f2a1
Compare
481c8af to
94daba9
Compare
ac3ad33 to
555d543
Compare
b7b6960 to
169b783
Compare
…e redundant call in E2EE Legacy Format
fa01b89 to
4ee8726
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (24)
.changeset/fair-dryers-behave.md(1 hunks)apps/meteor/.meteor/packages(0 hunks)apps/meteor/.meteor/versions(0 hunks)apps/meteor/app/api/server/default/info.ts(0 hunks)apps/meteor/app/ecdh/Session.ts(0 hunks)apps/meteor/app/ecdh/client/ClientSession.ts(0 hunks)apps/meteor/app/ui-master/server/scripts.ts(1 hunks)apps/meteor/client/ecdh.ts(0 hunks)apps/meteor/client/main.ts(0 hunks)apps/meteor/definition/externals/global.d.ts(0 hunks)apps/meteor/definition/externals/meteor/meteor.d.ts(0 hunks)apps/meteor/ee/app/ecdh/server/ServerSession.ts(0 hunks)apps/meteor/ee/server/services/ecdh-proxy/ECDHProxy.ts(0 hunks)apps/meteor/ee/server/services/ecdh-proxy/README.md(0 hunks)apps/meteor/ee/server/services/ecdh-proxy/lib/server.ts(0 hunks)apps/meteor/ee/server/services/ecdh-proxy/service.ts(0 hunks)apps/meteor/packages/rocketchat-ddp/client/index.js(0 hunks)apps/meteor/packages/rocketchat-ddp/package.js(0 hunks)apps/meteor/server/settings/general.ts(0 hunks)apps/meteor/server/startup/migrations/index.ts(1 hunks)apps/meteor/server/startup/migrations/v329.ts(1 hunks)apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.ts(2 hunks)apps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts(1 hunks)packages/rest-typings/src/default/index.ts(0 hunks)
💤 Files with no reviewable changes (18)
- packages/rest-typings/src/default/index.ts
- apps/meteor/definition/externals/meteor/meteor.d.ts
- apps/meteor/server/settings/general.ts
- apps/meteor/definition/externals/global.d.ts
- apps/meteor/ee/app/ecdh/server/ServerSession.ts
- apps/meteor/client/ecdh.ts
- apps/meteor/client/main.ts
- apps/meteor/ee/server/services/ecdh-proxy/README.md
- apps/meteor/.meteor/versions
- apps/meteor/packages/rocketchat-ddp/package.js
- apps/meteor/ee/server/services/ecdh-proxy/service.ts
- apps/meteor/app/ecdh/Session.ts
- apps/meteor/.meteor/packages
- apps/meteor/app/api/server/default/info.ts
- apps/meteor/app/ecdh/client/ClientSession.ts
- apps/meteor/ee/server/services/ecdh-proxy/lib/server.ts
- apps/meteor/ee/server/services/ecdh-proxy/ECDHProxy.ts
- apps/meteor/packages/rocketchat-ddp/client/index.js
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.tsapps/meteor/server/startup/migrations/v329.tsapps/meteor/app/ui-master/server/scripts.tsapps/meteor/server/startup/migrations/index.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/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
🧠 Learnings (17)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.tsapps/meteor/server/startup/migrations/index.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.tsapps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
.changeset/fair-dryers-behave.mdapps/meteor/server/startup/migrations/v329.tsapps/meteor/app/ui-master/server/scripts.ts
📚 Learning: 2025-10-16T21:09:51.816Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/keychain.ts:148-156
Timestamp: 2025-10-16T21:09:51.816Z
Learning: In the RocketChat/Rocket.Chat repository, only platforms with native crypto.randomUUID() support are targeted, so fallback implementations for crypto.randomUUID() are not required in E2EE or cryptographic code.
Applied to files:
.changeset/fair-dryers-behave.md
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/app/ui-master/server/scripts.ts
🔇 Additional comments (5)
.changeset/fair-dryers-behave.md (1)
1-6: LGTM!The changeset correctly specifies major version bumps for the affected packages, which is appropriate for removing ECDH functionality (a breaking change).
apps/meteor/server/startup/migrations/index.ts (1)
37-37: LGTM!The import follows the established sequential pattern and is correctly placed.
apps/meteor/server/startup/migrations/v329.ts (1)
5-11: LGTM - Migration correctly removes the ECDH_Enabled setting.The migration correctly uses
deleteOneto remove the specific setting by_id. Note that this migration is non-reversible (nodown()method), which is acceptable for cleanup of deprecated functionality.apps/meteor/app/ui-master/server/scripts.ts (1)
53-59: Removal of ECDH_Enabled from watched settings is correct.The change properly removes ECDH_Enabled from the observed settings list. The setting itself was deleted via migration v329.ts. Translation strings in i18n locale files are retained but do not affect functionality.
apps/meteor/tests/e2e/e2e-encryption/e2ee-key-reset.spec.ts (1)
4-4: LGTM! Good addition for test isolation.Adding
injectInitialData()inafterEachensures clean state between test executions and maintains proper test isolation, aligning with Playwright best practices.Based on learnings, this change helps ensure clean state for each test execution in Playwright tests.
Also applies to: 39-39
| import { BASE_API_URL } from '../config/constants'; | ||
| import injectInitialData from '../fixtures/inject-initial-data'; | ||
| import { Users, restoreState } from '../fixtures/userStates'; | ||
| import { Users } from '../fixtures/userStates'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify cleanup strategy and injectInitialData usage in E2E encryption tests
echo "=== Checking for injectInitialData usage in E2E encryption tests ==="
rg -n -C3 'injectInitialData' apps/meteor/tests/e2e/e2e-encryption/
echo -e "\n=== Checking afterEach cleanup patterns in E2E encryption tests ==="
rg -n -A5 'test\.afterEach' apps/meteor/tests/e2e/e2e-encryption/
echo -e "\n=== Listing all test files in e2e-encryption directory ==="
fd -e spec.ts . apps/meteor/tests/e2e/e2e-encryption/Repository: RocketChat/Rocket.Chat
Length of output: 7178
🏁 Script executed:
#!/bin/bash
# Read the target file to check current cleanup hooks
echo "=== Content of e2ee-legacy-format.spec.ts ==="
cat -n apps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts
echo -e "\n=== Checking for any cleanup hooks in e2ee-legacy-format.spec.ts ==="
rg -n 'test\.(after|before)' apps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.tsRepository: RocketChat/Rocket.Chat
Length of output: 4370
Add cleanup hook to reset test state between executions.
This test file lacks an afterEach hook, while e2ee-key-reset.spec.ts uses test.afterEach() with injectInitialData() for cleanup. Since this test creates encrypted channels that persist across test runs, add a similar cleanup hook to ensure test isolation and prevent state leakage between parallel test executions.
test.afterEach(async () => {
await injectInitialData();
});Also add the import at the top:
import injectInitialData from '../fixtures/inject-initial-data';🤖 Prompt for AI Agents
In apps/meteor/tests/e2e/e2e-encryption/e2ee-legacy-format.spec.ts around line
5, the test file lacks a cleanup hook to reset state between runs; add an import
for injectInitialData from '../fixtures/inject-initial-data' near the top (with
the other imports) and add a test.afterEach(async () => { await
injectInitialData(); }); after the tests to ensure encrypted channels and other
state are reset between executions.
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
ARCH-1736
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Removals
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.