From c9413e3b75c74915aa9d1a9e9b6839bc672f57a0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 23 Aug 2026 09:35:02 -0700 Subject: [PATCH 1/2] Reject Symlinks From Shebang Discovery, Propagate read Failures Fixes 2 findings from coderabbitai on PR #952 (the develop -> main promotion PR carrying #951's shell-lint-gate work), both reproduced before the fix. ## A tracked symlink could read an arbitrary host file The extensionless-shebang scan opens each candidate file to check its first line, host-side, before Docker ever starts. `[ -f "$file" ]` and Python's `Path.open()` both follow a symlink, so a tracked symlink pointing outside the checkout (`ops/evil -> /etc/shadow`, or anywhere else the CI runner or a dev's own machine can read) had its target's first line read on the host as part of merely checking whether it looks like a shell script. Reproduced: a symlink to a file containing `TOP SECRET` content was read through `read <` on the CI side and `Path.open()` on the Python side. - `.github/workflows/validate-task.yml`: added `[ ! -h "$file" ]` (checks the tracked path itself via `lstat`, never follows it) alongside the existing `-f` check, before any read. - `scripts/docker_lint.py`: `has_shell_shebang` now checks `is_symlink()` first and returns `False` without ever opening the path. - `scripts/tests/test_docker_lint.py`: added `track_symlink()` and two regression tests proving a symlinked extensionless script is excluded from discovery and never opened. This matches established fleet precedent: `build_dist.py`, `skills_install.py`, and `carry.py` (`spec/`) already reject symlinks for the same reason, confirmed by their own existing test suites passing unaffected. ## `read`'s `|| true` masked a genuine read failure too `IFS= read -r first_line < "$file" || true` (landed in #953) tolerated the harmless no-trailing-newline EOF case, but the same `|| true` also swallowed a genuine read failure (permission denied, file removed mid-run), silently skipping a tracked script CI should have linted. - `.github/workflows/validate-task.yml`: replaced the `read`/`|| true` pair with `first_line="$(head -n 1 -- "$file")"`, which reads a no-trailing-newline file cleanly (exit 0) while still failing loudly on a genuine read error, per CodeRabbit's own verified reproduction. ## Verified Reproduced all three cases end to end in a scratch repo: a tracked symlink to a file containing secret content is excluded from discovery on both the CI step's exact commands and `docker_lint.py` (and never opened, confirmed via the new Python test), a no-trailing-newline script is still discovered and read correctly, and a genuine permission-denied read aborts the script instead of being silently skipped. Full test suite (797 tests), ruff, mypy, actionlint, `repo_gate.py`, `prose_lint.py --diff origin/develop`, and the complete `docker_lint.py` run (all 7 linters) all pass clean. --- .github/workflows/validate-task.yml | 8 ++++---- scripts/docker_lint.py | 11 +++++++++-- scripts/tests/test_docker_lint.py | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 7be9e98e..196a09da 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -115,10 +115,10 @@ jobs: mapfile -d '' -t candidates < <(git ls-files -z) for file in "${candidates[@]}"; do base="${file##*/}" - if [[ "$base" != *.* ]] && [ -f "$file" ]; then - # `read` fails at EOF even when it fills first_line, so the check reads the content regardless. - first_line="" - IFS= read -r first_line < "$file" || true + # Never follows a tracked symlink: -h checks the git-tracked path itself, before any read. + if [[ "$base" != *.* ]] && [ -f "$file" ] && [ ! -h "$file" ]; then + # `head` reads a no-trailing-newline file cleanly and still fails on a genuine read error. + first_line="$(head -n 1 -- "$file")" if is_shell_shebang "$first_line"; then scripts+=("$file") fi diff --git a/scripts/docker_lint.py b/scripts/docker_lint.py index 738b44d2..7db39d5a 100755 --- a/scripts/docker_lint.py +++ b/scripts/docker_lint.py @@ -194,9 +194,16 @@ def shell_shebang_interpreter(line: str) -> str | None: def has_shell_shebang(root: Path, relative_path: str) -> bool: - """Report whether a tracked file's shebang directly names bash or sh.""" + """Report whether a tracked file's shebang directly names bash or sh. + + Never follows a tracked symlink: `is_symlink()` uses `lstat`, so checking it first + keeps a symlink pointing outside the checkout from ever reaching `open()`. + """ + path = root / relative_path try: - with (root / relative_path).open("rb") as handle: + if path.is_symlink(): + return False + with path.open("rb") as handle: first_line = handle.readline(256) except OSError: return False diff --git a/scripts/tests/test_docker_lint.py b/scripts/tests/test_docker_lint.py index a0fa1174..dc4e6364 100755 --- a/scripts/tests/test_docker_lint.py +++ b/scripts/tests/test_docker_lint.py @@ -51,6 +51,12 @@ def track(self, name: str, body: str = "content\n") -> None: path.write_text(body, encoding="utf-8") subprocess.run(["git", "-C", str(self.root), "add", "--", name], check=True) + def track_symlink(self, name: str, target: Path) -> None: + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.symlink_to(target) + subprocess.run(["git", "-C", str(self.root), "add", "--", name], check=True) + def invoke(self, selected: set[str], runner: FakeRunner) -> tuple[int, str]: output = io.StringIO() with contextlib.redirect_stdout(output): @@ -157,6 +163,19 @@ def test_extensionless_shebang_script_merges_with_glob_matched_scripts(self) -> docker_lint.tracked_files(self.root, linter), ) + def test_extensionless_symlink_is_never_followed(self) -> None: + secret = self.root.parent / "secret" + secret.write_text("TOP SECRET\n#!/usr/bin/env bash\n", encoding="utf-8") + self.track_symlink("ops/evil-symlink", secret) + linter = next(linter for linter in docker_lint.LINTERS if linter.name == "shellcheck") + self.assertEqual([], docker_lint.tracked_files(self.root, linter)) + + def test_has_shell_shebang_reports_false_for_a_symlink_without_reading_it(self) -> None: + secret = self.root.parent / "secret" + secret.write_text("#!/usr/bin/env bash\n", encoding="utf-8") + self.track_symlink("ops/evil-symlink", secret) + self.assertFalse(docker_lint.has_shell_shebang(self.root, "ops/evil-symlink")) + def test_extensionless_untracked_shebang_script_is_not_picked_up(self) -> None: path = self.root / "ops" / "vps-backup-pull" path.parent.mkdir(parents=True, exist_ok=True) From 8bf4cc8b9863cc193b7973899013a041c2de64e3 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 23 Aug 2026 09:45:01 -0700 Subject: [PATCH 2/2] Fully Short-Circuit the Symlink Check, Harden Its Tests Fixes 3 real findings from qodo-code-review and coderabbitai on PR #955 (this chain's own symlink-rejection fix), plus one docstring-wrap style fix. ## The workflow's -f still dereferenced the symlink target `[ -f "$file" ] && [ ! -h "$file" ]` evaluates left to right, so `-f`'s own dereferencing stat still ran against the symlink target before `-h` ever got a chance to reject it, contradicting the step's own "never follows" comment. Reordered to `[ ! -h "$file" ] && [ -f "$file" ]`, so `-h` (lstat, never follows) short-circuits the chain before any dereference. Verified against a symlink to a nonexistent target: the reordered check skips it cleanly with no stat error. ## The symlink tests could pass even if the target were opened Both new tests only asserted the final result (an empty target list, a False return), which a bug that actually opened the symlink target could still produce coincidentally. Patched `Path.open` to raise if called during either test, and changed the secret fixture to a plain shell shebang (a "TOP SECRET" first line before it would have looked like a non-match too, masking the same class of bug). A regression that reintroduces the dereference now fails the test directly instead of relying on the target's content happening to not match. ## A predictable temp path could collide across parallel runs Both symlink tests wrote their secret file to `self.root.parent`, which collapses to the shared system temp root (`self.root` is itself the leaf of a unique per-test `TemporaryDirectory`), not a per-test unique location. `setUp` now creates a second, separate `TemporaryDirectory` (`self.outside`) for this purpose. ## Declined: symlink creation isn't guarded for Windows without dev mode Matches this repo's own established, unguarded precedent exactly: `test_build_dist.py` (4 call sites), `test_carry.py` (2 call sites), and `test_skills_install.py` (2 call sites) all call `Path.symlink_to` directly with no `try/except OSError`/`skipTest` guard. Singling out these 2 new tests would be inconsistent with the other 8 already in the suite; if this is a real gap, it is a fleet-wide one, not specific to this PR. ## Also `scripts/docker_lint.py`: reflowed `has_shell_shebang`'s docstring to one sentence per line (a real comment-wrap violation from the prior fix, a docstring gap `prose_lint.py` doesn't scan but the human style rule still applies). ## Verified Full test suite (797 tests), ruff, mypy, actionlint, `repo_gate.py`, `prose_lint.py --diff origin/develop`, and the complete `docker_lint.py` run (all 7 linters) all pass clean. Confirmed the reordered symlink check short-circuits cleanly against a symlink to a nonexistent target. --- .github/workflows/validate-task.yml | 4 ++-- scripts/docker_lint.py | 3 +-- scripts/tests/test_docker_lint.py | 14 +++++++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 196a09da..0a1ded2b 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -115,8 +115,8 @@ jobs: mapfile -d '' -t candidates < <(git ls-files -z) for file in "${candidates[@]}"; do base="${file##*/}" - # Never follows a tracked symlink: -h checks the git-tracked path itself, before any read. - if [[ "$base" != *.* ]] && [ -f "$file" ] && [ ! -h "$file" ]; then + # Never follows a tracked symlink: -h short-circuits before -f's own dereferencing stat. + if [[ "$base" != *.* ]] && [ ! -h "$file" ] && [ -f "$file" ]; then # `head` reads a no-trailing-newline file cleanly and still fails on a genuine read error. first_line="$(head -n 1 -- "$file")" if is_shell_shebang "$first_line"; then diff --git a/scripts/docker_lint.py b/scripts/docker_lint.py index 7db39d5a..f1f9d1fa 100755 --- a/scripts/docker_lint.py +++ b/scripts/docker_lint.py @@ -196,8 +196,7 @@ def shell_shebang_interpreter(line: str) -> str | None: def has_shell_shebang(root: Path, relative_path: str) -> bool: """Report whether a tracked file's shebang directly names bash or sh. - Never follows a tracked symlink: `is_symlink()` uses `lstat`, so checking it first - keeps a symlink pointing outside the checkout from ever reaching `open()`. + Never follows a tracked symlink: `is_symlink()` uses `lstat`, keeping the target unreached. """ path = root / relative_path try: diff --git a/scripts/tests/test_docker_lint.py b/scripts/tests/test_docker_lint.py index dc4e6364..065d020b 100755 --- a/scripts/tests/test_docker_lint.py +++ b/scripts/tests/test_docker_lint.py @@ -44,6 +44,8 @@ class DockerLintCase(unittest.TestCase): def setUp(self) -> None: self.root = Path(self.enterContext(tempfile.TemporaryDirectory())) subprocess.run(["git", "init", "-q", str(self.root)], check=True) + # A separate temp dir, not self.root's own parent, which is the shared system temp root. + self.outside = Path(self.enterContext(tempfile.TemporaryDirectory())) def track(self, name: str, body: str = "content\n") -> None: path = self.root / name @@ -164,17 +166,19 @@ def test_extensionless_shebang_script_merges_with_glob_matched_scripts(self) -> ) def test_extensionless_symlink_is_never_followed(self) -> None: - secret = self.root.parent / "secret" - secret.write_text("TOP SECRET\n#!/usr/bin/env bash\n", encoding="utf-8") + secret = self.outside / "secret" + secret.write_text("#!/usr/bin/env bash\n", encoding="utf-8") self.track_symlink("ops/evil-symlink", secret) linter = next(linter for linter in docker_lint.LINTERS if linter.name == "shellcheck") - self.assertEqual([], docker_lint.tracked_files(self.root, linter)) + with mock.patch.object(Path, "open", side_effect=AssertionError("symlink target opened")): + self.assertEqual([], docker_lint.tracked_files(self.root, linter)) def test_has_shell_shebang_reports_false_for_a_symlink_without_reading_it(self) -> None: - secret = self.root.parent / "secret" + secret = self.outside / "secret" secret.write_text("#!/usr/bin/env bash\n", encoding="utf-8") self.track_symlink("ops/evil-symlink", secret) - self.assertFalse(docker_lint.has_shell_shebang(self.root, "ops/evil-symlink")) + with mock.patch.object(Path, "open", side_effect=AssertionError("symlink target opened")): + self.assertFalse(docker_lint.has_shell_shebang(self.root, "ops/evil-symlink")) def test_extensionless_untracked_shebang_script_is_not_picked_up(self) -> None: path = self.root / "ops" / "vps-backup-pull"