Repository: ducaale/xh (rust)

Issue:
Download tries to write to subfolder
Command:
```
xh -d 'https://huggingface.co/rhasspy/piper-voices/resolve/v1.0.0/en/en_GB/cori/high/en_GB-cori-high.onnx.json'
```
xh output (parts):
```
content-disposition: inline; filename*=UTF-8''en%2Fen_GB%2Fcori%2Fhigh%2Fen_GB-cori-high.onnx.json; filename="en/en_GB/cori/high/en_GB-cori-high.onnx.json";
```
```
xh: error: No such file or directory (os error 2)
```
strace output:
```
openat(AT_FDCWD, "en/en_GB/cori/high/en_GB-cori-high.onnx.json", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = -1 ENOENT (No such file or directory)
```

I haven't tested if this also respects `..` or `/…`, but if it does, this is a minor security vulnerability (or major, if you're using xh as root…) If it doesn't, it's merely a minor nuisance and an inconsistency with httpie.

```
➜ xh --version
xh 0.22.0
+native-tls +rustls
```
(from nixpkgs-unstable)

Pull request description:
Prevent directory traversal in server-supplied filenames
If the `Content-Disposition` header includes directory separators (e.g. `/`) then we now only take the base filename. Including the directories is a vulnerability.

Originally fixed in 028cbb0165af54123a4829162a6a00f46e8dce74 but then broken again in 330d3f2ed4e1af82ef89fefce2e6e84a8ac66330. This time I added a regression test.

Fixes #378.

(Commit based on v0.22.0, we could do a bugfix release instead of a feature release.)

Patch:
diff --git a/src/download.rs b/src/download.rs
index 251c685..25181c3 100644
--- a/src/download.rs
+++ b/src/download.rs
@@ -28,8 +28,9 @@ fn get_content_length(headers: &HeaderMap) -> Option<u64> {
 fn get_file_name(response: &Response, orig_url: &reqwest::Url) -> String {
     fn from_header(response: &Response) -> Option<String> {
         let quoted = Regex::new("filename=\"([^\"]*)\"").unwrap();
-        // Against the spec, but used by e.g. Github's zip downloads
+        // Alternative form:
         let unquoted = Regex::new("filename=([^;=\"]*)").unwrap();
+        // TODO: support "filename*" version
 
         let header = response.headers().get(CONTENT_DISPOSITION)?.to_str().ok()?;
         let caps = quoted
@@ -51,11 +52,13 @@ fn get_file_name(response: &Response, orig_url: &reqwest::Url) -> String {
         mime2ext(mimetype)
     }
 
-    let mut filename = from_header(response)
+    let filename = from_header(response)
         .or_else(|| from_url(orig_url))
         .unwrap_or_else(|| "index".to_string());
 
-    filename = filename.trim().trim_start_matches('.').to_string();
+    let filename = filename.split(std::path::is_separator).last().unwrap();
+
+    let mut filename = filename.trim().trim_start_matches('.').to_string();
 
     if !filename.contains('.') {
         if let Some(extension) = guess_extension(response) {


Test patch:
diff --git a/tests/cli.rs b/tests/cli.rs
index 9e774f9..aa52af5 100644
--- a/tests/cli.rs
+++ b/tests/cli.rs
@@ -551,6 +551,49 @@ fn download_supplied_unquoted_filename() {
     );
 }
 
+#[test]
+fn download_filename_with_directory_traversal() {
+    let dir = tempdir().unwrap();
+    let server = server::http(|_req| async move {
+        hyper::Response::builder()
+            .header(
+                "Content-Disposition",
+                r#"attachment; filename="foo/baz/bar""#,
+            )
+            .body("file".into())
+            .unwrap()
+    });
+
+    get_command()
+        .args(["--download", &server.base_url()])
+        .current_dir(&dir)
+        .assert()
+        .success();
+    assert_eq!(fs::read_to_string(dir.path().join("bar")).unwrap(), "file");
+}
+
+#[cfg(windows)]
+#[test]
+fn download_filename_with_windows_directory_traversal() {
+    let dir = tempdir().unwrap();
+    let server = server::http(|_req| async move {
+        hyper::Response::builder()
+            .header(
+                "Content-Disposition",
+                r#"attachment; filename="foo\baz\bar""#,
+            )
+            .body("file".into())
+            .unwrap()
+    });
+
+    get_command()
+        .args(["--download", &server.base_url()])
+        .current_dir(&dir)
+        .assert()
+        .success();
+    assert_eq!(fs::read_to_string(dir.path().join("bar")).unwrap(), "file");
+}
+
 // TODO: test implicit download filenames
 // For this we have to pretend the output is a tty
 // This intersects with both #41 and #59