Reject overlapping host directories at mount registration - #741
Reject overlapping host directories at mount registration#741samuelcolvin wants to merge 1 commit into
Conversation
A mount's mode is selected by longest-prefix match on the virtual path, so one host file reachable through two mounts takes whichever mount the spelling selects — the weaker mode wins. Mounting a directory read-write and a subdirectory of it read-only did not protect the subdirectory (Hack Monty round 3). MountTable::push_mount now returns Result and refuses a mount whose canonical host path equals, contains, or is contained by an existing mount's, unconditionally on mode. Via the pool this surfaces at feed time as a session-preserving ValueError, since specs only meet there. Disjoint host directories at nested virtual paths still work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xsqqc7SY9Gaw8W5ojgMKS
Codecov Results 📊✅ Patch coverage is 80.85%. Project has 42170 uncovered lines. Files with missing lines (3)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 56.78% 56.78% —%
==========================================
Files 407 407 —
Lines 97465 97571 +106
Branches 207515 207679 +164
==========================================
+ Hits 55336 55401 +65
- Misses 42129 42170 +41
- Partials 3977 3983 +6Generated by Codecov Action |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /// Whether two canonical host paths name the same directory or nest one inside | ||
| /// the other. Component-wise, so `/a/bc` does not overlap `/a/b`. | ||
| fn host_paths_overlap(a: &Path, b: &Path) -> bool { | ||
| a.starts_with(b) || b.starts_with(a) |
There was a problem hiding this comment.
🔴 Critical src/mount_table.rs:431
host_paths_overlap permits the same directory to be mounted with conflicting modes after the directory is renamed: MountRoot::open compares the stale fs::canonicalize label (/base/shared) with the new label (/base/old-shared), even though both descriptors reference the same directory. The open-then-canonicalize TOCTOU also lets a replacement race produce a disjoint label for a descriptor inside an existing mount, bypassing the protection and allowing writes through the weaker mount; derive overlap and ancestry from the opened directory identity instead of cached path labels.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/monty-fs/src/mount_table.rs around line 431:
`host_paths_overlap` permits the same directory to be mounted with conflicting modes after the directory is renamed: `MountRoot::open` compares the stale `fs::canonicalize` label (`/base/shared`) with the new label (`/base/old-shared`), even though both descriptors reference the same directory. The open-then-canonicalize TOCTOU also lets a replacement race produce a disjoint label for a descriptor inside an existing mount, bypassing the protection and allowing writes through the weaker mount; derive overlap and ancestry from the opened directory identity instead of cached path labels.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR modifies mount validation in You can customize Macroscope's approvability policy. Learn more. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty-fs/src/error.rs">
<violation number="1" location="crates/monty-fs/src/error.rs:184">
P3: When both overlapping mounts use the same mode, no less restrictive mode exists, but this `ValueError` claims one would apply. Because `push_mount` rejects overlaps unconditionally, report the mode-independent overlap policy instead.</violation>
</file>
<file name="crates/monty-fs/src/mount_table.rs">
<violation number="1" location="crates/monty-fs/src/mount_table.rs:431">
P1: On case-insensitive filesystems, `canonicalize` can preserve input casing, so `Path::starts_with` treats case-only aliases as disjoint. Both mounts can then register, allowing a read-write parent to bypass a read-only child; compare filesystem identities or perform a case-aware ancestor check.</violation>
</file>
<file name="crates/monty-python/tests/test_mount_table.py">
<violation number="1" location="crates/monty-python/tests/test_mount_table.py:425">
P3: The test asserts an exact full-message match that embeds both canonical host paths via `test_dir.resolve()`. Those strings must byte-for-byte equal Rust's `fs::canonicalize`/`Path::display()` output on the OS running CI (Windows and macOS, per ci.yml). Platform-specific canonicalization output tends to diverge between the two languages — e.g. Rust's `fs::canonicalize` on Windows can yield a `\\?\`-prefixed verbatim path and Python's `Path.resolve()` does not, and macOS symlinked temp roots (like `/var` → `/private/var`) resolve differently in edge cases. A harmless formatting or path-casing difference then fails the whole test even though the behavior under test — the overlapping mounts are rejected with a clear ValueError — is correct. Anchor the assertion to the stable, intent-bearing part of the message instead of the exact canonical path spelling.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// Whether two canonical host paths name the same directory or nest one inside | ||
| /// the other. Component-wise, so `/a/bc` does not overlap `/a/b`. | ||
| fn host_paths_overlap(a: &Path, b: &Path) -> bool { | ||
| a.starts_with(b) || b.starts_with(a) |
There was a problem hiding this comment.
P1: On case-insensitive filesystems, canonicalize can preserve input casing, so Path::starts_with treats case-only aliases as disjoint. Both mounts can then register, allowing a read-write parent to bypass a read-only child; compare filesystem identities or perform a case-aware ancestor check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-fs/src/mount_table.rs, line 431:
<comment>On case-insensitive filesystems, `canonicalize` can preserve input casing, so `Path::starts_with` treats case-only aliases as disjoint. Both mounts can then register, allowing a read-write parent to bypass a read-only child; compare filesystem identities or perform a case-aware ancestor check.</comment>
<file context>
@@ -384,13 +417,20 @@ impl MountRoot {
+/// Whether two canonical host paths name the same directory or nest one inside
+/// the other. Component-wise, so `/a/bc` does not overlap `/a/b`.
+fn host_paths_overlap(a: &Path, b: &Path) -> bool {
+ a.starts_with(b) || b.starts_with(a)
+}
+
</file context>
| '{existing_virtual_path}', which would let the less restrictive mount's mode apply to the \ | ||
| other's files", |
There was a problem hiding this comment.
P3: When both overlapping mounts use the same mode, no less restrictive mode exists, but this ValueError claims one would apply. Because push_mount rejects overlaps unconditionally, report the mode-independent overlap policy instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-fs/src/error.rs, line 184:
<comment>When both overlapping mounts use the same mode, no less restrictive mode exists, but this `ValueError` claims one would apply. Because `push_mount` rejects overlaps unconditionally, report the mode-independent overlap policy instead.</comment>
<file context>
@@ -151,6 +168,25 @@ impl MountError {
+ ExcType::ValueError,
+ Some(format!(
+ "cannot mount '{}' at '{virtual_path}': its host directory overlaps the mount of '{}' at \
+ '{existing_virtual_path}', which would let the less restrictive mount's mode apply to the \
+ other's files",
+ host_path.display(),
</file context>
| '{existing_virtual_path}', which would let the less restrictive mount's mode apply to the \ | |
| other's files", | |
| '{existing_virtual_path}', because overlapping host directories are not allowed", |
| with pytest.raises(MontyRuntimeError) as exc_info: | ||
| monty_run('1', mount=mounts) | ||
| resolved = test_dir.resolve() | ||
| assert str(exc_info.value) == ( |
There was a problem hiding this comment.
P3: The test asserts an exact full-message match that embeds both canonical host paths via test_dir.resolve(). Those strings must byte-for-byte equal Rust's fs::canonicalize/Path::display() output on the OS running CI (Windows and macOS, per ci.yml). Platform-specific canonicalization output tends to diverge between the two languages — e.g. Rust's fs::canonicalize on Windows can yield a \\?\-prefixed verbatim path and Python's Path.resolve() does not, and macOS symlinked temp roots (like /var → /private/var) resolve differently in edge cases. A harmless formatting or path-casing difference then fails the whole test even though the behavior under test — the overlapping mounts are rejected with a clear ValueError — is correct. Anchor the assertion to the stable, intent-bearing part of the message instead of the exact canonical path spelling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-python/tests/test_mount_table.py, line 425:
<comment>The test asserts an exact full-message match that embeds both canonical host paths via `test_dir.resolve()`. Those strings must byte-for-byte equal Rust's `fs::canonicalize`/`Path::display()` output on the OS running CI (Windows and macOS, per ci.yml). Platform-specific canonicalization output tends to diverge between the two languages — e.g. Rust's `fs::canonicalize` on Windows can yield a `\\?\`-prefixed verbatim path and Python's `Path.resolve()` does not, and macOS symlinked temp roots (like `/var` → `/private/var`) resolve differently in edge cases. A harmless formatting or path-casing difference then fails the whole test even though the behavior under test — the overlapping mounts are rejected with a clear ValueError — is correct. Anchor the assertion to the stable, intent-bearing part of the message instead of the exact canonical path spelling.</comment>
<file context>
@@ -412,6 +412,23 @@ def test_multiple_mounts_different_modes(monty_run: RunMonty, test_dir: Path):
+ with pytest.raises(MontyRuntimeError) as exc_info:
+ monty_run('1', mount=mounts)
+ resolved = test_dir.resolve()
+ assert str(exc_info.value) == (
+ f"ValueError: cannot mount '{resolved / 'subdir'}' at '/m/subdir': its host directory overlaps the "
+ f"mount of '{resolved}' at '/m', which would let the less restrictive mount's mode apply to the "
</file context>
Merging this PR will not alter performance
Comparing 
Footnotes
|
1 participant
A mount's mode is selected by longest-prefix match on the virtual path, so one host file reachable through two mounts takes whichever mount the spelling selects — the weaker mode wins. Mounting a directory read-write and a subdirectory of it read-only did not protect the subdirectory (Hack Monty round 3).
MountTable::push_mount now returns Result and refuses a mount whose canonical host path equals, contains, or is contained by an existing mount's, unconditionally on mode. Via the pool this surfaces at feed time as a session-preserving ValueError, since specs only meet there. Disjoint host directories at nested virtual paths still work.
Claude-Session: https://claude.ai/code/session_015xsqqc7SY9Gaw8W5ojgMKS
Summary by cubic
Rejects mounts whose host directories overlap (same directory, ancestor, or descendant) at registration to prevent bypassing stricter modes. Previously, a file reachable through two mounts used the weaker mode via longest-prefix virtual-path routing (e.g., rw parent over ro child); now registration fails with a session-preserving ValueError.
monty-fs:MountTable::push_mountreturnsResultand rejects overlaps using canonical host paths; disjoint host directories at nested virtual paths remain valid.MountError::OverlappingMountswith a clear message naming both mounts; surfaced via the pool asPoolError::Runtimewhen a feed starts. JS/Python bindings and CLI help document the restriction; tests cover rejection and allowed cases.Migration
ValueError.MountTable::push_mountto handle aResultand propagate or report the error.Written for commit 0a5efb3. Summary will update on new commits.