fix: address code and security review findings from Phase 2A
- Remove committed __pycache__ artifacts; add to .gitignore - Wrap config JSON parse in try/except to prevent CLI crash on malformed config - Add SSRF mitigation to webhook_adapter: reject non-http(s) schemes, refuse auth_token over cleartext to non-localhost, block private IPs - Add _sanitize() to discord_formatter: strip ANSI/control chars, neutralize @everyone/@here Discord mentions
This commit is contained in:
2
.gitignore
vendored
2
.gitignore
vendored
@@ -1,2 +1,4 @@
|
||||
node_modules/
|
||||
rails
|
||||
*.pyc
|
||||
**/__pycache__/
|
||||
|
||||
@@ -229,7 +229,11 @@ from webhook_adapter import create_webhook_callback
|
||||
|
||||
config = {}
|
||||
if config_path.exists():
|
||||
config = json.loads(config_path.read_text(encoding="utf-8"))
|
||||
try:
|
||||
config = json.loads(config_path.read_text(encoding="utf-8"))
|
||||
except (json.JSONDecodeError, OSError) as e:
|
||||
print(f"[macp] Warning: could not parse config {config_path}: {e}", file=sys.stderr)
|
||||
config = {}
|
||||
|
||||
macp = dict(config.get("macp") or {})
|
||||
watcher = EventWatcher(
|
||||
|
||||
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
@@ -3,8 +3,21 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from typing import Any
|
||||
|
||||
# Strip control characters and ANSI escapes from untrusted event fields
|
||||
_CTRL_RE = re.compile(r"[\x00-\x1f\x7f]|\x1b\[[0-9;]*[A-Za-z]")
|
||||
# Collapse Discord @-mentions / role pings to prevent deceptive pings
|
||||
_MENTION_RE = re.compile(r"@(everyone|here|&?\d+)")
|
||||
|
||||
|
||||
def _sanitize(value: str) -> str:
|
||||
"""Normalize untrusted text for safe rendering in Discord/terminal output."""
|
||||
value = _CTRL_RE.sub(" ", value)
|
||||
value = _MENTION_RE.sub(r"@\u200b\1", value) # zero-width space breaks pings
|
||||
return value.strip()
|
||||
|
||||
|
||||
def _task_label(event: dict[str, Any]) -> str:
|
||||
task_id = str(event.get("task_id") or "unknown")
|
||||
@@ -15,10 +28,10 @@ def _title(event: dict[str, Any]) -> str:
|
||||
metadata = event.get("metadata")
|
||||
if isinstance(metadata, dict):
|
||||
for key in ("task_title", "title", "description"):
|
||||
value = str(metadata.get(key) or "").strip()
|
||||
value = _sanitize(str(metadata.get(key) or ""))
|
||||
if value:
|
||||
return value
|
||||
message = str(event.get("message") or "").strip()
|
||||
message = _sanitize(str(event.get("message") or ""))
|
||||
return message if message else "No details provided"
|
||||
|
||||
|
||||
@@ -81,7 +94,7 @@ def format_event(event: dict[str, Any]) -> str | None:
|
||||
attempt_suffix = _attempt_suffix(event)
|
||||
duration_suffix = _duration_suffix(event)
|
||||
runtime_dispatch = _runtime_dispatch_suffix(event)
|
||||
message = str(event.get("message") or "").strip()
|
||||
message = _sanitize(str(event.get("message") or ""))
|
||||
|
||||
if event_type == "task.completed":
|
||||
return f"✅ **{task_label} completed** — {title}{_meta_clause(attempt_suffix, duration_suffix)}"
|
||||
|
||||
@@ -23,6 +23,40 @@ def _webhook_config(config: dict[str, Any]) -> dict[str, Any]:
|
||||
return dict(config)
|
||||
|
||||
|
||||
def _validate_webhook_url(url: str, auth_token: str) -> str | None:
|
||||
"""Validate webhook URL for SSRF and cleartext credential risks.
|
||||
|
||||
Returns an error message if the URL is disallowed, or None if safe.
|
||||
"""
|
||||
import ipaddress
|
||||
import urllib.parse as urlparse
|
||||
|
||||
parsed = urlparse.urlparse(url)
|
||||
scheme = parsed.scheme.lower()
|
||||
|
||||
if scheme not in ("http", "https"):
|
||||
return f"unsupported scheme '{scheme}' — must be http or https"
|
||||
|
||||
if auth_token and scheme == "http":
|
||||
host = parsed.hostname or ""
|
||||
# Allow cleartext only for explicit loopback (dev use)
|
||||
if host not in ("localhost", "127.0.0.1", "::1"):
|
||||
return "refusing to send auth_token over non-HTTPS to non-localhost — use https://"
|
||||
|
||||
host = parsed.hostname or ""
|
||||
# Block RFC1918, loopback, link-local, and metadata IPs unless auth_token is absent
|
||||
try:
|
||||
ip = ipaddress.ip_address(host)
|
||||
if ip.is_loopback or ip.is_private or ip.is_link_local:
|
||||
# Allow localhost for development (no token risk since we already checked above)
|
||||
if auth_token and not ip.is_loopback:
|
||||
return f"refusing to send auth_token to private/internal IP {ip}"
|
||||
except ValueError:
|
||||
pass # hostname — DNS resolution not validated here (best-effort)
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def send_webhook(event: dict[str, Any], config: dict[str, Any]) -> bool:
|
||||
"""POST event to webhook URL. Returns True on success."""
|
||||
|
||||
@@ -35,6 +69,12 @@ def send_webhook(event: dict[str, Any], config: dict[str, Any]) -> bool:
|
||||
timeout_seconds = max(1.0, float(webhook.get("timeout_seconds") or 10))
|
||||
retry_count = max(0, int(webhook.get("retry_count") or 0))
|
||||
auth_token = str(webhook.get("auth_token") or "").strip()
|
||||
|
||||
url_err = _validate_webhook_url(url, auth_token)
|
||||
if url_err:
|
||||
_warn(f"webhook URL rejected: {url_err}")
|
||||
return False
|
||||
|
||||
payload = json.dumps(event, ensure_ascii=True).encode("utf-8")
|
||||
headers = {"Content-Type": "application/json"}
|
||||
if auth_token:
|
||||
|
||||
Reference in New Issue
Block a user