fix(security): remove all password-based authentication#861
Merged
2witstudios merged 8 commits intomasterfrom Apr 9, 2026
Merged
fix(security): remove all password-based authentication#8612witstudios merged 8 commits intomasterfrom
2witstudios merged 8 commits intomasterfrom
Conversation
Completely removes password auth from the entire codebase. Passwords are obsolete and represent a credential stuffing threat vector — passkeys (WebAuthn) and magic links are already fully implemented as replacements. - Delete 5 password API routes (login, signup, mobile login/signup, password change) - Delete PasswordLoginForm component and all password-related test files - Remove password column from users schema + 'both' from AuthProvider enum - Remove bcryptjs dependency from all 3 packages - Update 5 OAuth callbacks to remove 'user.password ? both : provider' logic - Update on-prem signin to use magic link instead of password form - Remove password change UI from account settings - Remove password fields from admin user creation - Rewrite setup-onprem-admin.ts and control-plane admin seeder (no passwords) - Remove useAuth login function (called deleted /api/auth/login endpoint) - Update marketing docs and privacy page - Add data migration for provider='both' users → their OAuth provider - Annotate password_change in activity enums as @deprecated (historical) 72 files changed, 99 insertions, 7144 deletions. Zero password auth traces remain (verified via grep). On-prem passkey UX follow-up tracked in #860. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 17 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR removes password-based authentication across the codebase: deletes bcrypt usage and BCRYPT constants, drops the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProvisioningEngine
participant AdminSeeder
participant DB
participant EmailService
Client->>ProvisioningEngine: provision(tenant, adminEmail, adminName)
ProvisioningEngine->>AdminSeeder: seedAdmin(adminEmail, adminName)
AdminSeeder->>DB: INSERT users (id, email, role, name) -- no password
DB-->>AdminSeeder: inserted / alreadyExisted result
AdminSeeder-->>ProvisioningEngine: { email, alreadyExisted }
ProvisioningEngine->>EmailService: sendProvisioningEmail({ email }) -- no temporaryPassword
EmailService-->>Client: email delivered (async)
ProvisioningEngine-->>Client: return { tenantId, email }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/web/src/lib/auth/device-auth-helpers.ts (1)
12-12: Consider whether 'email' is the most semantically accurate default.While 'email' is a reasonable default now that password authentication is removed (and magic links use email), you might consider alternatives like
'magic_link','passwordless', or'unknown'if you want the log message to be more semantically precise when the provider is not explicitly passed.That said, since all production callers pass the provider explicitly (per the relevant code snippets) and this value only affects logging, the current choice is acceptable. This is more of a semantic consistency consideration than a functional issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/auth/device-auth-helpers.ts` at line 12, The default parameter value provider = 'email' in device-auth-helpers.ts is semantically imprecise now that password auth is gone; change the default to a more accurate token such as 'magic_link' or 'passwordless' (or 'unknown' if you prefer a neutral sentinel) wherever the provider parameter is defined, and update any related log messages that rely on this default (search for uses of the provider parameter and logging in device-auth-helpers.ts) so logs reflect the new default string; ensure callers that pass provider explicitly remain unchanged and adjust tests or fixtures expecting 'email' as the implicit default.packages/db/drizzle/0094_wakeful_champions.sql (1)
1-13: Hand-written migration is appropriate here, but document the edge case.Per coding guidelines, SQL files should be generated via
pnpm db:generate. However, this migration requires custom data migration logic (converting'both'to specific providers) that Drizzle cannot auto-generate, making hand-authoring necessary.Edge case to document: Users who have both
googleIdANDappleIdwill be assigned to'google'(line 2 matches first). Consider adding a comment explaining this prioritization:-- Migrate users with provider='both' to their OAuth provider before enum change +-- Note: Users with both googleId and appleId are assigned to 'google' (first linked provider takes priority) UPDATE "users" SET "provider" = 'google' WHERE "provider" = 'both' AND "googleId" IS NOT NULL;--> statement-breakpoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0094_wakeful_champions.sql` around lines 1 - 13, Add an explicit comment in this hand-written migration documenting the edge case where users with both googleId and appleId are assigned to 'google' due to the ordering of the UPDATE statements (the UPDATE setting provider = 'google' runs before the one for 'apple'); mention the prioritization rationale (or note if this is intentional) next to the first UPDATE ("UPDATE \"users\" SET \"provider\" = 'google'...") and optionally describe how to change handling if different behavior is desired; also mention that this was hand-written because Drizzle cannot auto-generate the data-migration for the AuthProvider enum change.apps/control-plane/src/services/__tests__/provisioning-engine.test.ts (1)
363-363: Rename stale test description (“with credentials”).The test no longer validates credential data, so the title is outdated and can cause confusion during triage.
✏️ Suggested rename
- test('given a new admin user, should send provisioning email with credentials', async () => { + test('given a new admin user, should send provisioning email', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/control-plane/src/services/__tests__/provisioning-engine.test.ts` at line 363, Rename the stale test title string "given a new admin user, should send provisioning email with credentials" to reflect actual assertions (e.g., "given a new admin user, should send provisioning email" or "given a new admin user, should send provisioning email to the admin") inside the test block in provisioning-engine.test.ts so the description no longer mentions credentials that are not asserted; update only the test name string used in the test(...) call.apps/control-plane/src/services/admin-seeder.ts (1)
14-17: Verify the hardcoded admin name matches organizational expectations.The
namecolumn is hardcoded to'Admin'(line 41). This is a minor consideration—confirm this generic name is acceptable for all tenant admin users, or if it should derive from the owner email or be configurable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/control-plane/src/services/admin-seeder.ts` around lines 14 - 17, The admin name is hardcoded to 'Admin' in the seeding logic (update where the admin user record is created in admin-seeder.ts); change it so the name is either derived from the owner email (e.g., local-part or capitalized display name) or passed in as a configurable parameter to the seeding function (e.g., add a name param to the existing seed function or use a config value), and update any call sites to supply that value; ensure the code that constructs the user record replaces the literal 'Admin' with the derived or injected name while preserving the existing SeedResult behavior.
🤖 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/web/src/app/api/auth/apple/native/route.ts`:
- Line 104: The code currently forces provider: 'apple' which will overwrite an
existing user's provider and can downgrade linked accounts; change the
assignment so that when an existing user object is present you preserve
user.provider and only set provider='apple' for newly created users (e.g., check
if user exists via the user variable or createOrUpdateUser/upsertUser flow and
set provider = user?.provider ?? 'apple'). Ensure any call sites that write the
provider field (the object containing provider) use this conditional logic so
existing user.provider is not overwritten.
In `@apps/web/src/app/api/auth/google/callback/route.ts`:
- Line 138: The hardcoded provider: 'google' risks overwriting an existing
user's provider; change the assignment to preserve any existing provider by
using the current user object (e.g., provider: user?.provider ?? 'google') or
merge logic that prefers user.provider when present; update the provider field
in the OAuth callback/update logic (the object containing provider, and the user
variable used to load the existing account) so existing linked-provider state is
retained.
In `@apps/web/src/app/api/auth/google/native/route.ts`:
- Line 115: The route currently unconditionally sets provider: 'google' which
overwrites an existing linked provider; change the assignment so it derives
newProvider from the existing user record instead of hardcoding 'google' (e.g.,
compute newProvider = user?.provider ?? 'google' or otherwise apply your
deterministic merge policy) and use that newProvider wherever the code currently
sets provider; update references to provider, newProvider, user (or
existingUser) in this route handler to preserve previously linked providers
rather than forcing 'google'.
In `@apps/web/src/app/api/auth/google/one-tap/route.ts`:
- Line 138: The code unconditionally sets provider: 'google' which can overwrite
an existing user's linked-provider; change the logic in the one-tap route
handler that constructs the user/create-or-update payload (the object with
provider: 'google') to compute provider from the existing user record if present
(e.g., provider = existingUser?.provider ?? 'google') and use that computed
value when calling the user upsert/update function so we preserve an
already-linked provider instead of always overwriting with 'google'.
In `@packages/db/src/schema/auth.ts`:
- Line 138: The VerificationType union in
packages/lib/src/auth/verification-utils.ts must be expanded to include
'webauthn_signup' so it matches the DB schema and usages; update the exported
type alias VerificationType to "export type VerificationType =
'email_verification' | 'magic_link' | 'webauthn_signup';" and then run
TypeScript checks to ensure callers like the code in passkey-service.ts that
call db.insert(..., type: 'webauthn_signup', ...) no longer bypass the type
system.
In `@scripts/setup-onprem-admin.ts`:
- Around line 6-7: Update the on-prem deployment docs to remove the stale
--password flag from the setup:admin example: replace the example invocation
that includes --password "SecurePassword123!" with the current passwordless
invocation for the setup:admin script (showing only --email and --name flags),
ensuring the docs reflect the CLI contract change and the exact snippet matches
the example used in scripts/setup-onprem-admin.ts.
---
Nitpick comments:
In `@apps/control-plane/src/services/__tests__/provisioning-engine.test.ts`:
- Line 363: Rename the stale test title string "given a new admin user, should
send provisioning email with credentials" to reflect actual assertions (e.g.,
"given a new admin user, should send provisioning email" or "given a new admin
user, should send provisioning email to the admin") inside the test block in
provisioning-engine.test.ts so the description no longer mentions credentials
that are not asserted; update only the test name string used in the test(...)
call.
In `@apps/control-plane/src/services/admin-seeder.ts`:
- Around line 14-17: The admin name is hardcoded to 'Admin' in the seeding logic
(update where the admin user record is created in admin-seeder.ts); change it so
the name is either derived from the owner email (e.g., local-part or capitalized
display name) or passed in as a configurable parameter to the seeding function
(e.g., add a name param to the existing seed function or use a config value),
and update any call sites to supply that value; ensure the code that constructs
the user record replaces the literal 'Admin' with the derived or injected name
while preserving the existing SeedResult behavior.
In `@apps/web/src/lib/auth/device-auth-helpers.ts`:
- Line 12: The default parameter value provider = 'email' in
device-auth-helpers.ts is semantically imprecise now that password auth is gone;
change the default to a more accurate token such as 'magic_link' or
'passwordless' (or 'unknown' if you prefer a neutral sentinel) wherever the
provider parameter is defined, and update any related log messages that rely on
this default (search for uses of the provider parameter and logging in
device-auth-helpers.ts) so logs reflect the new default string; ensure callers
that pass provider explicitly remain unchanged and adjust tests or fixtures
expecting 'email' as the implicit default.
In `@packages/db/drizzle/0094_wakeful_champions.sql`:
- Around line 1-13: Add an explicit comment in this hand-written migration
documenting the edge case where users with both googleId and appleId are
assigned to 'google' due to the ordering of the UPDATE statements (the UPDATE
setting provider = 'google' runs before the one for 'apple'); mention the
prioritization rationale (or note if this is intentional) next to the first
UPDATE ("UPDATE \"users\" SET \"provider\" = 'google'...") and optionally
describe how to change handling if different behavior is desired; also mention
that this was hand-written because Drizzle cannot auto-generate the
data-migration for the AuthProvider enum change.
🪄 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
Run ID: b4130d07-ed21-4490-a319-8615d303213e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (73)
apps/control-plane/package.jsonapps/control-plane/src/services/__tests__/admin-seeder.test.tsapps/control-plane/src/services/__tests__/provisioning-engine.test.tsapps/control-plane/src/services/admin-seeder.tsapps/control-plane/src/services/provisioning-engine.tsapps/marketing/src/app/docs/security/authentication/page.tsxapps/marketing/src/app/privacy/page.tsxapps/marketing/src/components/sections/SecurityPageSections.tsxapps/processor/src/services/__tests__/user-validator.test.tsapps/web/package.jsonapps/web/src/app/api/account/__tests__/get-patch-route.test.tsapps/web/src/app/api/account/password/__tests__/route.test.tsapps/web/src/app/api/account/password/route.tsapps/web/src/app/api/account/route.tsapps/web/src/app/api/admin/users/[userId]/gift-subscription/__tests__/route.security.test.tsapps/web/src/app/api/admin/users/create/route.tsapps/web/src/app/api/auth/__tests__/login-redirect.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/me.test.tsapps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/mobile-signup.test.tsapps/web/src/app/api/auth/__tests__/signup-redirect.test.tsapps/web/src/app/api/auth/__tests__/signup.test.tsapps/web/src/app/api/auth/apple/callback/__tests__/route.test.tsapps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/apple/native/__tests__/route.test.tsapps/web/src/app/api/auth/apple/native/route.tsapps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/google/__tests__/open-redirect-protection.test.tsapps/web/src/app/api/auth/google/callback/__tests__/route.test.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/native/__tests__/route.test.tsapps/web/src/app/api/auth/google/native/route.tsapps/web/src/app/api/auth/google/one-tap/__tests__/route.test.tsapps/web/src/app/api/auth/google/one-tap/route.tsapps/web/src/app/api/auth/login/__tests__/desktop-login.test.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/api/auth/mobile/signup/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/auth/signin/page.tsxapps/web/src/app/settings/account/page.tsxapps/web/src/components/admin/CreateUserForm.tsxapps/web/src/components/auth/PasswordLoginForm.tsxapps/web/src/components/auth/index.tsapps/web/src/hooks/__tests__/useAuth.test.tsapps/web/src/hooks/useAuth.tsapps/web/src/lib/auth/__tests__/admin-role-version.test.tsapps/web/src/lib/auth/device-auth-helpers.tsknip.jsonpackages/db/drizzle/0094_wakeful_champions.sqlpackages/db/drizzle/meta/0094_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/package.jsonpackages/db/src/schema/auth.tspackages/db/src/test/factories.tspackages/lib/src/auth/__tests__/constants.test.tspackages/lib/src/auth/__tests__/oauth-utils-unit.test.tspackages/lib/src/auth/__tests__/session-service.test.tspackages/lib/src/auth/__tests__/verification-utils.test.tspackages/lib/src/auth/account-lockout.tspackages/lib/src/auth/constants.tspackages/lib/src/auth/magic-link-service.test.tspackages/lib/src/auth/oauth-types.tspackages/lib/src/auth/oauth-utils.tspackages/lib/src/auth/passkey-service.test.tspackages/lib/src/auth/verification-utils.tspackages/lib/src/email-templates/TenantProvisioningCompleteEmail.tsxpackages/lib/src/monitoring/activity-logger.tsscripts/__tests__/setup.tsscripts/seed-loadtest-user.tsscripts/setup-onprem-admin.tstests/load/auth-baseline.k6.js
💤 Files with no reviewable changes (39)
- apps/control-plane/package.json
- packages/db/package.json
- apps/web/src/app/api/auth/google/tests/open-redirect-protection.test.ts
- apps/web/package.json
- apps/web/src/app/api/admin/users/[userId]/gift-subscription/tests/route.security.test.ts
- apps/processor/src/services/tests/user-validator.test.ts
- apps/web/src/app/api/auth/google/tests/google-callback-redirect.test.ts
- packages/lib/src/auth/tests/session-service.test.ts
- packages/lib/src/auth/passkey-service.test.ts
- apps/web/src/app/api/auth/google/callback/tests/route.test.ts
- packages/lib/src/auth/magic-link-service.test.ts
- packages/lib/src/auth/constants.ts
- apps/web/src/app/api/auth/google/one-tap/tests/route.test.ts
- packages/lib/src/auth/tests/constants.test.ts
- apps/web/src/app/api/account/route.ts
- apps/web/src/hooks/tests/useAuth.test.ts
- packages/db/src/test/factories.ts
- apps/web/src/app/api/account/tests/get-patch-route.test.ts
- apps/web/src/app/api/auth/apple/native/tests/route.test.ts
- apps/web/src/app/api/auth/google/native/tests/route.test.ts
- apps/web/src/lib/auth/tests/admin-role-version.test.ts
- scripts/seed-loadtest-user.ts
- apps/web/src/app/api/auth/tests/login-redirect.test.ts
- apps/web/src/app/api/account/password/tests/route.test.ts
- apps/control-plane/src/services/provisioning-engine.ts
- apps/web/src/app/api/auth/login/tests/desktop-login.test.ts
- apps/web/src/app/api/auth/signup/route.ts
- apps/web/src/app/api/account/password/route.ts
- scripts/tests/setup.ts
- tests/load/auth-baseline.k6.js
- apps/web/src/app/api/auth/login/route.ts
- apps/web/src/components/auth/PasswordLoginForm.tsx
- apps/web/src/app/api/auth/tests/mobile-signup.test.ts
- apps/web/src/app/api/auth/mobile/signup/route.ts
- apps/web/src/app/api/auth/tests/signup.test.ts
- apps/web/src/app/api/auth/tests/signup-redirect.test.ts
- apps/web/src/app/api/auth/mobile/login/route.ts
- apps/web/src/app/api/auth/tests/mobile-login.test.ts
- apps/web/src/app/api/auth/tests/login.test.ts
P1 fixes: - Add passkey login button to on-prem signin page alongside magic link, preventing lockout for users who registered passkeys - Generate one-time magic link in setup-onprem-admin.ts, enabling initial sign-in without SMTP configuration P2/Critical fixes: - Remove password column from tenant-export USER_COLUMNS - Remove password from scripts/__tests__/setup.ts SQL fixture - Remove password from tenant-import test fixture High: - Update marketing API auth docs: remove deleted /api/auth/signup and /api/auth/login endpoints, add magic link endpoint - Remove deleted /api/account/password from users API docs Medium: - Fix stale middleware comment mentioning "email/password" - Remove dead login/signup routes from middleware public route list - Remove dead CSRF exemptions for /api/auth/login and /api/auth/signup - Remove deleted routes from check-fetch-auth.js Low: - Fix privacy page: "JWT tokens" → "opaque session tokens" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (3)
apps/web/middleware.ts (2)
31-39:⚠️ Potential issue | 🔴 CriticalOn-prem/tenant route block conflicts with new passwordless auth paths
Because Line 49 applies this prefix block first, keeping
/api/auth/magic-link/and/api/auth/passkey/inCLOUD_ONLY_ROUTE_PREFIXESwill return 404 for the exact auth routes now required after password removal.Suggested fix
const CLOUD_ONLY_ROUTE_PREFIXES = [ '/api/stripe/', '/api/auth/google/', '/api/auth/apple/', - '/api/auth/magic-link/', - '/api/auth/passkey/', '/api/auth/signup', '/api/auth/mobile/signup', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/middleware.ts` around lines 31 - 39, The CLOUD_ONLY_ROUTE_PREFIXES array currently contains '/api/auth/magic-link/' and '/api/auth/passkey/' which causes the middleware’s cloud-only prefix check to short-circuit and 404 the new passwordless auth endpoints; update the middleware by removing those two entries from CLOUD_ONLY_ROUTE_PREFIXES (or otherwise ensure the tenant/on-prem route check runs before this cloud-only check) so that the passwordless routes (handled by the auth handlers) are not blocked; verify the affected symbols CLOUD_ONLY_ROUTE_PREFIXES and the middleware route-check logic (the function that iterates/uses CLOUD_ONLY_ROUTE_PREFIXES) are updated accordingly and consider normalizing prefixes without/with trailing slashes to match actual route paths.
100-112:⚠️ Potential issue | 🔴 CriticalMissing public bypass for login bootstrap endpoints causes 401 for signed-out users
/api/auth/login-csrfand magic-link send endpoints are part of unauthenticated sign-in initiation, but this bypass list doesn’t include them. They will hit the session-cookie check and fail for logged-out users.Suggested fix
if ( + pathname.startsWith('/api/auth/login-csrf') || pathname.startsWith('/api/auth/csrf') || + pathname.startsWith('/api/auth/magic-link/') || pathname.startsWith('/api/auth/google') || pathname.startsWith('/api/auth/passkey/authenticate') || pathname.startsWith('/api/auth/device/') ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/middleware.ts` around lines 100 - 112, The pathname bypass condition that checks multiple public API routes currently omits unauthenticated sign-in initiation endpoints, causing 401s for signed-out users; update the if-condition that uses pathname.startsWith(...) to also allow '/api/auth/login-csrf' and the magic-link send endpoint(s) (e.g. add pathname.startsWith('/api/auth/magic') or pathname.startsWith('/api/auth/magic/send') as appropriate) so these endpoints skip the session-cookie check and remain accessible without a session.apps/marketing/src/app/docs/api/auth/page.tsx (1)
45-53:⚠️ Potential issue | 🟡 MinorProvider response docs still include removed
bothvalueLine 52 still documents
"provider": "email | google | both", but this PR removesbothfrom the enum. Please update this response shape to avoid stale auth contract docs.Suggested doc fix
- "provider": "email | google | both", + "provider": "email | google",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/marketing/src/app/docs/api/auth/page.tsx` around lines 45 - 53, The Response example still lists the removed enum value "both" for the provider field; update the Response JSON example in page.tsx so "provider" only documents "email | google" (or the concrete string values) and remove any other occurrences of "both" in nearby auth response docs or examples (search for the "provider" field in the Response block and the Response JSON snippet to locate and fix).
🤖 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/marketing/src/app/docs/api/auth/page.tsx`:
- Around line 18-27: The docs in page.tsx list the endpoint as POST
/api/auth/magic-link but the implemented flow actually calls POST
/api/auth/magic-link/send; update the documentation to use the exact implemented
path (replace "/api/auth/magic-link" with "/api/auth/magic-link/send") or
alternatively change the implementation to accept the shorter route—search for
the endpoint string in page.tsx and align the path and any body/response text to
match the actual handler name used by the auth flow.
---
Outside diff comments:
In `@apps/marketing/src/app/docs/api/auth/page.tsx`:
- Around line 45-53: The Response example still lists the removed enum value
"both" for the provider field; update the Response JSON example in page.tsx so
"provider" only documents "email | google" (or the concrete string values) and
remove any other occurrences of "both" in nearby auth response docs or examples
(search for the "provider" field in the Response block and the Response JSON
snippet to locate and fix).
In `@apps/web/middleware.ts`:
- Around line 31-39: The CLOUD_ONLY_ROUTE_PREFIXES array currently contains
'/api/auth/magic-link/' and '/api/auth/passkey/' which causes the middleware’s
cloud-only prefix check to short-circuit and 404 the new passwordless auth
endpoints; update the middleware by removing those two entries from
CLOUD_ONLY_ROUTE_PREFIXES (or otherwise ensure the tenant/on-prem route check
runs before this cloud-only check) so that the passwordless routes (handled by
the auth handlers) are not blocked; verify the affected symbols
CLOUD_ONLY_ROUTE_PREFIXES and the middleware route-check logic (the function
that iterates/uses CLOUD_ONLY_ROUTE_PREFIXES) are updated accordingly and
consider normalizing prefixes without/with trailing slashes to match actual
route paths.
- Around line 100-112: The pathname bypass condition that checks multiple public
API routes currently omits unauthenticated sign-in initiation endpoints, causing
401s for signed-out users; update the if-condition that uses
pathname.startsWith(...) to also allow '/api/auth/login-csrf' and the magic-link
send endpoint(s) (e.g. add pathname.startsWith('/api/auth/magic') or
pathname.startsWith('/api/auth/magic/send') as appropriate) so these endpoints
skip the session-cookie check and remain accessible without a session.
🪄 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
Run ID: 81b4fb71-f596-4cd2-b7a7-822e66a45e8e
📒 Files selected for processing (11)
apps/marketing/src/app/docs/api/auth/page.tsxapps/marketing/src/app/docs/api/users/page.tsxapps/marketing/src/app/privacy/page.tsxapps/web/middleware.tsapps/web/src/app/auth/signin/page.tsxapps/web/src/lib/auth/auth-fetch.tsscripts/__tests__/setup.tsscripts/__tests__/tenant-import.test.tsscripts/check-fetch-auth.jsscripts/setup-onprem-admin.tsscripts/tenant-export.ts
💤 Files with no reviewable changes (3)
- apps/marketing/src/app/docs/api/users/page.tsx
- scripts/check-fetch-auth.js
- apps/web/src/lib/auth/auth-fetch.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/tests/tenant-import.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/marketing/src/app/privacy/page.tsx
- scripts/tests/setup.ts
- scripts/setup-onprem-admin.ts
- apps/web/src/app/auth/signin/page.tsx
- Split migration 0094 into two files to fix PostgreSQL enum transaction error (new enum values can't be used in the same transaction they're created) - Fix provider downgrade in 4 OAuth routes: preserve existing provider instead of unconditionally overwriting (apple/native, google/callback, google/native, google/one-tap) - Remove magic-link and passkey from CLOUD_ONLY_ROUTE_PREFIXES so on-prem users can actually sign in after password auth removal - Add login-csrf and magic-link to public route bypass in middleware - Add 'webauthn_signup' to VerificationType union - Fix stale --password flag in on-prem deployment docs - Fix magic-link endpoint path in marketing API docs (/send suffix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
Addressing review feedback (a3ea4e8)Critical middleware fixes (from CodeRabbit's outside-diff comments)
All inline review comments addressed
CI blocker fix
|
- Add comment documenting edge case in migration where users with both googleId and appleId are assigned to 'google' - Rename stale test description that referenced credentials Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PostgreSQL error 55P04: "unsafe use of new value 'apple' of enum type AuthProvider — new enum values must be committed before they can be used." Root cause: Drizzle's migrate() runs ALL pending migrations in a single transaction, so splitting into two migration files doesn't help. Fix: Skip the enum recreation entirely. The data migration cleans out all 'both' values, and the TypeScript schema already excludes 'both' so the app code won't produce it. The unused 'both' value stays harmlessly in the DB enum. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/middleware.ts (1)
31-37: Unify OAuth path matchers between cloud-only shield and public bypass for consistency.The
CLOUD_ONLY_ROUTE_PREFIXESincludes'/api/auth/google/'and'/api/auth/apple/'(with trailing slash), while the public bypass checks usepathname.startsWith('/api/auth/google')(no trailing slash) on line 102. Although no exact-path OAuth routes currently exist (only nested routes like/callback,/signin,/one-tap), the matcher inconsistency is a maintenance risk. Reuse the same prefix constants in both places to prevent future misconfigurations and keep the policy logic easier to audit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/middleware.ts` around lines 31 - 37, The CLOUD_ONLY_ROUTE_PREFIXES array is inconsistent with hardcoded checks like pathname.startsWith('/api/auth/google') — update the middleware to reuse the same prefix constants instead of hardcoding strings: replace the individual pathname.startsWith calls with a check that iterates over CLOUD_ONLY_ROUTE_PREFIXES (or extract a shared AUTH prefixes constant used by both the cloud-only shield and the public bypass) so both policy branches use the identical prefix values; reference CLOUD_ONLY_ROUTE_PREFIXES and the existing pathname.startsWith('/api/auth/google') checks when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/middleware.ts`:
- Around line 31-37: The CLOUD_ONLY_ROUTE_PREFIXES array is inconsistent with
hardcoded checks like pathname.startsWith('/api/auth/google') — update the
middleware to reuse the same prefix constants instead of hardcoding strings:
replace the individual pathname.startsWith calls with a check that iterates over
CLOUD_ONLY_ROUTE_PREFIXES (or extract a shared AUTH prefixes constant used by
both the cloud-only shield and the public bypass) so both policy branches use
the identical prefix values; reference CLOUD_ONLY_ROUTE_PREFIXES and the
existing pathname.startsWith('/api/auth/google') checks when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b425f0a-37a2-44fc-a13b-d94e87dfd47c
📒 Files selected for processing (10)
apps/control-plane/src/services/__tests__/provisioning-engine.test.tsapps/marketing/src/app/docs/api/auth/page.tsxapps/web/middleware.tsapps/web/src/app/api/auth/apple/native/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/native/route.tsapps/web/src/app/api/auth/google/one-tap/route.tsdocs/on-prem-deployment.mdpackages/db/drizzle/0094_wakeful_champions.sqlpackages/lib/src/auth/verification-utils.ts
💤 Files with no reviewable changes (1)
- docs/on-prem-deployment.md
✅ Files skipped from review due to trivial changes (2)
- apps/web/src/app/api/auth/google/callback/route.ts
- apps/control-plane/src/services/tests/provisioning-engine.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/app/api/auth/google/one-tap/route.ts
- apps/web/src/app/api/auth/google/native/route.ts
- packages/db/drizzle/0094_wakeful_champions.sql
- apps/marketing/src/app/docs/api/auth/page.tsx
- packages/lib/src/auth/verification-utils.ts
Two fixes for PostgreSQL 55P04 "new enum values must be committed before use": 1. Custom migration runner: each migration now runs in its own transaction instead of Drizzle's default single-transaction approach. This fixes the issue where migration 0049 adds 'apple' to AuthProvider enum and migration 0094 tries to use it. 2. Skip enum recreation in 0094: even with per-migration transactions, CREATE TYPE within a single migration creates "new" values that can't be used in the same transaction. Leave 'both' as harmless unused DB enum value. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…runner The migrate.ts was changed from using Drizzle's built-in migrate() to a custom runner. Updated tests to mock readMigrationFiles, db.execute, and db.transaction instead of the old drizzle-orm/node-postgres/migrator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
What changed
Deleted (15 files)
/api/auth/login,/api/auth/signup,/api/auth/mobile/login,/api/auth/mobile/signup,/api/account/passwordPasswordLoginForm.tsxcomponentseed-loadtest-user.ts,auth-baseline.k6.js)Schema & Core
passwordcolumn from users table'both'fromAuthProviderenum (data migration for existingprovider='both'users)BCRYPT_COSTconstant,'password_reset'verification typebcryptjsfrom all 3 package.json filesOAuth Callbacks (5 routes)
user.password ? 'both' : '<provider>'logic from Google callback, one-tap, native + Apple callback, nativeFrontend
PasswordLoginForm→MagicLinkFormCreateUserForm: removed password fieldsuseAuthhook: removedlogin()function (called deleted endpoint)Scripts & Control Plane
setup-onprem-admin.ts: rewritten without bcrypt/passwords — creates admin with email/name/role onlyTenantProvisioningCompleteEmail: removed temporary password sectionDocumentation
password_changein activity enums as@deprecated(historical audit data)Migration
0094_wakeful_champions.sql: migratesprovider='both'→ OAuth provider, recreates enum without'both', drops password columnVerification
All grep patterns return zero hits:
TypeScript compiles. Build passes.
Breaking changes
/api/auth/mobile/loginand/api/auth/mobile/signupdeleted — mobile needs passkey/magic link updatepasswordcolumn and removes'both'enum value — run migration before deployingTest plan
'google'or'apple'(never'both')pnpm typecheck— passespnpm test— verify no password-related test failures🤖 Generated with Claude Code
Summary by CodeRabbit
Changed Features
Documentation
Other