fix(security): path containment checks for Windows environments

[Gracy769]

• Fixed an issue in extract_path_candidates where shlex.split(posix=True) would strip backslashes from Windows paths, mangling UNC paths (e.g. \server\share) before they could be evaluated by _is_windows_absolute.
• Fixed a bypass in validate_path where Windows absolute paths bypassed glob expansion and symlink resolution. On Windows, they now fall through to the standard Path logic, allowing glob expansion and strict resolution while still properly checking containment.

Summary

• TBD

Anti-slop triage

• Classification:
• Evidence:
• Non-destructive review result:

Verification

• Targeted tests/docs checks ran, or the gap is explicitly recorded.
• git diff --check passes.
• No live secrets, tokens, private logs, or unrelated generated churn are included.

Resolution gate

• If this PR resolves an issue, the issue number and fix evidence are linked.
• If this PR should not merge, the rejection/defer rationale is evidence-backed and does not rely on vibes.
• I did not merge/close remote PRs or issues from an automation lane without owner approval.

[1716775457damn]

Confirmed real issue: shlex.split(posix=True) indeed strips backslashes and mangles UNC paths on Windows. The fall-through for validate_path on Windows to standard Path logic is the right call, good fix. One suggestion: adding a test that feeds a UNC path (e.g. \server\share\foo) through extract_path_candidates and validate_path would prevent regression, since this path-normalization bug is easy to reintroduce.

[1716775457damn]

Nice — adding the UNC path regression test (2477cf2) covers exactly the scenario I was worried about, so this fix now has proper guardrails against reintroduction. The Windows path fall-through to standard Path logic is solid. Looks ready to merge once checks pass.

[1716775457damn]

Confirmed both bugs, and the second one is the more serious of the two.

On the validate_path bypass: skipping symlink resolution for Windows absolute paths means a path that looks contained can still resolve outside the allowed root. A symlink sitting inside the permitted directory but pointing at something outside it (e.g. a junction to C:\Users\someone-else) passes a purely lexical containment check, then lands outside after resolution — the check and the subsequent open() disagree about which file is actually being accessed. Routing these through the standard Path logic with resolve(strict=True) closes the gap, because resolution now happens before the comparison.

On the shlex.split(posix=True) fix: mangling \\server\share into servershare is more than cosmetic — _is_windows_absolute then returns false for a genuinely absolute UNC path, so it gets treated as relative and joined against the CWD. That silently turns a network path into a local one, which is a different file than the caller asked for.

Worth adding a regression test that drops a symlink escaping the allowed root and asserts it's rejected — that's the case most likely to silently regress if the fast-path is ever reintroduced.

[1716775457damn]

Thanks for adding test_windows_absolute_symlink_escape_is_denied (3ceb78a) — the absolute-path flavor of the symlink escape is the variant most likely to silently regress, so this closes the gap I flagged earlier.

One thing worth calling out: the winerror == 1314 skip is the correct guard, but it means this test silently no-ops on any runner without SeCreateSymbolicLinkPrivilege (non-elevated Windows accounts, most default CI images). If the Windows job isn't elevated or in Developer Mode, the new assertion never actually executes there and a regression would look like a green build. Consider either documenting the privilege requirement for the Windows job, or emitting a visible marker when the skip fires so it isn't confused with a pass.

Other than that this looks good to me — the resolve(strict=True) before containment comparison is the right ordering.

[Gracy769]

Addressed the unprivileged Windows runner feedback in commit 38f8cfa:

• Added _create_directory_link helper that falls back to unprivileged NTFS directory junctions (_winapi.CreateJunction) when SeCreateSymbolicLinkPrivilege is missing on non-elevated Windows CI runners. Both est_issue_3007_symlink_escape_is_denied and est_windows_absolute_symlink_escape_is_denied now execute their full assertion path with 0 skips.
• Added est_symlink_resolution_escape_mocked to verify the resolution-before-containment check deterministically in any environment.

All 12 security test cases pass cleanly.

[1716775457damn]

Good — the junction fallback is the right fix for the skip problem. A skipped assertion is worse than a failing one, because it reports green on the exact case the test exists to catch, so getting test_issue_3007_symlink_escape_is_denied and test_windows_absolute_symlink_escape_is_denied down to 0 skips is what actually makes the containment check trustworthy on non-elevated runners.

One boundary worth flagging: NTFS junctions can only target local directories and cannot point at UNC/remote paths, so the _create_directory_link fallback can't cover a link that resolves to a network location. That remains the variant most likely to slip through if a fast-path is ever reintroduced. test_symlink_resolution_escape_mocked covers the logic deterministically, but a short comment in the helper noting the junction limitation would make the gap discoverable to whoever touches this next.

Remaining blocker on my side: Checks is still at 0 — one workflow is awaiting maintainer approval, so CI hasn't actually run against 38f8cfa. Once that's approved and green, this is good to merge.

[Copilot]

🟡 Changes recommended

src/path_scope.py introduces a whitespace-only line (trailing whitespace) that will fail git diff --check and should be cleaned up before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens the Python port’s workspace path containment logic for Windows-style paths and adds regression tests to cover Windows symlink/junction escapes and UNC path tokenization.

Changes:

• Adjust WorkspacePathScope.validate_path() so Windows absolute paths on Windows go through the standard glob + Path.resolve() containment logic (instead of the PureWindowsPath-only branch).
• Update extract_path_candidates() to prioritize raw token splitting so UNC paths (e.g. \\server\share\...) aren’t mangled by shlex.split(posix=True).
• Expand the security scope test suite to cover Windows symlink/junction scenarios and mocked resolution escapes (including UNC).

File summaries

File	Description
tests/test_security_scope.py	Adds Windows-focused regression tests (junction fallback, absolute-path symlink escape, UNC preservation, mocked resolve escapes).
src/path_scope.py	Updates Windows absolute path handling and candidate extraction ordering to prevent Windows/UNC path validation bypasses.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[1716775457damn]

2b271e1 clears the trailing-whitespace line Copilot flagged, so git diff --check should be clean again — worth confirming locally before merge since it was the only thing that review caught.

d5efc4b addresses the other item I raised: documenting the NTFS junction limitation in the helper plus the mocked UNC link case means the junction-can't-target-UNC gap is now discoverable in-tree instead of living in this thread.

From my side the code is settled. The remaining gate is unchanged: one workflow is still awaiting maintainer approval, so Checks is at 0 and 38f8cfa / 2b271e1 have not actually run in CI. The junction fallback and the 0-skip symlink assertions are exactly the kind of thing that only proves itself on a real Windows runner, so I'd hold merge until that workflow is approved and green rather than merging on review alone.

[1716775457damn]

Following up on the junction limitation I raised earlier: the other edge worth pinning down is the failure mode of resolve(strict=True). For a path that does not exist yet, or a dangling link, resolve(strict=True) raises OSError/FileNotFoundError instead of returning something to compare, so the containment decision then depends entirely on how that exception is handled. That should be an explicit deny rather than a fall back to the lexical check, otherwise a non-resolvable path effectively skips the resolution step this PR just added, which is the same bypass in a different shape. A test asserting that a dangling or unresolvable path is rejected rather than passed through or crashing would make that guarantee explicit.

[1716775457damn]

Two notes on c3ed607:

1. require_bash() returns False unconditionally when os.name == 'nt', so both pre-push hook contract tests never execute on a Windows runner. That's the same shape as the skip problem we just fixed for the symlink tests — the assertions report green without running, so a broken hook contract would only surface on Linux/macOS. Git for Windows ships bash.exe, so matching shutil.which('bash') first and only falling back to False (or additionally probing C:\Program Files\Git\bin\bash.exe) would let these run on Windows where they actually can. If turning them off on Windows is deliberate, calling that out in the commit message / CI docs would help it not read as a pass.

2. The resolve(strict=True) point from my last comment still looks unaddressed: for a not-yet-created path or a dangling link it raises OSError/FileNotFoundError rather than returning a comparable path, so the containment decision falls to whatever catches it. Unless that exception is turned into an explicit deny, a non-resolvable path still bypasses the resolution step this PR adds. Worth confirming whether that's planned here or as a follow-up.

Everything else on the path-scope change looks right to me.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[1716775457damn]

31a037f closes both items from my last review.

On validate_path: wrapping resolve() in try/except and returning an explicit PathScopeDecision(False, ...) is the right shape — the failure now denies instead of falling through to whatever the caller does with the exception. Worth noting that strict=False is the correct choice here: a not-yet-created path that sits inside a root should stay allowed (that is the normal create-a-file case), and strict=False only surfaces the cases that genuinely cannot be resolved — ELOOP from a symlink cycle, invalid characters or >MAX_PATH on Windows, permission errors — which are exactly the ones that were silently bypassing the check. Catching (OSError, ValueError, RuntimeError) covers all three of those shapes.

On the Git bash detection: probing the known Git for Windows locations before shutil.which('bash'), then filtering out WindowsApps, fixes the root cause — on Windows 'bash' usually resolves to the WSL App Execution Alias stub, which exists but fails to run. Going one step further and actually executing bash -c 'echo 1' with a timeout turns require_bash() into an execution check rather than an existence check, which is what stops these tests from reporting green on a runner where bash is present but broken.

One minor note: require_bash() now shells out at collection time (decorator evaluation), so an environment without bash pays the 2s timeout once per test module. Not a problem in practice, just worth knowing if collection ever looks slow.

Everything else on the path-scope change looks right to me. Remaining gate is unchanged: Checks is still 0 with a workflow awaiting maintainer approval, so 31a037f has not actually run in CI. I would still hold merge until the Windows job is approved and green, since both of these fixes are Windows-specific and only prove themselves on a real Windows runner.

[1716775457damn]

Status check on this one: still Open, Checks is 0, and the workflow is awaiting maintainer approval, so nothing from 31a037f has actually executed on a runner yet. I'm keeping my hold on merge — the junction fallback and the now-0-skip symlink assertions are exactly the kind of change that only proves itself on a real Windows runner, and approving on review alone is how the original bypass slipped through in the first place.

Small follow-up to the require_bash() note I raised: instead of paying the 2s probe at collection time (decorator evaluation, once per test module), caching the probe result — e.g. functools.lru_cache around the execution check, or moving it to a session-scoped fixture — would keep the "bash actually runs, not just exists" guarantee while making the cost once per session rather than once per module. Worth doing only if collection time ever becomes noticeable; the correctness of the current version is fine.

Everything else on the path-scope change looks right to me. I'll re-review once the Windows workflow is approved and green.

[1716775457damn]

Reviewed c1f2471.

The lru_cache(maxsize=1) placement on get_bash_executable() is the right target: that's the function paying the 2s bash -c 'echo 1' probe, so caching it removes the per-test cost without giving up the "bash actually runs, not just exists" guarantee that 31a037f added.

Two things worth weighing before merge:

1. The cache is per-module, not per-session. test_pre_push_hook_contract.py and test_roadmap_helpers.py each define their own copy of get_bash_executable, so each module keeps a separate cache entry — on a runner without bash that's still ~2s per module rather than once for the whole run. Moving the helper into a shared module (e.g. tests/conftest.py or tests/_bash.py) and importing it in both would make it genuinely once-per-session, which is what my earlier note was pointing at.

2. lru_cache interacts badly with monkeypatch. If any test patches PATH or shutil.which and expects require_bash() to re-evaluate, the cached value now wins and the test silently asserts against the pre-patch result. Worth either clearing both caches in an autouse fixture (get_bash_executable.cache_clear() / require_bash.cache_clear()) or documenting that these two functions must not be monkeypatched around.

The import cleanup in test_roadmap_helpers.py (hoisting os to module scope) is fine.

Merge hold unchanged: Checks is still 0 with a workflow awaiting maintainer approval, so c1f2471 has not run on a Windows runner yet — and the bash probe is exactly the kind of change that only proves itself there, since the Git-for-Windows path probing and the junction fallback are both Windows-specific.

[1716775457damn]

Reviewed cabcdc0 — moving get_bash_executable / require_bash into tests/_bash.py is what makes the cache genuinely once-per-session instead of once-per-module, so the 2s bash -c 'echo 1' probe is now paid a single time for the whole run. That was the actual ask, so this resolves it.

Two things I'd still change before merge, both about making the guarantee enforced rather than documented:

1. The monkeypatch hazard is currently only a comment. A comment is a convention that survives exactly as long as nobody reads past it, and the failure mode is silent — a test that patches PATH or shutil.which will assert against the pre-patch cached value and go green for the wrong reason. Six lines in tests/conftest.py turn it into something that can't be forgotten:

@pytest.fixture(autouse=True)
def _clear_bash_cache():
    get_bash_executable.cache_clear()
    require_bash.cache_clear()
    yield
    get_bash_executable.cache_clear()
    require_bash.cache_clear()

2. from tests._bash import ... only resolves when pytest is invoked from the repo root with the default prepend import mode (tests gets picked up as an implicit namespace package). Running cd tests && pytest, or --import-mode=importlib, turns that into ModuleNotFoundError and takes the whole hook-contract module down with it. Either add tests/__init__.py, or import as from _bash import ... and let conftest.py keep the directory on sys.path.

One behavioral gap worth noting: the Windows branch validates candidates with os.path.exists only, so a Git-for-Windows install that exists but is broken yields require_bash() == False and every @skipUnless(require_bash(), ...) test skips — which is the same "green without running" shape we just spent several commits removing from the symlink tests. Running the exec probe inside get_bash_executable (still cached) would let you distinguish "no bash" from "bash present but unusable" and report that in the skip reason instead of hiding it.

Merge hold unchanged: Checks is still 0 with workflows awaiting maintainer approval, so nothing in cabcdc0 has run on a Windows runner yet — and per-session caching plus the Git-for-Windows path probing are precisely the parts that only prove themselves there.

[1716775457damn]

Reviewed 1b6a32b. Collapsing the probe into _check_bash_state and having get_bash_executable() / bash_skip_reason() read from that one cached tuple is the right consolidation: one lru_cache entry, one subprocess probe, and the skip message can now say "bash present at X but unusable" instead of a bare "Requires bash". That is exactly the distinction I was asking for, and it also removes the odd shape where require_bash() paid the probe a second time. Two things I would change before merge: 1) The Windows loop gives up too early. _probe_bash(candidate) runs inside for candidate in (...) and returns None on the first candidate that exists but fails the probe, so a broken C:\Program Files\Git\bin\bash.exe means %LOCALAPPDATA%\Programs\Git\bin\bash.exe is never tried even when it is usable. Continuing to the next candidate and keeping the accumulated reasons preserves the better skip message without reporting "no bash" on a machine that actually has a working bash — and a broken-but-present Git for Windows is exactly the machine this message exists to describe. 2) The autouse pytest fixture in tests/conftest.py will not actually run for these tests. pytest does not inject fixtures into unittest.TestCase methods, autouse included — only setUp/tearDown execute. Both consumers (PrePushHookContractTests, RoadmapHelperTests) are unittest.TestCase subclasses, so _check_bash_state.cache_clear() never fires around them and the monkeypatch hazard is back in place. Moving it into unittest setUp/tearDown, or calling cache_clear() from those classes, would make it real. Still open from my last review: tests/_bash.py is imported as from tests._bash import ..., which only resolves under prepend import mode from the repo root; adding tests/init.py would make cd tests && pytest work as well. Merge hold unchanged — Checks is still 0 with workflows awaiting maintainer approval, so none of the bash-detection behavior has executed on a Windows runner yet, and that is the only environment where any of this branch is actually live.

[1716775457damn]

Thanks for taking this on — both fixes line up with what I'd expect.

1. shlex.split(posix=True) stripping backslashes is real: \\server\share\dir gets collapsed before _is_windows_absolute ever sees it, so UNC paths were effectively unvalidated. Switching to non-POSIX splitting for the candidate extraction is the right call.
2. The bypass is confirmed as well — Windows absolute paths skipping glob expansion and symlink resolution meant a symlink pointing outside the workspace would never be resolved, so containment was checked against the unresolved path. Falling through to the standard Path logic closes that.

Suggestion: add regression tests for the two cases most likely to silently regress — an escaped UNC path (\\\\server\\share\\dir) and a symlink inside the workspace that resolves outside it — plus a case where the path is a drive-relative form like C:foo, which is neither absolute nor relative in the usual sense.

[1716775457damn]

Reviewed the diff in src/path_scope.py. The bypass looks real and the fix is the right shape: previously a Windows-absolute candidate short-circuited into _validate_windows_path before glob expansion and symlink resolution, so containment was never evaluated against the resolved path. Letting it fall through to the shared Path logic on os.name == "nt" (while keeping the old branch on POSIX) closes that without changing non-Windows behavior.

Two details I appreciate / want to confirm:

1. Wrapping Path.resolve() in try/except (OSError, ValueError, RuntimeError) and returning an explicit deny decision is important on Windows — invalid drive letters, over-long paths and some UNC forms raise instead of returning. Failing closed here is correct.
2. In extract_path_candidates, iterating raw_tokens before the shlex-split tokens is what preserves \\server\share. Does changing the order risk double-counting or precedence surprises for quoted tokens that legitimately contain escapes, or are those still normalized downstream by _strip_redirection_operator?

If there is a test in tests/test_security_scope.py covering UNC containment end-to-end (\\server\share\... both inside and outside the roots), that would make this easy to sign off on.

[Gracy769]

Thanks for reviewing the core logic!

To answer your question about extract_path_candidates: we avoid double-counting with the if candidate not in candidates: check. By evaluating both the raw tokens and the shlex-split tokens, it actually acts as a fail-safe — if the shell parser mutilates an unquoted UNC path into an absolute escape, the whole payload just gets safely denied.

I just pushed d8b7169 to swap the basic UNC test for a full end-to-end one (test_unc_paths_are_evaluated_correctly_inside_and_outside). It proves UNC paths get denied when outside the root, but are correctly allowed if the workspace root is itself a UNC share.

Let me know if you need anything else to sign off!

[1716775457damn]

Thanks — the fail-safe reasoning makes sense: keeping both the raw token and the shlex-split token does cover the unquoted UNC case where the shell parser mangles the backslashes. Two things worth nailing down before merge, since they're the exact gaps the old short-circuit left open: (1) a unit test asserting an unquoted \server\share\file.txt argument survives into candidates unchanged — that's the regression shlex.split(posix=True) introduced and the cheapest way to keep it from coming back; (2) explicit coverage for the resolved-path direction: a symlink inside the workspace that points outside it, to confirm containment is now evaluated post-realpath on Windows too.

[Gracy769]

Pushed 87a64b5 to add the unquoted \\server\share\file.txt test to test_unc_paths_are_evaluated_correctly_inside_and_outside.

Regarding point 2, that's actually already in the PR! Check out test_windows_absolute_symlink_escape_is_denied — it sets up a symlink inside the workspace pointing outside, and passes it as a Windows absolute path (payload = f'cat {link}/secret.txt'). Under the old short-circuit, that bypassed resolve() and was allowed; now it falls through to the Path logic and is correctly denied.