Skip to content

Commit 494bfb4

Browse files
committed
fix: harden security validation and deduplicate logic
1 parent 35e3307 commit 494bfb4

File tree

10 files changed

+189
-173
lines changed

10 files changed

+189
-173
lines changed
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@googleworkspace/cli": patch
3+
---
4+
5+
Security: Harden validate_resource_name and fix Gmail watch path traversal

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ let url = format!("{}?q={}", base_url, user_query);
125125
When a user-supplied string is used as a GCP resource identifier (project ID, topic name, space name, etc.) that gets embedded in a URL path, validate it first:
126126

127127
```rust
128-
// Validates the string contains only safe characters (alphanumeric, hyphens, underscores, dots, slashes)
128+
// Validates the string does not contain path traversal segments (`..`), control characters, or URL-breaking characters like `?` and `#`.
129129
let project = crate::helpers::validate_resource_name(&project_id)?;
130130
let url = format!("https://pubsub.googleapis.com/v1/projects/{}/topics/my-topic", project);
131131
```

src/generate_skills.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,9 @@ struct SkillIndexEntry {
6868
/// Entry point for `gws generate-skills`.
6969
pub async fn handle_generate_skills(args: &[String]) -> Result<(), GwsError> {
7070
let output_dir = parse_output_dir(args);
71-
let output_path = Path::new(&output_dir);
71+
// Validate output_dir to prevent path traversal
72+
let output_path_buf = crate::validate::validate_safe_output_dir(&output_dir)?;
73+
let output_path = output_path_buf.as_path();
7274
let filter = parse_filter(args);
7375
let mut index: Vec<SkillIndexEntry> = Vec::new();
7476

src/helpers/calendar.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ async fn handle_agenda(matches: &ArgMatches) -> Result<(), GwsError> {
302302
async move {
303303
let events_url = format!(
304304
"https://www.googleapis.com/calendar/v3/calendars/{}/events",
305-
crate::helpers::encode_path_segment(&cal.id),
305+
crate::validate::encode_path_segment(&cal.id),
306306
);
307307

308308
let resp = crate::client::send_with_retry(|| {

src/helpers/events/renew.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ pub(super) async fn handle_renew(
3737

3838
if let Some(name) = config.name {
3939
// Reactivate a specific subscription
40-
let name = crate::helpers::validate_resource_name(&name)?;
40+
let name = crate::validate::validate_resource_name(&name)?;
4141
eprintln!("Reactivating subscription: {name}");
4242
let resp = client
4343
.post(format!(
@@ -79,7 +79,7 @@ pub(super) async fn handle_renew(
7979
let to_renew = filter_subscriptions_to_renew(subs, now, within_secs);
8080

8181
for name in to_renew {
82-
let name = crate::helpers::validate_resource_name(&name)?;
82+
let name = crate::validate::validate_resource_name(&name)?;
8383
eprintln!("Renewing {name}...");
8484
let _ = client
8585
.post(format!(

src/helpers/events/subscribe.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ fn parse_subscribe_args(matches: &ArgMatches) -> Result<SubscribeConfig, GwsErro
4848
builder.project(Some(ProjectId(project)));
4949
}
5050
if let Some(subscription) = matches.get_one::<String>("subscription") {
51-
crate::helpers::validate_resource_name(subscription)?;
51+
crate::validate::validate_resource_name(subscription)?;
5252
builder.subscription(Some(SubscriptionName(subscription.clone())));
5353
}
5454
if let Some(max_messages) = matches
@@ -124,7 +124,7 @@ pub(super) async fn handle_subscribe(
124124
// Full setup: create Pub/Sub topic + subscription + Workspace Events subscription
125125
let target = config.target.clone().unwrap();
126126
let project =
127-
crate::helpers::validate_resource_name(&config.project.clone().unwrap().0)?
127+
crate::validate::validate_resource_name(&config.project.clone().unwrap().0)?
128128
.to_string();
129129
let event_types_str: Vec<&str> =
130130
config.event_types.iter().map(|s| s.as_str()).collect();

src/helpers/gmail/watch.rs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ pub(super) async fn handle_watch(
3737

3838
let suffix = format!("{:08x}", rand::random::<u32>());
3939
let topic = if let Some(ref t) = config.topic {
40-
crate::helpers::validate_resource_name(t)?.to_string()
40+
crate::validate::validate_resource_name(t)?.to_string()
4141
} else {
42-
let project = crate::helpers::validate_resource_name(&project)?;
42+
let project = crate::validate::validate_resource_name(&project)?;
4343
let t = format!("projects/{project}/topics/gws-gmail-watch-{suffix}");
4444
// Create Pub/Sub topic
4545
eprintln!("Creating Pub/Sub topic: {t}");
@@ -98,7 +98,7 @@ pub(super) async fn handle_watch(
9898
t
9999
};
100100

101-
let project = crate::helpers::validate_resource_name(&project)?;
101+
let project = crate::validate::validate_resource_name(&project)?;
102102
let sub = format!("projects/{project}/subscriptions/gws-gmail-watch-{suffix}");
103103

104104
// 3. Create Pub/Sub subscription
@@ -209,14 +209,15 @@ pub(super) async fn handle_watch(
209209
eprintln!("\nCleaning up Pub/Sub resources...");
210210
let _ = client
211211
.delete(format!(
212-
"https://pubsub.googleapis.com/v1/{pubsub_subscription}"
212+
"https://pubsub.googleapis.com/v1/{}",
213+
pubsub_subscription
213214
))
214215
.bearer_auth(&pubsub_token)
215216
.send()
216217
.await;
217218
if let Some(ref topic) = topic_name {
218219
let _ = client
219-
.delete(format!("https://pubsub.googleapis.com/v1/{topic}"))
220+
.delete(format!("https://pubsub.googleapis.com/v1/{}", topic))
220221
.bearer_auth(&pubsub_token)
221222
.send()
222223
.await;
@@ -229,9 +230,9 @@ pub(super) async fn handle_watch(
229230
pubsub_subscription
230231
);
231232
if let Some(ref topic) = topic_name {
232-
eprintln!("Pub/Sub topic: {topic}");
233+
eprintln!("Pub/Sub topic: {}", topic);
233234
}
234-
eprintln!("Pub/Sub subscription: {pubsub_subscription}");
235+
eprintln!("Pub/Sub subscription: {}", pubsub_subscription);
235236
eprintln!("Note: Gmail watch expires after 7 days. Re-run +watch to renew.");
236237
}
237238
}
@@ -432,7 +433,10 @@ async fn fetch_and_output_messages(
432433
let json_str =
433434
serde_json::to_string_pretty(&full_msg).unwrap_or_else(|_| "{}".to_string());
434435
if let Some(dir) = output_dir {
435-
let path = dir.join(format!("{msg_id}.json"));
436+
let path = dir.join(format!(
437+
"{}.json",
438+
crate::validate::encode_path_segment(&msg_id)
439+
));
436440
if let Err(e) = std::fs::write(&path, &json_str) {
437441
eprintln!("Warning: failed to write {}: {e}", path.display());
438442
} else {

src/helpers/mod.rs

Lines changed: 0 additions & 153 deletions
Original file line numberDiff line numberDiff line change
@@ -63,156 +63,3 @@ pub fn get_helper(service: &str) -> Option<Box<dyn Helper>> {
6363
_ => None,
6464
}
6565
}
66-
67-
// ---------------------------------------------------------------------------
68-
// URL safety helpers
69-
// ---------------------------------------------------------------------------
70-
71-
/// Percent-encode a value for use as a single URL path segment (e.g., file ID,
72-
/// calendar ID, message ID). All non-alphanumeric characters are encoded.
73-
pub(crate) fn encode_path_segment(s: &str) -> String {
74-
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
75-
utf8_percent_encode(s, NON_ALPHANUMERIC).to_string()
76-
}
77-
78-
/// Validate a multi-segment resource name (e.g., `spaces/ABC`, `subscriptions/123`).
79-
/// Rejects path traversal and control characters while preserving the intentional
80-
/// `/`-delimited structure. Returns the validated name or an error with a clear
81-
/// message suitable for LLM callers.
82-
pub(crate) fn validate_resource_name(s: &str) -> Result<&str, GwsError> {
83-
if s.is_empty() {
84-
return Err(GwsError::Validation(
85-
"Resource name must not be empty".to_string(),
86-
));
87-
}
88-
if s.split('/').any(|seg| seg == "..") {
89-
return Err(GwsError::Validation(format!(
90-
"Resource name must not contain path traversal ('..') segments: {s}"
91-
)));
92-
}
93-
if s.contains('\0') || s.chars().any(|c| c.is_control()) {
94-
return Err(GwsError::Validation(format!(
95-
"Resource name contains invalid characters: {s}"
96-
)));
97-
}
98-
// Reject URL-special characters that could inject query params or fragments
99-
if s.contains('?') || s.contains('#') {
100-
return Err(GwsError::Validation(format!(
101-
"Resource name must not contain '?' or '#': {s}"
102-
)));
103-
}
104-
Ok(s)
105-
}
106-
107-
#[cfg(test)]
108-
mod tests {
109-
use super::*;
110-
111-
// -- encode_path_segment --------------------------------------------------
112-
113-
#[test]
114-
fn test_encode_path_segment_plain_id() {
115-
assert_eq!(encode_path_segment("abc123"), "abc123");
116-
}
117-
118-
#[test]
119-
fn test_encode_path_segment_email() {
120-
// Calendar IDs are often email addresses
121-
let encoded = encode_path_segment("user@gmail.com");
122-
assert!(!encoded.contains('@'));
123-
assert!(!encoded.contains('.'));
124-
}
125-
126-
#[test]
127-
fn test_encode_path_segment_query_injection() {
128-
// LLM might include query params in an ID by mistake
129-
let encoded = encode_path_segment("fileid?fields=name");
130-
assert!(!encoded.contains('?'));
131-
assert!(!encoded.contains('='));
132-
}
133-
134-
#[test]
135-
fn test_encode_path_segment_fragment_injection() {
136-
let encoded = encode_path_segment("fileid#section");
137-
assert!(!encoded.contains('#'));
138-
}
139-
140-
#[test]
141-
fn test_encode_path_segment_path_traversal() {
142-
// Encoding makes traversal segments harmless
143-
let encoded = encode_path_segment("../../etc/passwd");
144-
assert!(!encoded.contains('/'));
145-
assert!(!encoded.contains(".."));
146-
}
147-
148-
#[test]
149-
fn test_encode_path_segment_unicode() {
150-
// LLM might pass unicode characters
151-
let encoded = encode_path_segment("日本語ID");
152-
assert!(!encoded.contains('日'));
153-
}
154-
155-
#[test]
156-
fn test_encode_path_segment_spaces() {
157-
let encoded = encode_path_segment("my file id");
158-
assert!(!encoded.contains(' '));
159-
}
160-
161-
#[test]
162-
fn test_encode_path_segment_already_encoded() {
163-
// LLM might double-encode by passing pre-encoded values
164-
let encoded = encode_path_segment("user%40gmail.com");
165-
// The % itself gets encoded to %25, so %40 becomes %2540
166-
// This prevents double-encoding issues at the HTTP layer
167-
assert!(encoded.contains("%2540"));
168-
}
169-
170-
// -- validate_resource_name -----------------------------------------------
171-
172-
#[test]
173-
fn test_validate_resource_name_valid() {
174-
assert!(validate_resource_name("spaces/ABC123").is_ok());
175-
assert!(validate_resource_name("subscriptions/my-sub").is_ok());
176-
assert!(validate_resource_name("@default").is_ok());
177-
assert!(validate_resource_name("projects/p1/topics/t1").is_ok());
178-
}
179-
180-
#[test]
181-
fn test_validate_resource_name_traversal() {
182-
assert!(validate_resource_name("../../etc/passwd").is_err());
183-
assert!(validate_resource_name("spaces/../other").is_err());
184-
assert!(validate_resource_name("..").is_err());
185-
}
186-
187-
#[test]
188-
fn test_validate_resource_name_control_chars() {
189-
assert!(validate_resource_name("spaces/\0bad").is_err());
190-
assert!(validate_resource_name("spaces/\nbad").is_err());
191-
assert!(validate_resource_name("spaces/\rbad").is_err());
192-
assert!(validate_resource_name("spaces/\tbad").is_err());
193-
}
194-
195-
#[test]
196-
fn test_validate_resource_name_empty() {
197-
assert!(validate_resource_name("").is_err());
198-
}
199-
200-
#[test]
201-
fn test_validate_resource_name_query_injection() {
202-
// LLMs might append query strings or fragments to resource names
203-
assert!(validate_resource_name("spaces/ABC?key=val").is_err());
204-
assert!(validate_resource_name("spaces/ABC#fragment").is_err());
205-
}
206-
207-
#[test]
208-
fn test_validate_resource_name_error_messages_are_clear() {
209-
let err = validate_resource_name("").unwrap_err();
210-
assert!(err.to_string().contains("must not be empty"));
211-
212-
let err = validate_resource_name("../bad").unwrap_err();
213-
assert!(err.to_string().contains("path traversal"));
214-
215-
let err = validate_resource_name("bad\0id").unwrap_err();
216-
assert!(err.to_string().contains("invalid characters"));
217-
}
218-
}

src/helpers/workflows.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ async fn handle_meeting_prep(matches: &ArgMatches) -> Result<(), GwsError> {
383383

384384
let events_url = format!(
385385
"https://www.googleapis.com/calendar/v3/calendars/{}/events",
386-
crate::helpers::encode_path_segment(calendar_id),
386+
crate::validate::encode_path_segment(calendar_id),
387387
);
388388
let events_json = get_json(
389389
&client,
@@ -459,7 +459,7 @@ async fn handle_email_to_task(matches: &ArgMatches) -> Result<(), GwsError> {
459459
// 1. Fetch the email
460460
let msg_url = format!(
461461
"https://gmail.googleapis.com/gmail/v1/users/me/messages/{}",
462-
crate::helpers::encode_path_segment(message_id),
462+
crate::validate::encode_path_segment(message_id),
463463
);
464464
let msg_json = get_json(
465465
&client,
@@ -495,7 +495,7 @@ async fn handle_email_to_task(matches: &ArgMatches) -> Result<(), GwsError> {
495495
"notes": format!("From email: {}\n\n{}", message_id, snippet),
496496
});
497497

498-
let tasklist = crate::helpers::validate_resource_name(tasklist)?;
498+
let tasklist = crate::validate::validate_resource_name(tasklist)?;
499499
let task_url = format!(
500500
"https://tasks.googleapis.com/tasks/v1/lists/{}/tasks",
501501
tasklist,
@@ -628,7 +628,7 @@ async fn handle_file_announce(matches: &ArgMatches) -> Result<(), GwsError> {
628628
// 1. Fetch file metadata from Drive
629629
let file_url = format!(
630630
"https://www.googleapis.com/drive/v3/files/{}",
631-
crate::helpers::encode_path_segment(file_id),
631+
crate::validate::encode_path_segment(file_id),
632632
);
633633
let file_json = get_json(
634634
&client,
@@ -653,7 +653,7 @@ async fn handle_file_announce(matches: &ArgMatches) -> Result<(), GwsError> {
653653
.unwrap_or_else(|| format!("📎 {file_name}\n{file_link}"));
654654

655655
let chat_body = json!({ "text": msg_text });
656-
let space = crate::helpers::validate_resource_name(space)?;
656+
let space = crate::validate::validate_resource_name(space)?;
657657
let chat_url = format!("https://chat.googleapis.com/v1/{}/messages", space);
658658

659659
let chat_resp = client

0 commit comments

Comments
 (0)