chore(deps): bump several dependencies (elliptic, pbkdf2, fast-xml-parser etc) and replace twit#40294
chore(deps): bump several dependencies (elliptic, pbkdf2, fast-xml-parser etc) and replace twit#40294
Conversation
…rser etc) and replace twit
|
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 |
|
|
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)
📜 Recent 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). (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-04-23T18:10:55.887ZApplied to files:
📚 Learning: 2026-04-20T17:11:59.452ZApplied to files:
🪛 Biome (2.4.12)apps/meteor/app/lib/server/oauth/twitter.js[error] 2-2: Illegal use of an import declaration outside of a module (parse) [error] 3-3: Illegal use of an import declaration outside of a module (parse) [error] 5-5: Illegal use of an import declaration outside of a module (parse) 🔇 Additional comments (1)
WalkthroughThe Twitter OAuth integration is migrated from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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 (2)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/lib/server/oauth/twitter.js`:
- Around line 16-22: The try/catch in getIdentity is ineffective because
client.v1.verifyCredentials is returned without awaiting it; change the code in
getIdentity to await client.v1.verifyCredentials(...) inside the try so
rejections are caught, and when rethrowing wrap the error with a new
Error("Failed to fetch identity from Twitter. ...") that includes both
err.message and propagate the Twitter error payload by attaching err.data
(fallback to err.response if data is missing) to the thrown object so callers
get the wrapped message and the parsed Twitter error data.
In `@package.json`:
- Around line 42-45: The package override pins "browserify-sign/elliptic" and
"create-ecdh/elliptic" to elliptic@6.6.1 which is vulnerable to CVE-2025-14505;
update package.json to remove or replace these elliptic overrides and migrate
any code paths using the elliptic API (e.g., browserify-sign integration or
createECDH usage) to a maintained library such as noble-curves (or another
vetted ECDSA/ECDH implementation). Locate uses of elliptic-based APIs in your
codebase (areas that call ECDSA signing/verification or createECDH flows) and
refactor them to the chosen library’s APIs, update tests, and then update
package.json dependency entries (or lockfile overrides) to reference the new
library instead of "browserify-sign/elliptic" and "create-ecdh/elliptic". Ensure
all signature generation/verification and key derivation code paths are covered
by tests before removing the elliptic overrides.
- Around line 48-50: The package.json override for "fast-xml-parser@npm:5.3.6":
"^5.5.7" is resolving to 5.6.0 in the lockfile—confirm whether 5.6.0 is
acceptable and either tighten the override to a specific patch (e.g., 5.5.7) or
update it to explicitly allow 5.6.x; also update the accompanying note about API
compatibility to reflect that fast-xml-parser v5 keeps XMLParser
backward-compatible with v4 (the breaking change was splitting XMLBuilder into a
separate package in v5.4.0+), and mention that is-svg and webdav use XMLParser
so they do not require migration (refer to the override keys
"fast-xml-parser@npm:5.3.6": "^5.5.7", "is-svg/fast-xml-parser", and
"webdav/fast-xml-parser" when making the change).
🪄 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: b2719237-0aa1-4c3c-bad7-aa8faa2bd31d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
apps/meteor/app/lib/server/oauth/twitter.jsapps/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). (2)
- GitHub Check: 📦 Build Packages
- 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/lib/server/oauth/twitter.js
🧠 Learnings (5)
📓 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-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
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/package.json
📚 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: 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.
Applied to files:
package.json
📚 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
🪛 Biome (2.4.12)
apps/meteor/app/lib/server/oauth/twitter.js
[error] 2-2: Illegal use of an import declaration outside of a module
(parse)
[error] 3-3: Illegal use of an import declaration outside of a module
(parse)
[error] 5-5: Illegal use of an import declaration outside of a module
(parse)
🔇 Additional comments (2)
apps/meteor/package.json (1)
302-302: LGTM — dependency swap aligns with the OAuth implementation change.
twithas been deprecated/unmaintained for years; moving totwitter-api-v2is a good call. The addition matches the only remaining consumer atapps/meteor/app/lib/server/oauth/twitter.js.apps/meteor/app/lib/server/oauth/twitter.js (1)
9-15: Field mapping and return type usage are correct.The
accessSecretfield name is correct for theTwitterApiconstructor (mapping from theaccessTokenSecretparameter), andclient.v1.verifyCredentials()returns the user object directly as expected for the downstream usage at lines 44 and 51.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40294 +/- ##
===========================================
- Coverage 69.82% 69.80% -0.03%
===========================================
Files 3296 3296
Lines 119173 119173
Branches 21516 21454 -62
===========================================
- Hits 83213 83188 -25
- Misses 32662 32674 +12
- Partials 3298 3311 +13
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 |
…rser etc) and replace twit (#40294)
|
/backport 8.4.1 |
|
Pull request #40371 added to Project: "Patch 8.4.1" |
…rser etc) and replace twit (#40294)
|
/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 |
…rser etc) and replace twit (#40294)
|
/backport 8.3.3 |
|
Pull request #40386 added to Project: "Patch 8.3.3" |
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/SB-987
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores