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

Boundary-hardening pass addressing three audit findings on the local sidecar.

Shell command allowlist (andrewyng/openworker#28):
- Replace prefix-string matching in PermissionEngine._command_allowed with
  argv-aware matching: reject any command containing shell operators
  (; & | > < ` $( ( and newlines) before consulting the allowlist, then require
  the allowlisted entry's tokens to be an exact argv prefix. This closes the
  auto-run bypass where an allowlisted "git status" also auto-ran
  "git status && rm -rf ~", pipes, redirection, and command substitution.
- Drop language interpreters / package managers (python, python3, node, npm,
  npx) from DEFAULT_ALLOWED_COMMANDS — allowlisting an interpreter allowlists
  arbitrary code (python3 -c "..."), defeating approval gating. Read-only
  inspection commands and pytest remain.

MCP OAuth loopback (andrewyng/openworker#29):
- Verify the OAuth state at the loopback boundary. The MCP SDK already validates
  state (compare_digest), so this is not a CSRF fix but defense-in-depth: capture
  the state from the authorize URL and have deliver_callback ignore a callback
  whose state does not match WITHOUT consuming the pending future, so a stray or
  forged local hit can no longer abort a user's in-progress sign-in. Falls back to
  prior accept-any behavior when no state was captured.

WebSocket ingestion caps (andrewyng/openworker#38):
- Bound a single user_message frame in the session WS loop: max text length,
  max attachment count, and max total attachment bytes. Oversized frames get a
  visible error frame and are dropped instead of being buffered into a turn; the
  socket stays alive. Guards the unauthenticated loopback socket against cheap
  memory spikes.

Tests:
- Allowlist: reject operator chaining (8 variants), argv-boundary matching, and
  interpreters-not-auto-allowed-by-default.
- OAuth: state extraction, and mismatched/missing state ignored without consuming
  the flow while the matching state still resolves it.
- WS: oversized text and too-many-attachments rejected with an error frame, and a
  normal message still works afterwards.

Full suite: 865 passed (1 pre-existing unrelated failure in
test_provider_router::test_manager_curated_models, present on origin/main).
This commit is contained in:
Fahad Siddiqui
2026-07-24 01:41:05 +05:00
parent 4766e59c47
commit 657cf03460
7 changed files with 210 additions and 11 deletions
+41
View File
@@ -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).