django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.46k stars 207 forks source link

`PATH_INFO` set incorrectly by `WsgiToAsgiInstance.build_environ()` #442

Closed srittau closed 5 months ago

srittau commented 6 months ago

See also #424 and especially this comment by @tiangolo for background.

PATH_INFO seems to be set incorrectly by WsgiToAsgiInstance.build_environ(). Currently, it's just copied over from the path ASGI scope:

https://github.com/django/asgiref/blob/8cf847a27da34e7d4490b41b682f657b4f302eaa/asgiref/wsgi.py#L57-L70

But path includes the root_path, while the WSGI spec (PEP 3333) says:

SCRIPT_NAME The initial portion of the request URL’s “path” that corresponds to the application object, so that the application knows its virtual “location”. This may be an empty string, if the application corresponds to the “root” of the server. PATH_INFO The remainder of the request URL’s “path”, designating the virtual “location” of the request’s target within the application. This may be an empty string, if the request URL targets the application root and does not have a trailing slash.

A holdover from the CGI spec (RFC 3875).

When starlette fixed their path handling, they changed their WSGI adapter to account for that:

https://github.com/encode/starlette/blob/7533b61a34f8b0dcb5fc35c717d458439f0baf18/starlette/middleware/wsgi.py#L24-L42

    script_name = scope.get("root_path", "").encode("utf8").decode("latin1")
    path_info = scope["path"].encode("utf8").decode("latin1")
    if path_info.startswith(script_name):
        path_info = path_info[len(script_name) :]

    environ = {
        # ...
        "SCRIPT_NAME": script_name,
        "PATH_INFO": path_info,
        # ...
    }

I think asgiref's adapter should do the same.

andrewgodwin commented 6 months ago

Yes, I agree. I likely made a shortcut in the initial version to get it working and then never came back and improved it.