feat(server): createRequestStateCodec; pin conformance to main@d70d7ad; toNodeHandler onerror + ctx.log spec-align#2332
Conversation
🦋 Changeset detectedLatest commit: 75506c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
f2a3ec6 to
f0848e2
Compare
felixweinberger
force-pushed
the
fweinberger/on-tonodehandler
branch
from
f9069fa to
c8779f8
Compare
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
f0848e2 to
f4e4654
Compare
| // request method on that revision). The spec at 2026-07-28 | ||
| // says an absent key means the server MUST NOT send | ||
| // `notifications/message` for the request — so an absent key | ||
| // suppresses, it does not mean "send everything". On | ||
| // 2025-era connections the session-scoped level set via | ||
| // `logging/setLevel` applies exactly as before (an absent | ||
| // session level there continues to mean no filter). | ||
| let threshold: LoggingLevel | undefined; | ||
| if (this._servedModernEra()) { | ||
| threshold = ctx.mcpReq.envelope?.[LOG_LEVEL_META_KEY] as LoggingLevel | undefined; | ||
| if (threshold === undefined) { | ||
| return Promise.resolve(); | ||
| } | ||
| } else { | ||
| threshold = this._loggingLevels.get(undefined); | ||
| } | ||
| if (threshold !== undefined && this.LOG_LEVEL_SEVERITY.get(level)! < this.LOG_LEVEL_SEVERITY.get(threshold)!) { | ||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
🟡 Two follow-ups on the rewritten ctx.mcpReq.log() level filter: (1) the legacy branch reads the threshold via this._loggingLevels.get(undefined), but logging/setLevel stores the level under the transport session id, so on a sessionful 2025-era connection handler-emitted logs ignore the client-set level (this lookup pre-dates the PR, but these exact lines are rewritten and re-documented here — consult ctx.sessionId with a fallback to the undefined key); (2) the new modern-era behavior — an absent _meta['io.modelcontextprotocol/logLevel'] key now suppresses the notification entirely — is documented in the changeset and e2e test, but docs/server.md's Logging section still tells authors to call ctx.mcpReq.log() inside any handler with no mention that on 2026-07-28 requests delivery requires the per-request logLevel envelope key, so the documented pattern silently does nothing for the common case.
Extended reasoning...
Part 1 — legacy branch ignores the session-scoped logging/setLevel level (pre-existing).
The logging/setLevel handler (_registerLoggingHandler, server.ts ~290) stores the client's chosen level keyed by the transport session id:
const transportSessionId = ctx.sessionId || (ctx.http?.req?.headers.get('mcp-session-id') as string) || undefined;
this._loggingLevels.set(transportSessionId, parseResult.data);So on a sessionful Streamable HTTP connection the level lives under the session-id key. The legacy branch of the rewritten filter in buildContext (server.ts:336) reads only this._loggingLevels.get(undefined) — it never consults ctx.sessionId — so it finds nothing and applies no filter.
Step-by-step: (1) a 2025-era client on a sessionful Streamable HTTP transport sends logging/setLevel with level: 'warning'; the level is stored under the session id (e.g. 'abc-123'). (2) A tool handler calls ctx.mcpReq.log('debug', ...). (3) _servedModernEra() is false, so threshold = this._loggingLevels.get(undefined) → undefined. (4) The severity comparison is skipped (threshold !== undefined is false) and the debug notification is sent — even though the client asked for warning and above. By contrast, Server.sendLoggingMessage(params, sessionId) honors the per-session level via isMessageIgnored(level, sessionId), and the changeset/migration prose says ctx.mcpReq.log "respects the client's log level filter", so the behavior contradicts both the sibling API and the docs.
This lookup-by-undefined is byte-identical to the pre-PR ternary, so the PR did not introduce it — but the PR rewrites these exact lines into the new if/else and re-documents the legacy semantics in the comment ("the session-scoped level set via logging/setLevel applies exactly as before"), so this is the natural place to fix it. ctx.sessionId is in scope in buildContext; the fix is this._loggingLevels.get(ctx.sessionId) ?? this._loggingLevels.get(undefined) (or just reuse isMessageIgnored(level, ctx.sessionId)).
Part 2 — docs/server.md Logging prose now contradicts the modern-era behavior (introduced by this PR).
The new branch (server.ts:330-334) makes an absent _meta['io.modelcontextprotocol/logLevel'] key on a 2026-07-28-served request return early and emit nothing, where previously an absent key meant "no filter, send". The change is recorded in the changeset, and the e2e scenario (handler-context.test.ts) had to be updated to set the _meta key explicitly to keep the log delivered — which is itself evidence the default path now suppresses.
However, docs/server.md's "Logging" section (lines 345-376) — untouched by this PR — still tells server authors to declare the logging capability and "call ctx.mcpReq.log(level, data) inside any handler", with examples that just call ctx.mcpReq.log('info', ...). There is no mention of the per-request io.modelcontextprotocol/logLevel envelope key anywhere in docs/server.md or docs/client.md. After this change, the documented pattern silently does nothing for any 2026-era request whose client did not set that key — the common case, since nothing in the docs tells clients to set it. Per the repo checklist ("Bugfix or behavior change: check whether docs/**/*.md describes the old behavior and needs updating"), the Logging section should add a sentence noting that on 2026-07-28 requests delivery requires the client to set the per-request io.modelcontextprotocol/logLevel _meta key.
Impact and fix. Part 1: real correctness gap on the legacy sessionful path, but pre-existing — fix or note, not blocking. Part 2: documentation-only; one or two sentences in docs/server.md's Logging section close the gap. Neither blocks the PR.
There was a problem hiding this comment.
Part (1) is now addressed: the legacy branch of the ctx.mcpReq.log() filter in buildContext (packages/server/src/server/server.ts:342) reads this._loggingLevels.get(ctx.sessionId) ?? this._loggingLevels.get(undefined), so a sessionful 2025-era connection's logging/setLevel threshold is honored.
Part (2) is still open: docs/server.md's Logging section (lines 345-376) is unchanged — it still tells authors to declare the logging capability and call ctx.mcpReq.log(level, data) inside any handler, with no mention that on 2026-07-28-served requests delivery now requires the client to set the per-request io.modelcontextprotocol/logLevel _meta envelope key (an absent key suppresses the notification entirely under the new behavior in this PR). One or two sentences in that section would close the gap. Non-blocking.
| # NOTE: the SDK's modern-path rejection codes are aligned with what this | ||
| # referee asserts: header/body mismatches answer -32001 (HeaderMismatch) and a |
There was a problem hiding this comment.
🟡 The header NOTE in test/conformance/expected-failures.yaml (lines 9-13) still claims the SDK's modern-path rejection codes "are aligned with what this referee asserts" (-32001 / -32602), but the conformance#353 entries this same PR adds further down in this file (and in expected-failures.2026-07-28.yaml) state the new pin asserts -32020/-32021/-32022 while the SDK still emits -32001/-32003/-32004 — which is exactly why the new expected-failure entries exist. Rewrite or drop the NOTE (e.g. point at the #353 sections / the fweinberger/on-renumber follow-up) so the file's header doesn't contradict its own entries.
Extended reasoning...
What the inconsistency is. The PR repoints the conformance referee from the published 0.2.0-alpha.4 to a vendored build of main @ d70d7ad and rewrites the adjacent "Baseline established against..." paragraph (lines 4-7) of test/conformance/expected-failures.yaml to name the new pin. However, the NOTE immediately below it (lines 9-13) is carried forward unchanged: it still states that "the SDK's modern-path rejection codes are aligned with what this referee asserts: header/body mismatches answer -32001 (HeaderMismatch) and a missing _meta envelope (or missing protocolVersion key) answers -32602", and that mismatched cells would only be re-derived "if a future published conformance release changes those assignments".
Why it is now false at this pin. "This referee" now refers to the vendored 0.2.0-main.d70d7ad build, which carries the conformance#353 error-code renumber (spec#2907). The very entries this PR adds further down in the same file say so explicitly: under the new # --- conformance#353 error-code renumber (spec#2907) --- sections, the comments state the referee at this pin asserts -32020/-32021/-32022 for header/body mismatch, missing-required-client-capability, and unsupported-protocol-version, while the SDK "still emits -32001/-32003/-32004". The same wording appears in the new entries added to expected-failures.2026-07-28.yaml. The four #353-fallout expected failures (request-metadata, http-custom-header-server-validation, http-header-validation, server-stateless) exist precisely because the SDK's codes are no longer aligned with the referee — the future-tense escape hatch in the NOTE ("if a future ... release changes those assignments") has already happened at this pin, just via a vendored main build rather than a published release.
Step-by-step walk-through of the contradiction as the file stands after this PR:
- Lines 4-7 (rewritten by this PR): baseline is established against
0.2.0-main.d70d7ad. - Lines 9-10 (untouched): "the SDK's modern-path rejection codes are aligned with what this referee asserts: header/body mismatches answer -32001 (HeaderMismatch)...".
- The new server-section entry added by this PR for
http-custom-header-server-validation: "SDK still emits −32001/−32003/−32004; the referee at this pin asserts −32020/−32021/−32022 for header/body mismatch, ..." — i.e. the referee does not assert -32001 for header/body mismatch, directly contradicting step 2. - A reader (or the next person bumping the pin) following the header NOTE would conclude no error-code reconciliation is pending, when the file's own Notification on Capabilities #353 sections — and the
fweinberger/on-renumberbranch later in this stack, per the PR description — say the opposite.
Why nothing catches it. The expected-failures evaluator only checks scenario names against actual results (and stale entries); comment prose in the YAML is never validated, so the contradiction passes every gate silently.
Impact. Documentation/comment accuracy only — no runtime or CI behaviour is affected. The cost is reader confusion in the file that is supposed to be the authoritative record of why each conformance cell is expected to fail, which matters most when the next pin bump or the renumber follow-up reconciles this file.
Fix. Rewrite (or drop) the NOTE so it reflects the d70d7ad pin — e.g. note that at this pin the referee asserts the conformance#353-renumbered codes (-32020/-32021/-32022), the SDK still emits -32001/-32003/-32004, the affected cells are tracked in the #353 sections below (and in expected-failures.2026-07-28.yaml), and the SDK-side flip lands in fweinberger/on-renumber. The -32602 missing-envelope statement can be kept if it still holds, but the headline alignment claim should go.
felixweinberger
force-pushed
the
fweinberger/on-tonodehandler
branch
from
c8779f8 to
d6e2be7
Compare
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
f4e4654 to
50ac3ce
Compare
felixweinberger
force-pushed
the
fweinberger/on-tonodehandler
branch
from
d6e2be7 to
7327fb9
Compare
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
50ac3ce to
47f361b
Compare
| it('binds the version prefix into the MAC (a transplanted body.mac under a different prefix fails)', async () => { | ||
| const codec = createRequestStateCodec({ key: KEY }); | ||
| const wire = await codec.mint({ ok: true }); | ||
| // The MAC covers `"v1." + body`. The verifier hard-gates the prefix, | ||
| // so to assert the MAC binding we recompute it manually with `"v2."` | ||
| // over the same body and confirm the codec rejects that pair. | ||
| const body = wire.slice('v1.'.length, wire.lastIndexOf('.')); | ||
| const cryptoKey = await crypto.subtle.importKey('raw', KEY, { name: 'HMAC', hash: 'SHA-256' }, false, ['sign']); | ||
| const v2Mac = new Uint8Array(await crypto.subtle.sign('HMAC', cryptoKey, new TextEncoder().encode(`v2.${body}`))); | ||
| const b64url = (b: Uint8Array) => | ||
| btoa(String.fromCodePoint(...b)) | ||
| .replaceAll('+', '-') | ||
| .replaceAll('/', '_') | ||
| .replace(/=+$/, ''); | ||
| // Same body, MAC computed for a v2 prefix, presented under v1 — MAC fails. | ||
| await expect(codec.verify(`v1.${body}.${b64url(v2Mac)}`, fakeCtx('tools/call'))).rejects.toThrow('mac'); | ||
| }); |
There was a problem hiding this comment.
🟡 The 'binds the version prefix into the MAC' test asserts rejection of a MAC computed over 'v2.' + body presented under the v1. prefix — but any mismatching MAC satisfies that assertion (the sibling 'rejects a tampered MAC' test already covers it), so a hypothetical regression where mint/verify drop the prefix and MAC only the bare body would still pass this test. The discriminating case is the opposite construction: compute the MAC over the bare body (no prefix) and present it under v1. — the prefix-binding implementation rejects it, while a body-only-MAC regression would accept it. The shipped codec does bind the prefix correctly, so this is a test-effectiveness nit only.
Extended reasoning...
What the issue is. The new test 'binds the version prefix into the MAC (a transplanted body.mac under a different prefix fails)' (packages/server/test/server/requestStateCodec.test.ts:137-153) is intended to lock in the security property the PR/commit message highlights ('prefix bound into MAC'): mint MACs PREFIX + body, so a body.mac pair cannot be transplanted across codec versions sharing a key. As written, however, the test cannot distinguish a prefix-binding implementation from a regressed body-only-MAC implementation, so it provides no regression protection for that property.
The code path. The test mints a wire value, extracts the body, manually computes v2Mac = HMAC(key, 'v2.' + body), and asserts that codec.verify(\v1.${body}.${b64url(v2Mac)}`, ...)rejects with'mac'`.
Step-by-step proof that the assertion is non-discriminating:
- Correct (current) implementation:
verifyrecomputesHMAC(key, 'v1.' + body)(requestStateCodec.ts,subtle.verify('HMAC', ..., utf8.encode(PREFIX + body))). That differs from the presentedHMAC(key, 'v2.' + body)→subtle.verifyreturns false → throws'mac'. Test passes. ✓ - Hypothetical regression where both
mintandverifydrop the prefix and MAC only the bare body:verifyrecomputesHMAC(key, body). That also differs from the presentedHMAC(key, 'v2.' + body)→ still false → still throws'mac'. Test still passes, even though the domain-separation property it is named after is gone.
In other words the assertion is satisfied by any wrong MAC — which the sibling 'rejects a tampered MAC' test already exercises — so this test is functionally redundant with it.
Why nothing else catches it. No other test presents a MAC computed without the prefix, and the prefix-binding behavior is otherwise only documented in comments; a future refactor that accidentally moved the MAC input to the bare body would sail through the suite green.
Impact. Test-effectiveness only — the shipped codec is correct (both mint and verify use utf8.encode(PREFIX + body)), so there is no runtime or security bug. The cost is false confidence: the test named after the prefix-binding guarantee does not actually guard it.
How to fix. Make the manual MAC computation the genuinely discriminating case — sign the bare body (no prefix) and present it under v1. (or add this case alongside the existing one):
const bareMac = new Uint8Array(await crypto.subtle.sign('HMAC', cryptoKey, new TextEncoder().encode(body)));
await expect(codec.verify(`v1.${body}.${b64url(bareMac)}`, fakeCtx('tools/call'))).rejects.toThrow('mac');The prefix-binding implementation rejects this ('mac'); a body-only-MAC regression would accept it (the MAC matches, exp is valid, no bind tag) and the test would fail — exactly the regression the test exists to catch.
…he multi-round-trip requestState
`createRequestStateCodec({ key, ttlSeconds?, bind? })` returns `{ mint, verify }`:
`mint` HMAC-SHA256-seals a JSON-serializable payload (with TTL, default 600 s,
and optional context binding) into the opaque `requestState` wire string;
`verify` is the function consumers drop directly into
`ServerOptions.requestState.verify` (and call again in the handler to read the
payload back). WebCrypto-based and runtime-neutral; verification is fail-closed
and constant-time (`subtle.verify`). Thrown reasons are fixed opaque codes
(`malformed`/`mac`/`expired`/`bind`) — never decoded payload or binding values
— so the seam's `onerror` relay cannot leak principal identifiers.
The `ServerOptions.requestState.verify` return type is widened to
`unknown | Promise<unknown>` (the seam already awaited-and-discarded) so the
codec's payload-returning `verify` is directly assignable.
The hand-rolled HMAC in `examples/server/src/multiRoundTrip.ts` and the
conformance fixture switch to the helper (the
`input-required-result-tampered-state` scenario is now its conformance-side
proof).
…d7ad Vendors a tarball built from modelcontextprotocol/conformance@main (d70d7ad, post-0.2.0-alpha.4) and points the package.json devDependency at it via a file: spec, so #347 (client-scenario mocks serve server/discover) is available without waiting for the next published release. README documents the pin and how to revert to a published version. Baseline read against d6008bcd7 (no fixture changes in this commit): client:all 345p / 14f / 1w - stale baseline: auth/scope-step-up now passes (#337 gates the SEP-2350 scope-union WARNING to 2026-07-28+) - new unexpected: request-metadata (2f) — mock now rejects with -32020/-32022 (renumbered per spec#2907 in #353); SDK still emits and recognises -32001/-32004 - sep-2322-client-request-state, http-custom-headers, http-invalid-tool-headers all pass with the withLocalDiscoverResponse shim still in place; #347 makes those mocks serve server/discover natively, so the shim is now redundant (removal is a separate change) server:draft 68p / 4f - server-stateless 23p/1f, http-custom-header-server-validation 6p/3f, http-header-validation flagged — all #353 error-code renumbering (referee now asserts -32020/-32021/-32022; SDK emits -32001/-32004) - new on main: SEP-2663 tasks lifecycle scenario (#262), pending-list only — not in the draft suite - spec-types/draft.ts re-synced (#348)
felixweinberger
force-pushed
the
fweinberger/on-tonodehandler
branch
from
7327fb9 to
bb7a7ca
Compare
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
47f361b to
096183a
Compare
…ey snapshot, prefix bound into MAC, constant-time bind compare, fail-closed on unconfigured bind; align {fetch,close,notify,bus} doc-shape; reconcile conformance expected-failures to local main pin
felixweinberger
force-pushed
the
fweinberger/on-codec-conformance
branch
from
096183a to
75506c6
Compare
1 participant
Adds
createRequestStateCodec(opt-in HMAC sealing for the multi-round-triprequestState), pins the conformance referee to a local build ofmain @ d70d7ad, and folds in two follow-up fix rounds for earlier PRs in this stack. Stacked onfweinberger/on-tonodehandler.Motivation and Context
createRequestStateCodecThe multi-round-trip
requestStatehook expects callers to seal opaque state across the client round-trip; today every consumer hand-rollsnode:cryptoHMAC. NewcreateRequestStateCodec({ key, ttlSeconds?, bind? })in@modelcontextprotocol/serverreturns{ mint, verify }:subtle.importKey/sign/verify), runtime-neutral, constant-time MAC check"v1." b64url({"p":payload,"exp":unix,"b":bind?}) "." b64url(mac); TTL default 600 s; key ≥32 bytes elseRangeErrorbind?: (ctx) => stringfor principal/method binding — stored as a domain-separated keyed tag (b64url(HMAC(key, "mcp.requestState.bind:" + bind(ctx))[:16])), so the signed-not-encrypted body never carries a client-readable principal identifiermalformed/mac/expired/bind) — never decoded payload or binding valuesServerOptions.requestState.verifyreturn widenedvoid|Promise<void>→unknown|Promise<unknown>socodec.verifyis directly assignable (seam already awaited-and-discarded)examples/mrtr/server.tsandtest/conformance/src/everythingServer.tsswitch from hand-rolled HMAC to the helper; theinput-required-result-tampered-stateconformance scenario is now its proof. JSDoc andmigration.md§requestState carry the signed-not-encrypted note (payloadpis client-readable; use AEAD for confidentiality; the bind tag is keyed so it is not).Conformance referee pin
@modelcontextprotocol/conformancerepointed from0.2.0-alpha.4to a vendored tarball built fromorigin/main @ d70d7ad(file:./vendor/...tgz, version0.2.0-main.d70d7ad). README documents the rebuild recipe + tarball sha256. Whatmainbrings vs alpha.4: modelcontextprotocol/conformance#337 (gates the SEP-2350 scope-union WARNING), #347 (client-scenario mocks serveserver/discovernatively), #348 (re-syncs draft spec types), #353 (renumbers draft error codes per modelcontextprotocol/modelcontextprotocol#2907), #262 (SEP-2663 tasks-lifecycle scenario).expected-failures*.yamlreconciled so all gate legs exit 0; the four #353-fallout entries are annotated as renumber-pending (the SDK error codes are flipped infweinberger/on-renumber, later in this stack).Follow-up fixes folded in
ctx.mcpReq.log: absent_meta['io.modelcontextprotocol/logLevel']on a 2026-era request now suppresses (spec: absent → MUST NOT sendnotifications/message); legacy-era branch unchanged. The e2e scenario body sets the_metakey explicitly.toNodeHandler(handler, opts?): newopts.onerror(error)is called before the adapter-level 500 fallback when request-conversion /handler.fetchthrows; wrapped intry { … } catch {}so a throwing user callback never escapes past the 500/−32603 fallback. NewToNodeHandlerOptionsexported from@modelcontextprotocol/node.{ fetch, close, notify }→{ fetch, close, notify, bus }aligned in.changeset/create-mcp-handler.md,McpHttpHandlerJSDoc, andmigration-SKILL.md.How Has This Been Tested?
packages/server/test/server/requestStateCodec.test.ts(round-trip, body/MAC tamper, cross-key, malformed×4, expired, bind ok/mismatch-opaque/mint-requires-ctx, key-too-short, hook-assignability, decoded-body-never-carries-raw-bind)onerrorcalled; throwingonerrorswallowed)server:draft77p/0f at the codec commit (all 14input-required-result-*scenarios pass incl.tampered-state); after the pin reconcile all gate legs (client:all,client:2026,server,server:draft,server:2026) exit 0Breaking Changes
None.
createRequestStateCodecis opt-in;ServerOptions.requestState.verifywidening is backward-compatible.Types of changes
Checklist
Additional context
server:allremains red on the pre-existingjson-schema-2020-12/server-sse-pollingpair plus the new SEP-2663tasks-*pending-only scenarios from conformance#262 — not a gate leg, untouched. Reference shape: signed mode only; AEAD/HKDF deferred.