fix(computer-server): stop exposing an unauthenticated server to the network by default (#1892) - #1899
Conversation
…network by default (trycua#1892) By default the server bound 0.0.0.0 with auth OFF whenever CONTAINER_NAME wasn't set (local mode). So anyone on your LAN — or any website you happened to visit — could run shell commands, read/write files, and open a PTY shell. Now, Local mode now binds 127.0.0.1 If you really want a public --host in local mode, you now have to opt in with CUA_ALLOW_INSECURE=1 Added an Origin check on /ws, /cmd and /pty so a random web page can't drive your local server.
|
@ashutoshjoshi1 is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds two security controls to ChangesSecure-by-default networking
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CrossSiteOriginGuard
participant FastAPI
participant Server
rect rgba(255, 100, 100, 0.5)
Note over Browser,Server: Startup — bind-host resolution
Server->>Server: resolve_bind_host(args.host)
alt local mode, no CONTAINER_NAME
Server->>Server: bind to 127.0.0.1
else CUA_ALLOW_INSECURE=1 or CONTAINER_NAME set
Server->>Server: bind to requested_host or 0.0.0.0
else insecure request in local mode
Server->>Server: raise InsecureBindError → exit
end
end
rect rgba(100, 100, 255, 0.5)
Note over Browser,FastAPI: Runtime — cross-site origin guard
Browser->>CrossSiteOriginGuard: Request to /cmd, /ws, /pty
CrossSiteOriginGuard->>CrossSiteOriginGuard: Parse Origin header
alt No Origin or loopback
CrossSiteOriginGuard->>FastAPI: Forward request
FastAPI->>Browser: Normal response
else Cross-site or null Origin (HTTP)
CrossSiteOriginGuard->>Browser: 403 JSON error
else Cross-site or null Origin (WebSocket)
CrossSiteOriginGuard->>Browser: WS close 1008
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds secure-by-default networking protections to prevent accidental public exposure of an unauthenticated server and to block cross-site browser requests from driving sensitive endpoints.
Changes:
- Introduces
resolve_bind_host(andInsecureBindError) to default binds to loopback in local/unauthenticated mode and require explicit opt-in for public interfaces. - Adds
CrossSiteOriginGuardASGI middleware to reject cross-site browser requests for shell/file/PTY surfaces while leaving/mcpand/statusuntouched. - Adds unit tests covering both bind-host resolution and cross-site origin guarding behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libs/python/computer-server/tests/test_secure_bind.py | New tests for bind-host fail-closed behavior and cross-site origin guard behavior. |
| libs/python/computer-server/computer_server/main.py | Adds CrossSiteOriginGuard middleware and wires it into the ASGI app. |
| libs/python/computer-server/computer_server/cli.py | Implements secure bind-host resolution and updates CLI defaults/logging to use it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/python/computer-server/tests/test_secure_bind.py (2)
106-110: 💤 Low valueConsider adding IPv6 loopback origin test.
The parametrized test covers
localhostand127.0.0.1but not the IPv6 loopback::1. Adding this would ensure IPv6 loopback origins are correctly allowed.💡 Suggested addition
- `@pytest.mark.parametrize`("origin", ["http://localhost:3000", "http://127.0.0.1:8080"]) + `@pytest.mark.parametrize`("origin", ["http://localhost:3000", "http://127.0.0.1:8080", "http://[::1]:8080"]) async def test_loopback_origin_passes_through(self, origin):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/python/computer-server/tests/test_secure_bind.py` around lines 106 - 110, The test_loopback_origin_passes_through method parametrizes over only IPv4 loopback addresses (localhost and 127.0.0.1) but lacks coverage for IPv6 loopback. Add an IPv6 loopback origin (formatted as http://[::1]:PORT) to the parametrize decorator's origin list to ensure IPv6 loopback origins are correctly handled by the guard function.
48-55: ⚡ Quick winConsider adding test for empty string host.
Given the potential security concern with empty string in
_LOOPBACK_HOSTS, adding a test case for""would help document the expected behavior and catch regressions.💡 Suggested test addition
`@pytest.mark.parametrize`("host", ["127.0.0.1", "localhost", "::1"]) def test_explicit_loopback_allowed_in_local_mode(self, clean_env, host): assert resolve_bind_host(host) == host + def test_empty_string_host_defaults_to_loopback_in_local_mode(self, clean_env): + # Empty string should be treated as "no host specified" and default to loopback + # (not pass through as-is, which would bind to all interfaces) + result = resolve_bind_host("") + assert result == "127.0.0.1", "Empty host should default to loopback, not pass through" + `@pytest.mark.parametrize`("host", ["0.0.0.0", "192.168.1.10", "::"]) def test_explicit_public_host_refused_in_local_mode(self, clean_env, host): with pytest.raises(InsecureBindError): resolve_bind_host(host)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/python/computer-server/tests/test_secure_bind.py` around lines 48 - 55, Add a test case for empty string host to verify the behavior of resolve_bind_host("") function, which will help document the expected behavior and prevent regressions related to security concerns with empty strings in _LOOPBACK_HOSTS. You can add this either as a separate test method or include it as a parameter in the existing parametrized tests (test_explicit_loopback_allowed_in_local_mode or test_explicit_public_host_refused_in_local_mode, depending on whether empty string should be allowed or rejected).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/python/computer-server/computer_server/cli.py`:
- Around line 13-14: Remove the empty string from the _LOOPBACK_HOSTS frozenset
definition to eliminate the security bypass. The empty string should not be
considered a loopback host since socket binding resolves it to 0.0.0.0.
Additionally, add explicit handling for the empty string case in the
resolve_bind_host() function to ensure it is properly validated and rejected
with an InsecureBindError if passed as the host parameter, rather than allowing
it through as a loopback host.
---
Nitpick comments:
In `@libs/python/computer-server/tests/test_secure_bind.py`:
- Around line 106-110: The test_loopback_origin_passes_through method
parametrizes over only IPv4 loopback addresses (localhost and 127.0.0.1) but
lacks coverage for IPv6 loopback. Add an IPv6 loopback origin (formatted as
http://[::1]:PORT) to the parametrize decorator's origin list to ensure IPv6
loopback origins are correctly handled by the guard function.
- Around line 48-55: Add a test case for empty string host to verify the
behavior of resolve_bind_host("") function, which will help document the
expected behavior and prevent regressions related to security concerns with
empty strings in _LOOPBACK_HOSTS. You can add this either as a separate test
method or include it as a parameter in the existing parametrized tests
(test_explicit_loopback_allowed_in_local_mode or
test_explicit_public_host_refused_in_local_mode, depending on whether empty
string should be allowed or rejected).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b573e04a-a7ae-4c5f-968f-aeb105abbad7
📒 Files selected for processing (3)
libs/python/computer-server/computer_server/cli.pylibs/python/computer-server/computer_server/main.pylibs/python/computer-server/tests/test_secure_bind.py
… test skip scope) Review follow-ups on the trycua#1892 hardening: - Drop "" from _LOOPBACK_HOSTS — an empty/whitespace host binds all interfaces (INADDR_ANY), so it's refused in local mode instead of treated as loopback. - Match the Origin header case-insensitively (key.lower()) so a server that doesn't normalize header casing can't slip a differently-cased Origin past the cross-site guard. - Narrow test import guards from `except Exception` to `except ImportError` so a real regression in cli/main fails the suite instead of silently skipping.
|
The failing check is the Vercel docs preview — this PR is Python-only (computer-server) and doesn't touch docs/ |
|
Heads up: the binding half of this shipped via #1845 (merged) — local mode now defaults to |
…network by default (#1892)
By default the server bound 0.0.0.0 with auth OFF whenever CONTAINER_NAME wasn't set (local mode). So anyone on your LAN — or any website you happened to visit — could run shell commands, read/write files, and open a PTY shell.
Now,
Local mode now binds 127.0.0.1
If you really want a public --host in local mode, you now have to opt in with CUA_ALLOW_INSECURE=1 Added an Origin check on /ws, /cmd and /pty so a random web page can't drive your local server.
Summary by CodeRabbit
Bug Fixes
Tests