From de7e0a88750007079395da7c941371e85abfb7cd Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:48:50 -0700 Subject: [PATCH] fix(docker): heal pairing-dir ownership after `docker exec` writes (#10270) (#59130) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(docker): heal pairing-dir ownership after `docker exec` writes (#10270) The official Docker image runs the gateway as the unprivileged `hermes` user (uid 10000) via `gosu`, but `docker exec` defaults to root. Approval files written by `docker exec hermes pairing approve ` end up as `-rw------- root:root`, and the post-gosu gateway process cannot read them. The approval is silently ignored — the user keeps hitting 'Unauthorized user' on every message. The entrypoint's existing top-level chown is gated on the top-level $HERMES_HOME being mis-owned, so on warm boots (where /opt/data is already hermes:hermes) the recursive chown is skipped — meaning a container restart does NOT self-heal the bug either. Three-part fix: 1. docker/entrypoint.sh: chown the platforms/pairing/ (and legacy pairing/) subtree on every container start, regardless of the top-level decision. The directory is tiny (a few JSON files), so the unconditional chown is effectively free. Container restart now self-heals. 2. gateway/pairing.py: PairingStore._load_json was swallowing PermissionError under its bare 'except OSError' branch, which is what made this a silent failure. Split it out: log a WARNING that names the file, the gateway's uid, the file's owner/mode, and the exact docker exec -u hermes workaround. Still falls back to {} so the gateway stays up. 3. website/docs/user-guide/security.md: add a Docker tip to the pairing-CLI section pointing users at `docker exec -u hermes …` up front. Reproduced end-to-end in a containerized harness — before the fix the gateway sees 0 approved users after `docker exec` + restart; after the fix it sees the expected 1, and the file on disk goes from `root:root 600` back to `hermes:hermes 600` on next start. Fixes #10270 * fix(pairing): gate os.geteuid for Windows in PermissionError warning --- docker/stage2-hook.sh | 17 +++++++ gateway/pairing.py | 27 +++++++++++ tests/gateway/test_pairing.py | 69 +++++++++++++++++++++++++++++ website/docs/user-guide/security.md | 18 ++++++++ 4 files changed, 131 insertions(+) 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