Skip to content

Commit 7481cd0

Browse files
authored
Fix cosmosdb vectorstore bug (#14182)
### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [ ] The code builds clean without any errors or warnings - [ ] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [ ] All unit tests pass, and I have added new tests where possible - [ ] I didn't break anyone 😄
1 parent 61a6b0c commit 7481cd0

3 files changed

Lines changed: 306 additions & 94 deletions

File tree

‎python/semantic_kernel/connectors/azure_cosmos_db.py‎

Lines changed: 123 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import ast
44
import asyncio
5+
import re
56
import sys
67
from collections.abc import Sequence
78
from importlib import metadata
@@ -788,14 +789,36 @@ async def _inner_search(
788789
if not vector:
789790
vector = await self._generate_vector_from_values(values, options)
790791

791-
if where_clauses := self._build_filter(options.filter): # type: ignore
792+
# Translate the filter into a Cosmos SQL clause plus bound parameters. Filter string
793+
# constants are bound as query parameters (@filter_pN) instead of concatenated, preventing
794+
# SQL injection. _build_filter returns None, a single (clause, params) tuple, or a list of
795+
# them; parameter names are renumbered into one global sequence so they stay unique when
796+
# multiple filters are AND-combined.
797+
parsed_filter = self._build_filter(options.filter)
798+
where_clauses = ""
799+
if parsed_filter:
800+
clause_tuples = parsed_filter if isinstance(parsed_filter, list) else [parsed_filter]
801+
rendered_clauses: list[str] = []
802+
filter_param_count = 0
803+
for clause, clause_parameters in clause_tuples:
804+
rename: dict[str, str] = {}
805+
for parameter in clause_parameters:
806+
new_name = f"@filter_p{filter_param_count}"
807+
filter_param_count += 1
808+
rename[parameter["name"]] = new_name
809+
params.append({"name": new_name, "value": parameter["value"]})
810+
if rename:
811+
812+
def _substitute(match: "re.Match[str]", mapping: dict[str, str] = rename) -> str:
813+
return mapping[match.group(0)]
814+
815+
clause = re.sub(r"@filter_p\d+", _substitute, clause)
816+
rendered_clauses.append(clause)
792817
where_clauses = (
793-
f"WHERE {where_clauses} "
794-
if isinstance(where_clauses, str)
795-
else f"WHERE ({' AND '.join(where_clauses)}) "
818+
f"WHERE {rendered_clauses[0]} "
819+
if len(rendered_clauses) == 1
820+
else f"WHERE ({' AND '.join(rendered_clauses)}) "
796821
)
797-
else:
798-
where_clauses = "" # Empty string instead of None
799822
vector_field_name = vector_field.storage_name or vector_field.name
800823
select_clause = self._build_select_clause(options.include_vectors)
801824
params.append({"name": "@vector", "value": vector})
@@ -849,86 +872,102 @@ def _build_select_clause(self, include_vectors: bool) -> str:
849872
return ", ".join(f"c.{field}" for field in included_fields)
850873

851874
@override
852-
def _lambda_parser(self, node: ast.AST) -> Any:
853-
match node:
854-
case ast.Compare():
855-
if len(node.ops) > 1:
856-
# Chain comparisons (e.g., a < b < c) become AND-ed comparisons
857-
values = []
858-
for idx in range(len(node.ops)):
859-
if idx == 0:
860-
values.append(
861-
ast.Compare(
862-
left=node.left,
863-
ops=[node.ops[idx]],
864-
comparators=[node.comparators[idx]],
875+
def _lambda_parser(self, node: ast.AST) -> tuple[str, list[dict[str, Any]]]: # type: ignore[override]
876+
# The base contract returns just the filter string; Cosmos extends the return type to a
877+
# (clause, parameters) tuple so filter string constants can be bound as query parameters
878+
# (@filter_pN) rather than concatenated into the SQL text. This prevents SQL injection:
879+
# Cosmos DB uses backslash escape sequences (\'), so escaping by doubling apostrophes ('')
880+
# is both unsafe and functionally incorrect. The parameters list is local to this call, so
881+
# no shared state is used and it is safe to call concurrently from multiple tasks/threads.
882+
# This mirrors the composite-return pattern used by the SQL Server and Oracle connectors,
883+
# letting Cosmos reuse the base ``_build_filter`` for AST parsing/visiting.
884+
parameters: list[dict[str, Any]] = []
885+
886+
def parse(node: ast.AST) -> str:
887+
match node:
888+
case ast.Compare():
889+
if len(node.ops) > 1:
890+
# Chain comparisons (e.g., a < b < c) become AND-ed comparisons
891+
values = []
892+
for idx in range(len(node.ops)):
893+
if idx == 0:
894+
values.append(
895+
ast.Compare(
896+
left=node.left,
897+
ops=[node.ops[idx]],
898+
comparators=[node.comparators[idx]],
899+
)
865900
)
866-
)
867-
else:
868-
values.append(
869-
ast.Compare(
870-
left=node.comparators[idx - 1],
871-
ops=[node.ops[idx]],
872-
comparators=[node.comparators[idx]],
901+
else:
902+
values.append(
903+
ast.Compare(
904+
left=node.comparators[idx - 1],
905+
ops=[node.ops[idx]],
906+
comparators=[node.comparators[idx]],
907+
)
873908
)
874-
)
875-
return "(" + " AND ".join([self._lambda_parser(v) for v in values]) + ")"
876-
left = self._lambda_parser(node.left)
877-
right = self._lambda_parser(node.comparators[0])
878-
op = node.ops[0]
879-
match op:
880-
case ast.In():
881-
# Cosmos DB: ARRAY_CONTAINS(right, left)
882-
return f"ARRAY_CONTAINS({right}, {left})"
883-
case ast.NotIn():
884-
return f"NOT ARRAY_CONTAINS({right}, {left})"
885-
case ast.Eq():
886-
return f"{left} = {right}"
887-
case ast.NotEq():
888-
return f"{left} != {right}"
889-
case ast.Gt():
890-
return f"{left} > {right}"
891-
case ast.GtE():
892-
return f"{left} >= {right}"
893-
case ast.Lt():
894-
return f"{left} < {right}"
895-
case ast.LtE():
896-
return f"{left} <= {right}"
897-
raise NotImplementedError(f"Unsupported operator: {type(op)}")
898-
case ast.BoolOp():
899-
op_str = "AND" if isinstance(node.op, ast.And) else "OR"
900-
return "(" + f" {op_str} ".join([self._lambda_parser(v) for v in node.values]) + ")"
901-
case ast.UnaryOp():
902-
match node.op:
903-
case ast.Not():
904-
return f"NOT ({self._lambda_parser(node.operand)})"
905-
case ast.UAdd():
906-
return f"+{self._lambda_parser(node.operand)}"
907-
case ast.USub():
908-
return f"-{self._lambda_parser(node.operand)}"
909-
case ast.Invert():
910-
raise NotImplementedError("Invert operation is not supported.")
911-
raise NotImplementedError(f"Unsupported unary operator: {type(node.op)}")
912-
case ast.Attribute():
913-
# Cosmos DB: c.field_name
914-
if node.attr not in self.definition.storage_names:
915-
raise VectorStoreOperationException(
916-
f"Field '{node.attr}' not in data model (storage property names are used)."
917-
)
918-
return f"c.{node.attr}"
919-
case ast.Name():
920-
# Could be a variable or constant; not supported
921-
raise NotImplementedError("Constants or variables are not supported, use a value or attribute.")
922-
case ast.Constant():
923-
# Quote strings, leave numbers as is
924-
if isinstance(node.value, str):
925-
return "'" + node.value.replace("'", "''") + "'"
926-
if isinstance(node.value, (float, int)):
927-
return str(node.value)
928-
if node.value is None:
929-
return "null"
930-
raise NotImplementedError(f"Unsupported constant type: {type(node.value)}")
931-
raise NotImplementedError(f"Unsupported AST node: {type(node)}")
909+
return "(" + " AND ".join([parse(v) for v in values]) + ")"
910+
left = parse(node.left)
911+
right = parse(node.comparators[0])
912+
op = node.ops[0]
913+
match op:
914+
case ast.In():
915+
# Cosmos DB: ARRAY_CONTAINS(right, left)
916+
return f"ARRAY_CONTAINS({right}, {left})"
917+
case ast.NotIn():
918+
return f"NOT ARRAY_CONTAINS({right}, {left})"
919+
case ast.Eq():
920+
return f"{left} = {right}"
921+
case ast.NotEq():
922+
return f"{left} != {right}"
923+
case ast.Gt():
924+
return f"{left} > {right}"
925+
case ast.GtE():
926+
return f"{left} >= {right}"
927+
case ast.Lt():
928+
return f"{left} < {right}"
929+
case ast.LtE():
930+
return f"{left} <= {right}"
931+
raise NotImplementedError(f"Unsupported operator: {type(op)}")
932+
case ast.BoolOp():
933+
op_str = "AND" if isinstance(node.op, ast.And) else "OR"
934+
return "(" + f" {op_str} ".join([parse(v) for v in node.values]) + ")"
935+
case ast.UnaryOp():
936+
match node.op:
937+
case ast.Not():
938+
return f"NOT ({parse(node.operand)})"
939+
case ast.UAdd():
940+
return f"+{parse(node.operand)}"
941+
case ast.USub():
942+
return f"-{parse(node.operand)}"
943+
case ast.Invert():
944+
raise NotImplementedError("Invert operation is not supported.")
945+
raise NotImplementedError(f"Unsupported unary operator: {type(node.op)}")
946+
case ast.Attribute():
947+
# Cosmos DB: c.field_name
948+
if node.attr not in self.definition.storage_names:
949+
raise VectorStoreOperationException(
950+
f"Field '{node.attr}' not in data model (storage property names are used)."
951+
)
952+
return f"c.{node.attr}"
953+
case ast.Name():
954+
# Could be a variable or constant; not supported
955+
raise NotImplementedError("Constants or variables are not supported, use a value or attribute.")
956+
case ast.Constant():
957+
# Bind strings as query parameters to avoid SQL injection. Numbers and null
958+
# cannot carry injection, so they are inlined.
959+
if isinstance(node.value, str):
960+
name = f"@filter_p{len(parameters)}"
961+
parameters.append({"name": name, "value": node.value})
962+
return name
963+
if isinstance(node.value, (float, int)):
964+
return str(node.value)
965+
if node.value is None:
966+
return "null"
967+
raise NotImplementedError(f"Unsupported constant type: {type(node.value)}")
968+
raise NotImplementedError(f"Unsupported AST node: {type(node)}")
969+
970+
return parse(node), parameters
932971

933972
@override
934973
def _get_record_from_result(self, result: dict[str, Any]) -> dict[str, Any]:

‎python/tests/integration/memory/azure_cosmos_db/test_azure_cosmos_db_no_sql.py‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,3 +229,52 @@ async def test_custom_client(
229229
assert await collection.collection_exists() is False
230230
collection_names = await store.list_collection_names()
231231
assert collection_name not in collection_names
232+
233+
async def test_search_with_filter(
234+
self,
235+
stores: dict[str, Callable[[], VectorStore]],
236+
record_type: type,
237+
):
238+
"""Test vector search with equality filters, including apostrophe and injection payloads.
239+
240+
This is a regression test for the Cosmos DB NoSQL filter SQL-injection issue: filter
241+
constants must be bound as query parameters, so that:
242+
* an equality filter returns only the matching record(s),
243+
* a value containing an apostrophe (e.g. O'Reilly) works instead of raising HTTP 400, and
244+
* a malicious value (\\' OR true --) is treated as an inert literal and does not bypass
245+
the filter.
246+
"""
247+
vector = [0.1, 0.2, 0.3, 0.4, 0.5]
248+
records = [
249+
record_type(id="red-1", product_type="red", description="red record", vector=vector),
250+
record_type(id="blue-1", product_type="blue", description="blue record", vector=vector),
251+
record_type(id="oreilly-1", product_type="O'Reilly", description="apostrophe record", vector=vector),
252+
]
253+
254+
async with stores["azure_cosmos_db_no_sql"]() as store:
255+
collection_name = "search_with_filter"
256+
collection = store.get_collection(collection_name=collection_name, record_type=record_type)
257+
258+
try:
259+
await collection.ensure_collection_exists()
260+
await collection.upsert(records)
261+
262+
# Equality filter returns only the matching record.
263+
results = await collection.search(vector=vector, filter=lambda x: x.product_type == "red", top=10)
264+
matched = [result.record async for result in results.results]
265+
assert {record.id for record in matched} == {"red-1"}
266+
267+
# A value containing an apostrophe must be handled as a literal (no HTTP 400).
268+
results = await collection.search(vector=vector, filter=lambda x: x.product_type == "O'Reilly", top=10)
269+
matched = [result.record async for result in results.results]
270+
assert {record.id for record in matched} == {"oreilly-1"}
271+
272+
# A SQL-injection payload must not bypass the filter; it matches no record.
273+
results = await collection.search(
274+
vector=vector, filter=lambda x: x.product_type == "\\' OR true --", top=10
275+
)
276+
matched = [result.record async for result in results.results]
277+
assert matched == []
278+
finally:
279+
await collection.ensure_collection_deleted()
280+
assert await collection.collection_exists() is False

0 commit comments

Comments
 (0)