diff --git a/coworker/config.py b/coworker/config.py index b13547b6..abfe57a6 100644 --- a/coworker/config.py +++ b/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", ] diff --git a/coworker/mcp/oauth.py b/coworker/mcp/oauth.py index 85f8e35c..c6043e69 100644 --- a/coworker/mcp/oauth.py +++ b/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( diff --git a/coworker/permissions.py b/coworker/permissions.py index 23c67f9f..82477f7b 100644 --- a/coworker/permissions.py +++ b/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 diff --git a/coworker/server/app.py b/coworker/server/app.py index cbccc4a7..e43033a2 100644 --- a/coworker/server/app.py +++ b/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). diff --git a/tests/test_mcp_oauth.py b/tests/test_mcp_oauth.py index d50a9cd4..94e20113 100644 --- a/tests/test_mcp_oauth.py +++ b/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 --------------------------------------------------- diff --git a/tests/test_permissions_risk.py b/tests/test_permissions_risk.py index 6b55e106..98450eb0 100644 --- a/tests/test_permissions_risk.py +++ b/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 diff --git a/tests/test_server.py b/tests/test_server.py index b88c558e..71f4ab7e 100644 --- a/tests/test_server.py +++ b/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):