From 9e5c11f8a0844e106a02f9ae3ec6f14a215425d3 Mon Sep 17 00:00:00 2001 From: flamiinngo Date: Sun, 17 May 2026 03:31:08 +0100 Subject: [PATCH 1/3] fix(security): honor shell hook blocks even when message/reason is absent _parse_response in agent/shell_hooks.py only forwarded a pre_tool_call block directive if the hook also provided a non-empty message or reason. When either field was missing the function returned None, causing Hermes to treat the response as a no-op and execute the tool unconditionally. This means a hook that outputs {"action": "block"} or {"decision": "block"} without a reason string is silently ignored. The security boundary fails open: tools the user intended to gate are executed anyway. Fix: remove the message-presence guard. Honor the block unconditionally and fall back to a default message when none is provided. Existing hooks that already include a message or reason are unaffected. --- agent/shell_hooks.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index bad5388f88bf8..687af5ec4bafe 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -515,13 +515,11 @@ def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: if event == "pre_tool_call": if data.get("action") == "block": - message = data.get("message") or data.get("reason") or "" - if isinstance(message, str) and message: - return {"action": "block", "message": message} + message = data.get("message") or data.get("reason") or "Blocked by shell hook." + return {"action": "block", "message": message} if data.get("decision") == "block": - message = data.get("reason") or data.get("message") or "" - if isinstance(message, str) and message: - return {"action": "block", "message": message} + message = data.get("reason") or data.get("message") or "Blocked by shell hook." + return {"action": "block", "message": message} return None context = data.get("context") From 32464e9890522c636cc14273241757c33f0c41c1 Mon Sep 17 00:00:00 2001 From: flamiinngo Date: Sun, 17 May 2026 03:48:42 +0100 Subject: [PATCH 2/3] fix(security): restore type safety and extract constant in shell hook block handler Address code review feedback on _parse_response: 1. Restore isinstance(raw, str) guard so non-string message/reason values (e.g. integers, lists) from a malformed hook response fall back to the default rather than being forwarded as-is. This keeps the contract that message in the returned dict is always a string. 2. Extract the repeated literal 'Blocked by shell hook.' into a module-level constant _DEFAULT_BLOCK_MESSAGE to avoid duplication and make it easy to change in one place. Four new unit tests added to tests/agent/test_shell_hooks.py covering: - action block with no message (uses default) - decision block with no reason (uses default) - action block with empty string message (uses default) - action block with non-string message, e.g. integer (uses default) --- agent/shell_hooks.py | 7 +++++-- tests/agent/test_shell_hooks.py | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 687af5ec4bafe..6639700b55333 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -83,6 +83,7 @@ DEFAULT_TIMEOUT_SECONDS = 60 MAX_TIMEOUT_SECONDS = 300 ALLOWLIST_FILENAME = "shell-hooks-allowlist.json" +_DEFAULT_BLOCK_MESSAGE = "Blocked by shell hook." # (event, matcher, command) triples that have been wired to the plugin # manager in the current process. Matcher is part of the key because @@ -515,10 +516,12 @@ def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: if event == "pre_tool_call": if data.get("action") == "block": - message = data.get("message") or data.get("reason") or "Blocked by shell hook." + raw = data.get("message") or data.get("reason") + message = raw if isinstance(raw, str) and raw else _DEFAULT_BLOCK_MESSAGE return {"action": "block", "message": message} if data.get("decision") == "block": - message = data.get("reason") or data.get("message") or "Blocked by shell hook." + raw = data.get("reason") or data.get("message") + message = raw if isinstance(raw, str) and raw else _DEFAULT_BLOCK_MESSAGE return {"action": "block", "message": message} return None diff --git a/tests/agent/test_shell_hooks.py b/tests/agent/test_shell_hooks.py index 088c23eb46655..743c9acb843ff 100644 --- a/tests/agent/test_shell_hooks.py +++ b/tests/agent/test_shell_hooks.py @@ -100,6 +100,30 @@ def test_pre_llm_call_block_ignored(self): ) assert r is None + def test_block_action_without_message_uses_default(self): + """Block is honored even when message/reason is absent.""" + r = shell_hooks._parse_response("pre_tool_call", '{"action": "block"}') + assert r == {"action": "block", "message": shell_hooks._DEFAULT_BLOCK_MESSAGE} + + def test_block_decision_without_reason_uses_default(self): + """Block is honored even when reason/message is absent.""" + r = shell_hooks._parse_response("pre_tool_call", '{"decision": "block"}') + assert r == {"action": "block", "message": shell_hooks._DEFAULT_BLOCK_MESSAGE} + + def test_block_action_empty_message_uses_default(self): + """Empty string message falls back to default, not empty string.""" + r = shell_hooks._parse_response( + "pre_tool_call", '{"action": "block", "message": ""}', + ) + assert r == {"action": "block", "message": shell_hooks._DEFAULT_BLOCK_MESSAGE} + + def test_block_action_non_string_message_uses_default(self): + """Non-string message (e.g. integer) falls back to default.""" + r = shell_hooks._parse_response( + "pre_tool_call", '{"action": "block", "message": 42}', + ) + assert r == {"action": "block", "message": shell_hooks._DEFAULT_BLOCK_MESSAGE} + # ── _serialize_payload ──────────────────────────────────────────────────── From 12ec5d57281cd0a35d432396acb1696199982f90 Mon Sep 17 00:00:00 2001 From: flamiinngo Date: Sun, 17 May 2026 03:58:10 +0100 Subject: [PATCH 3/3] refactor(security): extract _block_message helper to unify block logic in _parse_response Both the `action=block` and `decision=block` branches in _parse_response shared identical field-priority and type-validation logic. Extract it into a single _block_message(primary, secondary) helper so the two branches are one line each and the type guard lives in exactly one place. No functional change: existing tests (TestParseResponse, 14 tests) all pass unchanged, confirming identical behaviour. --- agent/shell_hooks.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 6639700b55333..79d494d7dcbe8 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -482,6 +482,17 @@ def _serialize_payload(event: str, kwargs: Dict[str, Any]) -> str: return json.dumps(payload, ensure_ascii=False, default=str) +def _block_message(primary: Any, secondary: Any) -> str: + """Return a validated string block message, falling back to the default. + + Accepts two candidate fields (primary wins over secondary) so callers + can express field-priority differences between the two hook wire formats + without duplicating the type-check logic. + """ + raw = primary or secondary + return raw if isinstance(raw, str) and raw else _DEFAULT_BLOCK_MESSAGE + + def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: """Translate stdout JSON into a Hermes wire-shape dict. @@ -516,13 +527,9 @@ def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: if event == "pre_tool_call": if data.get("action") == "block": - raw = data.get("message") or data.get("reason") - message = raw if isinstance(raw, str) and raw else _DEFAULT_BLOCK_MESSAGE - return {"action": "block", "message": message} + return {"action": "block", "message": _block_message(data.get("message"), data.get("reason"))} if data.get("decision") == "block": - raw = data.get("reason") or data.get("message") - message = raw if isinstance(raw, str) and raw else _DEFAULT_BLOCK_MESSAGE - return {"action": "block", "message": message} + return {"action": "block", "message": _block_message(data.get("reason"), data.get("message"))} return None context = data.get("context")