Unauthorized WebSocket connection in Rails app causes Falcon::Adapters::Rack::FullHijack error

[deepj]

My Rails application uses ActionCable. When I access to unauthorized page (typically sign in's page), /cable is called to try to establish WebSocket connection. Unfortunately Falcon from some reason puts the below error to log when WebSocket is rejected and closed because an user is not authenticated. Puma does not cause this kind of error.

Started GET "/cable" for 127.0.0.1 at 2021-05-13 18:23:14 +0200
Started GET "/cable/" [WebSocket] for 127.0.0.1 at 2021-05-13 18:23:14 +0200
Successfully upgraded to WebSocket (REQUEST_METHOD: GET, HTTP_CONNECTION: keep-alive, Upgrade, HTTP_UPGRADE: websocket)
 49.0s    error: Falcon::Adapters::Rack [oid=0x8264] [ec=0x84a8] [pid=6782] [2021-05-13 18:23:14 +0200]
               |   Falcon::Adapters::Rack::FullHijack: The connection was hijacked.
               |   → ~/.gem/ruby/3.0.1/gems/falcon-0.38.1/lib/falcon/adapters/rack.rb:206 in `call'
               |     ~/.gem/ruby/3.0.1/gems/protocol-http-0.22.0/lib/protocol/http/middleware.rb:50 in `call'
               |     ~/.gem/ruby/3.0.1/gems/falcon-0.38.1/lib/falcon/adapters/rewindable.rb:70 in `call'
               |     ~/.gem/ruby/3.0.1/gems/protocol-http-0.22.0/lib/protocol/http/middleware.rb:50 in `call'
               |     ~/.gem/ruby/3.0.1/gems/async-http-0.56.2/lib/async/http/server.rb:64 in `block in accept'
               |     ~/.gem/ruby/3.0.1/gems/async-http-0.56.2/lib/async/http/protocol/http1/server.rb:62 in `each'
               |     ~/.gem/ruby/3.0.1/gems/async-http-0.56.2/lib/async/http/server.rb:53 in `accept'
               |     ~/.gem/ruby/3.0.1/gems/async-io-1.31.0/lib/async/io/server.rb:32 in `block in accept_each'
               |     ~/.gem/ruby/3.0.1/gems/async-io-1.31.0/lib/async/io/socket.rb:73 in `block in accept'
               |     ~/.gem/ruby/3.0.1/gems/async-1.29.0/lib/async/task.rb:263 in `block in make_fiber'
An unauthorized connection attempt was rejected
Finished "/cable/" [WebSocket] for 127.0.0.1 at 2021-05-13 18:23:14 +0200

WebSocket connection is authorized and set from Rails side using the following code. It's similar code which is described in Rails Guide.

# frozen_string_literal: true

module ApplicationCable
  class Connection < ActionCable::Connection::Base
    identified_by :current_user

    def connect
      self.current_user = find_verified_user
    end

    protected

    # Use current_user set by Warden (Devise)
    def find_verified_user
      if (verified_user = env['warden'].user)
        verified_user
      else
        reject_unauthorized_connection
      end
    end
  end
end

It calls reject_unauthorized_connection when no user is authenticated.

I would expect Falcon does not have a problem with non-authorized WebSocket connection in Rails application.

[ioquatix]

Is something actually broken or is it just noisy error messages?

(Warning, possibly orthogonal opinion ahead.)

The full hijack error was a design choice because full hijack needs to bypass the server's normal operational behaviour. The IO is hijacked and the exception is raised to kill the request/response as quickly as possible. I thought about catching the exception at the bottom of the server stack, but in the end, I dislike full hijack enough that I'm okay with it being noisy, since I kind of consider it broken by design. That is, full hijack is (1) very hard to implement correctly, (2) very hard to use correctly and (3) unable to work with HTTP/2 in general.

So, I'm kind of in a hard place because obviously I want to support your use case, but it's also a very "exceptional" flow control. I don't see a lot of value in making the core Async::HTTP abstraction significantly more complex just to support full hijack. I'd even like to remove support for partial hijack as currently implemented, although it's significantly better.

Another aspect of it is what kind of future designs do I want to encourage. People are working on async compatible "ActionCable" implementations: https://github.com/piecehealth/async_cable and https://github.com/senid231/async_cable are two that I know of. If I adopt full hijack, I'm kind of admitting that the current status quo is acceptable. But that's not how I feel. I'd rather encourage people to create something better than what we have. So, I don't want to fully commit what I consider an important public interface (Async::HTTP) to what I consider a kind of legacy interface or hack.

Practically speaking, we could solve this problem by introducing a base class for "Ignore this request/response" but that in and of itself feels like accepting the complexity of full hijack all the way to the core of Async::HTTP. I'd rather rethink what streaming looks like and build a better abstraction for it. The more complexity we support in Async::HTTP, the harder it gets to actually make changes and improvements.

That exception, is me protesting against the general full hijack model.

[deepj]

@ioquatix I apologize for replying so late. I totally forgot on this. Thank you for long explanation of the whole issue around hijacking.

Yes, it's noising a lot. From my perspective, I would wish to silence the error in the scenario at least. It's confusing, and flooding the log.

[ioquatix]

I understand, I'll think about a way we can make this work - probably at the logger level.

[ioquatix]

Do you mind checking again, I believe this has been fixed / the log should no longer be emitted.