Merge pull request #49 from fahadsiddiqui/harden/boundary-oauth-shell-ws

fix: harden local trust boundaries (shell allowlist, MCP OAuth loopback, WS ingestion)

coworker/config.py

@@ -13,6 +13,12 @@ from typing import Any, Optional
 
 from .secrets import state_dir
 
+# Commands auto-run WITHOUT an approval prompt. Deliberately limited to read-only
+# inspection: language interpreters and package managers (python/node/npm/npx) are NOT
+# here — allowlisting an interpreter allowlists arbitrary code (`python3 -c "..."`), which
+# defeats the point of approval gating. Users can still add them via config if they accept
+# that trade-off. Matching is exact-argv-prefix and rejects shell operators — see
+# PermissionEngine._command_allowed.
 DEFAULT_ALLOWED_COMMANDS = [
     "ls",
     "cat",
@@ -27,12 +33,7 @@ DEFAULT_ALLOWED_COMMANDS = [
     "git diff",
     "git log",
     "git show",
-    "python3",
-    "python",
     "pytest",
-    "node",
-    "npm",
-    "npx",
 ]
 
 

coworker/mcp/oauth.py

@@ -21,6 +21,7 @@ from __future__ import annotations
 import asyncio
 import logging
 import os
+import secrets
 from typing import Any, Optional
 
 from mcp.client.auth import OAuthClientProvider, TokenStorage
@@ -117,21 +118,48 @@ _pending: Optional[asyncio.Future] = None
 # The last authorize URL we sent the user to — surfaced over REST so the GUI can offer
 # a "reopen sign-in page" link if the browser popup was lost.
 last_authorize_url: Optional[str] = None
+# The `state` the SDK put in the current authorize URL. The SDK itself re-checks the
+# returned state (mcp.client.auth.oauth2 compare_digest), so this is NOT the CSRF guard —
+# it's a loopback gate: without it any local caller could hit /mcp/oauth/callback with a
+# bogus code and consume the single pending future, aborting the user's real sign-in
+# (which then finds no pending flow). Matching state here rejects that stray callback and
+# leaves the flow waiting for the genuine one.
+_expected_state: Optional[str] = None
+
+
+def _state_from_url(url: str) -> Optional[str]:
+    """Pull the `state` query param out of an authorize URL (None if absent)."""
+    from urllib.parse import parse_qs, urlsplit
+
+    values = parse_qs(urlsplit(url).query).get("state")
+    return values[0] if values else None
 
 
 def deliver_callback(code: str, state: Optional[str]) -> bool:
-    """Called by the loopback route. Resolves the waiting flow; False if none waits."""
+    """Called by the loopback route. Resolves the waiting flow; False if none waits.
+
+    A callback whose `state` doesn't match the pending flow's is ignored (returns False)
+    WITHOUT consuming the pending future, so a stray/forged local hit can't abort a live
+    sign-in — only the browser redirect carrying the SDK's own state resolves it.
+    """
     global _pending
-    pending, _pending = _pending, None
-    if pending is None or pending.done():
+    if _pending is None or _pending.done():
         return False
+    # Only enforce when we actually captured a state for this flow; a flow with no state
+    # in its authorize URL falls back to the prior accept-any behavior.
+    if _expected_state is not None and (
+        state is None or not secrets.compare_digest(state, _expected_state)
+    ):
+        return False
+    pending, _pending = _pending, None
     pending.set_result((code, state))
     return True
 
 
 async def _open_browser(url: str) -> None:
-    global last_authorize_url
+    global last_authorize_url, _expected_state
     last_authorize_url = url
+    _expected_state = _state_from_url(url)
     import webbrowser
 
     logger.info("mcp oauth: opening browser for sign-in")
@@ -155,7 +183,7 @@ async def _refuse_callback() -> tuple[str, Optional[str]]:
 
 
 async def _wait_for_callback() -> tuple[str, Optional[str]]:
-    global _pending
+    global _pending, _expected_state
     if _pending is not None and not _pending.done():
         _pending.cancel()  # a stale flow lost its browser tab; the new one wins
     _pending = asyncio.get_running_loop().create_future()
@@ -168,6 +196,7 @@ async def _wait_for_callback() -> tuple[str, Optional[str]]:
         )
     finally:
         _pending = None
+        _expected_state = None  # don't let this flow's state gate the next one
 
 
 def build_auth(

coworker/permissions.py

@@ -8,11 +8,22 @@ prefixes) and a session allowlist. The engine only *decides*; the turn engine ro
 
 from __future__ import annotations
 
+import shlex
 from dataclasses import dataclass, field
 from enum import Enum
 from pathlib import Path
 from typing import Any, Optional
 
+# Shell metacharacters that turn one "allowlisted" command into several. Any of these in a
+# command disqualifies it from allowlist auto-run — approval is required instead. Covers
+# chaining (`;` `&` `&&` `||`), pipes (`|`), redirection (`>` `<`), command substitution
+# (`` ` `` `$(`), process substitution / grouping (`(`), and newlines.
+_SHELL_OPERATORS = (";", "&", "|", ">", "<", "`", "$(", "(", "\n", "\r")
+
+
+def _has_shell_operators(command: str) -> bool:
+    return any(op in command for op in _SHELL_OPERATORS)
+
 from .risk import (  # re-exported for back-compat (manager.py imports WRITE_TOOLS)
     SHELL_TOOL,
     WRITE_TOOLS,
@@ -203,7 +214,25 @@ class PermissionEngine:
         return False
 
     def _command_allowed(self, command: str) -> bool:
+        # An allowlist entry auto-runs a command WITHOUT approval, so prefix matching is
+        # unsafe: `git status` would auto-approve `git status && rm -rf ~`. Reject anything
+        # carrying shell operators (chaining/redirection/substitution) up front, then match
+        # the parsed argv against each entry — the entry's own tokens must be an exact
+        # prefix of the command's tokens (so `git status` matches `git status -s` but never
+        # `git statusfoo` or a bare `git`).
+        if _has_shell_operators(command):
+            return False
+        try:
+            argv = shlex.split(command)
+        except ValueError:
+            return False  # unbalanced quotes etc. — treat as not-allowlisted
+        if not argv:
+            return False
         for allowed in self.allowed_commands:
-            if command == allowed or command.startswith(f"{allowed} "):
+            try:
+                prefix = shlex.split(allowed)
+            except ValueError:
+                continue
+            if prefix and argv[: len(prefix)] == prefix:
                 return True
         return False

coworker/server/app.py

@@ -39,6 +39,27 @@ def _origin_allowed(origin: str | None) -> bool:
     return origin is None or bool(_ALLOWED_ORIGIN_RE.match(origin))
 
 
+# Caps on a single inbound `user_message` frame. The loopback socket is unauthenticated
+# (any local process can reach it), so an oversized frame is a cheap way to spike memory —
+# these bound one message's text and its attachments before we build content / start a turn.
+_MAX_MESSAGE_TEXT_CHARS = 200_000
+_MAX_ATTACHMENTS = 25
+_MAX_ATTACHMENTS_BYTES = 32 * 1024 * 1024  # ~32 MB total across a message's attachments
+
+
+def _attachments_size(attachments: list) -> int:
+    """Approximate on-wire byte size of a message's attachments (data URLs dominate)."""
+    total = 0
+    for a in attachments:
+        if isinstance(a, str):
+            total += len(a)
+        elif isinstance(a, dict):
+            for v in a.values():
+                if isinstance(v, str):
+                    total += len(v)
+    return total
+
+
 # Brand colors for the connector badge riding the ✓ (UX-DECISIONS §30). The GUI owns the
 # real logos; this page must render offline with zero assets, so a colored initial stands in.
 _BRAND_COLORS = {
@@ -1665,6 +1686,26 @@ def create_app(manager: SessionManager) -> FastAPI:
                 elif kind == "user_message":
                     text = (message.get("text") or "").strip()
                     attachments = message.get("attachments") or []
+                    # Reject an oversized frame instead of buffering it into a turn. Send a
+                    # visible error so the surface can tell the user, and drop the message.
+                    if not isinstance(attachments, list):
+                        attachments = []
+                    reject = None
+                    if len(text) > _MAX_MESSAGE_TEXT_CHARS:
+                        reject = (
+                            f"Message too long ({len(text)} chars; "
+                            f"limit {_MAX_MESSAGE_TEXT_CHARS})."
+                        )
+                    elif len(attachments) > _MAX_ATTACHMENTS:
+                        reject = (
+                            f"Too many attachments ({len(attachments)}; "
+                            f"limit {_MAX_ATTACHMENTS})."
+                        )
+                    elif _attachments_size(attachments) > _MAX_ATTACHMENTS_BYTES:
+                        reject = "Attachments too large (limit 32 MB per message)."
+                    if reject is not None:
+                        await ws.send_json({"type": "error", "data": {"error": reject}})
+                        continue
                     # The composer sends its visible model with every message — the FIRST
                     # one binds the session (race-proof across reconnects; see api.ts
                     # Session.userMessage), later ones may switch it (notice persisted).

tests/test_mcp_oauth.py

@@ -31,8 +31,10 @@ def _state(tmp_path, monkeypatch, servers=None):
 @pytest.fixture(autouse=True)
 def _no_pending():
     mcp_oauth._pending = None
+    mcp_oauth._expected_state = None
     yield
     mcp_oauth._pending = None
+    mcp_oauth._expected_state = None
 
 
 # -- config --------------------------------------------------------------------
@@ -100,6 +102,30 @@ def test_wait_then_deliver_resolves():
     assert asyncio.run(run()) == ("c0de", "st4te")
 
 
+def test_state_from_url():
+    url = "https://idp.example/authorize?client_id=x&state=abc123&scope=y"
+    assert mcp_oauth._state_from_url(url) == "abc123"
+    assert mcp_oauth._state_from_url("https://idp.example/authorize?client_id=x") is None
+
+
+def test_deliver_rejects_mismatched_state_without_consuming_flow():
+    # A stray/forged loopback hit with the wrong state must NOT resolve or consume the
+    # pending future — the genuine redirect (correct state) still gets through.
+    async def run():
+        task = asyncio.create_task(mcp_oauth._wait_for_callback())
+        await asyncio.sleep(0)
+        mcp_oauth._expected_state = "good-state"  # captured from the authorize URL
+        # Wrong state and missing state are both ignored, flow stays pending.
+        assert mcp_oauth.deliver_callback("evil", "bad-state") is False
+        assert mcp_oauth.deliver_callback("evil", None) is False
+        assert not task.done()
+        # The real browser redirect carries the matching state and resolves the flow.
+        assert mcp_oauth.deliver_callback("c0de", "good-state") is True
+        return await task
+
+    assert asyncio.run(run()) == ("c0de", "good-state")
+
+
 # -- status surfacing over REST ---------------------------------------------------
 
 

tests/test_permissions_risk.py

@@ -98,3 +98,46 @@ def test_exec_uses_command_allowlist(tmp_path):
     assert eng.evaluate("run_shell", {"command": "pytest -q"}, None).allowed
     asked = eng.evaluate("run_shell", {"command": "rm -rf /"}, None)
     assert not asked.allowed and asked.needs_user
+
+
+@pytest.mark.parametrize(
+    "command",
+    [
+        "git status && rm -rf ~",  # chaining
+        "git status; rm -rf ~",  # sequencing
+        "git status | tee /tmp/x",  # pipe
+        "git status || curl evil",  # or-chain
+        "git status $(rm -rf ~)",  # command substitution
+        "git status `rm -rf ~`",  # backtick substitution
+        "git status > /etc/passwd",  # redirection
+        "git status\nrm -rf ~",  # newline-embedded second command
+    ],
+)
+def test_allowlist_rejects_shell_operator_chaining(tmp_path, command):
+    # An allowlisted prefix must NOT auto-run a command that chains anything after it.
+    eng = PermissionEngine(workspace_root=tmp_path, allowed_commands=["git status"])
+    d = eng.evaluate("run_shell", {"command": command}, None)
+    assert not d.allowed and d.needs_user, command
+
+
+def test_allowlist_prefix_is_argv_boundary(tmp_path):
+    eng = PermissionEngine(workspace_root=tmp_path, allowed_commands=["git status", "ls"])
+    # Exact and sub-argument extensions of the allowlisted argv are fine.
+    assert eng.evaluate("run_shell", {"command": "git status"}, None).allowed
+    assert eng.evaluate("run_shell", {"command": "git status -s"}, None).allowed
+    assert eng.evaluate("run_shell", {"command": "ls -la"}, None).allowed
+    # A different subcommand or a token that merely shares a prefix is NOT allowed.
+    assert eng.evaluate("run_shell", {"command": "git push"}, None).needs_user
+    assert eng.evaluate("run_shell", {"command": "lsof"}, None).needs_user
+
+
+def test_interpreters_not_auto_allowed_by_default(tmp_path):
+    # The default allowlist must not auto-run interpreters (arbitrary code execution).
+    from coworker.config import DEFAULT_ALLOWED_COMMANDS
+
+    eng = PermissionEngine(
+        workspace_root=tmp_path, allowed_commands=list(DEFAULT_ALLOWED_COMMANDS)
+    )
+    for cmd in ("python3 -c 'import os'", "node -e 1", "npm run x", "npx foo"):
+        d = eng.evaluate("run_shell", {"command": cmd}, None)
+        assert not d.allowed and d.needs_user, cmd

tests/test_server.py

@@ -323,6 +323,36 @@ def test_ws_simple_turn(tmp_path):
         assert "turn_end" in types
 
 
+def test_ws_rejects_oversized_message(tmp_path):
+    from coworker.server import app as app_mod
+
+    client = _client(tmp_path, [_text("should not run")])
+    with client.websocket_connect("/ws/session/big") as ws:
+        assert ws.receive_json()["type"] == "ready"
+
+        # Oversized text → single error frame, no turn runs.
+        ws.send_json(
+            {"type": "user_message", "text": "x" * (app_mod._MAX_MESSAGE_TEXT_CHARS + 1)}
+        )
+        evt = ws.receive_json()
+        assert evt["type"] == "error" and "too long" in evt["data"]["error"].lower()
+
+        # Too many attachments → error frame.
+        ws.send_json(
+            {
+                "type": "user_message",
+                "text": "hi",
+                "attachments": ["a"] * (app_mod._MAX_ATTACHMENTS + 1),
+            }
+        )
+        evt = ws.receive_json()
+        assert evt["type"] == "error" and "attachment" in evt["data"]["error"].lower()
+
+        # A normal message still works afterwards (the socket wasn't torn down).
+        ws.send_json({"type": "user_message", "text": "hello"})
+        assert "turn_done" in _drain(ws)
+
+
 def test_ws_error_persists_notice_and_retry_reruns(tmp_path):
     class FlakyProvider(ProviderClient):
         def __init__(self):