fix(auth): invalidate token caches after login#796
Conversation
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the authentication flow by proactively clearing cached authentication tokens and timezone information upon successful user login. This ensures that subsequent API calls utilize fresh credentials and accurate timezone data, improving the reliability and consistency of the application's interactions with Google Workspace services. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to invalidate token caches and timezone data upon successful login, ensuring that stale credentials are not reused. It also refactors the service account token cache path into a dedicated helper and adds a unit test for the cache invalidation process. A security improvement was suggested to sanitize error messages in the invalidate_token_caches function to prevent terminal escape sequence injection.
| std::fs::remove_file(&path).map_err(|e| { | ||
| GwsError::Validation(format!("Failed to remove {}: {e}", path.display())) | ||
| })?; |
There was a problem hiding this comment.
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
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
- Rust's
char::is_control()only covers theCc(Control) Unicode category, notCf(Format). Therefore,is_dangerous_unicodeis not redundant and is needed to check for dangerous Unicode characters. - 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.
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic token cache invalidation during the login process and refactors the logout logic to use a shared file removal helper. These changes ensure that stale credentials and cached timezones are cleared upon successful authentication. The review feedback focuses on making cache invalidation a "best-effort" operation to prevent non-critical file system errors from blocking the login or logout flows. Specifically, it is recommended to avoid early termination in the logout loop to ensure all sensitive credential files are processed even if one removal fails, and to update the function signatures and tests accordingly.
| 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 remove_file_if_exists(&path)? { | ||
| removed.push(path.display().to_string()); | ||
| } | ||
| } | ||
|
|
||
| Ok(removed) | ||
| } |
There was a problem hiding this comment.
Token cache invalidation is a maintenance task that should be performed on a "best-effort" basis. Hard-failing the entire command if a cache file cannot be removed (e.g., due to file locks or permission issues) can prevent users from logging in even when their credentials are valid. It is better to handle errors internally and allow the process to continue. Note: Potential Time-of-check to time-of-use (TOCTOU) race conditions are a known limitation of this file-path-based approach.
fn invalidate_token_caches() -> Vec<String> {
let mut removed = Vec::new();
for path in [token_cache_path(), service_account_token_cache_path()] {
// Best-effort removal: ignore errors so login/logout can proceed.
if let Ok(true) = remove_file_if_exists(&path) {
removed.push(path.display().to_string());
}
}
removed
}References
- Acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation in file path operations.
| // 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()?; |
There was a problem hiding this comment.
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();| 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())) | ||
| })?; | ||
| if remove_file_if_exists(path)? { | ||
| removed.push(path.display().to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
- Acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation in file path operations.
| std::fs::write(&token_cache, "{}").unwrap(); | ||
| std::fs::write(&sa_token_cache, "{}").unwrap(); | ||
|
|
||
| let removed = invalidate_token_caches().unwrap(); |
There was a problem hiding this comment.
Update the test case to match the suggested non-failing signature of invalidate_token_caches.
let removed = invalidate_token_caches();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces token cache invalidation during the login process to ensure that stale credentials are not reused when switching accounts or scopes. It also refactors file removal logic to mitigate TOCTOU races and adds unit tests for the new invalidation logic. A security concern was raised regarding the logout command, which now only issues a warning instead of failing when it cannot remove sensitive credential files, potentially leaving the system in an insecure state without notifying the user.
| 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) => eprintln!("Warning: {e}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
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());
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements token cache invalidation during login and logout by adding a remove_file_if_exists helper and a new invalidate_token_caches function. It also updates the logout logic to aggregate file deletion errors and adds a unit test for cache invalidation. Feedback was provided regarding the thread-safety of using std::env::set_var in tests, noting that while the new test is serialized, potential data races remain if other tests in the suite are not similarly protected.
| } | ||
|
|
||
| #[test] | ||
| #[serial_test::serial] |
There was a problem hiding this comment.
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
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
2 participants
Closes #764.
Closes #780.
Summary
Testing