[security] fix(gateway): harden callbacks, CDP, and health diagnostics

gateway/platforms/api_server.py

@@ -924,8 +924,14 @@ async def _handle_health_detailed(self, request: "web.Request") -> "web.Response
 
         Returns gateway state, connected platforms, PID, and uptime so the
         dashboard can display full status without needing a shared PID file or
-        /proc access.  No authentication required.
+        /proc access. When the API server is configured with an API key, this
+        detailed diagnostic endpoint requires the same authentication as other
+        state-bearing API routes.
         """
+        auth_err = self._check_auth(request)
+        if auth_err:
+            return auth_err
+
         from gateway.status import read_runtime_status
 
         runtime = read_runtime_status() or {}

gateway/platforms/telegram.py

@@ -2912,6 +2912,16 @@ async def _handle_callback_query(
         if data.startswith(("mp:", "mm:", "mb", "mx", "mg:")):
             chat_id = str(query.message.chat_id) if query.message else None
             if chat_id:
+                caller_id = str(getattr(query.from_user, "id", ""))
+                if not self._is_callback_user_authorized(
+                    caller_id,
+                    chat_id=chat_id,
+                    chat_type=query_chat_type,
+                    thread_id=str(query_thread_id) if query_thread_id is not None else None,
+                    user_name=query_user_name,
+                ):
+                    await query.answer(text="⛔ You are not authorized to change models.")
+                    return
                 await self._handle_model_picker_callback(query, data, chat_id)
             return
 

tests/gateway/test_api_server.py

@@ -518,13 +518,31 @@ async def test_health_detailed_no_runtime_status(self, adapter):
                 assert data["platforms"] == {}
 
     @pytest.mark.asyncio
-    async def test_health_detailed_does_not_require_auth(self, auth_adapter):
-        """Health detailed endpoint should be accessible without auth, like /health."""
+    async def test_health_detailed_requires_auth_when_api_key_configured(self, auth_adapter):
+        """Detailed runtime diagnostics require auth when API auth is configured."""
         app = _create_app(auth_adapter)
-        with patch("gateway.status.read_runtime_status", return_value=None):
+        runtime_status = {
+            "gateway_state": "running",
+            "platforms": {
+                "telegram": {
+                    "state": "error",
+                    "error_message": "sensitive diagnostic text",
+                }
+            },
+            "active_agents": 1,
+        }
+        with patch("gateway.status.read_runtime_status", return_value=runtime_status):
             async with TestClient(TestServer(app)) as cli:
-                resp = await cli.get("/health/detailed")
-                assert resp.status == 200
+                unauth = await cli.get("/health/detailed")
+                assert unauth.status == 401
+
+                auth = await cli.get(
+                    "/health/detailed",
+                    headers={"Authorization": "Bearer sk-secret"},
+                )
+                assert auth.status == 200
+                data = await auth.json()
+                assert data["platforms"]["telegram"]["error_message"] == "sensitive diagnostic text"
 
 
 # ---------------------------------------------------------------------------

tests/gateway/test_telegram_approval_buttons.py

@@ -452,14 +452,17 @@ async def test_already_resolved(self):
 
     @pytest.mark.asyncio
     async def test_model_picker_callback_not_affected(self):
-        """Ensure model picker callbacks still route correctly."""
+        """Ensure authorized model picker callbacks still route correctly."""
         adapter = _make_adapter()
 
         query = AsyncMock()
         query.data = "mp:some_provider"
         query.message = MagicMock()
         query.message.chat_id = 12345
+        query.message.chat.type = "private"
         query.from_user = MagicMock()
+        query.from_user.id = 111
+        query.from_user.first_name = "Alice"
 
         update = MagicMock()
         update.callback_query = query
@@ -468,10 +471,72 @@ async def test_model_picker_callback_not_affected(self):
         # Model picker callback should be handled (not crash)
         # We just verify it doesn't try to resolve an approval
         with patch("tools.approval.resolve_gateway_approval") as mock_resolve:
-            with patch.object(adapter, "_handle_model_picker_callback", new_callable=AsyncMock):
-                await adapter._handle_callback_query(update, context)
+            with patch.dict(os.environ, {"TELEGRAM_ALLOWED_USERS": "111"}):
+                with patch.object(adapter, "_handle_model_picker_callback", new_callable=AsyncMock) as mock_picker:
+                    await adapter._handle_callback_query(update, context)
 
         mock_resolve.assert_not_called()
+        mock_picker.assert_awaited_once_with(query, "mp:some_provider", "12345")
+
+    @pytest.mark.asyncio
+    async def test_model_picker_callback_rejects_unauthorized_user(self):
+        """Model picker buttons should honor TELEGRAM_ALLOWED_USERS."""
+        adapter = _make_adapter()
+
+        query = AsyncMock()
+        query.data = "mm:0"
+        query.message = MagicMock()
+        query.message.chat_id = 12345
+        query.message.chat.type = "supergroup"
+        query.message.message_thread_id = 99
+        query.from_user = MagicMock()
+        query.from_user.id = 222
+        query.from_user.first_name = "Mallory"
+        query.answer = AsyncMock()
+
+        update = MagicMock()
+        update.callback_query = query
+        context = MagicMock()
+
+        with patch.dict(os.environ, {"TELEGRAM_ALLOWED_USERS": "111"}):
+            with patch.object(adapter, "_handle_model_picker_callback", new_callable=AsyncMock) as mock_picker:
+                await adapter._handle_callback_query(update, context)
+
+        mock_picker.assert_not_called()
+        query.answer.assert_called_once()
+        assert "not authorized" in query.answer.call_args[1]["text"].lower()
+
+    @pytest.mark.asyncio
+    async def test_model_picker_callback_rejects_user_blocked_by_global_allowlist(self):
+        adapter = _make_adapter()
+        runner = _AuthRunner(authorized=False)
+        adapter._message_handler = runner._handle_message
+
+        query = AsyncMock()
+        query.data = "mg:1"
+        query.message = MagicMock()
+        query.message.chat_id = 12345
+        query.message.chat.type = "private"
+        query.from_user = MagicMock()
+        query.from_user.id = 222
+        query.from_user.first_name = "Mallory"
+        query.answer = AsyncMock()
+
+        update = MagicMock()
+        update.callback_query = query
+        context = MagicMock()
+
+        with patch.dict(os.environ, {"TELEGRAM_ALLOWED_USERS": ""}):
+            with patch.object(adapter, "_handle_model_picker_callback", new_callable=AsyncMock) as mock_picker:
+                await adapter._handle_callback_query(update, context)
+
+        mock_picker.assert_not_called()
+        query.answer.assert_called_once()
+        assert "not authorized" in query.answer.call_args[1]["text"].lower()
+        assert runner.last_source is not None
+        assert runner.last_source.platform == Platform.TELEGRAM
+        assert runner.last_source.user_id == "222"
+        assert runner.last_source.chat_id == "12345"
 
     @pytest.mark.asyncio
     async def test_update_prompt_callback_not_affected(self, tmp_path):

tests/tools/test_browser_cdp_override.py

@@ -12,7 +12,8 @@ class TestResolveCdpOverride:
     def test_keeps_full_devtools_websocket_url(self):
         from tools.browser_tool import _resolve_cdp_override
 
-        assert _resolve_cdp_override(WS_URL) == WS_URL
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True):
+            assert _resolve_cdp_override(WS_URL) == WS_URL
 
     def test_resolves_http_discovery_endpoint_to_websocket(self):
         from tools.browser_tool import _resolve_cdp_override
@@ -21,11 +22,12 @@ def test_resolves_http_discovery_endpoint_to_websocket(self):
         response.raise_for_status.return_value = None
         response.json.return_value = {"webSocketDebuggerUrl": WS_URL}
 
-        with patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
+             patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
             resolved = _resolve_cdp_override(HTTP_URL)
 
         assert resolved == WS_URL
-        mock_get.assert_called_once_with(VERSION_URL, timeout=10)
+        mock_get.assert_called_once_with(VERSION_URL, timeout=10, allow_redirects=False)
 
     def test_resolves_bare_ws_hostport_to_discovery_websocket(self):
         from tools.browser_tool import _resolve_cdp_override
@@ -34,16 +36,18 @@ def test_resolves_bare_ws_hostport_to_discovery_websocket(self):
         response.raise_for_status.return_value = None
         response.json.return_value = {"webSocketDebuggerUrl": WS_URL}
 
-        with patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
+             patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
             resolved = _resolve_cdp_override(f"ws://{HOST}:{PORT}")
 
         assert resolved == WS_URL
-        mock_get.assert_called_once_with(VERSION_URL, timeout=10)
+        mock_get.assert_called_once_with(VERSION_URL, timeout=10, allow_redirects=False)
 
     def test_falls_back_to_raw_url_when_discovery_fails(self):
         from tools.browser_tool import _resolve_cdp_override
 
-        with patch("tools.browser_tool.requests.get", side_effect=RuntimeError("boom")):
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
+             patch("tools.browser_tool.requests.get", side_effect=RuntimeError("boom")):
             assert _resolve_cdp_override(HTTP_URL) == HTTP_URL
 
     def test_normalizes_provider_returned_http_cdp_url_when_creating_session(self, monkeypatch):
@@ -68,14 +72,16 @@ def test_normalizes_provider_returned_http_cdp_url_when_creating_session(self, m
         monkeypatch.setattr(browser_tool, "_get_cdp_override", lambda: "")
         monkeypatch.setattr(browser_tool, "_get_cloud_provider", lambda: provider)
 
-        with patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
+             patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
             session_info = browser_tool._get_session_info("task-browser-use")
 
         assert session_info["cdp_url"] == WS_URL
         provider.create_session.assert_called_once_with("task-browser-use")
         mock_get.assert_called_once_with(
             "https://cdp.browser-use.example/session/json/version",
             timeout=10,
+            allow_redirects=False,
         )
 
 
@@ -95,11 +101,12 @@ def test_prefers_env_var_over_config(self, monkeypatch):
         response.raise_for_status.return_value = None
         response.json.return_value = {"webSocketDebuggerUrl": WS_URL}
 
-        with patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
+             patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
             resolved = browser_tool._get_cdp_override()
 
         assert resolved == WS_URL
-        mock_get.assert_called_once_with(VERSION_URL, timeout=10)
+        mock_get.assert_called_once_with(VERSION_URL, timeout=10, allow_redirects=False)
 
     def test_uses_config_browser_cdp_url_when_env_missing(self, monkeypatch):
         import tools.browser_tool as browser_tool
@@ -111,8 +118,45 @@ def test_uses_config_browser_cdp_url_when_env_missing(self, monkeypatch):
         response.json.return_value = {"webSocketDebuggerUrl": WS_URL}
 
         with patch("hermes_cli.config.read_raw_config", return_value={"browser": {"cdp_url": HTTP_URL}}), \
+             patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=True), \
              patch("tools.browser_tool.requests.get", return_value=response) as mock_get:
             resolved = browser_tool._get_cdp_override()
 
         assert resolved == WS_URL
-        mock_get.assert_called_once_with(VERSION_URL, timeout=10)
+        mock_get.assert_called_once_with(VERSION_URL, timeout=10, allow_redirects=False)
+
+    def test_blocks_unsafe_http_discovery_endpoint_without_request(self):
+        from tools.browser_tool import _resolve_cdp_override
+
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=False), \
+             patch("tools.browser_tool.requests.get") as mock_get:
+            assert _resolve_cdp_override("http://169.254.169.254:9222") == ""
+
+        mock_get.assert_not_called()
+
+    def test_blocks_unsafe_websocket_endpoint(self):
+        from tools.browser_tool import _resolve_cdp_override
+
+        unsafe_ws = "ws://169.254.169.254:9222/devtools/browser/secret"
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=False):
+            assert _resolve_cdp_override(unsafe_ws) == ""
+
+    def test_blocks_unsafe_non_devtools_websocket_endpoint(self):
+        from tools.browser_tool import _resolve_cdp_override
+
+        unsafe_ws = "ws://169.254.169.254:9222/custom/path"
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", return_value=False):
+            assert _resolve_cdp_override(unsafe_ws) == ""
+
+    def test_blocks_unsafe_returned_websocket(self):
+        from tools.browser_tool import _resolve_cdp_override
+
+        response = Mock()
+        response.raise_for_status.return_value = None
+        response.json.return_value = {
+            "webSocketDebuggerUrl": "ws://169.254.169.254:9222/devtools/browser/secret"
+        }
+
+        with patch("tools.browser_tool._is_safe_cdp_endpoint", side_effect=[True, False]), \
+             patch("tools.browser_tool.requests.get", return_value=response):
+            assert _resolve_cdp_override(HTTP_URL) == ""

tools/browser_tool.py

@@ -54,6 +54,7 @@
 import json
 import logging
 import os
+import ipaddress
 import re
 import signal
 import subprocess
@@ -65,6 +66,7 @@
 import requests
 from typing import Dict, Any, Optional, List, Tuple
 from pathlib import Path
+from urllib.parse import urlparse
 from agent.auxiliary_client import call_llm
 from hermes_constants import get_hermes_home
 from utils import is_truthy_value
@@ -232,6 +234,41 @@ def _get_extraction_model() -> Optional[str]:
     return os.getenv("AUXILIARY_WEB_EXTRACT_MODEL", "").strip() or None
 
 
+def _is_loopback_host(hostname: str) -> bool:
+    """Return True for localhost / loopback CDP endpoints."""
+    host = (hostname or "").strip().lower().rstrip(".")
+    if host == "localhost":
+        return True
+    try:
+        return ipaddress.ip_address(host).is_loopback
+    except ValueError:
+        return False
+
+
+def _is_safe_cdp_endpoint(url: str) -> bool:
+    """Return whether Hermes may probe/connect to a CDP endpoint.
+
+    Local Chromium normally exposes CDP on loopback, so loopback endpoints are
+    allowed. Other targets must pass the ordinary URL SSRF guard. The
+    always-blocked metadata/link-local floor is enforced for every endpoint,
+    including when private URLs are globally allowed.
+    """
+    try:
+        parsed = urlparse(url)
+        if (parsed.scheme or "").lower() not in {"http", "https", "ws", "wss"}:
+            return False
+        if not parsed.hostname:
+            return False
+        if _is_always_blocked_url(url):
+            return False
+        if _is_loopback_host(parsed.hostname):
+            return True
+        return _is_safe_url(url)
+    except Exception as exc:
+        logger.warning("Blocked CDP endpoint due to safety-check error for %s: %s", url, exc)
+        return False
+
+
 def _resolve_cdp_override(cdp_url: str) -> str:
     """Normalize a user-supplied CDP endpoint into a concrete connectable URL.
 
@@ -250,22 +287,32 @@ def _resolve_cdp_override(cdp_url: str) -> str:
 
     lowered = raw.lower()
     if "/devtools/browser/" in lowered:
+        if not _is_safe_cdp_endpoint(raw):
+            logger.warning("Blocked unsafe CDP websocket endpoint: %s", raw)
+            return ""
         return raw
 
     discovery_url = raw
     if lowered.startswith(("ws://", "wss://")):
         if raw.count(":") == 2 and raw.rstrip("/").rsplit(":", 1)[-1].isdigit() and "/" not in raw.split(":", 2)[-1]:
             discovery_url = ("http://" if lowered.startswith("ws://") else "https://") + raw.split("://", 1)[1]
         else:
+            if not _is_safe_cdp_endpoint(raw):
+                logger.warning("Blocked unsafe CDP websocket endpoint: %s", raw)
+                return ""
             return raw
 
     if discovery_url.lower().endswith("/json/version"):
         version_url = discovery_url
     else:
         version_url = discovery_url.rstrip("/") + "/json/version"
 
+    if not _is_safe_cdp_endpoint(version_url):
+        logger.warning("Blocked unsafe CDP discovery endpoint: %s", version_url)
+        return ""
+
     try:
-        response = requests.get(version_url, timeout=10)
+        response = requests.get(version_url, timeout=10, allow_redirects=False)
         response.raise_for_status()
         payload = response.json()
     except Exception as exc:
@@ -274,6 +321,9 @@ def _resolve_cdp_override(cdp_url: str) -> str:
 
     ws_url = str(payload.get("webSocketDebuggerUrl") or "").strip()
     if ws_url:
+        if not _is_safe_cdp_endpoint(ws_url):
+            logger.warning("Blocked unsafe CDP websocket returned by %s: %s", version_url, ws_url)
+            return ""
         logger.info("Resolved CDP endpoint %s -> %s", raw, ws_url)
         return ws_url