Bandit.HTTPTransport.Bandit.HTTP1.Socket.request_error!/2

[Houdini]

Just received one from sentry,

Bandit.HTTPError
closed

Stacktrace:

lib/bandit/http1/socket.ex in Bandit.HTTPTransport.Bandit.HTTP1.Socket.request_error!/2 at line 420
lib/bandit/http1/socket.ex in Bandit.HTTPTransport.Bandit.HTTP1.Socket.send_data/3 at line 357
lib/bandit/adapter.ex in Bandit.Adapter.send_data/3 at line 242
lib/bandit/adapter.ex in Bandit.Adapter.send_resp/4 at line 140
lib/plug/conn.ex in Plug.Conn.send_resp/1 at line 444
lib/phoenix/router.ex in Phoenix.Router.__call__/5 at line 484
lib/ma_web/endpoint.ex in MaWeb.Endpoint.plug_builder_call/2 at line 1
lib/ma_web/endpoint.ex in MaWeb.Endpoint."call (overridable 3)"/2 at line 1

elixir: 1.17.2 (compiled with Erlang/OTP 27) bandit: 1.5.7

[mtrudel]

This is a wholly normal error; one that comes from the client closing the connection before the server has fully sent its response. Bandit will selectively log these as 'protocol errors', which is configurable within Bandit via the http_errors.log_protocol_errors configuration point. Sentry should be selectively logging these as well via its own mechanism, though it seems that there may be an upstream issue affecting this. See here for more context.

[goncalotomas]

Sorry for re-opening this but I wanted to give @mtrudel my perspective from seeing these errors somewhat often when using Bandit with Phoenix 1.6 and 1.7:

At the team I'm working on security is very important so we don't want to disable http_errors.log_protocol_errors, but that means we have to keep seeing and ignoring these Bandit.HTTPError: :closed messages which is unfortunate.

As an aside, these errors also pop up during local development with LiveView. I believe saving a file and triggering hot code reloading sometimes (but not always) causes this error to be reported, and that's another place I get to see this error and I wish I could filter it out.

What are your thoughts about adding an option separate to http_errors.log_protocol_errors that would allow filtering of selected protocol errors? Judging from the fact that this issue was reported multiple times, maybe this would validate the need for that flexibility to be added, despite not being aware of how hard that would be to implement in Bandit.

[mtrudel]

How about a log_client_closures parameter that defaults to false?

[goncalotomas]

That would be excellent! 😃

I had a very quick look to see if I was up for opening a PR for this and while it looks like we would add this new option here and here, I don't know exactly how you'd test it. Would you like me to give it a try and open a PR?

[mtrudel]

I'd love that! The pipeline case should be tested for both HTTP/1 and HTTP/2, which you can do in request_test.exs and protocol_test.exs respectively. There are logging test cases there already, and you should be able to trigger them by closing the client in the tests (I think there may be examples of this already). The HTTP/2 case in handle_connection is a bit of a special case; it happens in the fairly narrow case where an HTTP/2 connection errors out at the earliest stages of the connection (before it even starts reading headers). This one may no be testable, since it's fundamentally super racy.