mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-11 05:42:19 +00:00
Merge pull request #544 from MBombeck/release/v1.30.20
release/v1.30.20 — refuse a restore whose payload names a different owner
This commit is contained in:
7 files changed
+162
-4
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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`).
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
+1
-1
@@ -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",
|
||||
|
||||
+1
-1
@@ -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/*`
|
||||
|
||||
@@ -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<Response>
|
||||
)(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<Response>
|
||||
)(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);
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user