From 86accf45ad9b8bd47d247dd6a0d6f44564dde5a7 Mon Sep 17 00:00:00 2001 From: Ansh-Karnwal <78387773+Ansh-Karnwal@users.noreply.github.com> Date: Sat, 18 Jul 2026 14:54:58 -0700 Subject: [PATCH 1/2] fix(graph): don't reject read-only Cypher that names properties like write keywords The shared is_cypher_safe gate rejected any read query whose property or map key shared a name with a write clause (n.delete, n.create), because the keyword regex used a plain \b word boundary. It also tripped on keywords that appeared only as string data (n.op = "CREATE"), since it stripped comments but never string literals. Replace the leading \b with (? bool: """Return True when the Cypher statement is read-only (MATCH/RETURN/schema CALLs). @@ -55,7 +79,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)) diff --git a/packages/ai/tests/ai/common/graph/__init__.py b/packages/ai/tests/ai/common/graph/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/packages/ai/tests/ai/common/graph/test_cypher_safety.py b/packages/ai/tests/ai/common/graph/test_cypher_safety.py new file mode 100644 index 000000000..5d04f4515 --- /dev/null +++ b/packages/ai/tests/ai/common/graph/test_cypher_safety.py @@ -0,0 +1,146 @@ +""" +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 From d5cabcd2d289906b16c424aeede0c4c38846be78 Mon Sep 17 00:00:00 2001 From: Ansh-Karnwal <78387773+Ansh-Karnwal@users.noreply.github.com> Date: Sat, 18 Jul 2026 15:06:10 -0700 Subject: [PATCH 2/2] fix(graph): end Cypher line comments at carriage return too The single-pass literal strip ended a // line comment only at \n. Cypher ends the comment at a carriage return as well, so `MATCH (n) // x\rDELETE n` was read as one long comment by the gate while Cypher runs the DELETE, letting a write slip through as read-only. Stop the line-comment span at \r or \n. Add regression tests for the CR-terminated line comment. --- .../ai/src/ai/common/graph/cypher_safety.py | 6 +++++- .../tests/ai/common/graph/test_cypher_safety.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/ai/src/ai/common/graph/cypher_safety.py b/packages/ai/src/ai/common/graph/cypher_safety.py index 8de7bfe78..211bee05a 100644 --- a/packages/ai/src/ai/common/graph/cypher_safety.py +++ b/packages/ai/src/ai/common/graph/cypher_safety.py @@ -58,9 +58,13 @@ # 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""" - //[^\n]* # line comment + //[^\r\n]* # line comment | /\*.*?\*/ # block comment | '(?:\\.|[^'\\])*' # single-quoted string | "(?:\\.|[^"\\])*" # double-quoted string diff --git a/packages/ai/tests/ai/common/graph/test_cypher_safety.py b/packages/ai/tests/ai/common/graph/test_cypher_safety.py index 5d04f4515..528084b2c 100644 --- a/packages/ai/tests/ai/common/graph/test_cypher_safety.py +++ b/packages/ai/tests/ai/common/graph/test_cypher_safety.py @@ -144,3 +144,20 @@ def test_unbalanced_quote_in_comment_cannot_hide_write(): """ 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