test_safe_join_rejects_symlink_escape fails on Windows without elevation or Developer Mode #3408

Activity

1. claude

   added

   v2Affects the v2 line (2.x on main)Affects the v2 line (2.x on main)

   on Aug 28, 2026
2. hhhhzzzj commented on Aug 30, 2026

   

   hhhhzzzj

   on Aug 30, 2026

   More actions

   I reproduced this on the current main revision (d2290ca) in a Windows environment without symlink privileges. In tests/shared/test_path_security.py:140-148, the unconditional symlink_to() at line 145 raises OSError [WinError 1314] before safe_join() is reached, so the test fails during setup rather than checking the escape case.

   The smallest fix looks like keeping the existing PathEscapeError assertion and wrapping only symlink_to() in try/except OSError, calling pytest.skip() when the platform does not permit symlink creation. That keeps the security assertion active on runners with symlink support and gives ordinary Windows checkouts a clean baseline. The runtime implementation in src/mcp/shared/path_security.py:121-178 does not need to change.

   One detail I would leave to your preference: should the skip cover any OSError from symlink creation, or only the privilege/unsupported-symlink cases? If this one-file test scope is right, I can prepare the focused PR after the issue is assigned.
3. iam-kira commented on Aug 30, 2026

   

   iam-kira

   on Aug 30, 2026

   Author

   More actions

   Thanks for reproducing it independently — good to have it confirmed at a
   specific revision, and your read of the scope matches mine: test-only, and
   src/mcp/shared/path_security.py doesn't need to change.

   On your question — catch broad OSError, not the specific privilege cases.
   Two reasons:

   1. WinError 1314 surfaces as a plain OSError. Python maps some Windows error
      codes onto subclasses (FileExistsError, PermissionError), but 1314 isn't in
      that table, so there is no narrow type to catch. You would have to match on
      .winerror, which is Windows-only and brittle.

   2. Narrow catching is exactly how this bug class survives. Test suite is unrunnable on Windows without symlink privilege: utils_temp_dir errors pypa/pipx#2021 is
      the same failure caused by suppress(FileExistsError) — it looks like a
      guard, and WinError 1314 walks straight through it. Their entire test suite
      errors on Windows as a result.

   The precedent I'd point at is psf/black, which uses
   except (OSError, NotImplementedError) for its symlink tests — NotImplementedError
   covers platforms where os.symlink exists but is unimplemented.

   So:

   try:
       (sandbox / "escape").symlink_to(outside)
   except OSError as exc:
       pytest.skip(f"symlink creation is not permitted here: {exc}")
   

   I have this written and verified locally — the suite goes from 5791 passed /
   1 failed to 5790 passed / 0 failed on Windows, and the test skips for the right
   reason rather than passing vacuously. Happy to open the PR once a maintainer
   assigns it, or to leave it to you if you'd rather take it.
4. tonydzi commented on Sep 1, 2026

   

   tonydzi

   on Sep 1, 2026 · Hidden as spam

   show commentMore actions
5. modelcontextprotocol

   blocked tonydzion Sep 4, 2026
6. maxisbey

   closed this as completedin #3609on Oct 1, 2026
7. maxisbey commented on Oct 2, 2026

   

   maxisbey

   on Oct 2, 2026

   Contributor

   More actions

   This is fixed on main by #3609. Thanks for the clear write-up and the offer of a PR.

   It's a little different from the skip you suggested. When symlink_to() fails with WinError 1314, the test falls back to a directory junction, which needs no privilege, so the escape assertion still runs on machines like yours instead of being skipped.

   I had no unprivileged Windows machine to try that path on, so if it still fails for you, or you hit a different error, please open a new issue.

   _{AI Disclaimer}