fix(tools): validate timeout, reject whitespace old_string, narrow /private/var block
Three lower-severity core-tool robustness fixes from a targeted audit, each reproduced live: 1. terminal_tool did not validate non-positive timeouts. 'timeout or default' silently coerced 0 to the config default (0 can't mean 'no timeout'), and a negative value is truthy so it flowed into 'deadline = now + timeout' and fired an immediate '-Ns' timeout. Reject timeout <= 0 with a clear message. 2. fuzzy_find_and_replace accepted a whitespace-only old_string, which matches trivially (blank line / run of spaces) and mass-replaces under replace_all or raises an opaque ambiguity error. Reject it alongside the empty check. 3. The '/private/var/' sensitive-path prefix over-blocked ALL macOS temp-file writes: , /tmp, and /var/folders realpath into /private/var/folders on macOS (and paths are resolved through symlinks), and /private/var/tmp is a normal temp dir. Narrowed to the genuinely-sensitive subtrees (/private/var/db, /private/var/root); /etc and /private/etc stay blocked. All verified with sabotage-checked regression tests. 85 terminal/fuzzy/file tests pass; normal timeouts, legit replacements, and /var + /boot + /etc blocking are unaffected.
This commit is contained in:
parent
62f00319db
commit
7f4d155159
|
|
@ -459,6 +459,20 @@ class TestSensitivePathCheck:
|
|||
assert "error" in result
|
||||
assert "sensitive system path" in result["error"]
|
||||
|
||||
def test_macos_private_var_carveouts(self):
|
||||
"""macOS temp dirs under /private/var must not be blanket-blocked,
|
||||
while the genuinely-sensitive /private/var subtrees still are."""
|
||||
from tools.file_tools import _check_sensitive_path
|
||||
|
||||
# $TMPDIR / /tmp / /var/folders realpath into these on macOS.
|
||||
assert _check_sensitive_path("/private/var/folders/xy/T/tmp.txt") is None
|
||||
assert _check_sensitive_path("/private/var/tmp/build.log") is None
|
||||
# Sensitive subtrees remain blocked.
|
||||
assert _check_sensitive_path("/private/var/db/secret") is not None
|
||||
assert _check_sensitive_path("/private/var/root/x") is not None
|
||||
# /etc (and its macOS /private/etc mirror) stay blocked.
|
||||
assert _check_sensitive_path("/private/etc/hosts") is not None
|
||||
|
||||
@patch("tools.file_tools._get_file_ops")
|
||||
def test_normal_file_not_blocked(self, mock_get, monkeypatch):
|
||||
monkeypatch.setattr("tools.file_tools._hermes_config_resolved", "/home/user/.hermes/config.yaml")
|
||||
|
|
|
|||
|
|
@ -11,6 +11,20 @@ class TestExactMatch:
|
|||
assert count == 1
|
||||
assert new == "hi world"
|
||||
|
||||
def test_whitespace_only_old_string_rejected(self):
|
||||
"""A whitespace-only old_string is not a meaningful anchor."""
|
||||
content = "alpha\n \nbeta\n"
|
||||
new, count, _, err = fuzzy_find_and_replace(content, " ", "XXX")
|
||||
assert count == 0
|
||||
assert err is not None
|
||||
assert "whitespace" in err
|
||||
assert new == content # untouched
|
||||
|
||||
def test_empty_old_string_rejected(self):
|
||||
new, count, _, err = fuzzy_find_and_replace("abc", "", "x")
|
||||
assert count == 0
|
||||
assert err is not None
|
||||
|
||||
|
||||
def test_multiline_exact(self):
|
||||
content = "line1\nline2\nline3"
|
||||
|
|
|
|||
|
|
@ -47,6 +47,28 @@ class TestForegroundTimeoutCap:
|
|||
assert str(FOREGROUND_MAX_TIMEOUT) in result["error"]
|
||||
assert "background=true" in result["error"]
|
||||
|
||||
def test_zero_timeout_rejected(self):
|
||||
"""timeout=0 must be rejected, not silently coerced to the default."""
|
||||
from tools.terminal_tool import terminal_tool
|
||||
|
||||
with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config()), \
|
||||
patch("tools.terminal_tool._start_cleanup_thread"):
|
||||
result = json.loads(terminal_tool(command="echo hi", timeout=0))
|
||||
|
||||
assert result.get("error")
|
||||
assert "positive" in result["error"]
|
||||
|
||||
def test_negative_timeout_rejected(self):
|
||||
"""timeout=-1 must be rejected, not fire an immediate '-1s' timeout."""
|
||||
from tools.terminal_tool import terminal_tool
|
||||
|
||||
with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config()), \
|
||||
patch("tools.terminal_tool._start_cleanup_thread"):
|
||||
result = json.loads(terminal_tool(command="echo hi", timeout=-1))
|
||||
|
||||
assert result.get("error")
|
||||
assert "positive" in result["error"]
|
||||
|
||||
|
||||
def test_foreground_allows_help_variant_for_server_command(self):
|
||||
"""Informational variants like '--help' should not be blocked."""
|
||||
|
|
|
|||
|
|
@ -641,7 +641,13 @@ def _filter_read_blocked_search_results(result, task_id: str = "default") -> int
|
|||
# terminal tool's approval system. These match prefixes after os.path.realpath.
|
||||
_SENSITIVE_PATH_PREFIXES = (
|
||||
"/etc/", "/boot/", "/usr/lib/systemd/",
|
||||
"/private/etc/", "/private/var/",
|
||||
"/private/etc/",
|
||||
# macOS: /private/var mirrors /var. Block the sensitive subtrees, NOT the
|
||||
# whole thing — a blanket "/private/var/" refused every legitimate temp-file
|
||||
# write, because $TMPDIR, /tmp, and /var/folders all realpath() into
|
||||
# /private/var/folders/... on macOS (and _resolve_path_for_task resolves
|
||||
# symlinks), and /private/var/tmp is a normal temp dir.
|
||||
"/private/var/db/", "/private/var/root/",
|
||||
)
|
||||
_SENSITIVE_EXACT_PATHS = {"/var/run/docker.sock", "/run/docker.sock"}
|
||||
|
||||
|
|
|
|||
|
|
@ -83,6 +83,13 @@ def fuzzy_find_and_replace(content: str, old_string: str, new_string: str,
|
|||
if not old_string:
|
||||
return content, 0, None, "old_string cannot be empty"
|
||||
|
||||
if not old_string.strip():
|
||||
# A whitespace-only old_string matches trivially (a blank line, run of
|
||||
# spaces, etc.) and, when it recurs, either mass-replaces under
|
||||
# replace_all or raises a hard-to-diagnose ambiguity error. It's never
|
||||
# a meaningful anchor — reject it so the caller provides real context.
|
||||
return content, 0, None, "old_string is only whitespace — provide non-blank text to match"
|
||||
|
||||
if old_string == new_string:
|
||||
return content, 0, None, "old_string and new_string are identical"
|
||||
|
||||
|
|
|
|||
|
|
@ -2307,6 +2307,16 @@ def terminal_tool(
|
|||
)
|
||||
cwd = config["cwd"]
|
||||
default_timeout = config["timeout"]
|
||||
|
||||
# Validate an explicit timeout before it flows into deadline math.
|
||||
# ``timeout or default`` silently turns 0 into the default (0 can't mean
|
||||
# "no timeout" here), and a negative value is truthy so it would sail
|
||||
# through to ``deadline = now + timeout`` and fire an immediate,
|
||||
# nonsensical "-Ns" timeout. Reject non-positive values outright.
|
||||
if timeout is not None and timeout <= 0:
|
||||
return tool_error(
|
||||
f"timeout must be a positive number of seconds (got {timeout})."
|
||||
)
|
||||
effective_timeout = timeout or default_timeout
|
||||
|
||||
# Reject foreground commands where the model explicitly requests
|
||||
|
|
|
|||
Loading…
Reference in New Issue