Skip to content

fix(conductor): require auth on loopback when --key is set (closes #726) - #727

Open
Kailigithub wants to merge 1 commit into
lsdefine:mainfrom
Kailigithub:fix/issue-726-conductor-loopback-auth
Open

fix(conductor): require auth on loopback when --key is set (closes #726)#727
Kailigithub wants to merge 1 commit into
lsdefine:mainfrom
Kailigithub:fix/issue-726-conductor-loopback-auth

Conversation

@Kailigithub

Copy link
Copy Markdown
Contributor

Closes #726

frontends/conductor.py RemoteAuth middleware exempts loopback callers
(127.0.0.1, ::1) from the auth check whenever --key is supplied:

if args.key and remote not in ("127.0.0.1", "::1") and not secrets.compare_digest(got, want):

Combined with CORSMiddleware(allow_origins=["*"], allow_methods=["*"], allow_headers=["*"])
on line 122, this means a visited web page can fetch('http://127.0.0.1:8900/subagent')
and reach POST /subagent, action=input, /approval without any auth — cross-principal
task dispatch and instruction injection into the user's agent session.

This PR ships the narrow half of the issue's proposed fix: remove the loopback exemption.
When --key is supplied, every caller (loopback or not) must send the matching
Authorization: Basic header. The default launch (no --key) keeps the historical
no-auth behavior so existing local installs are unaffected.

Change

 class RemoteAuth:
     def __init__(self, app): self.app = app
     async def __call__(self, scope, receive, send):
-        remote = (scope.get("client") or ("",))[0]
         got = dict(scope.get("headers", [])).get(b"authorization", b"")
         want = b"Basic " + base64.b64encode(f"conductor:{args.key}".encode())
-        if args.key and remote not in ("127.0.0.1", "::1") and not secrets.compare_digest(got, want):
+        if args.key and not secrets.compare_digest(got, want):
             if scope["type"] == "websocket":
                 return await send({"type": "websocket.close", "code": 1008})
             return await PlainTextResponse("Unauthorized", 401, {"WWW-Authenticate": 'Basic realm="Conductor"'})(scope, receive, send)
         await self.app(scope, receive, send)

Verification

Adds frontends/test_issue_726_loopback_auth.py covering 7 cases:

# Caller --key Header Expected Got (OLD) Got (NEW)
1 127.0.0.1 set none 401 200 (bug) 401
2 127.0.0.1 set correct 200 200 200
3 127.0.0.1 set wrong 401 401 401
4 10.0.0.5 set none 401 401 401
5 127.0.0.1 unset none 200 (no auth) 200 200
6 10.0.0.5 set correct 200 200 200
7 WS 127.0.0.1 set none ws.close passthrough ws.close

Run the three-step dance (test fails on main, passes on the fix branch):

git stash
python3 frontends/test_issue_726_loopback_auth.py    # → AssertionError on test 1
git stash pop
python3 frontends/test_issue_726_loopback_auth.py    # → ALL TESTS PASSED

Deferred (not in this PR)

Per #726's Suggested change, two other pieces remain — both are larger diffs and
strictly independent of the loopback exemption:

  1. CORSMiddleware(allow_origins=["*"]) at line 122 → explicit origin allow-list
  2. --key having no default at line 33 → auto-generate and write to a file

These are deferred to follow-up PRs to keep this change ≤5 source lines (per the
project's review principles in CONTRIBUTING.md: "small change radius",
"net line count: ideally negative or zero for refactors"). Happy to draft the
narrow piece for either as a separate PR if the maintainer concurs with the
split.

…define#726)

RemoteAuth exempted 127.0.0.1 / ::1 callers from the Authorization check,
letting any page with a fetch() reach POST /subagent etc. when --key was
supplied. With CORSMiddleware(allow_origins=["*"]) still on line 122, that
extended to cross-origin pages visited while the conductor runs.

Drop the loopback exemption. When --key is set, every caller must send
the matching Basic header. Default launch (no --key) keeps historical
no-auth behavior so local installs are unaffected.

Defer CORSMiddleware origin tightening and --key default to follow-up
PRs to keep this change small and focused.

Adds frontends/test_issue_726_loopback_auth.py covering 7 cases with
the git-stash three-step dance proving test 1 fails on main and passes
on the fix.
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.

conductor /subagent and /approval unauthenticated on the default launch; wildcard CORS exposes them to any visited web page

1 participant