From 56a362b395e54202ceaf9dbda29e5e3334ba96c2 Mon Sep 17 00:00:00 2001 From: eesb99 Date: Wed, 15 Apr 2026 12:45:11 +0800 Subject: [PATCH 1/4] security: fix 6 vulnerabilities (XSS, CSRF, SSRF, path traversal, credential storage, prompt injection) - Fix reflected XSS in OAuth callback by HTML-escaping error parameter - Add OAuth state parameter validation to prevent CSRF attacks - Add SSRF protection to http_request action (block private IPs, cloud metadata) - Add path traversal protection to read_file/write_file actions (block sensitive dirs) - Set restrictive file permissions (0600) on stored credentials - Make prompt sanitizer actually strip detected injection patterns instead of just logging Co-Authored-By: Claude Opus 4.6 (1M context) --- agent_core/core/credentials/oauth_server.py | 24 +++++++++++++++++---- app/data/action/http_request.py | 23 ++++++++++++++++++++ app/data/action/read_file.py | 23 ++++++++++++++++++-- app/data/action/write_file.py | 18 ++++++++++++---- app/external_comms/credentials.py | 12 +++++++++++ app/security/prompt_sanitizer.py | 9 +++++--- 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/agent_core/core/credentials/oauth_server.py b/agent_core/core/credentials/oauth_server.py index 9d8a701f..8b5a60b3 100644 --- a/agent_core/core/credentials/oauth_server.py +++ b/agent_core/core/credentials/oauth_server.py @@ -22,6 +22,7 @@ """ import asyncio +import html import ipaddress import logging import os @@ -120,10 +121,20 @@ class _OAuthCallbackHandler(BaseHTTPRequestHandler): def do_GET(self): """Handle GET request from OAuth callback.""" params = parse_qs(urlparse(self.path).query) - result_holder["code"] = params.get("code", [None])[0] - result_holder["state"] = params.get("state", [None])[0] + returned_state = params.get("state", [None])[0] result_holder["error"] = params.get("error", [None])[0] + # Validate OAuth state parameter to prevent CSRF + expected_state = result_holder.get("expected_state") + if expected_state and returned_state != expected_state: + result_holder["error"] = "OAuth state mismatch — possible CSRF attack" + result_holder["code"] = None + logger.warning("[OAUTH] State mismatch: expected %s, got %s", expected_state, returned_state) + else: + result_holder["code"] = params.get("code", [None])[0] + + result_holder["state"] = returned_state + self.send_response(200) self.send_header("Content-Type", "text/html") self.end_headers() @@ -132,8 +143,9 @@ def do_GET(self): b"

Authorization successful!

You can close this tab.

" ) else: + safe_error = html.escape(str(result_holder.get('error') or 'Unknown error')) self.wfile.write( - f"

Failed

{result_holder['error']}

".encode() + f"

Failed

{safe_error}

".encode() ) def log_message(self, format, *args): @@ -203,8 +215,12 @@ def run_oauth_flow( if cancel_event and cancel_event.is_set(): return None, "OAuth cancelled" + # Extract the state parameter from the auth URL for CSRF validation + auth_params = parse_qs(urlparse(auth_url).query) + expected_state = auth_params.get("state", [None])[0] + # Use instance-level result holder instead of class-level state - result_holder: Dict[str, Any] = {"code": None, "state": None, "error": None} + result_holder: Dict[str, Any] = {"code": None, "state": None, "error": None, "expected_state": expected_state} handler_class = _make_callback_handler(result_holder) try: diff --git a/app/data/action/http_request.py b/app/data/action/http_request.py index 803f7626..643299e9 100644 --- a/app/data/action/http_request.py +++ b/app/data/action/http_request.py @@ -169,6 +169,29 @@ def send_http_requests(input_data: dict) -> dict: return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Unsupported method.'} if not url or not (url.startswith('http://') or url.startswith('https://')): return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Invalid or missing URL.'} + + # SSRF protection: block requests to private/internal networks and cloud metadata + try: + from urllib.parse import urlparse as _urlparse + import ipaddress as _ipaddress + import socket as _socket + _parsed = _urlparse(url) + _hostname = _parsed.hostname or '' + # Block cloud metadata endpoints + _BLOCKED_HOSTS = {'169.254.169.254', 'metadata.google.internal', 'metadata.internal'} + if _hostname in _BLOCKED_HOSTS: + return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Blocked: requests to cloud metadata endpoints are not allowed.'} + # Resolve hostname and check for private IPs + try: + _resolved = _socket.getaddrinfo(_hostname, None) + for _family, _type, _proto, _canonname, _sockaddr in _resolved: + _ip = _ipaddress.ip_address(_sockaddr[0]) + if _ip.is_private or _ip.is_loopback or _ip.is_link_local: + return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':f'Blocked: requests to private/internal addresses ({_hostname}) are not allowed.'} + except (socket.gaierror, ValueError): + pass # Let the request library handle DNS resolution errors + except Exception: + pass # Best-effort SSRF check; don't block on parsing failures if json_body is not None and data_body is not None: return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Provide either json or data, not both.'} if not isinstance(headers, dict) or not isinstance(params, dict): diff --git a/app/data/action/read_file.py b/app/data/action/read_file.py index 979f644a..1374548d 100644 --- a/app/data/action/read_file.py +++ b/app/data/action/read_file.py @@ -130,7 +130,26 @@ def read_file(input_data: dict) -> dict: 'message': 'file_path is required.' } - if not os.path.isfile(file_path): + # Resolve symlinks and normalize path to prevent traversal attacks + resolved_path = os.path.realpath(file_path) + + # Block access to sensitive system files and directories + _BLOCKED_PREFIXES = ('/etc/shadow', '/etc/gshadow') + _BLOCKED_DIRS = ('.credentials', '.ssh', '.gnupg', '.aws') + path_parts = resolved_path.replace('\\', '/').split('/') + if any(resolved_path.startswith(p) for p in _BLOCKED_PREFIXES) or \ + any(part in _BLOCKED_DIRS for part in path_parts): + return { + 'status': 'error', + 'content': '', + 'total_lines': 0, + 'lines_returned': 0, + 'offset': 0, + 'has_more': False, + 'message': f'Access denied: {file_path} is in a restricted location.' + } + + if not os.path.isfile(resolved_path): return { 'status': 'error', 'content': '', @@ -142,7 +161,7 @@ def read_file(input_data: dict) -> dict: } try: - with open(file_path, 'r', encoding=encoding, errors='replace') as f: + with open(resolved_path, 'r', encoding=encoding, errors='replace') as f: all_lines = f.readlines() total_lines = len(all_lines) diff --git a/app/data/action/write_file.py b/app/data/action/write_file.py index 447ad4ef..849f3068 100644 --- a/app/data/action/write_file.py +++ b/app/data/action/write_file.py @@ -76,14 +76,24 @@ def write_file(input_data: dict) -> dict: if write_mode not in ('overwrite', 'append'): return {'status': 'error', 'file_path': '', 'bytes_written': 0, 'message': "mode must be 'overwrite' or 'append'."} + # Resolve path to prevent traversal attacks (resolve parent since file may not exist yet) + parent_dir = os.path.dirname(os.path.abspath(file_path)) + resolved_parent = os.path.realpath(parent_dir) if os.path.exists(parent_dir) else os.path.abspath(parent_dir) + resolved_path = os.path.join(resolved_parent, os.path.basename(file_path)) + + # Block writes to sensitive directories + _BLOCKED_DIRS = ('.credentials', '.ssh', '.gnupg', '.aws', '.env') + path_parts = resolved_path.replace('\\', '/').split('/') + if any(part in _BLOCKED_DIRS for part in path_parts): + return {'status': 'error', 'file_path': '', 'bytes_written': 0, 'message': f'Access denied: cannot write to restricted location.'} + try: # Create parent directories if needed - parent_dir = os.path.dirname(file_path) - if parent_dir: - os.makedirs(parent_dir, exist_ok=True) + if resolved_parent: + os.makedirs(resolved_parent, exist_ok=True) file_mode = 'w' if write_mode == 'overwrite' else 'a' - with open(file_path, file_mode, encoding=encoding) as f: + with open(resolved_path, file_mode, encoding=encoding) as f: bytes_written = f.write(content) return { diff --git a/app/external_comms/credentials.py b/app/external_comms/credentials.py index 3e6808cc..b1b23e78 100644 --- a/app/external_comms/credentials.py +++ b/app/external_comms/credentials.py @@ -10,6 +10,8 @@ import json import logging +import os +import stat from dataclasses import asdict, fields from pathlib import Path from typing import Optional, Type, TypeVar @@ -32,6 +34,11 @@ def _get_credentials_dir() -> Path: from app.config import PROJECT_ROOT _credentials_dir = PROJECT_ROOT / ".credentials" _credentials_dir.mkdir(parents=True, exist_ok=True) + # Restrict directory permissions to owner only (rwx------) + try: + os.chmod(_credentials_dir, stat.S_IRWXU) + except OSError: + pass # Best-effort on platforms that don't support chmod (e.g. Windows) return _credentials_dir @@ -78,6 +85,11 @@ def save_credential(filename: str, credential) -> None: try: with open(path, "w", encoding="utf-8") as f: json.dump(asdict(credential), f, indent=2, default=str) + # Restrict file permissions to owner read/write only (rw-------) + try: + os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) + except OSError: + pass # Best-effort on platforms that don't support chmod logger.info(f"Saved credential: {filename}") except Exception as e: logger.error(f"Failed to save credential {filename}: {e}") diff --git a/app/security/prompt_sanitizer.py b/app/security/prompt_sanitizer.py index 3dba8ced..23acb6a3 100644 --- a/app/security/prompt_sanitizer.py +++ b/app/security/prompt_sanitizer.py @@ -69,14 +69,17 @@ def sanitize_user_message(text: str, max_length: int = 5000) -> str: suspicious_patterns.append(pattern) if suspicious_patterns: - # Log these for monitoring (optional) + # Log and strip detected injection patterns import logging logger = logging.getLogger(__name__) logger.warning( - f"[SECURITY] Potential prompt injection detected. " + f"[SECURITY] Potential prompt injection detected and stripped. " f"Text: {text[:100]}... Patterns: {suspicious_patterns[:2]}" ) - + # Strip matched injection patterns from the text + for pattern in suspicious_patterns: + text = re.sub(pattern, '[FILTERED]', text) + return text @staticmethod From c8389b2026a800cf86c218a3fe20b52f4f6b21e6 Mon Sep 17 00:00:00 2001 From: eesb99 Date: Thu, 16 Apr 2026 00:46:16 +0800 Subject: [PATCH 2/4] security: fix SSRF NameError, timing-safe OAuth state, atomic credential perms - Fix socket.gaierror -> _socket.gaierror reference bug that silently disabled SSRF protection when DNS resolution failed - Use hmac.compare_digest() for OAuth state comparison to prevent timing side-channel attacks - Use os.open() with 0o600 mode for atomic credential file permissions instead of write-then-chmod race Co-Authored-By: Claude Opus 4.6 (1M context) --- agent_core/core/credentials/oauth_server.py | 5 ++++- app/data/action/http_request.py | 2 +- app/external_comms/credentials.py | 9 +++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/agent_core/core/credentials/oauth_server.py b/agent_core/core/credentials/oauth_server.py index 8b5a60b3..2844d0be 100644 --- a/agent_core/core/credentials/oauth_server.py +++ b/agent_core/core/credentials/oauth_server.py @@ -126,7 +126,10 @@ def do_GET(self): # Validate OAuth state parameter to prevent CSRF expected_state = result_holder.get("expected_state") - if expected_state and returned_state != expected_state: + import hmac + if expected_state and not hmac.compare_digest( + str(returned_state or ''), str(expected_state) + ): result_holder["error"] = "OAuth state mismatch — possible CSRF attack" result_holder["code"] = None logger.warning("[OAUTH] State mismatch: expected %s, got %s", expected_state, returned_state) diff --git a/app/data/action/http_request.py b/app/data/action/http_request.py index 643299e9..dd00bfbc 100644 --- a/app/data/action/http_request.py +++ b/app/data/action/http_request.py @@ -188,7 +188,7 @@ def send_http_requests(input_data: dict) -> dict: _ip = _ipaddress.ip_address(_sockaddr[0]) if _ip.is_private or _ip.is_loopback or _ip.is_link_local: return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':f'Blocked: requests to private/internal addresses ({_hostname}) are not allowed.'} - except (socket.gaierror, ValueError): + except (_socket.gaierror, ValueError): pass # Let the request library handle DNS resolution errors except Exception: pass # Best-effort SSRF check; don't block on parsing failures diff --git a/app/external_comms/credentials.py b/app/external_comms/credentials.py index b1b23e78..d8cc17d2 100644 --- a/app/external_comms/credentials.py +++ b/app/external_comms/credentials.py @@ -83,13 +83,10 @@ def save_credential(filename: str, credential) -> None: """ path = _get_credentials_dir() / filename try: - with open(path, "w", encoding="utf-8") as f: + # Create file with restricted permissions atomically (rw-------) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as f: json.dump(asdict(credential), f, indent=2, default=str) - # Restrict file permissions to owner read/write only (rw-------) - try: - os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) - except OSError: - pass # Best-effort on platforms that don't support chmod logger.info(f"Saved credential: {filename}") except Exception as e: logger.error(f"Failed to save credential {filename}: {e}") From 91ac96262749afb76fac4c1a6e30881ba2b0acd1 Mon Sep 17 00:00:00 2001 From: eesb99 Date: Thu, 16 Apr 2026 01:04:09 +0800 Subject: [PATCH 3/4] security: fix SSRF redirect bypass, fail-closed validation, CSRF enforcement SSRF (http_request.py): - Extract _is_url_ssrf_safe() helper for reuse across redirect hops - Disable allow_redirects in requests lib, manually follow redirects with SSRF validation on each hop (closes redirect bypass) - Remove fail-open except Exception: pass -- DNS failures now block the request instead of silently allowing it - Unresolvable hostnames return an error (fail closed) OAuth CSRF (oauth_server.py): - Explicit 3-way state handling: no state expected (warn + allow), state expected but missing from callback (reject), mismatch (reject) - Previously silently skipped validation when state was absent Co-Authored-By: Claude Opus 4.6 (1M context) --- agent_core/core/credentials/oauth_server.py | 13 ++++-- app/data/action/http_request.py | 44 +++++++++++++++------ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/agent_core/core/credentials/oauth_server.py b/agent_core/core/credentials/oauth_server.py index 2844d0be..6f9343e2 100644 --- a/agent_core/core/credentials/oauth_server.py +++ b/agent_core/core/credentials/oauth_server.py @@ -127,9 +127,16 @@ def do_GET(self): # Validate OAuth state parameter to prevent CSRF expected_state = result_holder.get("expected_state") import hmac - if expected_state and not hmac.compare_digest( - str(returned_state or ''), str(expected_state) - ): + if not expected_state: + # No state in auth URL -- log warning but allow (some providers don't support state) + logger.warning("[OAUTH] No state parameter in auth URL — CSRF protection disabled for this flow") + result_holder["code"] = params.get("code", [None])[0] + elif not returned_state: + # State was expected but callback didn't include it -- reject + result_holder["error"] = "OAuth callback missing state parameter — possible CSRF attack" + result_holder["code"] = None + logger.warning("[OAUTH] Expected state but callback had none") + elif not hmac.compare_digest(str(returned_state), str(expected_state)): result_holder["error"] = "OAuth state mismatch — possible CSRF attack" result_holder["code"] = None logger.warning("[OAUTH] State mismatch: expected %s, got %s", expected_state, returned_state) diff --git a/app/data/action/http_request.py b/app/data/action/http_request.py index dd00bfbc..a93565b0 100644 --- a/app/data/action/http_request.py +++ b/app/data/action/http_request.py @@ -171,34 +171,37 @@ def send_http_requests(input_data: dict) -> dict: return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Invalid or missing URL.'} # SSRF protection: block requests to private/internal networks and cloud metadata - try: - from urllib.parse import urlparse as _urlparse - import ipaddress as _ipaddress - import socket as _socket - _parsed = _urlparse(url) + from urllib.parse import urlparse as _urlparse + import ipaddress as _ipaddress + import socket as _socket + + def _is_url_ssrf_safe(check_url: str) -> str | None: + """Return an error message if the URL targets a blocked host, or None if safe.""" + _parsed = _urlparse(check_url) _hostname = _parsed.hostname or '' - # Block cloud metadata endpoints _BLOCKED_HOSTS = {'169.254.169.254', 'metadata.google.internal', 'metadata.internal'} if _hostname in _BLOCKED_HOSTS: - return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Blocked: requests to cloud metadata endpoints are not allowed.'} - # Resolve hostname and check for private IPs + return 'Blocked: requests to cloud metadata endpoints are not allowed.' try: _resolved = _socket.getaddrinfo(_hostname, None) for _family, _type, _proto, _canonname, _sockaddr in _resolved: _ip = _ipaddress.ip_address(_sockaddr[0]) if _ip.is_private or _ip.is_loopback or _ip.is_link_local: - return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':f'Blocked: requests to private/internal addresses ({_hostname}) are not allowed.'} + return f'Blocked: requests to private/internal addresses ({_hostname}) are not allowed.' except (_socket.gaierror, ValueError): - pass # Let the request library handle DNS resolution errors - except Exception: - pass # Best-effort SSRF check; don't block on parsing failures + return f'Blocked: could not resolve hostname ({_hostname}).' + return None + + ssrf_error = _is_url_ssrf_safe(url) + if ssrf_error: + return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message': ssrf_error} if json_body is not None and data_body is not None: return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Provide either json or data, not both.'} if not isinstance(headers, dict) or not isinstance(params, dict): return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'headers and params must be objects.'} headers = {str(k): str(v) for k, v in headers.items()} params = {str(k): str(v) for k, v in params.items()} - kwargs = {'headers': headers, 'params': params, 'timeout': timeout, 'allow_redirects': allow_redirects, 'verify': verify_tls} + kwargs = {'headers': headers, 'params': params, 'timeout': timeout, 'allow_redirects': False, 'verify': verify_tls} if json_body is not None: kwargs['json'] = json_body elif data_body is not None: @@ -206,6 +209,21 @@ def send_http_requests(input_data: dict) -> dict: try: t0 = time.time() resp = requests.request(method, url, **kwargs) + # Manually follow redirects with SSRF validation on each hop + _max_redirects = 10 + while allow_redirects and resp.is_redirect and _max_redirects > 0: + _max_redirects -= 1 + redirect_url = resp.headers.get('Location', '') + if not redirect_url: + break + # Resolve relative redirects + if not redirect_url.startswith(('http://', 'https://')): + from urllib.parse import urljoin + redirect_url = urljoin(resp.url, redirect_url) + redirect_error = _is_url_ssrf_safe(redirect_url) + if redirect_error: + return {'status':'error','status_code':resp.status_code,'response_headers':{},'body':'','final_url':resp.url,'elapsed_ms':int((time.time()-t0)*1000),'message':f'Redirect blocked: {redirect_error}'} + resp = requests.request('GET', redirect_url, **{**kwargs, 'allow_redirects': False}) elapsed_ms = int((time.time() - t0) * 1000) resp_headers = {k: v for k, v in resp.headers.items()} parsed_json = None From a5b4379bd1662c895603d6014e4bf94c499b4e47 Mon Sep 17 00:00:00 2001 From: eesb99 Date: Thu, 16 Apr 2026 01:24:41 +0800 Subject: [PATCH 4/4] docs: update session context for security review session 2 - context/claude.md: Session 2 summary with unified review findings, implementation details, code design assessment, and advisor recommendations - Updated PR references (#195 merged, #198 open) Co-Authored-By: Claude Opus 4.6 (1M context) --- context/claude.md | 124 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 context/claude.md diff --git a/context/claude.md b/context/claude.md new file mode 100644 index 00000000..b951ef25 --- /dev/null +++ b/context/claude.md @@ -0,0 +1,124 @@ +# CraftBot Security Review - Session Context + +## Current State + +- **Repo:** CraftOS-dev/CraftBot (cloned to ~/Claude/projects/CraftBot) +- **Fork:** eesb99/CraftBot +- **Branch:** security/fix-critical-vulnerabilities +- **PR:** CraftOS-dev/CraftBot#198 (open -- follow-up to merged #195) +- **Date:** 2026-04-16 (Session 2) + +## Session 1 Summary (2026-04-15) + +### Goals +- Clone CraftBot repo and perform a full security review +- Fix identified vulnerabilities and submit PR to upstream + +### Security Review Findings (12 total) + +| # | Severity | Finding | CWE | Fixed? | +|---|----------|---------|-----|--------| +| 1 | CRITICAL | Hardcoded OAuth credentials (base64-encoded in source) | CWE-798 | No (arch) | +| 2 | CRITICAL | Unsandboxed `exec()` of LLM-generated Python | CWE-94 | No (arch) | +| 3 | CRITICAL | Unrestricted `shell=True` command execution | CWE-78 | No (arch) | +| 4 | HIGH | Plaintext credential storage | CWE-312 | Yes (permissions) | +| 5 | HIGH | No path traversal protection on file ops | CWE-22 | Yes | +| 6 | HIGH | XSS in OAuth callback | CWE-79 | Yes | +| 7 | HIGH | Prompt sanitizer detects but doesn't block | CWE-20 | Yes | +| 8 | MEDIUM | Auto-install arbitrary pip packages | CWE-829 | No (design) | +| 9 | MEDIUM | Docker socket exposure | CWE-250 | No (deploy) | +| 10 | MEDIUM | No SSRF protection on HTTP requests | CWE-918 | Yes | +| 11 | MEDIUM | OAuth state not validated (CSRF) | CWE-352 | Yes | +| 12 | LOW | Broad `COPY .` in Dockerfile | - | No (low risk) | + +### Files Modified (6) +1. `agent_core/core/credentials/oauth_server.py` - XSS fix + CSRF state validation +2. `app/data/action/http_request.py` - SSRF protection (private IPs, cloud metadata) +3. `app/data/action/read_file.py` - Path traversal protection +4. `app/data/action/write_file.py` - Path traversal protection +5. `app/external_comms/credentials.py` - File permission hardening (0600) +6. `app/security/prompt_sanitizer.py` - Enforce pattern stripping + +### Commits +- `ae5025e` - security: fix 6 vulnerabilities (XSS, CSRF, SSRF, path traversal, credential storage, prompt injection) + +### Key Architecture Notes +- Python AI agent with LLM integrations (OpenAI, Gemini, Anthropic) +- OAuth for 6+ platforms (Google, Slack, Notion, LinkedIn, Discord, Telegram) +- Actions system in `app/data/action/` -- each action is a decorated function +- Credentials stored as JSON in `.credentials/` directory +- GUI mode uses Docker containers with desktop environments +- 150+ skills in `skills/` directory +- TUI built with Textual, browser UI with separate frontend + +### Next Steps +- [x] Monitor PR CraftOS-dev/CraftBot#195 for maintainer feedback -- MERGED +- [ ] Consider filing separate issues for the 3 CRITICAL unfixed items +- [ ] If PR is merged, the embedded credentials issue should be escalated as a security advisory + +## Session 2 Summary (2026-04-16) + +### Goals +- Rebase security branch onto latest origin/main +- Code review (unified-review --fix) of session 1 security fixes +- Fix bugs and architectural bypass vectors found during review +- Open follow-up PR (#198) to upstream + +### Unified Review Findings (10 total, >= 80 confidence) + +Dispatched parallel reviewers: python-code-reviewer + security-focused code-reviewer. + +| Confidence | Finding | Status | +|-----------|---------|--------| +| 95 | `socket.gaierror` vs `_socket.gaierror` NameError silently disabled SSRF | Fixed (auto-fix) | +| 93 | HTTP redirect bypass defeats SSRF check entirely | Fixed (PR A) | +| 92 | CSRF state validation silently skipped when state absent | Fixed (PR A) | +| 90 | DNS rebinding TOCTOU in resolve-then-request pattern | Deferred | +| 88 | Non-timing-safe OAuth state comparison | Fixed (auto-fix) | +| 85 | Path blocklist too narrow for read_file/write_file | Deferred | +| 85 | Credential file written world-readable before chmod | Fixed (auto-fix) | +| 82 | Outer `except Exception: pass` makes SSRF fail-open | Fixed (PR A) | +| 82 | Sequential pattern stripping creates new injection vectors | Deferred | +| 80 | Overly broad regex causes false positives on "run", "import" | Deferred | + +### Implementation Details + +**Commit 1 (c8389b2) -- Bug fixes (auto-fix phase):** +- `_socket.gaierror` NameError fix +- `hmac.compare_digest()` for timing-safe OAuth state comparison +- `os.open()` with 0o600 for atomic credential file permissions + +**Commit 2 (91ac962) -- Architectural fixes (PR A):** +- Extracted `_is_url_ssrf_safe()` helper for reuse across redirect hops +- Disabled `allow_redirects`, manually follow redirects with SSRF validation per hop +- Removed fail-open `except Exception: pass` -- DNS failures now block requests +- Explicit 3-way CSRF state handling (no state/missing from callback/mismatch) + +### Files Modified (3) +1. `app/data/action/http_request.py` - SSRF redirect validation, fail-closed, helper extraction +2. `agent_core/core/credentials/oauth_server.py` - CSRF enforcement, timing-safe comparison +3. `app/external_comms/credentials.py` - Atomic file permissions + +### Commits +- `c8389b2` - security: fix SSRF NameError, timing-safe OAuth state, atomic credential perms +- `91ac962` - security: fix SSRF redirect bypass, fail-closed validation, CSRF enforcement + +### Code Design Assessment +- Architecture is solid (protocol-driven, registry DI, plugin-based) +- Security was bolted on after the fact, not designed in +- Zero test infrastructure -- biggest gap for security confidence +- Prompt sanitizer is fundamentally unsound (regex vs semantic attacks) +- Blocklist approach is inherently weaker than allowlist for file/URL validation + +### Advisor Recommendations (prioritized) +1. **PR A** (done): SSRF redirect bypass + fail-closed + CSRF enforcement +2. **PR B** (next): pytest infrastructure + action file unit tests +3. **PR C** (future): Extract shared validation utilities (`validate_url()`, `validate_file_path()`) +4. **Skip**: Prompt sanitizer iteration (flawed approach, suggest architecture change instead) + +### Next Steps +- [ ] Monitor PR CraftOS-dev/CraftBot#198 for maintainer feedback +- [ ] PR B: Add pytest framework with tests for http_request, read_file, write_file +- [ ] PR C: Extract centralized validation utilities +- [ ] File GitHub issue recommending prompt injection architecture change +- [ ] Consider filing security advisory for embedded OAuth credentials (CWE-798)