[codex] Fix Windows allowedRoots bypass#40
Draft
Ahmed-Hindy wants to merge 1 commit into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #13.
This PR closes a Windows-specific
allowedRootsbypass where a path on a different drive from the configured allowlist root could be treated as allowed.Root cause
isPathInsideRoot()usespath.relative(resolvedRoot, resolvedPath)to decide whether a requested path is contained by an allowed root. The existing check rejected relative paths that escaped the root with..segments:That is enough for same-drive Windows paths and POSIX paths, but it misses an important Windows behavior: when
path.relative()compares paths on different drives, it can return an absolute path instead of a..-prefixed relative path.For example:
Because
"C:\\Users\\Administrator"does not start with.., the old predicate could incorrectly accept it as inside the allowed root.Security impact
This undermines the filesystem allowlist boundary on Windows. A server configured with a narrow allowlist such as:
could still allow MCP clients to open a workspace under a different drive, such as:
That matches the class of behavior reported in #13 and can allow reads/writes outside the configured approved roots.
Changes
isAbsolutefromnode:path.path.relative()results insideisPathInsideRoot().C:\Users\Administratoris rejected when the only allowed root is onG:\....Why this fix
A valid contained path should produce either:
...So the containment check now requires the relationship to be non-absolute before accepting it:
This preserves the existing same-drive behavior while closing the cross-drive Windows bypass.
Validation
Ran on Windows:
All passed.