fix(security): tighten IPC socket perms + use constant-time token compare

src/browser_harness/_ipc.py

@@ -168,6 +168,8 @@ async def serve(name, handler):
         old_umask = os.umask(0o077)
         try: server = await asyncio.start_unix_server(handler, path=path)
         finally: os.umask(old_umask)
+        # Belt-and-braces in case a future asyncio backend doesn't honour umask.
+        os.chmod(path, 0o600)
         _server_token = None
         async with server: await asyncio.Event().wait()
         return

src/browser_harness/daemon.py

@@ -1,5 +1,5 @@
 """CDP WS holder + IPC relay (Unix socket on POSIX, TCP loopback on Windows). One daemon per BU_NAME."""
-import asyncio, json, os, socket, sys, time, urllib.error, urllib.request
+import asyncio, hmac, json, os, socket, sys, time, urllib.error, urllib.request
 from urllib.parse import urlparse
 from collections import deque
 from pathlib import Path
@@ -262,9 +262,14 @@ async def handle(self, req):
         # Token guard for Windows TCP loopback: any local process can otherwise
         # connect and issue CDP commands. expected_token() is None on POSIX so
         # this check is a no-op there (AF_UNIX + chmod 600 is the boundary).
+        # hmac.compare_digest is constant-time so the per-character timing of
+        # `!=` can't be used to recover the token byte-by-byte; isinstance
+        # gate is required because compare_digest raises TypeError on non-str.
         expected = ipc.expected_token()
-        if expected is not None and req.get("token") != expected:
-            return {"error": "unauthorized"}
+        if expected is not None:
+            received = req.get("token")
+            if not isinstance(received, str) or not hmac.compare_digest(received, expected):
+                return {"error": "unauthorized"}
         meta = req.get("meta")
         # Liveness probe — lets clients confirm the listener is actually this
         # daemon and not an unrelated process that reused our port post-crash.

tests/unit/test_daemon.py

@@ -1,5 +1,7 @@
 import asyncio
+import hmac
 
+from browser_harness import _ipc as ipc
 from browser_harness import daemon
 
 
@@ -198,6 +200,75 @@ async def run():
     assert {"Page.enable", "DOM.enable", "Runtime.enable", "Network.enable"}.issubset(methods)
 
 
+def test_handle_token_compare_uses_constant_time_compare(monkeypatch):
+    """Windows TCP loopback path requires every request to carry an opaque
+    token (secrets.token_hex(32) — 256 bits). The previous comparison was
+    `req.get("token") != expected`, which short-circuits at the first
+    differing byte and leaks the prefix length via timing. Use
+    hmac.compare_digest, which the stdlib documents as constant-time."""
+    monkeypatch.setattr(ipc, "expected_token", lambda: "a" * 64)
+    calls = []
+    real_compare = hmac.compare_digest
+
+    def spy(a, b):
+        calls.append((a, b))
+        return real_compare(a, b)
+
+    monkeypatch.setattr(daemon.hmac, "compare_digest", spy)
+
+    d = _fresh_daemon()
+    resp = asyncio.run(d.handle({"meta": "ping", "token": "b" * 64}))
+
+    assert resp == {"error": "unauthorized"}
+    assert calls and calls[0] == ("b" * 64, "a" * 64), (
+        f"handle() must call hmac.compare_digest(received, expected) before "
+        f"accepting a request. Got call list: {calls}"
+    )
+
+
+def test_handle_rejects_non_string_token_without_crashing(monkeypatch):
+    """req is parsed JSON: req.get('token') can be None, list, dict, int —
+    hmac.compare_digest raises TypeError on non-str/bytes. The handler must
+    type-check and reject cleanly instead of letting that TypeError bubble
+    out of the IPC dispatch loop."""
+    monkeypatch.setattr(ipc, "expected_token", lambda: "a" * 64)
+    d = _fresh_daemon()
+
+    for bad in (None, 0, 1, True, False, ["a"], {"a": 1}, b"a" * 64):
+        resp = asyncio.run(d.handle({"meta": "ping", "token": bad}))
+        assert resp == {"error": "unauthorized"}, (
+            f"handle() must reject non-str token {bad!r} as unauthorized."
+        )
+
+
+def test_handle_accepts_correct_token(monkeypatch):
+    """Sanity check the constant-time path lets the right token through —
+    otherwise the compare_digest swap would silently lock everyone out."""
+    expected = "a" * 64
+    monkeypatch.setattr(ipc, "expected_token", lambda: expected)
+    d = _fresh_daemon()
+
+    resp = asyncio.run(d.handle({"meta": "ping", "token": expected}))
+    assert resp.get("pong") is True
+
+
+def test_handle_skips_token_check_on_posix_when_expected_is_none(monkeypatch):
+    """expected_token() is None on POSIX (AF_UNIX + chmod 600 is the
+    boundary). The handler must short-circuit *before* touching token
+    fields, so a missing or hostile token shape never trips the type guard
+    and a legitimate POSIX request reaches its meta branch."""
+    monkeypatch.setattr(ipc, "expected_token", lambda: None)
+    d = _fresh_daemon()
+
+    # No token at all.
+    resp = asyncio.run(d.handle({"meta": "ping"}))
+    assert resp.get("pong") is True
+
+    # Token of a wrong type — must still pass (no Windows guard active).
+    resp = asyncio.run(d.handle({"meta": "ping", "token": [1, 2, 3]}))
+    assert resp.get("pong") is True
+
+
 def test_set_session_first_attach_runs_four_enables_in_parallel():
     """When there's no previous session, the disable path is skipped — only
     the four enables run, still in parallel."""

tests/unit/test_ipc.py

@@ -1,6 +1,80 @@
+import asyncio
+import os
+import stat
+import sys
+
+import pytest
+
 from browser_harness import _ipc as ipc
 
 
+# --- serve(): AF_UNIX socket created with owner-only permissions ---
+
+@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only: AF_UNIX socket perms")
+def test_serve_posix_socket_is_created_with_owner_only_perms(tmp_path, monkeypatch):
+    """The AF_UNIX socket file must be mode 0o600 from the moment bind()
+    creates it, not just after the explicit chmod. asyncio.start_unix_server
+    begins accepting connections immediately, so a co-located unprivileged
+    user racing connect() against the chmod could otherwise issue CDP
+    commands during that window. Tightening the umask around bind() makes
+    the permissions correct atomically; this test asserts (a) umask is set
+    to 0o077 by the time start_unix_server's bind() runs, and (b) chmod
+    0o600 fires on the resulting path as a defence-in-depth follow-up."""
+    monkeypatch.setattr(ipc, "BH_TMP_DIR", str(tmp_path))
+    monkeypatch.setattr(ipc, "_TMP", tmp_path)
+
+    captured = {"umask_during_bind": None}
+    chmod_calls = []
+    real_start = asyncio.start_unix_server
+    real_chmod = os.chmod
+
+    async def spy_start(handler, path):
+        # Snapshot the umask the kernel will see when bind() creates the
+        # socket file. os.umask(0) flips to 0 and returns the prior value;
+        # restore immediately so we don't disturb the run.
+        current = os.umask(0)
+        os.umask(current)
+        captured["umask_during_bind"] = current
+        return await real_start(handler, path)
+
+    def spy_chmod(path, mode):
+        chmod_calls.append((str(path), mode))
+        return real_chmod(path, mode)
+
+    monkeypatch.setattr(asyncio, "start_unix_server", spy_start)
+    monkeypatch.setattr(os, "chmod", spy_chmod)
+
+    async def run():
+        async def handler(reader, writer):
+            writer.close()
+
+        task = asyncio.create_task(ipc.serve("default", handler))
+        # Yield until serve() has reached its blocking point — chmod is the
+        # last sync step before `await asyncio.Event().wait()`, so seeing
+        # the chmod call is the cleanest "serve is past setup" signal.
+        for _ in range(200):
+            await asyncio.sleep(0)
+            if chmod_calls and captured["umask_during_bind"] is not None:
+                break
+        task.cancel()
+        try:
+            await task
+        except (asyncio.CancelledError, BaseException):
+            pass
+
+    asyncio.run(run())
+    assert captured["umask_during_bind"] == 0o077, (
+        f"serve() must tighten umask to 0o077 before start_unix_server's "
+        f"bind() creates the socket file. Saw umask="
+        f"{oct(captured['umask_during_bind']) if captured['umask_during_bind'] is not None else 'unset'}."
+    )
+    sock_path = str(tmp_path / "bu.sock")
+    assert (sock_path, 0o600) in chmod_calls, (
+        f"serve() must chmod the socket file to 0o600 as a belt-and-braces "
+        f"defence after umask. Got chmod calls: {chmod_calls}"
+    )
+
+
 # --- identify(): ping payload sanitation ---
 
 class _FakeConn: