fix(proxy): allow settings routes for trusted gateway/dashboard clients#2491
fix(proxy): allow settings routes for trusted gateway/dashboard clients#2491Parideboy wants to merge 2 commits into
Conversation
/settings, /settings/schema, /settings/apply, and /dashboard/settings were gated by _require_loopback, which checks request.client.host directly and 404s for any non-loopback peer. Behind a reverse-proxy or container gateway, request.client.host is the gateway's IP, so the settings UI 404ed unconditionally even with HEADROOM_PROXY_TRUSTED_GATEWAY_CIDRS/HEADROOM_PROXY_TRUSTED_DASHBOARD_CLIENT_CIDRS configured — a trust chain /stats and /stats-lifetime already use via _request_can_view_dashboard_metadata. Add _require_loopback_or_trusted_dashboard_client, which reuses that same trust chain, and swap it in for _require_loopback on the five settings/dashboard routes only. All other loopback-only admin/debug routes are untouched. Fixes headroomlabs-ai#2466 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR governanceThis PR follows the template and is marked ready for human review. |
JerrettDavis
left a comment
There was a problem hiding this comment.
This is close, but the mutating settings routes are still broken for the browser/dashboard case the PR is trying to enable.
The new _require_loopback_or_trusted_dashboard_client dependency lets a trusted non-loopback dashboard client reach POST /settings and POST /settings/apply, but those routes still also depend on require_same_origin. That helper only accepts loopback origins, so a real same-origin dashboard POST from the trusted remote/IP-literal dashboard host is rejected:
client = TestClient(
app,
base_url="http://100.82.0.2:8787",
client=("100.90.0.5", 12345),
)
client.post("/settings", json={"values": {}}, headers={"Origin": "http://100.82.0.2:8787"})
# 403 {"detail":"cross-origin request rejected"}I verified locally on this branch that the same trusted client gets 200 with no Origin header, but gets 403 with the same-origin Origin header above. Browsers commonly include Origin on POST/fetch requests, so the settings UI can still fail behind the trusted gateway even though the GET routes work.
Please make the CSRF/provenance check for these settings writes match the same trusted-dashboard origin policy used by _request_can_view_dashboard_metadata (while still rejecting foreign origins), and add regression coverage for a trusted dashboard client posting with Origin: http://<dashboard-host>:<port> to both /settings and /settings/apply.
Other checked state: PR is mergeable and visible CI is green. I did not run the full file after finding this blocker; the reproduction above is from the latest checked-out source.
require_same_origin only accepted an Origin naming a loopback host, so a trusted-gateway dashboard client (allowed by the GET routes) still got 403 on POST /settings and POST /settings/apply since browsers send Origin on POST. Add _require_same_origin_or_trusted_dashboard_client, which additionally accepts a non-loopback caller whose Origin matches its own Host header and who is already a CIDR-trusted dashboard client. Loopback callers keep the stricter loopback-only origin check. Addresses JerrettDavis's review on headroomlabs-ai#2491. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@JerrettDavis Good catch, fixed in 2d14a8a. Added Swapped this in for Added regression coverage in
|
JerrettDavis
left a comment
There was a problem hiding this comment.
Latest changes look good. I re-ran the targeted settings/loopback gating suite locally (python -m pytest tests/test_proxy_loopback_gating.py tests/test_proxy_settings_endpoints.py -q: 78 passed) and reviewed the trusted-dashboard same-origin path. The settings routes now match the existing dashboard metadata trust model without widening the unrelated admin/debug endpoints.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
/settings,/settings/schema,/settings/apply, and/dashboard/settingswere gated by_require_loopback, which checksrequest.client.hostdirectly and 404s for any non-loopback caller. When headroom-proxy runs behind a reverse-proxy/gateway (e.g. in a container),request.client.hostis the gateway's IP, so these routes 404 unconditionally — even withHEADROOM_PROXY_TRUSTED_GATEWAY_CIDRS/HEADROOM_PROXY_TRUSTED_DASHBOARD_CLIENT_CIDRSconfigured, a trust chain/statsand/stats-lifetimealready use.Fixes #2466.
Type of Change
Changes Made
_require_loopback_or_trusted_dashboard_clientdependency inheadroom/proxy/server.py, reusing the existing_request_can_view_dashboard_metadatatrust chain (loopback check, IP-literal Host header check, same-origin check, trusted-gateway CIDR check)._require_loopbackon exactly five routes:/settings/schema,GET /settings,POST /settings,POST /settings/apply,/dashboard/settings. All other loopback-only admin/debug routes (/admin/*,/debug/*,/cache/clear,/v1/retrieve*) are untouched.tests/test_proxy_loopback_gating.py: non-loopback without trusted CIDR still 404s, loopback still allowed, trusted-gateway dashboard client is now allowed, and CIDR mismatch still 404s.Testing
Real Behavior Proof
python -m pytest tests/test_proxy_loopback_gating.py -qafter adding parametrized tests that setHEADROOM_PROXY_TRUSTED_DASHBOARD_CLIENT_CIDRSand hit/settings,/settings/schema,/dashboard/settingsfrom a simulated gateway-forwarded peer IPReview Readiness