feat: Custom OAuth using Passport#40203
Merged
Merged
Conversation
Contributor
|
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 |
|
Contributor
|
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 configurable Passport OAuth2 strategy with PKCE and identity extraction, Passport route registration, an async verify flow with account-merge hooks/transactional identity sync, changes Passport session serialization, updates type signatures, and adds the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Browser as ClientBrowser
participant AppServer as App/Passport
participant OAuthProvider as OAuthProvider
participant VerifyFn as VerifyFunction
participant Accounts as AccountsSystem
participant DB as UsersDB
Client->>Browser: Click "Login with {service}"
Browser->>AppServer: GET /oauth/{service}
AppServer->>OAuthProvider: Redirect to authorize (PKCE, config)
OAuthProvider-->>Browser: Redirect to provider login
Browser->>OAuthProvider: User authenticates
OAuthProvider-->>Browser: Redirect back with code
Browser->>AppServer: GET /oauth/{service}/callback?code=...
AppServer->>OAuthProvider: Exchange code for tokens
OAuthProvider-->>AppServer: accessToken, refreshToken
AppServer->>OAuthProvider: Fetch identity endpoint
OAuthProvider-->>AppServer: identity JSON
AppServer->>VerifyFn: verify(accessToken, refreshToken, profile)
VerifyFn->>Accounts: updateOrCreateUserFromExternalService(...)
Accounts->>DB: find/create/link user (transaction)
DB-->>Accounts: persisted user
Accounts-->>VerifyFn: result (userId)
VerifyFn->>DB: load full user
DB-->>VerifyFn: user object
VerifyFn-->>AppServer: done(null, user)
AppServer->>AppServer: generate stamped login token, insert
AppServer-->>Browser: Redirect /home?resumeToken=...
AppServer->>AppServer: destroy session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40203 +/- ##
===============================================================
+ Coverage 69.75% 69.79% +0.03%
===============================================================
Files 3297 3297
Lines 120970 121003 +33
Branches 21520 21562 +42
===============================================================
+ Hits 84381 84450 +69
+ Misses 33319 33285 -34
+ Partials 3270 3268 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
126-150:⚠️ Potential issue | 🟠 MajorAdd rate limiting and real error handling on the OAuth callback route.
Two gaps on the callback handler:
- Rate limiting (CodeQL). The handler issues authenticated login tokens and writes to the user document via
Accounts._insertLoginToken, but has no throttle. Pair it with the repo's rate-limiter middleware (the same pattern used elsewhere inapps/meteor/server), keyed per IP and/or perserviceKey.- Error handling.
failWithError: truecauses Passport tonext(err)on failures, butoAuthRouterhas no express error middleware, so users would see the default express error page rather than a graceful redirect. Add an error handler onoAuthRouter(or removefailWithErrorand rely onfailureRedirect).Also, line 135 contains a commented-out
return res.redirect('/login')and/noOauthUserappears to be a placeholder — please confirm that route exists (otherwise the redirect will 404).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts` around lines 126 - 150, Wrap the OAuth callback route registered with oAuthRouter.get and passport.authenticate(serviceKey, { failureRedirect: '/login', failureFlash: true, failWithError: true }) with the repo's rate-limiter middleware (same pattern used elsewhere in apps/meteor/server) keyed by IP and serviceKey to throttle calls before calling Accounts._insertLoginToken; ensure the limiter is applied to the route registration. Add an Express error handler on oAuthRouter (or remove failWithError from the passport.authenticate options) to catch passport errors and perform a graceful redirect (e.g., back to /login) instead of exposing the default error page. Replace the placeholder /noOauthUser redirect with a valid route (or restore the commented return res.redirect('/login')) so missing oAuthUser paths do not 404. Ensure session destruction is handled after successful token insertion but before final redirect or properly awaited in the flow so errors are logged via console.error.
🤖 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/custom-oauth/server/customOAuth.ts`:
- Line 235: The error message string in the return statement that constructs new
Error contains an extra closing brace in the template literal; update the return
in the function/method where it calls done(new Error(`Failed to fetch identity
from ${this.name} at ${this.identityPath}}`)) (references this.name and
this.identityPath) to remove the stray '}' so the template literal reads
...${this.identityPath}) and ensure the surrounding punctuation remains correct.
- Around line 91-97: The code contains a duplicated assignment block that sets
this.identityTokenSentVia based on this.tokenSentVia in customOAuth.ts; remove
the redundant second block so the logic appears only once (keep the first
occurrence that checks if this.identityTokenSentVia == null ||
this.identityTokenSentVia === 'default' and assigns this.tokenSentVia). Ensure
only one such check/assignment for identityTokenSentVia remains in the Custom
OAuth initialization code.
- Around line 82-85: The constructor currently calls .trim() unconditionally on
config.usernameField, config.emailField, config.nameField, and
config.avatarField which throws if any are undefined; update the initialization
in customOAuth.ts to guard each field before trimming (e.g., use conditional
checks or nullish/optional chaining and a safe default) so usernameField,
emailField, nameField, and avatarField are set to a trimmed value only when
present and a safe default otherwise; locate the assignments to
this.usernameField, this.emailField, this.nameField, and this.avatarField in the
Custom OAuth strategy constructor and replace the direct .trim() calls with
guarded trimming logic.
- Line 72: Remove the stray console.log debug prints and replace them with the
existing logger.debug (or remove entirely) while ensuring secrets/PII are
redacted: eliminate the console.log('strategy options', options) and either log
a sanitized options summary (omit/redact clientSecret, clientID, tokens) where
the strategy is initialized, replace console.log('normalizeIdentity', identity)
with a logger.debug that only includes non-sensitive identity fields or a
redacted marker inside the normalizeIdentity function, replace
console.log('afterRename', afterRename) with a redacted logger.debug in the
rename/afterRename handling, and remove/replace the console.log('error fetching
identity from', ...) with logger.debug/logger.error that omits full profile
payloads; use the variable names options, identity, and afterRename to locate
the sites to change.
- Around line 362-393: The patch replaces
Accounts.updateOrCreateUserFromExternalService at module load and can stack
wrappers and crash if updateOrCreateUserFromExternalService returns undefined;
fix by (1) guarding idempotency: only wrap
Accounts.updateOrCreateUserFromExternalService if it hasn’t been wrapped yet
(use a WeakSet or attach a non-enumerable sentinel/property to the function you
capture as updateOrCreateUserFromExternalService and check it before
reassigning) so BeforeUpdateOrCreateUserFromExternalService hooks aren’t applied
multiple times, (2) add a null check for the returned value from
updateOrCreateUserFromExternalService (verify user is truthy before accessing
user.userId) and return undefined early if missing, and (3) add a brief comment
near the wrapper explaining that this module must be imported early by consumers
that snapshot Accounts.updateOrCreateUserFromExternalService so callers see the
patched behavior (reference symbols:
Accounts.updateOrCreateUserFromExternalService,
updateOrCreateUserFromExternalService,
BeforeUpdateOrCreateUserFromExternalService, Users.findOneById,
LDAP.loginAuthenticatedUserRequest, callbacks.run('afterValidateNewOAuthUser')).
- Around line 133-173: The thrown Errors in getUsername, getEmail,
getCustomName, and getAvatarUrl currently pass error?.message as a second
positional argument which is ignored; update each catch to preserve the original
error as the cause by using the Error options object (e.g., throw new
Error('CustomOAuth: Failed to extract email', { cause: error })) or wrap with a
new Error and set { cause: error } so the original exception chain is retained
(apply the same change in getUsername, getEmail, getCustomName, and
getAvatarUrl).
- Around line 1-16: The file is missing imports for Match and Meteor which are
referenced via Match.test(config.serverURL, String) and new Meteor.Error(...) in
the constructor and other places; add imports for Match from 'meteor/check' and
Meteor from 'meteor/meteor' at the top of customOAuth.ts so Match.test and
Meteor.Error are defined when the Strategy/constructor, saveUserIdentity calls,
and other Meteor.Error usages are executed.
- Around line 248-359: The addHookToProcessUser() method registers the OAuth
merge hook and Accounts.validateNewUser but is never invoked; call
this.addHookToProcessUser() from the class constructor (after all relevant
instance properties like name, keyField, usernameField, emailField, nameField,
mergeUsers, mergeUsersDistinctServices are initialized) so the
BeforeUpdateOrCreateUserFromExternalService hook and Accounts.validateNewUser
validator are actually registered during instantiation.
In `@apps/meteor/server/configuration/configurePassport.ts`:
- Around line 36-45: Remove the console.log and stop serializing the full user
object: in passport.serializeUser only store the user's unique id (e.g.,
user._id) into the session instead of the entire IUser; in
passport.deserializeUser accept that id and perform a fresh DB lookup (e.g.,
Users.findOneById(id) or equivalent) to rehydrate the user so changes (roles,
tokens, deactivation) are reflected, and ensure decode returns the found user or
an error if not found; reference passport.serializeUser,
passport.deserializeUser, IUser, and Users.findOneById (or your Users lookup
function) when making the change.
In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts`:
- Line 142: The redirect currently appends the bearer login token
(stampedToken.token produced by Accounts._insertLoginToken) into the URL which
leaks credentials; instead set the token in a Secure, HttpOnly, SameSite=Lax
cookie (or deliver via a short self-submitting POST) and then redirect to /home
without the resumeToken query param. Locate the redirect call that uses
stampedToken.token and replace it with logic that writes the token into a cookie
(Secure, HttpOnly, SameSite=Lax, appropriate path and expiry) on the response,
and then call res.redirect('/home') so the token is never exposed in the URL or
referer. Ensure any client-side code that previously read resumeToken from the
URL is updated to rely on the cookie/session.
- Around line 130-131: Remove the two debug console.log statements (the calls to
console.log('req -> user', req.user) and console.log('YAY!!!')) from the OAuth
callback handling in updateOAuthServices (where req.user is available); if
observability is needed, replace them with the existing logger at an appropriate
level (e.g., logger.debug/info) and log only non-sensitive, minimal identifiers
(e.g., user._id or a correlation id), never the full user document or tokens.
Ensure no user service, email, or token fields are printed.
- Around line 107-119: Remove the deprecated CustomOAuth instantiation so the
new Passport-based CustomOAuthStrategy is the only registration path: delete the
line that calls new CustomOAuth(serviceKey, config) (which causes
Accounts.oauth.registerService and hook/token registration side-effects),
leaving passport.unuse(serviceKey) and passport.use(...) with
CustomOAuthStrategy (verifyFunction, serviceKey, config) intact to avoid
duplicate hook/service registration.
In `@apps/meteor/server/lib/oauth/verifyFunction.ts`:
- Around line 18-29: The service-data object passed to
Accounts.updateOrCreateUserFromExternalService currently spreads
provider-controlled restProfile and _json after the sanitized fields
(accessToken, refreshToken, name, email), allowing provider payloads to
overwrite those keys; in verifyFunction.ts, change the construction so provider
data is spread first and then explicitly set the controlled fields (accessToken,
refreshToken, name, email, id/username if used by customOAuth.ts) last, or
alternatively build the object by copying only safe keys from restProfile/_json
and then assign the canonical controlled fields (ensuring serviceData.id,
serviceData.email, serviceData.username are set explicitly) so provider-supplied
keys cannot override them.
- Around line 15-17: Remove the dead commented placeholder and the stale ESLint
suppression in verifyFunction.ts: delete the commented-out "// if
(profile.username) {}" and the "// eslint-disable-next-line
`@typescript-eslint/await-thenable`" line, since
Accounts.updateOrCreateUserFromExternalService is monkey-patched to be async
(see custom OAuth patch of Accounts.updateOrCreateUserFromExternalService), so
the await is valid and no suppression or placeholder comment is needed.
In `@package.json`:
- Around line 58-59: Remove the "passport-oauth2" dependency from the root
package.json and add the same dependency entry ("passport-oauth2": "^1.8.0") to
the meteor app's package.json so all passport-related modules (passport,
passport-facebook, passport-facebook2, passport-github2, passport-oauth2) live
together; update the root package.json dependency list to no longer include
passport-oauth2 and run the package install command in the meteor app directory
to ensure it is installed there.
---
Duplicate comments:
In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts`:
- Around line 126-150: Wrap the OAuth callback route registered with
oAuthRouter.get and passport.authenticate(serviceKey, { failureRedirect:
'/login', failureFlash: true, failWithError: true }) with the repo's
rate-limiter middleware (same pattern used elsewhere in apps/meteor/server)
keyed by IP and serviceKey to throttle calls before calling
Accounts._insertLoginToken; ensure the limiter is applied to the route
registration. Add an Express error handler on oAuthRouter (or remove
failWithError from the passport.authenticate options) to catch passport errors
and perform a graceful redirect (e.g., back to /login) instead of exposing the
default error page. Replace the placeholder /noOauthUser redirect with a valid
route (or restore the commented return res.redirect('/login')) so missing
oAuthUser paths do not 404. Ensure session destruction is handled after
successful token insertion but before final redirect or properly awaited in the
flow so errors are logged via console.error.
🪄 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: 915cbe28-ef4d-42e1-8aac-6d8482549a3d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
apps/meteor/app/custom-oauth/server/customOAuth.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tspackage.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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/oauth/getOAuthServices.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/app/custom-oauth/server/customOAuth.ts
🧠 Learnings (9)
📓 Common learnings
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: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39492
File: apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts:22-24
Timestamp: 2026-03-09T23:46:52.173Z
Learning: In `apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts`, the `oAuth2ServerAuth` function's `authorization` field in `partialRequest` is exclusively expected to carry Bearer tokens. Basic authentication is not supported in this OAuth flow, so there is no need to guard against non-Bearer schemes when extracting the token from the `Authorization` header.
📚 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/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/updateOAuthServices.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 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/oauth/getOAuthServices.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/app/custom-oauth/server/customOAuth.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/oauth/getOAuthServices.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/app/custom-oauth/server/customOAuth.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/custom-oauth/server/customOAuth.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/custom-oauth/server/customOAuth.ts
📚 Learning: 2026-03-09T23:46:52.173Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39492
File: apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts:22-24
Timestamp: 2026-03-09T23:46:52.173Z
Learning: In `apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts`, the `oAuth2ServerAuth` function's `authorization` field in `partialRequest` is exclusively expected to carry Bearer tokens. Basic authentication is not supported in this OAuth flow, so there is no need to guard against non-Bearer schemes when extracting the token from the `Authorization` header.
Applied to files:
apps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/app/custom-oauth/server/customOAuth.ts
📚 Learning: 2026-03-14T14:58:58.834Z
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.
Applied to files:
apps/meteor/server/lib/oauth/updateOAuthServices.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/server/lib/oauth/updateOAuthServices.ts
🪛 GitHub Check: CodeQL
apps/meteor/server/lib/oauth/updateOAuthServices.ts
[failure] 129-149: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (1)
apps/meteor/server/lib/oauth/getOAuthServices.ts (1)
19-19: Whitespace-only change.No functional impact.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
12 issues found across 7 files
You’re at about 91% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/lib/oauth/verifyFunction.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/verifyFunction.ts:26">
P1: Spreading `..._json` last overwrites the explicitly set `accessToken`, `refreshToken`, `name`, and `email` fields if the raw OAuth provider response contains any of those keys. Move `..._json` before the explicit assignments, or selectively pick only the fields you need from `_json`.</violation>
</file>
<file name="apps/meteor/app/custom-oauth/server/customOAuth.ts">
<violation number="1" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:6">
P0: Missing imports for `Match` and `Meteor`. The constructor uses `Match.test(config.serverURL, String)` at line 57 and creates `Meteor.Error` instances at multiple locations, but neither `Match` (from `meteor/check`) nor `Meteor` (from `meteor/meteor`) is imported. This will throw `ReferenceError` at runtime when the strategy is instantiated.</violation>
<violation number="2" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:72">
P1: `console.log('strategy options', options)` prints the `clientSecret` in plaintext to stdout on every strategy instantiation. Remove or use the existing `logger` at debug level with secret redaction.</violation>
<violation number="3" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:121">
P0: `addHookToProcessUser()` is defined but never invoked — OAuth user merge/linking and custom field mapping (username, email, name overrides via `Accounts.validateNewUser`) will not work. Call `this.addHookToProcessUser()` from the constructor.</violation>
<violation number="4" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:133">
P2: The `Error` constructor's second argument must be an options object `{ cause }`, not a string. The original error information is silently lost here.</violation>
<violation number="5" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:190">
P2: Debug `console.log` statements left in production code. These bypass the project's `Logger` (already instantiated) and leak user identity data to stdout. Replace with `logger.debug(...)` or remove.</violation>
<violation number="6" location="apps/meteor/app/custom-oauth/server/customOAuth.ts:235">
P3: Typo: extra `}` in the error message template — `${this.identityPath}}` should be `${this.identityPath}`.</violation>
</file>
<file name="apps/meteor/server/lib/oauth/updateOAuthServices.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/updateOAuthServices.ts:121">
P1: Route handlers accumulate on every settings change. `oAuthRouter.get(...)` always appends — Express has no route-replacement API. After the second settings change, two handlers fire for the same path, and the second one crashes with `ERR_HTTP_HEADERS_SENT` because the first already redirected.
Consider registering routes once and delegating to a mutable strategy reference, or clearing the router's route stack before re-registering.</violation>
<violation number="2" location="apps/meteor/server/lib/oauth/updateOAuthServices.ts:130">
P1: Debug `console.log` statements leak the full OAuth user object (including `_id` and potentially tokens) to stdout. Remove these before merging.</violation>
<violation number="3" location="apps/meteor/server/lib/oauth/updateOAuthServices.ts:142">
P1: The `resumeToken` (a full login credential) is passed as a URL query parameter, where it persists in browser history, server access logs, proxy logs, and `Referer` headers. Consider transmitting it via an HTTP-only cookie or a short-lived intermediary code that the client exchanges for the token.</violation>
</file>
<file name="apps/meteor/server/configuration/configurePassport.ts">
<violation number="1" location="apps/meteor/server/configuration/configurePassport.ts:37">
P1: Debug `console.log` left in production code. This will log the full user object (potentially including password hashes, tokens, and PII) to stdout on every authentication. Remove before merging.</violation>
<violation number="2" location="apps/meteor/server/configuration/configurePassport.ts:38">
P1: Storing the entire user object in the session instead of just the ID is a security and correctness concern. Sensitive fields (password hashes, auth tokens) will be persisted in session storage, and the session data will become stale if the user record is updated. Prefer serializing only `user._id` and deserializing via a DB lookup (the original pattern), or at minimum strip sensitive fields before serialization.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| if ((res && res.statusCode !== 200) || !body) { | ||
| return done(new Error(`Failed to fetch identity from ${this.name} at ${this.identityPath}}`)); |
Contributor
There was a problem hiding this comment.
P3: Typo: extra } in the error message template — ${this.identityPath}} should be ${this.identityPath}.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/custom-oauth/server/customOAuth.ts, line 235:
<comment>Typo: extra `}` in the error message template — `${this.identityPath}}` should be `${this.identityPath}`.</comment>
<file context>
@@ -0,0 +1,393 @@
+ }
+
+ if ((res && res.statusCode !== 200) || !body) {
+ return done(new Error(`Failed to fetch identity from ${this.name} at ${this.identityPath}}`));
+ }
+
</file context>
KevLehman
requested changes
Apr 17, 2026
Comment on lines
+42
to
43
| // const user = await Users.findOneById(id as string); | ||
| // we don’t actually use this user later |
Member
There was a problem hiding this comment.
Suggested change
| // const user = await Users.findOneById(id as string); | |
| // we don’t actually use this user later |
Member
jessicaschelly
left a comment
jessicaschelly
left a comment
There was a problem hiding this comment.
can you add a description, and also, if possible, include some test coverage? maybe some unit for verifyFunction and the profile mapping
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
34-135:⚠️ Potential issue | 🔴 CriticalFix the enabled/disabled branching before it removes active OAuth services.
For
value === trueand a non-custom provider, execution falls into the nestedelseat Line 127 and removes the service. Forvalue !== true, no removal runs at all, so disabled services remain configured. The removal branch should belong to the outervaluecheck, whilecreateOrUpdateServiceshould run for all enabled providers.Restructure the control flow
- if (value === true) { + if (value !== true) { + const service = await LoginServiceConfiguration.findOneByService(serviceName, { projection: { _id: 1 } }); + if (service?._id) { + const { deletedCount } = await LoginServiceConfiguration.removeService(service._id); + if (deletedCount > 0) { + void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); + } + } + continue; + } + const data: Partial<ILoginServiceConfiguration & Omit<OAuthConfiguration, '_id'>> = { clientId: settings.get(`${key}_id`), secret: settings.get(`${key}_secret`), }; if (/Accounts_OAuth_Custom-/.test(key)) { data.custom = true; data.clientId = settings.get(`${key}-id`); data.secret = settings.get(`${key}-secret`); data.serverURL = settings.get(`${key}-url`); data.tokenPath = settings.get(`${key}-token_path`); data.identityPath = settings.get(`${key}-identity_path`); data.authorizePath = settings.get(`${key}-authorize_path`); data.scope = settings.get(`${key}-scope`); data.accessTokenParam = settings.get(`${key}-access_token_param`); data.buttonLabelText = settings.get(`${key}-button_label_text`); data.buttonLabelColor = settings.get(`${key}-button_label_color`); data.loginStyle = settings.get(`${key}-login_style`); data.buttonColor = settings.get(`${key}-button_color`); data.tokenSentVia = settings.get(`${key}-token_sent_via`); data.identityTokenSentVia = settings.get(`${key}-identity_token_sent_via`); data.keyField = settings.get(`${key}-key_field`); data.usernameField = settings.get(`${key}-username_field`); data.emailField = settings.get(`${key}-email_field`); data.nameField = settings.get(`${key}-name_field`); data.avatarField = settings.get(`${key}-avatar_field`); data.rolesClaim = settings.get(`${key}-roles_claim`); data.groupsClaim = settings.get(`${key}-groups_claim`); data.channelsMap = settings.get(`${key}-groups_channel_map`); data.channelsAdmin = settings.get(`${key}-channels_admin`); data.mergeUsers = settings.get(`${key}-merge_users`); data.mergeUsersDistinctServices = settings.get(`${key}-merge_users_distinct_services`); data.mapChannels = settings.get(`${key}-map_channels`); data.mergeRoles = settings.get(`${key}-merge_roles`); data.rolesToSync = settings.get(`${key}-roles_to_sync`); data.showButton = settings.get(`${key}-show_button`); const config = { serverURL: data.serverURL, tokenPath: data.tokenPath, identityPath: data.identityPath, authorizePath: data.authorizePath, scope: data.scope, loginStyle: data.loginStyle, tokenSentVia: data.tokenSentVia, identityTokenSentVia: data.identityTokenSentVia, keyField: data.keyField, usernameField: data.usernameField, emailField: data.emailField, nameField: data.nameField, avatarField: data.avatarField, rolesClaim: data.rolesClaim, groupsClaim: data.groupsClaim, mapChannels: data.mapChannels, channelsMap: data.channelsMap, channelsAdmin: data.channelsAdmin, mergeUsers: data.mergeUsers, mergeUsersDistinctServices: data.mergeUsersDistinctServices, mergeRoles: data.mergeRoles, rolesToSync: data.rolesToSync, accessTokenParam: data.accessTokenParam, showButton: data.showButton, clientSecret: data.secret, clientId: data.clientId, }; new CustomOAuth(serviceKey, config); addPassportCustomOAuth(serviceKey, config); - if (serviceName === 'Facebook') { - (data as FacebookOAuthConfiguration).appId = data.clientId as string; - delete data.clientId; - } - if (serviceName === 'Twitter') { - (data as TwitterOAuthConfiguration).consumerKey = data.clientId as string; - delete data.clientId; - } + } - if (serviceName === 'Linkedin') { - (data as LinkedinOAuthConfiguration).clientConfig = { - requestPermissions: ['openid', 'email', 'profile'], - }; - } + if (serviceName === 'Facebook') { + (data as FacebookOAuthConfiguration).appId = data.clientId as string; + delete data.clientId; + } + if (serviceName === 'Twitter') { + (data as TwitterOAuthConfiguration).consumerKey = data.clientId as string; + delete data.clientId; + } - if (serviceName === 'Nextcloud') { - data.buttonLabelText = settings.get('Accounts_OAuth_Nextcloud_button_label_text'); - data.buttonLabelColor = settings.get('Accounts_OAuth_Nextcloud_button_label_color'); - data.buttonColor = settings.get('Accounts_OAuth_Nextcloud_button_color'); - } + if (serviceName === 'Linkedin') { + (data as LinkedinOAuthConfiguration).clientConfig = { + requestPermissions: ['openid', 'email', 'profile'], + }; + } - await LoginServiceConfiguration.createOrUpdateService(serviceKey, data); - void notifyOnLoginServiceConfigurationChangedByService(serviceKey); - } else { - const service = await LoginServiceConfiguration.findOneByService(serviceName, { projection: { _id: 1 } }); - if (service?._id) { - const { deletedCount } = await LoginServiceConfiguration.removeService(service._id); - if (deletedCount > 0) { - void notifyOnLoginServiceConfigurationChanged({ _id: service._id }, 'removed'); - } - } + if (serviceName === 'Nextcloud') { + data.buttonLabelText = settings.get('Accounts_OAuth_Nextcloud_button_label_text'); + data.buttonLabelColor = settings.get('Accounts_OAuth_Nextcloud_button_label_color'); + data.buttonColor = settings.get('Accounts_OAuth_Nextcloud_button_color'); } - } + + await LoginServiceConfiguration.createOrUpdateService(serviceKey, data); + void notifyOnLoginServiceConfigurationChangedByService(serviceKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts` around lines 34 - 135, The code currently removes non-custom services inside the value === true branch; instead, ensure createOrUpdateService runs for any enabled provider and removals run only when disabled: move the removal logic (calls to LoginServiceConfiguration.findOneByService and LoginServiceConfiguration.removeService and notifyOnLoginServiceConfigurationChanged) into the outer branch for value !== true, and keep the creation path (including CustomOAuth, addPassportCustomOAuth, building config and await LoginServiceConfiguration.createOrUpdateService plus notifyOnLoginServiceConfigurationChangedByService) inside the value === true branch for both custom and non-custom providers; update handling for provider-specific tweaks (Facebook/Twitter/Linkedin/Nextcloud) to remain in the enabled path and reference LoginServiceConfiguration.createOrUpdateService, LoginServiceConfiguration.removeService, CustomOAuth, addPassportCustomOAuth, notifyOnLoginServiceConfigurationChanged, and notifyOnLoginServiceConfigurationChangedByService accordingly.apps/meteor/definition/externals/meteor/accounts-base.d.ts (1)
40-44:⚠️ Potential issue | 🔴 CriticalThe Apple login handler must await the async call to
Accounts.updateOrCreateUserFromExternalService.Line 32 in
apps/meteor/app/apple/server/loginHandler.tscalls this method withoutawait, but the function now returns aPromise. The handler then immediately checksresult.userId(line 35), which will always be undefined sinceresultis a Promise object rather than the resolved user data. This breaks Apple login authentication.Other call sites have been verified:
verifyFunction.ts,configureOAuthServices.ts, andoauth.jsall properly await or handle the Promise.Fix for Apple handler
- const result = Accounts.updateOrCreateUserFromExternalService('apple', serviceData, { profile }); + const result = await Accounts.updateOrCreateUserFromExternalService('apple', serviceData, { profile });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/definition/externals/meteor/accounts-base.d.ts` around lines 40 - 44, The Apple login handler calls Accounts.updateOrCreateUserFromExternalService (which now returns a Promise) without awaiting it, so replace the direct call with an awaited call and ensure the enclosing function (e.g., the handler in apps/meteor/app/apple/server/loginHandler.ts) is async if not already; specifically, change the call that assigns result from Accounts.updateOrCreateUserFromExternalService to use await so that subsequent checks like result.userId operate on the resolved object rather than a Promise.
♻️ Duplicate comments (8)
apps/meteor/app/custom-oauth/server/customOAuth.ts (4)
80-83:⚠️ Potential issue | 🟡 MinorGuard optional field trimming.
Custom OAuth field mappings can be unset; calling
.trim()directly on an undefined setting will fail strategy construction.Suggested safe defaults
- this.usernameField = config.usernameField.trim(); - this.emailField = config.emailField.trim(); - this.nameField = config.nameField.trim(); - this.avatarField = config.avatarField.trim(); + this.usernameField = config.usernameField?.trim() ?? ''; + this.emailField = config.emailField?.trim() ?? ''; + this.nameField = config.nameField?.trim() ?? ''; + this.avatarField = config.avatarField?.trim() ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/customOAuth.ts` around lines 80 - 83, The code calls .trim() on optional config fields (this.usernameField, this.emailField, this.nameField, this.avatarField) which will throw if the config values are undefined; update the constructor/initialization to guard each field from config (e.g. check config.usernameField, config.emailField, config.nameField, config.avatarField) and only call .trim() when the value is a string, otherwise assign a safe default (empty string or null) so strategy construction won't fail.
168-174:⚠️ Potential issue | 🟠 MajorDo not log the full provider identity payload.
datais the full IdP profile and can include emails, names, external IDs, and custom claims. Keep this log to the missing field name/service only, or redact the payload.Redact the debug log
- logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, data }); + logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, service: this.name });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/customOAuth.ts` around lines 168 - 174, The debug log in getAvatarUrl currently emits the full IdP profile via the data variable; change the logger.debug call to avoid logging the full payload by only including non-sensitive identifiers (e.g. the missing field name this.avatarField and the provider/service identifier like this.service) or redact the payload entirely (do not include data). Update the logger.debug invocation in getAvatarUrl to remove data from the structured log and only include avatarField (and an optional this.service) so the full identity/profile is never logged.
121-121:⚠️ Potential issue | 🟠 MajorMake hook registration idempotent.
Every
CustomOAuthStrategyconstruction callsaddHookToProcessUser(), which pushes another before-hook and registers anotherAccounts.validateNewUsercallback. Since settings refreshes can recreate strategies, OAuth logins can run the same merge/validation logic multiple times for one service.Also applies to: 247-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/customOAuth.ts` at line 121, The addHookToProcessUser() call in CustomOAuthStrategy is registering before-hooks and an Accounts.validateNewUser callback on every construction, causing duplicate callbacks when strategies are recreated; make registration idempotent by adding a guard (for example a static/class-level boolean or a symbol property on the strategy prototype) so addHookToProcessUser() only registers the before-hook and Accounts.validateNewUser callback once per runtime (check the flag at start of addHookToProcessUser(), set it after successful registration), and apply the same pattern to the other registration sites referenced (lines ~247-362) to prevent duplicate merge/validation executions.
1-9:⚠️ Potential issue | 🔴 CriticalImport
MatchandMeteorbefore using them.Line 57 uses
Match.test(...)and Line 58 throwsnew Meteor.Error(...), but neither symbol is imported in this module. That can fail TypeScript compilation or throw at runtime when the strategy is instantiated.Suggested imports
import { Accounts } from 'meteor/accounts-base'; +import { Match } from 'meteor/check'; +import { Meteor } from 'meteor/meteor'; import type { DoneCallback } from 'passport';Verify the missing imports:
#!/bin/bash # Description: Confirm Meteor/Match are referenced but not imported in this module. rg -nP '\b(Match|Meteor)\.' apps/meteor/app/custom-oauth/server/customOAuth.ts rg -nP "from 'meteor/(check|meteor)'" apps/meteor/app/custom-oauth/server/customOAuth.tsAlso applies to: 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/customOAuth.ts` around lines 1 - 9, The module uses Match.test(...) and new Meteor.Error(...) but does not import them; add the missing imports near the top of the file: import { Match } from 'meteor/check' and import { Meteor } from 'meteor/meteor' so calls to Match.test and constructing Meteor.Error in the OAuth strategy (the code around the Strategy/verify flow that uses Match and Meteor) compile and run correctly.apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts (1)
38-41:⚠️ Potential issue | 🔴 CriticalDo not expose the Meteor resume token in the URL.
stampedToken.tokengrants login and is being appended to/home?resumeToken=.... That leaks through common URL side channels. Use a server-side handoff/one-time code or a secure cookie and redirect without the credential.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts` around lines 38 - 41, The code is exposing a Meteor resume token in the URL via res.redirect(`/home?resumeToken=${stampedToken.token}`); instead of leaking the credential, change the handoff to a server-side mechanism: generate the stamped token with Accounts._generateStampedLoginToken and Accounts._insertLoginToken as before, but deliver it via a secure HttpOnly, SameSite cookie or issue a short-lived one-time code (store mapping server-side) and redirect without the token; update the redirect call (res.redirect) to omit stampedToken.token and implement server-side storage of the one-time code or set the cookie so the client can authenticate without the token appearing in the URL.apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
101-102:⚠️ Potential issue | 🟠 MajorAvoid registering the legacy and Passport custom OAuth paths together.
new CustomOAuth(...)has registration side effects, thenaddPassportCustomOAuth(...)registers the Passport strategy/routes for the same service. Keeping both paths active risks duplicate hooks and legacy callback execution; remove the legacy instantiation once the Passport strategy is the intended replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts` around lines 101 - 102, Remove the legacy registration by deleting the instantiation that causes side effects: remove the call to new CustomOAuth(serviceKey, config) where addPassportCustomOAuth(serviceKey, config) is intended to register the Passport-based strategy; keep addPassportCustomOAuth only so the serviceKey/config are registered once and no legacy hooks/callbacks are double-registered.apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
82-85:⚠️ Potential issue | 🔴 CriticalAvoid sending the login token in the redirect URL.
stampedToken.tokenis a bearer login credential; placing it in/home?resumeToken=...exposes it through access logs, browser history, bookmarks, andRefererheaders. Transfer this via a server-side handoff, one-time code, or secure cookie and redirect to/homewithout the token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 82 - 85, Do not put the bearer credential stampedToken.token into the redirect URL; after creating stampedToken via Accounts._generateStampedLoginToken and persisting it with Accounts._insertLoginToken, transfer it via a secure server-side channel (e.g., set a secure, HttpOnly, SameSite cookie containing stampedToken.token or store the token server-side and issue a short one-time code/key) and then call res.redirect('/home') without any resumeToken query param; update the code around Accounts._generateStampedLoginToken / Accounts._insertLoginToken and res.redirect to implement the cookie or one-time-code handoff and remove the token from the URL.apps/meteor/server/lib/oauth/verifyFunction.ts (1)
15-39:⚠️ Potential issue | 🔴 CriticalHarden the Passport verify path against rejected async work and IdP field overrides.
restProfile/_jsonare provider-controlled but currently overrideaccessToken,refreshToken,name, anddone. Put provider data first, canonical fields last, and wrap the body withtry/catch.Suggested hardening
export const verifyFunction = async ( accessToken: string, refreshToken: string, profile: Profile, done: DoneCallback, serviceName: string, ) => { - const profileWithRaw = profile as Profile & { _json?: Record<string, unknown>; _raw?: string }; - const { _json, _raw, ...restProfile } = profileWithRaw; - - // eslint-disable-next-line `@typescript-eslint/await-thenable` - const user = await Accounts.updateOrCreateUserFromExternalService( - serviceName, - { - accessToken, - refreshToken, - name: profile.displayName, - email: profile?.emails?.[0]?.value, - ...restProfile, - ..._json, - }, - {}, - ); - - if (!user?.userId || typeof user?.userId !== 'string') { - return done(new Error('User not found')); - } - - const userFromDB = await Users.findOneById(user.userId); - - if (!userFromDB) { - return done(new Error('User not found')); - } - - return done(null, userFromDB); + try { + const profileWithRaw = profile as Profile & { _json?: Record<string, unknown>; _raw?: string }; + const { _json, _raw, ...restProfile } = profileWithRaw; + + const user = await Accounts.updateOrCreateUserFromExternalService( + serviceName, + { + ...restProfile, + ..._json, + accessToken, + refreshToken, + name: profile.displayName, + email: profile?.emails?.[0]?.value, + }, + {}, + ); + + if (!user?.userId || typeof user?.userId !== 'string') { + return done(new Error('User not found')); + } + + const userFromDB = await Users.findOneById(user.userId); + + if (!userFromDB) { + return done(new Error('User not found')); + } + + return done(null, userFromDB); + } catch (error) { + return done(error as Error); + } };Verify Passport’s callback contract for the version in use:
Does passport-oauth2 await or catch rejected Promises returned from an async verify callback, or must errors be passed to the done callback?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/verifyFunction.ts` around lines 15 - 39, Wrap the verify routine in a try/catch and ensure all awaited calls never let a rejection escape—call done(err) on any caught error; when calling Accounts.updateOrCreateUserFromExternalService, place provider-controlled spreads (restProfile and _json) first and then pass canonical fields last (explicit accessToken, refreshToken, name: profile.displayName, email: profile?.emails?.[0]?.value) so those canonical values cannot be overridden by the IdP; after the call validate user?.userId is a string and then lookup Users.findOneById(user.userId) and if missing call done(new Error('User not found')), otherwise call done(null, userFromDB).
🤖 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/custom-oauth/server/customOAuth.ts`:
- Around line 247-264: The current guard in addHookToProcessUser uses if
(serviceData.username) which prevents email-keyed providers from reaching the
branch that checks this.keyField === 'email'; change the guard to check the
configured key field instead (e.g. if (this.keyField === 'username' ?
serviceData.username : serviceData.email)) so the merge lookup runs based on the
selected key; update the hook registered with
BeforeUpdateOrCreateUserFromExternalService and any local variables
(serviceData.username/serviceData.email) accordingly to avoid skipping
email-only providers when this.keyField === 'email'.
- Around line 61-76: The options passed into Strategy use raw config values so
authorizationURL/tokenURL can end up as serverURL + undefined or serverURL +
absolute URL; normalize config.tokenPath, config.authorizePath and
config.identityPath into absolute paths (apply default '/oauth/token' and '/me'
and resolve absolute vs relative against config.serverURL) before building the
StrategyOptions object, then construct options (authorizationURL/tokenURL) from
those normalized values so the internal OAuth2 client and later
this.tokenPath/this.identityPath remain consistent; update the constructor logic
around options, StrategyOptions, super(options, verify), this.tokenPath and
this.identityPath to use the normalized variables.
---
Outside diff comments:
In `@apps/meteor/definition/externals/meteor/accounts-base.d.ts`:
- Around line 40-44: The Apple login handler calls
Accounts.updateOrCreateUserFromExternalService (which now returns a Promise)
without awaiting it, so replace the direct call with an awaited call and ensure
the enclosing function (e.g., the handler in
apps/meteor/app/apple/server/loginHandler.ts) is async if not already;
specifically, change the call that assigns result from
Accounts.updateOrCreateUserFromExternalService to use await so that subsequent
checks like result.userId operate on the resolved object rather than a Promise.
In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts`:
- Around line 34-135: The code currently removes non-custom services inside the
value === true branch; instead, ensure createOrUpdateService runs for any
enabled provider and removals run only when disabled: move the removal logic
(calls to LoginServiceConfiguration.findOneByService and
LoginServiceConfiguration.removeService and
notifyOnLoginServiceConfigurationChanged) into the outer branch for value !==
true, and keep the creation path (including CustomOAuth, addPassportCustomOAuth,
building config and await LoginServiceConfiguration.createOrUpdateService plus
notifyOnLoginServiceConfigurationChangedByService) inside the value === true
branch for both custom and non-custom providers; update handling for
provider-specific tweaks (Facebook/Twitter/Linkedin/Nextcloud) to remain in the
enabled path and reference LoginServiceConfiguration.createOrUpdateService,
LoginServiceConfiguration.removeService, CustomOAuth, addPassportCustomOAuth,
notifyOnLoginServiceConfigurationChanged, and
notifyOnLoginServiceConfigurationChangedByService accordingly.
---
Duplicate comments:
In `@apps/meteor/app/custom-oauth/server/customOAuth.ts`:
- Around line 80-83: The code calls .trim() on optional config fields
(this.usernameField, this.emailField, this.nameField, this.avatarField) which
will throw if the config values are undefined; update the
constructor/initialization to guard each field from config (e.g. check
config.usernameField, config.emailField, config.nameField, config.avatarField)
and only call .trim() when the value is a string, otherwise assign a safe
default (empty string or null) so strategy construction won't fail.
- Around line 168-174: The debug log in getAvatarUrl currently emits the full
IdP profile via the data variable; change the logger.debug call to avoid logging
the full payload by only including non-sensitive identifiers (e.g. the missing
field name this.avatarField and the provider/service identifier like
this.service) or redact the payload entirely (do not include data). Update the
logger.debug invocation in getAvatarUrl to remove data from the structured log
and only include avatarField (and an optional this.service) so the full
identity/profile is never logged.
- Line 121: The addHookToProcessUser() call in CustomOAuthStrategy is
registering before-hooks and an Accounts.validateNewUser callback on every
construction, causing duplicate callbacks when strategies are recreated; make
registration idempotent by adding a guard (for example a static/class-level
boolean or a symbol property on the strategy prototype) so
addHookToProcessUser() only registers the before-hook and
Accounts.validateNewUser callback once per runtime (check the flag at start of
addHookToProcessUser(), set it after successful registration), and apply the
same pattern to the other registration sites referenced (lines ~247-362) to
prevent duplicate merge/validation executions.
- Around line 1-9: The module uses Match.test(...) and new Meteor.Error(...) but
does not import them; add the missing imports near the top of the file: import {
Match } from 'meteor/check' and import { Meteor } from 'meteor/meteor' so calls
to Match.test and constructing Meteor.Error in the OAuth strategy (the code
around the Strategy/verify flow that uses Match and Meteor) compile and run
correctly.
In `@apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts`:
- Around line 38-41: The code is exposing a Meteor resume token in the URL via
res.redirect(`/home?resumeToken=${stampedToken.token}`); instead of leaking the
credential, change the handoff to a server-side mechanism: generate the stamped
token with Accounts._generateStampedLoginToken and Accounts._insertLoginToken as
before, but deliver it via a secure HttpOnly, SameSite cookie or issue a
short-lived one-time code (store mapping server-side) and redirect without the
token; update the redirect call (res.redirect) to omit stampedToken.token and
implement server-side storage of the one-time code or set the cookie so the
client can authenticate without the token appearing in the URL.
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 82-85: Do not put the bearer credential stampedToken.token into
the redirect URL; after creating stampedToken via
Accounts._generateStampedLoginToken and persisting it with
Accounts._insertLoginToken, transfer it via a secure server-side channel (e.g.,
set a secure, HttpOnly, SameSite cookie containing stampedToken.token or store
the token server-side and issue a short one-time code/key) and then call
res.redirect('/home') without any resumeToken query param; update the code
around Accounts._generateStampedLoginToken / Accounts._insertLoginToken and
res.redirect to implement the cookie or one-time-code handoff and remove the
token from the URL.
In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts`:
- Around line 101-102: Remove the legacy registration by deleting the
instantiation that causes side effects: remove the call to new
CustomOAuth(serviceKey, config) where addPassportCustomOAuth(serviceKey, config)
is intended to register the Passport-based strategy; keep addPassportCustomOAuth
only so the serviceKey/config are registered once and no legacy hooks/callbacks
are double-registered.
In `@apps/meteor/server/lib/oauth/verifyFunction.ts`:
- Around line 15-39: Wrap the verify routine in a try/catch and ensure all
awaited calls never let a rejection escape—call done(err) on any caught error;
when calling Accounts.updateOrCreateUserFromExternalService, place
provider-controlled spreads (restProfile and _json) first and then pass
canonical fields last (explicit accessToken, refreshToken, name:
profile.displayName, email: profile?.emails?.[0]?.value) so those canonical
values cannot be overridden by the IdP; after the call validate user?.userId is
a string and then lookup Users.findOneById(user.userId) and if missing call
done(new Error('User not found')), otherwise call done(null, userFromDB).
🪄 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: aac3f6ec-7c71-448d-af7e-17f471894e07
📒 Files selected for processing (7)
apps/meteor/app/custom-oauth/server/customOAuth.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/addPassportCustomOAuth.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/configuration/configurePassport.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
73-75:⚠️ Potential issue | 🟡 MinorRemove the commented code and redirect to
/loginconsistently.Line 75 redirects to
/noOauthUser, which is not registered anywhere in the codebase. Bothpassport.authenticate()calls (lines 65, 69) are configured withfailureRedirect: '/login', and the parallel implementation inaddPassportCustomOAuth.tsredirects to/loginin the same scenario. The commented-out line at 74 should also be removed per coding guidelines.Suggested fix
if (!oAuthUser) { - // return res.redirect('/login'); - return res.redirect('/noOauthUser'); + return res.redirect('/login'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 73 - 75, In configureOAuthServices.ts, replace the stray redirect to '/noOauthUser' with a consistent redirect to '/login' and remove the commented-out return line; specifically update the block where it checks if (!oAuthUser) (the same failure case handled by passport.authenticate() and mirrored in addPassportCustomOAuth.ts) so it calls res.redirect('/login') and delete the commented code to match the rest of the OAuth failure handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 73-75: In configureOAuthServices.ts, replace the stray redirect to
'/noOauthUser' with a consistent redirect to '/login' and remove the
commented-out return line; specifically update the block where it checks if
(!oAuthUser) (the same failure case handled by passport.authenticate() and
mirrored in addPassportCustomOAuth.ts) so it calls res.redirect('/login') and
delete the commented code to match the rest of the OAuth failure handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 579d484c-8996-4ed0-805d-8a4f08c573ce
📒 Files selected for processing (1)
apps/meteor/server/lib/oauth/configureOAuthServices.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). (1)
- 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/oauth/configureOAuthServices.ts
🧠 Learnings (12)
📓 Common learnings
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: 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.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
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-03-09T23:46:52.173Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39492
File: apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts:22-24
Timestamp: 2026-03-09T23:46:52.173Z
Learning: In `apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts`, the `oAuth2ServerAuth` function's `authorization` field in `partialRequest` is exclusively expected to carry Bearer tokens. Basic authentication is not supported in this OAuth flow, so there is no need to guard against non-Bearer schemes when extracting the token from the `Authorization` header.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-03-14T14:58:58.834Z
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.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 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/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 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/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.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 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/oauth/configureOAuthServices.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/oauth/configureOAuthServices.ts
KevLehman
requested changes
Apr 23, 2026
tassoevan
approved these changes
May 15, 2026
tassoevan
merged commit 500e008
into
feat/phishing-resistant-mfa
68 of 84 checks passed
Member
|
Merged despite failed PR checks to unblock fixes in the feature branch. |
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
PRM-12
Summary by CodeRabbit
New Features
Bug Fixes
Chores