fix(appapi): an ownership 403 is not an access problem — stop printing the Apps-author remedy - #522
Conversation
…g the Apps-author remedy 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.
…ent, and add the README row 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.
|
Round 1 audit findings addressed in |
…orrected claim survived in two more copies, and the remedy was unpinned
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.
|
Round 2 (delta) findings addressed in |
…the trigger missed its own hazard, and wantRemedy pinned 2 of 3 clauses
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.
|
Round 3 (delta) findings addressed in |
…laim was still one step short 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.
|
Round 4 (delta) finding addressed in Why the ladder stops, stated rather than implied. Neither mechanical stop condition fired: this round was not clean, and it was the FIRST zero-payload fix round, not the second (round 2's fix changed 1 payload line, round 3's changed 0 — an earlier framing of mine said otherwise and the round-4 auditor correctly refused to carry it). It stops because the remaining fix was three test literals with zero payload consequence, payload has run 3 → 1 → 0 modified lines across the fix rounds, and every finding since round 1 has been about the doc comments and the test table rather than the shipped change. A round 5 over two added string literals would be the ladder auditing itself. Recorded as open, not absent — two over-statements round 4 measured and deliberately did not file, so a later reader knows they were seen: |
The bug
listingError's 403 arm ends in a catch-all that appends "— managing store listings needs Apps-author access (invite-only beta)" to every non-scope, non-takedown 403. One of those is the server'sNOT_OWNEDrefusal, where the caller's Apps-author access is fine and is not what failed.Third instance of the wrong-subject class this package already catalogs: #374/#391 on the 400 arm, #509 on the takedown arm, ownership here. The comment above the fallback already names the class — this is the case it did not yet cover.
Why it is definitely not an access problem
Measured 2026-09-03 against a real listing. The refusing account was a moderator and the publish request's
submitted_by_user_id, and was still refused:resolveListingAccess→appBlock.app.userId) — not the listing's denormalizeduser_id, and notapp_block_publish_requests.submitted_by_user_id;So no grant, re-login or cohort invite changes the outcome, and the remedy the CLI printed was unreachable.
Why the classifier reads the message
The wire carries no distinct code:
mapOffsiteError(app-listings.router.ts:359) collapsesNOT_OWNEDandFORBIDDENonto the same TRPCError code, so the discriminator is gone before the CLI sees it. That is the identical constraintisInsufficientScopeMsgandisModeratorTakedownMsgalready document, andisNotOwnedMsgis written in their style.Matched on the core shared by all three reachable spellings — they share no noun and no verb:
offsite-listing.service.ts:2467you can only manage your own listings:1323you can only edit your own listings:1811you can only submit your own revisionA fourth,
:582 you can only withdraw your own publish requests, is unreachable here — its own route (api/v1/blocks/withdraw.ts:229) mapsNOT_OWNEDto a 404 whose body is identical toNOT_FOUND, deliberately, so it never arrives as a 403.Tests
Four rows added to
TestListingForbiddenTakedownDoesNotBlameTheAccount— red atbf4db4b(all four fail onwantAccessClaim, the five pre-existing rows still pass), green here.A mutation sweep then found something worth fixing in the tests themselves: the predicate's two clauses were each individually redundant — dropping either one left every row green. Two further rows were added to pin the narrowness, built from real shipped near-miss strings rather than invented ones. Each mutant is now killed by exactly its own row, and no other:
invariant: the not-owned core is matched case-insensitively"you can only "invariant: a non-ownership 'your own' refusal is NOT not-owned(You cannot purchase access to your own chapter.)"your own"invariant: a non-ownership 'you can only' refusal is NOT not-owned(You can only request 2 email changes per day.)The near-miss rows are labelled invariant guards, not regression coverage — neither string can reach this arm today. What they pin is that a widening to a bare
"you can only"prefix would start telling a rate-limited user their listing belongs to someone else.One fixture retargeted
TestFallbackFailuresKeepTheirOwnError's 403 row used"you can only manage your own listings"incidentally — its own comment sayswantMsgis "a fragment of the FALLBACK's own error, taken fromlistingError's arm for that status". That string now has its own arm, so the fixture is changed to a cohort refusal, which is the arm itswantMsgnames. The row's subject — the fallback's error surviving instead of inheriting the submission's not-found — is unchanged, and the assertion string is untouched. The not-owned routing is pinned by the dedicated test instead.Verification
red at bf4db4b, green at HEADfor the four regression rows.gofmt -lclean,go vet ./...clean.ErrUnauthorized→ exit 3, asserted witherrors.Isper AGENTS.md item 7, never on text.