Skip to content

Fall back to an NTFS junction when the symlink escape test can't create a symlink #3609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maxisbey merged 1 commit into from
Oct 1, 2026
Merged

Fall back to an NTFS junction when the symlink escape test can't create a symlink #3609

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion tests/shared/test_path_security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests for filesystem path safety primitives."""

import sys
from pathlib import Path

import pytest
Expand Down Expand Up @@ -142,7 +143,14 @@ def test_safe_join_rejects_symlink_escape(tmp_path: Path):
outside.mkdir()
sandbox = tmp_path / "sandbox"
sandbox.mkdir()
(sandbox / "escape").symlink_to(outside)
try:
(sandbox / "escape").symlink_to(outside)
except OSError as exc: # pragma: lax no cover
if sys.platform != "win32" or exc.winerror != 1314: # ERROR_PRIVILEGE_NOT_HELD; a junction needs no privilege

@claude claude Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): AGENTS.md says to avoid adding new coverage pragmas, and documents # pragma: lax no cover as for "lines covered on some platforms/versions but not others". The diff adds # pragma: lax no cover on the except OSError arm, which the PR description says no CI matrix entry executes (hosted Windows runners can create symlinks), so it is not covered on any platform CI runs. Fix: either restructure so the fallback is exercised by a test on all platforms (e.g. a small helper with symlink_to patched to raise OSError(winerror=1314) and a stub _winapi in sys.modules), or, if a pragma must stay, use the form whose documented meaning matches the line.

Why this was flagged

Nothing fails at runtime. The instruction guards the 100% branch-coverage gate from accumulating excluded lines; here the excluded block contains real logic (the winerror check and the CreateJunction call) that the PR itself says has not been run on a real unprivileged Windows machine, and the pragma also hides the if ... raise branch arcs. The author reports exercising the control flow manually on Linux via patching, but that check is not committed as a test. Small consequence: a future regression in the fallback (wrong argument order to CreateJunction, AttributeError on winerror) would go unnoticed by CI.

Verification: AGENTS.md (base d639cf7) "Coverage" section: "Avoid adding new # pragma: no cover, # type: ignore, or # noqa comments." with the audit command git diff origin/main... | grep -E '^\+.*(pragma|type: ignore|noqa)', and it documents # pragma: lax no cover as the form to "Use for lines covered on some platforms/versions but not others."

raise
import _winapi

@claude claude Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers get an inline import _winapi inside the except arm at tests/shared/test_path_security.py:151, which the repo's import rule asks to keep at file top. The rule allows an inline import only when a top-level one genuinely cannot work; _winapi is Windows-only, but a top-level if sys.platform == "win32": import _winapi (the pattern already used in src/mcp/os/win32/utilities.py:19) does work on every platform. Fix: hoist the import into a platform-guarded top-level block (with # pragma: lax no cover if needed for the Linux cells) and keep the except arm to the CreateJunction call, or state why the guarded top-level form is unsuitable here. [also at: tests/shared/test_path_security.py:151 - nit: AGENTS.md requires all imports at the top of the file, with inline imports allowed only when a top-level import genuinely can't work.]

Why this was flagged

The diff adds import _winapi at tests/shared/test_path_security.py:151, inside the except OSError arm that only runs on Windows machines that cannot create symlinks. AGENTS.md says all imports go at the top of the file, with the sole exception of a top-level import that genuinely cannot work. An unconditional top-level import _winapi fails on Linux, but a sys.platform == "win32" guarded top-level import (as in src/mcp/os/win32/utilities.py:19) works everywhere, so the exception is at best borderline here. Nothing fails at runtime: on Linux the sys.platform != "win32" check at line 149 re-raises before the import is reached, and on Windows the module exists. The cost is the one the rule names: the Windows-only dependency on a private CPython module is hidden inside a branch instead of being visible in the file's import block.

Verification: nit. Rule in AGENTS.md:56-59. The diff adds import _winapi inside the except OSError arm at tests/shared/test_path_security.py:151. The repository's own established pattern at src/mcp/os/win32/utilities.py:19-25 shows a platform-guarded top-level import works on every platform. Nothing fails at runtime; this is purely the written import convention not being followed.


_winapi.CreateJunction(str(outside), str(sandbox / "escape"))

with pytest.raises(PathEscapeError, match="escapes base"):

@claude claude Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: Maintainers keep a test that pins SDK-authored error text via match="escapes base" at tests/shared/test_path_security.py:155, which the test-quality skill forbids. The PR touches this very test (lines 146-153) and now adds a junction arm whose only assertion is this message match, so a wording change in src/mcp/shared/path_security.py:174 would fail the test for a non-behavioural reason. Fix: assert the raise via pytest.raises(PathEscapeError) and check .code or snapshot the message per the skill, covering both this test and the sibling match="null byte" tests at lines 131/136.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

This is the only ledger entry, so it is nominated by requirement; the dismissal's pre_existing claim is accurate (base commit has the same match="escapes base" at the same line). The trigger is any rewording of the f-string at src/mcp/shared/path_security.py:174 (f"Path {target} escapes base {base_resolved}"); the test at tests/shared/test_path_security.py:155 then fails on text rather than behaviour. The PR modifies the body of this test (lines 146-153) and routes a new junction arm into the same assertion, so the author was editing the test and the skill's rule (.claude/skills/test-quality/SKILL.md:60: never match= on message text) applies to the touched test. Base branch behaves identically; the consequence is test fragility, not a production defect. Remedy: drop the match= and assert the error type or .code.

Verification: pre-existing. Triggering condition: any rewording of the f-string at src/mcp/shared/path_security.py:174 makes tests/shared/test_path_security.py:155 fail on text rather than behaviour. However, the violating line is NOT introduced by this PR: git show d639cf7f:tests/shared/test_path_security.py has the identical match="escapes base" at line 147.

safe_join(sandbox, "escape", "secret.txt")
Expand Down
Loading