fix(db_neo4j): don't reject read-only Cypher that names properties like write keywords#1592
Conversation
…ke write keywords _is_cypher_safe matched write keywords with \bKEYWORD\b, so a read-only query that references a property or identifier spelled like a write keyword (e.g. MATCH (n) RETURN n.delete) was wrongly refused as a write. Add a leading negative look-behind (?<![\w.]) so a keyword used as attribute access or a compound identifier is not matched, and strip string literals before comments so a keyword inside a string -- or a // sequence inside a string -- neither triggers a false rejection nor hides a real trailing write keyword. This mirrors the accepted db_arango fix (rocketride-org#1294). Unlike db_arango, db_neo4j has no EXPLAIN-plan gate (_validate_query runs EXPLAIN for syntax only), so this keyword scan is the primary read-only gate; a bare label or identifier spelled exactly like a keyword can still match, which a text scan cannot disambiguate. That deeper plan-based gate is tracked separately and is out of scope here. Adds nodes/test/test_db_neo4j.py covering the safety gate (attribute access, string literals, comments), isValid parsing, and namespace stripping. Test plan: # utils imports only the stdlib, so the gate is verifiable without infra: python3 - <<'PY' import importlib.util; from pathlib import Path s = importlib.util.spec_from_file_location('u', 'nodes/src/nodes/db_neo4j/utils.py') u = importlib.util.module_from_spec(s); s.loader.exec_module(u) assert u._is_cypher_safe('MATCH (n) RETURN n.delete') is True assert u._is_cypher_safe('MATCH (n) DETACH DELETE n') is False PY # full suite (in the repo env): builder nodes:test # or: pytest nodes/test/test_db_neo4j.py
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
📝 WalkthroughWalkthroughChangesNeo4j Cypher safety gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/db_neo4j/utils.py`:
- Around line 79-85: The query sanitization logic in the visible stripping
sequence must use one combined regex pass for quoted strings, line comments, and
block comments, with alternatives ordered so comments are consumed before quote
matching can cross them. Update the `stripped` processing in the surrounding
utility to remove all three token types left-to-right, preserving escaped-quote
handling and preventing matches from spanning comment boundaries.
In `@nodes/test/test_db_neo4j.py`:
- Around line 85-87: Add a test alongside
test_slashslash_in_string_does_not_hide_write_keyword that passes
_is_cypher_safe a query containing a line comment with an unbalanced quote
followed across a newline by a write keyword, and assert the result is False.
Ensure the case specifically verifies comment quotes cannot hide valid Cypher
write operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76792d02-02c5-4be0-9193-18192e8f9912
📒 Files selected for processing (2)
nodes/src/nodes/db_neo4j/utils.pynodes/test/test_db_neo4j.py
… safety-gate bypass
Stripping string literals and comments in separate passes let an unbalanced
quote inside a comment swallow a trailing write keyword across newlines: the
string pattern's character class matches newlines, so a lone `"` in a comment
consumed everything up to the next quote. A query like
MATCH (n)
// start "
DELETE n
// end "
got reduced to `MATCH (n)` and slipped past _is_cypher_safe as read-only.
Fold the four patterns into a single alternation so tokens are matched
left-to-right and a comment consumes its own contents before a string
pattern can be tricked by a quote inside it. Add a regression test.
|
Addressed the CodeRabbit review in 4c1945e: folded the string/comment stripping into a single alternation pass to close the safety-gate bypass, and added a regression test for the unbalanced-quote-in-comment case. Confirmed the gate now rejects the exploit query while still allowing keyword-named attributes/strings. |
|
The fix itself is right and still needed, but The exact same read-only gate now lives in For the test: Thanks for chasing this down — the false-positive analysis is spot on, it just needs to land one layer up. |
2 participants
Fixes #1375
Problem
db_neo4j's read-only gate_is_cypher_safe(innodes/src/nodes/db_neo4j/utils.py) matches write keywords with\bKEYWORD\b. That word-boundary matches a keyword used as an attribute name, so a perfectly read-only query like:is wrongly rejected as a write. This is the same class of bug that was fixed for
db_arangoin #1294.Why neo4j needs its own fix:
db_arangocan lean on its EXPLAIN-plan gate as the authoritative read-only check, so there the keyword scan is only a soft backstop.db_neo4jhas no such gate —_validate_queryrunsEXPLAINfor syntax only — so the keyword scan is the primary read-only gate here.Change
_UNSAFE_CYPHER: add a leading negative look-behind(?<![\w.])so a write keyword used as attribute access or a compound identifier (n.delete,last_set) is not matched._is_cypher_safe: strip string literals and comments in a single alternation pass, so a keyword inside a string (WHERE n.op = "CREATE") doesn't cause a false rejection, and a stray quote inside a comment can't make string-stripping span newlines and swallow a real trailing write keyword. This mirrors the arango implementation.A bare label or identifier spelled exactly like a keyword (e.g. a node label literally named
Delete) can still match — a text scan can't tell a keyword from an identifier. Closing that gap needs a real plan-based read-only gate, which is tracked separately and left out of scope here to keep this change focused.Tests
Adds
nodes/test/test_db_neo4j.pycovering the safety gate (attribute access, string literals, comments, case-insensitivity),_parse_is_valid, and_strip_ns.utils.pyimports only the stdlib, so the gate is testable without a live Neo4j.builder nodes:test # or: pytest nodes/test/test_db_neo4j.pyRead-only queries such as
MATCH (n) RETURN n.deletenow pass the gate; writes such asMATCH (n) DETACH DELETE nare still rejected.Summary by CodeRabbit
Bug Fixes
Tests