From 89abc0937bc3ace5c79fc467fa0f68766ade0504 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Wed, 12 Aug 2026 12:38:22 +0100 Subject: [PATCH] fix(oracle): fail closed on a malformed set-price-cap body (GH#2509) `POST /api/oracle/set-price-cap` treats an EMPTY body as "apply to every admin-oracle market". It implemented that by initialising `body = {}` and swallowing every exception from `req.json()`: let body: { slabAddress?: string; maxChangeE2bps?: number } = {}; try { body = await req.json(); } catch { // empty body is valid -- means "all admin-oracle markets" } So any parse failure left `body` as `{}` -- the exact state an empty body produces. A truncated or malformed payload was therefore indistinguishable from the deliberate all-market command: `body.slabAddress` is undefined, the route takes the `else` branch at :155, selects every market whose `oracle_authority` matches the crank pubkey (bounded only by MAX_SLAB_BATCH = 50), and submits one signed transaction per validated market. That is fail-OPEN scope expansion on an administrative write path -- the worse the input, the broader the operation. It is reachable only after the admin secret and crank-key checks pass, so this is not an auth bypass; it is an authenticated operator's malformed request silently becoming a batch. The fix reads the raw text once and branches on whether the caller actually sent anything, so "no body" and "bad body" stop being the same state: - empty / whitespace-only body -> unchanged, still the all-markets command - non-empty body that fails JSON.parse -> 400 - non-empty body that parses to a NON-OBJECT -> 400 The third case matters and a try/catch alone does not cover it: `[]`, `"str"`, `123` and `null` all parse successfully, and every field read on them yields `undefined`, which reaches the same all-market branch. `typeof null === "object"`, so null is excluded explicitly. Tests exercise the real route through the existing mocked harness rather than modelling the logic. One of them needs its evidence explained, and says so in a comment: the harness cannot reach a response on the all-market branch, because that path calls `new PublicKey(config.programId)` and the file's PublicKey mock is an arrow function, so it throws "is not a constructor". That throw is raised BELOW the parse gate, so reaching it proves the empty body was accepted and dispatched -- rejection at the gate would return 400 and never touch PublicKey. Mutation-tested, control either side: control 8 passed restore the swallowed parse 2 failed drop the non-object guard only 1 failed control 8 passed Verified: 302 files, 3053 passed, 17 skipped; tsc --noEmit exit 0. Co-Authored-By: Claude Opus 5 --- .../api/oracle-set-price-cap.test.ts | 67 +++++++++++++++++++ app/app/api/oracle/set-price-cap/route.ts | 46 +++++++++++-- 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/app/__tests__/api/oracle-set-price-cap.test.ts b/app/__tests__/api/oracle-set-price-cap.test.ts index 98c34614f..b34d38e49 100644 --- a/app/__tests__/api/oracle-set-price-cap.test.ts +++ b/app/__tests__/api/oracle-set-price-cap.test.ts @@ -55,6 +55,18 @@ function post( }); } +/** GH#2509: send a raw body verbatim, so malformed/empty payloads can be exercised. */ +function postRaw( + raw: string | undefined, + headers: Record = {}, +): NextRequest { + return new NextRequest("http://localhost/api/oracle/set-price-cap", { + method: "POST", + headers: { "Content-Type": "application/json", ...headers }, + ...(raw === undefined ? {} : { body: raw }), + }); +} + beforeEach(() => { vi.clearAllMocks(); process.env.ADMIN_API_SECRET = "test-admin-secret"; @@ -95,6 +107,61 @@ describe("POST /api/oracle/set-price-cap", () => { expect(res.status).toBe(401); }); + // ── GH#2509: malformed input must fail closed, not widen the operation ────── + // + // The route treats an EMPTY body as "apply to every admin-oracle market". It + // used to reach that same state on ANY parse failure, so a truncated payload + // was indistinguishable from the deliberate all-market command and could + // submit one signed transaction per market. + + it("GH#2509: returns 400 for malformed JSON instead of targeting all markets", async () => { + const req = postRaw('{"slabAddress": "7G3SsnevWwUWjWAwGGmr2N11x8KAGn1abzjV3bBbZkAM"', { + "x-admin-secret": "test-admin-secret", + }); + const res = await POST(req); + expect(res.status).toBe(400); + const j = await res.json(); + expect(j.error).toMatch(/malformed JSON/i); + // The point of the fix: no transaction may be signed for a request we + // could not parse. + expect(mockSendAndConfirm).not.toHaveBeenCalled(); + }); + + it("GH#2509: returns 400 for a non-object JSON body", async () => { + // These parse successfully, so a try/catch alone would not catch them, yet + // every field read yields undefined — the same all-market path. + for (const raw of ["[]", '"a string"', "123", "null"]) { + const res = await POST(postRaw(raw, { "x-admin-secret": "test-admin-secret" })); + expect(res.status).toBe(400); + const j = await res.json(); + expect(j.error).toMatch(/must be a JSON object/i); + } + expect(mockSendAndConfirm).not.toHaveBeenCalled(); + }); + + it("GH#2509: an empty body is still routed to the all-markets command", async () => { + // The contract this fix must NOT break: empty body still means "all + // admin-oracle markets". + // + // What counts as evidence here needs stating, because this harness does not + // mock far enough to reach a response on that branch. The all-market path + // runs `new PublicKey(config.programId)`, and this file's PublicKey mock is + // an arrow function, so it throws "is not a constructor". That throw is + // raised BELOW the parse gate — so reaching it proves the empty body was + // accepted and dispatched to the all-market branch. Being rejected at the + // gate would instead return a 400 and never touch PublicKey. + for (const raw of ["", " \n ", undefined]) { + let status: number | null = null; + try { + status = (await POST(postRaw(raw, { "x-admin-secret": "test-admin-secret" }))).status; + } catch (err) { + expect(String(err)).toMatch(/not a constructor/); + continue; // got past the gate — which is the property under test + } + expect(status).not.toBe(400); + } + }); + it("returns 400 for non-integer maxChangeE2bps", async () => { const req = post( { slabAddress: "7G3SsnevWwUWjWAwGGmr2N11x8KAGn1abzjV3bBbZkAM", maxChangeE2bps: 1.5 }, diff --git a/app/app/api/oracle/set-price-cap/route.ts b/app/app/api/oracle/set-price-cap/route.ts index 8d7d1c2ca..affb9eb7f 100644 --- a/app/app/api/oracle/set-price-cap/route.ts +++ b/app/app/api/oracle/set-price-cap/route.ts @@ -97,11 +97,47 @@ export async function POST(req: NextRequest) { ); } - let body: { slabAddress?: string; maxChangeE2bps?: number } = {}; - try { - body = await req.json(); - } catch { - // empty body is valid — means "all admin-oracle markets" + // GH#2509: an EMPTY body is the documented "all admin-oracle markets" command. + // Malformed JSON must not be indistinguishable from it. + // + // This previously read `body = await req.json()` inside a try whose catch was + // empty, so any parse failure left `body` as `{}` — the same state an empty + // body produces. A truncated or malformed payload therefore fell through to + // the `else` branch below, which selects every admin-oracle market and can + // submit one signed transaction per market (bounded only by MAX_SLAB_BATCH). + // That is a fail-OPEN scope expansion on an administrative write path: the + // worse the input, the broader the operation. + // + // Read the raw text once and branch on whether the caller actually sent + // anything, so "no body" and "bad body" are distinguishable. + let body: { slabAddress?: string; maxChangeE2bps?: number | string } = {}; + const rawBody = await req.text(); + if (rawBody.trim() !== "") { + let parsed: unknown; + try { + parsed = JSON.parse(rawBody); + } catch { + return NextResponse.json( + { + error: + "malformed JSON body. Send a valid JSON object, or an empty body to target all admin-oracle markets.", + }, + { status: 400 }, + ); + } + // A non-object (array, string, number, null) also yields `undefined` for + // every field read below, which would silently reach the all-market path + // the same way. `typeof null === "object"`, so null is excluded explicitly. + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + return NextResponse.json( + { + error: + "request body must be a JSON object, or empty to target all admin-oracle markets.", + }, + { status: 400 }, + ); + } + body = parsed as { slabAddress?: string; maxChangeE2bps?: number | string }; } let maxChangeE2bps: bigint;