fix(auth): enforce per-permission constraints on execute paths

[cristim]

Problem

Closes #1141 (review finding SEC-01, P1).

Per-permission Constraints (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) configured on a group's execute:purchases or execute:ri-exchange permission were never enforced at execution time. HasPermissionAPI hardcoded nil request-side constraints, so checkPermissionConstraints short-circuited to allow and the constraint matchers (matchPurchaseAmountConstraint, the Providers/Services/Regions/AccountIDs matchers) were unreachable dead code on every request path. A group member with a constrained execute permission could execute a purchase of any size (up to the hardcoded global $10M sanity cap), in any provider/service/region: a silent fail-open on a money-mutation path, while the configured cap appeared set in the UI.

Fix

• internal/auth/service_api.go: new Service.HasPermissionForConstraintsAPI(ctx, userID, action, resource, constraintSets) evaluates every request-derived constraint set against the user's effective permissions (single permission fetch; each set must be granted by at least one permission, matching the union semantics of group grants). Fails closed: an empty set list returns an explicit error, never an allow.
• internal/auth/service_group.go: HasPermission's matching loop is extracted into permissionsAllow so the batch caller reuses the exact same matching logic (admin short-circuit, action/resource match, constraint match) instead of duplicating it.
• internal/api/handler.go: new Handler.requirePermissionConstraints re-checks an already-authenticated session against constraint sets, returning 403 on denial. The stateless admin API key bypasses it exactly as it bypasses requirePermission's per-user lookup.
• internal/api/handler_purchases.go: validateExecutePurchaseRequest now builds one constraint set per recommendation (single-value provider/service/region/account lists, so a batch cannot pass on the strength of one in-scope rec) with the batch's total upfront cost as the amount on every set (so the cap cannot be evaded by splitting a purchase across recs), and enforces them before any execution is persisted.
• internal/api/handler_ri_exchange.go: executeExchange enforces a constraint set carrying the AWS EC2 scope, the request's region, and the max_payment_due_usd guardrail as the amount (which ExecuteExchange independently enforces against the actual quoted payment due).
• internal/api/types.go / internal/server/app.go / internal/api/mocks_test.go: interface, adapter, and test mock wiring.

Any residual token mismatch denies (fail closed); the request side normalizes provider casing before the constraint sets are built.

Test evidence

• Regression tests replicate the real failing scenario: a session that passes the bare verb/resource gate (exactly what happens for a constrained permission) but whose constraints reject the request must 403 before any execution is persisted or any exchange is submitted.
  • TestHandler_executePurchase_PermissionConstraintsDenied (also pins the constraint-set shape: one set per rec, batch total upfront on each)
  • TestExecuteExchange_PermissionConstraintsDenied
  • TestService_HasPermissionForConstraintsAPI (over-cap denied, in-scope allowed, out-of-scope provider denied, batch with one out-of-scope set denied, admin allowed, empty sets error, store error denies)
• Confirmed both handler regression tests FAIL against the pre-fix code (production changes stashed, tests kept): the purchase test fails with the constraint check never called, the exchange test reaches AWS instead of returning 403.
• go build ./... clean; go test ./internal/api/... ./internal/auth/... ./internal/server/... -count=1: 2561 passed, 0 failed; go vet and gofmt clean.

Summary by CodeRabbit

• New Features

  • Added execution-time, constraint-aware authorization for purchase execution and RI exchange, validating per-item/provider/service/region/account scope (with “unattributed” fallback) and enforcing max purchase/payment amounts.
  • Prevented approval by splitting amounts across multiple items.

• Bug Fixes

  • Fail-closed behavior when attribution is incomplete or constraint evaluation fails; denied requests now return 403.

• Tests

  • Added regression and unit coverage for constraint-set denial, exact constraint payloads (including always-populated account dimension), and fail-closed exchange/purchase scenarios.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7a77a74f-aafe-4b52-8efb-716d943203b7

📥 Commits

Reviewing files that changed from the base of the PR and between 0256828 and 91ba9ff.

📒 Files selected for processing (12)

• internal/api/handler.go
• internal/api/handler_per_account_perms_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/api/types.go
• internal/auth/service_api.go
• internal/auth/service_api_test.go
• internal/auth/service_group.go
• internal/server/app.go

🚧 Files skipped from review as they are similar to previous changes (12)

• internal/api/handler_per_account_perms_test.go
• internal/server/app.go
• internal/api/types.go
• internal/auth/service_api.go
• internal/api/handler_ri_exchange.go
• internal/api/handler.go
• internal/api/handler_purchases.go
• internal/api/handler_ri_exchange_test.go
• internal/auth/service_group.go
• internal/api/mocks_test.go
• internal/api/handler_purchases_test.go
• internal/auth/service_api_test.go

---

📝 Walkthrough

Walkthrough

Adds execution-time permission constraint enforcement for purchase and RI-exchange requests. The change introduces a constraint-aware auth API, shared handler checks, request-derived purchase and exchange constraints, unattributed-account handling, and regression coverage.

Changes

Constraint-aware permission enforcement

Layer / File(s)	Summary
Auth service constraint API internal/api/types.go, internal/auth/*, internal/server/app.go	Adds HasPermissionForConstraintsAPI, reuses permissionsAllow for constraint matching, and tests amount, provider, region, admin, empty-input, and store-error behavior.
Handler constraint gateway internal/api/handler.go, internal/api/mocks_test.go, internal/api/handler_per_account_perms_test.go	Adds fail-closed request constraint checks, admin-key bypass behavior, the unattributed-account sentinel, and supporting auth mocks.
Purchase constraint enforcement internal/api/handler_purchases.go, internal/api/handler_purchases_test.go	Builds per-recommendation constraints with batch purchase caps and populated account dimensions, then rejects denied executions with HTTP 403.
RI exchange constraint enforcement internal/api/handler_ri_exchange.go, internal/api/handler_ri_exchange_test.go	Applies AWS/EC2, region, account, and payment-cap constraints, including fail-closed account resolution and unattributed-account handling.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210) #1300: Extends the same execution-time constraint mechanism to additional purchase execution and approval paths.

Possibly related PRs

• LeanerCloud/CUDly#79: Modifies the RI-exchange account-resolution flow used by this PR’s constraint enforcement.
• LeanerCloud/CUDly#309: Introduced the per-account permission test fixture extended here with constraint-check expectations.
• LeanerCloud/CUDly#655: Modifies the purchase execution validation path extended here with permission constraints.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PurchaseHandler
  participant AuthService
  participant PermissionStore
  Client->>PurchaseHandler: execute purchase or RI exchange
  PurchaseHandler->>AuthService: validate base permission
  AuthService->>PermissionStore: load effective permissions
  PermissionStore-->>AuthService: permissions
  PurchaseHandler->>AuthService: evaluate request constraints
  AuthService-->>PurchaseHandler: allowed or denied
  PurchaseHandler-->>Client: execute operation or HTTP 403

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	#1141 is addressed, but the directly linked AWS IAM onboarding issue #39 is not implemented in this changeset.	Implement the AWS IAM console deep link, new-tab rel attributes, and tests from #39, or remove it from the linked scope.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: enforcing permission constraints on execute paths.
Out of Scope Changes check	✅ Passed	The changes stay focused on permission-constraint enforcement, interface wiring, and tests for the execute paths.
Docstring Coverage	✅ Passed	Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sec-01-enforce-permission-constraints

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

internal/api/handler.go (1)

246-271: 💤 Low value

Consider adding a defensive nil check for the session parameter.

The method dereferences session.UserID at line 257 without checking if session is nil. While the documented calling pattern requires callers to pass the session from requirePermission (which returns non-nil on success), a defensive nil check would catch programmer errors more gracefully.

🛡️ Suggested defensive nil check

 func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, action, resource string, constraintSets []auth.PermissionConstraints) error {
+	if session == nil {
+		return fmt.Errorf("internal error: session is nil")
+	}
 	if session.UserID == apiKeyAdminUserID {
 		return nil
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler.go` around lines 246 - 271, The function
requirePermissionConstraints dereferences session.UserID without checking
session for nil; add a defensive nil check at the start of
requirePermissionConstraints (before comparing session.UserID to
apiKeyAdminUserID) to detect a nil session and return an appropriate error
(e.g., an internal/server error indicating a missing session) so programmer
errors are caught gracefully; update any tests or callers if necessary to
preserve behavior when session is non-nil.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 1552-1583: The helper purchaseConstraintSets drops the AccountIDs
dimension when rec.CloudAccountID is empty, which lets the auth matcher treat
the account constraint as satisfied; always populate
PermissionConstraints.AccountIDs: when rec.CloudAccountID is non-empty set
c.AccountIDs = []string{*rec.CloudAccountID}, and when nil/empty set
c.AccountIDs to an explicit non-nil sentinel slice (e.g. []string{""}) so the
AccountIDs field is present and cannot be interpreted as omitted by the auth
matcher.

In `@internal/api/handler_ri_exchange.go`:
- Around line 740-751: The permission-constraint block passed to
h.requirePermissionConstraints is missing AccountIDs so account-scoped
execute:ri-exchange permissions can be bypassed; modify the constraints literal
in this call (the []auth.PermissionConstraints entry) to include AccountIDs
populated from the current session (e.g., AccountIDs:
[]string{session.AccountID}) so the deployment account is enforced by the auth
check.

---

Nitpick comments:
In `@internal/api/handler.go`:
- Around line 246-271: The function requirePermissionConstraints dereferences
session.UserID without checking session for nil; add a defensive nil check at
the start of requirePermissionConstraints (before comparing session.UserID to
apiKeyAdminUserID) to detect a nil session and return an appropriate error
(e.g., an internal/server error indicating a missing session) so programmer
errors are caught gracefully; update any tests or callers if necessary to
preserve behavior when session is non-nil.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 434a3d76-23a6-4d69-9784-02206e098421

📥 Commits

Reviewing files that changed from the base of the PR and between cfead3b and d47432e.

📒 Files selected for processing (12)

• internal/api/handler.go
• internal/api/handler_per_account_perms_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/api/types.go
• internal/auth/service_api.go
• internal/auth/service_api_test.go
• internal/auth/service_group.go
• internal/server/app.go

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/api/handler_purchases_test.go (1)

3360-3363: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert provider and service on the second constraint set too.

The matcher currently verifies sets[1].Regions but not sets[1].Providers or sets[1].Services, so a regression that drops those dimensions from the second recommendation would still pass.

🧪 Proposed matcher tightening

 			return assert.ObjectsAreEqual([]string{"aws"}, sets[0].Providers) &&
 				assert.ObjectsAreEqual([]string{"ec2"}, sets[0].Services) &&
 				assert.ObjectsAreEqual([]string{"us-east-1"}, sets[0].Regions) &&
+				assert.ObjectsAreEqual([]string{"aws"}, sets[1].Providers) &&
+				assert.ObjectsAreEqual([]string{"ec2"}, sets[1].Services) &&
 				assert.ObjectsAreEqual([]string{"eu-west-1"}, sets[1].Regions)
 		})).Return(false, nil)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases_test.go` around lines 3360 - 3363, The
assertion block in the matcher is incomplete because it validates
sets[0].Providers, sets[0].Services, and sets[0].Regions but only validates
sets[1].Regions without checking sets[1].Providers and sets[1].Services. Add
assertions for sets[1].Providers and sets[1].Services using the same
assert.ObjectsAreEqual pattern used for the other fields to ensure the second
constraint set is fully validated and prevent regressions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 3360-3363: The assertion block in the matcher is incomplete
because it validates sets[0].Providers, sets[0].Services, and sets[0].Regions
but only validates sets[1].Regions without checking sets[1].Providers and
sets[1].Services. Add assertions for sets[1].Providers and sets[1].Services
using the same assert.ObjectsAreEqual pattern used for the other fields to
ensure the second constraint set is fully validated and prevent regressions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 77211abf-7c86-4d91-b268-f0a55683d2d7

📥 Commits

Reviewing files that changed from the base of the PR and between d47432e and e3d1f72.

⛔ Files ignored due to path filters (3)

• go.sum is excluded by !**/*.sum
• providers/azure/go.sum is excluded by !**/*.sum
• providers/gcp/go.sum is excluded by !**/*.sum

📒 Files selected for processing (11)

• go.mod
• internal/api/handler.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/config/store_postgres_recommendations_test.go
• internal/database/postgres/testhelpers/postgres.go
• internal/server/integration_test.go
• providers/azure/go.mod
• providers/gcp/go.mod

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/api/handler.go
• internal/api/handler_ri_exchange.go

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/config/store_postgres_recommendations.go (1)

338-350: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use explicit OnDemandCost before rejecting Term == 0.

Line 338 makes records with a valid provider OnDemandCost look “uncomputable” when Term is zero, even though Term is only needed for amortizing upfront cost during reconstruction. Because ListStoredRecommendations keeps rows when recEffectiveSavingsPct returns ok=false, this can let records bypass a min_savings_pct floor despite having a usable canonical baseline.

🐛 Proposed fix

 func recOnDemandBaseline(rec *RecommendationRecord) (float64, bool) {
-	if rec.Term == 0 {
-		return 0, false
-	}
 	hasOnDemand := rec.OnDemandCost != nil && *rec.OnDemandCost > 0
+	if hasOnDemand {
+		return *rec.OnDemandCost, true
+	}
+	if rec.Term == 0 {
+		return 0, false
+	}
 	if !hasOnDemand && rec.MonthlyCost == nil {
 		return 0, false
 	}
 	if rec.Provider == providerAWS && !hasOnDemand {
 		return 0, false
 	}
-	if hasOnDemand {
-		return *rec.OnDemandCost, true
-	}
 	amortized := rec.UpfrontCost / float64(rec.Term*12)
 	return *rec.MonthlyCost + rec.Savings + amortized, true
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/store_postgres_recommendations.go` around lines 338 - 350,
The function is rejecting records with a zero Term value too early, before
checking if the record has a usable OnDemandCost that doesn't require Term for
computation. Reorganize the logic so that the Term == 0 check (line 338) is
performed after verifying whether OnDemandCost is available. Specifically, move
the Term == 0 rejection to only apply when OnDemandCost is not usable (when
either hasOnDemand is false or the record lacks alternative cost data), since
Term is only needed for amortizing upfront costs, not for returning a direct
OnDemandCost value. This way, records with valid OnDemandCost values will not be
incorrectly rejected based on missing Term information.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/config/store_postgres_recommendations.go`:
- Around line 338-350: The function is rejecting records with a zero Term value
too early, before checking if the record has a usable OnDemandCost that doesn't
require Term for computation. Reorganize the logic so that the Term == 0 check
(line 338) is performed after verifying whether OnDemandCost is available.
Specifically, move the Term == 0 rejection to only apply when OnDemandCost is
not usable (when either hasOnDemand is false or the record lacks alternative
cost data), since Term is only needed for amortizing upfront costs, not for
returning a direct OnDemandCost value. This way, records with valid OnDemandCost
values will not be incorrectly rejected based on missing Term information.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 58cc39fa-1e79-4330-b9d3-da6d2e8d77eb

📥 Commits

Reviewing files that changed from the base of the PR and between e3d1f72 and 165d1f2.

📒 Files selected for processing (3)

• internal/api/handler.go
• internal/api/handler_purchases_test.go
• internal/config/store_postgres_recommendations.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/api/handler.go
• internal/api/handler_purchases_test.go

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

Adversarial review (PR #1210)

Reviewed against the SEC-01 brief surfaces (constraint encoding, every-handler enforcement, update-any over-broad bypass, per-constraint cross-product, fail-open via missing AccountIDs / silent fallbacks, PII in logs). The SEC-01 fix itself is sound and CR's three prior rounds (AccountIDs sentinel, nil-session guard, sets[1] provider/service assertions, Term-guard restoration) are clean. Found one merge-breaking rebase regression in scope that I just pushed, and one out-of-scope class extension I filed as a follow-up.

Verified

• HasPermissionForConstraintsAPI shape: empty constraintSets returns an explicit error (internal/auth/service_api.go:367-370), not (true, nil). Single permission fetch + per-set evaluation via the extracted permissionsAllow matches the documented union semantics ("every set must be granted by at least one permission"). ✓
• requirePermissionConstraints fail-closed: nil session, nil auth service, lookup error all return an error before any allow; admin API-key sentinel bypass is documented + consistent with requirePermission (handler.go:269-285). ✓
• purchaseConstraintSets fail-closed shape: every set carries single-value Provider/Service/Region/AccountIDs (so the auth matcher's any-overlap becomes strict containment and a batch cannot pass on one in-scope rec); AccountIDs is ALWAYS populated with unattributedAccountConstraint when rec.CloudAccountID is nil/empty, defeating the matcher's "empty request list = satisfied" fail-open; MaxPurchaseAmount carries the BATCH total upfront so a split-across-recs evasion is closed. ✓
• RI-exchange constraint set: explicit string(common.ProviderAWS) / string(common.ServiceEC2) (not bare literals — matches feedback_sdk_enum_string_literals.md); resolveReshapeCloudAccountID failure fails the request closed (handler_ri_exchange.go:751-754); unattributed sentinel when no registered account maps. ✓
• Constraint check ordering on the web execute path: runs AFTER validatePurchaseRecommendation (which strings.ToLowers rec.Provider) so provider casing is normalized before the matcher sees it. ✓ Region and Service are TrimSpace-only — the matcher uses exact-match so Service="EC2" would containsAny against permission Services=["ec2"] and fail-closed (denial, not bypass), so this is a UX paper cut not a security gap.
• unattributedAccountConstraint = "unattributed": a real cloud-account ID is a UUID, so the sentinel cannot collide. ✓
• maxRat.Float64() precision: a huge MaxPaymentDueUSD overflows to +Inf which fails any finite MaxPurchaseAmount cap (+Inf > permMax → false → DENY); a negative or zero value is the caller's problem (ExecuteExchange independently enforces against the actual quote). ✓
• Test fail-closed proof: every regression test confirmed the documented "FAILS against pre-fix code" property (the purchase test mock has NO expectation registered for HasPermissionForConstraintsAPI in the assertion path it pins, so pre-fix code that never calls the constraint check passes the bare gate and reaches store writes — assertions then catch the missed denial). ✓
• No PII in logs / no silent fallbacks on money paths: handler_purchases.go / handler_ri_exchange.go log execution IDs (UUIDs) and timing, no emails or amounts in any of the SEC-01-added branches. ✓

Fixed in-scope and pushed

• afa683a6e — rebase onto current origin/main: the PR was branched off 2d4d6ae63 (1 commit behind origin/main). Critically, the missing commit 5894580f3 fix(db): renumber audit_actor_stamps migration 074->077 is the one that ADDS 000077_audit_actor_stamps.{up,down}.sql to main. The PR's "drop stale migration files" chore (then 0256828) removed the old 000074_audit_actor_stamps.* files on the branch, assuming the renumbered 000077 ones would be picked up from the rebase target — but the rebase target predated the renumber, so the rename never landed. Effect: the PR's diff vs origin/main was DELETING 000077_audit_actor_stamps.{up,down}.sql, which would silently strip the transitioned_by / transitioned_at columns on purchase_executions, ri_exchange_history, and account_registrations on every fresh DB built from main after merge. Runtime code at internal/config/store_postgres.go:904 (TransitionExecutionStatus UPDATE clause), :2240 (RI-exchange transition), and internal/config/store_postgres_registrations.go:185 writes these columns on every state transition — so a fresh-DB deploy would 500 on every approve / cancel / planned-run / RI-exchange action. Rebased onto origin/main, kept the renamed 000077 files, force-pushed. go build ./... clean; go test ./internal/api/... ./internal/auth/... ./internal/server/... -count=1 → 2573 passed; gofmt -l . clean; go vet ./... clean.

Follow-up filed

• sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210) #1300 — sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210). The same fail-open class PR fix(auth): enforce per-permission constraints on execute paths #1210 fixed for execute:purchases and execute:ri-exchange still exists on the sibling money-spending verbs:

  • authorizeSessionExecuteDirect (handler_purchases.go:792) bare-checks execute-any / execute-own without consulting their Constraints → a permission capped tighter than the upstream execute:purchases can still direct-execute past its own cap.
  • authorizeSessionApprove (handler_purchases.go:592) bare-checks approve-any / approve-own; approvePurchaseViaSession then synchronously calls purchase.Manager.ApproveAndExecute which fires the AWS purchase — the approve path IS an execute path and its Constraints are silently ignored.
  • runPlannedPurchase (handler_purchases.go:306) doesn't re-evaluate the current session's execute:purchases Constraints against the persisted recommendations — relevant when an update-any:purchases holder runs another user's plan.

  Same fix pattern: build purchaseConstraintSets against the (live or rehydrated) recommendations and call requirePermissionConstraints against the verb actually being authorized. Triage: type/security, priority/p1, severity/high, urgency/this-sprint, impact/all-users, effort/m, triaged.

UNSTABLE state

Pre-existing failing checks on main (none caused by this PR):

• Lint Code: widespread errcheck violations across cmd/configure_azure.go, cmd/configure_gcp.go, cmd/cleanup-lambda/main.go, cmd/rekey/main.go, internal/api/handler.go (multiple), internal/api/handler_accounts.go, internal/api/handler_dashboard.go, internal/api/handler_recommendations_refresh.go — same set that's been failing on main for the last few PRs in this sweep.
• Integration Tests / Security Scanning / CI Success: pre-existing main failures, unrelated to the auth/permission diff.

Not caused by this PR. Not blocking.

Verdict

LGTM after the rebase fix above. CR re-ping posted (@coderabbitai review).

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim Thanks — this is a very strong adversarial pass.

Good catch on the rebase regression around 000077_audit_actor_stamps; that would have been a nasty fresh-DB break, and I appreciate you fixing it in-scope. The SEC-01 validation notes are also helpful, especially the fail-closed properties around empty constraint sets, unattributed account handling, and batch-total enforcement in purchaseConstraintSets.

Noted as well that the remaining risk has been split cleanly into follow-up issue #1300 for the sibling money paths (execute-any/own, approve-any/own, and runPlanned), which keeps this PR scoped appropriately.

ᕱᕱ Thanks for the thorough rabbit-hole dive.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1210

Timestamp: 2026-06-26T15:49:21.321Z
Learning: In this repository's Go auth flow, execution-time permission constraint checks must fail closed: `internal/auth/service_api.go` `Service.HasPermissionForConstraintsAPI` should reject empty `constraintSets`, and `internal/api/handler.go` `Handler.requirePermissionConstraints` should deny on nil session, missing auth service, or auth lookup/check errors before allowing access.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1210

Timestamp: 2026-06-26T15:49:21.321Z
Learning: In this repository's Go purchase authorization flow, `internal/api/handler_purchases.go` `purchaseConstraintSets` is expected to encode one constraint set per recommendation with single-value `Providers`, `Services`, `Regions`, and always-populated `AccountIDs`; when a recommendation has no cloud account ID, it should use the `unattributedAccountConstraint` sentinel so account matching does not fail open on an empty request list.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1210

Timestamp: 2026-06-26T15:49:21.321Z
Learning: In this repository's Go purchase authorization flow, `internal/api/handler_purchases.go` `purchaseConstraintSets` should use the batch total upfront cost as `MaxPurchaseAmount` on every per-recommendation constraint set, so users cannot evade a per-permission spend cap by splitting a purchase across multiple recommendations in one request.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1210

Timestamp: 2026-06-26T15:49:21.321Z
Learning: Security follow-up issue `#1300` tracks that per-permission constraints still need enforcement on sibling money-spending verbs in `internal/api/handler_purchases.go`: `authorizeSessionExecuteDirect` for `execute-any`/`execute-own`, `authorizeSessionApprove` for `approve-any`/`approve-own`, and `runPlannedPurchase` for persisted planned purchases. Future reviews should verify these paths build purchase constraint sets and call `requirePermissionConstraints` for the specific authorized verb.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.