From 410651d7814d184e1bba9d72903ce662584dfeea Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 18:57:39 -0600 Subject: [PATCH] Bound workflow packaging depth and reject invalid supplied configs --- docs/public/agents/mcp.mdx | 5 +- lib/components/fabro-manifest/src/lib.rs | 4 +- .../fabro-manifest/src/supplied_workflow.rs | 13 ++- .../fabro-manifest/src/workflow_bundler.rs | 35 +++++-- .../src/workflow_version_collector.rs | 68 +++++++++++++ .../src/workflow_version_packager.rs | 98 ++++++++++++++++++- 6 files changed, 211 insertions(+), 12 deletions(-) diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index 9ee9bf5a1..e769a4bf3 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -75,7 +75,10 @@ values are text, never host paths or URLs to fetch. Missing references and paths that escape the supplied tree fail. The source tree is limited to 512 files, 512 KiB per file, and 2 MiB of text; each resulting serialized version must also fit the existing 2 MiB API limit. Case-insensitive file and ancestor collisions -are rejected before staging. Collection follows declared file references; command +are rejected before staging. Graph nesting through child workflows and imports +is limited to 64 levels, including the entrypoint. A supplied sibling +`workflow.toml` must be valid even when a graph is the entrypoint; a valid config +that selects another graph is omitted. Collection follows declared file references; command `script` values remain literal text, and paths embedded in shell commands are not inspected or acquired. diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index bac761ffb..bfd5f0162 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -39,8 +39,8 @@ pub use crate::local_workflow_package::{ pub use crate::supplied_workflow::collect_supplied_workflow_versions; use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ - CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, - collect_workflow_versions_at_location, + CollectedWorkflowClosure, MAX_WORKFLOW_VERSION_DEPTH, WorkflowVersionCollectError, + collect_workflow_versions, collect_workflow_versions_at_location, }; pub use crate::workflow_version_packager::SuppliedWorkflowVersionPackager; diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index db95dac8e..064516f64 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -60,7 +60,7 @@ fn collect_in_staging( })?; let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; for (_, version) in closure.versions() { - confine_to_supplied(version.version(), files)?; + confine_to_supplied(version.version(), files, &root)?; } Ok(closure) } @@ -73,6 +73,7 @@ fn collect_in_staging( fn confine_to_supplied( version: &fabro_types::WorkflowVersion, files: &BTreeMap, + root: &Path, ) -> Result<()> { // A supplied sibling config attaches only when its `[workflow].graph` // selects this entrypoint, exactly as for a checkout; several graphs may @@ -92,6 +93,16 @@ fn confine_to_supplied( alias: alias.clone(), }); } + } else if !version.files().contains_key(&config_path) { + // Graph discovery deliberately ignores sibling configs that it cannot + // load. A supplied config may be omitted only if it is valid and selects + // a different graph; malformed settings must not silently disappear. + WorkflowLocation::from_exact_path(Path::new(config_path.as_str()), root).map_err( + |source| WorkflowVersionCollectError::InvalidSuppliedConfig { + path: config_path, + source: Box::new(source), + }, + )?; } // A case-insensitive host must not satisfy a reference that is missing // from the supplied tree under its exact key. diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index f79a15d20..2c56a1310 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -17,7 +17,7 @@ use fabro_template::{ use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; -use crate::{manifest_path_from_absolute, normalize_absolute_path}; +use crate::{manifest_path_from_absolute, normalize_absolute_path, workflow_version_collector}; pub(super) struct WorkflowBundler<'a> { package_root: &'a Path, @@ -64,7 +64,7 @@ impl<'a> WorkflowBundler<'a> { workflow: &Path, project_config: Option<(&ManifestPath, &str)>, ) -> Result> { - let root_key = self.collect_workflow_entry(workflow, self.package_root)?; + let root_key = self.collect_workflow_entry(workflow, self.package_root, 1)?; if let Some((config_path, source)) = project_config { let mut root = self @@ -89,7 +89,7 @@ impl<'a> WorkflowBundler<'a> { root: &WorkflowLocation, ) -> Result { self.workflow_version_projection = true; - let root_key = self.collect_workflow_location(root)?; + let root_key = self.collect_workflow_location(root, 1)?; Ok(CollectedWorkflowSources { root_key, workflows: self.workflows, @@ -97,12 +97,19 @@ impl<'a> WorkflowBundler<'a> { } /// Collects the workflow at `location` and returns its manifest key. - fn collect_workflow_location(&mut self, location: &WorkflowLocation) -> Result { + fn collect_workflow_location( + &mut self, + location: &WorkflowLocation, + depth: usize, + ) -> Result { let dot_path = manifest_path_from_absolute(&location.graph, self.package_root)?; let dot_key = dot_path.to_string(); if !self.visited_workflows.insert(dot_key.clone()) { return Ok(dot_key); } + if self.workflow_version_projection { + workflow_version_collector::check_workflow_depth(depth, &dot_key)?; + } let source = self.read_package_file(&location.graph)?; let config = if let Some(workflow_toml_path) = location.toml.as_ref() { @@ -147,6 +154,7 @@ impl<'a> WorkflowBundler<'a> { &mut visited_imports, &mut dependency_keys, GraphPosition::Entrypoint, + depth, )?; self.workflows @@ -168,7 +176,12 @@ impl<'a> WorkflowBundler<'a> { /// key. Workflow-version projection normalizes every reference and /// resolves it as an exact path inside the package root, with no /// workflow-name lookup. Returns the collected workflow's manifest key. - fn collect_workflow_entry(&mut self, workflow: &Path, resolve_from: &Path) -> Result { + fn collect_workflow_entry( + &mut self, + workflow: &Path, + resolve_from: &Path, + depth: usize, + ) -> Result { let normalize = self.workflow_version_projection || (workflow.extension().is_some() && workflow.is_relative()); let normalized = if normalize { @@ -197,7 +210,7 @@ impl<'a> WorkflowBundler<'a> { } else { WorkflowLocation::resolve(&normalized, resolve_from)? }; - self.collect_workflow_location(&location) + self.collect_workflow_location(&location, depth) } fn collect_workflow_files( @@ -207,7 +220,14 @@ impl<'a> WorkflowBundler<'a> { visited_imports: &mut HashSet, dependency_keys: &mut BTreeSet, position: GraphPosition, + depth: usize, ) -> Result<()> { + if self.workflow_version_projection { + workflow_version_collector::check_workflow_depth( + depth, + &workflow.dot_path.to_string(), + )?; + } let graph = parser::parse(&workflow.source) .with_context(|| format!("Failed to parse {}", workflow.absolute_dot_path.display()))?; let workflow_base_dir = workflow @@ -305,12 +325,13 @@ impl<'a> WorkflowBundler<'a> { visited_imports, dependency_keys, GraphPosition::Imported, + depth + 1, )?; } } for child in children { let dependency_key = - self.collect_workflow_entry(Path::new(child), workflow_base_dir)?; + self.collect_workflow_entry(Path::new(child), workflow_base_dir, depth + 1)?; dependency_keys.insert(dependency_key); } diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 57faa0d43..b3667d808 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -14,6 +14,23 @@ use crate::workflow_bundler::{ CollectedWorkflowSource, CollectedWorkflowSources, MissingPackageFile, WorkflowBundler, }; +/// Maximum active graph nesting while collecting or assembling a version. +/// Bounds native stack use independently of file-count and byte budgets. +pub const MAX_WORKFLOW_VERSION_DEPTH: usize = 64; + +pub(super) fn check_workflow_depth( + depth: usize, + path: &str, +) -> Result<(), WorkflowVersionCollectError> { + if depth > MAX_WORKFLOW_VERSION_DEPTH { + return Err(WorkflowVersionCollectError::DepthExceeded { + path: path.to_owned(), + maximum: MAX_WORKFLOW_VERSION_DEPTH, + }); + } + Ok(()) +} + /// One locally packaged workflow-version closure in dependency-first order. #[derive(Debug)] pub struct CollectedWorkflowClosure { @@ -47,6 +64,8 @@ impl CollectedWorkflowClosure { #[derive(Debug, Error)] pub enum WorkflowVersionCollectError { + #[error("workflow dependency nesting at `{path}` exceeds {maximum} levels")] + DepthExceeded { path: String, maximum: usize }, #[error("workflow `{path}` was not found")] WorkflowNotFound { path: PathBuf }, #[error("failed to collect workflow `{path}`")] @@ -99,6 +118,12 @@ pub enum WorkflowVersionCollectError { config_path: WorkflowPath, alias: WorkflowPath, }, + #[error("supplied workflow configuration `{path}` is invalid")] + InvalidSuppliedConfig { + path: WorkflowPath, + #[source] + source: Box, + }, #[error("failed to stage supplied workflow files")] Stage { #[source] @@ -182,6 +207,10 @@ pub fn collect_workflow_versions_at_location( let collected = WorkflowBundler::new(package_root, &inputs) .collect_versions(location) .map_err(|source| { + let source = match source.downcast::() { + Ok(error) => return error, + Err(source) => source, + }; let missing = source .chain() .find_map(|cause| cause.downcast_ref::()); @@ -244,6 +273,7 @@ impl VersionAssembler { if let Some(id) = self.ids.get(key) { return Ok(*id); } + check_workflow_depth(self.visiting.len() + 1, key)?; if !self.visiting.insert(key.to_owned()) { return Err(WorkflowVersionCollectError::DependencyCycle { path: workflow_path(key)?, @@ -349,6 +379,44 @@ mod tests { use super::*; + #[test] + fn version_assembly_bounds_its_own_dependency_traversal() { + // Collection and assembly visit shared dependencies in different orders. + // Assembly must bound its stack even if collection already cached nodes. + for count in [MAX_WORKFLOW_VERSION_DEPTH, MAX_WORKFLOW_VERSION_DEPTH + 1] { + let workflows = (0..count) + .map(|index| { + let child = (index + 1 < count).then(|| format!("f{}.fabro", index + 1)); + let source = child.as_ref().map_or_else( + || "digraph W {}".to_owned(), + |child| format!("digraph W {{ child [stack.child_workflow=\"{child}\"] }}"), + ); + (format!("f{index}.fabro"), CollectedWorkflowSource { + workflow: types::ManifestWorkflow { + config: None, + files: HashMap::new(), + source, + }, + dependency_keys: child.into_iter().collect(), + }) + }) + .collect(); + let result = VersionAssembler::new(CollectedWorkflowSources { + root_key: "f0.fabro".to_owned(), + workflows, + }) + .assemble(); + if count == MAX_WORKFLOW_VERSION_DEPTH { + assert_eq!(result.unwrap().versions().count(), count); + } else { + assert!(matches!( + result, + Err(WorkflowVersionCollectError::DepthExceeded { .. }) + )); + } + } + } + fn write(root: &Path, path: &str, content: &str) { let path = root.join(path); fs::create_dir_all(path.parent().expect("fixture path should have a parent")).unwrap(); diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index 54cb39521..c13067f5f 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -64,7 +64,8 @@ fn package_blocking( /// failures that reach them stop at the last path-only level and add a hint. fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { let quotes_source = match err { - WorkflowVersionCollectError::Collect { .. } => true, + WorkflowVersionCollectError::Collect { .. } + | WorkflowVersionCollectError::InvalidSuppliedConfig { .. } => true, WorkflowVersionCollectError::InvalidVersion { source, .. } => matches!( source, WorkflowVersionError::GraphParse { .. } @@ -87,6 +88,7 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { #[cfg(test)] mod tests { + use std::collections::BTreeMap; #[expect( clippy::disallowed_types, reason = "test log capture writes synchronously into memory" @@ -151,6 +153,93 @@ mod tests { format!("{error:#}") } + #[tokio::test] + async fn deeply_nested_graphs_return_errors_on_the_packaging_thread() { + for kind in ["child", "import", "mixed"] { + for count in [ + crate::MAX_WORKFLOW_VERSION_DEPTH, + crate::MAX_WORKFLOW_VERSION_DEPTH + 1, + 384, + 512, + ] { + let files: BTreeMap<_, _> = (0..count) + .map(|index| { + let attribute = if kind == "import" || (kind == "mixed" && index % 2 == 0) { + "import" + } else { + "stack.child_workflow" + }; + let graph = if index + 1 < count { + format!( + "digraph W {{ node{index} [{attribute}=\"f{}.fabro\"] }}", + index + 1 + ) + } else { + "digraph W {}".to_owned() + }; + (format!("f{index}.fabro").parse().unwrap(), graph) + }) + .collect(); + let input = ValidatedWorkflowVersionCreate::try_from( + fabro_tool::FabroWorkflowVersionCreateParams { + entrypoint: "f0.fabro".parse().unwrap(), + files, + }, + ) + .unwrap(); + let result = SuppliedWorkflowVersionPackager.package(input).await; + if count == crate::MAX_WORKFLOW_VERSION_DEPTH { + assert!(result.is_ok(), "{kind} at limit: {result:?}"); + } else { + let error = result.unwrap_err(); + assert!( + error.to_string().contains("exceeds 64 levels"), + "{kind}/{count}: {error:#}" + ); + } + } + } + } + + #[tokio::test] + async fn invalid_supplied_sibling_configs_fail_without_quoting_source() { + for config in [ + "_version = 1\nPRIVATE_CONTENT = [unterminated", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run]\ngoal = \"PRIVATE_CONTENT\"\nunknown_setting = true\n", + ] { + for child in [false, true] { + let input = if child { + source("root.fabro", &[ + ( + "root.fabro", + r#"digraph W { child [stack.child_workflow="sub/workflow.fabro"] }"#, + ), + ("sub/workflow.fabro", "digraph Child {}"), + ("sub/workflow.toml", config), + ]) + } else { + source("workflow.fabro", &[ + ("workflow.fabro", "digraph W {}"), + ("workflow.toml", config), + ]) + }; + let error = package_error(input).await; + let path = if child { + "sub/workflow.toml" + } else { + "workflow.toml" + }; + assert!( + error.contains(&format!( + "supplied workflow configuration `{path}` is invalid" + )), + "{error}" + ); + assert!(!error.contains("PRIVATE_CONTENT"), "{error}"); + } + } + } + #[tokio::test] async fn packager_returns_dependencies_before_root() { let packaged = SuppliedWorkflowVersionPackager @@ -218,6 +307,13 @@ mod tests { "workflow.toml", "_version = 1\nPRIVATE_CONTENT = [unterminated", )]), + source("workflow.fabro", &[ + ("workflow.fabro", "digraph W {}"), + ( + "workflow.toml", + "_version = 1\nPRIVATE_CONTENT = [unterminated", + ), + ]), ]; // The guard is load-bearing: the full chain does quote the source. let leaky =