From 774d92fbfadb8acbadcb94fa894b30bd9122fd13 Mon Sep 17 00:00:00 2001 From: Flownium <157689911+itsflownium@users.noreply.github.com> Date: Mon, 15 Jun 2026 20:19:53 +1000 Subject: [PATCH 1/4] fix: approve listed pairing requests --- gateway/pairing.py | 69 +++++++++++++++---- hermes_cli/pairing.py | 22 +++--- hermes_cli/web_models.py | 3 +- hermes_cli/web_server.py | 18 +++-- tests/gateway/test_pairing.py | 18 +++++ .../test_dashboard_admin_endpoints.py | 20 ++++++ web/src/lib/api.ts | 6 +- web/src/pages/PairingPage.tsx | 20 ++++-- 8 files changed, 138 insertions(+), 38 deletions(-) diff --git a/gateway/pairing.py b/gateway/pairing.py index bf6212f97f3..f5dd131721a 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -412,6 +412,22 @@ class PairingStore: """Hash a pairing code with the given salt using SHA-256.""" return hashlib.sha256(salt + code.encode("utf-8")).hexdigest() + def _finish_approval( + self, platform: str, pending: dict, matched_key: str, matched_entry: dict + ) -> dict: + """Remove a pending request and approve its user. Must hold self._lock.""" + del pending[matched_key] + self._save_json(self._pending_path(platform), pending) + + self._approve_user( + platform, matched_entry["user_id"], matched_entry.get("user_name", "") + ) + + return { + "user_id": matched_entry["user_id"], + "user_name": matched_entry.get("user_name", ""), + } + def generate_code( self, platform: str, user_id: str, user_name: str = "" ) -> Optional[str]: @@ -524,26 +540,44 @@ class PairingStore: self._record_failed_attempt(platform) return None - del pending[matched_key] - self._save_json(self._pending_path(platform), pending) + return self._finish_approval(platform, pending, matched_key, matched_entry) - # Add to approved list - self._approve_user(platform, matched_entry["user_id"], - matched_entry.get("user_name", "")) + def approve_request(self, platform: str, request_id: str) -> Optional[dict]: + """ + Approve a pending pairing request by its server-side request id. - return { - "user_id": matched_entry["user_id"], - "user_name": matched_entry.get("user_name", ""), - } + This is for admin surfaces (`pairing list`, dashboard approve buttons) + that show pending requests but must not reveal the one-time code sent + to the user. Returns ``{user_id, user_name}`` on success and ``None`` + for invalid/expired requests or platform lockout. + """ + with self._lock: + self._cleanup_expired(platform) + request_id = str(request_id or "").strip().lower() + + if self._is_locked_out(platform): + return None + + pending = self._load_json(self._pending_path(platform)) + for entry_id, entry in pending.items(): + if not isinstance(entry, dict): + continue + if "salt" not in entry or "hash" not in entry: + continue + if secrets.compare_digest(str(entry_id).lower(), request_id): + return self._finish_approval(platform, pending, entry_id, entry) + + self._record_failed_attempt(platform) + return None def list_pending(self, platform: str = None) -> list: """List pending pairing requests, optionally filtered by platform. - Codes are stored hashed — the ``code`` field is replaced with the - first 8 hex characters of the hash so admins can distinguish entries - without revealing the original code. Legacy plaintext-key entries - (pre-hash format) are shown with a "legacy" placeholder so admins - can see them age out without crashing on a missing ``hash`` field. + Codes are stored hashed and are never returned. Modern entries expose a + server-side ``request_id`` that an admin can pass to approve the + listed request directly. ``code`` remains as a backward-compatible alias + for ``request_id`` for older dashboard clients. ``code_hash_prefix`` is + diagnostic-only and is not an approvable code. """ results = [] with self._lock: @@ -559,10 +593,15 @@ class PairingStore: continue age_min = int((time.time() - created_at) / 60) hash_val = info.get("hash") + salt_val = info.get("salt") + is_modern = isinstance(hash_val, str) and isinstance(salt_val, str) + request_id = str(entry_id) if is_modern else "" code_display = hash_val[:8] if isinstance(hash_val, str) else "legacy" results.append({ "platform": p, - "code": code_display, + "request_id": request_id, + "code": request_id, + "code_hash_prefix": code_display, "user_id": info.get("user_id", ""), "user_name": info.get("user_name", ""), "age_minutes": age_min, diff --git a/hermes_cli/pairing.py b/hermes_cli/pairing.py index 101a1d10bc7..3982540006b 100644 --- a/hermes_cli/pairing.py +++ b/hermes_cli/pairing.py @@ -3,7 +3,7 @@ CLI commands for the DM pairing system. Usage: hermes pairing list # Show all pending + approved users - hermes pairing approve # Approve a pairing code + hermes pairing approve # Approve a pairing request hermes pairing revoke # Revoke user access hermes pairing clear-pending # Clear all expired/pending codes """ @@ -39,13 +39,16 @@ def _cmd_list(store): if pending: print(f"\n Pending Pairing Requests ({len(pending)}):") - print(f" {'Platform':<12} {'Code':<10} {'User ID':<20} {'Name':<20} {'Age'}") - print(f" {'--------':<12} {'----':<10} {'-------':<20} {'----':<20} {'---'}") + print(f" {'Platform':<12} {'Request ID':<18} {'User ID':<20} {'Name':<20} {'Age'}") + print(f" {'--------':<12} {'----------':<18} {'-------':<20} {'----':<20} {'---'}") for p in pending: + request_id = p.get("request_id") or p.get("code") or "" print( - f" {p['platform']:<12} {p['code']:<10} {p['user_id']:<20} " + f" {p['platform']:<12} {request_id:<18} {p['user_id']:<20} " f"{(p.get('user_name') or ''):<20} {p['age_minutes']}m ago" ) + print("\n Approve with: hermes pairing approve ") + print(" The bot-delivered code also still works if the user shares it.") else: print("\n No pending pairing requests.") @@ -62,11 +65,12 @@ def _cmd_list(store): def _cmd_approve(store, platform: str, code: str): - """Approve a pairing code.""" + """Approve a pairing request id or pairing code.""" platform = platform.lower().strip() - code = code.upper().strip() + code = code.strip() + is_request_id = len(code) == 16 and all(c in "0123456789abcdefABCDEF" for c in code) - result = store.approve_code(platform, code) + result = store.approve_request(platform, code) if is_request_id else store.approve_code(platform, code) if result: uid = result["user_id"] name = result.get("user_name") or "" @@ -92,8 +96,8 @@ def _cmd_approve(store, platform: str, code: str): "~/.hermes/platforms/pairing/_rate_limits.json\n".format(platform) ) else: - print(f"\n Code '{code}' not found or expired for platform '{platform}'.") - print(" Run 'hermes pairing list' to see pending codes.\n") + print(f"\n Pairing request or code '{code}' not found or expired for platform '{platform}'.") + print(" Run 'hermes pairing list' to see pending requests.\n") def _cmd_revoke(store, platform: str, user_id: str): diff --git a/hermes_cli/web_models.py b/hermes_cli/web_models.py index ce89433cbef..ad24fca6b84 100644 --- a/hermes_cli/web_models.py +++ b/hermes_cli/web_models.py @@ -428,7 +428,8 @@ class MCPCatalogInstall(BaseModel): class PairingApprove(BaseModel): platform: str - code: str + code: str = "" + request_id: str = "" class PairingRevoke(BaseModel): diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index f5549c0ec19..16295e40cc7 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -13021,11 +13021,19 @@ async def list_pairing(): async def approve_pairing(body: PairingApprove): store = _pairing_store() platform = (body.platform or "").lower().strip() - code = (body.code or "").upper().strip() - if not platform or not code: - raise HTTPException(status_code=400, detail="platform and code are required") + code = (body.code or "").strip() + request_id = (body.request_id or "").strip() + if not platform or not (request_id or code): + raise HTTPException(status_code=400, detail="platform and request_id or code are required") - result = store.approve_code(platform, code) + is_request_id = len(code) == 16 and all(c in "0123456789abcdefABCDEF" for c in code) + result = ( + store.approve_request(platform, request_id) + if request_id + else store.approve_request(platform, code) + if is_request_id + else store.approve_code(platform, code) + ) if result: return {"ok": True, "user": result} if store._is_locked_out(platform): @@ -13035,7 +13043,7 @@ async def approve_pairing(body: PairingApprove): ) raise HTTPException( status_code=404, - detail=f"Code '{code}' not found or expired for platform '{platform}'.", + detail=f"Pairing request or code not found or expired for platform '{platform}'.", ) diff --git a/tests/gateway/test_pairing.py b/tests/gateway/test_pairing.py index bfe6d714d46..6ecbc35b6a9 100644 --- a/tests/gateway/test_pairing.py +++ b/tests/gateway/test_pairing.py @@ -247,6 +247,24 @@ class TestApprovalFlow: store.approve_code("telegram", code) assert store.is_approved("telegram", "user1") is True + def test_approve_request_id_from_pending_list(self, tmp_path): + with patch("gateway.pairing.PAIRING_DIR", tmp_path): + store = PairingStore() + bot_code = store.generate_code("telegram", "user1", "Alice") + pending = store.list_pending("telegram") + request_id = pending[0]["request_id"] + + assert request_id + assert request_id != bot_code + + result = store.approve_request("telegram", request_id.upper()) + remaining = store.list_pending("telegram") + + assert isinstance(result, dict) + assert result["user_id"] == "user1" + assert result["user_name"] == "Alice" + assert remaining == [] + def test_whatsapp_legacy_raw_jid_approval_survives_alias_flip(self, tmp_path, monkeypatch): mapping_dir = tmp_path / "whatsapp" / "session" diff --git a/tests/hermes_cli/test_dashboard_admin_endpoints.py b/tests/hermes_cli/test_dashboard_admin_endpoints.py index 876c63c151a..f155d58f28d 100644 --- a/tests/hermes_cli/test_dashboard_admin_endpoints.py +++ b/tests/hermes_cli/test_dashboard_admin_endpoints.py @@ -253,6 +253,26 @@ class TestPairingEndpoints: def _setup(self, _isolate_hermes_home): self.client, _ = _client() + def test_approve_pending_request_id(self): + from gateway.pairing import PairingStore + + store = PairingStore() + bot_code = store.generate_code("telegram", "user1", "Alice") + data = self.client.get("/api/pairing").json() + request_id = data["pending"][0]["request_id"] + + assert request_id + assert request_id != bot_code + + r = self.client.post( + "/api/pairing/approve", + json={"platform": "telegram", "request_id": request_id}, + ) + + assert r.status_code == 200 + assert r.json()["user"]["user_id"] == "user1" + assert self.client.get("/api/pairing").json()["pending"] == [] + class TestWebhookEndpoints: @pytest.fixture(autouse=True) diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 0d83cdefda0..097cc98382d 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -1091,11 +1091,11 @@ export const api = { // ── Admin: Pairing ────────────────────────────────────────────────── getPairing: () => fetchJSON("/api/pairing"), - approvePairing: (platform: string, code: string) => + approvePairing: (platform: string, request_id: string) => fetchJSON<{ ok: boolean; user: PairingUser }>("/api/pairing/approve", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ platform, code }), + body: JSON.stringify({ platform, request_id }), }), revokePairing: (platform: string, user_id: string) => fetchJSON<{ ok: boolean }>("/api/pairing/revoke", { @@ -1587,7 +1587,9 @@ export interface PairingUser { platform: string; user_id: string; user_name?: string; + request_id?: string; code?: string; + code_hash_prefix?: string; age_minutes?: number; } diff --git a/web/src/pages/PairingPage.tsx b/web/src/pages/PairingPage.tsx index 7f7a8c68b87..df46f0f124c 100644 --- a/web/src/pages/PairingPage.tsx +++ b/web/src/pages/PairingPage.tsx @@ -52,14 +52,15 @@ export default function PairingPage() { }, [loadPairing]); const handleApprove = async (user: PairingUser) => { - if (!user.code) { - showToast("Missing pairing code", "error"); + const requestId = user.request_id || user.code; + if (!requestId) { + showToast("Missing pairing request", "error"); return; } const key = getUserKey(user); setApproving(key); try { - await api.approvePairing(user.platform, user.code); + await api.approvePairing(user.platform, requestId); showToast(`Approved: "${getUserLabel(user)}"`, "success"); loadPairing(); } catch (e) { @@ -179,8 +180,15 @@ export default function PairingPage() {
{user.platform} - {user.code && ( - {user.code} + {(user.request_id || user.code) && ( + + {user.request_id || user.code} + + )} + {user.code_hash_prefix && ( + + hash {user.code_hash_prefix} + )}
@@ -199,7 +207,7 @@ export default function PairingPage() { size="sm" className="uppercase" onClick={() => handleApprove(user)} - disabled={approving === key || !user.code} + disabled={approving === key || !(user.request_id || user.code)} prefix={ approving === key ? ( From 57ad22262060bf8b8553ca13fcbf05b26709fab0 Mon Sep 17 00:00:00 2001 From: Flownium <157689911+itsflownium@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:22:59 +1000 Subject: [PATCH 2/4] test: cover pairing CLI approval paths --- tests/hermes_cli/test_pairing.py | 43 ++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tests/hermes_cli/test_pairing.py diff --git a/tests/hermes_cli/test_pairing.py b/tests/hermes_cli/test_pairing.py new file mode 100644 index 00000000000..be8b6f847f1 --- /dev/null +++ b/tests/hermes_cli/test_pairing.py @@ -0,0 +1,43 @@ +from argparse import Namespace +from unittest.mock import patch + +from gateway.pairing import PairingStore +from hermes_cli.pairing import pairing_command + + +def test_cli_listed_request_id_and_bot_code_can_be_approved(tmp_path, capsys): + with patch("gateway.pairing.PAIRING_DIR", tmp_path): + store = PairingStore() + store.generate_code("telegram", "listed-user", "Listed User") + + with patch("gateway.pairing.PairingStore", return_value=store): + pairing_command(Namespace(pairing_action="list")) + list_output = capsys.readouterr().out + request_id = store.list_pending("telegram")[0]["request_id"] + + assert request_id in list_output + + pairing_command( + Namespace( + pairing_action="approve", + platform="telegram", + code=request_id, + ) + ) + request_approval_output = capsys.readouterr().out + + bot_code = store.generate_code("telegram", "code-user", "Code User") + pairing_command( + Namespace( + pairing_action="approve", + platform="telegram", + code=bot_code, + ) + ) + code_approval_output = capsys.readouterr().out + + approved_ids = {entry["user_id"] for entry in store.list_approved("telegram")} + + assert "listed-user" in request_approval_output + assert "code-user" in code_approval_output + assert approved_ids == {"listed-user", "code-user"} From c352a322b081c7074fb99737b0c596c48e24fe83 Mon Sep 17 00:00:00 2001 From: solyanviktor-star <233359899+solyanviktor-star@users.noreply.github.com> Date: Fri, 10 Jul 2026 18:18:34 +0300 Subject: [PATCH 3/4] fix(pairing): reset the failed-approval counter on a successful approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit approve_code()'s success path never cleared _failures:{platform}. The counter is incremented on every non-matching code, persisted in _rate_limits.json, and only ever reset to 0 when it reaches MAX_FAILED_ATTEMPTS (firing the lockout). So it counts failures over the gateway's entire lifetime, not consecutive ones. An owner who mistypes a pairing code on a handful of separate occasions — each time immediately retyping it correctly and successfully pairing — accumulates those isolated typos. A later single fresh typo then hits MAX_FAILED_ATTEMPTS and locks the whole platform out for an hour, at which point _is_locked_out gates approve_code and even the *correct* code is rejected. Reset the counter on a successful approval, matching standard brute-force-guard semantics (the counter tracks consecutive failures). This does not weaken protection: an attacker cannot produce a success without a valid code, and 5 consecutive wrong attempts still lock out. Co-Authored-By: Claude Fable 5 --- gateway/pairing.py | 21 +++++++++++++++++++++ tests/gateway/test_pairing.py | 24 ++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/gateway/pairing.py b/gateway/pairing.py index f5dd131721a..5a1388f4760 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -419,6 +419,14 @@ class PairingStore: del pending[matched_key] self._save_json(self._pending_path(platform), pending) + # A successful approval proves the requester is legitimate, so the + # brute-force failure streak must not carry over. Without this, + # isolated mistyped codes accumulate across the gateway's lifetime + # (the counter is persisted in _rate_limits.json and only ever + # reset when a lockout fires) and eventually trip a spurious + # lockout on a single fresh typo — rejecting even a valid code. + self._reset_failed_attempts(platform) + self._approve_user( platform, matched_entry["user_id"], matched_entry.get("user_name", "") ) @@ -661,6 +669,19 @@ class PairingStore: f"after {MAX_FAILED_ATTEMPTS} failed attempts", flush=True) self._save_json(self._rate_limit_path(), limits) + def _reset_failed_attempts(self, platform: str) -> None: + """Clear the accumulated failed-approval counter after a success. + + Called from the ``approve_code`` success path so that a legitimate + approval resets the brute-force streak (standard lockout semantics: + the counter tracks *consecutive* failures, not lifetime ones). + """ + limits = self._load_json(self._rate_limit_path()) + fail_key = f"_failures:{platform}" + if limits.get(fail_key): + limits[fail_key] = 0 + self._save_json(self._rate_limit_path(), limits) + # ----- Cleanup ----- def _cleanup_expired(self, platform: str) -> None: diff --git a/tests/gateway/test_pairing.py b/tests/gateway/test_pairing.py index 6ecbc35b6a9..8a46ca7aa43 100644 --- a/tests/gateway/test_pairing.py +++ b/tests/gateway/test_pairing.py @@ -302,6 +302,30 @@ class TestApprovalFlow: class TestLockout: + def test_successful_approval_resets_failure_counter(self, tmp_path): + """A successful approval clears the brute-force streak, so isolated + typos across the gateway's lifetime don't accumulate into a spurious + lockout that rejects a valid code. + """ + with patch("gateway.pairing.PAIRING_DIR", tmp_path): + store = PairingStore() + + # One short of the lockout threshold — not locked out yet. + for _ in range(MAX_FAILED_ATTEMPTS - 1): + assert store.approve_code("telegram", "WRONGCODE") is None + assert store._is_locked_out("telegram") is False + + # A legitimate approval must reset the accumulated failures. + code = store.generate_code("telegram", "user1", "Alice") + assert store.approve_code("telegram", code) is not None + limits = store._load_json(store._rate_limit_path()) + assert limits.get("_failures:telegram", 0) == 0 + + # Because the streak was cleared, a single fresh typo afterwards + # must NOT trip the lockout (it would have with the stale count). + assert store.approve_code("telegram", "WRONGCODE") is None + assert store._is_locked_out("telegram") is False + def test_lockout_blocks_code_approval(self, tmp_path): """Regression guard for #10195: lockout must also gate approve_code. From 37d0766ba594ead9aade3ee44a5bfe0dd54a6616 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:40:44 -0500 Subject: [PATCH 4/4] fix(pairing): keep GUI approvals off the code brute-force lockout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on the request-id grant path. approve_request took the same lockout treatment as approve_code: gated by it, and recording a miss toward it. But the two paths defend different things. The lockout exists to stop guessing at the 8-char code space over a messaging channel; a request id is only ever obtained by an admin already authenticated to the store, so a miss means the row they clicked went stale. Counting those let a handful of clicks on a stale list lock the operator out of `hermes pairing approve` for an hour — the GUI DoSing the CLI. Also drops the `code`/`code_hash_prefix` compat fields from list_pending. The hash prefix is what admin surfaces mistook for an approvable code in the first place, and re-exporting the request id under the old `code` key just preserves the ambiguity; both consumers in the tree read `request_id` now. The 16-hex sniffing that had been copy-pasted into the CLI and the endpoint (where a chained conditional consulted it against the wrong field) moves to one owner, PairingStore.looks_like_request_id. The endpoint no longer reports a 429 on the request-id path, where lockout can't apply — a stale id surfaced as a bogus "locked out" while the platform sat locked for something else entirely. --- gateway/pairing.py | 54 +++++++++++++++++++------------ hermes_cli/pairing.py | 13 ++++---- hermes_cli/subcommands/pairing.py | 8 +++-- hermes_cli/web_server.py | 32 ++++++++++-------- tests/gateway/test_pairing.py | 52 +++++++++++++++++++++++++++++ web/src/lib/api.ts | 2 -- web/src/pages/PairingPage.tsx | 23 ++++--------- 7 files changed, 124 insertions(+), 60 deletions(-) diff --git a/gateway/pairing.py b/gateway/pairing.py index 5a1388f4760..de8a3c8c929 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -550,20 +550,40 @@ class PairingStore: return self._finish_approval(platform, pending, matched_key, matched_entry) + @staticmethod + def looks_like_request_id(value: str) -> bool: + """True when ``value`` has the shape of a ``list_pending`` request id. + + Request ids are ``secrets.token_hex(8)`` (16 lowercase hex chars); + pairing codes are 8 chars from an unambiguous uppercase alphabet that + excludes every hex letter's ambiguity partner. The two shapes cannot + collide, so callers accepting either can dispatch on this. + """ + value = str(value or "").strip() + return len(value) == 16 and all(c in "0123456789abcdefABCDEF" for c in value) + def approve_request(self, platform: str, request_id: str) -> Optional[dict]: """ Approve a pending pairing request by its server-side request id. - This is for admin surfaces (`pairing list`, dashboard approve buttons) - that show pending requests but must not reveal the one-time code sent - to the user. Returns ``{user_id, user_name}`` on success and ``None`` - for invalid/expired requests or platform lockout. + This is the grant path for authenticated admin surfaces (``hermes + pairing list``, the dashboard/desktop approve buttons), which show + pending requests but must never reveal the one-time code DM'd to the + user. Returns ``{user_id, user_name}`` on success, ``None`` for an + unknown/expired request id. + + Unlike :meth:`approve_code` this does NOT count a miss toward the + brute-force lockout, and is not itself gated by one. The lockout + protects the 8-char code space against guessing over a messaging + channel; a request id is only ever obtained by an admin already + authenticated to this store, so a stale id means "the row you clicked + expired", not an attack. Counting it here let a few GUI clicks on a + stale list lock the operator out of the CLI's code path too. """ with self._lock: self._cleanup_expired(platform) request_id = str(request_id or "").strip().lower() - - if self._is_locked_out(platform): + if not request_id: return None pending = self._load_json(self._pending_path(platform)) @@ -575,17 +595,15 @@ class PairingStore: if secrets.compare_digest(str(entry_id).lower(), request_id): return self._finish_approval(platform, pending, entry_id, entry) - self._record_failed_attempt(platform) return None def list_pending(self, platform: str = None) -> list: """List pending pairing requests, optionally filtered by platform. - Codes are stored hashed and are never returned. Modern entries expose a - server-side ``request_id`` that an admin can pass to approve the - listed request directly. ``code`` remains as a backward-compatible alias - for ``request_id`` for older dashboard clients. ``code_hash_prefix`` is - diagnostic-only and is not an approvable code. + Codes are stored hashed and are never returned. Each entry exposes a + server-side ``request_id`` that an authenticated admin surface passes + to :meth:`approve_request`. Legacy pre-hash entries have no approvable + id — they report an empty ``request_id`` and age out at TTL. """ results = [] with self._lock: @@ -600,16 +618,12 @@ class PairingStore: if not isinstance(created_at, (int, float)): continue age_min = int((time.time() - created_at) / 60) - hash_val = info.get("hash") - salt_val = info.get("salt") - is_modern = isinstance(hash_val, str) and isinstance(salt_val, str) - request_id = str(entry_id) if is_modern else "" - code_display = hash_val[:8] if isinstance(hash_val, str) else "legacy" + is_modern = isinstance(info.get("hash"), str) and isinstance( + info.get("salt"), str + ) results.append({ "platform": p, - "request_id": request_id, - "code": request_id, - "code_hash_prefix": code_display, + "request_id": str(entry_id) if is_modern else "", "user_id": info.get("user_id", ""), "user_name": info.get("user_name", ""), "age_minutes": age_min, diff --git a/hermes_cli/pairing.py b/hermes_cli/pairing.py index 3982540006b..17b2173d8b6 100644 --- a/hermes_cli/pairing.py +++ b/hermes_cli/pairing.py @@ -42,13 +42,12 @@ def _cmd_list(store): print(f" {'Platform':<12} {'Request ID':<18} {'User ID':<20} {'Name':<20} {'Age'}") print(f" {'--------':<12} {'----------':<18} {'-------':<20} {'----':<20} {'---'}") for p in pending: - request_id = p.get("request_id") or p.get("code") or "" print( - f" {p['platform']:<12} {request_id:<18} {p['user_id']:<20} " + f" {p['platform']:<12} {(p.get('request_id') or '-'):<18} {p['user_id']:<20} " f"{(p.get('user_name') or ''):<20} {p['age_minutes']}m ago" ) print("\n Approve with: hermes pairing approve ") - print(" The bot-delivered code also still works if the user shares it.") + print(" The code the bot DM'd the user also works if they relay it.") else: print("\n No pending pairing requests.") @@ -65,12 +64,14 @@ def _cmd_list(store): def _cmd_approve(store, platform: str, code: str): - """Approve a pairing request id or pairing code.""" + """Approve a pairing request id (from ``pairing list``) or a DM'd code.""" platform = platform.lower().strip() code = code.strip() - is_request_id = len(code) == 16 and all(c in "0123456789abcdefABCDEF" for c in code) - result = store.approve_request(platform, code) if is_request_id else store.approve_code(platform, code) + if store.looks_like_request_id(code): + result = store.approve_request(platform, code) + else: + result = store.approve_code(platform, code.upper()) if result: uid = result["user_id"] name = result.get("user_name") or "" diff --git a/hermes_cli/subcommands/pairing.py b/hermes_cli/subcommands/pairing.py index 55b022ed6db..0da4003c2a2 100644 --- a/hermes_cli/subcommands/pairing.py +++ b/hermes_cli/subcommands/pairing.py @@ -21,12 +21,16 @@ def build_pairing_parser(subparsers, *, cmd_pairing: Callable) -> None: pairing_sub.add_parser("list", help="Show pending + approved users") pairing_approve_parser = pairing_sub.add_parser( - "approve", help="Approve a pairing code" + "approve", help="Approve a pairing request" ) pairing_approve_parser.add_argument( "platform", help="Platform name (telegram, discord, slack, whatsapp)" ) - pairing_approve_parser.add_argument("code", help="Pairing code to approve") + pairing_approve_parser.add_argument( + "code", + metavar="request-id|code", + help="Request ID from 'pairing list', or the code the bot DM'd the user", + ) pairing_revoke_parser = pairing_sub.add_parser("revoke", help="Revoke user access") pairing_revoke_parser.add_argument("platform", help="Platform name") diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 16295e40cc7..49613d11437 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -13021,22 +13021,28 @@ async def list_pairing(): async def approve_pairing(body: PairingApprove): store = _pairing_store() platform = (body.platform or "").lower().strip() - code = (body.code or "").strip() - request_id = (body.request_id or "").strip() - if not platform or not (request_id or code): - raise HTTPException(status_code=400, detail="platform and request_id or code are required") + # `request_id` is what an admin surface sends after listing pending + # requests; `code` is the one-time code the user relays from their DM. + # A GUI that only knows the older field name still works — a value with + # request-id shape routes to the request path either way. + target = (body.request_id or body.code or "").strip() + if not platform or not target: + raise HTTPException( + status_code=400, detail="platform and request_id or code are required" + ) + + by_request_id = bool(body.request_id) or store.looks_like_request_id(target) + if by_request_id: + result = store.approve_request(platform, target) + else: + result = store.approve_code(platform, target.upper()) - is_request_id = len(code) == 16 and all(c in "0123456789abcdefABCDEF" for c in code) - result = ( - store.approve_request(platform, request_id) - if request_id - else store.approve_request(platform, code) - if is_request_id - else store.approve_code(platform, code) - ) if result: return {"ok": True, "user": result} - if store._is_locked_out(platform): + # Lockout only gates the code path, so only report it there — otherwise a + # stale request id would surface as a bogus 429 while the platform sat + # locked out for an unrelated reason. + if not by_request_id and store._is_locked_out(platform): raise HTTPException( status_code=429, detail=f"Platform '{platform}' is locked out after too many failed approvals.", diff --git a/tests/gateway/test_pairing.py b/tests/gateway/test_pairing.py index 8a46ca7aa43..8034cacc6a0 100644 --- a/tests/gateway/test_pairing.py +++ b/tests/gateway/test_pairing.py @@ -265,6 +265,58 @@ class TestApprovalFlow: assert result["user_name"] == "Alice" assert remaining == [] + def test_approve_request_never_reveals_or_accepts_the_code_digest(self, tmp_path): + """`list_pending` exposes an approvable id and nothing derived from the code. + + The pre-fix listing returned the code's hash prefix under a ``code`` + key, which admin GUIs posted straight back to approve — it could never + match, because approval hashes its input and compares to that digest. + """ + with patch("gateway.pairing.PAIRING_DIR", tmp_path): + store = PairingStore() + bot_code = store.generate_code("telegram", "user1", "Alice") + entry = store.list_pending("telegram")[0] + + digest = json.loads( + (tmp_path / "telegram-pending.json").read_text() + )[entry["request_id"]]["hash"] + + assert set(entry) == { + "platform", + "request_id", + "user_id", + "user_name", + "age_minutes", + } + assert bot_code not in entry.values() + assert entry["request_id"] not in (digest, digest[:8]) + # The digest prefix is not a credential on either grant path. + assert store.approve_code("telegram", digest[:8]) is None + assert store.approve_request("telegram", digest[:8]) is None + + def test_stale_request_id_never_locks_out_the_code_path(self, tmp_path): + """Clicking Approve on an expired row is not a brute-force attempt. + + Request ids only reach an admin already authenticated to this store, so + a miss means the row went stale — counting it toward the code lockout + let a handful of GUI clicks lock the operator out of `pairing approve`. + """ + with patch("gateway.pairing.PAIRING_DIR", tmp_path): + store = PairingStore() + code = store.generate_code("telegram", "user1", "Alice") + stale_id = store.list_pending("telegram")[0]["request_id"] + assert store.approve_request("telegram", stale_id) is not None + + # Re-click the now-approved row well past the lockout threshold. + for _ in range(MAX_FAILED_ATTEMPTS + 3): + assert store.approve_request("telegram", stale_id) is None + + assert store._is_locked_out("telegram") is False + # And the code path is still usable for the next real request. + next_code = store.generate_code("telegram", "user2", "Bee") + assert store.approve_code("telegram", next_code) is not None + assert code != next_code + def test_whatsapp_legacy_raw_jid_approval_survives_alias_flip(self, tmp_path, monkeypatch): mapping_dir = tmp_path / "whatsapp" / "session" diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 097cc98382d..2bd7a4b1797 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -1588,8 +1588,6 @@ export interface PairingUser { user_id: string; user_name?: string; request_id?: string; - code?: string; - code_hash_prefix?: string; age_minutes?: number; } diff --git a/web/src/pages/PairingPage.tsx b/web/src/pages/PairingPage.tsx index df46f0f124c..f11aed50833 100644 --- a/web/src/pages/PairingPage.tsx +++ b/web/src/pages/PairingPage.tsx @@ -52,15 +52,14 @@ export default function PairingPage() { }, [loadPairing]); const handleApprove = async (user: PairingUser) => { - const requestId = user.request_id || user.code; - if (!requestId) { + if (!user.request_id) { showToast("Missing pairing request", "error"); return; } const key = getUserKey(user); setApproving(key); try { - await api.approvePairing(user.platform, requestId); + await api.approvePairing(user.platform, user.request_id); showToast(`Approved: "${getUserLabel(user)}"`, "success"); loadPairing(); } catch (e) { @@ -180,22 +179,12 @@ export default function PairingPage() {
{user.platform} - {(user.request_id || user.code) && ( - - {user.request_id || user.code} - - )} - {user.code_hash_prefix && ( - - hash {user.code_hash_prefix} - - )} + + {getUserLabel(user)} +
{user.user_id} - {user.user_name && ( - {user.user_name} - )} {typeof user.age_minutes === "number" && ( {user.age_minutes}m ago )} @@ -207,7 +196,7 @@ export default function PairingPage() { size="sm" className="uppercase" onClick={() => handleApprove(user)} - disabled={approving === key || !(user.request_id || user.code)} + disabled={approving === key || !user.request_id} prefix={ approving === key ? (