Merge pull request #6280 from isharak7m/fix/token-cache-race-condition

fix: atomic token cache swap to eliminate race condition

.github/workflows/codeql.yml

@@ -31,11 +31,11 @@ jobs:
         with:
           persist-credentials: false
       - name: Initialize CodeQL
-        uses: github/codeql-action/init@cdf488f595d80d6e07e03d4674febd5ab45fa938  # v4.37.9
+        uses: github/codeql-action/init@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd  # v4.37.7
         with:
           languages: ${{ matrix.language }}
           build-mode: none
       - name: Perform CodeQL Analysis
-        uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938  # v4.37.9
+        uses: github/codeql-action/analyze@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd  # v4.37.7
         with:
           category: "/language:${{ matrix.language }}"

.github/workflows/container-scan.yml

@@ -42,7 +42,7 @@ jobs:
           persist-credentials: false
 
       - name: Lint Dockerfile
-        uses: hadolint/hadolint-action@06be81baf89a55ffd0e24b8f04a4185738dd3387  # v3.5.0
+        uses: hadolint/hadolint-action@2a66e89f53d0771bb131a7fa31f3136336094aa6  # v3.4.0
         with:
           dockerfile: Dockerfile
           # DL3008: pinning apt package versions is impractical on a -slim base

.github/workflows/container-trivy.yml

@@ -123,7 +123,7 @@ jobs:
           TRIVY_DB_REPOSITORY: ghcr.io/aquasecurity/trivy-db:2
 
       - name: Upload Trivy results
-        uses: github/codeql-action/upload-sarif@cdf488f595d80d6e07e03d4674febd5ab45fa938  # v4.37.9
+        uses: github/codeql-action/upload-sarif@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd  # v4.37.7
         with:
           sarif_file: trivy-results.sarif
           category: trivy-image

.github/workflows/deploy-pages.yml

@@ -47,4 +47,4 @@ jobs:
     steps:
       - name: Deploy to GitHub Pages
         id: deployment
-        uses: actions/deploy-pages@368f82528645a54fb793d4d04e342629a3f51346  # v5.0.1
+        uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128  # v5.0.0

app.py

@@ -313,6 +313,7 @@ if AUTH_ENABLED:
 
     def _refresh_token_cache():
         """Rebuild the prefix→[(id,hash)] map from the DB."""
+        global _token_cache
         from collections import defaultdict
         new_map = defaultdict(list)
         db = SessionLocal()
@@ -331,8 +332,8 @@ if AUTH_ENABLED:
                 new_map[r.token_prefix].append((r.id, r.token_hash, owner_key, scopes))
         finally:
             db.close()
-        _token_cache.clear()
+        _token_cache = dict(new_map)
-        _token_cache.update(new_map)
+        app.state._token_cache = _token_cache
         app.state._token_cache_dirty = False
 
     # Headers that prove a request was forwarded by a proxy/tunnel (cloudflared,

routes/mcp/mcp_routes.py

@@ -181,10 +181,17 @@ def setup_mcp_routes(mcp_manager: McpManager):
         if transport == "http" and not url:
             raise HTTPException(400, "url is required for HTTP transport")
 
-        # Parse JSON fields
+        # Parse JSON fields. args is not defaulted on a parse failure: an
-        try:
+        # unparseable value is silently discarded downstream (stdio spawns
-            parsed_args = json.loads(args) if args else []
+        # with an empty argv), so the caller must be told instead.
-        except json.JSONDecodeError:
+        if args:
+            try:
+                parsed_args = json.loads(args)
+            except json.JSONDecodeError:
+                raise HTTPException(400, "args must be valid JSON, e.g. [\"-y\", \"pkg\"]")
+            if not isinstance(parsed_args, list):
+                raise HTTPException(400, "args must be a JSON array, e.g. [\"-y\", \"pkg\"]")
+        else:
             parsed_args = []
         try:
             parsed_env = json.loads(env) if env else {}

static/js/admin.js

@@ -2366,6 +2366,7 @@ function initMcpForm() {
     if (transport === 'stdio' && !command) { msg.textContent = 'Command is required for stdio'; msg.className = 'admin-error'; return; }
     if (transport === 'sse' && !url) { msg.textContent = 'URL is required for SSE'; msg.className = 'admin-error'; return; }
     try { JSON.parse(env); } catch { msg.textContent = 'Env must be valid JSON'; msg.className = 'admin-error'; return; }
+    try { JSON.parse(args); } catch { msg.textContent = 'Args must be valid JSON, e.g. ["-y", "pkg"]'; msg.className = 'admin-error'; return; }
     const fd = new FormData();
     fd.append('name', name); fd.append('transport', transport); fd.append('command', command); fd.append('args', args); fd.append('env', env); fd.append('url', url);
     // If preset has oauthFile config, send credentials for file generation
@@ -2386,6 +2387,10 @@ function initMcpForm() {
     try {
       const res = await fetch('/api/mcp/servers', { method: 'POST', body: fd, credentials: 'same-origin' });
       const data = await res.json();
+      if (!res.ok) {
+        msg.textContent = data.detail || `Failed (${res.status})`; msg.className = 'admin-error';
+        return;
+      }
       if (data.needs_oauth) {
         msg.innerHTML = `Added ${esc(name)} — <a href="/api/mcp/oauth/authorize/${data.id}" target="_blank" style="color:var(--red);font-weight:600;">Authorize with Google</a> to connect`;
         msg.className = 'admin-success';

static/js/settings.js

@@ -5036,7 +5036,11 @@ async function initUnifiedIntegrations() {
         fd.append('transport', transport);
         if (transport === 'stdio') {
           fd.append('command', el('uf-mcp-cmd').value);
-          let args = '[]'; try { args = JSON.stringify(JSON.parse(el('uf-mcp-args').value || '[]')); } catch (_) {}
+          // Unlike env below, an unparseable args value is not silently
+          // defaulted: it would spawn the subprocess with an empty argv.
+          let args;
+          try { args = JSON.stringify(JSON.parse(el('uf-mcp-args').value || '[]')); }
+          catch (_) { el('uf-mcp-msg').textContent = 'Args must be valid JSON, e.g. ["-y", "pkg"]'; return; }
           let env  = '{}'; try { env  = JSON.stringify(JSON.parse(el('uf-mcp-env').value  || '{}')); } catch (_) {}
           fd.append('args', args);
           fd.append('env', env);

tests/test_mcp_add_server_args_validation.py

@@ -0,0 +1,147 @@
+"""Regression test for issue #6211: a malformed Args value on the "Add MCP
+Server" form must not be silently discarded into an empty argv.
+
+routes/mcp/mcp_routes.py's add_server() wrapped json.loads(args) in a bare
+except that fell back to `[]`, so a non-JSON Args value registered the
+server as "Connected" while forwarding no arguments to the spawned stdio
+subprocess at all, with no error surfaced anywhere.
+"""
+import asyncio
+import json
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from fastapi import HTTPException
+
+from routes.mcp import mcp_routes
+
+
+class _FakeSession:
+    """Stands in for core.database.SessionLocal(); add_server only adds+commits."""
+
+    def __init__(self):
+        self.added = []
+
+    def add(self, obj):
+        self.added.append(obj)
+
+    def commit(self):
+        pass
+
+    def close(self):
+        pass
+
+
+def _add_server(monkeypatch):
+    """Register add_server on the shared module-level router and return the
+    freshly-added route's raw endpoint function, bypassing HTTP/Form parsing
+    (require_admin is the only other thing the function touches via `request`).
+
+    Callers must pass every Form(...) parameter add_server reads past the args
+    check (url, oauth_file, oauth_config): calling the endpoint directly skips
+    FastAPI's dependency resolution, so an omitted one arrives as the Form
+    marker object itself rather than its declared default, and later code
+    (e.g. `if oauth_file:`) reads that marker as truthy.
+    """
+    monkeypatch.setattr(mcp_routes, "require_admin", lambda request: None)
+    manager = MagicMock()
+    manager.connect_server = AsyncMock(return_value=True)
+    manager.get_server_status = MagicMock(return_value={"status": "connected", "tool_count": 1})
+    router = mcp_routes.setup_mcp_routes(manager)
+    # setup_mcp_routes appends new APIRoute objects to the shared router on
+    # every call, so take the LAST "add_server" route: the one just registered
+    # with our fake manager, not an earlier registration from importing app.py.
+    route = [r for r in router.routes if getattr(r, "name", None) == "add_server"][-1]
+    return route.endpoint, manager
+
+
+def test_add_server_rejects_malformed_args_instead_of_defaulting(monkeypatch):
+    add_server, manager = _add_server(monkeypatch)
+    monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: (_ for _ in ()).throw(
+        AssertionError("must not reach the DB when args is rejected")))
+
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(add_server(
+            request=None,
+            name="filesystem",
+            transport="stdio",
+            command="mcp-server-filesystem",
+            args="/app/data/jarvis-files",  # the exact value from issue #6211
+            env="{}",
+            url=None,
+            oauth_file=None,
+            oauth_config=None,
+        ))
+
+    assert exc.value.status_code == 400
+    manager.connect_server.assert_not_called()
+
+
+def test_add_server_still_accepts_valid_json_args(monkeypatch):
+    add_server, manager = _add_server(monkeypatch)
+    fake_session = _FakeSession()
+    monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: fake_session)
+
+    result = asyncio.run(add_server(
+        request=None,
+        name="filesystem",
+        transport="stdio",
+        command="mcp-server-filesystem",
+        args=json.dumps(["/app/data/jarvis-files"]),
+        env="{}",
+        url=None,
+        oauth_file=None,
+        oauth_config=None,
+    ))
+
+    assert result["connected"] is True
+    manager.connect_server.assert_awaited_once()
+    assert manager.connect_server.call_args.kwargs["args"] == ["/app/data/jarvis-files"]
+    assert fake_session.added[0].args == json.dumps(["/app/data/jarvis-files"])
+
+
+def test_add_server_rejects_valid_json_args_that_is_not_a_list(monkeypatch):
+    """Valid JSON that is not a list (e.g. args=5) must not reach
+    StdioServerParameters(args=5), which raises an unhandled TypeError when
+    the error formatter later does " ".join([command, *args])."""
+    add_server, manager = _add_server(monkeypatch)
+    monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: (_ for _ in ()).throw(
+        AssertionError("must not reach the DB when args has the wrong shape")))
+
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(add_server(
+            request=None,
+            name="filesystem",
+            transport="stdio",
+            command="mcp-server-filesystem",
+            args="5",
+            env="{}",
+            url=None,
+            oauth_file=None,
+            oauth_config=None,
+        ))
+
+    assert exc.value.status_code == 400
+    manager.connect_server.assert_not_called()
+
+
+def test_add_server_still_defaults_empty_args_to_empty_list(monkeypatch):
+    """No behavior change for the common case of an empty Args field."""
+    add_server, manager = _add_server(monkeypatch)
+    fake_session = _FakeSession()
+    monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: fake_session)
+
+    result = asyncio.run(add_server(
+        request=None,
+        name="no-args-server",
+        transport="stdio",
+        command="some-command",
+        args="",
+        env="{}",
+        url=None,
+        oauth_file=None,
+        oauth_config=None,
+    ))
+
+    assert result["connected"] is True
+    assert manager.connect_server.call_args.kwargs["args"] == []

tests/test_token_cache_atomic_swap.py

@@ -0,0 +1,188 @@
+"""Regression test for token cache race condition.
+
+Exercises the actual _refresh_token_cache() and _token_cache in app.py
+to verify the atomic swap fix eliminates the race window.
+"""
+import os
+import sys
+import threading
+import time
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+import pytest
+
+
+@pytest.fixture
+def app_module(monkeypatch):
+    """Import app.py with AUTH_ENABLED=true and minimal mocked deps.
+
+    Sets up a real AuthManager user ('admin') so normalize_known_username
+    resolves the token owner.  Replaces SessionLocal with a MagicMock so
+    _refresh_token_cache() can run without a real DB.
+    """
+    monkeypatch.setenv("AUTH_ENABLED", "true")
+    monkeypatch.setenv("DATABASE_URL", "sqlite:///:memory:")
+
+    # Clear cached app module
+    monkeypatch.delitem(sys.modules, "app", raising=False)
+
+    import app as app_mod  # noqa: E402
+
+    app_mod.SessionLocal = MagicMock()
+    app_mod.logger = MagicMock()
+    app_mod.auth_manager.setup("admin", "TestPass123!")
+
+    return app_mod
+
+
+def _seed(app_mod, rows):
+    """Set up the mocked SessionLocal to return *rows* on next query."""
+    app_mod.SessionLocal.return_value.query.return_value.filter.return_value.all.return_value = rows
+
+
+def _row(prefix, tid="t1", th="h1", owner="admin", scopes="chat"):
+    return SimpleNamespace(
+        token_prefix=prefix, id=tid, token_hash=th,
+        owner=owner, scopes=scopes, is_active=True,
+    )
+
+
+# ---------------------------------------------------------------------------
+# Tests — all use the REAL app._refresh_token_cache and REAL app._token_cache
+# ---------------------------------------------------------------------------
+
+class TestRefreshPopulatesCache:
+    """Single refresh call should populate _token_cache from DB rows."""
+
+    def test_single_row(self, app_module):
+        _seed(app_module, [_row("ody_abc")])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+        assert "ody_abc" in app_module._token_cache
+        assert app_module._token_cache["ody_abc"][0][0] == "t1"
+
+    def test_multiple_prefixes(self, app_module):
+        _seed(app_module, [
+            _row("ody_aaaa", "t1", "h1", "admin", "chat"),
+            _row("ody_bbbb", "t2", "h2", "admin", "chat,tools"),
+            _row("ody_aaaa", "t3", "h3", "admin", "memory"),
+        ])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+        assert len(app_module._token_cache) == 2
+        assert len(app_module._token_cache["ody_aaaa"]) == 2
+        assert app_module._token_cache["ody_bbbb"][0][3] == ["chat", "tools"]
+
+    def test_empty_db_clears_cache(self, app_module):
+        app_module._token_cache["stale"] = [("x", "y", "z", ["chat"])]
+        _seed(app_module, [])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+        assert len(app_module._token_cache) == 0
+
+
+class TestAppStateSync:
+    """app.state._token_cache must stay synchronized with _token_cache."""
+
+    def test_state_ref_matches_after_refresh(self, app_module):
+        _seed(app_module, [_row("ody_sync")])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+        assert app_module.app.state._token_cache is app_module._token_cache
+        assert "ody_sync" in app_module.app.state._token_cache
+
+    def test_state_dirty_cleared(self, app_module):
+        _seed(app_module, [_row("ody_x")])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+        assert app_module.app.state._token_cache_dirty is False
+
+
+class TestConcurrentReaders:
+    """The core regression: concurrent readers must never see an empty cache."""
+
+    def test_no_empty_reads_during_refresh(self, app_module):
+        """4 reader threads + 100 refreshes on the real _token_cache global."""
+        _seed(app_module, [_row("ody_race")])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+
+        stop = threading.Event()
+        results = {"empty": 0, "ok": 0}
+
+        def reader():
+            while not stop.is_set():
+                if len(app_module._token_cache) == 0:
+                    results["empty"] += 1
+                else:
+                    results["ok"] += 1
+
+        def churner():
+            for _ in range(100):
+                app_module._refresh_token_cache()
+
+        readers = [threading.Thread(target=reader, daemon=True) for _ in range(4)]
+        for t in readers:
+            t.start()
+
+        churn = threading.Thread(target=churner)
+        churn.start()
+
+        time.sleep(0.1)
+        churn.join(timeout=5)
+        stop.set()
+        for t in readers:
+            t.join(timeout=2)
+
+        assert results["empty"] == 0, (
+            "Readers saw empty cache %d times (ok=%d)"
+            % (results["empty"], results["ok"])
+        )
+        assert results["ok"] > 0
+
+    def test_no_empty_reads_with_token_churn(self, app_module):
+        """Simulate token create/revoke churn while reading."""
+        _seed(app_module, [_row("ody_keep", "t1", "h1", "admin", "chat")])
+        app_module.app.state._token_cache_dirty = True
+        app_module._refresh_token_cache()
+
+        stop = threading.Event()
+        results = {"empty": 0, "ok": 0}
+
+        def reader():
+            while not stop.is_set():
+                if len(app_module._token_cache) == 0:
+                    results["empty"] += 1
+                else:
+                    results["ok"] += 1
+
+        def churner():
+            for i in range(50):
+                _seed(app_module, [
+                    _row("ody_keep", "t1", "h1", "admin", "chat"),
+                    _row("ody_new_%d" % i, "t%d" % (i + 10), "h%d" % (i + 10), "admin", "chat"),
+                ])
+                app_module._refresh_token_cache()
+                _seed(app_module, [_row("ody_keep", "t1", "h1", "admin", "chat")])
+                app_module._refresh_token_cache()
+
+        readers = [threading.Thread(target=reader, daemon=True) for _ in range(4)]
+        for t in readers:
+            t.start()
+
+        churn = threading.Thread(target=churner)
+        churn.start()
+
+        churn.join(timeout=10)
+        stop.set()
+        for t in readers:
+            t.join(timeout=2)
+
+        assert results["empty"] == 0, (
+            "Readers saw empty cache %d times during churn (ok=%d)"
+            % (results["empty"], results["ok"])
+        )
+        assert results["ok"] > 0
+        assert "ody_keep" in app_module._token_cache
+        assert len(app_module._token_cache) == 1