Repository navigation
Quiet websocket protocol errors found after deserialization - #746
Merged
Merged
Conversation
Frames that deserialize correctly can still violate the protocol: a text frame that is not valid UTF-8, a continuation frame with no message in progress, a data frame in the middle of a fragmented message, a compressed frame when compression was not negotiated or that fails to inflate, or a fragmented message over the configured limit. These stopped the connection process with the bare error string as its reason, so each one produced a GenServer crash report holding the full connection state, WebSock state included, and the log_protocol_errors websocket option did not apply to them. Handle them the way frame deserialization errors are handled since mtrudel#610: send the close frame and terminate the websock as before, log a one-line summary governed by log_protocol_errors, and exit with a {:shutdown, reason} reason so no crash report is emitted. The close code, the reason given to terminate/2 and the websocket telemetry stop event are unchanged. Errors returned or raised by the websock itself keep their current behaviour.
Owner
|
Looks good! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A frame that deserializes correctly can still violate the protocol: a text frame that is not valid UTF-8, a continuation frame with no message in progress, a data frame in the middle of a fragmented message, a compressed frame when compression was not negotiated or that fails to inflate, or a fragmented message over the configured limit.
Bandit.WebSocket.Connectionreturned these fromhandle_frame/3as{:error, reason, connection}with the bare string as the reason, so the handler process stopped abnormally. Each one produced a GenServer crash report containing the whole handler state, the WebSock state included, andwebsocket_options: [log_protocol_errors: false]did not apply to them. #610 fixed this for errors found while deserializing a frame; these are the ones found afterwards.This change adds
do_protocol_error/4next todo_error/4inBandit.WebSocket.Connection. It callsdo_error/4unchanged, logs throughBandit.Logger.maybe_log_websocket_protocol_error/2, and returns{:error, {:shutdown, reason}, connection}. The ten call sites where the peer caused the error use it. Errors returned or raised by the WebSock callbacks, and the deflation error on an outgoing message, still go throughdo_error/4and behave as before, crash report included.The close code sent to the client, the reason passed to
terminate/2, and theerrorvalue in the[:bandit, :websocket, :stop]event are unchanged. The connection process exit reason changes from the string to{:shutdown, string}. Theerrorin[:thousand_island, :connection, :stop]is also unchanged by default; withsilent_terminate_on_error: trueit becomes{:shutdown, string}, as it already is for deserialization errors.Nine tests in the "protocol error logging" block of
test/bandit/websocket/protocol_test.exs: invalid UTF-8, unexpected continuation and unexpected compressed frame, each withlog_protocol_errorsset to:short,:verboseandfalse. Each asserts the close frame, theterminate/2reason, a{:shutdown, reason}exit with no crash report, and one log line or none. The existing tests that exercise these errors now also assert that no crash report is logged for them, which covers all ten call sites; fourteen existing tests fail without the change.Verified with
mix format --check-formatted,mix compile --warnings-as-errors,mix credo --strict,mix dialyzer, andmix teston Elixir 1.20.4 / OTP 29 (1083 passed, 1 skipped, 8 excluded; the 3 failures are inHibernationTestand also fail on main under OTP 29, where a hibernating process reports{:gen_server, :loop_hibernate, 4}).