Skip to content

fix(core): reject a plain Mcp-Param or Mcp-Name value with invalid characters - #3063

Open
chrikrah wants to merge 3 commits into
modelcontextprotocol:mainfrom
chrikrah:fix/mcp-param-invalid-chars
Open

chrikrah wants to merge 3 commits into
modelcontextprotocol:mainfrom
chrikrah:fix/mcp-param-invalid-chars

Conversation

@chrikrah

Copy link
Copy Markdown

The server accepts a plain Mcp-Param-* or Mcp-Name value that holds a character outside 0x20–0x7E and tab, because decodeMcpParamValue (packages/core-internal/src/shared/mcpParamHeaders.ts:274) returns any value without the Base64 sentinel as it is. The 2026-07-28 spec says servers MUST reject such an Mcp-Param-* header. For Mcp-Name, Server Validation lists "a header value contains invalid characters" among the 400 / -32020 failures. Its Value Encoding section says the client must Base64-wrap such a value.

Node's HTTP parser passes a raw 0xE9 byte through as é, so the value reaches createMcpHandler. Before, at c05225a, through http.createServer and a raw socket:

$ npx tsx repro-e2e.mts    # tools/call route {region: "café"}, header "Mcp-Param-Region: caf\xE9"
node parsed mcp-param-region = "café"
HTTP/1.1 200 OK
{"result":{"content":[{"type":"text","text":"routed café"}],...},"jsonrpc":"2.0","id":7}

After:

HTTP/1.1 400 Bad Request
{"jsonrpc":"2.0","error":{"code":-32020,"message":"Bad Request: the request headers and body disagree: the Mcp-Param-Region header carries invalid characters or an invalid Base64 sentinel value",...},"id":7}

The new check uses the same character set as needsBase64 in the encoder. Both server checks get it: Mcp-Param-* and Mcp-Name in validateStandardRequestHeaders. The cost: a client that sends raw Latin-1 in these headers now gets 400 / -32020, not 200. The client in this SDK already wraps such values. v1.x has no SEP-2243 code, so this is for main only. A changeset is included. There is no issue for this.

Verification

# base c05225a, node v22.23.3, vitest 4.1.2
$ vitest run --maxWorkers=4   # packages/core-internal
Tests  1836 passed (1836)   # base
Tests  1838 passed (1838)   # this branch
$ vitest run --maxWorkers=4   # packages/server
Tests  674 passed (674)   # base
Tests  675 passed (675)   # this branch
# both production files reverted, tests kept:
$ vitest run test/shared/mcpParamHeaders.test.ts test/shared/standardHeaderValidation.test.ts   # core-internal
Tests  2 failed | 90 passed (92)   # expected 'café' to be undefined; expected undefined to be defined
$ vitest run test/server/mcpParamValidation.test.ts   # server
Tests  1 failed | 8 passed (9)   # expected 200 to be 400
$ pnpm --filter ./packages/core-internal --filter ./packages/server run lint   # eslint 9.39.4, prettier 3.6.2
All matched files use Prettier code style!
$ pnpm --filter ./packages/core-internal --filter ./packages/server run typecheck   # tsgo 7.0.0-dev.20260327.2
rc 0
$ vitest run scenarios/sep2243.test.ts coverage.test.ts   # test/e2e
Tests  2 passed (2); Tests  6 passed (6)
# not run: test/conformance

@felixweinberger, I kept the *-invalid-encoding cells for this rejection because the wire answer is the same -32020. Do you want a separate *-invalid-characters cell for it?

…aracters

Signed-off-by: Christopher Krah <48338417+chrikrah@users.noreply.github.com>
@chrikrah
chrikrah requested a review from a team as a code owner October 22, 2026 02:55
@changeset-bot

changeset-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f7476ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/core-internal Patch
@modelcontextprotocol/server Patch
@modelcontextprotocol/client Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/server-legacy Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3063

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3063

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3063

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3063

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3063

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3063

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3063

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3063

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3063

commit: f7476ad

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Oct 22, 2026
@0xamlab

0xamlab commented Oct 22, 2026

Copy link
Copy Markdown

Verified this against base c05225a plus this branch. On main, decodeMcpParamValue('café') returns 'café' because the non-sentinel path at packages/core-internal/src/shared/mcpParamHeaders.ts:276 hands back any plain value unchecked; this branch gates it at :279. Running pnpm vitest run test/shared/mcpParamHeaders.test.ts test/shared/standardHeaderValidation.test.ts in packages/core-internal gives 92 passed (62 + 30), while the same PR tests against base fail exactly once with "expected 'café' to be undefined". packages/server test/server/mcpParamValidation.test.ts passes 9/9. The interior-tab case ('a\tb') still decodes, matching the 0x09 allowance in the spec.

@arhancanli

Copy link
Copy Markdown

Read the diff (not run here). The shared hasOnlyPlainFieldChars between needsBase64 and decodeMcpParamValue is the right way to keep encoder and decoder from drifting, and the repro with a raw 0xE9 byte makes the gap clear.

Two checks I could not see in the tests: (1) 0x7F (DEL) and a C0 control such as 0x01 in the plain value. The predicate rejects them (c < 0x20 || c > 0x7e), but both tests use café, so a regression to a "non-ASCII only" check would still pass. (2) The same Latin-1 byte path through a real http.createServer request: Node hands UTF-8 bytes over as mojibake (café), which also now gets a 400. That seems right to me, but is the -32020 message ("invalid characters or an invalid Base64 sentinel") enough for a client author to tell this case from a bad sentinel, since the cell name is still *-invalid-encoding for both?

…cted

Both rejections read "invalid characters or an invalid Base64 sentinel
value", so a client author could not tell a plain value that needed
Base64 wrapping from a broken sentinel. The message now names the one
that applies; the cell stays *-invalid-encoding for both.

Also pin DEL (0x7F) and a C0 control (0x01) as rejected plain values,
since the earlier tests only used a non-ASCII character.

Signed-off-by: Christopher Krah <48338417+chrikrah@users.noreply.github.com>

@chrikrah chrikrah left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arhancanli the DEL/C0 tests and the message are in the new commit. I left out a real http.createServer test, because a Node request goes through toWebRequest and reaches the same decodeMcpParamValue check the café case pins. The message was not enough: it now says a plain value with a character outside 0x20-0x7E / 0x09, which has to be sent Base64-wrapped for your mojibake case and an invalid Base64 sentinel value for a broken sentinel. The cell stays *-invalid-encoding for both, since the spec gives them one row. The test now pins a\x7fb and a\x01b as rejected next to café.

$ pnpm exec vitest run test/shared/mcpParamHeaders.test.ts test/shared/standardHeaderValidation.test.ts   # packages/core-internal, new commit on effb6d6, Node v22.23.3
Tests  92 passed (92)
# both src files from effb6d6, new assertions kept
Tests  2 failed | 90 passed (92)   # the sentinel and the plain-value message checks
$ pnpm exec vitest run test/server/mcpParamValidation.test.ts   # packages/server
Tests  9 passed (9)
$ pnpm run check   # packages/core-internal: tsgo, eslint, prettier
All matched files use Prettier code style!

You raised the client-author view, so does the plain-value wording tell you what to fix?

@arhancanli

Copy link
Copy Markdown

Read describeMcpParamDecodeFailure at abf71d5 (not run). Yes, the plain-value wording tells a client author what to fix: "a character outside 0x20-0x7E / 0x09, which has to be sent Base64-wrapped" names both the cause and the remedy, and the sentinel case now reads differently. Keeping one *-invalid-encoding cell for both matches the single spec row, and the café / a\x7fb / a\x01b trio pins the boundary on each side.

One edge to check: a value that starts with =?base64? but has no closing ?= falls through to the plain-value message. If it also holds only plain characters it is accepted as a literal, which is fine; if it holds a non-plain one, the message points at wrapping, which is still the right fix. Is that open-sentinel-without-suffix case covered by a test, or deliberately left to the plain-value path?

A value that starts with =?base64? but lacks the ?= suffix is not a
sentinel, so it decodes as a literal when its characters are plain and is
rejected with the plain-value message when they are not.

Signed-off-by: Christopher Krah <48338417+chrikrah@users.noreply.github.com>

@chrikrah chrikrah left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arhancanli It's the plain path on purpose: SEP-2243's edge-case table lists the missing-suffix value as a literal, so the plain-character check then applies as usual. It had no test, and the new commit pins it. One overlap stays as it was before this PR: the prefix followed by a single = reuses the prefix's closing question mark as its suffix, matches the spec's starts-with/ends-with pattern, and decodes to the empty string.

# packages/core-internal, Node v22.23.3, new test on top of abf71d5
$ pnpm exec vitest run test/shared/mcpParamHeaders.test.ts test/shared/standardHeaderValidation.test.ts
Tests  92 passed (92)              # abf71d5
Tests  93 passed (93)              # with the new test
#   '=?base64?SGVsbG8='  -> decoded as the literal
#   '=?base64?café'      -> rejected, "has to be sent Base64-wrapped"
# with decode/describe keyed on the prefix alone, same tests:
Tests  1 failed | 92 passed (93)
$ pnpm run check
All matched files use Prettier code style!
$ node -e 'const v="=?base64?=";console.log(v.startsWith("=?base64?")&&v.endsWith("?="), JSON.stringify(v.slice(9,v.length-2)))'
true ""

That's the last open point from your read, so the branch stays as it is now until a maintainer reviews it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants