-
Notifications
You must be signed in to change notification settings - Fork 0
fix: AMPH-v2 full codebase audit remediation (C1-C7, H1-H7) #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f5e61b8
7454f57
ce8d3dc
0b2acf7
5d9d709
02b0055
7fbd4e6
2b3b21b
353e161
b3b42a3
5e79ce1
aa0622d
c3b286a
dc97161
11dc15d
ab7a2f6
bb4eba4
b055cf5
96db602
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| -- Add claim token fields to User model (C4 - guest account claiming fix) | ||
| ALTER TABLE "User" ADD COLUMN "claimTokenHash" TEXT; | ||
| ALTER TABLE "User" ADD COLUMN "claimTokenExpiresAt" TIMESTAMP(3); | ||
|
|
||
| CREATE INDEX "User_claimTokenHash_idx" ON "User"("claimTokenHash"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # Please do not edit this file manually | ||
| # It should be added in your version-control system (i.e. Git) | ||
| provider = "postgresql" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| 'use server'; | ||
|
|
||
| import { redirect } from 'next/navigation'; | ||
| import { z } from 'zod'; | ||
| import { db } from '@/lib/db'; | ||
| import { | ||
| hashPassword, | ||
|
|
@@ -13,6 +14,7 @@ import { | |
| setAuthCookie, | ||
| clearAuthCookie, | ||
| getSession, | ||
| verifyClaimToken, | ||
| } from '@/lib/auth'; | ||
| import { logger } from '@/lib/logger'; | ||
| import { rateLimit } from '@/lib/rate-limit'; | ||
|
|
@@ -27,7 +29,12 @@ import { | |
| // Sign up | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export const signUpAction = createSafeAction(signUpSchema, async (data) => { | ||
| // Extended schema for guest-account claiming with a claim token | ||
| const signUpWithClaimSchema = signUpSchema.extend({ | ||
| claimToken: z.string().optional(), | ||
| }); | ||
|
|
||
| export const signUpAction = createSafeAction(signUpWithClaimSchema, async (data) => { | ||
| const rl = rateLimit(`signup:${data.email.toLowerCase()}`, 5, 60_000); | ||
| if (!rl.allowed) { | ||
| throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`); | ||
|
|
@@ -42,17 +49,37 @@ export const signUpAction = createSafeAction(signUpSchema, async (data) => { | |
| // password. Upgrade in place: replace hash, set name, mark verified. | ||
| // 3. Existing email with a real passwordHash — email is taken. | ||
| if (existing) { | ||
| const isPlaceholder = | ||
| existing.passwordHash && existing.passwordHash.startsWith('placeholder_'); | ||
| if (!isPlaceholder) { | ||
| const isClaimable = | ||
| existing.passwordHash === 'placeholder_claim' && | ||
| existing.claimTokenHash && | ||
| existing.claimTokenExpiresAt && | ||
| existing.claimTokenExpiresAt > new Date(); | ||
| if (!isClaimable) { | ||
| throw new Error('An account with that email already exists.'); | ||
| } | ||
|
|
||
| // Validate the claim token (C4 - must prove access to the email inbox) | ||
| if (!data.claimToken || !existing.claimTokenHash || !existing.claimTokenExpiresAt) { | ||
| throw new Error( | ||
| 'This email is registered to a guest checkout account. ' + | ||
| 'Check your email for the claim link with your account token.', | ||
| ); | ||
| } | ||
| if (!verifyClaimToken(data.claimToken, existing.claimTokenHash, existing.claimTokenExpiresAt)) { | ||
| throw new Error( | ||
| 'Invalid or expired claim token. Request a new claim link.', | ||
| ); | ||
| } | ||
|
|
||
| // Invalidate all claim tokens after successful use | ||
| const upgraded = await db.user.update({ | ||
| where: { id: existing.id }, | ||
| data: { | ||
| name: data.name ?? existing.name, | ||
| passwordHash: await hashPassword(data.password), | ||
| emailVerified: new Date(), | ||
| claimTokenHash: null, | ||
| claimTokenExpiresAt: null, | ||
| }, | ||
| }); | ||
|
Comment on lines
+52
to
84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift Atomically consume the one-time claim token. Two concurrent requests can both verify the same hash before either update clears it. Both then set a password and receive valid sessions, with the last password winning. Guard the update by the placeholder marker, token hash, and expiry, and proceed only when exactly one row is updated. Add a concurrent regression test. As per coding guidelines, when fixing a defect, reproduce it with the smallest test, fix the root cause, and add a regression test. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| const token = await signToken({ | ||
|
|
@@ -153,10 +180,13 @@ export async function signOutAction(): Promise<ActionResult<{ ok: true }>> { | |
| // --------------------------------------------------------------------------- | ||
|
|
||
| export async function signUpFormAction(formData: FormData): Promise<void> { | ||
| const password = formData.get('password') as string; | ||
| const result = await signUpAction({ | ||
| email: formData.get('email'), | ||
| password: formData.get('password'), | ||
| password, | ||
| confirmPassword: password, // Same as password for no-JS path (progressive enhancement) | ||
| name: formData.get('name') || undefined, | ||
| claimToken: (formData.get('claimToken') as string) || undefined, | ||
| }); | ||
|
|
||
| if (result.success) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use
safeExtendfor the refined Zod schema.signUpSchemacontains.refine(...). In Zod 4, extending a schema with refinements through.extend()can throw during module initialization. Use.safeExtend(...)and cover schema construction in a test.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents