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..ec90981f40 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 `

` when the `` 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. @@ -22,6 +24,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 @@ -34,11 +37,15 @@ 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: <name>`), as the specification requires. So is getting a prompt it does not have (`Unknown prompt: <name>`), 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: <name>`), as the specification requires. So is getting a prompt it does not have (`Unknown prompt: <name>`), or the `fetch` prompt without its `url` (`URL is required`) or with a `url` that is not a valid URL (`Invalid URL '<url>': <reason>`). - 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. +### 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 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 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. @@ -46,6 +53,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) @@ -188,6 +196,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 @@ -202,6 +242,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. @@ -213,11 +272,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 +308,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 +353,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 +381,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/__init__.py b/src/fetch/src/mcp_server_fetch/__init__.py index 39ac728705..fa3a6eba88 100644 --- a/src/fetch/src/mcp_server_fetch/__init__.py +++ b/src/fetch/src/mcp_server_fetch/__init__.py @@ -1,15 +1,32 @@ +from .allowlist import parse_allowed_host 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", @@ -21,6 +38,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", @@ -29,12 +55,21 @@ 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, 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 *.<domain> " + "(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/readability_js.py b/src/fetch/src/mcp_server_fetch/readability_js.py new file mode 100644 index 0000000000..424778c9ad --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/readability_js.py @@ -0,0 +1,274 @@ +# 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 +# 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. +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": + 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. + 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) + # 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) + + +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..faed1cef40 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, @@ -23,23 +25,34 @@ 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 import httpx +from pydantic import TypeAdapter, ValidationError import markdownify import readabilipy.simple_json +from .readability_js import extract_article 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 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 +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. @@ -168,11 +181,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 +283,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 +298,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 = "<error>Page failed to be simplified from HTML</error>" - 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( @@ -461,6 +489,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 +500,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, @@ -507,6 +540,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. @@ -523,8 +567,10 @@ 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, + allowed_hosts: tuple[str, ...] = (), check_robots_txt: bool = False, use_readability_js: bool = True, ) -> Tuple[str, str]: @@ -546,23 +592,33 @@ 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: hooks.append(_check_redirect_robots_txt) if hooks: client.event_hooks = {"request": hooks} + # 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: response = await client.get( 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}") @@ -574,6 +630,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 = ( "<html" in page_raw[:100] or "text/html" in content_type.lower() @@ -582,7 +642,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 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)" @@ -597,6 +660,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`. @@ -639,6 +706,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,8 +716,10 @@ 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 + 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,20 +773,31 @@ 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) 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( page_url, user_agent_autonomous, force_raw=raw, + timeout_ms=timeout_ms, 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, ) @@ -742,14 +823,26 @@ 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: content, prefix = await fetch_url( url, user_agent_manual, + timeout_ms=default_timeout_ms, 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 +857,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. @@ -772,16 +866,22 @@ 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 - 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, 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/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/src/mcp_server_fetch/title.py b/src/fetch/src/mcp_server_fetch/title.py new file mode 100644 index 0000000000..074387211f --- /dev/null +++ b/src/fetch/src/mcp_server_fetch/title.py @@ -0,0 +1,150 @@ +# 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>" + +# 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_]+") +# Characters that Markdown or inline HTML would interpret in the emitted +# heading ("<T>" 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: + """``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 _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. 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. + """ + out: list[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), + ("](", ")", False), + ): + 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) + 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: + 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(_visible(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 + heading = _MARKDOWN_SPECIAL.sub(r"\\\1", title) + return f"# {heading}\n\n{content.lstrip()}" 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_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_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, "<html><body></body></html>", ""]) 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_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_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_markdown_negotiation.py b/src/fetch/tests/test_markdown_negotiation.py new file mode 100644 index 0000000000..197a90bef7 --- /dev/null +++ b/src/fetch/tests/test_markdown_negotiation.py @@ -0,0 +1,137 @@ +"""Protocol-level tests for asking for native markdown (#5198). + +A fetch used to send httpx's default ``Accept: */*``, so a site that can serve +markdown through content negotiation sent HTML to be simplified, and a +``text/markdown`` response was labeled as content that "cannot be simplified +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 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 + +import httpx +import pytest + +from mcp_server_fetch.server import MARKDOWN_ACCEPT, _is_markdown + +from .harness import FakeWeb, connect + +PAGE = "https://example.com/doc" +ROBOTS = "https://example.com/robots.txt" + +# Opens with an "<html" mention inside the first 100 characters, which the +# HTML sniff would otherwise take for a page to simplify. +MARKDOWN = "# Title\n\nUse an `<html>` tag at the top.\n\n- one\n- two\n" + +ARTICLE = """<html><head><title>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 diff --git a/src/fetch/tests/test_page_title.py b/src/fetch/tests/test_page_title.py new file mode 100644 index 0000000000..be4978a31b --- /dev/null +++ b/src/fetch/tests/test_page_title.py @@ -0,0 +1,225 @@ +"""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: `` 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."), + # 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"), + ], +) +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 + + +@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_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( + ("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. + 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." 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..c91ca82ab4 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 @@ -187,6 +186,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", @@ -999,9 +1007,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, _): @@ -1183,26 +1192,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 # --------------------------------------------------------------------------- diff --git a/src/fetch/tests/test_readability_js_opt_out.py b/src/fetch/tests/test_readability_js_opt_out.py index 05cb77d89b..1a428d04c1 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: @@ -76,7 +79,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( diff --git a/src/fetch/tests/test_readability_subprocess.py b/src/fetch/tests/test_readability_subprocess.py new file mode 100644 index 0000000000..c548032dba --- /dev/null +++ b/src/fetch/tests/test_readability_subprocess.py @@ -0,0 +1,516 @@ +"""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 shutil +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 only place the runner looks for node and npm. + + 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 + + +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] + + +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, +) -> 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 diff --git a/src/fetch/tests/test_timeout.py b/src/fetch/tests/test_timeout.py new file mode 100644 index 0000000000..cdfff77502 --- /dev/null +++ b/src/fetch/tests/test_timeout.py @@ -0,0 +1,287 @@ +"""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, 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: + 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