From 1b49237e2735bbfb551f4bbf592b1db59ddaa58e Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 5 Apr 2017 09:57:12 -0700 Subject: [PATCH 1/3] fix(uri): fix Uri.path() from containing '?' ever Closes #1113 --- src/uri.rs | 137 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 81 insertions(+), 56 deletions(-) diff --git a/src/uri.rs b/src/uri.rs index 6afe7bda..c6821a0a 100644 --- a/src/uri.rs +++ b/src/uri.rs @@ -32,8 +32,8 @@ pub struct Uri { source: ByteStr, scheme_end: Option, authority_end: Option, - query: Option, - fragment: Option, + query_start: Option, + fragment_start: Option, } impl Uri { @@ -47,8 +47,8 @@ impl Uri { source: ByteStr::from_static("*"), scheme_end: None, authority_end: None, - query: None, - fragment: None, + query_start: None, + fragment_start: None, }) } else if s.as_bytes() == b"/" { // shortcut for '/' @@ -61,13 +61,13 @@ impl Uri { source: s, scheme_end: None, authority_end: None, - query: query, - fragment: fragment, + query_start: query, + fragment_start: fragment, }) } else if s.contains("://") { // absolute-form let scheme = parse_scheme(&s); - let auth = parse_authority(&s); + let auth = Some(parse_authority(&s)); let scheme_end = scheme.expect("just checked for ':' above"); let auth_end = auth.expect("just checked for ://"); if scheme_end + 3 == auth_end { @@ -80,8 +80,8 @@ impl Uri { source: s, scheme_end: scheme, authority_end: auth, - query: query, - fragment: fragment, + query_start: query, + fragment_start: fragment, }) } else if (s.contains("/") || s.contains("?")) && !s.contains("://") { // last possibility is authority-form, above are illegal characters @@ -93,8 +93,8 @@ impl Uri { source: s, scheme_end: None, authority_end: Some(len), - query: None, - fragment: None, + query_start: None, + fragment_start: None, }) } } @@ -102,10 +102,13 @@ impl Uri { /// Get the path of this `Uri`. pub fn path(&self) -> &str { let index = self.authority_end.unwrap_or(self.scheme_end.unwrap_or(0)); - let query_len = self.query.unwrap_or(0); - let fragment_len = self.fragment.unwrap_or(0); - let end = self.source.len() - if query_len > 0 { query_len + 1 } else { 0 } - - if fragment_len > 0 { fragment_len + 1 } else { 0 }; + let end = if let Some(query) = self.query_start { + query + } else if let Some(fragment) = self.fragment_start { + fragment + } else { + self.source.len() + }; if index >= end { if self.scheme().is_some() { "/" // absolute-form MUST have path @@ -156,23 +159,23 @@ impl Uri { /// Get the query string of this `Uri`, starting after the `?`. pub fn query(&self) -> Option<&str> { - let fragment_len = self.fragment.unwrap_or(0); - let fragment_len = if fragment_len > 0 { fragment_len + 1 } else { 0 }; - if let Some(len) = self.query { - Some(&self.source[self.source.len() - len - fragment_len.. - self.source.len() - fragment_len]) - } else { - None - } + self.query_start.map(|start| { + // +1 to remove '?' + let start = start + 1; + if let Some(end) = self.fragment_start { + &self.source[start..end] + } else { + &self.source[start..] + } + }) } #[cfg(test)] fn fragment(&self) -> Option<&str> { - if let Some(len) = self.fragment { - Some(&self.source[self.source.len() - len..self.source.len()]) - } else { - None - } + self.fragment_start.map(|start| { + // +1 to remove the '#' + &self.source[start + 1..] + }) } } @@ -180,35 +183,31 @@ fn parse_scheme(s: &str) -> Option { s.find(':') } -fn parse_authority(s: &str) -> Option { - let i = s.find("://").and_then(|p| Some(p + 3)).unwrap_or(0); - - Some(&s[i..].split("/") - .next() - .unwrap_or(s) - .len() + i) +fn parse_authority(s: &str) -> usize { + let i = s.find("://").map(|p| p + 3).unwrap_or(0); + s[i..].find('/') + .or_else(|| s[i..].find('?')) + .or_else(|| s[i..].find('#')) + .map(|end| end + i) + .unwrap_or(s.len()) } fn parse_query(s: &str) -> Option { - match s.find('?') { - Some(i) => { - let frag_pos = s.find('#').unwrap_or(s.len()); - - if frag_pos < i + 1 { + s.find('?').and_then(|i| { + if let Some(frag) = s.find('#') { + if frag < i { None } else { - Some(frag_pos - i - 1) + Some(i) } - }, - None => None, - } + } else { + Some(i) + } + }) } fn parse_fragment(s: &str) -> Option { - match s.find('#') { - Some(i) => Some(s.len() - i - 1), - None => None, - } + s.find('#') } impl FromStr for Uri { @@ -241,8 +240,8 @@ impl Default for Uri { source: ByteStr::from_static("/"), scheme_end: None, authority_end: None, - query: None, - fragment: None, + query_start: None, + fragment_start: None, } } } @@ -269,8 +268,8 @@ pub fn scheme_and_authority(uri: &Uri) -> Option { source: uri.source.slice_to(uri.authority_end.expect("scheme without authority")), scheme_end: uri.scheme_end, authority_end: uri.authority_end, - query: None, - fragment: None, + query_start: None, + fragment_start: None, }) } else { None @@ -279,8 +278,8 @@ pub fn scheme_and_authority(uri: &Uri) -> Option { pub fn origin_form(uri: &Uri) -> Uri { let start = uri.authority_end.unwrap_or(uri.scheme_end.unwrap_or(0)); - let end = if let Some(f) = uri.fragment { - uri.source.len() - f - 1 + let end = if let Some(f) = uri.fragment_start { + f } else { uri.source.len() }; @@ -288,8 +287,8 @@ pub fn origin_form(uri: &Uri) -> Uri { source: uri.source.slice(start, end), scheme_end: None, authority_end: None, - query: uri.query, - fragment: None, + query_start: uri.query_start, + fragment_start: None, } } @@ -329,6 +328,7 @@ macro_rules! test_parse { #[test] fn $test_name() { let uri = Uri::from_str($str).unwrap(); + println!("{:?} = {:#?}", $str, uri); $( assert_eq!(uri.$method(), $value); )+ @@ -442,6 +442,30 @@ test_parse! { port = None, } +test_parse! { + test_uri_parse_path_with_terminating_questionmark, + "http://127.0.0.1/path?", + + scheme = Some("http"), + authority = Some("127.0.0.1"), + path = "/path", + query = Some(""), + fragment = None, + port = None, +} + +test_parse! { + test_uri_parse_absolute_form_with_empty_path_and_nonempty_query, + "http://127.0.0.1?foo=bar", + + scheme = Some("http"), + authority = Some("127.0.0.1"), + path = "/", + query = Some("foo=bar"), + fragment = None, + port = None, +} + #[test] fn test_uri_parse_error() { fn err(s: &str) { @@ -452,6 +476,7 @@ fn test_uri_parse_error() { err("htt:p//host"); err("hyper.rs/"); err("hyper.rs?key=val"); + err("?key=val"); err("localhost/"); err("localhost?key=val"); } From cb1927553ee1db4aaf09f0b77f0d9aae174114e8 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 5 Apr 2017 11:40:57 -0700 Subject: [PATCH 2/3] fix(uri): fix Uri to_origin_form to always include '/' Closes #1112 --- src/http/h1/parse.rs | 2 +- src/http/str.rs | 15 ++++++- src/uri.rs | 94 ++++++++++++++++++++++++++++++++++++++------ 3 files changed, 96 insertions(+), 15 deletions(-) diff --git a/src/http/h1/parse.rs b/src/http/h1/parse.rs index b6e85cb1..dc7e6a00 100644 --- a/src/http/h1/parse.rs +++ b/src/http/h1/parse.rs @@ -344,7 +344,7 @@ mod tests { let (req, len) = parse::(&mut raw).unwrap().unwrap(); assert_eq!(len, expected_len); assert_eq!(req.subject.0, ::Method::Get); - assert_eq!(req.subject.1, "/echo".parse().unwrap()); + assert_eq!(req.subject.1, "/echo"); assert_eq!(req.version, ::HttpVersion::Http11); assert_eq!(req.headers.len(), 1); assert_eq!(req.headers.get_raw("Host").map(|raw| &raw[0]), Some(b"hyper.rs".as_ref())); diff --git a/src/http/str.rs b/src/http/str.rs index 4479a01b..6fd22364 100644 --- a/src/http/str.rs +++ b/src/http/str.rs @@ -1,7 +1,7 @@ use std::ops::Deref; use std::str; -use bytes::Bytes; +use bytes::{Bytes, BytesMut}; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ByteStr(Bytes); @@ -38,6 +38,19 @@ impl Deref for ByteStr { } } + +impl From for Bytes { + fn from(s: ByteStr) -> Bytes { + s.0 + } +} + +impl From for BytesMut { + fn from(s: ByteStr) -> BytesMut { + s.0.into() + } +} + impl<'a> From<&'a str> for ByteStr { fn from(s: &'a str) -> ByteStr { ByteStr(Bytes::from(s)) diff --git a/src/uri.rs b/src/uri.rs index c6821a0a..e2b57c9d 100644 --- a/src/uri.rs +++ b/src/uri.rs @@ -3,6 +3,7 @@ use std::fmt::{Display, self}; use std::str::{self, FromStr}; use http::ByteStr; +use bytes::{BufMut, BytesMut}; /// The Request-URI of a Request's StartLine. /// @@ -101,14 +102,8 @@ impl Uri { /// Get the path of this `Uri`. pub fn path(&self) -> &str { - let index = self.authority_end.unwrap_or(self.scheme_end.unwrap_or(0)); - let end = if let Some(query) = self.query_start { - query - } else if let Some(fragment) = self.fragment_start { - fragment - } else { - self.source.len() - }; + let index = self.path_start(); + let end = self.path_end(); if index >= end { if self.scheme().is_some() { "/" // absolute-form MUST have path @@ -120,6 +115,31 @@ impl Uri { } } + #[inline] + fn path_start(&self) -> usize { + self.authority_end.unwrap_or(self.scheme_end.unwrap_or(0)) + } + + #[inline] + fn path_end(&self) -> usize { + if let Some(query) = self.query_start { + query + } else if let Some(fragment) = self.fragment_start { + fragment + } else { + self.source.len() + } + } + + #[inline] + fn origin_form_end(&self) -> usize { + if let Some(fragment) = self.fragment_start { + fragment + } else { + self.source.len() + } + } + /// Get the scheme of this `Uri`. pub fn scheme(&self) -> Option<&str> { if let Some(end) = self.scheme_end { @@ -226,6 +246,18 @@ impl PartialEq for Uri { } } +impl<'a> PartialEq<&'a str> for Uri { + fn eq(&self, other: & &'a str) -> bool { + self.source.as_str() == *other + } +} + +impl<'a> PartialEq for &'a str{ + fn eq(&self, other: &Uri) -> bool { + *self == other.source.as_str() + } +} + impl Eq for Uri {} impl AsRef for Uri { @@ -277,14 +309,24 @@ pub fn scheme_and_authority(uri: &Uri) -> Option { } pub fn origin_form(uri: &Uri) -> Uri { - let start = uri.authority_end.unwrap_or(uri.scheme_end.unwrap_or(0)); - let end = if let Some(f) = uri.fragment_start { - f + let range = Range(uri.path_start(), uri.origin_form_end()); + + let clone = if range.len() == 0 { + ByteStr::from_static("/") + } else if uri.source.as_bytes()[range.0] != b'/' { + let mut new = BytesMut::with_capacity(range.1 - range.0 + 1); + new.put_u8(b'/'); + new.put_slice(&uri.source.as_bytes()[range.0..range.1]); + // safety: the bytes are '/' + previous utf8 str + unsafe { ByteStr::from_utf8_unchecked(new.freeze()) } + } else if range.0 == 0 && range.1 == uri.source.len() { + uri.source.clone() } else { - uri.source.len() + uri.source.slice(range.0, range.1) }; + Uri { - source: uri.source.slice(start, end), + source: clone, scheme_end: None, authority_end: None, query_start: uri.query_start, @@ -292,6 +334,14 @@ pub fn origin_form(uri: &Uri) -> Uri { } } +struct Range(usize, usize); + +impl Range { + fn len(&self) -> usize { + self.1 - self.0 + } +} + /// An error parsing a `Uri`. #[derive(Clone, Debug)] pub struct UriError(ErrorKind); @@ -480,3 +530,21 @@ fn test_uri_parse_error() { err("localhost/"); err("localhost?key=val"); } + +#[test] +fn test_uri_to_origin_form() { + let cases = vec![ + ("/", "/"), + ("/foo?bar", "/foo?bar"), + ("/foo?bar#nope", "/foo?bar"), + ("http://hyper.rs", "/"), + ("http://hyper.rs/", "/"), + ("http://hyper.rs/path", "/path"), + ("http://hyper.rs?query", "/?query"), + ]; + + for case in cases { + let uri = Uri::from_str(case.0).unwrap(); + assert_eq!(origin_form(&uri), case.1); + } +} From 5d3499e036721c3cb495740513917dd59e09899b Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 5 Apr 2017 13:50:29 -0700 Subject: [PATCH 3/3] fix(uri): fix Uri to origin_form when path is '*' --- src/http/str.rs | 15 +-------------- src/uri.rs | 24 ++++++++++++++++-------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/http/str.rs b/src/http/str.rs index 6fd22364..4479a01b 100644 --- a/src/http/str.rs +++ b/src/http/str.rs @@ -1,7 +1,7 @@ use std::ops::Deref; use std::str; -use bytes::{Bytes, BytesMut}; +use bytes::Bytes; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ByteStr(Bytes); @@ -38,19 +38,6 @@ impl Deref for ByteStr { } } - -impl From for Bytes { - fn from(s: ByteStr) -> Bytes { - s.0 - } -} - -impl From for BytesMut { - fn from(s: ByteStr) -> BytesMut { - s.0.into() - } -} - impl<'a> From<&'a str> for ByteStr { fn from(s: &'a str) -> ByteStr { ByteStr(Bytes::from(s)) diff --git a/src/uri.rs b/src/uri.rs index e2b57c9d..c1f72736 100644 --- a/src/uri.rs +++ b/src/uri.rs @@ -44,13 +44,7 @@ impl Uri { Err(UriError(ErrorKind::Empty)) } else if s.as_bytes() == b"*" { // asterisk-form - Ok(Uri { - source: ByteStr::from_static("*"), - scheme_end: None, - authority_end: None, - query_start: None, - fragment_start: None, - }) + Ok(asterisk_form()) } else if s.as_bytes() == b"/" { // shortcut for '/' Ok(Uri::default()) @@ -308,11 +302,24 @@ pub fn scheme_and_authority(uri: &Uri) -> Option { } } +#[inline] +fn asterisk_form() -> Uri { + Uri { + source: ByteStr::from_static("*"), + scheme_end: None, + authority_end: None, + query_start: None, + fragment_start: None, + } +} + pub fn origin_form(uri: &Uri) -> Uri { let range = Range(uri.path_start(), uri.origin_form_end()); let clone = if range.len() == 0 { ByteStr::from_static("/") + } else if uri.source.as_bytes()[range.0] == b'*' { + return asterisk_form(); } else if uri.source.as_bytes()[range.0] != b'/' { let mut new = BytesMut::with_capacity(range.1 - range.0 + 1); new.put_u8(b'/'); @@ -541,10 +548,11 @@ fn test_uri_to_origin_form() { ("http://hyper.rs/", "/"), ("http://hyper.rs/path", "/path"), ("http://hyper.rs?query", "/?query"), + ("*", "*"), ]; for case in cases { let uri = Uri::from_str(case.0).unwrap(); - assert_eq!(origin_form(&uri), case.1); + assert_eq!(origin_form(&uri), case.1, "{:?}", case); } }