From cc3cc216d5647f0171447bc4ed56a5cfcdba5ed1 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Mon, 11 Oct 2021 08:47:35 +0300
Subject: [PATCH 1/7] files: percent-decode url path

---
 actix-files/Cargo.toml     |  1 +
 actix-files/src/lib.rs     | 23 +++++++++++++++++++++++
 actix-files/src/service.rs | 12 +++++++-----
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/actix-files/Cargo.toml b/actix-files/Cargo.toml
index eccf49a772b..e17eb9d9fe1 100644
--- a/actix-files/Cargo.toml
+++ b/actix-files/Cargo.toml
@@ -35,3 +35,4 @@ percent-encoding = "2.1"
 actix-rt = "2.2"
 actix-web = "4.0.0-beta.9"
 actix-test = "0.1.0-beta.3"
+tempfile = "3.2"
diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs
index 1eb091aaf58..895910fafb9 100644
--- a/actix-files/src/lib.rs
+++ b/actix-files/src/lib.rs
@@ -787,6 +787,29 @@ mod tests {
         assert_eq!(res.status(), StatusCode::OK);
     }
 
+    #[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]
     async fn test_serve_named_file() {
         let srv =
diff --git a/actix-files/src/service.rs b/actix-files/src/service.rs
index 09122c63e5a..30093796760 100644
--- a/actix-files/src/service.rs
+++ b/actix-files/src/service.rs
@@ -77,11 +77,13 @@ impl Service<ServiceRequest> for FilesService {
             )));
         }
 
-        let real_path =
-            match PathBufWrap::parse_path(req.match_info().path(), self.hidden_files) {
-                Ok(item) => item,
-                Err(e) => return Box::pin(ok(req.error_response(e))),
-            };
+        let path_decoded =
+            percent_encoding::percent_decode_str(req.match_info().path()).decode_utf8_lossy();
+
+        let real_path = match PathBufWrap::parse_path(&path_decoded, self.hidden_files) {
+            Ok(item) => item,
+            Err(e) => return Box::pin(ok(req.error_response(e))),
+        };
 
         if let Some(filter) = &self.path_filter {
             if !filter(real_path.as_ref(), req.head()) {

From 0c664cd79aae0889524f6066d77ead30a9530147 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Tue, 12 Oct 2021 05:32:11 +0300
Subject: [PATCH 2/7] dontuse lossy decoding

---
 actix-files/src/error.rs    |  3 +++
 actix-files/src/path_buf.rs |  4 ++++
 actix-files/src/service.rs  | 12 +++++-------
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs
index f8e32eef760..242eda4bec6 100644
--- a/actix-files/src/error.rs
+++ b/actix-files/src/error.rs
@@ -33,6 +33,9 @@ pub enum UriSegmentError {
     /// 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`
diff --git a/actix-files/src/path_buf.rs b/actix-files/src/path_buf.rs
index 76f589307af..1eef8e0099b 100644
--- a/actix-files/src/path_buf.rs
+++ b/actix-files/src/path_buf.rs
@@ -24,6 +24,10 @@ impl PathBufWrap {
     pub fn parse_path(path: &str, hidden_files: bool) -> Result<Self, UriSegmentError> {
         let mut buf = PathBuf::new();
 
+        let path = percent_encoding::percent_decode_str(path)
+            .decode_utf8()
+            .map_err(|_| UriSegmentError::NotValidUtf8)?;
+
         for segment in path.split('/') {
             if segment == ".." {
                 buf.pop();
diff --git a/actix-files/src/service.rs b/actix-files/src/service.rs
index 30093796760..09122c63e5a 100644
--- a/actix-files/src/service.rs
+++ b/actix-files/src/service.rs
@@ -77,13 +77,11 @@ impl Service<ServiceRequest> for FilesService {
             )));
         }
 
-        let path_decoded =
-            percent_encoding::percent_decode_str(req.match_info().path()).decode_utf8_lossy();
-
-        let real_path = match PathBufWrap::parse_path(&path_decoded, self.hidden_files) {
-            Ok(item) => item,
-            Err(e) => return Box::pin(ok(req.error_response(e))),
-        };
+        let real_path =
+            match PathBufWrap::parse_path(req.match_info().path(), self.hidden_files) {
+                Ok(item) => item,
+                Err(e) => return Box::pin(ok(req.error_response(e))),
+            };
 
         if let Some(filter) = &self.path_filter {
             if !filter(real_path.as_ref(), req.head()) {

From 82c0059811176e6f9ae07e39b9a7520a0008c81e Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Wed, 29 Dec 2021 14:35:01 +0300
Subject: [PATCH 3/7] document behavior

---
 actix-files/src/files.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/actix-files/src/files.rs b/actix-files/src/files.rs
index d1dd6739ddc..d55009d214c 100644
--- a/actix-files/src/files.rs
+++ b/actix-files/src/files.rs
@@ -28,6 +28,17 @@ use crate::{
 ///
 /// `Files` service must be registered with `App::service()` method.
 ///
+/// # Security Coniderations
+///
+/// When converting the request URL path into the target [file path](std::path::Path),
+/// `Files` service *does* decode *all* percent-encoded chars in the path string.
+/// One implication is that the resulting file path may have more components than the URL path
+/// as a result of decoding `%2F` into `/`.
+///
+/// Any middleware that is responsibe for validating the paths managed under `Files`
+/// should be aware of this behvaior.
+///
+/// # Examples
 /// ```
 /// use actix_web::App;
 /// use actix_files::Files;

From 0d823d74e06f2af1ccf9c87a59d7649df1b7d5c2 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Wed, 29 Dec 2021 15:34:03 +0300
Subject: [PATCH 4/7] more tests + changelog

---
 actix-files/CHANGES.md   | 4 ++++
 actix-files/src/files.rs | 8 ++++----
 actix-files/src/lib.rs   | 9 +++++++++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md
index c626fd3fb2a..78da2688fa5 100644
--- a/actix-files/CHANGES.md
+++ b/actix-files/CHANGES.md
@@ -1,8 +1,12 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+- `Files`: `%2F` in request URL path is now decoded to  `/` and thus functions as a path separator. [#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`.
diff --git a/actix-files/src/files.rs b/actix-files/src/files.rs
index d55009d214c..9d35472bc72 100644
--- a/actix-files/src/files.rs
+++ b/actix-files/src/files.rs
@@ -28,15 +28,15 @@ use crate::{
 ///
 /// `Files` service must be registered with `App::service()` method.
 ///
-/// # Security Coniderations
+/// # Percent-Encoding and Security Considerations
 ///
 /// When converting the request URL path into the target [file path](std::path::Path),
-/// `Files` service *does* decode *all* percent-encoded chars in the path string.
+/// `Files` service *does* decode *all* percent-encoded characters in the path string.
 /// One implication is that the resulting file path may have more components than the URL path
 /// as a result of decoding `%2F` into `/`.
 ///
-/// Any middleware that is responsibe for validating the paths managed under `Files`
-/// should be aware of this behvaior.
+/// Any middleware that is responsible for validating the paths managed under `Files`
+/// should be aware of this behavior.
 ///
 /// # Examples
 /// ```
diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs
index 050c7df6963..8ae937fbcee 100644
--- a/actix-files/src/lib.rs
+++ b/actix-files/src/lib.rs
@@ -802,6 +802,15 @@ 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/%2F..%2F..%2Ftests%2Ftest.binary").to_request();
+        let res = test::call_service(&srv, req).await;
+        assert_eq!(res.status(), StatusCode::OK);
+
+        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]

From 74b77679ed5b5cb69e0427a2677261e2dd821ce7 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Wed, 29 Dec 2021 15:37:07 +0300
Subject: [PATCH 5/7] fmt

---
 actix-files/CHANGES.md | 2 +-
 actix-files/src/lib.rs | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md
index 78da2688fa5..3e159099126 100644
--- a/actix-files/CHANGES.md
+++ b/actix-files/CHANGES.md
@@ -1,7 +1,7 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
-- `Files`: `%2F` in request URL path is now decoded to  `/` and thus functions as a path separator. [#2398]
+- `Files`: `%2F` in request URL path is now decoded to `/` and thus functions as a path separator. [#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.
 
diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs
index 8ae937fbcee..4af28ca9210 100644
--- a/actix-files/src/lib.rs
+++ b/actix-files/src/lib.rs
@@ -804,7 +804,9 @@ mod tests {
         assert_eq!(res.status(), StatusCode::OK);
 
         // `%2F` == `/`
-        let req = TestRequest::get().uri("/test/%2F..%2F..%2Ftests%2Ftest.binary").to_request();
+        let req = TestRequest::get()
+            .uri("/test/%2F..%2F..%2Ftests%2Ftest.binary")
+            .to_request();
         let res = test::call_service(&srv, req).await;
         assert_eq!(res.status(), StatusCode::OK);
 

From 6dacc342864d70433aa6afcb5c82e623b5cbf66c Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Tue, 4 Jan 2022 15:09:31 +0300
Subject: [PATCH 6/7] reject %2F in url paths

---
 actix-files/CHANGES.md      |  2 +-
 actix-files/src/error.rs    |  4 ++++
 actix-files/src/files.rs    | 10 ----------
 actix-files/src/lib.rs      |  6 ++----
 actix-files/src/path_buf.rs | 18 +++++++++++++++++-
 5 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md
index 3e159099126..501181d92cc 100644
--- a/actix-files/CHANGES.md
+++ b/actix-files/CHANGES.md
@@ -1,7 +1,7 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
-- `Files`: `%2F` in request URL path is now decoded to `/` and thus functions as a path separator. [#2398]
+- `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.
 
diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs
index 242eda4bec6..d28889e73f2 100644
--- a/actix-files/src/error.rs
+++ b/actix-files/src/error.rs
@@ -23,16 +23,20 @@ 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,
diff --git a/actix-files/src/files.rs b/actix-files/src/files.rs
index 9d35472bc72..adfb93232d1 100644
--- a/actix-files/src/files.rs
+++ b/actix-files/src/files.rs
@@ -28,16 +28,6 @@ use crate::{
 ///
 /// `Files` service must be registered with `App::service()` method.
 ///
-/// # Percent-Encoding and Security Considerations
-///
-/// When converting the request URL path into the target [file path](std::path::Path),
-/// `Files` service *does* decode *all* percent-encoded characters in the path string.
-/// One implication is that the resulting file path may have more components than the URL path
-/// as a result of decoding `%2F` into `/`.
-///
-/// Any middleware that is responsible for validating the paths managed under `Files`
-/// should be aware of this behavior.
-///
 /// # Examples
 /// ```
 /// use actix_web::App;
diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs
index 5d141d6eaa2..a11aa32c753 100644
--- a/actix-files/src/lib.rs
+++ b/actix-files/src/lib.rs
@@ -805,11 +805,9 @@ mod tests {
         assert_eq!(res.status(), StatusCode::OK);
 
         // `%2F` == `/`
-        let req = TestRequest::get()
-            .uri("/test/%2F..%2F..%2Ftests%2Ftest.binary")
-            .to_request();
+        let req = TestRequest::get().uri("/test%2Ftest.binary").to_request();
         let res = test::call_service(&srv, req).await;
-        assert_eq!(res.status(), StatusCode::OK);
+        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;
diff --git a/actix-files/src/path_buf.rs b/actix-files/src/path_buf.rs
index e46d3f23254..fdd69c10cfb 100644
--- a/actix-files/src/path_buf.rs
+++ b/actix-files/src/path_buf.rs
@@ -1,5 +1,5 @@
 use std::{
-    path::{Path, PathBuf},
+    path::{Component, Path, PathBuf},
     str::FromStr,
 };
 
@@ -26,12 +26,21 @@ 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;
+
         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('.'));
@@ -44,6 +53,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('\\'));
@@ -52,6 +62,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))
     }
 }

From 6f40b60bd4414b84caf6c913d82eff4be342a208 Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Tue, 4 Jan 2022 15:26:48 +0300
Subject: [PATCH 7/7] comment

---
 actix-files/src/path_buf.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/actix-files/src/path_buf.rs b/actix-files/src/path_buf.rs
index fdd69c10cfb..03b2cd76610 100644
--- a/actix-files/src/path_buf.rs
+++ b/actix-files/src/path_buf.rs
@@ -29,6 +29,8 @@ impl PathBufWrap {
         // 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)?;