Fix: Decline non-object JSON bodies as foreign traffic in the JSON-RPC parsers - #1118
Conversation
…C parsers a2a-parser and mcp-parser logged a top-level JSON array as "invalid JSON-RPC" with an unmarshal error, because json.Unmarshal into the JSONRPCRequest struct fails before the namespace guard that commit f345284 added for the sibling case. An array body is not malformed traffic, though — it is simply a body that was never JSON-RPC, the same category as the inference bodies that guard already declines calmly. Peek the first non-whitespace byte before the unmarshal and route non-object bodies (array, string, number, null, whitespace-only) into the existing "not an X-namespace method, skipping" line. No new log call and no new message string; the "invalid" branch survives, now narrowed to bodies that start with '{' and still fail to decode, where the word is accurate. null already took the calm path, so it is observably unchanged. Both skip lines also gain host and path, which the report needed to identify the sender at all. pipeline.Context.Path is query-stripped by every listener, so no query handling is needed. Fixes rossoctl#1084 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Right fix, right shape. Peeking the first non-whitespace byte puts the decision where it belongs — a top-level array was never JSON-RPC, so declining it on shape before any method check is the same category as the inference bodies the f345284 guard already declines calmly. Reusing the existing skip line rather than adding a message is what keeps this from being a logging change with a behaviour change hidden in it.
!isObject || !isA2AMethod(rpc.Method) is the correct structure: when isObject is false, rpc is the zero value, so the line reports an empty method — honest for a body that never had one.
What I verified rather than took on faith
| Claim | Result |
|---|---|
| Non-object bodies take the calm skip path | Confirmed empirically — throwaway table test over array / string / number / null / whitespace / empty; all Continue, no extension |
| Leading-whitespace objects still match | Confirmed — " \n\t{...message/send...}" parses and populates Extensions.A2A |
The invalid branch survives for {-prefixed bodies |
Confirmed — {"method": still reaches it |
bytes.TrimLeft(body, " \t\r\n") is the right set |
Correct — exactly RFC 8259's four whitespace bytes, no more |
mcp-parser's wording is quoted in the README |
Confirmed — authbridge/demos/mcp-parser/README.md:207 quotes "mcp-parser: body is not valid JSON-RPC" verbatim, so preserving the string was right and the example still holds |
| Array fixtures wrap real protocol methods | Confirmed — message/send and tools/list, so the tests pin shape rather than substring |
bytes imported in both files |
Confirmed |
Assisted-By per CLAUDE.md |
Confirmed in both the commit and the PR body |
The array fixtures carrying a method that would match if the body were an object is the detail that makes those tests worth having — they fail if someone later reintroduces a substring check.
One correction: the CI failure is misattributed
Go CI (authlib) is red, and the description attributes a pre-existing failure to plugins/tokenbroker/client under HTTPS_PROXY. That is not what CI is failing on. From the job log:
--- FAIL: TestHandleUsage_LedgerBackedModelSeriesDisclosesWhatItLeavesOut (0.01s)
usage_test.go:809: Totals.CostMicros = 100000, want 350000 — both rows are real spend
That is sessionapi — a cost-ledger test where one of two recorded rows went uncounted. This PR is not the cause, and I checked that rather than assuming it:
- The branch's merge-base is
mainHEAD (3be8d31f), zero commits behind, and the diff touches only the two parser packages — thesessionapicode is byte-identical tomain. - The full
authlibsuite passes locally on bothmainand this branch. Go CI (authlib)passes on #1114, #1115 and #1117, so it is intermittent rather than broken.insideTodayAt's own comment documents prior clock-dependent CI breakage in this family ("the case that actually broke CI was 00:00:30"), which fits a flake.
I could not reproduce it across repeated runs or three timezones, so I cannot name the trigger — only rule this PR out as the cause. Worth a re-run, and if it reproduces, a separate issue against sessionapi.
Either way the description is worth correcting: "pre-existing, unrelated: tokenbroker/client + proxy" points a future bisector at the wrong package, and a genuinely flaky ledger test is more worth knowing about than a proxy artifact. Not blocking — the parser change stands on its own and the failing test cannot reach it.
Areas reviewed: Go (JSON handling, plugin pipeline), tests, CI failure triage, README/log-string coupling, commit conventions
Commits: 1, signed off, Assisted-By per CLAUDE.md
CI status: 26/27 pass; authlib fails on the unrelated flaky sessionapi test triaged above
3 participants
A top-level JSON array logged as
a2a-parser: invalid JSON-RPCwith an unmarshal error, becausejson.Unmarshalinto theJSONRPCRequeststruct fails one line before the namespace guard from f345284. An array body isn't malformed — it was never JSON-RPC, same category as the inference bodies that guard already declines calmly.mcp-parserhad the identical defect.Peek the first non-whitespace byte before the unmarshal; non-object bodies fall into the existing "not an X-namespace method, skipping" line. No new log call, no new message string —
mcp-parser's wording is quoted indemos/mcp-parser/README.md, so it stays put. Theinvalidbranch survives, narrowed to bodies that start with{and still fail to decode.Both skip lines also gain
hostandpath, which #1084 needed to identify the sender at all.[...],"str",42, whitespaceinvalid JSON-RPC+ errornull{"method":invalid JSON-RPCinvalid JSON-RPC+ host/pathNo behavior change beyond logging:
Continue, no extension, no Invocation, as before.Testing. Three rows added to a2a's existing non-JSON-RPC table plus one sibling test for mcp; both array fixtures wrap a real protocol method, so they pin the decision to shape rather than substring. These assertions pass under the old code too, so I also captured actual
slogoutput with a temporary test (not committed) to confirm each row of the table above.gofmt/go vetclean; both packages pass under-race.Pre-existing on
main, unrelated:plugins/tokenbroker/clientfails two tests locally whenHTTPS_PROXYis set, since a proxy answers 502 where the test expects a transport-level failure.Fixes #1084
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com