core: fix oauth callback with mail only#1110
Conversation
WalkthroughThe OAuth callback handler in the user module was changed to add an email-based binding step. If no user is found by the OAuth provider id, the handler now looks up a user by the OAuth-provided email; if found, it binds the OAuth id to that existing user, authenticates, and redirects to '/'. If no user is found by id or email, the previous binding/registration flow (permission checks, username validation, registration token flow) remains unchanged. Existing behavior for a direct id match is unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/hydrooj/src/handler/user.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/hydrooj/src/handler/user.ts (1)
framework/framework/error.ts (1)
BadRequestError(49-49)
⏰ 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)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/hydrooj/src/handler/user.ts (1)
479-484: LGTM: Initial OAuth lookup is correct.The initial lookup by OAuth provider id followed by authentication is straightforward and correct.
Refactor OAuth binding logic to simplify account binding checks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hydrooj/src/handler/user.ts (1)
485-491: Email validation still missing before lookup.The past review comment flagged that
r.emailmight be undefined or null if the OAuth provider doesn't return an email. This concern was not addressed in the current code. Callinguser.getByEmail('system', r.email)without validation could lead to unexpected behavior.🔎 Proposed fix to add email validation
- const udoc = await user.getByEmail('system', r.email); - if (udoc) { + const udoc = r.email ? await user.getByEmail('system', r.email) : null; + if (udoc) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/hydrooj/src/handler/user.ts
⏰ 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)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
| const udoc = await user.getByEmail('system', r.email); | ||
| if (udoc) { | ||
| await this.ctx.oauth.set(args.type, r._id, udoc._id); | ||
| await successfulAuth.call(this, udoc); | ||
| this.response.redirect = '/'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google OAuth email verification guaranteed in access token response
💡 Result:
Short answer: No — Google does not guarantee an email_verified field in the access token response. email and email_verified are provided as claims in the ID token (or returned by userinfo when you call the UserInfo endpoint) only if you request the appropriate scope (openid email). You should verify email_verified from the ID token (or the /userinfo response), not from the access token itself. [1][2][3]
Sources:
- OpenID Connect / email scope: ID token includes email and email_verified when you request the email scope. [1]
- Google Sign‑in / ID token claims (example showing email_verified). [2]
- Backend auth guide: verify the ID token (email/email_verified) or call tokeninfo/userinfo to confirm. [3]
🌐 Web query:
GitHub OAuth email verification status in API response
💡 Result:
Short answer: GitHub returns email verification status only from the Emails API (GET /user/emails) — each email object includes "verified": true|false. The standard user object (GET /user) may include an email field but does not expose a "verified" flag. OAuth tokens must include the user:email (or appropriate fine‑grained "Email addresses") scope to call the Emails endpoints. [1][2]
Sources:
- Emails API (response includes "verified"). [1]
- Get authenticated user (email in /user is just the public email; no verified field). [2]
🌐 Web query:
Facebook OAuth email verification guarantee
💡 Result:
Short answer: Don’t rely on Facebook Login to guarantee an app-ready, permanently verified email — verify it yourself.
Details: Facebook may return an email via OAuth when the user has one, but the docs and Facebook guidance tell apps to confirm email ownership themselves (e.g., send a verification link) because the email field isn’t always present/guaranteed for every account and practices/fields can vary. [1][2].
If you need a definitive flag you can trust for downstream account actions, send your own confirmation email (or use an IdP-aware user.profile/email_verified field in your identity provider stack). [1][3]
Sources:
- Facebook Login guidance (recommendation to verify provider emails in your app). [1]
- Facebook Help Center: how/when Facebook sends email confirmations and how it uses account emails. [2]
- Notes/discussion about Graph API email behavior (email may be null/not guaranteed). [3]
🏁 Script executed:
cd packages/hydrooj/src/handler && wc -l user.tsRepository: hydro-dev/Hydro
Length of output: 70
🏁 Script executed:
cd packages/hydrooj/src/handler && sed -n '470,510p' user.tsRepository: hydro-dev/Hydro
Length of output: 1805
🏁 Script executed:
cd packages/hydrooj/src && find . -name "*.ts" -o -name "*.js" | xargs rg -l "oauth" | head -20Repository: hydro-dev/Hydro
Length of output: 175
🏁 Script executed:
cd packages/hydrooj/src/handler && sed -n '430,495p' user.tsRepository: hydro-dev/Hydro
Length of output: 2390
🏁 Script executed:
cd packages/hydrooj/src && rg -A 5 -B 5 "const udoc = await user.getByEmail" handler/user.tsRepository: hydro-dev/Hydro
Length of output: 960
🏁 Script executed:
cd packages/hydrooj/src && cat -n model/oauth.ts | head -100Repository: hydro-dev/Hydro
Length of output: 3092
🏁 Script executed:
cd packages/hydrooj/src && rg -l "OAuthProvider" --type ts | head -20Repository: hydro-dev/Hydro
Length of output: 86
🏁 Script executed:
cd packages/hydrooj/src && rg "provide.*OAuthProvider\|OAuthProvider.*provide" -A 10 -B 2 | head -100Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages && find . -path "*/plugin*" -name "*.ts" | xargs rg -l "oauth" 2>/dev/null | head -10Repository: hydro-dev/Hydro
Length of output: 86
🏁 Script executed:
cd packages/hydrooj/src && rg "OAuthProvider\|oauth.provide" -B 5 -A 10 plugin-api.ts interface.tsRepository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages && find . -name "*.ts" -o -name "*.js" | xargs rg "email_verified\|emailVerified" 2>/dev/null | head -30Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages && find . -path "*plugin*" -name "*.ts" -o -name "*.js" | xargs rg "provide.*oauth\|oauth.*callback" 2>/dev/null | head -50Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages/hydrooj/src && head -150 plugin-api.ts | tail -80Repository: hydro-dev/Hydro
Length of output: 2756
🏁 Script executed:
cd packages/hydrooj/src && grep -n "oauth" plugin-api.ts interface.tsRepository: hydro-dev/Hydro
Length of output: 401
🏁 Script executed:
cd packages && git ls-files | grep -E "plugin.*oauth|oauth.*plugin" | head -20Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages && git ls-files | grep -i plugin | head -30Repository: hydro-dev/Hydro
Length of output: 84
🏁 Script executed:
cd packages && ls -la | grep -E "^d"Repository: hydro-dev/Hydro
Length of output: 1583
🏁 Script executed:
cd packages && find . -maxdepth 2 -type d -name "*plugin*" -o -type d -name "*oauth*" | head -20Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
cd packages/login-with-google && find . -name "*.ts" -o -name "*.js" | head -20Repository: hydro-dev/Hydro
Length of output: 69
🏁 Script executed:
cd packages/login-with-github && find . -name "*.ts" -o -name "*.js" | head -20Repository: hydro-dev/Hydro
Length of output: 69
🏁 Script executed:
cd packages/login-with-google && cat -n index.ts | head -100Repository: hydro-dev/Hydro
Length of output: 4315
🏁 Script executed:
cd packages/login-with-github && cat -n index.ts | head -150Repository: hydro-dev/Hydro
Length of output: 5364
Validate email verification for providers that support it.
The auto-binding logic trusts the OAuth provider's email claim without verifying it has been confirmed by the provider. Google's ID token includes an email_verified claim that should be checked before auto-binding; currently, only the email field is used. GitHub's implementation correctly validates the verified flag from the Emails API. Add a check for payload.email_verified in the Google provider's callback (around login-with-google/index.ts line 59-67), or document that only providers with mandatory email verification are configured for auto-binding.
🤖 Prompt for AI Agents
In packages/hydrooj/src/handler/user.ts around lines 485 to 491, the
auto-binding flow unconditionally trusts the provider email; update the logic so
that for Google you only auto-bind when the token payload includes
email_verified === true (or otherwise assert the provider guarantees verified
emails). Specifically: inspect the OAuth payload for email_verified before
calling user.getByEmail and set the oauth mapping; if email_verified is false or
missing, skip auto-binding (or route the user to a verification/confirmation
path) and ensure a clear log or response is emitted so binding does not occur
for unverified addresses.
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.