fix(vmcp): reject Authorization and Cookie in passthroughHeaders, as documented - #6235
Open
SashaMIT wants to merge 2 commits into
Open
fix(vmcp): reject Authorization and Cookie in passthroughHeaders, as documented#6235SashaMIT wants to merge 2 commits into
SashaMIT wants to merge 2 commits into
Conversation
shouldSkipInitialAuthorization skipped authorization for any POST whose Content-Type is not application/json. Such requests are never parsed as MCP, so message-level authorization cannot run on them - but the transparent proxy still forwards the body verbatim, and MCP backends parse JSON-RPC without checking Content-Type. A tools/call smuggled as text/plain therefore reached the backend with no Cedar evaluation. A non-JSON POST now falls through to the parsed-request check, which rejects it with 400. The codebase already treats non-JSON smuggling as in-scope: the batch guard runs independently of Content-Type for exactly this reason.
…documented The vMCP passthroughHeaders allowlist was validated only against middleware.RestrictedHeaders, whose omission of Authorization is deliberate for the standalone header-forward middleware (a test asserts it stays forwardable there). For vMCP passthrough the documented contract is stricter: virtualmcpserver-api.md promises Authorization is rejected at startup, and the sibling embeddingHeaders field already excludes it via CEL validation. Forwarding a caller's Authorization or Cookie verbatim to every backend is a credential-leak footgun, not a pass-through use case. Reject both at startup with the same error shape as the other restricted headers.
SashaMIT
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
jerm-dro,
jhrozek,
rdimitrov and
tgrunnagle
as code owners
August 7, 2026 07:19
jhrozek
reviewed
Aug 7, 2026
jhrozek
left a comment
jhrozek
left a comment
Contributor
There was a problem hiding this comment.
A couple of small nits from review, nothing blocking.
| package authz | ||
|
|
||
| import ( | ||
| "strings" |
Contributor
There was a problem hiding this comment.
The new "strings" import breaks alphabetical ordering in the stdlib block (it's placed before "bytes"). task lint will fail on this - running task lint-fix should reorder it automatically (it belongs between "os" and "testing").
| // documented contract is stricter: Authorization and Cookie are rejected at | ||
| // startup because forwarding caller-supplied credentials verbatim to every | ||
| // backend is a credential-leak footgun, not a pass-through use case. | ||
| vmcpRestricted := map[string]bool{ |
Contributor
There was a problem hiding this comment.
Minor nit: vmcpRestricted is a local map literal rebuilt on every call to validatePassthroughHeaders. The sibling set it's OR'd against, middleware.RestrictedHeaders, is a package-level var. This only runs at startup/config-validation time so there's no real perf concern, but hoisting this to a package-level var (e.g. vmcpRestrictedHeaders) would match the existing pattern.
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
2 participants
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.
Problem
validatePassthroughHeaderschecks names againstmiddleware.RestrictedHeadersonly. That map's omission ofAuthorizationis deliberate for its original consumer (the standalone header-forward middleware, whereTestCreateHeaderForwardMiddleware_AuthorizationAllowedasserts it stays forwardable) — but for vMCP passthrough the documented contract is stricter:The validator contradicts the doc:
passthroughHeaders: [authorization]starts cleanly, and the client'sAuthorization(orCookie) header is then forwarded verbatim to every backend — leaking the caller's incoming credential to all upstream MCP servers. The siblingembeddingHeadersfield already excludesauthorizationvia CEL validation (config.go:976), so the codebase's own convention says these headers don't belong in operator-configured forward lists.Fix
The vMCP validator rejects
AuthorizationandCookieat startup with the same error shape as the other restricted headers. The standalone middleware's behavior (and its test) is untouched — the stricter set lives in the vMCP validator where the documented contract applies.Tests
Added "Authorization is restricted" (lowercase input, exercising canonicalization) and "Cookie is restricted" cases to
TestValidator_ValidatePassthroughHeaders. Fullpkg/vmcp/configsuite passes.Related: #6234 (non-JSON POST authz skip) — same seam family, independent fix.
Made with Cursor
Made with Cursor