diff --git a/Cargo.lock b/Cargo.lock index b3457ea3a..d1d8b4719 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2804,6 +2804,7 @@ dependencies = [ "temp-env", "tempfile", "toml 0.8.23", + "walkdir", ] [[package]] diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index bd6a3fdb4..da998f2b4 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9003,7 +9003,7 @@ components: properties: version: type: integer - description: Manifest schema version. + description: Manifest schema version. Version 2 adds workflow runtime files. example: 1 run_id: type: ["string", "null"] @@ -9288,6 +9288,11 @@ components: type: object additionalProperties: $ref: "#/components/schemas/ManifestFileEntry" + runtime_files: + description: UTF-8 files from the workflow package that Fabro writes into fresh remote sandboxes before preparation. + type: object + additionalProperties: + type: string PreflightResponse: type: object diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 611fd7cd8..ac65765bf 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -40,7 +40,9 @@ use fabro_workflow::operations::{ }; use fabro_workflow::pipeline::Validated; use fabro_workflow::run_materialization::materialize_run_with_ready_providers; -use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; +use fabro_workflow::workflow_bundle::{ + self, BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle, +}; use futures_util::stream::{self, StreamExt}; use tokio::process::Command; use tokio::time; @@ -81,9 +83,17 @@ pub(crate) fn prepare_manifest_with_environment_defaults( manifest_mcp_server_catalog: &HashMap, manifest: &types::RunManifest, ) -> Result { - if manifest.version != 1 { + if !matches!(manifest.version, 1 | 2) { bail!("unsupported manifest version {}", manifest.version); } + if manifest.version == 1 + && manifest + .workflows + .values() + .any(|workflow| !workflow.runtime_files.is_empty()) + { + bail!("workflow runtime files require manifest version 2"); + } let cwd = PathBuf::from(&manifest.cwd); let target_path = ManifestPath::from_wire(&manifest.target.path) @@ -302,6 +312,9 @@ pub fn workflow_bundle_from_manifest( ) -> Result { let mut bundled = HashMap::new(); let mut workflow_wire_keys = HashMap::new(); + let mut runtime_destinations = HashMap::new(); + let mut runtime_file_count = 0usize; + let mut runtime_file_bytes = 0usize; for (wire_key, workflow) in workflows { let path = ManifestPath::from_wire(wire_key) @@ -315,6 +328,35 @@ pub fn workflow_bundle_from_manifest( workflow_wire_keys.insert(path.clone(), wire_key.clone()); let files = workflow_files_from_manifest(&workflow.files)?; + let runtime_files = workflow_runtime_files_from_manifest(&path, &workflow.runtime_files)?; + for (runtime_path, content) in &runtime_files { + runtime_file_count = runtime_file_count + .checked_add(1) + .ok_or_else(|| anyhow!("workflow runtime file count overflow"))?; + if runtime_file_count > workflow_bundle::MAX_WORKFLOW_RUNTIME_FILES { + bail!( + "workflow manifest contains more than {} runtime files", + workflow_bundle::MAX_WORKFLOW_RUNTIME_FILES + ); + } + runtime_file_bytes = runtime_file_bytes + .checked_add(content.len()) + .ok_or_else(|| anyhow!("workflow runtime file byte count overflow"))?; + if runtime_file_bytes > workflow_bundle::MAX_WORKFLOW_RUNTIME_BYTES { + bail!( + "workflow runtime files exceed the {} byte limit", + workflow_bundle::MAX_WORKFLOW_RUNTIME_BYTES + ); + } + + if let Some(previous) = runtime_destinations.get(runtime_path) { + if previous != content { + bail!("conflicting workflow runtime file contents for path: {runtime_path}"); + } + } else { + runtime_destinations.insert(runtime_path.clone(), content.clone()); + } + } let config = workflow .config .as_ref() @@ -334,12 +376,44 @@ pub fn workflow_bundle_from_manifest( source: workflow.source.clone(), config, files, + runtime_files, }); } Ok(WorkflowBundle::new(bundled)) } +fn workflow_runtime_files_from_manifest( + workflow_path: &ManifestPath, + files: &HashMap, +) -> Result> { + let mut bundled = HashMap::new(); + let mut file_wire_keys = HashMap::new(); + + for (wire_key, content) in files { + if wire_key.split('/').any(|segment| segment == "..") + || wire_key.chars().any(char::is_control) + { + bail!("invalid workflow runtime file path: {wire_key:?}"); + } + let path = ManifestPath::from_wire(wire_key) + .ok_or_else(|| anyhow!("invalid workflow runtime file path: {wire_key:?}"))?; + if !workflow_bundle::is_workflow_runtime_file_path(workflow_path, &path) { + bail!("workflow runtime file path is outside the package directories: {wire_key:?}"); + } + if let Some(previous) = file_wire_keys.get(&path) { + bail!( + "duplicate canonical workflow runtime file path: {path} (from wire keys \ + {previous:?} and {wire_key:?})" + ); + } + file_wire_keys.insert(path.clone(), wire_key.clone()); + bundled.insert(path, content.clone()); + } + + Ok(bundled) +} + fn workflow_files_from_manifest( files: &HashMap, ) -> Result> { @@ -1419,8 +1493,9 @@ mod tests { }, version: 1, workflows: HashMap::from([("workflow.fabro".to_string(), types::ManifestWorkflow { - config: None, - files: HashMap::new(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), source: "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" .to_string(), @@ -1431,9 +1506,10 @@ mod tests { fn invalid_manifest() -> types::RunManifest { types::RunManifest { workflows: HashMap::from([("workflow.fabro".to_string(), types::ManifestWorkflow { - config: None, - files: HashMap::new(), - source: "digraph Invalid { exit [shape=Msquare] orphan exit -> orphan }" + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), + source: "digraph Invalid { exit [shape=Msquare] orphan exit -> orphan }" .to_string(), })]), ..minimal_manifest() @@ -1591,10 +1667,12 @@ digraph Demo {{ fn manifest_workflow() -> types::ManifestWorkflow { types::ManifestWorkflow { - config: None, - files: HashMap::new(), - source: "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - .to_string(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), + source: + "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" + .to_string(), } } @@ -1609,6 +1687,147 @@ digraph Demo {{ } } + #[test] + fn manifest_version_two_preserves_valid_workflow_runtime_files() { + let mut manifest = minimal_manifest(); + manifest.version = 2; + manifest + .workflows + .get_mut("workflow.fabro") + .unwrap() + .runtime_files + .insert( + "scripts/security_review.py".to_string(), + "print('review')\n".to_string(), + ); + + let bundle = workflow_bundle_from_manifest(&manifest.workflows).unwrap(); + let workflow = bundle + .workflow(&ManifestPath::from_wire("workflow.fabro").unwrap()) + .unwrap(); + + assert_eq!( + workflow + .runtime_files + .get(&ManifestPath::from_wire("scripts/security_review.py").unwrap()) + .map(String::as_str), + Some("print('review')\n") + ); + } + + #[test] + fn manifest_version_one_rejects_workflow_runtime_files() { + let mut manifest = minimal_manifest(); + manifest + .workflows + .get_mut("workflow.fabro") + .unwrap() + .runtime_files + .insert( + "scripts/review.py".to_string(), + "print('review')\n".to_string(), + ); + + let result = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ); + let Err(err) = result else { + panic!("manifest version 1 should reject workflow runtime files"); + }; + + assert_eq!( + err.to_string(), + "workflow runtime files require manifest version 2" + ); + } + + #[test] + fn workflow_runtime_files_reject_parent_traversal() { + let mut workflow = manifest_workflow(); + workflow + .runtime_files + .insert("../scripts/review.py".to_string(), "unsafe".to_string()); + + let err = workflow_bundle_from_manifest(&HashMap::from([( + "workflow.fabro".to_string(), + workflow, + )])) + .unwrap_err(); + + assert!( + err.to_string() + .contains("invalid workflow runtime file path") + ); + } + + #[test] + fn workflow_runtime_files_reject_paths_outside_package_directories() { + let mut workflow = manifest_workflow(); + workflow + .runtime_files + .insert("prompts/review.md".to_string(), "unsafe".to_string()); + + let err = workflow_bundle_from_manifest(&HashMap::from([( + "workflow.fabro".to_string(), + workflow, + )])) + .unwrap_err(); + + assert!(err.to_string().contains("outside the package directories")); + } + + #[test] + fn workflow_runtime_files_reject_conflicting_destinations() { + let mut first = manifest_workflow(); + first + .runtime_files + .insert("scripts/review.py".to_string(), "first".to_string()); + let mut second = manifest_workflow(); + second + .runtime_files + .insert("scripts/review.py".to_string(), "second".to_string()); + + let err = workflow_bundle_from_manifest(&HashMap::from([ + ("first.fabro".to_string(), first), + ("second.fabro".to_string(), second), + ])) + .unwrap_err(); + + assert!( + err.to_string() + .contains("conflicting workflow runtime file contents") + ); + } + + #[test] + fn workflow_runtime_files_enforce_count_and_size_limits() { + let mut too_many = manifest_workflow(); + for index in 0..=workflow_bundle::MAX_WORKFLOW_RUNTIME_FILES { + too_many + .runtime_files + .insert(format!("assets/file-{index}.txt"), String::new()); + } + let count_err = workflow_bundle_from_manifest(&HashMap::from([( + "workflow.fabro".to_string(), + too_many, + )])) + .unwrap_err(); + assert!(count_err.to_string().contains("runtime files")); + + let mut too_large = manifest_workflow(); + too_large.runtime_files.insert( + "assets/large.txt".to_string(), + "x".repeat(workflow_bundle::MAX_WORKFLOW_RUNTIME_BYTES + 1), + ); + let size_err = workflow_bundle_from_manifest(&HashMap::from([( + "workflow.fabro".to_string(), + too_large, + )])) + .unwrap_err(); + assert!(size_err.to_string().contains("runtime files exceed")); + } + fn git_context(origin_url: &str, branch: &str) -> types::GitContext { types::GitContext { origin_url: origin_url.to_string(), @@ -2759,14 +2978,16 @@ digraph Demo { fn workflow_with_config(source: &str) -> BundledWorkflow { BundledWorkflow { - path: ManifestPath::from_wire("workflow.fabro").expect("path should be valid"), - source: "digraph G {}".to_string(), - config: Some(ParsedWorkflowConfig { + path: ManifestPath::from_wire("workflow.fabro") + .expect("path should be valid"), + source: "digraph G {}".to_string(), + config: Some(ParsedWorkflowConfig { path: ManifestPath::from_wire("workflow.toml") .expect("config path should be valid"), source: source.to_string(), }), - files: std::collections::HashMap::new(), + files: std::collections::HashMap::new(), + runtime_files: std::collections::HashMap::new(), } } diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 0327d0cda..d5677152c 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -38,10 +38,11 @@ use tokio::fs; use tracing::info; use super::super::{ - AppState, DeleteRunOutcome, ListResponse, RunExecutionMode, VariableError, answer_from_request, - api_question_from_pending_interview, clamp_page_limit, clamp_page_offset, default_page_limit, - delete_run_internal, load_pending_interview, managed_run, parse_run_id_path, - parse_stage_id_path, reject_if_archived, submit_pending_interview_answer, workflow_event, + AppState, DefaultBodyLimit, DeleteRunOutcome, ListResponse, RunExecutionMode, VariableError, + answer_from_request, api_question_from_pending_interview, clamp_page_limit, clamp_page_offset, + default_page_limit, delete_run_internal, load_pending_interview, managed_run, + parse_run_id_path, parse_stage_id_path, reject_if_archived, submit_pending_interview_answer, + workflow_event, }; use crate::error::ApiError; use crate::principal_middleware::{ @@ -55,15 +56,23 @@ use crate::run_title_generation::{self, GenerateTitleInput, TitlePromptInput, Wo #[cfg(any(test, feature = "test-support"))] use crate::test_support as server_test_support; +const RUN_MANIFEST_BODY_LIMIT: usize = 10 * 1024 * 1024; + pub(super) fn manifest_routes() -> Router> { Router::new() .route("/preflight", post(run_preflight)) .route("/validate", post(validate_run_manifest)) + .layer(DefaultBodyLimit::max(RUN_MANIFEST_BODY_LIMIT)) } pub(super) fn routes() -> Router> { Router::new() - .route("/runs", get(list_runs).post(create_run)) + .route( + "/runs", + get(list_runs) + .post(create_run) + .layer(DefaultBodyLimit::max(RUN_MANIFEST_BODY_LIMIT)), + ) .route("/runs/resolve", get(resolve_run)) .route( "/runs/{id}", diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index 0cc8a695a..cf001b127 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -23,6 +23,7 @@ fabro-types = { path = "../../foundation/fabro-types" } fabro-workflow = { path = "../fabro-workflow" } git2.workspace = true toml.workspace = true +walkdir.workspace = true [dev-dependencies] tempfile = "3" diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index f2f1b066b..152a95c1c 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -7,7 +7,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail}; use fabro_api::types; use fabro_config::project::{self, WorkflowLocation, discover_project_config}; use fabro_config::run::{resolve_run_goal_from_layer, resolve_run_goal_from_namespace}; @@ -33,6 +33,10 @@ use fabro_workflow::git::{ use fabro_workflow::static_reference::{ AttributeScope, ReferenceKind, reference_kind_for_attribute, }; +use fabro_workflow::workflow_bundle::{ + MAX_WORKFLOW_RUNTIME_BYTES, MAX_WORKFLOW_RUNTIME_FILES, WORKFLOW_PACKAGE_DIR_NAMES, +}; +use walkdir::WalkDir; #[derive(Debug, Default)] pub struct ManifestBuildInput { @@ -136,10 +140,13 @@ pub fn build_sparse_run_overrides(input: RunOverrideInput<'_>) -> Option { - cwd: &'a Path, - inputs: HashMap, - workflows: HashMap, - visited_workflows: HashSet, + cwd: &'a Path, + canonical_cwd: PathBuf, + inputs: HashMap, + workflows: HashMap, + visited_workflows: HashSet, + runtime_file_count: usize, + runtime_file_bytes: usize, } #[derive(Clone)] @@ -198,10 +205,14 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let target_key = target_manifest_path.to_string(); let mut context = CollectContext { - cwd: &input.cwd, - inputs: workflow_settings.run.inputs.clone(), - workflows: HashMap::new(), - visited_workflows: HashSet::new(), + cwd: &input.cwd, + canonical_cwd: std::fs::canonicalize(&input.cwd) + .with_context(|| format!("Failed to resolve {}", input.cwd.display()))?, + inputs: workflow_settings.run.inputs.clone(), + workflows: HashMap::new(), + visited_workflows: HashSet::new(), + runtime_file_count: 0, + runtime_file_bytes: 0, }; collect_workflow_entry(&mut context, &input.workflow, &input.cwd)?; if let Some((_, config_path, source)) = project_config_source.as_ref() { @@ -251,6 +262,16 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let git = build_git_context(&working_directory, configured_repo_origin_url.as_deref()); let args = input.args.filter(|args| !manifest_args_is_empty(args)); + let version = if context + .workflows + .values() + .any(|workflow| !workflow.runtime_files.is_empty()) + { + 2 + } else { + 1 + }; + Ok(BuiltManifest { manifest: types::RunManifest { args, @@ -265,7 +286,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { identifier: input.workflow.display().to_string(), path: target_key, }, - version: 1, + version, workflows: context.workflows, }, target_path, @@ -306,12 +327,14 @@ fn collect_workflow_entry( None }; + let workflow_dir = location.dir.clone(); let scan = WorkflowScanInput { absolute_dot_path: location.graph, dot_path, source: source.clone(), }; let mut files = HashMap::new(); + let mut runtime_files = HashMap::new(); let mut visited_imports = HashSet::new(); if let Some(config) = config.as_ref() { let config_path = ManifestPath::from_wire(&config.path) @@ -319,16 +342,122 @@ fn collect_workflow_entry( collect_config_dockerfile(context.cwd, &config_path, &config.source, &mut files)?; } collect_workflow_files(context, &scan, &mut files, &mut visited_imports)?; + collect_workflow_runtime_files(context, &workflow_dir, &mut runtime_files)?; context.workflows.insert(dot_key, types::ManifestWorkflow { config, files, + runtime_files, source, }); Ok(()) } +fn collect_workflow_runtime_files( + context: &mut CollectContext<'_>, + workflow_dir: &Path, + runtime_files: &mut HashMap, +) -> Result<()> { + for package_dir_name in WORKFLOW_PACKAGE_DIR_NAMES { + let package_dir = workflow_dir.join(package_dir_name); + let metadata = match std::fs::symlink_metadata(&package_dir) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + Err(err) => { + return Err(err) + .with_context(|| format!("Failed to inspect {}", package_dir.display())); + } + }; + if metadata.file_type().is_symlink() { + bail!( + "workflow package directory cannot be a symlink: {}", + package_dir.display() + ); + } + if !metadata.is_dir() { + bail!( + "workflow package path must be a directory: {}", + package_dir.display() + ); + } + + for entry in WalkDir::new(&package_dir) + .follow_links(false) + .sort_by_file_name() + { + let entry = entry + .map_err(anyhow::Error::new) + .with_context(|| format!("Failed to walk {}", package_dir.display()))?; + let file_type = entry.file_type(); + if file_type.is_dir() { + continue; + } + if file_type.is_symlink() { + bail!( + "workflow package files cannot be symlinks: {}", + entry.path().display() + ); + } + if !file_type.is_file() { + bail!( + "workflow package entries must be regular files: {}", + entry.path().display() + ); + } + + let canonical_path = std::fs::canonicalize(entry.path()) + .with_context(|| format!("Failed to resolve {}", entry.path().display()))?; + if !canonical_path.starts_with(&context.canonical_cwd) { + bail!( + "workflow package file must be inside the working directory: {}", + entry.path().display() + ); + } + + let metadata = entry + .metadata() + .map_err(anyhow::Error::new) + .with_context(|| format!("Failed to inspect {}", entry.path().display()))?; + let metadata_size = usize::try_from(metadata.len()) + .context("workflow package file size does not fit in memory")?; + validate_runtime_file_limits(context, metadata_size)?; + + let content = std::fs::read_to_string(entry.path()) + .with_context(|| format!("Failed to read {}", entry.path().display()))?; + validate_runtime_file_limits(context, content.len())?; + + let path = manifest_path_from_absolute(entry.path(), context.cwd)?; + let content_bytes = content.len(); + runtime_files.insert(path.to_string(), content); + context.runtime_file_count += 1; + context.runtime_file_bytes += content_bytes; + } + } + + Ok(()) +} + +fn validate_runtime_file_limits(context: &CollectContext<'_>, file_bytes: usize) -> Result<()> { + let file_count = context + .runtime_file_count + .checked_add(1) + .ok_or_else(|| anyhow!("workflow package file count overflow"))?; + if file_count > MAX_WORKFLOW_RUNTIME_FILES { + bail!("workflow packages contain more than {MAX_WORKFLOW_RUNTIME_FILES} runtime files"); + } + + let total_bytes = context + .runtime_file_bytes + .checked_add(file_bytes) + .ok_or_else(|| anyhow!("workflow package byte count overflow"))?; + if total_bytes > MAX_WORKFLOW_RUNTIME_BYTES { + bail!("workflow package files exceed the {MAX_WORKFLOW_RUNTIME_BYTES} byte limit"); + } + + Ok(()) +} + fn collect_workflow_files( context: &mut CollectContext<'_>, workflow: &WorkflowScanInput, @@ -913,6 +1042,23 @@ mod tests { )])) } + fn write_minimal_workflow(project: &Path, name: &str) -> PathBuf { + let workflow_dir = project.join(".fabro/workflows").join(name); + std::fs::create_dir_all(&workflow_dir).unwrap(); + std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + workflow_dir + } + fn assert_manifest_bundles_output_schema_file(node_attributes: &str) { let temp = tempfile::tempdir().unwrap(); let project = temp.path(); @@ -1039,6 +1185,142 @@ mod tests { assert_manifest_bundles_output_schema_file(r#"type="command", script="echo""#); } + #[test] + fn build_manifest_bundles_conventional_workflow_runtime_files() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = write_minimal_workflow(project, "security-review"); + std::fs::create_dir_all(workflow_dir.join("scripts/lib")).unwrap(); + std::fs::create_dir_all(workflow_dir.join("references")).unwrap(); + std::fs::create_dir_all(workflow_dir.join("assets/fixtures")).unwrap(); + std::fs::create_dir_all(workflow_dir.join("other")).unwrap(); + std::fs::write( + workflow_dir.join("scripts/security_review.py"), + "print('review')\n", + ) + .unwrap(); + std::fs::write(workflow_dir.join("scripts/lib/check.py"), "CHECK = True\n").unwrap(); + std::fs::write(workflow_dir.join("references/policy.md"), "# Policy\n").unwrap(); + std::fs::write( + workflow_dir.join("assets/fixtures/input.json"), + "{\"known\":true}\n", + ) + .unwrap(); + std::fs::write(workflow_dir.join("other/ignored.txt"), "ignored\n").unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/security-review/workflow.toml"), + cwd: project.to_path_buf(), + environment_defaults: test_environment_defaults(), + ..Default::default() + }) + .unwrap(); + + assert_eq!(built.manifest.version, 2); + let workflow = &built.manifest.workflows[".fabro/workflows/security-review/workflow.fabro"]; + assert_eq!( + workflow.runtime_files, + HashMap::from([ + ( + ".fabro/workflows/security-review/scripts/security_review.py".to_string(), + "print('review')\n".to_string(), + ), + ( + ".fabro/workflows/security-review/scripts/lib/check.py".to_string(), + "CHECK = True\n".to_string(), + ), + ( + ".fabro/workflows/security-review/references/policy.md".to_string(), + "# Policy\n".to_string(), + ), + ( + ".fabro/workflows/security-review/assets/fixtures/input.json".to_string(), + "{\"known\":true}\n".to_string(), + ), + ]) + ); + assert!( + !workflow + .runtime_files + .contains_key(".fabro/workflows/security-review/other/ignored.txt") + ); + } + + #[test] + fn build_manifest_without_workflow_runtime_files_stays_at_version_one() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + write_minimal_workflow(project, "demo"); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + environment_defaults: test_environment_defaults(), + ..Default::default() + }) + .unwrap(); + + assert_eq!(built.manifest.version, 1); + assert!( + built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"] + .runtime_files + .is_empty() + ); + } + + #[test] + fn build_manifest_rejects_non_utf8_workflow_runtime_files() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = write_minimal_workflow(project, "demo"); + std::fs::create_dir_all(workflow_dir.join("assets")).unwrap(); + std::fs::write(workflow_dir.join("assets/binary.dat"), [0xff, 0xfe]).unwrap(); + + let err = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + environment_defaults: test_environment_defaults(), + ..Default::default() + }) + .unwrap_err(); + + assert!(err.chain().any(|cause| { + cause + .to_string() + .contains("stream did not contain valid UTF-8") + })); + } + + #[cfg(unix)] + #[test] + fn build_manifest_rejects_symlinks_in_workflow_package_directories() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = write_minimal_workflow(project, "demo"); + std::fs::create_dir_all(workflow_dir.join("scripts")).unwrap(); + std::fs::write(workflow_dir.join("target.py"), "print('target')\n").unwrap(); + symlink( + workflow_dir.join("target.py"), + workflow_dir.join("scripts/review.py"), + ) + .unwrap(); + + let err = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + environment_defaults: test_environment_defaults(), + ..Default::default() + }) + .unwrap_err(); + + assert!( + err.to_string() + .contains("workflow package files cannot be symlinks") + ); + } + #[test] fn build_manifest_bundles_imports_prompts_and_children() { let temp = tempfile::tempdir().unwrap(); @@ -1047,7 +1329,7 @@ mod tests { let child_dir = project.join(".fabro/workflows/child"); std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); std::fs::create_dir_all(workflow_dir.join("imports")).unwrap(); - std::fs::create_dir_all(&child_dir).unwrap(); + std::fs::create_dir_all(child_dir.join("scripts")).unwrap(); std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); std::fs::write( workflow_dir.join("workflow.toml"), @@ -1085,6 +1367,11 @@ mod tests { r"digraph Child { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", ) .unwrap(); + std::fs::write( + child_dir.join("scripts/child.py"), + "print('child package')\n", + ) + .unwrap(); let built = build_run_manifest(ManifestBuildInput { workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), @@ -1123,6 +1410,10 @@ mod tests { .workflows .contains_key(".fabro/workflows/child/workflow.fabro") ); + assert_eq!( + built.manifest.workflows[".fabro/workflows/child/workflow.fabro"].runtime_files[".fabro/workflows/child/scripts/child.py"], + "print('child package')\n" + ); } #[test] diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index 5668954c6..6a77ae9e3 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -597,10 +597,11 @@ mod tests { services.workflow_bundle = Some(Arc::new(WorkflowBundle::new(HashMap::from([( ManifestPath::from_wire("children/review.fabro").unwrap(), BundledWorkflow { - path: ManifestPath::from_wire("children/review.fabro").unwrap(), - source: child_dot_succeeds().to_string(), - config: None, - files: HashMap::new(), + path: ManifestPath::from_wire("children/review.fabro").unwrap(), + source: child_dot_succeeds().to_string(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), }, )])))); diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 8451ef1df..386bbcc5e 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -1242,8 +1242,8 @@ reasoning = false fn validate_from_bundle_resolves_nested_import_files_relative_to_imported_graph() { let validated = validate(ValidateInput { workflow: WorkflowInput::Bundled(BundledWorkflow { - path: ManifestPath::from_wire("workflow.fabro").unwrap(), - source: r#"digraph Test { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Test { graph [goal="Ship"] start [shape=Mdiamond] validate [import="./child/validate.fabro"] @@ -1251,8 +1251,8 @@ reasoning = false start -> validate -> exit }"# .to_string(), - config: None, - files: HashMap::from([ + config: None, + files: HashMap::from([ ( ManifestPath::from_wire("child/validate.fabro").unwrap(), r#"digraph Validate { @@ -1268,6 +1268,7 @@ reasoning = false "Lint {{ goal }}".to_string(), ), ]), + runtime_files: HashMap::new(), }), settings: WorkflowSettings::default(), vars: HashMap::new(), @@ -1290,8 +1291,8 @@ reasoning = false fn validate_from_bundle_resolves_minijinja_includes_in_prompt_and_goal_files() { let validated = validate(ValidateInput { workflow: WorkflowInput::Bundled(BundledWorkflow { - path: ManifestPath::from_wire("workflow.fabro").unwrap(), - source: r#"digraph Test { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Test { graph [goal="@goals/goal.md"] start [shape=Mdiamond] work [prompt="@prompts/work.md"] @@ -1299,8 +1300,8 @@ reasoning = false start -> work -> exit }"# .to_string(), - config: None, - files: HashMap::from([ + config: None, + files: HashMap::from([ ( ManifestPath::from_wire("goals/goal.md").unwrap(), r#"{% include "goal.tpl.md" %}"#.to_string(), @@ -1318,6 +1319,7 @@ reasoning = false "Bundled prompt".to_string(), ), ]), + runtime_files: HashMap::new(), }), settings: WorkflowSettings::default(), vars: HashMap::new(), diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 729140879..c739bf7a0 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -2422,8 +2422,8 @@ reasoning = false ( ManifestPath::from_wire("workflow.fabro").unwrap(), BundledWorkflow { - path: ManifestPath::from_wire("workflow.fabro").unwrap(), - source: r#"digraph Root { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Root { graph [goal="Bundle child"] start [shape=Mdiamond] manager [ @@ -2436,22 +2436,24 @@ reasoning = false start -> manager -> exit }"# .to_string(), - config: None, - files: HashMap::new(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), }, ), ( ManifestPath::from_wire("children/review.fabro").unwrap(), BundledWorkflow { - path: ManifestPath::from_wire("children/review.fabro").unwrap(), - source: r"digraph Review { + path: ManifestPath::from_wire("children/review.fabro").unwrap(), + source: r"digraph Review { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" .to_string(), - config: None, - files: HashMap::new(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::new(), }, ), ])); diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 0f4bfb440..80a3e28af 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::path::PathBuf; use std::sync::Arc; use std::time::Instant; @@ -33,6 +33,7 @@ use crate::services::{ }; use crate::stage_execution::{StageExecutionSeed, StageExecutionTracker}; use crate::steering_hub::SteeringHub; +use crate::workflow_bundle::WorkflowBundle; type BuiltSandboxEnv = (HashMap, Option>); @@ -48,6 +49,38 @@ async fn run_hooks( runner.run(hook_context, sandbox, execution_context).await } +async fn materialize_workflow_runtime_files( + sandbox: &dyn Sandbox, + workflow_bundle: &WorkflowBundle, +) -> Result<(), Error> { + let mut runtime_files = BTreeMap::new(); + for workflow in workflow_bundle.workflows().values() { + for (path, content) in &workflow.runtime_files { + let path = path.to_string(); + if let Some(previous) = runtime_files.get(&path) { + if *previous != content.as_str() { + return Err(Error::Precondition(format!( + "conflicting workflow runtime file contents for path: {path}" + ))); + } + } else { + runtime_files.insert(path, content.as_str()); + } + } + } + + for (path, content) in runtime_files { + sandbox.write_file(&path, content).await.map_err(|err| { + Error::engine_with_source( + format!("Failed to materialize workflow runtime file {path}"), + err, + ) + })?; + } + + Ok(()) +} + fn git_setup_intent(run_options: &RunOptions) -> GitSetupIntent { if let Some(source) = run_options.fork_source_ref.as_ref() { GitSetupIntent::ForkFromCheckpoint { @@ -319,7 +352,9 @@ pub async fn initialize( options.emitter.notice( RunNoticeLevel::Warn, RunNoticeCode::DirtyWorktree, - "Uncommitted changes will not be included in the remote sandbox.", + "Uncommitted project changes will not be included in the remote sandbox. Files under \ + workflow package scripts/, references/, and assets/ directories are bundled \ + separately.", ); } @@ -389,6 +424,12 @@ pub async fn initialize( .map_err(|e| Error::engine_with_source("Failed to initialize sandbox", e))?; } + if !attach_existing && !matches!(&options.sandbox, SandboxSpec::Local { .. }) { + if let Some(workflow_bundle) = options.workflow_bundle.as_deref() { + materialize_workflow_runtime_files(sandbox.as_ref(), workflow_bundle).await?; + } + } + let locations = RunLocations::for_sandbox(host_source_dir, sandbox.as_ref(), run_dir.clone()); let hook_ctx = HookContext::new( @@ -653,9 +694,12 @@ mod tests { use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_interview::AutoApproveInterviewer; use fabro_sandbox::SandboxSpec; + use fabro_sandbox::test_support::MockSandbox; use fabro_store::Database; use fabro_types::settings::run::RunModelControls; - use fabro_types::{EventBody, RunEvent, RunId, WorkflowSettings, fixtures, test_support}; + use fabro_types::{ + EventBody, ManifestPath, RunEvent, RunId, WorkflowSettings, fixtures, test_support, + }; use fabro_vault::{SecretType, Vault}; use object_store::memory::InMemory; use tokio::fs::{create_dir_all, write}; @@ -667,6 +711,7 @@ mod tests { use crate::pipeline::types::InitOptions; use crate::records::RunSpec; use crate::run_options::RunOptions; + use crate::workflow_bundle::BundledWorkflow; fn test_run_id() -> RunId { fixtures::RUN_1 @@ -679,6 +724,46 @@ mod tests { } } + #[tokio::test] + async fn materialize_workflow_runtime_files_writes_each_destination_in_path_order() { + let workflow_path = + ManifestPath::from_wire(".fabro/workflows/demo/workflow.fabro").unwrap(); + let workflow_bundle = + WorkflowBundle::new(HashMap::from([(workflow_path.clone(), BundledWorkflow { + path: workflow_path, + source: "digraph Demo {}".to_string(), + config: None, + files: HashMap::new(), + runtime_files: HashMap::from([ + ( + ManifestPath::from_wire(".fabro/workflows/demo/scripts/z-last.py").unwrap(), + "print('last')\n".to_string(), + ), + ( + ManifestPath::from_wire(".fabro/workflows/demo/assets/a-first.txt") + .unwrap(), + "first\n".to_string(), + ), + ]), + })])); + let sandbox = MockSandbox::default(); + + materialize_workflow_runtime_files(&sandbox, &workflow_bundle) + .await + .unwrap(); + + assert_eq!(*sandbox.written_files.lock().unwrap(), vec![ + ( + ".fabro/workflows/demo/assets/a-first.txt".to_string(), + "first\n".to_string(), + ), + ( + ".fabro/workflows/demo/scripts/z-last.py".to_string(), + "print('last')\n".to_string(), + ), + ]); + } + fn test_catalog() -> Arc { Arc::new(Catalog::from_builtin().expect("default catalog should build")) } diff --git a/lib/components/fabro-workflow/src/workflow_bundle.rs b/lib/components/fabro-workflow/src/workflow_bundle.rs index dddcda492..4f3969b9e 100644 --- a/lib/components/fabro-workflow/src/workflow_bundle.rs +++ b/lib/components/fabro-workflow/src/workflow_bundle.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use fabro_types::ManifestPath; @@ -7,6 +7,10 @@ use serde::{Deserialize, Serialize}; use crate::file_resolver::{BundleFileResolver, FileResolver}; +pub const WORKFLOW_PACKAGE_DIR_NAMES: [&str; 3] = ["scripts", "references", "assets"]; +pub const MAX_WORKFLOW_RUNTIME_FILES: usize = 256; +pub const MAX_WORKFLOW_RUNTIME_BYTES: usize = 5 * 1024 * 1024; + #[derive(Clone, Debug, Serialize, Deserialize)] pub struct ParsedWorkflowConfig { pub path: ManifestPath, @@ -15,10 +19,12 @@ pub struct ParsedWorkflowConfig { #[derive(Clone, Debug, Default, Serialize, Deserialize)] pub struct BundledWorkflow { - pub path: ManifestPath, - pub source: String, - pub config: Option, - pub files: HashMap, + pub path: ManifestPath, + pub source: String, + pub config: Option, + pub files: HashMap, + #[serde(default)] + pub runtime_files: HashMap, } impl BundledWorkflow { @@ -33,6 +39,43 @@ impl BundledWorkflow { } } +#[must_use] +pub fn is_workflow_runtime_file_path( + workflow_path: &ManifestPath, + candidate: &ManifestPath, +) -> bool { + if candidate + .as_path() + .components() + .any(|component| matches!(component, Component::ParentDir)) + { + return false; + } + + let package_dir = workflow_path.parent_or_dot(); + let relative = if package_dir.as_os_str().is_empty() || package_dir == Path::new(".") { + candidate.as_path() + } else { + let Ok(relative) = candidate.as_path().strip_prefix(package_dir) else { + return false; + }; + relative + }; + let mut components = relative.components(); + let Some(Component::Normal(package_dir_name)) = components.next() else { + return false; + }; + if !WORKFLOW_PACKAGE_DIR_NAMES + .iter() + .any(|allowed| package_dir_name == *allowed) + { + return false; + } + + matches!(components.next(), Some(Component::Normal(_))) + && components.all(|component| matches!(component, Component::Normal(_))) +} + #[derive(Clone, Debug, Default, Serialize, Deserialize)] pub struct WorkflowBundle { workflows: HashMap, @@ -83,3 +126,66 @@ impl RunDefinition { WorkflowBundle::new(self.workflows.clone()) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn manifest_path(value: &str) -> ManifestPath { + ManifestPath::from_wire(value).expect("test path should be valid") + } + + #[test] + fn workflow_runtime_file_path_accepts_reserved_package_directories() { + let workflow = manifest_path(".fabro/workflows/security-review/workflow.fabro"); + + for path in [ + ".fabro/workflows/security-review/scripts/review.py", + ".fabro/workflows/security-review/references/policy.md", + ".fabro/workflows/security-review/assets/fixtures/input.json", + ] { + assert!(is_workflow_runtime_file_path( + &workflow, + &manifest_path(path) + )); + } + } + + #[test] + fn workflow_runtime_file_path_rejects_paths_outside_reserved_directories() { + let workflow = manifest_path(".fabro/workflows/security-review/workflow.fabro"); + + for path in [ + "../secrets.txt", + ".fabro/workflows/other/scripts/review.py", + ".fabro/workflows/security-review/workflow.toml", + ".fabro/workflows/security-review/scripts", + ] { + assert!(!is_workflow_runtime_file_path( + &workflow, + &manifest_path(path) + )); + } + } + + #[test] + fn workflow_runtime_file_path_supports_a_workflow_at_the_workspace_root() { + assert!(is_workflow_runtime_file_path( + &manifest_path("workflow.fabro"), + &manifest_path("scripts/review.py") + )); + } + + #[test] + fn bundled_workflow_deserializes_definitions_without_runtime_files() { + let workflow: BundledWorkflow = serde_json::from_value(serde_json::json!({ + "path": "workflow.fabro", + "source": "digraph Demo {}", + "config": null, + "files": {} + })) + .expect("existing bundled workflow should deserialize"); + + assert!(workflow.runtime_files.is_empty()); + } +} diff --git a/lib/packages/fabro-api-client/src/models/manifest-workflow.ts b/lib/packages/fabro-api-client/src/models/manifest-workflow.ts index 222092b7a..d65ba1eda 100644 --- a/lib/packages/fabro-api-client/src/models/manifest-workflow.ts +++ b/lib/packages/fabro-api-client/src/models/manifest-workflow.ts @@ -24,4 +24,8 @@ export interface ManifestWorkflow { 'source': string; 'config'?: ManifestWorkflowConfig; 'files'?: { [key: string]: ManifestFileEntry; }; + /** + * UTF-8 files from the workflow package that Fabro writes into fresh remote sandboxes before preparation. + */ + 'runtime_files'?: { [key: string]: string; }; } diff --git a/lib/packages/fabro-api-client/src/models/run-manifest.ts b/lib/packages/fabro-api-client/src/models/run-manifest.ts index 7fc5ed48f..42c6a3197 100644 --- a/lib/packages/fabro-api-client/src/models/run-manifest.ts +++ b/lib/packages/fabro-api-client/src/models/run-manifest.ts @@ -37,7 +37,7 @@ import type { ManifestWorkflow } from './manifest-workflow'; */ export interface RunManifest { /** - * Manifest schema version. + * Manifest schema version. Version 2 adds workflow runtime files. */ 'version': number; /**