diff --git a/docker/stage2-hook.sh b/docker/stage2-hook.sh index 6e17e6b3611..b73afdd3772 100755 --- a/docker/stage2-hook.sh +++ b/docker/stage2-hook.sh @@ -287,6 +287,23 @@ if [ -d "$HERMES_HOME/cron" ]; then chown_hermes_tree "$HERMES_HOME/cron" fi +# Always reset ownership of pairing data on every boot, same docker-exec/ +# root-write reason as profiles/ and cron/. `docker exec +# hermes pairing approve …` defaults to uid=0 and writes 0600 root-owned +# approval files that the unprivileged hermes gateway cannot read, +# silently leaving the approved user unauthorized (#10270). The targeted +# data-volume chown above only runs when the top-level $HERMES_HOME is +# mis-owned, so warm boots skip it — this block makes a container restart +# self-heal. Tiny directory (a handful of small JSON files), so the cost +# is negligible. +if [ -d "$HERMES_HOME/platforms/pairing" ]; then + chown_hermes_tree "$HERMES_HOME/platforms/pairing" +fi +# Legacy location (pre-consolidated layout). +if [ -d "$HERMES_HOME/pairing" ]; then + chown_hermes_tree "$HERMES_HOME/pairing" +fi + # Reset ownership of hermes-owned top-level state files on every boot. # The targeted data-volume chown above only covers hermes-owned # *subdirectories*; loose state files living directly under $HERMES_HOME diff --git a/gateway/pairing.py b/gateway/pairing.py index 278823d6ee2..c7d3a8c7440 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -20,6 +20,7 @@ Storage: ~/.hermes/pairing/ import hashlib import json +import logging import os import secrets import tempfile @@ -35,6 +36,8 @@ from gateway.whatsapp_identity import ( from hermes_constants import get_hermes_dir from utils import atomic_replace +logger = logging.getLogger(__name__) + # Unambiguous alphabet -- excludes 0/O, 1/I to prevent confusion ALPHABET = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789" @@ -213,6 +216,30 @@ class PairingStore: if path.exists(): try: return json.loads(path.read_text(encoding="utf-8")) + except PermissionError as e: + # Surface this loudly: a 0600 file owned by a different user + # (classic Docker symptom: `docker exec` runs as root and writes + # the file, then the gateway process — running as `hermes` after + # gosu drop — can't read it) would otherwise be swallowed by + # the generic OSError branch below, silently leaving the user + # marked unauthorized. See issue #10270. + try: + st = path.stat() + owner_info = f"owner_uid={st.st_uid} mode={oct(st.st_mode)[-4:]}" + except OSError: + owner_info = "" + # os.geteuid doesn't exist on Windows; the Docker scenario is + # POSIX-only, but the gateway (and this fallback) runs anywhere. + euid = os.geteuid() if hasattr(os, "geteuid") else "n/a" + logger.warning( + "Pairing file %s exists but is not readable as uid=%s (%s; %s). " + "If you ran `docker exec hermes pairing approve ...` as root, " + "re-run with `docker exec -u hermes ...` and " + "chown the existing file to the hermes user, or restart the " + "container so the entrypoint can fix ownership.", + path, euid, owner_info, e, + ) + return {} except (json.JSONDecodeError, OSError): return {} return {} diff --git a/tests/gateway/test_pairing.py b/tests/gateway/test_pairing.py index 74e718f181a..c08ac205577 100644 --- a/tests/gateway/test_pairing.py +++ b/tests/gateway/test_pairing.py @@ -4,6 +4,7 @@ import json import os import sys import time +from pathlib import Path from unittest.mock import patch import pytest @@ -651,3 +652,71 @@ class TestListAndClear: store.generate_code("discord", "user2") count = store.clear_pending() assert count == 2 + + +# --------------------------------------------------------------------------- +# Unreadable approved-list file logs a warning instead of failing silently +# (issue #10270: Docker `docker exec` writes root-owned 0600 files that the +# post-gosu gateway can't read; the previous OSError swallow turned the bug +# into a mystery "Unauthorized user" message) +# --------------------------------------------------------------------------- + + +class TestUnreadablePairingFile: + def test_permission_error_logs_warning_and_returns_empty(self, tmp_path, caplog): + import logging + import builtins + + approved_path = tmp_path / "weixin-approved.json" + approved_path.write_text( + '{"o9cq80fake@im.wechat": {"user_name": "x", "approved_at": 0}}' + ) + + real_open = builtins.open + + def fake_read_text(self, *a, **kw): + # Path.read_text uses Path.open internally; raise PermissionError + # to mimic a 0600 file owned by a different uid. + raise PermissionError(13, "Permission denied", str(self)) + + with patch("gateway.pairing.PAIRING_DIR", tmp_path), \ + patch.object(Path, "read_text", fake_read_text), \ + caplog.at_level(logging.WARNING, logger="gateway.pairing"): + store = PairingStore() + result = store._load_json(approved_path) + + assert result == {}, "should fall back to empty dict, not raise" + assert any( + "not readable" in rec.getMessage() and "#10270" not in rec.getMessage() + or "not readable" in rec.getMessage() + for rec in caplog.records + ), f"expected a warning about unreadable pairing file, got {caplog.records!r}" + # And the warning should include actionable advice + msgs = " ".join(rec.getMessage() for rec in caplog.records) + assert "docker exec" in msgs + assert "-u hermes" in msgs + + def test_is_approved_returns_false_when_file_unreadable(self, tmp_path, caplog): + """End-to-end: an unreadable approved.json must not crash the gateway, + and the affected user must stay unauthorized (the documented fallback + behaviour) rather than triggering a 500.""" + import logging + + approved_path = tmp_path / "weixin-approved.json" + approved_path.write_text( + '{"o9cq80fake@im.wechat": {"user_name": "x", "approved_at": 0}}' + ) + + def fake_read_text(self, *a, **kw): + raise PermissionError(13, "Permission denied", str(self)) + + with patch("gateway.pairing.PAIRING_DIR", tmp_path), \ + patch.object(Path, "read_text", fake_read_text), \ + caplog.at_level(logging.WARNING, logger="gateway.pairing"): + store = PairingStore() + ok = store.is_approved("weixin", "o9cq80fake@im.wechat") + + assert ok is False + # The warning must fire — otherwise this is the silent-failure bug. + assert any(rec.levelno == logging.WARNING for rec in caplog.records), \ + "PermissionError on approved.json must produce a WARNING log line" diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index c3e41ebe2be..60eec972e28 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -332,6 +332,24 @@ hermes pairing revoke telegram 123456789 hermes pairing clear-pending ``` +:::tip Docker users: run pairing commands as the `hermes` user +The official Docker image runs the gateway as the unprivileged `hermes` user +(uid 10000) via `gosu`, but `docker exec` defaults to root. Approval files +created by root are written with mode `0600 root:root` and the gateway +cannot read them — the approval is silently ignored ([#10270][i10270]). + +Always pass `-u hermes`: + +```bash +docker exec -u hermes hermes-agent hermes pairing approve telegram ABC12DEF +``` + +If you already ran the command as root and the user is still unauthorized, +restart the container — the entrypoint will fix ownership on the next start. + +[i10270]: https://github.com/NousResearch/hermes-agent/issues/10270 +::: + **Storage:** Pairing data is stored in `~/.hermes/pairing/` with per-platform JSON files: - `{platform}-pending.json` — pending pairing requests - `{platform}-approved.json` — approved users