diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 93b7abcef60b..3e0088067ed4 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -652,6 +652,17 @@ def _nous_extra_body() -> dict: _ANTHROPIC_DEFAULT_BASE_URL = "https://api.anthropic.com" _AUTH_JSON_PATH = get_hermes_home() / "auth.json" + +def _auth_json_path() -> Path: + """Return the active profile's auth.json path at call time. + + Long-lived multi-profile runtimes (Dashboard/TUI/Desktop backend, cron) + import this module once and later bind different profiles. Resolving + at call time ensures the live profile-scoped HERMES_HOME is always + respected (#40677). + """ + return get_hermes_home() / "auth.json" + # Codex OAuth endpoint used when a caller explicitly requests # provider="openai-codex". There is deliberately no hardcoded default # model: the set of models OpenAI accepts on this endpoint for @@ -1490,9 +1501,10 @@ def _read_nous_auth() -> Optional[dict]: } try: - if not _AUTH_JSON_PATH.is_file(): + auth_path = _auth_json_path() + if not auth_path.is_file(): return None - data = json.loads(_AUTH_JSON_PATH.read_text()) + data = json.loads(auth_path.read_text()) if data.get("active_provider") != "nous": return None provider = data.get("providers", {}).get("nous", {}) diff --git a/agent/skill_commands.py b/agent/skill_commands.py index f4f470c4c014..1109f997e398 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -22,6 +22,7 @@ _skill_commands: Dict[str, Dict[str, Any]] = {} _skill_commands_platform: Optional[str] = None +_skill_commands_skills_dir: Optional[Path] = None # profile-scoped invalidation (#40677) # Patterns for sanitizing skill names into clean hyphen-separated slugs. _SKILL_INVALID_CHARS = re.compile(r"[^a-z0-9-]") _SKILL_MULTI_HYPHEN = re.compile(r"-{2,}") @@ -323,19 +324,19 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: Returns: Dict mapping "/skill-name" to {name, description, skill_md_path, skill_dir}. """ - global _skill_commands, _skill_commands_platform + global _skill_commands, _skill_commands_platform, _skill_commands_skills_dir _skill_commands_platform = _resolve_skill_commands_platform() _skill_commands = {} try: - from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform, skill_matches_environment, _get_disabled_skill_names + from tools.skills_tool import _skills_dir, _parse_frontmatter, skill_matches_platform, skill_matches_environment, _get_disabled_skill_names from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files + skills_root = _skills_dir() + _skill_commands_skills_dir = skills_root disabled = _get_disabled_skill_names() seen_names: set = set() - - # Scan local dir first, then external dirs dirs_to_scan = [] - if SKILLS_DIR.exists(): - dirs_to_scan.append(SKILLS_DIR) + if skills_root.exists(): + dirs_to_scan.append(skills_root) dirs_to_scan.extend(get_external_skills_dirs()) for scan_dir in dirs_to_scan: @@ -393,10 +394,19 @@ def get_skill_commands() -> Dict[str, Dict[str, Any]]: Rescans when the active platform scope changes (e.g. a gateway process serving Telegram and Discord concurrently) so each platform sees its own ``skills.platform_disabled`` view (#14536). + Also rescans when the active profile's skills directory changes + (e.g. a Dashboard/TUI/Desktop backend serving multiple profiles + from one long-lived process — #40677). """ + try: + from tools.skills_tool import _skills_dir + current_skills_dir = _skills_dir() + except Exception: + current_skills_dir = None if ( not _skill_commands or _skill_commands_platform != _resolve_skill_commands_platform() + or _skill_commands_skills_dir != current_skills_dir ): scan_skill_commands() return _skill_commands diff --git a/agent/skill_utils.py b/agent/skill_utils.py index 23c8d99c997d..ccf72b735043 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -528,11 +528,14 @@ def normalize_skill_lookup_name(identifier: str) -> str: # (not via get_skills_dir()): callers and tests patch # ``tools.skills_tool.SKILLS_DIR`` and skill_view() itself resolves # against that module attribute, so normalization must agree with the - # exact root skill_view() will enforce. Import deferred to avoid a + # exact root skill_view() will enforce. Use _skills_dir() (not the + # module-level SKILLS_DIR) so the live profile-scoped HERMES_HOME is + # always respected in long-lived multi-profile runtimes (#40677). + # Import deferred to avoid a # module cycle (tools.skills_tool imports agent.skill_utils). try: from tools import skills_tool as _skills_tool - primary_root = Path(_skills_tool.SKILLS_DIR) + primary_root = Path(_skills_tool._skills_dir()) except Exception: primary_root = get_skills_dir() diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 8232ab77c0a5..619b479b93fc 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11507,18 +11507,15 @@ def _profile_scope(profile: Optional[str]): 1. ``load_config``/``save_config`` resolve ``get_hermes_home()`` at call time — the context-local override from ``set_hermes_home_override`` reaches them (same pattern as ``_write_profile_model``). - 2. ``tools.skills_tool`` and ``tools.skill_manager_tool`` bind - ``SKILLS_DIR`` at import time, so the override CANNOT reach them. - Like ``_call_cron_for_profile`` does for cron's module globals, - temporarily retarget both under a lock and restore them - immediately after. + 2. ``tools.skills_tool`` and ``tools.skill_manager_tool`` resolve + ``SKILLS_DIR`` at call time via ``_skills_dir()`` (#60180), so the + ``set_hermes_home_override`` above is sufficient — no module-global + patching is needed. The lock serializes concurrent skills-list + calls that may compete for the override. ``profile`` of None/""/"current" means "the dashboard's own profile" — - config resolution is untouched, but the skill-module globals are still - retargeted to the *current* ``get_hermes_home()`` so writes land in the - live home even when the import-time binding is stale (e.g. the process - imported the modules before a HERMES_HOME override, or under test - isolation). + config resolution is untouched; skills resolution uses the current + ``get_hermes_home()`` via ``_skills_dir()`` (#60180). """ requested = (profile or "").strip() @@ -11527,8 +11524,6 @@ def _profile_scope(profile: Optional[str]): set_hermes_home_override, reset_hermes_home_override, ) - from tools import skills_tool as _skills_tool - from tools import skill_manager_tool as _skill_mgr token = None if not requested or requested.lower() == "current": @@ -11538,21 +11533,9 @@ def _profile_scope(profile: Optional[str]): token = set_hermes_home_override(str(profile_dir)) with _SKILLS_PROFILE_LOCK: - old_home = _skills_tool.HERMES_HOME - old_skills_dir = _skills_tool.SKILLS_DIR - old_mgr_home = _skill_mgr.HERMES_HOME - old_mgr_skills_dir = _skill_mgr.SKILLS_DIR - _skills_tool.HERMES_HOME = profile_dir - _skills_tool.SKILLS_DIR = profile_dir / "skills" - _skill_mgr.HERMES_HOME = profile_dir - _skill_mgr.SKILLS_DIR = profile_dir / "skills" try: yield profile_dir if token is not None else None finally: - _skills_tool.HERMES_HOME = old_home - _skills_tool.SKILLS_DIR = old_skills_dir - _skill_mgr.HERMES_HOME = old_mgr_home - _skill_mgr.SKILLS_DIR = old_mgr_skills_dir if token is not None: reset_hermes_home_override(token) @@ -11562,13 +11545,10 @@ def _config_profile_scope(profile: Optional[str]): """Await-safe, config-only profile scope for handlers that ``await``. Unlike ``_profile_scope`` this touches ONLY the context-local - ``set_hermes_home_override`` contextvar — it does NOT swap the - process-global ``skills_tool``/``skill_manager`` module attributes. - Those globals are shared across all event-loop tasks, so holding them - across an ``await`` lets a concurrent skills request restore THIS - request's profile dir on its ``finally`` (cross-contamination). The - contextvar override is task-local and survives an ``await`` cleanly, - which is all endpoints that resolve ``get_hermes_home()`` at call time + ``set_hermes_home_override`` contextvar — it does NOT swap + process-global module attributes. Since ``_skills_dir()`` resolves + at call time (#60180), the contextvar override alone is sufficient + for all endpoints that resolve ``get_hermes_home()`` at call time. (config, env, gateway status) actually need. None/""/"current" means the dashboard's own profile — no override. diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index 1698f65d6c16..14ca13d8935f 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -2464,7 +2464,9 @@ async def send_multiple_images( self.name, resp.status, image_url[:80], ) continue - data = await resp.read() + data = await _read_response_bytes_bounded( + resp, _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES + ) ct = resp.headers.get("content-type", "image/png") ext = "png" if "jpeg" in ct or "jpg" in ct: @@ -3552,7 +3554,9 @@ async def send_image( if resp.status != 200: raise Exception(f"Failed to download image: HTTP {resp.status}") - image_data = await resp.read() + image_data = await _read_response_bytes_bounded( + resp, _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES + ) # Determine filename from URL or content type content_type = resp.headers.get("content-type", "image/png") @@ -3631,7 +3635,9 @@ async def send_animation( if resp.status != 200: raise Exception(f"Failed to download animation: HTTP {resp.status}") - animation_data = await resp.read() + animation_data = await _read_response_bytes_bounded( + resp, _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES + ) import io file = discord.File(io.BytesIO(animation_data), filename="animation.gif") @@ -5796,7 +5802,9 @@ async def _cache_discord_document(self, att, ext: str) -> bytes: ) as resp: if resp.status != 200: raise Exception(f"HTTP {resp.status}") - return await resp.read() + return await _read_response_bytes_bounded( + resp, _DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES + ) async def _handle_message(self, message: DiscordMessage, role_authorized: bool = False) -> None: """Handle incoming Discord messages.""" @@ -7436,12 +7444,25 @@ async def on_timeout(self): _DISCORD_CHANNEL_TYPE_PROBE_CACHE: Dict[str, bool] = {} _DISCORD_STANDALONE_JSON_BODY_LIMIT_BYTES = 1 * 1024 * 1024 _DISCORD_STANDALONE_ERROR_BODY_LIMIT_BYTES = 8 * 1024 +_DISCORD_IMAGE_DOWNLOAD_MAX_BYTES = 50 * 1024 * 1024 # generous limit for images/animations +_DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES = 100 * 1024 * 1024 # generous for file attachments def _remember_channel_is_forum(chat_id: str, is_forum: bool) -> None: _DISCORD_CHANNEL_TYPE_PROBE_CACHE[str(chat_id)] = bool(is_forum) +async def _read_response_bytes_bounded(resp: Any, limit_bytes: int) -> bytes: + """Read at most *limit_bytes* from an aiohttp response, raising on overflow.""" + data = await resp.content.read(limit_bytes + 1) + if len(data) > limit_bytes: + resp.close() + raise ValueError( + f"Response body exceeded {limit_bytes} bytes ({len(data)} bytes read)" + ) + return data + + def _probe_is_forum_cached(chat_id: str) -> Optional[bool]: return _DISCORD_CHANNEL_TYPE_PROBE_CACHE.get(str(chat_id)) diff --git a/tests/gateway/test_discord_image_download_bounds.py b/tests/gateway/test_discord_image_download_bounds.py new file mode 100644 index 000000000000..3bd4d726e54b --- /dev/null +++ b/tests/gateway/test_discord_image_download_bounds.py @@ -0,0 +1,73 @@ +"""Tests for bounded HTTP response reads in Discord image/download paths. + +Companion to #60122, #60112 (REST body bounding) — extends the same +resource-limiting pattern to image/animation/attachment downloads +in the Discord adapter that were left unbounded. +""" + +import pytest +from unittest.mock import AsyncMock, MagicMock, patch + +from plugins.platforms.discord.adapter import ( + _read_response_bytes_bounded, + _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES, + _DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES, +) + + +class AsyncContextManagerMock: + """Mock that supports ``async with``.""" + + def __init__(self, return_value): + self._return_value = return_value + + async def __aenter__(self): + return self._return_value + + async def __aexit__(self, *args): + pass + + +class TestReadResponseBytesBounded: + def test_reads_within_limit(self): + resp = MagicMock() + resp.content = MagicMock() + resp.content.read = AsyncMock(return_value=b"x" * 100) + + result = pytest.run_sync( + _read_response_bytes_bounded(resp, 200) + ) + assert result == b"x" * 100 + resp.content.read.assert_awaited_once_with(201) + + def test_raises_on_overflow(self): + resp = MagicMock() + resp.content = MagicMock() + resp.content.read = AsyncMock(return_value=b"x" * 101) + resp.close = MagicMock() + + with pytest.raises(ValueError, match="exceeded 100 bytes"): + pytest.run_sync( + _read_response_bytes_bounded(resp, 100) + ) + resp.close.assert_called_once() + + def test_exact_limit_passes(self): + resp = MagicMock() + resp.content = MagicMock() + resp.content.read = AsyncMock(return_value=b"x" * 100) + + result = pytest.run_sync( + _read_response_bytes_bounded(resp, 100) + ) + assert result == b"x" * 100 + + +class TestImageDownloadLimits: + def test_constants_are_reasonable(self): + assert _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES == 50 * 1024 * 1024 + assert _DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES == 100 * 1024 * 1024 + + def test_image_limit_greater_than_zero(self): + assert _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES > 0 + assert _DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES > 0 diff --git a/tests/tools/test_environment_snapshot_security.py b/tests/tools/test_environment_snapshot_security.py new file mode 100644 index 000000000000..0d73135ed4c2 --- /dev/null +++ b/tests/tools/test_environment_snapshot_security.py @@ -0,0 +1,37 @@ +"""Tests for owner-only permissions on JSON snapshot stores (#60259).""" + +import json +import os +import tempfile +from pathlib import Path + +import pytest + +from tools.environments.base import _save_json_store, _load_json_store + + +class TestSaveJsonStorePermissions: + def test_writes_with_owner_only_perms(self): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "test_store.json" + _save_json_store(path, {"key": "value"}) + assert path.exists() + st = path.stat() + if os.name != "nt": + assert (st.st_mode & 0o777) == 0o600 + + def test_umask_is_restored_after_write(self): + old_umask = os.umask(0o022) + os.umask(old_umask) + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "test_store.json" + _save_json_store(path, {"x": 1}) + restored = os.umask(0o022) + os.umask(restored) + assert restored == old_umask + + def test_creates_parent_directories(self): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "sub" / "nested" / "store.json" + _save_json_store(path, {}) + assert path.exists() diff --git a/tools/environments/base.py b/tools/environments/base.py index ef5b683aad2c..2eb8a26a69ee 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -167,11 +167,13 @@ def _load_json_store(path: Path) -> dict: def _save_json_store(path: Path, data: dict) -> None: - """Write *data* as pretty-printed JSON to *path*.""" + """Write *data* as pretty-printed JSON to *path* with owner-only perms.""" path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(json.dumps(data, indent=2), encoding="utf-8") - - + old_umask = os.umask(0o077) + try: + path.write_text(json.dumps(data, indent=2), encoding="utf-8") + finally: + os.umask(old_umask) def _file_mtime_key(host_path: str) -> tuple[float, int] | None: """Return ``(mtime, size)`` for cache comparison, or ``None`` if unreadable.""" try: diff --git a/tools/environments/modal.py b/tools/environments/modal.py index 3137b322113f..2fea6e28a46f 100644 --- a/tools/environments/modal.py +++ b/tools/environments/modal.py @@ -31,16 +31,25 @@ logger = logging.getLogger(__name__) -_SNAPSHOT_STORE = get_hermes_home() / "modal_snapshots.json" _DIRECT_SNAPSHOT_NAMESPACE = "direct" +def _snapshot_store_path() -> Path: + """Return the active profile's modal snapshot store path at call time. + + Long-lived multi-profile runtimes import this module once; resolve at + call time so the live profile-scoped HERMES_HOME is always respected + (#40677). + """ + return get_hermes_home() / "modal_snapshots.json" + + def _load_snapshots() -> dict: - return _load_json_store(_SNAPSHOT_STORE) + return _load_json_store(_snapshot_store_path()) def _save_snapshots(data: dict) -> None: - _save_json_store(_SNAPSHOT_STORE, data) + _save_json_store(_snapshot_store_path(), data) def _direct_snapshot_key(task_id: str) -> str: diff --git a/tools/environments/singularity.py b/tools/environments/singularity.py index 666d908b2568..a11620c81a60 100644 --- a/tools/environments/singularity.py +++ b/tools/environments/singularity.py @@ -24,7 +24,14 @@ logger = logging.getLogger(__name__) -_SNAPSHOT_STORE = get_hermes_home() / "singularity_snapshots.json" +def _snapshot_store_path() -> Path: + """Return the active profile's singularity snapshot store path at call time. + + Long-lived multi-profile runtimes import this module once; resolve at + call time so the live profile-scoped HERMES_HOME is always respected + (#40677). + """ + return get_hermes_home() / "singularity_snapshots.json" def _find_singularity_executable() -> str: @@ -62,11 +69,11 @@ def _ensure_singularity_available() -> str: def _load_snapshots() -> dict: - return _load_json_store(_SNAPSHOT_STORE) + return _load_json_store(_snapshot_store_path()) def _save_snapshots(data: dict) -> None: - _save_json_store(_SNAPSHOT_STORE, data) + _save_json_store(_snapshot_store_path(), data) def _get_scratch_dir() -> Path: diff --git a/tui_gateway/server.py b/tui_gateway/server.py index db9826bbb2f3..39ac515042ac 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -26,7 +26,7 @@ from hermes_cli.env_loader import load_hermes_dotenv from utils import is_truthy_value from tools.environments.local import hermes_subprocess_env -from agent.replay_cleanup import sanitize_replay_history +from agent.replay_cleanup import sanitize_replay_history, strip_stale_dangerous_confirmations from tui_gateway import git_probe from tui_gateway.transport import ( StdioTransport, @@ -5534,6 +5534,10 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: # not replay the unanswered call forever (#29086). prefix = display_history[: max(0, len(display_history) - len(raw_history))] history = sanitize_replay_history(raw_history) + # Strip stale dangerous-confirmation text so that a high-risk + # confirmation phrase spoken in a previous turn does not survive + # a session resume and trigger the destructive action again (#59607). + history = strip_stale_dangerous_confirmations(history, now=time.time()) # Restore the model/provider/reasoning/tier this chat last used so the # deferred build (and the info below) match the eager path — without them # the build drops the provider ("No LLM provider configured"). @@ -5609,6 +5613,10 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: : max(0, len(display_history) - len(raw_history)) ] history = sanitize_replay_history(raw_history) + # Strip stale dangerous-confirmation text so that a high-risk + # confirmation phrase spoken in a previous turn does not survive + # a session resume and trigger the destructive action again (#59607). + history = strip_stale_dangerous_confirmations(history, now=time.time()) messages = _history_to_messages(display_history) tokens = _set_session_context(target) try: