fix: encode URL template path params in build_url (#84)

* fix: encode URL path template params in build_url

* chore: trigger CLA recheck

* fix: validate +path params and align replacement logic

.changeset/fix-path-template-encoding.md

@@ -0,0 +1,8 @@
+---
+"@googleworkspace/cli": patch
+---
+
+Fix URL template path expansion to safely encode path parameters, including
+Sheets `range` values with Unicode and reserved characters. `{var}` expansions
+now encode as a path segment, `{+var}` preserves slashes while encoding each
+segment, and invalid path parameter/template mismatches fail fast.

src/executor.rs

@@ -502,15 +502,44 @@ fn build_url(
         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 url_path.contains(&placeholder) {
-            url_path = url_path.replace(&placeholder, &val_str);
-        } else if url_path.contains(&plus_placeholder) {
-            url_path = url_path.replace(&plus_placeholder, &val_str);
-        } else {
-            // It's a query parameter
-            query_params.insert(key.clone(), val_str);
+        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);
+            }
+            continue;
         }
+
+        let is_path_param = method
+            .parameters
+            .get(key)
+            .and_then(|p| p.location.as_deref())
+            == Some("path");
+
+        if is_path_param {
+            return Err(GwsError::Validation(format!(
+                "Path parameter '{}' was provided but is not present in URL template '{}'",
+                key, path_template
+            )));
+        }
+
+        // It's a query parameter
+        query_params.insert(key.clone(), val_str);
     }
 
     let full_url = if is_upload {
@@ -1162,6 +1191,160 @@ mod tests {
         assert_eq!(query.get("q").unwrap(), "search term");
     }
 
+    #[test]
+    fn test_build_url_encodes_path_parameter_chars() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.com/".to_string()),
+            ..Default::default()
+        };
+        let mut parameters = HashMap::new();
+        parameters.insert(
+            "spreadsheetId".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        parameters.insert(
+            "range".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        let method = RestMethod {
+            path: "spreadsheets/{spreadsheetId}/values/{range}".to_string(),
+            flat_path: Some("spreadsheets/{spreadsheetId}/values/{range}".to_string()),
+            parameters,
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert("spreadsheetId".to_string(), json!("abc123"));
+        params.insert("range".to_string(), json!("hash#1!A1:B2"));
+
+        let (url, _) = build_url(&doc, &method, &params, false).unwrap();
+        assert_eq!(
+            url,
+            "https://api.example.com/spreadsheets/abc123/values/hash%231%21A1%3AB2"
+        );
+    }
+
+    #[test]
+    fn test_build_url_plus_expansion_preserves_slashes() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.com/".to_string()),
+            ..Default::default()
+        };
+        let mut parameters = HashMap::new();
+        parameters.insert(
+            "name".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        let method = RestMethod {
+            path: "v1/{+name}".to_string(),
+            flat_path: Some("v1/{+name}".to_string()),
+            parameters,
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert(
+            "name".to_string(),
+            json!("projects/p1/locations/us/topics/t1"),
+        );
+
+        let (url, _) = build_url(&doc, &method, &params, false).unwrap();
+        assert_eq!(
+            url,
+            "https://api.example.com/v1/projects/p1/locations/us/topics/t1"
+        );
+    }
+
+    #[test]
+    fn test_build_url_plus_expansion_rejects_reserved_chars() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.com/".to_string()),
+            ..Default::default()
+        };
+        let mut parameters = HashMap::new();
+        parameters.insert(
+            "name".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        let method = RestMethod {
+            path: "v1/{+name}".to_string(),
+            flat_path: Some("v1/{+name}".to_string()),
+            parameters,
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert("name".to_string(), json!("projects/p1#frag?x=y"));
+
+        let err = build_url(&doc, &method, &params, false).unwrap_err();
+        assert!(err.to_string().contains("must not contain '?' or '#'"));
+    }
+
+    #[test]
+    fn test_build_url_plus_expansion_rejects_path_traversal() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.com/".to_string()),
+            ..Default::default()
+        };
+        let mut parameters = HashMap::new();
+        parameters.insert(
+            "name".to_string(),
+            crate::discovery::MethodParameter {
+                location: Some("path".to_string()),
+                ..Default::default()
+            },
+        );
+        let method = RestMethod {
+            path: "v1/{+name}".to_string(),
+            flat_path: Some("v1/{+name}".to_string()),
+            parameters,
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert("name".to_string(), json!("projects/../../etc/passwd"));
+
+        let err = build_url(&doc, &method, &params, false).unwrap_err();
+        assert!(err.to_string().contains("path traversal"));
+    }
+
+    #[test]
+    fn test_build_url_errors_for_path_param_not_in_template() {
+        let doc = RestDescription {
+            base_url: Some("https://api.example.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: "files".to_string(),
+            flat_path: Some("files".to_string()),
+            parameters,
+            ..Default::default()
+        };
+        let mut params = Map::new();
+        params.insert("fileId".to_string(), json!("123"));
+
+        let err = build_url(&doc, &method, &params, false).unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("Path parameter 'fileId' was provided but is not present"));
+    }
+
     #[test]
     fn test_handle_error_response_401() {
         let err = handle_error_response::<()>(

src/validate.rs

@@ -173,6 +173,19 @@ pub fn encode_path_segment(s: &str) -> String {
     utf8_percent_encode(s, NON_ALPHANUMERIC).to_string()
 }
 
+/// Percent-encode a value for use in URI path templates where `/` should stay
+/// as a path separator (e.g., RFC 6570 `{+name}` expansions).
+///
+/// Each path segment is encoded independently, then joined with `/`, so
+/// dangerous characters like `#`/`?` are still escaped while hierarchical
+/// resource names such as `projects/p/locations/l` remain readable.
+pub fn encode_path_preserving_slashes(s: &str) -> String {
+    s.split('/')
+        .map(encode_path_segment)
+        .collect::<Vec<_>>()
+        .join("/")
+}
+
 /// Validate a multi-segment resource name (e.g., `spaces/ABC`, `subscriptions/123`).
 /// Rejects path traversal, control characters, and URL-special characters including `%`
 /// to prevent URL-encoded bypasses. Returns the validated name or an error.
@@ -414,6 +427,25 @@ mod tests {
         assert!(encoded.contains("%2540"));
     }
 
+    #[test]
+    fn test_encode_path_preserving_slashes_hierarchical_name() {
+        let encoded = encode_path_preserving_slashes("projects/p1/locations/us/topics/t1");
+        assert_eq!(encoded, "projects/p1/locations/us/topics/t1");
+    }
+
+    #[test]
+    fn test_encode_path_preserving_slashes_escapes_reserved_chars() {
+        let encoded = encode_path_preserving_slashes("hash#1/child?x=y");
+        assert_eq!(encoded, "hash%231/child%3Fx%3Dy");
+    }
+
+    #[test]
+    fn test_encode_path_preserving_slashes_spaces_and_unicode() {
+        let encoded = encode_path_preserving_slashes("タイムライン 1/列 A");
+        assert!(!encoded.contains(' '));
+        assert!(encoded.contains('/'));
+    }
+
     // -- validate_resource_name -----------------------------------------------
 
     #[test]