Skip to content

fix(cron): skip NUL-bearing remote fallback content in lifecycle guard - #80005

Closed
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:fix/cron-lifecycle-guard-nul-fallback
Closed

fix(cron): skip NUL-bearing remote fallback content in lifecycle guard#80005
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:fix/cron-lifecycle-guard-nul-fallback

Conversation

@33hodl

@33hodl 33hodl commented Aug 6, 2026

Copy link
Copy Markdown

Summary

Harden the reusable gateway-lifecycle guard against NUL-bearing remote fallback content:

  • Skip the remote-read fallback for referenced script paths containing an embedded NUL byte (machine code tokenized into a bogus path by the recursion).
  • Treat NUL-bearing text returned by a remote fallback reader as binary — "nothing to scan" — mirroring the local binary skip, instead of tokenizing machine code and risking false blocks or ValueError crashes.
  • Real remotely-read shell scripts containing lifecycle commands remain blocked.

Why

Upstream #77703 fixed the terminal_tool path (its _read_script_in_env pre-filters binary output); this diff sits on top of that work. However, the reusable contains_gateway_lifecycle_command_or_referenced_script API accepts an arbitrary read_remote_script callback and can be called directly. Raw NUL-bearing fallback bytes were still treated as shell text, so a binary containing lifecycle-looking strings could false-block a benign command. This makes the guard itself robust regardless of caller pre-filtering.

Validation

  • pytest tests/hermes_cli/test_gateway_restart_loop.py -q -o 'addopts=' — 86 passed, including two new regressions:
    • raw binary fallback reference is skipped (no false block);
    • a real remote shell script containing a lifecycle command is still blocked.
  • Full-repo Python compile + import smoke checks.
  • Desktop TypeScript typecheck and Electron platform suite.
  • Web check (tests/typecheck/lint) and production build.
  • git diff --check clean.
  • Deployed and verified on the production backend: serve healthy, gateway running, Telegram connected, auth boundary intact.

@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
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

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.

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.

3 participants