Skip to content

Commit cc5a973

Browse files
committed
fix(security): validate --upload and --output paths against traversal
The --upload and --output flags accepted arbitrary file paths without validation, allowing path traversal (../../.ssh/id_rsa) and symlink escape attacks. This is especially dangerous when the CLI is invoked by an AI agent that may receive adversarial input. Add validate_safe_file_path() to validate.rs and call it in main.rs before any file I/O. Rejects paths that resolve outside CWD, contain control characters, or follow symlinks to external locations.
1 parent e3e1e86 commit cc5a973

File tree

3 files changed

+122
-0
lines changed

3 files changed

+122
-0
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+
fix(security): validate --upload and --output file paths against traversal

src/main.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,14 @@ async fn run() -> Result<(), GwsError> {
217217
.flatten()
218218
.map(|s| s.as_str());
219219

220+
// Validate file paths against traversal before any I/O.
221+
if let Some(p) = upload_path {
222+
crate::validate::validate_safe_file_path(p, "--upload")?;
223+
}
224+
if let Some(p) = output_path {
225+
crate::validate::validate_safe_file_path(p, "--output")?;
226+
}
227+
220228
let dry_run = matched_args.get_flag("dry-run");
221229

222230
// Build pagination config from flags

src/validate.rs

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,50 @@ pub fn validate_safe_dir_path(dir: &str) -> Result<PathBuf, GwsError> {
118118
Ok(canonical)
119119
}
120120

121+
/// Validates that a file path (e.g. `--upload` or `--output`) is safe.
122+
///
123+
/// Rejects paths that escape above CWD via `..` traversal, contain
124+
/// control characters, or follow symlinks to locations outside CWD.
125+
/// Absolute paths are allowed for `--upload` (reading an existing file)
126+
/// but the resolved target must not escape CWD for `--output`.
127+
pub fn validate_safe_file_path(path_str: &str, flag_name: &str) -> Result<PathBuf, GwsError> {
128+
reject_control_chars(path_str, flag_name)?;
129+
130+
let path = Path::new(path_str);
131+
let cwd = std::env::current_dir()
132+
.map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))?;
133+
134+
let resolved = if path.is_absolute() {
135+
path.to_path_buf()
136+
} else {
137+
cwd.join(path)
138+
};
139+
140+
// For existing files, canonicalize to resolve symlinks.
141+
// For non-existing files, normalize the path.
142+
let canonical = if resolved.exists() {
143+
resolved.canonicalize().map_err(|e| {
144+
GwsError::Validation(format!("Failed to resolve {flag_name} '{}': {e}", path_str))
145+
})?
146+
} else {
147+
normalize_non_existing(&resolved)?
148+
};
149+
150+
let canonical_cwd = cwd.canonicalize().map_err(|e| {
151+
GwsError::Validation(format!("Failed to canonicalize current directory: {e}"))
152+
})?;
153+
154+
if !canonical.starts_with(&canonical_cwd) {
155+
return Err(GwsError::Validation(format!(
156+
"{flag_name} '{}' resolves to '{}' which is outside the current directory",
157+
path_str,
158+
canonical.display()
159+
)));
160+
}
161+
162+
Ok(canonical)
163+
}
164+
121165
/// Rejects strings containing null bytes or ASCII control characters
122166
/// (including DEL, 0x7F).
123167
fn reject_control_chars(value: &str, flag_name: &str) -> Result<(), GwsError> {
@@ -566,4 +610,69 @@ mod tests {
566610
fn test_validate_api_identifier_empty() {
567611
assert!(validate_api_identifier("").is_err());
568612
}
613+
614+
// --- validate_safe_file_path ---
615+
616+
#[test]
617+
#[serial]
618+
fn test_file_path_relative_is_ok() {
619+
let dir = tempdir().unwrap();
620+
let canonical_dir = dir.path().canonicalize().unwrap();
621+
fs::write(canonical_dir.join("test.txt"), "data").unwrap();
622+
623+
let saved_cwd = std::env::current_dir().unwrap();
624+
std::env::set_current_dir(&canonical_dir).unwrap();
625+
626+
let result = validate_safe_file_path("test.txt", "--upload");
627+
std::env::set_current_dir(&saved_cwd).unwrap();
628+
629+
assert!(result.is_ok(), "expected Ok, got: {result:?}");
630+
}
631+
632+
#[test]
633+
#[serial]
634+
fn test_file_path_rejects_traversal() {
635+
let dir = tempdir().unwrap();
636+
let canonical_dir = dir.path().canonicalize().unwrap();
637+
638+
let saved_cwd = std::env::current_dir().unwrap();
639+
std::env::set_current_dir(&canonical_dir).unwrap();
640+
641+
let result = validate_safe_file_path("../../etc/passwd", "--upload");
642+
std::env::set_current_dir(&saved_cwd).unwrap();
643+
644+
assert!(result.is_err(), "path traversal should be rejected");
645+
assert!(
646+
result.unwrap_err().to_string().contains("outside"),
647+
"error should mention 'outside'"
648+
);
649+
}
650+
651+
#[test]
652+
fn test_file_path_rejects_control_chars() {
653+
let result = validate_safe_file_path("file\x00.txt", "--output");
654+
assert!(result.is_err(), "null bytes should be rejected");
655+
}
656+
657+
#[test]
658+
#[serial]
659+
fn test_file_path_rejects_symlink_escape() {
660+
let dir = tempdir().unwrap();
661+
let canonical_dir = dir.path().canonicalize().unwrap();
662+
663+
// Create a symlink that points outside the directory
664+
#[cfg(unix)]
665+
{
666+
let link_path = canonical_dir.join("escape");
667+
std::os::unix::fs::symlink("/tmp", &link_path).unwrap();
668+
669+
let saved_cwd = std::env::current_dir().unwrap();
670+
std::env::set_current_dir(&canonical_dir).unwrap();
671+
672+
let result = validate_safe_file_path("escape/secret.txt", "--output");
673+
std::env::set_current_dir(&saved_cwd).unwrap();
674+
675+
assert!(result.is_err(), "symlink escape should be rejected");
676+
}
677+
}
569678
}

0 commit comments

Comments
 (0)