feat: GitHub OAuth login with auto-linking and access control#121
Merged
Conversation
- Add GitHub as a social login provider via Better Auth with custom getUserInfo - Auto-link GitHub users to companyGithubUsers on each login via session hook - Support account linking (same email merges Google + GitHub accounts) - Show pulsing blue beacon on login button for last-used provider - Update auth docs with correct account_id (numeric ID) behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… error on login page - Check all org tenant DBs for GitHub login in companyGithubUsers before allowing sign-in - Redirect OAuth errors to /login with error query param - Display user-friendly error message on login page - Add errorCallbackURL to signInSocial call Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds GitHub OAuth as a social provider, implements server-side GitHub profile/email resolution and cross-tenant company linking, updates the login UI to support GitHub sign-in and error handling, and documents the updated GitHub linking behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant LoginUI as Login Page
participant AuthServer as Auth Server
participant GithubAPI as GitHub API
participant TenantDB as Tenant DB
participant SessionHook as Session Hook
User->>LoginUI: Click "Login with GitHub"
LoginUI->>AuthServer: POST provider='github' + errorCallbackURL
AuthServer->>GithubAPI: GET /user (with access token)
GithubAPI-->>AuthServer: User profile {id, login, email}
AuthServer->>TenantDB: Query orgs / companyGithubUsers by profile.login
TenantDB-->>AuthServer: Membership/match result
AuthServer->>AuthServer: Create session
AuthServer->>SessionHook: Trigger session.create.after
SessionHook->>GithubAPI: GET /user (resolve login/email)
SessionHook->>TenantDB: UPDATE companyGithubUsers SET userId WHERE login=profile.login
TenantDB-->>SessionHook: Update results
SessionHook-->>AuthServer: Linking complete
AuthServer-->>LoginUI: Redirect with session
LoginUI-->>User: Authenticated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
🧹 Nitpick comments (1)
app/libs/auth.server.ts (1)
174-176: Don’t silently swallow post-session linking failures.Line [175] drops all errors with
.catch(() => {}), which removes operational visibility when linking breaks. At minimum, log a structured warning withsession.userId.🔍 Proposed improvement
- await linkGithubUserToCompanyUsers(session.userId).catch(() => {}) + await linkGithubUserToCompanyUsers(session.userId).catch((error) => { + console.warn('[GitHub linking] post-session linking failed', { + userId: session.userId, + error, + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/auth.server.ts` around lines 174 - 176, The post-session hook is currently swallowing all errors from linkGithubUserToCompanyUsers via .catch(() => {}); change this to catch and log a structured warning that includes the session.userId and the error details (e.g., in the after: async (session) => handler, call linkGithubUserToCompanyUsers(session.userId).catch(err => /* log warn with session.userId and err */)). Use the project's logger (or console.warn if none) so failures are visible and include a clear message, session.userId, and the caught 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 `@app/libs/auth.server.ts`:
- Around line 58-71: The current orgs.map + Promise.all spawns an unbounded
query per org causing FD/IO spikes; replace it with a bounded-concurrency or
early-exit loop: either iterate orgs with a for..of and await each tenant check
(calling getTenantDb(...) and the
selectFrom('companyGithubUsers').select(['login']).where('login','=',profile.login').executeTakeFirst())
returning immediately when a match is found, or use a concurrency limiter (e.g.,
p-limit) to run N tenantDb checks in parallel and stop further scheduling when a
match is discovered; update the code around the orgs.map usage to use
getTenantDb and the selectFrom query inside the bounded/early-exit approach.
- Around line 30-35: The GitHub fetches in getUserInfo currently have no
timeouts; wrap both external calls (the fetch to 'https://api.github.com/user'
and the other GitHub API call inside getUserInfo) with an AbortController-based
timeout (create AbortController, pass controller.signal to fetch, set a
setTimeout to call controller.abort() after a reasonable timeout like 3–10s, and
clearTimeout on success), and handle the abort error path so getUserInfo returns
or throws a clear timeout error; update both fetch invocations and their error
handling to use the same pattern so upstream stalls cannot hang the auth flow.
- Around line 23-24: The GitHub provider is being force-cast to string
(process.env.GITHUB_CLIENT_ID as string / process.env.GITHUB_CLIENT_SECRET as
string), which hides missing env vars; update the provider registration in
auth.server.ts to mirror the Google pattern by only adding the GitHub provider
when both process.env.GITHUB_CLIENT_ID and process.env.GITHUB_CLIENT_SECRET are
present, remove the `as string` casts, and conditionally push or spread the
GitHub provider into the providers array used by the function that builds the
auth options (e.g., where the Google provider is registered) so provider setup
fails fast if envs are missing.
- Around line 63-66: The current query against companyGithubUsers uses
exact-case matching (.where('login', '=', profile.login) in the
selectFrom('companyGithubUsers') block), which breaks because GitHub logins are
case-insensitive; update the codebase to normalize and compare logins
case-insensitively: ensure addGithubUser and updateGithubUser in
mutations.server.ts store logins as lowercase, and change lookups (the select in
auth.server.ts and the query in github-linking.server.ts) to compare lowercase
values (e.g., compare lower(login) to profile.login.toLowerCase or use a
case-insensitive DB operator) so matching works regardless of stored or returned
casing.
In `@app/routes/_auth/login.tsx`:
- Around line 41-43: Replace the raw FormData cast for provider with Conform +
Zod validation: define a Zod schema (e.g., providerSchema or part of the
existing form schema) that enforces provider as z.enum(['google','github']),
then call parseWithZod (the same helper used across routes) against
request.formData() and extract the validated provider value instead of using
String(formData.get(...)). Update any local references in this file (the
provider const and related logic that uses it) to use the validated value
returned by parseWithZod.
In `@app/services/github-linking.server.ts`:
- Around line 26-32: The GitHub fetch call (the
fetch('https://api.github.com/user', { Authorization: `Bearer
${githubAccount.accessToken}` })) needs a client-side timeout so a
slow/unresponsive GitHub API doesn't block the post-login hook; wrap the fetch
with an AbortController, pass controller.signal to fetch, schedule a setTimeout
that calls controller.abort() after a short timeout (e.g., 3–5s), and clear that
timer after the fetch completes; ensure the existing try/catch still handles the
abort error, and apply this change around the fetch invocation that produces res
so the request is aborted on timeout instead of indefinitely awaiting a
response.
In `@docs/auth-and-invitation.md`:
- Around line 85-90: The docs snippet under socialProviders (github.clientId /
github.clientSecret / mapProfileToUser) uses GITHUB_APP_CLIENT_ID and
GITHUB_APP_CLIENT_SECRET but the runtime auth config reads GITHUB_CLIENT_ID and
GITHUB_CLIENT_SECRET; update the documented env var names to match the runtime
(change GITHUB_APP_CLIENT_ID -> GITHUB_CLIENT_ID and GITHUB_APP_CLIENT_SECRET ->
GITHUB_CLIENT_SECRET) or explicitly note both variants and the expected one, so
the socialProviders.clientId/clientSecret examples align with the values read by
the auth config.
---
Nitpick comments:
In `@app/libs/auth.server.ts`:
- Around line 174-176: The post-session hook is currently swallowing all errors
from linkGithubUserToCompanyUsers via .catch(() => {}); change this to catch and
log a structured warning that includes the session.userId and the error details
(e.g., in the after: async (session) => handler, call
linkGithubUserToCompanyUsers(session.userId).catch(err => /* log warn with
session.userId and err */)). Use the project's logger (or console.warn if none)
so failures are visible and include a clear message, session.userId, and the
caught error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.exampleapp/libs/auth.server.tsapp/routes/_auth/login.tsxapp/services/github-linking.server.tsdocs/auth-and-invitation.mdvite.config.ts
Comment on lines
+23
to
+24
| clientId: process.env.GITHUB_CLIENT_ID as string, | ||
| clientSecret: process.env.GITHUB_CLIENT_SECRET as string, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and read the relevant section
fd -t f "auth.server.ts" app/Repository: coji/upflow
Length of output: 78
🏁 Script executed:
# Read the auth.server.ts file with context around lines 23-24
wc -l app/libs/auth.server.tsRepository: coji/upflow
Length of output: 82
🏁 Script executed:
# Read the file to see the exact code
cat -n app/libs/auth.server.ts | head -100Repository: coji/upflow
Length of output: 4052
🏁 Script executed:
# Continue reading the file to see the rest of the implementation
cat -n app/libs/auth.server.ts | sed -n '100,200p'Repository: coji/upflow
Length of output: 3080
🏁 Script executed:
# Check for any env var validation or initialization
cat -n app/libs/auth.server.ts | sed -n '200,384p'Repository: coji/upflow
Length of output: 6154
🏁 Script executed:
# Search for usage of socialProviders or auth config elsewhere in the codebase
rg "socialProviders|betterAuth\(" --type ts --type tsx -B 2 -A 2Repository: coji/upflow
Length of output: 82
🏁 Script executed:
# Check how the auth object is exported and used
rg "export.*auth|from.*auth\.server" --type ts --type tsx -A 2Repository: coji/upflow
Length of output: 82
🏁 Script executed:
# Search for usage of auth or socialProviders
rg "auth\." --max-count 20Repository: coji/upflow
Length of output: 3582
🏁 Script executed:
# Check how the auth object is used in routes or handlers
rg "from.*auth\.server|import.*auth" --max-count 20Repository: coji/upflow
Length of output: 4745
🏁 Script executed:
# Look for environment variable configuration/validation
rg "GITHUB_CLIENT_ID|GITHUB_CLIENT_SECRET"Repository: coji/upflow
Length of output: 222
🏁 Script executed:
# Check betterAuth documentation or error handling around provider config
rg "betterAuth" -A 5 -B 5Repository: coji/upflow
Length of output: 1038
🏁 Script executed:
# Check if there are environment variable setup/docs
fd -t f "\.env|README|SETUP" | head -20Repository: coji/upflow
Length of output: 94
🏁 Script executed:
# Verify the login flow that uses the auth object
cat -n app/routes/_auth/login.tsx | head -60Repository: coji/upflow
Length of output: 2032
Don't force-cast optional GitHub env vars to string.
Lines 23-24 mask missing configuration at compile-time and can leave runtime provider setup broken. Gate GitHub provider registration on actual env presence, following the pattern used for the Google provider (lines 18-20).
🔧 Proposed fix
- github: {
- clientId: process.env.GITHUB_CLIENT_ID as string,
- clientSecret: process.env.GITHUB_CLIENT_SECRET as string,
- getUserInfo: async (token) => {
+ ...(process.env.GITHUB_CLIENT_ID && process.env.GITHUB_CLIENT_SECRET
+ ? {
+ github: {
+ clientId: process.env.GITHUB_CLIENT_ID,
+ clientSecret: process.env.GITHUB_CLIENT_SECRET,
+ getUserInfo: async (token) => {
// existing implementation...
- },
- },
+ },
+ },
+ }
+ : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/libs/auth.server.ts` around lines 23 - 24, The GitHub provider is being
force-cast to string (process.env.GITHUB_CLIENT_ID as string /
process.env.GITHUB_CLIENT_SECRET as string), which hides missing env vars;
update the provider registration in auth.server.ts to mirror the Google pattern
by only adding the GitHub provider when both process.env.GITHUB_CLIENT_ID and
process.env.GITHUB_CLIENT_SECRET are present, remove the `as string` casts, and
conditionally push or spread the GitHub provider into the providers array used
by the function that builds the auth options (e.g., where the Google provider is
registered) so provider setup fails fast if envs are missing.
Owner
Author
There was a problem hiding this comment.
意図的に必須化しました。GitHub OAuth はこのプロジェクトでは必須設定なので、条件分岐は不要です。env 未設定時はアプリ起動時に better-auth 側でエラーになります。
There was a problem hiding this comment.
@coji, 了解しました！GitHub OAuth を必須とする設計判断ですね。better-auth が起動時にエラーを出してくれるので、環境変数の欠落は早期に検知できますね。ご説明ありがとうございます。
Comment on lines
+41
to
+43
| const formData = await request.formData() | ||
| const provider = String(formData.get('provider') || 'google') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n app/routes/_auth/login.tsx | head -80Repository: coji/upflow
Length of output: 2819
🏁 Script executed:
# Check if conform is installed and what version
cat package.json | grep -i conformRepository: coji/upflow
Length of output: 122
🏁 Script executed:
# Search for parseWithZod usage in the codebase to verify this pattern
rg "parseWithZod" app/routes --type ts --type tsx -A 3Repository: coji/upflow
Length of output: 82
🏁 Script executed:
# Search for parseWithZod usage in routes
rg "parseWithZod" app/routes -A 3Repository: coji/upflow
Length of output: 9080
🏁 Script executed:
# Check for other provider values in auth configuration
rg "provider" app/libs/auth.server -iRepository: coji/upflow
Length of output: 115
🏁 Script executed:
# Find auth.server file
find . -name "*auth*server*" -type fRepository: coji/upflow
Length of output: 80
🏁 Script executed:
# Check for auth import and provider handling
rg "auth.api.signInSocial" -A 5 -B 5Repository: coji/upflow
Length of output: 634
🏁 Script executed:
# Check auth.server implementation
cat -n app/libs/auth.server.ts | head -150Repository: coji/upflow
Length of output: 5520
Validate provider with Conform + Zod to align with route validation pattern.
Line 42 accepts any string via raw FormData casting, bypassing schema validation. All other routes in the codebase use parseWithZod for form validation. Please enforce provider as an enum (google | github).
Example refactor
+import { parseWithZod } from '@conform-to/zod/v4'
+import { z } from 'zod'
...
const formData = await request.formData()
- const provider = String(formData.get('provider') || 'google')
+ const submission = parseWithZod(formData, {
+ schema: z.object({
+ provider: z.enum(['google', 'github']).default('google'),
+ }),
+ })
+ if (submission.status !== 'success') {
+ throw new Response('Invalid provider', { status: 400 })
+ }
+ const { provider } = submission.value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/_auth/login.tsx` around lines 41 - 43, Replace the raw FormData
cast for provider with Conform + Zod validation: define a Zod schema (e.g.,
providerSchema or part of the existing form schema) that enforces provider as
z.enum(['google','github']), then call parseWithZod (the same helper used across
routes) against request.formData() and extract the validated provider value
instead of using String(formData.get(...)). Update any local references in this
file (the provider const and related logic that uses it) to use the validated
value returned by parseWithZod.
Owner
Author
There was a problem hiding this comment.
ログインページの action はユーザーが直接操作するボタン経由でのみ呼ばれ、provider 値は固定文字列 ('google' / 'github') です。Conform + Zod でのバリデーションは over-engineering と判断し、対応しません。
There was a problem hiding this comment.
@coji, understood! I appreciate your reasoning. While the validation would provide defense-in-depth against unexpected requests, I agree that in this case—with button-driven submissions using hardcoded strings—the risk is minimal and the added complexity may not be warranted.
Thank you for the explanation. I'll note your decision.
🧠 Learnings used
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-26T06:54:40.352Z
Learning: Applies to app/routes/$orgSlug/**/*.{ts,tsx} : For org-scoped routes, call `requireOrgMember` or `requireOrgAdmin` authentication guards BEFORE `parseWithZod(request.formData())` to prevent unauthenticated users from receiving validation errors
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-26T06:54:40.352Z
Learning: Applies to app/routes/**/*.{ts,tsx} : Use Conform with Zod for type-safe form validation with `parseWithZod` from 'conform-to/zod/v4'
…info Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 5s fetch timeouts (AbortSignal.timeout) to all GitHub API calls - Replace Promise.all with for-of early exit for org allowlist check - Normalize GitHub login comparison to case-insensitive (lower()) - Log errors in post-session GitHub linking catch handler - Fix env var names in docs (GITHUB_APP_CLIENT_* → GITHUB_CLIENT_*) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
CodeRabbit review feedback addressed in f04a416
|
8 tasks
4 tasks
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.
Summary
getUserInfoで安定動作）companyGithubUsers.loginで自動紐付け（session hook で毎回実行、冪等）companyGithubUsersに登録済みの GitHub ユーザーのみログイン許可（全 org の tenant DB を検索）Test plan
companyGithubUsersに未登録の GitHub アカウントでログイン → エラーメッセージが表示されるcompanyGithubUsers.userIdが自動で紐付けされることを確認pnpm typecheckが通ること🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores