feat(auth): rotate api_token_pepper when blocking a user#4314
Conversation
|
Addressed [PR4314-1] in 6497112.
Preserved:
Added |
|
Two notes: [PR4314-1] is already addressed — that re-review looks stale. It cites Re: using So I added an opt-in Tests updated: |
Blocking a user previously set blocked_reason/blocked_at but left api_token_pepper unchanged, so already-issued API tokens kept a valid pepper and continued to be accepted by services that validate the pepper against the DB (e.g. KiloClaw) until natural expiry (~5 years for device-auth tokens). Add a central blockUser helper that sets the blocked fields AND rotates api_token_pepper, guarded by isNull(blocked_reason) so an existing block is never overwritten and callers can detect the unblocked->blocked transition. Route the scattered block sites through it: Stripe disputes, kilo-pass duplicate-card, kilo-pass cancel-and-refund, Stytch autoban, and the admin updateBlockStatus mutation. Rotate the pepper per-row in bulkBlock via gen_random_uuid() so each blocked user gets a distinct pepper. Blocked users' tokens are now revoked on every pepper-checking service.
markijbema
force-pushed
the
mark/block-rotates-pepper
branch
from
14b4568 to
71ae194
Compare
markijbema
left a comment
markijbema
left a comment
There was a problem hiding this comment.
LLM-generated review comment.
Findings:
-
PR4314-F1 High —
apps/web/src/app/admin/api/backfills/block-blacklisted-domains/route.ts:99still blocks users by directly settingblocked_reason,blocked_at, andblocked_by_kilo_user_idwithout rotatingapi_token_pepper. It only revokes gateway grants atapps/web/src/app/admin/api/backfills/block-blacklisted-domains/route.ts:119, so users blocked through this active admin backfill keep any existing pepper-valid API/device tokens. This should route through the same rotation behavior or addapi_token_pepper: gen_random_uuid()::textin the guarded update. -
PR4314-F2 High —
apps/web/src/lib/user/block.ts:37intentionally skips already-blocked users, andapps/web/src/routers/admin-router.ts:524ignores the helper’sfalseresult. That preserves old peppers for users who were already blocked before this PR, leaving the exact long-lived-token exposure described in the PR for the existing blocked cohort. A one-shot/backfill rotation for currently blocked users, or an explicit preserve-reason-but-rotate path, is needed if this PR is meant to close the revocation gap for existing blocks. -
PR4314-F3 Medium —
apps/web/src/lib/abuse/bulkBlock.ts:54validates “not already blocked” in a separate read, but the update atapps/web/src/lib/abuse/bulkBlock.ts:64is not guarded withisNull(kilocode_users.blocked_reason). If another block lands between the read and update, bulk block overwrites the original reason/actor/time and rotates the pepper again; laterunblockBulkBlockedUserscan then clear the wrong block group. Add theblocked_reason IS NULLpredicate to the update and treat a short update count as a conflict/retryable failure.
No automated tests were run; this was a code review only.
…4-F1) This admin backfill blocked users without rotating api_token_pepper, so users blocked through it kept pepper-valid API/device tokens. Add per-row gen_random_uuid()::text rotation to the guarded update. Extract the batch into backfillBlockBlacklistedDomainsBatch so it is testable without auth mocking, mirroring the blocked-at backfill.
The 'already blocked' filter runs in a separate read, so a concurrent block landing between the read and the update could overwrite the original reason/actor/time (and re-rotate the pepper) — which unblockBulkBlockedUsers could later clear as the wrong group. Add isNull(blocked_reason) to the update WHERE and surface a short update count as a retryable conflict listing the skipped users.
…-F2) Block rotation only fires on the unblocked->blocked transition, so users blocked before this change keep their original (usually null) pepper and their existing tokens stay valid. Add a batched admin backfill, mirroring the blocked-at backfill, that assigns a fresh gen_random_uuid()::text to users with blocked_reason set and a null api_token_pepper. Non-null peppers are excluded since they already reflect a prior rotation (block, admin reset, or soft delete).
|
Addressed all three review findings as atomic commits:
Note on scope of F2: the predicate is Verification: 17 tests across the three areas pass; oxlint clean; full |
…314-F2) Wire the blocked-user-pepper backfill into /admin/backfills with a count badge + batched run button, mirroring the existing BlockedAt backfill card so the new route is actually runnable from the admin UI.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Previous Review Summary (commit 7c6238f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7c6238f)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (18 files)
Reviewed by gpt-5.4-20260305 · Input: 38.6K · Output: 5K · Cached: 182.8K Review guidance: REVIEW.md from base branch |
…lt (PR4314-W1) cancel-and-refund read the user outside the transaction and then keyed the admin note, user.blocked event, and alreadyBlocked result off that stale blocked_reason snapshot while ignoring blockUser's return. Under a concurrent block landing between the read and the guarded update, it would falsely record 'Account blocked.', emit user.blocked, and return alreadyBlocked: false. Capture didBlock from blockUser and drive the note/event/result from it (matching disputes.ts).
|
Thanks for the review. Status on the two warnings:
|
2 participants
Summary
Blocking a user previously set
blocked_reason/blocked_atbut leftapi_token_pepperunchanged. Since the pepper is the only per-user revocation lever that edge services check against the database, a blocked user's already-issued API tokens kept a valid pepper and continued to be accepted by pepper-validating services (e.g. KiloClaw) until the token's natural expiry (up to ~5 years for device-auth tokens).This change makes blocking a user revoke their tokens everywhere by rotating the pepper as part of the block:
blockUserhelper (apps/web/src/lib/user/block.ts) that setsblocked_reason/blocked_at/blocked_by_kilo_user_idand rotatesapi_token_pepper, guarded byisNull(blocked_reason)so an existing block is never overwritten. Returns whether this call actually transitioned the user to blocked, and accepts an optionaldbOrTxso it composes inside existing transactions.SMART_RATE_LIMIT_BANNEDautoban.bulkBlockUsersto rotate the pepper per-row in SQL (gen_random_uuid()::text) so each blocked user gets a distinct new pepper rather than a shared one.This is Workstream B of a larger device-auth / token-revocation hardening effort. It is independent and safe to ship on its own; its benefit is fully realized on services that validate the pepper against the DB.
No schema change (the
api_token_pepper,blocked_*columns already exist), so no migration. No new PII, so no GDPR soft-delete changes.Verification
apps/web/src/lib/user/block.test.tscovering: blocks + rotates pepper; defaultsblocked_byto null; does not overwrite an existing block and leaves the pepper untouched; returns false for a non-existent user; runs inside a provided transaction; and rolls back (block + rotation) when the surrounding transaction throws.bulkBlockUsers(each of 4 users gets a distinct, freshly-rotated pepper).user/block,abuse/bulkBlock,stytch,stripe/disputes,kilo-pass/cancel-and-refund,kilo-pass/stripe-handlers-invoice-paid— all green.Visual Changes
N/A
Reviewer Notes
blocked_*and do not depend on the pre-block pepper.db.transactionnow delegate toblockUser({ ..., dbOrTx: tx }); the dispute path keeps itsSELECT ... FOR UPDATEfor locking and admin-note logic and now derivesdidBlockfrom the helper's return value.bulkBlockrotates per-row in SQL on purpose — a single JSrandomUUID()in a bulkUPDATEwould assign the same pepper to every blocked user.