Add GitHub, Google, and X social sign-in - #677
Closed
kentcdodds wants to merge 8 commits into
Closed
kentcdodds wants to merge 8 commits into
kentcdodds wants to merge 8 commits into
Conversation
Implement Epic Stack-style auth_connections linking, remix/auth OAuth providers, signed OAuth transaction cookies, login UI buttons, mock provider HTTP for tests, and deploy workflow secret sync hooks.
kentcdodds
marked this pull request as ready for review
July 8, 2026 14:16
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds social sign-in for GitHub, Google, and X, and updates account flows to distinguish OAuth-only accounts from accounts with usable passwords. It also adds the supporting routes, storage, docs, tests, and deploy/preview secret synchronization. ChangesAuthentication feature updates
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SocialAuthStartHandler
participant SocialAuthCallbackHandler
participant ResolveSocialAuthUser
participant DB
Browser->>SocialAuthStartHandler: GET /auth/:provider
SocialAuthStartHandler-->>Browser: redirect + OAuth transaction cookie
Browser->>SocialAuthCallbackHandler: GET /auth/:provider/callback
SocialAuthCallbackHandler->>ResolveSocialAuthUser: resolveSocialAuthUser(profile)
ResolveSocialAuthUser->>DB: lookup or create user and auth_connections
DB-->>ResolveSocialAuthUser: resolution result
ResolveSocialAuthUser-->>SocialAuthCallbackHandler: resolved user
SocialAuthCallbackHandler-->>Browser: session redirect
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
|
🔎 Preview deployed: https://kody-pr-677.kody-a99.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/worker/src/app/social-auth-provider-factory.ts (1)
38-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplication between real and mock provider creation paths.
createConfiguredSocialAuthProvider(lines 70–97) duplicates the provider-switching logic already in the main function (lines 38–67). Both branches construct the samecreate{Provider}AuthProvider({ clientId, clientSecret, redirectUri })call per provider. Consolidating into a single helper that accepts credentials would eliminate ~30 lines of duplicated branching.♻️ Proposed refactor: single helper for both paths
export function createSocialAuthProvider( env: Env, provider: SocialAuthProviderName, requestUrl: string | URL, ): AnySocialAuthProvider | null { const origin = new URL(requestUrl).origin const redirectUri = new URL( `${getSocialAuthStartPath(provider)}/callback`, origin, ) + let clientId: string | undefined + let clientSecret: string | undefined + if (isSocialAuthMockEnabled(env)) { - return createConfiguredSocialAuthProvider({ - provider, - clientId: mockClientId, - clientSecret: mockClientSecret, - redirectUri, - }) + clientId = mockClientId + clientSecret = mockClientSecret + } else { + clientId = readProviderCredentials(env, provider)?.clientId + clientSecret = readProviderCredentials(env, provider)?.clientSecret } - if (provider === 'github') { - const clientId = env.GITHUB_CLIENT_ID?.trim() - const clientSecret = env.GITHUB_CLIENT_SECRET?.trim() - if (!clientId || !clientSecret) return null - return createGitHubAuthProvider({ - clientId, - clientSecret, - redirectUri, - }) as AnySocialAuthProvider - } - - if (provider === 'google') { - const clientId = env.GOOGLE_CLIENT_ID?.trim() - const clientSecret = env.GOOGLE_CLIENT_SECRET?.trim() - if (!clientId || !clientSecret) return null - return createGoogleAuthProvider({ - clientId, - clientSecret, - redirectUri, - }) as AnySocialAuthProvider - } - - const clientId = env.X_CLIENT_ID?.trim() - const clientSecret = env.X_CLIENT_SECRET?.trim() if (!clientId || !clientSecret) return null - return createXAuthProvider({ - clientId, - clientSecret, - redirectUri, - }) as AnySocialAuthProvider + return createConfiguredSocialAuthProvider({ provider, clientId, clientSecret, redirectUri }) }🤖 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 `@packages/worker/src/app/social-auth-provider-factory.ts` around lines 38 - 97, The provider selection logic is duplicated between the main factory path and createConfiguredSocialAuthProvider, with the same github/google/X branching and create{Provider}AuthProvider calls repeated in both places. Refactor this by extracting a single helper that takes provider, clientId, clientSecret, and redirectUri, and have both the env-based path and createConfiguredSocialAuthProvider delegate to it. Keep the existing provider-specific constructors (createGitHubAuthProvider, createGoogleAuthProvider, createXAuthProvider) but centralize the switch so there is only one branching implementation.packages/worker/src/app/resolve-social-auth.node.test.ts (1)
165-245: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for unverified-email and new-user signup paths.
The current tests only cover existing linked/email-match flows. Please add regression cases for unverified provider email not auto-linking, invite-required new OAuth signup, and successful new-user connection creation.
🤖 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 `@packages/worker/src/app/resolve-social-auth.node.test.ts` around lines 165 - 245, The current resolveSocialAuthUser tests only cover linked-account and verified email-match paths, so add regression coverage for the missing signup branches. In resolve-social-auth.node.test.ts, extend the resolveSocialAuthUser suite with cases for an unverified provider email that must not auto-link to an existing user, an invite-required OAuth signup that should be blocked when no invite is present, and a successful new-user connection creation flow. Use the existing helpers like createResolveSocialAuthTestEnv and assert on the returned userId/isNewUser plus any new connection state to verify the behavior of resolveSocialAuthUser.packages/worker/src/app/loader-data.ts (1)
434-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
SocialAuthProviderNameinstead of duplicating the literal union.The
idfield uses'github' | 'google' | 'x'as a literal union, which duplicatesSocialAuthProviderNamefromsocial-auth-providers.ts. If a new provider is added toSocialAuthProviderName, this type won't catch the mismatch.♻️ Suggested fix: import and reference the shared type
+import { type SocialAuthProviderName } from '`#app/social-auth-providers.ts`' + export type LoginAuthLoaderData = { providers: Array<{ - id: 'github' | 'google' | 'x' + id: SocialAuthProviderName label: string startPath: string }> }🤖 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 `@packages/worker/src/app/loader-data.ts` around lines 434 - 439, The LoginAuthLoaderData type duplicates the provider name union instead of reusing the shared SocialAuthProviderName. Update the provider id field in LoginAuthLoaderData to reference SocialAuthProviderName directly, and add the needed import from social-auth-providers.ts so the loader data stays aligned with the central provider सूची when new providers are added.packages/worker/src/app/handlers/social-auth.ts (1)
127-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared provider validation to reduce duplication.
The start handler (lines 137-159) and callback handler (lines 191-213) contain identical logic:
isSocialAuthProviderNamecheck,isSocialAuthProviderConfiguredcheck,createSocialAuthProvidercall, and error redirect on failure. Extracting a helper would eliminate ~20 lines of copy-paste and prevent divergence when validation rules change.♻️ Suggested extraction
+function resolveSocialAuthProvider( + env: Env, + providerName: string, + request: Request, +) { + if (!isSocialAuthProviderName(providerName)) { + return { error: new Response('Not found', { status: 404 }) } as const + } + if (!isSocialAuthProviderConfigured(env, providerName)) { + return { error: buildLoginErrorRedirect(request, `${providerName} sign-in is not configured.`) } as const + } + const authProvider = createSocialAuthProvider(env, providerName, request.url) + if (!authProvider) { + return { error: buildLoginErrorRedirect(request, `${providerName} sign-in is not configured.`) } as const + } + return { authProvider, providerName } as const +}Then in both handlers:
- const providerName = params.provider - if (!isSocialAuthProviderName(providerName)) { - return new Response('Not found', { status: 404 }) - } - if (!isSocialAuthProviderConfigured(env, providerName)) { - return buildLoginErrorRedirect(request, `${providerName} sign-in is not configured.`) - } - const authProvider = createSocialAuthProvider(env, providerName, request.url) - if (!authProvider) { - return buildLoginErrorRedirect(request, `${providerName} sign-in is not configured.`) - } + const resolved = resolveSocialAuthProvider(env, params.provider, request) + if ('error' in resolved) return resolved.error + const { authProvider, providerName } = resolvedAlso applies to: 181-268
🤖 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 `@packages/worker/src/app/handlers/social-auth.ts` around lines 127 - 178, The start and callback handlers duplicate the same provider validation and provider creation flow, so extract that shared logic into a helper used by createSocialAuthStartHandler and the callback handler path. Move the repeated isSocialAuthProviderName, isSocialAuthProviderConfigured, createSocialAuthProvider, and failure redirect behavior into a single reusable function, then have both handlers call it and only keep their handler-specific start/callback behavior separate.packages/worker/src/app/handlers/social-auth.node.test.ts (1)
93-99: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd assertions for
stateandcodeVerifierin the OAuth transaction cookie.The test verifies
providerandreturnTobut does not assert thatstate(CSRF protection) andcodeVerifier(PKCE) are present and non-empty. These are security-critical fields — a regression that omits them could go undetected.🛡️ Proposed additions
expect(transaction?.provider).toBe('github') expect(transaction?.returnTo).toBe('/account') + expect(transaction?.state).toBeTruthy() + expect(transaction?.codeVerifier).toBeTruthy() })🤖 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 `@packages/worker/src/app/handlers/social-auth.node.test.ts` around lines 93 - 99, The social auth test reads the OAuth transaction via readOAuthTransaction but only checks provider and returnTo; update this test to also assert that the returned transaction includes non-empty state and codeVerifier fields. Use the existing transaction variable in social-auth.node.test.ts alongside the current provider and returnTo expectations so regressions that drop CSRF/PKCE data are caught.
🤖 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 `@packages/worker/migrations/0056-auth-connections.sql`:
- Around line 13-14: Remove the redundant auth_connections provider index: the
UNIQUE constraint on (provider_name, provider_id) in the migration already
creates the needed SQLite index, so delete the explicit CREATE INDEX statement
in the migration and keep the UNIQUE constraint as the single source of indexing
for those columns.
In `@packages/worker/src/app/handlers/social-auth.ts`:
- Around line 54-56: The user-not-found path in issueSessionForSocialAuth
currently redirects without audit logging, creating an observability gap. Update
the userRecord check in social-auth.ts to call logAuditEvent before
buildLoginErrorRedirect, using the same audit patterns already used in the
callback handler for failed social auth flows. Keep the existing redirect
behavior, but ensure the “user missing after resolution” case is recorded with
enough context to identify the social auth failure.
- Around line 40-56: `issueSessionForSocialAuth` is doing an unnecessary
`createDb(...).findOne(usersTable, ...)` lookup even though `input.userId` and
`input.email` are already available from `resolveSocialAuthUser`. Update the
session issuance flow in `issueSessionForSocialAuth` to use the `input` fields
directly for cookie/session creation, and only keep a minimal guard if you still
want to handle a deleted-user edge case. Remove the stale `userRecord`
dependency so `userId` and `email` come from the function arguments instead of a
second database roundtrip.
- Around line 224-228: The social-auth handler is casting around a real type
mismatch between finishSocialAuth and resolveSocialAuthUser. Update the typing
in social-auth.ts so the concrete social-auth result type is threaded through
finishSocialAuth and the call site that builds the object passed into
resolveSocialAuthUser, instead of using a Parameters<typeof
resolveSocialAuthUser>[0]['result'] cast. Make the return and argument types
align with OAuthResult<SocialAuthProfile, SocialAuthProviderName> so the result
value is inferred correctly without assertions.
In `@packages/worker/src/app/resolve-social-auth.ts`:
- Around line 192-201: The OAuth email handling in resolve-social-auth should
only trust provider emails when isProviderEmailVerified reports true. Update the
email selection in the flow around readProfileEmail, syntheticEmailForProvider,
and getAvailableUsername so unverified provider emails do not drive account
linking, and use a synthetic email for new users when verification is absent.
Also guard the auto-linking path in the section that links by profileEmail so it
only matches existing accounts on verified email claims, leaving unverified
claims unlinked.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/social-auth.node.test.ts`:
- Around line 93-99: The social auth test reads the OAuth transaction via
readOAuthTransaction but only checks provider and returnTo; update this test to
also assert that the returned transaction includes non-empty state and
codeVerifier fields. Use the existing transaction variable in
social-auth.node.test.ts alongside the current provider and returnTo
expectations so regressions that drop CSRF/PKCE data are caught.
In `@packages/worker/src/app/handlers/social-auth.ts`:
- Around line 127-178: The start and callback handlers duplicate the same
provider validation and provider creation flow, so extract that shared logic
into a helper used by createSocialAuthStartHandler and the callback handler
path. Move the repeated isSocialAuthProviderName,
isSocialAuthProviderConfigured, createSocialAuthProvider, and failure redirect
behavior into a single reusable function, then have both handlers call it and
only keep their handler-specific start/callback behavior separate.
In `@packages/worker/src/app/loader-data.ts`:
- Around line 434-439: The LoginAuthLoaderData type duplicates the provider name
union instead of reusing the shared SocialAuthProviderName. Update the provider
id field in LoginAuthLoaderData to reference SocialAuthProviderName directly,
and add the needed import from social-auth-providers.ts so the loader data stays
aligned with the central provider सूची when new providers are added.
In `@packages/worker/src/app/resolve-social-auth.node.test.ts`:
- Around line 165-245: The current resolveSocialAuthUser tests only cover
linked-account and verified email-match paths, so add regression coverage for
the missing signup branches. In resolve-social-auth.node.test.ts, extend the
resolveSocialAuthUser suite with cases for an unverified provider email that
must not auto-link to an existing user, an invite-required OAuth signup that
should be blocked when no invite is present, and a successful new-user
connection creation flow. Use the existing helpers like
createResolveSocialAuthTestEnv and assert on the returned userId/isNewUser plus
any new connection state to verify the behavior of resolveSocialAuthUser.
In `@packages/worker/src/app/social-auth-provider-factory.ts`:
- Around line 38-97: The provider selection logic is duplicated between the main
factory path and createConfiguredSocialAuthProvider, with the same
github/google/X branching and create{Provider}AuthProvider calls repeated in
both places. Refactor this by extracting a single helper that takes provider,
clientId, clientSecret, and redirectUri, and have both the env-based path and
createConfiguredSocialAuthProvider delegate to it. Keep the existing
provider-specific constructors (createGitHubAuthProvider,
createGoogleAuthProvider, createXAuthProvider) but centralize the switch so
there is only one branching implementation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae97613-ce60-4af8-9167-251c1d94f19d
📒 Files selected for processing (24)
.github/workflows/deploy.yml.github/workflows/preview.ymldocs/contributing/architecture/authentication.mddocs/contributing/architecture/primitives.yamldocs/contributing/environment-variables.mdpackages/worker/.env.examplepackages/worker/client/routes/login.tsxpackages/worker/migrations/0056-auth-connections.sqlpackages/worker/src/app/account-data-targets.tspackages/worker/src/app/handlers/auth-page.tspackages/worker/src/app/handlers/social-auth.node.test.tspackages/worker/src/app/handlers/social-auth.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/oauth-transaction.tspackages/worker/src/app/resolve-social-auth.node.test.tspackages/worker/src/app/resolve-social-auth.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/app/social-auth-flow.tspackages/worker/src/app/social-auth-mock.tspackages/worker/src/app/social-auth-provider-factory.tspackages/worker/src/app/social-auth-providers.tspackages/worker/src/db.tspackages/worker/src/env-schema.ts
Comment on lines
+54
to
+56
| if (!userRecord) { | ||
| return buildLoginErrorRedirect(input.request, 'Unable to sign in.') | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing audit log for user-not-found edge case.
The callback handler logs failures via logAuditEvent (lines 242-249, 254-261), but issueSessionForSocialAuth silently redirects on user-not-found without logging. This creates an observability gap for a rare but security-relevant scenario (user deleted between resolution and session issuance).
🛡️ Suggested fix: add audit log before redirect
if (!userRecord) {
+ void logAuditEvent({
+ category: 'auth',
+ action: 'social_login',
+ result: 'failure',
+ email: input.email,
+ reason: 'user_not_found_after_resolution',
+ })
return buildLoginErrorRedirect(input.request, 'Unable to sign in.')
}📝 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 (!userRecord) { | |
| return buildLoginErrorRedirect(input.request, 'Unable to sign in.') | |
| } | |
| if (!userRecord) { | |
| void logAuditEvent({ | |
| category: 'auth', | |
| action: 'social_login', | |
| result: 'failure', | |
| email: input.email, | |
| reason: 'user_not_found_after_resolution', | |
| }) | |
| return buildLoginErrorRedirect(input.request, 'Unable to sign in.') | |
| } |
🤖 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 `@packages/worker/src/app/handlers/social-auth.ts` around lines 54 - 56, The
user-not-found path in issueSessionForSocialAuth currently redirects without
audit logging, creating an observability gap. Update the userRecord check in
social-auth.ts to call logAuditEvent before buildLoginErrorRedirect, using the
same audit patterns already used in the callback handler for failed social auth
flows. Keep the existing redirect behavior, but ensure the “user missing after
resolution” case is recorded with enough context to identify the social auth
failure.
Only Google OIDC email_verified claims can auto-link to an existing account or mark a new signup verified. GitHub and X no longer trust unverified IdP email for account linking.
- Trust GitHub profile emails for auto-link while keeping Google email_verified gating and X synthetic addresses - Remove redundant auth_connections provider index from migration - Clear OAuth transaction cookie on callback failures - Install mock OAuth fetch when SOCIAL_AUTH_MOCK is enabled - Keep social provider buttons visible across SPA login/signup toggle - Refactor provider factory and handler validation helpers - Drop redundant user lookup in session issuance; fix OAuth result typing - Add regression tests for GitHub link, unverified Google, and invite gate
- Allow email change and account deletion without a password when the account uses an unusable OAuth/admin sentinel hash - Expose hasUsablePassword on account profile loader data and hide the password field for OAuth-only users - Add hasUsablePasswordHash helper and regression coverage - Fix social-auth handler formatting that failed CI format:check
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/password-hash.ts (1)
108-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant prefix branch and dead code after the early return.
Since
hasUsablePasswordHash(normalizedHash)already asserts thepbkdf2_sha256$prefix, theif (normalizedHash.startsWith(...))guard on Line 111 is now always true and the trailingreturn falseon Line 136 is unreachable. The body can be flattened for clarity.♻️ Flatten the always-true branch
const normalizedHash = storedHash.trim() if (!hasUsablePasswordHash(normalizedHash)) { return false } - if (normalizedHash.startsWith(`${passwordHashPrefix}$`)) { - const [prefix, iterationsRaw, saltHex, hashHex, ...extra] = - normalizedHash.split('$') - if (prefix !== passwordHashPrefix || extra.length > 0) { - return false - } - if (!iterationsRaw || !/^\d+$/.test(iterationsRaw)) return false - const iterations = Number(iterationsRaw) - const salt = saltHex ? fromHex(saltHex) : null - const hash = hashHex ? fromHex(hashHex) : null - if (!Number.isSafeInteger(iterations) || iterations < 1 || !salt || !hash) { - return false - } - if (iterations > maxPasswordHashIterations) { - return false - } - const derived = await derivePasswordKey( - password, - salt, - iterations, - hash.length, - ) - return timingSafeEqual(derived, hash) - } - - return false + const [prefix, iterationsRaw, saltHex, hashHex, ...extra] = + normalizedHash.split('$') + if (prefix !== passwordHashPrefix || extra.length > 0) { + return false + } + if (!iterationsRaw || !/^\d+$/.test(iterationsRaw)) return false + const iterations = Number(iterationsRaw) + const salt = saltHex ? fromHex(saltHex) : null + const hash = hashHex ? fromHex(hashHex) : null + if (!Number.isSafeInteger(iterations) || iterations < 1 || !salt || !hash) { + return false + } + if (iterations > maxPasswordHashIterations) { + return false + } + const derived = await derivePasswordKey(password, salt, iterations, hash.length) + return timingSafeEqual(derived, hash)🤖 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 `@packages/shared/src/password-hash.ts` around lines 108 - 137, Flatten the password verification logic in the `verifyPasswordHash` path by removing the redundant `normalizedHash.startsWith(`${passwordHashPrefix}$`)` branch, since `hasUsablePasswordHash(normalizedHash)` already guarantees the prefix. Keep the parsing and validation of `prefix`, `iterationsRaw`, `saltHex`, `hashHex`, and the `derivePasswordKey`/`timingSafeEqual` checks directly in the main flow, and remove the unreachable trailing `return false` after the branch to make the control flow clearer.
🤖 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.
Nitpick comments:
In `@packages/shared/src/password-hash.ts`:
- Around line 108-137: Flatten the password verification logic in the
`verifyPasswordHash` path by removing the redundant
`normalizedHash.startsWith(`${passwordHashPrefix}$`)` branch, since
`hasUsablePasswordHash(normalizedHash)` already guarantees the prefix. Keep the
parsing and validation of `prefix`, `iterationsRaw`, `saltHex`, `hashHex`, and
the `derivePasswordKey`/`timingSafeEqual` checks directly in the main flow, and
remove the unreachable trailing `return false` after the branch to make the
control flow clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65d3b3d9-0c43-4360-a3b4-1dadd32c7443
📒 Files selected for processing (27)
packages/shared/src/password-hash.node.test.tspackages/shared/src/password-hash.tspackages/worker/client/loader-data-context.node.test.tspackages/worker/client/navigation-data.node.test.tspackages/worker/client/routes/account.tsxpackages/worker/client/routes/login.tsxpackages/worker/migrations/0056-auth-connections.sqlpackages/worker/src/app/account-profile-data.tspackages/worker/src/app/handler.tspackages/worker/src/app/handlers/account-delete.tspackages/worker/src/app/handlers/account-email-change.node.test.tspackages/worker/src/app/handlers/account-email-change.tspackages/worker/src/app/handlers/account-profile.node.test.tspackages/worker/src/app/handlers/account-profile.tspackages/worker/src/app/handlers/account.tspackages/worker/src/app/handlers/social-auth.node.test.tspackages/worker/src/app/handlers/social-auth.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/resolve-social-auth.node.test.tspackages/worker/src/app/resolve-social-auth.tspackages/worker/src/app/social-auth-flow.tspackages/worker/src/app/social-auth-mock.tspackages/worker/src/app/social-auth-provider-factory.tspackages/worker/src/app/social-auth-provider-names.tspackages/worker/src/app/social-auth-providers.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/tsconfig-client.json
💤 Files with no reviewable changes (1)
- packages/worker/migrations/0056-auth-connections.sql
✅ Files skipped from review due to trivial changes (3)
- packages/worker/src/app/social-auth-provider-names.ts
- packages/worker/client/loader-data-context.node.test.ts
- packages/worker/tsconfig-client.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/app/handlers/social-auth.node.test.ts
- packages/worker/client/routes/login.tsx
- packages/worker/src/app/social-auth-flow.ts
- packages/worker/src/app/social-auth-providers.ts
- packages/worker/src/app/handlers/social-auth.ts
- packages/worker/src/app/social-auth-provider-factory.ts
- packages/worker/src/app/resolve-social-auth.ts
Refuse to attach a social identity to an existing password account when email_verified_at is null, so an unverified signup cannot squat an address and later capture a real owner's IdP sign-in.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit abce768. Configure here.
- Keep the typed invite field in component state so social buttons forward it even when it is not in the URL - Carry inviteCode across login/signup toggle links alongside redirectTo
This branch was successfully deployed
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
2 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.
Summary
Adds optional OAuth sign-in with GitHub, Google, and X on
/loginand/signup, following Epic Stack’s connection model and Remix 3’s built-inremix/authproviders.auth_connectionstable linksprovider_name+provider_id→users.idGET /auth/{github,google,x}andGET /auth/{provider}/callbackkody_oauth_transactioncookie (WebAuthn-style)email_verified); new users get OAuth-only accounts (sentinel password hash)inviteCodequery param)SOCIAL_AUTH_MOCK=1(or test env) installs mock OAuth fetch at worker startup for local dev and unit testsReview feedback addressed (
5342a59b)(provider_name, provider_id)index (UNIQUE already indexes it)email_verifiedgating; X stays synthetickody_oauth_transactioncookie on OAuth callback failures/auth/providers.jsonso social buttons persist across SPA login ↔ signup togglefinishSocialAuth; added regression testsProvider app setup
Register OAuth apps on each provider using your deployment origin (production:
https://heykody.dev, or your preview URL). Callback paths:https://<your-domain>/auth/github/callbackhttps://<your-domain>/auth/google/callbackhttps://<your-domain>/auth/x/callbackGitHub
https://<your-domain>/auth/github/callbackGoogle
https://<your-domain>/auth/google/callbackemail,profile,openidscopesX (Twitter)
https://<your-domain>/auth/x/callbackusers.read(default in code)GitHub Actions secrets
Add these repository secrets (Settings → Secrets and variables → Actions). All are optional — omit a pair to hide that provider’s button.
GITHUB_CLIENT_IDGITHUB_CLIENT_SECRETGOOGLE_CLIENT_IDGOOGLE_CLIENT_SECRETX_CLIENT_IDX_CLIENT_SECRETProduction (
.github/workflows/deploy.yml) and preview (.github/workflows/preview.yml) already sync these viatools/ci/sync-worker-secrets.tswhen the secrets exist.Local development
Add to
packages/worker/.env(see.env.example):GITHUB_CLIENT_ID=... GITHUB_CLIENT_SECRET=... # etc.Or use mock mode without real apps:
Testing
social-auth.node.test.ts— OAuth start redirect + transaction cookie (mock fetch)resolve-social-auth.node.test.ts— connection lookup, GitHub/Google email auto-link, unverified Google skip, invite gateRun:
npx nx run worker:test -- social-authMigration
Apply
packages/worker/migrations/0056-auth-connections.sql(included in normal D1 migrate on deploy).System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@8367cec· Head:5342a59bClassification: extends — adds OAuth social sign-in paths and
auth_connectionsstorage on top of existing browser session auth.Primitives touched
app-sessionsd1-app-dbauth_connectionstable + account deletion cascadeSystem map
Invariants
Per-user isolation preserved:
auth_connectionsrows are deleted viaaccount-data-targetsusingdb_user_idcascade on account deletion.Summary by CodeRabbit
.envexample for the new social sign-in settings.