From f438d5538e71305bce701f684b06c27a46dd0c98 Mon Sep 17 00:00:00 2001 From: Yong Wen Chua Date: Tue, 19 Mar 2019 10:19:10 +0800 Subject: [PATCH] Better Opaque Exact Match Error Handling and Docs (#64) * Add notes about Opaque Origins * Improve error handling for Opaque Exact matches * Cleanups * Add more test condition --- src/headers.rs | 33 +++++++++ src/lib.rs | 181 +++++++++++++++++++++++++++++++++++-------------- 2 files changed, 163 insertions(+), 51 deletions(-) diff --git a/src/headers.rs b/src/headers.rs index 161ffff..eb00dd9 100644 --- a/src/headers.rs +++ b/src/headers.rs @@ -217,6 +217,39 @@ mod tests { Client::new(rocket).expect("valid rocket instance") } + // `Origin::from_str` tests + + #[test] + fn origin_is_parsed_properly() { + let url = "https://foo.bar.xyz"; + let parsed = not_err!(Origin::from_str(url)); + assert_eq!(parsed.ascii_serialization(), url); + } + + #[test] + fn origin_parsing_strips_paths() { + // this should never really be sent by a compliant user agent + let url = "https://foo.bar.xyz/path/somewhere"; + let parsed = not_err!(Origin::from_str(url)); + let expected = "https://foo.bar.xyz"; + assert_eq!(parsed.ascii_serialization(), expected); + } + + #[test] + #[should_panic(expected = "BadOrigin")] + fn origin_parsing_disallows_invalid_origins() { + let url = "invalid_url"; + let _ = Origin::from_str(url).unwrap(); + } + + #[test] + fn origin_parses_opaque_origins() { + let url = "blob://foobar"; + let parsed = not_err!(Origin::from_str(url)); + + assert!(!parsed.is_tuple()); + } + // The following tests check that CORS Request headers are parsed correctly #[test] diff --git a/src/lib.rs b/src/lib.rs index 886bf51..4bec1d9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -309,8 +309,8 @@ pub enum Error { MissingOrigin, /// The HTTP request header `Origin` could not be parsed correctly. BadOrigin(url::ParseError), - /// The configured Allowed Origin is opaque and cannot be parsed. - OpaqueAllowedOrigin(String), + /// The configured Allowed Origins are Opaque origins. Use a Regex instead. + OpaqueAllowedOrigin(Vec), /// The request header `Access-Control-Request-Method` is required but is missing MissingRequestMethod, /// The request header `Access-Control-Request-Method` has an invalid value @@ -399,11 +399,11 @@ impl fmt::Display for Error { "The `on_response` handler of Fairing could not find the injected header from the \ Request. Either some other fairing has removed it, or this is a bug.") } - Error::OpaqueAllowedOrigin(ref origin) => write!( + Error::OpaqueAllowedOrigin(ref origins) => write!( f, - "The configured Origin '{}' does \ - not have a parsable Origin. Use a regex instead.", - origin + "The configured Origins '{}' are Opaque Origins. \ + Use regex instead.", + origins.join("; ") ), Error::RegexError(ref e) => write!(f, "{}", e), } @@ -462,7 +462,7 @@ impl Default for AllOrSome { impl AllOrSome { /// Returns whether this is an `All` variant pub fn is_all(&self) -> bool { - match *self { + match self { AllOrSome::All => true, AllOrSome::Some(_) => false, } @@ -472,6 +472,17 @@ impl AllOrSome { pub fn is_some(&self) -> bool { !self.is_all() } + + /// Unwrap a `Some` variant and get its inner value + /// + /// # Panics + /// Panics if the variant is `All` + pub fn unwrap(self) -> T { + match self { + AllOrSome::All => panic!("Attempting to unwrap an `All`"), + AllOrSome::Some(inner) => inner, + } + } } /// A wrapper type around `rocket::http::Method` to support serialization and deserialization @@ -566,6 +577,15 @@ mod method_serde { /// [ASCII Serialization](https://html.spec.whatwg.org/multipage/origin.html#ascii-serialisation-of-an-origin) /// of the origin. /// +/// # Opaque Origins +/// The [specification](https://html.spec.whatwg.org/multipage/origin.html) defines an Opaque Origin +/// as one that cannot be recreated. You can refer to the source code for the [`url::Url::origin`] +/// method to see how an Opaque Origin is determined. Examples of Opaque origins might include +/// schemes like `file://` or Browser specific schemes like `"moz-extension://` or +/// `chrome-extension://`. +/// +/// Opaque Origins cannot be matched exactly. You must use Regex to match Opaque Origins. If you +/// attempt to create [`Cors`] from [`CorsOptions`], you will get an error. /// # Warning about Regex expressions /// By default, regex expressions are /// [unanchored](https://docs.rs/regex/1.1.2/regex/struct.RegexSet.html#method.is_match). @@ -603,6 +623,15 @@ impl AllowedOrigins { /// [ASCII Serialization](https://html.spec.whatwg.org/multipage/origin.html#ascii-serialisation-of-an-origin) /// of the origin. /// + /// # Opaque Origins + /// The [specification](https://html.spec.whatwg.org/multipage/origin.html) defines an Opaque Origin + /// as one that cannot be recreated. You can refer to the source code for the [`url::Url::origin`] + /// method to see how an Opaque Origin is determined. Examples of Opaque origins might include + /// schemes like `file://` or Browser specific schemes like `"moz-extension://` or + /// `chrome-extension://`. + /// + /// Opaque Origins cannot be matched exactly. You must use Regex to match Opaque Origins. If you + /// attempt to create [`Cors`] from [`CorsOptions`], you will get an error. /// # Warning about Regex expressions /// By default, regex expressions are /// [unanchored](https://docs.rs/regex/1.1.2/regex/struct.RegexSet.html#method.is_match). @@ -626,6 +655,12 @@ impl AllowedOrigins { /// Exact matches are matched exactly with the /// [ASCII Serialization](https://html.spec.whatwg.org/multipage/origin.html#ascii-serialisation-of-an-origin) /// of the origin. + /// # Opaque Origins + /// The [specification](https://html.spec.whatwg.org/multipage/origin.html) defines an Opaque Origin + /// as one that cannot be recreated. You can refer to the source code for the [`url::Url::origin`] + /// method to see how an Opaque Origin is determined. Examples of Opaque origins might include + /// schemes like `file://` or Browser specific schemes like `"moz-extension://` or + /// `chrome-extension://`. pub fn some_exact>(exact: &[S]) -> Self { AllOrSome::Some(Origins { exact: Some(exact.iter().map(|s| s.as_ref().to_string()).collect()), @@ -687,6 +722,16 @@ impl AllowedOrigins { /// [ASCII Serialization](https://html.spec.whatwg.org/multipage/origin.html#ascii-serialisation-of-an-origin) /// of the origin. /// +/// # Opaque Origins +/// The [specification](https://html.spec.whatwg.org/multipage/origin.html) defines an Opaque Origin +/// as one that cannot be recreated. You can refer to the source code for the [`url::Url::origin`] +/// method to see how an Opaque Origin is determined. Examples of Opaque origins might include +/// schemes like `file://` or Browser specific schemes like `"moz-extension://` or +/// `chrome-extension://`. +/// +/// Opaque Origins cannot be matched exactly. You must use Regex to match Opaque Origins. If you +/// attempt to create [`Cors`] from [`CorsOptions`], you will get an error. +/// /// # Warning about Regex expressions /// By default, regex expressions are /// [unanchored](https://docs.rs/regex/1.1.2/regex/struct.RegexSet.html#method.is_match). @@ -709,6 +754,16 @@ pub struct Origins { /// Exact matches are matched exactly with the /// [ASCII Serialization](https://html.spec.whatwg.org/multipage/origin.html#ascii-serialisation-of-an-origin) /// of the origin. + /// + /// # Opaque Origins + /// The [specification](https://html.spec.whatwg.org/multipage/origin.html) defines an Opaque Origin + /// as one that cannot be recreated. You can refer to the source code for the [`url::Url::origin`] + /// method to see how an Opaque Origin is determined. Examples of Opaque origins might include + /// schemes like `file://` or Browser specific schemes like `"moz-extension://` or + /// `chrome-extension://`. + /// + /// Opaque Origins cannot be matched exactly. You must use Regex to match Opaque Origins. If you + /// attempt to create [`Cors`] from [`CorsOptions`], you will get an error. #[cfg_attr(feature = "serialization", serde(default))] pub exact: Option>, /// Origins that will be matched via __any__ regex in this list. @@ -743,20 +798,29 @@ pub(crate) struct ParsedAllowedOrigins { impl ParsedAllowedOrigins { fn parse(origins: &Origins) -> Result { - let exact: Result, Error> = match &origins.exact { - Some(exact) => exact.iter().map(|url| to_origin(url.as_str())).collect(), + let exact: Result, Error> = match &origins.exact { + Some(exact) => exact + .iter() + .map(|url| Ok((url.as_str(), to_origin(url.as_str())?))) + .collect(), None => Ok(Default::default()), }; let exact = exact?; - // Let's check if any of them is Opaque - exact.iter().try_for_each(|url| { - if !url.is_tuple() { - Err(Error::OpaqueAllowedOrigin(url.ascii_serialization())) - } else { - Ok(()) - } - })?; + // Let's check if they are Opaque + let (tuple, opaque): (Vec<_>, Vec<_>) = + exact.into_iter().partition(|(_, url)| url.is_tuple()); + + if !opaque.is_empty() { + Err(Error::OpaqueAllowedOrigin( + opaque + .into_iter() + .map(|(original, _)| original.to_string()) + .collect(), + ))? + } + + let exact = tuple.into_iter().map(|(_, url)| url).collect(); let regex = match &origins.regex { None => None, @@ -771,6 +835,7 @@ impl ParsedAllowedOrigins { } fn verify(&self, origin: &Origin) -> bool { + info_!("Verifying origin: {}", origin); match origin { Origin::Null => { info_!("Origin is null. Allowing? {}", self.allow_null); @@ -1945,39 +2010,6 @@ mod tests { Client::new(rocket).expect("valid rocket instance") } - // `to_origin` tests - - #[test] - fn origin_is_parsed_properly() { - let url = "https://foo.bar.xyz"; - let parsed = not_err!(Origin::from_str(url)); - assert_eq!(parsed.ascii_serialization(), url); - } - - #[test] - fn origin_parsing_strips_paths() { - // this should never really be sent by a compliant user agent - let url = "https://foo.bar.xyz/path/somewhere"; - let parsed = not_err!(Origin::from_str(url)); - let expected = "https://foo.bar.xyz"; - assert_eq!(parsed.ascii_serialization(), expected); - } - - #[test] - #[should_panic(expected = "BadOrigin")] - fn origin_parsing_disallows_invalid_origins() { - let url = "invalid_url"; - let _ = Origin::from_str(url).unwrap(); - } - - #[test] - fn origin_parses_opaque_origins() { - let url = "blob://foobar"; - let parsed = not_err!(Origin::from_str(url)); - - assert!(!parsed.is_tuple()); - } - // CORS options test #[test] @@ -2073,6 +2105,54 @@ mod tests { let _ = AllowedOrigins::some(&static_exact, &random_regex); } + // `ParsedAllowedOrigins::parse` tests + #[test] + fn allowed_origins_are_parsed_correctly() { + let allowed_origins = not_err!(parse_allowed_origins(&AllowedOrigins::some( + &["https://www.acme.com"], + &["^https://www.example-[A-z0-9]+.com$"] + ))); + assert!(allowed_origins.is_some()); + + let expected_exact: HashSet = [url::Url::from_str("https://www.acme.com") + .expect("not to fail") + .origin()] + .iter() + .map(Clone::clone) + .collect(); + let expected_regex = ["^https://www.example-[A-z0-9]+.com$"]; + + let actual = allowed_origins.unwrap(); + assert_eq!(expected_exact, actual.exact); + assert_eq!(expected_regex, actual.regex.expect("to be some").patterns()); + } + + #[test] + fn allowed_origins_errors_on_opaque_exact() { + let error = parse_allowed_origins(&AllowedOrigins::some::<_, &str>( + &[ + "chrome-extension://something", + "moz-extension://something", + "https://valid.com", + ], + &[], + )) + .unwrap_err(); + + match error { + Error::OpaqueAllowedOrigin(mut origins) => { + origins.sort(); + assert_eq!( + origins, + ["chrome-extension://something", "moz-extension://something"] + ); + } + others => { + panic!("Unexpected error: {:#?}", others); + } + }; + } + // The following tests check validation #[test] @@ -2135,7 +2215,6 @@ mod tests { fn validate_origin_validates_opaque_origins() { let url = "moz-extension://8c7c4444-e29f-…cb8-1ade813dbd12/js/content.js:505"; let origin = not_err!(to_parsed_origin(&url)); - println!("{:#?}", origin); let allowed_origins = not_err!(parse_allowed_origins(&AllowedOrigins::some_regex(&[ "moz-extension://.*" ])));