-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(auth): enforce readonly scopes by revoking stale tokens and adding client-side guard #520
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.
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
gerfalcon
wants to merge
15
commits into
googleworkspace:main
Choose a base branch
from
gerfalcon:fix/issue-168-readonly-scope-enforcement
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+224
−2
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
14d3fd8
fix(auth): enforce readonly scopes by revoking stale tokens and addin…
gerfalcon e15724f
merge: resolve conflict with upstream/main
gerfalcon 80b47e7
fix(auth): propagate save_scopes errors instead of silently ignoring
gerfalcon ec96752
refactor(auth): extract is_non_write_scope helper to eliminate duplic…
gerfalcon 14a2dfe
fix(auth): handle revocation and file removal errors properly
gerfalcon b834833
merge upstream/main and use Debug format for revocation error
gerfalcon 85a0f2f
fix(auth): move scope guard into auth::get_token for full coverage
gerfalcon 27ef1e1
refactor(auth): convert to async I/O and sanitize error output
gerfalcon fd5f68b
perf(auth): batch scope check to avoid reading scopes.json per scope
gerfalcon ca37cc6
perf(auth): reuse loaded scopes in handle_status to avoid double read
gerfalcon 6b321d4
perf(auth): offload credential decryption to blocking thread pool
gerfalcon 39b43d4
revert: remove spawn_blocking for credential decryption
gerfalcon cb1a586
fix(auth): add 'profile' as a non-write scope alias
gerfalcon 9f735c6
fix(auth): surface errors from corrupt scopes.json and credential loa…
gerfalcon e5ffef7
refactor(auth): extract attempt_token_revocation helper
gerfalcon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@googleworkspace/cli": patch | ||
| --- | ||
|
|
||
| fix(auth): enforce readonly scopes by revoking stale tokens on scope change and adding client-side guard | ||
|
|
||
| Fixes #168 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,80 @@ fn token_cache_path() -> PathBuf { | |
| config_dir().join("token_cache.json") | ||
| } | ||
|
|
||
| fn scopes_path() -> PathBuf { | ||
| config_dir().join("scopes.json") | ||
| } | ||
|
|
||
| /// Save the configured scope set so scope changes can be detected across sessions. | ||
| async fn save_scopes(scopes: &[String]) -> Result<(), GwsError> { | ||
| let json = serde_json::to_string_pretty(scopes) | ||
| .map_err(|e| GwsError::Validation(format!("Failed to serialize scopes: {e}")))?; | ||
| crate::fs_util::atomic_write_async(&scopes_path(), json.as_bytes()) | ||
| .await | ||
| .map_err(|e| GwsError::Validation(format!("Failed to save scopes file: {e}")))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Load the previously saved scope set, if any. | ||
| /// | ||
| /// Returns `Ok(None)` if `scopes.json` does not exist, `Ok(Some(...))` on | ||
| /// success, or `Err` if the file exists but is unreadable or contains invalid | ||
| /// JSON. This ensures a corrupt file is surfaced as an error rather than | ||
| /// silently disabling the readonly guard. | ||
| pub async fn load_saved_scopes() -> Result<Option<Vec<String>>, GwsError> { | ||
| let path = scopes_path(); | ||
| match tokio::fs::read_to_string(&path).await { | ||
| Ok(data) => { | ||
| let scopes: Vec<String> = serde_json::from_str(&data).map_err(|e| { | ||
| GwsError::Validation(format!("Failed to parse {}: {e}", path.display())) | ||
| })?; | ||
| Ok(Some(scopes)) | ||
| } | ||
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), | ||
| Err(e) => Err(GwsError::Validation(format!( | ||
| "Failed to read {}: {e}", | ||
| path.display() | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| /// Returns true if a scope does not grant write access (identity or .readonly scopes). | ||
| fn is_non_write_scope(scope: &str) -> bool { | ||
| scope.ends_with(".readonly") | ||
| || scope == "openid" | ||
| || scope.starts_with("https://www.googleapis.com/auth/userinfo.") | ||
| || scope == "email" | ||
| || scope == "profile" | ||
| } | ||
|
|
||
| /// Returns true if the saved scopes are all read-only. | ||
| /// | ||
| /// Propagates errors from `load_saved_scopes` (corrupt file). | ||
| /// Returns `false` if no scopes file exists. | ||
| pub async fn is_readonly_session() -> Result<bool, GwsError> { | ||
| Ok(load_saved_scopes() | ||
| .await? | ||
| .map(|scopes| scopes.iter().all(|s| is_non_write_scope(s))) | ||
| .unwrap_or(false)) | ||
| } | ||
|
|
||
| /// Check if the requested scopes are compatible with the current session. | ||
| /// | ||
| /// In a readonly session, write-scope requests are rejected with a clear error. | ||
| /// Reads `scopes.json` once for the entire batch. | ||
| pub async fn check_scopes_allowed(scopes: &[&str]) -> Result<(), GwsError> { | ||
| if is_readonly_session().await? { | ||
| if let Some(scope) = scopes.iter().find(|s| !is_non_write_scope(s)) { | ||
| return Err(GwsError::Auth(format!( | ||
| "This operation requires scope '{}' (write access), but the current session \ | ||
| uses read-only scopes. Run `gws auth login` (without --readonly) to upgrade.", | ||
| scope | ||
| ))); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Handle `gws auth <subcommand>`. | ||
| pub async fn handle_auth_command(args: &[String]) -> Result<(), GwsError> { | ||
| const USAGE: &str = concat!( | ||
|
|
@@ -210,6 +284,63 @@ impl yup_oauth2::authenticator_delegate::InstalledFlowDelegate for CliFlowDelega | |
| } | ||
| } | ||
|
|
||
| /// Attempt to revoke the old refresh token via Google's revocation endpoint. | ||
| /// | ||
| /// Best-effort: warns on failure but does not return an error, since the | ||
| /// subsequent credential cleanup and fresh login will proceed regardless. | ||
| /// Kept as sync `load_encrypted()` intentionally — the macOS Keychain ties | ||
| /// ACL permissions to the calling thread, and `spawn_blocking` causes | ||
| /// repeated Keychain prompts. | ||
| async fn attempt_token_revocation() { | ||
| let creds_str = match credential_store::load_encrypted() { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!( | ||
| "Warning: could not load credentials ({}). Old token was not revoked.", | ||
| crate::output::sanitize_for_terminal(&e.to_string()) | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let creds: serde_json::Value = match serde_json::from_str(&creds_str) { | ||
| Ok(j) => j, | ||
| Err(e) => { | ||
| eprintln!( | ||
| "Warning: could not parse credentials ({}). Old token was not revoked.", | ||
| crate::output::sanitize_for_terminal(&e.to_string()) | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if let Some(rt) = creds.get("refresh_token").and_then(|v| v.as_str()) { | ||
| let client = reqwest::Client::new(); | ||
| match client | ||
| .post("https://oauth2.googleapis.com/revoke") | ||
| .form(&[("token", rt)]) | ||
| .send() | ||
| .await | ||
| { | ||
| Ok(resp) if resp.status().is_success() => {} | ||
| Ok(resp) => { | ||
| eprintln!( | ||
| "Warning: token revocation returned HTTP {}. \ | ||
| The old token may still be valid on Google's side.", | ||
| resp.status() | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| eprintln!( | ||
| "Warning: could not revoke old token ({}). \ | ||
| The old token may still be valid on Google's side.", | ||
| crate::output::sanitize_for_terminal(&e.to_string()) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async fn handle_login(args: &[String]) -> Result<(), GwsError> { | ||
| // Extract -s/--services from args | ||
| let mut services_filter: Option<HashSet<String>> = None; | ||
|
|
@@ -275,6 +406,35 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> { | |
| ..Default::default() | ||
| }; | ||
|
|
||
| // If scopes changed from the previous login, revoke the old refresh token | ||
| // so Google removes the prior consent grant. Without revocation, Google's | ||
| // consent screen shows previously-granted scopes pre-checked and the user | ||
| // may unknowingly re-grant broad access. | ||
| if let Some(prev_scopes) = load_saved_scopes().await? { | ||
| let prev_set: HashSet<&str> = prev_scopes.iter().map(|s| s.as_str()).collect(); | ||
| let new_set: HashSet<&str> = scopes.iter().map(|s| s.as_str()).collect(); | ||
| if prev_set != new_set { | ||
| attempt_token_revocation().await; | ||
| // Clear local credential and cache files to force a fresh login. | ||
| let enc_path = credential_store::encrypted_credentials_path(); | ||
| 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." | ||
| ))); | ||
| } | ||
| } | ||
|
Comment on lines
+420
to
+433
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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()
)));
}
} |
||
| eprintln!("Scopes changed — revoked previous credentials."); | ||
| } | ||
| } | ||
|
|
||
| // Ensure openid + email + profile scopes are always present so we can | ||
| // identify the user via the userinfo endpoint after login, and so the | ||
| // Gmail helpers can fall back to the People API to populate the From | ||
|
|
@@ -357,6 +517,10 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> { | |
| let enc_path = credential_store::save_encrypted(&creds_str) | ||
| .map_err(|e| GwsError::Auth(format!("Failed to encrypt credentials: {e}")))?; | ||
|
|
||
| // Persist the configured scope set for scope-change detection and | ||
| // client-side guard enforcement. | ||
| save_scopes(&scopes).await?; | ||
|
|
||
| // Clean up temp file | ||
| let _ = std::fs::remove_file(&temp_path); | ||
gerfalcon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
@@ -1173,6 +1337,17 @@ async fn handle_status() -> Result<(), GwsError> { | |
| } | ||
| } // end !cfg!(test) | ||
|
|
||
| // Show configured scope mode from scopes.json (independent of network) | ||
| if let Some(saved_scopes) = load_saved_scopes().await? { | ||
| let is_readonly = saved_scopes.iter().all(|s| is_non_write_scope(s)); | ||
| output["configured_scopes"] = json!(saved_scopes); | ||
| output["scope_mode"] = json!(if is_readonly { | ||
| "readonly" | ||
| } else { | ||
| "default" | ||
| }); | ||
| } | ||
|
|
||
| println!( | ||
| "{}", | ||
| serde_json::to_string_pretty(&output).unwrap_or_default() | ||
|
|
@@ -1185,10 +1360,11 @@ fn handle_logout() -> Result<(), GwsError> { | |
| 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 scopes_file = scopes_path(); | ||
|
|
||
| let mut removed = Vec::new(); | ||
|
|
||
| for path in [&enc_path, &plain_path, &token_cache, &sa_token_cache] { | ||
| for path in [&enc_path, &plain_path, &token_cache, &sa_token_cache, &scopes_file] { | ||
| if path.exists() { | ||
| std::fs::remove_file(path).map_err(|e| { | ||
| GwsError::Validation(format!("Failed to remove {}: {e}", path.display())) | ||
|
|
@@ -2214,4 +2390,38 @@ mod tests { | |
| let result = extract_scopes_from_doc(&doc, false); | ||
| assert!(result.is_empty()); | ||
| } | ||
|
|
||
| // --- Scope persistence and guard tests --- | ||
|
|
||
| #[test] | ||
| fn test_save_and_load_scopes_roundtrip() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let path = dir.path().join("scopes.json"); | ||
| let scopes = vec![ | ||
| "https://www.googleapis.com/auth/gmail.readonly".to_string(), | ||
| "openid".to_string(), | ||
| ]; | ||
| let json = serde_json::to_string_pretty(&scopes).unwrap(); | ||
| crate::fs_util::atomic_write(&path, json.as_bytes()).unwrap(); | ||
| let loaded: Vec<String> = | ||
| serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); | ||
| assert_eq!(loaded, scopes); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_non_write_scope() { | ||
| // Readonly and identity scopes are non-write | ||
| assert!(is_non_write_scope("https://www.googleapis.com/auth/drive.readonly")); | ||
| assert!(is_non_write_scope("https://www.googleapis.com/auth/gmail.readonly")); | ||
| assert!(is_non_write_scope("openid")); | ||
| assert!(is_non_write_scope("email")); | ||
| assert!(is_non_write_scope("profile")); | ||
| assert!(is_non_write_scope("https://www.googleapis.com/auth/userinfo.email")); | ||
|
|
||
| // Write scopes are not non-write | ||
| assert!(!is_non_write_scope("https://www.googleapis.com/auth/drive")); | ||
| assert!(!is_non_write_scope("https://www.googleapis.com/auth/gmail.modify")); | ||
| assert!(!is_non_write_scope("https://www.googleapis.com/auth/calendar")); | ||
| assert!(!is_non_write_scope("https://www.googleapis.com/auth/pubsub")); | ||
| } | ||
gerfalcon marked this conversation as resolved.
Show resolved
Hide resolved
gerfalcon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.