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