diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 6a28949..012e839 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -1450,8 +1450,8 @@ against acme -- DERIVED FROM ORIGIN, not pinned. […] Pin it with The pin is asked first and the forge second. A destination on the allow-list — the pinned owner, `allowed_owners`, `allowed_repos` — passes exactly as before, -with no network call. Only a destination the list refused, and only on GitHub, -is put to `gh`: whether `gh api user` is the destination's owner, and failing +with no network call. Only a destination the list refused, and only on +github.com, is put to `gh`: whether `gh api user` is the destination's owner, and failing that whether `gh api repos//` reports `permissions.admin`. Either is ownership and the run exits `0` with nothing extra printed. This covers what a pin cannot, since a bundled rule takes no parameter, and it is not the @@ -1463,7 +1463,10 @@ the forge was asked and disagreed too. A forge that **could not be asked** — n `gh`, not authenticated, no network, output that is neither a yes nor a no — is exit `2` with the same report and a line saying why: a question that could not be asked is not a pass. A destination on a host no client answers for keeps the -allow-list's answer unchanged. +allow-list's answer unchanged — a refusal, exit `1`. A GitHub Enterprise host +such as `github.acme.com` is one of those: `gh api` without `--hostname` asks +github.com, whose same-named repository is somebody else's, so it is never +asked. ### Built-in parameters diff --git a/src/git.rs b/src/git.rs index ed13471..302eeb6 100644 --- a/src/git.rs +++ b/src/git.rs @@ -423,10 +423,18 @@ pub(crate) fn remote_url(root: &Path, remote: &str) -> Option { /// for https, http, ssh, git and `git+ssh`, and the scp-like /// `[user@]host:path`, which git reads as scp-like only where the colon comes /// before any slash. A local path, or a `file://` url, names no host. +/// +/// [`host`] and [`owner_repo`] agree about a url, or both yield None when +/// [`ambiguous`] says this parser cannot be sure which host git contacts. pub(crate) fn host(url: &str) -> Option { let url = url.trim(); - let authority = if let Some((_, rest)) = url.split_once("://") { - rest.split('/').next().unwrap_or(rest) + // A `file://` url names no host, whatever stands where one would: git + // pushes it to a local path. + if ambiguous(url) || is_file_url(url) { + return None; + } + let authority = if let Some((authority, _)) = url_parts(url) { + authority } else { let (authority, _) = url.split_once(':')?; if authority.contains('/') { @@ -444,9 +452,70 @@ pub(crate) fn host(url: &str) -> Option { (!host.is_empty()).then(|| host.to_lowercase()) } +/// Whether a url is spelled so that git may contact a host other than the one +/// read here, so that neither a host nor an owner/repo is read from it. +/// +/// Fail-closed rather than a second copy of git's parser: git url-decodes a +/// `scheme://` authority before splitting it, so a `%2F@` can put the real +/// host in front of an `@` that seems to end the userinfo; git strips the +/// brackets of a scp-like `[host]:path` and lets ssh read whatever is inside +/// them. +fn ambiguous(url: &str) -> bool { + if let Some((authority, _)) = url_parts(url) { + return authority.contains('%'); + } + url.split_once(':') + .is_some_and(|(authority, _)| !authority.contains('/') && authority.contains('[')) +} + +/// Whether a url is spelled `file://`, in any case. +fn is_file_url(url: &str) -> bool { + url.get(..7) + .is_some_and(|scheme| scheme.eq_ignore_ascii_case("file://")) +} + +/// The authority and path of a `scheme://` url, the path without any query or +/// fragment. +/// +/// The authority ends at the first `/`, `?` or `#`, as RFC 3986 and curl read +/// it. Ending it at `/` alone took `https://evil.com#@github.com/acme/widget` +/// to be github.com's `acme/widget`, while git sends the push to evil.com. +fn url_parts(url: &str) -> Option<(&str, &str)> { + let (_, rest) = url.split_once("://")?; + let end = rest.find(['/', '?', '#']).unwrap_or(rest.len()); + let (authority, tail) = rest.split_at(end); + let path = tail.split(['?', '#']).next().unwrap_or(tail); + Some((authority, path)) +} + +/// Whether `gh` can answer for a name written against this host -- the one +/// definition of "GitHub" in this binary. +/// +/// Only GitHub, and only exactly GitHub. A GitHub Enterprise host is a +/// DIFFERENT forge that happens to share the software: asking github.com about +/// `acme/widget` seen on `github.acme.com` answers about someone else's +/// repository, and a public answer there passes a private repository here. The +/// same holds for ownership: `gh api` without `--hostname` asks github.com, so +/// an enterprise push judged by it would be judged by a stranger's repository. +pub(crate) fn is_github_host(host: &str) -> bool { + matches!( + host.to_lowercase().as_str(), + "github.com" | "www.github.com" | "raw.githubusercontent.com" + ) +} + /// `owner/repo` from any spelling of a forge url. +/// +/// None for every url [`ambiguous`] refuses, as [`host`] is. pub(crate) fn owner_repo(url: &str) -> Option<(String, String)> { - let trimmed = url.trim().trim_end_matches('/'); + let url = url.trim(); + if ambiguous(url) { + return None; + } + // A `scheme://` url names its repository in its path and nowhere else. + let trimmed = url_parts(url) + .map_or(url, |(_, path)| path) + .trim_end_matches('/'); let without_git = trimmed.strip_suffix(".git").unwrap_or(trimmed); // scp-like (`git@host:owner/repo`) and url forms both end in owner/repo. let tail = without_git @@ -486,6 +555,14 @@ mod tests { ("git@github.com:acme/widget.git", "github.com"), ("github.com:acme/widget.git", "github.com"), ("ssh://git@[::1]:22/acme/widget.git", "::1"), + // The authority ends at a query or fragment, so an `@` after one + // is not userinfo and the host is the one git connects to. + ("https://evil.com#@github.com/acme/widget.git", "evil.com"), + ("https://evil.com?@github.com/acme/widget.git", "evil.com"), + ( + "https://u@evil.com#x@github.com/acme/widget.git", + "evil.com", + ), ] { assert_eq!(host(url).as_deref(), Some(want), "{url}"); } @@ -504,6 +581,24 @@ mod tests { } } + #[test] + fn a_file_url_names_no_host_but_its_path_still_names_a_repository() { + // git pushes a file:// url to a local path, so no forge answers for + // it, but its path is read as a plain path is. + for (url, owner, repo) in [ + ("file://github.com/acme/widget.git", "acme", "widget"), + ("FILE://github.com/acme/widget.git", "acme", "widget"), + ("file:///tmp/x/bare.git", "x", "bare"), + ] { + assert_eq!(host(url), None, "{url}"); + assert_eq!( + owner_repo(url), + Some((owner.to_owned(), repo.to_owned())), + "{url}" + ); + } + } + #[test] fn owner_and_repo_come_out_of_every_url_spelling() { for url in [ @@ -512,6 +607,49 @@ mod tests { "git@github.com:acme/widget.git", "ssh://git@github.com/acme/widget.git", "https://github.com/acme/widget/", + "https://github.com/acme/widget.git#main", + "https://github.com/acme/widget?tab=readme", + ] { + assert_eq!( + owner_repo(url), + Some(("acme".to_owned(), "widget".to_owned())), + "{url}" + ); + } + } + + #[test] + fn owner_and_repo_are_never_read_past_a_query_or_fragment() { + for url in [ + "https://evil.com#@github.com/acme/widget.git", + "https://evil.com?@github.com/acme/widget.git", + ] { + assert_eq!(owner_repo(url), None, "{url}"); + } + } + + #[test] + fn a_url_git_may_send_to_another_host_names_no_host_and_no_repository() { + for url in [ + // git url-decodes the authority, so the user ends at the `/` and + // ssh connects to evil.com. + "ssh://evil.com%2F@github.com/acme/widget.git", + "git+ssh://evil.com%2F@github.com/acme/widget.git", + "git://evil.com%2F@github.com/acme/widget.git", + "https://evil.com%2F@github.com/acme/widget.git", + // git strips the brackets and ssh reads `x@evil.com` as the host. + "[github.com:x@evil.com]:acme/widget.git", + "git@[github.com]:acme/widget.git", + ] { + assert_eq!(host(url), None, "{url}"); + assert_eq!(owner_repo(url), None, "{url}"); + } + // The controls: the same repository spelled plainly still reads. + for url in [ + "ssh://git@github.com/acme/widget.git", + "git@github.com:acme/widget.git", + "https://github.com/acme/widget.git", + "acme/widget", ] { assert_eq!( owner_repo(url), diff --git a/src/guard/names.rs b/src/guard/names.rs index fdb606d..0932da0 100644 --- a/src/guard/names.rs +++ b/src/guard/names.rs @@ -126,19 +126,6 @@ fn url_pattern() -> &'static Regex { }) } -/// Whether `gh` can answer for a name written against this host. -/// -/// Only GitHub, and only exactly GitHub. A GitHub Enterprise host is a -/// DIFFERENT forge that happens to share the software: asking github.com about -/// `acme/widget` seen on `github.acme.com` answers about someone else's -/// repository, and a public answer there passes a private repository here. -fn is_github_host(host: &str) -> bool { - matches!( - host.to_lowercase().as_str(), - "github.com" | "www.github.com" | "raw.githubusercontent.com" - ) -} - /// The hosts a policy has said carry no repository this guard must resolve. /// /// The field this replaces was a list of six forge hostnames written into the @@ -212,7 +199,7 @@ fn unanswerable_names(text: &str, quiet: &ForeignHosts) -> BTreeSet<(String, Str let mut found = BTreeSet::new(); for capture in url_pattern().captures_iter(text) { let host = capture[1].to_lowercase(); - if is_github_host(&host) || quiet.quiets(&host) { + if git::is_github_host(&host) || quiet.quiets(&host) { continue; } let repo = clean_repo(&capture[3]); @@ -241,7 +228,7 @@ fn unanswerable_names(text: &str, quiet: &ForeignHosts) -> BTreeSet<(String, Str fn candidates(text: &str, owners: &OwnerMatchers) -> BTreeSet<(String, String)> { let mut found: BTreeSet<(String, String)> = BTreeSet::new(); for capture in url_pattern().captures_iter(text) { - if !is_github_host(&capture[1]) { + if !git::is_github_host(&capture[1]) { continue; } let owner = capture[2].to_string(); diff --git a/src/shim.rs b/src/shim.rs index 8653138..6550190 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1766,12 +1766,18 @@ impl Forge { /// `github.com/acme/gitlab-tools` is on GitHub and /// `example.com:org/github-mirror` is on neither. A self-hosted forge still /// counts by its name: `gitlab.example.com` is GitLab. + /// + /// GitHub is github.com and nothing else, by [`git::is_github_host`]. A + /// GitHub Enterprise host such as `github.acme.com` is a different forge, + /// and `gh api` asked without `--hostname` answers about github.com: the + /// visibility and ownership it reported would be a same-named stranger's. + /// So an enterprise host is a host no client is asked about. pub(crate) fn of_url(url: &str) -> Option { let host = git::host(url)?; - if host.contains("gitlab") { - Some(Self::GitLab) - } else if host.contains("github") { + if git::is_github_host(&host) { Some(Self::GitHub) + } else if host.contains("gitlab") { + Some(Self::GitLab) } else { // Not a guess. An unrecognised host means no client applies, and // the caller says so rather than reporting an answer nobody read. @@ -3950,6 +3956,21 @@ mod tests { Some(Forge::GitHub), ), ("GIT@GitHub.COM:acme/widget.git", Some(Forge::GitHub)), + // GitHub Enterprise is not GitHub: `gh api` without `--hostname` + // would answer about a same-named github.com repository. + ("https://github.acme.com/acme/widget.git", None), + ("git@github.acme.com:acme/widget.git", None), + ("ssh://git@github.acme.com:2222/acme/widget.git", None), + ("https://notgithub.com/acme/widget.git", None), + // An `@` after a query or fragment is not userinfo: git connects + // to evil.com. + ("https://evil.com#@github.com/acme/widget.git", None), + ("https://evil.com?@github.com/acme/widget.git", None), + // git url-decodes the authority, strips scp-like brackets, and + // reads no host from file://, so none of these is github.com. + ("ssh://evil.com%2F@github.com/acme/widget.git", None), + ("[github.com:x@evil.com]:acme/widget.git", None), + ("file://github.com/acme/widget.git", None), ] { assert_eq!(Forge::of_url(url), want, "{url}"); } diff --git a/tests/guard_cli.rs b/tests/guard_cli.rs index 73ee6f4..cc1544c 100644 --- a/tests/guard_cli.rs +++ b/tests/guard_cli.rs @@ -1791,3 +1791,142 @@ fn a_destination_on_a_host_with_no_client_is_judged_by_the_allow_list_alone() { assert!(text.contains("someone-else/thing"), "{text}"); assert!(!text.contains("The forge"), "{text}"); } + +/// A GitHub Enterprise remote is not asked about on github.com. +/// +/// `gh api` without `--hostname` answers about github.com, so a `gh` that +/// administers `github.com/someone-else/thing` would have passed a push to +/// `github.acme.com/someone-else/thing` -- a different forge's repository. The +/// stub says yes to everything, so only a guard that never asks it refuses. +#[test] +fn an_enterprise_remote_is_not_answered_for_by_github_com() { + let root = repository(PINNED_PUSH); + gh_says( + &root, + "case \"$*\" in\n\ + 'api user --jq .login') echo someone-else ;;\n\ + 'api repos/'*' --jq .permissions.admin') echo true ;;\n\ + *) echo \"gh: unexpected call: $*\" >&2; exit 1 ;;\n\ + esac\n", + ); + + for url in [ + "https://github.acme.com/someone-else/thing.git", + "git@github.acme.com:someone-else/thing.git", + ] { + let output = push_guard(&root, &["--remote-url", url]); + assert_eq!(code(&output), 1, "{url}: {}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("someone-else/thing"), "{text}"); + assert!(!text.contains("The forge"), "{text}"); + } +} + +#[test] +fn an_at_sign_after_a_query_or_fragment_does_not_make_github_com_the_host() { + // git connects to evil.com: the `@` sits in the fragment or query, not in + // the userinfo. Reading github.com out of it let a github.com administrator + // of acme/widget push off the list to a stranger's host. + let root = repository(PINNED_PUSH); + gh_says( + &root, + "case \"$*\" in\n\ + 'api user --jq .login') echo someone-else ;;\n\ + 'api repos/'*' --jq .permissions.admin') echo true ;;\n\ + *) echo \"gh: unexpected call: $*\" >&2; exit 1 ;;\n\ + esac\n", + ); + + for url in [ + "https://evil.com#@github.com/acme/widget.git", + "https://evil.com?@github.com/acme/widget.git", + ] { + let output = push_guard(&root, &["--remote-url", url]); + assert_eq!(code(&output), 1, "{url}: {}", stderr(&output)); + let text = stderr(&output); + assert!(!text.contains("The forge"), "{text}"); + } +} + +/// A push to `url`, which git sends somewhere other than github.com, is +/// refused although a stub `gh` says the pusher administers github.com's +/// acme/widget. +fn refused_though_github_com_says_yes(url: &str) { + let root = repository(PINNED_PUSH); + gh_says( + &root, + "case \"$*\" in\n\ + 'api user --jq .login') echo someone-else ;;\n\ + 'api repos/'*' --jq .permissions.admin') echo true ;;\n\ + *) echo \"gh: unexpected call: $*\" >&2; exit 1 ;;\n\ + esac\n", + ); + let output = push_guard(&root, &["--remote-url", url]); + assert_eq!(code(&output), 1, "{url}: {}", stderr(&output)); + let text = stderr(&output); + assert!(!text.contains("The forge"), "{text}"); +} + +#[test] +fn a_percent_encoded_authority_does_not_make_github_com_the_host() { + // git url-decodes the authority before splitting it, so the user ends at + // the `/` and ssh connects to evil.com. + refused_though_github_com_says_yes("ssh://evil.com%2F@github.com/acme/widget.git"); +} + +#[test] +fn a_bracketed_scp_like_host_does_not_make_github_com_the_host() { + // git strips the brackets and ssh connects to evil.com as user x. + refused_though_github_com_says_yes("[github.com:x@evil.com]:acme/widget.git"); +} + +/// A `file://` url names no host, so no forge is asked about it. +/// +/// git pushes `file://github.com/...` to a local path, so github.com's answer +/// about the repository its path names is an answer about somewhere else. The +/// allow-list alone judges it: an owner not on it is refused, and the `gh` +/// that would have said yes is never run. +#[test] +fn a_file_url_does_not_make_github_com_the_host() { + let root = repository(PINNED_PUSH); + let asked = root.join("gh-was-asked"); + gh_says( + &root, + &format!( + "touch '{}'\n\ + case \"$*\" in\n\ + 'api user --jq .login') echo someone-else ;;\n\ + 'api repos/'*' --jq .permissions.admin') echo true ;;\n\ + *) echo \"gh: unexpected call: $*\" >&2; exit 1 ;;\n\ + esac\n", + asked.display() + ), + ); + let url = "file://github.com/someone-else/thing.git"; + let output = push_guard(&root, &["--remote-url", url]); + assert_eq!(code(&output), 1, "{url}: {}", stderr(&output)); + assert!(!asked.exists(), "gh was asked about {url}"); +} + +/// A `file://` url to a bare repository is read by its path, as the same path +/// spelled plainly is, so the allow-list admits it. +#[test] +fn a_file_url_to_an_allowed_bare_repository_passes() { + let root = repository( + "[rule.prevent-public-push]\nbuiltin = \"prevent-public-push\"\n\ + allowed_repos = [\"x/bare\"]\n\n\ + [rule.prevent-public-push.git]\nhooks = [\"pre-push\"]\n", + ); + gh_says(&root, GH_SAYS_NO); + let bare = root.join("x/bare.git"); + std::fs::create_dir_all(&bare).unwrap(); + support::git(&bare, &["init", "-q", "--bare"]); + + for url in [ + format!("file://{}", bare.display()), + bare.display().to_string(), + ] { + let output = push_guard(&root, &["--remote-url", &url]); + assert_eq!(code(&output), 0, "{url}: {}", stderr(&output)); + } +}