Skip to content
Open
Changes from all commits
Commits
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
354 changes: 349 additions & 5 deletions crates/google-workspace-cli/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,7 @@ async fn build_http_request(
}
}

// Set quota project from ADC for billing/quota attribution
if let Some(quota_project) = crate::auth::get_quota_project() {
request = request.header("x-goog-user-project", quota_project);
}
request = add_quota_project_header(request);

let mut all_query_params = input.query_params.clone();
if let Some(pt) = page_token {
Expand Down Expand Up @@ -384,6 +381,68 @@ 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_drive_download_operation(json_val: &Value) -> bool {
json_val
.get("kind")
.and_then(Value::as_str)
.is_some_and(|kind| kind == "drive#operation")
}
Comment on lines +397 to +402
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

Checking for the presence of a done field is too generic and can lead to false positives when downloading user-provided JSON files. This increases the risk of the CLI misinterpreting file content as an API operation. Rely on the more specific kind field or other unique markers of a Drive operation response.

fn is_drive_download_operation(json_val: &Value) -> bool {
    json_val
        .get("kind")
        .and_then(Value::as_str)
        .is_some_and(|kind| kind == "drive#operation")
}


fn parse_download_uri_host(uri: &str) -> Option<String> {
let Ok(url) = reqwest::Url::parse(uri) else {
return None;
};
if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() {
return None;
};
url.host_str().map(ToOwned::to_owned)
}

fn is_google_download_uri(uri: &str) -> bool {
matches!(
parse_download_uri_host(uri).as_deref(),
Some("googleapis.com" | "www.googleapis.com" | "storage.googleapis.com")
)
}
Comment on lines +414 to +419
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 whitelist for following download URIs is too restrictive and will likely break legitimate Google Drive downloads. Google frequently uses various subdomains for content delivery, such as drive.googleusercontent.com or dynamically generated subdomains like doc-0k-8s-docs.googleusercontent.com.

It is safer to allow any subdomain of googleapis.com and googleusercontent.com for the purpose of following the redirect, as long as the authentication token is only sent to a more strictly validated set of hosts (which is handled separately in is_google_api_download_host).

fn is_google_download_uri(uri: &str) -> bool {
    parse_download_uri_host(uri).as_deref().is_some_and(|host| {
        host == "googleapis.com" || host.ends_with(".googleapis.com") || host.ends_with(".googleusercontent.com")
    })
}


fn is_google_api_download_host(uri: &str) -> bool {
matches!(
parse_download_uri_host(uri).as_deref(),
Some("googleapis.com" | "www.googleapis.com")
)
}
Comment on lines +421 to +426
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);
};
if !is_drive_download_operation(&json_val) {
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 @@ -464,7 +523,10 @@ pub async fn execute_method(
.to_string();

if !status.is_success() {
let error_body = response.text().await.unwrap_or_default();
let error_body = response
.text()
.await
.context("Failed to read API error response body")?;
tracing::warn!(
api_method = method_id,
http_method = %method.http_method,
Expand Down Expand Up @@ -495,6 +557,45 @@ 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 +560 to +561
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 download_request =
build_download_request(&client, &download_uri, token, &auth_method);
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
.context("Failed to read Drive download error response body")?;
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 @@ -537,6 +638,50 @@ pub async fn execute_method(
Ok(None)
}

fn add_quota_project_header(request: reqwest::RequestBuilder) -> reqwest::RequestBuilder {
if let Some(quota_project) = crate::auth::get_quota_project() {
request.header("x-goog-user-project", quota_project)
} else {
request
}
}

fn is_signed_download_uri(download_uri: &str) -> bool {
reqwest::Url::parse(download_uri)
.map(|url| {
url.query_pairs().any(|(key, _)| {
let key = key.as_ref();
key.eq_ignore_ascii_case("GoogleAccessId")
|| key.eq_ignore_ascii_case("Signature")
|| key.to_ascii_lowercase().starts_with("x-goog-")
})
})
.unwrap_or(false)
}

fn build_download_request(
client: &reqwest::Client,
download_uri: &str,
token: Option<&str>,
auth_method: &AuthMethod,
) -> reqwest::RequestBuilder {
// Keep secondary Drive downloads on the same reqwest client so they use
// the same client-level configuration as API requests.
let mut request = client.get(download_uri);
let is_signed = is_signed_download_uri(download_uri);

if !is_signed {
request = add_quota_project_header(request);
if let Some(token) = token {
if *auth_method == AuthMethod::OAuth && is_google_api_download_host(download_uri) {
request = request.bearer_auth(token);
}
}
}

request
}
Comment on lines +662 to +683
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-critical critical

This implementation has two significant issues:

  1. Signed URL Compatibility: Adding the x-goog-user-project header to a signed URL (like those from GCS) will cause a SignatureDoesNotMatch (403) error because the header was not part of the original signature.
  2. Security (Token Leakage): Sending Bearer tokens to storage.googleapis.com is dangerous as it allows token exfiltration to user-controlled buckets. Bearer tokens should only be sent to the main API surface (www.googleapis.com). GCS downloads should rely on signed URLs which do not require the Authorization header.
fn build_download_request(
    client: &reqwest::Client,
    download_uri: &str,
    token: Option<&str>,
    auth_method: &AuthMethod,
) -> reqwest::RequestBuilder {
    let mut request = client.get(download_uri);
    let is_signed = is_signed_download_uri(download_uri);

    // Only add auth/quota headers if the URI is NOT a signed URL.
    if !is_signed {
        request = add_quota_project_header(request);
        if let Some(token) = token {
            // Only send Bearer tokens to the main API surface. 
            // GCS downloads (storage.googleapis.com) should use signed URLs.
            let is_api_host = reqwest::Url::parse(download_uri)
                .map(|u| matches!(u.host_str(), Some("www.googleapis.com" | "googleapis.com")))
                .unwrap_or(false);

            if *auth_method == AuthMethod::OAuth && is_api_host {
                request = request.bearer_auth(token);
            }
        }
    }
    request
}


fn build_url(
doc: &RestDescription,
method: &RestMethod,
Expand Down Expand Up @@ -1209,6 +1354,205 @@ 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_ignores_user_json_file_content() {
let file_content = json!({
"done": true,
"downloadUri": "https://www.googleapis.com/download/drive/v3/files/abc?alt=media"
})
.to_string();

assert_eq!(extract_google_download_uri(&file_content).unwrap(), None);
}

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

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

#[test]
fn test_extract_google_download_uri_rejects_non_google_url() {
let operation = json!({
"kind": "drive#operation",
"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_is_google_download_uri_allows_googleapis_hosts_only() {
assert!(is_google_download_uri("https://googleapis.com/download"));
assert!(is_google_download_uri(
"https://storage.googleapis.com/download/storage/v1/b/bucket/o/file"
));
assert!(!is_google_download_uri(
"https://storage.googleapis.com.evil.example/file"
));
assert!(!is_google_download_uri(
"https://attacker-bucket.storage.googleapis.com/file"
));
assert!(!is_google_download_uri(
"https://drive.googleusercontent.com/file"
));
}

#[test]
#[serial_test::serial]
fn test_add_quota_project_header_uses_configured_project() {
unsafe {
std::env::set_var("GOOGLE_WORKSPACE_PROJECT_ID", "quota-project");
}

let request = add_quota_project_header(reqwest::Client::new().get("https://example.com"))
.build()
.unwrap();

unsafe {
std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID");
}

assert_eq!(
request
.headers()
.get("x-goog-user-project")
.and_then(|value| value.to_str().ok()),
Some("quota-project")
);
}

#[test]
#[serial_test::serial]
fn test_build_download_request_keeps_client_auth_and_quota_headers() {
let client = reqwest::Client::new();

unsafe {
std::env::set_var("GOOGLE_WORKSPACE_PROJECT_ID", "quota-project");
}

let request = build_download_request(
&client,
"https://www.googleapis.com/download/drive/v3/files/abc?alt=media",
Some("access-token"),
&AuthMethod::OAuth,
)
.build()
.unwrap();

unsafe {
std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID");
}

assert_eq!(
request
.headers()
.get("x-goog-user-project")
.and_then(|value| value.to_str().ok()),
Some("quota-project")
);
assert_eq!(
request
.headers()
.get(reqwest::header::AUTHORIZATION)
.and_then(|value| value.to_str().ok()),
Some("Bearer access-token")
);
}

#[test]
#[serial_test::serial]
fn test_build_download_request_skips_headers_for_signed_uri() {
let client = reqwest::Client::new();

unsafe {
std::env::set_var("GOOGLE_WORKSPACE_PROJECT_ID", "quota-project");
}

let request = build_download_request(
&client,
"https://storage.googleapis.com/download/storage/v1/b/bucket/o/file?X-Goog-Signature=sig&X-Goog-Credential=credential",
Some("access-token"),
&AuthMethod::OAuth,
)
.build()
.unwrap();

unsafe {
std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID");
}

assert!(request.headers().get("x-goog-user-project").is_none());
assert!(request
.headers()
.get(reqwest::header::AUTHORIZATION)
.is_none());
}

#[test]
#[serial_test::serial]
fn test_build_download_request_never_sends_bearer_to_storage_host() {
let client = reqwest::Client::new();

unsafe {
std::env::set_var("GOOGLE_WORKSPACE_PROJECT_ID", "quota-project");
}

let request = build_download_request(
&client,
"https://storage.googleapis.com/download/storage/v1/b/bucket/o/file",
Some("access-token"),
&AuthMethod::OAuth,
)
.build()
.unwrap();

unsafe {
std::env::remove_var("GOOGLE_WORKSPACE_PROJECT_ID");
}

assert_eq!(
request
.headers()
.get("x-goog-user-project")
.and_then(|value| value.to_str().ok()),
Some("quota-project")
);
assert!(request
.headers()
.get(reqwest::header::AUTHORIZATION)
.is_none());
}

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