fix(filter): fail closed on unrecognized filter shapes
The filter body is arbitrary client JSON with no schema, so validation
was emergent: any shape the DSL didn't recognize surfaced as an
unhandled 500 from somewhere in SQLAlchemy or psycopg. Fixing individual
shapes doesn't converge — a fuzz over the DSL found five more families
beyond the three already fixed here:
{"AND": [None]} TypeError, non-dict in a logical list
{"AND": [[]]} AttributeError on .items()
{"session_id": {"gte": true}} SQLAlchemy ArgumentError
{"embedding": []} NotImplementedError, no python_type
{"session_id": {"ne": {...}}} execute-time "cannot adapt type 'dict'"
Two generic guards instead:
1. Any operand bound to a non-JSONB column must be a scalar, checked
element-wise for `in`. A dict or list bound to a scalar column
compiles cleanly and only fails in the driver at execute time, so it
has to be rejected during construction. JSONB columns are exempt —
a dict there is a containment match.
2. apply_filter fails closed: FilterError propagates, anything else is
logged with logger.exception (filter shape included) and re-raised as
FilterError. Unknown filter failures become 422s while staying fully
visible as errors rather than being swallowed.
Adds two invariant tests over a generated matrix of filter shapes: every
shape either compiles or raises FilterError, and no non-scalar is ever
bound to a scalar column. Both fail without the guards above. They cover
shapes nobody enumerated, so the next unimagined body fails in CI rather
than in production.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a7ce38e454
commit
d9de9b88f8
|
|
@ -65,6 +65,48 @@ ALLOWED_EXTERNAL_TO_INTERNAL_COLUMN_MAPPING_DOCUMENTS = {
|
|||
|
||||
MAX_SESSION_ALLOWLIST_ENTRIES = 1000
|
||||
|
||||
# Values that can be bound to a non-JSONB column. Anything else (dict, list,
|
||||
# bytes, arbitrary objects) compiles into a valid statement and then fails in
|
||||
# psycopg at execute time as an unhandled 500, so it is rejected up front.
|
||||
SCALAR_OPERAND_TYPES = (
|
||||
str,
|
||||
bool,
|
||||
int,
|
||||
float,
|
||||
Decimal,
|
||||
datetime.datetime,
|
||||
datetime.date,
|
||||
)
|
||||
|
||||
|
||||
def _require_bindable_operand(
|
||||
column_name: str, op_value: Any, operator: str = ""
|
||||
) -> None:
|
||||
"""Reject an operand that cannot be bound to a scalar column.
|
||||
|
||||
For ``in``, each element is checked: a dict nested in the list is bound the
|
||||
same way a bare dict operand would be, and fails identically.
|
||||
|
||||
Args:
|
||||
column_name: Internal column name, for the error message.
|
||||
op_value: The operand to check.
|
||||
operator: The comparison operator, when the operand came from one.
|
||||
|
||||
Raises:
|
||||
FilterError: If a value is neither None nor a scalar.
|
||||
"""
|
||||
values: Sequence[Any] = (
|
||||
typing_cast("Sequence[Any]", op_value)
|
||||
if operator == "in" and isinstance(op_value, list | tuple | set)
|
||||
else (op_value,)
|
||||
)
|
||||
for value in values:
|
||||
if value is None or isinstance(value, SCALAR_OPERAND_TYPES):
|
||||
continue
|
||||
raise FilterError(
|
||||
f"Invalid value for column '{column_name}': expected a scalar, got {type(value).__name__}"
|
||||
)
|
||||
|
||||
|
||||
def extract_session_allowlist(
|
||||
filters: dict[str, Any] | None,
|
||||
|
|
@ -183,9 +225,21 @@ def apply_filter(
|
|||
if filters is None:
|
||||
return stmt
|
||||
|
||||
conditions = _build_filter_conditions(filters, model_class)
|
||||
if conditions is not None:
|
||||
stmt = stmt.where(conditions)
|
||||
# Fail closed. The filter body is arbitrary client JSON, so any shape the
|
||||
# DSL doesn't recognize must become a 422, not an unhandled 500 from
|
||||
# somewhere deep in SQLAlchemy. The exception is still logged in full so a
|
||||
# genuine bug in the builder stays visible rather than being swallowed.
|
||||
try:
|
||||
conditions = _build_filter_conditions(filters, model_class)
|
||||
if conditions is not None:
|
||||
stmt = stmt.where(conditions)
|
||||
except FilterError:
|
||||
raise
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"Unexpected error building filter for %s: %r", model_class.__name__, filters
|
||||
)
|
||||
raise FilterError("Invalid filter configuration") from None
|
||||
|
||||
return stmt
|
||||
|
||||
|
|
@ -361,6 +415,7 @@ def _build_field_condition(
|
|||
if column_name in JSONB_COLUMNS:
|
||||
return column.contains(value)
|
||||
else:
|
||||
_require_bindable_operand(column_name, value)
|
||||
return column == value
|
||||
|
||||
|
||||
|
|
@ -593,6 +648,11 @@ def _build_comparison_conditions(
|
|||
conditions.append(column.is_not(None))
|
||||
continue
|
||||
|
||||
# Every operand bound to a scalar column must itself be a scalar. JSONB
|
||||
# columns are exempt: a dict there is a containment match.
|
||||
if not isinstance(column.type, JSONB):
|
||||
_require_bindable_operand(column_name, op_value, operator)
|
||||
|
||||
condition = None
|
||||
|
||||
# For datetime columns, cast string values to timestamp
|
||||
|
|
|
|||
|
|
@ -1,10 +1,17 @@
|
|||
"""Unit tests for filter condition building."""
|
||||
|
||||
from collections.abc import Sequence
|
||||
from datetime import datetime
|
||||
from decimal import Decimal
|
||||
from typing import Any, cast
|
||||
|
||||
import pytest
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.dialects import postgresql
|
||||
from sqlalchemy.dialects.postgresql import JSONB
|
||||
|
||||
from src.exceptions import FilterError
|
||||
from src.models import Document, Message
|
||||
from src.models import Document, Message, Peer, Session
|
||||
from src.utils.filter import apply_filter
|
||||
|
||||
|
||||
|
|
@ -58,3 +65,115 @@ def test_ne_none_on_numeric_column_is_not_null():
|
|||
def test_null_operand_on_non_ne_operator_raises():
|
||||
with pytest.raises(FilterError):
|
||||
apply_filter(select(Message), Message, {"token_count": {"gt": None}})
|
||||
|
||||
|
||||
# --- Invariants over the whole DSL -------------------------------------------
|
||||
#
|
||||
# The filter body is arbitrary client JSON. Enumerating bad shapes one at a time
|
||||
# is endless, so these two tests assert the properties that make any unhandled
|
||||
# shape a 422 instead of a 500, and fail on the next shape nobody thought of.
|
||||
|
||||
_OPERANDS: list[Any] = [
|
||||
None,
|
||||
True,
|
||||
False,
|
||||
0,
|
||||
-1,
|
||||
1.5,
|
||||
"",
|
||||
"abc",
|
||||
"*",
|
||||
[],
|
||||
[None],
|
||||
[[1]],
|
||||
[{"a": 1}],
|
||||
{},
|
||||
{"operator": "null"},
|
||||
{"ne": None},
|
||||
{"ne": {"a": 1}},
|
||||
{"ne": [1]},
|
||||
{"in": None},
|
||||
{"in": "abc"},
|
||||
{"in": [{"a": 1}]},
|
||||
{"in": [[1]]},
|
||||
{"gt": {}},
|
||||
{"gt": []},
|
||||
{"gt": True},
|
||||
{"contains": None},
|
||||
{"contains": {"a": 1}},
|
||||
{"lt": [1, 2]},
|
||||
]
|
||||
|
||||
_COLUMNS: dict[Any, list[str]] = {
|
||||
Document: ["session_id", "workspace_id", "metadata", "level", "source_ids", "id"],
|
||||
Message: ["session_id", "peer_id", "token_count", "created_at", "metadata"],
|
||||
Session: ["id", "is_active", "created_at", "configuration"],
|
||||
Peer: ["id", "created_at", "metadata"],
|
||||
}
|
||||
|
||||
_MALFORMED: list[dict[str, Any]] = [
|
||||
{"AND": "notalist"},
|
||||
{"AND": [None]},
|
||||
{"AND": [[]]},
|
||||
{"AND": [1]},
|
||||
{"OR": [None]},
|
||||
{"OR": [1]},
|
||||
{"NOT": None},
|
||||
{"NOT": [None]},
|
||||
{"unknown_column": 1},
|
||||
{"metadata": None},
|
||||
]
|
||||
|
||||
|
||||
def _filter_shapes() -> list[tuple[Any, dict[str, Any]]]:
|
||||
shapes: list[tuple[Any, dict[str, Any]]] = []
|
||||
for model, columns in _COLUMNS.items():
|
||||
for column in columns:
|
||||
for operand in _OPERANDS:
|
||||
leaf = {column: operand}
|
||||
shapes.append((model, leaf))
|
||||
shapes.append((model, {"AND": [leaf]}))
|
||||
shapes.append((model, {"NOT": [leaf]}))
|
||||
shapes.extend((model, bad) for bad in _MALFORMED)
|
||||
return shapes
|
||||
|
||||
|
||||
def test_every_filter_shape_either_compiles_or_raises_filter_error():
|
||||
"""No filter body may escape as anything other than a compiled statement or
|
||||
a FilterError. Anything else reaches the client as an unhandled 500."""
|
||||
escaped: list[tuple[str, dict[str, Any], str]] = []
|
||||
for model, filters in _filter_shapes():
|
||||
try:
|
||||
str(apply_filter(select(model), model, filters))
|
||||
except FilterError:
|
||||
pass
|
||||
except Exception as exc: # pragma: no cover - failure path
|
||||
escaped.append((model.__name__, filters, type(exc).__name__))
|
||||
assert not escaped, f"non-FilterError escapes: {escaped[:10]}"
|
||||
|
||||
|
||||
def test_no_non_scalar_value_is_bound_to_a_scalar_column():
|
||||
"""A dict or list bound to a non-JSONB parameter compiles cleanly and then
|
||||
fails in psycopg at execute time — the original 500. Nothing may reach that
|
||||
state, including non-scalars nested inside an `in` list."""
|
||||
offenders: list[tuple[str, dict[str, Any], str]] = []
|
||||
for model, filters in _filter_shapes():
|
||||
try:
|
||||
stmt = apply_filter(select(model), model, filters)
|
||||
except FilterError:
|
||||
continue
|
||||
compiled = stmt.compile(dialect=postgresql.dialect())
|
||||
for bind in compiled.binds.values():
|
||||
if isinstance(bind.type, JSONB):
|
||||
continue
|
||||
value: Any = bind.value
|
||||
# An expanding IN bind holds the list itself; check its elements.
|
||||
elements = cast(
|
||||
"Sequence[Any]", value if isinstance(value, list | tuple) else [value]
|
||||
)
|
||||
for element in elements:
|
||||
if element is not None and not isinstance(
|
||||
element, str | bool | int | float | Decimal | datetime
|
||||
):
|
||||
offenders.append((model.__name__, filters, repr(element)[:40]))
|
||||
assert not offenders, f"non-scalar bound to scalar column: {offenders[:10]}"
|
||||
|
|
|
|||
Loading…
Reference in New Issue