Skip to content

Commit 31b5bb1

Browse files
gdamoregdamore
committed
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!
1 parent 169166a commit 31b5bb1

File tree

4 files changed

+146
-42
lines changed

4 files changed

+146
-42
lines changed

‎src/supplemental/http/http_msg.c

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -213,31 +213,39 @@ http_scan_line(void *vbuf, size_t n, size_t *lenp)
213213
static int
214214
http_req_parse_line(nng_http *conn, void *line)
215215
{
216-
int rv;
217216
char *method;
218217
char *uri;
219218
char *version;
220219

221220
method = line;
222221
if ((uri = strchr(method, ' ')) == NULL) {
223-
return (NNG_EPROTO);
222+
nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
223+
return (0);
224224
}
225225
*uri = '\0';
226226
uri++;
227227

228228
if ((version = strchr(uri, ' ')) == NULL) {
229-
return (NNG_EPROTO);
229+
nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
230+
return (0);
230231
}
231232
*version = '\0';
232233
version++;
233234

234-
nni_http_set_method(conn, method);
235-
if (((rv = nni_url_canonify_uri(uri)) != 0) ||
236-
((rv = nni_http_set_uri(conn, uri, NULL)) != 0) ||
237-
((rv = nni_http_set_version(conn, version)) != 0)) {
238-
return (rv);
235+
if (nni_url_canonify_uri(uri) != 0) {
236+
nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL);
237+
return (0);
239238
}
240-
return (0);
239+
if (nni_http_set_version(conn, version)) {
240+
nni_http_set_status(
241+
conn, NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP, NULL);
242+
return (0);
243+
}
244+
245+
nni_http_set_method(conn, method);
246+
247+
// this one only can fail due to ENOMEM
248+
return (nni_http_set_uri(conn, uri, NULL));
241249
}
242250

243251
static int
@@ -303,12 +311,9 @@ nni_http_req_parse(nng_http *conn, void *buf, size_t n, size_t *lenp)
303311

304312
if (req->data.parsed) {
305313
rv = http_parse_header(conn, line);
306-
} else if ((rv = http_req_parse_line(conn, line)) == 0) {
314+
} else {
307315
req->data.parsed = true;
308-
}
309-
310-
if (rv != 0) {
311-
break;
316+
rv = http_req_parse_line(conn, line);
312317
}
313318
}
314319

‎src/supplemental/http/http_server.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,11 @@ http_sconn_rxdone(void *arg)
466466
// Validate the request -- it has to at least look like HTTP
467467
// 1.x. We flatly refuse to deal with HTTP 0.9, and we can't
468468
// cope with HTTP/2.
469-
if ((val = nni_http_get_version(sc->conn)) == NULL) {
469+
if (nng_http_get_status(sc->conn) >= NNG_HTTP_STATUS_BAD_REQUEST) {
470+
http_sconn_error(sc, nng_http_get_status(sc->conn));
471+
return;
472+
}
473+
if ((val = nng_http_get_version(sc->conn)) == NULL) {
470474
sc->close = true;
471475
http_sconn_error(sc, NNG_HTTP_STATUS_BAD_REQUEST);
472476
return;
@@ -485,7 +489,7 @@ http_sconn_rxdone(void *arg)
485489
}
486490

487491
// NB: The URI will already have been canonified by the REQ parser
488-
uri = nni_http_get_uri(sc->conn);
492+
uri = nng_http_get_uri(sc->conn);
489493
if (uri[0] != '/') {
490494
// We do not support authority form or asterisk form at present
491495
sc->close = true;

‎src/supplemental/http/http_server_test.c

Lines changed: 112 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,49 @@ test_server_basic(void)
218218
nng_aio_wait(st.aio);
219219
NUTS_PASS(nng_aio_result(st.aio));
220220

221+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_OK);
222+
223+
ptr = nng_http_get_header(st.conn, "Content-Length");
224+
NUTS_TRUE(ptr != NULL);
225+
NUTS_TRUE(atoi(ptr) == (int) strlen(doc1));
226+
227+
iov.iov_len = strlen(doc1);
228+
iov.iov_buf = chunk;
229+
NUTS_PASS(nng_aio_set_iov(st.aio, 1, &iov));
230+
nng_http_read_all(st.conn, st.aio);
231+
nng_aio_wait(st.aio);
232+
NUTS_PASS(nng_aio_result(st.aio));
233+
NUTS_TRUE(nng_aio_count(st.aio) == strlen(doc1));
234+
NUTS_TRUE(memcmp(chunk, doc1, strlen(doc1)) == 0);
235+
236+
server_free(&st);
237+
}
238+
239+
static void
240+
test_server_canonify(void)
241+
{
242+
struct server_test st;
243+
char chunk[256];
244+
const void *ptr;
245+
nng_iov iov;
246+
nng_http_handler *h;
247+
248+
NUTS_PASS(nng_http_handler_alloc_static(
249+
&h, "/home/index.html", doc1, strlen(doc1), "text/html"));
250+
251+
server_setup(&st, h);
252+
253+
NUTS_PASS(nng_http_set_uri(
254+
st.conn, "/someplace/..////home/./%69ndex.html", NULL));
255+
nng_http_write_request(st.conn, st.aio);
256+
257+
nng_aio_wait(st.aio);
258+
NUTS_PASS(nng_aio_result(st.aio));
259+
260+
nng_http_read_response(st.conn, st.aio);
261+
nng_aio_wait(st.aio);
262+
NUTS_PASS(nng_aio_result(st.aio));
263+
221264
NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_OK);
222265

223266
ptr = nng_http_get_header(st.conn, "Content-Length");
@@ -300,7 +343,60 @@ test_server_404(void)
300343
nng_aio_wait(st.aio);
301344
NUTS_PASS(nng_aio_result(st.aio));
302345

303-
NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_NOT_FOUND);
346+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_NOT_FOUND);
347+
348+
server_free(&st);
349+
}
350+
351+
static void
352+
test_server_no_authoritive_form(void)
353+
{
354+
struct server_test st;
355+
nng_http_handler *h;
356+
357+
NUTS_PASS(nng_http_handler_alloc_static(
358+
&h, "/home.html", doc1, strlen(doc1), "text/html"));
359+
360+
server_setup(&st, h);
361+
362+
NUTS_PASS(
363+
nng_http_set_uri(st.conn, "http://127.0.0.1/home.html", NULL));
364+
nng_http_write_request(st.conn, st.aio);
365+
366+
nng_aio_wait(st.aio);
367+
NUTS_PASS(nng_aio_result(st.aio));
368+
369+
nng_http_read_response(st.conn, st.aio);
370+
nng_aio_wait(st.aio);
371+
NUTS_PASS(nng_aio_result(st.aio));
372+
373+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
374+
375+
server_free(&st);
376+
}
377+
378+
static void
379+
test_server_bad_canonify(void)
380+
{
381+
struct server_test st;
382+
nng_http_handler *h;
383+
384+
server_setup(&st, NULL);
385+
386+
NUTS_PASS(nng_http_handler_alloc_static(
387+
&h, "/home.html", doc1, strlen(doc1), "text/html"));
388+
389+
NUTS_PASS(nng_http_set_uri(st.conn, "/%home.html", NULL));
390+
nng_http_write_request(st.conn, st.aio);
391+
392+
nng_aio_wait(st.aio);
393+
NUTS_PASS(nng_aio_result(st.aio));
394+
395+
nng_http_read_response(st.conn, st.aio);
396+
nng_aio_wait(st.aio);
397+
NUTS_PASS(nng_aio_result(st.aio));
398+
399+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
304400

305401
server_free(&st);
306402
}
@@ -323,8 +419,7 @@ test_server_bad_version(void)
323419
nng_aio_wait(st.aio);
324420
NUTS_PASS(nng_aio_result(st.aio));
325421

326-
NUTS_TRUE(nng_http_get_status(st.conn) ==
327-
NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP);
422+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_HTTP_VERSION_NOT_SUPP);
328423

329424
server_free(&st);
330425
}
@@ -346,7 +441,7 @@ test_server_missing_host(void)
346441
nng_aio_wait(st.aio);
347442
NUTS_PASS(nng_aio_result(st.aio));
348443

349-
NUTS_TRUE(nng_http_get_status(st.conn) == 400);
444+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST);
350445

351446
server_free(&st);
352447
}
@@ -388,10 +483,7 @@ test_server_wrong_method(void)
388483
nng_aio_wait(st.aio);
389484
NUTS_PASS(nng_aio_result(st.aio));
390485

391-
NUTS_TRUE(nng_http_get_status(st.conn) ==
392-
NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
393-
NUTS_MSG("Got result %d: %s", nng_http_get_status(st.conn),
394-
nng_http_get_reason(st.conn));
486+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
395487

396488
server_free(&st);
397489
}
@@ -417,7 +509,7 @@ test_server_post_handler(void)
417509
nng_http_set_body(st.conn, txdata, strlen(txdata));
418510
nng_http_set_method(st.conn, "POST");
419511
NUTS_PASS(httpdo(&st, (void **) &rxdata, &size));
420-
NUTS_TRUE(nng_http_get_status(st.conn) == NNG_HTTP_STATUS_OK);
512+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_OK);
421513
NUTS_TRUE(size == strlen(txdata));
422514
NUTS_TRUE(strncmp(txdata, rxdata, size) == 0);
423515
nng_free(rxdata, size);
@@ -429,9 +521,7 @@ test_server_post_handler(void)
429521
nng_http_set_body(st.conn, txdata, strlen(txdata));
430522

431523
NUTS_PASS(httpdo(&st, &data, &size));
432-
NUTS_TRUE(nng_http_get_status(st.conn) ==
433-
NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
434-
NUTS_MSG("HTTP status was %u", nng_http_get_status(st.conn));
524+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_METHOD_NOT_ALLOWED);
435525
nng_free(data, size);
436526

437527
server_free(&st);
@@ -440,25 +530,22 @@ test_server_post_handler(void)
440530
static void
441531
test_server_get_redirect(void)
442532
{
443-
char fullurl[256];
444533
const char *dest;
445534
void *data;
446535
size_t size;
447536
nng_http_handler *h;
448537
struct server_test st;
449538

450-
// We'll use a 303 to ensure codes carry thru
539+
// We'll use a 303 (SEE OTHER) to ensure codes carry thru
451540
NUTS_PASS(nng_http_handler_alloc_redirect(
452-
&h, "/here", 303, "http://127.0.0.1/there"));
541+
&h, "/here", NNG_HTTP_STATUS_SEE_OTHER, "http://127.0.0.1/there"));
453542
server_setup(&st, h);
454543

455544
NUTS_PASS(nng_http_set_uri(st.conn, "/here", NULL));
456545
nng_http_set_method(st.conn, "GET");
457546

458547
NUTS_PASS(httpdo(&st, &data, &size));
459-
NUTS_TRUE(nng_http_get_status(st.conn) == 303);
460-
NUTS_MSG("HTTP status got %d, expected %d (url %s)",
461-
nng_http_get_status(st.conn), 303, fullurl);
548+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_SEE_OTHER);
462549
NUTS_TRUE((dest = nng_http_get_header(st.conn, "Location")) != NULL);
463550
NUTS_MATCH(dest, "http://127.0.0.1/there");
464551
nng_free(data, size);
@@ -469,26 +556,23 @@ test_server_get_redirect(void)
469556
static void
470557
test_server_tree_redirect(void)
471558
{
472-
char fullurl[256];
473559
const char *dest;
474560
void *data;
475561
size_t size;
476562
nng_http_handler *h;
477563
struct server_test st;
478564

479-
// We'll use a 303 to ensure codes carry thru
480-
NUTS_PASS(nng_http_handler_alloc_redirect(
481-
&h, "/here", 303, "http://127.0.0.1/there"));
565+
// We'll use a permanent redirect to ensure codes carry thru
566+
NUTS_PASS(nng_http_handler_alloc_redirect(&h, "/here",
567+
NNG_HTTP_STATUS_PERMANENT_REDIRECT, "http://127.0.0.1/there"));
482568
nng_http_handler_set_tree(h);
483569
server_setup(&st, h);
484570

485571
NUTS_PASS(nng_http_set_uri(st.conn, "/here/i/go/again", NULL));
486572
nng_http_set_method(st.conn, "GET");
487573

488574
NUTS_PASS(httpdo(&st, &data, &size));
489-
NUTS_TRUE(nng_http_get_status(st.conn) == 303);
490-
NUTS_MSG("HTTP status got %d, expected %d (url %s)",
491-
nng_http_get_status(st.conn), 303, fullurl);
575+
NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_PERMANENT_REDIRECT);
492576
NUTS_TRUE((dest = nng_http_get_header(st.conn, "Location")) != NULL);
493577
NUTS_MATCH(dest, "http://127.0.0.1/there/i/go/again");
494578
nng_free(data, size);
@@ -933,8 +1017,11 @@ test_serve_subdir_index(void)
9331017

9341018
NUTS_TESTS = {
9351019
{ "server basic", test_server_basic },
1020+
{ "server canonify", test_server_canonify },
9361021
{ "server head", test_server_head },
9371022
{ "server 404", test_server_404 },
1023+
{ "server authoritiative form", test_server_no_authoritive_form },
1024+
{ "server bad canonify", test_server_bad_canonify },
9381025
{ "server bad version", test_server_bad_version },
9391026
{ "server missing host", test_server_missing_host },
9401027
{ "server wrong method", test_server_wrong_method },

‎src/testing/nuts.h

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ extern const char *nuts_ecdsa_client_crt;
214214
nng_strerror(result_), result_); \
215215
} while (0)
216216

217-
// NUTS_ERROR tests for a specific NNG error code.
217+
// NUTS_FAIL tests for a specific NNG error code.
218218
#define NUTS_FAIL(cond, expect) \
219219
do { \
220220
int result_ = (cond); \
@@ -325,4 +325,12 @@ extern const char *nuts_ecdsa_client_crt;
325325
};
326326
#endif
327327

328+
#define NUTS_HTTP_STATUS(conn, expect) \
329+
do { \
330+
TEST_CHECK(nng_http_get_status(conn) == (expect)); \
331+
TEST_MSG("HTTP status: expected %d (%s) got %d (%s)", expect, \
332+
#expect, nng_http_get_status(conn), \
333+
nng_http_get_reason(conn)); \
334+
} while (0)
335+
328336
#endif // NNG_TEST_NUTS_H

0 commit comments

Comments
 (0)