mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
refactor(acp): validate method_id against advertised provider in authenticate() (#13468)
* feat(models): hide OpenRouter models that don't advertise tool support Port from Kilo-Org/kilocode#9068. hermes-agent is tool-calling-first — every provider path assumes the model can invoke tools. Models whose OpenRouter supported_parameters doesn't include 'tools' (e.g. image-only or completion-only models) cannot be driven by the agent loop and fail at the first tool call. Filter them out of fetch_openrouter_models() so they never appear in the model picker (`hermes model`, setup wizard, /model slash command). Permissive when the field is missing — OpenRouter-compatible gateways (Nous Portal, private mirrors, older snapshots) don't always populate supported_parameters. Treat missing as 'unknown → allow' rather than silently emptying the picker on those gateways. Only hide models whose supported_parameters is an explicit list that omits tools. Tests cover: tools present → kept, tools absent → dropped, field missing → kept, malformed non-list → kept, non-dict item → kept, empty list → dropped. * refactor(acp): validate method_id against advertised provider in authenticate() Previously authenticate() accepted any method_id whenever the server had provider credentials configured. This was not a vulnerability under the personal-assistant trust model (ACP is stdio-only, local-trust — anything that can reach the transport is already code-execution-equivalent to the user), but it was sloppy API hygiene: the advertised auth_methods list from initialize() was effectively ignored. Now authenticate() only returns AuthenticateResponse when method_id matches the currently-advertised provider (case-insensitive). Mismatched or missing method_id returns None, consistent with the no-credentials case. Raised by xeloxa via GHSA-g5pf-8w9m-h72x. Declined as a CVE (ACP transport is stdio, local-trust model), but the correctness fix is worth having on its own.
This commit is contained in:
parent
d1cfe53d85
commit
c6974043ef
4 changed files with 191 additions and 9 deletions
|
|
@ -51,7 +51,7 @@ try:
|
|||
except ImportError:
|
||||
from acp.schema import AuthMethod as AuthMethodAgent # type: ignore[attr-defined]
|
||||
|
||||
from acp_adapter.auth import detect_provider, has_provider
|
||||
from acp_adapter.auth import detect_provider
|
||||
from acp_adapter.events import (
|
||||
make_message_cb,
|
||||
make_step_cb,
|
||||
|
|
@ -351,9 +351,18 @@ class HermesACPAgent(acp.Agent):
|
|||
)
|
||||
|
||||
async def authenticate(self, method_id: str, **kwargs: Any) -> AuthenticateResponse | None:
|
||||
if has_provider():
|
||||
return AuthenticateResponse()
|
||||
return None
|
||||
# Only accept authenticate() calls whose method_id matches the
|
||||
# provider we advertised in initialize(). Without this check,
|
||||
# authenticate() would acknowledge any method_id as long as the
|
||||
# server has provider credentials configured — harmless under
|
||||
# Hermes' threat model (ACP is stdio-only, local-trust), but poor
|
||||
# API hygiene and confusing if ACP ever grows multi-method auth.
|
||||
provider = detect_provider()
|
||||
if not provider:
|
||||
return None
|
||||
if not isinstance(method_id, str) or method_id.strip().lower() != provider:
|
||||
return None
|
||||
return AuthenticateResponse()
|
||||
|
||||
# ---- Session management -------------------------------------------------
|
||||
|
||||
|
|
|
|||
|
|
@ -688,6 +688,31 @@ def _openrouter_model_is_free(pricing: Any) -> bool:
|
|||
return False
|
||||
|
||||
|
||||
def _openrouter_model_supports_tools(item: Any) -> bool:
|
||||
"""Return True when the model's ``supported_parameters`` advertise tool calling.
|
||||
|
||||
hermes-agent is tool-calling-first — every provider path assumes the model
|
||||
can invoke tools. Models that don't advertise ``tools`` in their
|
||||
``supported_parameters`` (e.g. image-only or completion-only models) cannot
|
||||
be driven by the agent loop and would fail at the first tool call.
|
||||
|
||||
**Permissive when the field is missing.** Some OpenRouter-compatible gateways
|
||||
(Nous Portal, private mirrors, older catalog snapshots) don't populate
|
||||
``supported_parameters`` at all. Treat that as "unknown capability → allow"
|
||||
so the picker doesn't silently empty for those users. Only hide models
|
||||
whose ``supported_parameters`` is an explicit list that omits ``tools``.
|
||||
|
||||
Ported from Kilo-Org/kilocode#9068.
|
||||
"""
|
||||
if not isinstance(item, dict):
|
||||
return True
|
||||
params = item.get("supported_parameters")
|
||||
if not isinstance(params, list):
|
||||
# Field absent / malformed / None — be permissive.
|
||||
return True
|
||||
return "tools" in params
|
||||
|
||||
|
||||
def fetch_openrouter_models(
|
||||
timeout: float = 8.0,
|
||||
*,
|
||||
|
|
@ -730,6 +755,11 @@ def fetch_openrouter_models(
|
|||
live_item = live_by_id.get(preferred_id)
|
||||
if live_item is None:
|
||||
continue
|
||||
# Hide models that don't advertise tool-calling support — hermes-agent
|
||||
# requires it and surfacing them leads to immediate runtime failures
|
||||
# when the user selects them. Ported from Kilo-Org/kilocode#9068.
|
||||
if not _openrouter_model_supports_tools(live_item):
|
||||
continue
|
||||
desc = "free" if _openrouter_model_is_free(live_item.get("pricing")) else ""
|
||||
curated.append((preferred_id, desc))
|
||||
|
||||
|
|
|
|||
|
|
@ -95,19 +95,37 @@ class TestInitialize:
|
|||
|
||||
class TestAuthenticate:
|
||||
@pytest.mark.asyncio
|
||||
async def test_authenticate_with_provider_configured(self, agent, monkeypatch):
|
||||
async def test_authenticate_with_matching_method_id(self, agent, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"acp_adapter.server.has_provider",
|
||||
lambda: True,
|
||||
"acp_adapter.server.detect_provider",
|
||||
lambda: "openrouter",
|
||||
)
|
||||
resp = await agent.authenticate(method_id="openrouter")
|
||||
assert isinstance(resp, AuthenticateResponse)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_authenticate_is_case_insensitive(self, agent, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"acp_adapter.server.detect_provider",
|
||||
lambda: "openrouter",
|
||||
)
|
||||
resp = await agent.authenticate(method_id="OpenRouter")
|
||||
assert isinstance(resp, AuthenticateResponse)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_authenticate_rejects_mismatched_method_id(self, agent, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"acp_adapter.server.detect_provider",
|
||||
lambda: "openrouter",
|
||||
)
|
||||
resp = await agent.authenticate(method_id="totally-invalid-method")
|
||||
assert resp is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_authenticate_without_provider(self, agent, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"acp_adapter.server.has_provider",
|
||||
lambda: False,
|
||||
"acp_adapter.server.detect_provider",
|
||||
lambda: None,
|
||||
)
|
||||
resp = await agent.authenticate(method_id="openrouter")
|
||||
assert resp is None
|
||||
|
|
|
|||
|
|
@ -88,6 +88,131 @@ class TestFetchOpenRouterModels:
|
|||
|
||||
assert models == OPENROUTER_MODELS
|
||||
|
||||
def test_filters_out_models_without_tool_support(self, monkeypatch):
|
||||
"""Models whose supported_parameters omits 'tools' must not appear in the picker.
|
||||
|
||||
hermes-agent is tool-calling-first — surfacing a non-tool model leads to
|
||||
immediate runtime failures when the user selects it. Ported from
|
||||
Kilo-Org/kilocode#9068.
|
||||
"""
|
||||
class _Resp:
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
def read(self):
|
||||
# opus-4.6 advertises tools → kept
|
||||
# nano-image has explicit supported_parameters that OMITS tools → dropped
|
||||
# qwen3.6-plus advertises tools → kept
|
||||
return (
|
||||
b'{"data":['
|
||||
b'{"id":"anthropic/claude-opus-4.6","pricing":{"prompt":"0.000015","completion":"0.000075"},'
|
||||
b'"supported_parameters":["temperature","tools","tool_choice"]},'
|
||||
b'{"id":"google/gemini-3-pro-image-preview","pricing":{"prompt":"0.00001","completion":"0.00003"},'
|
||||
b'"supported_parameters":["temperature","response_format"]},'
|
||||
b'{"id":"qwen/qwen3.6-plus","pricing":{"prompt":"0.000000325","completion":"0.00000195"},'
|
||||
b'"supported_parameters":["tools","temperature"]}'
|
||||
b']}'
|
||||
)
|
||||
|
||||
# Include the image-only id in the curated list so it has a chance to be surfaced.
|
||||
monkeypatch.setattr(
|
||||
_models_mod,
|
||||
"OPENROUTER_MODELS",
|
||||
[
|
||||
("anthropic/claude-opus-4.6", ""),
|
||||
("google/gemini-3-pro-image-preview", ""),
|
||||
("qwen/qwen3.6-plus", ""),
|
||||
],
|
||||
)
|
||||
monkeypatch.setattr(_models_mod, "_openrouter_catalog_cache", None)
|
||||
with patch("hermes_cli.models.urllib.request.urlopen", return_value=_Resp()):
|
||||
models = fetch_openrouter_models(force_refresh=True)
|
||||
|
||||
ids = [mid for mid, _ in models]
|
||||
assert "anthropic/claude-opus-4.6" in ids
|
||||
assert "qwen/qwen3.6-plus" in ids
|
||||
# Image-only model advertised supported_parameters WITHOUT tools → must be dropped.
|
||||
assert "google/gemini-3-pro-image-preview" not in ids
|
||||
|
||||
def test_permissive_when_supported_parameters_missing(self, monkeypatch):
|
||||
"""Models missing the supported_parameters field keep appearing in the picker.
|
||||
|
||||
Some OpenRouter-compatible gateways (Nous Portal, private mirrors, older
|
||||
catalog snapshots) don't populate supported_parameters. Treating missing
|
||||
as 'unknown → allow' prevents the picker from silently emptying on
|
||||
those gateways.
|
||||
"""
|
||||
class _Resp:
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb):
|
||||
return False
|
||||
|
||||
def read(self):
|
||||
# No supported_parameters field at all on either entry.
|
||||
return (
|
||||
b'{"data":['
|
||||
b'{"id":"anthropic/claude-opus-4.6","pricing":{"prompt":"0.000015","completion":"0.000075"}},'
|
||||
b'{"id":"qwen/qwen3.6-plus","pricing":{"prompt":"0.000000325","completion":"0.00000195"}}'
|
||||
b']}'
|
||||
)
|
||||
|
||||
monkeypatch.setattr(_models_mod, "_openrouter_catalog_cache", None)
|
||||
with patch("hermes_cli.models.urllib.request.urlopen", return_value=_Resp()):
|
||||
models = fetch_openrouter_models(force_refresh=True)
|
||||
|
||||
ids = [mid for mid, _ in models]
|
||||
assert "anthropic/claude-opus-4.6" in ids
|
||||
assert "qwen/qwen3.6-plus" in ids
|
||||
|
||||
|
||||
class TestOpenRouterToolSupportHelper:
|
||||
"""Unit tests for _openrouter_model_supports_tools (Kilo port #9068)."""
|
||||
|
||||
def test_tools_in_supported_parameters(self):
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools(
|
||||
{"id": "x", "supported_parameters": ["temperature", "tools"]}
|
||||
) is True
|
||||
|
||||
def test_tools_missing_from_supported_parameters(self):
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools(
|
||||
{"id": "x", "supported_parameters": ["temperature", "response_format"]}
|
||||
) is False
|
||||
|
||||
def test_supported_parameters_absent_is_permissive(self):
|
||||
"""Missing field → allow (so older / non-OR gateways still work)."""
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools({"id": "x"}) is True
|
||||
|
||||
def test_supported_parameters_none_is_permissive(self):
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools({"id": "x", "supported_parameters": None}) is True
|
||||
|
||||
def test_supported_parameters_malformed_is_permissive(self):
|
||||
"""Malformed (non-list) value → allow rather than silently drop."""
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools(
|
||||
{"id": "x", "supported_parameters": "tools,temperature"}
|
||||
) is True
|
||||
|
||||
def test_non_dict_item_is_permissive(self):
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools(None) is True
|
||||
assert _openrouter_model_supports_tools("anthropic/claude-opus-4.6") is True
|
||||
|
||||
def test_empty_supported_parameters_list_drops_model(self):
|
||||
"""Explicit empty list → no tools → drop."""
|
||||
from hermes_cli.models import _openrouter_model_supports_tools
|
||||
assert _openrouter_model_supports_tools(
|
||||
{"id": "x", "supported_parameters": []}
|
||||
) is False
|
||||
|
||||
|
||||
class TestFindOpenrouterSlug:
|
||||
def test_exact_match(self):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue