mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-24 05:41:40 +00:00
fix(dashboard): MiniMax 'Login' button launched Claude OAuth (#22832)
Fixes #22832. ## Root cause `hermes_cli/web_server.py:start_oauth_login` dispatched OAuth flows by the catalog's `flow` field rather than provider id: if catalog_entry["flow"] == "pkce": return _start_anthropic_pkce() The catalog had two `flow: "pkce"` entries — `anthropic` and `minimax-oauth` — so clicking "Login" on MiniMax in the dashboard's Keys tab unconditionally launched the Anthropic/Claude PKCE flow. ## Fix Three changes in `hermes_cli/web_server.py`: 1. Catalog entry for `minimax-oauth` changed from `flow: "pkce"` to `flow: "device_code"`. From a UX perspective MiniMax is a verification-URI + user-code flow (open URL, enter code, backend polls) — same shape as Nous's device-code flow. The PKCE bit (verifier + challenge from `_minimax_pkce_pair`) is a security extension that doesn't change the operator experience; the existing dashboard modal already renders `device_code` correctly for this UX. 2. New MiniMax branch in `_start_device_code_flow`, mirroring the existing Nous branch but calling MiniMax-specific helpers (`_minimax_request_user_code`, `_minimax_pkce_pair`). Stashes verifier + state in the session for the poller to consume. Handles the overloaded `expired_in` field (could be unix-ms timestamp OR seconds-from-now duration) the same way `_minimax_poll_token` does. 3. New `_minimax_poller` background thread mirroring `_nous_poller`. Calls `_minimax_poll_token` → on success builds the same `auth_state` dict the CLI flow (`_minimax_oauth_login`) builds, and persists via `_minimax_save_auth_state` so the dashboard path leaves the system in the same state as `hermes auth add minimax-oauth`. Plus a dispatcher tightening to prevent regression: the `pkce` branch now requires `provider_id == "anthropic"`, so any future PKCE provider added without a proper start function gets a clean `400 Unsupported flow` rather than silently launching Anthropic OAuth. ## Test New `tests/hermes_cli/test_web_oauth_dispatch.py`: - Regression test asserting MiniMax start does NOT return claude.ai - Sanity test that Anthropic PKCE still works after the dispatcher tightening - Forward-looking test: a hypothetical pkce-flagged provider without an explicit branch is rejected cleanly rather than misrouted ## Limitations - The dashboard MiniMax path defaults to `region="global"`. CN-region operators can still use the CLI flow which supports `--region cn`. Adding a region toggle to the dashboard UI is a follow-up.
This commit is contained in:
parent
ea1d0462cf
commit
05bad7b1e7
2 changed files with 299 additions and 3 deletions
|
|
@ -1456,7 +1456,12 @@ _OAUTH_PROVIDER_CATALOG: tuple[Dict[str, Any], ...] = (
|
||||||
{
|
{
|
||||||
"id": "minimax-oauth",
|
"id": "minimax-oauth",
|
||||||
"name": "MiniMax (OAuth)",
|
"name": "MiniMax (OAuth)",
|
||||||
"flow": "pkce",
|
# MiniMax's flow is structurally device-code (verification URI +
|
||||||
|
# user code, backend polls the token endpoint) with a PKCE
|
||||||
|
# extension for code-binding. The dashboard renders the same UX
|
||||||
|
# as Nous's device-code flow; the PKCE bit is a security
|
||||||
|
# extension that doesn't change the operator experience.
|
||||||
|
"flow": "device_code",
|
||||||
"cli_command": "hermes auth add minimax-oauth",
|
"cli_command": "hermes auth add minimax-oauth",
|
||||||
"docs_url": "https://www.minimax.io",
|
"docs_url": "https://www.minimax.io",
|
||||||
"status_fn": None, # dispatched via auth.get_minimax_oauth_auth_status
|
"status_fn": None, # dispatched via auth.get_minimax_oauth_auth_status
|
||||||
|
|
@ -1819,7 +1824,7 @@ def _submit_anthropic_pkce(session_id: str, code_input: str) -> Dict[str, Any]:
|
||||||
|
|
||||||
|
|
||||||
async def _start_device_code_flow(provider_id: str) -> Dict[str, Any]:
|
async def _start_device_code_flow(provider_id: str) -> Dict[str, Any]:
|
||||||
"""Initiate a device-code flow (Nous or OpenAI Codex).
|
"""Initiate a device-code flow (Nous, OpenAI Codex, or MiniMax).
|
||||||
|
|
||||||
Calls the provider's device-auth endpoint via the existing CLI helpers,
|
Calls the provider's device-auth endpoint via the existing CLI helpers,
|
||||||
then spawns a background poller. Returns the user-facing display fields
|
then spawns a background poller. Returns the user-facing display fields
|
||||||
|
|
@ -1898,6 +1903,82 @@ async def _start_device_code_flow(provider_id: str) -> Dict[str, Any]:
|
||||||
"poll_interval": int(s.get("interval") or 5),
|
"poll_interval": int(s.get("interval") or 5),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if provider_id == "minimax-oauth":
|
||||||
|
# MiniMax uses a device-code-style flow (verification URI + user
|
||||||
|
# code + background poll) with a PKCE extension on top. From the
|
||||||
|
# operator's perspective it's identical to Nous's device-code
|
||||||
|
# flow; the PKCE bit (verifier + challenge from
|
||||||
|
# _minimax_pkce_pair) is a security extension that binds the
|
||||||
|
# token exchange to the original session.
|
||||||
|
from hermes_cli.auth import (
|
||||||
|
_minimax_pkce_pair,
|
||||||
|
_minimax_request_user_code,
|
||||||
|
MINIMAX_OAUTH_CLIENT_ID,
|
||||||
|
MINIMAX_OAUTH_GLOBAL_BASE,
|
||||||
|
)
|
||||||
|
import httpx
|
||||||
|
verifier, challenge, state = _minimax_pkce_pair()
|
||||||
|
portal_base_url = (
|
||||||
|
os.getenv("MINIMAX_PORTAL_BASE_URL") or MINIMAX_OAUTH_GLOBAL_BASE
|
||||||
|
).rstrip("/")
|
||||||
|
def _do_minimax_request():
|
||||||
|
with httpx.Client(
|
||||||
|
timeout=httpx.Timeout(15.0),
|
||||||
|
headers={"Accept": "application/json"},
|
||||||
|
follow_redirects=True,
|
||||||
|
) as client:
|
||||||
|
return _minimax_request_user_code(
|
||||||
|
client=client,
|
||||||
|
portal_base_url=portal_base_url,
|
||||||
|
client_id=MINIMAX_OAUTH_CLIENT_ID,
|
||||||
|
code_challenge=challenge,
|
||||||
|
state=state,
|
||||||
|
)
|
||||||
|
device_data = await asyncio.get_event_loop().run_in_executor(
|
||||||
|
None, _do_minimax_request
|
||||||
|
)
|
||||||
|
sid, sess = _new_oauth_session("minimax-oauth", "device_code")
|
||||||
|
# The CLI flow names this `interval_ms` because MiniMax's
|
||||||
|
# `interval` field is in milliseconds (defensive default 2000ms
|
||||||
|
# in _minimax_poll_token).
|
||||||
|
interval_raw = device_data.get("interval")
|
||||||
|
sess["interval_ms"] = (
|
||||||
|
int(interval_raw) if interval_raw is not None else None
|
||||||
|
)
|
||||||
|
sess["user_code"] = str(device_data["user_code"])
|
||||||
|
sess["code_verifier"] = verifier
|
||||||
|
sess["state"] = state
|
||||||
|
sess["portal_base_url"] = portal_base_url
|
||||||
|
sess["client_id"] = MINIMAX_OAUTH_CLIENT_ID
|
||||||
|
sess["region"] = "global"
|
||||||
|
# `expired_in` from MiniMax is overloaded — could be a unix-ms
|
||||||
|
# timestamp OR a seconds-from-now duration. Mirror the heuristic
|
||||||
|
# in _minimax_poll_token. Stash the raw value for the poller;
|
||||||
|
# compute a derived expires_at + UI-friendly expires_in seconds.
|
||||||
|
expired_in_raw = int(device_data["expired_in"])
|
||||||
|
sess["expired_in_raw"] = expired_in_raw
|
||||||
|
if expired_in_raw > 1_000_000_000_000: # likely unix-ms
|
||||||
|
expires_at_ts = expired_in_raw / 1000.0
|
||||||
|
expires_in_seconds = max(0, int(expires_at_ts - time.time()))
|
||||||
|
else:
|
||||||
|
expires_at_ts = time.time() + expired_in_raw
|
||||||
|
expires_in_seconds = expired_in_raw
|
||||||
|
sess["expires_at"] = expires_at_ts
|
||||||
|
threading.Thread(
|
||||||
|
target=_minimax_poller,
|
||||||
|
args=(sid,),
|
||||||
|
daemon=True,
|
||||||
|
name=f"oauth-poll-{sid[:6]}",
|
||||||
|
).start()
|
||||||
|
return {
|
||||||
|
"session_id": sid,
|
||||||
|
"flow": "device_code",
|
||||||
|
"user_code": str(device_data["user_code"]),
|
||||||
|
"verification_url": str(device_data["verification_uri"]),
|
||||||
|
"expires_in": expires_in_seconds,
|
||||||
|
"poll_interval": max(2, (sess["interval_ms"] or 2000) // 1000),
|
||||||
|
}
|
||||||
|
|
||||||
raise HTTPException(status_code=400, detail=f"Provider {provider_id} does not support device-code flow")
|
raise HTTPException(status_code=400, detail=f"Provider {provider_id} does not support device-code flow")
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -1959,6 +2040,86 @@ def _nous_poller(session_id: str) -> None:
|
||||||
sess["error_message"] = str(e)
|
sess["error_message"] = str(e)
|
||||||
|
|
||||||
|
|
||||||
|
def _minimax_poller(session_id: str) -> None:
|
||||||
|
"""Background poller that drives a MiniMax OAuth flow to completion.
|
||||||
|
|
||||||
|
Mirrors `_nous_poller` but calls the MiniMax-specific token endpoint,
|
||||||
|
which uses a PKCE-style ``code_verifier`` + ``user_code`` rather than
|
||||||
|
the ``device_code`` field used by Nous. On success, builds the same
|
||||||
|
auth_state dict that ``_minimax_oauth_login`` (the CLI flow) builds
|
||||||
|
and persists via ``_minimax_save_auth_state`` — so the dashboard
|
||||||
|
path leaves the system in the same state as
|
||||||
|
``hermes auth add minimax-oauth``.
|
||||||
|
"""
|
||||||
|
from hermes_cli.auth import (
|
||||||
|
_minimax_poll_token,
|
||||||
|
_minimax_save_auth_state,
|
||||||
|
MINIMAX_OAUTH_GLOBAL_INFERENCE,
|
||||||
|
MINIMAX_OAUTH_SCOPE,
|
||||||
|
)
|
||||||
|
from datetime import datetime, timezone
|
||||||
|
import httpx
|
||||||
|
with _oauth_sessions_lock:
|
||||||
|
sess = _oauth_sessions.get(session_id)
|
||||||
|
if not sess:
|
||||||
|
return
|
||||||
|
portal_base_url = sess["portal_base_url"]
|
||||||
|
client_id = sess["client_id"]
|
||||||
|
user_code = sess["user_code"]
|
||||||
|
code_verifier = sess["code_verifier"]
|
||||||
|
interval_ms = sess.get("interval_ms")
|
||||||
|
expired_in_raw = sess["expired_in_raw"]
|
||||||
|
try:
|
||||||
|
with httpx.Client(
|
||||||
|
timeout=httpx.Timeout(15.0),
|
||||||
|
headers={"Accept": "application/json"},
|
||||||
|
follow_redirects=True,
|
||||||
|
) as client:
|
||||||
|
token_data = _minimax_poll_token(
|
||||||
|
client=client,
|
||||||
|
portal_base_url=portal_base_url,
|
||||||
|
client_id=client_id,
|
||||||
|
user_code=user_code,
|
||||||
|
code_verifier=code_verifier,
|
||||||
|
expired_in=expired_in_raw,
|
||||||
|
interval_ms=interval_ms,
|
||||||
|
)
|
||||||
|
# Build the auth_state dict in the same shape as the CLI flow's
|
||||||
|
# `_minimax_oauth_login` so `_minimax_save_auth_state` writes
|
||||||
|
# the canonical record. Region is fixed to "global" for the
|
||||||
|
# dashboard path; cn-region operators can still use the CLI
|
||||||
|
# flow which supports `--region cn`.
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
expires_in_s = int(token_data["expired_in"])
|
||||||
|
expires_at_ts = now.timestamp() + expires_in_s
|
||||||
|
auth_state = {
|
||||||
|
"provider": "minimax-oauth",
|
||||||
|
"region": sess.get("region", "global"),
|
||||||
|
"portal_base_url": portal_base_url,
|
||||||
|
"inference_base_url": MINIMAX_OAUTH_GLOBAL_INFERENCE,
|
||||||
|
"client_id": client_id,
|
||||||
|
"scope": MINIMAX_OAUTH_SCOPE,
|
||||||
|
"token_type": token_data.get("token_type", "Bearer"),
|
||||||
|
"access_token": token_data["access_token"],
|
||||||
|
"refresh_token": token_data["refresh_token"],
|
||||||
|
"resource_url": token_data.get("resource_url"),
|
||||||
|
"obtained_at": now.isoformat(),
|
||||||
|
"expires_at": datetime.fromtimestamp(
|
||||||
|
expires_at_ts, tz=timezone.utc
|
||||||
|
).isoformat(),
|
||||||
|
"expires_in": expires_in_s,
|
||||||
|
}
|
||||||
|
_minimax_save_auth_state(auth_state)
|
||||||
|
with _oauth_sessions_lock:
|
||||||
|
sess["status"] = "approved"
|
||||||
|
_log.info("oauth/device: minimax login completed (session=%s)", session_id)
|
||||||
|
except Exception as e:
|
||||||
|
_log.warning("minimax device-code poll failed (session=%s): %s", session_id, e)
|
||||||
|
with _oauth_sessions_lock:
|
||||||
|
sess["status"] = "error"
|
||||||
|
sess["error_message"] = str(e)
|
||||||
|
|
||||||
|
|
||||||
def _codex_full_login_worker(session_id: str) -> None:
|
def _codex_full_login_worker(session_id: str) -> None:
|
||||||
"""Run the complete OpenAI Codex device-code flow.
|
"""Run the complete OpenAI Codex device-code flow.
|
||||||
|
|
||||||
|
|
@ -2111,7 +2272,13 @@ async def start_oauth_login(provider_id: str, request: Request):
|
||||||
detail=f"{provider_id} uses an external CLI; run `{catalog_entry['cli_command']}` manually",
|
detail=f"{provider_id} uses an external CLI; run `{catalog_entry['cli_command']}` manually",
|
||||||
)
|
)
|
||||||
try:
|
try:
|
||||||
if catalog_entry["flow"] == "pkce":
|
# The pkce branch is gated on provider_id == "anthropic" because
|
||||||
|
# `_start_anthropic_pkce()` is hardcoded to the Anthropic flow.
|
||||||
|
# Routing any other future pkce-flagged provider through it would
|
||||||
|
# silently launch the Anthropic OAuth flow (the bug fixed in this
|
||||||
|
# change for MiniMax). New PKCE providers must add their own
|
||||||
|
# start function and an explicit branch here.
|
||||||
|
if catalog_entry["flow"] == "pkce" and provider_id == "anthropic":
|
||||||
return _start_anthropic_pkce()
|
return _start_anthropic_pkce()
|
||||||
if catalog_entry["flow"] == "device_code":
|
if catalog_entry["flow"] == "device_code":
|
||||||
return await _start_device_code_flow(provider_id)
|
return await _start_device_code_flow(provider_id)
|
||||||
|
|
|
||||||
129
tests/hermes_cli/test_web_oauth_dispatch.py
Normal file
129
tests/hermes_cli/test_web_oauth_dispatch.py
Normal file
|
|
@ -0,0 +1,129 @@
|
||||||
|
"""Regression tests for the OAuth dispatcher in hermes_cli.web_server.
|
||||||
|
|
||||||
|
Bug history (2026-05-09): the `_OAUTH_PROVIDER_CATALOG` had two entries
|
||||||
|
flagged ``flow: "pkce"`` — anthropic and minimax-oauth — and the
|
||||||
|
dispatcher ``start_oauth_login`` hardcoded ``_start_anthropic_pkce()``
|
||||||
|
for any pkce-flagged provider. So clicking "Login" next to MiniMax in
|
||||||
|
the dashboard's Keys tab silently launched the Anthropic/Claude OAuth
|
||||||
|
flow.
|
||||||
|
|
||||||
|
The fix:
|
||||||
|
1. Catalog entry for minimax-oauth changed from ``flow: "pkce"`` to
|
||||||
|
``flow: "device_code"`` (the actual UX is verification URI + user
|
||||||
|
code + background poll, with PKCE as a security extension).
|
||||||
|
2. New MiniMax branch added to ``_start_device_code_flow``.
|
||||||
|
3. Dispatcher tightened: pkce branch now requires
|
||||||
|
``provider_id == "anthropic"``, so any future PKCE provider added
|
||||||
|
without an explicit branch gets a clean ``400 Unsupported flow``
|
||||||
|
instead of silently launching Anthropic OAuth.
|
||||||
|
|
||||||
|
These tests pin the corrected behavior.
|
||||||
|
"""
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from fastapi.testclient import TestClient
|
||||||
|
|
||||||
|
from hermes_cli.web_server import _SESSION_TOKEN, app
|
||||||
|
|
||||||
|
client = TestClient(app)
|
||||||
|
HEADERS = {"X-Hermes-Session-Token": _SESSION_TOKEN}
|
||||||
|
|
||||||
|
|
||||||
|
def test_minimax_login_does_not_launch_anthropic_flow():
|
||||||
|
"""Click 'Login' on MiniMax → MUST NOT return claude.ai auth_url."""
|
||||||
|
fake_user_code_resp = {
|
||||||
|
"user_code": "ABCD-1234",
|
||||||
|
"verification_uri": "https://api.minimax.io/oauth/verify",
|
||||||
|
# `expired_in` < 1e12 so the heuristic treats it as seconds.
|
||||||
|
"expired_in": 600,
|
||||||
|
"interval": 2000,
|
||||||
|
"state": "stub-state",
|
||||||
|
}
|
||||||
|
with patch(
|
||||||
|
"hermes_cli.auth._minimax_request_user_code",
|
||||||
|
return_value=fake_user_code_resp,
|
||||||
|
), patch(
|
||||||
|
"hermes_cli.auth._minimax_pkce_pair",
|
||||||
|
return_value=("verifier-stub", "challenge-stub", "stub-state"),
|
||||||
|
):
|
||||||
|
resp = client.post(
|
||||||
|
"/api/providers/oauth/minimax-oauth/start",
|
||||||
|
headers=HEADERS,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
body = resp.json()
|
||||||
|
|
||||||
|
# The bug used to return Anthropic's auth_url — make sure the response
|
||||||
|
# references neither the auth_url field nor anything Claude-related.
|
||||||
|
assert "auth_url" not in body
|
||||||
|
assert "claude.ai" not in str(body).lower()
|
||||||
|
|
||||||
|
# And the response IS the device-code shape pointing at MiniMax.
|
||||||
|
assert body["flow"] == "device_code"
|
||||||
|
assert "minimax" in body["verification_url"].lower()
|
||||||
|
assert body["user_code"] == "ABCD-1234"
|
||||||
|
assert body["expires_in"] == 600
|
||||||
|
|
||||||
|
|
||||||
|
def test_anthropic_pkce_branch_still_works():
|
||||||
|
"""Sanity: the dispatcher tightening doesn't break the legitimate Anthropic PKCE path."""
|
||||||
|
fake_anthropic_response = {
|
||||||
|
"session_id": "stub-session",
|
||||||
|
"flow": "pkce",
|
||||||
|
"auth_url": "https://claude.ai/oauth/authorize?code=true&...",
|
||||||
|
"expires_in": 600,
|
||||||
|
}
|
||||||
|
with patch(
|
||||||
|
"hermes_cli.web_server._start_anthropic_pkce",
|
||||||
|
return_value=fake_anthropic_response,
|
||||||
|
):
|
||||||
|
resp = client.post(
|
||||||
|
"/api/providers/oauth/anthropic/start",
|
||||||
|
headers=HEADERS,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
body = resp.json()
|
||||||
|
assert body["flow"] == "pkce"
|
||||||
|
assert "claude.ai" in body["auth_url"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_unknown_pkce_provider_rejected_cleanly():
|
||||||
|
"""A future PKCE provider without an explicit branch must NOT silently route to Anthropic.
|
||||||
|
|
||||||
|
Simulates a hypothetical catalog entry with ``flow: "pkce"`` and an
|
||||||
|
id other than "anthropic". The dispatcher should fall through past
|
||||||
|
the pkce branch (now gated on provider_id) and the device_code
|
||||||
|
branch, then hit "Unsupported flow" — proving the bug class is
|
||||||
|
structurally prevented.
|
||||||
|
"""
|
||||||
|
from hermes_cli import web_server as ws
|
||||||
|
|
||||||
|
# Inject a hypothetical catalog entry that's pkce-flagged but isn't
|
||||||
|
# anthropic. This shape mirrors what would happen if a developer
|
||||||
|
# added a new provider entry without remembering to wire up its
|
||||||
|
# start function.
|
||||||
|
fake_entry = {
|
||||||
|
"id": "hypothetical-pkce-provider",
|
||||||
|
"name": "Hypothetical PKCE Provider",
|
||||||
|
"flow": "pkce",
|
||||||
|
"cli_command": "hermes auth add hypothetical-pkce-provider",
|
||||||
|
"docs_url": "https://example.com",
|
||||||
|
"status_fn": None,
|
||||||
|
}
|
||||||
|
original_catalog = ws._OAUTH_PROVIDER_CATALOG
|
||||||
|
try:
|
||||||
|
ws._OAUTH_PROVIDER_CATALOG = original_catalog + (fake_entry,)
|
||||||
|
resp = client.post(
|
||||||
|
"/api/providers/oauth/hypothetical-pkce-provider/start",
|
||||||
|
headers=HEADERS,
|
||||||
|
)
|
||||||
|
finally:
|
||||||
|
ws._OAUTH_PROVIDER_CATALOG = original_catalog
|
||||||
|
|
||||||
|
# Either 400 "Unsupported flow" (the explicit fall-through) or any
|
||||||
|
# 4xx — what we MUST NOT see is a 200 with claude.ai in the body.
|
||||||
|
assert resp.status_code >= 400, resp.text
|
||||||
|
assert "claude.ai" not in resp.text.lower()
|
||||||
Loading…
Add table
Add a link
Reference in a new issue