mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 12:32:43 +00:00
test: pin three invariants that were fixed but unguarded
Each of these was already correct in the tree and had nothing asserting it, so a refactor would have regressed it silently. The decrypted-snapshot LRU keys on userId, but only same-user memoisation and scope-change recomputation were covered — dropping the userId segment would have served one account's health snapshot to another with a green suite. The project has shipped one cross-user cache-leak fix already; this is a repeat class. The GlitchTip forwarder redacts path-segment secrets in the url it sends. The query-string strip was tested; the path-segment pass was not, and that is the one that reaches a durable external incident UI. The Withings credentials DELETE narrows its connection-row delete to P2025 and rethrows everything else, so a real failure can no longer null the client credentials and report a successful disconnect while the encrypted OAuth tokens survive. The route had no test file at all.
This commit is contained in:
1 parent
94157a50c7
commit
4b87b9e33b
3 files changed
+294
No files matched your search
@@ -0,0 +1,96 @@
|
||||
/**
|
||||
* `DELETE /api/withings/credentials` — disconnect honesty.
|
||||
*
|
||||
* The handler deletes the connection row (holding the encrypted OAuth access +
|
||||
* refresh tokens) before nulling the credential columns. That delete used to be
|
||||
* `.catch(() => {})`: on a real failure the handler carried on, nulled the
|
||||
* client id/secret, and returned `{ deleted: true }` while the token-bearing
|
||||
* row survived — the user is told they are disconnected and they are not.
|
||||
*
|
||||
* It now narrows to P2025 ("already gone", a genuine no-op) and rethrows
|
||||
* everything else. Withings had no test file at all, which is why this one
|
||||
* exists; the WHOOP and Fitbit twins carry the same shape.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
withingsConnection: { delete: vi.fn() },
|
||||
user: { update: vi.fn() },
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() }));
|
||||
vi.mock("@/lib/auth/audit", () => ({
|
||||
auditLog: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() }));
|
||||
vi.mock("@/lib/db-compat", () => ({
|
||||
ensureDbCompatibility: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
vi.mock("next/headers", () => ({
|
||||
headers: vi.fn(async () => ({ get: () => null })),
|
||||
cookies: vi.fn(async () => ({
|
||||
get: () => undefined,
|
||||
set: () => {},
|
||||
delete: () => {},
|
||||
})),
|
||||
}));
|
||||
|
||||
import { DELETE } from "../route";
|
||||
import { prisma } from "@/lib/db";
|
||||
import { getSession } from "@/lib/auth/session";
|
||||
|
||||
const SESSION_OK = {
|
||||
session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) },
|
||||
user: { id: "user-1", username: "tester", role: "USER" as const },
|
||||
};
|
||||
|
||||
/** Shape of a Prisma "record to delete does not exist" rejection. */
|
||||
function p2025() {
|
||||
return Object.assign(new Error("Record to delete does not exist."), {
|
||||
code: "P2025",
|
||||
name: "PrismaClientKnownRequestError",
|
||||
});
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.resetAllMocks();
|
||||
vi.mocked(getSession).mockResolvedValue(SESSION_OK as never);
|
||||
vi.mocked(prisma.user.update).mockResolvedValue({} as never);
|
||||
});
|
||||
|
||||
describe("DELETE /api/withings/credentials", () => {
|
||||
it("disconnects and nulls the credential columns on the happy path", async () => {
|
||||
vi.mocked(prisma.withingsConnection.delete).mockResolvedValue({} as never);
|
||||
const res = await DELETE();
|
||||
expect(res.status).toBe(200);
|
||||
expect(prisma.user.update).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
data: {
|
||||
withingsClientIdEncrypted: null,
|
||||
withingsClientSecretEncrypted: null,
|
||||
},
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("treats an already-missing connection row (P2025) as a benign no-op", async () => {
|
||||
vi.mocked(prisma.withingsConnection.delete).mockRejectedValue(p2025());
|
||||
const res = await DELETE();
|
||||
expect(res.status).toBe(200);
|
||||
// Idempotent disconnect still clears the credentials.
|
||||
expect(prisma.user.update).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does NOT report success when the token-bearing row fails to delete", async () => {
|
||||
vi.mocked(prisma.withingsConnection.delete).mockRejectedValue(
|
||||
new Error("connection terminated unexpectedly"),
|
||||
);
|
||||
const res = await DELETE();
|
||||
expect(res.status).toBe(500);
|
||||
// Critically: the credentials must NOT be nulled, because doing so while
|
||||
// the encrypted OAuth tokens survive is the worst of both states.
|
||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,118 @@
|
||||
/**
|
||||
* The GlitchTip forwarder must redact path-segment secrets in `url`.
|
||||
*
|
||||
* `reportToGlitchtip` strips the query string, which covers `?secret=…` and
|
||||
* `?code=…&state=…`. It does NOT cover a secret carried as a path SEGMENT —
|
||||
* `/api/withings/webhook/<secret>`, `/api/whoop/webhook/<secret>` — and those
|
||||
* are exactly the shapes `PATH_SECRET_PATHS` exists for. Every other sink (the
|
||||
* stdout wide event, the admin app-logs render) already redacts them; GlitchTip
|
||||
* was the one gap, and it is the worst one, because the value lands durably in
|
||||
* an external incident UI.
|
||||
*
|
||||
* The fix is in place. This pins it: the assertion is on the wiring, not on
|
||||
* `redactSecrets` itself, because the defect was a missing call, not a broken
|
||||
* redactor.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
apiToken: { findUnique: vi.fn(), update: vi.fn() },
|
||||
user: { findUnique: vi.fn() },
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() }));
|
||||
vi.mock("@/lib/auth/audit", () => ({
|
||||
auditLog: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() }));
|
||||
vi.mock("@/lib/db-compat", () => ({
|
||||
ensureDbCompatibility: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/monitoring-settings", () => ({
|
||||
getGlitchtipSettings: vi.fn(async () => ({
|
||||
glitchtipEnabled: true,
|
||||
glitchtipDsn: "https://key@glitchtip.example/1",
|
||||
glitchtipEnvironment: "test",
|
||||
})),
|
||||
}));
|
||||
|
||||
const sendGlitchtipEvent = vi.fn(async (_payload: unknown) => {});
|
||||
vi.mock("@/lib/monitoring/glitchtip", () => ({
|
||||
sendGlitchtipEvent: (payload: unknown) => sendGlitchtipEvent(payload),
|
||||
}));
|
||||
|
||||
vi.mock("next/headers", () => ({
|
||||
headers: vi.fn(async () => ({ get: () => null })),
|
||||
cookies: vi.fn(async () => ({
|
||||
get: () => undefined,
|
||||
set: () => {},
|
||||
delete: () => {},
|
||||
})),
|
||||
}));
|
||||
|
||||
import { NextRequest } from "next/server";
|
||||
import { apiHandler } from "../api-handler";
|
||||
|
||||
const WEBHOOK_SECRET = "s3cr3t-webhook-path-segment-value";
|
||||
|
||||
beforeEach(() => {
|
||||
sendGlitchtipEvent.mockClear();
|
||||
});
|
||||
|
||||
function throwingHandler() {
|
||||
return apiHandler(async (_request: NextRequest) => {
|
||||
throw new Error("boom");
|
||||
});
|
||||
}
|
||||
|
||||
async function forwardedPayload(url: string) {
|
||||
const GET = throwingHandler();
|
||||
await GET(new NextRequest(url, { method: "GET" }));
|
||||
// The forwarder is fire-and-forget inside the handler's catch; let its
|
||||
// dynamic imports and the send settle before asserting.
|
||||
for (let i = 0; i < 20; i++) await Promise.resolve();
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
expect(sendGlitchtipEvent).toHaveBeenCalledTimes(1);
|
||||
const call = sendGlitchtipEvent.mock.calls[0]?.[0] as {
|
||||
input: { url: string };
|
||||
};
|
||||
return call.input;
|
||||
}
|
||||
|
||||
describe("GlitchTip forwarder — path-segment secret redaction", () => {
|
||||
it("does not forward a Withings webhook path secret verbatim", async () => {
|
||||
const input = await forwardedPayload(
|
||||
`http://localhost/api/withings/webhook/${WEBHOOK_SECRET}`,
|
||||
);
|
||||
expect(input.url).not.toContain(WEBHOOK_SECRET);
|
||||
});
|
||||
|
||||
it("does not forward a WHOOP webhook path secret verbatim", async () => {
|
||||
const input = await forwardedPayload(
|
||||
`http://localhost/api/whoop/webhook/${WEBHOOK_SECRET}`,
|
||||
);
|
||||
expect(input.url).not.toContain(WEBHOOK_SECRET);
|
||||
});
|
||||
|
||||
it("does not forward a clinician share token verbatim", async () => {
|
||||
const shareToken = "hls_abcdef0123456789abcdef0123456789";
|
||||
const input = await forwardedPayload(`http://localhost/c/${shareToken}`);
|
||||
expect(input.url).not.toContain(shareToken);
|
||||
});
|
||||
|
||||
it("still strips the query string", async () => {
|
||||
const input = await forwardedPayload(
|
||||
"http://localhost/api/auth/oidc/callback?code=abc123&state=xyz789",
|
||||
);
|
||||
expect(input.url).not.toContain("abc123");
|
||||
expect(input.url).not.toContain("xyz789");
|
||||
});
|
||||
|
||||
it("leaves an ordinary path readable", async () => {
|
||||
const input = await forwardedPayload("http://localhost/api/measurements");
|
||||
expect(input.url).toContain("/api/measurements");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,80 @@
|
||||
/**
|
||||
* Cross-tenant isolation of the decrypted-snapshot LRU.
|
||||
*
|
||||
* The cache holds a fully-assembled health snapshot — one account's
|
||||
* measurements, mood, medication intake — for 60 seconds, keyed by
|
||||
* `${userId}|${window}|${sourceList}`. `userId` IS in the key today, so there
|
||||
* is no live defect. The gap this file closes is that nothing PINNED it: the
|
||||
* existing suite covers same-user memoisation and scope-change recomputation,
|
||||
* so a refactor dropping the `userId` segment would serve one user's snapshot
|
||||
* to another with a fully green run.
|
||||
*
|
||||
* The project has already shipped one cross-user cache-leak fix (v1.25.1), so
|
||||
* this is a repeat class, not a hypothetical one.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it } from "vitest";
|
||||
|
||||
import {
|
||||
__resetCoachSnapshotCacheForTests,
|
||||
readSnapshotCache,
|
||||
snapshotCacheKey,
|
||||
writeSnapshotCache,
|
||||
} from "../snapshot-cache";
|
||||
import type { CoachSnapshotResult } from "../snapshot";
|
||||
import type { CoachScope } from "../types";
|
||||
|
||||
function fakeResult(marker: string): CoachSnapshotResult {
|
||||
return { snapshotJson: marker } as unknown as CoachSnapshotResult;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
__resetCoachSnapshotCacheForTests();
|
||||
});
|
||||
|
||||
describe("snapshot cache — tenant isolation", () => {
|
||||
it("never returns user A's snapshot to user B at identical scope", () => {
|
||||
const scope: CoachScope = { window: "last30days" };
|
||||
|
||||
const keyA = snapshotCacheKey("user-a", scope);
|
||||
const keyB = snapshotCacheKey("user-b", scope);
|
||||
|
||||
// The keys must differ in the first place — this is the invariant.
|
||||
expect(keyA).not.toBe(keyB);
|
||||
|
||||
writeSnapshotCache(keyA, fakeResult("A-PRIVATE"));
|
||||
|
||||
// B, at the very same scope, must miss.
|
||||
expect(readSnapshotCache(keyB)).toBeNull();
|
||||
|
||||
// And once B has its own entry, the two never cross.
|
||||
writeSnapshotCache(keyB, fakeResult("B-PRIVATE"));
|
||||
expect(readSnapshotCache(keyA)?.snapshotJson).toBe("A-PRIVATE");
|
||||
expect(readSnapshotCache(keyB)?.snapshotJson).toBe("B-PRIVATE");
|
||||
});
|
||||
|
||||
it("keeps the userId segment leading, so no scope string can forge another tenant", () => {
|
||||
// A source list is user-controllable in the request. It must not be able to
|
||||
// reconstruct a different user's key.
|
||||
const forged = snapshotCacheKey("user-a", {
|
||||
window: "last30days",
|
||||
sources: ["user-b"],
|
||||
} as unknown as CoachScope);
|
||||
const victim = snapshotCacheKey("user-b", { window: "last30days" });
|
||||
expect(forged).not.toBe(victim);
|
||||
expect(forged.startsWith("user-a|")).toBe(true);
|
||||
});
|
||||
|
||||
it("isolates identical explicit source lists across users", () => {
|
||||
const scope: CoachScope = {
|
||||
window: "last7days",
|
||||
sources: ["MANUAL", "APPLE_HEALTH"],
|
||||
} as unknown as CoachScope;
|
||||
|
||||
const keyA = snapshotCacheKey("user-a", scope);
|
||||
const keyB = snapshotCacheKey("user-b", scope);
|
||||
expect(keyA).not.toBe(keyB);
|
||||
|
||||
writeSnapshotCache(keyA, fakeResult("A-PRIVATE"));
|
||||
expect(readSnapshotCache(keyB)).toBeNull();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user