Skip to content

fix(guard): skip NUL-binary text in recursion and bound remote script reads - #80241

Closed
tilllt wants to merge 1 commit into
NousResearch:mainfrom
tilllt:fix/guard-nul-remote-fallback
Closed

fix(guard): skip NUL-binary text in recursion and bound remote script reads#80241
tilllt wants to merge 1 commit into
NousResearch:mainfrom
tilllt:fix/guard-nul-remote-fallback

Conversation

@tilllt

@tilllt tilllt commented Aug 6, 2026

Copy link
Copy Markdown

What does this PR do?

Closes the two remaining fringe cases in the lifecycle-guard scanner that
survived #79454 (merged as 49d8a155c4):

  1. NUL-binary text in the recursion (cron/lifecycle_guard.py):
    _contains_unsafe_gateway_action feeds script_text returned by a
    read_remote_script callback into the recursive scan without checking
    for NUL bytes. Remote backends (terminal_tool's env-based cat
    fallback, SSH/Modal/Daytona) can return binary content; re-tokenizing
    machine code produces NUL-bearing paths that crash os.open with
    ValueError: embedded null byte (terminal guard crashes with embedded null byte when remote-read fallback feeds binary content into the scanner (full-path ELF executables) #77703, lifecycle_guard crashes on ValueError: embedded null byte from os.open — breaks all terminal commands #77780). This mirrors the
    existing NUL skip in _read_referenced_script and treats such text as
    "nothing to scan".

  2. Unbounded remote cat (tools/terminal_tool.py):
    the env-based cat fallback in _read_script_in_env had no size
    bound. A large remote binary (>1 MiB) was fully scanned, hitting the
    same oversized-content path that triggers false-positive blocks
    ("cannot restart or stop the gateway"). Applies the same 1-MiB bound
    as the local read branch.

Verification

guard-fringe-tests.py against this branch (imports lifecycle_guard.py
via importlib and statically inspects terminal_tool.py):

  • T3 (recursion guard: NUL-binary text from callback) — PASS
  • T4 (remote cat size bound) — PASS

For comparison, main + #79454 (49d8a155c4) fails 0/4 of these fringe
tests. T1/T2 (expanduser NUL in _resolve_terminal_script_path /
_resolve_script_path) are a separate, intentionally out-of-scope fix.

Related

… reads

After NousResearch#79454 two fringe cases remain in the lifecycle-guard scanner:

- _contains_unsafe_gateway_action still feeds NUL-bearing binary text
  returned by read_remote_script callbacks into the recursion, which
  re-tokenizes machine code into NUL-bearing paths and crashes os.open
  with ValueError: embedded null byte (NousResearch#77703, NousResearch#77780).
- The remote `cat` fallback in _read_script_in_env has no size bound, so
  a large remote binary (>1 MiB) is fully scanned, mirroring the
  oversized-content path that triggers false-positive blocks.

Treat NUL-bearing script text as "nothing to scan" at the recursion
site (mirroring _read_referenced_script) and apply the same 1-MiB bound
as the local branch to the remote cat fallback.

Verified with guard-fringe-tests.py against this branch: T3 (recursion
guard) and T4 (remote size bound) now pass; T1/T2 (expanduser NUL in
path resolution) are a separate fix and intentionally out of scope.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels Aug 6, 2026

@monerostar monerostar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubuntu 26.04 on linux-5800x.

Spot-check: this helps the remote script_text NUL path, but the fuller suite from #80258 still fails 6 cases here on this branch (tilde+NUL walk, oversized remote fail-closed, adversarial total-function). Prefer landing #80258 as the whole-class fix; this can fold into it or close as superseded once that merges.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing as superseded by #80258 — but this one deserves specific credit: your regression suite on #79454 (T1–T4, the expanduser-before-os.open trace, the recursion-boundary analysis, and the remote size-bound gap) is what mapped the residual class and drove the architectural fix. #80258 adopts T1–T4 verbatim as regression tests, sanitizes at the three boundaries you identified (ingestion path candidates, recursion-boundary callback text, bounded remote read at the source), and adds a never-raises adversarial sweep + a walk-crash fallback so future sibling frames degrade to a logged warning instead of a production outage.

0/4 of your tests passed on merged main; 4/4 pass on #80258. Thank you for the rigor — this is exactly the kind of analysis that turns hot-fixes into a closed bug class.

@tilllt

tilllt commented Aug 6, 2026

Copy link
Copy Markdown
Author

Superseded by #80258 — the ingestion-level sanitizer (_sanitize_remote_script_text) covers T3 (NUL-binary text from callbacks) and T4 (1 MiB byte-cap, fail-closed) on current main, plus the T1/T2 expanduser cases that were out of scope here. Verified against main (863e313185): fringe suite T1–T4 pass. Closing as implemented on main — thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants