Added RCE validation to both devserver.py and dev_server_manager.py#152
Added RCE validation to both devserver.py and dev_server_manager.py#152syphonetic wants to merge 2 commits intoAutoForgeAI:masterfrom
Conversation
Refactor dev server management to improve command execution and security checks. Introduce lock file handling and command validation enhancements.
Refactor dev server API to enhance security and command validation. Added logging and improved command handling.
📝 WalkthroughWalkthroughIntroduces comprehensive security validation for dev server control endpoints. Adds command validation layer blocking shell injection and dangerous interpreters, replaces shell-based execution with direct process spawning, and implements project-scoped dev server management with HTTP error handling for missing projects or directories. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as Dev Server Router
participant Validator as Command Validator
participant Manager as Dev Server Manager
participant Process as OS Process
Client->>Router: start_devserver(project_name, request)
Router->>Router: validate project exists
Router->>Router: get_project_dir(project_name)
Router->>Validator: validate_dev_command(command, project_dir)
alt Command Valid
Validator-->>Router: ✓ Valid
Router->>Manager: start(project_name, validated_command)
Manager->>Manager: strip & parse command (shlex)
Manager->>Manager: block shell runners (sh/bash/zsh/cmd/powershell)
Manager->>Manager: construct argv (direct executable)
Manager->>Process: spawn via Popen (no shell)
Manager->>Manager: create lock file + record PID
Manager-->>Router: DevServerActionResponse (started)
Router-->>Client: 200 OK
else Command Invalid
Validator-->>Router: ✗ Blocked (injection/disallowed)
Router-->>Client: 400 Bad Request
else Project Missing
Router-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routers/devserver.py (1)
690-742:⚠️ Potential issue | 🔴 CriticalDuplicate
update_devserver_configfunction definition.This function is already defined at lines 627-679. FastAPI will use the second definition, but this duplication is clearly unintentional and results from the file corruption.
🤖 Fix all issues with AI agents
In `@server/routers/devserver.py`:
- Around line 682-687: There is a duplicate, unreachable return that constructs
DevServerConfigResponse; locate the function that returns a
DevServerConfigResponse (look for the earlier return that already returns the
response) and remove the second/duplicate return DevServerConfigResponse(...)
block so only the single, intended return remains (or consolidate any differing
fields into the first return if needed).
- Around line 78-133: The uvicorn flag parsing and mode checks in
validate_custom_command_strict are case-sensitive and don't accept --flag=value
forms; update the function to use the already-created lowered list for all
comparisons (e.g., compare lowered[1:3] to ["-m","uvicorn"] and use lowered for
base checks) and normalize flags when iterating by splitting on '=' and
lowercasing the flag name before membership tests (e.g., flag_name =
a.split("=",1)[0].lower()) so that allowed_flags (make it a lowercased set)
accepts both "--port" and "--port=8000" and comparisons like "-m"/"uvicorn" are
case-insensitive.
- Around line 309-351: The file contains a duplicated second copy of the module
(starting at the malformed line "config = get_project_config(project_dir)""" and
repeating the module docstring, imports, constants and route handlers) which
causes a syntax error and multiple duplicate definitions; remove the entire
duplicated block after that malformed line so only one module-level docstring,
one set of imports, one ALLOWED_RUNNERS, and single definitions of
get_project_dir, validate_custom_command_strict, get_devserver_config,
update_devserver_config, and route registrations remain, and ensure the
truncated get_devserver_config statement is completed/removed as needed and
imports/helpers (extract_commands, get_effective_commands, is_command_allowed,
get_devserver_manager, project_config functions, _get_project_path,
validate_project_name) are only declared once.
🧹 Nitpick comments (5)
server/services/dev_server_manager.py (4)
28-28: Unused imports from security module.The imports
extract_commands,get_effective_commands, andis_command_allowedare not used anywhere in this file. The command validation using these functions happens inserver/routers/devserver.pyinstead.🧹 Remove unused imports
-from security import extract_commands, get_effective_commands, is_command_allowed +# Security validation is handled in the router layer (server/routers/devserver.py)
312-315: Consider expanding the dangerous operators list.The current blocklist catches common shell chaining operators, but file redirection operators (
>,<,>>,2>) could also be used maliciously to overwrite files or exfiltrate data.🛡️ Add file redirection operators
# SECURITY: block shell operators/metacharacters (defense-in-depth) - dangerous_ops = ["&&", "||", ";", "|", "`", "$("] + dangerous_ops = ["&&", "||", ";", "|", "`", "$(", ">", "<", ">>", "2>"] if any(op in command for op in dangerous_ops): return False, "Shell operators are not allowed in dev server command"
352-352:datetime.utcnow()is deprecated in Python 3.12+.Per the coding guidelines requiring Python 3.11+, this deprecation warning will appear in newer Python versions. Use timezone-aware datetime instead.
♻️ Use timezone-aware datetime
+from datetime import datetime, timezone -from datetime import datetime ... - self.started_at = datetime.utcnow() + self.started_at = datetime.now(timezone.utc)
354-360: Lock creation timing could leave orphaned process on failure.The lock file is created after the process starts. If
_create_lock()fails (e.g., due to a disk error), the process will be running but untracked. Consider wrapping this in try/except to terminate the process if lock creation fails.🛡️ Handle lock creation failure
# Create lock once we have a PID - self._create_lock() + try: + self._create_lock() + except OSError as e: + logger.error(f"Failed to create lock file, terminating process: {e}") + self.process.terminate() + self.process = None + return False, f"Failed to create lock file: {e}" # Start output streaming self.status = "running"server/routers/devserver.py (1)
33-36:sys.pathmanipulation is fragile for importing the security module.Modifying
sys.pathat runtime can cause import issues in different execution contexts. Consider makingsecuritya proper package import or using relative imports if the module structure allows.Also applies to: 341-344
| def validate_custom_command_strict(cmd: str) -> None: | ||
| """ | ||
| Strict allowlist validation for dev server commands. | ||
| Prevents arbitrary command execution (no sh -c, no cmd /c, no python -c, etc.) | ||
| """ | ||
| if not isinstance(cmd, str) or not cmd.strip(): | ||
| raise ValueError("custom_command cannot be empty") | ||
|
|
||
| argv = shlex.split(cmd, posix=(sys.platform != "win32")) | ||
| if not argv: | ||
| raise ValueError("custom_command could not be parsed") | ||
|
|
||
| base = Path(argv[0]).name.lower() | ||
|
|
||
| # Block direct shells / interpreters commonly used for command injection | ||
| if base in {"sh", "bash", "zsh", "cmd", "powershell", "pwsh"}: | ||
| raise ValueError(f"custom_command runner not allowed: {base}") | ||
|
|
||
| if base not in ALLOWED_RUNNERS: | ||
| raise ValueError(f"custom_command runner not allowed: {base}") | ||
|
|
||
| # Block one-liner execution | ||
| lowered = [a.lower() for a in argv] | ||
| if base in {"python", "python3"}: | ||
| if "-c" in lowered: | ||
| raise ValueError("python -c is not allowed") | ||
| # Only allow: python -m uvicorn ... | ||
| if len(argv) < 3 or argv[1:3] != ["-m", "uvicorn"]: | ||
| raise ValueError("Only 'python -m uvicorn ...' is allowed") | ||
|
|
||
| if base == "uvicorn": | ||
| if len(argv) < 2 or ":" not in argv[1]: | ||
| raise ValueError("uvicorn must specify an app like module:app") | ||
|
|
||
| allowed_flags = {"--host", "--port", "--reload", "--log-level", "--workers"} | ||
| i = 2 | ||
| while i < len(argv): | ||
| a = argv[i] | ||
| if a.startswith("-") and a not in allowed_flags: | ||
| raise ValueError(f"uvicorn flag not allowed: {a}") | ||
| i += 1 | ||
|
|
||
| if base in {"npm", "pnpm", "yarn"}: | ||
| # Allow only dev/start scripts (no arbitrary exec) | ||
| if base == "npm": | ||
| if len(argv) < 3 or argv[1] != "run" or argv[2] not in {"dev", "start"}: | ||
| raise ValueError("npm custom_command must be 'npm run dev' or 'npm run start'") | ||
| elif base == "pnpm": | ||
| ok = (len(argv) >= 2 and argv[1] in {"dev", "start"}) or (len(argv) >= 3 and argv[1] == "run" and argv[2] in {"dev", "start"}) | ||
| if not ok: | ||
| raise ValueError("pnpm custom_command must be 'pnpm dev/start' or 'pnpm run dev/start'") | ||
| elif base == "yarn": | ||
| ok = (len(argv) >= 2 and argv[1] in {"dev", "start"}) or (len(argv) >= 3 and argv[1] == "run" and argv[2] in {"dev", "start"}) | ||
| if not ok: | ||
| raise ValueError("yarn custom_command must be 'yarn dev/start' or 'yarn run dev/start'") | ||
|
|
There was a problem hiding this comment.
Review of validate_custom_command_strict logic (appears twice due to duplication).
The validation logic has a potential gap in the uvicorn flag parsing:
-
Flags with
=syntax not handled: Flags like--port=8000will be rejected because--port=8000is not inallowed_flags, even though--portis allowed. -
Inconsistent case handling: The
-ccheck usesloweredlist (line 102/410), but theargv[1:3] != ["-m", "uvicorn"]check (line 105/413) is case-sensitive. A user could bypass with-M uvicorn.
🔧 Fix flag validation and casing consistency
if base == "uvicorn":
if len(argv) < 2 or ":" not in argv[1]:
raise ValueError("uvicorn must specify an app like module:app")
allowed_flags = {"--host", "--port", "--reload", "--log-level", "--workers"}
i = 2
while i < len(argv):
a = argv[i]
- if a.startswith("-") and a not in allowed_flags:
- raise ValueError(f"uvicorn flag not allowed: {a}")
+ if a.startswith("-"):
+ # Handle --flag=value syntax
+ flag_name = a.split("=")[0] if "=" in a else a
+ if flag_name not in allowed_flags:
+ raise ValueError(f"uvicorn flag not allowed: {flag_name}")
i += 1
if base in {"python", "python3"}:
if "-c" in lowered:
raise ValueError("python -c is not allowed")
# Only allow: python -m uvicorn ...
- if len(argv) < 3 or argv[1:3] != ["-m", "uvicorn"]:
+ if len(argv) < 3 or [x.lower() for x in argv[1:3]] != ["-m", "uvicorn"]:
raise ValueError("Only 'python -m uvicorn ...' is allowed")Also applies to: 386-440
🤖 Prompt for AI Agents
In `@server/routers/devserver.py` around lines 78 - 133, The uvicorn flag parsing
and mode checks in validate_custom_command_strict are case-sensitive and don't
accept --flag=value forms; update the function to use the already-created
lowered list for all comparisons (e.g., compare lowered[1:3] to ["-m","uvicorn"]
and use lowered for base checks) and normalize flags when iterating by splitting
on '=' and lowercasing the flag name before membership tests (e.g., flag_name =
a.split("=",1)[0].lower()) so that allowed_flags (make it a lowercased set)
accepts both "--port" and "--port=8000" and comparisons like "-m"/"uvicorn" are
case-insensitive.
| config = get_project_config(project_dir)""" | ||
| Dev Server Router | ||
| ================= | ||
|
|
||
| API endpoints for dev server control (start/stop) and configuration. | ||
| Uses project registry for path lookups and project_config for command detection. | ||
| """ | ||
|
|
||
| import logging | ||
| import sys | ||
| import shlex | ||
| from pathlib import Path | ||
|
|
||
| from fastapi import APIRouter, HTTPException | ||
|
|
||
| from ..schemas import ( | ||
| DevServerActionResponse, | ||
| DevServerConfigResponse, | ||
| DevServerConfigUpdate, | ||
| DevServerStartRequest, | ||
| DevServerStatus, | ||
| ) | ||
| from ..services.dev_server_manager import get_devserver_manager | ||
| from ..services.project_config import ( | ||
| clear_dev_command, | ||
| get_dev_command, | ||
| get_project_config, | ||
| set_dev_command, | ||
| ) | ||
| from ..utils.project_helpers import get_project_path as _get_project_path | ||
| from ..utils.validation import validate_project_name | ||
|
|
||
| # Add root to path for security module import | ||
| _root = Path(__file__).parent.parent.parent | ||
| if str(_root) not in sys.path: | ||
| sys.path.insert(0, str(_root)) | ||
|
|
||
| from security import extract_commands, get_effective_commands, is_command_allowed | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| router = APIRouter(prefix="/api/projects/{project_name}/devserver", tags=["devserver"]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the duplicate content in the file
echo "=== Checking for duplicate function definitions ==="
rg -n "^def get_project_dir|^def validate_custom_command_strict|^async def update_devserver_config" server/routers/devserver.py
echo ""
echo "=== Checking for duplicate module docstring ==="
rg -n "Dev Server Router" server/routers/devserver.py
echo ""
echo "=== Checking for duplicate ALLOWED_RUNNERS ==="
rg -n "ALLOWED_RUNNERS" server/routers/devserver.pyRepository: leonvanzyl/autocoder
Length of output: 784
🏁 Script executed:
#!/bin/bash
# Check the actual content around line 309
echo "=== Lines 305-315 around the suspected corruption ==="
sed -n '305,315p' server/routers/devserver.py | cat -n
echo ""
echo "=== Total lines in file ==="
wc -l server/routers/devserver.py
echo ""
echo "=== Check for syntax errors (first occurrence of malformed string) ==="
sed -n '309p' server/routers/devserver.py | cat -ARepository: leonvanzyl/autocoder
Length of output: 743
Critical: File contains duplicate code causing syntax/runtime errors.
Starting at line 309, the file content is duplicated. The get_devserver_config function is cut off mid-statement with a malformed string (config = get_project_config(project_dir)""") followed by the entire module docstring, imports, and helper functions repeating.
This causes:
- Syntax error from the unclosed/malformed triple-quoted string at line 309
- Duplicate function definitions:
get_project_dir(lines 46, 354),validate_custom_command_strict(lines 78, 386),update_devserver_config(lines 628, 691) - Duplicate module docstring (lines 2, 310) and
ALLOWED_RUNNERSconstant (lines 76, 384) - Duplicate route registrations
Please review the merge/commit history and remove the duplicate content starting from line 309 onward.
🤖 Prompt for AI Agents
In `@server/routers/devserver.py` around lines 309 - 351, The file contains a
duplicated second copy of the module (starting at the malformed line "config =
get_project_config(project_dir)""" and repeating the module docstring, imports,
constants and route handlers) which causes a syntax error and multiple duplicate
definitions; remove the entire duplicated block after that malformed line so
only one module-level docstring, one set of imports, one ALLOWED_RUNNERS, and
single definitions of get_project_dir, validate_custom_command_strict,
get_devserver_config, update_devserver_config, and route registrations remain,
and ensure the truncated get_devserver_config statement is completed/removed as
needed and imports/helpers (extract_commands, get_effective_commands,
is_command_allowed, get_devserver_manager, project_config functions,
_get_project_path, validate_project_name) are only declared once.
| return DevServerConfigResponse( | ||
| detected_type=config["detected_type"], | ||
| detected_command=config["detected_command"], | ||
| custom_command=config["custom_command"], | ||
| effective_command=config["effective_command"], | ||
| ) |
There was a problem hiding this comment.
Unreachable duplicate return statement.
Lines 682-687 contain a duplicate return DevServerConfigResponse(...) statement that appears after the function has already returned at line 679. This is dead code resulting from the file duplication issue.
🤖 Prompt for AI Agents
In `@server/routers/devserver.py` around lines 682 - 687, There is a duplicate,
unreachable return that constructs DevServerConfigResponse; locate the function
that returns a DevServerConfigResponse (look for the earlier return that already
returns the response) and remove the second/duplicate return
DevServerConfigResponse(...) block so only the single, intended return remains
(or consolidate any differing fields into the first return if needed).
…ensive programming - Created 24 unit tests using Vitest + React Testing Library - Tests verify ChatTab handles undefined/null data gracefully - Coverage includes: * Undefined conversations array * Null messages handling * Missing conversation properties * Malformed API responses (404, 500, 503) * Network error handling * Type safety (wrong data types) * Edge cases (no project, empty name, long content) * Error Boundary functionality * 100% coverage of error paths All test code written and verified with automated script. Tests will run when system load decreases. Related: Feature AutoForgeAI#147 (error handling), Feature AutoForgeAI#146 (404 handling)
1 participant
Added a validation function (validate_custom_command_strict) in devserver.py
If raw commands were retrieved from request body to request.command, raise error code 400.
Called validate_custom_command_strict before set_dev_command
Replaced start function in dev_server_manager.py to filter for any command injection attempts.
Summary by CodeRabbit
Release Notes
New Features
Improvements