security: fix SSRF redirect bypass, timing-safe OAuth, atomic credentials

agent_core/core/credentials/oauth_server.py

@@ -22,6 +22,7 @@
 """
 
 import asyncio
+import hmac
 import html
 import ipaddress
 import logging
@@ -126,7 +127,16 @@ 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:
+            if expected_state is None:
+                # 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 returned_state is None:
+                # 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)

app/data/action/http_request.py

@@ -165,47 +165,84 @@ def send_http_requests(input_data: dict) -> dict:
     allow_redirects = bool(input_data.get('allow_redirects', True))
     verify_tls = bool(input_data.get('verify_tls', True))
     allowed = {'GET','POST','PUT','PATCH','DELETE'}
+
+    def _error(message, status_code=0, final_url='', elapsed_ms=0):
+        return {'status':'error','status_code':status_code,'response_headers':{},'body':'','final_url':final_url,'elapsed_ms':elapsed_ms,'message':message}
+
     if method not in allowed:
-        return {'status':'error','status_code':0,'response_headers':{},'body':'','final_url':'','elapsed_ms':0,'message':'Unsupported method.'}
+        return _error('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.'}
+        return _error('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, urljoin as _urljoin
+    import ipaddress as _ipaddress
+    import socket as _socket
+
+    _BLOCKED_HOSTS = frozenset({'169.254.169.254', 'metadata.google.internal', 'metadata.internal'})
+
+    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)
+        # Block non-HTTP schemes (file://, gopher://, etc.)
+        if _parsed.scheme not in ('http', 'https'):
+            return f'Blocked: only http/https schemes are allowed (got {_parsed.scheme}).'
         _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.'}
-        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: requests to private/internal addresses ({_hostname}) are not allowed.'
+        except (_socket.gaierror, ValueError):
+            return f'Blocked: could not resolve hostname ({_hostname}).'
+        return None
+
+    ssrf_error = _is_url_ssrf_safe(url)
+    if ssrf_error:
+        return _error(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.'}
+        return _error('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.'}
+        return _error('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:
         kwargs['data'] = data_body
     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://')):
+                redirect_url = _urljoin(resp.url, redirect_url)
+            redirect_error = _is_url_ssrf_safe(redirect_url)
+            if redirect_error:
+                return _error(f'Redirect blocked: {redirect_error}', status_code=resp.status_code, final_url=resp.url, elapsed_ms=int((time.time()-t0)*1000))
+            # 307/308 preserve method; all others downgrade to GET per RFC 7231
+            redirect_method = method if resp.status_code in (307, 308) else 'GET'
+            redirect_kwargs = {**kwargs, 'allow_redirects': False}
+            # Strip body on method downgrade to GET
+            if redirect_method == 'GET':
+                redirect_kwargs.pop('json', None)
+                redirect_kwargs.pop('data', None)
+            # Strip auth headers on cross-origin redirects
+            _orig_host = _urlparse(url).netloc
+            _redir_host = _urlparse(redirect_url).netloc
+            if _orig_host != _redir_host and 'Authorization' in redirect_kwargs.get('headers', {}):
+                redirect_kwargs['headers'] = {k: v for k, v in redirect_kwargs['headers'].items() if k != 'Authorization'}
+            resp = requests.request(redirect_method, redirect_url, **redirect_kwargs)
         elapsed_ms = int((time.time() - t0) * 1000)
         resp_headers = {k: v for k, v in resp.headers.items()}
         parsed_json = None

app/external_comms/credentials.py

@@ -83,13 +83,11 @@ 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)
+        os.fchmod(fd, 0o600)  # Ensure permissions even if file pre-existed
+        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}")