Skip to content

fix(services): support api:version syntax for unlisted Discovery APIs #826

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.

Sign up for GitHub

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
nuthalapativarun wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(services): support api:version syntax for unlisted Discovery APIs #826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2f08e7
fix(services): support <api>:<version> syntax for unlisted Discovery …
nuthalapativarun Jun 1, 2026
5b7452c
fix(services): handle --api-version=VER equals-sign syntax for unlist…
nuthalapativarun Jun 1, 2026
34edcb0
fix(services): validate chars in unlisted service/version to prevent …
nuthalapativarun Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-api-version-syntax.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@googleworkspace/cli": patch
---

Fix `<api>:<version>` syntax so unlisted Discovery APIs can be called directly
82 changes: 79 additions & 3 deletions crates/google-workspace-cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,12 @@ pub fn parse_service_and_version(
let mut service_arg = first_arg;
let mut version_override: Option<String> = None;

// Check for --api-version flag anywhere in args
// Check for --api-version flag anywhere in args (space-separated or = form)
for i in 0..args.len() {
if args[i] == "--api-version" && i + 1 < args.len() {
version_override = Some(args[i + 1].clone());
} else if let Some(ver) = args[i].strip_prefix("--api-version=") {
version_override = Some(ver.to_string());
}
}

Expand All @@ -340,8 +342,27 @@ pub fn parse_service_and_version(
}
}

let (api_name, default_version) = services::resolve_service(service_arg)?;
let version = version_override.unwrap_or(default_version);
let (api_name, version) = match services::resolve_service(service_arg) {
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is an inconsistency in how --api-version is parsed. In run() and filter_args_for_subcommand(), the --api-version=VER syntax (with an equals sign) is recognized and stripped. However, in parse_service_and_version(), the loop only checks for args[i] == "--api-version", meaning --api-version=VER is completely ignored. This causes unlisted APIs using the equals-sign syntax to fail with an 'Unknown service' error, and listed APIs to ignore the version override.

Please update the parsing loop in parse_service_and_version (around line 328) to also support the --api-version=VER syntax:

for i in 0..args.len() {
    if args[i] == "--api-version" && i + 1 < args.len() {
        version_override = Some(args[i + 1].clone());
    } else if args[i].starts_with("--api-version=") {
        if let Some((_, ver)) = args[i].split_once('=') {
            version_override = Some(ver.to_string());
        }
    }
}

Copy link
Copy Markdown
Author

@nuthalapativarun nuthalapativarun Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5b7452c. The parsing loop in \parse_service_and_version\ now handles both forms:

\
ust
} else if let Some(ver) = args[i].strip_prefix(--api-version=) {
version_override = Some(ver.to_string());
}
\\

\strip_prefix\ is equivalent to your suggested \starts_with\ + \split_once\ but avoids the redundant split. A test covering this path (\ est_parse_service_and_version_unlisted_with_equals_flag) is also included.

Copy link
Copy Markdown
Author

@nuthalapativarun nuthalapativarun Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment addressed the form in the previous commit (5b7452c). The -form is now handled by alongside the space-separated form.

Ok((name, default_ver)) => (name, version_override.unwrap_or(default_ver)),
Err(e) => {
if let Some(ver) = version_override {
let is_valid = |s: &str| {
!s.is_empty()
&& s.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.')
};
if is_valid(service_arg) && is_valid(&ver) {
(service_arg.to_string(), ver)
} else {
return Err(GwsError::Validation(
"Invalid service name or version: only alphanumeric characters, underscores, hyphens, and dots are allowed.".to_string()
));
}
} else {
return Err(e);
}
}
};
Comment on lines +345 to +365
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

Allowing arbitrary unlisted service names and versions introduces a potential path traversal vulnerability when fetching and caching the Discovery Document (e.g., if the service name contains ../ or other directory traversal sequences). Additionally, echoing unsanitized user input in error messages can lead to escape sequence injection in the terminal.

To mitigate these risks, we should strictly validate that both the service name and version contain only safe characters (alphanumeric, underscores, hyphens, and dots) and avoid echoing the raw invalid input in the error message. Note that when implementing this path validation, we should acknowledge 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.

Suggested change
let (api_name, version) = match services::resolve_service(service_arg) {
Ok((name, default_ver)) => (name, version_override.unwrap_or(default_ver)),
Err(e) => {
if let Some(ver) = version_override {
(service_arg.to_string(), ver)
} else {
return Err(e);
}
}
};
let (api_name, version) = match services::resolve_service(service_arg) {
Ok((name, default_ver)) => (name, version_override.unwrap_or(default_ver)),
Err(e) => {
if let Some(ver) = version_override {
let is_valid = |s: &str| !s.is_empty() && s.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.');
if is_valid(service_arg) && is_valid(&ver) {
(service_arg.to_string(), ver)
} else {
return Err(GwsError::Validation(
"Invalid service name or version. Only alphanumeric characters, underscores, hyphens, and dots are allowed.".to_string()
));
}
} else {
return Err(e);
}
}
};
References
  1. Sanitize error strings printed to the terminal to prevent escape sequence injection.
  2. 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.

Copy link
Copy Markdown
Author

@nuthalapativarun nuthalapativarun Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 34edcb0. Added a strict closure that requires all characters to be ASCII alphanumeric or one of _, -, .. Inputs containing ../ traversal, ?/# injection, or other special characters are now rejected with a GwsError::Validation before reaching the Discovery URL or filesystem cache. Also added three regression tests covering path traversal in service name, path traversal in version, and special chars in service name.

Comment on lines +345 to +365
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

Security Vulnerability: Validation Bypass for Listed Services

When services::resolve_service(service_arg) succeeds (i.e., for any listed/known service like drive), the Ok branch is taken. In this branch, version_override is used directly without any validation. This allows an attacker or user to pass arbitrary/malicious strings (such as path traversal sequences like ../) in the --api-version flag when targeting listed services, completely bypassing the safety checks.

To fix this, we should validate version_override immediately if it is present, regardless of whether the service is listed or unlisted.

    let is_valid = |s: &str| {
        !s.is_empty()
            && s.chars()
                .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.')
    };

    if let Some(ref ver) = version_override {
        if !is_valid(ver) {
            return Err(GwsError::Validation(
                "Invalid version: only alphanumeric characters, underscores, hyphens, and dots are allowed.".to_string()
            ));
        }
    }

    let (api_name, version) = match services::resolve_service(service_arg) {
        Ok((name, default_ver)) => (name, version_override.unwrap_or(default_ver)),
        Err(e) => {
            if let Some(ver) = version_override {
                if is_valid(service_arg) {
                    (service_arg.to_string(), ver)
                } else {
                    return Err(GwsError::Validation(
                        "Invalid service name: only alphanumeric characters, underscores, hyphens, and dots are allowed.".to_string()
                    ));
                }
            } else {
                return Err(e);
            }
        }
    };

Ok((api_name, version))
}

Expand Down Expand Up @@ -735,4 +756,59 @@ mod tests {
let scopes: Vec<String> = vec![];
assert_eq!(select_scope(&scopes), None);
}

#[test]
fn test_parse_service_and_version_unlisted_with_colon() {
// admob is not in the known services list, but admob:v1 should work
let args = vec![
"gws".to_string(),
"admob:v1".to_string(),
"accounts".to_string(),
"list".to_string(),
];
let result = parse_service_and_version(&args, "admob:v1");
assert_eq!(result.unwrap(), ("admob".to_string(), "v1".to_string()));
}

#[test]
fn test_parse_service_and_version_unlisted_with_equals_flag() {
// admob is not in the known services list; --api-version=v1 (equals form) should work
let args = vec![
"gws".to_string(),
"admob".to_string(),
"--api-version=v1".to_string(),
"accounts".to_string(),
"list".to_string(),
];
let result = parse_service_and_version(&args, "admob");
assert_eq!(result.unwrap(), ("admob".to_string(), "v1".to_string()));
}

#[test]
fn test_parse_service_and_version_rejects_path_traversal_in_service() {
let args = vec![
"gws".to_string(),
"../evil:v1".to_string(),
];
let result = parse_service_and_version(&args, "../evil:v1");
assert!(result.is_err(), "path traversal in service name must be rejected");
}

#[test]
fn test_parse_service_and_version_rejects_path_traversal_in_version() {
let args = vec![
"gws".to_string(),
"admob".to_string(),
"--api-version=../evil".to_string(),
];
let result = parse_service_and_version(&args, "admob");
assert!(result.is_err(), "path traversal in version must be rejected");
}

#[test]
fn test_parse_service_and_version_rejects_special_chars_in_service() {
let args = vec!["gws".to_string(), "svc?foo:v1".to_string()];
let result = parse_service_and_version(&args, "svc?foo:v1");
assert!(result.is_err(), "special chars in service name must be rejected");
}
}
Loading