From 6ae0cc993ffa9fdca56bc5aafa5029cd8755e03b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 7 Aug 2026 01:00:02 +0200 Subject: [PATCH] fix: close four small gaps around the record surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The privacy dashboard re-implemented the audit-retention resolver with its own default and its own floor. Two implementations of one disclosure, quoted to the same person on two pages and by the purge job; it uses the shared resolver now. The record-content branch in the settings gate matched a classification rather than a page, so a second manage-writable destination would have rendered the allergy and family-history managers under its own heading without anybody choosing that. It resolves through a slug map, and the contract test fails if the map and the classification disagree in either direction — a destination cannot join the kind without somebody deciding what it renders. The managed-profile nav filter assumed a guardian grant is always manage. It is today, which is why the check was easy to leave out; an entry below that level would have listed destinations the gate refuses. Interpolation used a replacement string, and String.replace reads $&, $`, $', $1 and $$ out of one. A display name containing $' rewrote the sentence around it — and display names land in exactly the sentences that say who did what in somebody's health record. A replacer function has no $ grammar at all, which removes the class rather than escaping the members of it somebody thought of. --- .../managed-record-navigation.test.tsx | 15 ++++ .../record-settings-contract.test.ts | 18 ++++ src/app/api/settings/privacy-summary/route.ts | 20 +++-- .../record-settings-section-gate.test.tsx | 86 ++++++++++++++++++- .../settings/record-settings-section-gate.tsx | 33 ++++++- src/components/settings/settings-shell.tsx | 9 +- .../interpolation-is-literal.test.ts | 83 ++++++++++++++++++ src/lib/i18n/context.tsx | 11 ++- src/lib/i18n/server-translator.ts | 11 ++- 9 files changed, 272 insertions(+), 14 deletions(-) create mode 100644 src/lib/i18n/__tests__/interpolation-is-literal.test.ts diff --git a/src/__tests__/managed-record-navigation.test.tsx b/src/__tests__/managed-record-navigation.test.tsx index 1f0c56634..5f8fd08d7 100644 --- a/src/__tests__/managed-record-navigation.test.tsx +++ b/src/__tests__/managed-record-navigation.test.tsx @@ -151,6 +151,21 @@ describe("managed record navigation", () => { } }); + it("offers a managed entry below MANAGE nothing at all", () => { + // A guardian grant is always MANAGE today, which is exactly why the level + // check was easy to leave out of the managed branch. Without it a managed + // entry that ever arrived below MANAGE would list destinations the section + // gate refuses, sending somebody to a page that explains it cannot open. + recordRef.value = { kind: "managed", level: "read" }; + const html = renderSettings(); + + for (const section of SETTINGS_SECTIONS) { + expect(html, section.slug).not.toContain( + `href="/settings/${section.slug}"`, + ); + } + }); + it("keeps the exit control present while administering a managed record", () => { recordRef.value = { kind: "managed", level: "manage" }; const html = renderToStaticMarkup( diff --git a/src/__tests__/record-settings-contract.test.ts b/src/__tests__/record-settings-contract.test.ts index 50d3781ab..0e1c2b3d0 100644 --- a/src/__tests__/record-settings-contract.test.ts +++ b/src/__tests__/record-settings-contract.test.ts @@ -10,6 +10,7 @@ import { isManageDelegateSettingsDestination, SETTINGS_DESTINATION_INVENTORY, } from "@/lib/record-settings/classification"; +import { RECORD_CONTENT_SECTIONS } from "@/components/settings/record-settings-section-gate"; import { toRecordSettingsDto } from "@/lib/record-settings/dto"; import { resolveManagedIntegrationState } from "@/lib/record-settings/integrations"; import { assertRecordSettingsResponseForRecord } from "@/lib/record-settings/response"; @@ -114,6 +115,23 @@ describe("record settings contract", () => { } }); + it("gives every record-content destination a page of its own", () => { + // Both directions, and the first is the one that matters. `manage-writable` + // is a classification; the gate has to know which PAGE each slug is, or a + // destination added to the kind would silently inherit the allergy and + // family-history managers under its own heading. The map is what says + // which, and this is what stops the two drifting apart — a new + // manage-writable slug fails here, at the moment it is added, rather than + // rendering somebody else's forms. + const classified = Object.keys(SETTINGS_DESTINATION_INVENTORY) + .filter(isManageDelegateSettingsDestination) + .sort(); + const rendered = Object.keys(RECORD_CONTENT_SECTIONS).sort(); + + expect(classified).not.toHaveLength(0); + expect(rendered).toEqual(classified); + }); + it("leaves exactly one destination on the manage-writable list", () => { // The list is short on purpose and a second entry is a consent decision, // not a classification tidy-up. Pinning the count means a destination diff --git a/src/app/api/settings/privacy-summary/route.ts b/src/app/api/settings/privacy-summary/route.ts index 8bc58c600..f7e8fae74 100644 --- a/src/app/api/settings/privacy-summary/route.ts +++ b/src/app/api/settings/privacy-summary/route.ts @@ -8,17 +8,25 @@ */ import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiSuccess } from "@/lib/api-response"; +import { getAuditLogRetentionDays } from "@/lib/jobs/audit-log-cleanup"; import { annotate } from "@/lib/logging/context"; import { ENCRYPTED_COLUMNS } from "@/lib/crypto/encrypted-columns"; export const dynamic = "force-dynamic"; /** - * Retention windows. These mirror the defaults + env names the cleanup jobs - * read (`coach-message-cleanup.ts`, `audit-log-cleanup.ts`, the reminder - * cleanup handlers); read here directly so the dashboard never imports a - * pg-boss job module. Off-host backup retention is operator infrastructure - * (not an app setting), so it is disclosed as prose, not a number. + * Retention windows, as the privacy dashboard states them. + * + * The audit window comes from `getAuditLogRetentionDays()` rather than from a + * second parse of the same variable. It used to be re-implemented here with + * its own default and its own floor, which is two implementations of one + * disclosure — and this surface, the sharing panel and the purge job all quote + * the number to the same person. A drift between them is a page telling + * somebody their data is kept for a length of time it is not. + * + * The rest still parse locally so the dashboard never imports a pg-boss job + * module. Off-host backup retention is operator infrastructure (not an app + * setting), so it is disclosed as prose, not a number. */ function resolveRetention() { const intEnv = (name: string, fallback: number, min: number): number => { @@ -28,7 +36,7 @@ function resolveRetention() { }; return { coachMessagesDays: intEnv("COACH_MESSAGE_RETENTION_DAYS", 365, 30), - auditLogDays: intEnv("AUDIT_LOG_RETENTION_DAYS", 365, 7), + auditLogDays: getAuditLogRetentionDays(), // The push-attempt + mood-dispatch ledgers are a fixed 90-day window. deliveryLogDays: 90, }; diff --git a/src/components/settings/__tests__/record-settings-section-gate.test.tsx b/src/components/settings/__tests__/record-settings-section-gate.test.tsx index 63045a08e..9bac09382 100644 --- a/src/components/settings/__tests__/record-settings-section-gate.test.tsx +++ b/src/components/settings/__tests__/record-settings-section-gate.test.tsx @@ -8,7 +8,7 @@ const authRef: { active: { accountId: string; recordKind: "managed" | "shared"; - level: "manage"; + level: "manage" | "write" | "read"; sections: null; canWrite: true; } | null; @@ -38,10 +38,13 @@ vi.mock("../managed-record-settings-section", () => ({ vi.mock("../managed-integration-status", () => ({ ManagedIntegrationStatus: () =>
, })); +vi.mock("../record-anamnesis-section", () => ({ + RecordAnamnesisSection: () =>
, +})); import { RecordSettingsSectionGate } from "../record-settings-section-gate"; -function render(section: "modules" | "integrations") { +function render(section: "modules" | "integrations" | "anamnesis" | "labs") { return renderToStaticMarkup( @@ -77,4 +80,83 @@ describe("RecordSettingsSectionGate", () => { expect(html).toContain("managed-settings"); expect(html).not.toContain("actor-settings"); }); + + /** + * The record-content destination, and the two ways it must not open. + * + * The branch keys on the SLUG as well as on the classification. Keying on + * the kind alone would render the allergy and family-history managers under + * the heading of any future `manage-writable` destination — silently, + * because a category matched where a page was meant. + */ + describe("the anamnesis destination", () => { + it("opens for an adult manager on an ordinary shared record", () => { + authRef.isLoading = false; + authRef.active = { + accountId: "shared-record", + recordKind: "shared", + level: "manage", + sections: null, + canWrite: true, + }; + + const html = render("anamnesis"); + + expect(html).toContain("record-anamnesis"); + expect(html).not.toContain("actor-settings"); + expect(html).not.toContain("managed-settings"); + }); + + it("opens for a Guardian, rather than the guardian configuration panel", () => { + authRef.isLoading = false; + authRef.active = { + accountId: "managed-record", + recordKind: "managed", + level: "manage", + sections: null, + canWrite: true, + }; + + const html = render("anamnesis"); + + expect(html).toContain("record-anamnesis"); + expect(html).not.toContain("managed-settings"); + }); + + it("stays shut below MANAGE", () => { + authRef.isLoading = false; + authRef.active = { + accountId: "shared-record", + recordKind: "shared", + level: "write", + sections: null, + canWrite: true, + }; + + const html = render("anamnesis"); + + expect(html).not.toContain("record-anamnesis"); + expect(html).toContain("shared-record-settings-unavailable-title"); + }); + + it("does not lend its page to another destination of the same kind", () => { + // `labs` is `adult-shared-unavailable` today, so this passes for the + // right reason now; the claim it pins is that the branch asks WHICH + // page, not which category. A second `manage-writable` slug added + // without an arm falls through to the refusal panel. + authRef.isLoading = false; + authRef.active = { + accountId: "shared-record", + recordKind: "shared", + level: "manage", + sections: null, + canWrite: true, + }; + + const html = render("labs"); + + expect(html).not.toContain("record-anamnesis"); + expect(html).toContain("shared-record-settings-unavailable-title"); + }); + }); }); diff --git a/src/components/settings/record-settings-section-gate.tsx b/src/components/settings/record-settings-section-gate.tsx index 449c55bf6..aaf697477 100644 --- a/src/components/settings/record-settings-section-gate.tsx +++ b/src/components/settings/record-settings-section-gate.tsx @@ -14,6 +14,20 @@ import { RecordAnamnesisSection } from "@/components/settings/record-anamnesis-s import { SettingsCard } from "@/components/settings/settings-card"; import type { SettingsSectionSlug } from "@/components/settings/section-slugs"; +/** + * The record-CONTENT destinations, and what each one renders inside a record. + * + * Keyed by slug so the branch below asks which page it is on rather than which + * category the page belongs to. Every `manage-writable` slug must have an + * entry and nothing else may: the contract test holds both directions, which + * is what stops a new destination inheriting this one's forms. + */ +export const RECORD_CONTENT_SECTIONS: Partial< + Record ReactNode> +> = { + anamnesis: RecordAnamnesisSection, +}; + const MANAGED_RECORD_SETTINGS_FAMILY_BY_SECTION = { account: "profile", modules: "modules", @@ -65,9 +79,22 @@ export function RecordSettingsSectionGate({ // Checked before the managed branches so a Guardian gets the same record // surface an adult manager does, rather than the guardian configuration // panel, which has no family for it. - if (destination.kind === "manage-writable" && level === "manage") { - return ; - } + // + // Resolved through a slug map rather than by matching the kind, and that is + // not belt-and-braces. `manage-writable` is a classification, not a + // component: a second destination joining it — a future record-content page + // — would otherwise render the allergy and family-history managers under its + // own heading, silently, because the branch matched a category where a page + // was meant. `RECORD_CONTENT_SECTIONS` says which page each slug is, a slug + // with no entry falls through to the refusal panel below, and + // `record-settings-contract.test.ts` fails if the map and the classification + // ever disagree — so a destination cannot join the kind without somebody + // deciding what it renders. + const RecordContent = + level === "manage" && destination.kind === "manage-writable" + ? RECORD_CONTENT_SECTIONS[section] + : undefined; + if (RecordContent) return ; if ( recordKind === "managed" && diff --git a/src/components/settings/settings-shell.tsx b/src/components/settings/settings-shell.tsx index 7d1a03655..15d3f9335 100644 --- a/src/components/settings/settings-shell.tsx +++ b/src/components/settings/settings-shell.tsx @@ -460,10 +460,17 @@ export function SettingsShell({ ? allVisibleSections : allVisibleSections.filter((section) => { const kind = classifySettingsDestination(section.slug).kind; + // Every destination on either list needs MANAGE, so the level is + // checked once for both record kinds rather than only for the adult + // one. A guardian grant is always MANAGE today, which is exactly why + // the check was easy to leave out — and a managed entry that ever + // arrived below it would have listed destinations the section gate + // refuses, sending somebody to a page that explains it cannot open. + if (activeRecord.level !== "manage") return false; if (activeRecord.recordKind === "managed") { return kind === "managed-guardian" || kind === "manage-writable"; } - return kind === "manage-writable" && activeRecord.level === "manage"; + return kind === "manage-writable"; }); const activeSection = allVisibleSections.find( (section) => section.slug === activeSlug, diff --git a/src/lib/i18n/__tests__/interpolation-is-literal.test.ts b/src/lib/i18n/__tests__/interpolation-is-literal.test.ts new file mode 100644 index 000000000..16459558a --- /dev/null +++ b/src/lib/i18n/__tests__/interpolation-is-literal.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from "vitest"; + +import { getServerTranslator } from "@/lib/i18n/server-translator"; + +/** + * An interpolated value is substituted literally, whatever is in it. + * + * ## The defect + * + * Both translators filled `{param}` with + * `value.replace(new RegExp("\\{k\\}", "g"), String(v))`, and a replacement + * STRING is not literal text: `String.prototype.replace` reads `$&`, `` $` ``, + * `$'`, `$1` and `$$` out of it. So a value containing `$'` splices in + * everything that followed the placeholder, `` $` `` splices in everything + * before it, and `$&` repeats the placeholder itself. The sentence a person + * reads is then not the sentence the bundle holds. + * + * Every value that reaches this path is user-controlled, and the ones that + * matter most are the ones this release added: a display name lands in + * "{name} removed a reading" and in "{name} tried to change a dose and was + * refused", on the panel where an owner works out what somebody did in their + * health record. A name of `$'` turns "Alex removed a reading" into a sentence + * with the wrong subject and no way for the reader to tell. + * + * Not an injection in the XSS sense — the result is React text children, and + * this repository ships no markdown renderer for exactly that reason. It is a + * correctness and honesty defect in an audit-facing sentence, which on this + * surface is the more expensive kind. + * + * ## The fix, and why the test is written this way + * + * A replacer FUNCTION. Its return value is used verbatim and has no `$` + * grammar at all, which removes the whole class rather than escaping the + * members of it somebody thought of. + * + * The cases below are the five `$` forms plus a plain control, and each + * asserts the WHOLE sentence rather than that it contains the value: a + * splice adds text without removing the value, so "contains" would pass on + * every one of them. + */ +describe("a translated sentence survives its own parameters", () => { + const t = getServerTranslator("en").t; + // A real key with one placeholder, and it is one of the ones this release + // added — the sharing activity line an owner reads about somebody else. + const KEY = "recordSharing.activityVerb.measurementDelete"; + + it("has a template with exactly one placeholder", () => { + // The pin. A key whose text lost its `{name}` would make every case below + // pass by never substituting anything. + const raw = t(KEY); + expect(raw).toContain("{name}"); + expect(raw.match(/\{name\}/g)).toHaveLength(1); + expect(t(KEY, { name: "Alex" })).toBe("Alex removed a reading"); + }); + + const hostile = [ + { label: "the trailing-context form", value: "$'" }, + { label: "the leading-context form", value: "$`" }, + { label: "the whole-match form", value: "$&" }, + { label: "a capture reference", value: "$1" }, + { label: "an escaped dollar", value: "$$" }, + { label: "a plain name, as the control", value: "Alex" }, + ] as const; + + for (const { label, value } of hostile) { + it(`substitutes ${label} verbatim`, () => { + expect(t(KEY, { name: value })).toBe(`${value} removed a reading`); + }); + } + + it("substitutes every occurrence, and only the named placeholder", () => { + // The `g` flag is load-bearing for keys that name a value twice, and the + // replacer must not change that. + const both = getServerTranslator("en").t("recordSharing.activity.opened", { + name: "$'", + count: 3, + }); + expect(both).toContain("$'"); + expect(both).toContain("3"); + expect(both).not.toContain("{name}"); + expect(both).not.toContain("{count}"); + }); +}); diff --git a/src/lib/i18n/context.tsx b/src/lib/i18n/context.tsx index b46e57dd6..45149980c 100644 --- a/src/lib/i18n/context.tsx +++ b/src/lib/i18n/context.tsx @@ -290,7 +290,16 @@ export function I18nProvider({ // Replace {param} placeholders if (params) { for (const [k, v] of Object.entries(params)) { - value = value.replace(new RegExp(`\\{${k}\\}`, "g"), String(v)); + // A REPLACER FUNCTION, not a replacement string. `String.replace` + // reads `$&`, `$\``, `$'`, `$1` and `$$` out of a replacement + // STRING, so a value carrying one of them rewrites the sentence + // around it: a display name of `$\'` splices everything after the + // placeholder back in, and `$\`` splices everything before it. Every + // interpolated value here is user-controlled — a person's display + // name is the common one, and it lands in sentences on the sharing + // panel that describe who did what. A function replacement is + // substituted verbatim and has no `$` grammar at all. + value = value.replace(new RegExp(`\\{${k}\\}`, "g"), () => String(v)); } } diff --git a/src/lib/i18n/server-translator.ts b/src/lib/i18n/server-translator.ts index 950e4d17f..096d9f8da 100644 --- a/src/lib/i18n/server-translator.ts +++ b/src/lib/i18n/server-translator.ts @@ -24,7 +24,16 @@ export function getServerTranslator(locale: Locale): ServerTranslator { if (value === undefined) return key; if (params) { for (const [k, v] of Object.entries(params)) { - value = value.replace(new RegExp(`\\{${k}\\}`, "g"), String(v)); + // A REPLACER FUNCTION, not a replacement string. `String.replace` + // reads `$&`, `$\``, `$'`, `$1` and `$$` out of a replacement + // STRING, so a value carrying one of them rewrites the sentence + // around it: a display name of `$\'` splices everything after the + // placeholder back in, and `$\`` splices everything before it. Every + // interpolated value here is user-controlled — a person's display + // name is the common one, and it lands in sentences on the sharing + // panel that describe who did what. A function replacement is + // substituted verbatim and has no `$` grammar at all. + value = value.replace(new RegExp(`\\{${k}\\}`, "g"), () => String(v)); } } return value;