From 0e110b9376f686ba64f50f870aeb0cd2f68383f7 Mon Sep 17 00:00:00 2001 From: crypto-a Date: Mon, 31 Aug 2026 15:15:24 -0400 Subject: [PATCH 1/2] feat(httpapi): trusted_prefixes, for paths that are not browser surfaces An operator or fleet surface reached by another server holding a bearer is not a browser surface. Two of the conventions aimed at browsers and public callers are wrong for it, and one parameter now turns both off, because it is one fact about the service rather than two settings. Split across CORS and RateLimit, a deployment could exempt a prefix from one and forget the other. CORS is skipped because access-control-allow-origin on those paths names a browser origin that will never call them. The header is a statement about who may read the response, and there the statement is false. BE PRECISE ABOUT WHAT THAT DOES NOT DO, because the tempting claim is wrong and would be believed. It does not hide the paths. CORSMiddleware answers a preflight without consulting the router, so OPTIONS on a path matching no route already returns 200 with the allow-list, and an exempt path and a nonexistent one look alike either way. What is removed is a header on a real response. That is a false statement withdrawn, not a disclosure fixed, and a test pins the distinction so nobody documents it as the latter. The limiter is skipped for the reason EXEMPT_PATHS already gives about the probes. A per-address bucket is the wrong instrument for a surface whose every caller shares one trusted address: the ceiling refuses an operator rather than an abuser, and the credential on the router is what guards it. Concretely, a bare RateLimit() resolves to 60 requests a minute with a burst of 20 per address, which a console walking a customer list exhausts in twenty rows and then crawls, presenting as a hung page rather than as a limit. exempt_prefixes is a SEPARATE field from exempt_paths and the matching rules differ deliberately. The probes stay exact so /healthz-fake inherits nothing; an operator surface has to be a prefix because its routes are not enumerable from here. Widening exempt_paths to prefix matching would have been one less field and would have exempted every path merely beginning with /healthz, which reads as a simplification in review and is a hole afterwards. Both are pinned. It grants nothing. A prefix listed here is exactly as reachable as it was, and whatever guards its router still does. --- src/kit/httpapi/_cors_middleware.py | 58 ++++++++ src/kit/httpapi/_install.py | 76 +++++++--- src/kit/httpapi/_ratelimit_middleware.py | 15 +- tests/test_trusted_prefixes.py | 169 +++++++++++++++++++++++ 4 files changed, 299 insertions(+), 19 deletions(-) create mode 100644 src/kit/httpapi/_cors_middleware.py create mode 100644 tests/test_trusted_prefixes.py diff --git a/src/kit/httpapi/_cors_middleware.py b/src/kit/httpapi/_cors_middleware.py new file mode 100644 index 0000000..9474c8b --- /dev/null +++ b/src/kit/httpapi/_cors_middleware.py @@ -0,0 +1,58 @@ +"""CORS, except on the paths that are not browser surfaces.""" + +from __future__ import annotations + +from typing import Any + +from starlette.middleware.cors import CORSMiddleware +from starlette.types import ASGIApp, Receive, Scope, Send + + +class ScopedCORSMiddleware: + """``CORSMiddleware``, skipped entirely under a trusted prefix. + + WHY A SERVICE WOULD WANT THIS. An operator or fleet surface reached only by + another server holding a bearer is not a browser surface. Answering it with + ``access-control-allow-origin`` names an origin whose browser may read the + response, and for those paths there is no such origin and never will be. The + header is a statement about who may call, and on those paths the statement is + false. + + BE PRECISE ABOUT WHAT THIS DOES NOT DO, because the tempting claim is wrong + and would be believed. It does NOT hide the paths. ``CORSMiddleware`` answers + a preflight without consulting the router, so ``OPTIONS`` on a path that + matches no route already returns 200 with the allow-list; exempting a prefix + changes nothing a prober can see, because an exempt path and a nonexistent + one look alike either way. What it removes is the header on a REAL response, + which is the false statement, not a disclosure. + + Nor is it authorization. A path under a trusted prefix is exactly as reachable + as it was; CORS never restricted a server-side caller, only a browser. This + narrows what the service SAYS, and the credential on the route is still the + entire boundary. + + The inner app is wrapped twice on purpose: ``self.app`` is the chain without + CORS and ``self.cors`` is the same chain with it, so dispatch is a prefix test + and neither path pays for the other. + """ + + def __init__( + self, + app: ASGIApp, + *, + options: dict[str, Any], + prefixes: tuple[str, ...] = (), + ) -> None: + self.app = app + self.cors = CORSMiddleware(app, **options) + # A tuple, because `str.startswith` takes one and testing a tuple is a + # single call rather than a loop that a future edit turns into `any`. + self.prefixes = prefixes + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] == "http" and self.prefixes: + path: str = scope.get("path", "") + if path.startswith(self.prefixes): + await self.app(scope, receive, send) + return + await self.cors(scope, receive, send) diff --git a/src/kit/httpapi/_install.py b/src/kit/httpapi/_install.py index d194b3d..f6dee13 100644 --- a/src/kit/httpapi/_install.py +++ b/src/kit/httpapi/_install.py @@ -5,7 +5,6 @@ import logging from fastapi import FastAPI -from starlette.middleware.cors import CORSMiddleware from kit.health import Registry from kit.httpapi._cors import ( @@ -18,6 +17,7 @@ PUBLIC_READ_METHODS, normalize_origins, ) +from kit.httpapi._cors_middleware import ScopedCORSMiddleware from kit.httpapi._handlers import CodeMapper, default_code_for, install_error_handlers from kit.httpapi._middleware import ( RecoveryMiddleware, @@ -29,12 +29,23 @@ from kit.httpapi._ratelimit_middleware import RateLimitMiddleware +def _options(**kwargs: object) -> dict[str, object]: + """The CORSMiddleware keyword arguments, as a dict it can be splatted from. + + A named helper rather than a literal so the two branches below still read as + argument lists rather than as dictionaries, which is what makes the + difference between them, credentials against wildcard, legible at a glance. + """ + return kwargs + + def install_conventions( app: FastAPI, *, readiness: Registry, cors: CORS | None = None, rate_limit: RateLimit | None = None, + trusted_prefixes: tuple[str, ...] = (), logger: logging.Logger | None = None, code_for: CodeMapper = default_code_for, ) -> None: @@ -57,37 +68,66 @@ def install_conventions( Starlette applies middleware in reverse registration order, so the calls below read outermost-last. + + ``trusted_prefixes`` names path prefixes that are NOT BROWSER SURFACES: an + operator or fleet surface reached by another server holding a credential. One + parameter drives two exemptions because it is one fact about the service, and + splitting it across ``CORS`` and ``RateLimit`` would let a deployment exempt + a prefix from one and forget the other. + + * CORS is skipped, because ``access-control-allow-origin`` on those paths + names a browser origin that will never call them. See + ``ScopedCORSMiddleware`` for what that does and, more importantly, does not + achieve: it removes a false statement, it does not hide a route. + * The limiter is skipped, for the reason ``EXEMPT_PATHS`` already gives about + the probes. A per-address bucket is the wrong instrument for a surface + whose every caller shares one trusted address; the credential is what + guards it, and a 429 there refuses an operator rather than an abuser. + + It is deliberately NOT authorization and grants nothing. A prefix listed here + is exactly as reachable as it was, and whatever guards its router still does. """ log = logger or logging.getLogger("kit.httpapi") # Innermost first, because Starlette wraps in reverse. if rate_limit is not None: - app.add_middleware(RateLimitMiddleware, limit=rate_limit, logger=log) + app.add_middleware( + RateLimitMiddleware, + limit=rate_limit, + logger=log, + exempt_prefixes=trusted_prefixes, + ) app.add_middleware(RecoveryMiddleware, logger=log) if cors is not None and cors.enabled: if cors.public_read: app.add_middleware( - CORSMiddleware, - allow_origins=["*"], - # Never credentials. Its absence is what makes the wildcard both - # browser-legal and safe. - allow_credentials=False, - allow_methods=[m.strip() for m in PUBLIC_READ_METHODS.split(",")], - allow_headers=[h.strip() for h in PUBLIC_READ_HEADERS.split(",")], - expose_headers=[h.strip() for h in EXPOSED_HEADERS.split(",")], - max_age=MAX_AGE_SECONDS, + ScopedCORSMiddleware, + prefixes=trusted_prefixes, + options=_options( + allow_origins=["*"], + # Never credentials. Its absence is what makes the wildcard both + # browser-legal and safe. + allow_credentials=False, + allow_methods=[m.strip() for m in PUBLIC_READ_METHODS.split(",")], + allow_headers=[h.strip() for h in PUBLIC_READ_HEADERS.split(",")], + expose_headers=[h.strip() for h in EXPOSED_HEADERS.split(",")], + max_age=MAX_AGE_SECONDS, + ), ) else: app.add_middleware( - CORSMiddleware, - allow_origins=sorted(normalize_origins(cors.allowed_origins)), - allow_credentials=True, - allow_methods=[m.strip() for m in CREDENTIALED_METHODS.split(",")], - allow_headers=[h.strip() for h in CREDENTIALED_HEADERS.split(",")], - expose_headers=[h.strip() for h in EXPOSED_HEADERS.split(",")], - max_age=MAX_AGE_SECONDS, + ScopedCORSMiddleware, + prefixes=trusted_prefixes, + options=_options( + allow_origins=sorted(normalize_origins(cors.allowed_origins)), + allow_credentials=True, + allow_methods=[m.strip() for m in CREDENTIALED_METHODS.split(",")], + allow_headers=[h.strip() for h in CREDENTIALED_HEADERS.split(",")], + expose_headers=[h.strip() for h in EXPOSED_HEADERS.split(",")], + max_age=MAX_AGE_SECONDS, + ), ) app.add_middleware(RequestLogMiddleware, logger=log) diff --git a/src/kit/httpapi/_ratelimit_middleware.py b/src/kit/httpapi/_ratelimit_middleware.py index 83d48f8..24f110d 100644 --- a/src/kit/httpapi/_ratelimit_middleware.py +++ b/src/kit/httpapi/_ratelimit_middleware.py @@ -62,18 +62,31 @@ def __init__( limit: RateLimit, logger: logging.Logger | None = None, exempt_paths: frozenset[str] = EXEMPT_PATHS, + exempt_prefixes: tuple[str, ...] = (), ) -> None: self.app = app self.log = logger or logging.getLogger("kit.httpapi") self.limiter = Limiter(limit=limit, logger=self.log) self.enabled = not limit.off self.exempt_paths = exempt_paths + # PREFIXES ARE A SEPARATE FIELD FROM PATHS, and the two matching rules + # differ on purpose. The probes are exempted by EXACT match so that + # `/healthz-fake` inherits nothing; a whole operator surface has to be + # exempted by prefix because its routes are not enumerable here. + # + # Widening `exempt_paths` to prefix matching would have been one less + # field and would silently have exempted every path merely BEGINNING + # with `/healthz`, which is the kind of change that looks like a + # simplification in review and is a hole afterwards. + self.exempt_prefixes = exempt_prefixes async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + path: str = scope.get("path", "") if ( scope["type"] != "http" or not self.enabled - or scope.get("path", "") in self.exempt_paths + or path in self.exempt_paths + or (bool(self.exempt_prefixes) and path.startswith(self.exempt_prefixes)) ): await self.app(scope, receive, send) return diff --git a/tests/test_trusted_prefixes.py b/tests/test_trusted_prefixes.py new file mode 100644 index 0000000..33f89bf --- /dev/null +++ b/tests/test_trusted_prefixes.py @@ -0,0 +1,169 @@ +"""Prefixes that are not browser surfaces, and what exempting one does. + +An operator or fleet surface is reached by another server holding a credential. +It is not a browser surface, so two of the conventions aimed at browsers and +public callers are wrong for it, and `trusted_prefixes` turns both off with one +statement because they are one fact about the service. +""" + +from __future__ import annotations + +import httpx +import pytest +from fastapi import FastAPI + +from kit.health import Registry +from kit.httpapi import CORS, RateLimit, install_conventions + +ORIGIN = "https://app.example.com" +ADMIN = "/api/admin" + + +def app_with(*, trusted: tuple[str, ...] = (), limit: RateLimit | None = None) -> FastAPI: + app = FastAPI() + install_conventions( + app, + readiness=Registry(), + cors=CORS(allowed_origins=(ORIGIN,)), + rate_limit=limit if limit is not None else RateLimit(), + trusted_prefixes=trusted, + ) + + @app.get("/api/v1/thing") + async def customer() -> dict[str, bool]: + return {"ok": True} + + @app.get(f"{ADMIN}/v1/organizations") + async def operator() -> dict[str, bool]: + return {"ok": True} + + return app + + +async def get(app: FastAPI, path: str, headers: dict[str, str] | None = None) -> httpx.Response: + transport = httpx.ASGITransport(app=app, raise_app_exceptions=False) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + return await client.get(path, headers=headers) + + +class TestCORSIsSkippedUnderATrustedPrefix: + async def test_a_customer_route_still_answers_a_browser(self) -> None: + """The exemption must be narrow. Everything not under the prefix keeps + the credentialed allow-list it had, or this is a regression dressed as a + hardening.""" + response = await get(app_with(trusted=(ADMIN,)), "/api/v1/thing", {"origin": ORIGIN}) + + assert response.headers["access-control-allow-origin"] == ORIGIN + assert response.headers["access-control-allow-credentials"] == "true" + + async def test_an_operator_route_names_no_origin(self) -> None: + """THE POINT OF THE FEATURE, in one assertion. + + `access-control-allow-origin` is a statement that a browser at that + origin may read the response. On a surface only another server ever + calls, that statement is false, and a false statement in security config + is the kind that becomes true when somebody later adds an origin. + """ + response = await get( + app_with(trusted=(ADMIN,)), f"{ADMIN}/v1/organizations", {"origin": ORIGIN} + ) + + assert response.status_code == 200 + assert "access-control-allow-origin" not in response.headers + assert "access-control-allow-credentials" not in response.headers + + async def test_without_the_prefix_the_operator_route_would_carry_the_header(self) -> None: + """The before picture, so the test above is measuring the feature rather + than a route that never had CORS in the first place.""" + response = await get(app_with(), f"{ADMIN}/v1/organizations", {"origin": ORIGIN}) + + assert response.headers["access-control-allow-origin"] == ORIGIN + + async def test_it_does_not_hide_the_route(self) -> None: + """WHAT THIS DELIBERATELY DOES NOT DO, pinned so nobody claims otherwise. + + The tempting justification is that exempting a prefix conceals an + operator surface from a browser probe. It does not. The route answers + exactly as before; only the header is gone. Anybody documenting this as + a disclosure fix is wrong, and this test is where they find out. + """ + response = await get( + app_with(trusted=(ADMIN,)), f"{ADMIN}/v1/organizations", {"origin": ORIGIN} + ) + + assert response.status_code == 200 + assert response.json() == {"ok": True} + + async def test_a_request_with_no_origin_is_unaffected(self) -> None: + """The real caller. A server-side client sends no Origin at all, which is + why CORS was inert for it either way.""" + response = await get(app_with(trusted=(ADMIN,)), f"{ADMIN}/v1/organizations") + + assert response.status_code == 200 + + +class TestTheLimiterIsSkippedUnderATrustedPrefix: + LIMIT = RateLimit(requests=1, window_seconds=60.0, burst=1) + + async def test_a_customer_route_is_still_limited(self) -> None: + app = app_with(trusted=(ADMIN,), limit=self.LIMIT) + + first = await get(app, "/api/v1/thing") + second = await get(app, "/api/v1/thing") + + assert first.status_code == 200 + assert second.status_code == 429 + + async def test_an_operator_route_is_not(self) -> None: + """A per-address bucket is the wrong instrument here. Every operator + action arrives from one trusted address, so the ceiling refuses an + operator rather than an abuser, and the credential is what guards it.""" + app = app_with(trusted=(ADMIN,), limit=self.LIMIT) + + for _ in range(5): + response = await get(app, f"{ADMIN}/v1/organizations") + assert response.status_code == 200 + + async def test_the_probes_stay_exempt_by_exact_match(self) -> None: + """`EXEMPT_PATHS` is exact and stays exact. Widening it to prefixes would + have been one less field and would have exempted every path merely + BEGINNING with `/healthz`, which reads as a simplification and is a + hole.""" + app = app_with(limit=self.LIMIT) + + for _ in range(5): + assert (await get(app, "/healthz")).status_code == 200 + + async def test_a_path_merely_beginning_with_a_probe_name_is_not_exempt(self) -> None: + app = app_with(limit=self.LIMIT) + + first = await get(app, "/healthz-fake") + second = await get(app, "/healthz-fake") + + # 404 rather than 200: the route does not exist. What matters is that the + # limiter counted it, which the second answer proves. + assert first.status_code == 404 + assert second.status_code == 429 + + +@pytest.mark.parametrize("public_read", [True, False]) +async def test_both_cors_modes_honour_the_exemption(public_read: bool) -> None: + """The wildcard branch and the credentialed branch are separate calls in + `install_conventions`, so exempting one and forgetting the other is a live + possibility rather than a hypothetical.""" + app = FastAPI() + install_conventions( + app, + readiness=Registry(), + cors=CORS(allowed_origins=() if public_read else (ORIGIN,), public_read=public_read), + rate_limit=RateLimit(), + trusted_prefixes=(ADMIN,), + ) + + @app.get(f"{ADMIN}/v1/organizations") + async def operator() -> dict[str, bool]: + return {"ok": True} + + response = await get(app, f"{ADMIN}/v1/organizations", {"origin": ORIGIN}) + + assert "access-control-allow-origin" not in response.headers From 99888c2dfaa9745b8c6245f220714a97876b6793 Mon Sep 17 00:00:00 2001 From: crypto-a Date: Mon, 31 Aug 2026 15:29:09 -0400 Subject: [PATCH 2/2] fix(httpapi): close three ways a trusted prefix goes overbroad All three found by codex review, and each fails in the direction that does not announce itself. AN EMPTY PREFIX EXEMPTED EVERYTHING. Every path starts with the empty string, so one empty entry turned off CORS and rate limiting for the whole application, silently, with a green deployment. Not a hypothetical typo: splitting an unset comma-separated environment variable produces exactly `("",)`, which is the ordinary shape of a setting somebody forgot to fill in. Now refused at construction, matching CORS.__post_init__ one module over and for the same reason, that a service configured into nonsense should fail to start rather than serve whichever reading won. Relative prefixes are refused too: they can never match, so they are a statement that does nothing. A BARE startswith EXEMPTED NEIGHBOURS. Trusting /api/admin also exempted /api/administrator and /api/admin-fake. This is precisely the mistake EXEMPT_PATHS avoids by staying an exact match for the probes, which the previous commit's own comment said out loud before the prefix matcher went and made it. Matching is now on a segment boundary: equal to the prefix, or followed by a slash. THE MOUNT PREFIX BROKE IT ENTIRELY. scope["path"] carries the mount when conventions are installed on a mounted application, so a service at /service compared /service/api/admin/... against /api/admin and never matched. That one failed SAFELY, the exemption simply never applying, which is why it would have survived unnoticed. root_path is now stripped. By hand rather than with starlette's helper: get_route_path is not exported from starlette.routing and lives in a private module, so importing it would tie kit to an internal that moves between releases. The behaviour is the ASGI specification itself, so spelling it out is both stabler and readable. Both middlewares now ask one module the same question. Two copies of a startswith is how they drift, and a prefix exempt from CORS but not from the limiter is a service whose behaviour nobody can state in a sentence. --- src/kit/httpapi/_cors_middleware.py | 12 ++-- src/kit/httpapi/_install.py | 4 ++ src/kit/httpapi/_prefixes.py | 75 +++++++++++++++++++ src/kit/httpapi/_ratelimit_middleware.py | 6 +- tests/test_trusted_prefixes.py | 92 ++++++++++++++++++++++++ 5 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 src/kit/httpapi/_prefixes.py diff --git a/src/kit/httpapi/_cors_middleware.py b/src/kit/httpapi/_cors_middleware.py index 9474c8b..6167875 100644 --- a/src/kit/httpapi/_cors_middleware.py +++ b/src/kit/httpapi/_cors_middleware.py @@ -7,6 +7,8 @@ from starlette.middleware.cors import CORSMiddleware from starlette.types import ASGIApp, Receive, Scope, Send +from kit.httpapi._prefixes import under + class ScopedCORSMiddleware: """``CORSMiddleware``, skipped entirely under a trusted prefix. @@ -45,14 +47,10 @@ def __init__( ) -> None: self.app = app self.cors = CORSMiddleware(app, **options) - # A tuple, because `str.startswith` takes one and testing a tuple is a - # single call rather than a loop that a future edit turns into `any`. self.prefixes = prefixes async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: - if scope["type"] == "http" and self.prefixes: - path: str = scope.get("path", "") - if path.startswith(self.prefixes): - await self.app(scope, receive, send) - return + if scope["type"] == "http" and under(scope, self.prefixes): + await self.app(scope, receive, send) + return await self.cors(scope, receive, send) diff --git a/src/kit/httpapi/_install.py b/src/kit/httpapi/_install.py index f6dee13..324ef9d 100644 --- a/src/kit/httpapi/_install.py +++ b/src/kit/httpapi/_install.py @@ -24,6 +24,7 @@ RequestIDMiddleware, RequestLogMiddleware, ) +from kit.httpapi._prefixes import normalize_trusted_prefixes from kit.httpapi._probes import probe_router from kit.httpapi._ratelimit import RateLimit from kit.httpapi._ratelimit_middleware import RateLimitMiddleware @@ -88,6 +89,9 @@ def install_conventions( is exactly as reachable as it was, and whatever guards its router still does. """ log = logger or logging.getLogger("kit.httpapi") + # Checked before a single middleware is added, so a service configured + # into "exempt everything" fails to start rather than serving wide open. + trusted_prefixes = normalize_trusted_prefixes(trusted_prefixes) # Innermost first, because Starlette wraps in reverse. if rate_limit is not None: diff --git a/src/kit/httpapi/_prefixes.py b/src/kit/httpapi/_prefixes.py new file mode 100644 index 0000000..fd26a7d --- /dev/null +++ b/src/kit/httpapi/_prefixes.py @@ -0,0 +1,75 @@ +"""Which paths count as being under a trusted prefix. + +ONE MODULE BECAUSE TWO MIDDLEWARES MUST AGREE. The CORS wrapper and the rate +limiter both ask this question, and a prefix exempt from one and not the other is +a service whose behaviour nobody can state in a sentence. Duplicating a +`startswith` in both is how they drift. +""" + +from __future__ import annotations + +from starlette.types import Scope + + +def normalize_trusted_prefixes(prefixes: tuple[str, ...]) -> tuple[str, ...]: + """The prefixes, checked at construction, or a refusal. + + RAISES RATHER THAN DROPPING, matching `CORS.__post_init__` one module over + and for the same reason: this runs once, before the process serves anything, + so a service configured into nonsense fails to start instead of serving + whichever reading happened to win. + + THE EMPTY STRING IS THE ONE THAT MATTERS. Every path starts with it, so a + single empty entry exempts the entire application from both CORS and rate + limiting, silently, with a green deployment. That is not a hypothetical + typo: splitting an unset comma-separated environment variable produces + exactly `("",)`, which is the ordinary shape of a setting somebody forgot to + fill in. Dropping it quietly would leave the deployment believing it had + exempted something. + + A prefix must also be absolute. A relative one can never match a path and is + therefore a statement that does nothing, which is worth failing on for the + same reason: somebody wrote it expecting an effect. + """ + for prefix in prefixes: + if not prefix or not prefix.startswith("/"): + raise ValueError( + f"trusted_prefixes must be absolute paths, got {prefix!r}. " + "An empty prefix matches every path and would exempt the whole " + "application from CORS and rate limiting; a relative one matches " + "nothing and would exempt something the author expected it to." + ) + return prefixes + + +def under(scope: Scope, prefixes: tuple[str, ...]) -> bool: + """Whether this request is under one of the prefixes. + + ON A SEGMENT BOUNDARY, never a bare `startswith`. Trusting `/api/admin` must + not exempt `/api/administrator` or `/api/admin-fake`: those are different + routes that merely share an opening, and exempting them is the same mistake + `EXEMPT_PATHS` avoids by staying an exact match for the probes. A bare + `startswith` reads correct and hands an unrelated route the exemption. + + THE APP-RELATIVE PATH, not the raw one. `scope["path"]` carries the mount + prefix when conventions are installed on an application mounted under one, so + a service mounted at `/service` would compare `/service/admin/...` against + `/admin` and never match. That failure is silent and in the safe direction, + which is exactly why it would survive unnoticed: the exemption simply never + applies and nothing reports it. + + Stripping `root_path` by hand rather than borrowing starlette's helper. The + helper is not exported from `starlette.routing` and lives in a private module, + so importing it would tie this to an internal that can move between releases. + The behaviour is the ASGI specification itself, `root_path` is the mount point + and `path` includes it, so spelling it out is both stabler and readable. + """ + if not prefixes: + return False + path: str = scope.get("path", "") + root: str = scope.get("root_path", "") + if root and path.startswith(root): + # `or "/"` because stripping the mount from a request AT the mount leaves + # an empty string, and every prefix here is absolute. + path = path[len(root) :] or "/" + return any(path == prefix or path.startswith(f"{prefix}/") for prefix in prefixes) diff --git a/src/kit/httpapi/_ratelimit_middleware.py b/src/kit/httpapi/_ratelimit_middleware.py index 24f110d..c624c80 100644 --- a/src/kit/httpapi/_ratelimit_middleware.py +++ b/src/kit/httpapi/_ratelimit_middleware.py @@ -8,6 +8,7 @@ from starlette.types import ASGIApp, Receive, Scope, Send from kit.httpapi._envelope import RATE_LIMITED, error_response +from kit.httpapi._prefixes import under from kit.httpapi._ratelimit import ( SESSION_COOKIE, Limiter, @@ -81,12 +82,11 @@ def __init__( self.exempt_prefixes = exempt_prefixes async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: - path: str = scope.get("path", "") if ( scope["type"] != "http" or not self.enabled - or path in self.exempt_paths - or (bool(self.exempt_prefixes) and path.startswith(self.exempt_prefixes)) + or scope.get("path", "") in self.exempt_paths + or under(scope, self.exempt_prefixes) ): await self.app(scope, receive, send) return diff --git a/tests/test_trusted_prefixes.py b/tests/test_trusted_prefixes.py index 33f89bf..d7437e4 100644 --- a/tests/test_trusted_prefixes.py +++ b/tests/test_trusted_prefixes.py @@ -167,3 +167,95 @@ async def operator() -> dict[str, bool]: response = await get(app, f"{ADMIN}/v1/organizations", {"origin": ORIGIN}) assert "access-control-allow-origin" not in response.headers + + +class TestTheExemptionCannotBecomeOverbroad: + """Three ways a prefix exemption goes wrong, all found in review. + + Each one fails in the direction that does not announce itself: the first + silently exempts everything, the second silently exempts a neighbour, and the + third silently exempts nothing at all. + """ + + def test_an_empty_prefix_is_refused_at_construction(self) -> None: + """THE ONE THAT MATTERS. Every path starts with the empty string, so a + single empty entry exempts the whole application from both CORS and rate + limiting, with a green deployment and nothing in the logs. + + Not a hypothetical typo either: splitting an unset comma-separated + environment variable produces exactly `("",)`, which is the ordinary + shape of a setting somebody forgot to fill in. + """ + with pytest.raises(ValueError, match="absolute paths"): + app_with(trusted=("",)) + + def test_a_relative_prefix_is_refused_too(self) -> None: + """It can never match, so it is a statement that does nothing. Failing is + right for the same reason: somebody wrote it expecting an effect.""" + with pytest.raises(ValueError, match="absolute paths"): + app_with(trusted=("api/admin",)) + + async def test_a_neighbouring_route_is_not_exempt(self) -> None: + """Trusting `/api/admin` must not exempt `/api/admin-fake`. + + This is the same mistake `EXEMPT_PATHS` avoids by staying exact for the + probes, and the first version of this feature made it: a bare + `startswith` reads correct and hands an unrelated route the exemption. + """ + app = app_with(trusted=(ADMIN,)) + + @app.get("/api/admin-fake") + async def neighbour() -> dict[str, bool]: + return {"ok": True} + + response = await get(app, "/api/admin-fake", {"origin": ORIGIN}) + + assert response.headers["access-control-allow-origin"] == ORIGIN + + async def test_the_prefix_itself_is_exempt(self) -> None: + """The boundary rule is "equal, or followed by a slash". Without the + first half, trusting `/api/admin` would exempt everything under it and + not the collection route itself.""" + app = app_with(trusted=(ADMIN,)) + + @app.get(ADMIN) + async def root() -> dict[str, bool]: + return {"ok": True} + + response = await get(app, ADMIN, {"origin": ORIGIN}) + + assert "access-control-allow-origin" not in response.headers + + async def test_a_neighbouring_route_is_still_rate_limited(self) -> None: + """The same boundary, on the other middleware. One module answers this + question for both so they cannot drift.""" + app = app_with(trusted=(ADMIN,), limit=RateLimit(requests=1, window_seconds=60.0, burst=1)) + + @app.get("/api/administrator") + async def neighbour() -> dict[str, bool]: + return {"ok": True} + + first = await get(app, "/api/administrator") + second = await get(app, "/api/administrator") + + assert first.status_code == 200 + assert second.status_code == 429 + + +async def test_the_exemption_survives_being_mounted_under_a_prefix() -> None: + """`scope["path"]` carries the mount prefix, so a service mounted at + `/service` would compare `/service/api/admin/...` against `/api/admin` and + never match. + + That failure is silent and in the SAFE direction, which is exactly why it + would have survived: the exemption never applies, CORS and the limiter stay + on, and nothing reports that the feature did nothing. + """ + inner = app_with(trusted=(ADMIN,)) + outer = FastAPI() + outer.mount("/service", inner) + + response = await get(outer, f"/service{ADMIN}/v1/organizations", {"origin": ORIGIN}) + + assert response.status_code == 200 + assert "access-control-allow-origin" not in response.headers