Skip to content
Closed
5 changes: 5 additions & 0 deletions .changeset/add-no-localhost-flag.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@googleworkspace/cli": minor
---

feat(auth): add `--no-localhost` flag to `gws auth login`
176 changes: 157 additions & 19 deletions src/auth_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ pub async fn handle_auth_command(args: &[String]) -> Result<(), GwsError> {
" --scopes Comma-separated custom scopes\n",
" -s, --services Comma-separated service names to limit scope picker\n",
" (e.g. -s drive,gmail,sheets)\n",
" --no-localhost Use out-of-band flow instead of starting a local server\n",
" setup Configure GCP project + OAuth client (requires gcloud)\n",
" --project Use a specific GCP project\n",
" status Show current authentication state\n",
Expand Down Expand Up @@ -183,7 +184,7 @@ impl yup_oauth2::authenticator_delegate::InstalledFlowDelegate for CliFlowDelega
fn present_user_url<'a>(
&'a self,
url: &'a str,
_need_code: bool,
need_code: bool,
) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<String, String>> + Send + 'a>>
{
Box::pin(async move {
Expand All @@ -204,22 +205,59 @@ impl yup_oauth2::authenticator_delegate::InstalledFlowDelegate for CliFlowDelega
};
eprintln!("Open this URL in your browser to authenticate:\n");
eprintln!(" {display_url}\n");
Ok(String::new())

if need_code {
eprintln!("Enter the authorization code (or paste the full redirect URL):");
let mut user_input = String::new();
std::io::stdin()
.read_line(&mut user_input)
.map_err(|e| format!("Failed to read code: {e}"))?;
Comment on lines +211 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of std::io::stdin().read_line() here is a blocking I/O operation inside an async block. This can block the entire worker thread of the async runtime, leading to poor performance and unresponsiveness, especially if other async tasks are pending. It's crucial to avoid blocking calls in async code.

To fix this, you should move the blocking operation to a dedicated thread pool using tokio::task::spawn_blocking.

Suggested change
let mut user_input = String::new();
std::io::stdin()
.read_line(&mut user_input)
.map_err(|e| format!("Failed to read code: {e}"))?;
let user_input = tokio::task::spawn_blocking(|| {
let mut line = String::new();
std::io::stdin().read_line(&mut line).map(|_| line)
})
.await
.map_err(|e| format!("Failed to join blocking task: {e}"))?
.map_err(|e| format!("Failed to read code: {e}"))?;

Comment on lines +211 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The use of std::io::stdin().read_line() within an async block is a blocking operation. This can block the Tokio runtime's worker thread, preventing other asynchronous tasks from running and potentially causing your application to hang, especially if the user takes a long time to provide input.

To fix this, you should perform the blocking I/O operation on a dedicated thread pool using tokio::task::spawn_blocking.

Suggested change
let mut user_input = String::new();
std::io::stdin()
.read_line(&mut user_input)
.map_err(|e| format!("Failed to read code: {e}"))?;
let user_input = tokio::task::spawn_blocking(|| {
let mut input = String::new();
std::io::stdin().read_line(&mut input).map(|_| input)
})
.await
.map_err(|e| format!("Task join error: {e}"))?
.map_err(|e| format!("Failed to read code: {e}"))?;

Comment on lines +211 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of std::io::stdin().read_line() within an async block is a blocking operation that can starve the Tokio runtime executor, preventing other asynchronous tasks from running. This can lead to performance degradation or even deadlocks in the application.

To fix this, you should perform the blocking I/O operation on a dedicated blocking thread using tokio::task::spawn_blocking.

Suggested change
let mut user_input = String::new();
std::io::stdin()
.read_line(&mut user_input)
.map_err(|e| format!("Failed to read code: {e}"))?;
let user_input = tokio::task::spawn_blocking(|| {
let mut input = String::new();
std::io::stdin().read_line(&mut input).map(|_| input)
})
.await
.map_err(|e| format!("Task join error: {e}"))?
.map_err(|e| format!("Failed to read code: {e}"))?;

Comment on lines +212 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using std::io::stdin() for I/O within an async block is a blocking operation. This can block the current thread and prevent other asynchronous tasks from running, potentially leading to performance issues or deadlocks in more complex applications.

While this might not cause a noticeable problem in the current context of a simple CLI prompt, it is a good practice to use non-blocking alternatives in async code.

Consider one of the following approaches:

  1. Wrap the blocking call in tokio::task::spawn_blocking. This moves the operation to a dedicated thread pool.
  2. Use tokio::io::stdin() along with tokio::io::AsyncBufReadExt to perform the read asynchronously.

Both solutions would require adding the necessary use statements for tokio components.


Ok(extract_code_from_input(&user_input))
} else {
Ok(String::new())
}
})
}
}

async fn handle_login(args: &[String]) -> Result<(), GwsError> {
// Extract --account and -s/--services from args
/// Extracts the authorization code from user input. If the input is a full URL,
/// it parses the URL and extracts the `code` query parameter. Otherwise, it
/// assumes the entire input (trimmed) is the authorization code.
fn extract_code_from_input(input: &str) -> String {
let input = input.trim();
if let Ok(parsed_url) = reqwest::Url::parse(input) {
for (k, v) in parsed_url.query_pairs() {
if k == "code" {
return v.to_string();
}
}
}
input.to_string()
}

struct LoginArgs {
account_email: Option<String>,
services_filter: Option<HashSet<String>>,
no_localhost: bool,
filtered_args: Vec<String>,
}

fn parse_login_args(args: &[String]) -> LoginArgs {
let mut account_email: Option<String> = None;
let mut services_filter: Option<HashSet<String>> = None;
let mut no_localhost = false;
let mut filtered_args: Vec<String> = Vec::new();
let mut skip_next = false;
for i in 0..args.len() {
if skip_next {
skip_next = false;
continue;
}
if args[i] == "--no-localhost" {
no_localhost = true;
continue;
}
Comment on lines +257 to +260
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The manual argument parsing logic in this function has a bug that affects this new flag. If a user provides an argument-taking flag like --account without a value, followed by --no-localhost, the --no-localhost flag will be incorrectly consumed as the value for --account.

For example: gws auth login --account --no-localhost

This will result in account_email being Some("--no-localhost") and this no_localhost flag being ignored, leading to confusing errors.

The root cause is that argument-taking flags don't check if the next token is another flag. While a full refactor to use clap for subcommand arguments is a larger change, the current parser should be made more robust to prevent this kind of incorrect behavior.

A potential fix for the --account flag (and similar flags) would be to check that the value is not a flag itself:

if args[i] == "--account" && i + 1 < args.len() && !args[i + 1].starts_with('-') {
    // ...
}

Since this change would be outside the diff, I'm pointing out the issue here on the new code that is impacted by it.

if args[i] == "--account" && i + 1 < args.len() {
account_email = Some(args[i + 1].clone());
skip_next = true;
Expand Down Expand Up @@ -249,6 +287,21 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> {
filtered_args.push(args[i].clone());
}

LoginArgs {
account_email,
services_filter,
no_localhost,
filtered_args,
}
}

async fn handle_login(args: &[String]) -> Result<(), GwsError> {
let parsed = parse_login_args(args);
let account_email = parsed.account_email;
let services_filter = parsed.services_filter;
let no_localhost = parsed.no_localhost;
let filtered_args = parsed.filtered_args;

// Resolve client_id and client_secret:
// 1. Env vars (highest priority)
// 2. Saved client_secret.json from `gws auth setup` or manual download
Expand Down Expand Up @@ -278,12 +331,24 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> {
// are already included.
let mut scopes = filter_redundant_restrictive_scopes(scopes);

let redirect_uris = if no_localhost {
vec![
"urn:ietf:wg:oauth:2.0:oob".to_string(),
"http://localhost".to_string(),
]
} else {
vec![
"http://localhost".to_string(),
"urn:ietf:wg:oauth:2.0:oob".to_string(),
]
};
Comment on lines +334 to +344
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This block of code appears to have a copy-paste error, resulting in duplicated logic and a syntax error that will prevent compilation. The if no_localhost block is defined twice, and the first definition incorrectly includes http://localhost in the redirect_uris for the --no-localhost case. The logic should be consolidated into a single, correct if/else block.

    let redirect_uris = if no_localhost {
        vec!["urn:ietf:wg:oauth:2.0:oob".to_string()]
    } else {
        vec!["http://localhost".to_string()]
    };


let secret = yup_oauth2::ApplicationSecret {
client_id: client_id.clone(),
client_secret: client_secret.clone(),
auth_uri: "https://accounts.google.com/o/oauth2/auth".to_string(),
token_uri: "https://oauth2.googleapis.com/token".to_string(),
redirect_uris: vec!["http://localhost".to_string()],
redirect_uris,
..Default::default()
};

Expand All @@ -310,20 +375,23 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> {
.map_err(|e| GwsError::Validation(format!("Failed to create config directory: {e}")))?;
}

let auth = yup_oauth2::InstalledFlowAuthenticator::builder(
secret,
yup_oauth2::InstalledFlowReturnMethod::HTTPRedirect,
)
.with_storage(Box::new(crate::token_storage::EncryptedTokenStorage::new(
temp_path.clone(),
)))
.force_account_selection(true) // Adds prompt=consent so Google always returns a refresh_token
.flow_delegate(Box::new(CliFlowDelegate {
login_hint: account_email.clone(),
}))
.build()
.await
.map_err(|e| GwsError::Auth(format!("Failed to build authenticator: {e}")))?;
let return_method = if no_localhost {
yup_oauth2::InstalledFlowReturnMethod::Interactive
} else {
yup_oauth2::InstalledFlowReturnMethod::HTTPRedirect
};

let auth = yup_oauth2::InstalledFlowAuthenticator::builder(secret, return_method)
.with_storage(Box::new(crate::token_storage::EncryptedTokenStorage::new(
temp_path.clone(),
)))
.force_account_selection(true) // Adds prompt=consent so Google always returns a refresh_token
.flow_delegate(Box::new(CliFlowDelegate {
login_hint: account_email.clone(),
}))
.build()
.await
.map_err(|e| GwsError::Auth(format!("Failed to build authenticator: {e}")))?;

// Request a token — this triggers the browser OAuth flow
let scope_refs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
Expand Down Expand Up @@ -2184,4 +2252,74 @@ mod tests {
// Exactly 9 chars — first 4 + last 4 with "..." in between
assert_eq!(mask_secret("123456789"), "1234...6789");
}

#[test]
fn test_extract_code_from_input_raw_code() {
assert_eq!(extract_code_from_input("4/0Aea..."), "4/0Aea...");
assert_eq!(extract_code_from_input(" 4/0Aea... \n"), "4/0Aea...");
}

#[test]
fn test_extract_code_from_input_url() {
assert_eq!(
extract_code_from_input("http://localhost/?code=4/0Aea...&scope=email"),
"4/0Aea..."
);
assert_eq!(
extract_code_from_input("urn:ietf:wg:oauth:2.0:oob?code=my-secret-code"),
"my-secret-code"
);
}

#[test]
fn test_extract_code_from_input_url_no_code() {
assert_eq!(
extract_code_from_input("http://localhost/?error=access_denied"),
"http://localhost/?error=access_denied"
);
}

#[test]
fn test_parse_login_args_no_localhost() {
let args = vec![
"--no-localhost".to_string(),
"--scopes".to_string(),
"drive".to_string(),
];
let parsed = parse_login_args(&args);
assert!(parsed.no_localhost);
assert_eq!(parsed.filtered_args, vec!["--scopes", "drive"]);
}

#[test]
fn test_parse_login_args_account() {
let args = vec![
"--account".to_string(),
"test@example.com".to_string(),
"--no-localhost".to_string(),
];
let parsed = parse_login_args(&args);
assert_eq!(parsed.account_email.unwrap(), "test@example.com");
assert!(parsed.no_localhost);
assert!(parsed.filtered_args.is_empty());

let args2 = vec!["--account=test2@example.com".to_string()];
let parsed2 = parse_login_args(&args2);
assert_eq!(parsed2.account_email.unwrap(), "test2@example.com");
}

#[test]
fn test_parse_login_args_services() {
let args = vec!["-s".to_string(), "drive,gmail".to_string()];
let parsed = parse_login_args(&args);
let filter = parsed.services_filter.unwrap();
assert!(filter.contains("drive"));
assert!(filter.contains("gmail"));
assert!(!parsed.no_localhost);

let args2 = vec!["--services=sheets".to_string()];
let parsed2 = parse_login_args(&args2);
let filter2 = parsed2.services_filter.unwrap();
assert!(filter2.contains("sheets"));
}
}
Loading