From 7233845b10ad07b81951acb8312355652dababc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 31 Jul 2026 11:08:06 +0200 Subject: [PATCH] Close four ways around the locale guard The guard read a parameter as required whenever it carried no question mark and no default, so `Locale | undefined`, `Locale | null`, a locale widened to `any`, and a default sitting on the destructuring pattern all walked straight past it. Each of those lets a caller say nothing and the sentence come back in English, which is the bug the guard exists to stop. A counter-case pins the other direction: a plainly required locale still reads clean, so the guard stays quiet on the shape the chain is meant to have. The correlation read now also asserts the language it was asked in; nothing else in that file would have noticed it going back to English. --- src/__tests__/helpers/narrated-locale.ts | 75 ++++++++- .../narrated-locale-guard-defeat.test.ts | 74 +++++++++ .../insights/__tests__/route-locale.test.ts | 142 ++++++++++++++++++ src/components/cycle/use-cycle.ts | 6 +- src/lib/mcp/__tests__/rich-reads.test.ts | 4 + src/lib/query-keys/cycle.ts | 9 +- 6 files changed, 302 insertions(+), 8 deletions(-) create mode 100644 src/app/api/cycle/insights/__tests__/route-locale.test.ts diff --git a/src/__tests__/helpers/narrated-locale.ts b/src/__tests__/helpers/narrated-locale.ts index 7389fb835..a7042029b 100644 --- a/src/__tests__/helpers/narrated-locale.ts +++ b/src/__tests__/helpers/narrated-locale.ts @@ -103,6 +103,50 @@ function isLocaleType(node: ts.TypeNode | undefined, alias: string): boolean { return false; } +/** + * Does the type still let a caller say nothing? `Locale | undefined` and + * `Locale | null` carry no `?` and no default, so a check that only looked at + * the question mark read them as required — and they are the original bug + * wearing a different hat: the caller passes `undefined`, the body picks a + * language, and the sentence comes back in English again. `any` and `unknown` + * are the same hole with the door taken off. + */ +function toleratesAbsence(node: ts.TypeNode | undefined): boolean { + if (!node) return false; + if ( + node.kind === ts.SyntaxKind.UndefinedKeyword || + node.kind === ts.SyntaxKind.NullKeyword || + node.kind === ts.SyntaxKind.AnyKeyword || + node.kind === ts.SyntaxKind.UnknownKeyword || + node.kind === ts.SyntaxKind.VoidKeyword + ) { + return true; + } + if (ts.isLiteralTypeNode(node)) { + return node.literal.kind === ts.SyntaxKind.NullKeyword; + } + if (ts.isUnionTypeNode(node)) return node.types.some(toleratesAbsence); + return false; +} + +/** + * Property names given a default inside an object binding pattern — + * `function narrate({ locale = "en" }: { locale: Locale })`. The default sits + * on the binding, not on the parameter, so `parameter.initializer` is empty + * and the signature reads clean. + */ +function defaultedBindingNames( + parameter: ts.ParameterDeclaration, +): Set { + const names = new Set(); + if (!ts.isObjectBindingPattern(parameter.name)) return names; + for (const element of parameter.name.elements) { + if (element.initializer === undefined) continue; + names.add((element.propertyName ?? element.name).getText()); + } + return names; +} + /** Find a top-level function/interface declaration by name. */ function findDeclaration( source: ts.SourceFile, @@ -147,16 +191,23 @@ export function localeSites( owner: string, ownerOptional: boolean, ownerHasInitializer: boolean, + defaultedNames: Set = new Set(), ) => { for (const member of members) { if (!ts.isPropertySignature(member)) continue; if (!isLocaleType(member.type, alias)) continue; + const name = member.name.getText(); sites.push({ - where: `${owner}.${member.name.getText()}`, + where: `${owner}.${name}`, // The property itself may be required while the object it lives on is - // optional or defaulted — which puts the silence right back. - optional: member.questionToken !== undefined || ownerOptional, - hasInitializer: ownerHasInitializer, + // optional or defaulted — which puts the silence right back. So does a + // type that merely admits `undefined` / `null`: no question mark, and a + // caller can still hand over nothing. + optional: + member.questionToken !== undefined || + ownerOptional || + toleratesAbsence(member.type), + hasInitializer: ownerHasInitializer || defaultedNames.has(name), }); } }; @@ -168,13 +219,24 @@ export function localeSites( const owner = parameter.name.getText(); const optional = parameter.questionToken !== undefined; const hasInitializer = parameter.initializer !== undefined; + const defaultedNames = defaultedBindingNames(parameter); if (isLocaleType(parameter.type, alias)) { - sites.push({ where: owner, optional, hasInitializer }); + sites.push({ + where: owner, + optional: optional || toleratesAbsence(parameter.type), + hasInitializer, + }); continue; } const type = parameter.type; if (type && ts.isTypeLiteralNode(type)) { - collectFromMembers(type.members, owner, optional, hasInitializer); + collectFromMembers( + type.members, + owner, + optional, + hasInitializer, + defaultedNames, + ); } else if (type && ts.isTypeReferenceNode(type)) { const referenced = findDeclaration(source, type.typeName.getText()); if (referenced && ts.isInterfaceDeclaration(referenced)) { @@ -183,6 +245,7 @@ export function localeSites( type.typeName.getText(), optional, hasInitializer, + defaultedNames, ); } } diff --git a/src/__tests__/narrated-locale-guard-defeat.test.ts b/src/__tests__/narrated-locale-guard-defeat.test.ts index 475068986..cd94e11b1 100644 --- a/src/__tests__/narrated-locale-guard-defeat.test.ts +++ b/src/__tests__/narrated-locale-guard-defeat.test.ts @@ -107,6 +107,80 @@ describe("the locale guard cannot be talked around", () => { expect(site.optional).toBe(true); }); + it("catches `Locale | undefined` written WITHOUT the question mark", () => { + // The nastiest disguise, because it reads as required at a glance: no `?`, + // no default, and the field is spelled out in the signature. But the caller + // can still pass `undefined`, the body picks a language, and the sentence + // comes back in English — the original bug, one hat later. + const [site] = localeSites( + parse( + FILE, + `import type { Locale } from "@/lib/i18n/config"; + export function narrate(opts: { locale: Locale | undefined }) { + return opts.locale === undefined ? "en" : opts.locale; + }`, + ), + "narrate", + ); + expect(site.optional).toBe(true); + }); + + it("catches `Locale | null` the same way", () => { + const [site] = localeSites( + parse( + FILE, + `import type { Locale } from "@/lib/i18n/config"; + export function narrate(opts: { locale: Locale | null }) { + return opts.locale === null ? "en" : opts.locale; + }`, + ), + "narrate", + ); + expect(site.optional).toBe(true); + }); + + it("catches a locale widened to `any`", () => { + const [site] = localeSites( + parse( + FILE, + `import type { Locale } from "@/lib/i18n/config"; + export function narrate(opts: { locale: Locale | any }) { return opts; }`, + ), + "narrate", + ); + expect(site.optional).toBe(true); + }); + + it("catches a default hidden in the parameter's own binding pattern", () => { + // The field is required in the type and the parameter has no initializer, + // so both of the earlier checks read clean — the default sits on the + // destructuring instead. + const [site] = localeSites( + parse( + FILE, + `import type { Locale } from "@/lib/i18n/config"; + export function narrate({ locale = "en" }: { locale: Locale }) { return locale; }`, + ), + "narrate", + ); + expect(site.hasInitializer).toBe(true); + }); + + it("leaves a plainly required locale alone", () => { + // The shape the whole chain is supposed to have. If this ever reads as a + // violation the guard stops being a guard and starts being noise. + const [site] = localeSites( + parse( + FILE, + `import type { Locale } from "@/lib/i18n/config"; + export function narrate(opts: { locale: Locale }) { return opts.locale; }`, + ), + "narrate", + ); + expect(site.optional).toBe(false); + expect(site.hasInitializer).toBe(false); + }); + it("refuses to pass a declaration that dropped the locale entirely", () => { expect(() => localeSites( diff --git a/src/app/api/cycle/insights/__tests__/route-locale.test.ts b/src/app/api/cycle/insights/__tests__/route-locale.test.ts new file mode 100644 index 000000000..9cac955dc --- /dev/null +++ b/src/app/api/cycle/insights/__tests__/route-locale.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +/** + * The cycle insights route has to say who is reading, too. + * + * Each surviving lagged row carries a finished `interpretation` sentence that + * this route serves straight to the client. The engine underneath can write it + * in any of the six languages, and it only writes the right one if the route + * resolves the reader's language and hands it over — the same handoff the + * metric page's "Coach read" strip was missing when it answered a German page + * in English. + * + * So what is pinned here is the handoff itself: the resolved locale arriving at + * `discoverPhaseCorrelations`, from the cookie a language switch sets and from + * the stored preference when there is no cookie. Without this the route could + * go back to naming one language outright and nothing in the suite would + * notice. + */ + +const discoverPhaseCorrelations = vi.fn(() => ({ + discovered: [], + pairsTested: 0, + fdrQ: 0.1, + minPairs: 20, +})); +vi.mock("@/lib/cycle/phase-crosstab", async (importOriginal) => ({ + ...(await importOriginal()), + discoverPhaseCorrelations: (args: unknown) => discoverPhaseCorrelations(args), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + auditLog: { create: vi.fn() }, + appSettings: { findUnique: vi.fn().mockResolvedValue(null) }, + menstrualCycle: { findMany: vi.fn().mockResolvedValue([]) }, + cycleDayLog: { findMany: vi.fn().mockResolvedValue([]) }, + measurement: { findMany: vi.fn().mockResolvedValue([]) }, + moodEntry: { findMany: vi.fn().mockResolvedValue([]) }, + user: { + findUnique: vi.fn().mockResolvedValue({ sourcePriorityJson: null }), + }, + }, +})); +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/cycle/gate", () => ({ + requireCycleEnabled: vi.fn(async () => ({ + enabled: true, + profile: { lutealPhaseLength: 14, averageCycleLength: 28 }, + })), +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(async () => ({ allowed: true })), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +const localeCookie = vi.fn<() => string | undefined>(() => undefined); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: (name: string) => + name === "healthlog-locale" && localeCookie() !== undefined + ? { value: localeCookie() } + : undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { GET } from "../route"; +import { getSession } from "@/lib/auth/session"; + +function sessionWithLocale(locale: string | null) { + return { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { + id: "user-1", + username: "testuser", + role: "USER" as const, + gender: "FEMALE" as const, + timezone: "Europe/Berlin", + locale, + }, + }; +} + +beforeEach(() => { + vi.mocked(getSession).mockResolvedValue( + sessionWithLocale(null) as unknown as Awaited< + ReturnType + >, + ); + discoverPhaseCorrelations.mockClear(); + localeCookie.mockReturnValue(undefined); +}); + +/** The locale the route handed the discovery engine on the last call. */ +function passedLocale(): unknown { + const args = discoverPhaseCorrelations.mock.calls.at(-1)?.[0] as + { locale?: unknown } | undefined; + return args?.locale; +} + +describe("GET /api/cycle/insights — the reader's language", () => { + it("passes the language the switcher's cookie names", async () => { + localeCookie.mockReturnValue("de"); + + const res = await GET(new Request("http://localhost/api/cycle/insights")); + + expect(res.status).toBe(200); + expect(passedLocale()).toBe("de"); + }); + + it("falls back to the stored preference when there is no cookie", async () => { + vi.mocked(getSession).mockResolvedValue( + sessionWithLocale("pl") as unknown as Awaited< + ReturnType + >, + ); + + await GET(new Request("http://localhost/api/cycle/insights")); + + expect(passedLocale()).toBe("pl"); + }); + + it("never hard-codes a language of its own", async () => { + // Two readers, two languages, from the same handler. A route that named + // one outright would pass this only by accident. + localeCookie.mockReturnValue("it"); + await GET(new Request("http://localhost/api/cycle/insights")); + const first = passedLocale(); + localeCookie.mockReturnValue("fr"); + await GET(new Request("http://localhost/api/cycle/insights")); + + expect([first, passedLocale()]).toEqual(["it", "fr"]); + }); +}); diff --git a/src/components/cycle/use-cycle.ts b/src/components/cycle/use-cycle.ts index 591971cfd..323a7ee19 100644 --- a/src/components/cycle/use-cycle.ts +++ b/src/components/cycle/use-cycle.ts @@ -204,8 +204,12 @@ export function useCycleProfile() { * insights tab. */ export function useCycleInsights() { + // The lagged rows come back with a narrated `interpretation` in the reader's + // language, so the cache cell belongs to one language — otherwise a switch + // would keep serving the sentence written for the previous one. + const { locale } = useTranslations(); return useQuery({ - queryKey: queryKeys.cycleInsights(), + queryKey: queryKeys.cycleInsights(locale), queryFn: () => apiGet("/api/cycle/insights"), staleTime: 5 * 60_000, }); diff --git a/src/lib/mcp/__tests__/rich-reads.test.ts b/src/lib/mcp/__tests__/rich-reads.test.ts index 907d9722f..923016292 100644 --- a/src/lib/mcp/__tests__/rich-reads.test.ts +++ b/src/lib/mcp/__tests__/rich-reads.test.ts @@ -218,6 +218,10 @@ describe("get_correlation", () => { expect(res.pair?.r).toBe(-0.42); expect(res.association).toBe("descriptive"); expect(res.windowDays).toBe(180); + // The `note` on the pair is a finished sentence an MCP client shows as it + // stands, so the account's language has to reach the reader. Nothing else + // in this file would notice if the read went back to asking in English. + expect(readCoachCorrelations).toHaveBeenCalledWith(USER, "de"); }); it("honest-null when the pair has no surviving association", async () => { diff --git a/src/lib/query-keys/cycle.ts b/src/lib/query-keys/cycle.ts index c4ca520d9..09e394aa7 100644 --- a/src/lib/query-keys/cycle.ts +++ b/src/lib/query-keys/cycle.ts @@ -22,7 +22,14 @@ export const cycleKeys = { * on-ramp reads this so a non-FEMALE account can opt in before the gated * cycle page is reachable. */ cyclePrefs: () => ["cycle", "prefs"] as const, - cycleInsights: () => ["cycle", "insights"] as const, + /** + * The FDR-guarded phase-correlation read (`/api/cycle/insights`). Keyed by + * locale: each surviving lagged row now carries an `interpretation` the + * server writes in the reader's language, so a cached payload belongs to one + * language only. `["cycle"]` stays the prefix, so every cycle write still + * evicts it through `cycleDependentKeys`. + */ + cycleInsights: (locale: string) => ["cycle", "insights", locale] as const, cycleDayLog: (date: string) => ["cycle", "day-log", date] as const, /** The caller's own custom symptoms (decrypted labels) the log-day sheet * merges into the seeded chip grid. */