fix(cron): scan dot-operator sourced scripts in lifecycle guard (#77925) - #77926
Open
Mengchee118 wants to merge 1 commit into
Open
fix(cron): scan dot-operator sourced scripts in lifecycle guard (#77925)#77926Mengchee118 wants to merge 1 commit into
Mengchee118 wants to merge 1 commit into
Conversation
`_iter_referenced_shell_scripts` recognises the `source` builtin so a script
pulled in with `source ./restart.sh` gets scanned for lifecycle commands. The
POSIX dot operator is the same builtin, but it was not caught:
if executable_name in {".", "source"}:
`executable_name` is `Path(executable).name`, and `Path(".").name` is the
**empty string** -- pathlib normalises "." to the current directory, whose name
is "". So the set membership never matched for `.`, the sourced script was
never added to the reference walk, and its contents were never scanned.
Verified against current main:
. /tmp/restart.sh -> not blocked (script never scanned)
source /tmp/restart.sh -> blocked
bash /tmp/restart.sh -> blocked
where /tmp/restart.sh contains a `hermes gateway restart` line. Sourcing runs
the script in the current shell, so the dot spelling is not merely equivalent
to `source` -- it is the more common form in practice.
Fix compares the raw token as well as the basename:
if executable in {".", "source"} or executable_name == "source":
Keeping the `executable_name == "source"` arm preserves the existing behaviour
for a path-qualified spelling, while the raw-token test catches `.` without
relying on pathlib normalisation.
Tests (tests/hermes_cli/test_gateway_restart_loop.py):
- test_dot_operator_sourced_script_is_scanned -- the regression; fails on main
- test_source_builtin_sourced_script_is_scanned -- `source` stays blocked
- test_dot_operator_clean_script_not_blocked -- widening the check must not
false-block an innocent `. ./activate.sh`
Found while auditing the guard after NousResearch#76762. Scoped deliberately to this one
defect; the NUL-padded-script bypass I found in the same audit is a separate
PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
cron/lifecycle_guard.pyscans scripts pulled in with thesourcebuiltin so agateway-lifecycle command inside them is blocked. The equivalent POSIX dot
operator was not caught, so
. ./restart.shwas never scanned.The executable test compared basename only:
Path(".").nameis the empty string — pathlib normalises"."to thecurrent directory, whose
.nameis""— so the"."entry could never match.sourcematched;.silently fell through, the sourced script was never addedto the reference walk, and its contents were never scanned.
The fix compares the raw token as well as the basename:
The
executable_name == "source"arm is kept deliberately so a path-qualifiedspelling keeps behaving as before, while the raw-token test catches
.withoutdepending on pathlib normalisation.
Related Issue
Fixes #77925
Type of Change
Changes Made
cron/lifecycle_guard.py—_iter_referenced_shell_scripts: compare the rawexecutable token as well as the basename, so the dot operator reaches the
referenced-script scan. Comment records why the basename test cannot work
for
..tests/hermes_cli/test_gateway_restart_loop.py— three tests inTestLifecycleGuardModule.How to Test
Reproduce on
main(all three assertions describe current behaviour):With this PR the first assertion becomes
True.Automated:
cron/lifecycle_guard.pytomainand keeping the new testsfails exactly
test_dot_operator_sourced_script_is_scanned(
AssertionError: assert False is True) — so the test genuinely covers thedefect rather than passing vacuously.
Checklist
Code
pytest tests/hermes_cli/test_gateway_restart_loop.py -qand all tests pass (85 passed)Documentation & Housekeeping
cli-config.yaml.example— N/A, no config keysCONTRIBUTING.md/AGENTS.md— N/A, no architecture changedependency rather than adding one.
.andsourceare POSIX-shellbuiltins, so behaviour is unchanged on Windows, where this walk is not
reached for shell scripts.
Notes for reviewers
On duplicates: there are several open PRs on this file (#77383, #77729,
#77806, #77894) — all of them address the crash class (
ValueError: embedded null byte). This is a different defect: a silent scan miss, nocrash involved. It does not overlap or conflict with those changes; the hunk is
40 lines away from the crash sites.
Scope: the same audit turned up a second, independent defect — the
#76762binary test (
b"\x00" in data) treats any NUL-bearing file as an unscannablebinary, but
bashexecutes a text script straight past an embedded NUL, soa single pad byte bypasses the scan entirely. That is a separate PR; keeping
them apart so each can be reviewed on its own merits.