fix(auth): password policy + current-password check for the superuser (#1285)

[frankbria]

Summary

Implements #1285: [P2.49] a password policy for the sole superuser, and a current-password check before its password or email can change.

• UserManager.validate_password → password_policy_error(): at least 12 characters, and not the email (case-insensitive). fastapi-users calls it on register and on every password PATCH, "" included, so one hook covers all three routes.
• UserManager.update: changing the password or email requires current_password, checked against the target user on /users/me and on /users/{id}. When one PATCH changes both email and password, the new password is also checked against the new email (codex review P2).
• UserUpdate.current_password is an optional field. It is not a column, and UserRead never returns it.
• Offline cf auth set-password applies the same rule; without it, it would be the way around the policy.
• Web sign-up form: minLength={12} on register only. Sign-in has none, because an existing account may predate the policy.
• cf auth register and the web sign-up form both show the policy reason. The web form used to map every register 400 to "email already exists" (claude-review).

Acceptance Criteria

• validate_password requires a minimum length of 12 and rejects a password equal to the email: register + both PATCH routes
• The form enforces the same minLength
• A PATCH that changes the password or email requires the current password
• A test asserts 400 for a short password on each route (tests/auth/test_password_policy_1285.py, 21 tests)

Demo evidence (live codeframe serve on a fresh DB, tasks/demo_1285.sh)

register password 'a'                          -> 400 "Password must be at least 12 characters."
register password = email                      -> 400 "Password must not be the account's email address."
register 21-char password                      -> 201 is_superuser=true
PATCH /users/me  password=''  (+current)       -> 400
PATCH /users/2   password='short' (+current)   -> 400
PATCH /users/me  new password, no current      -> 400 "The current password is required ..."
PATCH /users/2   new email, no current         -> 400
PATCH /users/me  new password, correct current -> 200, and login with it -> 200
cf auth set-password ... --password short      -> "Error: Password must be at least 12 characters." exit 1

Browser (Playwright on next dev): sign-up with a 5-character password shows "Please lengthen this text to 12 characters or more", and no /auth/register request is sent.

Test Plan

• TDD: 15 of 20 new tests failed first (the other 5 were the expected-green controls). The new-email case was red before its fix.
• Auth suites (tests/auth, the auth/login/rate-limit/enforcement UI tests, audit and disabled-account tests): all pass after the fixture bump
• Web: login.test.tsx, 8 passing, including a new minLength assertion
• ruff + mypy clean
• Cross-family review: codex, 3 rounds.
  • Round 1 reported a P1 that came from my own mutation run. codex read the tree while a mutation was applied; the committed code was intact.
  • Round 2 found a real P2 (new password equal to the new email); it is fixed.
  • Round 3 was clean.
• Internal review: self-review only (advisory)
• Mutation check: 6 of 7 caught. The one survivor was the create_update_dict override stripping current_password, which turned out to be dead code, so it was removed.

Known Limitations / Intentionally Deferred

• Passwords shorter than 12 that already exist still work. There is no forced rotation, so the rule applies only when a password is set.
• /users/{id} checks the target user's own current password. Today there is only one real account (a second user is [P2.67] Hosted-launch prerequisites (not needed for self-host): no way to create a second user, no per-tenant spend cap, no tenant-owned LLM keys #1303), so an admin resetting someone else's password is not a flow that exists. When [P2.67] Hosted-launch prerequisites (not needed for self-host): no way to create a second user, no per-tenant spend cap, no tenant-owned LLM keys #1303 adds one, it should check the acting admin's password on that route.
• current_password guessing on PATCH is not rate-limited by this PR. Guessing needs a valid JWT for that account. /users/* has no auth rate limit dependency; the users router could get enforce_auth_rate_limit if that matters.
• No breach-list or complexity rules. The AC is length plus not-the-email.
• is_active/is_superuser changes through /users/{id} need no current_password. That route is already superuser-only, and the AC covers the password and email only.

Implementation Notes

• Test data: registration fixtures in test_registration_bootstrap.py and test_v2_auth_enforcement.py used secret123 (9 characters) and now use a 12+ character value. The login page tests likewise. No assertion changed.
• test_atomic_writes_954::...[rotate] fails on this machine with and without this branch: the real OS keyring leaks into it, and it passes under CODEFRAME_DISABLE_KEYRING=1. It is unrelated to this change and will be filed separately.

Closes #1285

[frankbria]

Cross-family review (codex, 3 rounds)

• Round 1: a P1 that my own concurrent mutation run caused (codex read the tree while the email check was mutated to False). The committed code was intact (manager.py:291).
• Round 2: a real P2, fixed in b174a21:

The current-password checks cover both update routes, but simultaneous email/password updates bypass the new password policy. This was reproduced directly through UserManager.update.

Review comment:

- [P2] Validate passwords against the updated email address — /home/frankbria/projects/codeframe/codeframe/auth/manager.py:274-278

• Round 3:

No actionable regressions were identified in the password-policy enforcement, current-password checks, offline recovery command, or registration UI. The added tests cover the main changed behaviors, though tests were not executed in this read-only environment.

[github-actions]

Claude finished @frankbria's task in 12m 30s —— View job

---

Bug-hunting review complete

• Pull PR context, full diff, and prior comments
• Read changed files + surrounding callers/callees to confirm or discard each suspicious hunk
• Post inline findings (severity | failure scenario | suggestion)
• Post summary table

Method note: read-only review — no builds/tests executed (per review guardrails). fastapi-users 15.0.5 / pydantic 2.12.2 semantics were verified from uv.lock, the repo's own pre-existing handling of its error contracts (auth_commands.py:468-470, tests/api/test_disabled_account_login_938.py), and the live-server evidence in the PR description.

severity	file:line	finding
minor	web-ui/src/lib/auth.ts:165 (anchored on web-ui/src/app/login/page.tsx:153)	Register-400 is unconditionally mapped to "An account with that email already exists." — this PR adds REGISTER_INVALID_PASSWORD as a second meaning of that status, so a password==email rejection (which passes the new client minLength) shows a false "email taken" message. The PR fixed the identical mapping in the CLI but not in the web UI. Inline comment

Verified clean (candidates investigated and discarded):

• UserManager.update override — signature/arg order matches the mounted get_users_router PATCH routes (/users/me safe, /users/{id} superuser); sensitive computation is fail-closed in every direction I could construct (case-differing email, password: null, password: "", same-email resends, name-only PATCHes). No path mutates hashed_password/email outside it: reset/verify routers are not mounted (auth/router.py:270-280), and the only direct writers are the offline CLI (auth_commands.py:1147) and the !DISABLED! seed.
• current_password remaining in create_update_dict() output after dropping the override — confirmed harmless: the User model is a plain declarative model (no slots, no such column), so the adapter's setattr leaves a transient attribute that is neither persisted nor returned by UserRead. Matches the dead-code removal claim.
• _current_password_matches — UnknownHashError handling for the seeded !DISABLED! row is correct and mirrors the proven authenticate override ([P1.20] Return 400, not 500, when logging in as the seeded admin@localhost placeholder #938); empty/absent current_password fails closed.
• Register/patch enforcement surfaces — CLI register now prints detail.reason (escaped, [P3.8] Replace the ad-hoc Rich-markup scanner test with a real lint rule #1054-compliant); set-password applies the policy before touching the DB; e2e/lifecycle/rate-limit fixtures all use ≥12-char passwords (the rate-limit test never reaches the manager — form-encoded → 422).
• Not defects, already documented in the PR: /users/{id} checks the target user's password; no rate limit on current_password guessing; pre-policy short passwords still log in.

The "Test Suite (Unit + E2E)" CI run was still in progress at review time; nothing above depends on its outcome.

[claude]

Review

Scope: password policy (>=12 chars, not-the-email) plus a current_password requirement for changing password/email on /users/me and /users/{id}, mirrored in cf auth set-password.

Code quality / correctness

• codeframe/auth/manager.py: the UserManager.update() override and the sensitive computation are clean and match the stated intent — checked against the target user on both PATCH routes, so the sole superuser can't dodge the check by addressing its own id. Good catch (and good test, test_the_new_password_cannot_be_the_new_email) on the fastapi-users quirk where validate_password is called against the pre-update row, so a same-PATCH email+password pair could slip past the not-the-email rule — the extra password_policy_error(changes["password"], changes["email"]) check closes that.
• _current_password_matches correctly reuses the UnknownHashError handling pattern already established for the seeded !DISABLED! row ([P1.20] Return 400, not 500, when logging in as the seeded admin@localhost placeholder #938), and except UnknownHashError is scoped tightly.
• The current_password field's lifecycle is well-understood by the author (PR description explains that SQLAlchemyUserDatabase.update will setattr() it as an unmapped attribute, that it's transient, and that UserRead never returns it) — confirmed by reading schemas.py; no leak.
• cf auth set-password and cf auth register reuse the shared password_policy_error() rather than re-implementing the rule, and the CLI's dict-vs-string handling of error_detail in register() matches the actual {"code": ..., "reason": ...} shape fastapi-users puts on InvalidPasswordException — exercised by the new tests' REGISTER_INVALID_PASSWORD assertion.
• Rich-escaping: the touched console.print in auth_commands.py uses escape(str(error_detail)), consistent with the CLAUDE.md rule against unescaped Rich output; print_error is used in set_password. Good.

Minor / non-blocking

• UserManager.update()'s sensitive check only gates password/email; changing is_active/is_superuser via /users/{id} still needs no current_password. With one user today that's low-risk and outside the stated AC, but worth a one-line note in the code or the "Known Limitations" section if it's a deliberate scope cut rather than an oversight.
• Edge case I couldn't execute/verify in this sandbox: sending an explicit "password": null in the PATCH body (as opposed to omitting the field) passes pydantic's Optional[str] validation, and changes.get("password") is not None would then be False, so the new current-password gate is skipped. Whether that then reaches password_policy_error(None, ...) (which would len(None) and raise TypeError rather than a clean 400) depends on whether fastapi-users' own _update() guards None before calling validate_password. If it doesn't, an explicit null would 500 instead of 400. Worth a quick test ({"password": None}) to confirm a graceful 400 rather than an unhandled exception — low severity since it requires deliberately sending null rather than omitting the field.

Test coverage

Thorough — 21 new tests across registration, both PATCH routes (/users/me and /users/{id}), the email-change path (including the same-PATCH email/password interaction that was the codex-flagged P2), the offline CLI command, and web-ui minLength behavior for register-only. TestOfflineSetPassword asserting get_db_for_cli is never called on a bad password is a nice way to pin "fail before touching the DB."

Security

This closes a real hole (permanent account takeover via a long-lived, non-revocable JWT once one is leaked) and the fix is appropriately scoped — no over-reach into unrelated auth surface. The known limitations called out in the PR description (no rate limiting on current_password guessing, /users/{id} checking the target's own password rather than an admin's) are accurately described as deferred, not silently ignored.

Overall: solid, well-tested, well-documented security fix. Nothing blocking; the null-password edge case above is worth a quick manual check but not a required change before merge.

[frankbria]

Response to the claude-review summary:

• "password": null: this is not a bypass and does not return 500. fastapi-users treats null as "no change" and never calls validate_password for it. test_an_explicit_null_password_changes_nothing now pins a 200 with the old password still working.
• is_active/is_superuser without current_password: this is out of scope on purpose. The route is superuser-only, and the AC covers the password and email. It is now listed under Known Limitations.

[github-actions]

Claude finished @frankbria's task in 8m 42s —— View job

---

Bug-hunting review — re-run on the latest push

Scope: the two changes in 08e0630 (the normalizeAuthError register-400 fix + the "password": null pinning test), re-verified against the full diff vs origin/main and the callers/callees around each. Read-only — no builds or tests executed on this runner (no venv present); fastapi-users semantics were verified against uv.lock (15.x) and the repo's own pinned behavior.

• Pull the full diff and the commits new since the last review round
• Verify the normalizeAuthError register-400 fix in web-ui/src/lib/auth.ts (and its tests)
• Verify the "password": null claim — read the pinning test and the fastapi-users 15.x update() path
• Re-read UserManager.update / password_policy_error final state for bypasses
• Check CLI set-password / register consistency with the server-side policy
• Sweep for other surfaces that register or PATCH users and would break under the new policy
• Post summary

✅ GLM review: no defects found.

severity	file:line	finding
—	—	none confirmed on 08e0630

What was verified (candidates investigated and discarded):

• normalizeAuthError fix (web-ui/src/lib/auth.ts:165-171) — correct in both directions: a dict detail with a non-empty reason string surfaces the policy reason; the string detail REGISTER_USER_ALREADY_EXISTS still maps to "email already exists". typeof detail?.reason is safe against string/null/array details, the login-400 branch short-circuits before it, and both cases are pinned in auth.test.ts (00655dc). The prior round's finding is properly closed.
• {"password": null} → 200 no-change (manager.py:280-305) — consistent with fastapi-users 15.x BaseUserUpdate, whose prepare model-validator deletes None-valued fields so they are absent from hasattr(user_update, "password") and from any model_dump — no validate_password(None) path, so no len(None) 500. The override's own guards (changes.get("password") is not None, and .get("email") is not None before indexing changes["email"]) are safe for the same deleted-field reason, and test_an_explicit_null_password_changes_nothing now pins the outcome in CI.
• Gate fail-closed matrix (manager.py:289-298) — non-null password (incl. ""), any differing email (case-only change included), or both, all require the target row's current password; absent/empty/wrong current_password → 400; the seeded !DISABLED! hash → UnknownHashError → fail closed; same-email resend and name-only PATCHes stay open, matching the tests.
• Policy cross-check — register (new email), password-only PATCH (old row via super()), and same-PATCH email+password (override, manager.py:301-304) each catch password==email; the only residual ordering — changing the email to equal the current password in a later PATCH — requires the current password to perform, so it crosses no privilege boundary.
• CLI — set-password applies the identical password_policy_error(password, email) against the target email before touching the DB (pinned by TestOfflineSetPassword); register prints detail.reason for dict details, Rich-escaped ([P3.8] Replace the ad-hoc Rich-markup scanner test with a real lint rule #1054).
• Collateral sweep — every registration fixture now uses ≥12-char values consistently for register and login (test_registration_bootstrap.py, test_v2_auth_enforcement.py); the short "nope"/"pw123" strings remaining in the tree are login-failure or axios-mocked paths that never reach validate_password; no other web-UI or CLI surface PATCHes /users/me, so nothing silently breaks under the new current_password requirement; UserRead (schemas.py) has no current_password, and the transient setattr from the update dict is neither persisted nor returned.
• minLength (login/page.tsx:153) — isRegister ? 12 : undefined omits the attribute in React for sign-in (pinned both ways in login.test.tsx); the server policy remains the backstop for direct API callers.

Not defects, already documented by the author in the PR/known limitations: /users/{id} checks the target user's own password; no rate limit on current_password guessing; is_active/is_superuser PATCHes ungated; pre-policy short passwords still log in.

[claude]

Review

Scope: password policy (at least 12 chars, not-the-email) on register + both PATCH routes, plus a current_password gate for changing password/email, mirrored in cf auth set-password.

This PR already went through three codex rounds and two prior claude reviews (bug-hunting + general), and every point raised there has actually been addressed in the current diff:

• The web UI register 400 handler (web-ui/src/lib/auth.ts) no longer collapses a password-policy rejection into email already exists -- it now prefers detail.reason first, with a passing test for both cases.
• password: null on PATCH is pinned by test_an_explicit_null_password_changes_nothing as a no-op 200, not a TypeError/500 -- fastapi-users treats an explicit null as no change and never calls validate_password for it.
• The is_active/is_superuser-without-current_password scope cut is now called out explicitly in Known Limitations.
• The codex-flagged same-PATCH email == new password gap is closed by the extra password_policy_error(changes[password], changes[email]) check in UserManager.update (codeframe/auth/manager.py:301-304), since fastapi-users own validate_password hook only sees the pre-update email.

I re-read manager.py, schemas.py, auth_commands.py, and the web UI changes independently and did not find a new correctness or security issue beyond what has already been raised and fixed. A few smaller notes:

• Order of checks is fine but slightly non-obvious: a PATCH with both a wrong current_password and a too-short new password reports current password required rather than the length error, because the gate in update() runs before super().update() reaches validate_password. Not a bug -- just means the length/email-policy message only surfaces once the current password is right.
• Minor gap, pre-existing pattern, not introduced here: AuditEventType.USER_UPDATED exists in codeframe/lib/audit_logger.py but is never emitted anywhere in the codebase, including from the new UserManager.update() override. The authenticate() override in the same file already treats a bad login attempt as exactly the kind of signal an audit trail exists for (repeated ones are the signature of credential stuffing -- see its docstring at [P1.19] Emit audit events for auth, authorization and API-key lifecycle, or delete the taxonomy #937); a wrong current_password on /users/me or /users/{id} is the equivalent signal for an account-takeover attempt via a leaked JWT, which is the exact threat this PR's description says it is closing. Worth a follow-up (not blocking this PR) to audit both successful sensitive updates and failed current_password attempts, the same way failed logins are audited today.
• Case handling: the sensitive check compares changes[email] != user.email (case-sensitive) while password_policy_error's not-the-email check is case-insensitive. This can only make the gate more conservative (a case-only email edit still demands current_password), not less -- no bypass, just noting the asymmetry.

Test coverage: thorough and well-organized -- registration, both PATCH routes, the email-change interaction, the offline CLI path, and the web UI minLength/error-mapping behavior are all covered (21 backend tests + web tests). The TestOfflineSetPassword test asserting get_db_for_cli is never called on a bad password is a nice way to pin fail before touching the DB.

Security: the fix is scoped correctly and closes a real hole (a leaked, non-revocable 24h JWT converting into a permanent account takeover). Known limitations (no rate limit on current_password guessing, /users/{id} checking the target's password rather than an admin's until a second real user exists) are accurately documented rather than silently left out.

Overall: solid, and consistent with the extensive review already done on this PR. Nothing here should block merge; the audit-logging gap is worth a follow-up issue rather than scope creep on this one.