fix(lexical-graph): escape OpenCypher metadata filter values and keys - #12
Open
noel-improv wants to merge 4 commits into
Open
noel-improv wants to merge 4 commits into
noel-improv wants to merge 4 commits into
Conversation
Metadata filters were converted to an OpenCypher filter substring by raw string
interpolation: text values were wrapped in single quotes with no escaping and
property keys were interpolated as source.{key}. A filter value or key
containing a single quote (or backtick) could break out and inject OpenCypher.
This substring is spliced into queries by several callers, so it bypasses the
parameter binding the graph store uses elsewhere.
Escape at the point of interpolation, reusing the module's existing
backtick-escaping helper:
- new escape_cypher_string doubles backslashes then backslash-escapes single
quotes, so a value cannot close its literal.
- formatter_for_type routes text and timestamp values through it.
- to_key backtick-quotes the property key via escape_cypher_label
(source.`{key}`), so a key cannot break out of the identifier.
Adds tests asserting values are escaped inside their literal and keys are
backtick-quoted (embedded backticks doubled), plus the benign positive paths.
Existing graph_utils assertions are updated for the backtick-quoted keys.
|
Lexical Graph Coverage Report: The coverage is at 60.13% (target: 80%). Download the HTML report here. |
…ed numeric path Records that doubling a backslash in escape_cypher_string changes match semantics for a value containing a literal backslash (acceptable, since a correct non-injectable filter takes priority), and that the numeric formatter is intentionally unquoted because the type is derived from the value's Python type. Comments only, no behaviour change.
|
Lexical Graph Coverage Report: The coverage is at 60.13% (target: 80%). Download the HTML report here. |
Couples the integration test with this fix. It runs the clause emitted by the real filter builder against a live openCypher engine, so a regression that drops the escaping is caught. A dedicated workflow runs it against a Neo4j service container on release and on PRs touching the sink or the test. Skips when NEO4J_TEST_URI is unset.
|
Lexical Graph Coverage Report: The coverage is at 60.13% (target: 80%). Download the HTML report here. |
…fety suite Move the standalone OpenCypher filter-injection test into the lexical cypher-safety suite as LexicalGraphFilterInjectionSafety, and register it in lexical.short. Removes the standalone workflow and test file it replaces. Default the Neptune DB data-plane endpoint to port 8182 in the suite's _make_graph_store helpers. Without it the neptunedata client uses the https default (443), which the cluster does not serve, so the neptune-db tests time out instead of connecting. Validated live on a Neptune DB stack: LexicalGraphFilterInjectionSafety and the LexicalGraphLabelInjectionSafety baseline both pass; the filter key and value escaping holds (canary survives key and value injection).
|
Lexical Graph Coverage Report: The coverage is at 60.13% (target: 80%). Download the HTML report here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Escape metadata filter values and property keys in the OpenCypher filter builder (
graph_utils.py) instead of interpolating them raw into the filter substring, closing an OpenCypher injection where a value or key containing a single quote (or backtick) could break out and inject Cypher. The substring is spliced into queries by several callers, so it bypasses the parameter binding the graph store uses on its own_execute_querypaths.This follows the same escaping approach already merged for Cypher label injection (
escape_cypher_label), reusing that helper for the property-key case.Changes
A new
escape_cypher_stringdoubles backslashes and then backslash-escapes single quotes, so a value cannot close its literal.formatter_for_typeroutes text and timestamp values through it.to_keybacktick-quotes the property key viaescape_cypher_label(source.\{key}``), so a key cannot break out of the identifier.Tests
New
test_graph_utils_injection.pyasserts that values are escaped inside their single-quoted literal and that keys are backtick-quoted with embedded backticks doubled, plus the benign positive paths. The existingtest_graph_utils.pyassertions are updated for the backtick-quoted keys.The emitted clauses were also validated against a live openCypher engine (Neo4j): legitimate filters match, the injection payloads are inert, and the old interpolated pattern breaks out on the same engine. Bandit reports no findings.