From f5034c0e4b6ee0204f1f5118b6d3b1919d04b1be Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 30 Sep 2026 09:09:09 +0900 Subject: [PATCH] fix: bound filesystem execution and preserve guarantees --- crates/wip-fs-server/README.md | 14 +- crates/wip-fs-server/src/lib.rs | 12 +- crates/wip-fs-server/src/projection.rs | 182 +++++++++++++++++++---- crates/wip-fs-server/src/server.rs | 30 ++-- crates/wip-fs-server/tests/end_to_end.rs | 81 +++++++++- 5 files changed, 263 insertions(+), 56 deletions(-) diff --git a/crates/wip-fs-server/README.md b/crates/wip-fs-server/README.md index 339688e..b2685b9 100644 --- a/crates/wip-fs-server/README.md +++ b/crates/wip-fs-server/README.md @@ -8,14 +8,19 @@ envelopes, batching, and HTTP status mapping. ## Security model -- The effective root is opened once as a directory capability. Every request is - resolved segment-by-segment with directory-relative, `O_NOFOLLOW` handles. +- The effective root is opened once as a directory capability. Root path + components must be real directories (symlink components are rejected), and the + displayed effective root is read back from that same pinned descriptor. Every + request is resolved segment-by-segment with directory-relative, `O_NOFOLLOW` + handles. - Symlinks are projected as links and are never followed. Their stored target is available only through the `SymlinkInspect.read_link` application operation. - Regular files, directories, symlinks, and unsupported entry types have distinct public descriptions and capability sets. - WIP names are Unicode. Linux names that are not valid UTF-8 are deliberately not published; valid UTF-8 names map byte-for-byte and therefore cannot collide. + Unicode paths containing backend-invalid NUL bytes are likewise treated as + unpublished rather than as backend failures. - No object `ref` is issued. This avoids claiming representation identity for hard links or path-specific names. Regular files and symlinks receive an opaque validator containing filesystem identity plus protected metadata; replacing a @@ -122,8 +127,9 @@ standard `POST /wip/v1/batch` route. `call_operation` is never batchable. `ResourceLimits` bounds request/response bytes, JSON nesting, batch items, file read/write bytes, tree depth/node count, pinned handles, concurrent requests, and -a cooperative deadline. Tree traversal is breadth-first and opens only requested -entries. Once a limit or depth boundary is reached it reads only far enough to +a cooperative deadline. Advisory file-lock acquisition uses a nonblocking loop and +checks that deadline/cancellation rather than allowing a contended lock to stall +a worker. Tree traversal is breadth-first and opens only requested entries. Once a limit or depth boundary is reached it reads only far enough to prove whether an additional visible child exists, so `truncated` is true only when something was actually omitted. I/O failure and cancellation remain errors, not clean exhaustion. diff --git a/crates/wip-fs-server/src/lib.rs b/crates/wip-fs-server/src/lib.rs index e4411f1..cb5124b 100644 --- a/crates/wip-fs-server/src/lib.rs +++ b/crates/wip-fs-server/src/lib.rs @@ -18,7 +18,7 @@ mod projection; mod server; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::time::Duration; pub use interfaces::{FileRead, FileWrite, SymlinkInspect}; @@ -138,10 +138,6 @@ impl ServerConfig { Ok(()) } - pub(crate) fn effective_root(&self) -> Result { - std::fs::canonicalize(&self.root) - } - pub(crate) fn endpoint(&self) -> Result { let endpoint = format!("https://localhost{}", self.endpoint_base_path); wip_https::Endpoint::parse(&endpoint).map_err(|_| ConfigError::InvalidBasePath) @@ -189,9 +185,3 @@ fn valid_base_path(path: &str) -> bool { .any(|segment| segment == "." || segment == "..") && (path == "/" || !path.ends_with('/')) } - -/// Returns whether `root` is an existing directory suitable for projection. -#[must_use] -pub fn is_projectable_root(root: &Path) -> bool { - root.is_dir() -} diff --git a/crates/wip-fs-server/src/projection.rs b/crates/wip-fs-server/src/projection.rs index aa65395..22dec17 100644 --- a/crates/wip-fs-server/src/projection.rs +++ b/crates/wip-fs-server/src/projection.rs @@ -2,6 +2,7 @@ use std::collections::VecDeque; use std::ffi::CStr; use std::mem::MaybeUninit; use std::os::fd::{AsRawFd, OwnedFd}; +use std::path::{Component, Path, PathBuf}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; @@ -173,7 +174,6 @@ pub(crate) struct Projection { access: AccessMode, limits: ResourceLimits, handles: Arc, - write_gate: Mutex<()>, injection: Mutex, } @@ -183,13 +183,7 @@ impl Projection { access: AccessMode, limits: ResourceLimits, ) -> Result { - let root = openat( - CWD, - root, - OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, - FsMode::empty(), - ) - .map_err(std::io::Error::from)?; + let root = open_root(root)?; let mut registry = InterfaceRegistry::new(); let read_interface = registry .register(FileReadDefinition, None) @@ -212,11 +206,14 @@ impl Projection { current: AtomicUsize::new(0), maximum: limits.max_open_handles, }), - write_gate: Mutex::new(()), injection: Mutex::new(FailureInjection::None), }) } + pub(crate) fn effective_root(&self) -> Result { + std::fs::read_link(format!("/proc/self/fd/{}", self.root.as_raw_fd())) + } + pub(crate) fn fetch_interface( &self, request: &FetchInterfaceRequest, @@ -367,6 +364,9 @@ impl Projection { let mut segments = path[1..].split('/').peekable(); while let Some(segment) = segments.next() { check_context(context)?; + if segment.as_bytes().contains(&0) { + return Err(ProjectionError::NotFound); + } let probe = openat( &parent, segment, @@ -486,15 +486,6 @@ impl ObjectProvider for Projection { path: &str, operation: impl FnOnce(&PublishedObject) -> OperationAttempt, ) -> ProviderResult { - let _gate = match self.write_gate.lock() { - Ok(gate) => gate, - Err(_) => { - return ProviderResult::Failure(HostError::internal( - "filesystem operation lock failed", - EffectGuarantee::NotCommitted, - )); - } - }; let entry = match self.resolve(context, path) { Ok(entry) => entry, Err(ProjectionError::NotFound) => return ProviderResult::NotFound, @@ -511,13 +502,22 @@ impl ObjectProvider for Projection { )); } }; - if entry.kind == EntryKind::Regular - && flock(entry.fd(), FlockOperation::LockExclusive).is_err() - { - return ProviderResult::Failure(HostError::internal( - "filesystem object could not be locked", - EffectGuarantee::NotCommitted, - )); + if entry.kind == EntryKind::Regular { + match lock_with_context(entry.fd(), context, self.limits.request_timeout) { + Ok(()) => {} + Err(ProjectionError::Resource) => { + return ProviderResult::Failure(HostError::resource_limit( + "filesystem lock deadline exceeded", + EffectGuarantee::NotCommitted, + )); + } + Err(ProjectionError::Internal | ProjectionError::NotFound) => { + return ProviderResult::Failure(HostError::internal( + "filesystem object could not be locked", + EffectGuarantee::NotCommitted, + )); + } + } } let publication = self.publication(entry.clone(), context.clone()); let mut attempt = operation(&publication); @@ -661,6 +661,71 @@ impl FileWrite for FilesystemObject { } } +fn open_root(path: &Path) -> Result { + let start = if path.is_absolute() { "/" } else { "." }; + let mut current = openat( + CWD, + start, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, + FsMode::empty(), + ) + .map_err(std::io::Error::from)?; + for component in path.components() { + let name = match component { + Component::RootDir | Component::CurDir => continue, + Component::Normal(name) => name, + Component::ParentDir | Component::Prefix(_) => { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "filesystem root must not contain parent or platform-prefix components", + )); + } + }; + let probe = openat( + ¤t, + name, + OFlags::PATH | OFlags::NOFOLLOW | OFlags::CLOEXEC, + FsMode::empty(), + ) + .map_err(std::io::Error::from)?; + let stat = fstat(&probe).map_err(std::io::Error::from)?; + if EntryKind::from_mode(stat.st_mode) != EntryKind::Directory { + return Err(std::io::Error::new( + std::io::ErrorKind::NotADirectory, + "filesystem root contains a symlink or non-directory component", + )); + } + current = reopen(&probe, OFlags::RDONLY | OFlags::DIRECTORY) + .map_err(|_| std::io::Error::other("filesystem root could not be pinned"))?; + } + Ok(current) +} + +fn lock_with_context( + fd: &OwnedFd, + context: &ExecutionContext, + maximum_wait: std::time::Duration, +) -> Result<(), ProjectionError> { + let now = std::time::Instant::now(); + let fallback_deadline = now.checked_add(maximum_wait).unwrap_or(now); + let deadline = context.deadline().map_or(fallback_deadline, |configured| { + configured.min(fallback_deadline) + }); + loop { + check_context(context)?; + if std::time::Instant::now() >= deadline { + return Err(ProjectionError::Resource); + } + match flock(fd, FlockOperation::NonBlockingLockExclusive) { + Ok(()) => return Ok(()), + Err(rustix::io::Errno::WOULDBLOCK) => { + std::thread::sleep(std::time::Duration::from_millis(1)); + } + Err(_) => return Err(ProjectionError::Internal), + } + } +} + fn reopen(fd: &OwnedFd, flags: OFlags) -> Result { let path = format!("/proc/self/fd/{}", fd.as_raw_fd()); openat(CWD, path, flags | OFlags::CLOEXEC, FsMode::empty()).map_err(classify_lookup) @@ -683,7 +748,13 @@ fn classify_lookup(error: rustix::io::Errno) -> ProjectionError { use rustix::io::Errno; if matches!( error, - Errno::NOENT | Errno::NOTDIR | Errno::LOOP | Errno::ACCESS | Errno::PERM + Errno::NOENT + | Errno::NOTDIR + | Errno::LOOP + | Errno::ACCESS + | Errno::PERM + | Errno::INVAL + | Errno::NAMETOOLONG ) { ProjectionError::NotFound } else { @@ -740,7 +811,7 @@ mod tests { }; use wip_protocol::{FetchTreeRequest, ProtocolErrorCode, Value}; - use super::{FailureInjection, Projection}; + use super::{FailureInjection, FlockOperation, Projection}; use crate::{AccessMode, ResourceLimits}; fn projection(root: &std::path::Path, access: AccessMode) -> Projection { @@ -1031,4 +1102,61 @@ mod tests { .expect_err("cancelled traversal"); assert_eq!(error.code, ProtocolErrorCode::ResourceLimitExceeded); } + + #[test] + fn filesystem_invalid_names_are_not_published() { + let temporary = tempdir().expect("temporary root"); + let projection = projection(temporary.path(), AccessMode::ReadOnly); + let error = projection + .fetch("/bad\0name") + .expect_err("NUL cannot name a backend entry"); + assert_eq!(error.kind(), CallErrorKind::NotFound); + } + + #[cfg(unix)] + #[test] + fn configured_root_rejects_symlink_components() { + let temporary = tempdir().expect("temporary parent"); + let actual = temporary.path().join("actual"); + fs::create_dir(&actual).expect("create actual root"); + let linked = temporary.path().join("linked"); + std::os::unix::fs::symlink(&actual, &linked).expect("create root symlink"); + assert!( + Projection::open(&linked, AccessMode::ReadOnly, ResourceLimits::default()).is_err() + ); + } + + #[test] + fn file_lock_wait_observes_the_operation_deadline() { + let temporary = tempdir().expect("temporary root"); + let path = temporary.path().join("file"); + fs::write(&path, b"locked").expect("write file"); + let locked = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .expect("open lock owner"); + rustix::fs::flock(&locked, FlockOperation::LockExclusive).expect("hold external lock"); + + let limits = ResourceLimits { + request_timeout: std::time::Duration::from_millis(20), + ..ResourceLimits::default() + }; + let projection = Projection::open(temporary.path(), AccessMode::ReadOnly, limits) + .expect("open projection"); + let observed = projection.fetch("/file").expect("fetch file"); + let started = std::time::Instant::now(); + let error = call( + &projection, + "/file", + observed.validator, + observed.interfaces[0].clone(), + "read", + BTreeMap::from([("length".into(), Value::Integer(1))]), + ) + .expect_err("lock wait must time out"); + assert_eq!(error.kind(), CallErrorKind::ResourceLimitExceeded); + assert!(started.elapsed() < std::time::Duration::from_secs(1)); + rustix::fs::flock(&locked, FlockOperation::Unlock).expect("release lock"); + } } diff --git a/crates/wip-fs-server/src/server.rs b/crates/wip-fs-server/src/server.rs index a5aa5e9..ae36a62 100644 --- a/crates/wip-fs-server/src/server.rs +++ b/crates/wip-fs-server/src/server.rs @@ -34,9 +34,9 @@ impl WipFsServer { /// Validates configuration and opens the configured directory capability. pub fn new(config: ServerConfig) -> Result { config.validate()?; - let effective_root = config.effective_root()?; let endpoint = config.endpoint()?; - let projection = Projection::open(&effective_root, config.access, config.limits)?; + let projection = Projection::open(&config.root, config.access, config.limits)?; + let effective_root = projection.effective_root()?; Ok(Self { config, endpoint, @@ -161,6 +161,7 @@ impl WipFsServer { Ok(metadata) => metadata, Err(error) => return self.codec_error(ProtocolInteraction::CallOperation, error), }; + let may_commit = metadata.operation == "replace"; let decoded = std::sync::Mutex::new(None); let execution = self.projection.call_operation_with_context_and_decoder( context, @@ -181,14 +182,15 @@ impl WipFsServer { .into_inner() .unwrap_or_else(|error| error.into_inner()) else { - return self.protocol_error( - ProtocolInteraction::CallOperation, - &ProtocolError { - code: ProtocolErrorCode::OperationOutcomeUnknown, - message: "operation completed without response metadata".into(), + let error = ProtocolError { + code: if may_commit { + ProtocolErrorCode::OperationOutcomeUnknown + } else { + ProtocolErrorCode::Internal }, - None, - ); + message: "operation completed without response metadata".into(), + }; + return self.protocol_error(ProtocolInteraction::CallOperation, &error, None); }; encode_call_operation_response( &request, @@ -196,9 +198,13 @@ impl WipFsServer { &result, self.config.limits.wire, ) - .unwrap_or_else(|_| { - // Dispatch succeeded. A transport/result encoding failure cannot - // prove that a write did not commit. + .unwrap_or_else(|encode_error| { + if !may_commit { + return self + .read_encode_error(ProtocolInteraction::CallOperation, encode_error); + } + // Replacement may already be committed, so a response + // encoding failure cannot carry a non-commit guarantee. let error = ProtocolError { code: ProtocolErrorCode::OperationOutcomeUnknown, message: "operation completed but its response could not be encoded".into(), diff --git a/crates/wip-fs-server/tests/end_to_end.rs b/crates/wip-fs-server/tests/end_to_end.rs index 9e44542..71bcc5b 100644 --- a/crates/wip-fs-server/tests/end_to_end.rs +++ b/crates/wip-fs-server/tests/end_to_end.rs @@ -10,14 +10,14 @@ use tempfile::tempdir; use wip_fs_server::{AccessMode, ServerConfig, TransportSecurity, WipFsServer}; use wip_https::{ BatchRequest, BatchRequestItem, BatchRequestKind, BatchResponse, BatchSuccess, DecodedResponse, - Endpoint, JSON_CONTENT_TYPE, Route, decode_batch_request, decode_batch_response, + Endpoint, JSON_CONTENT_TYPE, Limits, Route, decode_batch_request, decode_batch_response, decode_call_operation_response, decode_fetch_interface_response, decode_fetch_response, decode_fetch_tree_response, encode_batch_request, encode_call_operation_request, encode_fetch_interface_request, encode_fetch_request, encode_fetch_tree_request, }; use wip_protocol::{ CallOperationRequest, FetchInterfaceRequest, FetchRequest, FetchTreeRequest, InterfaceTarget, - Target, Value, + ProtocolErrorCode, Target, Value, }; #[test] @@ -256,6 +256,83 @@ fn call_argument_decoding_does_not_mask_missing_target() { ); } +#[test] +fn backend_invalid_unicode_path_is_not_found_not_internal() { + let temporary = tempdir().expect("temporary root"); + let config = ServerConfig::for_root(temporary.path()); + let limits = config.limits.wire; + let server = WipFsServer::new(config).expect("build server"); + let endpoint = Endpoint::parse("https://localhost/wip").expect("endpoint"); + let request = FetchRequest { + path: "/bad\0name".into(), + }; + let response = server + .handle_http(encode_fetch_request(&endpoint, &request, limits).expect("encode fetch")); + assert_eq!(response.status(), http::StatusCode::NOT_FOUND); + match decode_fetch_response(&request, &response, limits).expect("decode missing response") { + DecodedResponse::ProtocolFailure { error, .. } => { + assert_eq!(error.code, ProtocolErrorCode::NotFound); + } + DecodedResponse::Success(_) => panic!("backend-invalid path must not resolve"), + } +} + +#[test] +fn read_response_size_failure_preserves_non_commit_guarantee() { + let temporary = tempdir().expect("temporary root"); + std::fs::write(temporary.path().join("large"), vec![b'x'; 2_000]).expect("write file"); + let mut config = ServerConfig::for_root(temporary.path()); + config.limits.wire = Limits::new(4_096, 1_024, 64, 64).expect("wire limits"); + let limits = config.limits.wire; + let server = WipFsServer::new(config).expect("build server"); + let endpoint = Endpoint::parse("https://localhost/wip").expect("endpoint"); + + let fetch_request = FetchRequest { + path: "/large".into(), + }; + let response = server.handle_http( + encode_fetch_request(&endpoint, &fetch_request, limits).expect("encode fetch"), + ); + let file = + success(decode_fetch_response(&fetch_request, &response, limits).expect("decode fetch")); + let interface_request = FetchInterfaceRequest { + interface: file.interfaces[0].clone(), + }; + let response = server.handle_http( + encode_fetch_interface_request(&endpoint, &interface_request, limits) + .expect("encode interface fetch"), + ); + let interface = success( + decode_fetch_interface_response(&interface_request, &response, limits) + .expect("decode interface"), + ); + let call = CallOperationRequest { + target: Target { + path: "/large".into(), + validator: file.validator, + }, + interface: InterfaceTarget { + reference: file.interfaces[0].clone(), + validator: None, + }, + operation: "read".into(), + arguments: BTreeMap::from([("length".into(), Value::Integer(1_500))]), + }; + let response = server.handle_http( + encode_call_operation_request(&endpoint, &call, &interface.descriptor, limits) + .expect("encode read call"), + ); + assert_eq!(response.status(), http::StatusCode::PAYLOAD_TOO_LARGE); + match decode_call_operation_response(&call, &interface.descriptor, &response, limits) + .expect("decode resource failure") + { + DecodedResponse::ProtocolFailure { error, .. } => { + assert_eq!(error.code, ProtocolErrorCode::ResourceLimitExceeded); + } + DecodedResponse::Success(_) => panic!("oversized read response must fail"), + } +} + fn assert_batch_successes(batch: &BatchResponse) { assert_eq!(batch.responses.len(), 3); assert!(matches!(