From 4e08fff35fecd5626fbbe0491b874975710bbc8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Maia?= Date: Sun, 5 Apr 2026 13:42:44 +0100 Subject: [PATCH] Fix shell injection in command hook $ARGUMENTS substitution _inject_arguments replaced $ARGUMENTS with json.dumps(payload) directly into shell command strings. JSON encoding wraps values in double quotes, but bash still expands $(...), backticks, and other metacharacters inside double-quoted strings. A payload containing {"command": "$(whoami)"} would result in the subshell being executed. Add shlex.quote() around the serialized JSON when substituting into command hooks (shell_escape=True), wrapping it in single quotes so bash treats the entire payload as a literal string. Prompt hooks continue to use raw substitution since they pass the result to the LLM API, not a shell. The existing OPENHARNESS_HOOK_PAYLOAD environment variable (already set on line 83) remains the recommended way to access payload data from hook scripts, since env vars are not subject to shell expansion. --- CHANGELOG.md | 1 + src/openharness/hooks/executor.py | 12 ++++++-- tests/test_hooks/test_executor.py | 49 +++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05c4441..5d27187 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on Keep a Changelog, and this project currently tracks chang ### Fixed +- Shell-escape `$ARGUMENTS` substitution in command hooks to prevent shell injection from payload values containing metacharacters like `$(...)` or backticks. - Memory scanner now parses YAML frontmatter (`name`, `description`, `type`) instead of returning raw `---` as description. - Memory search matches against body content in addition to metadata, with metadata weighted higher for relevance. - Memory search tokenizer handles Han characters for multilingual queries. diff --git a/src/openharness/hooks/executor.py b/src/openharness/hooks/executor.py index 23e2e74..0b742d7 100644 --- a/src/openharness/hooks/executor.py +++ b/src/openharness/hooks/executor.py @@ -6,6 +6,7 @@ import asyncio import fnmatch import json import os +import shlex from dataclasses import dataclass from pathlib import Path from typing import Any @@ -70,7 +71,7 @@ class HookExecutor: event: HookEvent, payload: dict[str, Any], ) -> HookResult: - command = _inject_arguments(hook.command, payload) + command = _inject_arguments(hook.command, payload, shell_escape=True) try: process = await create_shell_subprocess( command, @@ -207,8 +208,13 @@ def _matches_hook(hook: HookDefinition, payload: dict[str, Any]) -> bool: return fnmatch.fnmatch(subject, matcher) -def _inject_arguments(template: str, payload: dict[str, Any]) -> str: - return template.replace("$ARGUMENTS", json.dumps(payload, ensure_ascii=True)) +def _inject_arguments( + template: str, payload: dict[str, Any], *, shell_escape: bool = False +) -> str: + serialized = json.dumps(payload, ensure_ascii=True) + if shell_escape: + serialized = shlex.quote(serialized) + return template.replace("$ARGUMENTS", serialized) def _parse_hook_json(text: str) -> dict[str, Any]: diff --git a/tests/test_hooks/test_executor.py b/tests/test_hooks/test_executor.py index 530bade..a249bfa 100644 --- a/tests/test_hooks/test_executor.py +++ b/tests/test_hooks/test_executor.py @@ -10,6 +10,7 @@ from openharness.api.client import ApiMessageCompleteEvent from openharness.api.usage import UsageSnapshot from openharness.engine.messages import ConversationMessage, TextBlock from openharness.hooks import HookEvent, HookExecutionContext, HookExecutor +from openharness.hooks.executor import _inject_arguments from openharness.hooks.loader import HookRegistry from openharness.hooks.schemas import CommandHookDefinition, PromptHookDefinition @@ -70,3 +71,51 @@ async def test_prompt_hook_can_block(tmp_path: Path): assert result.blocked is True assert result.reason == "blocked by policy" + + +# --------------------------------------------------------------------------- +# _inject_arguments shell escaping +# --------------------------------------------------------------------------- + + +def test_inject_arguments_no_escape_by_default(): + payload = {"command": "$(whoami)"} + result = _inject_arguments("echo $ARGUMENTS", payload) + # Without shell_escape, the raw JSON is substituted + assert result == 'echo {"command": "$(whoami)"}' + + +def test_inject_arguments_shell_escape_wraps_in_single_quotes(): + payload = {"command": "$(whoami)"} + result = _inject_arguments("echo $ARGUMENTS", payload, shell_escape=True) + # With shell_escape, shlex.quote wraps the JSON in single quotes + # so bash treats it as a literal string + assert result.startswith("echo '") + assert "$(whoami)" in result + + +@pytest.mark.asyncio +async def test_command_hook_escapes_shell_metacharacters(tmp_path: Path): + """$ARGUMENTS in command hooks must be shell-escaped to prevent injection.""" + registry = HookRegistry() + registry.register( + HookEvent.PRE_TOOL_USE, + CommandHookDefinition(command="echo $ARGUMENTS"), + ) + executor = HookExecutor( + registry, + HookExecutionContext( + cwd=tmp_path, + api_client=FakeApiClient('{"ok": true}'), + default_model="claude-test", + ), + ) + + # $(echo INJECTED) would execute as a subshell if not properly escaped + payload = {"tool_name": "test", "input": "$(echo INJECTED)"} + result = await executor.execute(HookEvent.PRE_TOOL_USE, payload) + + output = result.results[0].output + # With proper escaping, the literal $(echo INJECTED) must survive. + # Without escaping, bash expands the subshell and the $() wrapper is gone. + assert "$(echo INJECTED)" in output