Skip to content

fix(api): make network allowlists configurable via env vars#255

Merged
haofeif merged 1 commit into
awslabs:mainfrom
call-me-ram:fix/issue-149-configurable-ws-allowlist
May 23, 2026
Merged

fix(api): make network allowlists configurable via env vars#255
haofeif merged 1 commit into
awslabs:mainfrom
call-me-ram:fix/issue-149-configurable-ws-allowlist

Conversation

@call-me-ram

Copy link
Copy Markdown
Contributor

Summary

cao-server ships with three hardcoded loopback-only allowlists:

  • ALLOWED_HOSTS — Host header allowlist for TrustedHostMiddleware
  • CORS_ORIGINS — browser origins permitted by CORS
  • The WebSocket PTY client_host check in terminal_ws

That posture is correct for the default install. It breaks two real workflows that the linked issues describe:

The constants.py block above ALLOWED_HOSTS already documented this as a "future extension point" mentioning CAO_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 new WS_ALLOWED_CLIENTS constant now extend their built-in defaults with comma-separated entries from CAO_ALLOWED_HOSTS, CAO_CORS_ORIGINS, and CAO_WS_ALLOWED_CLIENTS respectively.
  • api/main.py — the WebSocket terminal_ws endpoint reads WS_ALLOWED_CLIENTS instead 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_HOSTS extension in install_service._allowed_download_hosts(), so contributors familiar with that path will recognise it.

Security posture

  • Defaults are unchanged. An operator that does not set any of the three env vars sees the same loopback-only behavior as before.
  • Extends, never replaces. Setting CAO_WS_ALLOWED_CLIENTS=172.17.0.1 keeps 127.0.0.1, ::1, and localhost allowed. This is intentional — replace-semantics (as in _allowed_download_hosts()) is easy to misuse by locking out loopback.
  • The WebSocket endpoint remains unauthenticated, so the docs section calls out that the env var should only list IPs the operator actually trusts.

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.
  • Full unit suite — 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_connect and call the endpoint coroutine directly with an AsyncMock websocket. Starlette's TestClient maps both pre-accept and post-accept close into the same WebSocketDenialResponse, 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: accept is called or not, and close.code is exactly 4003 or 4004. The existing test_websocket_rejects_non_loopback integration 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).

@codecov-commenter

codecov-commenter commented May 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@dec0347). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #255   +/-   ##
=======================================
  Coverage        ?   92.30%           
=======================================
  Files           ?       68           
  Lines           ?     6157           
  Branches        ?        0           
=======================================
  Hits            ?     5683           
  Misses          ?      474           
  Partials        ?        0           
Flag Coverage Δ
unittests 92.30% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 new WS_ALLOWED_CLIENTS from CAO_ALLOWED_HOSTS, CAO_CORS_ORIGINS, and CAO_WS_ALLOWED_CLIENTS.
  • Updated the terminal PTY WebSocket endpoint to use WS_ALLOWED_CLIENTS and 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.

@haofeif

haofeif commented May 23, 2026

Copy link
Copy Markdown
Contributor

@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!

@haofeif haofeif left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.
@call-me-ram call-me-ram force-pushed the fix/issue-149-configurable-ws-allowlist branch from 4311195 to 57b12be Compare May 23, 2026 07:44
@call-me-ram

Copy link
Copy Markdown
Contributor Author

Thanks @haofeif! Rebased on main with #256's changes pulled in. The only conflict was a one-line import in test/api/test_terminals.py (from typing import Dict from #256) — kept it since it's still used by the TERM tests. Full unit suite (1720 tests) green on 3.10/3.11/3.12, black/isort/mypy clean on the changed files. Let me know if you'd like anything else tweaked.

@haofeif haofeif self-assigned this May 23, 2026
@haofeif haofeif merged commit 6cf5101 into awslabs:main May 23, 2026
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] WebSocket PTY rejects connections from Docker bridge IPs

4 participants