chore: upgrade xml-crypto to v6 - #41379
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
548f870 to
7e84bc9
Compare
WalkthroughUpdates ChangesSAML signature modernization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (3)
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.
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 `@apps/meteor/server/lib/saml/lib/parsers/Response.ts`:
- Around line 362-364: Remove the explanatory comment immediately preceding the
signedReference null check in the Response parser, leaving the existing
conditional logic unchanged.
🪄 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: 031af725-4202-4f4a-b46a-807273f4f221
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/meteor/package.jsonapps/meteor/server/lib/saml/lib/parsers/Response.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- 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/server/lib/saml/lib/parsers/Response.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:
apps/meteor/server/lib/saml/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/server/lib/saml/lib/parsers/Response.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/server/lib/saml/lib/parsers/Response.ts
🔇 Additional comments (5)
apps/meteor/package.json (2)
322-322: LGTM!
423-424: LGTM!apps/meteor/server/lib/saml/lib/parsers/Response.ts (3)
2-3: LGTM!
321-325: LGTM!
374-385: 🎯 Functional Correctness
xml-cryptov6.1.2 supports these accesses.SignedXmlacceptspublicCert,ReferenceincludesvalidationErrorandsignedReference, andgetReferences()returnsReference[], so the null-safe.map()change isn’t needed.> Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41379 +/- ##
===========================================
- Coverage 68.21% 68.18% -0.04%
===========================================
Files 3968 3968
Lines 154740 154740
Branches 27809 27821 +12
===========================================
- Hits 105562 105502 -60
- Misses 44387 44449 +62
+ Partials 4791 4789 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Upgrades
xml-cryptofrom~3.2.1to~6.1.2(the only usage site isapps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts). The 3.x line is EOL; 6.x is maintained and has stronger anti-signature-wrapping protections. Follow-up to CORE-2357.Issue(s)
Closes: https://rocketchat.atlassian.net/browse/CORE-2374
Steps to test or reproduce
signatureValidationTypeset toResponse,Assertion,All, andEither.Further comments
CORE-2374
Summary by CodeRabbit