Merge pull request #74427 from NousResearch/bb/pairing-approve-entry

fix(pairing): make the listed pending request approvable

gateway/pairing.py

@@ -412,6 +412,30 @@ 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)
+
+        # 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", "")
+        )
+
+        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 +548,62 @@ 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", ""))
+    @staticmethod
+    def looks_like_request_id(value: str) -> bool:
+        """True when ``value`` has the shape of a ``list_pending`` request id.
 
-            return {
-                "user_id": matched_entry["user_id"],
-                "user_name": matched_entry.get("user_name", ""),
-            }
+        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 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 not request_id:
+                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)
+
+            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. 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:
@@ -558,11 +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")
-                    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,
-                        "code": 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,
@@ -622,6 +683,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:

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 <platform> <code>  # Approve a pairing code
+    hermes pairing approve <platform> <request-id|code>  # Approve a pairing request
     hermes pairing revoke <platform> <user_id> # Revoke user access
     hermes pairing clear-pending     # Clear all expired/pending codes
 """
@@ -39,13 +39,15 @@ 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:
             print(
-                f"  {p['platform']:<12} {p['code']:<10} {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 <platform> <request-id>")
+        print("  The code the bot DM'd the user also works if they relay it.")
     else:
         print("\n  No pending pairing requests.")
 
@@ -62,11 +64,14 @@ def _cmd_list(store):
 
 
 def _cmd_approve(store, platform: str, code: str):
-    """Approve a pairing code."""
+    """Approve a pairing request id (from ``pairing list``) or a DM'd code."""
     platform = platform.lower().strip()
-    code = code.upper().strip()
+    code = code.strip()
 
-    result = 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 ""
@@ -92,8 +97,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):

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")

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):

hermes_cli/web_server.py

@@ -13021,21 +13021,35 @@ 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")
+    # `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())
 
-    result = 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.",
         )
     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}'.",
     )
 
 

tests/gateway/test_pairing.py

@@ -247,6 +247,76 @@ 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_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"
@@ -284,6 +354,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.
 

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)

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"}

web/src/lib/api.ts

@@ -1091,11 +1091,11 @@ export const api = {
 
   // ── Admin: Pairing ──────────────────────────────────────────────────
   getPairing: () => fetchJSON<PairingResponse>("/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,7 @@ export interface PairingUser {
   platform: string;
   user_id: string;
   user_name?: string;
-  code?: string;
+  request_id?: string;
   age_minutes?: number;
 }
 

web/src/pages/PairingPage.tsx

@@ -52,14 +52,14 @@ export default function PairingPage() {
   }, [loadPairing]);
 
   const handleApprove = async (user: PairingUser) => {
-    if (!user.code) {
-      showToast("Missing pairing code", "error");
+    if (!user.request_id) {
+      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, user.request_id);
       showToast(`Approved: "${getUserLabel(user)}"`, "success");
       loadPairing();
     } catch (e) {
@@ -179,15 +179,12 @@ export default function PairingPage() {
                 <div className="flex-1 min-w-0">
                   <div className="flex items-center gap-2 mb-1">
                     <Badge tone="outline">{user.platform}</Badge>
-                    {user.code && (
-                      <span className="font-mono text-sm">{user.code}</span>
-                    )}
+                    <span className="font-medium text-sm truncate">
+                      {getUserLabel(user)}
+                    </span>
                   </div>
                   <div className="flex items-center gap-4 text-xs text-muted-foreground">
                     <span className="truncate">{user.user_id}</span>
-                    {user.user_name && (
-                      <span className="truncate">{user.user_name}</span>
-                    )}
                     {typeof user.age_minutes === "number" && (
                       <span>{user.age_minutes}m ago</span>
                     )}
@@ -199,7 +196,7 @@ export default function PairingPage() {
                     size="sm"
                     className="uppercase"
                     onClick={() => handleApprove(user)}
-                    disabled={approving === key || !user.code}
+                    disabled={approving === key || !user.request_id}
                     prefix={
                       approving === key ? (
                         <Spinner />