feat(dashboard): make the CRM access log readable, and stop it hiding its horizon - #44
Merged
Conversation
… its horizon The log had no reader. It also answered a paged list with no statement of what the page could not contain, which is the defect this fixes on the server side. THE HORIZON. The audit log is the one CRM table that ages out, so an empty result means either that nothing happened or that it happened too long ago — and the rows alone cannot tell those apart. An auditor reading the first when the second is true has drawn exactly the wrong conclusion from an access log. `GET /api/crm/audit` now returns `retention_days` and `covers_since` with every page, and the panel renders "nothing before X is retained". This is the same rule the rollup cap and the export cap already follow: never a silent truncation, by count or by time. `covers_since` is the guarantee rather than the oldest row, since purging runs hourly and slightly older entries may still be present. THE ACTOR. Entries store `actor_user_id` because it is stable and leaves no email behind once an account is closed, but "operator 8f3a1c…" is a record, not accountability. The response resolves it to `actor_email` at read time, so the name follows the account rather than being frozen at write time, and is null once the account is gone — the id stays either way, so an entry always names someone specific. This is a deliberate disclosure: it tells a team admin the addresses of the colleagues who read this site's contacts, which no other session-reachable route does. Everyone who can appear holds a role on that admin's own team, because that is what authorized the access being reported. The resolve chunks at D1_MAX_IN_PARAMS. One page can name at most 100 distinct actors and D1's ceiling is exactly 100 bound parameters, so a single lookup would sit precisely on the cliff and stay correct only while two unrelated limits keep their current relationship. THE PANEL states the other thing the rows invite a reader to get wrong: an entry records that an operator was authorized to do this, not that it succeeded. A run of reads against ids that do not exist is someone probing, not someone being shown anything. Erasures and exports are weighted so they do not read as more list traffic. `target_id` is shown raw and never resolved to a name — the log holds no contact fields and after an erasure the id points at nothing, so the honest offer is "filter by this id", not a label the log cannot stand behind. From a contact or company, "Access log" opens the log filtered to that record — "who has looked at this person" is the question a subject-access request or a suspected leak asks, and a whole-site log answers it only by being read end to end. Also, autonomously: `CrmAccessNotice` takes a per-surface 403, because the shared wording names `analyst` and says the reader has no CRM access, both wrong for a surface gated on `admin` — a refusal that states the wrong requirement sends the reader to ask for a role that would not have helped. Every CRM mutation now invalidates the log, since every one of them writes an entry.
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.
#43 gave the contact store an access log with no reader, and — reviewing it while building one — a defect: the response paged a table that ages out and said nothing about the horizon. An empty result meant either "this never happened" or "it happened more than a year ago", and the rows could not tell them apart. An auditor concluding the first when the second is true has drawn exactly the wrong conclusion from an audit log. That is the same class of error #42 fixed for the rollup cap and the export cap; it should never have shipped by time when it is refused by count.
GET /api/crm/auditnow returnsretention_daysandcovers_sincewith every page, and the panel renders "nothing before X is retained".covers_sinceis the guarantee, not the oldest row — purging runs hourly, so entries a little older may still be present; it is the point before which nothing can be relied on to still be there.An entry stored
actor_user_idand nothing could resolve it. That is the right column to store — stable, and it leaves no email behind once the account is closed — but "operator 8f3a1c…" is a record rather than accountability, and an audit log nobody can act on is decoration. The response resolves it toactor_emailat read time, so the name follows the account instead of being frozen at write time, and comes backnullonce the account is gone. The id stays either way, so an entry always names someone specific and the panel falls back to it rather than rendering a blank. This is a deliberate disclosure and is documented as one: it tells a team admin the addresses of the colleagues who read this site's contacts, which no other session-reachable route does. It is bounded to exactly those people — everyone who can appear in the log holds a role on that admin's own team, since that is what authorized the access being reported.The resolve chunks at
D1_MAX_IN_PARAMS. One page can name at most 100 distinct actors and D1's ceiling is exactly 100 bound parameters, so a single lookup would sit precisely on the cliff and stay correct only while two unrelated limits keep their current relationship. The covering test builds the largest page that can ever exist — 99 seeded actors plus the reader's own entry — and asserts all 100 resolve.The panel states the other thing the rows invite a reader to get wrong. An entry records that an operator was authorized to do this, not that it succeeded: the record is written before the request runs, which is what makes an unrecorded access impossible, so a lookup that then found nothing looks identical to one that returned a record. A run of reads against ids that do not exist is someone probing, not someone being shown anything. Erasures and exports are weighted so they do not read as more list traffic.
target_idis shown raw and never resolved to a name — the log holds no contact fields by design and after an erasure the id points at nothing, so the honest offer is "filter by this id", not a label the log cannot stand behind.From a contact or a company, Access log opens filtered to that record. "Who has looked at this person" is the question a subject-access request or a suspected leak actually asks, and a whole-site log answers it only by being read end to end. The filtered view also says what survives an erasure, because that is exactly when it gets asked.
CrmAccessNoticegained a per-surface 403. The shared wording namesanalystand says the reader has no CRM access — both wrong for a surface gated onadmin, and a refusal that states the wrong requirement sends the reader to ask for a role that would not have helped. Every CRM mutation now invalidates the log, since every one of them writes an entry.What the risk is not: no schema change and no migration —
covers_sinceis computed, not stored; nothing new is recorded; and a deployment withoutCRM_DBstill gets501before anything else, so the new tab renders the same "not installed" panel every other CRM surface does. The only behaviour change on an existing route is two extra fields and a resolved email on a response that admins alone can reach.Every new assertion was mutation-checked: the horizon dropped from the response and from the panel, the actor left unresolved, the id fallback removed, the per-surface 403 removed, the target filter stopped reaching the query, and the retention floor loosened — each time confirming exactly its covering test fails, then restored. One process note: the first mutation pass used
git checkout --on a file with uncommitted work and silently reverted real changes, which is why the second pass ran against a committed baseline. Restored and re-verified before this branch was pushed.pnpm lint,pnpm typecheck,pnpm testgreen: 1615 tests across 176 files, up from 1601.