security(server): make an issued session token withdrawable - #45
Merged
Conversation
A session was an HMAC-signed bearer token with a 30-day expiry and no server-side state, so nothing could end one early. /logout deleted a cookie in the browser that asked and left a copied token valid for the rest of its life. The CRM audit log made that concrete: it can now show an admin exactly which operator session read the contact table, and there was no lever to stop it. users.session_epoch is that lever. Every token carries the epoch it was signed at, a session resolves only while the two still match, and incrementing the column ends every outstanding session for that person at once. A counter rather than a timestamp: two revocations in one millisecond are two revocations, and a clock that steps backwards must not resurrect a session. The increment is done in SQL rather than read-then-write, so two racing revocations both land instead of the second overwriting the first with the same value — which would report success while leaving the sessions it was called to kill alive. sessionUser() is now the only way to resolve a session. Both halves live in it because a signature check not followed by an epoch check silently restores the old unrevocable token, at whichever route forgot, which is the failure nobody notices. verifySession stays pure and exported for what it is: proof the token was issued here and has not expired, never proof the session is live — the same split consent.ts draws between verifyConsentRecord and findActiveConsent. It costs no round trips. sessionUser returns the row it read, so /api/auth/me spends the one query it always did, and siteRole is now a single joined statement instead of two sequential reads, which pays for the epoch read on every RBAC path. The join also expresses "an unowned site grants nobody anything" directly: a NULL team_id matches no membership row, so that case no longer needs its own branch. A token with no epoch claim is rejected rather than read as epoch 0. It cannot be compared against a revocation, and an unverifiable revocation state has to mean revoked; the price is one forced sign-in at deploy. Deleting a user now also ends their sessions, since an account that does not exist holds none — a test asserting the opposite was rewritten, because it pinned the behaviour this replaces. POST /api/auth/logout-everywhere is the self-service control. It is deliberately all-or-nothing: with no session table there is no device list to revoke from, and the honest control is the one that ends everything, which is what someone reaching for it wants anyway. Also, autonomously: dropped two imports in routes/auth.ts left unused by the /me rewrite.
…tion /audit-file on lib/accounts.ts. The three inserts that bootstrap an account ran as separate statements, so a failure between them left a user row with no team — and permanently, because every later login takes the `existing` branch and returns without ever looking for a membership. That operator sees no teams from /api/auth/me and siteRole answers null for every site, while the users table looks perfectly healthy. One batch, which for D1 is one transaction. Same reasoning and same remedy as deleteCompany, which already batches for exactly this reason. The covering test pins the invariant rather than the statement count: a refactor that drops the membership insert fails it. Mutation-checked by removing that insert, which fails it and three existing tests.
POST /api/users/:id/revoke-sessions, behind ADMIN_TOKEN. Deferred by the accounts.ts audit and worth closing now: revocation existed only as /api/auth/logout-everywhere, so the only person who could end a session was the operator holding it — precisely the wrong person when the question is whether that session was stolen. The CRM audit log names the operator; this is the lever it points at. Behind the admin token rather than a team role deliberately. Team admins have no user-management surface at all today — they cannot list their members, rename them or remove them — so a route reaching across to another person's sessions would be the first of its kind, arriving without any of the structure that should come with it. 404 rather than a cheerful 200 for an unknown id, so a typo is never reported as a revocation that did not happen. Idempotent: revoking twice is two epochs and the same outcome. Documented both revocation routes in docs/api.md, including the two consequences an operator will actually meet: pre-upgrade tokens carry no epoch and are rejected, so everyone signs in once; and deleting a user ends their sessions, since an account that does not exist holds none.
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.
A session was an HMAC-signed bearer token with a 30-day expiry and no server-side state, so nothing could end one early.
/logoutdeleted a cookie in the browser that asked and did nothing to a token already copied out of it. #43 and #44 made that concrete rather than theoretical: the CRM audit log can now show an admin exactly which operator session read the contact table, and there was no lever to pull.users.session_epochis the lever. Every token carries the epoch it was signed at, a session resolves only while the two still match, and incrementing the column ends every outstanding session for that person at once. A counter rather than a timestamp, because two revocations in the same millisecond are two revocations and a clock that steps backwards must not resurrect a session. The increment is done in SQL rather than read-then-write, so two racing revocations both land instead of the second overwriting the first with the same value — which would report success while leaving alive the sessions it was called to kill.sessionUser()is now the only way to resolve a session, and both halves live inside it. A signature check not followed by an epoch check silently restores the old unrevocable token, at whichever route forgot, which is the failure nobody notices.verifySessionstays pure and exported for exactly what it is — proof the token was issued here and has not expired, never proof the session is live — the same splitconsent.tsdraws betweenverifyConsentRecordandfindActiveConsent. Verified by grep oversrc/:verifySessionhas exactly one caller.It costs no extra round trips, which is the reason this design was picked over a session table.
sessionUserreturns the row it read, so/api/auth/mespends the one query it always did; andsiteRoleis now a single joined statement instead of two sequential reads, which pays for the epoch read on every RBAC path. The join also expresses "an unowned site grants nobody anything" directly — a NULLteam_idmatches no membership row — so that case no longer needs its own branch, andadmin.test.tsalready pinned it.Two consequences an operator will actually meet, both deliberate. A token carrying no epoch claim is rejected rather than read as epoch 0: it cannot be compared against a revocation, and an unverifiable revocation state has to mean revoked, so everyone signs in once after this deploys. And deleting a user now ends their sessions, since an account that does not exist holds none — a test that asserted the opposite was rewritten rather than worked around, because it pinned the behaviour this replaces.
POST /api/auth/logout-everywhereis the self-service control, all-or-nothing by design: with no session table there is no device list to revoke from, and the honest control is the one that ends everything.POST /api/users/:id/revoke-sessionsis the same call applied to someone else, behindADMIN_TOKEN— that is the lever the audit log points at, since the only person who could otherwise act on a suspicious entry was the operator named in it. It stays a deployment-operator action rather than a team-admin one because team admins have no user-management surface at all today, and a route reaching across to another person's sessions would be the first of its kind, arriving without any of the structure that should come with it./audit-fileon the auth kernel found one unrelated defect worth fixing and it is in this branch.upsertUserByEmailwrote its three bootstrap rows as separate statements, so a failure between them left a user with no team, permanently — every later login takes theexistingbranch and returns without ever looking for a membership, so nothing repairs it while the users table still looks healthy. Now onebatch(), which for D1 is one transaction, matching whatdeleteCompanyalready does for the same reason. Eight other candidates were refuted against the corpus and are listed in the audit rather than filed: thesiteRolejoin returning an arbitrary row (memberships is keyed on(team_id, user_id)), the dropped null-team guard (already covered byadmin.test.ts:95), a timing oracle on the epoch (a counter is not a secret), and CSRF on the new POST (sameSite: Laxmeans a cross-site POST carries no cookie).What the risk is not: no change to ingest, to the API-key path, or to any analytics table. The migration adds one column with a default, so existing rows read as epoch 0 and existing users are simply asked to sign in again.
verifySessionis unchanged as a cryptographic check — the new rejection is a missing claim, not a stricter signature.Every new assertion was mutation-checked against a committed baseline: the epoch comparison removed, the epoch claim check removed, the increment made a no-op,
requireAdmindropped from the new route, and the membership insert dropped from the bootstrap batch — each time confirming exactly its covering tests fail, then restored.pnpm lint,pnpm typecheck,pnpm testgreen: 1625 tests across 176 files, up from 1615.