harden: container isolation, MCP argument handling, bridge token compare

.dockerignore

@@ -3,8 +3,6 @@
 __pycache__
 *.py[cod]
 *.log
-.env
-.env.*
 .venv
 venv
 node_modules
@@ -23,3 +21,41 @@ install.bat
 OmniParser_CraftOS
 debug_images
 workspace
+
+# ---------------------------------------------------------------------------
+# Secrets and per-install state. `COPY . .` runs against a developer's working
+# tree, and the release workflow pushes the resulting image to a public
+# registry — anything listed here would otherwise be baked into a layer and
+# published. Layers are permanent: deleting the file in a later layer does not
+# remove it. Keep this list in step with .gitignore.
+# ---------------------------------------------------------------------------
+.env
+.env.*
+.credentials
+.credentials/
+config.json
+app/config/settings.json
+app/config/mcp_config.json
+app/config/skills_config.json
+app/config/scheduler_config.json
+app/data/.usage
+app/data/.file_index
+agent_file_system
+agent_bundle
+.craftbot
+**/.craftbot
+**/logs
+**/agent_logs.txt
+**/memory.txt
+**/USER.md
+**/CONVERSATION_HISTORY.md
+**/EVENT.md
+**/EVENT_UNPROCESSED.md
+**/onboarding_config.json
+**/gui/config
+**/.whatsapp_web_sessions
+.playwright-mcp
+.claude
+.vscode
+.idea
+runtime

Dockerfile

@@ -24,16 +24,16 @@ RUN apt-get update \
         x11-apps \
         fonts-dejavu \
         curl \
-        gnupg \
-        lsb-release \
     && rm -rf /var/lib/apt/lists/*
 
-# Install Docker CLI (needed for docker exec against sibling desktop container)
-RUN curl -fsSL https://download.docker.com/linux/debian/gpg | gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg \
-    && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/debian $(lsb_release -cs) stable" > /etc/apt/sources.list.d/docker.list \
-    && apt-get update \
-    && apt-get install -y --no-install-recommends docker-ce-cli \
-    && rm -rf /var/lib/apt/lists/*
+# The Docker CLI used to be installed here, paired with a bind mount of
+# /var/run/docker.sock in docker-compose.yml. Nothing in the codebase ever
+# called it: a repo-wide search for `docker exec`, `docker run`, `docker.sock`,
+# `DOCKER_HOST` and the docker SDK finds no usage. The socket is the host's
+# control plane, so mounting it into a container that executes model-chosen
+# shell commands hands that container root on the host. Both are removed; if a
+# future feature needs to drive Docker, give it a scoped proxy rather than the
+# raw socket.
 
 WORKDIR /app
 
@@ -44,4 +44,16 @@ RUN pip install --no-cache-dir --upgrade pip \
 
 COPY . .
 
+# Run as an unprivileged user. The agent executes commands the model chooses,
+# so a container escape or a bad command should not land as root. The UID is
+# fixed (not auto-assigned) so a host bind mount can be chowned to match:
+#   sudo chown -R 10001:10001 ./workspace
+# The directories the compose file bind-mounts are pre-created here so they
+# exist with the right owner when no host directory is supplied.
+RUN useradd --create-home --uid 10001 --shell /usr/sbin/nologin craftbot \
+    && mkdir -p /app/workspace /app/logs \
+    && chown -R craftbot:craftbot /app
+
+USER craftbot
+
 CMD ["python", "-m", "app.main"]

agent_core/core/impl/mcp/server.py

@@ -58,6 +58,39 @@ def get_default_stdio_cwd() -> Optional[str]:
     return _default_stdio_cwd
 
 
+# Windows runs a .cmd/.bat through cmd.exe however it is spawned, and cmd.exe
+# re-parses the whole command line — so an argument carrying one of these
+# escapes into a second command even when the arguments are passed as a list.
+# No quoting closes it (this is the "BatBadBut" class, CVE-2024-24576 in other
+# runtimes), so the argument is refused rather than pretended to be escaped.
+# `^` and `%` are deliberately absent: `^` only escapes the next character and
+# cannot start a command, and rejecting `%` would break percent-encoded URLs,
+# which are ordinary MCP arguments. The worst `%` can do is expand an
+# environment variable into a value the same config already controls.
+_CMD_METACHARACTERS = '"&|<>\r\n'
+
+
+def _reject_unsafe_batch_args(command: str, args: List[str]) -> None:
+    """Refuse arguments cmd.exe would reinterpret when the target is a batch
+    wrapper. No-op off Windows and for real executables, where the argument
+    list reaches the process untouched."""
+    if sys.platform != "win32":
+        return
+    if os.path.splitext(command)[1].lower() not in (".cmd", ".bat"):
+        return
+    for arg in args:
+        found = sorted({c for c in str(arg) if c in _CMD_METACHARACTERS})
+        if found:
+            shown = "".join(c if c not in "\r\n" else repr(c).strip("'") for c in found)
+            raise ValueError(
+                f"MCP server argument {arg!r} contains {shown!r}, which cmd.exe "
+                f"would interpret while running the batch wrapper "
+                f"{os.path.basename(command)} — it could run a second command. "
+                f"Rewrite the argument, or point the server at the real "
+                f"executable instead of the .cmd shim."
+            )
+
+
 @dataclass
 class MCPTool:
     """Represents an MCP tool discovered from a server."""
@@ -177,41 +210,25 @@ async def connect(self) -> bool:
                 f" (cwd={cwd or os.getcwd()})"
             )
 
-            # Start the subprocess
+            # Start the subprocess. One exec path for both platforms: the
+            # arguments reach the OS as a list, so an argument can never be
+            # read back as part of the command line. _resolve_command has
+            # already turned `command` into a concrete path (npx.cmd, uvx.exe,
+            # ...), so there is nothing left for a shell to look up.
+            _reject_unsafe_batch_args(command, self.args)
             try:
-                if sys.platform == "win32":
-                    # On Windows, use shell=True to properly resolve commands like npx
-                    # This allows Windows to find npx.cmd in PATH
-                    full_command = f'"{command}" ' + " ".join(
-                        f'"{arg}"' for arg in self.args
-                    )
-                    logger.debug(
-                        f"[StdioTransport] Windows shell command: {full_command}"
-                    )
-                    self._process = await asyncio.create_subprocess_shell(
-                        full_command,
-                        stdin=asyncio.subprocess.PIPE,
-                        stdout=asyncio.subprocess.PIPE,
-                        stderr=asyncio.subprocess.PIPE,
-                        env=full_env,
-                        cwd=cwd,
-                        limit=10
-                        * 1024
-                        * 1024,  # 10MB limit for large MCP responses (e.g., screenshots)
-                    )
-                else:
-                    self._process = await asyncio.create_subprocess_exec(
-                        command,
-                        *self.args,
-                        stdin=asyncio.subprocess.PIPE,
-                        stdout=asyncio.subprocess.PIPE,
-                        stderr=asyncio.subprocess.PIPE,
-                        env=full_env,
-                        cwd=cwd,
-                        limit=10
-                        * 1024
-                        * 1024,  # 10MB limit for large MCP responses (e.g., screenshots)
-                    )
+                self._process = await asyncio.create_subprocess_exec(
+                    command,
+                    *self.args,
+                    stdin=asyncio.subprocess.PIPE,
+                    stdout=asyncio.subprocess.PIPE,
+                    stderr=asyncio.subprocess.PIPE,
+                    env=full_env,
+                    cwd=cwd,
+                    limit=10
+                    * 1024
+                    * 1024,  # 10MB limit for large MCP responses (e.g., screenshots)
+                )
             except FileNotFoundError as e:
                 logger.error(
                     f"[StdioTransport] Command not found: '{command}'. Make sure it is installed and in PATH. Error: {e}"

app/agent_app/manager.py

@@ -12,6 +12,7 @@
 
 import asyncio
 import errno
+import hmac
 import json
 import os
 import re
@@ -33,7 +34,6 @@
 from app import node_runtime
 from app.process_ledger import (
     ROLE_AGENT_APP,
-    ROLE_TUNNEL,
     get_ledger,
     kill_tree,
     listening_pids,
@@ -4172,8 +4172,18 @@ def validate_bridge_token(self, token: str) -> Optional[str]:
         Returns:
             project_id if token is valid, None otherwise.
         """
+        if not token:
+            return None
+        # Constant-time: this compares a caller-supplied header against a live
+        # secret, and a plain == leaks how many leading characters matched.
+        # Compared as bytes because compare_digest's str form rejects
+        # non-ASCII, and this value arrives from an HTTP header.
+        presented = token.encode("utf-8", "surrogateescape")
         for project_id, project in self.projects.items():
-            if project.bridge_token and project.bridge_token == token:
+            if not project.bridge_token:
+                continue
+            expected = project.bridge_token.encode("utf-8", "surrogateescape")
+            if hmac.compare_digest(presented, expected):
                 return project_id
         return None
 

docker-compose.yml

@@ -37,10 +37,22 @@ services:
     environment:
       - OMNIPARSER_BASE_URL=http://omniparser-server:7861
       - USE_OMNIPARSER=${USE_OMNIPARSER:-True}
+    # The image runs as UID 10001 (see Dockerfile). Bind-mounted host
+    # directories keep their host ownership, so chown them to match once:
+    #   sudo chown -R 10001:10001 ./workspace
     volumes:
-      - /var/run/docker.sock:/var/run/docker.sock
+      # /var/run/docker.sock was mounted here and is deliberately gone. It is
+      # the host's Docker control plane: a container holding it can start a
+      # privileged container and take the host. Nothing in the codebase used
+      # it (no `docker exec`, no docker SDK, no DOCKER_HOST), and this
+      # container runs commands the model chooses, so it was pure blast
+      # radius. Do not add it back without a scoped proxy in front.
       - ./workspace:/app/workspace
       - ./config.json:/app/config.json
+    security_opt:
+      # A process inside cannot gain privileges it did not start with
+      # (blocks setuid escalation paths).
+      - no-new-privileges:true
     depends_on:
       omniparser:
         condition: service_healthy

tests/test_bridge_token_validation.py

@@ -0,0 +1,77 @@
+"""validate_bridge_token: the integration bridge's only authentication.
+
+An Agent App backend presents this token to POST /api/integrations/proxy,
+which then injects the user's real OAuth credentials into an outbound call.
+The token therefore guards every connected integration, and it is compared
+against a value an attacker supplies in an HTTP header -- so the comparison
+has to be constant-time, and it has to survive whatever bytes arrive.
+"""
+
+import types
+
+import pytest
+
+from app.agent_app.manager import AgentAppManager
+
+TOKEN = "kR3n-9bQwTf1xZa7LmPd0sVhYcE2gJu4"
+
+
+@pytest.fixture
+def manager():
+    mgr = AgentAppManager.__new__(AgentAppManager)  # no filesystem, no ports
+    mgr.projects = {
+        "alpha": types.SimpleNamespace(bridge_token=TOKEN),
+        "beta": types.SimpleNamespace(bridge_token="another-token-entirely"),
+        "gamma": types.SimpleNamespace(bridge_token=""),  # never launched
+    }
+    return mgr
+
+
+def test_the_right_token_resolves_to_its_project(manager):
+    assert manager.validate_bridge_token(TOKEN) == "alpha"
+    assert manager.validate_bridge_token("another-token-entirely") == "beta"
+
+
+@pytest.mark.parametrize(
+    "presented",
+    [
+        "",
+        None,
+        "wrong",
+        TOKEN[:-1],  # prefix
+        TOKEN + "x",  # extension
+        TOKEN[:-1] + "X",  # last character differs
+        TOKEN.upper(),  # case
+        " " + TOKEN,  # leading space
+        TOKEN + " ",  # trailing space
+    ],
+)
+def test_anything_else_is_refused(manager, presented):
+    assert manager.validate_bridge_token(presented) is None
+
+
+def test_a_project_with_no_token_is_never_matched(manager):
+    """An empty bridge_token means 'not launched'. Presenting '' must not
+    match it -- that would hand out a project id for free."""
+    assert manager.validate_bridge_token("") is None
+
+
+@pytest.mark.parametrize(
+    "presented",
+    [
+        "tökén-with-non-ascii",
+        "\udcff\udcfe",  # lone surrogates, as a mangled header can carry
+        "x" * 10000,  # absurd length
+        "null\x00byte",
+    ],
+)
+def test_hostile_input_is_refused_without_raising(manager, presented):
+    """hmac.compare_digest rejects non-ASCII str outright, so the comparison
+    works on bytes. A caller controls this value; it must not be able to turn
+    a failed auth into a 500."""
+    assert manager.validate_bridge_token(presented) is None
+
+
+def test_no_project_at_all(manager):
+    manager.projects = {}
+    assert manager.validate_bridge_token(TOKEN) is None