[IAmJSD]

Summary

This is the backend half of the #134 item "MFA step retains the plaintext password in module scope after an abandoned login". The item's fix direction says to "prefer holding a server-issued short-lived MFA continuation token over the raw password".

Root cause: POST /api/auth/login with a correct password for an MFA-enrolled user returned only 401 {"error":"mfa_required"}. The only way to finish the login was to send password + mfa_code again, so the browser had to keep the plaintext password until the user typed the code.

Fix:

• The mfa_required answer now also carries mfa_token and mfa_token_expires_at. The token is 32 random bytes. Only its SHA-256 is stored, in the new mfa_login_tokens table (migration 000152), using the same hash helper as session tokens.
• The token is bound to its user and to the users.password_version that the password step verified. A password change between the two steps makes it invalid, as does MFA being turned off or the user being deleted (FK cascade).
• It expires after 5 minutes (MFALoginTokenTTL, computed on the database clock) and allows 3 code attempts (MaxMFALoginTokenAttempts).
• New request form: {email, mfa_token, mfa_code} with no password. email is required and must name the token's user.
• The password + mfa_code form is unchanged and keeps working until the frontend switches.
• Error codes: invalid_mfa_code for a wrong code (unchanged), new invalid_mfa_token for an unknown, expired, used, exhausted or wrong-account token (the client starts again from the password), and invalid credentials for locked or inactive accounts (unchanged).
• If the token can't be stored, login answers with a bare mfa_required as before, and the old form still works.

Concurrency:

• Every request carrying a live token spends one attempt before the code is checked: UPDATE ... SET attempts = attempts + 1 WHERE token_hash = $1 AND expires_at > NOW() AND attempts < $max RETURNING .... The row lock plus READ COMMITTED re-evaluation means at most $max code checks happen per token, however many requests race.
• After a correct code, the token is deleted with DELETE ... WHERE token_hash = $1 AND expires_at > NOW(). Only the request that actually deletes the row gets a session, so the token is single use even when two different valid factors (a TOTP code and a recovery code) arrive at the same time.

Rate limiting and lockout:

• The token form goes through the same login handler, so the per-IP login bucket applies.
• Because email and mfa_code are required in the token form, the per-account login_mfa bucket (sec(auth): TOTP replay window and no lockout on MFA lifecycle failures (follow-up to #94) #442) applies too. It is keyed by email, and a mismatched email can never succeed, so leaving it out or faking it doesn't escape the bucket.
• A wrong code still calls RecordFailedLogin, so it counts toward the account lockout (5).
• The per-token cap (3) is deliberately below the lockout, so a user who mistypes is sent back to the password step before the account locks.

Request validation:

• The handler returns 400, without calling the service or spending a rate-limit slot, when a body mixes password and mfa_token, or sends mfa_token without email or mfa_code.
• The service also refuses those shapes.

Refs #134

Changes

• internal/database/postgres/migrations/000152_mfa_login_tokens.{up,down}.sql: new table (hash PK, user FK ON DELETE CASCADE, password_version, attempts >= 0, expires_at), with indexes on expires_at and user_id. Expired rows are removed whenever a new token is issued.
• internal/auth/store_postgres_mfa_login_tokens.go: CreateMFALoginToken, ClaimMFALoginTokenAttempt, ConsumeMFALoginToken. These are added to StoreInterface and to the three test doubles (auth.MockStore, mocks.MockAuthStore, server.mockAuthStoreForHealth).
• internal/auth/service_mfa_login_token.go: MFARequiredError (it unwraps to ErrMFARequired, so every existing errors.Is check still holds), issueMFAChallenge, loginWithMFAToken, claimMFALoginToken.
• internal/auth/service.go: Login routes the token form and issues the challenge. The TOTP/recovery-code check is moved out of verifyPasswordAndMFA into verifyLoginMFACode so both forms share it. Its behavior is unchanged.
• internal/auth/errors.go: new ErrInvalidMFAToken. internal/auth/types.go and internal/api/types.go: new LoginRequest.MFAToken. internal/server/app.go: the adapter forwards the field.
• internal/api/handler_auth.go: validateLoginForm and mapLoginError. The token and its expiry are flattened into the 401 body through NewClientErrorWithDetails.
• internal/api/openapi.yaml: documents both login forms, mfa_token on LoginRequest (password is no longer listed as required), and a new LoginError 401 schema with the four error codes plus mfa_token / mfa_token_expires_at.

Verification

Commands run locally on the branch (Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):

go build ./...                                   # ok
go vet ./... && go vet -tags integration ./...   # ok
go test -count=1 ./internal/auth/                # ok
go test -count=1 ./internal/api/                 # ok
go test -count=1 ./internal/server/              # ok
go test -count=1 ./internal/mocks/               # ok
golangci-lint run --allow-parallel-runners ./internal/auth/ ./internal/api/ ./internal/server/ ./internal/mocks/                                          # 0 issues
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/auth/ ./internal/database/postgres/migrations/ ./internal/server/          # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>   # nothing over 10
go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_MFALoginToken' ./internal/auth/                 # ok (real Postgres, testcontainers)
go test -race -tags integration -count=1 -timeout 20m -run 'TestIntegration_MFALoginToken' ./internal/auth/           # ok
go test -tags integration -count=1 -timeout 20m -run 'TestMigration_MFALoginTokens' ./internal/database/postgres/migrations/   # ok

New tests:

• Integration, real Postgres (internal/auth/service_mfa_login_token_db_test.go, TestIntegration_MFALoginToken), one subtest per scenario:
  • happy-path-stores-hash-and-signs-in: the hash is stored and the raw token is not.
  • expired: no code is checked, no attempt is spent, and the row is removed on the next issue.
  • reused: a recovery code does not bring a used token back, and no recovery code is spent.
  • wrong-user: A's token with B's email or B's code, and the reverse, never produce a session.
  • too-many-attempts: the token is spent after 3 wrong codes while the account is still unlocked, and a fresh password login works.
  • password-changed-voids-token.
  • old-password-and-code-form-still-works: it also leaves the token alone, and a non-MFA user gets no token.
  • concurrent-guesses-capped: 12 parallel wrong guesses result in exactly 3 code checks and failed_login_attempts == 3.
  • concurrent-valid-codes-one-session: a barrier holds both requests until each has passed its code check (one TOTP, one recovery code), then exactly one session is created.
• Migration (000152_mfa_login_tokens_test.go): up, constraints (unique and non-empty hash, attempts >= 0), cascade on user delete, and down drops the table. The predecessor comes from previousMigrationVersion(t, 152).
• Unit, MockStore (internal/auth/service_mfa_login_token_test.go, plus updated TestService_Login_MFA / TestLogin_WithMFA_MissingCode): happy path, unclaimable token, claim error, malformed form, user / password-version / MFA-off mismatch, locked account, a wrong code that records a failure, a lost consume race, the challenge storing only the hash, and the tokenless fallback when storing fails.
• Handler unit (internal/api/handler_auth_login_mfa_limit_test.go): the 401 body carries mfa_token and mfa_token_expires_at; the token form is forwarded and invalid_mfa_token is mapped; mixed or incomplete forms get a 400 that spends no login_mfa slot; the token form and the password form share one per-email login_mfa budget.
• Full-stack unit (internal/server/adapter_mfa_token_test.go, TestLogin_MFATokenRoundTrip): router, handler, authServiceAdapter and the real auth.Service with a mocked store. Password step, then token, then a session.

Fail-before / pass-after. This is local evidence: unit and mock tests plus real-Postgres integration tests through testcontainers.

• The new tests do not compile against origin/main, because LoginRequest.MFAToken and MFARequiredError don't exist there.
• With Login on the branch reverted to main's behavior (no token issued, token form not routed), these fail: TestLoginWithMFAToken_HappyPath, TestLogin_MFATokenRoundTrip, and all 9 TestIntegration_MFALoginToken subtests. Restoring the fix makes them pass.
• Mutation probes against the integration test each fail the named subtests:

Mutation	Subtests that fail
drop attempts < $max from the claim	too-many-attempts, concurrent-guesses-capped
ignore the consume result	concurrent-valid-codes-one-session
drop the expiry check from the claim	expired
drop the user-ID binding	wrong-user
drop the password-version binding	password-changed-voids-token

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

Branch	Tested head
fix/issue-712-fargate	2df9723
fix/issue-41-ri-exchange-wiring	bb548fe
fix/issue-349-ci-hygiene	360ae6c
fix/issue-756-next-collection	2cb1b2f
fix/issue-36-go-clock	aaea757
fix/issue-36-fe-waits	7feb01d
fix/issue-35-purchase-asserts	fbafeb0
fix/issue-134-mfa-token	33da813

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. The full suite ran on 644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase and internal/server all pass. Migration 000152 applies on top of main's 000151.
• Frontend: the full jest suite passes on this tree (175 suites, 4595 tests, 1 skipped).
• 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: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run on this tree (CI runs them). Nothing was applied to any cloud.

Out of scope

• Every frontend item in chore(frontend/core): medium/low findings from the 2026-07-28 full review #134, including the frontend half of this one (UI owner). The rest of chore(frontend/core): medium/low findings from the 2026-07-28 full review #134 stays open.
• A periodic cleanup job for the table. Expired rows are removed each time a token is issued, and exhausted rows that haven't expired yet live at most 5 minutes, so the table can't grow without bound.
• Removing the password + mfa_code form. That should happen after the frontend switches, in a separate change.

Frontend follow-up for the UI owner

frontend/src/auth.ts (pendingMFAPassword) and frontend/src/api/auth.ts (login):

1. On a 401 with error === "mfa_required", read mfa_token from the body (and mfa_token_expires_at if you want a countdown or auto-reset). Clear the password field and drop pendingMFAPassword right away. Keep only the email and the token.
2. Submit the code as POST /api/auth/login with {"email": <email>, "mfa_token": <token>, "mfa_code": <code>} and no password key. Sending both password and mfa_token is a 400.
3. On invalid_mfa_code, keep the token and let the user retry. After 3 wrong codes the token is spent.
4. On invalid_mfa_token (expired, used, too many wrong codes, password changed), clear the token and return to the password step with a message such as "Your sign-in timed out, please enter your password again". Also clear the token when the user abandons the step ("Back to login", tab hidden for a long time, or after mfa_token_expires_at).
5. Fallback: if an mfa_required answer has no mfa_token (the server couldn't store one), the old password + mfa_code resend still works. The UI can either keep that path for this case or ask for the password again.

Notes for reviewers

• email is required in the token form, although the issue text only mentions {mfa_token, code}. Requiring it is what keeps the per-account login_mfa bucket (sec(auth): TOTP replay window and no lockout on MFA lifecycle failures (follow-up to #94) #442) on this path, and it gives a concrete "wrong user" check.
• An attempt is spent before the email and user checks, so a request with a mismatched email also uses up one of the token's 3 attempts. Only someone who already passed the password step holds a token.
• password_version is the same binding sessions use (auth: password-rotation credential invalidation is best-effort, not atomic #398). A concurrent password change after the token is checked still revokes the new session through checkSessionUser.

Please mirror the issue's triage labels onto this PR: effort/l, type/chore, triaged, priority/p3, severity/low, urgency/eventually, impact/many.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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 33da813987becd22e201a9370a7b278d77066740

Blocking:

• none

Non-blocking:

• internal/auth/service_mfa_login_token.go:86-97 (loginWithMFAToken): the code is verified, and a recovery code is persisted as spent, before ConsumeMFALoginToken runs. A request that loses the consume (a concurrent second factor, or the token expiring between claim and consume) burns that recovery code and gets no session. This is rare and fails closed, but a one-line comment would help, or consume-then-verify with a re-issue on a wrong code.
• Migration numbering: 000152 is right against main (main ends at 000151; common.txt's "main is at 000148" note is out of date). The held feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241 also uses 000152, so it has to renumber after this merges, and fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by #1253 (000153) and fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239 (000154/155) have to stay above it.
• mapLoginError: mfa_token_expires_at is formatted with RFC3339, so it loses sub-second precision. That doesn't matter for a countdown. I note it only because the OpenAPI schema says date-time and that is still correct.
• The token is accepted without checking the account's current MFA secret generation. If MFA is disabled and re-enabled between the two steps, the token still works with a code from the new secret. The password_version binding plus the MFAEnabled check make this harmless, because the holder already proved the password.

Attacked:

• Read the full Login / verifyPasswordAndMFA / getUserAndValidateStatus path. Checked these:
  • An unknown token returns invalid_mfa_token before any user lookup, so there is no email enumeration.
  • A locked or inactive account is still refused on the token form.
  • A wrong code still calls recordFailedLogin.
  • The token can't be used as a session token or the other way round (separate tables).
  • The login (IP) and login_mfa (email) buckets both apply. A mismatched email can't succeed, so faking it doesn't escape the per-email bucket.
  • The adapter passes *MFARequiredError through unwrapped, handleRequestError flattens details without letting them shadow error, and the current frontend reads only error, so the extra 401 fields are harmless.
  • Migration: FK cascade, the hash CHECK, both indexes, and the down drops the table. 000152 sits after main's 000151.
• Ran: go build ./...; go vet with and without -tags integration on auth/api/server/mocks/migrations; go test -race -short -count=1 on auth, api, server and mocks (all ok); go test -race -tags integration -count=1 -run TestIntegration_MFALoginToken ./internal/auth/ (ok, real Postgres); the migrations package integration run for 000152 (ok).
• Fail-before (mutation): I dropped user.ID != claim.UserID from the claim check and replaced attempts < $2 with a no-op. wrong-user, too-many-attempts and concurrent-guesses-capped all fail. Restored afterwards.

Body-stale:

• none found. The claims I checked (fallback when storing fails, 3 attempts / 5 minutes, 400 on mixed forms, Refs rather than Closes chore(frontend/core): medium/low findings from the 2026-07-28 full review #134 since the frontend half is still open) match the code.

_{Reviewer: rI3. 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 33da813987becd22e201a9370a7b278d77066740

Blocking:

• none

Non-blocking:

• internal/auth/service_mfa_login_token.go claimMFALoginToken: a small account-existence oracle that the password form doesn't have (sec(api): hash approval and revocation tokens at rest #416 equalized that form). With a live token for account A, the token form answers differently depending on the email sent:

  • an existing, active, unlocked account B gets invalid_mfa_token (user.ID != claim.UserID);
  • a nonexistent, inactive or locked email gets invalid credentials (from getUserAndValidateStatus).

  So anyone who holds A's password can test 3 emails per password login. It needs valid credentials and is rate-limited, so it isn't blocking. The fix is cheap: load the user by claim.UserID and compare the email case-insensitively, or map every getUserAndValidateStatus failure in this path to ErrInvalidMFAToken except when the token owner's own account is locked.

• loginWithMFAToken: when a TOTP request and a recovery-code request race on one token, the request that loses ConsumeMFALoginToken has already burned its TOTP step or recovery code (the concurrent-valid-codes-one-session test shows only that one session results). The user would have to race themselves for this to happen, so it is harmless. A comment noting it would save a future reader the analysis.

• Migration number: 000152 is right on top of main's 000151. Held feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241 also carries a 000152, so whichever lands second has to renumber (common practice for the held PRs already).

• Until the frontend half ships, every MFA password step mints a 5-minute, 3-attempt token that the client ignores. That is acceptable (it is stored hashed, swept on issue, and still needs a valid code), but the benefit only arrives with the frontend change, so it is worth tracking that half explicitly on chore(frontend/core): medium/low findings from the 2026-07-28 full review #134.

• internal/api/handler_auth.go is now 810 lines. It was already over the 500-line guideline before this PR.

Attacked:

• Auth paths:
  • Token form routing happens after email parsing. Mixed or incomplete bodies get a 400 before any rate-limit slot or attempt is spent.
  • The claim UPDATE (attempts < $max AND expires_at > NOW()) runs before the user lookup, so an unknown token can't reveal which user it belongs to.
  • The token is bound to user_id, password_version (bumped by the 000103 trigger on every hash change) and MFAEnabled.
  • Locked and inactive accounts are refused, and a wrong code still calls recordFailedLogin.
  • Only the request whose DELETE affects the row reaches completeSuccessfulLogin.
  • The raw token appears only in the 401 details. MFARequiredError.Error() is plain mfa_required, so logging the error can't leak the token.
  • The legacy password + code form is unchanged apart from the extract-method into verifyLoginMFACode.
• Migration: the table is additive and IF NOT EXISTS, with no backfill or locks on existing tables. Old app versions ignore it, and the down migration is a lossy-by-design DROP. If the table is missing, the new app falls back to a bare mfa_required.
• Fail-before:
  • I removed the token routing and issueMFAChallenge from Login. TestLoginWithMFAToken_HappyPath and TestLogin_MFATokenRoundTrip then FAIL.
  • My own mutation (not in the body): I dropped the !user.MFAEnabled binding. TestLoginWithMFAToken_BindingMismatchRefused/mfa-disabled then FAILS.
  • Both reverted.
• Ran on 33da813:
  • go build ./..., and go vet ./... with and without -tags integration
  • go test -race -short -count=1 on ./internal/{auth,api,server,mocks} (all ok)
  • go test -race -tags integration -count=1 on the full internal/auth (174s), internal/database/postgres/migrations (292s), internal/server and internal/api (262s) packages on real Postgres (all ok)
  • golangci-lint v2.10.1 on the changed packages with and without --build-tags integration (0 issues)
  • gocyclo -over 10 on the changed non-test files (silent)

Body-stale:

• none

_{Reviewer: rI2. 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 33da813987becd22e201a9370a7b278d77066740 merged onto main (76180820), together with #1571, #1568, #1570 in that order (combined tree cf2e973e, 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 33da8139 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 33da813987becd22e201a9370a7b278d77066740.