Skip to content

fix(acp): validate method_id in authenticate against advertised provider (#9438) - #9470

Closed
LarHope wants to merge 1 commit into
NousResearch:mainfrom
LarHope:fix/acp-authenticate-method-id-9438
Closed

fix(acp): validate method_id in authenticate against advertised provider (#9438)#9470
LarHope wants to merge 1 commit into
NousResearch:mainfrom
LarHope:fix/acp-authenticate-method-id-9438

Conversation

@LarHope

@LarHope LarHope commented Apr 14, 2026

Copy link
Copy Markdown

Closes #9438

What does this PR do?

HermesACPAgent.authenticate() ignored the method_id argument — it returned AuthenticateResponse() for any requested auth method as long as some provider was configured. Meanwhile initialize() advertises exactly one AuthMethodAgent whose id is the resolved provider. This PR makes authenticate() match method_id against that same advertised id, so stale/unknown method ids are rejected.

To prevent drift between the id initialize() publishes and the one authenticate() compares against, both paths now route the provider spelling through a shared _canonical_provider_id() helper (strip + lowercase + None-guard). Previously authenticate() implicitly depended on detect_provider() always returning canonical casing — a silent cross-module invariant flagged during review.

Related Issue

Fixes #9438

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix (defense-in-depth for ACP auth)

Changes Made

  • acp_adapter/server.py — add _canonical_provider_id() helper; initialize() canonicalizes the advertised id; authenticate() returns None when no provider is configured, method_id is not a non-empty string, or the canonicalized method_id does not equal the advertised provider (logged at INFO).
  • tests/acp/test_server.py — replace the has_provider monkeypatch with detect_provider (which authenticate() now uses directly) and add regressions: unknown method_id rejection, case-insensitive match, non-string method_id, and initialize/authenticate canonicalization agreement.

Diff: 2 files, +71/-9.

How to Test

Reproduction from the issue:

```python
import asyncio
from unittest.mock import MagicMock
from acp_adapter.server import HermesACPAgent
from acp_adapter.session import SessionManager
import acp_adapter.server as server_mod

async def main():
agent = HermesACPAgent(session_manager=SessionManager(agent_factory=lambda: MagicMock()))
server_mod.detect_provider = lambda: 'openrouter'
print('matching:', (await agent.authenticate(method_id='openrouter')) is not None)
print('bogus: ', (await agent.authenticate(method_id='definitely-not-advertised')) is not None)

asyncio.run(main())
```

Before this PR:
```
matching: True
bogus: True
```

After:
```
matching: True
bogus: False
```

Run the ACP server test module:

```bash
pytest tests/acp/test_server.py -v
```

57 tests pass (52 existing + 5 updated/new around authenticate).

Checklist

Code

  • I've read the Contributing Guide
  • Commit message follows Conventional Commits
  • Searched for existing PRs — none touching acp_adapter/server.py auth
  • PR contains only changes related to this security fix
  • Ran pytest tests/acp/test_server.py -v — all pass
  • Added regression tests for the reported bug + related edge cases
  • Tested on Ubuntu 24.04 (Python 3.11)

Documentation & Housekeeping

  • No doc changes needed — N/A
  • No config keys changed — N/A
  • No architecture/workflow changes — N/A
  • Pure-Python string/type comparison, no cross-platform concerns — N/A
  • No tool behavior changes — N/A

Follow-up (not in this PR)

A follow-up could cache the advertised method_id on the connection/session at initialize() time instead of recomputing detect_provider() at authenticate() time. That would also close a latent TOCTOU between initialize and authenticate (provider credentials changing mid-handshake). Happy to open a separate PR if maintainers want that — keeping this one minimal and focused on the reported issue.

…der (NousResearch#9438)

HermesACPAgent.authenticate() previously accepted any method_id as long
as some provider was resolvable, so a client could send an arbitrary or
stale id and still be authenticated. initialize() advertises exactly
one AuthMethodAgent whose id is the resolved provider, so authenticate()
must now match against that same id.

Changes:
- Add a module-level _canonical_provider_id() helper that strips +
  lowercases both sides of the comparison, so initialize() and
  authenticate() route the provider spelling through the same path.
- authenticate() returns None when no provider is configured, when
  method_id is not a non-empty string, or when the canonicalized
  method_id does not match the advertised provider.
- Replace the has_provider() monkeypatch in existing tests with a
  detect_provider() monkeypatch and add regressions for:
  unknown method_id rejection (the reported bug), case-insensitive
  matching, non-string method_id, and advertised/authenticate
  canonicalization agreement.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter labels Apr 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #13468 (merged) — same fix: validate method_id against advertised provider in ACP authenticate(). Also related to open issue #13452.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the contribution, @LarHope! This fix has already landed on main via #13468 (merged 2026-04-21) — a maintainer-merged PR with the same change: authenticate() now validates method_id against the advertised provider using a case-insensitive comparison and returns None on mismatch.

This was also flagged by @alt-glitch in the review thread above.

Evidence:

Closing as duplicate — the fix is in. Thanks again for the report and clean reproduction!

Automated review by hermes-sweeper.

@teknium1 teknium1 closed this Jun 10, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP authenticate() accepts any method_id when any provider is configured

3 participants