From e8138f9c9ecdc0007e6e1c7f31bbd0b34894df9d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:17:16 -0400 Subject: [PATCH 01/14] fix(fetch): reject an invalid prompt URL with -32602 The fetch prompt passed its url argument straight to httpx, so a malformed URL raised httpx.InvalidURL, which is not an HTTPError: it escaped the prompt's error handling and the SDK answered with JSON-RPC code 0 ("Error rendering prompt fetch"). Validate the url with the same AnyUrl type the tool's parameter uses and answer a bad one with INVALID_PARAMS and the validation reason. A valid URL is fetched normalized, as the tool does. Closes #4914 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/README.md | 2 +- src/fetch/src/mcp_server_fetch/server.py | 17 +++++ src/fetch/tests/test_prompt_url.py | 91 ++++++++++++++++++++++++ src/fetch/tests/test_protocol.py | 21 ------ 4 files changed, 109 insertions(+), 22 deletions(-) create mode 100644 src/fetch/tests/test_prompt_url.py diff --git a/src/fetch/README.md b/src/fetch/README.md index 09b4e8b5d8..077a2abf3e 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -34,7 +34,7 @@ The fetch tool will truncate the response, but by using the `start_index` argume ### Errors - A failed fetch (a robots.txt refusal, an unreachable page, an HTTP error status, a refused private address, an unusable proxy setting) and arguments that do not match the tool's schema are returned as a tool result with `isError: true`, whose text starts `Error executing tool fetch:` and says what went wrong, so the model can read it and decide what to do. -- Calling a tool the server does not have is a JSON-RPC protocol error, `-32602` (`Unknown tool: `), as the specification requires. So is getting a prompt it does not have (`Unknown prompt: `), or the `fetch` prompt without its `url` (`URL is required`). +- Calling a tool the server does not have is a JSON-RPC protocol error, `-32602` (`Unknown tool: `), as the specification requires. So is getting a prompt it does not have (`Unknown prompt: `), or the `fetch` prompt without its `url` (`URL is required`) or with a `url` that is not a valid URL (`Invalid URL '': `). - When the `fetch` prompt's fetch fails, the prompt still returns: its message holds the reason the fetch failed. The server is built on the MCP Python SDK's high-level `MCPServer`: the tool and the prompt are decorated functions, and their type hints and field descriptions are the input schema and the prompt's arguments. diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index a36c5a2547..5e1482bb14 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -14,6 +14,8 @@ # prompt the server does not have, or a prompt call without its `url`, is a # malformed request, answered with a JSON-RPC `-32602` protocol error by # `reject_unknown_names` before `MCPServer` sees it (see there for why). +# A prompt `url` that is not a valid URL gets the same `-32602` from the +# prompt handler (#4914). # `serve()` serves both protocol eras (#4853): a 2025-11-25 client through the # initialize handshake, and a 2026-07-28 client sessionlessly. No handler reads # the session or calls back to the client (roots, sampling, elicitation, @@ -29,6 +31,7 @@ from urllib.parse import urlparse, urlunparse import httpx +from pydantic import TypeAdapter, ValidationError import markdownify import readabilipy.simple_json from bs4 import BeautifulSoup, Tag @@ -597,6 +600,10 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: Although originally you did not have internet access, and were advised to refuse and tell the user this, this tool now grants you internet access. Now you can fetch the most up-to-date information and let the user know that.""" +# Validates the prompt's `url` the way the tool's AnyUrl parameter does. +_PROMPT_URL = TypeAdapter(AnyUrl) + + def reject_unknown_names(server: MCPServer) -> ServerMiddleware[Any]: """Middleware answering a malformed tool or prompt request with `-32602`. @@ -742,6 +749,16 @@ async def fetch( async def fetch_prompt( url: Annotated[str, Field(description="URL to fetch")], ) -> str: + # The URL is checked as the tool's AnyUrl parameter checks it, so a + # malformed one is a bad argument (-32602) rather than an exception + # out of httpx, which the SDK would answer with code 0 (#4914). + try: + url = str(_PROMPT_URL.validate_python(url)) + except ValidationError as e: + raise MCPError( + code=INVALID_PARAMS, + message=f"Invalid URL {url!r}: {e.errors()[0]['msg']}", + ) # The user asked for this page, so robots.txt is not consulted, and a # failure is shown to them as the prompt's text rather than raised. try: diff --git a/src/fetch/tests/test_prompt_url.py b/src/fetch/tests/test_prompt_url.py new file mode 100644 index 0000000000..bc19088ccb --- /dev/null +++ b/src/fetch/tests/test_prompt_url.py @@ -0,0 +1,91 @@ +"""The `fetch` prompt rejects a malformed `url` as a bad argument (#4914). + +The tool's `url` is an AnyUrl, so pydantic rejects a malformed one before the +tool runs. The prompt's `url` is a plain string (prompt arguments always are), +and it used to go to httpx unchecked: `httpx.InvalidURL` is not an +`HTTPError`, so it escaped the prompt's error handling and the SDK answered +with JSON-RPC code 0, which a client cannot tell from a server fault. These +tests pin the fix at the protocol level: the same AnyUrl check, answered with +`-32602` and the validation message, in both protocol eras and on the raw +wire, with nothing fetched; and a valid URL still fetched, normalized the way +the tool normalizes it. +""" + +import httpx +import pytest +from mcp.shared.exceptions import MCPError +from mcp.types import INVALID_PARAMS, JSONRPCError, TextContent + +from .harness import FakeWeb, connect, raw_request +from .test_eras import ERAS +from .test_eras import connect as connect_era + +# (url, the pydantic message the error carries) +INVALID = [ + pytest.param("http://[::1", "invalid IPv6 address", id="unclosed-ipv6"), + pytest.param("not a url", "relative URL without a base", id="no-scheme"), + pytest.param("http://", "empty host", id="empty-host"), + pytest.param("http://example.com:99999/", "invalid port number", id="bad-port"), +] + + +def expected_message(url: str, reason: str) -> str: + return f"Invalid URL {url!r}: Input should be a valid URL, {reason}" + + +@pytest.mark.parametrize(("url", "reason"), INVALID) +async def test_invalid_prompt_url_is_invalid_params_on_the_wire( + web: FakeWeb, url: str, reason: str +) -> None: + # The issue's reproduction, as raw JSON-RPC: code -32602, never 0. + response = await raw_request( + "prompts/get", {"name": "fetch", "arguments": {"url": url}} + ) + assert isinstance(response, JSONRPCError), response + assert response.error.code == INVALID_PARAMS + assert response.error.message == expected_message(url, reason) + assert web.requests == [] + + +@pytest.mark.parametrize(("url", "reason"), INVALID) +async def test_invalid_prompt_url_is_invalid_params_through_a_client( + web: FakeWeb, url: str, reason: str +) -> None: + async with connect() as (session, _): + with pytest.raises(MCPError) as excinfo: + await session.get_prompt("fetch", {"url": url}) + assert excinfo.value.error.code == INVALID_PARAMS + assert excinfo.value.error.message == expected_message(url, reason) + assert web.requests == [] + + +@pytest.mark.parametrize(("mode", "negotiated"), ERAS) +async def test_invalid_prompt_url_is_invalid_params_in_both_eras( + web: FakeWeb, mode: str, negotiated: str +) -> None: + async with connect_era(mode) as client: + assert client.session.protocol_version == negotiated + with pytest.raises(MCPError) as excinfo: + await client.get_prompt("fetch", {"url": "http://[::1"}) + assert excinfo.value.error.code == INVALID_PARAMS + assert excinfo.value.error.message == expected_message( + "http://[::1", "invalid IPv6 address" + ) + assert web.requests == [] + + +async def test_valid_prompt_url_is_fetched_as_the_tool_would_fetch_it( + web: FakeWeb, +) -> None: + # A bare host is normalized with its trailing slash, as the tool's AnyUrl + # normalizes it, and the page comes back as the prompt's text. + web.add( + "https://example.com/", + httpx.Response(200, content=b"hello", headers={"content-type": "text/plain"}), + ) + async with connect() as (session, _): + result = await session.get_prompt("fetch", {"url": "https://example.com"}) + content = result.messages[0].content + assert isinstance(content, TextContent) + assert content.text.endswith("hello") + assert web.urls() == ["https://example.com/"] diff --git a/src/fetch/tests/test_protocol.py b/src/fetch/tests/test_protocol.py index 8090c7e8c3..30d283d8cd 100644 --- a/src/fetch/tests/test_protocol.py +++ b/src/fetch/tests/test_protocol.py @@ -15,7 +15,6 @@ from __future__ import annotations -import os from collections.abc import AsyncIterator from contextlib import asynccontextmanager from importlib.metadata import version @@ -1183,26 +1182,6 @@ async def test_get_prompt_with_unknown_name_is_a_jsonrpc_error( assert web.requests == [] -async def test_get_prompt_does_not_validate_the_url( - monkeypatch: pytest.MonkeyPatch, -) -> None: - # The prompt passes the raw string to httpx; an unusable URL comes back as - # a "Failed to fetch" prompt message rather than a validation error. - # No `web` fixture: the real transport rejects the URL before any I/O. - # Proxy variables are cleared so the real client cannot pick up one from - # the environment and send the request through it. - for name in list(os.environ): - if name.lower() in ("http_proxy", "https_proxy", "all_proxy"): - monkeypatch.delenv(name) - async with connect() as (session, _): - result = await session.get_prompt("fetch", {"url": "not a url"}) - data = wire(result) - assert data["description"] == PROMPT_DESCRIPTION - assert data["messages"][0]["content"]["text"].startswith( - "Failed to fetch not a url: UnsupportedProtocol(" - ) - - # --------------------------------------------------------------------------- # Protocol era # --------------------------------------------------------------------------- From 2eb63af9f166b45cdcc3c348415ab18c0aab7429 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:19:19 -0400 Subject: [PATCH 02/14] feat(fetch): ask for native markdown through Accept content negotiation A non-raw fetch now sends Accept: text/markdown, text/html;q=0.9, */*;q=0.8, so a site that negotiates content can serve markdown directly. A text/markdown response is returned as-is, without the raw-content prefix that labeled it as content that "cannot be simplified to markdown". A raw fetch keeps httpx's default Accept, so it still gets the page's own HTML. Closes #5198 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/README.md | 4 + src/fetch/src/mcp_server_fetch/server.py | 18 +++ src/fetch/tests/test_markdown_negotiation.py | 136 +++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 src/fetch/tests/test_markdown_negotiation.py diff --git a/src/fetch/README.md b/src/fetch/README.md index 09b4e8b5d8..2824c45600 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -39,6 +39,10 @@ The fetch tool will truncate the response, but by using the `start_index` argume The server is built on the MCP Python SDK's high-level `MCPServer`: the tool and the prompt are decorated functions, and their type hints and field descriptions are the input schema and the prompt's arguments. +### Content negotiation + +The tool and the prompt ask for markdown first, with the request header `Accept: text/markdown, text/html;q=0.9, */*;q=0.8`. A site that serves markdown through content negotiation sends it directly, and a `text/markdown` response is returned as-is: it is not simplified and carries no raw-content notice. A site that ignores the header sends what it always did, and HTML is simplified to markdown as before. A `raw` fetch does not ask for markdown, so it still gets the page's own HTML (a markdown response it receives anyway is returned as-is too). + ### Protocol versions The server speaks both eras of the Model Context Protocol over stdio, with nothing to configure: a **2026-07-28** client (discovered with `server/discover`, no session) and a **2025-11-25** client (the `initialize` handshake). The client's first request decides which. Tools and prompts behave the same in both. Methods the 2026-07-28 specification removes, such as `ping`, are answered only on a 2025-11-25 connection. diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index a36c5a2547..b6723cee54 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -507,6 +507,17 @@ async def check_may_autonomously_fetch_url( ) +# The Accept header of a non-raw fetch (#5198). A site that negotiates content +# can then serve native markdown, which is returned as-is, and a site that does +# not still serves HTML (preferred over everything else) to be simplified. +MARKDOWN_ACCEPT = "text/markdown, text/html;q=0.9, */*;q=0.8" + + +def _is_markdown(content_type: str) -> bool: + """Whether a Content-Type header declares markdown (``text/markdown``, RFC 7763).""" + return content_type.split(";", 1)[0].strip().lower() == "text/markdown" + + def _body_looks_like_html(page_raw: str) -> bool: """Whether a body served with no Content-Type is an HTML document. @@ -557,6 +568,9 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: hooks.append(_check_redirect_robots_txt) if hooks: client.event_hooks = {"request": hooks} + # A raw fetch keeps httpx's default Accept, so it still gets the HTML. + if not force_raw: + client.headers["Accept"] = MARKDOWN_ACCEPT try: response = await client.get( url, @@ -574,6 +588,10 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: page_raw = decode_page(response) content_type = response.headers.get("content-type", "") + # Markdown is already what this tool returns, so it is not simplified, and + # it carries no raw-content prefix even under force_raw: it is unchanged. + if _is_markdown(content_type): + return page_raw, "" is_page_html = ( "Test Page +
+

Hello World

+

This is a link in a test paragraph +with enough words to count as the main content of the page.

+
+""" + + +def markdown(content_type: str = "text/markdown; charset=utf-8") -> httpx.Response: + return httpx.Response( + 200, content=MARKDOWN.encode(), headers={"content-type": content_type} + ) + + +async def call_fetch(web: FakeWeb, arguments: dict[str, object]) -> str: + async with connect() as (session, _): + result = await session.call_tool("fetch", {"url": PAGE, **arguments}) + data = result.model_dump(by_alias=True, mode="json", exclude_none=True) + assert data["isError"] is False + return data["content"][0]["text"] + + +def page_requests(web: FakeWeb) -> list[httpx.Request]: + return [r for r in web.requests if str(r.url) == PAGE] + + +async def test_a_fetch_prefers_markdown_then_html(web: FakeWeb) -> None: + web.add(ROBOTS, httpx.Response(404)) + web.add(PAGE, markdown()) + await call_fetch(web, {}) + [request] = page_requests(web) + assert request.headers["accept"] == "text/markdown, text/html;q=0.9, */*;q=0.8" + assert MARKDOWN_ACCEPT == request.headers["accept"] + # The robots.txt lookup is not a page fetch and asks for nothing new. + [robots] = [r for r in web.requests if str(r.url) == ROBOTS] + assert robots.headers["accept"] == "*/*" + + +@pytest.mark.parametrize( + "content_type", ["text/markdown", "text/markdown; charset=utf-8", "Text/Markdown"] +) +async def test_markdown_is_returned_as_is_without_the_raw_prefix( + web: FakeWeb, content_type: str +) -> None: + web.add(ROBOTS, httpx.Response(404)) + web.add(PAGE, markdown(content_type)) + text = await call_fetch(web, {}) + assert text == f"Contents of {PAGE}:\n{MARKDOWN}" + + +async def test_a_raw_fetch_keeps_the_default_accept(web: FakeWeb) -> None: + web.add(ROBOTS, httpx.Response(404)) + web.add( + PAGE, httpx.Response(200, text=ARTICLE, headers={"content-type": "text/html"}) + ) + text = await call_fetch(web, {"raw": True}) + [request] = page_requests(web) + assert request.headers["accept"] == "*/*" + assert text.endswith(f"Contents of {PAGE}:\n{ARTICLE}") + + +async def test_markdown_under_raw_is_unchanged_and_unprefixed(web: FakeWeb) -> None: + # Markdown served unasked (a .md file, say) is the same either way. + web.add(ROBOTS, httpx.Response(404)) + web.add(PAGE, markdown()) + text = await call_fetch(web, {"raw": True}) + assert text == f"Contents of {PAGE}:\n{MARKDOWN}" + + +@pytest.mark.usefixtures("python_readability") +async def test_html_from_a_site_that_ignores_accept_is_still_simplified( + web: FakeWeb, +) -> None: + web.add(ROBOTS, httpx.Response(404)) + web.add( + PAGE, httpx.Response(200, text=ARTICLE, headers={"content-type": "text/html"}) + ) + text = await call_fetch(web, {}) + assert "cannot be simplified" not in text + assert "
" not in text + assert "Hello World" in text + + +async def test_the_prompt_returns_markdown_as_is(web: FakeWeb) -> None: + web.add(PAGE, markdown()) + async with connect() as (session, _): + result = await session.get_prompt("fetch", {"url": PAGE}) + message = result.model_dump(by_alias=True, mode="json")["messages"][0] + assert message["content"]["text"].endswith(MARKDOWN) + assert "cannot be simplified" not in message["content"]["text"] + [request] = page_requests(web) + assert request.headers["accept"] == MARKDOWN_ACCEPT + + +@pytest.mark.parametrize( + ("content_type", "expected"), + [ + ("text/markdown", True), + (" text/markdown ; variant=GFM", True), + ("text/markdownish", False), + ("text/x-markdown", False), + ("text/plain", False), + ("", False), + ], +) +def test_is_markdown(content_type: str, expected: bool) -> None: + assert _is_markdown(content_type) is expected From fed94f819c895e27e1a748aa703525cd3148b41d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:24:05 -0400 Subject: [PATCH 03/14] feat(fetch): make the HTTP request timeout configurable The page request's 30 s timeout was hard-coded. It can now be set per call with the fetch tool's timeout_ms argument (100 to 600000 ms, advertised with the server default as its default), server-wide with --timeout-ms or FETCH_TIMEOUT_MS, the most specific winning, and stays 30 s when none is given. A timed-out request says how long it waited. The robots.txt check keeps httpx's own default. Closes #4448 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/README.md | 20 ++ src/fetch/src/mcp_server_fetch/__init__.py | 24 ++ src/fetch/src/mcp_server_fetch/server.py | 25 +- src/fetch/src/mcp_server_fetch/timeout.py | 57 +++++ src/fetch/tests/test_protocol.py | 9 + src/fetch/tests/test_timeout.py | 285 +++++++++++++++++++++ 6 files changed, 419 insertions(+), 1 deletion(-) create mode 100644 src/fetch/src/mcp_server_fetch/timeout.py create mode 100644 src/fetch/tests/test_timeout.py diff --git a/src/fetch/README.md b/src/fetch/README.md index 09b4e8b5d8..fd0bd0cf71 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -22,6 +22,7 @@ The fetch tool will truncate the response, but by using the `start_index` argume - `max_length` (integer, optional): Maximum number of characters to return (default: 5000) - `start_index` (integer, optional): Start content from this character index (default: 0) - `raw` (boolean, optional): Get raw content without markdown conversion (default: false) + - `timeout_ms` (integer, optional): HTTP request timeout for this call, in milliseconds, from 100 to 600000 (default: the server's timeout, 30000 unless configured; see [Customization - Timeout](#customization---timeout)) - Annotations: `readOnlyHint: true`, `destructiveHint: false`, `idempotentHint: true`, `openWorldHint: true` (it only reads, and it reaches arbitrary hosts on the internet) ### Prompts @@ -202,6 +203,25 @@ ModelContextProtocol/1.0 (User-Specified; +https://github.com/modelcontextprotoc This can be customized by adding the argument `--user-agent=YourUserAgent` to the `args` list in the configuration. +### Customization - Timeout + +The HTTP request for the page waits up to 30 seconds (30000 ms) by default: to connect, and then for each read of the +response. It bounds each of those separately, not the whole download, so a large page that keeps arriving is not cut +off. A request that times out fails with a tool error saying how long it waited. + +The timeout can be set at three levels, the most specific winning: + +1. The `fetch` tool's `timeout_ms` argument, for one call (for example `120000` for a large download, or `5000` to + fail fast). +2. The argument `--timeout-ms=60000` in the `args` list in the configuration, for every call. +3. The `FETCH_TIMEOUT_MS` environment variable (for example `"env": {"FETCH_TIMEOUT_MS": "60000"}` in the + configuration), for every call when `--timeout-ms` is not given. + +The server-wide value also applies to the `fetch` prompt, and is the default the tool advertises for `timeout_ms`. +Every level takes a whole number of milliseconds from 100 to 600000 (10 minutes); the server refuses to start with a +value outside that range, and the tool refuses such a `timeout_ms`. This is the HTTP request timeout only: the +robots.txt check before an autonomous fetch keeps its own 5-second timeout, and HTML extraction is not covered. + ### Customization - Proxy The server can be configured to use a proxy by using the `--proxy-url` argument. diff --git a/src/fetch/src/mcp_server_fetch/__init__.py b/src/fetch/src/mcp_server_fetch/__init__.py index 39ac728705..853b945695 100644 --- a/src/fetch/src/mcp_server_fetch/__init__.py +++ b/src/fetch/src/mcp_server_fetch/__init__.py @@ -1,15 +1,31 @@ from .server import serve +from .timeout import ( + DEFAULT_TIMEOUT_MS, + MAX_TIMEOUT_MS, + MIN_TIMEOUT_MS, + TIMEOUT_ENV, + parse_timeout_ms, + timeout_ms_from_env, +) def main(): """MCP Fetch Server - HTTP fetching functionality for MCP""" import argparse import asyncio + import os parser = argparse.ArgumentParser( description="give a model the ability to make web requests" ) parser.add_argument("--user-agent", type=str, help="Custom User-Agent string") + parser.add_argument( + "--timeout-ms", + type=parse_timeout_ms, + help=f"HTTP request timeout in milliseconds, {MIN_TIMEOUT_MS} to " + f"{MAX_TIMEOUT_MS} (default: ${TIMEOUT_ENV}, else {DEFAULT_TIMEOUT_MS}); " + "a tool call's timeout_ms argument overrides it", + ) parser.add_argument( "--ignore-robots-txt", action="store_true", @@ -29,6 +45,14 @@ def main(): ) args = parser.parse_args() + # The server reads its timeout from the environment (see timeout.py), and + # the flag outranks a value already there; a bad value stops it here. + if args.timeout_ms is not None: + os.environ[TIMEOUT_ENV] = str(args.timeout_ms) + try: + timeout_ms_from_env() + except ValueError as e: + parser.error(str(e)) asyncio.run( serve( args.user_agent, diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index a36c5a2547..9b07f57e83 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -36,6 +36,12 @@ from mcp.server.context import CallNext, HandlerResult, ServerMiddleware from mcp.server.mcpserver.exceptions import ToolError from mcp.shared.exceptions import MCPError +from .timeout import ( + DEFAULT_TIMEOUT_MS, + MAX_TIMEOUT_MS, + MIN_TIMEOUT_MS, + timeout_ms_from_env, +) from mcp.types import INVALID_PARAMS, ToolAnnotations from protego import Protego from pydantic import AnyUrl, Field @@ -523,6 +529,7 @@ async def fetch_url( url: str, user_agent: str, force_raw: bool = False, + timeout_ms: int = DEFAULT_TIMEOUT_MS, proxy_url: str | None = None, allow_private_ips: bool = False, check_robots_txt: bool = False, @@ -562,7 +569,11 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: url, follow_redirects=True, headers={"User-Agent": user_agent}, - timeout=30, + timeout=timeout_ms / 1000, + ) + except httpx.TimeoutException as e: + raise ToolError( + f"Failed to fetch {url}: timed out after {timeout_ms} ms: {e!r}" ) except HTTPError as e: raise ToolError(f"Failed to fetch {url}: {e!r}") @@ -650,6 +661,7 @@ def create_server( normalize_proxy_env() user_agent_autonomous = custom_user_agent or DEFAULT_USER_AGENT_AUTONOMOUS user_agent_manual = custom_user_agent or DEFAULT_USER_AGENT_MANUAL + default_timeout_ms = timeout_ms_from_env() # subscriptions=False: the tool and prompt lists never change and there are # no resources to watch, so `subscriptions/listen` is not served and the @@ -703,6 +715,15 @@ async def fetch( strict=True, ), ] = False, + timeout_ms: Annotated[ + int, + Field( + description="How long to wait, in milliseconds, to connect to the server and for each read of its response before giving up. Raise it for a slow server or a large download; lower it to fail fast.", + ge=MIN_TIMEOUT_MS, + le=MAX_TIMEOUT_MS, + strict=True, + ), + ] = default_timeout_ms, ) -> str: page_url = str(url) @@ -715,6 +736,7 @@ async def fetch( page_url, user_agent_autonomous, force_raw=raw, + timeout_ms=timeout_ms, proxy_url=proxy_url, allow_private_ips=allow_private_ips, check_robots_txt=not ignore_robots_txt, @@ -748,6 +770,7 @@ async def fetch_prompt( content, prefix = await fetch_url( url, user_agent_manual, + timeout_ms=default_timeout_ms, proxy_url=proxy_url, allow_private_ips=allow_private_ips, use_readability_js=use_readability_js, diff --git a/src/fetch/src/mcp_server_fetch/timeout.py b/src/fetch/src/mcp_server_fetch/timeout.py new file mode 100644 index 0000000000..b066949a6a --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/timeout.py @@ -0,0 +1,57 @@ +# The fetch server's HTTP request timeout (#4448): its default, its bounds, and +# how a configured value is read. It applies to the page request a `fetch` tool +# call or prompt makes, and is set at three levels, the most specific winning: +# the tool's `timeout_ms` argument, the `--timeout-ms` flag, the +# `FETCH_TIMEOUT_MS` environment variable, and otherwise 30 s (the value the +# server always used). It is httpx's per-phase timeout, so it bounds the +# connect, each read and each write separately rather than the whole transfer. +# The same bounds hold at every level, so a server default always satisfies the +# tool argument's advertised minimum and maximum, and becomes its advertised +# default. robots.txt checks are not covered: they keep httpx's own default. +# +# The server-wide value has one source, the environment variable: +# `create_server` reads it, and `main()` copies `--timeout-ms` into it (the flag +# outranks a value the environment already had). So the setting reaches the +# server the same way whether it was given as a flag or as a variable, and +# `serve()`'s positional signature is unchanged for its existing callers. + +import argparse +import os + +DEFAULT_TIMEOUT_MS = 30_000 +MIN_TIMEOUT_MS = 100 +MAX_TIMEOUT_MS = 600_000 +TIMEOUT_ENV = "FETCH_TIMEOUT_MS" + + +def parse_timeout_ms(value: str) -> int: + """Parse a timeout in milliseconds, as the ``--timeout-ms`` argparse type. + + Raises ``argparse.ArgumentTypeError``, whose message argparse prints as is. + """ + try: + timeout_ms = int(value.strip()) + except ValueError: + raise argparse.ArgumentTypeError( + f"{value!r} is not a whole number of milliseconds" + ) + if not MIN_TIMEOUT_MS <= timeout_ms <= MAX_TIMEOUT_MS: + raise argparse.ArgumentTypeError( + f"{timeout_ms} ms is outside {MIN_TIMEOUT_MS}..{MAX_TIMEOUT_MS} ms" + ) + return timeout_ms + + +def timeout_ms_from_env() -> int: + """The server-wide timeout: ``FETCH_TIMEOUT_MS`` when set, else 30 s. + + Raises ``ValueError`` for an unparseable or out-of-range value, so a bad + setting stops the server at startup rather than failing every request. + """ + value = os.environ.get(TIMEOUT_ENV, "").strip() + if not value: + return DEFAULT_TIMEOUT_MS + try: + return parse_timeout_ms(value) + except argparse.ArgumentTypeError as e: + raise ValueError(f"{TIMEOUT_ENV}: {e}") from None diff --git a/src/fetch/tests/test_protocol.py b/src/fetch/tests/test_protocol.py index 8090c7e8c3..60d214c190 100644 --- a/src/fetch/tests/test_protocol.py +++ b/src/fetch/tests/test_protocol.py @@ -187,6 +187,15 @@ async def test_list_tools_wire_shape(web: FakeWeb) -> None: "title": "Raw", "type": "boolean", }, + # #4448: the server-wide timeout is the advertised default. + "timeout_ms": { + "default": 30000, + "description": "How long to wait, in milliseconds, to connect to the server and for each read of its response before giving up. Raise it for a slow server or a large download; lower it to fail fast.", + "maximum": 600000, + "minimum": 100, + "title": "Timeout Ms", + "type": "integer", + }, }, "required": ["url"], "title": "fetchArguments", diff --git a/src/fetch/tests/test_timeout.py b/src/fetch/tests/test_timeout.py new file mode 100644 index 0000000000..4cd437dec1 --- /dev/null +++ b/src/fetch/tests/test_timeout.py @@ -0,0 +1,285 @@ +"""Tests for the configurable HTTP request timeout (#4448). + +The timeout is set at three levels, the most specific winning: the ``fetch`` +tool's ``timeout_ms`` argument, then ``--timeout-ms`` / ``FETCH_TIMEOUT_MS``, +then the 30 s the server always used. Tool and prompt tests drive the real +server through an MCP client (``harness.connect``) with HTTP served by +``harness.FakeWeb``, and read the timeout httpx actually applied from the +request's ``timeout`` extension, so they assert what reaches the transport +rather than what a function was called with. ``main()`` runs in-process with +``serve`` patched. +""" + +from __future__ import annotations + +import os +import sys +from typing import Any +from unittest.mock import AsyncMock + +import httpx +import pytest + +import mcp_server_fetch +from mcp_server_fetch.server import create_server +from mcp_server_fetch.timeout import ( + DEFAULT_TIMEOUT_MS, + MAX_TIMEOUT_MS, + MIN_TIMEOUT_MS, + TIMEOUT_ENV, + timeout_ms_from_env, +) + +from .harness import FakeWeb, connect + +PAGE = "https://example.com/page" +TOOL_ERROR = "Error executing tool fetch: " + + +@pytest.fixture(autouse=True) +def no_timeout_env(monkeypatch: pytest.MonkeyPatch) -> None: + # A developer's own FETCH_TIMEOUT_MS must not leak into these tests, and + # monkeypatch restores whatever main() writes to the variable. + monkeypatch.delenv(TIMEOUT_ENV, raising=False) + + +def recording_page(seen: list[dict[str, Any]]) -> Any: + def respond(request: httpx.Request) -> httpx.Response: + seen.append(dict(request.extensions["timeout"])) + return httpx.Response( + 200, content=b"hello", headers={"content-type": "text/plain"} + ) + + return respond + + +def every_phase(seconds: float) -> dict[str, float]: + return {"connect": seconds, "read": seconds, "write": seconds, "pool": seconds} + + +def wire(result: Any) -> dict[str, Any]: + return result.model_dump(by_alias=True, mode="json", exclude_none=True) + + +async def fetch_timeouts( + web: FakeWeb, arguments: dict[str, Any] +) -> list[dict[str, Any]]: + seen: list[dict[str, Any]] = [] + web.add(PAGE, recording_page(seen)) + async with connect(ignore_robots_txt=True) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE, **arguments}) + assert wire(result)["isError"] is False + return seen + + +# --- tool ------------------------------------------------------------------- + + +async def test_default_is_thirty_seconds(web: FakeWeb) -> None: + assert await fetch_timeouts(web, {}) == [every_phase(30.0)] + + +async def test_environment_sets_the_server_default( + web: FakeWeb, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "5000") + assert await fetch_timeouts(web, {}) == [every_phase(5.0)] + + +async def test_tool_argument_outranks_the_server_default( + web: FakeWeb, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "5000") + assert await fetch_timeouts(web, {"timeout_ms": 120000}) == [every_phase(120.0)] + + +@pytest.mark.parametrize("timeout_ms", [MIN_TIMEOUT_MS, MAX_TIMEOUT_MS]) +async def test_tool_argument_bounds_are_inclusive( + web: FakeWeb, timeout_ms: int +) -> None: + assert await fetch_timeouts(web, {"timeout_ms": timeout_ms}) == [ + every_phase(timeout_ms / 1000) + ] + + +@pytest.mark.parametrize( + "timeout_ms", [MIN_TIMEOUT_MS - 1, MAX_TIMEOUT_MS + 1, 0, -1, "1000", 1.5, True] +) +async def test_tool_argument_outside_the_schema_is_a_tool_error( + web: FakeWeb, timeout_ms: Any +) -> None: + web.add(PAGE, httpx.Response(200, content=b"never fetched")) + async with connect(ignore_robots_txt=True) as (session, _): + result = await session.call_tool( + "fetch", {"url": PAGE, "timeout_ms": timeout_ms} + ) + data = wire(result) + assert data["isError"] is True + assert "timeout_ms" in data["content"][0]["text"] + assert web.requests == [] + + +async def test_schema_advertises_bounds_and_the_server_default( + web: FakeWeb, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "7000") + async with connect() as (session, _): + [tool] = wire(await session.list_tools())["tools"] + prop = tool["inputSchema"]["properties"]["timeout_ms"] + assert prop["type"] == "integer" + assert prop["minimum"] == MIN_TIMEOUT_MS + assert prop["maximum"] == MAX_TIMEOUT_MS + assert prop["default"] == 7000 + assert "milliseconds" in prop["description"] + assert tool["inputSchema"]["required"] == ["url"] + + +async def test_a_timeout_says_how_long_it_waited(web: FakeWeb) -> None: + def hang(request: httpx.Request) -> httpx.Response: + raise httpx.ReadTimeout("timed out", request=request) + + web.add(PAGE, hang) + async with connect(ignore_robots_txt=True) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE, "timeout_ms": 2000}) + data = wire(result) + assert data["isError"] is True + [content] = data["content"] + assert content["text"].startswith( + f"{TOOL_ERROR}Failed to fetch {PAGE}: timed out after 2000 ms: ReadTimeout(" + ) + + +async def test_robots_txt_check_keeps_the_httpx_default(web: FakeWeb) -> None: + # Only the page request is configurable; the robots.txt check is not. + robots: list[dict[str, Any]] = [] + page: list[dict[str, Any]] = [] + + def robots_txt(request: httpx.Request) -> httpx.Response: + robots.append(dict(request.extensions["timeout"])) + return httpx.Response(200, content=b"User-agent: *\nAllow: /") + + web.add("https://example.com/robots.txt", robots_txt) + web.add(PAGE, recording_page(page)) + async with connect() as (session, _): + result = await session.call_tool("fetch", {"url": PAGE, "timeout_ms": 9000}) + assert wire(result)["isError"] is False + assert robots == [every_phase(5.0)] + assert page == [every_phase(9.0)] + + +# --- prompt ----------------------------------------------------------------- + + +async def test_prompt_uses_the_server_default( + web: FakeWeb, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "4000") + seen: list[dict[str, Any]] = [] + web.add(PAGE, recording_page(seen)) + async with connect() as (session, _): + result = await session.get_prompt("fetch", {"url": PAGE}) + assert "hello" in str(result.messages[0].content) + assert seen == [every_phase(4.0)] + + +# --- server-wide setting ---------------------------------------------------- + + +@pytest.mark.parametrize("value", ["", " "]) +def test_unset_or_blank_environment_is_the_default( + monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, value) + assert timeout_ms_from_env() == DEFAULT_TIMEOUT_MS + + +def test_environment_value_is_trimmed(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(TIMEOUT_ENV, " 2500 ") + assert timeout_ms_from_env() == 2500 + + +@pytest.mark.parametrize( + ("value", "message"), + [ + ("abc", "'abc' is not a whole number of milliseconds"), + ("1.5", "'1.5' is not a whole number of milliseconds"), + ("99", f"99 ms is outside {MIN_TIMEOUT_MS}..{MAX_TIMEOUT_MS} ms"), + ("600001", f"600001 ms is outside {MIN_TIMEOUT_MS}..{MAX_TIMEOUT_MS} ms"), + ], +) +def test_bad_environment_stops_the_server_at_startup( + monkeypatch: pytest.MonkeyPatch, value: str, message: str +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, value) + with pytest.raises(ValueError, match=f"^{TIMEOUT_ENV}: {message}$"): + create_server() + + +# --- command line ----------------------------------------------------------- + + +def run_main(monkeypatch: pytest.MonkeyPatch, *argv: str) -> AsyncMock: + serve = AsyncMock() + monkeypatch.setattr(mcp_server_fetch, "serve", serve) + monkeypatch.setattr(sys, "argv", ["mcp-server-fetch", *argv]) + mcp_server_fetch.main() + return serve + + +def test_flag_sets_the_variable_the_server_reads( + monkeypatch: pytest.MonkeyPatch, +) -> None: + serve = run_main(monkeypatch, "--timeout-ms", "60000") + serve.assert_awaited_once() + assert os.environ[TIMEOUT_ENV] == "60000" + assert timeout_ms_from_env() == 60000 + + +def test_flag_outranks_the_environment(monkeypatch: pytest.MonkeyPatch) -> None: + # Even a bad value in the environment: the flag replaces it. + monkeypatch.setenv(TIMEOUT_ENV, "nonsense") + run_main(monkeypatch, "--timeout-ms", "1500") + assert os.environ[TIMEOUT_ENV] == "1500" + + +def test_no_flag_leaves_the_environment_alone( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "8000") + run_main(monkeypatch) + assert os.environ[TIMEOUT_ENV] == "8000" + + +@pytest.mark.parametrize("value", ["abc", "50", "600001"]) +def test_bad_flag_is_a_usage_error( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], value: str +) -> None: + with pytest.raises(SystemExit) as excinfo: + run_main(monkeypatch, "--timeout-ms", value) + assert excinfo.value.code == 2 + assert "argument --timeout-ms:" in capsys.readouterr().err + assert TIMEOUT_ENV not in os.environ + + +def test_bad_environment_is_a_usage_error( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setenv(TIMEOUT_ENV, "soon") + with pytest.raises(SystemExit) as excinfo: + run_main(monkeypatch) + assert excinfo.value.code == 2 + assert ( + f"{TIMEOUT_ENV}: 'soon' is not a whole number of milliseconds" + in capsys.readouterr().err + ) + + +def test_flag_is_in_help( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as excinfo: + run_main(monkeypatch, "--help") + assert excinfo.value.code == 0 + out = capsys.readouterr().out + assert "--timeout-ms" in out + assert TIMEOUT_ENV in out From baf1306ac3f29ce329486a8277c3415fb3d332e8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:25:03 -0400 Subject: [PATCH 04/14] fix(fetch): run Readability.js under the server's own bounded subprocess readabilipy's Node.js wrapper had no timeout and ran on the event loop (#5306), let `npm install` and a failed run's stderr reach stdout, the stdio JSON-RPC channel (#5310), and ran a bare `npm` that Windows cannot resolve to npm.cmd (#5313). The new readability_js module runs ExtractArticle.js itself: node and npm resolved with shutil.which, stdin from the null device, output captured and forwarded to stderr, a timeout on every process with the whole process tree killed on expiry, the dependency install done at most once per process under a lock, and any failure falling back to the pure-Python extractor. fetch_url runs the extraction in a worker thread. The README states that the first HTML extraction with Node.js installs Readability.js's dependencies from the npm registry, what that means for an egress allowlist and for pinning, and how to install them ahead of time (#4830). The Windows CI leg no longer pre-installs them, so it exercises the server's own install through npm.cmd. Closes #5306 Closes #5310 Closes #5313 Closes #4830 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .github/workflows/python.yml | 18 +- src/fetch/README.md | 43 +- .../src/mcp_server_fetch/readability_js.py | 259 ++++++++++ src/fetch/src/mcp_server_fetch/server.py | 46 +- src/fetch/tests/harness.py | 28 +- src/fetch/tests/test_empty_tree.py | 4 +- src/fetch/tests/test_hidden_ssr.py | 26 +- .../tests/test_readability_js_opt_out.py | 39 +- .../tests/test_readability_subprocess.py | 457 ++++++++++++++++++ 9 files changed, 850 insertions(+), 70 deletions(-) create mode 100644 src/fetch/src/mcp_server_fetch/readability_js.py create mode 100644 src/fetch/tests/test_readability_subprocess.py diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index f5ddd6b26a..219e515924 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -142,24 +142,16 @@ jobs: working-directory: src/${{ matrix.package }} run: uv sync --locked --all-extras --dev - # fetch's tests pin readabilipy's Readability.js path, which needs Node - # and readabilipy's JavaScript dependencies. readabilipy >= 0.3.0 ships - # only their package.json and runs `npm install` on first use, but on - # Windows its subprocess call cannot find npm (npm.cmd), so it falls back - # to the pure-Python extractor. Install them here, as an operator would. + # fetch's tests pin the Readability.js path, which needs Node and npm. + # readabilipy >= 0.3.0 ships only its JavaScript's package.json, and the + # server installs the dependencies on first use, resolving npm through + # PATH so Windows' npm.cmd is found (#5313). Nothing is pre-installed + # here, so this leg proves that install works on Windows. - uses: actions/setup-node@v7 if: matrix.package == 'fetch' with: node-version: 24 - - name: Install readabilipy's JavaScript dependencies - if: matrix.package == 'fetch' - working-directory: src/fetch - shell: bash - run: | - jsdir=$(uv run --frozen python -c "import os, readabilipy; print(os.path.join(os.path.dirname(readabilipy.__file__), 'javascript'))") - cd "$jsdir" && npm install --no-audit --no-fund - - name: Run pytest working-directory: src/${{ matrix.package }} run: uv run --frozen pytest diff --git a/src/fetch/README.md b/src/fetch/README.md index 09b4e8b5d8..7f90ee4a7c 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -46,6 +46,7 @@ The server speaks both eras of the Model Context Protocol over stdio, with nothi ## Installation Optionally: Install node.js, this will cause the fetch server to use a different HTML simplifier that is more robust. +Its JavaScript dependencies are then installed with npm on first use, which contacts the npm registry. To keep the server from ever starting Node.js, see [Customization - HTML extraction](#customization---html-extraction). ### Using uv (recommended) @@ -213,11 +214,34 @@ installed with the server). A `socks://` proxy URL, as many desktop proxy settin ### Customization - HTML extraction -HTML is converted to Markdown by [readabilipy](https://github.com/alan-turing-institute/ReadabiliPy). When Node.js is -on the `PATH`, readabilipy runs Mozilla's Readability.js in a Node.js subprocess; otherwise it uses its own -pure-Python extractor, which keeps headings and text but drops link targets. The Node.js subprocess runs with no -timeout, so a Node.js that hangs, or fails in a way readabilipy does not detect, stalls the tool call until the client -gives up. +HTML is converted to Markdown with [readabilipy](https://github.com/alan-turing-institute/ReadabiliPy). When Node.js 14 +or newer is on the `PATH`, the server runs Mozilla's Readability.js (readabilipy's `ExtractArticle.js`) in a Node.js +subprocess; otherwise it uses readabilipy's pure-Python extractor, which keeps headings and text but drops link +targets. The subprocess runs off the server's event loop, so other requests are answered while it works, and under a +30 second timeout: a Node.js that hangs is killed, together with anything it started, and that page is extracted with +the pure-Python extractor instead. If Readability.js fails in any other way, the same fallback is used. Every reason +for a fallback is written to stderr (the server's log), never to stdout, which carries the protocol. + +**Readability.js's dependencies are installed with npm on first use, which reaches the npm registry.** readabilipy +ships only the `package.json` for its JavaScript (`@mozilla/readability`, `jsdom`, `minimist`). The first time the +server extracts HTML with Node.js on the `PATH` and those packages missing, it runs `npm install` in readabilipy's +`javascript` directory, inside that tool call, so: + +- `registry.npmjs.org` belongs in the egress allowlist of a deployment that restricts outbound traffic, unless the + packages are installed ahead of time (below) or Readability.js is turned off. Without it the install fails and the + server uses the pure-Python extractor. +- The JavaScript that runs is resolved from readabilipy's unpinned version ranges when it is installed, so pinning + `mcp-server-fetch` does not pin it. +- The install runs at most once per server process, under a 120 second timeout. Its output goes to stderr. If it + fails, or Node.js is missing or older than 14, the server uses the pure-Python extractor until it is restarted. + +To install the packages ahead of time (in a container image, say), run `npm install` in readabilipy's `javascript` +directory from the Python environment the server runs in; the server then finds them and installs nothing: + +```bash +cd "$(python -c "import os, readabilipy; print(os.path.join(os.path.dirname(readabilipy.__file__), 'javascript'))")" +npm install --no-audit --no-fund +``` Readability.js discards hidden elements. Pages rendered progressively on the server often send a visible loading shell, then the real content in a container hidden (`display: none`, `visibility: hidden`, `hidden`) until scripts @@ -226,7 +250,8 @@ Readability.js extracted, the server unhides it and runs Readability.js again, k text. Other pages are extracted once, exactly as before. To always use the pure-Python extractor, add the argument `--no-readability-js` to the `args` list in the -configuration. The server then never starts Node.js (or npm), whether or not it is installed. +configuration. The server then never starts Node.js (or npm), whether or not it is installed, and never contacts the +npm registry. ## Windows Configuration @@ -270,6 +295,10 @@ If you're experiencing timeout issues on Windows, you may need to set the `PYTHO This addresses character encoding issues that can cause the server to timeout on Windows systems. +Readability.js works on Windows as it does elsewhere: the server finds Node.js and npm (`npm.cmd`) on the `PATH`, and +installs Readability.js's dependencies on first use as described in +[Customization - HTML extraction](#customization---html-extraction). + ## Debugging You can use the MCP inspector to debug the server. For uvx installations: @@ -294,7 +323,7 @@ uv sync --frozen --all-extras --dev uv run --frozen pytest ``` -The tests drive `serve()` through an MCP `ClientSession` in-process (and, in `tests/test_eras.py`, through a `Client` as a 2026-07-28 and as a 2025-11-25 client), with HTTP mocked, so they need no network. The HTML extraction tests need Node.js on `PATH` (readabilipy uses Readability.js when Node is present and a pure-Python fallback when it is not). +The tests drive `serve()` through an MCP `ClientSession` in-process (and, in `tests/test_eras.py`, through a `Client` as a 2026-07-28 and as a 2025-11-25 client), with HTTP mocked, so they need no network. The HTML extraction tests need Node.js and npm on `PATH` (the server uses Readability.js when Node is present and a pure-Python fallback when it is not), and the first run installs Readability.js's dependencies from the npm registry. Coverage, measured per file with branch coverage: diff --git a/src/fetch/src/mcp_server_fetch/readability_js.py b/src/fetch/src/mcp_server_fetch/readability_js.py new file mode 100644 index 0000000000..770896b3ae --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/readability_js.py @@ -0,0 +1,259 @@ +# Runs Mozilla's Readability.js (readabilipy's `ExtractArticle.js`) under +# Node.js in a subprocess the server controls, instead of through readabilipy's +# own wrapper (`simple_json_from_html_string(use_readability=True)`). +# +# readabilipy's wrapper had four defects for a stdio MCP server, each fixed +# here rather than worked around at the call site: +# +# - No timeout (#5306). Its `subprocess.run` calls wait forever, so a Node.js +# or npm that hangs stalled the tool call. Every process here runs under a +# timeout, and on timeout its whole process tree is killed (a process group +# on POSIX, `taskkill /T` on Windows), so nothing is left running. +# - Child output on fd 1 (#5310). Its `npm install` inherits the server's +# stdout, which over stdio is the JSON-RPC channel, and it `print`s Node's +# stderr to stdout on a failure. Here every child gets stdin from the null +# device (it must never read the server's JSON-RPC input either) and pipes +# for stdout and stderr; whatever a child prints is forwarded to stderr. +# - npm not found on Windows (#5313). It runs a bare `npm`, which Windows' +# `CreateProcess` does not resolve to `npm.cmd`. Here `node` and `npm` are +# resolved with `shutil.which`, which honours `PATHEXT`. +# - A failed Readability.js run raised out of the tool call. Here any failure +# (no Node.js, an old one, no npm, a failed install, a timeout, a crash, bad +# output) returns None, and the caller falls back to readabilipy's +# pure-Python extractor, with the reason written to stderr. +# +# readabilipy >= 0.3.0 ships only `package.json` for its JavaScript and runs +# `npm install` on first use, which reaches the npm registry during a tool call +# (#4830). That stays the behaviour (the README states it), but the install now +# runs at most once per process, under a lock and a timeout. It installs into +# readabilipy's own `javascript` directory, so dependencies an operator +# installed there ahead of time (the README says how) are found and nothing is +# fetched. +# +# Everything here is synchronous: the caller runs extraction in a worker +# thread (`asyncio.to_thread`), so the server's event loop never waits on it. + +from __future__ import annotations + +import json +import os +import shutil +import signal +import subprocess +import sys +import tempfile +import threading +from pathlib import Path + +import readabilipy + +# How long one Readability.js run may take before it is killed. +EXTRACT_TIMEOUT_SECONDS = 30.0 +# How long the one-time `npm install` of readabilipy's JavaScript may take. +INSTALL_TIMEOUT_SECONDS = 120.0 +# How long `node -v` may take. +VERSION_TIMEOUT_SECONDS = 10.0 +# readabilipy's `package.json` declares `"engines": {"node": ">=14.0.0"}`. +MIN_NODE_MAJOR = 14 +# What `ExtractArticle.js` requires; all three present means installed. +JS_DEPENDENCIES = ("@mozilla/readability", "jsdom", "minimist") + +_probe_lock = threading.Lock() +_probed = False +_node: str | None = None + + +def javascript_dir() -> Path: + """readabilipy's `javascript` directory: `ExtractArticle.js` and `node_modules`.""" + return Path(readabilipy.__file__).parent / "javascript" + + +def _log(message: str) -> None: + # stderr only: stdout is the JSON-RPC channel. + print(f"mcp-server-fetch: {message}", file=sys.stderr, flush=True) + + +def _forward(output: str) -> None: + """Copy a child's captured output to stderr, never to stdout.""" + if output: + sys.stderr.write(output if output.endswith("\n") else output + "\n") + sys.stderr.flush() + + +def _kill_tree(proc: subprocess.Popen[str]) -> None: + """Kill ``proc`` and everything it started (npm runs node, `npm.cmd` runs cmd).""" + if sys.platform == "win32": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(proc.pid)], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + else: + try: + # start_new_session made the child a process-group leader. + os.killpg(proc.pid, signal.SIGKILL) + except ProcessLookupError: + pass + # Whatever the tree kill did, the direct child is certainly gone after this. + proc.kill() + + +def _run( + argv: list[str], timeout: float, cwd: Path | None = None +) -> subprocess.CompletedProcess[str] | None: + """Run ``argv`` detached from the server's stdio, or None if it could not + start or did not finish within ``timeout`` seconds (it is killed then).""" + try: + proc = subprocess.Popen( + argv, + cwd=cwd, + stdin=subprocess.DEVNULL, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + encoding="utf-8", + errors="replace", + # POSIX only (ignored on Windows): its own process group, so a + # timeout can kill the processes it starts as well. + start_new_session=True, + ) + except OSError as e: + _log(f"could not start {argv[0]}: {e}") + return None + try: + stdout, stderr = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + _kill_tree(proc) + proc.communicate() + _log(f"{Path(argv[0]).name} did not finish within {timeout:g} s and was killed") + return None + return subprocess.CompletedProcess(argv, proc.returncode, stdout, stderr) + + +def _node_major(version: str) -> int | None: + """The major version in `node -v` output (``v24.1.0``), or None.""" + head = version.strip().lstrip("v").split(".", 1)[0] + return int(head) if head.isdigit() else None + + +def _dependencies_installed(jsdir: Path) -> bool: + modules = jsdir / "node_modules" + return all((modules / name / "package.json").is_file() for name in JS_DEPENDENCIES) + + +def _install_dependencies(jsdir: Path) -> bool: + """`npm install` readabilipy's JavaScript dependencies; whether it worked.""" + npm = shutil.which("npm") + if npm is None: + _log( + "npm was not found on PATH, so Readability.js's dependencies cannot be " + f"installed in {jsdir}; using the pure-Python extractor" + ) + return False + _log( + f"installing Readability.js's dependencies with npm in {jsdir} (first use; " + "this contacts the npm registry)" + ) + done = _run( + [npm, "install", "--no-audit", "--no-fund"], + timeout=INSTALL_TIMEOUT_SECONDS, + cwd=jsdir, + ) + if done is None: + return False + _forward(done.stdout) + _forward(done.stderr) + if done.returncode != 0 or not _dependencies_installed(jsdir): + _log( + f"npm install failed (exit {done.returncode}); using the pure-Python extractor" + ) + return False + return True + + +def _find_node() -> str | None: + """A Node.js ready to run `ExtractArticle.js`, installing its dependencies + if they are missing, or None (the reason is logged).""" + node = shutil.which("node") + if node is None: + _log("Node.js was not found on PATH; using the pure-Python extractor") + return None + done = _run([node, "-v"], timeout=VERSION_TIMEOUT_SECONDS) + major = _node_major(done.stdout) if done and done.returncode == 0 else None + if major is None or major < MIN_NODE_MAJOR: + _log( + f"Node.js >= {MIN_NODE_MAJOR} is needed for Readability.js " + f"({node} did not report one); using the pure-Python extractor" + ) + return None + jsdir = javascript_dir() + if not _dependencies_installed(jsdir) and not _install_dependencies(jsdir): + return None + return node + + +def node_path() -> str | None: + """The Node.js to run Readability.js with, or None to use pure Python. + + Probed once per process, under a lock, so concurrent first calls run one + `npm install` between them. A failed probe is not retried until the server + restarts, so a missing or broken Node.js costs one probe, not one per call. + """ + global _probed, _node + with _probe_lock: + if not _probed: + _node = _find_node() + _probed = True + return _node + + +def extract_article(html: str) -> str | None: + """Readability.js's article HTML for ``html`` ("" when it finds none), or + None when Readability.js is unavailable or failed, to fall back on.""" + node = node_path() + if node is None: + return None + jsdir = javascript_dir() + with tempfile.TemporaryDirectory( + prefix="mcp-server-fetch-", ignore_cleanup_errors=True + ) as tmp: + page = Path(tmp, "page.html") + result = Path(tmp, "article.json") + page.write_text(html, encoding="utf-8") + done = _run( + [ + node, + str(jsdir / "ExtractArticle.js"), + "-i", + str(page), + "-o", + str(result), + ], + timeout=EXTRACT_TIMEOUT_SECONDS, + cwd=jsdir, + ) + if done is None: + _log( + "Readability.js was stopped; using the pure-Python extractor for this " + "page. Start the server with --no-readability-js to never use it." + ) + return None + if done.returncode != 0 or not result.is_file(): + _forward(done.stderr) + _log( + f"Readability.js failed (exit {done.returncode}); using the " + "pure-Python extractor for this page" + ) + return None + try: + article = json.loads(result.read_text(encoding="utf-8")) + except ValueError: + _log( + "Readability.js wrote unreadable output; using the pure-Python extractor" + ) + return None + # `Readability.parse()` returns null for a page with no article. + content = article.get("content") if isinstance(article, dict) else None + return content if isinstance(content, str) else "" diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index a36c5a2547..7315a65ce5 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -31,6 +31,7 @@ import httpx import markdownify import readabilipy.simple_json +from .readability_js import extract_article from bs4 import BeautifulSoup, Tag from mcp.server import MCPServer, ServerRequestContext from mcp.server.context import CallNext, HandlerResult, ServerMiddleware @@ -168,11 +169,15 @@ async def _refuse_private_destination(request: httpx.Request) -> None: ) -def _readability_content(html: str, use_readability_js: bool) -> str: - """readabilipy's simplified HTML for ``html``, or "" when it has none.""" +def _readability_content(html: str) -> str: + """readabilipy's pure-Python simplified HTML for ``html``, or "" when it has none. + + Readability.js is not run through readabilipy (see `readability_js`), so + this only ever asks it for its pure-Python extractor, which starts no process. + """ try: ret = readabilipy.simple_json.simple_json_from_html_string( - html, use_readability=use_readability_js + html, use_readability=False ) except IndexError: # readabilipy's pure-Python extractor wraps its simplified tree with @@ -266,8 +271,12 @@ def _recover_hidden_content(html: str, content: str) -> str: if not _hiding_declaration(d) ] tag["style"] = ";".join(style) - recovered = _readability_content(str(soup), use_readability_js=True) - if _text_length(BeautifulSoup(recovered, "html.parser")) > extracted: + recovered = extract_article(str(soup)) + # None: this second run failed or timed out, so keep the first result. + if ( + recovered is not None + and _text_length(BeautifulSoup(recovered, "html.parser")) > extracted + ): return recovered return content @@ -277,19 +286,26 @@ def extract_content_from_html(html: str, use_readability_js: bool = True) -> str Args: html: Raw HTML content to process - use_readability_js: Ask readabilipy for Readability.js, which it runs - under Node when Node is on PATH. False always uses its pure-Python - extractor, so no Node (or npm) process is started (#4199). + use_readability_js: Use Readability.js under Node.js when Node.js is + on PATH (`readability_js`), falling back to readabilipy's + pure-Python extractor when it is unavailable, fails or times out. + False always uses the pure-Python extractor, so no Node.js (or + npm) process is started (#4199). + + Synchronous and possibly slow (a Node.js subprocess, up to its timeout): + `fetch_url` runs it in a worker thread, off the event loop (#5306). Returns: Simplified markdown version of the content """ failed = "Page failed to be simplified from HTML" - content_html = _readability_content(html, use_readability_js) - if use_readability_js: + content_html = extract_article(html) if use_readability_js else None + if content_html is not None: # Only Readability.js drops hidden elements; the pure-Python extractor # keeps them, so its result never needs recovering. content_html = _recover_hidden_content(html, content_html) + else: + content_html = _readability_content(html) if not content_html: return failed content = markdownify.markdownify( @@ -582,7 +598,10 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: ) if is_page_html and not force_raw: - return extract_content_from_html(page_raw, use_readability_js), "" + content = await asyncio.to_thread( + extract_content_from_html, page_raw, use_readability_js + ) + return content, "" # A response with no Content-Type would otherwise leave an empty slot here. declared_type = content_type or "(not declared)" @@ -774,8 +793,9 @@ async def serve( proxy_url: Optional proxy URL to use for requests allow_private_ips: Allow fetching private, loopback and link-local addresses, which are refused by default - use_readability_js: Extract HTML with Readability.js under Node when - Node is on PATH; False always uses the pure-Python extractor + use_readability_js: Extract HTML with Readability.js under Node.js + when Node.js is on PATH (falling back to the pure-Python extractor + when it cannot run); False always uses the pure-Python extractor """ server = create_server( custom_user_agent, diff --git a/src/fetch/tests/harness.py b/src/fetch/tests/harness.py index 2d566a62a9..f54d88a21f 100644 --- a/src/fetch/tests/harness.py +++ b/src/fetch/tests/harness.py @@ -27,7 +27,6 @@ import anyio import httpx import pytest -import readabilipy.simple_json from mcp import ClientSession from mcp.shared.memory import create_client_server_memory_streams from mcp.shared.message import SessionMessage @@ -40,6 +39,7 @@ ) import mcp_server_fetch.server as server_module +from mcp_server_fetch import readability_js Responder = Union[httpx.Response, Callable[[httpx.Request], httpx.Response]] @@ -132,31 +132,35 @@ async def fake_stdio_server() -> AsyncIterator[Any]: @pytest.fixture def node_readability() -> None: - """Require the Readability.js path that readabilipy takes when Node is on PATH. + """Require the Readability.js path the server takes when Node is on PATH. - readabilipy silently falls back to a pure-Python extractor without Node, - and the two produce different Markdown, so each HTML test names the mode - it pins. Node is a prerequisite of this repository's gate, so a missing - Node fails the test rather than skipping it. + The server silently falls back to readabilipy's pure-Python extractor + without Node, and the two produce different Markdown, so each HTML test + names the mode it pins. Node is a prerequisite of this repository's gate, + so a missing Node fails the test rather than skipping it. """ - if not readabilipy.simple_json.have_node(): - pytest.fail("Node.js >= 10 must be on PATH for the Readability.js path") + if readability_js.node_path() is None: + pytest.fail( + f"Node.js >= {readability_js.MIN_NODE_MAJOR} (and npm, or readabilipy's " + "JavaScript dependencies) must be on PATH for the Readability.js path" + ) @pytest.fixture def python_readability(monkeypatch: pytest.MonkeyPatch) -> None: - """Force readabilipy's pure-Python fallback, as on a machine without Node.""" - monkeypatch.setattr(readabilipy.simple_json, "have_node", lambda: False) + """Force the pure-Python fallback, as on a machine without Node.""" + monkeypatch.setattr(readability_js, "node_path", lambda: None) @pytest.fixture(autouse=True) def private_tempdir(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: """Give each test its own ``tempfile.gettempdir()``. - readabilipy's Readability.js path writes its input and output to fixed + readabilipy 0.2's Readability.js path wrote its input and output to fixed names (``full.html``, ``article.json``) in the shared temp directory, so two test runs at once (parallel gate runs in separate worktrees, say) read - each other's pages. + each other's pages. The server's own runner uses a fresh directory per + page, so this now only keeps each test's temporary files to itself. """ monkeypatch.setattr(tempfile, "tempdir", str(tmp_path)) diff --git a/src/fetch/tests/test_empty_tree.py b/src/fetch/tests/test_empty_tree.py index 0c13586f58..6835e49d77 100644 --- a/src/fetch/tests/test_empty_tree.py +++ b/src/fetch/tests/test_empty_tree.py @@ -73,7 +73,9 @@ def html(body: str) -> httpx.Response: @pytest.mark.parametrize("page", [SPA_SHELL, "", ""]) def test_emulation_reproduces_the_reported_crash(page: str) -> None: with pytest.raises(IndexError, match="list index out of range"): - readabilipy.simple_json.simple_json_from_html_string(page, use_readability=True) + readabilipy.simple_json.simple_json_from_html_string( + page, use_readability=False + ) @pytest.mark.usefixtures("python_readability", "bs4_413_append") diff --git a/src/fetch/tests/test_hidden_ssr.py b/src/fetch/tests/test_hidden_ssr.py index 5834564de0..4bdf338997 100644 --- a/src/fetch/tests/test_hidden_ssr.py +++ b/src/fetch/tests/test_hidden_ssr.py @@ -12,7 +12,7 @@ ``hidden``; the ``@mozilla/readability`` that readabilipy 0.3 installs also drops ``visibility: hidden`` and ``aria-hidden="true"``. The protocol tests that prove the second run happens use the two hidings both versions drop. They -count readabilipy calls, so "a normal page is unchanged" is asserted as "it +count extractor calls, so "a normal page is unchanged" is asserted as "it was extracted once, as before", not inferred from the Markdown. """ @@ -26,6 +26,7 @@ from bs4 import BeautifulSoup from mcp.types import CallToolResult +import mcp_server_fetch.server as server_module from mcp_server_fetch.server import _is_hidden from .harness import FakeWeb, connect @@ -49,15 +50,28 @@ def ssr_page(hiding: str) -> str: @pytest.fixture def calls(monkeypatch: pytest.MonkeyPatch) -> list[str]: - """Record every HTML string readabilipy is asked to simplify.""" + """Record every HTML string an extractor is asked to simplify. + + Both extractors: the server's Readability.js runner and readabilipy's + pure-Python extractor, which it falls back to. + """ seen: list[str] = [] - original = readabilipy.simple_json.simple_json_from_html_string + original_js = server_module.extract_article + original_py = readabilipy.simple_json.simple_json_from_html_string + + def spy_js(html: str) -> Any: + result = original_js(html) + # None: Readability.js did not run (no Node), so nothing was extracted. + if result is not None: + seen.append(html) + return result - def spy(html: str, **kwargs: Any) -> Any: + def spy_py(html: str, **kwargs: Any) -> Any: seen.append(html) - return original(html, **kwargs) + return original_py(html, **kwargs) - monkeypatch.setattr(readabilipy.simple_json, "simple_json_from_html_string", spy) + monkeypatch.setattr(server_module, "extract_article", spy_js) + monkeypatch.setattr(readabilipy.simple_json, "simple_json_from_html_string", spy_py) return seen diff --git a/src/fetch/tests/test_readability_js_opt_out.py b/src/fetch/tests/test_readability_js_opt_out.py index 05cb77d89b..cd3911ebb7 100644 --- a/src/fetch/tests/test_readability_js_opt_out.py +++ b/src/fetch/tests/test_readability_js_opt_out.py @@ -1,15 +1,14 @@ """Tests for ``--no-readability-js`` (#4199): an operator opt-out from Node.js. -readabilipy runs Mozilla's Readability.js under Node whenever Node is on PATH -(and readabilipy 0.3 may first run ``npm install``), in a subprocess with no -timeout. A Node or npm that hangs, or a deployment that must not start them, -had no remedy short of editing the installed package. With the flag the -server asks readabilipy for its pure-Python extractor, which never probes for -Node at all. - -The opt-out tests replace readabilipy's ``subprocess`` module with one that -fails on any use, so they prove no process is started rather than inferring -it from the Markdown that comes back. The default-path test needs Node on PATH, +The server runs Mozilla's Readability.js under Node whenever Node is on PATH +(and may first run ``npm install``, see ``readability_js``). A deployment that +must not start them can opt out: with the flag the server uses readabilipy's +pure-Python extractor and never probes for Node at all. + +The opt-out tests replace the ``subprocess`` module of both the server's +Readability.js runner and readabilipy with one that fails on any use, so they +prove no process is started rather than inferring it from the Markdown that +comes back. The default-path test needs Node on PATH, like the repository's other Readability.js tests. """ @@ -25,6 +24,8 @@ from mcp.types import CallToolResult import mcp_server_fetch +import mcp_server_fetch.server as server_module +from mcp_server_fetch import readability_js from .harness import FakeWeb, connect @@ -38,16 +39,20 @@ class NoProcesses: - """Stands in for readabilipy's ``subprocess`` module: any use fails the test.""" + """Stands in for a ``subprocess`` module: any use fails the test.""" def __getattr__(self, name: str) -> Any: - raise AssertionError(f"readabilipy used subprocess.{name}") + raise AssertionError(f"a process was started: subprocess.{name}") @pytest.fixture def no_processes(monkeypatch: pytest.MonkeyPatch) -> None: - # Both readabilipy's Node probe (`have_node`) and its Readability.js call - # go through this module attribute, as does 0.3's `npm install`. + # The server's Node probe, its `npm install` and its Readability.js run + # all go through `readability_js.subprocess`; readabilipy's own Node path + # (which the server no longer takes) goes through its module's. The probe + # cache is cleared so a probe made by an earlier test cannot hide one. + monkeypatch.setattr(readability_js, "_probed", False) + monkeypatch.setattr(readability_js, "subprocess", NoProcesses()) monkeypatch.setattr(readabilipy.simple_json, "subprocess", NoProcesses()) @@ -65,10 +70,8 @@ def html(body: str) -> httpx.Response: def test_no_processes_fixture_catches_the_node_probe(no_processes: None) -> None: # The guard the opt-out tests rely on does fire on the default path. - with pytest.raises(AssertionError, match="readabilipy used subprocess.run"): - readabilipy.simple_json.simple_json_from_html_string( - ARTICLE, use_readability=True - ) + with pytest.raises(AssertionError, match="a process was started: subprocess"): + server_module.extract_content_from_html(ARTICLE) def test_flag_is_passed_to_serve(monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/src/fetch/tests/test_readability_subprocess.py b/src/fetch/tests/test_readability_subprocess.py new file mode 100644 index 0000000000..bda870527f --- /dev/null +++ b/src/fetch/tests/test_readability_subprocess.py @@ -0,0 +1,457 @@ +"""Tests for the server's own Readability.js runner (``readability_js``). + +It replaces readabilipy's Node.js wrapper, which had no timeout and blocked +the event loop (#5306), wrote ``npm install`` output to stdout, the stdio +JSON-RPC channel (#5310), and could not find ``npm.cmd`` on Windows (#5313). + +Most tests put fake ``node`` and ``npm`` shell scripts first on ``PATH`` (the +reproduction #5306 describes), point the runner at a scratch copy of +readabilipy's ``javascript`` directory, and clear its once-per-process probe, +so no real Node.js runs and nothing reaches the network. Shell scripts need a +POSIX shell, so those tests skip on Windows; there the ``node_readability`` +tests in the other files drive the real ``npm.cmd`` (the Windows CI leg no +longer installs readabilipy's JavaScript for them). +""" + +from __future__ import annotations + +import os +import stat +import subprocess +import sys +import time +from pathlib import Path +from typing import Any, cast + +import anyio +import httpx +import pytest + +import mcp_server_fetch.server as server_module +from mcp_server_fetch import readability_js + +from .harness import FakeWeb, connect + +PAGE = "https://example.com/article" + +ARTICLE = """Article
+

Heading

+

This is a link in a paragraph with +enough words to count as the main content of the page.

+
""" + +posix_only = pytest.mark.skipif( + sys.platform == "win32", reason="the fake node and npm are POSIX shell scripts" +) + +# A `node` that reports a version and then does what each test's body says +# with ExtractArticle.js's arguments: "$1" is the script, "$3" the input +# page and "$5" the output JSON. +NODE_VERSION = 'if [ "$1" = "-v" ]; then echo v24.0.0; exit 0; fi\n' + + +def html(body: str) -> httpx.Response: + return httpx.Response( + 200, content=body.encode(), headers={"content-type": "text/html"} + ) + + +@pytest.fixture +def bindir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """An empty directory that is the first entry on PATH, ahead of system tools. + + The real Node.js and npm (in /usr/local/bin and the like) are left off, so + only the fakes a test writes here are found. + """ + path = tmp_path / "bin" + path.mkdir() + monkeypatch.setenv("PATH", os.pathsep.join([str(path), "/usr/bin", "/bin"])) + return path + + +def tool(bindir: Path, name: str, body: str) -> Path: + script = bindir / name + script.write_text("#!/bin/sh\n" + body) + script.chmod(script.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + return script + + +@pytest.fixture +def jsdir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A scratch readabilipy `javascript` directory, with a fresh probe.""" + path = tmp_path / "javascript" + path.mkdir() + monkeypatch.setattr(readability_js, "javascript_dir", lambda: path) + monkeypatch.setattr(readability_js, "_probed", False) + monkeypatch.setattr(readability_js, "_node", None) + return path + + +def install(jsdir: Path) -> None: + """Lay out node_modules as a completed `npm install` would.""" + for name in readability_js.JS_DEPENDENCIES: + package = jsdir / "node_modules" / name + package.mkdir(parents=True) + (package / "package.json").write_text("{}") + + +def alive(pid: int) -> bool: + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + return True + + +def wait_dead(pid: int) -> bool: + # A killed process is reaped by its parent; give the kernel a moment. + deadline = time.monotonic() + 5 + while alive(pid): + if time.monotonic() > deadline: # pragma: no cover # only on failure + return False + time.sleep(0.05) + return True + + +# --- #5306: a bounded run, killed on timeout, off the event loop ------------ + + +@posix_only +async def test_hanging_node_is_killed_and_the_call_falls_back( + web: FakeWeb, bindir: Path, jsdir: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The reproduction from #5306: a node that never exits. It also starts a + # child of its own, which the timeout must kill as well. + install(jsdir) + pids = bindir / "pids" + tool( + bindir, + "node", + NODE_VERSION + f'sleep 3600 &\necho "$$ $!" > {pids}\nwait\n', + ) + monkeypatch.setattr(readability_js, "EXTRACT_TIMEOUT_SECONDS", 1.0) + web.add(PAGE, html(ARTICLE)) + started = time.monotonic() + async with connect(ignore_robots_txt=True) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE}) + assert time.monotonic() - started < 10 + assert result.is_error is False + [content] = result.content + assert content.type == "text" + # The pure-Python extractor's Markdown: it drops link targets. + assert "This is a link in a paragraph" in content.text + assert "https://example.com/x" not in content.text + for pid in map(int, pids.read_text().split()): + assert wait_dead(pid), f"process {pid} outlived the timeout" + + +@posix_only +async def test_extraction_does_not_block_other_requests( + web: FakeWeb, bindir: Path, jsdir: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + install(jsdir) + started = bindir / "started" + tool(bindir, "node", NODE_VERSION + f"touch {started}\nexec sleep 3600\n") + monkeypatch.setattr(readability_js, "EXTRACT_TIMEOUT_SECONDS", 5.0) + web.add(PAGE, html(ARTICLE)) + async with connect(ignore_robots_txt=True) as (session, _): + fetched: list[float] = [] + listed = float("inf") + + async def call() -> None: + await session.call_tool("fetch", {"url": PAGE}) + fetched.append(time.monotonic()) + + async with anyio.create_task_group() as tg: + tg.start_soon(call) + while not started.exists(): + await anyio.sleep(0.05) + # Node.js is running (and hung); the loop still answers. + tools = await session.list_tools() + listed = time.monotonic() + assert [t.name for t in tools.tools] == ["fetch"] + # The fetch finished (by falling back) only after the timeout, well + # after the listing was answered. + assert listed < fetched[0] - 1 + + +# --- #5310: nothing a child prints reaches stdout --------------------------- + + +@posix_only +def test_npm_install_output_goes_to_stderr_not_stdout( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + node = tool(bindir, "node", NODE_VERSION) + # npm prints to both streams, as the real one does, and installs. + deps = " ".join(f"{jsdir}/node_modules/{n}" for n in readability_js.JS_DEPENDENCIES) + tool( + bindir, + "npm", + "echo 'added 38 packages, and audited 39 packages in 470ms'\n" + "echo 'npm warn deprecated' >&2\n" + f"for d in {deps}; do mkdir -p $d && echo '{{}}' > $d/package.json; done\n", + ) + assert readability_js.node_path() == str(node) + out, err = capfd.readouterr() + assert out == "" + assert "added 38 packages" in err + assert "npm warn deprecated" in err + assert "contacts the npm registry" in err + + +@posix_only +def test_failed_extraction_stderr_goes_to_stderr_not_stdout( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + install(jsdir) + tool( + bindir, + "node", + NODE_VERSION + "echo 'on stdout'\necho 'TypeError: x' >&2\nexit 1\n", + ) + assert readability_js.extract_article(ARTICLE) is None + out, err = capfd.readouterr() + assert out == "" + assert "TypeError: x" in err + assert "Readability.js failed (exit 1)" in err + + +@posix_only +def test_children_do_not_read_the_servers_stdin( + bindir: Path, jsdir: Path, tmp_path: Path +) -> None: + # A child given the server's stdin could consume JSON-RPC input. + seen = tmp_path / "stdin" + script = tool(bindir, "reader", f"cat > {seen}\n") + r, w = os.pipe() + saved = os.dup(0) + try: + os.dup2(r, 0) + os.write(w, b"not for the child\n") + os.close(w) + done = readability_js._run([str(script)], timeout=5) + finally: + os.dup2(saved, 0) + os.close(saved) + os.close(r) + assert done is not None and done.returncode == 0 + assert seen.read_text() == "" + + +# --- #5313: node and npm are resolved through PATH (and PATHEXT) ------------ + + +def test_npm_is_run_by_the_path_shutil_which_resolves( + jsdir: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # On Windows `shutil.which("npm")` finds `npm.cmd`, which a bare "npm" + # passed to CreateProcess does not; whatever it finds is what runs. + found = {"node": r"C:\nodejs\node.exe", "npm": r"C:\nodejs\npm.CMD"} + monkeypatch.setattr(readability_js.shutil, "which", found.get) + runs: list[list[str]] = [] + + def run(argv: list[str], timeout: float, cwd: Path | None = None) -> Any: + runs.append(argv) + if argv[1] == "install": + install(jsdir) + return subprocess.CompletedProcess(argv, 0, "v24.0.0\n", "") + + monkeypatch.setattr(readability_js, "_run", run) + assert readability_js.node_path() == found["node"] + assert runs == [ + [found["node"], "-v"], + [found["npm"], "install", "--no-audit", "--no-fund"], + ] + + +# --- Each way Readability.js can be unavailable falls back ------------------- + + +@posix_only +def test_no_node_on_path( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + assert readability_js.extract_article(ARTICLE) is None + assert "Node.js was not found on PATH" in capfd.readouterr().err + + +@posix_only +@pytest.mark.parametrize( + "body", ["echo v12.22.0\n", "echo garbage\n", "exit 3\n", "exec sleep 3600\n"] +) +def test_old_or_broken_node( + bindir: Path, + jsdir: Path, + body: str, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setattr(readability_js, "VERSION_TIMEOUT_SECONDS", 0.5) + tool(bindir, "node", body) + assert readability_js.node_path() is None + assert "Node.js >= 14 is needed" in capfd.readouterr().err + + +@posix_only +def test_no_npm_for_missing_dependencies( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + tool(bindir, "node", NODE_VERSION) + assert readability_js.node_path() is None + assert "npm was not found on PATH" in capfd.readouterr().err + + +@posix_only +def test_failed_npm_install( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + tool(bindir, "node", NODE_VERSION) + tool(bindir, "npm", "echo 'npm error network ENOTFOUND' >&2\nexit 1\n") + assert readability_js.node_path() is None + err = capfd.readouterr().err + assert "npm error network ENOTFOUND" in err + assert "npm install failed (exit 1)" in err + + +@posix_only +def test_hanging_npm_install_is_killed( + bindir: Path, + jsdir: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setattr(readability_js, "INSTALL_TIMEOUT_SECONDS", 0.5) + tool(bindir, "node", NODE_VERSION) + tool(bindir, "npm", "exec sleep 3600\n") + assert readability_js.node_path() is None + assert "npm did not finish within 0.5 s and was killed" in capfd.readouterr().err + + +@posix_only +def test_probe_runs_once_per_process(bindir: Path, jsdir: Path) -> None: + calls = bindir / "calls" + node = tool(bindir, "node", f"echo x >> {calls}\n" + NODE_VERSION) + install(jsdir) + assert readability_js.node_path() == str(node) + assert readability_js.node_path() == str(node) + assert calls.read_text() == "x\n" + + +@posix_only +def test_unstartable_node( + bindir: Path, jsdir: Path, capfd: pytest.CaptureFixture[str] +) -> None: + # Found on PATH but not executable as a program (no interpreter line). + script = bindir / "node" + script.write_bytes(b"\x00\x01") + script.chmod(0o755) + assert readability_js.node_path() is None + assert "could not start" in capfd.readouterr().err + + +@posix_only +@pytest.mark.parametrize( + ("body", "expected"), + [ + # Readability.parse() found no article. + ('echo null > "$5"\n', ""), + ('echo \'{"content": null}\' > "$5"\n', ""), + ( + 'echo \'{"content": "

hi

"}\' > "$5"\n', + "

hi

", + ), + # Exit 0 but no output file: a failure, so fall back. + ("exit 0\n", None), + # Output that is not JSON. + ('echo "{" > "$5"\n', None), + ], +) +def test_extraction_outcomes( + bindir: Path, jsdir: Path, body: str, expected: str | None +) -> None: + install(jsdir) + tool(bindir, "node", NODE_VERSION + body) + assert readability_js.extract_article(ARTICLE) == expected + + +@posix_only +def test_extraction_gets_the_page_and_runs_extractarticle( + bindir: Path, jsdir: Path, tmp_path: Path +) -> None: + install(jsdir) + copy = tmp_path / "seen.html" + args = tmp_path / "args" + tool( + bindir, + "node", + NODE_VERSION + + f'echo "$1" > {args}\ncp "$3" {copy}\n' + + 'echo \'{"content": "

ok

"}\' > "$5"\n', + ) + assert readability_js.extract_article("

é

") == "

ok

" + assert copy.read_text(encoding="utf-8") == "

é

" + assert args.read_text().strip() == str(jsdir / "ExtractArticle.js") + + +# --- Killing the process tree ------------------------------------------------ + + +def test_kill_tree_uses_taskkill_on_windows(monkeypatch: pytest.MonkeyPatch) -> None: + runs: list[list[str]] = [] + killed: list[bool] = [] + + class Proc: + pid = 4242 + + def kill(self) -> None: + killed.append(True) + + monkeypatch.setattr(readability_js.sys, "platform", "win32") + monkeypatch.setattr( + readability_js.subprocess, "run", lambda argv, **_: runs.append(argv) + ) + readability_js._kill_tree(cast("subprocess.Popen[str]", Proc())) + assert runs == [["taskkill", "/F", "/T", "/PID", "4242"]] + assert killed == [True] + + +@posix_only +def test_kill_tree_tolerates_a_group_already_gone( + monkeypatch: pytest.MonkeyPatch, +) -> None: + proc = subprocess.Popen(["true"], start_new_session=True) + proc.wait() + + def gone(pid: int, sig: int) -> None: + raise ProcessLookupError + + monkeypatch.setattr(readability_js.os, "killpg", gone) + readability_js._kill_tree(cast("subprocess.Popen[str]", proc)) + + +# --- The server's use of the runner ----------------------------------------- + + +def test_a_failed_second_run_keeps_the_first_result( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # #3878's recovery re-runs Readability.js; if that run fails (None), the + # first run's content is returned rather than nothing. + page = ( + "

short shell

" + ) + results = iter(["

short shell

", None]) + monkeypatch.setattr(server_module, "extract_article", lambda _: next(results)) + assert server_module.extract_content_from_html(page).strip() == "short shell" + + +def test_unavailable_readability_js_falls_back_to_python( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(server_module, "extract_article", lambda _: None) + text = server_module.extract_content_from_html(ARTICLE) + assert "This is a link in a paragraph" in text + assert "https://example.com/x" not in text From 533b9a3ae31f8896d2a402bea1b914e008e57b7a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:26:31 -0400 Subject: [PATCH 05/14] feat(fetch): add an operator host allowlist, --allowed-hosts With --allowed-hosts, every request the server sends (the tool's page, the prompt's page, each robots.txt check, and every redirect hop of any of them) must name a listed host, or it is refused before it is sent. An entry is an exact host (a name or an IP literal) or *.example.com for any subdomain of example.com. The list is checked by name, alongside the private-address guard rather than in place of it: listing an internal host still needs --allow-private-ips. Without the flag nothing changes. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/README.md | 32 ++ src/fetch/src/mcp_server_fetch/__init__.py | 11 + src/fetch/src/mcp_server_fetch/allowlist.py | 101 ++++++ src/fetch/src/mcp_server_fetch/server.py | 26 +- src/fetch/tests/test_allowed_hosts.py | 305 ++++++++++++++++++ src/fetch/tests/test_entrypoints.py | 6 +- .../tests/test_readability_js_opt_out.py | 2 +- 7 files changed, 475 insertions(+), 8 deletions(-) create mode 100644 src/fetch/src/mcp_server_fetch/allowlist.py create mode 100644 src/fetch/tests/test_allowed_hosts.py diff --git a/src/fetch/README.md b/src/fetch/README.md index 09b4e8b5d8..abcbdc900f 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -188,6 +188,38 @@ The client receives an error naming the refused address. To fetch from local or internal hosts (a development server, an intranet page), add the argument `--allow-private-ips` to the `args` list in the configuration. +### Customization - Allowed hosts + +By default the server may fetch from any host (subject to the private-address guard above). To restrict it to the +hosts you name, add `--allowed-hosts` followed by one or more entries to the `args` list (the argument can also be +repeated). Each entry is one of: + +- an exact host name, such as `docs.python.org`, which matches that host only, not its subdomains; +- `*.` followed by a domain, such as `*.github.com`, which matches every subdomain of it at any depth + (`api.github.com`, `a.b.github.com`) but not `github.com` itself: list both to allow both; +- an IP address, such as `203.0.113.7` or `2001:db8::7`, which matches a URL that uses that address literally. + +Matching ignores case and a trailing dot, and an internationalized name may be given in either its Unicode or its +`xn--` form. An entry names a host only, so it carries no scheme, port or path. The list is checked before every request +the server sends, by the host name in the URL rather than by the addresses it resolves to: the requested page, its +robots.txt check, and every redirect hop of either, from the tool and from the prompt alike. A request to any other host +is refused before it is sent, and the client receives an error naming the refused host. + +The allowlist and the private-address guard are separate checks, and a request must pass both. Listing an internal +host, such as `localhost` or `10.0.0.5`, does not by itself let the server reach it: that also needs +`--allow-private-ips`, which in turn does not lift the allowlist. + +```json +{ + "mcpServers": { + "fetch": { + "command": "uvx", + "args": ["mcp-server-fetch", "--allowed-hosts", "docs.python.org", "*.wikipedia.org"] + } + } +} +``` + ### Customization - User-agent By default, depending on if the request came from the model (via a tool), or was user initiated (via a prompt), the diff --git a/src/fetch/src/mcp_server_fetch/__init__.py b/src/fetch/src/mcp_server_fetch/__init__.py index 39ac728705..921c965bec 100644 --- a/src/fetch/src/mcp_server_fetch/__init__.py +++ b/src/fetch/src/mcp_server_fetch/__init__.py @@ -1,3 +1,4 @@ +from .allowlist import parse_allowed_host from .server import serve @@ -21,6 +22,15 @@ def main(): action="store_true", help="Allow fetching private, loopback and link-local addresses (refused by default)", ) + parser.add_argument( + "--allowed-hosts", + nargs="+", + action="extend", + type=parse_allowed_host, + metavar="HOST", + help="Only fetch from these hosts: an exact host name or IP address, or " + "*.example.com for any subdomain of example.com (all hosts by default)", + ) parser.add_argument( "--no-readability-js", action="store_true", @@ -35,6 +45,7 @@ def main(): args.ignore_robots_txt, args.proxy_url, args.allow_private_ips, + args.allowed_hosts, not args.no_readability_js, ) ) diff --git a/src/fetch/src/mcp_server_fetch/allowlist.py b/src/fetch/src/mcp_server_fetch/allowlist.py new file mode 100644 index 0000000000..102a87fba5 --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/allowlist.py @@ -0,0 +1,101 @@ +# The fetch server's operator-configured host allowlist (#2317). With +# `--allowed-hosts`, every request the server sends (the tool's page, the +# prompt's page, each robots.txt check and every redirect hop of any of them) +# must name a listed host, or it is refused before it is sent. Without the flag +# nothing is restricted, so existing configurations keep working. +# +# The allowlist names hosts, and it is checked against the host the request +# connects to (`raw_host`, the ASCII form), never against resolved addresses: +# a name the operator chose is trusted by name. It is a separate check from +# the private-address guard in server.py, and both apply: listing `localhost` +# or an intranet name does not, by itself, let the server reach a private +# address; that still needs `--allow-private-ips`. Two entry forms only, to +# keep the semantics obvious: an exact host (a name or an IP literal), and +# `*.example.com`, which matches any subdomain at any depth of example.com but +# not example.com itself. Ports and schemes are not part of an entry. + +import argparse +import ipaddress +from collections.abc import Awaitable, Callable, Iterable + +import httpx +from mcp.server.mcpserver.exceptions import ToolError + + +def normalize_allowed_host(entry: str) -> str: + """The canonical form of one ``--allowed-hosts`` entry, or ``ValueError``. + + A name is lowercased, its trailing dot dropped, and IDNA-encoded to the + ASCII form requests are checked in (``bücher.example`` becomes + ``xn--bcher-kva.example``). An IP literal is written in its compressed + form, an IPv6 one with or without brackets. A wildcard is ``*.`` followed + by a name. + """ + value = entry.strip().rstrip(".").lower() + wildcard = value.startswith("*.") + name = value[2:] if wildcard else value + if not wildcard: + try: + return ipaddress.ip_address(name.strip("[]")).compressed + except ValueError: + pass + if not name or any(c in name for c in "*/\\:@[]?#% ") or ".." in name: + raise ValueError( + f"{entry!r} is not a host name, an IP address or *. " + "(no scheme, port, path or other wildcard)" + ) + try: + ascii_name = httpx.URL(f"http://{name}/").raw_host.decode("ascii") + except httpx.InvalidURL as e: + raise ValueError(f"{entry!r} is not a valid host name: {e}") from None + return f"*.{ascii_name}" if wildcard else ascii_name + + +def parse_allowed_host(entry: str) -> str: + """``normalize_allowed_host`` as an argparse ``type``, with its message.""" + try: + return normalize_allowed_host(entry) + except ValueError as e: + raise argparse.ArgumentTypeError(str(e)) from None + + +def normalize_allowed_hosts(entries: Iterable[str] | None) -> tuple[str, ...]: + """Every entry normalized; an empty tuple means no allowlist.""" + return tuple(normalize_allowed_host(entry) for entry in entries or ()) + + +def host_is_allowed(host: str, allowed_hosts: tuple[str, ...]) -> bool: + """Whether ``host`` (a request's ``raw_host``) matches a normalized entry.""" + host = host.rstrip(".").lower() + try: + host = ipaddress.ip_address(host).compressed + except ValueError: + pass + for entry in allowed_hosts: + if entry.startswith("*."): + if host.endswith(entry[1:]): + return True + elif host == entry: + return True + return False + + +def refuse_unlisted_destination( + allowed_hosts: tuple[str, ...], +) -> Callable[[httpx.Request], Awaitable[None]]: + """An httpx request hook refusing any request to a host not on the list. + + httpx runs request hooks before every request it sends, redirect hops + included, so a listed host cannot redirect the fetch to an unlisted one. + The error does not repeat the list, which is the operator's configuration. + """ + + async def hook(request: httpx.Request) -> None: + host = request.url.raw_host.decode("ascii") + if not host_is_allowed(host, allowed_hosts): + raise ToolError( + f"Refused to fetch {request.url}: {host} is not one of the " + "hosts this server is allowed to fetch from (--allowed-hosts)." + ) + + return hook diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index a36c5a2547..56d914f200 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -23,7 +23,7 @@ import ipaddress import os import socket -from collections.abc import Awaitable, Callable, Mapping +from collections.abc import Awaitable, Callable, Mapping, Sequence from typing import Annotated, Any, Tuple from importlib.metadata import version from urllib.parse import urlparse, urlunparse @@ -32,6 +32,7 @@ import markdownify import readabilipy.simple_json from bs4 import BeautifulSoup, Tag +from .allowlist import normalize_allowed_hosts, refuse_unlisted_destination from mcp.server import MCPServer, ServerRequestContext from mcp.server.context import CallNext, HandlerResult, ServerMiddleware from mcp.server.mcpserver.exceptions import ToolError @@ -461,6 +462,7 @@ async def check_may_autonomously_fetch_url( user_agent: str, proxy_url: str | None = None, allow_private_ips: bool = False, + allowed_hosts: tuple[str, ...] = (), ) -> None: """ Check if the URL can be fetched by the user agent according to the robots.txt file. @@ -471,8 +473,12 @@ async def check_may_autonomously_fetch_url( robot_txt_url = get_robots_txt_url(url) async with make_client(proxy_url) as client: + hooks: list[Callable[[httpx.Request], Awaitable[None]]] = [] + if allowed_hosts: + hooks.append(refuse_unlisted_destination(allowed_hosts)) if not allow_private_ips: - client.event_hooks = {"request": [_refuse_private_destination]} + hooks.append(_refuse_private_destination) + client.event_hooks = {"request": hooks} try: response = await client.get( robot_txt_url, @@ -525,6 +531,7 @@ async def fetch_url( force_raw: bool = False, proxy_url: str | None = None, allow_private_ips: bool = False, + allowed_hosts: tuple[str, ...] = (), check_robots_txt: bool = False, use_readability_js: bool = True, ) -> Tuple[str, str]: @@ -546,11 +553,13 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: seen_first_request = True return await check_may_autonomously_fetch_url( - str(request.url), user_agent, proxy_url, allow_private_ips + str(request.url), user_agent, proxy_url, allow_private_ips, allowed_hosts ) async with make_client(proxy_url) as client: hooks: list[Callable[[httpx.Request], Awaitable[None]]] = [] + if allowed_hosts: + hooks.append(refuse_unlisted_destination(allowed_hosts)) if not allow_private_ips: hooks.append(_refuse_private_destination) if check_robots_txt: @@ -639,6 +648,7 @@ def create_server( ignore_robots_txt: bool = False, proxy_url: str | None = None, allow_private_ips: bool = False, + allowed_hosts: Sequence[str] | None = None, use_readability_js: bool = True, ) -> MCPServer: """Build the fetch server: the `fetch` tool, the `fetch` prompt, and the guard. @@ -648,6 +658,7 @@ def create_server( if proxy_url: proxy_url = normalize_proxy_url(proxy_url) normalize_proxy_env() + allowed = normalize_allowed_hosts(allowed_hosts) user_agent_autonomous = custom_user_agent or DEFAULT_USER_AGENT_AUTONOMOUS user_agent_manual = custom_user_agent or DEFAULT_USER_AGENT_MANUAL @@ -708,7 +719,7 @@ async def fetch( if not ignore_robots_txt: await check_may_autonomously_fetch_url( - page_url, user_agent_autonomous, proxy_url, allow_private_ips + page_url, user_agent_autonomous, proxy_url, allow_private_ips, allowed ) content, prefix = await fetch_url( @@ -717,6 +728,7 @@ async def fetch( force_raw=raw, proxy_url=proxy_url, allow_private_ips=allow_private_ips, + allowed_hosts=allowed, check_robots_txt=not ignore_robots_txt, use_readability_js=use_readability_js, ) @@ -750,6 +762,7 @@ async def fetch_prompt( user_agent_manual, proxy_url=proxy_url, allow_private_ips=allow_private_ips, + allowed_hosts=allowed, use_readability_js=use_readability_js, ) except ToolError as e: @@ -764,6 +777,7 @@ async def serve( ignore_robots_txt: bool = False, proxy_url: str | None = None, allow_private_ips: bool = False, + allowed_hosts: Sequence[str] | None = None, use_readability_js: bool = True, ) -> None: """Run the fetch MCP server over stdio. @@ -774,6 +788,9 @@ async def serve( proxy_url: Optional proxy URL to use for requests allow_private_ips: Allow fetching private, loopback and link-local addresses, which are refused by default + allowed_hosts: When given, the only hosts the server may fetch from: + exact hosts, or ``*.example.com`` for any subdomain of it. Empty + or None allows every host use_readability_js: Extract HTML with Readability.js under Node when Node is on PATH; False always uses the pure-Python extractor """ @@ -782,6 +799,7 @@ async def serve( ignore_robots_txt, proxy_url, allow_private_ips, + allowed_hosts, use_readability_js, ) # Both eras (#4853): the SDK's dual-era loop lets the client's first diff --git a/src/fetch/tests/test_allowed_hosts.py b/src/fetch/tests/test_allowed_hosts.py new file mode 100644 index 0000000000..0ba647a221 --- /dev/null +++ b/src/fetch/tests/test_allowed_hosts.py @@ -0,0 +1,305 @@ +"""The operator-configured host allowlist, ``--allowed-hosts`` (#2317). + +Entry parsing and matching are unit-tested directly in ``allowlist``; the +enforcement is driven through the real server over the MCP protocol (see +``harness.connect``), so every request path is covered as a client sees it: +the tool's page, the prompt's page, the robots.txt check, and a redirect hop +of each. ``web.urls()`` proves a refused request was never sent. +""" + +from __future__ import annotations + +import argparse +from typing import Any + +import httpx +import pytest +from mcp.types import CallToolResult + +import mcp_server_fetch +from mcp_server_fetch.allowlist import ( + host_is_allowed, + normalize_allowed_host, + normalize_allowed_hosts, + parse_allowed_host, +) + +from .harness import FakeWeb, connect + +TOOL_ERROR = "Error executing tool fetch: " +PAGE = "https://docs.example.com/guide" +ROBOTS = "https://docs.example.com/robots.txt" +OTHER = "https://other.example.org/secret" +OTHER_ROBOTS = "https://other.example.org/robots.txt" + + +def refused(url: str, host: str) -> str: + return ( + f"Refused to fetch {url}: {host} is not one of the hosts this server " + "is allowed to fetch from (--allowed-hosts)." + ) + + +def plain(text: str, status: int = 200) -> httpx.Response: + return httpx.Response( + status, content=text.encode(), headers={"content-type": "text/plain"} + ) + + +def wire(result: CallToolResult) -> dict[str, Any]: + return result.model_dump( + by_alias=True, mode="json", exclude_none=True, exclude_unset=True + ) + + +def text_of(result: CallToolResult) -> str: + data = wire(result) + assert len(data["content"]) == 1 + return data["content"][0]["text"] + + +# --- entries --------------------------------------------------------------- + + +@pytest.mark.parametrize( + "entry, normalized", + [ + ("Example.COM", "example.com"), + ("example.com.", "example.com"), + (" example.com ", "example.com"), + ("*.Example.com", "*.example.com"), + ("bücher.example", "xn--bcher-kva.example"), + ("*.bücher.example", "*.xn--bcher-kva.example"), + ("10.0.0.1", "10.0.0.1"), + ("::1", "::1"), + ("[0:0::1]", "::1"), + ("localhost", "localhost"), + ], +) +def test_entries_are_normalized(entry: str, normalized: str) -> None: + assert normalize_allowed_host(entry) == normalized + + +@pytest.mark.parametrize( + "entry", + [ + "", + ".", + "*.", + "*", + "*example.com", + "a.*.example.com", + "https://example.com", + "example.com:443", + "example.com/path", + "user@example.com", + "exa mple.com", + "example.com?x", + "a..b", + "*.*.example.com", + ], +) +def test_malformed_entries_are_rejected(entry: str) -> None: + with pytest.raises(ValueError): + normalize_allowed_host(entry) + + +def test_an_entry_httpx_cannot_encode_is_rejected() -> None: + with pytest.raises(ValueError, match="is not a valid host name"): + normalize_allowed_host("☃.example.com") + + +def test_parse_allowed_host_reports_through_argparse() -> None: + assert parse_allowed_host("Example.com") == "example.com" + with pytest.raises(argparse.ArgumentTypeError, match="no scheme, port"): + parse_allowed_host("https://example.com") + + +def test_no_entries_is_no_allowlist() -> None: + assert normalize_allowed_hosts(None) == () + assert normalize_allowed_hosts([]) == () + + +@pytest.mark.parametrize( + "host, allowed", + [ + ("example.com", True), + ("EXAMPLE.com.", True), + ("www.example.com", False), + ("api.example.org", True), + ("a.b.example.org", True), + ("example.org", False), + ("badexample.org", False), + ("example.org.evil.com", False), + ("10.0.0.1", True), + ("10.0.0.2", False), + ("0:0::1", True), + ("evil.com", False), + ], +) +def test_matching(host: str, allowed: bool) -> None: + entries = normalize_allowed_hosts(["example.com", "*.example.org", "10.0.0.1"]) + entries += normalize_allowed_hosts(["::1"]) + assert host_is_allowed(host, entries) is allowed + + +# --- command line ---------------------------------------------------------- + + +def run_main(monkeypatch: pytest.MonkeyPatch, *argv: str) -> Any: + from unittest.mock import AsyncMock + + serve = AsyncMock() + monkeypatch.setattr(mcp_server_fetch, "serve", serve) + monkeypatch.setattr("sys.argv", ["mcp-server-fetch", *argv]) + mcp_server_fetch.main() + return serve + + +def test_flag_is_normalized_and_passed_to_serve( + monkeypatch: pytest.MonkeyPatch, +) -> None: + serve = run_main( + monkeypatch, + "--allowed-hosts", + "Example.com", + "*.docs.example.org", + "--allowed-hosts", + "10.0.0.1", + ) + serve.assert_awaited_once_with( + None, + False, + None, + False, + ["example.com", "*.docs.example.org", "10.0.0.1"], + True, + ) + + +def test_malformed_flag_value_exits_with_usage_error( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + with pytest.raises(SystemExit) as excinfo: + run_main(monkeypatch, "--allowed-hosts", "example.com:8080") + assert excinfo.value.code == 2 + assert "argument --allowed-hosts: 'example.com:8080' is not a host name" in ( + capsys.readouterr().err + ) + + +async def test_create_server_rejects_a_malformed_entry() -> None: + from mcp_server_fetch.server import create_server + + with pytest.raises(ValueError, match="'https://x' is not a host name"): + create_server(allowed_hosts=["https://x"]) + + +# --- enforcement ----------------------------------------------------------- + + +async def test_listed_host_is_fetched_with_its_robots_txt(web: FakeWeb) -> None: + web.add(ROBOTS, plain("", status=404)) + web.add(PAGE, plain("guide")) + async with connect(allowed_hosts=["*.example.com"]) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE}) + assert wire(result)["isError"] is False + assert web.urls() == [ROBOTS, PAGE] + + +async def test_unlisted_host_is_refused_before_robots_txt(web: FakeWeb) -> None: + async with connect(allowed_hosts=["docs.example.com"]) as (session, _): + result = await session.call_tool("fetch", {"url": OTHER}) + assert text_of(result) == TOOL_ERROR + refused(OTHER_ROBOTS, "other.example.org") + assert web.requests == [] + # Checked by name: nothing was resolved for the unlisted host. + assert web.lookups == [] + + +async def test_unlisted_host_is_refused_with_robots_txt_ignored( + web: FakeWeb, +) -> None: + async with connect(ignore_robots_txt=True, allowed_hosts=["docs.example.com"]) as ( + session, + _, + ): + result = await session.call_tool("fetch", {"url": OTHER}) + assert text_of(result) == TOOL_ERROR + refused(OTHER, "other.example.org") + assert web.requests == [] + + +async def test_redirect_to_unlisted_host_is_refused(web: FakeWeb) -> None: + web.add(ROBOTS, plain("", status=404)) + web.add(PAGE, httpx.Response(302, headers={"location": OTHER})) + web.add(OTHER, plain("secret")) + async with connect(allowed_hosts=["docs.example.com"]) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE}) + assert text_of(result) == TOOL_ERROR + refused(OTHER, "other.example.org") + # Neither the hop's robots.txt nor the hop itself was requested. + assert web.urls() == [ROBOTS, PAGE] + + +async def test_robots_redirect_to_unlisted_host_is_refused(web: FakeWeb) -> None: + web.add(ROBOTS, httpx.Response(301, headers={"location": OTHER_ROBOTS})) + async with connect(allowed_hosts=["docs.example.com"]) as (session, _): + result = await session.call_tool("fetch", {"url": PAGE}) + assert text_of(result) == TOOL_ERROR + refused(OTHER_ROBOTS, "other.example.org") + assert web.urls() == [ROBOTS] + + +async def test_redirect_between_listed_hosts_is_followed(web: FakeWeb) -> None: + web.add(ROBOTS, plain("", status=404)) + web.add(PAGE, httpx.Response(302, headers={"location": OTHER})) + web.add(OTHER_ROBOTS, plain("", status=404)) + web.add(OTHER, plain("moved here")) + async with connect(allowed_hosts=["docs.example.com", "*.example.org"]) as ( + session, + _, + ): + result = await session.call_tool("fetch", {"url": PAGE}) + assert wire(result)["isError"] is False + assert text_of(result).endswith("moved here") + assert web.urls() == [ROBOTS, PAGE, OTHER_ROBOTS, OTHER] + + +async def test_prompt_refuses_unlisted_host_and_its_redirects(web: FakeWeb) -> None: + web.add(PAGE, httpx.Response(302, headers={"location": OTHER})) + async with connect(allowed_hosts=["docs.example.com"]) as (session, _): + direct = await session.get_prompt("fetch", {"url": OTHER}) + redirected = await session.get_prompt("fetch", {"url": PAGE}) + for prompt in (direct, redirected): + text = prompt.messages[0].content + assert getattr(text, "text") == refused(OTHER, "other.example.org") + assert web.urls() == [PAGE] + + +async def test_listed_private_address_still_needs_allow_private_ips( + web: FakeWeb, +) -> None: + # Both checks apply: listing an internal host does not lift the + # private-address guard, and --allow-private-ips does not lift the list. + internal = "http://10.0.0.1/status" + web.add(internal, plain("internal")) + async with connect(ignore_robots_txt=True, allowed_hosts=["10.0.0.1"]) as ( + session, + _, + ): + guarded = await session.call_tool("fetch", {"url": internal}) + assert "which is not a public address" in text_of(guarded) + async with connect( + ignore_robots_txt=True, + allow_private_ips=True, + allowed_hosts=["10.0.0.1"], + ) as (session, _): + allowed = await session.call_tool("fetch", {"url": internal}) + unlisted = await session.call_tool("fetch", {"url": "http://10.0.0.2/"}) + assert text_of(allowed).endswith("internal") + assert text_of(unlisted) == TOOL_ERROR + refused("http://10.0.0.2/", "10.0.0.2") + assert web.urls() == [internal] + + +async def test_no_allowlist_fetches_any_public_host(web: FakeWeb) -> None: + web.add(OTHER, plain("anywhere")) + async with connect(ignore_robots_txt=True) as (session, _): + result = await session.call_tool("fetch", {"url": OTHER}) + assert text_of(result).endswith("anywhere") diff --git a/src/fetch/tests/test_entrypoints.py b/src/fetch/tests/test_entrypoints.py index a929c2d792..90fbcd3de9 100644 --- a/src/fetch/tests/test_entrypoints.py +++ b/src/fetch/tests/test_entrypoints.py @@ -31,7 +31,7 @@ def run_main(monkeypatch: pytest.MonkeyPatch, *argv: str) -> AsyncMock: def test_main_defaults(monkeypatch: pytest.MonkeyPatch) -> None: serve = run_main(monkeypatch) - serve.assert_awaited_once_with(None, False, None, False, True) + serve.assert_awaited_once_with(None, False, None, False, None, True) def test_main_passes_every_flag(monkeypatch: pytest.MonkeyPatch) -> None: @@ -45,7 +45,7 @@ def test_main_passes_every_flag(monkeypatch: pytest.MonkeyPatch) -> None: "--allow-private-ips", ) serve.assert_awaited_once_with( - "MyBot/1.0", True, "http://proxy.example.com:8080", True, True + "MyBot/1.0", True, "http://proxy.example.com:8080", True, None, True ) @@ -90,7 +90,7 @@ def test_package_init_run_as_main_calls_main(monkeypatch: pytest.MonkeyPatch) -> monkeypatch.setattr(mcp_server_fetch.server, "serve", serve) monkeypatch.setattr(sys, "argv", ["mcp-server-fetch"]) runpy.run_module("mcp_server_fetch.__init__", run_name="__main__") - serve.assert_awaited_once_with(None, False, None, False, True) + serve.assert_awaited_once_with(None, False, None, False, None, True) def console_script() -> Path: diff --git a/src/fetch/tests/test_readability_js_opt_out.py b/src/fetch/tests/test_readability_js_opt_out.py index 05cb77d89b..e297f372a9 100644 --- a/src/fetch/tests/test_readability_js_opt_out.py +++ b/src/fetch/tests/test_readability_js_opt_out.py @@ -76,7 +76,7 @@ def test_flag_is_passed_to_serve(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(mcp_server_fetch, "serve", serve) monkeypatch.setattr(sys, "argv", ["mcp-server-fetch", "--no-readability-js"]) mcp_server_fetch.main() - serve.assert_awaited_once_with(None, False, None, False, False) + serve.assert_awaited_once_with(None, False, None, False, None, False) def test_flag_is_in_help( From 2af0580be4425222140aeaf2ba45e43bb1b97b33 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 19:13:23 -0400 Subject: [PATCH 06/14] docs(fetch): move the allowed_hosts docstring entry clear of use_readability_js Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/src/mcp_server_fetch/server.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index 56d914f200..24345f7e51 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -786,11 +786,12 @@ async def serve( custom_user_agent: Optional custom User-Agent string to use for requests ignore_robots_txt: Whether to ignore robots.txt restrictions proxy_url: Optional proxy URL to use for requests + allowed_hosts: When given, the only hosts the server may fetch from: + exact hosts, or ``*.example.com`` for any subdomain of it, checked + in addition to the private-address guard. Empty or None allows + every host allow_private_ips: Allow fetching private, loopback and link-local addresses, which are refused by default - allowed_hosts: When given, the only hosts the server may fetch from: - exact hosts, or ``*.example.com`` for any subdomain of it. Empty - or None allows every host use_readability_js: Extract HTML with Readability.js under Node when Node is on PATH; False always uses the pure-Python extractor """ From 3892d337dec9261dcdf78dd448fe17bb38f425e3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 19:16:18 -0400 Subject: [PATCH 07/14] test(fetch): keep the timeout tests from leaking FETCH_TIMEOUT_MS delenv of an absent variable records no undo, so main()'s write to FETCH_TIMEOUT_MS outlived the test and changed later tests' default. Set it first so teardown removes it. Also retitle the characterization test that called the timeouts hard-coded. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/tests/test_protocol.py | 7 ++++--- src/fetch/tests/test_timeout.py | 8 +++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/fetch/tests/test_protocol.py b/src/fetch/tests/test_protocol.py index 60d214c190..9f8540ebae 100644 --- a/src/fetch/tests/test_protocol.py +++ b/src/fetch/tests/test_protocol.py @@ -1008,9 +1008,10 @@ async def test_guard_uses_the_real_resolver() -> None: ) -async def test_timeouts_are_hard_coded(web: FakeWeb) -> None: - # Characterizes #4448: the page fetch uses a fixed 30s timeout and the - # robots.txt fetch uses httpx's 5s default; neither is configurable. +async def test_default_timeouts(web: FakeWeb) -> None: + # With nothing configured the page fetch waits 30s and the robots.txt + # fetch uses httpx's 5s default. Only the page timeout is configurable + # (#4448; tests/test_timeout.py). web.add(ROBOTS, plain("", status=404)) web.add(PAGE, plain("ok")) async with connect() as (session, _): diff --git a/src/fetch/tests/test_timeout.py b/src/fetch/tests/test_timeout.py index 4cd437dec1..cdfff77502 100644 --- a/src/fetch/tests/test_timeout.py +++ b/src/fetch/tests/test_timeout.py @@ -38,9 +38,11 @@ @pytest.fixture(autouse=True) def no_timeout_env(monkeypatch: pytest.MonkeyPatch) -> None: - # A developer's own FETCH_TIMEOUT_MS must not leak into these tests, and - # monkeypatch restores whatever main() writes to the variable. - monkeypatch.delenv(TIMEOUT_ENV, raising=False) + # A developer's own FETCH_TIMEOUT_MS must not leak into these tests, nor + # what main() writes to it into later tests. setenv first: delenv of an + # absent variable records no undo, so main()'s write would survive. + monkeypatch.setenv(TIMEOUT_ENV, "") + monkeypatch.delenv(TIMEOUT_ENV) def recording_page(seen: list[dict[str, Any]]) -> Any: From 6ca876fe615de796bff06c018ed0f576dddadae6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 19:51:56 -0400 Subject: [PATCH 08/14] docs(fetch): say a raw fetch gets the site's default representation Accept: */* lets the site choose, so a raw fetch is not guaranteed HTML. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/README.md | 2 +- src/fetch/src/mcp_server_fetch/server.py | 3 ++- src/fetch/tests/test_markdown_negotiation.py | 5 +++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/fetch/README.md b/src/fetch/README.md index 2824c45600..9c48bbea78 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -41,7 +41,7 @@ The server is built on the MCP Python SDK's high-level `MCPServer`: the tool and ### Content negotiation -The tool and the prompt ask for markdown first, with the request header `Accept: text/markdown, text/html;q=0.9, */*;q=0.8`. A site that serves markdown through content negotiation sends it directly, and a `text/markdown` response is returned as-is: it is not simplified and carries no raw-content notice. A site that ignores the header sends what it always did, and HTML is simplified to markdown as before. A `raw` fetch does not ask for markdown, so it still gets the page's own HTML (a markdown response it receives anyway is returned as-is too). +The tool and the prompt ask for markdown first, with the request header `Accept: text/markdown, text/html;q=0.9, */*;q=0.8`. A site that serves markdown through content negotiation sends it directly, and a `text/markdown` response is returned as-is: it is not simplified and carries no raw-content notice. A site that ignores the header sends what it always did, and HTML is simplified to markdown as before. A `raw` fetch keeps the default `Accept: */*`, so it gets whatever the site serves by default (for a web page, its HTML), unsimplified; a markdown response it receives anyway is returned as-is too. ### Protocol versions diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index b6723cee54..535e2cc805 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -568,7 +568,8 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: hooks.append(_check_redirect_robots_txt) if hooks: client.event_hooks = {"request": hooks} - # A raw fetch keeps httpx's default Accept, so it still gets the HTML. + # A raw fetch keeps httpx's default Accept: */*, so it gets the site's + # default representation (a page's HTML), as before this negotiation. if not force_raw: client.headers["Accept"] = MARKDOWN_ACCEPT try: diff --git a/src/fetch/tests/test_markdown_negotiation.py b/src/fetch/tests/test_markdown_negotiation.py index 40e247aeff..197a90bef7 100644 --- a/src/fetch/tests/test_markdown_negotiation.py +++ b/src/fetch/tests/test_markdown_negotiation.py @@ -6,8 +6,9 @@ to markdown" behind the raw-content prefix. Now a non-raw fetch prefers ``text/markdown`` (HTML next, anything else last), and a markdown response is returned as-is with no prefix. A ``raw`` fetch keeps the default ``Accept``, -so it still gets the page's own HTML. These tests drive ``serve()`` through an -MCP client with HTTP served by ``harness.FakeWeb``. +so it gets the site's default representation (a page's HTML) as before. These +tests drive ``serve()`` through an MCP client with HTTP served by +``harness.FakeWeb``. """ from __future__ import annotations From ab56c396a653bf7bce9f6c9d2efbffdde1c7a60b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 20:02:09 -0400 Subject: [PATCH 09/14] test(fetch): look for node and npm only in the fake bin directory A real Node.js or npm in /usr/bin (a distribution install) was still found by the runner, so the missing-tool tests could find it and run real npm. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/tests/test_readability_subprocess.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/fetch/tests/test_readability_subprocess.py b/src/fetch/tests/test_readability_subprocess.py index bda870527f..c673701ceb 100644 --- a/src/fetch/tests/test_readability_subprocess.py +++ b/src/fetch/tests/test_readability_subprocess.py @@ -16,6 +16,7 @@ from __future__ import annotations import os +import shutil import stat import subprocess import sys @@ -58,14 +59,22 @@ def html(body: str) -> httpx.Response: @pytest.fixture def bindir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: - """An empty directory that is the first entry on PATH, ahead of system tools. + """An empty directory that is the only place the runner looks for node and npm. - The real Node.js and npm (in /usr/local/bin and the like) are left off, so - only the fakes a test writes here are found. + The fake scripts' own utilities (sleep, cat, mkdir) still come from the + system directories on PATH, but the runner's lookup sees this directory + alone, so a real Node.js or npm installed anywhere (/usr/bin included, as + a distribution installs it) is never found or run. """ path = tmp_path / "bin" path.mkdir() monkeypatch.setenv("PATH", os.pathsep.join([str(path), "/usr/bin", "/bin"])) + which = shutil.which # the real one, before the module's is replaced + + def only_bindir(name: str) -> str | None: + return which(name, path=str(path)) + + monkeypatch.setattr(readability_js.shutil, "which", only_bindir) return path From 9b1570af3c4e04edd61f47cba7f2164b6b297b3a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 18:18:03 -0400 Subject: [PATCH 10/14] feat(fetch): open the simplified result with the page title Readability.js drops an article heading that repeats the document title, so a page like pandas' 2.1.0 release note came back without the line naming it. A new title module reads the title from the raw HTML (og:title, the h1 when holds it, <title>, the h1) and puts it on top of the markdown unless the markdown already opens with it. Raw results and failed simplifications are untouched. Issue: #2472 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com> --- src/fetch/README.md | 2 + src/fetch/src/mcp_server_fetch/server.py | 4 +- src/fetch/src/mcp_server_fetch/title.py | 94 ++++++++++++++ src/fetch/tests/test_page_title.py | 154 +++++++++++++++++++++++ 4 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 src/fetch/src/mcp_server_fetch/title.py create mode 100644 src/fetch/tests/test_page_title.py diff --git a/src/fetch/README.md b/src/fetch/README.md index 7f90ee4a7c..6c7a078920 100644 --- a/src/fetch/README.md +++ b/src/fetch/README.md @@ -15,6 +15,8 @@ Requires MCP Python SDK 2.x (`mcp>=2.3,<3`). The fetch tool will truncate the response, but by using the `start_index` argument, you can specify where to start the content extraction. This lets models read a webpage in chunks, until they find the information they need. +The markdown of an HTML page opens with the page's title as a `#` heading (its `og:title`, else its `<h1>` when the `<title>` holds it, else its `<title>` or `<h1>`), because the extraction drops a heading that repeats the title. It is not added when the markdown already opens with it, to a raw result, or to a page that failed to simplify. + ### Available Tools - `fetch` - Fetches a URL from the internet and extracts its contents as markdown. diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index 7315a65ce5..21d00eb169 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -41,6 +41,8 @@ from protego import Protego from pydantic import AnyUrl, Field +from .title import with_page_title + # The version this server reports in serverInfo, read from the installed # distribution's metadata (pyproject.toml) so it cannot drift from the # published version (#360). Without it the SDK reports its own `mcp` version. @@ -601,7 +603,7 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: content = await asyncio.to_thread( extract_content_from_html, page_raw, use_readability_js ) - return content, "" + return with_page_title(page_raw, content), "" # A response with no Content-Type would otherwise leave an empty slot here. declared_type = content_type or "(not declared)" diff --git a/src/fetch/src/mcp_server_fetch/title.py b/src/fetch/src/mcp_server_fetch/title.py new file mode 100644 index 0000000000..b380af91f6 --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/title.py @@ -0,0 +1,94 @@ +# The page title for the fetch server's simplified (Markdown) result (#2472). +# +# Readability.js removes an article heading that repeats the document title, +# and neither extractor returns the `<title>` itself in the content the server +# converts, so a page such as a Sphinx release note came back without the line +# that says what it is. This module reads the title from the raw HTML and puts +# it at the top of the Markdown as a level-one heading, unless the Markdown +# already opens with it. +# +# It works from the raw page rather than from readabilipy's result, so it is +# independent of how the extractor is invoked and is applied at the one call +# site in `fetch_url` that simplifies HTML. The raw result (`raw: true`) and +# non-HTML bodies never pass through it. + +import re + +from bs4 import BeautifulSoup, Tag + +# The marker `extract_content_from_html` returns when a page simplifies to +# nothing. A title on top of it would make the failure read as content. +_FAILURE_PREFIX = "<error>" + +# Markdown heading markers, emphasis, escapes, Sphinx's "¶" permalink and any +# other punctuation are ignored when comparing a heading with the title. +_NOT_WORD = re.compile(r"[\W_]+") + + +def _normalized(text: str) -> str: + return _NOT_WORD.sub("", text).casefold() + + +def _collapsed(text: str) -> str: + return " ".join(text.split()) + + +def _text(candidate: str) -> str: + # A trailing "¶" is a permalink that is not marked up as one. + return _collapsed(candidate).rstrip("¶").strip() + + +def page_title(html: str) -> str | None: + """The page's title, or None when the page names none. + + In order: `og:title`; the first `<h1>` when `<title>` holds it, since + `<title>` then only adds the site's name ("What's new — pandas docs"); + `<title>`; the first `<h1>`. + """ + soup = BeautifulSoup(html, "html.parser") + og = soup.find("meta", attrs={"property": "og:title"}) + og_title = _text(str(og.get("content", ""))) if isinstance(og, Tag) else "" + title = _text(soup.title.get_text(" ")) if soup.title else "" + h1 = soup.find("h1") + heading = "" + if isinstance(h1, Tag): + # Sphinx appends a permalink anchor ("¶", or "#" in some themes) to + # each heading; it is not part of the heading's text. + for link in h1.find_all("a", class_="headerlink"): + link.decompose() + heading = _text(h1.get_text(" ")) + heading_in_title = bool(_normalized(heading)) and _normalized( + heading + ) in _normalized(title) + if og_title: + return og_title + if heading_in_title: + return heading + return title or heading or None + + +def _opens_with_title(content: str, title: str) -> bool: + """Whether the Markdown's first non-empty line already carries the title. + + A first line holding the whole title counts (the pure-Python extractor + opens with the title in a paragraph), and so does a first heading that the + title holds: `<title>` often adds the site name ("What's new — pandas + docs") to the page's own heading ("What's new"). + """ + first = next((line for line in content.splitlines() if line.strip()), "") + line, wanted = _normalized(first), _normalized(title) + if not line or not wanted: + return False + if wanted in line: + return True + return first.lstrip().startswith("#") and line in wanted + + +def with_page_title(html: str, content: str) -> str: + """``content`` with the page's title as a heading on top, if it lacks one.""" + if content.startswith(_FAILURE_PREFIX): + return content + title = page_title(html) + if title is None or _opens_with_title(content, title): + return content + return f"# {title}\n\n{content.lstrip()}" diff --git a/src/fetch/tests/test_page_title.py b/src/fetch/tests/test_page_title.py new file mode 100644 index 0000000000..9d8fa3fb8f --- /dev/null +++ b/src/fetch/tests/test_page_title.py @@ -0,0 +1,154 @@ +"""Tests for the page title on the fetch tool's simplified result (#2472). + +Readability.js drops an article heading that repeats the document title, so a +page such as pandas' "What's new in 2.1.0" release note came back without the +line naming it. ``with_page_title`` now puts the title on top as a heading +unless the Markdown already opens with it. The protocol-level tests drive the +real server through an MCP client with HTTP served by ``harness.FakeWeb``, on +both extractor paths, and pin that the raw result is untouched; the unit tests +below pin each rule for choosing the title and spotting a duplicate. +""" + +from __future__ import annotations + +from typing import Any + +import httpx +import pytest +from mcp import ClientSession +from mcp.types import CallToolResult + +from mcp_server_fetch.title import page_title, with_page_title + +from .harness import FakeWeb, connect + +PAGE = "https://example.com/page" +ROBOTS = "https://example.com/robots.txt" +PREFIX = f"Contents of {PAGE}:\n" + +# The shape of a Sphinx (pydata theme) page: `<title>` adds the site name to +# the `<h1>`, which carries a "#" permalink. +BODY = "<p>These are the changes in pandas 2.1.0, with a long enough body. " * 8 +SPHINX_PAGE = f"""<!DOCTYPE html> +<html><head><title>What’s new in 2.1.0 (Aug 30, 2023) — pandas documentation +
+

What’s new in 2.1.0 (Aug 30, 2023)#

+{BODY}

+
""" + + +def text_of(result: CallToolResult) -> str: + assert not result.is_error, result + data = result.model_dump(by_alias=True, mode="json", exclude_none=True) + assert len(data["content"]) == 1 + return data["content"][0]["text"] + + +async def fetch(web: FakeWeb, html: str, **arguments: Any) -> str: + web.add(ROBOTS, httpx.Response(404)) + web.add( + PAGE, + httpx.Response(200, text=html, headers={"content-type": "text/html"}), + ) + async with connect() as (session, _): + assert isinstance(session, ClientSession) + result = await session.call_tool("fetch", {"url": PAGE, **arguments}) + return text_of(result) + + +@pytest.mark.usefixtures("node_readability", "private_tempdir") +async def test_readability_js_result_gains_the_dropped_title(web: FakeWeb) -> None: + text = await fetch(web, SPHINX_PAGE) + assert text.startswith(f"{PREFIX}# What’s new in 2.1.0 (Aug 30, 2023)\n\n") + assert text.count("What’s new in 2.1.0") == 1 + assert "These are the changes in pandas 2.1.0" in text + + +@pytest.mark.usefixtures("python_readability") +async def test_python_result_is_not_given_a_second_title(web: FakeWeb) -> None: + # The pure-Python extractor keeps the `

` (and opens with the + # ``), so the title is already there and nothing is added. + text = await fetch(web, SPHINX_PAGE) + assert not text.startswith(f"{PREFIX}# ") + assert "What’s new in 2.1.0" in text + assert "These are the changes in pandas 2.1.0" in text + + +async def test_raw_result_is_untouched(web: FakeWeb) -> None: + text = await fetch(web, SPHINX_PAGE, raw=True) + assert text.endswith(SPHINX_PAGE) + assert "\n# What’s new" not in text + + +@pytest.mark.usefixtures("node_readability", "private_tempdir") +async def test_failed_simplification_is_not_titled(web: FakeWeb) -> None: + text = await fetch( + web, "<html><head><title>Shell" + ) + assert text == f"{PREFIX}Page failed to be simplified from HTML" + + +@pytest.mark.parametrize( + ("html", "expected"), + [ + # og:title wins over everything else. + ( + 'T

T

', + "Social title", + ), + # The `

` wins when `` holds it. + ("<title>Heading | Site

Heading

", "Heading"), + # Otherwise ``. + ("<title>Document title

Something else

", "Document title"), + # A trailing "¶" permalink that is plain text is dropped. + ("Doc ¶", "Doc"), + # No ``: the `<h1>`. + ("<body><h1>Only heading</h1></body>", "Only heading"), + # An empty og:title and title fall through to the `<h1>`. + ( + '<meta property="og:title" content=""><title>

H

', + "H", + ), + # Nothing names the page. + ("

No title

", None), + ("

#

", None), + ], +) +def test_page_title(html: str, expected: str | None) -> None: + assert page_title(html) == expected + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + # No heading: the title goes on top. + ("Body text.\n", "# Title Here\n\nBody text.\n"), + # Leading blank lines are folded into the separator. + ("\n\nBody text.\n", "# Title Here\n\nBody text.\n"), + # Already opens with the title as a heading, at any level. + ("# Title Here\n\nBody.", "# Title Here\n\nBody."), + ("\n## title here ¶\n\nBody.", "\n## title here ¶\n\nBody."), + # A first line that holds the whole title, heading or not. + ("

Title Here — Site

Skip\n", "

Title Here — Site

Skip\n"), + # A first heading the title holds (the title adds a site name). + ("# Title\n\nBody.", "# Title\n\nBody."), + # A first paragraph the title holds is not a heading: title added. + ("Title\n\nBody.", "# Title Here\n\nTitle\n\nBody."), + # A first line of punctuation only cannot match. + ("---\n\nBody.", "# Title Here\n\n---\n\nBody."), + # Empty content still gains the title. + ("", "# Title Here\n\n"), + ], +) +def test_with_page_title(content: str, expected: str) -> None: + assert with_page_title("Title Here", content) == expected + + +def test_with_page_title_leaves_untitled_page_and_failure_alone() -> None: + assert with_page_title("

x

", "Body.") == "Body." + failure = "Page failed to be simplified from HTML" + assert with_page_title("T", failure) == failure + + +def test_title_with_no_word_characters_is_not_matched() -> None: + assert with_page_title("—", "Body.") == "# —\n\nBody." From 8b912e12f85eaec80832ef3d310b64e120a8919d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 20:32:39 -0400 Subject: [PATCH 11/14] fix(fetch): match the page title by visible whole words, off the loop Copilot round 1 on #5398: the duplicate check compared characters with word boundaries removed and read link destinations and tags, so 'Rust' matched 'Trust' and a title only in a URL suppressed the heading. Compare the first line's visible words instead. Title extraction parses the whole page, so run it in a worker thread like the extraction itself. Issue: #2472 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/src/mcp_server_fetch/server.py | 2 +- src/fetch/src/mcp_server_fetch/title.py | 24 ++++++++++++++++++----- src/fetch/tests/test_page_title.py | 25 ++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/fetch/src/mcp_server_fetch/server.py b/src/fetch/src/mcp_server_fetch/server.py index 21d00eb169..2135436c32 100644 --- a/src/fetch/src/mcp_server_fetch/server.py +++ b/src/fetch/src/mcp_server_fetch/server.py @@ -603,7 +603,7 @@ async def _check_redirect_robots_txt(request: httpx.Request) -> None: content = await asyncio.to_thread( extract_content_from_html, page_raw, use_readability_js ) - return with_page_title(page_raw, content), "" + return await asyncio.to_thread(with_page_title, page_raw, content), "" # A response with no Content-Type would otherwise leave an empty slot here. declared_type = content_type or "(not declared)" diff --git a/src/fetch/src/mcp_server_fetch/title.py b/src/fetch/src/mcp_server_fetch/title.py index b380af91f6..5d58599137 100644 --- a/src/fetch/src/mcp_server_fetch/title.py +++ b/src/fetch/src/mcp_server_fetch/title.py @@ -20,13 +20,27 @@ # nothing. A title on top of it would make the failure read as content. _FAILURE_PREFIX = "" -# Markdown heading markers, emphasis, escapes, Sphinx's "¶" permalink and any -# other punctuation are ignored when comparing a heading with the title. -_NOT_WORD = re.compile(r"[\W_]+") +# Titles are compared word by word: case, Markdown heading markers, emphasis, +# escapes, Sphinx's "¶" permalink and other punctuation are ignored, but word +# boundaries are kept, so "Rust" does not match "Trust". +_WORD = re.compile(r"[^\W_]+") +# What a reader does not see on a Markdown line: a link's destination and any +# HTML tag (an autolink's URL included). A title found only there is not shown. +_LINK_DESTINATION = re.compile(r"\]\([^)]*\)") +_HTML_TAG = re.compile(r"<[^>]*>") def _normalized(text: str) -> str: - return _NOT_WORD.sub("", text).casefold() + """``text`` as its casefolded words, one space apart, padded with spaces. + + The padding makes ``in`` a whole-word, whole-phrase containment test. + """ + words = _WORD.findall(text.casefold()) + return f" {' '.join(words)} " if words else "" + + +def _visible(line: str) -> str: + return _HTML_TAG.sub(" ", _LINK_DESTINATION.sub("]", line)) def _collapsed(text: str) -> str: @@ -76,7 +90,7 @@ def _opens_with_title(content: str, title: str) -> bool: docs") to the page's own heading ("What's new"). """ first = next((line for line in content.splitlines() if line.strip()), "") - line, wanted = _normalized(first), _normalized(title) + line, wanted = _normalized(_visible(first)), _normalized(title) if not line or not wanted: return False if wanted in line: diff --git a/src/fetch/tests/test_page_title.py b/src/fetch/tests/test_page_title.py index 9d8fa3fb8f..f0c6b383c1 100644 --- a/src/fetch/tests/test_page_title.py +++ b/src/fetch/tests/test_page_title.py @@ -136,6 +136,17 @@ def test_page_title(html: str, expected: str | None) -> None: ("Title\n\nBody.", "# Title Here\n\nTitle\n\nBody."), # A first line of punctuation only cannot match. ("---\n\nBody.", "# Title Here\n\n---\n\nBody."), + # The title only in a link destination or tag is not shown: added. + ( + "[Source](https://example.com/title-here)\n", + "# Title Here\n\n[Source](https://example.com/title-here)\n", + ), + ( + "x\n", + "# Title Here\n\nx\n", + ), + # The title as a link's visible text is shown: not added. + ("[Title Here](https://example.com)\n", "[Title Here](https://example.com)\n"), # Empty content still gains the title. ("", "# Title Here\n\n"), ], @@ -150,5 +161,19 @@ def test_with_page_title_leaves_untitled_page_and_failure_alone() -> None: assert with_page_title("T", failure) == failure +@pytest.mark.parametrize( + ("title", "content"), + [ + # Matching is by whole words, in both directions. + ("Rust", "Trust the compiler."), + ("Go", "Algorithms make this possible."), + ("Rust Guide", "# Rust Gui\n"), + ], +) +def test_partial_words_do_not_match(title: str, content: str) -> None: + html = f"{title}" + assert with_page_title(html, content) == f"# {title}\n\n{content}" + + def test_title_with_no_word_characters_is_not_matched() -> None: assert with_page_title("—", "Body.") == "# —\n\nBody." From e4ce738d920d6d7f29b6f72805df625c402455da Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 20:39:08 -0400 Subject: [PATCH 12/14] fix(fetch): bound the cleanup after a Readability.js timeout After a timeout the runner drained the killed child's pipes with an unbounded communicate(), so a helper that left the process group and kept stdout or stderr open hung the call anyway. It now only reaps the child, for at most 5 s. On Windows, a taskkill that cannot start or does not finish no longer skips killing the direct child. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../src/mcp_server_fetch/readability_js.py | 31 +++++++++--- .../tests/test_readability_subprocess.py | 50 +++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/fetch/src/mcp_server_fetch/readability_js.py b/src/fetch/src/mcp_server_fetch/readability_js.py index 770896b3ae..424778c9ad 100644 --- a/src/fetch/src/mcp_server_fetch/readability_js.py +++ b/src/fetch/src/mcp_server_fetch/readability_js.py @@ -53,6 +53,9 @@ INSTALL_TIMEOUT_SECONDS = 120.0 # How long `node -v` may take. VERSION_TIMEOUT_SECONDS = 10.0 +# How long cleanup after a timeout may wait: for `taskkill`, and for the +# killed child to be reaped. +KILL_TIMEOUT_SECONDS = 5.0 # readabilipy's `package.json` declares `"engines": {"node": ">=14.0.0"}`. MIN_NODE_MAJOR = 14 # What `ExtractArticle.js` requires; all three present means installed. @@ -83,13 +86,19 @@ def _forward(output: str) -> None: def _kill_tree(proc: subprocess.Popen[str]) -> None: """Kill ``proc`` and everything it started (npm runs node, `npm.cmd` runs cmd).""" if sys.platform == "win32": - subprocess.run( - ["taskkill", "/F", "/T", "/PID", str(proc.pid)], - stdin=subprocess.DEVNULL, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, - ) + try: + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(proc.pid)], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + timeout=KILL_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError): + # taskkill could not start or did not finish: the direct child + # is still killed below. + pass else: try: # start_new_session made the child a process-group leader. @@ -126,7 +135,13 @@ def _run( stdout, stderr = proc.communicate(timeout=timeout) except subprocess.TimeoutExpired: _kill_tree(proc) - proc.communicate() + # Reap the child, but do not drain its pipes: a descendant that left + # its process group (or survived taskkill) can hold them open forever. + # The pipes close when `proc` is collected. + try: + proc.wait(timeout=KILL_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + pass _log(f"{Path(argv[0]).name} did not finish within {timeout:g} s and was killed") return None return subprocess.CompletedProcess(argv, proc.returncode, stdout, stderr) diff --git a/src/fetch/tests/test_readability_subprocess.py b/src/fetch/tests/test_readability_subprocess.py index c673701ceb..c548032dba 100644 --- a/src/fetch/tests/test_readability_subprocess.py +++ b/src/fetch/tests/test_readability_subprocess.py @@ -425,6 +425,56 @@ def kill(self) -> None: assert killed == [True] +def test_kill_tree_kills_the_child_when_taskkill_fails( + monkeypatch: pytest.MonkeyPatch, +) -> None: + killed: list[bool] = [] + timeouts: list[Any] = [] + + class Proc: + pid = 4242 + + def kill(self) -> None: + killed.append(True) + + def run(argv: list[str], **kwargs: Any) -> None: + timeouts.append(kwargs.get("timeout")) + raise OSError("taskkill is blocked") + + monkeypatch.setattr(readability_js.sys, "platform", "win32") + monkeypatch.setattr(readability_js.subprocess, "run", run) + readability_js._kill_tree(cast("subprocess.Popen[str]", Proc())) + assert timeouts == [readability_js.KILL_TIMEOUT_SECONDS] + assert killed == [True] + + +@posix_only +def test_timeout_returns_while_a_detached_helper_holds_the_pipes( + bindir: Path, jsdir: Path, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + # A helper that leaves the process group (its own session) survives the + # group kill and keeps stdout and stderr open; the run must still end. + install(jsdir) + pid = tmp_path / "helper.pid" + helper = ( + "import os, time; os.setsid(); " + f"open({str(pid)!r}, 'w').write(str(os.getpid())); time.sleep(3600)" + ) + tool(bindir, "node", NODE_VERSION + f'"{sys.executable}" -c "{helper}" &\nwait\n') + monkeypatch.setattr(readability_js, "EXTRACT_TIMEOUT_SECONDS", 1.0) + started = time.monotonic() + try: + assert readability_js.extract_article(ARTICLE) is None + assert ( + time.monotonic() - started < 1.0 + readability_js.KILL_TIMEOUT_SECONDS + 3 + ) + finally: + deadline = time.monotonic() + 5 + while not pid.exists() and time.monotonic() < deadline: # pragma: no cover + time.sleep(0.05) + os.kill(int(pid.read_text()), 9) + + @posix_only def test_kill_tree_tolerates_a_group_already_gone( monkeypatch: pytest.MonkeyPatch, From eabad3f48fceb044006bed0bed2f10e5f0331a70 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 20:59:17 -0400 Subject: [PATCH 13/14] fix(fetch): scan the first line in linear time and escape the heading Copilot round 2 on #5398: the link-destination and tag regexes rescanned an unterminated line quadratically, stripped tag-like text inside code spans, and the title was emitted unescaped, so '' became a tag and '__init__' emphasis. Read the visible text in one pass that keeps code spans, and backslash-escape Markdown and HTML specials in the heading. Issue: #2472 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/src/mcp_server_fetch/title.py | 42 +++++++++++++++++++++---- src/fetch/tests/test_page_title.py | 33 +++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/src/fetch/src/mcp_server_fetch/title.py b/src/fetch/src/mcp_server_fetch/title.py index 5d58599137..bab279aed1 100644 --- a/src/fetch/src/mcp_server_fetch/title.py +++ b/src/fetch/src/mcp_server_fetch/title.py @@ -24,10 +24,10 @@ # escapes, Sphinx's "¶" permalink and other punctuation are ignored, but word # boundaries are kept, so "Rust" does not match "Trust". _WORD = re.compile(r"[^\W_]+") -# What a reader does not see on a Markdown line: a link's destination and any -# HTML tag (an autolink's URL included). A title found only there is not shown. -_LINK_DESTINATION = re.compile(r"\]\([^)]*\)") -_HTML_TAG = re.compile(r"<[^>]*>") +# Characters that Markdown or inline HTML would interpret in the emitted +# heading ("" read as a tag, "__init__" as emphasis, "<" as an entity), +# each escaped with a backslash so the heading shows the title as written. +_MARKDOWN_SPECIAL = re.compile(r"([\\`*_\[\]<>#&|~])") def _normalized(text: str) -> str: @@ -40,7 +40,36 @@ def _normalized(text: str) -> str: def _visible(line: str) -> str: - return _HTML_TAG.sub(" ", _LINK_DESTINATION.sub("]", line)) + """The text a reader sees on one Markdown line. + + A link's destination (`](...)`) and an HTML tag (an autolink's URL + included) are dropped; a code span's contents are kept, tag-like text + and all. One pass, so a long line with an unterminated `](`, `<` or + backtick costs linear time: each closer is searched for forward from + where it is needed, and once a search for one fails it is never repeated. + """ + out: list[str] = [] + missing: set[str] = set() + i, end = 0, len(line) + while i < end: + for opener, closer, keep in ( + ("`", "`", True), + ("<", ">", False), + ("](", ")", False), + ): + if closer in missing or not line.startswith(opener, i): + continue + j = line.find(closer, i + len(opener)) + if j == -1: + missing.add(closer) + continue + out.append(line[i + len(opener) : j] if keep else " ") + i = j + 1 + break + else: + out.append(line[i]) + i += 1 + return "".join(out) def _collapsed(text: str) -> str: @@ -105,4 +134,5 @@ def with_page_title(html: str, content: str) -> str: title = page_title(html) if title is None or _opens_with_title(content, title): return content - return f"# {title}\n\n{content.lstrip()}" + heading = _MARKDOWN_SPECIAL.sub(r"\\\1", title) + return f"# {heading}\n\n{content.lstrip()}" diff --git a/src/fetch/tests/test_page_title.py b/src/fetch/tests/test_page_title.py index f0c6b383c1..1aea9a2f59 100644 --- a/src/fetch/tests/test_page_title.py +++ b/src/fetch/tests/test_page_title.py @@ -175,5 +175,38 @@ def test_partial_words_do_not_match(title: str, content: str) -> None: assert with_page_title(html, content) == f"# {title}\n\n{content}" +def test_code_span_contents_count_as_visible() -> None: + # A heading's inline HTML example is text, not a tag, inside a code span. + content = "# The `` element\n\nBody." + assert ( + with_page_title("The <input> element", content) == content + ) + + +@pytest.mark.parametrize("opener", ["](", "<", "`"]) +def test_unterminated_markup_on_a_long_first_line(opener: str) -> None: + # Linear in the line's length: a quadratic scan of 200,000 openers would + # not finish in the test's time. The title is not on the line, so it is added. + content = opener * 200_000 + result = with_page_title("Title Here", content) + assert result == f"# Title Here\n\n{content}" + + +@pytest.mark.parametrize( + ("title_html", "heading"), + [ + ("Using <T>", "# Using \\"), + ("__init__ and *args", "# \\_\\_init\\_\\_ and \\*args"), + ( + "[Draft] R&D #3 `x` | a~b \\ c", + "# \\[Draft\\] R\\&D \\#3 \\`x\\` \\| a\\~b \\\\ c", + ), + ], +) +def test_heading_escapes_markdown_and_html(title_html: str, heading: str) -> None: + result = with_page_title(f"{title_html}", "Body.") + assert result == f"{heading}\n\nBody." + + def test_title_with_no_word_characters_is_not_matched() -> None: assert with_page_title("—", "Body.") == "# —\n\nBody." From 8b836f7c0291ac199ab4c7d13b7516a64f6462a5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 28 Oct 2026 21:12:30 -0400 Subject: [PATCH 14/14] fix(fetch): keep literal and escaped angle brackets as visible text Copilot round 3 on #5398: the scanner dropped any <...> run, so a heading 'Comparing x < 5 and y > 3' did not match its own title and a duplicate heading was added. As in CommonMark, '<' opens a tag only before a letter, '/', '!' or '?', and a backslash-escaped character is text. Issue: #2472 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/fetch/src/mcp_server_fetch/title.py | 14 +++++++++++++- src/fetch/tests/test_page_title.py | 15 ++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/fetch/src/mcp_server_fetch/title.py b/src/fetch/src/mcp_server_fetch/title.py index bab279aed1..074387211f 100644 --- a/src/fetch/src/mcp_server_fetch/title.py +++ b/src/fetch/src/mcp_server_fetch/title.py @@ -39,12 +39,18 @@ def _normalized(text: str) -> str: return f" {' '.join(words)} " if words else "" +def _starts_tag(char: str) -> bool: + return char.isascii() and (char.isalpha() or char in "/!?") + + def _visible(line: str) -> str: """The text a reader sees on one Markdown line. A link's destination (`](...)`) and an HTML tag (an autolink's URL included) are dropped; a code span's contents are kept, tag-like text - and all. One pass, so a long line with an unterminated `](`, `<` or + and all. As in CommonMark, a `<` starts a tag only before a letter, `/`, + `!` or `?` (so "x < 5 and y > 3" is text), and a backslash-escaped + character is text. One pass, so a long line with an unterminated `](`, `<` or backtick costs linear time: each closer is searched for forward from where it is needed, and once a search for one fails it is never repeated. """ @@ -52,6 +58,10 @@ def _visible(line: str) -> str: missing: set[str] = set() i, end = 0, len(line) while i < end: + if line[i] == "\\" and i + 1 < end: + out.append(line[i + 1]) + i += 2 + continue for opener, closer, keep in ( ("`", "`", True), ("<", ">", False), @@ -59,6 +69,8 @@ def _visible(line: str) -> str: ): if closer in missing or not line.startswith(opener, i): continue + if opener == "<" and not _starts_tag(line[i + 1 : i + 2]): + continue j = line.find(closer, i + len(opener)) if j == -1: missing.add(closer) diff --git a/src/fetch/tests/test_page_title.py b/src/fetch/tests/test_page_title.py index 1aea9a2f59..be4978a31b 100644 --- a/src/fetch/tests/test_page_title.py +++ b/src/fetch/tests/test_page_title.py @@ -183,7 +183,20 @@ def test_code_span_contents_count_as_visible() -> None: ) -@pytest.mark.parametrize("opener", ["](", "<", "`"]) +@pytest.mark.parametrize( + ("title_html", "content"), + [ + # A "<" before a space is text, not a tag. + ("Comparing x < 5 and y > 3", "# Comparing x < 5 and y > 3\n\nBody."), + # Backslash-escaped brackets are text too. + ("Using <T>", "# Using \\\n\nBody."), + ], +) +def test_literal_angle_brackets_count_as_visible(title_html: str, content: str) -> None: + assert with_page_title(f"{title_html}", content) == content + + +@pytest.mark.parametrize("opener", ["](", " None: # Linear in the line's length: a quadratic scan of 200,000 openers would # not finish in the test's time. The title is not on the line, so it is added.