diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index d3f21e78a..9d5bb5388 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -161,13 +161,13 @@ pub(crate) async fn execute( artifact_sink, run_control: Some(run_control), github_app, - github_permissions: run_spec + github_integration: run_spec .settings .run .integrations .github - .resolve_permissions() - .context("failed to resolve github permissions")?, + .resolve_integration() + .context("failed to resolve github integration")?, vault, catalog, on_node: None, diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 8c7d5514d..3c6935f51 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -4081,15 +4081,15 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { return; } }; - let github_permissions = match persisted + let github_integration = match persisted .run_spec() .settings .run .integrations .github - .resolve_permissions() + .resolve_integration() { - Ok(permissions) => permissions, + Ok(integration) => integration, Err(err) => { tracing::error!( run_id = %run_id, @@ -4131,7 +4131,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { artifact_sink: Some(ArtifactSink::Store(state.artifact_store.clone())), run_control: None, github_app, - github_permissions, + github_integration, vault: Arc::new(AsyncRwLock::new(vault.into_vault())), catalog: state.catalog(), on_node: None, diff --git a/lib/components/fabro-github/src/access.rs b/lib/components/fabro-github/src/access.rs index 6ca1eb2aa..b71fd0c64 100644 --- a/lib/components/fabro-github/src/access.rs +++ b/lib/components/fabro-github/src/access.rs @@ -140,6 +140,19 @@ impl GitHubRepositoryAccess { !self.additional.is_empty() } + /// [`Self::resolve_shared_installation`] against the production GitHub + /// API with a fresh HTTP client. + pub async fn resolve_shared_installation_via_api( + &self, + creds: &GitHubAppCredentials, + ) -> anyhow::Result { + let client = fabro_http::http_client() + .map_err(anyhow::Error::new) + .context("building HTTP client for installation resolution")?; + self.resolve_shared_installation(creds, &client, &crate::github_api_base_url()) + .await + } + /// Resolve every target's App installation and require one shared /// installation ID, so a repository the App cannot see — or one that /// resolves to a different installation — is named before any token is diff --git a/lib/components/fabro-workflow/src/git_bridge.rs b/lib/components/fabro-workflow/src/git_bridge.rs new file mode 100644 index 000000000..df9c7e300 --- /dev/null +++ b/lib/components/fabro-workflow/src/git_bridge.rs @@ -0,0 +1,448 @@ +//! Secret-free Git bridging environment for additional-repository access. +//! +//! When a run declares additional GitHub repositories, every resolved +//! command/tool/ACP environment receives `GIT_CONFIG_COUNT` / +//! `GIT_CONFIG_KEY_n` / `GIT_CONFIG_VALUE_n` entries that make plain Git +//! commands work against the declared set through the managed +//! `GITHUB_TOKEN`: +//! +//! - a credential helper for `https://github.com` that reads `$GITHUB_TOKEN` +//! from the invoking Git process's environment at invocation time, so token +//! refresh flows through per-stage environment resolution with no bridging +//! update; +//! - per-repository `url..insteadOf` rewrites for the +//! `git@github.com:owner/repo[.git]` and +//! `ssh://git@github.com/owner/repo[.git]` SSH spellings of each effective +//! repository. +//! +//! None of the values contain a secret; the token lives only in +//! `GITHUB_TOKEN`. +//! +//! The credential helper is host-scoped to `https://github.com`, not +//! path-scoped. This is safe because the token is scoped server-side to the +//! declared repository set and is only ever offered to github.com. It does +//! change one failure mode for *undeclared* repositories: public HTTPS +//! clones are unaffected (Git tries unauthenticated first), while private +//! undeclared HTTPS repositories fail with a GitHub authorization error +//! instead of a missing-credential error. Both fail; only the diagnostic +//! differs. +//! +//! `insteadOf` matches by string prefix, not exactly: a rule for +//! `owner/repo` also matches `owner/repo-other`. An undeclared repository +//! that shares a declared prefix is therefore rewritten to HTTPS; the scoped +//! token is invalid for it at GitHub, so authority is unchanged, but its Git +//! transport changes from SSH to HTTPS. + +use std::collections::HashMap; + +use fabro_types::GitHubRepositorySlug; + +use crate::error::Error; + +/// Section base for the effective repositories' HTTPS routes. +const GITHUB_HTTPS_BASE: &str = "https://github.com/"; + +const CREDENTIAL_HELPER_KEY: &str = "credential.https://github.com.helper"; +/// Reads the invoking process's `$GITHUB_TOKEN` at invocation time; contains +/// no secret itself. Non-`get` operations (`store`, `erase`) are ignored. +const CREDENTIAL_HELPER: &str = r#"!f() { if [ "$1" = get ]; then echo username=x-access-token; echo "password=$GITHUB_TOKEN"; fi; }; f"#; + +/// Merge the bridging entries into `env` for the effective repository set +/// (primary first). Appends after any valid user-provided `GIT_CONFIG_COUNT` +/// overlay without overwriting it, and fails with a configuration error when +/// the user overlay is malformed rather than silently replacing it. +pub(crate) fn merge_git_bridge_env( + env: &mut HashMap, + targets: &[&GitHubRepositorySlug], +) -> Result<(), Error> { + let start = user_git_config_count(env)?; + for (offset, (key, value)) in bridge_entries(targets, GITHUB_HTTPS_BASE) + .into_iter() + .enumerate() + { + let index = start + offset; + env.insert(format!("GIT_CONFIG_KEY_{index}"), key); + env.insert(format!("GIT_CONFIG_VALUE_{index}"), value); + } + let total = start + bridge_entry_count(targets); + env.insert("GIT_CONFIG_COUNT".to_string(), total.to_string()); + // Fail instead of hanging when access is missing or invalid; a user who + // explicitly configured prompting keeps their value. + env.entry("GIT_TERMINAL_PROMPT".to_string()) + .or_insert_with(|| "0".to_string()); + Ok(()) +} + +fn bridge_entry_count(targets: &[&GitHubRepositorySlug]) -> usize { + 1 + targets.len() * 2 +} + +/// The bridge's Git config entries in order: the credential helper, then two +/// SSH-to-HTTPS rewrites per repository. `https_base` is +/// [`GITHUB_HTTPS_BASE`] in production; contract tests substitute a local +/// `file://` root to prove real Git applies the generated entries without +/// touching the network. +fn bridge_entries(targets: &[&GitHubRepositorySlug], https_base: &str) -> Vec<(String, String)> { + let mut entries = Vec::with_capacity(bridge_entry_count(targets)); + entries.push(( + CREDENTIAL_HELPER_KEY.to_string(), + CREDENTIAL_HELPER.to_string(), + )); + for slug in targets { + let owner = slug.owner(); + let repo = slug.repo(); + let https = format!("{https_base}{owner}/{repo}"); + // One prefix rule per SSH spelling covers both the bare and `.git` + // suffixed forms. + entries.push(( + format!("url.{https}.insteadOf"), + format!("git@github.com:{owner}/{repo}"), + )); + entries.push(( + format!("url.{https}.insteadOf"), + format!("ssh://git@github.com/{owner}/{repo}"), + )); + } + entries +} + +/// Validate and measure a user-provided `GIT_CONFIG_COUNT` overlay so the +/// bridge appends after it. Orphaned `GIT_CONFIG_KEY_n` entries without a +/// count are inert to Git and are treated as absent. +fn user_git_config_count(env: &HashMap) -> Result { + let Some(raw) = env.get("GIT_CONFIG_COUNT") else { + return Ok(0); + }; + let count: usize = raw.trim().parse().map_err(|_| { + Error::Precondition(format!( + "environment variable GIT_CONFIG_COUNT must be a non-negative integer to combine \ + with Fabro's Git bridging entries, got `{raw}`" + )) + })?; + for index in 0..count { + let key = format!("GIT_CONFIG_KEY_{index}"); + let value = format!("GIT_CONFIG_VALUE_{index}"); + if !env.contains_key(&key) || !env.contains_key(&value) { + return Err(Error::Precondition(format!( + "GIT_CONFIG_COUNT is {count} but {key} or {value} is missing; fix the indexed \ + Git config overlay so Fabro can append its bridging entries after it" + ))); + } + } + Ok(count) +} + +#[cfg(test)] +#[expect( + clippy::disallowed_methods, + clippy::disallowed_types, + reason = "contract tests drive the installed git binary synchronously in non-async tests" +)] +mod tests { + use std::path::Path; + use std::process::Command; + + use super::*; + + fn slug(value: &str) -> GitHubRepositorySlug { + value.parse().expect("test slug should parse") + } + + fn bridged_env( + base_env: HashMap, + targets: &[&GitHubRepositorySlug], + ) -> HashMap { + let mut env = base_env; + merge_git_bridge_env(&mut env, targets).expect("bridge entries should merge"); + env + } + + /// Run `git` with ONLY the bridge-relevant environment: the inherited + /// user/system/global Git config is disabled so assertions observe just + /// the generated entries. + fn git(args: &[&str], env: &HashMap, cwd: &Path) -> std::process::Output { + let mut command = Command::new("git"); + command + .args(args) + .current_dir(cwd) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_ASKPASS", "true"); + for (key, value) in env { + command.env(key, value); + } + command.output().expect("git should run") + } + + /// Create a bare fixture answering both the bare and `.git`-suffixed + /// routes, the way GitHub serves both HTTPS spellings. + fn init_bare_fixture(root: &Path, owner_repo: &str) -> String { + let fixture = root.join(format!("{owner_repo}.git")); + std::fs::create_dir_all(&fixture).unwrap(); + let init = Command::new("git") + .args(["init", "--bare", "--initial-branch=main"]) + .arg(&fixture) + .output() + .expect("git init should run"); + assert!(init.status.success(), "{init:?}"); + #[cfg(unix)] + std::os::unix::fs::symlink(&fixture, root.join(owner_repo)).unwrap(); + format!("file://{}/", root.display()) + } + + #[test] + fn no_targets_means_no_bridge_call_and_empty_env_stays_empty() { + // The caller only bridges when the additional set is non-empty; the + // pure entry builder is still total for the primary-only case. + assert_eq!(bridge_entry_count(&[]), 1); + let env: HashMap = HashMap::new(); + assert!(!env.contains_key("GIT_CONFIG_COUNT")); + } + + #[test] + fn merges_helper_rewrites_count_and_terminal_prompt() { + let keystone = slug("fabro-sh/keystone"); + let fabro = slug("fabro-sh/fabro"); + let env = bridged_env(HashMap::new(), &[&fabro, &keystone]); + + assert_eq!(env.get("GIT_CONFIG_COUNT").map(String::as_str), Some("5")); + assert_eq!( + env.get("GIT_CONFIG_KEY_0").map(String::as_str), + Some("credential.https://github.com.helper") + ); + assert_eq!( + env.get("GIT_CONFIG_KEY_1").map(String::as_str), + Some("url.https://github.com/fabro-sh/fabro.insteadOf") + ); + assert_eq!( + env.get("GIT_CONFIG_VALUE_1").map(String::as_str), + Some("git@github.com:fabro-sh/fabro") + ); + assert_eq!( + env.get("GIT_CONFIG_VALUE_2").map(String::as_str), + Some("ssh://git@github.com/fabro-sh/fabro") + ); + assert_eq!( + env.get("GIT_TERMINAL_PROMPT").map(String::as_str), + Some("0") + ); + + // No secrets anywhere in the generated values. + for (key, value) in &env { + assert!(!value.contains("ghs_"), "{key}={value}"); + } + } + + #[test] + fn respects_an_explicit_user_terminal_prompt() { + let keystone = slug("fabro-sh/keystone"); + let env = bridged_env( + HashMap::from([("GIT_TERMINAL_PROMPT".to_string(), "1".to_string())]), + &[&keystone], + ); + assert_eq!( + env.get("GIT_TERMINAL_PROMPT").map(String::as_str), + Some("1") + ); + } + + #[test] + fn appends_after_a_valid_user_git_config_overlay() { + let keystone = slug("fabro-sh/keystone"); + let env = bridged_env( + HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), "1".to_string()), + ("GIT_CONFIG_KEY_0".to_string(), "user.name".to_string()), + ("GIT_CONFIG_VALUE_0".to_string(), "Overlay User".to_string()), + ]), + &[&keystone], + ); + + assert_eq!(env.get("GIT_CONFIG_COUNT").map(String::as_str), Some("4")); + assert_eq!( + env.get("GIT_CONFIG_KEY_0").map(String::as_str), + Some("user.name"), + "user entry must survive at its original index" + ); + assert_eq!( + env.get("GIT_CONFIG_KEY_1").map(String::as_str), + Some("credential.https://github.com.helper") + ); + + // Real Git sees both the user's entry and the appended bridge entry. + let dir = tempfile::tempdir().unwrap(); + let output = git(&["config", "--list"], &env, dir.path()); + assert!(output.status.success(), "{output:?}"); + let listed = String::from_utf8_lossy(&output.stdout); + assert!(listed.contains("user.name=Overlay User"), "{listed}"); + assert!( + listed.contains("credential.https://github.com.helper"), + "{listed}" + ); + } + + #[test] + fn rejects_a_malformed_user_git_config_overlay() { + let keystone = slug("fabro-sh/keystone"); + + let mut non_numeric = HashMap::from([("GIT_CONFIG_COUNT".to_string(), "two".to_string())]); + let err = merge_git_bridge_env(&mut non_numeric, &[&keystone]).unwrap_err(); + assert!(err.to_string().contains("GIT_CONFIG_COUNT"), "{err}"); + + let mut missing_index = HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), "2".to_string()), + ("GIT_CONFIG_KEY_0".to_string(), "user.name".to_string()), + ("GIT_CONFIG_VALUE_0".to_string(), "Overlay".to_string()), + ]); + let err = merge_git_bridge_env(&mut missing_index, &[&keystone]).unwrap_err(); + assert!(err.to_string().contains("GIT_CONFIG_KEY_1"), "{err}"); + } + + /// With the bridge active, `git credential fill` for github.com resolves + /// through the generated helper and reads `$GITHUB_TOKEN` from the + /// invoking process environment at invocation time. + #[test] + fn credential_helper_reads_github_token_at_invocation_time() { + use std::io::Write as _; + + let keystone = slug("fabro-sh/keystone"); + let mut env = bridged_env(HashMap::new(), &[&keystone]); + env.insert("GITHUB_TOKEN".to_string(), "test-token-value".to_string()); + + let dir = tempfile::tempdir().unwrap(); + let mut command = Command::new("git"); + command + .args(["credential", "fill"]) + .current_dir(dir.path()) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + for (key, value) in &env { + command.env(key, value); + } + let mut child = command.spawn().expect("git credential fill should spawn"); + child + .stdin + .as_mut() + .unwrap() + .write_all(b"protocol=https\nhost=github.com\npath=fabro-sh/keystone\n\n") + .unwrap(); + let output = child.wait_with_output().unwrap(); + + assert!(output.status.success(), "{output:?}"); + let filled = String::from_utf8_lossy(&output.stdout); + assert!(filled.contains("username=x-access-token"), "{filled}"); + assert!(filled.contains("password=test-token-value"), "{filled}"); + } + + /// Real Git applies the generated `insteadOf` rewrites: the exact SSH + /// spellings of a declared repository resolve to their HTTPS-analog + /// route (a local `file://` fixture here, so no network is involved), + /// while `GIT_SSH_COMMAND=false` proves SSH is never attempted. + #[test] + fn declared_ssh_urls_rewrite_to_the_https_route() { + let root = tempfile::tempdir().unwrap(); + let base = init_bare_fixture(root.path(), "fabro-sh/keystone"); + let keystone = slug("fabro-sh/keystone"); + + let mut env: HashMap = HashMap::new(); + for (offset, (key, value)) in bridge_entries(&[&keystone], &base).into_iter().enumerate() { + env.insert(format!("GIT_CONFIG_KEY_{offset}"), key); + env.insert(format!("GIT_CONFIG_VALUE_{offset}"), value); + } + env.insert("GIT_CONFIG_COUNT".to_string(), "3".to_string()); + env.insert("GIT_SSH_COMMAND".to_string(), "false".to_string()); + + for url in [ + "ssh://git@github.com/fabro-sh/keystone.git", + "ssh://git@github.com/fabro-sh/keystone", + "git@github.com:fabro-sh/keystone.git", + "git@github.com:fabro-sh/keystone", + ] { + let output = git(&["ls-remote", url], &env, root.path()); + assert!( + output.status.success(), + "{url} should rewrite to the fixture route: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + } + + /// An undeclared SSH URL that shares no declared prefix is not + /// rewritten: Git still routes it to SSH, where the scripted + /// `GIT_SSH_COMMAND=false` fails immediately without network access. + #[test] + fn undeclared_ssh_urls_are_not_rewritten() { + let root = tempfile::tempdir().unwrap(); + let base = init_bare_fixture(root.path(), "fabro-sh/keystone"); + let keystone = slug("fabro-sh/keystone"); + + let mut env: HashMap = HashMap::new(); + for (offset, (key, value)) in bridge_entries(&[&keystone], &base).into_iter().enumerate() { + env.insert(format!("GIT_CONFIG_KEY_{offset}"), key); + env.insert(format!("GIT_CONFIG_VALUE_{offset}"), value); + } + env.insert("GIT_CONFIG_COUNT".to_string(), "3".to_string()); + env.insert("GIT_SSH_COMMAND".to_string(), "false".to_string()); + env.insert("GITHUB_TOKEN".to_string(), "test-token-value".to_string()); + + let output = git( + &["ls-remote", "git@github.com:fabro-sh/undeclared"], + &env, + root.path(), + ); + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8_lossy(&output.stderr); + // Not rewritten: the failure never mentions the local HTTPS-analog + // fixture route, so Git still chose the SSH transport. + assert!( + !stderr.contains(&root.path().display().to_string()), + "undeclared URL must not be rewritten to the fixture route: {stderr}" + ); + assert!(!stderr.contains("test-token-value"), "{stderr}"); + } + + /// Prefix collision: with `fabro-sh/keystone` declared, both SSH + /// spellings of `fabro-sh/keystone-other` are rewritten to the HTTPS + /// route (prefix match), where access fails — at GitHub this is an + /// authorization error for the scoped token — and no token leaks into + /// the output. + #[test] + fn prefix_colliding_undeclared_repositories_rewrite_and_fail_without_token_leak() { + let root = tempfile::tempdir().unwrap(); + let base = init_bare_fixture(root.path(), "fabro-sh/keystone"); + let keystone = slug("fabro-sh/keystone"); + + let mut env: HashMap = HashMap::new(); + for (offset, (key, value)) in bridge_entries(&[&keystone], &base).into_iter().enumerate() { + env.insert(format!("GIT_CONFIG_KEY_{offset}"), key); + env.insert(format!("GIT_CONFIG_VALUE_{offset}"), value); + } + env.insert("GIT_CONFIG_COUNT".to_string(), "3".to_string()); + env.insert("GIT_SSH_COMMAND".to_string(), "false".to_string()); + env.insert("GITHUB_TOKEN".to_string(), "test-token-value".to_string()); + + for url in [ + "git@github.com:fabro-sh/keystone-other", + "ssh://git@github.com/fabro-sh/keystone-other.git", + ] { + let output = git(&["ls-remote", url], &env, root.path()); + assert!(!output.status.success(), "{url}: {output:?}"); + let stderr = String::from_utf8_lossy(&output.stderr); + // The failure names the (missing) HTTPS-analog fixture route, + // proving the prefix rule rewrote the URL away from SSH. + assert!( + stderr.contains("keystone-other"), + "{url} must be rewritten away from SSH, got: {stderr}" + ); + assert!( + stderr.contains(&root.path().display().to_string()), + "{url} must land on the rewritten route, got: {stderr}" + ); + assert!(!stderr.contains("test-token-value"), "{stderr}"); + } + } +} diff --git a/lib/components/fabro-workflow/src/handler/llm/acp.rs b/lib/components/fabro-workflow/src/handler/llm/acp.rs index cfd498beb..9e9f30c39 100644 --- a/lib/components/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/components/fabro-workflow/src/handler/llm/acp.rs @@ -473,8 +473,9 @@ impl AgentAcpBackend { emitter.notice( RunNoticeLevel::Info, RunNoticeCode::GithubTokenRefreshLimited, - "ACP agent stages receive workflow env at process launch; stages running beyond \ - token expiry may need to be retried.", + "ACP agent stages receive workflow env at process launch; GITHUB_TOKEN access to \ + every declared repository expires together, so stages running beyond token \ + expiry may need to be retried.", ); } provider diff --git a/lib/components/fabro-workflow/src/lib.rs b/lib/components/fabro-workflow/src/lib.rs index 3178542d7..29cd629ec 100644 --- a/lib/components/fabro-workflow/src/lib.rs +++ b/lib/components/fabro-workflow/src/lib.rs @@ -293,6 +293,7 @@ pub mod error; pub mod event; pub mod file_resolver; pub mod git; +pub(crate) mod git_bridge; pub(crate) mod graph; pub mod handler; mod hook_context; diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 3ba470151..3acf6255b 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::path::Path; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; @@ -17,7 +17,7 @@ use fabro_sandbox::{DockerSandboxOptions, SandboxSpec}; use fabro_static::EnvVars; use fabro_types::settings::run::{ ApprovalMode, McpServerSettings as ResolvedMcpServerSettings, PullRequestSettings, - ResolvedMcpEntry, RunMode, RunNamespace as ResolvedRunSettings, + ResolvedGithubIntegration, ResolvedMcpEntry, RunMode, RunNamespace as ResolvedRunSettings, RunPrepareSettings as ResolvedRunPrepareSettings, }; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; @@ -104,9 +104,10 @@ pub struct StartServices { pub artifact_sink: Option, pub run_control: Option>, pub github_app: Option, - /// Server-resolved GitHub integration permissions to inject into the - /// sandbox env. Empty when github integration has no permissions. - pub github_permissions: HashMap, + /// The resolved GitHub integration request (interpolated permissions + /// plus declared additional repositories) to inject into the sandbox + /// env. Empty when the github integration requests no token. + pub github_integration: ResolvedGithubIntegration, pub vault: Arc>, pub catalog: Arc, pub on_node: crate::OnNodeCallback, @@ -452,11 +453,13 @@ impl RunSession { .environment .resolve_env(secret_lookup) .map_err(|err| Error::engine_with_source("failed to resolve run environment", err))?; - let github_permissions: Option> = - (!services.github_permissions.is_empty()).then(|| services.github_permissions.clone()); + let github_integration = services + .github_integration + .is_token_requested() + .then(|| services.github_integration.clone()); let sandbox_env = SandboxEnvSpec { toml_env, - github_permissions, + github_integration, origin_url: record.repo_origin_url().map(str::to_string), }; @@ -1725,7 +1728,7 @@ reasoning = false artifact_sink: None, run_control: None, github_app: None, - github_permissions: HashMap::new(), + github_integration: ResolvedGithubIntegration::default(), vault: Arc::new(AsyncRwLock::new(start_vault(&[]))), catalog: test_catalog(), on_node: None, diff --git a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs index 8056192bd..af83d7cfe 100644 --- a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs @@ -287,7 +287,7 @@ async fn execute_test_run_with_options( hooks: HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -349,7 +349,7 @@ async fn execute_runs_start_to_exit_and_returns_final_context() { hooks: HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -488,7 +488,7 @@ async fn resumed_in_flight_node_starts_a_new_stage_execution() { hooks: HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -599,7 +599,7 @@ async fn run_with_lifecycle( hooks: HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index a91929c7b..cde82cacf 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -24,6 +24,7 @@ use super::types::{InitOptions, Initialized, LlmSpec, Persisted, SandboxEnvSpec} use crate::error::Error; use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::git::GitAuthor; +use crate::git_bridge; use crate::handler::llm::{AgentAcpBackend, AgentApiBackend, BackendRouter, routing}; use crate::handler::{HandlerRegistry, default_registry}; #[cfg(test)] @@ -37,10 +38,14 @@ use crate::services::{ use crate::stage_execution::{StageExecutionSeed, StageExecutionTracker}; use crate::steering_hub::SteeringHub; -type BuiltSandboxEnv = ( - HashMap, - Option>, -); +struct BuiltSandboxEnv { + env: HashMap, + github_token: Option>, + /// The validated effective repository set behind `github_token`. + /// Present only in App mode or when additional repositories are + /// declared; drives the eager access validation at initialization. + github_access: Option, +} async fn run_hooks( hook_runner: Option<&HookRunner>, @@ -91,41 +96,120 @@ fn build_sandbox_env( spec: &SandboxEnvSpec, github_app: Option<&fabro_github::GitHubCredentials>, ) -> Result { - let env = spec.toml_env.clone(); + let mut env = spec.toml_env.clone(); - let Some(permissions) = spec.github_permissions.as_ref().filter(|p| !p.is_empty()) else { - return Ok((env, None)); + let no_token = |env| BuiltSandboxEnv { + env, + github_token: None, + github_access: None, }; + let Some(integration) = spec + .github_integration + .as_ref() + .filter(|integration| integration.is_token_requested()) + else { + return Ok(no_token(env)); + }; + let declares_additional = integration.has_additional_repositories(); let Some(creds) = github_app else { - return Ok((env, None)); + if declares_additional { + // Legacy permissions-only configuration stays best-effort, but a + // declared additional set is an explicit access requirement. + return Err(Error::Precondition( + "run.integrations.github.additional_repositories requires GitHub credentials, \ + but none are configured" + .to_string(), + )); + } + return Ok(no_token(env)); }; - let source = match creds { + // Validate the effective repository set whenever it matters: App mode + // scopes the mint to it, and any declared additional set must hold its + // invariants regardless of credential kind. Legacy PAT/static + // permissions-only runs skip it to preserve their origin-agnostic + // behavior. + let github_access = + if declares_additional || matches!(creds, fabro_github::GitHubCredentials::App(_)) { + fabro_github::GitHubRepositoryAccess::new( + spec.origin_url.as_deref(), + &integration.additional_repositories, + integration.permissions.clone(), + ) + .map_err(|err| { + Error::engine_with_anyhow("Failed to validate GitHub repository access", err) + })? + } else { + None + }; + + let github_token = match creds { fabro_github::GitHubCredentials::Pat(token) => { Some(InstallationTokenSource::pat(token.clone())) } fabro_github::GitHubCredentials::Installation(token) => { Some(InstallationTokenSource::installation(token.clone())) } - fabro_github::GitHubCredentials::App(_) => { - let Some(origin_url) = spec.origin_url.as_deref() else { - return Ok((env, None)); - }; - let https_url = fabro_github::ssh_url_to_https(origin_url); - let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url) - .map_err(|err| Error::engine_with_anyhow("Failed to parse GitHub origin", err))?; - let permissions = serde_json::to_value(permissions).map_err(|err| { - Error::engine_with_source("Failed to serialize GitHub permissions", err) - })?; - Some( - InstallationTokenSource::for_repository(creds, owner, repo, permissions).map_err( - |err| Error::engine_with_anyhow("Failed to build GitHub token source", err), - )?, - ) - } + fabro_github::GitHubCredentials::App(_) => match github_access.as_ref() { + Some(access) => Some(InstallationTokenSource::for_access(creds, access).map_err( + |err| Error::engine_with_anyhow("Failed to build GitHub token source", err), + )?), + // No origin URL and nothing declared: keep the legacy + // best-effort skip. + None => None, + }, }; - Ok((env, source)) + if declares_additional { + let access = github_access + .as_ref() + .expect("access is always constructed when additional repositories are declared"); + git_bridge::merge_git_bridge_env(&mut env, &access.targets())?; + } + + Ok(BuiltSandboxEnv { + env, + github_token, + github_access, + }) +} + +/// When additional repositories are declared, prove the whole effective set +/// is reachable before the first workflow stage: resolve every repository's +/// App installation (naming any repository the App cannot see), then resolve +/// the token once eagerly. Legacy permissions-only runs skip this and keep +/// their best-effort behavior. +async fn validate_declared_repository_access( + built: &BuiltSandboxEnv, + github_app: Option<&fabro_github::GitHubCredentials>, +) -> Result<(), Error> { + let Some(access) = built + .github_access + .as_ref() + .filter(|access| access.has_additional_repositories()) + else { + return Ok(()); + }; + if let Some(fabro_github::GitHubCredentials::App(app)) = github_app { + access + .resolve_shared_installation_via_api(app) + .await + .map_err(|err| { + Error::engine_with_anyhow( + "Declared additional GitHub repository is not accessible", + err, + ) + })?; + } + if let Some(source) = built.github_token.as_ref() { + source.resolve().await.map_err(|err| { + Error::engine_with_anyhow( + "Failed to resolve GitHub access for the declared repository set", + err, + ) + })?; + } + Ok(()) } async fn build_registry( @@ -443,10 +527,17 @@ pub async fn initialize( }); } - let (base_env, github_token) = build_sandbox_env( + let built_env = build_sandbox_env( &options.sandbox_env, options.run_options.github_app.as_ref(), )?; + validate_declared_repository_access(&built_env, options.run_options.github_app.as_ref()) + .await?; + let BuiltSandboxEnv { + env: base_env, + github_token, + github_access: _, + } = built_env; let tool_env_provider = Arc::new(WorkflowToolEnvProvider { base_env: base_env.clone(), github_token: github_token.clone(), @@ -818,7 +909,7 @@ mod tests { hooks: fabro_hooks::HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -948,7 +1039,7 @@ mod tests { let initialized = initialize(persisted, InitOptions { sandbox_env: SandboxEnvSpec { toml_env: HashMap::from([("TEST_KEY".to_string(), "value".to_string())]), - github_permissions: None, + github_integration: None, origin_url: None, }, ..test_init_options( @@ -1271,7 +1362,7 @@ mod tests { hooks: fabro_hooks::HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault, @@ -1366,7 +1457,7 @@ mod tests { hooks: fabro_hooks::HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -1508,7 +1599,7 @@ mod tests { hooks: fabro_hooks::HookSettings { hooks: vec![] }, sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), - github_permissions: None, + github_integration: None, origin_url: None, }, vault: auth_test_support::empty_vault(), @@ -1524,4 +1615,171 @@ mod tests { assert!(matches!(result, Err(Error::Cancelled))); } + + mod github_integration_env { + //! Focused tests for `build_sandbox_env` / + //! `validate_declared_repository_access` around declared additional + //! repositories. Installation-resolution failure naming is covered + //! by `fabro_github::access` tests; these prove the initialization + //! wiring: hard errors for declared sets, best-effort behavior for + //! legacy permissions-only configuration. + + use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; + use fabro_github::{GitHubAppCredentials, GitHubCredentials, InstallationToken}; + use fabro_types::settings::run::ResolvedGithubIntegration; + + use super::*; + + fn integration(additional: &[&str]) -> ResolvedGithubIntegration { + ResolvedGithubIntegration { + permissions: HashMap::from([( + "contents".to_string(), + "read".to_string(), + )]), + additional_repositories: additional + .iter() + .map(|value| value.parse().expect("test slug should parse")) + .collect(), + } + } + + fn spec( + origin: Option<&str>, + github_integration: Option, + ) -> SandboxEnvSpec { + SandboxEnvSpec { + toml_env: HashMap::new(), + github_integration, + origin_url: origin.map(str::to_string), + } + } + + #[test] + fn declared_additional_repositories_require_credentials() { + let spec = spec( + Some("https://github.com/fabro-sh/fabro"), + Some(integration(&["fabro-sh/keystone"])), + ); + let Err(err) = build_sandbox_env(&spec, None) else { + panic!("declared additional repositories without credentials must fail"); + }; + assert!( + err.to_string().contains("requires GitHub credentials"), + "{err}" + ); + } + + #[test] + fn declared_additional_repositories_require_an_origin() { + let spec = spec(None, Some(integration(&["fabro-sh/keystone"]))); + let creds = GitHubCredentials::Pat("ghp_x".to_string()); + let Err(err) = build_sandbox_env(&spec, Some(&creds)) else { + panic!("declared additional repositories without an origin must fail"); + }; + assert!( + err.to_string().contains("GitHub repository access"), + "{err}" + ); + } + + #[test] + fn declared_repositories_inject_bridge_entries_and_keep_the_pat_source() { + let spec = spec( + Some("https://github.com/fabro-sh/fabro"), + Some(integration(&["fabro-sh/keystone"])), + ); + let creds = GitHubCredentials::Pat("ghp_x".to_string()); + let built = build_sandbox_env(&spec, Some(&creds)).unwrap(); + + assert!(built.github_token.is_some()); + let access = built.github_access.expect("access should be constructed"); + assert!(access.has_additional_repositories()); + // Helper entry plus two SSH rewrites for each of the two + // effective repositories (origin + declared additional). + assert_eq!( + built.env.get("GIT_CONFIG_COUNT").map(String::as_str), + Some("5") + ); + assert_eq!( + built.env.get("GIT_CONFIG_KEY_0").map(String::as_str), + Some("credential.https://github.com.helper") + ); + assert_eq!( + built.env.get("GIT_TERMINAL_PROMPT").map(String::as_str), + Some("0") + ); + } + + #[test] + fn legacy_permissions_only_configuration_stays_best_effort() { + // No credentials: no error, no token source, no bridge entries. + let no_creds = spec( + Some("https://github.com/fabro-sh/fabro"), + Some(integration(&[])), + ); + let built = build_sandbox_env(&no_creds, None).unwrap(); + assert!(built.github_token.is_none()); + assert!(!built.env.contains_key("GIT_CONFIG_COUNT")); + + // App credentials without an origin: legacy best-effort skip. + let creds = GitHubCredentials::App(GitHubAppCredentials { + app_id: "1".to_string(), + private_key_pem: "unused".to_string(), + slug: None, + }); + let no_origin = spec(None, Some(integration(&[]))); + let built = build_sandbox_env(&no_origin, Some(&creds)).unwrap(); + assert!(built.github_token.is_none()); + assert!(built.github_access.is_none()); + } + + struct FailingMinter; + + #[async_trait::async_trait] + impl InstallationTokenMinter for FailingMinter { + async fn mint(&self) -> anyhow::Result { + Err(anyhow::anyhow!("scripted mint failure")) + } + } + + #[tokio::test] + async fn eager_validation_fails_when_the_declared_token_cannot_resolve() { + let access = fabro_github::GitHubRepositoryAccess::new( + Some("https://github.com/fabro-sh/fabro"), + &["fabro-sh/keystone".parse().unwrap()].into_iter().collect(), + HashMap::from([("contents".to_string(), "read".to_string())]), + ) + .unwrap(); + let built = BuiltSandboxEnv { + env: HashMap::new(), + github_token: Some(installation_token_source( + "fabro-sh/fabro (+1 additional)", + Arc::new(FailingMinter), + )), + github_access: access, + }; + + let err = validate_declared_repository_access(&built, None) + .await + .unwrap_err(); + let message = err.to_string(); + assert!(message.contains("declared repository set"), "{message}"); + } + + #[tokio::test] + async fn eager_validation_skips_legacy_permissions_only_runs() { + let built = BuiltSandboxEnv { + env: HashMap::new(), + github_token: Some(installation_token_source( + "fabro-sh/fabro", + Arc::new(FailingMinter), + )), + github_access: None, + }; + + validate_declared_repository_access(&built, None) + .await + .expect("legacy permissions-only runs must not resolve eagerly"); + } + } } diff --git a/lib/components/fabro-workflow/src/pipeline/types.rs b/lib/components/fabro-workflow/src/pipeline/types.rs index ebbe3f4f8..c65f1fdf6 100644 --- a/lib/components/fabro-workflow/src/pipeline/types.rs +++ b/lib/components/fabro-workflow/src/pipeline/types.rs @@ -8,7 +8,9 @@ use fabro_mcp::config::McpServerSettings; use fabro_model::{Catalog, ProviderId}; use fabro_sandbox::SandboxSpec; use fabro_template::TemplateContext; -use fabro_types::settings::run::{PullRequestSettings, RunModelControls}; +use fabro_types::settings::run::{ + PullRequestSettings, ResolvedGithubIntegration, RunModelControls, +}; use fabro_types::{ManifestPath, RunId, RunProjection}; use fabro_validate::{Diagnostic, Severity}; use fabro_vault::Vault; @@ -246,7 +248,10 @@ pub struct LlmSpec { #[derive(Clone)] pub struct SandboxEnvSpec { pub toml_env: HashMap, - pub github_permissions: Option>, + /// The resolved GitHub integration request (interpolated permissions + /// plus declared additional repositories). `None` when the run requests + /// no `GITHUB_TOKEN`. + pub github_integration: Option, pub origin_url: Option, }