Skip to content

fix(security): restrict Conductor browser origins - #741

Open
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/conductor-browser-origin-guard-upstream
Open

fix(security): restrict Conductor browser origins#741
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/conductor-browser-origin-guard-upstream

Conversation

@imMamdouhaboammar

Copy link
Copy Markdown

Summary

Fixes the browser-origin portion of upstream issue #726.

Conductor currently enables wildcard CORS for every HTTP route and accepts WebSocket handshakes without an Origin check. Because the desktop bridge starts Conductor on loopback without --key, an unrelated webpage can reach 127.0.0.1:8900 and attempt to invoke Conductor HTTP APIs or open its WebSocket.

The bundled desktop UI intentionally talks from port 14168 to Conductor on port 8900, so the policy must preserve trusted cross-port traffic on the same loopback host.

Changes

  • Replace wildcard CORSMiddleware with a small ASGI BrowserOriginGuard
  • Protect both HTTP and WebSocket handshakes before route dispatch
  • When Conductor is bound to loopback, require both Origin hostname and request Host hostname to be known loopback names: 127.0.0.1, localhost, or ::1
  • For intentional non-loopback binds, require validated Origin hostname to match request Host hostname
  • Accept only serialized HTTP(S) Origin values with a valid port and no userinfo, path outside the serialized-origin form, query, or fragment
  • Reject opaque values such as Origin: null
  • Reflect only an accepted Origin in CORS responses, with Vary: Origin
  • Keep no-Origin native/local callers working
  • Keep the desktop UI on 127.0.0.1:14168 able to call and connect to Conductor on 127.0.0.1:8900
  • Preserve the existing RemoteAuth behavior and all subagent/API logic

Regression coverage

The suite covers nine behaviors:

  1. Cross-origin preflight rejection
  2. Cross-origin POST rejection
  3. DNS-rebinding-style matching attacker Host/Origin rejection on loopback
  4. Rejection of malformed serialized Origins: path, query, userinfo, invalid port, and fragment
  5. Trusted desktop cross-port HTTP access
  6. Origin: null rejection
  7. No-Origin native/local access
  8. Cross-origin WebSocket rejection
  9. Trusted desktop cross-port WebSocket access

Verification

  • Tests were written before the production middleware existed
  • Initial RED confirmed current main had no BrowserOriginGuard
  • The first implementation passed 6/7 original cases; the single failure was isolated to Starlette TestClient using a different WebSocket Host, so only the test Host header was corrected
  • A DNS-rebinding regression was then added and independently observed failing with 200 != 403 before the loopback anchor was added
  • Code review identified uncanonical Origin reflection; malformed serialized-Origin cases were added and independently observed failing before strict parsing was introduced
  • Each production patch passed python -m py_compile frontends/conductor.py tests/test_conductor_origin.py, the full regression suite, and git diff --check before commit
  • Final persisted-state GitHub Actions run 31159379674 passed the test job while every self-patching step was skipped, proving the committed code itself is green
  • Final contribution branch is exactly one commit ahead of current main and changes only two files
  • The review thread for uncanonical Origin handling is resolved and outdated after the parser hardening

No new runtime dependency is introduced. FastAPI is already used by Conductor.

Scope and residual boundary

This PR intentionally addresses the browser-origin half of upstream #726. It does not change RemoteAuth, which still does not require a key for default loopback callers. Adding a generated per-launch key affects the desktop bridge, Conductor HTTP and WebSocket clients, and the standalone browser UI, so it should be handled as a separate protocol-level contribution.

Same-loopback-host browser content on another port remains trusted because the supported desktop UI itself uses that pattern. A per-launch secret is the appropriate follow-up if maintainers want to remove that trust assumption.

The connected GitHub integration has write access to this fork, but upstream write operations return 403 Resource not accessible by integration, so I cannot open the cross-fork PR to lsdefine/GenericAgent directly from this session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant