fix(code-exec): make dropped HERMES_* env vars diagnosable in sandbox scrub
Follow-up mitigation for the #27303 env-scrub tightening. Dropping the broad HERMES_ prefix in favor of a 4-var operational allowlist is correct hardening, but a sandbox script that imports a repo module reading a non-allowlisted HERMES_* var at import time would otherwise see it silently unset. _scrub_child_env now emits a one-shot debug log naming the dropped non-secret HERMES_* vars and pointing at the env_passthrough opt-in escape hatch. Secret-shaped vars are never named in the log. Tests: dropped vars are logged + env_passthrough named; no log when nothing is dropped; secret vars excluded from the diagnostic.
This commit is contained in:
@ -299,3 +299,51 @@ def test_execute_code_entry_blocks_before_spawn_when_guard_denies(monkeypatch, t
|
||||
assert result["status"] == "error"
|
||||
assert "BLOCKED" in result["error"]
|
||||
assert not marker.exists() # guard denied before the child was spawned
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 6. Env-scrub diagnosability mitigation (#27303 follow-up)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_env_scrub_logs_dropped_hermes_vars(caplog):
|
||||
"""Dropping a non-allowlisted, non-secret HERMES_* var must be diagnosable:
|
||||
the scrub emits a one-shot debug log naming the dropped vars and pointing at
|
||||
the env_passthrough opt-in, so the silent behavior change (#27303) doesn't
|
||||
leave users guessing why a sandbox script sees an unset HERMES_* var."""
|
||||
import logging
|
||||
|
||||
from tools.code_execution_tool import _scrub_child_env
|
||||
|
||||
env = {
|
||||
"HERMES_HOME": "/h", # allowlisted → kept, not logged
|
||||
"HERMES_BASE_URL": "https://x", # dropped → logged
|
||||
"HERMES_KANBAN_DB": "postgres://u:p@h/db", # dropped → logged
|
||||
"HERMES_API_KEY": "sk", # secret → dropped silently (not logged)
|
||||
"PATH": "/usr/bin", # safe prefix → kept
|
||||
}
|
||||
with caplog.at_level(logging.DEBUG, logger="tools.code_execution_tool"):
|
||||
out = _scrub_child_env(env, is_passthrough=lambda _: False, is_windows=False)
|
||||
|
||||
assert "HERMES_HOME" in out and "PATH" in out
|
||||
assert "HERMES_BASE_URL" not in out and "HERMES_KANBAN_DB" not in out
|
||||
|
||||
msgs = "\n".join(r.getMessage() for r in caplog.records)
|
||||
assert "HERMES_BASE_URL" in msgs and "HERMES_KANBAN_DB" in msgs
|
||||
assert "env_passthrough" in msgs
|
||||
# Secret vars are dropped but must NOT be named in the diagnostic log.
|
||||
assert "HERMES_API_KEY" not in msgs
|
||||
|
||||
|
||||
def test_env_scrub_no_log_when_nothing_dropped(caplog):
|
||||
"""No diagnostic noise when there are no dropped HERMES_* vars."""
|
||||
import logging
|
||||
|
||||
from tools.code_execution_tool import _scrub_child_env
|
||||
|
||||
with caplog.at_level(logging.DEBUG, logger="tools.code_execution_tool"):
|
||||
_scrub_child_env(
|
||||
{"HERMES_HOME": "/h", "PATH": "/usr/bin"},
|
||||
is_passthrough=lambda _: False,
|
||||
is_windows=False,
|
||||
)
|
||||
assert "dropped" not in "\n".join(r.getMessage() for r in caplog.records)
|
||||
|
||||
@ -158,6 +158,14 @@ def _scrub_child_env(source_env, is_passthrough=None, is_windows=None):
|
||||
is_windows = _IS_WINDOWS
|
||||
|
||||
scrubbed = {}
|
||||
# Non-secret HERMES_* vars dropped by the tightened allowlist (#27303). The
|
||||
# broad "HERMES_" prefix used to pass these through; now only the
|
||||
# operational set does. The drop is intentional (those vars can carry
|
||||
# config like HERMES_KANBAN_DB / HERMES_BASE_URL), but a sandbox script
|
||||
# that imports a repo module reading one at import time would otherwise see
|
||||
# it silently unset. Surface the drop once so the behavior change is
|
||||
# diagnosable and points at the env_passthrough opt-in escape hatch.
|
||||
_dropped_hermes = []
|
||||
for k, v in source_env.items():
|
||||
if is_passthrough(k):
|
||||
scrubbed[k] = v
|
||||
@ -172,6 +180,20 @@ def _scrub_child_env(source_env, is_passthrough=None, is_windows=None):
|
||||
continue
|
||||
if is_windows and k.upper() in _WINDOWS_ESSENTIAL_ENV_VARS:
|
||||
scrubbed[k] = v
|
||||
continue
|
||||
if k.startswith("HERMES_"):
|
||||
# Non-secret (secrets were already dropped above) and not in any
|
||||
# allowlist — a deliberately-dropped HERMES_* var.
|
||||
_dropped_hermes.append(k)
|
||||
if _dropped_hermes:
|
||||
logger.debug(
|
||||
"execute_code: dropped %d non-allowlisted HERMES_* var(s) from the "
|
||||
"sandbox child env (%s). This is intentional hardening (#27303); if "
|
||||
"a sandbox script legitimately needs one, declare it via "
|
||||
"env_passthrough in the skill/config so it passes by explicit opt-in.",
|
||||
len(_dropped_hermes),
|
||||
", ".join(sorted(_dropped_hermes)),
|
||||
)
|
||||
return scrubbed
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user