Quoter::requote returns Vec<u8> (actix#2613)

lb-cake · Jan 31, 2022 · fd412a8 · fd412a8
1 parent cd511af
commit fd412a8
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 16 deletions.
diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md
@@ -1,6 +1,9 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+- `Quoter::requote` now returns `Option<Vec<u8>>`. [#2613]
+
+[#2613]: https://github.com/actix/actix-web/pull/2613
 
 
 ## 0.5.0-rc.2 - 2022-01-21

diff --git a/actix-router/src/de.rs b/actix-router/src/de.rs
@@ -52,7 +52,7 @@ macro_rules! parse_value {
             V: Visitor<'de>,
         {
             let decoded = FULL_QUOTER
-                .with(|q| q.requote(self.value.as_bytes()))
+                .with(|q| q.requote_str_lossy(self.value))
                 .map(Cow::Owned)
                 .unwrap_or(Cow::Borrowed(self.value));
 
@@ -332,7 +332,7 @@ impl<'de> Deserializer<'de> for Value<'de> {
     where
         V: Visitor<'de>,
     {
-        match FULL_QUOTER.with(|q| q.requote(self.value.as_bytes())) {
+        match FULL_QUOTER.with(|q| q.requote_str_lossy(self.value)) {
             Some(s) => visitor.visit_string(s),
             None => visitor.visit_borrowed_str(self.value),
         }
@@ -342,7 +342,7 @@ impl<'de> Deserializer<'de> for Value<'de> {
     where
         V: Visitor<'de>,
     {
-        match FULL_QUOTER.with(|q| q.requote(self.value.as_bytes())) {
+        match FULL_QUOTER.with(|q| q.requote_str_lossy(self.value)) {
             Some(s) => visitor.visit_byte_buf(s.into()),
             None => visitor.visit_borrowed_bytes(self.value.as_bytes()),
         }

diff --git a/actix-router/src/quoter.rs b/actix-router/src/quoter.rs
@@ -66,8 +66,13 @@ impl Quoter {
 
     /// Re-quotes... ?
     ///
-    /// Returns `None` when no modification to the original string was required.
-    pub fn requote(&self, val: &[u8]) -> Option<String> {
+    /// Returns `None` when no modification to the original byte string was required.
+    ///
+    /// Non-ASCII bytes are accepted as valid input.
+    ///
+    /// Behavior for invalid/incomplete percent-encoding sequences is unspecified and may include removing
+    /// the invalid sequence from the output or passing it as it is.
+    pub fn requote(&self, val: &[u8]) -> Option<Vec<u8>> {
         let mut has_pct = 0;
         let mut pct = [b'%', 0, 0];
         let mut idx = 0;
@@ -121,7 +126,12 @@ impl Quoter {
             idx += 1;
         }
 
-        cloned.map(|data| String::from_utf8_lossy(&data).into_owned())
+        cloned
+    }
+
+    pub(crate) fn requote_str_lossy(&self, val: &str) -> Option<String> {
+        self.requote(val.as_bytes())
+            .map(|data| String::from_utf8_lossy(&data).into_owned())
     }
 }
 
@@ -201,14 +211,29 @@ mod tests {
     #[test]
     fn custom_quoter() {
         let q = Quoter::new(b"", b"+");
-        assert_eq!(q.requote(b"/a%25c").unwrap(), "/a%c");
-        assert_eq!(q.requote(b"/a%2Bc").unwrap(), "/a%2Bc");
+        assert_eq!(q.requote(b"/a%25c").unwrap(), b"/a%c");
+        assert_eq!(q.requote(b"/a%2Bc").unwrap(), b"/a%2Bc");
+
+        let q = Quoter::new(b"%+", b"/");
+        assert_eq!(q.requote(b"/a%25b%2Bc").unwrap(), b"/a%b+c");
+        assert_eq!(q.requote(b"/a%2fb").unwrap(), b"/a%2fb");
+        assert_eq!(q.requote(b"/a%2Fb").unwrap(), b"/a%2Fb");
+        assert_eq!(q.requote(b"/a%0Ab").unwrap(), b"/a\nb");
+        assert_eq!(q.requote(b"/a%FE\xffb").unwrap(), b"/a\xfe\xffb");
+        assert_eq!(q.requote(b"/a\xfe\xffb"), None);
+    }
+
+    #[test]
+    fn non_ascii() {
+        let q = Quoter::new(b"%+", b"/");
+        assert_eq!(q.requote(b"/a%FE\xffb").unwrap(), b"/a\xfe\xffb");
+        assert_eq!(q.requote(b"/a\xfe\xffb"), None);
+    }
 
+    #[test]
+    fn invalid_sequences() {
         let q = Quoter::new(b"%+", b"/");
-        assert_eq!(q.requote(b"/a%25b%2Bc").unwrap(), "/a%b+c");
-        assert_eq!(q.requote(b"/a%2fb").unwrap(), "/a%2fb");
-        assert_eq!(q.requote(b"/a%2Fb").unwrap(), "/a%2Fb");
-        assert_eq!(q.requote(b"/a%0Ab").unwrap(), "/a\nb");
+        assert_eq!(q.requote(b"/a%2x%2X%%").unwrap(), b"/a%2x%2X");
     }
 
     #[test]

diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs
@@ -15,14 +15,14 @@ pub struct Url {
 impl Url {
     #[inline]
     pub fn new(uri: http::Uri) -> Url {
-        let path = DEFAULT_QUOTER.with(|q| q.requote(uri.path().as_bytes()));
+        let path = DEFAULT_QUOTER.with(|q| q.requote_str_lossy(uri.path()));
         Url { uri, path }
     }
 
     #[inline]
     pub fn new_with_quoter(uri: http::Uri, quoter: &Quoter) -> Url {
         Url {
-            path: quoter.requote(uri.path().as_bytes()),
+            path: quoter.requote_str_lossy(uri.path()),
             uri,
         }
     }
@@ -45,13 +45,13 @@ impl Url {
     #[inline]
     pub fn update(&mut self, uri: &http::Uri) {
         self.uri = uri.clone();
-        self.path = DEFAULT_QUOTER.with(|q| q.requote(uri.path().as_bytes()));
+        self.path = DEFAULT_QUOTER.with(|q| q.requote_str_lossy(uri.path()));
     }
 
     #[inline]
     pub fn update_with_quoter(&mut self, uri: &http::Uri, quoter: &Quoter) {
         self.uri = uri.clone();
-        self.path = quoter.requote(uri.path().as_bytes());
+        self.path = quoter.requote_str_lossy(uri.path());
     }
 }