Release 8.4.1#40368
Conversation
🦋 Changeset detectedLatest commit: 5b291c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 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 |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a changeset for a patch bump to ChangesMonorepo dependency bump, release marker, typings, and SAML decryption flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40368 +/- ##
==========================================
- Coverage 69.97% 69.92% -0.05%
==========================================
Files 3299 3307 +8
Lines 120261 120581 +320
Branches 21559 21590 +31
==========================================
+ Hits 84153 84318 +165
- Misses 32830 32976 +146
- Partials 3278 3287 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts`:
- Around line 211-213: Update the xml-encryption options object used in
Response.ts (the const options in the Response class where
disallowDecryptionWithInsecureAlgorithm is set to false) to include
warnInsecureAlgorithm: false to suppress per-login console warnings, and revise
the surrounding comment to remove the outdated "3DES" reference (mentioning only
AES-CBC compatibility concerns); also make the identical change to the options
passed in getSubject so both decryption call sites use warnInsecureAlgorithm:
false and have the corrected comment.
🪄 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: b74bac20-d31c-4ffa-bad2-aacf80481aa5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.github/actions/update-version-durability/package.jsonapps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.tsapps/meteor/package.jsonpackage.json
✅ Files skipped from review due to trivial changes (1)
- .github/actions/update-version-durability/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- apps/meteor/package.json
📜 Review details
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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:
apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts
🧠 Learnings (2)
📚 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/meteor-accounts-saml/server/lib/parsers/Response.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/meteor-accounts-saml/server/lib/parsers/Response.ts
| // disallowDecryptionWithInsecureAlgorithm defaults to true in xml-encryption v4, but AES-CBC/3DES | ||
| // are still widely used by SAML IdPs in practice, so we keep the pre-v4 behaviour here. | ||
| const options = { key: this.serviceProviderOptions.privateKey, disallowDecryptionWithInsecureAlgorithm: false }; |
There was a problem hiding this comment.
disallowDecryptionWithInsecureAlgorithm: false degrades security posture and will produce per-login stderr noise
xml-encryption versions prior to 4.0 treated AES-128-CBC and AES-256-CBC as secure; in v4 they are classified as insecure because CBC mode does not provide integrity guarantees. The deliberate opt-out here is understandable for broad IdP compatibility, but two follow-up concerns are worth addressing:
-
Per-login stderr noise. When
disallowDecryptionWithInsecureAlgorithm: falseand an insecure algorithm is encountered, a warning is piped to stderr viaconsole.warn()by default; this can be disabled via thewarnInsecureAlgorithmflag. Since AES-CBC is the most common SAML encryption algorithm, every SSO login attempt will generate console noise in production unlesswarnInsecureAlgorithm: falseis added to the options. -
3DES reference in the comment is dead code. Node 18+ does not support Triple DES algorithms, and this PR targets Node 22.16.0, so the comment mentioning "3DES" is misleading — it will never be reached regardless of this flag.
🛠️ Proposed options fix
-// disallowDecryptionWithInsecureAlgorithm defaults to true in xml-encryption v4, but AES-CBC/3DES
-// are still widely used by SAML IdPs in practice, so we keep the pre-v4 behaviour here.
-const options = { key: this.serviceProviderOptions.privateKey, disallowDecryptionWithInsecureAlgorithm: false };
+// xml-encryption v4 classifies AES-CBC as insecure; opt out for backwards compatibility with common IdPs.
+const options = { key: this.serviceProviderOptions.privateKey, disallowDecryptionWithInsecureAlgorithm: false, warnInsecureAlgorithm: false };Apply the same change to the getSubject options at line 355.
📝 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.
| // disallowDecryptionWithInsecureAlgorithm defaults to true in xml-encryption v4, but AES-CBC/3DES | |
| // are still widely used by SAML IdPs in practice, so we keep the pre-v4 behaviour here. | |
| const options = { key: this.serviceProviderOptions.privateKey, disallowDecryptionWithInsecureAlgorithm: false }; | |
| // xml-encryption v4 classifies AES-CBC as insecure; opt out for backwards compatibility with common IdPs. | |
| const options = { key: this.serviceProviderOptions.privateKey, disallowDecryptionWithInsecureAlgorithm: false, warnInsecureAlgorithm: false }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts` around
lines 211 - 213, Update the xml-encryption options object used in Response.ts
(the const options in the Response class where
disallowDecryptionWithInsecureAlgorithm is set to false) to include
warnInsecureAlgorithm: false to suppress per-login console warnings, and revise
the surrounding comment to remove the outdated "3DES" reference (mentioning only
AES-CBC compatibility concerns); also make the identical change to the options
passed in getSubject so both decryption call sites use warnInsecureAlgorithm:
false and have the corrected comment.
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
…rser etc) and replace twit (#40371) Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat> Co-authored-by: Ricardo Garim <rswarovsky@gmail.com> Co-authored-by: Pierre Lehnen <pierre.lehnen@rocket.chat>
1 participant
Summary by CodeRabbit
Chores
Bug Fixes
You can see below a preview of the release change log:
8.4.1
Engine versions
22.16.02.3.18.01.62.0Patch Changes
Bump @rocket.chat/meteor version.
(#40410 by @dionisio-bot) Disables SAML login when it is set to validate signatures without the proper configuration for it
(#40410 by @dionisio-bot) Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates)
Updated dependencies [5b291c3]: