fix(security): normalize path casing for Windows write-safety checks - #11148
fix(security): normalize path casing for Windows write-safety checks#11148Ruzzgar wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the Windows write-safety investigation. The case-normalization gap still exists on current main, but the implementation needs to be rebased onto the shared safety layer.
Problems
- Current enforcement is
agent/file_safety.py:91-106,134-139, which comparesrealpath()results directly.tools/file_operations.py:146-148now only delegates to that shared function, so the proposed helper in the wrapper would not affect the live predicate. - The shared predicate also protects
mcp-tokensandpairingsubtrees atagent/file_safety.py:120-130; a central normalization must cover those comparisons too. - Current ACP writes call the same predicate at
agent/copilot_acp_client.py:727-735, while the submitted test fixture reloads onlytools.file_operations.py.
Suggested changes
- Apply the normalizer centrally in
agent/file_safety.pyto both operands of every protected-path and safe-root comparison. - Port the Windows casing regressions to the shared predicate and cover its shared caller behavior.
This is an automated hermes-sweeper review.
|
|
||
| _HOME = str(Path.home()) | ||
|
|
||
|
|
There was a problem hiding this comment.
Current main moved the live predicate to agent/file_safety.py; tools/file_operations._is_write_denied() now delegates there. Please place this normalizer in the shared module and apply it to its candidate, deny-list, protected-subtree, and safe-root comparisons so both file tools and ACP writes are covered.
|
|
||
| import tools.file_operations as file_operations | ||
| from tools.file_operations import _is_write_denied | ||
|
|
There was a problem hiding this comment.
On current main this fixture reloads only the compatibility wrapper, while enforcement is in the already-imported agent.file_safety module. Port these regressions to exercise agent.file_safety.is_write_denied() so they prove the salvaged implementation.
Summary
This fixes a Windows-specific write-safety bypass in tools.file_operations.
The write deny-list, deny-prefix checks, and HERMES_WRITE_SAFE_ROOT sandboxing were comparing resolved paths with case-sensitive string checks. On Windows, that can produce incorrect security decisions because the filesystem is case-insensitive even when the strings are not.
This patch normalizes all write-safety path comparisons through a shared helper before checking:
static denied paths
denied path prefixes
HERMES_WRITE_SAFE_ROOT
Root Cause
_is_write_denied() and _get_safe_write_root() used realpath()/expanduser() but did not normalize case before comparing paths.
That meant paths like the following could behave incorrectly on Windows:
C:\Users\name.SSH\ID_RSA
C:\USERS\NAME\workspace\file.txt when HERMES_WRITE_SAFE_ROOT was configured with different casing
Changes
Added _normalize_write_path() in tools/file_operations.py
Applied that normalization to:
WRITE_DENIED_PATHS
WRITE_DENIED_PREFIXES
_get_safe_write_root()
_is_write_denied()
Added regression coverage for Windows-style case-insensitive behavior in tests/tools/test_file_write_safety.py
Tests
Ran targeted write-safety tests:
uv run --extra dev python -m pytest -n 0
tests/tools/test_file_write_safety.py::TestStaticDenyList
tests/tools/test_file_write_safety.py::TestSafeWriteRoot
tests/tools/test_file_write_safety.py::TestWindowsCaseInsensitiveComparisons -q
Result:
12 passed