diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 3596dfa24..24c423f12 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -501,7 +501,7 @@ fn maybe_build_github_credentials( let resolved_run = &settings.run; let resolved_server = ServerSettingsBuilder::load_default().ok(); let required_github_credentials = (resolved_run.execution.mode != RunMode::DryRun - && resolved_run.sandbox.provider == "daytona") + && clone_sandbox_requires_github_credentials(&resolved_run.sandbox.provider)) || resolved_server .as_ref() .is_some_and(|settings| !settings.server.integrations.github.permissions.is_empty()); @@ -515,20 +515,33 @@ fn maybe_build_github_credentials( .as_ref() .and_then(|settings| settings.server.integrations.github.app_id.as_ref()) .map(InterpString::as_source); + let app_slug = resolved_server + .as_ref() + .and_then(|settings| settings.server.integrations.github.slug.as_ref()) + .map(InterpString::as_source); if required_github_credentials { - return build_github_credentials(strategy, app_id.as_deref(), vault); + return build_github_credentials(strategy, app_id.as_deref(), app_slug.as_deref(), vault); } if pull_request_enabled { - return Ok(build_github_credentials(strategy, app_id.as_deref(), vault) - .ok() - .flatten()); + return Ok(build_github_credentials( + strategy, + app_id.as_deref(), + app_slug.as_deref(), + vault, + ) + .ok() + .flatten()); } Ok(None) } +fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { + matches!(provider, "docker" | "daytona") +} + fn install_signal_handlers( run_control: Arc, cancel_token: Arc, @@ -599,6 +612,13 @@ mod tests { }; use crate::args::RunWorkerMode; + #[test] + fn clone_sandbox_credentials_are_required_for_clone_based_providers() { + assert!(super::clone_sandbox_requires_github_credentials("docker")); + assert!(super::clone_sandbox_requires_github_credentials("daytona")); + assert!(!super::clone_sandbox_requires_github_credentials("local")); + } + fn running_event(actor: Option) -> fabro_types::RunEvent { fabro_types::RunEvent { id: "evt_1".to_string(), diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index 7eea66df3..5d6cbf51b 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -7,11 +7,12 @@ use fabro_vault::Vault; pub(crate) fn build_github_credentials( strategy: GithubIntegrationStrategy, app_id: Option<&str>, + app_slug: Option<&str>, vault: Option<&Vault>, ) -> anyhow::Result> { match strategy { GithubIntegrationStrategy::App => { - GitHubCredentials::from_env(app_id).map_err(|err| anyhow!(err)) + GitHubCredentials::from_env_with_slug(app_id, app_slug).map_err(|err| anyhow!(err)) } GithubIntegrationStrategy::Token => { let token = lookup_github_token(vault); diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 2c339e7db..3f82f1009 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -97,6 +97,7 @@ pub struct AppInfo { pub struct GitHubAppCredentials { pub app_id: String, pub private_key_pem: String, + pub slug: Option, } impl GitHubAppCredentials { @@ -112,6 +113,13 @@ impl GitHubAppCredentials { } pub fn from_env(app_id: Option<&str>) -> Result, String> { + Self::from_env_with_slug(app_id, None) + } + + pub fn from_env_with_slug( + app_id: Option<&str>, + slug: Option<&str>, + ) -> Result, String> { let Some(app_id) = app_id else { return Ok(None); }; @@ -121,8 +129,18 @@ impl GitHubAppCredentials { Ok(Some(Self { app_id: app_id.to_string(), private_key_pem, + slug: slug + .map(str::trim) + .filter(|slug| !slug.is_empty()) + .map(str::to_string), })) } + + pub fn installation_url(&self, owner: &str) -> Option { + self.slug.as_ref().map(|slug| { + format!("https://github.com/organizations/{owner}/settings/apps/{slug}/installations") + }) + } } #[derive(Clone, Debug)] @@ -136,6 +154,13 @@ impl GitHubCredentials { Ok(GitHubAppCredentials::from_env(app_id)?.map(Self::App)) } + pub fn from_env_with_slug( + app_id: Option<&str>, + slug: Option<&str>, + ) -> Result, String> { + Ok(GitHubAppCredentials::from_env_with_slug(app_id, slug)?.map(Self::App)) + } + async fn resolve_bearer_token( &self, client: &impl HttpClient, @@ -147,13 +172,15 @@ impl GitHubCredentials { match self { Self::App(creds) => { let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - create_installation_access_token_with_permissions( + let install_url = creds.installation_url(owner); + create_installation_access_token_with_permissions_and_install_url( client, &jwt, owner, repo, base_url, permissions, + install_url.as_deref(), ) .await } @@ -357,6 +384,27 @@ pub async fn create_installation_access_token_with_permissions( repo: &str, base_url: &str, permissions: serde_json::Value, +) -> Result { + create_installation_access_token_with_permissions_and_install_url( + client, + jwt, + owner, + repo, + base_url, + permissions, + None, + ) + .await +} + +pub async fn create_installation_access_token_with_permissions_and_install_url( + client: &impl HttpClient, + jwt: &str, + owner: &str, + repo: &str, + base_url: &str, + permissions: serde_json::Value, + install_url: Option<&str>, ) -> Result { #[derive(Deserialize)] struct Installation { @@ -369,19 +417,27 @@ pub async fn create_installation_access_token_with_permissions( } // Step 1: Find the installation for this repo - let install_url = format!("{base_url}/repos/{owner}/{repo}/installation"); + let installation_endpoint = format!("{base_url}/repos/{owner}/{repo}/installation"); let auth = format!("Bearer {jwt}"); let resp = client - .request(HttpMethod::Get, &install_url, &github_headers(&auth), None) + .request( + HttpMethod::Get, + &installation_endpoint, + &github_headers(&auth), + None, + ) .await .map_err(|e| format!("Failed to look up GitHub App installation: {e}"))?; match resp.status { 200 => {} 404 => { + let install_url = install_url.map(str::to_string).unwrap_or_else(|| { + format!("https://github.com/organizations/{owner}/settings/installations") + }); return Err(format!( "GitHub App is not installed for {owner}. \ - Install it at https://github.com/organizations/{owner}/settings/installations" + Install it at {install_url}" )); } 403 => { @@ -1600,6 +1656,33 @@ mod tests { assert!(err.contains("owner"), "got: {err}"); } + #[tokio::test] + async fn create_iat_not_installed_uses_app_specific_install_url() { + let mock = + MockHttpClient::new().on(HttpMethod::Get, "/repos/owner/repo/installation", 404, ""); + let install_url = + "https://github.com/organizations/owner/settings/apps/fabro-test/installations"; + + let err = create_installation_access_token_with_permissions_and_install_url( + &mock, + "jwt", + "owner", + "repo", + "", + serde_json::json!({ "contents": "write" }), + Some(install_url), + ) + .await + .unwrap_err(); + + assert!(err.contains("not installed"), "got: {err}"); + assert!(err.contains(install_url), "got: {err}"); + assert!( + !err.contains("https://github.com/organizations/owner/settings/installations"), + "got: {err}" + ); + } + #[tokio::test] async fn create_iat_suspended() { let mock = @@ -1706,6 +1789,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1744,6 +1828,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1782,6 +1867,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1943,6 +2029,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let detail = get_pull_request_with_client( &mock, @@ -1988,6 +2075,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = get_pull_request_with_client( &mock, @@ -2084,6 +2172,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); merge_pull_request_with_client( &mock, @@ -2118,6 +2207,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = merge_pull_request_with_client( &mock, @@ -2153,6 +2243,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = merge_pull_request_with_client( &mock, @@ -2197,6 +2288,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); close_pull_request_with_client(&mock, &GitHubContext::new(&creds, ""), "owner", "repo", 42) .await @@ -2224,6 +2316,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = close_pull_request_with_client( &mock, diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index 53a3eae2c..de46ea983 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -12,6 +12,7 @@ fn github_credentials() -> GitHubCredentials { GitHubCredentials::App(GitHubAppCredentials { app_id: "42".to_string(), private_key_pem: TEST_RSA_KEY.to_string(), + slug: Some("test-app".to_string()), }) } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index d18bf7ab1..3668c9a40 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -969,13 +969,15 @@ async fn mint_github_token( .map_err(|err| anyhow!("{err}"))?; let client = fabro_http::http_client()?; let perms_json = serde_json::to_value(permissions)?; - fabro_github::create_installation_access_token_with_permissions( + let install_url = creds.installation_url(&owner); + fabro_github::create_installation_access_token_with_permissions_and_install_url( &client, &jwt, &owner, &repo, &fabro_github::github_api_base_url(), perms_json, + install_url.as_deref(), ) .await .map_err(|err| anyhow!("{err}")) diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 4c8eb55a3..2ccae6603 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -801,6 +801,7 @@ impl AppState { fabro_github::GitHubAppCredentials { app_id, private_key_pem, + slug: settings.slug.as_ref().map(InterpString::as_source), }, ))) } @@ -1743,6 +1744,10 @@ fn system_sandbox_provider( ) } +fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { + matches!(provider, "docker" | "daytona") +} + fn parse_system_duration(raw: &str) -> anyhow::Result { let raw = raw.trim(); anyhow::ensure!(!raw.is_empty(), "empty duration string"); @@ -2033,13 +2038,14 @@ async fn get_github_repo( .into_response(); } - match fabro_github::create_installation_access_token_with_permissions( + match fabro_github::create_installation_access_token_with_permissions_and_install_url( client_ref, &jwt, &owner, &name, &base_url, serde_json::json!({ "contents": "write", "pull_requests": "write" }), + Some(&install_url), ) .await { @@ -4632,7 +4638,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { let github_app_result = { let settings = &persisted.run_spec().settings.run; let required_github_credentials = (settings.execution.mode != RunMode::DryRun - && settings.sandbox.provider == "daytona") + && clone_sandbox_requires_github_credentials(&settings.sandbox.provider)) || !github_settings.permissions.is_empty(); if required_github_credentials { state.github_credentials(github_settings) @@ -8667,6 +8673,13 @@ provider = "invalid-provider" ); } + #[test] + fn clone_sandbox_credentials_are_required_for_clone_based_providers() { + assert!(clone_sandbox_requires_github_credentials("docker")); + assert!(clone_sandbox_requires_github_credentials("daytona")); + assert!(!clone_sandbox_requires_github_credentials("local")); + } + #[tokio::test] async fn create_secret_stores_file_secret_and_excludes_it_from_snapshot() { let state = create_app_state(); diff --git a/lib/crates/fabro-tracker/src/github.rs b/lib/crates/fabro-tracker/src/github.rs index 10aa4de9e..92485ed73 100644 --- a/lib/crates/fabro-tracker/src/github.rs +++ b/lib/crates/fabro-tracker/src/github.rs @@ -634,6 +634,7 @@ mod tests { GitHubCredentials::App(GitHubAppCredentials { app_id: "test-app".to_string(), private_key_pem: pem, + slug: None, }), test_http_client(), "owner".to_string(), diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index b1f3b3f27..69bead9e8 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -229,13 +229,15 @@ async fn mint_github_token( .map_err(|e| Error::engine(e.clone()))?; let client = fabro_http::http_client().map_err(|e| Error::engine(e.to_string()))?; let perms_json = serde_json::to_value(permissions).map_err(|e| Error::engine(e.to_string()))?; - fabro_github::create_installation_access_token_with_permissions( + let install_url = creds.installation_url(&owner); + fabro_github::create_installation_access_token_with_permissions_and_install_url( &client, &jwt, &owner, &repo, &fabro_github::github_api_base_url(), perms_json, + install_url.as_deref(), ) .await .map_err(|e| Error::engine(e.clone())) diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 48572a331..81f308d7b 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1510,6 +1510,7 @@ mod tests { let creds = fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id: "123".to_string(), private_key_pem: "unused".to_string(), + slug: None, }); let base_url = github_app::github_api_base_url(); let result = maybe_open_pull_request(OpenPullRequestRequest { diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 5c4874406..5b33f0789 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -207,6 +207,7 @@ fn load_github_app_credentials() -> fabro_github::GitHubCredentials { fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id, private_key_pem, + slug: None, }) }