Websocket returning 500 instead of 4XX when unable to resolve the path

[sprezz-arthur]

What is the expected behavior when attempting to access a path not available in websocket_urlpatterns?
Currently my application will cause a status code 500 and I would hope to have a 404 (or 403) instead.

HOW TO REPRODUCE:

• Recreate the tutorial chat app up to the second step: https://channels.readthedocs.io/en/latest/tutorial/part_2.html
• Modify the path path(r"ws/chat/<int:room_id>/", consumers.ChatConsumer.as_asgi())

Now connecting to a room using int will behave normally:

• Create a room named 42: WebSocket CONNECT /ws/chat/42/ [127.0.0.1:57662]

But when attempting to create a room with a non-int name, the behavior seems strange.

EXPECTED BEHAVIOR

• Create a room named "banana": WebSocket REJECT /ws/chat/banana/. (status 403)

CURRENT BEHAVIOR

• Create a room named "banana": ValueError: No route found for path 'ws/chat/banana/'. (status 500)

[sprezz-arthur]

Currently I'm getting away with this RouteValidator class.

But it seems rather brittle and I suppose there's a reason this is not the default behavior.

# mysite/asgi.py
import os

from channels.auth import AuthMiddlewareStack
from channels.routing import ProtocolTypeRouter, URLRouter
from channels.security.websocket import AllowedHostsOriginValidator, WebsocketDenier
from channels.middleware import BaseMiddleware
from django.core.asgi import get_asgi_application
from django.urls import Resolver404

os.environ.setdefault("DJANGO_SETTINGS_MODULE", "mysite.settings")
# Initialize Django ASGI application early to ensure the AppRegistry
# is populated before importing code that may import ORM models.
django_asgi_app = get_asgi_application()

from chat.routing import websocket_urlpatterns


class RouteValidator(BaseMiddleware):
    async def __call__(self, *args, **kwargs):
        try:
            await self.inner(*args, **kwargs)
        except (Resolver404, ValueError) as e:
            if "No route found for path" in str(e):
                return await WebsocketDenier()(*args, **kwargs)
            raise

application = ProtocolTypeRouter(
    {
        "http": django_asgi_app,
        "websocket": AllowedHostsOriginValidator(
            AuthMiddlewareStack(RouteValidator(URLRouter(websocket_urlpatterns)))
        ),
    }
)

[carltongibson]

The behaviour here has been the same since URLRouter was first added: a1044b4

I'm not immediately sure about a change here. We'd have to think that through.

(Why 403 as expected, not 404?)

[sprezz-arthur]

Thanks for the quick feedback.
I'm concerned whether I'm missing out on some of the reasoning, as it seems very deliberate choice to raise ValueError and cause 500 status code, instead of rejecting the handshake and causing some 4XX status code.

As for the 404 vs 403, at first I attempted to get a 404 out of it, but I couldn't manage to do so.
On the other hand, in order to get a 403, all I had to do is close the websocket in a try-except wrapper (the condition seems very brittle as I said though), similarly to how it's done on the middleware AllowedHostsOriginValidator in a rather straightforward approach.

[bigfootjon]

As for the 404 vs 403, at first I attempted to get a 404 out of it, but I couldn't manage to do so.

In other words a 404 is what you want, but the best you can do right now is a 403?

We'd have to think that through.

@carltongibson what are your concerns? Or what are you thinking through?

As things stand, we're inconsistent with Django core (which returns a 404 in this case), so it seems like we should consider making things consistent here.

[sprezz-arthur]

In other words a 404 is what you want, but the best you can do right now is a 403?

That's precisely it. I would just add 500 is what I don't understand.

[carltongibson]

@carltongibson what are your concerns? Or what are you thinking through?

Only that it's a change in a long-standing behaviour. I agree it doesn't seem right, but I'm wondering what work arounds people have used. 🤔

Likely a clear release note is the main thing.

[bigfootjon]

@sprezz-arthur can you write a PR to make the changes here? (with some test cases). Then we can discuss how to roll this out (release note warnings, etc)

[sprezz-arthur]

I'm away on holidays and will remain until the end of the month. I can come up with a proposal on June when I return.