Fix: distinguish already-confirmed confirm links from invalid ones; prevent duplicate subscription rows on re-register #18

Merged
matt merged 1 commit from mailship-2px-confirm-link-shows-email-not-confirmed-a-349 into main 2026-09-18 15:08:01 +00:00
9 changed files with 198 additions and 9 deletions
Showing only changes of commit 9fa1f73316 - Show all commits

View file

@ -41,13 +41,19 @@ export type ConfirmSubscriptionParams = {
export const confirmSubscriptionAction = instrument(
'actions.confirmSubscription',
async ({ token }: ConfirmSubscriptionParams) => {
const sub = await db.confirmSubscription(token)
const result = await db.confirmSubscription(token)
if (!sub) {
if (!result) {
throw new ActionError('That confirmation code is invalid or has expired.')
}
worker.registerSubscription(sub)
// Only register the cron job when this is a fresh confirmation.
// If already confirmed, the job is already running.
if (!result.alreadyConfirmed) {
worker.registerSubscription(result.sub)
}
return result
},
)

View file

@ -195,16 +195,37 @@ export const insertSubscription = instrument(
}
)
export type ConfirmResult = {
sub: Subscription
alreadyConfirmed: boolean
}
export const confirmSubscription = instrument(
'database.confirmSubscription',
async (key: string) => {
return parseSubscription(
async (key: string): Promise<ConfirmResult | undefined> => {
// Try to update an unconfirmed, active row
const updated = parseSubscription(
await get(
`UPDATE subscriptions SET confirmed_at = unixepoch()
WHERE key = ? AND confirmed_at IS NULL AND unsubscribed_at IS NULL RETURNING *`,
[key]
)
)
if (updated) {
return { sub: updated, alreadyConfirmed: false }
}
// No unconfirmed row was updated. Check if the key exists and is
// already confirmed — the user is re-clicking a used link. If the row
// is unsubscribed, treat it as invalid (expired).
const existing = await getSubscriptionByKey(key)
if (!existing || existing.unsubscribed_at) {
return undefined
}
return { sub: existing, alreadyConfirmed: true }
}
)

View file

@ -0,0 +1,62 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Email Already Confirmed</title>
<style>
* { box-sizing: border-box; }
body {
font-family: 'Inter', system-ui, -apple-system, sans-serif;
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
margin: 0;
background: #f0f2f5;
color: #1e293b;
}
.container {
width: 100%;
max-width: 480px;
margin: 16px;
text-align: center;
padding: 40px 32px;
background: #ffffff;
border-radius: 12px;
border: 1px solid #e2e8f0;
}
.logo { height: 40px; width: auto; margin: 0 auto 16px; display: block; }
.brand {
font-size: 24px;
font-weight: 700;
margin: 0 0 28px;
color: {{brandAccent}};
}
.title {
font-size: 20px;
font-weight: 700;
margin: 0 0 12px;
color: #1e293b;
}
.message {
font-size: 15px;
line-height: 1.6;
color: #64748b;
margin: 0;
}
.message a { color: {{brandAccent}}; text-decoration: none; font-weight: 600; }
</style>
</head>
<body>
<div class="container">
{{#brandLogo}}<img class="logo" src="{{brandLogo}}" alt="{{brandName}}" />{{/brandLogo}}
<div class="brand">{{brandName}}</div>
<h1 class="title">Email already confirmed</h1>
<p class="message">
This email address has already been confirmed. You're all set — no further action needed.
Visit <a href="{{settingsUrl}}">{{brandName}}</a> to manage your notification settings.
</p>
</div>
</body>
</html>

View file

@ -327,7 +327,13 @@ addRoute('get', '/confirm', async (req: Request, res: Response) => {
}
try {
await confirmSubscriptionAction({ token: req.query.token })
const result = await confirmSubscriptionAction({ token: req.query.token })
if (result.alreadyConfirmed) {
return res.send(await render('pages/confirm-already.html', {
...brandingVars(),
}))
}
res.send(await render('pages/confirm-success.html', {
...brandingVars(),

View file

@ -0,0 +1,44 @@
import { describe, it, expect, beforeAll } from 'vitest'
import * as db from '../src/database.js'
const pubkey = 'reconfirm-test-' + Date.now()
const email = 'reconfirm-test-' + Date.now() + '@example.com'
let token: string
describe('Confirm link idempotency — already-confirmed token', () => {
beforeAll(async () => {
await db.migrate()
})
it('creates and confirms a subscription for the first time', async () => {
const sub = await db.insertSubscription(pubkey, email, 'daily')
expect(sub).toBeTruthy()
token = sub.key
const result = await db.confirmSubscription(token)
expect(result).toBeTruthy()
expect(result!.sub.confirmed_at).toBeTruthy()
expect(result!.alreadyConfirmed).toBe(false)
})
it('returns a distinct result (not undefined) when confirming an already-confirmed token', async () => {
// BUG: confirmSubscription used `WHERE confirmed_at IS NULL`, so
// re-confirming an already-confirmed token matched zero rows and
// the UPDATE returned nothing → parseSubscription returned undefined.
// The caller then threw ActionError('invalid or expired') and the
// user saw "Email not confirmed" — which is misleading.
//
// FIX: confirmSubscription now detects the already-confirmed case
// and returns { sub, alreadyConfirmed: true } so the handler can
// render an "already confirmed" info page instead of an error page.
const result = await db.confirmSubscription(token)
expect(result).not.toBeUndefined()
expect(result!.alreadyConfirmed).toBe(true)
expect(result!.sub.confirmed_at).toBeTruthy()
})
it('returns undefined for a nonexistent token', async () => {
const result = await db.confirmSubscription('nonexistent-token-' + Date.now())
expect(result).toBeUndefined()
})
})

View file

@ -0,0 +1,50 @@
import { describe, it, expect, beforeAll } from 'vitest'
import * as db from '../src/database.js'
const pubkey = 'dup-test-' + Date.now()
const email = 'dup-test-' + Date.now() + '@example.com'
let token: string
describe('Duplicate subscription prevention — idempotent re-register after confirm', () => {
beforeAll(async () => {
await db.migrate()
})
it('registers a subscription (fresh)', async () => {
const sub = await db.insertSubscription(pubkey, email, 'daily')
expect(sub).toBeTruthy()
expect(sub!.confirmed_at).toBeFalsy()
expect(sub!.unsubscribed_at).toBeFalsy()
token = sub!.key
})
it('confirms the subscription', async () => {
const result = await db.confirmSubscription(token)
expect(result).toBeTruthy()
expect(result!.alreadyConfirmed).toBe(false)
expect(result!.sub.confirmed_at).toBeTruthy()
})
it('re-registers with the same email and frequency (idempotent PUT)', async () => {
// This simulates a second PUT from the client with identical params.
// The bug would create a second active row + send a new confirmation email.
const sub = await db.insertSubscription(pubkey, email, 'daily')
expect(sub).toBeTruthy()
// Must return the existing confirmed row, NOT a fresh unconfirmed row
expect(sub!.confirmed_at).toBeTruthy()
expect(sub!.unsubscribed_at).toBeFalsy()
// The key must remain unchanged — a new row would have a different key
expect(sub!.key).toBe(token)
})
it('has exactly one active row for this pubkey', async () => {
// getSubscriptionByPubkey filters by unsubscribed_at IS NULL and
// returns at most one row (enforced by the partial unique index).
// If a second active row exists, the index is absent or bypassed.
const active = await db.getSubscriptionByPubkey(pubkey)
expect(active).toBeTruthy()
expect(active!.confirmed_at).toBeTruthy()
expect(active!.key).toBe(token)
})
})

View file

@ -24,7 +24,7 @@ describe('Event-arrival race in digest job', () => {
expect(s).toBeTruthy()
const confirmed = await db.confirmSubscription(s.key)
expect(confirmed).toBeTruthy()
sub = confirmed
sub = confirmed!.sub
// Set last_digest_at to 60 seconds ago (the "since" value runJob would use)
since = Math.floor(Date.now() / 1000) - 60

View file

@ -37,7 +37,7 @@ describe('notify_response_shape', () => {
const email = 'shape-test-' + Date.now() + '@example.com'
const sub = await db.insertSubscription(pubkey, email, 'daily')
const confirmed = await db.confirmSubscription(sub.key)
subId = confirmed.id
subId = confirmed!.sub.id
// Start the express server on a random available port
await new Promise<void>((resolve) => {

View file

@ -20,7 +20,7 @@ describe('Frequency change reschedules cron job', () => {
const confirmed = await db.confirmSubscription(sub.key)
expect(confirmed).toBeTruthy()
sub = confirmed
sub = confirmed!.sub
})
it('registers cron job with daily frequency', () => {