http: server error handling improvements and tests

We want to consume the request properly on an error, so that
we can give a reasonable response.  We were prematurely closing
the connection for certain failure modes.  We still have to fix
overly long URIs and headers, but thats next!

3 files changed, 137 insertions, 41 deletions

diff --git a/src/supplemental/http/http_msg.c b/src/supplemental/http/http_msg.c
index 3c4ce491..b147c811 100644
--- a/src/supplemental/http/http_msg.c
+++ b/src/supplemental/http/http_msg.c
@@ -213,31 +213,39 @@ http_scan_line(void *vbuf, size_t n, size_t *lenp)
 static int
 http_req_parse_line(nng_http *conn, void *line)
 {
-	int   rv;
 	char *method;
 	char *uri;
 	char *version;
 
 	method = line;
 	if ((uri = strchr(method, ' ')) == NULL) {
-		return (NNG_EPROTO);
+		nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
+		return (0);
 	}
 	*uri = '\0';
 	uri++;
 
 	if ((version = strchr(uri, ' ')) == NULL) {
-		return (NNG_EPROTO);
+		nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
+		return (0);
 	}
 	*version = '\0';
 	version++;
 
-	nni_http_set_method(conn, method);
-	if (((rv = nni_url_canonify_uri(uri)) != 0) ||
-	    ((rv = nni_http_set_uri(conn, uri, NULL)) != 0) ||
-	    ((rv = nni_http_set_version(conn, version)) != 0)) {
-		return (rv);
+	if (nni_url_canonify_uri(uri) != 0) {
+		nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
+		return (0);
 	}
-	return (0);
+	if (nni_http_set_version(conn, version)) {
+		nni_http_set_status(
+		    conn, NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP, NULL);
+		return (0);
+	}
+
+	nni_http_set_method(conn, method);
+
+	// this one only can fail due to ENOMEM
+	return (nni_http_set_uri(conn, uri, NULL));
 }
 
 static int
@@ -303,12 +311,9 @@ nni_http_req_parse(nng_http *conn, void *buf, size_t n, size_t *lenp)
 
 		if (req->data.parsed) {
 			rv = http_parse_header(conn, line);
-		} else if ((rv = http_req_parse_line(conn, line)) == 0) {
+		} else {
 			req->data.parsed = true;
-		}
-
-		if (rv != 0) {
-			break;
+			rv               = http_req_parse_line(conn, line);
 		}
 	}
 
diff --git a/src/supplemental/http/http_server.c b/src/supplemental/http/http_server.c
index 096d985c..5d6a2aee 100644
--- a/src/supplemental/http/http_server.c
+++ b/src/supplemental/http/http_server.c
@@ -466,7 +466,11 @@ http_sconn_rxdone(void *arg)
 	// Validate the request -- it has to at least look like HTTP
 	// 1.x.  We flatly refuse to deal with HTTP 0.9, and we can't
 	// cope with HTTP/2.
-	if ((val = nni_http_get_version(sc->conn)) == NULL) {
+	if (nng_http_get_status(sc->conn) >= NNG_HTTP_STATUS_BAD_REQUEST) {
+		http_sconn_error(sc, nng_http_get_status(sc->conn));
+		return;
+	}
+	if ((val = nng_http_get_version(sc->conn)) == NULL) {
 		sc->close = true;
 		http_sconn_error(sc, NNG_HTTP_STATUS_BAD_REQUEST);
 		return;
@@ -485,7 +489,7 @@ http_sconn_rxdone(void *arg)
 	}
 
 	// NB: The URI will already have been canonified by the REQ parser
-	uri = nni_http_get_uri(sc->conn);
+	uri = nng_http_get_uri(sc->conn);
 	if (uri[0] != '/') {
 		// We do not support authority form or asterisk form at present
 		sc->close = true;
diff --git a/src/supplemental/http/http_server_test.c b/src/supplemental/http/http_server_test.c
index 77ec304f..940248fd 100644
--- a/src/supplemental/http/http_server_test.c
+++ b/src/supplemental/http/http_server_test.c
@@ -218,6 +218,49 @@ test_server_basic(void)
 	nng_aio_wait(st.aio);
 	NUTS_PASS(nng_aio_result(st.aio));
 
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_OK);
+
+	ptr = nng_http_get_header(st.conn, "Content-Length");
+	NUTS_TRUE(ptr != NULL);
+	NUTS_TRUE(atoi(ptr) == (int) strlen(doc1));
+
+	iov.iov_len = strlen(doc1);
+	iov.iov_buf = chunk;
+	NUTS_PASS(nng_aio_set_iov(st.aio, 1, &iov));
+	nng_http_read_all(st.conn, st.aio);
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+	NUTS_TRUE(nng_aio_count(st.aio) == strlen(doc1));
+	NUTS_TRUE(memcmp(chunk, doc1, strlen(doc1)) == 0);
+
+	server_free(&st);
+}
+
+static void
+test_server_canonify(void)
+{
+	struct server_test st;
+	char               chunk[256];
+	const void        *ptr;
+	nng_iov            iov;
+	nng_http_handler  *h;
+
+	NUTS_PASS(nng_http_handler_alloc_static(
+	    &h, "/home/index.html", doc1, strlen(doc1), "text/html"));
+
+	server_setup(&st, h);
+
+	NUTS_PASS(nng_http_set_uri(
+	    st.conn, "/someplace/..////home/./%69ndex.html", NULL));
+	nng_http_write_request(st.conn, st.aio);
+
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
+	nng_http_read_response(st.conn, st.aio);
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
 	NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_OK);
 
 	ptr = nng_http_get_header(st.conn, "Content-Length");
@@ -300,7 +343,60 @@ test_server_404(void)
 	nng_aio_wait(st.aio);
 	NUTS_PASS(nng_aio_result(st.aio));
 
-	NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_NOT_FOUND);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_NOT_FOUND);
+
+	server_free(&st);
+}
+
+static void
+test_server_no_authoritive_form(void)
+{
+	struct server_test st;
+	nng_http_handler  *h;
+
+	NUTS_PASS(nng_http_handler_alloc_static(
+	    &h, "/home.html", doc1, strlen(doc1), "text/html"));
+
+	server_setup(&st, h);
+
+	NUTS_PASS(
+	    nng_http_set_uri(st.conn, "http://127.0.0.1/home.html", NULL));
+	nng_http_write_request(st.conn, st.aio);
+
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
+	nng_http_read_response(st.conn, st.aio);
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
+
+	server_free(&st);
+}
+
+static void
+test_server_bad_canonify(void)
+{
+	struct server_test st;
+	nng_http_handler  *h;
+
+	NUTS_PASS(nng_http_handler_alloc_static(
+	    &h, "/home.html", doc1, strlen(doc1), "text/html"));
+
+	server_setup(&st, h);
+
+	NUTS_PASS(nng_http_set_uri(st.conn, "/%home.html", NULL));
+	nng_http_write_request(st.conn, st.aio);
+
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
+	nng_http_read_response(st.conn, st.aio);
+	nng_aio_wait(st.aio);
+	NUTS_PASS(nng_aio_result(st.aio));
+
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
 
 	server_free(&st);
 }
@@ -323,8 +419,7 @@ test_server_bad_version(void)
 	nng_aio_wait(st.aio);
 	NUTS_PASS(nng_aio_result(st.aio));
 
-	NUTS_TRUE(nng_http_get_status(st.conn) ==
-	    NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP);
 
 	server_free(&st);
 }
@@ -346,7 +441,7 @@ test_server_missing_host(void)
 	nng_aio_wait(st.aio);
 	NUTS_PASS(nng_aio_result(st.aio));
 
-	NUTS_TRUE(nng_http_get_status(st.conn) == 400);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
 
 	server_free(&st);
 }
@@ -388,10 +483,7 @@ test_server_wrong_method(void)
 	nng_aio_wait(st.aio);
 	NUTS_PASS(nng_aio_result(st.aio));
 
-	NUTS_TRUE(nng_http_get_status(st.conn) ==
-	    NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
-	NUTS_MSG("Got result %d: %s", nng_http_get_status(st.conn),
-	    nng_http_get_reason(st.conn));
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
 
 	server_free(&st);
 }
@@ -417,7 +509,7 @@ test_server_post_handler(void)
 	nng_http_set_body(st.conn, txdata, strlen(txdata));
 	nng_http_set_method(st.conn, "POST");
 	NUTS_PASS(httpdo(&st, (void **) &rxdata, &size));
-	NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_OK);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_OK);
 	NUTS_TRUE(size == strlen(txdata));
 	NUTS_TRUE(strncmp(txdata, rxdata, size) == 0);
 	nng_free(rxdata, size);
@@ -429,9 +521,7 @@ test_server_post_handler(void)
 	nng_http_set_body(st.conn, txdata, strlen(txdata));
 
 	NUTS_PASS(httpdo(&st, &data, &size));
-	NUTS_TRUE(nng_http_get_status(st.conn) ==
-	    NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
-	NUTS_MSG("HTTP status was %u", nng_http_get_status(st.conn));
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
 	nng_free(data, size);
 
 	server_free(&st);
@@ -440,25 +530,22 @@ test_server_post_handler(void)
 static void
 test_server_get_redirect(void)
 {
-	char               fullurl[256];
 	const char        *dest;
 	void              *data;
 	size_t             size;
 	nng_http_handler  *h;
 	struct server_test st;
 
-	// We'll use a 303 to ensure codes carry thru
+	// We'll use a 303 (SEE OTHER) to ensure codes carry thru
 	NUTS_PASS(nng_http_handler_alloc_redirect(
-	    &h, "/here", 303, "http://127.0.0.1/there"));
+	    &h, "/here", NNG_HTTP_STATUS_SEE_OTHER, "http://127.0.0.1/there"));
 	server_setup(&st, h);
 
 	NUTS_PASS(nng_http_set_uri(st.conn, "/here", NULL));
 	nng_http_set_method(st.conn, "GET");
 
 	NUTS_PASS(httpdo(&st, &data, &size));
-	NUTS_TRUE(nng_http_get_status(st.conn) == 303);
-	NUTS_MSG("HTTP status got %d, expected %d (url %s)",
-	    nng_http_get_status(st.conn), 303, fullurl);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_SEE_OTHER);
 	NUTS_TRUE((dest = nng_http_get_header(st.conn, "Location")) != NULL);
 	NUTS_MATCH(dest, "http://127.0.0.1/there");
 	nng_free(data, size);
@@ -469,16 +556,15 @@ test_server_get_redirect(void)
 static void
 test_server_tree_redirect(void)
 {
-	char               fullurl[256];
 	const char        *dest;
 	void              *data;
 	size_t             size;
 	nng_http_handler  *h;
 	struct server_test st;
 
-	// We'll use a 303 to ensure codes carry thru
-	NUTS_PASS(nng_http_handler_alloc_redirect(
-	    &h, "/here", 303, "http://127.0.0.1/there"));
+	// We'll use a permanent redirect to ensure codes carry thru
+	NUTS_PASS(nng_http_handler_alloc_redirect(&h, "/here",
+	    NNG_HTTP_STATUS_PERMANENT_REDIRECT, "http://127.0.0.1/there"));
 	nng_http_handler_set_tree(h);
 	server_setup(&st, h);
 
@@ -486,9 +572,7 @@ test_server_tree_redirect(void)
 	nng_http_set_method(st.conn, "GET");
 
 	NUTS_PASS(httpdo(&st, &data, &size));
-	NUTS_TRUE(nng_http_get_status(st.conn) == 303);
-	NUTS_MSG("HTTP status got %d, expected %d (url %s)",
-	    nng_http_get_status(st.conn), 303, fullurl);
+	NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_PERMANENT_REDIRECT);
 	NUTS_TRUE((dest = nng_http_get_header(st.conn, "Location")) != NULL);
 	NUTS_MATCH(dest, "http://127.0.0.1/there/i/go/again");
 	nng_free(data, size);
@@ -933,8 +1017,11 @@ test_serve_subdir_index(void)
 
 NUTS_TESTS = {
 	{ "server basic", test_server_basic },
+	{ "server canonify", test_server_canonify },
 	{ "server head", test_server_head },
 	{ "server 404", test_server_404 },
+	{ "server authoritiative form", test_server_no_authoritive_form },
+	{ "server bad canonify", test_server_bad_canonify },
 	{ "server bad version", test_server_bad_version },
 	{ "server missing host", test_server_missing_host },
 	{ "server wrong method", test_server_wrong_method },