files: percent-decode url path (#2398)

Co-authored-by: Rob Ede <robjtede@icloud.com>

actix-files/CHANGES.md

@@ -1,8 +1,12 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+- `Files`: request URL paths with `%2F` are now rejected. [#2398]
+- `Files`: Fixed a regression where `%25` in the URL path is not decoded to `%` in the file path. [#2398]
 - Minimum supported Rust version (MSRV) is now 1.54.
 
+[#2398]: https://github.com/actix/actix-web/pull/2398
+
 
 ## 0.6.0-beta.12 - 2021-12-29
 - No significant changes since `0.6.0-beta.11`.

actix-files/Cargo.toml

@@ -45,3 +45,4 @@ tokio-uring = { version = "0.1", optional = true }
 actix-rt = "2.2"
 actix-test = "0.1.0-beta.10"
 actix-web = "4.0.0-beta.18"
+tempfile = "3.2"

actix-files/src/error.rs

@@ -23,16 +23,23 @@ impl ResponseError for FilesError {
 
 #[allow(clippy::enum_variant_names)]
 #[derive(Display, Debug, PartialEq)]
+#[non_exhaustive]
 pub enum UriSegmentError {
     /// The segment started with the wrapped invalid character.
     #[display(fmt = "The segment started with the wrapped invalid character")]
     BadStart(char),
+
     /// The segment contained the wrapped invalid character.
     #[display(fmt = "The segment contained the wrapped invalid character")]
     BadChar(char),
+
     /// The segment ended with the wrapped invalid character.
     #[display(fmt = "The segment ended with the wrapped invalid character")]
     BadEnd(char),
+
+    /// The path is not a valid UTF-8 string after doing percent decoding.
+    #[display(fmt = "The path is not a valif UTF-8 string after percent-decoding")]
+    NotValidUtf8,
 }
 
 /// Return `BadRequest` for `UriSegmentError`

actix-files/src/files.rs

@@ -28,6 +28,7 @@ use crate::{
 ///
 /// `Files` service must be registered with `App::service()` method.
 ///
+/// # Examples
 /// ```
 /// use actix_web::App;
 /// use actix_files::Files;

actix-files/src/lib.rs

@@ -803,6 +803,38 @@ mod tests {
         let req = TestRequest::get().uri("/test/%43argo.toml").to_request();
         let res = test::call_service(&srv, req).await;
         assert_eq!(res.status(), StatusCode::OK);
+
+        // `%2F` == `/`
+        let req = TestRequest::get().uri("/test%2Ftest.binary").to_request();
+        let res = test::call_service(&srv, req).await;
+        assert_eq!(res.status(), StatusCode::NOT_FOUND);
+
+        let req = TestRequest::get().uri("/test/Cargo.toml%00").to_request();
+        let res = test::call_service(&srv, req).await;
+        assert_eq!(res.status(), StatusCode::NOT_FOUND);
+    }
+
+    #[actix_rt::test]
+    async fn test_percent_encoding_2() {
+        let tmpdir = tempfile::tempdir().unwrap();
+        let filename = match cfg!(unix) {
+            true => "ض:?#[]{}<>()@!$&'`|*+,;= %20.test",
+            false => "ض#[]{}()@!$&'`+,;= %20.test",
+        };
+        let filename_encoded = filename
+            .as_bytes()
+            .iter()
+            .map(|c| format!("%{:02X}", c))
+            .collect::<String>();
+        std::fs::File::create(tmpdir.path().join(filename)).unwrap();
+
+        let srv = test::init_service(App::new().service(Files::new("", tmpdir.path()))).await;
+
+        let req = TestRequest::get()
+            .uri(&format!("/{}", filename_encoded))
+            .to_request();
+        let res = test::call_service(&srv, req).await;
+        assert_eq!(res.status(), StatusCode::OK);
     }
 
     #[actix_rt::test]

actix-files/src/path_buf.rs

@@ -1,5 +1,5 @@
 use std::{
-    path::{Path, PathBuf},
+    path::{Component, Path, PathBuf},
     str::FromStr,
 };
 
@@ -26,8 +26,23 @@ impl PathBufWrap {
     pub fn parse_path(path: &str, hidden_files: bool) -> Result<Self, UriSegmentError> {
         let mut buf = PathBuf::new();
 
+        // equivalent to `path.split('/').count()`
+        let mut segment_count = path.matches('/').count() + 1;
+
+        // we can decode the whole path here (instead of per-segment decoding)
+        // because we will reject `%2F` in paths using `segement_count`.
+        let path = percent_encoding::percent_decode_str(path)
+            .decode_utf8()
+            .map_err(|_| UriSegmentError::NotValidUtf8)?;
+
+        // disallow decoding `%2F` into `/`
+        if segment_count != path.matches('/').count() + 1 {
+            return Err(UriSegmentError::BadChar('/'));
+        }
+
         for segment in path.split('/') {
             if segment == ".." {
+                segment_count -= 1;
                 buf.pop();
             } else if !hidden_files && segment.starts_with('.') {
                 return Err(UriSegmentError::BadStart('.'));
@@ -40,6 +55,7 @@ impl PathBufWrap {
             } else if segment.ends_with('<') {
                 return Err(UriSegmentError::BadEnd('<'));
             } else if segment.is_empty() {
+                segment_count -= 1;
                 continue;
             } else if cfg!(windows) && segment.contains('\\') {
                 return Err(UriSegmentError::BadChar('\\'));
@@ -48,6 +64,12 @@ impl PathBufWrap {
             }
         }
 
+        // make sure we agree with stdlib parser
+        for (i, component) in buf.components().enumerate() {
+            assert!(matches!(component, Component::Normal(_)));
+            assert!(i < segment_count);
+        }
+
         Ok(PathBufWrap(buf))
     }
 }