Bound workflow packaging depth and reject invalid supplied configs

This commit is contained in:
Scott Werner 2026-09-11 18:57:39 -06:00
parent 709d15f908
commit 410651d781
6 changed files with 211 additions and 12 deletions

View file

@ -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.

View file

@ -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;

View file

@ -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<WorkflowPath, String>,
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.

View file

@ -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<HashMap<String, types::ManifestWorkflow>> {
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<CollectedWorkflowSources> {
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<String> {
fn collect_workflow_location(
&mut self,
location: &WorkflowLocation,
depth: usize,
) -> Result<String> {
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<String> {
fn collect_workflow_entry(
&mut self,
workflow: &Path,
resolve_from: &Path,
depth: usize,
) -> Result<String> {
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<String>,
dependency_keys: &mut BTreeSet<String>,
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);
}

View file

@ -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<fabro_config::Error>,
},
#[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::<WorkflowVersionCollectError>() {
Ok(error) => return error,
Err(source) => source,
};
let missing = source
.chain()
.find_map(|cause| cause.downcast_ref::<MissingPackageFile>());
@ -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();

View file

@ -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 =