From 836ceac30ebdbf9e6ae4ea2a702a45fdd867cb12 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 16:41:35 -0400 Subject: [PATCH 01/22] Add KeyValueResponseCacheStore for distributed client response caching (#4479) * Add KeyValueResponseCacheStore adapter for client response cache * Test KeyValueResponseCacheStore round-trip, isolation, and distributed sharing * Document distributed response cache store --- docs/clients/client.mdx | 21 ++ docs/development/v4-notes/change-register.mdx | 15 + fastmcp_slim/fastmcp/client/caching.py | 223 ++++++++++++++ tests/client/client/test_kv_response_cache.py | 279 ++++++++++++++++++ 4 files changed, 538 insertions(+) create mode 100644 fastmcp_slim/fastmcp/client/caching.py create mode 100644 tests/client/client/test_kv_response_cache.py diff --git a/docs/clients/client.mdx b/docs/clients/client.mdx index 30e09e613..a4141b71d 100644 --- a/docs/clients/client.mdx +++ b/docs/clients/client.mdx @@ -234,6 +234,27 @@ async with client: fresh = await client.list_tools_mcp(cache_mode="refresh") ``` +### Sharing a cache across clients + +The default cache lives in each client's process. To share cached responses across a fleet — a set of proxy replicas backed by one Redis, for example — pass a `KeyValueResponseCacheStore`, FastMCP's adapter over the same `AsyncKeyValue` key-value abstraction the event store and OAuth proxy use. It accepts any compatible backend (memory, Redis, and more). + +A shared store mingles responses from different principals, so it requires an explicit `partition` that isolates them. Derive the partition from a verified credential — never from request data or the server URL — and construct a new client when the principal changes. Only responses the server marks `"public"` are ever served across partitions. + +```python +from fastmcp import Client +from fastmcp.client.caching import KeyValueResponseCacheStore +from mcp.client.caching import CacheConfig +from key_value.aio.stores.redis import RedisStore + +backend = RedisStore(url="redis://localhost") +store = KeyValueResponseCacheStore(storage=backend) + +config = CacheConfig(store=store, partition="tenant-a", target_id="weather-api") +client = Client("https://example.com/mcp", mode="auto", cache=config) +``` + +The adapter serializes each result through a type-tagged envelope validated against an allowlist of cacheable result models, so a value naming an unknown type is treated as a cache miss rather than deserialized blindly. Each store instance owns its own collection namespace; `clear()` affects only that namespace, never another tenant's entries. + ## Operations FastMCP clients interact with three types of server components. diff --git a/docs/development/v4-notes/change-register.mdx b/docs/development/v4-notes/change-register.mdx index 6476d96cd..0b9e56e8b 100644 --- a/docs/development/v4-notes/change-register.mdx +++ b/docs/development/v4-notes/change-register.mdx @@ -219,6 +219,21 @@ Proxy forwarding handlers stash the request context so a backend that issues a s *Verify:* commit `1ac166bd`, `server/providers/proxy.py`. +### Shared response cache via `KeyValueResponseCacheStore` — New + +The SDK's client response cache (SEP-2549) reads and writes through a pluggable `ResponseCacheStore`; the default is a per-client in-memory LRU. FastMCP adds `KeyValueResponseCacheStore`, an adapter over the same `AsyncKeyValue` key-value abstraction the event store and OAuth proxy already use, so a fleet of clients (e.g. proxy replicas) can share one Redis-backed response cache. Pass it via `CacheConfig(store=...)`; a custom store requires an explicit `partition` (SDK) and `target_id` (FastMCP). Results serialize through a type-tagged envelope validated against an allowlist of cacheable result models — an unknown tag is a cache miss, never an import-by-name — and each adapter owns its own collection so `clear()` never touches another tenant. + +```python +from fastmcp.client.caching import KeyValueResponseCacheStore +from mcp.client.caching import CacheConfig +from key_value.aio.stores.redis import RedisStore + +store = KeyValueResponseCacheStore(storage=RedisStore(url="redis://localhost")) +config = CacheConfig(store=store, partition="tenant-a", target_id="weather-api") +``` + +*Verify:* `fastmcp_slim/fastmcp/client/caching.py`, `tests/client/client/test_kv_response_cache.py`. + ## HTTP The maintainer asked whether FastMCP can now delete its custom HTTP app and let the SDK's `Server.streamable_http_app()` handle everything. The answer for this PR is **no** — every override earns its keep. Convergence is a v4 project gated on three upstream additions (see [Feature Program](/development/v4-notes/feature-program)). diff --git a/fastmcp_slim/fastmcp/client/caching.py b/fastmcp_slim/fastmcp/client/caching.py new file mode 100644 index 000000000..75423819f --- /dev/null +++ b/fastmcp_slim/fastmcp/client/caching.py @@ -0,0 +1,223 @@ +"""A client response cache store backed by AsyncKeyValue. + +The MCP SDK's client response cache (SEP-2549) reads and writes through a +pluggable `ResponseCacheStore` protocol; the default is a per-client in-memory +LRU. This module adapts that protocol onto the `AsyncKeyValue` key-value +abstraction FastMCP already uses for its other state-management surfaces (the +event store, the OAuth proxy, the response-caching middleware), so a fleet of +FastMCP clients — for example a set of proxy replicas — can share one +Redis-backed response cache. + +Because a shared store mingles cached responses across principals, the SDK +requires an explicit `partition` on any custom store (and FastMCP additionally +requires a `target_id`). The partition is folded into every stored key so +entries can never collide or leak across authorization contexts, and the +adapter round-trips each result through a small type-tagged envelope validated +against an allowlist of cacheable result models — a stored value that names an +unknown type is treated as a miss, never imported by name. + +Example: + ```python + from fastmcp import Client + from fastmcp.client.caching import KeyValueResponseCacheStore + from mcp.client.caching import CacheConfig + from key_value.aio.stores.redis import RedisStore + + store = KeyValueResponseCacheStore(storage=RedisStore(url="redis://localhost")) + config = CacheConfig(store=store, partition="tenant-a", target_id="weather-api") + client = Client("https://example.com/mcp", mode="auto", cache=config) + ``` +""" + +from __future__ import annotations + +import hashlib +import time +from types import UnionType +from typing import Literal, get_args + +from key_value.aio.adapters.pydantic import PydanticAdapter +from key_value.aio.protocols import AsyncKeyValue +from key_value.aio.protocols.key_value import ( + AsyncDestroyCollectionProtocol, + AsyncEnumerateKeysProtocol, +) +from key_value.aio.stores.memory import MemoryStore +from mcp.client.caching import CacheEntry, CacheKey +from mcp_types import CacheableResult +from mcp_types.methods import MONOLITH_RESULTS + +from fastmcp.utilities.logging import get_logger +from fastmcp.utilities.types import FastMCPBaseModel + +logger = get_logger(__name__) + +DEFAULT_CACHE_COLLECTION = "fastmcp_response_cache" +"""Collection namespace owned by one adapter instance; `clear()` never reaches beyond it.""" + + +def _cacheable_result_models() -> dict[str, type[CacheableResult]]: + """Allowlist of `{class name: model}` for every cacheable result type. + + Derived from `MONOLITH_RESULTS` (the SDK's per-method result registry) so it + tracks the CACHEABLE_METHODS surface automatically. The class name is the + type tag written into the envelope; reconstruction looks the model up here + rather than importing an arbitrary name from store contents. + """ + models: dict[str, type[CacheableResult]] = {} + for row in MONOLITH_RESULTS.values(): + arms = get_args(row) if isinstance(row, UnionType) else (row,) + for arm in arms: + if isinstance(arm, type) and issubclass(arm, CacheableResult): + models[arm.__name__] = arm + return models + + +CACHEABLE_RESULT_MODELS = _cacheable_result_models() +"""Type tag -> model class allowlist for envelope reconstruction.""" + + +class _CacheEnvelope(FastMCPBaseModel): + """Serializable form of a `CacheEntry` for a remote store. + + A `CacheEntry.value` is a cacheable result model; a remote store cannot hold + it as an object, so it is serialized to `value_json` under a `type_tag` + (the model class name) and reconstructed against the allowlist on read. The + freshness/sharing metadata (`scope`, `expires_at`) round-trips alongside it. + """ + + type_tag: str + value_json: str + scope: str + expires_at: float | None + + +class KeyValueResponseCacheStore: + """A `ResponseCacheStore` backed by any `AsyncKeyValue` store. + + Implements the SDK client response cache contract (`get`/`set`/`delete`/ + `clear`) over the key-value abstraction FastMCP already uses elsewhere, so a + distributed deployment can point every client at one shared backend (memory, + Redis, etc.). Pass an instance as `CacheConfig(store=...)`; the SDK requires + an explicit `partition` on any custom store, and FastMCP additionally + requires a `target_id`. + + Each adapter instance owns one collection (`collection`), so `clear()` only + affects its own namespace and never another tenant's data. `clear()` needs + the backend to support collection destruction or key enumeration; against a + backend that supports neither it is a no-op and entries age out by TTL (a + warning is logged once). + + The SDK wraps every store call defensively — a raised operation degrades to + a cache miss rather than failing the request — so this adapter does not + re-wrap its own operations. + + Args: + storage: The `AsyncKeyValue` backend. Defaults to an in-process `MemoryStore`. + collection: Collection namespace for this adapter's entries. + """ + + def __init__( + self, + storage: AsyncKeyValue | None = None, + *, + collection: str = DEFAULT_CACHE_COLLECTION, + ) -> None: + self._storage: AsyncKeyValue = storage or MemoryStore() + self._collection = collection + self._adapter: PydanticAdapter[_CacheEnvelope] = PydanticAdapter[ + _CacheEnvelope + ]( + key_value=self._storage, + pydantic_model=_CacheEnvelope, + default_collection=collection, + ) + self._warned_clear_unsupported = False + + @staticmethod + def _string_key(key: CacheKey) -> str: + """Derive a stable store key from every `CacheKey` field. + + `CacheKey` is `(method, params_key, partition)`, where the coordinator has + already packed scope, negotiated protocol version, server arm id, and the + caller's partition into the `partition` field as a JSON array. Every field + is folded into the digest, so entries cannot collide across partitions, + protocol eras, or servers. The fields are length-prefixed before hashing + so no two distinct field tuples can produce the same pre-image. + """ + parts = [key.method, key.params_key, key.partition] + preimage = "".join(f"{len(part)}:{part}" for part in parts) + return hashlib.sha256(preimage.encode("utf-8")).hexdigest() + + async def get(self, key: CacheKey) -> CacheEntry | None: + envelope = await self._adapter.get(key=self._string_key(key)) + if envelope is None: + return None + model = CACHEABLE_RESULT_MODELS.get(envelope.type_tag) + if model is None: + # An unknown tag is never imported by name; a wrong-shape entry is a miss. + return None + value = model.model_validate_json(envelope.value_json) + scope: Literal["public", "private"] = ( + "public" if envelope.scope == "public" else "private" + ) + return CacheEntry(value=value, scope=scope, expires_at=envelope.expires_at) + + async def set(self, key: CacheKey, entry: CacheEntry) -> None: + value = entry.value + if not isinstance(value, CacheableResult): + return + type_tag = type(value).__name__ + if type_tag not in CACHEABLE_RESULT_MODELS: + return + envelope = _CacheEnvelope( + type_tag=type_tag, + value_json=value.model_dump_json(by_alias=True), + scope=entry.scope, + expires_at=entry.expires_at, + ) + ttl = self._entry_ttl(entry) + await self._adapter.put(key=self._string_key(key), value=envelope, ttl=ttl) + + async def delete(self, key: CacheKey) -> None: + await self._adapter.delete(key=self._string_key(key)) + + async def clear(self) -> None: + """Clear this adapter's collection only. + + Prefers deleting each enumerated key (which leaves the collection + usable), and falls back to whole-collection destruction. Against a + backend that supports neither, this is a no-op (entries age out by TTL) + and a warning is logged once. Either path is scoped to this adapter's + own collection, so a shared store's other tenants are never touched. + """ + storage = self._storage + if isinstance(storage, AsyncEnumerateKeysProtocol): + keys = await storage.keys(collection=self._collection) + for stored_key in keys: + await storage.delete(key=stored_key, collection=self._collection) + return + if isinstance(storage, AsyncDestroyCollectionProtocol): + await storage.destroy_collection(collection=self._collection) + return + if not self._warned_clear_unsupported: + self._warned_clear_unsupported = True + logger.warning( + "Response cache store backend %s supports neither collection " + "destruction nor key enumeration; clear() is a no-op and entries " + "will age out by TTL.", + type(storage).__name__, + ) + + def _entry_ttl(self, entry: CacheEntry) -> float | None: + """Seconds until the entry's own expiry, so the backend can evict it independently. + + The SDK gates freshness on `expires_at`, but a shared backend should not + retain a stale entry indefinitely; a store TTL lets it reclaim space. A + non-positive remaining lifetime stores with no backend TTL (the SDK will + still treat the already-stale entry as a miss). + """ + if entry.expires_at is None: + return None + remaining = entry.expires_at - time.time() + return remaining if remaining > 0 else None diff --git a/tests/client/client/test_kv_response_cache.py b/tests/client/client/test_kv_response_cache.py new file mode 100644 index 000000000..108bf3e60 --- /dev/null +++ b/tests/client/client/test_kv_response_cache.py @@ -0,0 +1,279 @@ +"""`KeyValueResponseCacheStore`: the AsyncKeyValue-backed client response cache store. + +The SDK's client response cache (SEP-2549) reads and writes through a pluggable +`ResponseCacheStore`. `KeyValueResponseCacheStore` adapts that contract onto the +`AsyncKeyValue` abstraction FastMCP already uses for its other state surfaces, so +a fleet of clients can share one backend (memory, Redis, etc.). + +These tests cover the store in isolation (round-trip, partition isolation, +clear, allowlist) and end-to-end: two independent `fastmcp.Client` instances +sharing one adapter-backed store, where the second client serves the first's +cached `tools/list` with zero wire calls. +""" + +from __future__ import annotations + +import json +import time + +import pytest +from key_value.aio.stores.memory import MemoryStore +from mcp.client.caching import CacheConfig, CacheEntry, CacheKey +from mcp.server.caching import CacheHint +from mcp.server.mcpserver import MCPServer +from mcp_types import ListToolsResult, Tool + +from fastmcp import Client, FastMCP +from fastmcp.client.caching import ( + CACHEABLE_RESULT_MODELS, + KeyValueResponseCacheStore, + _CacheEnvelope, +) + + +def _tools_result() -> ListToolsResult: + return ListToolsResult( + tools=[Tool(name="add", input_schema={"type": "object"})], + ttl_ms=60000, + cache_scope="public", + ) + + +def _key( + partition: str, *, method: str = "tools/list", params_key: str = "" +) -> CacheKey: + # The coordinator packs scope/version/arm into CacheKey.partition as a JSON + # array; mirror that shape so the derived string key is realistic. + arm = json.dumps(["public", "2026-07-28", "srv", partition]) + return CacheKey(method, params_key, arm) + + +def _cached_server(ttl_ms: int = 60000) -> MCPServer: + """An SDK MCPServer whose tools/list carries a positive ttlMs hint at 2026.""" + server = MCPServer( + "cached", cache_hints={"tools/list": CacheHint(ttl_ms=ttl_ms, scope="public")} + ) + + @server.tool() + def add(a: int, b: int) -> int: + return a + b + + return server + + +class TestRoundTrip: + async def test_set_get_reconstructs_model(self): + """A stored entry round-trips back to an equal result model object.""" + store = KeyValueResponseCacheStore() + result = _tools_result() + key = _key("p1") + + await store.set( + key, CacheEntry(value=result, scope="public", expires_at=time.time() + 60) + ) + got = await store.get(key) + + assert got is not None + assert isinstance(got.value, ListToolsResult) + assert got.value == result + assert got.scope == "public" + + async def test_get_miss_returns_none(self): + store = KeyValueResponseCacheStore() + assert await store.get(_key("p1")) is None + + async def test_delete_removes_entry(self): + store = KeyValueResponseCacheStore() + key = _key("p1") + await store.set( + key, + CacheEntry( + value=_tools_result(), scope="public", expires_at=time.time() + 60 + ), + ) + await store.delete(key) + assert await store.get(key) is None + + async def test_private_scope_roundtrips(self): + store = KeyValueResponseCacheStore() + key = _key("p1") + result = ListToolsResult( + tools=[Tool(name="add", input_schema={"type": "object"})] + ) + await store.set( + key, CacheEntry(value=result, scope="private", expires_at=time.time() + 60) + ) + got = await store.get(key) + assert got is not None + assert got.scope == "private" + + +class TestPartitionIsolation: + async def test_two_partitions_do_not_bleed(self): + """Entries written under different CacheKey.partition arms never collide.""" + store = KeyValueResponseCacheStore() + result = _tools_result() + + await store.set( + _key("tenant-a"), + CacheEntry(value=result, scope="public", expires_at=time.time() + 60), + ) + + # A different partition is a distinct key: a clean miss, not a shared hit. + assert await store.get(_key("tenant-b")) is None + assert await store.get(_key("tenant-a")) is not None + + async def test_method_and_params_key_isolate(self): + """Distinct method / params_key never collide in the derived string key.""" + store = KeyValueResponseCacheStore() + result = _tools_result() + await store.set( + _key("p1", method="resources/read", params_key="file:///a"), + CacheEntry(value=result, scope="public", expires_at=time.time() + 60), + ) + assert ( + await store.get(_key("p1", method="resources/read", params_key="file:///b")) + is None + ) + assert await store.get(_key("p1", method="tools/list")) is None + + +class TestClear: + async def test_clear_empties_and_keeps_collection_usable(self): + store = KeyValueResponseCacheStore() + key = _key("p1") + await store.set( + key, + CacheEntry( + value=_tools_result(), scope="public", expires_at=time.time() + 60 + ), + ) + + await store.clear() + assert await store.get(key) is None + + # The collection remains usable for subsequent writes. + await store.set( + key, + CacheEntry( + value=_tools_result(), scope="public", expires_at=time.time() + 60 + ), + ) + assert await store.get(key) is not None + + async def test_clear_scoped_to_own_collection(self): + """Two adapters over one backend clear independently.""" + backend = MemoryStore() + store_a = KeyValueResponseCacheStore(backend, collection="cache_a") + store_b = KeyValueResponseCacheStore(backend, collection="cache_b") + key = _key("p1") + entry = CacheEntry( + value=_tools_result(), scope="public", expires_at=time.time() + 60 + ) + + await store_a.set(key, entry) + await store_b.set(key, entry) + + await store_a.clear() + + assert await store_a.get(key) is None + assert await store_b.get(key) is not None # untouched + + +class TestAllowlist: + async def test_unknown_type_tag_is_a_miss(self): + """A stored envelope naming a type outside the allowlist is a miss, never imported.""" + store = KeyValueResponseCacheStore() + key = _key("p1") + forged = _CacheEnvelope( + type_tag="EvilResult", + value_json="{}", + scope="public", + expires_at=time.time() + 60, + ) + await store._adapter.put(key=store._string_key(key), value=forged) + + assert await store.get(key) is None + + def test_allowlist_matches_cacheable_methods(self): + """The allowlist covers exactly the SDK's cacheable result models.""" + assert set(CACHEABLE_RESULT_MODELS) == { + "DiscoverResult", + "ListPromptsResult", + "ListResourceTemplatesResult", + "ListResourcesResult", + "ListToolsResult", + "ReadResourceResult", + } + + +class TestFastMCPConstruction: + def test_custom_store_without_target_id_raises(self): + """FastMCP requires a target_id for a custom shared store on an in-memory transport.""" + store = KeyValueResponseCacheStore() + with pytest.raises(ValueError, match="requires CacheConfig.target_id"): + Client(FastMCP("x"), cache=CacheConfig(store=store, partition="p")) + + def test_custom_store_without_partition_raises(self): + """The SDK requires an explicit partition for any custom store.""" + with pytest.raises(ValueError, match="requires an explicit partition"): + CacheConfig(store=KeyValueResponseCacheStore(), target_id="srv") + + def test_custom_store_builds_cache(self): + store = KeyValueResponseCacheStore() + config = CacheConfig(store=store, partition="p", target_id="srv") + client = Client(FastMCP("x"), mode="auto", cache=config) + assert client._response_cache is not None + + +class TestDistributedSharing: + async def test_second_client_serves_first_clients_cache(self): + """Two independent Clients sharing one adapter-backed store: client B's first + list_tools is served from the entry client A populated, with zero wire calls.""" + backend = MemoryStore() + store = KeyValueResponseCacheStore(backend) + + def make_client() -> Client: + config = CacheConfig(store=store, partition="tenant-a", target_id="cached") + return Client(_cached_server(), mode="auto", cache=config) + + async with make_client() as client_a: + first = await client_a.list_tools() + assert [t.name for t in first] == ["add"] + + async with make_client() as client_b: + calls = {"n": 0} + original = client_b.session.list_tools + + async def spy(**kwargs): + calls["n"] += 1 + return await original(**kwargs) + + client_b.session.list_tools = spy # type: ignore[method-assign] + served = await client_b.list_tools() + + assert calls["n"] == 0 # served from the shared store, no wire round-trip + assert [t.name for t in served] == ["add"] + + async def test_distinct_partitions_do_not_share(self): + """Two clients on the same store but different partitions each hit the wire.""" + backend = MemoryStore() + store = KeyValueResponseCacheStore(backend) + + config_a = CacheConfig(store=store, partition="tenant-a", target_id="cached") + async with Client(_cached_server(), mode="auto", cache=config_a) as client_a: + await client_a.list_tools() + + config_b = CacheConfig(store=store, partition="tenant-b", target_id="cached") + async with Client(_cached_server(), mode="auto", cache=config_b) as client_b: + calls = {"n": 0} + original = client_b.session.list_tools + + async def spy(**kwargs): + calls["n"] += 1 + return await original(**kwargs) + + client_b.session.list_tools = spy # type: ignore[method-assign] + await client_b.list_tools() + + assert calls["n"] == 1 # different partition -> not shared, hits the wire From 266c129b626d8ac40ac62dd034508970440c788e Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:02:26 -0400 Subject: [PATCH 02/22] Test lifespan fires once per process over HTTP (#4480) * Add regression test: HTTP lifespan fires once per process across sessions * Drop redundant enter-count assertion at teardown (CodeQL) * Assert session-manager lifespan entry directly, not user-lifespan count --- .../http/test_lifespan_once_per_process.py | 67 +++++++++++++++---- 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/tests/server/http/test_lifespan_once_per_process.py b/tests/server/http/test_lifespan_once_per_process.py index 07a1a8c9d..0f58b7c6b 100644 --- a/tests/server/http/test_lifespan_once_per_process.py +++ b/tests/server/http/test_lifespan_once_per_process.py @@ -3,23 +3,46 @@ Before FastMCP handed its lifespan to the SDK lowlevel Server (PR #4446), the SDK v1 lifespan was effectively session-scoped and FastMCP worked around it by driving its own ``_lifespan_manager`` beside the session manager. The SDK v2 -``StreamableHTTPSessionManager`` now enters ``app.lifespan(app)`` exactly once -for the manager's lifetime, so the user lifespan must fire once per process and -persist across multiple HTTP client sessions -- not once per session. +``StreamableHTTPSessionManager`` now enters ``app.lifespan(app)`` -- FastMCP's +``_lifespan_proxy`` -- exactly once for the manager's lifetime and reuses the +yielded state for every session. The user lifespan must therefore fire once per +process and persist across HTTP client sessions, not once per session. + +The invariant that actually matters is "the session manager drives the lifespan +exactly once, regardless of how many client sessions connect." A plain +user-lifespan enter/exit counter cannot guard it: ``_lifespan_manager`` is +ref-counted, and ``run_http_async`` opens an *outer* ``_lifespan_manager`` +around uvicorn. That outer entry holds the ref count at >= 1 for the whole +server lifetime, so even if the session manager regressed to re-entering +``_lifespan_proxy`` once per session, the user lifespan would still be entered +exactly once (the proxy's nested ``_lifespan_manager`` entries would all reuse +the outer result). The user counter would stay ``1`` while the behavior it +claims to guard was broken. + +So this test spies on the session-manager entry point directly -- it counts how +many times the SDK enters ``server._mcp_server.lifespan`` (the ``_lifespan_proxy`` +wrapper) -- and asserts that count is exactly one across sequential and +overlapping sessions. A regression that moves ``app.lifespan(app)`` into the +per-session code path makes this count grow with the session count and fails +loudly. The user enter/exit counter is kept as a secondary check on the +"entered once, exited once at shutdown" shape. """ from collections.abc import AsyncIterator from contextlib import asynccontextmanager from typing import Any +from mcp.server.lowlevel.server import Server as LowLevelServer + from fastmcp import Client, FastMCP from fastmcp.client.transports import StreamableHttpTransport from fastmcp.utilities.tests import run_server_async async def test_http_user_lifespan_fires_once_across_sessions(): - """The user lifespan must be entered exactly once for the server process, - even when several independent HTTP client sessions connect and disconnect. + """The session manager must drive the user lifespan exactly once for the + server process, even when several independent HTTP client sessions connect + and disconnect. """ enter_count = 0 exit_count = 0 @@ -39,6 +62,24 @@ async def test_http_user_lifespan_fires_once_across_sessions(): def ping() -> str: return "pong" + # Spy on the session manager's single entry point into FastMCP's lifespan. + # `StreamableHTTPSessionManager.run()` calls `self.app.lifespan(self.app)` + # exactly once and reuses the yielded state per session; `self.app` is + # `server._mcp_server`, so `server._mcp_server.lifespan` is the + # `_lifespan_proxy` wrapper. Counting entries here asserts the invariant + # directly, independent of `_lifespan_manager`'s ref-count masking. + proxy_enter_count = 0 + original_lifespan = server._mcp_server.lifespan + + @asynccontextmanager + async def counting_proxy(app: LowLevelServer[Any]) -> AsyncIterator[Any]: + nonlocal proxy_enter_count + proxy_enter_count += 1 + async with original_lifespan(app) as state: + yield state + + server._mcp_server.lifespan = counting_proxy + async with run_server_async(server, transport="http") as mcp_url: # `run_server_async` yields a URL that already includes the `/mcp` path. # Three separate, sequential client sessions against the same process. @@ -46,9 +87,9 @@ async def test_http_user_lifespan_fires_once_across_sessions(): async with Client(StreamableHttpTransport(mcp_url)) as client: result = await client.call_tool("ping", {}) assert result.data == "pong" - # The lifespan must not have exited when a session closed -- it is - # owned by the session manager for the whole process lifetime. - assert enter_count == 1 + # The session manager must not re-drive the lifespan when a session + # closes -- it owns a single entry for the whole process lifetime. + assert proxy_enter_count == 1 assert exit_count == 0 # Overlapping sessions must also observe a single, still-open lifespan. @@ -58,10 +99,12 @@ async def test_http_user_lifespan_fires_once_across_sessions(): ): assert (await c1.call_tool("ping", {})).data == "pong" assert (await c2.call_tool("ping", {})).data == "pong" - assert enter_count == 1 + assert proxy_enter_count == 1 assert exit_count == 0 - # After the server process task is torn down, the lifespan has exited once. - # (A spurious re-entry during teardown would re-exit, so this also guards - # that the lifespan was entered exactly once.) + # The session manager entered the lifespan exactly once across every + # session, and the user lifespan was entered once and exited once at + # process shutdown. + assert proxy_enter_count == 1 + assert enter_count == 1 assert exit_count == 1 From a04f6fd9116933eb5983ce2cabbcba44af8649b6 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:02:48 -0400 Subject: [PATCH 03/22] Add telemetry off-switch and mcp.protocol.version span attribute (#4481) * Turn OpenTelemetry instrumentation on by default with explicit off-switch Add FASTMCP_ENABLE_TELEMETRY setting (default true) and mcp.protocol.version span attribute for SDK parity. * Make disabled telemetry a transparent pass-through, not a NoOpTracer The stock NoOpTracer.start_as_current_span attaches a NonRecordingSpan, hijacking the current OTel context from any enclosing application span. When telemetry is disabled, get_tracer() now returns a non-attaching pass-through tracer so trace.get_current_span() inside handlers still resolves to the caller's span. --- docs/development/v4-notes/change-register.mdx | 6 + docs/more/settings.mdx | 6 + docs/servers/telemetry.mdx | 9 +- fastmcp_slim/fastmcp/server/telemetry.py | 18 ++ fastmcp_slim/fastmcp/settings.py | 18 ++ fastmcp_slim/fastmcp/telemetry.py | 60 +++++- tests/server/telemetry/test_server_tracing.py | 188 +++++++++++++++++- 7 files changed, 301 insertions(+), 4 deletions(-) diff --git a/docs/development/v4-notes/change-register.mdx b/docs/development/v4-notes/change-register.mdx index 0b9e56e8b..33fe92a98 100644 --- a/docs/development/v4-notes/change-register.mdx +++ b/docs/development/v4-notes/change-register.mdx @@ -152,6 +152,12 @@ SDK v2 seeds an `OpenTelemetryMiddleware` into every lowlevel `Server`, so each *Verify:* `fastmcp_slim/fastmcp/server/low_level.py` (the `OpenTelemetryMiddleware` filter); `tests/server/telemetry/test_server_tracing.py::TestSingleServerSpan`. +### Telemetry on by default, with an explicit off-switch — Absorbed + +FastMCP's OpenTelemetry instrumentation is on by default. Because FastMCP uses only the OpenTelemetry API, span creation is a no-op with negligible overhead (the API's `NonRecordingSpan`) unless the user configures an SDK and exporter — so being always-on costs nothing until you opt into collection. The new `FASTMCP_ENABLE_TELEMETRY` setting (`fastmcp.settings.enable_telemetry`, default `true`) is the explicit off-switch: set it to `false` and `get_tracer()` returns a genuine no-op tracer, so no FastMCP spans are created even when an SDK is configured. The off-switch governs FastMCP's own spans (all SERVER spans, plus FastMCP's high-level CLIENT span); the SDK's low-level `mcp-python-sdk` `MCP send ` CLIENT spans are governed by the user's OpenTelemetry SDK, not this setting. FastMCP's SERVER span now also carries `mcp.protocol.version` — the attribute the dropped SDK `OpenTelemetryMiddleware` set — restoring parity with the SDK's semantic conventions. + +*Verify:* `fastmcp_slim/fastmcp/settings.py` (`enable_telemetry`); `fastmcp_slim/fastmcp/telemetry.py` (`get_tracer` off-switch); `fastmcp_slim/fastmcp/server/telemetry.py` (`get_protocol_span_attributes`); `tests/server/telemetry/test_server_tracing.py::TestTelemetryEnabledByDefault`, `::TestProtocolVersionAttribute`. + ### Spec-correct error codes via a central translator — Breaking (wire error code) Resource-not-found responses from the core `resources/read` handler previously used `-32002`. SEP-2164 (and the SDK's own mcpserver, which maps `ResourceNotFoundError` → `INVALID_PARAMS`) makes this `-32602`. The per-adapter `MCPError(code=..., ...)` literals in `server/mixins/mcp_operations.py` are replaced by a single `fastmcp.exceptions.to_mcp_error()` translator that maps FastMCP's public exceptions to the `mcp_types` code constants (`NotFoundError`/`DisabledError`/`ValidationError` → `INVALID_PARAMS`, else `INTERNAL_ERROR`). Clients that string-matched on the old `-32002` for resource-not-found must switch to `-32602`; the human-readable message ("Resource not found: ...") is unchanged. The opt-in `ErrorHandlingMiddleware`, which has its own documented per-method-prefix code mapping, is intentionally left as-is. diff --git a/docs/more/settings.mdx b/docs/more/settings.mdx index a50cca6e1..b672af316 100644 --- a/docs/more/settings.mdx +++ b/docs/more/settings.mdx @@ -72,6 +72,12 @@ These control how the server listens when running with an HTTP transport. | `FASTMCP_SHOW_SERVER_BANNER` | `bool` | `true` | Show the server banner on startup. Also controllable via `--no-banner` or `server.run(show_banner=False)`. | | `FASTMCP_CHECK_FOR_UPDATES` | `Literal["stable", "prerelease", "off"]` | `stable` | Update checking on CLI startup. `stable` checks stable releases only, `prerelease` includes pre-releases, `off` disables checking. | +## Telemetry + +| Environment Variable | Type | Default | Description | +|---|---|---|---| +| `FASTMCP_ENABLE_TELEMETRY` | `bool` | `true` | Whether FastMCP's native [OpenTelemetry instrumentation](/servers/telemetry) is active. Enabled by default; FastMCP uses only the OpenTelemetry API, so span creation is a no-op with negligible overhead unless an OpenTelemetry SDK and exporter are configured. Set to `false` to turn instrumentation off entirely, in which case no FastMCP spans are created even when an SDK is configured. | + ## Tasks (Docket) These configure the [Docket](https://github.com/prefecthq/docket) task queue used by [server tasks](/servers/tasks). All use the `FASTMCP_DOCKET_` prefix. diff --git a/docs/servers/telemetry.mdx b/docs/servers/telemetry.mdx index aed7308db..7553868f5 100644 --- a/docs/servers/telemetry.mdx +++ b/docs/servers/telemetry.mdx @@ -12,11 +12,17 @@ FastMCP includes native OpenTelemetry instrumentation for observability. Traces FastMCP uses the OpenTelemetry API for instrumentation. This means: -- **Zero configuration required** - Instrumentation is always active +- **On by default** - Instrumentation is active out of the box, no opt-in required - **No overhead when unused** - Without an SDK, all operations are no-ops - **Bring your own SDK** - You control collection, export, and sampling - **Works with any OTEL backend** - Jaeger, Zipkin, Datadog, New Relic, etc. +Because FastMCP only depends on the OpenTelemetry API, span creation is a no-op until you configure an SDK and exporter — so being on by default costs nothing until you opt into collection. + +### Turning Telemetry Off + +To disable FastMCP's instrumentation entirely, set `FASTMCP_ENABLE_TELEMETRY=false` (or `fastmcp.settings.enable_telemetry = False`). When disabled, FastMCP creates no spans even if an SDK is configured. + ## Enabling Telemetry The easiest way to export traces is using `opentelemetry-instrument`, which configures the SDK automatically: @@ -271,6 +277,7 @@ FastMCP implements the [OpenTelemetry MCP semantic conventions](https://opentele | Attribute | Description | |-----------|-------------| | `mcp.method.name` | The MCP method being called (`tools/call`, `resources/read`, `prompts/get`) | +| `mcp.protocol.version` | The negotiated MCP protocol version for the request | | `mcp.session.id` | Session identifier for the MCP connection | | `mcp.resource.uri` | The resource URI (for resource operations) | | `gen_ai.tool.name` | Tool name (on `tools/call` spans) | diff --git a/fastmcp_slim/fastmcp/server/telemetry.py b/fastmcp_slim/fastmcp/server/telemetry.py index eeaa0aeb7..e9f6d4344 100644 --- a/fastmcp_slim/fastmcp/server/telemetry.py +++ b/fastmcp_slim/fastmcp/server/telemetry.py @@ -58,6 +58,21 @@ def get_session_span_attributes() -> dict[str, str]: return attrs +def get_protocol_span_attributes() -> dict[str, str]: + """Get the negotiated MCP protocol version for the current request. + + Mirrors the `mcp.protocol.version` attribute the SDK's own + `OpenTelemetryMiddleware` sets — FastMCP drops that middleware to avoid a + duplicate SERVER span, so this restores the attribute on FastMCP's span. + """ + from fastmcp.server.dependencies import fastmcp_request_ctx + + req_ctx = fastmcp_request_ctx.get() + if req_ctx is not None and req_ctx.protocol_version: + return {"mcp.protocol.version": req_ctx.protocol_version} + return {} + + def _get_parent_trace_context() -> Context | None: """Get parent trace context from request meta for distributed tracing.""" from fastmcp.server.dependencies import fastmcp_request_ctx @@ -84,6 +99,7 @@ def _build_server_span_attrs( "fastmcp.server.name": server_name, "fastmcp.component.type": component_type, "fastmcp.component.key": component_key, + **get_protocol_span_attributes(), **get_auth_span_attributes(), **get_session_span_attributes(), } @@ -131,6 +147,7 @@ def seam_span(method: str, server_name: str) -> Generator[Span, None, None]: { "mcp.method.name": method, "fastmcp.server.name": server_name, + **get_protocol_span_attributes(), **get_auth_span_attributes(), **get_session_span_attributes(), } @@ -244,6 +261,7 @@ __all__ = [ "SEAM_SPAN_MARKER", "delegate_span", "get_auth_span_attributes", + "get_protocol_span_attributes", "get_session_span_attributes", "record_span_exception", "seam_span", diff --git a/fastmcp_slim/fastmcp/settings.py b/fastmcp_slim/fastmcp/settings.py index dccfcf5f1..a2a37d24a 100644 --- a/fastmcp_slim/fastmcp/settings.py +++ b/fastmcp_slim/fastmcp/settings.py @@ -210,6 +210,24 @@ class Settings(BaseSettings): ), ] = True + enable_telemetry: Annotated[ + bool, + Field( + description=inspect.cleandoc( + """ + Whether FastMCP's native OpenTelemetry instrumentation is active. + Enabled by default: FastMCP uses only the OpenTelemetry API, so + span creation is a no-op with negligible overhead unless an + OpenTelemetry SDK and exporter are configured. Set to False to + turn instrumentation off entirely, in which case FastMCP's span + helpers become a transparent pass-through: no FastMCP spans are + created even when an SDK is configured, and the surrounding OTel + trace context is left untouched. + """ + ) + ), + ] = True + deprecation_warnings: Annotated[ bool, Field( diff --git a/fastmcp_slim/fastmcp/telemetry.py b/fastmcp_slim/fastmcp/telemetry.py index 0965b8b71..c1bb66dfd 100644 --- a/fastmcp_slim/fastmcp/telemetry.py +++ b/fastmcp_slim/fastmcp/telemetry.py @@ -21,13 +21,24 @@ Example usage with SDK: ``` """ +from collections.abc import Iterator +from contextlib import contextmanager from typing import Any from opentelemetry import context as otel_context from opentelemetry import propagate, trace from opentelemetry.context import Context -from opentelemetry.trace import Span, Status, StatusCode, Tracer +from opentelemetry.trace import ( + INVALID_SPAN, + NoOpTracer, + Span, + SpanKind, + Status, + StatusCode, + Tracer, +) from opentelemetry.trace import get_tracer as otel_get_tracer +from opentelemetry.util import types as otel_types INSTRUMENTATION_NAME = "fastmcp" @@ -35,15 +46,60 @@ TRACE_PARENT_KEY = "traceparent" TRACE_STATE_KEY = "tracestate" +class _DisabledTracer(NoOpTracer): + """A tracer that neither records spans nor touches the OTel context. + + When telemetry is disabled FastMCP must be fully transparent. The stock + `NoOpTracer.start_as_current_span` still *attaches* a `NonRecordingSpan` to + the current OTel context, so an enclosing application span (from ASGI/HTTP + instrumentation or a user-created span) is hidden while a FastMCP span + helper is active — `trace.get_current_span()` inside a handler would then + return that non-recording span instead of the caller's span. This tracer + yields the invalid span *without* entering it as current, leaving the + surrounding trace context untouched. + """ + + @contextmanager + def start_as_current_span( + self, + name: str, + context: Context | None = None, + kind: SpanKind = SpanKind.INTERNAL, + attributes: otel_types.Attributes = None, + links: Any = None, + start_time: int | None = None, + record_exception: bool = True, + set_status_on_exception: bool = True, + end_on_exit: bool = True, + ) -> Iterator[Span]: + yield INVALID_SPAN + + +_DISABLED_TRACER = _DisabledTracer() + + def get_tracer(version: str | None = None) -> Tracer: """Get the FastMCP tracer for creating spans. + Instrumentation is on by default. FastMCP uses only the OpenTelemetry API, + so span creation is a no-op with negligible overhead unless an OpenTelemetry + SDK and exporter are configured. Set `fastmcp.settings.enable_telemetry` to + False (env `FASTMCP_ENABLE_TELEMETRY=false`) to turn instrumentation off + entirely, in which case this returns a pass-through tracer that leaves the + current OTel context untouched even when an SDK is configured. + Args: version: Optional version string for the instrumentation Returns: - A tracer instance. Returns a no-op tracer if no SDK is configured. + A tracer instance. Returns a non-attaching pass-through tracer if + telemetry is disabled; span creation is otherwise a no-op unless an SDK + is configured. """ + import fastmcp + + if not fastmcp.settings.enable_telemetry: + return _DISABLED_TRACER return otel_get_tracer(INSTRUMENTATION_NAME, version) diff --git a/tests/server/telemetry/test_server_tracing.py b/tests/server/telemetry/test_server_tracing.py index c04f49fd6..3c6594aa5 100644 --- a/tests/server/telemetry/test_server_tracing.py +++ b/tests/server/telemetry/test_server_tracing.py @@ -5,9 +5,11 @@ from __future__ import annotations from unittest.mock import patch import pytest +from opentelemetry import trace from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter -from opentelemetry.trace import SpanKind, StatusCode +from opentelemetry.trace import Span, SpanKind, StatusCode +import fastmcp from fastmcp import Client, FastMCP from fastmcp.exceptions import NotFoundError, ToolError from fastmcp.server.auth import AccessToken @@ -575,3 +577,187 @@ class TestFailurePathServerSpan: tool_call_server_spans[0].attributes is not None and tool_call_server_spans[0].attributes["mcp.method.name"] == "tools/call" ) + + +class TestTelemetryEnabledByDefault: + """Instrumentation is on by default and controllable via the off-switch. + + FastMCP uses only the OpenTelemetry API, so spans are created unconditionally + and light up when an SDK is configured. `FASTMCP_ENABLE_TELEMETRY=false` + (`fastmcp.settings.enable_telemetry`) turns span creation off entirely, so no + FastMCP spans are exported even with an SDK configured. + """ + + async def test_spans_fire_by_default(self, trace_exporter: InMemorySpanExporter): + """No opt-in required: a tool call produces a span out of the box.""" + assert fastmcp.settings.enable_telemetry is True + + mcp = FastMCP("test-server") + + @mcp.tool() + def greet(name: str) -> str: + return f"Hello, {name}!" + + await mcp.call_tool("greet", {"name": "World"}) + + spans = trace_exporter.get_finished_spans() + assert len(spans) == 1 + assert spans[0].name == "tools/call greet" + + async def test_off_switch_suppresses_spans( + self, + trace_exporter: InMemorySpanExporter, + monkeypatch: pytest.MonkeyPatch, + ): + """With telemetry disabled, no spans are created even with an SDK + configured (the exporter fixture installs one).""" + monkeypatch.setattr(fastmcp.settings, "enable_telemetry", False) + + mcp = FastMCP("test-server") + + @mcp.tool() + def greet(name: str) -> str: + return f"Hello, {name}!" + + result = await mcp.call_tool("greet", {"name": "World"}) + assert "Hello, World!" in str(result) + + spans = trace_exporter.get_finished_spans() + assert len(spans) == 0 + + async def test_off_switch_suppresses_spans_via_client( + self, + trace_exporter: InMemorySpanExporter, + monkeypatch: pytest.MonkeyPatch, + ): + """The off-switch suppresses every FastMCP span on the full request path + (seam SERVER span and FastMCP CLIENT span alike). + + The SDK's own low-level `mcp-python-sdk` CLIENT spans ("MCP send ...") + are governed by the user's OpenTelemetry SDK, not FastMCP's off-switch, + so they may still appear — the assertion filters them out. + """ + monkeypatch.setattr(fastmcp.settings, "enable_telemetry", False) + + mcp = FastMCP("test-server") + + @mcp.tool() + def greet(name: str) -> str: + return f"Hello, {name}!" + + async with Client(mcp) as client: + await client.call_tool("greet", {"name": "World"}) + + spans = trace_exporter.get_finished_spans() + fastmcp_spans = [ + s + for s in spans + if s.instrumentation_scope is not None + and s.instrumentation_scope.name == "fastmcp" + ] + assert fastmcp_spans == [] + # In particular, no FastMCP SERVER span (FastMCP owns all SERVER spans). + assert [s for s in spans if s.kind == SpanKind.SERVER] == [] + + async def test_off_switch_leaves_enclosing_span_current( + self, + trace_exporter: InMemorySpanExporter, + monkeypatch: pytest.MonkeyPatch, + ): + """Disabling telemetry must be a transparent pass-through. + + The stock OpenTelemetry `NoOpTracer.start_as_current_span` attaches a + `NonRecordingSpan` as the current span, which would hijack the trace + context from an enclosing application span. With FastMCP's off-switch, + `trace.get_current_span()` inside a handler must still return the + caller's enclosing span, and attributes written there must land on it. + """ + monkeypatch.setattr(fastmcp.settings, "enable_telemetry", False) + + tracer = trace.get_tracer("test-enclosing") + captured: dict[str, Span] = {} + + mcp = FastMCP("test-server") + + @mcp.tool() + def annotate() -> str: + current = trace.get_current_span() + captured["current"] = current + current.set_attribute("tool.touched", True) + return "ok" + + with tracer.start_as_current_span("enclosing-app-span") as enclosing: + await mcp.call_tool("annotate", {}) + # The tool ran with the enclosing span still current — FastMCP did + # not attach a replacement (non-recording) span. + assert captured["current"] is enclosing + assert enclosing.is_recording() + + spans = trace_exporter.get_finished_spans() + app_spans = [s for s in spans if s.name == "enclosing-app-span"] + assert len(app_spans) == 1 + assert app_spans[0].attributes is not None + assert app_spans[0].attributes["tool.touched"] is True + # No FastMCP spans were exported. + fastmcp_spans = [ + s + for s in spans + if s.instrumentation_scope is not None + and s.instrumentation_scope.name == "fastmcp" + ] + assert fastmcp_spans == [] + + +class TestProtocolVersionAttribute: + """The SERVER span carries `mcp.protocol.version`, matching the SDK. + + FastMCP drops the SDK's `OpenTelemetryMiddleware` to avoid a duplicate SERVER + span, so it re-emits the SDK's `mcp.protocol.version` attribute on its own + span for parity. + """ + + async def test_tool_call_span_has_protocol_version( + self, trace_exporter: InMemorySpanExporter + ): + mcp = FastMCP("test-server") + + @mcp.tool() + def greet(name: str) -> str: + return f"Hello, {name}!" + + async with Client(mcp) as client: + await client.call_tool("greet", {"name": "World"}) + + spans = trace_exporter.get_finished_spans() + server_span = next( + s + for s in spans + if s.kind == SpanKind.SERVER + and s.attributes is not None + and s.attributes.get("mcp.method.name") == "tools/call" + ) + assert server_span.attributes is not None + version = server_span.attributes.get("mcp.protocol.version") + assert isinstance(version, str) + assert version + + async def test_seam_span_has_protocol_version( + self, trace_exporter: InMemorySpanExporter + ): + """Seam-only methods (never reaching the high-level path) also carry the + protocol version.""" + mcp = FastMCP("test-server") + + async with Client(mcp) as client: + await client.set_logging_level("info") + + spans = trace_exporter.get_finished_spans() + seam_span = next( + s + for s in spans + if s.kind == SpanKind.SERVER and s.name == "logging/setLevel" + ) + assert seam_span.attributes is not None + version = seam_span.attributes.get("mcp.protocol.version") + assert isinstance(version, str) + assert version From 291fab87892215d171be410b137bea0aa48e2198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=9F=B3=E5=B2=B3=E5=B3=B0?= <132282304+syf2211@users.noreply.github.com> Date: Sat, 18 Jul 2026 05:28:30 +0800 Subject: [PATCH 04/22] fix(server): omit ScalarElicitationType wrapper title from elicitation schemas (#4502) --- fastmcp_slim/fastmcp/server/elicitation.py | 7 +++++++ tests/client/test_elicitation.py | 2 -- tests/client/test_elicitation_enums.py | 10 ++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/fastmcp_slim/fastmcp/server/elicitation.py b/fastmcp_slim/fastmcp/server/elicitation.py index 5947d223b..8a3fe0a5e 100644 --- a/fastmcp_slim/fastmcp/server/elicitation.py +++ b/fastmcp_slim/fastmcp/server/elicitation.py @@ -378,6 +378,13 @@ def get_elicitation_schema(response_type: type[T]) -> dict[str, Any]: ) schema = compress_schema(schema) + # Pydantic emits the internal wrapper class name as a top-level title for + # ScalarElicitationType schemas. That value is not meaningful on the wire and + # breaks strict clients (e.g. Codex) that reject unknown top-level fields. + origin = get_origin(response_type) + if origin is ScalarElicitationType or response_type is ScalarElicitationType: + schema.pop("title", None) + # Validate the schema to ensure it follows MCP elicitation requirements validate_elicitation_json_schema(schema) diff --git a/tests/client/test_elicitation.py b/tests/client/test_elicitation.py index 1db306077..2686edb79 100644 --- a/tests/client/test_elicitation.py +++ b/tests/client/test_elicitation.py @@ -107,11 +107,9 @@ async def test_elicitation_handler_parameters(): await client.call_tool("test_tool", {}) assert captured_params["message"] == "Test message" - assert "ScalarElicitationType" in str(captured_params["response_type"]) assert captured_params["params"].requested_schema == { "properties": {"value": {"title": "Value", "type": "integer"}}, "required": ["value"], - "title": "ScalarElicitationType", "type": "object", } assert captured_params["ctx"] is not None diff --git a/tests/client/test_elicitation_enums.py b/tests/client/test_elicitation_enums.py index baa281edd..f1beb62d5 100644 --- a/tests/client/test_elicitation_enums.py +++ b/tests/client/test_elicitation_enums.py @@ -514,3 +514,13 @@ class TestElicitationDefaults: assert "default" in props["string_field"] assert "default" in props["integer_field"] + + +def test_scalar_elicitation_schema_omits_wrapper_title() -> None: + """Scalar/list wrappers must not leak the internal class name on the wire.""" + from fastmcp.server.elicitation import parse_elicit_response_type + + schema = parse_elicit_response_type(["yes", "no"]).schema + + assert "title" not in schema + assert schema["properties"]["value"]["enum"] == ["yes", "no"] From 1fca15abe6c623edb21afda59e4826a3e5252687 Mon Sep 17 00:00:00 2001 From: Burt Matthews <80060660+earfman@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:28:43 -0700 Subject: [PATCH 05/22] Skip unsupported JWKS keys instead of failing the whole key set (#4515) (#4517) --- .../fastmcp/server/auth/providers/jwt.py | 29 ++++++- tests/server/auth/test_jwt_provider.py | 81 ++++++++++++++++++- 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/fastmcp_slim/fastmcp/server/auth/providers/jwt.py b/fastmcp_slim/fastmcp/server/auth/providers/jwt.py index 6ed151fdf..9167eb9d1 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/jwt.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/jwt.py @@ -347,11 +347,26 @@ class JWTVerifier(TokenVerifier): try: jwks_data = await self._fetch_jwks() - # Cache all keys + # Cache all usable keys. A key that cannot be converted (e.g. an + # unsupported kty like OKP/Ed25519) is skipped rather than failing + # the whole set — per RFC 7517 §5, clients should ignore JWKs they + # don't understand. Otherwise one exotic key published by the + # authorization server would reject every token, including ones + # signed by supported keys in the same set (#4515). self._jwks_cache = {} + skipped_kids: set[str] = set() for key_data in jwks_data.get("keys", []): + if not isinstance(key_data, dict): + self.logger.debug("Skipping non-object JWKS entry: %r", key_data) + continue key_kid = key_data.get("kid") - public_key = _jwk_to_pem(key_data) + try: + public_key = _jwk_to_pem(key_data) + except (JoseError, TypeError, KeyError, ValueError) as e: + self.logger.debug("Skipping unusable JWKS key %r: %s", key_kid, e) + if key_kid: + skipped_kids.add(key_kid) + continue if key_kid: self._jwks_cache[key_kid] = public_key @@ -364,6 +379,16 @@ class JWTVerifier(TokenVerifier): # Select the appropriate key if kid: if kid not in self._jwks_cache: + if kid in skipped_kids: + self.logger.debug( + "JWKS key lookup failed: key ID '%s' is present " + "but its key type is unsupported", + kid, + ) + raise ValueError( + f"Key ID '{kid}' found in JWKS but its key type " + "is unsupported" + ) self.logger.debug( "JWKS key lookup failed: key ID '%s' not found", kid ) diff --git a/tests/server/auth/test_jwt_provider.py b/tests/server/auth/test_jwt_provider.py index 8ccb1f37c..1961a0c18 100644 --- a/tests/server/auth/test_jwt_provider.py +++ b/tests/server/auth/test_jwt_provider.py @@ -1,6 +1,6 @@ import time from collections.abc import AsyncGenerator -from typing import Any +from typing import Any, cast from unittest.mock import patch import pytest @@ -559,6 +559,85 @@ class TestBearerTokenJWKS: assert access_token.claims.get("iss") == issuer assert access_token.claims.get("aud") == audience + async def test_jwks_skips_unsupported_key_types( + self, + rsa_key_pair: RSAKeyPair, + jwks_provider: JWTVerifier, + mock_jwks_data: JWKSData, + httpx_mock: HTTPXMock, + mock_dns, + ): + """An unsupported key type in the JWKS (e.g. OKP/Ed25519) must be + skipped, not poison the whole key set - #4515. + + Some authorization servers (e.g. Rauthy, Ory Hydra) publish an + Ed25519 key alongside RSA keys; tokens signed by the RSA keys must + still verify. + """ + okp_key = cast( + "JWKData", + { + "kty": "OKP", + "crv": "Ed25519", + "kid": "ed25519-key", + "alg": "EdDSA", + "use": "sig", + "x": "11qYAYKxCrfVS_7TyWQHOg7hcvPapiMlrwIaaPcHURo", + }, + ) + mock_jwks_data["keys"][0]["kid"] = "test-key-1" + # Unsupported key FIRST, so an unguarded conversion loop would + # abort before reaching the RSA key the token needs + mock_jwks_data["keys"].insert(0, okp_key) + httpx_mock.add_response(json=mock_jwks_data) + + token = rsa_key_pair.create_token( + subject="test-user", + issuer="https://test.example.com", + audience="https://api.example.com", + kid="test-key-1", + ) + + access_token = await jwks_provider.load_access_token(token) + assert access_token is not None + assert access_token.client_id == "test-user" + + async def test_jwks_with_only_unsupported_keys_rejects_cleanly( + self, + rsa_key_pair: RSAKeyPair, + jwks_provider: JWTVerifier, + httpx_mock: HTTPXMock, + mock_dns, + ): + """If every key in the JWKS is unsupported, verification fails + cleanly (returns None) rather than crashing - #4515.""" + okp_only = { + "keys": [ + cast( + "JWKData", + { + "kty": "OKP", + "crv": "Ed25519", + "kid": "ed25519-key", + "alg": "EdDSA", + "use": "sig", + "x": "11qYAYKxCrfVS_7TyWQHOg7hcvPapiMlrwIaaPcHURo", + }, + ) + ] + } + httpx_mock.add_response(json=okp_only) + + token = rsa_key_pair.create_token( + subject="test-user", + issuer="https://test.example.com", + audience="https://api.example.com", + kid="ed25519-key", + ) + + access_token = await jwks_provider.load_access_token(token) + assert access_token is None + async def test_jwks_token_validation_with_invalid_key( self, rsa_key_pair: RSAKeyPair, From 6202008cf327389325206a6ef25ff608765253ac Mon Sep 17 00:00:00 2001 From: WinkleMad Date: Sat, 18 Jul 2026 02:59:01 +0530 Subject: [PATCH 06/22] Don't mutate the caller's schema in compress_schema (#4492) --- fastmcp_slim/fastmcp/utilities/json_schema.py | 5 +++ tests/utilities/test_json_schema.py | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/fastmcp_slim/fastmcp/utilities/json_schema.py b/fastmcp_slim/fastmcp/utilities/json_schema.py index 2cd604e78..f8996c2f4 100644 --- a/fastmcp_slim/fastmcp/utilities/json_schema.py +++ b/fastmcp_slim/fastmcp/utilities/json_schema.py @@ -498,6 +498,11 @@ def _single_pass_optimize( if not (prune_defs or prune_titles or prune_additional_properties): return schema # Nothing to do + # Work on a copy so the caller's schema is never mutated (see docstring). The + # pruning phases below pop keys/$defs in place, which would otherwise corrupt a + # shared dict such as a live Tool.input_schema passed straight to compress_schema. + schema = copy.deepcopy(schema) + # Phase 1: Collect references and apply simple cleanups # Track which $defs are referenced from the main schema and from other $defs root_refs: set[str] = set() # $defs referenced directly from main schema diff --git a/tests/utilities/test_json_schema.py b/tests/utilities/test_json_schema.py index 1620127e1..8d16665da 100644 --- a/tests/utilities/test_json_schema.py +++ b/tests/utilities/test_json_schema.py @@ -358,6 +358,39 @@ class TestDereferenceRefs: class TestCompressSchema: """Tests for the compress_schema function.""" + def test_does_not_mutate_input(self): + """compress_schema must return a new dict and leave the caller's schema + untouched, even when it prunes titles, additionalProperties and unused + $defs (a live Tool.input_schema is passed straight in at some call sites).""" + schema = { + "type": "object", + "title": "MySchema", + "additionalProperties": False, + "properties": { + "a": {"type": "string", "title": "A"}, + "b": { + "type": "object", + "title": "B", + "properties": {"c": {"type": "integer", "title": "C"}}, + }, + }, + "$defs": {"Unused": {"type": "string", "title": "Unused"}}, + } + original = copy.deepcopy(schema) + + result = compress_schema( + schema, prune_titles=True, prune_additional_properties=True + ) + + # The input is untouched... + assert schema == original + assert result is not schema + # ...and the returned copy really was optimized (so it is not a no-op). + assert "title" not in result + assert "title" not in result["properties"]["b"]["properties"]["c"] + assert "additionalProperties" not in result + assert "$defs" not in result + def test_preserves_refs_by_default(self): """Test that compress_schema preserves $refs by default.""" schema = { From 977f02347b0074a1e2c810db8e65fc7e7687efa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire?= Date: Fri, 17 Jul 2026 23:29:15 +0200 Subject: [PATCH 07/22] Forward upstream instructions through create_proxy (#4512) Co-authored-by: Mistral Vibe --- .../fastmcp/server/providers/proxy.py | 21 ++++++++++++- .../providers/proxy/test_proxy_server.py | 30 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/fastmcp_slim/fastmcp/server/providers/proxy.py b/fastmcp_slim/fastmcp/server/providers/proxy.py index fd0817af1..29e6d6c19 100644 --- a/fastmcp_slim/fastmcp/server/providers/proxy.py +++ b/fastmcp_slim/fastmcp/server/providers/proxy.py @@ -106,6 +106,7 @@ class ProxyInitializeMiddleware(Middleware): ], ) -> mcp_types.InitializeResult | None: client = await self.proxy._get_client() + upstream_instructions: str | None = None try: if isinstance(client, ProxyClient): ctx = context.fastmcp_context @@ -116,6 +117,11 @@ class ProxyInitializeMiddleware(Middleware): ) async with client: await client.initialize() + # Capture the upstream's instructions while the session is live; + # `initialize_result` clears once the client context exits. + init_result = client.initialize_result + if init_result is not None: + upstream_instructions = init_result.instructions except MCPError: raise except ( @@ -128,7 +134,20 @@ class ProxyInitializeMiddleware(Middleware): ) as error: raise _proxy_upstream_error(error) from error - return await call_next(context) + result = await call_next(context) + + # Forward the upstream server's instructions unless the proxy defines its + # own. `instructions` is part of the MCP InitializeResult and is meant to + # steer the model, so a proxy that dropped it would silently degrade any + # downstream consumer relying on upstream guidance. + if ( + result is not None + and self.proxy.instructions is None + and upstream_instructions is not None + ): + result.instructions = upstream_instructions + + return result # ----------------------------------------------------------------------------- diff --git a/tests/server/providers/proxy/test_proxy_server.py b/tests/server/providers/proxy/test_proxy_server.py index 762a09abc..9f1579b45 100644 --- a/tests/server/providers/proxy/test_proxy_server.py +++ b/tests/server/providers/proxy/test_proxy_server.py @@ -189,6 +189,36 @@ async def test_create_proxy_with_transport(fastmcp_server): assert result.data == "Hello, Test!" +async def test_proxy_forwards_upstream_instructions(): + """A proxy should surface the upstream server's instructions in the handshake.""" + upstream = FastMCP(name="upstream", instructions="USE_THIS_MARKER_123") + proxy = create_proxy(upstream, name="proxy") + + async with Client(proxy) as client: + assert client.initialize_result is not None + assert client.initialize_result.instructions == "USE_THIS_MARKER_123" + + +async def test_proxy_own_instructions_take_precedence(): + """Instructions explicitly set on the proxy override the upstream's.""" + upstream = FastMCP(name="upstream", instructions="upstream instructions") + proxy = create_proxy(upstream, name="proxy", instructions="proxy instructions") + + async with Client(proxy) as client: + assert client.initialize_result is not None + assert client.initialize_result.instructions == "proxy instructions" + + +async def test_proxy_instructions_none_when_upstream_has_none(): + """A proxy over an upstream without instructions reports no instructions.""" + upstream = FastMCP(name="upstream") + proxy = create_proxy(upstream, name="proxy") + + async with Client(proxy) as client: + assert client.initialize_result is not None + assert client.initialize_result.instructions is None + + def test_create_proxy_with_url(): """create_proxy should accept a URL without connecting.""" proxy = create_proxy("http://example.com/mcp/") From b623c23183a1ee25bce1db0ab47715eab05b151e Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:33:57 -0400 Subject: [PATCH 08/22] Serialize deep object query parameters (#4523) --- .../fastmcp/utilities/openapi/director.py | 12 +++--- tests/utilities/openapi/test_director.py | 39 +++++++++++++++++++ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/fastmcp_slim/fastmcp/utilities/openapi/director.py b/fastmcp_slim/fastmcp/utilities/openapi/director.py index 461573960..9c8a52695 100644 --- a/fastmcp_slim/fastmcp/utilities/openapi/director.py +++ b/fastmcp_slim/fastmcp/utilities/openapi/director.py @@ -313,13 +313,13 @@ class RequestDirector: if not value: continue if explode: - # form,explode=true on objects: each property becomes - # a separate query parameter. - # e.g. {"R": 100, "G": 200} → R=100&G=200 for k, v in value.items(): - serialized[_query_scalar_to_str(k)] = _query_scalar_to_str( - v - ) + # deepObject keeps the parent parameter name; + # form style emits each property as a bare key. + property_name = _query_scalar_to_str(k) + if param_info.style == "deepObject": + property_name = f"{key}[{property_name}]" + serialized[property_name] = _query_scalar_to_str(v) else: style = param_info.style or "form" delimiter = self._STYLE_DELIMITERS.get(style, ",") diff --git a/tests/utilities/openapi/test_director.py b/tests/utilities/openapi/test_director.py index 6e8fffb82..651d8c4a9 100644 --- a/tests/utilities/openapi/test_director.py +++ b/tests/utilities/openapi/test_director.py @@ -890,6 +890,45 @@ class TestQueryParameterSerialization: assert "myAttribute=true" in url assert "data=" not in url + def test_deep_object_explode_true_uses_bracket_notation(self, director): + route = HTTPRoute( + path="/items", + method="GET", + operation_id="list_items", + parameters=[ + ParameterInfo( + name="filter", + location="query", + required=True, + schema={ + "type": "object", + "properties": { + "eq": {"type": "string"}, + "display name": {"type": "string"}, + }, + }, + explode=True, + style="deepObject", + ) + ], + parameter_map={ + "filter": {"location": "query", "openapi_name": "filter"}, + }, + ) + + request = director.build( + route, + {"filter": {"eq": "foo/bar", "display name": "active & ready"}}, + "https://example.com", + ) + + assert request.url.params["filter[eq]"] == "foo/bar" + assert request.url.params["filter[display name]"] == "active & ready" + assert "filter%5Beq%5D=foo%2Fbar" in str(request.url) + assert "filter%5Bdisplay+name%5D=active+%26+ready" in str(request.url) + assert "eq" not in request.url.params + assert "display name" not in request.url.params + def test_explode_default_dict_expands_to_separate_params(self, director): """Default explode (None → true) on objects expands properties.""" route = HTTPRoute( From ff2fc234b2b011db4fa7f2f45fb6601401df18c2 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:37:12 -0400 Subject: [PATCH 09/22] Trace client task management requests (#4525) --- docs/servers/telemetry.mdx | 55 ++++++- .../fastmcp/client/mixins/task_management.py | 150 ++++++++++++------ .../telemetry/test_client_task_tracing.py | 93 +++++++++++ 3 files changed, 248 insertions(+), 50 deletions(-) create mode 100644 tests/client/telemetry/test_client_task_tracing.py diff --git a/docs/servers/telemetry.mdx b/docs/servers/telemetry.mdx index 7553868f5..77fb0f1de 100644 --- a/docs/servers/telemetry.mdx +++ b/docs/servers/telemetry.mdx @@ -6,7 +6,7 @@ icon: chart-line tag: NEW --- -FastMCP includes native OpenTelemetry instrumentation for observability. Traces are automatically generated for tool, prompt, resource, and resource template operations, providing visibility into server behavior, request handling, and provider delegation chains. +FastMCP includes native OpenTelemetry instrumentation for observability. Traces are automatically generated for tool, prompt, resource, resource template, and task management operations, providing visibility into server behavior, request handling, and provider delegation chains. ## How It Works @@ -69,12 +69,13 @@ The server creates spans for each operation using [MCP semantic conventions](htt | `tools/call {name}` | Tool execution (e.g., `tools/call get_weather`) | | `resources/read` | Resource read (URI in `mcp.resource.uri` attribute, not span name) | | `prompts/get {name}` | Prompt render (e.g., `prompts/get greeting`) | +| `tasks/{operation}` | Task management (`tasks/get`, `tasks/result`, `tasks/list`, or `tasks/cancel`) | For mounted servers, an additional `delegate {name}` span shows the delegation to the child server. ### Client Spans -The FastMCP client creates spans for outgoing requests with the same naming pattern (`tools/call {name}`, `resources/read`, `prompts/get {name}`). +The FastMCP client creates spans for outgoing requests with the same naming pattern (`tools/call {name}`, `resources/read`, `prompts/get {name}`, and `tasks/{operation}`). ### Span Hierarchy @@ -95,6 +96,54 @@ tools/call remote_search (CLIENT) └── [remote server spans via trace context propagation] ``` +### Background tasks + +Background task traces have two parts: + +- Task submission and management requests use normal client-to-server context propagation. `tasks/get`, `tasks/result`, `tasks/list`, and `tasks/cancel` server spans are descendants of the corresponding FastMCP client spans. +- Deferred execution runs in a Docket worker. Docket records its `CONSUMER` span as a new trace root with a span link to the submission context, rather than making it a child of the submission span. Custom spans created inside the task are children of that worker span. + +Span links preserve the causal relationship without forcing worker sampling to inherit the submit trace's sampling decision. Some tracing backends do not display links prominently, so the worker trace may look disconnected even though the link is present. + +Frequent status and list polling can produce more detail than you need. You can drop those client and server spans with a sampler that checks the span name before delegating to `ParentBased`: + +```python +from opentelemetry import trace +from opentelemetry.sdk.trace import TracerProvider +from opentelemetry.sdk.trace.sampling import ( + ALWAYS_ON, + Decision, + ParentBased, + Sampler, + SamplingResult, +) + + +class DropTaskPolls(Sampler): + def __init__(self): + self._delegate = ParentBased(ALWAYS_ON) + + def should_sample(self, parent_context, trace_id, name, *args, **kwargs): + if name in {"tasks/get", "tasks/list"}: + return SamplingResult(Decision.DROP) + return self._delegate.should_sample( + parent_context, + trace_id, + name, + *args, + **kwargs, + ) + + def get_description(self): + return "DropTaskPolls" + + +provider = TracerProvider(sampler=DropTaskPolls()) +trace.set_tracer_provider(provider) +``` + +The name check must happen before `ParentBased` delegates. If the name-based sampler is nested inside `ParentBased`, it is not consulted for child spans whose parent was already sampled. + ## Programmatic Configuration For more control, configure the SDK in your Python code before importing FastMCP: @@ -276,7 +325,7 @@ FastMCP implements the [OpenTelemetry MCP semantic conventions](https://opentele | Attribute | Description | |-----------|-------------| -| `mcp.method.name` | The MCP method being called (`tools/call`, `resources/read`, `prompts/get`) | +| `mcp.method.name` | The MCP method being called (`tools/call`, `resources/read`, `prompts/get`, `tasks/get`, etc.) | | `mcp.protocol.version` | The negotiated MCP protocol version for the request | | `mcp.session.id` | Session identifier for the MCP connection | | `mcp.resource.uri` | The resource URI (for resource operations) | diff --git a/fastmcp_slim/fastmcp/client/mixins/task_management.py b/fastmcp_slim/fastmcp/client/mixins/task_management.py index a011b138a..533f78304 100644 --- a/fastmcp_slim/fastmcp/client/mixins/task_management.py +++ b/fastmcp_slim/fastmcp/client/mixins/task_management.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, cast import mcp_types from mcp import MCPError @@ -23,6 +23,8 @@ from mcp_types import ( PaginatedRequestParams, ) +from fastmcp.client.telemetry import client_span +from fastmcp.telemetry import inject_trace_context from fastmcp.utilities.logging import get_logger logger = get_logger(__name__) @@ -64,13 +66,27 @@ class ClientTaskManagementMixin: RuntimeError: If client not connected MCPError: If the request results in a TimeoutError | JSONRPCError """ - request = GetTaskRequest(params=GetTaskRequestParams(task_id=task_id)) - return await self._await_with_session_monitoring( - self.session.send_request( - request=request, # type: ignore[arg-type] - result_type=GetTaskResult, + with client_span( + "tasks/get", + "tasks/get", + task_id, + session_id=self.transport.get_session_id(), + ): + request_meta = cast( + "mcp_types.RequestParamsMeta | None", inject_trace_context() + ) + request = GetTaskRequest( + params=GetTaskRequestParams( + task_id=task_id, + _meta=request_meta, # type: ignore[unknown-argument] + ) + ) + return await self._await_with_session_monitoring( + self.session.send_request( + request=request, # type: ignore[arg-type] + result_type=GetTaskResult, + ) ) - ) async def get_task_result(self: Client, task_id: str) -> Any: """Retrieve the raw result of a completed background task. @@ -88,20 +104,32 @@ class ClientTaskManagementMixin: RuntimeError: If client not connected, task not found, or task failed MCPError: If the request results in a TimeoutError | JSONRPCError """ - request = GetTaskPayloadRequest( - params=GetTaskPayloadRequestParams(task_id=task_id) - ) - # Return raw result - Task classes handle type-specific parsing - result = await self._await_with_session_monitoring( - self.session.send_request( - request=request, # type: ignore[arg-type] - result_type=_RawTaskPayloadResult, + with client_span( + "tasks/result", + "tasks/result", + task_id, + session_id=self.transport.get_session_id(), + ): + request_meta = cast( + "mcp_types.RequestParamsMeta | None", inject_trace_context() ) - ) - # Return as dict for compatibility with Task class parsing. The payload - # fields (content, structuredContent, messages, contents, ...) survive - # via the permissive result type's extra="allow". - return result.model_dump(exclude_none=True, by_alias=True) + request = GetTaskPayloadRequest( + params=GetTaskPayloadRequestParams( + task_id=task_id, + _meta=request_meta, # type: ignore[unknown-argument] + ) + ) + # Return raw result - Task classes handle type-specific parsing + result = await self._await_with_session_monitoring( + self.session.send_request( + request=request, # type: ignore[arg-type] + result_type=_RawTaskPayloadResult, + ) + ) + # Return as dict for compatibility with Task class parsing. The payload + # fields (content, structuredContent, messages, contents, ...) survive + # via the permissive result type's extra="allow". + return result.model_dump(exclude_none=True, by_alias=True) async def list_tasks( self: Client, @@ -127,31 +155,45 @@ class ClientTaskManagementMixin: RuntimeError: If client not connected MCPError: If the request results in a TimeoutError | JSONRPCError """ - # Send protocol request - params = PaginatedRequestParams(cursor=cursor, limit=limit) # type: ignore[call-arg] # Optional field in MCP SDK # ty:ignore[unknown-argument] - request = ListTasksRequest(params=params) - server_response = await self._await_with_session_monitoring( - self.session.send_request( - request=request, # type: ignore[invalid-argument-type] - result_type=mcp_types.ListTasksResult, + with client_span( + "tasks/list", + "tasks/list", + "", + session_id=self.transport.get_session_id(), + ): + request_meta = cast( + "mcp_types.RequestParamsMeta | None", inject_trace_context() ) - ) - # If server returned tasks, use those - if server_response.tasks: - return server_response.model_dump(by_alias=True) + # Send protocol request + params = PaginatedRequestParams( + cursor=cursor, + limit=limit, # type: ignore[call-arg] # Optional field in MCP SDK # ty:ignore[unknown-argument] + _meta=request_meta, # type: ignore[unknown-argument] + ) + request = ListTasksRequest(params=params) + server_response = await self._await_with_session_monitoring( + self.session.send_request( + request=request, # type: ignore[invalid-argument-type] + result_type=mcp_types.ListTasksResult, + ) + ) - # Server returned empty - fall back to client-side tracking - tasks = [] - for task_id in list(self._submitted_task_ids)[:limit]: - try: - status = await self.get_task_status(task_id) - tasks.append(status.model_dump(by_alias=True)) - except MCPError: - # Task may have expired or been deleted, skip it - continue + # If server returned tasks, use those + if server_response.tasks: + return server_response.model_dump(by_alias=True) - return {"tasks": tasks, "nextCursor": None} + # Server returned empty - fall back to client-side tracking + tasks = [] + for task_id in list(self._submitted_task_ids)[:limit]: + try: + status = await self.get_task_status(task_id) + tasks.append(status.model_dump(by_alias=True)) + except MCPError: + # Task may have expired or been deleted, skip it + continue + + return {"tasks": tasks, "nextCursor": None} async def cancel_task(self: Client, task_id: str) -> mcp_types.CancelTaskResult: """Cancel a task, transitioning it to cancelled state. @@ -169,10 +211,24 @@ class ClientTaskManagementMixin: RuntimeError: If task doesn't exist MCPError: If the request results in a TimeoutError | JSONRPCError """ - request = CancelTaskRequest(params=CancelTaskRequestParams(task_id=task_id)) - return await self._await_with_session_monitoring( - self.session.send_request( - request=request, # type: ignore[invalid-argument-type] - result_type=mcp_types.CancelTaskResult, + with client_span( + "tasks/cancel", + "tasks/cancel", + task_id, + session_id=self.transport.get_session_id(), + ): + request_meta = cast( + "mcp_types.RequestParamsMeta | None", inject_trace_context() + ) + request = CancelTaskRequest( + params=CancelTaskRequestParams( + task_id=task_id, + _meta=request_meta, # type: ignore[unknown-argument] + ) + ) + return await self._await_with_session_monitoring( + self.session.send_request( + request=request, # type: ignore[invalid-argument-type] + result_type=mcp_types.CancelTaskResult, + ) ) - ) diff --git a/tests/client/telemetry/test_client_task_tracing.py b/tests/client/telemetry/test_client_task_tracing.py new file mode 100644 index 000000000..4d0b71718 --- /dev/null +++ b/tests/client/telemetry/test_client_task_tracing.py @@ -0,0 +1,93 @@ +"""Tests for client OpenTelemetry tracing on task operations.""" + +import asyncio + +from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( + InMemorySpanExporter, +) +from opentelemetry.trace import SpanKind + +from fastmcp import Client, FastMCP + + +def assert_propagating_client_span( + trace_exporter: InMemorySpanExporter, + method: str, + component_key: str, +) -> None: + all_spans = trace_exporter.get_finished_spans() + spans = [span for span in all_spans if span.name == method] + client_span = next( + span + for span in spans + if span.attributes is not None and "fastmcp.server.name" not in span.attributes + ) + server_span = next( + span + for span in spans + if span.attributes is not None and "fastmcp.server.name" in span.attributes + ) + + assert client_span.kind == SpanKind.CLIENT + assert client_span.attributes is not None + assert client_span.attributes["mcp.method.name"] == method + assert client_span.attributes["fastmcp.component.key"] == component_key + assert server_span.parent is not None + assert server_span.context.trace_id == client_span.context.trace_id + + spans_by_id = {span.context.span_id: span for span in all_spans} + current = server_span + while current.parent is not None: + parent = spans_by_id.get(current.parent.span_id) + assert parent is not None + if parent.context.span_id == client_span.context.span_id: + break + current = parent + else: + raise AssertionError("Server span should descend from the client span") + + +async def test_list_tasks_creates_propagating_client_span( + trace_exporter: InMemorySpanExporter, +): + server = FastMCP("test-server") + + async with Client(server) as client: + await client.list_tasks() + + assert_propagating_client_span(trace_exporter, "tasks/list", "") + + +async def test_task_id_operations_create_propagating_client_spans( + trace_exporter: InMemorySpanExporter, +): + started = asyncio.Event() + server = FastMCP("test-server") + + @server.tool(task=True) + async def quick_tool() -> str: + return "done" + + @server.tool(task=True) + async def slow_tool() -> str: + started.set() + await asyncio.sleep(10) + return "done" + + async with Client(server) as client: + completed_task = await client.call_tool("quick_tool", task=True) + await completed_task.wait(timeout=2) + trace_exporter.clear() + + await client.get_task_status(completed_task.task_id) + await client.get_task_result(completed_task.task_id) + + running_task = await client.call_tool("slow_tool", task=True) + await asyncio.wait_for(started.wait(), timeout=2) + await client.cancel_task(running_task.task_id) + + assert_propagating_client_span(trace_exporter, "tasks/get", completed_task.task_id) + assert_propagating_client_span( + trace_exporter, "tasks/result", completed_task.task_id + ) + assert_propagating_client_span(trace_exporter, "tasks/cancel", running_task.task_id) From 918b85f9b2650607c0a4a2a53fdb1059921db071 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:37:28 -0400 Subject: [PATCH 10/22] Reject positional-only tool parameters (#4524) --- .../fastmcp/tools/function_parsing.py | 10 ++++++- tests/tools/tool/test_tool.py | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/fastmcp_slim/fastmcp/tools/function_parsing.py b/fastmcp_slim/fastmcp/tools/function_parsing.py index 9f653a336..79967f9a2 100644 --- a/fastmcp_slim/fastmcp/tools/function_parsing.py +++ b/fastmcp_slim/fastmcp/tools/function_parsing.py @@ -182,8 +182,16 @@ class ParsedFunction: ) -> ParsedFunction: if validate: sig = inspect.signature(fn) - # Reject functions with *args or **kwargs + # Reject signatures that cannot be represented by MCP's + # object-shaped tool arguments. for param in sig.parameters.values(): + if param.kind == inspect.Parameter.POSITIONAL_ONLY: + raise ValueError( + "Functions with positional-only parameters are not " + "supported as tools because MCP passes tool arguments by " + "name. Replace them with standard parameters that can be " + "passed as keywords." + ) if param.kind == inspect.Parameter.VAR_POSITIONAL: raise ValueError("Functions with *args are not supported as tools") if param.kind == inspect.Parameter.VAR_KEYWORD: diff --git a/tests/tools/tool/test_tool.py b/tests/tools/tool/test_tool.py index d877b7046..b6165cb5e 100644 --- a/tests/tools/tool/test_tool.py +++ b/tests/tools/tool/test_tool.py @@ -341,6 +341,32 @@ class TestToolFromFunction: ): Tool.from_function(func) + def test_tool_with_positional_only_parameters_not_allowed(self): + def func(a: int, /, b: int) -> int: + return a + b + + with pytest.raises( + ValueError, + match=( + "Functions with positional-only parameters are not supported as " + "tools.*standard parameters" + ), + ): + Tool.from_function(func) + + def test_tool_with_keyword_capable_parameters(self): + def func(a: int, *, b: int) -> int: + return a + b + + tool = Tool.from_function(func) + + assert tool.parameters["type"] == "object" + assert tool.parameters["required"] == ["a", "b"] + assert tool.parameters["properties"] == { + "a": {"type": "integer"}, + "b": {"type": "integer"}, + } + def test_tool_with_varkwargs_not_allowed(self): def func(a: int, b: int, **kwargs: int) -> int: """Add two numbers.""" From d779414f8ad0997d37d01da2ecca4e31fd4debfb Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:42:48 -0400 Subject: [PATCH 11/22] Screen templated resource parameters for path traversal by default (#4482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add ResourceSecurity screening for templated resources (defaults on) * Add tests for resource path-security screening * Document resource path-security; fix ty in tests * Carry child template security policy through provider mount Preserve a mounted template's explicit ResourceSecurity (per-param exemptions or a deliberate opt-out) through FastMCPProviderResourceTemplate.wrap so the parent read chokepoint honours it instead of the parent default. * Defer mcp SDK import so fastmcp.resources loads without the [mcp] extra * Make resource path-security docs examples self-contained and runnable * Match exempt_params under both hyphen and underscore spellings Template placeholders like {git-ref} extract as git_ref, so an exemption written with the natural URI-template spelling never matched. * Docs: describe net-depth traversal rule accurately; make example runnable The screening only rejects .. segments that escape the starting depth (foo/../bar passes) — saying any standalone .. is rejected overstated the guarantee. Also define DOCS_ROOT so the example runs. --- docs/development/v4-notes/change-register.mdx | 8 + docs/servers/resources.mdx | 82 ++- fastmcp_slim/fastmcp/exceptions.py | 11 + fastmcp_slim/fastmcp/resources/__init__.py | 2 + .../fastmcp/resources/function_resource.py | 8 + fastmcp_slim/fastmcp/resources/security.py | 162 ++++++ fastmcp_slim/fastmcp/resources/template.py | 32 ++ .../server/providers/fastmcp_provider.py | 1 + .../local_provider/decorators/resources.py | 8 + fastmcp_slim/fastmcp/server/server.py | 35 ++ tests/resources/test_resource_security.py | 473 ++++++++++++++++++ .../openapi/test_openapi_features.py | 13 +- 12 files changed, 824 insertions(+), 11 deletions(-) create mode 100644 fastmcp_slim/fastmcp/resources/security.py create mode 100644 tests/resources/test_resource_security.py diff --git a/docs/development/v4-notes/change-register.mdx b/docs/development/v4-notes/change-register.mdx index 33fe92a98..97259fb3d 100644 --- a/docs/development/v4-notes/change-register.mdx +++ b/docs/development/v4-notes/change-register.mdx @@ -324,6 +324,14 @@ FastMCP keeps its own DCR redirect-URI hardening (PRs #4419, #4408) regardless o *Verify:* recent commits `67527c1f` (block unsafe OAuth redirect schemes), `57a27992` (DNS rebinding), `cccb529f` (DCR redirect URI validation) on `main`. +### Templated resource parameters are path-screened by default — Breaking (behavior) + +Every templated resource now has its extracted parameter values screened for path-traversal (`..` segments), absolute paths, and null bytes **before the handler runs** — on by default, at the server's read chokepoint, covering local and provider-sourced (mounted/proxied) templates alike. Previously these payloads reached handlers raw; a template whose parameter flowed into a filesystem path or upstream URL was exposed unless the author added their own check. A rejected read now surfaces a non-leaky "resource not found" error (`-32602`) and a debug log. + +The check is component-based, matching the SDK's `contains_path_traversal`: only a standalone `..` segment is traversal, so values that merely contain dots (`HEAD~3..HEAD`, `file.tar.gz`) and dotfiles (`.env`) still pass. This can break a template that legitimately accepts `..`-bearing or absolute values — exempt the parameter with `ResourceSecurity(exempt_params={...})`, disable per-component with `security=None`, or set a server-wide default with `FastMCP(resource_security=...)`. See [Resources → Path Security](/servers/resources#path-security). + +*Verify:* `fastmcp_slim/fastmcp/resources/security.py` (`ResourceSecurity`), the screening block in `FastMCP.read_resource` (`fastmcp_slim/fastmcp/server/server.py`), and `tests/resources/test_resource_security.py`. + ## Removed in 4.0 Deprecations that warned in 3.x are removed in 4.0. Each entry below is a hard removal — the old surface raises `TypeError` / `AttributeError` rather than warning, unless noted otherwise. diff --git a/docs/servers/resources.mdx b/docs/servers/resources.mdx index c756c5ff9..37d8de62c 100644 --- a/docs/servers/resources.mdx +++ b/docs/servers/resources.mdx @@ -522,11 +522,85 @@ Wildcard parameters are useful when: Note that like regular parameters, each wildcard parameter must still be a named parameter in your function signature, and all required function parameters must appear in the URI template. -#### Filesystem Path Safety +#### Path Security -Template parameters are decoded before your function receives them. A standard `{filename}` parameter matches one URI segment before decoding, so a request like `files://a%2Fb` passes `filename="a/b"` to the handler. Treat template values as untrusted decoded URI data whenever they determine filesystem paths. +Template parameters are extracted from the request URI and decoded before your function receives them, so a path-traversal payload like `../` or an absolute path can reach a handler that builds filesystem paths or upstream URLs. FastMCP screens every templated resource's parameter values **before the handler runs**, and this screening is **on by default**. -Validate the final resolved path against an allowed root before reading: +By default, a parameter value is rejected if its `..` path segments would escape the value's own starting depth, if it looks like an absolute path, or if it contains a null byte. A rejected read surfaces a clean "resource not found" error to the client and logs the reason at debug level, so the failing parameter and policy are never revealed on the wire. + +The traversal check is component-based and tracks net depth: `..` only counts against you when it climbs above where the value starts. `../secret`, a bare `..`, and `a/../../b` are rejected; `foo/../bar` is allowed because it never leaves the starting directory, and values that merely *contain* dots — `HEAD~3..HEAD`, `v1..v2`, `file.tar.gz`, dotfiles like `.env` — all pass. Screening runs on the decoded value, so `..%2F` is caught the same as a literal `../`. This bounds relative escapes; anchoring the *final* path inside a root directory is still your handler's job (for example with `safe_join`), since only the handler knows what the value is joined to. + +```python +from pathlib import Path + +from fastmcp import FastMCP + +mcp = FastMCP(name="DocsServer") + +DOCS_ROOT = Path("/srv/docs") + + +@mcp.resource("docs://{path*}") +def read_doc(path: str) -> str: + # A request for docs://../secret is rejected before this runs. + return (DOCS_ROOT / path).read_text(encoding="utf-8") +``` + +##### Exempting parameters + +Some parameters legitimately carry values that look like traversal — a git ref, a version range, an opaque token. Exempt them by name with `ResourceSecurity`: + +```python +from fastmcp import FastMCP +from fastmcp.resources import ResourceSecurity + +mcp = FastMCP(name="DocsServer") + + +@mcp.resource( + "git://diff/{ref}", + security=ResourceSecurity(exempt_params={"ref"}), +) +def git_diff(ref: str) -> str: + # ref="HEAD~3..HEAD" is allowed + ... +``` + +##### Disabling screening + +Pass `security=None` to turn screening off for a single component: + +```python +from fastmcp import FastMCP + +mcp = FastMCP(name="DocsServer") + + +@mcp.resource("raw://{value}", security=None) +def raw(value: str) -> str: ... +``` + +Or set a server-wide default with `resource_security`, which applies to every templated resource that does not set its own `security`: + +```python +from fastmcp import FastMCP +from fastmcp.resources import ResourceSecurity + +# Relax one check across the whole server: +relaxed = FastMCP( + name="DocsServer", + resource_security=ResourceSecurity(reject_absolute_paths=False), +) + +# Or disable screening entirely across the server: +unscreened = FastMCP(name="DocsServer", resource_security=None) +``` + +A per-component `security` always overrides the server default. + + +Screening rejects the obvious injection shapes, but it does not know your filesystem root. When a parameter determines a real path, still resolve it against an allowed root and confirm containment before reading — screening and containment are complementary layers. + ```python from pathlib import Path @@ -548,8 +622,6 @@ def read_doc(filename: str) -> str: return requested_path.read_text(encoding="utf-8") ``` -Use wildcard parameters (`{path*}`) for resources whose URI shape intentionally includes slashes, and apply the same containment check before accessing the filesystem. - #### Query Parameters diff --git a/fastmcp_slim/fastmcp/exceptions.py b/fastmcp_slim/fastmcp/exceptions.py index fb6571f05..b74b50998 100644 --- a/fastmcp_slim/fastmcp/exceptions.py +++ b/fastmcp_slim/fastmcp/exceptions.py @@ -80,6 +80,17 @@ class DisabledError(Exception): """Object is disabled.""" +class ResourceSecurityError(NotFoundError): + """A templated resource parameter failed path-security screening. + + Subclasses ``NotFoundError`` so the read handler surfaces a + non-leaky ``INVALID_PARAMS`` (-32602) "resource not found" error to + the client — a traversal attempt is indistinguishable from a request + for a resource that does not exist, and never reveals which parameter + or policy tripped. + """ + + class AuthorizationError(FastMCPError): """Error when authorization check fails.""" diff --git a/fastmcp_slim/fastmcp/resources/__init__.py b/fastmcp_slim/fastmcp/resources/__init__.py index cbcfff785..cbe819c95 100644 --- a/fastmcp_slim/fastmcp/resources/__init__.py +++ b/fastmcp_slim/fastmcp/resources/__init__.py @@ -2,6 +2,7 @@ import sys from .function_resource import FunctionResource, resource from .base import Resource, ResourceContent, ResourceResult +from .security import ResourceSecurity from .template import ResourceTemplate from .types import ( BinaryResource, @@ -20,6 +21,7 @@ __all__ = [ "Resource", "ResourceContent", "ResourceResult", + "ResourceSecurity", "ResourceTemplate", "TextResource", "resource", diff --git a/fastmcp_slim/fastmcp/resources/function_resource.py b/fastmcp_slim/fastmcp/resources/function_resource.py index 71817f835..aa71508d9 100644 --- a/fastmcp_slim/fastmcp/resources/function_resource.py +++ b/fastmcp_slim/fastmcp/resources/function_resource.py @@ -22,6 +22,11 @@ from pydantic import AnyUrl from pydantic.json_schema import SkipJsonSchema from fastmcp.resources.base import Resource, ResourceResult +from fastmcp.resources.security import ( + INHERIT_SECURITY, + InheritSecurity, + ResourceSecurity, +) from fastmcp.utilities.async_utils import ( call_sync_fn_in_threadpool, is_coroutine_function, @@ -64,6 +69,7 @@ class ResourceMeta: task: bool | TaskConfig | None = None auth: AuthCheck | list[AuthCheck] | None = None enabled: bool = True + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY class FunctionResource(Resource): @@ -255,6 +261,7 @@ def resource( meta: dict[str, Any] | None = None, task: bool | TaskConfig | None = None, auth: AuthCheck | list[AuthCheck] | None = None, + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY, ) -> Callable[[F], F]: """Standalone decorator to mark a function as an MCP resource. @@ -284,6 +291,7 @@ def resource( meta=meta, task=task, auth=auth, + security=security, ) target = fn.__func__ if isinstance(fn, staticmethod | MethodType) else fn cast(Any, target).__fastmcp__ = metadata diff --git a/fastmcp_slim/fastmcp/resources/security.py b/fastmcp_slim/fastmcp/resources/security.py new file mode 100644 index 000000000..bc5ba3174 --- /dev/null +++ b/fastmcp_slim/fastmcp/resources/security.py @@ -0,0 +1,162 @@ +"""Path-safety policy for templated resource parameters. + +Templated resources (`@mcp.resource("file:///{path}")`-style) extract +parameter values straight out of the request URI and hand them to the +resource function. When those values flow into filesystem or URI +construction, a malicious client can smuggle path-traversal payloads +(`../`, absolute paths, null bytes) through the template. + +`ResourceSecurity` screens extracted parameter values *before* the +resource handler runs. It is applied by default to every templated +read, mirroring the posture of the underlying MCP SDK's +`ResourceSecurity` (traversal, absolute paths, and null bytes rejected). + +The screening reuses the SDK's component-based traversal check, so a +value that merely *contains* dots (e.g. `HEAD~3..HEAD`, `v1..v2`, +`file.tar.gz`) is not rejected — only an actual `..` path segment is. +""" + +from __future__ import annotations + +from collections.abc import Callable, Mapping, Set +from dataclasses import dataclass, field +from functools import cache +from typing import Any + +from pydantic import GetCoreSchemaHandler +from pydantic_core import core_schema + +__all__ = ["ResourceSecurity"] + + +@cache +def _path_checks() -> tuple[Callable[[str], bool], Callable[[str], bool]]: + """Lazily load the SDK's path-safety helpers. + + The screening logic lives in the `mcp` SDK, which is an optional + dependency of `fastmcp-slim`. Importing it at module top would make + `from fastmcp.resources import Resource` require the SDK, so the + import is deferred to the point of first use (and cached). + """ + from mcp.shared.path_security import ( + contains_path_traversal, + is_absolute_path, + ) + + return contains_path_traversal, is_absolute_path + + +class InheritSecurity: + """Sentinel type: inherit the server-wide resource-security default. + + Distinguishes "no per-component policy was set" (inherit whatever the + server configured) from an explicit ``None`` (screening disabled for + this component). + """ + + __slots__ = () + + def __repr__(self) -> str: # pragma: no cover - debug aid + return "INHERIT_SECURITY" + + @classmethod + def __get_pydantic_core_schema__( + cls, source_type: Any, handler: GetCoreSchemaHandler + ) -> core_schema.CoreSchema: + # Accept the singleton sentinel as-is; it is an internal, excluded + # field value, so no serialization support is needed. + return core_schema.is_instance_schema(cls) + + +INHERIT_SECURITY = InheritSecurity() +"""Sentinel instance signalling a template should inherit the server default.""" + + +@dataclass(frozen=True) +class ResourceSecurity: + """Security policy applied to extracted resource template parameters. + + These checks run after a URI has matched a template and its + parameter values have been extracted and percent-decoded. They catch + path-traversal and absolute-path injection regardless of how the + value was encoded in the URI (literal, `%2F`, `%5C`, `%2E%2E`). + + All checks default on. Screen a value like `HEAD~3..HEAD` (dots + inside a single segment) passes — only a standalone `..` segment is + treated as traversal. + + Example: + Opt a parameter out of screening (e.g. a git ref that may + legitimately contain `..`): + + ```python + from fastmcp.resources import ResourceSecurity + + @mcp.resource( + "git://diff/{ref}", + security=ResourceSecurity(exempt_params={"ref"}), + ) + def git_diff(ref: str) -> str: ... + ``` + """ + + reject_path_traversal: bool = True + """Reject values containing `..` as a path component.""" + + reject_absolute_paths: bool = True + """Reject values that look like absolute filesystem paths.""" + + reject_null_bytes: bool = True + """Reject values containing NUL (`\\x00`). Null bytes defeat string + comparisons (`"..\\x00" != ".."`) and can cause truncation in C + extensions or subprocess calls.""" + + exempt_params: Set[str] = field(default_factory=frozenset) + """Parameter names to skip all checks for. Hyphenated URI-template + spellings are accepted: `{git-ref}` is extracted as `git_ref`, and an + exemption written either way matches it.""" + + def _exempt(self, name: str) -> bool: + """True if `name` is exempted under either its extracted or its + URI-template spelling (hyphens normalize to underscores on + extraction, so `exempt_params={"git-ref"}` must match `git_ref`).""" + if name in self.exempt_params: + return True + return any(exempt.replace("-", "_") == name for exempt in self.exempt_params) + + def validate(self, params: Mapping[str, object]) -> str | None: + """Check all parameter values against the configured policy. + + String values (and lists of strings, from wildcard `{path*}` + parameters that span multiple segments) are screened; non-string + values are ignored, since traversal is a string-path concern. + + Args: + params: Extracted template parameters. + + Returns: + The name of the first parameter that fails, or `None` if all + values pass. + """ + contains_path_traversal, is_absolute_path = _path_checks() + for name, value in params.items(): + if self._exempt(name): + continue + if isinstance(value, str): + candidates = [value] + elif isinstance(value, (list, tuple)): + candidates = [v for v in value if isinstance(v, str)] + else: + continue + for candidate in candidates: + if self.reject_null_bytes and "\0" in candidate: + return name + if self.reject_path_traversal and contains_path_traversal(candidate): + return name + if self.reject_absolute_paths and is_absolute_path(candidate): + return name + return None + + +DEFAULT_RESOURCE_SECURITY = ResourceSecurity() +"""Secure-by-default policy: traversal, absolute paths, and null bytes rejected.""" diff --git a/fastmcp_slim/fastmcp/resources/template.py b/fastmcp_slim/fastmcp/resources/template.py index 7cd7b244e..00cceaeec 100644 --- a/fastmcp_slim/fastmcp/resources/template.py +++ b/fastmcp_slim/fastmcp/resources/template.py @@ -24,6 +24,11 @@ from pydantic import ( ) from fastmcp.resources.base import Resource, ResourceResult +from fastmcp.resources.security import ( + INHERIT_SECURITY, + InheritSecurity, + ResourceSecurity, +) from fastmcp.utilities.authorization import AuthCheck from fastmcp.utilities.components import FastMCPComponent from fastmcp.utilities.json_schema import compress_schema @@ -186,6 +191,29 @@ class ResourceTemplate(FastMCPComponent): description="Authorization checks for this resource template", exclude=True, ) + security: SkipJsonSchema[ResourceSecurity | None | InheritSecurity] = Field( + default=INHERIT_SECURITY, + description=( + "Path-safety policy for extracted parameters. INHERIT_SECURITY " + "(default) inherits the server-wide default; None disables " + "screening; a ResourceSecurity instance applies that explicit " + "policy." + ), + exclude=True, + ) + + def resolve_security( + self, server_default: ResourceSecurity | None + ) -> ResourceSecurity | None: + """Resolve the effective security policy for this template. + + A per-component ``security`` overrides the server default. + ``INHERIT_SECURITY`` (the field default) inherits ``server_default``; + an explicit ``None`` disables screening for this template. + """ + if isinstance(self.security, InheritSecurity): + return server_default + return self.security def __repr__(self) -> str: return f"{self.__class__.__name__}(uri_template={self.uri_template!r}, name={self.name!r}, description={self.description!r}, tags={self.tags})" @@ -205,6 +233,7 @@ class ResourceTemplate(FastMCPComponent): meta: dict[str, Any] | None = None, task: bool | TaskConfig | None = None, auth: AuthCheck | list[AuthCheck] | None = None, + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY, ) -> FunctionResourceTemplate: return FunctionResourceTemplate.from_function( fn=fn, @@ -220,6 +249,7 @@ class ResourceTemplate(FastMCPComponent): meta=meta, task=task, auth=auth, + security=security, ) @field_validator("mime_type", mode="before") @@ -544,6 +574,7 @@ class FunctionResourceTemplate(ResourceTemplate): meta: dict[str, Any] | None = None, task: bool | TaskConfig | None = None, auth: AuthCheck | list[AuthCheck] | None = None, + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY, ) -> FunctionResourceTemplate: """Create a template from a function.""" @@ -683,4 +714,5 @@ class FunctionResourceTemplate(ResourceTemplate): meta=meta, task_config=task_config, auth=auth, + security=security, ) diff --git a/fastmcp_slim/fastmcp/server/providers/fastmcp_provider.py b/fastmcp_slim/fastmcp/server/providers/fastmcp_provider.py index 14dbbbae0..27787d2ef 100644 --- a/fastmcp_slim/fastmcp/server/providers/fastmcp_provider.py +++ b/fastmcp_slim/fastmcp/server/providers/fastmcp_provider.py @@ -363,6 +363,7 @@ class FastMCPProviderResourceTemplate(ResourceTemplate): meta=template.get_meta(), title=template.title, icons=template.icons, + security=template.security, ) async def create_resource(self, uri: str, params: dict[str, Any]) -> Resource: diff --git a/fastmcp_slim/fastmcp/server/providers/local_provider/decorators/resources.py b/fastmcp_slim/fastmcp/server/providers/local_provider/decorators/resources.py index c9cb475ac..75d23a967 100644 --- a/fastmcp_slim/fastmcp/server/providers/local_provider/decorators/resources.py +++ b/fastmcp_slim/fastmcp/server/providers/local_provider/decorators/resources.py @@ -14,6 +14,11 @@ import mcp_types from mcp_types import Annotations from fastmcp.resources.base import Resource +from fastmcp.resources.security import ( + INHERIT_SECURITY, + InheritSecurity, + ResourceSecurity, +) from fastmcp.resources.template import ResourceTemplate from fastmcp.server.auth.authorization import AuthCheck from fastmcp.server.tasks.config import TaskConfig @@ -70,6 +75,7 @@ class ResourceDecoratorMixin: meta=meta.meta, task=resolved_task, auth=meta.auth, + security=meta.security, ) else: resource = Resource.from_function( @@ -119,6 +125,7 @@ class ResourceDecoratorMixin: meta: dict[str, Any] | None = None, task: bool | TaskConfig | None = None, auth: AuthCheck | list[AuthCheck] | None = None, + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY, ) -> Callable[[F], F]: """Decorator to register a function as a resource. @@ -202,6 +209,7 @@ class ResourceDecoratorMixin: task=task, auth=auth, enabled=enabled, + security=security, ) target = fn.__func__ if hasattr(fn, "__func__") else fn target.__fastmcp__ = metadata # type: ignore[attr-defined] # ty:ignore[unresolved-attribute] diff --git a/fastmcp_slim/fastmcp/server/server.py b/fastmcp_slim/fastmcp/server/server.py index aea64f111..e940ac2a2 100644 --- a/fastmcp_slim/fastmcp/server/server.py +++ b/fastmcp_slim/fastmcp/server/server.py @@ -49,6 +49,7 @@ from fastmcp.exceptions import ( NotFoundError, PromptError, ResourceError, + ResourceSecurityError, ToolError, ValidationError, ) @@ -57,6 +58,12 @@ from fastmcp.prompts import Prompt from fastmcp.prompts.base import PromptResult from fastmcp.prompts.function_prompt import FunctionPrompt from fastmcp.resources.base import Resource, ResourceResult +from fastmcp.resources.security import ( + DEFAULT_RESOURCE_SECURITY, + INHERIT_SECURITY, + InheritSecurity, + ResourceSecurity, +) from fastmcp.resources.template import ResourceTemplate from fastmcp.server.auth import AuthCheck, AuthContext, AuthProvider, run_auth_checks from fastmcp.server.caching import build_cache_hints @@ -330,6 +337,7 @@ class FastMCP( dereference_schemas: bool = True, strict_input_validation: bool | None = None, list_page_size: int | None = None, + resource_security: ResourceSecurity | None = DEFAULT_RESOURCE_SECURITY, cache_ttl: int | None = None, cache_scope: Literal["public", "private"] | None = None, tasks: bool | None = None, @@ -390,6 +398,13 @@ class FastMCP( raise ValueError("list_page_size must be a positive integer") self._list_page_size: int | None = list_page_size + # Server-wide default path-security policy for templated resources. + # Applied before the handler runs to every templated read whose + # component does not override it. DEFAULT_RESOURCE_SECURITY screens + # traversal, absolute paths, and null bytes; None disables screening + # server-wide. + self._resource_security: ResourceSecurity | None = resource_security + # Server-level SEP-2549 cache hints, applied uniformly to every # SDK-cacheable result by the low-level server's runner (raises on # invalid ttl/scope). @@ -1490,6 +1505,24 @@ class FastMCP( span.set_attributes(template.get_span_attributes()) params = template.matches(uri) assert params is not None + + # Path-security screening: reject traversal / absolute-path / + # null-byte payloads in extracted parameter values BEFORE the + # handler runs. This is the single chokepoint for every + # templated read (local decorator and provider-sourced), so + # enforcement lives here rather than in any decorator. + security = template.resolve_security(self._resource_security) + if security is not None: + failed = security.validate(params) + if failed is not None: + logger.debug( + "Rejected resource %r: parameter %r failed " + "path-security screening", + uri, + failed, + ) + raise ResourceSecurityError(f"Unknown resource: {uri!r}") + if task_meta is not None and task_meta.fn_key is None: task_meta = replace(task_meta, fn_key=template.key) try: @@ -1824,6 +1857,7 @@ class FastMCP( app: AppConfig | dict[str, Any] | bool | None = None, task: bool | TaskConfig | None = None, auth: AuthCheck | list[AuthCheck] | None = None, + security: ResourceSecurity | None | InheritSecurity = INHERIT_SECURITY, ) -> Callable[[F], F]: """Decorator to register a function as a resource. @@ -1923,6 +1957,7 @@ class FastMCP( meta=meta, task=task if task is not None else self._support_tasks_by_default, auth=auth, + security=security, ) return inner_decorator diff --git a/tests/resources/test_resource_security.py b/tests/resources/test_resource_security.py new file mode 100644 index 000000000..f530d63c9 --- /dev/null +++ b/tests/resources/test_resource_security.py @@ -0,0 +1,473 @@ +"""Tests for path-security screening of templated resource parameters. + +Templated resources extract parameter values from request URIs and hand +them to the handler. `ResourceSecurity` screens those values (traversal, +absolute paths, null bytes) before the handler runs, defaults-on, at the +server's read chokepoint. + +The screening is applied to the *raw* URI string reaching the server +(`FastMCP.read_resource(str)`), which is the path the JSON-RPC handler and +internal callers use. Over the in-memory `Client`, URIs are wrapped in +`AnyUrl`, which independently normalises many `..` payloads away before +they reach the server — a separate layer of defense. +""" + +import subprocess +import sys +import textwrap + +import pytest + +from fastmcp import Client, FastMCP +from fastmcp.exceptions import ResourceSecurityError +from fastmcp.resources.security import ( + DEFAULT_RESOURCE_SECURITY, + INHERIT_SECURITY, + ResourceSecurity, +) +from fastmcp.resources.template import ResourceTemplate + +# --------------------------------------------------------------------------- +# ResourceSecurity model (unit) +# --------------------------------------------------------------------------- + + +class TestResourceSecurityModel: + @pytest.mark.parametrize( + "value", + [ + "../etc/passwd", + "..", + "a/../../b", + "nested/../../outside", + ], + ) + def test_rejects_traversal(self, value: str): + assert ResourceSecurity().validate({"path": value}) == "path" + + @pytest.mark.parametrize( + "value", + [ + "/etc/passwd", + "/absolute/injection", + "C:\\Windows", + "C:relative", + "\\\\server\\share", + ], + ) + def test_rejects_absolute(self, value: str): + assert ResourceSecurity().validate({"path": value}) == "path" + + @pytest.mark.parametrize( + "value", + [ + "a\x00b", + "good\x00/../../../etc/passwd", + ], + ) + def test_rejects_null_bytes(self, value: str): + assert ResourceSecurity().validate({"path": value}) == "path" + + @pytest.mark.parametrize( + "value", + [ + "HEAD~3..HEAD", + "v1..v2", + "a.b.c", + "file.tar.gz", + "1.0..2.0", + ".env", + ".git/config", + "...", + "docs/readme.txt", + "foo/../bar", # net depth stays >= 0 -> not an escape (SDK semantics) + "café/naïve", + ], + ) + def test_allows_safe_values(self, value: str): + """Dots inside a segment, benign relative paths, and dotfiles pass. + + This mirrors the SDK's component-based `contains_path_traversal`: + only a standalone `..` segment counts as traversal. A leading-dot + single segment (`.env`) is an ordinary name, not traversal, and + passes default screening — filesystem exposure of such names is the + handler's concern (e.g. via `safe_join` to a root), not this check. + """ + assert ResourceSecurity().validate({"path": value}) is None + + def test_exempt_params_skipped(self): + security = ResourceSecurity(exempt_params={"ref"}) + assert security.validate({"ref": "../anything"}) is None + # A non-exempt param is still screened. + assert security.validate({"path": "../x", "ref": "../y"}) == "path" + + def test_hyphenated_exemption_matches_normalized_param(self): + """`{git-ref}` extracts as `git_ref`; an exemption written with the + URI-template (hyphen) spelling must still match it.""" + security = ResourceSecurity(exempt_params={"git-ref"}) + assert security.validate({"git_ref": "HEAD~3../x"}) is None + assert security.validate({"git_ref": "../x"}) is None + # The underscore spelling keeps working too. + assert ( + ResourceSecurity(exempt_params={"git_ref"}).validate({"git_ref": "../x"}) + is None + ) + # An unrelated hyphenated exemption does not leak onto other params. + assert security.validate({"path": "../x"}) == "path" + + def test_wildcard_segments_screened_element_wise(self): + """List values (from wildcard {path*}) are screened per element.""" + assert ResourceSecurity().validate({"path": ["a", "..", "b"]}) == "path" + assert ResourceSecurity().validate({"path": ["a", "b", "c"]}) is None + + def test_non_string_values_ignored(self): + assert ResourceSecurity().validate({"n": 5, "flag": True}) is None + + def test_individual_checks_toggleable(self): + no_traversal = ResourceSecurity(reject_path_traversal=False) + assert no_traversal.validate({"path": "../x"}) is None + # but absolute still rejected + assert no_traversal.validate({"path": "/etc/passwd"}) == "path" + + def test_returns_first_failing_param_name(self): + # dict order preserved; first failing name returned + result = ResourceSecurity().validate({"safe": "ok", "bad": ".."}) + assert result == "bad" + + +# --------------------------------------------------------------------------- +# Bare-slim import: the module must not eagerly require the optional SDK +# --------------------------------------------------------------------------- + + +class TestBareSlimImport: + """`fastmcp-slim` installs the `mcp` SDK only under the `[mcp]` extra. + + The path-safety helpers live in `mcp.shared.path_security`, so importing + them at module top would make `from fastmcp.resources import Resource` + require the SDK — regressing a previously dependency-free import path. + The import must be deferred to the point of actual screening. + """ + + def test_resources_import_without_sdk(self): + code = textwrap.dedent( + """ + import sys, builtins + _real_import = builtins.__import__ + + def blocked_import(name, *args, **kwargs): + if name == "mcp" or name.startswith("mcp."): + raise ModuleNotFoundError(f"No module named '{name}'") + return _real_import(name, *args, **kwargs) + + builtins.__import__ = blocked_import + for mod in list(sys.modules): + if mod == "mcp" or mod.startswith("mcp."): + del sys.modules[mod] + + from fastmcp.resources import Resource, ResourceSecurity # noqa: F401 + import fastmcp.resources # noqa: F401 + print("OK") + """ + ) + result = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stderr + assert "OK" in result.stdout + + def test_screening_still_works_with_sdk(self): + # With the SDK present (the normal test environment), the deferred + # import resolves and screening behaves exactly as before. + assert ResourceSecurity().validate({"path": "../etc/passwd"}) == "path" + assert ResourceSecurity().validate({"path": "/etc/passwd"}) == "path" + assert ResourceSecurity().validate({"path": "safe/file.txt"}) is None + + +# --------------------------------------------------------------------------- +# Enforcement at the server chokepoint (raw-string reads) +# --------------------------------------------------------------------------- + + +class TestChokepointEnforcement: + @pytest.fixture + def server(self) -> FastMCP: + mcp = FastMCP("test") + + @mcp.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"content:{path}" + + return mcp + + @pytest.mark.parametrize( + "uri", + [ + "file:///../etc/passwd", + "file:///a/../../b", + "file:////etc/passwd", # -> path param '/etc/passwd' (absolute) + "file:///a\x00b", + ], + ) + async def test_traversal_rejected_by_default(self, server: FastMCP, uri: str): + with pytest.raises(ResourceSecurityError): + await server.read_resource(uri) + + @pytest.mark.parametrize( + "uri", + [ + "file:///docs/readme.txt", + "file:///HEAD~3..HEAD", + "file:///v1..v2", + "file:///file.tar.gz", + "file:///.env", + ], + ) + async def test_safe_uris_pass_by_default(self, server: FastMCP, uri: str): + result = await server.read_resource(uri) + content = result.contents[0].content + assert isinstance(content, str) + assert content.startswith("content:") + + +class TestServerDefaultConfiguration: + async def test_server_default_disabled(self): + mcp = FastMCP("test", resource_security=None) + + @mcp.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"content:{path}" + + # Traversal passes when server-wide screening is disabled. + result = await mcp.read_resource("file:///../etc/passwd") + assert result.contents[0].content == "content:../etc/passwd" + + async def test_server_default_custom_exemption(self): + mcp = FastMCP( + "test", + resource_security=ResourceSecurity(exempt_params={"path"}), + ) + + @mcp.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"content:{path}" + + result = await mcp.read_resource("file:///../etc/passwd") + assert result.contents[0].content == "content:../etc/passwd" + + async def test_server_default_applies_to_all_templates(self): + """A single server default screens every templated resource.""" + mcp = FastMCP("test") + + @mcp.resource("a://{path*}") + def read_a(path: str) -> str: + return path + + @mcp.resource("b://{path*}") + def read_b(path: str) -> str: + return path + + for scheme in ("a", "b"): + with pytest.raises(ResourceSecurityError): + await mcp.read_resource(f"{scheme}://../escape") + + +class TestPerComponentOverride: + async def test_component_disable_overrides_server_default(self): + mcp = FastMCP("test") # default: screening on + + @mcp.resource("git://diff/{ref}", security=None) + def git_diff(ref: str) -> str: + return f"diff:{ref}" + + # '..' in the ref is allowed because this component disabled screening. + result = await mcp.read_resource("git://diff/HEAD~3..HEAD") + assert result.contents[0].content == "diff:HEAD~3..HEAD" + + async def test_component_exemption_overrides_server_default(self): + mcp = FastMCP("test") + + @mcp.resource( + "git://diff/{ref}", + security=ResourceSecurity(exempt_params={"ref"}), + ) + def git_diff(ref: str) -> str: + return f"diff:{ref}" + + result = await mcp.read_resource("git://diff/..") + assert result.contents[0].content == "diff:.." + + async def test_component_enables_over_disabled_server_default(self): + """A per-component policy overrides a server default of None.""" + mcp = FastMCP("test", resource_security=None) + + @mcp.resource("file:///{path*}", security=ResourceSecurity()) + def read_file(path: str) -> str: + return path + + with pytest.raises(ResourceSecurityError): + await mcp.read_resource("file:///../etc/passwd") + + def test_inherit_default_on_template(self): + def read_file(path: str) -> str: + return path + + template = ResourceTemplate.from_function(read_file, "file:///{path*}") + assert template.security is INHERIT_SECURITY + assert template.resolve_security(DEFAULT_RESOURCE_SECURITY) is ( + DEFAULT_RESOURCE_SECURITY + ) + + def test_explicit_none_disables(self): + def read_file(path: str) -> str: + return path + + template = ResourceTemplate.from_function( + read_file, "file:///{path*}", security=None + ) + assert template.resolve_security(DEFAULT_RESOURCE_SECURITY) is None + + +# --------------------------------------------------------------------------- +# End-to-end through the in-memory Client +# --------------------------------------------------------------------------- + + +class TestEndToEndClient: + async def test_traversal_read_gets_clean_not_found(self): + """A traversal attempt over the wire surfaces a non-leaky error. + + `resource://..` survives `AnyUrl` normalisation (the `..` sits in + the authority, not the path), so it reaches the server chokepoint + and is rejected. The client sees a generic "resource not found" + error that never reveals the screening reason. + """ + mcp = FastMCP("test") + + @mcp.resource("resource://{path*}") + def read(path: str) -> str: + return path + + async with Client(mcp) as client: + with pytest.raises(Exception) as exc_info: + await client.read_resource("resource://..") + + message = str(exc_info.value) + assert "not found" in message.lower() + # Non-leaky: the error must not name the failing parameter or policy. + assert "path" not in message.lower() + assert "security" not in message.lower() + + async def test_legit_read_succeeds(self): + mcp = FastMCP("test") + + @mcp.resource("file:///{path*}") + def read(path: str) -> str: + return f"content:{path}" + + async with Client(mcp) as client: + result = await client.read_resource("file:///docs/readme.txt") + + assert result[0].text == "content:docs/readme.txt" + + +# --------------------------------------------------------------------------- +# Provider-sourced templates (mounted servers) +# --------------------------------------------------------------------------- + + +class TestProviderSourcedTemplates: + """Templates surfaced by a provider route through the same chokepoint. + + Enforcement lives at the server read chokepoint, not in the decorator, + so a mounted server's templates inherit the *parent* server's default + policy and are screened before the request is delegated. + """ + + async def test_mounted_template_screened_by_parent_default(self): + child = FastMCP("child") + + @child.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"child:{path}" + + parent = FastMCP("parent") + parent.mount(child) + + with pytest.raises(ResourceSecurityError): + await parent.read_resource("file:///../escape") + + async def test_mounted_template_safe_read_succeeds(self): + child = FastMCP("child") + + @child.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"child:{path}" + + parent = FastMCP("parent") + parent.mount(child) + + result = await parent.read_resource("file:///docs/ok.txt") + assert result.contents[0].content == "child:docs/ok.txt" + + async def test_parent_default_screens_even_if_child_disabled(self): + """The parent's policy applies even when the child disabled its own. + + Screening runs at each server's chokepoint. A traversal is caught by + the parent before delegation regardless of the child's configuration. + """ + child = FastMCP("child", resource_security=None) + + @child.resource("file:///{path*}") + def read_file(path: str) -> str: + return f"child:{path}" + + parent = FastMCP("parent") # default screening on + parent.mount(child) + + with pytest.raises(ResourceSecurityError): + await parent.read_resource("file:///../escape") + + async def test_mounted_template_exempt_param_preserved(self): + """A child template's explicit per-param exemption survives the mount. + + The child opts one parameter out of screening. That policy must be + carried through the provider-wrapped template so the parent's read + chokepoint honours it instead of falling back to the parent default. + """ + child = FastMCP("child") + + @child.resource( + "git://diff/{ref}/{path*}", + security=ResourceSecurity(exempt_params={"ref"}), + ) + def git_diff(ref: str, path: str) -> str: + return f"child:{ref}:{path}" + + parent = FastMCP("parent") # default screening on + parent.mount(child) + + # `..` in the exempt `ref` param is allowed through the mount. + result = await parent.read_resource("git://diff/../safe") + assert result.contents[0].content == "child:..:safe" + + # A traversal on the NON-exempt `path` param is still rejected. + with pytest.raises(ResourceSecurityError): + await parent.read_resource("git://diff/main/../escape") + + async def test_mounted_template_disabled_security_preserved(self): + """A child template that explicitly disables screening keeps that + opt-out through the mount rather than inheriting the parent default.""" + child = FastMCP("child") + + @child.resource("git://raw/{path*}", security=None) + def read_raw(path: str) -> str: + return f"child:{path}" + + parent = FastMCP("parent") # default screening on + parent.mount(child) + + result = await parent.read_resource("git://raw/../escape") + assert result.contents[0].content == "child:../escape" diff --git a/tests/server/providers/openapi/test_openapi_features.py b/tests/server/providers/openapi/test_openapi_features.py index 95c7f1a82..2c94e4386 100644 --- a/tests/server/providers/openapi/test_openapi_features.py +++ b/tests/server/providers/openapi/test_openapi_features.py @@ -762,14 +762,15 @@ class TestResourceTemplateRequestBuilding: mcp.add_provider(provider) async with Client(mcp) as mcp_client: - await mcp_client.read_resource( - "resource://get_user/..%2F..%2Fadmin%2Fsecret" - ) + # Reserved characters (encoded slash + space) must be + # re-encoded when building the outbound URL. A traversal + # payload (`..%2F...`) would be rejected by the default + # resource-security screening, so use a benign value that + # still exercises reserved-character encoding. + await mcp_client.read_resource("resource://get_user/a%2Fb%20c") assert seen_urls == [ - httpx.URL( - "https://api.example.com/api/v1/users/%2E%2E%2F%2E%2E%2Fadmin%2Fsecret" - ) + httpx.URL("https://api.example.com/api/v1/users/a%2Fb%20c") ] async def test_resource_template_ignores_unmatched_query_string( From a3ecd1edb1e27ff53b5b5b11cc57dc2b6235cae3 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:43:05 -0400 Subject: [PATCH 12/22] Clarify PR-reopen flow and fix label-race that broke auto-reopen (#4518) Co-authored-by: Claude Opus 4.8 Co-authored-by: Claude --- .github/scripts/triage-label.sh | 73 +++++++++++++++++++++++ .github/workflows/marvin-label-triage.yml | 19 +++--- .github/workflows/require-issue-link.yml | 23 +++---- CONTRIBUTING.md | 9 ++- 4 files changed, 104 insertions(+), 20 deletions(-) create mode 100755 .github/scripts/triage-label.sh diff --git a/.github/scripts/triage-label.sh b/.github/scripts/triage-label.sh new file mode 100755 index 000000000..88081766f --- /dev/null +++ b/.github/scripts/triage-label.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# Locked-down label helper for the Marvin triage workflow. +# +# Marvin runs on untrusted issue/PR bodies from non-write users, so it must +# NOT be handed raw `gh api` (that would expose every endpoint the app token +# can reach). This helper is the ONLY GitHub write it is allowed to perform: +# it adds or removes repository labels on the one issue/PR being triaged. +# +# The target repo and number come from the environment set by the workflow — +# never from the model — and the operation is fixed to the additive labels +# endpoint (POST/DELETE /repos/{repo}/issues/{n}/labels), which works for both +# issues and PRs and cannot clobber labels applied by other workflows. +set -euo pipefail + +repo="${TRIAGE_REPO:?TRIAGE_REPO not set}" +number="${TRIAGE_NUMBER:?TRIAGE_NUMBER not set}" + +if [[ ! "$number" =~ ^[0-9]+$ ]]; then + echo "TRIAGE_NUMBER must be numeric, got: $number" >&2 + exit 1 +fi + +op="${1:-}" +shift || true +case "$op" in + add) method=POST ;; + remove) method=DELETE ;; + *) + echo "usage: triage-label.sh