feat(server): add the CRM extension behind an optional second D1 database - #40
Merged
Conversation
…base Facet had no way to hold a customer record, and the obvious way to add one would have quietly broken the thing being sold. Contacts are people who handed over their details directly; analytics visitors are salted hashes with no cross-session identity. Putting both in one database makes the join between them a foreign key someone adds later, and nothing structural stops them. So the CRM is a SEPARATE D1 database, bound as the optional `CRM_DB`, mounted in the same Worker. With no binding there is no database, no migration, no table, and every /api/crm route returns 501 `crm_unavailable`. That is the same gate every other optional feature already uses (AE, AI, FACET_SIGNING_JWK, WEBHOOK_URL, SCITT_URL, SESSION_SECRET), and it is the only arrangement where "excluded" means the tables do not exist. It also buys the property that matters: **D1 cannot join across databases**, so the contact-to-analytics link cannot be a foreign key. It has to be assembled in the Worker, which forces it through the consent check. The only bridge is `external_user_id` behind an ACTIVE `identified`-tier consent record. `events` gains no person-keyed column and a contact with no consent row has zero analytics linkage. `findLinkedVisitorHashes` treats the column purely as an index: the visitor hash it returns comes out of the SIGNED claims, verified against the deployment key, so a hand-written consent row pointing at someone else's hash links nothing. That is the case the mutation check covers — disabling the pinning fails exactly one test, with a forged payload complete enough that every non-cryptographic field check passes. Nothing in the CRM caches a derived visitor hash, which is what makes retention work without a CRM-side purge. Contacts are deliberately NOT on the raw-event schedule (a contact is a business record with its own lifecycle), but the LINK is: once `enforceRetention` drops the consent record, the join stops resolving on its own. Erasure is real deletion, not a tombstone, because a tombstone still holding an email is still that person's data; deleting a contact also ERASES rather than revokes their consent rows, since `revoked_at` would leave the raw external id at rest. A per-contact data-subject export ships in v1 rather than as a follow-up. **Auth is a session cookie and deliberately never an API key.** This is the one authenticated surface that refuses `clk_` keys, because they are not secret in the way a session is: /llms.txt advertises where to send one and a public demo dashboard can ship with one compiled in. A leaked key costs aggregate pageview counts; a key that could read /api/crm/contacts would cost customers' names, emails and phone numbers. `analyst` reads and writes, `admin` deletes and exports, and `viewer` gets nothing, because PII is a different kind of access rather than more of the same one. Binding this changes what the deployment IS, so it changes what the deployment ATTESTS. `privacyDpvClaims` was hardcoded and env-free while being served at /.well-known/facet-privacy.json and embedded in the SIGNED PrivacyAttestationCredential, so binding CRM_DB without touching it would have made the deployment cryptographically sign a false statement. It now takes `env`: with the CRM bound it adds dpv:Store and dpv:Erase, adds dpv:Consent alongside legitimate interest, names dpv:CustomerRelationshipManagement, declares the pd: categories actually held, and stops letting dpv:Pseudonymisation stand as the sole measure while contact details sit in the clear. Every term was checked against the DPV 2.1 spec rather than recalled. Migration tooling was single-database throughout. A second drizzle config, schema, and migrations directory land with their own generate/apply scripts; vitest reads both migration sets and the test config generator injects the CRM binding with a DISTINCT database_id, since miniflare keys local databases by that id and reusing one string would have silently made CRM_DB an alias of DB. A test asserts the `contacts` table is absent from the analytics database and `events` from the CRM one, so the isolation claim is checked rather than assumed. The shipped wrangler.jsonc carries the block commented out with what enabling it implies. Also, autonomously: `sites.team_id` was writable by nothing in the shipped code. `siteRole` therefore always returned null, which made the dashboard-session branch of `requireSiteAccess` unreachable in production and left the whole accounts/RBAC surface exercisable only by a test setting the column with raw SQL. Gating contact PII on a team role would have been gating it on something no operator could turn on, so `PATCH /api/sites/:id/team` closes it; passing null unassigns the site and revokes every session's access in one step. `contactActivity` counted pageviews as `name = 'pageview'`. This schema defines a pageview as `name IS NULL`, so that would have reported zero pageviews for every real visitor while the test fixture, written under the same wrong assumption, kept agreeing with it. It now imports `pageviewCount`/`eventCount` from db/stats.ts, so one contact's numbers cannot disagree with the numbers /api/stats reports for the same rows. The unique-constraint check walked only `err.message`, but drizzle wraps driver errors in `DrizzleQueryError` with the D1 error as `cause`, so a duplicate contact returned 500 instead of 409. It now walks the bounded cause chain, which survives a drizzle upgrade that adds or removes a wrapper. `verifyConsentRecord`'s deployment-key pinning is split into a shared helper rather than copied, because the two callers bind different things afterwards: ingest knows the hash and window up front, the CRM link derives the hash from the statement. LIKE metacharacters in the contact search are escaped, so `q=%` matches a literal percent sign instead of scanning the table. 1537 tests across 172 files, up from 1499/170.
…n contact search SQLite ignores backslash escapes in LIKE unless an ESCAPE clause is present, so prefixing the term did nothing on its own: \% was read as a literal backslash followed by the wildcard. Nothing matched, because no name contains a backslash, which made the guard look correct while quietly making a literal % unsearchable. The covering test asserted only that q=% did not return everything, so it passed either way. It now seeds a contact whose name actually contains a %, and requires that row to come back — matching neither everything nor nothing. Mutation-checked: dropping the ESCAPE clause fails exactly that test.
…m events only `events_truncated` compared the returned row count to `activity.events`, which counts custom events only. A contact whose traffic is entirely pageviews — the common case — has zero of those, so the flag read false while a thousand rows were dropped from the export. That is a subject-access request answered incorrectly, which is precisely what the flag exists to prevent. The comparison is now against `activity.total`. Missed when `contactActivity` was switched onto the canonical pageview/event split and this call site was not re-traced. Covered by a test seeding past the cap with pageviews specifically, since custom-event traffic would have passed either way; mutation-checked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Facet has never been able to hold a customer record, and the obvious way to add one would have quietly broken the thing being sold. Contacts are people who handed their details over directly. Analytics visitors are salted hashes with no cross-session identity. Put both in one database and the join between them is a foreign key someone adds later, with nothing structural in the way — the privacy claim would then depend on a code review noticing.
So the CRM is a separate D1 database, bound as the optional
CRM_DB, mounted in the same Worker. With no binding there is no database, no migration, no table, and every/api/crmroute returns 501crm_unavailable— the same gate AE, AI,FACET_SIGNING_JWK,WEBHOOK_URL,SCITT_URLandSESSION_SECRETalready use, and the only arrangement where "excluded" means the tables do not exist rather than that a flag hides them. It also buys the property this rests on: D1 cannot join across databases, so the contact→analytics link cannot be a foreign key. It has to be assembled in the Worker, which is what forces it through the consent check instead of past it.The only bridge is
external_user_idbehind an activeidentified-tier consent record, and the column is only an index.findLinkedVisitorHashesreturns the visitor hash out of the signed claims — verified against the deployment key, pinned byproof.kid, and required to assert this site and this tier — never out of thevisitor_hashcolumn beside it. A row hand-written intoconsent_recordspointing at an arbitrary victim's hash therefore links nothing.eventsgains no person-keyed column, and a contact with no consent row has zero analytics linkage. The pinning is shared withverifyConsentRecordrather than copied, because the two callers bind different things afterwards: ingest knows the hash and window up front, the CRM derives the hash from the statement.Nothing in the CRM caches a derived visitor hash, which is what makes retention work with no CRM-side purge at all. Contacts are deliberately not on the raw-event schedule — a contact is a business record with its own lifecycle, not telemetry — but the link is: once
enforceRetentiondrops the consent record, the join simply stops resolving, and there is no stale copy anywhere to forget. A test ages a grant past the window, runs the ordinary purge job, and asserts the link severs while the contact survives untouched.Erasure is real deletion and ships in v1, not as a follow-up. No tombstone, because a tombstone still holding an email is still that person's data. Deleting a contact also erases rather than revokes their consent rows:
revoked_atwould stop future elevation but leave the rawexternal_user_idat rest, which is precisely what the request was about. The pseudonymous event rows stay — with the consent record gone nothing can re-associate them with a person, and destroying the link is what erasing the identifiable data means here. A per-contact data-subject export lands alongside it, carrying the signed statements verbatim (they are PII-free by construction, so they add cryptographic evidence of what was consented to without widening what the export discloses) and statingevents_truncatedexplicitly rather than silently returning a prefix that would read as a complete answer to a subject-access request.Auth is a session cookie and deliberately never an API key. This is the one authenticated surface in the codebase that refuses
clk_keys, and the asymmetry is the point: a key is not secret the way a session is./llms.txtadvertises where to send one, and a public demo dashboard can ship with one compiled in (VITE_FACET_DEMO_API_KEY). A leaked key costs aggregate pageview counts; a key that could read/api/crm/contactswould cost customers' names, emails and phone numbers.analystreads and writes,admindeletes and exports, andviewer— who can see aggregate analytics — gets nothing, because PII is a different kind of access rather than more of the same one.ADMIN_TOKENis refused too, so the rule is uniform rather than mostly-uniform.Binding this changes what the deployment is, so it changes what the deployment attests.
privacyDpvClaims()took no env and hardcoded legitimate interest and pseudonymisation while being served at/.well-known/facet-privacy.jsonand embedded in the signedPrivacyAttestationCredential— so bindingCRM_DBwithout touching it would have made the deployment cryptographically sign a false statement about itself. It now takesenv: with the CRM bound it addsdpv:Storeanddpv:Erase, addsdpv:Consentalongside legitimate interest, namesdpv:CustomerRelationshipManagement, declares thepd:categories actually held, and stops lettingdpv:Pseudonymisationstand as the sole measure while contact details sit in the clear. Every term was checked against the DPV 2.1 spec rather than recalled. Three existing tests asserted the old constant and now assert the shape the binding produces.Migration tooling was single-database throughout. A second drizzle config, schema and migrations directory land with their own generate/apply scripts; vitest reads both sets, and the test-config generator injects
CRM_DBwith a distinctdatabase_id— miniflare keys local databases by that id, so reusing one string would have silently madeCRM_DBan alias ofDBand destroyed the isolation the whole design exists for. A test assertscontactsis absent from the analytics database andeventsfrom the CRM one, so that claim is checked rather than assumed.wrangler.test.jsoncregenerates byte-identically apart from the new binding, verified against the tracked config rather than the local one. The shippedwrangler.jsonccarries the block commented out, with what enabling it implies spelled out next to it.What the risk is not: no new identifier, no new column on
events, no change to any analytics table, no change to ingest, and no migration for the existing database. Nothing in this PR alters a single existing route or response shape. A deployment that does not bindCRM_DB— which is every deployment until someone runswrangler d1 create facet-crm— behaves byte-for-byte as before, right down to the DPV claims it serves and signs.Two things found along the way that had to land with it.
sites.team_idwas writable by nothing in the shipped code:siteRoletherefore always returned null, the dashboard-session branch ofrequireSiteAccesswas unreachable in production, and the entire accounts/RBAC surface was exercisable only by a test setting the column with raw SQL. Gating contact PII on a team role would have meant gating it on something no operator could turn on, soPATCH /api/sites/:id/teamcloses it. AndcontactActivitycounted pageviews asname = 'pageview'when this schema defines a pageview asname IS NULL— it would have reported zero pageviews for every real visitor, while the test fixture, written under the same wrong assumption, kept agreeing with it. It now importspageviewCount/eventCountfromdb/stats.ts, so one contact's numbers cannot disagree with what/api/statsreports for the same rows. Separately, the duplicate-contact check walked onlyerr.message, but drizzle wraps driver errors with the D1 error ascause, so a collision returned 500 instead of 409; it now walks the bounded cause chain.Left out on purpose: no dashboard UI consumes any of this yet, and companies, deals and activities are v2–v4. Each is shippable alone on top of this.
The new safety assertions were mutation-checked — disabling the consent signature pinning fails exactly the forgery test, letting
requireTeamRolefall through to API-key auth fails exactly the key-rejection test, and removing therequireCrmgate fails exactly the two 501 tests. The forged payload in that first test is deliberately complete (right site, right tier, uid-present, victim's hash) so that every non-cryptographic field check passes and only the crypto can reject it; a sparser payload would have passed with the pinning disabled.pnpm lint,pnpm typecheck,pnpm testgreen: 1537 tests across 172 files, up from 1499/170.Two defects found in self-review after the first commit, both in code this PR introduced, both fixed with a mutation-checked test on top.
SQLite ignores backslash escapes in
LIKEunless anESCAPEclause is present. The contact search prefixed metacharacters and stopped there, so\%was read as a literal backslash followed by the wildcard. Nothing matched — no name contains a backslash — which made the guard look correct while quietly making a literal%unsearchable. The covering test asserted only thatq=%did not return everything, so it passed either way. It now seeds a contact whose name actually contains a%and requires that row back: matching neither everything nor nothing.events_truncatedcompared against the wrong count. WhencontactActivitywas moved onto the codebase's canonical pageview/custom-event split, this call site was not re-traced, so the export's cap flag compared returned rows against custom events only. A contact whose traffic is entirely pageviews — the common case — has zero of those, so the flag readfalsewhile a thousand rows were dropped. That is a subject-access request answered incorrectly, which is the one thing the flag exists to prevent. Now compared against the row total, covered by a test that seeds past the cap with pageviews specifically, since custom-event traffic would have passed either way.