[IAmJSD]

Root cause

ConfirmPasswordReset (internal/auth/service_password.go) returned ErrAccountDeactivated for a live, unexpired reset token of an admin-deactivated user, and resetPassword (internal/api/handler_auth.go) mapped it to 403 {"error":"account is deactivated"}. Unknown and used tokens get 400 {"error":"invalid or expired reset token"}. That made POST the only place the deactivated state could be seen, and it disagreed with GET /api/auth/reset-password/status, which since #592 reports the same token as used.

Fix

• New sentinel auth.ErrInvalidResetToken ("invalid or expired reset token"). validateResetToken now returns it for the three unknown/mismatch cases; the message is unchanged.
• The deactivated branch in ConfirmPasswordReset returns that same sentinel. The check itself is unchanged: the reset is still refused before any password write or reactivation (A03-006).
• Token is still consumed. That stops replay, and it also makes the later GET status answer used, the same as for an unknown token, so GET and POST now agree.
• Removed ErrAccountDeactivated and the handler's 403 branch. The sentinel goes through the existing isResetPasswordClientError path and comes back as a 400 with a body identical to an unknown token's.
• Forgot-password (anti-enumeration) behavior is unchanged.

Closes #595

Verification

All evidence is from unit, mock-store or testcontainers runs. No real cloud, email or deploy was involved.

Regression test: TestHandleRequest_ConfirmPasswordReset_DeactivatedUserLooksLikeUnknownToken in internal/server/reset_token_status_deactivated_test.go. It goes through Handler.HandleRequest into the real auth.Service with only the store mocked. It replaces the old test that asserted 403. It sends POST with a deactivated user's live token and with an unknown token, then checks that status, headers and JSON body are equal (400), and that the body does not contain "deactivated". The mock expectations check that the token is consumed and that nothing else is written to the store, so there is no password write.

Fail-before (fix stashed, new test kept):

expected: 400  actual: 403   body: {"error":"account is deactivated"}
expected: {"error":"invalid or expired reset token"}  actual: {"error":"account is deactivated"}
FAIL github.com/LeanerCloud/cloud-commitments-platform/internal/server

Pass-after: ok internal/server.

Updated tests:

• internal/auth/service_password_test.go: the deactivated-account case now expects ErrInvalidResetToken with the exact unknown-token message. It still checks that Active stays false, the password hash is unchanged and the token is consumed.
• internal/api/handler_auth_test.go: TestHandler_resetPassword_AccountDeactivated (403) is replaced by TestHandler_resetPassword_InvalidResetToken, which expects 400 and the exact message.
• internal/auth/service_credentials_db_test.go (integration): the replay after a concurrent deactivation now expects ErrInvalidResetToken.

Commands. The /dev/null AWS env vars are there only because this machine's default ~/.aws/config profile is invalid.

go build ./...
go vet ./internal/auth ./internal/api ./internal/server
go vet -tags=integration ./...
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/auth ./internal/api ./internal/server -count=1   # ok
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -short -race ./...                                       # all ok
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -tags integration ./internal/auth -run TestIntegration_ResetConfirmRejectsStaleRead -count=1 -v   # PASS incl. after-concurrent-deactivation (testcontainers Postgres)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/auth/... ./internal/api/... ./internal/server/...   # 0 issues
gocyclo -over 10 internal/auth/errors.go internal/auth/service_password.go internal/api/handler_auth.go           # silent
bash scripts/gosec-hook.sh <changed .go files>                                                                     # no findings

git-secrets is not installed here, so that hook was not run.

Out of scope / notes

• The deactivated path still makes one store write (ConsumePasswordResetToken) that the unknown-token path does not, so there is a small timing difference. Exploiting it needs a live 256-bit token, so I left it alone.
• If an account is deactivated during a confirm, the confirm still answers 409 ErrUserChanged (sec(auth): stale whole-row user writes can still erase MFA fields #493). That is a race-only signal and is not changed here; a replay afterwards gets the generic 400.
• internal/auth/service_password.go (594 lines) and internal/api/handler_auth.go (664 lines) were already over the 500-line guideline. This PR changes them by +2 and -3 lines.

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

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: e9d9e75d-5c2f-4b93-8d61-b0b9cbe3120a

📥 Commits

Reviewing files that changed from the base of the PR and between 33cd663 and 6d20289.

📒 Files selected for processing: 7

• internal/api/handler_auth.go
• internal/api/handler_auth_test.go
• internal/auth/errors.go
• internal/auth/service_credentials_db_test.go
• internal/auth/service_password.go
• internal/auth/service_password_test.go
• internal/server/reset_token_status_deactivated_test.go

+---------------------------------------------------+
| This is not a microservice. This is a macro-mess. |
+---------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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 of 6d20289: draft: blocks ready

No blocking correctness or security findings in the diff. Remaining blocks are gate items only.

Findings

1. Nit, pre-existing, out of scope: an expired token answers 400 "reset token has expired" (service_password.go:533), while unknown/used/deactivated answer 400 "invalid or expired reset token". Expiry vs unknown is therefore distinguishable, as it was before this PR. Deactivated is now indistinguishable from unknown, which is what #595 asks for. Optional follow-up.
2. Nit: a deactivated live token runs one extra DB write (ConsumePasswordResetToken, service_password.go:398) that an unknown token does not. This is a theoretical timing signal for someone who already holds a live 256-bit token; no practical exposure. Consuming the token is the right call (no replay after reactivation).

What I attacked

• Status/body/headers: new HandleRequest test (internal/server/reset_token_status_deactivated_test.go) compares deactivated vs unknown on status, headers and JSON body, and asserts the body lacks "deactivated". Handler maps the error via isResetPasswordClientError substring "invalid or expired reset token" (handler_auth.go:304), so 400 is retained.
• Fail-before: restored the 3 production files from origin/main with the new test; it fails (403, body "account is deactivated"). Passes at HEAD.
• Consumption and replay: deactivated path still calls ConsumePasswordResetToken before returning (service_password.go:397-401), so the token cannot be replayed after reactivation. Password write is unreachable: the return precedes processPasswordReset/CompletePasswordReset; the mock fails on any other store call.
• Other ErrAccountDeactivated callers: grep over the tree finds none left (only the removed handler branch). Status endpoint already reports "used" for deactivated (service_password.go:503-508); forgot-password already answers identically (service_password.go:324-327). Login was not changed here and was not in #595 scope.
• go test ./internal/auth/... ./internal/api/... ./internal/server/...: 4587 passed.
• One mutation probe attempt (drop the consume call) was malformed and reverted without running; not claimed as evidence. Worktree clean.

Gate status

• Independent review: this comment, on the SHA above.
• Findings resolved: none actionable.
• CI on SHA: pending when checked (E2E, pre-commit, terraform checks pass; Unit Tests, Lint, Integration, Security still pending).
• Labels: match #595 (triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/xs, type/bug).
• Local real-path evidence: the HandleRequest test above with mocked store, fail-before/pass-after shown; the PR itself does not yet record this, so this comment is the record.
• PR is a draft from an external contributor.

Blocks ready

• CI green on this SHA (Unit Tests, Lint Code, Integration Tests, Security Scanning still pending).
• Mark the PR ready for review (currently draft).

[cristim]

Gate record for 6d20289 (origin/main CLAUDE.md "Review gate"):

• Independent review of 6d20289: no actionable findings.
  • A deactivated user's live reset token now gets the same status, headers and JSON body as an unknown token.
  • The password write is unreachable for deactivated users; the return comes before processPasswordReset and CompletePasswordReset.
  • The token is consumed, so it can't be replayed after reactivation.
  • No other ErrAccountDeactivated leak remains in sec(auth): POST reset-password answers 403 'account is deactivated' for a live token of a deactivated user, unlike unknown/used/expired tokens #595's scope.
• CI on 6d20289: CI Success and Run pre-commit hooks pass.
• Local verification (macOS, mocked store):
  • go test on internal/auth, internal/api and internal/server: 4587 passed.
  • Fail-before: with the three production files restored from origin/main, the new HandleRequest test fails with 403 "account is deactivated". It passes here.
• CR skipped: draft and quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(auth): POST reset-password answers 403 'account is deactivated' for a live token of a deactivated user, unlike unknown/used/expired tokens #595.

Marking ready (owner-authorized) and merging at 6d20289 with --match-head-commit.