[IAmJSD]

Summary

Clears what can be cleared now from the four internal/auth findings in #313 (audit findings A03-022, A03-023, A03-020 and A16-014). Each finding was re-checked against current main (a14dc8f2). The items left out are listed in the table with the reason. This is a partial fix: the User.Salt column drop, the redactEmail consolidation and the MatchesAccount duplicate stay open, so this PR says Refs, not Closes.

Refs #313

Per-finding status

Finding	Item	Status
A03-022	RoleAdmin / RoleUser / RoleReadOnly constants	Fixed. Deleted. The verb comments in types.go that named them now name the seeded groups (Administrators / Standard Users / Read-Only Users).
A03-022	Service.UpdateLastUsed	Fixed. Deleted, with its test. It was the only caller of StoreInterface.UpdateAPIKeyLastUsed, so that method, its Postgres implementation, its three pgxmock tests and its three mocks (auth.MockStore, mocks.MockAuthStore, server health mock) are deleted too. The comment on effectiveLifetimeUsage already warned that any caller of it would break the last_used_at/request_count_total invariant.
A03-022	credentials.ResolveGCPCredentials	Fixed. Deleted, with its three tests. It had no non-test caller, and internal/ can't be imported from outside the module.
A03-022	auth.EmailSenderInterface.SendWelcomeEmail	Fixed. Removed from the interface and from the auth-package mocks (MockEmailSender, recordingMailSink). The concrete email senders still implement it, so they still satisfy the narrower interface.
A03-022	User.Salt field	Deferred. The issue says to delete the field "once the column is dropped", and users.salt still exists. The store INSERTs/UPDATEs/SELECTs it (store_postgres.go, store_postgres_credentials.go). Dropping it takes a migration plus a deploy order that stops writing the column before the drop, so it belongs in its own PR.
A03-023	inline loadUser copies	Fixed. loadUser now wraps store errors as failed to get user: %w. The new loadActiveUser adds the Active check. Callers routed through them: UpdateUserProfile, ChangePassword, CreateAPIKey (its loadActiveUserForAPIKey copy is gone), ListUserAPIKeys, authorizeAPIKeyAccess, the API-key validation path (lookupAPIKeyUser is gone), and also GetUserPermissions and BuildAuthContext, which were byte-identical copies.
A03-023	DeleteUser repeats checkLastAdminConstraint	Fixed. It calls the helper now. The deferred trigger backstop (isLastAdminConstraintViolation) is unchanged.
A03-023	sessionSuffix computed twice	Fixed. credentials.roleSessionName(accountID) is shared by AssumeRole and AssumeRoleWithWebIdentity.
A03-023	redactEmail x3 + redactEmailLocal	Deferred. The suggested home, pkg/logging, is in LeanerCloud/cloud-commitments-go, which go.mod pins, so this needs a release there first. The copies also mask differently: the auth one masks the domain too (us***@ex***.com), the other three don't. Choosing one rule changes what the logs show, which is an owner decision. Also, open PR #1152 (fix/issue-263) edits resetRecentlyIssued, including its redactEmail(email) call line.
A03-023	MatchesAccount duplicates AccountScope.Allows	Deferred to open PR #1116 (fix/issue-1034). It deletes MatchesAccount and CanAccessAccount and rewrites AccountScope.Allows, which are the same lines.
A03-020	four post-fetch subtle.ConstantTimeCompare calls (ValidateSession, ValidateUserAPIKey, ResetTokenStatus, validateResetToken)	Fixed (comments). Compares kept on purpose. See below.
A16-014	permissionCoveredBy return true never reached	Already covered on main. No change. TestGrantCeiling_CarvedOutMayBeKeptNotWidened/kept_unchanged_is_allowed (added in 1f363bc, #1737) is the case the issue asks for: an admin:*-only actor (admin:* does not reach the carved-out money verbs) resubmits a group's existing execute:purchases unchanged and the update must succeed. Mutation probe: changing return true to return false in permissionCoveredBy makes go test ./internal/auth/ fail on exactly that test. The prerequisite fix, cli#1901, merged as 9ec4051 (#2069). I couldn't check the audit commit 3c0f8ac9 because it isn't in this repository's history.

Why the constant-time compares stay (A03-020)

Each compare checks the stored hash against the hash the row was just selected by:

• GetSession: WHERE token = $1 AND expires_at > NOW()
• GetAPIKeyByHash: WHERE key_hash = $1 AND is_active = true
• GetUserByResetToken: WHERE password_reset_token = $1

So with a correct store they always pass. The audit is right that they close no timing oracle: the database's equality has already run by then. That lookup timing is harmless anyway. The value compared is the SHA-256 of a 32-byte crypto/rand token or key (generateToken, CreateAPIKey), and an attacker can neither guess it nor steer it byte by byte.

The compares are still a store-integrity check: they reject a row the store returned for a different credential. The brief for this change was not to remove a compare that sits on a credential path unless it is provably bypassed, and these aren't bypassed, only redundant with a correct store. So the four checks stay and only the false comments are replaced. The tests that cover them were renamed from "constant-time mismatch …" to "stored hash mismatch …" and their comments describe the store-integrity check. No behavior change.

Not done here: the issue also asks to correct #104's "verified sound" section, which lists these compares as a timing defense. That's an issue comment, so it's left to the maintainer.

Behavior changes (all error text, no control flow)

• Store errors from UpdateUser, DeleteUser, ChangePassword, GetUserPermissions, BuildAuthContext and ValidateSession are now wrapped as failed to get user: <err> (with %w, so errors.Is still works). The API-key paths and UpdateUserProfile already did this. For ValidateSession the result reads failed to load session user: failed to get user: <err>.
• A missing API-key owner now reads user not found instead of user account not found, matching every other path. Nothing in the repo matched the old string.

Verification

All unit or mock evidence, on this branch. Every command ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

• go build ./..., go vet ./... and go vet -tags integration ./...: clean.
• go test -count=1 run one package at a time on ./internal/auth/, ./internal/credentials/, ./internal/mocks/, ./internal/server/ and ./internal/api/: all ok.
• golangci-lint run --allow-parallel-runners (v2.10.1) on ./internal/auth/... ./internal/credentials/... ./internal/mocks/... ./internal/server/..., plus --build-tags integration ./internal/auth/...: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: no output.
• git merge-tree of this commit against fix(auth): bind group account scope entries to account IDs #1116, test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152, fork/fix/issue-64, fork/fix/issue-169 and fork/fix/issue-310: all merge cleanly. None of those diffs adds a use of a symbol deleted here. test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152's new inviteMailSink keeps a SendWelcomeEmail method, which is harmless against the narrower interface.

Fail-before / pass-after for the new internal/auth/service_user_lookup_test.go. It pins one lookup contract on all ten routed callers: pgx.ErrNoRows → user not found, a nil user → user not found, a store error → failed to get user: … with errors.Is, and an inactive owner → user account is not active on the two active-only paths. I copied the test file unchanged onto a clean origin/main worktree:

--- FAIL: TestUserLookup_UniformNotFoundAndStoreError
    --- FAIL: .../UpdateUser/store_error
    --- FAIL: .../DeleteUser/store_error
    --- FAIL: .../ChangePassword/store_error
    --- FAIL: .../GetUserPermissions/store_error
    --- FAIL: .../BuildAuthContext/store_error
    --- FAIL: .../ValidateUserAPIKey/ErrNoRows
    --- FAIL: .../ValidateUserAPIKey/nil_user
FAIL	github.com/LeanerCloud/cloud-commitments-platform/internal/auth

On this branch: ok.

internal/credentials/resolver_test.go: TestRoleSessionName_SharedByAssumeRoleAndWebIdentity records the RoleSessionName that both the AssumeRole and the web-identity provider send to a fake STS client, for 22-, 8- and 5-character IDs. It is a pin for the refactor, not a fail-before test: both paths already sent the same value.

A16-014 mutation probe, on main: sed -i '444s/return true/return false/' internal/auth/group_ceiling.go gives --- FAIL: TestGrantCeiling_CarvedOutMayBeKeptNotWidened. The file was restored afterwards.

No DB schema or query changed: the only store change deletes an unused method. So there's no new integration test and no concurrency surface.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope / follow-ups

• User.Salt / users.salt column drop (see table).
• redactEmail consolidation: needs cloud-commitments-go pkg/logging and an owner decision on the masking rule (see table).
• MatchesAccount vs AccountScope.Allows: handled by fix(auth): bind group account scope entries to account IDs #1116.
• email.SenderInterface.SendWelcomeEmail and its template and implementations in internal/email (plus the mocks in internal/mocks, api, server, scheduler, purchase) have no production caller anywhere either. Deleting them is a separate internal/email cleanup.
• The MFA entry points (MFASetup, MFAEnable, MFADisable, MFARegenerateRecoveryCodes) still call GetUserByID directly. That's deliberate: they map every lookup failure to ErrMFAAuthFailed, so they were left alone.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/m.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 21 billable files and costs up to $5.25.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 503eb2e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5063143a-cb3c-46e4-8491-d6151aebd1a5

Commits

Reviewing files that changed from the base of the PR and between 503eb2e and 4d35ad1.

Files selected for processing: 21

• internal/auth/interfaces.go
• internal/auth/service.go
• internal/auth/service_apikeys.go
• internal/auth/service_apikeys_api.go
• internal/auth/service_apikeys_test.go
• internal/auth/service_credentials_db_test.go
• internal/auth/service_group.go
• internal/auth/service_password.go
• internal/auth/service_password_test.go
• internal/auth/service_test.go
• internal/auth/service_user.go
• internal/auth/service_user_lookup_test.go
• internal/auth/store_postgres_apikeys.go
• internal/auth/store_postgres_pgxmock_test.go
• internal/auth/test_helpers.go
• internal/auth/types.go
• internal/credentials/resolver.go
• internal/credentials/resolver_extra_test.go
• internal/credentials/resolver_test.go
• internal/mocks/stores.go
• internal/server/health_test.go

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

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 4d35ad1ef98e9eb91a2f3d895cf12d1f9e5040c4

Blocking:

• none

Non-blocking:

• internal/auth/service_user_lookup_test.go:35: lookupCallers() covers 10 of the 13 routed callers. HasAPIKeyPermissionForConstraintsAPI and GetAPIKeyEffectivePermissionsAPI (service_apikeys_api.go:428, :480, both now loadActiveUser) and ValidateSession (service.go:357, loadUser) aren't in the table. Their inactive-owner refusal is still pinned by existing tests: with the Active check removed from loadActiveUser, TestGetAPIKeyEffectivePermissionsAPI_FailsClosed/inactive_owner fails. Adding the three rows would make the "one contract on every caller" claim literal.
• The error text changes (user account not found -> user not found on the API-key path, and store errors now wrapped failed to get user: %w) don't change any status code. mapAuthError and the other handlers classify with errors.Is, and no handler, middleware or frontend code matches on the old strings (grepped internal/, cmd/, frontend/src).

Attacked:

• Checked every routed caller for a dropped check. The Active check survives on all three paths that had it: CreateAPIKey, ValidateUserAPIKey and the two *API key-permission paths, the old lookupAPIKeyUser callers. No path gains or loses an Active check. DeleteUser -> checkLastAdminConstraint is byte-identical (same count, same <= 1, same error), and the deferred-trigger backstop is unchanged. The four constant-time compares are still in place and only their comments changed. MFA and GetUser still call GetUserByID directly, as the body says. Removed symbols have no remaining references (grep). roleSessionName keeps the same "cudly-"+first-8 output.
• Ran on head 4d35ad1: go build ./..., go vet ./..., go vet -tags integration ./..., tests/e2e vet with -tags e2e, go test -race -short -count=1 on internal/{auth,credentials,mocks,server,api}, go test -race -tags integration -count=1 on internal/{auth,credentials} (testcontainers), and golangci-lint v2.10.1 on auth/credentials/mocks, with and without --build-tags integration. All clean.
• Fail-before: making loadUser return the raw store error again (main's behavior) fails TestUserLookup_UniformNotFoundAndStoreError/*/store_error for all 10 callers. Removing the Active check from loadActiveUser fails TestUserLookup_ActiveOnlyCallersRefuseInactiveUser/{CreateAPIKey,ValidateUserAPIKey} plus 3 existing tests. A16-014 probe: changing return true to return false at group_ceiling.go:444 fails TestGrantCeiling_CarvedOutMayBeKeptNotWidened, confirming the body's "already covered". All files restored after each probe.
• Refs #313, not Closes, is correct: User.Salt, redactEmail and MatchesAccount are still open, as the body says.

Body-stale:

• "pins one lookup contract on all ten routed callers": 13 callers are routed (see the first non-blocking item). The test covers 10.
• The combined-tree verification lists fix/issue-313 at 3156bc45. Head is now 4d35ad1e, a merge of origin/main on top of it. My checks above ran on the merged head.

_{Reviewer: rB14. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 4d35ad1ef98e9eb91a2f3d895cf12d1f9e5040c4

Blocking:

• none

Non-blocking:

• internal/auth/service_group.go:76 and :168: GetUserPermissions and BuildAuthContext now wrap store errors as failed to get user: %w. They used to return them unwrapped. errors.Is still works, and nothing in internal/api matches these strings, so behavior is unchanged. The body lists this correctly.
• User.Salt, redactEmail and MatchesAccount stay open, as the table says, and the PR uses Refs #313. That's correct for a partial fix.

Attacked:

• Active-check semantics (security). I mapped every routed caller against main. The callers that used to refuse inactive users (loadActiveUserForAPIKey in CreateAPIKey, lookupAPIKeyUser in ValidateUserAPIKey, HasAPIKeyPermissionForConstraintsAPI and GetAPIKeyEffectivePermissionsAPI) all go to loadActiveUser now. The callers without an active check (ListUserAPIKeys, authorizeAPIKeyAccess, GetUserPermissions, BuildAuthContext, ChangePassword, UpdateUserProfile) go to loadUser. No path gained or lost an active check. Mutation probe: disabling the !user.Active check in loadActiveUser fails TestService_ValidateUserAPIKey/fail_when_user_is_inactive, TestService_CreateAPIKey/fail_when_user_is_inactive and TestGetAPIKeyEffectivePermissionsAPI_FailsClosed/inactive_owner.
• DeleteUser → checkLastAdminConstraint is the same code (same count, same <= 1, same wrapped error). roleSessionName produces the same strings on both STS paths. The four ConstantTimeCompare calls are unchanged; only their comments changed. The deleted symbols (UpdateAPIKeyLastUsed, UpdateLastUsed, ResolveGCPCredentials, Role* consts, SendWelcomeEmail on the auth interface) have no remaining callers: go build ./... plus go vet ./... with and without -tags integration are clean.
• Fail-before: restoring only main's unwrapped return nil, err in loadUser fails the store_error subtests of TestUserLookup_UniformNotFoundAndStoreError (UpdateUser, DeleteUser, ChangePassword, GetUserPermissions, BuildAuthContext and more). A16-014 probe: return true → return false at group_ceiling.go:444 fails TestGrantCeiling_CarvedOutMayBeKeptNotWidened, which confirms that case is already covered on main. Files restored.
• Ran go test -race -short -count=1 on auth, credentials, mocks, server and api, go test -race -tags integration -count=1 ./internal/auth/ ./internal/credentials/ (testcontainers), and golangci-lint with and without --build-tags integration on the changed packages (0 issues).

Body-stale:

• none found. The verification commands name main a14dc8f2, which is older than today's main, but the branch has main merged in and everything above passes on the current head.

_{Reviewer: rB15. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 4d35ad1ef98e9eb91a2f3d895cf12d1f9e5040c4 merged onto main (5f717aa4), together with #1260, #1261, #1263, #1264, #1269, #1271, #1273, #1308, #1416 in that order (combined tree 62127c84, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 4d35ad1e (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 4d35ad1ef98e9eb91a2f3d895cf12d1f9e5040c4.