[cristim]

Summary

An API key created without explicit scopes silently inherited the owning
user's full permission set at request time. Leaving scope blank was the
most permissive option in the UI, and a long-lived unscoped key became
admin-capable the moment its owner joined the Administrators group.

Root cause

computeEffectivePermissionsFromAuthCtx (internal/auth/service_apikeys.go)
returned authCtx.Permissions (the owner's full set) when the key carried
zero permissions, instead of treating "no scopes granted" as "no permissions
granted". CreateAPIKey also happily accepted a zero-length Permissions
slice, so the trap was reachable from key creation, not just from legacy
data.

Fix

Defense in depth, per the issue's proposed fix:

1. computeEffectivePermissionsFromAuthCtx now returns []Permission{} for
   a zero-permission key. An unscoped key still authenticates its owner but
   authorizes nothing (fail closed). Both call sites
   (HasAPIKeyPermissionAPI, HasAPIKeyPermissionForConstraintsAPI) share
   this helper, so both are fixed by the one change.
2. CreateAPIKey rejects len(permissions) == 0 with a new
   ErrEmptyAPIKeyPermissions sentinel, mapped to HTTP 400 in the
   createAPIKey handler. Scoping is now a deliberate choice at creation
   time, not the default.
3. APIKeyInfo.Permissions in the create/list API responses is no longer
   omitempty, so a legacy zero-scope key (minted before this fix) is
   visible as "permissions":[] in listings instead of looking identical to
   a fully-scoped key -- closing the audit-gap point the issue raised.
4. openapi.yaml's CreateAPIKeyRequest now documents permissions as
   required with minItems: 1.

loadActiveUserForAPIKey was extracted out of CreateAPIKey to keep its
cyclomatic complexity under the project's gocyclo gate (it was already at
the limit of 10; the new check pushed it to 11; the extraction brings it to
8).

Regression test

internal/server/apikey_unscoped_permission_test.go drives the real stack
end to end: a real auth.Service over a mock store, behind a real
api.Handler, through HandleRequest, with an admin-owned API key that
carries zero explicit permissions hitting GET /api/api-keys.

• Pre-fix: verified locally by temporarily reverting
  internal/auth/service_apikeys.go to its pre-fix content and re-running
  this test. It fails: the unscoped key inherits the admin owner's {admin, *} wildcard, requirePermission passes, and the request proceeds into
  listAPIKeys, panicking on an unmocked downstream store call -- proof the
  request was never denied.
• Post-fix: the same test passes, asserting 403.

Unit-level regression coverage was also added/updated in
internal/auth/service_apikeys_test.go:

• TestService_ComputeEffectivePermissions/admin with unscoped key returns no permissions (fail closed) (previously asserted the opposite).
• TestService_HasAPIKeyPermissionAPI/regression #61: unscoped key is denied (fail closed), not inherited from the owner (previously asserted
  the opposite).
• TestService_CreateAPIKey/fail when permissions is empty and /fail when permissions is nil (new, per the issue's acceptance criteria).

Fixtures that relied on the old inheritance behavior as an implementation
detail (not the thing under test) were updated to scope the key explicitly:
internal/server/apikey_usage_booking_test.go,
internal/server/adapter_test.go,
internal/auth/service_apikeys_api_test.go.

Verification

• go build ./... -- pass
• go vet ./... -- pass
• go test -race -short ./... -- 6135 passed, 0 failed, in 33 packages
  (includes the full suite, not just the touched packages)
• go mod tidy -diff -- empty
• gofmt -l on all touched files -- empty
• gocyclo -over 10 -ignore "_test\.go" . -- empty (repo-wide, matches the
  CI gate exactly)
• golangci-lint (exact CI-pinned v2.10.1, ~/go/bin/golangci-lint):
  could not run locally -- it panics with file requires newer Go version go1.27 (application built with go1.26) while type-checking this module.
  Confirmed this is a pre-existing local toolchain/version mismatch, not
  caused by this change: reproduced the identical panic on an unmodified
  checkout of origin/main before making any edits. Relying on CI's
  golangci-lint-action-managed environment (which pins its own Go
  toolchain) for lint verification.
• testcontainers-based DB integration tests were not run locally (Docker is
  unavailable in this environment); no DB-touching code changed in this PR,
  so CI's container suite should be unaffected either way.

Notes

• This PR closes the exact bug described as property (1) of the composed
  finding tracked in a sibling issue (sec(auth): unscoped API key is a permanent MFA-bypassing clone of its owner that survives password reset #102); that issue's fix builds on top
  of this one rather than duplicating it.

Update: frontend fix-first review round

Independent review of this PR found that the backend fix was correct but
incomplete: nothing in the dashboard could mint an API key anymore once
CreateAPIKey started rejecting an empty permissions array.
frontend/src/apikeys.ts always called createApiKey(name, undefined, expiresAt), and the create-key modal (frontend/src/index.html) had no
way to select permissions -- so every dashboard-created key would now 400.
Two changes address this:

1. Permission picker (major). showCreateKeyModal now fetches the
   caller's own effective permissions from GET /api/auth/me/permissions
   on every open and renders one checkbox per entry the owner actually
   holds -- never a wider catalog, so a key can never be scoped beyond
   what its creator holds. The Create button stays disabled until at
   least one permission is checked; handleCreateApiKey re-validates a
   non-empty selection before calling the API (belt-and-suspenders
   alongside the disabled button) and sends it as permissions in the
   request body. Every interpolated action/resource value is escaped
   before the innerHTML write.
2. Legacy-key normalization (minor). A key minted before CreateAPIKey
   started rejecting empty permissions can still have a NULL
   permissions column, which the JSONB scan surfaces as a nil slice.
   Since APIKeyInfo.Permissions is no longer omitempty (this PR's
   original fix), a raw nil would serialize as "permissions":null
   instead of "permissions":[]. normalizeAPIKeyPermissions in
   internal/auth/service_apikeys_api.go closes that gap in both the
   create and list API mappings.

New/updated tests: frontend/src/__tests__/apikeys.test.ts covers
fetching and rendering the picker, labeling the admin wildcard entry,
escaping a hostile action/resource value (asserts no <img> element is
created and the checkbox's dataset round-trips the exact hostile
string), the empty-permissions and fetch-failure states, the Create
button's enable/disable wiring, and handleCreateApiKey sending a
non-empty permissions array (plus rejecting submission when nothing is
checked, and sending every checked entry when more than one is selected).
internal/auth/service_apikeys_api_test.go adds a regression asserting a
legacy nil-permissions key normalizes to an empty, non-nil slice through
ListUserAPIKeysAPI.

Verification for this round: go build ./... / go vet ./... clean
(GOTOOLCHAIN=go1.26.6); go test -race -short ./internal/auth/...
passes; gocyclo -over 10 -ignore "_test\.go" . empty; go mod tidy -diff empty; npx tsc --noEmit clean; npm test -- 91 suites, 2974
passed, 1 pre-existing skip; npm run lint -- 0 errors (only
pre-existing warnings, none in touched files); npm run build succeeds.

Closes #61

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 13 billable files and costs up to $3.25.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 33 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 56 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: c6d677aa-c7a2-4ca5-814e-4c613792dd7a

📥 Commits

Reviewing files that changed from the base of the PR and between fb83c51 and f87a069.

📒 Files selected for processing (13)

• frontend/src/__tests__/apikeys.test.ts
• frontend/src/apikeys.ts
• frontend/src/index.html
• internal/api/handler_apikeys.go
• internal/api/openapi.yaml
• internal/auth/errors.go
• internal/auth/service_apikeys.go
• internal/auth/service_apikeys_api.go
• internal/auth/service_apikeys_api_test.go
• internal/auth/service_apikeys_test.go
• internal/server/adapter_test.go
• internal/server/apikey_unscoped_permission_test.go
• internal/server/apikey_usage_booking_test.go

---

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

[cristim]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

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]

Independent adversarial review, 2 passes, final head f87a069: MERGE. Pass 1 approved the unscoped-API-key fix at 8070813. Pass 2 (Opus 5.5, delta after the rebase past #392): #392's guards are intact; the password re-verify still runs first (401) before name, empty-permissions (400), and expiresAt required/past/cap. No main test was dropped; the only changed tests are the intended #61 fail-closed semantics and a required permission fixture on three expiresAt tests. Local on PR+main: build, vet, go test -race -short auth/api/server 3505 pass, golangci-lint v2.10.1 clean, jest apikeys 73/73, eslint+tsc ok; CI 27/27. Optional nit: pin that a wrong password plus empty permissions gives 401.