fix(graph): don't reject read-only Cypher that names properties like write keywords

packages/ai/src/ai/common/graph/cypher_safety.py

@@ -39,12 +39,40 @@
 
 import re
 
+# The leading ``(?<![\w.])`` refuses a match when the keyword is preceded by a
+# word character or a dot, so a property or map read like ``n.delete`` or
+# ``n.create`` is not mistaken for the DELETE / CREATE clause. Bare reserved
+# words still match, which is correct: Cypher requires such identifiers to be
+# back-quoted, and back-quoted spans are stripped before this runs.
 _UNSAFE_CYPHER = re.compile(
-    r'\b(?:CREATE|MERGE|DELETE|DETACH\s+DELETE|SET|REMOVE|DROP|FOREACH|LOAD\s+CSV|'
+    r'(?<![\w.])(?:CREATE|MERGE|DELETE|DETACH\s+DELETE|SET|REMOVE|DROP|FOREACH|LOAD\s+CSV|'
     r'CALL\s+apoc\.(?:create|merge|delete|periodic\.commit|refactor|load))\b',
     re.IGNORECASE,
 )
 
+# Comments and string / back-quoted-identifier literals, matched in one pass.
+# Whichever span opens first at a given position wins, so a ``//`` inside a
+# string is consumed as string content and a quote inside a comment is consumed
+# as comment content. That single left-to-right scan is what closes the
+# cross-boundary bypass a chain of independent strippers allows (e.g. an
+# unbalanced quote in a comment swallowing a trailing write keyword). An
+# unterminated string or block comment does not match, so its keywords survive
+# and the statement is rejected, which is the safe direction.
+#
+# A line comment ends at a carriage return as well as a line feed. Stopping only
+# at ``\n`` would let ``// ...\rDELETE`` be read as one long comment here while
+# Cypher ends the comment at the ``\r`` and runs the DELETE.
+_STRIP_LITERALS = re.compile(
+    r"""
+      //[^\r\n]*           # line comment
+    | /\*.*?\*/            # block comment
+    | '(?:\\.|[^'\\])*'    # single-quoted string
+    | "(?:\\.|[^"\\])*"    # double-quoted string
+    | `[^`]*`              # back-quoted identifier
+    """,
+    re.DOTALL | re.VERBOSE,
+)
+
 
 def is_cypher_safe(cypher: str) -> bool:
     """Return True when the Cypher statement is read-only (MATCH/RETURN/schema CALLs).
@@ -55,7 +83,8 @@ def is_cypher_safe(cypher: str) -> bool:
     Returns:
         bool: ``True`` if the statement contains no write or admin clauses.
     """
-    # Strip line and block comments so a commented-out DELETE can't hide a live one.
-    stripped = re.sub(r'//[^\n]*', '', cypher)
-    stripped = re.sub(r'/\*.*?\*/', '', stripped, flags=re.DOTALL)
+    # Remove comments and literals first so a keyword that is only data (a
+    # commented-out DELETE, or ``n.op = "CREATE"``) can neither trip the gate
+    # nor smuggle a live write past it.
+    stripped = _STRIP_LITERALS.sub('', cypher)
     return not bool(_UNSAFE_CYPHER.search(stripped))

packages/ai/tests/ai/common/graph/test_cypher_safety.py

@@ -0,0 +1,163 @@
+"""
+Unit tests for ai.common.graph.cypher_safety.is_cypher_safe.
+
+is_cypher_safe is the shared read-only gate for every graph node (called from
+graph_instance_base for FalkorDB today and any future driver on that base). Only
+read clauses (MATCH / RETURN / schema CALLs) are allowed; write and admin clauses
+(CREATE, MERGE, DELETE, SET, REMOVE, DROP, FOREACH, LOAD CSV, apoc write procs)
+must be rejected.
+
+Two failure modes get the most coverage here:
+  - false positives: a property or map key that happens to share a name with a
+    write keyword (n.delete), or a keyword that only appears as string / comment
+    data (n.op = "CREATE"), must NOT be rejected;
+  - bypasses: a comment or string must never hide a live write from the gate.
+"""
+
+import pytest
+
+from ai.common.graph.cypher_safety import is_cypher_safe
+
+
+# ---------------------------------------------------------------------------
+# Allowed read-only statements
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'MATCH (n) RETURN n',
+        'match (n) return n',
+        '   MATCH (n) RETURN count(n)   ',
+        'MATCH (n:Person) WHERE n.age > 30 RETURN n.name',
+        'MATCH (a)-[r]->(b) RETURN a, r, b',
+        'CALL db.labels()',
+        'CALL db.schema.visualization()',
+    ],
+)
+def test_allows_read_only(cypher):
+    """Read clauses and schema CALLs must be accepted."""
+    assert is_cypher_safe(cypher) is True
+
+
+@pytest.mark.parametrize('cypher', ['', '   ', '\n'])
+def test_empty_input_passes_vacuously(cypher):
+    """Empty / whitespace-only input has no write clause, so it is not rejected."""
+    assert is_cypher_safe(cypher) is True
+
+
+# ---------------------------------------------------------------------------
+# Property / identifier names that collide with write keywords (the #1375 bug)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'MATCH (n) WHERE n.delete = true RETURN n',
+        'MATCH (n) RETURN n.create',
+        'MATCH (n) RETURN n.create, n.set, n.remove, n.drop',
+        'MATCH (n) WHERE n.merge IS NOT NULL RETURN n',
+        'MATCH (n) RETURN n.foreach',
+    ],
+)
+def test_dotted_property_named_like_keyword_is_allowed(cypher):
+    """A ``.keyword`` property read is not the write clause and must be allowed."""
+    assert is_cypher_safe(cypher) is True
+
+
+# ---------------------------------------------------------------------------
+# Keywords that appear only as data (string literals / comments)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'MATCH (n) WHERE n.name = "CREATE" RETURN n',
+        "MATCH (n) WHERE n.note = 'please DELETE this later' RETURN n",
+        'MATCH (n) WHERE n.name = "DROP the beat" RETURN n',
+        '// DELETE this query someday\nMATCH (n) RETURN n',
+        '/* CREATE was here */ MATCH (n) RETURN n',
+        'MATCH (n) RETURN n  // remember to SET things up',
+    ],
+)
+def test_keyword_only_as_data_is_allowed(cypher):
+    """A write keyword inside a string or comment is data, not a clause."""
+    assert is_cypher_safe(cypher) is True
+
+
+# ---------------------------------------------------------------------------
+# Write / admin clauses must be rejected
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'CREATE (n:Person {name: "x"})',
+        'MERGE (n:Person {id: 1})',
+        'MATCH (n) DELETE n',
+        'MATCH (n) DETACH DELETE n',
+        'MATCH (n) SET n.age = 31',
+        'MATCH (n) REMOVE n:Person',
+        'DROP INDEX ON :Person(name)',
+        'MATCH (n) FOREACH (x IN n.items | SET x.seen = true)',
+        'LOAD CSV FROM "file:///data.csv" AS row CREATE (:Row)',
+        'CALL apoc.create.node(["Person"], {name: "x"})',
+        'CALL apoc.periodic.commit("MATCH (n) DELETE n", {})',
+        'match (n) delete n',
+    ],
+)
+def test_rejects_write_clauses(cypher):
+    """Any write or admin clause must be rejected regardless of case."""
+    assert is_cypher_safe(cypher) is False
+
+
+# ---------------------------------------------------------------------------
+# Comments / strings must not hide a live write (bypass guard)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'MATCH (n) /* harmless */ DELETE n',
+        '/* SELECT-ish */ MATCH (n) SET n.x = 1',
+        "MATCH (n {name: 'safe'}) DELETE n",
+        'MATCH (n) WHERE n.x = "CREATE" CREATE (m)',
+    ],
+)
+def test_comment_or_string_does_not_hide_live_write(cypher):
+    """Stripping a comment or string literal must not blind the gate to a real write."""
+    assert is_cypher_safe(cypher) is False
+
+
+def test_unbalanced_quote_in_comment_cannot_hide_write():
+    """
+    Regression for the chained-stripper bypass.
+
+    An unbalanced quote inside a line comment must not swallow a write keyword on
+    the following line. The single-pass strip consumes the comment only to the
+    newline, so the DELETE on the next line stays visible and is rejected.
+    """
+    cypher = "MATCH (n) // note: don't forget\nDELETE n"
+    assert is_cypher_safe(cypher) is False
+
+
+@pytest.mark.parametrize(
+    'cypher',
+    [
+        'MATCH (n) // hide me\rDELETE n',
+        'MATCH (n) //\rCREATE (m)',
+    ],
+)
+def test_carriage_return_ends_line_comment(cypher):
+    """
+    A line comment ends at a carriage return, not only a line feed.
+
+    Cypher stops the ``//`` comment at the ``\\r`` and executes what follows, so
+    the strip must too, otherwise a write hidden after a lone ``\\r`` slips past.
+    """
+    assert is_cypher_safe(cypher) is False