ci-secure: don't follow a workflow symlink outside .github/workflows/ (issue #37)

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Glob workflow entries] --> B{Workflow directory resolves to literal in-repo boundary?}
  B -- No --> C[Record entries as not scanned]
  B -- Yes --> D{Entry resolves successfully inside boundary?}
  D -- No --> C
  D -- Yes --> E[Admit resolved workflow file]
  C --> F[Coverage notes or zero-workflow refusal]
  E --> G[Scan workflow]

_{Reviews (2): Last reviewed commit: "ci-secure: close the symlinked-directory..." | Re-trigger Greptile}

@claude review

Claude finished @yonasb's task in 4m 19s —— View job

Review: PR #74 — ci-secure symlink containment

• Gather context (diff vs 64cc1db, read scan.py)
• Analyze the containment logic for bypasses
• Review tests + changelog
• Post review
• Run the test suite — blocked in this environment (see note at the end)

Verdict

The core fix is correct and the security property holds. I tried to break the containment check and couldn't. Three design choices in particular are right, and each is the non-obvious one:

1. The boundary is not self-referential. wf_dir_expected = root.resolve() / ".github" / "workflows" (scan.py:390) is built by joining literal components, then compared against wf_dir_on_disk.resolve(). Comparing "resolved match" against "resolved .github/workflows" would have been trivially true for every escape. This is the whole reason the greptile-reported ancestor bypass is actually closed and not just papered over.
2. The check runs on every match, not only is_symlink() matches. Correct — glob.glob dereferences a symlinked ancestor, so those entries arrive as plain regular files. Gating on the leaf's symlink-ness is exactly the bug that was there in c79425f.
3. _repo_relative resolves the parent but not the leaf (scan.py:916-917). Subtle and important: without it the report re-follows the symlink at render time and prints the target's name in the very note that exists to avoid naming it. Good catch during testing.

Order of operations in discover_workflow_files is also right: dir-trust → resolve() (guarded) → exists() → relative_to() → is_file(). And _DROPPED_MATCHES.clear() at scan.py:5238 precedes all_workflow_files() at 5241, so discovery-time records survive into the report.

Tests are good — test_symlinked_workflows_directory_does_not_bypass_containment asserting an exact n["file"] == rather than .endswith("evil.yml"), with the comment explaining why, is the kind of test that will still be doing its job in a year.

Below are four things I'd like addressed. None of them re-opens the disclosure hole; #1 is the one I'd actually fix before merge.

1. The symlinked-directory branch's coverage note never reaches the user (medium)

When .github/ or .github/workflows/ is itself a symlink, every match is dropped, so workflow_files is empty and scan() raises the zero-workflows CoverageError at scan.py:5254 — before the loop at scan.py:5565 that turns _DROPPED_MATCHES into coverage_notes and emits the logger.warning. main() catches it and prints only the message (scan.py:5798-5800).

So the carefully-worded reason recorded at scan.py:412-417 — "resolves outside .github/workflows/ (.github/ or .github/workflows/ itself is a symlink…)" — is unreachable end-to-end. What an operator actually sees is:

ERROR: coverage failure — no workflow files found under /repo/.github/workflows —
there is nothing to scan, so every check would pass vacuously.

…for a directory that visibly contains YAML. That's the confusing-to-the-point-of-being-misleading case, and it's the one a maintainer will hit. Nothing in the suite pins the user-facing diagnostic either: the end-to-end test asserts on that generic string, and the unit test reaches into scan._DROPPED_MATCHES directly.

Suggested fix: when the zero-workflow refusal fires and there are _KIND_NOT_SCANNED drops, append a count and reason to the CoverageError message — never the target paths, which keeps the non-disclosure property intact. Something like …nothing to scan (N entry/entries excluded: resolved outside .github/workflows/).

Fix this →

2. A dangling symlink is reported twice, once with the wrong reason (low)

_workflow_files_absent_from_tree lists .github/workflows from git ls-tree and tests (root / rel).is_file() (scan.py:1489) — which follows symlinks. A committed dangling symlink is in the tree but is_file() is False, so it lands in absent and renders as:

present in the audited commit but absent from the scanned working tree (partial or sparse checkout) — never read

…alongside the new and correct coverage_notes entry "symlink target does not exist". Two entries for one file, one of them attributing the cause to a sparse checkout that didn't happen.

The PR's test_dangling_symlinked_workflow_is_skipped_with_a_coverage_note can't catch this because tmp_path isn't a git repo, so git ls-tree returns non-zero and the probe short-circuits to []. Worth a git init + git add variant of that fixture.

Fix this →

3. _undiscovered_workflows has an undocumented ordering dependency (low, latent)

The skipped set at scan.py:478-482 filters _DROPPED_MATCHES on kind == _KIND_NOT_SCANNED and parent-resolves-to-wf_dir, keyed by basename. That's correct only because the tripwire runs at scan.py:5242, immediately after discovery and before any detector runs.

But P14.24's _gap() helper also appends _KIND_NOT_SCANNED entries whose "file" is a workflow file in .github/workflows/ (scan.py:4178-4184). If the tripwire ever moves after the detector loop — or another pre-discovery producer appears — a P14.24 gap on build.yml would silently suppress a genuine "discovery lost build.yml" miss, disarming the exact tripwire that exists to stop a broken scan rendering as a clean one.

The docstring explains why the exclusion exists but not that its safety rests on call ordering. Either add that to the docstring, or make it structural — a dedicated list for discovery-time skips, or a sentinel field on the record rather than sharing _KIND_NOT_SCANNED.

4. discover_workflow_files still takes globs but hardcodes the boundary (low, contract nit)

The signature says "resolve globs under root", but the function now unconditionally requires every match to land under root/.github/workflows. Pass it [".github/actions/**/action.yml"] and it drops 100% of matches and files a coverage gap for each. Harmless today — all_workflow_files is the only caller — but the parameter now advertises generality the body no longer has. Consider taking the boundary as an explicit argument, or dropping globs in favour of the constant list.

Nit: the _repo_relative docstring says the resolved target is never surfaced, but p.parent.resolve() does follow a symlinked .github/workflows. If that symlink pointed to an in-repo directory, relative_to would succeed and print the target path. Unreachable in practice (that shape always refuses at #1's CoverageError), so it's a docstring accuracy point, not a leak.

Repo conventions

✅ Changelog entry present, dated UTC, under Fixed, and it documents all three rounds honestly including the bypass found in review. ✅ No maintainer infra added under skills/. ✅ Stdlib only.

On the test suite

I could not run python3 -m pytest — the command required approval that wasn't granted in this environment, so I reviewed statically and did not independently verify the suite passes. The PR description reports it green. If you want me to run it on a future invocation, Bash(python3 -m pytest*) needs to be in --allowedTools.

• branch Issue-37-symlined-disclose-file