diff --git a/crates/cargo-aprz-lib/Cargo.toml b/crates/cargo-aprz-lib/Cargo.toml index a89921fd1..a28e77712 100644 --- a/crates/cargo-aprz-lib/Cargo.toml +++ b/crates/cargo-aprz-lib/Cargo.toml @@ -172,6 +172,10 @@ required-features = ["internals"] name = "docs_provider_integration" required-features = ["internals"] +[[test]] +name = "github_credentials_process_integration" +required-features = ["internals"] + [[test]] name = "hosting_provider_integration" required-features = ["internals"] diff --git a/crates/cargo-aprz-lib/src/commands/common.rs b/crates/cargo-aprz-lib/src/commands/common.rs index ccd6c4529..aa0b9a6dd 100644 --- a/crates/cargo-aprz-lib/src/commands/common.rs +++ b/crates/cargo-aprz-lib/src/commands/common.rs @@ -19,6 +19,7 @@ use ohno::IntoAppError; use super::ProgressReporter; use super::cache_dir::platform_cache_dir; use super::config::Config; +use super::github_credentials::{GitHubToken, discover}; use crate::Result; use crate::expr::{ExpressionDisposition, ExpressionOutcome, Risk, evaluate}; use crate::facts::{Collector, CrateFacts, CrateRef, Endpoints, ProviderResult}; @@ -75,10 +76,20 @@ pub enum ConsoleSection { /// Common arguments shared between crates and deps commands #[derive(Args, Debug)] +#[expect(clippy::struct_excessive_bools, reason = "each bool is an independent clap CLI flag")] pub struct CommonArgs { - /// GitHub personal access token - #[arg(long, value_name = "TOKEN", env = "GITHUB_TOKEN")] - pub github_token: Option, + /// GitHub token. + /// + /// Defaults to `GITHUB_TOKEN`. If none is available, GitHub access is anonymous. + #[arg(long, value_name = "TOKEN")] + pub github_token: Option, + + /// Discover a GitHub token from the authenticated `gh` CLI. + /// + /// Used only after `--github-token` and `GITHUB_TOKEN`, for the configured + /// GitHub host. + #[arg(long)] + pub github_token_from_gh: bool, /// Codeberg personal access token #[arg(long, value_name = "TOKEN", env = "CODEBERG_TOKEN")] @@ -260,8 +271,11 @@ impl<'a, H: super::Host> Common<'a, H> { let progress_reporter = ProgressReporter::new(delay, use_colors_for_progress); + let endpoints = args.endpoints(); + let github_token = discover(args.github_token.as_ref(), args.github_token_from_gh, &endpoints).await; + let collector = Collector::new( - args.github_token.as_deref(), + github_token.as_ref().map(GitHubToken::expose_secret), args.codeberg_token.as_deref(), &cache_dir, config.crates_cache_ttl, @@ -272,7 +286,7 @@ impl<'a, H: super::Host> Common<'a, H> { args.ignore_cached, config.bug_label_matcher()?.into(), progress_reporter, - &args.endpoints(), + &endpoints, ) .await?; @@ -678,7 +692,7 @@ fn should_include_rejection_details(console_mode: Option<&ConsoleOutputMode>) -> #[cfg(test)] #[cfg(not(miri))] mod tests { - use clap::Parser; + use clap::{CommandFactory, Parser}; use semver::{Version, VersionReq}; use super::*; @@ -725,6 +739,27 @@ mod tests { assert_eq!(endpoints.advisory_url(), defaults.advisory_url()); } + #[test] + fn github_cli_token_discovery_is_visible_and_opt_in() { + let crates_default = crate::commands::CratesArgs::parse_from(["crates"]); + assert!(!crates_default.common.github_token_from_gh); + let deps_default = crate::commands::DepsArgs::parse_from(["deps"]); + assert!(!deps_default.common.github_token_from_gh); + + let crates = crate::commands::CratesArgs::parse_from(["crates", "--github-token-from-gh"]); + assert!(crates.common.github_token_from_gh); + let deps = crate::commands::DepsArgs::parse_from(["deps", "--github-token-from-gh"]); + assert!(deps.common.github_token_from_gh); + + for mut command in [crate::commands::CratesArgs::command(), crate::commands::DepsArgs::command()] { + let help = command.render_long_help().to_string(); + assert!( + help.contains("--github-token-from-gh"), + "the opt-in switch must be visible in help:\n{help}" + ); + } + } + #[test] fn test_endpoints_apply_every_override() { let parsed = ArgsHarness::parse_from([ diff --git a/crates/cargo-aprz-lib/src/commands/github_credentials.rs b/crates/cargo-aprz-lib/src/commands/github_credentials.rs new file mode 100644 index 000000000..ee55830db --- /dev/null +++ b/crates/cargo-aprz-lib/src/commands/github_credentials.rs @@ -0,0 +1,836 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Best-effort GitHub credential discovery. + +use std::convert::Infallible; +use std::ffi::{OsStr, OsString}; +use std::path::{Path, PathBuf}; +use std::process::Stdio; +use std::str::FromStr; +use std::time::Duration; +use std::{fmt, io}; + +use tokio::process::Command; +use url::Url; + +use crate::facts::Endpoints; + +const GITHUB_TOKEN_ENV: &str = "GITHUB_TOKEN"; +const GH_COMMAND_TIMEOUT: Duration = Duration::from_secs(10); +const LOG_TARGET: &str = "credentials"; + +/// A GitHub token whose debug representation never exposes its value. +#[derive(Clone, PartialEq, Eq)] +pub struct GitHubToken(String); + +impl GitHubToken { + /// Expose the token only where it is attached to the GitHub HTTP client. + pub fn expose_secret(&self) -> &str { + &self.0 + } +} + +impl fmt::Debug for GitHubToken { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("GitHubToken([REDACTED])") + } +} + +impl FromStr for GitHubToken { + type Err = Infallible; + + fn from_str(value: &str) -> Result { + Ok(Self(value.to_owned())) + } +} + +struct GhCommandOutput { + success: bool, + stdout: Vec, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum GhStdio { + Null, + Capture, +} + +impl GhStdio { + fn into_stdio(self) -> Stdio { + match self { + Self::Null => Stdio::null(), + Self::Capture => Stdio::piped(), + } + } +} + +#[derive(Debug, PartialEq, Eq)] +struct GhCommandRequest { + executable: PathBuf, + args: Vec, + stdin: GhStdio, + stdout: GhStdio, + stderr: GhStdio, + timeout: Duration, +} + +/// Resolve the GitHub credential used by the hosting provider. +pub(super) async fn discover(explicit: Option<&GitHubToken>, github_token_from_gh: bool, endpoints: &Endpoints) -> Option { + discover_with( + explicit, + github_token_from_gh, + endpoints, + || std::env::var_os(GITHUB_TOKEN_ENV), + query_gh, + ) + .await +} + +async fn discover_with( + explicit: Option<&GitHubToken>, + github_token_from_gh: bool, + endpoints: &Endpoints, + read_environment: impl FnOnce() -> Option, + query_gh: impl FnOnce(String) -> OutputFuture, +) -> Option +where + OutputFuture: Future>, +{ + if let Some(token) = explicit { + let token = token.expose_secret().trim(); + if !token.is_empty() { + log::trace!(target: LOG_TARGET, "GitHub credential source: --github-token"); + return Some(GitHubToken(token.to_owned())); + } + log::trace!(target: LOG_TARGET, "GitHub credential source --github-token is blank; continuing credential discovery"); + } + + if let Some(token) = read_environment() { + if let Ok(token) = token.into_string() { + let token = token.trim(); + if !token.is_empty() { + log::trace!(target: LOG_TARGET, "GitHub credential source: {GITHUB_TOKEN_ENV}"); + return Some(GitHubToken(token.to_owned())); + } + log::trace!( + target: LOG_TARGET, + "GitHub credential source {GITHUB_TOKEN_ENV} is blank; continuing credential discovery" + ); + } else { + log::trace!( + target: LOG_TARGET, + "GitHub credential source {GITHUB_TOKEN_ENV} is not valid UTF-8; continuing credential discovery" + ); + } + } + + if !github_token_from_gh { + log::trace!(target: LOG_TARGET, "GitHub CLI credential discovery was not requested; using anonymous access"); + return None; + } + + let Some(hostname) = github_hostname(endpoints) else { + log::trace!( + target: LOG_TARGET, + "Effective GitHub service URL has no usable hostname; using anonymous access" + ); + return None; + }; + + let output = match query_gh(hostname.clone()).await { + Ok(output) => output, + Err(error) => { + log::trace!( + target: LOG_TARGET, + "GitHub credential source gh could not be executed for host '{hostname}' ({}); using anonymous access", + error.kind() + ); + return None; + } + }; + + if !output.success { + log::trace!( + target: LOG_TARGET, + "GitHub credential source gh did not return a token for host '{hostname}'; using anonymous access" + ); + return None; + } + + let Ok(stdout) = String::from_utf8(output.stdout) else { + log::trace!( + target: LOG_TARGET, + "GitHub credential source gh returned non-UTF-8 output for host '{hostname}'; using anonymous access" + ); + return None; + }; + + let token = stdout.trim(); + if token.is_empty() { + log::trace!( + target: LOG_TARGET, + "GitHub credential source gh returned a blank token for host '{hostname}'; using anonymous access" + ); + return None; + } + + log::trace!(target: LOG_TARGET, "GitHub credential source: gh for host '{hostname}'"); + Some(GitHubToken(token.to_owned())) +} + +fn github_hostname(endpoints: &Endpoints) -> Option { + let url = Url::parse(endpoints.host_url("github.com")?).ok()?; + let hostname = url.host_str()?; + + Some(if hostname.eq_ignore_ascii_case("api.github.com") { + "github.com".to_owned() + } else { + hostname.to_owned() + }) +} + +async fn query_gh(hostname: String) -> io::Result { + query_gh_with(hostname, resolve_gh_executable, run_gh_command).await +} + +async fn query_gh_with( + hostname: String, + resolve_executable: impl FnOnce() -> ResolveFuture, + run_command: impl FnOnce(GhCommandRequest) -> RunFuture, +) -> io::Result +where + ResolveFuture: Future>, + RunFuture: Future>, +{ + let executable = resolve_executable().await?; + if !executable.is_absolute() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "resolved gh executable path was not absolute", + )); + } + + run_command(GhCommandRequest { + executable, + args: ["auth", "token", "--hostname"] + .into_iter() + .map(OsString::from) + .chain(std::iter::once(OsString::from(hostname))) + .collect(), + stdin: GhStdio::Null, + stdout: GhStdio::Capture, + stderr: GhStdio::Null, + timeout: GH_COMMAND_TIMEOUT, + }) + .await +} + +async fn run_gh_command(request: GhCommandRequest) -> io::Result { + let GhCommandRequest { + executable, + args, + stdin, + stdout, + stderr, + timeout, + } = request; + let child = Command::new(executable) + .args(args) + // Rejected blank values must not make gh ignore its authenticated account. + .env_remove(GITHUB_TOKEN_ENV) + .stdin(stdin.into_stdio()) + .stdout(stdout.into_stdio()) + .stderr(stderr.into_stdio()) + .kill_on_drop(true) + .spawn()?; + let output = tokio::time::timeout(timeout, child.wait_with_output()) + .await + .map_err(|elapsed| io::Error::new(io::ErrorKind::TimedOut, elapsed))??; + + Ok(GhCommandOutput { + success: output.status.success(), + stdout: output.stdout, + }) +} + +async fn resolve_gh_executable() -> io::Result { + resolve_executable_async(resolve_gh_from_environment).await +} + +async fn resolve_executable_async(resolver: impl FnOnce() -> Option + Send + 'static) -> io::Result { + tokio::task::spawn_blocking(resolver) + .await + .map_err(io::Error::other)? + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "gh was not found on PATH")) +} + +fn resolve_gh_from_environment() -> Option { + let path = std::env::var_os("PATH")?; + let current_dir = std::env::current_dir().ok()?; + let path_ext = std::env::var_os("PATHEXT"); + + resolve_executable(OsStr::new("gh"), &path, path_ext.as_deref(), ¤t_dir) +} + +/// Resolves `program` only from explicit, non-empty PATH entries. +fn resolve_executable(program: &OsStr, path: &OsStr, path_ext: Option<&OsStr>, current_dir: &Path) -> Option { + debug_assert!(current_dir.is_absolute(), "the process current directory is absolute"); + let executable_names = executable_names(program, path_ext); + + std::env::split_paths(path) + // An empty component can make OS command lookup search CWD implicitly. Requiring an + // actual entry such as `.` keeps repository-local executables opt-in. + .filter(|directory| !directory.as_os_str().is_empty()) + .map(|directory| { + if directory.is_absolute() { + directory + } else { + current_dir.join(directory) + } + }) + .flat_map(|directory| executable_names.iter().map(move |name| directory.join(name))) + .find(|candidate| is_executable(candidate)) +} + +#[cfg(windows)] +fn executable_names(program: &OsStr, path_ext: Option<&OsStr>) -> Vec { + const DEFAULT_PATH_EXT: &str = ".COM;.EXE;.BAT;.CMD"; + + if Path::new(program).extension().is_some() { + return windows_executable_image_extension(Path::new(program).extension().unwrap_or_default()) + .then(|| program.to_owned()) + .into_iter() + .collect(); + } + + let path_ext = path_ext + .filter(|value| !value.is_empty()) + .unwrap_or_else(|| OsStr::new(DEFAULT_PATH_EXT)); + std::env::split_paths(path_ext) + .filter(|extension| !extension.as_os_str().is_empty()) + .filter(|extension| windows_executable_image_extension(extension.as_os_str())) + .map(|extension| { + let extension = extension.as_os_str(); + let mut executable = program.to_owned(); + if !extension.to_string_lossy().starts_with('.') { + executable.push("."); + } + executable.push(extension); + executable + }) + .collect() +} + +#[cfg(windows)] +fn windows_executable_image_extension(extension: &OsStr) -> bool { + let extension = extension.to_string_lossy(); + let extension = extension.strip_prefix('.').unwrap_or_else(|| extension.as_ref()); + extension.eq_ignore_ascii_case("com") || extension.eq_ignore_ascii_case("exe") +} + +#[cfg(not(windows))] +fn executable_names(program: &OsStr, _path_ext: Option<&OsStr>) -> Vec { + vec![program.to_owned()] +} + +#[cfg(unix)] +fn is_executable(candidate: &Path) -> bool { + use std::os::unix::fs::PermissionsExt as _; + + candidate + .metadata() + .is_ok_and(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) +} + +#[cfg(not(unix))] +fn is_executable(candidate: &Path) -> bool { + candidate.is_file() +} + +#[cfg(test)] +#[cfg(not(miri))] +mod tests { + use std::cell::{Cell, RefCell}; + use std::fs; + use std::time::Instant; + + use super::*; + + fn token(value: &str) -> GitHubToken { + value.parse().expect("GitHubToken parsing is infallible") + } + + fn successful(stdout: &[u8]) -> GhCommandOutput { + GhCommandOutput { + success: true, + stdout: stdout.to_vec(), + } + } + + #[tokio::test] + async fn explicit_token_precedes_environment_and_gh() { + let environment_read = Cell::new(false); + let gh_called = Cell::new(false); + let explicit = token("explicit-secret"); + + let selected = discover_with( + Some(&explicit), + true, + &Endpoints::default(), + || { + environment_read.set(true); + Some(OsString::from("environment-secret")) + }, + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await + .expect("the explicit token is selected"); + + assert_eq!(selected.expose_secret(), "explicit-secret"); + assert!(!environment_read.get(), "an explicit token suppresses environment lookup"); + assert!(!gh_called.get(), "an explicit token suppresses gh"); + } + + #[tokio::test] + async fn blank_explicit_token_continues_credential_discovery() { + let explicit = token(" \r\n\t "); + let gh_called = Cell::new(false); + let selected = discover_with( + Some(&explicit), + true, + &Endpoints::default(), + || Some(OsString::from("environment-secret")), + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await + .expect("the environment token is selected after a blank explicit token"); + + assert_eq!(selected.expose_secret(), "environment-secret"); + assert!(!gh_called.get(), "the environment token still suppresses gh"); + } + + #[tokio::test] + async fn environment_token_precedes_gh() { + let gh_called = Cell::new(false); + + let selected = discover_with( + None, + true, + &Endpoints::default(), + || Some(OsString::from("environment-secret")), + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await + .expect("the environment token is selected"); + + assert_eq!(selected.expose_secret(), "environment-secret"); + assert!(!gh_called.get(), "an environment token suppresses gh"); + } + + #[tokio::test] + async fn default_off_never_queries_gh_for_absent_or_blank_environment_tokens() { + for environment in [None, Some(""), Some(" \r\n\t ")] { + let gh_called = Cell::new(false); + + let selected = discover_with( + None, + false, + &Endpoints::default(), + || environment.map(OsString::from), + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await; + + assert!(selected.is_none()); + assert!(!gh_called.get(), "gh must remain disabled for environment value {environment:?}"); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn non_utf8_environment_token_continues_to_gh_when_enabled() { + use std::os::unix::ffi::OsStringExt as _; + + let gh_called = Cell::new(false); + let selected = discover_with( + None, + true, + &Endpoints::default(), + || Some(OsString::from_vec(vec![0xff])), + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await + .expect("an unusable environment token falls through to gh"); + + assert_eq!(selected.expose_secret(), "gh-secret"); + assert!(gh_called.get()); + } + + #[tokio::test] + async fn blank_environment_tokens_continue_to_gh_when_enabled() { + for environment in ["", " \r\n\t "] { + let gh_called = Cell::new(false); + + let selected = discover_with( + None, + true, + &Endpoints::default(), + || Some(OsString::from(environment)), + |_| { + gh_called.set(true); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await + .expect("a blank environment token falls through to gh"); + + assert_eq!(selected.expose_secret(), "gh-secret"); + assert!(gh_called.get(), "gh is queried for a blank environment token"); + } + } + + #[tokio::test] + async fn gh_uses_the_enterprise_hostname_and_trims_stdout() { + let requested_hostname = RefCell::new(None); + let endpoints = Endpoints::default().with_github_url("https://github.example.test/api/v3"); + + let selected = discover_with( + None, + true, + &endpoints, + || None, + |hostname| { + requested_hostname.replace(Some(hostname)); + std::future::ready(Ok(successful(b" gh-secret\r\n"))) + }, + ) + .await + .expect("gh returned a token"); + + assert_eq!(requested_hostname.borrow().as_deref(), Some("github.example.test")); + assert_eq!(selected.expose_secret(), "gh-secret"); + } + + #[tokio::test] + async fn public_api_uses_the_github_com_login() { + let requested_hostname = RefCell::new(None); + + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |hostname| { + requested_hostname.replace(Some(hostname)); + std::future::ready(Ok(successful(b"gh-secret"))) + }, + ) + .await; + + assert!(selected.is_some()); + assert_eq!(requested_hostname.borrow().as_deref(), Some("github.com")); + } + + #[tokio::test] + async fn command_not_found_continues_anonymously() { + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |_| std::future::ready(Err(io::Error::new(io::ErrorKind::NotFound, "test gh is absent"))), + ) + .await; + + assert!(selected.is_none()); + } + + #[tokio::test] + async fn timed_out_command_continues_anonymously() { + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |_| std::future::ready(Err(io::Error::new(io::ErrorKind::TimedOut, "test gh lookup expired"))), + ) + .await; + + assert!(selected.is_none()); + } + + #[tokio::test] + async fn unsuccessful_command_continues_anonymously_without_exposing_stdout() { + let secret = "failed-command-secret"; + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |_| { + std::future::ready(Ok(GhCommandOutput { + success: false, + stdout: secret.as_bytes().to_vec(), + })) + }, + ) + .await; + + assert!(selected.is_none()); + } + + #[tokio::test] + async fn blank_command_output_continues_anonymously() { + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |_| std::future::ready(Ok(successful(b" \r\n\t "))), + ) + .await; + + assert!(selected.is_none()); + } + + #[tokio::test] + async fn non_utf8_command_output_continues_anonymously() { + let selected = discover_with( + None, + true, + &Endpoints::default(), + || None, + |_| std::future::ready(Ok(successful(&[0xff, 0xfe]))), + ) + .await; + + assert!(selected.is_none()); + } + + #[tokio::test] + async fn query_gh_builds_the_production_process_request() { + let root = tempfile::tempdir().expect("creating a command fixture"); + let executable = root.path().join(test_gh_name()); + let observed = RefCell::new(None); + + let output = query_gh_with( + "github.example.test".to_owned(), + || std::future::ready(Ok(executable.clone())), + |request| { + observed.replace(Some(request)); + std::future::ready(Ok(successful(b"fixture-token\n"))) + }, + ) + .await + .expect("the injected command runner succeeds"); + + assert!(output.success); + assert_eq!(output.stdout, b"fixture-token\n"); + assert_eq!( + observed.into_inner(), + Some(GhCommandRequest { + executable, + args: ["auth", "token", "--hostname", "github.example.test"] + .into_iter() + .map(OsString::from) + .collect(), + stdin: GhStdio::Null, + stdout: GhStdio::Capture, + stderr: GhStdio::Null, + timeout: GH_COMMAND_TIMEOUT, + }) + ); + } + + #[tokio::test] + async fn command_timeout_is_reported_and_does_not_wait_for_the_child() { + let executable = std::env::current_exe().expect("the test harness executable has an absolute path"); + let module = module_path!().split_once("::").map_or(module_path!(), |(_, module)| module); + let fixture = format!("{module}::command_timeout_child_fixture"); + let started = Instant::now(); + + let Err(error) = run_gh_command(GhCommandRequest { + executable, + args: ["--ignored", "--exact"] + .into_iter() + .map(OsString::from) + .chain(std::iter::once(OsString::from(fixture))) + .collect(), + stdin: GhStdio::Null, + stdout: GhStdio::Capture, + stderr: GhStdio::Null, + timeout: Duration::from_millis(50), + }) + .await + else { + panic!("the child exceeds the test timeout"); + }; + + assert_eq!(error.kind(), io::ErrorKind::TimedOut); + assert!( + started.elapsed() < Duration::from_secs(5), + "the timed-out child was awaited instead of terminated" + ); + } + + #[test] + #[ignore = "subprocess fixture for command_timeout_is_reported_and_does_not_wait_for_the_child"] + #[cfg_attr(coverage_nightly, coverage(off))] + fn command_timeout_child_fixture() { + std::thread::sleep(Duration::from_secs(30)); + } + + #[test] + fn path_resolution_prefers_path_over_a_planted_cwd_executable() { + let root = tempfile::tempdir().expect("creating a resolver fixture"); + let current_dir = root.path().join("project"); + let path_dir = root.path().join("bin"); + fs::create_dir_all(¤t_dir).expect("creating the project directory"); + fs::create_dir_all(&path_dir).expect("creating the PATH directory"); + write_test_executable(¤t_dir.join(test_gh_name())); + let expected = path_dir.join(test_gh_name()); + write_test_executable(&expected); + let path = std::env::join_paths([&path_dir]).expect("the fixture PATH is valid"); + + let resolved = resolve_test_executable(&path, ¤t_dir); + + assert_eq!(resolved.as_deref(), Some(expected.as_path())); + } + + #[test] + fn path_resolution_does_not_search_cwd_implicitly() { + let root = tempfile::tempdir().expect("creating a resolver fixture"); + let current_dir = root.path().join("project"); + fs::create_dir_all(¤t_dir).expect("creating the project directory"); + write_test_executable(¤t_dir.join(test_gh_name())); + + let resolved = resolve_test_executable(OsStr::new(""), ¤t_dir); + + assert!(resolved.is_none()); + } + + #[test] + fn path_resolution_honors_an_explicit_cwd_entry() { + let root = tempfile::tempdir().expect("creating a resolver fixture"); + let current_dir = root.path().join("project"); + fs::create_dir_all(¤t_dir).expect("creating the project directory"); + let expected = current_dir.join(test_gh_name()); + write_test_executable(&expected); + + let resolved = resolve_test_executable(OsStr::new("."), ¤t_dir); + + assert_eq!(resolved.as_deref(), Some(current_dir.join(".").join(test_gh_name()).as_path())); + } + + #[cfg(windows)] + #[test] + fn path_resolution_uses_pathext_order() { + let root = tempfile::tempdir().expect("creating a resolver fixture"); + let current_dir = root.path().join("project"); + let path_dir = root.path().join("bin"); + fs::create_dir_all(¤t_dir).expect("creating the project directory"); + fs::create_dir_all(&path_dir).expect("creating the PATH directory"); + write_test_executable(&path_dir.join("gh.EXE")); + let expected = path_dir.join("gh.COM"); + write_test_executable(&expected); + let path = std::env::join_paths([&path_dir]).expect("the fixture PATH is valid"); + + let resolved = resolve_executable(OsStr::new("gh"), &path, Some(OsStr::new(".CMD;.COM;.EXE")), ¤t_dir); + + assert_eq!(resolved.as_deref(), Some(expected.as_path())); + } + + #[cfg(windows)] + #[test] + fn path_resolution_rejects_batch_scripts() { + let root = tempfile::tempdir().expect("creating a resolver fixture"); + let current_dir = root.path().join("project"); + let path_dir = root.path().join("bin"); + fs::create_dir_all(¤t_dir).expect("creating the project directory"); + fs::create_dir_all(&path_dir).expect("creating the PATH directory"); + write_test_executable(&path_dir.join("gh.CMD")); + write_test_executable(&path_dir.join("gh.BAT")); + let path = std::env::join_paths([&path_dir]).expect("the fixture PATH is valid"); + + let resolved = resolve_executable(OsStr::new("gh"), &path, Some(OsStr::new(".CMD;.BAT")), ¤t_dir); + + assert!(resolved.is_none()); + } + + #[tokio::test(flavor = "current_thread")] + async fn executable_resolution_does_not_block_the_async_worker() { + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let resolution = tokio::spawn(resolve_executable_async(move || { + let _ = started_tx.send(Instant::now()); + std::thread::sleep(Duration::from_millis(400)); + None + })); + + let started_at = started_rx.await.expect("the blocking resolver starts"); + assert!( + started_at.elapsed() < Duration::from_millis(200), + "the async worker was blocked by executable resolution" + ); + + let error = resolution + .await + .expect("the resolution task does not panic") + .expect_err("the fixture resolver returns no executable"); + assert_eq!(error.kind(), io::ErrorKind::NotFound); + } + + #[test] + fn token_debug_output_is_redacted() { + let secret = "diagnostic-secret"; + let diagnostic = format!("{:?}", token(secret)); + + assert!(!diagnostic.contains(secret)); + assert_eq!(diagnostic, "GitHubToken([REDACTED])"); + } + + #[cfg(windows)] + fn test_gh_name() -> &'static str { + "gh.EXE" + } + + #[cfg(not(windows))] + fn test_gh_name() -> &'static str { + "gh" + } + + fn resolve_test_executable(path: &OsStr, current_dir: &Path) -> Option { + #[cfg(windows)] + let path_ext = Some(OsStr::new(".EXE")); + #[cfg(not(windows))] + let path_ext = None; + + resolve_executable(OsStr::new("gh"), path, path_ext, current_dir) + } + + fn write_test_executable(path: &Path) { + fs::write(path, b"resolver fixture").expect("writing the resolver fixture"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).expect("making the resolver fixture executable"); + } + } +} diff --git a/crates/cargo-aprz-lib/src/commands/mod.rs b/crates/cargo-aprz-lib/src/commands/mod.rs index 7ff0cc73b..364835d0d 100644 --- a/crates/cargo-aprz-lib/src/commands/mod.rs +++ b/crates/cargo-aprz-lib/src/commands/mod.rs @@ -44,6 +44,7 @@ mod config; mod crates; mod deps; mod duration; +mod github_credentials; mod host; mod init; mod progress_reporter; diff --git a/crates/cargo-aprz-lib/tests/github_credentials_process_integration.rs b/crates/cargo-aprz-lib/tests/github_credentials_process_integration.rs new file mode 100644 index 000000000..eacbeb3b8 --- /dev/null +++ b/crates/cargo-aprz-lib/tests/github_credentials_process_integration.rs @@ -0,0 +1,197 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! End-to-end coverage for GitHub CLI credential discovery. +//! +//! This binary deliberately contains one test because it temporarily changes +//! process-global environment variables used by executable discovery. + +#![cfg(not(miri))] + +mod support; + +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use chrono::Utc; +use serde_json::json; +use support::dump::Dump; +use support::{TestHost, dump_server, dump_url, failing_server, seed_advisory_db}; +use url::Url; +use wiremock::matchers::{header, method, path}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +const DISCOVERED_TOKEN: &str = "integration-discovered-secret"; + +fn compile_fake_gh(bin: &Path) -> PathBuf { + const SOURCE: &str = r#" +use std::env; +use std::fs; + +fn main() { + let args = env::args().skip(1).collect::>(); + let log = format!( + "inherited-token={}\nargs={}", + env::var_os("GITHUB_TOKEN").is_some(), + args.join("|") + ); + fs::write(env::var_os("FAKE_GH_LOG").expect("log path"), log) + .expect("write fake gh log"); + println!("integration-discovered-secret"); +} +"#; + + std::fs::create_dir_all(bin).expect("creating fake executable directory"); + let source = bin.join("fake-gh.rs"); + std::fs::write(&source, SOURCE).expect("writing fake gh source"); + let executable = bin.join(if cfg!(windows) { "gh.exe" } else { "gh" }); + let rustc = std::env::var_os("RUSTC").unwrap_or_else(|| OsString::from("rustc")); + let output = Command::new(rustc) + .args(["--edition=2024", "-o"]) + .arg(&executable) + .arg(source) + .output() + .expect("rustc is available while running Rust integration tests"); + assert!( + output.status.success(), + "compiling fake gh failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + executable +} + +#[test] +fn run_discovers_an_enterprise_token_through_the_production_process_path() { + let temp = tempfile::tempdir().expect("creating test directory"); + let bin = temp.path().join("bin"); + compile_fake_gh(&bin); + let gh_log = temp.path().join("gh.log"); + let path = std::env::join_paths(std::iter::once(bin).chain(std::env::split_paths(&std::env::var_os("PATH").unwrap_or_default()))) + .expect("the fake gh directory and inherited PATH form a valid search path"); + let mut command = Command::new(std::env::current_exe().expect("the integration test knows its executable")); + command + .args([ + "--ignored", + "--exact", + "helper_run_discovers_an_enterprise_token_through_the_production_process_path", + "--nocapture", + ]) + .env("PATH", path) + .env("GITHUB_TOKEN", " \t ") + .env("FAKE_GH_LOG", &gh_log); + if cfg!(windows) { + command.env("PATHEXT", ".COM;.EXE;.BAT;.CMD"); + } + + let output = command.output().expect("start isolated credential scenario"); + assert!( + output.status.success(), + "isolated credential scenario failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + String::from_utf8_lossy(&output.stdout).contains("production credential path passed"), + "isolated scenario did not report completion" + ); + let gh_call = std::fs::read_to_string(gh_log).expect("the production path invokes fake gh"); + assert!( + gh_call.contains("inherited-token=false"), + "gh inherited the rejected blank token:\n{gh_call}" + ); + assert!( + !gh_call.contains(DISCOVERED_TOKEN), + "the token appeared in gh arguments or test diagnostics" + ); +} + +#[test] +#[ignore = "subprocess fixture for run_discovers_an_enterprise_token_through_the_production_process_path"] +fn helper_run_discovers_an_enterprise_token_through_the_production_process_path() { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("create isolated runtime") + .block_on(run_credential_scenario()); + println!("production credential path passed"); +} + +async fn run_credential_scenario() { + let dump = dump_server(Dump::sample(Utc::now()).to_tar_gz(), Some(1)).await; + let github = MockServer::start().await; + let services = failing_server(404).await; + let temp = tempfile::tempdir().expect("creating test directory"); + let cache = temp.path().join("cache"); + seed_advisory_db(&cache); + + for request_path in [ + "/repos/fake-org/schemeless-repo-crate", + "/repos/fake-org/schemeless-repo-crate/issues", + ] { + let body = if request_path.ends_with("/issues") { + json!([]) + } else { + json!({ + "stargazers_count": 1, + "forks_count": 2, + "subscribers_count": 3, + }) + }; + Mock::given(method("GET")) + .and(path(request_path)) + .and(header("authorization", format!("token {DISCOVERED_TOKEN}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .expect(1) + .mount(&github) + .await; + } + + let github_url = github.uri(); + let dump_url = dump_url(&dump); + let service_url = services.uri(); + let cache = cache.to_string_lossy().into_owned(); + let args = [ + "cargo", + "aprz", + "crates", + "schemeless-repo-crate@0.1.0", + "--console", + "--color", + "never", + "--cache-dir", + &cache, + "--dump-url", + &dump_url, + "--docs-url", + &service_url, + "--coverage-url", + &service_url, + "--github-url", + &github_url, + "--github-token-from-gh", + "--codeberg-url", + &service_url, + "--advisory-url", + &service_url, + ]; + let mut host = TestHost::new(); + + cargo_aprz_lib::run(&mut host, args).await; + + assert!(host.exit_code.is_none(), "the command should succeed: {}", host.error_str()); + let expected_hostname = Url::parse(&github_url) + .expect("wiremock URI is valid") + .host_str() + .expect("wiremock URI has a host") + .to_owned(); + let gh_call = std::fs::read_to_string(std::env::var_os("FAKE_GH_LOG").expect("fake gh log configured")) + .expect("the production path invokes fake gh"); + assert!( + gh_call.contains(&format!("args=auth|token|--hostname|{expected_hostname}")), + "gh did not receive the Enterprise hostname:\n{gh_call}" + ); + let diagnostics = format!("{}{}", host.output_str(), host.error_str()); + assert!(!diagnostics.contains(DISCOVERED_TOKEN), "diagnostics exposed the discovered token"); +} diff --git a/crates/cargo-aprz-lib/tests/hosting_provider_integration.rs b/crates/cargo-aprz-lib/tests/hosting_provider_integration.rs index a58a96360..f8ae37f9c 100644 --- a/crates/cargo-aprz-lib/tests/hosting_provider_integration.rs +++ b/crates/cargo-aprz-lib/tests/hosting_provider_integration.rs @@ -19,7 +19,7 @@ use chrono::{DateTime, SecondsFormat, Utc}; use semver::Version; use serde_json::json; use url::Url; -use wiremock::matchers::{method, path, query_param}; +use wiremock::matchers::{header, method, path, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; /// No-op progress reporter for testing @@ -210,6 +210,64 @@ async fn derives_hosting_data_from_a_github_repository() { assert_eq!(data.open_pr_age.p50, 5); } +#[tokio::test] +#[cfg_attr(miri, ignore = "Miri cannot call CreateIoCompletionPort")] +async fn scopes_credentials_to_their_host_without_leaking_them_in_errors() { + let server = MockServer::start().await; + let github_token = "github-diagnostic-secret"; + let codeberg_token = "codeberg-diagnostic-secret"; + + Mock::given(method("GET")) + .and(path("/repos/github-owner/private")) + .and(header("authorization", format!("token {github_token}"))) + .respond_with(ResponseTemplate::new(400)) + .expect(1) + .mount(&server) + .await; + + Mock::given(method("GET")) + .and(path("/repos/codeberg-owner/public")) + .and(header("authorization", format!("token {codeberg_token}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "stars_count": 3, + "forks_count": 2, + "watchers_count": 1, + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/codeberg-owner/public/issues")) + .and(header("authorization", format!("token {codeberg_token}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(Vec::::new())) + .expect(1) + .mount(&server) + .await; + + let endpoints = Endpoints::default().with_github_url(server.uri()).with_codeberg_url(server.uri()); + let cache_dir = tempfile::tempdir().expect("creating a cache directory"); + let provider = Provider::new( + Some(github_token), + Some(codeberg_token), + cache_in(cache_dir.path(), false), + bug_matcher(), + &endpoints, + ) + .expect("credential headers are valid"); + + let github_error = expect_error(fetch_one(&provider, spec_for("private", "github.com", "github-owner", "private")).await); + assert!(!github_error.contains(github_token), "GitHub errors must redact credentials"); + assert!( + !github_error.contains(codeberg_token), + "GitHub errors must not contain Codeberg credentials" + ); + + let codeberg_data = expect_found(fetch_one(&provider, spec_for("public", "codeberg.org", "codeberg-owner", "public")).await); + assert_eq!(codeberg_data.stars, 3); + assert_eq!(codeberg_data.forks, 2); + assert_eq!(codeberg_data.subscribers, 1); +} + #[tokio::test] #[cfg_attr(miri, ignore = "Miri cannot call CreateIoCompletionPort")] async fn consumes_every_page_advertised_by_the_link_header() { diff --git a/crates/cargo-aprz-lib/tests/process_globals_integration.rs b/crates/cargo-aprz-lib/tests/process_globals_integration.rs index 6344de153..a1dd0f341 100644 --- a/crates/cargo-aprz-lib/tests/process_globals_integration.rs +++ b/crates/cargo-aprz-lib/tests/process_globals_integration.rs @@ -65,6 +65,8 @@ async fn cli_uses_the_platform_cache_directory_and_installs_a_logger() { &service_uri, "--github-url", &service_uri, + "--github-token", + "integration-github-token", "--codeberg-url", &service_uri, "--advisory-url", diff --git a/crates/cargo-aprz-lib/tests/support/mod.rs b/crates/cargo-aprz-lib/tests/support/mod.rs index 8d5c4658c..7db865350 100644 --- a/crates/cargo-aprz-lib/tests/support/mod.rs +++ b/crates/cargo-aprz-lib/tests/support/mod.rs @@ -159,6 +159,8 @@ impl MockWorld { service_uri.clone(), "--github-url".to_owned(), service_uri.clone(), + "--github-token".to_owned(), + "integration-github-token".to_owned(), "--codeberg-url".to_owned(), service_uri, "--color".to_owned(), diff --git a/crates/cargo-aprz/README.md b/crates/cargo-aprz/README.md index c36d11763..4cc96c47c 100644 --- a/crates/cargo-aprz/README.md +++ b/crates/cargo-aprz/README.md @@ -167,14 +167,28 @@ results in very low rate limits. If `cargo-aprz` detects it is being throttled b to try the operation again. When using the `deps` command on a large project, it’s likely you’ll hit these rate limits, which can make the process take hours to complete fully. -In such a case, you can provide a GitHub or Codeberg token on the command-line or through environment variables, which gives you substantially higher +In such a case, you can provide a GitHub or Codeberg token on the command line or through environment variables, which gives you substantially higher rate limits. ```bash cargo aprz deps --github-token --codeberg-token ``` -You can also set the `GITHUB_TOKEN` and `CODEBERG_TOKEN` environment variables, which `cargo-aprz` will automatically pick up. +GitHub credentials are discovered in this order: + +1. `--github-token` +1. `GITHUB_TOKEN` +1. when `--github-token-from-gh` is present, the token reported by + `gh auth token --hostname ` +1. anonymous access + +The host passed to `gh` comes from the effective GitHub service URL, including +`--github-url` or `APRZ_GITHUB_URL` overrides for GitHub Enterprise. Missing, +blank, or non-UTF-8 credentials and a `gh` lookup that exceeds its finite +deadline are ignored and retain the existing anonymous rate-limit behavior. +Without `--github-token-from-gh`, `cargo-aprz` never searches for or invokes +`gh`. Codeberg credentials continue to use `--codeberg-token` or +`CODEBERG_TOKEN`. ### Reports diff --git a/crates/cargo-aprz/docs/DESIGN.md b/crates/cargo-aprz/docs/DESIGN.md index 8e07da68b..5758c57c2 100644 --- a/crates/cargo-aprz/docs/DESIGN.md +++ b/crates/cargo-aprz/docs/DESIGN.md @@ -59,6 +59,54 @@ service continues to use its default. Overriding an address does not change how the corresponding provider interprets the responses it receives, so a substitute service must speak the same protocol as the one it replaces. +### GitHub credential discovery + +GitHub fact collection accepts anonymous access, but a dependency appraisal can +exhaust anonymous API limits quickly. Credential resolution follows this order: + +1. the explicit `--github-token` command-line option; +2. the `GITHUB_TOKEN` environment variable; +3. when `--github-token-from-gh` is present, the output of + `gh auth token --hostname ` when the `gh` executable is available and + has an authenticated account for the configured GitHub host; +4. anonymous access. + +`` comes from the effective GitHub service address, so a +`--github-url`/`APRZ_GITHUB_URL` override can use the matching GitHub Enterprise +login rather than accidentally querying `github.com`. + +GitHub CLI discovery is explicit opt-in rather than a default fallback. Without +`--github-token-from-gh`, exhausting the explicit and environment sources +continues anonymously without resolving the effective hostname, searching +`PATH`, scheduling a blocking lookup, or starting a `gh` process. + +When enabled, the `gh` fallback is convenience, not a prerequisite. A missing +executable, missing login, nonzero `gh auth token` result, blank output, or +non-UTF-8 output continues anonymously and retains the provider's existing +rate-limit behavior. A nonblank explicit option is authoritative, as is a +nonblank environment token; cargo-aprz never invokes `gh` when either applies, +even when the switch is present. Explicit and environment values are trimmed; +empty or whitespace-only values are treated as absent so later configured +sources can continue. On Unix, an environment value can also contain non-UTF-8 +bytes; such a value is likewise unusable as an HTTP credential and treated as +absent. A rejected environment value is removed from the `gh` child environment +so the CLI can consult its authenticated account instead of treating the +environment override as authoritative. + +The command is spawned directly without a shell. Its stdout is trimmed and used +only as the request credential; it is never logged, cached, included in an +error, or inherited by unrelated child processes. Stderr from a failed +best-effort lookup is suppressed. Diagnostic tracing reports only the credential +source, never the token. Before spawning, cargo-aprz resolves `gh` to an absolute +path by scanning only explicit `PATH` entries; it does not use the process +current directory unless that directory appears in `PATH`. On Windows, only +directly executable `.COM` and `.EXE` images are considered, in `PATHEXT` +ordering; `.BAT` and `.CMD` shims are excluded so lookup never delegates +argument parsing to `cmd.exe`. Filesystem resolution runs on a blocking worker +and the child process is awaited asynchronously with a ten-second deadline. +Expiry terminates the child and continues anonymously, so credential discovery +does not block an async runtime worker indefinitely. + ## Cache storage Provider data is stored beneath a platform-specific cache root, partitioned by diff --git a/crates/cargo-aprz/src/main.rs b/crates/cargo-aprz/src/main.rs index dd5ee27f7..c3d0a83c6 100644 --- a/crates/cargo-aprz/src/main.rs +++ b/crates/cargo-aprz/src/main.rs @@ -148,14 +148,28 @@ //! to try the operation again. //! //! When using the `deps` command on a large project, it's likely you'll hit these rate limits, which can make the process take hours to complete fully. -//! In such a case, you can provide a GitHub or Codeberg token on the command-line or through environment variables, which gives you substantially higher +//! In such a case, you can provide a GitHub or Codeberg token on the command line or through environment variables, which gives you substantially higher //! rate limits. //! //! ```bash //! cargo aprz deps --github-token --codeberg-token //! ``` //! -//! You can also set the `GITHUB_TOKEN` and `CODEBERG_TOKEN` environment variables, which `cargo-aprz` will automatically pick up. +//! GitHub credentials are discovered in this order: +//! +//! 1. `--github-token` +//! 2. `GITHUB_TOKEN` +//! 3. when `--github-token-from-gh` is present, the token reported by +//! `gh auth token --hostname ` +//! 4. anonymous access +//! +//! The host passed to `gh` comes from the effective GitHub service URL, including +//! `--github-url` or `APRZ_GITHUB_URL` overrides for GitHub Enterprise. Missing, +//! blank, or non-UTF-8 credentials and a `gh` lookup that exceeds its finite +//! deadline are ignored and retain the existing anonymous rate-limit behavior. +//! Without `--github-token-from-gh`, `cargo-aprz` never searches for or invokes +//! `gh`. Codeberg credentials continue to use `--codeberg-token` or +//! `CODEBERG_TOKEN`. //! //! ## Reports //!