From 429e350a63cb1f5475e48cda53fad26df410f819 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Tue, 18 Aug 2026 13:31:03 +0100 Subject: [PATCH 1/4] Change pw page Signed-off-by: Gabriel Costa --- e2e/auth/login-flow.spec.ts | 36 +++ e2e/auth/password-change-required.spec.ts | 127 +++++++++ e2e/fixtures/api-mock.ts | 34 +++ e2e/utils/paths.ts | 2 + server/src/index.ts | 2 + server/src/lib/establish-session.ts | 65 +++++ server/src/lib/revoke-upstream-token.ts | 44 +++ .../routes/auth/change-password-required.ts | 226 ++++++++++++++++ server/src/routes/auth/login.ts | 56 +--- server/src/routes/auth/logout.ts | 24 +- server/test/auth.test.ts | 226 +++++++++++++++- server/test/helpers/build-app.ts | 2 + src/App.tsx | 2 + src/api/changePasswordRequiredErrors.test.ts | 51 ++++ src/api/changePasswordRequiredErrors.ts | 36 +++ src/api/client.ts | 7 +- src/api/loginErrors.test.ts | 47 ++++ src/api/loginErrors.ts | 53 ++++ src/auth/AuthContext.test.tsx | 52 ++++ src/auth/AuthContext.tsx | 31 +++ src/components/auth/AuthCard.tsx | 30 +++ src/components/auth/AuthSuccessPanel.tsx | 53 ++++ src/components/layout/HeaderQuickNav.test.tsx | 1 + .../mcp-servers/AdvancedSettings.test.tsx | 1 + src/components/prompts/PromptForm.test.tsx | 3 + src/components/users/PasswordInput.tsx | 146 +++++----- src/hooks/usePromptForm.test.ts | 1 + src/i18n/locales/en-US/auth.json | 19 ++ src/i18n/locales/es-ES/auth.json | 19 ++ src/i18n/locales/pt-BR/auth.json | 19 ++ src/lib/passwordPolicy.ts | 9 + src/pages/ForgotPassword.tsx | 134 +++++----- src/pages/Login.test.tsx | 50 ++++ src/pages/Login.tsx | 16 +- src/pages/PasswordChangeRequired.test.tsx | 145 ++++++++++ src/pages/PasswordChangeRequired.tsx | 250 ++++++++++++++++++ src/pages/ResetPassword.tsx | 215 +++++++-------- src/pages/Users.test.tsx | 1 + src/router/index.tsx | 6 +- 39 files changed, 1896 insertions(+), 345 deletions(-) create mode 100644 e2e/auth/password-change-required.spec.ts create mode 100644 server/src/lib/establish-session.ts create mode 100644 server/src/lib/revoke-upstream-token.ts create mode 100644 server/src/routes/auth/change-password-required.ts create mode 100644 src/api/changePasswordRequiredErrors.test.ts create mode 100644 src/api/changePasswordRequiredErrors.ts create mode 100644 src/api/loginErrors.test.ts create mode 100644 src/api/loginErrors.ts create mode 100644 src/components/auth/AuthCard.tsx create mode 100644 src/components/auth/AuthSuccessPanel.tsx create mode 100644 src/lib/passwordPolicy.ts create mode 100644 src/pages/PasswordChangeRequired.test.tsx create mode 100644 src/pages/PasswordChangeRequired.tsx diff --git a/e2e/auth/login-flow.spec.ts b/e2e/auth/login-flow.spec.ts index 6bbf51e..16439b3 100644 --- a/e2e/auth/login-flow.spec.ts +++ b/e2e/auth/login-flow.spec.ts @@ -44,6 +44,42 @@ test.describe("Login flow", () => { expect(token).toBeNull(); }); + test("403 password-change-required response redirects to the change-password page", async ({ + page, + apiMock, + }) => { + await apiMock.mockLogin({ + status: 403, + detail: JSON.stringify({ + detail: "Password change required. Please change your password before continuing.", + }), + }); + + await page.goto(APP.LOGIN); + await page.getByLabel(/email address/i).fill("test@example.com"); + await page.getByLabel(/password/i).fill("old-password"); + await page.getByRole("button", { name: /^sign in$/i }).click(); + + await expect(page).toHaveURL( + new RegExp(`${APP.CHANGE_PASSWORD_REQUIRED}\\?email=test%40example\\.com$`), + ); + }); + + test("403 response with unrelated detail keeps user on the login page", async ({ + page, + apiMock, + }) => { + await apiMock.mockLogin({ status: 403, detail: JSON.stringify({ detail: "Forbidden" }) }); + + await page.goto(APP.LOGIN); + await page.getByLabel(/email address/i).fill("test@example.com"); + await page.getByLabel(/password/i).fill("password123"); + await page.getByRole("button", { name: /^sign in$/i }).click(); + + await expect(page).toHaveURL(new RegExp(`${APP.LOGIN}$`)); + await expect(page.getByRole("alert")).toHaveText(/login failed/i); + }); + test("500 response surfaces generic failure message", async ({ page, apiMock }) => { await apiMock.mockLogin({ status: 500 }); diff --git a/e2e/auth/password-change-required.spec.ts b/e2e/auth/password-change-required.spec.ts new file mode 100644 index 0000000..c4990b7 --- /dev/null +++ b/e2e/auth/password-change-required.spec.ts @@ -0,0 +1,127 @@ +import { test, expect } from "../fixtures/api-mock"; +import { APP, TOKEN_STORAGE_KEY } from "../utils/paths"; + +test.describe("Password change required flow", () => { + test.beforeEach(async ({ page, apiMock }) => { + await apiMock.mockSession({ authenticated: false }); + await page.addInitScript((key) => { + window.sessionStorage.removeItem(key); + }, TOKEN_STORAGE_KEY); + }); + + test("successful change lands the user straight in the app, no manual re-login", async ({ + page, + apiMock, + }) => { + await apiMock.mockChangePasswordRequired(); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await expect(page.getByLabel(/email address/i)).toHaveValue("test@example.com"); + + await page.getByLabel(/current password/i).fill("old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + await expect(page).toHaveURL(new RegExp(`${APP.ROOT}$`)); + await expect(page.getByRole("heading", { name: /dashboard/i })).toBeVisible(); + }); + + test("password changed but auto sign-in failed shows a fallback screen back to login", async ({ + page, + }) => { + await page.route("**/auth/change-password-required", async (route) => { + await route.fulfill({ + status: 502, + contentType: "application/json", + body: JSON.stringify({ error: "login_after_change_failed" }), + }); + }); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + await expect(page.getByRole("status")).toHaveText(/password changed/i); + await page.getByRole("button", { name: /return to login/i }).click(); + await expect(page).toHaveURL(new RegExp(`${APP.LOGIN}$`)); + }); + + test("invalid old password shows a fallback link to forgot-password", async ({ + page, + apiMock, + }) => { + await apiMock.mockChangePasswordRequired({ status: 401 }); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("wrong-old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + await expect(page.getByRole("alert")).toHaveText(/current password is incorrect/i); + await page.getByRole("button", { name: /forgot your password/i }).click(); + await expect(page).toHaveURL(new RegExp(`${APP.FORGOT_PASSWORD}$`)); + }); + + test("mismatched new passwords are rejected client-side", async ({ page }) => { + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("Different-password2"); + await page.getByRole("button", { name: /change password/i }).click(); + + await expect(page.getByText(/passwords do not match/i)).toBeVisible(); + }); + + test("focus moves to error input field when validation fails", async ({ page }) => { + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + + // Trigger new password validation error + await page.getByLabel(/current password/i).fill("old-password"); + await page.getByLabel(/^new password/i).fill("short"); + await page.getByLabel(/confirm new password/i).fill("short"); + await page.getByRole("button", { name: /change password/i }).click(); + + // Verify focus moved to the new password input + await expect(page.getByLabel(/^new password/i)).toBeFocused(); + }); + + test("focus moves to submit error when server returns error", async ({ page, apiMock }) => { + await apiMock.mockChangePasswordRequired({ status: 401 }); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("wrong-old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + // Verify focus moved to the error notification container + const errorContainer = page.locator('[role="alert"]').locator(".."); + await expect(errorContainer).toBeFocused(); + }); + + test("success heading receives focus after password changed but login failed", async ({ + page, + }) => { + await page.route("**/auth/change-password-required", async (route) => { + await route.fulfill({ + status: 502, + contentType: "application/json", + body: JSON.stringify({ error: "login_after_change_failed" }), + }); + }); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("old-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + // Verify focus moved to the success heading + const successHeading = page.getByRole("heading", { name: /password changed/i }); + await expect(successHeading).toBeFocused(); + }); +}); diff --git a/e2e/fixtures/api-mock.ts b/e2e/fixtures/api-mock.ts index 2858125..1302360 100644 --- a/e2e/fixtures/api-mock.ts +++ b/e2e/fixtures/api-mock.ts @@ -49,6 +49,18 @@ export interface ApiMock { */ mockPermissions(options?: { permissions?: string[] }): Promise; mockUnauthorized(urlPattern: string | RegExp): Promise; + /** + * Mocks POST /auth/change-password-required, the BFF's route used by + * PasswordChangeRequired.tsx (client/src/pages/) after a "password change + * required" login failure. On success it returns the same { user, + * csrfToken } shape as /auth/login — the BFF re-authenticates with the new + * password and establishes a real session as part of this one call. + */ + mockChangePasswordRequired(options?: { + user?: MockUser; + status?: number; + detail?: string; + }): Promise; } export function createApiMock(page: Page): ApiMock { @@ -102,6 +114,28 @@ export function createApiMock(page: Page): ApiMock { }); }, + async mockChangePasswordRequired({ + user = DEFAULT_TEST_USER, + status = 200, + detail = "Invalid credentials", + } = {}) { + await page.route("**/auth/change-password-required", async (route) => { + if (status === 200) { + await route.fulfill({ + status, + contentType: "application/json", + body: JSON.stringify({ user, csrfToken: MOCK_CSRF_TOKEN }), + }); + return; + } + await route.fulfill({ + status, + contentType: "application/json", + body: JSON.stringify({ error: "change_password_failed", detail }), + }); + }); + }, + async mockUnauthorized(urlPattern) { await page.route(urlPattern, async (route) => { await route.fulfill({ diff --git a/e2e/utils/paths.ts b/e2e/utils/paths.ts index 5cd9627..5828af4 100644 --- a/e2e/utils/paths.ts +++ b/e2e/utils/paths.ts @@ -10,6 +10,7 @@ export const APP = { LOGIN: "/app/login", FORGOT_PASSWORD: "/app/forgot-password", CHANGE_PASSWORD: "/app/change-password", // pragma: allowlist secret + CHANGE_PASSWORD_REQUIRED: "/app/change-password-required", // pragma: allowlist secret GATEWAYS: "/app/gateways", SERVERS: "/app/servers", TOOLS: "/app/tools", @@ -40,6 +41,7 @@ export const APP = { export const API = { LOGIN: "**/auth/login", SESSION: "**/auth/session", + CHANGE_PASSWORD_REQUIRED: "**/auth/change-password-required", } as const; /** diff --git a/server/src/index.ts b/server/src/index.ts index 2294db8..2147991 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -16,6 +16,7 @@ import redisPlugin from "./plugins/redis.js"; import sessionPlugin from "./plugins/session.js"; import staticPlugin from "./plugins/static.js"; import appRoute from "./routes/app.js"; +import changePasswordRequiredRoute from "./routes/auth/change-password-required.js"; import loginRoute from "./routes/auth/login.js"; import logoutRoute from "./routes/auth/logout.js"; import sessionRoute from "./routes/auth/session.js"; @@ -37,6 +38,7 @@ fastify.get("/healthz", async () => ({ ok: true })); await fastify.register(loginRoute); await fastify.register(logoutRoute); await fastify.register(sessionRoute); +await fastify.register(changePasswordRequiredRoute); await fastify.register(sseRoutes); await fastify.register(catchAllProxyRoute); await fastify.register(appRoute); diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts new file mode 100644 index 0000000..2ce3201 --- /dev/null +++ b/server/src/lib/establish-session.ts @@ -0,0 +1,65 @@ +// Location: ./client/server/src/lib/establish-session.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Given an upstream AuthenticationResponse, create the BFF session + rotate +// the CSRF secret + hand back what the caller needs to reply with. Shared by +// routes/auth/login.ts and routes/auth/change-password-required.ts so the +// JWT-lifetime/session-TTL matching and CSRF-rotation security properties +// live in exactly one place. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { config } from "../config.js"; +import { createSession, setSessionCookie, type SessionUser } from "./session-store.js"; +import { CSRF_COOKIE_NAME } from "../plugins/csrf.js"; + +// Mirrors mcpgateway.schemas.AuthenticationResponse. +export interface UpstreamAuthenticationResponse { + access_token: string; + expires_in: number; + user: SessionUser; +} + +export async function establishSession( + fastify: FastifyInstance, + request: FastifyRequest, + reply: FastifyReply, + auth: UpstreamAuthenticationResponse, +): Promise<{ user: SessionUser; csrfToken: string }> { + // The BFF session/cookie must not outlive the bearer token it wraps — use + // the upstream JWT's own lifetime, not a fixed BFF-side default. See + // createSession's comment in lib/session-store.ts. + let ttlSeconds = config.sessionTtlSeconds; + if (Number.isFinite(auth.expires_in) && auth.expires_in > 0) { + ttlSeconds = auth.expires_in; + } else { + // Upstream returned a bogus expires_in — fall back, but log it: this + // means the BFF session can outlive the JWT it wraps until the proxy's + // revoke-on-401 catches up (see session-store.ts). + request.log.warn( + { expires_in: auth.expires_in }, + "upstream login returned invalid expires_in, using BFF default session TTL", + ); + } + + const sessionId = await createSession( + fastify.redis, + { bearerToken: auth.access_token, user: auth.user }, + ttlSeconds, + ); + + setSessionCookie(reply, sessionId, ttlSeconds); + // generateCsrf() only mints a fresh secret when request.cookies has no + // bff_csrf entry — reply.clearCookie() alone doesn't clear that (it only + // queues an outgoing Set-Cookie, request.cookies is untouched), so delete + // it directly to force rotation. Otherwise a secret planted before login + // (subdomain XSS, a plaintext hop with COOKIE_SECURE=false) survives into + // the authenticated session. + delete request.cookies[CSRF_COOKIE_NAME]; + // Cookie holds the CSRF secret (HttpOnly); the SPA needs the derived token + // itself to echo back via X-CSRF-Token — see plugins/csrf.ts. + const csrfToken = await reply.generateCsrf(); + + return { user: auth.user, csrfToken }; +} diff --git a/server/src/lib/revoke-upstream-token.ts b/server/src/lib/revoke-upstream-token.ts new file mode 100644 index 0000000..7bb3687 --- /dev/null +++ b/server/src/lib/revoke-upstream-token.ts @@ -0,0 +1,44 @@ +// Location: ./client/server/src/lib/revoke-upstream-token.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Revokes an upstream bearer token via FastAPI's bearer-token logout +// (mcpgateway/routers/auth.py POST /auth/logout, blocklist-backed — DB or +// Redis depending on deployment). Without this, a token that's no longer +// reachable from the browser stays cryptographically valid until its +// natural TOKEN_EXPIRY. Best-effort: an upstream failure (network blip, +// already-revoked token) must never block whatever the caller is doing. +// +// Shared by routes/auth/logout.ts (revoking the real session token) and +// routes/auth/change-password-required.ts (revoking the short-lived bypass +// token minted via /auth/login). + +import type { FastifyRequest } from "fastify"; + +import { config } from "../config.js"; +import { upstreamAuthHeader } from "./upstream-auth.js"; + +// The caller is usually waiting on this request, so cap how long a hung +// (not refused) upstream can hold it open. +const UPSTREAM_REVOKE_TIMEOUT_MS = 3000; + +export async function revokeUpstreamToken( + request: FastifyRequest, + bearerToken: string, +): Promise { + try { + const response = await fetch(`${config.contextforgeUrl}/auth/logout`, { + method: "POST", + headers: upstreamAuthHeader(bearerToken), + signal: AbortSignal.timeout(UPSTREAM_REVOKE_TIMEOUT_MS), + }); + if (!response.ok) { + request.log.warn( + { status: response.status }, + "upstream token revocation returned a non-2xx status", + ); + } + } catch (err) { + request.log.warn({ err }, "upstream token revocation failed"); + } +} diff --git a/server/src/routes/auth/change-password-required.ts b/server/src/routes/auth/change-password-required.ts new file mode 100644 index 0000000..1166ad6 --- /dev/null +++ b/server/src/routes/auth/change-password-required.ts @@ -0,0 +1,226 @@ +// Location: ./client/server/src/routes/auth/change-password-required.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// POST /auth/change-password-required: browser -> BFF only, pre-login. Used +// when /auth/login rejected valid credentials with a "password change +// required" 403 (see routes/auth/login.ts, which authenticates against +// upstream's /auth/email/login — that endpoint hard-blocks with 403 for the +// whole lifetime of the password_change_required flag, confirmed against a +// live backend). This route instead: +// +// A. authenticates against upstream's plain /auth/login ("Tier 1" session +// auth), which does NOT enforce that flag and still mints a token for a +// flagged account, using the OLD password; the response's +// user.password_change_required is then checked server-side — this is +// the ONLY gate before mutating state, so it must actually be true, or +// this route would just be an unauthenticated "change password with old +// password" endpoint for any account; +// B. uses that bypass token once to call the authenticated change-password +// endpoint; +// C. revokes the bypass token upstream (best-effort — it's served its one +// purpose and shouldn't linger); +// D. logs in again for real, via the same upstream /auth/email/login +// login.ts uses, now with the NEW password (the block is gone once the +// password's been changed); +// E. establishes a normal BFF session from that — identical to a plain +// login, so a successful password change lands the user straight in +// the app. +// +// On success this returns the exact same { user, csrfToken } shape as +// POST /auth/login, so the SPA can treat it identically to a login. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { config } from "../../config.js"; +import { + establishSession, + type UpstreamAuthenticationResponse, +} from "../../lib/establish-session.js"; +import { revokeUpstreamToken } from "../../lib/revoke-upstream-token.js"; +import { setNoStore } from "../../lib/no-store.js"; +import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; +import type { SessionUser } from "../../lib/session-store.js"; +import { upstreamAuthHeader } from "../../lib/upstream-auth.js"; + +interface ChangePasswordRequiredBody { + email: string; + oldPassword: string; + newPassword: string; +} + +interface UpstreamBypassLoginResponse { + access_token: string; + user: SessionUser; +} + +// Caps each individual upstream leg — a hung (not refused) upstream must not +// hold the request open indefinitely, same rationale as +// revoke-upstream-token.ts's UPSTREAM_REVOKE_TIMEOUT_MS. +const UPSTREAM_REQUEST_TIMEOUT_MS = 3000; + +async function upstreamLogin( + request: FastifyRequest, + email: string, + password: string, +): Promise { + return fetch(`${config.contextforgeUrl}/auth/login`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-forwarded-for": request.ip, + "x-real-ip": request.ip, + }, + body: JSON.stringify({ email, password }), + signal: AbortSignal.timeout(UPSTREAM_REQUEST_TIMEOUT_MS), + }); +} + +export default async function changePasswordRequiredRoute(fastify: FastifyInstance): Promise { + fastify.post<{ Body: ChangePasswordRequiredBody }>( + "/auth/change-password-required", + async (request: FastifyRequest<{ Body: ChangePasswordRequiredBody }>, reply: FastifyReply) => { + setNoStore(reply); + + if (isForbiddenCrossOrigin(request)) { + return reply.code(403).send({ error: "cross_site_request_forbidden" }); + } + + const { email, oldPassword, newPassword } = request.body ?? {}; + if (!email || !oldPassword || !newPassword) { + return reply.code(400).send({ error: "email, oldPassword and newPassword are required" }); + } + + // Step A: mint a bypass token using the old (still-valid) credentials. + // Deliberately /auth/login, not /auth/email/login — the latter + // hard-blocks while password_change_required is set, the former does + // not (verified against a live backend). + let bypassLoginResponse: Response; + try { + bypassLoginResponse = await upstreamLogin(request, email, oldPassword); + } catch (err) { + request.log.error({ err }, "upstream bypass login (change-password-required) failed"); + return reply.code(502).send({ error: "upstream_unavailable" }); + } + + if (!bypassLoginResponse.ok) { + // Covers both "still password-change-blocked" and "old password + // wrong" — don't try to disambiguate via text; the SPA falls back to + // the forgot-password flow on any failure here. + const detail = await bypassLoginResponse.text(); + return reply + .code(bypassLoginResponse.status) + .send({ error: "change_password_failed", detail }); + } + + let bypassAuth: UpstreamBypassLoginResponse; // pragma: allowlist secret + try { + bypassAuth = (await bypassLoginResponse.json()) as UpstreamBypassLoginResponse; + } catch (err) { + request.log.error({ err }, "upstream bypass login returned a non-JSON 2xx body"); + return reply.code(502).send({ error: "upstream_invalid_response" }); + } + + if (typeof bypassAuth.access_token !== "string" || !bypassAuth.access_token) { + request.log.error( + { bypassAuth }, + "upstream bypass login 2xx response missing access_token", + ); + return reply.code(502).send({ error: "upstream_invalid_response" }); + } + const bypassToken = bypassAuth.access_token; + + // Precondition: this route only exists to unblock accounts upstream + // has actually flagged. Without this check, a valid old password alone + // would rotate ANY account's password through this pre-auth endpoint. + if (bypassAuth.user?.password_change_required !== true) { + request.log.warn( + { email }, + "change-password-required called for an account that does not require a password change", + ); + // Bypass token served no purpose — revoke it before replying, same + // as the change-password-failed branch below. + await revokeUpstreamToken(request, bypassToken); + return reply.code(403).send({ error: "password_change_not_required" }); + } + + // Step B: use the bypass token once, immediately, to change the password. + let changeResponse: Response; + try { + changeResponse = await fetch(`${config.contextforgeUrl}/auth/email/change-password`, { + method: "POST", + headers: { + "content-type": "application/json", + ...upstreamAuthHeader(bypassToken), + }, + body: JSON.stringify({ old_password: oldPassword, new_password: newPassword }), + signal: AbortSignal.timeout(UPSTREAM_REQUEST_TIMEOUT_MS), + }); + } catch (err) { + request.log.error({ err }, "upstream change-password failed"); + return reply.code(502).send({ error: "upstream_unavailable" }); + } + + if (!changeResponse.ok) { + // Password change itself failed (e.g. new-password policy violation) + // — the bypass token served no purpose, revoke it before replying. + await revokeUpstreamToken(request, bypassToken); + const detail = await changeResponse.text(); + return reply.code(changeResponse.status).send({ error: "change_password_failed", detail }); + } + + // Step C: the bypass token has done its one job — revoke it upstream + // rather than let it float until its natural expiry. Best-effort. + await revokeUpstreamToken(request, bypassToken); + + // Step D: the password is changed — log in for real with the new + // password, exactly like login.ts does, to get a full session. + let realLoginResponse: Response; + try { + realLoginResponse = await fetch(`${config.contextforgeUrl}/auth/email/login`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-forwarded-for": request.ip, + "x-real-ip": request.ip, + }, + body: JSON.stringify({ email, password: newPassword }), + signal: AbortSignal.timeout(UPSTREAM_REQUEST_TIMEOUT_MS), + }); + } catch (err) { + request.log.error({ err }, "post-change-password login failed"); + return reply.code(502).send({ error: "login_after_change_failed" }); + } + + if (!realLoginResponse.ok) { + // The password WAS changed successfully — this is not a + // change-password failure, it's a (rare) inability to establish a + // session right after. Distinct error so the SPA doesn't show a + // "wrong password"/policy-violation message for a change that + // actually succeeded. + request.log.error( + { status: realLoginResponse.status }, + "password changed but post-change login was rejected", + ); + return reply.code(502).send({ error: "login_after_change_failed" }); + } + + let realAuth: UpstreamAuthenticationResponse; // pragma: allowlist secret + try { + realAuth = (await realLoginResponse.json()) as UpstreamAuthenticationResponse; + } catch (err) { + request.log.error({ err }, "post-change-password login returned a non-JSON 2xx body"); + return reply.code(502).send({ error: "login_after_change_failed" }); + } + + if (typeof realAuth.access_token !== "string" || !realAuth.access_token) { + request.log.error({ realAuth }, "post-change-password login missing access_token"); + return reply.code(502).send({ error: "login_after_change_failed" }); + } + + // Step E: establish a normal BFF session, identical to login.ts. + const { user, csrfToken } = await establishSession(fastify, request, reply, realAuth); + return reply.send({ user, csrfToken }); + }, + ); +} diff --git a/server/src/routes/auth/login.ts b/server/src/routes/auth/login.ts index 0accce5..306299a 100644 --- a/server/src/routes/auth/login.ts +++ b/server/src/routes/auth/login.ts @@ -12,12 +12,13 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; import { clearSessionCookie, - createSession, deleteSession, - setSessionCookie, SESSION_COOKIE_NAME, - type SessionUser, } from "../../lib/session-store.js"; +import { + establishSession, + type UpstreamAuthenticationResponse, +} from "../../lib/establish-session.js"; import { setNoStore } from "../../lib/no-store.js"; import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; import { CSRF_COOKIE_NAME } from "../../plugins/csrf.js"; @@ -61,15 +62,6 @@ async function clearStaleSession( reply.clearCookie(CSRF_COOKIE_NAME, { path: "/", domain: config.cookieDomain }); } -// Mirrors mcpgateway.schemas.AuthenticationResponse. `user` is forwarded to -// the browser verbatim (see SessionUser) — the BFF only needs access_token -// and expires_in. -interface UpstreamAuthenticationResponse { - access_token: string; - expires_in: number; - user: SessionUser; -} - export default async function loginRoute(fastify: FastifyInstance): Promise { fastify.post<{ Body: LoginBody }>( "/auth/login", @@ -127,44 +119,8 @@ export default async function loginRoute(fastify: FastifyInstance): Promise 0) { - ttlSeconds = auth.expires_in; - } else { - // Upstream returned a bogus expires_in — fall back, but log it: this - // means the BFF session can outlive the JWT it wraps until the - // proxy's revoke-on-401 catches up (see session-store.ts). - request.log.warn( - { expires_in: auth.expires_in }, - "upstream login returned invalid expires_in, using BFF default session TTL", - ); - } - - const sessionId = await createSession( - fastify.redis, - { - bearerToken: auth.access_token, - user: auth.user, - }, - ttlSeconds, - ); - - setSessionCookie(reply, sessionId, ttlSeconds); - // generateCsrf() only mints a fresh secret when request.cookies has no - // bff_csrf entry — reply.clearCookie() alone doesn't clear that (it - // only queues an outgoing Set-Cookie, request.cookies is untouched), - // so delete it directly to force rotation. Otherwise a secret planted - // before login (subdomain XSS, a plaintext hop with COOKIE_SECURE=false) - // survives into the authenticated session. - delete request.cookies[CSRF_COOKIE_NAME]; - // Cookie holds the CSRF secret (HttpOnly); the SPA needs the derived - // token itself to echo back via X-CSRF-Token — see plugins/csrf.ts. - const csrfToken = await reply.generateCsrf(); - - return reply.send({ user: auth.user, csrfToken }); + const { user, csrfToken } = await establishSession(fastify, request, reply, auth); + return reply.send({ user, csrfToken }); }, ); } diff --git a/server/src/routes/auth/logout.ts b/server/src/routes/auth/logout.ts index 918d9b7..0f9aefc 100644 --- a/server/src/routes/auth/logout.ts +++ b/server/src/routes/auth/logout.ts @@ -26,31 +26,9 @@ import { SESSION_COOKIE_NAME, } from "../../lib/session-store.js"; import { CSRF_COOKIE_NAME } from "../../plugins/csrf.js"; -import { upstreamAuthHeader } from "../../lib/upstream-auth.js"; +import { revokeUpstreamToken } from "../../lib/revoke-upstream-token.js"; import { setNoStore } from "../../lib/no-store.js"; -// The user is waiting on this request, so cap how long a hung (not refused) -// upstream can hold it open. -const UPSTREAM_REVOKE_TIMEOUT_MS = 3000; - -async function revokeUpstreamToken(request: FastifyRequest, bearerToken: string): Promise { - try { - const response = await fetch(`${config.contextforgeUrl}/auth/logout`, { - method: "POST", - headers: upstreamAuthHeader(bearerToken), - signal: AbortSignal.timeout(UPSTREAM_REVOKE_TIMEOUT_MS), - }); - if (!response.ok) { - request.log.warn( - { status: response.status }, - "upstream token revocation returned a non-2xx status", - ); - } - } catch (err) { - request.log.warn({ err }, "upstream token revocation failed"); - } -} - export default async function logoutRoute(fastify: FastifyInstance): Promise { fastify.post( "/auth/logout", diff --git a/server/test/auth.test.ts b/server/test/auth.test.ts index dbe090e..19a4329 100644 --- a/server/test/auth.test.ts +++ b/server/test/auth.test.ts @@ -2,7 +2,7 @@ // Copyright contributors to the MCP-CONTEXT-FORGE project // SPDX-License-Identifier: Apache-2.0 -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { config } from "../src/config.js"; import { buildTestApp, type TestApp } from "./helpers/build-app.js"; @@ -222,6 +222,230 @@ describe("POST /auth/login", () => { }); }); +describe("POST /auth/change-password-required", () => { + afterEach(() => vi.unstubAllGlobals()); + + interface UpstreamCall { + url: string; + authorization: string | undefined; + } + + interface MockLegOptions { + ok?: boolean; + status?: number; + body?: unknown; + } + + interface MockUpstreamOptions { + bypassLogin?: MockLegOptions; // POST /auth/login (old password) + changePassword?: MockLegOptions; // POST /auth/email/change-password + revoke?: MockLegOptions; // POST /auth/logout (bypass token) + realLogin?: MockLegOptions; // POST /auth/email/login (new password) + } + + /** Mocks all four upstream legs the route can call, and records every call made. */ + function mockUpstream(options: MockUpstreamOptions = {}): UpstreamCall[] { + const legs = { + bypassLogin: { + ok: true, + status: 200, + body: { + access_token: "bypass-jwt", // pragma: allowlist secret + user: { email: "user@example.com", password_change_required: true }, + }, + ...options.bypassLogin, + }, + changePassword: { ok: true, status: 200, body: {}, ...options.changePassword }, + revoke: { ok: true, status: 200, body: {}, ...options.revoke }, + realLogin: { + ok: true, + status: 200, + body: { + access_token: "real-jwt", // pragma: allowlist secret + expires_in: 1200, + user: { email: "user@example.com" }, + }, + ...options.realLogin, + }, + }; + + const calls: UpstreamCall[] = []; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init?: RequestInit) => { + const headers = init?.headers as Record | undefined; + calls.push({ url: String(url), authorization: headers?.authorization }); + + const leg = String(url).endsWith("/auth/email/change-password") + ? legs.changePassword + : String(url).endsWith("/auth/email/login") + ? legs.realLogin + : String(url).endsWith("/auth/logout") + ? legs.revoke + : String(url).endsWith("/auth/login") + ? legs.bypassLogin + : null; + if (!leg) throw new Error(`unexpected upstream fetch: ${url}`); + + return { + ok: leg.ok, + status: leg.status, + json: async () => leg.body, + text: async () => JSON.stringify(leg.body), + }; + }), + ); + return calls; + } + + async function requestChange(payload?: Record) { + return app!.fastify.inject({ + method: "POST", + url: "/auth/change-password-required", + payload: payload ?? { + email: "user@example.com", + oldPassword: "old-secret", // pragma: allowlist secret + newPassword: "New-secret1", // pragma: allowlist secret + }, + }); + } + + let app: TestApp | undefined; + beforeEach(async () => { + app = await buildTestApp(); + }); + + it("re-authenticates with the old password, changes it, then establishes a real session with the new password", async () => { + mockUpstream(); + const response = await requestChange(); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ + user: { email: "user@example.com" }, + csrfToken: expect.any(String), + }); + const setCookieNames = response.cookies.map((c) => c.name); + expect(setCookieNames).toContain("bff_sid"); + expect(setCookieNames).toContain("bff_csrf"); + }); + + it("calls /auth/login (not /auth/email/login) for the bypass step, since the latter hard-blocks while the flag is set", async () => { + const calls = mockUpstream(); + await requestChange(); + + const urls = calls.map((c) => c.url); + expect(urls).toContain(`${config.contextforgeUrl}/auth/login`); + expect(urls).toContain(`${config.contextforgeUrl}/auth/email/change-password`); + expect(urls).toContain(`${config.contextforgeUrl}/auth/email/login`); + }); + + it("revokes the bypass token upstream after a successful change", async () => { + const calls = mockUpstream(); + await requestChange(); + + const revokeCall = calls.find((c) => c.url === `${config.contextforgeUrl}/auth/logout`); + expect(revokeCall?.authorization).toBe("Bearer bypass-jwt"); + }); + + it("never leaks either upstream token (bypass or real) to the browser", async () => { + mockUpstream(); + const response = await requestChange(); + + const raw = JSON.stringify(response.json()); + expect(raw).not.toContain("bypass-jwt"); + expect(raw).not.toContain("real-jwt"); + }); + + it("passes through the bypass-login failure status when the old password is rejected, without touching later steps", async () => { + const calls = mockUpstream({ + bypassLogin: { ok: false, status: 401, body: { detail: "Invalid email or password" } }, + }); + const response = await requestChange(); + + expect(response.statusCode).toBe(401); + expect(response.json().error).toBe("change_password_failed"); + expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); + expect(calls.map((c) => c.url)).toEqual([`${config.contextforgeUrl}/auth/login`]); + }); + + it("rejects the change when the account does not actually require a password change", async () => { + const calls = mockUpstream({ + bypassLogin: { + body: { + access_token: "bypass-jwt", // pragma: allowlist secret + user: { email: "user@example.com", password_change_required: false }, + }, + }, + }); + const response = await requestChange(); + + expect(response.statusCode).toBe(403); + expect(response.json()).toEqual({ error: "password_change_not_required" }); + expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); + // The bypass token served no purpose — it must still be revoked. + const revokeCall = calls.find((c) => c.url === `${config.contextforgeUrl}/auth/logout`); + expect(revokeCall?.authorization).toBe("Bearer bypass-jwt"); + // Never reaches the change-password step. + expect(calls.map((c) => c.url)).not.toContain( + `${config.contextforgeUrl}/auth/email/change-password`, + ); + }); + + it("passes through the change-password failure status when the new password is rejected, and still revokes the bypass token", async () => { + const calls = mockUpstream({ + changePassword: { + ok: false, + status: 400, + body: { detail: "Password must not be a commonly used password" }, + }, + }); + const response = await requestChange(); + + expect(response.statusCode).toBe(400); + expect(response.json().error).toBe("change_password_failed"); + expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); + const revokeCall = calls.find((c) => c.url === `${config.contextforgeUrl}/auth/logout`); + expect(revokeCall?.authorization).toBe("Bearer bypass-jwt"); + }); + + it("reports login_after_change_failed (not change_password_failed) when the password changed but the follow-up login fails", async () => { + const calls = mockUpstream({ + realLogin: { ok: false, status: 401, body: { detail: "unexpected" } }, + }); + const response = await requestChange(); + + expect(response.statusCode).toBe(502); + expect(response.json()).toEqual({ error: "login_after_change_failed" }); + expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); + // The password change itself did happen before the follow-up login failed. + expect(calls.map((c) => c.url)).toContain( + `${config.contextforgeUrl}/auth/email/change-password`, + ); + }); + + it("returns 502 when upstream is unreachable", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("upstream unreachable"); + }), + ); + + const response = await requestChange(); + expect(response.statusCode).toBe(502); + }); + + it("rejects a request missing fields before calling upstream", async () => { + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + + const response = await requestChange({ email: "user@example.com", oldPassword: "old-secret" }); // pragma: allowlist secret + + expect(response.statusCode).toBe(400); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); + describe("GET /auth/session", () => { afterEach(() => vi.unstubAllGlobals()); diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index a4218ee..b943568 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -13,6 +13,7 @@ import { type Redis } from "ioredis"; import cookiePlugin from "../../src/plugins/cookie.js"; import csrfPlugin from "../../src/plugins/csrf.js"; import sessionPlugin from "../../src/plugins/session.js"; +import changePasswordRequiredRoute from "../../src/routes/auth/change-password-required.js"; import loginRoute from "../../src/routes/auth/login.js"; import logoutRoute from "../../src/routes/auth/logout.js"; import sessionRoute from "../../src/routes/auth/session.js"; @@ -58,6 +59,7 @@ export async function buildTestApp(opts: { withProxy?: boolean } = {}): Promise< await fastify.register(loginRoute); await fastify.register(logoutRoute); await fastify.register(sessionRoute); + await fastify.register(changePasswordRequiredRoute); if (opts.withProxy) { await fastify.register(catchAllProxyRoute); diff --git a/src/App.tsx b/src/App.tsx index ec28ebd..47a621a 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -7,6 +7,7 @@ import { Login } from "./pages/Login"; import { ForgotPassword } from "./pages/ForgotPassword"; import { ResetPassword } from "./pages/ResetPassword"; import { ChangePassword } from "./pages/ChangePassword"; +import { PasswordChangeRequired } from "./pages/PasswordChangeRequired"; import { Dashboard } from "./pages/Dashboard"; import { Gateways } from "./pages/Gateways"; import { CreateServer } from "./pages/CreateServer"; @@ -46,6 +47,7 @@ function PublicRoutes() { + ); } diff --git a/src/api/changePasswordRequiredErrors.test.ts b/src/api/changePasswordRequiredErrors.test.ts new file mode 100644 index 0000000..3f22056 --- /dev/null +++ b/src/api/changePasswordRequiredErrors.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { ApiError } from "./client"; +import { classifyChangePasswordRequiredError } from "./changePasswordRequiredErrors"; + +describe("classifyChangePasswordRequiredError", () => { + it.each([ + [401, "invalidOldPassword"], + [403, "invalidOldPassword"], + [500, "failed"], + ])("maps HTTP %s to %s", (status, kind) => { + expect( + classifyChangePasswordRequiredError(new ApiError(status, null, `HTTP ${status}`)), + ).toEqual({ + kind, + }); + }); + + it("preserves 400 detail as a policy-violation message", () => { + const detail = "Password must not be a commonly used password"; + expect(classifyChangePasswordRequiredError(new ApiError(400, { detail }, "HTTP 400"))).toEqual({ + kind: "policyViolation", + message: detail, + }); + }); + + it("preserves 422 detail as a policy-violation message", () => { + const detail = "Password too short"; + expect(classifyChangePasswordRequiredError(new ApiError(422, { detail }, "HTTP 422"))).toEqual({ + kind: "policyViolation", + message: detail, + }); + }); + + it("handles a 400 without detail", () => { + expect(classifyChangePasswordRequiredError(new ApiError(400, null, "HTTP 400"))).toEqual({ + kind: "policyViolation", + message: null, + }); + }); + + it("maps a login_after_change_failed body to changedButLoginFailed, ahead of status-based checks", () => { + const body = { error: "login_after_change_failed" }; + expect(classifyChangePasswordRequiredError(new ApiError(502, body, "HTTP 502"))).toEqual({ + kind: "changedButLoginFailed", + }); + }); + + it("falls back to failed for a non-ApiError", () => { + expect(classifyChangePasswordRequiredError(new Error("network"))).toEqual({ kind: "failed" }); + }); +}); diff --git a/src/api/changePasswordRequiredErrors.ts b/src/api/changePasswordRequiredErrors.ts new file mode 100644 index 0000000..097d852 --- /dev/null +++ b/src/api/changePasswordRequiredErrors.ts @@ -0,0 +1,36 @@ +import { ApiError } from "./client"; +import { extractApiErrorDetail } from "@/utils/errors"; + +export type ChangePasswordRequiredError = + | { kind: "invalidOldPassword" } + | { kind: "policyViolation"; message: string | null } + // Password WAS changed successfully — the BFF just couldn't log back in + // and establish a session right after. Not a validation failure. + | { kind: "changedButLoginFailed" } + | { kind: "failed" }; + +/** + * Classify POST /auth/change-password-required failures. The BFF route + * (server/src/routes/auth/change-password-required.ts) forwards whichever + * upstream call failed first: a 401/403 means the re-authentication step + * (old password) was rejected; a 400/422 means the new password itself was + * rejected (policy); `{ error: "login_after_change_failed" }` means the + * change succeeded but the follow-up login/session-establishment didn't. + * Scoped to this endpoint only — do not merge with classifyLoginError or + * classifyPasswordResetError, which assign 403 different meanings for their + * own endpoints. + */ +export function classifyChangePasswordRequiredError(error: unknown): ChangePasswordRequiredError { + if (!(error instanceof ApiError)) return { kind: "failed" }; + + const body = error.body as { error?: string } | null; + if (body?.error === "login_after_change_failed") return { kind: "changedButLoginFailed" }; + + if (error.status === 401 || error.status === 403) return { kind: "invalidOldPassword" }; + + if (error.status === 400 || error.status === 422) { + return { kind: "policyViolation", message: extractApiErrorDetail(error.body) }; + } + + return { kind: "failed" }; +} diff --git a/src/api/client.ts b/src/api/client.ts index b469e2e..7e845b4 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -23,7 +23,12 @@ const LOGIN_PATH = "/app/login"; const API_PREFIX = "/api"; const SESSION_CHECK_PATH = "/auth/session"; -const BFF_OWNED_AUTH_PATHS = new Set(["/auth/login", "/auth/logout", SESSION_CHECK_PATH]); +const BFF_OWNED_AUTH_PATHS = new Set([ + "/auth/login", + "/auth/logout", + "/auth/change-password-required", + SESSION_CHECK_PATH, +]); export class ApiError extends Error { constructor( diff --git a/src/api/loginErrors.test.ts b/src/api/loginErrors.test.ts new file mode 100644 index 0000000..03d3e61 --- /dev/null +++ b/src/api/loginErrors.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import { ApiError } from "./client"; +import { classifyLoginError } from "./loginErrors"; + +describe("classifyLoginError", () => { + it("maps 401 to invalidCredentials regardless of body", () => { + expect(classifyLoginError(new ApiError(401, null, "HTTP 401"))).toEqual({ + kind: "invalidCredentials", + }); + }); + + it("detects password-change-required from a double-JSON-encoded 403 detail", () => { + const body = { + error: "login_failed", + detail: JSON.stringify({ + detail: "Password change required. Please change your password before continuing.", + }), + }; + expect(classifyLoginError(new ApiError(403, body, "HTTP 403"))).toEqual({ + kind: "passwordChangeRequired", + }); + }); + + it("falls back to failed for an unrelated 403", () => { + const body = { error: "login_failed", detail: JSON.stringify({ detail: "Forbidden" }) }; + expect(classifyLoginError(new ApiError(403, body, "HTTP 403"))).toEqual({ + kind: "failed", + status: 403, + }); + }); + + it("does not throw on non-JSON detail (e.g. a plain-text 429 body)", () => { + const body = { error: "login_failed", detail: "Too many requests" }; + expect(classifyLoginError(new ApiError(429, body, "HTTP 429"))).toEqual({ + kind: "failed", + status: 429, + }); + }); + + it("falls back to failed for a malformed/missing body or non-ApiError", () => { + expect(classifyLoginError(new ApiError(403, null, "HTTP 403"))).toEqual({ + kind: "failed", + status: 403, + }); + expect(classifyLoginError(new Error("network"))).toEqual({ kind: "failed", status: 0 }); + }); +}); diff --git a/src/api/loginErrors.ts b/src/api/loginErrors.ts new file mode 100644 index 0000000..59b7a0d --- /dev/null +++ b/src/api/loginErrors.ts @@ -0,0 +1,53 @@ +import { ApiError } from "./client"; + +export type LoginError = + | { kind: "invalidCredentials" } + | { kind: "passwordChangeRequired" } + | { kind: "failed"; status: number }; + +interface UpstreamLoginErrorBody { + error?: string; + detail?: string; +} + +const PASSWORD_CHANGE_REQUIRED_PATTERN = /password change required/i; + +/** + * server/src/routes/auth/login.ts forwards non-2xx upstream responses as + * { error: "login_failed", detail: }. When upstream's + * body was JSON, `detail` here is itself a JSON-encoded string — parse + * defensively, since other failures (e.g. a bare 429 rate-limit string) may + * not be JSON at all. + */ +function extractUpstreamDetail(body: unknown): string | null { + if (!body || typeof body !== "object") return null; + const envelope = body as UpstreamLoginErrorBody; + if (typeof envelope.detail !== "string") return null; + + try { + const inner = JSON.parse(envelope.detail) as { detail?: string }; + if (typeof inner.detail === "string") return inner.detail; + } catch { + // Not JSON — upstream sent plain text. + } + return envelope.detail; +} + +/** + * Classify POST /auth/login failures. Scoped to the login endpoint only — + * do not merge with classifyPasswordResetError (./passwordResetErrors), which + * assigns a different meaning to 403 for the password-reset-request flow. + */ +export function classifyLoginError(error: unknown): LoginError { + if (!(error instanceof ApiError)) return { kind: "failed", status: 0 }; + if (error.status === 401) return { kind: "invalidCredentials" }; + + if (error.status === 403) { + const detail = extractUpstreamDetail(error.body); + if (detail && PASSWORD_CHANGE_REQUIRED_PATTERN.test(detail)) { + return { kind: "passwordChangeRequired" }; + } + } + + return { kind: "failed", status: error.status }; +} diff --git a/src/auth/AuthContext.test.tsx b/src/auth/AuthContext.test.tsx index 7c5a78c..8d93edd 100644 --- a/src/auth/AuthContext.test.tsx +++ b/src/auth/AuthContext.test.tsx @@ -50,6 +50,15 @@ function TestComponent() { > Login + ); @@ -341,6 +350,49 @@ describe("AuthContext", () => { expect(screen.queryByTestId("user-email")).not.toBeInTheDocument(); }); + it("handles a successful completePasswordChangeRequired identically to login", async () => { + vi.mocked(api.get).mockRejectedValueOnce(new ApiError(401, "Unauthorized", "")); + + render( + + + , + ); + + await waitFor(() => { + expect(screen.queryByTestId("loading")).not.toBeInTheDocument(); + }); + + const mockUser = { + email: "test@example.com", + full_name: "Test User", + is_admin: false, + is_active: true, + auth_provider: "local", + email_verified: true, + password_change_required: false, + }; + + vi.mocked(api.post).mockResolvedValueOnce({ + user: mockUser, + csrfToken: "test-csrf-token", + }); + + screen.getByText("CompletePasswordChangeRequired").click(); + + await waitFor(() => { + expect(screen.getByTestId("auth-status")).toHaveTextContent("authenticated"); + expect(screen.getByTestId("user-email")).toHaveTextContent("test@example.com"); + }); + + expect(api.post).toHaveBeenCalledWith( + "/auth/change-password-required", + { email: "test@example.com", oldPassword: "old-pass", newPassword: "new-pass" }, + { authenticated: false }, + ); + expect(setCsrfToken).toHaveBeenCalledWith("test-csrf-token"); + }); + it("handles successful logout", async () => { const mockUser = { email: "user@example.com", diff --git a/src/auth/AuthContext.tsx b/src/auth/AuthContext.tsx index 35b9479..3fdecda 100644 --- a/src/auth/AuthContext.tsx +++ b/src/auth/AuthContext.tsx @@ -38,6 +38,17 @@ interface AuthState { interface AuthContextValue extends AuthState { login: (email: string, password: string) => Promise; // pragma: allowlist secret + /** + * Completes the "password change required" flow: BFF re-authenticates with + * the old password, changes it, then logs in again with the new one and + * establishes a real session — same response shape as login(), so this + * updates auth state identically. See PasswordChangeRequired.tsx. + */ + completePasswordChangeRequired: ( + email: string, + oldPassword: string, // pragma: allowlist secret + newPassword: string, // pragma: allowlist secret + ) => Promise; logout: () => Promise; setSelectedTeamId: (teamId: string | null) => void; /** Caller's effective permissions from GET /rbac/my/permissions (in-memory only). */ @@ -153,6 +164,25 @@ export function AuthProvider({ children }: { children: ReactNode }) { [], ); + const completePasswordChangeRequired = useCallback( + async ( + email: string, + oldPassword: string, // pragma: allowlist secret + newPassword: string, // pragma: allowlist secret + ): Promise => { + const data = await api.post( + "/auth/change-password-required", + { email, oldPassword, newPassword }, + { authenticated: false }, + ); + + setCsrfToken(data.csrfToken); + authVersion.current += 1; + setState({ user: data.user, isAuthenticated: true, isLoading: false, selectedTeamId: null }); + }, + [], + ); + const logout = useCallback(async (): Promise => { try { // Empty body would still send Content-Type: application/json, which Fastify's @@ -210,6 +240,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { value={{ ...state, login, + completePasswordChangeRequired, logout, setSelectedTeamId, permissions: perms.permissions, diff --git a/src/components/auth/AuthCard.tsx b/src/components/auth/AuthCard.tsx new file mode 100644 index 0000000..4414911 --- /dev/null +++ b/src/components/auth/AuthCard.tsx @@ -0,0 +1,30 @@ +import type { ReactNode } from "react"; + +/** + * Shared
/
chrome for the auth pages (ForgotPassword, + * ResetPassword, PasswordChangeRequired). `compact` switches to the + * narrower/rounder success-panel sizing used once a page's success state is + * entered. + */ +export function AuthCard({ + titleId, + compact = false, + children, +}: { + titleId: string; + compact?: boolean; + children: ReactNode; +}) { + return ( +
+
+ {children} +
+
+ ); +} diff --git a/src/components/auth/AuthSuccessPanel.tsx b/src/components/auth/AuthSuccessPanel.tsx new file mode 100644 index 0000000..e0b974a --- /dev/null +++ b/src/components/auth/AuthSuccessPanel.tsx @@ -0,0 +1,53 @@ +import { useEffect, useRef } from "react"; +import { CircleCheck } from "lucide-react"; +import { Button } from "@/components/ui/button"; + +/** + * Shared success state for ResetPassword / PasswordChangeRequired: a + * CircleCheck heading (focus-managed for screen-reader/keyboard users), + * body copy, and a single CTA. Not used by ForgotPassword, which shows its + * success state as an InlineNotification instead of this pattern. + */ +export function AuthSuccessPanel({ + titleId, + title, + body, + ctaLabel, + onCta, +}: { + titleId: string; + title: string; + body: string; + ctaLabel: string; + onCta: () => void; +}) { + const headingRef = useRef(null); + useEffect(() => { + // This component only mounts once the success state is entered, so + // focusing on mount is equivalent to the old useEffect([succeeded]). + headingRef.current?.focus(); + }, []); + + return ( +
+
+

+

+

{body}

+
+ +
+ ); +} diff --git a/src/components/layout/HeaderQuickNav.test.tsx b/src/components/layout/HeaderQuickNav.test.tsx index 2a9ef75..6798a1a 100644 --- a/src/components/layout/HeaderQuickNav.test.tsx +++ b/src/components/layout/HeaderQuickNav.test.tsx @@ -49,6 +49,7 @@ const defaultAuthContext = { isLoading: false, selectedTeamId: null, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], diff --git a/src/components/mcp-servers/AdvancedSettings.test.tsx b/src/components/mcp-servers/AdvancedSettings.test.tsx index 6cdec5b..bfaddee 100644 --- a/src/components/mcp-servers/AdvancedSettings.test.tsx +++ b/src/components/mcp-servers/AdvancedSettings.test.tsx @@ -19,6 +19,7 @@ const makeAuthContext = (selectedTeamId: string | null = null) => isAuthenticated: false, isLoading: false, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], diff --git a/src/components/prompts/PromptForm.test.tsx b/src/components/prompts/PromptForm.test.tsx index 5a59abc..447dc20 100644 --- a/src/components/prompts/PromptForm.test.tsx +++ b/src/components/prompts/PromptForm.test.tsx @@ -75,6 +75,7 @@ describe("PromptForm", () => { isAuthenticated: true, isLoading: false, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], @@ -117,6 +118,7 @@ describe("PromptForm", () => { isAuthenticated: true, isLoading: false, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], @@ -200,6 +202,7 @@ describe("PromptForm", () => { isAuthenticated: true, isLoading: false, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], diff --git a/src/components/users/PasswordInput.tsx b/src/components/users/PasswordInput.tsx index f9091ae..100d631 100644 --- a/src/components/users/PasswordInput.tsx +++ b/src/components/users/PasswordInput.tsx @@ -17,77 +17,83 @@ interface PasswordInputProps { hint?: string; } -export function PasswordInput({ - id, - value, - onChange, - placeholder, - label, - required = false, - autoComplete = "new-password", - error, - hint, -}: PasswordInputProps) { - const intl = useIntl(); - const [showPassword, setShowPassword] = React.useState(false); - const errorId = `${id}-error`; - const hintId = `${id}-hint`; +export const PasswordInput = React.forwardRef( + function PasswordInput( + { + id, + value, + onChange, + placeholder, + label, + required = false, + autoComplete = "new-password", + error, + hint, + }, + ref, + ) { + const intl = useIntl(); + const [showPassword, setShowPassword] = React.useState(false); + const errorId = `${id}-error`; + const hintId = `${id}-hint`; - return ( -
- -
- onChange(event.target.value)} - placeholder={placeholder} - className="h-10 border-neutral-300 pr-10 shadow-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-0 dark:border-neutral-700" - aria-invalid={!!error} - aria-describedby={error ? errorId : hint ? hintId : undefined} - /> - + +
+ onChange(event.target.value)} + placeholder={placeholder} + className="h-10 border-neutral-300 pr-10 shadow-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-0 dark:border-neutral-700" + aria-invalid={!!error} + aria-describedby={error ? errorId : hint ? hintId : undefined} + /> + +
+ {error ? ( + + ) : hint ? ( +

+ {hint} +

+ ) : null}
- {error ? ( - - ) : hint ? ( -

- {hint} -

- ) : null} -
- ); -} + ); + }, +); diff --git a/src/hooks/usePromptForm.test.ts b/src/hooks/usePromptForm.test.ts index da30dcc..e1b729c 100644 --- a/src/hooks/usePromptForm.test.ts +++ b/src/hooks/usePromptForm.test.ts @@ -42,6 +42,7 @@ function mockAuth(selectedTeamId: string | null = null) { isAuthenticated: true, isLoading: false, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], diff --git a/src/i18n/locales/en-US/auth.json b/src/i18n/locales/en-US/auth.json index df243a1..cfcd8a1 100644 --- a/src/i18n/locales/en-US/auth.json +++ b/src/i18n/locales/en-US/auth.json @@ -40,6 +40,25 @@ "auth.changePassword.newPassword": "New Password", "auth.changePassword.confirmPassword": "Confirm New Password", "auth.changePassword.submit": "Change Password", + "auth.passwordChangeRequired.title": "Password Change Required", + "auth.passwordChangeRequired.description": "Your password needs to be changed before you can sign in. Enter your current password and choose a new one.", + "auth.passwordChangeRequired.email": "Email Address", + "auth.passwordChangeRequired.oldPassword": "Current Password", + "auth.passwordChangeRequired.newPassword": "New Password", + "auth.passwordChangeRequired.confirmPassword": "Confirm New Password", + "auth.passwordChangeRequired.passwordHint": "Use at least 12 characters and 3 of: uppercase, lowercase, number, or special character. Privileged accounts may require more.", + "auth.passwordChangeRequired.submit": "Change Password", + "auth.passwordChangeRequired.submitting": "Changing password…", + "auth.passwordChangeRequired.successTitle": "Password changed", + "auth.passwordChangeRequired.success": "Your password was changed for ContextForge, but we couldn't sign you in automatically. Please log in with your new password.", + "auth.passwordChangeRequired.returnToLogin": "Return to login", + "auth.passwordChangeRequired.forgotPasswordFallback": "Forgot your password instead?", + "auth.passwordChangeRequired.error.oldPasswordRequired": "Enter your current password.", + "auth.passwordChangeRequired.error.tooShort": "Password must be at least 12 characters.", + "auth.passwordChangeRequired.error.complexity": "Password must contain at least 3 of: uppercase, lowercase, number, or special character.", + "auth.passwordChangeRequired.error.mismatch": "Passwords do not match.", + "auth.passwordChangeRequired.error.invalidOldPassword": "Your current password is incorrect.", + "auth.passwordChangeRequired.error.failed": "We could not change your password. Please try again.", "auth.logout": "Sign Out", "auth.login.error.invalidCredentials": "Invalid credentials.", "auth.login.error.failed": "Login failed ({status}).", diff --git a/src/i18n/locales/es-ES/auth.json b/src/i18n/locales/es-ES/auth.json index b63afe5..3242f74 100644 --- a/src/i18n/locales/es-ES/auth.json +++ b/src/i18n/locales/es-ES/auth.json @@ -40,6 +40,25 @@ "auth.changePassword.newPassword": "Nueva Contraseña", "auth.changePassword.confirmPassword": "Confirmar Nueva Contraseña", "auth.changePassword.submit": "Cambiar Contraseña", + "auth.passwordChangeRequired.title": "Cambio de Contraseña Requerido", + "auth.passwordChangeRequired.description": "Debes cambiar tu contraseña antes de poder iniciar sesión. Ingresa tu contraseña actual y elige una nueva.", + "auth.passwordChangeRequired.email": "Dirección de Correo Electrónico", + "auth.passwordChangeRequired.oldPassword": "Contraseña Actual", + "auth.passwordChangeRequired.newPassword": "Nueva Contraseña", + "auth.passwordChangeRequired.confirmPassword": "Confirmar Nueva Contraseña", + "auth.passwordChangeRequired.passwordHint": "Usa al menos 12 caracteres y 3 de estos tipos: mayúscula, minúscula, número o carácter especial. Las cuentas privilegiadas pueden requerir más.", + "auth.passwordChangeRequired.submit": "Cambiar Contraseña", + "auth.passwordChangeRequired.submitting": "Cambiando contraseña…", + "auth.passwordChangeRequired.successTitle": "Contraseña cambiada", + "auth.passwordChangeRequired.success": "Tu contraseña de ContextForge fue cambiada, pero no pudimos iniciar sesión automáticamente. Inicia sesión con tu nueva contraseña.", + "auth.passwordChangeRequired.returnToLogin": "Volver al inicio de sesión", + "auth.passwordChangeRequired.forgotPasswordFallback": "¿Olvidaste tu contraseña?", + "auth.passwordChangeRequired.error.oldPasswordRequired": "Ingresa tu contraseña actual.", + "auth.passwordChangeRequired.error.tooShort": "La contraseña debe tener al menos 12 caracteres.", + "auth.passwordChangeRequired.error.complexity": "La contraseña debe contener al menos 3 de estos tipos: mayúscula, minúscula, número o carácter especial.", + "auth.passwordChangeRequired.error.mismatch": "Las contraseñas no coinciden.", + "auth.passwordChangeRequired.error.invalidOldPassword": "Tu contraseña actual es incorrecta.", + "auth.passwordChangeRequired.error.failed": "No pudimos cambiar tu contraseña. Inténtalo de nuevo.", "auth.logout": "Cerrar Sesión", "auth.login.error.invalidCredentials": "Credenciales inválidas.", "auth.login.error.failed": "Error al iniciar sesión ({status}).", diff --git a/src/i18n/locales/pt-BR/auth.json b/src/i18n/locales/pt-BR/auth.json index 55dd4a8..fe5cd07 100644 --- a/src/i18n/locales/pt-BR/auth.json +++ b/src/i18n/locales/pt-BR/auth.json @@ -40,6 +40,25 @@ "auth.changePassword.newPassword": "Nova Senha", "auth.changePassword.confirmPassword": "Confirmar Nova Senha", "auth.changePassword.submit": "Alterar Senha", + "auth.passwordChangeRequired.title": "Alteração de Senha Necessária", + "auth.passwordChangeRequired.description": "Sua senha precisa ser alterada antes que você possa entrar. Digite sua senha atual e escolha uma nova.", + "auth.passwordChangeRequired.email": "Endereço de E-mail", + "auth.passwordChangeRequired.oldPassword": "Senha Atual", + "auth.passwordChangeRequired.newPassword": "Nova Senha", + "auth.passwordChangeRequired.confirmPassword": "Confirmar Nova Senha", + "auth.passwordChangeRequired.passwordHint": "Use pelo menos 12 caracteres e 3 destes tipos: maiúscula, minúscula, número ou caractere especial. Contas privilegiadas podem exigir mais.", + "auth.passwordChangeRequired.submit": "Alterar Senha", + "auth.passwordChangeRequired.submitting": "Alterando senha…", + "auth.passwordChangeRequired.successTitle": "Senha alterada", + "auth.passwordChangeRequired.success": "Sua senha do ContextForge foi alterada, mas não conseguimos fazer login automaticamente. Faça login com sua nova senha.", + "auth.passwordChangeRequired.returnToLogin": "Voltar ao login", + "auth.passwordChangeRequired.forgotPasswordFallback": "Esqueceu a senha?", + "auth.passwordChangeRequired.error.oldPasswordRequired": "Digite sua senha atual.", + "auth.passwordChangeRequired.error.tooShort": "A senha deve ter pelo menos 12 caracteres.", + "auth.passwordChangeRequired.error.complexity": "A senha deve conter pelo menos 3 destes tipos: maiúscula, minúscula, número ou caractere especial.", + "auth.passwordChangeRequired.error.mismatch": "As senhas não coincidem.", + "auth.passwordChangeRequired.error.invalidOldPassword": "Sua senha atual está incorreta.", + "auth.passwordChangeRequired.error.failed": "Não foi possível alterar sua senha. Tente novamente.", "auth.logout": "Sair", "auth.login.error.invalidCredentials": "Credenciais inválidas.", "auth.login.error.failed": "Falha ao entrar ({status}).", diff --git a/src/lib/passwordPolicy.ts b/src/lib/passwordPolicy.ts new file mode 100644 index 0000000..b0558a7 --- /dev/null +++ b/src/lib/passwordPolicy.ts @@ -0,0 +1,9 @@ +// Shared 3-of-4-character-class password complexity check, used by +// ResetPassword.tsx and PasswordChangeRequired.tsx so the policy can't drift +// between the two forms. +const CHARACTER_CLASS_PATTERNS = [/[a-z]/, /[A-Z]/, /\d/, /[^A-Za-z0-9]/]; +export const MIN_PASSWORD_CHARACTER_CLASSES = 3; + +export function countPasswordCharacterClasses(password: string): number { + return CHARACTER_CLASS_PATTERNS.filter((pattern) => pattern.test(password)).length; +} diff --git a/src/pages/ForgotPassword.tsx b/src/pages/ForgotPassword.tsx index c7553e3..3e05b45 100644 --- a/src/pages/ForgotPassword.tsx +++ b/src/pages/ForgotPassword.tsx @@ -2,6 +2,7 @@ import { useState } from "react"; import { useIntl } from "react-intl"; import { requestPasswordReset } from "@/api/passwordReset"; import { classifyPasswordResetError } from "@/api/passwordResetErrors"; +import { AuthCard } from "@/components/auth/AuthCard"; import { Button } from "@/components/ui/button"; import { InlineNotification } from "@/components/ui/inline-notification"; import { Input } from "@/components/ui/input"; @@ -47,79 +48,72 @@ export function ForgotPassword() { } return ( -
-
+

-

- {intl.formatMessage({ id: "auth.forgotPassword.title" })} -

-

- {intl.formatMessage({ id: "auth.forgotPassword.description" })} -

+ {intl.formatMessage({ id: "auth.forgotPassword.title" })} + +

+ {intl.formatMessage({ id: "auth.forgotPassword.description" })} +

- {submitted ? ( -
- + + +
+ ) : ( +
+
+ + { + setEmail(event.target.value); + setEmailError(null); + }} + aria-invalid={!!emailError} + aria-describedby={emailError ? "forgot-password-email-error" : undefined} /> - + {emailError && ( + + )}
- ) : ( - -
- - { - setEmail(event.target.value); - setEmailError(null); - }} - aria-invalid={!!emailError} - aria-describedby={emailError ? "forgot-password-email-error" : undefined} - /> - {emailError && ( - - )} -
- {error && } - - - - )} -
-
+ {error && } + + + + )} + ); } diff --git a/src/pages/Login.test.tsx b/src/pages/Login.test.tsx index 5962984..119a553 100644 --- a/src/pages/Login.test.tsx +++ b/src/pages/Login.test.tsx @@ -97,6 +97,56 @@ describe("Login", () => { }); }); + it("redirects to the password-change-required page on a 403 password-change-required response", async () => { + const body = { + error: "login_failed", + detail: JSON.stringify({ + detail: "Password change required. Please change your password before continuing.", + }), + }; + const error = new ApiError(403, body, "HTTP 403"); + + vi.mocked(useAuth).mockReturnValue({ + isAuthenticated: false, + login: mockLogin.mockRejectedValue(error), + } as unknown as ReturnType); + + renderWithI18n(); + + fireEvent.change(screen.getByLabelText(/Email/i), { target: { value: "test@example.com" } }); + fireEvent.change(screen.getByLabelText(/Password/i), { target: { value: "oldpass" } }); + fireEvent.submit(screen.getByRole("button", { name: /Sign in/i })); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith( + "/app/change-password-required?email=test%40example.com", + ); + }); + }); + + it("displays the generic failed error for an unrelated 403", async () => { + const body = { error: "login_failed", detail: JSON.stringify({ detail: "Forbidden" }) }; + const error = new ApiError(403, body, "HTTP 403"); + + vi.mocked(useAuth).mockReturnValue({ + isAuthenticated: false, + login: mockLogin.mockRejectedValue(error), + } as unknown as ReturnType); + + renderWithI18n(); + + fireEvent.change(screen.getByLabelText(/Email/i), { target: { value: "test@example.com" } }); + fireEvent.change(screen.getByLabelText(/Password/i), { target: { value: "pass" } }); + fireEvent.submit(screen.getByRole("button", { name: /Sign in/i })); + + await waitFor(() => { + expect(screen.getByRole("alert")).toHaveTextContent("Login failed (403)."); + }); + expect(mockNavigate).not.toHaveBeenCalledWith( + expect.stringContaining("change-password-required"), + ); + }); + it("displays generic failed error on non-401 ApiError", async () => { const error = new ApiError(500, "ApiError", "") as ApiError & { status?: number }; error.status = 500; diff --git a/src/pages/Login.tsx b/src/pages/Login.tsx index 70102ef..4a96cdb 100644 --- a/src/pages/Login.tsx +++ b/src/pages/Login.tsx @@ -3,6 +3,7 @@ import { useIntl } from "react-intl"; import { useAuth } from "../auth/useAuth"; import { useRouter, resolveNextParam } from "../router"; import { ApiError } from "../api/client"; +import { classifyLoginError } from "../api/loginErrors"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; @@ -31,12 +32,15 @@ export function Login() { await login(email, password); navigate(returnTo); } catch (err) { - if (err instanceof ApiError) { - setError( - err.status === 401 - ? intl.formatMessage({ id: "auth.login.error.invalidCredentials" }) - : intl.formatMessage({ id: "auth.login.error.failed" }, { status: err.status }), - ); + const loginError = classifyLoginError(err); + if (loginError.kind === "passwordChangeRequired") { + navigate(`/app/change-password-required?email=${encodeURIComponent(email)}`); + return; + } + if (loginError.kind === "invalidCredentials") { + setError(intl.formatMessage({ id: "auth.login.error.invalidCredentials" })); + } else if (err instanceof ApiError) { + setError(intl.formatMessage({ id: "auth.login.error.failed" }, { status: err.status })); } else { setError(intl.formatMessage({ id: "auth.login.error.unexpected" })); } diff --git a/src/pages/PasswordChangeRequired.test.tsx b/src/pages/PasswordChangeRequired.test.tsx new file mode 100644 index 0000000..0d12f6a --- /dev/null +++ b/src/pages/PasswordChangeRequired.test.tsx @@ -0,0 +1,145 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ApiError } from "@/api/client"; +import { I18nProvider } from "@/i18n"; +import { useAuth } from "@/auth/useAuth"; +import { useRouter, resolveNextParam } from "@/router"; +import { PasswordChangeRequired } from "./PasswordChangeRequired"; + +vi.mock("@/auth/useAuth", () => ({ useAuth: vi.fn() })); +vi.mock("@/router", () => ({ useRouter: vi.fn(), resolveNextParam: vi.fn() })); + +describe("PasswordChangeRequired", () => { + const navigate = vi.fn(); + const completePasswordChangeRequired = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(useRouter).mockReturnValue({ navigate } as unknown as ReturnType); + vi.mocked(resolveNextParam).mockReturnValue("/app/"); + vi.mocked(useAuth).mockReturnValue({ + isAuthenticated: false, + completePasswordChangeRequired, + } as unknown as ReturnType); + window.history.pushState({}, "", "/app/change-password-required?email=user%40example.com"); + }); + + function renderPage() { + return render( + + + , + ); + } + + async function fillPasswords( + oldPassword: string, + newPassword: string, + confirmation = newPassword, + ) { + await waitFor(() => expect(document.getElementById("old-password")).toBeInTheDocument()); + fireEvent.change(document.getElementById("old-password")!, { target: { value: oldPassword } }); + fireEvent.change(document.getElementById("new-password")!, { target: { value: newPassword } }); + fireEvent.change(document.getElementById("confirm-password")!, { + target: { value: confirmation }, + }); + } + + it("pre-fills the read-only email from the query param", () => { + renderPage(); + expect(screen.getByLabelText("Email Address")).toHaveValue("user@example.com"); + expect(screen.getByLabelText("Email Address")).toBeDisabled(); + }); + + it("redirects into the app if already authenticated", () => { + vi.mocked(useAuth).mockReturnValue({ + isAuthenticated: true, + completePasswordChangeRequired, + } as unknown as ReturnType); + + renderPage(); + expect(navigate).toHaveBeenCalledWith("/app/"); + }); + + it("rejects mismatched new passwords without an API request", async () => { + renderPage(); + await fillPasswords("old-pass", "New-password1", "New-password2"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + expect(await screen.findByText("Passwords do not match.")).toBeInTheDocument(); + expect(completePasswordChangeRequired).not.toHaveBeenCalled(); + }); + + it("enforces minimum length and complexity before submission", async () => { + renderPage(); + await fillPasswords("old-pass", "short-A1!"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + expect(await screen.findByText("Password must be at least 12 characters.")).toBeInTheDocument(); + + await fillPasswords("old-pass", "alllowercasepassword"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + expect( + await screen.findByText( + "Password must contain at least 3 of: uppercase, lowercase, number, or special character.", + ), + ).toBeInTheDocument(); + expect(completePasswordChangeRequired).not.toHaveBeenCalled(); + }); + + it("on success, calls AuthContext and navigates straight into the app (no static success screen)", async () => { + completePasswordChangeRequired.mockResolvedValue(undefined); + renderPage(); + await fillPasswords("old-pass", "New-password1"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + await waitFor(() => { + expect(completePasswordChangeRequired).toHaveBeenCalledWith( + "user@example.com", + "old-pass", + "New-password1", + ); + }); + // No local success screen is rendered — AuthContext's own state flip + // (isAuthenticated: true on next render) is what drives navigation. + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("shows an invalid-old-password message with a forgot-password fallback", async () => { + completePasswordChangeRequired.mockRejectedValue(new ApiError(401, null, "HTTP 401")); + renderPage(); + await fillPasswords("wrong-old-pass", "New-password1"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + expect(await screen.findByRole("alert")).toHaveTextContent(/current password is incorrect/i); + const fallback = screen.getByRole("button", { name: /forgot your password/i }); + fireEvent.click(fallback); + expect(navigate).toHaveBeenCalledWith("/app/forgot-password"); + }); + + it("surfaces a backend policy-violation message on the new password field", async () => { + completePasswordChangeRequired.mockRejectedValue( + new ApiError(400, { detail: "Password must not be a commonly used password" }, "HTTP 400"), + ); + renderPage(); + await fillPasswords("old-pass", "New-password1"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + expect( + await screen.findByText("Password must not be a commonly used password"), + ).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /forgot your password/i })).not.toBeInTheDocument(); + }); + + it("shows the fallback success screen when the password changed but auto sign-in failed", async () => { + completePasswordChangeRequired.mockRejectedValue( + new ApiError(502, { error: "login_after_change_failed" }, "HTTP 502"), + ); + renderPage(); + await fillPasswords("old-pass", "New-password1"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + expect(await screen.findByRole("status")).toHaveTextContent("Password changed"); + fireEvent.click(screen.getByRole("button", { name: "Return to login" })); + expect(navigate).toHaveBeenCalledWith("/app/login"); + }); +}); diff --git a/src/pages/PasswordChangeRequired.tsx b/src/pages/PasswordChangeRequired.tsx new file mode 100644 index 0000000..642f492 --- /dev/null +++ b/src/pages/PasswordChangeRequired.tsx @@ -0,0 +1,250 @@ +import { useEffect, useMemo, useRef, useState } from "react"; +import { useIntl } from "react-intl"; +import { useAuth } from "@/auth/useAuth"; +import { classifyChangePasswordRequiredError } from "@/api/changePasswordRequiredErrors"; +import { AuthCard } from "@/components/auth/AuthCard"; +import { AuthSuccessPanel } from "@/components/auth/AuthSuccessPanel"; +import { PasswordInput } from "@/components/users/PasswordInput"; +import { Button } from "@/components/ui/button"; +import { InlineNotification } from "@/components/ui/inline-notification"; +import { Input } from "@/components/ui/input"; +import { Label } from "@/components/ui/label"; +import { VALIDATION } from "@/lib/constants"; +import { + countPasswordCharacterClasses, + MIN_PASSWORD_CHARACTER_CLASSES, +} from "@/lib/passwordPolicy"; +import { useRouter, resolveNextParam } from "@/router"; + +/** + * Public, pre-auth screen the user lands on after Login.tsx detects a + * "password change required" 403 (see classifyLoginError). Distinct from the + * private, authenticated self-service /app/change-password stub — this page + * re-authenticates with the old password, changes it, then logs in again + * with the new password via AuthContext.completePasswordChangeRequired, + * which lands the user in a real session exactly like a normal login. + */ +export function PasswordChangeRequired() { + const intl = useIntl(); + const { isAuthenticated, completePasswordChangeRequired } = useAuth(); + const { navigate } = useRouter(); + const returnTo = resolveNextParam(window.location.search); + const email = useMemo(() => new URLSearchParams(window.location.search).get("email") ?? "", []); + const [oldPassword, setOldPassword] = useState(""); + const [newPassword, setNewPassword] = useState(""); + const [confirmPassword, setConfirmPassword] = useState(""); + const [oldPasswordError, setOldPasswordError] = useState(null); + const [newPasswordError, setNewPasswordError] = useState(null); + const [confirmPasswordError, setConfirmPasswordError] = useState(null); + const [submitError, setSubmitError] = useState(null); + const [showForgotPasswordFallback, setShowForgotPasswordFallback] = useState(false); + const [submitting, setSubmitting] = useState(false); + // Only used for the rare "password changed but couldn't auto sign-in" + // fallback (see classifyChangePasswordRequiredError's changedButLoginFailed) + // — the happy path navigates straight into the app instead of showing this. + const [changedButLoginFailed, setChangedButLoginFailed] = useState(false); + const submitErrorRef = useRef(null); + const oldPasswordInputRef = useRef(null); + const newPasswordInputRef = useRef(null); + const confirmPasswordInputRef = useRef(null); + + useEffect(() => { + if (isAuthenticated) { + navigate(returnTo); + } + }, [isAuthenticated, navigate, returnTo]); + + useEffect(() => { + if (submitError) { + submitErrorRef.current?.focus(); + } + }, [submitError]); + + useEffect(() => { + if (oldPasswordError) { + oldPasswordInputRef.current?.focus(); + } + }, [oldPasswordError]); + + useEffect(() => { + if (newPasswordError) { + newPasswordInputRef.current?.focus(); + } + }, [newPasswordError]); + + useEffect(() => { + if (confirmPasswordError) { + confirmPasswordInputRef.current?.focus(); + } + }, [confirmPasswordError]); + + async function handleSubmit(event: React.FormEvent) { + event.preventDefault(); + setOldPasswordError(null); + setNewPasswordError(null); + setConfirmPasswordError(null); + setSubmitError(null); + setShowForgotPasswordFallback(false); + + if (!oldPassword) { + setOldPasswordError( + intl.formatMessage({ id: "auth.passwordChangeRequired.error.oldPasswordRequired" }), + ); + return; + } + if (newPassword.length < VALIDATION.MIN_PASSWORD_LENGTH) { + setNewPasswordError(intl.formatMessage({ id: "auth.passwordChangeRequired.error.tooShort" })); + return; + } + const characterTypes = countPasswordCharacterClasses(newPassword); + if (characterTypes < MIN_PASSWORD_CHARACTER_CLASSES) { + setNewPasswordError( + intl.formatMessage({ id: "auth.passwordChangeRequired.error.complexity" }), + ); + return; + } + if (newPassword !== confirmPassword) { + setConfirmPasswordError( + intl.formatMessage({ id: "auth.passwordChangeRequired.error.mismatch" }), + ); + return; + } + + setSubmitting(true); + try { + // On success, AuthContext's state update flips isAuthenticated, and the + // effect above navigates into the app — no local success state needed. + await completePasswordChangeRequired(email, oldPassword, newPassword); + setOldPassword(""); + setNewPassword(""); + setConfirmPassword(""); + } catch (err) { + const changeError = classifyChangePasswordRequiredError(err); + if (changeError.kind === "changedButLoginFailed") { + setOldPassword(""); + setNewPassword(""); + setConfirmPassword(""); + setChangedButLoginFailed(true); + } else if (changeError.kind === "invalidOldPassword") { + setSubmitError( + intl.formatMessage({ id: "auth.passwordChangeRequired.error.invalidOldPassword" }), + ); + setShowForgotPasswordFallback(true); + } else if (changeError.kind === "policyViolation") { + setNewPasswordError( + changeError.message ?? + intl.formatMessage({ id: "auth.passwordChangeRequired.error.failed" }), + ); + } else { + setSubmitError(intl.formatMessage({ id: "auth.passwordChangeRequired.error.failed" })); + } + } finally { + setSubmitting(false); + } + } + + return ( + + {changedButLoginFailed ? ( + navigate("/app/login")} + /> + ) : ( + <> +

+ {intl.formatMessage({ id: "auth.passwordChangeRequired.title" })} +

+

+ {intl.formatMessage({ id: "auth.passwordChangeRequired.description" })} +

+ +
+
+ + +
+ { + setOldPassword(value); + setOldPasswordError(null); + setSubmitError(null); + setShowForgotPasswordFallback(false); + }} + autoComplete="current-password" + label={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} + placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} + required + error={oldPasswordError ?? undefined} + /> + { + setNewPassword(value); + setNewPasswordError(null); + setConfirmPasswordError(null); + }} + label={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} + placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} + required + hint={intl.formatMessage({ id: "auth.passwordChangeRequired.passwordHint" })} + error={newPasswordError ?? undefined} + /> + { + setConfirmPassword(value); + setConfirmPasswordError(null); + }} + label={intl.formatMessage({ id: "auth.passwordChangeRequired.confirmPassword" })} + placeholder={intl.formatMessage({ + id: "auth.passwordChangeRequired.confirmPassword", + })} + required + error={confirmPasswordError ?? undefined} + /> + {submitError && ( +
+ +
+ )} + + {showForgotPasswordFallback && ( + + )} + + + )} +
+ ); +} diff --git a/src/pages/ResetPassword.tsx b/src/pages/ResetPassword.tsx index 9d6f76e..a3bc491 100644 --- a/src/pages/ResetPassword.tsx +++ b/src/pages/ResetPassword.tsx @@ -1,13 +1,18 @@ -import { useEffect, useRef, useState } from "react"; -import { CircleCheck } from "lucide-react"; +import { useEffect, useState } from "react"; import { useIntl } from "react-intl"; import { resetPassword, validatePasswordResetToken } from "@/api/passwordReset"; import { classifyPasswordResetError, type PasswordResetError } from "@/api/passwordResetErrors"; +import { AuthCard } from "@/components/auth/AuthCard"; +import { AuthSuccessPanel } from "@/components/auth/AuthSuccessPanel"; import { PasswordInput } from "@/components/users/PasswordInput"; import { Button } from "@/components/ui/button"; import { InlineNotification } from "@/components/ui/inline-notification"; import { Loading } from "@/components/ui/loading"; import { VALIDATION } from "@/lib/constants"; +import { + countPasswordCharacterClasses, + MIN_PASSWORD_CHARACTER_CLASSES, +} from "@/lib/passwordPolicy"; import { useRouter } from "@/router"; type TokenState = @@ -34,7 +39,6 @@ export function ResetPassword({ token = "" }: { token?: string }) { const [submitError, setSubmitError] = useState(null); const [submitting, setSubmitting] = useState(false); const [succeeded, setSucceeded] = useState(false); - const successHeadingRef = useRef(null); useEffect(() => { const controller = new AbortController(); @@ -54,10 +58,6 @@ export function ResetPassword({ token = "" }: { token?: string }) { return () => controller.abort(); }, [token]); - useEffect(() => { - if (succeeded) successHeadingRef.current?.focus(); - }, [succeeded]); - async function handleSubmit(event: React.FormEvent) { event.preventDefault(); setPasswordError(null); @@ -68,10 +68,8 @@ export function ResetPassword({ token = "" }: { token?: string }) { setPasswordError(intl.formatMessage({ id: "auth.resetPassword.error.tooShort" })); return; } - const characterTypes = [/[a-z]/, /[A-Z]/, /\d/, /[^A-Za-z0-9]/].filter((pattern) => - pattern.test(password), - ).length; - if (characterTypes < 3) { + const characterTypes = countPasswordCharacterClasses(password); + if (characterTypes < MIN_PASSWORD_CHARACTER_CLASSES) { setPasswordError(intl.formatMessage({ id: "auth.resetPassword.error.complexity" })); return; } @@ -124,119 +122,90 @@ export function ResetPassword({ token = "" }: { token?: string }) { const canRequestNewLink = tokenState === "invalid" || tokenState === "expired"; return ( -
-
- {succeeded ? ( -
-
-

+ {succeeded ? ( + navigate("/app/login")} + /> + ) : ( + <> +

+ {intl.formatMessage({ id: "auth.resetPassword.title" })} +

+

+ {intl.formatMessage({ id: "auth.resetPassword.description" })} +

+ + {tokenState === "validating" ? ( +
+ +
+ ) : tokenState !== "valid" ? ( +
+ +

-

- {intl.formatMessage({ id: "auth.resetPassword.success" })} -

+ {intl.formatMessage({ + id: canRequestNewLink + ? "auth.resetPassword.requestNewLink" + : "auth.resetPassword.returnToLogin", + })} +
- -
- ) : ( - <> -

- {intl.formatMessage({ id: "auth.resetPassword.title" })} -

-

- {intl.formatMessage({ id: "auth.resetPassword.description" })} -

- - {tokenState === "validating" ? ( -
- -
- ) : tokenState !== "valid" ? ( -
- - -
- ) : ( -
- { - setPassword(value); - setPasswordError(null); - setConfirmPasswordError(null); - setSubmitError(null); - }} - label={intl.formatMessage({ id: "auth.resetPassword.password" })} - placeholder={intl.formatMessage({ id: "auth.resetPassword.password" })} - required - hint={intl.formatMessage({ id: "auth.resetPassword.passwordHint" })} - error={passwordError ?? undefined} - /> - { - setConfirmPassword(value); - setConfirmPasswordError(null); - setSubmitError(null); - }} - label={intl.formatMessage({ id: "auth.resetPassword.confirmPassword" })} - placeholder={intl.formatMessage({ id: "auth.resetPassword.confirmPassword" })} - required - error={confirmPasswordError ?? undefined} - /> - {submitError && } - - - )} - - )} -
-
+ ) : ( +
+ { + setPassword(value); + setPasswordError(null); + setConfirmPasswordError(null); + setSubmitError(null); + }} + label={intl.formatMessage({ id: "auth.resetPassword.password" })} + placeholder={intl.formatMessage({ id: "auth.resetPassword.password" })} + required + hint={intl.formatMessage({ id: "auth.resetPassword.passwordHint" })} + error={passwordError ?? undefined} + /> + { + setConfirmPassword(value); + setConfirmPasswordError(null); + setSubmitError(null); + }} + label={intl.formatMessage({ id: "auth.resetPassword.confirmPassword" })} + placeholder={intl.formatMessage({ id: "auth.resetPassword.confirmPassword" })} + required + error={confirmPasswordError ?? undefined} + /> + {submitError && } + + + )} + + )} + ); } diff --git a/src/pages/Users.test.tsx b/src/pages/Users.test.tsx index b326aba..efe4866 100644 --- a/src/pages/Users.test.tsx +++ b/src/pages/Users.test.tsx @@ -90,6 +90,7 @@ function makeAuthContext(email = "admin@example.com") { isLoading: false, selectedTeamId: null, login: vi.fn(), + completePasswordChangeRequired: vi.fn(), logout: vi.fn(), setSelectedTeamId: vi.fn(), permissions: [], diff --git a/src/router/index.tsx b/src/router/index.tsx index b3280ea..38f65b6 100644 --- a/src/router/index.tsx +++ b/src/router/index.tsx @@ -241,7 +241,11 @@ export function Redirect({ to }: { to: string }) { // --------------------------------------------------------------------------- // Exact paths that are always public. -const DEFAULT_PUBLIC_PATHS: readonly string[] = ["/app/login", "/app/forgot-password"]; +const DEFAULT_PUBLIC_PATHS: readonly string[] = [ + "/app/login", + "/app/forgot-password", + "/app/change-password-required", +]; // Path prefixes whose subtrees are always public. const DEFAULT_PUBLIC_PREFIXES: readonly string[] = ["/app/reset-password/"]; From 25f646ffd3ee080ce3b9871b6b66802691da51cb Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Tue, 18 Aug 2026 16:54:36 +0100 Subject: [PATCH 2/4] PW requirements fix Signed-off-by: Gabriel Costa --- src/api/changePasswordRequiredErrors.test.ts | 16 +++++++++ src/api/changePasswordRequiredErrors.ts | 4 +-- src/api/loginErrors.ts | 29 ++------------- src/utils/errors.test.ts | 37 +++++++++++++++++++- src/utils/errors.ts | 28 +++++++++++++++ 5 files changed, 84 insertions(+), 30 deletions(-) diff --git a/src/api/changePasswordRequiredErrors.test.ts b/src/api/changePasswordRequiredErrors.test.ts index 3f22056..4fa2859 100644 --- a/src/api/changePasswordRequiredErrors.test.ts +++ b/src/api/changePasswordRequiredErrors.test.ts @@ -31,6 +31,22 @@ describe("classifyChangePasswordRequiredError", () => { }); }); + it("unwraps the double-JSON-encoded detail the BFF route actually forwards (raw upstream response text)", () => { + // server/src/routes/auth/change-password-required.ts sends + // { error: "change_password_failed", detail: await changeResponse.text() } — + // `detail` here is itself the raw JSON text of upstream's error body. + const body = { + error: "change_password_failed", + detail: JSON.stringify({ + detail: "Password must be at least 22 characters long (privileged account)", + }), + }; + expect(classifyChangePasswordRequiredError(new ApiError(400, body, "HTTP 400"))).toEqual({ + kind: "policyViolation", + message: "Password must be at least 22 characters long (privileged account)", + }); + }); + it("handles a 400 without detail", () => { expect(classifyChangePasswordRequiredError(new ApiError(400, null, "HTTP 400"))).toEqual({ kind: "policyViolation", diff --git a/src/api/changePasswordRequiredErrors.ts b/src/api/changePasswordRequiredErrors.ts index 097d852..992ec6a 100644 --- a/src/api/changePasswordRequiredErrors.ts +++ b/src/api/changePasswordRequiredErrors.ts @@ -1,5 +1,5 @@ import { ApiError } from "./client"; -import { extractApiErrorDetail } from "@/utils/errors"; +import { extractUpstreamApiErrorDetail } from "@/utils/errors"; export type ChangePasswordRequiredError = | { kind: "invalidOldPassword" } @@ -29,7 +29,7 @@ export function classifyChangePasswordRequiredError(error: unknown): ChangePassw if (error.status === 401 || error.status === 403) return { kind: "invalidOldPassword" }; if (error.status === 400 || error.status === 422) { - return { kind: "policyViolation", message: extractApiErrorDetail(error.body) }; + return { kind: "policyViolation", message: extractUpstreamApiErrorDetail(error.body) }; } return { kind: "failed" }; diff --git a/src/api/loginErrors.ts b/src/api/loginErrors.ts index 59b7a0d..34bc6b3 100644 --- a/src/api/loginErrors.ts +++ b/src/api/loginErrors.ts @@ -1,38 +1,13 @@ import { ApiError } from "./client"; +import { extractUpstreamApiErrorDetail } from "@/utils/errors"; export type LoginError = | { kind: "invalidCredentials" } | { kind: "passwordChangeRequired" } | { kind: "failed"; status: number }; -interface UpstreamLoginErrorBody { - error?: string; - detail?: string; -} - const PASSWORD_CHANGE_REQUIRED_PATTERN = /password change required/i; -/** - * server/src/routes/auth/login.ts forwards non-2xx upstream responses as - * { error: "login_failed", detail: }. When upstream's - * body was JSON, `detail` here is itself a JSON-encoded string — parse - * defensively, since other failures (e.g. a bare 429 rate-limit string) may - * not be JSON at all. - */ -function extractUpstreamDetail(body: unknown): string | null { - if (!body || typeof body !== "object") return null; - const envelope = body as UpstreamLoginErrorBody; - if (typeof envelope.detail !== "string") return null; - - try { - const inner = JSON.parse(envelope.detail) as { detail?: string }; - if (typeof inner.detail === "string") return inner.detail; - } catch { - // Not JSON — upstream sent plain text. - } - return envelope.detail; -} - /** * Classify POST /auth/login failures. Scoped to the login endpoint only — * do not merge with classifyPasswordResetError (./passwordResetErrors), which @@ -43,7 +18,7 @@ export function classifyLoginError(error: unknown): LoginError { if (error.status === 401) return { kind: "invalidCredentials" }; if (error.status === 403) { - const detail = extractUpstreamDetail(error.body); + const detail = extractUpstreamApiErrorDetail(error.body); if (detail && PASSWORD_CHANGE_REQUIRED_PATTERN.test(detail)) { return { kind: "passwordChangeRequired" }; } diff --git a/src/utils/errors.test.ts b/src/utils/errors.test.ts index e23bb8e..cba65f0 100644 --- a/src/utils/errors.test.ts +++ b/src/utils/errors.test.ts @@ -1,5 +1,10 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { extractApiErrorDetail, sanitizeError, withErrorHandling } from "./errors"; +import { + extractApiErrorDetail, + extractUpstreamApiErrorDetail, + sanitizeError, + withErrorHandling, +} from "./errors"; describe("extractApiErrorDetail", () => { it("returns null for null", () => { @@ -51,6 +56,36 @@ describe("extractApiErrorDetail", () => { }); }); +describe("extractUpstreamApiErrorDetail", () => { + it("unwraps a JSON-encoded detail string (BFF-owned auth routes forward raw upstream text)", () => { + const detail = JSON.stringify({ + detail: "Password must be at least 22 characters long (privileged account)", + }); + expect(extractUpstreamApiErrorDetail({ detail })).toBe( + "Password must be at least 22 characters long (privileged account)", + ); + }); + + it("unwraps a JSON-encoded FastAPI validation-error array", () => { + const detail = JSON.stringify({ detail: [{ msg: "too short" }] }); + expect(extractUpstreamApiErrorDetail({ detail })).toBe("too short"); + }); + + it("falls back to the raw string when detail is plain text, not JSON", () => { + expect(extractUpstreamApiErrorDetail({ detail: "Too many requests" })).toBe( + "Too many requests", + ); + }); + + it("returns null when detail is missing", () => { + expect(extractUpstreamApiErrorDetail({})).toBeNull(); + }); + + it("returns null for a non-object body", () => { + expect(extractUpstreamApiErrorDetail(null)).toBeNull(); + }); +}); + describe("sanitizeError", () => { beforeEach(() => { vi.spyOn(console, "error").mockImplementation(() => {}); diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 45bccfb..dedd14f 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -48,6 +48,34 @@ export function extractApiErrorDetail(body: unknown): string | null { return null; } +/** + * Extracts the real error message from a BFF-owned auth route's forwarded + * failure envelope. Routes like server/src/routes/auth/login.ts and + * change-password-required.ts forward a rejected upstream response as + * `{ error: , detail: }` — when + * upstream's body was JSON, `detail` here is itself a JSON-encoded string, so + * naively treating it as the message (extractApiErrorDetail's usual contract) + * renders the raw JSON text. Parse it defensively: other failures (e.g. a + * bare rate-limit string) may not be JSON at all, in which case `detail` IS + * already the message. + * + * @param body - The error body from a BFF-owned auth route's ApiError + * @returns Extracted error message or null if not found + */ +export function extractUpstreamApiErrorDetail(body: unknown): string | null { + if (!body || typeof body !== "object") return null; + const envelope = body as { detail?: string }; + if (typeof envelope.detail !== "string") return null; + + try { + const inner = extractApiErrorDetail(JSON.parse(envelope.detail)); + if (inner) return inner; + } catch { + // Not JSON — upstream sent plain text, `detail` is already the message. + } + return envelope.detail; +} + /** * Sanitizes error messages to prevent information leakage * From f43892e744f0fb9ee49172d4858b9f907ebccfb2 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Wed, 19 Aug 2026 10:04:33 +0100 Subject: [PATCH 3/4] Fix bugs + security patches Signed-off-by: Gabriel Costa --- e2e/auth/password-change-required.spec.ts | 38 +++ server/src/lib/establish-session.ts | 43 +++- server/src/lib/upstream-login.ts | 75 ++++++ .../routes/auth/change-password-required.ts | 229 +++++++----------- server/src/routes/auth/login.ts | 82 +++---- server/test/auth.test.ts | 156 +++++++++--- src/api/changePasswordRequiredErrors.test.ts | 13 + src/api/changePasswordRequiredErrors.ts | 19 +- src/auth/AuthContext.tsx | 41 ++-- src/i18n/locales/en-US/auth.json | 2 + src/i18n/locales/es-ES/auth.json | 2 + src/i18n/locales/pt-BR/auth.json | 2 + src/lib/passwordPolicy.test.ts | 27 +++ src/lib/passwordPolicy.ts | 8 +- src/pages/PasswordChangeRequired.test.tsx | 31 +++ src/pages/PasswordChangeRequired.tsx | 181 ++++++++------ 16 files changed, 624 insertions(+), 325 deletions(-) create mode 100644 server/src/lib/upstream-login.ts create mode 100644 src/lib/passwordPolicy.test.ts diff --git a/e2e/auth/password-change-required.spec.ts b/e2e/auth/password-change-required.spec.ts index c4990b7..f039f35 100644 --- a/e2e/auth/password-change-required.spec.ts +++ b/e2e/auth/password-change-required.spec.ts @@ -66,6 +66,44 @@ test.describe("Password change required flow", () => { await expect(page).toHaveURL(new RegExp(`${APP.FORGOT_PASSWORD}$`)); }); + test("account that doesn't need a change shows a distinct message and a return-to-login CTA", async ({ + page, + }) => { + // Raw route, not the apiMock fixture — this case has its own error code + // ({ error: "password_change_not_required" }), distinct from the + // fixture's generic change_password_failed body for a wrong old password. + await page.route("**/auth/change-password-required", async (route) => { + await route.fulfill({ + status: 403, + contentType: "application/json", + body: JSON.stringify({ error: "password_change_not_required" }), + }); + }); + + await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); + await page.getByLabel(/current password/i).fill("correct-password"); + await page.getByLabel(/^new password/i).fill("New-password1"); + await page.getByLabel(/confirm new password/i).fill("New-password1"); + await page.getByRole("button", { name: /change password/i }).click(); + + const alert = page.getByRole("alert"); + await expect(alert).toHaveText(/doesn't need to be changed/i); + await expect(page.getByRole("button", { name: /forgot your password/i })).toHaveCount(0); + await page.getByRole("button", { name: /return to login/i }).click(); + await expect(page).toHaveURL(new RegExp(`${APP.LOGIN}$`)); + }); + + test("missing ?email= shows an error and a return-to-login CTA instead of a submittable form", async ({ + page, + }) => { + await page.goto(APP.CHANGE_PASSWORD_REQUIRED); + + await expect(page.getByRole("alert")).toHaveText(/couldn't tell which account/i); + await expect(page.getByLabel(/current password/i)).toHaveCount(0); + await page.getByRole("button", { name: /return to login/i }).click(); + await expect(page).toHaveURL(new RegExp(`${APP.LOGIN}$`)); + }); + test("mismatched new passwords are rejected client-side", async ({ page }) => { await page.goto(`${APP.CHANGE_PASSWORD_REQUIRED}?email=test%40example.com`); await page.getByLabel(/current password/i).fill("old-password"); diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts index 2ce3201..d9af2cf 100644 --- a/server/src/lib/establish-session.ts +++ b/server/src/lib/establish-session.ts @@ -14,29 +14,56 @@ import { config } from "../config.js"; import { createSession, setSessionCookie, type SessionUser } from "./session-store.js"; import { CSRF_COOKIE_NAME } from "../plugins/csrf.js"; -// Mirrors mcpgateway.schemas.AuthenticationResponse. +// Mirrors mcpgateway.schemas.AuthenticationResponse. expires_in is optional — +// not every upstream login-shaped endpoint this BFF calls is guaranteed to +// send it (see lib/upstream-login.ts's Tier-1 /auth/login bypass call). export interface UpstreamAuthenticationResponse { access_token: string; - expires_in: number; + expires_in?: number; user: SessionUser; } +/** + * Thrown when the upstream auth response still reports + * password_change_required=true. This is the single chokepoint every + * session-establishing route goes through, so it's also the single place + * that guarantees a session is never minted for a still-flagged account — + * callers don't each have to re-implement that check correctly. In today's + * two callers this should never actually trip (routes/auth/login.ts only + * gets here after upstream's own 2xx says the account is clear; + * routes/auth/change-password-required.ts only gets here after a successful + * change), but it's the backstop for any future login path (SSO callback, + * admin impersonation, ...) that calls establishSession() directly. + */ +export class PasswordChangeStillRequiredError extends Error { + constructor() { + super("upstream auth response still has password_change_required=true"); + this.name = "PasswordChangeStillRequiredError"; + } +} + export async function establishSession( fastify: FastifyInstance, request: FastifyRequest, reply: FastifyReply, auth: UpstreamAuthenticationResponse, ): Promise<{ user: SessionUser; csrfToken: string }> { + if (auth.user?.password_change_required === true) { + throw new PasswordChangeStillRequiredError(); + } + // The BFF session/cookie must not outlive the bearer token it wraps — use // the upstream JWT's own lifetime, not a fixed BFF-side default. See // createSession's comment in lib/session-store.ts. let ttlSeconds = config.sessionTtlSeconds; - if (Number.isFinite(auth.expires_in) && auth.expires_in > 0) { - ttlSeconds = auth.expires_in; - } else { - // Upstream returned a bogus expires_in — fall back, but log it: this - // means the BFF session can outlive the JWT it wraps until the proxy's - // revoke-on-401 catches up (see session-store.ts). + if (Number.isFinite(auth.expires_in) && auth.expires_in! > 0) { + ttlSeconds = auth.expires_in!; + } else if (auth.expires_in !== undefined) { + // Upstream sent expires_in, but it's not a usable positive number — fall + // back, but log it: this means the BFF session can outlive the JWT it + // wraps until the proxy's revoke-on-401 catches up (see session-store.ts). + // A simply *absent* expires_in is not logged here — some upstream + // login-shaped endpoints don't send it by design (see upstream-login.ts). request.log.warn( { expires_in: auth.expires_in }, "upstream login returned invalid expires_in, using BFF default session TTL", diff --git a/server/src/lib/upstream-login.ts b/server/src/lib/upstream-login.ts new file mode 100644 index 0000000..c814f61 --- /dev/null +++ b/server/src/lib/upstream-login.ts @@ -0,0 +1,75 @@ +// Location: ./client/server/src/lib/upstream-login.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Shared "call an upstream login-shaped endpoint, validate the 2xx body" +// logic. Used by routes/auth/login.ts (Tier-2 /auth/email/login) and +// routes/auth/change-password-required.ts (both the Tier-2 precondition +// check and the Tier-1 /auth/login bypass, plus the post-change Tier-2 +// login) so the fetch/timeout/header/JSON-parse/access_token-check pattern +// lives in exactly one place instead of being copied per call site. + +import type { FastifyRequest } from "fastify"; + +import { config } from "../config.js"; +import type { UpstreamAuthenticationResponse } from "./establish-session.js"; + +// A hung (not refused) upstream must not hold the request open indefinitely +// — same rationale as revoke-upstream-token.ts's UPSTREAM_REVOKE_TIMEOUT_MS. +const UPSTREAM_LOGIN_TIMEOUT_MS = 3000; + +export type UpstreamLoginResult = + | { ok: true; auth: UpstreamAuthenticationResponse } + | { ok: false; kind: "unavailable" } + | { ok: false; kind: "rejected"; status: number; detail: string } + | { ok: false; kind: "invalid_response" }; + +/** + * POSTs { email, password } to an upstream login-shaped path and validates + * the 2xx body. `path` is the upstream path only (e.g. "/auth/login" or + * "/auth/email/login") — config.contextforgeUrl is prepended. + */ +export async function upstreamLogin( + request: FastifyRequest, + path: string, + email: string, + password: string, +): Promise { + let response: Response; + try { + response = await fetch(`${config.contextforgeUrl}${path}`, { + method: "POST", + headers: { + "content-type": "application/json", + // Preserve real client IP for upstream audit logging. + "x-forwarded-for": request.ip, + "x-real-ip": request.ip, + }, + body: JSON.stringify({ email, password }), + signal: AbortSignal.timeout(UPSTREAM_LOGIN_TIMEOUT_MS), + }); + } catch (err) { + request.log.error({ err, path }, "upstream login request failed"); + return { ok: false, kind: "unavailable" }; + } + + if (!response.ok) { + const detail = await response.text(); + return { ok: false, kind: "rejected", status: response.status, detail }; + } + + let auth: UpstreamAuthenticationResponse; // pragma: allowlist secret + try { + auth = (await response.json()) as UpstreamAuthenticationResponse; + } catch (err) { + request.log.error({ err, path }, "upstream login returned a non-JSON 2xx body"); + return { ok: false, kind: "invalid_response" }; + } + + if (typeof auth.access_token !== "string" || !auth.access_token) { + request.log.error({ auth, path }, "upstream login 2xx response missing access_token"); + return { ok: false, kind: "invalid_response" }; + } + + return { ok: true, auth }; +} diff --git a/server/src/routes/auth/change-password-required.ts b/server/src/routes/auth/change-password-required.ts index 1166ad6..f1b3ff6 100644 --- a/server/src/routes/auth/change-password-required.ts +++ b/server/src/routes/auth/change-password-required.ts @@ -4,26 +4,30 @@ // // POST /auth/change-password-required: browser -> BFF only, pre-login. Used // when /auth/login rejected valid credentials with a "password change -// required" 403 (see routes/auth/login.ts, which authenticates against -// upstream's /auth/email/login — that endpoint hard-blocks with 403 for the -// whole lifetime of the password_change_required flag, confirmed against a -// live backend). This route instead: +// required" 403 (see routes/auth/login.ts). This route: // -// A. authenticates against upstream's plain /auth/login ("Tier 1" session -// auth), which does NOT enforce that flag and still mints a token for a -// flagged account, using the OLD password; the response's -// user.password_change_required is then checked server-side — this is -// the ONLY gate before mutating state, so it must actually be true, or -// this route would just be an unauthenticated "change password with old -// password" endpoint for any account; -// B. uses that bypass token once to call the authenticated change-password -// endpoint; -// C. revokes the bypass token upstream (best-effort — it's served its one -// purpose and shouldn't linger); -// D. logs in again for real, via the same upstream /auth/email/login +// 1. Verifies the precondition via the SAME endpoint/logic that produced +// the original block: upstream's /auth/email/login, with the OLD +// password. Deliberately NOT a check against a persisted flag — upstream +// computes "needs password change" from several independent sources +// (a persisted flag, password-age expiry, default-password detection), +// and only /auth/email/login's own 403 reflects all of them. A 403 here +// also proves the old password is correct (upstream validates +// credentials before deciding whether to block). A 200 here means the +// account does NOT currently need a change — reject without ever +// minting a bypass token. +// 2. Mints a short-lived bypass token via upstream's plain /auth/login +// ("Tier 1" session auth), which does not enforce the block, using the +// same OLD password step 1 just validated. +// 3. Uses that bypass token once to call the authenticated change-password +// endpoint. +// 4. Revokes the bypass token upstream (best-effort, fire-and-forget — it's +// served its one purpose and shouldn't linger, but nothing downstream +// depends on the revoke actually completing). +// 5. Logs in again for real, via the same upstream /auth/email/login // login.ts uses, now with the NEW password (the block is gone once the -// password's been changed); -// E. establishes a normal BFF session from that — identical to a plain +// password's been changed). +// 6. Establishes a normal BFF session from that — identical to a plain // login, so a successful password change lands the user straight in // the app. // @@ -33,15 +37,12 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; -import { - establishSession, - type UpstreamAuthenticationResponse, -} from "../../lib/establish-session.js"; +import { establishSession, PasswordChangeStillRequiredError } from "../../lib/establish-session.js"; import { revokeUpstreamToken } from "../../lib/revoke-upstream-token.js"; import { setNoStore } from "../../lib/no-store.js"; import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; -import type { SessionUser } from "../../lib/session-store.js"; import { upstreamAuthHeader } from "../../lib/upstream-auth.js"; +import { upstreamLogin } from "../../lib/upstream-login.js"; interface ChangePasswordRequiredBody { email: string; @@ -49,33 +50,10 @@ interface ChangePasswordRequiredBody { newPassword: string; } -interface UpstreamBypassLoginResponse { - access_token: string; - user: SessionUser; -} - -// Caps each individual upstream leg — a hung (not refused) upstream must not -// hold the request open indefinitely, same rationale as -// revoke-upstream-token.ts's UPSTREAM_REVOKE_TIMEOUT_MS. +// Caps the change-password call itself — the two login-shaped calls already +// time out via upstreamLogin()'s own UPSTREAM_LOGIN_TIMEOUT_MS. const UPSTREAM_REQUEST_TIMEOUT_MS = 3000; -async function upstreamLogin( - request: FastifyRequest, - email: string, - password: string, -): Promise { - return fetch(`${config.contextforgeUrl}/auth/login`, { - method: "POST", - headers: { - "content-type": "application/json", - "x-forwarded-for": request.ip, - "x-real-ip": request.ip, - }, - body: JSON.stringify({ email, password }), - signal: AbortSignal.timeout(UPSTREAM_REQUEST_TIMEOUT_MS), - }); -} - export default async function changePasswordRequiredRoute(fastify: FastifyInstance): Promise { fastify.post<{ Body: ChangePasswordRequiredBody }>( "/auth/change-password-required", @@ -91,66 +69,63 @@ export default async function changePasswordRequiredRoute(fastify: FastifyInstan return reply.code(400).send({ error: "email, oldPassword and newPassword are required" }); } - // Step A: mint a bypass token using the old (still-valid) credentials. - // Deliberately /auth/login, not /auth/email/login — the latter - // hard-blocks while password_change_required is set, the former does - // not (verified against a live backend). - let bypassLoginResponse: Response; - try { - bypassLoginResponse = await upstreamLogin(request, email, oldPassword); - } catch (err) { - request.log.error({ err }, "upstream bypass login (change-password-required) failed"); - return reply.code(502).send({ error: "upstream_unavailable" }); + // Step 1: precondition + credential check, via the endpoint that + // actually computes "needs password change" (persisted flag, password + // age, default-password detection — see module comment above). + const precondition = await upstreamLogin(request, "/auth/email/login", email, oldPassword); + + if (precondition.ok) { + // Correct old password, but the account doesn't currently need a + // change (stale link, flag cleared elsewhere, etc.) — nothing to do, + // and no bypass token was ever minted. Revoke the token this + // legitimate login just handed us, since we're not using it. + void revokeUpstreamToken(request, precondition.auth.access_token); + return reply.code(403).send({ error: "password_change_not_required" }); } - if (!bypassLoginResponse.ok) { - // Covers both "still password-change-blocked" and "old password - // wrong" — don't try to disambiguate via text; the SPA falls back to - // the forgot-password flow on any failure here. - const detail = await bypassLoginResponse.text(); - return reply - .code(bypassLoginResponse.status) - .send({ error: "change_password_failed", detail }); + if (precondition.kind === "unavailable") { + return reply.code(502).send({ error: "upstream_unavailable" }); } - - let bypassAuth: UpstreamBypassLoginResponse; // pragma: allowlist secret - try { - bypassAuth = (await bypassLoginResponse.json()) as UpstreamBypassLoginResponse; - } catch (err) { - request.log.error({ err }, "upstream bypass login returned a non-JSON 2xx body"); + if (precondition.kind === "invalid_response") { return reply.code(502).send({ error: "upstream_invalid_response" }); } - - if (typeof bypassAuth.access_token !== "string" || !bypassAuth.access_token) { - request.log.error( - { bypassAuth }, - "upstream bypass login 2xx response missing access_token", - ); - return reply.code(502).send({ error: "upstream_invalid_response" }); + if (precondition.status !== 403) { + // 401 (wrong old password), 429 (rate-limited), etc. — pass through + // as-is; don't try to mint a bypass token for a credential the + // precondition check already told us is wrong or blocked. + return reply + .code(precondition.status) + .send({ error: "change_password_failed", detail: precondition.detail }); } - const bypassToken = bypassAuth.access_token; - - // Precondition: this route only exists to unblock accounts upstream - // has actually flagged. Without this check, a valid old password alone - // would rotate ANY account's password through this pre-auth endpoint. - if (bypassAuth.user?.password_change_required !== true) { - request.log.warn( - { email }, - "change-password-required called for an account that does not require a password change", - ); - // Bypass token served no purpose — revoke it before replying, same - // as the change-password-failed branch below. - await revokeUpstreamToken(request, bypassToken); - return reply.code(403).send({ error: "password_change_not_required" }); + + // Step 2: mint a bypass token using the same old credentials step 1 + // just validated. Deliberately /auth/login, not /auth/email/login — + // the latter hard-blocks while the account needs a password change, + // the former does not (verified against a live backend). + const bypass = await upstreamLogin(request, "/auth/login", email, oldPassword); + if (!bypass.ok) { + if (bypass.kind === "unavailable") { + return reply.code(502).send({ error: "upstream_unavailable" }); + } + if (bypass.kind === "invalid_response") { + return reply.code(502).send({ error: "upstream_invalid_response" }); + } + return reply + .code(bypass.status) + .send({ error: "change_password_failed", detail: bypass.detail }); } + const bypassToken = bypass.auth.access_token; - // Step B: use the bypass token once, immediately, to change the password. + // Step 3: use the bypass token once, immediately, to change the + // password. Same audit-IP headers as every other upstream call here. let changeResponse: Response; try { changeResponse = await fetch(`${config.contextforgeUrl}/auth/email/change-password`, { method: "POST", headers: { "content-type": "application/json", + "x-forwarded-for": request.ip, + "x-real-ip": request.ip, ...upstreamAuthHeader(bypassToken), }, body: JSON.stringify({ old_password: oldPassword, new_password: newPassword }), @@ -163,64 +138,48 @@ export default async function changePasswordRequiredRoute(fastify: FastifyInstan if (!changeResponse.ok) { // Password change itself failed (e.g. new-password policy violation) - // — the bypass token served no purpose, revoke it before replying. - await revokeUpstreamToken(request, bypassToken); + // — the bypass token served no purpose. Fire-and-forget revoke: the + // reply doesn't depend on it, and revokeUpstreamToken never throws. + void revokeUpstreamToken(request, bypassToken); const detail = await changeResponse.text(); return reply.code(changeResponse.status).send({ error: "change_password_failed", detail }); } - // Step C: the bypass token has done its one job — revoke it upstream - // rather than let it float until its natural expiry. Best-effort. - await revokeUpstreamToken(request, bypassToken); + // Step 4: the bypass token has done its one job — revoke it upstream + // rather than let it float until its natural expiry. Best-effort, + // fire-and-forget: nothing below depends on this finishing first. + void revokeUpstreamToken(request, bypassToken); - // Step D: the password is changed — log in for real with the new + // Step 5: the password is changed — log in for real with the new // password, exactly like login.ts does, to get a full session. - let realLoginResponse: Response; - try { - realLoginResponse = await fetch(`${config.contextforgeUrl}/auth/email/login`, { - method: "POST", - headers: { - "content-type": "application/json", - "x-forwarded-for": request.ip, - "x-real-ip": request.ip, - }, - body: JSON.stringify({ email, password: newPassword }), - signal: AbortSignal.timeout(UPSTREAM_REQUEST_TIMEOUT_MS), - }); - } catch (err) { - request.log.error({ err }, "post-change-password login failed"); - return reply.code(502).send({ error: "login_after_change_failed" }); - } - - if (!realLoginResponse.ok) { + const realLogin = await upstreamLogin(request, "/auth/email/login", email, newPassword); + if (!realLogin.ok) { // The password WAS changed successfully — this is not a // change-password failure, it's a (rare) inability to establish a // session right after. Distinct error so the SPA doesn't show a // "wrong password"/policy-violation message for a change that // actually succeeded. - request.log.error( - { status: realLoginResponse.status }, - "password changed but post-change login was rejected", - ); + request.log.error({ realLogin }, "password changed but post-change login was rejected"); return reply.code(502).send({ error: "login_after_change_failed" }); } - let realAuth: UpstreamAuthenticationResponse; // pragma: allowlist secret + // Step 6: establish a normal BFF session, identical to login.ts. try { - realAuth = (await realLoginResponse.json()) as UpstreamAuthenticationResponse; + const { user, csrfToken } = await establishSession(fastify, request, reply, realLogin.auth); + return reply.send({ user, csrfToken }); } catch (err) { - request.log.error({ err }, "post-change-password login returned a non-JSON 2xx body"); - return reply.code(502).send({ error: "login_after_change_failed" }); + if (err instanceof PasswordChangeStillRequiredError) { + // Password changed, but upstream still reports the account as + // flagged (flag not cleared, age not reset, ...) — same "changed + // but couldn't sign back in" story as the branches above. + request.log.error( + { email }, + "password changed but post-change login still flagged password_change_required", + ); + return reply.code(502).send({ error: "login_after_change_failed" }); + } + throw err; } - - if (typeof realAuth.access_token !== "string" || !realAuth.access_token) { - request.log.error({ realAuth }, "post-change-password login missing access_token"); - return reply.code(502).send({ error: "login_after_change_failed" }); - } - - // Step E: establish a normal BFF session, identical to login.ts. - const { user, csrfToken } = await establishSession(fastify, request, reply, realAuth); - return reply.send({ user, csrfToken }); }, ); } diff --git a/server/src/routes/auth/login.ts b/server/src/routes/auth/login.ts index 306299a..dd572fe 100644 --- a/server/src/routes/auth/login.ts +++ b/server/src/routes/auth/login.ts @@ -10,17 +10,11 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; -import { - clearSessionCookie, - deleteSession, - SESSION_COOKIE_NAME, -} from "../../lib/session-store.js"; -import { - establishSession, - type UpstreamAuthenticationResponse, -} from "../../lib/establish-session.js"; +import { clearSessionCookie, deleteSession, SESSION_COOKIE_NAME } from "../../lib/session-store.js"; +import { establishSession, PasswordChangeStillRequiredError } from "../../lib/establish-session.js"; import { setNoStore } from "../../lib/no-store.js"; import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; +import { upstreamLogin } from "../../lib/upstream-login.js"; import { CSRF_COOKIE_NAME } from "../../plugins/csrf.js"; interface LoginBody { @@ -33,9 +27,10 @@ interface LoginBody { // the failed attempt and a mutating request made right after can ride that // leftover cookie into a confusing downstream 403 instead of a clean 401. // Called from every same-origin failure exit below (missing credentials, -// upstream unreachable, non-2xx upstream, malformed/incomplete 2xx body) so -// the cleanup can't drift out of sync with the SPA's AuthContext.login(), -// which resets its own state on any rejected login call. +// upstream unreachable, non-2xx upstream, malformed/incomplete 2xx body, +// still-flagged account) so the cleanup can't drift out of sync with the +// SPA's AuthContext.login(), which resets its own state on any rejected +// login call. // Deliberately NOT called from the isForbiddenCrossOrigin() branch above — // that guards against a cross-site page silently POSTing to /auth/login to // mass-clear victims' legitimate sessions; only same-origin login attempts @@ -78,49 +73,44 @@ export default async function loginRoute(fastify: FastifyInstance): Promise { expect(response.statusCode).toBe(400); expect(fetchSpy).not.toHaveBeenCalled(); }); + + it("refuses to establish a session if upstream 2xxs but the user is still flagged password_change_required (defensive backstop)", async () => { + const app = await buildTestApp(); + mockUpstreamLogin(true, { + access_token: "upstream-jwt", // pragma: allowlist secret + user: { email: "user@example.com", password_change_required: true }, + }); + + const response = await app.fastify.inject({ + method: "POST", + url: "/auth/login", + payload: { email: "user@example.com", password: "secret" }, // pragma: allowlist secret + }); + + expect(response.statusCode).toBe(403); + expect(response.json().error).toBe("login_failed"); + expect(JSON.parse(response.json().detail).detail).toMatch(/password change required/i); + // clearStaleSession always emits a clearing Set-Cookie for bff_sid (see + // its doc comment) — assert it's cleared, not merely check for absence. + const cleared = response.cookies.find((c) => c.name === "bff_sid"); + expect(cleared?.value).toBe(""); + }); }); describe("POST /auth/change-password-required", () => { afterEach(() => vi.unstubAllGlobals()); + const OLD_PASSWORD = "old-secret"; // pragma: allowlist secret + const NEW_PASSWORD = "New-secret1"; // pragma: allowlist secret + interface UpstreamCall { url: string; authorization: string | undefined; + headers: Record; } interface MockLegOptions { @@ -237,22 +263,32 @@ describe("POST /auth/change-password-required", () => { } interface MockUpstreamOptions { + precondition?: MockLegOptions; // POST /auth/email/login (old password) bypassLogin?: MockLegOptions; // POST /auth/login (old password) changePassword?: MockLegOptions; // POST /auth/email/change-password - revoke?: MockLegOptions; // POST /auth/logout (bypass token) + revoke?: MockLegOptions; // POST /auth/logout realLogin?: MockLegOptions; // POST /auth/email/login (new password) } - /** Mocks all four upstream legs the route can call, and records every call made. */ + /** Mocks every upstream leg the route can call, and records every call made. */ function mockUpstream(options: MockUpstreamOptions = {}): UpstreamCall[] { const legs = { + // Default: upstream's own precondition check blocks with 403 — this is + // the ONLY signal the route trusts (see finding #1: not a persisted + // flag, which is why this leg's body carries no + // password_change_required field at all). + precondition: { + ok: false, + status: 403, + body: { + detail: "Password change required. Please change your password before continuing.", + }, + ...options.precondition, + }, bypassLogin: { ok: true, status: 200, - body: { - access_token: "bypass-jwt", // pragma: allowlist secret - user: { email: "user@example.com", password_change_required: true }, - }, + body: { access_token: "bypass-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret ...options.bypassLogin, }, changePassword: { ok: true, status: 200, body: {}, ...options.changePassword }, @@ -273,19 +309,19 @@ describe("POST /auth/change-password-required", () => { vi.stubGlobal( "fetch", vi.fn(async (url: string, init?: RequestInit) => { - const headers = init?.headers as Record | undefined; - calls.push({ url: String(url), authorization: headers?.authorization }); - - const leg = String(url).endsWith("/auth/email/change-password") - ? legs.changePassword - : String(url).endsWith("/auth/email/login") - ? legs.realLogin - : String(url).endsWith("/auth/logout") - ? legs.revoke - : String(url).endsWith("/auth/login") - ? legs.bypassLogin - : null; - if (!leg) throw new Error(`unexpected upstream fetch: ${url}`); + const headers = (init?.headers as Record | undefined) ?? {}; + const bodyPassword = init?.body + ? (JSON.parse(String(init.body)) as { password?: string }).password + : undefined; + calls.push({ url: String(url), authorization: headers.authorization, headers }); + + let leg: MockLegOptions & { ok: boolean; status: number; body: unknown }; + if (String(url).endsWith("/auth/email/change-password")) leg = legs.changePassword; + else if (String(url).endsWith("/auth/logout")) leg = legs.revoke; + else if (String(url).endsWith("/auth/login")) leg = legs.bypassLogin; + else if (String(url).endsWith("/auth/email/login")) { + leg = bodyPassword === NEW_PASSWORD ? legs.realLogin : legs.precondition; + } else throw new Error(`unexpected upstream fetch: ${url}`); return { ok: leg.ok, @@ -304,8 +340,8 @@ describe("POST /auth/change-password-required", () => { url: "/auth/change-password-required", payload: payload ?? { email: "user@example.com", - oldPassword: "old-secret", // pragma: allowlist secret - newPassword: "New-secret1", // pragma: allowlist secret + oldPassword: OLD_PASSWORD, + newPassword: NEW_PASSWORD, }, }); } @@ -329,14 +365,38 @@ describe("POST /auth/change-password-required", () => { expect(setCookieNames).toContain("bff_csrf"); }); - it("calls /auth/login (not /auth/email/login) for the bypass step, since the latter hard-blocks while the flag is set", async () => { + it("verifies the precondition via /auth/email/login's own 403, not a persisted flag — so a password-age or default-password block (never persisted) is honored too", async () => { + // The precondition leg's default body (set up above) carries no + // password_change_required field whatsoever — only its 403 status + // matters. If the route still keyed off a persisted flag, this would 403 + // with password_change_not_required instead of succeeding. + mockUpstream(); + const response = await requestChange(); + + expect(response.statusCode).toBe(200); + }); + + it("checks the precondition (old password, /auth/email/login) before ever minting a bypass token", async () => { const calls = mockUpstream(); await requestChange(); const urls = calls.map((c) => c.url); + expect(urls[0]).toBe(`${config.contextforgeUrl}/auth/email/login`); expect(urls).toContain(`${config.contextforgeUrl}/auth/login`); expect(urls).toContain(`${config.contextforgeUrl}/auth/email/change-password`); - expect(urls).toContain(`${config.contextforgeUrl}/auth/email/login`); + // The follow-up real login also hits /auth/email/login — appears twice. + expect(urls.filter((u) => u === `${config.contextforgeUrl}/auth/email/login`)).toHaveLength(2); + }); + + it("sends client-IP audit headers on the change-password call, same as every other upstream call in this flow", async () => { + const calls = mockUpstream(); + await requestChange(); + + const changeCall = calls.find( + (c) => c.url === `${config.contextforgeUrl}/auth/email/change-password`, + ); + expect(changeCall?.headers["x-forwarded-for"]).toBeTruthy(); + expect(changeCall?.headers["x-real-ip"]).toBeTruthy(); }); it("revokes the bypass token upstream after a successful change", async () => { @@ -356,25 +416,24 @@ describe("POST /auth/change-password-required", () => { expect(raw).not.toContain("real-jwt"); }); - it("passes through the bypass-login failure status when the old password is rejected, without touching later steps", async () => { + it("passes through the precondition failure status when the old password is wrong, without minting a bypass token", async () => { const calls = mockUpstream({ - bypassLogin: { ok: false, status: 401, body: { detail: "Invalid email or password" } }, + precondition: { ok: false, status: 401, body: { detail: "Invalid email or password" } }, }); const response = await requestChange(); expect(response.statusCode).toBe(401); expect(response.json().error).toBe("change_password_failed"); expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); - expect(calls.map((c) => c.url)).toEqual([`${config.contextforgeUrl}/auth/login`]); + expect(calls.map((c) => c.url)).toEqual([`${config.contextforgeUrl}/auth/email/login`]); }); - it("rejects the change when the account does not actually require a password change", async () => { + it("rejects (and revokes the unused token) when the account does not actually require a password change", async () => { const calls = mockUpstream({ - bypassLogin: { - body: { - access_token: "bypass-jwt", // pragma: allowlist secret - user: { email: "user@example.com", password_change_required: false }, - }, + precondition: { + ok: true, + status: 200, + body: { access_token: "unused-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret }, }); const response = await requestChange(); @@ -382,13 +441,13 @@ describe("POST /auth/change-password-required", () => { expect(response.statusCode).toBe(403); expect(response.json()).toEqual({ error: "password_change_not_required" }); expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); - // The bypass token served no purpose — it must still be revoked. - const revokeCall = calls.find((c) => c.url === `${config.contextforgeUrl}/auth/logout`); - expect(revokeCall?.authorization).toBe("Bearer bypass-jwt"); - // Never reaches the change-password step. - expect(calls.map((c) => c.url)).not.toContain( - `${config.contextforgeUrl}/auth/email/change-password`, - ); + // No bypass token was ever minted — only the precondition call happened, + // plus revoking the token that call itself returned. + expect(calls.map((c) => c.url)).toEqual([ + `${config.contextforgeUrl}/auth/email/login`, + `${config.contextforgeUrl}/auth/logout`, + ]); + expect(calls[1]?.authorization).toBe("Bearer unused-jwt"); }); it("passes through the change-password failure status when the new password is rejected, and still revokes the bypass token", async () => { @@ -423,6 +482,23 @@ describe("POST /auth/change-password-required", () => { ); }); + it("reports login_after_change_failed when the password changed but the follow-up login still reports the account as flagged", async () => { + mockUpstream({ + realLogin: { + body: { + access_token: "real-jwt", // pragma: allowlist secret + expires_in: 1200, + user: { email: "user@example.com", password_change_required: true }, + }, + }, + }); + const response = await requestChange(); + + expect(response.statusCode).toBe(502); + expect(response.json()).toEqual({ error: "login_after_change_failed" }); + expect(response.cookies.map((c) => c.name)).not.toContain("bff_sid"); + }); + it("returns 502 when upstream is unreachable", async () => { vi.stubGlobal( "fetch", @@ -439,7 +515,7 @@ describe("POST /auth/change-password-required", () => { const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); - const response = await requestChange({ email: "user@example.com", oldPassword: "old-secret" }); // pragma: allowlist secret + const response = await requestChange({ email: "user@example.com", oldPassword: OLD_PASSWORD }); expect(response.statusCode).toBe(400); expect(fetchSpy).not.toHaveBeenCalled(); diff --git a/src/api/changePasswordRequiredErrors.test.ts b/src/api/changePasswordRequiredErrors.test.ts index 4fa2859..c4a4fc7 100644 --- a/src/api/changePasswordRequiredErrors.test.ts +++ b/src/api/changePasswordRequiredErrors.test.ts @@ -61,6 +61,19 @@ describe("classifyChangePasswordRequiredError", () => { }); }); + it("maps a password_change_not_required body to notRequired, not invalidOldPassword, despite the shared 403 status", () => { + const body = { error: "password_change_not_required" }; + expect(classifyChangePasswordRequiredError(new ApiError(403, body, "HTTP 403"))).toEqual({ + kind: "notRequired", + }); + }); + + it("falls back to invalidOldPassword for a 403 without the password_change_not_required body", () => { + expect(classifyChangePasswordRequiredError(new ApiError(403, null, "HTTP 403"))).toEqual({ + kind: "invalidOldPassword", + }); + }); + it("falls back to failed for a non-ApiError", () => { expect(classifyChangePasswordRequiredError(new Error("network"))).toEqual({ kind: "failed" }); }); diff --git a/src/api/changePasswordRequiredErrors.ts b/src/api/changePasswordRequiredErrors.ts index 992ec6a..cffa4de 100644 --- a/src/api/changePasswordRequiredErrors.ts +++ b/src/api/changePasswordRequiredErrors.ts @@ -3,6 +3,11 @@ import { extractUpstreamApiErrorDetail } from "@/utils/errors"; export type ChangePasswordRequiredError = | { kind: "invalidOldPassword" } + // Correct old password, but the account doesn't currently need a change + // (stale link, flag cleared elsewhere, ...). Distinct from + // invalidOldPassword: credentials were fine, so the forgot-password + // fallback would be the wrong next step to offer. + | { kind: "notRequired" } | { kind: "policyViolation"; message: string | null } // Password WAS changed successfully — the BFF just couldn't log back in // and establish a session right after. Not a validation failure. @@ -13,18 +18,20 @@ export type ChangePasswordRequiredError = * Classify POST /auth/change-password-required failures. The BFF route * (server/src/routes/auth/change-password-required.ts) forwards whichever * upstream call failed first: a 401/403 means the re-authentication step - * (old password) was rejected; a 400/422 means the new password itself was - * rejected (policy); `{ error: "login_after_change_failed" }` means the - * change succeeded but the follow-up login/session-establishment didn't. - * Scoped to this endpoint only — do not merge with classifyLoginError or - * classifyPasswordResetError, which assign 403 different meanings for their - * own endpoints. + * (old password) was rejected; `{ error: "password_change_not_required" }` + * (also 403) means credentials were fine but no change is needed; a 400/422 + * means the new password itself was rejected (policy); + * `{ error: "login_after_change_failed" }` means the change succeeded but + * the follow-up login/session-establishment didn't. Scoped to this endpoint + * only — do not merge with classifyLoginError or classifyPasswordResetError, + * which assign 403 different meanings for their own endpoints. */ export function classifyChangePasswordRequiredError(error: unknown): ChangePasswordRequiredError { if (!(error instanceof ApiError)) return { kind: "failed" }; const body = error.body as { error?: string } | null; if (body?.error === "login_after_change_failed") return { kind: "changedButLoginFailed" }; + if (body?.error === "password_change_not_required") return { kind: "notRequired" }; if (error.status === 401 || error.status === 403) return { kind: "invalidOldPassword" }; diff --git a/src/auth/AuthContext.tsx b/src/auth/AuthContext.tsx index 3fdecda..5118f97 100644 --- a/src/auth/AuthContext.tsx +++ b/src/auth/AuthContext.tsx @@ -1,4 +1,12 @@ -import { createContext, useCallback, useContext, useState, useEffect, useRef } from "react"; +import { + createContext, + useCallback, + useContext, + useState, + useEffect, + useMemo, + useRef, +} from "react"; import type { ReactNode } from "react"; import { api, ApiError, setCsrfToken } from "../api/client"; import { permissionsApi } from "../api/permissions"; @@ -235,23 +243,22 @@ export function AuthProvider({ children }: { children: ReactNode }) { [perms], ); - return ( - - {children} - + const value = useMemo( + () => ({ + ...state, + login, + completePasswordChangeRequired, + logout, + setSelectedTeamId, + permissions: perms.permissions, + permissionsLoading: perms.loading, + permissionsError: perms.error, + hasPermission, + }), + [state, login, completePasswordChangeRequired, logout, setSelectedTeamId, perms, hasPermission], ); + + return {children}; } // --------------------------------------------------------------------------- diff --git a/src/i18n/locales/en-US/auth.json b/src/i18n/locales/en-US/auth.json index cfcd8a1..fa2bc54 100644 --- a/src/i18n/locales/en-US/auth.json +++ b/src/i18n/locales/en-US/auth.json @@ -58,6 +58,8 @@ "auth.passwordChangeRequired.error.complexity": "Password must contain at least 3 of: uppercase, lowercase, number, or special character.", "auth.passwordChangeRequired.error.mismatch": "Passwords do not match.", "auth.passwordChangeRequired.error.invalidOldPassword": "Your current password is incorrect.", + "auth.passwordChangeRequired.error.notRequired": "Your password doesn't need to be changed. You can sign in normally.", + "auth.passwordChangeRequired.error.missingEmail": "We couldn't tell which account this is for. Please sign in again to restart this process.", "auth.passwordChangeRequired.error.failed": "We could not change your password. Please try again.", "auth.logout": "Sign Out", "auth.login.error.invalidCredentials": "Invalid credentials.", diff --git a/src/i18n/locales/es-ES/auth.json b/src/i18n/locales/es-ES/auth.json index 3242f74..94ee65d 100644 --- a/src/i18n/locales/es-ES/auth.json +++ b/src/i18n/locales/es-ES/auth.json @@ -58,6 +58,8 @@ "auth.passwordChangeRequired.error.complexity": "La contraseña debe contener al menos 3 de estos tipos: mayúscula, minúscula, número o carácter especial.", "auth.passwordChangeRequired.error.mismatch": "Las contraseñas no coinciden.", "auth.passwordChangeRequired.error.invalidOldPassword": "Tu contraseña actual es incorrecta.", + "auth.passwordChangeRequired.error.notRequired": "Tu contraseña no necesita ser cambiada. Puedes iniciar sesión normalmente.", + "auth.passwordChangeRequired.error.missingEmail": "No pudimos identificar la cuenta. Inicia sesión de nuevo para reiniciar este proceso.", "auth.passwordChangeRequired.error.failed": "No pudimos cambiar tu contraseña. Inténtalo de nuevo.", "auth.logout": "Cerrar Sesión", "auth.login.error.invalidCredentials": "Credenciales inválidas.", diff --git a/src/i18n/locales/pt-BR/auth.json b/src/i18n/locales/pt-BR/auth.json index fe5cd07..4a9c280 100644 --- a/src/i18n/locales/pt-BR/auth.json +++ b/src/i18n/locales/pt-BR/auth.json @@ -58,6 +58,8 @@ "auth.passwordChangeRequired.error.complexity": "A senha deve conter pelo menos 3 destes tipos: maiúscula, minúscula, número ou caractere especial.", "auth.passwordChangeRequired.error.mismatch": "As senhas não coincidem.", "auth.passwordChangeRequired.error.invalidOldPassword": "Sua senha atual está incorreta.", + "auth.passwordChangeRequired.error.notRequired": "Sua senha não precisa ser alterada. Você pode entrar normalmente.", + "auth.passwordChangeRequired.error.missingEmail": "Não conseguimos identificar a conta. Entre novamente para reiniciar este processo.", "auth.passwordChangeRequired.error.failed": "Não foi possível alterar sua senha. Tente novamente.", "auth.logout": "Sair", "auth.login.error.invalidCredentials": "Credenciais inválidas.", diff --git a/src/lib/passwordPolicy.test.ts b/src/lib/passwordPolicy.test.ts new file mode 100644 index 0000000..a1c6b80 --- /dev/null +++ b/src/lib/passwordPolicy.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from "vitest"; +import { countPasswordCharacterClasses, MIN_PASSWORD_CHARACTER_CLASSES } from "./passwordPolicy"; + +describe("countPasswordCharacterClasses", () => { + it("counts all four classes for a password that genuinely has them", () => { + expect(countPasswordCharacterClasses("Password123!")).toBe(4); + }); + + it("does not count a trailing space as a special character", () => { + expect(countPasswordCharacterClasses("Password123 ")).toBe( + MIN_PASSWORD_CHARACTER_CLASSES, // upper, lower, digit — no genuine special char + ); + }); + + it("does not count accented/non-ASCII letters as a special character", () => { + // "á" is neither [a-z] nor [A-Z] nor \d nor ASCII punctuation — the old + // `/[^A-Za-z0-9]/` pattern over-counted it as "special". + expect(countPasswordCharacterClasses("Passworda1")).toBe(3); + expect(countPasswordCharacterClasses("Passwordá1")).toBe(3); + }); + + it("counts common ASCII punctuation as the special-character class", () => { + for (const char of ["!", "@", "#", "$", "%", "-", "_", "."]) { + expect(countPasswordCharacterClasses(`Password1${char}`)).toBe(4); + } + }); +}); diff --git a/src/lib/passwordPolicy.ts b/src/lib/passwordPolicy.ts index b0558a7..9b5f12c 100644 --- a/src/lib/passwordPolicy.ts +++ b/src/lib/passwordPolicy.ts @@ -1,7 +1,13 @@ // Shared 3-of-4-character-class password complexity check, used by // ResetPassword.tsx and PasswordChangeRequired.tsx so the policy can't drift // between the two forms. -const CHARACTER_CLASS_PATTERNS = [/[a-z]/, /[A-Z]/, /\d/, /[^A-Za-z0-9]/]; +// +// The "special character" class is an explicit ASCII punctuation set, not +// `/[^A-Za-z0-9]/` — that broader pattern also matches whitespace/control +// characters (so a trailing space would count) and non-ASCII letters (á, ñ, +// ç — which the other three classes already treat as neither upper- nor +// lowercase, so counting them here just double-credits the same character). +const CHARACTER_CLASS_PATTERNS = [/[a-z]/, /[A-Z]/, /\d/, /[!"#$%&'()*+,\-./:;<=>?@[\]^_`{|}~]/]; export const MIN_PASSWORD_CHARACTER_CLASSES = 3; export function countPasswordCharacterClasses(password: string): number { diff --git a/src/pages/PasswordChangeRequired.test.tsx b/src/pages/PasswordChangeRequired.test.tsx index 0d12f6a..342f024 100644 --- a/src/pages/PasswordChangeRequired.test.tsx +++ b/src/pages/PasswordChangeRequired.test.tsx @@ -130,6 +130,37 @@ describe("PasswordChangeRequired", () => { expect(screen.queryByRole("button", { name: /forgot your password/i })).not.toBeInTheDocument(); }); + it("shows a distinct message with a return-to-login fallback when the account doesn't need a change (correct credentials)", async () => { + completePasswordChangeRequired.mockRejectedValue( + new ApiError(403, { error: "password_change_not_required" }, "HTTP 403"), + ); + renderPage(); + await fillPasswords("old-pass", "New-password1"); + fireEvent.click(screen.getByRole("button", { name: "Change Password" })); + + const alert = await screen.findByRole("alert"); + expect(alert).toHaveTextContent(/doesn't need to be changed/i); + // Not the "wrong password" copy, and no forgot-password offer — the + // credentials were correct. + expect(alert).not.toHaveTextContent(/current password is incorrect/i); + expect(screen.queryByRole("button", { name: /forgot your password/i })).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole("button", { name: "Return to login" })); + expect(navigate).toHaveBeenCalledWith("/app/login"); + }); + + it("shows an error and a return-to-login CTA instead of a submittable form when ?email= is missing", () => { + window.history.pushState({}, "", "/app/change-password-required"); + renderPage(); + + expect(screen.getByRole("alert")).toHaveTextContent(/couldn't tell which account/i); + expect(document.getElementById("old-password")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Change Password" })).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole("button", { name: "Return to login" })); + expect(navigate).toHaveBeenCalledWith("/app/login"); + }); + it("shows the fallback success screen when the password changed but auto sign-in failed", async () => { completePasswordChangeRequired.mockRejectedValue( new ApiError(502, { error: "login_after_change_failed" }, "HTTP 502"), diff --git a/src/pages/PasswordChangeRequired.tsx b/src/pages/PasswordChangeRequired.tsx index 642f492..c1d9582 100644 --- a/src/pages/PasswordChangeRequired.tsx +++ b/src/pages/PasswordChangeRequired.tsx @@ -38,7 +38,13 @@ export function PasswordChangeRequired() { const [confirmPasswordError, setConfirmPasswordError] = useState(null); const [submitError, setSubmitError] = useState(null); const [showForgotPasswordFallback, setShowForgotPasswordFallback] = useState(false); + const [showReturnToLoginFallback, setShowReturnToLoginFallback] = useState(false); const [submitting, setSubmitting] = useState(false); + // Stale link / manual nav / bookmark with no ?email= — the read-only email + // field would otherwise be silently blank and submittable, and the BFF's + // generic 400 for a missing email gets misclassified as a new-password + // policy violation by the shared classifier. + const emailMissing = !email; // Only used for the rare "password changed but couldn't auto sign-in" // fallback (see classifyChangePasswordRequiredError's changedButLoginFailed) // — the happy path navigates straight into the app instead of showing this. @@ -85,6 +91,7 @@ export function PasswordChangeRequired() { setConfirmPasswordError(null); setSubmitError(null); setShowForgotPasswordFallback(false); + setShowReturnToLoginFallback(false); if (!oldPassword) { setOldPasswordError( @@ -130,6 +137,12 @@ export function PasswordChangeRequired() { intl.formatMessage({ id: "auth.passwordChangeRequired.error.invalidOldPassword" }), ); setShowForgotPasswordFallback(true); + } else if (changeError.kind === "notRequired") { + // Credentials were correct — forgot-password would be the wrong + // fallback here, offer a way back to the (now-usable) login form + // instead. + setSubmitError(intl.formatMessage({ id: "auth.passwordChangeRequired.error.notRequired" })); + setShowReturnToLoginFallback(true); } else if (changeError.kind === "policyViolation") { setNewPasswordError( changeError.message ?? @@ -165,84 +178,108 @@ export function PasswordChangeRequired() { {intl.formatMessage({ id: "auth.passwordChangeRequired.description" })}

-
-
- - + {emailMissing ? ( +
+ +
- { - setOldPassword(value); - setOldPasswordError(null); - setSubmitError(null); - setShowForgotPasswordFallback(false); - }} - autoComplete="current-password" - label={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} - placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} - required - error={oldPasswordError ?? undefined} - /> - { - setNewPassword(value); - setNewPasswordError(null); - setConfirmPasswordError(null); - }} - label={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} - placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} - required - hint={intl.formatMessage({ id: "auth.passwordChangeRequired.passwordHint" })} - error={newPasswordError ?? undefined} - /> - { - setConfirmPassword(value); - setConfirmPasswordError(null); - }} - label={intl.formatMessage({ id: "auth.passwordChangeRequired.confirmPassword" })} - placeholder={intl.formatMessage({ - id: "auth.passwordChangeRequired.confirmPassword", - })} - required - error={confirmPasswordError ?? undefined} - /> - {submitError && ( -
- + ) : ( + +
+ +
- )} - - {showForgotPasswordFallback && ( + { + setOldPassword(value); + setOldPasswordError(null); + setSubmitError(null); + setShowForgotPasswordFallback(false); + }} + autoComplete="current-password" + label={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} + placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.oldPassword" })} + required + error={oldPasswordError ?? undefined} + /> + { + setNewPassword(value); + setNewPasswordError(null); + setConfirmPasswordError(null); + }} + label={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} + placeholder={intl.formatMessage({ id: "auth.passwordChangeRequired.newPassword" })} + required + hint={intl.formatMessage({ id: "auth.passwordChangeRequired.passwordHint" })} + error={newPasswordError ?? undefined} + /> + { + setConfirmPassword(value); + setConfirmPasswordError(null); + }} + label={intl.formatMessage({ id: "auth.passwordChangeRequired.confirmPassword" })} + placeholder={intl.formatMessage({ + id: "auth.passwordChangeRequired.confirmPassword", + })} + required + error={confirmPasswordError ?? undefined} + /> + {submitError && ( +
+ +
+ )} - )} - + {showForgotPasswordFallback && ( + + )} + {showReturnToLoginFallback && ( + + )} + + )} )} From 7ab4ef5cee73eb8b780ffbc8f53d625d41c37912 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Wed, 19 Aug 2026 12:06:14 +0100 Subject: [PATCH 4/4] Clear auth on fail Signed-off-by: Gabriel Costa --- src/auth/AuthContext.test.tsx | 42 +++++++++++++++++++++++++++++++++++ src/auth/AuthContext.tsx | 32 +++++++++++++++++++------- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/auth/AuthContext.test.tsx b/src/auth/AuthContext.test.tsx index 8d93edd..0abf15a 100644 --- a/src/auth/AuthContext.test.tsx +++ b/src/auth/AuthContext.test.tsx @@ -393,6 +393,48 @@ describe("AuthContext", () => { expect(setCsrfToken).toHaveBeenCalledWith("test-csrf-token"); }); + it("clears stale auth state and CSRF token when completePasswordChangeRequired fails", async () => { + // Same scenario as login()'s equivalent test: a user reaches this + // pre-auth page (bookmark, still-open tab) while still holding a + // previously-authenticated state. + const mockUser = { + email: "user@example.com", + full_name: "Test User", + is_admin: false, + is_active: true, + auth_provider: "local", + email_verified: true, + password_change_required: false, + }; + + vi.mocked(api.get).mockResolvedValueOnce({ + authenticated: true, + user: mockUser, + csrfToken: "stale-csrf-token", + }); + + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId("auth-status")).toHaveTextContent("authenticated"); + }); + + vi.mocked(api.post).mockRejectedValueOnce(new ApiError(401, "Unauthorized", "")); + + screen.getByText("CompletePasswordChangeRequired").click(); + + await waitFor(() => { + expect(screen.getByTestId("auth-status")).toHaveTextContent("guest"); + }); + + expect(screen.queryByTestId("user-email")).not.toBeInTheDocument(); + expect(setCsrfToken).toHaveBeenLastCalledWith(null); + }); + it("handles successful logout", async () => { const mockUser = { email: "user@example.com", diff --git a/src/auth/AuthContext.tsx b/src/auth/AuthContext.tsx index 5118f97..75a8667 100644 --- a/src/auth/AuthContext.tsx +++ b/src/auth/AuthContext.tsx @@ -178,15 +178,31 @@ export function AuthProvider({ children }: { children: ReactNode }) { oldPassword: string, // pragma: allowlist secret newPassword: string, // pragma: allowlist secret ): Promise => { - const data = await api.post( - "/auth/change-password-required", - { email, oldPassword, newPassword }, - { authenticated: false }, - ); + try { + const data = await api.post( + "/auth/change-password-required", + { email, oldPassword, newPassword }, + { authenticated: false }, + ); - setCsrfToken(data.csrfToken); - authVersion.current += 1; - setState({ user: data.user, isAuthenticated: true, isLoading: false, selectedTeamId: null }); + setCsrfToken(data.csrfToken); + authVersion.current += 1; + setState({ + user: data.user, + isAuthenticated: true, + isLoading: false, + selectedTeamId: null, + }); + } catch (err) { + // Same reasoning as login()'s catch: don't leave a stale CSRF token + // or a previously "authenticated" state around on failure — a user + // who lands on this pre-auth page with an existing session (bookmark, + // still-open tab) must not keep that state if the BFF call fails. + setCsrfToken(null); + authVersion.current += 1; + setState({ user: null, isAuthenticated: false, isLoading: false, selectedTeamId: null }); + throw err; + } }, [], );