Conversation
added 2 commits
September 19, 2026 13:52
#428) Section G planted its "foreign" fixture at the hard-coded /tmp/claude-hook-log-999999.tmp with a plain `>` redirect, which follows a symlink and truncates whatever it points at. A co-tenant on a shared host or multi-tenant runner could pre-create that path as a symlink and have it truncated the next time the suite ran, while assert_file still passed (the link resolves) and the trailing rm -f removed only the link -- so the suite reported green with the hazard still armed. The fixture now lives under the suite's own private mktemp -d tree ($TMP/g-foreign), matching the pattern tests/test-hook-tmpfile.sh section D already uses for the hooks, and refuses to write at all if the path already exists or is a symlink. Found by a Claude Security whole-repository scan (F5/F6). Test suite only; nothing shipped is affected. Co-Authored-By: Max <noreply>
Self-review (Explore + oss:auditor) on the #428 fix flagged two issues: - The [ -e ]/[ -L ]-then-write guard added in the first pass is the exact check-then-act pattern tests/test-hook-tmpfile.sh's own header documents as rejected for this class of hazard (a race between the check and the write). It bought nothing here -- $TMP is a fresh, private mktemp -d (mode 0700) that can never already hold the fixture path -- and the branch that would fail loudly had no test exercising it, so it shipped as untested dead code. - The comment block above section G was stale: it explained the pre-#428 threat model and said nothing about the new fixture location or why a guard was tried and dropped. Dropped the guard, kept the move to the private $TMP tree (which is what actually closes the hazard), and updated the comment to say what changed and why the guard was not kept. Updated changelog.d/428.fixed.md to match. Co-Authored-By: Max <noreply>
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
1 participant
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.
Section G of
tests/test-session-markers.shplanted its "foreign" log fixture at a hard-coded shared path,/tmp/claude-hook-log-999999.tmp, usingprintf ... > "$FOREIGN". That redirect opens O_CREAT|O_TRUNC and follows a symbolic link, with no[ -L ]check first. On a shared host or multi-tenant CI runner, a co-tenant who pre-planted a symlink at that exact path got the link target truncated the next time the suite ran, whileassert_filestill passed (the link resolves) and the trailingrm -fremoved only the link -- so the suite reported green with the hazard still armed. Found by a Claude Security whole-repo scan at bc17f57 (findings F5 and F6). Test suite only; nothing shipped is affected.The fixture now lives under the suite's own private, mode-0700
mktemp -dtree, matching the sibling pattern already intests/test-hook-tmpfile.shsection D. An[ -e ]/[ -L ]-then-write guard was tried per the issue's "at minimum" fallback suggestion, then dropped on self-review: it is exactly the check-then-act patterntest-hook-tmpfile.sh's own header comment documents as rejected for this hazard class, it bought nothing once the fixture sits in a freshly-made private directory, and its FAIL branch was never exercised by any assertion.Ran the targeted suite (
bash tests/test-session-markers.sh): 57 passed, 0 failed, including all 5 assertions in section G. Reviewed by two spawns (Explore and oss:auditor) against commit b706509; three findings, all fixed in the follow-up commit.Closes #428.
[AI-generated]