fix: middleware to warn if response headers are invalid

[dangdennis]

Resolves #233. I was unable to replicate the socket hanging issue on alpine because I couldn't build it due to ssl dep issues. However, I ran the debian build on fly.io, and fly rejects http messages with invalid headers. So we should fix this regardless of linux distro. We can fix the alpine ssl issue separately.

• Learned that dropping empty headers via the builtin middleware will not work because it runs before the user's middleware, and definitely before the helper methods are called
• Spiral in dune usage lol
• Clean headers at lowest root response call

[aantron]

Thanks!

• Learned that dropping empty headers via the builtin middleware will not work because it runs before the user's middleware, and definitely before the helper methods are called

Not yet a full review, but AFAIK built-in middleware runs both before and after the user's middleware:

let my_middleware user's_middleware = fun request ->
  do_stuff_before;
  let%lwt response = user's_middleware request in
  do_stuff_after;
  response

[dangdennis]

Thanks!

• Learned that dropping empty headers via the builtin middleware will not work because it runs before the user's middleware, and definitely before the helper methods are called

Not yet a full review, but AFAIK built-in middleware runs both before and after the user's middleware:

let my_middleware user's_middleware = fun request ->
  do_stuff_before;
  let%lwt response = user's_middleware request in
  do_stuff_after;
  response

Ah thanks for clarifying. I'll re-implement this as a built-in middleware then to log a warning if there are empty header keys.

[dangdennis]

😆

AFAIK built-in middleware runs

This cracks me up because I'm assuming you've written the middleware logic yourself.

[aantron]

This cracks me up because I'm assuming you've written the middleware logic yourself.

Need to be careful in case I forgot or something has been broken since :)

[dangdennis]

unfortunately can't use expect-test against log outputs because we can't control the time when the logger executes. not that i can see any option. since i'm not performing any mutations anyways, think we're safe to not have a test.

done!

[dangdennis]

autoformatter kicked in for the file too.

[aantron]

I can merge this now to avoid extending this PR unnecessarily. However, as I recall, I wanted to move Dream away from the built-in middleware model. That's why there was only one left, and the plan was to find a way to get rid of it as a built-in middleware as well. I think this warning can go unconditionally into the HTTP adapters -- hopefully in a place where it will be triggered for both HTTP1 and HTTP2. This area:

dream/src/http/http.ml

Lines 407 to 448 in a2961cd

	let user's_dream_handler =
	if builtins then
	built_in_middleware error_handler user's_dream_handler
	else
	user's_dream_handler
	in
	(* Create the wrapped httpaf or h2 handler from the user's Dream handler. *)
	let httpaf_connection_handler =
	tls_library.create_handler
	~certificate_file
	~key_file
	~handler:user's_dream_handler
	~error_handler
	in
	(* TODO Should probably move out to the TLS library options. *)
	let tls_error_handler = Error_handler.tls error_handler in
	(* Some parts of the various HTTP servers that are under heavy development
	( *cough* Gluten SSL/TLS at the moment) leak exceptions out of the
	top-level handler instead of passing them to the error handler that we
	specified above with ~error_handler. So, to work around that, we pass the
	errors manually. Since we don't even have request or response objects at
	this point, we simply ignore the Dream.response that the error handler
	generates. We call it for any logging that it may do, and to swallow the
	error. Otherwise, it will go to !Lwt.async_exception_hook. *)
	(* TODO SSL alerts follow this pathway into the log at ERROR level, which is
	questionable - I understand that means clients can cause ERROR level log
	messages to be written into the log at will. To work around this, the
	exception should be formatted and passed as `Bad_request, or there should
	be pattern matching on the exception (but that might introduce dependency
	coupling), or the upstream should be patched to distinguish the errors in
	some useful way. *)
	let httpaf_connection_handler client_address socket =
	Lwt.catch
	(fun () ->
	httpaf_connection_handler client_address socket)
	(fun exn ->
	tls_error_handler client_address exn;
	Lwt.return_unit)
	in

already has several wrappers around the user's handler, so we just need to add the middleware here unconditionally. It should run outside of the catch middleware, since catch might also somehow set an empty header.

[dangdennis]

I'd prefer do this as a follow-up too - re: move the header check to h1/h2 paths. I thought it was a little strange to have important checks be opt-in, so I'm glad to see this direction change. The gold standard imo is asp.net & phoenix. Baking in security and best-practices seems best.

[dangdennis]

leaving these dune files here for the future instead of having to set them up again.

[aantron]

I think this should be the outer middleware, as the error handler can also be a source of empty headers, while, conversely, check_headers_middleware cannot raise any exceptions.

[aantron]

I'd prefer do this as a follow-up too - re: move the header check to h1/h2 paths. I thought it was a little strange to have important checks be opt-in, so I'm glad to see this direction change. The gold standard imo is asp.net & phoenix. Baking in security and best-practices seems best.

Not sure what you mean here -- built-in middlewares are/were opt-out, they are included in the middleware stack by default.

[dangdennis]

I'd prefer do this as a follow-up too - re: move the header check to h1/h2 paths. I thought it was a little strange to have important checks be opt-in, so I'm glad to see this direction change. The gold standard imo is asp.net & phoenix. Baking in security and best-practices seems best.

Not sure what you mean here -- built-in middlewares are/were opt-out, they are included in the middleware stack by default.

Oops! Yes you're right. It's already defaulted to true. I forgot to check the actual default value 😅 . I only ever read the lines below, and not specifically the args set on run.

  let user's_dream_handler =
    if builtins then
      built_in_middleware error_handler user's_dream_handler
    else
      user's_dream_handler
  in

[aantron]

I've made a few edits. Notably:

diff --git a/src/http/http.ml b/src/http/http.ml
index 11943be..d1b7614 100644
--- a/src/http/http.ml
+++ b/src/http/http.ml
@@ -390,7 +390,7 @@ let check_headers_middleware next_handler request =
   in
   if invalid_headers_exist then
     log.warning (fun log ->
-      log "A response header is empty or contains only whitespace");
+      log ~request "A response header is empty or contains only whitespace");
   Lwt.return response

 let built_in_middleware error_handler =

changes the log output from

23.04.23 14:13:42.184      dream.http  WARN A response header is empty or contains only whitespace

to

23.04.23 14:13:42.184      dream.http  WARN REQ 1 A response header is empty or contains only whitespace

I then tested example 9-error to see if this PR would warn about empty headers set in the error handler:

diff --git a/example/9-error/error.eml.ml b/example/9-error/error.eml.ml
index 515091c..8a995d7 100644
--- a/example/9-error/error.eml.ml
+++ b/example/9-error/error.eml.ml
@@ -12,6 +12,7 @@ let my_error_template _error debug_info suggested_response =
     </body>
     </html>
   end;
+  Dream.set_header suggested_response "" "abc";
   Lwt.return suggested_response

 let () =

The PR didn't warn about this header, as expected, so I added

diff --git a/src/http/http.ml b/src/http/http.ml
index d1b7614..2deb1db 100644
--- a/src/http/http.ml
+++ b/src/http/http.ml
@@ -395,8 +395,8 @@ let check_headers_middleware next_handler request =

 let built_in_middleware error_handler =
   Message.pipeline [
-    Catch.catch (Error_handler.app error_handler);
     check_headers_middleware;
+    Catch.catch (Error_handler.app error_handler);
   ]

It now warns about empty headers set in error handlers:

23.04.23 14:17:19.285       dream.log  INFO REQ 1 GET / 127.0.0.1:39224 Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/112.0.0.0 Safari/537.36
23.04.23 14:17:19.285       dream.log  WARN REQ 1 404 in 62 μs
23.04.23 14:17:19.285      dream.http  WARN REQ 1 A response header is empty or contains only whitespace

[aantron]

Thank you! As for built-in middleware, it's probably fine to leave the implementation in http.ml as it is now. I'll probably just hide them (their remainder, Dream.catch) from dream.mli. All the other former built-in middlewares have already been factored away anyway, and all that's left is a few that we want to keep around unconditionally, and it's fine to internally interpret them as a middleware stack.