chore: Passport apple oauth#40481
Open
yash-rajpal wants to merge 5 commits into
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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). (2)
WalkthroughAdds Apple OAuth: installs passport-apple and types, registers AppleStrategy with name/email scopes, extends OAuth config for Apple credentials (teamID, keyID, privateKeyString with normalized newlines), changes strategy option precedence, and consolidates GET/POST callback routing. ChangesApple OAuth Provider Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts`:
- Around line 20-24: Ensure you validate that OAuthConfigs[service] exists
before accessing its properties: in createOAuthServiceConfig (the branch
handling service === 'apple' and the other branch using
OAuthConfigs[service].strategy/scope/clientId/clientSecret), first check that
OAuthConfigs.hasOwnProperty(service) or OAuthConfigs[service] !== undefined and
return a safe value or throw a clear error; update the provider creation logic
to guard access to OAuthConfigs[service].strategy, .scope, .clientId and
.clientSecret so an unexpected service string cannot cause undefined property
dereference.
- Line 28: The code calls .replace() directly on
settings.get('Accounts_OAuth_Apple_secretKey') which can be undefined; change
the construction of privateKeyString to guard and normalize the value before
calling replace — read the raw value into a local (e.g., secretKeyRaw), check
for undefined/null and default to an empty string (or handle missing
configuration appropriately), then call .replace(/\n/g, '\n') on that normalized
string so privateKeyString is always a string; update the assignment that
produces privateKeyString in createOAuthServiceConfig to use this guarded value.
🪄 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: 97ceaa07-9cfe-4ff0-80ac-4ab06cdc1fbb
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/meteor/package.jsonapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/oauthConfigs.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). (3)
- GitHub Check: 📦 Build Packages
- 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/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.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:
apps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
Comment on lines
+20
to
+24
| if (service === 'apple') { | ||
| return { | ||
| provider: service, | ||
| strategy: OAuthConfigs[service].strategy, | ||
| scope: OAuthConfigs[service].scope, |
Contributor
There was a problem hiding this comment.
Validate provider key before dereferencing OAuthConfigs[service]
service is typed as string, but Line 23/24/34/37 assumes it exists in OAuthConfigs. An unexpected service value will crash with undefined access.
Proposed fix
export const createOAuthServiceConfig = (settings: ICachedSettings, services: string[]): OAuthServiceConfig[] => {
return services.map((service) => {
+ const providerConfig = OAuthConfigs[service];
+ if (!providerConfig) {
+ throw new Error(`Unsupported OAuth provider: ${service}`);
+ }
+
if (service === 'apple') {
return {
provider: service,
- strategy: OAuthConfigs[service].strategy,
- scope: OAuthConfigs[service].scope,
+ strategy: providerConfig.strategy,
+ scope: providerConfig.scope,
clientId: settings.get<string>('Accounts_OAuth_Apple_id'),
teamID: settings.get<string>('Accounts_OAuth_Apple_iss'),
keyID: settings.get<string>('Accounts_OAuth_Apple_kid'),
privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey')?.replace(/\\n/g, '\n'),
};
}
return {
provider: service,
- strategy: OAuthConfigs[service].strategy,
+ strategy: providerConfig.strategy,
clientId: settings.get<string>(`Accounts_OAuth_${capitalize(service)}_id`),
clientSecret: settings.get<string>(`Accounts_OAuth_${capitalize(service)}_secret`),
- scope: OAuthConfigs[service].scope,
+ scope: providerConfig.scope,
};
});
};Also applies to: 34-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts` around lines 20 -
24, Ensure you validate that OAuthConfigs[service] exists before accessing its
properties: in createOAuthServiceConfig (the branch handling service === 'apple'
and the other branch using
OAuthConfigs[service].strategy/scope/clientId/clientSecret), first check that
OAuthConfigs.hasOwnProperty(service) or OAuthConfigs[service] !== undefined and
return a safe value or throw a clear error; update the provider creation logic
to guard access to OAuthConfigs[service].strategy, .scope, .clientId and
.clientSecret so an unexpected service string cannot cause undefined property
dereference.
| clientId: settings.get<string>('Accounts_OAuth_Apple_id'), | ||
| teamID: settings.get<string>('Accounts_OAuth_Apple_iss'), | ||
| keyID: settings.get<string>('Accounts_OAuth_Apple_kid'), | ||
| privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey').replace(/\\n/g, '\n'), |
Contributor
There was a problem hiding this comment.
Guard privateKeyString before calling .replace()
If Accounts_OAuth_Apple_secretKey is unset, Line 28 throws at runtime (Cannot read properties of undefined). Please normalize safely before replace.
Proposed fix
- privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey').replace(/\\n/g, '\n'),
+ privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey')?.replace(/\\n/g, '\n'),📝 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
| privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey').replace(/\\n/g, '\n'), | |
| privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey')?.replace(/\\n/g, '\n'), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts` at line 28, The
code calls .replace() directly on settings.get('Accounts_OAuth_Apple_secretKey')
which can be undefined; change the construction of privateKeyString to guard and
normalize the value before calling replace — read the raw value into a local
(e.g., secretKeyRaw), check for undefined/null and default to an empty string
(or handle missing configuration appropriately), then call .replace(/\n/g, '\n')
on that normalized string so privateKeyString is always a string; update the
assignment that produces privateKeyString in createOAuthServiceConfig to use
this guarded value.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40481 +/- ##
============================================================
Coverage 69.75% 69.76%
============================================================
Files 3297 3297
Lines 120970 120970
Branches 21520 21519 -1
============================================================
+ Hits 84381 84389 +8
+ Misses 33319 33317 -2
+ Partials 3270 3264 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
63-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the OAuth debug dumps.
This logs
req.user, the whole session object, and the session id during sign-in. That leaks auth/session data into server logs on a sensitive path.🧹 Proposed fix
const callbackHandler = [ passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true }), async (req: Request, res: Response) => { - console.log('req -> user', req.user); const oAuthUser = req.user as IUser; @@ oAuthRouter.get( `/oauth/${config.provider}`, - (req, _res, next) => { - console.log('authenticate', req.session, req.session.id); - next(); - }, passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 63 - 94, Remove the debug console dumps in the OAuth flow: delete the console.log('req -> user', req.user) inside the callbackHandler async middleware and the console.log('authenticate', req.session, req.session.id) in the oAuthRouter.get pre-handler so sensitive session/user data is not logged; keep the existing redirect and token logic in callbackHandler and retain the passport.authenticate(...) calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 63-65: The shared callbackHandler currently uses
passport.authenticate(..., { failureRedirect: '/login', failureFlash: true,
failWithError: true }) but the reused async handler swallows the error path so
passport's failWithError forwarding (to next(err)) is lost; modify the route
setup so that the original authentication middleware (passport.authenticate with
failWithError) remains as its own middleware before the shared async handler, or
ensure the shared callbackHandler array includes an error-forwarding middleware
that calls next(err) on authentication failure; update both uses of
callbackHandler (the one around callbackHandler and the one at lines 96-99) so
provider denials still reach the route-specific error handler and preserve the
redirect/failureFlash behavior.
- Around line 96-99: The oAuthRouter callback POST handler currently lacks a
URL-encoded body parser so POSTed form data (e.g. Apple callback) yields
undefined req.body; fix by adding express.urlencoded({ extended: false })
middleware to the router before registering the callback routes in
configureOAuthServices.ts: call oAuthRouter.use(express.urlencoded({ extended:
false })) prior to
oAuthRouter.route(`/oauth/${config.provider}/callback`).post(...callbackHandler)
so passport.authenticate() sees the parsed body.
---
Outside diff comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 63-94: Remove the debug console dumps in the OAuth flow: delete
the console.log('req -> user', req.user) inside the callbackHandler async
middleware and the console.log('authenticate', req.session, req.session.id) in
the oAuthRouter.get pre-handler so sensitive session/user data is not logged;
keep the existing redirect and token logic in callbackHandler and retain the
passport.authenticate(...) calls unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d9dd8aa-4b51-452b-b04d-5ed4d19e8ac5
📒 Files selected for processing (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- 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/server/lib/oauth/configureOAuthServices.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
🔇 Additional comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
22-29: LGTM!
Comment on lines
+63
to
65
| const callbackHandler = [ | ||
| passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true }), | ||
| async (req: Request, res: Response) => { |
Contributor
There was a problem hiding this comment.
Preserve the callback error middleware when reusing callbackHandler.
failWithError: true forwards auth failures to next(err), but the shared handler now stops at the success path. That drops the callback route's local failure handling and can turn provider denials into generic error responses instead of the expected redirect.
↩️ Proposed fix
const callbackHandler = [
passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true }),
- async (req: Request, res: Response) => {
- const oAuthUser = req.user as IUser;
+ async (req: Request, res: Response, next: NextFunction) => {
+ try {
+ const oAuthUser = req.user as IUser;
- if (!oAuthUser) {
- return res.redirect('/noOauthUser');
- }
+ if (!oAuthUser) {
+ return res.redirect('/noOauthUser');
+ }
- const stampedToken = Accounts._generateStampedLoginToken();
- await Accounts._insertLoginToken(oAuthUser._id, stampedToken);
+ const stampedToken = Accounts._generateStampedLoginToken();
+ await Accounts._insertLoginToken(oAuthUser._id, stampedToken);
- res.redirect(`/home?resumeToken=${stampedToken.token}`);
+ res.redirect(`/home?resumeToken=${stampedToken.token}`);
- req.session.destroy((err) => {
- if (err) {
- console.error('Error destroying session', err);
- }
- });
+ req.session.destroy((err) => {
+ if (err) {
+ console.error('Error destroying session', err);
+ }
+ });
+ } catch (error) {
+ next(error);
+ }
},
+ (_error: unknown, _req: Request, res: Response, _next: NextFunction) => {
+ res.redirect('/login');
+ },
];Also applies to: 96-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 63 - 65,
The shared callbackHandler currently uses passport.authenticate(..., {
failureRedirect: '/login', failureFlash: true, failWithError: true }) but the
reused async handler swallows the error path so passport's failWithError
forwarding (to next(err)) is lost; modify the route setup so that the original
authentication middleware (passport.authenticate with failWithError) remains as
its own middleware before the shared async handler, or ensure the shared
callbackHandler array includes an error-forwarding middleware that calls
next(err) on authentication failure; update both uses of callbackHandler (the
one around callbackHandler and the one at lines 96-99) so provider denials still
reach the route-specific error handler and preserve the redirect/failureFlash
behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
66-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove debug logging of user data and commented-out code.
- Line 66:
console.log('req -> user', req.user)dumps the authenticated user object (including PII such as email/id) to stdout on every successful callback — please drop it before merging.- Line 70: leftover
// return res.redirect('/login');is dead commented code; either restore the behavior intentionally or delete the comment.As per coding guidelines (
**/*.{ts,tsx,js}: Avoid code comments in the implementation).🧹 Proposed cleanup
async (req: Request, res: Response) => { - console.log('req -> user', req.user); const oAuthUser = req.user as IUser; if (!oAuthUser) { - // return res.redirect('/login'); return res.redirect('/noOauthUser'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 66 - 72, Remove the debug dump and dead comment: delete the console.log('req -> user', req.user) line and remove the commented-out "// return res.redirect('/login');" so the code uses the existing oAuthUser check (req.user cast to IUser in oAuthUser) and returns res.redirect('/noOauthUser') when no user; ensure no other debug prints remain in configureOAuthServices-related callback handling.
♻️ Duplicate comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
63-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve a failure path for
failWithError: true.
passport.authenticate(..., { failWithError: true })forwards auth failures tonext(err), butcallbackHandlerends at the success handler with no trailing 4-arg error middleware. Provider denials/CSRF failures will fall through to Express's default error handler (HTML 500) instead of redirecting to/login. Append an error handler to the chain so both GET and POST callbacks behave consistently.↩️ Proposed fix
const callbackHandler = [ passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true }), async (req: Request, res: Response) => { ... }, + (_err: unknown, _req: Request, res: Response, _next: unknown) => { + res.redirect('/login'); + }, ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 63 - 65, The callbackHandler currently uses passport.authenticate(..., { failWithError: true }) but only provides the success handler, so authentication errors are forwarded to next(err) and hit Express's default handler; add a trailing 4-arg error middleware to the callbackHandler array (after the async (req, res) success handler) that matches (err, req, res, next) and handles failures by invoking the same failure behavior as the non-error path—e.g., redirect to '/login', set a failure flash (or call req.flash with the error message), and end the response—so both GET/POST callbacks and provider denials/CSRF failures behave consistently when using passport.authenticate with failWithError.
🧹 Nitpick comments (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
27-28: ⚡ Quick winSpreading
...configafter hard-coded defaults lets config override security-relevant flags.By placing
...configafterstate: true,pkce: true,callbackURL, andprofileFields, anyOAuthServiceConfigentry that happens to carry those keys will silently override the defaults — including the CSRF/replay protection toggles. IfprofileFieldsis the only field intended to be overridable, lift just that property out ofconfigexplicitly so the security defaults remain authoritative.♻️ Proposed fix
new Strategy( { clientID: config.clientId, callbackURL: `${siteUrl}/oauth/${config.provider}/callback`, state: true, pkce: true, - profileFields: ['id', 'displayName', 'emails'], ...config, + profileFields: config.profileFields ?? ['id', 'displayName', 'emails'], + state: true, + pkce: true, + callbackURL: `${siteUrl}/oauth/${config.provider}/callback`, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 27 - 28, The current spread of ...config after hard-coded defaults allows callers to override security-relevant flags (state, pkce, callbackURL, profileFields); update the code in configureOAuthServices so you explicitly pull profileFields out of the incoming config (e.g., const { profileFields, ...restConfig } = config) and then build the options by spreading restConfig before the defaults or better: apply defaults (state: true, pkce: true, callbackURL, profileFields: ['id','displayName','emails']) and only override profileFields with the extracted profileFields value if present, ensuring state/pkce/callbackURL cannot be overridden by config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 87-94: Remove the passthrough debug middleware from the OAuth
entry route: in the oAuthRouter.get call for `/oauth/${config.provider}` delete
the inline (req, _res, next) => { console.log('authenticate', req.session,
req.session.id); next(); } handler and invoke
passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent',
failureRedirect: '/login' }) directly as the route handler so the route uses
only passport.authenticate and no sensitive-session logging.
---
Outside diff comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 66-72: Remove the debug dump and dead comment: delete the
console.log('req -> user', req.user) line and remove the commented-out "//
return res.redirect('/login');" so the code uses the existing oAuthUser check
(req.user cast to IUser in oAuthUser) and returns res.redirect('/noOauthUser')
when no user; ensure no other debug prints remain in
configureOAuthServices-related callback handling.
---
Duplicate comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 63-65: The callbackHandler currently uses
passport.authenticate(..., { failWithError: true }) but only provides the
success handler, so authentication errors are forwarded to next(err) and hit
Express's default handler; add a trailing 4-arg error middleware to the
callbackHandler array (after the async (req, res) success handler) that matches
(err, req, res, next) and handles failures by invoking the same failure behavior
as the non-error path—e.g., redirect to '/login', set a failure flash (or call
req.flash with the error message), and end the response—so both GET/POST
callbacks and provider denials/CSRF failures behave consistently when using
passport.authenticate with failWithError.
---
Nitpick comments:
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts`:
- Around line 27-28: The current spread of ...config after hard-coded defaults
allows callers to override security-relevant flags (state, pkce, callbackURL,
profileFields); update the code in configureOAuthServices so you explicitly pull
profileFields out of the incoming config (e.g., const { profileFields,
...restConfig } = config) and then build the options by spreading restConfig
before the defaults or better: apply defaults (state: true, pkce: true,
callbackURL, profileFields: ['id','displayName','emails']) and only override
profileFields with the extracted profileFields value if present, ensuring
state/pkce/callbackURL cannot be overridden by config.
🪄 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: b943a8b0-63a1-41dc-b025-964aac9619b2
📒 Files selected for processing (1)
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- 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/server/lib/oauth/configureOAuthServices.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/oauth/configureOAuthServices.ts
Comment on lines
+87
to
94
| oAuthRouter.get( | ||
| `/oauth/${config.provider}`, | ||
| (req, _res, next) => { | ||
| console.log('authenticate', req.session, req.session.id); | ||
| next(); | ||
| }, | ||
| passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Drop the pass-through debug middleware on the auth entry route.
The inline (req, _res, next) => { console.log('authenticate', req.session, req.session.id); next(); } only exists to log the session and adds no behavior. Session ids are sensitive (hijacking risk if logs leak) and the extra middleware obscures the route. Remove it and call passport.authenticate directly.
🧹 Proposed cleanup
- oAuthRouter.get(
- `/oauth/${config.provider}`,
- (req, _res, next) => {
- console.log('authenticate', req.session, req.session.id);
- next();
- },
- passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }),
- );
+ oAuthRouter.get(
+ `/oauth/${config.provider}`,
+ passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }),
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/oauth/configureOAuthServices.ts` around lines 87 - 94,
Remove the passthrough debug middleware from the OAuth entry route: in the
oAuthRouter.get call for `/oauth/${config.provider}` delete the inline (req,
_res, next) => { console.log('authenticate', req.session, req.session.id);
next(); } handler and invoke passport.authenticate(config.provider, { scope:
config.scope, prompt: 'consent', failureRedirect: '/login' }) directly as the
route handler so the route uses only passport.authenticate and no
sensitive-session logging.
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
1 participant
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
Bug Fixes / Improvements