diff --git a/README.md b/README.md index 0c9db92..d0634e6 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`. | [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 993dad7..1638b10 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -1886,6 +1886,122 @@ 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. 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 +// (`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". +// +// 🔴 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 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. +// +// 🔴 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 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 +// 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'}` (: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 +// `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") +} + // 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..5373cc4 100644 --- a/internal/appapi/listing.go +++ b/internal/appapi/listing.go @@ -1016,6 +1016,38 @@ 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 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 + // 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: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. + if isNotOwnedMsg(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: 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..62f9743 100644 --- a/internal/appapi/listing_forbidden_test.go +++ b/internal/appapi/listing_forbidden_test.go @@ -57,6 +57,39 @@ 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 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 + // 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, @@ -90,6 +123,123 @@ 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 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{ + "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). + // 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, + 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 + // 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: 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", + wantAccessClaim: false, + 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 + // 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 @@ -144,6 +294,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) 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, ""},