files: percent-decode url path

[aliemjay]

PR Type

Bug fix

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt.
• (Team) Label with affected crates and semver status.

Overview

re.match_info().path() may still have some percent-encoded chars.

This is partly a regression after upgrading actix-router because of actix/actix-net#357

[robjtede]

After some thought about this, I think the best way to move this forward is to just decode the remaining chars we want to, excluding /. I just don't see a good reason to allow decoding this character and the security doubts make it a good choice to exclude in my opinion.

Since percent-encoding can't target character sets for decoding, we can use the Quoter from actix-router.

Thoughts on this patch @aliemjay ?

diff --git a/actix-files/Cargo.toml b/actix-files/Cargo.toml
index 745e3afee..8b7a83f96 100644
--- a/actix-files/Cargo.toml
+++ b/actix-files/Cargo.toml
@@ -25,6 +25,7 @@ experimental-io-uring = ["actix-web/experimental-io-uring", "tokio-uring"]
 actix-http = "3.0.0-beta.17"
 actix-service = "2"
 actix-utils = "3"
+actix-router = "0.5.0-beta.3"
 actix-web = { version = "4.0.0-beta.18", default-features = false }
 
 askama_escape = "0.10"
diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs
index 242eda4be..a7da6452e 100644
--- a/actix-files/src/error.rs
+++ b/actix-files/src/error.rs
@@ -27,15 +27,14 @@ 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`
diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs
index 5d141d6ea..a32e19d2a 100644
--- a/actix-files/src/lib.rs
+++ b/actix-files/src/lib.rs
@@ -809,7 +809,7 @@ mod tests {
             .uri("/test/%2F..%2F..%2Ftests%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 e46d3f232..2e6a68ab0 100644
--- a/actix-files/src/path_buf.rs
+++ b/actix-files/src/path_buf.rs
@@ -3,11 +3,17 @@ use std::{
     str::FromStr,
 };
 
+use actix_router::Quoter;
 use actix_utils::future::{ready, Ready};
 use actix_web::{dev::Payload, FromRequest, HttpRequest};
 
 use crate::error::UriSegmentError;
 
+thread_local! {
+    /// Replaces remaining reserved characters except `%2F` (forward slash "/").
+    static FILENAME_QUOTER: Quoter = Quoter::new(b"%+", b"/");
+}
+
 #[derive(Debug, PartialEq, Eq)]
 pub(crate) struct PathBufWrap(PathBuf);
 
@@ -26,10 +32,6 @@ 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();
@@ -48,7 +50,11 @@ impl PathBufWrap {
             } else if cfg!(windows) && segment.contains('\\') {
                 return Err(UriSegmentError::BadChar('\\'));
             } else {
-                buf.push(segment)
+                let decoded_segment = FILENAME_QUOTER
+                    .with(|q| q.requote(segment.as_bytes()))
+                    .unwrap_or_else(|| segment.to_owned());
+
+                buf.push(decoded_segment)
             }
         }

[aliemjay]

We need then to ouright reject these paths. Leaving some encoded chars seems wrong to me.

Will look into it later today.

[robjtede]

They are being rejected with the 404. The only edge case left is if the fs really does contain a file with %2F in the name. And if so, you're 1) asking for trouble 2) possibly really actually want to serve such a stupid file.

IMO, this is such a niche case that it is not worth the performance hit of re-scanning the decoded segment for "%2F". Plus, it's not really a security problem to leave it.

[robjtede]

cool, this approach is fine

[robjtede]

needs a comment in here about why the "full" decode is acceptable

[aliemjay]

Files with %2F in the name should be already encoded to %252F, so this shouldn't be a
problem.

I also added some code to double-check the resulting path using std::path::Path because of Windows, which seems to have many edge cases.

One Idea I have now is to expose PathBufWrap (under a better name ofc).