mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
Zachary Lowden
4aab099c91
* fix(app-blocks): a no-user flag eval returns the flag's BASE value, not a deny
Several docblocks in app-blocks-flag.ts derived a security property from an
inference that does not hold:
no user -> a global eval that can never match a segment -> fail-closed
The premise is true. A no-user call reaches Flipt as entityId 'global' with an
empty context, and every identity/tier/cohort segment we have is a
STRING_COMPARISON_TYPE constraint that reads the context, so none can match. The
conclusion does not follow from it: when no rollout matches, Flipt answers with
the flag's own base 'enabled' value. The denial came from the BASE being false,
not from the segment miss — so a base-true widening of app-blocks-author or
app-blocks-enabled turns every no-user branch from a deny into a pass.
MEASURED against the real @flipt-io/flipt-client-js wasm engine over a real
evaluation snapshot (new test app-blocks-flag.base-enabled-flip.test.ts):
base enabled:true + a non-matching SEGMENT_ROLLOUT, no entityId/context -> true
base enabled:false + the same rollout, no entityId/context -> false
unknown flag key, no entityId/context -> false
Both flags are base-false with segment rollouts today (civitai/flipt-state,
civitai-app/default/features.yaml), so nothing is exposed now. This is the latent
gate closed before any base-true flip, not a live defect.
Code:
- isAppBlocksAuthorEnabled: an undefined user now returns false structurally
instead of falling through to a global eval. Enumerated: all 10 call sites pass
{ user }; none wants a global eval of this key.
- blocks.router assertAppBlocksEnabledForTokenUser / assertViewerIsAppDeveloper:
refuse an unhydratable token subject before consulting the flag, with a distinct
message. Same shape apps.router.ts already uses.
- apps-shared.router resolveSharedContext: separate a VANISHED subject (refuse)
from an ANON token (keep the global eval — that widening is intended). The
read ops list/get had no second belt behind the flag.
- isAppBlocksEnabled's no-user branch is deliberately KEPT: it has a real caller
(pages/api/v1/developer/block-manifests.ts) that wants the base value.
Comments: every fail-closed / fail-safe claim in app-blocks-flag.ts swept and
given an accurate statement of what makes it closed; a new GLOBAL-EVAL SEMANTICS
block records the mechanism and the measurement once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(app-blocks): make the compiler the guard — require a subject on the author capability
Round-1 review: the structural guard I said needed type-level nullability analysis
is analysis this repo already runs on every PR (tekton / typecheck, App unit tests
+ typecheck). Measured, then implemented.
isAppBlocksAuthorEnabled's parameter is now REQUIRED and non-nullable:
(opts?: { user?: SessionUser }) -> (opts: { user: SessionUser })
Making it required errored at exactly 2 of the 10 call sites — both bare
middleware(...) whose ctx.user is not narrowed by the protectedProcedure they are
attached to (app-listings.router.ts, app-collaborators.router.ts). Each now refuses
explicitly instead of handing a possibly-undefined subject to an authz gate. The
other 8 already held a non-null subject. The runtime `if (!user) return false`
branch this makes dead is DELETED rather than kept as defence in depth, and the two
runtime tests that pinned it are deleted with it: they could only be re-added behind
an `as never` cast, i.e. testing a path the type system forbids while reading as
coverage. What replaces them is the typecheck gate, plus one test pinning the
residual the docblock now states — a cast-defeated call THROWS, never returns true.
isAppBlocksEnabled keeps its optional overload. That asymmetry is now justified on
SEMANTICS (a kill-switch answers "is the feature on at all", which a subject-less
machine path may legitimately ask and which the base value is; a capability answers
"may THIS subject", unanswerable without one) rather than on its sole no-arg caller
existing — block-manifests.ts is dormant, so that justification is the half that can
vanish.
Also:
- Delete fixtures/flipt-base-enabled-flip.snapshot.json (152 lines). Derived at
runtime from the sibling fixture by re-key + enabled flip. A checked-in twin cannot
track the original — re-capturing the source means re-anonymising it, so the copy
silently keeps the old segment shape while claiming production fidelity, and it had
already lagged a segment.
- Consolidate ~60 lines of duplicated harness into fixtures/flipt-fixture-server.ts,
shared with the pre-existing real-flipt-client integration suite. Only the snapshot
differs between them; the instrument is the same.
- Stop enumerating live flag state in the GLOBAL-EVAL SEMANTICS header — that is the
same rot class the paragraph warns about. Keep the imperative, point at flipt-state.
- apps-shared: say READ_OPS rather than naming two of its four members.
- Replace a rejects.not.toMatchObject(...) positive control, which passes on any
other rejection, with the specific NOT_FOUND / 'Block install not found' outcome.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): watchlist the two new refusals — the type guard cannot see a bundler
Round-1 review, 2 x yellow. Both taken.
WATCHLIST (the important one). My stated defence for deleting the helper's runtime
branch — "deleting it is a type error rather than a silent re-opening" — is a claim
about SOURCE. Release 5.1.18 is the case where the source is correct and the emitted
artefact is not: the bundler dropped two of three returns from a function in
app-blocks-flag.ts and served the whole App-store catalog to anonymous callers
(civitai#3983). That is why scripts/compiled-branch-watchlist.mjs exists, enforced at
Dockerfile:103, and two of the three refusals this PR adds are pure runtime branches
whose loss silently restores the exposure it closes. Both are now listed:
shared-storage-subject-refusal apps-shared.router.ts — lost, every READ_OPS op
serves shared rows to a vanished subject
block-token-subject-refusal blocks.router.ts — lost, the kill-switch is
evaluated with no subject on 16 runtime procs
assertViewerIsAppDeveloper's guard is deliberately NOT listed, and that is measured:
isAppBlocksAuthorEnabled takes a non-nullable subject and dereferences it at once, so
losing that guard throws rather than passes.
VERIFIED AGAINST A REAL BUILD, not just added. Full `next build` (green), then the
gate exactly as the Dockerfile runs it:
positive 26,600 maps scanned; both new entries report OK with their control
anchors mapped (an unmapped control is exit 2, not a pass)
negative required anchors repointed at unmapped comment lines in the same
functions -> exit 1 naming both entry ids, controls still mapping
deletion the required line removed in a throwaway source copy, one entry at a
time -> exit 2 naming that entry and its anchor
The two refusal messages were identical strings under different codes, so neither was
anchorable and "separable in a log" was not true. The author-gate message is now
'app-authoring subject could not be resolved'.
ROT (the second yellow). The isAppBlocksAgenticReviewEnabled docblock I wrote for this
rot class had the rot: it called the absent-flag half "load-bearing while this flag
does not exist in Flipt", but app-blocks-agentic-review has existed since 2026-07-21
(live: base false, `moderators` segment). No live exposure — all three call sites
check isModerator first — but it is a false claim in the one paragraph my own sweep
rewrote. Re-derived every app-blocks flag's live state rather than fixing only the
reported line, and found a second instance I had authored: the dev-tunnel docblock
offered "absent" as a live unconditional closure for a flag that also exists. Both
corrected; the only flag in this file genuinely absent from Flipt is
app-blocks-backpay-enabled.
Nits: assertAppBlocksEnabledForTokenUser has 16 call sites, not 17 (17 is the
parseSubjectUserId count; the substantive claim was right). deriveSnapshotFromFlagShape
now rejects an EMPTY rollouts array, which its error text already claimed to reject.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): resolve the two docblocks that answer the reader twice, oppositely
Follow-on from the same re-check that found the agentic-review rot. Eleven docblocks
in this file carry an inherited "the flag does NOT exist in Flipt at merge time"
sentence; the new header frames all of them as as-merged history, which is the
deliberate treatment (substituting a fresher enumeration would be the same rot one
generation on).
But in exactly TWO docblocks that inherited sentence sits in the SAME block as a
live-state sentence I wrote, so the reader gets opposite answers eight lines apart:
isAppListingsEnabled "does NOT exist" vs "Both are base-`false` today"
APP_BLOCKS_SHARED_STORAGE_FLAG "does NOT exist" vs "Closed today because the base
is `false`"
Both inherited sentences moved to past tense and marked as as-merged notes. Nothing
else changed: the other eleven are untouched, because they carry no competing claim.
Criterion for the split, so it can be re-applied: fix where a paragraph contradicts
itself; leave where the header already governs.
Re-verified after the edit rather than carrying the previous measurement over — these
are comments, but they shift line numbers in a WATCHLISTED module, and source/artefact
correspondence is exactly what this PR stopped assuming. Fresh `next build` (green),
then the compiled-branch gate: positive OK on all three entries (26,600 maps; controls
mapped), negative control still exit 1 naming both new entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): say at each watchlisted refusal that it is watchlisted
The compiled-branch gate names the entry, module, line and reason when it fires — but
only at Docker build time, after someone has already deleted the branch and pushed. The
two refusals this PR puts on the watchlist carried no signal in the file itself, so a
reader deciding whether the guard is load-bearing had nothing to go on. Two comments,
one at each site, naming the entry id and what a bundler can do to a pure runtime
branch.
Deliberately scoped: moving or rewording the throw is fine (the gate resolves its
anchor from source at run time), deleting it is not.
Re-verified after the edit, for the same reason as the previous commit: comments shift
line numbers in watchlisted modules. Fresh `next build` (green), gate positive OK on
all three entries, negative control still exit 1 naming both new entries. That is the
third independent repetition of both controls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): re-anchor the watchlist on format-robust text, and stop telling people to reword it
Round 2. Five defects, all introduced by this PR's own later commits.
F1 (the important one). The comment I added in 1f3fd96 said "Moving or rewording the
throw below is fine". Moving is fine; rewording is NOT, and neither is reformatting —
the anchor is an exact substring, a miss is die(2), and exit 2 is deliberately not
downgraded by --warn-only, so the first red is the production image build. Worse, both
anchors baked in the FORMATTING (a trailing `});` and a trailing `,`), and the
apps-shared line was 95 chars at printWidth 100 — so adding a `cause` or a metrics
counter, which apps.router.ts already has, reflows it and reds the build.
Fixed at the anchor, not just in the prose:
shared-storage required -> `if (userId != null && !subjectUser) {` (the CONDITION)
control -> `isAppBlocksSharedStorageEnabled(` (stops at the paren)
block-token required -> `'runtime block token subject could not be resolved'`
control -> `await isAppBlocksEnabled({ user })`
Measured, resolution phase isolated (it runs before any map is read):
OLD anchors + old source resolution OK (control)
OLD anchors + the apps-shared reflow RESOLUTION FAILED
NEW anchors + the same apps-shared reflow resolution OK
NEW anchors + apps-shared message reword resolution OK (anchor is the condition)
NEW anchors + blocks.router property-add resolution OK
NEW anchors + blocks.router message reword RESOLUTION FAILED <- inherent, documented
NEW anchors + unmodified copy resolution OK (control)
The last row cannot be fixed by choosing better text: `if (!user) {` is not unique in
blocks.router.ts (the author gate uses the same condition), so that entry must anchor a
literal. It is sound only because the literal is now unique APP-WIDE — nothing can
intern it from elsewhere — and that constraint is written into the entry, the site
comment and a new anchor-authoring rule in the watchlist header.
F5, which is the same edit. The kill-switch refusal was byte-identical in code AND
message to apps.router.ts:148, so an operator could not tell which gate refused.
Renamed to 'runtime block token subject could not be resolved' — the message is new in
this PR, so no existing consumer sees the change — which clears the collision and
supplies the unique literal F1 needs.
F2. 220fdd8e0 created a new "answers the reader twice, oppositely" by its own criterion:
the dev-tunnel docblock lost the absent-flag case three lines above an untouched
sentence that relies on it. That sentence is a live-state claim, so the file header does
not govern it. Made state-independent, matching the treatment its sibling edit in the
same commit already used.
F3. The nit fix swapped one wrong count for another. There are 16 parseSubjectUserId
sites, not 17: the site at :5674 guards BOTH gates, so the two sets overlap and must not
be added. (19 raw occurrences = 16 calls + the import + two comments.)
F4. The fixture guard checked only that some rollout exists while its message and the
consuming test's title both claim a SEGMENT rollout — so a re-capture with a threshold
rollout would pass, and a 100% threshold would sail past the base-false backstop too.
Now checks SEGMENT_ROLLOUT_TYPE, and a new test drives all three refusal arms plus a
positive control, so the guard is reachable rather than merely present.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): round 3 — stop writing the count down at all, and widen the fixture guard to its own docstring
Round 3 returned no 🔴, two 🟡 and two 🟢. This fixes both 🟡 and the 🟢 that is a
false claim in a doc governing how anchors are authored. Each was re-measured here
before being accepted; the auditor was right on all three.
🟡-1. The previous commit's "fix by FORM" was self-falsifying. It replaced a wrong
total with `grep -c 'await assertAppBlocksEnabledForTokenUser'` -> 16, and that line
CONTAINS the pattern, so it matches itself:
31d768fba2 (base) 16
5ad903c5c1 16
origin/main 16
7f029a6947 (merge) 17 <- the comment counted itself
Run without a path constraint it is 18, because apps.router.ts:139 declares a
DIFFERENT function of the same name. So a maintainer reading "16 such parse sites"
and running the very next line would get 17, conclude the doc was stale by one, and
reinstate the off-by-one that round 1 introduced and round 2 wrote the paragraph to
kill — the fourth consecutive wrong count in one parenthetical.
No number and no command now. The paragraph says why both are unsafe here and tells
the reader to enumerate. It also records that it has been wrong four rounds running,
each time by writing a figure down, so the next editor knows the cost before reaching
for a fifth.
Note for the record: the merge commit's own justification for that rewrite was wrong
in two specifics — it named `assertAppBlocksEnabledForTokenUser` where the count is
about `parseSubjectUserId`, and said the "import" did not exist when it does (line
15). Not force-pushed; the correction lives here beside it.
🟡-2. `flipt-fixture-server.ts` checked `.some(r => r.type === 'SEGMENT_ROLLOUT_TYPE')`
while its own comment names the hazard as a THRESHOLD rollout. `.some()` does not
cover a threshold rollout captured ALONGSIDE the segment one: the segment rollout is
still present, the guard passes, and `base-false-control` then evaluates true for
every subject — the exact attribution failure the fixture exists to prevent, restored
by a routine snapshot re-capture with a green suite. A guard whose description was
wider than its implementation.
Now requires a segment rollout AND no threshold rollout, with DISTINCT text per arm so
neither can die to the other's error. The test grew the arm that was uncovered, and
each arm now carries its own expected message rather than sharing one regex.
🔴 The first cut of this fix was itself wrong and an existing test caught it: keying
the message on `hasThreshold` alone made a THRESHOLD-ONLY template announce a rollout
"alongside its segment rollout" that was not there. `hasSegment &&` is load-bearing;
the comment says so.
Mutation-tested, each dying for its OWN reason on a DIFFERENT arm:
M1 drop the threshold arm (pre-fix behaviour) -> red: "expected [Function] to throw an error"
M2 key the message on hasThreshold alone -> red: message mismatch on the threshold-only arm
M1 is red at the pre-change code, so the new arm is regression coverage rather than an
invariant guard.
🟢-3. `compiled-branch-watchlist.mjs` asserted "comment churn above it cannot break
it", full stop. False: `resolveAnchor` scans every line including comments and >1
match is a hard error, so a comment merely CONTAINING the anchor text breaks
resolution exactly as a reword does — and rule 1's "prefer the shortest fragment"
makes a collision likelier, not less. This sentence had already misled a reader into
treating a docblock edit in a watchlisted module as free. Corrected, and it now says
the first red is this gate's own unit suite rather than the image build, which is the
only reason it is not deploy-blocking.
NOT fixed, deliberately: 🟢-4, that F5's new message is a strict SUPERSET of the
sibling it was renamed to be separable from, so a Loki grep for the shorter string
still returns both. The fix is another reword of a WATCHLISTED anchor plus its two
consumers, which is not a change to make at the tail of a ladder to close a 🟢.
Recorded as a ranked item instead.
Verified: 5 suites / 163 tests green (assert-compiled-branches, app-blocks-flag,
app-blocks-flag.base-enabled-flip, blocks.router.flag-gate-hydrate,
apps-shared.router); `pnpm typecheck` 0 errors.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9mHaY8iuJYDNfgYeSeWNj
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>