mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-09-23 16:42:21 +02:00
fix(mcp): reject malformed Args on Add MCP Server instead of silently defaulting to [] (#6215)
* fix(mcp): reject malformed Args on Add MCP Server instead of silently defaulting to [] * test(mcp): pass every Form param add_server reads past args validation CI's pytest run showed test_add_server_still_accepts_valid_json_args and test_add_server_still_defaults_empty_args_to_empty_list failing with TypeError: the JSON object must be str, bytes or bytearray, not Form. Calling the endpoint function directly bypasses FastAPI's dependency resolution, so an unpassed Form(...) parameter (url, oauth_file, oauth_config) arrives as the Form marker object itself rather than its declared default, and add_server's later `if oauth_file:` check reads that marker as truthy. The malformed-args test never hit this because it raises before reaching that code. Not a production bug: a real HTTP request resolves these through FastAPI before add_server ever runs. * fix(mcp): reject non-list args and surface the new 400 in the Admin panel o3LL's review on #6215 found two gaps in the args validation this PR adds: the Admin panel posts to the same /api/mcp/servers endpoint but never validates Args client-side, so the new 400 falls into the generic failure branch and shows "Added but connection failed: unknown". Mirror the same JSON.parse guard settings.js already has. Also add an isinstance(list) check next to the existing JSON parse, since valid-but-wrong-shaped JSON (args=5) reaches StdioServerParameters(args=5) and 500s in the error formatter. Pre-existing on dev, same validation site this PR already touches. * fix(admin): surface the server's 400 detail instead of a generic connection-failed message The Admin add-server handler read needs_oauth/connected/error but never res.ok, so a request rejected by the isinstance(list) check added for #6211 (args=5, a valid-JSON-but-non-list value the client-side JSON.parse guard cannot catch) fell into the same-shape else branch as a successful add whose connection attempt failed, and the form fields were cleared as if the server had accepted it.
This commit is contained in:
@@ -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';
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user