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

@@ -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
 

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()

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]:

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"

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 = "<feed xmlns='http://www.w3.org/2005/Atom'></feed>"
+
+            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(