fix(filter): reject session ids that can't name a real session
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.
This commit is contained in:
parent
59f82944c5
commit
5279e0f865
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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"):
|
||||
|
|
|
|||
Loading…
Reference in New Issue