mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
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)
This commit is contained in:
parent
aeda146112
commit
63805965e7
2 changed files with 29 additions and 2 deletions
|
|
@ -83,6 +83,7 @@ logger = logging.getLogger(__name__)
|
|||
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
|
||||
|
||||
|
|
|
|||
|
|
@ -100,6 +100,30 @@ class TestParseResponse:
|
|||
)
|
||||
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 ────────────────────────────────────────────────────
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue