From 5d5a45565ef7f6b7029b61b212b1e27283f3a3d2 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Mon, 24 Aug 2026 16:56:54 +0200 Subject: [PATCH 1/2] fix: escape glob metacharacters in SCAN match patterns SCAN/KEYS MATCH patterns are glob-style, but several call sites interpolated a caller-supplied literal straight into one. A cache name, index prefix or route name containing *, ?, [ or \ therefore produced a pattern matching keys the caller did not own -- and on the destructive paths those keys were then deleted or rewritten. Verified on Redis 8.4.5: SemanticCache(name="cache[ab]").clear() deletes every entry under cachea: and cacheb: while leaving its own keys intact, silently and with no error. The same shape reaches delete_route_references and the migration executor's RENAME/DUMP-RESTORE-DEL path. Patterns are now built with match_pattern(), which escapes each literal segment and appends the trailing glob. Exposing a builder rather than a bare escaper is deliberate: escaping stops being a step a future call site can forget, which matters because no lint rule can express the invariant (ruff and flake8 are unconfigured, mypy is not strict, and patterns are consumed several call sites away from where they are built). Covered sites: BaseCache.clear/aclear, SemanticRouter._route_pattern, build_scan_match_patterns (and through it executor, async_executor, validation, async_validation), and both planners' key sampling. Escaping rather than rejecting such names, because rejecting is a breaking change that orphans keys: a deployment whose cache is already named "cache[ab]" could no longer construct the object to clean up after itself. Tests split by what can actually answer each question. That RedisVL emits the right pattern is an exact-string assertion, needing no matcher. That Redis then interprets it as intended is asked of a real server: the existing cache regression in test_embedcache.py, and a new migration and router prefix case in test_migration_v1.py. No glob matcher is reimplemented anywhere, so nothing can drift from Redis. Two pre-existing issues in the same neighbourhood are deliberately left alone: BaseCache.clear/aclear never advance a Mapping cursor on Redis Cluster (fixed separately by replacing the hand-rolled loop with scan_iter), and async_planner appends the key separator where the other three sites do not, so it samples a narrower key set than the index covers. --- redisvl/extensions/cache/base.py | 9 +- redisvl/extensions/router/semantic.py | 11 ++- redisvl/migration/async_planner.py | 11 ++- redisvl/migration/planner.py | 7 +- redisvl/migration/utils.py | 3 +- redisvl/utils/utils.py | 30 ++++++ tests/integration/test_embedcache.py | 52 +++++++++++ tests/integration/test_migration_v1.py | 31 +++++- tests/unit/test_scan_pattern_escaping.py | 114 +++++++++++++++++++++++ tests/unit/test_utils.py | 21 +++++ 10 files changed, 273 insertions(+), 16 deletions(-) create mode 100644 tests/unit/test_scan_pattern_escaping.py diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 8ff3566db..7063be886 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -12,6 +12,7 @@ from redisvl.redis.connection import RedisConnectionFactory from redisvl.types import AsyncRedisClient, SyncRedisClient +from redisvl.utils.utils import match_pattern class BaseCache: @@ -184,12 +185,12 @@ async def aexpire(self, key: str, ttl: int | None = None) -> None: def clear(self) -> None: """Clear the cache of all keys.""" client = self._get_redis_client() - prefix = self._get_prefix() + pattern = match_pattern(self._get_prefix()) # Scan for all keys with our prefix cursor = 0 # Start with cursor 0 while True: - cursor_int, keys = client.scan(cursor=cursor, match=f"{prefix}*", count=100) # type: ignore + cursor_int, keys = client.scan(cursor=cursor, match=pattern, count=100) # type: ignore if keys: client.delete(*keys) if cursor_int == 0: # Redis returns 0 when scan is complete @@ -206,13 +207,13 @@ def clear(self) -> None: async def aclear(self) -> None: """Async clear the cache of all keys.""" client = await self._get_async_redis_client() - prefix = self._get_prefix() + pattern = match_pattern(self._get_prefix()) # Scan for all keys with our prefix cursor = 0 # Start with cursor 0 while True: cursor_int, keys = await client.scan( - cursor=cursor, match=f"{prefix}*", count=100 + cursor=cursor, match=pattern, count=100 ) # type: ignore if keys: await client.delete(*keys) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 16472615b..8da9bf4db 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -26,7 +26,12 @@ from redisvl.redis.utils import convert_bytes, hashify, make_dict from redisvl.types import SyncRedisClient from redisvl.utils.log import get_logger -from redisvl.utils.utils import deprecated_argument, model_to_dict, scan_by_pattern +from redisvl.utils.utils import ( + deprecated_argument, + match_pattern, + model_to_dict, + scan_by_pattern, +) from redisvl.utils.vectorize.base import BaseVectorizer from redisvl.utils.vectorize.text.huggingface import HFTextVectorizer @@ -336,9 +341,9 @@ def _route_pattern(index: SearchIndex, route_name: str) -> str: # Normalize prefix to avoid double separators prefix = index.prefix.rstrip(sep) if sep and index.prefix else index.prefix if prefix: - return f"{prefix}{sep}{route_name}{sep}*" + return match_pattern(prefix, sep, route_name, sep) else: - return f"{route_name}{sep}*" + return match_pattern(route_name, sep) def _add_routes(self, routes: list[Route]): """Add routes to the router and index. diff --git a/redisvl/migration/async_planner.py b/redisvl/migration/async_planner.py index 6c75efda2..5c24853f8 100644 --- a/redisvl/migration/async_planner.py +++ b/redisvl/migration/async_planner.py @@ -13,6 +13,7 @@ from redisvl.redis.connection import supports_svs_async from redisvl.schema.schema import IndexSchema from redisvl.types import AsyncRedisClient +from redisvl.utils.utils import match_pattern class AsyncMigrationPlanner: @@ -266,17 +267,19 @@ async def _async_sample_keys( for prefix in prefixes: if len(key_sample) >= self.key_sample_limit: break + # NOTE: appending key_separator diverges from the sync planner and + # samples a narrower key set than the index covers. Pre-existing. if prefix == "": - match_pattern = "*" + scan_match = "*" elif prefix.endswith(key_separator): - match_pattern = f"{prefix}*" + scan_match = match_pattern(prefix) else: - match_pattern = f"{prefix}{key_separator}*" + scan_match = match_pattern(prefix, key_separator) cursor: int = 0 while True: cursor, keys = await client.scan( cursor=cursor, - match=match_pattern, + match=scan_match, count=max(self.key_sample_limit, 10), ) for key in keys: diff --git a/redisvl/migration/planner.py b/redisvl/migration/planner.py index 4c09fe04c..1e407c129 100644 --- a/redisvl/migration/planner.py +++ b/redisvl/migration/planner.py @@ -18,6 +18,7 @@ ) from redisvl.redis.connection import supports_svs from redisvl.schema.schema import IndexSchema +from redisvl.utils.utils import match_pattern class MigrationPlanner: @@ -657,18 +658,18 @@ def _sample_keys( if len(key_sample) >= self.key_sample_limit: break if prefix == "": - match_pattern = "*" + scan_match = "*" else: # Use literal prefix + glob, matching Redis Search PREFIX # semantics (pure string-prefix match). Do NOT insert the # key_separator — a PREFIX of "doc" must match "doc:1", # "doca:1", etc., exactly like FT.CREATE does. - match_pattern = f"{prefix}*" + scan_match = match_pattern(prefix) cursor = 0 while True: cursor, keys = client.scan( cursor=cursor, - match=match_pattern, + match=scan_match, count=max(self.key_sample_limit, 1000), ) for key in keys: diff --git a/redisvl/migration/utils.py b/redisvl/migration/utils.py index 0f317daba..14d5c5ba7 100644 --- a/redisvl/migration/utils.py +++ b/redisvl/migration/utils.py @@ -21,6 +21,7 @@ from redisvl.redis.connection import RedisConnectionFactory from redisvl.schema.schema import IndexSchema from redisvl.utils.log import get_logger +from redisvl.utils.utils import match_pattern logger = get_logger(__name__) @@ -104,7 +105,7 @@ def build_scan_match_patterns(prefixes: List[str], key_separator: str) -> List[s # (pure string-prefix match). Do NOT insert the key_separator — a # PREFIX of "doc" must match "doc:1", "doca:1", etc., exactly like # FT.CREATE does. - patterns.add(f"{prefix}*") + patterns.add(match_pattern(prefix)) return sorted(patterns) diff --git a/redisvl/utils/utils.py b/redisvl/utils/utils.py index 85f74397c..1c1dc2e8a 100644 --- a/redisvl/utils/utils.py +++ b/redisvl/utils/utils.py @@ -282,6 +282,36 @@ def norm_l2_distance(value: float) -> float: return 1 / (1 + value) +# Redis glob specials (https://redis.io/docs/latest/commands/keys/). ``]``, ``^`` +# and ``-`` are absent deliberately: they only bite inside a ``[...]`` class, +# which can never open once ``[`` is escaped. +_GLOB_METACHARACTERS = frozenset("\\*?[") + + +def match_pattern(*segments: str) -> str: + """Build a ``SCAN``/``KEYS`` ``MATCH`` pattern from literal key segments. + + Every segment -- prefix, separator, cache or route name -- is escaped, so a + name containing glob metacharacters matches its own keys instead of someone + else's. Build patterns here rather than interpolating names into a pattern. + + Args: + *segments (str): Literal parts of the key prefix, in order. Passing none + (or only empty ones) yields ``"*"``. + + Returns: + str: A pattern matching exactly the keys starting with those segments. + """ + return ( + "".join( + "\\" + char if char in _GLOB_METACHARACTERS else char + for segment in segments + for char in segment + ) + + "*" + ) + + def scan_by_pattern( redis_client: Redis, pattern: str, diff --git a/tests/integration/test_embedcache.py b/tests/integration/test_embedcache.py index 28fcfc253..184efe408 100644 --- a/tests/integration/test_embedcache.py +++ b/tests/integration/test_embedcache.py @@ -773,3 +773,55 @@ def test_large_batch_operations(cache): assert not cache.exists_by_key(key) else: assert cache.exists_by_key(key) + + +@pytest.fixture +def colliding_caches(redis_url, redis_test_name): + """Three caches whose names collide under an unescaped SCAN glob. + + ``base[ab]:*`` as a glob matches ``basea:``/``baseb:`` but not + ``base[ab]:``, so clearing the bracketed cache used to wipe the other two. + """ + base = redis_test_name("glob_collision") + caches = { + name: EmbeddingsCache(name=name, redis_url=redis_url) + for name in (f"{base}[ab]", f"{base}a", f"{base}b") + } + yield f"{base}[ab]", caches + for cache_instance in caches.values(): + cache_instance.clear() + + +def test_clear_does_not_delete_caches_colliding_under_glob(colliding_caches): + target_name, caches = colliding_caches + for name, cache_instance in caches.items(): + cache_instance.set(content=name, model_name="test-model", embedding=[0.1, 0.2]) + assert cache_instance.exists(name, "test-model") + + caches[target_name].clear() + + assert not caches[target_name].exists(target_name, "test-model") + for name, cache_instance in caches.items(): + if name != target_name: + assert cache_instance.exists( + name, "test-model" + ), f"clear() on {target_name!r} deleted entries belonging to {name!r}" + + +@pytest.mark.asyncio +async def test_aclear_does_not_delete_caches_colliding_under_glob(colliding_caches): + target_name, caches = colliding_caches + for name, cache_instance in caches.items(): + await cache_instance.aset( + content=name, model_name="test-model", embedding=[0.1, 0.2] + ) + assert await cache_instance.aexists(name, "test-model") + + await caches[target_name].aclear() + + assert not await caches[target_name].aexists(target_name, "test-model") + for name, cache_instance in caches.items(): + if name != target_name: + assert await cache_instance.aexists( + name, "test-model" + ), f"aclear() on {target_name!r} deleted entries belonging to {name!r}" diff --git a/tests/integration/test_migration_v1.py b/tests/integration/test_migration_v1.py index 08391d97f..a0a2a9d0d 100644 --- a/tests/integration/test_migration_v1.py +++ b/tests/integration/test_migration_v1.py @@ -5,7 +5,7 @@ from redisvl.index import SearchIndex from redisvl.migration import MigrationExecutor, MigrationPlanner, MigrationValidator from redisvl.migration.utils import load_migration_plan, schemas_equal -from redisvl.redis.utils import array_to_buffer +from redisvl.redis.utils import array_to_buffer, convert_bytes def test_drop_recreate_plan_apply_validate_flow(redis_url, worker_id, tmp_path): @@ -128,3 +128,32 @@ def test_drop_recreate_plan_apply_validate_flow(redis_url, worker_id, tmp_path): finally: live_index = SearchIndex.from_existing(index_name, redis_url=redis_url) live_index.delete(drop=True) + + +def test_scan_patterns_select_only_the_named_prefix(client, redis_test_name): + """Redis, not a reimplementation of its glob, decides what a pattern matches. + + Exercises the patterns the migration and router builders emit for a prefix + carrying glob metacharacters: unescaped, ``base[ab]*`` selects ``basea`` + and ``baseb`` and misses ``base[ab]`` entirely. + """ + from redisvl.migration.utils import build_scan_match_patterns + from redisvl.utils.utils import match_pattern + + base = redis_test_name("glob_prefix") + owned = {f"{base}[ab]:1", f"{base}[ab]:2"} + others = {f"{base}a:1", f"{base}b:1"} + for key in owned | others: + client.set(key, "1") + + (pattern,) = build_scan_match_patterns([f"{base}[ab]"], ":") + assert set(convert_bytes(list(client.scan_iter(match=pattern)))) == owned + + route_pattern = match_pattern(f"{base}[ab]", ":", "route", ":") + client.set(f"{base}[ab]:route:ref", "1") + client.set(f"{base}a:route:ref", "1") + assert convert_bytes(list(client.scan_iter(match=route_pattern))) == [ + f"{base}[ab]:route:ref" + ] + + client.delete(*(owned | others), f"{base}[ab]:route:ref", f"{base}a:route:ref") diff --git a/tests/unit/test_scan_pattern_escaping.py b/tests/unit/test_scan_pattern_escaping.py new file mode 100644 index 000000000..867c9db72 --- /dev/null +++ b/tests/unit/test_scan_pattern_escaping.py @@ -0,0 +1,114 @@ +"""Every SCAN pattern built from a caller-supplied name escapes it. + +Hermetic: these capture the pattern handed to Redis instead of needing a +server. ``match_pattern`` itself is tested in ``test_utils.py``. +""" + +import asyncio + +import pytest + +from redisvl.extensions.cache.base import BaseCache +from redisvl.extensions.router.semantic import SemanticRouter +from redisvl.migration.utils import build_scan_match_patterns + +# Unescaped, "cache[ab]" matches "cachea"/"cacheb" but not itself. +COLLIDING_NAME = "cache[ab]" + + +class RecordingClient: + """Records SCAN patterns and returns no keys.""" + + def __init__(self): + self.patterns: list[str] = [] + + def scan(self, cursor=0, match=None, count=None): + self.patterns.append(match) + return 0, [] + + def delete(self, *keys): # pragma: no cover - no keys are ever returned + raise AssertionError("delete() reached with no scan hits") + + +class AsyncRecordingClient(RecordingClient): + async def scan(self, cursor=0, match=None, count=None): # type: ignore[override] + self.patterns.append(match) + return 0, [] + + +class StubIndex: + """Only the attributes ``_route_pattern`` reads.""" + + def __init__(self, prefix, key_separator=":"): + self.prefix = prefix + self.key_separator = key_separator + + +def test_cache_clear_escapes_name(): + cache = BaseCache(name=COLLIDING_NAME) + client = RecordingClient() + cache._get_redis_client = lambda: client # type: ignore[method-assign] + + cache.clear() + + assert client.patterns == ["cache\\[ab]:*"] + + +@pytest.mark.asyncio +async def test_cache_aclear_escapes_name(): + cache = BaseCache(name=COLLIDING_NAME) + client = AsyncRecordingClient() + + async def _get_client(): + return client + + cache._get_async_redis_client = _get_client # type: ignore[method-assign] + + await cache.aclear() + + assert client.patterns == ["cache\\[ab]:*"] + + +@pytest.mark.parametrize( + "prefix, route_name, expected", + [ + ("router[ab]", "route", "router\\[ab]:route:*"), + ("router", "route[ab]", "router:route\\[ab]:*"), + ("", "route[ab]", "route\\[ab]:*"), + ("router", "route", "router:route:*"), + ], +) +def test_route_pattern_escapes_prefix_and_route(prefix, route_name, expected): + assert SemanticRouter._route_pattern(StubIndex(prefix), route_name) == expected + + +def test_build_scan_match_patterns_escapes_prefixes(): + assert build_scan_match_patterns(["doc[ab]", "plain"], ":") == [ + "doc\\[ab]*", + "plain*", + ] + + +@pytest.mark.parametrize("flavour", ["sync", "async"]) +def test_planner_sample_keys_escapes_prefix(flavour): + """Asserts only the escaping; the two planners differ on the separator.""" + if flavour == "sync": + from redisvl.migration.planner import MigrationPlanner + + planner = MigrationPlanner.__new__(MigrationPlanner) + planner.key_sample_limit = 10 + client = RecordingClient() + planner._sample_keys(client=client, prefixes=["doc[ab]"], key_separator=":") + else: + from redisvl.migration.async_planner import AsyncMigrationPlanner + + planner = AsyncMigrationPlanner.__new__(AsyncMigrationPlanner) + planner.key_sample_limit = 10 + client = AsyncRecordingClient() + asyncio.run( + planner._async_sample_keys( + client=client, prefixes=["doc[ab]"], key_separator=":" + ) + ) + + assert client.patterns == [f"doc\\[ab]{'' if flavour == 'sync' else ':'}*"] diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index d7c1e2f0e..692d2eb95 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -19,6 +19,7 @@ deprecated_class, deprecated_function, lazy_import, + match_pattern, norm_cosine_distance, ) @@ -927,3 +928,23 @@ def test_keys_share_hash_tag_multiple_braces(): keys = ["prefix:{tag1}:middle:{tag2}:key1", "prefix:{tag1}:middle:{tag2}:key2"] # Should use the first hash tag found assert _keys_share_hash_tag(keys) is True + + +@pytest.mark.parametrize( + "segments, expected", + [ + (("plain",), "plain*"), + (("with:separators:only",), "with:separators:only*"), + (("cache[ab]",), "cache\\[ab]*"), + (("star*",), "star\\**"), + (("question?",), "question\\?*"), + (("back\\slash",), "back\\\\slash*"), + (("all*?[]\\",), "all\\*\\?\\[]\\\\*"), + # Separators are literals too. + (("router", ":", "route[ab]", ":"), "router:route\\[ab]:*"), + ((), "*"), + (("",), "*"), + ], +) +def test_match_pattern(segments, expected): + assert match_pattern(*segments) == expected From edc99b3b80df538b90d853b913880086d42253a5 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Mon, 24 Aug 2026 16:57:10 +0200 Subject: [PATCH 2/2] docs: ACL requirements for cache reads and glob key patterns Two things that surprise anyone assembling a least-privilege credential, both measured on Redis 8.4.5 rather than inferred. A cache read is not read-only: with a TTL configured, every hit refreshes the matched entries' TTL, so the read path issues EXPIRE, which is in @write and not @read. A lookup-only credential therefore fails on a cache hit rather than on the write that populated the entry. Granting the command is only half of it -- ACL DRYRUN shows +expire under a read-only key pattern such as %R~llmcache:* is denied on the key, and that is the shape the Key permissions table presents as sufficient for querying. Placed in "Roles built from @read and @write", where a reader assembling exactly that role will be. ACL key patterns are also glob-style, matched by the same engine as SCAN MATCH, so they carry the same metacharacter trap: ~cache[ab]:* grants cachea:1 and cacheb:1 and denies the literal cache[ab]:1. --- docs/user_guide/installation.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index ce70fe8a5..4954b2623 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -226,6 +226,18 @@ RedisSearchError: Error while fetching llmcache index info: User has no permissions to run the 'FT.INFO' command ``` +Reading a cache is not read-only either. When a cache has a TTL configured — at construction or later via `set_ttl()` — every read that hits refreshes the matched entries' TTL, so it issues `EXPIRE`. That includes `SemanticCache.check()`, every `EmbeddingsCache` getter, their async equivalents, and the reads a vectorizer performs for you when constructed with `cache=`. `EXPIRE` is in `@write`, not `@read` (measured with `ACL CAT write`), so a lookup-only credential fails on a cache *hit* rather than on the write that populated it. + +Granting the command is only half of it, because the key patterns must permit writes too. Measured on 8.4.5 with `ACL DRYRUN EXPIRE llmcache:abc 60`: + +| Rules | Result | +|---|---| +| `+@read ~llmcache:*` | `no permissions to run the 'expire' command` | +| `+@read +expire ~llmcache:*` | Permitted | +| `+@read +expire %R~llmcache:*` | `no permissions to access the 'llmcache:abc' key` | + +So `%R~llmcache:*` — the read-only shape the [Key permissions](#key-permissions) table below presents as sufficient for querying — is denied on the key, not the command. `+@read +@write` is unaffected, as is a cache with no TTL. See [Cache LLM Responses](03_llmcache.ipynb) for the refresh behaviour itself, including that `set_ttl()` starts adding TTLs to entries stored without one. + ### "no permissions to run the 'FT.INFO' command" RedisVL does not guess its way around this. A credential that cannot ask whether the index exists also cannot create one, so there is nothing useful to infer — instead, tell RedisVL that the index is already there: @@ -281,6 +293,8 @@ Measured on 8.4.5 against an index prefixed `doc:`, with the command categories Partial overlap is worth emphasising: it fails exactly like no overlap at all, rather than returning the subset you can read. `FT.CREATE` is not checked this way, so a credential can create an index it is then unable to query. +Key patterns are glob-style, matched by the same engine as `SCAN MATCH`, so metacharacters in a prefix do not mean what they look like. Measured on 8.4.5, `~cache[ab]:*` grants `cachea:1` and `cacheb:1` while *denying* the literal key `cache[ab]:1`; escaped as `~cache\[ab]:*` it does the reverse. Escape `*`, `?`, `[` and `\` in a prefix, and confirm the rule with `ACL DRYRUN`. + `create_index=False` does not help here — the very commands it lets you avoid are joined by the ones it cannot, so widen the key patterns instead. Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO`. That command is tagged `@connection` and `@slow`, and belongs to neither `@read` nor `@write`, so a rule built up from those categories never grants it. A credential that cannot run it still connects: identification only populates the `lib-name` field that `CLIENT LIST` and `CLIENT INFO` display, so a refusal is ignored (and logged, if you have configured logging at debug level). Grant `+client|setinfo` if you want RedisVL to appear as the connecting library there — note that this labels the connection RedisVL opens, while redis-py labels the rest of the pool as plain `redis-py`.