feat: Configure Passport#39604
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
|
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)
🚧 Files skipped from review as they are similar to previous changes (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). (3)
WalkthroughAdds Passport-based OAuth to the Meteor app: new runtime deps, session and Passport initialization, dynamic OAuth provider discovery/configuration, route handlers for OAuth flows, and a small IUser type addition. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Express as Express/Passport
participant OAuth as OAuth Provider
participant Meteor as Meteor App
participant DB as User Database
Client->>Express: GET /oauth/{provider}
Express->>OAuth: Redirect to provider (passport.authenticate)
OAuth->>Client: Login & consent
Client->>Express: GET /oauth/{provider}/callback?code=...
Express->>OAuth: Exchange code for token/profile
OAuth->>Express: Return profile & tokens
Express->>Meteor: Accounts.updateOrCreateUserFromExternalService(...)
Meteor->>DB: Create/Update user
DB->>Meteor: Return user doc
Meteor->>Express: Return user object
Express->>Express: Generate stamped login token
Express->>DB: Insert login token into user record
Express->>Client: Redirect /home?resumeToken={token}
Express->>Express: Destroy session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
coderabbitai
Bot
added
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 9 files
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/configuration/configurePassport.ts">
<violation number="1" location="apps/meteor/server/configuration/configurePassport.ts:18">
P1: The session secret is generated randomly at module load time. In a multi-instance deployment, each instance will have a different secret, breaking OAuth sessions when requests are routed to a different instance. On restart, all in-flight OAuth flows are also invalidated. The secret should be derived from a persistent, shared source (e.g., an environment variable or a database-stored value).</violation>
</file>
<file name="apps/meteor/server/configuration/index.ts">
<violation number="1" location="apps/meteor/server/configuration/index.ts:32">
P1: This enables Passport initialization that re-registers the same OAuth routes on every matching setting event, so handlers will accumulate and execute multiple times.</violation>
</file>
<file name="apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts:10">
P2: Narrow `services` to known providers instead of casting arbitrary strings to `Provider`; otherwise an unsupported service can flow through as `strategy: undefined` and break passport setup at runtime.
(Based on your team's feedback about avoiding unsafe type casts.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/server/lib/oauth/configureOAuthServices.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:30">
P1: `console.log` exposes `accessToken` and `refreshToken` in plaintext. Even for debugging, tokens should not be logged. Remove this statement (along with the other `console.log` calls on L47 and L69) before merging.</violation>
<violation number="2" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:68">
P1: `userFromDB` is not null-checked after `findOneById`. If the lookup returns `null`, `userFromDB?._id as string` passes `undefined` to `_insertLoginToken`. Add a guard similar to the `user?.userId` check above.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.) [FEEDBACK_USED]</violation>
<violation number="3" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:74">
P0: Hardcoded `http://localhost:3000` will break in any deployed environment. Use `Meteor.absoluteUrl()` (already used on L25 for the callback URL) to build the redirect target.</violation>
</file>
<file name="apps/meteor/server/lib/oauth/strategiesMap.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/strategiesMap.ts:5">
P1: This registry drops the existing `meteor-developer` provider, so `Accounts_OAuth_Meteor` will now be silently skipped during OAuth service discovery.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 5
🧹 Nitpick comments (4)
apps/meteor/server/configuration/configurePassport.ts (1)
24-24: Remove inline implementation comments from this module.Please move these notes to tests/docs/issue tracker and keep runtime code comment-free.
As per coding guidelines,
Avoid code comments in the implementation.Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/configuration/configurePassport.ts` at line 24, Remove inline implementation comments from the configurePassport module: locate the session configuration object inside the configurePassport function (the property "maxAge: 5 * 60 * 1000") and any other inline notes (including the one around line 39) and delete those trailing comments; move the explanatory notes into a test, documentation file, or issue tracker instead so the runtime code contains no implementation comments.apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
53-54: Remove TODO comments from implementation code.Track these follow-ups in issues/docs and keep runtime code clean.
As per coding guidelines,
Avoid code comments in the implementation.🤖 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 53 - 54, Remove the inline TODO comments from runtime code in configureOAuthServices.ts (the TODOs near the service data/types comments) and instead create tracked follow-ups in your issue tracker or documentation; update or create issues that describe the required refactor for "service data" and adding "types" and reference those issue IDs in code only if needed (e.g., a single short comment like "// tracked: ISSUE-1234") or in the PR description, then commit the cleaned file so runtime/implementation code no longer contains TODOs.apps/meteor/package.json (1)
261-263: Remove unusedpassport-facebook2dependency.
passport-facebook2is declared inpackage.jsonbut is not imported or used anywhere in the codebase. Removing it reduces the dependency surface and maintenance overhead."passport-facebook": "^3.0.0", - "passport-facebook2": "^1.0.3", "passport-github2": "^0.1.12",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/package.json` around lines 261 - 263, Remove the unused dependency "passport-facebook2" from package.json (delete the "passport-facebook2": "^1.0.3" entry), then update the lockfile by running the project's package manager install (npm install or yarn install) to regenerate package-lock.json / yarn.lock; also run a quick repo-wide search for "passport-facebook2" to confirm there are no leftover imports/usages before committing the change.apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts (1)
6-12: Tightenservicesparameter typing to avoid unsafe provider casts.Currently
services: string[]withstrategyMap[service as Provider]hides invalid inputs. Change toProvider[]to make access type-safe by contract. Note: this requires also typinggetOAuthServicesto returnProvider[]explicitly, since it currently has no return type annotation. The function already validates at runtime (lines 25-28) that services exist instrategyMap, so the change is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts` around lines 6 - 12, Change the services parameter of createOAuthServiceConfig from string[] to Provider[] and propagate the stronger typing by updating getOAuthServices to return Provider[] so callers and the strategyMap lookup are type-safe; update any call sites if needed to ensure they produce Provider values, relying on the existing runtime validation (strategyMap checks) to remain intact, and reference the Provider type wherever services is declared so the implicit cast (strategyMap[service as Provider]) can be removed inside createOAuthServiceConfig.
🤖 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/server/configuration/configurePassport.ts`:
- Around line 33-40: serializeUser stores user.providerId but deserializeUser
calls Users.findOneById(id), causing a mismatch; change one side to use the same
identifier. Either serialize the internal _id (e.g., in passport.serializeUser
return user._id) or update passport.deserializeUser to query by providerId
(e.g., Users.findOneByProviderId(id as string)); update the code paths
referencing serializeUser/deserializeUser so they use the same key and ensure
Users.findOneById or Users.findOneByProviderId is used accordingly.
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Line 30: Remove plaintext logging of sensitive OAuth payloads in
configureOAuthServices.ts: specifically eliminate console.log statements that
print accessToken, refreshToken, and profile (and any similar logs at the other
locations). Replace them with minimal, redacted operational logs that do not
include tokens or PII — for example log the provider name and a non-sensitive
identifier (e.g., provider and a hashed/masked profile id) or simply log
success/failure of the OAuth step. Update any occurrences referencing
accessToken, refreshToken, or profile to use the redacted/success-only message
to avoid exposing secrets.
- Line 74: Replace the hardcoded redirect URL with an environment-aware
construction: build the redirect using Meteor.absoluteUrl (or
process.env.ROOT_URL fallback) and append the path and stampedToken.token
instead of hardcoding "http://localhost:3000"; update the res.redirect call that
currently uses
res.redirect(`http://localhost:3000/home?resumeToken=${stampedToken.token}`) to
use e.g.
res.redirect(`${Meteor.absoluteUrl('home')}?resumeToken=${stampedToken.token}`)
or `${process.env.ROOT_URL ||
'http://localhost:3000'}home?resumeToken=${stampedToken.token}` so callbacks
work in non-local environments.
- Around line 68-73: The code currently calls
Accounts._generateStampedLoginToken and Accounts._insertLoginToken without
verifying userFromDB exists; guard the result of Users.findOneById(user?.userId)
(the userFromDB variable) before generating or inserting tokens, e.g., if
userFromDB is null/undefined log an error (include user?.userId) and
abort/return or throw, otherwise proceed to call
Accounts._generateStampedLoginToken and Accounts._insertLoginToken with
userFromDB._id; ensure no token APIs are invoked when userFromDB is falsy.
- Around line 16-43: The configureOAuthServices function re-registers passport
strategies and oAuthRouter routes every time it runs; before calling
passport.use(...) and oAuthRouter.get(...) detect and remove existing
registrations for the same provider to avoid duplicates. Concretely, for each
config.provider check for an existing strategy (e.g.
passport._strategies[config.provider] or use passport.unuse(config.provider) if
available) and remove/unregister it prior to passport.use(...), and likewise
prune existing oAuthRouter route handlers whose path matches
`/oauth/${config.provider}` and `/oauth/${config.provider}/callback` from the
router stack (e.g. filter oAuthRouter.stack or router._router.stack entries)
before adding new oAuthRouter.get(...) handlers so reconfiguration replaces
instead of appends registrations.
---
Nitpick comments:
In `@apps/meteor/package.json`:
- Around line 261-263: Remove the unused dependency "passport-facebook2" from
package.json (delete the "passport-facebook2": "^1.0.3" entry), then update the
lockfile by running the project's package manager install (npm install or yarn
install) to regenerate package-lock.json / yarn.lock; also run a quick repo-wide
search for "passport-facebook2" to confirm there are no leftover imports/usages
before committing the change.
In `@apps/meteor/server/configuration/configurePassport.ts`:
- Line 24: Remove inline implementation comments from the configurePassport
module: locate the session configuration object inside the configurePassport
function (the property "maxAge: 5 * 60 * 1000") and any other inline notes
(including the one around line 39) and delete those trailing comments; move the
explanatory notes into a test, documentation file, or issue tracker instead so
the runtime code contains no implementation comments.
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 53-54: Remove the inline TODO comments from runtime code in
configureOAuthServices.ts (the TODOs near the service data/types comments) and
instead create tracked follow-ups in your issue tracker or documentation; update
or create issues that describe the required refactor for "service data" and
adding "types" and reference those issue IDs in code only if needed (e.g., a
single short comment like "// tracked: ISSUE-1234") or in the PR description,
then commit the cleaned file so runtime/implementation code no longer contains
TODOs.
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts`:
- Around line 6-12: Change the services parameter of createOAuthServiceConfig
from string[] to Provider[] and propagate the stronger typing by updating
getOAuthServices to return Provider[] so callers and the strategyMap lookup are
type-safe; update any call sites if needed to ensure they produce Provider
values, relying on the existing runtime validation (strategyMap checks) to
remain intact, and reference the Provider type wherever services is declared so
the implicit cast (strategyMap[service as Provider]) can be removed inside
createOAuthServiceConfig.
🪄 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: f47a6b17-a9ee-48bf-9356-7664f3278d87
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/meteor/package.jsonapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/strategiesMap.tspackages/core-typings/src/IUser.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-typings/src/IUser.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/strategiesMap.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
🧠 Learnings (6)
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
packages/core-typings/src/IUser.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:
packages/core-typings/src/IUser.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/strategiesMap.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.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:
packages/core-typings/src/IUser.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/strategiesMap.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.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/getOAuthServices.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.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.tsapps/meteor/package.json
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/configuration/index.ts
🪛 Biome (2.4.6)
apps/meteor/server/lib/oauth/getOAuthServices.ts
[error] 10-10: This callback passed to map() iterable method should always return a value.
(lint/suspicious/useIterableCallbackReturn)
🔇 Additional comments (3)
packages/core-typings/src/IUser.ts (1)
237-237: Good backward-compatible typing update.Adding
providerIdas optional keeps existingIUserusages safe while enabling OAuth identity linkage.apps/meteor/server/lib/oauth/strategiesMap.ts (1)
5-10: Nice centralization of provider strategy mapping.The key-derived
Providertype keeps downstream provider handling aligned with the registered strategies.apps/meteor/server/configuration/configurePassport.ts (1)
43-47: No action needed. ThewatchByRegexcallback is immediately invoked for all matching settings during registration (iterating through existing entries), then continues watching for subsequent changes. OAuth services are configured on initial registration, not deferred until the first setting update.> Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #39604 +/- ##
===============================================================
- Coverage 70.92% 70.88% -0.05%
===============================================================
Files 3207 3207
Lines 113353 113353
Branches 20554 20533 -21
===============================================================
- Hits 80399 80350 -49
- Misses 30908 30956 +48
- Partials 2046 2047 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tassoevan
reviewed
Mar 13, 2026
| oAuthRouter.use( | ||
| session({ | ||
| name: 'oauth', | ||
| secret: Random.secret(), |
Member
There was a problem hiding this comment.
TODO: check with @cardoso if this is FIPS-compliant.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/authentication/server/startup/index.js`:
- Line 201: Remove the debug console.log that prints sensitive data in
onCreateUserAsync: the statement console.log('onCreateUserAsync ->', options,
user) must be deleted (or replaced with a safe log) because options contains
OAuth tokens and PII (see configureOAuthServices.ts handling and
Accounts.updateOrCreateUserFromExternalService). If you need any logging, log
only non-sensitive identifiers (e.g., user._id, provider name) and never log
options, accessToken, refreshToken, profile emails, or full user objects.
🪄 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: 8f53a3c4-8a39-4c81-b8dc-ebe404b73f8d
📒 Files selected for processing (6)
apps/meteor/app/authentication/server/startup/index.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/oauthConfigs.ts
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/server/lib/oauth/oauthConfigs.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/server/lib/oauth/getOAuthServices.ts
- apps/meteor/server/configuration/configurePassport.ts
- apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
- 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). (8)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/authentication/server/startup/index.js
🧠 Learnings (3)
📓 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: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
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.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/authentication/server/startup/index.js
📚 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/app/authentication/server/startup/index.js
Contributor
There was a problem hiding this comment.
6 issues found across 10 files
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/oauthConfigs.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/oauthConfigs.ts:10">
P2: `Provider` is widened to `string` due to `Record<string, OAuthConfig>`, so unsupported provider names are not type-checked.</violation>
</file>
<file name="apps/meteor/server/configuration/configurePassport.ts">
<violation number="1" location="apps/meteor/server/configuration/configurePassport.ts:18">
P1: Session secret is generated randomly at module load time. In a multi-instance deployment each process gets a different secret, breaking session portability across instances. On restart, all in-flight OAuth sessions are invalidated.
Consider reading the secret from an environment variable or persistent configuration (e.g., `process.env.OAUTH_SESSION_SECRET`) so all instances share the same value and restarts don't break active flows.</violation>
</file>
<file name="apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts:20">
P1: Skip OAuth providers that are enabled but missing client credentials before creating strategy config.</violation>
</file>
<file name="apps/meteor/server/lib/oauth/configureOAuthServices.ts">
<violation number="1" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:47">
P2: Unsafe `as string` cast on `user.userId` — if the upstream call fails to produce a `userId`, this silently passes `undefined` to `findOneById`. Validate the value before using it.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:49">
P2: `Users.findOneById` may return `null`. Passing `null` to `done()` silently triggers a passport authentication failure with no diagnostics. Add an explicit null check.</violation>
<violation number="3" location="apps/meteor/server/lib/oauth/configureOAuthServices.ts:71">
P1: Passing the login token as a URL query parameter exposes it in browser history, server logs, and Referer headers. Consider using a short-lived, single-use authorization code that the client exchanges for the actual login token via a POST request, or set the token in a secure, httpOnly cookie instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pierre-lehnen-rc
approved these changes
Mar 27, 2026
yash-rajpal
merged commit 6889133
into
feat/phishing-resistant-mfa
34 of 39 checks passed
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
4 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
Summary by CodeRabbit
New Features
Chores