Skip to content

Commit

Permalink
http: server error handling improvements and tests
Browse files Browse the repository at this point in the history 
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!
  • Loading branch information
@gdamore
gdamore committed Jan 12, 2025
1 parent 169166a commit 6cd9d6c
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 41 deletions.
33 changes: 19 additions & 14 deletions src/supplemental/http/http_msg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Check warning on line 223 in src/supplemental/http/http_msg.c

View check run for this annotation

Codecov / codecov/patch

src/supplemental/http/http_msg.c#L222-L223 

Added lines #L222 - L223 were not covered by tests
}
*uri = '\0';
uri++;

if ((version = strchr(uri, ' ')) == NULL) {
return (NNG_EPROTO);
nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
return (0);

Check warning on line 230 in src/supplemental/http/http_msg.c

View check run for this annotation

Codecov / codecov/patch

src/supplemental/http/http_msg.c#L229-L230 

Added lines #L229 - L230 were not covered by tests
}
*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);

Check warning on line 242 in src/supplemental/http/http_msg.c

View check run for this annotation

Codecov / codecov/patch

src/supplemental/http/http_msg.c#L240-L242 

Added lines #L240 - L242 were not covered by tests
}

nni_http_set_method(conn, method);

// this one only can fail due to ENOMEM
return (nni_http_set_uri(conn, uri, NULL));
}

static int
Expand Down Expand Up @@ -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);
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/supplemental/http/http_server.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
136 changes: 112 additions & 24 deletions src/supplemental/http/http_server_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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;

server_setup(&st, NULL);

NUTS_PASS(nng_http_handler_alloc_static(
&h, "/home.html", doc1, strlen(doc1), "text/html"));

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;

server_setup(&st, NULL);

NUTS_PASS(nng_http_handler_alloc_static(
&h, "/home.html", doc1, strlen(doc1), "text/html"));

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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -476,19 +563,17 @@ test_server_tree_redirect(void)
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);

NUTS_PASS(nng_http_set_uri(st.conn, "/here/i/go/again", 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_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);
Expand Down Expand Up @@ -933,8 +1018,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 },
Expand Down
10 changes: 9 additions & 1 deletion src/testing/nuts.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ extern const char *nuts_ecdsa_client_crt;
nng_strerror(result_), result_); \
} while (0)

// NUTS_ERROR tests for a specific NNG error code.
// NUTS_FAIL tests for a specific NNG error code.
#define NUTS_FAIL(cond, expect) \
do { \
int result_ = (cond); \
Expand Down Expand Up @@ -325,4 +325,12 @@ extern const char *nuts_ecdsa_client_crt;
};
#endif

#define NUTS_HTTP_STATUS(conn, expect) \
do { \
TEST_CHECK(nng_http_get_status(conn) == (expect)); \
TEST_MSG("HTTP status: expected %d (%s) got %d (%s)", expect, \
#expect, nng_http_get_status(conn), \
nng_http_get_reason(conn)); \
} while (0)

#endif // NNG_TEST_NUTS_H

0 comments on commit 6cd9d6c

Please sign in to comment.