From 74cd78b94eae9f65aebced421a06a286bc97497d Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 12:36:16 -0500 Subject: [PATCH 1/4] fix(clipboard): security and correctness hardening from full audit Addresses eight findings from a four-angle code review (security, correctness, architecture, test coverage). All changes are local to the clipboard module, parser, and server tool layer. Security: - Cap image reads at 10 MB (configurable via MCP_CLIPBOARD_MAX_IMAGE_BYTES) via new ClipboardSizeError so a large clipboard bitmap cannot exhaust host memory or saturate the MCP transport. - Validate image subtype against an allowlist before passing to mcp.Image(format=...); clipboard-controlled MIME parameter strings now fall back to "png" rather than flowing through verbatim. - Size markdown code fences dynamically so clipboard text containing triple backticks cannot break out of the JSON, code, or RTF fence and inject Markdown. Correctness: - Deduplicate Windows list_formats output (Text and UnicodeText both map to text/plain), matching the existing macOS behavior. - Stop nested HTML tables from leaking inner-cell text into the outer cell by gating handle_data on _table_depth == 1. - Chunk base64 across multiple AppleScript statements to stay under AppleScript's 32,767-character per-line limit; previously broke for HTML/RTF writes larger than ~24 KB. - Add finally-block kill in _run_subprocess and _run_with_stdin so a CancelledError (BaseException, not caught by the timeout-only except) cannot orphan the subprocess. - Reject single-cell parse_tsv input (e.g. "word\t" from one-cell Excel copies on Windows) instead of presenting a 1x2 table with a phantom empty column. All eight fixes have regression tests; the existing 487-test suite still passes (502 total now), ruff and mypy clean. --- CHANGELOG.md | 39 ++++++ src/mcp_clipboard/clipboard.py | 159 +++++++++++++++++------- src/mcp_clipboard/parser.py | 13 +- src/mcp_clipboard/server.py | 42 ++++++- tests/test_parser.py | 23 ++++ tests/test_server.py | 214 ++++++++++++++++++++++++++++++++- 6 files changed, 437 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7094c2..3fbb69a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,45 @@ All notable changes to this project will be documented here. and the queue-time parser rejects the empty form on `workflow_dispatch`. The `workflow_run` path tolerated it, so the bug was latent here but blocked manual dispatch and fresh-repo cascades. Closes #91. +- Windows clipboard `list_formats` now deduplicates MIME types when multiple + native format names map to the same MIME (e.g. `Text` and `UnicodeText` + both map to `text/plain`), matching the existing macOS behavior. Prevents + inflated format counts and downstream duplicate iteration. (#101) +- Nested HTML tables no longer leak inner-table cell text into the outer + cell. `_TableExtractor.handle_data` now gates on `_table_depth == 1` so + text inside an inner `
...
` is no longer + concatenated into the surrounding outer cell. (#101) +- macOS `_macos_write_typed` no longer trips AppleScript's 32,767-character + per-line limit when writing HTML or RTF content larger than ~24 KB. The + base64-encoded payload is split across multiple `set b64 to b64 & "..."` + statements with a 4,000-character chunk size. (#101) +- `_run_subprocess` and `_run_with_stdin` no longer orphan their child + process when the calling task is cancelled (e.g. on MCP client + disconnect). A `finally` block now calls `proc.kill()` on any non-normal + exit including `asyncio.CancelledError`, which inherits from + `BaseException` and previously bypassed the timeout-only `except` + handler. (#101) +- `parse_tsv` no longer treats single-cell input with a stray tab + (`"word\t"`, commonly produced when copying one Excel cell on Windows) + as a 1x2 table with a phantom empty column. Single-row results now + require at least two non-empty cells. (#101) + +### Security +- New `MCP_CLIPBOARD_MAX_IMAGE_BYTES` cap (default 10 MB) on + `read_clipboard_image`. A 100 MB clipboard bitmap previously became + ~133 MB base64 in a single MCP response, which could exhaust memory on + constrained hosts or drop the MCP transport. Oversized reads now raise + the new `ClipboardSizeError`, and `clipboard_paste` returns an + explanatory message instead of forwarding the payload. (#101) +- Image subtype passed to `mcp.Image(format=...)` is now validated against + an allowlist (`png`, `jpeg`, `gif`, `webp`, `tiff`, `bmp`). Clipboard- + controlled MIME strings with parameter injection or unexpected subtypes + fall back to `png` rather than flowing through to the host. (#101) +- Markdown code fences in `clipboard_paste` (JSON, code, RTF branches) now + size dynamically to one longer than the longest backtick run inside the + wrapped content. Prevents clipboard text containing literal triple + backticks from closing the fence early and rendering injected content + as Markdown (or HTML on permissive hosts). (#101) ## [2.2.1] - 2026-04-16 diff --git a/src/mcp_clipboard/clipboard.py b/src/mcp_clipboard/clipboard.py index c106a5a..45ef7b8 100644 --- a/src/mcp_clipboard/clipboard.py +++ b/src/mcp_clipboard/clipboard.py @@ -26,6 +26,21 @@ class ClipboardError(Exception): """Raised when clipboard access fails.""" +class ClipboardSizeError(ClipboardError): + """Raised when clipboard content exceeds a configured size cap.""" + + +# AppleScript source has a 32,767-character per-line limit. Chunk size for +# base64 string literals must stay well under that to leave headroom for +# the surrounding `set b64 to "..."` syntax. +_APPLESCRIPT_CHUNK = 4000 + +# Cap on image read size. A large clipboard bitmap (e.g. 100 MB uncompressed +# TIFF screenshot) becomes ~133 MB base64 in a single MCP response and can +# exhaust memory on constrained hosts. Configurable via env var. +_MAX_IMAGE_BYTES = int(os.environ.get("MCP_CLIPBOARD_MAX_IMAGE_BYTES", 10 * 1024 * 1024)) + + def base_mime_type(mime: str) -> str: """Strip parameters from a MIME type string. @@ -50,29 +65,40 @@ async def _run_subprocess( available"). Set it to ``False`` for macOS and Windows backends where exit code 1 indicates a real error (script failure, permission denied, etc.). """ + proc: asyncio.subprocess.Process | None = None try: - proc = await asyncio.create_subprocess_exec( - *cmd, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=env, - ) - stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=timeout) - except FileNotFoundError as fnf: - raise ClipboardError(f"Command not found: {cmd[0]}") from fnf - except TimeoutError as te: - proc.kill() - with contextlib.suppress(TimeoutError): - await asyncio.wait_for(proc.wait(), timeout=1.0) - raise ClipboardError(f"Clipboard command timed out: {' '.join(cmd)}") from te - - if proc.returncode != 0: - if allow_empty_exit and proc.returncode == 1: - return b"" - err = stderr.decode(errors="replace").strip() - raise ClipboardError(f"Clipboard command failed (rc={proc.returncode}): {err}") - - return stdout + try: + proc = await asyncio.create_subprocess_exec( + *cmd, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=env, + ) + except FileNotFoundError as fnf: + raise ClipboardError(f"Command not found: {cmd[0]}") from fnf + try: + stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=timeout) + except TimeoutError as te: + proc.kill() + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(proc.wait(), timeout=1.0) + raise ClipboardError(f"Clipboard command timed out: {' '.join(cmd)}") from te + + if proc.returncode != 0: + if allow_empty_exit and proc.returncode == 1: + return b"" + err = stderr.decode(errors="replace").strip() + raise ClipboardError(f"Clipboard command failed (rc={proc.returncode}): {err}") + + return stdout + finally: + # Belt-and-suspenders cleanup for paths that bypass the explicit + # kill above -- specifically asyncio.CancelledError (BaseException) + # from a cancelled MCP request, which would otherwise orphan the + # subprocess. kill() is a no-op once the process has exited. + if proc is not None and proc.returncode is None: + with contextlib.suppress(ProcessLookupError): + proc.kill() async def _run( @@ -116,28 +142,39 @@ async def _run_with_stdin( """ debug = os.environ.get("MCP_CLIPBOARD_DEBUG", "") == "1" stderr_mode = asyncio.subprocess.PIPE if debug else asyncio.subprocess.DEVNULL + proc: asyncio.subprocess.Process | None = None try: - proc = await asyncio.create_subprocess_exec( - *cmd, - stdin=asyncio.subprocess.PIPE, - stdout=asyncio.subprocess.DEVNULL, - stderr=stderr_mode, - env=env, - ) - _, stderr_data = await asyncio.wait_for(proc.communicate(input=input_data), timeout=timeout) - except FileNotFoundError as fnf: - raise ClipboardError(f"Command not found: {cmd[0]}") from fnf - except TimeoutError as te: - proc.kill() - with contextlib.suppress(TimeoutError): - await asyncio.wait_for(proc.wait(), timeout=1.0) - raise ClipboardError(f"Clipboard command timed out: {' '.join(cmd)}") from te - - if proc.returncode != 0: - msg = f"Clipboard write failed (rc={proc.returncode}): {cmd[0]}" - if debug and stderr_data: - msg += f"\nstderr: {stderr_data.decode(errors='replace').strip()}" - raise ClipboardError(msg) + try: + proc = await asyncio.create_subprocess_exec( + *cmd, + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.DEVNULL, + stderr=stderr_mode, + env=env, + ) + except FileNotFoundError as fnf: + raise ClipboardError(f"Command not found: {cmd[0]}") from fnf + try: + _, stderr_data = await asyncio.wait_for( + proc.communicate(input=input_data), timeout=timeout + ) + except TimeoutError as te: + proc.kill() + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(proc.wait(), timeout=1.0) + raise ClipboardError(f"Clipboard command timed out: {' '.join(cmd)}") from te + + if proc.returncode != 0: + msg = f"Clipboard write failed (rc={proc.returncode}): {cmd[0]}" + if debug and stderr_data: + msg += f"\nstderr: {stderr_data.decode(errors='replace').strip()}" + raise ClipboardError(msg) + finally: + # See _run_subprocess: belt-and-suspenders cleanup for the + # CancelledError path which bypasses except handlers entirely. + if proc is not None and proc.returncode is None: + with contextlib.suppress(ProcessLookupError): + proc.kill() def _find_wayland_display() -> str | None: @@ -390,7 +427,16 @@ async def _windows_list_formats() -> list[str]: ) raw = await _run(["powershell", "-NoProfile", "-Command", script], allow_empty_exit=False) native = [line.strip() for line in raw.splitlines() if line.strip()] - return [_WIN_TO_MIME.get(f, f) for f in native] + # Deduplicate: Windows clipboards routinely expose both "Text" and + # "UnicodeText" which both map to text/plain. Mirror _macos_list_formats. + seen: set[str] = set() + result: list[str] = [] + for f in native: + mime = _WIN_TO_MIME.get(f, f) + if mime not in seen: + seen.add(mime) + result.append(mime) + return result # --------------------------------------------------------------------------- @@ -513,11 +559,23 @@ async def _macos_write_typed(content: str, mime_type: str) -> None: if mime_type in ("text/html", "text/rtf"): uti = "public.html" if mime_type == "text/html" else "public.rtf" b64 = base64.b64encode(content.encode("utf-8")).decode("ascii") + # AppleScript has a 32,767-char per-line limit, so a single + # `set b64 to "..."` literal breaks for content >~24 KB once + # base64-encoded. Split the literal across multiple statements. + b64_chunks = [ + b64[i : i + _APPLESCRIPT_CHUNK] for i in range(0, len(b64), _APPLESCRIPT_CHUNK) + ] + if not b64_chunks: + b64_chunks = [""] + b64_lines = [f'set b64 to "{b64_chunks[0]}"'] + for chunk in b64_chunks[1:]: + b64_lines.append(f'set b64 to b64 & "{chunk}"') script = ( 'use framework "AppKit"\n' 'use framework "Foundation"\n' - f'set b64 to "{b64}"\n' - "set decoded to (current application's NSData's alloc()'s " + + "\n".join(b64_lines) + + "\n" + + "set decoded to (current application's NSData's alloc()'s " "initWithBase64EncodedString:b64 options:0)\n" "set pb to current application's NSPasteboard's generalPasteboard()\n" "pb's clearContents()\n" @@ -721,6 +779,9 @@ async def read_clipboard_image(mime_type: str = "image/png") -> bytes: Like :func:`read_clipboard`, falls back to a matching suffixed MIME type when the exact requested type is not available. + + Raises :exc:`ClipboardSizeError` when the image exceeds + ``MCP_CLIPBOARD_MAX_IMAGE_BYTES`` (default 10 MB). """ backend = _get_backend() result = await _IMAGE_READERS[backend](mime_type) @@ -734,6 +795,12 @@ async def read_clipboard_image(mime_type: str = "image/png") -> bytes: if result: break + if len(result) > _MAX_IMAGE_BYTES: + raise ClipboardSizeError( + f"Image exceeds clipboard read limit " + f"({len(result):,} bytes, max {_MAX_IMAGE_BYTES:,}). " + f"Set MCP_CLIPBOARD_MAX_IMAGE_BYTES to increase." + ) return result diff --git a/src/mcp_clipboard/parser.py b/src/mcp_clipboard/parser.py index b11717e..2d86702 100644 --- a/src/mcp_clipboard/parser.py +++ b/src/mcp_clipboard/parser.py @@ -59,7 +59,10 @@ def handle_endtag(self, tag: str) -> None: self._current_row = None def handle_data(self, data: str) -> None: - if self._current_cell is not None: + # Only collect text at the outer table's depth: a nested table inside + # a cell would otherwise leak its inner-cell text into the outer cell + # because _current_cell is still set while inner / are skipped. + if self._current_cell is not None and self._table_depth == 1: self._current_cell.append(data) @@ -90,6 +93,14 @@ def parse_tsv(text: str) -> list[list[str]]: for row in reader: if any(cell.strip() for cell in row): rows.append(row) + + # Reject a single row with fewer than two non-empty cells: copying a + # single Excel cell on Windows commonly produces "word\t" which would + # otherwise be rendered as a misleading 1x2 table with a phantom + # empty column. + if len(rows) == 1 and sum(1 for c in rows[0] if c.strip()) < 2: + return [] + return rows diff --git a/src/mcp_clipboard/server.py b/src/mcp_clipboard/server.py index 7d8b058..125715f 100644 --- a/src/mcp_clipboard/server.py +++ b/src/mcp_clipboard/server.py @@ -21,6 +21,7 @@ from .clipboard import ( ClipboardError, + ClipboardSizeError, base_mime_type, list_clipboard_formats, read_clipboard, @@ -102,6 +103,12 @@ def _load_icons() -> list[Icon]: _BINARY_MIME_PREFIXES = ("image/", "audio/", "video/") _BINARY_MIME_EXACT = frozenset({"application/octet-stream"}) +# Whitelist of image subtypes recognized as a safe `format=` value to pass +# to mcp.Image. Anything else (including parameter-laden or malformed +# clipboard-controlled MIME strings) falls back to "png" so the host +# never sees an unexpected format string. +_IMAGE_SUBTYPE_ALLOWLIST = frozenset({"png", "jpeg", "gif", "webp", "tiff", "bmp"}) + # image/* entries that are text-readable (not actual binary). _TEXT_READABLE_MIMES = frozenset({"image/svg+xml"}) @@ -109,6 +116,25 @@ def _load_icons() -> list[Icon]: _MIME_RE = re.compile(r"^[a-zA-Z][\w.+\-]*/[a-zA-Z][\w.+\-]*(;\s*[\w.+\-]+=[\w.+\-]+)*$") +def _safe_code_fence(text: str) -> str: + """Return a backtick fence long enough to wrap ``text`` without escape. + + Markdown spec: a fenced code block can only be closed by a fence at + least as long as the opening fence. Pick a fence one longer than any + backtick run inside the content; minimum length 3. + """ + longest = 0 + current = 0 + for ch in text: + if ch == "`": + current += 1 + if current > longest: + longest = current + else: + current = 0 + return "`" * max(3, longest + 1) + + async def _read_clipboard_content() -> tuple[list[list[str]], str, str]: """Read clipboard and attempt to extract tabular data. @@ -158,13 +184,16 @@ def _format_non_tabular(text: str) -> str: try: parsed = json.loads(text.strip()) formatted = json.dumps(parsed, indent=2, ensure_ascii=False) - result = f"Clipboard contains JSON:\n\n```json\n{formatted}\n```" + fence = _safe_code_fence(formatted) + result = f"Clipboard contains JSON:\n\n{fence}json\n{formatted}\n{fence}" except (json.JSONDecodeError, ValueError): result = f"Clipboard content:\n\n{text}" elif content_type == "url": result = f"Clipboard contains URL:\n\n{text.strip()}" elif content_type == "code": - result = f"Clipboard contains code:\n\n```\n{text.rstrip()}\n```" + body = text.rstrip() + fence = _safe_code_fence(body) + result = f"Clipboard contains code:\n\n{fence}\n{body}\n{fence}" else: result = f"Clipboard content:\n\n{text}" @@ -246,7 +275,8 @@ async def clipboard_paste( if rtf.strip(): truncated = len(rtf) > _MAX_CONTENT_CHARS display = rtf[:_MAX_CONTENT_CHARS] - result = f"Clipboard contains rich text (RTF):\n\n```\n{display}\n```" + fence = _safe_code_fence(display) + result = f"Clipboard contains rich text (RTF):\n\n{fence}\n{display}\n{fence}" if truncated: result += f"\n\n... [truncated at {_MAX_CONTENT_CHARS:,} characters]" return result @@ -267,8 +297,12 @@ async def clipboard_paste( try: data = await read_clipboard_image(mime) if data: - fmt = base_mime_type(mime).split("/", 1)[1] + fmt = base_mime_type(mime).split("/", 1)[1].lower() + if fmt not in _IMAGE_SUBTYPE_ALLOWLIST: + fmt = "png" return Image(data=data, format=fmt) + except ClipboardSizeError as e: + return f"Clipboard image too large to return: {e}" except ClipboardError as e: logger.debug("Image read failed: %s", e) # Non-image binary (audio/video) — report but can't return diff --git a/tests/test_parser.py b/tests/test_parser.py index 1fb795a..8dec0d9 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -77,6 +77,16 @@ def test_parse_html_empty(): assert rows == [] +def test_parse_html_nested_table_isolates_outer(): + """Inner-table text must not leak into the outer cell.""" + html = ( + "
outer
inner
after
" + ) + rows = parse_html_table(html) + assert len(rows) == 1 + assert rows[0] == ["outer", "after"] + + # --------------------------------------------------------------------------- # TSV parsing # --------------------------------------------------------------------------- @@ -101,6 +111,19 @@ def test_parse_tsv_single_column(): assert rows == [] +def test_parse_tsv_single_cell_trailing_tab(): + """A single non-empty cell with a trailing tab is not a table.""" + # Common output from copying one cell out of Excel on Windows. + rows = parse_tsv("word\t") + assert rows == [] + + +def test_parse_tsv_single_cell_leading_tab(): + """A single non-empty cell preceded by a tab is not a table.""" + rows = parse_tsv("\tword") + assert rows == [] + + def test_parse_tsv_quoted_tab(): """A quoted field containing a tab should not split the cell.""" text = '"has\ttab"\tplain' diff --git a/tests/test_server.py b/tests/test_server.py index bad74e6..829b8f6 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -13,6 +13,7 @@ from mcp_clipboard.clipboard import ( ClipboardError, + ClipboardSizeError, _detect_backend, _find_wayland_display, _macos_write_typed, @@ -321,6 +322,34 @@ async def test_paste_code_snippet(): assert "def hello():" in result +@pytest.mark.asyncio +async def test_paste_code_with_triple_backticks_uses_longer_fence(): + """Code containing ``` must not break out of its enclosing markdown fence.""" + # Strong code pattern (def + parens) so detect_content_type returns "code". + code = "def f():\n return '```evil```'" + with patch("mcp_clipboard.server.read_clipboard", side_effect=_mock_read(html="", text=code)): + result = await clipboard_paste() + + assert "```evil```" in result # content preserved verbatim + # Opening fence must be at least 4 backticks (longer than the embedded run). + assert "````\n" in result + + +@pytest.mark.asyncio +async def test_paste_json_with_triple_backticks_in_string_uses_longer_fence(): + """JSON whose string values contain ``` must not break out of the json fence.""" + json_text = '{"snippet": "look: ```bad```"}' + with patch( + "mcp_clipboard.server.read_clipboard", + side_effect=_mock_read(html="", text=json_text), + ): + result = await clipboard_paste() + + assert "Clipboard contains JSON:" in result + assert "```bad```" in result + assert "````json\n" in result + + @pytest.mark.asyncio async def test_paste_html_without_table(): """clipboard_paste extracts text from HTML that has no table.""" @@ -889,12 +918,13 @@ async def test_windows_read_rtf(): @pytest.mark.asyncio async def test_windows_list_formats_maps_names_to_mime(): - """_windows_list_formats maps known Windows format names to MIME types.""" + """_windows_list_formats maps known Windows format names to MIME types and + deduplicates collisions (Text and UnicodeText both map to text/plain).""" raw_output = "HTML Format\nText\nUnicodeText\nPNG\n" with patch("mcp_clipboard.clipboard._run", new_callable=AsyncMock, return_value=raw_output): result = await _windows_list_formats() - assert result == ["text/html", "text/plain", "text/plain", "image/png"] + assert result == ["text/html", "text/plain", "image/png"] @pytest.mark.asyncio @@ -907,6 +937,16 @@ async def test_windows_list_formats_passthrough_unknown(): assert result == ["text/html", "System.String"] +@pytest.mark.asyncio +async def test_windows_list_formats_deduplicates_mime_types(): + """Multiple native names mapping to the same MIME collapse to one entry.""" + raw_output = "Text\nUnicodeText\nText\nHTML Format\n" + with patch("mcp_clipboard.clipboard._run", new_callable=AsyncMock, return_value=raw_output): + result = await _windows_list_formats() + + assert result == ["text/plain", "text/html"] + + # --------------------------------------------------------------------------- # 10. X11 backend # --------------------------------------------------------------------------- @@ -1228,6 +1268,59 @@ async def test_paste_image_empty_data(): assert isinstance(result, str) +@pytest.mark.asyncio +async def test_read_clipboard_image_size_cap(): + """read_clipboard_image raises ClipboardSizeError when bytes exceed cap.""" + huge = b"\x89PNG" + (b"\x00" * (11 * 1024 * 1024)) # > 10 MB default cap + mock_reader = AsyncMock(return_value=huge) + with patch("mcp_clipboard.clipboard._get_backend", return_value="wayland"): + with patch.dict("mcp_clipboard.clipboard._IMAGE_READERS", {"wayland": mock_reader}): + with pytest.raises(ClipboardSizeError, match="exceeds clipboard read limit"): + await read_clipboard_image("image/png") + + +@pytest.mark.asyncio +async def test_paste_image_format_unknown_subtype_falls_back_to_png(): + """A weird clipboard-controlled MIME subtype must not flow into Image(format=...).""" + fake_data = b"\x89PNG\x00\x00\x00" + weird_mime = 'image/png; injected="oops"' + with patch("mcp_clipboard.server.read_clipboard", _mock_read(html="", text="")): + with patch("mcp_clipboard.server.list_clipboard_formats", return_value=[weird_mime]): + with patch("mcp_clipboard.server.read_clipboard_image", return_value=fake_data): + result = await clipboard_paste() + + assert isinstance(result, Image) + assert result._format == "png" + + +@pytest.mark.asyncio +async def test_paste_image_format_uppercase_subtype_normalizes(): + """Image subtype is case-insensitive when matching the allowlist.""" + fake_data = b"GIF89a" + with patch("mcp_clipboard.server.read_clipboard", _mock_read(html="", text="")): + with patch("mcp_clipboard.server.list_clipboard_formats", return_value=["image/GIF"]): + with patch("mcp_clipboard.server.read_clipboard_image", return_value=fake_data): + result = await clipboard_paste() + + assert isinstance(result, Image) + assert result._format == "gif" + + +@pytest.mark.asyncio +async def test_paste_image_too_large_returns_message(): + """clipboard_paste surfaces an explanatory message when image exceeds cap.""" + err = ClipboardSizeError( + "Image exceeds clipboard read limit (12,000,000 bytes, max 10,485,760)." + ) + with patch("mcp_clipboard.server.read_clipboard", _mock_read(html="", text="")): + with patch("mcp_clipboard.server.list_clipboard_formats", return_value=["image/png"]): + with patch("mcp_clipboard.server.read_clipboard_image", side_effect=err): + result = await clipboard_paste() + + assert isinstance(result, str) + assert "too large" in result.lower() + + # --------------------------------------------------------------------------- # 15. Image read backends # --------------------------------------------------------------------------- @@ -1665,6 +1758,88 @@ async def fake_create(*_args, **_kwargs): assert elapsed < 2.5, f"bounded wait not enforced: elapsed {elapsed:.2f}s" +@pytest.mark.asyncio +async def test_run_subprocess_kills_on_cancellation(): + """asyncio.CancelledError must trigger proc.kill() to avoid orphans. + + CancelledError inherits from BaseException, not Exception, so a bare + `except Exception` would skip it. The fix relies on a `finally` block. + """ + from mcp_clipboard.clipboard import _run_subprocess + + class FakeProc: + def __init__(self): + self.kill_called = False + self.returncode: int | None = None + + async def communicate(self): + await asyncio.sleep(10) + return b"", b"" + + def kill(self): + self.kill_called = True + self.returncode = -9 + + async def wait(self): + return self.returncode + + fake = FakeProc() + + async def fake_create(*_args, **_kwargs): + return fake + + async def runner(): + with patch("mcp_clipboard.clipboard.asyncio.create_subprocess_exec", fake_create): + await _run_subprocess(["sleep", "10"], timeout=10.0) + + task = asyncio.create_task(runner()) + await asyncio.sleep(0.05) # let the task enter communicate() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert fake.kill_called, "kill() must be called when the task is cancelled" + + +@pytest.mark.asyncio +async def test_run_with_stdin_kills_on_cancellation(): + """asyncio.CancelledError must trigger proc.kill() in _run_with_stdin too.""" + from mcp_clipboard.clipboard import _run_with_stdin + + class FakeProc: + def __init__(self): + self.kill_called = False + self.returncode: int | None = None + + async def communicate(self, input=None): + await asyncio.sleep(10) + return b"", b"" + + def kill(self): + self.kill_called = True + self.returncode = -9 + + async def wait(self): + return self.returncode + + fake = FakeProc() + + async def fake_create(*_args, **_kwargs): + return fake + + async def runner(): + with patch("mcp_clipboard.clipboard.asyncio.create_subprocess_exec", fake_create): + await _run_with_stdin(["sleep", "10"], b"data", timeout=10.0) + + task = asyncio.create_task(runner()) + await asyncio.sleep(0.05) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert fake.kill_called, "kill() must be called when the task is cancelled" + + @pytest.mark.asyncio async def test_run_binary_exit_code_1_returns_empty(): """_run_binary returns empty bytes for exit code 1 (format not available).""" @@ -1909,6 +2084,24 @@ async def mock_read(mime_type="text/plain"): assert rtf_content in result +@pytest.mark.asyncio +async def test_paste_rtf_with_triple_backticks_uses_longer_fence(): + """RTF content containing ``` must not break out of the markdown fence.""" + rtf_content = r"{\rtf1\ansi look: ```escape```}" + + async def mock_read(mime_type="text/plain"): + if mime_type == "text/rtf": + return rtf_content + return "" + + with patch("mcp_clipboard.server.read_clipboard", side_effect=mock_read): + with patch("mcp_clipboard.server.list_clipboard_formats", return_value=["text/rtf"]): + result = await clipboard_paste() + + assert "```escape```" in result + assert "````\n" in result + + @pytest.mark.asyncio async def test_paste_rtf_truncated(): """clipboard_paste truncates oversized RTF at 50KB.""" @@ -2385,6 +2578,23 @@ async def test_macos_write_typed_unsupported(): await _macos_write_typed("data", "text/csv") +@pytest.mark.asyncio +async def test_macos_write_typed_large_content_chunks_base64(): + """Large HTML content must split base64 across multiple AppleScript + statements so no single line exceeds AppleScript's 32,767-char limit.""" + # 100 KB of HTML -> ~133 KB base64; well over the per-line cap + large_html = "

" + ("X" * 100_000) + "

" + with patch("mcp_clipboard.clipboard._run", new_callable=AsyncMock) as mock: + await _macos_write_typed(large_html, "text/html") + + script = mock.call_args[0][0][-1] + # Split into AppleScript source lines and check each stays under the limit. + for line in script.split("\n"): + assert len(line) < 32_767, f"AppleScript line exceeds limit: {len(line)} chars" + # Concatenation must be present (proves the chunking happened). + assert "set b64 to b64 &" in script + + @pytest.mark.asyncio async def test_windows_write_typed_plain(): """_windows_write_typed uses Set-Clipboard for text/plain.""" From b71808130fe981992abe5e3ed6c7b7ea7f7212bf Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 12:42:33 -0500 Subject: [PATCH 2/4] test(coverage): cover image-subtype fallback and empty AppleScript b64 Codecov flagged two patch lines as uncovered in PR #101: - server.py:302 (the explicit `fmt = "png"` fallback when the clipboard reports an image MIME outside _IMAGE_SUBTYPE_ALLOWLIST) - clipboard.py:569 (the `b64_chunks = [""]` guard that handles empty content in _macos_write_typed) The existing parameter-stripping test never reached the fallback because base_mime_type strips parameters first, leaving "png" (in the allowlist). Added a non-allowlisted subtype (image/x-icon) test that actually hits line 302, plus a separate test that exercises the parameter-stripping behavior for documentation. Added an empty-HTML write test for the b64 chunk-list fallback on macOS. --- tests/test_server.py | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/tests/test_server.py b/tests/test_server.py index 829b8f6..48d9d0c 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1281,9 +1281,13 @@ async def test_read_clipboard_image_size_cap(): @pytest.mark.asyncio async def test_paste_image_format_unknown_subtype_falls_back_to_png(): - """A weird clipboard-controlled MIME subtype must not flow into Image(format=...).""" + """A subtype outside the allowlist must not flow into Image(format=...).""" fake_data = b"\x89PNG\x00\x00\x00" - weird_mime = 'image/png; injected="oops"' + # x-icon is a real image MIME but not in _IMAGE_SUBTYPE_ALLOWLIST; this + # exercises the explicit `fmt = "png"` fallback. (Parameter forms like + # 'image/png; injected="oops"' get stripped by base_mime_type before + # subtype extraction, so they never reach the fallback assignment.) + weird_mime = "image/x-icon" with patch("mcp_clipboard.server.read_clipboard", _mock_read(html="", text="")): with patch("mcp_clipboard.server.list_clipboard_formats", return_value=[weird_mime]): with patch("mcp_clipboard.server.read_clipboard_image", return_value=fake_data): @@ -1293,6 +1297,22 @@ async def test_paste_image_format_unknown_subtype_falls_back_to_png(): assert result._format == "png" +@pytest.mark.asyncio +async def test_paste_image_format_strips_mime_parameters(): + """MIME parameters (e.g. ;charset=...) must be stripped before subtype check.""" + fake_data = b"\x89PNG\x00\x00\x00" + with patch("mcp_clipboard.server.read_clipboard", _mock_read(html="", text="")): + with patch( + "mcp_clipboard.server.list_clipboard_formats", + return_value=['image/png; injected="oops"'], + ): + with patch("mcp_clipboard.server.read_clipboard_image", return_value=fake_data): + result = await clipboard_paste() + + assert isinstance(result, Image) + assert result._format == "png" + + @pytest.mark.asyncio async def test_paste_image_format_uppercase_subtype_normalizes(): """Image subtype is case-insensitive when matching the allowlist.""" @@ -2578,6 +2598,19 @@ async def test_macos_write_typed_unsupported(): await _macos_write_typed("data", "text/csv") +@pytest.mark.asyncio +async def test_macos_write_typed_empty_content(): + """Empty HTML content must still produce a valid (empty-b64) AppleScript.""" + with patch("mcp_clipboard.clipboard._run", new_callable=AsyncMock) as mock: + await _macos_write_typed("", "text/html") + + script = mock.call_args[0][0][-1] + # range(0, 0, _APPLESCRIPT_CHUNK) is empty, so the chunk-list fallback + # to [""] must keep the script syntactically valid. + assert 'set b64 to ""' in script + assert "public.html" in script + + @pytest.mark.asyncio async def test_macos_write_typed_large_content_chunks_base64(): """Large HTML content must split base64 across multiple AppleScript From fc9a7c5fa2129d3f07d07463d6ff31097288a695 Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 12:57:32 -0500 Subject: [PATCH 3/4] docs(changelog): clarify image cap is wire-level, not memory-bounded QA flagged that the original "exhaust memory on constrained hosts" claim was overstated: read_clipboard_image fully buffers the bitmap before the size check fires (macOS holds the base64 string before b64decode; wayland/x11/windows _run_binary's proc.communicate() reads the entire stdout into memory). The cap is therefore a wire-level mitigation -- it prevents the inflated MCP response from going out -- but doesn't bound the local backend buffer. Reword to reflect that. --- CHANGELOG.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fbb69a..f77187a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,10 +105,14 @@ All notable changes to this project will be documented here. ### Security - New `MCP_CLIPBOARD_MAX_IMAGE_BYTES` cap (default 10 MB) on `read_clipboard_image`. A 100 MB clipboard bitmap previously became - ~133 MB base64 in a single MCP response, which could exhaust memory on - constrained hosts or drop the MCP transport. Oversized reads now raise - the new `ClipboardSizeError`, and `clipboard_paste` returns an - explanatory message instead of forwarding the payload. (#101) + ~133 MB base64 in a single MCP response and could time out or drop + the MCP transport. Oversized reads now raise the new + `ClipboardSizeError`, and `clipboard_paste` returns an explanatory + message instead of forwarding the payload. The cap is a wire-level + guard rather than a memory-bounded read: the backend still buffers + the full image before the size check, so the inflated wire response + is prevented but local memory pressure on the host running the + server is not. (#101) - Image subtype passed to `mcp.Image(format=...)` is now validated against an allowlist (`png`, `jpeg`, `gif`, `webp`, `tiff`, `bmp`). Clipboard- controlled MIME strings with parameter injection or unexpected subtypes From 8a8b36cf8aa2d37fb95c0a3ff65826bd148956ef Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 13:04:12 -0500 Subject: [PATCH 4/4] docs(clipboard): mirror wire-level wording in _MAX_IMAGE_BYTES comment QA round 2 (F3) caught that the source comment for _MAX_IMAGE_BYTES still carried the old "exhaust memory on constrained hosts" framing that the CHANGELOG and PR Summary already moved away from in fc9a7c5. Reword the comment to match: the cap is a wire-level guard; the full image is still buffered before the size check fires, so backend memory is not bounded. Repo-wide grep for "constrained host" / "exhaust memory" now empty across src/, tests/, and CHANGELOG.md. --- src/mcp_clipboard/clipboard.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/mcp_clipboard/clipboard.py b/src/mcp_clipboard/clipboard.py index 45ef7b8..b3436df 100644 --- a/src/mcp_clipboard/clipboard.py +++ b/src/mcp_clipboard/clipboard.py @@ -37,7 +37,9 @@ class ClipboardSizeError(ClipboardError): # Cap on image read size. A large clipboard bitmap (e.g. 100 MB uncompressed # TIFF screenshot) becomes ~133 MB base64 in a single MCP response and can -# exhaust memory on constrained hosts. Configurable via env var. +# time out or drop the MCP transport. The cap is a wire-level guard; +# backend memory is not bounded (the full image is still buffered before +# the size check fires). Configurable via env var. _MAX_IMAGE_BYTES = int(os.environ.get("MCP_CLIPBOARD_MAX_IMAGE_BYTES", 10 * 1024 * 1024))