diff --git a/package.json b/package.json index 306a4b7..d470759 100644 --- a/package.json +++ b/package.json @@ -18,9 +18,9 @@ "@eslint/js": "^9.35.0", "@types/better-sqlite3": "^7.6.13", "@types/express": "^5.0.3", - "@types/express-ws": "^3.0.5", "@types/mjml": "^4.7.4", "@types/mustache": "^4.2.6", + "@types/node": "^22.18.1", "@types/nodemailer": "^8.0.1", "@types/sanitize-html": "^2.16.0", "@types/ws": "^8.18.1", @@ -34,7 +34,6 @@ "vitest": "^5.0.0" }, "dependencies": { - "@types/node": "^22.18.1", "@welshman/content": "^0.6.3", "@welshman/feeds": "^0.6.3", "@welshman/lib": "^0.6.3", @@ -43,13 +42,11 @@ "@welshman/signer": "^0.6.3", "@welshman/store": "^0.6.3", "@welshman/util": "^0.6.3", - "bcrypt": "^5.1.1", "cron": "^4.3.3", "cron-parser": "^5.3.1", "dotenv": "^16.6.1", "express": "^4.21.2", "express-rate-limit": "^7.5.1", - "express-ws": "^5.0.2", "localstorage-polyfill": "^1.0.1", "mjml": "^4.15.3", "mustache": "^4.2.0", @@ -58,12 +55,10 @@ "sanitize-html": "^2.17.0", "sqlite3": "^5.1.7", "succinct-async": "^1.0.4", - "ts-node-dev": "^2.0.0", "ws": "^8.18.3" }, "pnpm": { "onlyBuiltDependencies": [ - "bcrypt", "sqlite3" ] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f444b05..95b78b8 100644 Binary files a/pnpm-lock.yaml and b/pnpm-lock.yaml differ 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', () => {