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`. 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