fix(lexical-graph): parameterize SQL filters and id lists in PGVector store - #11
Open
noel-improv wants to merge 5 commits into
Open
noel-improv wants to merge 5 commits into
noel-improv wants to merge 5 commits into
Conversation
|
Lexical Graph Coverage Report: The coverage is at 60.46% (target: 80%). Download the HTML report here. |
|
Lexical Graph Coverage Report: The coverage is at 60.46% (target: 80%). Download the HTML report here. |
… store Metadata filter values, metadata key names, and id lists were string-interpolated into the WHERE clauses of the PGVector store. A filter value or id containing a single quote could break out of its literal and inject arbitrary SQL, widening to UPDATE and DELETE via the raw IN (...) id-list builders. Bind all filter values, keys, and ids as psycopg2 parameters, matching the binding already used for the embedding vector and LIMIT: - parse_metadata_filters_recursive / filter_config_to_sql_filters return (clause, params); values use %s placeholders and the metadata key is bound into the JSON path operand (->>%s). - top_k splices the filter params between the embedding and LIMIT placeholders. - get_embeddings, update_versioning, and delete_embeddings build IN (%s, ...) placeholder lists; update_versioning also binds the timestamp. - formatter_for_type coerces the value for binding rather than quoting it. Only the type cast and the table/schema/index identifiers (internal config) remain interpolated, so the nosec B608 annotations are kept with accurate justifications. Adds tests that drive each sink against a mocked cursor and assert payloads reach cur.execute as bound parameters, never as inline SQL.
noel-improv
force-pushed
the
fix/pgvector-filter-sql-injection
branch
from
June 29, 2026 22:15
d2d19ad to
82dce0a
Compare
|
Lexical Graph Coverage Report: The coverage is at 60.46% (target: 80%). Download the HTML report here. |
An empty ids list produced `... IN ()`, which Postgres rejects as a syntax error, in get_embeddings, update_versioning, and delete_embeddings. Return early before opening a connection so an empty list is a no-op. Adds tests asserting no query is emitted for an empty list.
|
Lexical Graph Coverage Report: The coverage is at 60.81% (target: 80%). Download the HTML report here. |
Couples the integration test with this fix. It drives the real PGIndex sinks against a live Postgres+pgvector engine, asserting injected filter values, metadata keys, and id lists are inert while the pre-fix interpolation still breaks out. A dedicated workflow runs it against a Postgres+pgvector service container on release and on PRs touching the sink or the test. Skips when PGVECTOR_TEST_DSN is unset.
|
Lexical Graph Coverage Report: The coverage is at 60.81% (target: 80%). Download the HTML report here. |
The module docstring referenced its "parent fix PR", which is meaningless to a reader on main. Describe what the test does without the process context.
|
Lexical Graph Coverage Report: The coverage is at 60.81% (target: 80%). Download the HTML report here. |
…work Address review feedback on awslabs#362: - Remove the standalone pytest test and its GitHub Actions workflow; the toolkit runs integration tests manually on a SageMaker notebook, not in CI. - Add lexical_graph_pgvector_safety.LexicalGraphPGVectorInjectionSafety, extending IntegrationTestBase and driving the real PGIndex via VectorStoreFactory instead of constructing one directly. It skips unless VECTOR_STORE is Postgres-backed, so it is safe in lexical.short for every env-type. Assertions are unchanged: injected filter values, metadata keys, and id lists stay inert, with a red-state proof of the pre-fix interpolation. - Register it in lexical.short next to the Cypher-safety test, and document how to run it (a *-postgresql env-type) in the integration-tests README.
|
Lexical Graph Coverage Report: The coverage is at 60.81% (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
Metadata filter values, metadata key names, and id lists were string-interpolated into the SQL
WHEREclauses of the PGVector store. A filter value or id containing a single quote could break out of its literal and inject arbitrary SQL. Because theget_embeddings,update_versioning, anddelete_embeddingspaths build rawIN (...)lists the same way, the blast radius includesUPDATEandDELETE, not justSELECT. This change binds all filter values, keys, and ids as psycopg2 parameters instead of inlining them.The codebase already had the right pattern: the embedding vector and
LIMITintop_k, and every value inadd_embeddings, were already bound with%s. This change applies the same approach to the remaining sinks.What changed
parse_metadata_filters_recursiveandfilter_config_to_sql_filtersnow return(clause, params). Filter values become%splaceholders and the metadata key is bound into the JSON path operand (->>%s), so neither can break out of a literal.top_ksplices the filter params between the embedding andLIMITplaceholders, preserving order.get_embeddings,update_versioning, anddelete_embeddingsbuildIN (%s, ...)placeholder lists;update_versioningalso binds its timestamp.formatter_for_typenow coerces a value for parameter binding (native numbers,format_datetimefor timestamps,strfor text) rather than quoting it into SQL. TheLIKEwildcard wrapper drops its%%doubling, which is no longer needed once the value is a bound parameter.Only internally-derived tokens stay interpolated: the type cast and the table/schema/index identifiers (sourced from config, not user input). The
# nosec B608annotations are kept for those identifier interpolations, with corrected justifications noting that values, keys, and ids are now bound.Testing
New
tests/unit/storage/vector/test_pg_vector_injection.py(17 tests) drives each sink against a mocked cursor and asserts the payload reachescur.executeas a bound parameter, never as inline SQL. It covers the filter-value, filter-key, and three id-list sinks, a parametrized set of break-out payloads (tautology, stacked statement, comment terminator, null byte, unicode quote, backslash, newline), the clause-builder(clause, params)contract, and a positive path.Before the fix the five injection tests fail, printing the actual injected SQL; after the fix all pass.
tests/unit/storage(310 tests) and the related filter/metadata/graph suites (167 tests) pass unchanged. Bandit-llreports no medium+ findings.