soup-headers: misc improvements
authorDan Winship <danw@gnome.org>
Wed, 21 Dec 2011 16:50:41 +0000 (11:50 -0500)
committerDan Winship <danw@gnome.org>
Wed, 21 Dec 2011 16:55:25 +0000 (11:55 -0500)
Return an error if the headers start with '\0', ignore header lines
with zero-length header names, and convert excess CRs to spaces. All
of these could possibly occur in received headers, and were previously
hitting g_return_if_fails().

Add a bunch of test cases to header-tests to test these cases (and
some others that we already handled correctly but weren't testing).

Fix the documentation of @str on soup_headers_parse* to reflect
reality.

Based on a patch from Simon McVittie.
https://bugzilla.gnome.org/show_bug.cgi?id=666316

libsoup/soup-headers.c
tests/header-parsing.c

index 58d53a6..965f9da 100644 (file)
@@ -17,8 +17,8 @@
 /**
  * soup_headers_parse:
  * @str: the header string (including the Request-Line or Status-Line,
- * and the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ *   but not the trailing blank line)
+ * @len: length of @str
  * @dest: #SoupMessageHeaders to store the header values in
  *
  * Parses the headers of an HTTP request or response in @str and
@@ -37,15 +37,14 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
 {
        const char *headers_start;
        char *headers_copy, *name, *name_end, *value, *value_end;
-       char *eol, *sol;
+       char *eol, *sol, *p;
        gboolean success = FALSE;
 
        g_return_val_if_fail (str != NULL, FALSE);
        g_return_val_if_fail (dest != NULL, FALSE);
 
-       /* Technically, the grammar does allow NUL bytes in the
-        * headers, but this is probably a bug, and if it's not, we
-        * can't deal with them anyway.
+       /* RFC 2616 does allow NUL bytes in the headers, but httpbis
+        * is changing that, and we can't deal with them anyway.
         */
        if (memchr (str, '\0', len))
                return FALSE;
@@ -70,11 +69,16 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
        while (*(value_end + 1)) {
                name = value_end + 1;
                name_end = strchr (name, ':');
-               if (!name_end || name + strcspn (name, " \t\r\n") < name_end) {
-                       /* Bad header; just ignore this line. Note
-                        * that if it has continuation lines, we'll
-                        * end up ignoring them too since they'll
-                        * start with spaces.
+
+               /* Reject if there is no ':', or the header name is
+                * empty, or it contains whitespace.
+                */
+               if (!name_end ||
+                   name_end == name ||
+                   name + strcspn (name, " \t\r\n") < name_end) {
+                       /* Ignore this line. Note that if it has
+                        * continuation lines, we'll end up ignoring
+                        * them too since they'll start with spaces.
                         */
                        value_end = strchr (name, '\n');
                        if (!value_end)
@@ -127,6 +131,10 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest)
                        eol--;
                *eol = '\0';
 
+               /* convert (illegal) '\r's to spaces */
+               for (p = strchr (value, '\r'); p; p = strchr (p, '\r'))
+                       *p = ' ';
+
                soup_message_headers_append (dest, name, value);
         }
        success = TRUE;
@@ -138,8 +146,8 @@ done:
 
 /**
  * soup_headers_parse_request:
- * @str: the header string (including the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ * @str: the headers (up to, but not including, the trailing blank line)
+ * @len: length of @str
  * @req_headers: #SoupMessageHeaders to store the header values in
  * @req_method: (out) (allow-none): if non-%NULL, will be filled in with the
  * request method
@@ -169,7 +177,7 @@ soup_headers_parse_request (const char          *str,
        unsigned long major_version, minor_version;
        char *p;
 
-       g_return_val_if_fail (str && *str, SOUP_STATUS_MALFORMED);
+       g_return_val_if_fail (str != NULL, SOUP_STATUS_MALFORMED);
 
        /* RFC 2616 4.1 "servers SHOULD ignore any empty line(s)
         * received where a Request-Line is expected."
@@ -325,8 +333,8 @@ soup_headers_parse_status_line (const char       *status_line,
 
 /**
  * soup_headers_parse_response:
- * @str: the header string (including the trailing blank line)
- * @len: length of @str up to (but not including) the terminating blank line.
+ * @str: the headers (up to, but not including, the trailing blank line)
+ * @len: length of @str
  * @headers: #SoupMessageHeaders to store the header values in
  * @ver: (out) (allow-none): if non-%NULL, will be filled in with the HTTP
  * version
@@ -352,7 +360,7 @@ soup_headers_parse_response (const char          *str,
 {
        SoupHTTPVersion version;
 
-       g_return_val_if_fail (str && *str, FALSE);
+       g_return_val_if_fail (str != NULL, FALSE);
 
        /* Workaround for broken servers that send extra line breaks
         * after a response, which we then see prepended to the next
index 63a29bb..626b27c 100644 (file)
@@ -19,7 +19,7 @@ static struct RequestTest {
        guint status;
        const char *method, *path;
        SoupHTTPVersion version;
-       Header headers[4];
+       Header headers[10];
 } reqtests[] = {
        /**********************/
        /*** VALID REQUESTS ***/
@@ -206,7 +206,7 @@ static struct RequestTest {
        /* RFC 2616 section 19.3 says we SHOULD accept these */
 
        { "LF instead of CRLF after header",
-         "GET / HTTP/1.1\nHost: example.com\nConnection: close\n", -1,
+         "GET / HTTP/1.1\r\nHost: example.com\nConnection: close\n", -1,
          SOUP_STATUS_OK,
          "GET", "/", SOUP_HTTP_1_1,
          { { "Host", "example.com" },
@@ -224,6 +224,18 @@ static struct RequestTest {
          }
        },
 
+       { "Mixed CRLF/LF",
+         "GET / HTTP/1.1\r\na: b\r\nc: d\ne: f\r\ng: h\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           { "c", "d" },
+           { "e", "f" },
+           { "g", "h" },
+           { NULL }
+         }
+       },
+
        { "Req w/ incorrect whitespace in Request-Line",
          "GET  /\tHTTP/1.1\r\nHost: example.com\r\n", -1,
          SOUP_STATUS_OK,
@@ -242,7 +254,11 @@ static struct RequestTest {
          }
        },
 
-       /* qv bug 579318, do_bad_header_tests() below */
+       /* If the request/status line is parseable, then we
+        * just ignore any invalid-looking headers after that.
+        * (qv bug 579318).
+        */
+
        { "Req w/ mangled header",
          "GET / HTTP/1.1\r\nHost: example.com\r\nFoo one\r\nBar: two\r\n", -1,
          SOUP_STATUS_OK,
@@ -253,6 +269,77 @@ static struct RequestTest {
          }
        },
 
+       { "First header line is continuation",
+         "GET / HTTP/1.1\r\n b\r\nHost: example.com\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "Host", "example.com" },
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
+       { "Zero-length header name",
+         "GET / HTTP/1.1\r\na: b\r\n: example.com\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
+       { "CR in header name",
+         "GET / HTTP/1.1\r\na: b\r\na\rb: cd\r\nx\r: y\r\n\rz: w\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
+       { "CR in header value",
+         "GET / HTTP/1.1\r\na: b\r\nHost: example\rcom\r\np: \rq\r\ns: t\r\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           { "Host", "example com" },  /* CR in the middle turns to space */
+           { "p", "q" },               /* CR at beginning is ignored */
+           { "s", "t" },               /* CR at end is ignored */
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
+       { "Tab in header name",
+         "GET / HTTP/1.1\r\na: b\r\na\tb: cd\r\nx\t: y\r\np: q\r\n\tz: w\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           /* Tab anywhere in the header name causes it to be
+            * ignored... except at beginning of line where it's a
+            * continuation line
+            */
+           { "p", "q z: w" },
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
+       { "Tab in header value",
+         "GET / HTTP/1.1\r\na: b\r\nab: c\td\r\nx: \ty\r\nz: w\t\r\nc: d\r\n", -1,
+         SOUP_STATUS_OK,
+         "GET", "/", SOUP_HTTP_1_1,
+         { { "a", "b" },
+           { "ab", "c\td" },   /* internal tab preserved */
+           { "x", "y" },       /* leading tab ignored */
+           { "z", "w" },       /* trailing tab ignored */
+           { "c", "d" },
+           { NULL }
+         }
+       },
+
        /************************/
        /*** INVALID REQUESTS ***/
        /************************/
@@ -299,6 +386,13 @@ static struct RequestTest {
          { { NULL } }
        },
 
+       { "NUL at beginning of Method",
+         "\x00 / HTTP/1.1\r\nHost: example.com\r\n", 35,
+         SOUP_STATUS_BAD_REQUEST,
+         NULL, NULL, -1,
+         { { NULL } }
+       },
+
        { "NUL in Path",
          "GET /\x00 HTTP/1.1\r\nHost: example.com\r\n", 38,
          SOUP_STATUS_BAD_REQUEST,
@@ -306,7 +400,14 @@ static struct RequestTest {
          { { NULL } }
        },
 
-       { "NUL in Header",
+       { "NUL in header name",
+         "GET / HTTP/1.1\r\n\x00: silly\r\n", 37,
+         SOUP_STATUS_BAD_REQUEST,
+         NULL, NULL, -1,
+         { { NULL } }
+       },
+
+       { "NUL in header value",
          "GET / HTTP/1.1\r\nHost: example\x00com\r\n", 37,
          SOUP_STATUS_BAD_REQUEST,
          NULL, NULL, -1,
@@ -535,13 +636,25 @@ static struct ResponseTest {
          { { NULL } }
        },
 
+       { "NUL at start",
+         "\x00HTTP/1.1 200 OK\r\nFoo: bar\r\n", 28,
+         -1, 0, NULL,
+         { { NULL } }
+       },
+
        { "NUL in Reason Phrase",
          "HTTP/1.1 200 O\x00K\r\nFoo: bar\r\n", 28,
          -1, 0, NULL,
          { { NULL } }
        },
 
-       { "NUL in Header",
+       { "NUL in header name",
+         "HTTP/1.1 200 OK\r\nF\x00oo: bar\r\n", 28,
+         -1, 0, NULL,
+         { { NULL } }
+       },
+
+       { "NUL in header value",
          "HTTP/1.1 200 OK\r\nFoo: b\x00ar\r\n", 28,
          -1, 0, NULL,
          { { NULL } }