Skip to content

fix(server): close a D1 limit, a consent replay, and the CRM's missing bounds - #42

Merged
dcondrey merged 5 commits into
mainfrom
feat/crm-hardening
Aug 5, 2026
Merged

fix(server): close a D1 limit, a consent replay, and the CRM's missing bounds#42
dcondrey merged 5 commits into
mainfrom
feat/crm-hardening

Conversation

@dcondrey

@dcondrey dcondrey commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Five findings from auditing the CRM after #41, plus the dashboard tab that finally consumes it. The first two are defects that would have reached production; the third is a hole in a security property the code claimed to hold.

The company rollup 500s on any account with 99 or more linkable contacts. D1 refuses a query carrying more than 100 bound parameters, and the consent lookup binds site_id and now on top of one per contact — so it asked for 101. COMPANY_ROLLUP_MAX_CONTACTS was set to 100 as though the whole allowance were available. It is a hard failure, not a slow one, and it lands on precisely the largest account rather than on the small ones a test reaches for. Verified against a live probe rather than from the docs alone: 98 ids plus 2 fixed binds passes, 99 fails with too many SQL variables. 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 on any deployment that raises RAW_RETENTION_DAYS.

All three now chunk, and the merge is written so the aggregate stays exact. Totals add; first and last seen take the extremes across chunks rather than whichever chunk finished last. 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 returns a plausible and subtly wrong ranking. The covering test is built on exactly that case: /sleeper is rank 13 in both chunks and rank 1 overall, so a prefix-and-merge implementation drops it entirely.

A genuine consent statement authorized the wrong person. 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 real, 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, and the two existing forgery tests both used an invalid signature, so they proved only that the signature check runs.

Concretely: the per-contact export returns consent statements verbatim, by design, as cryptographic evidence. 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 turns out to be recoverable rather than absent — the identified pre-image is uid:<uid>|salt|siteId and the claims carry the salt window — so the hash is now recomputed from the row's uid and required to equal the signed one. A missing salt fails closed, and that cannot strand 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. 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.

/api/crm is the only route group returning 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. One stolen analyst session could pull the entire contacts table — total comes back on the first response so the page count is known immediately, each page is 100 complete records including up to 4000 characters of notes per person, and nothing bounded the request rate. The limiter is keyed by the operator, not the site: everything else here keys per site because the risk there is one tenant drowning another, but a per-site key would let a compromised session hide inside its team's traffic while punishing colleagues for it. It sits after the role guard at every route, matching /api/event's deliberate ordering, and the tests assert an anonymous caller sees 401 rather than 429.

Three smaller ones found in the same pass. A capped rollup answered reason: no_linked_contacts, which is a claim about contacts it never examined — now none_linked_within_cap. deleteCompany captured the company name before the transaction, so a rename committing in between stamped every contact 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, and the unlinked count is taken inside the transaction instead of materialising one row per contact through .returning() to produce a single integer. Erasing a contact wrote to two databases in the order that loses data: 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. Erasing consent first leaves a retryable state. A foreign-key violation reached the client as a 500 when it is the caller losing a race with a concurrent company delete. And ContactUpdateSchema dropped the identifier invariant ContactCreateSchema enforces, so a PATCH could blank email, external id and name and leave a row that can never be matched, deduped or erased — NULLs being distinct in both unique indexes, nothing downstream would object.

The dashboard now consumes all of it. A CRM tab with contacts and companies as master/detail, searchable, status-filtered and paged. The states that carry the design: 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; linked: false renders its reason plus an explicit note that it is not a report of zero activity, and the figures are not drawn at all; the company rollup always shows "N of M contacts linked" above its numbers so one-of-twelve cannot read as the account's traffic. Auth is the session cookie through a new sessionFetch that deliberately sends no Authorization header, since these routes refuse API keys.

That surfaced one more server gap. The browser cannot learn its own role for a site — /api/auth/me reports a role per team, and no session-reachable route maps a site to its owning team — so the first version inferred admin only when every membership granted it. Sound, but it 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 the list responses now report it.

What the risk is not: no schema change, no migration, no new identifier, no change to ingest or to any analytics table, and no change to what a deployment without CRM_DB does. The consent kernel change is a tightening — it can only reject links that were previously accepted, and the test asserts the rightful owner still resolves, because a check that severed real links would pass a one-sided test.

Every new safety assertion was mutation-checked: each guard disabled in turn, confirming exactly its covering test fails, then restored. That includes the two D1 limits and the foreign key, which are pinned by live probes rather than by documentation — I checked that D1 actually enforces both before relying on either.

Two process notes. The bind-limit regression test passed on the first run because without a signing key the consent lookup returns before it ever builds the query; it only became a real test once given a key, which is why "make a new passing test fail on purpose" is in the standard. And a biome check --write run from inside apps/dashboard reformatted lib/worldGeo.ts, which the root config deliberately ignores as minified generated data — caught and reverted before it shipped.

pnpm lint, pnpm typecheck, pnpm test green: 1583 tests across 174 files, up from 1537.

…imit

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.
…ed for

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:<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.
/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.
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.
@dcondrey
dcondrey merged commit eaa24ac into main Aug 5, 2026
6 checks passed
@dcondrey
dcondrey deleted the feat/crm-hardening branch August 5, 2026 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant