fix(security): 2 improvements across 1 files#315
Open
tomaioo wants to merge 1 commit into
Open
Conversation
- Security: Command Injection via Unsanitized Terminal ID in restore() - Security: Path Traversal via Terminal ID in restore() Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
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_idbefore using it in paths/commands. - Use
shlex.quote()when composingwindow_shell(scrollback path and login shell).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+32
| if not re.fullmatch(r"[A-Za-z0-9_-]+", terminal_id): | ||
| raise click.ClickException( | ||
| f"Invalid terminal_id '{terminal_id}'. Only alphanumeric, underscore, and hyphen characters are allowed." | ||
| ) |
| @@ -57,9 +64,9 @@ def restore(terminal_id: str): | |||
| login_shell = os.environ.get("SHELL", "bash") | |||
Comment on lines
+29
to
+32
| if not re.fullmatch(r"[A-Za-z0-9_-]+", terminal_id): | ||
| raise click.ClickException( | ||
| f"Invalid terminal_id '{terminal_id}'. Only alphanumeric, underscore, and hyphen characters are allowed." | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fix(security): 2 improvements across 1 files
Problem
Severity:
High| File:src/cli_agent_orchestrator/cli/commands/terminal.py:L28The
restore()function interminal.pyconstructs a shell command string using unsanitized user input (terminal_id) and passes it toget_backend().create_window()withwindow_shell. While theterminal_idis used to construct file paths, thewindow_shellparameter containsterminal_idinterpolated into a shell command:f"cat '{scrollback_path}'; exec {login_shell} -l". Thescrollback_pathis derived fromTERMINAL_LOG_DIR / f"{terminal_id}.scrollback". Ifterminal_idcontains shell metacharacters, this could lead to command injection. More critically, thecreate_windowmethod likely passes this to tmux'ssend-keysornew-windowwith shell execution, creating a command injection vector.Solution
Sanitize
terminal_idusing a strict whitelist (e.g., alphanumeric only) before using it in file paths or shell commands. Useshlex.quote()for any values embedded in shell commands. Consider validating thatterminal_idmatches expected format before proceeding.Changes
src/cli_agent_orchestrator/cli/commands/terminal.py(modified)