diff --git a/e2e/v137-sharing-managed-profiles.spec.ts b/e2e/v137-sharing-managed-profiles.spec.ts index 016bb3f76..2a2a503d2 100644 --- a/e2e/v137-sharing-managed-profiles.spec.ts +++ b/e2e/v137-sharing-managed-profiles.spec.ts @@ -51,6 +51,45 @@ async function leaveRecord(page: Page) { ); } +function withDivergentActiveAccountAccess(body: unknown): unknown { + if (body === null || typeof body !== "object") { + throw new Error("Expected an object from /api/auth/me"); + } + + const envelope = body as { data?: unknown }; + const payload = "data" in envelope ? envelope.data : envelope; + if (payload === null || typeof payload !== "object") { + throw new Error("Expected an auth payload from /api/auth/me"); + } + + const auth = payload as { + accountAccess?: { + active?: { + access?: unknown; + level?: unknown; + canWrite?: unknown; + } | null; + }; + }; + if (!auth.accountAccess?.active) { + throw new Error("Expected a switched account-access payload"); + } + + const corrupted = { + ...auth, + accountAccess: { + ...auth.accountAccess, + active: { + ...auth.accountAccess.active, + access: "write", + level: "write", + canWrite: true, + }, + }, + }; + return "data" in envelope ? { ...envelope, data: corrupted } : corrupted; +} + test.describe.serial("scoped sharing browser journeys", () => { test.use({ storageState: SCOPE_DELEGATE_STORAGE_STATE_PATH }); @@ -99,6 +138,76 @@ test.describe.serial("scoped sharing browser journeys", () => { }); } + test("refuses a malformed switched payload before owner controls or reads mount", async ({ + page, + }) => { + await page.goto("/"); + await openSwitcher(page); + + const entry = page.locator( + '[data-slot="account-switcher-entry"][data-account-username="e2e-scope-labs"]', + ); + await expect(entry).toHaveCount(1); + + let corruptNextAuthPayload = true; + await page.route("**/api/auth/me", async (route) => { + const response = await route.fetch(); + if (!corruptNextAuthPayload) { + await route.fulfill({ response }); + return; + } + corruptNextAuthPayload = false; + await route.fulfill({ + response, + json: withDivergentActiveAccountAccess(await response.json()), + }); + }); + + const ownerOnlyReads: string[] = []; + const trackOwnerOnlyRead = (request: { + method(): string; + url(): string; + }) => { + if (request.method() !== "GET") return; + const path = new URL(request.url()).pathname; + if ( + [ + "/api/labs/ocr/capability", + "/api/insights/coach/nudge-status", + "/api/coach/about-me/questions", + "/api/auth/me/notification-prefs", + ].includes(path) + ) { + ownerOnlyReads.push(path); + } + }; + page.on("request", trackOwnerOnlyRead); + try { + await entry.click(); + + await expect(page).toHaveURL(/\/labs$/); + await expect( + page.locator('[data-slot="invalid-record-access-refusal"]'), + ).toBeVisible(); + await expect( + page.locator('[data-slot="shared-record-unavailable-leave"]'), + ).toBeVisible(); + await expect(page.locator('[data-tour-id="labs-hero"]')).toHaveCount(0); + await expect( + page.locator('[data-slot="shared-record-banner"]'), + ).toHaveCount(0); + expect(ownerOnlyReads).toEqual([]); + + await page + .locator('[data-slot="shared-record-unavailable-leave"]') + .click(); + await expect(page).toHaveURL(/\/$/); + } finally { + page.off("request", trackOwnerOnlyRead); + await page.unroute("**/api/auth/me"); + } + }); + test("keeps an adult READ grant read-only and outside Settings", async ({ page, }) => { diff --git a/src/app/coach/__tests__/page-launch-params.test.tsx b/src/app/coach/__tests__/page-launch-params.test.tsx index 13c42e388..3f56c28c8 100644 --- a/src/app/coach/__tests__/page-launch-params.test.tsx +++ b/src/app/coach/__tests__/page-launch-params.test.tsx @@ -44,6 +44,12 @@ vi.mock("@/hooks/use-feature-flags", () => ({ vi.mock("@/hooks/use-disable-coach", () => ({ useDisableCoach: () => false, })); +vi.mock("@/hooks/use-auth", () => ({ + useAuth: () => ({ user: {}, isLoading: false }), +})); +vi.mock("@/hooks/use-record-capabilities", () => ({ + useRecordCapabilities: () => ({ inSharedRecord: false }), +})); vi.mock("@tanstack/react-query", () => ({ useQuery: () => ({ data: undefined }), })); diff --git a/src/app/coach/page-client.tsx b/src/app/coach/page-client.tsx index db99c0c42..0b8bb28ce 100644 --- a/src/app/coach/page-client.tsx +++ b/src/app/coach/page-client.tsx @@ -10,6 +10,8 @@ import type { CoachLaunchScope } from "@/lib/insights/coach-launch-context"; import { useCoachLaunch } from "@/lib/insights/coach-launch-context"; import { useFeatureFlags } from "@/hooks/use-feature-flags"; import { useDisableCoach } from "@/hooks/use-disable-coach"; +import { useAuth } from "@/hooks/use-auth"; +import { useRecordCapabilities } from "@/hooks/use-record-capabilities"; import { queryKeys } from "@/lib/query-keys"; import { apiGet } from "@/lib/api/api-fetch"; import { coachScopeSourceSchema } from "@/lib/ai/coach/types"; @@ -163,6 +165,8 @@ export default function CoachPageClient() { const launch = useCoachLaunch(); const flags = useFeatureFlags(); const disableCoach = useDisableCoach(); + const { user, isLoading } = useAuth(); + const { inSharedRecord } = useRecordCapabilities(); const coachUnavailable = !flags.coach || disableCoach; @@ -170,11 +174,13 @@ export default function CoachPageClient() { // operator master flag OR per-user opt-out hides the Coach entirely. // Send a direct navigator back to the Insights mother page so the // route is never a dead-end. + const canRenderOwnCoach = user !== null && !isLoading && !inSharedRecord; + useEffect(() => { - if (coachUnavailable) { + if (canRenderOwnCoach && coachUnavailable) { router.replace("/insights"); } - }, [coachUnavailable, router]); + }, [canRenderOwnCoach, coachUnavailable, router]); // Reopening the bottom-right FAB drawer is no longer wired to a page // control, but keep the launch context referenced so the lint rule does @@ -182,7 +188,7 @@ export default function CoachPageClient() { // the shared ``. void launch; - if (coachUnavailable) return null; + if (!canRenderOwnCoach || coachUnavailable) return null; return (
({ useAuth: () => ({ @@ -63,6 +66,7 @@ vi.mock("@/hooks/use-auth", () => ({ avatarUrl: null, modules: {}, accountAccess: mockAccessRef.value, + accountAccessStatus: mockAccountAccessStatusRef.value, }, isAuthenticated: true, isAuthUnknown: false, @@ -138,9 +142,14 @@ const MANAGED_GUARDIAN: AccountAccessEntry = { canWrite: true, }; -function render(access: AccountAccess, pathname = "/"): string { +function render( + access: AccountAccess, + pathname = "/", + accountAccessStatus: "absent" | "valid" | "invalid" = "valid", +): string { mockAccessRef.value = access; mockPathRef.value = pathname; + mockAccountAccessStatusRef.value = accountAccessStatus; return renderToStaticMarkup( @@ -205,6 +214,18 @@ describe(" — the owner-only doors", () => { } }); + it("refuses an invalid record block without mounting owner-only doors or children", () => { + const html = render(OWN_RECORD, "/", "invalid"); + + expect(html).toContain("sentinel-unavailable"); + expect(html).not.toContain("sentinel-page"); + for (const slot of DOORS) { + expect(html, `${slot} must not paint for a refused record`).not.toContain( + slot, + ); + } + }); + it.each([ ["read", OWNER_READ], ["write", OWNER_WRITE], diff --git a/src/components/layout/auth-shell.tsx b/src/components/layout/auth-shell.tsx index e8a56cb95..f913d29e3 100644 --- a/src/components/layout/auth-shell.tsx +++ b/src/components/layout/auth-shell.tsx @@ -79,7 +79,8 @@ export function AuthShell({ // decides what to mount and the surfaces beneath it decide what to offer, // and the two disagreeing is the whole class of defect this hook exists to // close. - const { inSharedRecord, recordKind, sections } = useRecordCapabilities(); + const { inSharedRecord, recordKind, sections, accessRefused } = + useRecordCapabilities(); // A managed profile has a deliberately small Settings surface of its own. // Let that route family reach its record-aware gate; adult shared records // stay in the generic refusal branch even at MANAGE. @@ -245,6 +246,23 @@ export function AuthShell({ ); } + // A present but malformed access block is not the older-server case. The + // server may still be switched, so mounting children here could fetch target + // data under owner controls. Offer only the actor-scoped way out. + if (accessRefused) { + return ( +
+ +
+ +
+
+ ); + } + // Onboarding page — minimal shell, no sidebar/nav if (isOnboardingPage) { return ( diff --git a/src/hooks/__tests__/use-auth-account-access-state.test.ts b/src/hooks/__tests__/use-auth-account-access-state.test.ts new file mode 100644 index 000000000..b9ec29a73 --- /dev/null +++ b/src/hooks/__tests__/use-auth-account-access-state.test.ts @@ -0,0 +1,106 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { + __resetRecordScopeForTests, + getRecordScope, + isReadingSharedRecord, +} from "@/lib/query-keys/record-scope"; + +const apiGet = vi.fn(); + +vi.mock("@/lib/api/api-fetch", () => ({ + apiGet: (...args: unknown[]) => apiGet(...args), + apiFetchRaw: vi.fn(), + ApiError: class ApiError extends Error {}, +})); + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ push: vi.fn(), replace: vi.fn() }), +})); + +import { fetchMe, NO_ACCOUNT_ACCESS } from "../use-auth"; + +const response = { + id: "delegate", + username: "delegate", + role: "USER", + timezone: "Europe/Berlin", +}; + +const canonicalEntry = { + accountId: "record-a", + username: "record-a", + displayName: "Record A", + access: "read", + level: "read", + recordKind: "shared", + sections: ["labs"], + canWrite: false, +}; + +beforeEach(() => { + apiGet.mockReset(); + __resetRecordScopeForTests(); +}); + +describe("fetchMe account-access resolution", () => { + it("keeps a genuinely absent block compatible with an older server", async () => { + apiGet.mockResolvedValue(response); + + const user = await fetchMe(); + + expect(user.accountAccessStatus).toBe("absent"); + expect(user.accountAccess).toEqual(NO_ACCOUNT_ACCESS); + expect(getRecordScope()).toBeNull(); + expect(isReadingSharedRecord()).toBe(false); + }); + + it("uses a valid published block as the active record scope", async () => { + apiGet.mockResolvedValue({ + ...response, + accountAccess: { + accounts: [canonicalEntry], + active: canonicalEntry, + recordKind: "shared", + canSwitch: true, + }, + }); + + const user = await fetchMe(); + + expect(user.accountAccessStatus).toBe("valid"); + expect(user.accountAccess?.active).toEqual(canonicalEntry); + expect(getRecordScope()).toBe("record-a"); + }); + + it.each([ + ["an explicitly undefined block", undefined], + ["a null block", null], + [ + "a divergent active entry", + { + accounts: [canonicalEntry], + active: { + ...canonicalEntry, + access: "write", + level: "write", + canWrite: true, + }, + recordKind: "shared", + canSwitch: true, + }, + ], + ])( + "refuses %s without restoring an owner cache scope", + async (_name, block) => { + apiGet.mockResolvedValue({ ...response, accountAccess: block }); + + const user = await fetchMe(); + + expect(user.accountAccessStatus).toBe("invalid"); + expect(user.accountAccess).toEqual(NO_ACCOUNT_ACCESS); + expect(getRecordScope()).not.toBeNull(); + expect(isReadingSharedRecord()).toBe(true); + }, + ); +}); diff --git a/src/hooks/__tests__/use-record-capabilities.test.ts b/src/hooks/__tests__/use-record-capabilities.test.ts index 90dc83abc..1b4600950 100644 --- a/src/hooks/__tests__/use-record-capabilities.test.ts +++ b/src/hooks/__tests__/use-record-capabilities.test.ts @@ -72,6 +72,19 @@ describe("resolveRecordCapabilities", () => { ); }); + it("refuses a malformed published block instead of treating it as own record", () => { + expect(resolveRecordCapabilities(null, true)).toEqual({ + inSharedRecord: true, + canWrite: false, + canAdd: false, + canManage: false, + level: null, + sections: [], + recordKind: "shared", + accessRefused: true, + }); + }); + it("a read-only delegate adds nothing", () => { expect(resolveRecordCapabilities(READ_ONLY)).toEqual({ inSharedRecord: true, diff --git a/src/hooks/use-auth.ts b/src/hooks/use-auth.ts index 18899c26c..453476e0d 100644 --- a/src/hooks/use-auth.ts +++ b/src/hooks/use-auth.ts @@ -23,7 +23,10 @@ import { storeTimezone } from "@/lib/timezone-mirror"; import type { ModuleKey } from "@/lib/modules/registry"; import type { TourProgress } from "@/lib/onboarding/tour-progress"; import { clearOfflineCachesForSessionEnd } from "@/lib/pwa/query-persister"; -import { setRecordScope } from "@/lib/query-keys/record-scope"; +import { + setRecordScope, + setRefusedRecordScope, +} from "@/lib/query-keys/record-scope"; import type { AccountAccess } from "@/lib/sharing/account-access-view"; import { parseAccountAccess } from "@/lib/sharing/account-access-schema"; @@ -39,6 +42,9 @@ export const NO_ACCOUNT_ACCESS: AccountAccess = { canSwitch: false, }; +/** Whether the account-access block was absent, valid, or refused as malformed. */ +export type AccountAccessStatus = "absent" | "valid" | "invalid"; + /** * v1.18.6 — resume point for the module tour as the client sees it on * `/api/auth/me`. Mirrors the server-side `TourProgress` shape. @@ -193,6 +199,13 @@ export interface AuthUser { * the empty one — no accounts, no active record, no switcher. */ accountAccess?: AccountAccess; + /** + * The parser outcome for `accountAccess`. An absent block is the only + * owner-compatible fallback: it is how older server images answered before + * sharing existed. A present malformed block stays explicitly refused so a + * server-side switch cannot paint owner controls in the target record. + */ + accountAccessStatus?: AccountAccessStatus; } /** @@ -253,7 +266,7 @@ export function isAuthVerdictUnknown(error: unknown): boolean { return true; } -async function fetchMe(): Promise { +export async function fetchMe(): Promise { // v1.16.4 — routed through the typed wrapper; a non-OK /me (401) // throws `ApiError`, which `useAuth` treats as "not authenticated" // exactly like the old hand-rolled throw. @@ -300,8 +313,13 @@ async function fetchMe(): Promise { // whatever the switch flow stamped before its reload, the block is what the // resolver will actually honour on the next request, so the mirror tracks it // rather than the client's memory of what it asked for. - const accountAccess = coerceAccountAccess(data.accountAccess); - setRecordScope(accountAccess.active?.accountId ?? null); + const { accountAccess, status: accountAccessStatus } = + resolveAccountAccess(data); + if (accountAccessStatus === "invalid") { + setRefusedRecordScope(); + } else { + setRecordScope(accountAccess.active?.accountId ?? null); + } return { ...(data as AuthUser), disableCoach: data.disableCoach ?? false, @@ -329,21 +347,32 @@ async function fetchMe(): Promise { ? data.moduleAvailability : {}, accountAccess, + accountAccessStatus, }; } /** * Read the sharing block off a payload that may not carry one. * - * Fails CLOSED, unlike the module maps above: an older server image (or a - * malformed block) resolves to no accounts, no active record, and no switcher. - * The module gates fail open because a missing map must never blank the nav; - * this one fails closed because the affordance it drives asks the server to - * open somebody else's health record, and an affordance offered on a server - * that cannot honour it is worse than one that is missing. + * A genuinely missing field is the pre-sharing server contract and remains an + * own-record fallback. Once a server publishes the field, every other value — + * including `undefined` as an own property and `null` — is a malformed block, + * not permission to fall back to the caller's own presentation. */ -function coerceAccountAccess(value: unknown): AccountAccess { - return parseAccountAccess(value) ?? NO_ACCOUNT_ACCESS; +function resolveAccountAccess(value: object): { + accountAccess: AccountAccess; + status: AccountAccessStatus; +} { + if (!Object.hasOwn(value, "accountAccess")) { + return { accountAccess: NO_ACCOUNT_ACCESS, status: "absent" }; + } + + const accountAccess = parseAccountAccess( + (value as { accountAccess?: unknown }).accountAccess, + ); + return accountAccess + ? { accountAccess, status: "valid" } + : { accountAccess: NO_ACCOUNT_ACCESS, status: "invalid" }; } export function useAuth() { diff --git a/src/hooks/use-record-capabilities.ts b/src/hooks/use-record-capabilities.ts index 53219288d..8c536b850 100644 --- a/src/hooks/use-record-capabilities.ts +++ b/src/hooks/use-record-capabilities.ts @@ -71,6 +71,8 @@ import { * search a button in gets a sentence. */ export interface RecordCapabilities { + /** A present account-access block was malformed and must not mount a record. */ + accessRefused?: boolean; /** Is this browser acting on somebody else's record right now. */ inSharedRecord: boolean; /** The grant's resolved level for the record on screen. */ @@ -111,7 +113,20 @@ export interface RecordCapabilities { */ export function resolveRecordCapabilities( active: AccountAccessEntry | null | undefined, + accessRefused = false, ): RecordCapabilities { + if (accessRefused) { + return { + accessRefused: true, + inSharedRecord: true, + canWrite: false, + canAdd: false, + canManage: false, + level: null, + sections: [], + recordKind: "shared", + }; + } if (!active) { return { inSharedRecord: false, @@ -156,7 +171,10 @@ export function resolveRecordCapabilities( */ export function useRecordCapabilities(): RecordCapabilities { const { user } = useAuth(); - return resolveRecordCapabilities(user?.accountAccess?.active); + return resolveRecordCapabilities( + user?.accountAccess?.active, + user?.accountAccessStatus === "invalid", + ); } /** diff --git a/src/lib/query-keys/__tests__/record-scope.test.ts b/src/lib/query-keys/__tests__/record-scope.test.ts index befe0bc56..79d85b24b 100644 --- a/src/lib/query-keys/__tests__/record-scope.test.ts +++ b/src/lib/query-keys/__tests__/record-scope.test.ts @@ -21,6 +21,7 @@ import { getRecordScope, isReadingSharedRecord, recordScopedQueryKeyHashFn, + setRefusedRecordScope, setRecordScope, } from "@/lib/query-keys/record-scope"; @@ -101,6 +102,24 @@ describe("the cache cannot serve one record's data while reading another", () => expect(client.getQueryData(queryKeys.dashboardSnapshot())).toBeUndefined(); }); + it("never puts data from a refused record into the caller's own cache slot", () => { + const client = appClient(); + client.setQueryData(queryKeys.dashboardSnapshot(), { weightKg: 71 }); + + setRefusedRecordScope(); + + expect(isReadingSharedRecord()).toBe(true); + expect(client.getQueryData(queryKeys.dashboardSnapshot())).toBeUndefined(); + }); + + it("keeps the auth identity reachable when a refused record changes scope", () => { + const ownAuthHash = recordScopedQueryKeyHashFn(queryKeys.authMe()); + + setRefusedRecordScope(); + + expect(recordScopedQueryKeyHashFn(queryKeys.authMe())).toBe(ownAuthHash); + }); + it("keeps two records' entries apart at the same time", () => { const client = appClient(); setRecordScope("acct-a"); diff --git a/src/lib/query-keys/record-scope.ts b/src/lib/query-keys/record-scope.ts index 93468e317..bad85d65c 100644 --- a/src/lib/query-keys/record-scope.ts +++ b/src/lib/query-keys/record-scope.ts @@ -52,6 +52,7 @@ import type { QueryKey } from "@tanstack/react-query"; const STORAGE_KEY = "healthlog-record-scope"; +const REFUSED_RECORD_SCOPE = "__healthlog_refused_record__"; /** * The in-process copy. Seeded from storage on first read so the hash function @@ -113,6 +114,18 @@ export function setRecordScope(accountId: string | null): void { } } +/** + * Keep a malformed active-record response out of the caller's own cache slot. + * + * No record id from an invalid payload is trusted, but `null` would make + * record-scoped reads share the caller's own cache identity. This sentinel is + * a cache partition only; the shell refuses the presentation until the caller + * leaves the server-side switch. + */ +export function setRefusedRecordScope(): void { + setRecordScope(REFUSED_RECORD_SCOPE); +} + /** True while this browser is reading somebody else's record. */ export function isReadingSharedRecord(): boolean { return getRecordScope() !== null; @@ -138,6 +151,10 @@ export function recordScopedQueryKeyHashFn(queryKey: QueryKey): string { }, {}) : val, ); + // `/api/auth/me` identifies the caller, never the record they are reading. + // Its query begins before `fetchMe` can resolve or refuse the record scope, + // so it must remain reachable across that transition. + if (queryKey[0] === "auth") return key; const record = getRecordScope(); return record === null ? key : `record:${record}|${key}`; }