Skip to content

Commit 2fcd037

Browse files
rhdedgarcdoern
andauthored
fix: OCI26ai sql query patches (#5046)
# What does this PR do? <!-- Provide a short summary of what this PR does and why. Link to relevant issues if applicable. --> Parameterizes SQL queries to prevent injection in the sqlstore and OCI providers. - Replace manual string escaping (`replace("'", "''")`) with parameterized bind variables in the authorized sqlstore's access control WHERE clause builder - Fix SQL injection in OCI 26AI vector_io provider where `chunk_id` values were directly concatenated into a `DELETE` query - Add `where_sql_params` support to the `SqlStore` protocol and SQLAlchemy implementation so raw SQL clauses can carry bound parameters - Add a pre-commit hook to detect f-string interpolation in SQL statements and prevent regressions <!-- If resolving an issue, uncomment and update the line below --> <!-- Closes #[issue-number] --> Closes RHAIENG-3254 ## Test Plan <!-- Describe the tests you ran to verify your changes with result summaries. *Provide clear instructions so the plan can be easily re-executed.* --> Existing unit tests pass (`test_authorized_sqlstore.py`, `test_sqlstore.py` — 15/15) Pre-commit hooks pass on all modified files <!-- For API changes, include: 1. A testing script (Python, curl, etc.) that exercises the new/modified endpoints 2. The output from running your script Example: ```python ... ... ``` Output: ``` <paste actual output here> ``` --> Signed-off-by: Doug Edgar <dedgar@redhat.com> Co-authored-by: Charlie Doern <cdoern@redhat.com>
1 parent 7fa5738 commit 2fcd037

5 files changed

Lines changed: 64 additions & 25 deletions

File tree

.pre-commit-config.yaml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,33 @@ repos:
221221
echo;
222222
exit 1;
223223
} || true
224+
- id: no-sql-string-interpolation
225+
name: Block f-string SQL construction (SQL injection risk)
226+
entry: bash
227+
language: system
228+
types: [python]
229+
pass_filenames: true
230+
args:
231+
- -c
232+
- |
233+
grep -EnH 'f"""[^"]*\b(DELETE|INSERT|UPDATE|SELECT|MERGE)\b.*\{[^}]*\}' "$@" \
234+
| grep -v 'self\.table_name' \
235+
| grep -v 'self\.dimensions' \
236+
| grep -v 'self\.vector_datatype' \
237+
| grep -v '#\s*nosec' \
238+
&& {
239+
echo;
240+
echo "SQL injection risk: f-string interpolation in SQL query detected."
241+
echo "Use parameterized bind variables instead:"
242+
echo " oracledb: :param_name"
243+
echo " psycopg2: %s or %(name)s"
244+
echo " aiosqlite: ? or :name"
245+
echo " SQLAlchemy: text().bindparams()"
246+
echo "If the interpolated value is a safe schema identifier (not user data),"
247+
echo "add '# nosec' to the line to suppress this check."
248+
echo;
249+
exit 1;
250+
} || true
224251
- id: check-api-independence
225252
name: Ensure llama_stack_api does not import llama_stack
226253
entry: bash

src/llama_stack/core/storage/sqlstore/authorized_sqlstore.py

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
# This source code is licensed under the terms described in the LICENSE file in
55
# the root directory of this source tree.
66

7+
import re
78
from collections.abc import Mapping, Sequence
89
from typing import Any, Literal
910

@@ -165,11 +166,12 @@ async def fetch_all(
165166
action: Action = Action.READ,
166167
) -> PaginatedResponse:
167168
"""Fetch all rows with automatic access control filtering."""
168-
access_where = self._build_access_control_where_clause(self.policy)
169+
access_where, access_params = self._build_access_control_where_clause(self.policy)
169170
rows = await self.sql_store.fetch_all(
170171
table=table,
171172
where=where,
172173
where_sql=access_where,
174+
where_sql_params=access_params,
173175
limit=limit,
174176
order_by=order_by,
175177
cursor=cursor,
@@ -236,9 +238,10 @@ async def delete(self, table: str, where: Mapping[str, Any]) -> None:
236238
"""Delete rows with automatic access control filtering."""
237239
await self.sql_store.delete(table, where)
238240

239-
def _build_access_control_where_clause(self, policy: list[AccessRule]) -> str:
241+
def _build_access_control_where_clause(self, policy: list[AccessRule]) -> tuple[str, dict[str, Any]]:
240242
"""Build SQL WHERE clause for access control filtering.
241243
244+
Returns a tuple of (sql_clause, bind_params) using parameterized queries.
242245
Only applies SQL filtering for the default policy to ensure correctness.
243246
For custom policies, uses conservative filtering to avoid blocking legitimate access.
244247
"""
@@ -294,46 +297,46 @@ def _get_public_access_conditions(self) -> list[str]:
294297
"""
295298
return ["owner_principal = ''"]
296299

297-
def _build_default_policy_where_clause(self, current_user: User | None) -> str:
300+
def _build_default_policy_where_clause(self, current_user: User | None) -> tuple[str, dict[str, Any]]:
298301
"""Build SQL WHERE clause for the default policy.
299302
303+
Returns a tuple of (sql_clause, bind_params) using parameterized queries.
300304
Default policy: permit all actions when user in owners [roles, teams, projects, namespaces]
301305
This means user must match ANY attribute category that exists in the resource (OR logic).
302306
"""
303307
base_conditions = self._get_public_access_conditions()
308+
params: dict[str, Any] = {}
304309

305310
if current_user:
306-
# Add "user is owner" condition - user's principal matches owner_principal
307-
escaped_principal = current_user.principal.replace("'", "''")
308-
base_conditions.append(f"owner_principal = '{escaped_principal}'")
311+
params["owner_principal_match"] = current_user.principal
312+
base_conditions.append("owner_principal = :owner_principal_match")
309313

310-
# Add "user in owners" conditions for attribute matching
311314
if current_user.attributes:
312315
for attr_key, user_values in current_user.attributes.items():
313316
if user_values:
314317
value_conditions = []
315-
for value in user_values:
316-
# Check if JSON array contains the value
317-
escaped_value = value.replace("'", "''")
318+
safe_key = re.sub(r"[^a-zA-Z0-9_]", "_", attr_key)
319+
for j, value in enumerate(user_values):
320+
param_name = f"attr_{safe_key}_{j}"
318321
json_text = self._json_extract_text("access_attributes", attr_key)
319-
value_conditions.append(f"({json_text} LIKE '%\"{escaped_value}\"%')")
322+
value_conditions.append(f"({json_text} LIKE :{param_name})")
323+
params[param_name] = f'%"{value}"%'
320324

321325
if value_conditions:
322-
# User matches this category if any of their values match
323-
user_matches_category = f"({' OR '.join(value_conditions)})"
324-
base_conditions.append(user_matches_category)
325-
return f"({' OR '.join(base_conditions)})"
326+
base_conditions.append(f"({' OR '.join(value_conditions)})")
326327

327-
def _build_conservative_where_clause(self) -> str:
328+
return f"({' OR '.join(base_conditions)})", params
329+
330+
def _build_conservative_where_clause(self) -> tuple[str, dict[str, Any]]:
328331
"""Conservative SQL filtering for custom policies.
329332
333+
Returns a tuple of (sql_clause, bind_params) using parameterized queries.
330334
Only filters records we're 100% certain would be denied by any reasonable policy.
331335
"""
332336
current_user = get_authenticated_user()
333337

334338
if not current_user:
335-
# Only allow public records
336339
base_conditions = self._get_public_access_conditions()
337-
return f"({' OR '.join(base_conditions)})"
340+
return f"({' OR '.join(base_conditions)})", {}
338341

339-
return "1=1"
342+
return "1=1", {}

src/llama_stack/core/storage/sqlstore/sqlalchemy_sqlstore.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ async def fetch_all(
187187
table: str,
188188
where: Mapping[str, Any] | None = None,
189189
where_sql: str | None = None,
190+
where_sql_params: Mapping[str, Any] | None = None,
190191
limit: int | None = None,
191192
order_by: list[tuple[str, Literal["asc", "desc"]]] | None = None,
192193
cursor: tuple[str, str] | None = None,
@@ -200,7 +201,10 @@ async def fetch_all(
200201
query = query.where(_build_where_expr(table_obj.c[key], value))
201202

202203
if where_sql:
203-
query = query.where(text(where_sql))
204+
clause = text(where_sql)
205+
if where_sql_params:
206+
clause = clause.bindparams(**where_sql_params)
207+
query = query.where(clause)
204208

205209
# Handle cursor-based pagination
206210
if cursor:
@@ -287,9 +291,10 @@ async def fetch_one(
287291
table: str,
288292
where: Mapping[str, Any] | None = None,
289293
where_sql: str | None = None,
294+
where_sql_params: Mapping[str, Any] | None = None,
290295
order_by: list[tuple[str, Literal["asc", "desc"]]] | None = None,
291296
) -> dict[str, Any] | None:
292-
result = await self.fetch_all(table, where, where_sql, limit=1, order_by=order_by)
297+
result = await self.fetch_all(table, where, where_sql, where_sql_params, limit=1, order_by=order_by)
293298
if not result.data:
294299
return None
295300
return result.data[0]

src/llama_stack/providers/remote/vector_io/oci/oci26ai.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -425,13 +425,15 @@ async def delete(self):
425425

426426
async def delete_chunks(self, chunks_for_deletion: list[ChunkForDeletion]) -> None:
427427
chunk_ids = [c.chunk_id for c in chunks_for_deletion]
428+
if not chunk_ids:
429+
return
430+
placeholders = [f":id_{i}" for i in range(len(chunk_ids))]
431+
params = {f"id_{i}": cid for i, cid in enumerate(chunk_ids)}
428432
cursor = self.connection.cursor()
429433
try:
430434
cursor.execute(
431-
f"""
432-
DELETE FROM {self.table_name}
433-
WHERE chunk_id IN ({", ".join([f"'{chunk_id}'" for chunk_id in chunk_ids])})
434-
"""
435+
f"DELETE FROM {self.table_name} WHERE chunk_id IN ({', '.join(placeholders)})",
436+
params,
435437
)
436438
except Exception as e:
437439
logger.error(f"Error deleting chunks from Oracle 26AI table {self.table_name}: {e}")

src/llama_stack_api/internal/sqlstore.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ async def fetch_all(
5050
table: str,
5151
where: Mapping[str, Any] | None = None,
5252
where_sql: str | None = None,
53+
where_sql_params: Mapping[str, Any] | None = None,
5354
limit: int | None = None,
5455
order_by: list[tuple[str, Literal["asc", "desc"]]] | None = None,
5556
cursor: tuple[str, str] | None = None,
@@ -60,6 +61,7 @@ async def fetch_one(
6061
table: str,
6162
where: Mapping[str, Any] | None = None,
6263
where_sql: str | None = None,
64+
where_sql_params: Mapping[str, Any] | None = None,
6365
order_by: list[tuple[str, Literal["asc", "desc"]]] | None = None,
6466
) -> dict[str, Any] | None: ...
6567

0 commit comments

Comments
 (0)