[IAmJSD]

Summary

Follow-up to #133 / #983. Owner decision on #133: the API rejects an explicit max_amount: 0 with a 400, and an absent cap still means "no cap". #983 did this for group writes. API-key creation over HTTP still minted the key with no cap, which fails open on a money path.

Root cause

POST /api/api-keys decoded the body into api.CreateAPIKeyRequest/api.Permission. There, max_amount is a plain float64 with omitempty. auth.CreateAPIKeyAPI(req any) then re-encoded that struct through JSON into auth.APICreateAPIKeyRequest (the #1440 workaround noted in #950).

The omitempty re-encode dropped the explicit 0. As a result:

• APIPermissionConstraint.UnmarshalJSON never saw max_amount, so maxAmountSet stayed false;
• validateExplicitZeroCap in apiPermissionsToPermissions never fired;
• the key was stored with MaxPurchaseAmount: 0, which every reader treats as unlimited.

Fix

• createAPIKey decodes straight into auth.APICreateAPIKeyRequest, the same way the group handlers decode into auth.APICreateGroupRequest. The explicit-zero signal now survives to the shared check.
• CreateAPIKeyAPI takes auth.APICreateAPIKeyRequest instead of any. This applies on AuthServiceInterface, auth.Service and the server adapter. The JSON re-encode is removed, so no caller type can bring the erasure back. The compiler now enforces what the re-encode used to work around.
• The unused api.CreateAPIKeyRequest type is removed.
• openapi.yaml (PermissionConstraint.max_amount) now documents the 400 on API-key creation. The CHANGELOG has a new entry, and the "not covered yet (fix(api): API key create erases an explicit max_amount of 0 before validation #980)" wording is gone from the fix(frontend): a max_amount spend cap of 0 is silently erased when a group is edited #133 entry and from the validateExplicitZeroCap comment.

Behavior:

Request max_amount	Result
0, 0.0, -0, 0e5	400 ... max_amount must be greater than 0; omit it for no limit; no key stored
absent, null	key created, no cap (unchanged)
5000	key created with a cap of 5000 (unchanged)
negative / NaN	400, as before (#940/#957)

There is no DB, migration or concurrency change. This is a pure request-decode and type change, so no race test applies.

Closes #980

Verification (local, macOS; unit and mock evidence, no real cloud or DB)

New internal/api/apikey_explicit_zero_cap_test.go drives the real Handler.HandleRequest into a real auth.Service backed by auth.MockStore. It reuses the #951 round-trip harness, so it tests the production handler-to-service conversion.

• TestHandleRequest_CreateAPIKey_RejectsExplicitZeroMaxAmount: 0, 0.0, -0 and 0e5 must return 400 with the message, and no key may be stored.
• TestHandleRequest_CreateAPIKey_AcceptsAbsentNullAndPositiveMaxAmount: absent and null are stored with a cap of 0 (no cap), and 5000 is stored as 5000. Regions are kept in every case.

Fail-before / pass-after:

• On unfixed main (2d822f3), with only the new test file added: all 4 rejection subtests FAIL with expected: 400, actual: 200, and the key is stored with constraints: {"regions":["us-east-1"]} and no cap.
• With the fix: all pass.
• Mutation, caught: I put the erasing decode back in the handler (decode into a local struct with api.Permission, then JSON re-encode into the auth type). All 4 rejection subtests fail again.

Test updates:

• TestService_CreateAPIKeyAPI_CrossPackageType (#1440) is removed. It asserted that the re-encode accepts foreign struct types, which is the behavior removed here. The typed signature now makes the handler-to-service handoff a compile-time guarantee.
• TestAuthServiceAdapter_CreateAPIKeyAPI_KeepsPermissionConstraints keeps its assertions and now uses the auth wire types.
• The mock matcher in TestHandler_createAPIKey_PasswordDecodedBeforeForwarding takes the typed request.

Commands, all passing (tests 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 ./internal/auth/ ./internal/api/ ./internal/server/ ./internal/mocks/   # one package at a time
go test -short -race -count=1 ./internal/auth/ ./internal/api/ ./internal/server/          # one package at a time
golangci-lint run --allow-parallel-runners ./internal/api/... ./internal/auth/... ./internal/server/...   # v2.10.1: 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>                                 # silent
bash scripts/gosec-hook.sh <changed non-test files>                                           # clean
git secrets --register-aws + --scan <changed files>   # in a fresh git init repo, isolated HOME: clean
bash scripts/test-git-secrets-allowlist.sh            # 42 PASS, 0 FAIL
npx markdownlint-cli@0.47.0 CHANGELOG.md              # clean

Coverage gaps: nothing runs against a deployed stack or real Postgres. The path changed here is request decoding and validation before any store write, and the store is mocked at that boundary.

Out of scope

• Other any-typed *API methods (CreateGroupAPI, UpdateGroupAPI, CreateUserAPI, ...) use a type assertion, not a re-encode. They are left as they are.
• The frontend half of fix(frontend): a max_amount spend cap of 0 is silently erased when a group is edited #133 (UI refusing 0) is not touched here.

Maintainer: please mirror the issue's triage labels on this PR: triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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

[cristim]

VERDICT: REQUEST_CHANGES 102d3a2

Reviewed: git diff origin/main...102d3a24 (1 commit, 16 files, +104/-121), base 2d822f3, current origin/main ca1e8a8. git merge-tree is textually clean (tree 253beb5f), but the merged tree does not compile.

Findings

• blocking internal/server/apikey_admin_constraints_test.go:43 (on origin/main, added by dec9ad7 / fix(auth): enforce constraints on an admin-scoped permission on every check #1096 after this PR's base) - the test calls adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{... Permissions: []api.Permission{...}, Constraints: &api.PermissionConstraint{...}}). This PR deletes api.CreateAPIKeyRequest (internal/api/types_apikeys.go) and changes the adapter signature to auth.APICreateAPIKeyRequest (internal/server/app.go:1265). On the merged tree go vet ./... and go test ./internal/server/ fail with undefined: api.CreateAPIKeyRequest, so main would go red on merge. Fix: rebase onto origin/main and port that test to auth.APICreateAPIKeyRequest / []auth.APIPermission / &auth.APIPermissionConstraint{Accounts: ..., MaxAmount: 500}. Since that test feeds a Go literal (not decoded JSON), maxAmountSet stays false and the 500 cap is kept, so the port should be mechanical.
• nit (process) - the single commit is 225 changed lines, within the 400-line cap.

Correctness of the fix itself (checked, no findings)

• Decode: createAPIKey (internal/api/handler_apikeys.go:62) now unmarshals into auth.APICreateAPIKeyRequest. APIPermissionConstraint.UnmarshalJSON (internal/auth/service_api.go:69-81) sets maxAmountSet only for a present, non-null max_amount. 0 / 0.0 / -0 / 0e5 give 400. Absent, null, or "constraints": null (UnmarshalJSON never runs) give no cap. 5000 is kept. A quoted "0" is a 400 decode error, as it was before.
• Password: same field and tag on the auth type, and the base64 decode still runs before forwarding.
• Readers: matchPurchaseAmountConstraint, invalidPurchaseAmountCap and the grant ceiling (internal/auth/purchase_amount_cap.go) read the stored Permission.MaxPurchaseAmount and are unchanged. Stored rows never pass through APIPermissionConstraint.UnmarshalJSON, so existing keys with 0 stored keep meaning no cap. The list/permissions responses omit 0 (omitempty), so a UI round-trip cannot produce an explicit 0. frontend/src/apikeys.ts sends no constraints.
• The group path shares apiPermissionsToPermissions, so the two cannot drift.

Local tests (merged tree 253beb5f, macOS)

• go build ./...: ok.
• go test ./internal/auth/ ./internal/api/ ./internal/mocks/: ok.
• go vet ./... and go test ./internal/server/: FAIL (build), see the blocking finding.

Attacked

• Rebase check with merge-tree, then a compile of the merged tree (this found the blocker).
• JSON edge cases: 0 vs absent vs null vs -0 vs constraints: null, quoted numbers, case-insensitive keys.
• Every reader of the cap, and the stored-zero compatibility.
• Mutation probes on the merged tree, each reverted:
  • P1: re-add a JSON re-encode of createReq in the handler. The 4 RejectsExplicitZeroMaxAmount subtests fail (killed).
  • P2: drop the validateExplicitZeroCap call. The API-key and group explicit-zero tests fail (killed).
  • P3: set maxAmountSet = true unconditionally. The AcceptsAbsentNull absent/null subtests fail (killed).

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES 102d3a2

The fix is correct on its own branch. Once merged with current main, the test build breaks, so it needs a rebase plus a 3-line test fix.

Findings:

• [blocking] internal/server/apikey_admin_constraints_test.go:43-49 - This test is on origin/main b6a0649, added by dec9ad7 (fix(auth): enforce constraints on an admin-scoped permission on every check #1096) after this PR's base.
  • It calls adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{... []api.Permission{... &api.PermissionConstraint{...}}}).
  • This PR deletes api.CreateAPIKeyRequest and makes the parameter auth.APICreateAPIKeyRequest.
  • git merge-tree --write-tree origin/main <head> merges cleanly, but the merged tree does not compile: go vet/go test ./internal/server/ gives undefined: api.CreateAPIKeyRequest.
  • So internal/server reports [build failed], and CI on the merge result goes red.
  • Fix: rebase, and switch that call to auth.APICreateAPIKeyRequest / auth.APIPermission / auth.APIPermissionConstraint. This is the same edit the PR already made in adapter_test.go:453.
• No other findings.

Root cause: the claim checks out.

• On base, api.PermissionConstraint.MaxAmount (internal/api/types.go:396) is float64,omitempty.
• auth.Service.CreateAPIKeyAPI re-encoded the request through JSON, which dropped an explicit 0 before APIPermissionConstraint.UnmarshalJSON (internal/auth/service_api.go:69-82) could set maxAmountSet.
• Head decodes straight into auth.APICreateAPIKeyRequest (internal/api/handler_apikeys.go:62), so validateExplicitZeroCap runs through apiPermissionsToPermissions (service_api.go:324).
• The JSON tags of the old and new request types match field for field, so no other wire behaviour changes.
• The fix is complete. POST /api/api-keys is the only API-key write route (router.go:253), and there is no update-key route.

Attacked:

• Blast radius: every CreateAPIKeyAPI implementer and caller, in head and in the merged tree, is retyped, except the main-only test above.
• The removed type: nothing else references api.CreateAPIKeyRequest in head. auth.CreateAPIKeyRequest (auth/types.go:335) is a separate type and is untouched.
• Edge cases:
  • -0 is rejected.
  • null or absent is accepted.
  • A negative value is still rejected.
  • Unknown fields are lenient in both versions.
• Mutation probes on a scratch copy, each restored from a backup and confirmed identical with cmp. All 4 were caught:
  • P1, the handler's JSON re-encode put back: RejectsExplicitZero FAILS.
  • P2, validateExplicitZeroCap returns nil: the new API-key test and TestGroupWritesHTTP_ExplicitZeroSpendCap FAIL.
  • P3, createReq.Password = password dropped: 3 create tests FAIL.
  • P4, constraints nil-ed after conversion: ResponseUsesDocumentedConstraintShape, RejectsNegativeMaxAmount and AcceptsAbsentNull... FAIL.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK.
  • go vet, plain and with -tags integration, on auth, api, server and mocks: OK.
  • gofmt -l on the 14 changed files: clean.
  • go test -count=1 on auth, api, server and mocks: pass.
  • -race -run 'CreateAPIKey|ExplicitZero|APIKey' on api, auth and server: pass.
• Fail-before: with the new test file on base, all 4 rejection subtests (0, 0.0, -0, 0e5) FAIL with "expected 400, actual 200". The accept subtests pass on base, which matches the PR's claim.
• Merged tree: go build ./... OK, and auth, api and mocks pass. internal/server fails to build (the finding above).
• No migration, DB, frontend or terraform changes.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

@IAmJSD could you rebase this onto current main and retype one test? After the merge, internal/server no longer builds. internal/server/apikey_admin_constraints_test.go (added by #1096, after this branch's base) still builds an api.CreateAPIKeyRequest with []api.Permission / &api.PermissionConstraint{MaxAmount: 500}, and this PR deletes those types. It needs a mechanical port to auth.APICreateAPIKeyRequest / auth.APIPermission; a Go literal leaves maxAmountSet false, so the 500 cap is unaffected. Both reviews found the fix itself correct. Since the explicit-zero cap issue (#980) is live on main, we'd like this in soon: if the branch hasn't moved in about 12 hours, we'll open a PR that cherry-picks your commit with you as author and adds the test retype as a separate commit.

[IAmJSD]

Thanks to both reviewers. Addressed on head 57c5e78f83858653e729460c40f10b17097bd2a2.

[blocking] internal/server/apikey_admin_constraints_test.go uses the removed api.CreateAPIKeyRequest (both reviews)

• I merged origin/main (c8bdd46) into the branch as b52fbd30 instead of rebasing. The merge had no textual conflicts.
• 57c5e78f ports the fix(auth): enforce constraints on an admin-scoped permission on every check #1096 test: internal/server/apikey_admin_constraints_test.go:42-48 now uses auth.APICreateAPIKeyRequest / []auth.APIPermission / &auth.APIPermissionConstraint{Accounts: ..., MaxAmount: 500}, and the unused internal/api import is gone. The literal leaves maxAmountSet false, so the 500 cap is kept. All three subtests pass, including "amount over key cap refused".
• On the merged tree, nothing else references api.CreateAPIKeyRequest. CreateAPIKeyRequest in openapi.yaml is the schema name, not the Go type.

nit (process): commit size. No action needed. The new commit changes 7 lines.

Verification (merged tree, macOS, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, no cloud or DB):

• go build ./...: ok
• go vet ./... and go vet -tags integration on api, auth, server and mocks: ok
• go test -count=1, one package at a time, on ./internal/server/, ./internal/api/, ./internal/auth/ and ./internal/mocks/: all ok
• go test -short -race -run 'CreateAPIKey|ExplicitZero|APIKey|ConstrainedAdmin' ./internal/server/: ok
• golangci-lint v2.10.1 --allow-parallel-runners on api, auth and server (also with --build-tags integration on server): 0 issues
• gocyclo -over 10 on the changed non-test files: silent
• git-secrets (--register-aws, fresh repo, isolated HOME, repo .gitallowed) on the changed files: clean

The PR body is unchanged; no review asked for a body edit.

[IAmJSD]

Follow-up: the branch was green on its own, but go vet ./... failed once it was merged with current main. Fixed on head e425b1011dd445f02889483ec81b70139c2bd314.

What broke

• internal/api/handler_coverage_test.go:400:49: undefined: CreateAPIKeyRequest.
• main did not rename anything. test(api): assert responses and consumed expectations in TestRouter_Handlers_Coverage #1089 (5c2dcbd) landed after this branch's last sync and added a createAPIKeyHandler subtest to TestRouter_Handlers_Coverage. That subtest builds api.CreateAPIKeyRequest, which this PR removes. Like the fix(auth): enforce constraints on an admin-scoped permission on every check #1096 test in the last round, the textual merge is clean but the result does not compile.

Fix

• 92d9bbe6 merges origin/main (a14dc8f) into the branch. There were no textual conflicts.
• e425b101 changes internal/api/handler_coverage_test.go:400 to expect auth.APICreateAPIKeyRequest{Name: "My Key", Password: "pass123"}. The request body, the decoded-password check and the response assertion are unchanged. The strict mock still fails the subtest if the handler forwards anything else.
• I re-checked the fix on main's current request path:
  • createAPIKey still decodes straight into auth.APICreateAPIKeyRequest (internal/api/handler_apikeys.go:62).
  • Service.CreateAPIKeyAPI still runs apiPermissionsToPermissions → validateExplicitZeroCap (internal/auth/service_apikeys_api.go:200, internal/auth/service_api.go:324).
  • The RejectsExplicitZeroMaxAmount and AcceptsAbsentNullAndPositiveMaxAmount tests pass on the merged tree.

Interaction with IAmJSD:fix/issue-169 (not opened yet; it types the user and group AuthServiceInterface methods as auth.API*)

• It does not touch CreateAPIKeyAPI, and it is based on the same main (a14dc8f).
• git merge-tree --write-tree e425b101 fork/fix/issue-169 is clean (tree 264a32bd). On that combined tree, go build ./..., go vet ./..., go vet -tags integration (api, auth, server, mocks) and go test -count=1 on api, auth, server and mocks all pass.
• Whichever of the two merges second needs no code change. It only needs a re-check against main at that time, since a test added to main in between could again use a removed or retyped symbol.

Verification (merged tree, Linux, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, no cloud or DB)

• go build ./..., go vet ./... and go vet -tags integration ./...: ok
• go test -count=1, one package at a time, on ./internal/api/, ./internal/auth/, ./internal/server/ and ./internal/mocks/: all ok
• golangci-lint v2.10.1 --allow-parallel-runners on api, auth and server, plus --build-tags integration on api: 0 issues
• gocyclo -over 10 on the PR's changed non-test files: silent. gofmt -l: clean
• git-secrets on the PR's 18 changed files plus .gitallowed (fresh repo, isolated HOME, --register-aws): clean. A planted fake key in the same repo was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL

The PR body is unchanged; it is still accurate. The PR stays a draft.

🤖 Generated with Claude Code