fix(mcp): escape text filter values and reject an empty projection - #667
Conversation
Two independent holes in the `search-records` request boundary.
Text filter values reached the query string unescaped. `Text`'s operator
templates are `@field:("value")` for eq/ne and `@field:(value)` for like,
so a value containing a quote or a paren could close its own clause and
have the remainder parsed as RediSearch syntax. Tag and numeric values
are already escaped or type-checked upstream; text was not. This escapes
text at the MCP filter boundary rather than in `Text.__str__`, so library
users who pass raw RediSearch syntax deliberately are unaffected. `like`
keeps `*` and `?` live, since it is the pattern-matching operator.
Note this is a behavior change on a surface that has shipped since
v0.17.0: a structured DSL value that previously smuggled query syntax
through now matches literally. That passthrough was never documented, and
the raw-string `filter` form still serves callers who want it.
Separately, `return_fields: []` was accepted and reached Redis as no
RETURN clause at all -- which returns every field, including the vector
that the same function refuses a few lines down. An empty list now
raises, with the error pointing at omitting the argument instead.
Also moves the shared `_schema()` builder into conftest, since the filter
and search unit tests were carrying identical copies.
9fee7d6 to
7a25665
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Follows up review feedback on the empty-projection guard: the guard rejects an explicit `return_fields: []`, but the *default* projection could still come out empty, and nothing caught that. The default projection is every non-vector field. `validate_runtime_mapping` only checked that `text_field_name` appeared in the schema, not that it was text-searchable, so a fulltext or hybrid binding could name the vector field on an index with no other fields. The projection then resolved to `[]`, and an empty `return_fields` is falsy -- RedisVL omits the RETURN clause entirely, so Redis returns every stored field, including the embedding this tool refuses to return when a caller names it explicitly. `vector_field_name` was already type-checked a few lines below, so this restores the symmetry that was missing. Worth recording why this is the whole fix. `VectorQuery` adds `vector_distance` to its own return fields, so the vector path emits a RETURN clause even from an empty projection; the hybrid paths prepend `__key` for the same reason. `TextQuery` does neither. With `text_field_name` now guaranteed to be a non-vector field, the fulltext and hybrid projections always contain at least that field, which makes "every search path emits a RETURN clause" true by construction rather than by coincidence. A test pins that invariant so a future search mode cannot quietly reintroduce the hole.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8e7a87f. Configure here.
The escaping change used `preserve_wildcards=True` for `like`, which keeps `*` and `?` but still escapes a space and `%`. Both carry meaning in a `like` pattern, so two documented behaviors broke, and neither broke loudly -- an over-escaped pattern is a valid query that matches nothing: like "foo bar" -> @content:(foo\ bar) AND between terms lost like "%foo%" -> @content:(\%foo\%) fuzzy matching lost `like` now uses its own character set: the shared no-wildcard set minus `%` and space. Exact-match values keep the stricter escaping, since an `eq` value is a literal while a `like` value is a pattern -- the same reason `*` was already exempt. Containment is unaffected, because it never came from these characters. It comes from the delimiters: with `(` and `)` escaped, a value cannot close its own `@field:(...)`, so anything it carries stays scoped to that one field. The injection payload from the escaping tests still renders fully contained, and its `|` -- which no escaper in RedisVL touches -- unions within `content` rather than reaching the surrounding expression. Tests pin each metacharacter separately, assert parity with `Text.__mod__` for ordinary patterns, and keep a containment case so a future widening of the preserved set cannot quietly reopen the hole.
tylerhutcherson
left a comment
tylerhutcherson
left a comment
There was a problem hiding this comment.
Left one comment, but looks good to merge generally.
| # a few lines down. "Omit the argument" is the way to ask for the | ||
| # default projection. | ||
| raise RedisVLMCPError( | ||
| "return_fields must not be empty; omit it to use the default " |
There was a problem hiding this comment.
we are sure that this return_fields won't get caught as a falsy value that could be interpreted 2 different ways (empty vs omitted)?
There was a problem hiding this comment.
Good instinct — the answer splits in two, and the second half found a gap worth pinning. Added a test in 1a1c078.
At this boundary: no, they can't be confused. The branch is if return_fields is None, not a truthiness check, so [] falls through to the else and hits the explicit rejection. Verified both ways:
omitted (None) -> fields=['content']
explicit empty [] -> REJECTED: return_fields must not be empty; omit it to use the default projection
And it's guarded, not incidental: weakening that line to if not return_fields makes test_search_records_rejects_an_empty_return_fields_list fail.
One layer down, though, you're pointing at something real. The default projection is every non-vector field, so an index whose only field is the vector resolves it to [] — and RedisVL does read a falsy return_fields as "no projection" and omits RETURN entirely:
text+vector index -> ['content'] falsy=False
vector-only index -> [] falsy=True
That path is safe today only because VectorQuery adds vector_distance to its own return fields, so a RETURN clause gets emitted anyway. That dependency was implicit — nothing would have caught it changing — so there's now a test asserting the default projection really is empty there, that the built query still carries RETURN, and that what comes back is the score rather than the embedding.
Fulltext and hybrid can't reach that state at all as of 8e7a87f, which requires text_field_name to be a non-vector field and so always puts it in the projection.
🤖 Addressed by Claude Code
Review question: can `return_fields` be read two ways, empty versus omitted? At the request boundary, no -- the branch is `is None`, so `[]` takes the else and is rejected rather than treated as absent. Weakening that check to `if not return_fields` makes `test_search_records_rejects_an_empty_return_fields_list` fail, so the distinction is guarded rather than incidental. The instinct is right one layer down, though. The *default* projection is every non-vector field, so an index whose only field is the vector resolves it to `[]` -- and RedisVL does read a falsy `return_fields` as "no projection" and omits RETURN. That path stays safe only because `VectorQuery` adds `vector_distance` to its own return fields, so a RETURN clause is emitted regardless. This adds a test for exactly that case: it asserts the default projection really is empty there, that the built query still carries RETURN, and that what comes back is the score rather than the embedding. The dependency on `VectorQuery` was previously implicit, so nothing would have caught it changing. Fulltext and hybrid cannot reach this state at all now that `text_field_name` must be a non-vector field, which puts it in the projection by construction.
|
🚀 PR was released in |
applied-ai-release-bot
Bot
added
the
released
2 participants
Two independent holes in the
search-recordsrequest boundary. Both are fixes to code already onmain, independent of any other MCP work in flight.Text filter values reached the query string unescaped
Text's operator templates are@field:("value")foreq/neand@field:(value)forlike, so a value containing a quote or a paren could close its own clause and have the remainder parsed as RediSearch syntax. Tag values are already escaped and numeric values are type-checked; text was neither.The fix escapes text at the MCP filter boundary rather than in
Text.__str__, so library users who pass raw RediSearch syntax deliberately are unaffected.likekeeps*and?live, since it is the pattern-matching operator.This is a behavior change on a surface that has shipped since v0.17.0 and is worth a release note. A structured DSL value that previously smuggled query syntax through now matches literally. That passthrough was never documented, and the raw-string
filterform still serves callers who want raw syntax — but it is a change, not purely an internal fix.return_fields: []was accepted and meant the opposite of what it reads likeAn empty list reached Redis as no
RETURNclause at all, which returns every field — including the vector that the same function refuses a few lines down. An empty list now raises, with the error pointing at omitting the argument to get the default projection.This guard had no test coverage. It does now, and I verified by mutation that the test fails when the guard is removed.
Also
Moves the shared
_schema()builder intoconftest.py; the filter and search unit tests were carrying identical copies.Verification
make check-types: cleanNote
Medium Risk
Changes query semantics for structured text filters (documented breaking behavior) and tightens search/config validation; scope is MCP-only, not core RedisVL Text rendering.
Overview
Hardens the MCP
search-recordsboundary in three related ways.Structured text filters now escape caller values before building RediSearch
Textexpressions (eq/ne/inuse full escaping;likeuses a narrower escaper so*,?,%, and spaces stay pattern-active). Raw stringfilterpassthrough is unchanged. This is a behavior change for DSL values that previously injected query syntax.return_fields: []is rejected with guidance to omit the argument for the default projection, because an empty list omitted RedisRETURNand could return every field, including vectors the tool blocks when named explicitly.Startup validation rejects
runtime.text_field_namepointing at a vector field for fulltext/hybrid (not just missing from the schema), avoiding an empty default projection on text search paths.Unit tests cover injection,
likesemantics, empty projection, and config; shared_schema()moved toconftest.py.Reviewed by Cursor Bugbot for commit 1a1c078. Bugbot is set up for automated code reviews on this repo. Configure here.