GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add a subscription authorization hook and stop sending unrequested change notifications by maxisbey · Pull Request #3197 · modelcontextprotocol/python-sdk Conversation
|
📚 Documentation preview
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 8 files
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:2826">
P2: The three replacement notification calls after `notify_tools_changed()` omit `await`. Copying them into an async handler creates unexecuted coroutines and publishes no change event; show `await` for every `Context.notify_*` call.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
| On a 2026-07-28 connection, `notifications/tools/list_changed`, `notifications/prompts/list_changed`, `notifications/resources/list_changed`, and `notifications/resources/updated` reach a client only through a `subscriptions/listen` stream it opened — the spec forbids sending a notification type a subscription did not request. The v1-style session helpers (`ctx.session.send_tool_list_changed()`, `send_prompt_list_changed()`, `send_resource_list_changed()`, `send_resource_updated(uri)`) push a bare copy onto the connection's standalone channel instead, so on such a connection they are dropped with a debug log: silently on streamable HTTP (there is no standalone channel), and on stdio, where earlier v2 releases wrote the bare notification to the shared pipe, it is now dropped too. On pre-2026 connections the helpers behave as in v1. | ||
|
|
||
| Migrate to publishing on the subscription bus, which stamps and filters per stream: `await ctx.notify_tools_changed()`, `notify_prompts_changed()`, `notify_resources_changed()`, and `notify_resource_updated(uri)` on `MCPServer`'s `Context`, or `await bus.publish(...)` on a low-level `Server`'s own `SubscriptionBus` — see [Subscriptions](handlers/subscriptions.md). A stream only ever receives the kinds and URIs the server acknowledged for it; to decide per caller what that is, pass a `narrow_subscriptions` hook (`MCPServer(narrow_subscriptions=...)`, or `ListenHandler(bus, narrow=...)` on the low-level `Server`), covered on the same page. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
P2: The three replacement notification calls after notify_tools_changed() omit await. Copying them into an async handler creates unexecuted coroutines and publishes no change event; show await for every Context.notify_* call.
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 2826:
<comment>The three replacement notification calls after `notify_tools_changed()` omit `await`. Copying them into an async handler creates unexecuted coroutines and publishes no change event; show `await` for every `Context.notify_*` call.</comment>
<file context>
@@ -2819,6 +2819,12 @@ async def book_table(date: str, answer: Annotated[Confirmation, Resolve(ask_to_c
+
+On a 2026-07-28 connection, `notifications/tools/list_changed`, `notifications/prompts/list_changed`, `notifications/resources/list_changed`, and `notifications/resources/updated` reach a client only through a `subscriptions/listen` stream it opened — the spec forbids sending a notification type a subscription did not request. The v1-style session helpers (`ctx.session.send_tool_list_changed()`, `send_prompt_list_changed()`, `send_resource_list_changed()`, `send_resource_updated(uri)`) push a bare copy onto the connection's standalone channel instead, so on such a connection they are dropped with a debug log: silently on streamable HTTP (there is no standalone channel), and on stdio, where earlier v2 releases wrote the bare notification to the shared pipe, it is now dropped too. On pre-2026 connections the helpers behave as in v1.
+
+Migrate to publishing on the subscription bus, which stamps and filters per stream: `await ctx.notify_tools_changed()`, `notify_prompts_changed()`, `notify_resources_changed()`, and `notify_resource_updated(uri)` on `MCPServer`'s `Context`, or `await bus.publish(...)` on a low-level `Server`'s own `SubscriptionBus` — see [Subscriptions](handlers/subscriptions.md). A stream only ever receives the kinds and URIs the server acknowledged for it; to decide per caller what that is, pass a `narrow_subscriptions` hook (`MCPServer(narrow_subscriptions=...)`, or `ListenHandler(bus, narrow=...)` on the low-level `Server`), covered on the same page.
+
### Servers validate `Mcp-Param-*` headers against the request body ([SEP-2243](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2243))
</file context>
| Migrate to publishing on the subscription bus, which stamps and filters per stream: `await ctx.notify_tools_changed()`, `notify_prompts_changed()`, `notify_resources_changed()`, and `notify_resource_updated(uri)` on `MCPServer`'s `Context`, or `await bus.publish(...)` on a low-level `Server`'s own `SubscriptionBus` — see [Subscriptions](handlers/subscriptions.md). A stream only ever receives the kinds and URIs the server acknowledged for it; to decide per caller what that is, pass a `narrow_subscriptions` hook (`MCPServer(narrow_subscriptions=...)`, or `ListenHandler(bus, narrow=...)` on the low-level `Server`), covered on the same page. | |
| Migrate to publishing on the subscription bus, which stamps and filters per stream: `await ctx.notify_tools_changed()`, `await ctx.notify_prompts_changed()`, `await ctx.notify_resources_changed()`, and `await ctx.notify_resource_updated(uri)` on `MCPServer`'s `Context`, or `await bus.publish(...)` on a low-level `Server`'s own `SubscriptionBus` — see [Subscriptions](handlers/subscriptions.md). A stream only ever receives the kinds and URIs the server acknowledged for it; to decide per caller what that is, pass a `narrow_subscriptions` hook (`MCPServer(narrow_subscriptions=...)`, or `ListenHandler(bus, narrow=...)` on the low-level `Server`), covered on the same page. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Beyond the inline aliasing nit, two other candidates were examined and ruled out: (1) the narrow hook running before the max_subscriptions cap check — the request is still rejected pre-ack, so an at-capacity hook invocation is a wasted policy call, not a grant; (2) NotifyOnlyOutbound's method-name drop swallowing stamped listen-stream frames — ListenHandler always sends with related_request_id, which routes through the request-scoped outbound, never this connection-scoped channel.
One nit was found (in-place mutation of the request filter by a widening hook can defeat the intersection); it is posted inline and is defense-in-depth only — no correctly written hook is affected. The two candidates above were verified against the code: the cap check ordering is benign because the INTERNAL_ERROR rejection still precedes the ack, and the stamped-frame concern is impossible because session.send_notification with a related_request_id uses the request-scoped outbound (src/mcp/server/session.py:109), so subscription stream frames never traverse NotifyOnlyOutbound.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| granted = await self._grant(ctx, params.notifications) | ||
| if len(self._streams) >= self._max_subscriptions: | ||
| raise MCPError(INTERNAL_ERROR, "Subscription limit reached") | ||
| honored = _honored_subset(params.notifications) | ||
| honored = _honored_subset(params.notifications, granted) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The "can narrow but never widen" guarantee has an aliasing hole: _grant passes the live params.notifications object to the narrow hook (line 233), and _honored_subset then intersects against that same object (line 236), so a hook that widens the filter by in-place mutation (setting a kind flag or appending/rewriting URIs) and returns it makes the intersection a no-op — unrequested kinds and URIs leak into the ack and the delivery filter. Fix is a one-line defensive snapshot, e.g. requested = params.notifications.model_copy(deep=True) before invoking the hook, and passing the snapshot as the "requested" side of _honored_subset.
ListenHandler.__call__ in src/mcp/server/subscriptions.py does:
granted = await self._grant(ctx, params.notifications) # line 233 — hook receives the LIVE request filter ... honored = _honored_subset(params.notifications, granted) # line 236 — intersects against the SAME object
SubscriptionFilter is an ordinary mutable pydantic model (MCPModel in src/mcp-types/mcp_types/_types.py sets no frozen=True; the config even uses extra='allow'). So a narrow hook that mutates the filter in place and returns it makes the requested and granted arguments of _honored_subset the same (or equally mutated) data, and the intersection (requested.X and granted.X for the kind flags, uri in granted_uris for URIs) passes every added kind and URI straight through.
subscriptions/listen with SubscriptionFilter(tools_list_changed=True, resource_subscriptions=["r://a"]).async def narrow(ctx, requested): requested.prompts_list_changed = True # or: rewrites URIs to canonical forms requested.resource_subscriptions.append("r://extra") return requested
_grant returns that object — which is params.notifications, now mutated._honored_subset(params.notifications, granted) computes:
prompts_list_changed: requested.prompts_list_changed and granted.prompts_list_changed → True and True → honored, though the client never asked for it.[uri for uri in requested.resource_subscriptions if uri in granted_uris] → ["r://a", "r://extra"] — the never-requested URI survives.honored feeds both the acknowledgment (the client is told it got things it never asked for) and deliver()'s event_matches filter — the stream now delivers notifications/prompts/list_changed and ResourceUpdated(uri="r://extra"), violating the 2026-07-28 MUST NOT ("the server MUST NOT send notification types the client has not explicitly requested"). Note the SDK client admits any ResourceUpdated once any URI subscription was honored (src/mcp/client/subscriptions.py), so the unrequested events surface to the application.A verifier reproduced this empirically: the in-place widen yields honored = {tools_list_changed=True, prompts_list_changed=True, resource_subscriptions=['r://a','r://extra']}, while the identical widen via a fresh SubscriptionFilter is correctly blocked.
test_narrow_hook_can_only_narrow_never_widen only exercises a hook that returns a fresh filter, where the intersection works. The mutation path is untested.NarrowSubscription docstring ("Return the filter to grant - typically the request itself, possibly with kinds cleared or resource_subscriptions cut down") actively invites returning an edited request object._grant only covers exceptions, not aliasing.The hook is trusted, server-authored code, and every correct usage is safe: the no-hook default, the documented model_copy(update=...) example, and even in-place narrowing hooks (a narrowed object intersected with itself is still a subset of the original request). Only a hook that itself widens in place — which already violates its documented contract — escapes. So nothing breaks for correctly written servers; this is a defense-in-depth gap in an advertised "holds by construction" invariant rather than a client-triggerable failure. That is why this is a nit, not merge-blocking.
One line at the top of __call__: snapshot the request before the hook runs, e.g.
requested = params.notifications.model_copy(deep=True) granted = await self._grant(ctx, params.notifications) honored = _honored_subset(requested, granted)
(or snapshot the three flags plus a frozenset of the URIs). With the snapshot as the requested side, no mutation the hook performs can widen the intersection, and the docstring's claim becomes literally true. A companion test with an in-place-widening hook would pin it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…narrowing Rework the `subscriptions/listen` authorization seam to Go's shape: the hook (now `authorize=` on ListenHandler / `authorize_subscriptions=` on MCPServer, typed `AuthorizeSubscription`) either returns to accept the request as asked or raises MCPError to refuse the whole request before the acknowledgment. It no longer returns a narrowed filter, and the honored subset is again the plain support-based echo of the request rather than an intersection with a grant. Refusing on any unauthorized URI, with a uniform error, is now the documented pattern; a non-MCPError from the hook still fails closed. No-Verification-Needed: pushing early for review at maintainer request; verification to follow
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 6 files (changes from recent commits).
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/subscriptions.py">
<violation number="1" location="src/mcp/server/subscriptions.py:289">
P2: A mutating authorization hook can add event kinds or URIs to the live request, causing the ack and stream to include notifications the client never requested. Pass the hook a defensive copy so authorization remains accept-or-refuse rather than a way to widen the filter.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if self._authorize is None: | ||
| return | ||
| try: | ||
| await self._authorize(ctx, requested) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
P2: A mutating authorization hook can add event kinds or URIs to the live request, causing the ack and stream to include notifications the client never requested. Pass the hook a defensive copy so authorization remains accept-or-refuse rather than a way to widen the filter.
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/subscriptions.py, line 289:
<comment>A mutating authorization hook can add event kinds or URIs to the live request, causing the ack and stream to include notifications the client never requested. Pass the hook a defensive copy so authorization remains accept-or-refuse rather than a way to widen the filter.</comment>
<file context>
@@ -281,21 +276,21 @@ def deliver(event: ServerEvent) -> None:
+ return
try:
- return await self._narrow(ctx, requested)
+ await self._authorize(ctx, requested)
except MCPError:
raise
</file context>
| await self._authorize(ctx, requested) | |
| await self._authorize(ctx, requested.model_copy(deep=True)) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No bugs found in this revision — the accept-or-refuse rework resolves the widen-by-mutation aliasing hole I flagged on the earlier narrowing design (there is no returned filter anymore, and _honored_subset no longer intersects against a hook-visible object). Deferring to a human because this adds an authorization surface and a breaking notification-routing change.
What was reviewed:
authorize hook's three paths (accept, MCPError refusal pre-ack, fail-closed on any other exception) — all covered by the new unit tests, and the sanitized INTERNAL_ERROR leaks no policy detail.NotifyOnlyOutbound drop list — it matches the four ServerEvent kinds exactly, and it is installed only on the modern dual-era path, so pre-2026 connections (including resources/subscribe clients served via send_resource_updated) are unaffected as the migration note claims.params.notifications could still widen delivery — with accept-or-refuse there is no intersection to defeat, so mutation is only server-side misuse, not a client-reachable widen.The PR adds an optional accept-or-refuse authorization hook for subscriptions/listen (AuthorizeSubscription on ListenHandler, surfaced as MCPServer(authorize_subscriptions=...)) and makes the modern-era standalone channel (NotifyOnlyOutbound) drop the four change-notification methods instead of forwarding them as bare, unstamped frames. It touches src/mcp/server/subscriptions.py, connection.py, and mcpserver/server.py, plus docs (subscriptions page, migration guide, whats-new) and tests for both behaviors.
This is authorization-adjacent by design: the hook is the new per-caller gate on the listen path. The implementation is fail-closed (non-MCPError exceptions refuse with a sanitized INTERNAL_ERROR), runs before the ack, and the docs explicitly advise uniform refusal errors to avoid a per-URI oracle. The earlier narrowing iteration had a widen-by-mutation aliasing hole (flagged by both cubic and my prior run); the rework eliminates it structurally — the hook returns None or raises, and _honored_subset takes only the requested filter. I verified there is no remaining client-reachable path to widen delivery. The notification-drop change is a hardening measure (stops unstamped, unrequested change notifications on modern stdio) and its method set matches the subscription event vocabulary exactly.
High. This is production server code defining a new authorization seam, and it includes a breaking behavior change (v1-style send_*_changed helpers are silently dropped on 2026-era stdio connections — debug-logged only). Both the API shape (accept-or-refuse vs narrowing, which diverges deliberately from the TypeScript SDK's capability-based narrowing) and the breaking-change posture are design decisions a maintainer should own. Per the repo's own bar, security-sensitive surfaces should not be shadow-approved.
Test coverage is good: all three hook outcomes, per-method drop tests, and the existing listen-stream suite still exercises ack ordering, filtering, backlog overflow, and cleanup. The migration guide documents the breaking change as AGENTS.md requires. The PR description notes re-verification of the rework over real stdio was still in progress at submission time, which is another reason for a human to confirm before merge. The bug hunting system found no bugs in this revision.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Superseded by #3201, which drops the subscriptions-specific authorization hook in favour of exposing the existing middleware chain on |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.