fix(security): apply_patch paths stay inside the working directory (GHSA-pw64-v3mg-xxmm) - #942

Claude finished @cdeust's task in 1m 31s —— View job

Review of PR #942 done: no Blockers, verdict approve

• Read the diff and the code around it (host_patch.py, host_event.py, host_dispatch.py)
• Ran five reviewers: security, code quality, test coverage, docs accuracy and performance
• Posted the review: fix(security): apply_patch paths stay inside the working directory (GHSA-pw64-v3mg-xxmm) #942 (comment)

Each Add, Delete and Update path, and each Move to: destination, now goes through _resolve. I traced every escape path I could think of: ../ traversal, an absolute path, a symlinked file, a symlinked directory and an outside Move to: destination. The fix refuses each one before it reads any preimage.

I could not run pytest, ruff or the craftsmanship gate here because those commands needed approval in this runner. The review comes from reading and tracing the code.

Verdict: approve

Review 1 of 3: PR #942 (GHSA-pw64-v3mg-xxmm)

Reviewed at commit f4517b0. Five reviewers checked it: security, code quality, test coverage, docs accuracy and performance.

Blockers: none.

Each Add, Delete and Update header, and each *** Move to: destination, now passes through _resolve (mcp_server/hooks/host_patch.py:47-59). On PreToolUse, host_dispatch.py:109 turns a refusal into exit 2. I traced the escape paths below and each one is refused before any preimage is read:

• ../ traversal
• an absolute path
• a symlinked file
• a symlinked directory
• a *** Move to: destination outside the working directory

The 4 escape tests would fail on the unfixed resolver. The existing hook tests use only relative paths inside tmp_path and always set cwd, so they still hold. The CHANGELOG entry and docstrings are accurate. I could not run pytest, ruff or the craftsmanship gate here because those commands needed approval in this runner. The results above come from reading and tracing the code.

Notes, no action before merge

• No test covers a *** Move to: ../x destination or *** Add File: ../x. The fix refuses both (host_patch.py:156, :176).
• This predates the PR and I did not reproduce it: a NUL byte in a patch path may make Path.resolve() raise ValueError, which host_dispatch.py:109 doesn't catch, so the hook would exit 1 instead of 2. That does not read any file outside the project.
• The module docstring at tests_py/hooks/test_host_patch_containment.py:6 says "Each case", but the last test is the inside-project control. "Each refusal case" would be accurate.

Verdict: approve

Claude finished @cdeust's task in 1m 13s —— View job

Review 2 of 3: PR #942 (GHSA-pw64-v3mg-xxmm)

• Check earlier Blockers (review 1 had none)
• Read the commit pushed after f4517b0: 538f60f (a test-only fixup)
• Run the five reviewers: security, code quality, test coverage, docs accuracy and performance
• Post the review

Blockers: none.

538f60f swaps the three substring matches for the anchored regex ESCAPE = r"^Patch path escapes the working directory$". That is exactly the text raised at mcp_server/hooks/host_patch.py:58. HostEventError is a plain ValueError subclass with no custom __str__, so re.search on str(exc) matches. A reworded message, or one with a prefix or suffix, now fails the escape tests. The refusal is also pinned to the containment check in _resolve, so a later preimage-read error no longer satisfies the tests. The craftsmanship rule that every constant carries # source: applies only to numeric constants, so ESCAPE passes.

I could not run pytest, ruff or the craftsmanship gate here because those commands needed approval in this runner. I also could not post a separate comment with gh pr comment, because the runner blocked the command, so this comment is the review. The results above come from reading and tracing the code.

Notes, no action before merge

• In Python re, $ also matches just before a final \n, so a mutant that adds a trailing newline would still pass. Using \Z would make the match exact (tests_py/hooks/test_host_patch_containment.py:21).
• This is carried over from review 1. No test covers a *** Move to: ../x destination or *** Add File: ../x. The fix refuses both through the same _resolve.

Verdict: approve