Repository navigation
Add invite-gated signup and email verification - #615
Conversation
📝 WalkthroughWalkthroughThis PR adds invite-gated signup and email verification, plus the supporting admin invite management, session propagation, outbound email gating, tests, and docs. It also wires the new routes into the worker client and server routers. ChangesInvite Signup and Email Verification
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 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-615.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
packages/worker/src/email/outbound.workers.test.ts (1)
17-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test-schema helpers across files.
ensureAccountVerificationTestSchema/seedVerifiedAccounthere largely duplicate the same table-creation/seeding logic added inpackages/worker/src/email/inbound.workers.test.ts(lines 23-57), but with a different signature (seedVerifiedAccount(email)vs.seedVerifiedAccount({db, email, username})). Since both already diverge, future schema changes (e.g., additionaluserscolumns) risk being applied in only one place. Consider extracting a shared test-schema helper (similar toensureEmailTestSchema) that both files import.🤖 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/email/outbound.workers.test.ts` around lines 17 - 41, The test helpers for verified accounts are duplicated between the outbound and inbound worker tests, so schema changes could drift. Extract the shared `ensureAccountVerificationTestSchema` and `seedVerifiedAccount` logic into a common test utility (similar to `ensureEmailTestSchema`) and update both `packages/worker/src/email/outbound.workers.test.ts` and the corresponding inbound test to import and use the shared helper, keeping the existing call sites and signatures consistent where needed.packages/worker/client/routes/login.tsx (1)
228-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlaceholder copy uses internal jargon.
"Required for production launch cohorts" uses internal terminology ("cohorts") that may confuse end users signing up. Consider simpler copy, e.g. "Required if invite-only signup is enabled."
🤖 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/login.tsx` around lines 228 - 239, The signup invite code field in login.tsx uses internal jargon in the input placeholder, so update the copy in the isSignup block to user-friendly wording. Keep the field name and behavior unchanged, but replace the placeholder text in the inviteCode input with simpler language like “Required if invite-only signup is enabled.”packages/worker/client/routes/account.tsx (1)
246-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo resend-verification affordance.
The unverified banner only tells users to check their inbox, with no way to request a new verification email if the original is lost or expired. Worth confirming this is intentionally deferred (e.g., handled elsewhere) rather than an oversight.
🤖 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 246 - 261, The unverified email banner in the account route only displays status and has no resend-verification action, so add a clear affordance to request a new verification email or confirm that resend is handled elsewhere. Update the conditional block in the account page component around the email verification section to include a resend control tied to the existing verification flow, using the relevant account/email verification handlers in this route. If the resend action is intentionally omitted here, add a brief note in the component to make that decision explicit.packages/worker/src/app/invites.node.test.ts (1)
6-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock DB doesn't actually interleave, so this doesn't test a real race.
run()has no internalawait, soPromise.allexecutes bothconsumeInviteCodecalls' DB writes fully synchronously and sequentially (JS never yields mid-update). The test currently passes because of execution order, not because atomicity was verified under contention — it would pass even if the implementation weren't atomic. Consider inserting an artificialawait Promise.resolve()(or a microtask delay) inside the mock'srun()before mutating state, so both calls genuinely race for the record.Also applies to: 100-113
🤖 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/invites.node.test.ts` around lines 6 - 46, The invite DB mock in createInviteDb is not yielding during updates, so the Promise.all test in invs.node.test.ts does not simulate contention. Update the mocked prepare(...).bind(...).run() path to include an async yield before mutating records so concurrent consumeInviteCode calls can interleave, and keep the atomicity assertion meaningful under real race conditions. Use createInviteDb, prepare, bind, and run as the key locations to adjust the mock behavior.
🤖 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/admin-invites.tsx`:
- Around line 138-152: The invite mutation flow in admin-invites.tsx only parses
the JSON response and falls back to a generic error, so expired or forbidden
sessions are not handled like the GET paths. Update the mutation handling around
the fetch/readJson logic to inspect response.status for 401 and 403 before
relying on payload.ok, and route those cases the same way the existing
loader/query code does. Use the existing invite mutation handlers in
adminInvitesApiPath as the place to add this status-based redirect or permission
error behavior, then keep the generic fallback only for other failures.
- Around line 194-205: Clear the failed-load latch when revisiting the route so
a transient failure does not permanently block reloads. In the admin invites
route logic, update the state handling around `needsLoad`/`lastFailedHref` in
`admin-invites.tsx` so that when the visible `currentHref` changes back to this
route, the failed-href guard is reset like the admin users route does. Keep the
existing load gating in place, but ensure `needsLoad` can become true again
after a route revisit instead of staying false forever for `/admin/invites`.
In `@packages/worker/migrations/0046-invites-email-verification.sql`:
- Around line 32-33: The backfill in the migration currently updates every row
in users, which marks all legacy accounts as verified. Update the migration to
scope the email_verified_at backfill to only the intended launch cohort, or if
the global grandfathering is intentional, add an explicit note/documentation in
the migration explaining that behavior. Use the users backfill statement as the
place to adjust the filter.
In `@packages/worker/src/app/admin-invites-data.ts`:
- Around line 15-31: `loadAdminInvitesData` currently reads all invites and
creator emails without any user scope, so update this read path to be explicitly
scoped by actor identity or clearly marked as an operator-only exception. Pass
`userId`/actor context into `loadAdminInvitesData` and add the appropriate
`WHERE i.created_by = ?` filter if invites are user-owned; otherwise remove or
avoid the `users u` join for email data and document the global admin-only
behavior. Keep the fix centered on `loadAdminInvitesData` and the invite query
so all signed-in data access remains isolated.
In `@packages/worker/src/app/email-verification.ts`:
- Around line 155-181: The `isAccountEmailVerified` fallback for `stableUserId`
currently scans the entire `users` table and recomputes
`createStableUserIdFromEmail` for every row, which is both unscoped and
inefficient. Update this flow so the `users` record stores a persisted, indexed
stable user id at creation time, then change `isAccountEmailVerified` to query
that column directly with a scoped lookup instead of calling
`prepare(...).all()` and looping through every row.
In `@packages/worker/src/app/handlers/admin-invites.ts`:
- Around line 91-94: The invite creation flow in admin-invites currently
defaults invalid maxUses to 1, which can silently create unintended one-use
invites. Update the handling around readPositiveInt in the invite handlers
(including the create/update paths referenced by admin-invites) to validate that
maxUses is a safe positive integer and return a 400 response when it is missing,
malformed, or non-positive instead of applying a fallback. Make sure the change
is applied consistently anywhere maxUses is parsed in the invite request
handlers.
In `@packages/worker/src/app/handlers/auth.ts`:
- Around line 182-220: The signup flow in auth.ts currently computes
createPasswordHash before validating the invite, which wastes expensive CPU on
rejected requests. Move the invite gating logic in the signup handler so the
inviteRequired/normalizeInviteCode check and consumeInviteCode run before
createPasswordHash, and only call createPasswordHash after a valid invite has
been accepted and consumed. Keep releaseConsumedInvite handling tied to the
successful invite path in the same handler.
In `@packages/worker/src/app/handlers/verify-email.ts`:
- Around line 28-55: The verify-email flow in verifyEmailToken currently
consumes the token on the first GET, which makes GET /verify-email
non-idempotent and vulnerable to mailbox scanners. Update the verify-email
handler and the verifyEmailToken path so a GET only validates/marks completion
without invalidating the token, or move the actual consumption to a
user-initiated confirmation POST; keep the token usable until expiry and
preserve the existing logAuditEvent and renderAppPage behavior in
verify-email.ts.
- Around line 34-42: The email_verify audit event writes are currently
fire-and-forget, so they can be lost when the handler returns. Update the
verify-email flow to thread the ExecutionContext through the app handler path
and use ctx.waitUntil() for both the success and failure logAuditEvent calls.
Make sure the changes are applied in the verify-email handler and the
renderAppPage app-path wiring so the audit writes stay alive after the response
is sent.
---
Nitpick comments:
In `@packages/worker/client/routes/account.tsx`:
- Around line 246-261: The unverified email banner in the account route only
displays status and has no resend-verification action, so add a clear affordance
to request a new verification email or confirm that resend is handled elsewhere.
Update the conditional block in the account page component around the email
verification section to include a resend control tied to the existing
verification flow, using the relevant account/email verification handlers in
this route. If the resend action is intentionally omitted here, add a brief note
in the component to make that decision explicit.
In `@packages/worker/client/routes/login.tsx`:
- Around line 228-239: The signup invite code field in login.tsx uses internal
jargon in the input placeholder, so update the copy in the isSignup block to
user-friendly wording. Keep the field name and behavior unchanged, but replace
the placeholder text in the inviteCode input with simpler language like
“Required if invite-only signup is enabled.”
In `@packages/worker/src/app/invites.node.test.ts`:
- Around line 6-46: The invite DB mock in createInviteDb is not yielding during
updates, so the Promise.all test in invs.node.test.ts does not simulate
contention. Update the mocked prepare(...).bind(...).run() path to include an
async yield before mutating records so concurrent consumeInviteCode calls can
interleave, and keep the atomicity assertion meaningful under real race
conditions. Use createInviteDb, prepare, bind, and run as the key locations to
adjust the mock behavior.
In `@packages/worker/src/email/outbound.workers.test.ts`:
- Around line 17-41: The test helpers for verified accounts are duplicated
between the outbound and inbound worker tests, so schema changes could drift.
Extract the shared `ensureAccountVerificationTestSchema` and
`seedVerifiedAccount` logic into a common test utility (similar to
`ensureEmailTestSchema`) and update both
`packages/worker/src/email/outbound.workers.test.ts` and the corresponding
inbound test to import and use the shared helper, keeping the existing call
sites and signatures consistent where needed.
🪄 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: 1207d4d3-9f0b-46bd-87d6-1b0fa9ee599b
📒 Files selected for processing (43)
docs/contributing/architecture/authentication.mddocs/contributing/architecture/authorization.mddocs/contributing/architecture/primitives.yamldocs/contributing/project-intent.mddocs/contributing/setup.mde2e/auth-test-user.tse2e/d1-utils.tse2e/invite-signup-verification.spec.tspackages/worker/client/navigation-data.node.test.tspackages/worker/client/routes/account.tsxpackages/worker/client/routes/admin-invites.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/login.tsxpackages/worker/client/routes/verify-email.tsxpackages/worker/client/session.tspackages/worker/migrations/0046-invites-email-verification.sqlpackages/worker/src/app/account-profile-data.tspackages/worker/src/app/admin-invites-data.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/email-verification.tspackages/worker/src/app/handlers/account-profile.node.test.tspackages/worker/src/app/handlers/admin-invites.tspackages/worker/src/app/handlers/auth-handler.node.test.tspackages/worker/src/app/handlers/auth.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/app/handlers/verify-email.tspackages/worker/src/app/invites.node.test.tspackages/worker/src/app/invites.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/app/session-info.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/db.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/mcp/capabilities/email/email-reply.node.test.tspackages/worker/src/mcp/capabilities/email/email-reply.tspackages/worker/src/mcp/capabilities/email/email-send.tstools/seed-test-data.ts
| const response = await fetch(adminInvitesApiPath, { | ||
| method: 'POST', | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| credentials: 'include', | ||
| body: JSON.stringify(body), | ||
| }) | ||
| const payload = await readJson< | ||
| AdminInvitesLoaderData & { ok?: boolean; error?: string } | ||
| >(response) | ||
| if (!response.ok || !payload?.ok) { | ||
| throw new Error(payload?.error ?? 'Unable to update invites.') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle expired/forbidden sessions on invite mutations.
The GET paths redirect/report 401/403, but create/revoke tries to parse the response and surfaces a generic “Unable to update invites.” Add the same status handling here so admins are sent back to login or shown the permission error.
Proposed fix
const response = await fetch(adminInvitesApiPath, {
method: 'POST',
headers: {
Accept: 'application/json',
'Content-Type': 'application/json',
},
credentials: 'include',
body: JSON.stringify(body),
})
+ if (response.status === 401) {
+ window.location.assign('/login')
+ return
+ }
+ if (response.status === 403) {
+ throw new Error('You do not have permission to manage invites.')
+ }
const payload = await readJson<📝 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.
| const response = await fetch(adminInvitesApiPath, { | |
| method: 'POST', | |
| headers: { | |
| Accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| }, | |
| credentials: 'include', | |
| body: JSON.stringify(body), | |
| }) | |
| const payload = await readJson< | |
| AdminInvitesLoaderData & { ok?: boolean; error?: string } | |
| >(response) | |
| if (!response.ok || !payload?.ok) { | |
| throw new Error(payload?.error ?? 'Unable to update invites.') | |
| } | |
| const response = await fetch(adminInvitesApiPath, { | |
| method: 'POST', | |
| headers: { | |
| Accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| }, | |
| credentials: 'include', | |
| body: JSON.stringify(body), | |
| }) | |
| if (response.status === 401) { | |
| window.location.assign('/login') | |
| return | |
| } | |
| if (response.status === 403) { | |
| throw new Error('You do not have permission to manage invites.') | |
| } | |
| const payload = await readJson< | |
| AdminInvitesLoaderData & { ok?: boolean; error?: string } | |
| >(response) | |
| if (!response.ok || !payload?.ok) { | |
| throw new Error(payload?.error ?? 'Unable to update invites.') | |
| } |
🤖 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/admin-invites.tsx` around lines 138 - 152, The
invite mutation flow in admin-invites.tsx only parses the JSON response and
falls back to a generic error, so expired or forbidden sessions are not handled
like the GET paths. Update the mutation handling around the fetch/readJson logic
to inspect response.status for 401 and 403 before relying on payload.ok, and
route those cases the same way the existing loader/query code does. Use the
existing invite mutation handlers in adminInvitesApiPath as the place to add
this status-based redirect or permission error behavior, then keep the generic
fallback only for other failures.
| UPDATE users | ||
| SET email_verified_at = COALESCE(email_verified_at, CURRENT_TIMESTAMP); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files 'packages/worker/migrations/*' | sed -n '1,120p'
echo
echo "== target migration =="
cat -n packages/worker/migrations/0046-invites-email-verification.sql
echo
echo "== nearby migrations mentioning email verification =="
rg -n "email_verified_at|invites-email-verification|verified" packages/worker/migrations packages -g '!**/node_modules/**' | sed -n '1,200p'Repository: kentcdodds/kody
Length of output: 12974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== email verification app logic =="
cat -n packages/worker/src/app/email-verification.ts | sed -n '1,260p'
echo
echo "== outbound email gate =="
cat -n packages/worker/src/email/outbound.ts | sed -n '140,240p'
echo
echo "== account UI text around email verification =="
cat -n packages/worker/client/routes/account.tsx | sed -n '240,320p'
echo
echo "== search for backfill / rollout notes =="
rg -n "backfill|email_verified_at|email verification|verified before sending|invite" packages/worker -g '!**/node_modules/**' | sed -n '1,240p'Repository: kentcdodds/kody
Length of output: 31564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== auth signup flow around email verification =="
cat -n packages/worker/src/app/handlers/auth.ts | sed -n '240,380p'
echo
echo "== invite/admin text that may explain the rollout =="
cat -n packages/worker/client/routes/admin-invites.tsx | sed -n '200,260p'
echo
echo "== search for grandfathering / launch cohort language =="
rg -n "grandfather|launch cohort|launch-cohort|trusted|existing accounts|existing users|production signup|verify your email|email stays on the account" packages/worker -g '!**/node_modules/**' | sed -n '1,220p'Repository: kentcdodds/kody
Length of output: 13071
Scope the email_verified_at backfill
This marks every existing account as verified, so the outbound-email gate will trust legacy users too. If that grandfathering is intentional, document it; otherwise limit the migration to the intended launch cohort.
🤖 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/migrations/0046-invites-email-verification.sql` around lines
32 - 33, The backfill in the migration currently updates every row in users,
which marks all legacy accounts as verified. Update the migration to scope the
email_verified_at backfill to only the intended launch cohort, or if the global
grandfathering is intentional, add an explicit note/documentation in the
migration explaining that behavior. Use the users backfill statement as the
place to adjust the filter.
| export async function loadAdminInvitesData( | ||
| env: Env, | ||
| ): Promise<AdminInvitesLoaderData> { | ||
| const result = await env.APP_DB.prepare( | ||
| `SELECT i.code, | ||
| i.created_by, | ||
| u.email AS created_by_email, | ||
| i.note, | ||
| i.max_uses, | ||
| i.use_count, | ||
| i.expires_at, | ||
| i.revoked_at, | ||
| i.created_at | ||
| FROM invites i | ||
| LEFT JOIN users u ON u.id = i.created_by | ||
| ORDER BY i.created_at DESC, i.code ASC | ||
| LIMIT 200`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the invite read path by userId or make the operator-shared exception explicit.
loadAdminInvitesData reads every invite and creator email without an actor/userId parameter. If invite metadata is user-owned, pass the actor user id through and add a WHERE i.created_by = ? scope; if it is intentionally global operator state, avoid joining user-owned email data here or document the explicit exception. As per coding guidelines, “All read/write paths must be scoped by userId so every signed-in user’s data remains isolated.”
🤖 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-invites-data.ts` around lines 15 - 31,
`loadAdminInvitesData` currently reads all invites and creator emails without
any user scope, so update this read path to be explicitly scoped by actor
identity or clearly marked as an operator-only exception. Pass `userId`/actor
context into `loadAdminInvitesData` and add the appropriate `WHERE i.created_by
= ?` filter if invites are user-owned; otherwise remove or avoid the `users u`
join for email data and document the global admin-only behavior. Keep the fix
centered on `loadAdminInvitesData` and the invite query so all signed-in data
access remains isolated.
Source: Coding guidelines
| export async function isAccountEmailVerified(input: { | ||
| db: D1Database | ||
| email?: string | null | ||
| stableUserId?: string | null | ||
| }) { | ||
| const normalizedEmail = | ||
| typeof input.email === 'string' ? normalizeEmail(input.email) : '' | ||
| if (normalizedEmail) { | ||
| const row = await input.db | ||
| .prepare(`SELECT email_verified_at FROM users WHERE email = ?`) | ||
| .bind(normalizedEmail) | ||
| .first<{ email_verified_at: string | null }>() | ||
| return Boolean(row?.email_verified_at) | ||
| } | ||
|
|
||
| const stableUserId = input.stableUserId?.trim() | ||
| if (!stableUserId) return false | ||
| const rows = await input.db | ||
| .prepare(`SELECT email, email_verified_at FROM users`) | ||
| .all<{ email: string; email_verified_at: string | null }>() | ||
| for (const row of rows.results ?? []) { | ||
| if ((await createStableUserIdFromEmail(row.email)) === stableUserId) { | ||
| return Boolean(row.email_verified_at) | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Full users-table scan for stableUserId verification lookups.
When falling back to stableUserId, this loads every row from users and hashes each email until a match is found — an unscoped, O(n) read that recomputes createStableUserIdFromEmail per row. If this check runs on the outbound-email hot path per MCP send, cost grows linearly with user count.
Consider persisting the stable user id as an indexed column at account creation so this lookup becomes a direct, scoped query instead of a full-table scan.
As per coding guidelines, "All read/write paths must be scoped by userId so every signed-in user's data remains isolated," which this unscoped table scan does not satisfy.
🤖 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/email-verification.ts` around lines 155 - 181, The
`isAccountEmailVerified` fallback for `stableUserId` currently scans the entire
`users` table and recomputes `createStableUserIdFromEmail` for every row, which
is both unscoped and inefficient. Update this flow so the `users` record stores
a persisted, indexed stable user id at creation time, then change
`isAccountEmailVerified` to query that column directly with a scoped lookup
instead of calling `prepare(...).all()` and looping through every row.
Source: Coding guidelines
| const code = readString(input.body, 'code') | ||
| const note = readString(input.body, 'note') ?? '' | ||
| const maxUses = readPositiveInt(readString(input.body, 'maxUses'), 1) | ||
| const expiresAt = readExpiresAt(input.body) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject invalid maxUses instead of silently creating a one-use invite.
Invalid values currently fall back to 1, so a bad admin/API request can create a valid invite with unintended capacity. Return 400 unless the value is a safe positive integer.
Proposed fix
- const maxUses = readPositiveInt(readString(input.body, 'maxUses'), 1)
+ const maxUses = readPositiveInt(readString(input.body, 'maxUses'))
+ if (maxUses == null) {
+ return jsonResponse(
+ { ok: false, error: 'Max uses must be a positive integer.' },
+ 400,
+ )
+ }
...
-function readPositiveInt(value: string | null, fallback: number) {
- if (!value) return fallback
- const parsed = Number.parseInt(value, 10)
- if (!Number.isFinite(parsed) || parsed < 1) {
- return fallback
- }
+function readPositiveInt(value: string | null) {
+ if (!value || !/^[1-9]\d*$/.test(value)) return null
+ const parsed = Number(value)
+ if (!Number.isSafeInteger(parsed)) return null
return parsed
}Also applies to: 184-190
🤖 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/admin-invites.ts` around lines 91 - 94, The
invite creation flow in admin-invites currently defaults invalid maxUses to 1,
which can silently create unintended one-use invites. Update the handling around
readPositiveInt in the invite handlers (including the create/update paths
referenced by admin-invites) to validate that maxUses is a safe positive integer
and return a 400 response when it is missing, malformed, or non-positive instead
of applying a fallback. Make sure the change is applied consistently anywhere
maxUses is parsed in the invite request handlers.
| const passwordHash = await createPasswordHash(normalizedPassword) | ||
| let consumedInviteCode: string | null = null | ||
| async function releaseConsumedInvite() { | ||
| if (!consumedInviteCode) return | ||
| await releaseInviteUse({ | ||
| db: appEnv.APP_DB, | ||
| code: consumedInviteCode, | ||
| }) | ||
| consumedInviteCode = null | ||
| } | ||
|
|
||
| const inviteRequired = isInviteRequiredForSignup(appEnv) | ||
| if (inviteRequired || normalizeInviteCode(inviteCode)) { | ||
| const inviteResult = await consumeInviteCode({ | ||
| db: appEnv.APP_DB, | ||
| code: inviteCode, | ||
| }) | ||
| if (!inviteResult.ok) { | ||
| void logAuditEvent({ | ||
| category: 'auth', | ||
| action: 'signup', | ||
| result: 'failure', | ||
| email: normalizedEmail, | ||
| ip: requestIp, | ||
| path: url.pathname, | ||
| reason: `invite_${inviteResult.reason}`, | ||
| }) | ||
| return Response.json( | ||
| { error: getInviteFailureMessage(inviteResult.reason) }, | ||
| { | ||
| status: | ||
| inviteResult.reason === 'missing' && inviteRequired | ||
| ? 400 | ||
| : 403, | ||
| }, | ||
| ) | ||
| } | ||
| consumedInviteCode = inviteResult.invite.code | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Validate the invite code before hashing the password.
Invite validation (cheap DB read/update) currently runs after createPasswordHash, which is deliberately expensive (PBKDF2, ~100k iterations per the dummy hash format). Since invite gating exists precisely to restrict who can sign up, an attacker can force full password-hash computation on every rejected signup attempt just by omitting/spoiling the invite code — wasted CPU on an unauthenticated endpoint. Move the invite check above the password hash so invalid/missing invites are rejected before the expensive hash runs.
⚡ Proposed reorder
- const passwordHash = await createPasswordHash(normalizedPassword)
let consumedInviteCode: string | null = null
async function releaseConsumedInvite() {
if (!consumedInviteCode) return
await releaseInviteUse({
db: appEnv.APP_DB,
code: consumedInviteCode,
})
consumedInviteCode = null
}
const inviteRequired = isInviteRequiredForSignup(appEnv)
if (inviteRequired || normalizeInviteCode(inviteCode)) {
const inviteResult = await consumeInviteCode({
db: appEnv.APP_DB,
code: inviteCode,
})
if (!inviteResult.ok) {
void logAuditEvent({
category: 'auth',
action: 'signup',
result: 'failure',
email: normalizedEmail,
ip: requestIp,
path: url.pathname,
reason: `invite_${inviteResult.reason}`,
})
return Response.json(
{ error: getInviteFailureMessage(inviteResult.reason) },
{
status:
inviteResult.reason === 'missing' && inviteRequired
? 400
: 403,
},
)
}
consumedInviteCode = inviteResult.invite.code
}
+ const passwordHash = await createPasswordHash(normalizedPassword)📝 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.
| const passwordHash = await createPasswordHash(normalizedPassword) | |
| let consumedInviteCode: string | null = null | |
| async function releaseConsumedInvite() { | |
| if (!consumedInviteCode) return | |
| await releaseInviteUse({ | |
| db: appEnv.APP_DB, | |
| code: consumedInviteCode, | |
| }) | |
| consumedInviteCode = null | |
| } | |
| const inviteRequired = isInviteRequiredForSignup(appEnv) | |
| if (inviteRequired || normalizeInviteCode(inviteCode)) { | |
| const inviteResult = await consumeInviteCode({ | |
| db: appEnv.APP_DB, | |
| code: inviteCode, | |
| }) | |
| if (!inviteResult.ok) { | |
| void logAuditEvent({ | |
| category: 'auth', | |
| action: 'signup', | |
| result: 'failure', | |
| email: normalizedEmail, | |
| ip: requestIp, | |
| path: url.pathname, | |
| reason: `invite_${inviteResult.reason}`, | |
| }) | |
| return Response.json( | |
| { error: getInviteFailureMessage(inviteResult.reason) }, | |
| { | |
| status: | |
| inviteResult.reason === 'missing' && inviteRequired | |
| ? 400 | |
| : 403, | |
| }, | |
| ) | |
| } | |
| consumedInviteCode = inviteResult.invite.code | |
| } | |
| let consumedInviteCode: string | null = null | |
| async function releaseConsumedInvite() { | |
| if (!consumedInviteCode) return | |
| await releaseInviteUse({ | |
| db: appEnv.APP_DB, | |
| code: consumedInviteCode, | |
| }) | |
| consumedInviteCode = null | |
| } | |
| const inviteRequired = isInviteRequiredForSignup(appEnv) | |
| if (inviteRequired || normalizeInviteCode(inviteCode)) { | |
| const inviteResult = await consumeInviteCode({ | |
| db: appEnv.APP_DB, | |
| code: inviteCode, | |
| }) | |
| if (!inviteResult.ok) { | |
| void logAuditEvent({ | |
| category: 'auth', | |
| action: 'signup', | |
| result: 'failure', | |
| email: normalizedEmail, | |
| ip: requestIp, | |
| path: url.pathname, | |
| reason: `invite_${inviteResult.reason}`, | |
| }) | |
| return Response.json( | |
| { error: getInviteFailureMessage(inviteResult.reason) }, | |
| { | |
| status: | |
| inviteResult.reason === 'missing' && inviteRequired | |
| ? 400 | |
| : 403, | |
| }, | |
| ) | |
| } | |
| consumedInviteCode = inviteResult.invite.code | |
| } | |
| const passwordHash = await createPasswordHash(normalizedPassword) |
🤖 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/auth.ts` around lines 182 - 220, The signup
flow in auth.ts currently computes createPasswordHash before validating the
invite, which wastes expensive CPU on rejected requests. Move the invite gating
logic in the signup handler so the inviteRequired/normalizeInviteCode check and
consumeInviteCode run before createPasswordHash, and only call
createPasswordHash after a valid invite has been accepted and consumed. Keep
releaseConsumedInvite handling tied to the successful invite path in the same
handler.
| const result = await verifyEmailToken({ | ||
| db: env.APP_DB, | ||
| token: url.searchParams.get('token'), | ||
| }) | ||
| const requestIp = getRequestIp(request) ?? undefined | ||
|
|
||
| if (!result.ok) { | ||
| void logAuditEvent({ | ||
| category: 'auth', | ||
| action: 'email_verify', | ||
| result: 'failure', | ||
| ip: requestIp, | ||
| path: url.pathname, | ||
| reason: result.reason, | ||
| }) | ||
| return renderAppPage({ | ||
| request, | ||
| env, | ||
| title: 'Verify email', | ||
| status: 400, | ||
| loaderData: { | ||
| emailVerification: { | ||
| ok: false, | ||
| error: getVerifyEmailError(result.reason), | ||
| }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files 'packages/worker/src/app/handlers/verify-email.ts' 'packages/worker/src/app/lib/*email*' 'packages/worker/src/app/**/*email*'
printf '\n== Outline verify-email.ts ==\n'
ast-grep outline packages/worker/src/app/handlers/verify-email.ts --view expanded || true
printf '\n== Read verify-email.ts ==\n'
cat -n packages/worker/src/app/handlers/verify-email.ts | sed -n '1,220p'
printf '\n== Locate verifyEmailToken ==\n'
rg -n "verifyEmailToken|email_verifications|email_verified_at|getVerifyEmailError|Verification link is invalid" packages/worker/src/app -S
printf '\n== Outline email-verification source candidates ==\n'
for f in $(git ls-files 'packages/worker/src/app/**/*email*' | grep -E 'email-verification|verification'); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
done
printf '\n== Read matching source slices ==\n'
for f in $(git ls-files 'packages/worker/src/app/**/*email*' | grep -E 'email-verification|verification'); do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: kentcdodds/kody
Length of output: 5155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Read email-verification.ts ==\n'
wc -l packages/worker/src/app/email-verification.ts
cat -n packages/worker/src/app/email-verification.ts | sed -n '1,260p'
printf '\n== Read auth-handler tests around email verification ==\n'
cat -n packages/worker/src/app/handlers/auth-handler.node.test.ts | sed -n '120,250p'
printf '\n== Search for repeated verify-email expectations ==\n'
rg -n "verify email|email_verify|Verification link|email_verifications|email_verified_at|already verified|single-use" packages/worker/src/app -SRepository: kentcdodds/kody
Length of output: 13775
Make the email-verify link idempotent
GET /verify-email consumes the token on first hit, so mailbox scanners can complete the flow before the user opens the link; a later click then shows “Verification link is invalid.” Keep the token valid until expiry, or switch to a user-initiated POST/confirmation step.
🤖 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/verify-email.ts` around lines 28 - 55, The
verify-email flow in verifyEmailToken currently consumes the token on the first
GET, which makes GET /verify-email non-idempotent and vulnerable to mailbox
scanners. Update the verify-email handler and the verifyEmailToken path so a GET
only validates/marks completion without invalidating the token, or move the
actual consumption to a user-initiated confirmation POST; keep the token usable
until expiry and preserve the existing logAuditEvent and renderAppPage behavior
in verify-email.ts.
| if (!result.ok) { | ||
| void logAuditEvent({ | ||
| category: 'auth', | ||
| action: 'email_verify', | ||
| result: 'failure', | ||
| ip: requestIp, | ||
| path: url.pathname, | ||
| reason: result.reason, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the router/handler execution wraps actions with ctx.waitUntil
rg -n "waitUntil" --type=ts -C3
ast-grep run --pattern 'export function createRouter($$$) { $$$ }' --lang typescript packages/worker/srcRepository: kentcdodds/kody
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the handler and router/runtime glue
git ls-files 'packages/worker/src/**' | sed -n '1,200p'
echo '--- verify-email.ts ---'
wc -l packages/worker/src/app/handlers/verify-email.ts
cat -n packages/worker/src/app/handlers/verify-email.ts | sed -n '1,220p'
echo '--- waitUntil references under worker src ---'
rg -n "waitUntil|ctx\.|ExecutionContext|logAuditEvent\(" packages/worker/src -S -C 3
echo '--- router/create action wiring ---'
rg -n "createRouter|action\(|loader\(|renderAppPage|verify-email" packages/worker/src -S -C 3Repository: kentcdodds/kody
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- app/handler.ts ---'
wc -l packages/worker/src/app/handler.ts
cat -n packages/worker/src/app/handler.ts | sed -n '1,260p'
echo '--- app/router.ts ---'
wc -l packages/worker/src/app/router.ts
cat -n packages/worker/src/app/router.ts | sed -n '1,260p'
echo '--- app/render.ts ---'
wc -l packages/worker/src/app/render.ts
cat -n packages/worker/src/app/render.ts | sed -n '1,240p'
echo '--- app/ssr-render.tsx ---'
wc -l packages/worker/src/app/ssr-render.tsx
cat -n packages/worker/src/app/ssr-render.tsx | sed -n '1,260p'
echo '--- app/audit-log.ts ---'
wc -l packages/worker/src/app/audit-log.ts
cat -n packages/worker/src/app/audit-log.ts | sed -n '1,220p'Repository: kentcdodds/kody
Length of output: 14671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- worker entrypoint ---'
wc -l packages/worker/src/index.ts
cat -n packages/worker/src/index.ts | sed -n '1,220p'
echo '--- request handling path ---'
rg -n "handleRequest\(|router\.fetch\(|ExecutionContext|waitUntil" packages/worker/src/index.ts packages/worker/src/app packages/worker/src -S -C 2 | sed -n '1,220p'
echo '--- verify-email test coverage ---'
rg -n "verify-email|email_verify|logAuditEvent|waitUntil" packages/worker/src/app/handlers/verify-email.* packages/worker/src/app/handlers/*.node.test.ts -S -C 2 | sed -n '1,220p'Repository: kentcdodds/kody
Length of output: 28519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- index.ts around app handler invocation ---'
sed -n '320,350p' packages/worker/src/index.ts | cat -n
echo '--- withCors implementation ---'
rg -n "function withCors|export function withCors" packages/worker/src -S -C 3Repository: kentcdodds/kody
Length of output: 1595
Keep the email_verify audit writes alive. The app handler never receives ExecutionContext, so these void logAuditEvent(...) calls can be dropped once renderAppPage(...) returns. Thread ctx through this path and register both success and failure audit writes with waitUntil().
🤖 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/verify-email.ts` around lines 34 - 42, The
email_verify audit event writes are currently fire-and-forget, so they can be
lost when the handler returns. Update the verify-email flow to thread the
ExecutionContext through the app handler path and use ctx.waitUntil() for both
the success and failure logAuditEvent calls. Make sure the changes are applied
in the verify-email handler and the renderAppPage app-path wiring so the audit
writes stay alive after the response is sent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e/playwright-utils.ts`:
- Line 49: The shared auth rate-limit reset in clearAuthRateLimitsInE2eDatabase
is unsafe for parallel Playwright workers because it can delete keys used by
other tests. Update the e2e setup so auth rate-limit data is isolated per worker
or per test (using a worker-specific prefix/namespace), or mark the auth-related
specs as serial where they depend on shared state. Keep the fix localized around
clearAuthRateLimitsInE2eDatabase and the related Playwright e2e auth setup.
🪄 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: 6004234c-aebe-4b6d-97cd-1361f4a533ef
📒 Files selected for processing (15)
docs/contributing/architecture/authentication.mddocs/contributing/architecture/authorization.mde2e/admin-rbac.spec.tse2e/d1-utils.tse2e/invite-signup-verification.spec.tse2e/playwright-utils.tse2e/smoke.spec.tspackages/worker/client/routes/admin-invites.tsxpackages/worker/src/app/admin-user-creation.node.test.tspackages/worker/src/app/admin-user-creation.tspackages/worker/src/app/handlers/admin-invites.tspackages/worker/src/app/handlers/password-reset.node.test.tspackages/worker/src/app/handlers/password-reset.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/password-reset-tokens.ts
✅ Files skipped from review due to trivial changes (1)
- docs/contributing/architecture/authorization.md
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/invite-signup-verification.spec.ts
- docs/contributing/architecture/authentication.md
- packages/worker/src/app/loader-data.ts
| return { email, username: primaryTestUser.username, password } | ||
| } | ||
|
|
||
| clearAuthRateLimitsInE2eDatabase() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Playwright config for worker/parallelism settings and DB isolation
fd -e ts -e js 'playwright.config' | xargs -I{} cat -n {}
rg -n "workers|fullyParallel" playwright.config.ts 2>/dev/nullRepository: kentcdodds/kody
Length of output: 1420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== playwright.config.ts =="
cat -n playwright.config.ts
echo
echo "== helper definition/usages =="
rg -n "clearAuthRateLimitsInE2eDatabase|auth:ip:|_rate_limits|rate limit" e2e -S
echo
echo "== tests that mention rate limiting or serial =="
rg -n "serial|rate limit|auth.*limit|login.*limit|signup.*limit" e2e -SRepository: kentcdodds/kody
Length of output: 3035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== e2e/playwright-utils.ts (relevant region) =="
sed -n '1,140p' e2e/playwright-utils.ts | cat -n
echo
echo "== e2e/d1-utils.ts (helper definition) =="
sed -n '90,130p' e2e/d1-utils.ts | cat -n
echo
echo "== e2e/smoke.spec.ts =="
sed -n '1,40p' e2e/smoke.spec.ts | cat -n
echo
echo "== e2e/invite-signup-verification.spec.ts (relevant region) =="
sed -n '50,95p' e2e/invite-signup-verification.spec.ts | cat -nRepository: kentcdodds/kody
Length of output: 11142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== e2e/d1-utils.ts (top section) =="
sed -n '1,110p' e2e/d1-utils.ts | cat -n
echo
echo "== wrangler config files mentioning CLOUDFLARE_ENV or D1 =="
rg -n "CLOUDFLARE_ENV|d1|database" wrangler* . -g '!node_modules' -g '!dist' -g '!build' -SRepository: kentcdodds/kody
Length of output: 50371
Avoid clearing shared auth rate limits in parallel e2e runs. This global delete can race with other Playwright workers against the shared e2e D1 file, masking or destabilizing auth rate-limit checks. Scope the keys per worker/test or make the auth specs serial.
🤖 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 `@e2e/playwright-utils.ts` at line 49, The shared auth rate-limit reset in
clearAuthRateLimitsInE2eDatabase is unsafe for parallel Playwright workers
because it can delete keys used by other tests. Update the e2e setup so auth
rate-limit data is isolated per worker or per test (using a worker-specific
prefix/namespace), or mark the auth-related specs as serial where they depend on
shared state. Keep the fix localized around clearAuthRateLimitsInE2eDatabase and
the related Playwright e2e auth setup.
| db: appEnv.APP_DB, | ||
| userId: userRecord.id, | ||
| expiresAt, | ||
| }) |
There was a problem hiding this comment.
Password reset replaces setup link
Medium Severity
Admin-created accounts get a seven-day setup link via password_resets, but a normal POST /password-reset for the same email calls createPasswordResetToken, which deletes existing rows and stores a new one-hour token. The admin’s copied link stops working, and the flow may auto-email a different reset link than the one the operator intended to send manually.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 95936ef. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
95936ef to
59c1681
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 59c1681. Configure here.
| if ((result.meta.changes ?? 0) > 0) { | ||
| const invite = await getInviteByCode(input.db, code) | ||
| if (!invite) return { ok: false, reason: 'not_found' } | ||
| return { ok: true, invite } |
There was a problem hiding this comment.
Invite use leaked on fetch failure
Low Severity
If the conditional invite UPDATE increments use_count but the follow-up getInviteByCode returns no row, consumeInviteCode reports failure without decrementing. Signup aborts without calling releaseInviteUse, so that invite use is consumed with no account created.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 59c1681. Configure here.


Summary
/verify-email, session/account UI state, outbound email blocking for unverified accounts, and account-deletion cleanup for verification tokens.0046, updates auth/RBAC docs, and covers invite/verification/admin-created-user flows with unit + Playwright tests.Validation
npm run validate✅System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@7019c1d· Head:59c1681Classification: extends — this PR changes auth/session behavior and extends the D1 app schema; no new system primitive id was added.
Primitives touched
app-sessionsrbacd1-app-dbinvites,email_verifications,users.email_verified_at, andusers.plan; reusespassword_resetsfor setup linksmcp-serverSystem map
Change flow
Before / after
users.email_verified_at, verification tokens, UI stateInvariants
userId; invite and direct-user creation are operator account metadata/actions behind the existing RBACadminrole.userrole and a no-usable-password sentinel until the displayed setup link is used.email_verifications.user_idalongside other integer user-id auth tables.Summary by CodeRabbit