Fix GitHub token auto discovery (#1335)

* Fix GitHub token auto discovery

Fixed #1333

 - Rm dep `gh-token` since it is broken and we can simply run
   `gh auth token` in `cargo-binstall` instead.
 - binstalk-downloader: Make sure GitHub token is at least 40B long
   and other than the `_`, composes of only alphanumeric characters.
 - Warn on failure to read `git/credential` files
 - Optimize `try_from_home` to avoid heap allocation of `PathBuf`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Fix typo and clippy

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Simplify `is_valid_gh_token` & `is_ascii_alphanumeric` impl

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Improve err msg in `get_inner`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

* Improve err msg of `cargo_binstall::gh_token::get`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

---------

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Cargo.lock

@@ -527,7 +527,6 @@ dependencies = [
  "dirs",
  "embed-resource",
  "file-format",
- "gh-token",
  "home",
  "log",
  "miette",
@@ -1169,18 +1168,6 @@ dependencies = [
  "wasi",
 ]
 
-[[package]]
-name = "gh-token"
-version = "0.1.4"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c29796559a0962994fcc5994ff97a18e68618508d3f0d8de794475a5045caf09"
-dependencies = [
- "home",
- "serde",
- "serde_derive",
- "serde_yaml",
-]
-
 [[package]]
 name = "gimli"
 version = "0.28.0"
@@ -3363,19 +3350,6 @@ dependencies = [
  "serde",
 ]
 
-[[package]]
-name = "serde_yaml"
-version = "0.9.25"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1a49e178e4452f45cb61d0cd8cebc1b0fafd3e41929e996cef79aa3aca91f574"
-dependencies = [
- "indexmap 2.0.0",
- "itoa",
- "ryu",
- "serde",
- "unsafe-libyaml",
-]
-
 [[package]]
 name = "sha1"
 version = "0.10.5"
@@ -4028,12 +4002,6 @@ version = "0.1.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c0edd1e5b14653f783770bce4a4dabb4a5108a5370a5f5d8cfe8710c361f6c8b"
 
-[[package]]
-name = "unsafe-libyaml"
-version = "0.2.9"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f28467d3e1d3c6586d8f25fa243f544f5800fec42d97032474e17222c2b75cfa"
-
 [[package]]
 name = "untrusted"
 version = "0.7.1"

crates/bin/Cargo.toml

@@ -28,7 +28,6 @@ clap = { version = "4.3.0", features = ["derive", "env"] }
 compact_str = "0.7.0"
 dirs = "5.0.1"
 file-format = { version = "0.19.0", default-features = false }
-gh-token = "0.1.2"
 home = "0.5.5"
 log = { version = "0.4.18", features = ["std"] }
 miette = "5.9.0"

crates/bin/src/args.rs

@@ -538,12 +538,6 @@ You cannot use --{option} and specify multiple packages at the same time. Do one
     if opts.github_token.is_none() {
         if let Ok(github_token) = env::var("GH_TOKEN") {
             opts.github_token = Some(github_token.into());
-        } else if !opts.no_discover_github_token {
-            if let Some(github_token) = crate::git_credentials::try_from_home() {
-                opts.github_token = Some(github_token);
-            } else if let Ok(github_token) = gh_token::get() {
-                opts.github_token = Some(github_token.into());
-            }
         }
     }
 

crates/bin/src/entry.rs

@@ -33,7 +33,7 @@ use tracing::{debug, error, info, warn};
 
 use crate::{
     args::{Args, Strategy},
-    install_path,
+    gh_token, git_credentials, install_path,
     ui::confirm,
 };
 
@@ -107,7 +107,16 @@ pub fn install_crates(
     )
     .map_err(BinstallError::from)?;
 
-    let gh_api_client = GhApiClient::new(client.clone(), args.github_token);
+    let gh_api_client = GhApiClient::new(
+        client.clone(),
+        args.github_token.or_else(|| {
+            if args.no_discover_github_token {
+                None
+            } else {
+                git_credentials::try_from_home().or_else(gh_token::get)
+            }
+        }),
+    );
 
     // Create binstall_opts
     let binstall_opts = Arc::new(Options {

crates/bin/src/gh_token.rs

@@ -0,0 +1,38 @@
+use std::{io, process};
+
+use compact_str::CompactString;
+use tracing::warn;
+
+fn get_inner() -> io::Result<CompactString> {
+    let process::Output { status, stdout, .. } = process::Command::new("gh")
+        .args(["auth", "token"])
+        .stdin(process::Stdio::null())
+        .stdout(process::Stdio::piped())
+        .stderr(process::Stdio::null())
+        .output()?;
+
+    if !status.success() {
+        return Err(io::Error::new(
+            io::ErrorKind::Other,
+            format!("process exited with `{status}`"),
+        ));
+    }
+
+    // Use String here instead of CompactString here since
+    // `CompactString::from_utf8` allocates if it's longer than 24B.
+    let s = String::from_utf8(stdout).map_err(|_err| {
+        io::Error::new(io::ErrorKind::InvalidData, "Invalid output, expected utf8")
+    })?;
+
+    Ok(s.trim().into())
+}
+
+pub(super) fn get() -> Option<CompactString> {
+    match get_inner() {
+        Ok(token) => Some(token),
+        Err(err) => {
+            warn!(?err, "Failed to retrieve token from `gh auth token`");
+            None
+        }
+    }
+}

crates/bin/src/git_credentials.rs

@@ -1,10 +1,8 @@
-use std::{
-    env, fs,
-    path::{Path, PathBuf},
-};
+use std::{env, fs, path::PathBuf};
 
 use compact_str::CompactString;
 use dirs::home_dir;
+use tracing::warn;
 
 pub fn try_from_home() -> Option<CompactString> {
     if let Some(mut home) = home_dir() {
@@ -15,7 +13,9 @@ pub fn try_from_home() -> Option<CompactString> {
     }
 
     if let Some(home) = env::var_os("XDG_CONFIG_HOME") {
-        let home = Path::new(&home).join("git/credentials");
+        let mut home = PathBuf::from(home);
+        home.push("git/credentials");
+
         if let Some(cred) = from_file(home) {
             return Some(cred);
         }
@@ -25,8 +25,7 @@ pub fn try_from_home() -> Option<CompactString> {
 }
 
 fn from_file(path: PathBuf) -> Option<CompactString> {
-    fs::read_to_string(path)
-        .ok()?
+    read_cred_file(path)?
         .lines()
         .find_map(from_line)
         .map(CompactString::from)
@@ -41,6 +40,20 @@ fn from_line(line: &str) -> Option<&str> {
     Some(cred.split_once(':')?.1)
 }
 
+fn read_cred_file(path: PathBuf) -> Option<String> {
+    match fs::read_to_string(&path) {
+        Ok(s) => Some(s),
+        Err(err) => {
+            warn!(
+                ?err,
+                "Failed to read git credential file {}",
+                path.display()
+            );
+            None
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::*;

crates/bin/src/lib.rs

@@ -3,6 +3,7 @@
 mod args;
 mod bin_util;
 mod entry;
+mod gh_token;
 mod git_credentials;
 mod install_path;
 mod logging;

crates/binstalk-downloader/src/gh_api_client.rs

@@ -132,19 +132,29 @@ struct Inner {
 #[derive(Clone, Debug)]
 pub struct GhApiClient(Arc<Inner>);
 
-fn gh_prefixed(token: &str) -> bool {
-    matches!((token.get(0..2), token.get(3..4)), (Some("gh"), Some("_")))
-        || token.starts_with("github_")
+fn is_ascii_alphanumeric(s: &[u8]) -> bool {
+    s.iter().all(|byte| byte.is_ascii_alphanumeric())
+}
+
+fn is_valid_gh_token(token: &str) -> bool {
+    let token = token.as_bytes();
+
+    token.len() >= 40
+        && ((&token[0..2] == b"gh"
+            && token[2].is_ascii_alphanumeric()
+            && token[3] == b'_'
+            && is_ascii_alphanumeric(&token[4..]))
+            || (token.starts_with(b"github_") && is_ascii_alphanumeric(&token[7..])))
 }
 
 impl GhApiClient {
     pub fn new(client: remote::Client, auth_token: Option<CompactString>) -> Self {
         let auth_token = auth_token.and_then(|auth_token| {
-            if gh_prefixed(&auth_token) {
+            if is_valid_gh_token(&auth_token) {
                 debug!("Using gh api token");
                 Some(auth_token)
             } else {
-                warn!("Invalid auth_token, expected 'gh*_' or `github_*`, fallback to unauthorized mode");
+                warn!("Invalid auth_token, expected 'gh*_' or `github_*` with [A-Za-z0-9], fallback to unauthorized mode");
                 None
             }
         });