From e7535e333182442efb6aa481f9dfd5844220d573 Mon Sep 17 00:00:00 2001 From: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:36:52 -0500 Subject: [PATCH] fix(agt): return -32600 Invalid Request for unsupported batches and invalid JSON-RPC frames Reject JSON-RPC 2.0 batch arrays (#130) and non-JSON-RPC 2.0 frames (missing or non-'2.0' jsonrpc version, or missing/non-string method, #163) with standard JSON-RPC -32600 Invalid Request before ACS envelope validation or denyOnInvalidEnvelope runs, and classify -32600 as a guardian refusal in the host-adapter failure posture. Fixes #130 Fixes #163 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com> --- .../conformance/src/failure-domains.ts | 3 +- .../agt/packages/guardian/src/server.ts | 41 +++++++++- .../agt/packages/guardian/test/server.test.ts | 78 +++++++++++++++++++ .../host-adapter/src/failure-posture.ts | 2 +- .../host-adapter/test/failure-posture.test.ts | 4 +- .../test/invalid-request-refusal.test.ts | 53 +++++++++++++ 6 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 reference-implementations/agt/packages/host-adapter/test/invalid-request-refusal.test.ts diff --git a/reference-implementations/agt/packages/conformance/src/failure-domains.ts b/reference-implementations/agt/packages/conformance/src/failure-domains.ts index 0700e3e4..d9e3c976 100644 --- a/reference-implementations/agt/packages/conformance/src/failure-domains.ts +++ b/reference-implementations/agt/packages/conformance/src/failure-domains.ts @@ -9,8 +9,7 @@ * TWO FAILURE DOMAINS, kept apart here exactly as `deny-on-invalid-envelope.ts` * keeps them apart at the point that answers them: * - * (1) AGT'S EVALUATION LAYER FAILS CLOSED (§6.4). An envelope that fails - * schema validation, or whose evaluation throws, arrives as an + * (1) AGT'S EVALUATION LAYER FAILS CLOSED (§6.4). A JSON-RPC request whose envelope fails schema validation, or whose evaluation throws, arrives as an * honoured ACS `deny` decision -- never a bare JSON-RPC error. * `denyOnInvalidEnvelope` (`packages/guardian/src/deny-on-invalid- * envelope.ts`) is what does it, reached from two different catches in diff --git a/reference-implementations/agt/packages/guardian/src/server.ts b/reference-implementations/agt/packages/guardian/src/server.ts index 8a38eed4..3c9bfe31 100644 --- a/reference-implementations/agt/packages/guardian/src/server.ts +++ b/reference-implementations/agt/packages/guardian/src/server.ts @@ -198,6 +198,9 @@ export function toRepoRelativeMessage(error: unknown): string { } } +/** JSON-RPC's own code, not an ACS one: the body is not a Request at all (#130, #163). */ +const INVALID_REQUEST_CODE = -32600; + /** * ACS reserves -32000..-32099 for application errors (Specification §17), * but only enumerates named codes -32000..-32007 in the §17.1 registry @@ -876,9 +879,18 @@ async function dispatch( ): Promise { const rpcId = extractId(raw); + // JSON-RPC 2.0 request framing boundary (#130, #163): array-shaped batch + // requests (unsupported by this Guardian) and non-JSON-RPC objects (missing + // or non-"2.0" jsonrpc, or missing/non-string method) are rejected as + // standard JSON-RPC -32600 Invalid Request before dispatch acts on ACS envelope validation + // or denyOnInvalidEnvelope runs. + if (!isValidJsonRpcRequest(raw)) { + return { response: errorResponse(rpcId, INVALID_REQUEST_CODE, "Invalid Request"), rejection: "envelope_invalid" }; + } + // validateEnvelope checked the general request-envelope.json shape for // every method, plus -- only for steps/toolCallRequest -- the hook-specific - // payload schema (validateForDispatch, above). An invalid steps/* envelope + // payload schema (validateForDispatch, above). A valid JSON-RPC request with an invalid steps/* envelope // becomes an honoured ACS `deny` decision rather than a bare JSON-RPC error, // since there is an identifiable step to answer for. A handshake failure // and an undispatched method are not steps/*, so they always fall through @@ -1110,6 +1122,33 @@ function extractMethod(raw: unknown): string | null { return null; } +/** Whether the parsed JSON is a JSON-RPC 2.0 Request object, before ACS + * schema validation runs. Rejects array-shaped batch inputs (#130) and + * objects with a missing or non-"2.0" jsonrpc version, a missing/non-string + * method, an id that is not a string, number or null, or scalar params (#163). */ +function isValidJsonRpcRequest(raw: unknown): boolean { + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + return false; + } + const obj = raw as Record; + if (obj.jsonrpc !== "2.0") { + return false; + } + if (typeof obj.method !== "string" || obj.method.length === 0) { + return false; + } + // JSON-RPC 2.0 section 4: id is a string, a number, or null. A missing id is + // a notification, which is valid JSON-RPC, so ACS validation answers it. + if ("id" in obj && obj.id !== null && typeof obj.id !== "string" && typeof obj.id !== "number") { + return false; + } + // JSON-RPC 2.0 section 4.2: params, when present, is an object or an array. + if ("params" in obj && (typeof obj.params !== "object" || obj.params === null)) { + return false; + } + return true; +} + /** Whether the raw envelope names a `steps/*` method -- read before * validation, so it is a string test and nothing more. denyOnInvalidEnvelope * only turns a schema-validation failure into a deny decision for steps/*: diff --git a/reference-implementations/agt/packages/guardian/test/server.test.ts b/reference-implementations/agt/packages/guardian/test/server.test.ts index 60f0be35..a67110f1 100644 --- a/reference-implementations/agt/packages/guardian/test/server.test.ts +++ b/reference-implementations/agt/packages/guardian/test/server.test.ts @@ -334,6 +334,84 @@ describe("startGuardian POST /acs -- denyOnInvalidEnvelope's boundary: what stay expect(response.error?.code).toBeLessThanOrEqual(-32000); }); + it("rejects array-shaped batch requests with -32600 Invalid Request (#130)", async () => { + const batch = [toolCallEnvelope("ls -la", { id: 1 }), toolCallEnvelope("pwd", { id: 2 })]; + const response = await postAcs(url, batch); + + expect(response.result).toBeUndefined(); + expect(response.id).toBeNull(); + expect(response.error).toEqual({ code: -32600, message: "Invalid Request" }); + }); + + it("rejects valid JSON that is not a JSON-RPC 2.0 request with -32600 Invalid Request, never an ACS deny (#163)", async () => { + const missingJsonrpc = await postAcs(url, { hello: "world" }); + expect(missingJsonrpc.result).toBeUndefined(); + expect(missingJsonrpc.id).toBeNull(); + expect(missingJsonrpc.error).toEqual({ code: -32600, message: "Invalid Request" }); + + const wrongVersionStep = await postAcs(url, { + jsonrpc: "1.0", + id: 1, + method: "steps/toolCallRequest", + params: {}, + }); + expect(wrongVersionStep.result).toBeUndefined(); + expect(wrongVersionStep.id).toBe(1); + expect(wrongVersionStep.error).toEqual({ code: -32600, message: "Invalid Request" }); + }); + + // JSON-RPC 2.0 section 4: id is a string, a number, or null, and params, when + // present, is an object or an array. A scalar params on a step came back as + // an ACS deny, which is what issue #163 item 4 rules out. + it.each([ + ["an object id", { id: {} }, null], + ["an array id", { id: [] }, null], + ["a boolean id", { id: true }, null], + ["a number as params", { id: 1, params: 5 }, 1], + ["a string as params", { id: 1, params: "x" }, 1], + ["null as params", { id: 1, params: null }, 1], + ])("rejects a steps/* request with %s as -32600, never an ACS deny (#163)", async (_label, fields, expectedId) => { + const response = await postAcs(url, { jsonrpc: "2.0", method: "steps/toolCallRequest", params: {}, ...fields }); + + expect(response.result).toBeUndefined(); + expect(response.id).toBe(expectedId); + expect(response.error).toEqual({ code: -32600, message: "Invalid Request" }); + }); + + it.each([ + ["a non-string method", { jsonrpc: "2.0", id: 1, method: 5, params: {} }], + ["a missing method", { jsonrpc: "2.0", id: 1, params: {} }], + ["a bare number", 5], + ["null", null], + ])("rejects %s as -32600 (#163)", async (_label, body) => { + const response = await postAcs(url, body); + + expect(response.result).toBeUndefined(); + expect(response.error).toEqual({ code: -32600, message: "Invalid Request" }); + }); + + // Valid JSON-RPC that fails ACS stays on the ACS path (issue #163 item 3). + it.each([ + ["object params", {}], + ["array params", []], + ])("keeps a steps/* request with empty %s an ACS deny (#163)", async (_label, params) => { + const response = await postAcs(url, { jsonrpc: "2.0", id: 1, method: "steps/toolCallRequest", params }); + + expect(response.error).toBeUndefined(); + expect(response.result?.decision).toBe("deny"); + expect(response.result?.reason_codes).toEqual(["envelope_invalid"]); + }); + + it.each([ + ["a null id", { id: null }], + ["no id", {}], + ])("keeps a steps/* request with %s on the ACS path (#163)", async (_label, fields) => { + const response = await postAcs(url, { jsonrpc: "2.0", method: "steps/toolCallRequest", params: {}, ...fields }); + + expect(response.result).toBeUndefined(); + expect(response.error?.code).toBe(-32010); + }); + // Envelope-log tapping: the response for the schema-invalid deny path is a // JSON-RPC success rather than an error, so this confirms the tap still // pairs request and response for it, the same as any other response. diff --git a/reference-implementations/agt/packages/host-adapter/src/failure-posture.ts b/reference-implementations/agt/packages/host-adapter/src/failure-posture.ts index 5ef2389c..b4e91e62 100644 --- a/reference-implementations/agt/packages/host-adapter/src/failure-posture.ts +++ b/reference-implementations/agt/packages/host-adapter/src/failure-posture.ts @@ -157,7 +157,7 @@ const TRANSPORT_ERROR_CODES = new Set([ * preferred to guessing, because widening this to every error object would * also fail closed on an intermediary's error the Guardian never sent. */ -const REFUSAL_RPC_CODES = new Set([-32700, -32010, -32011, -32020]); +const REFUSAL_RPC_CODES = new Set([-32700, -32600, -32010, -32011, -32020]); /** * Names what came back INSTEAD of a decision, for the audit entry. Total: an diff --git a/reference-implementations/agt/packages/host-adapter/test/failure-posture.test.ts b/reference-implementations/agt/packages/host-adapter/test/failure-posture.test.ts index c9562692..15ec84dc 100644 --- a/reference-implementations/agt/packages/host-adapter/test/failure-posture.test.ts +++ b/reference-implementations/agt/packages/host-adapter/test/failure-posture.test.ts @@ -422,7 +422,7 @@ describe("classifyDeliveryFailure — what came back instead of a decision", () // decide (packages/guardian/src/server.ts). Asserted per code rather than // as a set, so a failure names which route regressed. client.test.ts drives // three of them against a real Guardian; this is the unit half. - it.each([-32700, -32010, -32011, -32020])("classifies the guardian's %i as a refusal, not a delivery failure", (code) => { + it.each([-32700, -32600, -32010, -32011, -32020])("classifies the guardian's %i as a refusal, not a delivery failure", (code) => { const { kind, message } = classifyDeliveryFailure({ code, message: "no" }); expect(kind).toBe("refused"); // The code an incident reviewer needs, in the field that reaches the @@ -462,7 +462,7 @@ describe("applyFailurePosture — a guardian that was alive and refused the enve ).toBe("deny"); }); - it.each([-32700, -32010, -32011, -32020])("denies on %i, which the guardian mints for an envelope it will not decide", (code) => { + it.each([-32700, -32600, -32010, -32011, -32020])("denies on %i, which the guardian mints for an envelope it will not decide", (code) => { const { sink } = recordingSink(); expect( applyFailurePosture({ failure: { code, message: "no" }, session: SESSION("proceed"), audit: sink, ...CALL }) diff --git a/reference-implementations/agt/packages/host-adapter/test/invalid-request-refusal.test.ts b/reference-implementations/agt/packages/host-adapter/test/invalid-request-refusal.test.ts new file mode 100644 index 00000000..0e0c7c7d --- /dev/null +++ b/reference-implementations/agt/packages/host-adapter/test/invalid-request-refusal.test.ts @@ -0,0 +1,53 @@ +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +// Test-only import, the same arrangement as client.test.ts: a real Guardian +// mints the code, so this fails if the Guardian and the host stop agreeing. +import { startGuardian, type StartedGuardian } from "guardian"; +import { applyFailurePosture, classifyDeliveryFailure } from "../src/failure-posture.ts"; + +// In its own file because PR #229 replaces the refusal set, and a rebase that +// takes its side of the conflict loses -32600 with no failing test. +describe("a real Guardian's -32600 blocks the step under proceed (#130, #163)", () => { + let guardian: StartedGuardian; + + beforeAll(async () => { + guardian = await startGuardian({ port: 0, manifestPath: "policy/manifest.yaml" }); + }); + + afterAll(async () => { + await guardian.close(); + }); + + it.each([ + ["a batch", [{ jsonrpc: "2.0", id: 1, method: "steps/toolCallRequest", params: {} }]], + ["a wrong jsonrpc version", { jsonrpc: "1.0", id: 1, method: "steps/toolCallRequest", params: {} }], + ])("%s", async (_label, body) => { + const res = await fetch(guardian.url, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + const { error } = (await res.json()) as { error: { code: number; message: string } }; + + expect(error.code).toBe(-32600); + expect(classifyDeliveryFailure(error).kind).toBe("refused"); + expect( + applyFailurePosture({ + failure: error, + session: { + config: { + negotiated_version: "0.1.0", + methods_evaluated: ["steps/toolCallRequest"], + selected_transport: "http", + timeout_config: { default_ms: 5000 }, + on_decision_failure: "proceed", + }, + failure: undefined, + }, + sessionId: "sess-1", + method: "steps/toolCallRequest", + rpcId: "req-1", + audit: { path: "test", write: () => true }, + }).decision, + ).toBe("deny"); + }); +});