From d93244aa4f0b7e55c065990102e7a87da22e3c54 Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Fri, 27 Oct 2017 22:19:00 -0700 Subject: [PATCH] Do not use as it can not parse some valid paths --- CHANGES.md | 2 ++ Cargo.toml | 1 + src/httprequest.rs | 39 +++++++++++++++++++------------------- src/lib.rs | 1 + src/logger.rs | 8 +++++++- src/reader.rs | 32 +++++++++++++++++++++++++++---- src/ws.rs | 34 ++++++++++++++++----------------- tests/test_httprequest.rs | 28 +++++++++++++-------------- tests/test_httpresponse.rs | 4 ++-- 9 files changed, 91 insertions(+), 58 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b20895abc..88f4ffc25 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,8 @@ ## 0.2.0 (2017-10-xx) +* Do not use `http::Uri` as it can not parse some valid paths + * Refactor response `Body` * Refactor `RouteRecognizer` usability diff --git a/Cargo.toml b/Cargo.toml index f1f832877..18c62c0ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ regex = "0.2" slab = "0.4" sha1 = "0.2" url = "1.5" +percent-encoding = "1.0" # tokio bytes = "0.4" diff --git a/src/httprequest.rs b/src/httprequest.rs index cb5bbe764..7287e4faf 100644 --- a/src/httprequest.rs +++ b/src/httprequest.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use bytes::BytesMut; use futures::{Async, Future, Stream, Poll}; use url::form_urlencoded; -use http::{header, Method, Version, Uri, HeaderMap, Extensions}; +use http::{header, Method, Version, HeaderMap, Extensions}; use {Cookie, CookieParseError}; use {HttpRange, HttpRangeParseError}; @@ -18,7 +18,8 @@ use multipart::{Multipart, MultipartError}; pub struct HttpRequest { version: Version, method: Method, - uri: Uri, + path: String, + query: String, headers: HeaderMap, params: Params, cookies: Vec>, @@ -28,10 +29,13 @@ pub struct HttpRequest { impl HttpRequest { /// Construct a new Request. #[inline] - pub fn new(method: Method, uri: Uri, version: Version, headers: HeaderMap) -> Self { + pub fn new(method: Method, path: String, + version: Version, headers: HeaderMap, query: String) -> Self + { HttpRequest { method: method, - uri: uri, + path: path, + query: query, version: version, headers: headers, params: Params::empty(), @@ -43,7 +47,8 @@ impl HttpRequest { pub(crate) fn for_error() -> HttpRequest { HttpRequest { method: Method::GET, - uri: Uri::default(), + path: String::new(), + query: String::new(), version: Version::HTTP_11, headers: HeaderMap::new(), params: Params::empty(), @@ -58,10 +63,6 @@ impl HttpRequest { &mut self.extensions } - /// Read the Request Uri. - #[inline] - pub fn uri(&self) -> &Uri { &self.uri } - /// Read the Request method. #[inline] pub fn method(&self) -> &Method { &self.method } @@ -79,13 +80,13 @@ impl HttpRequest { /// The target path of this Request. #[inline] pub fn path(&self) -> &str { - self.uri.path() + &self.path } /// Return a new iterator that yields pairs of `Cow` for query parameters #[inline] pub fn query(&self) -> form_urlencoded::Parse { - form_urlencoded::parse(self.query_string().as_ref()) + form_urlencoded::parse(self.query.as_ref()) } /// The query string in the URL. @@ -93,7 +94,7 @@ impl HttpRequest { /// E.g., id=10 #[inline] pub fn query_string(&self) -> &str { - self.uri.query().unwrap_or("") + &self.query } /// Return request cookies. @@ -238,7 +239,8 @@ impl HttpRequest { impl fmt::Debug for HttpRequest { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let res = write!(f, "\nHttpRequest {:?} {}:{}\n", self.version, self.method, self.uri); + let res = write!(f, "\nHttpRequest {:?} {}:{}\n", + self.version, self.method, self.path); if !self.params.is_empty() { let _ = write!(f, " params: {:?}\n", self.params); } @@ -289,9 +291,6 @@ impl Future for UrlEncoded { #[cfg(test)] mod tests { use super::*; - use http::{Uri, HttpTryFrom}; - // use futures::future::{lazy, result}; - // use tokio_core::reactor::Core; use payload::Payload; #[test] @@ -300,7 +299,7 @@ mod tests { headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_static("chunked")); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); let (_, payload) = Payload::new(false); assert!(req.urlencoded(payload).is_err()); @@ -311,7 +310,7 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("xxxx")); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); let (_, payload) = Payload::new(false); assert!(req.urlencoded(payload).is_err()); @@ -322,7 +321,7 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("1000000")); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); let (_, payload) = Payload::new(false); assert!(req.urlencoded(payload).is_err()); @@ -333,7 +332,7 @@ mod tests { headers.insert(header::CONTENT_LENGTH, header::HeaderValue::from_static("10")); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); let (_, payload) = Payload::new(false); assert!(req.urlencoded(payload).is_err()); diff --git a/src/lib.rs b/src/lib.rs index 6c2d3eb23..fe0f1f8ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,6 +19,7 @@ extern crate http_range; extern crate mime; extern crate mime_guess; extern crate url; +extern crate percent_encoding; extern crate actix; mod application; diff --git a/src/logger.rs b/src/logger.rs index 3311821a7..d600a99c4 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -52,7 +52,13 @@ impl Logger { match *text { FormatText::Str(ref string) => fmt.write_str(string), FormatText::Method => req.method().fmt(fmt), - FormatText::URI => req.uri().fmt(fmt), + FormatText::URI => { + if req.query_string().is_empty() { + fmt.write_fmt(format_args!("{}", req.path())) + } else { + fmt.write_fmt(format_args!("{}?{}", req.path(), req.query_string())) + } + }, FormatText::Status => resp.status().fmt(fmt), FormatText::ResponseTime => fmt.write_fmt(format_args!("{} ms", response_time_ms)), diff --git a/src/reader.rs b/src/reader.rs index 85af482cb..c47c96908 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -1,11 +1,12 @@ use std::{self, io, ptr}; use httparse; -use http::{Method, Version, Uri, HttpTryFrom, HeaderMap}; +use http::{Method, Version, HttpTryFrom, HeaderMap}; use http::header::{self, HeaderName, HeaderValue}; use bytes::{BytesMut, BufMut}; use futures::{Async, Poll}; use tokio_io::AsyncRead; +use percent_encoding; use error::ParseError; use decode::Decoder; @@ -248,8 +249,31 @@ impl Reader { let slice = buf.split_to(len).freeze(); let path = slice.slice(path.0, path.1); - // path was found to be utf8 by httparse - let uri = Uri::from_shared(path).map_err(|_| ParseError::Uri)?; + + // manually split path, path was found to be utf8 by httparse + let uri = { + if let Ok(path) = percent_encoding::percent_decode(&path).decode_utf8() { + let parts: Vec<&str> = path.splitn(2, '?').collect(); + if parts.len() == 2 { + Some((parts[0].to_owned(), parts[1][1..].to_owned())) + } else { + Some((parts[0].to_owned(), String::new())) + } + } else { + None + } + }; + let (path, query) = if let Some(uri) = uri { + uri + } else { + let parts: Vec<&str> = unsafe{ + std::str::from_utf8_unchecked(&path)}.splitn(2, '?').collect(); + if parts.len() == 2 { + (parts[0].to_owned(), parts[1][1..].to_owned()) + } else { + (parts[0].to_owned(), String::new()) + } + }; // convert headers let mut headers = HeaderMap::with_capacity(headers_len); @@ -267,7 +291,7 @@ impl Reader { } } - let msg = HttpRequest::new(method, uri, version, headers); + let msg = HttpRequest::new(method, path, version, headers, query); let decoder = if msg.upgrade() { Some(Decoder::eof()) diff --git a/src/ws.rs b/src/ws.rs index 06bdb29f5..f39489a6b 100644 --- a/src/ws.rs +++ b/src/ws.rs @@ -325,20 +325,20 @@ impl WsWriter { #[cfg(test)] mod tests { - use http::{Method, HeaderMap, StatusCode, Uri, Version, HttpTryFrom, header}; + use http::{Method, HeaderMap, StatusCode, Version, header}; use super::{HttpRequest, SEC_WEBSOCKET_VERSION, SEC_WEBSOCKET_KEY, handshake}; #[test] fn test_handshake() { - let req = HttpRequest::new(Method::POST, Uri::try_from("/").unwrap(), - Version::HTTP_11, HeaderMap::new()); + let req = HttpRequest::new(Method::POST, "/".to_owned(), + Version::HTTP_11, HeaderMap::new(), String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::METHOD_NOT_ALLOWED), _ => panic!("should not happen"), } - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, HeaderMap::new()); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, HeaderMap::new(), String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::METHOD_NOT_ALLOWED), _ => panic!("should not happen"), @@ -347,8 +347,8 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(header::UPGRADE, header::HeaderValue::from_static("test")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::METHOD_NOT_ALLOWED), _ => panic!("should not happen"), @@ -357,8 +357,8 @@ mod tests { let mut headers = HeaderMap::new(); headers.insert(header::UPGRADE, header::HeaderValue::from_static("websocket")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::BAD_REQUEST), _ => panic!("should not happen"), @@ -369,8 +369,8 @@ mod tests { header::HeaderValue::from_static("websocket")); headers.insert(header::CONNECTION, header::HeaderValue::from_static("upgrade")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::BAD_REQUEST), _ => panic!("should not happen"), @@ -383,8 +383,8 @@ mod tests { header::HeaderValue::from_static("upgrade")); headers.insert(SEC_WEBSOCKET_VERSION, header::HeaderValue::from_static("5")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::BAD_REQUEST), _ => panic!("should not happen"), @@ -397,8 +397,8 @@ mod tests { header::HeaderValue::from_static("upgrade")); headers.insert(SEC_WEBSOCKET_VERSION, header::HeaderValue::from_static("13")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Err(err) => assert_eq!(err.status(), StatusCode::BAD_REQUEST), _ => panic!("should not happen"), @@ -413,8 +413,8 @@ mod tests { header::HeaderValue::from_static("13")); headers.insert(SEC_WEBSOCKET_KEY, header::HeaderValue::from_static("13")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); match handshake(&req) { Ok(resp) => { assert_eq!(resp.status(), StatusCode::SWITCHING_PROTOCOLS) diff --git a/tests/test_httprequest.rs b/tests/test_httprequest.rs index 0e57adc09..298fcf9dc 100644 --- a/tests/test_httprequest.rs +++ b/tests/test_httprequest.rs @@ -4,13 +4,13 @@ extern crate time; use std::str; use actix_web::*; -use http::{header, Method, Uri, Version, HeaderMap, HttpTryFrom}; +use http::{header, Method, Version, HeaderMap}; #[test] fn test_no_request_cookies() { let mut req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, HeaderMap::new()); + Method::GET, "/".to_owned(), Version::HTTP_11, HeaderMap::new(), String::new()); assert!(req.cookies().is_empty()); let _ = req.load_cookies(); assert!(req.cookies().is_empty()); @@ -23,7 +23,7 @@ fn test_request_cookies() { header::HeaderValue::from_static("cookie1=value1; cookie2=value2")); let mut req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); assert!(req.cookies().is_empty()); { let cookies = req.load_cookies().unwrap(); @@ -46,8 +46,8 @@ fn test_request_cookies() { #[test] fn test_no_request_range_header() { - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, HeaderMap::new()); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, HeaderMap::new(), String::new()); let ranges = req.range(100).unwrap(); assert!(ranges.is_empty()); } @@ -58,8 +58,8 @@ fn test_request_range_header() { headers.insert(header::RANGE, header::HeaderValue::from_static("bytes=0-4")); - let req = HttpRequest::new(Method::GET, Uri::try_from("/").unwrap(), - Version::HTTP_11, headers); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, headers, String::new()); let ranges = req.range(100).unwrap(); assert_eq!(ranges.len(), 1); assert_eq!(ranges[0].start, 0); @@ -68,8 +68,8 @@ fn test_request_range_header() { #[test] fn test_request_query() { - let req = HttpRequest::new(Method::GET, Uri::try_from("/?id=test").unwrap(), - Version::HTTP_11, HeaderMap::new()); + let req = HttpRequest::new(Method::GET, "/".to_owned(), + Version::HTTP_11, HeaderMap::new(), "id=test".to_owned()); assert_eq!(req.query_string(), "id=test"); let query: Vec<_> = req.query().collect(); @@ -79,8 +79,8 @@ fn test_request_query() { #[test] fn test_request_match_info() { - let mut req = HttpRequest::new(Method::GET, Uri::try_from("/value/?id=test").unwrap(), - Version::HTTP_11, HeaderMap::new()); + let mut req = HttpRequest::new(Method::GET, "/value/".to_owned(), + Version::HTTP_11, HeaderMap::new(), "?id=test".to_owned()); let rec = RouteRecognizer::new("/".to_owned(), vec![("/{key}/".to_owned(), 1)]); let (params, _) = rec.recognize(req.path()).unwrap(); @@ -93,14 +93,14 @@ fn test_request_match_info() { #[test] fn test_chunked() { let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, HeaderMap::new()); + Method::GET, "/".to_owned(), Version::HTTP_11, HeaderMap::new(), String::new()); assert!(!req.chunked().unwrap()); let mut headers = HeaderMap::new(); headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_static("chunked")); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); assert!(req.chunked().unwrap()); let mut headers = HeaderMap::new(); @@ -109,6 +109,6 @@ fn test_chunked() { headers.insert(header::TRANSFER_ENCODING, header::HeaderValue::from_str(s).unwrap()); let req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); assert!(req.chunked().is_err()); } diff --git a/tests/test_httpresponse.rs b/tests/test_httpresponse.rs index e4805cc14..aff8d9073 100644 --- a/tests/test_httpresponse.rs +++ b/tests/test_httpresponse.rs @@ -4,7 +4,7 @@ extern crate time; use actix_web::*; use time::Duration; -use http::{header, Method, Uri, Version, HeaderMap, HttpTryFrom}; +use http::{header, Method, Version, HeaderMap}; #[test] @@ -14,7 +14,7 @@ fn test_response_cookies() { header::HeaderValue::from_static("cookie1=value1; cookie2=value2")); let mut req = HttpRequest::new( - Method::GET, Uri::try_from("/").unwrap(), Version::HTTP_11, headers); + Method::GET, "/".to_owned(), Version::HTTP_11, headers, String::new()); let cookies = req.load_cookies().unwrap(); let resp = httpcodes::HTTPOk