fix: URL template rendering for upload endpoints (#129)

2026-03-05 12:16:14 +05:30
parent 87e4bb1dc4
commit 75cec1b444
2 changed files with 144 additions and 26 deletions
@@ -0,0 +1,5 @@
+---
+"@googleworkspace/cli": patch
+---
+
+Fix URL template expansion so media upload endpoints substitute path parameters and avoid iterative replacement side effects.
@@ -18,7 +18,7 @@
 //! Responsibilities include multipart file uploads, response pagination,
 //! error mapping, and optionally running text content through Model Armor for sanitization.

-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::path::PathBuf;

 use anyhow::Context;
@@ -513,7 +513,7 @@ fn build_url(
    };

    // Substitute path parameters and separate query parameters
-    let mut url_path = path_template.to_string();
+    let path_parameters = extract_template_path_parameters(path_template);
    let mut query_params: HashMap<String, String> = HashMap::new();

    for (key, value) in params {
@@ -522,29 +522,7 @@ fn build_url(
            other => other.to_string(),
        };

-        let placeholder = format!("{{{key}}}");
-        // Also handle {+key} style
-        let plus_placeholder = format!("{{+{key}}}");
-        let has_plain_placeholder = url_path.contains(&placeholder);
-        let has_plus_placeholder = url_path.contains(&plus_placeholder);
-
-        if has_plain_placeholder || has_plus_placeholder {
-            // RFC 6570-like expansion:
-            // - {var}: path segment expansion (encode all reserved characters)
-            // - {+var}: reserved expansion (preserve '/' path separators only)
-            if has_plain_placeholder {
-                // TODO: This iterative `replace` can expand placeholder-looking
-                // substrings that appear inside another parameter's value.
-                // A single-pass template renderer would avoid that class of bugs.
-                let encoded = crate::validate::encode_path_segment(&val_str);
-                url_path = url_path.replace(&placeholder, &encoded);
-            } else if has_plus_placeholder {
-                // `{+var}` keeps `/` separators, so validate before encoding to
-                // block traversal and URL-special injection payloads.
-                let validated = crate::validate::validate_resource_name(&val_str)?;
-                let encoded = crate::validate::encode_path_preserving_slashes(validated);
-                url_path = url_path.replace(&plus_placeholder, &encoded);
-            }
+        if path_parameters.contains(key.as_str()) {
            continue;
        }

@@ -565,6 +543,8 @@ fn build_url(
        query_params.insert(key.clone(), val_str);
    }

+    let url_path = render_path_template(path_template, params)?;
+
    let full_url = if is_upload {
        // Use the upload endpoint from the Discovery Document
        let upload_endpoint = method
@@ -579,7 +559,8 @@ fn build_url(
                        .to_string(),
                )
            })?;
-        format!("{}{}", doc.root_url.trim_end_matches('/'), upload_endpoint)
+        let upload_path = render_path_template(upload_endpoint, params)?;
+        format!("{}{}", doc.root_url.trim_end_matches('/'), upload_path)
    } else {
        format!("{base_url}{url_path}")
    };
@@ -587,6 +568,76 @@ fn build_url(
    Ok((full_url, query_params))
 }

+fn extract_template_path_parameters(path_template: &str) -> HashSet<&str> {
+    let mut found = HashSet::new();
+    let mut cursor = 0;
+
+    while let Some(open_idx) = path_template[cursor..].find('{') {
+        let token_start = cursor + open_idx;
+        let Some(close_idx) = path_template[token_start..].find('}') else {
+            break;
+        };
+
+        let token_end = token_start + close_idx;
+        let token = &path_template[token_start + 1..token_end];
+        if let Some(key) = token.strip_prefix('+') {
+            found.insert(key);
+        } else {
+            found.insert(token);
+        }
+        cursor = token_end + 1;
+    }
+
+    found
+}
+
+fn render_path_template(
+    path_template: &str,
+    params: &Map<String, Value>,
+) -> Result<String, GwsError> {
+    let mut rendered = String::with_capacity(path_template.len());
+    let mut cursor = 0;
+
+    while let Some(open_idx) = path_template[cursor..].find('{') {
+        let token_start = cursor + open_idx;
+        rendered.push_str(&path_template[cursor..token_start]);
+
+        let Some(close_idx) = path_template[token_start..].find('}') else {
+            rendered.push_str(&path_template[token_start..]);
+            return Ok(rendered);
+        };
+
+        let token_end = token_start + close_idx;
+        let token = &path_template[token_start + 1..token_end];
+        let (is_plus, key) = if let Some(key) = token.strip_prefix('+') {
+            (true, key)
+        } else {
+            (false, token)
+        };
+
+        if let Some(value) = params.get(key) {
+            let val_str = match value {
+                Value::String(s) => s.clone(),
+                other => other.to_string(),
+            };
+            let encoded = if is_plus {
+                let validated = crate::validate::validate_resource_name(&val_str)?;
+                crate::validate::encode_path_preserving_slashes(validated)
+            } else {
+                crate::validate::encode_path_segment(&val_str)
+            };
+            rendered.push_str(&encoded);
+        } else {
+            rendered.push_str(&path_template[token_start..=token_end]);
+        }
+
+        cursor = token_end + 1;
+    }
+
+    rendered.push_str(&path_template[cursor..]);
+    Ok(rendered)
+}
+
 /// Attempts to extract a GCP console enable URL from a Google API `accessNotConfigured`
 /// error message.
 ///
@@ -1339,6 +1390,68 @@ mod tests {
        assert!(err.to_string().contains("path traversal"));
    }

+    #[test]
+    fn test_build_url_upload_endpoint_substitutes_path_params() {
+        let doc = RestDescription {
+            root_url: "https://www.googleapis.com/".to_string(),
+            ..Default::default()
+        };
+        let mut parameters = HashMap::new();
+        parameters.insert(
+            "fileId".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        let method = RestMethod {
+            path: "drive/v3/files/{fileId}".to_string(),
+            flat_path: Some("drive/v3/files/{fileId}".to_string()),
+            parameters,
+            media_upload: Some(crate::discovery::MediaUpload {
+                protocols: Some(crate::discovery::MediaUploadProtocols {
+                    simple: Some(crate::discovery::MediaUploadProtocol {
+                        path: "/upload/drive/v3/files/{fileId}".to_string(),
+                        multipart: Some(true),
+                    }),
+                }),
+                ..Default::default()
+            }),
+            ..Default::default()
+        };
+
+        let mut params = Map::new();
+        params.insert("fileId".to_string(), json!("abc/123"));
+
+        let (url, _) = build_url(&doc, &method, &params, true).unwrap();
+        assert_eq!(
+            url,
+            "https://www.googleapis.com/upload/drive/v3/files/abc%2F123"
+        );
+    }
+
+    #[test]
+    fn test_build_url_does_not_replace_placeholder_like_values() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.com/".to_string()),
+            ..Default::default()
+        };
+        let method = RestMethod {
+            path: "v1/{parent}/{child}".to_string(),
+            flat_path: Some("v1/{parent}/{child}".to_string()),
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert("parent".to_string(), json!("literal-{child}-value"));
+        params.insert("child".to_string(), json!("ok"));
+
+        let (url, _) = build_url(&doc, &method, &params, false).unwrap();
+        assert_eq!(
+            url,
+            "https://api.example.com/v1/literal%2D%7Bchild%7D%2Dvalue/ok"
+        );
+    }
+
    #[test]
    fn test_build_url_errors_for_path_param_not_in_template() {
        let doc = RestDescription {