From b46830cc2eb6ee461b286433fb65e7c5f47e4dbc Mon Sep 17 00:00:00 2001 From: qbit-mirror-bot Date: Wed, 1 Jul 2026 11:18:40 +0000 Subject: [PATCH] fix(api-server): require auth for /health/detailed and fail closed on weak keys --- gateway/platforms/api_server.py | 116 ++++++++++++-------- tests/gateway/test_api_server.py | 91 ++++++++++++++- tests/gateway/test_api_server_bind_guard.py | 13 +++ 3 files changed, 173 insertions(+), 47 deletions(-) diff --git a/gateway/platforms/api_server.py b/gateway/platforms/api_server.py index ea91aea4329b..1e4ad4c3a9ad 100644 --- a/gateway/platforms/api_server.py +++ b/gateway/platforms/api_server.py @@ -1108,6 +1108,18 @@ def _create_agent( reasoning_config = GatewayRunner._load_reasoning_config() model = _resolve_gateway_model() + # When the primary provider's auth fails (expired token / 429 quota + # cap), _resolve_runtime_agent_kwargs() falls through to the fallback + # provider chain, whose runtime dict carries its own ``model`` key. + # Pop it and let it override the config model, mirroring the native + # gateway path (_resolve_session_agent_runtime in run.py). Otherwise + # the explicit ``model=model`` below collides with the ``**runtime_kwargs`` + # spread → "got multiple values for keyword argument 'model'", 500ing + # every /v1/chat/completions request while a fallback is active. + runtime_model = runtime_kwargs.pop("model", None) + if runtime_model: + model = runtime_model + user_config = _load_gateway_config() enabled_toolsets = sorted(_get_platform_tools(user_config, "api_server")) @@ -1153,8 +1165,12 @@ 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. Requires the same Bearer auth as other API routes. """ + auth_err = self._check_auth(request) + if auth_err: + return auth_err + from gateway.status import ( derive_gateway_busy, derive_gateway_drainable, @@ -3982,7 +3998,12 @@ async def _handle_runs(self, request: "web.Request") -> "web.Response": run_id = f"run_{uuid.uuid4().hex}" session_id = body.get("session_id") or stored_session_id or run_id - approval_session_key = gateway_session_key or session_id or run_id + # Approval queues gate host-side tool execution and must be isolated + # per API run. Client-provided session IDs and memory session keys are + # conversation/memory scopes, not authorization namespaces: multiple + # concurrent runs can intentionally share them, and resolving an + # approval for one run must not unblock another run's dangerous command. + approval_session_key = run_id ephemeral_system_prompt = instructions loop = asyncio.get_running_loop() q: "asyncio.Queue[Optional[Dict]]" = asyncio.Queue() @@ -4437,12 +4458,60 @@ async def _sweep_orphaned_runs(self) -> None: # BasePlatformAdapter interface # ------------------------------------------------------------------ + def _api_key_passes_startup_guard(self) -> bool: + """Return True when API_SERVER_KEY is present and strong enough to start.""" + if not self._api_key: + logger.error( + "[%s] Refusing to start: API_SERVER_KEY is required for the API server, " + "including loopback-only binds on %s.", + self.name, self._host, + ) + return False + + try: + from hermes_cli.auth import has_usable_secret + if not has_usable_secret(self._api_key, min_length=16): + logger.error( + "[%s] Refusing to start: API_SERVER_KEY is a " + "placeholder or too short (<16 chars). This endpoint " + "dispatches terminal-capable agent work — a guessable " + "key is remote code execution. Generate a strong secret " + "(e.g. `openssl rand -hex 32`) and set API_SERVER_KEY " + "before starting the API server on %s.", + self.name, self._host, + ) + return False + except ImportError: + pass + return True + + def _port_is_available(self) -> bool: + """Return True when the configured listen port is free.""" + try: + with _socket.socket(_socket.AF_INET, _socket.SOCK_STREAM) as _s: + _s.settimeout(1) + _s.connect(('127.0.0.1', self._port)) + logger.error( + "[%s] Port %d already in use. Set a different port in config.yaml: " + "platforms.api_server.port", + self.name, self._port, + ) + return False + except (ConnectionRefusedError, OSError): + return True + async def connect(self, *, is_reconnect: bool = False) -> bool: """Start the aiohttp web server.""" if not AIOHTTP_AVAILABLE: logger.warning("[%s] aiohttp not installed", self.name) return False + if not self._api_key_passes_startup_guard(): + return False + + if not self._port_is_available(): + return False + try: mws = [mw for mw in (cors_middleware, body_limit_middleware, security_headers_middleware) if mw is not None] self._app = web.Application(middlewares=mws, client_max_size=MAX_REQUEST_BYTES) @@ -4503,39 +4572,6 @@ async def connect(self, *, is_reconnect: bool = False) -> bool: if hasattr(sweep_task, "add_done_callback"): sweep_task.add_done_callback(self._background_tasks.discard) - # Refuse to start without authentication. The API server can - # dispatch terminal-capable agent work, so every deployment needs - # an explicit API_SERVER_KEY regardless of bind address. - if not self._api_key: - logger.error( - "[%s] Refusing to start: API_SERVER_KEY is required for the API server, " - "including loopback-only binds on %s.", - self.name, self._host, - ) - return False - - # Refuse to start network-accessible with a placeholder or weak key. - # Ported from openclaw/openclaw#64586; entropy floor raised to 16 in - # the June 2026 hermes-0day hardening (an 8-char key dispatching - # terminal-capable agent work on a public bind is brute-forceable). - if is_network_accessible(self._host) and self._api_key: - try: - from hermes_cli.auth import has_usable_secret - if not has_usable_secret(self._api_key, min_length=16): - logger.error( - "[%s] Refusing to start: API_SERVER_KEY is a " - "placeholder or too short (<16 chars) for a " - "network-accessible bind. This endpoint dispatches " - "terminal-capable agent work — a guessable key is " - "remote code execution. Generate a strong secret " - "(e.g. `openssl rand -hex 32`) and set " - "API_SERVER_KEY before exposing it on %s.", - self.name, self._host, - ) - return False - except ImportError: - pass - # Loud warning when a network-accessible API server runs against an # unsandboxed local terminal backend. The API server can drive the # agent's terminal/file tools as the host user; on a public bind @@ -4564,16 +4600,6 @@ async def connect(self, *, is_reconnect: bool = False) -> bool: self.name, self._host, ) - # Port conflict detection — fail fast if port is already in use - try: - with _socket.socket(_socket.AF_INET, _socket.SOCK_STREAM) as _s: - _s.settimeout(1) - _s.connect(('127.0.0.1', self._port)) - logger.error('[%s] Port %d already in use. Set a different port in config.yaml: platforms.api_server.port', self.name, self._port) - return False - except (ConnectionRefusedError, OSError): - pass # port is free - self._runner = web.AppRunner(self._app) await self._runner.setup() self._site = web.TCPSite(self._runner, self._host, self._port) diff --git a/tests/gateway/test_api_server.py b/tests/gateway/test_api_server.py index c0a2f52d6c70..a94b34f4dc9f 100644 --- a/tests/gateway/test_api_server.py +++ b/tests/gateway/test_api_server.py @@ -400,6 +400,84 @@ def __init__(self, **kwargs): assert isinstance(agent, FakeAgent) assert captured["max_iterations"] == 200 + def test_create_agent_handles_fallback_model_kwarg_collision(self, monkeypatch): + """When the primary provider auth-fails, _resolve_runtime_agent_kwargs() + returns a runtime dict that carries its own ``model`` key. _create_agent + must pop it and let it override the config model — otherwise the explicit + ``model=`` collides with ``**runtime_kwargs`` and every request 500s with + "got multiple values for keyword argument 'model'".""" + captured = {} + + class FakeAgent: + def __init__(self, **kwargs): + captured.update(kwargs) + + monkeypatch.setattr("run_agent.AIAgent", FakeAgent) + monkeypatch.setattr( + "gateway.run._resolve_runtime_agent_kwargs", + lambda: { + "provider": "openrouter", + "base_url": "https://openrouter.ai/api/v1", + "api_mode": "chat_completions", + "model": "anthropic/claude-haiku", # from the fallback entry + }, + ) + monkeypatch.setattr("gateway.run._resolve_gateway_model", lambda: "primary/model") + monkeypatch.setattr("gateway.run._load_gateway_config", lambda: {}) + monkeypatch.setattr( + "gateway.run.GatewayRunner._load_reasoning_config", + staticmethod(lambda: {}), + ) + monkeypatch.setattr("gateway.run.GatewayRunner._load_fallback_model", staticmethod(lambda: None)) + monkeypatch.setattr("gateway.run._current_max_iterations", lambda: 90) + monkeypatch.setattr("hermes_cli.tools_config._get_platform_tools", lambda *_: set()) + + adapter = APIServerAdapter(PlatformConfig(enabled=True)) + monkeypatch.setattr(adapter, "_ensure_session_db", lambda: None) + + # Must not raise TypeError on the duplicate 'model' kwarg. + agent = adapter._create_agent(session_id="api-session") + + assert isinstance(agent, FakeAgent) + # Fallback model overrides the config model, mirroring the native path. + assert captured["model"] == "anthropic/claude-haiku" + + def test_create_agent_keeps_config_model_when_runtime_omits_it(self, monkeypatch): + """Happy path (no fallback active): runtime_kwargs has no 'model', so the + resolved gateway model is used unchanged. Regression guard for the pop.""" + captured = {} + + class FakeAgent: + def __init__(self, **kwargs): + captured.update(kwargs) + + monkeypatch.setattr("run_agent.AIAgent", FakeAgent) + monkeypatch.setattr( + "gateway.run._resolve_runtime_agent_kwargs", + lambda: { + "provider": "openrouter", + "base_url": "https://openrouter.ai/api/v1", + "api_mode": "chat_completions", + }, + ) + monkeypatch.setattr("gateway.run._resolve_gateway_model", lambda: "primary/model") + monkeypatch.setattr("gateway.run._load_gateway_config", lambda: {}) + monkeypatch.setattr( + "gateway.run.GatewayRunner._load_reasoning_config", + staticmethod(lambda: {}), + ) + monkeypatch.setattr("gateway.run.GatewayRunner._load_fallback_model", staticmethod(lambda: None)) + monkeypatch.setattr("gateway.run._current_max_iterations", lambda: 90) + monkeypatch.setattr("hermes_cli.tools_config._get_platform_tools", lambda *_: set()) + + adapter = APIServerAdapter(PlatformConfig(enabled=True)) + monkeypatch.setattr(adapter, "_ensure_session_db", lambda: None) + + agent = adapter._create_agent(session_id="api-session") + + assert isinstance(agent, FakeAgent) + assert captured["model"] == "primary/model" + # --------------------------------------------------------------------------- # Auth checking @@ -694,12 +772,21 @@ async def test_health_detailed_no_runtime_status(self, adapter): assert data["gateway_drainable"] is False @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(self, auth_adapter): + """Detailed health must not leak runtime state without Bearer auth.""" app = _create_app(auth_adapter) with patch("gateway.status.read_runtime_status", return_value=None): async with TestClient(TestServer(app)) as cli: resp = await cli.get("/health/detailed") + assert resp.status == 401 + + @pytest.mark.asyncio + async def test_health_detailed_allows_authenticated_request(self, auth_adapter): + app = _create_app(auth_adapter) + headers = {"Authorization": f"Bearer {auth_adapter._api_key}"} + with patch("gateway.status.read_runtime_status", return_value={"gateway_state": "running"}): + async with TestClient(TestServer(app)) as cli: + resp = await cli.get("/health/detailed", headers=headers) assert resp.status == 200 diff --git a/tests/gateway/test_api_server_bind_guard.py b/tests/gateway/test_api_server_bind_guard.py index edab34eb3825..706059887d96 100644 --- a/tests/gateway/test_api_server_bind_guard.py +++ b/tests/gateway/test_api_server_bind_guard.py @@ -119,6 +119,19 @@ async def test_refuses_loopback_without_key(self): assert is_network_accessible(adapter._host) is False result = await adapter.connect() assert result is False + assert adapter._app is None + assert adapter._background_tasks == set() + + @pytest.mark.asyncio + async def test_refuses_weak_key_without_partial_startup(self): + """Weak API_SERVER_KEY rejection must not create app or background tasks.""" + adapter = APIServerAdapter( + PlatformConfig(enabled=True, extra={"host": "127.0.0.1", "key": "short"}), + ) + result = await adapter.connect() + assert result is False + assert adapter._app is None + assert adapter._background_tasks == set() @pytest.mark.asyncio async def test_allows_wildcard_with_key(self):