Bug: Rate Limiter Middleware: exclude path is ignored, static files throw 429 error

ThinksFast commented 2 years ago

Describe the bug When using the RateLimiter middleware, the exclude path does not seem to be honored. In the code example below, the app serves an HTML response to the root URL path. In the HTML response, there is a reference to a static file which serves CSS. With a rate limit of 1 request per second, the homepage will get served, but the CSS file throws a 429 error code.

To Reproduce

APP:

import uvicorn
from starlite import (
    Request,
    Starlite,
    State,
    StaticFilesConfig,
    Template,
    TemplateConfig,
    get,
)
from starlite.middleware import RateLimitConfig
from starlite.template.jinja import JinjaTemplateEngine

@get("/")
async def serveHomepage(request: Request, state: State) -> Template:

    return Template(
        name="index.html.j2",
        context={"request": request},
    )

rate_limit_config = RateLimitConfig(
    rate_limit=("second", 1),
    exclude=[r"^/src.*$"],
)

app = Starlite(
    route_handlers=[serveHomepage],
    template_config=TemplateConfig(
        directory="src/templates", engine=JinjaTemplateEngine
    ),
    static_files_config=[
        StaticFilesConfig(directories=["src/static"], path="/src/static"),
        StaticFilesConfig(
            directories=["src/templates"], path="/src/templates", html_mode=True
        ),
    ],
    middleware=[rate_limit_config.middleware],
)

if __name__ == "__main__":
    uvicorn.run(app, host="127.0.0.1", port=8000)

HTML TEMPLATE:

<html>

<head>
  <link href="/src/static/css/styles.css" rel="stylesheet" type="text/css" />
</head>

<body>
  <h1>Header</h1>
</body>

</html>

STYLES.CSS... you can just make an empty file 😁

Additional context On my local, I'm running Starlite 1.38.0 on Python 3.11. Maybe there's an issue with how I'm declaring the path, but I've tried numerous string formats for the excludes option, none seem to work:

exclude=[r"^/src.*$"] exclude=[r"^src.*$"] exclude=["/src/"] exclude=["src/"] exclude=["/src/static/css/styles.css"]

provinzkraut commented 1 year ago

Somewhat easier to reproduce example:

from starlite import Starlite, StaticFilesConfig, TestClient
from starlite.middleware import RateLimitConfig

rate_limit_config = RateLimitConfig(rate_limit=("second", 1), exclude=[r"^/static.*$"])

app = Starlite(
    route_handlers=[],
    static_files_config=[
        StaticFilesConfig(directories=["static"], path="static"),
    ],
    middleware=[rate_limit_config.middleware],
)

with TestClient(app=app) as client:
    assert client.get("/static/starlite-hero.svg").status_code == 200
    assert client.get("/static/starlite-hero.svg").status_code == 200

I already pinned down the problem somewhat. middleware.utils.should_bypass_middleware does not receive the correct path. The path it receives does not include the /static prefix; This gets stripped away in asgi.routing_trie.traversal.parse_path_to_route. I'm not sure how far this is intended/expected behaviour or what the best fix for this is. Before I take a deep dive here, @Goldziher maybe you want to take a look at it?

provinzkraut commented 1 year ago

A possible solution would be to simply use scope["raw_path"] in should_bypass_middleware. I think that might more closely match expected behaviour? Although I'd still like to hear your input @Goldziher about the expected behaviour for paths there.

jtraub commented 1 year ago

I'm not sure how far this is intended/expected behaviour or what the best fix for this is

I think we have discussed this in Discord. Also, it is documented in https://starlite-api.github.io/starlite/usage/1-routing/5-mounting-asgi-apps/ (see Info box there).

StaticFiles is handled as a mounted ASGI app

provinzkraut commented 1 year ago

StaticFiles is handled as a mounted ASGI app

Hmm, I see. That makes handling of path-based exclusion a bit un-intuitive I think. WDYT of the solution to check against scope["raw_path"]?

jtraub commented 1 year ago

@provinzkraut I would personally expect here a more advanced fix than looking into raw_path.

For example, when I apply RateLimitingMiddleware on an app and then I mount this app into another Starlite app I expect that RateLimitingMiddleware here would not take into account raw_path because I expect that the mounted app behaves the same way standalone app works.

Maybe we can somehow expose is_mount flag to middlewares depending on their application point?

jtraub commented 1 year ago

Just to be explicit about StaticFiles: This line makes StaticFiles to be treated as a mounted app.

jtraub commented 1 year ago

WDYT of the solution to check against scope["raw_path"]

I think it should be a big PR that would propagate is_mount flag value to middlewares and route handlers. Looking into path or raw_path would depend on value of that flag. FastAPI seems to be doing something like this - I took a look at it on Monday (yes, I am strugging with a similar problem so I was exploring what others do).

Related issue here - #788

provinzkraut commented 1 year ago

Maybe we can somehow expose is_mount flag to middlewares depending on their application point?

That could work but would be messy. How about adding the relevant information to the scope? E.g. something like scope["mount_path"] = self.path if something is mounted?

jtraub commented 1 year ago

How about adding the relevant information to the scope?

This would work but what is our policy on internal middlewares and asgi apps compatibility? Do we want to make them compatible so others can use them outside of Starlite?

If we do not care about this we can take your way for sure.

provinzkraut commented 1 year ago

We could do it in a compatible way as well. Passing is_mount around is also not really compatible. What we could do is simply check if such a key exists, if it does use it to re-construct the path from scope["mount_path"] and scope["path"], if it doesn't exist, we just use the scope["path"] as is.

jtraub commented 1 year ago

Yeah, that makes sense. Lets do it the way your described.

@provinzkraut I can do it this weekend + fix openapi generation for mounted starlite apps (and maybe #788) if you are busy with something else as it directly affects my employer apps :-) If you want to fix it yourself - feel free to do it. I have a lot of problems with pydantic-factories at the moment so I will work on something else.

provinzkraut commented 1 year ago

@jtraub I'm fine either way, if you wanna do it have at it, otherwise I'll take this on.

jtraub commented 1 year ago

@provinzkraut ok, let me take this then as I would be able to test the fix extensively tomorrow.

Goldziher commented 1 year ago

so, any updates on this?

jtraub commented 1 year ago

I got it working fine but got buried with trying to fix piccolo admin and make all middlewares work in a mounted starlite instance

I am in the bed already so give me some time to make a PR tomorrow :-)

litestar-org / litestar

Bug: Rate Limiter Middleware: exclude path is ignored, static files throw 429 error #781