From 9fa1f73316b64f447a263ca609c55844f15db9a9 Mon Sep 17 00:00:00 2001 From: Agent Date: Thu, 17 Sep 2026 16:57:30 -0400 Subject: [PATCH] fix: distinguish already-confirmed tokens from invalid ones; prevent duplicate active subscriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem ------- 1. Re-clicking an already-confirmed confirmation link (e.g. /confirm?token=…) returned from confirmSubscription because the SQL WHERE clause required . The caller then threw an ActionError('invalid or expired') which rendered the 'Email not confirmed' error page — misleading for someone who had already confirmed. 2. A second PUT /subscription/email with the same email+frequency could silently bypass the upsert path when getSubscriptionByPubkey found the active row but updateSubscription returned it unchanged (email and frequency matched). While the unique index prevented a true duplicate INSERT, the code path was fragile and the regression test was missing. Changes ------- database.ts: - confirmSubscription now returns { sub, alreadyConfirmed } | undefined. First it tries the existing UPDATE (unconfirmed tokens only). If that returns no rows, it looks up the key directly: if the row exists and is already confirmed, returns { sub, alreadyConfirmed: true }. If the row doesn't exist or is unsubscribed, returns undefined (invalid/expired). - Exported new ConfirmResult type for callers. actions.ts: - confirmSubscriptionAction destructures the new return type. - Only registers the cron job on fresh confirmation (not re-confirms). - Returns the ConfirmResult so the route can distinguish the two cases. server.ts: - /confirm route checks result.alreadyConfirmed and renders confirm-already.html instead of confirm-success.html. pages/confirm-already.html: - New page with title 'Email already confirmed' and an info message explaining the address was already confirmed. Tests: - test/confirm-already-confirmed.test.ts — NEW (3 tests): first confirm succeeds with alreadyConfirmed=false; second confirm returns alreadyConfirmed=true; nonexistent token returns undefined. - test/duplicate-subscription.test.ts — NEW (4 tests): full cycle of register → confirm → re-register → assert one active row with unchanged key, verifying the upsert is idempotent. - Adapted 3 existing test files to destructure the new ConfirmResult. --- src/actions.ts | 12 +++- src/database.ts | 25 ++++++++- src/pages/confirm-already.html | 62 +++++++++++++++++++++ src/server.ts | 8 ++- test/confirm-already-confirmed.test.ts | 44 +++++++++++++++ test/duplicate-subscription.test.ts | 50 +++++++++++++++++ test/event-arrival-race.test.ts | 2 +- test/notify-response-shape.test.ts | 2 +- test/reschedule-on-frequency-change.test.ts | 2 +- 9 files changed, 198 insertions(+), 9 deletions(-) create mode 100644 src/pages/confirm-already.html create mode 100644 test/confirm-already-confirmed.test.ts create mode 100644 test/duplicate-subscription.test.ts diff --git a/src/actions.ts b/src/actions.ts index 975accb..3235afd 100644 --- a/src/actions.ts +++ b/src/actions.ts @@ -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 }, ) diff --git a/src/database.ts b/src/database.ts index 718e225..25d70d1 100644 --- a/src/database.ts +++ b/src/database.ts @@ -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 => { + // 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 } } ) diff --git a/src/pages/confirm-already.html b/src/pages/confirm-already.html new file mode 100644 index 0000000..e511e1c --- /dev/null +++ b/src/pages/confirm-already.html @@ -0,0 +1,62 @@ + + + + + + Email Already Confirmed + + + +
+ {{#brandLogo}}{{/brandLogo}} +
{{brandName}}
+

Email already confirmed

+

+ This email address has already been confirmed. You're all set — no further action needed. + Visit {{brandName}} to manage your notification settings. +

+
+ + \ No newline at end of file diff --git a/src/server.ts b/src/server.ts index c7acb43..54b30ad 100644 --- a/src/server.ts +++ b/src/server.ts @@ -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(), diff --git a/test/confirm-already-confirmed.test.ts b/test/confirm-already-confirmed.test.ts new file mode 100644 index 0000000..2d3cde3 --- /dev/null +++ b/test/confirm-already-confirmed.test.ts @@ -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() + }) +}) \ No newline at end of file diff --git a/test/duplicate-subscription.test.ts b/test/duplicate-subscription.test.ts new file mode 100644 index 0000000..e7b3371 --- /dev/null +++ b/test/duplicate-subscription.test.ts @@ -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) + }) +}) \ No newline at end of file diff --git a/test/event-arrival-race.test.ts b/test/event-arrival-race.test.ts index 93e5c44..b24175a 100644 --- a/test/event-arrival-race.test.ts +++ b/test/event-arrival-race.test.ts @@ -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 diff --git a/test/notify-response-shape.test.ts b/test/notify-response-shape.test.ts index 9d229b8..01491d8 100644 --- a/test/notify-response-shape.test.ts +++ b/test/notify-response-shape.test.ts @@ -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((resolve) => { diff --git a/test/reschedule-on-frequency-change.test.ts b/test/reschedule-on-frequency-change.test.ts index 208ff93..6e78919 100644 --- a/test/reschedule-on-frequency-change.test.ts +++ b/test/reschedule-on-frequency-change.test.ts @@ -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', () => { -- 2.45.2