From 3ffaf1906c383e9cc9ba6069ba7743d5f4de37f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 19 Jul 2026 02:16:54 +0200 Subject: [PATCH] fix(admin): refuse a restore whose payload names a different owner The restore target came from the encrypted payload's own userId field while the audit entry recorded the backup row's userId. A file declaring a different account would therefore have been written into that account, with the trail naming the one the operator selected. Both values are in hand at that point, so compare them and refuse on mismatch, recording both ids. Adds the first test coverage for this route; the mismatch case fails without the guard. Also correct the CLAUDE.md claim that every API route wraps apiHandler - the three MCP OAuth routes deliberately use withBackgroundEvent so they can return RFC-format error bodies, and carry the same protections by hand. chore(release): v1.30.20 --- CHANGELOG.md | 6 + CLAUDE.md | 2 +- docs/api/openapi.yaml | 2 +- package.json | 2 +- public/sw.js | 2 +- .../restore/__tests__/owner-mismatch.test.ts | 129 ++++++++++++++++++ .../api/admin/backups/[id]/restore/route.ts | 23 ++++ 7 files changed, 162 insertions(+), 4 deletions(-) create mode 100644 src/app/api/admin/backups/[id]/restore/__tests__/owner-mismatch.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fa337d5d..0b88eef28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## [Unreleased] +## [1.30.20] — 2026-07-19 + +- **A restore now refuses when the backup file names a different account than the backup record.** The restore target was read from inside the encrypted file rather than from the record the operator selected, so a file claiming a different account would have been written into that account instead — while the audit entry recorded the one the operator picked. Both values were already available at that point; they are now compared, and a mismatch is refused and recorded with both. + +No breaking changes. + ## [1.30.19] — 2026-07-19 Switching a module off now holds everywhere. diff --git a/CLAUDE.md b/CLAUDE.md index ac5b3c414..8481b2002 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -86,7 +86,7 @@ The architecture map in `.planning/codebase/arch.md` walks each layer with file: - **UI work follows the design standards.** `.planning/audits/2026-07-05-fable/UI-STANDARDS.md` is the binding design rulebook (card anatomy via TileHeader, spacing scale, text-colour rules — foreground for content, muted for meta only —, token discipline, primitive mapping). Read it before touching any surface; deviations need a written reason in the PR. Where the standards conflict with older in-file comments, the standards win. - **RSC by default.** Server components are the default; add `"use client"` only when a hook, state, or browser API actually needs it. -- **Every API route wraps in `apiHandler`.** Verified — zero exceptions in the current tree. Every body-accepting route runs Zod `safeParse` and returns 422 via `returnAllZodIssues` (multi-issue envelope sanitised against echoed input through `sanitiseZodIssues`). +- **Every API route wraps in `apiHandler`, with three documented exceptions.** The MCP OAuth routes (`mcp/oauth/{authorize,token,register}`) use `withBackgroundEvent` instead, because OAuth requires RFC-format error bodies that the standard envelope cannot express. They carry the same protections by hand — rate limit, body cap, kill switch, mandatory PKCE-S256, redirect-URI allowlist. Every other route wraps. Every body-accepting route runs Zod `safeParse` and returns 422 via `returnAllZodIssues` (multi-issue envelope sanitised against echoed input through `sanitiseZodIssues`). - **`userId` is always narrowed from session or Bearer.** No route accepts `userId` as a body field — it comes from `requireAuth()` and feeds the Prisma `where`. The batch endpoints (`/api/measurements/batch`, `/api/mood-entries/bulk`, `/api/medications/intake/bulk`, `/api/workouts/batch`) don't even declare a `userId` field in their Zod schemas. - **No mass assignment.** Every `prisma.X.{create,update}({ data: ... })` builds its `data` object field-by-field from `parsed.data`, never by spreading the parsed object whole. `tokens.create` builds `permissions` from a hardcoded literal, never from request input. - **Bearer scope is fail-closed.** `requireAuth()` with no argument admits a cookie session or a wildcard (`["*"]`) token only — a narrow-scope token is refused with 403 and an `undeclared_scope` audit row. A route that means to accept a narrow token names the scope: `requireAuth(FHIR_READ_SCOPE)`. Adding a route therefore cannot silently widen an existing token's reach, because the default arm denies. Only the login / passkey / refresh paths mint `["*"]`; every user-facing mint is narrow, and narrow now means narrow. Two surfaces resolve a Bearer token outside `requireAuth` — `/api/ingest/medication` and `/mcp` — and that set is frozen by a structural test (`src/__tests__/bearer-scope-enforcement-guard.test.ts`). diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index d6f3df023..475928533 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: HealthLog API - version: 1.30.19 + version: 1.30.20 description: >- Self-hosted personal-health-tracking PWA — public API surface for the iOS native client and external ingest. diff --git a/package.json b/package.json index 184e8b6fd..e14f27017 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "healthlog", - "version": "1.30.19", + "version": "1.30.20", "description": "Self-hosted personal-health-tracking PWA with Withings integration, AI insights, and doctor-report PDF export.", "license": "PolyForm-Noncommercial-1.0.0", "homepage": "https://healthlog.dev", diff --git a/public/sw.js b/public/sw.js index f41f407f6..6cd992933 100644 --- a/public/sw.js +++ b/public/sw.js @@ -36,7 +36,7 @@ try { // v1.4.38.4 → v1.4.42. Do not hand-edit; bump `package.json` and rebuild. const CACHE_VERSION = (typeof self !== "undefined" && self.__APP_VERSION__) || - /* @sw-version-fallback */ "v1.30.19"; + /* @sw-version-fallback */ "v1.30.20"; const STATIC_CACHE = `healthlog-static-${CACHE_VERSION}`; const PAGE_CACHE = `healthlog-pages-${CACHE_VERSION}`; // v1.18.6 — read-only data cache for a curated allowlist of safe GET `/api/*` diff --git a/src/app/api/admin/backups/[id]/restore/__tests__/owner-mismatch.test.ts b/src/app/api/admin/backups/[id]/restore/__tests__/owner-mismatch.test.ts new file mode 100644 index 000000000..48ee3186d --- /dev/null +++ b/src/app/api/admin/backups/[id]/restore/__tests__/owner-mismatch.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * A restore writes into the account the BACKUP RECORD names, not the one the + * encrypted payload claims. + * + * The route took its target from `payload.userId` — a field inside the blob — + * while logging `backup.userId` from the row. An admin selecting one user's + * backup could therefore write into a different account, and the audit trail + * would record the account they thought they picked. Both values are in hand at + * that point, so the only correct behaviour is to compare them. + * + * This is inside the admin privilege boundary, so it is an integrity problem + * rather than an escalation: the operator's intent was not honoured. + */ + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: (fn: unknown) => fn, + HttpError: class extends Error { + constructor( + public status: number, + message: string, + ) { + super(message); + } + }, + requireAdmin: vi.fn(async () => ({ user: { id: "admin-1" } })), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + dataBackup: { findUnique: vi.fn() }, + user: { findUnique: vi.fn() }, + }, +})); + +vi.mock("@/lib/crypto", () => ({ decrypt: vi.fn(() => "{}") })); +vi.mock("@/lib/crypto/note-cipher", () => ({ encryptNote: vi.fn() })); +vi.mock("@/lib/validations/backup", () => ({ + parseBackupPayload: vi.fn(), + summarizeBackup: vi.fn(() => ({})), +})); +vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); +vi.mock("@/lib/idempotency", () => ({ + withIdempotency: (fn: unknown) => fn, + defaultUserIdResolver: vi.fn(), +})); +vi.mock("@/lib/rollups/mood-rollups", () => ({ + recomputeUserMoodRollups: vi.fn(), +})); +vi.mock("@/lib/rollups/medication-compliance-rollups", () => ({ + recomputeUserMedicationCompliance: vi.fn(), + MEDICATION_COMPLIANCE_BACKFILL_DAYS: 30, +})); +vi.mock("@/lib/rollups/measurement-rollups", () => ({ + recomputeUserRollups: vi.fn(), +})); + +import { POST } from "../route"; +import { prisma } from "@/lib/db"; +import { parseBackupPayload } from "@/lib/validations/backup"; +import { auditLog } from "@/lib/auth/audit"; + +function request(): NextRequest { + return new NextRequest("http://localhost/api/admin/backups/b-1/restore", { + method: "POST", + headers: { "content-type": "application/json" }, + // The route requires a typed confirmation before it will restore anything. + body: JSON.stringify({ confirm: "RESTORE" }), + }); +} + +const params = Promise.resolve({ id: "b-1" }); + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(prisma.dataBackup.findUnique).mockResolvedValue({ + id: "b-1", + userId: "user-A", + data: "cipher", + user: { id: "user-A", username: "alice" }, + } as never); +}); + +describe("admin backup restore — declared owner", () => { + it("refuses when the payload names a different owner than the record", async () => { + // The blob claims user-B; the backup row says user-A. + vi.mocked(parseBackupPayload).mockReturnValue({ + userId: "user-B", + } as never); + + const res = await ( + POST as unknown as (r: NextRequest, c: unknown) => Promise + )(request(), { params }); + + expect(res.status).toBe(409); + // The restore must not have gone looking for the claimed user at all. + expect(prisma.user.findUnique).not.toHaveBeenCalled(); + // And the refusal is recorded with BOTH ids, so the attempt is reviewable. + expect(auditLog).toHaveBeenCalledWith( + "admin.backups.restore.failed", + expect.objectContaining({ + details: expect.objectContaining({ + reason: "owner_mismatch", + ownerId: "user-A", + declaredOwnerId: "user-B", + }), + }), + ); + }); + + it("proceeds past the check when the two agree", async () => { + vi.mocked(parseBackupPayload).mockReturnValue({ + userId: "user-A", + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue(null as never); + + const res = await ( + POST as unknown as (r: NextRequest, c: unknown) => Promise + )(request(), { params }); + + // Reaching the owner lookup proves the mismatch branch did not fire; this + // case then fails on the deleted-user branch, which is the next guard. + expect(prisma.user.findUnique).toHaveBeenCalled(); + expect(res.status).not.toBe(409); + }); +}); diff --git a/src/app/api/admin/backups/[id]/restore/route.ts b/src/app/api/admin/backups/[id]/restore/route.ts index 81c99dd63..9d553f2cb 100644 --- a/src/app/api/admin/backups/[id]/restore/route.ts +++ b/src/app/api/admin/backups/[id]/restore/route.ts @@ -195,6 +195,29 @@ const handler = apiHandler( return apiError("Backup payload failed schema validation", 500); } + // The payload declares its own owner, and the backup ROW records who the + // backup was taken for. Those must agree. Taking the owner from the + // payload alone means the restored-into account is whatever the blob + // claims — so an admin who selects one user's backup could write into a + // different account without the interface ever showing it. Both values are + // already in hand here; refuse on mismatch rather than trusting the blob. + if (payload.userId !== backup.userId) { + await auditLog("admin.backups.restore.failed", { + userId: admin.id, + ipAddress: getClientIp(request), + details: { + backupId: id, + ownerId: backup.userId, + reason: "owner_mismatch", + declaredOwnerId: payload.userId, + }, + }); + return apiError( + "Backup payload declares a different owner than the backup record", + 409, + ); + } + // The target of the restore is whoever the backup is for, NOT the // admin running the operation. Make sure that user still exists — // an upload referencing a since-deleted user would otherwise leave