sec(auth): retry-any:purchases has no handler coverage and no non-test reference in internal/api #1743

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/highSignificant harmSignificant harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/manyAffects most usersAffects most users

   effort/sHoursHours

   type/securitySecurity findingSecurity finding

   on Aug 8, 2026
2. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   Correction: this issue's premise is wrong. retry-any:purchases IS enforced.

   Established while investigating the sibling issue #1644. Recording it here before anyone acts on the original framing.

   The verb is demanded on a routed, reachable path:

   • internal/api/handler_purchases.go:1962, inside authorizeSessionRetry (:1952):
     h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRetryAny, auth.ResourcePurchases)
   • Reached from router.go:178 — POST /api/purchases/retry/, Auth: AuthUser
   • {ActionRetryAny, ResourcePurchases} is already in adminCarvedOuts (types.go:118), so a plain admin:* is correctly refused today

   So this is a coverage gap on an enforced control, not a missing control. Nothing needs adding to the carve-out set. The original conclusion — "no handler coverage and no non-test reference in internal/api" — was half right: the coverage gap is real, the missing-reference claim is not.

   Why the original conclusion was wrong, and why it matters beyond this issue

   The instrumentation searched one form of the name. This handler uses the constant auth.ActionRetryAny; the string literal "retry-any" appears only in its own declaration. A literal-only sweep therefore returns zero.

   Its sibling #1644 fails in exactly the opposite direction: the RI-exchange handlers use the bare literal "ri-exchange", while the constant ResourceRIExchange appears only in its own declaration. A constant-only sweep returns zero there.

   There is no safe default form to grep. Either single-form sweep produces a false "unreferenced" on one of the two — and in both cases that is the answer that ends the investigation early. Any future "is this verb enforced anywhere" question must search the constant and the literal, and sanity-check the pattern against a line already known to match before trusting a negative.

   Re-scoped work

   Add handler-level coverage for the retry carve-out — both directions: an admin:* principal refused, and a principal with legitimate retry rights allowed. A refusal-only test passes trivially against a handler that refuses everyone.

   One thing must be established before the test is written. authorizeSessionRetry is the same two-tier shape as approve: retry-any first, falling back to retry-own. On PR #1744 that exact shape meant 11 of 21 repaired tests were denied by the ownership comparison rather than the permission check, because admin:* grants approve-own. If admin:* likewise grants retry-own, a plain-admin refusal test would pass while pinning ownership logic instead of the carve-out.

   So determine by execution which check produces the 403 — the error message discriminates (requires retry-any or retry-own versus an ownership denial) — and build the test so the carve-out is the only thing that can refuse.

   Verify by mutation: remove {ActionRetryAny, ResourcePurchases} from adminCarvedOuts and confirm the new test fails. Per-test, run alone; an aggregate package run misreports this because a mock panic kills the binary.

   Related

   • sec(auth): execute:ri-exchange missing from adminCarvedOuts; admin:* bypasses every exchange guardrail #1644 — execute:ri-exchange, confirmed live bypass (admin:* executes today, constraint gates bypassed too). Genuinely missing from the carve-out set, unlike this one.
   • fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 — grantAdmin now models a real principal (merged as d04cf31c7), so carve-out mutations are no longer silent. That is what makes this testable.
3. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   Experiment: which check produces the 403 — answered, with a twist

   Run against origin/main, driving the routed handler retryPurchase (behind POST /api/purchases/retry/), not authorizeSessionRetry in isolation. Principal is a plain admin:* via grantAdmin().

   Baseline (carve-out present, i.e. today)

   execution's CreatedByUserID	error
   someone else	permission denied: cannot retry another user's failed purchase
   the admin itself	permission constraint check failed: no permission constraint sets provided for execute on purchases

   So on the surface this is the #1744 approve shape: the ownership comparison produces the 403, not a requires retry-any or retry-own message. admin:* grants retry-own, so when the admin owns the row it passes retry authorization entirely and is stopped later by an unrelated downstream execute:purchases gate.

   But the carve-out is load-bearing — mutation proves it

   Removing {ActionRetryAny, ResourcePurchases} from adminCarvedOuts:

   execution's CreatedByUserID	error
   someone else	permission constraint check failed: ... execute on purchases ← changed
   the admin itself	unchanged

   With the carve-out, a plain admin retrying someone else's execution is pushed down the ownership path and refused there. Without it, admin:* grants retry-any, which short-circuits the ownership check entirely and the admin sails past it.

   So the carve-out does not produce the message — it determines whether ownership is consulted at all. That is a meaningful difference from #1744, where the ownership comparison was doing the work independently of the carve-out.

   What this means for the coverage work

   A handler-level guard is writable and does discriminate, but only in a specific shape:

   1. The fixture must use an execution created by someone other than the session user. The own-execution case is useless: retry-own (granted by admin:*) permits it with or without the carve-out.
   2. The assertion must pin the ownership message specifically (cannot retry another user's failed purchase), not merely "an error occurred". Asserting only that an error was returned passes under the mutation too, because the mutated path still errors — just at a different, unrelated gate. That would be a green test pinning nothing, which is the trap this issue family exists to close.
   3. Mutation-verify by removing the pair from adminCarvedOuts, per test, run alone, after committing (a git checkout -- revert of an uncommitted fix silently takes the fix with it).

   Corrected premise, for the record

   The original framing — "zero non-test references in internal/api" — came from a constant-only sweep. auth.ActionRetryAny is referenced at internal/api/handler_purchases.go:1962 inside authorizeSessionRetry, reached from router.go:178. The verb is enforced and already carved out; this is a coverage gap on a working control, not a missing control.

   The general rule, since the two sibling issues fail in opposite directions: grep the constant AND the literal, and sanity-check the pattern against a line you know matches before believing a negative. #1644's handlers use the bare literal "ri-exchange" while its constant appears only in its own declaration; #1743 is the mirror image.

   Handover

   Experiment only, as scoped — no implementation. A successor has everything needed above: the verdict, both reproductions, the mutation result, and the three constraints the test must satisfy.
4. cristim commented on Aug 18, 2026

   

   cristim

   on Aug 18, 2026

   MemberAuthor

   More actions

   Premise correction: the verb IS enforced, and the path is reachable

   Investigated before writing anything, per the fork in the issue. The second branch is the true one: this is a coverage gap, not an enforcement gap. Recording it here so nobody re-derives a conclusion that has already been disproved.

   The "no non-test reference in internal/api" claim is false

   There is one, and it is the real enforcement point:

   internal/api/handler_purchases.go:1962

   hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRetryAny, auth.ResourcePurchases)

   Fully traced back to a routed HTTP request:

   POST /api/purchases/retry/{id}          router.go:178  (Auth: AuthUser)
     -> Router.retryPurchaseHandler        router.go:604
     -> Handler.retryPurchase              handler_purchases.go:1611
     -> loadAndValidateRetryRequest        handler_purchases.go:1672
     -> Handler.authorizeSessionRetry      handler_purchases.go:1699 -> 1952
     -> h.auth.HasPermissionAPI(ActionRetryAny, ResourcePurchases)   handler_purchases.go:1962
     -> auth.Service.permissionsAllow      service_group.go:325
     -> adminCarvedOuts consulted          service_group.go:328
   

   permissionsAllow skips the admin:* short-circuit for any pair in adminCarvedOuts (service_group.go:326-332), mirroring AuthContext.HasPermission (types.go:146-156). So the carve-out genuinely applies in production on this path. No enforcement change is needed and none was made.

   Why the instrumentation recorded zero asks

   The instrumentation counted pairs asked through grantAdmin. The retry handler's tests never go through it. The retry suite's shared fixture buildSessionRetryHandler (internal/api/handler_purchases_test.go:3053-3076) hand-registers the decision itself:

   mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(hasAny, nil).Maybe()

   Note the bare string literal "retry-any", not auth.ActionRetryAny. Grepping the named constant finds the production call site and misses this fixture; grepping the literal finds the fixture and misses production. Both greps were required to see the whole picture.

   The real gap follows from that line: every retryPurchase test dictates the authorization answer as a caller-supplied boolean, so the real matcher, and with it adminCarvedOuts, is never consulted on this path.

   Why this verb specifically, and not its two siblings

   execute:purchases and approve-any:purchases are enforced through requirePermission. retry-any is the only one of the three whose enforcement point calls HasPermissionAPI directly, bypassing requirePermission. That is a structural reason, not an oversight: the handler-level coverage PR #1744 added sits at the requirePermission seam, so it cannot reach the retry path.

   Mutation evidence

   Ran the mutation this issue prescribes (remove the verb from adminCarvedOuts), against the code as it stands:

   internal/api suite: 2166 passed, 2 failed
   [FAIL] TestGrantAdmin_CarveOutIsEnforcedAtHandler/retry-any:purchases
   

   Only the generic requirePermission-level test from #1744 fails. Zero retryPurchase tests notice. That is the gap stated as a measurement.

   A trap worth recording for whoever reviews the fix

   Under that mutation the plain admin is still refused overall, just at a later gate: retryPurchase also runs the SEC-01 constraint gate (enforcePurchaseConstraints), which asks execute:purchases, still carved out, and denies with a different message and still a 403.

   The consequence is sharp: a test asserting only require.Error passes against the mutated code and is worthless. The regression barrier has to assert which gate refused. That is real defense in depth, but it also means the coverage rests on a single message assertion that looks like an incidental detail to anyone reading the diff rather than this thread.

   On the labels

   type/security and severity/high were applied on the premise that this might be an unenforced money verb. That premise does not hold: the control works and always did. The real gap is that a money-path handler suite is blind to carve-out regressions, which is a test-integrity issue in the same family as #1596.

   Flagging for re-triage rather than acting on it. I have not changed any labels.

   Related

   • fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 / PR fix(test): make grantAdmin model the principal instead of stubbing the authorization decision #1744 established the pattern this fix follows; fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 is closed, its remainder (per-handler pair pinning, per-handler .Maybe() decisions, restricted-account coverage) untouched.
   • sec(auth): execute:ri-exchange missing from adminCarvedOuts; admin:* bypasses every exchange guardrail #1644 (ri_exchange_carveout_test.go) is the closest structural sibling.

   Residual gap not closed

   The tests exercise AuthContext.HasPermission through the mock, but production reaches the carve-out via a different function, Service.permissionsAllow. Both consult adminCarvedOuts and both were read to confirm it, but nothing pins them in sync: a regression removing the check from permissionsAllow alone would not be caught. This is pre-existing and identical in shape for the two sibling verbs and for #1644, so it may deserve its own issue.
5. cristim

   closed this as completedin #1853on Aug 19, 2026