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.