diff --git a/CHANGELOG.md b/CHANGELOG.md index b7dcc2cf..e6a409bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -244,6 +244,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **Cypher injection via unvalidated node labels, relationship types, and property keys** (#910, GHSA-482h-hw99-h62p) by @KaifAhmad1 + - Node labels and property keys passed to `create_node`/`create_relationship` were interpolated directly into Cypher strings in the Neptune, Neo4j, and FalkorDB graph stores. Property *values* are parameterized, but labels and keys can't be bound as query parameters, and nothing validated them — so a document-derived entity type or property name (the normal ingest path) could close the current Cypher token early and append arbitrary statements (e.g. `DETACH DELETE`), running with the application's database credentials + - New shared `semantica/graph_store/query_sanitize.py`: `sanitize_identifier()` generalizes `age_store.py`'s existing `_sanitize_label`/`_sanitize_rel_type` (the only backend that already validated this) into a helper the other backends import without an import cycle with `graph_store.py`/`methods.py` + - Applied at every label/relationship-type/property-key interpolation site in `amazon_neptune.py`, `neo4j_store.py`, `falkordb_store.py`, `graph_store.py` (`degree_centrality`'s own query builder), and `methods.py` (`update_relationship`'s own query builder) — covers `create_node`, `create_nodes`, `create_relationship`, `get_nodes`, `get_relationships`, `get_neighbors`, `shortest_path`, `update_node`, `create_index`, and all relationship-type filters across the three backends + - **Fixed along the way** (caught in review, by @Sameer6305): `depth`/`max_depth` path-length parameters are meant to be integers, but `Neo4jStore.get_neighbors()`/`shortest_path()` interpolated them into the Cypher variable-length-path syntax (`*1..{depth}`) without coercion — unlike the Neptune/FalkorDB equivalents, which already cast to `int()`. A string `depth` (e.g. `"1]->(x) DETACH DELETE x //"`) reached the query verbatim. Added the same `int()` coercion Neptune/FalkorDB already had, plus `GraphStore.get_neighbors()`'s `hops`/`depth` alias resolution + - New `tests/graph_store/test_cypher_injection.py` (unit tests on `sanitize_identifier` plus the labels/keys/rel-types injection payload run against Neptune/Neo4j/FalkorDB `create_node`/`create_relationship`, asserting the malicious query is never built or sent) and the depth-coercion regression above; plus additions to `tests/test_graph_store.py` (`degree_centrality`) and `tests/test_graph_store_methods.py` (`update_relationship`). Full graph_store suite: 224+ tests passing + - **4 critical/high vulnerabilities in the Explorer API and vector store: RCE, SSRF, XXE, and DoS, plus Cypher/SPARQL injection hardening found along the way** (#898) by @Sunil56224972 - **[CWE-502] Arbitrary code execution via `pickle.load()`**: `VectorStore.save()`/`load()` used `pickle` for the on-disk `store_data.pkl`; a crafted `.pkl` file placed in the store directory (file upload, shared filesystem, or supply-chain compromise) could execute arbitrary code on deserialization. Replaced with JSON — vectors and metadata are fully JSON-serializable, so nothing is lost — and `load()` now refuses any legacy `.pkl` file it finds with a migration error rather than deserializing it - **[CWE-918] SSRF via redirect bypass in `ontology.py`'s URL fetcher**: `_validate_fetch_url()` correctly blocked private/loopback/reserved addresses on the caller-supplied URL, but `_fetch_url_sync()` fetched with `allow_redirects=True`, so a validated *public* first hop could 302 to `http://169.254.169.254/...` (cloud instance metadata) or an internal service, and `requests` followed it with no re-check. Redirects are now followed manually, capped at 5 hops, with `_validate_fetch_url()` re-run against every hop's target — including relative `Location` headers, resolved via `urljoin()` before validation — and every response (redirect or final) is explicitly closed to avoid leaking connections back to the pool diff --git a/semantica/graph_store/amazon_neptune.py b/semantica/graph_store/amazon_neptune.py index 988fc470..43a4e8af 100644 --- a/semantica/graph_store/amazon_neptune.py +++ b/semantica/graph_store/amazon_neptune.py @@ -47,6 +47,7 @@ from typing import Any, Dict, List, Optional, Union from ..utils.exceptions import ProcessingError, ValidationError from ..utils.logging import get_logger from ..utils.progress_tracker import get_progress_tracker +from .query_sanitize import sanitize_identifier # Optional boto3 for AWS credentials and SigV4 signing try: @@ -981,7 +982,8 @@ class AmazonNeptuneStore: node_id = props_copy.pop("id", None) or self._generate_id() use_merge = options.get("merge", True) - label_str = ":".join(labels) if labels else "Node" + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) if labels else "Node" + safe_keys = [sanitize_identifier(k, "property key") for k in props_copy.keys()] # Build parameters params = {"node_id": str(node_id)} @@ -990,9 +992,7 @@ class AmazonNeptuneStore: if use_merge: # MERGE: Return existing node if ID matches, or create new - set_parts = [] - for key in props_copy.keys(): - set_parts.append(f"n.{key} = ${key}") + set_parts = [f"n.{key} = ${key}" for key in safe_keys] if set_parts: set_clause = ", ".join(set_parts) @@ -1005,9 +1005,7 @@ class AmazonNeptuneStore: query = f"MERGE (n:{label_str} {{`~id`: $node_id}}) RETURN n" else: # CREATE: Will fail if node with same ID exists - prop_parts = ["`~id`: $node_id"] - for key in props_copy.keys(): - prop_parts.append(f"{key}: ${key}") + prop_parts = ["`~id`: $node_id"] + [f"{key}: ${key}" for key in safe_keys] prop_assignments = ", ".join(prop_parts) query = f"CREATE (n:{label_str} {{{prop_assignments}}}) RETURN n" @@ -1162,7 +1160,7 @@ class AmazonNeptuneStore: # Build query if labels: - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) query = f"MATCH (n:{label_str})" else: query = "MATCH (n)" @@ -1172,8 +1170,9 @@ class AmazonNeptuneStore: if properties: conditions = [] for key, value in properties.items(): - param_key = f"prop_{key}" - conditions.append(f"n.{key} = ${param_key}") + safe_key = sanitize_identifier(key, "property key") + param_key = f"prop_{safe_key}" + conditions.append(f"n.{safe_key} = ${param_key}") params[param_key] = value query += " WHERE " + " AND ".join(conditions) @@ -1341,15 +1340,16 @@ class AmazonNeptuneStore: } # Build property assignments including ~id + safe_rel_type = sanitize_identifier(rel_type, "relationship type") prop_parts = ["`~id`: $rel_id"] for key, value in props_copy.items(): - prop_parts.append(f"{key}: ${key}") + prop_parts.append(f"{sanitize_identifier(key, 'property key')}: ${key}") params[key] = value prop_assignments = ", ".join(prop_parts) query = ( f"MATCH (a), (b) WHERE id(a) = $start_id AND id(b) = $end_id " - f"CREATE (a)-[r:{rel_type} {{{prop_assignments}}}]->(b) RETURN r" + f"CREATE (a)-[r:{safe_rel_type} {{{prop_assignments}}}]->(b) RETURN r" ) records = self._run_query(query, params) @@ -1405,7 +1405,7 @@ class AmazonNeptuneStore: try: self._ensure_connected() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" params = {} if node_id is not None: @@ -1564,7 +1564,8 @@ class AmazonNeptuneStore: try: self._ensure_connected() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" + depth = int(depth) if direction == "out": pattern = f"-[r{type_filter}*1..{depth}]->" @@ -1634,7 +1635,7 @@ class AmazonNeptuneStore: try: self._ensure_connected() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" # Neptune doesn't support named path patterns in shortestPath # Use iterative depth search instead diff --git a/semantica/graph_store/falkordb_store.py b/semantica/graph_store/falkordb_store.py index 6049d3b1..c2f3fef6 100644 --- a/semantica/graph_store/falkordb_store.py +++ b/semantica/graph_store/falkordb_store.py @@ -41,6 +41,7 @@ from typing import Any, Dict, List, Optional, Union from ..utils.exceptions import ProcessingError, ValidationError from ..utils.logging import get_logger from ..utils.progress_tracker import get_progress_tracker +from .query_sanitize import sanitize_identifier # Optional FalkorDB import try: @@ -330,11 +331,11 @@ class FalkorDBStore: try: graph = self._ensure_graph() - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) # Build property string for Cypher props_str = ", ".join( - f"{k}: ${k}" for k in properties.keys() + f"{sanitize_identifier(k, 'property key')}: ${k}" for k in properties.keys() ) query = f"CREATE (n:{label_str} {{{props_str}}}) RETURN id(n) as id, n" @@ -392,9 +393,9 @@ class FalkorDBStore: labels = node.get("labels", []) properties = node.get("properties", {}) - label_str = ":".join(labels) if labels else "Node" + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) if labels else "Node" props_str = ", ".join( - f"{k}: ${k}" for k in properties.keys() + f"{sanitize_identifier(k, 'property key')}: ${k}" for k in properties.keys() ) query = f"CREATE (n:{label_str} {{{props_str}}}) RETURN id(n) as id" @@ -447,7 +448,7 @@ class FalkorDBStore: # Build query if labels: - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) query = f"MATCH (n:{label_str})" else: query = "MATCH (n)" @@ -456,7 +457,8 @@ class FalkorDBStore: if properties: conditions = [] for key in properties.keys(): - conditions.append(f"n.{key} = ${key}") + safe_key = sanitize_identifier(key, "property key") + conditions.append(f"n.{safe_key} = ${safe_key}") query += " WHERE " + " AND ".join(conditions) query += f" RETURN id(n) as id, n, labels(n) as labels LIMIT {limit}" @@ -504,7 +506,8 @@ class FalkorDBStore: # Build SET clause set_parts = [] for key in properties.keys(): - set_parts.append(f"n.{key} = ${key}") + safe_key = sanitize_identifier(key, "property key") + set_parts.append(f"n.{safe_key} = ${safe_key}") if merge: query = f"MATCH (n) WHERE id(n) = $node_id SET {', '.join(set_parts)} RETURN id(n) as id, n, labels(n) as labels" @@ -592,9 +595,13 @@ class FalkorDBStore: graph = self._ensure_graph() properties = properties or {} + safe_rel_type = sanitize_identifier(rel_type, "relationship type") + # Build property string if properties: - props_str = ", ".join(f"{k}: ${k}" for k in properties.keys()) + props_str = ", ".join( + f"{sanitize_identifier(k, 'property key')}: ${k}" for k in properties.keys() + ) props_str = f" {{{props_str}}}" else: props_str = "" @@ -602,7 +609,7 @@ class FalkorDBStore: query = f""" MATCH (a), (b) WHERE id(a) = $start_id AND id(b) = $end_id - CREATE (a)-[r:{rel_type}{props_str}]->(b) + CREATE (a)-[r:{safe_rel_type}{props_str}]->(b) RETURN id(r) as id, type(r) as type """ @@ -656,7 +663,7 @@ class FalkorDBStore: """ try: graph = self._ensure_graph() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" if node_id is not None: if direction == "out": @@ -806,7 +813,8 @@ class FalkorDBStore: """ try: graph = self._ensure_graph() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" + depth = int(depth) if direction == "out": pattern = f"-[r{type_filter}*1..{depth}]->" @@ -860,7 +868,8 @@ class FalkorDBStore: """ try: graph = self._ensure_graph() - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" + max_depth = int(max_depth) query = f""" MATCH path = shortestPath((start)-[r{type_filter}*..{max_depth}]-(end)) @@ -929,11 +938,13 @@ class FalkorDBStore: """ try: graph = self._ensure_graph() + safe_label = sanitize_identifier(label, "label") + safe_property = sanitize_identifier(property_name, "property key") if index_type == "fulltext": - query = f"CALL db.idx.fulltext.createNodeIndex('{label}', '{property_name}')" + query = f"CALL db.idx.fulltext.createNodeIndex('{safe_label}', '{safe_property}')" else: - query = f"CREATE INDEX FOR (n:{label}) ON (n.{property_name})" + query = f"CREATE INDEX FOR (n:{safe_label}) ON (n.{safe_property})" graph.query(query) self.logger.info(f"Created {index_type} index on {label}.{property_name}") diff --git a/semantica/graph_store/graph_store.py b/semantica/graph_store/graph_store.py index 5b7e2637..a8bd73f9 100644 --- a/semantica/graph_store/graph_store.py +++ b/semantica/graph_store/graph_store.py @@ -38,6 +38,7 @@ from ..utils.exceptions import ValidationError from ..utils.logging import get_logger from ..utils.progress_tracker import get_progress_tracker from .config import graph_store_config +from .query_sanitize import sanitize_identifier class NodeManager: @@ -393,12 +394,12 @@ class GraphAnalytics: """ # Build query based on direction if labels: - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) match = f"MATCH (n:{label_str})" else: match = "MATCH (n)" - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" if direction == "out": query = f""" @@ -756,7 +757,7 @@ class GraphStore: **options: Additional options """ # Support 'hops' as alias for 'depth' for ContextRetriever compatibility - actual_depth = options.get("hops", depth) + actual_depth = int(options.get("hops", depth)) return self._manager.analytics.get_neighbors( node_id, rel_type, direction, actual_depth, **options ) diff --git a/semantica/graph_store/methods.py b/semantica/graph_store/methods.py index 03bc4e87..ba16d9a8 100644 --- a/semantica/graph_store/methods.py +++ b/semantica/graph_store/methods.py @@ -62,6 +62,7 @@ from typing import Any, Dict, List, Optional, Union from .config import graph_store_config from .graph_store import GraphAnalytics, GraphStore, NodeManager, QueryEngine, RelationshipManager +from .query_sanitize import sanitize_identifier from .registry import method_registry # Global store instance @@ -357,7 +358,8 @@ def update_relationship( # Default implementation - execute update query store = _get_store() - set_parts = ", ".join(f"r.{k} = ${k}" for k in properties.keys()) + safe_keys = [sanitize_identifier(k, "property key") for k in properties.keys()] + set_parts = ", ".join(f"r.{k} = ${k}" for k in safe_keys) query = f"MATCH ()-[r]->() WHERE id(r) = $rel_id SET {set_parts} RETURN id(r) as id, type(r) as type, r" params = {"rel_id": rel_id, **properties} result = store.execute_query(query, params) diff --git a/semantica/graph_store/neo4j_store.py b/semantica/graph_store/neo4j_store.py index da2a8c75..93270efe 100644 --- a/semantica/graph_store/neo4j_store.py +++ b/semantica/graph_store/neo4j_store.py @@ -38,6 +38,7 @@ from typing import Any, Dict, List, Optional, Union from ..utils.exceptions import ProcessingError, ValidationError from ..utils.logging import get_logger from ..utils.progress_tracker import get_progress_tracker +from .query_sanitize import sanitize_identifier # Optional Neo4j import try: @@ -366,7 +367,7 @@ class Neo4jStore: ) try: - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) query = f"CREATE (n:{label_str} $props) RETURN id(n) as id, n" with self.get_session() as session: @@ -424,7 +425,7 @@ class Neo4jStore: labels = node.get("labels", []) properties = node.get("properties", {}) - label_str = ":".join(labels) if labels else "Node" + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) if labels else "Node" query = f"CREATE (n:{label_str} $props) RETURN id(n) as id, n" result = session.run(query, {"props": properties}) @@ -505,7 +506,7 @@ class Neo4jStore: try: # Build query if labels: - label_str = ":".join(labels) + label_str = ":".join(sanitize_identifier(l, "label") for l in labels) query = f"MATCH (n:{label_str})" else: query = "MATCH (n)" @@ -514,7 +515,8 @@ class Neo4jStore: if properties: conditions = [] for key, value in properties.items(): - conditions.append(f"n.{key} = ${key}") + safe_key = sanitize_identifier(key, "property key") + conditions.append(f"n.{safe_key} = ${safe_key}") query += " WHERE " + " AND ".join(conditions) query += f" RETURN id(n) as id, n, labels(n) as labels LIMIT {limit}" @@ -635,10 +637,11 @@ class Neo4jStore: try: properties = properties or {} + safe_rel_type = sanitize_identifier(rel_type, "relationship type") query = f""" MATCH (a), (b) WHERE id(a) = $start_id AND id(b) = $end_id - CREATE (a)-[r:{rel_type} $props]->(b) + CREATE (a)-[r:{safe_rel_type} $props]->(b) RETURN id(r) as id, type(r) as type, r """ @@ -696,7 +699,7 @@ class Neo4jStore: List of matching relationships """ try: - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" if node_id is not None: if direction == "out": @@ -857,7 +860,8 @@ class Neo4jStore: List of neighboring nodes with path information """ try: - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" + depth = int(depth) if direction == "out": pattern = f"-[r{type_filter}*1..{depth}]->" @@ -910,7 +914,8 @@ class Neo4jStore: Shortest path information or None if not found """ try: - type_filter = f":{rel_type}" if rel_type else "" + type_filter = f":{sanitize_identifier(rel_type, 'relationship type')}" if rel_type else "" + max_depth = int(max_depth) query = f""" MATCH path = shortestPath((start)-[r{type_filter}*..{max_depth}]-(end)) @@ -975,17 +980,22 @@ class Neo4jStore: True if index created successfully """ try: - index_name = options.get("index_name", f"idx_{label}_{property_name}") + safe_label = sanitize_identifier(label, "label") + safe_property = sanitize_identifier(property_name, "property key") + index_name = sanitize_identifier( + options.get("index_name", f"idx_{safe_label}_{safe_property}"), + "index name", + ) if index_type == "fulltext": query = f""" CREATE FULLTEXT INDEX {index_name} IF NOT EXISTS - FOR (n:{label}) ON EACH [n.{property_name}] + FOR (n:{safe_label}) ON EACH [n.{safe_property}] """ else: query = f""" CREATE INDEX {index_name} IF NOT EXISTS - FOR (n:{label}) ON (n.{property_name}) + FOR (n:{safe_label}) ON (n.{safe_property}) """ with self.get_session() as session: diff --git a/semantica/graph_store/query_sanitize.py b/semantica/graph_store/query_sanitize.py new file mode 100644 index 00000000..008ad098 --- /dev/null +++ b/semantica/graph_store/query_sanitize.py @@ -0,0 +1,32 @@ +""" +Shared identifier validation for Cypher/SPARQL query builders. + +Node labels, relationship types, and property keys can't be bound as query +parameters the way values can, so any such identifier that reaches a query +string unvalidated is a direct injection point (GHSA-482h-hw99-h62p). +`age_store.py` already validates its labels/relationship types this way; +this module generalizes that pattern for reuse across the other graph +store backends without introducing an import cycle with `graph_store.py` +or `methods.py`. +""" + +import re + +from ..utils.exceptions import ValidationError + +_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def sanitize_identifier(name: str, kind: str = "identifier") -> str: + """Validate a Cypher/SPARQL label, relationship type, or property key. + + Only alphanumeric/underscore identifiers starting with a letter or + underscore are allowed. + """ + if not isinstance(name, str) or not _IDENTIFIER_RE.match(name): + raise ValidationError( + f"Invalid {kind}: {name!r}. Must start with a letter or " + "underscore and contain only alphanumeric characters and " + "underscores." + ) + return name diff --git a/tests/graph_store/test_cypher_injection.py b/tests/graph_store/test_cypher_injection.py new file mode 100644 index 00000000..d9b5fa23 --- /dev/null +++ b/tests/graph_store/test_cypher_injection.py @@ -0,0 +1,396 @@ +"""Regression tests for GHSA-482h-hw99-h62p: unvalidated node labels and +property keys allowed arbitrary Cypher injection in the Neptune, Neo4j, and +FalkorDB graph stores (labels/keys can't be bound as query parameters, so +an unvalidated value reaching the query string is a direct injection +point). + +Mirrors the advisory's own PoC shape: a label/key crafted to close the +current Cypher token early and append a destructive statement +(`DETACH DELETE victim`). Before the fix, these reached `_run_query` / +`session.run` / `graph.query` verbatim. After the fix, `sanitize_identifier` +(graph_store/query_sanitize.py) rejects them with ValidationError before +any query is built, matching the existing age_store.py `_sanitize_label` +behavior used as the reference implementation. +""" + +import pytest +import unittest +from unittest.mock import MagicMock + +from semantica.graph_store.query_sanitize import sanitize_identifier +from semantica.utils.exceptions import ProcessingError, ValidationError + +# AmazonNeptuneStore/Neo4jStore/FalkorDBStore.create_node() wrap their whole +# body in `except Exception: raise ProcessingError(...)` (pre-existing, +# unrelated to this fix), so the ValidationError sanitize_identifier raises +# surfaces to callers as ProcessingError. Either way the malicious query is +# never built or sent — these tests assert exactly that via the query-capture +# stubs, and check the wrapped message to confirm it's the sanitizer firing. + +EVIL_LABEL = "N}) MATCH (victim) DETACH DELETE victim //" +EVIL_KEY = "k1`: 1}) MATCH (victim) DETACH DELETE victim //" + + +def _wire(store): + store.logger = MagicMock() + store.progress_tracker = MagicMock() + store.progress_tracker.start_tracking.return_value = "tid" + store.config = {} + return store + + +class TestSanitizeIdentifier(unittest.TestCase): + def test_valid_identifiers_pass_through_unchanged(self): + self.assertEqual(sanitize_identifier("Person"), "Person") + self.assertEqual(sanitize_identifier("_hidden"), "_hidden") + self.assertEqual(sanitize_identifier("Rel_Type2"), "Rel_Type2") + + def test_injection_payload_is_rejected(self): + with self.assertRaises(ValidationError): + sanitize_identifier(EVIL_LABEL) + + def test_property_key_injection_payload_is_rejected(self): + with self.assertRaises(ValidationError): + sanitize_identifier(EVIL_KEY) + + def test_rejects_non_string(self): + with self.assertRaises(ValidationError): + sanitize_identifier(123) # type: ignore[arg-type] + + def test_rejects_spaces_and_dashes(self): + with self.assertRaises(ValidationError): + sanitize_identifier("no spaces") + with self.assertRaises(ValidationError): + sanitize_identifier("no-dashes") + + +class TestAmazonNeptuneCypherInjection(unittest.TestCase): + def _make_store(self): + from semantica.graph_store.amazon_neptune import AmazonNeptuneStore + + store = _wire(AmazonNeptuneStore.__new__(AmazonNeptuneStore)) + store._connected = True + store._ensure_connected = lambda: None + store._generate_id = lambda: "generated-id" + store._run_query = MagicMock(return_value=[]) + store._parse_results = lambda r: [] + return store + + def test_create_node_rejects_malicious_label_before_querying(self): + store = self._make_store() + with self.assertRaises(ProcessingError) as ctx: + store.create_node(labels=[EVIL_LABEL], properties={"name": "x"}) + self.assertIn("Invalid label", str(ctx.exception)) + store._run_query.assert_not_called() + + def test_create_node_rejects_malicious_property_key_before_querying(self): + store = self._make_store() + with self.assertRaises(ProcessingError) as ctx: + store.create_node(labels=["Person"], properties={"name": "x", EVIL_KEY: 1}) + self.assertIn("Invalid property key", str(ctx.exception)) + store._run_query.assert_not_called() + + def test_create_node_with_legitimate_labels_still_works(self): + store = self._make_store() + store.create_node(labels=["Person", "Employee"], properties={"name": "Alice"}) + query = store._run_query.call_args[0][0] + self.assertIn("Person:Employee", query) + self.assertNotIn("DETACH DELETE", query) + + +class TestNeo4jCypherInjection(unittest.TestCase): + def _make_store(self): + from semantica.graph_store import neo4j_store as m + + store = _wire(m.Neo4jStore.__new__(m.Neo4jStore)) + captured = {} + + class Session: + def run(self, q, params=None): + captured["query"] = q + rec = {"n": {"name": "x"}, "id": 1} + return type("R", (), {"single": lambda self: rec})() + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + store.get_session = lambda: Session() + store._captured = captured + return store + + def test_create_node_rejects_malicious_label_before_querying(self): + store = self._make_store() + with self.assertRaises(ProcessingError) as ctx: + store.create_node(labels=["Person", EVIL_LABEL], properties={"name": "x"}) + self.assertIn("Invalid label", str(ctx.exception)) + self.assertNotIn("query", store._captured) + + def test_create_relationship_rejects_malicious_rel_type(self): + store = self._make_store() + with self.assertRaises(ProcessingError) as ctx: + store.create_relationship(start_node_id=1, end_node_id=2, rel_type=EVIL_LABEL) + self.assertIn("Invalid relationship type", str(ctx.exception)) + self.assertNotIn("query", store._captured) + + +class TestFalkorDBCypherInjection(unittest.TestCase): + def _make_store(self): + from semantica.graph_store import falkordb_store as m + + store = _wire(m.FalkorDBStore.__new__(m.FalkorDBStore)) + captured = {} + + class Graph: + def query(self, q, params=None): + captured["query"] = q + return type("R", (), {"result_set": []})() + + store._ensure_graph = lambda: Graph() + store._captured = captured + return store + + def test_create_node_rejects_malicious_label_and_key(self): + store = self._make_store() + with self.assertRaises(ProcessingError) as ctx: + store.create_node(labels=[EVIL_LABEL], properties={"name": "x", EVIL_KEY: 1}) + self.assertIn("Invalid label", str(ctx.exception)) + self.assertNotIn("query", store._captured) + + def test_create_node_with_legitimate_input_still_works(self): + store = self._make_store() + store.create_node(labels=["Person"], properties={"name": "Alice"}) + query = store._captured["query"] + self.assertIn("Person", query) + self.assertNotIn("DETACH DELETE", query) + + +# --------------------------------------------------------------------------- +# BLOCKER 1 regression: Neo4jStore.get_neighbors / shortest_path depth coercion +# --------------------------------------------------------------------------- +# Payload representative of the confirmed injection (review BLOCKER 1): +# supplying a string as `depth` / `max_depth` previously reached the Cypher +# f-string verbatim because Neo4jStore did not call int() like Neptune/FalkorDB. +# +# After the fix `depth = int(depth)` / `max_depth = int(max_depth)` are added +# at the top of each method's try-block. A malicious string raises ValueError +# (wrapped in ProcessingError), and the session.run / _run_query mock must +# never be called. + +EVIL_DEPTH = "1]->(x) DETACH DELETE x //" + + +class TestNeo4jDepthInjection(unittest.TestCase): + """Regression tests for BLOCKER 1: Neo4jStore depth/max_depth coercion.""" + + def _make_store(self): + """Build a Neo4jStore with a session that records every query string sent to it.""" + from semantica.graph_store import neo4j_store as m + + store = _wire(m.Neo4jStore.__new__(m.Neo4jStore)) + captured = {} + + class IterSession: + """Returns an empty iterator so get_neighbors' for-loop completes cleanly.""" + def run(self, q, params=None): + captured["query"] = q + return iter([]) + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + class SingleSession: + """Returns a FakeResult whose .single() yields None (shortest_path).""" + def run(self, q, params=None): + captured["query"] = q + return type("R", (), {"single": lambda self: None})() + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + store._captured = captured + store._iter_session = IterSession + store._single_session = SingleSession + return store + + # -- get_neighbors -------------------------------------------------------- + + def test_get_neighbors_malicious_depth_raises_before_query(self): + """Malicious string depth must never reach session.run.""" + store = self._make_store() + store.get_session = lambda: store._iter_session() + with self.assertRaises(ProcessingError): + store.get_neighbors(node_id=1, depth=EVIL_DEPTH) + self.assertNotIn("query", store._captured, + "session.run was called — injected query reached the database layer") + + def test_get_neighbors_malicious_depth_does_not_contain_payload(self): + """Double-check: if somehow a query were built, it must not contain the payload.""" + store = self._make_store() + store.get_session = lambda: store._iter_session() + with pytest.raises(ProcessingError): + store.get_neighbors(node_id=1, depth=EVIL_DEPTH) + query = store._captured.get("query", "") + self.assertNotIn("DETACH DELETE", query, + f"Injection payload found in query: {query!r}") + + def test_get_neighbors_legitimate_depth_works(self): + """Valid integer depth must still produce a correct traversal pattern.""" + store = self._make_store() + store.get_session = lambda: store._iter_session() + result = store.get_neighbors(node_id=1, depth=2) + self.assertIsInstance(result, list) + self.assertIn("query", store._captured) + self.assertIn("*1..2", store._captured["query"]) + self.assertNotIn("DETACH DELETE", store._captured["query"]) + + def test_get_neighbors_depth_string_int_is_coerced(self): + """A string representation of a valid integer must be coerced and work.""" + store = self._make_store() + store.get_session = lambda: store._iter_session() + result = store.get_neighbors(node_id=1, depth="3") + self.assertIsInstance(result, list) + self.assertIn("*1..3", store._captured["query"]) + + # -- shortest_path -------------------------------------------------------- + + def test_shortest_path_malicious_max_depth_raises_before_query(self): + """Malicious string max_depth must never reach session.run.""" + store = self._make_store() + store.get_session = lambda: store._single_session() + with self.assertRaises(ProcessingError): + store.shortest_path(start_node_id=1, end_node_id=2, max_depth=EVIL_DEPTH) + self.assertNotIn("query", store._captured, + "session.run was called — injected query reached the database layer") + + def test_shortest_path_malicious_max_depth_does_not_contain_payload(self): + store = self._make_store() + store.get_session = lambda: store._single_session() + with pytest.raises(ProcessingError): + store.shortest_path(start_node_id=1, end_node_id=2, max_depth=EVIL_DEPTH) + query = store._captured.get("query", "") + self.assertNotIn("DETACH DELETE", query, + f"Injection payload found in query: {query!r}") + + def test_shortest_path_legitimate_max_depth_works(self): + """Valid integer max_depth must produce a correct shortestPath pattern.""" + store = self._make_store() + store.get_session = lambda: store._single_session() + result = store.shortest_path(start_node_id=1, end_node_id=2, max_depth=5) + self.assertIsNone(result) # single() returns None → correct + self.assertIn("query", store._captured) + self.assertIn("*..5", store._captured["query"]) + self.assertNotIn("DETACH DELETE", store._captured["query"]) + + def test_shortest_path_max_depth_string_int_is_coerced(self): + store = self._make_store() + store.get_session = lambda: store._single_session() + store.shortest_path(start_node_id=1, end_node_id=2, max_depth="7") + self.assertIn("*..7", store._captured["query"]) + + +# --------------------------------------------------------------------------- +# BLOCKER 2 regression: GraphStore.get_neighbors hops forwarding +# --------------------------------------------------------------------------- +# Before the fix, GraphStore.get_neighbors() set: +# actual_depth = options.get("hops", depth) +# and forwarded the raw value to Neo4jStore.get_neighbors(depth=actual_depth). +# Because Neo4jStore did not coerce depth, an attacker-controlled hops string +# reached Cypher verbatim. +# +# The fix adds int() at the GraphStore facade: +# actual_depth = int(options.get("hops", depth)) +# This closes the path regardless of which backend is wired up. + +class TestGraphStoreHopsForwarding(unittest.TestCase): + """Regression tests for BLOCKER 2: GraphStore hops→depth forwarding.""" + + def _make_graph_store_with_neo4j(self): + """ + Wire a GraphStore whose backend is a Neo4j store stub that records every + query passed to session.run. Returns (graph_store, captured_dict). + """ + from semantica.graph_store import neo4j_store as m + from semantica.graph_store.graph_store import ( + GraphAnalytics, + GraphManager, + GraphStore, + ) + + captured = {} + + class IterSession: + def run(self, q, params=None): + captured["query"] = q + return iter([]) + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + neo4j_stub = _wire(m.Neo4jStore.__new__(m.Neo4jStore)) + neo4j_stub.get_session = lambda: IterSession() + + gs = GraphStore.__new__(GraphStore) + gs.logger = MagicMock() + gs.progress_tracker = MagicMock() + gs._store_backend = neo4j_stub + gs._manager = GraphManager(neo4j_stub) + + return gs, captured + + # -- hops injection ------------------------------------------------------- + + def test_hops_malicious_string_raises_before_query(self): + """Malicious hops string must be rejected before session.run is reached.""" + gs, captured = self._make_graph_store_with_neo4j() + with self.assertRaises(Exception): + gs.get_neighbors(node_id=1, hops=EVIL_DEPTH) + self.assertNotIn("query", captured, + "session.run was called — injected hops reached the database layer") + + def test_hops_malicious_string_payload_not_in_any_query(self): + """Belt-and-suspenders: payload text must not appear in any built query.""" + gs, captured = self._make_graph_store_with_neo4j() + with pytest.raises(ValueError): + gs.get_neighbors(node_id=1, hops=EVIL_DEPTH) + query = captured.get("query", "") + self.assertNotIn("DETACH DELETE", query, + f"Injection payload found in forwarded query: {query!r}") + + def test_hops_legitimate_integer_works(self): + """Valid integer hops value must produce a correct query.""" + gs, captured = self._make_graph_store_with_neo4j() + result = gs.get_neighbors(node_id=1, hops=2) + self.assertIsInstance(result, list) + self.assertIn("query", captured) + self.assertIn("*1..2", captured["query"]) + self.assertNotIn("DETACH DELETE", captured["query"]) + + def test_hops_string_int_is_coerced_and_works(self): + """String '3' forwarded as hops must be coerced to int and produce *1..3.""" + gs, captured = self._make_graph_store_with_neo4j() + result = gs.get_neighbors(node_id=1, hops="3") + self.assertIsInstance(result, list) + self.assertIn("*1..3", captured["query"]) + + def test_depth_param_still_works_without_hops(self): + """depth positional arg (no hops kwarg) must still be coerced and forwarded.""" + gs, captured = self._make_graph_store_with_neo4j() + result = gs.get_neighbors(node_id=1, depth=4) + self.assertIsInstance(result, list) + self.assertIn("*1..4", captured["query"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_graph_store.py b/tests/test_graph_store.py index 26e40581..0576f077 100644 --- a/tests/test_graph_store.py +++ b/tests/test_graph_store.py @@ -215,6 +215,26 @@ class TestGraphStore(unittest.TestCase): result = self.store.execute_query("MATCH (n) RETURN n") self.assertEqual(result["summary"], "Mock query executed") + def test_degree_centrality_rejects_malicious_label(self): + """Regression test for GHSA-482h-hw99-h62p: degree_centrality() + interpolates labels/rel_type directly into a Cypher MATCH clause + (graph_store.py's own query builder, not delegated to the backend), + so an unvalidated label was a direct injection point.""" + from semantica.utils.exceptions import ValidationError + evil_label = "N}) MATCH (victim) DETACH DELETE victim //" + with self.assertRaises(ValidationError): + self.store._manager.analytics.degree_centrality(labels=[evil_label]) + + def test_degree_centrality_rejects_malicious_rel_type(self): + from semantica.utils.exceptions import ValidationError + evil_rel_type = "R]-() DETACH DELETE n //" + with self.assertRaises(ValidationError): + self.store._manager.analytics.degree_centrality(rel_type=evil_rel_type) + + def test_degree_centrality_with_legitimate_input_still_works(self): + result = self.store._manager.analytics.degree_centrality(labels=["Person"]) + self.assertEqual(result, []) # MockGraphStore.execute_query returns no records + class TestGraphStoreInitialization(unittest.TestCase): def test_falkordb_initialization(self): with patch('semantica.graph_store.falkordb_store.FalkorDBStore', side_effect=MockGraphStore) as mock_falkor: diff --git a/tests/test_graph_store_methods.py b/tests/test_graph_store_methods.py index 23cadfe1..b288d680 100644 --- a/tests/test_graph_store_methods.py +++ b/tests/test_graph_store_methods.py @@ -57,5 +57,26 @@ class TestGraphStoreMethods(unittest.TestCase): # Verify self.mock_store.execute_query.assert_called_once_with(query, None) + def test_update_relationship_rejects_malicious_property_key(self): + """Regression test for GHSA-482h-hw99-h62p: update_relationship() + interpolates property keys directly into a Cypher SET clause + (methods.py's own query builder, not delegated to the backend + store), so an unvalidated key was a direct injection point.""" + from semantica.utils.exceptions import ValidationError + + evil_key = "x} MATCH (victim) DETACH DELETE victim //" + with self.assertRaises(ValidationError): + methods.update_relationship(1, {evil_key: "value"}) + self.mock_store.execute_query.assert_not_called() + + def test_update_relationship_with_legitimate_keys_still_works(self): + self.mock_store.execute_query.return_value = { + "records": [{"id": 1, "type": "KNOWS"}] + } + result = methods.update_relationship(1, {"weight": 0.5}) + query = self.mock_store.execute_query.call_args[0][0] + self.assertIn("r.weight = $weight", query) + self.assertNotIn("DETACH DELETE", query) + if __name__ == '__main__': unittest.main() \ No newline at end of file