Add more header validation (#188)

This adds validation of header values on receive, and of both header
names and header values on send. This doesn't change the return
type of set to be a Result, it just validates when the request is
sent. Also removes the section in the README describing handling
of invalid headers, and updates a test that verified acceptance of
non-ASCII headers so that it verifies rejection of them instead.

README.md

@@ -82,21 +82,6 @@ tree and an obvious API. It is inspired by libraries like
 [superagent](http://visionmedia.github.io/superagent/) and
 [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API).
 
-This library does not try to enforce web standards correctness. It uses HTTP/1.1,
-but whether the request is _perfect_ HTTP/1.1 compatible is up to the user of the
-library. For example:
-
-```rust
-    let resp = ureq::post("https://myapi.acme.com/blah")
-        .set("Jättegött", "Vegankörv")
-        .call();
-```
-
-The header name and value would be encoded in utf-8 and sent, but that is actually not
-correct according to spec cause an HTTP header name should be ascii. The absolutely
-correct way would be to have `.set(header, value)` return a `Result`. This library opts
-for convenience over correctness, so the decision is left to the user.
-
 ### Sync forever
 
 This library uses blocking socket reads and writes. When it was

src/header.rs

@@ -63,6 +63,13 @@ impl Header {
     pub fn is_name(&self, other: &str) -> bool {
         self.name().eq_ignore_ascii_case(other)
     }
+
+    pub(crate) fn validate(&self) -> Result<(), Error> {
+        if !valid_name(self.name()) || !valid_value(&self.line.as_str()[self.index + 1..]) {
+            return Err(Error::BadHeader);
+        }
+        Ok(())
+    }
 }
 
 pub fn get_header<'a, 'b>(headers: &'b [Header], name: &'a str) -> Option<&'b str> {
@@ -89,7 +96,7 @@ pub fn add_header(headers: &mut Vec<Header>, header: Header) {
     headers.push(header);
 }
 
-// https://tools.ietf.org/html/rfc7230#section-3.2.3
+// https://tools.ietf.org/html/rfc7230#section-3.2
 // Each header field consists of a case-insensitive field name followed
 // by a colon (":"), optional leading whitespace, the field value, and
 // optional trailing whitespace.
@@ -112,6 +119,32 @@ fn is_tchar(b: u8) -> bool {
     }
 }
 
+// https://tools.ietf.org/html/rfc7230#section-3.2
+// Note that field-content has an errata:
+// https://www.rfc-editor.org/errata/eid4189
+// field-value    = *( field-content / obs-fold )
+// field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+// field-vchar    = VCHAR / obs-text
+//
+// obs-fold       = CRLF 1*( SP / HTAB )
+//               ; obsolete line folding
+//               ; see Section 3.2.4
+// https://tools.ietf.org/html/rfc5234#appendix-B.1
+// VCHAR          =  %x21-7E
+//                        ; visible (printing) characters
+fn valid_value(value: &str) -> bool {
+    value.bytes().all(is_field_vchar_or_obs_fold)
+}
+
+#[inline]
+fn is_field_vchar_or_obs_fold(b: u8) -> bool {
+    match b {
+        b' ' | b'\t' => true,
+        0x21..=0x7E => true,
+        _ => false,
+    }
+}
+
 impl FromStr for Header {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
@@ -124,11 +157,9 @@ impl FromStr for Header {
             return Err(Error::BadHeader);
         }
 
-        if !valid_name(&line[0..index]) {
+        let header = Header { line, index };
-            return Err(Error::BadHeader);
+        header.validate()?;
-        }
+        Ok(header)
-
-        Ok(Header { line, index })
     }
 }
 
@@ -144,12 +175,39 @@ fn test_valid_name() {
     assert!(!valid_name("Gödel"));
 }
 
+#[test]
+fn test_valid_value() {
+    assert!(valid_value("example"));
+    assert!(valid_value("foo bar"));
+    assert!(valid_value(" foobar "));
+    assert!(valid_value(" foo\tbar "));
+    assert!(valid_value(" foo~"));
+    assert!(valid_value(" !bar"));
+    assert!(valid_value(" "));
+    assert!(!valid_value(" \nfoo"));
+    assert!(!valid_value("foo\x7F"));
+}
+
 #[test]
 fn test_parse_invalid_name() {
-    let h = "Content-Type   :".parse::<Header>();
+    let cases = vec![
-    match h {
+        "Content-Type  :",
-        Err(Error::BadHeader) => {}
+        " Content-Type: foo",
-        h => assert!(false, "expected BadHeader error, got {:?}", h),
+        "Content-Type foo",
+        "\"some-header\": foo",
+        "Gödel: Escher, Bach",
+        "Foo: \n",
+        "Foo: \nbar",
+        "Foo: \x7F bar",
+    ];
+    for c in cases {
+        let result = c.parse::<Header>();
+        assert!(
+            matches!(result, Err(Error::BadHeader)),
+            "'{}'.parse(): expected BadHeader, got {:?}",
+            c,
+            result
+        );
     }
 }
 

src/lib.rs

@@ -7,7 +7,6 @@
 //! * Minimal dependency tree
 //! * Obvious API
 //! * Blocking API
-//! * Convenience over correctness
 //! * No use of unsafe
 //!
 //! ```

src/request.rs

@@ -112,6 +112,9 @@ impl Request {
     }
 
     fn do_call(&self, payload: Payload) -> Result<Response> {
+        for h in &self.headers {
+            h.validate()?;
+        }
         let response = self.to_url().and_then(|url| {
             let reader = payload.into_read();
             let unit = Unit::new(&self, &url, true, &reader);

src/test/simple.rs

@@ -10,7 +10,10 @@ fn header_passing() {
         assert_eq!(unit.header("X-Foo").unwrap(), "bar");
         test::make_response(200, "OK", vec!["X-Bar: foo"], vec![])
     });
-    let resp = get("test://host/header_passing").set("X-Foo", "bar").call().unwrap();
+    let resp = get("test://host/header_passing")
+        .set("X-Foo", "bar")
+        .call()
+        .unwrap();
     assert_eq!(resp.status(), 200);
     assert!(resp.has("X-Bar"));
     assert_eq!(resp.header("X-Bar").unwrap(), "foo");
@@ -26,7 +29,8 @@ fn repeat_non_x_header() {
     let resp = get("test://host/repeat_non_x_header")
         .set("Accept", "bar")
         .set("Accept", "baz")
-        .call().unwrap();
+        .call()
+        .unwrap();
     assert_eq!(resp.status(), 200);
 }
 
@@ -44,7 +48,8 @@ fn repeat_x_header() {
     let resp = get("test://host/repeat_x_header")
         .set("X-Forwarded-For", "130.240.19.2")
         .set("X-Forwarded-For", "130.240.19.3")
-        .call().unwrap();
+        .call()
+        .unwrap();
     assert_eq!(resp.status(), 200);
 }
 
@@ -156,11 +161,12 @@ fn non_ascii_header() {
     });
     let resp = get("test://host/non_ascii_header")
         .set("Bäd", "Headör")
-        .call().unwrap();
+        .call();
-    // surprisingly, this is ok, because this lib is not about enforcing standards.
+    assert!(
-    assert!(resp.ok());
+        matches!(resp, Err(Error::BadHeader)),
-    assert_eq!(resp.status(), 200);
+        "expected Some(&BadHeader), got {:?}",
-    assert_eq!(resp.status_text(), "OK");
+        resp
+    );
 }
 
 #[test]
@@ -184,7 +190,8 @@ pub fn header_with_spaces_before_value() {
     });
     let resp = get("test://host/space_before_value")
         .set("X-Test", "     value")
-        .call().unwrap();
+        .call()
+        .unwrap();
     assert_eq!(resp.status(), 200);
 }