mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
a6ab359a5d
## 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>
|
||
|---|---|---|
| .. | ||
| audit | ||
| backends | ||
| cache | ||
| capture | ||
| ccr | ||
| cli | ||
| compression | ||
| dashboard | ||
| evals | ||
| graph | ||
| image | ||
| install | ||
| integrations | ||
| learn | ||
| mcp_registry | ||
| memory | ||
| models | ||
| observability | ||
| perf | ||
| pricing | ||
| providers | ||
| proxy | ||
| relevance | ||
| reporting | ||
| storage | ||
| subscription | ||
| telemetry | ||
| testing | ||
| tokenizers | ||
| transforms | ||
| __init__.py | ||
| _ort.py | ||
| _subprocess.py | ||
| _version.py | ||
| agent_savings.py | ||
| binaries.py | ||
| cli.py | ||
| client.py | ||
| compress.py | ||
| config.py | ||
| context_tool_cleanup.py | ||
| copilot_auth.py | ||
| copilot_linux_secret.py | ||
| copilot_macos_keychain.py | ||
| exceptions.py | ||
| fsutil.py | ||
| hooks.py | ||
| offline.py | ||
| onnx_runtime.py | ||
| parser.py | ||
| paths.py | ||
| pipeline.py | ||
| py.typed | ||
| release_version.py | ||
| rollout.py | ||
| savings_ledger.py | ||
| settings_store.py | ||
| shared_context.py | ||
| tokenizer.py | ||
| tools.json | ||
| update_check.py | ||
| utils.py | ||