security(gateway): re-resolve hooks directory per call to fix profile isolation - #428
Open
hashbender wants to merge 1 commit into
Open
security(gateway): re-resolve hooks directory per call to fix profile isolation#428hashbender wants to merge 1 commit into
hashbender wants to merge 1 commit into
Conversation
|
Review Complete Risk: 🟠 High (62/100) — no findings · 91 LOC across 2 files Profile-aware hook directory resolution introduced in this PR creates a sys.modules namespace collision when two profiles register hooks with the same name, risking cross-profile type resolution errors. Files Reviewed (2 files) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gateway/hooks.py::HOOKS_DIRis resolved once at import time viaget_hermes_home(), which reads a context-localContextVar(_HERMES_HOME_OVERRIDE) set per-request under the multiplexed gateway (multiple profiles served from one process, e.g. the desktoptui_gateway). Freezing the resolved path at import time pins every laterHookRegistry.discover_and_load()call to whichever profile'sHERMES_HOMEhappened to be active the first time this module was imported in the process.This is more serious than the typical instance of this bug class (already fixed for cache dirs,
skills_hub.py,rich_sent_store.py, and — from this same audit — the Anthropic OAuth file, Nous auth.json, sessions.json index, checkpoint store, and sticker cache): the hooks system loads and executes arbitrary Python (handler.py, dynamically imported viaimportlib.util) in response to live agent events. Under the multiplexed gateway, each profile owns its ownGateway/HookRegistryinstance, but they all read the same frozen module constant — so a later-starting profile'sHookRegistrydiscovers and executes the FIRST profile's hook handlers against its own live event context (session_id,chat_id,message,responsetext truncated to 500 chars, per the module's own docstring), not the profile's own hooks. This is both a cross-profile data leak and an isolation-boundary violation (profile B's process runs profile A's hook code).Changes
gateway/hooks.py: added_resolve_hooks_dir(), called fromdiscover_and_load()instead of readingHOOKS_DIRdirectly. Keeps the module constant for backward compat and to preserve the existing test seam (tests/gateway/test_hooks.pyhas many call sites thatpatch("gateway.hooks.HOOKS_DIR", tmp_path)) — the resolver honors a monkeypatched value away from its import-time default, otherwise re-resolves fresh, mirroringgateway/platforms/base.py::_resolve_cache_dir's established pattern.tests/test_profile_isolation_runtime.py(the existing profile-isolation regression suite): addedTestGatewayHooksDirResolutionwith three tests — the resolved path actually differs between two distinct profile overrides, an end-to-end test proving a hook that only exists under profile B's hooks dir is NOT discovered when profile A's override is active (and vice versa), and a "monkeypatched constant still wins" regression test for the test-seam-preserving resolver.Test plan
pytest tests/gateway/test_hooks.py tests/gateway/test_background_command.py -q— 42 passed, 1 pre-existing unrelated failure (confirmed viagit stash: a macOS/private/varvs/varsymlink path-comparison quirk intest_media_files_routed_by_type, present identically before this change)pytest tests/test_profile_isolation_runtime.py -q— 13 passed (10 pre-existing + 3 new)ruff checkon all changed files — cleanMirror-of: NousResearch#56508
NousResearch#56508