fix: Github OAuth not getting user emails#39753
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: f763d35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughCustomOAuth now accepts an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CustomOAuth
participant IdentityProvider as Identity Provider
participant EmailProvider as Email Endpoint
Client->>CustomOAuth: Login / exchange code
CustomOAuth->>IdentityProvider: GET identity (include accessToken)
IdentityProvider-->>CustomOAuth: Identity response
alt identity contains email
CustomOAuth->>CustomOAuth: async normalizeIdentity(identity, accessToken) — use email
else identity lacks email and emailPath configured
CustomOAuth->>EmailProvider: GET emailPath (Authorization: Bearer accessToken or accessToken param)
EmailProvider-->>CustomOAuth: Email list response
CustomOAuth->>CustomOAuth: parse primary email and set identity.email
CustomOAuth->>CustomOAuth: async normalizeIdentity(identity, accessToken)
end
CustomOAuth-->>Client: Complete authentication (session/token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:79">
P2: `emailPath` is never resolved against `serverURL` for relative paths, unlike `tokenPath` and `identityPath`. A relative `emailPath` will cause a runtime fetch failure. Add the same `isAbsoluteURL` guard after the existing path resolutions.</violation>
<violation number="2" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:266">
P1: Debug `console.log` left in production code. This leaks user PII (email, name, etc.) to stdout on every OAuth login. Remove it or replace with `logger.debug`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
79-103:⚠️ Potential issue | 🟠 MajorNormalize
emailPaththe same way as the other OAuth endpoints.
tokenPathandidentityPathsupport relative values, butemailPathis stored as-is. A provider configured withemailPath: '/user/emails'will now fetch a hostless URL and the fallback breaks even though the rest of the config is valid.Suggested fix
if (!isAbsoluteURL(this.identityPath)) { this.identityPath = this.serverURL + this.identityPath; } + + if (this.emailPath && !isAbsoluteURL(this.emailPath)) { + this.emailPath = this.serverURL + this.emailPath; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js` around lines 79 - 103, Normalize emailPath like tokenPath and identityPath: detect when this.emailPath is not an absolute URL using isAbsoluteURL and, if not, prepend this.serverURL so the stored this.emailPath becomes absolute. Update the constructor/init block that currently adjusts this.tokenPath and this.identityPath to also handle this.emailPath, referencing the same helper isAbsoluteURL and this.serverURL so provider configs with relative emailPath (e.g., '/user/emails') are resolved correctly.
248-254:⚠️ Potential issue | 🟠 MajorDon't let
getEmail()short-circuit the new fallback.If
this.emailFieldis configured but missing in the primary payload,this.getEmail(identity)throws on Lines 327-329, so Lines 252-253 are never reached. That makes the newemailPathflow ineffective for providers that keepemailFieldenabled and only need the secondary endpoint when the field is absent.One way to keep the fallback reachable
- if (this.emailField) { - identity.email = this.getEmail(identity); - } + if (this.emailField) { + const extractedEmail = fromTemplate(this.emailField, identity); + if (extractedEmail) { + identity.email = extractedEmail; + } else if (!this.emailPath) { + throw new Meteor.Error('field_not_found', `Email field "${this.emailField}" not found in data`, identity); + } + } if (!identity.email && this.emailPath) { identity.email = await this.getEmailFromPath(accessToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js` around lines 248 - 254, The current call to this.getEmail(identity) when this.emailField is set can throw and prevents the emailPath fallback; update the flow so a missing primary email doesn't short-circuit the fallback by either (a) performing a safe presence check before calling getEmail (e.g. verify the configured emailField exists on the identity payload) or (b) wrapping the this.getEmail(identity) call in a try/catch that only swallows the "missing email" case and leaves other errors to bubble; ensure identity.email remains undefined/null on missing primary so the subsequent if (!identity.email && this.emailPath) branch can invoke this.getEmailFromPath(accessToken). Use the symbols this.emailField, getEmail(identity), getEmailFromPath(accessToken), and identity.email to locate the code to change.
🧹 Nitpick comments (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
277-277: Remove the new implementation comments in this helper.They don't add much beyond the code and reintroduce inline comments into the JS implementation.
As per coding guidelines
Avoid code comments in the implementation.Also applies to: 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js` at line 277, Remove the inline implementation comments that follow the header assignments in the custom OAuth helper where the request headers are built (specifically the comment after "'User-Agent': this.userAgent" and the additional new-implementation comments around the same header block at the later lines); edit the helper that constructs the headers so it only contains the header key/value pairs (e.g., "'User-Agent': this.userAgent") without the trailing URL/comment text and remove the related comments at the other occurrences (lines near the same header block), keeping only code and no inline explanatory comments.
🤖 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/custom_oauth_server.js`:
- Line 266: Remove the raw payload console logging that prints sensitive OAuth
data: delete or replace the console.log('identity', identity) call (and the
similar log at the other occurrence) so you no longer dump the normalized
identity or provider email response; if you need telemetry, log only
non-sensitive metadata (e.g., provider name, user id, or success/failure flags)
by referencing the same locations where identity is used (look for the
console.log calls that print the identity variable and the provider email
response) and ensure no full profile or email objects are emitted.
---
Outside diff comments:
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Around line 79-103: Normalize emailPath like tokenPath and identityPath:
detect when this.emailPath is not an absolute URL using isAbsoluteURL and, if
not, prepend this.serverURL so the stored this.emailPath becomes absolute.
Update the constructor/init block that currently adjusts this.tokenPath and
this.identityPath to also handle this.emailPath, referencing the same helper
isAbsoluteURL and this.serverURL so provider configs with relative emailPath
(e.g., '/user/emails') are resolved correctly.
- Around line 248-254: The current call to this.getEmail(identity) when
this.emailField is set can throw and prevents the emailPath fallback; update the
flow so a missing primary email doesn't short-circuit the fallback by either (a)
performing a safe presence check before calling getEmail (e.g. verify the
configured emailField exists on the identity payload) or (b) wrapping the
this.getEmail(identity) call in a try/catch that only swallows the "missing
email" case and leaves other errors to bubble; ensure identity.email remains
undefined/null on missing primary so the subsequent if (!identity.email &&
this.emailPath) branch can invoke this.getEmailFromPath(accessToken). Use the
symbols this.emailField, getEmail(identity), getEmailFromPath(accessToken), and
identity.email to locate the code to change.
---
Nitpick comments:
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Line 277: Remove the inline implementation comments that follow the header
assignments in the custom OAuth helper where the request headers are built
(specifically the comment after "'User-Agent': this.userAgent" and the
additional new-implementation comments around the same header block at the later
lines); edit the helper that constructs the headers so it only contains the
header key/value pairs (e.g., "'User-Agent': this.userAgent") without the
trailing URL/comment text and remove the related comments at the other
occurrences (lines near the same header block), keeping only code and no inline
explanatory comments.
🪄 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: 055cf6b0-4189-47ef-84cd-31b7168499d0
📒 Files selected for processing (3)
apps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/github/server/lib.tspackages/core-typings/src/OauthConfig.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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-typings/src/OauthConfig.tsapps/meteor/app/github/server/lib.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧠 Learnings (6)
📓 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.
📚 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:
packages/core-typings/src/OauthConfig.tsapps/meteor/app/github/server/lib.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/core-typings/src/OauthConfig.tsapps/meteor/app/github/server/lib.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/core-typings/src/OauthConfig.tsapps/meteor/app/github/server/lib.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/app/github/server/lib.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/app/github/server/lib.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.js
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/app/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:105">
P1: `emailPath` is normalized even when unset, turning `undefined` into an invalid URL and triggering failing email fetches.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
non-blocking: the emailPath parsing is GitHub-specific, which is fine since it’s currently the only provider that needs it.
for future extensibility, we could consider adding normalizers similar to what we have for identityPath. we can create a follow-up task for that.
You're absolutely right that this implementation is mostly Github specific but we don't need to extend it since very soon all of this legacy OAuth stuff will be replaced by new Passport implementation. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39753 +/- ##
===========================================
+ Coverage 70.37% 70.50% +0.12%
===========================================
Files 3238 3243 +5
Lines 114939 115243 +304
Branches 20928 20947 +19
===========================================
+ Hits 80891 81247 +356
+ Misses 32001 31940 -61
- Partials 2047 2056 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
i was testing locally and, both with and without the change, the email showed up for me on the RC side… so i checked the GH docs and the thing is: if the user has a public email, it shows up on the |
|
Exactly, this is the reason that this bug went unnoticed. When I last worked on it, it was working as expected since the account I used for testing had public email address. It was only today I realized this bug when I was testing with a new account with no public email address. |
🔴 Layne — 1 finding(s)Found 1 issue(s): 1 high. |
We can ignore Layne's comment on this one. There are security comments in the code (and Layne should've picked that up, but I'll fix the bug that made it not see it) |
Looks like it's working as expected now |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Julio Araujo <[email protected]>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Julio Araujo <[email protected]>
5 participants
Proposed changes (including videos or screenshots)
We recently moved out Github OAuth away from meteor to our custom oAuth implementation #37604 , this introduced a bug where we were not getting user's email address when user was being logged in using Github OAuth provider.
This is because Github by default doesn't return us the user's email in user identity which is very different from how other OAuth providers work, due to which users were bring signed up/logged in without an email address.
Our current custom OAuth implementation wasn't equipped to handle such cases. This PR adds the ability for our custom OAuth to handle such cases and fetch emails separately if the config providers an email fetch path. This fixes the issue for Github OAuth.
Issue(s)
CORE-1986
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes