mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-10 14:27:00 -04:00
3076e32172
## Description The Headroom dashboard tunnels `GET /favicon.ico` requests to the wrapped upstream provider instead of serving its own. No route matched `/favicon.ico` in `headroom/proxy/server.py`, so the request fell through to the catch-all passthrough route (`headroom/providers/proxy_routes.py:994-1026`) registered by `register_provider_routes(app, proxy)`, and got forwarded to whichever LLM backend the proxy is wrapping — burning a real upstream request (and possibly failing auth) for a browser's automatic favicon fetch while viewing `/dashboard`. Closes #1787 ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `headroom/proxy/server.py`: added a `GET /favicon.ico` route returning `Response(status_code=204)`, registered next to the existing `/dashboard` route — i.e. before `register_provider_routes(app, proxy)` (line ~4184) registers the passthrough catch-all, so it takes priority. - `tests/test_proxy_handler_helpers.py`: `_PassthroughRequest.url.path` was hardcoded to `/favicon.ico` as a generic "goes to passthrough" example, which encoded the bug as expected behavior. Changed to `/some/other/path` so the passthrough-helper test no longer depends on favicon requests going upstream. - `tests/test_proxy_favicon_route.py` (new): regression test spinning up the real FastAPI app via `create_app`/`TestClient`, asserting `GET /favicon.ico` returns 204 and `proxy.handle_passthrough` is never called. - `CHANGELOG.md`: added an entry under `## Unreleased` / `### Fixed`. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ python -m pytest tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py -q 28 passed $ python -m pytest tests/test_proxy_healthchecks.py tests/test_proxy_passthrough_integration.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py -q 41 passed, 19 skipped $ python -m ruff check headroom/proxy/server.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py All checks passed! $ python -m ruff format --check headroom/proxy/server.py tests/test_proxy_favicon_route.py tests/test_proxy_handler_helpers.py 3 files already formatted $ python -m mypy headroom/proxy/server.py (no errors) ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.13, local checkout, `python -m pytest`/`ruff`/`mypy` run directly (no `uv` available in this shell). - Exact command / steps: `python -m pytest tests/test_proxy_favicon_route.py -v` — this test builds the real proxy app with `create_app(ProxyConfig(...))`, wraps `client.app.state.proxy.handle_passthrough` with a mock, then issues `client.get("/favicon.ico")` via a real `TestClient` request through the full FastAPI routing stack (not a unit-level call of the handler function directly). - Observed result: response status is `204`, and `handle_passthrough` (the function that forwards to the upstream provider) is asserted `not_called()` — confirming the request is now intercepted before reaching the catch-all passthrough route, and does not tunnel to the wrapped provider. - Not tested: did not manually run `headroom wrap <provider>` end-to-end and open a real browser tab to `/dashboard` to visually confirm the favicon icon in the tab (the fix returns 204/no-icon rather than a real bundled `.ico` — browsers handle this fine, but the visual "no more broken/upstream favicon request" experience wasn't screenshotted). The FastAPI-level test above exercises the actual routing/dispatch path this bug lived in. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation (N/A — no user-facing docs describe dashboard route internals beyond CHANGELOG) - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated CHANGELOG.md where applicable ## Screenshots (if applicable) N/A — server-side route change, no UI change. ## Additional Notes Deliberately kept the fix minimal: no `StaticFiles` mount or general static-asset serving system was added, since a single favicon route doesn't warrant that abstraction. No real `.ico` binary asset was bundled either — a `204 No Content` response is sufficient for browsers and avoids maintaining a binary asset in the repo; this can be upgraded to serve a real branded icon later if desired. Co-authored-by: JD Davis <mxjerrett@gmail.com>
31 lines
925 B
Python
31 lines
925 B
Python
from unittest.mock import AsyncMock, patch
|
|
|
|
import pytest
|
|
|
|
pytest.importorskip("fastapi")
|
|
pytest.importorskip("httpx")
|
|
|
|
from fastapi.testclient import TestClient
|
|
|
|
from headroom.proxy.server import ProxyConfig, create_app
|
|
|
|
|
|
def test_favicon_is_served_locally_and_never_reaches_passthrough(monkeypatch) -> None:
|
|
"""GH #1787: /favicon.ico must not be tunneled to the upstream provider."""
|
|
monkeypatch.setenv("HEADROOM_SKIP_UPSTREAM_CHECK", "1")
|
|
config = ProxyConfig(
|
|
optimize=False,
|
|
cache_enabled=False,
|
|
rate_limit_enabled=False,
|
|
cost_tracking_enabled=False,
|
|
)
|
|
app = create_app(config)
|
|
|
|
with TestClient(app) as client:
|
|
with patch.object(
|
|
client.app.state.proxy, "handle_passthrough", new=AsyncMock()
|
|
) as passthrough:
|
|
response = client.get("/favicon.ico")
|
|
|
|
assert response.status_code == 204
|
|
passthrough.assert_not_called()
|