From 48aa0bfed25240c7bdbd9667b793129c4618efe3 Mon Sep 17 00:00:00 2001 From: Robert-Jan Huijsman <22160949+rjhuijsman@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:30:46 +0000 Subject: [PATCH] `reboot`: bind an `app_internal` route's context to its endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An HTTP route that opts into an app-internal `ExternalContext` via `app_internal=True` must receive one for every request it handles, whatever the shape of its path, and no other route may receive one. Before this change, `PythonWebFramework.HTTP._api_route` recorded an app-internal route's path string in a set, and the middleware that puts the context on `request.state` looked the request's path up in that set before routing. A route whose path carries parameters (e.g. `/__/things/{id}`) therefore never matched its own requests — the set held the template while requests carried concrete paths — so such a route silently got the external context it had not asked for and failed later on the trusted call it was meant to make. Pattern matching the paths in the middleware would not have been a fix either: the middleware runs before routing and cannot know which endpoint Starlette will dispatch to, so a `{param}` pattern would have granted the app-internal context to every ordinary route under the same prefix as well. The grant is now bound to the endpoint rather than to the path: `APIRoute` carries an `app_internal` flag, and when the server process registers such a route it attaches a FastAPI route-level dependency that replaces the request's context with an app-internal one. That dependency runs after Starlette has dispatched to the endpoint and before the endpoint itself, so it reaches exactly that endpoint's requests. The middleware now always installs the external context. Handlers keep reading `request.state.reboot_external_context`, and existing `app_internal=True` routes need no change. The new `tests/reboot/aio/http_app_internal_test.py` covers a static `app_internal=True` route, a parameterized one (which failed before this change), and a route under the same prefix registered without `app_internal` (which must keep its external context). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D8vHXG2KLHDUruv8642AXY --- reboot/aio/http.py | 68 ++++++++++------ tests/reboot/aio/BUILD.bazel | 16 ++++ tests/reboot/aio/http_app_internal_test.py | 93 ++++++++++++++++++++++ 3 files changed, 153 insertions(+), 24 deletions(-) create mode 100644 tests/reboot/aio/http_app_internal_test.py diff --git a/reboot/aio/http.py b/reboot/aio/http.py index e4d91d2ec..13fc538ac 100644 --- a/reboot/aio/http.py +++ b/reboot/aio/http.py @@ -66,6 +66,11 @@ class APIRoute: path: str kwargs: dict endpoint: Callable[..., Any] + # Whether the endpoint is handed an *app-internal* + # `ExternalContext` (one carrying the application's + # `caller_id`) instead of the external one. See the DANGER note + # in `HTTP._api_route`. + app_internal: bool = False @dataclass(kw_only=True, frozen=True) class Mount: @@ -88,11 +93,6 @@ class HTTP: def __init__(self): self._api_routes: list[PythonWebFramework.APIRoute] = [] self._mounts: list[PythonWebFramework.Mount] = [] - # Exact request paths whose handlers receive an *app-internal* - # context (one that can call app-internal-only servicers) - # instead of the usual external one, because they opted in via - # `app_internal=True`. See the DANGER note in `_api_route`. - self._app_internal_paths: set[str] = set() def _api_route(self, path: str, **kwargs): # `app_internal` is our own kwarg, not one of FastAPI's, so we @@ -100,6 +100,10 @@ def _api_route(self, path: str, **kwargs): # handler is given an *app-internal* `ExternalContext` (one # carrying the application's `caller_id`, able to call # app-internal-only servicers) instead of the external one. + # The grant is bound to the endpoint: it reaches exactly the + # requests that Starlette dispatches to this handler, however + # its path is spelled (a template such as `/things/{id}` + # included). # # DANGER: an app-internal context bypasses authorizers, so a # route that gets one can make trusted in-app calls on behalf @@ -109,8 +113,7 @@ def _api_route(self, path: str, **kwargs): # only after the authorization code has been exchanged and # validated. Never set it on a route that acts on unvalidated # request input. - if kwargs.pop("app_internal", False): - self._app_internal_paths.add(path) + app_internal: bool = kwargs.pop("app_internal", False) # TODO: add type annotations for `endpoint` so that what # we take in is exactly what we return. @@ -127,6 +130,7 @@ def decorator(endpoint): path=path, endpoint=endpoint, kwargs=kwargs, + app_internal=app_internal, ) ) return endpoint @@ -271,26 +275,34 @@ def app_internal_external_context_from_request( caller_id=CallerID(application_id=application_id), ) + def app_internal_external_context_dependency(request: Request): + # A route-level dependency that hands its endpoint an + # app-internal context. FastAPI resolves it after Starlette + # has dispatched the request to that endpoint and before the + # endpoint runs, so the grant reaches exactly the requests + # the endpoint handles. + request.state.reboot_external_context = ( + app_internal_external_context_from_request(request) + ) + fastapi = FastAPI() @fastapi.middleware("http") async def external_context_middleware(request: Request, call_next): - # Most routes get an *external* context (no `caller_id`): an - # HTTP handler serves untrusted external traffic, so handing it - # a caller that bypasses authorizers would let external - # requests escalate to trusted in-app calls. Those routes must - # do their own end-user auth. Only routes that opted in via - # `app_internal=True` get an *app-internal* context instead — - # see the DANGER note on `HTTP._api_route`. We namespace this - # on `request.state` so other middleware doesn't clash. - if request.url.path in self._http._app_internal_paths: - request.state.reboot_external_context = ( - app_internal_external_context_from_request(request) - ) - else: - request.state.reboot_external_context = ( - external_context_from_request(request) - ) + # Every request gets an *external* context (no `caller_id`): + # an HTTP handler serves untrusted external traffic, so + # handing it a caller that bypasses authorizers would let + # external requests escalate to trusted in-app calls. Routes + # must do their own end-user auth. Only routes that opted in + # via `app_internal=True` get an *app-internal* context + # instead, which `app_internal_external_context_dependency` + # puts in place once the request has been routed to such an + # endpoint — see the DANGER note on `HTTP._api_route`. We + # namespace this on `request.state` so other middleware + # doesn't clash. + request.state.reboot_external_context = ( + external_context_from_request(request) + ) return await call_next(request) @@ -304,10 +316,18 @@ async def external_context_middleware(request: Request, call_next): ) for api_route in self._http._api_routes: + kwargs = dict(api_route.kwargs) + if api_route.app_internal: + # Ahead of any dependencies the route declared itself, + # so those already see the app-internal context. + kwargs["dependencies"] = [ + Depends(app_internal_external_context_dependency), + *(kwargs.get("dependencies") or []), + ] fastapi.add_api_route( api_route.path, api_route.endpoint, - **api_route.kwargs, + **kwargs, ) config = uvicorn.Config( diff --git a/tests/reboot/aio/BUILD.bazel b/tests/reboot/aio/BUILD.bazel index 069fd4dec..75322339b 100644 --- a/tests/reboot/aio/BUILD.bazel +++ b/tests/reboot/aio/BUILD.bazel @@ -1,3 +1,4 @@ +load("@rbt_pypi//:requirements.bzl", "requirement") load("@rules_python//python:defs.bzl", "py_test") py_test( @@ -61,6 +62,21 @@ py_test( ], ) +py_test( + name = "http_app_internal_test_py", + timeout = "short", + srcs = [":http_app_internal_test.py"], + main = "http_app_internal_test.py", + deps = [ + "//reboot/aio:applications_py", + "//reboot/aio:external_py", + "//reboot/aio:http_py", + "//reboot/aio:tests_py", + "//tests/reboot:greeter_servicers_py", + requirement("httpx"), + ], +) + py_test( name = "caller_id_test_py", timeout = "short", diff --git a/tests/reboot/aio/http_app_internal_test.py b/tests/reboot/aio/http_app_internal_test.py new file mode 100644 index 000000000..62b62b77c --- /dev/null +++ b/tests/reboot/aio/http_app_internal_test.py @@ -0,0 +1,93 @@ +""" +Tests which `ExternalContext` a custom HTTP route is handed: a route +registered with `app_internal=True` gets an app-internal one (carrying +the application's `caller_id`), whatever the shape of its path, while a +route registered without it gets an external one even when it lives +under the same path prefix as an app-internal route. +""" + +import httpx +import unittest +from reboot.aio.applications import Application +from reboot.aio.external import ExternalContext +from reboot.aio.http import InjectExternalContext +from reboot.aio.tests import Reboot +from tests.reboot.greeter_servicers import MyGreeterServicer + +# Generous per-request HTTP timeout: each request crosses a full Reboot +# cluster plus a local Envoy, which can take a while on a loaded CI +# runner, and Bazel's test timeout remains the backstop against a hang. +_HTTP_TIMEOUT_SECONDS = 30.0 + + +def _describe(context: ExternalContext) -> dict[str, bool]: + """The kind of context a handler was given: an app-internal context + carries the application's `caller_id`, an external one carries + none.""" + return {"app_internal": context.caller_id is not None} + + +class HTTPAppInternalTest(unittest.IsolatedAsyncioTestCase): + + async def asyncSetUp(self) -> None: + self.rbt = Reboot() + await self.rbt.start() + + application = Application(servicers=[MyGreeterServicer]) + + # Starlette dispatches to the first route whose path matches, + # so the static routes are registered ahead of the + # parameterized one that would otherwise capture them. + @application.http.get("/__/test/static", app_internal=True) + def static(context: ExternalContext = InjectExternalContext): + return _describe(context) + + @application.http.get("/__/test/plain") + def plain(context: ExternalContext = InjectExternalContext): + return _describe(context) + + @application.http.get("/__/test/{item}", app_internal=True) + def parameterized( + item: str, + context: ExternalContext = InjectExternalContext, + ): + return _describe(context) + + await self.rbt.up(application) + + async def asyncTearDown(self) -> None: + await self.rbt.stop() + + async def _get(self, path: str) -> dict[str, bool]: + async with httpx.AsyncClient(timeout=_HTTP_TIMEOUT_SECONDS) as client: + response = await client.get(self.rbt.http_localhost_url(path)) + self.assertEqual(200, response.status_code, response.text) + return response.json() + + async def test_static_app_internal_route(self) -> None: + self.assertEqual( + {"app_internal": True}, + await self._get("/__/test/static"), + ) + + async def test_parameterized_app_internal_route(self) -> None: + # The route's path is a template (`/__/test/{item}`); the + # request's path is concrete, so an exact path lookup would + # never match it. + self.assertEqual( + {"app_internal": True}, + await self._get("/__/test/some-item"), + ) + + async def test_plain_route_under_app_internal_prefix(self) -> None: + # `/__/test/plain` also matches the `/__/test/{item}` pattern, + # so an app-internal grant keyed on a path pattern rather than + # on the dispatched endpoint would leak to this route. + self.assertEqual( + {"app_internal": False}, + await self._get("/__/test/plain"), + ) + + +if __name__ == "__main__": + unittest.main()