fix(security)!: make raw Cypher endpoints read-only with write-clause denylist
/query/graph scoping was a documented no-op (graph_service returned the query unscoped) and neo4j_client permitted writes; a live probe showed a nonexistent user could read the whole graph. - Add Neo4jClient.execute_read() that opens the session with default_access_mode=READ_ACCESS so the database refuses writes even if validation is bypassed. - GraphService.execute_query() now rejects queries containing CREATE/MERGE/DELETE/DETACH/SET/REMOVE/DROP/FOREACH/LOAD or any CALL (conservative word-boundary denylist on the uppercased query) and executes through the read-only session; the no-op _scope_query_to_user is removed. - Remove the false user-scoping claims from /query/graph (main.py) and /graph/query docs and the CypherQueryRequest model: the endpoints are documented as admin/debug, unscoped read-only (per-tenant label injection for arbitrary Cypher would need a real parser; /graph/nodes remains the tenant-scoped path). - Offline unit tests: denylist coverage (incl. lowercase/multiline/CALL), word-boundary false-positive check, and READ_ACCESS session assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
+12
-6
@@ -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_<Tenant>_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=<tenant>
|
||||
```
|
||||
|
||||
**Security:** All queries are user-scoped to prevent cross-user data access.
|
||||
"""
|
||||
from src.services.graph_service import GraphService
|
||||
|
||||
|
||||
@@ -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_<Tenant>_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": "<tenant>"
|
||||
}
|
||||
```
|
||||
|
||||
**Security:** Query is automatically scoped with user label.
|
||||
"""
|
||||
try:
|
||||
return await graph_service.execute_query(
|
||||
|
||||
@@ -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_<Tenant>`/`User_<Tenant>_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,
|
||||
|
||||
@@ -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)
|
||||
Reference in New Issue
Block a user