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: ASGI mounted at base root ("/") intercepts dynamic path params #3535

Open
2 of 4 tasks
cemrehancavdar opened this issue May 29, 2024 · 7 comments
Open
2 of 4 tasks
Labels
Bug 🐛 This is something that is not working as expected

Comments

@cemrehancavdar
Copy link

cemrehancavdar commented May 29, 2024

Description

ASGI mounted at base root ("/") intercepts dynamic path params.
As explained by @peterschutt
peterschutt/sqladmin-litestar-plugin#11 (comment)

URL to code causing the issue

No response

MCVE

from litestar import Litestar, asgi, get
from litestar.types.asgi_types import Receive, Scope, Send


@get("/")
async def home() -> str:
    return "Hello, world!"


@asgi("/", is_mount=True)
async def mounted(scope: Scope, receive: Receive, send: Send) -> None:
    await send({"type": "http.response.start", "status": 200})
    await send({"type": "http.response.body", "body": b"Mounted!"})


@get("/dynamic/{name:int}")
async def dynamic(name: int) -> str:
    return f"Hello, {name}!"

@get("/not_dynamic")
async def not_dynamic() -> str:
    return "Hello, there!"

app = Litestar([home, dynamic, not_dynamic, mounted])

Steps to reproduce

1. check not_dynamic or / routes
2. It has no problem
3. /dynamic/test
4. See error

Screenshots

"![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

No response

Litestar Version

2.8.2

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
@cemrehancavdar cemrehancavdar added the Bug 🐛 This is something that is not working as expected label May 29, 2024
@peterschutt
Copy link
Contributor

peterschutt commented May 29, 2024

Also worth noting that if the path isn't dynamic, it is not a problem - this is b/c we handle "plain" routes separately, and before we test for mounted paths:

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

We then test for mounted paths before we step through the route trie:

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, {}

This is the source of the issue, b/c the dynamic route is only resolved as part of the trie traversal, but it never gets that far because the path /whatever/123 matches the mount path /.

So, I think we'd either need to get smarter with our mount_paths_regex so that it doesn't match on routes that are registered adjacent to the mounted application with variable paths, or, traverse the routing trie before checking for a match on a mount path, and only then try to match a mount path if we'd have otherwise thrown the 404.

@peterschutt
Copy link
Contributor

peterschutt commented May 29, 2024

Loosely related, we also cannot mount under a variable path, b/c the mount path regex includes the path variable placeholder, i.e., re.compile("/mounted/{num:int}") doesn't match /mounted/123.

E.g.,

@asgi("/mounted/{num:int}", is_mount=True)
async def mounted(scope: Scope, receive: Receive, send: Send) -> None:
    await send({"type": "http.response.start", "status": 200})
    await send({"type": "http.response.body", "body": b"Mounted!"})

@peterschutt
Copy link
Contributor

Maybe mounts should be routed during tree traversal, instead of separate from it. If we hit a node that has both a mount and child nodes with regular handlers, we try to route it further via the trie, and if that fails we then pipe it through the mount?

@ryanolf-tb
Copy link

I also came across this bug. Was going to file over at sqladmin-litestar-plugin but found it's already been raised there and pushed over here.

@provinzkraut
Copy link
Member

Maybe mounts should be routed during tree traversal, instead of separate from it. If we hit a node that has both a mount and child nodes with regular handlers, we try to route it further via the trie, and if that fails we then pipe it through the mount?

Sounds solid

@provinzkraut
Copy link
Member

Going back to this, I'm having second thought if we actually want to allow this. Mounting ASGI apps at the root path is always going to be kinda weird and cause unexpected behaviour in some cases, because you then have two entry points for your application. Maybe we should just not allow this in the first place and save us a lot of trouble?

@Rey092
Copy link

Rey092 commented Jun 30, 2024

Proposed a PR for sqladmin-litestar-plugin. I think mounting at "/" is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Development

No branches or pull requests

5 participants