Uh oh!
There was an error while loading. Please reload this page.
chore: fast-follow nits — SEP-2243 Number() coercion, connect({prior}) docs, icons example - #2364
Conversation
|
@modelcontextprotocol/client@modelcontextprotocol/codemod@modelcontextprotocol/server@modelcontextprotocol/server-legacy@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
Uh oh!
There was an error while loading. Please reload this page.
…) docs, icons example
- mcpParamHeaders: gate the integer/number numeric comparison on a strict
decimal pattern so bare Number() never coerces hex / exponent / padded
forms into a false equality with the body value (with test).
- client.ts: _onnotification comment now names ClientResponseCache.write
(the encapsulated home of the race guard; _cacheListResult is gone).
- docs/client.md: add "Skipping the probe: connect({ prior })" under the
version-negotiation section, covering ConnectOptions.prior,
getDiscoverResult(), discover(), and the gateway/ example. New
type-checked snippet region in clientGuide.examples.ts.
- examples/tools: advertise an icon on the calc tool and assert it
reaches tools/list — first self-verifying coverage of the icons field.cd90c2e to
d159ef6CompareThere was a problem hiding this comment.
LGTM — the body-side coercion concern from my earlier comment is addressed (the gate now applies only to the decoded header, with on the body side plus a regression test for the exponent-form case).
Extended reasoning...
Overview
This is a small fast-follow PR touching three independent areas: (1) the SEP-2243 validateMcpParamHeaders numeric comparison in packages/core/src/shared/mcpParamHeaders.ts, now gated by a strict-decimal regex on the decoded header value and a typeof bodyRaw === 'number' check on the body side; (2) a comment rename in packages/client/src/client/client.ts (_cacheListResult → ClientResponseCache.write); (3) docs/example additions — a connect({ prior }) subsection in docs/client.md with a type-checked snippet, and an icons entry in the examples/tools/ story with a client-side assertion.
Prior review state
On the first revision I flagged that the strict-decimal gate was applied to the body-derived string as well, which would reject numerically-equal pairs whose body value stringifies in exponent form (e.g. 0.0000001 → '1e-7'). The current revision (d159ef6) implements exactly the suggested fix — header-side regex gate plus typeof bodyRaw === 'number' body-side gate comparing Number(decoded) === bodyRaw — and adds a regression test covering the exponent-form body value as well as the hex/exponent header rejection cases. No new issues were found in this run.
Security risks
The only logic change tightens header/body cross-check semantics: looser numeric forms in the attacker-controllable header (0x2a, 4.2e1, padded strings) no longer coerce into a false equality, which is a strict hardening relative to the previous bare Number() comparison. No auth, crypto, or data-exposure surface is touched. The docs note about reusing a persisted DiscoverResult only across the same authorization context is appropriate.
Level of scrutiny
Moderate-low. The core change is a ~10-line validation tweak with direct unit-test coverage of both the new acceptance case and the new rejection cases; the rest is documentation, comments, and example coverage. The PR description reports the full check matrix (typecheck, lint, core suite, snippet sync, docs check, all example legs) passing. The missing changeset is consistent with this being a chore-level fast-follow on the v2 integration branch.
Other factors
The added test cases directly exercise the regression scenario raised in the previous review round, and the example/docs additions are self-verifying (snippet region is type-checked, the icons assertion runs in the example harness). Overall the changes are small, contained, and correct.
Uh oh!
There was an error while loading. Please reload this page.
Three small fast-follows on the v2 integration branch.
validateMcpParamHeaders: gate the integer/number numeric comparison on a strict-decimal pattern (/^-?\d+(\.\d+)?$/) so bareNumber()never coerces hex (0x2a), exponent (4.2e1), or padded forms into a false equality with the body value. Adds a regression test.client.ts_onnotificationcomment: now namesClientResponseCache.write(the encapsulated home of the race guard;_cacheListResultwas renamed in feat(client): minimal response-cache substrate (ResponseCacheStore + aggregate-then-write list*()) #2336).docs/client.md: new "Skipping the probe:connect({ prior })" subsection under the version-negotiation section, coveringConnectOptions.prior,getDiscoverResult(),discover(), and thegateway/example. Type-checked snippet region added inclientGuide.examples.ts.examples/tools/: thecalctool now advertises aniconsentry and the client asserts it reachestools/list— first self-verifying coverage of the icons field that the rebase onto main brought in.Motivation and Context
Post-merge nits flagged on #2327 / #2336 / #2350, plus the example-coverage gap noted under the rebase-onto-main audit.
How Has This Been Tested?
typecheck:all,lint:all, core suite (1285 tests),sync:snippets --check,docs:check,run:examples(65/65 legs,toolsstory across all four transport×era legs).Breaking Changes
None.
Types of changes
Checklist
Additional context
The
ClientOptions.responseCacheStoreJSDoc tense (the second comment nit on #2336) was already corrected in a subsequent PR; nothing to do there.