From 346a587c4049d455f898d69b501386a539e09213 Mon Sep 17 00:00:00 2001
From: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Date: Mon, 13 Apr 2020 20:49:29 -0700
Subject: [PATCH] parser: commit after parsing each header line (#54)

Upstream issue https://github.com/inhabitedtype/httpaf/issues/18 details
a case where the http/af parser is unable to parse a message if the read
buffer size isn't as big as all the bytes that form the message headers.
While it is desirable, and a design goal of the library, to be efficient
in parsing and thus avoid DoS attacks, it is more sensible to allow each
message header to fit within the read buffer size up to its limit. This
diff makes that change, which better accommodates real world use cases
for the default read buffer size.
---
 lib/parse.ml                       |  1 +
 lib_test/test_client_connection.ml | 47 +++++++++++++++++++++++++++++-
 lib_test/test_server_connection.ml | 24 +++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/lib/parse.ml b/lib/parse.ml
index 2f870a3..9f7f888 100644
--- a/lib/parse.ml
+++ b/lib/parse.ml
@@ -103,6 +103,7 @@ let header =
   lift2 (fun key value -> (key, value))
     (take_till P.is_space_or_colon <* char ':' <* spaces)
     (take_till P.is_cr <* eol >>| String.trim)
+  <* commit
   <?> "header"
 
 let headers =
diff --git a/lib_test/test_client_connection.ml b/lib_test/test_client_connection.ml
index 1c8aaaa..e793181 100644
--- a/lib_test/test_client_connection.ml
+++ b/lib_test/test_client_connection.ml
@@ -75,6 +75,50 @@ let default_response_handler expected_response response body =
 
 let no_error_handler _ = assert false
 
+let test_commit_parse_after_every_header () =
+  let request' = Request.create `GET "/" in
+  let response =
+    Response.create
+      ~headers:(Headers.of_list
+        [ "Links", "/path/to/some/website"
+        ; "Links", "/path/to/some/website"
+        ; "connection", "close"
+        ])
+      `OK
+  in
+
+  let t = create ?config:None in
+  let body =
+    request
+      t
+      request'
+      ~response_handler:(default_response_handler response)
+      ~error_handler:no_error_handler
+  in
+  Body.close_writer body;
+  write_request t request';
+
+  let response_line = "HTTP/1.1 200 OK\r\n" in
+  let single_header = "Links: /path/to/some/website\r\n" in
+  let r =
+    (* Each header is 30 bytes *)
+    response_line ^ single_header ^ single_header ^ "connection: close\r\n\r\n"
+  in
+  let bs = Bigstringaf.of_string r ~off:0 ~len:(String.length r) in
+  let c = read t bs ~off:0 ~len:(String.length response_line + 15) in
+  Alcotest.(check int) "only reads the response line" (String.length response_line) c;
+
+  let c' =
+    read t bs ~off:c ~len:(String.length single_header)
+  in
+  Alcotest.(check int) "parser can read a single header and commit" (String.length single_header) c';
+
+  let c'' = read_eof t bs ~off:(c + c') ~len:(String.length r - (c + c')) in
+  Alcotest.(check int) "read_eof with the rest of the input is accepted" (String.length r - (c + c')) c'';
+
+  connection_is_shutdown t;
+;;
+
 let test_get () =
   let request' = Request.create `GET "/" in
   let response = Response.create `OK in
@@ -580,7 +624,8 @@ let test_client_upgrade () =
 ;;
 
 let tests =
-  [ "GET"         , `Quick, test_get
+  [ "commit parse after every header line", `Quick, test_commit_parse_after_every_header
+  ; "GET"         , `Quick, test_get
   ; "Response EOF", `Quick, test_response_eof
   ; "Response header order preserved", `Quick, test_response_header_order
   ; "report_exn"  , `Quick, test_report_exn
diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml
index a61ec3a..c186b8b 100644
--- a/lib_test/test_server_connection.ml
+++ b/lib_test/test_server_connection.ml
@@ -126,6 +126,29 @@ let test_reader_is_closed_after_eof () =
   connection_is_shutdown t;
 ;;
 
+let test_commit_parse_after_every_header () =
+  let t = create default_request_handler in
+  let request_line = "GET / HTTP/1.1\r\n" in
+  let single_header = "Links: /path/to/some/website\r\n" in
+  let r =
+    (* Each header is 30 bytes *)
+    request_line ^ single_header ^ single_header ^ "connection: close\r\n\r\n"
+  in
+  let bs = Bigstringaf.of_string r ~off:0 ~len:(String.length r) in
+  let c = read t bs ~off:0 ~len:30 in
+  Alcotest.(check int) "only reads the request line" (String.length request_line) c;
+
+  let c' =
+    read t bs ~off:c ~len:(String.length single_header)
+  in
+  Alcotest.(check int) "parser can read a single header and commit" (String.length single_header) c';
+
+  let c'' = read_eof t bs ~off:(c + c') ~len:(String.length r - (c + c')) in
+  Alcotest.(check int) "read_eof with the rest of the input is accepted" (String.length r - (c + c')) c'';
+  write_response t (Response.create `OK);
+  connection_is_shutdown t;
+;;
+
 let test_single_get () =
   (* Single GET *)
   let t = create default_request_handler in
@@ -748,6 +771,7 @@ let test_yield_before_starting_a_response () =
 let tests =
   [ "initial reader state"  , `Quick, test_initial_reader_state
   ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof
+  ; "commit parse after every header line", `Quick, test_commit_parse_after_every_header
   ; "single GET"            , `Quick, test_single_get
   ; "multiple GETs"         , `Quick, test_multiple_get
   ; "asynchronous response" , `Quick, test_asynchronous_response