Client is always accepted for undeclared namespaces

[llunn]

Describe the bug
This issue is manifesting itself through flask-socketio, but I believe the source of the issue belongs here. When a client connects using a namespace that is not declared, the namespace is simply added and the client connects with no ability to execute code against the connection in order to reject it.

The end result is that it is possible to pollute the connection pool with phantom clients, and the server cannot detect when new clients connect.

The source of issue I believe is here:

python-socketio/src/socketio/base_manager.py

Line 119 in 3bd1357

if namespace not in self.rooms:

To Reproduce

1. Stand up a bare bones server instance.
2. Define an on_connect handler for '/'
3. Define zero or more class based namespaces and register them
4. Connect a client using a namespace that the server is not aware of.
5. Client connection is blindly accepted, regardless of always_connect value.

Expected behavior
One of:

1. Connections to namespaces that do not exist are flat out rejected; or
2. Connections to namespaces that do not exist produce an exception; or
3. Connect event is delegated to to / namespace connect, with relevant details on the namespace attempted to connect to.

In all cases, it is also expected that the / namespace connect event fires, which is also not occurring.

Logs

(58685) wsgi starting up on http://0.0.0.0:5000
(58685) accepted ('127.0.0.1', 50507)
1_szfyPlEneUJwA3AAAA: Sending packet OPEN data {'sid': '1_szfyPlEneUJwA3AAAA', 'upgrades': [], 'pingTimeout': 20000, 'pingInterval': 25000}
kombu backend initialized.
1_szfyPlEneUJwA3AAAA: Received request to upgrade to websocket
1_szfyPlEneUJwA3AAAA: Upgrade to websocket successful
1_szfyPlEneUJwA3AAAA: Received packet MESSAGE data 0/celery,{"authToken":"bGVlQHRvcnVzb2Z0LmNvbTo2MUE1MUI1NTrMnz4SGR56ssXsYgxiiGHGa5ck6g"}
/celery
1_szfyPlEneUJwA3AAAA: Sending packet MESSAGE data 0/celery,{"sid":"hOsZtTCikRL7RTIfAAAB"}
(58685) accepted ('192.168.0.60', 50513)
DTnpUXYe8iuRM0UdAAAC: Sending packet OPEN data {'sid': 'DTnpUXYe8iuRM0UdAAAC', 'upgrades': [], 'pingTimeout': 20000, 'pingInterval': 25000}
DTnpUXYe8iuRM0UdAAAC: Received request to upgrade to websocket
DTnpUXYe8iuRM0UdAAAC: Upgrade to websocket successful
DTnpUXYe8iuRM0UdAAAC: Received packet MESSAGE data 0/remote,{"authToken":"bGVlQHRvcnVzb2Z0LmNvbTo2MUE1MUI1NTrMnz4SGR56ssXsYgxiiGHGa5ck6g","userCtx":{"name":"lee@torusoft.com","roles":["confide-analytics","emmit-admin","confide-admin"]}}
/remote
DTnpUXYe8iuRM0UdAAAC: Sending packet MESSAGE data 0/remote,{"sid":"DhB9khVI587ck8pjAAAD"}
(58685) accepted ('192.168.0.60', 50558)

Additional context
If this is not intended behaviour, a possible easy solution is to add an else to to the if else if block of Server._trigger_event

If more detail is required, please let me know!

[miguelgrinberg]

Yes, I believe your analysis is correct. The server should not accept a connection for a namespace it does not know about.

In all cases, it is also expected that the / namespace connect event fires, which is also not occurring.

I don't understand what you mean here. The current version of the Socket.IO protocol does not treat the / namespace in a special way as it did in previous versions. Only the namespaces the client connects to are connected, / needs to be connected explicitly now.

[llunn]

@miguelgrinberg Thanks!

In all cases, it is also expected that the / namespace connect event fires, which is also not occurring.

I don't understand what you mean here. The current version of the Socket.IO protocol does not treat the / namespace in a special way as it did in previous versions. Only the namespaces the client connects to are connected, / needs to be connected explicitly now.

Full disclosure, I spent a good amount of time looking for other reasons why I was not getting connect events to fire. In one issue or documentation somewhere, lost to the ether, I'm certain I seen something to the effect that a client connecting to a namespace also results in a connect event being triggered on / (since the user also joins their own room off of /?)

There is a high probability my understanding is incorrect and I defer to your understanding of the protocol: I have not read myself into it.

With this said, if you want me to PR on this let me know, but I'm not clear on what the appropriate fix should be. Perhaps a ConnectionRefusedError originating from _handle_connect for namespace not being in self.namespace_handlers?

[miguelgrinberg]

I'm certain I seen something to the effect that a client connecting to a namespace also results in a connect event being triggered on /

This used to be true, so I'm sure there are references to this behavior in old issues, but it is not done anymore.

With this said, if you want me to PR on this let me know,

I'm not sure what behavior we want for this use case. Probably same as if the connect handler returns False or raises a connection refused, but I have to think about it, because it would be nice if the client can raise a BadNamespace error.

[ba1dr]

Actually this fix was the thing that broke the functionality of my service yesterday after upgrading Socketio to v5.7.0.
I have a Django web service with Socketio/ASGI as a separate process. Clients in the browser can send a request to establish a connection to the namespace which is unknown to the server, but I have other services that can only emit messages to the RedisManager. Hence the clients have connection established and can receive messages from other processes.

After this fix I have to keep the list of all possible namespaces on the server to let them connect. Or monkeypatch the connection handler to allow all connections.

Can we have an option to allow non-declared namespaces to connect?

[miguelgrinberg]

@ba1dr this was really an oversight, it does not really make much sense to accept unknown namespaces, the best default is to reject them. To change this default, all you need to do is add a connect handler for these namespaces. The handle doesn't need to do anything, just by existing the default will change to allow those connections.

[ba1dr]

@miguelgrinberg I understand why this has been fixed. Indeed, to prevent the server of keeping pool of potentially wrong/useless connections better to reject them in most cases. But in my case this is an internal server and all clients are trusted.
The code for message senders is in other services (perhaps could be in another repository) and web server does not need to be updated each time when I add a new namespace to the side service.
I see that I can overload method _handle_connect and add a handler for unknown namespace, this will fix the problem for me.

[miguelgrinberg]

Okay, I added namespaces argument to the Server and AsyncServer classes. Set this to the list of namespaces you want the server to accept (in addition to any that have handlers defined). The default is ['/'], so connections on the default namespace are accepted. Set to '*' (as a string, not a list) to accept all namespaces, which is the pre-5.7 behavior. Let me know if this addresses your issues.

[ba1dr]

Yes, I think this will help. Thank you for your efforts!

[miguelgrinberg]

This is now released in v5.7.1.