Skip the WebSocket upgrade when a before filter already answered

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`.

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

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