diff --git a/src/app/api/notifications/web-push/__tests__/route.test.ts b/src/app/api/notifications/web-push/__tests__/route.test.ts new file mode 100644 index 000000000..516cd7838 --- /dev/null +++ b/src/app/api/notifications/web-push/__tests__/route.test.ts @@ -0,0 +1,174 @@ +/** + * `/api/notifications/web-push` — the refusal has to say which rule was broken. + * + * Both handlers used to answer a flat `apiError("Invalid data", 422)` and throw + * the Zod issues away. A non-HTTPS endpoint, an endpoint aimed at an internal + * host, an over-long one and a missing subscription key all produced the same + * three words, so a browser whose subscription was refused could not tell a + * fixable mistake from a policy it would never satisfy. + * + * The cases below assert on CONTENT rather than status. A status-only test + * passes just as happily against the flat error it replaces, which is the whole + * reason the defect survived this long. + * + * The other half is what must NOT come back. The endpoint is the routing secret + * for a subscription and `keys` is crypto material, so the last test pins that + * neither reaches the wire: `sanitiseZodIssues` emits `path`, `code` and + * `message` only, and every message this schema can produce is a fixed string + * or a length/format default rather than an interpolation of the value. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: (fn: unknown) => fn, + requireAuth: vi.fn(), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + pushSubscription: { upsert: vi.fn(), deleteMany: vi.fn() }, + notificationChannel: { findFirst: vi.fn(), create: vi.fn() }, + }, +})); + +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); + +vi.mock("@/lib/crypto", () => ({ encrypt: (v: string) => `enc(${v})` })); + +import { POST, DELETE } from "../route"; +import { requireAuth } from "@/lib/api-handler"; +import { prisma } from "@/lib/db"; + +const USER = { id: "user-1", username: "u" }; + +/** A well-formed subscription, for the cases that should succeed. */ +const GOOD = { + endpoint: "https://fcm.googleapis.com/fcm/send/abcdef-routing-secret", + keys: { p256dh: "BPkey", auth: "authsecret" }, +}; + +type IssueEnvelope = { + error: string; + details?: { + issues?: Array<{ path?: string; code?: string; message?: string }>; + }; +}; + +function req(body: unknown): Request { + return new Request("http://localhost/api/notifications/web-push", { + method: "POST", + body: JSON.stringify(body), + headers: { "Content-Type": "application/json" }, + }); +} + +type Handler = (r: Request) => Promise; + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(requireAuth).mockResolvedValue({ user: USER } as never); +}); + +describe("POST /api/notifications/web-push — the subscribe refusal", () => { + it("names the offending field for a plain-http endpoint", async () => { + const res = await (POST as unknown as Handler)( + req({ ...GOOD, endpoint: "http://fcm.googleapis.com/fcm/send/x" }), + ); + + expect(res.status).toBe(422); + const env = (await res.json()) as IssueEnvelope; + expect(env.error).toBe("Validation failed"); + expect(env.details?.issues?.length).toBeGreaterThan(0); + expect(env.details?.issues?.some((i) => i.path === "endpoint")).toBe(true); + expect(prisma.pushSubscription.upsert).not.toHaveBeenCalled(); + }); + + it("names the offending field for a missing subscription key", async () => { + const res = await (POST as unknown as Handler)( + req({ endpoint: GOOD.endpoint, keys: { p256dh: "BPkey" } }), + ); + + expect(res.status).toBe(422); + const env = (await res.json()) as IssueEnvelope; + expect(env.details?.issues?.some((i) => i.path === "keys.auth")).toBe(true); + expect(prisma.pushSubscription.upsert).not.toHaveBeenCalled(); + }); + + it("tells an internal-host endpoint apart from an http one", async () => { + // The two are different problems with different fixes — one is a client + // bug, the other is a subscription this instance will never accept. Under + // the flat error the bodies were byte-identical. + const insecure = await (POST as unknown as Handler)( + req({ ...GOOD, endpoint: "http://fcm.googleapis.com/fcm/send/x" }), + ); + const internal = await (POST as unknown as Handler)( + req({ ...GOOD, endpoint: "https://192.168.1.10/push/x" }), + ); + + expect(insecure.status).toBe(422); + expect(internal.status).toBe(422); + expect(JSON.stringify(await insecure.json())).not.toBe( + JSON.stringify(await internal.json()), + ); + }); + + it("still stores a well-formed subscription", async () => { + vi.mocked(prisma.notificationChannel.findFirst).mockResolvedValue( + null as never, + ); + vi.mocked(prisma.pushSubscription.upsert).mockResolvedValue({} as never); + vi.mocked(prisma.notificationChannel.create).mockResolvedValue({} as never); + + const res = await (POST as unknown as Handler)(req(GOOD)); + + expect(res.status).toBe(200); + expect(prisma.pushSubscription.upsert).toHaveBeenCalledTimes(1); + }); +}); + +describe("DELETE /api/notifications/web-push — the unsubscribe refusal", () => { + it("names the offending field rather than answering flat", async () => { + const res = await (DELETE as unknown as Handler)( + req({ endpoint: "not-a-url" }), + ); + + expect(res.status).toBe(422); + const env = (await res.json()) as IssueEnvelope; + expect(env.error).toBe("Validation failed"); + expect(env.details?.issues?.some((i) => i.path === "endpoint")).toBe(true); + expect(prisma.pushSubscription.deleteMany).not.toHaveBeenCalled(); + }); + + it("distinguishes a missing endpoint from a malformed one", async () => { + const missing = await (DELETE as unknown as Handler)(req({})); + const malformed = await (DELETE as unknown as Handler)( + req({ endpoint: "not-a-url" }), + ); + + expect(JSON.stringify(await missing.json())).not.toBe( + JSON.stringify(await malformed.json()), + ); + }); +}); + +describe("what the refusal must never carry", () => { + it("echoes neither the endpoint nor the subscription keys", async () => { + // The endpoint path is the routing secret for a push subscription and the + // keys are its crypto material. Publishing the issue list is only safe + // while this holds; if a future schema change adds a message that + // interpolates the value, this fails rather than leaking quietly. + const res = await (POST as unknown as Handler)( + req({ + endpoint: "http://attacker.example/fcm/send/ROUTING-SECRET-abc123", + keys: { p256dh: "P256-SECRET-xyz", auth: "AUTH-SECRET-789" }, + }), + ); + + expect(res.status).toBe(422); + const raw = await res.text(); + expect(raw).not.toContain("ROUTING-SECRET-abc123"); + expect(raw).not.toContain("P256-SECRET-xyz"); + expect(raw).not.toContain("AUTH-SECRET-789"); + expect(raw).not.toContain("attacker.example"); + }); +}); diff --git a/src/app/api/notifications/web-push/route.ts b/src/app/api/notifications/web-push/route.ts index ccc2d07ef..5e64b11cf 100644 --- a/src/app/api/notifications/web-push/route.ts +++ b/src/app/api/notifications/web-push/route.ts @@ -2,7 +2,7 @@ import { NextRequest } from "next/server"; import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; -import { apiSuccess, apiError } from "@/lib/api-response"; +import { apiSuccess, apiError, returnAllZodIssues } from "@/lib/api-response"; import { encrypt } from "@/lib/crypto"; import { webPushSubscriptionSchema } from "@/lib/validations/notifications"; import { z } from "zod/v4"; @@ -18,6 +18,23 @@ const unsubscribeSchema = z.object({ endpoint: z.string().url(), }); +/* + * Both parses below answer with the multi-issue envelope, like every other + * body-parsing route in the tree. They used to answer a flat + * `apiError("Invalid data", 422)`, which made a non-HTTPS endpoint, an + * endpoint pointing at an internal host, an over-long one and a missing + * subscription key byte-identical to each other — a client could see that its + * subscription was refused and had no way to learn which rule it broke. + * + * Echoing the issues is safe here, and it is worth writing down why rather + * than leaving it to be re-derived. The endpoint is a routing secret and the + * keys are subscription crypto material, so the refusal must not carry them + * back. It does not: `sanitiseZodIssues` emits `path`, `code` and `message` + * only — `issue.params`, which is where Zod keeps a rejected value, stays + * server-side — and every message this schema can produce is a fixed string + * or a length/format default. None of them interpolates the value. + */ + /** * POST /api/notifications/web-push * Save a Web Push subscription for the current user. @@ -38,7 +55,7 @@ export const POST = apiHandler(async (request: NextRequest) => { } const parsed = subscribeSchema.safeParse(body); - if (!parsed.success) return apiError("Invalid data", 422); + if (!parsed.success) return returnAllZodIssues(parsed.error, 422); const { endpoint, keys } = parsed.data; const userAgent = request.headers.get("user-agent") ?? undefined; @@ -104,7 +121,7 @@ export const DELETE = apiHandler(async (request: NextRequest) => { } const parsed = unsubscribeSchema.safeParse(body); - if (!parsed.success) return apiError("Invalid data", 422); + if (!parsed.success) return returnAllZodIssues(parsed.error, 422); await prisma.pushSubscription.deleteMany({ where: { diff --git a/src/lib/openapi/routes/notifications-transport.ts b/src/lib/openapi/routes/notifications-transport.ts index 369e75a86..e364f0754 100644 --- a/src/lib/openapi/routes/notifications-transport.ts +++ b/src/lib/openapi/routes/notifications-transport.ts @@ -130,7 +130,7 @@ export const notificationTransportPaths: NonNullable< description: "Stores a browser Push subscription for the caller and, on the first one, creates the account's Web Push notification channel enabled. Upsert by endpoint: re-posting the same endpoint refreshes its keys rather than duplicating the row, so a client can send this on every page load without checking first.\n\n" + "The subscription keys are encrypted at rest. The endpoint is validated as HTTPS and as a public host before anything is written — the push library dials it later through its own fetch, outside the shared egress wrapper, so input time is where that has to be caught.\n\n" + - "Body capped at 64 KB. A malformed body and a rejected endpoint both answer 422 with one flat message; the Zod issues are not surfaced, so a client cannot tell an invalid endpoint from a missing key.", + "Body capped at 64 KB. A rejected subscription answers the standard multi-issue 422, so a client can tell a non-HTTPS endpoint from an internal-host one from a missing key. The issue list carries `path`, `code` and `message` and never the rejected value — neither the endpoint nor the subscription keys are echoed back.", requestBody: { required: true, content: { "application/json": { schema: webPushSubscribeRequest } }, @@ -159,7 +159,7 @@ export const notificationTransportPaths: NonNullable< summary: "Forget a Web Push subscription", description: "Removes the caller's subscription for the given endpoint. Idempotent and silent about the outcome: an endpoint that was never stored, and one belonging to another account, both answer the same success — the response says nothing about how many rows matched, so it cannot be used to probe whether an endpoint exists.\n\n" + - "The Web Push channel row is left in place; this removes a destination, not the channel. Body capped at 64 KB.", + "The Web Push channel row is left in place; this removes a destination, not the channel. Body capped at 64 KB; a malformed `endpoint` answers the standard multi-issue 422, which does not echo the value.", requestBody: { required: true, content: { "application/json": { schema: webPushUnsubscribeRequest } },