fix(clipboard): security and correctness hardening from full audit#101
Conversation
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.
github-actions
Bot
added
Awaiting CI
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
github-actions
Bot
added
Awaiting CI
cmeans
added
QA Active
|
[QA] Starting review. Applying |
cmeans
left a comment
cmeans
left a comment
There was a problem hiding this comment.
QA review — round 1
Eight fixes implemented and tested cleanly. Code is solid: nested-table extractor uses depth gating, AppleScript chunking stays under the 32,767-char per-line cap with empty-content fallback, subprocess cleanup uses finally with ProcessLookupError suppression, image subtype allowlist plus base_mime_type strip is the right shape, and the TSV phantom-column guard correctly distinguishes Excel single-cell copies from genuine 1×2 tables.
Findings
| # | Severity | Finding |
|---|---|---|
| F1 | substantive | Test count drift in PR body. Test plan says now 502 passed. Actual is 504 passed, 6 deselected, 5 xfailed — the follow-up commit b718081 (test/coverage) added two tests (test_paste_image_format_strips_mime_parameters, test_macos_write_typed_empty_content) after the body was drafted, and the body was not refreshed. Update the count and check the box. |
| F2 | substantive observation | Image cap is post-read, not stream-bounded. read_clipboard_image runs the backend reader to completion before checking len(result) > _MAX_IMAGE_BYTES. On macOS the full base64 string lives in memory before b64decode; on wayland/x11/windows _run_binary's proc.communicate() reads the entire stdout into memory before the cap fires. The "drop the MCP transport" half of the claim is fully addressed (no inflated payload reaches the wire). The "exhaust memory on constrained hosts" half is overstated — a 1 GB clipboard image still gets fully buffered before the cap rejects it. Either tighten the wording in CHANGELOG.md and the PR Summary to focus on the wire-level mitigation, or bound subprocess output (e.g. read in chunks and abort early) so the constrained-hosts claim holds. Wording fix is the cheaper option and probably the right one for this PR. |
Verification
| Check | Status |
|---|---|
uv run pytest |
504 passed, 6 deselected, 5 xfailed |
uv run pytest tests/test_integration.py -m integration (live wayland) |
6/6 passed |
uv run ruff check src/ tests/ |
clean |
uv run ruff format --check src/ tests/ |
9 files already formatted |
uv run mypy src/ |
clean |
Manual: _safe_code_fence length math |
3/4/5/7 backticks → 3/4/5/7 — correct |
Manual: parse_tsv("word\t") / parse_tsv("\tword") |
both [] — correct |
| Manual: nested HTML extractor (single-level nest) | [['outer', 'after']] — inner cell dropped, outer trailing text preserved |
Manual: _MAX_IMAGE_BYTES default |
10485760 (10 MB) |
Manual: ClipboardSizeError MRO |
subclass of ClipboardError ✓ |
| 6 deselected | tests/test_integration.py integration marker, expected (consistent with prior reviews) |
| 5 xfailed | unchanged from prior baseline |
| CI gates | all green (lint, typecheck, test 3.11/3.12/3.13, codecov/patch, qa-approved) |
Ruff/format/mypy boxes checked; pytest box left unchecked pending F1 update.
Applying QA Failed as the final act per the workflow — F1 needs a body edit and F2 needs either a wording or implementation tweak before signoff.
|
[QA] Verdict: QA Failed. 2 substantive findings (1 PR-body drift, 1 changelog/PR-text overstatement). Both are wording-level fixes; no code changes required for F1, and F2 can be either a wording tightening or an implementation tightening. Once both are addressed, ping for round 2. |
cmeans
added
QA Failed
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.
github-actions
Bot
added
Awaiting CI
|
QA round 1 findings addressed: F1 (test count drift): PR body updated to F2 (image cap wording overstatement): Took the wording-fix path. CHANGELOG Commit: Pinging for round 2. |
cmeans
added
QA Active
|
[QA] Round 2 starting. Re-running gates against fc9a7c5 and re-verifying both F1 (test count) and F2 (image cap wording). |
cmeans
left a comment
cmeans
left a comment
There was a problem hiding this comment.
QA review — round 2
CHANGELOG-only commit at fc9a7c5 addresses the wording overstatement (F2) and the body's test count is now correct (F1). Verification re-run: 504 passed, ruff/format/mypy clean, CI fully green. PR body checkboxes for pytest/ruff/format/mypy all flipped.
One residual issue caught by repo-wide grep:
Findings
| # | Severity | Finding |
|---|---|---|
| F3 | substantive | Same overstatement still in src/mcp_clipboard/clipboard.py:38-40. The CHANGELOG entry was tightened to "wire-level guard rather than a memory-bounded read" and the PR body Summary got a matching caveat, but the source comment that describes _MAX_IMAGE_BYTES still reads # 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. Same wording the CHANGELOG dropped. Update the comment to mirror the new framing — e.g. "…can drop the MCP transport. The cap is a wire-level guard; backend memory is not bounded." Doc-only fix, no code change. |
Verification
| Check | Status |
|---|---|
uv run pytest |
504 passed, 6 deselected, 5 xfailed |
uv run ruff check src/ tests/ |
clean |
uv run ruff format --check src/ tests/ |
9 files already formatted |
uv run mypy src/ |
clean |
| CHANGELOG drift fix | correctly applied — matches PR-body Summary caveat |
Repo-wide grep constrained host|exhaust memory|wire-level|memory-bounded |
1 stale hit in clipboard.py:40 (the F3 comment); CHANGELOG.md lines 111-112 are the new wording, both correct |
| CI gates | all green (lint, typecheck, test 3.11/3.12/3.13, codecov/patch, qa-approved) |
Applying QA Failed as the final act — F3 is the same wording overstatement F2 caught, just in a sibling location. Comment edit, no code, no test changes. Ping for round 3 when fixed.
|
[QA] Verdict: QA Failed (round 2). F1/F2 fixed. F3: same wording overstatement still in src/mcp_clipboard/clipboard.py:38-40 — comment-only fix to mirror the CHANGELOG framing. |
cmeans
removed
the
QA Active
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.
github-actions
Bot
added
Awaiting CI
|
F3 addressed: the Verified repo-wide grep for Commit: Pinging for round 3. |
github-actions
Bot
added
Ready for QA
cmeans
added
QA Active
|
[QA] Round 3 starting. Re-verifying F3 fix at 8a8b36c plus repo-wide grep for residual drift. |
cmeans
left a comment
cmeans
left a comment
There was a problem hiding this comment.
QA review — round 3
F3 closed at 8a8b36c. The source comment for _MAX_IMAGE_BYTES now mirrors the CHANGELOG framing — "wire-level guard; backend memory is not bounded (the full image is still buffered before the size check fires)" — and a repo-wide grep for the old "constrained host" / "exhaust memory" wording comes back empty.
Verification
| Check | Status |
|---|---|
uv run pytest |
504 passed, 6 deselected, 5 xfailed |
uv run ruff check src/ tests/ |
clean |
uv run ruff format --check src/ tests/ |
9 files already formatted |
uv run mypy src/ |
clean |
| F3 wording fix | applied at src/mcp_clipboard/clipboard.py:38-42, mirrors CHANGELOG.md:107-115 |
Repo-wide grep constrained host|exhaust memory |
0 hits |
Repo-wide grep wire-level|memory-bounded |
2 hits (clipboard.py:40, CHANGELOG.md:111-112) — both correct |
| CI gates | all green (lint, typecheck, test 3.11/3.12/3.13, codecov/patch, qa-approved) |
All 3 findings closed across the round 1 / round 2 / round 3 cycle:
- F1 — PR-body test count corrected to 504 ✓
- F2 — CHANGELOG and PR-Summary wording tightened to wire-level framing ✓
- F3 — source comment mirrors the new framing ✓
Applying Ready for QA Signoff as the final act. Awaiting maintainer QA Approved.
|
[QA] Verdict: Ready for QA Signoff. F3 closed; all 3 findings across rounds 1-3 cleared. Gates re-verified at 8a8b36c (504/6/5, ruff/format/mypy clean, CI green). Awaiting maintainer QA Approved. |
cmeans
added
Ready for QA Signoff
#104) ## Summary Closes the four minor audit items left after PRs #101 and #102. All four are nice-to-haves; none are bugs. Bundled into a single PR per the audit synthesis. ### Added 1. **`clipboard.reset_backend_cache()`** public helper. Replaces direct mutation of the module-private `_backend` global in test fixtures (`tests/test_server.py` and `tests/test_integration_x11.py`). Production code has no reason to call this; the helper exists for tests that switch backends or re-read `MCP_CLIPBOARD_BACKEND` mid-process. 2. **Direct `_has_header_row` tests** covering the `len(rows) < 2` early-return path, the uniform-text-no-header path, and a positive text-header-over-numeric-data case. Previously exercised only indirectly via `format_table` JSON output. 3. **Padding-loop regression test** for `clipboard_paste(include_schema=True)` when the data rows are wider than the header row. Asserts synthetic `Col 3` / `Col 4` labels are emitted by the padding loop at `server.py:222-228`. 4. **Env-var validation tests** for `MCP_CLIPBOARD_MAX_WRITE_BYTES` and `MCP_CLIPBOARD_MAX_IMAGE_BYTES`. Both vars are parsed via `int(os.environ.get(...))` at module import; non-integer values raise `ValueError` before anything else runs. Tests exercise each in a subprocess so the partial-import state stays isolated from the in-process module cache. ## Implementation note The env-var-validation tests originally tried `importlib.reload()` in-process, which broke 21 other tests because their `pytest.raises(ClipboardError)` calls compared against the pre-reload class object, while the reloaded module raised the new class. Switched to subprocess-based assertions that capture stderr and verify both the `ValueError` traceback and the offending value appear. ## Test plan - [x] `uv run pytest` (511 passed, 11 deselected, 5 xfailed) - [x] `uv run ruff check src/ tests/` - [x] `uv run ruff format --check src/ tests/` - [x] `uv run mypy src/` ## CHANGELOG - [x] `## [Unreleased]` updated with four `### Added` entries referencing #104. Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com>
Bump pyproject.toml 2.2.1 -> 2.3.0 and convert the [Unreleased] block into [2.3.0] - 2026-05-02. A fresh empty [Unreleased] section sits above for the next cycle. 13 PRs aggregated since v2.2.1: #88, #92, #93, #94, #95, #96, #98, #99, #100, #101, #102, #103, #104. Tag-push (v2.3.0) after merge triggers .github/workflows/publish.yml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Eight fixes surfaced by a four-angle code review (security, correctness, architecture, test coverage). Three are security findings, five are correctness bugs. All include regression tests.
Security
ClipboardSizeErroris raised inread_clipboard_imageand surfaced to the caller viaclipboard_paste. This is a wire-level guard rather than a memory-bounded read: the backend still buffers the full image before the cap fires, so the inflated MCP response is prevented but local backend memory is not bounded.mcp.Image(format=...)now only seespng,jpeg,gif,webp,tiff, orbmp. Clipboard-controlled MIME parameter strings fall back topng.clipboard_paste(JSON, code, RTF branches) are now sized one longer than the longest backtick run inside the wrapped content, so clipboard text containing literal triple backticks cannot break out of the fence and inject Markdown.Correctness
list_formatsdedup.TextandUnicodeTextboth map totext/plain; previous output reported the duplicate. Mirrors the existing macOS dedup._TableExtractor.handle_datanow gates on_table_depth == 1, so an inner-table cell's text no longer leaks into the surrounding outer cell._macos_write_typedchunks the base64 payload across multiple statements (4,000-char chunks) so it stays under AppleScript's 32,767-char per-line limit. Previously broke HTML/RTF writes larger than ~24 KB._run_subprocessand_run_with_stdinnow kill the child in afinallyblock.asyncio.CancelledError(BaseException) bypassed the prior timeout-onlyexceptand could leak the process on MCP client disconnect.parse_tsvrejects single-row results with fewer than two non-empty cells."word\t"(one Excel cell copied on Windows) no longer renders as a misleading 1x2 table.Test plan
uv run pytest(504 passed, 6 deselected, 5 xfailed)uv run ruff check src/ tests/uv run ruff format --check src/ tests/uv run mypy src/CHANGELOG
## [Unreleased]entry under Fixed (5 bullets) and a new Security subsection (3 bullets), all referencing fix(clipboard): security and correctness hardening from full audit #101.