Repository: actix/actix-web
Pull request #577 (2018-11-04 21:13:26)

decode reserved characters when extracting path with configuration
concerning #556

I limited default decoding to unreserved characters as per [RFC3986](
https://tools.ietf.org/html/rfc3986#section-2.4) those can be decoded anytime.

I then added an option to path extractor (enabled by default) to also decoded the reserved characters.

I'm open to discussion if you think anything can be improved

mockersf: a few issue when looking at RFC:
* Some characters are neither in reserved or unreserved sets: ``< > " \ ` { } |``. I decided to decode them on the second pass
* Non ascii characters are not mentioned in the RFC. They get decoded on the first pass to help match routes
DoumanAsh: Thank you, I have concern that we might consider it as breaking changes though.

@fafhrd91 If you have time, I'd like you to take a look too
DoumanAsh: >Non ascii characters are not mentioned in the RFC. They get decoded on the first pass to help match routes

Most browsers would re-encode non-ascii characters also
codecov[bot]: # [Codecov](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=h1) Report
> Merging [#577](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=desc) into [master](https://codecov.io/gh/actix/actix-web/commit/3b536ee96c0118e8cf452b28f6acab6085db22a6?src=pr&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `87.5%`.

[![Impacted file tree graph](https://codecov.io/gh/actix/actix-web/pull/577/graphs/tree.svg?width=650&token=zeNInsA6hP&height=150&src=pr)](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
+ Coverage   76.23%   76.28%   +0.05%     
==========================================
  Files         103      103              
  Lines       16825    16874      +49     
==========================================
+ Hits        12826    12872      +46     
- Misses       3999     4002       +3
```


| [Impacted Files](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tests/test\_handlers.rs](https://codecov.io/gh/actix/actix-web/pull/577/diff?src=pr&el=tree#diff-dGVzdHMvdGVzdF9oYW5kbGVycy5ycw==) | `88.75% <100%> (ø)` | :arrow_up: |
| [src/uri.rs](https://codecov.io/gh/actix/actix-web/pull/577/diff?src=pr&el=tree#diff-c3JjL3VyaS5ycw==) | `82.02% <76.47%> (-7.98%)` | :arrow_down: |
| [src/de.rs](https://codecov.io/gh/actix/actix-web/pull/577/diff?src=pr&el=tree#diff-c3JjL2RlLnJz) | `43.3% <83.87%> (+9.97%)` | :arrow_up: |
| [src/extractor.rs](https://codecov.io/gh/actix/actix-web/pull/577/diff?src=pr&el=tree#diff-c3JjL2V4dHJhY3Rvci5ycw==) | `81.44% <96.77%> (+1.74%)` | :arrow_up: |
| [src/middleware/cors.rs](https://codecov.io/gh/actix/actix-web/pull/577/diff?src=pr&el=tree#diff-c3JjL21pZGRsZXdhcmUvY29ycy5ycw==) | `75.47% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=footer). Last update [3b536ee...e085042](https://codecov.io/gh/actix/actix-web/pull/577?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mockersf: I agree it is a breaking change, the following characters are not decoded on first pass anymore: `'@:!$'()* ,`
it should not break for anyone not doing anything too strange, but still...
fafhrd91: i think breaking change is not that big, just add to entry to MIGRATION.md 
fafhrd91: @mockersf ping
mockersf: After testing, the "breaking" is even more limited, only the `'  '` character is changing because pretty much no one is respecting the RFC and the other characters are not automatically encoded by most http client
mockersf: That a choice of convenience vs rfc, but it's possible and easy to change.

I just tested two other http frameworks I use:
* Flask in Python is decoding spaces
* Play in Scala is not even compiling with a space in a route

So... that's your choice, I'm OK with both
fafhrd91: i'd split path into segments first, then url decode each of segment and then match resources
davechallis: I'd agree with @fafhrd91 , that sounds like the most correct (and unsurprising) way of handling it.

In terms of convenience vs. RFC, I'd side in favour of the RFC (again for least surprise when starting to use actix-web, or when frequently switching between languages). Though providing a convenience function or type could be an option, allowing users to switch between providing raw/encoded routes (but at least it would be explicit then).

Also, are spaces in routes common enough to warrant convenience over the RFC? I've always found them to be very rare in most sites/APIs I've come across (spaces seem to be much more common in dynamic path components, e.g. usernames, file uploads, document names, etc.).


DoumanAsh: I don't really feel strongly about as I do not use spaces in routes, so I'm fine with either way.
DoumanAsh: @mockersf Let's follow up with @fafhrd91 suggestion?
mockersf: This is pretty much what is happening right now except for two class of characters:
* Characters in the unreserved set from the RFC (`abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890-._~`). This lets actix match both `/a` and `/%61` to route `/a`, in the unlikely case where a client would encode every character in the url, which it is allowed to do
* Characters outside of ASCII are decoded before splitting into path segment. They are not mentioned in the RFC, I think it's better to be able to write them unencoded in routes, and Rust let you put `"😀"` which is more readable than `"%F0%9F%98%80"`
DoumanAsh: @mockersf Oh so we're ready with it?
mockersf: I updated the PR with the 0.7.14 release

yes, what this changes is:
* some characters that shouldn't have been decoded before matching routes are not anymore : `"!$'()*,"` and `" "`, space being more notable as it's the only one most http client will automatically encode
* every characters are now decoded when extracting path parameters by default, and this can be disabled. previously there was no mean to decode the remaining characters
DoumanAsh: Yeah, it looks good.

Just a clarification for @fafhrd91 query, what aoubt [Params](https://actix.rs/api/actix-web/stable/actix_web/dev/struct.Params.html) ?
mockersf: first decoding is done when creating the [`uri::Url`](https://github.com/actix/actix-web/blob/master/src/uri.rs#L45). Raw `http::Uri` is stored in `uri`, decoded `String` is stored in `path`
`Params` are built from the [`path`](https://github.com/actix/actix-web/blob/master/src/param.rs#L158), so they are have the decoded allowed characters
Then `Path` extractor takes those `Params` and decode the remaining characters
DoumanAsh: @mockersf Hmm What I think is that `Params` itself should be able to have method to decode self, in case user wouldn't use `Path` extractor
mockersf: @DoumanAsh I tried that today, but I quickly get stuck
`Params` is working on `&str` with no copy, but decoding the string implies making a copy of it, and I lose the lifetime and can't go back to a `&str` that lives long enough...
DoumanAsh: @mockersf Yes, since it is `&str` we need method like `fn decode(&self) -> String`, so that user would call it by himself, if needed. But otherwise we do not need to do it automatically
mockersf: I added a [`get_decoded`](https://github.com/actix/actix-web/pull/577/files#diff-e89b073ce3beca3ee7465aea55b9067eR107) method to `Params`

But I think `Params` are more often used with [`params.iter()`](https://actix.rs/api/actix-web/stable/actix_web/dev/struct.Params.html#method.iter), [`params[0]`](https://actix.rs/api/actix-web/stable/actix_web/dev/struct.Params.html#impl-Index%3Cusize%3E) or [`params["key"]`](https://actix.rs/api/actix-web/stable/actix_web/dev/struct.Params.html#impl-Index%3C%26%27a%20str%3E). I can add a `decoded_iter()` method that would give an iterator that would decode everything, but `Index` impls can't be overloaded to return either raw or decoded. So I think leaving those to raw path components is better
DoumanAsh: @mockersf Yeah I think it is good to be like that

@fafhrd91 I'd like your approve on review
mockersf: sure ! 

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 2e028d6d..902a84f6 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -8,6 +8,12 @@
 
 * `QueryConfig` and `PathConfig` are made public.
 
+### Added
+
+* By default, `Path` extractor now percent decode all characters. This behaviour can be disabled
+  with `PathConfig::default().disable_decoding()`
+
+
 ## [0.7.14] - 2018-11-14
 
 ### Added
@@ -16,6 +22,9 @@
 
 * Add method to configure `SameSite` option in `CookieIdentityPolicy`.
 
+* By default, `Path` extractor now percent decode all characters. This behaviour can be disabled
+  with `PathConfig::default().disable_decoding()`
+
 
 ### Fixed
 
diff --git a/MIGRATION.md b/MIGRATION.md
index 3c0bdd94..26a31424 100644
--- a/MIGRATION.md
+++ b/MIGRATION.md
@@ -1,3 +1,31 @@
+## 0.7.15
+
+* The `' '` character is not percent decoded anymore before matching routes. If you need to use it in
+  your routes, you should use `%20`.
+
+  instead of
+
+    ```rust
+    fn main() {
+         let app = App::new().resource("/my index", |r| {
+             r.method(http::Method::GET)
+                    .with(index);
+         });
+    }
+    ```
+
+  use
+
+    ```rust
+    fn main() {
+         let app = App::new().resource("/my%20index", |r| {
+             r.method(http::Method::GET)
+                    .with(index);
+         });
+    }
+    ```
+
+
 ## 0.7.4
 
 * `Route::with_config()`/`Route::with_async_config()` always passes configuration objects as tuple
diff --git a/src/de.rs b/src/de.rs
index 59ab79ba..05f8914f 100644
--- a/src/de.rs
+++ b/src/de.rs
@@ -1,7 +1,10 @@
+use std::rc::Rc;
+
 use serde::de::{self, Deserializer, Error as DeError, Visitor};
 
 use httprequest::HttpRequest;
 use param::ParamsIter;
+use uri::RESERVED_QUOTER;
 
 macro_rules! unsupported_type {
     ($trait_fn:ident, $name:expr) => {
@@ -13,6 +16,20 @@ macro_rules! unsupported_type {
     };
 }
 
+macro_rules! percent_decode_if_needed {
+    ($value:expr, $decode:expr) => {
+        if $decode {
+            if let Some(ref mut value) = RESERVED_QUOTER.requote($value.as_bytes()) {
+                Rc::make_mut(value).parse()
+            } else {
+                $value.parse()
+            }
+        } else {
+            $value.parse()
+        }
+    }
+}
+
 macro_rules! parse_single_value {
     ($trait_fn:ident, $visit_fn:ident, $tp:tt) => {
         fn $trait_fn<V>(self, visitor: V) -> Result<V::Value, Self::Error>
@@ -23,11 +40,11 @@ macro_rules! parse_single_value {
                     format!("wrong number of parameters: {} expected 1",
                             self.req.match_info().len()).as_str()))
             } else {
-                let v = self.req.match_info()[0].parse().map_err(
-                    |_| de::value::Error::custom(
-                        format!("can not parse {:?} to a {}",
-                                &self.req.match_info()[0], $tp)))?;
-                visitor.$visit_fn(v)
+                let v_parsed = percent_decode_if_needed!(&self.req.match_info()[0], self.decode)
+                    .map_err(|_| de::value::Error::custom(
+                        format!("can not parse {:?} to a {}", &self.req.match_info()[0], $tp)
+                    ))?;
+                visitor.$visit_fn(v_parsed)
             }
         }
     }
@@ -35,11 +52,12 @@ macro_rules! parse_single_value {
 
 pub struct PathDeserializer<'de, S: 'de> {
     req: &'de HttpRequest<S>,
+    decode: bool,
 }
 
 impl<'de, S: 'de> PathDeserializer<'de, S> {
-    pub fn new(req: &'de HttpRequest<S>) -> Self {
-        PathDeserializer { req }
+    pub fn new(req: &'de HttpRequest<S>, decode: bool) -> Self {
+        PathDeserializer { req, decode }
     }
 }
 
@@ -53,6 +71,7 @@ impl<'de, S: 'de> Deserializer<'de> for PathDeserializer<'de, S> {
         visitor.visit_map(ParamsDeserializer {
             params: self.req.match_info().iter(),
             current: None,
+            decode: self.decode,
         })
     }
 
@@ -107,6 +126,7 @@ impl<'de, S: 'de> Deserializer<'de> for PathDeserializer<'de, S> {
         } else {
             visitor.visit_seq(ParamsSeq {
                 params: self.req.match_info().iter(),
+                decode: self.decode,
             })
         }
     }
@@ -128,6 +148,7 @@ impl<'de, S: 'de> Deserializer<'de> for PathDeserializer<'de, S> {
         } else {
             visitor.visit_seq(ParamsSeq {
                 params: self.req.match_info().iter(),
+                decode: self.decode,
             })
         }
     }
@@ -141,28 +162,13 @@ impl<'de, S: 'de> Deserializer<'de> for PathDeserializer<'de, S> {
         Err(de::value::Error::custom("unsupported type: enum"))
     }
 
-    fn deserialize_str<V>(self, visitor: V) -> Result<V::Value, Self::Error>
-    where
-        V: Visitor<'de>,
-    {
-        if self.req.match_info().len() != 1 {
-            Err(de::value::Error::custom(
-                format!(
-                    "wrong number of parameters: {} expected 1",
-                    self.req.match_info().len()
-                ).as_str(),
-            ))
-        } else {
-            visitor.visit_str(&self.req.match_info()[0])
-        }
-    }
-
     fn deserialize_seq<V>(self, visitor: V) -> Result<V::Value, Self::Error>
     where
         V: Visitor<'de>,
     {
         visitor.visit_seq(ParamsSeq {
             params: self.req.match_info().iter(),
+            decode: self.decode,
         })
     }
 
@@ -184,13 +190,16 @@ impl<'de, S: 'de> Deserializer<'de> for PathDeserializer<'de, S> {
     parse_single_value!(deserialize_f32, visit_f32, "f32");
     parse_single_value!(deserialize_f64, visit_f64, "f64");
     parse_single_value!(deserialize_string, visit_string, "String");
+    parse_single_value!(deserialize_str, visit_string, "String");
     parse_single_value!(deserialize_byte_buf, visit_string, "String");
     parse_single_value!(deserialize_char, visit_char, "char");
+
 }
 
 struct ParamsDeserializer<'de> {
     params: ParamsIter<'de>,
     current: Option<(&'de str, &'de str)>,
+    decode: bool,
 }
 
 impl<'de> de::MapAccess<'de> for ParamsDeserializer<'de> {
@@ -212,7 +221,7 @@ impl<'de> de::MapAccess<'de> for ParamsDeserializer<'de> {
         V: de::DeserializeSeed<'de>,
     {
         if let Some((_, value)) = self.current.take() {
-            seed.deserialize(Value { value })
+            seed.deserialize(Value { value, decode: self.decode })
         } else {
             Err(de::value::Error::custom("unexpected item"))
         }
@@ -252,16 +261,18 @@ macro_rules! parse_value {
         fn $trait_fn<V>(self, visitor: V) -> Result<V::Value, Self::Error>
             where V: Visitor<'de>
         {
-            let v = self.value.parse().map_err(
-                |_| de::value::Error::custom(
-                    format!("can not parse {:?} to a {}", self.value, $tp)))?;
-            visitor.$visit_fn(v)
+            let v_parsed = percent_decode_if_needed!(&self.value, self.decode)
+                .map_err(|_| de::value::Error::custom(
+                    format!("can not parse {:?} to a {}", &self.value, $tp)
+                ))?;
+            visitor.$visit_fn(v_parsed)
         }
     }
 }
 
 struct Value<'de> {
     value: &'de str,
+    decode: bool,
 }
 
 impl<'de> Deserializer<'de> for Value<'de> {
@@ -377,6 +388,7 @@ impl<'de> Deserializer<'de> for Value<'de> {
 
 struct ParamsSeq<'de> {
     params: ParamsIter<'de>,
+    decode: bool,
 }
 
 impl<'de> de::SeqAccess<'de> for ParamsSeq<'de> {
@@ -387,7 +399,7 @@ impl<'de> de::SeqAccess<'de> for ParamsSeq<'de> {
         T: de::DeserializeSeed<'de>,
     {
         match self.params.next() {
-            Some(item) => Ok(Some(seed.deserialize(Value { value: item.1 })?)),
+            Some(item) => Ok(Some(seed.deserialize(Value { value: item.1, decode: self.decode })?)),
             None => Ok(None),
         }
     }
diff --git a/src/extractor.rs b/src/extractor.rs
index 45e29ace..717e0f6c 100644
--- a/src/extractor.rs
+++ b/src/extractor.rs
@@ -18,7 +18,8 @@ use httpmessage::{HttpMessage, MessageBody, UrlEncoded};
 use httprequest::HttpRequest;
 
 #[derive(PartialEq, Eq, PartialOrd, Ord)]
-/// Extract typed information from the request's path.
+/// Extract typed information from the request's path. Information from the path is
+/// URL decoded. Decoding of special characters can be disabled through `PathConfig`.
 ///
 /// ## Example
 ///
@@ -119,7 +120,7 @@ where
         let req = req.clone();
         let req2 = req.clone();
         let err = Rc::clone(&cfg.ehandler);
-        de::Deserialize::deserialize(PathDeserializer::new(&req))
+        de::Deserialize::deserialize(PathDeserializer::new(&req, cfg.decode))
             .map_err(move |e| (*err)(e, &req2))
             .map(|inner| Path { inner })
     }
@@ -149,6 +150,7 @@ where
 /// ```
 pub struct PathConfig<S> {
     ehandler: Rc<Fn(serde_urlencoded::de::Error, &HttpRequest<S>) -> Error>,
+    decode: bool,
 }
 impl<S> PathConfig<S> {
     /// Set custom error handler
@@ -159,12 +161,20 @@ impl<S> PathConfig<S> {
         self.ehandler = Rc::new(f);
         self
     }
+
+    /// Disable decoding of URL encoded special charaters from the path
+    pub fn disable_decoding(&mut self) -> &mut Self
+    {
+        self.decode = false;
+        self
+    }
 }
 
 impl<S> Default for PathConfig<S> {
     fn default() -> Self {
         PathConfig {
             ehandler: Rc::new(|e, _| ErrorNotFound(e)),
+            decode: true,
         }
     }
 }
@@ -1090,6 +1100,68 @@ mod tests {
         assert_eq!(*Path::<i8>::from_request(&req, &&PathConfig::default()).unwrap(), 32);
     }
 
+    #[test]
+    fn test_extract_path_decode() {
+        let mut router = Router::<()>::default();
+        router.register_resource(Resource::new(ResourceDef::new("/{value}/")));
+
+        macro_rules! test_single_value {
+            ($value:expr, $expected:expr) => {
+                {
+                    let req = TestRequest::with_uri($value).finish();
+                    let info = router.recognize(&req, &(), 0);
+                    let req = req.with_route_info(info);
+                    assert_eq!(*Path::<String>::from_request(&req, &PathConfig::default()).unwrap(), $expected);
+                }
+            }
+        }
+
+        test_single_value!("/%25/", "%");
+        test_single_value!("/%40%C2%A3%24%25%5E%26%2B%3D/", "@£$%^&+=");
+        test_single_value!("/%2B/", "+");
+        test_single_value!("/%252B/", "%2B");
+        test_single_value!("/%2F/", "/");
+        test_single_value!("/%252F/", "%2F");
+        test_single_value!("/http%3A%2F%2Flocalhost%3A80%2Ffoo/", "http://localhost:80/foo");
+        test_single_value!("/%2Fvar%2Flog%2Fsyslog/", "/var/log/syslog");
+        test_single_value!(
+            "/http%3A%2F%2Flocalhost%3A80%2Ffile%2F%252Fvar%252Flog%252Fsyslog/",
+            "http://localhost:80/file/%2Fvar%2Flog%2Fsyslog"
+        );
+
+        let req = TestRequest::with_uri("/%25/7/?id=test").finish();
+
+        let mut router = Router::<()>::default();
+        router.register_resource(Resource::new(ResourceDef::new("/{key}/{value}/")));
+        let info = router.recognize(&req, &(), 0);
+        let req = req.with_route_info(info);
+
+        let s = Path::<Test2>::from_request(&req, &PathConfig::default()).unwrap();
+        assert_eq!(s.key, "%");
+        assert_eq!(s.value, 7);
+
+        let s = Path::<(String, String)>::from_request(&req, &PathConfig::default()).unwrap();
+        assert_eq!(s.0, "%");
+        assert_eq!(s.1, "7");
+    }
+
+    #[test]
+    fn test_extract_path_no_decode() {
+        let mut router = Router::<()>::default();
+        router.register_resource(Resource::new(ResourceDef::new("/{value}/")));
+
+        let req = TestRequest::with_uri("/%25/").finish();
+        let info = router.recognize(&req, &(), 0);
+        let req = req.with_route_info(info);
+        assert_eq!(
+            *Path::<String>::from_request(
+                &req,
+                &&PathConfig::default().disable_decoding()
+            ).unwrap(),
+            "%25"
+        );
+    }
+
     #[test]
     fn test_tuple_extract() {
         let mut router = Router::<()>::default();
diff --git a/src/param.rs b/src/param.rs
index d0664df9..a3f60259 100644
--- a/src/param.rs
+++ b/src/param.rs
@@ -8,7 +8,7 @@ use http::StatusCode;
 use smallvec::SmallVec;
 
 use error::{InternalError, ResponseError, UriSegmentError};
-use uri::Url;
+use uri::{Url, RESERVED_QUOTER};
 
 /// A trait to abstract the idea of creating a new instance of a type from a
 /// path parameter.
@@ -103,6 +103,17 @@ impl Params {
         }
     }
 
+    /// Get URL-decoded matched parameter by name without type conversion
+    pub fn get_decoded(&self, key: &str) -> Option<String> {
+        self.get(key).map(|value| {
+            if let Some(ref mut value) = RESERVED_QUOTER.requote(value.as_bytes()) {
+                Rc::make_mut(value).to_string()
+            } else {
+                value.to_string()
+            }
+        })
+    }
+
     /// Get unprocessed part of path
     pub fn unprocessed(&self) -> &str {
         &self.url.path()[(self.tail as usize)..]
@@ -300,4 +311,24 @@ mod tests {
             Ok(PathBuf::from_iter(vec!["seg2"]))
         );
     }
+
+    #[test]
+    fn test_get_param_by_name() {
+        let mut params = Params::new();
+        params.add_static("item1", "path");
+        params.add_static("item2", "http%3A%2F%2Flocalhost%3A80%2Ffoo");
+
+        assert_eq!(params.get("item0"), None);
+        assert_eq!(params.get_decoded("item0"), None);
+        assert_eq!(params.get("item1"), Some("path"));
+        assert_eq!(params.get_decoded("item1"), Some("path".to_string()));
+        assert_eq!(
+            params.get("item2"),
+            Some("http%3A%2F%2Flocalhost%3A80%2Ffoo")
+        );
+        assert_eq!(
+            params.get_decoded("item2"),
+            Some("http://localhost:80/foo".to_string())
+        );
+    }
 }
diff --git a/src/uri.rs b/src/uri.rs
index 881cf20a..c87cb3d5 100644
--- a/src/uri.rs
+++ b/src/uri.rs
@@ -1,25 +1,12 @@
 use http::Uri;
 use std::rc::Rc;
 
-#[allow(dead_code)]
-const GEN_DELIMS: &[u8] = b":/?#[]@";
-#[allow(dead_code)]
-const SUB_DELIMS_WITHOUT_QS: &[u8] = b"!$'()*,";
-#[allow(dead_code)]
-const SUB_DELIMS: &[u8] = b"!$'()*,+?=;";
-#[allow(dead_code)]
-const RESERVED: &[u8] = b":/?#[]@!$'()*,+?=;";
-#[allow(dead_code)]
-const UNRESERVED: &[u8] = b"abcdefghijklmnopqrstuvwxyz
-    ABCDEFGHIJKLMNOPQRSTUVWXYZ
-    1234567890
-    -._~";
-const ALLOWED: &[u8] = b"abcdefghijklmnopqrstuvwxyz
-    ABCDEFGHIJKLMNOPQRSTUVWXYZ
-    1234567890
-    -._~
-    !$'()*,";
-const QS: &[u8] = b"+&=;b";
+// https://tools.ietf.org/html/rfc3986#section-2.2
+const RESERVED_PLUS_EXTRA: &[u8] = b":/?#[]@!$&'()*,+?;=%^ <>\"\\`{}|";
+
+// https://tools.ietf.org/html/rfc3986#section-2.3
+const UNRESERVED: &[u8] =
+    b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890-._~";
 
 #[inline]
 fn bit_at(array: &[u8], ch: u8) -> bool {
@@ -32,7 +19,8 @@ fn set_bit(array: &mut [u8], ch: u8) {
 }
 
 lazy_static! {
-    static ref DEFAULT_QUOTER: Quoter = { Quoter::new(b"@:", b"/+") };
+    static ref UNRESERVED_QUOTER: Quoter = { Quoter::new(UNRESERVED) };
+    pub(crate) static ref RESERVED_QUOTER: Quoter = { Quoter::new(RESERVED_PLUS_EXTRA) };
 }
 
 #[derive(Default, Clone, Debug)]
@@ -43,7 +31,7 @@ pub(crate) struct Url {
 
 impl Url {
     pub fn new(uri: Uri) -> Url {
-        let path = DEFAULT_QUOTER.requote(uri.path().as_bytes());
+        let path = UNRESERVED_QUOTER.requote(uri.path().as_bytes());
 
         Url { uri, path }
     }
@@ -63,36 +51,19 @@ impl Url {
 
 pub(crate) struct Quoter {
     safe_table: [u8; 16],
-    protected_table: [u8; 16],
 }
 
 impl Quoter {
-    pub fn new(safe: &[u8], protected: &[u8]) -> Quoter {
+    pub fn new(safe: &[u8]) -> Quoter {
         let mut q = Quoter {
             safe_table: [0; 16],
-            protected_table: [0; 16],
         };
 
         // prepare safe table
-        for i in 0..128 {
-            if ALLOWED.contains(&i) {
-                set_bit(&mut q.safe_table, i);
-            }
-            if QS.contains(&i) {
-                set_bit(&mut q.safe_table, i);
-            }
-        }
-
         for ch in safe {
             set_bit(&mut q.safe_table, *ch)
         }
 
-        // prepare protected table
-        for ch in protected {
-            set_bit(&mut q.safe_table, *ch);
-            set_bit(&mut q.protected_table, *ch);
-        }
-
         q
     }
 
@@ -115,19 +86,17 @@ impl Quoter {
 
                     if let Some(ch) = restore_ch(pct[1], pct[2]) {
                         if ch < 128 {
-                            if bit_at(&self.protected_table, ch) {
-                                buf.extend_from_slice(&pct);
-                                idx += 1;
-                                continue;
-                            }
-
                             if bit_at(&self.safe_table, ch) {
                                 buf.push(ch);
                                 idx += 1;
                                 continue;
                             }
+
+                            buf.extend_from_slice(&pct);
+                        } else {
+                            // Not ASCII, decode it
+                            buf.push(ch);
                         }
-                        buf.push(ch);
                     } else {
                         buf.extend_from_slice(&pct[..]);
                     }
@@ -172,3 +141,37 @@ fn from_hex(v: u8) -> Option<u8> {
 fn restore_ch(d1: u8, d2: u8) -> Option<u8> {
     from_hex(d1).and_then(|d1| from_hex(d2).and_then(move |d2| Some(d1 << 4 | d2)))
 }
+
+
+#[cfg(test)]
+mod tests {
+    use std::rc::Rc;
+
+    use super::*;
+
+    #[test]
+    fn decode_path() {
+        assert_eq!(UNRESERVED_QUOTER.requote(b"https://localhost:80/foo"), None);
+
+        assert_eq!(
+            Rc::try_unwrap(UNRESERVED_QUOTER.requote(
+                b"https://localhost:80/foo%25"
+            ).unwrap()).unwrap(),
+            "https://localhost:80/foo%25".to_string()
+        );
+
+        assert_eq!(
+            Rc::try_unwrap(UNRESERVED_QUOTER.requote(
+                b"http://cache-service/http%3A%2F%2Flocalhost%3A80%2Ffoo"
+            ).unwrap()).unwrap(),
+            "http://cache-service/http%3A%2F%2Flocalhost%3A80%2Ffoo".to_string()
+        );
+
+        assert_eq!(
+            Rc::try_unwrap(UNRESERVED_QUOTER.requote(
+                b"http://cache/http%3A%2F%2Flocal%3A80%2Ffile%2F%252Fvar%252Flog%0A"
+            ).unwrap()).unwrap(),
+            "http://cache/http%3A%2F%2Flocal%3A80%2Ffile%2F%252Fvar%252Flog%0A".to_string()
+        );
+    }
+}
\ No newline at end of file


Test patch:
diff --git a/tests/test_handlers.rs b/tests/test_handlers.rs
index 3ea709c9..debc1626 100644
--- a/tests/test_handlers.rs
+++ b/tests/test_handlers.rs
@@ -672,6 +672,6 @@ fn test_unsafe_path_route() {
     let bytes = srv.execute(response.body()).unwrap();
     assert_eq!(
         bytes,
-        Bytes::from_static(b"success: http:%2F%2Fexample.com")
+        Bytes::from_static(b"success: http%3A%2F%2Fexample.com")
     );
 }