From 63805965e7a907f6b5e3a687fc37bed2004e7634 Mon Sep 17 00:00:00 2001
From: flamiinngo <kingsleyemeka117@gmail.com>
Date: Sun, 17 May 2026 03:48:42 +0100
Subject: [PATCH] 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 687af5ec4ba..6639700b553 100644
--- a/agent/shell_hooks.py
+++ b/agent/shell_hooks.py
@@ -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
 
diff --git a/tests/agent/test_shell_hooks.py b/tests/agent/test_shell_hooks.py
index 088c23eb466..743c9acb843 100644
--- a/tests/agent/test_shell_hooks.py
+++ b/tests/agent/test_shell_hooks.py
@@ -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 ────────────────────────────────────────────────────