diff --git a/CHANGELOG.md b/CHANGELOG.md index 19bdf4e..6a585c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING: `user` is now required on every tenant-data endpoint** - The implicit `jpmschweitzer` default tenant (`DEFAULT_USER`) has been removed everywhere. All endpoints that read or write tenant data (`/query/*`, `/wiki/*`, `/vector/*`, `/graph/*`, `/ingest/*`, `/volatile/*`, `/documents/*`, `/stats`, `/rag/search`) now reject requests without an explicit, non-empty, non-whitespace `user` (HTTP 422), matching the existing `/maintenance/*` pattern. A shared validator (`require_user` dependency / `RequiredUser` model type) also rejects blank users. The Wiki.js change listener now skips changes whose notification email yields no user instead of attributing them to the production tenant. **Caller coordination required:** tatlock and the Scheduler ingest/prefetch/consolidation tasks must send an explicit `user` on every call — see the deploy checklist. +### Fixed (security) + +- **`/query/graph` and `/graph/query` hardened to read-only** - The documented "automatic user scoping" was a no-op (a live probe confirmed any user string could read the whole graph) and the client permitted writes. Raw Cypher queries are now (1) rejected with 400 when they contain write clauses (`CREATE`/`MERGE`/`DELETE`/`DETACH`/`SET`/`REMOVE`/`DROP`/`FOREACH`/`LOAD CSV`) or any `CALL` procedure (conservative denylist on the uppercased query), and (2) executed through a Neo4j session opened with `default_access_mode=READ_ACCESS` so the database itself refuses writes as a backstop. The endpoints are now honestly documented as **admin/debug, unscoped read-only**: results are not restricted to the caller's tenant labels — use `/graph/nodes` for tenant-scoped access. + ### Added - **Degradation signaling** - `HybridRAGResponse` now includes `source_status` (per-leg `'ok'`/`'failed'`/`'disabled'` for vector, graph, web, volatile, documents) and `degraded` (true when any enabled leg failed). Retrieval legs report errors instead of silently swallowing them; failed legs are logged at WARNING. Both fields are additive and optional, so clients that ignore them are unaffected. diff --git a/src/clients/neo4j_client.py b/src/clients/neo4j_client.py index fe93545..c990148 100644 --- a/src/clients/neo4j_client.py +++ b/src/clients/neo4j_client.py @@ -8,7 +8,7 @@ Provides async Neo4j operations with: - Automatic retry on transient failures """ -from neo4j import AsyncGraphDatabase, AsyncDriver, AsyncSession +from neo4j import AsyncGraphDatabase, AsyncDriver, AsyncSession, READ_ACCESS from typing import Optional, List, Dict, Any import logging @@ -97,6 +97,38 @@ class Neo4jClient: records = await result.data() return records + async def execute_read( + self, + cypher: str, + parameters: Optional[Dict[str, Any]] = None + ) -> List[Dict[str, Any]]: + """ + Execute Cypher in a READ-ONLY session. + + The session is opened with default_access_mode=READ_ACCESS, so the + database rejects any write attempt (CREATE/MERGE/DELETE/SET/...) + even if it slips past caller-side validation. Use this for any + query built from untrusted input (e.g. the /query/graph endpoint). + + Args: + cypher: Cypher query string + parameters: Query parameters + + Returns: + List of result records as dictionaries + + Raises: + Exception: If driver not initialized, query fails, or the + query attempts a write (rejected by the read session) + """ + if not self._driver: + await self.connect() + + async with self._driver.session(default_access_mode=READ_ACCESS) as session: + result = await session.run(cypher, parameters or {}) + records = await result.data() + return records + async def execute_write( self, cypher: str, diff --git a/src/main.py b/src/main.py index beeb653..2ab547e 100644 --- a/src/main.py +++ b/src/main.py @@ -338,17 +338,23 @@ async def graph_query( api_key: str = Depends(verify_api_key) ): """ - Execute a Cypher query against the Neo4j knowledge graph. + Execute a raw Cypher query against the Neo4j knowledge graph + (ADMIN/DEBUG — read-only, NOT tenant-scoped). - Queries are automatically scoped to the user's data for security. - Use this for custom graph traversals beyond what /graph/nodes provides. + **Security model:** + - Queries containing write clauses (CREATE/MERGE/DELETE/SET/REMOVE/DROP/ + DETACH/FOREACH/LOAD CSV) or any CALL are rejected with 400. + - Execution happens in a read-only Neo4j session, so writes are refused + by the database even if validation is bypassed. + - Results are NOT automatically restricted to the requesting user's + tenant: an arbitrary query can read any tenant's nodes. Scope your + own patterns (e.g. `MATCH (d:User__Document:Document) ...`). + For tenant-scoped access use /graph/nodes instead. **Example:** ``` - POST /query/graph?query=MATCH%20(d:Document)-[:MENTIONS]->(p:Person)%20RETURN%20d,p&user=jpmschweitzer + POST /query/graph?query=MATCH%20(d:Document)-[:MENTIONS]->(p:Person)%20RETURN%20d,p&user= ``` - - **Security:** All queries are user-scoped to prevent cross-user data access. """ from src.services.graph_service import GraphService diff --git a/src/routers/graph.py b/src/routers/graph.py index 5629108..08a4d7e 100644 --- a/src/routers/graph.py +++ b/src/routers/graph.py @@ -40,21 +40,24 @@ async def execute_cypher_query( api_key: str = Depends(verify_api_key) ): """ - Execute a user-scoped Cypher query. + Execute a raw Cypher query (ADMIN/DEBUG — read-only, NOT tenant-scoped). - The query is automatically scoped to the user's data for security. - This prevents users from accessing other users' graph data. + **Security model:** + - Write clauses (CREATE/MERGE/DELETE/SET/REMOVE/DROP/DETACH/FOREACH/ + LOAD CSV) and CALL procedures are rejected with 400. + - Execution happens in a read-only Neo4j session as a hard backstop. + - Results are NOT restricted to the requesting user's tenant labels — + scope your own patterns (e.g. match `User__Document`). + For tenant-scoped access use /graph/nodes instead. **Example Request:** ```json { - "query": "MATCH (d:Document) RETURN d LIMIT 10", + "query": "MATCH (d:User_Llm_Tester_Document:Document) RETURN d LIMIT 10", "parameters": {}, "user": "" } ``` - - **Security:** Query is automatically scoped with user label. """ try: return await graph_service.execute_query( diff --git a/src/services/graph_service.py b/src/services/graph_service.py index e0c1087..e383cec 100644 --- a/src/services/graph_service.py +++ b/src/services/graph_service.py @@ -73,6 +73,15 @@ class GraphService: self.neo4j = neo4j_client self.wiki = wikijs_client + # Conservative denylist of Cypher write clauses / procedure calls. + # Matched as whole words against the uppercased query. CALL is rejected + # entirely (covers db.*/apoc.* write procedures and CALL {} subqueries) + # because reliably distinguishing read from write procedures would + # require a real Cypher parser. + _WRITE_CLAUSE_PATTERN = re.compile( + r"\b(CREATE|MERGE|DELETE|DETACH|SET|REMOVE|DROP|FOREACH|LOAD|CALL)\b" + ) + async def execute_query( self, query: str, @@ -80,29 +89,39 @@ class GraphService: user: str ) -> CypherQueryResponse: """ - Execute user-scoped Cypher query. + Execute a raw Cypher query — READ-ONLY, NOT tenant-scoped. - Automatically injects user label into query for security. + Security model (admin/debug endpoint): + - Queries containing write clauses (CREATE/MERGE/DELETE/SET/REMOVE/ + DROP/DETACH/FOREACH/LOAD CSV) or any CALL are rejected up front. + - The query is executed through a session opened with + default_access_mode=READ_ACCESS, so the database itself refuses + writes even if the denylist is bypassed. + - Results are NOT automatically restricted to the user's tenant + labels: an arbitrary Cypher query can read any tenant's nodes. + Callers must scope patterns themselves (e.g. match on + `User_`/`User__Document` labels). Args: - query: Cypher query + query: Cypher query (read-only) parameters: Query parameters - user: User identifier + user: Requesting user (audit logging only — does NOT scope + the query) Returns: Query results with metadata + + Raises: + ValueError: If the query contains write clauses or fails """ start_time = time.time() - # Get user-specific label - user_label = get_neo4j_user_label(user) + self._reject_write_clauses(query) - # Inject user label into query for scoping - # This ensures users can only query their own data - scoped_query = self._scope_query_to_user(query, user_label) + logger.info(f"Read-only Cypher query for user '{user}' (unscoped): {query[:200]}") try: - results = await self.neo4j.execute_query(scoped_query, parameters) + results = await self.neo4j.execute_read(query, parameters) query_time_ms = (time.time() - start_time) * 1000 return CypherQueryResponse( @@ -111,28 +130,33 @@ class GraphService: query_time_ms=query_time_ms ) + except ValueError: + raise except Exception as e: logger.error(f"Cypher query failed: {e}", exc_info=True) raise ValueError(f"Query execution failed: {str(e)}") - def _scope_query_to_user(self, query: str, user_label: str) -> str: + def _reject_write_clauses(self, query: str) -> None: """ - Inject user label into Cypher query for multi-tenancy. + Reject Cypher queries containing write clauses or procedure calls. - Simple implementation: adds user label to node patterns. - Production version would use proper query parsing. + Conservative denylist on the uppercased query: false positives are + acceptable (e.g. the word SET in a string literal), false negatives + are not. The read-only session is the hard backstop. Args: - query: Original Cypher query - user_label: User-specific label + query: Raw Cypher query - Returns: - Scoped query + Raises: + ValueError: If a denylisted clause is found """ - # For now, return query as-is - # TODO: Implement proper query scoping with label injection - logger.warning("Query scoping not yet implemented - returning unscoped query") - return query + match = self._WRITE_CLAUSE_PATTERN.search(query.upper()) + if match: + raise ValueError( + f"Query rejected: '{match.group(1)}' is not allowed — " + "/query/graph and /graph/query are read-only (no write " + "clauses or CALL procedures)" + ) async def list_nodes( self, diff --git a/tests/test_graph_query_hardening.py b/tests/test_graph_query_hardening.py new file mode 100644 index 0000000..c39f7e6 --- /dev/null +++ b/tests/test_graph_query_hardening.py @@ -0,0 +1,112 @@ +""" +Offline unit tests for /query/graph hardening. + +The raw Cypher endpoint must: +- reject queries containing write clauses or CALL procedures (denylist), +- execute allowed queries through the READ-ONLY client path + (Neo4jClient.execute_read), never the writable execute_query path. +""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from src.services.graph_service import GraphService + + +@pytest.fixture +def mock_neo4j(): + neo4j = MagicMock() + neo4j.execute_read = AsyncMock(return_value=[{"n": {"name": "x"}}]) + neo4j.execute_query = AsyncMock(return_value=[{"n": {"name": "x"}}]) + return neo4j + + +@pytest.fixture +def service(mock_neo4j): + return GraphService(neo4j_client=mock_neo4j, wikijs_client=MagicMock()) + + +WRITE_QUERIES = [ + "CREATE (n:Evil) RETURN n", + "MATCH (n) DELETE n", + "MATCH (n) DETACH DELETE n", + "MERGE (n:Evil {name: 'x'}) RETURN n", + "MATCH (n) SET n.pwned = true RETURN n", + "MATCH (n) REMOVE n:Document RETURN n", + "DROP INDEX my_index", + "FOREACH (x IN [1] | CREATE (:Evil))", + "LOAD CSV FROM 'file:///etc/passwd' AS row RETURN row", + "CALL db.labels()", + "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'DELETE n', {})", + "call dbms.components()", + "match (n) detach delete n", # lowercase + "MATCH (n)\nSET n.x = 1", # multiline +] + + +@pytest.mark.unit +class TestWriteClauseDenylist: + """Write clauses and procedure calls must be rejected before execution.""" + + @pytest.mark.parametrize("query", WRITE_QUERIES) + async def test_write_query_rejected(self, service, mock_neo4j, query): + with pytest.raises(ValueError, match="read-only"): + await service.execute_query(query, {}, user="llm_tester") + + mock_neo4j.execute_read.assert_not_awaited() + mock_neo4j.execute_query.assert_not_awaited() + + async def test_read_query_allowed(self, service): + response = await service.execute_query( + "MATCH (n:Document) RETURN n LIMIT 5", {}, user="llm_tester" + ) + assert response.count == 1 + + async def test_word_boundary_no_false_positive(self, service): + """Words merely containing denylisted substrings must pass.""" + response = await service.execute_query( + "MATCH (n:Document) WHERE n.title = 'dataSET dropped' RETURN n", + {}, + user="llm_tester", + ) + assert response.count == 1 + + +@pytest.mark.unit +class TestReadOnlyExecution: + """Allowed queries must run through the read-only session path.""" + + async def test_uses_execute_read_not_execute_query(self, service, mock_neo4j): + await service.execute_query( + "MATCH (n) RETURN n LIMIT 1", {"p": 1}, user="llm_tester" + ) + + mock_neo4j.execute_read.assert_awaited_once_with( + "MATCH (n) RETURN n LIMIT 1", {"p": 1} + ) + mock_neo4j.execute_query.assert_not_awaited() + + async def test_neo4j_client_read_session_access_mode(self): + """Neo4jClient.execute_read must open the session with READ_ACCESS.""" + from neo4j import READ_ACCESS + from src.clients.neo4j_client import Neo4jClient + + client = Neo4jClient(uri="bolt://unused:7687", user="u", password="p") + + session = MagicMock() + run_result = MagicMock() + run_result.data = AsyncMock(return_value=[{"ok": 1}]) + session.run = AsyncMock(return_value=run_result) + session_cm = MagicMock() + session_cm.__aenter__ = AsyncMock(return_value=session) + session_cm.__aexit__ = AsyncMock(return_value=False) + + driver = MagicMock() + driver.session = MagicMock(return_value=session_cm) + client._driver = driver + + records = await client.execute_read("RETURN 1 AS ok") + + assert records == [{"ok": 1}] + driver.session.assert_called_once_with(default_access_mode=READ_ACCESS)