From 9150a453b415bef9e45852a74973a0c9110e43a4 Mon Sep 17 00:00:00 2001 From: RaresKeY <158580472+RaresKeY@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:07:40 +0000 Subject: [PATCH] fix(security): close API token agent authorization gaps --- core/models.py | 33 +- routes/assistant_routes.py | 12 +- routes/chat_helpers.py | 1 + routes/chat_routes.py | 113 ++++-- routes/codex_routes.py | 24 +- routes/history/history_routes.py | 14 +- routes/model_routes.py | 10 +- routes/research/research_routes.py | 11 +- routes/search/search_routes.py | 10 +- routes/session_routes.py | 18 + routes/skills_routes.py | 12 +- routes/task/task_routes.py | 14 +- routes/upload_routes.py | 6 + routes/webhook/webhook_routes.py | 22 +- src/auth_helpers.py | 78 +++- src/chat_processor.py | 3 +- src/message_metadata.py | 20 +- tests/test_api_chat_security.py | 57 ++- tests/test_api_token_forward_fixes.py | 554 ++++++++++++++++++++++++++ tests/test_null_owner_gates.py | 9 +- 20 files changed, 907 insertions(+), 114 deletions(-) create mode 100644 tests/test_api_token_forward_fixes.py diff --git a/core/models.py b/core/models.py index 0c8bff5f3..513cc0883 100644 --- a/core/models.py +++ b/core/models.py @@ -151,11 +151,34 @@ class Session: the model. Display/history-load paths use the raw ``history`` and are unaffected. """ - messages = [ - msg.to_dict() - for msg in self.history - if (msg.metadata or {}).get("source") != "slash" - ] + messages = [] + for msg in self.history: + raw_metadata = getattr(msg, "metadata", None) + if isinstance(raw_metadata, dict) and raw_metadata.get("source") == "slash": + continue + projected = msg.to_dict() + metadata = projected.get("metadata") + if not isinstance(metadata, dict): + # Old or malformed durable rows must not make context + # projection fail, and non-mapping metadata has no trusted + # fields that belong in the model context. + projected.pop("metadata", None) + messages.append(projected) + continue + if isinstance(metadata, dict) and CHAT_SESSION_APPROVAL_CONTEXT_MARKER in metadata: + # The marker is derived below from a verified persisted + # approval event. Never pass a raw durable/client marker + # through to the model context. + metadata = { + key: value + for key, value in metadata.items() + if key != CHAT_SESSION_APPROVAL_CONTEXT_MARKER + } + if metadata: + projected["metadata"] = metadata + else: + projected.pop("metadata", None) + messages.append(projected) if not _history_grants_chat_session_approval(self.history, self.id): return messages diff --git a/routes/assistant_routes.py b/routes/assistant_routes.py index f16f016e9..6bbd2ea4b 100644 --- a/routes/assistant_routes.py +++ b/routes/assistant_routes.py @@ -11,11 +11,11 @@ import json from datetime import datetime from typing import Optional -from fastapi import APIRouter, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel from core.database import SessionLocal, CrewMember, ScheduledTask -from src.auth_helpers import get_current_user +from src.auth_helpers import require_interactive_request from src.owner_identity import REQUEST_SENTINEL_OWNERS from src.task_scheduler import compute_next_run @@ -78,10 +78,14 @@ def _task_to_checkin_dict(t: ScheduledTask) -> dict: def setup_assistant_routes(task_scheduler) -> APIRouter: - router = APIRouter(prefix="/api/assistant", tags=["assistant"]) + router = APIRouter( + prefix="/api/assistant", + tags=["assistant"], + dependencies=[Depends(require_interactive_request)], + ) def _owner(request: Request) -> str: - owner = get_current_user(request) + owner = require_interactive_request(request) if not owner: raise HTTPException(status_code=401, detail="Not authenticated") return owner diff --git a/routes/chat_helpers.py b/routes/chat_helpers.py index 3d87da2b0..15b492f88 100644 --- a/routes/chat_helpers.py +++ b/routes/chat_helpers.py @@ -731,6 +731,7 @@ async def build_chat_context( agent_mode=agent_mode, incognito=incognito, use_skills=skills_enabled, + allow_tool_preprocessing=allow_tool_preprocessing, ) if use_rag is not None or is_research_spinoff or casual_low_signal: _preface_kwargs["use_rag"] = use_rag_val diff --git a/routes/chat_routes.py b/routes/chat_routes.py index 968dbd8cd..113a925b2 100644 --- a/routes/chat_routes.py +++ b/routes/chat_routes.py @@ -9,7 +9,7 @@ import logging from datetime import datetime from typing import Dict, Any, AsyncGenerator, List, Optional -from fastapi import APIRouter, Request, HTTPException, Form, Query +from fastapi import APIRouter, Depends, Request, HTTPException, Form, Query from fastapi.responses import StreamingResponse from pydantic import ValidationError @@ -45,6 +45,7 @@ from src.auth_helpers import ( enforce_api_token_chat_controls, get_current_user, require_chat_scope, + require_interactive_request, ) from routes.session_routes import _verify_session_owner from routes.document_helpers import _owner_session_filter @@ -736,7 +737,7 @@ def setup_chat_routes( webhook_manager=None, skills_manager=None, ) -> APIRouter: - router = APIRouter(tags=["chat"]) + router = APIRouter(tags=["chat"], dependencies=[Depends(require_chat_scope)]) # ------------------------------------------------------------------ # # POST /api/chat (non-streaming) @@ -754,6 +755,9 @@ def setup_chat_routes( time_filter = chat_request.time_filter preset_id = chat_request.preset_id + if getattr(request.state, "api_token", False) is True and use_research: + raise HTTPException(403, "API tokens cannot use research or agent execution") + # Verify the caller owns this session before loading it. # Without this, any authenticated user can post into another user's chat. _verify_session_owner(request, session) @@ -782,15 +786,16 @@ def setup_chat_routes( # non-streaming path can't be used to bypass). _enforce_chat_privileges(request, sess) + api_token_request = getattr(request.state, "api_token", False) is True tool_policy = build_effective_tool_policy(last_user_message=message) allow_tool_preprocessing = ( - not getattr(request.state, "api_token", False) + not api_token_request and not tool_policy.block_all_tool_calls ) # Inline memory command memory_response = None - if not tool_policy.blocks("manage_memory"): + if allow_tool_preprocessing and not tool_policy.blocks("manage_memory"): memory_response = await chat_handler.handle_memory_command(sess, message) if memory_response: return {"response": memory_response} @@ -819,7 +824,7 @@ def setup_chat_routes( tool_policy.blocks("trigger_research") or tool_policy.blocks("manage_research") ) - if use_research and not research_blocked_by_policy: + if use_research and not api_token_request and not research_blocked_by_policy: try: _r_ep, _r_model, _r_headers = _resolve_research_endpoint(sess) research_ctx = await research_handler.call_research_service( @@ -919,7 +924,7 @@ def setup_chat_routes( ctx.uprefs, memory_manager, memory_vector, webhook_manager, character_name=ctx.preset.character_name, owner=ctx.user, - allow_background_extraction=not tool_policy.block_all_tool_calls, + allow_background_extraction=allow_tool_preprocessing, ) return { @@ -958,6 +963,8 @@ def setup_chat_routes( attachments = form_data.get("attachments") use_web = form_data.get("use_web") use_research = form_data.get("use_research") + if use_research is None: + use_research = (body or {}).get("use_research") time_filter = form_data.get("time_filter") preset_id = form_data.get("preset_id") selected_endpoint_id = str( @@ -996,10 +1003,35 @@ def setup_chat_routes( approval_id=tool_approval_id, allow_bash=allow_bash, ) + + # A bearer token is a chat-only integration credential. Reject every + # remaining control-plane input before approval lookup, intent + # detection, context shaping, or active-email/workspace resolution can + # turn the request into an interactive agent turn. + approved_plan = "" + if not plan_mode: + approved_plan = str( + form_data.get("approved_plan") + or (body or {}).get("approved_plan") + or "" + ).strip()[:8192] + if api_token_request and ( + str(use_research).lower() == "true" + or bool(tool_approval_decision) + or bool(approved_plan) + ): + raise HTTPException( + 403, + "API tokens cannot use research, agent, plan, or tool approvals", + ) + # Workspace: confine the agent's file/shell tools to this folder. - workspace, workspace_rejected = _resolve_request_workspace( - request, form_data.get("workspace") - ) + if api_token_request: + workspace, workspace_rejected = "", "" + else: + workspace, workspace_rejected = _resolve_request_workspace( + request, form_data.get("workspace") + ) # Plan mode is a modifier on agent mode — it only makes sense with tools. if plan_mode: chat_mode = "agent" @@ -1008,9 +1040,6 @@ def setup_chat_routes( # weak model survives history truncation — the agent can always re-read # the plan. Ignored while still proposing (plan_mode on). Capped so a # huge plan can't blow the prompt. - approved_plan = "" - if not plan_mode: - approved_plan = (form_data.get("approved_plan") or "").strip()[:8192] # Did the USER explicitly pick agent mode? (vs. us auto-escalating # below). Skill extraction should only learn from real agent sessions, # not chats we quietly promoted for a notes/calendar intent. @@ -1045,7 +1074,7 @@ def setup_chat_routes( auto_escalated = False _tool_intent = _classify_tool_intent(message) if isinstance(message, str) else None _workspace_agent_intent = False - if chat_mode == "chat" and _tool_intent and _tool_intent.needs_tools: + if not api_token_request and chat_mode == "chat" and _tool_intent and _tool_intent.needs_tools: chat_mode = "agent" auto_escalated = True _workspace_agent_intent = _tool_intent.category in {"shell", "workspace"} @@ -1056,24 +1085,28 @@ def setup_chat_routes( _tool_intent.category, _tool_intent.reason, ) - elif chat_mode == "chat" and _search_enabled: + elif not api_token_request and chat_mode == "chat" and _search_enabled: chat_mode = "agent" auto_escalated = True logger.info("chat→agent auto-escalation: search enabled") - elif chat_mode == "chat" and _explicit_web_intent: + elif not api_token_request and chat_mode == "chat" and _explicit_web_intent: chat_mode = "agent" auto_escalated = True logger.info("chat→agent auto-escalation: explicit web intent") - active_doc_id = form_data.get("active_doc_id", "").strip() + active_doc_id = "" if api_token_request else str(form_data.get("active_doc_id") or "").strip() logger.info(f"[doc-inject] chat_mode={chat_mode}, active_doc_id={active_doc_id!r}") # Active email reader — when the user has an email open in the UI, the # frontend passes its uid/folder/account so "reply", "summarize this", # etc. resolve to the real email instead of the agent inventing a # fake markdown draft. - active_email_uid = form_data.get("active_email_uid", "").strip() - active_email_folder = form_data.get("active_email_folder", "INBOX").strip() or "INBOX" - active_email_account = form_data.get("active_email_account", "").strip() + active_email_uid = "" if api_token_request else str(form_data.get("active_email_uid") or "").strip() + active_email_folder = ( + "INBOX" + if api_token_request + else str(form_data.get("active_email_folder") or "INBOX").strip() or "INBOX" + ) + active_email_account = "" if api_token_request else str(form_data.get("active_email_account") or "").strip() active_email_ctx: Optional[Dict[str, str]] = None # Always reset between requests so a stale active-email pointer from # a previous turn (different reader closed, different account, etc.) @@ -1246,7 +1279,8 @@ def setup_chat_routes( if not (getattr(sess, "endpoint_url", "") or "").strip(): raise HTTPException(400, "Selected model endpoint is not configured") if ( - chat_mode == "chat" + not api_token_request + and chat_mode == "chat" and isinstance(message, str) and (not _tool_intent or not _tool_intent.needs_tools) and _is_contextual_web_followup(message, sess) @@ -1260,14 +1294,14 @@ def setup_chat_routes( _tool_intent.category, _tool_intent.reason, ) - if isinstance(message, str) and _is_contextual_browser_followup(message, sess): + if not api_token_request and isinstance(message, str) and _is_contextual_browser_followup(message, sess): _explicit_browser_intent = True if chat_mode == "chat": chat_mode = "agent" auto_escalated = True _workspace_agent_intent = False logger.info("chat→agent auto-escalation: contextual browser/form follow-up") - if not workspace and isinstance(message, str): + if not api_token_request and not workspace and isinstance(message, str): _auto_workspace, _ = _resolve_workspace_from_message_path(request, message) if _auto_workspace: workspace = _auto_workspace @@ -1309,6 +1343,8 @@ def setup_chat_routes( # mutable form fields, or a stale research_pending session marker, # consume the one-use grant on the unrelated research path. do_research = ( + not api_token_request + and not tool_approval_continuation and str(use_research).lower() == "true" ) @@ -1330,7 +1366,11 @@ def setup_chat_routes( except Exception as e: logger.warning("Failed to parse attachments JSON, ignoring attachments", exc_info=e) - image_generation_session = _is_image_generation_session(sess, owner=effective_user(request)) + image_generation_session = _is_image_generation_session( + sess, owner=effective_user(request) + ) + if api_token_request and image_generation_session: + raise HTTPException(403, "API tokens cannot use image generation") no_memory = str(form_data.get("no_memory", "")).lower() == "true" if image_generation_session: no_memory = True @@ -1845,7 +1885,7 @@ def setup_chat_routes( yield f'data: {json.dumps(_model_info)}\n\n' _terminal_saved = False - if _is_image_generation_session(sess, owner=_user): + if image_generation_session: from src.settings import get_setting if tool_policy.blocks("generate_image"): _blocked_msg = tool_policy.reason_for("generate_image") @@ -2256,7 +2296,7 @@ def setup_chat_routes( character_name=ctx.preset.character_name, owner=_user, allow_background_extraction=( - not tool_policy.block_all_tool_calls + allow_tool_preprocessing and not tool_approval_continuation ), ) @@ -2530,7 +2570,7 @@ def setup_chat_routes( and not tool_approval_continuation ), allow_background_extraction=( - not tool_policy.block_all_tool_calls + allow_tool_preprocessing and not tool_approval_continuation ), ) @@ -2621,8 +2661,12 @@ def setup_chat_routes( # GET /api/chat/resume — reconnect to a detached run that's still going # (e.g. after reopening a session whose agent kept running in the background) # ------------------------------------------------------------------ # - @router.get("/api/chat/resume/{session_id}") + @router.get( + "/api/chat/resume/{session_id}", + dependencies=[Depends(require_interactive_request)], + ) async def chat_resume(request: Request, session_id: str) -> StreamingResponse: + require_interactive_request(request) _verify_session_owner(request, session_id) _active_run = agent_runs.get_active_run(session_id) if _active_run is None: @@ -2637,8 +2681,12 @@ def setup_chat_routes( # POST /api/chat/stop — cancel a detached run (Stop button). Closing the SSE # no longer stops it (it's detached), so the Stop button must call this. # ------------------------------------------------------------------ # - @router.post("/api/chat/stop/{session_id}") + @router.post( + "/api/chat/stop/{session_id}", + dependencies=[Depends(require_interactive_request)], + ) async def chat_stop(request: Request, session_id: str) -> Dict[str, Any]: + require_interactive_request(request) _verify_session_owner(request, session_id) _expected_run_id = request.headers.get("X-Odysseus-Run-Id") stopped = agent_runs.stop(session_id, _expected_run_id) @@ -2647,8 +2695,12 @@ def setup_chat_routes( # ------------------------------------------------------------------ # # GET /api/chat/stream_status — check if a stream is active for a session # ------------------------------------------------------------------ # - @router.get("/api/chat/stream_status/{session_id}") + @router.get( + "/api/chat/stream_status/{session_id}", + dependencies=[Depends(require_interactive_request)], + ) async def chat_stream_status(request: Request, session_id: str) -> Dict[str, Any]: + require_interactive_request(request) _verify_session_owner(request, session_id) # A detached run can still be going even if _active_streams was popped; # report it as active so the client knows to reconnect via /resume. @@ -2667,6 +2719,7 @@ def setup_chat_routes( # ------------------------------------------------------------------ # @router.post("/api/inject_context/{session_id}") async def inject_context(request: Request, session_id: str, context: str = Form(...)) -> Dict[str, str]: + require_chat_scope(request) _verify_session_owner(request, session_id) try: sess = session_manager.get_session(session_id) @@ -2686,6 +2739,7 @@ def setup_chat_routes( q: str = Query("", min_length=0), limit: int = Query(20, ge=1, le=100), ) -> List[Dict[str, Any]]: + require_chat_scope(request) if not q or not q.strip(): return [] @@ -2711,6 +2765,7 @@ def setup_chat_routes( Unlike the full chat pipeline, this does NOT run the agent loop or tools. It just asks the LLM to rewrite the given text. """ + require_chat_scope(request) try: body = await request.json() except Exception: diff --git a/routes/codex_routes.py b/routes/codex_routes.py index 9fe36a822..f4e4467b7 100644 --- a/routes/codex_routes.py +++ b/routes/codex_routes.py @@ -16,7 +16,11 @@ from fastapi import APIRouter, BackgroundTasks, Body, HTTPException, Request from fastapi.responses import StreamingResponse from core.middleware import require_admin -from src.auth_helpers import require_authenticated_request, require_user +from src.auth_helpers import ( + require_api_token_owner, + require_authenticated_request, + require_user, +) from src.tool_implementations import do_manage_notes from src.constants import COOKBOOK_STATE_FILE from routes._validators import validate_remote_host, validate_ssh_port @@ -84,29 +88,23 @@ async def _as_owner(request: Request, owner: str, fn, *args, **kwargs): def _scope_owner(request: Request, allowed: set[str]) -> str: """Return the data owner if the caller is allowed for this Codex action.""" - if getattr(request.state, "api_token", False): + if getattr(request.state, "api_token", False) is True: scopes = set(getattr(request.state, "api_token_scopes", []) or []) if not scopes.intersection(allowed): required = " or ".join(sorted(allowed)) raise HTTPException(403, f"API token missing required scope: {required}") - owner = getattr(request.state, "api_token_owner", None) - if not owner: - raise HTTPException(403, "API token has no owner") - return owner + return require_api_token_owner(request) return require_user(request) def _scope_owner_all(request: Request, required: set[str]) -> str: """Return owner only when an API token has every required scope.""" - if getattr(request.state, "api_token", False): + if getattr(request.state, "api_token", False) is True: scopes = set(getattr(request.state, "api_token_scopes", []) or []) missing = required - scopes if missing: raise HTTPException(403, f"API token missing required scope: {' and '.join(sorted(missing))}") - owner = getattr(request.state, "api_token_owner", None) - if not owner: - raise HTTPException(403, "API token has no owner") - return owner + return require_api_token_owner(request) return require_user(request) @@ -119,7 +117,7 @@ def _require_cookbook_scope(request: Request, allowed: set[str]) -> str: commands, and model-serving controls. """ owner = _scope_owner(request, allowed) - if not getattr(request.state, "api_token", False): + if getattr(request.state, "api_token", False) is not True: require_admin(request) return owner @@ -167,7 +165,7 @@ def setup_codex_routes( @router.get("/capabilities") def capabilities(request: Request): token_scopes = set(getattr(request.state, "api_token_scopes", []) or []) - has_token = bool(getattr(request.state, "api_token", False)) + has_token = getattr(request.state, "api_token", False) is True def scoped(allowed): return bool(token_scopes.intersection(allowed)) if has_token else True return { diff --git a/routes/history/history_routes.py b/routes/history/history_routes.py index bbf5ea933..5d3c8d0ec 100644 --- a/routes/history/history_routes.py +++ b/routes/history/history_routes.py @@ -145,6 +145,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: limit: Optional[int] = None, offset: Optional[int] = None, ) -> Dict[str, Any]: + require_chat_scope(request) _verify_session_owner(request, session_id) if limit is not None: page_limit = max(1, min(int(limit), 100)) @@ -247,6 +248,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/truncate") async def truncate_session(request: Request, session_id: str): + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -262,6 +264,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/message") async def add_message(request: Request, session_id: str): """Add a message to a session (for slash command persistence).""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -280,6 +283,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/delete-messages") async def delete_messages(request: Request, session_id: str): """Delete specific messages by DB ID (or legacy index).""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -343,6 +347,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/edit-message") async def edit_message(request: Request, session_id: str): """Edit the content of a message by its database ID.""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -398,6 +403,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/mark-stopped") async def mark_stopped(request: Request, session_id: str): """Mark the last assistant message as stopped by user.""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: session = session_manager.get_session(session_id) @@ -453,10 +459,11 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/update-last-meta") async def update_last_meta(request: Request, session_id: str): """Merge metadata into the last assistant message (e.g. save variants).""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() - meta_update = sanitize_client_message_metadata(body.get("metadata", {})) + meta_update = sanitize_client_message_metadata(body.get("metadata", {})) or {} session = session_manager.get_session(session_id) # Update in-memory @@ -504,6 +511,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/merge-last-assistant") async def merge_last_assistant(request: Request, session_id: str): """Merge the last two assistant messages into one (for continue).""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -593,6 +601,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/fork") async def fork_session(request: Request, session_id: str): """Create a new session with messages copied up to keep_count.""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: body = await request.json() @@ -651,6 +660,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.get("/api/conversations/topics") async def get_conversation_topics(request: Request) -> Dict[str, Any]: + require_chat_scope(request) from src.auth_helpers import require_user user = require_user(request) try: @@ -666,6 +676,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: endpoint estimates the persisted session context so the header can show when the whole chat is approaching compaction. """ + require_chat_scope(request) _verify_session_owner(request, session_id) try: session = session_manager.get_session(session_id) @@ -710,6 +721,7 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter: @router.post("/api/session/{session_id}/compact") async def compact_session(request: Request, session_id: str): """Manually trigger context compaction for a session.""" + require_chat_scope(request) _verify_session_owner(request, session_id) from src.auth_helpers import effective_user owner = effective_user(request) diff --git a/routes/model_routes.py b/routes/model_routes.py index fcf9e1634..3123ab1fb 100644 --- a/routes/model_routes.py +++ b/routes/model_routes.py @@ -29,7 +29,7 @@ from src.endpoint_resolver import ( build_models_url, build_headers, ) -from src.auth_helpers import _auth_disabled, effective_user, owner_filter +from src.auth_helpers import _auth_disabled, owner_filter, require_chat_scope logger = logging.getLogger(__name__) @@ -1622,13 +1622,7 @@ def setup_model_routes(model_discovery): # Require auth; "" is the unconfigured single-user mode, treated as # "see everything" by _fetch_models. try: - if getattr(request.state, "api_token", False): - scopes = set(getattr(request.state, "api_token_scopes", []) or []) - if "chat" not in scopes: - raise HTTPException(403, "API token is not scoped for chat") - if not getattr(request.state, "api_token_owner", None): - raise HTTPException(403, "API token has no owner") - owner = effective_user(request) or "" + owner = require_chat_scope(request) or "" # Reject anonymous in configured deployments — no leaking the model # list to unauthenticated callers. diff --git a/routes/research/research_routes.py b/routes/research/research_routes.py index 905ee4b92..6d7ff58ff 100644 --- a/routes/research/research_routes.py +++ b/routes/research/research_routes.py @@ -9,12 +9,12 @@ from datetime import datetime from pathlib import Path from typing import Optional -from fastapi import APIRouter, HTTPException, Query, Request +from fastapi import APIRouter, Depends, HTTPException, Query, Request from fastapi.responses import HTMLResponse, StreamingResponse from pydantic import BaseModel, Field from core.middleware import INTERNAL_TOOL_USER from src.endpoint_resolver import resolve_endpoint -from src.auth_helpers import _auth_disabled, get_current_user +from src.auth_helpers import _auth_disabled, require_interactive_request from src.owner_identity import REQUEST_SENTINEL_OWNERS from src.constants import DEEP_RESEARCH_DIR @@ -207,14 +207,17 @@ def _resolve_endpoint_runtime(ep, owner=None, model: Optional[str] = None): def setup_research_routes(research_handler, session_manager=None) -> APIRouter: - router = APIRouter(tags=["research"]) + router = APIRouter( + tags=["research"], + dependencies=[Depends(require_interactive_request)], + ) def _require_user(request: Request) -> str: """All research endpoints require an authenticated user. Research data isn't owner-scoped in the on-disk JSON yet, so we at least block anonymous access. Multi-tenant deploys should additionally verify the session belongs to this user.""" - user = get_current_user(request) + user = require_interactive_request(request) if not user: if _auth_disabled(): return "" diff --git a/routes/search/search_routes.py b/routes/search/search_routes.py index 1effb7b8f..185825b6b 100644 --- a/routes/search/search_routes.py +++ b/routes/search/search_routes.py @@ -3,13 +3,14 @@ import logging from typing import Dict, Any -from fastapi import APIRouter, Request +from fastapi import APIRouter, Depends, Request import time from services.search import get_search_config, comprehensive_web_search, PROVIDER_INFO from services.search.core import _call_provider from services.search.providers import _get_provider_key, _get_search_instance +from src.auth_helpers import require_chat_scope logger = logging.getLogger(__name__) @@ -37,7 +38,10 @@ async def _request_values(request: Request) -> Dict[str, Any]: def setup_search_routes(config) -> APIRouter: - router = APIRouter(tags=["search"]) + router = APIRouter( + tags=["search"], + dependencies=[Depends(require_chat_scope)], + ) @router.get("/api/search/config") async def get_search_settings() -> Dict[str, Any]: @@ -49,6 +53,7 @@ def setup_search_routes(config) -> APIRouter: Used by Compare mode to pre-search once and share results across panes. """ + require_chat_scope(request) values = await _request_values(request) query = str(values.get("query") or values.get("q") or "").strip() if not query: @@ -87,6 +92,7 @@ def setup_search_routes(config) -> APIRouter: @router.post("/api/search/query") async def search_with_provider(request: Request) -> Dict[str, Any]: """Search using a specific provider. Used by compare search mode.""" + require_chat_scope(request) values = await _request_values(request) query = str(values.get("query") or values.get("q") or "").strip() provider = str(values.get("provider") or "").strip() diff --git a/routes/session_routes.py b/routes/session_routes.py index d16b8608f..f7447547c 100644 --- a/routes/session_routes.py +++ b/routes/session_routes.py @@ -225,6 +225,7 @@ def setup_session_routes( @router.get("/sessions") def list_sessions(request: Request): + require_chat_scope(request) user = effective_user(request) active_incognito_id = str(request.query_params.get("active_incognito_id") or "").strip() # Lazy purge: incognito sessions are ephemeral by design — wipe leftovers @@ -343,6 +344,7 @@ def setup_session_routes( api_key: str = Form(""), endpoint_id: str = Form(""), ): + require_chat_scope(request) skip_val = str(skip_validation).lower() == "true" user = effective_user(request) endpoint_api_key = "" @@ -470,6 +472,7 @@ def setup_session_routes( model: str = Form(None), endpoint_url: str = Form(None), endpoint_id: str = Form(None), ): + require_chat_scope(request) _verify_session_owner(request, sid) try: session = session_manager.get_session(sid) @@ -544,6 +547,7 @@ def setup_session_routes( @router.post("/session/{sid}/inject_messages") async def inject_messages(request: Request, sid: str): """Bulk-inject messages into a session's history (for group chat sync).""" + require_chat_scope(request) _verify_session_owner(request, sid) try: sess = session_manager.get_session(sid) @@ -580,11 +584,13 @@ def setup_session_routes( @router.post("/session/{sid}/delete") def delete_session_beacon(request: Request, sid: str): """Delete session via POST (for navigator.sendBeacon on page close).""" + require_chat_scope(request) return delete_session(request, sid) @router.post("/sessions/bulk-delete") async def bulk_delete_sessions(request: Request): """Delete multiple sessions (for compare cleanup via sendBeacon).""" + require_chat_scope(request) from core.database import ChatMessage as _CM try: body = await request.json() @@ -614,6 +620,7 @@ def setup_session_routes( @router.delete("/session/{sid}") def delete_session(request: Request, sid: str): """Permanently delete a session and all its messages.""" + require_chat_scope(request) _verify_session_owner(request, sid, session_manager) try: # Block deletion of starred/favorited sessions @@ -648,6 +655,7 @@ def setup_session_routes( @router.delete("/sessions/all") def delete_all_sessions(request: Request): """Admin only: permanently delete ALL sessions and their messages.""" + require_chat_scope(request) from core.middleware import require_admin require_admin(request) @@ -701,6 +709,7 @@ def setup_session_routes( @router.post("/session/{sid}/archive") def archive_session(request: Request, sid: str): """Archive a session, keeping its data but removing it from active sessions.""" + require_chat_scope(request) _verify_session_owner(request, sid) try: # First check if session exists @@ -739,6 +748,7 @@ def setup_session_routes( @router.post("/session/{sid}/unarchive") def unarchive_session(request: Request, sid: str): """Restore an archived session back to the active session list.""" + require_chat_scope(request) _verify_session_owner(request, sid) db = SessionLocal() try: @@ -769,6 +779,7 @@ def setup_session_routes( @router.get("/sessions/archived") def list_archived_sessions(request: Request, search: str = "", offset: int = 0, limit: int = 20, sort: str = "recent", model: str = ""): """List archived sessions for the archive browser.""" + require_chat_scope(request) user = effective_user(request) db = SessionLocal() try: @@ -816,6 +827,7 @@ def setup_session_routes( Supported formats: md (markdown), txt (plain text), json, html """ + require_chat_scope(request) _verify_session_owner(request, sid) try: session = session_manager.get_session(sid) @@ -902,6 +914,7 @@ def setup_session_routes( @router.post("/sessions/save") def sessions_save_now(request: Request): + require_chat_scope(request) user = effective_user(request) if not user: raise HTTPException(401, "Not authenticated") @@ -915,6 +928,7 @@ def setup_session_routes( model: str = Form("gpt-4o"), rag: str = Form(None) ): + require_chat_scope(request) if not OPENAI_API_KEY: raise HTTPException(400, "Server missing OPENAI_API_KEY") sid = str(uuid.uuid4()) @@ -936,6 +950,7 @@ def setup_session_routes( @router.post("/session/{session_id}/important") async def mark_session_important(request: Request, session_id: str, important: bool = Form(True)): """Mark a session as important to protect it from automatic cleanup.""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: # Validate session exists @@ -973,6 +988,7 @@ def setup_session_routes( @router.post("/session/{session_id}/compact") async def compact_session(request: Request, session_id: str): """Summarize older messages into one compacted history entry.""" + require_chat_scope(request) _verify_session_owner(request, session_id) try: session = session_manager.get_session(session_id) @@ -1059,6 +1075,7 @@ def setup_session_routes( after Phase 1 — used by the "Tidy (no AI)" UI affordance so users can clean junk without spending tokens. """ + require_chat_scope(request) from src.llm_core import llm_call user = effective_user(request) single_user_mode = not user and _auth_disabled() @@ -1339,6 +1356,7 @@ def setup_session_routes( @router.get("/session/{session_id}/context_info") async def get_context_info(request: Request, session_id: str): """Get the real context length for a session's model from the endpoint.""" + require_chat_scope(request) _verify_session_owner(request, session_id) session = session_manager.get_session(session_id) if not session: diff --git a/routes/skills_routes.py b/routes/skills_routes.py index 4b42835d9..2ec9e2fc0 100644 --- a/routes/skills_routes.py +++ b/routes/skills_routes.py @@ -13,11 +13,11 @@ from typing import List, Optional import httpx -from fastapi import APIRouter, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel, Field from services.memory.skills import SkillsManager -from src.auth_helpers import get_current_user +from src.auth_helpers import require_interactive_request from src.prompt_security import untrusted_context_message from core.middleware import require_admin @@ -1181,10 +1181,14 @@ async def run_scheduled_skill_audit(skills_manager: SkillsManager, def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter: - router = APIRouter(prefix="/api/skills", tags=["skills"]) + router = APIRouter( + prefix="/api/skills", + tags=["skills"], + dependencies=[Depends(require_interactive_request)], + ) def _owner(request: Request) -> Optional[str]: - return get_current_user(request) + return require_interactive_request(request) def _verify_owner(skill: dict, user: Optional[str]): if user is None: diff --git a/routes/task/task_routes.py b/routes/task/task_routes.py index d786c5730..32dd37df0 100644 --- a/routes/task/task_routes.py +++ b/routes/task/task_routes.py @@ -7,12 +7,12 @@ import uuid from datetime import datetime from typing import Optional, Dict, Any -from fastapi import APIRouter, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel from core.database import SessionLocal, ScheduledTask, TaskRun from core.constants import internal_api_base -from src.auth_helpers import get_current_user +from src.auth_helpers import get_current_user, require_interactive_request from src.constants import DATA_DIR, EMAIL_URGENCY_CACHE_DIR from src.task_action_policy import ( ADMIN_ONLY_TASK_ACTIONS, @@ -296,9 +296,17 @@ def _resolve_run_endpoint(db, task: ScheduledTask, run: TaskRun) -> str: def setup_task_routes(task_scheduler) -> APIRouter: - router = APIRouter(prefix="/api/tasks", tags=["tasks"]) + router = APIRouter( + prefix="/api/tasks", + tags=["tasks"], + dependencies=[Depends(require_interactive_request)], + ) def _owner(request: Request): + # Keep the route-local user lookup injectable for direct handler tests + # and legacy callers, but always run the centralized bearer-principal + # rejection first. + require_interactive_request(request) return get_current_user(request) async def _generate_task_name(prompt: str, owner: Optional[str] = None) -> str: diff --git a/routes/upload_routes.py b/routes/upload_routes.py index f0b401d36..8d6e1514d 100644 --- a/routes/upload_routes.py +++ b/routes/upload_routes.py @@ -265,6 +265,7 @@ def setup_upload_routes(upload_handler): session_id: Optional[str] = Form(None), ): """Upload files with enhanced security and organization.""" + require_chat_scope(request) if not isinstance(session_id, str): session_id = None if not files: @@ -324,6 +325,7 @@ def setup_upload_routes(upload_handler): @router.post("/cleanup") async def manual_cleanup(request: Request): """Manually trigger cleanup of old uploads.""" + require_chat_scope(request) require_admin(request) try: cleaned_count = await asyncio.to_thread( @@ -347,6 +349,7 @@ def setup_upload_routes(upload_handler): @router.get("/stats") async def upload_stats(request: Request): """Get statistics about uploaded files.""" + require_chat_scope(request) require_admin(request) try: return upload_handler.get_upload_stats() @@ -359,6 +362,7 @@ def setup_upload_routes(upload_handler): """Serve an uploaded file by its ID. `?thumb=1` returns a small cached JPEG thumbnail for images (used by chat attachment previews) so the client isn't downloading the full-resolution photo just to show it tiny.""" + require_chat_scope(request) if not upload_handler.validate_upload_id(file_id): raise HTTPException(400, "Invalid file ID") import mimetypes as _mt @@ -457,6 +461,7 @@ def setup_upload_routes(upload_handler): """Return the vision-model OCR/description for an uploaded image. Cached under UPLOAD_DIR/.vision/{file_id}.txt — first call computes, subsequent loads are instant. Pass force=1 to recompute.""" + require_chat_scope(request) if not upload_handler.validate_upload_id(file_id): raise HTTPException(400, "Invalid file ID") info = _load_upload_info(file_id) @@ -501,6 +506,7 @@ def setup_upload_routes(upload_handler): async def put_vision_text(request: Request, file_id: str): """Persist a user-edited vision/OCR text for an attachment. Stored in the same cache file so the chat send picks it up as the override.""" + require_chat_scope(request) if not upload_handler.validate_upload_id(file_id): raise HTTPException(400, "Invalid file ID") info = _load_upload_info(file_id) diff --git a/routes/webhook/webhook_routes.py b/routes/webhook/webhook_routes.py index 8d3a704c6..066e26189 100644 --- a/routes/webhook/webhook_routes.py +++ b/routes/webhook/webhook_routes.py @@ -9,7 +9,7 @@ from fastapi import APIRouter, HTTPException, Request, Form from pydantic import BaseModel, Field from core.database import SessionLocal, Webhook, ModelEndpoint -from src.auth_helpers import owner_filter +from src.auth_helpers import owner_filter, require_chat_scope from src.url_security import validate_public_http_url from src.webhook_manager import WebhookManager, validate_webhook_url, validate_events @@ -32,14 +32,15 @@ def _select_api_chat_fallback_endpoint(db, token_owner: Optional[str]): legacy null-owner ("shared") rows. Owner-scoped: an unscoped .first() would let a chat-scoped token fall back onto another user's private endpoint and silently spend that owner's API key/quota. Prefer owner rows before shared - rows. Fails closed to null-owner rows only when token_owner is absent. + rows. Fails closed when token_owner is absent; the sync endpoint requires + an owner-scoped bearer before this helper is reached. Does not validate base_url — admin-configured local/LAN endpoints remain allowed. """ query = db.query(ModelEndpoint).filter(ModelEndpoint.is_enabled == True) # noqa: E712 - if token_owner: - query = owner_filter(query, ModelEndpoint, token_owner) - return query.order_by(ModelEndpoint.owner.desc(), ModelEndpoint.created_at).first() - return query.filter(ModelEndpoint.owner == None).order_by(ModelEndpoint.created_at).first() # noqa: E711 + if not token_owner: + return None + query = owner_filter(query, ModelEndpoint, token_owner) + return query.order_by(ModelEndpoint.owner.desc(), ModelEndpoint.created_at).first() def _caller_owns_session(sess_owner, caller) -> bool: @@ -236,12 +237,11 @@ def setup_webhook_routes( @router.post("/v1/chat") async def sync_chat(request: Request, body: SyncChatRequest): - if not getattr(request.state, "api_token", False): + if getattr(request.state, "api_token", False) is not True: raise HTTPException(403, "This endpoint requires an API token") - scopes = set(getattr(request.state, "api_token_scopes", []) or []) - if "chat" not in scopes: - raise HTTPException(403, "API token is not scoped for chat") - token_owner = getattr(request.state, "api_token_owner", None) + token_owner = require_chat_scope(request) + if not token_owner: + raise HTTPException(403, "API token has no owner") from core.models import ChatMessage from src.llm_core import llm_call_async diff --git a/src/auth_helpers.py b/src/auth_helpers.py index 35fd3a3cd..edf76ca43 100644 --- a/src/auth_helpers.py +++ b/src/auth_helpers.py @@ -4,12 +4,17 @@ import os from typing import Optional from fastapi import Request, HTTPException -from src.owner_identity import auth_disabled, effective_storage_owner +from src.owner_identity import ( + auth_disabled, + effective_storage_owner, + is_request_sentinel_owner, +) def get_current_user(request: Request) -> Optional[str]: """Get current username from request state (set by auth middleware).""" - return getattr(request.state, 'current_user', None) + state = getattr(request, "state", None) + return getattr(state, "current_user", None) def effective_user(request: Request) -> Optional[str]: @@ -29,29 +34,58 @@ def effective_user(request: Request) -> Optional[str]: owner falls back to :func:`get_current_user` (the "api" pseudo-user), so it never escalates. """ - if getattr(request.state, "api_token", False): - owner = getattr(request.state, "api_token_owner", None) - if owner: - return owner + if _is_api_token_request(request): + state = getattr(request, "state", None) + owner = getattr(state, "api_token_owner", None) + if isinstance(owner, str) and owner.strip(): + return owner.strip() return get_current_user(request) def _is_api_token_request(request: Request) -> bool: """Return True when middleware authenticated a bearer API token.""" - return bool(getattr(request.state, "api_token", False)) + state = getattr(request, "state", None) + return getattr(state, "api_token", False) is True + + +def require_api_token_owner(request: Request) -> str: + """Return a real owner for a bearer request, failing closed otherwise. + + The middleware normally resolves token owners against configured human + accounts. Keep that invariant at route boundaries too: direct endpoint + tests, alternate ASGI entry points, and future middleware changes must not + turn a request sentinel or an ownerless token into a durable/executable + owner. + """ + state = getattr(request, "state", None) + owner = getattr(state, "api_token_owner", None) + if ( + not isinstance(owner, str) + or not owner.strip() + or is_request_sentinel_owner(owner) + ): + raise HTTPException(403, "API token has no owner") + return owner.strip() def require_api_token_scope(request: Request, required_scope: str) -> Optional[str]: """Require one declared scope for bearer callers; leave browser callers unchanged.""" if not _is_api_token_request(request): return effective_user(request) - scopes = set(getattr(request.state, "api_token_scopes", []) or []) - if required_scope not in scopes: + state = getattr(request, "state", None) + raw_scopes = getattr(state, "api_token_scopes", None) + if isinstance(raw_scopes, (list, tuple, set, frozenset)): + scopes = { + value.strip().casefold() + for value in raw_scopes + if isinstance(value, str) and value.strip() + } + else: + scopes = set() + normalized_scope = str(required_scope or "").strip().casefold() + if not normalized_scope or normalized_scope not in scopes: raise HTTPException(403, f"API token missing required scope: {required_scope}") - owner = getattr(request.state, "api_token_owner", None) - if not owner: - raise HTTPException(403, "API token has no owner") - return owner + return require_api_token_owner(request) def require_chat_scope(request: Request) -> Optional[str]: @@ -59,6 +93,22 @@ def require_chat_scope(request: Request) -> Optional[str]: return require_api_token_scope(request, "chat") +def require_interactive_request(request: Request) -> Optional[str]: + """Reject bearer integrations from browser-only agent/control surfaces. + + This is deliberately a bearer-principal gate rather than an authentication + requirement. Cookie sessions and AUTH_ENABLED=false keep their existing + route behavior, while API tokens cannot enter routes that start, resume, + approve, or otherwise control interactive agent work. + """ + current_user = get_current_user(request) + if _is_api_token_request(request) or ( + isinstance(current_user, str) and current_user.strip().casefold() == "api" + ): + raise HTTPException(403, "API tokens cannot use this interactive surface") + return current_user + + def enforce_api_token_chat_controls( request: Request, *, @@ -88,7 +138,7 @@ def require_authenticated_request(request: Request) -> str: sessions or their own API-token scope/owner gate. """ if _is_api_token_request(request): - return effective_user(request) or "" + return require_api_token_owner(request) return require_user(request) diff --git a/src/chat_processor.py b/src/chat_processor.py index 1f89bc36f..5c518bde2 100644 --- a/src/chat_processor.py +++ b/src/chat_processor.py @@ -274,6 +274,7 @@ class ChatProcessor: agent_mode: bool = False, incognito: bool = False, use_skills: bool = True, + allow_tool_preprocessing: bool = True, ) -> Tuple[List[Dict[str, str]], List[Dict[str, Any]], List[Dict[str, str]]]: """Build the context preface for LLM calls. @@ -457,7 +458,7 @@ class ChatProcessor: # hundreds of KB of duplicate page HTML and confuses the model) or for # link-heavy pastes (>3 URLs typically means it's a boilerplate-laden # blog post, not a "summarize this URL" request). - urls = extract_urls(message) + urls = extract_urls(message) if allow_tool_preprocessing else [] non_yt_urls = [u for u in urls if not is_youtube_url(u)] skip_url_fetch = len(message) > 2000 or len(non_yt_urls) > 3 if not skip_url_fetch: diff --git a/src/message_metadata.py b/src/message_metadata.py index bc8fe6cfc..00940d296 100644 --- a/src/message_metadata.py +++ b/src/message_metadata.py @@ -1,6 +1,6 @@ """Trust-boundary helpers for client-supplied chat metadata.""" -from typing import Any +from typing import Any, Optional from src.tool_approval_scopes import CHAT_SESSION_APPROVAL_CONTEXT_MARKER @@ -11,12 +11,22 @@ _SERVER_OWNED_MESSAGE_METADATA = frozenset({ }) -def sanitize_client_message_metadata(metadata: Any) -> Any: - """Drop fields that can only be produced by server-side tool execution.""" +def sanitize_client_message_metadata(metadata: Any) -> Optional[dict]: + """Normalize client metadata and drop server-owned fields. + + Client metadata is only a JSON object. In particular, do not let a + list-of-pairs value reach ``dict.update``: that mapping-compatible shape + can smuggle protected approval fields through an otherwise safe merge. + Malformed metadata is normalized away; server-generated metadata remains + untouched because this helper is called only at client ingress points. + """ + if metadata is None: + return None if not isinstance(metadata, dict): - return metadata - return { + return None + sanitized = { key: value for key, value in metadata.items() if key not in _SERVER_OWNED_MESSAGE_METADATA } + return sanitized or None diff --git a/tests/test_api_chat_security.py b/tests/test_api_chat_security.py index d92a31620..817d6ca4c 100644 --- a/tests/test_api_chat_security.py +++ b/tests/test_api_chat_security.py @@ -327,6 +327,53 @@ async def test_api_chat_direct_base_url_allows_mocked_public_endpoint(monkeypatc assert session_manager.created[0]["endpoint_url"] == "https://api.example.com/v1/chat/completions" +@pytest.mark.asyncio +async def test_api_chat_ownerless_token_cannot_use_direct_api_key(monkeypatch): + webhook_routes = _load_webhook_routes_for_test(monkeypatch) + _install_sync_chat_stubs(monkeypatch) + session_manager = _SessionManager() + sync_chat = _sync_chat_endpoint(webhook_routes, session_manager) + body = types.SimpleNamespace( + message="hello", + api_key="test-key", + base_url="https://api.example.com/v1", + model="test-model", + provider=None, + session=None, + ) + + with pytest.raises(webhook_routes.HTTPException) as exc: + await sync_chat(_Request(owner=None), body) + + assert exc.value.status_code == 403 + assert session_manager.created == [] + + +@pytest.mark.asyncio +async def test_api_chat_ownerless_token_cannot_use_configured_fallback(monkeypatch): + webhook_routes = _load_webhook_routes_for_test(monkeypatch) + _install_sync_chat_stubs(monkeypatch) + db = _DB([_Endpoint(owner=None, base_url="http://localhost:11434/v1", api_key="shared-key")]) + monkeypatch.setattr(webhook_routes, "ModelEndpoint", _ModelEndpoint) + monkeypatch.setattr(webhook_routes, "SessionLocal", lambda: db) + session_manager = _SessionManager() + sync_chat = _sync_chat_endpoint(webhook_routes, session_manager) + body = types.SimpleNamespace( + message="hello", + api_key=None, + base_url=None, + model="local-model", + provider=None, + session=None, + ) + + with pytest.raises(webhook_routes.HTTPException) as exc: + await sync_chat(_Request(owner=None), body) + + assert exc.value.status_code == 403 + assert session_manager.created == [] + + def test_api_chat_fallback_endpoint_selection_for_owned_token(monkeypatch): webhook_routes = _load_webhook_routes_for_test(monkeypatch) rows = [ @@ -345,7 +392,7 @@ def test_api_chat_fallback_endpoint_selection_for_owned_token(monkeypatch): assert selected.created_at == 2 -def test_api_chat_fallback_without_owner_uses_shared_only(monkeypatch): +def test_api_chat_fallback_without_owner_is_not_selectable(monkeypatch): webhook_routes = _load_webhook_routes_for_test(monkeypatch) rows = [ _Endpoint(owner="alice", created_at=0), @@ -357,9 +404,7 @@ def test_api_chat_fallback_without_owner_uses_shared_only(monkeypatch): selected = webhook_routes._select_api_chat_fallback_endpoint(_DB(rows), None) - assert selected.owner is None - assert selected.is_enabled is True - assert selected.created_at == 2 + assert selected is None @pytest.mark.asyncio @@ -367,7 +412,7 @@ async def test_api_chat_fallback_trusts_configured_local_endpoint(monkeypatch): webhook_routes = _load_webhook_routes_for_test(monkeypatch) _install_sync_chat_stubs(monkeypatch) local_endpoint = _Endpoint( - owner=None, + owner="alice", base_url="http://localhost:11434/v1", api_key="configured-key", ) @@ -396,7 +441,7 @@ async def test_api_chat_fallback_trusts_configured_local_endpoint(monkeypatch): session=None, ) - response = await sync_chat(_Request(owner=None), body) + response = await sync_chat(_Request(owner="alice"), body) assert response["response"] == "mocked response" assert response["model"] == "local-model" diff --git a/tests/test_api_token_forward_fixes.py b/tests/test_api_token_forward_fixes.py new file mode 100644 index 000000000..6dc558c78 --- /dev/null +++ b/tests/test_api_token_forward_fixes.py @@ -0,0 +1,554 @@ +"""Regression coverage for the API-token forward-fix boundary. + +These tests deliberately exercise both sides of FastAPI's router dependency +boundary: real ASGI requests run router dependencies, while direct endpoint +calls must still hit the same security helper before doing work. +""" + +import json +from types import SimpleNamespace +from unittest.mock import MagicMock + +import httpx +import pytest +from fastapi import FastAPI, HTTPException + +from core.models import ChatMessage, Session +from src.auth_helpers import ( + effective_user, + require_api_token_owner, + require_chat_scope, + require_interactive_request, +) +from src.message_metadata import sanitize_client_message_metadata +from src.request_models import ChatRequest +from src.tool_approval_scopes import CHAT_SESSION_APPROVAL_CONTEXT_MARKER + + +def _request(*, api_token=True, owner="alice", scopes=("chat",), current_user="api"): + return SimpleNamespace( + state=SimpleNamespace( + api_token=api_token, + api_token_owner=owner, + api_token_scopes=list(scopes), + current_user=current_user, + ), + app=SimpleNamespace(state=SimpleNamespace(auth_manager=None)), + headers={}, + ) + + +def test_token_scope_gate_requires_a_real_owner_and_normalizes_scope_input(): + assert require_chat_scope( + _request(owner=" alice ", scopes=(" CHAT ",)) + ) == "alice" + assert effective_user(_request(owner=" alice ")) == "alice" + + for owner in (None, "", " "): + with pytest.raises(HTTPException) as exc: + require_chat_scope(_request(owner=owner)) + assert exc.value.status_code == 403 + + with pytest.raises(HTTPException) as exc: + require_chat_scope(_request(scopes=(["invalid"],))) + assert exc.value.status_code == 403 + + +@pytest.mark.parametrize("owner", ["api", "internal-tool", "SYSTEM", " ", None]) +def test_bearer_owner_gate_rejects_sentinels_and_ownerless_values(owner): + with pytest.raises(HTTPException) as exc: + require_api_token_owner(_request(owner=owner)) + assert exc.value.status_code == 403 + + +def test_interactive_gate_rejects_api_sentinel_even_without_bearer_flag(): + with pytest.raises(HTTPException) as exc: + require_interactive_request( + _request(api_token=False, owner=None, scopes=(), current_user="api") + ) + assert exc.value.status_code == 403 + + +def test_interactive_gate_rejects_bearer_but_preserves_cookie_and_anonymous_modes(): + with pytest.raises(HTTPException) as exc: + require_interactive_request(_request()) + assert exc.value.status_code == 403 + + assert require_interactive_request( + _request(api_token=False, owner="alice", scopes=(), current_user="alice") + ) == "alice" + assert require_interactive_request( + _request(api_token=False, owner=None, scopes=(), current_user=None) + ) is None + + +class _PrincipalState: + """Inject the same request.state fields as auth middleware, without auth.""" + + def __init__(self, app): + self.app = app + + async def __call__(self, scope, receive, send): + if scope["type"] == "http": + headers = dict(scope.get("headers") or []) + if headers.get(b"x-api-token") == b"1": + raw_scopes = headers.get(b"x-api-scopes", b"").decode() + scopes = [item for item in raw_scopes.split(",") if item] + scope["state"] = { + "api_token": True, + "api_token_owner": headers.get(b"x-api-owner", b"").decode() or None, + "api_token_scopes": scopes, + "current_user": "api", + } + else: + scope["state"] = { + "api_token": False, + "current_user": headers.get(b"x-user", b"").decode() or None, + } + await self.app(scope, receive, send) + + +def _client(app): + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://forward-fix.test", + ) + + +@pytest.mark.asyncio +async def test_real_chat_and_search_routes_run_scope_dependencies(monkeypatch): + from routes.chat_routes import setup_chat_routes + from routes.search.search_routes import setup_search_routes + + app = FastAPI() + app.include_router(setup_chat_routes(None, None, None, None, None, None)) + app.include_router(setup_search_routes(None)) + + token_headers = { + "x-api-token": "1", + "x-api-owner": "alice", + "x-api-scopes": "email:read", + } + chat_headers = {**token_headers, "x-api-scopes": "chat"} + + async with _client(_PrincipalState(app)) as client: + # Both standalone direct-search POST routes are capability-gated. + for path, body in ( + ("/api/search", {"query": "private"}), + ("/api/search/query", {"query": "private", "provider": "brave"}), + ): + response = await client.post(path, json=body, headers=token_headers) + assert response.status_code == 403, (path, response.text) + + # The chat router's GET alias is gated too, and the detached-run + # controls add the stricter interactive-principal check after chat + # capability authorization succeeds. + assert (await client.get("/api/search?q=private", headers=token_headers)).status_code == 403 + assert (await client.get("/api/chat/stream_status/sid", headers=chat_headers)).status_code == 403 + + # Cookie and auth-disabled-shaped requests retain the old no-query + # behavior rather than being rejected by bearer policy. + assert (await client.get("/api/search?q=", headers={"x-user": "alice"})).status_code == 200 + assert (await client.get("/api/search?q=", headers={})).status_code == 200 + + +@pytest.mark.asyncio +async def test_real_agent_capable_routers_reject_bearer_principals(): + from routes.assistant_routes import setup_assistant_routes + from routes.research.research_routes import setup_research_routes + from routes.skills_routes import setup_skills_routes + from routes.task.task_routes import setup_task_routes + + app = FastAPI() + app.include_router(setup_task_routes(MagicMock())) + app.include_router(setup_skills_routes(MagicMock())) + app.include_router(setup_assistant_routes(MagicMock())) + app.include_router(setup_research_routes(SimpleNamespace(_active_tasks={}))) + + headers = { + "x-api-token": "1", + "x-api-owner": "alice", + "x-api-scopes": "chat", + } + requests = ( + ("/api/tasks/meta/events", "get", None), + ("/api/skills/index", "get", None), + ("/api/assistant/available-timezones", "get", None), + ("/api/research/active", "get", None), + ) + async with _client(_PrincipalState(app)) as client: + for path, method, body in requests: + response = await client.request(method.upper(), path, headers=headers) + assert response.status_code == 403, (path, response.text) + + +@pytest.mark.asyncio +async def test_direct_auxiliary_handlers_keep_their_bearer_gates(): + from routes.chat_routes import setup_chat_routes + from routes.search.search_routes import setup_search_routes + + chat_router = setup_chat_routes(None, None, None, None, None, None) + search_router = setup_search_routes(None) + request = _request(scopes=("email:read",)) + + chat_routes = {route.path: route.endpoint for route in chat_router.routes} + search_routes = {route.path: route.endpoint for route in search_router.routes} + + with pytest.raises(HTTPException): + await chat_routes["/api/search"](request, q="private", limit=20) + with pytest.raises(HTTPException): + await chat_routes["/api/inject_context/{session_id}"](request, "sid", "context") + with pytest.raises(HTTPException): + await chat_routes["/api/rewrite"](request) + with pytest.raises(HTTPException): + await search_routes["/api/search"](request) + with pytest.raises(HTTPException): + await search_routes["/api/search/query"](request) + + with pytest.raises(HTTPException): + await chat_routes["/api/chat/resume/{session_id}"](request, "sid") + + +@pytest.mark.asyncio +async def test_direct_task_skill_assistant_and_research_handlers_reject_bearer(): + from routes.assistant_routes import setup_assistant_routes + from routes.research.research_routes import setup_research_routes + from routes.skills_routes import setup_skills_routes + from routes.task.task_routes import setup_task_routes + + request = _request() + + task_router = setup_task_routes(MagicMock()) + task_create = next(route.endpoint for route in task_router.routes if route.path == "/api/tasks" and "POST" in route.methods) + with pytest.raises(HTTPException): + await task_create(request, req=SimpleNamespace()) + + skills_router = setup_skills_routes(MagicMock()) + skill_test = next(route.endpoint for route in skills_router.routes if route.path == "/api/skills/{skill_id}/test") + with pytest.raises(HTTPException): + await skill_test(request, "demo") + + assistant_router = setup_assistant_routes(MagicMock()) + assistant_session = next(route.endpoint for route in assistant_router.routes if route.path == "/api/assistant/session") + with pytest.raises(HTTPException): + await assistant_session(request) + + research_router = setup_research_routes(SimpleNamespace(_active_tasks={})) + research_active = next(route.endpoint for route in research_router.routes if route.path == "/api/research/active") + with pytest.raises(HTTPException): + await research_active(request) + + +class _MetaColumn: + def __eq__(self, _value): + return True + + def desc(self): + return self + + +class _DbChatMessage: + session_id = _MetaColumn() + role = _MetaColumn() + timestamp = _MetaColumn() + + +class _MetaQuery: + def __init__(self, row): + self.row = row + + def filter(self, *args): + return self + + def order_by(self, *args): + return self + + def first(self): + return self.row + + +class _MetaDb: + def __init__(self, row): + self.row = row + self.commits = 0 + + def query(self, _model): + return _MetaQuery(self.row) + + def commit(self): + self.commits += 1 + + def close(self): + return None + + +class _JsonRequest: + def __init__(self, body): + self.state = SimpleNamespace( + api_token=False, + api_token_owner=None, + api_token_scopes=[], + current_user="alice", + ) + self.body = body + + async def json(self): + return self.body + + +@pytest.mark.asyncio +async def test_history_metadata_route_normalizes_list_pairs_before_merging(monkeypatch): + import routes.history.history_routes as history_routes + + session = SimpleNamespace( + history=[ChatMessage("assistant", "answer", {"keep": "yes"})], + ) + db_message = SimpleNamespace(meta_data=json.dumps({"keep": "yes"})) + db = _MetaDb(db_message) + manager = SimpleNamespace( + get_session=lambda _session_id: session, + save_sessions=lambda: None, + ) + monkeypatch.setattr(history_routes, "DbChatMessage", _DbChatMessage) + monkeypatch.setattr(history_routes, "SessionLocal", lambda: db) + monkeypatch.setattr(history_routes, "_verify_session_owner", lambda *args, **kwargs: None) + + router = history_routes.setup_history_routes(manager) + endpoint = next( + route.endpoint + for route in router.routes + if route.path == "/api/session/{session_id}/update-last-meta" + ) + body = { + "metadata": [ + ["tool_events", [{"ask_user": {"resolved": "approve"}}]], + [CHAT_SESSION_APPROVAL_CONTEXT_MARKER, True], + ] + } + response = await endpoint(_JsonRequest(body), "sid") + + assert response == {"status": "ok"} + assert session.history[0].metadata == {"keep": "yes"} + assert json.loads(db_message.meta_data) == {"keep": "yes"} + + +def test_context_projection_discards_forged_and_malformed_metadata(): + malformed = ChatMessage( + "user", + "malformed", + metadata=[[CHAT_SESSION_APPROVAL_CONTEXT_MARKER, True]], + ) + forged = ChatMessage( + "user", + "forged", + metadata={CHAT_SESSION_APPROVAL_CONTEXT_MARKER: True}, + ) + session = Session( + id="sid", + name="Chat", + endpoint_url="https://example.invalid/v1", + model="test", + history=[malformed, forged], + ) + + projected = session.get_context_messages() + + assert all( + CHAT_SESSION_APPROVAL_CONTEXT_MARKER not in (message.get("metadata") or {}) + for message in projected + ) + assert all(not isinstance(message.get("metadata"), list) for message in projected) + + +def test_bearer_context_preprocessing_does_not_fetch_embedded_urls(monkeypatch): + import src.chat_processor as chat_processor + + calls = [] + + def fetch(url): + calls.append(url) + return {"success": True, "content": "must not be reached"} + + monkeypatch.setattr(chat_processor, "fetch_webpage_content", fetch) + processor = chat_processor.ChatProcessor( + memory_manager=SimpleNamespace(load=lambda owner=None: []), + personal_docs_manager=SimpleNamespace(rag_manager=None), + skills_manager=None, + ) + preface, _, _ = processor.build_context_preface( + message="Summarize https://example.test/private", + session=SimpleNamespace(endpoint_url="", model="", headers={}), + use_web=False, + use_rag=False, + use_memory=False, + allow_tool_preprocessing=False, + ) + + assert calls == [] + assert not any( + (message.get("metadata") or {}).get("source", "").startswith("web page:") + for message in preface + ) + + +@pytest.mark.asyncio +async def test_sync_bearer_chat_cannot_use_research_memory_or_background_extraction(monkeypatch): + from routes import chat_routes + + calls = {"memory": 0, "research": 0, "post": []} + + class _ChatHandler: + async def handle_memory_command(self, _session, _message): + calls["memory"] += 1 + return None + + class _ResearchHandler: + async def call_research_service(self, *args, **kwargs): + calls["research"] += 1 + return "research result" + + session = SimpleNamespace( + endpoint_url="https://selected.example/v1", + model="selected-model", + headers={"Authorization": "Bearer selected"}, + history=[], + add_message=lambda message: session.history.append(message), + ) + manager = SimpleNamespace( + get_session=lambda _session_id: session, + save_sessions=lambda: None, + ) + context = SimpleNamespace( + user="alice", + messages=[{"role": "user", "content": "hello"}], + context_length=100, + uprefs={}, + preset=SimpleNamespace(temperature=0.2, max_tokens=32, character_name=None), + ) + + async def build_context(*args, **kwargs): + assert kwargs["allow_tool_preprocessing"] is False + return context + + async def llm_call(*args, **kwargs): + return "answer", args[0][0], "selected-model" + + monkeypatch.setattr(chat_routes, "_verify_session_owner", lambda *args, **kwargs: None) + monkeypatch.setattr(chat_routes, "_clear_orphaned_session_endpoint", lambda *args, **kwargs: False) + monkeypatch.setattr(chat_routes, "_recover_empty_session_model", lambda *args, **kwargs: False) + monkeypatch.setattr(chat_routes, "_enforce_chat_privileges", lambda *args, **kwargs: None) + monkeypatch.setattr(chat_routes, "resolve_session_auth", lambda *args, **kwargs: None) + monkeypatch.setattr(chat_routes, "build_chat_context", build_context) + monkeypatch.setattr(chat_routes, "resolve_foreground_model_policy", lambda *args, **kwargs: SimpleNamespace(enabled=False, eligible_statuses=set())) + monkeypatch.setattr(chat_routes, "build_foreground_model_candidates", lambda *args, **kwargs: [("https://selected.example/v1", "selected-model", {})]) + monkeypatch.setattr(chat_routes, "build_foreground_route_descriptors", lambda *args, **kwargs: [{"endpoint_id": None, "endpoint_label": "Selected route"}]) + monkeypatch.setattr(chat_routes, "llm_call_async_with_route_fallback", llm_call) + monkeypatch.setattr(chat_routes, "apply_compaction_state", lambda *args, **kwargs: None) + monkeypatch.setattr(chat_routes, "clean_thinking_for_save", lambda reply, metadata: (reply, metadata)) + monkeypatch.setattr(chat_routes, "run_post_response_tasks", lambda *args, **kwargs: calls["post"].append(kwargs)) + + import core.database as database + + monkeypatch.setattr(database, "update_session_last_accessed", lambda _session_id: None) + router = chat_routes.setup_chat_routes( + manager, + _ChatHandler(), + SimpleNamespace(), + SimpleNamespace(), + _ResearchHandler(), + SimpleNamespace(), + ) + endpoint = next(route.endpoint for route in router.routes if route.path == "/api/chat") + with pytest.raises(HTTPException) as exc: + await endpoint( + _request(), + ChatRequest( + message="research this", + session="sid", + use_research=True, + ), + ) + assert exc.value.status_code == 403 + + result = await endpoint( + _request(), + ChatRequest( + message="remember this and research it", + session="sid", + use_research=False, + ), + ) + + assert result["response"] == "answer" + assert calls["memory"] == 0 + assert calls["research"] == 0 + assert calls["post"] and calls["post"][0]["allow_background_extraction"] is False + + +@pytest.mark.asyncio +async def test_stream_bearer_chat_disables_deferred_memory_extraction(monkeypatch): + from routes import chat_routes + from tests.test_foreground_model_routing import _chat_stream_endpoint + + captured = {} + endpoint = _chat_stream_endpoint( + monkeypatch, + "chat", + captured, + capture_completion=True, + ) + request = SimpleNamespace( + headers={}, + app=SimpleNamespace(state=SimpleNamespace(auth_manager=None)), + state=SimpleNamespace( + api_token=True, + api_token_owner="alice", + api_token_scopes=["chat"], + current_user="api", + ), + _form={"message": "hello", "session": "session-1", "mode": "chat"}, + ) + + async def form(): + return request._form + + request.form = form + + response = await endpoint(request) + async for _chunk in response.body_iterator: + pass + + assert captured["post_processed"] + assert captured["post_processed"][0][1]["allow_background_extraction"] is False + + +@pytest.mark.asyncio +async def test_stream_bearer_chat_cannot_dispatch_image_generation(monkeypatch): + from routes import chat_routes + from tests.test_foreground_model_routing import _chat_stream_endpoint + + captured = {} + endpoint = _chat_stream_endpoint(monkeypatch, "chat", captured) + monkeypatch.setattr(chat_routes, "_is_image_generation_session", lambda *args, **kwargs: True) + request = SimpleNamespace( + headers={}, + app=SimpleNamespace(state=SimpleNamespace(auth_manager=None)), + state=SimpleNamespace( + api_token=True, + api_token_owner="alice", + api_token_scopes=["chat"], + current_user="api", + ), + _form={"message": "generate an image", "session": "session-1", "mode": "chat"}, + ) + + async def form(): + return request._form + + request.form = form + + with pytest.raises(HTTPException) as exc: + await endpoint(request) + assert exc.value.status_code == 403 + assert "image" in str(exc.value.detail).lower() + assert "chat" not in captured diff --git a/tests/test_null_owner_gates.py b/tests/test_null_owner_gates.py index 97e66d007..4fcc0a41e 100644 --- a/tests/test_null_owner_gates.py +++ b/tests/test_null_owner_gates.py @@ -318,12 +318,13 @@ def test_sync_chat_fallback_skips_disabled_owned_endpoint(): assert ep is not None and ep.name == "shared" -def test_sync_chat_fallback_null_owner_uses_shared_rows_only(): - # When no token owner is known, only null-owner (shared) endpoints are - # visible — private endpoints of any user must not be returned. +def test_sync_chat_fallback_rejects_missing_token_owner(): + # The sync-chat route requires a resolved token owner before endpoint + # selection, so even a legacy/shared endpoint is not executable for an + # ownerless bearer. rows = [_ep("bob-private", "bob"), _ep("shared", None)] ep = _select(rows, None) - assert ep is not None and ep.name == "shared" + assert ep is None def test_sync_chat_fallback_null_owner_returns_none_with_no_shared():