diff --git a/CHANGELOG.md b/CHANGELOG.md index 1faf197a..ba72da5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ error 405 do |env| end ``` +- Skip the WebSocket upgrade when a `before` filter has already answered. A `halt` in a `before_all` — an authentication check answering `401` — closed the response, and `Kemal::WebSocketHandler` went on to attempt the upgrade regardless; the standard library handler raised `IO::Error: Closed stream` on the closed response. The client had its `401`, but every rejected handshake was logged as a server error. The handler now returns once it finds the response closed. - Stop confirming that a directory under `public/` exists when nothing is served for it. The standard library adds the trailing slash to a directory URL whenever its own `directory_listing` flag is on, and Kemal never set that flag from `serve_static`, so with `dir_listing` and `dir_index` both off `/admin` still answered `302 /admin/` while `/nope` answered `404` — the redirect was the only difference, and it told a scanner which directories are there. The redirect now happens only when a listing or an `index.html` would be served at the slashed URL; otherwise a directory falls through to the same `404` as a missing path. - Drop the bare `rescue` around url-param decoding. `URI.decode` passes a malformed percent-escape through unchanged rather than raising, so the rescue could only ever have hidden an unrelated bug as a silently undecoded value. The pass-through is now pinned by a spec. - Log an exception at `error` level before an `error MyException` handler renders it. Only the generic path and `error 500` logged; an exception matched by a class handler went unlogged, so a catch-all `error Exception do … end` — the usual way to get a JSON 500 page — took every crash out of the log, and any reporter reading `Log` never saw it. The handler still owns the response. Applications that use exception handlers for expected control flow will see an `error` line per occurrence; that was already the case for `error 500`. diff --git a/spec/websocket_handler_spec.cr b/spec/websocket_handler_spec.cr index d6c819a8..1711597c 100644 --- a/spec/websocket_handler_spec.cr +++ b/spec/websocket_handler_spec.cr @@ -1,4 +1,5 @@ require "./spec_helper" +require "log/spec" require "socket" # `Host` is part of a well formed HTTP/1.1 request and Crystal's @@ -434,4 +435,33 @@ describe "Kemal::WebSocketHandler" do raw.includes?("INTERNAL SECRET").should be_false end end + + it "does not attempt the upgrade when a before filter already answered" do + # `halt` in a `before_all` closes the response with the filter's status. The + # stdlib upgrade would then raise on the closed stream, which the client never + # saw but the log did, as an error per rejected handshake. + Kemal.config.websocket_allowed_origins = ["*"] + error(404) { "not found" } + ws("/chat", &.send("hi")) + + filter_handler = Kemal::FilterHandler.new + filter_handler._add_route_filter("ALL", "*", :before) do |env| + halt env, status_code: 401, response: "auth required" + end + Kemal.config.add_filter_handler(filter_handler) + + io = IO::Memory.new + response = HTTP::Server::Response.new(io) + context = HTTP::Server::Context.new(HTTP::Request.new("GET", "/chat", ws_upgrade_headers_for_origin), response) + + Log.capture do |logs| + build_main_handler.call(context) + logs.empty + end + + io.rewind + client_response = HTTP::Client::Response.from_io(io, decompress: false) + client_response.status_code.should eq(401) + client_response.body.should eq("auth required") + end end diff --git a/src/kemal/websocket_handler.cr b/src/kemal/websocket_handler.cr index c0b6e5a1..883579ba 100644 --- a/src/kemal/websocket_handler.cr +++ b/src/kemal/websocket_handler.cr @@ -11,6 +11,10 @@ module Kemal def call(context : HTTP::Server::Context) return call_next(context) unless context.ws_route_found? && websocket_upgrade_request?(context) + # A `before_all` filter that already answered - a `halt` with 401 - leaves + # nothing to upgrade. The stdlib handler would try anyway, hit the closed + # response and raise; the client had its answer, the log got a spurious error. + return if context.response.closed? unless context.request.method == "GET" reject_websocket_method_not_allowed!(context) return