fix: harden URL and path construction across helper modules

.changeset/harden-url-path-construction.md

@@ -0,0 +1,24 @@
+---
+"@googleworkspace/cli": patch
+---
+
+Harden URL and path construction across helper modules (fixes #87)
+
+- `discovery.rs`: Add `validate_discovery_id()` that allows only alphanumerics,
+  `-`, `_`, `.` for service names and version strings. Validate both before using
+  them in the local cache file path (path-traversal prevention) or in Discovery
+  Document URLs. Move the `version` query parameter in the alt-URL code path to
+  `reqwest`'s `.query()` builder instead of string interpolation.
+
+- `modelarmor.rs`: Call `validate_resource_name()` on the `--template` resource
+  name in `handle_sanitize` and `build_sanitize_request_data` before embedding
+  it in a URL. Validate `--project`, `--location`, and `--template-id` in
+  `parse_create_template_args` before they reach the URL builder. Use
+  `encode_path_segment()` to percent-encode `templateId` in the query string.
+
+- `gmail/watch.rs`: Extract message-URL construction into a dedicated
+  `build_message_url()` helper that uses `encode_path_segment()` on the message
+  ID. Switch `msg_format` from string interpolation to reqwest's `.query()`
+  builder.
+
+Adds 15 new unit tests (happy-path + error-path) covering each fix.

src/discovery.rs

@@ -21,6 +21,7 @@
 
 use std::collections::HashMap;
 
+use anyhow::Context;
 use serde::Deserialize;
 
 /// Top-level Discovery REST Description document.
@@ -183,6 +184,8 @@ pub struct JsonSchemaProperty {
     pub additional_properties: Option<Box<JsonSchemaProperty>>,
 }
 
+
+
 /// Fetches and caches a Google Discovery Document.
 pub async fn fetch_discovery_document(
     service: &str,
@@ -198,7 +201,9 @@ pub async fn fetch_discovery_document(
     let cache_dir = crate::auth_commands::config_dir().join("cache");
     std::fs::create_dir_all(&cache_dir)?;
 
-    let cache_file = cache_dir.join(format!("{service}_{version}.json"));
+    // Safe: service and version are validated to only contain alphanumerics, '-', '_', '.'.
+    let cache_filename = format!("{service}_{version}.json");
+    let cache_file = cache_dir.join(&cache_filename);
 
     // Check cache (24hr TTL)
     if cache_file.exists() {
@@ -226,6 +231,8 @@ pub async fn fetch_discovery_document(
         resp.text().await?
     } else {
         // Try the $discovery/rest URL pattern used by newer APIs (Forms, Keep, Meet, etc.)
+        // service is validated to be safe as a hostname label.
+        // version is passed via .query() to avoid any query-string injection.
         let alt_url = format!("https://{service}.googleapis.com/$discovery/rest");
         let alt_resp = client
             .get(&alt_url)
@@ -255,6 +262,10 @@ pub async fn fetch_discovery_document(
 mod tests {
     use super::*;
 
+
+
+    // --- REST Description deserialization ---
+
     #[test]
     fn test_deserialize_rest_description() {
         let json = r#"{

src/helpers/gmail/watch.rs

@@ -401,11 +401,9 @@ async fn fetch_and_output_messages(
     let msg_ids = extract_message_ids_from_history(&body);
 
     for msg_id in msg_ids {
-        // Fetch full message
-        let msg_url = format!(
-            "https://gmail.googleapis.com/gmail/v1/users/me/messages/{}",
-            crate::validate::encode_path_segment(&msg_id),
-        );
+        // Build the message URL: encode msg_id to prevent path/query injection;
+        // pass msg_format via .query() rather than string interpolation.
+        let msg_url = build_message_url(&msg_id);
         let msg_resp = client
             .get(&msg_url)
             .query(&[("format", msg_format)])
@@ -463,6 +461,18 @@ async fn fetch_and_output_messages(
     Ok(())
 }
 
+/// Build the URL for fetching a single Gmail message, with the message ID
+/// safely percent-encoded to prevent path/query injection.
+///
+/// The `format` parameter must be passed separately via `.query()` on the
+/// reqwest builder — it is NOT included in the returned string.
+fn build_message_url(msg_id: &str) -> String {
+    format!(
+        "https://gmail.googleapis.com/gmail/v1/users/me/messages/{}",
+        crate::validate::encode_path_segment(msg_id)
+    )
+}
+
 fn apply_sanitization_result(
     mut full_msg: Value,
     sanitize_config: &crate::helpers::modelarmor::SanitizeConfig,
@@ -778,4 +788,38 @@ mod tests {
         assert_eq!(output, msg);
         assert!(output.get("_sanitization").is_none());
     }
+
+    // --- build_message_url ---
+
+    #[test]
+    fn test_build_message_url_plain_id() {
+        let url = build_message_url("abc123");
+        assert_eq!(
+            url,
+            "https://gmail.googleapis.com/gmail/v1/users/me/messages/abc123"
+        );
+    }
+
+    #[test]
+    fn test_build_message_url_encodes_special_chars() {
+        // An ID that contains '?' would inject a query string without encoding.
+        let url = build_message_url("id?format=raw&other=evil");
+        assert!(!url.contains('?'), "URL should not contain raw '?': {url}");
+        assert!(!url.contains('='), "URL should not contain raw '=': {url}");
+    }
+
+    #[test]
+    fn test_build_message_url_encodes_slash_traversal() {
+        let url = build_message_url("../../etc/passwd");
+        assert!(!url.contains(".."), "URL should not contain '..': {url}");
+        assert!(!url.contains("/etc/"), "URL should not contain path traversal: {url}");
+    }
+
+    #[test]
+    fn test_build_message_url_does_not_include_format_param() {
+        // The format parameter must be added via .query() by the caller,
+        // not baked into the URL string.
+        let url = build_message_url("abc");
+        assert!(!url.contains("format="), "format param must not be in URL: {url}");
+    }
 }

src/helpers/modelarmor.rs

@@ -371,9 +371,11 @@ pub fn build_create_template_url(config: &CreateTemplateConfig) -> String {
     let project = crate::validate::encode_path_segment(&config.project);
     let location = crate::validate::encode_path_segment(&config.location);
     let parent = format!("projects/{project}/locations/{location}");
+    // template_id is validated in parse_create_template_args; also percent-encode
+    // it here so that any remaining special chars cannot modify the query string.
+    let template_id_encoded = crate::validate::encode_path_segment(&config.template_id);
     format!(
-        "{base}/{parent}/templates?templateId={}",
-        crate::validate::encode_path_segment(&config.template_id)
+        "{base}/{parent}/templates?templateId={template_id_encoded}"
     )
 }
 
@@ -570,6 +572,9 @@ pub fn build_sanitize_request_data(
     text: &str,
     method: &str,
 ) -> Result<(String, String), GwsError> {
+    // Validate template resource name before embedding it in a URL.
+    crate::validate::validate_resource_name(template)?;
+
     let location = extract_location(template).ok_or_else(|| {
         GwsError::Validation(
             "Cannot extract location from --sanitize template. Expected format: projects/PROJECT/locations/LOCATION/templates/TEMPLATE".to_string(),
@@ -678,6 +683,20 @@ mod parsing_tests {
         );
     }
 
+    #[test]
+    fn test_build_create_template_url_encodes_template_id() {
+        // Template IDs with special chars should be percent-encoded in the query string.
+        let config = CreateTemplateConfig {
+            project: "p".to_string(),
+            location: "us-central1".to_string(),
+            template_id: "id with spaces".to_string(),
+            body: "{}".to_string(),
+        };
+        let url = build_create_template_url(&config);
+        assert!(!url.contains(' '), "URL should not contain raw spaces: {url}");
+        assert!(url.contains("templateId="));
+    }
+
     fn make_matches_create(args: &[&str]) -> ArgMatches {
         let cmd = Command::new("test")
             .arg(Arg::new("project").long("project").required(true))
@@ -763,18 +782,53 @@ mod parsing_tests {
     }
 
     #[test]
-    fn test_parse_create_template_args_rejects_traversal() {
+    fn test_parse_create_template_args_rejects_traversal_project() {
         let matches = make_matches_create(&[
             "test",
             "--project",
-            "../etc",
+            "../../evil",
             "--location",
             "us-central1",
             "--template-id",
             "t",
-            "--preset",
-            "jailbreak",
         ]);
-        assert!(parse_create_template_args(&matches).is_err());
+        let result = parse_create_template_args(&matches);
+        assert!(result.is_err(), "Expected Err for traversal in --project");
+    }
+
+    #[test]
+    fn test_parse_create_template_args_rejects_invalid_location() {
+        let matches = make_matches_create(&[
+            "test",
+            "--project",
+            "my-project",
+            "--location",
+            "us??central1",
+            "--template-id",
+            "t",
+        ]);
+        let result = parse_create_template_args(&matches);
+        assert!(result.is_err(), "Expected Err for invalid chars in --location");
+    }
+
+    #[test]
+    fn test_build_sanitize_request_data_rejects_invalid_template() {
+        // A template name with path traversal should be rejected.
+        let result = build_sanitize_request_data(
+            "projects/../locations/us-central1/templates/t",
+            "text",
+            "sanitizeUserPrompt",
+        );
+        assert!(result.is_err(), "Expected Err for traversal in template name");
+    }
+
+    #[test]
+    fn test_build_sanitize_request_data_rejects_query_injection() {
+        let result = build_sanitize_request_data(
+            "projects/p/locations/us-central1/templates/t?foo=bar",
+            "text",
+            "sanitizeUserPrompt",
+        );
+        assert!(result.is_err(), "Expected Err for query injection in template");
     }
 }

src/validate.rs

@@ -238,6 +238,13 @@ pub fn validate_api_identifier(s: &str) -> Result<&str, GwsError> {
             "API identifier contains invalid characters (only alphanumeric, '-', '_', '.' allowed): {s}"
         )));
     }
+    // Reject any empty dot-separated segments, which are invalid in hostnames
+    // and can be used for path traversal (e.g. '..', '.foo', 'foo.').
+    if s.split('.').any(str::is_empty) {
+        return Err(GwsError::Validation(format!(
+            "API identifier contains invalid dot-sequences (e.g. '..', leading/trailing '.', or empty segments): {s}"
+        )));
+    }
     Ok(s)
 }