diff --git a/CHANGELOG.md b/CHANGELOG.md index cedda2c61..b93bb76ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A request to the approvals API that is not JSON answers 400 + +A body that could not be parsed as JSON, sent to any approvals route that reads one, such as +`PATCH /api/approvals/preferences` or `POST /api/approvals/rules`, answered 500 with the parser's own +message. It now answers 400 "Supply a valid request.", as the delivery routes do. + ### Syncing a memory source a policy refuses says why When a connected app's policy refused the read behind a memory source's sync, `POST diff --git a/server/src/approvals/routes.ts b/server/src/approvals/routes.ts index 689e3fc52..6af698cdf 100644 --- a/server/src/approvals/routes.ts +++ b/server/src/approvals/routes.ts @@ -42,14 +42,19 @@ export function createApprovalRoutes( const routes = new Hono<{ Variables: AppVariables }>(); routes.use("*", requireUser); routes.onError((error, context) => - context.json( - { error: error.message }, - error instanceof ApprovalNotFoundError - ? 404 - : error instanceof ApprovalRefusedError || error instanceof z.ZodError - ? 400 - : 500, - ), + // A body that is not JSON is the caller's mistake, as the delivery routes answer it, not a + // 500 carrying the parser's message. + error instanceof SyntaxError + ? context.json({ error: "Supply a valid request." }, 400) + : context.json( + { error: error.message }, + error instanceof ApprovalNotFoundError + ? 404 + : error instanceof ApprovalRefusedError || + error instanceof z.ZodError + ? 400 + : 500, + ), ); routes.get("/", async (context) => context.json(await service.inbox(context.var.actor.id)), diff --git a/server/tests/approvals.test.ts b/server/tests/approvals.test.ts index 2a4ec2a46..7fb8fdf7f 100644 --- a/server/tests/approvals.test.ts +++ b/server/tests/approvals.test.ts @@ -716,3 +716,25 @@ test.each([ expect(continued).toHaveLength(1); expect(continued[0]?.result.error).toContain("Not done"); }); + +test("a request body that is not JSON answers 400, not a 500 with the parser's message", async () => { + const routes = createApprovalRoutes( + {} as serviceModule.ApprovalService, + async (ctx, next) => { + ctx.set("actor", { + id: "owner", + email: "owner@example.com", + role: "user", + }); + await next(); + }, + ); + for (const [method, path] of [ + ["PATCH", "/preferences"], + ["POST", "/rules"], + ]) { + const response = await routes.request(path, { method, body: "{oops" }); + expect(response.status).toBe(400); + expect(await response.json()).toEqual({ error: "Supply a valid request." }); + } +});