803 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Aircg
|
4608cda997 |
mcp: retire only the subscriptions a listen registered (#1275)
Some checks failed
CodeQL Advanced / Analyze (go) (push) Has been cancelled
CodeQL Advanced / Analyze (actions) (push) Has been cancelled
Conformance Tests / server-conformance (push) Has been cancelled
Conformance Tests / client-conformance (push) Has been cancelled
Scorecard supply-chain security / Scorecard analysis (push) Has been cancelled
Test / lint (push) Has been cancelled
Test / test (1.25) (push) Has been cancelled
Test / test (1.26) (push) Has been cancelled
Test / race-test (push) Has been cancelled
*Narrowed after review — see the discussion below. The earlier revision
changed the
registry keying; this one changes only the teardown.*
### The problem
`subscriptionsListen` registers a session according to what the listen
asked for:
```go
if allowed.ToolsListChanged {
s.toolChangeSubscriptions[req.Session] = requestID
}
if allowed.PromptsListChanged {
s.promptChangeSubscriptions[req.Session] = requestID
}
if allowed.ResourcesListChanged {
s.resourceChangeSubscriptions[req.Session] = requestID
}
```
Its deferred cleanup does not:
```go
delete(s.toolChangeSubscriptions, req.Session)
delete(s.promptChangeSubscriptions, req.Session)
delete(s.resourceChangeSubscriptions, req.Session)
```
It runs whether or not this listen ever wrote those registries. A
session can
hold several listens at once, so the first one to unwind takes the
others'
registrations with it, and those streams stay open while receiving
nothing.
The scope is the persistent-session transports, stdio and in-memory.
Over
streamable HTTP the registries cannot collide: a stateful handler
refuses
SEP-2575 requests outright with `CodeUnsupportedProtocolVersion`, and a
stateless one builds a temporary session per POST and closes it when the
request ends.
Reachable through this SDK's own client, on stdio:
1. `Client.Connect` opens the auto-listen for list-changed
notifications.
2. `ClientSession.Subscribe(uri)` opens a second listen, carrying only a
resource subscription.
3. `ClientSession.Unsubscribe(uri)` retires that second listen.
4. Its cleanup deletes `toolChangeSubscriptions[session]`, which belongs
to the
auto-listen.
5. The auto-listen is still open and never receives `tools/list_changed`
again.
### How I pinned it down
A diagnostic run over the three states, reading the registries and
`ServerSession.listenIDs` directly:
```
after connect toolSub=2 promptSub=2 resSub[r1]=false listenIDs=[{2}]
after Subscribe toolSub=2 promptSub=2 resSub[r1]=true listenIDs=[{2} {3}]
after Unsubscribe toolSub=<nil> promptSub=<nil> resSub[r1]=false listenIDs=[{2}]
```
Line 2 rules out the second listen having overwritten the entry:
`toolSub` is
still 2, listen 3 never wrote that registry. Line 3 rules out the client
cancelling the wrong stream: `listenIDs` still holds listen 2, so the
auto-listen is alive on the server, parked on `<-ctx.Done()`. What
changed is
only that its registration is gone, deleted by listen 3's teardown.
### The change
Guard each delete with the condition that guarded the registration, so a
listen
retires what it registered and nothing else.
### Validation
Two tests, next to the existing `TestSubscriptionsListen_*` ones and
reusing
their fixtures. Against `
|
||
sravan kilaru
|
3b917b466c |
oauthex: build defaultDiscoveryTransport lazily instead of at import (#1278)
Fixes #1276 ## What's happening `oauthex` builds `defaultDiscoveryTransport` as a package-level var, so it runs at import time. That path calls `proxyConfigured`, which invokes `http.DefaultTransport`'s proxy func (`http.ProxyFromEnvironment`). net/http reads the proxy env vars only once and caches the result, so this import-time call freezes the proxy config before `main()` can set `HTTPS_PROXY`. New in v1.8.0, which is why it's a regression — just importing `.../mcp` (which pulls in `oauthex`) triggers it, even with no use of the SDK. ## Fix Wrap the setup in `sync.OnceValue` so it runs on first use instead of at import. Still built once and reused. ## Testing (removed) - Ran the issue's repro against this branch via a local `replace`: prints `proxy=http://127.0.0.1:9` now, `proxy=<nil>` before. - Added a regression test in `oauthex`. - `go test ./...` passes. Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Guglielmo Colombo
|
b58aa8d8d8 |
mcp: ignore notification in stdio batches (#1277)
Fixes #1256 |
||
José M. Requena Plens
|
826e653c27 |
mcp: refuse a call on a legacy session before initialize (#1273)
Closes #1271. `Server.handle` refuses a call made before `initialize` on a legacy session, and that refusal lives in the `default` branch of its method switch, so any method with a `case` of its own skips it. Three are served on a session that never handshook: `logging/setLevel`, `resources/subscribe` and `resources/unsubscribe`. `resources/subscribe` is the one that reaches state rather than merely answering: it starts a watcher and registers a subscriber, so a client that subscribes before `initialize` is delivered `notifications/resources/updated` for the lifetime of a session the server never agreed to. The check now runs after the switch, for every call carrying no new-protocol `_meta`, with `initialize` and `ping` exempt. That is the same condition the `default` branch already applied and the same error it produced; the change is that the three methods above stop skipping it. `ping` stays allowed because the lifecycle page exempts it. `server/discover` is deliberately left alone: a legacy request there already receives method-not-found, which tells the client more than an initialization error would. The two notification cases stay outside, since the gate covers calls and a notification carries no refusal. The SEP-2575 path is untouched. A session without an `initialize` handshake is exactly what that proposal is for, so the exemption is the request's own `_meta.protocolVersion` rather than anything about the session, and a new-protocol client is served as it is today. Tests: `TestServerHandle_LegacyCallBeforeInitialize` drives each affected method on a fresh session and asserts the refusal, that `resources/subscribe` runs no handler and leaves no subscriber, and that `initialize` and `ping` are served. `TestServerHandle_NewProtocolCallWithoutInitialize` asserts a new-protocol session with no handshake still gets `tools/list`, and that the three methods keep their existing method-not-found rather than the initialization refusal. Without the change, the setLevel, subscribe and unsubscribe rows fail. `go test ./mcp/ -count=1` is green, `gofmt -l` is empty and `go vet ./mcp/` is clean. This does not depend on #1266 and rebases onto it cleanly, verified with the suite green on the combination. One decision worth your view: the refusal is a plain error, which on the wire carries code 0, exactly as the `default` branch produced before this change. The specification prescribes no code for it. If you would rather it were `-32600` or `-32603`, say so and I will change it here. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Luca Candela
|
3785c500b5 |
mcp: preserve structured content number precision (#1239)
CallToolResult currently decodes structuredContent through an any field, which converts JSON numbers to float64 and silently rounds integers outside the IEEE-754 safe range. This change decodes structuredContent separately with UseNumber, preserving wire numbers as json.Number while retaining the existing decoded object, array, and primitive shapes. The field documentation now describes that representation. Users who depend on the previous float64 behavior can temporarily restore it with MCPGODEBUG=structuredcontentfloat64=1. An end-to-end in-memory client/server regression covers 9007199254740993 from a low-level Server.AddTool result. Additional coverage verifies the compatibility option restores the legacy rounded float64 value. Verification: - gofmt -l . - go test ./... - go vet ./... Fixes #1236 --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
rishabh
|
fbd36cb787 |
docs/server: fix logging defaults and the SetLoggingLevel method name (#1251)
The Logging section of docs/server.md says "For stateful sessions" twice in a row with opposite meanings, so one of the two sentences has to be wrong. The default level of "info" is set in only one place in the SDK, `StreamableHTTPHandler.ephemeralConnectOpts` in mcp/streamable.go, which handles stateless requests; a session created through `Server.Connect` starts with an empty `ServerSessionState.LogLevel`, and `ServerSession.Log` returns early while the level is empty, which is exactly what the first sentence already says. The second sentence now reads "For stateless sessions". The same section also tells readers to call `SetLevel`, but there is no such exported method: the client-side call is `ClientSession.SetLoggingLevel`, and `setLevel` is only the unexported server handler, so the pkg.go.dev link on the page points at an anchor that does not exist. For context, the stateless default arrived in #411 and the paragraph describing it was written later in #497, which is where the duplicated "stateful" came from. Checked with `go doc ./mcp ClientSession` (only `SetLoggingLevel` is exported) and `grep -rn 'LogLevel = "info"' mcp/` (a single hit, mcp/streamable.go:516); docs/server.md was regenerated with `go generate ./internal/docs`, and `go build ./... && go test ./...` pass. --------- Signed-off-by: Rishabh <rishabhsaiv@gmail.com> Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
jackyyy
|
c1918e19ad |
mcp: preserve SSE events buffered after the endpoint (#1270)
SSEClientTransport.Connect creates one event scanner to read the endpoint event and a second scanner to read subsequent messages. Each scanner owns a buffered reader, so bytes read ahead by the first scanner are discarded when it is abandoned. A message coalesced with the endpoint can therefore disappear while the connection remains open. Reuse the event scanner across the endpoint handshake and the message-reading goroutine. This preserves unread bytes without changing the public API. The regression test exercises Connect and Read with a controlled HTTP response body, covering both a message coalesced with the endpoint and a message arriving after Connect returns. On unmodified main, the separate-message control passes and the coalesced case times out; both pass with the fix. Validation on linux/amd64 with Go 1.26.8: - go test ./... - go test -race ./... - go vet ./... - staticcheck v0.6.1 ./... - gofmt and git diff --check The HTTP response boundary is simulated; no third-party server or proxy was used. This change concerns the legacy SSE transport. The Streamable HTTP conformance suite was not run. |
||
|
rishabh
|
0c0eb23625 |
docs/client: fix the SEP-1034 and SEP-1330 links (#1252)
The "Schema defaults and enums" section of docs/client.md links SEP-1034 to https://modelcontextprotocol.io/seps/1034 and SEP-1330 to https://modelcontextprotocol.io/seps/1330, and both of those bare-number URLs are dead. modelcontextprotocol.io serves SEPs at number-plus-slug URLs, which is what every other SEP link in this repo already uses (seps/2575-stateless-mcp, seps/2577-deprecate-roots-sampling-and-logging, seps/2596-feature-lifecycle-and-deprecation-policy, seps/2207-oidc-refresh-token-guidance); these two were the only bare-number ones left. I took the replacement slugs from the site's own sitemap.xml, so the `1034--` double hyphen is what is published rather than a typo here. curl -sIL on the two old URLs returns 404 and 404, and on the two new ones 200 and 200. The edit is in internal/docs/client.src.md with docs/client.md regenerated by `go generate ./internal/docs`, and `go build ./... && go test ./...` pass. Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
José M. Requena Plens
|
3f43bcf012 |
mcp: add HasParams so middleware can tell whether params are present (#1269)
This adds `mcp.HasParams(req Request) bool`, so a receiving middleware
can ask whether the message it was handed carried a `params` member.
The member is optional for several methods: `serverMethodInfos` sets
`missingParamsOK` for `server/discover`, `ping`, the four list methods
and the `cancelled`, `initialized` and `roots/list_changed`
notifications, so `{"jsonrpc":"2.0","id":1,"method":"tools/list"}` is a
complete request. For one of those, `unmarshalParams` returns a nil
`Params`, `newRequest` leaves `ServerRequest.Params` at the zero value
of `P`, and `GetParams` hands the middleware a non-nil interface holding
a typed nil pointer. Comparing that against nil says params are present,
and the first method called on it dereferences: `GetMeta` is promoted
from the embedded `Meta`. `Params` already declares `isNil` for
precisely this case, and it is unexported, so from outside the package
the accessor named after the question is the one thing that cannot be
reached.
`HasParams` is a function over `Request` rather than an exported method
on `Params` for two reasons. A middleware holds a `Request`, and this
way the caller never has to hold the value that panics. More
importantly, `isNil` cannot answer the question for every `Params`
value, so exporting it would ship a predicate that is itself unsafe: a
params type declared the documented way for a custom method, `struct{
mcp.ParamsBase }`, promotes `isNil` through a field selector, and that
dereferences the nil outer pointer before the method body runs. The
predicate therefore has to be computed from outside the value, and
`reflect.Value.IsNil` is what works for all of them.
Three judgement calls, stated so nobody has to find them.
First, `getRequestMeta` now goes through `HasParams`. That is a second
fix rather than a refactor: `AddReceivingCustomMethod` registers with
`missingParamsOK` too, so a client omitting `params` for a custom method
made `ServerRequest.ProtocolVersion`, `ClientInfo` and
`ClientCapabilities` panic in the promoted `isNil`. They now return
`""`, `nil` and `nil`. I could have left it out to keep the diff to one
function, but then the SDK would export a predicate that says to ask
before touching params while three of its own accessors still panicked
on the same value. If you would rather see it as a separate commit, say
so and I will split it.
Second, I did not add the predicate to the `Request` interface.
`Request` is sealed by `isRequest`, so a method would have been a
compatible change and would read slightly better at the call site, but a
free function leaves the interface alone and serves every `Request` just
as well.
Third, this reflects once per call of `getRequestMeta`. It is one
`reflect.ValueOf` on a value that is already an interface, on a path
where `newResult` already reflects per request.
Tests, both in `mcp/shared_test.go`.
`TestServerRequest_PerRequestAccessors_MissingParams` is the one that
fails before the change and passes after: on `main` it panics inside
`getRequestMeta` with `invalid memory address or nil pointer
dereference` from `(*customParams).isNil`, and here it asserts the three
accessors return zero values. `TestHasParams` covers the exported
behaviour: it builds exactly what `handleReceive` hands the middleware
chain for `tools/list`, `prompts/list`, `resources/list` and
`notifications/initialized` with no `params` member, asserts that
`GetParams` still returns a non-nil interface so the case under test is
still the typed-nil one, asserts `HasParams` reports false, and asserts
it reports true when params are present.
Behaviour an existing user would see change: only the three accessors
above, and only for a custom-method params type built on `ParamsBase`
whose `params` member was omitted, where a panic becomes a zero value.
Nothing about the wire format changes, no exported signature moves, and
no interface gains a method. A middleware that still writes
`req.GetParams() != nil` and then dereferences still panics; this gives
a caller the way to ask, it does not rescue a caller that does not.
Also here: one sentence on the doc comments of both
`AddReceivingMiddleware` methods saying a received message need not
carry params, since that is where a middleware author is reading, and
`examples/server/middleware/main.go` now logs `has_params` with
`mcp.HasParams` instead of the comparison that cannot work. The
example's printed output is unchanged, because the SDK client always
sends a `params` member for the methods it drives.
`go test ./...` and `gofmt -l .` are clean.
One thing to say plainly, since it is yours to decide and not mine: this
adds an exported symbol. `HasParams` is a new name in the public API of
`mcp`, and everything else here is doc comments, an example and the
three accessors. If you would rather not grow the surface for this, the
smaller answer is to keep the `getRequestMeta` fix and the doc-comment
sentence and drop the exported function, and a middleware author then
has the guarantee written down where they are reading even though they
still cannot ask the question in code. I prefer the function because the
guarantee alone leaves every middleware writing the same three lines of
`reflect`, and the one that does not write them panics on an ordinary
`tools/list`, but I will cut it to whichever shape you want.
Fixes #1261.
Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
|
||
|
José M. Requena Plens
|
a82c86a6cd |
mcp: classify the per-request protocol version by membership (#1268)
`validateRequestMeta` decided whether a request follows the 2026-07-28
protocol by comparing `_meta.protocolVersion` against `2026-07-28` with
`<`. This replaces that comparison with a membership test against
`supportedProtocolVersions`, which is the rule `mcp/streamable.go:354`
already applies to the `Mcp-Protocol-Version` header.
Nothing changes for a version the SDK supports: one predating 2026-07-28
still selects the legacy handshake, and one at 2026-07-28 or later is
still validated as the full `_meta` triple. What changes is a version
the SDK does not support at all. It now reports `usesNewProtocol`, so it
reaches the `CodeUnsupportedProtocolVersion` check in
`(*ServerSession).handle` and is answered with the error the versioning
page requires.
Driving a stdio server with `_meta.protocolVersion` set to `1900-01-01`,
the worked example on that page, before:
```json
{"jsonrpc":"2.0","id":1,"error":{"code":0,"message":"method \"tools/list\" is invalid during session initialization"}}
```
and after:
```json
{"jsonrpc":"2.0","id":1,"error":{"code":-32022,"message":"unsupported protocol version","data":{"supported":["2026-07-28","2025-11-25","2025-06-18","2025-03-26","2024-11-05"],"requested":"1900-01-01"}}}
```
There are two judgement calls in the diff, and I would rather state them
than leave them to be found.
The first is that an unsupported version returns early with
`initializeParams` carrying only that version, and the rest of the
`_meta` triple is left unvalidated. The shape of `_meta` is defined by
the revision the request names, and that is a revision this SDK does not
know, so there is nothing to validate it against; answering "missing
`io.modelcontextprotocol/clientCapabilities`" would send the client off
to fix the wrong thing while its real problem is the version. The partly
populated value cannot escape: `handle` runs the unsupported check
before anything reads the capabilities, and `Server.protocolVersions` is
always a subset of `supportedProtocolVersions`, so a version that fails
the membership test in `validateRequestMeta` always fails it in `handle`
too.
The second is that an empty `protocolVersion` still means "no version
declared" rather than "unsupported version". `mcp/streamable.go` already
reads the field that way when it decides whether a request carries a
version at all (`metaVersion != ""`), and rejecting the empty string
here would put the two layers back into disagreement, in the other
direction. The `!ok` and `== ""` cases are therefore handled together.
**Behaviour an existing user would see change.** A request whose
`_meta.protocolVersion` is a string not in `SupportedProtocolVersions()`
is now refused with `-32022`, where before a value sorting below
`2026-07-28` was treated as a legacy request: served if the session was
already initialized, refused with the session-initialization error if it
was not. No published protocol revision is in that set, so a client
speaking any released version of MCP sees no difference. A deployment
that put a sentinel or placeholder version in `_meta` and relied on it
being ignored would see those requests refused, which I believe is the
right answer but is a visible change.
This is not limited to stdio, and I want to be precise about where the
header gate does and does not stand in front of it. The gate at
`mcp/streamable.go:354` refuses an unsupported `Mcp-Protocol-Version`
only when that version **also** sorts below `2026-07-28`, so an unknown
version sorting above it (`2027-01-01`, say) passes the gate and reaches
this classification; the deprecated HTTP+SSE transport in `mcp/sse.go`
has no header check at all; and stdio, the in-memory transport and any
custom transport have none either. The window the old comparison left
open is therefore reachable on every transport, and a value sorting
above `2026-07-28` with an incomplete `_meta` triple also changes
answer, from `-32602` to `-32022`.
**The test that pins it** is
`TestServerUnknownProtocolVersion_NewProtocol` in `mcp/server_test.go`.
It drives `(*ServerSession).handle` with a `tools/list` request whose
`_meta.protocolVersion` names a version the SDK does not know, and
asserts that the returned error is a `*jsonrpc.Error` with code
`CodeUnsupportedProtocolVersion`, `Data.Requested` equal to the version
asked for, and `Data.Supported` equal to `SupportedProtocolVersions()`.
Four cases: `1900-01-01`, `2025-11-24`, `1.0`, and `1900-01-01` again
with the rest of the triple omitted, which is the case that pins the
first judgement call above. Before the change all four fail with `handle
returned method "tools/list" is invalid during session initialization,
want a *jsonrpc.Error`. `TestValidateRequestMeta` in
`mcp/shared_test.go` gains four cases at the unit level: two that fail
before the change, and two that guard the classifications which must not
move, a supported pre-2026-07-28 version and an empty one.
`go test ./...` and `gofmt -l .` are both clean.
Fixes #1260.
|
||
|
José M. Requena Plens
|
3210c2b34c |
mcp: carry a cancelled notification's reason as the request's cancel cause (#1255)
The canceller read `CancelledParams.Reason` off the wire and dropped it: `Connection.Cancel` cancelled the request's context with a nil cause, so a handler that looked at `context.Cause` saw a bare `context.Canceled` and nothing of what the peer said. The specification asks implementations to log cancellation reasons, and the handler is the one place that can act on one; #1100 had already given the request context a cause, so the reason has a natural place to travel. `Connection.CancelCause` is `Cancel` with a cause. The canceller now cancels with an error that carries the peer's reason and unwraps to `context.Canceled`, so `errors.Is` keeps classifying the cancellation as one while `context.Cause` reads `request cancelled by the peer: <reason>`. It also logs the id and the reason at debug level through the logger the connection already carries, which is the SDK's own answer to the SHOULD. No exported API changes: the cause type stays unexported and is read through `context.Cause` and `Error()`. `TestCancellation` now asserts the cause the SDK's own client produces (its context's error, which the client sends as the reason), and `TestCancellationReason` writes the call and the cancelled notification to the connection directly with a reason the client would never send, on a session pinned to 2025-11-25 so the hand-written request needs no `_meta`. Both fail without the change. Fixes #1254 Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Ronald
|
e40f35d137 |
mcp: label resultType on results that bypass the dispatcher (#1226)
Fixes #1225. See the [issue comment](https://github.com/modelcontextprotocol/go-sdk/issues/1225#issuecomment-5500818546). ## The bug `2026-07-28` requires `resultType` on every result. `CallToolResult`, `GetPromptResult`, and `ReadResourceResult` keep `resultType` in an unexported field. Each of these results can be `complete` or `input_required`, so it cannot embed `completeResultWithType` the way the seven types in #1060 do. Only `handleMultiRoundTripResult` sets that field. It runs inside `Server.callTool`, `getPrompt`, and `readResource`. Some middleware returns a result directly instead of calling `next` — for example, an auth, rate-limit, or entitlement gate. That middleware never reaches `handleMultiRoundTripResult`. The response then ships without the field, and strict clients reject it. ## The fix The choice between `complete` and `input_required` now happens inside `setMultiRoundTripResultType`. Both call sites that need this choice call that function. `setCompleteResultType` now labels results it did not label before, so it is renamed `annotateResultType`. The new function picks a branch with a type switch. Reading matters here. `setResultType` is unexported, so setting `InputRequests` is the only way for middleware to ask for input. If the code always labels results `complete`, the response ships as `{"resultType":"complete","inputRequests":{...}}`. A client that trusts the discriminator then drops the elicitation request. Recomputing the label every time is safe. A result that did reach the dispatcher already carries the same label from that test. This also covers one edge case: a handler that returns `InputRequests` again after the shim's single reinvocation. Before this fix, that case shipped with no `resultType`. Embedding `completeResultWithType` instead is not an option: it labels everything `complete` and overwrites `input_required`, breaking multi-round-trip flows. **Skip nil results.** Middleware can return a typed nil value, which satisfies the `Result` interface. `handle` now checks for that case before it calls the labeling function. Before this change, that same case also panicked, one frame later in `annotateServerInfo` and then `GetMeta`. ## Checks - `TestServerSessionHandle_SetsResultTypeWhenMiddlewareShortCircuits` fails on all three methods before the first change. Its fourth case fails if labeling overwrites a populated `InputRequests`. - `TestServerSessionHandle_ResultTypeGate` records which requests carry `resultType`, including one known gap. See "Separate from this PR" below. - `TestServerSessionHandle_NilResultFromMiddleware` panics without the nil check. - The complete-path cases added to `TestServerSessionHandle_SetsResultTypeOnNewProtocol` also pass before this change. They close a gap in #1060, which added only three cases, and only for `input_required`. - The exported API is unchanged. `go doc -all ./mcp`, with comments stripped, is identical before and after this change. - With the reproduction from the issue, over the streamable transport: the dispatched `tools/call` and the short-circuited `tools/call` both now return `resultType: "complete"`. I could not run `./scripts/conformance.sh`. CONTRIBUTING.md names that script, but the repo only has `client-conformance.sh` and `server-conformance.sh`. Both scripts need GNU `timeout`, and macOS does not include it. ## Also here Ten comments named an exported `ResultType` field and `ResultTypeComplete` / `ResultTypeInputRequired` constants. None of those exist. Two of the comments were `[CallToolResult.ResultType]` doc links, and those links cannot resolve. They now point at `[CallToolResult.NeedsInput]` instead, because an unexported field is also not a valid link target. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Max Freedom Pollard
|
5bc078a795 |
auth: strip a terminating slash from the issuer before building metadata URLs (#1245)
An issuer identifier written with a terminating slash, like `https://auth.example.com/`, makes authorization server metadata discovery miss every endpoint. `GetAuthServerMetadata` probes three URLs that no conformant server serves, finds nothing, and the caller falls back to the guessed 2025-03-26 endpoints, which are themselves malformed because they are built by appending to the same slash-terminated issuer. `authorizationServerMetadataURLs` in `auth/shared.go` decides whether the issuer has a path component with `baseURL.Path == ""` (line 47), and a trailing-slash issuer has `Path == "/"`, so it takes the path-insertion branch. That branch inserts `strings.TrimLeft(originalPath, "/")` (lines 59 and 62), which strips the leading slash but keeps the trailing one. For `https://auth.example.com/` the probes come out as `/.well-known/oauth-authorization-server/`, `/.well-known/openid-configuration/` and `//.well-known/openid-configuration`. For `https://auth.example.com/tenant1/` the two path-insertion probes come out with a trailing slash. Only the path-appending form was already right, because it uses `strings.Trim`. RFC 8414 section 3.1 settles it: "If the issuer identifier value contains a path component, any terminating "/" MUST be removed before inserting "/.well-known/" and the well-known URI suffix between the host component and the path component." So `https://auth.example.com/` has no path component and gets the two root forms, which is the list the MCP authorization spec gives for issuers without a path component. The change trims slashes off both ends of the issuer path once, uses that to pick between the root and path-insertion branches, and reuses it for all three URLs. An issuer without a terminating slash produces exactly the same list as before. `TestAuthorizationServerMetadataURLs` pins the URL list for all four issuer shapes. `TestGetAuthServerMetadataTrailingSlashIssuer` drives the whole lookup against the existing fake authorization server, serving metadata at the root well-known path. On unmodified main the first fails on both trailing-slash cases and the second fails with `GetAuthServerMetadata() error = Get "/.well-known/openid-configuration": redirect into loopback address "127.0.0.1"`, because `//.well-known/openid-configuration` makes `net/http.ServeMux` issue a canonicalizing redirect that the discovery client's `CheckRedirect` refuses. Both pass with the change. Verification on go1.26.4: `gofmt -l .` clean, `go vet ./...` clean, `staticcheck ./...` clean at v0.6.1 (the version the Test workflow pins), `go test ./auth/...` ok, `go test ./...` ok. I did not file an issue first. The fix is self-contained, confined to one unexported helper, and every issuer shape other than the slash-terminated ones is byte-identical to before. Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
akashjainn
|
f99cdb649b |
mcp: base64-encode a non-header-safe Mcp-Name (#1246)
Fixes: https://github.com/modelcontextprotocol/go-sdk/issues/1243 The 2026-07-28 transport spec only SHOULD-constrains names to header-safe characters, and lets a client Base64-wrap one that is not, using the `=?base64?...?=` sentinel. `setStandardHeaders` set `Mcp-Name` raw, while the sibling `Mcp-Param-*` path already ran its values through `encodeHeaderValue`, so a name outside the safe set went out unencoded. This routes `Mcp-Name` through `encodeHeaderValue` as well. #1242 landed the other direction — the server decoding the sentinel before comparing — so the two together close the round trip. Worth noting they interact: after #1242, a name that is literally shaped like the sentinel (`=?base64?abc?=`) is now rejected as invalid Base64, because the server tries to decode a value the client never encoded. The new test covers that case, and it fails on `main` today. `extractName` also covers `resources/read`, so this applies to resource URIs, which is where non-ASCII is most likely to show up in practice. ## Testing `TestMcpNameHeaderEncodingRoundTrip` drives `setStandardHeaders` and then feeds its output to `validateMcpHeaders`, so it pins the client and server halves against each other rather than asserting the encoding in isolation. Cases: an ASCII-safe name (must stay unencoded, no churn for the common path), non-ASCII, a leading space, a control character, and the sentinel-shaped literal above. Verified failing before the change and passing after. `go test ./...` passes. A note on process: [@remote-controlled-man](https://github.com/remote-controlled-man) asked about this issue earlier today and proposed much the same scope. I had the change written before I saw that comment and thought the #1242 interaction was worth surfacing either way, but I'm happy to close this if they'd rather carry it — please just say so. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011Wf643Lx36V5wdwuuf2EdT |
||
ARPAN MONDAL
|
a5026ea7b7 |
mcp: decode a Base64-wrapped Mcp-Name before comparing it to the body (#1242)
The 2026-07-28 transport spec lets a client Base64-wrap a name or resource URI that is not header-safe (`=?base64?...?=`) and requires the server to decode it before comparing. `validateMcpHeaders` decoded `Mcp-Param-*` values that way but compared `Mcp-Name` raw, so a spec-legal encoded name was refused as a header mismatch. This decodes `Mcp-Name` before comparing, mirroring the existing `Mcp-Param-*` path, and rejects an invalid Base64 wrapper with a clear error. Added table cases covering a valid encoded name and an invalid one (red before, green after). Fixes #1234 Co-authored-by: Arpan Mondal <arpanmondal@Arpans-MacBook-Pro.local> Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
|
Guglielmo Colombo
|
3f3b699b2b |
refactor: remove legacy MCPGODEBUG compatibility for new protocol release (#1237) v1.8.0 v1.8.0-pre.2 | ||
|
Guglielmo Colombo
|
830f0b76e7 |
mcp: update conformance tests (#1231)
v1.8.0-pre.1
Fixes #1230 |
||
Yaroslav
|
12cbafe54d |
oauthex: oauth discovery checks (#1220)
add extra checks for the default oauth discovery client: * disallow redirects which downgrade to http * disallow redirects to localhost or private ip * disallow the initial call to a private ip * proxy configuration opts out of dial time checking |
||
|
Yaroslav
|
3632967404 |
mcp: add an sse event size cap (#1205)
update sse I/O code to limit the amount of memory a single non-terminated event can take --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
|
Guglielmo Colombo
|
0d3036f785 |
mcp: allow per request Cacheable customization (#1203)
Fixes #1094 |
||
|
Yaroslav
|
cb0de64136 |
mcp: add a max request body size of the old transport (#1224)
added for consistency with the streamable transport because sse is not officially deprecated in the SDK Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
|
Guglielmo Colombo
|
2fdabde139 |
mcp: do not check metatada on notifications (#1215)
Fixes #1212 |
||
dependabot[bot]
|
59185e6969 |
build(deps): bump ossf/scorecard-action from 2.4.3 to 2.4.4 (#1217)
Bumps [ossf/scorecard-action](https://github.com/ossf/scorecard-action) from 2.4.3 to 2.4.4. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/ossf/scorecard-action/releases">ossf/scorecard-action's releases</a>.</em></p> <blockquote> <h2>v2.4.4</h2> <h2>What's Changed</h2> <p>This update bumps the Scorecard version to the v5.5.0 release. For a complete list of changes, please refer to the <a href="https://github.com/ossf/scorecard/releases/tag/v5.4.0">Scorecard v5.4.0 release notes</a> and the <a href="https://github.com/ossf/scorecard/releases/tag/v5.5.0">Scorecard v5.5.0 release notes</a>.</p> <ul> <li>log POST failures instead of failing entire action by <a href="https://github.com/spencerschrock"><code>@spencerschrock</code></a> in <a href="https://redirect.github.com/ossf/scorecard-action/pull/1625">ossf/scorecard-action#1625</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/ossf/scorecard-action/compare/v2.4.3...v2.4.4">https://github.com/ossf/scorecard-action/compare/v2.4.3...v2.4.4</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href=" |
||
|
dependabot[bot]
|
a6764cf838 |
build(deps): bump golang/govulncheck-action from 1.0.4 to 1.1.0 (#1218)
Bumps [golang/govulncheck-action](https://github.com/golang/govulncheck-action) from 1.0.4 to 1.1.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/golang/govulncheck-action/releases">golang/govulncheck-action's releases</a>.</em></p> <blockquote> <h2>v1.1.0</h2> <p><strong>Full Changelog</strong>: <a href="https://github.com/golang/govulncheck-action/compare/v1.0.4...v1.1.0">https://github.com/golang/govulncheck-action/compare/v1.0.4...v1.1.0</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href=" |
||
|
Yaroslav
|
8227246472 |
fix: only subscribe when server advertises capability (#1221) | ||
|
dependabot[bot]
|
f47d6ce156 |
build(deps): bump the codeql-action group with 3 updates (#1216)
Bumps the codeql-action group with 3 updates: [github/codeql-action/init](https://github.com/github/codeql-action), [github/codeql-action/analyze](https://github.com/github/codeql-action) and [github/codeql-action/upload-sarif](https://github.com/github/codeql-action). Updates `github/codeql-action/init` from 4.37.3 to 4.37.9 <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/github/codeql-action/releases">github/codeql-action/init's releases</a>.</em></p> <blockquote> <h2>v4.37.9</h2> <ul> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.4">2.26.4</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4106">#4106</a></li> </ul> <h2>v4.37.8</h2> <p>No user facing changes.</p> <h2>v4.37.7</h2> <ul> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.3">2.26.3</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4085">#4085</a></li> </ul> <h2>v4.37.6</h2> <ul> <li>Changed the default filepath for the new remote file address format that was introduced in CodeQL Action 4.37.0 / 3.37.0 to <code>.github/codeql-config.yml</code> to align it with the suggested path that is used elsewhere. <a href="https://redirect.github.com/github/codeql-action/pull/4070">#4070</a></li> </ul> <h2>v4.37.5</h2> <ul> <li>Fixed a bug where a network error while streaming the download of the CodeQL bundle could terminate the <code>init</code> Action instead of falling back to downloading the bundle before extracting it. <a href="https://redirect.github.com/github/codeql-action/pull/4061">#4061</a></li> </ul> <h2>v4.37.4</h2> <ul> <li>This version of the CodeQL Action adds support for the <code>tools</code> input for the <code>codeql-action/init</code> step to be specified using a <code>github-codeql-tools</code> <a href="https://docs.github.com/en/organizations/managing-organization-settings/managing-custom-properties-for-repositories-in-your-organization">repository property</a>. This feature will gradually be rolled out following the release of this version. Once rolled out, this allows for the CodeQL CLI version that is used in GitHub-managed workflows, such as Default Setup, to be set to a custom value. For example, customers who run into issues with rate limits when a new CodeQL CLI version is released can set the value to <code>toolcache</code> to always use the CodeQL CLI version that is available in the runner toolcache. For Advanced Setup workflows, the value provided for <code>tools</code> in the workflow definition always takes precedence unless the value of the repository property starts with <code>!</code>. <a href="https://redirect.github.com/github/codeql-action/pull/4037">#4037</a></li> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.2">2.26.2</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4051">#4051</a></li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/github/codeql-action/blob/main/CHANGELOG.md">github/codeql-action/init's changelog</a>.</em></p> <blockquote> <h1>CodeQL Action Changelog</h1> <p>See the <a href="https://github.com/github/codeql-action/releases">releases page</a> for the relevant changes to the CodeQL CLI and language packs.</p> <h2>[UNRELEASED]</h2> <p>No user facing changes.</p> <h2>4.37.9 - 26 Aug 2026</h2> <ul> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.4">2.26.4</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4106">#4106</a></li> </ul> <h2>4.37.8 - 21 Aug 2026</h2> <p>No user facing changes.</p> <h2>4.37.7 - 13 Aug 2026</h2> <ul> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.3">2.26.3</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4085">#4085</a></li> </ul> <h2>4.37.6 - 04 Aug 2026</h2> <ul> <li>Changed the default filepath for the new remote file address format that was introduced in CodeQL Action 4.37.0 / 3.37.0 to <code>.github/codeql-config.yml</code> to align it with the suggested path that is used elsewhere. <a href="https://redirect.github.com/github/codeql-action/pull/4070">#4070</a></li> </ul> <h2>4.37.5 - 03 Aug 2026</h2> <ul> <li>Fixed a bug where a network error while streaming the download of the CodeQL bundle could terminate the <code>init</code> Action instead of falling back to downloading the bundle before extracting it. <a href="https://redirect.github.com/github/codeql-action/pull/4061">#4061</a></li> </ul> <h2>4.37.4 - 29 Jul 2026</h2> <ul> <li>This version of the CodeQL Action adds support for the <code>tools</code> input for the <code>codeql-action/init</code> step to be specified using a <code>github-codeql-tools</code> <a href="https://docs.github.com/en/organizations/managing-organization-settings/managing-custom-properties-for-repositories-in-your-organization">repository property</a>. This feature will gradually be rolled out following the release of this version. Once rolled out, this allows for the CodeQL CLI version that is used in GitHub-managed workflows, such as Default Setup, to be set to a custom value. For example, customers who run into issues with rate limits when a new CodeQL CLI version is released can set the value to <code>toolcache</code> to always use the CodeQL CLI version that is available in the runner toolcache. For Advanced Setup workflows, the value provided for <code>tools</code> in the workflow definition always takes precedence unless the value of the repository property starts with <code>!</code>. <a href="https://redirect.github.com/github/codeql-action/pull/4037">#4037</a></li> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.2">2.26.2</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4051">#4051</a></li> </ul> <h2>4.37.3 - 22 Jul 2026</h2> <p>No user facing changes.</p> <h2>4.37.2 - 21 Jul 2026</h2> <ul> <li>The new address format for the <code>config-file</code> input that was introduced in CodeQL Action 4.37.0 is now enabled by default. In addition to the format described there, the <code>remote=</code> prefix can now be used to explicitly indicate that the input refers to a remote file. All previous input formats continue to be accepted as well. <a href="https://redirect.github.com/github/codeql-action/pull/4023">#4023</a></li> <li>The CodeQL Action can now make use of <a href="https://docs.github.com/en/code-security/how-tos/secure-at-scale/configure-organization-security/manage-usage-and-access/giving-org-access-private-registries">configured private registries</a> in Default Setup to retrieve CodeQL configuration files from remote repositories that require authentication. This will allow customers to store their CodeQL configuration in a single repository that can then be referenced by Default Setup workflows in other repositories. We expect to roll this and other, related changes out to everyone in July. <a href="https://redirect.github.com/github/codeql-action/pull/4007">#4007</a></li> </ul> <h2>4.37.1 - 16 Jul 2026</h2> <ul> <li><em>Upcoming breaking change</em>: Add a deprecation warning for customers using CodeQL version 2.20.6 and earlier. These versions of CodeQL were discontinued on 1 July 2026 alongside GitHub Enterprise Server 3.16, and will be unsupported by the next minor release of the CodeQL Action. <a href="https://redirect.github.com/github/codeql-action/pull/3956">#3956</a></li> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.1">2.26.1</a>. <a href="https://redirect.github.com/github/codeql-action/pull/4019">#4019</a></li> </ul> <h2>4.37.0 - 08 Jul 2026</h2> <ul> <li>Update default CodeQL bundle version to <a href="https://github.com/github/codeql-action/releases/tag/codeql-bundle-v2.26.0">2.26.0</a>. <a href="https://redirect.github.com/github/codeql-action/pull/3995">#3995</a></li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href=" |
||
Qinghong Zhu
|
ce3a92a0a8 |
mcp: fix ListChanged doc comments on server capabilities (#1219)
The doc comments on `ToolCapabilities.ListChanged` and `ResourceCapabilities.ListChanged` say the flag reports whether **the client** supports list-change notifications. Both types only ever appear inside `ServerCapabilities`, though, and the spec defines the field the other way around: whether **this server** supports notifications for changes to the list. `PromptCapabilities` in the same file already words it correctly. The "client" wording looks like it was copied from `RootCapabilities`, the one listChanged capability that genuinely belongs to the client. Docs only, no behavior change. |
||
Haoxin Yang
|
69c17de411 |
mcp: don't mutate caller params on multi-round-trip retries (#1145)
Fixes #1144. `clientMultiRoundTripMiddleware` calls `setMultiRoundTripRetryParams(req, ...)`, which writes `InputResponses` and `RequestState` through the caller's params pointer and never clears them. A `*CallToolParams` (or `GetPromptParams` / `ReadResourceParams`) struct reused across calls then carries the first call's answers into the next call: a server gating on `len(req.Params.InputResponses) == 0` — the idiom in this repo's own tests and examples — treats the fresh call as already answered, so the client's `ElicitationHandler` never fires and an elicitation-gated (e.g. destructive) tool runs without confirmation. This change makes the retry loop carry the fulfilled responses and echoed state on a shallow copy of the params (`multiRoundTripRetryRequest`) instead of mutating the original request. Caller-owned params are now left untouched by `CallTool`, `GetPrompt`, and `ReadResource`. The server-side shim (`serverMultiRoundTripMiddleware`) is unchanged: the params it mutates are SDK-owned, decoded from the wire. Tests: `TestMultiRoundTrip_AutoRetryDoesNotMutateCallerParams` (CallTool: params unchanged after the call, and reusing the same struct fulfills the input requests again instead of silently replaying the first call's answers — it failed with `elicitations after reuse = 1, want 2` before the fix), plus GetPrompt/ReadResource analogues asserting params are left clean. All three fail on main and pass with this change; `go test ./...` and `go vet ./...` are clean. Note on observable behavior: callers can no longer read the fulfilled responses off their own params struct after a call returns. That side effect was undocumented, and relying on it is exactly what makes the replay hazard above possible. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Minghui Wang
|
d19bfebab9 |
mcp: preserve single-request batch responses (#1214)
## What does this PR do? Preserve JSON-RPC batch response arrays when a batch contains a single request. ## Why is this needed? A single-request legacy batch must be returned as a one-element JSON array, but the JSON response path currently unwraps it into a single JSON object. ## Changes - Preserve the original batch state on the logical stream - Keep single-request batch responses as arrays - Add a regression test for legacy initialization batches ## Testing - `go test ./mcp -count=1` - `go test ./... -count=1` - `git diff --check` Fixes #1211 |
||
|
Guglielmo Colombo
|
21c18c6229 |
mcp: allow restricting the protocol versions a server supports (#1199)
Fixes #945 |
||
|
Yaroslav
|
5a89b63764 |
oauthex: bound dcr read to 1mb (#1206) | ||
Bobo
|
165121e53e |
jsonrpc2: wrap write error in server-closing error (#1186)
The "server is closing" error produced when a connection shuts down
because its write side failed formatted the underlying write error with
`%v`, so the cause - typically `io.EOF` when a stdio host closes its
pipe - was not part of the error chain. A caller that wants to
distinguish a clean host disconnect from a real failure via
`errors.Is(err, io.EOF)` therefore always got `false`.
This changes both `s.writeErr` wrappings to use `%w`, preserving the
write error in the error chain while keeping the same message text
("server is closing: EOF").
A regression test exercises the write-failure branch of `shuttingDown`
and asserts both `errors.Is(err, ErrServerClosing)` and `errors.Is(err,
io.EOF)`.
Fixes #1098
---------
Co-authored-by: Bo Zhang <187063395+Tethys0@users.noreply.github.com>
Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
|
||
|
Yaroslav
|
b55bcbd610 |
internal/json: json nesting depth check (#1200)
Added a lightweight JSON depth-checking pass before the actual parsing. |
||
Recoordinate
|
2732839dba |
mcp: prune completed listen request IDs from the session (#1190)
### Problem `ServerSession` tracks the request IDs of in-flight `subscriptions/listen` streams so `Close` can cancel the parked handlers and avoid the jsonrpc2 drain deadlock. The ID is appended when the listen starts, but it is never removed when the listen handler returns. Every completed listen stream (peer cancel, unsubscribe, stream break) therefore leaves one stale entry in the session for its entire lifetime. On a long-lived session where a client repeatedly subscribes and unsubscribes, the slice grows without bound: one leaked entry per cycle. `Close` calls `jsonrpc2.Connection.Cancel` on each recorded ID, and a stale ID is a no-op there today, but the bookkeeping is meant to hold only in-flight listens and currently never shrinks. ### Change Drop the request ID from the session's bookkeeping when the listen handler returns, matching the lifecycle of the in-flight handler that owns it. ### Tests Three tests reproduce the leak, failing on `main` and passing here: - `TestListenPrunedAfterSingleCompletion` - `TestListenIDsDoNotAccumulate` - `TestListenIDsLeakViaPublicSubscribeUnsubscribe` The third exercises the leak through the public `Subscribe`/`Unsubscribe` API. `go test ./mcp/` passes. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Scott Leggett
|
c68ad9a4e6 |
mcp: tolerate 404 on subscriptions/listen in stateless mode (#1193)
When connecting to remote stateless servers (such as GitHub's MCP server), optional streams like subscriptions/listen may be rejected by middleware with a plain-text HTTP 404 before reaching a JSON-RPC handler. Previously, checkResponse misclassified these 404s as lost stateful sessions (ErrSessionMissing) and tore down the transport connection because the error didn't wrap jsonrpc2.ErrRejected. This change aligns with the MCP 2026-07-28 Streamable HTTP specification: 1. Conditionally check for an active session ID before returning ErrSessionMissing, since a 404 on a stateless connection cannot mean a session was lost. 2. Wrap subscriptions/listen failures with ErrRejected (matching server/discover) so the jsonrpc2 layer doesn't permanently break the transport when encountering non-compliant plain-text 404s. --- I have manually tested this change against GitHub's hosted MCP server and confirmed that it fixes https://github.com/github/github-mcp-server/issues/3129 |
||
杨成锴
|
32404652e6 |
auth: validate unadvertised RFC 9207 iss instead of rejecting it (#1187)
## Summary Fixes #1152. Sentry, Linear, and Notion omit `authorization_response_iss_parameter_supported` from their authorization-server metadata. Sentry still sends a correct `iss` in the authorization response, so sign-in fails after the browser step: the user has already consented and an authorization code has been issued, but `validateIssuerResponse` rejects the exchange solely because the server did not advertise RFC 9207 support. RFC 9207 §2.4 says clients MUST extract and compare `iss` when the parameter is present. §2.4 also notes that clients SHOULD discard an unadvertised `iss`, but that is a recommendation and local policy may accept servers that send `iss` without advertising. Validating a matching `iss` is interoperable with those hosted MCP servers and still catches mix-up attacks: a wrong `iss` is rejected whether or not support was advertised. When `iss` is present, this change always compares it to the expected issuer (simple string equality). A match is accepted even if the server did not advertise support. A mismatch still returns `authorization response issuer %q does not match expected issuer %q`. Absence of `iss` remains an error only when the server advertised support. ## Test plan - [x] `go test ./auth/ -run TestValidateIssuerResponse -count=1` - [x] `go test ./auth/...` New `TestValidateIssuerResponse` cases: - `UnadvertisedIssCorrect`: `iss == expectedIssuer`, `issSupported == false` → no error - `UnadvertisedIssWrong`: `iss == "https://attacker.example.com"`, `issSupported == false` → error containing `does not match expected issuer` Existing advertised+match / advertised+mismatch / missing-iss cases are unchanged. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
Anton Dziatkovskii
|
810e6cb751 |
docs: make the protocol.md table of contents resolve (#1196)
Hi — I am an AI agent (Claude), working as Anton Dzyatkovsky's synthetic co-founder. Same author as #1159 (dead MRTR links in `client.md`), same genre, but this one has a cause worth naming rather than five links worth patching. **AI disclosure** (per [AI_POLICY.md](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/AI_POLICY.md)): this contribution — the investigation, the diff, the test and this description — was written by Claude Code running autonomously; Anton is the accountable human and reachable on this thread. Every measurement quoted below was produced by running the thing, not inferred: the id tables are scraped from the two rendered surfaces, the test's slug function was checked against 31 of GitHub's real ids, and the failure output is a real run against the pre-fix tree. Any reply from this account is written the same way unless it says otherwise. I edited only `internal/docs/**` and regenerated, per `AGENTS.md`. ## What is broken Five table-of-contents entries in `docs/protocol.md` point at anchors that no heading produces. They are dead on **both** surfaces — GitHub's render of the file, and the published site. | ToC entry links to | heading actually gets | |---|---| | `#discovery-(server/discover)` | `discovery-serverdiscover` | | `#per-request-meta-keys` | `per-request-_meta-keys` | | `#per-response-meta-keys` | `per-response-_meta-keys` | | `#subscriptions-(subscriptions/listen)` | `subscriptions-subscriptionslisten` | | `#sse-transport-(legacy)` | `sse-transport-legacy` | Right-hand column is scraped, not derived: `user-content-*` ids from GitHub's rendered `docs/protocol.md`, and `<h* id=...>` from `https://go.sdk.modelcontextprotocol.io/protocol`. Four of the five are dead on the live site too; `sse-transport-legacy` is not in the currently deployed build, so I could only confirm that one on GitHub. ## Why `%toc` and the renderer slug the same heading by different rules, and they disagree on exactly the punctuation these headings use: * **weave** keeps `(`, `)` and `/`, and drops `_` * **GitHub and python-markdown's `toc`** drop `(`, `)` and `/`, and keep `_` So the two agree on plain words and part ways the moment a heading carries a method name in parentheses or a `_meta` field. This is a class, not five typos: any future heading with those characters gets a dead ToC entry, silently — the docs regenerate fine, `docs-check` passes, and only a reader clicking the ToC finds out. Someone already hit this, by hand: `internal/docs/server.src.md` links to `protocol.md#subscriptions-subscriptionslisten`, i.e. the real renderer id, not the one the ToC uses. That cross-link is the odd one out today; the ToC is what is wrong. ## The fix The characters have to leave the headings, because weave computes the ToC and it is not in this repo. Each of the five sections already names its RPC or field in its first sentence, so the heading loses nothing: | was | becomes | the name is still in the first sentence | |---|---|---| | Discovery (``server/discover``) | **Discovery** | "the `server/discover` RPC lets a client discover…" | | Per-request ``_meta`` keys | **Per-request metadata keys** | "carries these keys inside its `_meta` map" | | Per-response ``_meta`` keys | **Per-response metadata keys** | "servers SHOULD identify themselves on every response" | | Subscriptions (``subscriptions/listen``) | **Subscriptions** | "`subscriptions/listen` replaces the legacy…" | | SSE Transport (legacy) | **Legacy SSE Transport** | "Before the streamable transport, the 2024-11-05 spec…" | Plus the one hand-written cross-link in `server.src.md`, updated to `protocol.md#subscriptions`. **The cost, stated plainly:** renaming a heading changes its id, so any external deep link to `#discovery-serverdiscover` or `#per-request-_meta-keys` breaks. Those ids are only reachable today by someone who scrolled and copied the permalink — the ToC never produced them — so I think the trade is worth it, but it is your call. **The alternative is to fix the slug in weave** (`golang.org/x/example/internal/cmd/weave`) and leave the headings alone. I am happy to take it there instead if you would rather keep the ids; say the word and I will close this. ## The guard `internal/docs/toc_test.go` — reads the generated docs and checks every `%toc` entry against the id its heading will actually get. No network, no new dependency. The slug function in it is validated, not asserted: run over all 31 headings of the current `docs/protocol.md`, it reproduces **31 of 31** of the ids GitHub actually emitted for that file (and 30 of 31 on the live site — the miss is the not-yet-deployed SSE section above). Behaviour on the pre-fix tree, which is the point of it: ``` --- FAIL: TestTOCAnchorsResolve protocol.md: table-of-contents entry "Discovery (`server/discover`)" points at #discovery-(server/discover), which no heading produces (want #discovery-serverdiscover); drop (, ), / and _ from the heading ... and the other four, each named ``` All five, by name, with the id they should have had. On this branch it passes. ## Runs `go generate ./...` → no diff (idempotent, so `docs-check` stays green) · `go vet ./...` clean · `go build ./...` clean · `go test ./...` → 14 packages ok, 0 failures, 26 without tests. Docs and one new test only; no SDK code touched. Co-authored-by: Anton Dziatkovskii <194927794+tonydzi@users.noreply.github.com> |
||
|
Guglielmo Colombo
|
b8c53fedd6 |
mcp: add docs for tier 1 assessment (#1195)
Closes open points in [comment](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/3220#issuecomment-5373089321) |
||
|
Guglielmo Colombo
|
6f0a645c20 |
mcp: add ServerSession.NotifyElicitationComplete (#1194)
Adds: ```go func (ss *ServerSession) NotifyElicitationComplete(ctx context.Context, params *ElicitationCompleteParams) error ``` As according to [spec](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-11-25/schema.ts#L2493-L2501) |
||
Lee
|
3d6450fccf |
mcp: bound streamable Close DELETE so a silent peer cannot hang teardown (#1184)
Close issued a synchronous session-termination DELETE on the connection context and cancelled that context only after `Do` returned. A reachable but silent peer could stall `Close()` until the TCP timeout. Give the DELETE its own short timeout. RED on current main: `TestCloseHangsOnBlackHoledDelete` failed with `Close() blocked on a black-holing DELETE`. GREEN: `go test ./mcp/ -count=1 -timeout 120s -run 'TestStreamableClient|TestCloseHangs|TestCallCancellation_FastReturn'` Fixes #1183 Made with [Cursor](https://cursor.com) Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
jansen
|
b074c7f3af |
internal/jsonrpc2: test WireError.Is and fix nil target panic (#1079)
# Problem WireError.Is backs errors.Is checks used across the SDK against sentinels such as ErrMethodNotFound, ErrRejected, and ErrClientClosing, but had no direct test. Add a table-driven test for it. While doing so, errors.Is(err, (*WireError)(nil)) was found to panic on a nil pointer dereference, because Is read w.Code without a nil check. Guard against a nil *WireError target so the comparison returns false. ```go errors.Is(err, (*WireError)(nil)) // panic: nil pointer dereference ``` # Approach Is read w.Code after a successful type assertion without checking whether w itself was nil. The fix guards against a nil *WireError target so the comparison returns false instead of panicking. # Test plan - go test ./internal/jsonrpc2/ passes. - The new typed nil WireError case panics without the one-line fix and passes with it, confirming the test exercises the guarded path. Co-authored-by: Yaroslav <yarolegovich@gmail.com> |
||
PratikDhanave
|
ce0b198202 |
mcp: prevent MemoryEventStore.After panic on index past the end (#1132)
## Summary
`MemoryEventStore.After` guarded only the lower bound of the requested
index:
```go
start := index + 1
if dl.first > start { // too old → ErrEventsPurged
return nil, fmt.Errorf(... ErrEventsPurged)
}
return slices.Clone(dl.data[start-dl.first:]), nil
```
There was no *upper*-bound guard, so an `index` greater than the highest
stored event made the final slice expression panic with `slice bounds
out of range`. The boundary `start-dl.first == len(dl.data)` (resume
from the last event seen) is a valid empty replay, so only
strictly-greater indices faulted.
This is reachable from untrusted input: `parseEventID` accepts any
non-negative index from the client-supplied `Last-Event-ID` header, and
`serveGET` passes it straight to `eventStore.After`. Direct callers of
the exported `EventStore.After` API crash outright.
Fixes #1131
## Change
Return an empty replay for an index at or beyond the latest stored event
— there is nothing after it — consistent with the already-valid `index
== last` case. `ErrEventsPurged` would be misleading here, since those
events were never sent rather than purged.
## Testing
Extended `TestMemoryEventStoreAfter` with indices past the end of a
stream (`3`, `100`, and one past a second stream), asserting an empty
replay with no error.
Verified the new cases **panic on the pre-fix code** (`panic: runtime
error: slice bounds out of range [3:2]`) and **pass with the fix**. `go
vet ./mcp/` and the full `go test ./mcp/` suite pass.
---------
Co-authored-by: Yaroslav Shevchuk <yarolegovich@gmail.com>
|
||
|
jansen
|
fe27fa9151 |
internal/jsonrpc2: plumb cancellation cause through request context (#1103)
Closes #1100. ## What `handleAsync` in `internal/jsonrpc2/conn.go` reported *why* an in-flight request had been canceled by inspecting the connection-level `s.writeErr`, guarded by a TODO waiting on golang/go#51365: ```go // Only deliver to the Handler if not already canceled. if err := req.ctx.Err(); err != nil { c.updateInFlight(func(s *inFlightState) { if s.writeErr != nil { // Assume that req.ctx was canceled due to s.writeErr. // TODO(#51365): use a Context API to plumb this through req.ctx. err = fmt.Errorf("%w: %v", ErrServerClosing, s.writeErr) } }) ... } ``` That API shipped as `context.WithCancelCause` / `context.Cause` in Go 1.21, and this module already targets Go 1.25, so the TODO is now actionable. This PR carries the cancellation cause on each request's context instead of inferring it from global state: - `incomingRequest.cancel` becomes a `context.CancelCauseFunc` (built via `context.WithCancelCause`). - The write-failure and read-side teardowns cancel each in-flight request with a cause wrapping `ErrServerClosing`. - The remaining cancel sites (peer cancel via `Cancel`, normal completion) pass `nil`. - `handleAsync` reads `context.Cause(req.ctx)` instead of looking up `s.writeErr`. ## Why Besides resolving the TODO, this fixes a latent mis-attribution. `s.writeErr` is connection-level state, not per-request: a request canceled for an unrelated reason (e.g. a peer cancel notification via `Cancel`) was reported as `ErrServerClosing` whenever `s.writeErr` happened to be non-nil at that moment. Tying the cause to the specific request's context removes that coupling. `ctx.Err()` still returns `context.Canceled` in all cases, so external behavior is unchanged; only the reported cause becomes accurate and per-request. ## Test plan - `go build ./...` - `go test ./internal/... ./mcp/` (including the longer `mcp` integration tests) — all pass. - Verified `context.WithCancelCause` semantics: with a cause, `Err()` stays `context.Canceled` while `Cause()` returns the wrapped `ErrServerClosing`; with `nil`, both resolve to `context.Canceled` — matching prior behavior. ## Note Per the discussion in #1100, the read-side teardown also carries a descriptive cause (wrapping `ErrServerClosing`) rather than a bare `context.Canceled`. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> Co-authored-by: Yaroslav <yarolegovich@gmail.com> |
||
gwyszynski-sa
|
b0408f2821 |
mcp: prefer the negotiated protocol version over the request context (#1163)
setMCPHeaders resolved Mcp-Protocol-Version from the request context before c.initializedResult. #1107 established the opposite order and said so in its description; the code it replaced also consulted the context last. The two branches were transposed by the switch-to-if rewrite in #1115, whose subject was an unrelated nil-pointer check. The context branch serves the pre-negotiation server/discover probe, which sets its own value in Client.Connect. Once a version has been negotiated it is the authoritative one for that connection, so initializedResult should win. _meta.protocolVersion from the outgoing message stays on top, so #1109 stays fixed. The existing table built its request with http.NewRequest, so protocolVersionFromContext returned "" in every case and the two orderings were indistinguishable, which is why the transposition went unnoticed. Adds a ctxVersion dimension and three cases pinning the precedence. Fixes #1162 Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
|
Guglielmo Colombo
|
1fcaf0c393 |
mcp: resolve deadlock on serverSession.Close() with subscriptionsListen call (#1166)
Fixes #1160 |
||
King Star
|
ba1496a9aa |
mcp: keep request log levels scoped to each request (#1168)
Fixes a request-scoping bug in the new-protocol logging path. `ServerSession.handle` decoded `_meta[io.modelcontextprotocol/logLevel]`, but stored it in `ServerSessionState.LogLevel`. Concurrent new-protocol requests on the same session could therefore clear or inherit another request's log threshold before the original handler emitted logs. This keeps the new-protocol log level on the handler context instead. `ServerSession.Log` and `LoggingHandler.Enabled` read that context value first, while legacy `logging/setLevel` behavior still uses session state. Tests: - `go test ./mcp -run TestServerLogLevelDoesNotLeakBetweenNewProtocolRequests -count=1` - `go test -race ./mcp -run TestServerLogLevelDoesNotLeakBetweenNewProtocolRequests -count=1` - `go test ./mcp -run TestEndToEnd -count=1` - `go test ./mcp -run TestValidateRequestMeta -count=1` - `go test ./mcp -run TestStreamableStateless_NewProtocolSession_NoFakeInit -count=1` - `go test ./... -skip TestSSELocalhostProtection|TestStreamableLocalhostProtection` A full `go test ./...` run in my local environment still fails the two localhost protection cases for `0.0.0.0 via localhost rejects evil.com`; those same two tests also fail when run directly and are unrelated to this logging change. Co-authored-by: Guglielmo Colombo <guglielmoc@google.com> |
||
|
Recoordinate
|
0eb1600eef |
mcp: avoid panicking when a resource handler returns nil (#1176)
### What `readResource` dereferenced the handler's result before the `if res == nil` check that was meant to guard it. A `ResourceHandler` returning `(nil, nil)` therefore panicked instead of returning an error: - modern clients: `handleMultiRoundTripResult` calls `res.setResultType` on the nil result, and - older clients: `res.setDefaultCacheableValues()` runs on the nil result. Either way the `if res == nil` guard a few lines below was dead code. ### Why it matters Request handlers run in their own goroutine and nothing recovers, so the panic does not just fail the one request. It terminates the whole server process and every concurrent session. The sibling handlers already avoid this: `callTool` and `getPrompt` both guard their result with `if err == nil && res != nil` before touching it, so a `(nil, nil)` return flows up and becomes a normal error response. ### Change Move the nil check in `readResource` ahead of `handleMultiRoundTripResult` and `setDefaultCacheableValues`, so a nil result returns the existing descriptive error (`read handler returned nil information`) instead of panicking. Added a regression test that drives a resource handler returning `(nil, nil)` through the server and asserts a clean error. |
||
|
Tony Dzi (Anton Dziatkovskii)
|
64e454e35c |
docs: fix four dead links to the Multi Round-Trip Requests section (#1159)
### What was broken
Four links in `docs/client.md` point at a section that does not exist.
Three of them sit right where a reader needs them — at the end of the
**Roots**, **Sampling**
and **Elicitation** sections, on the sentence explaining that from
protocol version
`2026-07-28` these requests are delivered via the Multi Round-Trip
Requests pattern:
```
[Multi Round-Trip Requests](protocol.md#multi-round-trip-requests-mrtr)
```
`protocol.md` has no MRTR section. Following the link drops you at the
top of a page that
never mentions the pattern — while the explanation you were promised is
one screen further
down the *same* page you were already on (`client.md` → *Multi
Round-Trip Requests*).
The fourth one is in that MRTR section itself, sending you to the server
side:
```
[server-side documentation](server.md#multi-round-trip-requests-mrtr).
```
`server.md` does have the section, but its heading is `## Multi
Round-Trip Requests`, so the
slug is `#multi-round-trip-requests` — the trailing `-mrtr` is stale.
### How I checked
Against the published site, not just the source:
```
$ curl -sL https://go.sdk.modelcontextprotocol.io/protocol | grep -o 'id="[^"]*round[^"]*"'
# (no output)
$ curl -sL https://go.sdk.modelcontextprotocol.io/client | grep -o 'id="[^"]*round[^"]*"'
id="multi-round-trip-requests"
$ curl -sL https://go.sdk.modelcontextprotocol.io/server | grep -o 'id="[^"]*round[^"]*"'
id="multi-round-trip-requests"
```
So `/protocol` carries no anchor containing `round` at all, and both
live targets use the
plain slug.
`git log -S` says the links arrived with
|
||
|
Guglielmo Colombo
|
4282916668 |
mcp: avoid Client.Connect leaks (#1158)
Fixes #1154 |
||
|
Recoordinate
|
e9a85e285f |
mcp: fix typo in protocol version comment (#1156)
`protcol` should be `protocol`. The comment sits above the `Mcp-Protocol-Version` header lookup, and the surrounding code uses `protocolVersion` and `protocolVersionFromContext`, so this is the only place in the package where the word is misspelled. |