diff --git a/routes/research/research_routes.py b/routes/research/research_routes.py index b35c94b69..fdc650d95 100644 --- a/routes/research/research_routes.py +++ b/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: + resolved.relative_to(root) + except ValueError: + 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: - candidate.relative_to(root) - except ValueError: - raise HTTPException(400, "Invalid session ID") - return candidate + 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: diff --git a/tests/test_research_routes_path_confinement.py b/tests/test_research_routes_path_confinement.py index 29c8a0715..ee19dcdbd 100644 --- a/tests/test_research_routes_path_confinement.py +++ b/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