diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md index 501181d92..93a122941 100644 --- a/actix-files/CHANGES.md +++ b/actix-files/CHANGES.md @@ -1,8 +1,8 @@ # 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] +- The `Files` service now rejects requests with URL paths that include `%2F` (decoded: `/`). [#2398] +- The `Files` service now correctly decodes `%25` in the URL path to `%` for the file path. [#2398] - Minimum supported Rust version (MSRV) is now 1.54. [#2398]: https://github.com/actix/actix-web/pull/2398 diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs index d28889e73..6682529f8 100644 --- a/actix-files/src/error.rs +++ b/actix-files/src/error.rs @@ -38,7 +38,7 @@ pub enum UriSegmentError { 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")] + #[display(fmt = "The path is not a valid UTF-8 string after percent-decoding")] NotValidUtf8, } diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 03f464626..22f294b9d 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -8,6 +8,7 @@ mod de; mod path; mod pattern; +mod quoter; mod resource; mod resource_path; mod router; @@ -18,9 +19,10 @@ mod url; pub use self::de::PathDeserializer; pub use self::path::Path; pub use self::pattern::{IntoPatterns, Patterns}; +pub use self::quoter::Quoter; pub use self::resource::ResourceDef; pub use self::resource_path::{Resource, ResourcePath}; pub use self::router::{ResourceInfo, Router, RouterBuilder}; #[cfg(feature = "http")] -pub use self::url::{Quoter, Url}; +pub use self::url::Url; diff --git a/actix-router/src/quoter.rs b/actix-router/src/quoter.rs new file mode 100644 index 000000000..26ecc92cd --- /dev/null +++ b/actix-router/src/quoter.rs @@ -0,0 +1,219 @@ +#[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"; + +/// A quoter +pub struct Quoter { + /// Simple bit-map of safe values in the 0-127 ASCII range. + safe_table: [u8; 16], + + /// Simple bit-map of protected values in the 0-127 ASCII range. + protected_table: [u8; 16], +} + +impl Quoter { + pub fn new(safe: &[u8], protected: &[u8]) -> Quoter { + let mut quoter = Quoter { + safe_table: [0; 16], + protected_table: [0; 16], + }; + + // prepare safe table + for ch in 0..128 { + if ALLOWED.contains(&ch) { + set_bit(&mut quoter.safe_table, ch); + } + + if QS.contains(&ch) { + set_bit(&mut quoter.safe_table, ch); + } + } + + for &ch in safe { + set_bit(&mut quoter.safe_table, ch) + } + + // prepare protected table + for &ch in protected { + set_bit(&mut quoter.safe_table, ch); + set_bit(&mut quoter.protected_table, ch); + } + + quoter + } + + /// Re-quotes... ? + /// + /// Returns `None` when no modification to the original string was required. + pub fn requote(&self, val: &[u8]) -> Option { + let mut has_pct = 0; + let mut pct = [b'%', 0, 0]; + let mut idx = 0; + let mut cloned: Option> = None; + + let len = val.len(); + + while idx < len { + let ch = val[idx]; + + if has_pct != 0 { + pct[has_pct] = val[idx]; + has_pct += 1; + + if has_pct == 3 { + has_pct = 0; + let buf = cloned.as_mut().unwrap(); + + if let Some(ch) = hex_pair_to_char(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.push(ch); + } else { + buf.extend_from_slice(&pct[..]); + } + } + } else if ch == b'%' { + has_pct = 1; + + if cloned.is_none() { + let mut c = Vec::with_capacity(len); + c.extend_from_slice(&val[..idx]); + cloned = Some(c); + } + } else if let Some(ref mut cloned) = cloned { + cloned.push(ch) + } + + idx += 1; + } + + cloned.map(|data| String::from_utf8_lossy(&data).into_owned()) + } +} + +/// Converts an ASCII character in the hex-encoded set (`0-9`, `A-F`, `a-f`) to its integer +/// representation from `0x0`–`0xF`. +/// +/// - `0x30 ('0') => 0x0` +/// - `0x39 ('9') => 0x9` +/// - `0x41 ('a') => 0xA` +/// - `0x61 ('A') => 0xA` +/// - `0x46 ('f') => 0xF` +/// - `0x66 ('F') => 0xF` +fn from_ascii_hex(v: u8) -> Option { + match v { + b'0'..=b'9' => Some(v - 0x30), // ord('0') == 0x30 + b'A'..=b'F' => Some(v - 0x41 + 10), // ord('A') == 0x41 + b'a'..=b'f' => Some(v - 0x61 + 10), // ord('a') == 0x61 + _ => None, + } +} + +/// Decode a ASCII hex-encoded pair to an integer. +/// +/// Returns `None` if either portion of the decoded pair does not evaluate to a valid hex value. +/// +/// - `0x33 ('3'), 0x30 ('0') => 0x30 ('0')` +/// - `0x34 ('4'), 0x31 ('1') => 0x41 ('A')` +/// - `0x36 ('6'), 0x31 ('1') => 0x61 ('a')` +fn hex_pair_to_char(d1: u8, d2: u8) -> Option { + let (d_high, d_low) = (from_ascii_hex(d1)?, from_ascii_hex(d2)?); + + // left shift high nibble by 4 bits + Some(d_high << 4 | d_low) +} + +/// Sets bit in given bit-map to 1=true. +/// +/// # Panics +/// Panics if `ch` index is out of bounds. +fn set_bit(array: &mut [u8], ch: u8) { + array[(ch >> 3) as usize] |= 0b1 << (ch & 0b111) +} + +/// Returns true if bit to true in given bit-map. +/// +/// # Panics +/// Panics if `ch` index is out of bounds. +fn bit_at(array: &[u8], ch: u8) -> bool { + array[(ch >> 3) as usize] & (0b1 << (ch & 0b111)) != 0 +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hex_encoding() { + let hex = b"0123456789abcdefABCDEF"; + + for i in 0..256 { + let c = i as u8; + if hex.contains(&c) { + assert!(from_ascii_hex(c).is_some()) + } else { + assert!(from_ascii_hex(c).is_none()) + } + } + + let expected = [ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 10, 11, 12, 13, 14, 15, + ]; + for i in 0..hex.len() { + assert_eq!(from_ascii_hex(hex[i]).unwrap(), expected[i]); + } + } + + #[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"); + + 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"); + } + + #[test] + fn quoter_no_modification() { + let q = Quoter::new(b"", b""); + assert_eq!(q.requote(b"/abc/../efg"), None); + } +} diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index 156c1e1c6..c5a3508aa 100644 --- a/actix-router/src/url.rs +++ b/actix-router/src/url.rs @@ -1,30 +1,6 @@ use crate::ResourcePath; -#[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"; +use crate::Quoter; thread_local! { static DEFAULT_QUOTER: Quoter = Quoter::new(b"@:", b"%/+"); @@ -44,18 +20,20 @@ impl Url { } #[inline] - pub fn with_quoter(uri: http::Uri, quoter: &Quoter) -> Url { + pub fn new_with_quoter(uri: http::Uri, quoter: &Quoter) -> Url { Url { path: quoter.requote(uri.path().as_bytes()), uri, } } + /// Returns URI. #[inline] pub fn uri(&self) -> &http::Uri { &self.uri } + /// Returns path. #[inline] pub fn path(&self) -> &str { match self.path { @@ -84,155 +62,6 @@ impl ResourcePath for Url { } } -/// A quoter -pub struct Quoter { - /// Simple bit-map of safe values in the 0-127 ASCII range. - safe_table: [u8; 16], - - /// Simple bit-map of protected values in the 0-127 ASCII range. - protected_table: [u8; 16], -} - -impl Quoter { - pub fn new(safe: &[u8], protected: &[u8]) -> Quoter { - let mut quoter = Quoter { - safe_table: [0; 16], - protected_table: [0; 16], - }; - - // prepare safe table - for ch in 0..128 { - if ALLOWED.contains(&ch) { - set_bit(&mut quoter.safe_table, ch); - } - - if QS.contains(&ch) { - set_bit(&mut quoter.safe_table, ch); - } - } - - for &ch in safe { - set_bit(&mut quoter.safe_table, ch) - } - - // prepare protected table - for &ch in protected { - set_bit(&mut quoter.safe_table, ch); - set_bit(&mut quoter.protected_table, ch); - } - - quoter - } - - /// Re-quotes... ? - /// - /// Returns `None` when no modification to the original string was required. - pub fn requote(&self, val: &[u8]) -> Option { - let mut has_pct = 0; - let mut pct = [b'%', 0, 0]; - let mut idx = 0; - let mut cloned: Option> = None; - - let len = val.len(); - - while idx < len { - let ch = val[idx]; - - if has_pct != 0 { - pct[has_pct] = val[idx]; - has_pct += 1; - - if has_pct == 3 { - has_pct = 0; - let buf = cloned.as_mut().unwrap(); - - if let Some(ch) = hex_pair_to_char(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.push(ch); - } else { - buf.extend_from_slice(&pct[..]); - } - } - } else if ch == b'%' { - has_pct = 1; - - if cloned.is_none() { - let mut c = Vec::with_capacity(len); - c.extend_from_slice(&val[..idx]); - cloned = Some(c); - } - } else if let Some(ref mut cloned) = cloned { - cloned.push(ch) - } - - idx += 1; - } - - cloned.map(|data| String::from_utf8_lossy(&data).into_owned()) - } -} - -/// Converts an ASCII character in the hex-encoded set (`0-9`, `A-F`, `a-f`) to its integer -/// representation from `0x0`–`0xF`. -/// -/// - `0x30 ('0') => 0x0` -/// - `0x39 ('9') => 0x9` -/// - `0x41 ('a') => 0xA` -/// - `0x61 ('A') => 0xA` -/// - `0x46 ('f') => 0xF` -/// - `0x66 ('F') => 0xF` -fn from_ascii_hex(v: u8) -> Option { - match v { - b'0'..=b'9' => Some(v - 0x30), // ord('0') == 0x30 - b'A'..=b'F' => Some(v - 0x41 + 10), // ord('A') == 0x41 - b'a'..=b'f' => Some(v - 0x61 + 10), // ord('a') == 0x61 - _ => None, - } -} - -/// Decode a ASCII hex-encoded pair to an integer. -/// -/// Returns `None` if either portion of the decoded pair does not evaluate to a valid hex value. -/// -/// - `0x33 ('3'), 0x30 ('0') => 0x30 ('0')` -/// - `0x34 ('4'), 0x31 ('1') => 0x41 ('A')` -/// - `0x36 ('6'), 0x31 ('1') => 0x61 ('a')` -fn hex_pair_to_char(d1: u8, d2: u8) -> Option { - let (d_high, d_low) = (from_ascii_hex(d1)?, from_ascii_hex(d2)?); - - // left shift high nibble by 4 bits - Some(d_high << 4 | d_low) -} - -/// Sets bit in given bit-map to 1=true. -/// -/// # Panics -/// Panics if `ch` index is out of bounds. -fn set_bit(array: &mut [u8], ch: u8) { - array[(ch >> 3) as usize] |= 0b1 << (ch & 0b111) -} - -/// Returns true if bit to true in given bit-map. -/// -/// # Panics -/// Panics if `ch` index is out of bounds. -fn bit_at(array: &[u8], ch: u8) -> bool { - array[(ch >> 3) as usize] & (0b1 << (ch & 0b111)) != 0 -} - #[cfg(test)] mod tests { use http::Uri; @@ -308,44 +137,4 @@ mod tests { // We should always get a valid utf8 string assert!(String::from_utf8(path.path().as_bytes().to_owned()).is_ok()); } - - #[test] - fn hex_encoding() { - let hex = b"0123456789abcdefABCDEF"; - - for i in 0..256 { - let c = i as u8; - if hex.contains(&c) { - assert!(from_ascii_hex(c).is_some()) - } else { - assert!(from_ascii_hex(c).is_none()) - } - } - - let expected = [ - 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 10, 11, 12, 13, 14, 15, - ]; - for i in 0..hex.len() { - assert_eq!(from_ascii_hex(hex[i]).unwrap(), expected[i]); - } - } - - #[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"); - - 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"); - } - - #[test] - fn quoter_no_modification() { - let q = Quoter::new(b"", b""); - assert_eq!(q.requote(b"/abc/../efg"), None); - } } diff --git a/src/request.rs b/src/request.rs index cbec70a29..e876c3b4d 100644 --- a/src/request.rs +++ b/src/request.rs @@ -122,7 +122,7 @@ impl HttpRequest { /// Returns a reference to the URL parameters container. /// - /// A url parameter is specified in the form `{identifier}`, where the identifier can be used + /// A URL parameter is specified in the form `{identifier}`, where the identifier can be used /// later in a request handler to access the matched value for that parameter. /// /// # Percent Encoding and URL Parameters diff --git a/src/types/path.rs b/src/types/path.rs index 4a694b763..5d52e0e1e 100644 --- a/src/types/path.rs +++ b/src/types/path.rs @@ -138,6 +138,7 @@ where /// enum Folder { /// #[serde(rename = "inbox")] /// Inbox, +/// /// #[serde(rename = "outbox")] /// Outbox, /// } @@ -147,19 +148,17 @@ where /// format!("Selected folder: {:?}!", folder) /// } /// -/// fn main() { -/// let app = App::new().service( -/// web::resource("/messages/{folder}") -/// .app_data(PathConfig::default().error_handler(|err, req| { -/// error::InternalError::from_response( -/// err, -/// HttpResponse::Conflict().into(), -/// ) -/// .into() -/// })) -/// .route(web::post().to(index)), -/// ); -/// } +/// let app = App::new().service( +/// web::resource("/messages/{folder}") +/// .app_data(PathConfig::default().error_handler(|err, req| { +/// error::InternalError::from_response( +/// err, +/// HttpResponse::Conflict().into(), +/// ) +/// .into() +/// })) +/// .route(web::post().to(index)), +/// ); /// ``` #[derive(Clone, Default)] pub struct PathConfig { @@ -167,7 +166,7 @@ pub struct PathConfig { } impl PathConfig { - /// Set custom error handler + /// Set custom error handler. pub fn error_handler(mut self, f: F) -> Self where F: Fn(PathError, &HttpRequest) -> Error + Send + Sync + 'static, diff --git a/tests/weird_poll.rs b/tests/weird_poll.rs new file mode 100644 index 000000000..5844ea2c2 --- /dev/null +++ b/tests/weird_poll.rs @@ -0,0 +1,30 @@ +//! Regression test for https://github.com/actix/actix-web/issues/1321 + +// use actix_http::body::{BodyStream, MessageBody}; +// use bytes::Bytes; +// use futures_channel::oneshot; +// use futures_util::{ +// stream::once, +// task::{noop_waker, Context}, +// }; + +// #[test] +// fn weird_poll() { +// let (sender, receiver) = oneshot::channel(); +// let mut body_stream = Ok(BodyStream::new(once(async { +// let x = Box::new(0); +// let y = &x; +// receiver.await.unwrap(); +// let _z = **y; +// Ok::<_, ()>(Bytes::new()) +// }))); + +// let waker = noop_waker(); +// let mut cx = Context::from_waker(&waker); + +// let _ = body_stream.as_mut().unwrap().poll_next(&mut cx); +// sender.send(()).unwrap(); +// let _ = std::mem::replace(&mut body_stream, Err([0; 32])) +// .unwrap() +// .poll_next(&mut cx); +// }