From 2a152f227d9b408e1d52a94680bb89f377fbd7b2 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 4 Sep 2026 12:50:08 -0500 Subject: [PATCH 1/5] =?UTF-8?q?fix(appapi):=20an=20ownership=20403=20is=20?= =?UTF-8?q?not=20an=20access=20problem=20=E2=80=94=20stop=20printing=20the?= =?UTF-8?q?=20Apps-author=20remedy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The listing 403 arm's catch-all appends "managing store listings needs Apps-author access (invite-only beta)" to every non-scope, non-takedown 403. One of those is the server's NOT_OWNED refusal, where the caller's Apps-author access is fine and is not what failed. This is the third instance of the wrong-subject class this package already catalogs: #374/#391 on the 400 arm, #509 on the takedown arm, ownership here. Measured 2026-09-03 against a real listing: the account was a moderator AND the publish request's submitted_by_user_id, and was still refused — ownership resolves kind-aware to the OAuth client's owner (resolveListingAccess -> appBlock.app.userId), not the listing's denormalized user_id and not submitted_by_user_id, and there is deliberately no moderator bypass on that gate. The same account managed a listing it owned in the same minutes. So no grant, re-login or cohort invite changes the outcome, and the remedy the CLI named was unreachable. isNotOwnedMsg classifies on the message because the wire carries no distinct code: mapOffsiteError (app-listings.router.ts:359) collapses NOT_OWNED and FORBIDDEN onto the same TRPCError code, so the discriminator is gone by the time the CLI sees it — the same constraint isInsufficientScopeMsg and isModeratorTakedownMsg document. Matched on the core shared by all three reachable spellings (offsite-listing.service.ts :2467 manage, :1323 edit, :1811 revision), which share no noun and no verb. The fourth (:582 withdraw) is unreachable here — its route maps NOT_OWNED to a 404 whose body is identical to NOT_FOUND, deliberately. Tests: four rows in TestListingForbiddenTakedownDoesNotBlameTheAccount, red at bf4db4b and green here. A mutation sweep then found the predicate's two clauses were each individually redundant — dropping either left every row green — so two further invariant rows were added, built from real shipped near-miss strings rather than invented ones ("You can only request 2 email changes per day", "You cannot purchase access to your own chapter"). Each of the four mutants is now killed by exactly its own row: no-case-fold, drop-"you can only ", drop-"your own", always-false. Also retargets one fixture in TestFallbackFailuresKeepTheirOwnError: its 403 row used "you can only manage your own listings" incidentally, and that string now has its own arm. Its subject (the fallback's error survives instead of inheriting the submission's not-found) is unchanged; the fixture is now a cohort refusal, which is the arm its wantMsg names. Full suite 21/21 packages ok, matching the baseline exactly. --- internal/appapi/appblocks.go | 35 ++++++++++ internal/appapi/listing.go | 16 +++++ internal/appapi/listing_forbidden_test.go | 84 +++++++++++++++++++++++ internal/cmd/app_offsite_test.go | 12 +++- 4 files changed, 146 insertions(+), 1 deletion(-) diff --git a/internal/appapi/appblocks.go b/internal/appapi/appblocks.go index 993dad7..2b7c3a9 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -1886,6 +1886,41 @@ func isModeratorTakedownMsg(msg string) bool { return strings.Contains(strings.ToLower(msg), "removed by a moderator") } +// isNotOwnedMsg detects the server's OWNERSHIP refusal, and exists for exactly +// the reason its two siblings above do: the wire carries no distinct code. The +// service throws `OffsiteRequestError('NOT_OWNED', …)`, but `mapOffsiteError` +// (`src/server/routers/app-listings.router.ts:359`) collapses NOT_OWNED and +// FORBIDDEN onto the SAME TRPCError code, so the original discriminator is gone +// by the time it reaches us and the message is all that is left. +// +// 🔴 THIS IS NOT AN ACCESS PROBLEM, which is the whole point of the branch it +// feeds. Ownership resolves KIND-AWARE to the OAuth client's owner +// (`resolveListingAccess` → `appBlock.app.userId`), NOT the listing's +// denormalized `user_id` and NOT `app_block_publish_requests.submitted_by_user_id` +// — and there is deliberately NO moderator bypass on that gate. Measured +// 2026-09-03: an account that was both a moderator and the publish request's +// submitter was still refused, while the SAME account managed a listing it owned +// in the same minutes. So no grant, re-login or cohort invite changes this +// outcome, and the fallback's "needs Apps-author access" is the wrong subject. +// +// Matched on the stable CORE shared by every reachable spelling rather than a +// whole sentence, because three of them ship today +// (`offsite-listing.service.ts`, civitai@origin/main): +// +// - :2467 "you can only manage your own listings" (the manage path) +// - :1323 "you can only edit your own listings" (the edit path) +// - :1811 "you can only submit your own revision" (the revision path) +// +// They share no noun and no verb — only "you can only …your own". A fourth, +// :582 "you can only withdraw your own publish requests", is unreachable HERE: +// its own route (`src/pages/api/v1/blocks/withdraw.ts:229`) maps NOT_OWNED to a +// 404 whose body is identical to NOT_FOUND, deliberately, so it never arrives as +// a 403. +func isNotOwnedMsg(msg string) bool { + m := strings.ToLower(msg) + return strings.Contains(m, "you can only ") && strings.Contains(m, "your own") +} + // devTunnelError maps a non-200 dev-tunnel tRPC response to an actionable CLI // error. tRPC error bodies are {error:{json:{message,code,...}}}; the HTTP // status carries the mapped code (403 flag-off/not-author, 404 not-your-app). diff --git a/internal/appapi/listing.go b/internal/appapi/listing.go index 7f8ed0e..f155287 100644 --- a/internal/appapi/listing.go +++ b/internal/appapi/listing.go @@ -1016,6 +1016,22 @@ func listingError(status int, raw []byte, route listingRoute) (err error) { if isModeratorTakedownMsg(msg) { return fmt.Errorf("this listing is under a moderator takedown (403): %s — your account's access is not the problem and no CLI command reverses this; ask a Civitai moderator to relist it", msg) } + // 🔴 THE THIRD INSTANCE OF THE WRONG-SUBJECT CLASS (#374/#391 on the 400 + // arm, #509 on the takedown arm above, this one on ownership). An + // OWNERSHIP refusal is not an access refusal: the caller's Apps-author + // access can be perfect — they can be a moderator, and the account that + // submitted the publish request — and still be refused, because the gate + // resolves the OAuth client's owner and has no moderator bypass. See + // isNotOwnedMsg for the measurement. + // + // It must sit BELOW the takedown branch. A listing can be BOTH + // moderator-removed and owned by someone else, and the server's ownership + // check runs FIRST, so the client only ever sees one of the two messages + // — but if that ever changes, "ask a moderator to relist it" is the more + // actionable of the two and should keep winning. + if isNotOwnedMsg(msg) { + return fmt.Errorf("this listing belongs to another account (403): %s — your account's access is not the problem; sign in as the app's owner (`civitai whoami` shows who you are) or ask its owner to make the change", msg) + } return fmt.Errorf("not permitted for your account (403): %s — managing store listings needs Apps-author access (invite-only beta)", msg) case http.StatusNotFound: return fmt.Errorf("no store listing found for this app (404): %s — a store listing is created when you run `civitai app submit` and is settable while the app is pending review; submit the app first, then these commands will work", msg) diff --git a/internal/appapi/listing_forbidden_test.go b/internal/appapi/listing_forbidden_test.go index 7e77596..289afa0 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -90,6 +90,90 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { wantSubstr: "ask a Civitai moderator to relist it", wantAccessClaim: false, }, + { + // 🔴 THE THIRD INSTANCE OF THE WRONG-SUBJECT CLASS, one arm over + // again. civitai/civitai@origin/main, + // src/server/services/blocks/offsite-listing.service.ts:2467 — + // `OffsiteRequestError('NOT_OWNED', 'you can only manage your own + // listings')`, thrown when `resolveListingRole` returns null, i.e. + // the caller is neither the listing's owner nor an accepted + // collaborator on it. + // + // Measured on 2026-09-03 against a real listing: the account was a + // moderator AND the publish request's `submitted_by_user_id`, and + // was still refused, because ownership resolves KIND-AWARE to the + // OAuth client's owner (`appBlock.app.userId`) and there is + // deliberately no moderator bypass on that gate. So the caller's + // Apps-author access is fine and is not what failed. + name: "not-owned, the manage refusal", + msg: "you can only manage your own listings", + wantSubstr: "belongs to another account", + wantAccessClaim: false, + }, + { + // A SECOND spelling from the same service (:1323, the edit path). + // Listed separately for the same reason the takedown rows are: a + // predicate keyed on one whole sentence answers the other with the + // false advice. + name: "not-owned, the edit refusal", + msg: "you can only edit your own listings", + wantSubstr: "belongs to another account", + wantAccessClaim: false, + }, + { + // A THIRD spelling (:1811, the revision-submit path). It shares + // neither the noun ("revision", not "listings") nor the verb with + // the two above — only the core the predicate matches. + // + // A FOURTH exists, ':582 — you can only withdraw your own publish + // requests', and is deliberately NOT a row here: its own route + // (src/pages/api/v1/blocks/withdraw.ts:229) maps NOT_OWNED to 404 + // with a body identical to NOT_FOUND, so it cannot reach this arm. + name: "not-owned, the revision-submit refusal", + msg: "you can only submit your own revision", + wantSubstr: "belongs to another account", + wantAccessClaim: false, + }, + { + // 🔴 INVARIANT GUARD, not regression coverage — labelled as one for + // the same reason as the takedown case-fold row above: no measured + // server string capitalises this core today, but the predicate + // folds case, so a "simplification" that drops the fold is a live + // hazard with no shipped string to catch it. + name: "invariant: the not-owned core is matched case-insensitively", + msg: "You Can Only manage Your Own listings", + wantSubstr: "belongs to another account", + wantAccessClaim: false, + }, + { + // 🔴 INVARIANT GUARD pinning the predicate's NARROWNESS, added + // because a mutation sweep found the conjunction's two halves were + // each individually redundant: dropping either clause left every + // row above still green. This row kills the "you can only " half. + // + // A REAL string, not an invented one — `civitai@origin/main` ships + // many "you can only …" refusals that are nothing to do with + // ownership (rate limits, upload caps, status preconditions). This + // is the rate-limit one. It cannot reach this arm today, hence + // invariant-guard rather than regression coverage; what it pins is + // that a future widening to a bare "you can only" prefix would + // start telling a rate-limited user their listing belongs to + // someone else. + name: "invariant: a non-ownership 'you can only' refusal is NOT not-owned", + msg: "You can only request 2 email changes per day. Please try again tomorrow.", + wantSubstr: accessClaim, + wantAccessClaim: true, + }, + { + // 🔴 INVARIANT GUARD, the mirror of the row above — it kills the + // "your own" half of the conjunction, which the same sweep found + // equally unpinned. Also a real shipped string: several refusals + // say "your own" without being ownership refusals of a LISTING. + name: "invariant: a non-ownership 'your own' refusal is NOT not-owned", + msg: "You cannot purchase access to your own chapter.", + wantSubstr: accessClaim, + wantAccessClaim: true, + }, { // The 403 the fallback is TRUE of — the invite-gated cohort. Same // string TestListingErrorMapping uses, kept here so a fix that diff --git a/internal/cmd/app_offsite_test.go b/internal/cmd/app_offsite_test.go index c86e2aa..24a18de 100644 --- a/internal/cmd/app_offsite_test.go +++ b/internal/cmd/app_offsite_test.go @@ -635,8 +635,18 @@ func TestFallbackFailuresKeepTheirOwnError(t *testing.T) { // listingError's arm for that status — not from a shared prefix. wantMsg string }{ + // 🔴 THE FIXTURE MESSAGE IS A COHORT REFUSAL ON PURPOSE. It used to be + // "you can only manage your own listings", which was incidental — any + // 403 body exercises this row's actual subject (the fallback's own + // error survives instead of inheriting the submission's not-found). + // That string now has its OWN arm in listingError (the NOT_OWNED + // ownership branch), so leaving it here would silently re-point this + // row at that arm and couple an unrelated test to it. The cohort + // message is the one the catch-all is genuinely true of, which is the + // arm `wantMsg` below names. The not-owned routing is pinned by + // appapi's TestListingForbiddenTakedownDoesNotBlameTheAccount. {"403 from the listings route", http.StatusForbidden, - `{"error":{"json":{"message":"you can only manage your own listings"}}}`, + `{"error":{"json":{"message":"Apps authoring is not enabled"}}}`, civitai.ErrUnauthorized, "not permitted for your account (403)"}, {"500 from the listings route", http.StatusInternalServerError, `{"error":{"json":{"message":"boom"}}}`, nil, ""}, From e319cff32bbc7c6be11bd89ff674d415f677f236 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 4 Sep 2026 13:14:41 -0500 Subject: [PATCH 2/5] =?UTF-8?q?fix(appapi):=20audit=20round=201=20?= =?UTF-8?q?=E2=80=94=20correct=20two=20false=20claims=20in=20the=20new=20c?= =?UTF-8?q?omment,=20and=20add=20the=20README=20row?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial audit of this PR returned "safe to merge" with three 🟡s. None changed runtime behaviour; two were my own comments asserting things that are false, which is the reasoning a future maintainer inherits. 1. README Troubleshooting index (AGENTS.md: a behaviour change is incomplete until README moves; #509, the class precedent this PR cites, shipped code + row in one commit). Adds the `belongs to another account (403)` row, and fixes the catch-all row, which said the takedown message was in "the row below" — there are now two siblings, not one. 2. The unreachability argument for the fourth spelling named the WRONG route. The conclusion holds; the mechanism did not. :582 is raised by withdrawExternalRequest, whose proc wraps every failure as BAD_REQUEST (400) and which the CLI never calls. The REST withdraw route handles a byte-identical TWIN thrown by a different class in publish-request.service.ts — not this string. Also records that safety here is route-population containment rather than a property of the string, names the two further unreachable offsite-moderation spellings, and flags that consolidating withdrawExternalRequest onto mapOffsiteError would turn :582 into a real 403. 3. The ownership claim gave only the ONSITE branch and named the offsite authority as the thing that is NOT the authority — backwards for exactly the case these throws serve. app-access.service.ts:283-292 returns blockOwnerUserId ?? listingUserId for onsite and listingUserId for offsite. Both branches now stated. Also, from the two 🟢s: the remedy now names the collaborator case (the gate is resolveListingRole === null, so an invited developer who has not accepted the seat lands here and "sign in as the owner" is the wrong step for them), and the accepted imprecision is recorded — the server reports NOT_OWNED for a listing deleted between its two reads, so this sentence is confidently wrong about a listing that no longer exists. Not a regression; the catch-all was equally false there. Re-verified after the fixes, because an audit fix resets the gate: gofmt -s -l clean, go vet clean, full suite 21/21 packages matching baseline, and the mutation sweep re-run because the assertions read the message I changed — identity control silent, and each of the four mutants still killed by exactly its own row. --- README.md | 3 +- internal/appapi/appblocks.go | 73 +++++++++++++++++++---- internal/appapi/listing.go | 15 ++++- internal/appapi/listing_forbidden_test.go | 9 ++- 4 files changed, 82 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 0c9db92..cf97def 100644 --- a/README.md +++ b/README.md @@ -3562,8 +3562,9 @@ credited it to the wrong command.) | `no token configured` | Nothing is logged in. Run `civitai login`, or set `CIVITAI_TOKEN`. This includes `civitai app list` / `app view`: the App **store** is not an anonymous read. | [Submit & auth](#submit--auth), [Browse the App store](#browse-the-app-store) | | `not logged in (401)` | The credential is present but invalid or expired. OAuth tokens refresh themselves; when the *refresh* token has also expired, log in again. For a personal key, mint a new one at `civitai.com/user/account`. | [Submit & auth](#submit--auth) | | `forbidden (403)` | Usually the invite-only Apps beta rather than a broken token — the same account reads the public API fine. | [Submit & auth](#submit--auth) | -| `not permitted for your account (403)` | The **catch-all** listing `403`: managing a **store listing** needs Apps-author access, which is a narrower grant than being able to submit. It is not what a **moderator takedown** prints — that has its own message, the row below. | [Listing media requirements](#listing-media-requirements) | +| `not permitted for your account (403)` | The **catch-all** listing `403`: managing a **store listing** needs Apps-author access, which is a narrower grant than being able to submit. It is what remains once the two listing `403`s that are *not* about your grant have been ruled out — a **moderator takedown** and a listing **owned by another account** — each of which prints its own message, in the two rows below. | [Listing media requirements](#listing-media-requirements) | | `under a moderator takedown (403)` | A moderator removed this **store listing**, so the platform refuses to edit it. **Your account's access is not the problem** — this is the listing `403` that is *not* about your credential — and no login, grant or CLI command reverses it: ask a Civitai moderator to relist it. The server's own sentence follows the code, and it is what names the state. Exit `3`. **Not** the same as unpublishing the listing yourself: that refuses a *material* change as a `400` and exits `2`. | [Exit code 3](#exit-code-3) | +| `belongs to another account (403)` | The listing is real and readable, but this account is **neither its owner nor an accepted collaborator on it**, so the platform refuses to manage it. **Your account's access is not the problem** — the second listing `403` that is not about your credential — and being a **moderator** does not help: this gate has no moderator bypass, and submitting the app's publish request is not ownership either. Sign in as the app's owner (`civitai whoami` shows who you are), accept a pending collaborator invite if you were sent one, or ask the owner to make the change. Exit `3`. | [Listing media requirements](#listing-media-requirements) | | `Submit Apps:` | The `civitai whoami` capability row, and it is **tri-state**: `yes` / `no` / `unknown`. **`unknown` is not `no`** — it is the CLI declining to answer, because the server reported no `tokenScope` for an **OAuth** credential (whose submit answer *is* the `AppBlocksSubmit` bit) or sent no `subject` at all, so which gate applies is itself unknown. Re-run `civitai login` to mint a token whose scope the server reports; a **personal API key** always answers `yes`, since it is not scope-gated for submit. In `--json` this row is `canSubmitApps`, which is `true` / `false` / `null`. | [Submit & auth](#submit--auth) | | `(token scope not reported by the server — Buzz capabilities unknown)` | `civitai whoami` got no `tokenScope`, so **Buzz** read/spend are unknowable and are omitted rather than printed as `no`. Scoped to Buzz deliberately: the **Submit Apps** row is still shown above it, because a personal key's submit answer does not depend on the scope mask. | [Submit & auth](#submit--auth) | | `not permitted to read this app's analytics (403)` | `app metrics` needs the **Apps submit scope**. An OAuth `civitai login` carries it — unless the token was minted before the scope existed, in which case re-run `civitai login`. A full-scope personal API key also works. | [App metrics](#app-metrics) | diff --git a/internal/appapi/appblocks.go b/internal/appapi/appblocks.go index 2b7c3a9..c26e752 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -1894,14 +1894,33 @@ func isModeratorTakedownMsg(msg string) bool { // by the time it reaches us and the message is all that is left. // // 🔴 THIS IS NOT AN ACCESS PROBLEM, which is the whole point of the branch it -// feeds. Ownership resolves KIND-AWARE to the OAuth client's owner -// (`resolveListingAccess` → `appBlock.app.userId`), NOT the listing's -// denormalized `user_id` and NOT `app_block_publish_requests.submitted_by_user_id` -// — and there is deliberately NO moderator bypass on that gate. Measured -// 2026-09-03: an account that was both a moderator and the publish request's -// submitter was still refused, while the SAME account managed a listing it owned -// in the same minutes. So no grant, re-login or cohort invite changes this -// outcome, and the fallback's "needs Apps-author access" is the wrong subject. +// feeds. The gate is `resolveListingRole(...) === null` — the caller is neither +// the listing's owner NOR an accepted collaborator on it — and there is +// deliberately NO moderator bypass on it (contrast +// `app-listing-assets.service::loadOwnedListing`, which does bypass for mods; +// `offsite-listing.service.ts:1289-1291` states the difference). Being +// `app_block_publish_requests.submitted_by_user_id` is not ownership either. +// +// 🔴 Ownership is resolved KIND-AWARE, and BOTH branches matter — an earlier +// draft of this comment gave only the first and named the second as the thing +// that is *not* the authority, which is backwards for the offsite case: +// +// // app-access.service.ts:283-292 +// if (args.kind === 'onsite') return args.blockOwnerUserId ?? args.listingUserId; +// return args.listingUserId; +// +// So for an ONSITE listing the authority is the OAuth client's owner +// (`appBlock.app.userId`), falling back to the listing's `user_id`; for OFFSITE +// (and any unrecognised kind) the authority IS the listing's `user_id`. All +// three throws above live in `offsite-listing.service.ts` and their gates are +// dual-kind, so a maintainer debugging an offsite refusal by looking at +// `AppBlock.app.userId` is looking at the wrong column — it can be null there. +// +// Measured 2026-09-03: an account that was both a moderator and the publish +// request's submitter was still refused, while the SAME account managed a +// listing it owned in the same minutes. So no grant, re-login or cohort invite +// changes this outcome, and the fallback's "needs Apps-author access" is the +// wrong subject. // // Matched on the stable CORE shared by every reachable spelling rather than a // whole sentence, because three of them ship today @@ -1911,11 +1930,39 @@ func isModeratorTakedownMsg(msg string) bool { // - :1323 "you can only edit your own listings" (the edit path) // - :1811 "you can only submit your own revision" (the revision path) // -// They share no noun and no verb — only "you can only …your own". A fourth, -// :582 "you can only withdraw your own publish requests", is unreachable HERE: -// its own route (`src/pages/api/v1/blocks/withdraw.ts:229`) maps NOT_OWNED to a -// 404 whose body is identical to NOT_FOUND, deliberately, so it never arrives as -// a 403. +// They share no noun and no verb — only "you can only …your own". +// +// 🔴 SAFETY HERE IS ROUTE-POPULATION CONTAINMENT, NOT A PROPERTY OF THE STRING. +// The predicate is not scoped to listings, so what keeps it from mis-firing is +// that only `appListings.*` procs reach `listingError`. Other NOT_OWNED-shaped +// refusals ship elsewhere in that server (challenges, collections, creator +// shop, remix gallery) and match the same core; none of them is on a route this +// function answers for. **If the CLI ever routes a non-listing proc through +// `listingError`, re-run that enumeration** — over the whole server repo, not +// over one file. +// +// Two more NOT_OWNED strings ship in `offsite-moderation.service.ts` (:1668, +// :2226) and are unreachable only because the CLI calls none of the procs that +// raise them (`unpublishOwnListing` / `republishOwnListing` / +// `listMyListingModerationEvents`). +// +// A fourth listing-service spelling, :582 "you can only withdraw your own +// publish requests", is also unreachable here — but by neither of the two +// mechanisms an earlier draft of this comment claimed. Its `OffsiteRequestError` +// is raised by `withdrawExternalRequest`, whose route is the tRPC proc +// `appListings.withdrawExternalRequest` (`app-listings.router.ts:646`); that +// proc does NOT use `mapOffsiteError` — it wraps every failure in +// `TRPCError{code:'BAD_REQUEST'}` (:653-659), i.e. HTTP 400, not 403. And the +// CLI does not call it at all: its withdraw path is the REST `WithdrawPath` +// with its own `withdrawError`. (The REST route `api/v1/blocks/withdraw.ts` +// handles a BYTE-IDENTICAL twin string thrown by a DIFFERENT class in +// `publish-request.service.ts:1649` — not this one. Do not merge the two.) +// +// 🔴 That proc is the one in its router that hand-rolls its error mapping while +// its own docstring says it "mirrors `blocks.withdrawPublishRequest`". An +// ordinary consolidation onto `mapOffsiteError` would turn :582 into a real 403 +// that this predicate matches — at which point the printed noun "this listing" +// is wrong for a publish request. Re-read this arm if that consolidation lands. func isNotOwnedMsg(msg string) bool { m := strings.ToLower(msg) return strings.Contains(m, "you can only ") && strings.Contains(m, "your own") diff --git a/internal/appapi/listing.go b/internal/appapi/listing.go index f155287..63758b5 100644 --- a/internal/appapi/listing.go +++ b/internal/appapi/listing.go @@ -1029,8 +1029,21 @@ func listingError(status int, raw []byte, route listingRoute) (err error) { // check runs FIRST, so the client only ever sees one of the two messages // — but if that ever changes, "ask a moderator to relist it" is the more // actionable of the two and should keep winning. + // 🔴 THE REMEDY NAMES THE COLLABORATOR CASE, because the gate is + // `resolveListingRole(...) === null` — neither owner NOR accepted + // collaborator. An invited developer who has not accepted the seat is + // refused here, and for them "sign in as the owner" is the wrong step + // while "accept the invite" is the one that works. Omitting it would be + // this arm's own defect one notch smaller. + // + // ⚠ KNOWN IMPRECISION, accepted: the server reports NOT_OWNED (not + // NOT_FOUND) for a listing DELETED between its two reads — a race its + // own source records at `offsite-listing.service.ts:2454-2461`. This + // sentence is then confidently wrong about a listing that no longer + // exists. It is not a regression — the catch-all it replaces was equally + // false there — but do not tighten the wording without re-reading that. if isNotOwnedMsg(msg) { - return fmt.Errorf("this listing belongs to another account (403): %s — your account's access is not the problem; sign in as the app's owner (`civitai whoami` shows who you are) or ask its owner to make the change", msg) + return fmt.Errorf("this listing belongs to another account (403): %s — your account's access is not the problem, and being a moderator does not help: sign in as the app's owner (`civitai whoami` shows who you are), accept a pending collaborator invite if you were sent one, or ask the owner to make the change", msg) } return fmt.Errorf("not permitted for your account (403): %s — managing store listings needs Apps-author access (invite-only beta)", msg) case http.StatusNotFound: diff --git a/internal/appapi/listing_forbidden_test.go b/internal/appapi/listing_forbidden_test.go index 289afa0..3429d6f 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -126,9 +126,12 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { // the two above — only the core the predicate matches. // // A FOURTH exists, ':582 — you can only withdraw your own publish - // requests', and is deliberately NOT a row here: its own route - // (src/pages/api/v1/blocks/withdraw.ts:229) maps NOT_OWNED to 404 - // with a body identical to NOT_FOUND, so it cannot reach this arm. + // requests', and is deliberately NOT a row here: it is raised by + // `withdrawExternalRequest`, whose proc wraps every failure as + // BAD_REQUEST (400, not 403) and which the CLI does not call at all. + // See isNotOwnedMsg for why the REST withdraw route is NOT the + // mechanism — it handles a byte-identical twin from a different + // service. name: "not-owned, the revision-submit refusal", msg: "you can only submit your own revision", wantSubstr: "belongs to another account", From 466e6d2dd8b3bdae01c93672189cf2c2dd8c9925 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 4 Sep 2026 13:37:25 -0500 Subject: [PATCH 3/5] =?UTF-8?q?fix(appapi):=20audit=20round=202=20?= =?UTF-8?q?=E2=80=94=20the=20containment=20premise=20was=20false,=20the=20?= =?UTF-8?q?corrected=20claim=20survived=20in=20two=20more=20copies,=20and?= =?UTF-8?q?=20the=20remedy=20was=20unpinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delta re-audit of round 1's fixes. Verdict was again "safe to merge", and again the findings were the fix round's own — which is why the round ran. F1 (payload). Round 1 replaced a wrong MECHANISM with a wrong INVARIANT. It claimed "only appListings.* procs reach listingError"; measured, listingError has three call sites and the third is MintImageUpload, passing ImageUploadPath — the REST endpoint /api/v1/image-upload, not a proc. False the day it was written. Worse, the trigger it prescribed ("if the CLI routes a NON-LISTING proc through listingError") is a different predicate from its own premise and would not fire for image-upload, since image-upload IS a listing route. Now stated as the listingRoute block with a mechanical trigger — re-run the enumeration when a route is ADDED TO THAT BLOCK — and the enumeration gains apps-shared.router.ts:611 ("you can only edit your own submissions"), the family closest to this CLI's own surface. Records that containment at /api/v1/image-upload is luck: its only ownership refusal is "You do not own this image", which does not match the core. F2 (payload + scaffolding). Round 1 corrected ONE of THREE copies of the onsite-only ownership claim. The two survivors are fixed here, including the test-file copy, which annotated the row it was most misleading for — getMyListingForApp is dual-kind, and on an offsite listing appBlockId is null, so that comment sent a debugger to a null column. F3 (scaffolding). The remedy this PR shipped was pinned by nothing: deleting BOTH the collaborator and moderator clauses left the entire suite green. Adds wantRemedy to the table, populated on the three not-owned rows. Watched red: the exact revert now fails all three. F4 (payload). The new README row linked to #listing-media-requirements — image formats and byte caps. The repo's own row-link ledger records that judgement verbatim for a different row, and the sibling takedown row added by this same PR points at #exit-code-3, which is the section that actually discusses "not every 403 here is about your credential". Retargeted. F5 (payload). Two citations named ranges a few lines off the code they describe: :653-659 -> :660-664 for the BAD_REQUEST wrap, and 2454-2461 -> 2453-2460 for the deleted-between-reads race. Both already landed in the right function. Re-verified: gofmt -s -l clean, go vet clean, 21/21 packages. --- README.md | 2 +- internal/appapi/appblocks.go | 29 ++++++++++++---- internal/appapi/listing.go | 9 +++-- internal/appapi/listing_forbidden_test.go | 42 ++++++++++++++++++++--- 4 files changed, 67 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index cf97def..d0634e6 100644 --- a/README.md +++ b/README.md @@ -3564,7 +3564,7 @@ credited it to the wrong command.) | `forbidden (403)` | Usually the invite-only Apps beta rather than a broken token — the same account reads the public API fine. | [Submit & auth](#submit--auth) | | `not permitted for your account (403)` | The **catch-all** listing `403`: managing a **store listing** needs Apps-author access, which is a narrower grant than being able to submit. It is what remains once the two listing `403`s that are *not* about your grant have been ruled out — a **moderator takedown** and a listing **owned by another account** — each of which prints its own message, in the two rows below. | [Listing media requirements](#listing-media-requirements) | | `under a moderator takedown (403)` | A moderator removed this **store listing**, so the platform refuses to edit it. **Your account's access is not the problem** — this is the listing `403` that is *not* about your credential — and no login, grant or CLI command reverses it: ask a Civitai moderator to relist it. The server's own sentence follows the code, and it is what names the state. Exit `3`. **Not** the same as unpublishing the listing yourself: that refuses a *material* change as a `400` and exits `2`. | [Exit code 3](#exit-code-3) | -| `belongs to another account (403)` | The listing is real and readable, but this account is **neither its owner nor an accepted collaborator on it**, so the platform refuses to manage it. **Your account's access is not the problem** — the second listing `403` that is not about your credential — and being a **moderator** does not help: this gate has no moderator bypass, and submitting the app's publish request is not ownership either. Sign in as the app's owner (`civitai whoami` shows who you are), accept a pending collaborator invite if you were sent one, or ask the owner to make the change. Exit `3`. | [Listing media requirements](#listing-media-requirements) | +| `belongs to another account (403)` | The listing is real and readable, but this account is **neither its owner nor an accepted collaborator on it**, so the platform refuses to manage it. **Your account's access is not the problem** — the second listing `403` that is not about your credential — and being a **moderator** does not help: this gate has no moderator bypass, and submitting the app's publish request is not ownership either. Sign in as the app's owner (`civitai whoami` shows who you are), accept a pending collaborator invite if you were sent one, or ask the owner to make the change. Exit `3`. | [Exit code 3](#exit-code-3) | | `Submit Apps:` | The `civitai whoami` capability row, and it is **tri-state**: `yes` / `no` / `unknown`. **`unknown` is not `no`** — it is the CLI declining to answer, because the server reported no `tokenScope` for an **OAuth** credential (whose submit answer *is* the `AppBlocksSubmit` bit) or sent no `subject` at all, so which gate applies is itself unknown. Re-run `civitai login` to mint a token whose scope the server reports; a **personal API key** always answers `yes`, since it is not scope-gated for submit. In `--json` this row is `canSubmitApps`, which is `true` / `false` / `null`. | [Submit & auth](#submit--auth) | | `(token scope not reported by the server — Buzz capabilities unknown)` | `civitai whoami` got no `tokenScope`, so **Buzz** read/spend are unknowable and are omitted rather than printed as `no`. Scoped to Buzz deliberately: the **Submit Apps** row is still shown above it, because a personal key's submit answer does not depend on the scope mask. | [Submit & auth](#submit--auth) | | `not permitted to read this app's analytics (403)` | `app metrics` needs the **Apps submit scope**. An OAuth `civitai login` carries it — unless the token was minted before the scope existed, in which case re-run `civitai login`. A full-scope personal API key also works. | [App metrics](#app-metrics) | diff --git a/internal/appapi/appblocks.go b/internal/appapi/appblocks.go index c26e752..890318f 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -1934,12 +1934,27 @@ func isModeratorTakedownMsg(msg string) bool { // // 🔴 SAFETY HERE IS ROUTE-POPULATION CONTAINMENT, NOT A PROPERTY OF THE STRING. // The predicate is not scoped to listings, so what keeps it from mis-firing is -// that only `appListings.*` procs reach `listingError`. Other NOT_OWNED-shaped -// refusals ship elsewhere in that server (challenges, collections, creator -// shop, remix gallery) and match the same core; none of them is on a route this -// function answers for. **If the CLI ever routes a non-listing proc through -// `listingError`, re-run that enumeration** — over the whole server repo, not -// over one file. +// WHICH ROUTES reach `listingError` — and that set is the `listingRoute` block +// at the top of `listing.go`, NOT "the `appListings.*` procs". An earlier draft +// of this paragraph said the latter and was already false when written: +// `listingError` has three call sites, and the third is `MintImageUpload` +// (`listing.go`), which passes `ImageUploadPath` — the REST endpoint +// `/api/v1/image-upload`, not a tRPC proc at all. +// +// So the trigger is mechanical and matches the premise: **re-run the +// enumeration when a route is ADDED TO THAT BLOCK**, whatever its shape. (An +// earlier trigger said "if the CLI routes a NON-LISTING proc through +// listingError" — a different predicate from its own premise, and one that +// would not fire for image-upload, since image-upload *is* a listing route.) +// +// The enumeration itself, over the whole server repo rather than one file: +// other NOT_OWNED-shaped refusals matching this core ship for challenges, +// collections, creator shop and remix gallery — and, closest to this CLI's own +// surface, `apps-shared.router.ts:611` ("you can only edit your own +// submissions"). None is on a route in that block today. `/api/v1/image-upload` +// is the one to watch: its only ownership refusal is currently "You do not own +// this image" (`block-image-upload.service.ts:360`), which does NOT match the +// core — so containment there is luck, not design. // // Two more NOT_OWNED strings ship in `offsite-moderation.service.ts` (:1668, // :2226) and are unreachable only because the CLI calls none of the procs that @@ -1952,7 +1967,7 @@ func isModeratorTakedownMsg(msg string) bool { // is raised by `withdrawExternalRequest`, whose route is the tRPC proc // `appListings.withdrawExternalRequest` (`app-listings.router.ts:646`); that // proc does NOT use `mapOffsiteError` — it wraps every failure in -// `TRPCError{code:'BAD_REQUEST'}` (:653-659), i.e. HTTP 400, not 403. And the +// `TRPCError{code:'BAD_REQUEST'}` (:660-664), i.e. HTTP 400, not 403. And the // CLI does not call it at all: its withdraw path is the REST `WithdrawPath` // with its own `withdrawError`. (The REST route `api/v1/blocks/withdraw.ts` // handles a BYTE-IDENTICAL twin string thrown by a DIFFERENT class in diff --git a/internal/appapi/listing.go b/internal/appapi/listing.go index 63758b5..5373cc4 100644 --- a/internal/appapi/listing.go +++ b/internal/appapi/listing.go @@ -1021,8 +1021,11 @@ func listingError(status int, raw []byte, route listingRoute) (err error) { // OWNERSHIP refusal is not an access refusal: the caller's Apps-author // access can be perfect — they can be a moderator, and the account that // submitted the publish request — and still be refused, because the gate - // resolves the OAuth client's owner and has no moderator bypass. See - // isNotOwnedMsg for the measurement. + // resolves ownership KIND-AWARE (onsite: the OAuth client's owner, + // falling back to the listing's `user_id`; OFFSITE: the listing's + // `user_id`) and has no moderator bypass. See isNotOwnedMsg for the + // measurement and for why the offsite branch is the one that matters + // here. // // It must sit BELOW the takedown branch. A listing can be BOTH // moderator-removed and owned by someone else, and the server's ownership @@ -1038,7 +1041,7 @@ func listingError(status int, raw []byte, route listingRoute) (err error) { // // ⚠ KNOWN IMPRECISION, accepted: the server reports NOT_OWNED (not // NOT_FOUND) for a listing DELETED between its two reads — a race its - // own source records at `offsite-listing.service.ts:2454-2461`. This + // own source records at `offsite-listing.service.ts:2453-2460`. This // sentence is then confidently wrong about a listing that no longer // exists. It is not a regression — the catch-all it replaces was equally // false there — but do not tighten the wording without re-reading that. diff --git a/internal/appapi/listing_forbidden_test.go b/internal/appapi/listing_forbidden_test.go index 3429d6f..86cf677 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -57,6 +57,23 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { // wantAccessClaim is whether the Apps-author-access diagnosis belongs // on this 403 at all. wantAccessClaim bool + // wantRemedy is every clause of the arm's REMEDY that must survive a + // reword. + // + // 🔴 IT EXISTS BECAUSE THE REMEDY WAS UNPINNED AND AN AUDIT PROVED IT: + // deleting BOTH the collaborator clause and the moderator clause from + // the not-owned arm left the ENTIRE suite green (measured, `-count=1`, + // full `./...`). Every other assertion here keys on the arm's opening + // words, the echoed server message, or the absence of the false advice + // — none of them touches the part that tells the user what to DO. + // + // That matters most for the collaborator clause: the gate is + // `resolveListingRole(...) === null`, so an invited developer who has + // not accepted the seat lands here, and "sign in as the owner" is a + // step they cannot take while "accept the invite" is the one that + // works. Dropping it would be this arm's own defect one notch smaller, + // which is the thing the whole PR exists to stop doing. + wantRemedy []string }{ { // civitai/civitai@origin/main, @@ -101,14 +118,21 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { // // Measured on 2026-09-03 against a real listing: the account was a // moderator AND the publish request's `submitted_by_user_id`, and - // was still refused, because ownership resolves KIND-AWARE to the - // OAuth client's owner (`appBlock.app.userId`) and there is - // deliberately no moderator bypass on that gate. So the caller's - // Apps-author access is fine and is not what failed. + // was still refused, because ownership resolves KIND-AWARE and + // there is deliberately no moderator bypass on that gate. So the + // caller's Apps-author access is fine and is not what failed. + // + // 🔴 KIND-AWARE means BOTH branches, and this row's throw + // (`getMyListingForApp`) is dual-kind: onsite resolves to the OAuth + // client's owner falling back to the listing's `user_id`, OFFSITE + // resolves to the listing's `user_id`. For an offsite listing + // `appBlockId` is null, so reading `appBlock.app.userId` to debug + // one sends you to a null column. See isNotOwnedMsg. name: "not-owned, the manage refusal", msg: "you can only manage your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, + wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, }, { // A SECOND spelling from the same service (:1323, the edit path). @@ -119,6 +143,7 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only edit your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, + wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, }, { // A THIRD spelling (:1811, the revision-submit path). It shares @@ -136,6 +161,7 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only submit your own revision", wantSubstr: "belongs to another account", wantAccessClaim: false, + wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, }, { // 🔴 INVARIANT GUARD, not regression coverage — labelled as one for @@ -231,6 +257,14 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { } } + for _, clause := range tc.wantRemedy { + if !strings.Contains(got, clause) { + t.Errorf("the 403 for %q lost the remedy clause %q. The arm still names the right "+ + "CAUSE, so every other assertion here stays green — but the user is no longer "+ + "told the step that works.\ngot: %s", tc.msg, clause, got) + } + } + // AGENTS.md item 7: the exit code is the sentinel, never the text. if !errors.Is(err, civitai.ErrUnauthorized) { t.Errorf("a listing 403 must stay tagged ErrUnauthorized (→ exit 3); got %T: %v", err, err) From 5e617eef91aea743f58f494966ec32f807e00f59 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 4 Sep 2026 13:54:04 -0500 Subject: [PATCH 4/5] =?UTF-8?q?fix(appapi):=20audit=20round=203=20?= =?UTF-8?q?=E2=80=94=20the=20image-upload=20attribution=20was=20false,=20t?= =?UTF-8?q?he=20trigger=20missed=20its=20own=20hazard,=20and=20wantRemedy?= =?UTF-8?q?=20pinned=202=20of=203=20clauses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third delta round. Same shape as the previous two: every finding was authored by the round before it. 🟡-1 (payload). Round 2 replaced round 1's false claim with a different false claim. It said /api/v1/image-upload's "only ownership refusal" is "You do not own this image" at block-image-upload.service.ts:360. Measured: that route is src/pages/api/v1/image-upload/index.ts, it performs NO ownership check at all and can only answer 401 or 200 — containment there is currently TOTAL, not luck. The cited string lives in gateBlockUploadImage, whose only caller is a tRPC proc (block-image-upload.router.ts:48) this CLI never calls. Corrected, and the mistake is recorded rather than quietly overwritten, because it is the third instance of the same class in three rounds. 🟡-2 (payload). The trigger was keyed on a LOCATION ("added to that block at the top of listing.go") rather than on the property, and it missed the hazard its own paragraph named. Two arms now: (1) a listingRoute declared ANYWHERE IN THIS PACKAGE — the package's own ledger test says so in its own words, "a route declared in a sibling file of the same package is exactly as reachable"; (2) a refusal CHANGING behind a route already in the set, which no addition-shaped trigger can see and which is the likelier of the two. The premise sentence above it is widened to match. 🟡-3 (scaffolding). wantRemedy's docstring claimed "every clause of the arm's REMEDY" while pinning two of three. The unpinned one was the PRIMARY remedy — "sign in as the app's owner (`civitai whoami` shows who you are)" — which README.md:3567 also repeats verbatim, so a reword would have staled both with no gate. Watched red: deleting only that clause now fails all three not-owned rows; identity control still green. Re-verified: gofmt -s -l clean (the appblocks.go reflow is gofmt's doc-comment list indentation, no content change), go vet clean, 21/21 packages. --- internal/appapi/appblocks.go | 41 +++++++++++++++++------ internal/appapi/listing_forbidden_test.go | 6 ++-- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/internal/appapi/appblocks.go b/internal/appapi/appblocks.go index 890318f..1638b10 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -1934,27 +1934,46 @@ func isModeratorTakedownMsg(msg string) bool { // // 🔴 SAFETY HERE IS ROUTE-POPULATION CONTAINMENT, NOT A PROPERTY OF THE STRING. // The predicate is not scoped to listings, so what keeps it from mis-firing is -// WHICH ROUTES reach `listingError` — and that set is the `listingRoute` block -// at the top of `listing.go`, NOT "the `appListings.*` procs". An earlier draft +// WHICH ROUTES reach `listingError` — and that set is every `listingRoute` +// DECLARED IN THIS PACKAGE (they all sit in one block in `listing.go` today, +// but see arm 1 below), NOT "the `appListings.*` procs". An earlier draft // of this paragraph said the latter and was already false when written: // `listingError` has three call sites, and the third is `MintImageUpload` // (`listing.go`), which passes `ImageUploadPath` — the REST endpoint // `/api/v1/image-upload`, not a tRPC proc at all. // -// So the trigger is mechanical and matches the premise: **re-run the -// enumeration when a route is ADDED TO THAT BLOCK**, whatever its shape. (An -// earlier trigger said "if the CLI routes a NON-LISTING proc through -// listingError" — a different predicate from its own premise, and one that -// would not fire for image-upload, since image-upload *is* a listing route.) +// 🔴 THE TRIGGER HAS TWO ARMS, because the hazard does. An earlier draft had +// only the first and was keyed on a LOCATION rather than on the property: +// +// 1. **A route becomes reachable.** Re-run the enumeration when a +// `listingRoute` is declared ANYWHERE IN THIS PACKAGE — not "added to the +// block in listing.go". `listing_op_test.go` says why in its own words: it +// reads the whole package "because a route declared in a sibling file of +// the same package is exactly as reachable". They all happen to live in +// one block today; that is tidiness, not a constraint. (That ledger test +// does fail on a new route, but its remedy text tells you to classify the +// route's op — it says nothing about re-running THIS enumeration.) +// 2. **A refusal changes behind a route already in the set.** Nothing about +// addition covers this, and it is the likelier of the two: any route here +// whose server side gains a `"you can only …your own"` refusal starts +// being answered by this arm, with the wrong noun. // // The enumeration itself, over the whole server repo rather than one file: // other NOT_OWNED-shaped refusals matching this core ship for challenges, // collections, creator shop and remix gallery — and, closest to this CLI's own // surface, `apps-shared.router.ts:611` ("you can only edit your own -// submissions"). None is on a route in that block today. `/api/v1/image-upload` -// is the one to watch: its only ownership refusal is currently "You do not own -// this image" (`block-image-upload.service.ts:360`), which does NOT match the -// core — so containment there is luck, not design. +// submissions"). None is on a route this package declares today. +// +// ⚠ `/api/v1/image-upload` is the interesting one, and an earlier draft of this +// paragraph got it WRONG in a way worth recording. It claimed that route's +// "only ownership refusal" was "You do not own this image" +// (`block-image-upload.service.ts:360`). Measured: that route is +// `src/pages/api/v1/image-upload/index.ts`, it performs NO ownership check at +// all, and can only answer 401 or 200 — so containment there is currently +// TOTAL, not luck. The cited string lives in `gateBlockUploadImage`, whose only +// caller is a tRPC proc (`block-image-upload.router.ts:48`) that this CLI never +// calls. The hazard is arm 2 above — a 403 being ADDED to that route later — +// not a refusal that is there now. // // Two more NOT_OWNED strings ship in `offsite-moderation.service.ts` (:1668, // :2226) and are unreachable only because the CLI calls none of the procs that diff --git a/internal/appapi/listing_forbidden_test.go b/internal/appapi/listing_forbidden_test.go index 86cf677..517fa01 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -132,7 +132,7 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only manage your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, }, { // A SECOND spelling from the same service (:1323, the edit path). @@ -143,7 +143,7 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only edit your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, }, { // A THIRD spelling (:1811, the revision-submit path). It shares @@ -161,7 +161,7 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only submit your own revision", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, }, { // 🔴 INVARIANT GUARD, not regression coverage — labelled as one for From b80ff72a495107f056449fb521eaf9018d0e377d Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 4 Sep 2026 14:06:29 -0500 Subject: [PATCH 5/5] =?UTF-8?q?test(appapi):=20pin=20the=20WHOLE=20not-own?= =?UTF-8?q?ed=20sentence=20=E2=80=94=20round=203's=20coverage=20claim=20wa?= =?UTF-8?q?s=20still=20one=20step=20short?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the audit ladder. Its only finding, and the last one. wantRemedy's docstring said "every clause of the arm's REMEDY". Round 3 wrote that while pinning two of three steps, and round 4 measured the survivor: deleting `, or ask the owner to make the change` left the FULL suite green (21/21), as did dropping the diagnosis `your account's access is not the problem`. The miscount is the interesting part: round 3 counted "being a moderator does not help" as one of the remedy STEPS. It is a negation of a non-remedy, so treating it as a step made the list look complete while a real step was unlisted. There are three steps — sign in / accept an invite / ask the owner — plus the diagnosis and that negation. All five are now pinned on all three not-owned rows, and the docstring is rewritten to be exactly as wide as what it pins, with the two-round history recorded so the next person does not re-narrow it. That third step matters most for the largest sub-population of this 403: a moderator, a publish-request submitter, or any third party who is neither the owner nor an invitable collaborator. For them the other two steps are both impossible and "ask the owner" is the only one that works — and README.md:3567 repeats it verbatim, so a reword would have staled the published contract too, with no gate. Watched red, each clause dying to its OWN named assertion: dropping "ask the owner to make the change" fails all three rows naming that clause; dropping "your account's access is not the problem" fails all three naming that one. Identity control green, full suite 21/21, gofmt -s -l clean, go vet clean. Ladder stops here on the auditor's own recommendation. Neither mechanical stop condition fired — this round was not clean, and it was the FIRST zero-payload round, not the second — but the fix is three test literals with zero payload consequence, and payload has run 3 -> 1 -> 0 modified lines across the fix rounds while every finding since round 1 has been about the comments and the test table rather than the shipped change. A round 5 over two added string literals would be the ladder auditing itself. --- internal/appapi/listing_forbidden_test.go | 47 ++++++++++++++++++++--- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/internal/appapi/listing_forbidden_test.go b/internal/appapi/listing_forbidden_test.go index 517fa01..62f9743 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -57,8 +57,24 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { // wantAccessClaim is whether the Apps-author-access diagnosis belongs // on this 403 at all. wantAccessClaim bool - // wantRemedy is every clause of the arm's REMEDY that must survive a - // reword. + // wantRemedy is every clause of the arm's DIAGNOSIS and REMEDY that + // must survive a reword — i.e. the whole sentence after the server's + // own message, clause by clause. + // + // 🔴 THE SENTENCE ABOVE IS THE COVERAGE CLAIM, AND IT WAS FALSE TWICE. + // Round 3 of the audit ladder wrote "every clause" while pinning two of + // three, and round 4 measured the survivor: deleting `, or ask the + // owner to make the change` left the FULL suite green (21/21). The + // miscount was treating "being a moderator does not help" — a negation + // of a non-remedy — as one of the remedy STEPS, which hid that a real + // step was unlisted. There are three steps (sign in / accept an invite + // / ask the owner), plus the diagnosis and that negation, and all five + // are listed on every not-owned row now. + // + // So: if you add a clause to the arm, add it here. A guard whose + // DESCRIPTION is wider than its implementation reads as coverage while + // providing none, which is worse than no guard, because it stops the + // next person looking. // // 🔴 IT EXISTS BECAUSE THE REMEDY WAS UNPINNED AND AN AUDIT PROVED IT: // deleting BOTH the collaborator clause and the moderator clause from @@ -132,7 +148,14 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only manage your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{ + "your account's access is not the problem", + "being a moderator does not help", + "sign in as the app's owner", + "civitai whoami", + "accept a pending collaborator invite", + "ask the owner to make the change", + }, }, { // A SECOND spelling from the same service (:1323, the edit path). @@ -143,7 +166,14 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only edit your own listings", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{ + "your account's access is not the problem", + "being a moderator does not help", + "sign in as the app's owner", + "civitai whoami", + "accept a pending collaborator invite", + "ask the owner to make the change", + }, }, { // A THIRD spelling (:1811, the revision-submit path). It shares @@ -161,7 +191,14 @@ func TestListingForbiddenTakedownDoesNotBlameTheAccount(t *testing.T) { msg: "you can only submit your own revision", wantSubstr: "belongs to another account", wantAccessClaim: false, - wantRemedy: []string{"sign in as the app's owner", "civitai whoami", "accept a pending collaborator invite", "being a moderator does not help"}, + wantRemedy: []string{ + "your account's access is not the problem", + "being a moderator does not help", + "sign in as the app's owner", + "civitai whoami", + "accept a pending collaborator invite", + "ask the owner to make the change", + }, }, { // 🔴 INVARIANT GUARD, not regression coverage — labelled as one for