chore(deps): bump xmldom and related deps#40270
Conversation
|
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 |
|
WalkthroughBump Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
78-78: Keep the React Aria resolution out of this XML CVE PR if it is incidental.Line 78 changes an unrelated package resolution. If this came from install drift, please split/revert it or call out why it is required for this dependency bump.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 78, The change adds an incidental package resolution entry "@react-aria/toolbar@npm:^3.0.0-nightly.5042" in package.json which is unrelated to the CVE PR; either remove/revert that resolution line from this PR or split it into a separate commit/PR with an explicit rationale. Locate the resolution key string "@react-aria/toolbar@npm:^3.0.0-nightly.5042" in package.json, revert that single resolution line (or move it to a new branch/PR) and, if it must remain, add a short note in the PR description explaining why this resolution is required and how it arose (install drift vs deliberate bump).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 78: The change adds an incidental package resolution entry
"@react-aria/toolbar@npm:^3.0.0-nightly.5042" in package.json which is unrelated
to the CVE PR; either remove/revert that resolution line from this PR or split
it into a separate commit/PR with an explicit rationale. Locate the resolution
key string "@react-aria/toolbar@npm:^3.0.0-nightly.5042" in package.json, revert
that single resolution line (or move it to a new branch/PR) and, if it must
remain, add a short note in the PR description explaining why this resolution is
required and how it arose (install drift vs deliberate bump).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c70c4c14-9960-4a0d-8cd5-152a7e3b5834
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.github/actions/update-version-durability/package.jsonapps/meteor/package.jsonpackage.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
package.json
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/package.json
🔇 Additional comments (4)
apps/meteor/package.json (2)
166-166: Looks aligned with the xmldom CVE remediation.The app has direct SAML parser usage of
@xmldom/xmldom, so moving the direct dependency to the patched0.9.xline is appropriate.
309-309: Test xml-encryption@4.0.0 compatibility with SAML providers before merging.The upgrade to xml-encryption@4.0.0 introduces breaking changes: CBC algorithms (AES-128-CBC, AES-256-CBC) are now marked insecure and blocked by default. The type definitions remain at
@types/xml-encryption@1.2.4, which predates these security changes.The decrypt calls at lines 213 and 354 in Response.ts will fail at runtime if SAML providers send assertions encrypted with CBC modes. Confirm either:
- SAML providers use AES-256-GCM or other non-CBC algorithms, or
- Pass
disallowDecryptionWithInsecureAlgorithm: falsein decrypt options for CBC support.github/actions/update-version-durability/package.json (1)
19-19: LGTM for the action dependency bump.The action imports
DOMParseras a named export, which matches the documented xmldom usage pattern, and this keeps the workflow dependency aligned with the CVE remediation.package.json (1)
79-80: The@xmldom/xmldomoverride is compatible withxml-encryption@4.0.0.The npm registry confirms that
xml-encryption@4.0.0declares@xmldom/xmldom@^0.8.5as a dependency. The forced override to0.8.13satisfies this range (0.8.5 ≤ 0.8.13 < 0.9.0), so the resolution is correct and no incompatible transitive dependency is masked.
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/package.json`:
- Line 309: The SAML decryption may break because xml-encryption v4.0.0 defaults
disallowDecryptionWithInsecureAlgorithm=true and the decrypt calls in
Response.ts create options with only the private key (see the
xmlenc.decrypt(...) calls that build const options = { key:
this.serviceProviderOptions.privateKey };). Update those decrypt option objects
to include disallowDecryptionWithInsecureAlgorithm: false (or conditionally set
based on a config flag) so AES-128-CBC/AES-256-CBC/3DES-encrypted assertions
from existing IdPs can be decrypted, and document/verify against your deployed
IdPs which algorithms are used.
🪄 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: 3ac86547-8a9b-45c6-8ed6-4f885cab9c9d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.github/actions/update-version-durability/package.jsonapps/meteor/package.json
✅ Files skipped from review due to trivial changes (1)
- .github/actions/update-version-durability/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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
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: Remove the explanatory comment and make the
insecure-decryption toggle configurable: replace the hardcoded
disallowDecryptionWithInsecureAlgorithm: false usage in the Response.ts options
object with a value derived from serviceProviderOptions (e.g., use a new boolean
like allowInsecureDecryptionAlgorithm that defaults to false and invert it for
disallowDecryptionWithInsecureAlgorithm), update the same change where
EncryptedID sets this flag so both locations (the options in Response.ts and the
EncryptedID handling) read the setting from serviceProviderOptions consistently,
and update any code that constructs serviceProviderOptions to accept the new
setting with a secure default.
🪄 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: 0a1df0be-adb7-44e0-a94d-38fff3a2d6c1
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts
📜 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). (3)
- GitHub Check: 📦 Build Packages
- 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 (3)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40270 +/- ##
===========================================
- Coverage 69.83% 69.82% -0.02%
===========================================
Files 3296 3296
Lines 119173 119173
Branches 21469 21513 +44
===========================================
- Hits 83230 83211 -19
- Misses 32641 32660 +19
Partials 3302 3302
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/backport 8.4.1 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
|
/backport 8.4.1 |
|
Pull request #40370 added to Project: "Patch 8.4.1" |
|
/backport 8.3.3 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
|
/backport 8.3.3 |
|
Pull request #40383 added to Project: "Patch 8.3.3" |
|
/backport 8.2.3 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
|
/backport 8.2.3 |
|
Pull request #40428 added to Project: "Patch 8.2.3" |
Proposed changes (including videos or screenshots)
Bump dependencies to solve CVEs. Also change
apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.tsto ensure the old behavior is supported and it doesn't break anything.Issue(s)
https://rocketchat.atlassian.net/browse/VLN-332
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Chores
Bug Fixes / Compatibility