mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-09-14 20:22:22 +02:00
Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
b5c8888fda |
@@ -313,7 +313,6 @@ 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()
|
||||
@@ -332,8 +331,8 @@ if AUTH_ENABLED:
|
||||
new_map[r.token_prefix].append((r.id, r.token_hash, owner_key, scopes))
|
||||
finally:
|
||||
db.close()
|
||||
_token_cache = dict(new_map)
|
||||
app.state._token_cache = _token_cache
|
||||
_token_cache.clear()
|
||||
_token_cache.update(new_map)
|
||||
app.state._token_cache_dirty = False
|
||||
|
||||
# Headers that prove a request was forwarded by a proxy/tunnel (cloudflared,
|
||||
|
||||
@@ -43,4 +43,4 @@ PyMuPDF
|
||||
# magika (onnxruntime), already a core dep via fastembed. We avoid the
|
||||
# [all]/Azure/audio extras (cloud + heavy). Pinned to a release >30 days old per
|
||||
# the dependency-age discussion in issue #485.
|
||||
markitdown[docx,pptx,xlsx,xls]==0.1.6
|
||||
markitdown[docx,pptx,xlsx,xls]==0.1.7
|
||||
|
||||
+4
-4
@@ -3,9 +3,9 @@ uvicorn
|
||||
python-multipart
|
||||
python-dotenv
|
||||
httpx
|
||||
httpcore>=1.0,<2.0
|
||||
pydantic>=2.13.4
|
||||
pydantic-settings>=2.14.1
|
||||
httpcore>=1.0.9,<2.0
|
||||
pydantic>=2.13.5
|
||||
pydantic-settings>=2.15.0
|
||||
SQLAlchemy
|
||||
pypdf
|
||||
beautifulsoup4
|
||||
@@ -41,7 +41,7 @@ bcrypt
|
||||
# Built-in servers use the v1 low-level Server decorator API. MCP SDK v2 is a
|
||||
# breaking rewrite, so keep fresh installs on the maintained v1 line until the
|
||||
# servers are migrated together.
|
||||
mcp<2
|
||||
mcp<3
|
||||
pyotp
|
||||
qrcode[pil]
|
||||
croniter
|
||||
|
||||
@@ -181,17 +181,10 @@ 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. args is not defaulted on a parse failure: an
|
||||
# unparseable value is silently discarded downstream (stdio spawns
|
||||
# with an empty argv), so the caller must be told instead.
|
||||
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:
|
||||
# Parse JSON fields
|
||||
try:
|
||||
parsed_args = json.loads(args) if args else []
|
||||
except json.JSONDecodeError:
|
||||
parsed_args = []
|
||||
try:
|
||||
parsed_env = json.loads(env) if env else {}
|
||||
|
||||
@@ -2366,7 +2366,6 @@ 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
|
||||
@@ -2387,10 +2386,6 @@ 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';
|
||||
|
||||
@@ -5036,11 +5036,7 @@ async function initUnifiedIntegrations() {
|
||||
fd.append('transport', transport);
|
||||
if (transport === 'stdio') {
|
||||
fd.append('command', el('uf-mcp-cmd').value);
|
||||
// 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 args = '[]'; try { args = JSON.stringify(JSON.parse(el('uf-mcp-args').value || '[]')); } catch (_) {}
|
||||
let env = '{}'; try { env = JSON.stringify(JSON.parse(el('uf-mcp-env').value || '{}')); } catch (_) {}
|
||||
fd.append('args', args);
|
||||
fd.append('env', env);
|
||||
|
||||
@@ -1,147 +0,0 @@
|
||||
"""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"] == []
|
||||
@@ -1,188 +0,0 @@
|
||||
"""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
|
||||
Reference in New Issue
Block a user