From e31fa65da2e1753a16b8475a8e23a28718e51314 Mon Sep 17 00:00:00 2001 From: David Condrey Date: Tue, 4 Aug 2026 22:10:42 -0700 Subject: [PATCH 1/5] fix(server): keep CRM analytics queries inside D1's bound-parameter limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1 refuses any query carrying more than 100 bound parameters, and the company rollup's consent lookup binds site_id and now on top of one per contact. A company with 99 linkable contacts therefore asked for 101 and the statement was rejected outright: a hard 500, on precisely the largest account rather than on the small ones a test reaches for. COMPANY_ROLLUP_MAX_CONTACTS was set to 100 as though the whole allowance were available. The events queries had the same defect and no cap at all. Their IN list is the union of every linked contact's live salt windows, so it is contacts multiplied by windows, and a single contact could reach it too on any deployment that raises RAW_RETENTION_DAYS. All three now chunk, with the merge done so it stays exact. Totals add; first/last seen take the extremes across chunks rather than the last one processed. The path ranking deliberately does NOT take a per-chunk top ten and merge the survivors: a path in the overall top ten need not be in any single chunk's, so that would return a plausible and subtly wrong ranking. It groups fully and ranks once. The export likewise re-ranks across chunks before applying its cap, or it would return the newest-per-chunk concatenated and call it the newest. Also fixed, all found reviewing the same code: A capped rollup answered `reason: no_linked_contacts`, which is a claim about contacts it never examined. It now says `none_linked_within_cap` when the fan-out was truncated. deleteCompany captured the company name before the transaction and wrote that captured value into every contact, so a rename committing in between stamped them all with the superseded name — with the company row then deleted and nothing left to correct it against. The name is now read by a correlated subquery inside the batch. The same statement counted the unlinked contacts by materialising one row per contact through `.returning()`; it now counts inside the transaction instead of reading tens of thousands of rows to produce one integer. Erasing a contact wrote to two databases with no transaction spanning them, deleting the contact first. A failure on the second write destroyed the only record of which uid to erase, stranding consent rows holding that person's raw identifier — the exact data the request was about, now unreachable by any retry. Erasing consent first leaves a retryable state. A foreign-key violation reached the client as a 500. resolveCompany checks the company exists, but the check and the write are separate statements, so a concurrent company delete makes the write fail on the constraint. That is the caller losing a race, and unknown_company is the honest answer. ContactUpdateSchema dropped the identifier invariant that ContactCreateSchema enforces, so a PATCH could blank email, external id and name and leave a row that can never be matched, deduped or erased on request — NULLs being distinct in both unique indexes, nothing downstream would object. The check runs against the merged row, so clearing one identifier while another survives is still ordinary editing. Also, autonomously: verified against a live probe that D1 enforces the foreign key and that the bound-parameter ceiling is exactly 100 (98 ids plus 2 fixed binds passes, 99 fails), rather than trusting either from documentation alone; both are now pinned by tests. --- apps/server/src/db/contact-analytics.ts | 157 +++++++++++------- apps/server/src/db/crm.ts | 98 +++++++++-- apps/server/src/lib/consent.ts | 54 +++--- apps/server/src/lib/constants.ts | 21 +++ apps/server/src/routes/crm.ts | 86 ++++++---- .../test/contact-analytics-chunking.test.ts | 92 ++++++++++ apps/server/test/crm.test.ts | 155 ++++++++++++++++- 7 files changed, 538 insertions(+), 125 deletions(-) create mode 100644 apps/server/test/contact-analytics-chunking.test.ts diff --git a/apps/server/src/db/contact-analytics.ts b/apps/server/src/db/contact-analytics.ts index 0eda6f8..7a86f38 100644 --- a/apps/server/src/db/contact-analytics.ts +++ b/apps/server/src/db/contact-analytics.ts @@ -10,6 +10,7 @@ import { and, desc, eq, inArray, sql } from 'drizzle-orm'; import type { Env } from '../env.js'; +import { chunked } from '../lib/constants.js'; import { db } from './queries.js'; import * as schema from './schema.js'; import { eventCount, pageviewCount } from './stats.js'; @@ -56,49 +57,80 @@ export async function contactActivity( visitorHashes: string[], ): Promise { if (visitorHashes.length === 0) return EMPTY; - const where = and( - eq(schema.events.siteId, siteId), - inArray(schema.events.visitorHash, visitorHashes), - ); const client = db(env); - const [totals, paths] = await Promise.all([ - client - // The SAME expressions /api/stats uses, imported rather than rewritten. A pageview is - // `name IS NULL` in this schema, so a hand-rolled `name = 'pageview'` would silently - // report zero pageviews for every real visitor — and the two surfaces would disagree - // about one person's numbers while agreeing about everyone's. - .select({ - total: sql`count(*)`, - pageviews: pageviewCount, - events: eventCount, - first_seen: sql`min(${schema.events.createdAt})`, - last_seen: sql`max(${schema.events.createdAt})`, - }) - .from(schema.events) - .where(where) - .get(), - client - .select({ - path: schema.events.path, - views: sql`count(*)`, - }) - .from(schema.events) - .where(where) - .groupBy(schema.events.path) - .orderBy(desc(sql`count(*)`)) - .limit(TOP_PATHS), - ]); - if (!totals || totals.total === 0) return EMPTY; + const summed = { ...EMPTY, top_paths: [] as { path: string; views: number }[] }; + const pathViews = new Map(); + // One statement per chunk: an `IN (...)` list is one bound parameter per hash, and D1 refuses a + // query with more than 100 of them. A company rollup unions every linked contact's live salt + // windows, so this list is contacts x windows and routinely passes that on a real account. + for (const batch of chunked(visitorHashes)) { + const where = and( + eq(schema.events.siteId, siteId), + inArray(schema.events.visitorHash, batch), + ); + const [totals, paths] = await Promise.all([ + client + // The SAME expressions /api/stats uses, imported rather than rewritten. A pageview is + // `name IS NULL` in this schema, so a hand-rolled `name = 'pageview'` would silently + // report zero pageviews for every real visitor — and the two surfaces would disagree + // about one person's numbers while agreeing about everyone's. + .select({ + total: sql`count(*)`, + pageviews: pageviewCount, + events: eventCount, + first_seen: sql`min(${schema.events.createdAt})`, + last_seen: sql`max(${schema.events.createdAt})`, + }) + .from(schema.events) + .where(where) + .get(), + // Deliberately NOT `LIMIT TOP_PATHS` per chunk. A path in the overall top ten need not be + // in any single chunk's top ten, so taking a prefix here and merging would return a + // plausible, subtly wrong ranking. Grouping fully and ranking once at the end is exact, + // and a visitor set's distinct paths are bounded by the site's own routes. + client + .select({ + path: schema.events.path, + views: sql`count(*)`, + }) + .from(schema.events) + .where(where) + .groupBy(schema.events.path), + ]); + if (!totals) continue; + summed.total += totals.total ?? 0; + summed.pageviews += totals.pageviews ?? 0; + summed.events += totals.events ?? 0; + // A hash appears in exactly one chunk, so counts add and the extremes are the extremes. + summed.first_seen = minDefined(summed.first_seen, totals.first_seen); + summed.last_seen = maxDefined(summed.last_seen, totals.last_seen); + for (const row of paths) { + pathViews.set(row.path, (pathViews.get(row.path) ?? 0) + row.views); + } + } + if (summed.total === 0) return EMPTY; return { - pageviews: totals.pageviews ?? 0, - events: totals.events ?? 0, - total: totals.total, - first_seen: totals.first_seen ?? null, - last_seen: totals.last_seen ?? null, - top_paths: paths, + ...summed, + top_paths: [...pathViews] + .map(([path, views]) => ({ path, views })) + .sort((a, b) => b.views - a.views || a.path.localeCompare(b.path)) + .slice(0, TOP_PATHS), }; } +/** `Math.min` over values that may be absent, where absent means "no opinion" rather than zero. */ +function minDefined(a: number | null, b: number | null | undefined): number | null { + if (a === null || a === undefined) return b ?? null; + if (b === null || b === undefined) return a; + return Math.min(a, b); +} + +function maxDefined(a: number | null, b: number | null | undefined): number | null { + if (a === null || a === undefined) return b ?? null; + if (b === null || b === undefined) return a; + return Math.max(a, b); +} + /** One event row as it appears in a data-subject export. */ export interface ContactEvent { created_at: number; @@ -119,26 +151,35 @@ export async function contactEvents( visitorHashes: string[], ): Promise { if (visitorHashes.length === 0) return []; - return db(env) - .select({ - created_at: schema.events.createdAt, - hostname: schema.events.hostname, - path: schema.events.path, - referrer: schema.events.referrer, - name: schema.events.name, - country: schema.events.country, - device: schema.events.device, - channel: schema.events.channel, - }) - .from(schema.events) - .where( - and( - eq(schema.events.siteId, siteId), - inArray(schema.events.visitorHash, visitorHashes), - ), - ) - .orderBy(desc(schema.events.createdAt)) - .limit(CONTACT_EXPORT_MAX_EVENTS); + const client = db(env); + const collected: ContactEvent[] = []; + // Chunked for D1's bound-parameter limit, as in `contactActivity`. Each chunk takes the full cap + // rather than a share of it: the newest `CONTACT_EXPORT_MAX_EVENTS` overall could all belong to + // one chunk, so a per-chunk share would drop rows that belong in the export and the caller's + // truncation flag would be computed over the wrong set. + for (const batch of chunked(visitorHashes)) { + const rows = await client + .select({ + created_at: schema.events.createdAt, + hostname: schema.events.hostname, + path: schema.events.path, + referrer: schema.events.referrer, + name: schema.events.name, + country: schema.events.country, + device: schema.events.device, + channel: schema.events.channel, + }) + .from(schema.events) + .where(and(eq(schema.events.siteId, siteId), inArray(schema.events.visitorHash, batch))) + .orderBy(desc(schema.events.createdAt)) + .limit(CONTACT_EXPORT_MAX_EVENTS); + collected.push(...rows); + } + // Re-rank across chunks, then apply the cap once, so the export is the genuinely newest rows + // rather than the newest-per-chunk concatenated. + return collected + .sort((a, b) => b.created_at - a.created_at) + .slice(0, CONTACT_EXPORT_MAX_EVENTS); } /** The consent records authorizing a contact's linkage, for the export. The signed statement is diff --git a/apps/server/src/db/crm.ts b/apps/server/src/db/crm.ts index acd9a91..73a4bb8 100644 --- a/apps/server/src/db/crm.ts +++ b/apps/server/src/db/crm.ts @@ -160,11 +160,35 @@ function normalizeEmail(email: string | null | undefined): string | null { * exactly what a single-level `err.message` check did. */ export function uniqueConstraintText(err: unknown): string | null { + return constraintText(err, /UNIQUE constraint failed/i); +} + +/** + * True when the failure is a foreign-key violation — in this schema, always a contact pointing at a + * company that is no longer there. + * + * `resolveCompany` checks the company exists before the insert, but the check and the write are two + * statements: a `DELETE /companies/:id` committing between them makes the write fail on the + * constraint. Without this the error falls through to a 500, telling the caller the server is broken + * when in fact their request simply lost a race and `unknown_company` is the accurate answer. + */ +export function foreignKeyViolation(err: unknown): boolean { + return constraintText(err, /FOREIGN KEY constraint failed/i) !== null; +} + +/** + * The text of a constraint violation matching `pattern`, or null for any other failure. + * + * Drizzle wraps driver errors (`DrizzleQueryError` carrying the D1 error as `cause`), and how deeply + * it nests them is a detail of the ORM version, not a contract. Walking the `cause` chain means a + * drizzle upgrade that adds or removes a wrapper changes nothing here. + */ +function constraintText(err: unknown, pattern: RegExp): string | null { let current: unknown = err; // Bounded, so a self-referential `cause` cannot spin here. for (let depth = 0; depth < 5; depth++) { if (!(current instanceof Error)) return null; - if (/UNIQUE constraint failed/i.test(current.message)) return current.message; + if (pattern.test(current.message)) return current.message; current = current.cause; } return null; @@ -360,6 +384,45 @@ async function setCompanyFields( if ('company_id' in input) set.company_id = null; } +/** + * Refuse a patch that would leave a contact with no email, no external id and no name. + * + * `ContactCreateSchema` enforces this at creation and states why: such a row "is not a contact, it is + * an empty row that can never be matched, deduped, or erased on request". A PATCH could reach exactly + * that state by blanking the three fields one request later, and the NULLs are distinct in both + * unique indexes so nothing downstream would object. The check has to run against the MERGED row — + * a patch that only clears `email` is fine when a name remains — so it reads the stored row rather + * than judging the patch alone, and only when the patch actually touches an identifier. + */ +async function assertStillIdentifiable( + binding: D1Database, + siteId: string, + id: string, + set: Record, +): Promise { + const IDENTIFIERS = ['email', 'external_user_id', 'name'] as const; + if (!IDENTIFIERS.some((field) => field in set)) return; + const existing = await crmDb(binding) + .select({ + email: crmSchema.contacts.email, + external_user_id: crmSchema.contacts.external_user_id, + name: crmSchema.contacts.name, + }) + .from(crmSchema.contacts) + .where(and(eq(crmSchema.contacts.site_id, siteId), eq(crmSchema.contacts.id, id))) + .get(); + // No row means the update will report 404 on its own; that is a better answer than this one. + if (!existing) return; + const survives = IDENTIFIERS.some((field) => (field in set ? set[field] : existing[field])); + if (!survives) { + throw new ApiError( + 'contact_needs_an_identifier', + 400, + 'a contact must keep at least one of email, external_user_id or name', + ); + } +} + /** Apply a partial update. Only keys actually present in `input` are written, so a PATCH that omits * a field leaves it alone rather than nulling it. Returns the updated row in the resolved read * shape, or undefined if the contact does not exist on this site. */ @@ -375,6 +438,7 @@ export async function updateContact( if ('external_user_id' in input) set.external_user_id = orNull(input.external_user_id); if ('email' in input) set.email = normalizeEmail(input.email); if ('name' in input) set.name = orNull(input.name); + await assertStillIdentifiable(binding, siteId, id, set); if ('phone' in input) set.phone = orNull(input.phone); await setCompanyFields(client, siteId, input, set); if ('title' in input) set.title = orNull(input.title); @@ -563,17 +627,29 @@ export async function deleteCompany( const client = crmDb(binding); const company = await getCompany(binding, siteId, id); if (!company) return undefined; - const [unlinked, deleted] = await client.batch([ + const atCompany = and( + eq(crmSchema.contacts.site_id, siteId), + eq(crmSchema.contacts.company_id, company.id), + ); + const [counted, , deleted] = await client.batch([ + // Counted inside the transaction rather than by materialising the rows: `.returning()` on the + // update would pull one row per contact across the wire to produce a single integer, which for + // a large account is tens of thousands of rows read to count them. + client + .select({ n: sql`count(*)` }) + .from(crmSchema.contacts) + .where(atCompany), client .update(crmSchema.contacts) - .set({ company: company.name, company_id: null }) - .where( - and( - eq(crmSchema.contacts.site_id, siteId), - eq(crmSchema.contacts.company_id, company.id), - ), - ) - .returning({ id: crmSchema.contacts.id }), + // The name is read by a correlated subquery, INSIDE the transaction, not captured from the + // `getCompany` above. A rename committing between that read and this write would otherwise + // stamp every contact with the superseded name — and with the company row then deleted, + // nothing would remain to correct it against. + .set({ + company: sql`(SELECT ${crmSchema.companies.name} FROM ${crmSchema.companies} WHERE ${crmSchema.companies.id} = ${company.id})`, + company_id: null, + }) + .where(atCompany), client .delete(crmSchema.companies) .where(and(eq(crmSchema.companies.site_id, siteId), eq(crmSchema.companies.id, id))) @@ -581,5 +657,5 @@ export async function deleteCompany( ]); // Lost a race with a concurrent delete: the batch changed nothing, and 404 is the honest answer. if (deleted.length === 0) return undefined; - return { company, contacts_unlinked: unlinked.length }; + return { company, contacts_unlinked: counted[0]?.n ?? 0 }; } diff --git a/apps/server/src/lib/consent.ts b/apps/server/src/lib/consent.ts index f227e12..acdadeb 100644 --- a/apps/server/src/lib/consent.ts +++ b/apps/server/src/lib/consent.ts @@ -27,6 +27,7 @@ import { verifyStatement, } from '@facet/trust'; import type { Env } from '../env.js'; +import { chunked } from './constants.js'; import { deploymentDid, getSigningKey } from './signing.js'; export const CONSENT_STATEMENT_TYPE = 'facet-consent/1'; @@ -225,7 +226,11 @@ export async function findLinkedVisitorHashes( * while pointing at another person's hash still has to survive the signature check, which is what * stops the grouping key from becoming an authorization key. * - * The caller must bound `externalUserIds`; this issues one query with one bind per id. + * The uid list is CHUNKED across statements rather than bound in one. D1 rejects any query carrying + * more than 100 bound parameters, and this one spends two of them on `site_id` and `now` — so a + * company of 99 linkable contacts asked for 101 and the statement was refused outright. That is a + * hard failure, not a slow one, and it lands on exactly the largest account rather than on the small + * ones a test would reach for. */ export async function findLinkedVisitorHashesForMany( env: Env, @@ -237,31 +242,34 @@ export async function findLinkedVisitorHashesForMany( const loading = getSigningKey(env); if (!loading) return byUid; const key = await loading; - const placeholders = lookup.externalUserIds.map(() => '?').join(', '); - const { results } = await env.DB.prepare( - `SELECT external_user_id, statement FROM consent_records WHERE site_id = ? AND external_user_id IN (${placeholders}) AND tier = 'identified' AND revoked_at IS NULL AND (expires_at IS NULL OR expires_at > ?)`, - ) - .bind(lookup.siteId, ...lookup.externalUserIds, lookup.now) - .all<{ external_user_id: string; statement: string }>(); const iss = deploymentDid(url); const seen = new Map>(); - for (const row of results ?? []) { - let stmt: SignedStatement; - try { - stmt = JSON.parse(row.statement) as SignedStatement; - } catch { - continue; + for (const batch of chunked(lookup.externalUserIds)) { + const placeholders = batch.map(() => '?').join(', '); + const { results } = await env.DB.prepare( + `SELECT external_user_id, statement FROM consent_records WHERE site_id = ? AND external_user_id IN (${placeholders}) AND tier = 'identified' AND revoked_at IS NULL AND (expires_at IS NULL OR expires_at > ?)`, + ) + .bind(lookup.siteId, ...batch, lookup.now) + .all<{ external_user_id: string; statement: string }>(); + for (const row of results ?? []) { + let stmt: SignedStatement; + try { + stmt = JSON.parse(row.statement) as SignedStatement; + } catch { + continue; + } + if (!(await verifyPinnedToDeployment(stmt, iss, key.kid))) continue; + const p = stmt.payload; + // The claims, not the columns: this grant must be for this site, at the identified tier, + // and must actually have been made against an external user id rather than an ip/ua + // pseudonym. + if (p.site_id !== lookup.siteId) continue; + if (p.tier !== 'identified') continue; + if (!p.external_user_id_present) continue; + const hashes = seen.get(row.external_user_id) ?? new Set(); + hashes.add(p.visitor_hash); + seen.set(row.external_user_id, hashes); } - if (!(await verifyPinnedToDeployment(stmt, iss, key.kid))) continue; - const p = stmt.payload; - // The claims, not the columns: this grant must be for this site, at the identified tier, and - // must actually have been made against an external user id rather than an ip/ua pseudonym. - if (p.site_id !== lookup.siteId) continue; - if (p.tier !== 'identified') continue; - if (!p.external_user_id_present) continue; - const hashes = seen.get(row.external_user_id) ?? new Set(); - hashes.add(p.visitor_hash); - seen.set(row.external_user_id, hashes); } for (const [uid, hashes] of seen) { byUid.set(uid, [...hashes]); diff --git a/apps/server/src/lib/constants.ts b/apps/server/src/lib/constants.ts index 94d30fa..79d9490 100644 --- a/apps/server/src/lib/constants.ts +++ b/apps/server/src/lib/constants.ts @@ -47,3 +47,24 @@ export const EXPORT_MAX_ROWS = 1000 as const; /** Trailing window for the realtime "active visitors" metric, in milliseconds (5 minutes). */ export const REALTIME_WINDOW_MS = 300_000 as const; + +/** + * How many values one `IN (...)` list may carry. + * + * D1 rejects any statement with more than 100 bound parameters ("too many SQL variables"), and every + * such query spends some of that budget on its other predicates — a site id, a timestamp, a limit. So + * the list gets a margin rather than the whole allowance, and anything longer is chunked across + * statements. This is not a tuning knob: exceed it and the query does not run slowly, it fails. + */ +export const D1_MAX_IN_PARAMS = 90 as const; + +/** Split `values` into runs of at most `size`, for queries whose `IN (...)` list would otherwise + * exceed D1's bound-parameter limit. An empty input yields no chunks, so a caller can iterate the + * result without a special case for "nothing to look up". */ +export function chunked(values: readonly T[], size: number = D1_MAX_IN_PARAMS): T[][] { + const out: T[][] = []; + for (let i = 0; i < values.length; i += size) { + out.push(values.slice(i, i + size)); + } + return out; +} diff --git a/apps/server/src/routes/crm.ts b/apps/server/src/routes/crm.ts index 8a881fb..9abfb03 100644 --- a/apps/server/src/routes/crm.ts +++ b/apps/server/src/routes/crm.ts @@ -54,6 +54,7 @@ import { companyContactLinkage, deleteCompany, deleteContact, + foreignKeyViolation, getCompany, getContact, insertCompany, @@ -108,6 +109,36 @@ async function assertOwnerExists(env: Env, ownerUserId: string | undefined): Pro } } +/** + * Map a failed contact write onto the status it deserves, or rethrow. + * + * The unique indexes on `(site_id, email)` and `(site_id, external_user_id)` are the dedupe, and + * naming the field that collided is safe: the caller holds a role on this site and submitted the + * value themselves. A foreign-key failure means the company was deleted between this request + * resolving it and writing the row — the caller lost a race, which is a 400 about their `company_id` + * and not a 500 about the server. + */ +function contactWriteError(err: unknown): never { + const conflict = uniqueConstraintText(err); + if (conflict) { + throw new ApiError( + 'contact_exists', + 409, + /external_user_id/i.test(conflict) + ? 'a contact with this external_user_id already exists' + : 'a contact with this email already exists', + ); + } + if (foreignKeyViolation(err)) { + throw new ApiError( + 'unknown_company', + 400, + 'company_id does not match a company on this site', + ); + } + throw err; +} + /** A contact's currently-authorized visitor hashes, or [] when nothing authorizes a link. */ function linkedHashes( env: Env, @@ -152,22 +183,7 @@ crmRoutes.post( c.get('siteId'), body, Date.now(), - ).catch((err: unknown) => { - // The (site_id, email) / (site_id, external_user_id) unique indexes are the dedupe. A - // collision is a client mistake, not a server fault, and saying which field collided is - // safe: the caller already holds a role on this site and submitted the value itself. - const conflict = uniqueConstraintText(err); - if (conflict) { - throw new ApiError( - 'contact_exists', - 409, - /external_user_id/i.test(conflict) - ? 'a contact with this external_user_id already exists' - : 'a contact with this email already exists', - ); - } - throw err; - }); + ).catch(contactWriteError); return c.json({ contact }, 201); }, ); @@ -190,16 +206,7 @@ crmRoutes.patch( c.req.param('id') ?? '', body, Date.now(), - ).catch((err: unknown) => { - if (uniqueConstraintText(err)) { - throw new ApiError( - 'contact_exists', - 409, - 'another contact already holds that value', - ); - } - throw err; - }); + ).catch(contactWriteError); if (!contact) { throw new ApiError('not_found', 404); } @@ -220,16 +227,24 @@ crmRoutes.patch( */ crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), async (c) => { const siteId = c.get('siteId'); - const contact = await deleteContact(requireCrmDb(c.env), siteId, c.req.param('id')); - if (!contact) { - throw new ApiError('not_found', 404); - } + const contact = await loadContact(c.env, siteId, c.req.param('id') ?? ''); + // The two writes land in DIFFERENT databases and D1 has no transaction spanning them, so one of + // them can be left undone. The order decides which. Erasing the consent records FIRST means a + // failure leaves the contact row still present and still naming the uid — an erasure that can + // simply be retried. Deleting the contact first would mean a failure destroys the only record of + // which uid to erase, stranding rows that hold that person's raw identifier: exactly the data the + // request was about, now unreachable by any retry. const consentErased = contact.external_user_id ? await eraseConsentByExternalUserId(c.env, { siteId, externalUserId: contact.external_user_id, }) : 0; + const deleted = await deleteContact(requireCrmDb(c.env), siteId, contact.id); + if (!deleted) { + // Lost a race with a concurrent delete, which already erased the same consent rows. + throw new ApiError('not_found', 404); + } return c.json({ deleted: true, consent_records_erased: consentErased }); }); @@ -461,7 +476,16 @@ crmRoutes.get('/companies/:id/analytics', requireTeamRole('analyst'), async (c) if (hashes.length === 0) { // Same honesty as the contact route: zeroes would read as "this account did nothing", which is // a different claim from "nobody here has authorized a link". - return c.json({ ...counts, linked: false, reason: 'no_linked_contacts' }); + // + // And when the fan-out was capped, even THAT is more than can be claimed. The contacts + // resolved are the newest `contacts_limit`; older ones outside the window may well be linked, + // so the honest reason names what was actually examined rather than asserting a fact about + // contacts nobody looked at. + return c.json({ + ...counts, + linked: false, + reason: linkage.truncated ? 'none_linked_within_cap' : 'no_linked_contacts', + }); } return c.json({ ...counts, diff --git a/apps/server/test/contact-analytics-chunking.test.ts b/apps/server/test/contact-analytics-chunking.test.ts new file mode 100644 index 0000000..9d87980 --- /dev/null +++ b/apps/server/test/contact-analytics-chunking.test.ts @@ -0,0 +1,92 @@ +// The chunked fan-out in db/contact-analytics.ts. D1 refuses a query with more than 100 bound +// parameters, so a hash list longer than the chunk size is split across statements and merged here +// rather than in SQL. Merging is where an aggregate quietly stops being exact, so these tests pin the +// two ways it could: totals that fail to add up, and a ranking assembled from per-chunk prefixes. + +import { env } from 'cloudflare:test'; +import { beforeEach, describe, expect, it } from 'vitest'; +import { contactActivity, contactEvents } from '../src/db/contact-analytics.js'; +import { D1_MAX_IN_PARAMS } from '../src/lib/constants.js'; + +const SITE = '66666666-6666-4666-8666-666666666666'; + +/** Distinct 64-hex hashes, so each lands in exactly one chunk. */ +function hash(i: number): string { + return i.toString(16).padStart(64, '0'); +} + +async function seed(rows: { hash: string; path: string; at: number }[]): Promise { + const insert = env.DB.prepare( + `INSERT INTO events (id, site_id, name, hostname, path, referrer, visitor_hash, created_at) + VALUES (?, ?, NULL, 'shop.example.com', ?, '', ?, ?)`, + ); + for (let i = 0; i < rows.length; i += 400) { + await env.DB.batch( + rows + .slice(i, i + 400) + .map((r) => insert.bind(crypto.randomUUID(), SITE, r.path, r.hash, r.at)), + ); + } +} + +beforeEach(async () => { + await env.DB.prepare( + 'INSERT OR IGNORE INTO sites (id, name, domain, created_at) VALUES (?, ?, ?, ?)', + ) + .bind(SITE, 'Test', 'shop.example.com', Date.now()) + .run(); +}); + +describe('a hash list longer than one query can bind', () => { + it('adds the totals up across chunks instead of reporting one of them', async () => { + const n = D1_MAX_IN_PARAMS + 5; + const hashes = Array.from({ length: n }, (_, i) => hash(i + 1)); + // One pageview per hash, each at a distinct time so the extremes are unambiguous. + await seed(hashes.map((h, i) => ({ hash: h, path: '/pricing', at: 1_000 + i }))); + + const activity = await contactActivity(env, SITE, hashes); + expect(activity.total).toBe(n); + expect(activity.pageviews).toBe(n); + // first/last must span every chunk, not just the last one processed. + expect(activity.first_seen).toBe(1_000); + expect(activity.last_seen).toBe(1_000 + n - 1); + }); + + it('ranks paths over the whole set, not over each chunk separately', async () => { + // The discriminating case for taking a per-chunk prefix. `/sleeper` is rank 13 within BOTH + // chunks, so any implementation that applied `LIMIT 10` per chunk and merged the survivors + // would drop it entirely — yet it is the single most-viewed path overall. + const chunkA = Array.from({ length: D1_MAX_IN_PARAMS }, (_, i) => hash(i + 1)); + const chunkB = Array.from({ length: 5 }, (_, i) => hash(D1_MAX_IN_PARAMS + i + 1)); + const rows: { hash: string; path: string; at: number }[] = []; + let t = 0; + const push = (h: string, path: string, times: number) => { + for (let i = 0; i < times; i++) rows.push({ hash: h, path, at: 5_000 + t++ }); + }; + for (let p = 0; p < 12; p++) push(chunkA[p] as string, `/a${p}`, 12); + for (let p = 0; p < 12; p++) push(chunkB[p % chunkB.length] as string, `/b${p}`, 12); + // Ten views in each chunk: below every per-chunk leader, above all of them combined. + push(chunkA[50] as string, '/sleeper', 10); + push(chunkB[4] as string, '/sleeper', 10); + await seed(rows); + + const activity = await contactActivity(env, SITE, [...chunkA, ...chunkB]); + expect(activity.top_paths[0]).toEqual({ path: '/sleeper', views: 20 }); + expect(activity.total).toBe(12 * 12 + 12 * 12 + 20); + }); + + it('returns the genuinely newest events, not the newest within each chunk', async () => { + // Same trap for the export: a per-chunk cap concatenated would return the later chunk's older + // rows ahead of the earlier chunk's newer ones. + const chunkA = Array.from({ length: D1_MAX_IN_PARAMS }, (_, i) => hash(i + 1)); + const chunkB = [hash(D1_MAX_IN_PARAMS + 1)]; + await seed([ + ...chunkA.map((h, i) => ({ hash: h, path: '/old', at: 1_000 + i })), + { hash: chunkB[0] as string, path: '/newest', at: 9_999_999 }, + ]); + + const events = await contactEvents(env, SITE, [...chunkA, ...chunkB]); + expect(events[0]?.path).toBe('/newest'); + expect(events.length).toBe(chunkA.length + 1); + }); +}); diff --git a/apps/server/test/crm.test.ts b/apps/server/test/crm.test.ts index 801a353..31af812 100644 --- a/apps/server/test/crm.test.ts +++ b/apps/server/test/crm.test.ts @@ -11,8 +11,11 @@ import { env } from 'cloudflare:test'; import { generateSigningJwk } from '@facet/trust'; import { beforeEach, describe, expect, it } from 'vitest'; import { createApp } from '../src/app.js'; -import { CONTACT_EXPORT_MAX_EVENTS } from '../src/db/contact-analytics.js'; -import { companyContactLinkage } from '../src/db/crm.js'; +import { + COMPANY_ROLLUP_MAX_CONTACTS, + CONTACT_EXPORT_MAX_EVENTS, +} from '../src/db/contact-analytics.js'; +import { companyContactLinkage, foreignKeyViolation } from '../src/db/crm.js'; import { SESSION_COOKIE, signSession, @@ -1157,3 +1160,151 @@ describe('the company rollup sums consent, it does not bypass it', () => { expect(uncapped.truncated).toBe(false); }); }); + +describe('the rollup fan-out stays inside D1 limits', () => { + it('answers for a company larger than one query can bind', async () => { + // D1 allows 100 bound parameters per query. The consent lookup binds site_id and `now` on top + // of one per contact, so a company with 99 linkable contacts asks for 101 and the statement is + // rejected outright — a hard 500 on exactly the large account that most wants a rollup, while + // every small company a test would naturally use keeps working. + // A signing key is required, or the consent lookup returns before it ever builds the query and + // the fan-out under test never happens. + const e = await withSigningKey(env); + const cookie = await operator(e, 'admin@example.com', 'admin'); + const company = await createCompany(e, cookie, { name: 'Acme' }); + const now = Date.now(); + const insert = e.CRM_DB.prepare( + `INSERT INTO contacts (id, site_id, external_user_id, name, company_id, status, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, 'lead', ?, ?)`, + ); + const n = 99; + await e.CRM_DB.batch( + Array.from({ length: n }, (_, i) => + insert.bind( + crypto.randomUUID(), + SITE, + `uid-${i}`, + `Person ${i}`, + company.id, + now - i, + now, + ), + ), + ); + const res = await crm(e, `/companies/${company.id}/analytics`, {}, cookie); + expect(res.status).toBe(200); + const body = (await res.json()) as { contacts_total: number; contacts_linked: number }; + expect(body.contacts_total).toBe(n); + // Nobody consented, so the honest answer is zero linked — but it has to be an ANSWER. + expect(body.contacts_linked).toBe(0); + }); +}); + +describe('a patch cannot strip a contact of every identifier', () => { + it('refuses to blank email, external id and name all at once', async () => { + // `ContactCreateSchema` rejects a row with none of the three because such a row "can never be + // matched, deduped, or erased on request". A PATCH one request later could reach exactly that + // state, and NULLs are distinct in both unique indexes so nothing downstream would object. + const cookie = await operator(env, 'admin@example.com', 'admin'); + const contact = await createContact(env, cookie, { name: 'Ada', email: 'ada@example.com' }); + const res = await crm( + env, + `/contacts/${contact.id}`, + { + method: 'PATCH', + body: JSON.stringify({ name: '', email: '', external_user_id: '' }), + }, + cookie, + ); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ error: 'contact_needs_an_identifier' }); + // And the row is untouched, not half-blanked. + const after = await crm(env, `/contacts/${contact.id}`, {}, cookie); + expect((await after.json()) as { contact: { name: string } }).toMatchObject({ + contact: { name: 'Ada', email: 'ada@example.com' }, + }); + }); + + it('still lets one identifier be cleared while another survives', async () => { + // The check is against the MERGED row, not the patch: clearing the email of a contact who + // still has a name is ordinary editing and must not be blocked. + const cookie = await operator(env, 'admin@example.com', 'admin'); + const contact = await createContact(env, cookie, { name: 'Ada', email: 'ada@example.com' }); + const res = await crm( + env, + `/contacts/${contact.id}`, + { method: 'PATCH', body: JSON.stringify({ email: '' }) }, + cookie, + ); + expect(res.status).toBe(200); + expect((await res.json()) as { contact: { email: null } }).toMatchObject({ + contact: { email: null, name: 'Ada' }, + }); + }); +}); + +describe('the foreign key is a real constraint, not a comment', () => { + it('refuses a contact pointing at a company that does not exist', async () => { + // `resolveCompany` is the site-scoped check and this is the backstop underneath it. If D1 did + // not enforce the constraint, the schema's claim that a bad link "cannot" be written would be + // decoration, and the race between resolving a company and inserting the row would corrupt + // data silently instead of failing loudly. + let message = ''; + try { + await env.CRM_DB.prepare( + `INSERT INTO contacts (id, site_id, name, company_id, status, created_at, updated_at) + VALUES (?, ?, 'Ada', 'does-not-exist', 'lead', 1, 1)`, + ) + .bind(crypto.randomUUID(), SITE) + .run(); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + expect(message).toMatch(/FOREIGN KEY constraint failed/i); + // And the classifier the route relies on recognises the real error shape, not a guessed one. + expect(foreignKeyViolation(new Error(message))).toBe(true); + expect(foreignKeyViolation(new Error('UNIQUE constraint failed: contacts.email'))).toBe( + false, + ); + }); +}); + +describe('a capped rollup does not claim what it did not look at', () => { + it('names the cap rather than asserting nobody is linked', async () => { + // With the fan-out truncated, "no linked contacts" is a statement about contacts that were + // never examined. The older ones outside the window may well be linked. + const e = await withSigningKey(env); + const cookie = await operator(e, 'admin@example.com', 'admin'); + const company = await createCompany(e, cookie, { name: 'Acme' }); + const now = Date.now(); + const insert = e.CRM_DB.prepare( + `INSERT INTO contacts (id, site_id, external_user_id, name, company_id, status, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, 'lead', ?, ?)`, + ); + const n = COMPANY_ROLLUP_MAX_CONTACTS + 1; + for (let i = 0; i < n; i += 200) { + await e.CRM_DB.batch( + Array.from({ length: Math.min(200, n - i) }, (_, j) => + insert.bind( + crypto.randomUUID(), + SITE, + `uid-${i + j}`, + `Person ${i + j}`, + company.id, + now - (i + j), + now, + ), + ), + ); + } + const res = await crm(e, `/companies/${company.id}/analytics`, {}, cookie); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ + linked: false, + reason: 'none_linked_within_cap', + contacts_total: n, + contacts_considered: COMPANY_ROLLUP_MAX_CONTACTS, + contacts_truncated: true, + }); + }); +}); From ef48cee650b067a1eb201a9c4963ca2add619035 Mon Sep 17 00:00:00 2001 From: David Condrey Date: Tue, 4 Aug 2026 22:17:18 -0700 Subject: [PATCH 2/5] security(server): bind a consent statement to the contact it was issued for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CRM link grouped consent records by the `external_user_id` COLUMN and authorized them by the SIGNED claims, and nothing tied the two together. The claims name a site, a tier and a visitor hash but never the uid — `external_user_id_present` is a bit, not a value — so a genuine, deployment-signed grant for one person satisfied every check when filed under another contact's id. Signature verification cannot catch it because nothing is forged. The comment on that function asserted the property held; it did not. Concretely: the per-contact export returns consent statements verbatim, by design, as cryptographic evidence of what was consented to. Copy one into a consent_records row naming a different uid and that contact's analytics page shows the first person's browsing. The binding is recoverable rather than absent. The identified pre-image is `uid:|salt|siteId` and the claims carry the salt window, so the hash is recomputed from the ROW's uid and required to equal the SIGNED one. A statement now authorizes exactly the person it was issued for. A missing salt fails closed. That cannot happen for a live grant — retention drops a consent record at `granted_at < cutoff` and its salt only at `window_end < cutoff`, and a window always ends after the grant inside it, so the salt outlives the record — but "the salt is gone" and "this hash belongs to someone else" are indistinguishable here, and the safe reading of an unverifiable link is that there is no link. Salts are read WITHOUT `getScopedSalt`, which mints one when absent: a verification that conjured the salt it was about to check against would resurrect an identifier retention had destroyed. The existing forgery tests both used an invalid signature, so they proved only that the signature check runs. The new test replays an untouched, validly-signed statement and also asserts the rightful owner still resolves, because a check that severed real links would pass a one-sided test. Also, autonomously: extracted `saltScope`, which was an inline template literal in the consent grant and revoke paths and is now named once — three copies of the string that must match is three chances for one to drift and silently stop resolving. --- apps/server/src/lib/consent.ts | 60 +++++++++++++++++++++-- apps/server/src/lib/identity.ts | 20 ++++++++ apps/server/src/routes/consent.ts | 5 +- apps/server/test/crm.test.ts | 79 +++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 5 deletions(-) diff --git a/apps/server/src/lib/consent.ts b/apps/server/src/lib/consent.ts index acdadeb..13f8779 100644 --- a/apps/server/src/lib/consent.ts +++ b/apps/server/src/lib/consent.ts @@ -28,6 +28,7 @@ import { } from '@facet/trust'; import type { Env } from '../env.js'; import { chunked } from './constants.js'; +import { deriveVisitorHash, readScopedSalt, saltScope } from './identity.js'; import { deploymentDid, getSigningKey } from './signing.js'; export const CONSENT_STATEMENT_TYPE = 'facet-consent/1'; @@ -182,6 +183,55 @@ export async function storeConsentRecord(env: Env, row: ConsentRecordRow): Promi .run(); } +/** + * Does this statement's hash actually belong to the uid the ROW is filed under? + * + * This is the check that makes the column safe to group by. The claims name a site, a tier and a + * hash, but they never name the uid — `external_user_id_present` is a bit, not a value — so a + * GENUINE, deployment-signed grant for Ada satisfies every other check when copied into a row whose + * `external_user_id` column says `bob-uid`. Signature verification cannot catch that: nothing is + * forged. Bob's contact page would simply show Ada's browsing. + * + * The binding is recoverable because the identified pre-image is `uid:|salt|siteId`, and the + * claims carry the window the salt belongs to. Recomputing the hash from the ROW's uid and requiring + * it to equal the SIGNED one closes the replay: a statement now authorizes exactly the person it was + * issued for. + * + * A missing salt fails closed. It cannot happen for a live grant — retention drops a consent record + * at `granted_at < cutoff` and its salt only at `window_end < cutoff`, and a window always ends after + * the grant inside it, so the salt outlives the record — but "the salt is gone" and "this hash is + * someone else's" are indistinguishable from here, and the safe reading of an unverifiable link is + * that there is no link. + */ +async function hashBelongsToUid( + env: Env, + siteId: string, + externalUserId: string, + claims: ConsentClaims, + saltCache: Map, +): Promise { + const scope = saltScope(siteId, claims.salt_window, claims.window_key); + let salt = saltCache.get(scope); + if (salt === undefined) { + salt = await readScopedSalt(env, scope); + saltCache.set(scope, salt); + } + if (!salt) return false; + // An empty uid would fall through `buildPreimage`'s identified branch to the ip/ua pre-image and + // compare an unrelated hash, so it is rejected here rather than answered by accident. + if (!externalUserId) return false; + // `ip`/`ua` are structurally required by `DeriveInputs` and unused on the identified branch, whose + // pre-image is `uid:|salt|siteId`. Blanks state that rather than smuggling in values this + // check has no business knowing. + const expected = await deriveVisitorHash( + 'identified', + { ip: '', ua: '', uid: externalUserId }, + salt, + siteId, + ); + return expected === claims.visitor_hash; +} + /** * The ONE bridge from a CRM contact to analytics. Resolve a site's opaque `external_user_id` to the * visitor hashes it is currently allowed to be linked to — one per salt window with a live grant. @@ -222,9 +272,9 @@ export async function findLinkedVisitorHashes( * from having no consent record at all. * * The `external_user_id` COLUMN groups the results and the SIGNED claims authorize them: the column - * says which contact asked, the statement says what they may see. A row whose column names one uid - * while pointing at another person's hash still has to survive the signature check, which is what - * stops the grouping key from becoming an authorization key. + * says which contact asked, the statement says what they may see. Those two are tied together by + * `hashBelongsToUid` and not by the signature — a genuine statement carries no uid to check, so + * verification alone would happily let one person's grant be filed under another's id. * * The uid list is CHUNKED across statements rather than bound in one. D1 rejects any query carrying * more than 100 bound parameters, and this one spends two of them on `site_id` and `now` — so a @@ -244,6 +294,8 @@ export async function findLinkedVisitorHashesForMany( const key = await loading; const iss = deploymentDid(url); const seen = new Map>(); + // Rows routinely share a salt window, so the salt is fetched once per window rather than per row. + const saltCache = new Map(); for (const batch of chunked(lookup.externalUserIds)) { const placeholders = batch.map(() => '?').join(', '); const { results } = await env.DB.prepare( @@ -266,6 +318,8 @@ export async function findLinkedVisitorHashesForMany( if (p.site_id !== lookup.siteId) continue; if (p.tier !== 'identified') continue; if (!p.external_user_id_present) continue; + if (!(await hashBelongsToUid(env, lookup.siteId, row.external_user_id, p, saltCache))) + continue; const hashes = seen.get(row.external_user_id) ?? new Set(); hashes.add(p.visitor_hash); seen.set(row.external_user_id, hashes); diff --git a/apps/server/src/lib/identity.ts b/apps/server/src/lib/identity.ts index b114b33..e2c8be7 100644 --- a/apps/server/src/lib/identity.ts +++ b/apps/server/src/lib/identity.ts @@ -144,6 +144,26 @@ export async function resolvePolicy(env: Env, siteId: string): Promise { + const row = await env.DB.prepare('SELECT salt FROM identity_salts WHERE scope = ?') + .bind(scope) + .first<{ salt: string }>(); + return row?.salt ?? null; +} + export async function getScopedSalt( env: Env, scope: string, diff --git a/apps/server/src/routes/consent.ts b/apps/server/src/routes/consent.ts index 6d45252..5f9ec40 100644 --- a/apps/server/src/routes/consent.ts +++ b/apps/server/src/routes/consent.ts @@ -22,6 +22,7 @@ import { validationErrorHook } from '../lib/http.js'; import { deriveVisitorHash, getScopedSalt, + saltScope, resolvePolicy, windowEndMs, windowKey, @@ -57,7 +58,7 @@ consentRoutes.post( } const now = Date.now(); const wk = windowKey(policy.window, now); - const scope = `${siteId}:${policy.window}:${wk}`; + const scope = saltScope(siteId, policy.window, wk); const salt = await getScopedSalt( c.env, scope, @@ -126,7 +127,7 @@ consentRoutes.delete( return c.json({ revoked: 0 }); } const wk = windowKey(policy.window, now); - const scope = `${siteId}:${policy.window}:${wk}`; + const scope = saltScope(siteId, policy.window, wk); const salt = await getScopedSalt( c.env, scope, diff --git a/apps/server/test/crm.test.ts b/apps/server/test/crm.test.ts index 31af812..1d72b32 100644 --- a/apps/server/test/crm.test.ts +++ b/apps/server/test/crm.test.ts @@ -1308,3 +1308,82 @@ describe('a capped rollup does not claim what it did not look at', () => { }); }); }); + +describe('a genuine consent statement authorizes only the person it was issued for', () => { + it('refuses a real, deployment-signed grant filed under another contact id', async () => { + // The gap signature verification cannot see. The claims name a site, a tier and a hash, but + // never the uid — `external_user_id_present` is a bit, not a value — so Ada's UNMODIFIED, + // validly-signed statement satisfies every signature and claim check when copied into a row + // whose `external_user_id` column says someone else. Nothing is forged, so the crypto has no + // objection; only recomputing the hash from the row's uid can tell the two apart. + // + // The statement is obtained the way an operator really could: the contact export returns it + // verbatim, deliberately, as cryptographic evidence of what was consented to. + const e = await withSigningKey(env); + const cookie = await operator(e, 'admin@example.com', 'admin'); + await e.DB.prepare( + 'INSERT OR REPLACE INTO site_config (site_id, tier, salt_window, updated_at) VALUES (?, ?, ?, ?)', + ) + .bind(SITE, 'identified', 'day', Date.now()) + .run(); + const { key } = await issueKey(e, SITE, 'server', Date.now()); + const grant = await app.request( + '/api/consent', + { + method: 'POST', + headers: { Authorization: `Bearer ${key}`, 'content-type': 'application/json' }, + body: JSON.stringify({ + tier: 'identified', + salt_window: 'day', + user_id: 'ada-uid', + ip: '203.0.113.9', + user_agent: 'test-agent', + }), + }, + e, + ); + expect(grant.status).toBe(201); + const ada = await e.DB.prepare( + 'SELECT visitor_hash, statement FROM consent_records WHERE site_id = ? AND external_user_id = ?', + ) + .bind(SITE, 'ada-uid') + .first<{ visitor_hash: string; statement: string }>(); + + // Ada really browsed; these are her events, reachable if the gate fails. + const insert = e.DB.prepare( + `INSERT INTO events (id, site_id, name, hostname, path, referrer, visitor_hash, created_at) + VALUES (?, ?, NULL, 'shop.example.com', '/pricing', '', ?, ?)`, + ); + await e.DB.batch([ + insert.bind(crypto.randomUUID(), SITE, ada?.visitor_hash as string, Date.now() - 10), + insert.bind(crypto.randomUUID(), SITE, ada?.visitor_hash as string, Date.now() - 5), + ]); + + // Ada's statement, byte for byte, filed under Mallory's id. + await e.DB.prepare( + `INSERT INTO consent_records + (id, site_id, visitor_hash, tier, external_user_id, salt_window, window_key, gpc_at_grant, granted_at, expires_at, revoked_at, statement) + SELECT ?, site_id, visitor_hash, tier, ?, salt_window, window_key, gpc_at_grant, granted_at, expires_at, revoked_at, statement + FROM consent_records WHERE site_id = ? AND external_user_id = ?`, + ) + .bind(crypto.randomUUID(), 'mallory-uid', SITE, 'ada-uid') + .run(); + + const mallory = await createContact(e, cookie, { + name: 'Mallory', + external_user_id: 'mallory-uid', + }); + const res = await crm(e, `/contacts/${mallory.id}/analytics`, {}, cookie); + expect(await res.json()).toMatchObject({ linked: false, reason: 'no_active_consent' }); + + // And the rightful owner is unaffected — the check binds the grant, it does not break it. + const adaContact = await createContact(e, cookie, { + name: 'Ada', + external_user_id: 'ada-uid', + }); + const hers = await crm(e, `/contacts/${adaContact.id}/analytics`, {}, cookie); + const body = (await hers.json()) as { linked: boolean; activity: { total: number } }; + expect(body.linked).toBe(true); + expect(body.activity.total).toBe(2); + }); +}); From 9a149da2d7d1d0805a586a3a9d1c73b3902c4825 Mon Sep 17 00:00:00 2001 From: David Condrey Date: Tue, 4 Aug 2026 22:21:18 -0700 Subject: [PATCH 3/5] security(server): bound the CRM routes, not just authenticate them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /api/crm is the only route group that returns names, emails and phone numbers, and it was the one with no limits on it at all. Every global middleware in app.ts is path-scoped to /api/collect or /api/experiments, so none reached it: no rate limit, no body limit, and an `offset` with a floor and no ceiling. Concretely, one stolen analyst session could pull the entire contacts table. `total` comes back on the first response, so the number of pages is known immediately; each page is 100 complete records including up to 4000 characters of notes per person; and nothing bounded the request rate. Ten thousand contacts is a hundred requests that can be issued in parallel. The rate limit is keyed by the OPERATOR, not the site. Everything else in this codebase keys per site because the risk there is one tenant drowning another. The risk here is a single compromised session, and a per-site key would let it hide inside its team's legitimate traffic while punishing colleagues for it. It is applied after the role guard at every route, matching /api/event's deliberate ordering, so an unauthenticated request is refused before it can consume anyone's bucket — asserted directly, since an anonymous caller must see 401 rather than 429. The body limit is router-wide. `offset` now has a maximum: SQLite walks every skipped row, so an unbounded one is both a full table scan and the natural shape of a page-by-page bulk read. The limiter is invisible in tests because RATE_LIMITER is deliberately unbound there and the middleware no-ops, which would make "is it actually attached" untestable. The tests inject a stub that denies everything, so a route missing the guard fails rather than passing quietly. --- apps/server/src/lib/constants.ts | 8 +++++ apps/server/src/routes/crm.ts | 51 +++++++++++++++++++++++++---- apps/server/test/crm.test.ts | 55 ++++++++++++++++++++++++++++++++ packages/shared/src/crm.ts | 14 +++++++- 4 files changed, 120 insertions(+), 8 deletions(-) diff --git a/apps/server/src/lib/constants.ts b/apps/server/src/lib/constants.ts index 79d9490..cb27827 100644 --- a/apps/server/src/lib/constants.ts +++ b/apps/server/src/lib/constants.ts @@ -48,6 +48,14 @@ export const EXPORT_MAX_ROWS = 1000 as const; /** Trailing window for the realtime "active visitors" metric, in milliseconds (5 minutes). */ export const REALTIME_WINDOW_MS = 300_000 as const; +/** + * Largest body a CRM write may carry. The global `bodyLimit` is path-scoped to `/api/collect`, so + * without this the one route group that stores personal data was the one with no ceiling at all. + * Generous against the field bounds — a contact's `notes` alone may be 4000 characters — because this + * is a backstop against an unbounded upload, not a second copy of the wire schema. + */ +export const CRM_MAX_BODY_BYTES = 16_384 as const; + /** * How many values one `IN (...)` list may carry. * diff --git a/apps/server/src/routes/crm.ts b/apps/server/src/routes/crm.ts index 9abfb03..12d5ebc 100644 --- a/apps/server/src/routes/crm.ts +++ b/apps/server/src/routes/crm.ts @@ -41,6 +41,7 @@ import { import { vValidator } from '@hono/valibot-validator'; import { eq } from 'drizzle-orm'; import { Hono } from 'hono'; +import { bodyLimit } from 'hono/body-limit'; import { COMPANY_ROLLUP_MAX_CONTACTS, CONTACT_EXPORT_MAX_EVENTS, @@ -77,12 +78,41 @@ import { findLinkedVisitorHashes, findLinkedVisitorHashesForMany, } from '../lib/consent.js'; +import { CRM_MAX_BODY_BYTES } from '../lib/constants.js'; import { ApiError, validationErrorHook } from '../lib/http.js'; +import { rateLimit } from '../lib/ratelimit.js'; export const crmRoutes = new Hono(); crmRoutes.use('*', requireCrm); +// The global body limit is path-scoped to /api/collect, so it never reached here — leaving the one +// route group that stores personal data as the only one accepting an unbounded upload. +crmRoutes.use( + '*', + bodyLimit({ + maxSize: CRM_MAX_BODY_BYTES, + onError: () => { + throw new ApiError('payload_too_large', 413); + }, + }), +); + +/** + * Rate limit, keyed by the OPERATOR rather than the site. + * + * Everything else in this codebase keys its bucket per site, because the risk it manages is one + * tenant's traffic drowning another's. The risk here is different: these are the only routes that + * return names, emails and phone numbers, and the threat is a single stolen session pulling the whole + * table a page at a time. Keying per site would let a compromised operator hide inside their team's + * legitimate traffic and would punish their colleagues for it; keying per operator caps the session + * that is actually doing it. + * + * Applied AFTER the role guard at every call site, matching /api/event: an unauthenticated request is + * rejected before it can consume anyone's bucket, and `userId` is only set once a session resolves. + */ +const crmRateLimit = rateLimit((c) => `crm:${c.get('userId') ?? 'unauthenticated'}`); + /** Resolve a contact or raise the canonical 404. Scoped by the authorized site, so a contact id from * another site is indistinguishable from one that does not exist. */ async function loadContact(env: Env, siteId: string, id: string): Promise { @@ -157,6 +187,7 @@ function linkedHashes( crmRoutes.get( '/contacts', requireTeamRole('analyst'), + crmRateLimit, vValidator('query', ContactListQuerySchema, validationErrorHook), async (c) => { const query = c.req.valid('query'); @@ -173,6 +204,7 @@ crmRoutes.get( crmRoutes.post( '/contacts', requireTeamRole('analyst'), + crmRateLimit, vValidator('json', ContactCreateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -188,7 +220,7 @@ crmRoutes.post( }, ); -crmRoutes.get('/contacts/:id', requireTeamRole('analyst'), async (c) => { +crmRoutes.get('/contacts/:id', requireTeamRole('analyst'), crmRateLimit, async (c) => { const contact = await loadContact(c.env, c.get('siteId'), c.req.param('id')); return c.json({ contact }); }); @@ -196,6 +228,7 @@ crmRoutes.get('/contacts/:id', requireTeamRole('analyst'), async (c) => { crmRoutes.patch( '/contacts/:id', requireTeamRole('analyst'), + crmRateLimit, vValidator('json', ContactUpdateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -225,7 +258,7 @@ crmRoutes.patch( * keyed by a salted hash, and with the consent record gone nothing can ever re-associate them with a * person; destroying the link is what erasure of the identifiable data means here. */ -crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), async (c) => { +crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), crmRateLimit, async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id') ?? ''); // The two writes land in DIFFERENT databases and D1 has no transaction spanning them, so one of @@ -251,7 +284,7 @@ crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), async (c) => { /** A contact's analytics, if and only if an active signed consent record authorizes the link. When * it does not, the response says so explicitly rather than returning zeroes that read like "this * person did nothing" — `linked: false` and a reason are the honest answer. */ -crmRoutes.get('/contacts/:id/analytics', requireTeamRole('analyst'), async (c) => { +crmRoutes.get('/contacts/:id/analytics', requireTeamRole('analyst'), crmRateLimit, async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id')); if (!contact.external_user_id) { @@ -278,7 +311,7 @@ crmRoutes.get('/contacts/:id/analytics', requireTeamRole('analyst'), async (c) = * (their claims are a derived hash, a tier and a window), so including them adds cryptographic * evidence of what was consented to without widening what the export reveals. */ -crmRoutes.get('/contacts/:id/export', requireTeamRole('admin'), async (c) => { +crmRoutes.get('/contacts/:id/export', requireTeamRole('admin'), crmRateLimit, async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id')); const externalUserId = contact.external_user_id; @@ -336,6 +369,7 @@ function companyConflict(err: unknown): never { crmRoutes.get( '/companies', requireTeamRole('analyst'), + crmRateLimit, vValidator('query', CompanyListQuerySchema, validationErrorHook), async (c) => { const query = c.req.valid('query'); @@ -352,6 +386,7 @@ crmRoutes.get( crmRoutes.post( '/companies', requireTeamRole('analyst'), + crmRateLimit, vValidator('json', CompanyCreateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -367,7 +402,7 @@ crmRoutes.post( }, ); -crmRoutes.get('/companies/:id', requireTeamRole('analyst'), async (c) => { +crmRoutes.get('/companies/:id', requireTeamRole('analyst'), crmRateLimit, async (c) => { const company = await loadCompany(c.env, c.get('siteId'), c.req.param('id')); return c.json({ company }); }); @@ -375,6 +410,7 @@ crmRoutes.get('/companies/:id', requireTeamRole('analyst'), async (c) => { crmRoutes.patch( '/companies/:id', requireTeamRole('analyst'), + crmRateLimit, vValidator('json', CompanyUpdateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -402,7 +438,7 @@ crmRoutes.patch( * `admin` rather than `analyst` because it is irreversible and it rewrites rows the caller did not * name — the same reason deleting a contact is. */ -crmRoutes.delete('/companies/:id', requireTeamRole('admin'), async (c) => { +crmRoutes.delete('/companies/:id', requireTeamRole('admin'), crmRateLimit, async (c) => { const result = await deleteCompany(requireCrmDb(c.env), c.get('siteId'), c.req.param('id')); if (!result) { throw new ApiError('not_found', 404); @@ -413,6 +449,7 @@ crmRoutes.delete('/companies/:id', requireTeamRole('admin'), async (c) => { crmRoutes.get( '/companies/:id/contacts', requireTeamRole('analyst'), + crmRateLimit, vValidator('query', CompanyContactsQuerySchema, validationErrorHook), async (c) => { const siteId = c.get('siteId'); @@ -449,7 +486,7 @@ crmRoutes.get( * start reasoning about the eleven who never consented. So `contacts_total` and `contacts_linked` are * reported side by side and one-of-twelve is visible as one-of-twelve. */ -crmRoutes.get('/companies/:id/analytics', requireTeamRole('analyst'), async (c) => { +crmRoutes.get('/companies/:id/analytics', requireTeamRole('analyst'), crmRateLimit, async (c) => { const siteId = c.get('siteId'); const company = await loadCompany(c.env, siteId, c.req.param('id')); const linkage = await companyContactLinkage( diff --git a/apps/server/test/crm.test.ts b/apps/server/test/crm.test.ts index 1d72b32..e2ebfbb 100644 --- a/apps/server/test/crm.test.ts +++ b/apps/server/test/crm.test.ts @@ -8,6 +8,7 @@ // events sitting right there and is still excluded — revoked consent, and a forged statement. import { env } from 'cloudflare:test'; +import { CRM_MAX_OFFSET } from '@facet/shared'; import { generateSigningJwk } from '@facet/trust'; import { beforeEach, describe, expect, it } from 'vitest'; import { createApp } from '../src/app.js'; @@ -1387,3 +1388,57 @@ describe('a genuine consent statement authorizes only the person it was issued f expect(body.activity.total).toBe(2); }); }); + +describe('the PII routes are bounded, not just authenticated', () => { + /** An env whose rate limiter denies everything, which is how a wired-up limiter is distinguished + * from one that was never attached. The real binding is absent in tests, so the middleware + * no-ops and its presence is otherwise unobservable. */ + function denyingLimiter(e: TestEnv): TestEnv { + return { ...e, RATE_LIMITER: { limit: async () => ({ success: false }) } } as TestEnv; + } + + it('rate limits an authenticated operator, and only after authenticating them', async () => { + const e = denyingLimiter(env); + const cookie = await operator(e, 'analyst@example.com', 'analyst'); + const limited = await crm(e, '/contacts', {}, cookie); + expect(limited.status).toBe(429); + expect(limited.headers.get('Retry-After')).toBe('60'); + + // Auth still runs first: an anonymous caller is rejected as unauthorized rather than being + // told it was rate limited, so an unauthenticated flood cannot consume anyone's bucket. + const anonymous = await crm(e, '/contacts'); + expect(anonymous.status).toBe(401); + + // A viewer is refused on role, also before the limiter. + const viewerCookie = await operator(e, 'viewer@example.com', 'viewer'); + expect((await crm(e, '/contacts', {}, viewerCookie)).status).toBe(403); + }); + + it('covers the company routes too, not just contacts', async () => { + const e = denyingLimiter(env); + const cookie = await operator(e, 'admin@example.com', 'admin'); + expect((await crm(e, '/companies', {}, cookie)).status).toBe(429); + }); + + it('refuses an oversized write body', async () => { + // The global bodyLimit is scoped to /api/collect, so before this the one route group storing + // personal data was the only one accepting an unbounded upload. + const cookie = await operator(env, 'admin@example.com', 'admin'); + const res = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada', notes: 'x'.repeat(50_000) }) }, + cookie, + ); + expect(res.status).toBe(413); + }); + + it('refuses to page arbitrarily deep', async () => { + const cookie = await operator(env, 'admin@example.com', 'admin'); + expect((await crm(env, `/contacts?offset=${CRM_MAX_OFFSET + 1}`, {}, cookie)).status).toBe( + 400, + ); + // The ceiling itself is still reachable, so this is a bound and not an off-by-one. + expect((await crm(env, `/contacts?offset=${CRM_MAX_OFFSET}`, {}, cookie)).status).toBe(200); + }); +}); diff --git a/packages/shared/src/crm.ts b/packages/shared/src/crm.ts index 60da00c..6d53bb3 100644 --- a/packages/shared/src/crm.ts +++ b/packages/shared/src/crm.ts @@ -93,6 +93,9 @@ export const ContactUpdateSchema = v.pipe( * thing enforcing it, so the server imports these rather than repeating the numbers next to a second * copy that can drift. */ export const CRM_MAX_PAGE = 100; + +/** How deep a CRM list may be paged. */ +export const CRM_MAX_OFFSET = 100_000; export const CRM_DEFAULT_PAGE = 25; const pageBounds = { @@ -106,8 +109,17 @@ const pageBounds = { v.maxValue(CRM_MAX_PAGE), ), ), + /** Bounded at both ends. SQLite walks every skipped row, so an unbounded `offset` is a full + * table scan and the natural shape of a page-by-page bulk read of the whole contact list. */ offset: v.optional( - v.pipe(v.string(), v.transform(Number), v.number(), v.integer(), v.minValue(0)), + v.pipe( + v.string(), + v.transform(Number), + v.number(), + v.integer(), + v.minValue(0), + v.maxValue(CRM_MAX_OFFSET), + ), ), }; From 467bf11a7770649bc408993f0841de87d7989d5a Mon Sep 17 00:00:00 2001 From: David Condrey Date: Tue, 4 Aug 2026 22:31:43 -0700 Subject: [PATCH 4/5] feat(dashboard): add the CRM tab for contacts and companies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing in the dashboard consumed the CRM API, so the extension was reachable only by raw HTTP. This adds a CRM tab with two master/detail sections — contacts and companies — each searchable, status-filtered and paged, with the record's analytics beside it. The states that carry the design are the ones worth naming. A 501 is the DEFAULT for any deployment that never bound CRM_DB, so it renders as a calm "not installed" panel naming the binding, with no alert role and no retry; `crmBlockOf` classifies 501/503/401/403 as non-transient so React Query stops rather than hammering. `linked: false` renders the reason and an explicit note that it is not a report of zero activity, and the figures are not drawn at all — zeroes would answer a different question. The company rollup always shows "N of M contacts linked" above its numbers, and `contacts_truncated` adds a lower-bound warning, so a rollup covering one person out of twelve cannot read as the account's traffic. Auth is the session cookie via a new `sessionFetch`, which deliberately sends no Authorization header because these routes refuse API keys. Its error mapping falls back to the status when a body carries no code, so a proxy cannot collapse "this deployment has no CRM" into a generic failure — the whole tab's behaviour turns on telling those apart. The list responses now carry the role they were served under, and that is a server change made because the browser genuinely cannot answer it: /api/auth/me reports a role per TEAM and no session-reachable route maps a site to its owning team. The first version inferred admin only when EVERY membership granted it, which is sound but hides the delete button from anyone who is admin on one team and viewer on another. The server already resolved the exact role to authorize the request, so it reports it. An absent role still reads as "no", so the action appears when the answer arrives rather than flickering away when it does. Also, autonomously: deleted `useSession` and the session types it needed, which became unreachable once the role stopped being inferred; added the `none_linked_within_cap` reason text; and taught the demo mock to answer 501/503, which is what a static demo genuinely is. Verified in the main tree rather than taking the agent's word for it: biome, tsc and the full workspace suite are clean at 1583 tests across 174 files, up from 1537. --- apps/dashboard/src/App.tsx | 5 + apps/dashboard/src/api.ts | 44 +++ apps/dashboard/src/components/Crm.tsx | 131 +++++++ .../src/components/crm/CompaniesPanel.tsx | 270 ++++++++++++++ .../src/components/crm/CompanyDetail.tsx | 315 +++++++++++++++++ .../src/components/crm/CompanyForm.tsx | 125 +++++++ .../src/components/crm/ContactDetail.tsx | 262 ++++++++++++++ .../src/components/crm/ContactForm.tsx | 234 +++++++++++++ .../src/components/crm/ContactsPanel.tsx | 288 +++++++++++++++ apps/dashboard/src/components/crm/shared.tsx | 262 ++++++++++++++ .../dashboard/src/components/settings/kit.tsx | 6 +- apps/dashboard/src/demo/mockApi.ts | 13 + apps/dashboard/src/hooks/crm.ts | 246 +++++++++++++ apps/dashboard/src/lib/crm.ts | 155 ++++++++ apps/dashboard/src/lib/download.ts | 43 ++- apps/dashboard/src/lib/segment.ts | 8 + apps/dashboard/src/test/a11y.test.tsx | 12 +- apps/dashboard/src/test/crm.test.tsx | 331 ++++++++++++++++++ apps/dashboard/src/test/segment.test.tsx | 1 + apps/server/src/routes/crm.ts | 15 +- docs/api.md | 17 +- 21 files changed, 2759 insertions(+), 24 deletions(-) create mode 100644 apps/dashboard/src/components/Crm.tsx create mode 100644 apps/dashboard/src/components/crm/CompaniesPanel.tsx create mode 100644 apps/dashboard/src/components/crm/CompanyDetail.tsx create mode 100644 apps/dashboard/src/components/crm/CompanyForm.tsx create mode 100644 apps/dashboard/src/components/crm/ContactDetail.tsx create mode 100644 apps/dashboard/src/components/crm/ContactForm.tsx create mode 100644 apps/dashboard/src/components/crm/ContactsPanel.tsx create mode 100644 apps/dashboard/src/components/crm/shared.tsx create mode 100644 apps/dashboard/src/hooks/crm.ts create mode 100644 apps/dashboard/src/lib/crm.ts create mode 100644 apps/dashboard/src/test/crm.test.tsx diff --git a/apps/dashboard/src/App.tsx b/apps/dashboard/src/App.tsx index 6d971d6..0655d1f 100644 --- a/apps/dashboard/src/App.tsx +++ b/apps/dashboard/src/App.tsx @@ -24,6 +24,7 @@ const AllSites = lazy(() => const Anomalies = lazy(() => import('./components/Anomalies.js').then((m) => ({ default: m.Anomalies })), ); +const Crm = lazy(() => import('./components/Crm.js').then((m) => ({ default: m.Crm }))); const AskPanel = lazy(() => import('./components/AskPanel.js').then((m) => ({ default: m.AskPanel })), ); @@ -92,6 +93,7 @@ type View = | 'retention' | 'experiments' | 'anomalies' + | 'crm' | 'ask' | 'docs'; @@ -167,6 +169,7 @@ const TABS: { id: View; label: string }[] = [ { id: 'retention', label: 'Retention' }, { id: 'experiments', label: 'Experiments' }, { id: 'anomalies', label: 'Anomalies' }, + { id: 'crm', label: 'CRM' }, { id: 'ask', label: 'Ask' }, { id: 'docs', label: 'Documentation' }, ]; @@ -620,6 +623,8 @@ function Dashboard(): ReactElement { range={range} onInvestigate={investigate} /> + ) : view === 'crm' ? ( + ) : view === 'ask' ? ( ) : ( diff --git a/apps/dashboard/src/api.ts b/apps/dashboard/src/api.ts index 80cc7b3..3307ded 100644 --- a/apps/dashboard/src/api.ts +++ b/apps/dashboard/src/api.ts @@ -56,3 +56,47 @@ export async function apiPost(path: string, apiKey: string, body: unknown): P export function fetchStats(apiKey: string, query: StatsQuery): Promise { return apiFetch(`/api/stats?${qs(query)}`, apiKey); } + +/** + * The error code for a failed response: the API's own `{ error }` when it sent one, otherwise + * derived from the status. The fallback matters for the session routes, whose whole UI hinges on + * telling a 501 (this deployment has no CRM database) apart from a 403 (your role is too low) — a + * proxy or a truncated body must not collapse both into an indistinguishable "request_failed". + */ +function errorCode(status: number, body: { error?: string }): string { + if (body.error) return body.error; + if (status === 501) return 'crm_unavailable'; + if (status === 403) return 'forbidden'; + if (status === 401) return 'unauthorized'; + return 'request_failed'; +} + +/** + * Canonical helper for the SESSION-authenticated API (`/api/auth/me`, `/api/crm/*`). + * + * Deliberately sends no `Authorization` header: those routes refuse an API key by design, because a + * `clk_` key reads aggregate analytics and is handed out accordingly while contact PII is not. Auth + * is the HttpOnly session cookie, which a same-origin request carries on its own — `same-origin` is + * the browser default and is stated here so the intent survives the next refactor. + */ +export async function sessionFetch( + path: string, + init?: { method?: 'GET' | 'POST' | 'PATCH' | 'DELETE'; body?: unknown }, +): Promise { + const method = init?.method ?? 'GET'; + const res = await fetch(path, { + method, + credentials: 'same-origin', + ...(init?.body === undefined + ? {} + : { + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(init.body), + }), + }); + if (!res.ok) { + const body = (await res.json().catch(() => ({}))) as { error?: string }; + throw new Error(errorCode(res.status, body)); + } + return (await res.json()) as T; +} diff --git a/apps/dashboard/src/components/Crm.tsx b/apps/dashboard/src/components/Crm.tsx new file mode 100644 index 0000000..7d6d9dd --- /dev/null +++ b/apps/dashboard/src/components/Crm.tsx @@ -0,0 +1,131 @@ +// The CRM tab: contacts and companies, the one surface in this dashboard that holds directly +// identifying personal data. +// +// It is authenticated differently from every other tab, and that is deliberate rather than an +// oversight to fix. Everything else here reads with a per-site `clk_` API key; these routes refuse +// one, because a key that leaks costs you aggregate pageview counts while a key that could read +// contacts would cost you your customers' names, emails and phone numbers. So the CRM is gated on an +// operator session cookie plus a team role, and the panels below explain that rather than failing +// with a bare 401. +// +// Two states dominate what this tab renders in practice: +// • NO CRM DATABASE (501). The extension is an optional second D1 binding that most deployments +// never enable, so this is the DEFAULT and it renders as an explanation, never as an error. +// • ROLE. `viewer` sees nothing here; `analyst` reads and writes; `admin` deletes and exports. +// The destructive controls are hidden rather than offered and then refused — see +// `canAdministerCrm` for what the browser can actually prove about the operator's role. + +import { type ReactElement, useState } from 'react'; +import { cn } from '../lib/cn.js'; +import { SegmentNotice } from './CubeFilterBar.js'; +import { CompaniesPanel } from './crm/CompaniesPanel.js'; +import { ContactsPanel } from './crm/ContactsPanel.js'; + +type Section = 'contacts' | 'companies'; + +const SECTIONS: { id: Section; label: string }[] = [ + { id: 'contacts', label: 'Contacts' }, + { id: 'companies', label: 'Companies' }, +]; + +/** Roving-tabindex arrow navigation, as `role="tablist"` promises to assistive tech. Returns true + * when the key was consumed, so the caller can suppress the page scroll. */ +function onSectionKey(key: string, current: Section, select: (id: Section) => void): boolean { + const index = SECTIONS.findIndex((s) => s.id === current); + if (index < 0) return false; + let next: number; + if (key === 'ArrowRight') next = (index + 1) % SECTIONS.length; + else if (key === 'ArrowLeft') next = (index - 1 + SECTIONS.length) % SECTIONS.length; + else if (key === 'Home') next = 0; + else if (key === 'End') next = SECTIONS.length - 1; + else return false; + const target = SECTIONS[next]; + if (!target) return false; + select(target.id); + document.getElementById(`crm-tab-${target.id}`)?.focus(); + return true; +} + +export function Crm({ siteId }: { siteId: string }): ReactElement { + const [section, setSection] = useState
('contacts'); + // Both selections live here so the two panels can hand off to each other: a contact's employer + // opens the company, and a company's roster opens the person. + const [contactId, setContactId] = useState(''); + const [companyId, setCompanyId] = useState(''); + + const openCompany = (id: string): void => { + setCompanyId(id); + if (id) setSection('companies'); + }; + const openContact = (id: string): void => { + setContactId(id); + if (id) setSection('contacts'); + }; + + return ( +
+
+

CRM

+

+ The people and organizations behind the numbers. A contact is only ever + connected to analytics through an active signed consent record — never through + anything stored on the contact itself — so a person with no consent simply has + no link, and this tab says so rather than showing zeroes. +

+
+ + {/* The chips above this tab are a filter over analytics dimensions. Nothing here honours + them, and a filtered label over unfiltered numbers is worse than no filter at all. */} + + +
+ {SECTIONS.map((s) => ( + + ))} +
+ +
+ {section === 'contacts' ? ( + + ) : ( + + )} +
+
+ ); +} diff --git a/apps/dashboard/src/components/crm/CompaniesPanel.tsx b/apps/dashboard/src/components/crm/CompaniesPanel.tsx new file mode 100644 index 0000000..640ff1f --- /dev/null +++ b/apps/dashboard/src/components/crm/CompaniesPanel.tsx @@ -0,0 +1,270 @@ +// The companies roster: search, status filter, paging, create, and a detail pane carrying the +// consent-gated rollup for the selected company. + +import { Plus } from 'lucide-react'; +import { type ReactElement, useEffect, useState } from 'react'; +import { CRM_PAGE_SIZE, useCompanies, useCompany, useCreateCompany } from '../../hooks/crm.js'; +import { cn } from '../../lib/cn.js'; +import { COMPANY_STATUSES, canAdministerCrm } from '../../lib/crm.js'; +import { CardSkeletons, EmptyState } from '../StatusStates.js'; +import { CompanyDetail } from './CompanyDetail.js'; +import { CompanyForm } from './CompanyForm.js'; +import { CrmAccessNotice, Pager, StatusChip } from './shared.js'; + +const SEARCH_DEBOUNCE_MS = 300; + +export function CompaniesPanel({ + siteId, + selectedId, + onSelect, + onOpenContact, +}: { + siteId: string; + /** Owned by the tab shell so a contact's company link can select one from the other panel. */ + selectedId: string; + onSelect: (companyId: string) => void; + onOpenContact: (contactId: string) => void; +}): ReactElement { + // The role the server served this list under — the only authoritative answer available to + // the browser. See `canAdministerCrm`. + const [search, setSearch] = useState(''); + const [query, setQuery] = useState(''); + const [status, setStatus] = useState(''); + const [offset, setOffset] = useState(0); + const [creating, setCreating] = useState(false); + const [deleted, setDeleted] = useState(null); + + useEffect(() => { + const timer = setTimeout(() => { + setQuery(search); + setOffset(0); + }, SEARCH_DEBOUNCE_MS); + return () => clearTimeout(timer); + }, [search]); + + const list = useCompanies(siteId, { status, q: query, offset }); + const canAdminister = canAdministerCrm(list.data?.role); + const selected = useCompany(siteId, selectedId); + const create = useCreateCompany(siteId); + + if (list.error) { + return ( + void list.refetch()} + retrying={list.isFetching} + /> + ); + } + + const companies = list.data?.companies ?? []; + const total = list.data?.total ?? 0; + const filtering = Boolean(query.trim() || status); + + return ( +
+
+
+
+ + setSearch(e.target.value)} + placeholder="Name or domain" + className="input mt-1 block w-full rounded-lg px-3 py-1.5 text-sm" + /> +
+
+ + +
+ +
+ + {creating ? ( + setCreating(false)} + onSubmit={(fields) => + create.mutate(fields, { + onSuccess: (result) => { + setCreating(false); + onSelect(result.company.id); + }, + }) + } + /> + ) : null} + + {deleted ? ( +

+ {deleted} +

+ ) : null} + + {list.isLoading ? ( + + ) : companies.length === 0 ? ( + + {filtering ? ( + <>Clear the search or the status filter to see every company. + ) : ( + <> + A company turns a contact’s employer into a structured link + instead of typed text, and gives you one rollup across everyone who + works there. + + )} + + ) : ( + <> +
+ + + + + {['Company', 'Domain', 'Status'].map((label) => ( + + ))} + + + + {companies.map((company) => ( + + + + + + ))} + +
Companies on this site.
+ {label} +
+ + + {company.domain ?? '—'} + + +
+
+ + + )} +
+ +
+ {!selectedId ? ( +

+ Pick a company to see who works there and how much of its traffic Facet is + actually allowed to attribute to it. +

+ ) : selected.error ? ( + void selected.refetch()} + retrying={selected.isFetching} + /> + ) : selected.data ? ( + { + setDeleted( + unlinked === 1 + ? 'Company deleted. 1 contact was unlinked and kept.' + : `Company deleted. ${unlinked} contacts were unlinked and kept.`, + ); + onSelect(''); + }} + /> + ) : ( + + )} +
+
+ ); +} diff --git a/apps/dashboard/src/components/crm/CompanyDetail.tsx b/apps/dashboard/src/components/crm/CompanyDetail.tsx new file mode 100644 index 0000000..e003a79 --- /dev/null +++ b/apps/dashboard/src/components/crm/CompanyDetail.tsx @@ -0,0 +1,315 @@ +// One company: its record, its roster, and the consent-gated rollup of its contacts' analytics. +// +// THE DENOMINATOR IS NOT DECORATION. A rollup covering one of twelve people reads as the account's +// traffic unless the coverage is stated next to the numbers, and an operator who mistakes it for the +// whole will start reasoning about the eleven who never consented. So `contacts_linked of +// contacts_total` is rendered above the figures on every response — linked or not — and a truncated +// rollup says it is a lower bound rather than presenting a capped sum as a total. + +import { Link2Off, Users } from 'lucide-react'; +import { type ReactElement, useState } from 'react'; +import { + CRM_PAGE_SIZE, + useCompanyAnalytics, + useCompanyContacts, + useDeleteCompany, + useUpdateCompany, +} from '../../hooks/crm.js'; +import { type CompanyRollupCounts, type CrmCompany, linkReasonText } from '../../lib/crm.js'; +import { formatDateTime } from '../../lib/datetime.js'; +import { formatNumber } from '../../lib/format.js'; +import { Skeleton } from '../StatusStates.js'; +import { ConfirmDelete, MutationStatus } from '../settings/kit.js'; +import { CompanyForm } from './CompanyForm.js'; +import { ActivityFigures, CrmAccessNotice, DetailRow, Pager, StatusChip } from './shared.js'; + +/** The coverage statement. Always rendered, because "how much of this company is in these numbers" + * is part of the answer and not a caveat on it. */ +function Coverage({ counts }: { counts: CompanyRollupCounts }): ReactElement { + const { contacts_linked, contacts_total, contacts_considered, contacts_truncated } = counts; + return ( +
+
+

+ Coverage +

+

+ {formatNumber(contacts_linked)} of {formatNumber(contacts_total)}{' '} + {contacts_total === 1 ? 'contact' : 'contacts'} linked +

+

+ Only a contact with an active signed consent record contributes. These figures + are not this company’s whole traffic — they are the part {contacts_linked}{' '} + of its {contacts_total} people authorized. +

+
+ {contacts_truncated ? ( +

+ Lower bound, not a total. Consent was resolved for the first{' '} + {formatNumber(contacts_considered)} contacts only (the per-rollup limit is{' '} + {formatNumber(counts.contacts_limit)}), so contacts beyond that are missing from + these numbers. +

+ ) : null} +
+ ); +} + +function CompanyAnalyticsPanel({ + siteId, + companyId, +}: { + siteId: string; + companyId: string; +}): ReactElement { + const analytics = useCompanyAnalytics(siteId, companyId); + + if (analytics.error) { + return ( + void analytics.refetch()} + retrying={analytics.isFetching} + /> + ); + } + if (!analytics.data) return ; + + const data = analytics.data; + return ( +
+ + {data.linked ? ( + <> + +

+ Summed over {formatNumber(data.visitor_hashes)} visitor{' '} + {data.visitor_hashes === 1 ? 'hash' : 'hashes'} — a linkage-breadth number, + not a headcount. The headcount is the coverage above. +

+ + ) : ( +
+
+ )} +
+ ); +} + +/** The roster query is owned by the detail, not by this list: the delete confirmation needs its + * `total` to say how many people survive the deletion, and a child cannot hand a parent state + * during render. */ +type RosterQuery = ReturnType; + +function Roster({ + roster, + offset, + onOffset, + onOpenContact, +}: { + roster: RosterQuery; + offset: number; + onOffset: (next: number) => void; + onOpenContact: (contactId: string) => void; +}): ReactElement { + if (roster.error) { + return ( + void roster.refetch()} + retrying={roster.isFetching} + /> + ); + } + if (!roster.data) return ; + if (roster.data.contacts.length === 0) { + return ( +

+ No contact is linked to this company yet. Link one from their record, under{' '} + Linked company. +

+ ); + } + + return ( +
+
    + {roster.data.contacts.map((contact) => ( +
  • + +
    • + ))} +
+ +
+ ); +} + +export function CompanyDetail({ + siteId, + company, + canAdminister, + onDeleted, + onOpenContact, +}: { + siteId: string; + company: CrmCompany; + /** True only when this operator provably holds `admin`; see `canAdministerCrm`. */ + canAdminister: boolean; + onDeleted: (contactsUnlinked: number) => void; + onOpenContact: (contactId: string) => void; +}): ReactElement { + const [editing, setEditing] = useState(false); + const [rosterOffset, setRosterOffset] = useState(0); + const roster = useCompanyContacts(siteId, company.id, rosterOffset); + const contactCount = roster.data?.total ?? null; + const update = useUpdateCompany(siteId, company.id); + const remove = useDeleteCompany(siteId); + + if (editing) { + return ( +
+

Edit company

+ setEditing(false)} + onSubmit={(fields) => + update.mutate(fields, { onSuccess: () => setEditing(false) }) + } + /> +
+ ); + } + + const survivors = + contactCount === null + ? 'Its contacts are kept' + : contactCount === 1 + ? 'Its 1 contact is kept' + : `Its ${contactCount} contacts are kept`; + + return ( +
+
+
+

+ {company.name} +

+

+ Updated {formatDateTime(company.updated_at)} +

+
+
+ + +
+
+ +
+ {company.domain} + + {company.notes ? ( + {company.notes} + ) : null} + + {formatDateTime(company.created_at)} +
+ +
+

+

+ +
+ +
+

Analytics rollup

+ +
+ +
+ {canAdminister ? ( + + remove.mutate(company.id, { + onSuccess: (result) => onDeleted(result.contacts_unlinked), + }) + } + /> + ) : ( +

+ Deleting a company needs the admin role on the team that + owns this site. +

+ )} + +
+
+ ); +} diff --git a/apps/dashboard/src/components/crm/CompanyForm.tsx b/apps/dashboard/src/components/crm/CompanyForm.tsx new file mode 100644 index 0000000..6217cc2 --- /dev/null +++ b/apps/dashboard/src/components/crm/CompanyForm.tsx @@ -0,0 +1,125 @@ +// One form for creating and for editing a company. As with contacts, every field is submitted on +// every save because the API reads `''` as "clear this" — the only way a form can unset a field. + +import { type ReactElement, useState } from 'react'; +import type { CrmFields } from '../../hooks/crm.js'; +import { COMPANY_STATUSES, type CrmCompany } from '../../lib/crm.js'; +import { BlockedReason, Field, FormControls, MutationStatus, Select } from '../settings/kit.js'; + +interface Draft { + name: string; + domain: string; + status: string; + notes: string; +} + +export function CompanyForm({ + company, + submitLabel, + pendingLabel, + onSubmit, + onCancel, + isPending, + error, +}: { + /** The company being edited, or null when creating. */ + company: CrmCompany | null; + submitLabel: string; + pendingLabel: string; + onSubmit: (fields: CrmFields) => void; + onCancel: () => void; + isPending: boolean; + error: unknown; +}): ReactElement { + const [draft, setDraft] = useState(() => ({ + name: company?.name ?? '', + domain: company?.domain ?? '', + status: company?.status ?? 'lead', + notes: company?.notes ?? '', + })); + const set = (key: keyof Draft) => (value: string) => + setDraft((prev) => ({ ...prev, [key]: value })); + // `companies.name` is NOT NULL and it is the display value: a company with no name is a row + // nothing can refer to. + const blocked = draft.name.trim() ? null : 'A company needs a name.'; + const idFor = (field: string) => `company-${company?.id ?? 'new'}-${field}`; + + return ( +
{ + e.preventDefault(); + if (blocked || isPending) return; + onSubmit({ + name: draft.name, + domain: draft.domain, + status: draft.status, + notes: draft.notes, + }); + }} + > + + + + +
+ +