Skip to content

fix: contain Text equality and numeric filter values in their own clause - #721

Open
vishal-bala wants to merge 1 commit into
fix/escape-pipe-in-tag-valuesfrom
fix/contain-filter-values-in-their-clause
Open

fix: contain Text equality and numeric filter values in their own clause#721
vishal-bala wants to merge 1 commit into
fix/escape-pipe-in-tag-valuesfrom
fix/contain-filter-values-in-their-clause

Conversation

@vishal-bala

@vishal-bala vishal-bala commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Motivation

A filter value could terminate the clause it was rendered into and append arbitrary RediSearch syntax. Text.__str__ interpolated the caller's value straight into its operator templates, so a value carrying a " closed the quoted phrase early; under DIALECT 2 an injected | binds looser than the implicit space-AND, so it lifts to the root of the parse tree and any surrounding filter stops constraining the query. Measured on Redis 8.4.5 against an index holding two tenants, str((Tag("tenant") == "acme") & (Text("v") == crafted)) returned the other tenant's document and FT.EXPLAINCLI reported a top-level UNION. Num had the same class of defect by a different route: _set_value type-checked without coercing, so a numeric subclass overriding __str__ satisfied the check and injected when formatted, and between never reached that check at all.

The library is the exposed surface. The MCP server escaped text at its own boundary and was never affected, but any caller building a filter from untrusted input was.

Changes

Text equality replaces the quote rather than escaping it

Escaping does not work here, and the reason decides the whole design. RediSearch tokenization is symmetric: punctuation separates tokens on both sides of the wire, and a backslash joins the separator into the term. Since RedisVL writes documents unescaped, @f:("say \"hi\" now") asks for a term containing a quote that no document ever stored. On == that matches nothing. On != the unmatchable phrase makes the negation match everything, so an exclusion filter silently stops excluding, which is strictly worse than the injection it would have prevented.

Equality and inequality therefore replace " with a space, which is what the tokenizer left at that position anyway. A value of nothing but quotes renders *, for the same reason. Only the quote is replaced: a trailing backslash does not terminate a phrase either, since @f:("x\") parses as the term x\, and replacing it would break matching against documents that were written escaped.

crafted = 'hello") | (@secret:{leaked} @v:"nothing'
str((Tag("tenant") == "acme") & (Text("v") == crafted))
# before: (@tenant:{acme} @v:("hello") | (@secret:{leaked} @v:"nothing"))   root UNION, leaks
# after:  (@tenant:{acme} @v:("hello ) | (@secret:{leaked} @v: nothing"))   root INTERSECT, 0 hits

% continues to interpolate its value raw, which is what makes *, %% and | work, and its docstring now says so and points a caller holding untrusted input at ==. Text lists that one raw operator rather than deriving the quoted ones from the templates, so a new operator is contained unless it opts out. That covers one added by a subclass, which a set of quoted operators computed in the base class body would miss.

Numeric values are coerced, not merely type-checked

All seven Num operators now pass their value through _coerce_numeric after the type check. Coercion is the guard rather than the check: every numeric value is formatted into the query string, so int() and float() returning builtins is what strips a subclass's __str__ override. numbers.Real keeps numpy scalars working, which a concrete (int, float) check would have rejected. NaN is refused, since @field:[nan ...] is a query RediSearch rejects outright.

class Hostile(float):
    def __str__(self): return "5] | (@secret:{leaked}) @r:[-inf +inf"

str(Num("r") <= Hostile(5))
# before: @r:[-inf 5] | (@secret:{leaked}) @r:[-inf +inf]
# after:  @r:[-inf 5]

The unreachable BETWEEN entry in OPERATOR_MAP, its branch in Num.__str__, and the tuple in SUPPORTED_VAL_TYPES that only that branch consumed are deleted. Nothing assigned FilterOperator.BETWEEN to an instance, which is why between looked like it was bypassing machinery that worked.

The MCP locked-filter backstop learns about quoted phrases

_reject_escapable_filter counted bracket depth without knowing quotes exist. That was safe only while the filter boundary escaped every parenthesis and brace in a text value; without it the guard refused ordinary values such as smiley :) and a[b. It now skips a quoted phrase wholesale, because inside quotes a parenthesis is literal text and a | is a separator rather than a union. Ending the skip at the first unescaped quote is what keeps it failing closed: a value that did reach the rendering raw ends the phrase there, and the injected remainder is counted as before.

Two near-identical scanners collapse into _find_unescaped, which takes a bound. Without one the range branch pays for the whole remaining string on every [, which made the walk quadratic in the number of spans. Measured at 14 seconds of blocking CPU on a 1 MB rendering, that is enough to stall the server's event loop for every concurrent request.

The MCP text boundary stops escaping eq, ne and in

Required rather than cleanup: with Text also handling the value it would be treated twice and mangled. This also fixes a live defect, because the boundary escaper escaped the space and so rendered any multi-word value as a literal that matched nothing.

# {"field": "job", "op": "eq", "value": "senior engineer"}
# before: @job:("senior\ engineer")   matches nothing
# after:  @job:("senior engineer")

like keeps its boundary escaping, since the library leaves % raw.

  • docs/concepts/mcp.md no longer attributes text escaping to the filter boundary, and no longer names a parenthesis as able to close a clause.
  • The filtering user guide states that == and != match a literal phrase while % takes a raw pattern.

Tests

Four test functions in tests/unit/test_filter.py were defined twice, so the earlier definition of each never collected. Num.between with its inclusive= variants and Tag != <falsy> had coverage only there and are ported forward; two of the dead assertions were simply wrong, which is independent evidence they had never run.

Because the existing suite passed identically with the fix applied and reverted, each guard is mutation-checked by reverting it alone. Fourteen mutations are each caught by a named test. The integration suite gains one row asserting that a quote-bearing value on != excludes its document, which is the one claim no rendering assertion can make: escaping the quote instead returns the whole corpus.

Notes

Two behaviour changes reach existing callers. A Text == or != value containing a " now renders differently, and a value of only quotes selects everything instead of producing a malformed query. Num.between now raises TypeError on a str or Decimal endpoint, both of which rendered successfully before, since neither is a numbers.Real; the Raises docstring names them explicitly. No working caller regresses on any other input.

The one change that widens a result set is the MCP boundary fix, and it stays inside the lock: a structured text eq, ne or in filter whose value contains a space went from matching nothing to matching correctly. Operators running a profile with a locked multi-word text filter should expect that scope to start applying.

Timestamp.between overrides Num.between and calls the formatter directly, so it does not inherit the endpoint coercion. It is not injectable, because _convert_to_timestamp raises on a non-ISO string, but between(None, None) renders @ts:[None None] and NaN renders @ts:[nan ...], both server-side syntax errors. Deliberately out of scope and filed separately.

Tag value handling is deliberately untouched here. A separate gap in it is addressed by #717, so folding it in would only couple two independently reviewable changes.

Three changes to the filter layer are in flight together: this one, #717 for tag values, and #720 for filter clauses in vector query strings. All three are independent, all target main, and none needs to merge before another on functional grounds. #717 merges cleanly against both of the others. This branch and #720 both touch redisvl/query/filter.py and tests/unit/test_filter.py, in different regions. git merge-tree reports a single conflict, in the tests/unit/test_filter.py import block, which both changes rewrote; whichever merges second resolves it in one hunk. The two fixes are complementary and neither substitutes for the other: #720 stops a legitimately built union binding across an intersection at assembly time, and verified against a live server, its parenthesisation does not contain the injection this change fixes: as a KNN pre-filter the crafted value still explains as a root UNION.

Next Steps

  1. Merge in the order fix: escape the union operator in tag filter values #717, this PR, fix: parenthesise filter clauses in vector query strings #720, so the smallest change lands first and the one conflict is resolved once. Any order works; only the conflict below is order-sensitive.
  2. Resolve the tests/unit/test_filter.py import conflict against fix: parenthesise filter clauses in vector query strings #720, whichever of the two lands second.
  3. File the deferred defects as issues: Timestamp.between endpoint validation, and TokenEscaper.escaped_chars_no_wildcard_re compiling from the class constant and so ignoring an injected pattern.
  4. Confirm on a live index that structured MCP text filters with multi-word values now return results, for any deployed profile that locks one.

Release Notes

Query filter values are now contained in the clause they render into. A value containing a double quote could previously close its own clause and have the remainder parsed as RediSearch syntax, so a filter built from untrusted input could be widened past the scope it was meant to enforce. Upgrade if any filter value in your application originates from user input.

One backwards-incompatible change comes with it. Num(field).between(start, end) now raises TypeError unless both bounds are real numbers. A str or Decimal bound rendered successfully before and now fails, so a caller passing a numeric string from a request body, or a Decimal from a database column, has to coerce it first. int, float and numpy scalars are unaffected.

One fix moves result counts rather than breaking anything. Text(field) != value with a quote-bearing value previously matched every document, so the filter excluded nothing at all; it now excludes correctly.

For the MCP server, structured text filters carrying a multi-word value were escaped in a way that matched nothing. eq, ne and in now work as intended, and like is unchanged.

Text(field) % pattern is deliberately unchanged and still interpolates its value raw, so do not pass untrusted input to it.

`Text.__str__` interpolated the caller's value straight into its operator
templates, so a value carrying a `"` could terminate the quoted phrase it was
meant to sit inside and append arbitrary RediSearch syntax. Under DIALECT 2 an
injected `|` binds looser than the implicit space-AND, so it lifts to the root
of the parse tree and any surrounding filter stops constraining the query.
`Num` had the same defect by a different route: `_set_value` type-checks
without coercing, so a numeric subclass overriding `__str__` satisfies the
check and injects when formatted, and `between` never reached that check at all.

Text equality and inequality now replace `"` with a space. Escaping it does not
work, because escaping is symmetric: a backslash joins the separator into the
term, so `@f:("say \"hi\" now")` asks for a term containing a quote and RedisVL
writes documents unescaped. On `==` that matches nothing; on `!=` the
unmatchable phrase makes the negation match everything, so an exclusion filter
silently stops excluding. A value of nothing but quotes now renders `*`, for
the same reason. Only the quote is replaced: a trailing backslash does not
escape the closing quote — `@f:("x\")` parses as the term `x\` — and replacing
it would break matching against documents that were written escaped.

`%` still interpolates raw, which is what makes `*`, `%%` and `|` work, and its
docstring now says so. `Text` lists the one raw operator rather than deriving
the quoted ones from the templates, so a new operator, including one added by a
subclass, is contained unless it opts out.

All seven `Num` operators now coerce their value to a builtin `int` or `float`
after the type check, which is what defeats the hostile-`__str__` case;
`numbers.Real` keeps numpy scalars working. `NaN` is rejected, since it renders
a query RediSearch refuses. The unreachable `BETWEEN` `OPERATOR_MAP` entry, its
`__str__` branch, and the `tuple` in `SUPPORTED_VAL_TYPES` that only that branch
consumed are all deleted.

The MCP locked-filter backstop needed a matching change. It counted bracket
depth without knowing quotes exist, which was only safe while the filter
boundary escaped every paren and brace in a text value; without that it refused
ordinary values such as `smiley :)` and `a[b`. It now skips a quoted phrase
wholesale, ending at the first unescaped quote so a value that did reach the
rendering raw is still counted. Two near-identical scanners collapse into
`_find_unescaped`, which takes a bound — without one the range branch pays for
the whole remaining string on every `[`, which measured 14s of blocking CPU on
a 1 MB rendering.

The boundary escaping in `redisvl/mcp/filters.py` is removed for eq/ne/in,
which is required rather than cleanup: with `Text` also handling the value it
would be treated twice and mangled. That also fixes a live defect, since the
escaper escaped the space and so rendered any multi-word value as a literal
that matched nothing. `like` keeps its boundary escaping, because the library
leaves `%` raw.
@vishal-bala vishal-bala added the auto:patch Increment the patch version when merged label Sep 3, 2026
@vishal-bala
vishal-bala changed the base branch from main to fix/escape-pipe-in-tag-values September 3, 2026 14:58
@vishal-bala
vishal-bala marked this pull request as ready for review September 3, 2026 15:32

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ 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 b239d96. Configure here.

escaped = True
break
position = end + 1
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locked filter rejects trailing backslash text

Medium Severity

For tools with a locked filter, _reject_escapable_filter can refuse caller text eq/ne values that end with \. Text renders those as a quoted phrase with the backslash preserved before the closing quote, but the phrase scanner skips a backslash and the next character as an escape pair, so it misses the real closing quote and treats the phrase as unterminated.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b239d96. Configure here.

@limjoobin limjoobin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The "replace rather than escape" reasoning for " and coercing instead of trusting isinstance are both right, and the integration test on != is the only thing that could catch the unmatchable-phrase case.

Just one thing to point out, not a blocker for this PR: Geo has the same containment gap and isn't covered here. GeoSpec.__init__ validates only unit, so longitude and latitude reach Geo.__str__'s %s slots unvalidated, and a string coordinate can close its clause and inject a |.

@limjoobin

limjoobin commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opened #723 for the Geo case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants