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

[cristim]

retry-any:purchases is a #923 carved-out money verb with no handler-level coverage of any kind, and no non-test reference in internal/api at all.

Found by instrumenting grantAdmin to record every (action, resource) pair the whole internal/api suite actually asks for. Thirty distinct pairs came back. retry-any:purchases was asked zero times.

Why this is distinct from #1596

#1596 covers the two carved-out verbs that are exercised — execute:purchases (12 asks) and approve-any:purchases (23 asks) — but exercised through grantAdmin, which answers true for them. Since production refuses an admin:* holder those verbs, those 24 tests assert behaviour for a principal that cannot exist. #1596 repairs that.

This verb is a different shape: it is not covered by an impossible principal, it is not covered at all. Fixing grantAdmin will not create coverage that was never written.

What is unclear and needs establishing first

The zero references in non-test internal/api code raise a question the fix depends on:

• Is the verb enforced somewhere else — the service layer, a different package — with the handler simply never consulting it? Then the gap is coverage, and the fix is a test.
• Or is it not enforced on any live path, in which case the gap is the enforcement itself and a test would be asserting a control that does not exist.

Resolve that before writing anything. Grep for the constant across the whole repo, not just internal/api, and trace whichever call sites exist back to a reachable request path.

If it is enforced elsewhere

Add handler-level coverage matching whatever #1596 lands for its two siblings: an admin:* principal must be refused, and a principal with legitimate Purchaser membership must be allowed. Both directions — a refusal-only test passes trivially against a handler that refuses everyone.

If it is not enforced

That is a live separation-of-duties gap rather than a test gap, and it should be re-triaged accordingly. retry-any is the verb that re-drives a purchase; combined with #1668 and #1718 — where retry successors could buy a second uncancellable Azure savings plan — an unenforced retry verb is not a paperwork problem.

Verification

Whichever branch it lands in, mutation-verify: remove the verb from adminCarvedOuts and confirm the new test fails. Today that mutation is entirely silent for all three carved-out verbs, which is the condition #1596 exists to end.

Related

• fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 — grantAdmin stubs the authorization decision; the two sibling verbs are covered by an impossible principal.
• Add Purchaser group + remove implicit admin grant for execute/approve-any/retry-any purchases (separation of duties) #923 — the separation-of-duties carve-out itself.
• sec(auth): the #923 money separation-of-duties carve-out is voidable by any admin in one request #1550 / PR sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737 — the service-layer grant ceiling defending the same control.
• sec(auth): execute:ri-exchange missing from adminCarvedOuts; admin:* bypasses every exchange guardrail #1644 — execute:ri-exchange is missing from adminCarvedOuts entirely; a related but distinct hole in the same set.

[cristim]

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.

[cristim]

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.

[cristim]

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.