[IAmJSD]

Root cause

matchPurchaseAmountConstraint only applied the cap when permMax > 0. A permission whose stored MaxPurchaseAmount was negative fell through to "no constraint" and allowed any amount. NaN did the same, because it compares false against everything. The grant ceiling's amountCovers had the same flaw (heldMax <= 0 returned "covers"). Nothing on the group or API-key write paths rejected a negative cap.

Fix

The issue asks for this to fail closed. One predicate, invalidPurchaseAmountCap(v) = v < 0 || NaN, is now used everywhere the cap is read or written:

• Enforcement (matchPurchaseAmountConstraint): a malformed cap denies every request. This covers rows that are already stored negative. Every money path goes through this matcher: web execute, retry, execute-any/own, approve-any/own, runPlannedPurchase (all via enforcePurchaseConstraints), the Azure RI-exchange execute path, and API-key sessions (HasAPIKeyPermissionForConstraintsAPI).
• Grant ceiling (amountCovers): a malformed held cap covers nothing. Without this, an edit could carry a stored negative cap on a carved-out money verb (for example execute:purchases) through as an uncapped permission, because permissionCoveredBy read the negative cap as "no cap". A ceiling must never be looser than enforcement.
• Write time: group create and update (validateConstraintEntries) and API-key create (validateCreateAPIKeyRequest) refuse a negative or NaN cap with ErrInvalidPermission. The group handler already maps that error to 400, and mapCreateAPIKeyError now maps it to 400 too.
• Requested amount (matchPurchaseAmountConstraint, follow-up to review): when a cap applies, a negative, NaN or infinite requested amount is denied too (a NaN request used to compare false against the cap and pass). With no cap the amount dimension stays unrestricted, matching a permission with no constraints at all, which never reaches the matcher. The requested amount is computed server-side, so this is defense in depth.
• Zero is unchanged. It is still the unset zero-value meaning "no cap", as the issue asks.

The cap helpers now live in a new internal/auth/purchase_amount_cap.go. Moving them keeps service_group.go under the 500-line guideline (466 lines). The grant-ceiling coverage helpers (constraintsCover, listCovers, amountCovers) moved unchanged into a new internal/auth/grant_ceiling_cover.go, which brings group_ceiling.go from 519 to 448 lines.

Closes #83

Verification

All evidence comes from unit and mock tests. No real cloud calls, purchases or deploys. The api-package tests use MockAuthService, which answers constraint checks with the real auth.PermissionsAllowForConstraintSets, so they exercise the production matcher. They call enforcePurchaseConstraints and checkAzureExecuteConstraints directly, with no purchase manager wired.

Commands (run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid):

go build ./... && go vet ./...
go test ./... -count=1                                  # all ok
golangci-lint run --allow-parallel-runners ./...        # v2.10.1: 0 issues
gocyclo -over 10 <changed non-test .go files>           # silent

Fail before / pass after. I stashed the non-test changes and ran the new tests against unchanged main:

--- FAIL: TestMatchPurchaseAmountConstraint_NegativeCapDeniesAll
--- FAIL: TestService_HasPermissionForConstraintsAPI_NegativeStoredCap
--- FAIL: TestAmountCovers_NegativeHeldCapCoversNothing
--- FAIL: TestGrantCeiling_StoredNegativeCapCannotBeWidened
--- FAIL: TestGroupWrite_RejectsNegativeMaxAmount
--- FAIL: TestCreateAPIKey_NegativeMaxPurchaseAmount
--- FAIL: TestEnforcePurchaseConstraints_NegativeCapRefuses/{execute,execute-any,approve-any}/negative_refused
    --- PASS: .../positive_cap_enforced_as_before, .../zero_is_no_cap   (unchanged behavior)
--- FAIL: TestCheckAzureExecuteConstraints_NegativeCapRefuses/{USD,EUR}/negative_refused
    --- PASS: .../USD/positive_cap_enforced_as_before
--- FAIL: TestMapCreateAPIKeyError_InvalidPermissionIs400

With the fix, all of these pass. The valid-cap controls (a positive cap enforced at and over the limit, and zero meaning no cap) pass both before and after, which shows the fix changes only the malformed case.

Coverage gap: I did not exercise this against a real Postgres-stored group row or a real cloud account. Malformed stored rows are covered by mock-store fixtures.

Behavior notes and out of scope

• A group that already holds a stored negative cap on a carved-out money verb can no longer have that cap changed through the API, because the existing negative cap covers no replacement. The permission can still be dropped. Repairing it needs a DB or migration edit. This is deliberate fail-closed behavior.
  • Operator note: a negative or NaN MaxPurchaseAmount can only exist from a manual DB edit, because the API now refuses to write one. On an ordinary permission, resubmit it with a zero (no cap) or positive cap. On a carved-out money verb (execute:purchases, approve-any:purchases, retry-any:purchases, execute:ri-exchange, or a * resource on those actions), the edit must drop the entry; restore it with a valid cap through the same SQL path that seeded the group. This note lives here rather than in a doc because the only fitting runbook, the Troubleshooting section of docs/DEPLOYMENT.md, is already 768 lines, over the 500-line guideline.
• For a non-USD Azure RI exchange under a negative cap, the second "amount neutralized" check also fails. The caller gets the generic constraint-denied 403 rather than the currency-specific one. It still fails closed. I updated the doc comment to match.
• Separate finding, not fixed here: api.CreateAPIKeyRequest.Permissions uses api.PermissionConstraint, whose JSON tags are accounts and max_amount. CreateAPIKeyAPI re-encodes the request into auth.Permission, whose tags are account_ids and max_purchase_amount. By reading the code, it looks like API-key constraints sent over HTTP are silently dropped during that re-encode. The owner's group constraints are still enforced separately. This is now tracked as API-key create drops accounts/max_amount constraints (JSON tag mismatch between api and auth types) #951, with its own PR.
• Frontend: groupModals.ts silently skips a negative max_amount in collectPermissions. The input's min="0" and native form validation (no novalidate) already block it in a real browser, and the backend now refuses it anyway, so I left the frontend alone.
• I deliberately left the currency-compare asymmetry noted in the issue alone, as the issue asks.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Invalid negative or NaN purchase limits are now rejected when creating or updating groups and when creating API keys.
  • Requests with malformed purchase limits are denied, and attempts to widen permissions using an invalid stored limit are blocked.
  • Zero continues to mean no purchase limit; positive limits continue to reject purchases above the specified amount.
  • Invalid permissions in API key creation requests now return HTTP 400.

[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: 0df8751c-6a19-4010-af41-0d0f8978dab3

📥 Commits

Reviewing files that changed from the base of the PR and between 2f41346 and 7dc1891.

📒 Files selected for processing: 9

• internal/api/handler_apikeys.go
• internal/api/handler_ri_exchange.go
• internal/api/negative_purchase_cap_test.go
• internal/auth/errors.go
• internal/auth/group_ceiling.go
• internal/auth/negative_purchase_cap_test.go
• internal/auth/purchase_amount_cap.go
• internal/auth/service_apikeys.go
• internal/auth/service_group.go

💤 Files with no reviewable changes: 1

• internal/auth/service_group.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

Purchase-cap validation now rejects negative and NaN caps on group and API-key writes. Invalid stored caps deny purchase requests and cannot cover widened grants. Zero remains uncapped, and positive caps continue to enforce their limits.

Changes: Purchase cap behavior

Layer / File(s)	Summary
Cap validation and group enforcement internal/auth/purchase_amount_cap.go, internal/auth/group_ceiling.go, internal/auth/service_group.go, internal/auth/negative_purchase_cap_test.go	Shared validation rejects negative and NaN caps. Group constraint checks deny requests for invalid caps, and invalid stored caps do not cover grants. Tests cover matching, service checks, grant ceilings, and group writes.
API-key permission validation internal/auth/errors.go, internal/auth/service_apikeys.go, internal/auth/negative_purchase_cap_test.go	API-key creation validates permission caps before storage. Invalid permission errors include negative and NaN cap cases. Tests cover rejection of invalid caps and acceptance of zero and positive caps.
API enforcement and error mapping internal/api/handler_apikeys.go, internal/api/handler_ri_exchange.go, internal/api/negative_purchase_cap_test.go	Tests cover negative, zero, and positive caps across execute, approve-any, and Azure execute constraints. API-key creation maps invalid permissions to HTTP 400. The Azure constraint comment clarifies how zero and malformed negative caps are handled.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 7dc18

Malformed purchase caps are denied on the inspected purchase path, and no issue requiring a change before merge was identified.

Security architecture risk: 🔵 Low · up to 7dc18

The change tightens purchase authorization without identifying a new privilege path. An existing API-key request-conversion gap still drops submitted purchase caps, so validation is not end-to-end on that route. The owner's permission limits continue to apply.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The affected authority is purchase spending permitted by group or delegated API-key permissions. A malformed permission now fails its amount check, but another independently sufficient permission may still authorize the request. The inspected API-key path remains bounded by the owner's permissions; deployment-wide exposure and the number of affected stored permissions are unknown.

Security Findings and Attack Paths

• inferred — An existing HTTP API-key conversion gap remains: request constraints serialize as max_amount and accounts, but the auth adapter expects max_purchase_amount and account_ids. JSON conversion consequently discards those fields, allowing a submitted negative cap to reach the new validator as zero and a positive cap to lose its intended restriction. The relevant schemas, adapter, and caller are unchanged against the PR base, so this is not an introduced or worsened concern. Effective spending remains subject to the owner's independent constraints.

Trust Boundaries and Controls

• observed — API-key creation requires create:api-keys authorization and password re-verification. Constraint-based API-key execution independently checks both key-effective permissions and owner group permissions. These controls limit the existing conversion gap and remain distinct from the newly added malformed-cap validation.

Resilience and Maintainability Implications

• inferred — Using one malformed-cap predicate across enforcement, delegation, and native write validation reduces disagreement between those security controls. It does not protect fields discarded before validation or establish validation coverage for every lower-level group-write caller.

Hardening Proposals

• proposed — Replace the API-key adapter's implicit nested JSON compatibility with explicit constraint mapping. Verify through the HTTP boundary that negative caps are rejected and positive caps and account restrictions survive creation and constrain execution. This would address the pre-existing conversion gap rather than an authority expansion introduced here.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: invalid negative MaxPurchaseAmount values now fail closed during authorization.
Linked Issues check		Issue #83 requires negative stored caps to deny requests, while zero remains no cap. The new matcher returns false for negative and NaN caps before the positive-cap check, and returns true for zero wh…
Out of Scope Changes check		The changed files support issue #83. The helper extraction, grant-ceiling protection, write validation, error mapping, comments, and tests implement or verify fail-closed purchase-cap behavior. The PR…

Full details: Linked Issues check

Explanation

Issue #83 requires negative stored caps to deny requests, while zero remains no cap. The new matcher returns false for negative and NaN caps before the positive-cap check, and returns true for zero when otherwise allowed. The same invalid-cap rule applies to grant ceilings. Group and API-key writes reject malformed caps with ErrInvalidPermission, with API-key errors mapped to HTTP 400. The added tests cover stored-cap enforcement, grant ceilings, write validation, zero, and positive controls across the reported paths.

Full details: Out of Scope Changes check

Explanation

The changed files support issue #83. The helper extraction, grant-ceiling protection, write validation, error mapping, comments, and tests implement or verify fail-closed purchase-cap behavior. The PR does not change the separately identified currency asymmetry, frontend handling, or API-key re-encoding finding.

• Fix all pre-merge checks with AI

✨ 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.}

[IAmJSD]

Pushed 7dc1891 to address the CI failures:

• git-secrets allowlist self-test: internal/auth/negative_purchase_cap_test.go declared an inline password literal, which the git-secrets password detector flagged. The test now reuses the package's existing testAPIKeyPassword constant. No allowlist entry was added. Locally, with awslabs/git-secrets on PATH, bash scripts/test-git-secrets-allowlist.sh reports 42 PASS, 0 FAIL. A git secrets --scan of every file this PR changes, using the repo's patterns and .gitallowed, exits 0.
• Lint Code: this was an infrastructure failure. golangci-lint couldn't download its config JSON schema, so the job needs a maintainer re-run. Locally, golangci-lint v2.10.1 reports 0 issues.

Also re-run locally: go test ./internal/auth/ ./internal/api/ ./internal/mocks/ passes, go vet is clean, and gocyclo -over 10 on the changed files is silent.

[cristim]

Independent review of 7dc1891: draft: blocks ready

No BLOCKING findings.

NON-BLOCKING

• internal/auth/group_ceiling.go is now 519 lines (guideline 500); it was 511 before. Additions are 422 (guideline 400). Both are guidelines only.
• Existing stored negative cap on a carved-out money verb: amountCovers (group_ceiling.go:511) now returns false for it, so that entry cannot be carried through an edit, only dropped. This is the intended fail-closed behavior and is documented in the PR body. The cap cannot reach the DB through the API today (JSON negative is now 400), so such rows only exist from manual DB edits; no migration is needed. Worth one line in the runbook if an operator hits it.
• matchPurchaseAmountConstraint does not reject a negative or NaN reqMax (the requested amount). Out of scope for #83 (reqMax is computed server-side from recommendation totals), noted only.

What I attacked (worktree at the head SHA, go test ./internal/auth/... ./internal/api/ all pass, 4052 tests)

• Every reader of the cap: matchConstraints (service_group.go:408) is the only enforcement call site; web execute, approve, retry, Azure RI-exchange and API-key paths all reach it. No other reader of MaxPurchaseAmount outside auth/api (grep of internal/ and cmd/; no scheduler or SQL reader, no migration seeds a cap).
• Semantics: 0 still means "no cap" (permMax > 0 guard kept); negative and NaN deny; amountCovers held 0 still covers anything, held negative covers nothing. No path turns an unset cap into deny-all.
• Write paths: validateConstraintEntries (group create/update via checkGrantCeiling, service_api.go:318 and :391) and validateCreateAPIKeyRequest (service_apikeys.go:161) reject negative and NaN; mapCreateAPIKeyError maps ErrInvalidPermission to 400.
• Existing negative rows: a non-carved-out permission with a stored negative cap now denies all requests (fail closed, as #83 asks) and can be repaired by resubmitting a zero or positive cap; only the carved-out verbs are drop-only.
• Mutation probe 1: make invalidPurchaseAmountCap ignore negatives. Fails TestMatchPurchaseAmountConstraint_NegativeCapDeniesAll, TestService_HasPermissionForConstraintsAPI_NegativeStoredCap, TestGroupWrite_RejectsNegativeMaxAmount, TestCreateAPIKey_NegativeMaxPurchaseAmount. Reverted.
• Fail-before: relied on the PR's own recorded run plus probe 1 (the same predicate gates all new tests); I did not rebuild the parent.
• The API-key constraint re-encode drop the PR author mentions (api.PermissionConstraint vs auth.Permission tags) is a separate, pre-existing issue and should get its own issue; it is not made worse here.

Gate status (origin/main Review gate)

• Independent review: this comment, against the SHA above.
• CI on the SHA: all checks pass (Unit, Integration, E2E, Lint, pre-commit, Security, gosec, Trivy). CodeRabbit skipped (draft); no review threads exist.
• Labels: match issue #83 (effort/s, impact/internal, priority/p3, severity/low, triaged, type/security, urgency/eventually).
• Local verification: unit and mock tests only, no real Postgres-stored row or cloud account; realistic mock-store fixtures cover the stored-negative case. Reviewer did not exercise a real DB.

Blocks ready

• Fail-closed predicate shared by enforcement, grant ceiling and both write paths.
• Fail-before/pass-after tests present and the probe confirms they bite.
• CI green on this SHA; labels mirrored.

[cristim]

Gate record for 7dc1891 (origin/main CLAUDE.md "Review gate"):

• Independent review of 7dc1891: no blocking findings. A negative MaxPurchaseAmount now fails closed, and 0 still means no cap.
  • Non-blocking: internal/auth/group_ceiling.go is 519 lines (over 500), and the PR is +422.
  • Non-blocking: a stored negative cap on a carved-out money verb can only be dropped, not re-set, which is intended.
• CI on 7dc1891: CI Success and Run pre-commit hooks pass.
• Local verification (macOS):
  • The auth and api tests pass.
  • A mutation probe that stops rejecting negatives fails 4 tests.
  • No real purchase.
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from hardening(auth): negative MaxPurchaseAmount constraint is treated as 'no constraint' instead of deny-all #83.

Merging at 7dc1891 with --match-head-commit.

[IAmJSD]

Follow-up to the review of 7dc1891. New head: 91679e2 (two new commits, no force-push).

1. group_ceiling.go over 500 lines. Fixed in 1d25358, a pure move. constraintsCover, listCovers and amountCovers now live in a new internal/auth/grant_ceiling_cover.go, with their bodies and comments unchanged. 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. matchPurchaseAmountConstraint and a malformed reqMax. Fixed in 91679e2.

• With a cap (permMax > 0): a negative, NaN or ±Inf requested amount is now denied. Before this change, NaN and negative values passed. +Inf was already denied, because it compares greater than any cap.
• With no cap (permMax == 0): the request is still allowed, whatever the amount. I chose this because checkPermissionConstraints never calls the matcher for a permission with nil constraints, and the admin path skips it too. If "no cap" denied a NaN, a permission constrained only by region would refuse a request that a fully unconstrained permission allows. That would be an inconsistent and surprising result. The amount checks on money paths that run before this matcher (for example requireNonZeroCommitment) are unchanged.
• I checked the callers. None of them produces a malformed value today. Non-money checks pass 0, which is still valid. The non-USD Azure path uses math.MaxFloat64, which is finite, as its "unbounded" sentinel, so its behavior is unchanged. No caller was modified.
• Regression test: TestMatchPurchaseAmountConstraint_MalformedRequestedAmount.

3. Operator note for a stored negative cap on a carved-out verb. I looked for a runbook to put it in. The only fitting place is the Troubleshooting section of docs/DEPLOYMENT.md, and that file is already 768 lines, over the 500-line guideline. Rather than grow it, I put the note in the PR body under "Behavior notes". I can move it into DEPLOYMENT.md if you prefer.

API-key constraint drop: this is tracked as #951 and has its own PR. It is not touched here. I updated the PR body to say so, and removed the label request now that the labels are mirrored.

Verification (unit/mock evidence only; no real DB, cloud account, purchase or deploy)

All commands ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

go build ./... && go vet ./internal/auth/... ./internal/api/...   # ok
go test ./internal/auth/... ./internal/api/... -count=1           # ok auth, ok api
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/... ./internal/api/...   # 0 issues
gocyclo -over 10 purchase_amount_cap.go grant_ceiling_cover.go group_ceiling.go             # silent
git secrets --scan (AWS patterns) on changed files                # clean
bash scripts/test-git-secrets-allowlist.sh                        # 42 PASS, 0 FAIL

Fail before / pass after for note 2. The test was added and run before the fix:

--- FAIL: TestMatchPurchaseAmountConstraint_MalformedRequestedAmount
    10 assertion failures: requests of -1, -0.01, -MaxFloat64, -Inf and NaN were allowed under caps of 1000 and MaxFloat64

With the fix, that test passes. All earlier #83 tests (NegativeCapDeniesAll, ValidCapsUnchanged, NegativeStoredCap, RejectsNegativeMaxAmount, NegativeMaxPurchaseAmount) still pass. Line counts of the touched files: group_ceiling.go 448, grant_ceiling_cover.go 77, purchase_amount_cap.go 64, negative_purchase_cap_test.go 263.

🤖 Generated with Claude Code

[IAmJSD]

This merged at 7dc1891 just before the follow-up commits for the non-blocking notes (91679e2) reached this branch. Those commits are therefore not in main. They are now in #954 (tracked by #953), cherry-picked unchanged onto current main, and that PR has the full verification. You can ignore my earlier point-by-point comment on this PR.

🤖 Generated with Claude Code