feat: Custom OAuth using Passport#40203
Open
yash-rajpal wants to merge 2 commits intofeat/phishing-resistant-mfafrom
Open
feat: Custom OAuth using Passport#40203yash-rajpal wants to merge 2 commits intofeat/phishing-resistant-mfafrom
yash-rajpal wants to merge 2 commits intofeat/phishing-resistant-mfafrom
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
WalkthroughImplements custom OAuth authentication using Passport, including a Strategy subclass for OAuth2 endpoints, verification flow for account creation/linking, Passport integration with callback routing, and dependency addition for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Passport as Passport/<br/>Strategy
participant OAuthProvider as OAuth<br/>Provider
participant VerifyFunc as Verify<br/>Function
participant Accounts as Accounts<br/>System
participant Users as Users<br/>Database
Client->>Passport: POST /oauth/{service}
Passport->>OAuthProvider: Initiate with PKCE
OAuthProvider-->>Client: Redirect to login
Client->>OAuthProvider: Authenticate
OAuthProvider-->>Passport: Callback with code
Passport->>OAuthProvider: Exchange code for token
OAuthProvider-->>Passport: Return access/refresh tokens
Passport->>Passport: Fetch identity from endpoint
Passport->>VerifyFunc: Invoke with tokens & profile
VerifyFunc->>Accounts: updateOrCreateUserFromExternalService
Accounts->>Users: Find/create user by username/email
Users-->>Accounts: Return user
Accounts->>Users: Link OAuth service to account
Accounts-->>VerifyFunc: Return user ID
VerifyFunc->>Users: Fetch complete user object
Users-->>VerifyFunc: Return user
VerifyFunc-->>Passport: done(null, user)
Passport->>Passport: Serialize user session
Passport-->>Client: Redirect /home?resumeToken=...
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Comment on lines
+129
to
+149
| async (req, res) => { | ||
| console.log('req -> user', req.user); | ||
| console.log('YAY!!!'); | ||
| const oAuthUser = req.user as IUser; | ||
|
|
||
| if (!oAuthUser) { | ||
| // return res.redirect('/login'); | ||
| return res.redirect('/noOauthUser'); | ||
| } | ||
|
|
||
| const stampedToken = Accounts._generateStampedLoginToken(); | ||
| await Accounts._insertLoginToken(oAuthUser._id, stampedToken); | ||
|
|
||
| res.redirect(`/home?resumeToken=${stampedToken.token}`); | ||
|
|
||
| req.session.destroy((err) => { | ||
| if (err) { | ||
| console.error('Error destroying session', err); | ||
| } | ||
| }); | ||
| }, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40203 +/- ##
===============================================================
+ Coverage 70.18% 70.19% +0.01%
===============================================================
Files 3280 3280
Lines 116874 116873 -1
Branches 20714 20760 +46
===============================================================
+ Hits 82024 82039 +15
+ Misses 31559 31542 -17
- Partials 3291 3292 +1
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.
Comment on lines
+1
to
+16
| import { LDAP } from '@rocket.chat/core-services'; | ||
| import type { OAuthConfiguration } from '@rocket.chat/core-typings'; | ||
| import { Logger } from '@rocket.chat/logger'; | ||
| import { Users } from '@rocket.chat/models'; | ||
| import { isAbsoluteURL } from '@rocket.chat/tools'; | ||
| import { Accounts } from 'meteor/accounts-base'; | ||
| import type { DoneCallback } from 'passport'; | ||
| import type { VerifyFunction, StrategyOptions } from 'passport-oauth2'; | ||
| import { Strategy } from 'passport-oauth2'; | ||
|
|
||
| import { normalizers, fromTemplate, renameInvalidProperties } from './transform_helpers'; | ||
| import { client } from '../../../server/database/utils'; | ||
| import { callbacks } from '../../../server/lib/callbacks'; | ||
| import { saveUserIdentity } from '../../lib/server/functions/saveUserIdentity'; | ||
| import { notifyOnUserChange } from '../../lib/server/lib/notifyListener'; | ||
| import { settings } from '../../settings/server/cached'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts '^\s*import .+from .meteor/(check|meteor).' apps/meteor/app/custom-oauthRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Get file info and check if it extends beyond line 16
wc -l apps/meteor/app/custom-oauth/server/customOAuth.tsRepository: RocketChat/Rocket.Chat
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Check for Match and Meteor usage in the file
rg -n 'Match\.|Meteor\.' apps/meteor/app/custom-oauth/server/customOAuth.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 532
🏁 Script executed:
#!/bin/bash
# View sections around the referenced lines to verify the issues
# Start with lines around 57-58 where constructor should be
sed -n '50,100p' apps/meteor/app/custom-oauth/server/customOAuth.tsRepository: RocketChat/Rocket.Chat
Length of output: 1887
🏁 Script executed:
#!/bin/bash
# View more sections to see if any of the other scratchpad issues are present
# Check for console.log instances
rg -n 'console\.log' apps/meteor/app/custom-oauth/server/customOAuth.tsRepository: RocketChat/Rocket.Chat
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Check the imports section to confirm what is/isn't imported
head -20 apps/meteor/app/custom-oauth/server/customOAuth.tsRepository: RocketChat/Rocket.Chat
Length of output: 1135
Add missing imports for Match and Meteor.
The constructor at line 57 uses Match.test(config.serverURL, String) and creates Meteor.Error instances at lines 58, 129, 142, and 284, but neither Match (from meteor/check) nor Meteor (from meteor/meteor) is imported. This will throw ReferenceError at runtime when the strategy is instantiated.
Suggested imports
import { LDAP } from '@rocket.chat/core-services';
import type { OAuthConfiguration } from '@rocket.chat/core-typings';
import { Logger } from '@rocket.chat/logger';
import { Users } from '@rocket.chat/models';
import { isAbsoluteURL } from '@rocket.chat/tools';
import { Accounts } from 'meteor/accounts-base';
+import { Match } from 'meteor/check';
+import { Meteor } from 'meteor/meteor';
import type { DoneCallback } from 'passport';
import type { VerifyFunction, StrategyOptions } from 'passport-oauth2';
import { Strategy } from 'passport-oauth2';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { LDAP } from '@rocket.chat/core-services'; | |
| import type { OAuthConfiguration } from '@rocket.chat/core-typings'; | |
| import { Logger } from '@rocket.chat/logger'; | |
| import { Users } from '@rocket.chat/models'; | |
| import { isAbsoluteURL } from '@rocket.chat/tools'; | |
| import { Accounts } from 'meteor/accounts-base'; | |
| import type { DoneCallback } from 'passport'; | |
| import type { VerifyFunction, StrategyOptions } from 'passport-oauth2'; | |
| import { Strategy } from 'passport-oauth2'; | |
| import { normalizers, fromTemplate, renameInvalidProperties } from './transform_helpers'; | |
| import { client } from '../../../server/database/utils'; | |
| import { callbacks } from '../../../server/lib/callbacks'; | |
| import { saveUserIdentity } from '../../lib/server/functions/saveUserIdentity'; | |
| import { notifyOnUserChange } from '../../lib/server/lib/notifyListener'; | |
| import { settings } from '../../settings/server/cached'; | |
| import { LDAP } from '@rocket.chat/core-services'; | |
| import type { OAuthConfiguration } from '@rocket.chat/core-typings'; | |
| import { Logger } from '@rocket.chat/logger'; | |
| import { Users } from '@rocket.chat/models'; | |
| import { isAbsoluteURL } from '@rocket.chat/tools'; | |
| import { Accounts } from 'meteor/accounts-base'; | |
| import { Match } from 'meteor/check'; | |
| import { Meteor } from 'meteor/meteor'; | |
| import type { DoneCallback } from 'passport'; | |
| import type { VerifyFunction, StrategyOptions } from 'passport-oauth2'; | |
| import { Strategy } from 'passport-oauth2'; | |
| import { normalizers, fromTemplate, renameInvalidProperties } from './transform_helpers'; | |
| import { client } from '../../../server/database/utils'; | |
| import { callbacks } from '../../../server/lib/callbacks'; | |
| import { saveUserIdentity } from '../../lib/server/functions/saveUserIdentity'; | |
| import { notifyOnUserChange } from '../../lib/server/lib/notifyListener'; | |
| import { settings } from '../../settings/server/cached'; |
🤖 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 - 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.
| scope: config.scope, | ||
| }; | ||
|
|
||
| console.log('strategy options', options); |
Contributor
There was a problem hiding this comment.
Remove debug console.log calls.
console.log('strategy options', options) (line 72), console.log('normalizeIdentity', identity) (190), console.log('afterRename', afterRename) (219), and console.log('error fetching identity from', ...) (230) are leftover debug output. The options log in particular prints clientSecret, and the identity logs print full user profiles from the IdP — secrets + PII into stdout. Use the existing logger at debug level and redact secrets, or remove.
🤖 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 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.
Comment on lines
+82
to
+85
| this.usernameField = config.usernameField.trim(); | ||
| this.emailField = config.emailField.trim(); | ||
| this.nameField = config.nameField.trim(); | ||
| this.avatarField = config.avatarField.trim(); |
Contributor
There was a problem hiding this comment.
.trim() will throw if any of these config fields is undefined.
config.usernameField, emailField, nameField, avatarField are all optional in practice (a Custom OAuth service can be configured without them), but here they are unconditionally dereferenced with .trim(). If any is undefined, the strategy construction fails. Guard before trimming.
🛠️ Suggested fix
- 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() ?? '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 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.
Comment on lines
+91
to
+97
| if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') { | ||
| this.identityTokenSentVia = this.tokenSentVia; | ||
| } | ||
|
|
||
| if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') { | ||
| this.identityTokenSentVia = this.tokenSentVia; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicated identityTokenSentVia block.
Lines 91–93 and 95–97 are identical — copy/paste. Remove one.
🧹 Suggested fix
if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') {
this.identityTokenSentVia = this.tokenSentVia;
}
-
- if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') {
- this.identityTokenSentVia = this.tokenSentVia;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') { | |
| this.identityTokenSentVia = this.tokenSentVia; | |
| } | |
| if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') { | |
| this.identityTokenSentVia = this.tokenSentVia; | |
| } | |
| if (this.identityTokenSentVia == null || this.identityTokenSentVia === 'default') { | |
| this.identityTokenSentVia = this.tokenSentVia; | |
| } |
🤖 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 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.
Comment on lines
+133
to
+173
| throw new Error('CustomOAuth: Failed to extract username', error?.message); | ||
| } | ||
| } | ||
|
|
||
| getEmail(data: Record<string, any>) { | ||
| try { | ||
| const value = fromTemplate(this.emailField, data); | ||
|
|
||
| if (!value) { | ||
| throw new Meteor.Error('field_not_found', `Email field "${this.emailField}" not found in data`, data); | ||
| } | ||
| return value; | ||
| } catch (error) { | ||
| throw new Error('CustomOAuth: Failed to extract email', error?.message); | ||
| } | ||
| } | ||
|
|
||
| getCustomName(data: Record<string, any>) { | ||
| try { | ||
| const value = fromTemplate(this.nameField, data); | ||
|
|
||
| if (!value) { | ||
| return this.getName(data); | ||
| } | ||
|
|
||
| return value; | ||
| } catch (error) { | ||
| throw new Error('CustomOAuth: Failed to extract custom name', error.message); | ||
| } | ||
| } | ||
|
|
||
| getAvatarUrl(data: Record<string, any>) { | ||
| try { | ||
| const value = fromTemplate(this.avatarField, data); | ||
|
|
||
| if (!value) { | ||
| logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, data }); | ||
| } | ||
| return value; | ||
| } catch (error) { | ||
| throw new Error('CustomOAuth: Failed to extract avatar url', error.message); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the signature of the JavaScript Error constructor and is a string accepted as the second argument?
💡 Result:
The signature of the JavaScript Error constructor, per MDN documentation and ECMAScript 2027 specification, is: new Error new Error(message) new Error(message, options) new Error(message, fileName) // non-standard new Error(message, fileName, lineNumber) // non-standard It can also be called as a function without new. A string is accepted as the second argument in the non-standard Error(message, fileName) and Error(message, fileName, lineNumber) forms, where it sets the fileName property. In the standard Error(message, options) form, the second argument is an object (with optional cause, fileName, lineNumber properties), but passing a primitive like a string avoids interpreting it as options with cause (no cause property is set, and it may be treated compatibly with the non-standard signature in some environments).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error
- 3: https://dev.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error
- 4: https://developer.cdn.mozilla.net/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error
- 5: https://nodejs.org/api/errors.html
🏁 Script executed:
cat -n apps/meteor/app/custom-oauth/server/customOAuth.ts | sed -n '130,180p'Repository: RocketChat/Rocket.Chat
Length of output: 1642
🏁 Script executed:
# Test the actual behavior of Error constructor with string as second argument
node << 'EOF'
// Test 1: Error with string as second argument
const error1 = new Error("Outer error", "some string");
console.log("Test 1: Error with string as 2nd arg");
console.log("message:", error1.message);
console.log("cause:", error1.cause);
console.log("fileName:", error1.fileName);
console.log("Full error:", error1);
console.log("---");
// Test 2: Error with cause in options object
const innerError = new Error("Inner error");
const error2 = new Error("Outer error", { cause: innerError });
console.log("Test 2: Error with cause in options");
console.log("message:", error2.message);
console.log("cause:", error2.cause);
console.log("Full error:", error2);
console.log("---");
// Test 3: What happens with error?.message
const error3 = new Error("Test", undefined?.message);
console.log("Test 3: Error with undefined?.message");
console.log("message:", error3.message);
console.log("cause:", error3.cause);
console.log("Full error:", error3);
EOFRepository: RocketChat/Rocket.Chat
Length of output: 2579
Preserve error chain using cause option instead of passing error message as second argument.
Error's second argument expects an options object ({ cause }), not a string. Passing error?.message as the second positional argument is silently ignored, losing the root error context. This occurs at lines 133, 146, 160, and 173.
Fix (example for getUsername)
- } catch (error) {
- throw new Error('CustomOAuth: Failed to extract username', error?.message);
- }
+ } catch (error) {
+ throw new Error(`CustomOAuth: Failed to extract username: ${(error as Error)?.message ?? error}`, { cause: error as Error });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| throw new Error('CustomOAuth: Failed to extract username', error?.message); | |
| } | |
| } | |
| getEmail(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.emailField, data); | |
| if (!value) { | |
| throw new Meteor.Error('field_not_found', `Email field "${this.emailField}" not found in data`, data); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error('CustomOAuth: Failed to extract email', error?.message); | |
| } | |
| } | |
| getCustomName(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.nameField, data); | |
| if (!value) { | |
| return this.getName(data); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error('CustomOAuth: Failed to extract custom name', error.message); | |
| } | |
| } | |
| getAvatarUrl(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.avatarField, data); | |
| if (!value) { | |
| logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, data }); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error('CustomOAuth: Failed to extract avatar url', error.message); | |
| throw new Error(`CustomOAuth: Failed to extract username: ${(error as Error)?.message ?? error}`, { cause: error as Error }); | |
| } | |
| } | |
| getEmail(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.emailField, data); | |
| if (!value) { | |
| throw new Meteor.Error('field_not_found', `Email field "${this.emailField}" not found in data`, data); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error(`CustomOAuth: Failed to extract email: ${(error as Error)?.message ?? error}`, { cause: error as Error }); | |
| } | |
| } | |
| getCustomName(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.nameField, data); | |
| if (!value) { | |
| return this.getName(data); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error(`CustomOAuth: Failed to extract custom name: ${(error as Error)?.message ?? error}`, { cause: error as Error }); | |
| } | |
| } | |
| getAvatarUrl(data: Record<string, any>) { | |
| try { | |
| const value = fromTemplate(this.avatarField, data); | |
| if (!value) { | |
| logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, data }); | |
| } | |
| return value; | |
| } catch (error) { | |
| throw new Error(`CustomOAuth: Failed to extract avatar url: ${(error as Error)?.message ?? error}`, { cause: error as Error }); | |
| } | |
| } |
🤖 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 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).
Comment on lines
+130
to
+131
| console.log('req -> user', req.user); | ||
| console.log('YAY!!!'); |
Contributor
There was a problem hiding this comment.
Remove debug console.log statements before merging.
console.log('req -> user', req.user) dumps the full user object (services, emails, tokens) on every OAuth callback, and console.log('YAY!!!') is clearly leftover scaffolding. Both should be removed; if observability is desired, use the existing logger with an appropriate level and without logging the user document.
🧹 Suggested cleanup
- async (req, res) => {
- console.log('req -> user', req.user);
- console.log('YAY!!!');
- const oAuthUser = req.user as IUser;
+ async (req, res) => {
+ const oAuthUser = req.user as IUser;🤖 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 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.
| const stampedToken = Accounts._generateStampedLoginToken(); | ||
| await Accounts._insertLoginToken(oAuthUser._id, stampedToken); | ||
|
|
||
| res.redirect(`/home?resumeToken=${stampedToken.token}`); |
Contributor
There was a problem hiding this comment.
Login token leaked via URL in redirect.
Embedding resumeToken in the query string exposes it through multiple well-known side channels:
- Web server / reverse proxy / CDN access logs
- Browser history and bookmarks
- The HTTP
Refererheader sent to any third-party resource (analytics, CDN, images) loaded by/home - Shoulder-surfing / screen shares
Since Accounts._insertLoginToken is effectively granting a bearer credential, it must not transit in the URL. Set it on an HttpOnly, Secure, SameSite=Lax cookie before redirecting, or return a short self-submitting POST form that posts the token to the client.
🤖 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` at 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.
Comment on lines
+15
to
+17
| // if (profile.username) {} | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/await-thenable |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dead comment and stale eslint-disable.
Line 15 is a commented-out placeholder and line 17 is an @typescript-eslint/await-thenable suppression that is no longer needed — Accounts.updateOrCreateUserFromExternalService is monkey-patched to async in apps/meteor/app/custom-oauth/server/customOAuth.ts (line 364), so the await is legitimately thenable. As per coding guidelines, avoid code comments in the implementation.
🧹 Suggested cleanup
- // if (profile.username) {}
-
- // eslint-disable-next-line `@typescript-eslint/await-thenable`
const user = await Accounts.updateOrCreateUserFromExternalService(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // if (profile.username) {} | |
| // eslint-disable-next-line @typescript-eslint/await-thenable | |
| const user = await Accounts.updateOrCreateUserFromExternalService( |
🤖 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 - 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.
Comment on lines
+18
to
+29
| const user = await Accounts.updateOrCreateUserFromExternalService( | ||
| serviceName, | ||
| { | ||
| accessToken, | ||
| refreshToken, | ||
| name: profile.displayName, | ||
| email: profile?.emails?.[0]?.value, | ||
| ...restProfile, | ||
| ..._json, | ||
| }, | ||
| {}, | ||
| ); |
Contributor
There was a problem hiding this comment.
Provider-controlled fields can override sanitized values in the service data.
...restProfile and ..._json are spread after accessToken, refreshToken, name, and email. Since the OAuth provider fully controls the contents of profile and profile._json, any field named accessToken, refreshToken, email, name, id, etc. in the provider payload will silently overwrite the values intentionally set above. At minimum this causes unpredictable behavior when provider payloads include those keys; at worst it lets a compromised/misbehaving provider response influence fields the downstream merge hook (apps/meteor/app/custom-oauth/server/customOAuth.ts) uses for account lookup/linking (serviceData.email, serviceData.username, serviceData.id).
Put controlled fields last, or build the object from _json/restProfile explicitly.
🛠️ Suggested fix
- const user = await Accounts.updateOrCreateUserFromExternalService(
- serviceName,
- {
- accessToken,
- refreshToken,
- name: profile.displayName,
- email: profile?.emails?.[0]?.value,
- ...restProfile,
- ..._json,
- },
- {},
- );
+ const user = await Accounts.updateOrCreateUserFromExternalService(
+ serviceName,
+ {
+ ...restProfile,
+ ..._json,
+ accessToken,
+ refreshToken,
+ name: profile.displayName,
+ email: profile?.emails?.[0]?.value,
+ },
+ {},
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const user = await Accounts.updateOrCreateUserFromExternalService( | |
| serviceName, | |
| { | |
| accessToken, | |
| refreshToken, | |
| name: profile.displayName, | |
| email: profile?.emails?.[0]?.value, | |
| ...restProfile, | |
| ..._json, | |
| }, | |
| {}, | |
| ); | |
| const user = await Accounts.updateOrCreateUserFromExternalService( | |
| serviceName, | |
| { | |
| ...restProfile, | |
| ..._json, | |
| accessToken, | |
| refreshToken, | |
| name: profile.displayName, | |
| email: profile?.emails?.[0]?.value, | |
| }, | |
| {}, | |
| ); |
🤖 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 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.
Comment on lines
+58
to
+59
| "node-gyp": "^10.2.0", | ||
| "passport-oauth2": "^1.8.0" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where passport and related modules are currently declared
rg -nP '"passport(-[a-z0-9-]+)?"\s*:' --glob 'package.json' --glob '**/package.json'Repository: RocketChat/Rocket.Chat
Length of output: 351
Move passport-oauth2 to apps/meteor/package.json.
All other passport modules (passport, passport-facebook, passport-facebook2, passport-github2) are declared in apps/meteor/package.json, not root. passport-oauth2 should follow the same pattern for consistency. Move it from the root package.json to apps/meteor/package.json where it belongs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 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.
![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.
|
|
||
| this.name = name; | ||
| this.options = options; | ||
| this.config = config; |
Contributor
There was a problem hiding this comment.
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.
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 121:
<comment>`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.</comment>
<file context>
@@ -0,0 +1,393 @@
+
+ this.name = name;
+ this.options = options;
+ this.config = config;
+ }
+
</file context>
| import { Logger } from '@rocket.chat/logger'; | ||
| import { Users } from '@rocket.chat/models'; | ||
| import { isAbsoluteURL } from '@rocket.chat/tools'; | ||
| import { Accounts } from 'meteor/accounts-base'; |
Contributor
There was a problem hiding this comment.
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.
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 6:
<comment>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.</comment>
<file context>
@@ -0,0 +1,393 @@
+import { Logger } from '@rocket.chat/logger';
+import { Users } from '@rocket.chat/models';
+import { isAbsoluteURL } from '@rocket.chat/tools';
+import { Accounts } from 'meteor/accounts-base';
+import type { DoneCallback } from 'passport';
+import type { VerifyFunction, StrategyOptions } from 'passport-oauth2';
</file context>
Suggested change
| import { Accounts } from 'meteor/accounts-base'; | |
| import { Accounts } from 'meteor/accounts-base'; | |
| import { Match } from 'meteor/check'; | |
| import { Meteor } from 'meteor/meteor'; |
| name: profile.displayName, | ||
| email: profile?.emails?.[0]?.value, | ||
| ...restProfile, | ||
| ..._json, |
Contributor
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/oauth/verifyFunction.ts, line 26:
<comment>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`.</comment>
<file context>
@@ -0,0 +1,42 @@
+ name: profile.displayName,
+ email: profile?.emails?.[0]?.value,
+ ...restProfile,
+ ..._json,
+ },
+ {},
</file context>
| `/oauth/${serviceKey}/callback`, | ||
| passport.authenticate(serviceKey, { failureRedirect: '/login', failureFlash: true, failWithError: true }), | ||
| async (req, res) => { | ||
| console.log('req -> user', req.user); |
Contributor
There was a problem hiding this comment.
P1: Debug console.log statements leak the full OAuth user object (including _id and potentially tokens) to stdout. Remove these before merging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/oauth/updateOAuthServices.ts, line 130:
<comment>Debug `console.log` statements leak the full OAuth user object (including `_id` and potentially tokens) to stdout. Remove these before merging.</comment>
<file context>
@@ -93,7 +100,54 @@ export async function updateOAuthServices(): Promise<void> {
+ `/oauth/${serviceKey}/callback`,
+ passport.authenticate(serviceKey, { failureRedirect: '/login', failureFlash: true, failWithError: true }),
+ async (req, res) => {
+ console.log('req -> user', req.user);
+ console.log('YAY!!!');
+ const oAuthUser = req.user as IUser;
</file context>
| ), | ||
| ); | ||
|
|
||
| oAuthRouter.get( |
Contributor
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/oauth/updateOAuthServices.ts, line 121:
<comment>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.</comment>
<file context>
@@ -93,7 +100,54 @@ export async function updateOAuthServices(): Promise<void> {
+ ),
+ );
+
+ oAuthRouter.get(
+ `/oauth/${serviceKey}`,
+ passport.authenticate(serviceKey, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }),
</file context>
| export const configurePassport = (settings: ICachedSettings) => { | ||
| passport.serializeUser((user: any, done) => { | ||
| done(null, user._id); | ||
| console.log('serializeUser', user); |
Contributor
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/configuration/configurePassport.ts, line 37:
<comment>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.</comment>
<file context>
@@ -33,11 +34,12 @@ oAuthRouter.use(flash());
export const configurePassport = (settings: ICachedSettings) => {
passport.serializeUser((user: any, done) => {
- done(null, user._id);
+ console.log('serializeUser', user);
+ done(null, user);
});
</file context>
| scope: config.scope, | ||
| }; | ||
|
|
||
| console.log('strategy options', options); |
Contributor
There was a problem hiding this comment.
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.
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 72:
<comment>`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.</comment>
<file context>
@@ -0,0 +1,393 @@
+ scope: config.scope,
+ };
+
+ console.log('strategy options', options);
+
+ super(options, verify);
</file context>
| } | ||
|
|
||
| normalizeIdentity(identity: Record<string, any>) { | ||
| console.log('normalizeIdentity', identity); |
Contributor
There was a problem hiding this comment.
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.
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 190:
<comment>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.</comment>
<file context>
@@ -0,0 +1,393 @@
+ }
+
+ normalizeIdentity(identity: Record<string, any>) {
+ console.log('normalizeIdentity', identity);
+ if (identity) {
+ for (const normalizer of Object.values(normalizers)) {
</file context>
| } | ||
| return value; | ||
| } catch (error) { | ||
| throw new Error('CustomOAuth: Failed to extract username', error?.message); |
Contributor
There was a problem hiding this comment.
P2: The Error constructor's second argument must be an options object { cause }, not a string. The original error information is silently lost here.
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 133:
<comment>The `Error` constructor's second argument must be an options object `{ cause }`, not a string. The original error information is silently lost here.</comment>
<file context>
@@ -0,0 +1,393 @@
+ }
+ return value;
+ } catch (error) {
+ throw new Error('CustomOAuth: Failed to extract username', error?.message);
+ }
+ }
</file context>
| } | ||
|
|
||
| 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
| tokenPath: string; | ||
|
|
||
| constructor(name: string, config: OAuthConfiguration & { clientSecret: string }, verify: VerifyFunction) { | ||
| if (!Match.test(config.serverURL, String)) { |
| scope: config.scope, | ||
| }; | ||
|
|
||
| console.log('strategy options', options); |
Member
There was a problem hiding this comment.
Suggested change
| console.log('strategy options', options); |
Comment on lines
+165
to
+171
| try { | ||
| const value = fromTemplate(this.avatarField, data); | ||
|
|
||
| if (!value) { | ||
| logger.debug({ msg: 'Avatar field not found in data', avatarField: this.avatarField, data }); | ||
| } | ||
| return value; |
Member
There was a problem hiding this comment.
This is repeated multiple times, consider a small helper
| } | ||
|
|
||
| normalizeIdentity(identity: Record<string, any>) { | ||
| console.log('normalizeIdentity', identity); |
Member
There was a problem hiding this comment.
Suggested change
| console.log('normalizeIdentity', identity); |
| } | ||
|
|
||
| const afterRename = renameInvalidProperties(identity); | ||
| console.log('afterRename', afterRename); |
Member
There was a problem hiding this comment.
Suggested change
| console.log('afterRename', afterRename); |
|
|
||
| this._oauth2.get(this.identityPath, accessToken, (err, body, res) => { | ||
| if (err) { | ||
| console.log('error fetching identity from', this.identityPath, err); |
Member
There was a problem hiding this comment.
If these logs are important, use the @logger package
| export const configurePassport = (settings: ICachedSettings) => { | ||
| passport.serializeUser((user: any, done) => { | ||
| done(null, user._id); | ||
| console.log('serializeUser', user); |
Member
There was a problem hiding this comment.
Suggested change
| console.log('serializeUser', user); |
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 |
| const profileWithRaw = profile as Profile & { _json?: Record<string, unknown>; _raw?: string }; | ||
| const { _json, _raw, ...restProfile } = profileWithRaw; | ||
|
|
||
| // if (profile.username) {} |
Member
There was a problem hiding this comment.
Suggested change
| // if (profile.username) {} |
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
3 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
Chores