Skip to content

fix(security): reject unknown MIME types in macOS image reader (#24)#47

Merged
cmeans-claude-dev[bot] merged 1 commit into
mainfrom
fix/applescript-injection-24
Apr 12, 2026
Merged

fix(security): reject unknown MIME types in macOS image reader (#24)#47
cmeans-claude-dev[bot] merged 1 commit into
mainfrom
fix/applescript-injection-24

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented Apr 12, 2026

Summary

  • _macos_read_image now rejects MIME types not in the UTI reverse map with ClipboardError instead of interpolating them raw into an AppleScript string literal
  • Eliminates a potential script injection vector via crafted MIME types containing "
  • Supported image types: image/png, image/tiff, image/jpeg (all mapped via _UTI_TO_MIME)

Test plan

  • uv run pytest tests/test_server.py::test_macos_read_image_unknown_mime_rejected -v passes
  • uv run pytest tests/test_server.py::test_macos_read_image_injection_rejected -v passes
  • uv run pytest tests/test_server.py::test_macos_read_image -v still passes (known types work)
  • uv run pytest tests/test_server.py::test_macos_read_image_jpeg_uti -v still passes (UTI mapping works)
  • Full suite: uv run pytest -q -- 429 passed, 11 xfailed

Closes #24

_macos_read_image previously fell back to interpolating raw
caller-supplied MIME types into an AppleScript string literal.
A crafted MIME containing a double-quote could escape the literal.

Now rejects any MIME type not in the UTI reverse map with a
ClipboardError, eliminating the injection vector.

Closes #24

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans cmeans force-pushed the fix/applescript-injection-24 branch from c431007 to f27bbef Compare April 12, 2026 13:01
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 12, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added Ready for QA Dev work complete — QA can begin review and removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels Apr 12, 2026
@cmeans cmeans added QA Active QA is actively reviewing; Dev should not push changes and removed Ready for QA Dev work complete — QA can begin review labels Apr 12, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Review — PR #47

Verdict: One observation. QA Failed pending a PR body edit.

Code review

Fix correctness: The old code uti = mime_to_uti.get(mime_type, mime_type) fell back to raw mime_type as the UTI, which was then interpolated into an AppleScript string literal. A crafted MIME type containing " could break out of the string and inject AppleScript commands. The new code rejects unknown types at the boundary with ClipboardError. Defense in depth — correct fix. ✓

Supported types: Only image/png, image/tiff, image/jpeg (the three types in _UTI_TO_MIME). Narrowing to known types is the right approach for a security fix. ✓

Error message: f"Unsupported image type: {mime_type}" — user input in an exception string, no injection risk in Python. ✓

Tests

Targeted (4/4 pass):

  • test_macos_read_image_unknown_mime_rejectedimage/webp (valid MIME, no UTI mapping) → ClipboardError ✓
  • test_macos_read_image_injection_rejectedimage/png"; -- (injection payload) → ClipboardError ✓
  • test_macos_read_image — known type still works ✓
  • test_macos_read_image_jpeg_uti — UTI mapping still works ✓

Full suite: 429 passed, 11 xfailed, 0 failed, 0 skipped ✓

The old test (test_macos_read_image_unknown_mime_passthrough) that asserted the insecure passthrough behavior is properly replaced — not just removed, but replaced with the rejection + injection tests. Good test evolution. ✓

Other checks

  • CHANGELOG: ### Fixed entry, correct category (security fix), accurate description, Closes #24. ✓
  • CI: test (3.11/3.12/3.13), codecov/patch SUCCESS, QA Gate pending ✓
  • Diff scope: 3 files (clipboard.py, test_server.py, CHANGELOG.md), +19/-13, single commit. Clean. ✓

Finding

F1 (observation) — PR body test count is stale.

PR body says: Full suite: uv run pytest -q -- 240 passed
Actual result: 429 passed, 11 xfailed

The branch includes #44, #45, and #46 content (test_escaping.py is present, all 189+11 escaping tests run). The expected count needs updating.

Fix: Edit PR body checkbox 5 from 240 passed to 429 passed, 11 xfailed. No commit needed.

Merge coordination note

This branch includes #44, #45, #46 content. Adding merge-order: 4 — merge after all three land. The CHANGELOG diff should auto-merge cleanly since the #24 entry appends to the Fixed section whose context lines (#16, #17) are stable.

@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 12, 2026

Applying QA Failed — code and tests are solid, but PR body claims '240 passed' when actual is '429 passed, 11 xfailed' (branch includes #46's escaping matrix). Edit the PR body to fix the count — no commit needed.

@cmeans cmeans added QA Failed QA found issues — needs dev attention and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 12, 2026
@cmeans-claude-dev cmeans-claude-dev Bot added Ready for QA Dev work complete — QA can begin review and removed QA Failed QA found issues — needs dev attention labels Apr 12, 2026
@cmeans cmeans added QA Active QA is actively reviewing; Dev should not push changes and removed Ready for QA Dev work complete — QA can begin review labels Apr 12, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Re-review — PR #47 (round 2)

Verdict: F1 resolved. Zero findings. Ready for maintainer signoff.

F1: fixed ✓

PR body checkbox 5 now says 429 passed, 11 xfailed — matches actual output on head f27bbef.

Re-verification (all 5 checkboxes on new head)

CHECKBOX 1: test_macos_read_image_unknown_mime_rejected  PASSED
CHECKBOX 2: test_macos_read_image_injection_rejected     PASSED
CHECKBOX 3: test_macos_read_image                        PASSED
CHECKBOX 4: test_macos_read_image_jpeg_uti               PASSED
CHECKBOX 5: Full suite — 429 passed, 11 xfailed          PASSED (matches PR body)

Code, tests, CHANGELOG, CI all unchanged from round 1 review (which had zero code findings). Applying Ready for QA Signoff.

@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 12, 2026

Applying Ready for QA Signoff. F1 resolved — PR body count matches actual (429 passed, 11 xfailed). All 5 checkboxes verified on new head f27bbef. Zero findings.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 12, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels Apr 12, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit 8e705c8 into main Apr 12, 2026
57 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the fix/applescript-injection-24 branch April 12, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AppleScript injection risk in _macos_read_image fallback path

1 participant