fix(api): make network allowlists configurable via env vars#255
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
=======================================
Coverage ? 92.30%
=======================================
Files ? 68
Lines ? 6157
Branches ? 0
=======================================
Hits ? 5683
Misses ? 474
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR makes cao-server’s loopback-only network allowlists configurable via environment variables while preserving the existing secure-by-default behavior (defaults unchanged; env vars extend, not replace). This addresses Docker/container workflows where clients arrive from a bridge IP and deployments where the UI origin/port differs from the baked-in defaults.
Changes:
- Added a shared env-var parsing helper and extended
ALLOWED_HOSTS,CORS_ORIGINS, and newWS_ALLOWED_CLIENTSfromCAO_ALLOWED_HOSTS,CAO_CORS_ORIGINS, andCAO_WS_ALLOWED_CLIENTS. - Updated the terminal PTY WebSocket endpoint to use
WS_ALLOWED_CLIENTSand clarified the rejection reason. - Documented the new server network settings and added/expanded tests covering env overrides and WebSocket allowlist behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/cli_agent_orchestrator/constants.py |
Introduces _split_env_list() and wires env-var extensions into CORS/Host/WebSocket allowlists. |
src/cli_agent_orchestrator/api/main.py |
Switches WebSocket client gating to WS_ALLOWED_CLIENTS and updates close reason text. |
test/test_constants.py |
Adds env-override reload tests ensuring defaults remain intact and overrides extend correctly. |
test/api/test_terminals.py |
Adds direct-call async tests validating allowlist pass/fail behavior and correct close codes. |
docs/settings.md |
Documents the three env vars, provides a Docker example, and notes WebSocket security implications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@call-me-ram the #256 is merged, can you please try to resolve conflicts for this PR, happy to approve it once it is rebased? many thanks for your contribution! |
Add CAO_ALLOWED_HOSTS, CAO_CORS_ORIGINS, and CAO_WS_ALLOWED_CLIENTS env vars (comma-separated). Each one extends -- not replaces -- the built-in host header, CORS, and WebSocket PTY allowlists. The WebSocket attach endpoint previously rejected every non-loopback client, which makes the web UI unusable when cao-server is run inside a container and the host browser arrives via a Docker bridge IP such as 172.17.0.1. The TrustedHostMiddleware host list and CORS origins were hardcoded for the same reason. constants.py already carried a "future extension point" comment naming CAO_ALLOWED_HOSTS; this wires that up and adds matching CORS / WebSocket overrides. Defaults are unchanged, so operators that do not set the env vars see the same loopback-only posture as before. The override pattern mirrors the existing CAO_PROFILE_ALLOWED_HOSTS used by install_service. Resolves awslabs#149. Partially addresses awslabs#151.
4311195 to
57b12be
Compare
|
Thanks @haofeif! Rebased on main with #256's changes pulled in. The only conflict was a one-line import in |
Summary
cao-serverships with three hardcoded loopback-only allowlists:ALLOWED_HOSTS— Host header allowlist forTrustedHostMiddlewareCORS_ORIGINS— browser origins permitted by CORSclient_hostcheck interminal_wsThat posture is correct for the default install. It breaks two real workflows that the linked issues describe:
cao-serverinside a container with--host 0.0.0.0. The host browser connects via a Docker bridge IP (e.g.172.17.0.1), which fails the hardcoded loopback check on the WebSocket endpoint, so the web UI cannot attach to any terminal.CORS_ORIGINSlist rejects the browser'sOriginheader.The
constants.pyblock aboveALLOWED_HOSTSalready documented this as a "future extension point" mentioningCAO_ALLOWED_HOSTS— this PR wires that up and adds matching CORS and WebSocket overrides.Changes
constants.py— new_split_env_list()helper.ALLOWED_HOSTS,CORS_ORIGINS, and a newWS_ALLOWED_CLIENTSconstant now extend their built-in defaults with comma-separated entries fromCAO_ALLOWED_HOSTS,CAO_CORS_ORIGINS, andCAO_WS_ALLOWED_CLIENTSrespectively.api/main.py— the WebSocketterminal_wsendpoint readsWS_ALLOWED_CLIENTSinstead of its inline tuple. The close reason is generalised from "restricted to localhost" to "restricted to allowed clients".docs/settings.md— new Server Network Settings section documenting the three env vars, with an example for the Docker scenario and a security note about the unauthenticated WebSocket endpoint.The override pattern matches the existing
CAO_PROFILE_ALLOWED_HOSTSextension ininstall_service._allowed_download_hosts(), so contributors familiar with that path will recognise it.Security posture
CAO_WS_ALLOWED_CLIENTS=172.17.0.1keeps127.0.0.1,::1, andlocalhostallowed. This is intentional — replace-semantics (as in_allowed_download_hosts()) is easy to misuse by locking out loopback.Test plan
uv run pytest test/test_constants.py test/api/test_terminals.py -v— 54/54 pass, including 5 new env-var-override tests and 2 new direct-call WebSocket allowlist tests.uv run pytest test/ --ignore=test/e2e -m "not integration"— 1713 passed, 1 skipped on Python 3.10, 3.11, and 3.12 (the CI matrix).uv run black --check src/ test/— clean.uv run isort --check-only src/ test/— clean.uv run mypy src/cli_agent_orchestrator/constants.py src/cli_agent_orchestrator/api/main.py— clean (no new findings on the changed modules).The new WebSocket tests intentionally bypass
TestClient.websocket_connectand call the endpoint coroutine directly with anAsyncMockwebsocket. Starlette'sTestClientmaps both pre-accept and post-accept close into the sameWebSocketDenialResponse, so it cannot distinguish a 4003 allowlist rejection from a 4004 metadata-not-found close after accept. Driving the coroutine directly keeps the assertion precise:acceptis called or not, andclose.codeis exactly 4003 or 4004. The existingtest_websocket_rejects_non_loopbackintegration test (which only asserts that some exception fires) is left in place as a smoke test.Closes #149.
Partially addresses #151 (the WebSocket and CORS gaps).