Keep peer payloads out of the SSE error-logging paths - #210
Merged
Conversation
2.1.0 replaced payload logging with method/id summaries (#198), but only on the paths that log successfully parsed messages. The paths that log *because* something was unexpected still wrote the value: - parse_sse_event_data logged a valid but non-object JSON payload with #inspect at WARN, which the default logger emits — so this leaked without anyone enabling DEBUG; - handle_server_message logged malformed event data verbatim at DEBUG. Both now report type and size only. A reproduction that sent a scalar containing a sentinel value found it in the WARN log before, and nothing after. The same reproduction exposed a related robustness bug: a JSON-parseable scalar or array on the events stream reached dispatch_server_message, which calls #key? on it and raised (caught upstream by the chunk handler's generic rescue, but only by luck). Non-object messages are now skipped with a typed warning, matching what the POST response path already did. Found by Codex adversarial review of the 2.1.0 release PR (#208). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
simonx1
added a commit
that referenced
this pull request
Aug 4, 2026
Version bump, changelog, and the documentation the release needs. - lib/mcp_client/version.rb and the Gemfile.lock self-reference move to 2.1.0; the gemspec reads the constant, so it needs no edit. - CHANGELOG documents the PRs merged since 2.0.0 (#188-#210) as one coherent story, with the behavior changes hosts must act on called out under Breaking Changes and Migration notes: tools/call is no longer auto-retried (including through session recovery), task operations refuse to guess a server, peer-facing error messages are constant, and logs no longer carry payloads. - README gains a "Treating the Server as Untrusted" section summarizing what the transports now refuse from a peer, documents the retry semantics for tools/call, and documents max_decompressed_body_bytes. Two claims are deliberately narrow: the OAuth check is textual (DNS is not resolved) and the response-size limit is Streamable HTTP only, since that is the only transport requesting gzip. Two defects found while writing the docs, both fixed here: - max_decompressed_body_bytes could not actually be set through the documented path. streamable_http_config did not accept it and ServerFactory did not forward it, so the option only worked when constructing ServerStreamableHTTP directly. Wired through both, with specs asserting the config path, the default, and the validation. - The README claimed "No runtime dependencies" while the gemspec declares faraday, faraday-follow_redirects, faraday-retry and base64. Metrics/ParameterLists now sets CountKeywordArgs: false. The *_config builders are keyword-only option factories, and the codebase already carried six hand-written disables for exactly this; those are removed. The two code bugs Codex found while reviewing this release ship separately in #209 and #210. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 4, 2026
simonx1
added a commit
that referenced
this pull request
Aug 4, 2026
## Summary Found by Codex in a follow-up review of the 2.1.0 release PR (#208), verified before fixing. `JSON::ParserError#message` **quotes the offending token**: ``` JSON.parse('{LEAKME-123-45-6789 bad}') # => expected object key, got 'LEAKME-123-45-6789' at line 1 column 2 ``` #210 replaced the log lines that printed payloads directly, but kept `e.message` — on my assumption that it carried only a position. It doesn't, so peer-controlled bytes still reached logs. The leak was reachable on **every** JSON receive path: | Path | Channel | |---|---| | GET events stream (`server_streamable_http.rb`) | `logger.error` | | POST SSE response parser (`server_streamable_http/json_rpc_transport.rb`) | `logger.warn` — **default-visible** | | Legacy SSE parser (`server_sse/sse_parser.rb`) | `logger.warn` — **default-visible** | | OAuth token-refresh (`auth/oauth_provider.rb`) | `logger.warn` — and the payload there is a *token response* | | All four transports' `Invalid JSON response from server` | `TransportError` message, which hosts routinely log | ## The test that gave false assurance Codex also spotted that the regression test I added in #210 **passed by accident**: its sentinel appeared *after* the first invalid token, so the parser never quoted it. That is the more useful half of the finding — the fix looked verified when it wasn't. The new tests place the sentinel as the **first** invalid token on each path, which is the case that actually exercises the quoting. ## Fix `describe_parse_error` (in `JsonRpcCommon`, shared by every transport) keeps what makes a parse failure diagnosable — the `line N column M` position and the payload byte size — and drops the quoted content: ``` Invalid JSON in server message: malformed JSON, at line 1 column 2, 18 bytes ``` I kept the position deliberately rather than logging only a class name: debugging a server that emits subtly broken JSON is painful without it, and the position is not peer content. The `Invalid JSON response from server:` prefix is unchanged, so existing rescues and the nine specs matching on it are unaffected. ## Testing New specs assert no sentinel reaches the log on the GET, POST-SSE and legacy-SSE paths, none reaches the raised `TransportError` for a malformed HTTP body, and that the position and size survive. Full suite 1699 examples / 0 failures, RuboCop clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
simonx1
added a commit
that referenced
this pull request
Aug 4, 2026
## Summary Releases **2.1.0**, covering the PRs merged since 2.0.0 (#188–#210): a security pass over every transport, plus Ruby 4.0.6 support. > **Rebased.** Codex's review of this PR found two real code bugs. Rather than smuggle them into a release commit, they shipped separately as **#209** (`tools/call` replayed by session recovery) and **#210** (peer payloads reaching the SSE error logs), both merged. This branch was rebuilt on top of them, so its diff is now version + docs + the two doc-driven fixes below — no security code changes. The review comment below has the reproductions. The theme is one sentence: **a remote MCP server is untrusted input.** 2.0.0 was correct against a cooperative server but assumed good faith in places where a hostile — or merely compromised — peer controls the data. The wire protocol is unchanged and the ordinary client API is unchanged; what changed is what the client accepts, retries, logs and reflects back. ## What's in the release | | | |---|---| | Version | `lib/mcp_client/version.rb`, `Gemfile.lock` self-reference → **2.1.0** (gemspec reads the constant) | | CHANGELOG | New 2.1.0 entry in the established format, with **Breaking Changes** and **Migration notes** | | README | New "Treating the Server as Untrusted" section; `tools/call` retry semantics; `max_decompressed_body_bytes` | ### Behaviour changes hosts must act on These are spelled out in the CHANGELOG's Migration notes: - **`tools/call` is never retried automatically** — including through session-expiry recovery (#209). A "transient" failure can arrive after the server executed the request, and JSON-RPC has no idempotency key to make a replay safe. Retry explicitly if your application knows it is safe, and treat the error as *outcome unknown*. - **Task operations refuse to guess a server.** Pass the `Task` handle from `call_tool_as_task`, or name the server. A bare ID still works with one configured server; with several it raises `ArgumentError` rather than acting on the wrong one. - **Peer-facing error messages are constant** and **DEBUG logs no longer contain payloads** — if you parsed either for detail, the detail now lives only in local logs. Error codes are unchanged. - **Cross-origin SSE endpoints/redirects and non-HTTPS or loopback OAuth discovery URLs from a peer are refused.** Local development still works when the *configured* server is itself local. ## Two defects found while writing the docs Writing the documentation turned out to be a review in its own right: 1. **`max_decompressed_body_bytes` could not be set through the documented path.** `streamable_http_config` did not accept it and `ServerFactory` did not forward it, so the option introduced in #188 only worked when constructing `ServerStreamableHTTP` directly — i.e. the knob was effectively unreachable for anyone using the normal config API. Wired through both, with specs asserting the config path, the default, and the positive-integer validation. 2. **The README claimed "No runtime dependencies"** while the gemspec declares `faraday`, `faraday-follow_redirects`, `faraday-retry` and `base64`. Pre-existing, corrected here. ## RuboCop config `Metrics/ParameterLists` now sets `CountKeywordArgs: false`. The `*_config` builders are keyword-only option factories where each keyword is a documented, defaulted setting; counting them against a positional-argument limit would push new options into an opaque hash. The codebase already carried **six hand-written `rubocop:disable Metrics/ParameterLists`** comments for exactly this reason — those are now removed, which is what the 58 autocorrections in the diff are. ## Verification | Check | Result | |---|---| | RSpec, Ruby 4.0.6 (default) | **1698 examples, 0 failures** | | RSpec, Ruby 3.3.5 | **1698 examples, 0 failures** | | RuboCop | 136 files, no offenses | | `gem build` | builds `ruby-mcp-client-2.1.0.gem`; ships lib + LICENSE + README only (no specs/examples/secrets) | | `Gem::Specification#validate` | passes | | Version consistency | `version.rb`, `Gemfile.lock`, gemspec all report 2.1.0 | | Examples suite (pre-release, on merged main) | 18 PASS · 0 FAIL · 2 SKIP | The two example skips are environmental: no `ANTHROPIC_API_KEY` in this environment, and the interactive browser-OAuth example. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found by Codex during the adversarial review of the 2.1.0 release PR (#208), reproduced before fixing.
#198 replaced payload logging with method/id summaries — but only on the paths that log a successfully parsed message. The paths that log because something was unexpected still wrote the value verbatim:
parse_sse_event_data(server_streamable_http/json_rpc_transport.rb)#inspecthandle_server_message(server_streamable_http.rb)The WARN one is the worse of the two: the default logger emits WARN, so that path leaked without anyone enabling DEBUG. A peer that sends
"patient-record-456"as SSE data got it written straight into the host's log.Fix
Both now log type and size only:
The
JSON::ParserErrormessage names the failure position, not the payload, so it is safe to keep.A related robustness bug the reproduction exposed
A JSON-parseable scalar or array on the GET events stream reached
dispatch_server_message, which calls#key?on it and raisedNoMethodError. It was caught upstream byprocess_event_chunk's generic rescue — so it never surfaced, but only by luck, and it aborted processing of that chunk. Non-object messages are now skipped with a typed warning, matching what the POST response path already did.Testing
New spec asserts sentinel values from both a non-object payload and a malformed one never appear in the log, while the typed warning does. Full suite 1694 examples / 0 failures, RuboCop clean.
🤖 Generated with Claude Code