From 5279e0f865b6b35434e69182a02a11a231a728ec Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:11:23 -0400 Subject: [PATCH] fix(filter): reject session ids that can't name a real session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extract_session_allowlist accepted any non-empty string, so "*" reached the three consumers of the allowlist — direct IN, the filter DSL, and a Python membership test — which disagree about it. The DSL reads "*" as "drop the condition" and matches every session; the others treat it as a literal name and match none. One /chat request could have some recall sources unscoped and others scoped to nothing. Entries are now validated against RESOURCE_NAME_PATTERN, the same pattern the API requires of session ids, so no session could ever be named "*" anyway. Wildcards were never part of this endpoint's documented contract (an id, a list of ids, or {"in": [...]}), and a wildcard alongside a top-level session_id already 422'd via must_include. --- src/utils/filter.py | 18 ++++++++++++++++++ tests/test_session_allowlist.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/utils/filter.py b/src/utils/filter.py index 9267b6dd..3e46615f 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -1,4 +1,5 @@ import datetime +import re from collections.abc import Callable, Sequence from decimal import Decimal from logging import getLogger @@ -10,6 +11,7 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.types import Numeric from ..exceptions import FilterError +from ..schemas.api import RESOURCE_NAME_PATTERN from .formatting import ILIKE_ESCAPE_CHAR, escape_ilike_pattern, parse_datetime_iso from .types import DocumentLevel, VectorSyncState @@ -233,6 +235,11 @@ def extract_session_allowlist( FilterError (422) rather than being silently ignored — a dropped filter on these endpoints would widen recall scope. + Entries must be well-formed session ids. Wildcards are not part of this + subset: the DSL treats ``*`` as "match everything" while the non-DSL + consumers of the allowlist treat it as a literal name, so it is rejected + rather than meaning two things at once. + Args: filters: The raw ``filters`` body, or None. must_include: A session id that must appear in the parsed allowlist — @@ -285,6 +292,17 @@ def extract_session_allowlist( for entry in entries: if not isinstance(entry, str) or not entry: raise FilterError("filters.session_id entries must be non-empty strings") + # Only names a session could actually have. The allowlist reaches + # queries three ways — direct `IN`, the filter DSL, and a Python + # membership test — and they don't agree on a value like "*", which the + # DSL reads as "drop the condition" while the others treat as a literal. + # Rejecting it here keeps the divergent value away from all three, and + # matches this endpoint's documented contract (an id, a list of ids, or + # {"in": [...]}) which never included wildcards. + if not re.fullmatch(RESOURCE_NAME_PATTERN, entry): + raise FilterError( + f"Invalid session id in filters.session_id: {entry!r}. Session ids match {RESOURCE_NAME_PATTERN}" + ) if entry not in seen: seen.add(entry) allowlist.append(entry) diff --git a/tests/test_session_allowlist.py b/tests/test_session_allowlist.py index aa6077cb..86c9280b 100644 --- a/tests/test_session_allowlist.py +++ b/tests/test_session_allowlist.py @@ -62,6 +62,38 @@ class TestExtractSessionAllowlist: with pytest.raises(FilterError): extract_session_allowlist({"session_id": bad}) + @pytest.mark.parametrize( + "filters", + [ + {"session_id": "*"}, + {"session_id": ["s1", "*"]}, + {"session_id": {"in": ["*"]}}, + ], + ) + def test_wildcard_rejected(self, filters: dict[str, Any]): + """A wildcard means two different things depending on which consumer + receives the allowlist: the filter DSL drops the condition entirely + (matching every session), while the direct `IN` and Python membership + paths treat "*" as a literal session name (matching none). It is not + part of this endpoint's contract, so it is rejected outright. + + The mixed list is the case that matters most — it looks narrowed. + """ + with pytest.raises(FilterError, match="Invalid session id"): + extract_session_allowlist(filters) + + @pytest.mark.parametrize("name", ["a b", "a/b", "a.b", "a%b", "s1;drop"]) + def test_malformed_session_ids_rejected(self, name: str): + with pytest.raises(FilterError, match="Invalid session id"): + extract_session_allowlist({"session_id": name}) + + def test_valid_id_characters_still_accepted(self): + """The pattern must not be stricter than the ids the API actually + issues, which include underscores and hyphens.""" + assert extract_session_allowlist({"session_id": "Valid_name-123"}) == [ + "Valid_name-123" + ] + def test_cap_enforced(self): too_many = [f"s{i}" for i in range(MAX_SESSION_ALLOWLIST_ENTRIES + 1)] with pytest.raises(FilterError, match="at most"):