fix(auth): enforce readonly scopes by revoking stale tokens and adding client-side guard#520
Conversation
…g client-side guard When a user previously authenticated with full scopes, `gws auth login --readonly` did not actually enforce read-only access. The underlying refresh token retained the original consent grant, and Google's token endpoint ignores the scope parameter on refresh requests — so the cached token silently carried write access. This commit fixes the issue with a layered approach: - Persist the configured scope set to scopes.json on login so scope changes can be detected across sessions. - When scopes change, revoke the old refresh token via Google's revocation endpoint and clear local credential/cache files before starting the new OAuth flow. - Add a client-side scope guard that rejects write-scope API requests when the session is readonly. - Show scope_mode and configured_scopes in `gws auth status`. - Clean up scopes.json on logout. Fixes googleworkspace#168
🦋 Changeset detectedLatest commit: e5ffef7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
area: auth
area: core
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 addresses a critical security vulnerability where Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 a robust mechanism for enforcing read-only scopes by persisting scope information, revoking stale tokens on scope changes, and adding a client-side guard. The implementation is well-structured and addresses the reported issue effectively.
I have one main suggestion for improvement:
Add error handling to the save_scopes function to prevent silent failures and ensure application state consistency.
This change will make the new authentication flow more robust.
Keep both upstream's new tests (find_unmatched_services, extract_scopes_from_doc) and our scope persistence/guard tests.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for enforcing read-only scopes, which is a critical security improvement. The layered approach of persisting scopes, revoking tokens on scope change, and adding a client-side guard is well-thought-out. My review includes a suggestion to improve error handling when saving scopes to ensure the system's state is always consistent. Overall, this is a solid contribution to improving the tool's security and reliability.
Address review feedback: save_scopes now returns Result<(), GwsError> and the call site uses ? to propagate failures. This ensures scopes.json is always consistent with the actual login state.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of enforcing read-only scopes by revoking stale tokens upon scope change and adding a client-side guard. The implementation is robust and includes new unit tests. I've identified a couple of areas for improvement regarding code duplication and test structure that would enhance the maintainability and reliability of the new logic. My comments focus on refactoring to remove duplicated code and making the tests less brittle.
…ation DRY up the scope classification logic that was duplicated between is_readonly_session and check_scope_allowed. Rewrite tests to call the extracted helper directly instead of re-implementing the logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to enforce read-only scopes by revoking old tokens and adding a client-side guard. The implementation is a solid step towards fixing the security issue. I've identified a critical issue where the failure to delete old credential files is ignored, which could lead to an inconsistent state and bypass the intended scope enforcement. I've also pointed out a high-severity issue where the failure of the token revocation API call is silently ignored. Addressing these points will make the implementation more robust and secure.
Warn the user when token revocation fails (network error or non-200 response) so they know the old token may still be valid server-side. Return errors when credential/cache file removal fails (ignoring NotFound) instead of silently continuing with stale files.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a security flaw in read-only scope enforcement by revoking stale tokens when scopes change. The implementation is robust, including a client-side guard as a defense-in-depth measure. My review includes one high-severity security recommendation to prevent terminal escape sequence injection by sanitizing error output, in line with the repository's general rules.
Resolve conflict: keep upstream's userinfo.profile identity scope
addition alongside our scope-change revocation block.
Use {:?} instead of {e} when printing reqwest errors to prevent
terminal escape sequence injection from attacker-controlled error
messages.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of enforcing read-only scopes by revoking stale tokens on scope changes and adding a client-side guard. The implementation is solid, introducing mechanisms to save, load, and compare scopes across sessions, and correctly handling token revocation and local credential cleanup. I've identified one high-severity issue related to security best practices that should be addressed.
The scope guard was only in main.rs, but helper commands (+send, +triage, etc.) call auth::get_token() directly, bypassing main.rs dispatch. Move the check into get_token() so ALL code paths are protected. Remove the now-redundant check from main.rs. Verified with manual testing: - gmail +send blocked in readonly session - calendar +agenda works (uses .readonly scope) - Switching scopes.json to default unblocks write ops
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces new functionality to enforce read-only scopes by revoking stale tokens upon scope changes and adding a client-side guard. This involves saving and loading configured scopes, checking if a session is read-only, and validating requested scopes. The changes also update handle_login, handle_logout, and handle_status to incorporate scope management and add new unit tests. Review feedback highlights a critical vulnerability regarding terminal escape sequence injection when printing error messages, and several high-severity issues related to blocking the async runtime with synchronous file I/O operations, requiring conversion of file system operations and related functions to their asynchronous counterparts and proper awaiting.
- Convert save_scopes, load_saved_scopes, is_readonly_session, and check_scope_allowed to async functions using tokio::fs and atomic_write_async to avoid blocking the executor thread. - Use tokio::fs::remove_file for credential/cache cleanup in handle_login. - Sanitize reqwest error via crate::output::sanitize_for_terminal before printing to prevent terminal escape sequence injection. - Update all call sites to .await the async functions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for enforcing read-only scopes, a critical security and user experience improvement. The changes include saving scopes on login, revoking old tokens when scopes changes, and adding a client-side guard to prevent write operations in a read-only session.
My review focuses on potential performance improvements. I've identified two areas where file I/O operations are performed repeatedly, and I've provided suggestions to optimize these by caching the results of file reads. These changes will make the authentication and status-checking flows more efficient.
Rename check_scope_allowed to check_scopes_allowed accepting a slice, so get_token reads scopes.json once for the entire batch instead of once per scope.
Inline the readonly check using the already-loaded scopes instead of calling is_readonly_session() which reads scopes.json a second time.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a critical fix to enforce read-only scopes by revoking stale tokens and adding a client-side guard. The changes are well-structured, touching upon login, status, and logout functionalities to ensure consistent behavior. The addition of tests for the new scope logic is also a great improvement. My review includes one high-severity comment regarding a performance issue in the new token revocation logic.
credential_store::load_encrypted() performs synchronous file I/O and AES-256-GCM decryption. Wrap it in tokio::task::spawn_blocking to avoid stalling the async runtime during the revocation flow.
|
/gemini review |
Reverts the spawn_blocking wrapper around load_encrypted(). On macOS, the Keychain ties access permissions to the calling thread identity — spawn_blocking runs on a different thread pool worker, causing repeated Keychain permission prompts. The sync call is fine here since it's a one-time operation during login, not a hot path.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism to enforce read-only sessions by revoking stale tokens on scope changes and adding a client-side guard to block write operations. The implementation is well-structured and includes necessary changes to login, logout, and status commands, along with corresponding tests. My review identifies a minor omission in the scope classification logic that could lead to incorrect behavior in an edge case. Overall, this is a high-quality contribution that significantly improves the authentication flow's security.
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for enforcing read-only scopes, which is a great security and usability improvement. The implementation correctly identifies scope changes, revokes old tokens, and adds a client-side guard. My review focuses on improving the error handling in the new logic to make it even more robust. Specifically, I've identified two places where file loading/parsing errors are silently ignored, which could undermine the new security features under certain failure conditions. Addressing these will ensure the read-only enforcement is reliable even when configuration files are corrupted.
'profile' is a standard OpenID Connect alias for userinfo.profile and does not grant write access. Without this, --scopes=profile would incorrectly classify the session as having write access.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements robust enforcement of read-only scopes for the Google Workspace CLI. Key changes include adding a client-side guard in get_token to reject write-scope requests if the session is read-only, persisting configured scopes to a scopes.json file, and revoking stale refresh tokens with Google when scopes change during login. Additionally, the gws auth status command now displays the current scope configuration, and the scopes.json file is properly cleaned up during logout. New unit tests validate the scope persistence and read-only detection logic.
…ding - load_saved_scopes now returns Result<Option<Vec<String>>, GwsError> to distinguish missing file (Ok(None)) from corrupt file (Err). A corrupt scopes.json previously silently disabled the readonly guard. - is_readonly_session now returns Result<bool, GwsError> to propagate load errors. - Credential loading/parsing failures during revocation now warn the user instead of being silently swallowed. - All call sites updated to propagate errors with ?.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical authentication bug by enforcing read-only scopes. The implementation correctly revokes stale tokens when scopes change and adds a client-side guard to prevent unintended write operations. The changes are well-tested and improve the tool's security and reliability. I have one suggestion to improve the maintainability of the new token revocation logic.
Move the deeply nested revocation logic out of handle_login into a dedicated async helper for readability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of enforcing read-only scopes by revoking stale tokens and adding a client-side guard. The changes are well-structured and include necessary additions like persisting scopes, handling token revocation, and updating status/logout commands. My review includes one suggestion to improve error reporting for file removal failures, which will enhance usability in error scenarios.
| if let Err(e) = tokio::fs::remove_file(&enc_path).await { | ||
| if e.kind() != std::io::ErrorKind::NotFound { | ||
| return Err(GwsError::Auth(format!( | ||
| "Failed to remove old credentials file: {e}. Please remove it manually." | ||
| ))); | ||
| } | ||
| } | ||
| if let Err(e) = tokio::fs::remove_file(token_cache_path()).await { | ||
| if e.kind() != std::io::ErrorKind::NotFound { | ||
| return Err(GwsError::Auth(format!( | ||
| "Failed to remove old token cache: {e}. Please remove it manually." | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The error messages for failing to remove the old credentials and token cache files should include the file paths. This will help users with manual cleanup if the automatic removal fails by telling them exactly which file to remove.
if let Err(e) = tokio::fs::remove_file(&enc_path).await {
if e.kind() != std::io::ErrorKind::NotFound {
return Err(GwsError::Auth(format!(
"Failed to remove old credentials file '{}': {e}. Please remove it manually.",
enc_path.display()
)));
}
}
let token_path = token_cache_path();
if let Err(e) = tokio::fs::remove_file(&token_path).await {
if e.kind() != std::io::ErrorKind::NotFound {
return Err(GwsError::Auth(format!(
"Failed to remove old token cache '{}': {e}. Please remove it manually.",
token_path.display()
)));
}
}2 participants
Summary
Fixes #168.
gws auth login --readonlydoesn't actually enforce read-only access when the user previously logged in with broader scopes. The refresh token keeps its original grants, and Google ignores thescopeparam on refresh — so the token silently has write access.What this PR does
scopes.jsonon login so we can detect scope changes laterauth::get_tokenso helpers like+sendare also covered)scope_modeingws auth statusfor transparencyWhy just
prompt=consentisn't enoughGoogle's consent screen shows previously-granted scopes pre-checked. Users click "Allow" and unknowingly re-grant broad access. Revoking the token first removes the prior grant entirely.
Test plan
cargo clippy -- -D warningscleangws gmail +sendin readonly session → blocked with clear error:"This operation requires scope '...' (write access), but the current session uses read-only scopes."gws calendar +agendain readonly session → works (uses.readonlyscope)gws drive files listin readonly session → blocked (Discovery doc's first scope is broaddrive)gws auth status→ showsscope_mode: readonlyandconfigured_scopesscopes.jsonto default (write) scopes →gws gmail +send --dry-runpasses scope guardgws auth login --readonly→gws auth login(full) → prints"Scopes changed — revoked previous credentials."and re-authenticates with full scopesgws auth logout→scopes.jsonlisted inremovedand confirmed deleted