From 0784cd2fed9aef7d78317033c56c8735045c5788 Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:53:06 +0100 Subject: [PATCH] fix(review): harden scholarly redirects, editor draft limits, and threat model - Enforce outbound URL policy on all hops via bounded manual redirects in scholarly lookups - Guard declared Content-Length in EditorDraftRoute before request body parsing - Keep scholarly lookup timeouts and budgets as internal constants rather than surface env vars - Narrow TUI threat-model description around demonstrable host shell bridge behavior - Add behavioral regression tests for redirect security and pre-parsing body size guards --- THREAT_MODEL.md | 2 +- routes/editor_draft_routes.py | 33 +++-- services/search/core.py | 74 +++++++---- src/constants.py | 6 +- tests/test_review_20260923_fixes.py | 197 +++++++++++++++++++++++++++- 5 files changed, 266 insertions(+), 46 deletions(-) diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 1dc4d4c20..d63d1335b 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -71,7 +71,7 @@ Operators who run the agent against untrusted web or email content with side-eff Two exemptions apply even when the gate is on, both deliberate: - Sources in `_CONTROL_PLANE_CONTEXT_SOURCES` (skills, runtime descriptors, the open editor document, the open email, uploaded files) are treated as control-plane metadata and still permit read-only tools. -- A TUI run that advertises an authenticated host bridge and declares `unattended_mode` exempts the local execution set in `TUI_CLIENT_TOOL_NAMES`. Personal, network and deployment-local tools are never exempted. +- A TUI run that advertises a host shell bridge and declares `unattended_mode` exempts the local execution set in `TUI_CLIENT_TOOL_NAMES`. Personal, network and deployment-local tools are never exempted. ## Security Headers diff --git a/routes/editor_draft_routes.py b/routes/editor_draft_routes.py index 2f107f236..ece6a8d8a 100644 --- a/routes/editor_draft_routes.py +++ b/routes/editor_draft_routes.py @@ -19,9 +19,10 @@ Each draft carries: import json import logging import uuid -from typing import Any, Dict, List, Optional +from typing import Any, Callable, Dict, List, Optional -from fastapi import APIRouter, Depends, HTTPException, Request +from fastapi import APIRouter, HTTPException, Request, Response +from fastapi.routing import APIRoute from pydantic import BaseModel from core.database import EditorDraft, SessionLocal @@ -84,13 +85,13 @@ def _draft_too_large() -> HTTPException: def reject_oversized_draft_body(request: Request) -> None: - """Refuse an oversized draft on ``Content-Length``, before the body is read. + """Refuse an oversized draft on declared ``Content-Length``, before the body is read or parsed. ``_dump_payload`` still owns the authoritative byte count, but it only runs - after the request has been parsed and re-serialised — by then the payload - has already been materialised several times over. Declaring a body past the - ceiling is enough to reject it, so do that first and cheaply. A request that - lies about or omits the header still hits the exact check further down. + after the request has been parsed and re-serialised. Declaring a body past the + ceiling is enough to reject it early and cheaply. Requests with absent, + malformed, or chunked transfer encoding still hit the authoritative byte count + check further down. """ raw_length = request.headers.get("content-length") if not raw_length: @@ -103,6 +104,20 @@ def reject_oversized_draft_body(request: Request) -> None: raise _draft_too_large() +class EditorDraftRoute(APIRoute): + """Route class that validates declared Content-Length before request body parsing.""" + + def get_route_handler(self) -> Callable: + original_route_handler = super().get_route_handler() + + async def custom_route_handler(request: Request) -> Response: + if request.method in ("POST", "PUT", "PATCH"): + reject_oversized_draft_body(request) + return await original_route_handler(request) + + return custom_route_handler + + def _dump_payload(payload: Dict[str, Any]) -> str: raw = json.dumps(payload or {}, separators=(",", ":")) if len(raw.encode("utf-8")) > EDITOR_DRAFT_MAX_BYTES: @@ -111,7 +126,7 @@ def _dump_payload(payload: Dict[str, Any]) -> str: def setup_editor_draft_routes() -> APIRouter: - router = APIRouter(tags=["editor-drafts"]) + router = APIRouter(tags=["editor-drafts"], route_class=EditorDraftRoute) @router.get("/api/editor-drafts") async def list_drafts(request: Request) -> Dict[str, List[Dict[str, Any]]]: @@ -147,7 +162,6 @@ def setup_editor_draft_routes() -> APIRouter: async def create_draft( request: Request, body: DraftCreate, - _size_guard: None = Depends(reject_oversized_draft_body), ) -> Dict[str, Any]: user = get_current_user(request) db = SessionLocal() @@ -180,7 +194,6 @@ def setup_editor_draft_routes() -> APIRouter: request: Request, draft_id: str, body: DraftUpdate, - _size_guard: None = Depends(reject_oversized_draft_body), ) -> Dict[str, Any]: user = get_current_user(request) db = SessionLocal() diff --git a/services/search/core.py b/services/search/core.py index d6fce2846..bd7d934f0 100644 --- a/services/search/core.py +++ b/services/search/core.py @@ -521,40 +521,66 @@ def _scholarly_budget(): _scholarly_deadline.reset(token) +MAX_SCHOLARLY_REDIRECTS = 3 + + def _scholarly_api_get(url: str, params: dict) -> Optional[httpx.Response]: """GET a scholarly metadata API under the shared outbound policy. - Returns ``None`` when the URL fails the outbound check or the caller's - budget is already spent, so callers degrade to their next source instead - of raising. ``follow_redirects`` stays on because both APIs redirect to - canonical paths, which is exactly why the destination needs checking. + Returns ``None`` when any destination URL fails the outbound check or the + caller's budget is already spent, so callers degrade to their next source + instead of raising. Bounded manual redirects ensure every hop passes + through ``check_outbound_url`` before the destination is contacted. """ from src.constants import SCHOLARLY_LOOKUP_TIMEOUT from src.url_safety import check_outbound_url - ok, reason = check_outbound_url(url, block_private=True) - if not ok: - logger.warning("Scholarly lookup blocked for %s: %s", url, reason) - return None + current_url = url + current_params: Optional[dict] = params - timeout = SCHOLARLY_LOOKUP_TIMEOUT - deadline = _scholarly_deadline.get() - if deadline is not None: - remaining = deadline - time.monotonic() - if remaining <= 0: - logger.info("Scholarly lookup budget exhausted before %s", url) + for _ in range(MAX_SCHOLARLY_REDIRECTS + 1): + ok, reason = check_outbound_url(current_url, block_private=True) + if not ok: + logger.warning("Scholarly lookup blocked for %s: %s", current_url, reason) return None - timeout = min(timeout, remaining) - response = httpx.get( - url, - params=params, - headers={"User-Agent": _scholarly_user_agent()}, - timeout=timeout, - follow_redirects=True, - ) - response.raise_for_status() - return response + timeout = SCHOLARLY_LOOKUP_TIMEOUT + deadline = _scholarly_deadline.get() + if deadline is not None: + remaining = deadline - time.monotonic() + if remaining <= 0: + logger.info("Scholarly lookup budget exhausted before %s", current_url) + return None + timeout = min(timeout, remaining) + + response = httpx.get( + current_url, + params=current_params, + headers={"User-Agent": _scholarly_user_agent()}, + timeout=timeout, + follow_redirects=False, + ) + + is_redirect = getattr(response, "is_redirect", False) or ( + getattr(response, "status_code", None) in (301, 302, 303, 307, 308) + ) + if is_redirect: + headers = getattr(response, "headers", {}) + location = headers.get("location") + if not location: + logger.warning( + "Scholarly redirect missing Location header from %s", current_url + ) + return None + current_url = str(httpx.URL(str(response.url)).join(location)) + current_params = None + continue + + response.raise_for_status() + return response + + logger.warning("Scholarly lookup exceeded max redirects from %s", url) + return None def _arxiv_title_results(title: str, count: int = 3) -> list[dict]: diff --git a/src/constants.py b/src/constants.py index d52249e25..633141b22 100644 --- a/src/constants.py +++ b/src/constants.py @@ -147,10 +147,8 @@ SEARXNG_INSTANCE = os.getenv("SEARXNG_INSTANCE", "http://localhost:8080") # could stall a user-facing search for the sum of all three. ARXIV_API_URL = "https://export.arxiv.org/api/query" OPENALEX_API_URL = "https://api.openalex.org/works" -SCHOLARLY_LOOKUP_TIMEOUT = float(os.getenv("ODYSSEUS_SCHOLARLY_LOOKUP_TIMEOUT", "12")) -SCHOLARLY_LOOKUP_TOTAL_BUDGET = float( - os.getenv("ODYSSEUS_SCHOLARLY_LOOKUP_BUDGET", "20") -) +SCHOLARLY_LOOKUP_TIMEOUT = 12.0 +SCHOLARLY_LOOKUP_TOTAL_BUDGET = 20.0 # Cleanup configuration CLEANUP_ENABLED = os.getenv("CLEANUP_ENABLED", "True").lower() == "true" diff --git a/tests/test_review_20260923_fixes.py b/tests/test_review_20260923_fixes.py index aafa72c22..9c1256c25 100644 --- a/tests/test_review_20260923_fixes.py +++ b/tests/test_review_20260923_fixes.py @@ -33,13 +33,37 @@ from src.upload_limits import EDITOR_DRAFT_MAX_BYTES # -------------------------------------------------------------------------- -def test_scholarly_endpoints_come_from_constants_not_literals(): - """The call sites must reference the constants, not inline URLs.""" - source = (search_core.__file__ and open(search_core.__file__).read()) or "" - assert "https://export.arxiv.org" not in source - assert "https://api.openalex.org" not in source - assert ARXIV_API_URL.startswith("https://export.arxiv.org") - assert OPENALEX_API_URL.startswith("https://api.openalex.org") +def test_scholarly_endpoints_use_configured_constants(monkeypatch): + """Call sites must route through configured endpoints, not hardcoded URLs.""" + requested_urls = [] + + def fake_scholarly_api_get(url: str, params: dict): + requested_urls.append(url) + if "arxiv" in url: + class FakeArxivResponse: + text = "" + + return FakeArxivResponse() + elif "openalex" in url: + class FakeOpenAlexResponse: + def json(self): + return {"results": []} + + return FakeOpenAlexResponse() + return None + + monkeypatch.setattr(search_core, "_scholarly_api_get", fake_scholarly_api_get) + + custom_arxiv = "https://custom.arxiv.test/api/query" + custom_openalex = "https://custom.openalex.test/works" + monkeypatch.setattr(search_core, "ARXIV_API_URL", custom_arxiv) + monkeypatch.setattr(search_core, "OPENALEX_API_URL", custom_openalex) + + search_core._arxiv_title_results("Attention Is All You Need") + search_core._openalex_title_results("Attention Is All You Need") + + assert custom_arxiv in requested_urls + assert custom_openalex in requested_urls def test_user_agent_tracks_app_version(): @@ -64,6 +88,105 @@ def test_outbound_policy_rejection_skips_the_request(monkeypatch): assert calls == [] +def test_redirect_to_prohibited_destination_is_blocked_and_never_requested(monkeypatch): + """An allowed initial URL must not be permitted to redirect into a prohibited destination.""" + from src.url_safety import check_outbound_url as real_check + + called_urls = [] + + def fake_get(url, **kwargs): + called_urls.append(url) + req = httpx.Request("GET", url) + return httpx.Response( + 302, + headers={"Location": "http://127.0.0.1:8080/internal-admin"}, + request=req, + ) + + def mock_check(url, **kwargs): + if url == OPENALEX_API_URL: + return (True, "") + return real_check(url, **kwargs) + + monkeypatch.setattr(httpx, "get", fake_get) + monkeypatch.setattr("src.url_safety.check_outbound_url", mock_check) + + result = search_core._scholarly_api_get(OPENALEX_API_URL, {}) + + assert result is None + # Only the initial allowed URL was contacted; the prohibited redirect destination was never requested + assert called_urls == [OPENALEX_API_URL] + + +def test_redirect_to_link_local_metadata_is_blocked_and_never_requested(monkeypatch): + """Redirects to cloud metadata or link-local addresses must be refused before connection.""" + from src.url_safety import check_outbound_url as real_check + + called_urls = [] + + def fake_get(url, **kwargs): + called_urls.append(url) + req = httpx.Request("GET", url) + return httpx.Response( + 301, + headers={"Location": "http://169.254.169.254/latest/meta-data"}, + request=req, + ) + + def mock_check(url, **kwargs): + if url == ARXIV_API_URL: + return (True, "") + return real_check(url, **kwargs) + + monkeypatch.setattr(httpx, "get", fake_get) + monkeypatch.setattr("src.url_safety.check_outbound_url", mock_check) + + result = search_core._scholarly_api_get(ARXIV_API_URL, {}) + + assert result is None + assert called_urls == [ARXIV_API_URL] + + +def test_allowed_redirect_is_followed_safely(monkeypatch): + """A safe redirect destination passing outbound checks is followed to completion.""" + called_urls = [] + canonical_url = "https://api.openalex.org/canonical-works" + + def fake_get(url, **kwargs): + called_urls.append(url) + req = httpx.Request("GET", url) + if url == OPENALEX_API_URL: + return httpx.Response(301, headers={"Location": "/canonical-works"}, request=req) + return httpx.Response(200, json={"results": []}, request=req) + + monkeypatch.setattr(httpx, "get", fake_get) + monkeypatch.setattr("src.url_safety.check_outbound_url", lambda url, **kw: (True, "")) + + result = search_core._scholarly_api_get(OPENALEX_API_URL, {}) + + assert result is not None + assert result.status_code == 200 + assert called_urls == [OPENALEX_API_URL, canonical_url] + + +def test_redirect_limit_is_bounded(monkeypatch): + """Redirects exceeding MAX_SCHOLARLY_REDIRECTS must fail safely without looping.""" + called_urls = [] + + def fake_get(url, **kwargs): + called_urls.append(url) + req = httpx.Request("GET", url) + return httpx.Response(302, headers={"Location": f"{url}/next"}, request=req) + + monkeypatch.setattr(httpx, "get", fake_get) + monkeypatch.setattr("src.url_safety.check_outbound_url", lambda url, **kw: (True, "")) + + result = search_core._scholarly_api_get("https://export.arxiv.org/api/query", {}) + + assert result is None + assert len(called_urls) == search_core.MAX_SCHOLARLY_REDIRECTS + 1 + + def test_exhausted_budget_skips_the_request(monkeypatch): """Once the chain's budget is spent, later hops are skipped, not retried.""" calls = [] @@ -172,6 +295,66 @@ def test_oversized_declared_body_is_refused_before_it_is_parsed(): assert "safety limit" in response.text +def test_oversized_declared_body_guard_runs_before_body_consumption(): + """An oversized Content-Length must reject the request without reading or consuming the body.""" + body_consumed = False + + def body_stream(): + nonlocal body_consumed + body_consumed = True + yield b'{"layers": []}' + + client = _draft_client() + response = client.post( + "/api/editor-drafts", + content=body_stream(), + headers={ + "content-type": "application/json", + "content-length": str(EDITOR_DRAFT_MAX_BYTES + 1), + }, + ) + assert response.status_code == 413 + assert "safety limit" in response.text + # Proves the body stream was never read or consumed before rejection + assert body_consumed is False + + +def test_oversized_declared_body_guard_runs_before_body_consumption_on_put(): + """Update route also rejects oversized Content-Length without consuming body.""" + body_consumed = False + + def body_stream(): + nonlocal body_consumed + body_consumed = True + yield b'{"layers": []}' + + client = _draft_client() + response = client.put( + "/api/editor-drafts/some-draft-id", + content=body_stream(), + headers={ + "content-type": "application/json", + "content-length": str(EDITOR_DRAFT_MAX_BYTES + 1), + }, + ) + assert response.status_code == 413 + assert body_consumed is False + + +def test_oversized_declared_body_rejects_without_json_parsing(): + """Even malformed or invalid JSON is rejected with 413 rather than 422 if Content-Length exceeds ceiling.""" + client = _draft_client() + response = client.post( + "/api/editor-drafts", + content=b"this is completely invalid json {[[", + headers={ + "content-type": "application/json", + "content-length": str(EDITOR_DRAFT_MAX_BYTES + 1), + }, + ) + assert response.status_code == 413 + + def test_update_route_carries_the_same_guard(): client = _draft_client() response = client.put(