Skip to content

Commit ca76bda

Browse files
bleleveclaude
andcommitted
fix(control-plane): guard non-object JSON in approval-body parsing
CodeRabbit follow-up review on #672 (the only thread that didn't auto- resolve after the previous fix pushes). The issue technically lives in #671's plans.handler.ts but CodeRabbit only flagged it now after seeing the post-fix state. `JSON.parse("null")` returns null, `JSON.parse("[1,2]")` returns an array, `JSON.parse("42")` returns a number. All three are syntactically valid JSON but none of them is the object payload approvePlan/rejectPlan expect. The previous code would parse successfully then crash later when dereferencing `body.implementationModel` on `null` (TypeError) or silently accept arrays and primitives as if they were valid bodies. Reject these early with HTTP 400 (`code: "invalid_body"`) via the existing InvalidApprovalBodyError path. Two new regression cases cover JSON-null and JSON-array bodies. Verification: npm run typecheck && npm test -w @open-inspect/control-plane (65/65 files, 2 new test cases green). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2f613bb commit ca76bda

2 files changed

Lines changed: 45 additions & 1 deletion

File tree

packages/control-plane/src/session/http/handlers/plans.handler.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,36 @@ describe("plansHandler.approvePlan / rejectPlan body validation", () => {
224224
expect(planService.approvePlanAndFlush).not.toHaveBeenCalled();
225225
});
226226

227+
it("approvePlan returns 400 when body is JSON null (not an object)", async () => {
228+
// Regression test for CodeRabbit #672 follow-up: JSON.parse("null")
229+
// succeeds with value null; the previous code then crashed later when
230+
// dereferencing body.implementationModel. The guard rejects non-object
231+
// payloads early.
232+
const { handler, planService } = createHandler();
233+
const req = new Request("http://internal/internal/plan/approve", {
234+
method: "POST",
235+
headers: { "Content-Type": "application/json" },
236+
body: "null",
237+
});
238+
const res = await handler.approvePlan(req);
239+
expect(res.status).toBe(400);
240+
const body = (await res.json()) as { code?: string };
241+
expect(body.code).toBe("invalid_body");
242+
expect(planService.approvePlanAndFlush).not.toHaveBeenCalled();
243+
});
244+
245+
it("approvePlan returns 400 when body is a JSON array", async () => {
246+
const { handler, planService } = createHandler();
247+
const req = new Request("http://internal/internal/plan/approve", {
248+
method: "POST",
249+
headers: { "Content-Type": "application/json" },
250+
body: "[1,2,3]",
251+
});
252+
const res = await handler.approvePlan(req);
253+
expect(res.status).toBe(400);
254+
expect(planService.approvePlanAndFlush).not.toHaveBeenCalled();
255+
});
256+
227257
it("approvePlan forwards explicit null reasoning effort to clear the value", async () => {
228258
const { handler, planService } = createHandler();
229259
vi.mocked(planService.approvePlanAndFlush).mockResolvedValue({

packages/control-plane/src/session/http/handlers/plans.handler.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,12 +183,26 @@ async function readApprovalBody(request: Request, log: Logger): Promise<Approval
183183
if (request.headers.get("content-length") === "0") return {};
184184
const text = await request.text();
185185
if (!text.trim()) return {};
186+
let parsed: unknown;
186187
try {
187-
return JSON.parse(text) as ApprovalRequestBody;
188+
parsed = JSON.parse(text);
188189
} catch (e) {
189190
log.warn("plans.approval.invalid_body", { error: e instanceof Error ? e : String(e) });
190191
throw new InvalidApprovalBodyError(e);
191192
}
193+
// JSON.parse("null") returns null, JSON.parse("[1]") returns an array,
194+
// and JSON.parse("42") returns a number — all syntactically valid JSON
195+
// but not the object payload we expect. Treat them as malformed bodies
196+
// so callers don't dereference properties on non-objects (which would
197+
// either crash with TypeError on null, or silently no-op on primitives
198+
// and let arrays through as objects).
199+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
200+
log.warn("plans.approval.invalid_body", {
201+
error: `Expected JSON object, got ${parsed === null ? "null" : Array.isArray(parsed) ? "array" : typeof parsed}`,
202+
});
203+
throw new InvalidApprovalBodyError(new Error("Expected JSON object"));
204+
}
205+
return parsed as ApprovalRequestBody;
192206
}
193207

194208
function errorResponseForApproval(e: unknown, log: Logger, logEvent: string): Response {

0 commit comments

Comments
 (0)