sec(api): approve/cancel discard an authorization denial and fall through — safe only by coincidence #173

Found while reviewing PR LeanerCloud/cloud-commitments-cli#1744. Latent, not live — verified by execution, no bypass exists on any constructible input today. Filed because it is correct only by accident, and the accident is one refactor from ending.

The defect

In approvePurchase (internal/api/handler_purchases.go:565-583), the outer dispatch calls authorizeSessionApprove and then discards a denial:

switch err := h.authorizeSessionApprove(ctx, session, execution); {
case err == nil:
    return h.approvePurchaseViaSession(ctx, req, execution)
case isPermissionDenied(err):
    // empty body, no return -- falls through
default:
    return nil, err
}

if token != "" {
    return h.approveViaToken(ctx, req, execution, token)
}

return h.approvePurchaseViaSession(ctx, req, execution)   // <-- same call as the authorized branch

With token == "", an authorization denial and an authorization success reach the identical call. The outer result changes nothing.

The fall-through itself is deliberate and documented — a logged-in user without approve-* may still be the per-account contact_email recipient, so the token branch gets a chance (PR LeanerCloud/cloud-commitments-cli#101). That intent is fine. What is wrong is that the token == "" case was not excluded from it.

cancelPurchase has the same shape.

Why it is not exploitable today

Safety rests on two independent facts, either of which could change alone:

1. approvePurchaseViaSession calls the literal same authorizeSessionApprove (:685), so the inner check cannot be weaker than the outer one.
2. This route family never caches a context principal. The inner requireSession consults principalFromContext(ctx) first and only falls back to bearer-token ValidateSession; the outer tryGetSession always uses the bearer header. Those could resolve different sessions — except validateSecurityContext returns at if h.isPublicEndpoint(path) before authenticatePrincipal/contextWithPrincipal, and /api/purchases/approve/* and /api/purchases/cancel/* are AuthPublic. So the context lookup always misses and both paths resolve the same session.

Fact 2 is the fragile one. If approve/cancel ever lose AuthPublic status and pick up context-principal caching, inner and outer can resolve different principals and this becomes a live bypass — with no test failing, because nothing currently pins the invariant.

The inner path also adds predicates the outer lacks (CSRF, status must be pending/notified, 4-eyes requireDifferentApprover), all of which only restrict further.

Execution evidence

Three scenarios, each with a valid CSRF token so CSRF cannot mask the outcome (the existing TestApproveViaSession_RequiresCSRF passes an empty token, which is why this was never visible):

• principal holds neither approve-any nor approve-own -> denied, ApproveAndExecute never called
• plain admin, approve-own via wildcard, ownership check fails (CreatedByUserID nil) -> denied, never called
• cancel path, no cancel-any/cancel-own -> denied, never called

Fix

Add an explicit return nil, err in the isPermissionDenied case when token == "", in both approvePurchase and cancelPurchase. The fall-through must remain for the token != "" case — that is the intended contact_email gate.

The test matters more than the fix. Add one pinning the invariant directly: outer denies implies the operation does not occur, with a valid CSRF token, asserting the money-moving call is never reached. Today that invariant holds by construction and nothing checks it, which is why a refactor would break it silently.

Verify by mutation: make the inner authorization permissive and confirm the new test fails. If it still passes, it is pinning the inner check rather than the invariant.

Also needs checking

approveRIExchange (internal/api/handler_ri_exchange.go) has a structurally similar three-mode dispatch but is not identical — it guards with if token == "" || !isPermissionDenied(sessErr) { return nil, sessErr } and passes session=nil rather than reusing the outer session. Different enough that it was not verified either way. Establish whether it is safe as part of this work; do not assume it inherits either the defect or the safety.

retryPurchase was checked and does not apply — single linear path, one requireSession, one RBAC check, no outer/inner duplication.

Related

• LeanerCloud/cloud-commitments-cli#1749 — pre-auth information leak in approvePurchaseViaSession (409 carrying status= before authorization). Same function, different defect: that one is about ordering a response before a check; this one is about discarding a check result. Fix separately.
• PR fix(test): make grantAdmin model the principal instead of stubbing the authorization decision cloud-commitments-cli#1744 — where this surfaced, while declining a test-fixture suggestion.