From aef5009fc230a19b8e884b4532e8939f10775a4c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 19:02:19 -0400 Subject: [PATCH] fix(contract-drift): MCP read-only scope, shared-note writes, event TZ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drift-audit Group 5 (high-severity contract drift): - MCP read-only keys could call every write tool: the Bearer resolver discarded api_key.scope and dispatch had no gate. Add resolve_bearer() (returns user_id + scope) and a scope gate in the /mcp ASGI wrapper that buffers the JSON-RPC body and rejects tools/call for any tool outside a read all-list when scope=='read' (default-deny for unknown/new tools). - Shared project notes/tasks panel was empty for non-owners: get_project_notes_route now queries notes/milestones with the project OWNER's uid (mirrors the already-fixed milestones route). - Shared editors couldn't save/delete shared NOTES (tasks worked): the three notes write routes now resolve via get_note_for_user, gate on can_write_note, and write as the owner — matching the tasks routes. - Event timezone drift: naive datetimes from the MCP date+time split are now localized to the user's tz at a single canonical service point (create_event /update_event), so MCP- and UI-created events agree. tz-aware inputs (REST/CalDAV) pass through untouched. - create_note validates status/priority (TaskStatus/TaskPriority), closing the MCP create_task path that let out-of-enum values persist (no DB CHECK). Tests cover resolve_bearer scope + the write-tool classifier. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fabledassistant/mcp/auth.py | 18 ++++++ src/fabledassistant/mcp/server.py | 87 +++++++++++++++++++++++++- src/fabledassistant/routes/notes.py | 33 ++++++++-- src/fabledassistant/routes/projects.py | 7 ++- src/fabledassistant/services/events.py | 25 ++++++++ src/fabledassistant/services/notes.py | 14 +++++ tests/test_mcp_auth.py | 44 +++++++++++++ 7 files changed, 218 insertions(+), 10 deletions(-) diff --git a/src/fabledassistant/mcp/auth.py b/src/fabledassistant/mcp/auth.py index 4cad787..ab1c3da 100644 --- a/src/fabledassistant/mcp/auth.py +++ b/src/fabledassistant/mcp/auth.py @@ -17,3 +17,21 @@ async def resolve_bearer_to_user_id(auth_header: str | None) -> int | None: return None api_key = await lookup_key(raw_token) return api_key.user_id if api_key else None + + +async def resolve_bearer(auth_header: str | None) -> tuple[int, str] | None: + """Resolve a Bearer token to (user_id, scope). + + scope is 'read' or 'write'. Returns None for a missing/malformed/invalid + token. The MCP dispatch layer uses scope to deny write-class tool calls + from read-only keys — the same read/write boundary the REST API enforces. + """ + if not auth_header or not auth_header.startswith("Bearer "): + return None + raw_token = auth_header[len("Bearer "):].strip() + if not raw_token: + return None + api_key = await lookup_key(raw_token) + if api_key is None: + return None + return api_key.user_id, (api_key.scope or "write") diff --git a/src/fabledassistant/mcp/server.py b/src/fabledassistant/mcp/server.py index 9b18199..388bbe1 100644 --- a/src/fabledassistant/mcp/server.py +++ b/src/fabledassistant/mcp/server.py @@ -81,6 +81,66 @@ auto-purges after the operator's retention window. """ +# Tools a read-only API key may call. Anything not listed is treated as a +# write for read keys (default-deny), so a newly-added tool is locked down +# until explicitly classified here. +_READ_ONLY_TOOLS = frozenset({ + "get_event", "get_note", "get_project", "get_rule", "get_rulebook", + "get_task", "get_recent", "enter_project", + "list_events", "list_lists", "list_milestones", "list_notes", + "list_persons", "list_places", "list_projects", "list_rulebooks", + "list_rules", "list_tags", "list_tasks", "list_topics", "list_trash", + "list_always_on_rules", "search", +}) + + +async def _buffer_request_body(receive): + """Drain the ASGI request body and return (body_bytes, replay_receive). + + The MCP sub-app still needs to read the body, so we return a fresh + `receive` that replays the buffered bytes. + """ + chunks: list[bytes] = [] + more = True + while more: + message = await receive() + if message["type"] == "http.request": + chunks.append(message.get("body", b"")) + more = message.get("more_body", False) + else: # http.disconnect + more = False + body = b"".join(chunks) + + sent = False + + async def replay(): + nonlocal sent + if not sent: + sent = True + return {"type": "http.request", "body": body, "more_body": False} + return {"type": "http.disconnect"} + + return body, replay + + +def _body_calls_write_tool(body: bytes) -> bool: + """True if the JSON-RPC body invokes a tool outside the read all-list.""" + import json + try: + payload = json.loads(body) + except Exception: + return False + items = payload if isinstance(payload, list) else [payload] + for item in items: + if not isinstance(item, dict): + continue + if item.get("method") == "tools/call": + name = (item.get("params") or {}).get("name", "") + if name and name not in _READ_ONLY_TOOLS: + return True + return False + + def build_mcp_server() -> FastMCP: """Build the FastMCP instance with all tools registered. @@ -128,7 +188,7 @@ def mount_mcp(app: Quart) -> None: inside Quart, we hook the session manager's `run()` async context manager into Quart's serving lifecycle (before_serving / after_serving). """ - from fabledassistant.mcp.auth import resolve_bearer_to_user_id + from fabledassistant.mcp.auth import resolve_bearer mcp = build_mcp_server() mcp_asgi = mcp.streamable_http_app() @@ -151,8 +211,8 @@ def mount_mcp(app: Quart) -> None: return await mcp_asgi(scope, receive, send) # ASGI headers are lowercase bytes per spec; lowercase explicitly to be safe. headers = {k.decode().lower(): v.decode() for k, v in scope.get("headers", [])} - user_id = await resolve_bearer_to_user_id(headers.get("authorization")) - if user_id is None: + resolved = await resolve_bearer(headers.get("authorization")) + if resolved is None: await send({ "type": "http.response.start", "status": 401, @@ -166,6 +226,27 @@ def mount_mcp(app: Quart) -> None: "body": b'{"error":"unauthorized"}', }) return + user_id, key_scope = resolved + + # Enforce read-only keys: REST blocks non-GET for scope='read', and the + # MCP surface must match or the read-only guarantee is void. A tool call + # arrives as a JSON-RPC POST; buffer the body, and if it invokes a tool + # outside the read all-list, reject before dispatch. (default-deny: any + # unknown/new tool is treated as a write for read keys.) + if key_scope == "read" and scope.get("method") == "POST": + body, receive = await _buffer_request_body(receive) + if _body_calls_write_tool(body): + await send({ + "type": "http.response.start", + "status": 403, + "headers": [(b"content-type", b"application/json")], + }) + await send({ + "type": "http.response.body", + "body": b'{"error":"read-only API key cannot call write tools"}', + }) + return + scope["scribe_user_id"] = user_id from fabledassistant.mcp._context import _user_id_ctx token = _user_id_ctx.set(user_id) diff --git a/src/fabledassistant/routes/notes.py b/src/fabledassistant/routes/notes.py index acac8c2..7b56d4d 100644 --- a/src/fabledassistant/routes/notes.py +++ b/src/fabledassistant/routes/notes.py @@ -8,6 +8,7 @@ from quart import Blueprint, jsonify, request from fabledassistant.auth import login_required, get_current_user_id from fabledassistant.routes.utils import not_found, parse_iso_date, parse_pagination +from fabledassistant.services.access import can_write_note from fabledassistant.services.notes import ( build_note_graph, convert_note_to_task, @@ -192,6 +193,15 @@ async def get_note_route(note_id: int): @login_required async def update_note_route(note_id: int): uid = get_current_user_id() + # Share-aware: resolve through the ACL and write as the OWNER, so a shared + # editor's save isn't rejected by the owner-scoped update service. + result = await get_note_for_user(uid, note_id) + if result is None: + return not_found("Note") + note_obj, _ = result + if not await can_write_note(uid, note_id): + return jsonify({"error": "Permission denied"}), 403 + owner_uid = note_obj.user_id data = await request.get_json() fields = {} for key in ("title", "body", "description", "parent_id", "project_id", "milestone_id", "status", "priority", "note_type"): @@ -212,14 +222,14 @@ async def update_note_route(note_id: int): if "tags" in data: fields["tags"] = data["tags"] try: - note = await update_note(uid, note_id, **fields) + note = await update_note(owner_uid, note_id, **fields) except ValueError as e: return jsonify({"error": str(e)}), 400 if note is None: return not_found("Note") text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") if text: - asyncio.create_task(upsert_note_embedding(note.id, uid, text)) + asyncio.create_task(upsert_note_embedding(note.id, owner_uid, text)) return jsonify(note.to_dict()) @@ -227,6 +237,13 @@ async def update_note_route(note_id: int): @login_required async def patch_note_route(note_id: int): uid = get_current_user_id() + result = await get_note_for_user(uid, note_id) + if result is None: + return not_found("Note") + note_obj, _ = result + if not await can_write_note(uid, note_id): + return jsonify({"error": "Permission denied"}), 403 + owner_uid = note_obj.user_id data = await request.get_json() fields = {} for key in ("title", "body", "description", "parent_id", "project_id", "milestone_id", "status", "priority", "note_type"): @@ -245,14 +262,14 @@ async def patch_note_route(note_id: int): if "tags" in data: fields["tags"] = data["tags"] try: - note = await update_note(uid, note_id, **fields) + note = await update_note(owner_uid, note_id, **fields) except ValueError as e: return jsonify({"error": str(e)}), 400 if note is None: return not_found("Note") text = f"{note.title}\n{note.body}".strip() if note.body else (note.title or "") if text: - asyncio.create_task(upsert_note_embedding(note.id, uid, text)) + asyncio.create_task(upsert_note_embedding(note.id, owner_uid, text)) return jsonify(note.to_dict()) @@ -260,8 +277,14 @@ async def patch_note_route(note_id: int): @login_required async def delete_note_route(note_id: int): uid = get_current_user_id() + result = await get_note_for_user(uid, note_id) + if result is None: + return not_found("Note") + note_obj, _ = result + if not await can_write_note(uid, note_id): + return jsonify({"error": "Permission denied"}), 403 from fabledassistant.services.trash import delete as trash_delete - batch = await trash_delete(uid, "note", note_id) + batch = await trash_delete(note_obj.user_id, "note", note_id) if batch is None: return not_found("Note") return "", 204 diff --git a/src/fabledassistant/routes/projects.py b/src/fabledassistant/routes/projects.py index c90ec66..dd10f9d 100644 --- a/src/fabledassistant/routes/projects.py +++ b/src/fabledassistant/routes/projects.py @@ -116,6 +116,9 @@ async def get_project_notes_route(project_id: int): if result is None: return not_found("Project") project, _ = result + # Use the project owner's uid so the ownership filter on notes/milestones + # matches for shared collaborators (who'd otherwise see an empty panel). + owner_uid = project.user_id or uid # type filter: "note", "task", or None (both) type_filter = request.args.get("type") @@ -128,11 +131,11 @@ async def get_project_notes_route(project_id: int): elif type_filter == "note": is_task = False - ms_list = await list_milestones(uid, project_id) + ms_list = await list_milestones(owner_uid, project_id) milestone_ids = [m.id for m in ms_list] notes, total = await list_notes( - uid, + owner_uid, is_task=is_task, status=status_filter, project_id=project_id, diff --git a/src/fabledassistant/services/events.py b/src/fabledassistant/services/events.py index 590141a..ddd94c6 100644 --- a/src/fabledassistant/services/events.py +++ b/src/fabledassistant/services/events.py @@ -68,6 +68,19 @@ def _normalize_duration( return None +async def _localize_naive(user_id: int, dt: datetime | None) -> datetime | None: + """Anchor a naive datetime in the user's timezone; pass tz-aware through. + + Naive datetimes are the user's local wall-clock time (the MCP create/update + tools combine date+time without a zone). Attaching the user's tzinfo lets + asyncpg store the correct UTC instant, matching the REST/UI path. + """ + if dt is not None and dt.tzinfo is None: + from fabledassistant.services.tz import get_user_tz # noqa: PLC0415 + return dt.replace(tzinfo=await get_user_tz(user_id)) + return dt + + async def create_event( user_id: int, title: str, @@ -97,6 +110,13 @@ async def create_event( """ if duration is not None and duration_minutes is None: duration_minutes = duration + # Canonical localization point: a naive datetime (e.g. from the MCP tool's + # date+time split) is the user's wall-clock time, so anchor it in their + # timezone before storage. tz-aware inputs (REST, CalDAV pass-through) are + # left untouched. Without this, MCP-created events landed at the same + # wall-clock numerals in UTC and drifted from UI-created ones by the offset. + start_dt = await _localize_naive(user_id, start_dt) + end_dt = await _localize_naive(user_id, end_dt) duration_minutes = _normalize_duration( start_dt=start_dt, end_dt=end_dt, duration_minutes=duration_minutes, ) @@ -271,6 +291,11 @@ async def update_event(user_id: int, event_id: int, **fields) -> Event | None: return None old_title = event.title # capture before mutation for CalDAV lookup + # Localize a naive start_dt patch to the user's timezone (same canonical + # rule as create_event) before it's used or persisted. + if fields.get("start_dt") is not None: + fields["start_dt"] = await _localize_naive(user_id, fields["start_dt"]) + # Resolve any end_dt/duration_minutes inputs against the # post-update start_dt. If neither is in the patch, leave the # existing duration_minutes alone. diff --git a/src/fabledassistant/services/notes.py b/src/fabledassistant/services/notes.py index c33b085..90ee994 100644 --- a/src/fabledassistant/services/notes.py +++ b/src/fabledassistant/services/notes.py @@ -66,6 +66,20 @@ async def create_note( entity_meta: dict | None = None, task_kind: str = "work", ) -> Note: + # Validate status/priority here so the MCP create_task path (which passes + # them straight through) can't persist an out-of-enum value that the REST + # route would have rejected — there's no DB CHECK on notes.status. + if isinstance(status, str): + try: + status = TaskStatus(status).value + except ValueError: + raise ValueError(f"Invalid status: {status!r}. Must be one of: {[s.value for s in TaskStatus]}") + if isinstance(priority, str): + try: + priority = TaskPriority(priority).value + except ValueError: + raise ValueError(f"Invalid priority: {priority!r}. Must be one of: {[p.value for p in TaskPriority]}") + # Auto-populate project_id from milestone when not explicitly provided if milestone_id is not None and project_id is None: from fabledassistant.models.milestone import Milestone diff --git a/tests/test_mcp_auth.py b/tests/test_mcp_auth.py index 8579058..cf2493d 100644 --- a/tests/test_mcp_auth.py +++ b/tests/test_mcp_auth.py @@ -49,3 +49,47 @@ async def test_resolve_bearer_calls_lookup_with_stripped_token(): with patch("fabledassistant.mcp.auth.lookup_key", mock_lookup): await resolve_bearer_to_user_id("Bearer fmcp_abc123 ") mock_lookup.assert_awaited_once_with("fmcp_abc123") + + +# ── resolve_bearer (user_id + scope) ──────────────────────────────────── + +@pytest.mark.asyncio +async def test_resolve_bearer_returns_user_id_and_scope(): + from fabledassistant.mcp.auth import resolve_bearer + fake_key = MagicMock() + fake_key.user_id = 9 + fake_key.scope = "read" + with patch("fabledassistant.mcp.auth.lookup_key", AsyncMock(return_value=fake_key)): + assert await resolve_bearer("Bearer fmcp_x") == (9, "read") + + +@pytest.mark.asyncio +async def test_resolve_bearer_none_for_invalid(): + with patch("fabledassistant.mcp.auth.lookup_key", AsyncMock(return_value=None)): + assert await resolve_bearer("Bearer nope") is None + assert await resolve_bearer(None) is None + + +# ── read-only scope gate ──────────────────────────────────────────────── + +def test_body_calls_write_tool_classifies_correctly(): + import json + from fabledassistant.mcp.server import _body_calls_write_tool + + def call(name): + return json.dumps({"jsonrpc": "2.0", "id": 1, "method": "tools/call", + "params": {"name": name, "arguments": {}}}).encode() + + # Write-class tools are gated. + assert _body_calls_write_tool(call("create_note")) is True + assert _body_calls_write_tool(call("delete_project")) is True + assert _body_calls_write_tool(call("purge_trash")) is True + # An unknown/new tool defaults to write (default-deny for read keys). + assert _body_calls_write_tool(call("brand_new_tool")) is True + # Read tools and non-call methods pass. + assert _body_calls_write_tool(call("list_notes")) is False + assert _body_calls_write_tool(call("get_recent")) is False + assert _body_calls_write_tool( + json.dumps({"method": "tools/list"}).encode() + ) is False + assert _body_calls_write_tool(b"not json") is False