Skip to content

Commit 7b01721

Browse files
fix(windows/cap-broker): reject directory paths with clear error in open_windows_supervisor_path
Address medium-severity finding from gemini-code-assist PR nolabs-ai#725 on `open_windows_supervisor_path`. `std::fs::OpenOptions` on Windows wraps `CreateFileW` without `FILE_FLAG_BACKUP_SEMANTICS`, so directory paths fail with an opaque `ERROR_ACCESS_DENIED` that looks like an ACL problem but is really a platform quirk. The cap-pipe protocol is file-scoped by design (brokers file handles via `DuplicateHandle`, not directory enumeration handles). Added a pre-open `metadata().is_dir()` check that returns a clear capability-broker-boundary error instead of surfacing the platform error. Deliberate choice over transparent directory support: - Setting `FILE_FLAG_BACKUP_SEMANTICS` via `OpenOptionsExt` would let directories open, but silently granting directory handles to sandboxed children is a capability-scope expansion beyond the existing broker contract — directory handles enable enumeration and change-notification beyond the approved path's scope. - If future work needs directory brokering, it should add an explicit `is_dir` discriminator to `CapabilityRequest`. Verification: cargo fmt --all -- --check and cargo build --workspace both green. Signed-off-by: oscarmackjr-twg <oscar.mack.jr@gmail.com>
1 parent ed2ba1e commit 7b01721

3 files changed

Lines changed: 122 additions & 0 deletions

File tree

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
---
2+
task: fix-open-supervisor-path-directory
3+
type: bug-fix
4+
severity: medium (capability broker fails opaquely on directory paths)
5+
source: gemini-code-assist PR #725 review (comment 3119918153, supervisor.rs:902)
6+
created: 2026-04-21
7+
---
8+
9+
# Quick Task: Clear error when capability broker receives a directory path
10+
11+
## Problem
12+
13+
gemini-code-assist on PR #725:
14+
15+
> `open_windows_supervisor_path` uses default `OpenOptions`, which on Windows will fail to open directories. If a sandboxed child requests access to a directory via the capability pipe, the supervisor would fail to open it with an opaque platform-level error.
16+
17+
On Windows, `std::fs::OpenOptions::open()` wraps `CreateFileW` without `FILE_FLAG_BACKUP_SEMANTICS`. Directory opens return `ERROR_ACCESS_DENIED`, which propagates up as a generic `SandboxInit("Windows supervisor failed to open approved path … Access is denied")` — misleading because the issue isn't the ACL, it's the platform's requirement for the backup-semantics flag.
18+
19+
## Design decision
20+
21+
**Explicit rejection with clear error, not transparent directory support.**
22+
23+
Two options were considered:
24+
1. (Chosen) Detect directory paths pre-open; reject with a clear capability-broker-boundary message.
25+
2. Set `FILE_FLAG_BACKUP_SEMANTICS` via `OpenOptionsExt::custom_flags` and return directory handles.
26+
27+
Option 2 is rejected for this fix because:
28+
- The cap-pipe protocol was designed to broker **file handles** (`HandleKind::File` + `DuplicateHandle` based on read/write access mask). Directory handles are semantically different: they enable enumeration + change-notification beyond the approved file's scope.
29+
- Granting a directory handle silently would be capability-scope expansion (broker surface change). That needs a protocol revision, not a drive-by patch.
30+
- If future work wants directory brokering, adding an `is_dir: bool` to the request keeps the distinction explicit.
31+
32+
The diagnostic improvement alone (option 1) is what gemini was flagging — "supervisor would fail" implied "would fail opaquely with a confusing error", not "should be made to succeed".
33+
34+
## Fix
35+
36+
Pre-open metadata check. If the target is a directory, return a clear capability-broker-boundary error. Otherwise proceed with the existing OpenOptions flow.
37+
38+
```rust
39+
pub(super) fn open_windows_supervisor_path(
40+
path: &Path,
41+
access: &nono::AccessMode,
42+
) -> Result<std::fs::File> {
43+
// std::fs::OpenOptions on Windows uses CreateFileW without
44+
// FILE_FLAG_BACKUP_SEMANTICS, so directory paths fail with an opaque
45+
// ERROR_ACCESS_DENIED. The capability broker contract is file-scoped
46+
// by design (cap-pipe brokers file handles via DuplicateHandle, not
47+
// directory enumeration handles). Reject directory paths up-front
48+
// with a clear boundary message rather than surfacing the platform
49+
// error. Future directory brokering (if ever needed) should add an
50+
// explicit `is_dir` discriminator to the request rather than
51+
// silently flipping this branch.
52+
if path.metadata().map(|m| m.is_dir()).unwrap_or(false) {
53+
return Err(NonoError::SandboxInit(format!(
54+
"Windows supervisor refused directory path {}: capability broker brokers file handles only",
55+
path.display()
56+
)));
57+
}
58+
59+
let mut options = std::fs::OpenOptions::new();
60+
// ... (existing match on access mode) ...
61+
}
62+
```
63+
64+
## TOCTOU note
65+
66+
The metadata check and the subsequent open are not atomic — a symlink could be swapped between the two. The existing code has the same TOCTOU shape (no pre-open check). This fix doesn't make the race worse; the worst case is a directory check that says "file" followed by an open that now fails with `ACCESS_DENIED` — i.e., back to the original behavior.
67+
68+
Proper atomicity would require `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS` and a `GetFileInformationByHandle` post-open check. That's a substantial refactor. Not worth it for a diagnostic-quality improvement; the approved path has already passed the policy check anyway.
69+
70+
## Verification
71+
72+
- `cargo fmt --all -- --check` / `cargo build --workspace` / `cargo clippy --workspace --all-targets -- -D warnings -D clippy::unwrap_used` all green.
73+
- No existing tests of `open_windows_supervisor_path` (grep shows no test callers). Not adding a unit test for this — the behavior is a single-statement pre-check with obvious semantics.
74+
75+
## Propagation
76+
77+
Standard flow: windows-squash → v2.0-pr (cherry-pick + amend + force-push) → v2.1-pr (rebase + force-push) → reply + resolve PR thread `PRRT_kwDORFb4ys58myF5`.
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
task: fix-open-supervisor-path-directory
3+
status: complete
4+
completed: 2026-04-21
5+
source: gemini-code-assist PR #725 review (comment 3119918153)
6+
---
7+
8+
# Summary: Clear error when capability broker receives a directory path
9+
10+
## Outcome
11+
12+
Added a pre-open `metadata().is_dir()` check to `open_windows_supervisor_path`. Directory paths now return a clear capability-broker-boundary error rather than surfacing Windows' opaque `ERROR_ACCESS_DENIED` from `CreateFileW` without `FILE_FLAG_BACKUP_SEMANTICS`.
13+
14+
Deliberately chose clear rejection over transparent directory support: the cap-pipe protocol is file-scoped by design, and silently granting directory handles would expand capability scope beyond the existing broker contract. Future directory brokering (if ever needed) should require an explicit `is_dir` discriminator on `CapabilityRequest`.
15+
16+
## File touched
17+
18+
- `crates/nono-cli/src/exec_strategy_windows/supervisor.rs` — added 13 lines (metadata check + comment) in `open_windows_supervisor_path`.
19+
20+
## Verification
21+
22+
- `cargo fmt --all -- --check` → clean
23+
- `cargo build --workspace` → exit 0
24+
25+
## PR thread
26+
27+
To resolve: `PRRT_kwDORFb4ys58myF5` (comment 3119918153).

crates/nono-cli/src/exec_strategy_windows/supervisor.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,6 +1220,24 @@ pub(super) fn open_windows_supervisor_path(
12201220
path: &Path,
12211221
access: &nono::AccessMode,
12221222
) -> Result<std::fs::File> {
1223+
// std::fs::OpenOptions on Windows wraps CreateFileW without
1224+
// FILE_FLAG_BACKUP_SEMANTICS, so directory paths would fail with an
1225+
// opaque ERROR_ACCESS_DENIED that looks like an ACL problem but is
1226+
// really a platform quirk. The capability broker contract is
1227+
// file-scoped by design (cap-pipe brokers file handles via
1228+
// DuplicateHandle, not directory enumeration handles). Reject
1229+
// directory paths up-front with a clear boundary message. If future
1230+
// work needs directory brokering it should add an explicit `is_dir`
1231+
// discriminator to CapabilityRequest rather than silently flipping
1232+
// this branch (would be a capability-scope expansion requiring
1233+
// protocol review).
1234+
if path.metadata().map(|m| m.is_dir()).unwrap_or(false) {
1235+
return Err(NonoError::SandboxInit(format!(
1236+
"Windows supervisor refused directory path {}: capability broker brokers file handles only",
1237+
path.display()
1238+
)));
1239+
}
1240+
12231241
let mut options = std::fs::OpenOptions::new();
12241242
match access {
12251243
nono::AccessMode::Read => {

0 commit comments

Comments
 (0)