Skip to content

fix(screen-capture): sanitize display index (CodeQL #33 RCE) - #485

Merged
sonichi merged 1 commit into
mainfrom
fix/screen-capture-command-injection
Apr 20, 2026
Merged

sonichi merged 1 commit into
mainfrom
fix/screen-capture-command-injection

Conversation

@sonichi

@sonichi sonichi commented Apr 20, 2026

Copy link
Copy Markdown
Owner

Summary

  • CodeQL alert #33 (py/command-line-injection, severity error): the display query parameter flowed from user input to subprocess.run's argv in src/screen-capture-server.py:124.
  • Existing isdigit() guard was a real sanitizer, but CodeQL's taint analysis didn't recognize it.
  • Fix: coerce to int (recognized sanitizer) + constrain to 1..9 (no Mac has more than a handful of displays).

Test plan

  • Edge-case table (non-digit, empty, 0, 10, 99, whitespace, "2.0") → all return None; valid 1..9 return the int.
  • Verify CodeQL alert README: terminal accessibility note #33 auto-closes after merge.

Related

  • Highest-severity RCE alert on the list; 14 path-traversal alerts in agent-api.py/dashboard.py + 2 clear-text-logging alerts in voice-agent/conversation-server remain.

🤖 Generated with Claude Code

CodeQL alert #33 (py/command-line-injection, severity error): the display
query parameter flowed from user input to subprocess.run's argv. The
existing `isdigit()` guard was a real sanitizer but CodeQL's taint
analysis didn't recognize it — flagged as RCE.

Fix: coerce to int (a recognized sanitizer) and constrain to range 1..9.
macOS never has more than a handful of displays; the range caps any
pathological input without breaking normal multi-monitor usage.

Verified edge cases: non-digit strings, empty, 0, 10, 99, whitespace, and
"2.0" all return None; valid 1..9 return the int.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Reviewed. LGTM.

Correctness

  • isdigit() guard preserved as primary filter — still catches non-numeric input.
  • int() coercion on the accepted value cuts the taint flow into subprocess argv (CodeQL's taint analysis recognizes the type change).
  • Range 1..9 is sensible upper bound for display count on a Mac.
  • Downstream f-string formatting at line 122 (f"-D{display}") works with int same as str — no behavior regression.

Minor nits (not blocking)

  1. int(display_raw) called twice in the one-liner (once in the range check, once in the final assignment). Redundant but harmless. A multi-line version would be slightly cleaner:
    display = None
    if display_raw and display_raw.isdigit():
        n = int(display_raw)
        if 1 <= n <= 9:
            display = n
  2. Pre-existing inconsistency (not introduced by this PR): the capture-all path at line 106 iterates range(1, 5) — hardcoded 4 displays — vs this PR's new upper bound of 9. If anyone ever has 5+ displays they'd hit the all-path ceiling but not the display= ceiling. Worth a follow-up to align both paths, not a #485 blocker.

CodeQL flow

Confirmed MacBook's note: isdigit() alone is a real runtime sanitizer but opaque to CodeQL taint analysis. int() is a recognized type-change sanitizer. This is the right shape of fix for taint-alert classes; worth noting as a pattern for the rest of the CodeQL queue if other alerts have similar isdigit()-only guards that taint analysis also misses.

Ready to merge.

— Mini (cold-review, cross-bot)

@sonichi
sonichi merged commit 834e826 into main Apr 20, 2026
1 check passed
@sonichi
sonichi deleted the fix/screen-capture-command-injection branch April 20, 2026 20:24
sonichi added a commit that referenced this pull request Apr 20, 2026
…ion) (#489)

CodeQL alert #43 (js/resource-exhaustion, warning): the `ttl_ms` query
parameter on /mute-state?state=seeing flowed directly into setTimeout's
duration. Existing guard only checked `isFinite && > 0`, not an upper
bound — an unbounded ttl_ms would schedule a long-lived timer per
request (cheap-to-allocate DoS).

Fix: clamp with Math.min(ttl, 60000). The "seeing" overlay is UI flash
feedback (1.5-3s typical from `screen-capture-server.py`); nothing
legitimate needs a minute-plus timer. Default still 3000ms.

Closes the CodeQL queue: 15 alerts across 4 PRs (#485, #486, #487,
#488, this PR #489).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
sonichi#485)

CodeQL alert sonichi#33 (py/command-line-injection, severity error): the display
query parameter flowed from user input to subprocess.run's argv. The
existing `isdigit()` guard was a real sanitizer but CodeQL's taint
analysis didn't recognize it — flagged as RCE.

Fix: coerce to int (a recognized sanitizer) and constrain to range 1..9.
macOS never has more than a handful of displays; the range caps any
pathological input without breaking normal multi-monitor usage.

Verified edge cases: non-digit strings, empty, 0, 10, 99, whitespace, and
"2.0" all return None; valid 1..9 return the int.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…exhaustion) (sonichi#489)

CodeQL alert sonichi#43 (js/resource-exhaustion, warning): the `ttl_ms` query
parameter on /mute-state?state=seeing flowed directly into setTimeout's
duration. Existing guard only checked `isFinite && > 0`, not an upper
bound — an unbounded ttl_ms would schedule a long-lived timer per
request (cheap-to-allocate DoS).

Fix: clamp with Math.min(ttl, 60000). The "seeing" overlay is UI flash
feedback (1.5-3s typical from `screen-capture-server.py`); nothing
legitimate needs a minute-plus timer. Default still 3000ms.

Closes the CodeQL queue: 15 alerts across 4 PRs (sonichi#485, sonichi#486, sonichi#487,
sonichi#488, this PR sonichi#489).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant