scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-11-05 07:37:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

fix(github): harden GitHub host handling after review

Browse source
This commit is contained in:
Dávid Balatoni 2026-10-25 02:10:40 +02:00
parent 904046dc72
commit 6fc4fbae64
No known key found for this signature in database
11 changed files with 196 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/apps/fabro-cli/src/commands/install.rs
View file

@ -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

12
lib/apps/fabro-cli/src/gh.rs
View file

@ -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)

4
lib/apps/fabro-server/src/spawn_env.rs
View file

@ -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,

24
lib/components/fabro-github/src/lib.rs
View file

@ -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<String> {
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]

20
lib/components/fabro-petri/src/stage_credentials.rs
View file

@ -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();

18
lib/components/fabro-workflow/src/pull_request.rs
View file

@ -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;
}

4
lib/foundation/fabro-static/src/env_vars.rs
View file

@ -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,

3
lib/foundation/fabro-static/src/secret_registry.rs
View file

@ -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,

121
lib/foundation/fabro-types/src/github_host.rs
View file

@ -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<url::Url, GitHubHostError> {
fn parse_origin(
field: &'static str,
raw: &str,
plain_http_anywhere: bool,
) -> Result<url::Url, GitHubHostError> {
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<url::Url, GitHubHostEr
let loopback = matches!(host, "localhost" | "127.0.0.1" | "[::1]");
match parsed.scheme() {
"https" => {}
"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<Self, GitHubHostError> {
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"
);
}

27
lib/foundation/fabro-types/src/pull_request.rs
View file

@ -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::<PullRequestLink>(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::<PullRequestLink>(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::<PullRequestLink>(json!({

2
lib/foundation/fabro-types/src/run_intent.rs
View file

@ -82,6 +82,7 @@ impl GitRunTarget {
///
/// Same as [`GitRunTarget::validate`].
pub fn canonical(self) -> Result<Self, GitCoordinateValidationError> {
// 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<Self, TargetValidationError> {
// The host does not affect the canonical form.
self.validate(&GitHubHost::default())
.map(|validated| validated.target)
}