diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1199001..c146a3a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -7,7 +7,7 @@ How `recoll-modern` is built today. Update in the same commit as any structural The Python service scaffold, SQLite schema, collection and membership repositories, document discovery/identity modules, durable job worker, engine adapter, authentication service, and initial HTTP shell, membership routes, job routes, scoped search, document retrieval, and Docker packaging are -built. Final contract, recovery and security acceptance coverage remains. Recoll behaviour +built and covered by host and container acceptance tests. Recoll behaviour below was verified against a real container. The goal: a self-hosted service that wraps Recoll's indexing and full-text search behind (a) a modern @@ -36,6 +36,7 @@ web UI and (b) an MCP server, running in Docker, with several independent collec | `src/recoll_modern/api/` | HTTP auth, health, file browsing, collection/membership/rule/job resources, search, document retrieval, prefix/proxy handling and JSON errors | | `src/recoll_modern/domain/search.py` | Validates explicit scope and fields, projects current SQLite identities and membership onto engine-ranked hits | | `tests/` | Automated service tests | +| `tests/contract/`, `tests/integration/`, `tests/security/` | Requirement matrix, real-container Recoll contract, lifecycle, and boundary checks | Discovery produces content hashes and root-relative paths without writing to the document volume. The document repository records these in one transaction, chooses the smallest live path as canonical, @@ -123,6 +124,12 @@ absent; Recoll may generate additional support files there. Compose mounts docum keeps the SQLite/config and Xapian index volumes separate. Initial health can be 503 until an indexing job creates the first index. +The host Recoll contract tests skip when no container is running. Required-container mode executes the +adapter inside Compose, avoiding a host Recoll dependency; the generated-corpus test proves that a +read-only file can be indexed, searched in two collections, and retrieved as extracted text. The HTTP +lifecycle test uses a deterministic engine to exercise rules, identity, projection, canonical-path +promotion and interrupted-job recovery. + The resource groups are auth, health, read-only file browsing, collections and membership sources, directory rules, scoped search, document text/original retrieval, and indexing jobs. Search callers select returned fields; only `document_id` is unconditional, and inline full text is bounded. Every diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 6e9b40a..dca4686 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -316,3 +316,15 @@ replacement); run the service as root. **Consequences:** service-state backups include generated Recoll configuration, and config changes made by an operator persist across image upgrades. Template updates require an explicit migration if an existing config should adopt them. + +--- + +## 2026-09-26 — Treat an empty Recoll document URL as a missing hit +**Status:** accepted +**Context:** the live Recoll 1.43 Python binding returns a `Doc` object with an empty `getbinurl()` for +a UDI absent from the index; it does not return `None`. Passing that object to `rclextract.Extractor` +raises an internal-file error. +**Decision:** check the returned URL before extraction and return empty text for an absent UDI. +**Alternatives:** catch the generic extraction error, which would hide genuine extraction failures. +**Consequences:** exact-path text lookup follows the engine contract for missing documents while real +extraction failures still surface as `engine_unavailable`. diff --git a/docs/TODO.md b/docs/TODO.md index 7e62980..0d02803 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -15,14 +15,13 @@ Where the workflow stands (`AGENTS.md` -> Workflow: brainstorm -> spec -> plan - selectable search fields, subpath hosting and the packaging seam for deferred desktop spec 005. - Plan `docs/plans/001-search-engine-api.md`: **approved 2026-09-18.** Its listed Python runtime and development dependencies are approved. Execution follows the plan task by task. -- Plan Tasks 1–3 are complete and independently reviewed. Tasks 4–13 add discovery, content-hash +- Plan Tasks 1–3 are complete and independently reviewed. Tasks 4–14 add discovery, content-hash identity, additive membership, durable jobs, the atomic membership map, the engine boundary, session authentication, the HTTP shell, membership/rule/job routes, scoped search and document - retrieval and Docker packaging. Their checkboxes are current in the plan. The next task is final - contract, recovery, security and acceptance coverage. + retrieval, Docker packaging and acceptance coverage. Their checkboxes are current in the plan. + Final code review and branch handoff remain. -The next action is Task 14, "Complete contract, recovery, security, and acceptance coverage." Do not -repeat Tasks 1–13. +The next action is Task 14 Step 6, "Request review before integration." Do not repeat Tasks 1–14. Things a resuming agent will otherwise get wrong: @@ -56,7 +55,7 @@ Things a resuming agent will otherwise get wrong: once per file per index run. - [ ] Re-measure indexing overhead against real documents (PDF, EPUB) rather than small text files — extraction cost should dwarf the helper, but that is an assumption until measured. -- [ ] Promote a new canonical path and force a reindex when a canonical path disappears, or the +- [x] Promote a new canonical path and force a reindex when a canonical path disappears, or the document silently leaves every collection it is in. - [x] Recover interrupted indexing jobs at startup; Task 6 requeues valid running jobs and fails unreadable requests. diff --git a/docs/plans/001-search-engine-api.md b/docs/plans/001-search-engine-api.md index 3af9581..012738c 100644 --- a/docs/plans/001-search-engine-api.md +++ b/docs/plans/001-search-engine-api.md @@ -980,11 +980,11 @@ git commit -m "feat: package Recoll service" - Consumes: all prior tasks - Produces: executable evidence for every spec acceptance criterion -- [ ] **Step 1: Add a requirement-to-test matrix** +- [x] **Step 1: Add a requirement-to-test matrix** At the top of `test_http_contract.py`, map `R1` through `R31` and `C1` through `C4` to named tests. Write missing tests before changing implementation. The end-to-end flow must: login under `/recoll`; create two collections; add explicit, snapshot, and persistent sources; exclude `.calibre`; index duplicate/same-name fixtures; search one and several collections; request projected fields and bounded text; fetch full text and a byte range; reconcile a moved/deleted canonical path; restart during a running job; and verify recovery. -- [ ] **Step 2: Prove each new test fails for its intended reason** +- [x] **Step 2: Prove each new test fails for its intended reason** Run each new node individually, for example: @@ -995,11 +995,11 @@ uv run pytest tests/security/test_proxy_headers.py::test_direct_client_cannot_fo Expected: each new test fails on the missing/incorrect behavior, not fixture setup. -- [ ] **Step 3: Apply minimal fixes and rerun focused tests** +- [x] **Step 3: Apply minimal fixes and rerun focused tests** For every failure, change only the responsible module, rerun the single red test to green, then its containing test file. Append deferred discoveries to `docs/TODO.md`; do not leave source TODO comments. -- [ ] **Step 4: Run the complete verification matrix** +- [x] **Step 4: Run the complete verification matrix** Run in this order: @@ -1020,7 +1020,7 @@ git status --short Expected: every command exits 0; host suite reports explicit skips only for Recoll-dependent tests when the container is down; required-container suite has no such skips; Git status contains only the intended final documentation/checkbox changes before the commit. -- [ ] **Step 5: Complete docs and commit** +- [x] **Step 5: Complete docs and commit** Check every acceptance box in the spec only when its named test passed in this session. Update the architecture map/data flow, append decisions that an implementer actually made, remove completed TODO diff --git a/docs/specs/001-search-engine-api.md b/docs/specs/001-search-engine-api.md index 8f66a37..bfe5e20 100644 --- a/docs/specs/001-search-engine-api.md +++ b/docs/specs/001-search-engine-api.md @@ -284,36 +284,36 @@ exists. Unexpected errors return a request ID rather than exception text or file ## Acceptance criteria -- [ ] A collection can be created, renamed and deleted over the API; renaming enqueues no job (R1–R4). -- [ ] Deleting without a matching confirmation value is refused, and no index is deleted or rebuilt (R4). -- [ ] Two files in the same directory can belong to different collections, and each is found only by its +- [x] A collection can be created, renamed and deleted over the API; renaming enqueues no job (R1–R4). +- [x] Deleting without a matching confirmation value is refused, and no index is deleted or rebuilt (R4). +- [x] Two files in the same directory can belong to different collections, and each is found only by its own collection's scope (C2, R6). -- [ ] Two byte-identical files under different names are one document and appear once in a scoped +- [x] Two byte-identical files under different names are one document and appear once in a scoped search (C3, R5, R7). -- [ ] Two files sharing a name with different content are two documents and both are findable (C3). -- [ ] Removing a canonical path promotes another path and the document remains findable (R8). -- [ ] Snapshot directories add their current documents once; persistent directories reconcile at +- [x] Two files sharing a name with different content are two documents and both are findable (C3). +- [x] Removing a canonical path promotes another path and the document remains findable (R8). +- [x] Snapshot directories add their current documents once; persistent directories reconcile at startup and on reindex, honoring scoped exact-directory exclusions (R10–R12). -- [ ] A direct file addition remains a member despite a rule exclusion, and removing a rule never +- [x] A direct file addition remains a member despite a rule exclusion, and removing a rule never removes membership contributed by another source (R11). -- [ ] A strict job applies no membership changes after any discovery failure; the same best-effort job +- [x] A strict job applies no membership changes after any discovery failure; the same best-effort job applies valid files and reports `completed_with_errors` (R18). -- [ ] A search without `collections` is rejected with `scope_required` (C4, R19). -- [ ] A search across two collections returns one ranked set, with each hit reporting its collections +- [x] A search without `collections` is rejected with `scope_required` (C4, R19). +- [x] A search across two collections returns one ranked set, with each hit reporting its collections (R20, R21). -- [ ] Search field selection omits unrequested data; inline text is bounded and full authenticated text +- [x] Search field selection omits unrequested data; inline text is bounded and full authenticated text and ranged original-file retrieval work (R21–R22). -- [ ] A membership change is visible to search after its job completes, without the document being +- [x] A membership change is visible to search after its job completes, without the document being written to (C1, R13, R14). -- [ ] Indexing a document volume mounted `:ro` succeeds (C1). -- [ ] A job interrupted by a restart is not left claiming to be in progress (R17). -- [ ] An unauthenticated request to any endpoint except `/health` and `/auth/login` is refused (R24). -- [ ] The service refuses to start with no user and no seeded credentials (R26). -- [ ] Hosting under `/recoll` produces working routes, cookies, OpenAPI and document links, while +- [x] Indexing a document volume mounted `:ro` succeeds (C1). +- [x] A job interrupted by a restart is not left claiming to be in progress (R17). +- [x] An unauthenticated request to any endpoint except `/health` and `/auth/login` is refused (R24). +- [x] The service refuses to start with no user and no seeded credentials (R26). +- [x] Hosting under `/recoll` produces working routes, cookies, OpenAPI and document links, while untrusted forwarded-prefix headers are ignored (R29). -- [ ] The service uses non-default state paths and listen settings without code changes, contains no +- [x] The service uses non-default state paths and listen settings without code changes, contains no runtime Docker invocation, and recovers durable work after a cleanly handled shutdown (R31). -- [ ] `uv run pytest`, `uv run ruff check .` and `uv run mypy src` pass, and the suite skips rather than +- [x] `uv run pytest`, `uv run ruff check .` and `uv run mypy src` pass, and the suite skips rather than fails with the container down. ## Known gaps diff --git a/src/recoll_modern/api/documents.py b/src/recoll_modern/api/documents.py index b9dbc04..ef85b1e 100644 --- a/src/recoll_modern/api/documents.py +++ b/src/recoll_modern/api/documents.py @@ -23,11 +23,11 @@ def _path(request: Request, document_hash: str) -> Path: if document is None: raise AppError("document_not_found", "Document was not found", 404) if document.canonical_path is None: - raise AppError("document_missing", "Document is missing", 404) + raise AppError("document_missing", "Document is missing", 410) root = request.app.state.path_policy.root path = (root / document.canonical_path).resolve() if not path.is_relative_to(root) or not path.is_file(): - raise AppError("document_missing", "Document is missing", 404) + raise AppError("document_missing", "Document is missing", 410) return cast(Path, path) diff --git a/src/recoll_modern/domain/search.py b/src/recoll_modern/domain/search.py index 696aad1..ae784ae 100644 --- a/src/recoll_modern/domain/search.py +++ b/src/recoll_modern/domain/search.py @@ -16,6 +16,24 @@ ALLOWED_FIELDS = frozenset({ DEFAULT_FIELDS = ALLOWED_FIELDS - {"text", "links"} +def valid_query(query: str) -> bool: + """Keep user grouping entirely inside the engine's mandatory scope clause.""" + if not query.strip() or "\\" in query: + return False + depth = 0 + quoted = False + for character in query: + if character == '"': + quoted = not quoted + elif not quoted and character == "(": + depth += 1 + elif not quoted and character == ")": + depth -= 1 + if depth < 0: + return False + return depth == 0 and not quoted + + @dataclass(frozen=True, slots=True) class SearchInput: query: str @@ -45,12 +63,12 @@ class SearchService: self._text_limit_max = text_limit_max def search(self, search: SearchInput) -> SearchResult: - if not search.query.strip(): + if not valid_query(search.query): raise AppError("invalid_query", "Query must not be empty", 400) if not search.collection_ids: raise AppError("scope_required", "At least one collection is required", 400) if search.fields is not None and set(search.fields) - ALLOWED_FIELDS: - raise AppError("invalid_query", "Unknown search field", 400) + raise AppError("invalid_field", "Unknown search field", 422) if not 1 <= search.limit <= 100 or search.offset < 0 or search.text_limit < 0: raise AppError("validation_failed", "Invalid search bounds", 422) fields = DEFAULT_FIELDS if search.fields is None else set(search.fields) @@ -61,6 +79,8 @@ class SearchService: )) except EngineUnavailable: raise AppError("engine_unavailable", "Search engine is unavailable", 503) from None + except ValueError: + raise AppError("invalid_query", "Query is invalid", 400) from None results: list[dict[str, object]] = [] for hit in page.hits: try: diff --git a/src/recoll_modern/engine/recoll.py b/src/recoll_modern/engine/recoll.py index e6c0c33..acc74a4 100644 --- a/src/recoll_modern/engine/recoll.py +++ b/src/recoll_modern/engine/recoll.py @@ -8,6 +8,7 @@ from datetime import UTC, datetime from pathlib import Path from typing import Any +from recoll_modern.domain.search import valid_query from recoll_modern.engine.protocol import ( EngineHealth, EngineHit, @@ -46,7 +47,7 @@ class RecollEngine: return EngineHealth(True) def search(self, request: SearchRequest) -> EnginePage: - if not _safe_query(request.query): + if not valid_query(request.query): raise ValueError("query syntax cannot escape collection scope") if not request.collection_ids or any( _COLLECTION_ID.fullmatch(collection_id) is None @@ -86,7 +87,7 @@ class RecollEngine: db = self._connect() try: doc = db.getDoc(fsudi.fs_udi(path)) - if doc is None: + if doc is None or not doc.getbinurl(): return "" extracted = rclextract.Extractor(doc).textextract(doc.ipath) return str(extracted.text) @@ -143,21 +144,3 @@ def _hit(doc: Any, query: Any, fetch_text: bool, rank: int) -> EngineHit: snippets=snippets, text=str(getattr(doc, "text", "")) if fetch_text else None, ) - - -def _safe_query(query: str) -> bool: - """Keep user grouping inside the enclosing scope expression.""" - if not query.strip() or "\\" in query: - return False - depth = 0 - quoted = False - for character in query: - if character == '"': - quoted = not quoted - elif not quoted and character == "(": - depth += 1 - elif not quoted and character == ")": - depth -= 1 - if depth < 0: - return False - return depth == 0 and not quoted diff --git a/tests/api/conftest.py b/tests/api/conftest.py deleted file mode 100644 index 46cf19c..0000000 --- a/tests/api/conftest.py +++ /dev/null @@ -1,47 +0,0 @@ -from collections.abc import Generator -from pathlib import Path - -import pytest -from fastapi.testclient import TestClient - -from recoll_modern.app import create_app -from recoll_modern.config import Settings -from recoll_modern.engine.fake import FakeEngine - - -@pytest.fixture -def app_client(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Generator[TestClient]: - monkeypatch.setenv("RECOLL_MODERN_ADMIN_USERNAME", "admin") - monkeypatch.setenv("RECOLL_MODERN_ADMIN_PASSWORD", "secret-password") - (tmp_path / "docs").mkdir() - settings = Settings.from_env({ - "RECOLL_MODERN_DOCUMENT_ROOT": str(tmp_path / "docs"), - "RECOLL_MODERN_STATE_ROOT": str(tmp_path / "state"), - "RECOLL_MODERN_RECOLL_CONFIG_DIR": str(tmp_path / "recoll"), - }) - with TestClient(create_app(settings, FakeEngine([])), base_url="https://testserver") as client: - yield client - - -@pytest.fixture -def prefixed_client(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Generator[TestClient]: - monkeypatch.setenv("RECOLL_MODERN_ADMIN_USERNAME", "admin") - monkeypatch.setenv("RECOLL_MODERN_ADMIN_PASSWORD", "secret-password") - (tmp_path / "docs").mkdir() - settings = Settings.from_env({ - "RECOLL_MODERN_DOCUMENT_ROOT": str(tmp_path / "docs"), - "RECOLL_MODERN_STATE_ROOT": str(tmp_path / "state"), - "RECOLL_MODERN_RECOLL_CONFIG_DIR": str(tmp_path / "recoll"), - "RECOLL_MODERN_ROOT_PATH": "/recoll", - }) - with TestClient(create_app(settings, FakeEngine([])), base_url="https://testserver") as client: - yield client - - -@pytest.fixture -def authenticated(app_client: TestClient) -> tuple[TestClient, str]: - response = app_client.post( - "/auth/login", json={"username": "admin", "password": "secret-password"} - ) - assert response.status_code == 200 - return app_client, response.json()["csrf_token"] diff --git a/tests/api/test_documents.py b/tests/api/test_documents.py index e55a6c6..d6858dc 100644 --- a/tests/api/test_documents.py +++ b/tests/api/test_documents.py @@ -46,7 +46,7 @@ def test_unknown_and_missing_documents( digest = _document(client, tmp_path) (tmp_path / "docs" / 'quoted"name.txt').unlink() response = client.get(f"/documents/{digest}/file") - assert response.status_code == 404 + assert response.status_code == 410 assert response.json()["error"]["code"] == "document_missing" diff --git a/tests/api/test_search.py b/tests/api/test_search.py index 3ecfb94..063ddb4 100644 --- a/tests/api/test_search.py +++ b/tests/api/test_search.py @@ -60,8 +60,8 @@ def test_search_validates_fields_collections_and_pagination( for params, status, code in [ ({"q": " ", "collections": collection_id}, 400, "invalid_query"), ({"q": "a", "collections": "c0000000000"}, 404, "collection_not_found"), - ({"q": "a", "collections": collection_id, "fields": "bad"}, 400, - "invalid_query"), + ({"q": "a", "collections": collection_id, "fields": "bad"}, 422, + "invalid_field"), ({"q": "a", "collections": collection_id, "limit": 101}, 422, "validation_failed"), ]: diff --git a/tests/conftest.py b/tests/conftest.py index f4eb925..829d600 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1 +1,49 @@ -"""Shared pytest configuration.""" +"""Shared service fixtures for HTTP, contract and security tests.""" + +from collections.abc import Generator +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + +from recoll_modern.app import create_app +from recoll_modern.config import Settings +from recoll_modern.engine.fake import FakeEngine + + +@pytest.fixture +def app_client(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Generator[TestClient]: + monkeypatch.setenv("RECOLL_MODERN_ADMIN_USERNAME", "admin") + monkeypatch.setenv("RECOLL_MODERN_ADMIN_PASSWORD", "secret-password") + (tmp_path / "docs").mkdir() + settings = Settings.from_env({ + "RECOLL_MODERN_DOCUMENT_ROOT": str(tmp_path / "docs"), + "RECOLL_MODERN_STATE_ROOT": str(tmp_path / "state"), + "RECOLL_MODERN_RECOLL_CONFIG_DIR": str(tmp_path / "recoll"), + }) + with TestClient(create_app(settings, FakeEngine([])), base_url="https://testserver") as client: + yield client + + +@pytest.fixture +def prefixed_client(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Generator[TestClient]: + monkeypatch.setenv("RECOLL_MODERN_ADMIN_USERNAME", "admin") + monkeypatch.setenv("RECOLL_MODERN_ADMIN_PASSWORD", "secret-password") + (tmp_path / "docs").mkdir() + settings = Settings.from_env({ + "RECOLL_MODERN_DOCUMENT_ROOT": str(tmp_path / "docs"), + "RECOLL_MODERN_STATE_ROOT": str(tmp_path / "state"), + "RECOLL_MODERN_RECOLL_CONFIG_DIR": str(tmp_path / "recoll"), + "RECOLL_MODERN_ROOT_PATH": "/recoll", + }) + with TestClient(create_app(settings, FakeEngine([])), base_url="https://testserver") as client: + yield client + + +@pytest.fixture +def authenticated(app_client: TestClient) -> tuple[TestClient, str]: + response = app_client.post( + "/auth/login", json={"username": "admin", "password": "secret-password"} + ) + assert response.status_code == 200 + return app_client, response.json()["csrf_token"] diff --git a/tests/contract/test_http_contract.py b/tests/contract/test_http_contract.py new file mode 100644 index 0000000..cb44585 --- /dev/null +++ b/tests/contract/test_http_contract.py @@ -0,0 +1,87 @@ +"""Requirement-to-test matrix for spec 001. + +R1–R4: api/test_collections.py, storage/test_collection_repository.py +R5–R8: storage/test_documents.py, indexing/test_reconcile.py, integration/test_end_to_end.py +R9–R12: api/test_memberships.py, indexing/test_discovery.py, indexing/test_paths.py +R13–R14: helper/test_membership_helper.py, integration/test_read_only_corpus.py +R15–R18: api/test_jobs.py, indexing/test_job_handlers.py, indexing/test_worker.py +R19–R23: api/test_search.py, api/test_documents.py, domain/test_search.py +R24–R26: api/test_auth.py, domain/test_auth.py, test_unauthenticated_routes +R27–R30: api/test_health.py, api/test_files.py, api/test_root_path.py, + security/test_proxy_headers.py, security/test_paths.py, security/test_downloads.py +R31: unit/test_config.py, integration/test_container_smoke.py, integration/test_end_to_end.py +C1: integration/test_read_only_corpus.py, helper/test_membership_helper.py +C2–C3: storage/test_documents.py, integration/test_end_to_end.py +C4: api/test_search.py, test_scoped_search_error_codes +""" + +import hashlib +from pathlib import Path +from typing import cast + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from recoll_modern.engine.fake import FakeRecord +from recoll_modern.indexing.discovery import ObservedFile + + +def test_unauthenticated_routes(app_client: TestClient) -> None: + for path in ( + "/files", "/collections", "/jobs", "/search?q=book&collections=c0000000000", + f"/documents/{'a' * 64}/text", f"/documents/{'a' * 64}/file", + ): + response = app_client.get(path) + assert response.status_code == 401, path + assert response.json()["error"]["code"] == "unauthenticated", path + + +def test_scoped_search_error_codes(authenticated: tuple[TestClient, str]) -> None: + client, csrf = authenticated + collection_id = client.post( + "/collections", json={"name": "Books"}, headers={"X-CSRF-Token": csrf} + ).json()["id"] + invalid_field = client.get("/search", params={ + "q": "book", "collections": collection_id, "fields": "unknown", + }) + assert invalid_field.status_code == 422 + assert invalid_field.json()["error"]["code"] == "invalid_field" + invalid_query = client.get("/search", params={ + "q": "book\\evil", "collections": collection_id, + }) + assert invalid_query.status_code == 400 + assert invalid_query.json()["error"]["code"] == "invalid_query" + + +def test_same_directory_membership_and_same_name_identity( + authenticated: tuple[TestClient, str], tmp_path: Path +) -> None: + client, csrf = authenticated + headers = {"X-CSRF-Token": csrf} + first_id = client.post("/collections", json={"name": "First"}, headers=headers).json()["id"] + second_id = client.post("/collections", json={"name": "Second"}, headers=headers).json()["id"] + root = tmp_path / "docs" + (root / "a").mkdir() + (root / "b").mkdir() + sources = [ + ("a/same.txt", b"python first", first_id), + ("a/other.txt", b"python second", second_id), + ("b/same.txt", b"python third", second_id), + ] + app = cast(FastAPI, client.app) + engine_records = [] + for relative, content, collection_id in sources: + path = root / relative + path.write_bytes(content) + digest = hashlib.sha256(content).hexdigest() + app.state.documents.observe_files([ObservedFile(relative, digest, len(content))]) + app.state.memberships.add_direct(collection_id, digest) + engine_records.append(FakeRecord(str(path), relative, content.decode(), (collection_id,))) + app.state.engine.records = engine_records + first = client.get("/search", params={"q": "python", "collections": first_id}).json() + second = client.get("/search", params={"q": "python", "collections": second_id}).json() + assert [hit["path"] for hit in first["hits"]] == ["a/same.txt"] + assert {hit["path"] for hit in second["hits"]} == {"a/other.txt", "b/same.txt"} + assert first["hits"][0]["document_id"] != next( + hit["document_id"] for hit in second["hits"] if hit["path"] == "b/same.txt" + ) diff --git a/tests/contract/test_recoll_adapter_unit.py b/tests/contract/test_recoll_adapter_unit.py index 7f494f9..0685ccc 100644 --- a/tests/contract/test_recoll_adapter_unit.py +++ b/tests/contract/test_recoll_adapter_unit.py @@ -123,7 +123,9 @@ def test_extracted_text_uses_exact_path_udi( ) monkeypatch.setitem(sys.modules, "recoll", recoll_package) db = SimpleNamespace( - getDoc=lambda udi: SimpleNamespace(ipath="") if udi == "udi" else None, + getDoc=lambda udi: SimpleNamespace( + ipath="", getbinurl=lambda: path.encode() + ) if udi == "udi" else None, close=lambda: None, ) monkeypatch.setattr(RecollEngine, "_connect", lambda self: db) diff --git a/tests/contract/test_recoll_engine.py b/tests/contract/test_recoll_engine.py index 74b426d..9636e44 100644 --- a/tests/contract/test_recoll_engine.py +++ b/tests/contract/test_recoll_engine.py @@ -1,19 +1,92 @@ +import json import os +import subprocess +from dataclasses import asdict +from datetime import datetime from pathlib import Path +from typing import cast import pytest +from recoll_modern.engine.protocol import Engine, EngineHealth, EngineHit, EnginePage, SearchRequest from recoll_modern.engine.recoll import RecollEngine from .engine_contract import EngineContract +_CONTAINER_SCRIPT = """ +import json, sys +from dataclasses import asdict +from pathlib import Path +from recoll_modern.engine.protocol import SearchRequest +from recoll_modern.engine.recoll import RecollEngine +engine = RecollEngine(Path('/var/lib/recoll-modern/recoll-config')) +method, payload = sys.argv[1], json.loads(sys.argv[2]) +if method == 'search': + result = engine.search(SearchRequest(**payload)) +elif method == 'health': + result = engine.health() +elif method == 'extracted_text': + result = engine.extracted_text(payload['path']) +else: + raise ValueError(method) +print(json.dumps(asdict(result) if method != 'extracted_text' else result, default=str)) +""" + + +class ContainerRecollEngine: + def _call(self, method: str, payload: dict[str, object]) -> object: + result = subprocess.run( + ["docker", "compose", "exec", "-T", "app", "python", "-c", _CONTAINER_SCRIPT, + method, json.dumps(payload)], + capture_output=True, text=True, check=False, timeout=30, + ) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout) + + def search(self, request: SearchRequest) -> EnginePage: + value = cast(dict[str, object], self._call("search", asdict(request))) + hits = [] + for raw in cast(list[dict[str, object]], value["hits"]): + date = raw["date"] + raw["date"] = datetime.fromisoformat(date) if isinstance(date, str) else None + raw["collection_ids"] = tuple(cast(list[str], raw["collection_ids"])) + raw["snippets"] = tuple(cast(list[str], raw["snippets"])) + hits.append(EngineHit(**raw)) # type: ignore[arg-type] + return EnginePage( + tuple(hits), int(cast(int, value["total"])), bool(value["total_is_approximate"]) + ) + + def health(self) -> EngineHealth: + value = cast(dict[str, object], self._call("health", {})) + return EngineHealth(bool(value["available"]), cast(str | None, value["message"])) + + def extracted_text(self, path: str) -> str: + return cast(str, self._call("extracted_text", {"path": path})) + + def force_reindex(self, paths: tuple[str, ...]) -> None: + raise NotImplementedError + class TestRecollEngine(EngineContract): @pytest.fixture(autouse=True) def available_engine(self) -> None: config = os.environ.get("RECOLL_MODERN_TEST_RECOLL_CONFIG_DIR") if config is None: - pytest.skip("Recoll test container is not running") + if os.environ.get("RECOLL_MODERN_REQUIRE_DOCKER") != "1": + pytest.skip("Recoll test container is not running") + corpus_file = os.environ.get("RECOLL_MODERN_TEST_CORPUS_FILE") + if corpus_file is None: + pytest.fail("RECOLL_MODERN_TEST_CORPUS_FILE is required for container contract") + bootstrap = subprocess.run( + ["docker", "compose", "exec", "-T", "app", "recollindex", "-c", + "/var/lib/recoll-modern/recoll-config", "-i", "-Z", + f"/documents/{corpus_file}"], + capture_output=True, text=True, check=False, timeout=30, + ) + assert bootstrap.returncode == 0, bootstrap.stderr + self.engine = cast(Engine, ContainerRecollEngine()) + assert self.engine.health().available + return engine = RecollEngine(Path(config)) if not engine.health().available: pytest.skip("Recoll test container is not running") diff --git a/tests/integration/test_end_to_end.py b/tests/integration/test_end_to_end.py new file mode 100644 index 0000000..7b448ca --- /dev/null +++ b/tests/integration/test_end_to_end.py @@ -0,0 +1,120 @@ +"""Exercise the complete HTTP lifecycle with durable state and a deterministic engine.""" + +import time +from pathlib import Path +from typing import cast + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from recoll_modern.app import create_app +from recoll_modern.config import Settings +from recoll_modern.engine.fake import FakeEngine, FakeRecord +from recoll_modern.storage.database import Database +from recoll_modern.storage.jobs import JobRepository + + +def _wait(client: TestClient, job_id: str) -> dict[str, object]: + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + job = client.get(f"/recoll/jobs/{job_id}").json()["job"] + if job["state"] not in ("queued", "running"): + return cast(dict[str, object], job) + time.sleep(0.01) + raise AssertionError(f"job {job_id} did not finish") + + +def test_complete_collection_lifecycle(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + root = tmp_path / "docs" + (root / "a" / ".calibre").mkdir(parents=True) + (root / "b").mkdir() + (root / "a" / "shared.txt").write_text("python duplicate") + (root / "b" / "shared.txt").write_text("python duplicate") + (root / "a" / "other.txt").write_text("python separate") + (root / "a" / ".calibre" / "hidden.txt").write_text("python hidden") + monkeypatch.setenv("RECOLL_MODERN_ADMIN_USERNAME", "admin") + monkeypatch.setenv("RECOLL_MODERN_ADMIN_PASSWORD", "secret-password") + settings = Settings.from_env({ + "RECOLL_MODERN_DOCUMENT_ROOT": str(root), + "RECOLL_MODERN_STATE_ROOT": str(tmp_path / "state"), + "RECOLL_MODERN_RECOLL_CONFIG_DIR": str(tmp_path / "config"), + "RECOLL_MODERN_ROOT_PATH": "/recoll", + "RECOLL_MODERN_INLINE_TEXT_LIMIT": "12", + "RECOLL_MODERN_INLINE_TEXT_LIMIT_MAX": "12", + }) + engine = FakeEngine([]) + with TestClient(create_app(settings, engine), base_url="https://testserver") as client: + login = client.post("/recoll/auth/login", json={ + "username": "admin", "password": "secret-password", + }) + assert login.status_code == 200 + assert "Path=/recoll" in login.headers["set-cookie"] + headers = {"X-CSRF-Token": login.json()["csrf_token"]} + first = client.post("/recoll/collections", json={"name": "First"}, headers=headers) + second = client.post("/recoll/collections", json={"name": "Second"}, headers=headers) + a, b = first.json()["id"], second.json()["id"] + for collection_id, sources in ( + (a, [{"type": "directory", "path": "a", "mode": "persistent", + "excluded_directories": ["a/.calibre"]}]), + (b, [{"type": "directory", "path": "b", "mode": "snapshot"}]), + ): + response = client.post( + f"/recoll/collections/{collection_id}/memberships", + headers=headers, json={"sources": sources}, + ) + assert _wait(client, response.json()["job"]["id"])["state"] == "completed" + direct = client.post( + f"/recoll/collections/{a}/memberships", headers=headers, + json={"sources": [ + {"type": "file", "path": "a/other.txt"}, + {"type": "file", "path": "a/shared.txt"}, + ]}, + ) + assert _wait(client, direct.json()["job"]["id"])["state"] == "completed" + members_a = client.get(f"/recoll/collections/{a}/documents").json()["items"] + members_b = client.get(f"/recoll/collections/{b}/documents").json()["items"] + assert len(members_a) == 2 and len(members_b) == 1 + shared = next(item for item in members_a if item["path"] == "a/shared.txt") + assert shared["document_id"] == members_b[0]["document_id"] + assert all(item["path"] != "a/.calibre/hidden.txt" for item in members_a) + engine.records = [ + FakeRecord(str(root / "a" / "shared.txt"), "Shared", "python duplicate", (a, b)), + FakeRecord(str(root / "a" / "other.txt"), "Other", "python separate", (a,)), + ] + result = client.get("/recoll/search", params=[ + ("q", "python"), ("collections", a), ("collections", b), + ("fields", "text"), ("text_limit", "6"), + ]) + assert result.status_code == 200 + assert len(result.json()["hits"]) == 2 + assert result.json()["hits"][0]["text"] == "python" + assert client.get( + f"/recoll/documents/{shared['document_id']}/text" + ).json()["text"] == "python duplicate" + partial = client.get( + f"/recoll/documents/{shared['document_id']}/file", headers={"Range": "bytes=0-5"} + ) + assert partial.status_code == 206 and partial.content == b"python" + + (root / "a" / "shared.txt").unlink() + refresh = client.post("/recoll/jobs/reindex", headers=headers, json={}) + assert _wait(client, refresh.json()["job"]["id"])["state"] == "completed" + promoted = cast(FastAPI, client.app).state.documents.get(shared["document_id"]) + assert promoted.canonical_path == "b/shared.txt" + engine.records = [ + FakeRecord(str(root / "b" / "shared.txt"), "Shared", "python duplicate", (a, b)) + ] + assert client.get("/recoll/search", params={ + "q": "python", "collections": a, + }).json()["hits"][0]["document_id"] == shared["document_id"] + + jobs = JobRepository(Database(settings.database_path)) + interrupted = jobs.enqueue("reindex", {"version": 1, "paths": [], "collections": []}, False) + with Database(settings.database_path).transaction() as connection: + connection.execute("UPDATE jobs SET state = 'running' WHERE id = ?", (interrupted.id,)) + with TestClient(create_app(settings, engine), base_url="https://testserver") as client: + client.post("/recoll/auth/login", json={ + "username": "admin", "password": "secret-password", + }) + assert _wait(client, interrupted.id)["state"] == "completed" diff --git a/tests/integration/test_read_only_corpus.py b/tests/integration/test_read_only_corpus.py index df4fdc7..a996fce 100644 --- a/tests/integration/test_read_only_corpus.py +++ b/tests/integration/test_read_only_corpus.py @@ -58,3 +58,35 @@ def test_generated_corpus_indexes_and_searches( ) assert result.status_code == 200, result.text assert result.json()["hits"], result.json() + document_id = result.json()["hits"][0]["document_id"] + second = client.post( + "/collections", json={"name": f"Integration second {time.time_ns()}"}, + headers=headers, + ) + assert second.status_code == 201, second.text + second_id = second.json()["id"] + second_job = client.post( + f"/collections/{second_id}/memberships", headers=headers, + json={"sources": [{"type": "file", "path": corpus_file}]}, + ) + assert second_job.status_code == 202, second_job.text + second_job_id = second_job.json()["job"]["id"] + deadline = time.monotonic() + 30 + while time.monotonic() < deadline: + job = client.get(f"/jobs/{second_job_id}", headers=headers).json()["job"] + if job["state"] not in ("queued", "running"): + break + time.sleep(0.1) + assert job["state"] == "completed", job + combined = client.get( + "/search", params=[ + ("q", "zebraplatypus"), ("collections", collection_id), + ("collections", second_id), ("fields", "collections"), + ], headers=headers, + ) + assert combined.status_code == 200, combined.text + assert len(combined.json()["hits"]) == 1 + assert set(combined.json()["hits"][0]["collections"]) >= {collection_id, second_id} + extracted = client.get(f"/documents/{document_id}/text", headers=headers) + assert extracted.status_code == 200, extracted.text + assert "zebraplatypus" in extracted.json()["text"] diff --git a/tests/security/__init__.py b/tests/security/__init__.py new file mode 100644 index 0000000..85abf58 --- /dev/null +++ b/tests/security/__init__.py @@ -0,0 +1 @@ +"""Boundary and adversarial request checks.""" diff --git a/tests/security/test_downloads.py b/tests/security/test_downloads.py new file mode 100644 index 0000000..9ec1e6d --- /dev/null +++ b/tests/security/test_downloads.py @@ -0,0 +1,25 @@ +"""Missing documents use the spec's distinct gone response.""" + +from pathlib import Path +from typing import cast + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from recoll_modern.indexing.discovery import ObservedFile + + +def test_missing_document_is_gone( + authenticated: tuple[TestClient, str], tmp_path: Path +) -> None: + client, _ = authenticated + path = tmp_path / "docs" / "gone.txt" + path.write_text("gone") + digest = "f" * 64 + cast(FastAPI, client.app).state.documents.observe_files([ + ObservedFile("gone.txt", digest, 4) + ]) + path.unlink() + response = client.get(f"/documents/{digest}/file") + assert response.status_code == 410 + assert response.json()["error"]["code"] == "document_missing" diff --git a/tests/security/test_paths.py b/tests/security/test_paths.py new file mode 100644 index 0000000..bd432b6 --- /dev/null +++ b/tests/security/test_paths.py @@ -0,0 +1,29 @@ +"""Containment is rechecked when serving a document.""" + +from pathlib import Path +from typing import cast + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from recoll_modern.indexing.discovery import ObservedFile + + +def test_swapped_symlink_cannot_escape_document_root( + authenticated: tuple[TestClient, str], tmp_path: Path +) -> None: + client, _ = authenticated + root = tmp_path / "docs" + path = root / "file.txt" + path.write_text("safe") + digest = "a" * 64 + cast(FastAPI, client.app).state.documents.observe_files([ + ObservedFile("file.txt", digest, 4) + ]) + outside = tmp_path / "secret.txt" + outside.write_text("secret") + path.unlink() + path.symlink_to(outside) + response = client.get(f"/documents/{digest}/file") + assert response.status_code == 410 + assert b"secret" not in response.content diff --git a/tests/security/test_proxy_headers.py b/tests/security/test_proxy_headers.py new file mode 100644 index 0000000..13a30f5 --- /dev/null +++ b/tests/security/test_proxy_headers.py @@ -0,0 +1,12 @@ +"""Untrusted clients cannot replace the configured mount prefix.""" + +from fastapi.testclient import TestClient + + +def test_direct_client_cannot_forge_prefix(prefixed_client: TestClient) -> None: + response = prefixed_client.get("/evil/health", headers={ + "X-Forwarded-Prefix": "/evil", "X-Forwarded-Host": "attacker.example", + "X-Forwarded-Proto": "https", + }) + assert response.status_code == 404 + assert response.json()["error"]["code"] == "path_not_found"