From 6fc4fbae64c91874e16a97337beebab27018ca58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Balatoni?= Date: Sun, 25 Oct 2026 02:10:40 +0200 Subject: [PATCH] fix(github): harden GitHub host handling after review --- lib/apps/fabro-cli/src/commands/install.rs | 2 +- lib/apps/fabro-cli/src/gh.rs | 12 +- lib/apps/fabro-server/src/spawn_env.rs | 4 + lib/components/fabro-github/src/lib.rs | 24 +++- .../fabro-petri/src/stage_credentials.rs | 20 ++- .../fabro-workflow/src/pull_request.rs | 18 ++- lib/foundation/fabro-static/src/env_vars.rs | 4 + .../fabro-static/src/secret_registry.rs | 3 + lib/foundation/fabro-types/src/github_host.rs | 121 ++++++++++++++---- .../fabro-types/src/pull_request.rs | 27 +++- lib/foundation/fabro-types/src/run_intent.rs | 2 + 11 files changed, 196 insertions(+), 41 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/install.rs b/lib/apps/fabro-cli/src/commands/install.rs index 1beed4120..adf30e7d1 100644 --- a/lib/apps/fabro-cli/src/commands/install.rs +++ b/lib/apps/fabro-cli/src/commands/install.rs @@ -1169,7 +1169,7 @@ async fn setup_github_app( printer, " {}", s.dim - .apply_to(format!("App: {}/apps/{slug}", github_host.web_url())) + .apply_to(format!("App: {}", github_host.app_page_url(&slug))) ); // Return secret pairs diff --git a/lib/apps/fabro-cli/src/gh.rs b/lib/apps/fabro-cli/src/gh.rs index 62078b03f..09742392b 100644 --- a/lib/apps/fabro-cli/src/gh.rs +++ b/lib/apps/fabro-cli/src/gh.rs @@ -32,19 +32,19 @@ impl GhCli { } let auth = Command::new(command) - .args(["auth", "status", "--hostname", host.host()]) + .args(["auth", "status", "--hostname", host.hostname()]) .output() .await; match auth { Ok(o) if o.status.success() => { - debug!("gh CLI available and authenticated for {}", host.host()); + debug!("gh CLI available and authenticated for {}", host.hostname()); Some(Self { command: command.to_string(), host: host.clone(), }) } _ => { - debug!("gh is not authenticated for {}", host.host()); + debug!("gh is not authenticated for {}", host.hostname()); None } } @@ -56,7 +56,7 @@ impl GhCli { .args([ "api", "--hostname", - self.host.host(), + self.host.hostname(), "/user", "--jq", ".login", @@ -80,7 +80,7 @@ impl GhCli { .args([ "api", "--hostname", - self.host.host(), + self.host.hostname(), "--paginate", "/user/memberships/orgs", "--jq", @@ -138,7 +138,7 @@ mod tests { for (web_url, hostname) in [ ("https://github.com", "github.com"), - ("https://ghe.example.com:8443", "ghe.example.com:8443"), + ("https://ghe.example.com:8443", "ghe.example.com"), ] { let host = GitHubHost::new(web_url, None).unwrap(); let gh = GhCli::detect_with_command(script.to_str().unwrap(), &host) diff --git a/lib/apps/fabro-server/src/spawn_env.rs b/lib/apps/fabro-server/src/spawn_env.rs index 02b433093..1dc7c97c9 100644 --- a/lib/apps/fabro-server/src/spawn_env.rs +++ b/lib/apps/fabro-server/src/spawn_env.rs @@ -73,6 +73,10 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ // reach it through FABRO_HOME. EnvVars::GITHUB_WEB_URL, EnvVars::GITHUB_BASE_URL, + // Private CA bundle for the worker's TLS stack (rustls platform verifier; + // honoured on Linux only), e.g. for a GitHub Enterprise Server host. + EnvVars::SSL_CERT_FILE, + EnvVars::SSL_CERT_DIR, // A test's checkpoint gates: the worker's hooks hold at a named point // until the test releases them, so a crash can be placed there. EnvVars::FABRO_TEST_CHECKPOINT_GATES, diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index 8f45ea432..4391e8f36 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -79,7 +79,13 @@ fn host_with_overrides( (None, Some(api)) => GitHubHost::new(configured.web_url(), Some(api)), (Some(web), api) => GitHubHost::new(web, api), }; - host.context("invalid GITHUB_WEB_URL or GITHUB_BASE_URL override") + host.map_err(|error| { + let variable = match error.field() { + "api_url" => EnvVars::GITHUB_BASE_URL, + _ => EnvVars::GITHUB_WEB_URL, + }; + anyhow::Error::new(error).context(format!("invalid {variable} override")) + }) } /// Bundle of GitHub credentials and the GitHub host, threaded through every @@ -345,7 +351,7 @@ pub fn validate_static_github_token(token: &str) -> anyhow::Result<()> { pub async fn gh_auth_token(host: &GitHubHost) -> anyhow::Result { let output = Command::new("gh") - .args(["auth", "token", "--hostname", host.host()]) + .args(["auth", "token", "--hostname", host.hostname()]) .output() .await .context("Failed to run `gh auth token`")?; @@ -1799,7 +1805,19 @@ mod tests { let web = host_with_overrides(&configured, Some("https://octocorp.ghe.com"), None).unwrap(); assert_eq!(web.api_url(), "https://api.octocorp.ghe.com"); - assert!(host_with_overrides(&configured, Some("http://ghe.example.com"), None).is_err()); + let plain_web = + host_with_overrides(&configured, Some("http://ghe.example.com"), None).unwrap_err(); + assert!( + plain_web.to_string().contains("GITHUB_WEB_URL"), + "{plain_web}" + ); + + let plain_api = + host_with_overrides(&configured, None, Some("http://mirror.corp:8080")).unwrap(); + assert_eq!(plain_api.api_url(), "http://mirror.corp:8080"); + let bad_api = + host_with_overrides(&configured, None, Some("ftp://mirror.corp")).unwrap_err(); + assert!(bad_api.to_string().contains("GITHUB_BASE_URL"), "{bad_api}"); } #[test] diff --git a/lib/components/fabro-petri/src/stage_credentials.rs b/lib/components/fabro-petri/src/stage_credentials.rs index 6bdbbdab4..5c6fa12a1 100644 --- a/lib/components/fabro-petri/src/stage_credentials.rs +++ b/lib/components/fabro-petri/src/stage_credentials.rs @@ -322,8 +322,8 @@ impl CredentialEnv { ), (helper_key.clone(), String::new()), (helper_key, format!("store --file={}", self.store_path())), - (rewrite_key.clone(), format!("git@{}:", host.host())), - (rewrite_key, format!("ssh://git@{}/", host.host())), + (rewrite_key.clone(), format!("git@{}:", host.hostname())), + (rewrite_key, format!("ssh://git@{}/", host.hostname())), ]; for index in 0..count { for prefix in ["GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"] { @@ -371,7 +371,10 @@ impl SpawnEnv for CredentialEnv { // The `gh` CLI reads GITHUB_TOKEN for github.com only; any other // host needs GH_HOST and GH_ENTERPRISE_TOKEN. if !self.credentials.host.is_github_com() { - env.insert(EnvVars::GH_HOST.into(), self.credentials.host.host().into()); + env.insert( + EnvVars::GH_HOST.into(), + self.credentials.host.hostname().into(), + ); env.insert( EnvVars::GH_ENTERPRISE_TOKEN.into(), resolved.token.expose().into(), @@ -606,6 +609,17 @@ mod tests { assert!(env["GITHUB_TOKEN"].starts_with("scripted-token-generation-")); } + #[tokio::test] + async fn a_ghes_scope_with_a_port_keeps_the_port_out_of_ssh_and_gh_host() { + let host = GitHubHost::new("https://ghe.example.com:8443", None).expect("host"); + let (env, _store) = process_env_on(host).await; + let config = git_config(&env); + assert_eq!(config[3].0, "url.https://ghe.example.com:8443/.insteadOf"); + assert_eq!(config[3].1, "git@ghe.example.com:"); + assert_eq!(config[4].1, "ssh://git@ghe.example.com/"); + assert_eq!(env["GH_HOST"], "ghe.example.com"); + } + #[test] fn default_origin_access_does_not_request_an_api_token() { let mut spec = types_support::test_run_spec(); diff --git a/lib/components/fabro-workflow/src/pull_request.rs b/lib/components/fabro-workflow/src/pull_request.rs index fbeee875b..9c226b898 100644 --- a/lib/components/fabro-workflow/src/pull_request.rs +++ b/lib/components/fabro-workflow/src/pull_request.rs @@ -468,7 +468,10 @@ async fn reconcile_existing_pull_request( owner: owner.to_string(), repo: repo.to_string(), number: existing.number, - html_url: existing.html_url.clone(), + html_url: req + .github + .host() + .pull_request_url(owner, repo, existing.number), }, title: existing.title, base_branch: req.base_branch.to_string(), @@ -622,11 +625,15 @@ pub async fn open_pull_request( ) .await; + let html_url = req + .github + .host() + .pull_request_url(&owner, &repo, created.number); let link = PullRequestLink { owner, repo, number: created.number, - html_url: created.html_url.clone(), + html_url, }; Ok(CreatedPullRequest { @@ -1547,7 +1554,7 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr &payload, "final-sha", serde_json::json!([{ - "html_url": "https://github.com/owner/repo/pull/7", + "html_url": "https://github.com/OWNER/Repo/pull/7", "number": 7, "node_id": "PR_existing", "title": "Reconciled title", @@ -1582,6 +1589,8 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr .expect("reconciliation should adopt the existing pull request"); assert_eq!(result.link.number, 7); + // The URL follows the stored coordinates, not the API's spelling. + assert_eq!(result.link.html_url, "https://github.com/owner/repo/pull/7"); assert_eq!(result.title, "Reconciled title"); // Adoption must not cost an LLM call or a create request. assert_eq!( @@ -1632,6 +1641,9 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr .expect("PR creation should succeed"); assert_eq!(result.title, "Fix telemetry leak"); + // The mock API answers with an `example.test` URL; the stored URL is + // derived from the stored coordinates instead. + assert_eq!(result.link.html_url, "https://github.com/owner/repo/pull/1"); harness.assert_mocks_called_once().await; } diff --git a/lib/foundation/fabro-static/src/env_vars.rs b/lib/foundation/fabro-static/src/env_vars.rs index 330c6f0a2..f56a3b789 100644 --- a/lib/foundation/fabro-static/src/env_vars.rs +++ b/lib/foundation/fabro-static/src/env_vars.rs @@ -188,6 +188,8 @@ impl EnvVars { pub const RUST_BACKTRACE: &'static str = "RUST_BACKTRACE"; pub const RUST_LOG: &'static str = "RUST_LOG"; pub const SHELL: &'static str = "SHELL"; + pub const SSL_CERT_DIR: &'static str = "SSL_CERT_DIR"; + pub const SSL_CERT_FILE: &'static str = "SSL_CERT_FILE"; pub const TERM: &'static str = "TERM"; pub const TMPDIR: &'static str = "TMPDIR"; pub const TWIN_OPENAI_BIND_ADDR: &'static str = "TWIN_OPENAI_BIND_ADDR"; @@ -345,6 +347,8 @@ mod tests { EnvVars::RUST_BACKTRACE, EnvVars::RUST_LOG, EnvVars::SHELL, + EnvVars::SSL_CERT_DIR, + EnvVars::SSL_CERT_FILE, EnvVars::TERM, EnvVars::TMPDIR, EnvVars::TWIN_OPENAI_BIND_ADDR, diff --git a/lib/foundation/fabro-static/src/secret_registry.rs b/lib/foundation/fabro-static/src/secret_registry.rs index 127bc77ad..0f45b3e7c 100644 --- a/lib/foundation/fabro-static/src/secret_registry.rs +++ b/lib/foundation/fabro-static/src/secret_registry.rs @@ -121,12 +121,15 @@ mod tests { #[test] fn leaves_legacy_aliases_and_non_secret_config_unclassified() { for name in [ + // A sandbox-injected copy of the resolved token, not a vault secret. EnvVars::GH_ENTERPRISE_TOKEN, EnvVars::GH_HOST, EnvVars::GH_TOKEN, EnvVars::GITHUB_BASE_URL, EnvVars::GITHUB_WEB_URL, EnvVars::SLACK_BASE_URL, + EnvVars::SSL_CERT_DIR, + EnvVars::SSL_CERT_FILE, EnvVars::DAYTONA_API_URL, EnvVars::DAYTONA_ORGANIZATION_ID, EnvVars::DAYTONA_SERVER_URL, diff --git a/lib/foundation/fabro-types/src/github_host.rs b/lib/foundation/fabro-types/src/github_host.rs index 0033c38fd..8cedcab16 100644 --- a/lib/foundation/fabro-types/src/github_host.rs +++ b/lib/foundation/fabro-types/src/github_host.rs @@ -67,15 +67,16 @@ impl Default for GitHubHost { #[expect( clippy::disallowed_types, - reason = "GitHub host settings are public service origins; validation errors never echo credentials." + reason = "GitHub host settings are public service origins; validation errors name the field and rule but never echo the value, which may embed credentials." )] -fn parse_origin(field: &'static str, raw: &str) -> Result { +fn parse_origin( + field: &'static str, + raw: &str, + plain_http_anywhere: bool, +) -> Result { let raw = raw.trim().trim_end_matches('/'); let parsed = url::Url::parse(raw).map_err(|_| { - GitHubHostError::new( - field, - format!("must be an https URL such as https://github.com (got `{raw}`)"), - ) + GitHubHostError::new(field, "must be an https URL such as https://github.com") })?; let host = parsed .host_str() @@ -83,12 +84,14 @@ fn parse_origin(field: &'static str, raw: &str) -> Result {} - "http" if loopback => {} + "http" if loopback || plain_http_anywhere => {} _ => { - return Err(GitHubHostError::new( - field, - format!("must use https (got `{raw}`); http is only allowed for localhost"), - )); + let problem = if plain_http_anywhere { + "must use http or https" + } else { + "must use https; http is only allowed for localhost" + }; + return Err(GitHubHostError::new(field, problem)); } } if !parsed.username().is_empty() || parsed.password().is_some() { @@ -107,7 +110,7 @@ impl GitHubHost { /// Builds a host from the web origin and an optional explicit API URL. /// Without `api_url`, the API URL is derived from the web origin. pub fn new(web_url: &str, api_url: Option<&str>) -> Result { - let web = parse_origin("web_url", web_url)?; + let web = parse_origin("web_url", web_url, false)?; if !matches!(web.path(), "" | "/") { return Err(GitHubHostError::new( "web_url", @@ -117,7 +120,7 @@ impl GitHubHost { let web_origin = web.origin().ascii_serialization(); let api_url = match api_url { Some(api) => { - parse_origin("api_url", api)?; + parse_origin("api_url", api, true)?; api.trim().trim_end_matches('/').to_string() } None => derive_api_url(&web_origin, web.host_str().unwrap_or_default()), @@ -170,6 +173,18 @@ impl GitHubHost { self.host() == "github.com" } + /// Public page of a GitHub App; also the base of its install link. + /// GitHub Enterprise Server serves it under `/github-apps/`. + #[must_use] + pub fn app_page_url(&self, slug: &str) -> String { + let prefix = if self.is_github_com() || self.hostname().ends_with(".ghe.com") { + "apps" + } else { + "github-apps" + }; + format!("{}/{prefix}/{slug}", self.web_url) + } + /// GraphQL endpoint derived from the REST API URL. #[must_use] pub fn graphql_url(&self) -> String { @@ -192,7 +207,7 @@ impl GitHubHost { /// Domain of GitHub's private noreply commit emails. #[must_use] pub fn noreply_email_domain(&self) -> String { - format!("users.noreply.{}", self.host()) + format!("users.noreply.{}", self.hostname()) } /// Git config key that routes this host's HTTPS credentials to a helper. @@ -206,18 +221,18 @@ impl GitHubHost { #[must_use] pub fn is_origin(&self, origin: &str) -> bool { let host = self.host(); + let hostname = self.hostname(); origin.starts_with(&format!("{}/", self.web_url)) || origin.starts_with(&format!("http://{host}/")) - || origin.starts_with(&format!("git@{host}:")) - || origin.starts_with(&format!("ssh://git@{host}/")) + || origin.starts_with(&format!("git@{hostname}:")) + || origin.starts_with(&format!("ssh://git@{hostname}/")) } } fn derive_api_url(web_origin: &str, hostname: &str) -> String { - let under = |domain: &str| hostname == domain || hostname.ends_with(&format!(".{domain}")); - if under("github.com") { + if hostname == "github.com" { DEFAULT_API_URL.to_string() - } else if under("ghe.com") { + } else if hostname == "ghe.com" || hostname.ends_with(".ghe.com") { web_origin.replacen("://", "://api.", 1) } else { format!("{web_origin}/api/v3") @@ -351,14 +366,72 @@ mod tests { path.to_string().contains("must not contain a path"), "{path}" ); - let api = GitHubHost::new( + let api = + GitHubHost::new("https://ghe.example.com", Some("ftp://ghe.example.com")).unwrap_err(); + assert_eq!(api.field(), "api_url"); + assert!( + api.to_string().starts_with("api_url must use http"), + "{api}" + ); + } + + #[test] + fn errors_never_echo_the_value() { + for web in [ + "http://user:secret@ghe.example.com", + "https://user:secret@ghe.example.com", + "secret ghe.example.com", + ] { + let error = GitHubHost::new(web, None).unwrap_err(); + assert!(!error.to_string().contains("secret"), "{error}"); + } + let error = GitHubHost::new( "https://ghe.example.com", - Some("http://ghe.example.com/api/v3"), + Some("ftp://user:secret@ghe.example.com"), ) .unwrap_err(); - assert!( - api.to_string().starts_with("api_url must use https"), - "{api}" + assert!(!error.to_string().contains("secret"), "{error}"); + } + + #[test] + fn api_url_accepts_plain_http_on_any_host() { + let ghes = GitHubHost::new( + "https://ghe.example.com", + Some("http://github-mirror.corp:8080/"), + ) + .unwrap(); + assert_eq!(ghes.api_url(), "http://github-mirror.corp:8080"); + } + + #[test] + fn port_is_kept_for_http_urls_only() { + let ghes = host("https://ghe.example.com:8443"); + assert_eq!(ghes.noreply_email_domain(), "users.noreply.ghe.example.com"); + assert!(ghes.is_origin("git@ghe.example.com:o/r.git")); + assert!(ghes.is_origin("ssh://git@ghe.example.com/o/r")); + assert!(ghes.is_origin("https://ghe.example.com:8443/o/r")); + } + + #[test] + fn only_github_com_itself_derives_the_github_com_api() { + let sub = host("https://foo.github.com"); + assert!(!sub.is_github_com()); + assert_eq!(sub.api_url(), "https://foo.github.com/api/v3"); + } + + #[test] + fn app_page_url_follows_the_host_kind() { + assert_eq!( + GitHubHost::default().app_page_url("my-app"), + "https://github.com/apps/my-app" + ); + assert_eq!( + host("https://octocorp.ghe.com").app_page_url("my-app"), + "https://octocorp.ghe.com/apps/my-app" + ); + assert_eq!( + host("https://ghe.example.com").app_page_url("my-app"), + "https://ghe.example.com/github-apps/my-app" ); } diff --git a/lib/foundation/fabro-types/src/pull_request.rs b/lib/foundation/fabro-types/src/pull_request.rs index 19b263313..54d11d384 100644 --- a/lib/foundation/fabro-types/src/pull_request.rs +++ b/lib/foundation/fabro-types/src/pull_request.rs @@ -121,7 +121,11 @@ impl<'de> Deserialize<'de> for PullRequestLink { .html_url .unwrap_or_else(|| GitHubHost::default().pull_request_url(&owner, &repo, number)); let url_link = github_pull_request_link_from_url(&html_url).map_err(D::Error::custom)?; - if (&url_link.owner, &url_link.repo, url_link.number) != (&owner, &repo, number) { + // GitHub owner and repository names are case-insensitive. + if !url_link.owner.eq_ignore_ascii_case(&owner) + || !url_link.repo.eq_ignore_ascii_case(&repo) + || url_link.number != number + { return Err(D::Error::custom( "pull request html_url does not match owner/repo/number", )); @@ -341,6 +345,27 @@ mod tests { ); } + #[test] + fn pull_request_link_owner_and_repo_match_html_url_case_insensitively() { + let link = serde_json::from_value::(json!({ + "owner": "Acme", + "repo": "Widgets", + "number": 7, + "html_url": "https://github.com/acme/widgets/pull/7" + })) + .unwrap(); + assert_eq!(link.number, 7); + assert!( + serde_json::from_value::(json!({ + "owner": "Acme", + "repo": "Other", + "number": 7, + "html_url": "https://github.com/acme/widgets/pull/7" + })) + .is_err() + ); + } + #[test] fn pull_request_link_rejects_extra_legacy_record_fields() { let result = serde_json::from_value::(json!({ diff --git a/lib/foundation/fabro-types/src/run_intent.rs b/lib/foundation/fabro-types/src/run_intent.rs index 02f0b5732..b47d79d5d 100644 --- a/lib/foundation/fabro-types/src/run_intent.rs +++ b/lib/foundation/fabro-types/src/run_intent.rs @@ -82,6 +82,7 @@ impl GitRunTarget { /// /// Same as [`GitRunTarget::validate`]. pub fn canonical(self) -> Result { + // The host does not affect the canonical form. self.validate(&GitHubHost::default()) .map(ValidatedGitRunTarget::into_target) } @@ -146,6 +147,7 @@ impl RunTarget { /// /// Same as [`RunTarget::validate`]. pub fn canonical(self) -> Result { + // The host does not affect the canonical form. self.validate(&GitHubHost::default()) .map(|validated| validated.target) }