fix(security): 2 improvements across 1 files

[tomaioo]

Summary

fix(security): 2 improvements across 1 files

Problem

Severity: High | File: src/cli_agent_orchestrator/cli/commands/terminal.py:L28

The restore() function in terminal.py constructs a shell command string using unsanitized user input (terminal_id) and passes it to get_backend().create_window() with window_shell. While the terminal_id is used to construct file paths, the window_shell parameter contains terminal_id interpolated into a shell command: f"cat '{scrollback_path}'; exec {login_shell} -l". The scrollback_path is derived from TERMINAL_LOG_DIR / f"{terminal_id}.scrollback". If terminal_id contains shell metacharacters, this could lead to command injection. More critically, the create_window method likely passes this to tmux's send-keys or new-window with shell execution, creating a command injection vector.

Solution

Sanitize terminal_id using a strict whitelist (e.g., alphanumeric only) before using it in file paths or shell commands. Use shlex.quote() for any values embedded in shell commands. Consider validating that terminal_id matches expected format before proceeding.

Changes

• src/cli_agent_orchestrator/cli/commands/terminal.py (modified)

[Copilot]

Pull request overview

This PR hardens the terminal restore CLI command against command-injection by validating terminal_id and properly shell-quoting values embedded in the window_shell command passed to the backend.

Changes:

• Add a strict allowlist check for terminal_id before using it in paths/commands.
• Use shlex.quote() when composing window_shell (scrollback path and login shell).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gutosantos82]

PR Review: #315 — fix(security): 2 improvements across 1 files

Summary

This PR hardens the cao terminal restore command against command injection: it adds a strict [A-Za-z0-9_-]+ whitelist on terminal_id (which flows into filesystem paths and, via the scrollback path, into a shell command) and wraps scrollback_path and login_shell in shlex.quote() when building window_shell. The security fix itself is complete and correct — both the injection and path-traversal surfaces are closed, and no downstream input remains unguarded. However, the PR breaks an existing test without updating it and ships zero tests for the new security behavior, so it should not merge as-is.

Blocking (must fix before merge)

• [tests] test/cli/commands/test_terminal.py:309 — test_restore_success asserts window_shell=f"cat '{scrollback_path}'; exec /bin/zsh -l" with literal single quotes. The new code builds this with shlex.quote(str(scrollback_path)), and shlex.quote leaves an all-safe path (pytest's tmp_path) unquoted — so the actual value is cat /tmp/.../abc12345.scrollback; exec /bin/zsh -l (no quotes) and the assertion now fails. Verified by the supervisor: the assertion at :309 hardcodes the quotes and was not touched by this PR. Fix: update the expected window_shell to match shlex.quote output. (introduced)

Important (should fix)

• [tests] terminal.py:29-32 — The core of the security fix — the re.fullmatch validation guard — has no test. Add test_restore_rejects_invalid_terminal_id covering payloads like "abc; rm -rf /", "../../etc/passwd", "a b", "$(whoami)", "" → non-zero exit and "Invalid terminal_id" in output, plus an explicit positive case for a valid id. (introduced)
• [tests] terminal.py:67,69 — No test exercises shlex.quote() with a metacharacter-bearing login_shell or scrollback path, so the quoting behavior (the whole point of the fix) is unverified. Add a test that sets SHELL to a value with spaces/;/$ and asserts the resulting window_shell is properly quoted. (introduced)
• [conventions] terminal.py:29 — Reuse the existing validation pattern instead of inlining a fresh regex. install.py:20 already defines a module-level _PROFILE_NAME_RE = re.compile(r"^[A-Za-z0-9_-]{1,64}$") for exactly this purpose. Define/share a module-level compiled _TERMINAL_ID_RE and reuse it — and adopt the {1,64} upper bound (the PR's + is unbounded). (introduced)
• [conventions] CHANGELOG.md — No [Unreleased] entry for this user-facing security change (previously-accepted terminal_ids can now be rejected). Add a line under ### Security / ### Fixed per Keep a Changelog. (introduced)

Nits (optional)

• [conventions] terminal.py:31 — Error-message f-string is ~117 chars, over the configured line-length=100. Black won't split an unsplittable string literal so CI passes, but shortening/wrapping is cleaner. (introduced)
• [conventions] PR/commit title — "fix(security): 2 improvements across 1 files" is machine-generated and ungrammatical; prefer something descriptive, e.g. fix(security): validate terminal_id and shlex-quote restore shell args. (introduced)
• [correctness] terminal.py:45-46 — session_name and working_directory read from the snapshot JSON are passed to create_window() unsanitized. Low risk and pre-existing (not introduced here): working_directory is independently validated downstream by _resolve_and_validate_working_directory (clients/tmux.py), session_name is passed as a libtmux arg (not a shell), and the snapshot file is app-written. Worth a follow-up, not a blocker. (pre-existing)

Tests

No tests were added or updated. The existing test/cli/commands/test_terminal.py (TestRestoreCommand) is a plain, correctly-structured unit test (CliRunner + mocked backend, in the fast unit run) but was not touched. Net effect: the PR breaks test_restore_success (blocking, above) and leaves both new security behaviors (validation + quoting) entirely uncovered. Adding the reject-invalid, accept-valid, and metachar-quoting tests — and fixing the :309 assertion — is required before merge.

Security assessment

The fix is complete and correct for the stated vector. re.fullmatch (not re.match) is the right anchor: no trailing-newline bypass, requires ≥1 char, and rejects empty/..//;/quote payloads before they reach the path build at :34-35 or the shell string at :67. shlex.quote() correctly neutralizes the confirmed original break-out (a terminal_id like x'; rm -rf ~ # escaped the old single-quoted f-string). window_shell is the only value reaching shell execution (via clients/tmux.py session.new_window), and both interpolated values in it are now quoted — the vector is fully closed. No credential, permission-bypass, or CI concerns.

Verdict

Request changes — the security fix is sound, but it breaks test_restore_success and adds no tests for the new validation/quoting. Fix the assertion, add the missing tests, and reuse the existing _PROFILE_NAME_RE-style pattern + CHANGELOG entry, then this is a clean merge.

Consistency review did not return; PR-description↔impl drift, cross-command terminal_id-validation consistency, and comment-accuracy at lines 60-62 were not independently covered (though the correctness and security reviewers confirmed the whitelist matches real terminal_id formats — uuid4 hex and hex+hyphens).