-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(drive): write files.download --output contents #805
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.
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
0aedd87
77b059b
6a233a0
09d48c3
3e1fd50
7f370b2
4fb470a
0cdbbeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 { | ||||||||||||||||||||||||||
|
|
@@ -384,6 +381,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") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+422
to
+427
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Note that exact matching on
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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: | ||||||||||||||||||||||||||
|
|
@@ -464,7 +505,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, | ||||||||||||||||||||||||||
|
|
@@ -495,6 +539,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
+561
to
+562
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two significant concerns with this logic:
To fix this, you should verify that the JSON response is indeed an API response (e.g., by checking for a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, | ||||||||||||||||||||||||||
|
|
@@ -537,6 +620,31 @@ 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 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 = add_quota_project_header(client.get(download_uri)); | ||||||||||||||||||||||||||
| if let Some(token) = token { | ||||||||||||||||||||||||||
| if *auth_method == AuthMethod::OAuth { | ||||||||||||||||||||||||||
| request = request.bearer_auth(token); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding an Authorization header to a Google download URI can cause a 400 Bad Request if the URI is a "Signed URL" (common for large file downloads from Google Cloud Storage). Signed URLs already contain authentication in their query parameters (e.g., GoogleAccessId or X-Goog-Signature), and providing a Bearer token simultaneously creates an authentication conflict. You should only add the bearer token if the URL does not appear to be signed. if let Some(token) = token {
if *auth_method == AuthMethod::OAuth {
// Only add Authorization header if the URL doesn't appear to be a GCS Signed URL.
// Signed URLs contain authentication in query parameters, and adding a Bearer
// token causes a 400 Bad Request conflict.
let is_signed_url = reqwest::Url::parse(download_uri)
.map(|u| {
u.query_pairs().any(|(k, _)| {
k == "GoogleAccessId" || k == "Signature" || k.starts_with("X-Goog-")
})
})
.unwrap_or(false);
if !is_signed_url {
request = request.bearer_auth(token);
}
}
} |
||||||||||||||||||||||||||
| request | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+663
to
+684
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation has two significant issues:
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, | ||||||||||||||||||||||||||
|
|
@@ -1209,6 +1317,97 @@ 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] | ||||||||||||||||||||||||||
| #[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] | ||||||||||||||||||||||||||
| fn test_mime_to_extension_more_types() { | ||||||||||||||||||||||||||
| assert_eq!(mime_to_extension("text/plain"), "txt"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check
host == "storage.googleapis.com"is redundant becausehost.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 likegoogleusercontent.comare omitted to avoid scope creep.References