From 4b473d3287518ad68face81eea217da83ce19e54 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Fri, 10 Jul 2026 15:41:42 -0700 Subject: [PATCH 1/4] [LXC] Harden denied-path masking + add most-specific-path resolver (AB#62861419) - Replace is_file() masking heuristic with explicit `type` schema field + symlink_metadata() fallback (no symlink follow; fail-closed on missing+untyped). - Add reusable most-specific-path-wins resolver in wxc_common (deny>ro>rw); wire LXC mounts to emit in specificity order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4 --- schemas/dev/mxc-config.schema.0.8.0-dev.json | 46 ++- sdk/src/generated/wire.ts | 28 +- .../lxc/common/src/filesystem_mounts.rs | 206 ++++++++++---- src/core/wxc_common/src/config_parser.rs | 69 ++++- src/core/wxc_common/src/filesystem_resolve.rs | 217 +------------- src/core/wxc_common/src/lib.rs | 1 + src/core/wxc_common/src/models.rs | 13 + src/core/wxc_common/src/path_specificity.rs | 266 ++++++++++++++++++ src/core/wxc_common/src/ts_emit.rs | 74 ++++- src/core/wxc_common/src/wire.rs | 37 ++- 10 files changed, 681 insertions(+), 276 deletions(-) create mode 100644 src/core/wxc_common/src/path_specificity.rs diff --git a/schemas/dev/mxc-config.schema.0.8.0-dev.json b/schemas/dev/mxc-config.schema.0.8.0-dev.json index 25c500333..2be1c89cc 100644 --- a/schemas/dev/mxc-config.schema.0.8.0-dev.json +++ b/schemas/dev/mxc-config.schema.0.8.0-dev.json @@ -136,6 +136,48 @@ } ] }, + "DeniedPath": { + "anyOf": [ + { + "type": "string" + }, + { + "$ref": "#/definitions/DeniedPathObject" + } + ], + "description": "A denied filesystem path, accepted as either a legacy string or an object with explicit mask intent." + }, + "DeniedPathKind": { + "description": "Mask type for a denied filesystem path.", + "enum": [ + "file", + "dir" + ], + "type": "string" + }, + "DeniedPathObject": { + "additionalProperties": false, + "description": "Object form for a denied filesystem path.", + "properties": { + "path": { + "description": "Path to deny.", + "type": "string" + }, + "type": { + "allOf": [ + { + "$ref": "#/definitions/DeniedPathKind" + } + ], + "description": "Whether to mask the path as a file or directory." + } + }, + "required": [ + "path", + "type" + ], + "type": "object" + }, "Experimental": { "description": "Experimental features (only honored with `--experimental`). This block is intentionally **permissive** (no `deny_unknown_fields`): experimental backends are in flux, so the schema documents the known shapes for editor help without rejecting in-progress fields. The strict, closed contract is the stable (top-level) surface.", "properties": { @@ -227,9 +269,9 @@ "description": "Filesystem access policy.", "properties": { "deniedPaths": { - "description": "Paths explicitly denied (override broader allow rules).", + "description": "Paths explicitly denied (override broader allow rules). Entries may be bare strings for backwards compatibility or objects with an explicit file/dir mask type.", "items": { - "type": "string" + "$ref": "#/definitions/DeniedPath" }, "type": [ "array", diff --git a/sdk/src/generated/wire.ts b/sdk/src/generated/wire.ts index 4e0026edb..71d90104a 100644 --- a/sdk/src/generated/wire.ts +++ b/sdk/src/generated/wire.ts @@ -47,6 +47,30 @@ export type ClipboardPolicy = "none" | "read" | "write" | "all"; */ export type Containment = "process" | "processcontainer" | "vm" | "windows_sandbox" | "lxc" | "microvm" | "hyperlight" | "wslc" | "seatbelt" | "isolation_session" | "bubblewrap"; +/** + * A denied filesystem path, accepted as either a legacy string or an object with explicit mask intent. + */ +export type DeniedPath = string | DeniedPathObject; + +/** + * Mask type for a denied filesystem path. + */ +export type DeniedPathKind = "file" | "dir"; + +/** + * Object form for a denied filesystem path. + */ +export interface DeniedPathObject { + /** + * Path to deny. + */ + path: string; + /** + * Whether to mask the path as a file or directory. + */ + type: DeniedPathKind; +} + /** * Experimental features (only honored with `--experimental`). This block is intentionally **permissive** (no `deny_unknown_fields`): experimental backends are in flux, so the schema documents the known shapes for editor help without rejecting in-progress fields. The strict, closed contract is the stable (top-level) surface. */ @@ -93,9 +117,9 @@ export interface Fallback { */ export interface Filesystem { /** - * Paths explicitly denied (override broader allow rules). + * Paths explicitly denied (override broader allow rules). Entries may be bare strings for backwards compatibility or objects with an explicit file/dir mask type. */ - deniedPaths?: string[] | null; + deniedPaths?: DeniedPath[] | null; /** * Paths the process can read but not write. */ diff --git a/src/backends/lxc/common/src/filesystem_mounts.rs b/src/backends/lxc/common/src/filesystem_mounts.rs index 4db6a8879..c7102e544 100644 --- a/src/backends/lxc/common/src/filesystem_mounts.rs +++ b/src/backends/lxc/common/src/filesystem_mounts.rs @@ -7,10 +7,13 @@ //! mount entries: //! - `readwritePaths` → `bind,rw` mount //! - `readonlyPaths` → `bind,ro` mount -//! - `deniedPaths` → not mounted (inaccessible inside container) +//! - `deniedPaths` → masked (inaccessible inside container) + +use std::path::Path; use wxc_common::logger::Logger; -use wxc_common::models::ContainerPolicy; +use wxc_common::models::{ContainerPolicy, MaskKind}; +use wxc_common::path_specificity::{resolve_mount_order, FsIntent}; use crate::lxc_bindings::LxcContainer; @@ -31,6 +34,52 @@ fn validate_path(path: &str) -> Result<(), String> { Ok(()) } +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum ObservedMaskPathKind { + File, + Dir, + Symlink, +} + +fn observed_mask_path_kind(full_path: &str) -> Result, String> { + match std::fs::symlink_metadata(full_path) { + Ok(metadata) => { + let file_type = metadata.file_type(); + if file_type.is_dir() { + Ok(Some(ObservedMaskPathKind::Dir)) + } else if file_type.is_symlink() { + Ok(Some(ObservedMaskPathKind::Symlink)) + } else { + Ok(Some(ObservedMaskPathKind::File)) + } + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(format!( + "Unable to inspect denied path '{}': {}. Set deniedPaths entry to an object with explicit type \"file\" or \"dir\".", + full_path, err + )), + } +} + +fn resolve_mask_kind( + denied_path: &str, + explicit: Option, + observed: Option, +) -> Result { + if let Some(kind) = explicit { + return Ok(kind); + } + + match observed { + Some(ObservedMaskPathKind::Dir) => Ok(MaskKind::Dir), + Some(ObservedMaskPathKind::File | ObservedMaskPathKind::Symlink) => Ok(MaskKind::File), + None => Err(format!( + "Denied path '{}' does not exist and no mask type was specified. Use deniedPaths object form {{\"path\":\"{}\",\"type\":\"file\"}} or {{\"path\":\"{}\",\"type\":\"dir\"}}.", + denied_path, denied_path, denied_path + )), + } +} + /// Configure filesystem bind mounts on the container based on the policy. /// /// Adds `lxc.mount.entry` config items for each path in the policy. @@ -39,56 +88,61 @@ pub fn configure_filesystem_mounts( policy: &ContainerPolicy, logger: &mut Logger, ) -> Result<(), String> { - // Read-write bind mounts - for host_path in &policy.readwrite_paths { - validate_path(host_path)?; - let container_path = host_path.trim_start_matches('/'); - let mount_entry = format!("{} {} none bind,create=dir 0 0", host_path, container_path); - logger.log_line(&format!( - "Adding rw bind mount: {} -> /{}", - host_path, container_path - )); - container.set_config_item("lxc.mount.entry", &mount_entry)?; - } - - // Read-only bind mounts - for host_path in &policy.readonly_paths { - validate_path(host_path)?; - let container_path = host_path.trim_start_matches('/'); - let mount_entry = format!( - "{} {} none bind,ro,create=dir 0 0", - host_path, container_path - ); - logger.log_line(&format!( - "Adding ro bind mount: {} -> /{}", - host_path, container_path - )); - container.set_config_item("lxc.mount.entry", &mount_entry)?; - } - - // Denied paths: mount tmpfs over them to mask contents - for host_path in &policy.denied_paths { + for mount in resolve_mount_order(policy) { + let host_path = &mount.path; validate_path(host_path)?; let container_path = host_path.trim_start_matches('/'); - // Determine if the path is a file or directory inside the container rootfs - // so we use the correct LXC `create=` type for the mount entry. - let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name()); - let full_path = format!("{}/{}", rootfs_base, container_path); - - // Use /dev/null bind mount for files, tmpfs for directories - let is_file = std::path::Path::new(&full_path).is_file(); - let mount_entry = if is_file { - format!("/dev/null {} none bind,ro,create=file 0 0", container_path) - } else { - format!("tmpfs {} tmpfs ro,size=0,create=dir 0 0", container_path) - }; - let create_type = if is_file { "file" } else { "dir" }; - logger.log_line(&format!( - "Masking denied path: /{} ({})", - container_path, create_type - )); - container.set_config_item("lxc.mount.entry", &mount_entry)?; + match mount.intent { + FsIntent::ReadWrite => { + let mount_entry = + format!("{} {} none bind,create=dir 0 0", host_path, container_path); + logger.log_line(&format!( + "Adding rw bind mount: {} -> /{}", + host_path, container_path + )); + container.set_config_item("lxc.mount.entry", &mount_entry)?; + } + FsIntent::ReadOnly => { + let mount_entry = format!( + "{} {} none bind,ro,create=dir 0 0", + host_path, container_path + ); + logger.log_line(&format!( + "Adding ro bind mount: {} -> /{}", + host_path, container_path + )); + container.set_config_item("lxc.mount.entry", &mount_entry)?; + } + FsIntent::Denied => { + let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name()); + let full_path = Path::new(&rootfs_base).join(container_path); + let explicit = policy.denied_path_kinds.get(host_path).copied(); + let kind = resolve_mask_kind( + host_path, + explicit, + observed_mask_path_kind(&full_path.to_string_lossy())?, + )?; + + let mount_entry = match kind { + MaskKind::File => { + format!("/dev/null {} none bind,ro,create=file 0 0", container_path) + } + MaskKind::Dir => { + format!("tmpfs {} tmpfs ro,size=0,create=dir 0 0", container_path) + } + }; + let create_type = match kind { + MaskKind::File => "file", + MaskKind::Dir => "dir", + }; + logger.log_line(&format!( + "Masking denied path: /{} ({})", + container_path, create_type + )); + container.set_config_item("lxc.mount.entry", &mount_entry)?; + } + } } Ok(()) @@ -141,4 +195,58 @@ mod tests { fn test_validate_path_accepts_normal() { assert!(validate_path("/mnt/shared").is_ok()); } + + #[test] + fn explicit_mask_kind_wins_over_observed_kind() { + assert_eq!( + resolve_mask_kind( + "/etc/shadow", + Some(MaskKind::File), + Some(ObservedMaskPathKind::Dir) + ) + .unwrap(), + MaskKind::File + ); + assert_eq!( + resolve_mask_kind( + "/var/lib/app", + Some(MaskKind::Dir), + Some(ObservedMaskPathKind::File) + ) + .unwrap(), + MaskKind::Dir + ); + } + + #[test] + fn observed_symlink_and_regular_file_use_file_mask() { + assert_eq!( + resolve_mask_kind( + "/etc/alternatives/editor", + None, + Some(ObservedMaskPathKind::Symlink) + ) + .unwrap(), + MaskKind::File + ); + assert_eq!( + resolve_mask_kind("/etc/shadow", None, Some(ObservedMaskPathKind::File)).unwrap(), + MaskKind::File + ); + } + + #[test] + fn observed_directory_uses_dir_mask() { + assert_eq!( + resolve_mask_kind("/var/lib/app", None, Some(ObservedMaskPathKind::Dir)).unwrap(), + MaskKind::Dir + ); + } + + #[test] + fn missing_path_without_explicit_kind_errors() { + let err = resolve_mask_kind("/missing/file", None, None).unwrap_err(); + assert!(err.contains("does not exist")); + assert!(err.contains("type")); + } } diff --git a/src/core/wxc_common/src/config_parser.rs b/src/core/wxc_common/src/config_parser.rs index 9696e5aac..b4f6dbfc2 100644 --- a/src/core/wxc_common/src/config_parser.rs +++ b/src/core/wxc_common/src/config_parser.rs @@ -9,9 +9,9 @@ use crate::error::WxcError; use crate::logger::Logger; use crate::models::{ ContainerPolicy, ContainmentBackend, ExecutionRequest, ExperimentalConfig, - IsolationSessionConfig, LifecycleConfig, LxcConfig, NetworkEnforcementMode, NetworkPolicy, - PortMapping, ProxyAddress, ProxyConfig, SeatbeltConfig, TelemetryConfig, TestFeatureConfig, - UiPolicy, WindowsSandboxConfig, WslcConfig, + IsolationSessionConfig, LifecycleConfig, LxcConfig, MaskKind, NetworkEnforcementMode, + NetworkPolicy, PortMapping, ProxyAddress, ProxyConfig, SeatbeltConfig, TelemetryConfig, + TestFeatureConfig, UiPolicy, WindowsSandboxConfig, WslcConfig, }; use crate::mxc_error::MxcError; use crate::state_aware_request::{MxcRequest, ParsedStateAwareRequest, Phase}; @@ -300,6 +300,19 @@ fn validate_paths(paths: &[String], logger: &mut Logger) -> Result<(), WxcError> Ok(()) } +fn convert_denied_path(entry: wire::DeniedPath) -> (String, Option) { + match entry { + wire::DeniedPath::Path(path) => (path, None), + wire::DeniedPath::Object(obj) => { + let kind = match obj.kind { + wire::DeniedPathKind::File => MaskKind::File, + wire::DeniedPathKind::Dir => MaskKind::Dir, + }; + (obj.path, Some(kind)) + } + } +} + /// Normalizes cross-list filesystem path constraints by applying /// **most-restrictive-wins** precedence (`deny` > `readonly` > `readwrite`): /// @@ -761,7 +774,13 @@ fn convert_wire_config( // Filesystem section if let Some(fscfg) = cfg.filesystem { if let Some(v) = fscfg.denied_paths { - policy.denied_paths = v; + for entry in v { + let (path, kind) = convert_denied_path(entry); + if let Some(kind) = kind { + policy.denied_path_kinds.insert(path.clone(), kind); + } + policy.denied_paths.push(path); + } } if let Some(v) = fscfg.readwrite_paths { policy.readwrite_paths = v; @@ -1726,6 +1745,48 @@ mod tests { assert_eq!(req.policy.denied_paths[1], "C:\\Program Files"); } + #[test] + fn filesystem_denied_paths_accept_string_and_typed_object() { + let json = r#"{ + "process": {"commandLine": "print('test')"}, + "filesystem": { + "deniedPaths": [ + "C:\\Windows\\System32", + {"path": "C:\\Users\\Public\\secret.txt", "type": "file"}, + {"path": "C:\\Users\\Public\\secret-dir", "type": "dir"} + ] + } + }"#; + let encoded = base64_encode(json.as_bytes()); + let mut logger = test_logger(); + + let req = load_request(&encoded, &mut logger, true).unwrap(); + assert_eq!( + req.policy.denied_paths, + vec![ + "C:\\Windows\\System32", + "C:\\Users\\Public\\secret.txt", + "C:\\Users\\Public\\secret-dir" + ] + ); + assert_eq!( + req.policy + .denied_path_kinds + .get("C:\\Users\\Public\\secret.txt"), + Some(&MaskKind::File) + ); + assert_eq!( + req.policy + .denied_path_kinds + .get("C:\\Users\\Public\\secret-dir"), + Some(&MaskKind::Dir) + ); + assert!(!req + .policy + .denied_path_kinds + .contains_key("C:\\Windows\\System32")); + } + #[test] fn block_evil_filesystem_paths() { let json = r#"{ diff --git a/src/core/wxc_common/src/filesystem_resolve.rs b/src/core/wxc_common/src/filesystem_resolve.rs index 83b5085ec..5ade9f977 100644 --- a/src/core/wxc_common/src/filesystem_resolve.rs +++ b/src/core/wxc_common/src/filesystem_resolve.rs @@ -1,218 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -//! Filesystem-policy **most-specific-path-wins** mount ordering (roadmap D4). -//! -//! The Linux containment backends (Bubblewrap, LXC) realise the filesystem -//! policy as an ordered list of mounts, and the kernel applies "the last mount -//! at a path wins". Overlap between a `readwritePaths` / `readonlyPaths` / -//! `deniedPaths` entry and one of its ancestors or descendants is therefore -//! resolved by **emission order**, not by path specificity. Emitting the three -//! lists in a fixed category order (rw, then ro, then denied) gets some overlaps -//! right by luck and others wrong: e.g. `readwritePaths: ["/data/secrets"]` with -//! `deniedPaths: ["/data"]` emits the deep read-write bind first and then masks -//! the whole `/data` subtree over it, so the *less* specific intent wins. -//! -//! [`resolve_mount_order`] fixes this by returning the policy paths ordered so -//! that a **deeper (more specific) path is always emitted after — and therefore -//! overrides — any shallower ancestor** with a different intent. A backend then -//! emits its baseline / virtual filesystems first and walks this ordered list -//! last, so the most-specific policy intent wins at every path regardless of -//! which list it came from. -//! -//! This resolver only **reorders**; it does not drop or merge entries. Exact -//! same-path conflicts across lists (e.g. the same object in both `readonlyPaths` -//! and `deniedPaths`) are resolved upstream to the most-restrictive intent by -//! [`crate::filesystem_object::normalize_object_conflicts`], which every runner -//! calls before building its mounts — so no exact-duplicate same-path conflicts -//! reach this function. +//! Compatibility re-export for callers that used the original resolver module. -use crate::models::ContainerPolicy; - -/// The access intent a resolved mount carries into the backend, mapped from the -/// policy list the path came from. -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum FsIntent { - /// From `readwritePaths` — read+write bind. - ReadWrite, - /// From `readonlyPaths` — read-only bind. - ReadOnly, - /// From `deniedPaths` — masked (invisible) inside the sandbox. - Denied, -} - -/// A single policy path paired with its intent, ready for a backend to emit as a -/// mount. Ordered relative to its peers by [`resolve_mount_order`]. -#[derive(Clone, PartialEq, Eq, Debug)] -pub struct ResolvedMount { - /// The host path exactly as it appears in the policy (not normalized — the - /// backend still owns path-to-mount translation). - pub path: String, - /// The intent this path was declared with. - pub intent: FsIntent, -} - -/// Number of path components in `path`, used as the specificity key: a deeper -/// path (more components) is more specific. Splits on both `/` and `\` and -/// ignores empty segments so a leading slash and any trailing slash do not skew -/// the count (`/data` and `/data/` are both depth 1; `/data/secrets` is depth 2). -fn specificity(path: &str) -> usize { - path.split(['/', '\\']).filter(|s| !s.is_empty()).count() -} - -/// Orders the policy's filesystem paths so that a more-specific (deeper) path is -/// always emitted **after** — and therefore overrides — a less-specific ancestor -/// with a different intent, for any backend that applies mounts in order with -/// "last at a path wins" semantics. -/// -/// The sort is **stable** and ascending by specificity, so: -/// - shallower paths come first, deeper paths last (deepest wins); -/// - paths of equal depth keep their category order (read-write, then -/// read-only, then denied), matching the backends' historical emission order -/// so an exact-depth tie still resolves denied-over-ro-over-rw. -/// -/// Assumes [`crate::filesystem_object::normalize_object_conflicts`] has already -/// collapsed any exact same-path conflicts; this function does not merge or drop -/// entries, it only reorders them. -pub fn resolve_mount_order(policy: &ContainerPolicy) -> Vec { - // Collect in category order (rw, ro, denied). A stable sort below preserves - // this order for equal-depth ties, so denied still wins over ro over rw at - // the same path depth — matching the backends' pre-resolver emission order. - let mut mounts: Vec = Vec::with_capacity( - policy.readwrite_paths.len() + policy.readonly_paths.len() + policy.denied_paths.len(), - ); - for path in &policy.readwrite_paths { - mounts.push(ResolvedMount { - path: path.clone(), - intent: FsIntent::ReadWrite, - }); - } - for path in &policy.readonly_paths { - mounts.push(ResolvedMount { - path: path.clone(), - intent: FsIntent::ReadOnly, - }); - } - for path in &policy.denied_paths { - mounts.push(ResolvedMount { - path: path.clone(), - intent: FsIntent::Denied, - }); - } - - mounts.sort_by_key(|m| specificity(&m.path)); - mounts -} - -#[cfg(test)] -mod tests { - use super::*; - - fn policy(rw: &[&str], ro: &[&str], denied: &[&str]) -> ContainerPolicy { - ContainerPolicy { - readwrite_paths: rw.iter().map(|s| s.to_string()).collect(), - readonly_paths: ro.iter().map(|s| s.to_string()).collect(), - denied_paths: denied.iter().map(|s| s.to_string()).collect(), - ..ContainerPolicy::default() - } - } - - fn order(mounts: &[ResolvedMount]) -> Vec<(&str, FsIntent)> { - mounts.iter().map(|m| (m.path.as_str(), m.intent)).collect() - } - - #[test] - fn deeper_denied_child_comes_after_shallower_rw_parent() { - // rw /data, denied /data/secrets: the deep denied path must be emitted - // last so it masks the subtree even though /data is read-write. - let p = policy(&["/data"], &[], &["/data/secrets"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/data", FsIntent::ReadWrite), - ("/data/secrets", FsIntent::Denied), - ] - ); - } - - #[test] - fn deeper_rw_child_comes_after_shallower_denied_parent() { - // denied /data, rw /data/secrets: previously the /data tmpfs was emitted - // last and shadowed the deep bind (less-specific won). The resolver must - // now emit the deeper /data/secrets rw mount last so it wins. - let p = policy(&["/data/secrets"], &[], &["/data"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/data", FsIntent::Denied), - ("/data/secrets", FsIntent::ReadWrite), - ] - ); - } - - #[test] - fn three_level_nesting_orders_shallow_to_deep() { - let p = policy(&["/a/b/c"], &["/a"], &["/a/b"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/a", FsIntent::ReadOnly), - ("/a/b", FsIntent::Denied), - ("/a/b/c", FsIntent::ReadWrite), - ] - ); - } - - #[test] - fn equal_depth_keeps_category_order_rw_ro_denied() { - // Three sibling paths at the same depth: order is stable and follows the - // category order so a same-depth tie resolves denied last (wins). - let p = policy(&["/x"], &["/y"], &["/z"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/x", FsIntent::ReadWrite), - ("/y", FsIntent::ReadOnly), - ("/z", FsIntent::Denied), - ] - ); - } - - #[test] - fn trailing_slash_does_not_change_specificity() { - // "/data/" and "/data" are the same depth; the deeper child still wins. - let p = policy(&["/data/"], &[], &["/data/secrets"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/data/", FsIntent::ReadWrite), - ("/data/secrets", FsIntent::Denied), - ] - ); - } - - #[test] - fn non_overlapping_paths_pass_through_by_depth() { - // Unrelated paths: no overlap to resolve, just ordered shallow-to-deep. - let p = policy(&["/srv/app/data"], &["/usr"], &["/opt/secret"]); - let resolved = resolve_mount_order(&p); - assert_eq!( - order(&resolved), - vec![ - ("/usr", FsIntent::ReadOnly), - ("/opt/secret", FsIntent::Denied), - ("/srv/app/data", FsIntent::ReadWrite), - ] - ); - } - - #[test] - fn empty_policy_yields_no_mounts() { - assert!(resolve_mount_order(&ContainerPolicy::default()).is_empty()); - } -} +pub use crate::path_specificity::{effective_intent, resolve_mount_order, resolve_path_plan}; +pub use crate::path_specificity::{FsIntent, ResolvedMount}; diff --git a/src/core/wxc_common/src/lib.rs b/src/core/wxc_common/src/lib.rs index daaa9ad95..11d8343ec 100644 --- a/src/core/wxc_common/src/lib.rs +++ b/src/core/wxc_common/src/lib.rs @@ -17,6 +17,7 @@ pub mod logger; pub mod microvm_staging; pub mod models; pub mod mxc_error; +pub mod path_specificity; pub mod sandbox_process; pub mod script_runner; pub mod state_aware_backend; diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index f6e403a5f..65c680c31 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +use std::collections::HashMap; + use serde::{Deserialize, Serialize}; /// Selects which containment backend to use for script execution. @@ -538,6 +540,8 @@ pub struct ContainerPolicy { pub readwrite_paths: Vec, pub readonly_paths: Vec, pub denied_paths: Vec, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub denied_path_kinds: HashMap, pub fallback: FallbackPolicy, pub default_network_policy: NetworkPolicy, pub network_enforcement_mode: NetworkEnforcementMode, @@ -555,6 +559,15 @@ pub struct ContainerPolicy { pub base_process_ui: BaseProcessUiConfig, } +/// How a denied filesystem path should be masked when the backend must choose +/// between file and directory mount primitives. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum MaskKind { + File, + Dir, +} + /// Port mapping for host↔container port forwarding. #[derive(Debug, Default, Clone, Serialize, Deserialize)] pub struct PortMapping { diff --git a/src/core/wxc_common/src/path_specificity.rs b/src/core/wxc_common/src/path_specificity.rs new file mode 100644 index 000000000..5f0fcba9b --- /dev/null +++ b/src/core/wxc_common/src/path_specificity.rs @@ -0,0 +1,266 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Filesystem-policy most-specific-path-wins resolution. +//! +//! Backends such as LXC and Bubblewrap apply filesystem mount operations in +//! order. This module normalizes policy paths by component, collapses exact-path +//! conflicts with most-restrictive-wins (`denied` > `readonly` > `readwrite`), +//! and returns a shallow-to-deep plan so later mounts implement +//! most-specific-path-wins. + +use crate::models::ContainerPolicy; + +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] +pub enum FsIntent { + ReadWrite, + ReadOnly, + Denied, +} + +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct ResolvedMount { + pub path: String, + pub intent: FsIntent, +} + +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] +struct PathKey(Vec); + +impl PathKey { + fn from_path(path: &str) -> Self { + Self( + path.split(['/', '\\']) + .filter(|component| !component.is_empty()) + .map(str::to_string) + .collect(), + ) + } + + fn depth(&self) -> usize { + self.0.len() + } + + fn is_prefix_of(&self, other: &Self) -> bool { + self.0.len() <= other.0.len() && self.0.iter().zip(other.0.iter()).all(|(a, b)| a == b) + } +} + +#[derive(Clone, Debug)] +struct Candidate { + path: String, + key: PathKey, + intent: FsIntent, + sequence: usize, +} + +pub fn resolve_path_plan( + readwrite_paths: &[String], + readonly_paths: &[String], + denied_paths: &[String], +) -> Vec { + let mut candidates = + Vec::with_capacity(readwrite_paths.len() + readonly_paths.len() + denied_paths.len()); + let mut sequence = 0; + for (paths, intent) in [ + (readwrite_paths, FsIntent::ReadWrite), + (readonly_paths, FsIntent::ReadOnly), + (denied_paths, FsIntent::Denied), + ] { + for path in paths { + candidates.push(Candidate { + path: path.clone(), + key: PathKey::from_path(path), + intent, + sequence, + }); + sequence += 1; + } + } + + candidates.sort_by(|a, b| { + a.key + .cmp(&b.key) + .then_with(|| b.intent.cmp(&a.intent)) + .then_with(|| a.sequence.cmp(&b.sequence)) + }); + candidates.dedup_by(|a, b| { + if a.key == b.key { + if b.intent > a.intent { + a.path = b.path.clone(); + a.intent = b.intent; + a.sequence = b.sequence; + } + true + } else { + false + } + }); + candidates.sort_by(|a, b| { + a.key + .depth() + .cmp(&b.key.depth()) + .then_with(|| a.sequence.cmp(&b.sequence)) + }); + + candidates + .into_iter() + .map(|candidate| ResolvedMount { + path: candidate.path, + intent: candidate.intent, + }) + .collect() +} + +pub fn resolve_mount_order(policy: &ContainerPolicy) -> Vec { + resolve_path_plan( + &policy.readwrite_paths, + &policy.readonly_paths, + &policy.denied_paths, + ) +} + +pub fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option { + let query = PathKey::from_path(path); + plan.iter() + .filter_map(|mount| { + let key = PathKey::from_path(&mount.path); + key.is_prefix_of(&query) + .then_some((key.depth(), mount.intent)) + }) + .max_by(|(left_depth, left_intent), (right_depth, right_intent)| { + left_depth + .cmp(right_depth) + .then_with(|| left_intent.cmp(right_intent)) + }) + .map(|(_, intent)| intent) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn strings(paths: &[&str]) -> Vec { + paths.iter().map(|path| (*path).to_string()).collect() + } + + fn plan(rw: &[&str], ro: &[&str], denied: &[&str]) -> Vec { + resolve_path_plan(&strings(rw), &strings(ro), &strings(denied)) + } + + fn order(mounts: &[ResolvedMount]) -> Vec<(&str, FsIntent)> { + mounts + .iter() + .map(|mount| (mount.path.as_str(), mount.intent)) + .collect() + } + + #[test] + fn nested_paths_are_ordered_shallow_to_deep() { + let resolved = plan(&["/workspace"], &["/workspace/.git"], &["/workspace/.env"]); + assert_eq!( + order(&resolved), + vec![ + ("/workspace", FsIntent::ReadWrite), + ("/workspace/.git", FsIntent::ReadOnly), + ("/workspace/.env", FsIntent::Denied), + ] + ); + assert_eq!( + effective_intent(&resolved, "/workspace/src/main.rs"), + Some(FsIntent::ReadWrite) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/.git/config"), + Some(FsIntent::ReadOnly) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/.env"), + Some(FsIntent::Denied) + ); + } + + #[test] + fn exact_path_conflict_uses_most_restrictive_intent() { + let resolved = plan(&["/workspace"], &["/workspace"], &["/workspace"]); + assert_eq!(order(&resolved), vec![("/workspace", FsIntent::Denied)]); + assert_eq!( + effective_intent(&resolved, "/workspace/file"), + Some(FsIntent::Denied) + ); + } + + #[test] + fn readonly_wins_exact_path_conflict_over_readwrite() { + let resolved = plan(&["/workspace/"], &["/workspace"], &[]); + assert_eq!(order(&resolved), vec![("/workspace", FsIntent::ReadOnly)]); + } + + #[test] + fn siblings_do_not_override_each_other() { + let resolved = plan(&["/workspace/src"], &["/workspace/docs"], &[]); + assert_eq!( + effective_intent(&resolved, "/workspace/src/lib.rs"), + Some(FsIntent::ReadWrite) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/docs/index.md"), + Some(FsIntent::ReadOnly) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/tests/test.rs"), + None + ); + } + + #[test] + fn unrelated_paths_remain_independent() { + let resolved = plan(&["/srv/app/data"], &["/usr"], &["/opt/secret"]); + assert_eq!( + order(&resolved), + vec![ + ("/usr", FsIntent::ReadOnly), + ("/opt/secret", FsIntent::Denied), + ("/srv/app/data", FsIntent::ReadWrite), + ] + ); + assert_eq!(effective_intent(&resolved, "/var/log/app.log"), None); + } + + #[test] + fn deep_override_of_shallow_deny_wins_for_child_only() { + let resolved = plan(&["/data/secrets"], &[], &["/data"]); + assert_eq!( + order(&resolved), + vec![ + ("/data", FsIntent::Denied), + ("/data/secrets", FsIntent::ReadWrite), + ] + ); + assert_eq!(effective_intent(&resolved, "/data"), Some(FsIntent::Denied)); + assert_eq!( + effective_intent(&resolved, "/data/secrets/file"), + Some(FsIntent::ReadWrite) + ); + } + + #[test] + fn component_prefix_does_not_match_partial_component() { + let resolved = plan(&["/workspace"], &[], &[]); + assert_eq!(effective_intent(&resolved, "/workspace2/file"), None); + } + + #[test] + fn backslash_paths_are_compared_by_components() { + let resolved = plan(&["C:\\workspace"], &["C:\\workspace\\.git\\"], &[]); + assert_eq!( + effective_intent(&resolved, "C:\\workspace\\.git\\config"), + Some(FsIntent::ReadOnly) + ); + } + + #[test] + fn empty_policy_yields_no_mounts() { + assert!(plan(&[], &[], &[]).is_empty()); + } +} diff --git a/src/core/wxc_common/src/ts_emit.rs b/src/core/wxc_common/src/ts_emit.rs index 2d0a72944..bd2fc1c33 100644 --- a/src/core/wxc_common/src/ts_emit.rs +++ b/src/core/wxc_common/src/ts_emit.rs @@ -83,6 +83,14 @@ fn emit_definition(out: &mut String, name: &str, def: &Value) { return; } + if obj.contains_key("anyOf") || obj.contains_key("allOf") { + push_doc(out, obj.get("description")); + let (ty, nullable) = ts_type(def); + let nul = if nullable { " | null" } else { "" }; + out.push_str(&format!("export type {name} = {ty}{nul};\n\n")); + return; + } + emit_object(out, name, obj_as_map(def)); } @@ -163,19 +171,34 @@ fn ts_type(prop: &Value) -> (String, bool) { return (ref_name(r), false); } + if let Some(Value::Array(all_of)) = obj.get("allOf") { + let mut nullable = false; + let mut types = Vec::new(); + for branch in all_of { + let (t, n) = ts_type(branch); + nullable = nullable || n; + if !types.contains(&t) { + types.push(t); + } + } + return (join_types(types, " & "), nullable); + } + if let Some(Value::Array(any_of)) = obj.get("anyOf") { let mut nullable = false; - let mut ty = "unknown".to_string(); + let mut types = Vec::new(); for branch in any_of { if is_null_schema(branch) { nullable = true; } else { let (t, n) = ts_type(branch); - ty = t; nullable = nullable || n; + if !types.contains(&t) { + types.push(t); + } } } - return (ty, nullable); + return (join_types(types, " | "), nullable); } match obj.get("type") { @@ -196,6 +219,14 @@ fn ts_type(prop: &Value) -> (String, bool) { } } +fn join_types(types: Vec, separator: &str) -> String { + if types.is_empty() { + "unknown".to_string() + } else { + types.join(separator) + } +} + /// Map a JSON Schema scalar/array `type` to its TypeScript spelling. fn scalar(ty: &str, obj: &serde_json::Map) -> String { match ty { @@ -334,6 +365,43 @@ mod tests { assert!(ts.contains("child?: Thing | null;"), "{ts}"); } + #[test] + fn emits_anyof_union_and_allof_ref_wrapper() { + let schema = json!({ + "type": "object", + "additionalProperties": false, + "properties": {}, + "definitions": { + "Kind": { + "enum": ["file", "dir"], + "type": "string" + }, + "ObjectEntry": { + "type": "object", + "additionalProperties": false, + "required": ["type"], + "properties": { + "type": { + "allOf": [{ "$ref": "#/definitions/Kind" }] + } + } + }, + "Entry": { + "anyOf": [ + { "type": "string" }, + { "$ref": "#/definitions/ObjectEntry" } + ] + } + } + }); + let ts = emit_ts(&schema); + assert!( + ts.contains("export type Entry = string | ObjectEntry;"), + "{ts}" + ); + assert!(ts.contains("type: Kind;"), "{ts}"); + } + #[test] fn open_object_gets_index_signature_closed_does_not() { let schema = json!({ diff --git a/src/core/wxc_common/src/wire.rs b/src/core/wxc_common/src/wire.rs index f6eeab029..5100af6d9 100644 --- a/src/core/wxc_common/src/wire.rs +++ b/src/core/wxc_common/src/wire.rs @@ -243,8 +243,41 @@ pub struct Filesystem { pub readwrite_paths: Option>, /// Paths the process can read but not write. pub readonly_paths: Option>, - /// Paths explicitly denied (override broader allow rules). - pub denied_paths: Option>, + /// Paths explicitly denied (override broader allow rules). Entries may be + /// bare strings for backwards compatibility or objects with an explicit + /// file/dir mask type. + pub denied_paths: Option>, +} + +/// A denied filesystem path, accepted as either a legacy string or an object +/// with explicit mask intent. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] +#[serde(untagged)] +pub enum DeniedPath { + Path(String), + Object(DeniedPathObject), +} + +/// Object form for a denied filesystem path. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct DeniedPathObject { + /// Path to deny. + pub path: String, + /// Whether to mask the path as a file or directory. + #[serde(rename = "type")] + pub kind: DeniedPathKind, +} + +/// Mask type for a denied filesystem path. +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] +#[serde(rename_all = "lowercase")] +pub enum DeniedPathKind { + File, + Dir, } /// AppContainer DACL-mutation fallback policy. From b7bfe39a2bbb300989bb8c6f863377427dfb637c Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Mon, 13 Jul 2026 14:06:39 -0700 Subject: [PATCH 2/4] [LXC] Observe host path for denied-mask kind + tolerate trailing-slash keys Address PR review feedback on denied-path masking: - `observed_mask_path_kind` now takes `&Path` and inspects the *host* path instead of the container rootfs path (`.../rootfs/`). The rootfs path does not exist before mounts are applied, so the previous code returned `NotFound` for real host denied paths and spuriously demanded an explicit `type`, breaking existing LXC object-policy validation. Also drops the `to_string_lossy()` allocation by passing `&Path` directly. - Add `lookup_mask_kind` with trailing-separator normalization so an explicit `type` is not dropped when the resolved mount path and the configured `denied_path_kinds` key differ only by a trailing slash. - Add unit tests for host-path observation (dir/file/missing) and normalized kind lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../lxc/common/src/filesystem_mounts.rs | 118 ++++++++++++++++-- 1 file changed, 111 insertions(+), 7 deletions(-) diff --git a/src/backends/lxc/common/src/filesystem_mounts.rs b/src/backends/lxc/common/src/filesystem_mounts.rs index c7102e544..f7dab5198 100644 --- a/src/backends/lxc/common/src/filesystem_mounts.rs +++ b/src/backends/lxc/common/src/filesystem_mounts.rs @@ -9,6 +9,7 @@ //! - `readonlyPaths` → `bind,ro` mount //! - `deniedPaths` → masked (inaccessible inside container) +use std::collections::HashMap; use std::path::Path; use wxc_common::logger::Logger; @@ -41,8 +42,17 @@ enum ObservedMaskPathKind { Symlink, } -fn observed_mask_path_kind(full_path: &str) -> Result, String> { - match std::fs::symlink_metadata(full_path) { +/// Inspect a denied path on the **host** filesystem to infer its mask kind. +/// +/// Uses `symlink_metadata` so a symlink is classified as itself rather than +/// followed (avoids the TOCTOU-prone `is_file` heuristic). A missing path +/// yields `Ok(None)` so the caller can fall back to an explicit `type`. +/// +/// This must observe the host path — not the container rootfs path, which does +/// not exist before mounts are applied — otherwise every host denied path would +/// resolve to `NotFound` and spuriously demand an explicit `type`. +fn observed_mask_path_kind(host_path: &Path) -> Result, String> { + match std::fs::symlink_metadata(host_path) { Ok(metadata) => { let file_type = metadata.file_type(); if file_type.is_dir() { @@ -56,11 +66,42 @@ fn observed_mask_path_kind(full_path: &str) -> Result Ok(None), Err(err) => Err(format!( "Unable to inspect denied path '{}': {}. Set deniedPaths entry to an object with explicit type \"file\" or \"dir\".", - full_path, err + host_path.display(), + err )), } } +/// Trim trailing path separators from a denied-path key so an explicit `type` +/// is not dropped when the resolved mount path and the configured +/// `denied_path_kinds` key differ only by a trailing slash. A root-only path is +/// preserved rather than normalized to an empty string. +fn normalize_denied_key(path: &str) -> &str { + let trimmed = path.trim_end_matches(['/', '\\']); + if trimmed.is_empty() { + path + } else { + trimmed + } +} + +/// Look up the explicit mask kind for a denied host path, tolerating a +/// trailing-separator mismatch between the resolved mount path and the +/// configured key. The exact match is tried first as a fast path. +fn lookup_mask_kind( + denied_path_kinds: &HashMap, + host_path: &str, +) -> Option { + if let Some(kind) = denied_path_kinds.get(host_path) { + return Some(*kind); + } + let normalized = normalize_denied_key(host_path); + denied_path_kinds + .iter() + .find(|(key, _)| normalize_denied_key(key) == normalized) + .map(|(_, kind)| *kind) +} + fn resolve_mask_kind( denied_path: &str, explicit: Option, @@ -115,13 +156,11 @@ pub fn configure_filesystem_mounts( container.set_config_item("lxc.mount.entry", &mount_entry)?; } FsIntent::Denied => { - let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name()); - let full_path = Path::new(&rootfs_base).join(container_path); - let explicit = policy.denied_path_kinds.get(host_path).copied(); + let explicit = lookup_mask_kind(&policy.denied_path_kinds, host_path); let kind = resolve_mask_kind( host_path, explicit, - observed_mask_path_kind(&full_path.to_string_lossy())?, + observed_mask_path_kind(Path::new(host_path))?, )?; let mount_entry = match kind { @@ -249,4 +288,69 @@ mod tests { assert!(err.contains("does not exist")); assert!(err.contains("type")); } + + #[test] + fn observed_mask_path_kind_reports_host_dir_file_and_missing() { + use std::fs; + + let base = std::env::temp_dir().join(format!( + "mxc_observed_mask_kind_{}_{:?}", + std::process::id(), + std::thread::current().id() + )); + let dir = base.join("a_dir"); + let file = base.join("a_file"); + fs::create_dir_all(&dir).unwrap(); + fs::write(&file, b"x").unwrap(); + + assert_eq!( + observed_mask_path_kind(&dir).unwrap(), + Some(ObservedMaskPathKind::Dir) + ); + assert_eq!( + observed_mask_path_kind(&file).unwrap(), + Some(ObservedMaskPathKind::File) + ); + assert_eq!( + observed_mask_path_kind(&base.join("does_not_exist")).unwrap(), + None + ); + + fs::remove_dir_all(&base).ok(); + } + + #[test] + fn lookup_mask_kind_prefers_exact_match() { + let mut kinds = HashMap::new(); + kinds.insert("/data/secret".to_string(), MaskKind::File); + assert_eq!( + lookup_mask_kind(&kinds, "/data/secret"), + Some(MaskKind::File) + ); + } + + #[test] + fn lookup_mask_kind_tolerates_trailing_separator_mismatch() { + // Configured key lacks the trailing slash the resolved mount path carries. + let mut kinds = HashMap::new(); + kinds.insert("/data/secret".to_string(), MaskKind::Dir); + assert_eq!( + lookup_mask_kind(&kinds, "/data/secret/"), + Some(MaskKind::Dir) + ); + + // Configured key carries a trailing slash the resolved mount path lacks. + let mut kinds = HashMap::new(); + kinds.insert("/data/secret/".to_string(), MaskKind::File); + assert_eq!( + lookup_mask_kind(&kinds, "/data/secret"), + Some(MaskKind::File) + ); + } + + #[test] + fn lookup_mask_kind_absent_returns_none() { + let kinds = HashMap::new(); + assert_eq!(lookup_mask_kind(&kinds, "/data/secret"), None); + } } From b0a5766d509b41b9ee9e60000cb8e5b41227f7bd Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Tue, 14 Jul 2026 11:44:28 -0700 Subject: [PATCH 3/4] [LXC] Scope PR to most-specific-path resolver; drop file/dir schema work Address review feedback on #630: - Remove the denied-path file/dir `type` discriminator (wire DeniedPath union, MaskKind, denied_path_kinds, regenerated schema + TS wire types, ts_emit anyOf/allOf handling, and the LXC mask-kind logic). Per reviewer guidance the file-vs-dir schema distinction is deferred (to be designed alongside leaf/tree), and the wire change duplicates bwrap #640. - Keep the most-specific-path-wins resolver but extend `filesystem_resolve` in place instead of renaming it to `path_specificity` with a compat shim, avoiding churn / conflict surface with the bwrap branches. Delete path_specificity.rs and the shim. - Fix the `dedup_by` dead code: the retained element is the earlier `b`, so mutating `a` was a no-op. Rely on the most-restrictive-first sort (with a comment) and add a test asserting the kept path string on an exact-key conflict across lists that differ only by a trailing slash. - Revert LXC denied-path masking to its original behavior while still emitting mounts in resolver (shallow->deep) order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- schemas/dev/mxc-config.schema.0.8.0-dev.json | 46 +-- sdk/src/generated/wire.ts | 28 +- .../lxc/common/src/filesystem_mounts.rs | 243 +------------- src/core/wxc_common/src/config_parser.rs | 69 +--- src/core/wxc_common/src/filesystem_resolve.rs | 313 +++++++++++++++++- src/core/wxc_common/src/lib.rs | 1 - src/core/wxc_common/src/models.rs | 13 - src/core/wxc_common/src/path_specificity.rs | 266 --------------- src/core/wxc_common/src/ts_emit.rs | 74 +---- src/core/wxc_common/src/wire.rs | 37 +-- 10 files changed, 338 insertions(+), 752 deletions(-) delete mode 100644 src/core/wxc_common/src/path_specificity.rs diff --git a/schemas/dev/mxc-config.schema.0.8.0-dev.json b/schemas/dev/mxc-config.schema.0.8.0-dev.json index 2be1c89cc..25c500333 100644 --- a/schemas/dev/mxc-config.schema.0.8.0-dev.json +++ b/schemas/dev/mxc-config.schema.0.8.0-dev.json @@ -136,48 +136,6 @@ } ] }, - "DeniedPath": { - "anyOf": [ - { - "type": "string" - }, - { - "$ref": "#/definitions/DeniedPathObject" - } - ], - "description": "A denied filesystem path, accepted as either a legacy string or an object with explicit mask intent." - }, - "DeniedPathKind": { - "description": "Mask type for a denied filesystem path.", - "enum": [ - "file", - "dir" - ], - "type": "string" - }, - "DeniedPathObject": { - "additionalProperties": false, - "description": "Object form for a denied filesystem path.", - "properties": { - "path": { - "description": "Path to deny.", - "type": "string" - }, - "type": { - "allOf": [ - { - "$ref": "#/definitions/DeniedPathKind" - } - ], - "description": "Whether to mask the path as a file or directory." - } - }, - "required": [ - "path", - "type" - ], - "type": "object" - }, "Experimental": { "description": "Experimental features (only honored with `--experimental`). This block is intentionally **permissive** (no `deny_unknown_fields`): experimental backends are in flux, so the schema documents the known shapes for editor help without rejecting in-progress fields. The strict, closed contract is the stable (top-level) surface.", "properties": { @@ -269,9 +227,9 @@ "description": "Filesystem access policy.", "properties": { "deniedPaths": { - "description": "Paths explicitly denied (override broader allow rules). Entries may be bare strings for backwards compatibility or objects with an explicit file/dir mask type.", + "description": "Paths explicitly denied (override broader allow rules).", "items": { - "$ref": "#/definitions/DeniedPath" + "type": "string" }, "type": [ "array", diff --git a/sdk/src/generated/wire.ts b/sdk/src/generated/wire.ts index 71d90104a..4e0026edb 100644 --- a/sdk/src/generated/wire.ts +++ b/sdk/src/generated/wire.ts @@ -47,30 +47,6 @@ export type ClipboardPolicy = "none" | "read" | "write" | "all"; */ export type Containment = "process" | "processcontainer" | "vm" | "windows_sandbox" | "lxc" | "microvm" | "hyperlight" | "wslc" | "seatbelt" | "isolation_session" | "bubblewrap"; -/** - * A denied filesystem path, accepted as either a legacy string or an object with explicit mask intent. - */ -export type DeniedPath = string | DeniedPathObject; - -/** - * Mask type for a denied filesystem path. - */ -export type DeniedPathKind = "file" | "dir"; - -/** - * Object form for a denied filesystem path. - */ -export interface DeniedPathObject { - /** - * Path to deny. - */ - path: string; - /** - * Whether to mask the path as a file or directory. - */ - type: DeniedPathKind; -} - /** * Experimental features (only honored with `--experimental`). This block is intentionally **permissive** (no `deny_unknown_fields`): experimental backends are in flux, so the schema documents the known shapes for editor help without rejecting in-progress fields. The strict, closed contract is the stable (top-level) surface. */ @@ -117,9 +93,9 @@ export interface Fallback { */ export interface Filesystem { /** - * Paths explicitly denied (override broader allow rules). Entries may be bare strings for backwards compatibility or objects with an explicit file/dir mask type. + * Paths explicitly denied (override broader allow rules). */ - deniedPaths?: DeniedPath[] | null; + deniedPaths?: string[] | null; /** * Paths the process can read but not write. */ diff --git a/src/backends/lxc/common/src/filesystem_mounts.rs b/src/backends/lxc/common/src/filesystem_mounts.rs index f7dab5198..24b184b0b 100644 --- a/src/backends/lxc/common/src/filesystem_mounts.rs +++ b/src/backends/lxc/common/src/filesystem_mounts.rs @@ -9,12 +9,9 @@ //! - `readonlyPaths` → `bind,ro` mount //! - `deniedPaths` → masked (inaccessible inside container) -use std::collections::HashMap; -use std::path::Path; - +use wxc_common::filesystem_resolve::{resolve_mount_order, FsIntent}; use wxc_common::logger::Logger; -use wxc_common::models::{ContainerPolicy, MaskKind}; -use wxc_common::path_specificity::{resolve_mount_order, FsIntent}; +use wxc_common::models::ContainerPolicy; use crate::lxc_bindings::LxcContainer; @@ -35,92 +32,6 @@ fn validate_path(path: &str) -> Result<(), String> { Ok(()) } -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -enum ObservedMaskPathKind { - File, - Dir, - Symlink, -} - -/// Inspect a denied path on the **host** filesystem to infer its mask kind. -/// -/// Uses `symlink_metadata` so a symlink is classified as itself rather than -/// followed (avoids the TOCTOU-prone `is_file` heuristic). A missing path -/// yields `Ok(None)` so the caller can fall back to an explicit `type`. -/// -/// This must observe the host path — not the container rootfs path, which does -/// not exist before mounts are applied — otherwise every host denied path would -/// resolve to `NotFound` and spuriously demand an explicit `type`. -fn observed_mask_path_kind(host_path: &Path) -> Result, String> { - match std::fs::symlink_metadata(host_path) { - Ok(metadata) => { - let file_type = metadata.file_type(); - if file_type.is_dir() { - Ok(Some(ObservedMaskPathKind::Dir)) - } else if file_type.is_symlink() { - Ok(Some(ObservedMaskPathKind::Symlink)) - } else { - Ok(Some(ObservedMaskPathKind::File)) - } - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), - Err(err) => Err(format!( - "Unable to inspect denied path '{}': {}. Set deniedPaths entry to an object with explicit type \"file\" or \"dir\".", - host_path.display(), - err - )), - } -} - -/// Trim trailing path separators from a denied-path key so an explicit `type` -/// is not dropped when the resolved mount path and the configured -/// `denied_path_kinds` key differ only by a trailing slash. A root-only path is -/// preserved rather than normalized to an empty string. -fn normalize_denied_key(path: &str) -> &str { - let trimmed = path.trim_end_matches(['/', '\\']); - if trimmed.is_empty() { - path - } else { - trimmed - } -} - -/// Look up the explicit mask kind for a denied host path, tolerating a -/// trailing-separator mismatch between the resolved mount path and the -/// configured key. The exact match is tried first as a fast path. -fn lookup_mask_kind( - denied_path_kinds: &HashMap, - host_path: &str, -) -> Option { - if let Some(kind) = denied_path_kinds.get(host_path) { - return Some(*kind); - } - let normalized = normalize_denied_key(host_path); - denied_path_kinds - .iter() - .find(|(key, _)| normalize_denied_key(key) == normalized) - .map(|(_, kind)| *kind) -} - -fn resolve_mask_kind( - denied_path: &str, - explicit: Option, - observed: Option, -) -> Result { - if let Some(kind) = explicit { - return Ok(kind); - } - - match observed { - Some(ObservedMaskPathKind::Dir) => Ok(MaskKind::Dir), - Some(ObservedMaskPathKind::File | ObservedMaskPathKind::Symlink) => Ok(MaskKind::File), - None => Err(format!( - "Denied path '{}' does not exist and no mask type was specified. Use deniedPaths object form {{\"path\":\"{}\",\"type\":\"file\"}} or {{\"path\":\"{}\",\"type\":\"dir\"}}.", - denied_path, denied_path, denied_path - )), - } -} - /// Configure filesystem bind mounts on the container based on the policy. /// /// Adds `lxc.mount.entry` config items for each path in the policy. @@ -156,25 +67,20 @@ pub fn configure_filesystem_mounts( container.set_config_item("lxc.mount.entry", &mount_entry)?; } FsIntent::Denied => { - let explicit = lookup_mask_kind(&policy.denied_path_kinds, host_path); - let kind = resolve_mask_kind( - host_path, - explicit, - observed_mask_path_kind(Path::new(host_path))?, - )?; - - let mount_entry = match kind { - MaskKind::File => { - format!("/dev/null {} none bind,ro,create=file 0 0", container_path) - } - MaskKind::Dir => { - format!("tmpfs {} tmpfs ro,size=0,create=dir 0 0", container_path) - } - }; - let create_type = match kind { - MaskKind::File => "file", - MaskKind::Dir => "dir", + // Determine if the path is a file or directory inside the + // container rootfs so we use the correct LXC `create=` type for + // the mount entry. + let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name()); + let full_path = format!("{}/{}", rootfs_base, container_path); + + // Use /dev/null bind mount for files, tmpfs for directories. + let is_file = std::path::Path::new(&full_path).is_file(); + let mount_entry = if is_file { + format!("/dev/null {} none bind,ro,create=file 0 0", container_path) + } else { + format!("tmpfs {} tmpfs ro,size=0,create=dir 0 0", container_path) }; + let create_type = if is_file { "file" } else { "dir" }; logger.log_line(&format!( "Masking denied path: /{} ({})", container_path, create_type @@ -234,123 +140,4 @@ mod tests { fn test_validate_path_accepts_normal() { assert!(validate_path("/mnt/shared").is_ok()); } - - #[test] - fn explicit_mask_kind_wins_over_observed_kind() { - assert_eq!( - resolve_mask_kind( - "/etc/shadow", - Some(MaskKind::File), - Some(ObservedMaskPathKind::Dir) - ) - .unwrap(), - MaskKind::File - ); - assert_eq!( - resolve_mask_kind( - "/var/lib/app", - Some(MaskKind::Dir), - Some(ObservedMaskPathKind::File) - ) - .unwrap(), - MaskKind::Dir - ); - } - - #[test] - fn observed_symlink_and_regular_file_use_file_mask() { - assert_eq!( - resolve_mask_kind( - "/etc/alternatives/editor", - None, - Some(ObservedMaskPathKind::Symlink) - ) - .unwrap(), - MaskKind::File - ); - assert_eq!( - resolve_mask_kind("/etc/shadow", None, Some(ObservedMaskPathKind::File)).unwrap(), - MaskKind::File - ); - } - - #[test] - fn observed_directory_uses_dir_mask() { - assert_eq!( - resolve_mask_kind("/var/lib/app", None, Some(ObservedMaskPathKind::Dir)).unwrap(), - MaskKind::Dir - ); - } - - #[test] - fn missing_path_without_explicit_kind_errors() { - let err = resolve_mask_kind("/missing/file", None, None).unwrap_err(); - assert!(err.contains("does not exist")); - assert!(err.contains("type")); - } - - #[test] - fn observed_mask_path_kind_reports_host_dir_file_and_missing() { - use std::fs; - - let base = std::env::temp_dir().join(format!( - "mxc_observed_mask_kind_{}_{:?}", - std::process::id(), - std::thread::current().id() - )); - let dir = base.join("a_dir"); - let file = base.join("a_file"); - fs::create_dir_all(&dir).unwrap(); - fs::write(&file, b"x").unwrap(); - - assert_eq!( - observed_mask_path_kind(&dir).unwrap(), - Some(ObservedMaskPathKind::Dir) - ); - assert_eq!( - observed_mask_path_kind(&file).unwrap(), - Some(ObservedMaskPathKind::File) - ); - assert_eq!( - observed_mask_path_kind(&base.join("does_not_exist")).unwrap(), - None - ); - - fs::remove_dir_all(&base).ok(); - } - - #[test] - fn lookup_mask_kind_prefers_exact_match() { - let mut kinds = HashMap::new(); - kinds.insert("/data/secret".to_string(), MaskKind::File); - assert_eq!( - lookup_mask_kind(&kinds, "/data/secret"), - Some(MaskKind::File) - ); - } - - #[test] - fn lookup_mask_kind_tolerates_trailing_separator_mismatch() { - // Configured key lacks the trailing slash the resolved mount path carries. - let mut kinds = HashMap::new(); - kinds.insert("/data/secret".to_string(), MaskKind::Dir); - assert_eq!( - lookup_mask_kind(&kinds, "/data/secret/"), - Some(MaskKind::Dir) - ); - - // Configured key carries a trailing slash the resolved mount path lacks. - let mut kinds = HashMap::new(); - kinds.insert("/data/secret/".to_string(), MaskKind::File); - assert_eq!( - lookup_mask_kind(&kinds, "/data/secret"), - Some(MaskKind::File) - ); - } - - #[test] - fn lookup_mask_kind_absent_returns_none() { - let kinds = HashMap::new(); - assert_eq!(lookup_mask_kind(&kinds, "/data/secret"), None); - } } diff --git a/src/core/wxc_common/src/config_parser.rs b/src/core/wxc_common/src/config_parser.rs index b4f6dbfc2..9696e5aac 100644 --- a/src/core/wxc_common/src/config_parser.rs +++ b/src/core/wxc_common/src/config_parser.rs @@ -9,9 +9,9 @@ use crate::error::WxcError; use crate::logger::Logger; use crate::models::{ ContainerPolicy, ContainmentBackend, ExecutionRequest, ExperimentalConfig, - IsolationSessionConfig, LifecycleConfig, LxcConfig, MaskKind, NetworkEnforcementMode, - NetworkPolicy, PortMapping, ProxyAddress, ProxyConfig, SeatbeltConfig, TelemetryConfig, - TestFeatureConfig, UiPolicy, WindowsSandboxConfig, WslcConfig, + IsolationSessionConfig, LifecycleConfig, LxcConfig, NetworkEnforcementMode, NetworkPolicy, + PortMapping, ProxyAddress, ProxyConfig, SeatbeltConfig, TelemetryConfig, TestFeatureConfig, + UiPolicy, WindowsSandboxConfig, WslcConfig, }; use crate::mxc_error::MxcError; use crate::state_aware_request::{MxcRequest, ParsedStateAwareRequest, Phase}; @@ -300,19 +300,6 @@ fn validate_paths(paths: &[String], logger: &mut Logger) -> Result<(), WxcError> Ok(()) } -fn convert_denied_path(entry: wire::DeniedPath) -> (String, Option) { - match entry { - wire::DeniedPath::Path(path) => (path, None), - wire::DeniedPath::Object(obj) => { - let kind = match obj.kind { - wire::DeniedPathKind::File => MaskKind::File, - wire::DeniedPathKind::Dir => MaskKind::Dir, - }; - (obj.path, Some(kind)) - } - } -} - /// Normalizes cross-list filesystem path constraints by applying /// **most-restrictive-wins** precedence (`deny` > `readonly` > `readwrite`): /// @@ -774,13 +761,7 @@ fn convert_wire_config( // Filesystem section if let Some(fscfg) = cfg.filesystem { if let Some(v) = fscfg.denied_paths { - for entry in v { - let (path, kind) = convert_denied_path(entry); - if let Some(kind) = kind { - policy.denied_path_kinds.insert(path.clone(), kind); - } - policy.denied_paths.push(path); - } + policy.denied_paths = v; } if let Some(v) = fscfg.readwrite_paths { policy.readwrite_paths = v; @@ -1745,48 +1726,6 @@ mod tests { assert_eq!(req.policy.denied_paths[1], "C:\\Program Files"); } - #[test] - fn filesystem_denied_paths_accept_string_and_typed_object() { - let json = r#"{ - "process": {"commandLine": "print('test')"}, - "filesystem": { - "deniedPaths": [ - "C:\\Windows\\System32", - {"path": "C:\\Users\\Public\\secret.txt", "type": "file"}, - {"path": "C:\\Users\\Public\\secret-dir", "type": "dir"} - ] - } - }"#; - let encoded = base64_encode(json.as_bytes()); - let mut logger = test_logger(); - - let req = load_request(&encoded, &mut logger, true).unwrap(); - assert_eq!( - req.policy.denied_paths, - vec![ - "C:\\Windows\\System32", - "C:\\Users\\Public\\secret.txt", - "C:\\Users\\Public\\secret-dir" - ] - ); - assert_eq!( - req.policy - .denied_path_kinds - .get("C:\\Users\\Public\\secret.txt"), - Some(&MaskKind::File) - ); - assert_eq!( - req.policy - .denied_path_kinds - .get("C:\\Users\\Public\\secret-dir"), - Some(&MaskKind::Dir) - ); - assert!(!req - .policy - .denied_path_kinds - .contains_key("C:\\Windows\\System32")); - } - #[test] fn block_evil_filesystem_paths() { let json = r#"{ diff --git a/src/core/wxc_common/src/filesystem_resolve.rs b/src/core/wxc_common/src/filesystem_resolve.rs index 5ade9f977..bf793331c 100644 --- a/src/core/wxc_common/src/filesystem_resolve.rs +++ b/src/core/wxc_common/src/filesystem_resolve.rs @@ -1,7 +1,314 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -//! Compatibility re-export for callers that used the original resolver module. +//! Filesystem-policy **most-specific-path-wins** mount ordering. +//! +//! The Linux containment backends (Bubblewrap, LXC) realise the filesystem +//! policy as an ordered list of mounts, and the kernel applies "the last mount +//! at a path wins". Overlap between a `readwritePaths` / `readonlyPaths` / +//! `deniedPaths` entry and one of its ancestors or descendants is therefore +//! resolved by **emission order**, not by path specificity. +//! +//! This module normalizes policy paths by component, collapses exact-path +//! conflicts with **most-restrictive-wins** (`denied` > `readonly` > +//! `readwrite`), and returns a **shallow-to-deep** plan so that a backend which +//! emits the plan in order has the deepest (most specific) intent win at every +//! path, regardless of which list it came from. -pub use crate::path_specificity::{effective_intent, resolve_mount_order, resolve_path_plan}; -pub use crate::path_specificity::{FsIntent, ResolvedMount}; +use crate::models::ContainerPolicy; + +/// The access intent a resolved mount carries into the backend, mapped from the +/// policy list the path came from. Ordered least- to most-restrictive so an +/// exact same-path tie resolves most-restrictive-wins. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] +pub enum FsIntent { + /// From `readwritePaths` — read+write bind. + ReadWrite, + /// From `readonlyPaths` — read-only bind. + ReadOnly, + /// From `deniedPaths` — masked (invisible) inside the sandbox. + Denied, +} + +/// A single policy path paired with its intent, ready for a backend to emit as +/// a mount. Ordered relative to its peers by [`resolve_path_plan`]. +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct ResolvedMount { + /// The host path exactly as it appears in the policy (not normalized — the + /// backend still owns path-to-mount translation). + pub path: String, + /// The intent this path was declared with. + pub intent: FsIntent, +} + +/// A path split into its non-empty components, so `/data` and `/data/` compare +/// equal and `/data/secrets` is recognized as a descendant of `/data`. Splits on +/// both `/` and `\` so Windows-style host paths compare the same way. +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] +struct PathKey(Vec); + +impl PathKey { + fn from_path(path: &str) -> Self { + Self( + path.split(['/', '\\']) + .filter(|component| !component.is_empty()) + .map(str::to_string) + .collect(), + ) + } + + fn depth(&self) -> usize { + self.0.len() + } + + fn is_prefix_of(&self, other: &Self) -> bool { + self.0.len() <= other.0.len() && self.0.iter().zip(other.0.iter()).all(|(a, b)| a == b) + } +} + +#[derive(Clone, Debug)] +struct Candidate { + path: String, + key: PathKey, + intent: FsIntent, + sequence: usize, +} + +/// Resolve the three policy lists into a single shallow-to-deep ordered plan. +/// +/// Exact same-path conflicts (equal [`PathKey`], including entries that differ +/// only by a trailing separator) collapse to the single most-restrictive +/// intent; the surviving entry keeps that intent's original path spelling. +pub fn resolve_path_plan( + readwrite_paths: &[String], + readonly_paths: &[String], + denied_paths: &[String], +) -> Vec { + let mut candidates = + Vec::with_capacity(readwrite_paths.len() + readonly_paths.len() + denied_paths.len()); + let mut sequence = 0; + for (paths, intent) in [ + (readwrite_paths, FsIntent::ReadWrite), + (readonly_paths, FsIntent::ReadOnly), + (denied_paths, FsIntent::Denied), + ] { + for path in paths { + candidates.push(Candidate { + path: path.clone(), + key: PathKey::from_path(path), + intent, + sequence, + }); + sequence += 1; + } + } + + // Group equal paths together and, within each group, place the + // most-restrictive entry first (intent descending), then original sequence + // for stability. + candidates.sort_by(|a, b| { + a.key + .cmp(&b.key) + .then_with(|| b.intent.cmp(&a.intent)) + .then_with(|| a.sequence.cmp(&b.sequence)) + }); + // Collapse each equal-path group to its most-restrictive entry. The sort + // above already surfaced that entry as the first of each consecutive-equal + // run, and `dedup_by` keeps the first element of a run and drops the rest. + // Note `dedup_by(|a, b| …)` passes the *later* element as `a` and the + // *retained* earlier element as `b`, so mutating `a` here would be dead + // code — we rely on the sort ordering rather than mutating the survivor. + candidates.dedup_by(|a, b| a.key == b.key); + // Re-order the survivors shallow-to-deep so a backend emitting them in order + // lets the deepest (most specific) intent win. Equal-depth ties keep their + // original category order (read-write, then read-only, then denied). + candidates.sort_by(|a, b| { + a.key + .depth() + .cmp(&b.key.depth()) + .then_with(|| a.sequence.cmp(&b.sequence)) + }); + + candidates + .into_iter() + .map(|candidate| ResolvedMount { + path: candidate.path, + intent: candidate.intent, + }) + .collect() +} + +/// Convenience wrapper over [`resolve_path_plan`] for a whole [`ContainerPolicy`]. +pub fn resolve_mount_order(policy: &ContainerPolicy) -> Vec { + resolve_path_plan( + &policy.readwrite_paths, + &policy.readonly_paths, + &policy.denied_paths, + ) +} + +/// Return the effective intent a resolved `plan` assigns to `path`: the intent +/// of the deepest plan entry that is an ancestor of (or equal to) `path`, with +/// most-restrictive-wins breaking an exact-depth tie. `None` if no entry covers +/// the path. +pub fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option { + let query = PathKey::from_path(path); + plan.iter() + .filter_map(|mount| { + let key = PathKey::from_path(&mount.path); + key.is_prefix_of(&query) + .then_some((key.depth(), mount.intent)) + }) + .max_by(|(left_depth, left_intent), (right_depth, right_intent)| { + left_depth + .cmp(right_depth) + .then_with(|| left_intent.cmp(right_intent)) + }) + .map(|(_, intent)| intent) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn strings(paths: &[&str]) -> Vec { + paths.iter().map(|path| (*path).to_string()).collect() + } + + fn plan(rw: &[&str], ro: &[&str], denied: &[&str]) -> Vec { + resolve_path_plan(&strings(rw), &strings(ro), &strings(denied)) + } + + fn order(mounts: &[ResolvedMount]) -> Vec<(&str, FsIntent)> { + mounts + .iter() + .map(|mount| (mount.path.as_str(), mount.intent)) + .collect() + } + + #[test] + fn nested_paths_are_ordered_shallow_to_deep() { + let resolved = plan(&["/workspace"], &["/workspace/.git"], &["/workspace/.env"]); + assert_eq!( + order(&resolved), + vec![ + ("/workspace", FsIntent::ReadWrite), + ("/workspace/.git", FsIntent::ReadOnly), + ("/workspace/.env", FsIntent::Denied), + ] + ); + assert_eq!( + effective_intent(&resolved, "/workspace/src/main.rs"), + Some(FsIntent::ReadWrite) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/.git/config"), + Some(FsIntent::ReadOnly) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/.env"), + Some(FsIntent::Denied) + ); + } + + #[test] + fn exact_path_conflict_uses_most_restrictive_intent() { + let resolved = plan(&["/workspace"], &["/workspace"], &["/workspace"]); + assert_eq!(order(&resolved), vec![("/workspace", FsIntent::Denied)]); + assert_eq!( + effective_intent(&resolved, "/workspace/file"), + Some(FsIntent::Denied) + ); + } + + #[test] + fn readonly_wins_exact_path_conflict_over_readwrite() { + let resolved = plan(&["/workspace/"], &["/workspace"], &[]); + assert_eq!(order(&resolved), vec![("/workspace", FsIntent::ReadOnly)]); + } + + #[test] + fn exact_key_conflict_keeps_most_restrictive_path_string() { + // "/data" (read-write) and "/data/" (denied) collapse to the same + // PathKey because a trailing separator is not significant. The + // most-restrictive (denied) entry must survive, and the retained mount + // must carry *denied's* exact path spelling ("/data/"), not the + // read-write one — proving the collapse keeps the correct entry rather + // than relying on a mutation of the discarded element. + let resolved = plan(&["/data"], &[], &["/data/"]); + assert_eq!(order(&resolved), vec![("/data/", FsIntent::Denied)]); + + // Same conflict with the trailing slash on the read-write spelling and + // the denied entry bare: denied still wins and keeps its own "/data". + let resolved = plan(&["/data/"], &[], &["/data"]); + assert_eq!(order(&resolved), vec![("/data", FsIntent::Denied)]); + } + + #[test] + fn siblings_do_not_override_each_other() { + let resolved = plan(&["/workspace/src"], &["/workspace/docs"], &[]); + assert_eq!( + effective_intent(&resolved, "/workspace/src/lib.rs"), + Some(FsIntent::ReadWrite) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/docs/index.md"), + Some(FsIntent::ReadOnly) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/tests/test.rs"), + None + ); + } + + #[test] + fn unrelated_paths_remain_independent() { + let resolved = plan(&["/srv/app/data"], &["/usr"], &["/opt/secret"]); + assert_eq!( + order(&resolved), + vec![ + ("/usr", FsIntent::ReadOnly), + ("/opt/secret", FsIntent::Denied), + ("/srv/app/data", FsIntent::ReadWrite), + ] + ); + assert_eq!(effective_intent(&resolved, "/var/log/app.log"), None); + } + + #[test] + fn deep_override_of_shallow_deny_wins_for_child_only() { + let resolved = plan(&["/data/secrets"], &[], &["/data"]); + assert_eq!( + order(&resolved), + vec![ + ("/data", FsIntent::Denied), + ("/data/secrets", FsIntent::ReadWrite), + ] + ); + assert_eq!(effective_intent(&resolved, "/data"), Some(FsIntent::Denied)); + assert_eq!( + effective_intent(&resolved, "/data/secrets/file"), + Some(FsIntent::ReadWrite) + ); + } + + #[test] + fn component_prefix_does_not_match_partial_component() { + let resolved = plan(&["/workspace"], &[], &[]); + assert_eq!(effective_intent(&resolved, "/workspace2/file"), None); + } + + #[test] + fn backslash_paths_are_compared_by_components() { + let resolved = plan(&["C:\\workspace"], &["C:\\workspace\\.git\\"], &[]); + assert_eq!( + effective_intent(&resolved, "C:\\workspace\\.git\\config"), + Some(FsIntent::ReadOnly) + ); + } + + #[test] + fn empty_policy_yields_no_mounts() { + assert!(plan(&[], &[], &[]).is_empty()); + } +} diff --git a/src/core/wxc_common/src/lib.rs b/src/core/wxc_common/src/lib.rs index 11d8343ec..daaa9ad95 100644 --- a/src/core/wxc_common/src/lib.rs +++ b/src/core/wxc_common/src/lib.rs @@ -17,7 +17,6 @@ pub mod logger; pub mod microvm_staging; pub mod models; pub mod mxc_error; -pub mod path_specificity; pub mod sandbox_process; pub mod script_runner; pub mod state_aware_backend; diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index 65c680c31..f6e403a5f 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use std::collections::HashMap; - use serde::{Deserialize, Serialize}; /// Selects which containment backend to use for script execution. @@ -540,8 +538,6 @@ pub struct ContainerPolicy { pub readwrite_paths: Vec, pub readonly_paths: Vec, pub denied_paths: Vec, - #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub denied_path_kinds: HashMap, pub fallback: FallbackPolicy, pub default_network_policy: NetworkPolicy, pub network_enforcement_mode: NetworkEnforcementMode, @@ -559,15 +555,6 @@ pub struct ContainerPolicy { pub base_process_ui: BaseProcessUiConfig, } -/// How a denied filesystem path should be masked when the backend must choose -/// between file and directory mount primitives. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "lowercase")] -pub enum MaskKind { - File, - Dir, -} - /// Port mapping for host↔container port forwarding. #[derive(Debug, Default, Clone, Serialize, Deserialize)] pub struct PortMapping { diff --git a/src/core/wxc_common/src/path_specificity.rs b/src/core/wxc_common/src/path_specificity.rs deleted file mode 100644 index 5f0fcba9b..000000000 --- a/src/core/wxc_common/src/path_specificity.rs +++ /dev/null @@ -1,266 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -//! Filesystem-policy most-specific-path-wins resolution. -//! -//! Backends such as LXC and Bubblewrap apply filesystem mount operations in -//! order. This module normalizes policy paths by component, collapses exact-path -//! conflicts with most-restrictive-wins (`denied` > `readonly` > `readwrite`), -//! and returns a shallow-to-deep plan so later mounts implement -//! most-specific-path-wins. - -use crate::models::ContainerPolicy; - -#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] -pub enum FsIntent { - ReadWrite, - ReadOnly, - Denied, -} - -#[derive(Clone, PartialEq, Eq, Debug)] -pub struct ResolvedMount { - pub path: String, - pub intent: FsIntent, -} - -#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] -struct PathKey(Vec); - -impl PathKey { - fn from_path(path: &str) -> Self { - Self( - path.split(['/', '\\']) - .filter(|component| !component.is_empty()) - .map(str::to_string) - .collect(), - ) - } - - fn depth(&self) -> usize { - self.0.len() - } - - fn is_prefix_of(&self, other: &Self) -> bool { - self.0.len() <= other.0.len() && self.0.iter().zip(other.0.iter()).all(|(a, b)| a == b) - } -} - -#[derive(Clone, Debug)] -struct Candidate { - path: String, - key: PathKey, - intent: FsIntent, - sequence: usize, -} - -pub fn resolve_path_plan( - readwrite_paths: &[String], - readonly_paths: &[String], - denied_paths: &[String], -) -> Vec { - let mut candidates = - Vec::with_capacity(readwrite_paths.len() + readonly_paths.len() + denied_paths.len()); - let mut sequence = 0; - for (paths, intent) in [ - (readwrite_paths, FsIntent::ReadWrite), - (readonly_paths, FsIntent::ReadOnly), - (denied_paths, FsIntent::Denied), - ] { - for path in paths { - candidates.push(Candidate { - path: path.clone(), - key: PathKey::from_path(path), - intent, - sequence, - }); - sequence += 1; - } - } - - candidates.sort_by(|a, b| { - a.key - .cmp(&b.key) - .then_with(|| b.intent.cmp(&a.intent)) - .then_with(|| a.sequence.cmp(&b.sequence)) - }); - candidates.dedup_by(|a, b| { - if a.key == b.key { - if b.intent > a.intent { - a.path = b.path.clone(); - a.intent = b.intent; - a.sequence = b.sequence; - } - true - } else { - false - } - }); - candidates.sort_by(|a, b| { - a.key - .depth() - .cmp(&b.key.depth()) - .then_with(|| a.sequence.cmp(&b.sequence)) - }); - - candidates - .into_iter() - .map(|candidate| ResolvedMount { - path: candidate.path, - intent: candidate.intent, - }) - .collect() -} - -pub fn resolve_mount_order(policy: &ContainerPolicy) -> Vec { - resolve_path_plan( - &policy.readwrite_paths, - &policy.readonly_paths, - &policy.denied_paths, - ) -} - -pub fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option { - let query = PathKey::from_path(path); - plan.iter() - .filter_map(|mount| { - let key = PathKey::from_path(&mount.path); - key.is_prefix_of(&query) - .then_some((key.depth(), mount.intent)) - }) - .max_by(|(left_depth, left_intent), (right_depth, right_intent)| { - left_depth - .cmp(right_depth) - .then_with(|| left_intent.cmp(right_intent)) - }) - .map(|(_, intent)| intent) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn strings(paths: &[&str]) -> Vec { - paths.iter().map(|path| (*path).to_string()).collect() - } - - fn plan(rw: &[&str], ro: &[&str], denied: &[&str]) -> Vec { - resolve_path_plan(&strings(rw), &strings(ro), &strings(denied)) - } - - fn order(mounts: &[ResolvedMount]) -> Vec<(&str, FsIntent)> { - mounts - .iter() - .map(|mount| (mount.path.as_str(), mount.intent)) - .collect() - } - - #[test] - fn nested_paths_are_ordered_shallow_to_deep() { - let resolved = plan(&["/workspace"], &["/workspace/.git"], &["/workspace/.env"]); - assert_eq!( - order(&resolved), - vec![ - ("/workspace", FsIntent::ReadWrite), - ("/workspace/.git", FsIntent::ReadOnly), - ("/workspace/.env", FsIntent::Denied), - ] - ); - assert_eq!( - effective_intent(&resolved, "/workspace/src/main.rs"), - Some(FsIntent::ReadWrite) - ); - assert_eq!( - effective_intent(&resolved, "/workspace/.git/config"), - Some(FsIntent::ReadOnly) - ); - assert_eq!( - effective_intent(&resolved, "/workspace/.env"), - Some(FsIntent::Denied) - ); - } - - #[test] - fn exact_path_conflict_uses_most_restrictive_intent() { - let resolved = plan(&["/workspace"], &["/workspace"], &["/workspace"]); - assert_eq!(order(&resolved), vec![("/workspace", FsIntent::Denied)]); - assert_eq!( - effective_intent(&resolved, "/workspace/file"), - Some(FsIntent::Denied) - ); - } - - #[test] - fn readonly_wins_exact_path_conflict_over_readwrite() { - let resolved = plan(&["/workspace/"], &["/workspace"], &[]); - assert_eq!(order(&resolved), vec![("/workspace", FsIntent::ReadOnly)]); - } - - #[test] - fn siblings_do_not_override_each_other() { - let resolved = plan(&["/workspace/src"], &["/workspace/docs"], &[]); - assert_eq!( - effective_intent(&resolved, "/workspace/src/lib.rs"), - Some(FsIntent::ReadWrite) - ); - assert_eq!( - effective_intent(&resolved, "/workspace/docs/index.md"), - Some(FsIntent::ReadOnly) - ); - assert_eq!( - effective_intent(&resolved, "/workspace/tests/test.rs"), - None - ); - } - - #[test] - fn unrelated_paths_remain_independent() { - let resolved = plan(&["/srv/app/data"], &["/usr"], &["/opt/secret"]); - assert_eq!( - order(&resolved), - vec![ - ("/usr", FsIntent::ReadOnly), - ("/opt/secret", FsIntent::Denied), - ("/srv/app/data", FsIntent::ReadWrite), - ] - ); - assert_eq!(effective_intent(&resolved, "/var/log/app.log"), None); - } - - #[test] - fn deep_override_of_shallow_deny_wins_for_child_only() { - let resolved = plan(&["/data/secrets"], &[], &["/data"]); - assert_eq!( - order(&resolved), - vec![ - ("/data", FsIntent::Denied), - ("/data/secrets", FsIntent::ReadWrite), - ] - ); - assert_eq!(effective_intent(&resolved, "/data"), Some(FsIntent::Denied)); - assert_eq!( - effective_intent(&resolved, "/data/secrets/file"), - Some(FsIntent::ReadWrite) - ); - } - - #[test] - fn component_prefix_does_not_match_partial_component() { - let resolved = plan(&["/workspace"], &[], &[]); - assert_eq!(effective_intent(&resolved, "/workspace2/file"), None); - } - - #[test] - fn backslash_paths_are_compared_by_components() { - let resolved = plan(&["C:\\workspace"], &["C:\\workspace\\.git\\"], &[]); - assert_eq!( - effective_intent(&resolved, "C:\\workspace\\.git\\config"), - Some(FsIntent::ReadOnly) - ); - } - - #[test] - fn empty_policy_yields_no_mounts() { - assert!(plan(&[], &[], &[]).is_empty()); - } -} diff --git a/src/core/wxc_common/src/ts_emit.rs b/src/core/wxc_common/src/ts_emit.rs index bd2fc1c33..2d0a72944 100644 --- a/src/core/wxc_common/src/ts_emit.rs +++ b/src/core/wxc_common/src/ts_emit.rs @@ -83,14 +83,6 @@ fn emit_definition(out: &mut String, name: &str, def: &Value) { return; } - if obj.contains_key("anyOf") || obj.contains_key("allOf") { - push_doc(out, obj.get("description")); - let (ty, nullable) = ts_type(def); - let nul = if nullable { " | null" } else { "" }; - out.push_str(&format!("export type {name} = {ty}{nul};\n\n")); - return; - } - emit_object(out, name, obj_as_map(def)); } @@ -171,34 +163,19 @@ fn ts_type(prop: &Value) -> (String, bool) { return (ref_name(r), false); } - if let Some(Value::Array(all_of)) = obj.get("allOf") { - let mut nullable = false; - let mut types = Vec::new(); - for branch in all_of { - let (t, n) = ts_type(branch); - nullable = nullable || n; - if !types.contains(&t) { - types.push(t); - } - } - return (join_types(types, " & "), nullable); - } - if let Some(Value::Array(any_of)) = obj.get("anyOf") { let mut nullable = false; - let mut types = Vec::new(); + let mut ty = "unknown".to_string(); for branch in any_of { if is_null_schema(branch) { nullable = true; } else { let (t, n) = ts_type(branch); + ty = t; nullable = nullable || n; - if !types.contains(&t) { - types.push(t); - } } } - return (join_types(types, " | "), nullable); + return (ty, nullable); } match obj.get("type") { @@ -219,14 +196,6 @@ fn ts_type(prop: &Value) -> (String, bool) { } } -fn join_types(types: Vec, separator: &str) -> String { - if types.is_empty() { - "unknown".to_string() - } else { - types.join(separator) - } -} - /// Map a JSON Schema scalar/array `type` to its TypeScript spelling. fn scalar(ty: &str, obj: &serde_json::Map) -> String { match ty { @@ -365,43 +334,6 @@ mod tests { assert!(ts.contains("child?: Thing | null;"), "{ts}"); } - #[test] - fn emits_anyof_union_and_allof_ref_wrapper() { - let schema = json!({ - "type": "object", - "additionalProperties": false, - "properties": {}, - "definitions": { - "Kind": { - "enum": ["file", "dir"], - "type": "string" - }, - "ObjectEntry": { - "type": "object", - "additionalProperties": false, - "required": ["type"], - "properties": { - "type": { - "allOf": [{ "$ref": "#/definitions/Kind" }] - } - } - }, - "Entry": { - "anyOf": [ - { "type": "string" }, - { "$ref": "#/definitions/ObjectEntry" } - ] - } - } - }); - let ts = emit_ts(&schema); - assert!( - ts.contains("export type Entry = string | ObjectEntry;"), - "{ts}" - ); - assert!(ts.contains("type: Kind;"), "{ts}"); - } - #[test] fn open_object_gets_index_signature_closed_does_not() { let schema = json!({ diff --git a/src/core/wxc_common/src/wire.rs b/src/core/wxc_common/src/wire.rs index 5100af6d9..f6eeab029 100644 --- a/src/core/wxc_common/src/wire.rs +++ b/src/core/wxc_common/src/wire.rs @@ -243,41 +243,8 @@ pub struct Filesystem { pub readwrite_paths: Option>, /// Paths the process can read but not write. pub readonly_paths: Option>, - /// Paths explicitly denied (override broader allow rules). Entries may be - /// bare strings for backwards compatibility or objects with an explicit - /// file/dir mask type. - pub denied_paths: Option>, -} - -/// A denied filesystem path, accepted as either a legacy string or an object -/// with explicit mask intent. -#[derive(Debug, Clone, Serialize, Deserialize)] -#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] -#[serde(untagged)] -pub enum DeniedPath { - Path(String), - Object(DeniedPathObject), -} - -/// Object form for a denied filesystem path. -#[derive(Debug, Clone, Serialize, Deserialize)] -#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct DeniedPathObject { - /// Path to deny. - pub path: String, - /// Whether to mask the path as a file or directory. - #[serde(rename = "type")] - pub kind: DeniedPathKind, -} - -/// Mask type for a denied filesystem path. -#[derive(Debug, Clone, Copy, Serialize, Deserialize)] -#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))] -#[serde(rename_all = "lowercase")] -pub enum DeniedPathKind { - File, - Dir, + /// Paths explicitly denied (override broader allow rules). + pub denied_paths: Option>, } /// AppContainer DACL-mutation fallback policy. From bae58f54536d3f2b1bebec1984ba3ce4eefb6eff Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Wed, 15 Jul 2026 16:35:13 -0700 Subject: [PATCH 4/4] [LXC] Address review: host-reality denied mask, drop redundant collapse Addresses the change requests on PR #630: - filesystem_mounts.rs: classify denied paths from the HOST path (via symlink_metadata, never following symlinks) instead of the not-yet-existing rootfs path, so a denied host file is masked with /dev/null (create=file) and a directory/symlink/missing path with an empty tmpfs (create=dir). - filesystem_resolve.rs: drop the exact-path most-restrictive collapse from resolve_path_plan; it duplicated the upstream normalize_filesystem_paths (config parser) and normalize_object_conflicts (every runner). The resolver now only orders shallow-to-deep, and emission order still yields most-restrictive-wins for any same-path duplicate that survives upstream. - filesystem_resolve.rs: move the test-only effective_intent oracle into the tests module and gate PathKey::is_prefix_of to test builds. Updated affected unit tests. cargo fmt/clippy clean; wxc_common 398 tests pass; lxc_common + bwrap_common clippy clean on x86_64-unknown-linux-gnu. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../lxc/common/src/filesystem_mounts.rs | 52 +++++- src/core/wxc_common/src/filesystem_resolve.rs | 148 +++++++++++------- 2 files changed, 137 insertions(+), 63 deletions(-) diff --git a/src/backends/lxc/common/src/filesystem_mounts.rs b/src/backends/lxc/common/src/filesystem_mounts.rs index 24b184b0b..912b0bdd4 100644 --- a/src/backends/lxc/common/src/filesystem_mounts.rs +++ b/src/backends/lxc/common/src/filesystem_mounts.rs @@ -32,6 +32,21 @@ fn validate_path(path: &str) -> Result<(), String> { Ok(()) } +/// Classify a denied **host** path as a regular file, to pick the LXC mask type. +/// +/// Denied paths are host paths, so the mask is decided from host reality rather +/// than from the container rootfs (which does not exist until mounts are +/// applied): a regular file is masked with a read-only `/dev/null` bind +/// (`create=file`) and everything else — a directory, a symlink, or a path that +/// does not exist on the host — with an empty read-only tmpfs (`create=dir`). +/// Uses `symlink_metadata` so a symlinked deny is never followed to an +/// unintended target when choosing the mask. +fn denied_path_is_file(host_path: &str) -> bool { + std::fs::symlink_metadata(host_path) + .map(|meta| meta.file_type().is_file()) + .unwrap_or(false) +} + /// Configure filesystem bind mounts on the container based on the policy. /// /// Adds `lxc.mount.entry` config items for each path in the policy. @@ -67,14 +82,15 @@ pub fn configure_filesystem_mounts( container.set_config_item("lxc.mount.entry", &mount_entry)?; } FsIntent::Denied => { - // Determine if the path is a file or directory inside the - // container rootfs so we use the correct LXC `create=` type for - // the mount entry. - let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name()); - let full_path = format!("{}/{}", rootfs_base, container_path); + // Classify the denied path from the HOST path, not the container + // rootfs path. Denied paths are host paths, and the rootfs path + // (`//rootfs/`) does not exist + // until mounts are applied, so inspecting it would always look + // "missing" and mask every deny as a directory. Host reality + // decides the `create=` type instead. + let is_file = denied_path_is_file(host_path); // Use /dev/null bind mount for files, tmpfs for directories. - let is_file = std::path::Path::new(&full_path).is_file(); let mount_entry = if is_file { format!("/dev/null {} none bind,ro,create=file 0 0", container_path) } else { @@ -140,4 +156,28 @@ mod tests { fn test_validate_path_accepts_normal() { assert!(validate_path("/mnt/shared").is_ok()); } + + #[test] + fn denied_path_is_file_reflects_host_reality() { + use std::io::Write; + let base = std::env::temp_dir(); + let unique = format!("mxc_lxc_denied_{}", std::process::id()); + let file_path = base.join(format!("{unique}.file")); + let dir_path = base.join(format!("{unique}.dir")); + let missing = base.join(format!("{unique}.missing")); + + let mut file = std::fs::File::create(&file_path).expect("create temp file"); + writeln!(file, "x").expect("write temp file"); + std::fs::create_dir_all(&dir_path).expect("create temp dir"); + + // Regular host file → masked with /dev/null (create=file). + assert!(denied_path_is_file(&file_path.to_string_lossy())); + // Host directory → masked with an empty tmpfs (create=dir). + assert!(!denied_path_is_file(&dir_path.to_string_lossy())); + // Missing host path → not a file, so masked as an empty directory. + assert!(!denied_path_is_file(&missing.to_string_lossy())); + + let _ = std::fs::remove_file(&file_path); + let _ = std::fs::remove_dir_all(&dir_path); + } } diff --git a/src/core/wxc_common/src/filesystem_resolve.rs b/src/core/wxc_common/src/filesystem_resolve.rs index bf793331c..f7d35417f 100644 --- a/src/core/wxc_common/src/filesystem_resolve.rs +++ b/src/core/wxc_common/src/filesystem_resolve.rs @@ -61,6 +61,10 @@ impl PathKey { self.0.len() } + /// Test-only: whether `self` is an ancestor of (or equal to) `other`. Only + /// the semantic-check oracle in the tests uses this, so it is gated to test + /// builds to keep the production surface free of dead code. + #[cfg(test)] fn is_prefix_of(&self, other: &Self) -> bool { self.0.len() <= other.0.len() && self.0.iter().zip(other.0.iter()).all(|(a, b)| a == b) } @@ -76,9 +80,12 @@ struct Candidate { /// Resolve the three policy lists into a single shallow-to-deep ordered plan. /// -/// Exact same-path conflicts (equal [`PathKey`], including entries that differ -/// only by a trailing separator) collapse to the single most-restrictive -/// intent; the surviving entry keeps that intent's original path spelling. +/// Exact same-path conflicts are resolved *upstream* — the config parser's +/// string-level `normalize_filesystem_paths` and each runner's object-identity +/// `filesystem_object::normalize_object_conflicts` — so this function only +/// **orders**. It emits shallow-to-deep and, for any same-path entries that still +/// arrive, places the most-restrictive intent last, so a backend applying "the +/// last mount at a path wins" resolves the tie most-restrictive-wins. pub fn resolve_path_plan( readwrite_paths: &[String], readonly_paths: &[String], @@ -103,25 +110,20 @@ pub fn resolve_path_plan( } } - // Group equal paths together and, within each group, place the - // most-restrictive entry first (intent descending), then original sequence - // for stability. - candidates.sort_by(|a, b| { - a.key - .cmp(&b.key) - .then_with(|| b.intent.cmp(&a.intent)) - .then_with(|| a.sequence.cmp(&b.sequence)) - }); - // Collapse each equal-path group to its most-restrictive entry. The sort - // above already surfaced that entry as the first of each consecutive-equal - // run, and `dedup_by` keeps the first element of a run and drops the rest. - // Note `dedup_by(|a, b| …)` passes the *later* element as `a` and the - // *retained* earlier element as `b`, so mutating `a` here would be dead - // code — we rely on the sort ordering rather than mutating the survivor. - candidates.dedup_by(|a, b| a.key == b.key); - // Re-order the survivors shallow-to-deep so a backend emitting them in order - // lets the deepest (most specific) intent win. Equal-depth ties keep their - // original category order (read-write, then read-only, then denied). + // Order shallow-to-deep so a backend that emits the plan in order lets the + // deepest (most specific) intent win at every path. Equal-depth ties keep + // their original category order (read-write, then read-only, then denied), + // so an exact same-path duplicate that survives upstream normalization is + // still resolved most-restrictive-wins by emission order: denied is added + // last, sorts last among equal-depth peers, and a backend applying "the last + // mount at a path wins" therefore lands on it. + // + // This resolver deliberately does *not* re-collapse exact same-path + // conflicts — that is already done upstream and duplicating it here adds no + // correctness: the config parser's `normalize_filesystem_paths` drops a + // string-equal path from a looser list, and each runner's + // `filesystem_object::normalize_object_conflicts` tightens same-object + // aliases to the strictest intent before this runs. candidates.sort_by(|a, b| { a.key .depth() @@ -147,26 +149,6 @@ pub fn resolve_mount_order(policy: &ContainerPolicy) -> Vec { ) } -/// Return the effective intent a resolved `plan` assigns to `path`: the intent -/// of the deepest plan entry that is an ancestor of (or equal to) `path`, with -/// most-restrictive-wins breaking an exact-depth tie. `None` if no entry covers -/// the path. -pub fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option { - let query = PathKey::from_path(path); - plan.iter() - .filter_map(|mount| { - let key = PathKey::from_path(&mount.path); - key.is_prefix_of(&query) - .then_some((key.depth(), mount.intent)) - }) - .max_by(|(left_depth, left_intent), (right_depth, right_intent)| { - left_depth - .cmp(right_depth) - .then_with(|| left_intent.cmp(right_intent)) - }) - .map(|(_, intent)| intent) -} - #[cfg(test)] mod tests { use super::*; @@ -186,6 +168,28 @@ mod tests { .collect() } + /// Test oracle: the effective intent a resolved `plan` assigns to `path` — + /// the intent of the deepest plan entry that is an ancestor of (or equal to) + /// `path`, with most-restrictive-wins breaking an exact-depth tie. `None` if + /// no entry covers the path. This models what a backend's "last mount at a + /// path wins" ultimately resolves to, so tests can assert semantics instead + /// of raw emission order. + fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option { + let query = PathKey::from_path(path); + plan.iter() + .filter_map(|mount| { + let key = PathKey::from_path(&mount.path); + key.is_prefix_of(&query) + .then_some((key.depth(), mount.intent)) + }) + .max_by(|(left_depth, left_intent), (right_depth, right_intent)| { + left_depth + .cmp(right_depth) + .then_with(|| left_intent.cmp(right_intent)) + }) + .map(|(_, intent)| intent) + } + #[test] fn nested_paths_are_ordered_shallow_to_deep() { let resolved = plan(&["/workspace"], &["/workspace/.git"], &["/workspace/.env"]); @@ -212,9 +216,20 @@ mod tests { } #[test] - fn exact_path_conflict_uses_most_restrictive_intent() { + fn exact_path_duplicates_emit_most_restrictive_last() { + // Exact same-path conflicts are collapsed upstream (config-parser string + // normalization + object-identity normalization), so the resolver no + // longer re-collapses them. When identical paths do reach it, it orders + // them so the most-restrictive intent is emitted LAST — a backend + // applying "the last mount at a path wins" therefore lands on denied, + // and the semantic oracle agrees. let resolved = plan(&["/workspace"], &["/workspace"], &["/workspace"]); - assert_eq!(order(&resolved), vec![("/workspace", FsIntent::Denied)]); + assert_eq!( + resolved + .last() + .map(|mount| (mount.path.as_str(), mount.intent)), + Some(("/workspace", FsIntent::Denied)) + ); assert_eq!( effective_intent(&resolved, "/workspace/file"), Some(FsIntent::Denied) @@ -222,26 +237,45 @@ mod tests { } #[test] - fn readonly_wins_exact_path_conflict_over_readwrite() { + fn readonly_emitted_after_readwrite_for_same_path() { + // Same path in read-write and read-only: read-only is emitted last so it + // wins by emission order, and the oracle resolves to read-only. let resolved = plan(&["/workspace/"], &["/workspace"], &[]); - assert_eq!(order(&resolved), vec![("/workspace", FsIntent::ReadOnly)]); + assert_eq!( + resolved.last().map(|mount| mount.intent), + Some(FsIntent::ReadOnly) + ); + assert_eq!( + effective_intent(&resolved, "/workspace/file"), + Some(FsIntent::ReadOnly) + ); } #[test] - fn exact_key_conflict_keeps_most_restrictive_path_string() { - // "/data" (read-write) and "/data/" (denied) collapse to the same - // PathKey because a trailing separator is not significant. The - // most-restrictive (denied) entry must survive, and the retained mount - // must carry *denied's* exact path spelling ("/data/"), not the - // read-write one — proving the collapse keeps the correct entry rather - // than relying on a mutation of the discarded element. + fn trailing_slash_variants_resolve_most_restrictive_by_order() { + // "/data" (read-write) and "/data/" (denied) are the same logical path + // but differ by a trailing slash, so the upstream string-level + // normalization does not merge them. The resolver does not collapse them + // either (that would duplicate the upstream / object-identity + // normalization); instead it emits denied last, so emission order yields + // most-restrictive-wins and the semantic oracle agrees. let resolved = plan(&["/data"], &[], &["/data/"]); - assert_eq!(order(&resolved), vec![("/data/", FsIntent::Denied)]); + assert_eq!( + resolved.last().map(|mount| mount.intent), + Some(FsIntent::Denied) + ); + assert_eq!( + effective_intent(&resolved, "/data/file"), + Some(FsIntent::Denied) + ); - // Same conflict with the trailing slash on the read-write spelling and - // the denied entry bare: denied still wins and keeps its own "/data". + // Swap the trailing slash onto the read-write spelling: denied still + // sorts last and wins. let resolved = plan(&["/data/"], &[], &["/data"]); - assert_eq!(order(&resolved), vec![("/data", FsIntent::Denied)]); + assert_eq!( + resolved.last().map(|mount| mount.intent), + Some(FsIntent::Denied) + ); } #[test]