diff --git a/CHANGELOG.md b/CHANGELOG.md index 27810010..6f24a620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,11 +26,20 @@ breaking entries are marked **BREAKING**. document, success or error, so a consumer windowing measurements by build no longer shells out to `kaish --version` per call. +- **`KernelBackend::path_access` and `Filesystem::path_access`** — the per-path + read/write/execute query behind the file tests. Both are defaulted, so + existing implementations keep compiling; not breaking. + ### Fixed - `help ` renders a tool's subcommands and their flags, and names each parameter's aliases. `help kj` and every wrapped command showed "No parameters." before. +- **File tests on virtual and real paths** — `-w` claimed read-only mounts and + root-owned files were writable, and `-x` denied that a memory-backed + directory is searchable. `-w`/`-r`/`-x` now answer from the owning mount plus + the OS's effective access. + ## [0.16.0] - 2026-08-23 ### Added diff --git a/Cargo.lock b/Cargo.lock index 37e492b1..7b0a5b1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1168,6 +1168,7 @@ dependencies = [ "async-trait", "getrandom 0.3.4", "kaish-types", + "rustix", "tempfile", "tokio", ] diff --git a/crates/kaish-kernel/src/backend/local.rs b/crates/kaish-kernel/src/backend/local.rs index 1aafa72c..9b769940 100644 --- a/crates/kaish-kernel/src/backend/local.rs +++ b/crates/kaish-kernel/src/backend/local.rs @@ -13,6 +13,7 @@ use super::{ }; use crate::tools::{ToolArgs, ToolCtx, ToolRegistry}; use crate::vfs::{DirEntry, Filesystem, MountInfo, VfsRouter}; +use kaish_types::PathAccess; /// Local backend implementation using VfsRouter and ToolRegistry. /// @@ -400,6 +401,14 @@ impl KernelBackend for LocalBackend { self.vfs.read_only() } + /// Asks the router, which asks the mount that owns the path. The trait + /// default would use `read_only()` above — the whole-router answer — + /// which is false whenever any one mount is writable and would call + /// `/v/bin/echo` writable on the strength of `/tmp`. + async fn path_access(&self, path: &Path) -> BackendResult { + Ok(self.vfs.path_access(path).await?) + } + fn backend_type(&self) -> &str { "local" } diff --git a/crates/kaish-kernel/src/backend/overlay.rs b/crates/kaish-kernel/src/backend/overlay.rs index 855e0974..8de3aba6 100644 --- a/crates/kaish-kernel/src/backend/overlay.rs +++ b/crates/kaish-kernel/src/backend/overlay.rs @@ -40,6 +40,7 @@ use super::{ }; use crate::tools::{ToolArgs, ToolCtx}; use crate::vfs::{DirEntry, Filesystem, MountInfo, VfsRouter}; +use kaish_types::PathAccess; /// The final path component, used to name a synthesized directory entry for a /// shared ancestor (`/v` → `v`). Falls back to `/` for a component-less path. @@ -418,6 +419,21 @@ impl KernelBackend for VirtualOverlayBackend { self.inner.read_only() && self.vfs.read_only() } + /// Routes the same way `stat` does, so the answer comes from the layer + /// that owns the path. The trait default would use `read_only()` above, + /// which is the AND of both layers and belongs to neither path. + async fn path_access(&self, path: &Path) -> BackendResult { + if self.is_virtual_path(path) { + Ok(self.vfs.path_access(path).await?) + } else if self.is_shared_ancestor(path) { + // A synthesized ancestor directory exists only to be traversed: + // readable and searchable, and kaish creates nothing in it. + Ok(PathAccess::resolve(Some(0o555), true)) + } else { + self.inner.path_access(path).await + } + } + fn backend_type(&self) -> &str { "virtual-overlay" } diff --git a/crates/kaish-kernel/src/kernel.rs b/crates/kaish-kernel/src/kernel.rs index 8a085d53..3eef9141 100644 --- a/crates/kaish-kernel/src/kernel.rs +++ b/crates/kaish-kernel/src/kernel.rs @@ -4343,20 +4343,32 @@ impl Kernel { let ctx = self.exec_ctx.read().await; (ctx.resolve_path(&path_str), ctx.backend.clone()) }; - let entry = backend.stat(&resolved).await.ok(); + // `-r`/`-w`/`-x` go through `path_access`, never through + // the raw mode bits: the mount's read-only state is half + // the answer and `stat` does not carry it. The `test` + // builtin's `file_test` reads the same query, and + // `file_test_writable_tests` runs every case through both + // spellings — this mirror has drifted before. Ok(match op { - FileTestOp::Exists => entry.is_some(), - FileTestOp::IsFile => entry.as_ref().is_some_and(|e| e.is_file()), - FileTestOp::IsDir => entry.as_ref().is_some_and(|e| e.is_dir()), - FileTestOp::Readable => entry.as_ref().is_some_and(|e| { - e.permissions.is_none_or(|p| p & 0o444 != 0) - }), - FileTestOp::Writable => entry.as_ref().is_some_and(|e| { - e.permissions.is_none_or(|p| p & 0o222 != 0) - }), - FileTestOp::Executable => entry.as_ref().is_some_and(|e| { - e.permissions.is_some_and(|p| p & 0o111 != 0) - }), + FileTestOp::Exists => backend.stat(&resolved).await.is_ok(), + FileTestOp::IsFile => { + backend.stat(&resolved).await.is_ok_and(|e| e.is_file()) + } + FileTestOp::IsDir => { + backend.stat(&resolved).await.is_ok_and(|e| e.is_dir()) + } + FileTestOp::Readable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.readable), + FileTestOp::Writable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.writable), + FileTestOp::Executable => backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.executable), }) } TestExpr::StringTest { op, value } => match op { diff --git a/crates/kaish-kernel/src/tools/builtin/test.rs b/crates/kaish-kernel/src/tools/builtin/test.rs index ba15c1af..442e90f3 100644 --- a/crates/kaish-kernel/src/tools/builtin/test.rs +++ b/crates/kaish-kernel/src/tools/builtin/test.rs @@ -338,20 +338,29 @@ async fn file_test(ctx: &ExecContext, op: &str, path: &str) -> bool { return false; } let resolved = ctx.resolve_path(path); - let entry = ctx.backend.stat(&resolved).await.ok(); + // `-r`/`-w`/`-x` go through `path_access`, never through the raw mode + // bits: the mount's read-only state is half the answer and `stat` does + // not carry it. `eval_test_async` reads the same query, and + // `file_test_writable_tests` runs every case through both. match op { - "-e" => entry.is_some(), - "-f" => entry.as_ref().is_some_and(|e| e.is_file()), - "-d" => entry.as_ref().is_some_and(|e| e.is_dir()), - "-r" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_none_or(|p| p & 0o444 != 0)), - "-w" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_none_or(|p| p & 0o222 != 0)), - "-x" => entry - .as_ref() - .is_some_and(|e| e.permissions.is_some_and(|p| p & 0o111 != 0)), + "-e" => ctx.backend.stat(&resolved).await.is_ok(), + "-f" => ctx.backend.stat(&resolved).await.is_ok_and(|e| e.is_file()), + "-d" => ctx.backend.stat(&resolved).await.is_ok_and(|e| e.is_dir()), + "-r" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.readable), + "-w" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.writable), + "-x" => ctx + .backend + .path_access(&resolved) + .await + .is_ok_and(|access| access.executable), _ => unreachable!("file_test called with non-file op {op:?}"), } } diff --git a/crates/kaish-kernel/src/vfs/builtin_fs.rs b/crates/kaish-kernel/src/vfs/builtin_fs.rs index b5fcc4a5..520706b1 100644 --- a/crates/kaish-kernel/src/vfs/builtin_fs.rs +++ b/crates/kaish-kernel/src/vfs/builtin_fs.rs @@ -1,4 +1,17 @@ -//! BuiltinFs — read-only VFS that presents builtins as executable entries under `/v/bin/`. +//! BuiltinFs — read-only VFS that lists builtins as file entries under `/v/bin/`. +//! +//! The entries are not executable and nothing here ever executes. `stat` +//! reports no mode, so `test -x /v/bin/grep` answers NO, and `real_path` +//! returns `None`, so there is no path for exec(2) to open. `read` returns a +//! line opening with `#!`, which makes an entry look executable. +//! +//! Running a builtin goes by name through the `ToolRegistry` and never routes +//! through this filesystem. `/v/bin` is an inventory to list and read, not a +//! directory of programs. Reporting `0o111` would flip `test -x /v/bin/*` from +//! NO to YES — a behavior change that wants its own decision. +//! +//! `read_only()` is `true`, and the closed `-w` default depends on it staying +//! true — see `Filesystem::path_access`. use std::io; use std::path::{Path, PathBuf}; @@ -9,7 +22,8 @@ use async_trait::async_trait; use crate::tools::ToolRegistry; use super::{DirEntry, Filesystem}; -/// A read-only filesystem that exposes registered builtins as entries. +/// A read-only filesystem that exposes registered builtins as file +/// entries. Listable and readable; not executable — see the module docs. pub struct BuiltinFs { tools: Arc, } diff --git a/crates/kaish-kernel/src/vfs/router.rs b/crates/kaish-kernel/src/vfs/router.rs index bbdfeebe..ef5338cc 100644 --- a/crates/kaish-kernel/src/vfs/router.rs +++ b/crates/kaish-kernel/src/vfs/router.rs @@ -3,6 +3,7 @@ //! Routes filesystem operations to the appropriate backend based on path. use super::{DirEntry, Filesystem}; +use kaish_vfs::PathAccess; use async_trait::async_trait; use std::collections::BTreeMap; use std::io; @@ -14,6 +15,12 @@ use std::sync::Arc; // paths keep working. pub use kaish_types::backend::MountInfo; +/// Mode reported for a directory the router synthesizes rather than reads +/// from a mount: the root, and any ancestor of a mount that has no mount of +/// its own. Readable and searchable, never writable — these directories are +/// derived from the mount table and the router creates nothing in them. +const SYNTHESIZED_DIRECTORY_MODE: u32 = 0o555; + /// Routes filesystem operations to mounted backends. /// /// Mount points are matched by longest prefix. For example, if `/mnt` and @@ -388,6 +395,33 @@ impl Filesystem for VfsRouter { from_fs.rename(&from_relative, &to_relative).await } + /// Delegates to the mount that owns the path, so the answer is that + /// mount's — not the whole router's. `read_only()` below is the + /// whole-router question and cannot answer for one path: a router with a + /// writable `/` and a read-only `/v/bin` is read-only for neither. + /// + /// Falls back the way `stat` does. `stat` synthesizes a directory for the + /// root and for any ancestor of a mount (`/v` above `/v/jobs`), so those + /// paths exist, and an answer here that errored on them would contradict + /// it: `[[ -e /v ]]` true and `[[ -r /v ]]` false about the same path. + /// A synthesized directory is readable and searchable, and never + /// writable — the router creates nothing in one. + async fn path_access(&self, path: &Path) -> io::Result { + match self.find_mount(path) { + Ok((fs, relative)) => fs.path_access(&relative).await, + Err(e) => { + let path_str = path.to_string_lossy(); + let is_synthesized = + path_str.is_empty() || path_str == "/" || self.has_mount_under(path); + if is_synthesized { + Ok(PathAccess::resolve(Some(SYNTHESIZED_DIRECTORY_MODE), true)) + } else { + Err(e) + } + } + } + } + fn read_only(&self) -> bool { // Router is read-only iff every mount is. Empty router returns // false — a router with no mounts isn't meaningfully read-only, @@ -622,6 +656,87 @@ mod tests { assert_eq!(result.unwrap_err().kind(), io::ErrorKind::Unsupported); } + // `stat` synthesizes a directory for the root and for any ancestor of a + // mount, so those paths exist. `path_access` has to agree with `stat` + // about the same path — going straight to `find_mount` errors where + // `stat` succeeds, and `[[ -e /v ]]` would be true while `[[ -r /v ]]` + // was false about the identical path. + #[tokio::test] + async fn path_access_agrees_with_stat_on_synthesized_directories() { + let mut router = VfsRouter::new(); + router.mount("/v/docs", MemoryFs::new()); + + for path in ["/", "/v"] { + let path = Path::new(path); + assert!( + router.stat(path).await.is_ok(), + "{} is synthesized by stat", + path.display() + ); + let access = router + .path_access(path) + .await + .unwrap_or_else(|e| panic!("path_access must not error where stat succeeds: {e}")); + assert!(access.readable, "{} must be readable", path.display()); + assert!(access.executable, "{} must be searchable", path.display()); + assert!( + !access.writable, + "the router creates nothing in {}", + path.display() + ); + } + } + + // The synthesis must not swallow a genuinely absent path. + #[tokio::test] + async fn path_access_errors_on_a_path_with_no_mount() { + let mut router = VfsRouter::new(); + router.mount("/v/docs", MemoryFs::new()); + assert!(router.path_access(Path::new("/nope")).await.is_err()); + assert!(router.path_access(Path::new("/v/docs/absent")).await.is_err()); + } + + // A real mount answers for itself, not with the synthesized defaults. + #[tokio::test] + async fn path_access_at_a_mount_point_asks_the_mount() { + let mut router = VfsRouter::new(); + router.mount("/rw", MemoryFs::new()); + router.mount("/ro", BuiltinFsStub); + + assert!(router.path_access(Path::new("/rw")).await.unwrap().writable); + assert!(!router.path_access(Path::new("/ro")).await.unwrap().writable); + assert!(router.path_access(Path::new("/ro")).await.unwrap().readable); + } + + /// A minimal read-only mount that reports no mode, standing in for + /// `BuiltinFs`/`JobFs` without dragging a ToolRegistry into this module. + struct BuiltinFsStub; + + #[async_trait] + impl Filesystem for BuiltinFsStub { + async fn read(&self, _path: &Path) -> io::Result> { + Ok(Vec::new()) + } + async fn write(&self, _path: &Path, _data: &[u8]) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + async fn list(&self, _path: &Path) -> io::Result> { + Ok(Vec::new()) + } + async fn stat(&self, _path: &Path) -> io::Result { + Ok(DirEntry::directory(".")) + } + async fn mkdir(&self, _path: &Path) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + async fn remove(&self, _path: &Path) -> io::Result<()> { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "read-only")) + } + fn read_only(&self) -> bool { + true + } + } + #[tokio::test] async fn read_only_empty_router_returns_false() { let router = VfsRouter::new(); diff --git a/crates/kaish-kernel/tests/file_test_writable_tests.rs b/crates/kaish-kernel/tests/file_test_writable_tests.rs new file mode 100644 index 00000000..88da12fe --- /dev/null +++ b/crates/kaish-kernel/tests/file_test_writable_tests.rs @@ -0,0 +1,503 @@ +//! `-w` must answer "can kaish write this path", and it gets that answer from +//! the mount's read-only state AND the mode bits the mount reports. +//! +//! Before this change `DirEntry.permissions` was `None` on MemoryFs +//! (writable), DevFs (writable — writes discard), BuiltinFs (read-only) and +//! JobFs (read-only), so an absent mode carried no information and neither +//! default was right: opening it (shipped 0.16) called `/v/bin/echo` and +//! `/v/jobs/1/status` writable; closing it called `/v/probe.txt` unwritable, +//! about a path the very next line writes. +//! +//! The fix fills the signal in at the source. MemoryFs and DevFs now report +//! real modes, so the only backends left reporting `None` are read-only ones +//! and "absent" now means "this backend does not model permissions" — +//! readable, not writable, not executable. `permissions` is a read-only +//! observation: there is no `chmod` builtin in this tree. +//! +//! A mode alone is still not enough. `LocalFs::stat` asks the OS, and the OS +//! does not know about a `LocalFs::read_only` wrapper, so a mode-644 file on +//! a read-only mount reports the write bit set while every write to it fails. +//! `PathAccess::resolve` combines the two facts and is the only constructor. +//! +//! Every case runs twice — once through the `test` builtin +//! (`tools/builtin/test.rs::file_test`) and once through `[[ ]]` +//! (`kernel.rs::eval_test_async`) — because a fix landing in one site and +//! not the other is the bug class this repo keeps getting caught by. + +// Test-fixture code: unwrap/expect on known-good setup is the idiom here. +#![allow(clippy::unwrap_used, clippy::expect_used)] +#![cfg(feature = "localfs")] + +mod common; + +use std::sync::Arc; + +use kaish_kernel::vfs::{DevFs, LocalFs, MemoryFs, VfsRouter}; +use kaish_kernel::{Kernel, KernelBackend, KernelConfig, LocalBackend}; + +use common::{kernel_at, run}; + +/// Assert that both spellings of a file test agree, and on what. +/// +/// `setup` runs first (same kernel), so a case can create its fixture in the +/// VFS. Asserting on both spellings in one helper is the point: it is not +/// possible to add a case here that covers only `test` or only `[[ ]]`. +async fn both_spellings(kernel: &Kernel, setup: &str, op: &str, path: &str, expected: bool) { + if !setup.is_empty() { + let (out, code) = run(kernel, setup).await; + assert_eq!(code, 0, "setup failed: {setup}: out={out:?}"); + } + let want = if expected { 0 } else { 1 }; + + let script = format!("if test {op} {path}; then echo YES; else echo NO; fi"); + let (out, _) = run(kernel, &script).await; + assert_eq!( + out, + if expected { "YES" } else { "NO" }, + "`test {op} {path}` disagrees (want exit {want})", + ); + + let script = format!("if [[ {op} {path} ]]; then echo YES; else echo NO; fi"); + let (out, _) = run(kernel, &script).await; + assert_eq!( + out, + if expected { "YES" } else { "NO" }, + "`[[ {op} {path} ]]` disagrees (want exit {want})", + ); +} + +// ── MemoryFs: writable, reports no mode ──────────────────────────────────── + +/// The regression that killed the close-the-default attempt: `/v` is +/// MemoryFs and MemoryFs is writable. Closing the absent-mode default is +/// only safe because MemoryFs now reports a real file mode (`0o666`) — an +/// implementation that closed the default without filling the mode in fails +/// here. +#[tokio::test] +async fn memoryfs_file_is_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "echo hi > /v/probe.txt", "-w", "/v/probe.txt", true).await; +} + +/// The proof that the answer above is not a guess: the same path really does +/// take a second write. If this fails, the fixture is wrong, not the fix. +#[tokio::test] +async fn memoryfs_file_really_takes_a_second_write() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (_, code) = run(&kernel, "echo one > /v/p2.txt; echo two >> /v/p2.txt").await; + assert_eq!(code, 0, "MemoryFs path must accept an append"); + let (out, _) = run(&kernel, "cat /v/p2.txt").await; + assert_eq!(out, "one\ntwo"); +} + +/// A MemoryFs *directory* is writable too — `mkdir`/`touch` land there — +/// and it is searchable, which is what the `x` bit means for a directory. +/// `-x` on a MemoryFs directory answered NO before this change; `0o777` makes +/// it YES, which is the POSIX answer. This is the user-visible behavior +/// change in this commit. +#[tokio::test] +async fn memoryfs_directory_is_writable_and_searchable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "mkdir -p /v/sub", "-w", "/v/sub", true).await; + both_spellings(&kernel, "", "-x", "/v/sub", true).await; + both_spellings(&kernel, "", "-r", "/v/sub", true).await; + // and the mount root itself + both_spellings(&kernel, "", "-x", "/v", true).await; + both_spellings(&kernel, "", "-w", "/v", true).await; + // the directory really does accept the entry `-w` promises + let (_, code) = run(&kernel, "mkdir -p /v/sub/deeper").await; + assert_eq!(code, 0, "a MemoryFs directory must accept mkdir"); +} + +// ── BuiltinFs: read-only, reports no mode ────────────────────────────────── + +/// `/v/bin` is BuiltinFs, whose `read_only()` is `true` and whose entries +/// report no mode. Shipped 0.16 answered "writable" here. This is one of the +/// two backends that still report `None` after the source fix, and both are +/// read-only — which is what makes the closed default correct. +#[tokio::test] +async fn builtinfs_entry_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-w", "/v/bin/echo", false).await; +} + +/// Read-only is about writes. A BuiltinFs entry reads fine, so `-r` must +/// stay true — an implementation that routed the mount's read-only state +/// into `readable` as well fails here. +#[tokio::test] +async fn builtinfs_entry_is_still_readable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "", "-r", "/v/bin/echo", true).await; +} + +/// The mount really does refuse the write the test predicts. +#[tokio::test] +async fn builtinfs_entry_really_refuses_a_write() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (_, code) = run(&kernel, "echo nope > /v/bin/echo").await; + assert_ne!(code, 0, "BuiltinFs must refuse a write to /v/bin/echo"); +} + +// ── JobFs: read-only, reports no mode ────────────────────────────────────── + +/// `/v/jobs/{id}/status` is JobFs — read-only, and the other backend that +/// reports no mode. Same wrong answer as BuiltinFs in shipped 0.16, from a +/// different mount, so a fix that special-cased `/v/bin` would still fail +/// here. +#[tokio::test] +async fn jobfs_node_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + let (out, code) = run(&kernel, "sleep 30 &").await; + assert_eq!(code, 0, "backgrounding a job failed: {out:?}"); + both_spellings(&kernel, "", "-w", "/v/jobs/1/status", false).await; + // and it is readable, for the same reason BuiltinFs is + both_spellings(&kernel, "", "-r", "/v/jobs/1/status", true).await; +} + +// ── DevFs: writable on purpose, reports no mode ──────────────────────────── + +/// Mount DevFs explicitly rather than trusting a config to do it. +/// +/// `kernel_at` is Passthrough — `/` is `LocalFs("/")`, so `/dev/null` there +/// is the host's device node and these assertions would pass without DevFs +/// being involved at all. That is how the first draft of this file was +/// vacuously green. +fn devfs_kernel() -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/", MemoryFs::new()); + vfs.mount("/dev", DevFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + +/// `DevFs::read_only()` is deliberately `false` — refusing the write would +/// break `> /dev/null`. So the mount cannot carry the answer here and the +/// mode has to: the device files report `0o666`, matching crw-rw-rw- on +/// Linux. +#[tokio::test] +async fn devfs_null_is_writable() { + let kernel = devfs_kernel(); + both_spellings(&kernel, "", "-w", "/dev/null", true).await; + let (_, code) = run(&kernel, "echo discard > /dev/null").await; + assert_eq!(code, 0, "/dev/null must accept the write it says it accepts"); +} + +/// A character device is not executable. +#[tokio::test] +async fn devfs_null_is_not_executable() { + let kernel = devfs_kernel(); + both_spellings(&kernel, "", "-x", "/dev/null", false).await; + both_spellings(&kernel, "", "-r", "/dev/null", true).await; +} + +/// The `/dev` *directory* is searchable but NOT writable, and that is a +/// deliberate one-bit divergence from Linux's 0755: kaish's `DevFs::mkdir` +/// and `DevFs::remove` refuse unconditionally, for every caller, because +/// kaish has no root user to be the exception 0755 carves out. `0o555` is +/// the mode that tells the truth about this mount. The `mkdir` below is the +/// receipt — `-w` and `mkdir` have to agree. +#[tokio::test] +async fn devfs_directory_is_searchable_but_not_writable() { + let kernel = devfs_kernel(); + both_spellings(&kernel, "", "-x", "/dev", true).await; + both_spellings(&kernel, "", "-r", "/dev", true).await; + both_spellings(&kernel, "", "-w", "/dev", false).await; + let (_, code) = run(&kernel, "mkdir /dev/newthing").await; + assert_ne!(code, 0, "/dev must refuse mkdir, as `-w /dev` said it would"); +} + +// ── LocalFs: real OS mode bits ───────────────────────────────────────────── + +/// True when the environment ignores mode restrictions — running as root, or +/// a filesystem/container that bypasses DAC. Detected empirically by trying +/// the thing, not by checking uid, so it covers every cause. +#[cfg(unix)] +fn dac_is_bypassed(dir: &std::path::Path) -> bool { + use std::os::unix::fs::PermissionsExt; + let probe = dir.join(".dac-probe"); + std::fs::write(&probe, b"x").unwrap(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o000)).unwrap(); + let bypassed = std::fs::File::open(&probe).is_ok(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o644)).unwrap(); + bypassed +} + +/// A writable LocalFs mount still has to honour the file's permissions — an +/// implementation that answered from the mount alone would call a mode-444 +/// file writable. +#[cfg(unix)] +#[tokio::test] +async fn localfs_permissions_still_decide() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("rw.txt"), b"hi\n").unwrap(); + std::fs::write(tmp.path().join("ro.txt"), b"hi\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("ro.txt"), + std::fs::Permissions::from_mode(0o444), + ) + .unwrap(); + + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + + let kernel = kernel_at(tmp.path()); + let rw = tmp.path().join("rw.txt"); + let ro = tmp.path().join("ro.txt"); + both_spellings(&kernel, "", "-w", &rw.display().to_string(), true).await; + both_spellings(&kernel, "", "-w", &ro.display().to_string(), false).await; +} + +/// **The case that was wrong before this change.** `0o222` means "some +/// principal may write", not "this process may write". +/// +/// A file the running user owns at mode `0o022` has the `0o222` mask set — +/// for group and other — while the owner class, which is the class that +/// applies to us, has no write bit. Unix checks the owner class and stops, +/// so the write fails. A mode-bit test says YES; the kernel says NO; the +/// `open()` below proves the kernel is right. +/// +/// This is the root-owned-`/etc/passwd` shape, reproduced without root: a +/// permissive bit that belongs to a principal we are not. `0o044` and `0o011` +/// do the same for `-r` and `-x`. +#[cfg(unix)] +#[tokio::test] +async fn permission_bits_for_another_principal_do_not_grant_access() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + + // (mode, op, the answer the raw mask would give, the true answer) + let cases = [ + (0o022u32, "-w", "w.txt"), + (0o044u32, "-r", "r.txt"), + (0o011u32, "-x", "x.txt"), + ]; + + let kernel = kernel_at(tmp.path()); + for (mode, op, name) in cases { + let path = tmp.path().join(name); + std::fs::write(&path, b"hi\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)).unwrap(); + + // The raw mask a mode-bit implementation would consult IS set. + let mask = match op { + "-w" => 0o222, + "-r" => 0o444, + _ => 0o111, + }; + assert_ne!( + mode & mask, + 0, + "fixture is pointless unless the raw mask is set for {name}", + ); + + both_spellings(&kernel, "", op, &path.display().to_string(), false).await; + + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + } +} + +/// The kernel really does refuse, so the answers above are not a theory. +#[cfg(unix)] +#[tokio::test] +async fn the_other_principal_fixture_really_is_refused() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + if dac_is_bypassed(tmp.path()) { + eprintln!("skipping: environment bypasses DAC (root?)"); + return; + } + let path = tmp.path().join("w.txt"); + std::fs::write(&path, b"hi\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o022)).unwrap(); + + let opened = std::fs::OpenOptions::new().append(true).open(&path); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert!( + opened.is_err(), + "mode 0o022 owned by us must refuse a write despite 0o222 being set", + ); +} + +/// `-x` on a real path keeps answering from the mode bits. Pinned so a later +/// symmetry argument cannot quietly move it. +#[cfg(unix)] +#[tokio::test] +async fn localfs_executable_bit_still_decides() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("run.sh"), b"#!/bin/sh\n").unwrap(); + std::fs::write(tmp.path().join("plain.txt"), b"hi\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("run.sh"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + + let kernel = kernel_at(tmp.path()); + let run_sh = tmp.path().join("run.sh"); + let plain = tmp.path().join("plain.txt"); + both_spellings(&kernel, "", "-x", &run_sh.display().to_string(), true).await; + both_spellings(&kernel, "", "-x", &plain.display().to_string(), false).await; +} + +/// No memory-backed *file* is executable. `real_path` is `None` for these +/// mounts, so there is nothing for exec(2) to open, and the modes say so: +/// MemoryFs files are `0o666` and DevFs devices are `0o666`. This is the +/// deliberate answer to "does `-x` mean anything on a memory-backed path" — +/// for files, no, and the mode is where that is written down. +/// +/// Directories are the exception, and not an inconsistency: `x` on a +/// directory means searchable, not executable, and these directories are +/// searchable. `memoryfs_directory_is_writable_and_searchable` pins that. +#[tokio::test] +async fn memory_backed_files_are_not_executable() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + both_spellings(&kernel, "echo hi > /v/probe.txt", "-x", "/v/probe.txt", false).await; + // a backend that models no permissions at all: not executable either + both_spellings(&kernel, "", "-x", "/v/bin/echo", false).await; +} + +// ── The hazard: read-only wrapper over an OS-writable directory ──────────── + +/// Build a kernel whose `/` is a `LocalFs::read_only` wrapper over a real, +/// OS-writable tempdir — the shape kaijutsu embeds. +fn read_only_wrapper_kernel(dir: &std::path::Path) -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/", LocalFs::read_only(dir)); + vfs.mount("/v", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + +/// The hazard from the design note: `LocalFs::stat` reports the real OS mode +/// bits and knows nothing about the read-only wrapper around it, so a raw +/// stat check calls a mode-644 file writable while every write to it fails. +/// An implementation that consults only the stat fails here; one that +/// consults only the mount passes here and fails `localfs_mode_bits_still_decide`. +#[cfg(unix)] +#[tokio::test] +async fn read_only_wrapper_over_writable_os_dir_is_not_writable() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("file.txt"), b"hi\n").unwrap(); + + // The OS says 0o644. The wrapper says no. + let mode = { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(tmp.path().join("file.txt")) + .unwrap() + .permissions() + .mode() + }; + assert_ne!(mode & 0o222, 0, "fixture must be OS-writable to be the hazard"); + + let kernel = read_only_wrapper_kernel(tmp.path()); + both_spellings(&kernel, "", "-w", "/file.txt", false).await; +} + +/// The wrapper really does refuse the write — the assertion above is about +/// the same file this one fails to write. +#[tokio::test] +async fn read_only_wrapper_really_refuses_a_write() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("file.txt"), b"hi\n").unwrap(); + let kernel = read_only_wrapper_kernel(tmp.path()); + let (_, code) = run(&kernel, "echo nope > /file.txt").await; + assert_ne!(code, 0, "a read-only LocalFs must refuse the write"); +} + +/// The read-only wrapper is still readable, and its `-x` still comes from +/// the OS mode bits — the wrapper is about writes only. +#[cfg(unix)] +#[tokio::test] +async fn read_only_wrapper_keeps_read_and_execute_answers() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("run.sh"), b"#!/bin/sh\n").unwrap(); + std::fs::set_permissions( + tmp.path().join("run.sh"), + std::fs::Permissions::from_mode(0o555), + ) + .unwrap(); + + let kernel = read_only_wrapper_kernel(tmp.path()); + both_spellings(&kernel, "", "-r", "/run.sh", true).await; + both_spellings(&kernel, "", "-x", "/run.sh", true).await; + both_spellings(&kernel, "", "-w", "/run.sh", false).await; +} + +// ── Absent paths ─────────────────────────────────────────────────────────── + +/// A path that does not exist is not readable, writable, or executable — +/// `path_access` reports `stat`'s error and the file test reads that as +/// false, on a writable mount and a read-only one alike. +#[tokio::test] +async fn missing_paths_answer_false_everywhere() { + let tmp = tempfile::tempdir().unwrap(); + let kernel = kernel_at(tmp.path()); + for op in ["-r", "-w", "-x"] { + both_spellings(&kernel, "", op, "/v/nope.txt", false).await; + both_spellings(&kernel, "", op, "/v/bin/definitely-not-a-builtin", false).await; + } +} + +// ── Synthesized directories (no mount of their own) ──────────────────────── + +/// A router with no `/` mount, only a nested one — the shape the +/// `Kernel::with_backend` doc example builds (`vfs.mount_arc("/v/docs", …)`). +fn nested_mount_only_kernel() -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount("/v/docs", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + Kernel::with_backend(backend, KernelConfig::isolated(), |_| {}, |_| {}) + .expect("with_backend kernel") +} + +/// `VfsRouter::stat` synthesizes a directory for the root and for any +/// ancestor of a mount, so `-e /v` is true even though nothing is mounted +/// there. `-r` and `-x` have to agree with `-e` about the same path: a +/// `path_access` that went straight to `find_mount` would error where `stat` +/// succeeds, and `[[ -e /v ]] && [[ -r /v ]]` would answer true then false. +#[tokio::test] +async fn synthesized_ancestor_directories_are_readable_and_searchable() { + let kernel = nested_mount_only_kernel(); + for path in ["/", "/v"] { + both_spellings(&kernel, "", "-e", path, true).await; + both_spellings(&kernel, "", "-d", path, true).await; + both_spellings(&kernel, "", "-r", path, true).await; + both_spellings(&kernel, "", "-x", path, true).await; + // The router creates nothing in a directory it synthesized. + both_spellings(&kernel, "", "-w", path, false).await; + } +} + +/// The real mount underneath still answers for itself. +#[tokio::test] +async fn a_real_mount_under_a_synthesized_ancestor_still_answers() { + let kernel = nested_mount_only_kernel(); + both_spellings(&kernel, "", "-w", "/v/docs", true).await; + let (_, code) = run(&kernel, "echo hi > /v/docs/note.txt").await; + assert_eq!(code, 0, "the real mount must accept the write"); + both_spellings(&kernel, "", "-w", "/v/docs/note.txt", true).await; +} diff --git a/crates/kaish-tool-api/src/backend.rs b/crates/kaish-tool-api/src/backend.rs index e2837406..7c235d8b 100644 --- a/crates/kaish-tool-api/src/backend.rs +++ b/crates/kaish-tool-api/src/backend.rs @@ -12,7 +12,7 @@ use async_trait::async_trait; use kaish_types::backend::{ BackendResult, MountInfo, PatchOp, ReadRange, ToolInfo, ToolResult, WriteMode, }; -use kaish_types::{DirEntry, ToolArgs}; +use kaish_types::{DirEntry, PathAccess, ToolArgs}; use crate::ctx::ToolCtx; @@ -118,6 +118,30 @@ pub trait KernelBackend: Send + Sync { /// Returns true if this backend is read-only. fn read_only(&self) -> bool; + /// What the kernel can do with one path: the query behind `test -r`, + /// `test -w`, and `test -x`. + /// + /// Neither [`KernelBackend::read_only`] nor `DirEntry.permissions` + /// answers "can this path be written" alone. A read-only wrapper over an + /// OS-writable directory reports permissive mode bits and refuses every + /// write; a `DevFs` mount reports `read_only() == false` (so `> + /// /dev/null` works) while its `/dev` directory accepts nothing. + /// [`PathAccess::resolve`] combines the two, and is the only way to build + /// a `PathAccess` — a caller cannot consult one fact by accident. + /// + /// The default answers from `stat` plus this backend's whole-backend + /// `read_only()`, which is right for a backend that is uniformly + /// read-only or uniformly writable. A backend whose mounts differ — + /// `LocalBackend`, which routes through a `VfsRouter` — overrides this to + /// ask the mount that owns the path. + /// + /// Errors exactly as `stat` does: a path that does not exist is an error, + /// not a `PathAccess` of all-false. + async fn path_access(&self, path: &Path) -> BackendResult { + let entry = self.stat(path).await?; + Ok(PathAccess::resolve(entry.permissions, self.read_only())) + } + /// Returns the backend type identifier (e.g. "local", "kaijutsu"). fn backend_type(&self) -> &str; diff --git a/crates/kaish-types/src/lib.rs b/crates/kaish-types/src/lib.rs index b5f935dd..5d0cd719 100644 --- a/crates/kaish-types/src/lib.rs +++ b/crates/kaish-types/src/lib.rs @@ -12,6 +12,7 @@ pub mod dir_entry; pub mod job; pub mod kernel; pub mod output; +pub mod path_access; // `plan` is deliberately NOT in the flat re-export block below: its names // are generic (Plan, PlannedCommand, PlannedValue) and would collide at the // crate root. Consumers write `kaish_types::plan::Plan`, like `clock`. @@ -29,6 +30,7 @@ pub use dir_entry::*; pub use job::*; pub use kernel::*; pub use output::*; +pub use path_access::*; pub use result::*; pub use tool::*; pub use value::*; diff --git a/crates/kaish-types/src/path_access.rs b/crates/kaish-types/src/path_access.rs new file mode 100644 index 00000000..3429b2a6 --- /dev/null +++ b/crates/kaish-types/src/path_access.rs @@ -0,0 +1,141 @@ +//! What the kernel can do with one path. + +/// What the operating system says *this process* may do with a path. +/// +/// The answer to `faccessat(..., AT_EACCESS)` — an access check against the +/// effective uid and gid, the same primitive `bash`'s `test -w` uses. Mode +/// bits answer a different question: `0o222` means "some principal may +/// write", and a root-owned `0o644` file has it set for a process that +/// cannot write a byte. Only the OS knows the process's identity. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct EffectiveAccess { + /// The process may read the path. + pub read: bool, + /// The process may write the path. + pub write: bool, + /// The process may execute the path, or search it if it is a directory. + pub execute: bool, +} + +/// Whether the kernel can read, write, or execute a path. +/// +/// A file test needs two facts and neither answers alone. A +/// `LocalFs::read_only` wrapper over an OS-writable directory reports mode +/// bits with the write bit set, because `LocalFs::stat` asks the OS and the +/// OS does not know about the wrapper. `DevFs::read_only()` is `false` so +/// that `> /dev/null` works, while `mkdir /dev/x` is refused for every +/// caller — only the mode separates those two. +/// +/// [`PathAccess::resolve`] is the only constructor, so no caller answers +/// from one fact by accident. Read the fields; the struct is +/// `#[non_exhaustive]`. +/// +/// An absent `DirEntry.permissions` reads as readable, not writable, not +/// executable. Every writable backend here reports a mode, so `None` means a +/// backend that does not model permissions, and all of those are read-only. +/// **A writable backend reporting `None` will be told its paths are +/// unwritable** — see `docs/EMBEDDING.md`, "Reporting file permissions". +#[non_exhaustive] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PathAccess { + /// The path's contents can be read. A read-only mount is readable. + pub readable: bool, + /// The path can be written. Needs both facts to agree: false whenever the + /// owning mount is read-only, whatever the mode says, and false whenever + /// the mode clears `0o222`, whatever the mount says. + pub writable: bool, + /// The path can be executed, or — on a directory — searched. False when + /// the mount reports no mode. + pub executable: bool, +} + +impl PathAccess { + /// Combine a mount's read-only state with the mode bits it reports for + /// one path. + /// + /// `mode` is `DirEntry.permissions`; `mount_read_only` is the + /// `read_only()` of the mount that owns the path, not of the whole + /// router. + /// + /// An absent mode still reads: a backend that does not model permissions + /// does not restrict reads. Writes need both facts to agree. Exec needs a + /// mode — absent means there is nothing here to run. On a directory + /// `0o111` is searchable, which is what `test -x DIR` answers. + pub fn resolve(mode: Option, mount_read_only: bool) -> Self { + Self { + readable: mode.is_none_or(|p| p & 0o444 != 0), + writable: !mount_read_only && mode.is_some_and(|p| p & 0o222 != 0), + executable: mode.is_some_and(|p| p & 0o111 != 0), + } + } + + /// Combine the OS's effective-access answer with a mount's read-only + /// state. + /// + /// The accurate constructor for a backend whose paths are real OS paths, + /// which is only `LocalFs`. A file test asks "may this process do this", + /// and [`PathAccess::resolve`]'s mode bits cannot answer that. + /// + /// `mount_read_only` is still ANDed into `writable`: a `LocalFs::read_only` + /// wrapper is a kaish-level restriction the OS cannot see. + pub fn from_effective_access(access: EffectiveAccess, mount_read_only: bool) -> Self { + Self { + readable: access.read, + writable: !mount_read_only && access.write, + executable: access.execute, + } + } +} + +#[cfg(test)] +mod tests { + use super::PathAccess; + + /// A backend that models no permissions reads, and nothing else — the + /// mount being writable does not make up for an absent mode, because + /// every writable backend in the workspace reports one. + #[test] + fn absent_mode_reads_and_nothing_else() { + for mount_read_only in [false, true] { + let access = PathAccess::resolve(None, mount_read_only); + assert!(access.readable, "read-only is about writes"); + assert!(!access.writable, "absent mode means read-only backend"); + assert!(!access.executable, "nothing here to hand exec(2)"); + } + } + + /// A directory mode of `0o777` (MemoryFs) is writable and searchable; + /// `0o555` (the `/dev` directory) is searchable and not writable. The + /// pair is the whole reason DevFs needed a mode of its own. + #[test] + fn directory_modes_separate_searchable_from_writable() { + let memory_dir = PathAccess::resolve(Some(0o777), false); + assert!(memory_dir.writable); + assert!(memory_dir.executable, "0o111 on a directory is searchable"); + + let dev_dir = PathAccess::resolve(Some(0o555), false); + assert!(!dev_dir.writable, "/dev accepts no mkdir"); + assert!(dev_dir.executable); + assert!(dev_dir.readable); + } + + /// A writable mount still honours the mode bits it reports. + #[test] + fn mode_bits_decide_on_a_writable_mount() { + assert!(PathAccess::resolve(Some(0o644), false).writable); + assert!(!PathAccess::resolve(Some(0o444), false).writable); + assert!(PathAccess::resolve(Some(0o755), false).executable); + assert!(!PathAccess::resolve(Some(0o644), false).executable); + assert!(!PathAccess::resolve(Some(0o000), false).readable); + } + + /// The hazard: a read-only wrapper over an OS-writable file. The mode + /// says yes and the mount says no; both must agree for a yes. + #[test] + fn read_only_mount_overrides_a_writable_mode() { + let access = PathAccess::resolve(Some(0o755), true); + assert!(!access.writable, "the mount's read-only state must win"); + assert!(access.readable, "read-only is about writes"); + assert!(access.executable, "read-only says nothing about exec"); + } +} diff --git a/crates/kaish-vfs/Cargo.toml b/crates/kaish-vfs/Cargo.toml index 3881c506..a9edda90 100644 --- a/crates/kaish-vfs/Cargo.toml +++ b/crates/kaish-vfs/Cargo.toml @@ -24,6 +24,15 @@ getrandom = { workspace = true } # `MemoryFs` needs only `tokio/sync` (no runtime, works on wasm). tokio = { version = "1", features = [], optional = true } +# `LocalFs::path_access` asks the OS whether THIS process can read/write/exec a +# path (faccessat with AT_EACCESS), which mode bits cannot answer. Unix-only: +# wasm32-wasip1 has no effective-uid model and takes the synthesized-mode path. +# rustix rather than libc because `unsafe_code = "deny"` workspace-wide; it is +# already a normal dependency in this tree via rustyline, procfs and +# terminal_size, so this adds no new supply chain. +[target.'cfg(unix)'.dependencies] +rustix = { version = "1.1", features = ["fs"] } + [features] default = [] # The real-filesystem backend. Off → only the `Filesystem` trait + `DirEntry`, diff --git a/crates/kaish-vfs/src/dev.rs b/crates/kaish-vfs/src/dev.rs index 5c923eca..eb5c1701 100644 --- a/crates/kaish-vfs/src/dev.rs +++ b/crates/kaish-vfs/src/dev.rs @@ -98,13 +98,28 @@ impl DevFs { ) } + /// Mode reported for a device node, matching `crw-rw-rw-` on Linux: + /// everyone reads, everyone writes, nobody executes. `write` accepts and + /// discards for every device, so the write bit is the truth. + pub const DEVICE_MODE: u32 = 0o666; + + /// Mode reported for the `/dev` directory itself: searchable and + /// readable, **not** writable. + /// + /// Linux ships `/dev` as 0755, but that write bit is for root, and kaish + /// has no root: `mkdir` and `remove` below refuse every caller + /// unconditionally. 0755 would make `test -w /dev` answer YES about a + /// directory that accepts nothing, which is the exact failure this mode + /// exists to prevent. 0555 is the mode that tells the truth here. + pub const DIRECTORY_MODE: u32 = 0o555; + fn entry(name: &str) -> DirEntry { DirEntry { name: name.to_string(), kind: DirEntryKind::File, size: 0, modified: None, - permissions: None, + permissions: Some(Self::DEVICE_MODE), symlink_target: None, } } @@ -191,7 +206,7 @@ impl Filesystem for DevFs { kind: DirEntryKind::Directory, size: 0, modified: None, - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -220,7 +235,9 @@ impl Filesystem for DevFs { fn read_only(&self) -> bool { // Writes to the devices "succeed" (they discard), so the mount is not // read-only in the sense the router cares about — refusing a write - // would break `> /dev/null`. + // would break `> /dev/null`. The consequence is that the mount cannot + // answer `test -w` here and the modes have to: DEVICE_MODE is + // writable, DIRECTORY_MODE is not. false } } diff --git a/crates/kaish-vfs/src/lib.rs b/crates/kaish-vfs/src/lib.rs index ddf34d93..8094d960 100644 --- a/crates/kaish-vfs/src/lib.rs +++ b/crates/kaish-vfs/src/lib.rs @@ -14,7 +14,7 @@ mod traits; pub use budget::ByteBudget; pub use dev::DevFs; -pub use traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; +pub use traits::{DirEntry, DirEntryKind, EffectiveAccess, Filesystem, PathAccess, ReadRange}; // `LocalFs` pulls in `tokio/fs`; gated so the in-memory/wasm sandbox build // (which doesn't enable `localfs`) stays free of a real-filesystem dependency. diff --git a/crates/kaish-vfs/src/local.rs b/crates/kaish-vfs/src/local.rs index 2d179a72..110d32ea 100644 --- a/crates/kaish-vfs/src/local.rs +++ b/crates/kaish-vfs/src/local.rs @@ -2,7 +2,7 @@ //! //! Provides access to real filesystem paths, with optional read-only mode. -use crate::traits::{DirEntry, DirEntryKind, Filesystem, ReadRange}; +use crate::traits::{DirEntry, DirEntryKind, EffectiveAccess, Filesystem, PathAccess, ReadRange}; use async_trait::async_trait; use std::io; use std::path::{Path, PathBuf}; @@ -195,9 +195,56 @@ impl LocalFs { Some(meta.permissions().mode()) } + /// Synthesize a Unix-shaped mode from the one permission fact a non-Unix + /// platform exposes. + /// + /// Reached on a non-Unix target that enables `localfs` — Windows in + /// practice. Not WASI: `localfs` pulls `tokio/fs`, which wasm rejects, so + /// `wasm32-wasip1` never compiles `LocalFs` at all. + /// + /// `LocalFs` is writable, so returning `None` would make every file test + /// answer "not writable" (`PathAccess::resolve` reads an absent mode as + /// read-only). `Permissions::readonly()` is the only bit to work from. + /// The `x` bit is never set: executability is decided by file extension + /// on these platforms, so claiming it would be a fabrication. #[cfg(not(unix))] - fn extract_permissions(_meta: &std::fs::Metadata) -> Option { - None + fn extract_permissions(meta: &std::fs::Metadata) -> Option { + Some(Self::synthesized_mode(meta.is_dir(), meta.permissions().readonly())) + } + + /// Ask the OS whether this process may read, write, and execute `full`. + /// + /// `faccessat(AT_FDCWD, full, ..., AT_EACCESS)` — an access check against + /// the effective uid/gid, the same primitive `bash`'s `test -w` uses. + /// rustix rather than `libc` because `unsafe_code` is denied + /// workspace-wide. + #[cfg(unix)] + fn effective_access(full: &Path) -> EffectiveAccess { + use rustix::fs::{Access, AtFlags, accessat}; + use rustix::fs::CWD; + + let ask = |mode: Access| { + accessat(CWD, full, mode, AtFlags::EACCESS).is_ok() + }; + EffectiveAccess { + read: ask(Access::READ_OK), + write: ask(Access::WRITE_OK), + execute: ask(Access::EXEC_OK), + } + } + + /// The mode [`extract_permissions`](Self::extract_permissions) reports on + /// a platform with no Unix mode bits. Split out from the `cfg` so it can + /// be tested everywhere, including the Unix targets that never call it. + #[cfg_attr(unix, allow(dead_code))] + pub(crate) fn synthesized_mode(is_dir: bool, readonly: bool) -> u32 { + match (is_dir, readonly) { + // Searchable, and writable unless the read-only attribute is set. + (true, false) => 0o777, + (true, true) => 0o555, + (false, false) => 0o666, + (false, true) => 0o444, + } } /// Build a [`DirEntry`] for one directory member named by `path`, *without* @@ -529,6 +576,31 @@ impl Filesystem for LocalFs { fs::rename(&from_path, &to_path).await } + /// Answers from the OS, not from mode bits — the one backend that can, + /// and so the one backend that does not use [`PathAccess::resolve`]. + /// + /// Mode bits say whether *some* principal may write. A root-owned `0o644` + /// file has the bit set for a kaish running as an ordinary user who cannot + /// write a byte, and `test -w` would have said yes. Every other backend + /// models modes we chose ourselves, where the bits are the whole truth. + /// + /// `from_effective_access` still ANDs in the read-only wrapper, which the + /// OS cannot see. On non-Unix this falls through to the trait default over + /// `synthesized_mode`. + #[cfg(unix)] + async fn path_access(&self, path: &Path) -> io::Result { + let full = self.resolve(path)?; + // Stat first so a missing path errors exactly the way `stat` does — + // `faccessat` would report a plain EACCES/ENOENT with no distinction + // the callers can use, and the trait contract is "errors as stat". + let _ = fs::metadata(&full).await?; + let access = + tokio::task::spawn_blocking(move || Self::effective_access(&full)) + .await + .map_err(io::Error::other)?; + Ok(PathAccess::from_effective_access(access, self.read_only)) + } + fn read_only(&self) -> bool { self.read_only } @@ -699,6 +771,26 @@ mod tests { cleanup(&dir).await; } + // Without this, `LocalFs` on Windows would be a writable backend + // reporting an absent mode, and `test -w` would answer NO about every + // file there. + #[test] + fn synthesized_mode_keeps_writability_and_never_claims_exec() { + assert_eq!(LocalFs::synthesized_mode(false, false) & 0o222, 0o222); + assert_eq!(LocalFs::synthesized_mode(false, true) & 0o222, 0); + assert_eq!(LocalFs::synthesized_mode(true, false) & 0o222, 0o222); + assert_eq!(LocalFs::synthesized_mode(true, true) & 0o222, 0); + // Everything readable, whatever the read-only attribute says. + for (is_dir, readonly) in [(false, false), (false, true), (true, false), (true, true)] { + assert_ne!(LocalFs::synthesized_mode(is_dir, readonly) & 0o444, 0); + } + // Only directories get the x bit, and it means searchable. + assert_eq!(LocalFs::synthesized_mode(false, false) & 0o111, 0); + assert_eq!(LocalFs::synthesized_mode(false, true) & 0o111, 0); + assert_ne!(LocalFs::synthesized_mode(true, false) & 0o111, 0); + assert_ne!(LocalFs::synthesized_mode(true, true) & 0o111, 0); + } + #[tokio::test] async fn test_read_only() { let (_, dir) = setup().await; diff --git a/crates/kaish-vfs/src/memory.rs b/crates/kaish-vfs/src/memory.rs index 8fa4dd08..de929625 100644 --- a/crates/kaish-vfs/src/memory.rs +++ b/crates/kaish-vfs/src/memory.rs @@ -114,6 +114,40 @@ impl MemoryFs { } /// Maximum symlink follow depth (matches Linux ELOOP limit). + /// Mode reported for a `MemoryFs` directory: readable, writable, and + /// searchable by everyone. + /// + /// `MemoryFs` has no permission model — it never refuses an operation + /// over a mode — so these constants describe what it actually does + /// rather than restricting anything. Reporting them (instead of `None`) + /// is what lets `test -w` close its absent-mode default: with every + /// writable backend reporting a mode, an absent mode means "this backend + /// does not model permissions", and every backend still in that position + /// is read-only. There is no `chmod` builtin; these are observations. + pub const DIRECTORY_MODE: u32 = 0o777; + + /// Mode reported for a `MemoryFs` file: readable and writable, never + /// executable. `real_path` is `None` for a memory-backed path, so there + /// is nothing for exec(2) to open and the `x` bit would be a lie. + pub const FILE_MODE: u32 = 0o666; + + /// Mode reported for a `MemoryFs` symlink, matching `lrwxrwxrwx` on + /// Linux. Only `lstat` and `list` ever show it — `stat` follows the link + /// and reports the target's mode. + pub const SYMLINK_MODE: u32 = 0o777; + + /// The mode this filesystem reports for an entry of `kind`. + fn mode_for(kind: DirEntryKind) -> Option { + Some(match kind { + DirEntryKind::Directory => Self::DIRECTORY_MODE, + DirEntryKind::File => Self::FILE_MODE, + DirEntryKind::Symlink => Self::SYMLINK_MODE, + // A kind this crate does not know about gets the conservative + // answer rather than a guess: not writable, not executable. + _ => 0o444, + }) + } + const MAX_SYMLINK_DEPTH: usize = 40; /// Read a file, following symlinks with depth limit. @@ -210,7 +244,7 @@ impl MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(system_now()), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -224,7 +258,7 @@ impl MemoryFs { kind: DirEntryKind::File, size: data.len() as u64, modified: Some(*modified), - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }, None, @@ -235,7 +269,7 @@ impl MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }, None, @@ -246,7 +280,7 @@ impl MemoryFs { kind: DirEntryKind::File, // placeholder, will be overridden size: 0, modified: None, - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }, Some(target.clone()), @@ -490,7 +524,7 @@ impl Filesystem for MemoryFs { kind, size, modified, - permissions: None, + permissions: Self::mode_for(kind), symlink_target, }); } @@ -529,7 +563,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(system_now()), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }); } @@ -540,7 +574,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::File, size: data.len() as u64, modified: Some(*modified), - permissions: None, + permissions: Some(Self::FILE_MODE), symlink_target: None, }), Some(Entry::Directory { modified }) => Ok(DirEntry { @@ -548,7 +582,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Directory, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::DIRECTORY_MODE), symlink_target: None, }), Some(Entry::Symlink { target, modified }) => Ok(DirEntry { @@ -556,7 +590,7 @@ impl Filesystem for MemoryFs { kind: DirEntryKind::Symlink, size: 0, modified: Some(*modified), - permissions: None, + permissions: Some(Self::SYMLINK_MODE), symlink_target: Some(target.clone()), }), None => Err(io::Error::new( diff --git a/crates/kaish-vfs/src/overlay.rs b/crates/kaish-vfs/src/overlay.rs index c64fffa4..bf2e926a 100644 --- a/crates/kaish-vfs/src/overlay.rs +++ b/crates/kaish-vfs/src/overlay.rs @@ -2033,6 +2033,17 @@ mod tests { use super::*; use crate::local::LocalFs; + // A whiteouted path is gone, so it has no access at all — not a + // PathAccess of all-false, an error, the same as stat. + #[tokio::test] + async fn path_access_errors_on_a_whiteouted_path() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("gone.txt"), b"x").unwrap(); + let overlay = OverlayFs::over(Arc::new(LocalFs::read_only(dir.path()))); + overlay.remove(Path::new("gone.txt")).await.unwrap(); + assert!(overlay.path_access(Path::new("gone.txt")).await.is_err()); + } + #[tokio::test] async fn test_real_tree_byte_identical_after_overlay_writes() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/kaish-vfs/src/traits.rs b/crates/kaish-vfs/src/traits.rs index efdda821..8842fed3 100644 --- a/crates/kaish-vfs/src/traits.rs +++ b/crates/kaish-vfs/src/traits.rs @@ -6,7 +6,7 @@ use std::path::{Path, PathBuf}; use std::time::SystemTime; // DirEntry and DirEntryKind live in kaish-types. -pub use kaish_types::{DirEntry, DirEntryKind, ReadRange}; +pub use kaish_types::{DirEntry, DirEntryKind, EffectiveAccess, PathAccess, ReadRange}; /// Abstract filesystem interface. /// @@ -94,6 +94,34 @@ pub trait Filesystem: Send + Sync { /// Returns true if this filesystem is read-only. fn read_only(&self) -> bool; + /// What the kernel can do with one path on this filesystem. + /// + /// The query behind `test -r`, `test -w`, and `test -x`. Neither + /// [`Filesystem::read_only`] nor `DirEntry.permissions` answers on its + /// own — `MemoryFs` (writable) and `JobFs` (read-only) both report + /// `permissions: None`, and a `LocalFs::read_only` wrapper over an + /// OS-writable directory reports the write bit set. [`PathAccess::resolve`] + /// is where the two combine. + /// + /// The default is right for a filesystem that is uniformly read-only or + /// uniformly writable. `VfsRouter` overrides it to ask the mount that owns + /// the path. + /// + /// A backend that can be written must report a mode; an absent one is read + /// as read-only, and nothing checks that for you. See `docs/EMBEDDING.md`, + /// "Reporting file permissions". + /// + /// `OverlayFs` keeps the default and inherits its one inaccuracy: writes + /// always land in the upper, so a lower file whose mode clears `0o222` + /// reports unwritable while copy-up would write it. + /// + /// Errors exactly as `stat` does: a path that does not exist is an error, + /// not a `PathAccess` of all-false. + async fn path_access(&self, path: &Path) -> io::Result { + let entry = self.stat(path).await?; + Ok(PathAccess::resolve(entry.permissions, self.read_only())) + } + /// Memory-resident content bytes this filesystem is holding, if it /// tracks them. /// diff --git a/docs/EMBEDDING.md b/docs/EMBEDDING.md index 194a201b..77499cac 100644 --- a/docs/EMBEDDING.md +++ b/docs/EMBEDDING.md @@ -371,6 +371,40 @@ vfs.mount("/", MemoryFs::with_budget(budget.clone())); // budget.used() / budget.remaining() are observable at any time. ``` +### Reporting file permissions (`path_access`) + +`test -r`, `test -w`, and `test -x` ask the mount that owns the path, through +`Filesystem::path_access`. The default implementation combines two facts — +`DirEntry.permissions` from `stat`, and the mount's own `read_only()` — in +`PathAccess::resolve`. `VfsRouter` overrides it to route the question to the +owning mount; `LocalFs` overrides it to ask the OS for an effective-access +answer, because mode bits say "some principal may write" where a file test has +to answer "may this process write". + +Report real modes from `stat` and `list` unless your backend is read-only. An +absent mode is not a neutral default here — it is read as a statement. Who +answers from what today: + +| Backend | Modes | +|---|---| +| `LocalFs` | The OS's effective-access answer on Unix; synthesized from `Permissions::readonly()` on a non-Unix target | +| `MemoryFs` | Constants: dir `0o777`, file `0o666`, symlink `0o777` | +| `DevFs` | Constants: device `0o666`, the `/dev` directory `0o555` | +| `OverlayFs` | Whichever layer holds the path | +| `VfsRouter` | The owning mount; `0o555` for directories it synthesizes | +| `BuiltinFs`, `JobFs` | None — and both are read-only | + +That last row is load-bearing. `PathAccess::resolve` treats an absent mode as +not writable, and that is correct only because every backend still reporting +`None` is read-only. **A writable backend that reports `None` will have every +one of its paths called unwritable** — `test -w` says no, and the write that +follows succeeds anyway. + +Nothing catches that for you. No assertion ties `read_only() == false` to +reporting a mode, and the failure is a wrong answer rather than an error, so +your backend's own tests will pass. If you add a writable backend, either +report a mode or change `resolve` and this table together. + ### Output Limits and Spill Mode (`OutputLimitConfig`) `KernelConfig::output_limit` caps how much a single command's output can grow diff --git a/docs/LANGUAGE.md b/docs/LANGUAGE.md index 2873d12d..fd5c9c55 100644 --- a/docs/LANGUAGE.md +++ b/docs/LANGUAGE.md @@ -629,8 +629,8 @@ mkdir /tmp/work && cd /tmp/work && echo "ready" [[ -d /path/dir ]] # is directory [[ -e /path/any ]] # exists [[ -r /path/file ]] # readable -[[ -w /path/file ]] # writable -[[ -x /path/file ]] # executable +[[ -w /path/file ]] # writable — the mount and the mode must agree +[[ -x /path/file ]] # executable; on a directory, searchable # String tests [[ -z $VAR ]] # empty