Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 77 additions & 7 deletions crates/google-workspace-cli/src/auth_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

use std::collections::HashSet;
use std::io::{BufRead, BufReader, Write};
use std::io::{BufRead, BufReader, ErrorKind, Write};
use std::net::TcpListener;
use std::path::{Path, PathBuf};

Expand Down Expand Up @@ -345,6 +345,36 @@ fn token_cache_path() -> PathBuf {
config_dir().join("token_cache.json")
}

fn service_account_token_cache_path() -> PathBuf {
config_dir().join("sa_token_cache.json")
}

fn remove_file_if_exists(path: &Path) -> Result<bool, GwsError> {
// Remove directly instead of checking existence first; a pre-check would
// still be subject to the usual filesystem TOCTOU race.
match std::fs::remove_file(path) {
Ok(()) => Ok(true),
Err(e) if e.kind() == ErrorKind::NotFound => Ok(false),
Err(e) => Err(GwsError::Validation(crate::output::sanitize_for_terminal(
&format!("Failed to remove {}: {e}", path.display()),
))),
}
}

fn invalidate_token_caches() -> Vec<String> {
let mut removed = Vec::new();

for path in [token_cache_path(), service_account_token_cache_path()] {
match remove_file_if_exists(&path) {
Ok(true) => removed.push(path.display().to_string()),
Ok(false) => {}
Err(e) => eprintln!("Warning: {e}"),
}
}

removed
}

/// Which scope set to use for login.
enum ScopeMode {
/// Use the default scopes (MINIMAL_SCOPES).
Expand Down Expand Up @@ -644,13 +674,22 @@ async fn handle_login_inner(
let enc_path = credential_store::save_encrypted(&creds_str)
.map_err(|e| GwsError::Auth(format!("Failed to encrypt credentials: {e}")))?;

// A successful login may change the active account or granted scopes.
// Remove cached access tokens so the next API call mints a token from the
// newly saved refresh token instead of reusing stale credentials.
let invalidated_token_caches = invalidate_token_caches();

// Invalidate cached account timezone (may belong to the previous account).
crate::timezone::invalidate_cache();

let output = json!({
"status": "success",
"message": "Authentication successful. Encrypted credentials saved.",
"account": actual_email.as_deref().unwrap_or("(unknown)"),
"credentials_file": enc_path.display().to_string(),
"encryption": "AES-256-GCM (key in OS keyring or local `.encryption_key`; set GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file for headless)",
"scopes": scopes,
"invalidated_token_caches": invalidated_token_caches,
});
println!(
"{}",
Expand Down Expand Up @@ -1457,22 +1496,29 @@ fn handle_logout() -> Result<(), GwsError> {
let plain_path = plain_credentials_path();
let enc_path = credential_store::encrypted_credentials_path();
let token_cache = token_cache_path();
let sa_token_cache = config_dir().join("sa_token_cache.json");
let sa_token_cache = service_account_token_cache_path();

let mut removed = Vec::new();
let mut failures = Vec::new();

for path in [&enc_path, &plain_path, &token_cache, &sa_token_cache] {
if path.exists() {
std::fs::remove_file(path).map_err(|e| {
GwsError::Validation(format!("Failed to remove {}: {e}", path.display()))
})?;
removed.push(path.display().to_string());
match remove_file_if_exists(path) {
Ok(true) => removed.push(path.display().to_string()),
Ok(false) => {}
Err(e) => failures.push(e.to_string()),
}
}
Comment on lines 1504 to 1510
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

In the logout flow, the command should attempt to remove all specified credential and cache files even if one removal fails. Using the ? operator causes the loop to terminate early, which might leave sensitive plaintext credentials on disk if a subsequent cache file removal fails. It is safer to continue the loop and report any errors as warnings. Note: Potential Time-of-check to time-of-use (TOCTOU) race conditions are a known limitation of this file-path-based approach.

    for path in [&enc_path, &plain_path, &token_cache, &sa_token_cache] {
        match remove_file_if_exists(path) {
            Ok(true) => removed.push(path.display().to_string()),
            Ok(false) => {},
            Err(e) => {
                // Log the error but continue to ensure other files are removed.
                eprintln!("Warning: {}", e);
            }
        }
    }
References
  1. Acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation in file path operations.

Comment on lines 1504 to 1510
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

The logout command should be definitive and reliable. By catching errors from remove_file_if_exists and only printing a warning, the command may return a success status even if sensitive credentials or token caches were not actually removed (e.g., due to permission issues or file locks). This is a regression from the previous implementation which correctly failed on such errors. For a security-sensitive operation like logout, it is critical to return an error to ensure the user or calling script is aware that the logout was not fully successful. Additionally, the success message returned at the end of the function would be misleading if some files failed to be removed.

    for path in [&enc_path, &plain_path, &token_cache, &sa_token_cache] {
        if remove_file_if_exists(path)? {
            removed.push(path.display().to_string());
        }
    }


// Invalidate cached account timezone (may belong to old account)
crate::timezone::invalidate_cache();

if !failures.is_empty() {
return Err(GwsError::Validation(format!(
"Logout incomplete. Failed to remove one or more credential/cache files: {}",
failures.join("; ")
)));
}

let output = if removed.is_empty() {
json!({
"status": "success",
Expand Down Expand Up @@ -1900,6 +1946,30 @@ mod tests {
assert!(path.starts_with(config_dir()));
}

#[test]
#[serial_test::serial]
Copy link
Copy Markdown
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::env::set_var in a multi-threaded test environment is inherently thread-unsafe and can lead to data races. While this test is marked with #[serial_test::serial], other tests in this file (such as line 2048) are not. To avoid scope creep, ensure the current changes are safe and consider a separate PR for a broader fix across the test suite.

References
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.

fn invalidate_token_caches_removes_user_and_service_account_caches() {
let dir = tempfile::tempdir().unwrap();
unsafe {
std::env::set_var("GOOGLE_WORKSPACE_CLI_CONFIG_DIR", dir.path());
}

let token_cache = token_cache_path();
let sa_token_cache = service_account_token_cache_path();
std::fs::write(&token_cache, "{}").unwrap();
std::fs::write(&sa_token_cache, "{}").unwrap();

let removed = invalidate_token_caches();

assert_eq!(removed.len(), 2);
assert!(!token_cache.exists());
assert!(!sa_token_cache.exists());

unsafe {
std::env::remove_var("GOOGLE_WORKSPACE_CLI_CONFIG_DIR");
}
}

#[tokio::test]
async fn handle_auth_command_empty_args_prints_usage() {
let args: Vec<String> = vec![];
Expand Down
Loading