From 7fc7f7427fff2c49a4322124f5b57020fe72b37a Mon Sep 17 00:00:00 2001 From: CI Test Date: Mon, 5 Oct 2026 22:24:15 +0100 Subject: [PATCH] fix(security): enforce registered endpoint authority Require caller-supplied model endpoints to resolve through enabled owner-visible registrations, harden session path encoding, and remove the SVG title HTML parsing sink. --- routes/assistant_routes.py | 9 +- routes/skills_routes.py | 20 +- routes/task/task_routes.py | 18 +- src/endpoint_resolver.py | 23 ++ static/js/markdown.js | 11 +- static/js/sessions.js | 2 +- tests/codeql_security_browser.cjs | 20 +- tests/test_endpoint_registered_authority.py | 249 ++++++++++++++++++++ tests/test_markdown_dom_xss_helpers.py | 11 + 9 files changed, 349 insertions(+), 14 deletions(-) create mode 100644 tests/test_endpoint_registered_authority.py diff --git a/routes/assistant_routes.py b/routes/assistant_routes.py index f16f016e9..d31376dc3 100644 --- a/routes/assistant_routes.py +++ b/routes/assistant_routes.py @@ -16,6 +16,7 @@ from pydantic import BaseModel from core.database import SessionLocal, CrewMember, ScheduledTask from src.auth_helpers import get_current_user +from src.endpoint_resolver import resolve_owner_registered_endpoint_url from src.owner_identity import REQUEST_SENTINEL_OWNERS from src.task_scheduler import compute_next_run @@ -178,7 +179,13 @@ def setup_assistant_routes(task_scheduler) -> APIRouter: if payload.model is not None: crew_db.model = payload.model or None if payload.endpoint_url is not None: - crew_db.endpoint_url = payload.endpoint_url or None + try: + crew_db.endpoint_url = ( + resolve_owner_registered_endpoint_url(db, payload.endpoint_url, owner) + if payload.endpoint_url else None + ) + except ValueError as exc: + raise HTTPException(400, str(exc)) from exc if payload.timezone is not None: crew_db.timezone = payload.timezone or None diff --git a/routes/skills_routes.py b/routes/skills_routes.py index ec5a49876..87d8cb435 100644 --- a/routes/skills_routes.py +++ b/routes/skills_routes.py @@ -2003,7 +2003,11 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter: it untouched). It never changes the skill's published/draft STATUS.""" import time as _time import asyncio as _asyncio - from src.endpoint_resolver import resolve_endpoint + from core.database import SessionLocal + from src.endpoint_resolver import ( + build_chat_url, build_headers, resolve_endpoint, + resolve_endpoint_runtime, resolve_owner_registered_endpoint, + ) user = _owner(request) body = await request.json() @@ -2027,10 +2031,18 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter: # session's model. Fall back to the caller's session model only if unset. url, model, headers = resolve_endpoint("utility", owner=user) if not url or not model: - url = url or ((body.get("endpoint_url") or "").strip() or None) + if not url and body.get("endpoint_url") is not None: + db = SessionLocal() + try: + endpoint = resolve_owner_registered_endpoint(db, body["endpoint_url"], user) + base, api_key = resolve_endpoint_runtime(endpoint, owner=user) + url = build_chat_url(base) + headers = build_headers(api_key, base) + except ValueError as exc: + raise HTTPException(400, str(exc)) from exc + finally: + db.close() model = model or ((body.get("model") or "").strip() or None) - if headers is None and isinstance(body.get("headers"), dict): - headers = body.get("headers") if not url or not model: raise HTTPException(400, "No model configured — set a Default or Utility model in Settings.") diff --git a/routes/task/task_routes.py b/routes/task/task_routes.py index 0749cb548..a1179d50e 100644 --- a/routes/task/task_routes.py +++ b/routes/task/task_routes.py @@ -14,6 +14,7 @@ from core.database import SessionLocal, ScheduledTask, TaskRun, NotificationLog from core.constants import internal_api_base from src.auth_helpers import get_current_user from src.constants import DATA_DIR, EMAIL_URGENCY_CACHE_DIR +from src.endpoint_resolver import resolve_owner_registered_endpoint_url from src.task_action_policy import ( ADMIN_ONLY_TASK_ACTIONS, is_admin_only_task_action, @@ -519,6 +520,12 @@ def setup_task_routes(task_scheduler) -> APIRouter: db = SessionLocal() try: then_task_id = _validate_then_task_id(db, req.then_task_id, user) + endpoint_url = None + if req.endpoint_url: + try: + endpoint_url = resolve_owner_registered_endpoint_url(db, req.endpoint_url, user) + except ValueError as exc: + raise HTTPException(400, str(exc)) from exc notifications_enabled = ( False if req.task_type == "action" and req.notifications_enabled is None else bool(req.notifications_enabled) if req.notifications_enabled is not None @@ -555,7 +562,7 @@ def setup_task_routes(task_scheduler) -> APIRouter: status="active" if (req.trigger_type in ("event", "webhook") or next_run) else "completed", output_target=req.output_target, model=req.model or None, - endpoint_url=req.endpoint_url or None, + endpoint_url=endpoint_url, then_task_id=then_task_id, webhook_token=webhook_token, notifications_enabled=notifications_enabled, @@ -755,7 +762,14 @@ def setup_task_routes(task_scheduler) -> APIRouter: if req.model is not None: task.model = req.model or None if req.endpoint_url is not None: - task.endpoint_url = req.endpoint_url or None + try: + # An empty override restores the existing default-model workflow. + task.endpoint_url = ( + resolve_owner_registered_endpoint_url(db, req.endpoint_url, user) + if req.endpoint_url else None + ) + except ValueError as exc: + raise HTTPException(400, str(exc)) from exc if req.trigger_type is not None: # Generate webhook token when switching to webhook trigger if req.trigger_type == "webhook" and not task.webhook_token: diff --git a/src/endpoint_resolver.py b/src/endpoint_resolver.py index d85a53f18..e83ee97e0 100644 --- a/src/endpoint_resolver.py +++ b/src/endpoint_resolver.py @@ -281,6 +281,29 @@ def same_endpoint_base(left, right) -> bool: return False +def resolve_owner_registered_endpoint(db, endpoint_url: str, owner: Optional[str] = None): + """Authorize a caller URL against enabled, owner-visible endpoint rows. + + Request credentials, query strings and fragments are never endpoint identity. + Return the server-owned row so runtime credentials come from registration. + """ + from src.auth_helpers import owner_filter + + if not isinstance(endpoint_url, str) or not same_endpoint_base(endpoint_url, endpoint_url): + raise ValueError("Invalid model endpoint URL") + query = db.query(ModelEndpoint).filter(ModelEndpoint.is_enabled.is_(True)) + for endpoint in owner_filter(query, ModelEndpoint, owner).all(): + if same_endpoint_base(endpoint_url, endpoint.base_url): + return endpoint + raise ValueError("Model endpoint must be enabled and registered for the current owner") + + +def resolve_owner_registered_endpoint_url(db, endpoint_url: str, owner: Optional[str] = None) -> str: + """Return only the registered canonical base, never the caller's URL.""" + endpoint = resolve_owner_registered_endpoint(db, endpoint_url, owner) + return normalize_base(endpoint.base_url) + + def _validated_endpoint_base(url: str) -> str: """Return a base URL that is safe for endpoint path appends.""" base = (url or "").strip().rstrip("/") diff --git a/static/js/markdown.js b/static/js/markdown.js index cabb624b3..b992c0abb 100644 --- a/static/js/markdown.js +++ b/static/js/markdown.js @@ -791,12 +791,13 @@ function renderSvgSandbox(source) { const height = viewBox ? Number(viewBox[2]) : 9; const ratio = Number.isFinite(width / height) && width > 0 && height > 0 ? Math.max(0.5, Math.min(3, width / height)) : (16 / 9); - // Parse in an inert template: nested/malformed SVG title markup stays text. + // XML parsing extracts text without inserting title markup into an HTML DOM. let title = 'Visual explanation'; - if (typeof document !== 'undefined') { - const template = document.createElement('template'); - template.innerHTML = cleaned; - title = template.content.querySelector?.('svg title')?.textContent?.trim() || title; + if (typeof DOMParser !== 'undefined') { + const svg = new DOMParser().parseFromString(cleaned, 'image/svg+xml'); + if (!svg.querySelector('parsererror')) { + title = svg.querySelector('svg title')?.textContent?.trim() || title; + } } const csp = "default-src 'none'; img-src 'none'; media-src 'none'; font-src 'none'; style-src 'unsafe-inline'"; const srcdoc = `${cleaned}`; diff --git a/static/js/sessions.js b/static/js/sessions.js index 2ff53986c..68b2a14c4 100644 --- a/static/js/sessions.js +++ b/static/js/sessions.js @@ -435,7 +435,7 @@ function getFolderNames() { async function moveToFolder(sessionId, folderName) { const fd = new FormData(); fd.append('folder', folderName || ''); - await fetch(`${API_BASE}/api/session/${sessionId}`, { method: 'PATCH', body: fd }); + await fetch(`${API_BASE}/api/session/${encodeURIComponent(sessionId)}`, { method: 'PATCH', body: fd }); // Update local data const s = sessions.find(x => x.id === sessionId); if (s) s.folder = folderName || null; diff --git a/tests/codeql_security_browser.cjs b/tests/codeql_security_browser.cjs index a7d6854c1..356a010ba 100644 --- a/tests/codeql_security_browser.cjs +++ b/tests/codeql_security_browser.cjs @@ -30,6 +30,8 @@ const { extractThemeBootstrap } = require('./helpers/theme_bootstrap.cjs'); // Expose the real internal loader only in this served test copy. await page.route('**/static/js/admin-codeql-harness.js', route => route.fulfill({ contentType: 'application/javascript', body: readFileSync('static/js/admin.js', 'utf8') + '\nexport { loadEndpoints };\n' })); + await page.route('**/static/js/sessions-codeql-harness.js', route => route.fulfill({ contentType: 'application/javascript', + body: readFileSync('static/js/sessions.js', 'utf8') + '\nexport { moveToFolder };\n' })); await page.goto(origin + '/security-harness'); const rendered = await page.evaluate(async () => { @@ -58,14 +60,19 @@ const { extractThemeBootstrap } = require('./helpers/theme_bootstrap.cjs'); } const valid = addMessage('user', 'In the document, edit this specific text (lines 1–2):\n```\nselected\n```\n\nInstruction: **Keep bold** and `code`'); const titles = [ + { source: 'Valid & safe', title: 'Valid & safe' }, { source: 'Nested bold & text', title: 'Nested bold & text' }, { source: '\" onload=\"parent.executed++ <script>', title: '\" onload=\"parent.executed++ nested', title: 'parent.executed++nested' }, + { source: '"><img src=x onerror="parent.executed++">', title: '">' }, + { source: '', title: 'Visual explanation' }, { source: ' ', title: 'Visual explanation' }, { source: 'No title', title: 'Visual explanation' }, ].map(({ source, title }) => { const host = document.createElement('div'); host.innerHTML = markdown.mdToHtml('```svg\n' + source + '\n```'); + document.body.appendChild(host); const frame = host.querySelector('iframe'); return { expected: title, actual: frame.title, sandbox: frame.getAttribute('sandbox'), referrer: frame.referrerPolicy, onload: frame.hasAttribute('onload'), @@ -92,6 +99,17 @@ const { extractThemeBootstrap } = require('./helpers/theme_bootstrap.cjs'); assert.equal(title.csp, "default-src 'none'; img-src 'none'; media-src 'none'; font-src 'none'; style-src 'unsafe-inline'"); } + const folderRequest = page.waitForRequest(request => request.method() === 'PATCH'); + const sessionId = 'session/other?folder=bad#fragment%value'; + await page.evaluate(async id => { + const { moveToFolder } = await import('/static/js/sessions-codeql-harness.js'); + await moveToFolder(id, 'Safe folder'); + }, sessionId); + const folderUrl = new URL((await folderRequest).url()); + assert.equal(folderUrl.pathname, '/api/session/' + encodeURIComponent(sessionId)); + assert.equal(folderUrl.search, ''); + assert.equal(folderUrl.hash, ''); + const menus = await page.evaluate(async () => { const { _showReaderMoreMenu } = await import('/static/js/emailLibrary/menus.js'); const { _safeRenderEmailBody } = await import('/static/js/emailLibrary/bodyRender.js'); diff --git a/tests/test_endpoint_registered_authority.py b/tests/test_endpoint_registered_authority.py new file mode 100644 index 000000000..8af81d748 --- /dev/null +++ b/tests/test_endpoint_registered_authority.py @@ -0,0 +1,249 @@ +"""Caller model URLs cannot exceed enabled, owner-visible registration.""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import HTTPException + +import core.database as database +import routes.assistant_routes as assistant_routes +import routes.skills_routes as skills_routes +import routes.task.task_routes as task_routes +import src.endpoint_resolver as resolver +from tests.helpers.database import disposable_database + + +LOCAL = "http://localhost:1234/v1" +LAN = "http://192.168.1.20:8000/v1" +SHARED = "http://127.0.0.1:11434/api" +REJECTED = [ + "https://unregistered.example/v1", + "http://169.254.169.254/latest/meta-data", + "http://bob.example/v1", + "http://disabled.example/v1", + "http://caller:secret@localhost:1234/v1", + LOCAL + "?api_key=caller-secret", + LOCAL + "#fragment", + LOCAL + "/other-base", + "not-a-url", + " ", +] + + +def _request(body=None, owner="alice"): + return SimpleNamespace( + state=SimpleNamespace(current_user=owner), + headers={}, + json=AsyncMock(return_value=body or {}), + ) + + +def _route(router, method, path): + return next(r.endpoint for r in router.routes if r.path == path and method in r.methods) + + +@pytest.fixture +def registered_db(tmp_path, monkeypatch): + with disposable_database(tmp_path) as factory: + for module in (database, task_routes, assistant_routes, resolver): + monkeypatch.setattr(module, "SessionLocal", factory) + with factory() as db: + for endpoint_id, owner, url, enabled in [ + ("local", "alice", LOCAL + "/", True), + ("lan", "alice", LAN, True), + ("shared", None, SHARED, True), + ("bob", "bob", "http://bob.example/v1", True), + ("disabled", "alice", "http://disabled.example/v1", False), + ]: + db.add(database.ModelEndpoint( + id=endpoint_id, name=endpoint_id, owner=owner, base_url=url, + is_enabled=enabled, api_key="server-secret", + )) + db.add(database.ScheduledTask( + id="task", owner="alice", name="Existing task", prompt="Work", + task_type="llm", trigger_type="webhook", status="active", + endpoint_url=LOCAL, + )) + db.add(database.CrewMember( + id="assistant", owner="alice", name="Assistant", + is_default_assistant=True, endpoint_url=LOCAL, + )) + db.commit() + yield factory + + +@pytest.mark.parametrize("url", REJECTED + ["", None, 42]) +def test_registered_endpoint_helper_rejects_invalid_or_invisible_url(registered_db, url): + with registered_db() as db, pytest.raises(ValueError): + resolver.resolve_owner_registered_endpoint_url(db, url, "alice") + + +@pytest.mark.parametrize("url, canonical", [ + ("HTTP://LOCALHOST:1234/v1/chat/completions/", LOCAL), + (LAN + "/models", LAN), + (SHARED + "/chat", SHARED), +]) +def test_registered_endpoint_helper_preserves_local_lan_and_shared(registered_db, url, canonical): + with registered_db() as db: + assert resolver.resolve_owner_registered_endpoint_url(db, url, "alice") == canonical + + +@pytest.mark.asyncio +@pytest.mark.parametrize("url", REJECTED) +async def test_task_create_rejects_unregistered_url_before_persistence(registered_db, url): + create = _route(task_routes.setup_task_routes(MagicMock()), "POST", "/api/tasks") + with pytest.raises(HTTPException) as exc: + await create(_request(), task_routes.TaskCreate( + name="Rejected", prompt="Work", trigger_type="webhook", endpoint_url=url, + )) + assert exc.value.status_code == 400 + with registered_db() as db: + assert db.query(database.ScheduledTask).count() == 1 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("url", REJECTED) +async def test_task_update_rejects_unregistered_url_before_persistence(registered_db, url): + update = _route(task_routes.setup_task_routes(MagicMock()), "PUT", "/api/tasks/{task_id}") + with pytest.raises(HTTPException) as exc: + await update(_request(), "task", task_routes.TaskUpdate(endpoint_url=url, name="Rejected")) + assert exc.value.status_code == 400 + with registered_db() as db: + task = db.get(database.ScheduledTask, "task") + assert task.endpoint_url == LOCAL + assert task.name == "Existing task" + + +@pytest.mark.asyncio +async def test_task_create_and_update_store_registered_canonical_url(registered_db): + router = task_routes.setup_task_routes(MagicMock()) + create = _route(router, "POST", "/api/tasks") + update = _route(router, "PUT", "/api/tasks/{task_id}") + result = await create(_request(), task_routes.TaskCreate( + name="Accepted", prompt="Work", trigger_type="webhook", + endpoint_url="HTTP://LOCALHOST:1234/v1/chat/completions/", + )) + assert result["endpoint_url"] == LOCAL + assert (await update(_request(), "task", task_routes.TaskUpdate( + endpoint_url=LOCAL + "/models", + )))["endpoint_url"] == LOCAL + with registered_db() as db: + created = db.get(database.ScheduledTask, result["id"]) + assert created.owner == "alice" + assert created.endpoint_url == LOCAL + assert created.request_authority_json + assert db.get(database.ScheduledTask, "task").endpoint_url == LOCAL + + +@pytest.mark.asyncio +async def test_task_empty_override_still_restores_default(registered_db): + router = task_routes.setup_task_routes(MagicMock()) + create = _route(router, "POST", "/api/tasks") + update = _route(router, "PUT", "/api/tasks/{task_id}") + result = await create(_request(), task_routes.TaskCreate( + name="Default", prompt="Work", trigger_type="webhook", endpoint_url="", + )) + assert result["endpoint_url"] is None + assert (await update(_request(), "task", task_routes.TaskUpdate(endpoint_url="")))["endpoint_url"] is None + + +@pytest.mark.asyncio +async def test_task_update_keeps_task_owner_verification(registered_db): + update = _route(task_routes.setup_task_routes(MagicMock()), "PUT", "/api/tasks/{task_id}") + with pytest.raises(HTTPException) as exc: + await update(_request(owner="bob"), "task", task_routes.TaskUpdate(endpoint_url=LOCAL)) + assert exc.value.status_code == 403 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("url", REJECTED) +async def test_assistant_settings_reject_unregistered_url(registered_db, url): + update = _route(assistant_routes.setup_assistant_routes(MagicMock()), "PATCH", "/api/assistant/settings") + with pytest.raises(HTTPException) as exc: + await update(assistant_routes.AssistantSettingsUpdate(endpoint_url=url, name="Rejected"), _request()) + assert exc.value.status_code == 400 + with registered_db() as db: + crew = db.get(database.CrewMember, "assistant") + assert crew.endpoint_url == LOCAL + assert crew.name == "Assistant" + + +@pytest.mark.asyncio +async def test_assistant_settings_accept_registered_local_endpoint(registered_db): + update = _route(assistant_routes.setup_assistant_routes(MagicMock()), "PATCH", "/api/assistant/settings") + await update(assistant_routes.AssistantSettingsUpdate(endpoint_url=LOCAL + "/chat/completions"), _request()) + with registered_db() as db: + assert db.get(database.CrewMember, "assistant").endpoint_url == LOCAL + + +@pytest.fixture +def skill_test_route(registered_db, monkeypatch): + manager = SimpleNamespace( + load=lambda owner: [{"name": "skill", "owner": "alice"}], + read_skill_md=lambda name, owner: "# Skill", + ) + monkeypatch.setattr(resolver, "resolve_endpoint", lambda *a, **kw: (None, None, None)) + probe = MagicMock(return_value=["model"]) + monkeypatch.setattr("src.llm_core.list_model_ids", probe) + run = AsyncMock() + monkeypatch.setattr(skills_routes, "_run_skill_test_job", run) + test = _route(skills_routes.setup_skills_routes(manager), "POST", "/api/skills/{skill_id}/test") + try: + yield test, probe, run + finally: + skills_routes._skill_test_jobs.pop(("alice", "skill"), None) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("url", REJECTED + [""]) +async def test_skill_test_rejects_raw_fallback_before_network_or_execution(skill_test_route, url): + test, probe, run = skill_test_route + with pytest.raises(HTTPException) as exc: + await test(_request({"endpoint_url": url, "model": "model", "headers": {"Authorization": "attacker"}}), "skill") + assert exc.value.status_code == 400 + probe.assert_not_called() + run.assert_not_called() + assert ("alice", "skill") not in skills_routes._skill_test_jobs + + +@pytest.mark.asyncio +@pytest.mark.parametrize("server_key", ["server-secret", None]) +async def test_skill_test_uses_registered_runtime_credentials(skill_test_route, registered_db, server_key): + import asyncio + + with registered_db() as db: + db.get(database.ModelEndpoint, "local").api_key = server_key + db.commit() + test, probe, run = skill_test_route + await test(_request({ + "endpoint_url": "HTTP://LOCALHOST:1234/v1/chat/completions/", "model": "model", + "headers": {"Authorization": "Bearer attacker", "x-api-key": "attacker", "Host": "169.254.169.254"}, + }), "skill") + await asyncio.sleep(0) + headers = {"Authorization": "Bearer server-secret"} if server_key else {} + probe.assert_called_once_with(LOCAL + "/chat/completions", headers=headers) + assert run.await_args.args[4:8] == (LOCAL + "/chat/completions", "model", headers, "alice") + assert skills_routes._skill_test_jobs[("alice", "skill")]["_run"]["headers"] == headers + + +@pytest.mark.asyncio +async def test_skill_test_prefers_utility_and_ignores_request_headers(skill_test_route, monkeypatch): + import asyncio + + test, probe, run = skill_test_route + monkeypatch.setattr(resolver, "resolve_endpoint", lambda *a, **kw: (LAN + "/chat/completions", "model", None)) + await test(_request({"endpoint_url": REJECTED[1], "headers": {"Authorization": "attacker"}}), "skill") + await asyncio.sleep(0) + probe.assert_called_once_with(LAN + "/chat/completions", headers=None) + assert run.await_args.args[6] is None + + +@pytest.mark.asyncio +async def test_skill_test_keeps_skill_owner_verification(skill_test_route): + test, probe, run = skill_test_route + with pytest.raises(HTTPException) as exc: + await test(_request({"endpoint_url": "http://bob.example/v1", "model": "model"}, owner="bob"), "skill") + assert exc.value.status_code == 404 + probe.assert_not_called() + run.assert_not_called() diff --git a/tests/test_markdown_dom_xss_helpers.py b/tests/test_markdown_dom_xss_helpers.py index a2dd63b16..0c3403706 100644 --- a/tests/test_markdown_dom_xss_helpers.py +++ b/tests/test_markdown_dom_xss_helpers.py @@ -7,6 +7,17 @@ from tests.helpers.document_source import document_source _REPO = Path(__file__).resolve().parent.parent +def test_svg_title_extraction_uses_xml_text_without_html_assignment(): + src = (_REPO / "static" / "js" / "markdown.js").read_text(encoding="utf-8") + render = src.split("function renderSvgSandbox(source)", 1)[1].split( + "function replaceRawSvgBlocks", 1 + )[0] + assert "innerHTML" not in render + assert "parseFromString(cleaned, 'image/svg+xml')" in render + assert "textContent" in render + assert "parsererror" in render + + def test_markdown_raw_html_sanitizer_checks_url_attr_edge_cases(): src = (_REPO / "static" / "js" / "markdown.js").read_text(encoding="utf-8")