Skip to content

fix(auth): invalidate token caches after login #796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Lubrsy706 wants to merge 4 commits into
base: main
Choose a base branch
from
+77 −7
Open

fix(auth): invalidate token caches after login #796

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7461d5b
fix(auth): invalidate token caches after login
Lubrsy706 May 13, 2026
05a6ee2
fix(auth): sanitize cache removal errors
Lubrsy706 May 14, 2026
b6c1e85
fix(auth): make cache invalidation best effort
Lubrsy706 May 14, 2026
a94efd1
fix(auth): report incomplete logout cleanup
Lubrsy706 May 14, 2026
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
54 changes: 53 additions & 1 deletion crates/google-workspace-cli/src/auth_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,25 @@ 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 invalidate_token_caches() -> Result<Vec<String>, GwsError> {
let mut removed = Vec::new();

for path in [token_cache_path(), service_account_token_cache_path()] {
if path.exists() {
std::fs::remove_file(&path).map_err(|e| {
GwsError::Validation(format!("Failed to remove {}: {e}", path.display()))
})?;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Choose a reason for hiding this comment

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

security-high high

The error message should be sanitized before being wrapped in GwsError::Validation to prevent escape sequence injection in the terminal. Since the path or the error string itself could contain malicious sequences, it is important to ensure they are safe for terminal output. When implementing sanitization, note that Rust's char::is_control() only covers the Cc category, so is_dangerous_unicode is necessary to check for Cf (Format) characters. Additionally, acknowledge potential TOCTOU race conditions as a known limitation for this file operation.

            std::fs::remove_file(&path).map_err(|e| {
                GwsError::Validation(crate::output::sanitize_for_terminal(&format!("Failed to remove {}: {e}", path.display())))
            })?;
References
  1. Sanitize error strings printed to the terminal to prevent escape sequence injection.
  2. Rust's char::is_control() only covers the Cc (Control) Unicode category, not Cf (Format). Therefore, is_dangerous_unicode is not redundant and is needed to check for dangerous Unicode characters.
  3. When implementing file path validation, acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation if a full mitigation (e.g., using openat(O_NOFOLLOW)) is considered out of scope.

Copy link
Copy Markdown
Author

@Lubrsy706 Lubrsy706 May 14, 2026

Choose a reason for hiding this comment

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

Fixed. I added a shared remove_file_if_exists helper that sanitizes the formatted path/error before wrapping it in GwsError::Validation. It also removes files directly and treats NotFound as a no-op, avoiding the previous existence-check TOCTOU window for these cache cleanup paths.

removed.push(path.display().to_string());
}
}

Ok(removed)
}

/// Which scope set to use for login.
enum ScopeMode {
/// Use the default scopes (MINIMAL_SCOPES).
Expand Down Expand Up @@ -644,13 +663,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()?;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

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

high

Since invalidate_token_caches should be a best-effort operation, the call site should be updated to reflect the non-failing signature. This ensures that a successful login is not reported as a failure just because a temporary cache file could not be deleted.

    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,7 +1485,7 @@ 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();

Expand Down Expand Up @@ -1900,6 +1928,30 @@ mod tests {
assert!(path.starts_with(config_dir()));
}

#[test]
#[serial_test::serial]
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 14, 2026

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().unwrap();
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Choose a reason for hiding this comment

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

high

Update the test case to match the suggested non-failing signature of invalidate_token_caches.

        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