fix(auth): enforce readonly scopes by revoking stale tokens and adding client-side guard#520
Conversation
🦋 Changeset detectedLatest commit: 505a52a 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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/gemini review |
1 similar comment
|
/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.
|
/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.
|
/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.
|
/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()
)));
}
}
|
|
gerfalcon
force-pushed
the
fix/issue-168-readonly-scope-enforcement
branch
from
e5ffef7 to
b1b3e53
Compare
…g client-side guard Rebased on main after workspace refactor (googleworkspace#613). All changes now target crates/google-workspace-cli/src/. - Persist configured scopes to scopes.json on login - Revoke old refresh token when scopes change (extracted into attempt_token_revocation helper) - Client-side scope guard in auth::get_token blocks write ops in readonly sessions (covers helpers like +send) - load_saved_scopes returns Result to surface corrupt scopes.json - Show scope_mode in auth status - Clean up scopes.json on logout - Sanitize error output via sanitize_for_terminal - Add 'profile' as non-write scope alias Fixes googleworkspace#168
gerfalcon
force-pushed
the
fix/issue-168-readonly-scope-enforcement
branch
from
b1b3e53 to
505a52a
Compare
|
Thank you for raising it @jpoehnelt! Resolved the conflicts and all the unit and manual tests passed. |
3 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