fix(security): make research path lookup CodeQL-friendly (#5129)

* fix(security): make research path lookup CodeQL-friendly

* fix(security): avoid duplicate research path scans

* fix(research): preserve active completed spinoff query

routes/research/research_routes.py

@@ -21,23 +21,52 @@ from src.constants import DEEP_RESEARCH_DIR
 _SESSION_ID_RE = re.compile(r"^[a-zA-Z0-9-]{1,128}$")
 
 
-def _confine_research_path(session_id: str) -> Path:
-    """Return the resolved Path for session_id's JSON inside DEEP_RESEARCH_DIR.
-
-    Validates the session ID format and asserts containment after symlink
-    expansion. Raises HTTPException(400) on format failures, traversal
-    attempts, absolute-path injection, and symlink escape so every caller
-    gets a safe, confined path with no extra validation needed.
-    """
+def _validate_session_id(session_id: str) -> str:
     if not _SESSION_ID_RE.fullmatch(session_id):
-        raise HTTPException(400, "Invalid session ID")
-    root = Path(DEEP_RESEARCH_DIR).resolve()
-    candidate = (root / f"{session_id}.json").resolve()
+        raise HTTPException(400, "Invalid session ID format")
+    return session_id
+
+
+def _research_storage_root() -> Path:
+    return Path(DEEP_RESEARCH_DIR).resolve()
+
+
+def _find_research_path(session_id: str) -> Path | None:
+    """Find a persisted research file without deriving its path from input."""
+    expected_name = f"{_validate_session_id(session_id)}.json"
+    root = _research_storage_root()
+    for stored_path in root.glob("*.json"):
+        if stored_path.name != expected_name:
+            continue
+        resolved = stored_path.resolve()
         try:
-        candidate.relative_to(root)
+            resolved.relative_to(root)
         except ValueError:
-        raise HTTPException(400, "Invalid session ID")
-    return candidate
+            return None
+        if not resolved.is_file():
+            return None
+        return resolved
+    return None
+
+
+def _require_research_path(session_id: str) -> Path:
+    path = _find_research_path(session_id)
+    if path is None:
+        raise HTTPException(404, "Research not found")
+    return path
+
+
+def _find_owned_research_path(session_id: str, user: str) -> Path | None:
+    path = _find_research_path(session_id)
+    if path is None:
+        return None
+    try:
+        owner = json.loads(path.read_text(encoding="utf-8")).get("owner")
+    except Exception:
+        return None
+    if owner != user:
+        return None
+    return path
 
 
 logger = logging.getLogger(__name__)
@@ -192,10 +221,6 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
             raise HTTPException(401, "Not authenticated")
         return user
 
-    def _validate_session_id(session_id: str) -> None:
-        if not _SESSION_ID_RE.fullmatch(session_id):
-            raise HTTPException(400, "Invalid session ID format")
-
     def _owns_in_memory(session_id: str, user: str) -> bool:
         """Ownership check for an in-flight (in-memory) research task.
         Falls back to the on-disk JSON if the task has already finished."""
@@ -204,15 +229,32 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
             return entry.get("owner", "") == user
         # Task no longer in memory — check the persisted JSON.
         try:
-            path = _confine_research_path(session_id)
+            return _find_owned_research_path(session_id, user) is not None
         except HTTPException:
             return False
-        if not path.exists():
-            return False
-        try:
-            return json.loads(path.read_text(encoding="utf-8")).get("owner") == user
-        except Exception:
-            return False
+
+    def _require_owned_or_active_research_path(session_id: str, user: str) -> Path | None:
+        """Validate ownership once and return the completed on-disk path.
+
+        Active running research has no completed disk path yet. Completed
+        tasks can remain in _active_tasks after persistence, so prefer their
+        owned disk path when available. Completed disk lookups still reuse the
+        path after the ownership gate.
+        """
+        entry = research_handler._active_tasks.get(session_id)
+        if entry is not None:
+            if entry.get("owner", "") != user:
+                raise HTTPException(404, "No research found for this session")
+            if entry.get("status") != "running":
+                path = _find_owned_research_path(session_id, user)
+                if path is not None:
+                    return path
+            return None
+
+        path = _find_owned_research_path(session_id, user)
+        if path is None:
+            raise HTTPException(404, "No research found for this session")
+        return path
 
     @router.get("/api/research/active")
     async def research_active(request: Request):
@@ -270,9 +312,7 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
     def _assert_owns_research(session_id: str, user: str) -> None:
         """404-not-403 ownership gate for a research session's on-disk JSON.
         Use BEFORE returning any data or mutating the file."""
-        path = _confine_research_path(session_id)
-        if not path.exists():
-            raise HTTPException(404, "Research not found")
+        path = _require_research_path(session_id)
         try:
             owner = json.loads(path.read_text(encoding="utf-8")).get("owner")
         except Exception:
@@ -384,9 +424,7 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         summary, stats — used by the Library preview panel."""
         user = _require_user(request)
         _validate_session_id(session_id)
-        path = _confine_research_path(session_id)
-        if not path.exists():
-            raise HTTPException(404, "Research not found")
+        path = _require_research_path(session_id)
         try:
             data = json.loads(path.read_text(encoding="utf-8"))
         except Exception as e:
@@ -401,9 +439,7 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         """Soft-archive / restore a research report (sets `archived` in its JSON)."""
         user = _require_user(request)
         _validate_session_id(session_id)
-        path = _confine_research_path(session_id)
-        if not path.exists():
-            raise HTTPException(404, "Research not found")
+        path = _require_research_path(session_id)
         try:
             data = json.loads(path.read_text(encoding="utf-8"))
             if data.get("owner") != user:
@@ -421,9 +457,9 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         """Delete a research result from disk."""
         user = _require_user(request)
         _validate_session_id(session_id)
-        json_path = _confine_research_path(session_id)
+        json_path = _find_research_path(session_id)
         deleted = False
-        if json_path.exists():
+        if json_path is not None:
             # SECURITY: verify ownership before letting the caller delete it.
             try:
                 data = json.loads(json_path.read_text(encoding="utf-8"))
@@ -579,12 +615,11 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         """Get research result without clearing it (for panel use)."""
         user = _require_user(request)
         _validate_session_id(session_id)
-        if not _owns_in_memory(session_id, user):
-            raise HTTPException(404, "No research found for this session")
+        owned_disk_path = _require_owned_or_active_research_path(session_id, user)
         result = research_handler.get_result(session_id)
         if result is None:
-            p = _confine_research_path(session_id)
-            if p.exists():
+            p = owned_disk_path
+            if p is not None:
                 d = json.loads(p.read_text(encoding="utf-8"))
                 return {
                     "result": d.get("result", ""),
@@ -613,8 +648,7 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         # otherwise any authenticated user could spin off (and thereby read)
         # another user's report by guessing its session ID. Mirrors every other
         # endpoint in this file (see result_peek above).
-        if not _owns_in_memory(session_id, user):
-            raise HTTPException(404, "No research found for this session")
+        owned_disk_path = _require_owned_or_active_research_path(session_id, user)
         if session_manager is None:
             raise HTTPException(500, "session_manager not configured")
 
@@ -623,8 +657,8 @@ def setup_research_routes(research_handler, session_manager=None) -> APIRouter:
         sources = research_handler.get_sources(session_id) or []
         query = ""
 
-        path = _confine_research_path(session_id)
-        if path.exists():
+        path = owned_disk_path
+        if path is not None:
             try:
                 disk = json.loads(path.read_text(encoding="utf-8"))
                 if not result:

tests/test_research_routes_path_confinement.py

@@ -1,6 +1,6 @@
 """Path-confinement regression tests for research routes.
 
-Covers the CodeQL py/path-injection alert cluster (#552-#567) in
+Covers the CodeQL py/path-injection alert cluster (#552-#567 and #594) in
 routes/research/research_routes.py:
   - _owns_in_memory disk fallback (alerts #552, #553)
   - _assert_owns_research (alerts #554, #555)
@@ -20,7 +20,11 @@ import pytest
 from fastapi import HTTPException
 
 from routes.research_routes import setup_research_routes
-from routes.research.research_routes import _confine_research_path
+from routes.research.research_routes import (
+    _find_owned_research_path,
+    _find_research_path,
+    _require_research_path,
+)
 
 
 @pytest.fixture(autouse=True)
@@ -58,15 +62,30 @@ def _research_handler():
 
 
 # ---------------------------------------------------------------------------
-# Helper-level tests — _confine_research_path
+# Helper-level tests
 # ---------------------------------------------------------------------------
 
-def test_confine_allows_valid_session_id(tmp_path, monkeypatch):
+def test_find_returns_existing_trusted_research_path(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    expected = _write_research(data_dir, "rp-abc123de4567", owner="alice")
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+    assert _find_research_path("rp-abc123de4567") == expected.resolve()
+
+
+def test_find_returns_none_for_missing_valid_session_id(tmp_path, monkeypatch):
     data_dir = tmp_path / "deep_research"
     data_dir.mkdir()
     monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
-    path = _confine_research_path("rp-abc123de4567")
-    assert path == (data_dir / "rp-abc123de4567.json").resolve()
+    assert _find_research_path("rp-missing12345") is None
+
+
+def test_require_returns_404_for_missing_valid_session_id(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    data_dir.mkdir()
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+    with pytest.raises(HTTPException) as exc:
+        _require_research_path("rp-missing12345")
+    assert exc.value.status_code == 404
 
 
 @pytest.mark.parametrize("bad_id", [
@@ -79,17 +98,45 @@ def test_confine_allows_valid_session_id(tmp_path, monkeypatch):
     "rp-bad.json",     # dot not in allowed charset
     "a" * 129,         # exceeds length limit
 ])
-def test_confine_rejects_bad_session_ids(tmp_path, monkeypatch, bad_id):
-    data_dir = tmp_path / "deep_research"
-    data_dir.mkdir()
-    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+def test_find_rejects_bad_session_ids_before_enumeration(monkeypatch, bad_id):
+    storage_root = MagicMock()
+    monkeypatch.setattr(
+        "routes.research.research_routes._research_storage_root",
+        MagicMock(return_value=storage_root),
+    )
     with pytest.raises(HTTPException) as exc:
-        _confine_research_path(bad_id)
+        _find_research_path(bad_id)
     assert exc.value.status_code == 400
+    storage_root.glob.assert_not_called()
 
 
-def test_confine_rejects_symlink_escape(tmp_path, monkeypatch):
-    """A symlink inside DEEP_RESEARCH_DIR that resolves outside is rejected."""
+def test_find_matches_names_from_trusted_enumeration_without_joining_input(
+    tmp_path, monkeypatch
+):
+    """Pin the CodeQL-friendly lookup: match a glob result, never root / input."""
+    data_dir = tmp_path / "deep_research"
+    expected = _write_research(data_dir, "rp-abc123de4567", owner="alice").resolve()
+
+    class EnumeratedRoot:
+        def glob(self, pattern):
+            assert pattern == "*.json"
+            return [expected]
+
+        def __fspath__(self):
+            return str(data_dir.resolve())
+
+        def __truediv__(self, _other):
+            raise AssertionError("user-derived path segment was joined to root")
+
+    monkeypatch.setattr(
+        "routes.research.research_routes._research_storage_root",
+        lambda: EnumeratedRoot(),
+    )
+    assert _find_research_path("rp-abc123de4567") == expected
+
+
+def test_find_ignores_symlink_escape(tmp_path, monkeypatch):
+    """A matching symlink that resolves outside is not a trusted file."""
     data_dir = tmp_path / "deep_research"
     outside = tmp_path / "outside"
     data_dir.mkdir()
@@ -102,9 +149,24 @@ def test_confine_rejects_symlink_escape(tmp_path, monkeypatch):
         link.symlink_to(target)
     except (AttributeError, NotImplementedError, OSError) as e:
         pytest.skip(f"symlinks unavailable: {e}")
-    with pytest.raises(HTTPException) as exc:
-        _confine_research_path("rp-linktest1234")
-    assert exc.value.status_code == 400
+    assert _find_research_path("rp-linktest1234") is None
+
+
+
+def test_find_owned_returns_path_for_matching_owner(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    expected = _write_research(data_dir, "rp-ownedalice1", owner="alice")
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+
+    assert _find_owned_research_path("rp-ownedalice1", "alice") == expected.resolve()
+
+
+def test_find_owned_returns_none_for_other_owner(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    _write_research(data_dir, "rp-ownedbybob12", owner="bob")
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+
+    assert _find_owned_research_path("rp-ownedbybob12", "alice") is None
 
 
 # ---------------------------------------------------------------------------
@@ -120,6 +182,24 @@ def test_detail_returns_data_for_owner(tmp_path):
     assert out["query"] == "valid query"
 
 
+def test_detail_returns_404_for_missing_valid_id():
+    router = setup_research_routes(_research_handler())
+    target = _route(router, "/api/research/detail/{session_id}", "GET")
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(target(session_id="rp-missing12345", request=_request("alice")))
+    assert exc.value.status_code == 404
+
+
+def test_detail_hides_other_owners_research_with_404(tmp_path):
+    data_dir = tmp_path / "deep_research"
+    _write_research(data_dir, "rp-ownedbybob12", owner="bob")
+    router = setup_research_routes(_research_handler())
+    target = _route(router, "/api/research/detail/{session_id}", "GET")
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(target(session_id="rp-ownedbybob12", request=_request("alice")))
+    assert exc.value.status_code == 404
+
+
 # ---------------------------------------------------------------------------
 # Route-level tests — traversal and injection rejected
 # ---------------------------------------------------------------------------
@@ -214,28 +294,76 @@ def test_detail_traversal_does_not_read_outside_file(tmp_path, monkeypatch):
 # Route-level symlink escape test
 # ---------------------------------------------------------------------------
 
-def test_detail_rejects_symlink_escape(tmp_path, monkeypatch):
-    """research_detail rejects a confined-format ID whose JSON is a symlink to outside."""
+def _write_outside_symlink(tmp_path, session_id: str, data: dict):
     data_dir = tmp_path / "deep_research"
     outside_dir = tmp_path / "outside"
     data_dir.mkdir(parents=True)
     outside_dir.mkdir()
-    outside_file = outside_dir / "rp-linktest5678.json"
-    outside_file.write_text(
-        json.dumps({"owner": "alice", "result": "secret"}), encoding="utf-8"
-    )
-    link = data_dir / "rp-linktest5678.json"
+    outside_file = outside_dir / f"{session_id}.json"
+    outside_file.write_text(json.dumps(data), encoding="utf-8")
+    link = data_dir / f"{session_id}.json"
     try:
         link.symlink_to(outside_file)
     except (AttributeError, NotImplementedError, OSError) as e:
         pytest.skip(f"symlinks unavailable: {e}")
+    return data_dir, outside_file
+
+
+def test_detail_rejects_symlink_escape(tmp_path, monkeypatch):
+    """research_detail never reads a matching symlink outside the root."""
+    data_dir, _ = _write_outside_symlink(
+        tmp_path,
+        "rp-linktest5678",
+        {"owner": "alice", "result": "secret"},
+    )
     monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
 
     router = setup_research_routes(_research_handler())
     target = _route(router, "/api/research/detail/{session_id}", "GET")
     with pytest.raises(HTTPException) as exc:
         asyncio.run(target(session_id="rp-linktest5678", request=_request("alice")))
-    assert exc.value.status_code == 400
+    assert exc.value.status_code == 404
+
+
+def test_archive_does_not_write_through_symlink_escape(tmp_path, monkeypatch):
+    data_dir, outside_file = _write_outside_symlink(
+        tmp_path,
+        "rp-linkarchive1",
+        {"owner": "alice", "archived": False},
+    )
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+
+    router = setup_research_routes(_research_handler())
+    target = _route(router, "/api/research/{session_id}/archive", "POST")
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(
+            target(
+                session_id="rp-linkarchive1",
+                request=_request("alice"),
+                archived=True,
+            )
+        )
+    assert exc.value.status_code == 404
+    assert json.loads(outside_file.read_text(encoding="utf-8"))["archived"] is False
+
+
+def test_delete_does_not_unlink_symlink_escape(tmp_path, monkeypatch):
+    data_dir, outside_file = _write_outside_symlink(
+        tmp_path,
+        "rp-linkdelete12",
+        {"owner": "alice"},
+    )
+    link = data_dir / "rp-linkdelete12.json"
+    monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
+
+    router = setup_research_routes(_research_handler())
+    target = _route(router, "/api/research/{session_id}", "DELETE")
+    out = asyncio.run(
+        target(session_id="rp-linkdelete12", request=_request("alice"))
+    )
+    assert out == {"deleted": False}
+    assert link.is_symlink()
+    assert outside_file.exists()
 
 
 # ---------------------------------------------------------------------------
@@ -243,14 +371,15 @@ def test_detail_rejects_symlink_escape(tmp_path, monkeypatch):
 # ---------------------------------------------------------------------------
 
 def test_owner_scoped_paths_stay_within_research_root(tmp_path, monkeypatch):
-    """Owner-scoped session IDs never produce paths outside DEEP_RESEARCH_DIR."""
+    """Owner-scoped persisted files resolve within DEEP_RESEARCH_DIR."""
     data_dir = tmp_path / "deep_research"
     data_dir.mkdir(parents=True)
     monkeypatch.setattr("routes.research_routes.DEEP_RESEARCH_DIR", str(data_dir))
 
     root = data_dir.resolve()
     for session_id in ("rp-abc123456789", "rp-000000000001", "abc-xyz-123"):
-        path = _confine_research_path(session_id)
+        _write_research(data_dir, session_id, owner="alice")
+        path = _require_research_path(session_id)
         assert path.resolve().is_relative_to(root), (
             f"{session_id!r} produced path outside research root: {path}"
         )
@@ -271,3 +400,166 @@ def test_spinoff_rejects_traversal(bad_id):
     with pytest.raises(HTTPException) as exc:
         asyncio.run(target(session_id=bad_id, request=_request("alice")))
     assert exc.value.status_code == 400
+
+def test_result_peek_uses_single_disk_lookup_for_completed_result(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    path = _write_research(
+        data_dir,
+        "rp-peeksingle1",
+        owner="alice",
+        result="saved result",
+        sources=["s1"],
+        raw_findings=["f1"],
+        category="security",
+    ).resolve()
+
+    calls = []
+
+    def fake_find_owned(session_id, user):
+        calls.append((session_id, user))
+        return path
+
+    monkeypatch.setattr(
+        "routes.research.research_routes._find_owned_research_path",
+        fake_find_owned,
+    )
+
+    handler = _research_handler()
+    handler.get_result.return_value = None
+    router = setup_research_routes(handler)
+    target = _route(router, "/api/research/result-peek/{session_id}", "POST")
+
+    out = asyncio.run(target(session_id="rp-peeksingle1", request=_request("alice")))
+
+    assert out["result"] == "saved result"
+    assert out["sources"] == ["s1"]
+    assert out["raw_findings"] == ["f1"]
+    assert out["category"] == "security"
+    assert calls == [("rp-peeksingle1", "alice")]
+
+
+def test_spinoff_uses_single_disk_lookup_for_completed_result(tmp_path, monkeypatch):
+    data_dir = tmp_path / "deep_research"
+    path = _write_research(
+        data_dir,
+        "rp-spinsingle1",
+        owner="alice",
+        result="saved report",
+        sources=["s1", "s2"],
+        query="original query",
+    ).resolve()
+
+    calls = []
+
+    def fake_find_owned(session_id, user):
+        calls.append((session_id, user))
+        return path
+
+    class FakeSession:
+        endpoint_url = ""
+        model = ""
+        headers = {}
+
+        def __init__(self):
+            self.messages = []
+
+        def add_message(self, message):
+            self.messages.append(message)
+
+    class FakeSessionManager:
+        def __init__(self):
+            self.created = None
+
+        def get_session(self, session_id):
+            raise KeyError(session_id)
+
+        def create_session(self, **kwargs):
+            self.created = FakeSession()
+            return self.created
+
+        def save_sessions(self):
+            pass
+
+    monkeypatch.setattr(
+        "routes.research.research_routes._find_owned_research_path",
+        fake_find_owned,
+    )
+    monkeypatch.setattr(
+        "routes.research.research_routes.resolve_endpoint",
+        lambda *_args, **_kwargs: ("http://endpoint/v1", "model", {}),
+    )
+
+    handler = _research_handler()
+    handler.get_result.return_value = None
+    handler.get_sources.return_value = []
+    session_manager = FakeSessionManager()
+    router = setup_research_routes(handler, session_manager=session_manager)
+    target = _route(router, "/api/research/spinoff/{session_id}", "POST")
+
+    out = asyncio.run(target(session_id="rp-spinsingle1", request=_request("alice")))
+
+    assert out["name"] == "Follow-up: original query"
+    assert out["source_count"] == 2
+    assert calls == [("rp-spinsingle1", "alice")]
+    assert session_manager.created is not None
+    assert session_manager.created.messages
+
+def test_spinoff_reads_saved_query_for_done_active_task(tmp_path, monkeypatch):
+    session_id = "rp-activedone1"
+    data_dir = tmp_path / "deep_research"
+    _write_research(
+        data_dir,
+        session_id,
+        owner="alice",
+        result="saved report",
+        sources=["s1"],
+        query="completed query",
+    )
+
+    class FakeSession:
+        endpoint_url = ""
+        model = ""
+        headers = {}
+
+        def __init__(self):
+            self.messages = []
+
+        def add_message(self, message):
+            self.messages.append(message)
+
+    class FakeSessionManager:
+        def __init__(self):
+            self.created = None
+
+        def get_session(self, session_id):
+            raise KeyError(session_id)
+
+        def create_session(self, **kwargs):
+            self.created = FakeSession()
+            return self.created
+
+        def save_sessions(self):
+            pass
+
+    monkeypatch.setattr(
+        "routes.research.research_routes.resolve_endpoint",
+        lambda *_args, **_kwargs: ("http://endpoint/v1", "model", {}),
+    )
+
+    handler = _research_handler()
+    handler._active_tasks[session_id] = {"owner": "alice", "status": "done"}
+    handler.get_result.return_value = None
+    handler.get_sources.return_value = []
+
+    session_manager = FakeSessionManager()
+    router = setup_research_routes(handler, session_manager=session_manager)
+    target = _route(router, "/api/research/spinoff/{session_id}", "POST")
+
+    out = asyncio.run(target(session_id=session_id, request=_request("alice")))
+
+    assert out["name"] == "Follow-up: completed query"
+    assert out["source_count"] == 1
+    assert session_manager.created is not None
+    primer = session_manager.created.messages[0].content
+    assert "completed query" in primer
+    assert "(not recorded)" not in primer