From 38c819b5c9f4bfd53e6ca752008023721ed7fe37 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:40:46 -0600 Subject: [PATCH] Contain the selected workflow file instead of pre-walking the checkout Remote acquisition walked the entire depth-1 checkout and failed on any symlink that dangled or resolved outside the root, even when the link was nowhere near the selected workflow. Submodule-style dangling links and links into the host are common in workflow repositories and made --workflow-git fail where the same commit collected fine locally. The bundler already root-checks every file it opens; the only unchecked reads were the selected TOML (or a graph selector's sibling TOML) during location resolution. Check those in collect_workflow_versions and drop the O(repo) walk. walkdir stays a dev-dependency for the dump tests. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/Cargo.toml | 2 +- .../src/commands/run/remote_workflow.rs | 52 ++++---- .../src/workflow_version_collector.rs | 115 ++++++++++++++++-- 3 files changed, 133 insertions(+), 36 deletions(-) diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 8b844e79d..cb86d79c4 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -94,7 +94,6 @@ serde_yaml = "0.9" tempfile = "3" sha2.workspace = true shlex = "1" -walkdir.workspace = true object_store.workspace = true bytes.workspace = true tokio-util.workspace = true @@ -116,6 +115,7 @@ chrono = { workspace = true } [dev-dependencies] assert_cmd = "2" +walkdir.workspace = true fabro-acp = { path = "../../components/fabro-acp", features = ["test-support"] } fabro-build-support = { path = "../../foundation/build-support" } fabro-server = { path = "../fabro-server", features = ["test-support"] } diff --git a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs index 1a77a07c0..6a7944c92 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -276,7 +276,6 @@ impl NativeGit { // it and clean up before reporting cancellation. let collection_cancel = cancel.clone(); let closure = task::spawn_blocking(move || { - check_source_symlinks(checkout.path())?; if collection_cancel.is_cancelled() { return Err(RemoteWorkflowError::Cancelled.into()); } @@ -458,23 +457,6 @@ fn resolve_name(records: &str, branch: &str, tag: &str) -> anyhow::Result anyhow::Result<()> { - let root = root.canonicalize()?; - let git_dir = root.join(".git"); - // Validate links before the collector's initial TOML lookup can read them. - for entry in walkdir::WalkDir::new(&root) - .follow_links(false) - .into_iter() - .filter_entry(|entry| entry.path() != git_dir) - { - let entry = entry?; - if entry.file_type().is_symlink() && !entry.path().canonicalize()?.starts_with(&root) { - bail!("remote workflow checkout contains a symlink outside its source root"); - } - } - Ok(()) -} - /// The task owns its child processes and temporary checkout. Dropping the /// waiter requests cooperative cleanup, not task abortion. Ctrl-C waits for /// cleanup; an in-progress blocking collection must finish first. @@ -664,6 +646,29 @@ mod tests { assert!(!path.exists()); } + #[tokio::test] + async fn remote_workflow_tolerates_symlinks_the_selected_workflow_never_reads() { + let fixture = Fixture::new(); + let source = fixture.repo.workdir().unwrap(); + // Submodule-style dangling links and links outside the checkout are + // common in workflow repositories and irrelevant to the selection. + std::os::unix::fs::symlink("../missing-submodule", source.join("vendor")).unwrap(); + std::os::unix::fs::symlink("/usr/local/lib/node_modules", source.join("tools")).unwrap(); + let sha = commit_all(&fixture.repo, "add links"); + let local = fabro_manifest::collect_workflow_versions(Path::new("review"), source).unwrap(); + let remote = fixture + .git + .collect( + Fixture::repository(), + "review".into(), + RemoteWorkflowRevision::Commit(sha), + CancellationToken::new(), + ) + .await + .unwrap(); + assert_eq!(local.root_id(), remote.root_id()); + } + #[tokio::test] async fn remote_workflow_rejects_toml_symlink_before_reading_host_content() { let fixture = Fixture::new(); @@ -690,7 +695,10 @@ mod tests { ) .await .unwrap_err(); - assert!(error.to_string().contains("outside its source root")); + assert!( + format!("{error:?}").contains("outside its source root"), + "{error:?}" + ); } #[tokio::test] @@ -964,7 +972,7 @@ mod tests { } #[test] - fn remote_workflow_matches_records_exactly_and_rejects_host_symlinks() { + fn remote_workflow_matches_records_exactly() { let sha = "1234567890123456789012345678901234567890"; assert!( resolve_name( @@ -974,10 +982,6 @@ mod tests { ) .is_err() ); - let root = tempfile::tempdir().unwrap(); - let host = tempfile::tempdir().unwrap(); - std::os::unix::fs::symlink(host.path(), root.path().join("outside")).unwrap(); - assert!(check_source_symlinks(root.path()).is_err()); } fn fake_git(script: &str) -> (tempfile::TempDir, NativeGit) { diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index a0fb815b6..81c36ab0e 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -83,17 +83,6 @@ pub fn collect_workflow_versions( checkout_root: &Path, ) -> Result { let repository_workflow = repository_workflow_path(workflow); - let location = crate::resolve_existing_workflow_location(&repository_workflow, checkout_root) - .map_err(|source| match source { - fabro_config::Error::WorkflowNotFound(_) => WorkflowVersionCollectError::WorkflowNotFound { - path: workflow.to_path_buf(), - }, - source => WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source: source.into(), - }, - })?; - let package_root = checkout_root .canonicalize() @@ -104,6 +93,21 @@ pub fn collect_workflow_versions( checkout_root.display() )), })?; + ensure_selection_contained( + &checkout_root.join(&repository_workflow), + &package_root, + workflow, + )?; + let location = crate::resolve_existing_workflow_location(&repository_workflow, checkout_root) + .map_err(|source| match source { + fabro_config::Error::WorkflowNotFound(_) => WorkflowVersionCollectError::WorkflowNotFound { + path: workflow.to_path_buf(), + }, + source => WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: source.into(), + }, + })?; let location = canonicalize_location(location, |path, source| { WorkflowVersionCollectError::Collect { path: workflow.to_path_buf(), @@ -152,6 +156,51 @@ pub(super) fn collect_workflow_versions_at_location( VersionAssembler::new(collected).assemble() } +/// Location resolution reads the selected TOML, or a selected graph's sibling +/// `workflow.toml`, before the bundler's root-checked reads begin. Refuse a +/// selection whose file resolves outside the package first, so a symlink in an +/// untrusted checkout never reads host content. Missing files are left for +/// resolution to report; symlinks elsewhere in the checkout are irrelevant +/// because every file the bundler reads is checked when it is opened. +fn ensure_selection_contained( + selected: &Path, + package_root: &Path, + workflow: &Path, +) -> Result<(), WorkflowVersionCollectError> { + let mut candidates = vec![selected.to_path_buf()]; + if selected + .extension() + .is_none_or(|extension| extension != "toml") + { + candidates.push(selected.with_file_name("workflow.toml")); + } + for path in candidates { + if path.symlink_metadata().is_err() { + continue; + } + let canonical = + path.canonicalize() + .map_err(|source| WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::Error::new(source).context(format!( + "failed to canonicalize workflow file {}", + path.display() + )), + })?; + if !canonical.starts_with(package_root) { + return Err(WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::anyhow!( + "workflow file `{}` resolves outside its source root `{}`", + path.display(), + package_root.display() + ), + }); + } + } + Ok(()) +} + fn repository_workflow_path(workflow: &Path) -> PathBuf { if workflow.is_relative() && workflow.extension().is_none() { Path::new(".fabro/workflows") @@ -352,6 +401,50 @@ dockerfile = { path = "Dockerfile" } ); } + #[cfg(unix)] + #[test] + fn rejects_selected_files_that_resolve_outside_the_package_root() { + let host = tempfile::tempdir().unwrap(); + write(host.path(), "workflow.toml", "_version = 1\n"); + for selector in ["root", ".fabro/workflows/root/workflow.fabro"] { + let temp = tempfile::tempdir().unwrap(); + write_complete_fixture(temp.path()); + let toml = temp.path().join(".fabro/workflows/root/workflow.toml"); + fs::remove_file(&toml).unwrap(); + std::os::unix::fs::symlink(host.path().join("workflow.toml"), &toml).unwrap(); + let error = collect_workflow_versions(Path::new(selector), temp.path()).unwrap_err(); + assert!( + format!("{error:?}").contains("outside its source root"), + "{selector}: {error:?}" + ); + // A dangling selection is refused rather than reported as missing. + fs::remove_file(&toml).unwrap(); + std::os::unix::fs::symlink(host.path().join("missing.toml"), &toml).unwrap(); + let error = collect_workflow_versions(Path::new(selector), temp.path()).unwrap_err(); + assert!( + format!("{error:?}").contains("failed to canonicalize workflow file"), + "{selector}: {error:?}" + ); + } + } + + #[cfg(unix)] + #[test] + fn ignores_symlinks_the_selected_workflow_never_reads() { + let host = tempfile::tempdir().unwrap(); + let temp = tempfile::tempdir().unwrap(); + write_complete_fixture(temp.path()); + std::os::unix::fs::symlink(host.path(), temp.path().join("tools")).unwrap(); + std::os::unix::fs::symlink("../missing", temp.path().join("vendor")).unwrap(); + std::os::unix::fs::symlink( + "/nonexistent/module", + temp.path().join(".fabro/workflows/root/unrelated"), + ) + .unwrap(); + let closure = collect_workflow_versions(Path::new("root"), temp.path()).unwrap(); + assert_eq!(closure.versions().count(), 2); + } + #[test] fn packages_named_workflow_without_project_config() { let temp = tempfile::tempdir().unwrap();