fix(security): lifecycle_guard must not crash or over-block on non-script paths

[temorian1]

What and why

_read_referenced_script detects binaries (NUL byte in the first chunk) and returns None. The caller treats that None as "not present locally" and falls through to read_remote_script:

script_text, unsafe = _read_referenced_script(script_path)
if unsafe:
    return True
if script_text is None and read_remote_script is not None:
    script_text = read_remote_script(str(script_path))   # no binary detection

That callback reads via cat and decodes with errors="replace". NUL bytes are valid UTF-8 and survive, so the machine code comes back as a string and is tokenized as a command line. The linker path in the ELF string table produces a candidate containing a NUL byte, which reaches os.open:

ValueError: embedded null byte
  cron/lifecycle_guard.py:260 in _read_referenced_script

The guard crashes instead of deciding. Any command invoked through an absolute binary or interpreter path is affected while _HERMES_GATEWAY=1, for example /path/to/chrome --version or /path/to/venv/bin/python3 -c "...".

The root cause is that script_text is None means two different things — "not present locally" and "deliberately skipped as a binary". Only the second is a decision; the fallback treats both as the first.

Why it survives testing

tools/terminal_tool.py is the only caller that supplies read_remote_script. Called directly, the guard never crashes. Every check outside a gateway worker is green, which is exactly where manual verification happens.

How to test

Fails on main, passes with this PR:

from cron.lifecycle_guard import (
    contains_gateway_lifecycle_command_or_referenced_script as guard,
)

def read_remote(path):
    """What tools/terminal_tool.py supplies: raw read, lossy decode."""
    with open(path, "rb") as handle:
        return handle.read(200_000).decode("utf-8", errors="replace")

cmd = "/usr/bin/python3 --version"

guard(cmd)                                   # False
guard(cmd, read_remote_script=read_remote)   # ValueError on main

The included regression test builds a minimal ELF-shaped file rather than depending on a system binary, so it is hermetic.

pytest tests/hermes_cli/test_gateway_restart_loop.py

• With the fix: 83 passed
• Without the fix: test_remote_read_of_binary_does_not_crash_the_guard fails with ValueError: embedded null byte

Protection is unchanged

Both lifecycle forms remain blocked, with and without the callback:

systemctl restart hermes-gateway   -> blocked
hermes gateway restart             -> blocked

A binary cannot carry a command, so declaring it "nothing to scan" removes no coverage. This mirrors the rule the local read path already applies (#76762).

Platforms tested

Linux (Ubuntu, x86-64), Python 3.11, in a gateway worker with _HERMES_GATEWAY=1 and via the test suite. Not tested on macOS or WSL2 — the change touches no platform-specific behaviour, only exception handling and a string check.

Related

Follow-up to #76762, which introduced the binary-is-not-a-script rule for the local read path. This PR extends the same rule to the remote path.

---

Second commit: the directory case

_read_referenced_script also has no S_ISDIR branch, so a directory falls into the conservative not S_ISREG case and the command is reported unsafe. Tokenizing a line like for candidate in (Path("/opt/ms-playwright"), ...) can yield a directory as a "referenced script". On v0.20.0 this blocked every command referencing one inside a gateway worker and silently disabled our PDF export for hours — being blocked is a normal guard outcome, so nothing surfaced as an error.

It is the exact counterpart to #76762: a directory cannot carry a command.

Flagged honestly: on current main we could not construct a command that still yields a directory as a candidate, so this commit ships without a failing test. It is defensive. Drop it if you would rather not carry a guard without a reproduction — the first commit stands on its own.

[kshitijk4poor]

Closing as superseded by #80258, which fixes this whole bug class architecturally rather than per-callsite: path candidates are sanitized once at the ingestion boundary (NUL/empty/unexpandable tokens rejected before any OS call), text from any read_remote_script callback is sanitized at the recursion boundary (NUL = binary = nothing to scan; >1 MiB = fail closed), the remote fallback read is bounded at the source (head -c, so oversized binaries never cross the wire), and the public guard is total by construction — an unexpected walk failure logs and falls back to the direct-scan verdict instead of breaking every terminal command.

Your report and fix targeted a real member of this class — thank you. The per-callsite patches kept leaving sibling frames exposed (#76762 → #77703 → #77780 → #78256 each crashed one frame away from the previous fix), which is why we went with the boundary fix instead of merging the fragments individually. #80258 carries regression tests for the NUL-path, binary-callback, oversized-read, unset-HOME, and walk-crash cases plus an adversarial never-raises sweep.