Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: 405: Method Not Allowed when using Websockets with Litestar and Nginx Unit #3887

Open
1 of 4 tasks
FixFlare opened this issue Dec 5, 2024 · 10 comments
Open
1 of 4 tasks
Labels
Upstream This is due to or tracking an upstream issue

Comments

@FixFlare
Copy link

FixFlare commented Dec 5, 2024

Description

I believe there may be an issue with how Litestar handles Websocket connections incoming from a client app hosted with Nginx Unit.

This problem does not happen with uvicorn, only Nginx Unit.

From my typescript react app I initiate the websocket connection:

      const ws = new WebSocket(
        `${import.meta.env.VITE_WEBSOCKET_URL}/test?token=myauthtoken`,
      );

and receive this error from litestar:

/site-packages/litestar/_asgi/routing_trie/traversal.py", line 174, in parse_path_to_route
    raise MethodNotAllowedException() from e
litestar.exceptions.http_exceptions.MethodNotAllowedException: 405: Method Not Allowed

I traced the issue back to this function in that same file:

def parse_node_handlers(
    node: RouteTrieNode,
    method: Method | None,
) -> ASGIHandlerTuple:
    """Retrieve the handler tuple from the node.

    Args:
        node: The trie node to parse.
        method: The scope's method.

    Raises:
        KeyError: If no matching method is found.

    Returns:
        An ASGI Handler tuple.
    """

    if node.is_asgi:
        return node.asgi_handlers["asgi"]
    if method:
        return node.asgi_handlers[method]
    return node.asgi_handlers["websocket"]

When I watch the "method" parameter on the incoming connection from the websocket and I'm using Uvicorn, method is "None" and everything works as expected.

When using Nginx Unit method is "GET" so it tries to handle it like an http connection rather than a websocket one and you get the above error.

If I then modify

if method:

to

if method and not node.asgi_handlers.get("websocket"):

I get past the "method not allowed" error but then I get

/site-packages/litestar/middleware/_internal/exceptions/middleware.py", line 232, in handle_websocket_exception
    await send(event)
RuntimeError: WebSocket connect not received

I then took a look at the function from the first error:

def parse_path_to_route(
    method: Method | None,
    mount_paths_regex: Pattern | None,
    mount_routes: dict[str, RouteTrieNode],
    path: str,
    plain_routes: set[str],
    root_node: RouteTrieNode,
) -> tuple[ASGIApp, RouteHandlerType, str, dict[str, Any], str]:
    """Given a scope object, retrieve the asgi_handlers and is_mount boolean values from correct trie node.

    Args:
        method: The scope's method, if any.
        root_node: The root trie node.
        path: The path to resolve scope instance.
        plain_routes: The set of plain routes.
        mount_routes: Mapping of mount routes to trie nodes.
        mount_paths_regex: A compiled regex to match the mount routes.

    Raises:
        MethodNotAllowedException: if no matching method is found.
        NotFoundException: If no correlating node is found or if path params can not be parsed into values according to the node definition.

    Returns:
        A tuple containing the stack of middlewares and the route handler that is wrapped by it.
    """

    try:
        if path in plain_routes:
            asgi_app, handler = parse_node_handlers(node=root_node.children[path], method=method)
            return asgi_app, handler, path, {}, path

        if mount_paths_regex and (match := mount_paths_regex.match(path)):
            mount_path = path[: match.end()]
            mount_node = mount_routes[mount_path]
            remaining_path = path[match.end() :]
            # since we allow regular handlers under static paths, we must validate that the request does not match
            # any such handler.
            children = (
                normalize_path(sub_route)
                for sub_route in mount_node.children or []
                if sub_route != mount_path and isinstance(sub_route, str)
            )
            if not any(remaining_path.startswith(f"{sub_route}/") for sub_route in children):
                asgi_app, handler = parse_node_handlers(node=mount_node, method=method)
                remaining_path = remaining_path or "/"
                if not mount_node.is_static:
                    remaining_path = remaining_path if remaining_path.endswith("/") else f"{remaining_path}/"
                return asgi_app, handler, remaining_path, {}, root_node.path_template

        node, path_parameters, path = traverse_route_map(
            root_node=root_node,
            path=path,
        )
        asgi_app, handler = parse_node_handlers(node=node, method=method)
        key = method or ("asgi" if node.is_asgi else "websocket")
        parsed_path_parameters = parse_path_params(node.path_parameters[key], tuple(path_parameters))

        return (
            asgi_app,
            handler,
            path,
            parsed_path_parameters,
            node.path_template,
        )
    except KeyError as e:
        raise MethodNotAllowedException() from e
    except ValueError as e:
        raise NotFoundException() from e

I then modified

key = method or ("asgi" if node.is_asgi else "websocket")

to

key = method if method and not node.asgi_handlers.get("websocket") else ("asgi" if node.is_asgi else "websocket")

and now everything works as expected.

The reason I believe this may be a bug is the way if its determining if it's a websocket connection in the "parse_node_handlers" function.

When I check the websocket spec, page 17, point 2 it says

"The method of the request MUST be GET, and the HTTP version MUST be at least 1.1."

So I think the method coming through as "GET" on the websocket connection from Nginx Unit is normal behavior and the method being "None" from Uvicorn is abnormal.

Unfortunately, it seems like Litestar relies on method being "none" currently to handle the websocket connection which is breaking websockets for servers following that spec.

URL to code causing the issue

No response

MCVE

No response

Steps to reproduce

No response

Screenshots

No response

Logs

/site-packages/litestar/_asgi/routing_trie/traversal.py", line 174, in parse_path_to_route
    raise MethodNotAllowedException() from e
litestar.exceptions.http_exceptions.MethodNotAllowedException: 405: Method Not Allowed

/site-packages/litestar/middleware/_internal/exceptions/middleware.py", line 232, in handle_websocket_exception
    await send(event)
RuntimeError: WebSocket connect not received

Litestar Version

2.13.0

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@FixFlare FixFlare added the Bug 🐛 This is something that is not working as expected label Dec 5, 2024
@euri10
Copy link
Contributor

euri10 commented Dec 5, 2024

interesting, I dont remember we do something fancy in uvicorn.
a few questions: do you use websockets or wsproto in uvicorn ? does the same happens with hypercorn or granian ?

@FixFlare
Copy link
Author

FixFlare commented Dec 5, 2024

This is actually for a work project so I'm a bit limited in what software I'm allowed to install - IT Security and all that.

I've only tested uvicorn and Nginx Unit but I've tested this on multiple machines. I didn't actually have this problem using uvicorn and developing the whole thing it was only when I deployed to our production server that was running a production asgi server that this issue cropped up.

Afterwards, I also installed Nginx Unit on my dev machine to confirm the issue and find the cause.

[[package]]
name = "websockets"
version = "14.1"

If it isn't uvicorn doing it, my only other thought was maybe there was some upstream code in Litestar that clears the method during the websocket connection in certain scenarios but, assuming the method parameter is literal and unaltered (meaning it is supposed to be exactly as sent GET, POST etc.) then according to the spec, method should be "GET" for websocket connections so I wouldn't think the way it is written now would be a valid way to determine if the connection is a websocket or not, no?

1    if node.is_asgi:
2        return node.asgi_handlers["asgi"]
3    if method:
4        return node.asgi_handlers[method]
5    return node.asgi_handlers["websocket"] 

I'm trying to understand how line 5 will ever get hit if method is always supposed to be "GET" for the incoming websocket connection?

@euri10
Copy link
Contributor

euri10 commented Dec 6, 2024

ok too bad, that would have been useful to pinpoint if uvicorn is doing something fancy.
would you mind setting up a small repro, I wanted to try nginx unit for a long time so that would be a good occasion ;)

@FixFlare
Copy link
Author

Sorry for the delay was out of the office Friday and Monday.

I wasn't entirely sure how to package this for you for a repro because of all the requirements but I tried my best.

Check the README.md on this repo: https://github.com/FixFlare/bug_3887 and let me know if you have any questions.

There is a sample nginx unit config in there as well for you to use.

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Dec 11, 2024

@euri10 This feels the same as #3840 (comment)

@euri10
Copy link
Contributor

euri10 commented Dec 11, 2024

ok thank you for linking that @Alc-Alc and thanks for the very involved repro @FixFlare , it seems like it's an issue upstream indeed.
on a side note, I see you're using aioredis which died last year, you may want to change that probably.

@provinzkraut provinzkraut added Upstream This is due to or tracking an upstream issue and removed Bug 🐛 This is something that is not working as expected labels Dec 11, 2024
@provinzkraut
Copy link
Member

So while this isn't really a bug, as it's caused by nginx unit not implementing the ASGI spec correctly, I'm thinking if we should maybe make some changes that would avoid this. Right not we're inferring if we should use a WS handler based on the absence of a method, but this information is also passed to us trough scope["type"]. If we were to use that instead, we wouldn't run into this issue in the first place.

@FixFlare
Copy link
Author

@euri10 Noted - thanks
I'm mainly a .NET guy so not as up-to-date on the happenings of python packages. I'm sure I got it off some old documentation page that hadn't been updated yet.

@Alc-Alc I agree, looks to be the same issue and I understand the point being made by @provinzkraut - thank you both

With that said, I opened up an issue over on the Nginx Unit github here

The good news is I have a workaround for now and I think it's best for everyone if the issue gets fixed at the actual source.

@Alc-Alc
Copy link
Contributor

Alc-Alc commented Dec 11, 2024

@FixFlare Just as an aside

During development I used Litestar's recommended dev server "uvicorn"

uvicorn is not just a dev server btw 🙂

@FixFlare
Copy link
Author

@Alc-Alc Fair enough, didn't mean to imply that was its only use.

That more came from this page

Where on Nginx Unit it mentions:

Running your application with nginx-unit is preferable when you need to run your application in a production environment, with a high level of control over the process.

To me that reads like use uvicorn for dev, use Nginx Unit if you're running in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Upstream This is due to or tracking an upstream issue
Projects
None yet
Development

No branches or pull requests

4 participants