fix(services): support api:version syntax for unlisted Discovery APIs#826
fix(services): support api:version syntax for unlisted Discovery APIs#826nuthalapativarun wants to merge 3 commits into
Conversation
…APIs When resolve_service fails but a version override is present (via colon syntax or --api-version flag), fall through to use the raw service arg as the API name with the provided version instead of returning an error. Fixes googleworkspace#670
🦋 Changeset detectedLatest commit: 34edcb0 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
the
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 improves the flexibility of the CLI by enabling support for unlisted Discovery APIs. By modifying the service resolution logic, the tool now correctly handles service arguments that include a version override, even if the service itself is not present in the internal known services list. This ensures that users can interact with a broader range of Google APIs without requiring explicit registration in the CLI. 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 the 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 counterproductive. 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
|
![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 fixes the <api>:<version> syntax handling to allow unlisted Discovery APIs to be called directly. It updates parse_service_and_version to fall back to the service argument and version override if the service resolution fails, and adds a corresponding unit test. The reviewer identified an issue where the --api-version=VER syntax (using an equals sign) is ignored in parse_service_and_version, causing unlisted APIs using this syntax to fail, and provided a code suggestion to resolve this inconsistency.
|
|
||
| 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) { |
There was a problem hiding this comment.
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());
}
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This comment addressed the form in the previous commit (5b7452c). The -form is now handled by alongside the space-separated form.
…ed APIs The version-override loop only matched the space-separated form (--api-version VER). Add a strip_prefix branch to also capture the equals-sign form (--api-version=VER), which filter_args_for_subcommand already stripped but parse_service_and_version silently ignored. Add a test to cover the equals-sign path with an unlisted service.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for calling unlisted Discovery APIs directly by updating the argument parsing logic to handle the <api>:<version> syntax and the --api-version= flag format. When a service is not found in the pre-defined list, the parser now falls back to using the raw service argument and the provided version override. The review feedback identifies a potential path traversal and terminal escape sequence injection vulnerability resulting from allowing arbitrary unlisted service names and versions, and suggests implementing strict character validation to mitigate these security risks.
| 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); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
| 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
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
- 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.
There was a problem hiding this comment.
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.
…path traversal Add alphanumeric + _-. allowlist check when falling back to a raw service name and version for unlisted Discovery APIs. Rejects inputs containing ../ traversal sequences or special URL characters before they can reach the Discovery URL or filesystem cache. Resolves Gemini r2 comment on PR googleworkspace#826.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the --api-version=<version> flag syntax and enables calling unlisted Discovery APIs directly by validating the service name and version when they are not in the known services list. The review feedback identifies a critical security vulnerability where the version_override is not validated when a listed service is successfully resolved, potentially allowing path traversal attacks. A code suggestion is provided to ensure the version override is validated in all code paths.
| 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); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
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);
}
}
};2 participants
Summary
When a user passes
gws admob:v1 ...,parse_service_and_versionnow falls through to use the raw API name and version rather than returning an error for unknown service aliases. The original error is still returned when no version override is present.Fixes #670
Changes
crates/google-workspace-cli/src/main.rs: whenresolve_servicefails but a version override is present, use the raw service arg as the API name#[cfg(test)]block covering theadmob:v1unlisted API caseChecklist
cargo fmt --allpassed (CI)cargo clippy -- -D warningspassed (CI)cargo testpassed (CI)