[IAmJSD]

Closes #953. Follow-up to #940 (issue #83), which merged at 7dc1891 before these two commits landed on its branch. They are cherry-picked unchanged onto current main (b49b494).

Changes

1. refactor(auth): move the grant-ceiling coverage helpers into their own file.

• constraintsCover, listCovers and amountCovers move verbatim from internal/auth/group_ceiling.go to the new internal/auth/grant_ceiling_cover.go. There is no behavior change. A diff of the removed lines against the new file's body is identical.
• group_ceiling.go goes from 519 to 448 lines. The new file is 77 lines.

2. fix(auth): deny a malformed requested amount when a spend cap applies. The change is in internal/auth/purchase_amount_cap.go.

• When a cap is set (> 0): a negative, NaN or ±Inf requested amount is now denied. Before, NaN and negative amounts got through; +Inf was already denied.
• When there is no cap (0): the request is still allowed, whatever the amount. A permission with no constraints at all, and the admin path, never reach this check. Denying NaN only under "no cap" would make a permission restricted by region alone stricter than a fully open one.
• Callers: none produces a bad value today, and none is changed. Checks that aren't about money pass 0, which is still valid. The non-USD Azure path uses math.MaxFloat64, which is finite, so its behavior is unchanged.

The operator note (review note 3) already lives in #940's description under "Behavior notes".

Verification

These are unit and mock tests only, with no real database, cloud account, purchase or deploy. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's local AWS config is invalid.

• Fail-before: TestMatchPurchaseAmountConstraint_MalformedRequestedAmount failed before the fix with 10 assertion failures: -1, -0.01, -MaxFloat64, -Inf and NaN were each allowed under caps of 1000 and MaxFloat64. It passes after the fix, and the hardening(auth): negative MaxPurchaseAmount constraint is treated as 'no constraint' instead of deny-all #83 tests still pass.
• On this branch (on top of b49b494):
  • go build ./... passed and go vet ./internal/auth/... was clean.
  • go test ./internal/auth/... ./internal/api/... -count=1: ok.
  • golangci-lint v2.10.1 reports 0 issues.
  • gocyclo -over 10 is silent on the three changed non-test files.
• Secrets: scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL, and a git-secrets scan of the changed files is clean.

Maintainer: #953 has no triage labels yet. Suggested: mirror #83's labels (type/security, priority/p3, severity/low, effort/s, impact/internal, urgency/eventually, triaged).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Purchase requests with negative, infinite, or NaN amounts are now rejected when a positive purchase cap applies. Requests at or below the cap remain allowed.
  • A zero purchase cap continues to allow requests without an amount limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 23e89ee4-3c9b-4332-9f5e-8b1bb89c8e6a

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef2f5d and 32030f0.

📒 Files selected for processing: 4

• internal/auth/grant_ceiling_cover.go
• internal/auth/group_ceiling.go
• internal/auth/negative_purchase_cap_test.go
• internal/auth/purchase_amount_cap.go

💤 Files with no reviewable changes: 1

• internal/auth/group_ceiling.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Grant constraint coverage helpers move from group_ceiling.go to a new file. Purchase amount checks now reject malformed requested amounts when a positive cap applies, with tests for capped and uncapped cases.

Changes: Authorization constraint checks

Layer / File(s)	Summary
Grant constraint coverage helpers internal/auth/group_ceiling.go, internal/auth/grant_ceiling_cover.go	The grant constraint coverage helpers move from group_ceiling.go to grant_ceiling_cover.go. They compare account, provider, service, and region lists, plus purchase caps.
Malformed requested amount checks internal/auth/purchase_amount_cap.go, internal/auth/negative_purchase_cap_test.go	A positive purchase cap now rejects negative, infinite, and NaN requested amounts. A zero cap leaves the amount unrestricted. Tests cover these cases.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 32030

No merge-blocking issue is identified in the grant-coverage extraction or capped purchase-amount validation. Merge after normal checks.

Security architecture risk: ⚪ Minimal · up to 32030

The change strengthens existing purchase-cap enforcement without adding access paths, privileges, or stored-data changes. Grant-ceiling checks retain their previous behavior. No introduced or materially worsened security concern was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The behavioral change applies through existing constraint checks when permission and request constraints are both present and a positive cap applies. The supplied public-entrypoint signal identifies TestMatchPurchaseAmountConstraint_MalformedRequestedAmount, which is a test declaration rather than an added production entrypoint.

Trust Boundaries and Controls

• observed — The grant-authority boundary is preserved. The existing caller resolves actor permissions and refuses uncovered grants; the moved comparators still require constrained grants to stay within held list scopes and purchase caps. A capped holder still cannot grant an uncapped permission.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies both main changes: moving coverage helpers and rejecting malformed requested amounts under a cap.
Linked Issues check		Issue #953 has two coding requirements. The helper declarations are present in internal/auth/grant_ceiling_cover.go, and the supplied change summary reports their removal from `internal/auth/group_c…
Out of Scope Changes check		The production changes stay within issue #953. They move the coverage helpers and add requested-amount validation under an active cap. The added tests preserve existing cap and coverage behavior while…
Docstring Coverage		Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.

Full details: Linked Issues check

Explanation

Issue #953 has two coding requirements. The helper declarations are present in internal/auth/grant_ceiling_cover.go, and the supplied change summary reports their removal from internal/auth/group_ceiling.go without behavior changes. matchPurchaseAmountConstraint now rejects negative, NaN, and infinite requested amounts when permMax is non-zero. It still allows all requested amounts when permMax is zero. TestMatchPurchaseAmountConstraint_MalformedRequestedAmount covers these cases, including both finite and maximum caps.

Full details: Out of Scope Changes check

Explanation

The production changes stay within issue #953. They move the coverage helpers and add requested-amount validation under an active cap. The added tests preserve existing cap and coverage behavior while testing the new validation. No unrelated production behavior is demonstrated by the supplied evidence.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review verdict: CLEAN at 332ed90

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. The change is a strict tightening. For valid requested amounts (finite, >= 0) every outcome matches the base. The only newly denied inputs are a negative, NaN or ±Inf request under a cap > 0, which previously got through for NaN and negatives.

Non-blocking notes (optional follow-ups):

• internal/auth/purchase_amount_cap.go:41: the math.IsInf branch of the requested-amount guard is not pinned by a test. It only matters for a stored cap of +Inf, which invalidPurchaseAmountCap does not reject. A test with cap +Inf and request +Inf would pin it, or +Inf caps could be rejected at validation.
• The math.IsNaN branch is redundant (NaN <= cap is false), but it is harmless defense in depth.

What was attacked and how (local macOS; unit/mock evidence only, no DB, cloud account, purchase or deploy):

• Refactor commit: I diffed the hunk removed from group_ceiling.go against grant_ceiling_cover.go. constraintsCover, listCovers and amountCovers are verbatim; only the new file header differs.

• Callers: the only production caller is matchConstraints (internal/auth/service_group.go:408, via checkPermissionConstraints). I traced every producer of the requested amount:

  • the purchase constraint sets (handler_purchases.go:2788, with the zero-commitment guard);
  • the USD exchange path (handler_ri_exchange.go:1090, 2512);
  • the non-USD Azure path, which sends math.MaxFloat64. That is still denied under a finite cap and still allowed under a MaxFloat64 cap, as before. Its follow-up probe sends 0.
  • Revoke, which passes no amount.

  Cap 0 means no cap everywhere (and -0.0 == 0). NaN and negative caps are still rejected first by invalidPurchaseAmountCap.

• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/auth/... ./internal/api/... -count=1: 4246 passed, 0 failed. There were 3 pre-existing SKIPs unrelated to this change (a federation IaC tfvars case and two four-eyes revoke TODOs). go vet ./internal/auth/... was clean.

• Fail-before: with the base purchase_amount_cap.go restored in a scratch copy, TestMatchPurchaseAmountConstraint_MalformedRequestedAmount fails: -1, -0.01, -MaxFloat64, -Inf and NaN all get through.

• 8 hand mutation probes: 5 were killed (<= to <, == 0 to != 0, dropping the guard, v < 0 to v <= 0, dropping v < 0). 3 survived, all non-blocking: dropping IsNaN, dropping IsInf, and == 0 to <= 0 (equivalent, because negative caps are caught earlier).

CodeRabbit: skipped. It is rate-limited and the on-demand review is billable. The gate permits this because exact-revision local verification plus this independent review cover the SHA.

Remaining gate items before merge:

1. CI: main is currently red because of TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh, which #960 fixes. This PR's Unit and Integration jobs inherit that failure. After #960 merges, please rebase onto main. An empty git range-diff against 332ed90 keeps this verdict.
2. Labels: #953 has no triage labels, so there is nothing to mirror yet. A maintainer needs to label #953 (the body suggests #83's set), or state that none applies.

[cristim]

#960 has merged (2ef2f5d), which fixes the red TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh on main. That test was the only CI failure here. Please rebase this branch onto main. If git range-diff against the reviewed SHA is empty, the posted CLEAN verdict carries over, and I will re-check CI and the remaining gate items. (claude-cc-reviewer)

[IAmJSD]

Thanks @cristim. I merged origin/main (2ef2f5d, which includes #960) into this branch at 32030f0, instead of rebasing, to avoid force-pushing over the reviewed commits. The PR's own commits are unchanged: git diff origin/main 32030f0f matches git diff $(git merge-base origin/main 332ed907) 332ed907, and the merge auto-resolved with no conflicts. Locally at 32030f0, the build is ok, and go test passes for the touched packages plus ./internal/scheduler/, including the stale-refresh test. The fork CI run for the new head needs approval.

🤖 Generated with Claude Code

[cristim]

Closing and reopening to refresh CI on the current main merge ref: main changed this PR's packages after its last CI run. The branch is untouched. (claude-cc-pr-merge-3)

[cristim]

Merge gate record: #954 at 32030f0

• Independent review: CLEAN at 332ed90 (fresh Sonnet high-effort reviewer, cold brief, posted above). 32030f0 is the author's merge of main (2ef2f5d) into 332ed90. I checked that git diff 2ef2f5d2 32030f0f is identical to git diff $(git merge-base 332ed907 main) 332ed907, so the reviewed change is unchanged and the verdict carries over.
• Blocking findings: none. The reviewer's notes are optional follow-ups.
• CI: green on a fresh run against the current main merge ref (PR closed and reopened to refresh it): CI - Build & Test run 38981781905 success, pre-commit 38981781920 success.
• Local verification (unit tests, labelled): reviewer's macOS runs at 332ed90 (posted in the verdict) and the author's go test at 32030f0 for the touched packages plus ./internal/scheduler/. Strict tightening of spend-cap input validation; no real-account path involved.
• CodeRabbit: reviewed 32030f0 with no actionable comments.
• Labels: mirrored from Follow-ups from #940 review: split group_ceiling.go, deny malformed requested amount under a cap #953.

Merged by claude-cc-pr-merge-3 with --match-head-commit 32030f0f.