fix(agent-server): stop using the session API key as the VSCode connection token

openhands-agent-server/openhands/agent_server/vscode_service.py

@@ -1,7 +1,11 @@
 """VSCode service for managing OpenVSCode Server in the agent server."""
 
 import asyncio
+import hashlib
+import hmac
 import os
+import shutil
+import tempfile
 from pathlib import Path
 
 from openhands.sdk.logger import get_logger
@@ -11,6 +15,40 @@
 logger = get_logger(__name__)
 
 
+# Domain separator for `derive_connection_token`. Anything that wants to build
+# the editor URL without asking the agent server for it must derive the token
+# the same way, so this string is part of the contract with those callers and
+# cannot change without changing the `:v1` suffix.
+VSCODE_TOKEN_DERIVATION_INFO = b"openhands-agent-server:vscode-connection-token:v1"
+
+
+def derive_connection_token(session_api_key: str) -> str:
+    """Derive the editor's connection token from a session API key.
+
+    The editor's token travels in the URL's ``?tkn=`` query parameter, so it
+    lands in browser history, in ``Referer`` headers from the pages the
+    workbench renders, and in the access log of anything proxying the editor.
+    Using the session API key itself there means each of those is a disclosure
+    of the credential that authenticates every ``/api/*`` call on this server.
+
+    Deriving instead keeps the property that made sharing attractive in the
+    first place (see #793): a caller holding the session API key can still
+    compute the editor URL on its own, with no round trip to the agent server.
+    What it loses is the reverse direction — the derived token is a one-way
+    function of the key, so disclosing it no longer discloses API access.
+
+    A hex digest also always satisfies the ``^[0-9A-Za-z_-]+$`` that
+    openvscode-server enforces on connection tokens, which an arbitrary session
+    API key does not: a key containing so much as a ``.`` currently makes the
+    editor refuse to start with a token parse error.
+    """
+    return hmac.new(
+        session_api_key.encode("utf-8"),
+        VSCODE_TOKEN_DERIVATION_INFO,
+        hashlib.sha256,
+    ).hexdigest()
+
+
 class VSCodeService:
     """Service to manage VSCode server startup and token generation."""
 
@@ -35,6 +73,7 @@ def __init__(
         self.process: asyncio.subprocess.Process | None = None
         self.openvscode_server_root: Path = Path("/openhands/.openvscode-server")
         self.extensions_dir: Path = self.openvscode_server_root / "extensions"
+        self._token_dir: Path | None = None
 
     async def start(self) -> bool:
         """Start the VSCode server.
@@ -69,6 +108,9 @@ async def start(self) -> bool:
 
         except Exception as e:
             logger.error(f"Failed to start VSCode server: {e}")
+            # The token file outlives a failed start otherwise: nothing will
+            # call stop() for a server that never came up.
+            self._remove_connection_token_file()
             return False
 
     async def stop(self) -> None:
@@ -86,6 +128,7 @@ async def stop(self) -> None:
                 logger.error(f"Error stopping VSCode server: {e}")
             finally:
                 self.process = None
+        self._remove_connection_token_file()
 
     def get_vscode_url(
         self,
@@ -153,31 +196,68 @@ async def _is_port_available(self) -> bool:
         except OSError:
             return False
 
+    def _write_connection_token_file(self) -> Path:
+        """Write the connection token to a file only this user can read.
+
+        openvscode-server's own option documentation recommends
+        ``--connection-token-file`` over ``--connection-token`` on a multi-user
+        system precisely because the latter "can be seen by other users using
+        ``ps`` or similar commands". That applies here: the agent runs bash in
+        this same container, so anything in the server's argv is readable by the
+        agent itself.
+
+        Returns:
+            Path of the token file, inside a directory owned by this process.
+        """
+        if self.connection_token is None:
+            raise ValueError("Cannot write a connection token file without a token")
+
+        # A restart must not leave the previous run's file behind.
+        self._remove_connection_token_file()
+
+        # mkdtemp is 0o700 and O_EXCL|0o600 creates the file with its final
+        # permissions, so there is no window in which either is readable by
+        # another user.
+        token_dir = Path(tempfile.mkdtemp(prefix="openhands-vscode-"))
+        token_file = token_dir / "connection-token"
+        fd = os.open(token_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
+        with os.fdopen(fd, "w") as f:
+            f.write(self.connection_token)
+        self._token_dir = token_dir
+        return token_file
+
+    def _remove_connection_token_file(self) -> None:
+        """Remove the token file and its directory, if one was written."""
+        if self._token_dir is None:
+            return
+        shutil.rmtree(self._token_dir, ignore_errors=True)
+        self._token_dir = None
+
     async def _start_vscode_process(self) -> None:
         """Start the VSCode server process."""
-        extensions_arg = (
-            f"--extensions-dir {self.extensions_dir} "
-            if self.extensions_dir.exists()
-            else ""
-        )
-        base_path_arg = (
-            f"--server-base-path {self.server_base_path} "
-            if self.server_base_path
-            else ""
-        )
-        cmd = (
-            f"exec {self.openvscode_server_root}/bin/openvscode-server "
-            f"--host 0.0.0.0 "
-            f"--connection-token {self.connection_token} "
-            f"--port {self.port} "
-            f"{extensions_arg}"
-            f"{base_path_arg}"
-            f"--disable-workspace-trust\n"
-        )
+        # An argument list rather than a shell string: `server_base_path` and
+        # the extensions path are configuration, and interpolating them into a
+        # command line means a value containing a space or a `;` is a broken
+        # server at best. There is no shell to `exec` past either, so
+        # `self.process` is the server itself and `terminate()` reaches it.
+        argv = [
+            str(self.openvscode_server_root / "bin" / "openvscode-server"),
+            "--host",
+            "0.0.0.0",
+            "--connection-token-file",
+            str(self._write_connection_token_file()),
+            "--port",
+            str(self.port),
+        ]
+        if self.extensions_dir.exists():
+            argv += ["--extensions-dir", str(self.extensions_dir)]
+        if self.server_base_path:
+            argv += ["--server-base-path", self.server_base_path]
+        argv.append("--disable-workspace-trust")
 
         # Start the process
-        self.process = await asyncio.create_subprocess_shell(
-            cmd,
+        self.process = await asyncio.create_subprocess_exec(
+            *argv,
             stdout=asyncio.subprocess.PIPE,
             stderr=asyncio.subprocess.STDOUT,
             env=sanitized_env(),
@@ -244,9 +324,13 @@ def get_vscode_service() -> VSCodeService | None:
             logger.info("VSCode is disabled in configuration")
             return None
         else:
+            # Derived, not copied. A caller that knows the session API key can
+            # still build the editor URL without calling this server — see
+            # `derive_connection_token` — but the token in that URL is no longer
+            # usable as an API credential.
             connection_token = None
             if config.session_api_keys:
-                connection_token = config.session_api_keys[0]
+                connection_token = derive_connection_token(config.session_api_keys[0])
             _vscode_service = VSCodeService(
                 port=config.vscode_port,
                 connection_token=connection_token,

tests/agent_server/test_vscode_service.py

@@ -1,12 +1,16 @@
 """Tests for VSCode service."""
 
 import asyncio
+import re
+import stat
+from pathlib import Path
 from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
 
 from openhands.agent_server.vscode_service import (
     VSCodeService,
+    derive_connection_token,
     get_vscode_service,
 )
 
@@ -292,7 +296,7 @@ async def test_start_vscode_process(vscode_service, tmp_path):
 
     with (
         patch(
-            "asyncio.create_subprocess_shell", return_value=mock_process
+            "asyncio.create_subprocess_exec", return_value=mock_process
         ) as mock_create,
         patch.object(vscode_service, "_wait_for_startup") as mock_wait,
     ):
@@ -315,15 +319,16 @@ async def test_start_vscode_process_with_server_base_path():
 
     with (
         patch(
-            "asyncio.create_subprocess_shell", return_value=mock_process
+            "asyncio.create_subprocess_exec", return_value=mock_process
         ) as mock_create,
         patch.object(service, "_wait_for_startup"),
     ):
         await service._start_vscode_process()
 
         # Verify the command includes --server-base-path
-        cmd = mock_create.call_args[0][0]
-        assert "--server-base-path /runtime/vscode" in cmd
+        argv = list(mock_create.call_args[0])
+        assert "--server-base-path" in argv
+        assert argv[argv.index("--server-base-path") + 1] == "/runtime/vscode"
 
 
 @pytest.mark.asyncio
@@ -336,15 +341,95 @@ async def test_start_vscode_process_without_server_base_path():
 
     with (
         patch(
-            "asyncio.create_subprocess_shell", return_value=mock_process
+            "asyncio.create_subprocess_exec", return_value=mock_process
         ) as mock_create,
         patch.object(service, "_wait_for_startup"),
     ):
         await service._start_vscode_process()
 
         # Verify the command does not include --server-base-path
-        cmd = mock_create.call_args[0][0]
-        assert "--server-base-path" not in cmd
+        assert "--server-base-path" not in list(mock_create.call_args[0])
+
+
+@pytest.mark.asyncio
+async def test_start_vscode_process_keeps_token_out_of_argv():
+    """The token must never reach the process table.
+
+    The agent runs bash in this same container, so an argument list readable by
+    `ps` is readable by the agent. openvscode-server's own docs recommend
+    `--connection-token-file` for exactly this reason.
+    """
+    service = VSCodeService(port=8001, connection_token="s3cr3t-token-value")
+
+    mock_process = AsyncMock()
+    mock_process.stdout = AsyncMock()
+
+    with (
+        patch(
+            "asyncio.create_subprocess_exec", return_value=mock_process
+        ) as mock_create,
+        patch.object(service, "_wait_for_startup"),
+    ):
+        await service._start_vscode_process()
+
+    argv = [str(arg) for arg in mock_create.call_args[0]]
+    assert "--connection-token" not in argv
+    assert not any("s3cr3t-token-value" in arg for arg in argv)
+
+    token_file = Path(argv[argv.index("--connection-token-file") + 1])
+    assert token_file.read_text() == "s3cr3t-token-value"
+
+    service._remove_connection_token_file()
+
+
+@pytest.mark.asyncio
+async def test_connection_token_file_is_not_world_readable():
+    """The token file is only useful if its permissions hold up."""
+    service = VSCodeService(port=8001, connection_token="test-token")
+
+    token_file = service._write_connection_token_file()
+    try:
+        assert stat.S_IMODE(token_file.stat().st_mode) == 0o600
+        assert stat.S_IMODE(token_file.parent.stat().st_mode) == 0o700
+    finally:
+        service._remove_connection_token_file()
+
+
+@pytest.mark.asyncio
+async def test_connection_token_file_removed_on_stop():
+    """Test that stopping the service cleans up the token file."""
+    service = VSCodeService(port=8001, connection_token="test-token")
+    token_file = service._write_connection_token_file()
+    assert token_file.exists()
+
+    await service.stop()
+
+    assert not token_file.exists()
+    assert not token_file.parent.exists()
+
+
+@pytest.mark.asyncio
+async def test_connection_token_file_removed_when_start_fails(mock_openvscode_binary):
+    """A start that never completes still has to clean up after itself.
+
+    Nothing calls stop() for a server that never came up, so the failure path
+    owns the token file it wrote.
+    """
+    service = VSCodeService(port=8001, connection_token="test-token")
+    service.openvscode_server_root = mock_openvscode_binary
+
+    with (
+        patch.object(service, "_is_port_available", return_value=True),
+        patch.object(
+            service, "_start_vscode_process", side_effect=OSError("no such binary")
+        ),
+    ):
+        # The token file is written inside _start_vscode_process, so stand one
+        # up first to represent the case where the failure comes after it.
+        token_file = service._write_connection_token_file()
+        assert await service.start() is False
+
+    assert not token_file.exists()
 
 
 @pytest.mark.asyncio
@@ -405,6 +490,55 @@ def test_get_vscode_service_enabled(tmp_path):
         assert isinstance(service, VSCodeService)
 
 
+def test_get_vscode_service_derives_token_from_session_api_key():
+    """The editor token must not be the session API key itself.
+
+    The token is a URL query parameter, so it reaches browser history, Referer
+    headers and proxy access logs. Deriving it keeps those disclosures from
+    being disclosures of the credential that authenticates every /api/* call.
+    """
+    with (
+        patch("openhands.agent_server.config.get_default_config") as mock_config,
+        patch("openhands.agent_server.vscode_service._vscode_service", None),
+    ):
+        mock_config.return_value.enable_vscode = True
+        mock_config.return_value.vscode_port = 8001
+        mock_config.return_value.vscode_base_path = None
+        mock_config.return_value.session_api_keys = ["super-secret-key", "second-key"]
+
+        service = get_vscode_service()
+
+        assert isinstance(service, VSCodeService)
+        assert service.connection_token != "super-secret-key"
+        assert service.connection_token == derive_connection_token("super-secret-key")
+        assert service.get_vscode_url() is not None
+        assert "super-secret-key" not in service.get_vscode_url()  # type: ignore[operator]
+
+
+def test_derive_connection_token_is_deterministic():
+    """Callers build the editor URL themselves, so the derivation is a contract.
+
+    A caller holding the session API key computes the same token this server
+    does, with no round trip — that is the property #793 wanted from sharing the
+    key outright.
+    """
+    assert derive_connection_token("key-a") == derive_connection_token("key-a")
+    assert derive_connection_token("key-a") != derive_connection_token("key-b")
+
+
+def test_derive_connection_token_satisfies_vscode_charset():
+    """openvscode-server enforces `^[0-9A-Za-z_-]+$` on connection tokens.
+
+    A session API key that violates it makes the editor refuse to start with a
+    token parse error, so passing the key through was a latent failure for any
+    key containing punctuation. A hex digest always satisfies the rule.
+    """
+    token = derive_connection_token("a key/with+punctuation=and spaces")
+
+    assert re.fullmatch(r"[0-9A-Za-z_-]+", token)
+    assert len(token) == 64
+
+
 def test_get_vscode_service_disabled():
     """Test get_vscode_service returns None when disabled."""
     with (