sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210) #60

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/all-usersAffects every userAffects every user

   effort/mDaysDays

   type/securitySecurity findingSecurity finding

   on Jun 26, 2026
2. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

   Re-derived independently during the 2026-07-28 full review of internal/api and confirmed still live on be11bdcb5. This issue is the right home; the review adds a complete census that is wider than the three verbs in the title, plus a sharper failure scenario. Please widen the acceptance criteria (or spawn one follow-up) rather than closing after the three named sites.

   Census: requirePermissionConstraints has exactly two call sites in the whole package

   Present:

   • internal/api/handler_purchases.go:2101 (enforcePurchaseConstraints, used by direct-execute and retry)
   • internal/api/handler_ri_exchange.go:774 (executeExchange)

   Missing, all of them mutating and most of them money-moving:

   Handler	Where
   approvePurchaseViaSession	internal/api/handler_purchases.go:665
   approveViaToken	internal/api/handler_purchases.go:588
   runPlannedPurchase	internal/api/handler_purchases.go:302
   upsertLadderConfig	internal/api/handler_ladder.go:77
   updateConfig	internal/api/handler_config.go:59
   updateRIExchangeConfig	internal/api/handler_ri_exchange.go:963
   marketplaceList	internal/api/handler_marketplace.go:139
   revokePurchase	internal/api/handler_purchases_revoke.go:139

   This issue names execute-any/own, approve-any/own and runPlanned. The five sites below those (ladder config, global config, RI-exchange config, marketplace listing, revoke) are the same fail-open class on the same Constraints struct and are not covered anywhere else. marketplaceList and revokePurchase in particular move money in the seller/refund direction, where MaxPurchaseAmount is not the relevant constraint but Providers / AccountIDs are.

   Sharper failure scenario for the approve path

   An admin grants U approve-any:purchases with Constraints{MaxPurchaseAmount: 1000, Regions: ["eu-west-1"]}. The UI presents this as a $1,000 ceiling.

   • U cannot direct-execute a $50,000 us-east-1 purchase: blocked at handler_purchases.go:2101.
   • U can approve a pending $50,000 us-east-1 execution created by anyone else: approvePurchaseViaSession calls authorizeSessionApprove and then purchase.ApproveAndExecute with no constraint evaluation at all.

   The configured cap is silently inert on the one path that actually spends the money, while appearing enforced on the path that does not. runPlannedPurchase has the identical shape for scheduler-created rows.

   Note on the two independent gates

   Worth stating explicitly in the fix, because they are easy to conflate: getAllowedAccounts (session/group scope) and requirePermissionConstraints (per-permission Constraints) are independent gates. A permission with no AccountIDs constraint satisfies the second unconditionally (empty-permission-side rule, internal/api/handler.go:450-461), so passing requirePermissionConstraints is not evidence that account scoping happened. Several handlers in this package satisfy exactly one of the two gates and read as guarded.

   Fix direction (unchanged in shape, wider in coverage)

   Build the constraint set from the execution's stored Recommendations (reuse purchaseConstraintSets, handler_purchases.go:2122-2183, whose evasion paths are already closed) and call requirePermissionConstraints("approve", "purchases", ...) on both approve branches and in runPlannedPurchase. Then do the same for the five config/marketplace/revoke sites with whichever constraint fields apply.

   ---

   The 2026-07-28 full-repo review re-derived this and confirmed all three paths listed here are still unguarded on be11bdcb5. Two additions: two more money verbs that are not in this issue's scope list, and a concrete operator-facing scenario worth attaching to the acceptance criteria.

   Where

   • internal/api/handler_purchases.go:2100 (enforcePurchaseConstraints), called only from internal/api/handler_purchases.go:2086 and internal/api/handler_purchases.go:1627
   • internal/api/handler_purchases.go:307 (run-now path)
   • internal/api/handler_purchases.go:350 (session-authed approve path)
   • internal/auth/service_api.go:359 (HasPermissionAPI passes constraints = nil)
   • internal/auth/service_group.go:232 (checkPermissionConstraints)

   What the review adds

   Two more verbs belong in the scope list. This issue covers execute-any / execute-own, approve-any / approve-own, and runPlanned. The review found the same bare-verb gate on the revoke and sell paths, which are also irreversible money actions. Whatever requirePermissionConstraints wiring lands here should cover all of them in one pass, otherwise the next review finds the same class again on the two that were left out.

   PermissionConstraints is a fully surfaced product feature, which raises the cost of the gap beyond "a cap is not applied". All five dimensions (MaxPurchaseAmount, AccountIDs, Providers, Services, Regions) are configurable per group permission and are round-tripped through the API (internal/auth/service_api.go:51) and the group editor UI. So an operator can build a restricted role, see it stored and rendered, and reasonably believe it is enforced.

   Failure scenario

   An operator creates a deliberately restricted role: a custom group holding approve-any:purchases constrained to MaxPurchaseAmount: 100 and AccountIDs: [dev-account]. A member of that group opens Purchase History and approves a pending $500,000 production execution. The constraint is stored, is rendered back in the group editor, and is silently ignored. The operator has a control that reads as enforced and is not, which is worse than not offering the control.

   Worth adding as a regression test alongside the TestHandler_executePurchase_PermissionConstraintsDenied mirrors already proposed here: a session that passes the bare approve verb but whose MaxPurchaseAmount rejects the execution must return 403 before ApproveAndExecute fires.

   Note on the failure direction

   As this issue already states, these paths do not merely skip the check, they affirmatively pass it: HasPermissionAPI supplies constraints = nil and checkPermissionConstraints returns true whenever the request-side constraints are nil. Recording it again only because it argues for the second half of the fix: checkPermissionConstraints should fail closed when the matched permission carries constraints but the caller supplied no request-side sets. Otherwise any future call site that forgets to build the sets inherits the same silent allow.
3. cristim

   transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026
4. cristim

   added 4 commits that reference this issue on Sep 28, 2026

   sec(api): make approve constraint denial terminal, close token-flow b…

   ...

   da30659

   sec(api): make approve constraint denial terminal, close token-flow b…

   ...

   8efd142

   test(api): cover runPlanned constraint and zero-commitment guards (#60)

   0060cfd

   test(api): assert runPlanned funnel not called on a locally typed mock (

   ...

   225a829
5. cristim

   closed this as completedin #378on Sep 28, 2026