Skip to content

Commit a6ab359

Browse files
chopratejasTejas Chopra
andauthored
fix(proxy): guard feedback endpoints and add CSRF checks to loopback writes (#3060)
## Description `#2927` brought eight telemetry/TOIN routes under `require_loopback`. Two structurally identical siblings 60 lines above them were missed: ``` GET /v1/feedback GET /v1/feedback/{tool_name} ``` Neither is an aggregate-counter endpoint. Their `common_queries` / `queried_fields` keys are built verbatim from agent search text — `event.query.lower()` at `headroom/cache/compression_feedback.py:311` — and up to 100 queries are retained per tool, keyed by real tool name. Under the shipped Docker default (`--host 0.0.0.0`) a LAN peer gets a 404 from `/v1/toin/patterns` and the query corpus from `/v1/feedback`. Separately, five mutating loopback-only routes had no CSRF guard. `require_loopback` cannot stop that attack: a remote page POSTing to a known `127.0.0.1` URL with `Content-Type: text/plain` is a CORS *simple* request, so there is no preflight, and the browser still sends the real loopback `Host` header — both of the guard's gates pass. Only `Origin` betrays the caller, and only `require_same_origin` inspects it. That guard already existed at `headroom/proxy/loopback_guard.py:219` and was applied solely to `/settings`. Closes #2927 (completes it — the original eight routes were already done). ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Changes Made - Added `Depends(_require_loopback)` to `/v1/feedback` and `/v1/feedback/{tool_name}`. - Stripped `common_queries` / `queried_fields` from both response bodies even on the guarded path, matching the whitelist discipline #2930 applied at `server.py:4909-4916`. - Added `_feedback_stats_without_query_text()` so the scrub happens at the HTTP boundary; `get_stats()` is unchanged and in-process compression decisions are untouched. - Added `Depends(_require_same_origin)` to `POST /stats/reset`, `/cache/clear`, `/v1/retrieve`, `/v1/telemetry/import`, `/admin/runtime-env`. ## Testing - [x] Unit tests pass - [x] Linting passes (ruff check + format) - [ ] Type checking passes (`uv run mypy headroom`) — not run - [x] New tests added for new functionality ### Test Output ```text $ .venv/bin/python -m pytest tests/test_proxy_loopback_gating.py -q 99 passed, 1 warning in 4.18s $ .venv/bin/python -m pytest tests/test_proxy_settings_endpoints.py tests/test_telemetry.py \ tests/test_proxy_cache_telemetry.py tests/test_proxy_telemetry_env.py tests/test_telemetry_context.py -q 101 passed, 1 warning in 3.67s $ .venv/bin/python -m pytest tests/test_critical_fixes.py tests/test_compression_store.py \ tests/test_toin_full_integration.py tests/test_ccr_feedback.py tests/test_critical_gaps.py \ tests/test_proxy_ccr.py tests/test_proxy_dashboard_stats_cache.py -q 168 passed, 4 skipped, 3 warnings in 13.18s $ .venv/bin/python -m ruff check headroom/proxy/server.py tests/test_proxy_loopback_gating.py All checks passed! ``` Against the parent commit (`git stash` of `server.py` only), all 14 new tests fail: ```text FAILED test_non_loopback_caller_gets_404[get-/v1/feedback] FAILED test_non_loopback_caller_gets_404[get-/v1/feedback/example] FAILED test_cross_origin_post_rejected[/stats/reset] FAILED test_cross_origin_post_rejected[/cache/clear] FAILED test_cross_origin_post_rejected[/v1/retrieve] FAILED test_cross_origin_post_rejected[/v1/telemetry/import] FAILED test_cross_origin_post_rejected[/admin/runtime-env] FAILED test_sandboxed_null_origin_post_rejected[...] (5 cases) FAILED test_feedback_stats_exclude_agent_query_text FAILED test_feedback_tool_detail_excludes_agent_query_text 14 failed, 85 passed ``` ## Real Behavior Proof - Environment: macOS 15 (darwin 25.4.0), Python 3.12.13, this branch, FastAPI `TestClient` against the real `create_app` proxy. - Exact command / steps: drive `/v1/feedback` with a feedback singleton whose `common_queries` contains `"find the customer api key rotation runbook"`, once from a non-loopback peer and once from a loopback peer; POST each of the five mutating routes with `Origin: https://attacker.example` and `Content-Type: text/plain`. - Observed result: non-loopback callers now receive 404 where they previously received 200 with the query corpus; on the loopback path the response no longer contains `common_queries`, `queried_fields`, or the substring `customer api key rotation`, while `retrieval_rate` still resolves to `0.25`. All five cross-origin POSTs return 403; the same requests with no `Origin`, or with `Origin: http://127.0.0.1`, are unaffected. - Not tested: a real browser issuing the cross-origin POST (the CORS simple-request shape is reproduced at the header level, not in a browser), and a live non-loopback deployment. ## Runtime Rollout Safety - Rollout-managed feature(s): none. - Minimum rollout channel: N/A. - Stable/default behavior changed: yes — `/v1/feedback*` now 404 for non-loopback callers and no longer return query text; five POST routes reject cross-origin browser callers. - Kill switch / disable path: none; these are security guards and are deliberately not configurable. - Unsafe override required: none. - Qualification impact: none. - Rollback path: revert this commit. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes ## Additional Notes `/stats` also calls `feedback.get_stats()` (`server.py:3896`) but only reads aggregate counters at `:4303-4311` and never emits query text — verified, and the reason the scrub is applied at the HTTP boundary rather than inside `get_stats()`. The five POST routes are strictly loopback-gated, so the trusted-dashboard wrapper `/settings` uses is unnecessary here; for a loopback caller that wrapper falls through to the same raw guard. No dashboard asset calls them, and the TypeScript SDK (`sdk/typescript/src/client.ts:322,443`) sends no `Origin` header, which the guard passes through unchanged. Co-authored-by: Tejas Chopra <tejas@Tejass-MacBook-Pro.local>
1 parent 96c25f5 commit a6ab359

2 files changed

Lines changed: 196 additions & 12 deletions

File tree

headroom/proxy/server.py

Lines changed: 61 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2438,6 +2438,35 @@ def _normalized_http_origin(value: str) -> tuple[str, str, int] | None:
24382438
return scheme, parsed.hostname.lower(), port
24392439

24402440

2441+
#: Feedback-pattern keys built verbatim from agent query text. They are useful
2442+
#: in-process for compression decisions but must never reach an HTTP response —
2443+
#: same privacy contract the TOIN endpoints were brought under in #2926/#2927.
2444+
_FEEDBACK_QUERY_TEXT_KEYS = ("common_queries", "queried_fields")
2445+
2446+
2447+
def _feedback_stats_without_query_text(stats: dict[str, Any]) -> dict[str, Any]:
2448+
"""Return ``stats`` with per-tool query text stripped from ``tool_patterns``.
2449+
2450+
Copies only the levels it edits; the aggregate counters are shared with the
2451+
caller's dict, which is fine because they are scalars.
2452+
"""
2453+
2454+
patterns = stats.get("tool_patterns")
2455+
if not isinstance(patterns, dict):
2456+
return stats
2457+
2458+
scrubbed: dict[str, Any] = {}
2459+
for name, pattern in patterns.items():
2460+
if isinstance(pattern, dict):
2461+
scrubbed[name] = {
2462+
key: value for key, value in pattern.items() if key not in _FEEDBACK_QUERY_TEXT_KEYS
2463+
}
2464+
else:
2465+
scrubbed[name] = pattern
2466+
2467+
return {**stats, "tool_patterns": scrubbed}
2468+
2469+
24412470
_is_known_websocket_callback_failure = is_known_websocket_callback_failure
24422471

24432472

@@ -3506,7 +3535,10 @@ async def debug_warmup():
35063535
payload["runtime"] = _runtime_payload()
35073536
return JSONResponse(status_code=200, content=payload)
35083537

3509-
@app.post("/admin/runtime-env", dependencies=[Depends(_require_loopback)])
3538+
@app.post(
3539+
"/admin/runtime-env",
3540+
dependencies=[Depends(_require_loopback), Depends(_require_same_origin)],
3541+
)
35103542
async def admin_runtime_env(request: Request):
35113543
"""Hot-reload live env knobs (the output-shaper family, the ast-grep
35123544
read threshold) without restarting the proxy.
@@ -4447,7 +4479,10 @@ async def stats_lifetime(request: Request):
44474479
payload["persistence"] = {**persistence, "error": None}
44484480
return payload
44494481

4450-
@app.post("/stats/reset", dependencies=[Depends(_require_loopback)])
4482+
@app.post(
4483+
"/stats/reset",
4484+
dependencies=[Depends(_require_loopback), Depends(_require_same_origin)],
4485+
)
44514486
async def stats_reset():
44524487
"""Reset in-memory proxy stats for local test/debug isolation."""
44534488
await proxy.metrics.reset_runtime()
@@ -4584,7 +4619,10 @@ async def debug_memory():
45844619
report = tracker.get_report()
45854620
return report.to_dict()
45864621

4587-
@app.post("/cache/clear", dependencies=[Depends(_require_loopback)])
4622+
@app.post(
4623+
"/cache/clear",
4624+
dependencies=[Depends(_require_loopback), Depends(_require_same_origin)],
4625+
)
45884626
async def clear_cache():
45894627
"""Clear the response cache.
45904628
@@ -4600,7 +4638,10 @@ async def clear_cache():
46004638
return {"status": "cache disabled"}
46014639

46024640
# CCR (Compress-Cache-Retrieve) endpoints
4603-
@app.post("/v1/retrieve", dependencies=[Depends(_require_loopback)])
4641+
@app.post(
4642+
"/v1/retrieve",
4643+
dependencies=[Depends(_require_loopback), Depends(_require_same_origin)],
4644+
)
46044645
async def ccr_retrieve(request: Request):
46054646
"""Retrieve original content from CCR compression cache.
46064647
@@ -4669,21 +4710,25 @@ async def ccr_stats():
46694710
],
46704711
}
46714712

4672-
@app.get("/v1/feedback")
4713+
@app.get("/v1/feedback", dependencies=[Depends(_require_loopback)])
46734714
async def ccr_feedback():
46744715
"""Get CCR feedback loop statistics and learned patterns.
46754716
46764717
This endpoint exposes the feedback loop's learned patterns for monitoring
46774718
and debugging. It shows:
46784719
- Per-tool retrieval rates (high = compress less aggressively)
4679-
- Common search queries per tool
4680-
- Queried fields (suggest what to preserve)
4720+
- Aggregate compression/retrieval counters per tool
46814721
46824722
Use this to understand how well compression is working and whether
46834723
the feedback loop is adjusting appropriately.
4724+
4725+
Loopback-guarded and query-text free for the same reason as the
4726+
telemetry and TOIN endpoints (#2926/#2927): ``common_queries`` and
4727+
``queried_fields`` are built verbatim from agent search queries, so
4728+
they stay out of the response even on the guarded path.
46844729
"""
46854730
feedback = get_compression_feedback()
4686-
stats = feedback.get_stats()
4731+
stats = _feedback_stats_without_query_text(feedback.get_stats())
46874732
return {
46884733
"feedback": stats,
46894734
"hints_example": {
@@ -4702,12 +4747,15 @@ async def ccr_feedback():
47024747
},
47034748
}
47044749

4705-
@app.get("/v1/feedback/{tool_name}")
4750+
@app.get("/v1/feedback/{tool_name}", dependencies=[Depends(_require_loopback)])
47064751
async def ccr_feedback_for_tool(tool_name: str):
47074752
"""Get compression hints for a specific tool.
47084753
47094754
Returns feedback-based hints that would be used for compressing
47104755
this tool's output.
4756+
4757+
Loopback-guarded, and the pattern block excludes ``common_queries``
4758+
and ``queried_fields`` — both are raw agent query text (#2926/#2927).
47114759
"""
47124760
feedback = get_compression_feedback()
47134761
hints = feedback.get_compression_hints(tool_name)
@@ -4730,8 +4778,6 @@ async def ccr_feedback_for_tool(tool_name: str):
47304778
"retrieval_rate": patterns.retrieval_rate if patterns else 0.0,
47314779
"full_retrieval_rate": patterns.full_retrieval_rate if patterns else 0.0,
47324780
"search_rate": patterns.search_rate if patterns else 0.0,
4733-
"common_queries": list(patterns.common_queries.keys())[:10] if patterns else [],
4734-
"queried_fields": list(patterns.queried_fields.keys())[:10] if patterns else [],
47354781
}
47364782
if patterns
47374783
else None,
@@ -4777,7 +4823,10 @@ async def telemetry_export():
47774823
telemetry = get_telemetry_collector()
47784824
return telemetry.export_stats()
47794825

4780-
@app.post("/v1/telemetry/import", dependencies=[Depends(_require_loopback)])
4826+
@app.post(
4827+
"/v1/telemetry/import",
4828+
dependencies=[Depends(_require_loopback), Depends(_require_same_origin)],
4829+
)
47814830
async def telemetry_import(request: Request):
47824831
"""Import telemetry data from another source.
47834832

tests/test_proxy_loopback_gating.py

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
from fastapi.testclient import TestClient
1616

1717
from headroom.cache.backends import InMemoryBackend
18+
from headroom.cache.compression_feedback import CompressionHints
1819
from headroom.cache.compression_store import get_compression_store, reset_compression_store
1920
from headroom.proxy.loopback_guard import is_ip_literal_host_header
2021
from headroom.proxy.server import ProxyConfig, create_app
@@ -30,6 +31,11 @@
3031
("get", "/v1/toin/stats"),
3132
("get", "/v1/toin/patterns"),
3233
("get", "/v1/toin/pattern/example"),
34+
# #2927 guarded the eight telemetry/TOIN routes the issue enumerated but
35+
# left these two siblings open, and their payload carries the same raw
36+
# agent query text (``common_queries``, built from ``event.query``).
37+
("get", "/v1/feedback"),
38+
("get", "/v1/feedback/example"),
3339
]
3440

3541

@@ -119,6 +125,135 @@ def export_patterns(self):
119125
}
120126

121127

128+
# Mutating routes reachable from loopback. `require_loopback` cannot stop a
129+
# remote page from POSTing to a known 127.0.0.1 URL: a "simple" cross-origin
130+
# request (Content-Type: text/plain carrying JSON) skips preflight, and the
131+
# browser still sends the real loopback Host header. Only `Origin` betrays the
132+
# attacker, and only `require_same_origin` inspects it.
133+
CSRF_GUARDED = [
134+
"/stats/reset",
135+
"/cache/clear",
136+
"/v1/retrieve",
137+
"/v1/telemetry/import",
138+
"/admin/runtime-env",
139+
]
140+
141+
142+
@pytest.mark.parametrize("path", CSRF_GUARDED)
143+
def test_cross_origin_post_rejected(path: str) -> None:
144+
resp = _loopback_client().post(
145+
path,
146+
headers={"Origin": "https://attacker.example", "Content-Type": "text/plain"},
147+
content="{}",
148+
)
149+
assert resp.status_code == 403, resp.text
150+
151+
152+
@pytest.mark.parametrize("path", CSRF_GUARDED)
153+
def test_sandboxed_null_origin_post_rejected(path: str) -> None:
154+
# A sandboxed iframe or file:// page sends the opaque literal "null".
155+
resp = _loopback_client().post(
156+
path,
157+
headers={"Origin": "null", "Content-Type": "text/plain"},
158+
content="{}",
159+
)
160+
assert resp.status_code == 403, resp.text
161+
162+
163+
@pytest.mark.parametrize("path", CSRF_GUARDED)
164+
def test_loopback_origin_post_allowed(path: str) -> None:
165+
# The local dashboard is same-origin on loopback and must keep working.
166+
resp = _loopback_client().post(
167+
path,
168+
headers={"Origin": "http://127.0.0.1"},
169+
json={},
170+
)
171+
assert resp.status_code != 403, resp.text
172+
173+
174+
@pytest.mark.parametrize("path", CSRF_GUARDED)
175+
def test_originless_post_allowed(path: str) -> None:
176+
# CLI tools and the TypeScript SDK send no Origin header at all; the guard
177+
# must pass them through or it breaks every non-browser client.
178+
resp = _loopback_client().post(path, json={})
179+
assert resp.status_code != 403, resp.text
180+
181+
182+
def _feedback_with_query_text():
183+
"""A feedback singleton whose patterns carry raw agent query text."""
184+
185+
class FakePattern:
186+
total_compressions = 8
187+
total_retrievals = 2
188+
retrieval_rate = 0.25
189+
full_retrieval_rate = 0.1
190+
search_rate = 0.5
191+
common_queries = {"find the customer api key rotation runbook": 3}
192+
queried_fields = {"internal_field_name": 2}
193+
194+
class FakeFeedback:
195+
def get_stats(self):
196+
return {
197+
"total_compressions": 8,
198+
"total_retrievals": 2,
199+
"global_retrieval_rate": 0.25,
200+
"tools_tracked": 1,
201+
"tool_patterns": {
202+
"Grep": {
203+
"compressions": 8,
204+
"retrievals": 2,
205+
"retrieval_rate": 0.25,
206+
"full_rate": 0.1,
207+
"search_rate": 0.5,
208+
"common_queries": ["find the customer api key rotation runbook"],
209+
"queried_fields": ["internal_field_name"],
210+
}
211+
},
212+
}
213+
214+
def get_compression_hints(self, tool_name):
215+
# The real implementation is annotated ``-> CompressionHints`` and
216+
# always returns one, so the double must too.
217+
return CompressionHints()
218+
219+
def get_all_patterns(self):
220+
return {"Grep": FakePattern()}
221+
222+
return FakeFeedback()
223+
224+
225+
def test_feedback_stats_exclude_agent_query_text(monkeypatch: pytest.MonkeyPatch) -> None:
226+
monkeypatch.setattr(
227+
"headroom.proxy.server.get_compression_feedback",
228+
_feedback_with_query_text,
229+
)
230+
response = _loopback_client().get("/v1/feedback")
231+
232+
assert response.status_code == 200
233+
pattern = response.json()["feedback"]["tool_patterns"]["Grep"]
234+
assert "common_queries" not in pattern
235+
assert "queried_fields" not in pattern
236+
# The aggregate counters the endpoint exists to expose still survive.
237+
assert pattern["retrieval_rate"] == 0.25
238+
assert "customer api key rotation" not in response.text
239+
240+
241+
def test_feedback_tool_detail_excludes_agent_query_text(monkeypatch: pytest.MonkeyPatch) -> None:
242+
monkeypatch.setattr(
243+
"headroom.proxy.server.get_compression_feedback",
244+
_feedback_with_query_text,
245+
)
246+
response = _loopback_client().get("/v1/feedback/Grep")
247+
248+
assert response.status_code == 200
249+
pattern = response.json()["pattern"]
250+
assert "common_queries" not in pattern
251+
assert "queried_fields" not in pattern
252+
assert pattern["retrieval_rate"] == 0.25
253+
assert "customer api key rotation" not in response.text
254+
assert "internal_field_name" not in response.text
255+
256+
122257
# CCR data endpoints — cached session content, gated to 404 off-loopback (#1227).
123258
def test_stats_lifetime_route_uses_dashboard_metadata_access_policy(
124259
monkeypatch: pytest.MonkeyPatch,

0 commit comments

Comments
 (0)