Skip to content

Fix included router rate limitingfix: handle included routers in default rate limiting - #282

Open
15054538509 wants to merge 1 commit into
laurentS:masterfrom
15054538509:codex/slowapi-router-tree-fix
Open

Fix included router rate limitingfix: handle included routers in default rate limiting#282
15054538509 wants to merge 1 commit into
laurentS:masterfrom
15054538509:codex/slowapi-router-tree-fix

Conversation

@15054538509

Copy link
Copy Markdown

Fix default rate limiting for FastAPI included routers.

Root cause: FastAPI 0.137+ stores include_router() entries as _IncludedRouter proxies, so app.routes no longer exposes a direct .endpoint for nested routes. slowapi was treating those routes as unmatched and skipping default limits.

Changes:

  • recurse through nested router trees when resolving the handler
  • add a FastAPI regression test for an included router with default_limits

Validation:

  • pytest tests/test_fastapi_extension.py -q
  • 60 passed

@tsukumijima

tsukumijima commented Jul 23, 2026

Copy link
Copy Markdown

@15054538509
Thanks for working on this. I tested the PR commit (63cb749d) against FastAPI 0.137.0 / Starlette 1.3.1, and the fix is incomplete for router prefixes applied by include_router() and for nested included routers.

The added test uses APIRouter(prefix="/api"). In that case the original APIRoute.path already contains /api, so walking original_router.routes happens to work. These common variants still bypass the default limit with both SlowAPIMiddleware and SlowAPIASGIMiddleware:

limiter = Limiter(key_func=lambda request: request.client.host, default_limits=["1/minute"])
app = FastAPI()
app.state.limiter = limiter
app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
app.add_middleware(SlowAPIMiddleware)  # same result with SlowAPIASGIMiddleware

router = APIRouter()

@router.get("/t1")
async def endpoint(request: Request):
    return {"ok": True}

app.include_router(router, prefix="/api")

with TestClient(app) as client:
    assert [client.get("/api/t1").status_code for _ in range(2)] == [200, 429]

Actual result on this PR: [200, 200].

A two-level include_router() tree also produces [200, 200]:

inner = APIRouter(prefix="/inner")
# add GET /t1 to inner
outer = APIRouter(prefix="/outer")
outer.include_router(inner)
app.include_router(outer, prefix="/api")
# GET /api/outer/inner/t1 twice -> [200, 200]

The reason is that FastAPI 0.137's _IncludedRouter.matches() returns Match.FULL with an empty child scope, while the combined include prefixes live in effective_candidates(). Recursing through original_router.routes discards the include context, so the child routes are matched against their uncombined paths.

A minimal change that I verified locally is to prefer the effective candidates before falling back to the existing generic route/original-router lookup:

def _get_nested_routes(route: BaseRoute) -> Optional[Iterable[BaseRoute]]:
    effective_candidates = getattr(route, "effective_candidates", None)
    if callable(effective_candidates):
        return effective_candidates()

    nested_routes = getattr(route, "routes", None)
    if nested_routes is not None:
        return nested_routes

    original_router = getattr(route, "original_router", None)
    return getattr(original_router, "routes", None)

With that change, I got [200, 429] for all of these cases with both middleware implementations:

  • prefix declared on APIRouter
  • prefix passed to app.include_router()
  • nested included routers with combined prefixes

There is also a test-environment gap: this repository's current poetry.lock resolves FastAPI 0.89.1 and Starlette 0.22.0. I ran the new test against the parent commit in that locked environment and it already passed ([200, 429]), so it does not prove the FastAPI 0.137 regression is fixed unless CI explicitly installs FastAPI 0.137+ (or the lock file is updated). I suggest changing the regression test to apply the prefix through include_router(), adding the nested case, and ensuring at least one CI job runs them on FastAPI 0.137+.

@Mukller Mukller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triage note for the included-router cluster (#285, #286, #282): I verified all three functionally on FastAPI 0.141.1 with a uniform probe (default_limits only, SlowAPIMiddleware, decorator-free endpoint — the path where only _find_route_handler can enforce):

app.include_router(sub)            -> second request status
app.include_router(sub, prefix="/sub")

master : 200,200 BYPASSED   | 200,200 BYPASSED     (bug confirmed)
#285   : 200,429 ENFORCED   | 200,429 ENFORCED
#286   : 200,429 ENFORCED   | 200,429 ENFORCED
#282   : 200,429 ENFORCED   | 200,200 BYPASSED    <- THIS PR: prefixed case still leaks

Repro of the remaining gap on this branch — the prefixed include still bypasses default limits:

limiter = Limiter(key_func=get_remote_address, default_limits=["1/minute"])
sub = APIRouter()

@sub.get("/inner")
def inner(): ...

app.add_middleware(SlowAPIMiddleware)
app.include_router(sub, prefix="/sub")   # <- with prefix
# GET /sub/inner x2 -> 200, 200  (expected 200, 429)

Likely cause: on 0.141 the lazy wrapper is _IncludedRouter, which has no .routes attribute; your _get_nested_routes falls back to original_router.routes, but for a prefixed include the nested routes' paths don't carry the prefix, so matches(scope) against the un-prefixed child scope returns NONE and recursion dies there. #285/#286 avoid it by recursing through effective_route_contexts()/_effective_candidates(), which carry per-context state.

Also worth noting: merging child_scope into the nested scope (which this PR does and others don't) is a good idea in principle — the winning candidates just need to reach the right level first.

Everything else about this PR (suite passes locally: 60 passed) is fine; it's only the prefixed-lazy case that needs one more hop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants