scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-11 22:53:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Contain the selected workflow file instead of pre-walking the checkout

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-09-11 09:40:46 -06:00
parent 67434d1c3a
commit 38c819b5c9
3 changed files with 133 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/apps/fabro-cli/Cargo.toml
View file

@ -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"] }

52
lib/apps/fabro-cli/src/commands/run/remote_workflow.rs
View file

@ -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<String
}
}
fn check_source_symlinks(root: &Path) -> 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) {

115
lib/components/fabro-manifest/src/workflow_version_collector.rs
View file

@ -83,17 +83,6 @@ pub fn collect_workflow_versions(
checkout_root: &Path,
) -> Result<CollectedWorkflowClosure, WorkflowVersionCollectError> {
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();