From bf9248ed5ee0779be94f676d681a6fcb9f6a38bd Mon Sep 17 00:00:00 2001 From: Jatin Awankar <161307361+jatin-awankar@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:45:58 +0530 Subject: [PATCH] fix(subscriptions): preserve entire-page scope on repeated signup (#2859) * fix(subscriptions): preserve entire-page scope on repeated signup * fix(subscriptions): rotate verification token when scope expands * fix(subscriptions): reject rotated tokens during acceptance --- .../__tests__/page-subscriber.test.ts | 266 ++++++++++++++++-- .../services/src/page-subscriber/upsert.ts | 25 +- .../services/src/page-subscriber/verify.ts | 23 +- 3 files changed, 278 insertions(+), 36 deletions(-) diff --git a/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts b/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts index 5b8e510ad..b453e054f 100644 --- a/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts +++ b/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts @@ -18,9 +18,11 @@ import { describe, test, } from "@std/testing/bdd"; +import { stub } from "@std/testing/mock"; import { clearAuditLog, + clearAuditLogFor, createWorkspaceFixture, expectAuditRow, makeApiKeyCtx, @@ -162,35 +164,184 @@ describe("upsertSelfSignupSubscriber", () => { }); test("merges new components into an existing pending subscription", async () => { - const result = await upsertSelfSignupSubscriber({ - visitor: null, - input: { email, pageId: PAGE_ID, componentIds: [COMPONENT_1] }, + await withTestTransaction(async (tx) => { + const first = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: "scope-merge@example.com", + pageId: PAGE_ID, + componentIds: [COMPONENT_1], + }, + }); + const result = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: first.email, + pageId: PAGE_ID, + componentIds: [COMPONENT_2], + }, + }); + expect(result.id).toBe(first.id); + expect(result.componentIds).toEqual([COMPONENT_1, COMPONENT_2]); + const stored = await tx.query.pageSubscriberToPageComponent.findMany({ + where: eq(pageSubscriberToPageComponent.pageSubscriberId, first.id), + }); + expect(stored.map((row) => row.pageComponentId).sort()).toEqual( + [COMPONENT_1, COMPONENT_2].sort(), + ); + await expectAuditRow({ + workspaceId: WORKSPACE_ID, + action: "page_subscriber.update", + entityType: "page_subscriber", + entityId: result.id, + actorType: "subscriber", + db: tx, + }); + const rows = await readAuditLog({ + workspaceId: WORKSPACE_ID, + entityType: "page_subscriber", + entityId: result.id, + db: tx, + }); + expect( + rows.find((row) => row.action === "page_subscriber.update")?.metadata, + ).toEqual({ componentIds: [COMPONENT_1, COMPONENT_2] }); }); - expect(result.componentIds).toContain(COMPONENT_1); + }); - // Component-merge path emits an `update` audit row. - await expectAuditRow({ - workspaceId: WORKSPACE_ID, - action: "page_subscriber.update", - entityType: "page_subscriber", - entityId: result.id, - actorType: "subscriber", + for (const scenario of [ + "entire-page first", + "entire-page second", + "omitted components second", + ] as const) { + test(`preserves entire-page coverage when ${scenario}`, async () => { + await withTestTransaction(async (tx) => { + const first = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: "scope-entire-page@example.com", + pageId: PAGE_ID, + componentIds: scenario === "entire-page first" ? [] : [COMPONENT_1], + }, + }); + const result = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: first.email, + pageId: PAGE_ID, + componentIds: + scenario === "entire-page first" + ? [COMPONENT_2] + : scenario === "entire-page second" + ? [] + : undefined, + }, + }); + expect(result.id).toBe(first.id); + if (scenario === "entire-page first") { + expect(result.token).toBe(first.token); + } else { + expect(result.token).not.toBe(first.token); + } + expect(result.componentIds).toEqual([]); + expect( + await tx.query.pageSubscriberToPageComponent.findMany({ + where: eq(pageSubscriberToPageComponent.pageSubscriberId, first.id), + }), + ).toEqual([]); + + if (scenario !== "entire-page first") { + const rows = await readAuditLog({ + workspaceId: WORKSPACE_ID, + entityType: "page_subscriber", + entityId: result.id, + db: tx, + }); + const scopeChange = rows.find( + (row) => row.action === "page_subscriber.update", + ); + expect(scopeChange?.actorType).toBe("subscriber"); + expect(scopeChange?.metadata).toEqual({ componentIds: [] }); + expect(scopeChange?.after).not.toHaveProperty("token"); + } + + if (!result.token) throw new Error("Expected a verification token"); + const verified = await verifySelfSignupSubscriber({ + input: { token: result.token }, + db: tx, + }); + expect(verified?.acceptedAt).not.toBeNull(); + expect(verified?.componentIds).toEqual([]); + }); }); + } - // Metadata records the merged component-id set so the change isn't - // dropped by the empty-diff guard in `emitAudit`. - const rows = await readAuditLog({ - workspaceId: WORKSPACE_ID, - entityType: "page_subscriber", - entityId: result.id, + for (const scope of ["entire page", "additional component"] as const) { + test(`old verification link cannot confirm expanded scope: ${scope}`, async () => { + await withTestTransaction(async (tx) => { + const first = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: "scope-verification@example.com", + pageId: PAGE_ID, + componentIds: [COMPONENT_1], + }, + }); + if (!first.token) throw new Error("Expected a verification token"); + await tx + .update(pageSubscriber) + .set({ expiresAt: new Date("2020-01-01T00:00:00Z") }) + .where(eq(pageSubscriber.id, first.id)); + await expect( + verifySelfSignupSubscriber({ + input: { token: first.token }, + db: tx, + }), + ).rejects.toThrow("Verification token expired"); + + const expanded = await upsertSelfSignupSubscriber({ + visitor: null, + db: tx, + input: { + email: first.email, + pageId: PAGE_ID, + componentIds: scope === "entire page" ? [] : [COMPONENT_2], + }, + }); + expect( + await verifySelfSignupSubscriber({ + input: { token: first.token }, + db: tx, + }), + ).toBeNull(); + expect( + ( + await tx.query.pageSubscriber.findFirst({ + where: eq(pageSubscriber.id, first.id), + }) + )?.acceptedAt, + ).toBeNull(); + expect(expanded.token).not.toBe(first.token); + if (!expanded.token) + throw new Error("Expected a new verification token"); + const verified = await verifySelfSignupSubscriber({ + input: { token: expanded.token }, + db: tx, + }); + expect(verified?.acceptedAt).toBeInstanceOf(Date); + expect(verified?.componentIds.sort((a, b) => a - b)).toEqual( + scope === "entire page" + ? [] + : [COMPONENT_1, COMPONENT_2].sort((a, b) => a - b), + ); + }); }); - const updateRow = rows.find((r) => r.action === "page_subscriber.update"); - const metadata = updateRow?.metadata as - | { componentIds?: number[] } - | null - | undefined; - expect(metadata?.componentIds).toContain(COMPONENT_1); - }); + } test("refreshes expiresAt for a still-pending subscription", async () => { const before = new Date(); @@ -373,6 +524,73 @@ describe("verifySelfSignupSubscriber", () => { pendingId = sub.id; }); + for (const scope of ["entire page", "additional component"] as const) { + test(`rejects an in-flight verification when scope expands to ${scope}`, async () => { + const first = await upsertSelfSignupSubscriber({ + visitor: null, + input: { + email: `verify-race-${crypto.randomUUID()}@example.com`, + pageId: PAGE_ID, + componentIds: [COMPONENT_1], + }, + }); + if (!first.token) throw new Error("Expected a verification token"); + let expanded: + | Awaited> + | undefined; + const transaction = db.transaction.bind(db); + // Commit the competing signup after token lookup, before acceptance starts. + const intercept = stub(db, "transaction", async (fn, config) => { + intercept.restore(); + expanded = await upsertSelfSignupSubscriber({ + visitor: null, + input: { + email: first.email, + pageId: PAGE_ID, + componentIds: scope === "entire page" ? [] : [COMPONENT_2], + }, + }); + return transaction(fn, config); + }); + try { + const result = await verifySelfSignupSubscriber({ + input: { token: first.token, domain: PAGE_SLUG }, + }); + expect(result).toBeNull(); + if (!expanded?.token) throw new Error("Expected a rotated token"); + expect(expanded.token).not.toBe(first.token); + const stored = await db.query.pageSubscriber.findFirst({ + where: eq(pageSubscriber.id, first.id), + }); + expect(stored?.acceptedAt).toBeNull(); + expect(stored?.token).toBe(expanded.token); + const audit = await readAuditLog({ + workspaceId: WORKSPACE_ID, + entityType: "page_subscriber", + entityId: first.id, + }); + expect(audit).toHaveLength(2); + expect(audit.some((row) => row.after?.acceptedAt != null)).toBe(false); + const verified = await verifySelfSignupSubscriber({ + input: { token: expanded.token, domain: PAGE_SLUG }, + }); + expect(verified?.acceptedAt).toBeInstanceOf(Date); + expect(verified?.componentIds.sort((a, b) => a - b)).toEqual( + scope === "entire page" + ? [] + : [COMPONENT_1, COMPONENT_2].sort((a, b) => a - b), + ); + } finally { + if (!intercept.restored) intercept.restore(); + await db.delete(pageSubscriber).where(eq(pageSubscriber.id, first.id)); + await clearAuditLogFor({ + entityType: "page_subscriber", + entityIds: [first.id], + }); + } + }); + } + test("returns null for an unknown token", async () => { const result = await verifySelfSignupSubscriber({ input: { token: "non-existent-token-xyz" }, diff --git a/packages/services/src/page-subscriber/upsert.ts b/packages/services/src/page-subscriber/upsert.ts index 2e0425bf0..15dbd43e9 100644 --- a/packages/services/src/page-subscriber/upsert.ts +++ b/packages/services/src/page-subscriber/upsert.ts @@ -126,8 +126,25 @@ export async function upsertSelfSignupSubscriber(args: { if (existing) { // Pending row — merge components, refresh expiry. const currentIds = existing.components.map((c) => c.pageComponentId); - const mergedIds = [...new Set([...currentIds, ...componentIds])]; + // Empty scope means the entire page, so it absorbs component scopes. + const mergedIds = + currentIds.length === 0 || componentIds.length === 0 + ? [] + : [...new Set([...currentIds, ...componentIds])]; const newIds = mergedIds.filter((id) => !currentIds.includes(id)); + const scopeChanged = + mergedIds.length !== currentIds.length || newIds.length > 0; + // A link issued for the previous scope must not authorize wider coverage. + const token = scopeChanged ? crypto.randomUUID() : existing.token; + + if (currentIds.length > 0 && mergedIds.length === 0) { + await tx + .delete(pageSubscriberToPageComponent) + .where( + eq(pageSubscriberToPageComponent.pageSubscriberId, existing.id), + ) + .run(); + } if (newIds.length > 0) { await tx @@ -146,7 +163,7 @@ export async function upsertSelfSignupSubscriber(args: { const beforeRow = selectPageSubscriberSchema.parse(existing); const updatedRow = await tx .update(pageSubscriber) - .set({ expiresAt: newExpiresAt, updatedAt: new Date() }) + .set({ token, expiresAt: newExpiresAt, updatedAt: new Date() }) .where(eq(pageSubscriber.id, existing.id)) .returning() .get(); @@ -169,7 +186,7 @@ export async function upsertSelfSignupSubscriber(args: { entityId: existing.id, before: beforeSnap, after: afterSnap, - ...(newIds.length > 0 ? { metadata: { componentIds: mergedIds } } : {}), + ...(scopeChanged ? { metadata: { componentIds: mergedIds } } : {}), }); return { @@ -180,7 +197,7 @@ export async function upsertSelfSignupSubscriber(args: { customDomain: pageData.customDomain, channelType: existing.channelType, email: existing.email ?? emailLower, - token: existing.token, + token, acceptedAt: null, componentIds: mergedIds, }; diff --git a/packages/services/src/page-subscriber/verify.ts b/packages/services/src/page-subscriber/verify.ts index 120d051f7..1f8450f7c 100644 --- a/packages/services/src/page-subscriber/verify.ts +++ b/packages/services/src/page-subscriber/verify.ts @@ -1,4 +1,4 @@ -import { eq } from "@openstatus/db"; +import { and, eq } from "@openstatus/db"; import { pageSubscriber, selectPageSubscriberSchema, @@ -75,10 +75,17 @@ export async function verifySelfSignupSubscriber(args: { const updated = await tx .update(pageSubscriber) .set({ acceptedAt: new Date(), updatedAt: new Date() }) - .where(eq(pageSubscriber.id, row.id)) + // Signup may rotate the token after the lookup above. + .where( + and( + eq(pageSubscriber.id, row.id), + eq(pageSubscriber.token, input.token), + ), + ) .returning() .get(); - const after = selectPageSubscriberSchema.parse(updated ?? row); + if (!updated) return null; + const after = selectPageSubscriberSchema.parse(updated); const auditCtx: ServiceContext = { workspace: pageData.workspace, @@ -101,11 +108,11 @@ export async function verifySelfSignupSubscriber(args: { pageName: pageData.title, pageSlug: pageData.slug, customDomain: pageData.customDomain, - channelType: updated?.channelType ?? row.channelType, - email: updated?.email ?? row.email, - webhookUrl: updated?.webhookUrl ?? row.webhookUrl, - token: updated?.token ?? row.token, - acceptedAt: updated?.acceptedAt ?? new Date(), + channelType: updated.channelType, + email: updated.email, + webhookUrl: updated.webhookUrl, + token: updated.token, + acceptedAt: updated.acceptedAt, componentIds: components, }; }); -- 2.54.0