mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
Zachary Lowden
f196799338
* fix(app-blocks): a publisher ban now revokes their live block instances
Banning a publisher already unpublished their models, cancelled the
subscription, blocked their media and invalidated their sessions — but it
wrote none of the three markers the block-token runtime guards read, so
every block token their apps already held kept authenticating against the
REST wrapper and the tRPC bridge until its natural exp (900s default,
14400s for a dev token). That residual, and only that residual, is what
this closes.
Adds the third production writer of BlockRevocation.revokeInstance:
revokeBlockInstancesForPublisher (blocks/publisher-ban-revocation.service.ts),
called from toggleBan's ban fan-out alongside the model unpublish and the
media block. It marks every live instance of every block the banned user
OWNS (app.userId).
Deliberately narrow, both ways:
- Owner only, never a seated collaborator. Widening it would let a ban on
one account revoke the live tokens of an app owned by another account
that was not banned. Recorded in the app-ownership gate ledger.
- No enabled filter. A disabled install's earlier marker is TTL-bound and
may have lapsed; re-marking costs one Redis SET and only narrows exposure.
- The unban branch clears nothing. The markers expire with one token
lifetime and re-minting is the recovery path.
- isRevoked still fails OPEN on a Redis error — untouched, and now pinned
by a live assertion rather than left to a diff review. Inverting it would
refuse every block during a Redis incident; that is a separate decision.
The guard comment at block-scope.middleware.ts is cited by five other files
as the authority on this behaviour and said a ban writes none of the three,
so it moves with the code — along with block-revocation.service,
block-bridge-auth.service, apps.router, apps-shared.router,
scope-grant.service and the bridge-token guard test's header.
Closes clawgate #618.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): reach all five instance-id namespaces, and make a ban marker unclearable
Review round 1 (civitai-review, five lanes) found the first cut of this writer was
wrong in the direction its own comments denied.
1. A blockInstanceId is not always a stored column. Four of the five namespaces are
SYNTHESISED: bus_pub_<busId> and bus_view_<busId> off a blanket subscription's
row id, pdb_<appBlockId> and page_<appBlockId> off the app block. The original
`where: { blockInstanceId: { not: null } }` reached ONE of five, and its comment
justified that with "a blanket subscription has no minted token keyed to a stored
one" — true, and irrelevant: the token is keyed to the synthesised id and
isRevoked compares claims.blockInstanceId verbatim. publisher_all_my_models
blanket is the publisher's own default install shape, so the most likely case was
the one it missed. The writer now enumerates subscriptions AND owned app blocks
and emits all five.
Pinned by a new seam guard, publisher-ban-revocation.namespaces.test.ts, against
deriveScopeFromInstanceId — the canonical parser. Fails on GROWTH (the parser
learns a prefix the writer does not emit) and on SHRINK/typo (the writer emits one
no token carries). Four mutations watched red, each on its own assertion.
2. A ban marker was clearable by a third party. clearInstance is called
unconditionally by toggleEnabled(true) and installOnModel, both driven by the
install's CONSUMER — the model owner, a different and un-banned account — and
blockInstanceId survives a disable. Toggling off and on undid the moderation
action. The marker now carries its cause as its value ('install' | 'ban'; the
legacy '1' reads as install) and clearInstance refuses a ban marker. It fails
CLOSED on a read error, the opposite of isRevoked and deliberately so: an
un-cleared marker expires within one token lifetime, a wrongly-cleared ban marker
needs a second moderator action.
3. Three mutations survived the first test suite, all found by running them:
- the Redis fake resolved in a microtask, so dropping the await on the fan-out and
detaching the leg from toggleBan's awaited Promise.all both passed. AC-1 is a
happens-before claim and the fake could not express it. Every fake now yields a
macrotask tick; both mutations are killed.
- "wrote a marker for each id AND FOR NO OTHER" filtered the key set to the
fixture's own ids, so over-revocation was invisible — the exact hazard the
ownership filter exists to prevent. It now asserts the whole key set plus the
call count; a mutant revoking a foreign id is killed.
- mint() hardcoded one appBlockId, so the fixture's "across two blocks" claim was
not exercised. It now carries each row's own.
Also: the chunk-and-await loop is replaced by limitConcurrency (the repo's helper) —
the old shape was a barrier, not the ceiling its own docblock described; the returned
count is now logged rather than discarded; and a call-site ledger pins the "exactly
three production call sites" sentence, which five files restate and which those same
comments record as having already been wrong twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): split the ban keyspace, route ownership canonically, make revocation observable
Audit round 1 findings F1-F5, plus the operator-approved counter. One commit so round 2
audits a single delta.
🔴 F1 (deploy-blocker) — the cause-on-one-key design did not hold. `toggleEnabled(false)`
calls `revokeInstance` with no cause, and the write was unconditional, so an ordinary
un-banned model owner disabling a banned publisher's install DOWNGRADED the ban marker to
`install`; `toggleEnabled(true)` then cleared it and the pre-ban token was served again.
`clearInstance` had been hardened; the write in front of it had not.
Fixed STRUCTURALLY rather than with a value guard: ban markers get their own Redis
keyspace (`blocks:revoked-instance-ban`) written by a separate method,
`revokeInstanceForBan`. A read-then-set would have been a read-modify-write with a race
in it, and this wrapper exposes no value-guarded SET; separate keys make the downgrade
UNREPRESENTABLE — the install path cannot name the key. `isRevoked` checks both in ONE
round trip via `mGet`, so the per-request cost claim in block-bridge-auth stays true.
`clearInstance` addresses the install keyspace only and needs no branch at all.
And the guard that hid it: the consumer-re-enable test modelled `clearInstance` ALONE,
omitting the `revokeInstance` leg the real pair always runs first, so it was green in both
arms. It now drives the REAL `BlockRegistry.toggleEnabled(false)` then `(true)` pair, with
a positive control that an UNBANNED publisher's install still restores.
🟡 F2 — "re-minting is the recovery path" was false, including in this card's AC-3.
Instance ids are stable across a re-mint, so after ban→unban a freshly minted token was
still 403 for up to 14400s with no product-level remedy. The unban branch now clears the
ban keyspace for that publisher's instances, through the SAME enumeration the ban used
(a clear addressing fewer ids is no remedy). Install markers are untouched, so lifting a
ban cannot silently re-enable an install its own consumer switched off. The three comments
and `block-registry.service.ts`'s re-enable comment now agree.
🟡 F3 — `page_` is three mint shapes, not one. Added `page_pubreq_<publishRequestId>` via
`appBlockPublishRequest` (submittedByUserId + pending, mirroring the mint). `page_local_*`
is DOCUMENTED as uncoverable: it exists precisely because no server row ties the slug to a
user, so there is nothing to enumerate — closing it needs a different mechanism, not a
wider query.
🟡 F4 — the seam guard pinned `deriveScopeFromInstanceId`, which its own docblock calls the
client-side path, while the mint dispatches on `BlockRegistry.resolveBlockInstance`. Adding
a 6th prefix to the resolver alone left it green 18/18. It now pins the resolver, the
client parser, AND the two against each other; that same mutation is killed by two
assertions. The call-site ledger is split per writer, so rewriting a ban site to call the
install writer is a red test.
🟡 F5 — settled by enumeration, not assumed. `BlockTokenService.sign` has exactly two
non-test call sites and NO mint path reads `AppListing` or a listing `kind` at all, so an
offsite-owned block CAN mint: mintability and canonical ownership are decided by disjoint
column sets. Ownership now routes through `resolveCanonicalListingOwner` as a three-branch
predicate. Two paths produce the divergence, and only one is a mod action —
`acceptTransfer` moves `OauthClient.userId` only under `isOnsite`. The "offsite 5 rows, 0
with a block" production count is recorded as the stale empirical claim it is, not as a
justification. Pinned by an executable branch-for-branch equivalence test against the real
resolver; dropping branch 1 (listing-less blocks, most of the fleet) and reverting to
`app: { userId }` are both killed on the right rows.
OBSERVABILITY (operator-approved, pre-existing gap) — the revocation 403 returns before
`recordScopeInvocation` registers its `res.on('finish')`, so it could never write a
`block_scope_invocations` row and the mechanism's firing was unfalsifiable. Adds
`civitai_app_block_revocation_refusals_total{surface,namespace}`, emitted from both guards.
The namespace label is bounded and would have made F3's gap readable — `page_pubreq_` and
`page_local_` are bucketed before `page_`, which is the collapse that hid them.
Not in scope, deliberately: ban durability beyond one token lifetime is clawgate #620 and
is layered on these markers rather than replacing them (the status flip is replica-read and
lag-delayed; these kill a live session at Redis speed). The two eventloop-watchdog timing
failures are pre-existing on origin/main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): cover the ephemeral + mod-review page shapes, guard the ban TTL, correct three false claims
Audit round 2: four 🟡 and one 🟢. One commit so round 3 audits a single delta.
🟡1 — the `page_` enumeration was incomplete and the narrowing was false. It is FIVE mint
shapes, not three, and TWO more were uncovered, both `dev:true` (4h):
- `page_ephemeral-<blockId>` (block-tokens/index.ts) — an unsubmitted app over a live
dev tunnel, held by the AUTHOR: precisely the publisher this control exists to cut
off. Filed as uncoverable last round; that was wrong. An ephemeral app has no
AppBlock row, but a live tunnel IS server state. dev-tunnel.service.ts now maintains
a per-user SET of tunnelled blockIds (`listActiveDevTunnelBlockIds`) so a ban can
enumerate them — `userBlockKey` can only answer "is THIS pair live", and the
alternative was a cluster-wide SCAN inside a fan-out Bulk Ban multiplies. Index
writes are try/catch, not a trailing `.catch()`: a missing method throws
SYNCHRONOUSLY and would otherwise take a developer's tunnel start with it — which is
exactly how the dev-tunnel suite broke the moment those lines were added.
- `page_<pubreq_ULID>` (publish-request.service.ts) — the MOD review preview, SINGLE
`pubreq_`, so dev-token's double-prefixed spelling never matched it. Both spellings
are now emitted from the same pending rows.
`page_local_<slug>` remains genuinely uncoverable and is documented as a statement about
the mechanism: no server row of any kind ties that slug to a user. Both the writer's and
the middleware's shape claims are corrected; the middleware no longer restates a count.
And the seam guard's structural blindness is closed rather than just confessed. Every
prefix ledger is blind to a new mint surface that REUSES an existing prefix —
`deriveScopeFromInstanceId('page_ephemeral-foo')` returns `viewer_global` via the bare
`page_` branch — which is how `page_` came to be documented as one shape when it is five.
Added a MINT-SITE ledger over the four files that CONSTRUCT a blockInstanceId, with each
one's coverage. Verified: a sixth `page_` shape added to an unledgered file leaves all
prefix assertions green and fails the mint-site ledger. Its residual (an EXISTING file
growing a new shape internally) is stated in its docblock rather than left implied.
🟡2 — a surviving mutant on a safety-critical invariant. The TTL suite probed only
`revokeInstance`, so `revokeInstanceForBan`'s EX was guarded by nothing: mutating it to
900 stayed green across 5 files / 116 tests while a banned publisher's dev token lives to
14400s — silently un-revoked from T+900s. `markerTtlSeconds` is now parameterised over
both writers as a cross product with the token kinds, plus a ledger that reads the
service's own `static async revoke*` surface so a THIRD keyspace cannot ship unprobed the
way the second did.
🟡3 — "ONE ROUND TRIP, NOT TWO" was false and is deleted, not repaired. This repo's client
wraps `mGet` into `Promise.all(keys.map(get))` to avoid CROSSSLOT, so the array path never
reaches the native MGET: it is two GETs. Wall-clock is likely unchanged (same tick,
pipelined) but the COMMAND RATE against the cache cluster is doubled on every REST and
bridge request. Reworded here and in the four stale "ONE Redis GET" lines in
block-bridge-auth. The read logic itself is untouched — it was verified correct.
🟡4 — the new counter had zero test coverage while its own docblock called the branch
order load-bearing. Added a suite pinning each real minted shape to its label plus a
structural check that no prefix is tested after one it extends. Round 2 could not run this
mutation; it runs now and is killed by four assertions.
🟢5 — an inverted empirical claim: "most app blocks predate W13 and have no listing row"
is 23-of-24 the other way. The conclusion (keep branch 1) is unchanged — that single
listing-less block is the entire reason — but the magnitude is corrected and labelled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): subject-scope the ephemeral ban marker, make the index write atomic and owned
Audit round 3: four 🟡, two 🟢, plus one out-of-range stale sentence. One commit so round 4
audits the delta from b3d0e22f5a.
🟡 R3-2 — A BAN 403'd AN INNOCENT AUTHOR. `page_ephemeral-<slug>` is the one instance id
that is NOT globally unique: the slug is developer-chosen, `resolveEphemeralDevPageBlock`
checks it only against AppBlock rows and pending requests (never against tunnels), and
`startDevTunnel` enforces uniqueness per (user, blockId) ONLY. Two authors can hold live
tunnels on the same unclaimed slug and both mint `page_ephemeral-demo`; banning one wrote
a GLOBAL marker that refused the other's own dev tunnel for up to 4h. Ban markers for this
shape now go to a SUBJECT-SCOPED keyspace and `isRevoked` consults it with the token's own
`sub`, so the ban lands on the holder it names. Every other id is derived from a unique row
id and stays global — those tokens are legitimately held by many viewers and all of them
must be refused. This also makes the reaper's orphan branch benign: it has no session
record to read a userId/blockId from and so cannot sRem, but a stale member now yields a
marker only under its own owner's key. The docblock that claimed the id "carries no other
user's token" is replaced by what is actually true.
🟡 R3-1 — took the helper. `sAdd` + `expire` in one swallowing catch is the racy pair
`sAddWithExpireGe` was written to replace; its own docblock says the EVAL form means a
crash cannot land SADD without EXPIRE. The failure it prevents is documented on this exact
client (sys-inflight.ts, 2026-07-03): a sentinel failover landing one and dropping the
other leaves a TTL-less set accumulating every blockId the user ever tunnels, so every
later ban emits a marker for all of them — and the index's own TTL guarantee would have
been false in that state. Kept the try/catch (not `.catch()`): a missing method throws
synchronously and must not take a tunnel start with it.
🟡 R3-3 — the mint-site ledger was walkable three ways, all now closed, each with a matched
pair: (a) it walked `.ts` only, while the REAL constructors are `.tsx` — so the very shape
this PR covered was minted in a file the population excluded; (b) MINT_RE matched
`PAGE_INSTANCE_PREFIX}`, which in block-tokens/index.ts occurs only in `!==` COMPARISONS,
so that file was ledgered as constructing two shapes it does not construct; (c) it ran over
raw source, so one prepended comment satisfied it. Comments are now stripped through a
single shared helper — the ad-hoc second copy is how the discipline was lost. The residual
sentence now describes what ships instead of being three ways too narrow.
🟡 R3-4 — the index write was payload nothing could fail on: deleting it left 707 files /
12,806 tests green, because both sides were hermetically tested and the seam was owned by
nobody. Five tests now drive the REAL startDevTunnel/teardown pair through
listActiveDevTunnelBlockIds; deleting the block fails three of them. The suite's fake gained
an `eval` that EXECUTES the script's effect rather than returning a canned reply — a stub
would let the assertions pass with the index never populated.
🟢 R3-5 — `page_pubreq_` was tested before `page_pubreq`, making the mod-review branch
unreachable so both spellings printed the dev-token label. Discriminates on the second
`pubreq_` now. 🟢 R3-6 — the SHRINK/TYPO guard extracted with `[a-z_]+`, which cannot match
`page_ephemeral-`; with the hyphen it can.
Also: the third surviving copy of the corrected "three mint shapes" sentence, in the file
that defines APP_BLOCK_REVOCATION_NAMESPACES.
Two spelled guards over the call shape correctly went red on the new second argument and
are updated to require it — `isRevoked(claims.blockInstanceId, claims.sub)` — since
dropping the subject type-checks (the parameter is optional so a caller without one
degrades rather than breaks) and silently un-scopes the ban.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): pass the subject at all four guards, and make three claims true of the code
Audit round 4. Last pass of the ladder, so every claim below was re-checked rather than
carried.
🟡 F2 — the only one with a behavioural consequence. `isRevoked` has FOUR production call
sites, not the "both guards" the docblock claimed, and two of them — `apps.router.ts`
(resolveStorageContext) and `apps-shared.router.ts` (resolveSharedContext) — passed no
subject, so they read only the global keys while the ephemeral ban marker exists solely
under the subject key. Latent today (both require an `approved` AppBlock row and an
ephemeral app has none) but pointed exactly where this work keeps going: storage for
unsubmitted apps. All four now pass `claims.sub`; the sentence names all four and says why
omitting it is silent — it NARROWS the check rather than breaking it.
🟡 F3 — made the guard see sub-shapes rather than narrowing the sentence a third time.
Widening the extraction to `[a-z_-]` put `page_ephemeral-` into `emitted`, but the
assertion it fed was "does the parser recognise this prefix", and the parser dispatches on
`page_` — so `page_ephemral-`, a one-letter typo, stayed GREEN. The emitted set is now
compared as a SET against the shapes the writer is supposed to build, which fails on a
prefix that appears AND on one that vanishes. The typo mutant is red; the `pdb_` → `pdx_`
positive control still reds the parser check too.
🟡 F1 — the docblock named a round-trip suite that did not exist: `subjectForUserId`
appeared in two files, neither a test, and the only thing holding the spelling was a
hand-typed `:user:<id>:` literal in the ban suite. Wrote the suite it promised —
`subjectForUserId` through `parseSubjectUserId` and back, with a positive control that the
parser rejects spellings this function cannot produce — and pointed the sentence at it
while crediting the literal for what it does pin.
🟢 F6 — took it. The third GET is gated on `isSubjectScopedInstanceId`, so only
`page_ephemeral-*` pays it; everything else, and every anon subject, is back to two.
🔴 The predicate is ONE function shared by the reader and the ban writer, because the
drift it prevents is silent in the worst direction: a writer that scopes while the reader
does not look leaves an unreadable marker — a ban that refuses nobody with every key-level
assertion still green. The writer asserts the predicate agrees before using the scoped
bucket, so that divergence throws at the one place both sides meet.
🟢 F5 — the third stale round-trip count, in the copy I missed last round. Fixed, and
recorded that this number has now been wrong three times across three files: if it goes
wrong a fourth time the count should be deleted rather than corrected, since the ordering
argument it supports does not need it.
🟢 F4 — `evalTtls` was written, never read, under a comment promising TTL coverage. It now
asserts the floor the service passes, and its comment says what it cannot do (model the GE
comparison — the fake has no TTL clock).
One spelled guard in `apps.router.storage.test.ts` correctly went red on the new second
argument and is updated to require the token claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): correct the gate-ledger entry the canonical-owner routing invalidated
Post-merge reconciliation, kept separate from the round-4 fix commit so the diff is
legible.
Merging origin/main surfaced a claim of mine that a previous round's own fix had made
false, in the one shared file nobody flagged. `app-access.call-site-ledger.test.ts`'s entry
for the ban writer still said it "resolves no AppListing, so D5 does not apply" and that
off-site coverage "must go through resolveCanonicalListingOwner" as a future direction —
both written before round 2's F5, which routed the writer through exactly that resolver.
It also still carried "off-site apps mint no block token", the defence round 2 disproved by
enumeration: no mint path reads AppListing or a listing kind at all.
The entry now states what the code does — the three-branch canonical-owner predicate, why
claimListing and acceptTransfer both produce the divergence, and that branch 1 (no listing
→ app.userId) is load-bearing because dropping it would UNDER-revoke most pre-W13 blocks.
MERGE REVIEW, recorded because a clean merge is not a coherent one. The shared file the
merge actually touched is `metrics/app-block-runtime.metrics.ts`, where #4946 adds
`civitai_app_block_bridge_messages_total` and this branch adds
`civitai_app_block_revocation_refusals_total`. Read by eye and checked mechanically: 14
distinct metric names with no name registered twice, no const/function/type defined twice,
no duplicated interface member or return-object key, disjoint label sets, no shared help
text or prefix constant. main's new `block-token-access.service.ts` touches neither
`BlockRevocation` nor any blockInstanceId construction, so neither the revokeInstance
ledger nor the mint-site ledger changes.
Also regenerated the Prisma client: the merge brought `BlockAuthorFeeAccrual` and the stale
client failed typecheck on main's own file, not on anything here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@civitai/redis
Redis clients for Civitai apps: the cache client (cluster-capable) and the sysRedis client (system/coordination), with the production hardening baked in — socket-timeout teardown, cluster self-heal watchdog, routing retries, packed (msgpack+brotli) values.
Add to an app
// package.json
"@civitai/redis": "workspace:*"
Transpile (raw TS): Next transpilePackages: ['@civitai/redis'], Vite ssr.noExternal: ['@civitai/redis'].
Often you don't add this directly — it comes in via @civitai/auth. Add it explicitly when the app
builds its own client (caching, pub/sub, or to inject isRevoked into the auth guard).
Env
| Var | Req | Notes |
|---|---|---|
REDIS_URL |
yes | cache client connection |
REDIS_SYS_URL |
yes | sysRedis connection |
REDIS_CLUSTER |
no | true to treat REDIS_URL as a cluster |
REDIS_SYS_SENTINELS / _NAME |
no | sysRedis HA via Sentinel discovery |
loadRedisEnv() requires both REDIS_URL and REDIS_SYS_URL — a partial config throws. Dozens of
optional tuning knobs (socket timeouts, self-heal thresholds, routing retries) have safe defaults; see
src/env.ts.
Use
import { createRedisClients } from '@civitai/redis';
export const { redis, sysRedis } = createRedisClients();
// or just one: createCacheRedis() / createSysRedis()
redis exposes .packed.get/set (msgpack+brotli) alongside the standard commands. Key namespaces are
exported as REDIS_KEYS, REDIS_SYS_KEYS, REDIS_SUB_KEYS.
Gotchas
- Both URLs or neither: setting only one throws on first use. This is the #1 footgun for apps that enable redis just for the auth cache.
- The clients are HMR/global-cached and self-healing in the package; the app shim just calls the factory.
- For real-time auth revocation, pass a sysRedis-backed
isRevokedtocreateSpokeGuard.
Reference: the auth app (apps/auth) wires both clients.