Implement stricter status-line parsing.

* src/mktable.c (gen_status_line): New lookup table generator. * src/ne_utils.c (ne_parse_statusline): Reimplement. * test/util-tests.c: Update test cases. (status_lines): Better error reporting. * test/twooh7.c (errors): Update to use compliant status-lines. * src/ne_207.c (end_element): Update debugging only. * doc/ref/status.xml: Update.
notroj · Apr 23, 2024 · 8f2d015 · 8f2d015
1 parent 8187d47
commit 8f2d015
Show file tree

Hide file tree

Showing 7 changed files with 138 additions and 71 deletions.
diff --git a/doc/ref/status.xml b/doc/ref/status.xml
@@ -15,29 +15,39 @@
 	<funcsynopsis><funcsynopsisinfo>#include &lt;ne_utils.h&gt;
 
 typedef struct {
-    int major_version, minor_version;
-    int code, klass;
-    const char *reason_phrase;
+    int major_version;
+    int minor_version;
+    int code;
+    int klass;
+    char *reason_phrase;
 } <type>ne_status</type>;</funcsynopsisinfo></funcsynopsis>
 
       </refsynopsisdiv>
 
       <refsect1>
 	<title>Description</title>
 
-	<para>An <type>ne_status</type> type represents an HTTP
-response status; used in response messages giving a result of request.
-The <structfield>major_version</structfield> and
-<structfield>minor_version</structfield> fields give the HTTP version
-supported by the server issuing the response.  The
-<structfield>code</structfield> field gives the status code of the
-result (lying between 100 and 999 inclusive), and the
-<structfield>klass</structfield> field gives the
-class, which is equal to the most significant digit
-of the status.</para>
+	<para>The <type>ne_status</type> type represents an HTTP
+	response status; used in response messages giving a result of
+	request.  The <structfield>major_version</structfield> and
+	<structfield>minor_version</structfield> fields give the
+	protocol version supported by the server issuing the response.
+	The <structfield>code</structfield> field gives the status
+	code of the result (lying between <literal>100</literal> and
+	<literal>599</literal> inclusive), and the
+	<structfield>klass</structfield> field gives the class, which
+	is equal to the most significant digit of the status. The
+	<structfield>reason_phrase</structfield> field gives a
+	human-readable string describing the status, which is purely
+	informational (and optional) - any interpretation of the
+	response status must be done using the
+	<structfield>klass</structfield> and/or
+	<structfield>code</structfield> fields.</para>
 
-	<para>There are five classes of HTTP status code defined by
-	RFC2616:</para>
+	<para>There are five classes of response status code defined
+	for HTTP/1.1 (see <ulink
+	url="https://www.rfc-editor.org/rfc/rfc9110#name-status-codes">RFC
+	9110 ẞ15</ulink>):</para>
 
 	<variablelist>
 	  <varlistentry>

diff --git a/src/mktable.c b/src/mktable.c
@@ -92,6 +92,28 @@ static unsigned char gen_extparam(unsigned char ch)
     }
 }
 
+/*
+ * Map: '0'-'9' => 0-9
+ * reason-phrase characters => 0-10
+ * bad things => 99
+ *  
+ * RFC 9112: reason-phrase  = 1*( HTAB / SP / VCHAR / obs-text )
+ * RFC 5234: VCHAR          = %x21-7E
+ * RFC 9110: obs-text       = %x80-FF
+ */
+static unsigned char gen_status_line(unsigned char ch)
+{
+    if (ch >= '0' && ch <= '9')
+        return ch - '0';
+
+    if (ch == '\t' || ch == ' ' 
+        || (ch >= 0x21 && ch != 0x7F)) {
+        return 10;
+    }
+
+    return 99;
+}
+
 #define FLAG_DECIMAL (0x01)
 #define FLAG_SHORT   (0x02)
 
@@ -105,6 +127,7 @@ static const struct {
     { "validb64", valid_b64, FLAG_DECIMAL | FLAG_SHORT },
     { "decodeb64", decode_b64, 0 },
     { "quote", gen_quote, FLAG_DECIMAL | FLAG_SHORT },
+    { "status_line", gen_status_line, FLAG_DECIMAL | FLAG_SHORT },
     { "extparam", gen_extparam, FLAG_DECIMAL | FLAG_SHORT },
     { "safe_username", safe_username, FLAG_DECIMAL | FLAG_SHORT },
 };

diff --git a/src/ne_207.c b/src/ne_207.c
@@ -209,15 +209,15 @@ end_element(void *userdata, int state, const char *nspace, const char *name)
             if (p->status.reason_phrase) ne_free(p->status.reason_phrase);
 	    if (ne_parse_statusline(cdata, &p->status)) {
 		char buf[500];
-		NE_DEBUG(NE_DBG_HTTP, "Status line: %s\n", cdata);
+                NE_DEBUG(NE_DBG_HTTP, "[207] Invalid status-line: [%s]\n", cdata);
 		ne_snprintf(buf, 500, 
 			    _("Invalid HTTP status line in status element "
                               "at line %d of response:\nStatus line was: %s"),
 			    ne_xml_currentline(p->parser), cdata);
 		ne_xml_set_error(p->parser, buf);
 		return -1;
 	    } else {
-		NE_DEBUG(NE_DBG_XML, "Decoded status line: %s\n", cdata);
+		NE_DEBUG(NE_DBG_XML, "[207] valid status-line: %s\n", cdata);
 	    }
 	}
 	break;

diff --git a/src/ne_utils.c b/src/ne_utils.c
@@ -1,6 +1,6 @@
 /* 
    HTTP utility functions
-   Copyright (C) 1999-2021, Joe Orton <joe@manyfish.co.uk>
+   Copyright (C) 1999-2024, Joe Orton <joe@manyfish.co.uk>
 
    This library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Library General Public
@@ -187,47 +187,74 @@ int ne_has_support(int feature)
     }
 }
 
+/* Lookup table - digit values=0-9, reason_phrase=0-10. */
+
+/* Generated with 'mktable status_line', do not alter here -- */
+static const unsigned char table_status_line[256] = {
+/* x00 */ 99, 99, 99, 99, 99, 99, 99, 99, 99, 10, 99, 99, 99, 99, 99, 99,
+/* x10 */ 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99, 99,
+/* x20 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* x30 */ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10, 10, 10,
+/* x40 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* x50 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* x60 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* x70 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 99,
+/* x80 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* x90 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xA0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xB0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xC0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xD0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xE0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
+/* xF0 */ 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10
+}; /* -- Generated code from 'mktable status_line' ends. */
+
+/* Strict parser per RFC9112ẞ4:
+ *
+ *    status-line = HTTP-version SP status-code SP [ reason-phrase ]
+ *  HTTP-version  = HTTP-name "/" DIGIT "." DIGIT
+ * reason-phrase  = 1*( HTAB / SP / VCHAR / obs-text )
+ */
 int ne_parse_statusline(const char *status_line, ne_status *st)
 {
-    const char *part;
-    int major, minor, status_code, klass;
+    const unsigned char *p = (const unsigned char *)status_line, *rp;
+    unsigned int major, minor, status_code, klass;
 
-    /* skip leading garbage if any. */
-    part = strstr(status_line, "HTTP/");
-    if (part == NULL) return -1;
+    /* p => status-line */
+    if (strncmp((const char *)p, "HTTP/", 5) != 0)
+        return -1;
 
     minor = major = 0;
+    p += 5;
 
-    /* Parse version string, skipping leading zeroes. */
-    for (part += 5; *part != '\0' && isdigit(*part); part++)
-	major = major*10 + (*part-'0');
+    /* X.Y */
+    if ((major = table_status_line[*p++]) > 9)
+        return -1;
+    if (*p++ != '.')
+        return -1;
+    if ((minor = table_status_line[*p++]) > 9)
+        return -1;
 
-    if (*part++ != '.') return -1;
+    if (*p++ != ' ') return -1;
 
-    for (;*part != '\0' && isdigit(*part); part++)
-	minor = minor*10 + (*part-'0');
+    if ((klass = table_status_line[p[0]]) > 5 /* note 5xx maximum */
+        || table_status_line[p[1]] > 9 || table_status_line[p[2]] > 9
+        || p[3] != ' ')
+        return -1;
 
-    if (*part != ' ') return -1;
+    status_code = klass * 100 + table_status_line[p[1]] * 10
+        + table_status_line[p[2]];
 
-    /* Skip any spaces */
-    for (; *part == ' '; part++) /* noop */;
-
-    /* Parse the Status-Code; part now points at the first Y in
-     * "HTTP/x.x YYY". */
-    if (!isdigit(part[0]) || !isdigit(part[1]) || !isdigit(part[2]) ||
-	(part[3] != '\0' && part[3] != ' ')) return -1;
-    status_code = 100*(part[0]-'0') + 10*(part[1]-'0') + (part[2]-'0');
-    klass = part[0]-'0';
-
-    /* Skip whitespace between status-code and reason-phrase */
-    for (part+=3; *part == ' ' || *part == '\t'; part++) /* noop */;
-
-    /* part now may be pointing to \0 if reason phrase is blank */
+    rp = p += 4; /* p => [ reason-phrase ] */
+    while (table_status_line[*p] < 11) /* note this terminates for *p == '\0' */
+        p++;
 
     /* Fill in the results */
     st->major_version = major;
     st->minor_version = minor;
-    st->reason_phrase = ne_strclean(ne_strdup(part));
+    st->reason_phrase = ne_malloc(p - rp + 1);
+    ne_strnzcpy(st->reason_phrase, (const char *)rp, p - rp + 1);
+    ne_strclean(st->reason_phrase);
     st->code = status_code;
     st->klass = klass;
     return 0;

diff --git a/src/ne_utils.h b/src/ne_utils.h
@@ -96,7 +96,7 @@ void ne_debug(int ch, const char *, ...) ne_attribute((format(printf, 2, 3)));
 typedef struct {
     int major_version;
     int minor_version;
-    int code; /* Status-Code value */
+    int code; /* Status-Code value (100..599 inclusive) */
     int klass; /* Class of Status-Code (1-5) */
     char *reason_phrase;
 } ne_status;

diff --git a/test/props.c b/test/props.c
@@ -264,7 +264,6 @@ static int run_207_response(char *resp, const char *expected)
     ctx.p207 = p207;
     ne_xml_push_handler(p, tos_startprop, tos_cdata, tos_endprop, &ctx);
 
-
     ONREQ(ne_request_dispatch(req));
 
     CALL(await_server());
@@ -318,10 +317,10 @@ static int two_oh_seven(void)
 
         /* test multiple responses */
         { MULTI_207(RESP_207("/hello/world", STAT_207("200 OK"))
-                    RESP_207("/foo/bar", STAT_207("999 French Fries"))), 
+                    RESP_207("/foo/bar", STAT_207("599 French Fries"))),
           "start-resp[/hello/world];end-resp[/hello/world]-status={200 OK};"
           "start-resp[/foo/bar];end-resp[/foo/bar]"
-          "-status={999 French Fries};" 
+          "-status={599 French Fries};"
         },
 
         /* test multiple propstats in multiple responses */
@@ -330,16 +329,16 @@ static int two_oh_seven(void)
                              PSTAT_207(STAT_207("432 Deux"))
                              PSTAT_207(STAT_207("543 Trois")))
                     RESP_207("/be/ta",
-                             PSTAT_207(STAT_207("787 Quatre"))
-                             PSTAT_207(STAT_207("878 Cinq")))),
+                             PSTAT_207(STAT_207("587 Quatre"))
+                             PSTAT_207(STAT_207("578 Cinq")))),
           "start-resp[/al/pha];"
           "start-pstat[/al/pha-1];end-pstat[/al/pha-1]-status={321 Une};"
           "start-pstat[/al/pha-2];end-pstat[/al/pha-2]-status={432 Deux};"
           "start-pstat[/al/pha-3];end-pstat[/al/pha-3]-status={543 Trois};"
           "end-resp[/al/pha];"
           "start-resp[/be/ta];"
-          "start-pstat[/be/ta-1];end-pstat[/be/ta-1]-status={787 Quatre};"
-          "start-pstat[/be/ta-2];end-pstat[/be/ta-2]-status={878 Cinq};"
+          "start-pstat[/be/ta-1];end-pstat[/be/ta-1]-status={587 Quatre};"
+          "start-pstat[/be/ta-2];end-pstat[/be/ta-2]-status={578 Cinq};"
           "end-resp[/be/ta];"
         },
 
@@ -355,9 +354,9 @@ static int two_oh_seven(void)
 
         /* tests for propstat status */
         { MULTI_207(RESP_207("/pstat",
-                            PSTAT_207("<D:prop/>" STAT_207("666 Doomed")))),
+                            PSTAT_207("<D:prop/>" STAT_207("466 Doomed")))),
           "start-resp[/pstat];start-pstat[/pstat-1];"
-          "end-pstat[/pstat-1]-status={666 Doomed};end-resp[/pstat];" },
+          "end-pstat[/pstat-1]-status={466 Doomed};end-resp[/pstat];" },
 
         { MULTI_207(RESP_207("/pstat", PSTAT_207("<D:status/>"))),
           "start-resp[/pstat];start-pstat[/pstat-1];"
@@ -382,7 +381,7 @@ static int two_oh_seven(void)
         { MULTI_207("<D:fish-food/>blargl" 
                     RESP_207("/b<ping-pong/>ar", "<D:sausages/>"
                              PSTAT_207("<D:hello-mum/>blergl") 
-                             STAT_207("200 <pong-ping/> OK") "foop"
+                             STAT_207("200 OK") "<D:blah>foop</D:blah>"
                              DESCR_207(DESCR_REM) "carroon") 
                     "carapi"), 
           "start-resp[/bar];start-pstat[/bar-1];end-pstat[/bar-1];"

diff --git a/test/util-tests.c b/test/util-tests.c
@@ -45,19 +45,13 @@ static const struct {
 } accept_sl[] = {
     /* These are really valid. */
     { "HTTP/1.1 200 OK", 1, 1, 200, "OK" },
-    { "HTTP/1.1000 200 OK", 1, 1000, 200, "OK" },
-    { "HTTP/1000.1000 200 OK", 1000, 1000, 200, "OK" },
-    { "HTTP/00001.1 200 OK", 1, 1, 200, "OK" },
-    { "HTTP/1.00001 200 OK", 1, 1, 200, "OK" },
-    { "HTTP/99.99 999 99999", 99, 99, 999, "99999" },
-    { "HTTP/1.1 100 ", 1, 1, 100, "" },
+    { "HTTP/9.9 599 OK", 9, 9, 599, "OK" },
+    { "HTTP/1.0 123 OK-is-OK1234", 1, 0, 123, "OK-is-OK1234" },
+    { "HTTP/1.1 100 Alpha\tBeta", 1, 1, 100, "Alpha Beta" }, /* should be cleaned. */
+    { "HTTP/1.1 100 Alpha  Beta", 1, 1, 100, "Alpha  Beta" },
+    { "HTTP/1.1 100 fØØbÆr", 1, 1, 100, "f    b  r" }, /* UTF-8 should be cleaned */
 
     /* these aren't really valid but we should be able to parse them. */
-    { "HTTP/1.1 100", 1, 1, 100, "" },
-    { "HTTP/1.1   200   OK", 1, 1, 200, "OK" },
-    { "HTTP/1.1   200 \t  OK", 1, 1, 200, "OK" },
-    { "   HTTP/1.1 200 OK", 1, 1, 200, "OK" },
-    { "Norman is a dog HTTP/1.1 200 OK", 1, 1, 200, "OK" },
     { NULL }
 };
 
@@ -76,6 +70,19 @@ static const char *const bad_sl[] = {
     "HTTP/1.1 10",
     "HTTP",
     "H\0TP/1.1 100 OK",
+
+    /* Previously allowed, now disallowed. */
+    "HTTP/1.1   200   OK",
+    "HTTP/1.1   200 \t  OK",
+    "   HTTP/1.1 200 OK",
+    "Norman is a dog HTTP/1.1 200 OK",
+    "HTTP/1.1000 100 OK",
+    "HTTP/1000.1000 100 OK",
+    "HTTP/00001.1 100 OK",
+    "HTTP/1.00001 100 OK",
+    "HTTP/99.99 100 OK",
+    "HTTP/1.1 600 OK",
+
     NULL
 };  
 
@@ -86,18 +93,19 @@ static int status_lines(void)
 
     for (n = 0; accept_sl[n].status != NULL; n++) {
 	ONV(ne_parse_statusline(accept_sl[n].status, &s),
-	    ("valid #%d: parse", n));
+	    ("valid #%d: parsing '%s' failed", n, accept_sl[n].status));
 	ONV(accept_sl[n].major != s.major_version, ("valid #%d: major", n));
 	ONV(accept_sl[n].minor != s.minor_version, ("valid #%d: minor", n));
-	ONV(accept_sl[n].code != s.code, ("valid #%d: code", n));
+	ONV(accept_sl[n].code != s.code, ("valid #%d: code %d not %d", n, s.code, accept_sl[n].code));
 	ONV(strcmp(accept_sl[n].rp, s.reason_phrase), 
-	    ("valid #%d: reason phrase", n));
+	    ("valid #%d: reason phrase [%s] not [%s]", n, s.reason_phrase, accept_sl[n].rp));
         ne_free(s.reason_phrase);
+        memset(&s, 0, sizeof s);
     }
 
     for (n = 0; bad_sl[n] != NULL; n++) {
 	ONV(ne_parse_statusline(bad_sl[n], &s) == 0, 
-	    ("invalid #%d", n));
+	    ("invalid #%d parsed OK - [%s]", n, bad_sl[n]));
     }
 
     return OK;