fix(host-agent): use new hooks schema + sandboxed env for install hooks#905
Conversation
_handle_install's inner _run_install worker used the legacy _resolve_setup_hook() which only read the old service.setup_hook field, silently skipping any extension that defines its setup via the new hooks.post_install schema. It also invoked subprocess.run without an env= kwarg, inheriting the full host-agent environment (AGENT_API_KEY, DREAM_AGENT_KEY, DASHBOARD_API_KEY and other secrets) into extension setup scripts — a credential exfiltration vector. Switch to _resolve_hook(ext_dir, "post_install") to honour both schemas, and build the same minimal env allowlist that _execute_hook already uses. Delete the now-unreferenced _resolve_setup_hook function and add a source-level regression test that fails if the kwarg, allowlist, or resolver call is ever reverted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lightheartdevs
left a comment
There was a problem hiding this comment.
Strong security fix — this closes two real bugs that were masking each other.
Bug 1 (legacy hook resolver): _run_install was the only call site still using _resolve_setup_hook() (reads old service.setup_hook manifest field). Any extension using the new hooks.post_install schema was silently skipped during install. Switching to _resolve_hook(ext_dir, 'post_install') matches the pattern already used by _handle_setup_hook and _handle_hook — consistent with the rest of the agent.
Bug 2 (subprocess env inheritance): more serious — subprocess.run(...) without env= let extension setup scripts inherit AGENT_API_KEY, DREAM_AGENT_KEY, DASHBOARD_API_KEY. A malicious or compromised extension could exfiltrate host-agent credentials during install. The 8-key allowlist (hook_env) is the right shape — matches _execute_hook exactly.
Test coverage: TestInstallHookEnvAllowlist as source-level assertions is clever — will fail loudly if anyone reverts the env= kwarg or re-adds a secret key. This is the right way to lock in a security invariant.
Dead code removal: _resolve_setup_hook now unreferenced repo-wide per author's grep — good.
Author's merge-order note: after #906, before #900 and #908. That matches what I've recommended elsewhere. Ship.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Checked the current diff and live checks again. This still cleanly delivers the new hooks schema + sandboxed env handoff without widening scope. Approved.
What
_handle_install's inner_run_installworker now uses the newhooks.post_installmanifest schema and passes a minimal allowlist environment to the subprocess, matching the pattern already used by_execute_hookelsewhere in the host agent.Why
Two bugs, one function:
_run_installwas the last call site still using_resolve_setup_hook(), which only reads the oldservice.setup_hookmanifest field. Any extension declaring its setup viahooks.post_installwas silently skipped during install.subprocess.run(...)had noenv=kwarg, so the extension's setup.sh inherited the full host-agent environment includingAGENT_API_KEY,DREAM_AGENT_KEY,DASHBOARD_API_KEY. A malicious extension could exfiltrate host-agent credentials during install.How
_resolve_setup_hook(ext_dir)with_resolve_hook(ext_dir, \"post_install\")(the schema-aware resolver that_handle_setup_hookand_handle_hookalready use).hook_envdict_execute_hookuses (8 keys: PATH, HOME, SERVICE_ID, SERVICE_PORT, SERVICE_DATA_DIR, DREAM_VERSION, GPU_BACKEND, HOOK_NAME). Pass viaenv=hook_env._resolve_setup_hook(now unreferenced repo-wide).TestInstallHookEnvAllowlist— 4 source-level assertions that fail loudly if anyone reverts the kwarg, re-adds a secret, or removes a required allowlist key.Testing
Platform Impact
Review notes