Fix shell injection, SSRF, broken fallback, add /agent command, improve LLM resilience#3
Conversation
…ce reliability Agent-Logs-Url: https://github.com/Rahulchaube1/QGo/sessions/289d04ce-df2e-4fb9-9951-2920afa8b14e Co-authored-by: Rahulchaube1 <157899057+Rahulchaube1@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens QGo’s REPL and web utilities by addressing command execution injection risks, tightening URL handling, restoring a broken urllib fallback, and adding the documented /agent command while improving LLM call resilience.
Changes:
- Hardened
/runand/gitexecution by removingshell=Trueand parsing args safely. - Added URL scheme validation to reduce scheme-based SSRF vectors and restored
_fetch_plain()fallback definition. - Implemented
/agentcommand wiring to the multi-agent orchestrator and improved LLM robustness (context-window warning + timeout retries).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| qgo/utils/web_scraper.py | Adds _validate_url() and applies it across fetch helpers; restores _fetch_plain() definition. |
| qgo/ui/terminal.py | Updates /help output to document /agent. |
| qgo/ui/commands.py | Removes shell execution for /run and /git; adds /agent command handler. |
| qgo/llm/litellm_provider.py | Retries on LiteLLM timeout errors in the exponential backoff loop. |
| qgo/coders/base_coder.py | Adds best-effort token estimation to warn near context-window overflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except ValueError as exc: | ||
| return f"[{exc}]" |
There was a problem hiding this comment.
fetch_url() docstring says it returns an empty string on failure, but on URL validation failure it now returns a non-empty bracketed error string. Because callers (e.g. /web) treat any non-empty return as success, invalid URLs can be incorrectly added to conversation context as “content”. Consider returning "" (to match the docstring) or changing the function contract + updating callers to handle an explicit error value.
| except ValueError as exc: | |
| return f"[{exc}]" | |
| except ValueError: | |
| return "" |
| def _validate_url(url: str) -> None: | ||
| """Raise ValueError if *url* uses a disallowed scheme. | ||
|
|
||
| Only http and https are permitted to prevent SSRF via file://, ftp://, | ||
| gopher://, etc. | ||
| """ | ||
| import urllib.parse | ||
|
|
||
| parsed = urllib.parse.urlparse(url) | ||
| if not parsed.scheme or parsed.scheme not in ("http", "https"): | ||
| raise ValueError( | ||
| f"Disallowed URL scheme {parsed.scheme!r}. Only http/https are permitted." | ||
| ) |
There was a problem hiding this comment.
_validate_url() only restricts the URL scheme. This prevents file:// etc., but it does not prevent SSRF to internal services over http(s) (e.g. http://localhost, http://169.254.169.254). If the goal is SSRF mitigation rather than just scheme hardening, consider additionally rejecting loopback/private/link-local IPs and optionally enforcing an allowlist of hosts.
| def _cmd_run(self, args: str) -> None: | ||
| if not args: | ||
| self.io.print_warning("Usage: /run <shell command>") | ||
| return | ||
| try: | ||
| cmd = shlex.split(args) | ||
| result = subprocess.run( | ||
| args, shell=True, capture_output=True, text=True, timeout=60 | ||
| cmd, shell=False, capture_output=True, text=True, timeout=60 | ||
| ) |
There was a problem hiding this comment.
/run no longer executes via a shell (shell=False + shlex.split()), so the usage/help text saying “shell command” is now misleading (pipes/redirection won’t work). Update the usage string (and help text if needed) to describe the new behavior (e.g., “run a command” / “run an executable with args”).
| self.io.print_warning( | ||
| "Usage: /agent [--agent <name>] <task description>\n" | ||
| "Available agents: planner, coder, reviewer, tester, " | ||
| "debugger, doc_writer, security, refactor" | ||
| ) |
There was a problem hiding this comment.
_cmd_agent advertises planner as an available --agent option, but AgentOrchestrator.run_single() only supports names in orchestrator._agents (coder/reviewer/tester/debugger/doc_writer/security/refactor) and will raise ValueError for planner. Either remove planner from the CLI/help list, or update run_single() / orchestrator internals to allow running the planner directly.
| # Parse optional --agent flag | ||
| agent_name: str | None = None | ||
| task = args | ||
| parts = args.split(maxsplit=2) | ||
| if len(parts) >= 2 and parts[0] == "--agent": | ||
| agent_name = parts[1] | ||
| task = parts[2] if len(parts) > 2 else "" | ||
| if not task: | ||
| self.io.print_warning("Please provide a task description after --agent <name>.") | ||
| return |
There was a problem hiding this comment.
--agent values are passed through as-is. Since AgentOrchestrator keys are lowercase (e.g. "coder"), /agent --agent Coder ... will currently fail even though it’s an obvious user input variant. Consider normalizing agent_name = agent_name.lower() (and maybe validating against orchestrator.list_agents() for a better error message) before calling run_single().
| self.io.print_assistant(report) | ||
| # Add the report to conversation context | ||
| self.coder.messages.append({ | ||
| "role": "assistant", | ||
| "content": report, | ||
| }) |
There was a problem hiding this comment.
BaseCoder.messages is a list[Message] and _build_messages() calls msg.to_dict() for each prior message. Appending a raw dict here will cause an AttributeError the next time the coder sends a message. Append a qgo.models.Message(role=..., content=...) instead (and consider aligning the other command handlers that also append dicts).
| except litellm.exceptions.RateLimitError as e: | ||
| wait = 2 ** attempt | ||
| time.sleep(wait) | ||
| last_error = e | ||
| except litellm.exceptions.APIConnectionError as e: | ||
| wait = 2 ** attempt | ||
| time.sleep(wait) | ||
| last_error = e | ||
| except litellm.exceptions.Timeout as e: | ||
| wait = 2 ** attempt | ||
| time.sleep(wait) | ||
| last_error = e |
There was a problem hiding this comment.
The retry logic now has three separate except blocks that all do the same backoff/sleep. This is correct but unnecessarily duplicated and easy to drift. Consider combining these into a single except (RateLimitError, APIConnectionError, Timeout) as e: block (or extracting a helper) so future retryable exceptions are added in one place.
Multiple security vulnerabilities, a silent runtime crash, and a documented-but-unimplemented feature addressed in one pass.
Security
commands.py):/runand/gitusedsubprocess.run(shell=True)with raw user input. Replaced withshlex.split()+shell=False.web_scraper.py): Added_validate_url()— rejects any scheme that isn'thttp/https(blocksfile://,ftp://,gopher://, etc.) acrossfetch_url,fetch_page_info, andfetch_image_base64.Bug fix
_fetch_plain()(web_scraper.py): Thedefline was missing — the function body existed but was unreachable dead code, causing aNameErrorwheneverrequests/bs4were absent. Restored the function definition.Feature:
/agentREPL commandREADME documented
/agentbutCommandHandlerhad no handler for it. Added_cmd_agent()wiringAgentOrchestrator:Updated
/helpoutput to reflect the new command.Reliability
base_coder.py): Estimates token count before each LLM call; warns when ≥90% of the model's limit is consumed, prompting/dropor/clear.litellm_provider.py): Extended exponential back-off to also catchlitellm.exceptions.TimeoutalongsideRateLimitErrorandAPIConnectionError.