mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(pairing): keep GUI approvals off the code brute-force lockout
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.
This commit is contained in:
Brooklyn Nicholson
parent
c352a322b0
commit
37d0766ba5
7 changed files with 124 additions and 60 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 <platform> <request-id>")
|
||||
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 ""
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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.",
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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() {
|
|||
<div className="flex-1 min-w-0">
|
||||
<div className="flex items-center gap-2 mb-1">
|
||||
<Badge tone="outline">{user.platform}</Badge>
|
||||
{(user.request_id || user.code) && (
|
||||
<span className="font-mono text-sm">
|
||||
{user.request_id || user.code}
|
||||
</span>
|
||||
)}
|
||||
{user.code_hash_prefix && (
|
||||
<span className="font-mono text-xs text-muted-foreground">
|
||||
hash {user.code_hash_prefix}
|
||||
</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>
|
||||
)}
|
||||
|
|
@ -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 ? (
|
||||
<Spinner />
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue