From 2ef4a9fcf6aa43ca195138c99d2f28d05bd4667b Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 02:11:32 +0300 Subject: [PATCH 1/5] feat(auth): carry remember_me from the sign-in exchanges to the backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RUK-290, first half. The backend already accepts `remember_me` on the two built-in sign-in endpoints and ignores it ("session modes are a separate change" — that change is this ticket). The frontend has been sending nothing, so Go read the zero value and every session got the same lifetime. This starts sending the flag truthfully; the backend still decides what it grants. Chain from `authorize` to the wire, and why it is spelled out so carefully: five of its sites are object literals rather than types, and widening a signature never forces a literal to pass the new field. A missed site produces no type error and no runtime error — the checkbox simply stops working for everyone, indistinguishable from a user who never ticked it. Three sites land here (the provider's `credentials` declaration, both `authorize` returns, both exchange call literals); the UI half follows. Three decisions worth keeping: - `authorize` parses fail-closed: only the exact string `"true"` is a yes. NextAuth carries credentials as strings and `Boolean("false") === true`, so the obvious coercion would hand long sessions to people who deliberately unticked the box. - Normalisation happens once, in `runBuiltInSignIn`. `interface User` must keep the field optional (Google and dev-bypass share that type and return no such field), but everything below takes a plain boolean. A `?? false` at the exchange instead would make the contract test pass whether or not the value ever arrived. - The body key is written unconditionally, never `...(x ? {k:v} : {})`. That idiom lives a few functions away in this same file, and copying it would drop the key on `false` — the one case worth proving. The contract test imports the path constants rather than retyping them: a hardcoded path asserts only itself and stays green against a wrong one. Each new test was mutation-checked — removing the `credentials` declaration, swapping the strict comparison for `Boolean()`, and dropping the field from a call literal each make it fail. Co-Authored-By: Claude Opus 5 --- .../built-in-sign-in-callback.test.ts | 10 +- .../auth/__tests__/built-in-sign-in.test.ts | 24 ++- .../auth/__tests__/remember-me-chain.test.ts | 152 +++++++++++++++++ src/server/auth/auth-config.ts | 28 ++- src/server/auth/backend-token-exchange.ts | 37 +++- src/server/auth/built-in-sign-in.ts | 17 +- src/server/auth/next-auth.d.ts | 7 + tests/contracts/remember-me.contract.test.ts | 161 ++++++++++++++++++ 8 files changed, 422 insertions(+), 14 deletions(-) create mode 100644 src/server/auth/__tests__/remember-me-chain.test.ts create mode 100644 tests/contracts/remember-me.contract.test.ts diff --git a/src/server/auth/__tests__/built-in-sign-in-callback.test.ts b/src/server/auth/__tests__/built-in-sign-in-callback.test.ts index e96ee27..59cbc4a 100644 --- a/src/server/auth/__tests__/built-in-sign-in-callback.test.ts +++ b/src/server/auth/__tests__/built-in-sign-in-callback.test.ts @@ -97,6 +97,9 @@ describe("AC-4 — a lost binding must never be reported as a wrong code", () => email: "someone@example.test", code: "123456", sessionNonce: "n-1", + // Normalised from an absent flag (RUK-290): `undefined` must not travel + // on, or `JSON.stringify` would turn "unticked" into "absent". + rememberMe: false, }); }); @@ -171,6 +174,7 @@ describe("AC-3a — a verified code establishes the session", () => { email: "someone@example.test", code: "123456", sessionNonce: "n-42", + rememberMe: false, }); }); @@ -200,7 +204,11 @@ describe("AC-3a — a verified code establishes the session", () => { callSignIn(account, { signInKind: "password", email: "admin@example.test", password: "pw" }), ).resolves.toBe(true); - expect(loginWithPassword).toHaveBeenCalledWith({ email: "admin@example.test", password: "pw" }); + expect(loginWithPassword).toHaveBeenCalledWith({ + email: "admin@example.test", + password: "pw", + rememberMe: false, + }); expect(account.maintmodeTokens).toEqual(TOKENS); // A password sign-in must never touch the OTP binding. expect(readOtpBinding).not.toHaveBeenCalled(); diff --git a/src/server/auth/__tests__/built-in-sign-in.test.ts b/src/server/auth/__tests__/built-in-sign-in.test.ts index d097694..3e055fa 100644 --- a/src/server/auth/__tests__/built-in-sign-in.test.ts +++ b/src/server/auth/__tests__/built-in-sign-in.test.ts @@ -64,7 +64,12 @@ describe("verifyOtpCode", () => { jsonResponse(200, { access_token: "at-1", refresh_token: "rt-1", expires_in: 3600 }), ); - await verifyOtpCode({ email: "a@example.test", code: "123456", sessionNonce: "nonce-1" }); + await verifyOtpCode({ + email: "a@example.test", + code: "123456", + sessionNonce: "nonce-1", + rememberMe: false, + }); const [url, init] = fetchMock.mock.calls[0]; expect(String(url)).toBe("http://backend.test/auth/api/v1/login/otp/verify"); @@ -72,6 +77,9 @@ describe("verifyOtpCode", () => { email: "a@example.test", code: "123456", session_nonce: "nonce-1", + // Present even when false (RUK-290): see remember-me.contract.test.ts for + // why absent and false must stay distinguishable on this endpoint. + remember_me: false, }); }); @@ -81,7 +89,7 @@ describe("verifyOtpCode", () => { fetchMock.mockResolvedValueOnce(jsonResponse(200, { access_token: "at-only", expires_in: 3600 })); await expect( - verifyOtpCode({ email: "a@example.test", code: "123456", sessionNonce: "n" }), + verifyOtpCode({ email: "a@example.test", code: "123456", sessionNonce: "n", rememberMe: false }), ).resolves.toMatchObject({ access_token: "at-only" }); }); @@ -92,7 +100,12 @@ describe("verifyOtpCode", () => { ); await expect( - verifyOtpCode({ email: "a@example.test", code: "123456", sessionNonce: "stale" }), + verifyOtpCode({ + email: "a@example.test", + code: "123456", + sessionNonce: "stale", + rememberMe: false, + }), ).rejects.toBeInstanceOf(BackendAuthError); fetchMock.mockResolvedValueOnce( @@ -103,6 +116,7 @@ describe("verifyOtpCode", () => { email: "a@example.test", code: "000000", sessionNonce: "good", + rememberMe: false, }).catch((e: unknown) => e); expect(wrongCode).toBeInstanceOf(BackendAuthError); @@ -117,13 +131,14 @@ describe("loginWithPassword", () => { jsonResponse(200, { access_token: "at-2", refresh_token: "rt-2", expires_in: 3600 }), ); - await loginWithPassword({ email: "admin@example.test", password: "hunter2" }); + await loginWithPassword({ email: "admin@example.test", password: "hunter2", rememberMe: false }); const [url, init] = fetchMock.mock.calls[0]; expect(String(url)).toBe("http://backend.test/auth/api/v1/login/password"); expect(JSON.parse(String(init?.body))).toEqual({ email: "admin@example.test", password: "hunter2", + remember_me: false, }); }); @@ -137,6 +152,7 @@ describe("loginWithPassword", () => { const error = await loginWithPassword({ email: "admin@example.test", password: "wrong", + rememberMe: false, }).catch((e: unknown) => e); expect(error).toBeInstanceOf(BackendAuthError); diff --git a/src/server/auth/__tests__/remember-me-chain.test.ts b/src/server/auth/__tests__/remember-me-chain.test.ts new file mode 100644 index 0000000..9243af2 --- /dev/null +++ b/src/server/auth/__tests__/remember-me-chain.test.ts @@ -0,0 +1,152 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** + * RUK-290 — the `remember_me` chain, and the four places it dies quietly. + * SPEC §3.2, AC 15 and AC 16. + * + * The flag crosses nine sites between the checkbox and the wire, and five of + * them are literals rather than types: the `signIn(...)` call object, the + * provider's `credentials` declaration, both `return`s in `authorize`, and both + * exchange call objects in `runBuiltInSignIn`. Widening a signature never forces + * a literal to pass the new field, so a missed site yields **no type error and + * no runtime error** — the box just silently stops working, for everyone, and + * looks exactly like a user who never ticked it. + * + * That is why the parser test below is not enough on its own and AC 16 exists: + * `authorize`'s parsing can be perfect while the value never reaches it. The + * two halves of this file are deliberate — one proves the decision, the other + * proves the delivery. + */ + +// --------------------------------------------------------------------------- +// AC 15 — fail-closed parsing, and the declaration that lets it run at all. +// --------------------------------------------------------------------------- + +/** + * Read as source text rather than by booting NextAuth: importing `auth-config` + * evaluates the real provider list at module load (it reads env), so a + * behavioural test here would assert more about the harness than the wiring. + * This mirrors `backend-login-provider.test.ts`, which guards the same file the + * same way and for the same reason. + */ +const authConfigSource = readFileSync(join(process.cwd(), "src/server/auth/auth-config.ts"), "utf8"); + +describe("AC 15 — the flag is declared, and parsed fail-closed", () => { + it("declares rememberMe among the provider's credentials", () => { + // Site 5. NextAuth forwards only declared keys, so without this line the + // value never reaches `authorize` and every other test here still passes. + expect(authConfigSource).toMatch(/credentials:\s*\{[^\n]*\brememberMe:\s*\{\}/); + }); + + it("treats only the exact string \"true\" as a yes", () => { + // Site 6. `Boolean("false") === true`, so the obvious coercion would hand a + // long session to someone who deliberately unticked the box. Asserting the + // comparison is strict equality against "true" is the point. + expect(authConfigSource).toContain('credentials?.rememberMe === "true"'); + expect(authConfigSource).not.toMatch(/Boolean\(\s*credentials\??\.\s*rememberMe/); + expect(authConfigSource).not.toMatch(/!!\s*credentials\??\.\s*rememberMe/); + }); + + it("puts the parsed flag on both returned users, not just one", () => { + // Site 6 again: an OTP-only fix is the easy half-miss, and it leaves + // password sign-in silently unable to ask for a long session. + const authorizeStart = authConfigSource.indexOf("async authorize(credentials)"); + const authorizeEnd = authConfigSource.indexOf("}),\n);", authorizeStart); + const body = authConfigSource.slice(authorizeStart, authorizeEnd); + + const otpBranch = body.indexOf('signInKind: "otp" as const'); + const passwordBranch = body.indexOf('signInKind: "password" as const'); + expect(otpBranch).toBeGreaterThan(-1); + expect(passwordBranch).toBeGreaterThan(-1); + + // One `rememberMe,` after each branch marker. + expect(body.slice(otpBranch, passwordBranch)).toContain("rememberMe,"); + expect(body.slice(passwordBranch)).toContain("rememberMe,"); + }); +}); + +// --------------------------------------------------------------------------- +// AC 16 — end-to-end: the value actually reaches the exchange. +// --------------------------------------------------------------------------- + +const verifyOtpCode = vi.fn(); +const loginWithPassword = vi.fn(); +const fetchBackendMe = vi.fn(); +const readOtpBinding = vi.fn(); +const clearOtpBinding = vi.fn(); + +vi.mock("@/server/auth/backend-token-exchange", () => ({ + verifyOtpCode: (...args: unknown[]) => verifyOtpCode(...args), + loginWithPassword: (...args: unknown[]) => loginWithPassword(...args), + fetchBackendMe: (...args: unknown[]) => fetchBackendMe(...args), + exchangeGoogleIdToken: vi.fn(), + acceptInvitation: vi.fn(), + refreshBackendToken: vi.fn(), +})); + +vi.mock("@/server/auth/otp-nonce-cookie", () => ({ + readOtpBinding: () => readOtpBinding(), + clearOtpBinding: () => clearOtpBinding(), + setOtpBinding: vi.fn(), + normalizeEmail: (email: string) => email.trim().toLowerCase(), +})); + +const { runBuiltInSignIn } = await import("@/server/auth/built-in-sign-in"); + +function callSignIn(user: Record) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return runBuiltInSignIn({ provider: "backend-login" } as any, user as any); +} + +beforeEach(() => { + vi.clearAllMocks(); + const tokens = { access_token: "a", refresh_token: "r", expires_in: 900 }; + verifyOtpCode.mockResolvedValue(tokens); + loginWithPassword.mockResolvedValue(tokens); + fetchBackendMe.mockResolvedValue({ + id: "u1", + email: "admin@example.test", + display_name: "Admin", + roles: ["admin"], + }); + readOtpBinding.mockResolvedValue({ email: "admin@example.test", nonce: "nonce-1" }); +}); + +describe("AC 16 — the ticked box reaches the exchange", () => { + it("passes rememberMe: true through the password branch", async () => { + await callSignIn({ signInKind: "password", email: "admin@example.test", password: "pw", rememberMe: true }); + + expect(loginWithPassword).toHaveBeenCalledWith(expect.objectContaining({ rememberMe: true })); + }); + + it("passes rememberMe: true through the OTP branch", async () => { + await callSignIn({ signInKind: "otp", email: "admin@example.test", otpCode: "123456", rememberMe: true }); + + expect(verifyOtpCode).toHaveBeenCalledWith(expect.objectContaining({ rememberMe: true })); + }); + + it("passes an explicit false when the box was left unticked", async () => { + await callSignIn({ + signInKind: "password", + email: "admin@example.test", + password: "pw", + rememberMe: false, + }); + + expect(loginWithPassword).toHaveBeenCalledWith(expect.objectContaining({ rememberMe: false })); + }); + + it("normalises a missing flag to false rather than passing undefined on", async () => { + // `interface User` has to keep the field optional — Google and dev-bypass + // share that type and return no such field — so `undefined` genuinely can + // arrive here. It must not travel further: `JSON.stringify` drops undefined + // keys, which would turn "unticked" into "absent" on the wire. + await callSignIn({ signInKind: "password", email: "admin@example.test", password: "pw" }); + + expect(loginWithPassword).toHaveBeenCalledWith(expect.objectContaining({ rememberMe: false })); + const [args] = loginWithPassword.mock.calls[0] as [Record]; + expect(args.rememberMe).not.toBeUndefined(); + }); +}); diff --git a/src/server/auth/auth-config.ts b/src/server/auth/auth-config.ts index c6cdfff..22a702c 100644 --- a/src/server/auth/auth-config.ts +++ b/src/server/auth/auth-config.ts @@ -65,7 +65,11 @@ providers.push( Credentials({ id: BACKEND_LOGIN_PROVIDER_ID, name: "Email sign-in", - credentials: { kind: {}, email: {}, code: {}, password: {} }, + // `rememberMe` MUST be declared here: NextAuth forwards only the keys a + // provider lists, so an undeclared field never reaches `authorize` at all — + // and the failure is silent (no type error, no runtime error, the checkbox + // simply never works). Values cross as strings, hence the parse below. + credentials: { kind: {}, email: {}, code: {}, password: {}, rememberMe: {} }, /** * Shape validation ONLY — deliberately no network call. The exchange lives * in the `signIn` callback so the backend call and its error mapping stay in @@ -80,6 +84,12 @@ providers.push( return null; } + // Fail closed: ONLY the exact string "true" is a yes. Anything else — + // "false", undefined, junk — is a no. Note `Boolean("false") === true`, + // so the obvious coercion would hand out long sessions to users who + // deliberately unticked the box. + const rememberMe = credentials?.rememberMe === "true"; + if (kind === "otp") { const code = typeof credentials?.code === "string" ? credentials.code.trim() : ""; // Rejecting a malformed code here avoids spending one of the five @@ -88,7 +98,13 @@ providers.push( if (!isWellFormedOtpCode(code)) { return null; } - return { id: BACKEND_LOGIN_PROVIDER_ID, signInKind: "otp" as const, email, otpCode: code }; + return { + id: BACKEND_LOGIN_PROVIDER_ID, + signInKind: "otp" as const, + email, + otpCode: code, + rememberMe, + }; } if (kind === "password") { @@ -96,7 +112,13 @@ providers.push( if (!password) { return null; } - return { id: BACKEND_LOGIN_PROVIDER_ID, signInKind: "password" as const, email, password }; + return { + id: BACKEND_LOGIN_PROVIDER_ID, + signInKind: "password" as const, + email, + password, + rememberMe, + }; } return null; diff --git a/src/server/auth/backend-token-exchange.ts b/src/server/auth/backend-token-exchange.ts index 2bfd421..3830fa3 100644 --- a/src/server/auth/backend-token-exchange.ts +++ b/src/server/auth/backend-token-exchange.ts @@ -10,8 +10,13 @@ const LOGOUT_PATH = "/api/v1/logout"; const LOGOUT_ALL_PATH = "/api/v1/logout/all"; const ME_PATH = "/api/v1/me"; const OTP_REQUEST_PATH = "/api/v1/login/otp/request"; -const OTP_VERIFY_PATH = "/api/v1/login/otp/verify"; -const PASSWORD_LOGIN_PATH = "/api/v1/login/password"; +/** + * Exported so the contract test can assert against the constant instead of + * retyping the string: a hardcoded copy in the test would assert only itself + * and stay green if the real path ever changed (SPEC §8.1). + */ +export const OTP_VERIFY_PATH = "/api/v1/login/otp/verify"; +export const PASSWORD_LOGIN_PATH = "/api/v1/login/password"; const PASSWORD_RESET_REQUEST_PATH = "/api/v1/password/reset/request"; const PASSWORD_RESET_CONFIRM_PATH = "/api/v1/password/reset/confirm"; const CHANGE_PASSWORD_PATH = "/api/v1/me/password"; @@ -122,10 +127,20 @@ export async function verifyOtpCode(args: { email: string; code: string; sessionNonce: string; + /** + * Session-length request, not a command: the backend decides what it grants. + * Non-optional on purpose — see the note on `loginWithPassword`. + */ + rememberMe: boolean; }): Promise { return postBackendJson( OTP_VERIFY_PATH, - { email: args.email, code: args.code, session_nonce: args.sessionNonce }, + { + email: args.email, + code: args.code, + session_nonce: args.sessionNonce, + remember_me: args.rememberMe, + }, // `refresh_token` carries `omitempty` and may legitimately be absent, so — // unlike the Google path — it is not required here. (parsed) => Boolean(parsed?.access_token), @@ -144,10 +159,24 @@ export async function verifyOtpCode(args: { export async function loginWithPassword(args: { email: string; password: string; + /** + * Session-length request (RUK-290). Two deliberate choices here: + * + * - **Non-optional**, so an unticked box cannot arrive as `undefined` and be + * silently dropped by `JSON.stringify`. Normalisation happens once, in + * `runBuiltInSignIn`; a `?? false` here instead would make the contract test + * pass whether or not the value ever reached this function. + * - Written into the body **unconditionally**, never as + * `...(rememberMe ? { remember_me: true } : {})`. That idiom appears a few + * functions below in `changeBackendPassword`, where absent genuinely differs + * from empty — copying it here would drop the key on `false`, which is the + * one case worth proving. + */ + rememberMe: boolean; }): Promise { return postBackendJson( PASSWORD_LOGIN_PATH, - { email: args.email, password: args.password }, + { email: args.email, password: args.password, remember_me: args.rememberMe }, (parsed) => Boolean(parsed?.access_token), ); } diff --git a/src/server/auth/built-in-sign-in.ts b/src/server/auth/built-in-sign-in.ts index c229369..28f025a 100644 --- a/src/server/auth/built-in-sign-in.ts +++ b/src/server/auth/built-in-sign-in.ts @@ -22,9 +22,21 @@ import { */ export async function runBuiltInSignIn( account: { maintmodeTokens?: BackendTokenPair; maintmodeUser?: AuthSessionUser }, - user: { signInKind?: "otp" | "password"; email?: string | null; otpCode?: string; password?: string }, + user: { + signInKind?: "otp" | "password"; + email?: string | null; + otpCode?: string; + password?: string; + rememberMe?: boolean; + }, ): Promise { const email = normalizeEmail(typeof user.email === "string" ? user.email : ""); + // Normalised exactly once, here. `interface User` must keep the field + // optional (Google and dev-bypass share that type and return no such field), + // but everything downstream takes a plain `boolean` — so the one `??` in the + // chain lives at this boundary. Putting it in the exchange functions instead + // would make their contract test pass whether or not the value ever arrived. + const rememberMe = user.rememberMe ?? false; let tokens: BackendTokenPair; if (user.signInKind === "otp") { @@ -49,6 +61,7 @@ export async function runBuiltInSignIn( email: binding.email, code: user.otpCode ?? "", sessionNonce: binding.nonce, + rememberMe, }); } catch (error) { // The backend checks the nonce before the code, so it can also report a @@ -73,7 +86,7 @@ export async function runBuiltInSignIn( await clearOtpBinding(); } else { try { - tokens = await loginWithPassword({ email, password: user.password ?? "" }); + tokens = await loginWithPassword({ email, password: user.password ?? "", rememberMe }); } catch { // The backend answers every password failure with one uniform 401 — // wrong password, blocked, signup refused, seats exhausted — precisely so diff --git a/src/server/auth/next-auth.d.ts b/src/server/auth/next-auth.d.ts index ccb60d8..6791c29 100644 --- a/src/server/auth/next-auth.d.ts +++ b/src/server/auth/next-auth.d.ts @@ -56,6 +56,13 @@ declare module "next-auth" { email?: string | null; otpCode?: string; password?: string; + /** + * Session-length request (RUK-290). Optional here and only here: this + * interface is shared with the Google and dev-bypass providers, which + * return no such field. `runBuiltInSignIn` normalises it to a plain + * `boolean` once, and every layer below takes it non-optional. + */ + rememberMe?: boolean; } } diff --git a/tests/contracts/remember-me.contract.test.ts b/tests/contracts/remember-me.contract.test.ts new file mode 100644 index 0000000..0f4d29c --- /dev/null +++ b/tests/contracts/remember-me.contract.test.ts @@ -0,0 +1,161 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Contract test — `remember_me` on the two built-in sign-in exchanges. + * RUK-290, SPEC §3.1, §3.2 (site 9), AC 13/14/20. + * + * **Why global `fetch` is stubbed rather than the shared harness.** The auth + * exchanges do not go through `backendRequest`: `backend-token-exchange.ts` owns + * a private `backendFetch` that calls `fetch` directly, so + * `tests/contracts/_harness.ts` (which mocks `authenticatedBackendRequest`) has + * nothing to intercept here. This is the same seam, and the same technique, as + * `me-password.contract.test.ts`. + * + * **What this test is for.** `remember_me` is a session-length request: the user + * ticks a box and the backend decides how long the refresh token lives. The + * whole value of the box is that the flag *arrives*. Two failures would be + * invisible without this test, because both leave every other test green: + * + * 1. the key is dropped when the box is UNTICKED — the tempting + * `...(rememberMe ? { remember_me: true } : {})` spread, an idiom that + * already lives a few functions away in this very file (see + * `changeBackendPassword`). Absent and `false` are the same to Go's `bool` + * *today*, so nothing breaks until the day the backend distinguishes them; + * 2. the value never leaves the browser layer at all, because one of the + * literals along SPEC §3.2's nine-site chain was not updated. A missed + * site produces no type error and no runtime error — the box just silently + * does nothing. + * + * So the assertions below read the body off the `fetch` stub, never off the + * arguments passed in, and they check `false` as carefully as `true`. + * + * The paths are IMPORTED, not retyped. A hardcoded `"/api/v1/login/password"` + * would assert only itself and stay green against a wrong path — the exact trap + * SPEC §8.1 describes. + */ + +const { OTP_VERIFY_PATH, PASSWORD_LOGIN_PATH, loginWithPassword, verifyOtpCode } = await import( + "@/server/auth/backend-token-exchange" +); + +const fetchMock = vi.fn(); + +beforeEach(() => { + vi.stubGlobal("fetch", fetchMock); + fetchMock.mockReset(); + process.env.MAINTMODE_API_BASE_URL = "http://backend.test/maintmode"; + process.env.MAINTMODE_AUTH_API_BASE_URL = "http://backend.test/auth"; +}); + +afterEach(() => vi.unstubAllGlobals()); + +/** A successful token pair, shaped as the backend sends it. */ +function backendReturnsTokens() { + fetchMock.mockResolvedValue({ + ok: true, + status: 200, + statusText: "OK", + text: async () => + JSON.stringify({ access_token: "access-1", refresh_token: "refresh-1", expires_in: 900 }), + }); +} + +/** What actually went on the wire, read from the stub rather than the input. */ +function wireRequest() { + const [url, init] = fetchMock.mock.calls[0]; + return { + url: String(url), + body: JSON.parse(String(init.body)) as Record, + }; +} + +describe("password sign-in — remember_me on the wire", () => { + it("sends remember_me: true to the password login path when the box is ticked", async () => { + backendReturnsTokens(); + + await loginWithPassword({ email: "admin@example.test", password: "hunter2", rememberMe: true }); + + const { url, body } = wireRequest(); + expect(url).toContain(PASSWORD_LOGIN_PATH); + expect(body.remember_me).toBe(true); + }); + + it("sends remember_me: false — present, not omitted — when the box is unticked", async () => { + backendReturnsTokens(); + + await loginWithPassword({ email: "admin@example.test", password: "hunter2", rememberMe: false }); + + const { body } = wireRequest(); + // `toBe(false)` alone would pass on `undefined`-free JSON only by accident; + // the `in` check is what rejects a conditional spread that drops the key. + expect("remember_me" in body).toBe(true); + expect(body.remember_me).toBe(false); + }); + + it("keeps the credentials it was given alongside the flag", async () => { + backendReturnsTokens(); + + await loginWithPassword({ email: "admin@example.test", password: "hunter2", rememberMe: true }); + + const { body } = wireRequest(); + expect(body).toMatchObject({ email: "admin@example.test", password: "hunter2" }); + }); +}); + +describe("OTP sign-in — remember_me on the wire", () => { + it("sends remember_me: true to the OTP verify path when the box is ticked", async () => { + backendReturnsTokens(); + + await verifyOtpCode({ + email: "admin@example.test", + code: "123456", + sessionNonce: "nonce-1", + rememberMe: true, + }); + + const { url, body } = wireRequest(); + expect(url).toContain(OTP_VERIFY_PATH); + expect(body.remember_me).toBe(true); + }); + + it("sends remember_me: false — present, not omitted — when the box is unticked", async () => { + backendReturnsTokens(); + + await verifyOtpCode({ + email: "admin@example.test", + code: "123456", + sessionNonce: "nonce-1", + rememberMe: false, + }); + + const { body } = wireRequest(); + expect("remember_me" in body).toBe(true); + expect(body.remember_me).toBe(false); + }); + + it("keeps the bound address, code and nonce alongside the flag", async () => { + backendReturnsTokens(); + + await verifyOtpCode({ + email: "admin@example.test", + code: "123456", + sessionNonce: "nonce-1", + rememberMe: false, + }); + + const { body } = wireRequest(); + expect(body).toMatchObject({ + email: "admin@example.test", + code: "123456", + session_nonce: "nonce-1", + }); + }); +}); + +describe("the two paths stay distinct", () => { + it("does not post the password login to the OTP verify path", () => { + // Guards against a copy-paste that would send both flows to one endpoint — + // cheap to check, and the kind of thing a body-only assertion would miss. + expect(PASSWORD_LOGIN_PATH).not.toBe(OTP_VERIFY_PATH); + }); +}); From 1cca4d6bc37462066f068c623a16b77872197c9a Mon Sep 17 00:00:00 2001 From: Ruslan Kosykh Date: Tue, 8 Sep 2026 02:19:03 +0300 Subject: [PATCH 2/5] feat(login): offer "Keep me signed in" on the built-in sign-in forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RUK-290, second half — the UI end of the chain the previous commit wired up. The box appears on the password form and on the OTP code step, unchecked by default, and its value reaches the backend as `remember_me`. Two of the five silent-failure sites land here: the `signIn(...)` object literal in `credentialsSignInAction`, and the prop chain from the forms through `LoginPageProps` to the two `"use server"` wrappers. Widening the input union without touching the literal compiles and sends nothing, so a test that follows the flag to the wire is the only thing that catches it — that is what the new cases in `built-in-sign-in-actions.test.ts` do, asserted off the `signIn` mock. Decisions worth keeping: - The third argument is REQUIRED, not optional. Optional would have let the three existing test files keep compiling while feeding `undefined` into a chain typed `boolean` — a mismatch with no compiler error anywhere. Those three files are updated here instead; the compiler failing on them was the point. - The box lives on the OTP *code* step, not the address step: the code submission is the request that mints a session. Returning to step one resets it explicitly in `backToEmail()` and in the `otp_session_mismatch` branch, because both reset the step in place rather than unmounting — nothing would have cleared it for us. - The label names no duration. The token pair carries no refresh TTL and no echo of the flag, so "stay signed in for N days" would be invented. - When the backend advertises both methods their forms render side by side, each with its own box. The one that counts is the one in the form actually submitted; hoisting shared state across two independent sign-in flows to make a single box serve both would couple them for cosmetics. Also fixes an unrelated jsdom gap this surfaced: Radix's Checkbox measures itself through `useSize`, so every auth test file rendering the login page died on `ResizeObserver is not defined` before reaching an assertion. Stubbed inline per file, matching the existing settings tests — `src/features/**` may not import `@/shared/testing/**`, and that boundary is worth more than deduplicating six lines. Co-Authored-By: Claude Opus 5 --- next-env.d.ts | 4 +- src/app/(public)/login/page.tsx | 8 ++-- .../accept-invite-no-query-provider.test.tsx | 12 +++++ .../__tests__/login-page-methods.test.tsx | 12 +++++ .../auth/__tests__/login-page-reset.test.tsx | 12 +++++ .../auth/__tests__/login-page.test.tsx | 12 +++++ .../auth/__tests__/otp-sign-in-flow.test.tsx | 41 ++++++++++++++++- .../__tests__/password-sign-in-form.test.tsx | 34 +++++++++++++- src/features/auth/login-page.tsx | 12 +++-- src/features/auth/otp-sign-in-flow.tsx | 37 +++++++++++++-- src/features/auth/password-sign-in-form.tsx | 24 +++++++++- .../built-in-sign-in-actions.test.ts | 46 ++++++++++++++++++- .../auth/__tests__/remember-me-chain.test.ts | 9 +++- src/server/auth/built-in-sign-in-actions.ts | 11 ++++- tests/contracts/remember-me.contract.test.ts | 5 +- 15 files changed, 252 insertions(+), 27 deletions(-) diff --git a/next-env.d.ts b/next-env.d.ts index a419cbe..ce4e94a 100644 --- a/next-env.d.ts +++ b/next-env.d.ts @@ -1,7 +1,7 @@ /// /// -import "./.next/dev/types/routes.d.ts"; -import "./.next/dev/types/root-params.d.ts"; +import "./.next/types/routes.d.ts"; +import "./.next/types/root-params.d.ts"; // NOTE: This file should not be edited // see https://nextjs.org/docs/app/api-reference/config/typescript for more information. diff --git a/src/app/(public)/login/page.tsx b/src/app/(public)/login/page.tsx index 0ecc280..21d4534 100644 --- a/src/app/(public)/login/page.tsx +++ b/src/app/(public)/login/page.tsx @@ -63,14 +63,14 @@ export default async function Page({ * server, and the sanitized `redirectTo` is closed over here so a client can * never supply a destination of its own and route around `safeNext`. */ - async function otpSignInAction(email: string, code: string) { + async function otpSignInAction(email: string, code: string, rememberMe: boolean) { "use server"; - return credentialsSignInAction({ kind: "otp", email, code, next: redirectTo }); + return credentialsSignInAction({ kind: "otp", email, code, rememberMe, next: redirectTo }); } - async function passwordSignInAction(email: string, password: string) { + async function passwordSignInAction(email: string, password: string, rememberMe: boolean) { "use server"; - return credentialsSignInAction({ kind: "password", email, password, next: redirectTo }); + return credentialsSignInAction({ kind: "password", email, password, rememberMe, next: redirectTo }); } return ( diff --git a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx index 65baa21..5876912 100644 --- a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx +++ b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx @@ -6,6 +6,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { AcceptInvitePage } from "../accept-invite-page"; import { LoginPage } from "../login-page"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); const noopAccept = vi.fn(async () => {}); diff --git a/src/features/auth/__tests__/login-page-methods.test.tsx b/src/features/auth/__tests__/login-page-methods.test.tsx index e078bfd..3137d44 100644 --- a/src/features/auth/__tests__/login-page-methods.test.tsx +++ b/src/features/auth/__tests__/login-page-methods.test.tsx @@ -5,6 +5,18 @@ import { afterEach, describe, expect, it } from "vitest"; import { LoginPage } from "@/features/auth/login-page"; import type { SignInMethod } from "@/domain/auth/sign-in-method"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + /** * RUK-288 AC-1 / AC-2 / AC-11 — the login page is drawn from the backend's * method list, and cannot lock anyone out when that list is unavailable. diff --git a/src/features/auth/__tests__/login-page-reset.test.tsx b/src/features/auth/__tests__/login-page-reset.test.tsx index 6a158cb..63fea29 100644 --- a/src/features/auth/__tests__/login-page-reset.test.tsx +++ b/src/features/auth/__tests__/login-page-reset.test.tsx @@ -5,6 +5,18 @@ import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/re import { LoginPage } from "@/features/auth/login-page"; import type { SignInMethod } from "@/domain/auth/sign-in-method"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); const PASSWORD_METHOD: SignInMethod = { diff --git a/src/features/auth/__tests__/login-page.test.tsx b/src/features/auth/__tests__/login-page.test.tsx index 36293b2..07aab0e 100644 --- a/src/features/auth/__tests__/login-page.test.tsx +++ b/src/features/auth/__tests__/login-page.test.tsx @@ -4,6 +4,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { LoginPage } from "../login-page"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + // This config has no global testing-library auto-cleanup, so unmount between // tests to keep the document free of stale renders. afterEach(() => cleanup()); diff --git a/src/features/auth/__tests__/otp-sign-in-flow.test.tsx b/src/features/auth/__tests__/otp-sign-in-flow.test.tsx index ee41076..e146dbe 100644 --- a/src/features/auth/__tests__/otp-sign-in-flow.test.tsx +++ b/src/features/auth/__tests__/otp-sign-in-flow.test.tsx @@ -4,6 +4,18 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { OtpSignInFlow } from "@/features/auth/otp-sign-in-flow"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + /** * RUK-288 AC-4 / AC-5 / AC-6 — the two-step code flow and its state table. */ @@ -364,7 +376,34 @@ describe("§6.6 — the address from step one is the one verified", () => { }); fireEvent.click(screen.getByRole("button", { name: "Sign in" })); - await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456")); + await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456", false)); + }); + + it("sends the remember-me choice with the code", async () => { + // RUK-290. The box lives on the CODE step because that is the request that + // mints the session; the address step issues no token. + const submitCode = vi.fn(async () => ({})); + setup({ submitCode }); + + fireEvent.change(screen.getByLabelText("Email code"), { + target: { value: "someone@example.test" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); + await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { + target: { value: "123456" }, + }); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + + await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456", true)); + }); + + it("does not offer the remember-me box on the address step", () => { + // It would attach the choice to a request that issues no token. + setup({}); + + expect(screen.queryByLabelText("Keep me signed in")).toBeNull(); }); it("refuses a short code locally rather than spending a backend attempt", async () => { diff --git a/src/features/auth/__tests__/password-sign-in-form.test.tsx b/src/features/auth/__tests__/password-sign-in-form.test.tsx index 24f0614..90503fe 100644 --- a/src/features/auth/__tests__/password-sign-in-form.test.tsx +++ b/src/features/auth/__tests__/password-sign-in-form.test.tsx @@ -4,6 +4,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { PasswordSignInForm } from "@/features/auth/password-sign-in-form"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); function setup(submit = vi.fn(async () => ({}) as { error?: string })) { @@ -21,7 +33,27 @@ describe("password sign-in form", () => { fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); fireEvent.click(screen.getByRole("button", { name: "Sign in" })); - await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2")); + await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2", false)); + }); + + it("sends the remember-me choice with the credentials", async () => { + // RUK-290. The box is a session-length REQUEST; the whole point is that it + // arrives. Asserted on the submit handler rather than on the checkbox's own + // state, because a checkbox that toggles but is never read looks identical. + const submit = setup(); + + fireEvent.change(screen.getByLabelText("Email"), { target: { value: "admin@example.test" } }); + fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + + await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2", true)); + }); + + it("starts unticked, so a shared computer is the default", () => { + setup(); + + expect(screen.getByLabelText("Keep me signed in").getAttribute("data-state")).toBe("unchecked"); }); it("stays disabled until both fields are filled", () => { diff --git a/src/features/auth/login-page.tsx b/src/features/auth/login-page.tsx index 5365a05..0a065b1 100644 --- a/src/features/auth/login-page.tsx +++ b/src/features/auth/login-page.tsx @@ -35,9 +35,15 @@ export interface LoginPageProps { signInAction: (providerId: string) => Promise; /** Step one of the OTP flow: mails a code and binds it to this browser. */ requestOtpAction: (email: string) => Promise<{ error?: string }>; - /** Step two, and the password form: establishes the session. */ - otpSignInAction: (email: string, code: string) => Promise<{ error?: string }>; - passwordSignInAction: (email: string, password: string) => Promise<{ error?: string }>; + /** + * Step two, and the password form: establishes the session. + * + * `rememberMe` is required rather than optional (RUK-290). Optional would let + * a caller omit it and feed `undefined` into a chain typed `boolean`, with no + * compiler error anywhere along the way. + */ + otpSignInAction: (email: string, code: string, rememberMe: boolean) => Promise<{ error?: string }>; + passwordSignInAction: (email: string, password: string, rememberMe: boolean) => Promise<{ error?: string }>; /** Abandons the current OTP flow so another address can be used. */ changeEmailAction: () => Promise; /** Step one of the password reset (RUK-289): mails a code, binds this browser. */ diff --git a/src/features/auth/otp-sign-in-flow.tsx b/src/features/auth/otp-sign-in-flow.tsx index a9adf6c..d50bc4f 100644 --- a/src/features/auth/otp-sign-in-flow.tsx +++ b/src/features/auth/otp-sign-in-flow.tsx @@ -3,6 +3,7 @@ import { useCallback, useState } from "react"; import { Button } from "@/shared/ui/shadcn/button"; +import { Checkbox } from "@/shared/ui/shadcn/checkbox"; import { Input } from "@/shared/ui/shadcn/input"; import { Label } from "@/shared/ui/shadcn/label"; import { isWellFormedOtpCode } from "@/domain/auth/sign-in-method"; @@ -22,7 +23,11 @@ type Step = "email" | "code"; export interface OtpSignInFlowProps { label: string; requestCode: (email: string) => Promise<{ error?: string }>; - submitCode: (email: string, code: string) => Promise<{ error?: string }>; + /** + * `rememberMe` is a required third argument (RUK-290) — optional would let a + * caller pass `undefined` into a chain typed `boolean` with no compiler error. + */ + submitCode: (email: string, code: string, rememberMe: boolean) => Promise<{ error?: string }>; onChangeEmail: () => Promise; } @@ -30,6 +35,9 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: const [step, setStep] = useState("email"); const [email, setEmail] = useState(""); const [code, setCode] = useState(""); + // Belongs to step two: this is the request that mints the session. Asking on + // the address step would attach the choice to a request that issues no token. + const [rememberMe, setRememberMe] = useState(false); const [error, setError] = useState(); const [pending, setPending] = useState(false); @@ -69,7 +77,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: const result = await timers.guard(async () => { setPending(true); setError(undefined); - const outcome = await submitCode(email, code.trim()); + const outcome = await submitCode(email, code.trim(), rememberMe); setPending(false); return outcome; }); @@ -83,6 +91,10 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: // is the primary action and nothing is throttled. setStep("email"); setCode(""); + // Reset explicitly: the step changes in place, the component is not + // unmounted, so nothing clears this for us. Returning to step one is a + // fresh sign-in and must not silently inherit the previous choice. + setRememberMe(false); timers.reset(); } setError(result.error); @@ -92,6 +104,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: await onChangeEmail(); setStep("email"); setCode(""); + setRememberMe(false); setError(undefined); timers.reset(); } @@ -158,9 +171,23 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }:

)} {!expired ? ( - + <> +
+ setRememberMe(checked === true)} + /> + {/* Names no duration on purpose — see the password form. */} + +
+ + ) : null}