Skip to content
113 changes: 113 additions & 0 deletions crates/google-workspace-cli/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,50 @@ async fn handle_binary_response(
Ok(None)
}

fn extract_download_uri(json_val: &Value) -> Option<&str> {
[
"/response/downloadUri",
"/response/downloadUrl",
"/metadata/downloadUri",
"/metadata/downloadUrl",
"/downloadUri",
"/downloadUrl",
]
.into_iter()
.find_map(|path| json_val.pointer(path).and_then(|v| v.as_str()))
}

fn is_google_download_uri(uri: &str) -> bool {
let Ok(url) = reqwest::Url::parse(uri) else {
return false;
};
if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() {
return false;
}
let Some(host) = url.host_str() else {
return false;
};

host == "storage.googleapis.com"
|| host.ends_with(".googleapis.com")
|| host.ends_with(".googleusercontent.com")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

The check host == "storage.googleapis.com" is redundant because host.ends_with(".googleapis.com") already covers it. To ensure the host is either exactly the base domain or a proper subdomain, the check should include the base domain explicitly. Suggestions to add additional domains like googleusercontent.com are omitted to avoid scope creep.

Suggested change
host == "storage.googleapis.com"
|| host.ends_with(".googleapis.com")
|| host.ends_with(".googleusercontent.com")
host == "googleapis.com"
|| host.ends_with(".googleapis.com")
References
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.

}
Comment on lines +422 to +427
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

The storage.googleapis.com host should be included in the list of hosts allowed to receive the Bearer authentication token. Many Drive download operations redirect to Google Cloud Storage (GCS). If the GCS URL is not a signed URL (e.g., it's a direct link to a private object), the request will fail with a 401/403 error unless the OAuth2 token is provided.

Note that exact matching on storage.googleapis.com is safe, as it prevents sending the token to virtual-hosted style buckets (e.g., bucket.storage.googleapis.com) which could be attacker-controlled.

Suggested change
fn is_google_api_download_host(uri: &str) -> bool {
matches!(
parse_download_uri_host(uri).as_deref(),
Some("googleapis.com" | "www.googleapis.com")
)
}
fn is_google_api_download_host(uri: &str) -> bool {
matches!(
parse_download_uri_host(uri).as_deref(),
Some("googleapis.com" | "www.googleapis.com" | "storage.googleapis.com")
)
}


fn extract_google_download_uri(body_text: &str) -> Result<Option<String>, GwsError> {
let Ok(json_val) = serde_json::from_str::<Value>(body_text) else {
return Ok(None);
};
let Some(uri) = extract_download_uri(&json_val) else {
return Ok(None);
};
if !is_google_download_uri(uri) {
return Err(GwsError::Validation(
"Refusing to follow non-Google downloadUri from API response".to_string(),
));
}
Ok(Some(uri.to_string()))
}

/// Executes an API method call.
///
/// This is the core function of the CLI that handles:
Expand Down Expand Up @@ -495,6 +539,46 @@ pub async fn execute_method(
.await
.context("Failed to read response body")?;

if output_path.is_some() && method.id.as_deref() == Some("drive.files.download") {
if let Some(download_uri) = extract_google_download_uri(&body_text)? {
Comment on lines +561 to +562
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

There are two significant concerns with this logic:

  1. Method ID Accuracy: Please verify that drive.files.download is the correct method ID. In the standard Google Drive API v3 Discovery Document, the method for downloading file content is drive.files.get (typically used with alt=media). If the ID is actually drive.files.get, this block will never execute.
  2. Security/Correctness Risk: Following a downloadUri from a JSON body is dangerous if the response is actually the file content (e.g., a user-controlled JSON file) rather than an API Operation or metadata. If a user's file contains a downloadUri field pointing to a malicious but Google-hosted domain (like a subdomain of googleusercontent.com), the CLI might follow it and leak the user's Bearer token.

To fix this, you should verify that the JSON response is indeed an API response (e.g., by checking for a "kind": "drive#operation" field or the presence of the "done" field characteristic of Google Operations) before attempting to extract and follow a download URI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

There is a security risk here: if a user downloads a malicious JSON file that mimics a Drive operation response, the CLI will follow the downloadUri contained within that file. While the host check and token restrictions (suggested elsewhere) mitigate token theft, this still constitutes a Server-Side Request Forgery (SSRF) where the CLI makes unexpected requests based on untrusted content. Consider adding a check to ensure the response is an actual API response (e.g., by verifying the presence of Google-specific API headers like X-Goog-Api-Client) before attempting to extract a download URI.

let mut download_request = client.get(download_uri);
if let Some(token) = token {
if auth_method == AuthMethod::OAuth {
download_request = download_request.bearer_auth(token);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The download request for the downloadUri is missing the x-goog-user-project header, which is required by many Google APIs for quota and billing attribution when using a service account or specific project-based credentials. For consistency with the primary API request built in build_http_request, this header should be included if a quota project is configured.

                    let mut download_request = client.get(download_uri);
                    if let Some(quota_project) = crate::auth::get_quota_project() {
                        download_request = download_request.header("x-goog-user-project", quota_project);
                    }
                    if let Some(token) = token {
                        if auth_method == AuthMethod::OAuth {
                            download_request = download_request.bearer_auth(token);
                        }
                    }

let download_response = download_request
.send()
.await
.context("HTTP download request failed")?;
let download_status = download_response.status();
let download_content_type = download_response
.headers()
.get("content-type")
.and_then(|v| v.to_str().ok())
.unwrap_or("application/octet-stream")
.to_string();

if !download_status.is_success() {
let error_body = download_response.text().await.unwrap_or_default();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using unwrap_or_default() when reading the error body might mask the actual error if the body cannot be read. While handle_error_response handles empty strings, it would be better to provide context if the body read fails, similar to how it's done for the main response body at line 537.

return handle_error_response(download_status, &error_body, &auth_method);
}

if let Some(res) = handle_binary_response(
download_response,
&download_content_type,
output_path,
output_format,
capture_output,
)
.await?
{
captured_values.push(res);
}
break;
}
}

let should_continue = handle_json_response(
&body_text,
pagination,
Expand Down Expand Up @@ -1209,6 +1293,35 @@ mod tests {
assert_ne!(AuthMethod::OAuth, AuthMethod::None);
}

#[test]
fn test_extract_download_uri_from_drive_operation_response() {
let operation = json!({
"done": true,
"response": {
"downloadUri": "https://www.googleapis.com/download/drive/v3/files/abc?alt=media"
}
});

assert_eq!(
extract_download_uri(&operation),
Some("https://www.googleapis.com/download/drive/v3/files/abc?alt=media")
);
}

#[test]
fn test_extract_google_download_uri_rejects_non_google_url() {
let operation = json!({
"done": true,
"response": {
"downloadUri": "https://example.com/file.csv"
}
})
.to_string();

let err = extract_google_download_uri(&operation).unwrap_err();
assert!(err.to_string().contains("non-Google downloadUri"));
}

#[test]
fn test_mime_to_extension_more_types() {
assert_eq!(mime_to_extension("text/plain"), "txt");
Expand Down
Loading