feat: Password change required page

[gcgoncalves]

Screen.Recording.2026-08-18.at.17.03.14.mov

[marekdano]

Findings

Blocking

1. Password-expiry-flagged accounts get permanently locked out of both login and the recovery flow.
server/src/routes/auth/change-password-required.ts:136-145, src/api/loginErrors.ts, server/src/routes/auth/login.ts

Verified against the upstream backend: mcpgateway/routers/email_auth.py's /auth/email/login computes needs_password_change from three sources — the persisted password_change_required flag, password-age expiry (never persisted to the DB), and default-password detection. It blocks with 403 in all three cases. But the BFF's bypass step in change-password-required.ts calls the Tier-1 /auth/login (mcpgateway/routers/auth.py), which doesn't compute needs_password_change at all — it just returns the persisted password_change_required flag. The BFF's only gate (bypassAuth.user?.password_change_required !== true) reads that persisted flag.

Result: a user blocked purely by password age (flag never persisted) is redirected to the recovery page by the frontend, but the BFF then rejects them with 403 password_change_not_required and revokes the bypass token — stuck, unable to log in or recover. password_change_enforcement_enabled defaults true and password_max_age_days defaults 90 in mcpgateway/config.py:1011,1017, so this is reachable under default configuration, not an edge case.

Functionally-Impacting

2. Password-mutation request omits client-IP audit headers.
server/src/routes/auth/change-password-required.ts:150-158 (Step B, POST /auth/email/change-password)

Both sibling calls in the same handler (Step A bypass login, Step D real login) forward x-forwarded-for/x-real-ip; the actual password-change mutation — the most security-sensitive call in the flow — does not. Upstream's audit log attributes the change to the BFF's IP instead of the real user's. Confirmed independently by two review passes plus direct source inspection.

3. 403 password_change_not_required is misclassified as "invalid old password."
src/api/changePasswordRequiredErrors.ts:29-31

if (error.status === 401 || error.status === 403) return { kind: "invalidOldPassword" };

The route's own header comment distinguishes 401/403 as "re-authentication rejected" from the distinct {error:"password_change_not_required"} 403 (correct old password, account just doesn't need a change — e.g. stale link, flag cleared elsewhere). The classifier doesn't check body.error for that case, so the UI shows "wrong password" and offers the forgot-password fallback even when credentials were correct.

4. Missing email validation allows blank-email submission.
src/pages/PasswordChangeRequired.tsx (~line 89 handleSubmit, ~line 228 submit-disabled condition)

If the page is reached without ?email= (stale link, manual nav, bookmark), the read-only email field is blank but the form remains submittable. The BFF 400s generically; the classifier maps 400 to policyViolation on the new-password field, hiding the real cause (missing email) from the user.

5. password_change_required is enforced only at login time, not continuously.
src/router/index.tsx (AuthGuard), server/src/plugins/session.ts

AuthGuard gates purely on isAuthenticated; it never consults password_change_required. The safety of the whole flow depends on every login code path independently re-implementing upstream's 403 check. A future login path (SSO callback, admin impersonation) that calls establishSession() directly would silently mint a full session for a flagged account, and nothing downstream would catch it.

Suggestions

6. Best-effort token revocation blocks the response unnecessarily.
server/src/routes/auth/change-password-required.ts:143, 167, 174

await revokeUpstreamToken(...) is awaited before replying on the failure branches and before Step D's real login on the success path — up to 3s of avoidable latency for a best-effort, log-only side effect that has no data dependency on what follows it.

7. AuthContext provider value isn't memoized.
src/auth/AuthContext.tsx (~line 221)

The context value object — now also carrying the new completePasswordChangeRequired closure — is a fresh literal every render; any unrelated state change in AuthProvider re-renders every consumer app-wide.

8. Upstream-login request/response handling is triplicated.
server/src/routes/auth/login.ts, server/src/routes/auth/change-password-required.ts (upstreamLogin() helper + Step D inline fetch)

The same fetch/timeout/header/JSON-parse/access_token-check pattern appears three times across two files instead of being factored alongside the new establishSession/revokeUpstreamToken helpers. A contract change to the upstream login response requires updating all three call sites by hand.

Minor

9. Password policy's "special character" regex over-counts whitespace and accented letters. src/lib/passwordPolicy.ts

/[^A-Za-z0-9]/ matches spaces/control characters and non-ASCII letters (á, ñ, ç). A password like Passwordá123 satisfies 4 character classes when the user likely intended none beyond letters/digits; a trailing space also counts toward complexity.

10. expires_in warning fires even when the field is simply absent, not just malformed. server/src/lib/establish-session.ts

The warn-log branch doesn't distinguish "upstream omitted expires_in" from "upstream sent a negative/non-finite value." If upstream ever omits the field by design, logs get spammed implying something is wrong when it's expected.

[marekdano]

The third commit (b10e443) addresses all four issues I flagged as blocking or functionally-impacting:

• ✅ 1. Lockout bug — precondition now correctly uses /auth/email/login (not /auth/login) so password-age-expired accounts can reach the recovery flow
• ✅ 2. Audit IP headers — x-forwarded-for / x-real-ip now forwarded on the change-password mutation
• ✅ 3. Error misclassification — password_change_not_required checked against body.error before the 403 status catch-all
• ✅ 4. Blank-email form — emailMissing guard renders an error + "Return to Login" instead of a submittable form

The module-level comment in change-password-required.ts explaining the two-endpoint strategy is genuinely valuable — please keep it.

Remaining open items to track as follow-up:

• 5. AuthGuard doesn't enforce password_change_required continuously — low risk today (one login path), but worth a dedicated issue before adding SSO/impersonation flows
• 8. upstreamLogin is still partially triplicated — the inline Step D fetch could move into the helper
• 9. [^A-Za-z0-9] in passwordPolicy.ts counts spaces and accented letters as special characters
• 10. expires_in warning fires on absent field, not just malformed values
  None of these block merge. The core flow is correct and the critical bugs are fixed.

LGTM 🚀

[marekdano]

🟡 Functionally-Impacting

completePasswordChangeRequired doesn't clear stale auth state on failure
src/auth/AuthContext.tsx

Compare login() vs completePasswordChangeRequired() on the failure path:

// login() — cleans up on failure:
setCsrfToken(null);
authVersion.current += 1;
setState({ user: null, isAuthenticated: false, isLoading: false, selectedTeamId: null });
throw err;

// completePasswordChangeRequired() — does not:
throw err;  // ← nothing else

If a user arrives at this page while still authenticated (bookmarked link, active session) and the BFF returns an unexpected error, stale isAuthenticated: true state and an old CSRF token can linger in memory. This is low-risk today (this is a pre-auth page, one login path), but the asymmetry with login() is a latent bug for future edge cases.

Suggested fix — mirror login()'s cleanup:

} catch (err) {
  setCsrfToken(null);
  authVersion.current += 1;
  setState({ user: null, isAuthenticated: false, isLoading: false, selectedTeamId: null });
  throw err;
}

[marekdano]

The PR resolves bugs and security concerns raised during the initial review, introduces strong validation, adds rigorous E2E coverage, and implements focus-routing.

LGTM 🚀 after green CI checks!