fix(auth): validate --services names and remove automatic cloud-platform injection#747
fix(auth): validate --services names and remove automatic cloud-platform injection#747hoyt-harness wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
…orm injection - `parse_login_args()` now validates each token in `-s`/`--services` against the known service registry (`crate::services::SERVICES`) and returns a clear error listing valid names before opening the browser. Tokens are lowercased before checking so `Drive` and `drive` both work. - `scope_matches_service()` no longer unconditionally returns `true` for `cloud-platform`. The scope is now filtered the same way as any other scope, so `-s drive` no longer silently injects cloud-platform into the token. - `run_discovery_scope_picker()` no longer forces cloud-platform into the result when a services filter is active. It is still auto-added in the unfiltered interactive flow (no `-s` flag). - Fixes pre-existing `clippy::collapsible_match` in `helpers/script.rs`. Closes googleworkspace#741.
🦋 Changeset detectedLatest commit: c6dd35b 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 |
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 improves the reliability and security of the authentication login flow by enforcing stricter input validation for service names and correcting the scope filtering logic. By removing the automatic injection of the cloud-platform scope when specific services are requested, the tool now provides more predictable and secure behavior, while also ensuring users are immediately notified of typos in their service filter arguments. 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 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces validation for the --services flag in the auth login command, ensuring that unknown service names are rejected with a descriptive error message. It also modifies the handling of the cloud-platform scope, which is no longer automatically included when a service filter is applied. Review feedback indicates that the current validation is overly restrictive as it excludes valid scopes like cloud-platform and pubsub. Additionally, it is recommended to sanitize user-provided input in error messages using .escape_debug() to prevent potential escape sequence injection vulnerabilities.
…or output Two issues from PR review: 1. Validation was too restrictive: cloud-platform and pubsub are valid requestable scopes (present in SCOPE_ENTRIES / FULL_SCOPES) but absent from the Workspace service registry (SERVICES), so they were incorrectly rejected by parse_login_args(). Since cloud-platform is no longer auto-injected when a filter is active, this made it impossible to request that scope via -s. Added PLATFORM_SCOPES allowlist for these two. 2. User-supplied tokens appeared unsanitized in the error message, allowing escape-sequence injection. Apply .escape_debug() on unknown tokens. Add test: parse_login_args_accepts_platform_scopes.
|
/gemini review |
|
Thanks for the review feedback. Both issues addressed in the latest commit:
New test: |
There was a problem hiding this comment.
Code Review
This pull request introduces validation for service names provided via the auth login --services flag, ensuring that unknown tokens result in a clear error message rather than being silently ignored. It also modifies the handling of the cloud-platform scope so that it is no longer automatically included when a specific services filter is applied. Furthermore, the file processing logic in the script helper was refactored for better readability using match guards. I have no feedback to provide.
Summary
Fixes two related bugs in
gws auth login -s/--servicesreported in #741:Unknown service names were silently dropped.
parse_login_args()now validates every token in the-slist against the built-in service registry (crate::services::SERVICES) before opening the browser. If any token is unrecognised, the command exits non-zero with a clear error that names the bad token(s) and lists all valid service names. Tokens are lowercased before checking, so-s Driveand-s driveboth work.cloud-platformwas unconditionally injected when a services filter was active.scope_matches_service()previously returnedtrueforcloud-platformregardless of what services the user asked for, so-s drivesilently includedcloud-platformin the resulting token. The unconditional pass-through is removed;cloud-platformis now filtered the same way as any other scope.run_discovery_scope_picker()retains the auto-add behaviour when no services filter is active (the existing unfiltered-interactive flow is unchanged).Behaviour change note: Users who relied on
-s <anything>always producing a token that includescloud-platformwill need to addcloud-platformexplicitly via--scopesor omit the-sfilter. This is an intentional correction: the previous injection violated the principle of least privilege and contradicted explicitly declined scopes.Also fixes a pre-existing
clippy::collapsible_matchinhelpers/script.rs(same lint fixed in #744, which targets the same upstream/main base).Test plan
parse_login_args_rejects_unknown_service— error returned for typo token, message names the bad token and lists valid servicesparse_login_args_accepts_known_services—Drive,Gmailaccepted and lowercased correctlyscope_matches_service_cloud_platform_does_not_match_unrelated_service— cloud-platform no longer auto-passes the drive filterresolve_scopes_with_services_filter— filtered result contains only drive/gmail scopes, no cloud-platformresolve_scopes_services_takes_priority_with_readonly— readonly + drive filter: only drive scopesresolve_scopes_services_takes_priority_with_full— full + gmail filter: only gmail scopescargo test -p google-workspace-cli— 701 passed, 2 pre-existing failures (unrelated to this change), 0 new failurescargo clippy -- -D warnings— clean🤖 Generated with Claude Code