From 5af04364c9cd1d95339d2fbc08f5ae5a032659eb Mon Sep 17 00:00:00 2001 From: 9qeklajc Date: Mon, 24 Aug 2026 01:08:33 +0200 Subject: [PATCH] Restrict proxy forwarding to an exact method/path allowlist --- routstr/core/settings.py | 11 ++ routstr/proxy.py | 148 ++++++++++++++---- .../integration/test_proxy_post_endpoints.py | 81 ++++++++-- tests/unit/test_proxy_path_allowlist.py | 145 ++++++++++++++--- .../test_proxy_tinfoil_attestation_routing.py | 18 +-- 5 files changed, 325 insertions(+), 78 deletions(-) diff --git a/routstr/core/settings.py b/routstr/core/settings.py index c02c6030..53e71a74 100644 --- a/routstr/core/settings.py +++ b/routstr/core/settings.py @@ -101,6 +101,12 @@ class Settings(BaseSettings): # Network cors_origins: list[str] = Field(default_factory=lambda: ["*"], env="CORS_ORIGINS") + # Comma-separated METHOD:path pairs adding to the proxy's canonical + # endpoint allowlist, e.g. "POST:v1/rerank,GET:batches". Only for upstreams + # exposing an endpoint outside the OpenAI-compatible set; each addition + # widens what the provider credential can be spent against, so wildcards + # and prefixes are not supported. + proxy_extra_allowed_paths: str = Field(default="", env="PROXY_EXTRA_ALLOWED_PATHS") tor_proxy_url: str = Field(default="socks5://127.0.0.1:9050", env="TOR_PROXY_URL") providers_refresh_interval_seconds: int = Field( default=0, env="PROVIDERS_REFRESH_INTERVAL_SECONDS" @@ -192,6 +198,11 @@ SECRET_FIELDS = frozenset({"admin_password", "nsec"}) # neither store nor shadow them; env is always authoritative. ENV_ONLY_FIELDS = frozenset( { + # Widening the proxy's reachable upstream surface is a deployment + # decision, not a runtime toggle: it changes what the provider + # credential can be spent against. Keeping it env-only also lets the + # proxy parse it once at import without going stale. + "proxy_extra_allowed_paths", "database_pool_size", "database_max_overflow", "database_pool_timeout", diff --git a/routstr/proxy.py b/routstr/proxy.py index c367d9a2..5feff279 100644 --- a/routstr/proxy.py +++ b/routstr/proxy.py @@ -26,6 +26,7 @@ from .core.db import ( ) from .core.exceptions import UpstreamError from .core.not_found import build_not_found_response +from .core.settings import settings from .payment.helpers import ( calculate_discounted_max_cost, check_token_balance, @@ -221,27 +222,93 @@ async def refresh_model_maps_periodically() -> None: ) -_API_PATH_PREFIXES = ( - "v1/", - "responses", - "chat/", - "completions", - "models", - "embeddings", - "audio/", - "images/", - "moderations", - "providers", - "tee/", - "attestation", -) +# Canonical endpoints this proxy will forward, keyed by the path with any +# leading "v1/" and trailing slash removed, mapped to the methods allowed on +# each. The provider credential is attached during forwarding, so endpoint +# permission has to come from this table rather than from the client-supplied +# path: an upstream's key-management, organization, or billing routes live +# under the same origin and must never be reachable through the proxy. +_ALLOWED_ENDPOINTS: dict[str, frozenset[str]] = { + "chat/completions": frozenset({"POST"}), + "completions": frozenset({"POST"}), + "responses": frozenset({"POST"}), + "messages": frozenset({"POST"}), + "embeddings": frozenset({"POST"}), + "moderations": frozenset({"POST"}), + "rerank": frozenset({"POST"}), + "audio/speech": frozenset({"POST"}), + "audio/transcriptions": frozenset({"POST"}), + "audio/translations": frozenset({"POST"}), + "images/generations": frozenset({"POST"}), + "images/edits": frozenset({"POST"}), + "images/variations": frozenset({"POST"}), + "models": frozenset({"GET"}), + "attestation": frozenset({"GET"}), + "tee/attestation": frozenset({"GET"}), +} -# Split the allowlist by spelling so bare tokens anchor to a path segment. -# A slash-terminated prefix ("v1/") is already segment-anchored under -# startswith. A bare token ("models") must match a whole segment — exactly or -# followed by "/" — so "modelsdump" / "attestationadmin" cannot slip through. -_API_SLASH_PREFIXES = tuple(p for p in _API_PATH_PREFIXES if p.endswith("/")) -_API_BARE_PREFIXES = tuple(p for p in _API_PATH_PREFIXES if not p.endswith("/")) +_ALLOWED_METHODS = frozenset({"GET", "POST"}) + + +def _canonical_api_path(path: str) -> str: + """Reduce a request path to its allowlist key. + + OpenAI-style clients reach the same endpoint with or without the ``v1/`` + prefix and with or without a trailing slash, so both spellings collapse to + one key. Callers must screen the path with + :func:`_is_ambiguously_spelled_path` first — this function assumes the path + has no dot segments, empty segments, or encoded separators left to resolve. + """ + core = path[:-1] if path.endswith("/") else path + if core.startswith("v1/"): + core = core[len("v1/") :] + return core + + +def _parse_extra_allowed_endpoints(raw: str) -> dict[str, frozenset[str]]: + """Parse operator-configured additions to the endpoint allowlist. + + Deployments whose provider exposes an endpoint outside the canonical set + opt in explicitly with ``PROXY_EXTRA_ALLOWED_PATHS``, a comma-separated + list of ``METHOD:path`` pairs (e.g. ``POST:v1/rerank,GET:batches``). Every + entry must name one concrete method and one unambiguous path; wildcards + and bare prefixes are deliberately unsupported, so widening the proxy's + reach is always a per-endpoint decision. Malformed entries are dropped + with a warning rather than silently widening or narrowing the surface. + """ + extra: dict[str, frozenset[str]] = {} + for entry in raw.split(","): + entry = entry.strip() + if not entry: + continue + method, separator, endpoint = entry.partition(":") + method = method.strip().upper() + endpoint = endpoint.strip() + if not separator or method not in _ALLOWED_METHODS or not endpoint: + logger.warning( + "Ignoring malformed PROXY_EXTRA_ALLOWED_PATHS entry", + extra={"entry": entry}, + ) + continue + if _is_ambiguously_spelled_path(endpoint): + logger.warning( + "Ignoring ambiguously spelled PROXY_EXTRA_ALLOWED_PATHS entry", + extra={"entry": entry}, + ) + continue + if any(character in endpoint for character in "*?["): + # Refuse glob syntax outright. Kept as a literal endpoint name it + # would never match a real request, so the operator would think + # they had widened the proxy when they had not. + logger.warning( + "Ignoring wildcard PROXY_EXTRA_ALLOWED_PATHS entry; " + "list each endpoint explicitly", + extra={"entry": entry}, + ) + continue + key = _canonical_api_path(endpoint) + extra[key] = extra.get(key, frozenset()) | {method} + return extra def _is_ambiguously_spelled_path(path: str) -> bool: @@ -267,20 +334,35 @@ def _is_ambiguously_spelled_path(path: str) -> bool: return "%2e" in lowered or "%2f" in lowered or "%5c" in lowered -def _forwarding_allowed(path: str, is_ehbp: bool) -> bool: - """Gate which paths may reach an upstream at all. +_EXTRA_ALLOWED_ENDPOINTS = _parse_extra_allowed_endpoints( + settings.proxy_extra_allowed_paths +) - The provider credential is attached during forwarding, so an unknown path - must never be forwarded on the caller's say-so. A path must resolve to a - known API prefix; EHBP requests are identified by header and carry their own - encrypted contract. Endpoint permission is derived from this allowlist, not - from the client-supplied path. + +def _allowed_methods_for(endpoint: str) -> frozenset[str]: + """Return the methods allowed on a canonical endpoint, empty if unknown.""" + methods = _ALLOWED_ENDPOINTS.get(endpoint, frozenset()) + methods |= _EXTRA_ALLOWED_ENDPOINTS.get(endpoint, frozenset()) + return methods + + +def _forwarding_allowed(path: str, method: str) -> bool: + """Gate which method/path pairs may reach an upstream at all. + + The provider credential is attached during forwarding, so an unknown + endpoint must never be forwarded on the caller's say-so. The path is + reduced to its canonical form and looked up in the endpoint table; there is + no prefix match, so a known prefix no longer carries an unknown endpoint + (``v1/organization/api_keys`` is rejected even though ``v1/`` is familiar). + + EHBP requests are gated by the same table. Their body is opaque to the + proxy, which is a reason to constrain the destination more tightly, not to + trust the caller's path: the encrypted contract covers the body, never the + endpoint the credential is spent against. """ - if is_ehbp: - return True - if path.startswith(_API_SLASH_PREFIXES): - return True - return any(path == p or path.startswith(p + "/") for p in _API_BARE_PREFIXES) + if method not in _ALLOWED_METHODS: + return False + return method in _allowed_methods_for(_canonical_api_path(path)) @proxy_router.api_route("/{path:path}", methods=["GET", "POST"], response_model=None) @@ -310,7 +392,7 @@ async def _proxy( headers = dict(request.headers) is_ehbp = "ehbp-encapsulated-key" in headers - if not _forwarding_allowed(path, is_ehbp): + if not _forwarding_allowed(path, request.method): return build_not_found_response(request, path) is_responses_api = path.startswith("v1/responses") or path.startswith("responses") diff --git a/tests/integration/test_proxy_post_endpoints.py b/tests/integration/test_proxy_post_endpoints.py index 9765ffd4..99bdc071 100644 --- a/tests/integration/test_proxy_post_endpoints.py +++ b/tests/integration/test_proxy_post_endpoints.py @@ -318,6 +318,43 @@ async def test_authenticated_post_to_unknown_path_is_rejected( assert response.status_code == 404 +@pytest.mark.integration +@pytest.mark.asyncio +@pytest.mark.parametrize( + "bad_path", + [ + # Unambiguously spelled, under a prefix the proxy serves, but not an + # endpoint it offers. These are real upstream routes that manage keys, + # org membership, and billing on the same origin as inference. + "v1/organization/api_keys", + "v1/api_keys", + "v1/billing/usage", + "v1/admin/keys", + # An id segment is honoured one level deep, and only where an endpoint + # takes one at all. + "v1/chat/completions/abc", + "v1/models/gpt-4/secret", + ], +) +async def test_known_prefix_does_not_carry_an_unknown_endpoint( + authenticated_client: AsyncClient, bad_path: str +) -> None: + """A familiar prefix must not be a passport for the rest of the origin. + + Nothing here is traversal-shaped, so the spelling screen lets it by; only + the endpoint allowlist stops it. Forwarding raises if the guard misses, + so a clean 404 also proves the credential never left.""" + with patch( + "routstr.upstream.base.BaseUpstreamProvider.forward_request", + AsyncMock(side_effect=AssertionError("must not forward unknown endpoint")), + ): + response = await authenticated_client.post( + f"/{bad_path}", + json={"model": "gpt-3.5-turbo", "messages": []}, + ) + assert response.status_code == 404 + + @pytest.mark.integration @pytest.mark.asyncio async def test_get_unknown_path_is_rejected(integration_client: AsyncClient) -> None: @@ -328,21 +365,37 @@ async def test_get_unknown_path_is_rejected(integration_client: AsyncClient) -> @pytest.mark.integration @pytest.mark.asyncio -async def test_ambiguous_path_is_rejected_before_ehbp_exemption( - integration_client: AsyncClient, +@pytest.mark.parametrize( + "bad_path", + [ + # Percent-encoded so the traversal survives the client to the server, + # which decodes it to "v1/../admin" before routing. + "v1/%2e%2e/admin", + # Well-spelled, so only the endpoint allowlist can stop it. + "v1/organization/api_keys", + "anything/encrypted", + ], +) +async def test_ehbp_request_is_gated_by_the_endpoint_allowlist( + integration_client: AsyncClient, bad_path: str ) -> None: - """The ambiguous-spelling screen runs before the EHBP header exemption, so - an EHBP request cannot smuggle a traversal path past it.""" - # Percent-encoded so the traversal survives the client to the server, which - # decodes it to "v1/../admin" before routing. - response = await integration_client.post( - "/v1/%2e%2e/admin", - content=b"encrypted", - headers={ - "ehbp-encapsulated-key": "x", - "x-routstr-model": "gpt-4", - }, - ) + """EHBP hides the body from the proxy, not the destination. + + The encrypted contract covers the request body; it says nothing about which + endpoint the provider credential gets spent against, so an EHBP request is + screened and allowlisted exactly like any other.""" + with patch( + "routstr.proxy.forward_ehbp_request", + AsyncMock(side_effect=AssertionError("must not forward unknown endpoint")), + ): + response = await integration_client.post( + f"/{bad_path}", + content=b"encrypted", + headers={ + "ehbp-encapsulated-key": "x", + "x-routstr-model": "gpt-4", + }, + ) assert response.status_code == 404 diff --git a/tests/unit/test_proxy_path_allowlist.py b/tests/unit/test_proxy_path_allowlist.py index a98679d5..33a2d610 100644 --- a/tests/unit/test_proxy_path_allowlist.py +++ b/tests/unit/test_proxy_path_allowlist.py @@ -2,8 +2,9 @@ An authenticated POST used to be forwarded for ANY path, so a caller could reach arbitrary or traversal-shaped upstream endpoints with the provider credential -attached. The proxy now rejects ambiguous path spellings for every method and -requires a known API prefix before anything is forwarded. +attached. The proxy now rejects ambiguous path spellings for every method, then +requires the method/path pair to name a canonical endpoint. A familiar prefix is +no longer enough: "v1/organization/api_keys" is refused just like "internal/admin". """ from __future__ import annotations @@ -18,6 +19,7 @@ import pytest # noqa: E402 from routstr.proxy import ( # noqa: E402 _forwarding_allowed, _is_ambiguously_spelled_path, + _parse_extra_allowed_endpoints, ) @@ -63,37 +65,138 @@ def test_canonical_paths_are_allowed(path: str) -> None: def test_unknown_paths_are_not_forwarded() -> None: # The credential is attached during forwarding, so an unknown endpoint must # never be forwarded on the caller's say-so. - assert _forwarding_allowed("internal/admin", is_ehbp=False) is False - assert _forwarding_allowed("secret-endpoint", is_ehbp=False) is False + assert _forwarding_allowed("internal/admin", "POST") is False + assert _forwarding_allowed("secret-endpoint", "POST") is False @pytest.mark.parametrize( "path", [ - "modelsdump", # bare token "models" must not match a longer segment + "modelsdump", # "models" must not match a longer segment "attestationadmin", "providers-secret", "embeddingsx", "completions-internal", ], ) -def test_bare_prefix_does_not_match_a_longer_segment(path: str) -> None: - assert _forwarding_allowed(path, is_ehbp=False) is False +def test_endpoint_name_does_not_match_a_longer_segment(path: str) -> None: + assert _forwarding_allowed(path, "POST") is False + assert _forwarding_allowed(path, "GET") is False -def test_known_prefixes_are_forwarded() -> None: - assert _forwarding_allowed("v1/chat/completions", is_ehbp=False) is True - assert _forwarding_allowed("chat/completions", is_ehbp=False) is True - # Bare tokens match a whole segment: exactly or followed by "/". - assert _forwarding_allowed("models", is_ehbp=False) is True - assert _forwarding_allowed("models/gpt-4", is_ehbp=False) is True - assert _forwarding_allowed("embeddings", is_ehbp=False) is True - assert _forwarding_allowed("attestation", is_ehbp=False) is True +@pytest.mark.parametrize( + "path", + [ + # A familiar prefix must not carry an unknown endpoint. These are real + # upstream routes that manage keys, org membership, and billing. + "v1/organization/api_keys", + "v1/api_keys", + "v1/admin/keys", + "v1/billing/usage", + "v1/files", + "v1/batches", + "chat/internal", + "audio/internal", + "images/internal", + "tee/keys", + # No endpoint takes a trailing id segment; a resource id never widens + # the reachable surface. + "models/gpt-4", + "models/gpt-4/secret", + "chat/completions/abc", + ], +) +def test_known_prefix_does_not_carry_an_unknown_endpoint(path: str) -> None: + assert _forwarding_allowed(path, "POST") is False + assert _forwarding_allowed(path, "GET") is False -def test_ehbp_bypasses_prefix_gate_by_header() -> None: - # Documents a deliberate exemption: EHBP is identified by header and carries - # its own encrypted contract, so the prefix gate does not apply. The - # ambiguous-spelling screen still runs on EHBP paths (see the ordering test - # in the integration suite). - assert _forwarding_allowed("anything/encrypted", is_ehbp=True) is True +@pytest.mark.parametrize( + ("path", "method"), + [ + ("v1/chat/completions", "POST"), + ("chat/completions", "POST"), + ("v1/chat/completions/", "POST"), + ("completions", "POST"), + ("v1/responses", "POST"), + ("v1/messages", "POST"), + ("v1/embeddings", "POST"), + ("moderations", "POST"), + ("audio/transcriptions", "POST"), + ("images/generations", "POST"), + ("models", "GET"), + ("attestation", "GET"), + ("tee/attestation", "GET"), + ], +) +def test_canonical_endpoints_are_forwarded(path: str, method: str) -> None: + assert _forwarding_allowed(path, method) is True + + +@pytest.mark.parametrize( + ("path", "method"), + [ + ("chat/completions", "GET"), # billed endpoints are POST-only + ("v1/embeddings", "GET"), + ("models", "POST"), # read-only endpoints are GET-only + ("attestation", "POST"), + ("v1/chat/completions", "DELETE"), # never routed here, refused anyway + ("v1/chat/completions", "PUT"), + ], +) +def test_method_must_match_the_endpoint(path: str, method: str) -> None: + assert _forwarding_allowed(path, method) is False + + +def test_operator_additions_are_parsed_per_endpoint() -> None: + parsed = _parse_extra_allowed_endpoints("POST:v1/rerank, GET:batches ,post:audio/x") + assert parsed == { + "rerank": frozenset({"POST"}), # the "v1/" prefix collapses like any path + "batches": frozenset({"GET"}), + "audio/x": frozenset({"POST"}), + } + + +def test_operator_additions_may_grant_two_methods_on_one_endpoint() -> None: + assert _parse_extra_allowed_endpoints("POST:batches,GET:batches") == { + "batches": frozenset({"POST", "GET"}) + } + + +@pytest.mark.parametrize( + "raw", + [ + "", + " ", + "v1/rerank", # no method + "POST:", # no path + ":v1/rerank", # empty method + "DELETE:v1/rerank", # method the proxy never routes + "POST:*", # wildcards are deliberately unsupported + "POST:v1/*", + "POST:../secret", # ambiguous spellings are screened here too + "POST:v1//rerank", + "POST:%2e%2e/secret", + ], +) +def test_malformed_operator_additions_widen_nothing(raw: str) -> None: + assert _parse_extra_allowed_endpoints(raw) == {} + + +def test_operator_additions_are_env_only() -> None: + # The proxy parses this once at import, so a persisted or admin-API-writable + # value would be read but never take effect. Keeping it env-only also means + # widening the reachable upstream surface takes a deploy. + from routstr.core.settings import ENV_ONLY_FIELDS + + assert "proxy_extra_allowed_paths" in ENV_ONLY_FIELDS + + +def test_ehbp_is_gated_by_the_same_allowlist() -> None: + # EHBP hides the request body from the proxy, which is a reason to constrain + # the destination more tightly rather than to trust the caller's path: the + # encrypted contract covers the body, never the endpoint the provider + # credential is spent against. + assert _forwarding_allowed("anything/encrypted", "POST") is False + assert _forwarding_allowed("v1/organization/api_keys", "POST") is False + assert _forwarding_allowed("v1/chat/completions", "POST") is True diff --git a/tests/unit/test_proxy_tinfoil_attestation_routing.py b/tests/unit/test_proxy_tinfoil_attestation_routing.py index cd335f3b..c367a049 100644 --- a/tests/unit/test_proxy_tinfoil_attestation_routing.py +++ b/tests/unit/test_proxy_tinfoil_attestation_routing.py @@ -103,13 +103,14 @@ async def test_attestation_trailing_slash_routes_directly_to_tinfoil( @pytest.mark.parametrize( - ("path", "expected_status"), + "path", [ - # Valid `attestation` segment but not the exact attestation route: - # fails model validation (empty body -> unknown model) before auth. - ("attestation/foo", 400), - # Not a known path segment at all: rejected at the edge before routing. - ("attestationjunk", 404), + # A valid `attestation` segment is not the exact attestation route, and + # `attestation` takes no id segment, so the endpoint allowlist rejects + # it at the edge rather than letting it reach model/auth handling. + "attestation/foo", + # Not a known endpoint at all: rejected at the edge before routing. + "attestationjunk", ], ) @pytest.mark.asyncio @@ -117,7 +118,6 @@ async def test_non_attestation_prefix_does_not_bypass_authentication( monkeypatch: pytest.MonkeyPatch, proxy_app: FastAPI, path: str, - expected_status: int, ) -> None: tinfoil = MagicMock() tinfoil.provider_type = "tinfoil" @@ -130,9 +130,7 @@ async def test_non_attestation_prefix_does_not_bypass_authentication( ) as client: response = await client.get(f"/{path}") - assert response.status_code == expected_status - if expected_status == 400: - assert response.json()["error"]["type"] == "invalid_model" + assert response.status_code == 404 tinfoil.forward_get_request.assert_not_awaited()