Add verified account email change flow - #645
Conversation
📝 WalkthroughWalkthroughAdds stored stable user ID support across auth and account flows, then implements an email-change request and verification flow with route wiring, tests, and client updates. ChangesStable user ID and email-change feature
Estimated code review effort: 4 (Complex) | ~80 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
🔎 Preview deployed: https://kody-pr-645.kentcdodds.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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/app/admin-usage-data.ts (1)
68-73: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winN+1 queries introduced by per-row
resolveUserStableIdByEmail.The bulk user query (lines 114-121) doesn't select
stable_user_id, soaddUsageIdsnow issues one extra DB round-trip per row viaresolveUserStableIdByEmail. Selectingstable_user_idalongside the other columns and using the synchronousresolveUserStableIdavoids the extra queries entirely.⚡ Suggested fix
type AdminUsageUserRow = { id: number username: string email: string plan: string | null + stable_user_id: string | null }env.APP_DB.prepare( - `SELECT id, username, email, plan + `SELECT id, username, email, plan, stable_user_id FROM users ORDER BY id ASC LIMIT ? OFFSET ?`, )- const users = await addUsageIds(env.APP_DB, userRows.results ?? []) + const users = await addUsageIds(userRows.results ?? [])async function addUsageIds( - db: D1Database, rows: Array<AdminUsageUserRow>, ): Promise<Array<UserWithUsageId>> { return await Promise.all( rows.map(async (row) => ({ id: row.id, username: row.username, email: row.email, plan: parsePlanName(row.plan), - usageUserId: await resolveUserStableIdByEmail({ db, email: row.email }), + usageUserId: await resolveUserStableId(row), })), ) }(
loadSelectedUser's fallback query at line 196 would also needstable_user_idadded, and itsaddUsageIds(input.db, [row])call updated accordingly.)Also applies to: 114-124, 168-181, 200-200
🤖 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/admin-usage-data.ts` around lines 68 - 73, The admin usage data flow is doing per-row lookups because `loadSelectedUsers` and the `loadSelectedUser` fallback path do not select `stable_user_id`, forcing `addUsageIds` to call `resolveUserStableIdByEmail` for each row. Update the `AdminUsageUserRow` shape and the queries in `loadSelectedUsers`/`loadSelectedUser` to include `stable_user_id`, then switch `addUsageIds` to use `resolveUserStableId` on the loaded rows so the IDs are resolved without extra DB round-trips.
🧹 Nitpick comments (4)
packages/worker/client/routes/account.tsx (1)
531-544: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winError message uses
role="status"instead ofrole="alert".
emailChangeMessage(including the "Password is incorrect." failure case) is always rendered withrole="status", whereas the top-levelmessageblock above usesrole="alert"unconditionally. Screen readers announcealertregions immediately/assertively but only pollstatusregions politely, so a password failure here may not be announced promptly.♿ Suggested fix
{emailChangeMessage ? ( <p - role="status" + role={emailChangeTone === 'error' ? 'alert' : 'status'} mix={css({ color: emailChangeTone === 'error' ? colors.error : colors.text, margin: 0, })} > {emailChangeMessage} </p> ) : null}🤖 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/client/routes/account.tsx` around lines 531 - 544, The email change feedback block in account.tsx is using role="status" for an error state, which makes failures like “Password is incorrect.” announced too politely. Update the emailChangeMessage rendering so the message uses role="alert" when emailChangeTone indicates an error, while preserving the existing non-error behavior for success/info states. Use the emailChangeMessage and emailChangeTone conditional block to locate the change.packages/worker/src/app/admin-user-creation.ts (1)
152-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold
stable_user_idinto the initial INSERT instead of a silently-swallowed follow-up UPDATE.
auth.tssetsstable_user_iddirectly in theINSERT/db.createcall for new users; here it's set via a secondUPDATEwhose failure is discarded with no logging, inconsistent with how other best-effort steps in this file report failures (e.g., role assignment, email inbox provisioning both log on error).♻️ Suggested fix
const stableUserId = await createStableUserIdFromEmail(email) let userId: number | null = null try { const result = await input.db .prepare( - `INSERT INTO users (username, email, password_hash, email_verified_at) - VALUES (?, ?, ?, ?)`, + `INSERT INTO users (username, email, password_hash, email_verified_at, stable_user_id) + VALUES (?, ?, ?, ?, ?)`, ) - .bind(username, email, adminCreatedNoUsablePasswordHash, nowIso) + .bind(username, email, adminCreatedNoUsablePasswordHash, nowIso, stableUserId) .run() const lastRowId = result.meta.last_row_id if (!Number.isSafeInteger(lastRowId) || lastRowId < 1) { throw new AdminCreateUserError( 'create_failed', 'Unable to create account.', ) } userId = lastRowId - await input.db - .prepare(`UPDATE users SET stable_user_id = ? WHERE id = ?`) - .bind(stableUserId, userId) - .run() - .catch(() => undefined) } catch (error) {🤖 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/admin-user-creation.ts` around lines 152 - 175, `createAdminUser` is inserting a user and then quietly patching `stable_user_id` in a separate UPDATE, which can fail without any visibility. Move the `stable_user_id` assignment into the initial INSERT in this flow, using the same `stableUserId` generated by `createStableUserIdFromEmail`, and remove the swallowed follow-up UPDATE so the user record is created atomically and consistently with `auth.ts` and `db.create`.packages/worker/src/user-id.ts (1)
43-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNarrow the fallback scan to legacy rows The post-miss scan currently walks every
usersrow, but the indexed lookup already covers rows with a storedstable_user_id; filtering the fallback tostable_user_id IS NULLavoids rehashing rows that can’t match.🤖 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/user-id.ts` around lines 43 - 76, The fallback scan in findUserRowByStableUserId is too broad because it rechecks every row after the indexed lookup has already handled stored stable_user_id values. Update the fallback query in this function to restrict the scan to legacy rows only, using stable_user_id IS NULL (with the existing withoutStableUserIdColumn path as needed), so resolveUserStableId is only applied to rows that could still match.packages/worker/src/db.ts (1)
10-10: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing
users.stable_user_id.This PR routes several resolvers (
findUserRowByStableUserId, entitlements lookups) throughWHERE stable_user_id = ?. Without an index, those queries fall back to table scans, and the legacy scan-and-hash path already scans all rows. An index onstable_user_idkeeps these lookups efficient as the table grows.🤖 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/db.ts` at line 10, Add an index for users.stable_user_id in the schema definition where the users table is declared in db.ts. The new index should target the stable_user_id column so the resolver paths using findUserRowByStableUserId and entitlement lookups can use indexed WHERE stable_user_id = ? queries instead of table scans. Keep the change localized to the users table schema alongside the existing column definitions.
🤖 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/client/routes/account.tsx`:
- Around line 214-221: The 401 handling in account.tsx is relying on the literal
error text from account-email-change.tsx, which is fragile. Update the response
handling to branch on a stable machine-readable discriminator instead of
comparing payload.error to “Password is incorrect.”, and use the existing inline
error path only for the invalid-password case. Add a backend code field in the
account-email-change handler so the client can distinguish invalid password from
session-expired/unauthorized responses without depending on human-readable
wording.
In `@packages/worker/client/routes/verify-email.tsx`:
- Around line 24-31: The title selection in verify-email.tsx is inferring
email-change state by substring-matching data.message, which tightly couples
EmailVerificationLoaderData to backend copy. Update the loader/data contract to
include an explicit discriminator such as kind on EmailVerificationLoaderData,
and use that in the verify-email route instead of checking message text when
computing isEmailChange and title.
In `@packages/worker/src/app/email-change.ts`:
- Around line 86-153: `sendEmailChange` currently blocks a new request when
`pending_email_changes.new_email` already has an active row, so a user cannot
get a fresh token for the same address. Update the flow in
`packages/worker/src/app/email-change.ts` to either replace any existing pending
row before `db.create(pendingEmailChangesTable, ...)` or add a resend path that
reuses the existing record, and make sure the cleanup logic in `discardNewToken`
and the final token cleanup still leave only the newest pending token per
user/email.
In `@packages/worker/src/app/handlers/account-email-change.node.test.ts`:
- Around line 293-304: The expiry setup in the verifyEmailChangeToken test is
using mismatched clocks, which can make the token look expired depending on when
the suite runs. Update the test in account-email-change.node.test.ts so the
seeded expires_at values and the now argument are derived from the same fixed
reference time, or make both relative to Date.now(). Use the existing
verifyEmailChangeToken test case and the email_verifications /
pending_email_changes inserts to keep the expiration assertion deterministic.
In `@packages/worker/src/app/handlers/account-email-change.ts`:
- Around line 86-145: The `/account-email-change` handler currently calls
`verifyPassword` before any user-specific throttling, which allows unlimited
password retries for a valid session. In `account-email-change.ts`, move the
existing `checkRateLimit` flow for the user (or add a dedicated failed-attempt
limiter) so it runs before `verifyPassword`, and ensure the same audit/logging
paths in this handler still record rate-limited failures and other rejection
reasons consistently.
In `@packages/worker/src/user-id.ts`:
- Around line 24-37: The resolveUserStableIdByEmail flow is swallowing database
errors by using .catch(() => null), which makes real query failures look like
missing rows and incorrectly falls back to createStableUserIdFromEmail. Update
resolveUserStableIdByEmail to let D1Database errors surface, and only use the
fallback path when the query explicitly returns no row; keep the existing
resolveUserStableId and createStableUserIdFromEmail branching, but distinguish
actual not-found from rejected queries.
---
Outside diff comments:
In `@packages/worker/src/app/admin-usage-data.ts`:
- Around line 68-73: The admin usage data flow is doing per-row lookups because
`loadSelectedUsers` and the `loadSelectedUser` fallback path do not select
`stable_user_id`, forcing `addUsageIds` to call `resolveUserStableIdByEmail` for
each row. Update the `AdminUsageUserRow` shape and the queries in
`loadSelectedUsers`/`loadSelectedUser` to include `stable_user_id`, then switch
`addUsageIds` to use `resolveUserStableId` on the loaded rows so the IDs are
resolved without extra DB round-trips.
---
Nitpick comments:
In `@packages/worker/client/routes/account.tsx`:
- Around line 531-544: The email change feedback block in account.tsx is using
role="status" for an error state, which makes failures like “Password is
incorrect.” announced too politely. Update the emailChangeMessage rendering so
the message uses role="alert" when emailChangeTone indicates an error, while
preserving the existing non-error behavior for success/info states. Use the
emailChangeMessage and emailChangeTone conditional block to locate the change.
In `@packages/worker/src/app/admin-user-creation.ts`:
- Around line 152-175: `createAdminUser` is inserting a user and then quietly
patching `stable_user_id` in a separate UPDATE, which can fail without any
visibility. Move the `stable_user_id` assignment into the initial INSERT in this
flow, using the same `stableUserId` generated by `createStableUserIdFromEmail`,
and remove the swallowed follow-up UPDATE so the user record is created
atomically and consistently with `auth.ts` and `db.create`.
In `@packages/worker/src/db.ts`:
- Line 10: Add an index for users.stable_user_id in the schema definition where
the users table is declared in db.ts. The new index should target the
stable_user_id column so the resolver paths using findUserRowByStableUserId and
entitlement lookups can use indexed WHERE stable_user_id = ? queries instead of
table scans. Keep the change localized to the users table schema alongside the
existing column definitions.
In `@packages/worker/src/user-id.ts`:
- Around line 43-76: The fallback scan in findUserRowByStableUserId is too broad
because it rechecks every row after the indexed lookup has already handled
stored stable_user_id values. Update the fallback query in this function to
restrict the scan to legacy rows only, using stable_user_id IS NULL (with the
existing withoutStableUserIdColumn path as needed), so resolveUserStableId is
only applied to rows that could still match.
🪄 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: f8b3994c-5368-430c-9771-87b1a804367a
📒 Files selected for processing (23)
packages/worker/client/routes/account.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/verify-email.tsxpackages/worker/migrations/0052-stable-user-id-email-changes.sqlpackages/worker/src/app/account-data-targets.tspackages/worker/src/app/account-export.tspackages/worker/src/app/admin-usage-data.tspackages/worker/src/app/admin-user-creation.tspackages/worker/src/app/email-change.tspackages/worker/src/app/email-verification.tspackages/worker/src/app/handlers/account-email-change.node.test.tspackages/worker/src/app/handlers/account-email-change.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/app/handlers/verify-email-change.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/app/user-lookup.tspackages/worker/src/db.tspackages/worker/src/email/platform-address.tspackages/worker/src/entitlements/service.tspackages/worker/src/oauth-handlers.tspackages/worker/src/user-id.ts
| .prepare(`UPDATE users SET stable_user_id = ? WHERE id = ?`) | ||
| .bind(stableUserId, userId) | ||
| .run() | ||
| .catch(() => undefined) |
There was a problem hiding this comment.
Admin create duplicates stable identity
High Severity
After another account changes away from an address but keeps its stable_user_id, admin creation can insert a new row for that freed email, silently ignore a conflicting stable_user_id update, and leave two database users resolving to the same MCP stable id via resolveUserStableId.
Reviewed by Cursor Bugbot for commit de3288f. Configure here.
| { | ||
| username: normalizedUsername, | ||
| email: normalizedEmail, | ||
| stable_user_id: stableUserId, |
There was a problem hiding this comment.
Signup ignores stable id conflict
Medium Severity
Signup now writes stable_user_id from the email hash, but unique failures on that column are not mapped to a client-facing conflict response, so registering a previously released address can surface as an unhandled error instead of a clear “email taken” outcome.
Reviewed by Cursor Bugbot for commit de3288f. Configure here.
| } | ||
| } | ||
| return false | ||
| const row = await findUserRowByStableUserId<{ |
There was a problem hiding this comment.
MCP blocked after email change
High Severity
After a verified email change, existing OAuth MCP tokens still carry the previous address in grant props. isAccountEmailVerified only looks up that stale email and never uses stableUserId, so it returns unverified and MCP requests get a 403 even though the account is verified under the new address.
Reviewed by Cursor Bugbot for commit 111dd40. Configure here.
| .prepare(`SELECT plan FROM users WHERE email = ?`) | ||
| .bind(email) | ||
| .first<{ plan: string | null }>() | ||
| if (!row) return null |
There was a problem hiding this comment.
Plan lookup breaks post change
Medium Severity
getUserPlan still treats a matching SHA-256 of the supplied email as sufficient proof of identity. After an email change, the stored stable_user_id no longer matches the new address hash, and lookups by the old address find no row, so plan enforcement is skipped or applied under the wrong user id namespace.
Reviewed by Cursor Bugbot for commit 111dd40. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0036e4f. Configure here.
| ) | ||
| } | ||
| const row = await input.env.APP_DB.prepare( | ||
| `SELECT id FROM users WHERE email = ?`, |
There was a problem hiding this comment.
Export lookup breaks after change
Medium Severity
Account export resolves the database user only by the caller email. After an email change, MCP export can still pass the previous address, the row is not found, and export fails even when mcpUserId still matches the preserved stable id.
Reviewed by Cursor Bugbot for commit 0036e4f. Configure here.
2 participants
Summary
/account, requiring the current password and a new-address verification link before updatingusers.email.users.stable_user_idand preserves existing user-owned namespaces when account emails change.Walkthrough
Testing
npx vitest run --project node-unit packages/worker/src/app/handlers/account-email-change.node.test.ts packages/worker/src/entitlements/entitlements.node.test.ts packages/worker/src/app/admin-user-creation.node.test.tsnpx vitest run --project workers-unit packages/worker/src/oauth-handlers.workers.test.tsnpx vitest run --project node-unit packages/worker/src/app/admin-usage-data.node.test.ts packages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tsnpm run typechecknpm run test:e2e:rungit push -u origin cursor/verified-email-change-4435(pre-push rannpm run testandnpm run test:e2e:run; final run passed 581 unit/workers tests and 6 E2E tests)npx wrangler d1 execute APP_DB --command "SELECT user_id, new_email, LENGTH(token_hash) AS token_hash_length FROM pending_email_changes WHERE new_email = 'email-change-demo-20260706-1624@example.com';" --local --env production --config packages/worker/wrangler.jsoncPassword is incorrect., then correct password showsVerification email sent to your new address.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@83f96229· Head:0036e4fdClassification: extends — changes account auth/session behavior, D1 account schema, OAuth identity lookup, and user identity resolution while preserving existing primitives.
Primitives touched
app-uiapp-sessionsmcp-oauthentitlementsaccount-exportemaild1-app-dbusers.stable_user_idandpending_email_changesSystem map
Change flow
Before / after
Invariants
per-user-isolation: account email changes preserve the stable user namespace so D1 rows, Durable Object names, KV-derived keys, and capability contexts continue to use the same user-owned identity.Summary by CodeRabbit