From 6cf2a6045ac52d8a6fd97fe136f937ec348cbc81 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 10:20:29 +0530 Subject: [PATCH 01/12] UN-4123 [FIX] A credential-free REDIS_URL no longer connects anonymously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In URL mode the resolved password was never passed to the client: credentials had to be embedded in the URL, and a {prefix}PASSWORD set beside a credential-free URL was silently ignored. The client connected ANONYMOUSLY and the endpoint answered NOAUTH on the first command, which reads as a broken server rather than a dropped password. This was not exercised by the live managed-Redis testing on UN-4123, because both configurations tried there avoided it: the URL-mode run embedded the password in the URL, and the discrete run had no URL at all. The gap sits in the third combination, which is the one worth recommending. WHY IT IS WORTH RECOMMENDING. A URL ends up in places a password should not: the endpoint helper's error messages, ArgoCD's ComparisonError condition, and — under ESO — the ExternalSecret's spec.target.template.data, which is not a Secret and is not redacted. All three were raised in review of the chart-side PR, and all three exist because the password is in a URL. Keeping it in its own key closes the class rather than patching each place the URL is printed; this change is what makes that configuration usable. Credentials FILL A GAP the URL leaves and never override one. ConnectionPool.from_url ends with kwargs.update(url_options), so an existing rediss://:@host URL keeps behaving exactly as before — verified for both the :pw@ and user:pw@ spellings. GATED ON THE PASSWORD, with the username riding along. A username alone is not a credential, and values.yaml ships REDIS_USER: default: filling that in on its own would make redis-py send AUTH to the in-cluster server, which has none, turning a working default deployment into a failing one. I wrote that regression into the first version of this change and caught it by testing the chart's own default posture rather than by reading the diff. Six cases added, covering both URL spellings, the gap-filling case, the username pairing, the username-alone regression, and the legitimately anonymous server. Mutation-verified: removing the fallback fails two, ungating the username fails the third. The three sample.env recipes now show the password as a separate key, and say that an existing URL-embedded credential still works. Co-Authored-By: Claude Opus 5 --- backend/sample.env | 7 ++- runner/sample.env | 7 ++- .../src/unstract/core/cache/redis_client.py | 23 +++++++++ .../core/tests/test_redis_client_config.py | 51 +++++++++++++++++++ workers/sample.env | 7 ++- 5 files changed, 92 insertions(+), 3 deletions(-) diff --git a/backend/sample.env b/backend/sample.env index daea191349..cceb6796ef 100644 --- a/backend/sample.env +++ b/backend/sample.env @@ -60,7 +60,12 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true # alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # (6380 is an EXAMPLE, not a default — the TLS port is provider-specific: # Memorystore 6378, ElastiCache 6379, Azure Cache 6380. A wrong port # hangs the connection rather than erroring, so read it off the instance.) diff --git a/runner/sample.env b/runner/sample.env index efdf245a19..7745e49faa 100644 --- a/runner/sample.env +++ b/runner/sample.env @@ -55,7 +55,12 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true # alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # (6380 is an EXAMPLE, not a default — the TLS port is provider-specific: # Memorystore 6378, ElastiCache 6379, Azure Cache 6380. A wrong port # hangs the connection rather than erroring, so read it off the instance.) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index dbf30ba500..77c19c5c54 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -674,6 +674,8 @@ def _create_standalone_client( ssl_ca_certs=env.get("ssl_ca_certs"), ssl_cert_reqs=env.get("ssl_cert_reqs"), ssl_check_hostname=env.get("ssl_check_hostname"), + password=env.get("password"), + username=env.get("username"), ) logger.info( @@ -715,6 +717,8 @@ def _create_client_from_url( ssl_ca_certs: str | None, ssl_cert_reqs: str | None = None, ssl_check_hostname: bool | None = None, + password: str | None = None, + username: str | None = None, ) -> redis.Redis: """Build a client from a full Redis URL. @@ -760,6 +764,25 @@ def _create_client_from_url( kwargs["db"] = db_override parts = urlsplit(url) + # Credentials FILL A GAP the URL leaves; they never override one. + # ConnectionPool.from_url ends with kwargs.update(url_options), so a URL + # carrying credentials still wins. + # + # Without this, a URL written WITHOUT credentials plus a separately + # configured {prefix}PASSWORD connected ANONYMOUSLY — and setting the two + # apart is the configuration the on-prem recipe should be able to recommend, + # because it keeps the password out of a URL that gets printed into error + # messages, ArgoCD conditions and ExternalSecret templates. The endpoint + # answers NOAUTH on the first command, which reads as a broken server rather + # than a dropped password. + # Gated on the PASSWORD, and the username rides with it. A username alone is + # not a credential: values.yaml ships REDIS_USER: default, so filling it in + # on its own would make redis-py send AUTH to the in-cluster server, which + # has none — turning a working default deployment into a failing one. + if password and "@" not in parts.netloc: + kwargs["password"] = password + if username: + kwargs["username"] = username logger.info( "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", parts.hostname, diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 63a0d191f4..128216303f 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -909,3 +909,54 @@ def test_the_prefixs_own_db_still_wins(self, monkeypatch): monkeypatch.setenv("REDIS_DB", "3") monkeypatch.setenv("CACHE_REDIS_DB", "1") assert _kwargs(create_redis_client("CACHE_REDIS_"))["db"] == 1 + + +class TestUrlModeCredentials: + """A URL written without credentials must not connect anonymously. + + Keeping the password OUT of the URL is the safer configuration — a URL is + printed into error messages, ArgoCD conditions and ExternalSecret templates, + and a password in it travels to all three. That configuration is only usable + if the separately-supplied credential is actually applied. + """ + + def test_the_configured_password_fills_a_gap_the_url_leaves(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "s3cret" + + def test_a_url_carrying_credentials_still_wins(self, monkeypatch): + """ConnectionPool.from_url ends with kwargs.update(url_options).""" + monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "in-url" + + def test_a_username_password_pair_in_the_url_wins_too(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://u:pw@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + monkeypatch.setenv("REDIS_USER", "alice") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "pw" + assert kwargs["username"] == "u" + + def test_the_username_rides_with_the_password(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + monkeypatch.setenv("REDIS_USER", "alice") + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_username_alone_is_not_a_credential(self, monkeypatch): + """values.yaml ships REDIS_USER: default, and the in-cluster server has + no auth. Filling in a username on its own would make redis-py send AUTH + to it — turning a working default deployment into a failing one. + """ + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_USER", "default") + monkeypatch.setenv("REDIS_PASSWORD", "") + kwargs = _kwargs(create_redis_client()) + assert kwargs.get("username") is None + assert kwargs.get("password") is None + + def test_no_credentials_anywhere_stays_anonymous(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + assert _kwargs(create_redis_client()).get("password") is None diff --git a/workers/sample.env b/workers/sample.env index 375a62d2a8..2bd23853f0 100644 --- a/workers/sample.env +++ b/workers/sample.env @@ -81,7 +81,12 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # Managed / external Redis with TLS (UN-4123). Optional — unset keeps the plaintext # connection above. Either set REDIS_SSL=true beside REDIS_HOST/REDIS_PORT, or give a # full URL whose scheme carries TLS: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # A URL wins over the discrete vars. Auth is password-only as the built-in `default` # user (what a managed AUTH string is), so leave the username empty for a managed # endpoint. REDIS_SSL_CA_CERTS is only needed when the server's CA is not publicly From 66a60ccec60cd903adb441c7b940e8bc0b1da29f Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 10:29:38 +0530 Subject: [PATCH 02/12] UN-4123 [FIX] Carry the credential fallback to the other two Redis consumers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first commit fixed create_redis_client and stopped there, which left the recommendation it enables half true. unstract has THREE Redis consumers and a credential-free URL reached only one of them: create_redis_client backend, workers, runner, platform-service fixed build_socketio_redis_url kombu / Socket.IO ANONYMOUS Django cache LOCATION django-redis ANONYMOUS Both now fill the same gap, on the same terms: credentials are added only when the URL carries none, and a URL bearing them still wins. SOCKET.IO. kombu takes a URL and nothing else — no connection kwargs — so a separately supplied password cannot reach it any other way; the credentials go into the URL string itself. The password is then in that string, unavoidably. That is acceptable here and not in a values file: this URL is built in-process and handed straight to the client, rather than written into a manifest, a Secret template or an error message, which is the distinction the whole change is about. An anonymous publisher against an authenticated server does not fail loudly — Socket.IO events simply stop arriving. DJANGO CACHE. django-redis reads OPTIONS["PASSWORD"] and feeds it to ConnectionPool.from_url, which ends with kwargs.update(url_options) — so setting it is safe even when the URL has its own. It is still skipped when the URL carries credentials, because passing them twice invites the two to disagree. Five cases added. The settings harness now splices the urllib import from source instead of hand-writing it: the hand-written copy had to be remembered the moment settings started using another name from that module, and the symptom was a NameError in every case rather than one clear failure. That is the second time a hand-copied import in this harness has broken; the unstract.core import block was the first. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 10 +++++- .../tests/test_redis_settings_derivation.py | 36 +++++++++++++++++-- .../src/unstract/core/cache/redis_client.py | 34 ++++++++++++++++++ .../core/tests/test_redis_client_config.py | 15 ++++++++ 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 9f51483702..c92ad69069 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -13,7 +13,7 @@ import os import re from pathlib import Path -from urllib.parse import quote +from urllib.parse import quote, urlsplit import httpx from django.core.validators import URLValidator @@ -660,6 +660,14 @@ def filter(self, record): _cache_options["DB"] = _cache_db _cache_options["USERNAME"] = REDIS_USER _cache_options["PASSWORD"] = REDIS_PASSWORD + elif REDIS_PASSWORD and "@" not in urlsplit(_redis_url).netloc: + # ...unless the URL carries NO credentials, which is the configuration + # that keeps the password out of a string that gets printed. django-redis + # feeds OPTIONS["PASSWORD"] into ConnectionPool.from_url, which ends with + # kwargs.update(url_options) — so this fills the gap and a URL bearing + # credentials still wins. Without it this cache connects ANONYMOUSLY + # while create_redis_client beside it authenticates. + _cache_options["PASSWORD"] = REDIS_PASSWORD # Gated on the EFFECTIVE scheme, not the flag. In URL mode TLS is carried by # the URL (and its query string), so a plaintext REDIS_URL left behind while # REDIS_SSL=true would otherwise hand ssl_cert_reqs to a plain diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index 67d3aa0d7c..f00809b4f5 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -55,10 +55,17 @@ def _derive(**env: str) -> dict: # without it that branch raises NameError instead of logging, which is part of # why it went untested. ns: dict = {"__name__": "backend.settings.base"} + # The urllib import is spliced from source for the same reason as the + # unstract.core one: a hand-written copy must be remembered every time the + # settings module starts using another name from it, and the symptom is a + # NameError in every case rather than one clear failure. + urllib_start = source.index("from urllib.parse import ") + urllib_end = source.index("\n", urllib_start) + 1 + prelude = ( - "import logging\n" - "import os\n" - "from urllib.parse import quote\n" + source[imports_start:imports_end] + "import logging\nimport os\n" + + source[urllib_start:urllib_end] + + source[imports_start:imports_end] ) + source[defs_start:defs_end] import os as _os @@ -438,3 +445,26 @@ def test_no_database_var_is_parsed_with_a_bare_int(self): "unstract.core.cache.redis_client.parse_db so the backend and the " "workers agree on a malformed value." ) + + +class TestUrlModeCacheCredentials: + """django-redis reads OPTIONS["PASSWORD"]; the URL is not its only source. + + A URL written without credentials kept this cache ANONYMOUS while + create_redis_client beside it authenticated — one process, one endpoint, two + outcomes. OPTIONS["PASSWORD"] reaches ConnectionPool.from_url, which ends + with kwargs.update(url_options), so a URL bearing credentials still wins. + """ + + def test_the_password_fills_a_gap_the_url_leaves(self): + derived = _derive(REDIS_URL="rediss://h:6380/0", REDIS_PASSWORD="s3cret") + assert derived["CACHES"]["default"]["OPTIONS"]["PASSWORD"] == "s3cret" + + def test_a_url_carrying_credentials_is_left_alone(self): + """Passing it again risks the two disagreeing about which wins.""" + derived = _derive(REDIS_URL="rediss://:in-url@h:6380/0", REDIS_PASSWORD="s3cret") + assert "PASSWORD" not in derived["CACHES"]["default"]["OPTIONS"] + + def test_no_password_stays_anonymous(self): + derived = _derive(REDIS_URL="rediss://h:6380/0") + assert "PASSWORD" not in derived["CACHES"]["default"]["OPTIONS"] diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 77c19c5c54..75a77187e7 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -380,6 +380,37 @@ def _compose_redis_url(env: dict[str, Any]) -> str: return f"{scheme}://{credentials}{env['host']}:{env['port']}" +def apply_url_credentials(url: str, password: str | None, username: str | None) -> str: + """Put credentials into a URL that carries none, leaving one that does alone. + + kombu takes a URL and NOTHING else — no connection kwargs — so a password + supplied separately cannot reach it any other way. A URL written without + credentials would otherwise produce an anonymous publisher against an + authenticated server, and the Socket.IO events simply stop arriving. + + The password IS in the returned string, unavoidably. That is acceptable here + and not in a values file: this URL is built in-process and handed straight to + the client, rather than written into a manifest, a Secret template or an + error message. + """ + if not password: + return url + parts = urlsplit(url) + if "@" in parts.netloc: + return url + credentials = f"{quote(str(username), safe='')}:" if username else ":" + credentials += f"{quote(str(password), safe='')}@" + return urlunsplit( + ( + parts.scheme, + credentials + parts.netloc, + parts.path, + parts.query, + parts.fragment, + ) + ) + + def build_socketio_redis_url(env_prefix: str = "REDIS_") -> str: """Redis URL for a Socket.IO/kombu client, TLS settings included (UN-4123). @@ -415,6 +446,9 @@ def build_socketio_redis_url(env_prefix: str = "REDIS_") -> str: ca_certs = env.get("ssl_ca_certs") url = env["url"] or _compose_redis_url(env) + # _compose_redis_url already embeds them for the discrete path; a configured + # URL may not carry any, and kombu can read them from nowhere else. + url = apply_url_credentials(url, env.get("password"), env.get("username")) if not url.startswith(_TLS_SCHEME): return url diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 128216303f..3f17d47b0e 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -960,3 +960,18 @@ def test_a_username_alone_is_not_a_credential(self, monkeypatch): def test_no_credentials_anywhere_stays_anonymous(self, monkeypatch): monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") assert _kwargs(create_redis_client()).get("password") is None + + def test_the_socketio_url_gains_the_credentials_too(self, monkeypatch): + """Kombu takes a URL and nothing else, so a separately-supplied password + cannot reach it any other way. Without this the publisher is anonymous + against an authenticated server and Socket.IO events simply stop. + """ + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert "://:s3cret@h:6380" in build_socketio_redis_url() + + def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + url = build_socketio_redis_url() + assert "in-url" in url and "s3cret" not in url From f55b2ce6c48f4366973bb3cbddf262794f881424 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 10:41:54 +0530 Subject: [PATCH 03/12] UN-4123 [FIX] Two credential-resolution findings from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BOTH ARE IN MY OWN FALLBACK, and both are cases the fallback gets wrong rather than cases it misses. A URL CARRYING ONLY A USERNAME NO LONGER DROPS THE PASSWORD. The check keyed on "@" being absent from the netloc, which is not the same question: redis://alice@host carries an @ and no password. The fallback was skipped, the client got a username with no password, and could not authenticate. Keyed on urlsplit().password being None instead, which asks it directly. apply_url_credentials now uses the same test and keeps a username the URL already carries. CREDENTIALS RESOLVE AT THE URL'S OWN LEVEL, by the same rule the database already follows. A prefix that brought its OWN url may point at a DIFFERENT endpoint — an anonymous one, say — while the generic REDIS_PASSWORD is set for the primary. Letting that password reach across made the prefixed client send AUTH to a server with none, turning a working connection into a failing one. An INHERITED url is the same endpoint as the generic one, so the full fallback chain still applies there. For env_prefix "REDIS_" the two levels are the same variable and nothing changes. That the database rule and the credential rule now say the same sentence is the point: "at the URL's own level" is one idea an operator can hold, rather than two resolution orders that happen to differ. Five cases added, covering the username-only URL, the prefix that must not inherit, the prefix's own password, the inherited url that still should, and the Socket.IO builder following the same rule. Co-Authored-By: Claude Opus 5 --- .../src/unstract/core/cache/redis_client.py | 32 +++++++++++--- .../core/tests/test_redis_client_config.py | 44 +++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 75a77187e7..caaa712d32 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -396,8 +396,9 @@ def apply_url_credentials(url: str, password: str | None, username: str | None) if not password: return url parts = urlsplit(url) - if "@" in parts.netloc: + if parts.password is not None: return url + username = parts.username or username credentials = f"{quote(str(username), safe='')}:" if username else ":" credentials += f"{quote(str(password), safe='')}@" return urlunsplit( @@ -448,7 +449,7 @@ def build_socketio_redis_url(env_prefix: str = "REDIS_") -> str: url = env["url"] or _compose_redis_url(env) # _compose_redis_url already embeds them for the discrete path; a configured # URL may not carry any, and kombu can read them from nowhere else. - url = apply_url_credentials(url, env.get("password"), env.get("username")) + url = apply_url_credentials(url, env.get("url_password"), env.get("url_username")) if not url.startswith(_TLS_SCHEME): return url @@ -533,6 +534,21 @@ def _resolve_redis_env( # docstring, sample.env or the chart state. For env_prefix="REDIS_" the two # levels are the same variable, so this reads identically there. explicit_db = prefixed_db if own_url else (prefixed_db or generic_db) + # CREDENTIALS AT THE URL'S OWN LEVEL, by the same rule as the database above. + # A prefix that brought its OWN url may point at a DIFFERENT endpoint, and + # an anonymous one at that; letting the generic REDIS_PASSWORD reach across + # would make that client send AUTH to a server with none, breaking a + # connection that worked before. An INHERITED url is the same endpoint as + # the generic one, so the full fallback chain applies. For env_prefix + # "REDIS_" the two levels are the same variable and this reads identically. + if own_url: + result["url_password"] = os.getenv(f"{env_prefix}PASSWORD") + result["url_username"] = os.getenv( + f"{env_prefix}USER", os.getenv(f"{env_prefix}USERNAME") + ) + else: + result["url_password"] = result["password"] + result["url_username"] = result["username"] if result["url"] and explicit_db and db_override is None: result["db_from_prefix_env"] = parse_db(explicit_db, env_prefix) # Read OUTSIDE the `if ssl` below: URL mode carries TLS in the scheme and never @@ -708,8 +724,8 @@ def _create_standalone_client( ssl_ca_certs=env.get("ssl_ca_certs"), ssl_cert_reqs=env.get("ssl_cert_reqs"), ssl_check_hostname=env.get("ssl_check_hostname"), - password=env.get("password"), - username=env.get("username"), + password=env.get("url_password"), + username=env.get("url_username"), ) logger.info( @@ -813,9 +829,13 @@ def _create_client_from_url( # not a credential: values.yaml ships REDIS_USER: default, so filling it in # on its own would make redis-py send AUTH to the in-cluster server, which # has none — turning a working default deployment into a failing one. - if password and "@" not in parts.netloc: + # Keyed on the URL's PASSWORD, not on the presence of "@". A URL may carry an + # ACL username alone — redis://alice@host — and that @ is not evidence of a + # password; treating it as such dropped the separately supplied one and left + # the client unable to authenticate. + if password and parts.password is None: kwargs["password"] = password - if username: + if username and not parts.username: kwargs["username"] = username logger.info( "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 3f17d47b0e..efc0abc5a6 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -975,3 +975,47 @@ def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): monkeypatch.setenv("REDIS_PASSWORD", "s3cret") url = build_socketio_redis_url() assert "in-url" in url and "s3cret" not in url + + def test_a_username_only_url_still_gets_the_password(self, monkeypatch): + """`redis://alice@host` carries an @ but NO password. + + Treating the @ as evidence of credentials dropped the separately + supplied password and left the client unable to authenticate. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "s3cret" + assert kwargs["username"] == "alice" + + +class TestUrlCredentialsAreResolvedAtTheUrlsLevel: + """Same rule as the database: a prefix that brought its OWN url owns its + own credentials. + + That url may point at a DIFFERENT endpoint, and an anonymous one; letting + the generic REDIS_PASSWORD reach across would make the client send AUTH to + a server that has none, breaking a connection that worked before. + """ + + def test_a_prefix_url_does_not_inherit_the_generic_password(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "redis://anon:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_")).get("password") is None + + def test_the_prefixs_own_password_applies(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "redis://anon:6379/0") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "own") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_"))["password"] == "own" + + def test_an_inherited_url_still_uses_the_generic_password(self, monkeypatch): + """Same endpoint as the generic one, so the fallback chain applies.""" + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_"))["password"] == "s3cret" + + def test_the_socketio_url_follows_the_same_rule(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "rediss://anon:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert "s3cret" not in build_socketio_redis_url("CACHE_REDIS_") From d1aef5d7e1ad5ef5694f03119deb55c98355bed1 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 10:56:42 +0530 Subject: [PATCH 04/12] UN-4123 [FIX] One credential helper for all three consumers, and the netloc bug in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A four-agent standardised review of this PR found two Criticals, both in code I added in the two commits before it. Every fix below is mutation-verified. apply_url_credentials CORRUPTED A URL THAT CARRIED A USERNAME. parts.netloc INCLUDES userinfo, and the function prepended to it — so redis://alice@host with a separately supplied password became redis://alice:pw@alice@host. Userinfo splits on the LAST "@", so kombu parsed the password as "pw@alice". Not a missing credential, a WRONG one: the server answers WRONGPASS, the publisher dies, and socketio.Server is built with logger=False so Socket.IO events simply stop. The docstring claimed this function existed to prevent exactly that. It now rebuilds the netloc from the host part, and reuses a username the URL already carries VERBATIM rather than re-quoting it — quoting an already-encoded username turned al%40ice into al%2540ice. THE DJANGO CACHE KEYED ON "@" BEING ABSENT — the precise heuristic the core comment rejects, 170 lines away, in the same PR. A username-only URL left the cache anonymous while create_redis_client authenticated: one process, one endpoint, two outcomes, which is the sentence this work exists to delete. Rather than align the third hand-rolled gate, it is gone: the cache LOCATION now goes through apply_url_credentials, the same helper the Socket.IO URL uses. That also closes a gap neither gate could — django-redis 5.4.0 discards OPTIONS["USERNAME"], so an ACL username could not reach this cache at all and it authenticated as `default` while the other consumers used the configured user. It reads REDIS_USER straight from os.environ rather than the module-level fallback of "default", so the cache and the client send the same AUTH form. EMPTY IS ABSENT. redis://:@host parses to password "", and both redis-py (`if url.password:`) and kombu (`unquote(password or "") or None`) read that as no password — so `is None` declined to fill a gap the libraries agree is a gap. That shape is what Helm emits from redis://:{{ .Values.password }}@host with an empty value. Truthiness in all three places now. A DEAD GUARD REMOVED. `and not parts.username` could not fail: from_url ends with kwargs.update(url_options), so a URL username wins regardless. An unfalsifiable guard is where a later reader deletes something load-bearing. TESTS 250 -> 257, and the existing ones were not strong enough. The socketio credential test asserted a SUBSTRING, which passes on the corrupt URL — that is why the Critical survived a test written for the same shape. Assertions are on parsed fields now. Added: username-only URLs on every consumer, double-encoding, a password containing @ / : (unencoded it reparses with host "ss" — a different server), the empty-password shape, the {prefix}USERNAME spelling, and the cache carrying an ACL username. Six mutations verified, including the netloc concat. COMMENT CORRECTIONS. "kombu takes a URL and NOTHING else — no connection kwargs" is false in both places it appeared: KombuManager forwards connection_options to kombu.Connection, which takes userid= and password=. The URL is where TLS must go; credentials ride along by choice, not necessity. And "never written into an error message" was wrong — the backend stores the result as settings.SOCKET_IO_MANAGER_URL, which Django's SafeExceptionReporterFilter does not redact. Two past-tense comments describing a mid-branch state as shipped history are now present tense. The three sample.env recipes now show REDIS_USER= blank, because this change makes that value load-bearing in URL mode for the first time while the files still ship REDIS_USER=default a few lines above. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 38 ++++++--- .../tests/test_redis_settings_derivation.py | 80 ++++++++++++++++--- backend/sample.env | 1 + runner/sample.env | 1 + .../src/unstract/core/cache/redis_client.py | 60 +++++++++----- .../core/tests/test_redis_client_config.py | 77 +++++++++++++++++- workers/sample.env | 1 + 7 files changed, 217 insertions(+), 41 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index c92ad69069..462cd1c05f 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -13,7 +13,7 @@ import os import re from pathlib import Path -from urllib.parse import quote, urlsplit +from urllib.parse import quote import httpx from django.core.validators import URLValidator @@ -22,6 +22,7 @@ from utils.cors_origin import normalize_web_app_origin from unstract.core.cache.redis_client import ( + apply_url_credentials, build_socketio_redis_url, ensure_tls_query_params, parse_db, @@ -660,14 +661,33 @@ def filter(self, record): _cache_options["DB"] = _cache_db _cache_options["USERNAME"] = REDIS_USER _cache_options["PASSWORD"] = REDIS_PASSWORD - elif REDIS_PASSWORD and "@" not in urlsplit(_redis_url).netloc: - # ...unless the URL carries NO credentials, which is the configuration - # that keeps the password out of a string that gets printed. django-redis - # feeds OPTIONS["PASSWORD"] into ConnectionPool.from_url, which ends with - # kwargs.update(url_options) — so this fills the gap and a URL bearing - # credentials still wins. Without it this cache connects ANONYMOUSLY - # while create_redis_client beside it authenticates. - _cache_options["PASSWORD"] = REDIS_PASSWORD + else: + # URL mode: the credentials go into the LOCATION, through the SAME helper + # the Socket.IO URL uses. Two reasons it is not OPTIONS["PASSWORD"]: + # + # - django-redis 5.4.0 discards OPTIONS["USERNAME"], so an ACL username + # could not reach this cache at all. It would authenticate as the + # built-in `default` user while create_redis_client in the same + # process authenticated as the configured one — and on an endpoint + # where `default` is disabled, the cache alone would fail. + # - a hand-rolled gate here diverged from the core one within a single + # PR: it keyed on "@" being absent from the netloc, which the core + # comment explicitly rejects, so a URL carrying only an ACL username + # left this cache anonymous. One helper, one rule, no second copy to + # keep in step. + # + # The helper returns the URL untouched when it already carries a + # password, so a credential-bearing REDIS_URL behaves exactly as before. + # os.environ direct, NOT the REDIS_USER above: that one defaults to + # "default", while create_redis_client reads the variable with no + # default. Passing the fallback here would make this cache send the + # two-argument `AUTH default ` while the client in the same process + # sends the one-argument form — both authenticate, but they would differ + # on the wire for no reason, and "the three consumers agree" is the + # property this change exists to establish. + _redis_url = apply_url_credentials( + _redis_url, REDIS_PASSWORD, os.environ.get("REDIS_USER") + ) # Gated on the EFFECTIVE scheme, not the flag. In URL mode TLS is carried by # the URL (and its query string), so a plaintext REDIS_URL left behind while # REDIS_SSL=true would otherwise hand ssl_cert_reqs to a plain diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index f00809b4f5..4dc26a5c2d 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -19,6 +19,7 @@ import logging import pathlib +from urllib.parse import urlsplit import pytest @@ -448,23 +449,78 @@ def test_no_database_var_is_parsed_with_a_bare_int(self): class TestUrlModeCacheCredentials: - """django-redis reads OPTIONS["PASSWORD"]; the URL is not its only source. + """The cache credentials travel in the LOCATION, through the shared helper. - A URL written without credentials kept this cache ANONYMOUS while - create_redis_client beside it authenticated — one process, one endpoint, two - outcomes. OPTIONS["PASSWORD"] reaches ConnectionPool.from_url, which ends - with kwargs.update(url_options), so a URL bearing credentials still wins. + Not OPTIONS["PASSWORD"]: django-redis 5.4.0 discards OPTIONS["USERNAME"], so + an ACL username could not reach this cache at all — it would authenticate as + the built-in `default` user while create_redis_client in the same process + used the configured one. + + A hand-rolled gate here also diverged from the core one within a single PR: + it keyed on "@" being absent from the netloc, which the core comment + explicitly rejects, leaving a username-only URL anonymous. One helper, one + rule. """ + def _location(self, **env: str) -> str: + return _derive(**env)["CACHES"]["default"]["LOCATION"] + def test_the_password_fills_a_gap_the_url_leaves(self): - derived = _derive(REDIS_URL="rediss://h:6380/0", REDIS_PASSWORD="s3cret") - assert derived["CACHES"]["default"]["OPTIONS"]["PASSWORD"] == "s3cret" + assert ( + urlsplit( + self._location(REDIS_URL="rediss://h:6380/0", REDIS_PASSWORD="s3cret") + ).password + == "s3cret" + ) + + def test_a_username_only_url_still_gets_the_password(self): + """The shape the `@` heuristic got wrong.""" + parts = urlsplit( + self._location(REDIS_URL="redis://alice@h:6379/0", REDIS_PASSWORD="s3cret") + ) + assert parts.hostname == "h" + assert parts.username == "alice" + assert parts.password == "s3cret" + + def test_an_acl_username_reaches_the_cache(self): + """django-redis discards OPTIONS["USERNAME"], so the LOCATION is the + only lever. Without it the cache authenticates as `default` while the + other consumers use the configured user. + """ + parts = urlsplit( + self._location( + REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw", REDIS_USER="alice" + ) + ) + assert parts.username == "alice" + assert parts.password == "pw" def test_a_url_carrying_credentials_is_left_alone(self): - """Passing it again risks the two disagreeing about which wins.""" - derived = _derive(REDIS_URL="rediss://:in-url@h:6380/0", REDIS_PASSWORD="s3cret") - assert "PASSWORD" not in derived["CACHES"]["default"]["OPTIONS"] + assert ( + urlsplit( + self._location( + REDIS_URL="rediss://:in-url@h:6380/0", REDIS_PASSWORD="s3cret" + ) + ).password + == "in-url" + ) def test_no_password_stays_anonymous(self): - derived = _derive(REDIS_URL="rediss://h:6380/0") - assert "PASSWORD" not in derived["CACHES"]["default"]["OPTIONS"] + assert urlsplit(self._location(REDIS_URL="rediss://h:6380/0")).password is None + + def test_the_shipped_default_user_alone_adds_nothing(self): + """REDIS_USER defaults to "default" in this module but not in + create_redis_client; passing the fallback would make the two send + different AUTH forms for the same configuration. + """ + assert ( + self._location(REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="") + == "redis://h:6379/0" + ) + + def test_discrete_mode_still_uses_options(self): + """URL mode moved to the LOCATION; the discrete path did not change.""" + options = _derive(REDIS_HOST="h", REDIS_PASSWORD="s3cret")["CACHES"]["default"][ + "OPTIONS" + ] + assert options["PASSWORD"] == "s3cret" diff --git a/backend/sample.env b/backend/sample.env index cceb6796ef..ca5456167d 100644 --- a/backend/sample.env +++ b/backend/sample.env @@ -62,6 +62,7 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: # REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required # REDIS_PASSWORD= +# REDIS_USER= # blank — see below # (The password is better kept OUT of the URL. A URL is printed into log # lines, error messages and deployment tooling; a password in one travels # with it. Credentials in the URL still work and still win, so an existing diff --git a/runner/sample.env b/runner/sample.env index 7745e49faa..23a9bc5079 100644 --- a/runner/sample.env +++ b/runner/sample.env @@ -57,6 +57,7 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: # REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required # REDIS_PASSWORD= +# REDIS_USER= # blank — see below # (The password is better kept OUT of the URL. A URL is printed into log # lines, error messages and deployment tooling; a password in one travels # with it. Credentials in the URL still work and still win, so an existing diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index caaa712d32..6c49878114 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -322,7 +322,8 @@ def ensure_tls_query_params( """Add the TLS settings a `rediss://` URL is missing, leaving present ones alone. Anything that hands a URL to a library that reads TLS out of the query string - needs this: kombu's KombuManager takes a URL and NOTHING else, and + needs this: kombu's KombuManager reads TLS from the URL alone — its + connection_options reach kombu.Connection, but the TLS settings do not — and django-redis's LOCATION is a string too (it also reads OPTIONS["CONNECTION_POOL_KWARGS"], but the query string wins on conflict). @@ -383,28 +384,44 @@ def _compose_redis_url(env: dict[str, Any]) -> str: def apply_url_credentials(url: str, password: str | None, username: str | None) -> str: """Put credentials into a URL that carries none, leaving one that does alone. - kombu takes a URL and NOTHING else — no connection kwargs — so a password - supplied separately cannot reach it any other way. A URL written without - credentials would otherwise produce an anonymous publisher against an - authenticated server, and the Socket.IO events simply stop arriving. + KombuManager reads TLS out of the query string only, so the URL is where TLS + has to go, and the credentials ride with it so ONE builder serves both the + kombu and the redis-py callers. That is a choice, not a constraint: + KombuManager forwards connection_options to kombu.Connection, which accepts + userid= and password=. - The password IS in the returned string, unavoidably. That is acceptable here - and not in a values file: this URL is built in-process and handed straight to - the client, rather than written into a manifest, a Secret template or an - error message. + A URL written without credentials otherwise produces an anonymous publisher + against an authenticated server, and Socket.IO events simply stop arriving. + + The password IS in the returned string. Smaller exposure than a values file, + but not none: the backend stores the result as settings.SOCKET_IO_MANAGER_URL, + and Django's SafeExceptionReporterFilter does not redact that name — a + technical-500 page would print it. One more reason DEBUG must stay off. """ if not password: return url parts = urlsplit(url) - if parts.password is not None: + # Truthiness, not `is not None`: `redis://:@host` parses to password "", and + # both redis-py (`if url.password:`) and kombu (`unquote(password or "") or + # None`) read that as absent. + if parts.password: return url - username = parts.username or username - credentials = f"{quote(str(username), safe='')}:" if username else ":" - credentials += f"{quote(str(password), safe='')}@" + # REBUILD the netloc; do not prepend to it. parts.netloc INCLUDES any + # userinfo, so prepending on a username-only URL produced + # `alice:pw@alice@host` — and userinfo splits on the LAST "@", so the + # password parsed as "pw@alice". A wrong credential, not a missing one. + # + # A username already in the URL is reused VERBATIM rather than re-quoted: it + # is percent-encoded already, and quoting it again turned `al%40ice` into + # `al%2540ice`. + userinfo, _, host_port = parts.netloc.rpartition("@") + url_user = userinfo.partition(":")[0] + user = url_user or (quote(str(username), safe="") if username else "") + credentials = f"{user}:{quote(str(password), safe='')}@" return urlunsplit( ( parts.scheme, - credentials + parts.netloc, + credentials + host_port, parts.path, parts.query, parts.fragment, @@ -819,7 +836,7 @@ def _create_client_from_url( # carrying credentials still wins. # # Without this, a URL written WITHOUT credentials plus a separately - # configured {prefix}PASSWORD connected ANONYMOUSLY — and setting the two + # configured {prefix}PASSWORD connects ANONYMOUSLY — and setting the two # apart is the configuration the on-prem recipe should be able to recommend, # because it keeps the password out of a URL that gets printed into error # messages, ArgoCD conditions and ExternalSecret templates. The endpoint @@ -831,11 +848,16 @@ def _create_client_from_url( # has none — turning a working default deployment into a failing one. # Keyed on the URL's PASSWORD, not on the presence of "@". A URL may carry an # ACL username alone — redis://alice@host — and that @ is not evidence of a - # password; treating it as such dropped the separately supplied one and left - # the client unable to authenticate. - if password and parts.password is None: + # password. Truthiness rather than `is None`: `redis://:@host` parses to "" + # and redis-py's own parse_url sets the key only `if url.password`. + # + # No `not parts.username` guard below: from_url ends with + # kwargs.update(url_options), so a URL username wins regardless — that guard + # was unfalsifiable, which is where a later reader deletes something + # load-bearing by mistake. + if password and not parts.password: kwargs["password"] = password - if username and not parts.username: + if username: kwargs["username"] = username logger.info( "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index efc0abc5a6..6cd8705ef7 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -13,6 +13,7 @@ import pathlib import re +from urllib.parse import urlsplit import pytest import redis @@ -968,7 +969,12 @@ def test_the_socketio_url_gains_the_credentials_too(self, monkeypatch): """ monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") monkeypatch.setenv("REDIS_PASSWORD", "s3cret") - assert "://:s3cret@h:6380" in build_socketio_redis_url() + # Assert on PARSED fields, not a substring: a substring check passes on + # `rediss://alice:s3cret@alice@h:6380/0`, where the password actually + # parses as "s3cret@alice". + parts = urlsplit(build_socketio_redis_url()) + assert parts.password == "s3cret" + assert parts.hostname == "h" def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") @@ -976,6 +982,17 @@ def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): url = build_socketio_redis_url() assert "in-url" in url and "s3cret" not in url + def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): + """`redis://:@host` parses to password "", which redis-py's own + parse_url reads as absent (`if url.password:`). Testing `is None` would + decline to fill a gap the library agrees is a gap — and the shape is + what Helm produces from `redis://:{{ .Values.password }}@host` when the + value is empty. + """ + monkeypatch.setenv("REDIS_URL", "redis://:@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "s3cret" + def test_a_username_only_url_still_gets_the_password(self, monkeypatch): """`redis://alice@host` carries an @ but NO password. @@ -1019,3 +1036,61 @@ def test_the_socketio_url_follows_the_same_rule(self, monkeypatch): monkeypatch.setenv("CACHE_REDIS_URL", "rediss://anon:6379/0") monkeypatch.setenv("REDIS_PASSWORD", "s3cret") assert "s3cret" not in build_socketio_redis_url("CACHE_REDIS_") + + +class TestSocketIoUrlCredentials: + """The Socket.IO builder must agree with create_redis_client on every shape. + + Its output is a STRING handed to kombu, so a malformed one authenticates + with a wrong secret rather than none — and `socketio.Server` is constructed + with logger=False, so the symptom is events silently stopping. + """ + + def test_a_username_only_url_keeps_its_host_and_gets_the_password(self, monkeypatch): + """`parts.netloc` INCLUDES userinfo; prepending to it produced + `alice:pw@alice@host`, and userinfo splits on the LAST @. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + parts = urlsplit(build_socketio_redis_url()) + assert parts.hostname == "h" + assert parts.username == "alice" + assert parts.password == "s3cret" + + def test_an_encoded_username_is_not_double_encoded(self, monkeypatch): + """It is percent-encoded already; quoting it again gave `al%2540ice`.""" + monkeypatch.setenv("REDIS_URL", "redis://al%40ice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert "%2540" not in build_socketio_redis_url() + + def test_a_password_with_url_metacharacters_is_encoded(self, monkeypatch): + """Unencoded, `p@ss/w:rd` reparses with host "ss" — a DIFFERENT server.""" + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "p@ss/w:rd") + url = build_socketio_redis_url() + assert urlsplit(url).hostname == "h" + assert "p%40ss%2Fw%3Ard" in url + + def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): + """`redis://:@host` parses to "", which redis-py and kombu both read as + no password — so the separately supplied one must fill the gap. + """ + monkeypatch.setenv("REDIS_URL", "redis://:@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert urlsplit(build_socketio_redis_url()).password == "s3cret" + + def test_a_username_alone_stays_anonymous(self, monkeypatch): + """values.yaml ships REDIS_USER: default against an unauthenticated + in-cluster server; adding credentials here would break it. + """ + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_USER", "default") + monkeypatch.setenv("REDIS_PASSWORD", "") + assert build_socketio_redis_url() == "redis://h:6379/0" + + def test_the_prefix_username_spelling_is_honoured(self, monkeypatch): + """{prefix}USERNAME is the compatibility spelling of {prefix}USER.""" + monkeypatch.setenv("CACHE_REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "pw") + monkeypatch.setenv("CACHE_REDIS_USERNAME", "alice") + assert urlsplit(build_socketio_redis_url("CACHE_REDIS_")).username == "alice" diff --git a/workers/sample.env b/workers/sample.env index 2bd23853f0..b6c1078571 100644 --- a/workers/sample.env +++ b/workers/sample.env @@ -83,6 +83,7 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # full URL whose scheme carries TLS: # REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required # REDIS_PASSWORD= +# REDIS_USER= # blank — see below # (The password is better kept OUT of the URL. A URL is printed into log # lines, error messages and deployment tooling; a password in one travels # with it. Credentials in the URL still work and still win, so an existing From 3ec96b81ea31b52d723d707089f91f344be67ea4 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 15:31:25 +0530 Subject: [PATCH 05/12] UN-4123 [FIX] Round-2 review: one resolver for usernames, and blank means unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second standard-review round. Round 1's two Criticals held up under attack — apply_url_credentials was fed 16 URL shapes round-tripped through urlsplit, redis-py and kombu, and all three agree on every one. What round 2 found is a layer down: the credential CHAIN, and tests that could not fail. * base.py re-spelled the username chain instead of asking core for it, and dropped a leg. The resolver accepts REDIS_USER and REDIS_USERNAME; platform-service ships the second, so one shared Redis-credential secret can inject it — and the Django cache then authenticated as `default` while create_redis_client in the SAME process authenticated as the configured ACL user. Where `default` is disabled, the cache alone fails. That is verbatim the divergence the round-1 comment says the change exists to delete, so the chain now lives in one exported helper both sides call. * BLANK MEANT SET for credentials, which broke this PR's own recipe. workers/sample.env ships `CACHE_REDIS_PASSWORD=` uncommented, and `os.getenv(prefixed, os.getenv(generic))` hands back that blank rather than consulting REDIS_PASSWORD. An operator following the documented managed-Redis recipe — REDIS_URL without credentials plus REDIS_PASSWORD — got url_password "" on every prefixed client, apply_url_credentials no-opped, and the worker caches connected ANONYMOUSLY. The module already documented blank-means-unset for parse_db and _parse_bool; host, port, password and user now follow it. This changes a test that asserted the old behaviour. It was a characterisation test that said so — "documents a trap rather than endorsing it… this test fails if that behaviour ever changes, so the chart-side guarantee can be revisited". This is that revisit: the guarantee it rested on (the chart never renders an empty credential) is not one sample.env keeps. * PORT was the one var with no error contract — int() straight off os.getenv, so a blank REDIS_PORT= raised at client construction naming neither the variable nor the prefix. parse_port now mirrors parse_db. TESTS — the round-1 lesson had not actually been applied. Four assertions still compared substrings of a constructed URL, including one two lines below the test I fixed in round 1, carrying a comment explaining the very bug it was still vulnerable to. Each is replaced by parsed fields, and the password one by an unquote() round-trip, since a hostname guard catches only the host half of a doubled userinfo. Two mutations survived round 2 and now fail: * reversing url_user/username precedence — no test set a URL username AND REDIS_USER at once, which is the only environment that tells them apart, and values.yaml ships REDIS_USER: default so the pairing is ordinary. * deleting `not parts.password` — harmless for the password (from_url ends with kwargs.update(url_options)) but load-bearing for the USERNAME rider, so it turned a one-argument AUTH into a two-argument one while the Socket.IO URL for the same config kept sending one. The comment above it argued the guard was unfalsifiable, which is true of the password half and false of the other — i.e. it documented the deletion the suite could not catch. The _derive harness executes two source ranges with a ~415-line gap between them, and anything in that gap is invisible to every assertion in the file: adding `REDIS_PASSWORD = ""` at base.py:251 leaves all 44 tests green while shipping a broken cache. It now asserts its own coverage — every Redis line in base.py must fall inside a spliced range — and fails naming the line that escaped. Verified against that exact insertion. Also: test_the_shipped_default_user_alone_adds_nothing passed REDIS_PASSWORD="" and so returned at the `if not password` guard before the username was read, making the test named for that choice blind to it. Core 120 -> 126 (263 in the dir), backend 42 -> 44. Six mutations verified caught. The exposure note now names CACHES["default"]["LOCATION"] as the second unredacted landing place for the password, and says why it is a deliberate trade rather than an oversight. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 28 ++-- .../tests/test_redis_settings_derivation.py | 98 +++++++++++++- .../src/unstract/core/cache/redis_client.py | 95 +++++++++++-- .../core/tests/test_redis_client_config.py | 127 ++++++++++++++++-- 4 files changed, 310 insertions(+), 38 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 462cd1c05f..ec7a038ff4 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -30,6 +30,7 @@ resolve_ssl_check_hostname, set_url_db_path, url_db_path, + url_username_from_env, ) # Django 5.0+ caps URLValidator at 2048 chars. S3 pre-signed URLs signed with @@ -678,15 +679,26 @@ def filter(self, record): # # The helper returns the URL untouched when it already carries a # password, so a credential-bearing REDIS_URL behaves exactly as before. - # os.environ direct, NOT the REDIS_USER above: that one defaults to - # "default", while create_redis_client reads the variable with no - # default. Passing the fallback here would make this cache send the - # two-argument `AUTH default ` while the client in the same process - # sends the one-argument form — both authenticate, but they would differ - # on the wire for no reason, and "the three consumers agree" is the - # property this change exists to establish. + # The username comes from the SAME resolver the core client uses, not + # from a hand-written os.environ.get here. Two reasons, and the second + # was a live divergence: + # + # - the module-level REDIS_USER above defaults to "default", while + # create_redis_client reads the variable with no default. Passing + # the fallback would make this cache send the two-argument + # `AUTH default ` while the client in the same process sends the + # one-argument form. + # - the resolver accepts BOTH spellings, REDIS_USER and + # REDIS_USERNAME. platform-service ships the second one, so a single + # shared Redis-credential secret can easily inject it — and reading + # only REDIS_USER left this cache authenticating as `default` while + # the client beside it authenticated as the configured ACL user. + # + # Re-deriving the chain here is the same second copy this commit removed + # for the "@" heuristic; asking core for it is what keeps the three + # consumers in step by construction rather than by comment. _redis_url = apply_url_credentials( - _redis_url, REDIS_PASSWORD, os.environ.get("REDIS_USER") + _redis_url, REDIS_PASSWORD, url_username_from_env() ) # Gated on the EFFECTIVE scheme, not the flag. In URL mode TLS is carried by # the URL (and its query string), so a plaintext REDIS_URL left behind while diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index 4dc26a5c2d..1d07386ca9 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -19,6 +19,7 @@ import logging import pathlib +import re from urllib.parse import urlsplit import pytest @@ -26,6 +27,73 @@ _SETTINGS = pathlib.Path(__file__).resolve().parents[1] / "settings" / "base.py" +def _slice_bounds(source: str) -> dict[str, tuple[int, int]]: + """Character ranges of base.py that _derive executes. + + Factored out so the coverage guard below can assert on the SAME ranges the + harness runs, rather than a second description of them that could drift. + """ + defs_start = source.index('REDIS_USER = os.environ.get("REDIS_USER"') + defs_end = ( + source.index("\n", source.index('REDIS_URL = os.environ.get("REDIS_URL"')) + 1 + ) + imports_start = source.index("from unstract.core.cache.redis_client import (") + imports_end = source.index(")\n", imports_start) + 2 + urllib_start = source.index("from urllib.parse import ") + urllib_end = source.index("\n", urllib_start) + 1 + return { + "urllib": (urllib_start, urllib_end), + "imports": (imports_start, imports_end), + "defs": (defs_start, defs_end), + "block": ( + source.index("REDIS_SENTINEL_MODE = ("), + source.index("SESSION_ENGINE ="), + ), + } + + +def test_the_harness_covers_every_redis_line_in_the_settings_file(): + """The splice must not silently stop covering the code it claims to test. + + _derive executes two ranges out of base.py with a ~415-line gap between + them, and anything in that gap is invisible to every assertion in this + file. That is not theoretical: adding `REDIS_PASSWORD = ""` at base.py:251, + or appending a CACHES["default"]["LOCATION"] override after SESSION_ENGINE, + leaves all of these tests green while shipping a broken cache. + + The file already concedes the gap once — test_no_database_var_is_parsed_with + _a_bare_int greps the source text because FILE_ACTIVE_CACHE_REDIS_DB sits + outside both slices. A grep only covers the one pattern someone thought of; + this covers the boundary itself, and fails naming the line that escaped. + """ + source = _SETTINGS.read_text() + bounds = _slice_bounds(source) + covered = [] + for start, end in bounds.values(): + covered.append(range(start, end)) + + offenders = [] + offset = 0 + for line in source.splitlines(keepends=True): + if re.match(r"\s*(REDIS_|_redis|_cache|CACHES|SOCKET_IO)", line) and not any( + offset in span for span in covered + ): + offenders.append((source[:offset].count("\n") + 1, line.strip()[:70])) + offset += len(line) + + # Known and deliberate: these are asserted by source-text inspection + # instead, because they are consumed far from the derivation block. + allowed = {"FILE_ACTIVE_CACHE_REDIS_DB", "REDIS_DB_PORTAL"} + offenders = [o for o in offenders if not any(a in o[1] for a in allowed)] + + assert not offenders, ( + "these Redis lines in base.py are OUTSIDE the ranges _derive executes, " + "so no test in this file can observe them:\n" + + "\n".join(f" base.py:{n}: {text}" for n, text in offenders) + + "\nWiden the slice, or add the name to `allowed` with a reason." + ) + + def _derive(**env: str) -> dict: """Execute the standalone Redis block with the given env.""" source = _SETTINGS.read_text() @@ -138,7 +206,10 @@ def test_db_travels_in_the_location(self): an empty db 0. """ derived = _derive(REDIS_HOST="h", REDIS_DB="3") - assert derived["CACHES"]["default"]["LOCATION"].endswith("/3") + # Full string, not endswith("/3"): the failure this guards is + # same-endpoint-WRONG-db, and endswith is equally satisfied by + # "redis://WRONGHOST:6379/3" or a changed port. + assert derived["CACHES"]["default"]["LOCATION"] == "redis://h:6379/3" def test_ssl_switches_scheme_and_pool_kwargs(self): derived = _derive(REDIS_HOST="h", REDIS_SSL="true", REDIS_SSL_CA_CERTS="/ca.pem") @@ -512,10 +583,31 @@ def test_the_shipped_default_user_alone_adds_nothing(self): """REDIS_USER defaults to "default" in this module but not in create_redis_client; passing the fallback would make the two send different AUTH forms for the same configuration. + + The password must be NON-empty. With REDIS_PASSWORD="" the helper + returns at its `if not password` guard before the username argument is + read at all, so the test named for this choice could not fail on it — + mutating the call to pass the module-level REDIS_USER left the suite + green while changing a one-argument AUTH into a two-argument one. """ assert ( - self._location(REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="") - == "redis://h:6379/0" + self._location(REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw") + == "redis://:pw@h:6379/0" + ) + + def test_the_cache_honours_the_second_username_spelling(self): + """platform-service ships REDIS_USERNAME, the chart ships REDIS_USER. + + Reading only REDIS_USER left this cache authenticating as the built-in + `default` while create_redis_client in the SAME process authenticated + as the configured ACL user — and where `default` is disabled, only the + cache fails. + """ + assert ( + self._location( + REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw", REDIS_USERNAME="alice" + ) + == "redis://alice:pw@h:6379/0" ) def test_discrete_mode_still_uses_options(self): diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 6c49878114..c22dadcf9f 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -381,6 +381,57 @@ def _compose_redis_url(env: dict[str, Any]) -> str: return f"{scheme}://{credentials}{env['host']}:{env['port']}" +def env_chain(*names: str) -> str | None: + """First NON-BLANK value among these env vars, else None. + + Blank means unset throughout this module — it is the "leave the default" + spelling the sample recipes and values files use, and it is why a nested + `os.getenv(prefixed, os.getenv(generic))` is wrong here: `os.getenv` reports + a set-but-empty variable as set, so the blank shadows the level below it. + """ + for name in names: + value = os.getenv(name, "").strip() + if value: + return value + return None + + +def parse_port(raw: str | None, env_prefix: str, default_port: str | int) -> int: + """A port number; blank means unset, unparseable warns and falls back. + + parse_db already does this for the database and says why: an int() straight + out of os.getenv raises at client construction with a message naming neither + the variable nor the prefix, which in most consumers kills the process at + import and in the two that catch broadly degrades to no-cache. + """ + if raw is None: + return int(default_port) + try: + return int(raw) + except ValueError: + logger.warning("Invalid %sPORT=%r; using %s", env_prefix, raw, default_port) + return int(default_port) + + +def url_username_from_env(env_prefix: str = "REDIS_") -> str | None: + """The ACL username for a URL, in BOTH spellings that ship. + + `{prefix}USER` is what the chart writes; `{prefix}USERNAME` is what + platform-service/sample.env writes and platform_service/env.py reads. One + shared Redis-credential secret naturally injects whichever name its author + picked, so a consumer that reads only one of them authenticates as a + different user than the client beside it in the same process — and on an + endpoint where the built-in `default` is disabled, only that one consumer + fails. + + Empty means absent, matching the rest of this module: `REDIS_USER=` is the + "leave it unset" spelling the sample recipes use, and `os.getenv(name, + fallback)` would return that blank instead of falling through to the second + spelling. + """ + return os.getenv(f"{env_prefix}USER") or os.getenv(f"{env_prefix}USERNAME") or None + + def apply_url_credentials(url: str, password: str | None, username: str | None) -> str: """Put credentials into a URL that carries none, leaving one that does alone. @@ -394,9 +445,20 @@ def apply_url_credentials(url: str, password: str | None, username: str | None) against an authenticated server, and Socket.IO events simply stop arriving. The password IS in the returned string. Smaller exposure than a values file, - but not none: the backend stores the result as settings.SOCKET_IO_MANAGER_URL, - and Django's SafeExceptionReporterFilter does not redact that name — a - technical-500 page would print it. One more reason DEBUG must stay off. + but not none, and it now has TWO landing places in Django settings: + SOCKET_IO_MANAGER_URL, and CACHES["default"]["LOCATION"]. Neither name + matches SafeExceptionReporterFilter's `API|TOKEN|KEY|SECRET|PASS|SIGNATURE| + HTTP_COOKIE`, so neither is redacted on a technical-500 page, in + `manage.py diffsettings`, or in any settings dump — whereas + OPTIONS["PASSWORD"], where the cache password used to sit, IS masked. + The LOCATION is also the key of django-redis's process-global pool dict. + + This is a deliberate trade, not an oversight: django-redis 5.4.0 discards + OPTIONS["USERNAME"], so the LOCATION is the ONLY route by which an ACL + username can reach that cache. Stated here because the call site in + backend/settings/base.py is where someone will look for the risk and the + reasoning lives on this side of the boundary. One more reason DEBUG must + stay off. """ if not password: return url @@ -483,12 +545,21 @@ def _resolve_redis_env( env_prefix: str, default_port: str = "6379", db_override: int | None = None ) -> dict[str, Any]: """Read common Redis env vars into a dict.""" - host = os.getenv(f"{env_prefix}HOST", os.getenv("REDIS_HOST", "localhost")) - port = int(os.getenv(f"{env_prefix}PORT", os.getenv("REDIS_PORT", default_port))) - password = os.getenv(f"{env_prefix}PASSWORD", os.getenv("REDIS_PASSWORD")) - username = os.getenv( - f"{env_prefix}USER", - os.getenv(f"{env_prefix}USERNAME", os.getenv("REDIS_USER")), + host = env_chain(f"{env_prefix}HOST", "REDIS_HOST") or "localhost" + port = parse_port( + env_chain(f"{env_prefix}PORT", "REDIS_PORT"), env_prefix, default_port + ) + # BLANK MEANS UNSET, like parse_db and _parse_bool in this same module — a + # nested os.getenv(prefixed, generic) takes the blank and never consults the + # generic one. workers/sample.env ships `CACHE_REDIS_PASSWORD=` uncommented, + # so an operator following this module's own recipe (REDIS_URL without + # credentials + REDIS_PASSWORD) got url_password="" for every prefixed + # client, apply_url_credentials no-opped, and the worker caches connected + # ANONYMOUSLY — the exact failure that helper exists to eliminate, + # reintroduced by an empty variable. + password = env_chain(f"{env_prefix}PASSWORD", "REDIS_PASSWORD") + username = env_chain( + f"{env_prefix}USER", f"{env_prefix}USERNAME", "REDIS_USER", "REDIS_USERNAME" ) prefixed_db = os.getenv(f"{env_prefix}DB", "").strip() generic_db = os.getenv("REDIS_DB", "").strip() @@ -559,10 +630,8 @@ def _resolve_redis_env( # the generic one, so the full fallback chain applies. For env_prefix # "REDIS_" the two levels are the same variable and this reads identically. if own_url: - result["url_password"] = os.getenv(f"{env_prefix}PASSWORD") - result["url_username"] = os.getenv( - f"{env_prefix}USER", os.getenv(f"{env_prefix}USERNAME") - ) + result["url_password"] = env_chain(f"{env_prefix}PASSWORD") + result["url_username"] = url_username_from_env(env_prefix) else: result["url_password"] = result["password"] result["url_username"] = result["username"] diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 6cd8705ef7..211dac59a1 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -13,7 +13,7 @@ import pathlib import re -from urllib.parse import urlsplit +from urllib.parse import unquote, urlsplit import pytest import redis @@ -264,19 +264,39 @@ def test_prefixed_client_inherits_the_global_password(self, monkeypatch): "s3cr3t" ) - def test_empty_prefixed_password_shadows_the_fallback(self, monkeypatch): - """Documents a trap rather than endorsing it. + def test_empty_prefixed_password_falls_through_to_the_fallback(self, monkeypatch): + """This is the revisit the previous version of this test asked for. - ``os.getenv(key, fallback)`` returns "" when the key exists but is empty, - so an empty CACHE_REDIS_PASSWORD suppresses REDIS_PASSWORD and the client - connects UNAUTHENTICATED. The Helm chart must therefore never render an - empty credential; this test fails if that behaviour ever changes, so the - chart-side guarantee can be revisited. + It used to assert the opposite and said so: an empty + CACHE_REDIS_PASSWORD suppressed REDIS_PASSWORD and the client connected + UNAUTHENTICATED, which was tolerated because "the Helm chart must + therefore never render an empty credential". + + That guarantee does not hold. workers/sample.env ships + `CACHE_REDIS_PASSWORD=` uncommented, so an operator following this + module's own managed-Redis recipe — REDIS_URL without credentials plus + REDIS_PASSWORD — got no password on any prefixed client and a NOAUTH on + first command. Blank now means unset here, the same rule parse_db and + _parse_bool already document two functions away. """ monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") monkeypatch.setenv("CACHE_REDIS_PASSWORD", "") kwargs = _kwargs(create_redis_client(env_prefix="CACHE_REDIS_")) - assert kwargs.get("password") is None + assert kwargs["password"] == "s3cr3t" + + def test_a_prefixed_password_still_overrides(self, monkeypatch): + """Blank falling through must not turn into the prefix being ignored.""" + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "other") + kwargs = _kwargs(create_redis_client(env_prefix="CACHE_REDIS_")) + assert kwargs["password"] == "other" + + def test_a_blank_password_on_a_prefixed_url_reaches_the_url(self, monkeypatch): + """The recipe this PR documents, with the sample.env blank in place.""" + monkeypatch.setenv("REDIS_URL", "rediss://managed:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "") + assert urlsplit(build_socketio_redis_url("CACHE_REDIS_")).password == "pw" class TestSocketIoUrl: @@ -977,10 +997,17 @@ def test_the_socketio_url_gains_the_credentials_too(self, monkeypatch): assert parts.hostname == "h" def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): + """Parsed fields, not substrings — the same rule as the test above. + + `"in-url" in url` also passes on `rediss://:in-url@in-url@h:6380/0`, + where the password parses as "in-url@in-url", and on + `rediss://h:6380/0?note=in-url`, where there is no password at all. + """ monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") monkeypatch.setenv("REDIS_PASSWORD", "s3cret") - url = build_socketio_redis_url() - assert "in-url" in url and "s3cret" not in url + parts = urlsplit(build_socketio_redis_url()) + assert parts.password == "in-url" + assert parts.hostname == "h" def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): """`redis://:@host` parses to password "", which redis-py's own @@ -1035,7 +1062,11 @@ def test_an_inherited_url_still_uses_the_generic_password(self, monkeypatch): def test_the_socketio_url_follows_the_same_rule(self, monkeypatch): monkeypatch.setenv("CACHE_REDIS_URL", "rediss://anon:6379/0") monkeypatch.setenv("REDIS_PASSWORD", "s3cret") - assert "s3cret" not in build_socketio_redis_url("CACHE_REDIS_") + # A bare `"s3cret" not in url` also passes on "" and on any mangled + # output, so the endpoint is pinned too. + parts = urlsplit(build_socketio_redis_url("CACHE_REDIS_")) + assert parts.password is None + assert parts.hostname == "anon" class TestSocketIoUrlCredentials: @@ -1061,7 +1092,9 @@ def test_an_encoded_username_is_not_double_encoded(self, monkeypatch): """It is percent-encoded already; quoting it again gave `al%2540ice`.""" monkeypatch.setenv("REDIS_URL", "redis://al%40ice@h:6379/0") monkeypatch.setenv("REDIS_PASSWORD", "s3cret") - assert "%2540" not in build_socketio_redis_url() + # Asserted POSITIVELY: `"%2540" not in url` is equally satisfied by a + # URL that dropped the username, or replaced it with REDIS_USER. + assert urlsplit(build_socketio_redis_url()).username == "al%40ice" def test_a_password_with_url_metacharacters_is_encoded(self, monkeypatch): """Unencoded, `p@ss/w:rd` reparses with host "ss" — a DIFFERENT server.""" @@ -1069,7 +1102,12 @@ def test_a_password_with_url_metacharacters_is_encoded(self, monkeypatch): monkeypatch.setenv("REDIS_PASSWORD", "p@ss/w:rd") url = build_socketio_redis_url() assert urlsplit(url).hostname == "h" - assert "p%40ss%2Fw%3Ard" in url + # Round-trip, not a spelling check. The hostname guard catches the + # host half of a doubled userinfo but not the credential half: + # `rediss://x:p%40ss%2Fw%3Ard@p%40ss%2Fw%3Ard@h:6380/0` has hostname + # "h" and contains the substring, yet kombu would send + # "p@ss/w:rd@p@ss/w:rd". unquote() is what kombu itself applies. + assert unquote(urlsplit(url).password) == "p@ss/w:rd" def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): """`redis://:@host` parses to "", which redis-py and kombu both read as @@ -1094,3 +1132,64 @@ def test_the_prefix_username_spelling_is_honoured(self, monkeypatch): monkeypatch.setenv("CACHE_REDIS_PASSWORD", "pw") monkeypatch.setenv("CACHE_REDIS_USERNAME", "alice") assert urlsplit(build_socketio_redis_url("CACHE_REDIS_")).username == "alice" + + +class TestTheConsumersAgreeOnTheUsername: + """The client, the Socket.IO URL and the Django cache must resolve the SAME + ACL user. Each was previously pinned only to itself, so a change that made + them disagree passed every test. + """ + + def test_a_url_username_outranks_the_configured_one(self, monkeypatch): + """Both set at once is the only environment that tells the two apart. + + values.yaml ships REDIS_USER: default, so pairing it with a URL that + names a different user is an ordinary config — and reversing the + precedence made the client authenticate as `alice` while kombu and the + Django cache authenticated as `default`. On an endpoint where `default` + is disabled, Socket.IO events stop and nothing else does. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + monkeypatch.setenv("REDIS_USER", "default") + assert urlsplit(build_socketio_redis_url()).username == "alice" + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_password_only_url_takes_no_username_from_the_env(self, monkeypatch): + """The `not parts.password` guard is load-bearing for the USERNAME. + + For the password it is not: from_url ends with kwargs.update(url_options) + so the URL's password wins either way. Dropping the guard therefore + looks harmless and is not — it lets REDIS_USER ride along, turning a + one-argument `AUTH ` into a two-argument `AUTH alice ` while the + Socket.IO URL for the same config still sends the one-argument form. + """ + monkeypatch.setenv("REDIS_URL", "redis://:urlpw@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "envpw") + monkeypatch.setenv("REDIS_USER", "alice") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "urlpw" + assert kwargs.get("username") is None + assert urlsplit(build_socketio_redis_url()).username in (None, "") + + def test_the_username_env_var_has_two_spellings(self, monkeypatch): + """REDIS_USER is what the chart writes; REDIS_USERNAME is what + platform-service/sample.env writes. One shared credential secret can + inject either, so a consumer that reads only one authenticates as a + different user than the client beside it. + """ + monkeypatch.delenv("REDIS_USER", raising=False) + monkeypatch.setenv("REDIS_USERNAME", "alice") + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + assert urlsplit(build_socketio_redis_url()).username == "alice" + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_blank_user_falls_through_to_the_other_spelling(self, monkeypatch): + """`REDIS_USER=` is the "leave it unset" spelling the recipes use, and + `os.getenv(name, fallback)` would hand back the blank instead.""" + monkeypatch.setenv("REDIS_USER", "") + monkeypatch.setenv("REDIS_USERNAME", "alice") + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + assert urlsplit(build_socketio_redis_url()).username == "alice" From 9893110f4412d8d56756c8aeb2c611e6417755ec Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 15:45:39 +0530 Subject: [PATCH 06/12] UN-4123 [FIX] Round 3: finish the blank-means-unset convention, and stop stripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the round-2 commit. Two of the three findings are defects that commit introduced, and one of them made the change actively worse than before it. * BLANK-MEANS-UNSET WAS APPLIED TO CREDENTIALS BUT NOT TO TLS. Round 2 made a blank {prefix}PASSWORD/USER/HOST fall through to the generic level; the four TLS vars kept shadowing. So `CACHE_REDIS_SSL=` beside `REDIS_SSL=true` now gave the worker cache a real AUTH over an UNENCRYPTED socket — the credential round 2 taught it to inherit, on the wire in clear. Before round 2 the password was suppressed too and the client simply failed to authenticate. Half a convention was worse than none. All four TLS vars go through env_chain now, and the comment claiming the rule held "throughout this module" is true again rather than aspirational. * env_chain STRIPPED what it returned, and backend/settings/base.py does not. A password or username from a file-mounted secret ends in a newline, so core truncated it while the settings module kept it, and the Socket.IO URL and the cache LOCATION — built ten lines apart — carried different secrets. Emptiness is now tested on the stripped value and the RAW value returned, so the two agree again. That is the property round 2 claimed to establish. * url_username_from_env, the helper round 2 added specifically to unify this chain, was itself the one place not honouring the rule: plain `or` with no blank handling, so ` alice ` and `appuser\n` resolved differently from every other read of the same variable. It calls env_chain now. Also: * base.py still did a bare int() on REDIS_PORT, so a blank REDIS_PORT= raised during Django settings import — the backend alone failing to start while every worker came up fine on 6379. parse_port was exported in round 2 and simply not wired up here. * parse_port and parse_db named the PREFIXED variable in their diagnostics even when the bad value came from the generic one, sending an operator to grep for a key that is not set. env_chain_named returns the winning name, and the port fallback is logged at error rather than warning: the fallback is a PORT, so on a host serving both 6379 and 6380 a typo lands the client on the other server rather than failing. TestContainerAllowlists caught something real and deserves the credit: it derives the container env allowlist by scanning `os.getenv` calls in the source, so rewriting those reads to go through env_chain made the whole TLS set vanish from the guard silently. That is the same drift class as the two test harnesses fixed in round 2. The derivation now scans the arguments of every env-reading call, still scoped to those call names so a variable merely named in a docstring is not demanded of every container. Core 126 -> 137 (274 in the dir). Five mutations verified caught, including both blank-TLS shadows and the strip asymmetry. NOT fixed here, recorded deliberately: unstract-cloud's values.yaml pins CACHE_REDIS_USERNAME: "" with a comment stating the shadowing is intentional and "must not be fixed". That reasoning is now void — the pin is inert rather than load-bearing. The wire change is benign (Redis 6+ treats `AUTH default ` and `AUTH ` alike, and redis-py retries one-arg anyway), but the comment belongs in the cloud PR, not this one. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 9 +- .../src/unstract/core/cache/redis_client.py | 71 ++++++++---- .../core/tests/test_redis_client_config.py | 108 +++++++++++++++++- 3 files changed, 156 insertions(+), 32 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index ec7a038ff4..c7c62d5437 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -26,6 +26,7 @@ build_socketio_redis_url, ensure_tls_query_params, parse_db, + parse_port, resolve_ssl_cert_reqs, resolve_ssl_check_hostname, set_url_db_path, @@ -114,7 +115,11 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | REDIS_USER = os.environ.get("REDIS_USER", "default") REDIS_PASSWORD = os.environ.get("REDIS_PASSWORD", "") REDIS_HOST = os.environ.get("REDIS_HOST", "localhost") -REDIS_PORT = os.environ.get("REDIS_PORT", "6379") +# Through the shared parser, not a bare int() at the Sentinel call site below: +# a blank REDIS_PORT= — this repo's "leave the default" spelling — raised +# ValueError while Django settings were being imported, so the backend alone +# failed to start while every worker came up healthy on 6379. +REDIS_PORT = parse_port(os.environ.get("REDIS_PORT"), "REDIS_PORT", 6379) REDIS_DB = os.environ.get("REDIS_DB", "") # TLS to Redis (UN-4123). Off by default, so the in-cluster/local server is # untouched. `rediss://` is what actually selects TLS for both django-redis and @@ -583,7 +588,7 @@ def filter(self, record): "CLIENT_CLASS": "django_redis.client.SentinelClient", "CONNECTION_POOL_CLASS": "redis.sentinel.SentinelConnectionPool", "CONNECTION_FACTORY": "django_redis.pool.SentinelConnectionFactory", - "SENTINELS": [(REDIS_HOST, int(REDIS_PORT))], + "SENTINELS": [(REDIS_HOST, REDIS_PORT)], "SENTINEL_KWARGS": _sentinel_kwargs, "DB": _redis_db, "PASSWORD": REDIS_PASSWORD, diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index c22dadcf9f..8443d678b4 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -136,15 +136,17 @@ def url_db_path(url: str) -> int | None: return None -def _parse_bool(raw: str, default: bool, name: str) -> bool: +def _parse_bool(raw: str | None, default: bool, name: str) -> bool: """Parse a boolean env var; blank means UNSET, unknown warns and defaults. Blank-means-unset is this repo's own convention — `FOO=` in a sample.env means "leave the default", and every other variable in UN-4123 treats it that way. `os.getenv` does not: it reports an empty string as SET, so a bare `os.getenv(...) == "true"` turns `FOO=` into False. + + Accepts None so callers can hand it env_chain's "nothing was set" directly. """ - value = raw.strip().lower() + value = (raw or "").strip().lower() if not value: return default if value in _TRUE_LITERALS: @@ -190,9 +192,7 @@ def resolve_ssl_cert_reqs(env_prefix: str = "REDIS_") -> str: endpoint, three verification policies. Case and stray whitespace had the same effect, since the "is verification off?" test is an equality check. """ - raw = os.getenv( - f"{env_prefix}SSL_CERT_REQS", os.getenv("REDIS_SSL_CERT_REQS", "") - ).strip() + raw = (env_chain(f"{env_prefix}SSL_CERT_REQS", "REDIS_SSL_CERT_REQS") or "").strip() value = raw.lower() if not value: return _DEFAULT_CERT_REQS @@ -216,13 +216,10 @@ def resolve_ssl_check_hostname(env_prefix: str = "REDIS_", default: bool = True) FALSE, silently downgrading the Django cache to an encrypted but unauthenticated connection. """ - return _parse_bool( - os.getenv( - f"{env_prefix}SSL_CHECK_HOSTNAME", os.getenv("REDIS_SSL_CHECK_HOSTNAME", "") - ), - default, - f"{env_prefix}SSL_CHECK_HOSTNAME", + raw, name = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" ) + return _parse_bool(raw, default, name) def url_cert_reqs(url: str) -> str | None: @@ -390,13 +387,32 @@ def env_chain(*names: str) -> str | None: a set-but-empty variable as set, so the blank shadows the level below it. """ for name in names: - value = os.getenv(name, "").strip() - if value: + value = os.getenv(name, "") + # Emptiness is tested on the STRIPPED value; the RAW one is returned. + # Stripping the return value silently truncated a password or username + # that came from a file-mounted secret with a trailing newline, while + # backend/settings/base.py read the same variable unstripped — so one + # process authenticated two different ways against one endpoint. + if value.strip(): return value return None -def parse_port(raw: str | None, env_prefix: str, default_port: str | int) -> int: +def env_chain_named(*names: str) -> tuple[str | None, str]: + """env_chain, plus the name of the variable the value came from. + + So a warning about an unusable value can name the variable that is actually + SET. Reporting the prefixed name for a value that came from the generic one + sends an operator grepping for a key that does not exist in their config. + """ + for name in names: + value = os.getenv(name, "") + if value.strip(): + return value, name + return None, names[0] + + +def parse_port(raw: str | None, var_name: str, default_port: str | int) -> int: """A port number; blank means unset, unparseable warns and falls back. parse_db already does this for the database and says why: an int() straight @@ -409,7 +425,10 @@ def parse_port(raw: str | None, env_prefix: str, default_port: str | int) -> int try: return int(raw) except ValueError: - logger.warning("Invalid %sPORT=%r; using %s", env_prefix, raw, default_port) + # error, not warning: the fallback is a PORT, and on a host serving both + # 6379 and 6380 a typo lands the client on the other server rather than + # failing — a wrong endpoint, reported as a wrong value. + logger.error("Invalid %s=%r; using %s", var_name, raw, default_port) return int(default_port) @@ -429,7 +448,7 @@ def url_username_from_env(env_prefix: str = "REDIS_") -> str | None: fallback)` would return that blank instead of falling through to the second spelling. """ - return os.getenv(f"{env_prefix}USER") or os.getenv(f"{env_prefix}USERNAME") or None + return env_chain(f"{env_prefix}USER", f"{env_prefix}USERNAME") def apply_url_credentials(url: str, password: str | None, username: str | None) -> str: @@ -546,9 +565,8 @@ def _resolve_redis_env( ) -> dict[str, Any]: """Read common Redis env vars into a dict.""" host = env_chain(f"{env_prefix}HOST", "REDIS_HOST") or "localhost" - port = parse_port( - env_chain(f"{env_prefix}PORT", "REDIS_PORT"), env_prefix, default_port - ) + port_raw, port_name = env_chain_named(f"{env_prefix}PORT", "REDIS_PORT") + port = parse_port(port_raw, port_name, default_port) # BLANK MEANS UNSET, like parse_db and _parse_bool in this same module — a # nested os.getenv(prefixed, generic) takes the blank and never consults the # generic one. workers/sample.env ships `CACHE_REDIS_PASSWORD=` uncommented, @@ -572,10 +590,13 @@ def _resolve_redis_env( # turning TLS on platform-wide meant remembering CACHE_REDIS_SSL and # MANUAL_REVIEW_REDIS_SSL too — and a missed one fails as a plaintext client # talking to a TLS port, not as a config error. - ssl = ( - os.getenv(f"{env_prefix}SSL", os.getenv("REDIS_SSL", "false")).strip().lower() - == "true" - ) + # BLANK MEANS UNSET here too. It did not, and the asymmetry was dangerous + # rather than merely untidy: once password/user fell through a blank prefixed + # value but TLS did not, a blank CACHE_REDIS_SSL beside REDIS_SSL=true gave + # the worker cache a real AUTH over an UNENCRYPTED socket — the credential + # this same commit taught it to inherit, now on the wire in clear. + ssl_raw, ssl_name = env_chain_named(f"{env_prefix}SSL", "REDIS_SSL") + ssl = _parse_bool(ssl_raw, False, ssl_name) own_url = os.getenv(f"{env_prefix}URL", "").strip() generic_url = os.getenv("REDIS_URL", "").strip() result: dict[str, Any] = { @@ -644,8 +665,8 @@ def _resolve_redis_env( # # Needed where the server's CA is not publicly trusted — notably Memorystore, # whose CA is Google-managed. ElastiCache and Azure chain to public CAs. - ca_certs = os.getenv( - f"{env_prefix}SSL_CA_CERTS", os.getenv("REDIS_SSL_CA_CERTS", "") + ca_certs = ( + env_chain(f"{env_prefix}SSL_CA_CERTS", "REDIS_SSL_CA_CERTS") or "" ).strip() if ca_certs: result["ssl_ca_certs"] = ca_certs diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 211dac59a1..3e13621980 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -11,6 +11,7 @@ None of those raise at import, so they are asserted here instead. """ +import logging import pathlib import re from urllib.parse import unquote, urlsplit @@ -18,6 +19,10 @@ import pytest import redis from unstract.core.cache.redis_client import ( + url_username_from_env, + parse_port, + env_chain_named, + env_chain, _build_connection_kwargs, _resolve_redis_env, build_socketio_redis_url, @@ -573,11 +578,25 @@ class TestContainerAllowlists: def _shared(cls) -> set[str]: """Every REDIS_* env var redis_client.py reads, minus the excused ones.""" source = cls._CLIENT.read_text() - names = set(re.findall(r'os\.getenv\(\s*"(REDIS_[A-Z_]+)"', source)) - names |= { - f"REDIS_{suffix}" - for suffix in re.findall(r'os\.getenv\(\s*f"\{env_prefix\}([A-Z_]+)"', source) - } + # Scans the ARGUMENTS of every env-reading call, not just os.getenv. + # Keying on os.getenv alone meant that rewriting a read to go through + # env_chain made the variable invisible here — the TLS set dropped out + # of this guard silently, which is the same drift this class exists to + # catch. Scoped to these call names rather than the whole file so a + # variable merely NAMED in a docstring is not demanded of every + # container. + calls = re.findall( + r"(?:os\.getenv|env_chain|env_chain_named)\(" + r"([^()]*(?:\([^()]*\)[^()]*)*)\)", + source, + ) + names: set[str] = set() + for args in calls: + names |= set(re.findall(r'"(REDIS_[A-Z_]+)"', args)) + names |= { + f"REDIS_{suffix}" + for suffix in re.findall(r'f"\{env_prefix\}([A-Z_]+)"', args) + } return names - cls._NOT_FORWARDED def test_the_derived_set_is_not_empty(self): @@ -1193,3 +1212,82 @@ def test_a_blank_user_falls_through_to_the_other_spelling(self, monkeypatch): monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") monkeypatch.setenv("REDIS_PASSWORD", "pw") assert urlsplit(build_socketio_redis_url()).username == "alice" + + +class TestBlankMeansUnsetForTlsToo: + """The convention has to cover TLS, not just credentials. + + Once a blank prefixed PASSWORD fell through to the generic one but a blank + prefixed SSL did not, `CACHE_REDIS_SSL=` beside `REDIS_SSL=true` gave the + worker cache a real AUTH over an UNENCRYPTED socket — the credential the + same change taught it to inherit, now in clear on the wire. + """ + + @pytest.mark.parametrize( + "blank,generic,key,expected", + [ + ("CACHE_REDIS_SSL", ("REDIS_SSL", "true"), "ssl", True), + ( + "CACHE_REDIS_SSL_CERT_REQS", + ("REDIS_SSL_CERT_REQS", "none"), + "ssl_cert_reqs", + "none", + ), + ( + "CACHE_REDIS_SSL_CHECK_HOSTNAME", + ("REDIS_SSL_CHECK_HOSTNAME", "false"), + "ssl_check_hostname", + False, + ), + ( + "CACHE_REDIS_SSL_CA_CERTS", + ("REDIS_SSL_CA_CERTS", "/etc/ca.pem"), + "ssl_ca_certs", + "/etc/ca.pem", + ), + ], + ) + def test_a_blank_prefixed_tls_var_does_not_shadow( + self, monkeypatch, blank, generic, key, expected + ): + monkeypatch.setenv(generic[0], generic[1]) + monkeypatch.setenv(blank, "") + assert _resolve_redis_env("CACHE_REDIS_")[key] == expected + + +class TestEnvChainHelpers: + """The helpers the consumers share. Each had a one-line failure mode.""" + + def test_env_chain_returns_the_raw_value_not_a_stripped_one(self, monkeypatch): + """A file-mounted secret ends in a newline. Stripping the RETURN value + truncated it here while backend/settings/base.py read the same variable + unstripped, so one process authenticated two different ways.""" + monkeypatch.setenv("REDIS_PASSWORD", "s3cret\n") + assert env_chain("REDIS_PASSWORD") == "s3cret\n" + + def test_env_chain_skips_whitespace_only_values(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_PASSWORD", " ") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert env_chain("CACHE_REDIS_PASSWORD", "REDIS_PASSWORD") == "s3cret" + + def test_env_chain_named_reports_the_variable_that_was_set(self, monkeypatch): + """So a warning cannot send an operator grepping for an unset key.""" + monkeypatch.delenv("CACHE_REDIS_PORT", raising=False) + monkeypatch.setenv("REDIS_PORT", "6380") + assert env_chain_named("CACHE_REDIS_PORT", "REDIS_PORT") == ("6380", "REDIS_PORT") + + def test_url_username_from_env_honours_the_same_blank_rule(self, monkeypatch): + monkeypatch.setenv("REDIS_USER", " ") + monkeypatch.setenv("REDIS_USERNAME", "alice") + assert url_username_from_env() == "alice" + + def test_parse_port_treats_blank_as_unset(self): + assert parse_port(None, "REDIS_PORT", 6379) == 6379 + + def test_parse_port_falls_back_on_an_unparseable_value(self, caplog): + with caplog.at_level(logging.ERROR): + assert parse_port("638O", "REDIS_PORT", 6379) == 6379 + assert "REDIS_PORT" in caplog.text + + def test_parse_port_returns_an_int(self): + assert parse_port("6380", "REDIS_PORT", 6379) == 6380 From 73b43c0734d6319ddb561ef063e6c8fd927f3522 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 16:00:58 +0530 Subject: [PATCH 07/12] UN-4123 [FIX] Round 4: the SSL flag parses the same in both consumers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 broadened REDIS_SSL parsing in core and left backend/settings/base.py on its own `== "true"`. _TRUE_LITERALS accepts 1, yes and on, so the two halves of one process disagreed about whether TLS was on: REDIS_SSL=1 -> every create_redis_client consumer connects rediss:// the Django cache builds a redis:// LOCATION and `if REDIS_SSL` never runs, so CONNECTION_POOL_KWARGS carries no cert_reqs, no check_hostname, no CA either Against a TLS-only managed endpoint the backend cache alone fails; against one accepting both it authenticates in clear while everything else is encrypted. In the same process SOCKET_IO_MANAGER_URL — built by core — came back rediss://. That is verbatim the "one endpoint, one process, two policies" failure this module was centralised to prevent, reintroduced by centralising only one side. resolve_ssl_enabled is exported and base.py calls it. Also: * The fifth read of SSL_CHECK_HOSTNAME was missed by round 3's conversion. It decides whether the value was EXPLICIT, so a blank prefixed value fell through for the value but not for the explicitness — on Sentinel + TLS a global REDIS_SSL_CHECK_HOSTNAME=true was honoured on the discovery connections and silently dropped on the master one. * parse_port treated "" as unparseable and logged at ERROR. base.py hands it the raw env value rather than env_chain's None, so a bare REDIS_PORT= — the repo's own "leave the default" spelling, and the wording of the comment round 3 added right above it — produced a startup error line in the backend and nowhere else. * host is stripped explicitly. env_chain returns the raw value so a credential keeps whatever whitespace it was given, but urlsplit() drops a trailing newline from a URL host while redis.Redis(host=...) keeps it, so a file-mounted REDIS_HOST would fail DNS on the discrete path and succeed on the URL one. Core 137 -> 139 (276 in the dir), backend 44 -> 57. The new backend class parametrises every true and false literal across both consumers, which is what pins them together; nothing previously exercised any literal but "true". Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 7 ++- .../tests/test_redis_settings_derivation.py | 30 +++++++++++++ .../src/unstract/core/cache/redis_client.py | 44 +++++++++++++++---- .../core/tests/test_redis_client_config.py | 11 ++++- 4 files changed, 81 insertions(+), 11 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index c7c62d5437..3107e7a037 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -28,6 +28,7 @@ parse_db, parse_port, resolve_ssl_cert_reqs, + resolve_ssl_enabled, resolve_ssl_check_hostname, set_url_db_path, url_db_path, @@ -124,7 +125,11 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | # TLS to Redis (UN-4123). Off by default, so the in-cluster/local server is # untouched. `rediss://` is what actually selects TLS for both django-redis and # kombu; this flag only decides which scheme gets built. -REDIS_SSL = os.environ.get("REDIS_SSL", "false").strip().lower() == "true" +# Through the shared resolver, not a local `== "true"`: _TRUE_LITERALS accepts +# 1, yes and on, so the bare comparison read REDIS_SSL=1 as FALSE — the workers +# connected rediss:// while this cache built a redis:// LOCATION and skipped its +# CONNECTION_POOL_KWARGS, i.e. one endpoint with two TLS policies in one process. +REDIS_SSL = resolve_ssl_enabled() # Resolved by unstract.core, not re-read here: the raw value needs trimming, # lower-casing and validating, and a second copy of that logic is how this file # and create_redis_client came to hold two verification policies for one endpoint. diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index 1d07386ca9..c572c83288 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -616,3 +616,33 @@ def test_discrete_mode_still_uses_options(self): "OPTIONS" ] assert options["PASSWORD"] == "s3cret" + + +class TestTheSslFlagParsesTheSameEverywhere: + """REDIS_SSL decides TLS for the whole platform, so every consumer must + read the same literals. A local `== "true"` here read `REDIS_SSL=1` as + FALSE while unstract.core read it as True: the workers connected rediss:// + and this cache built a redis:// LOCATION and skipped its + CONNECTION_POOL_KWARGS entirely — against a TLS-only managed endpoint the + backend cache alone fails, and against one accepting both it authenticates + in clear while everything else is encrypted. + """ + + @pytest.mark.parametrize("literal", ["true", "1", "yes", "on", "TRUE", " on "]) + def test_every_true_literal_switches_the_scheme(self, literal): + derived = _derive(REDIS_HOST="h", REDIS_SSL=literal) + assert derived["CACHES"]["default"]["LOCATION"].startswith("rediss://") + assert derived["REDIS_SSL"] is True + + @pytest.mark.parametrize("literal", ["false", "0", "no", "off", "", " "]) + def test_every_false_and_blank_literal_leaves_it_plaintext(self, literal): + derived = _derive(REDIS_HOST="h", REDIS_SSL=literal) + assert derived["CACHES"]["default"]["LOCATION"].startswith("redis://") + assert derived["REDIS_SSL"] is False + + def test_the_pool_kwargs_follow_the_same_flag(self): + """The scheme and the verification settings must not disagree: the + `if REDIS_SSL` gate guards both, so a literal one reader accepts and + the other does not strips the cert settings as well as the scheme.""" + derived = _derive(REDIS_HOST="h", REDIS_SSL="1") + assert derived["CACHES"]["default"]["OPTIONS"]["CONNECTION_POOL_KWARGS"] diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 8443d678b4..f67b3bc375 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -420,7 +420,11 @@ def parse_port(raw: str | None, var_name: str, default_port: str | int) -> int: the variable nor the prefix, which in most consumers kills the process at import and in the two that catch broadly degrades to no-cache. """ - if raw is None: + # Blank is unset, like everywhere else here. base.py hands this the raw env + # value rather than env_chain's None, so without this a bare `REDIS_PORT=` — + # the repo's own "leave the default" spelling — logged an ERROR at startup + # in the backend and nowhere else. + if raw is None or not raw.strip(): return int(default_port) try: return int(raw) @@ -432,6 +436,20 @@ def parse_port(raw: str | None, var_name: str, default_port: str | int) -> int: return int(default_port) +def resolve_ssl_enabled(env_prefix: str = "REDIS_") -> bool: + """{prefix}SSL, falling back to REDIS_SSL; blank means unset. + + Exported so the backend's settings module uses the SAME parse rather than + its own `== "true"`. _TRUE_LITERALS accepts 1, yes and on, so the bare + comparison read `REDIS_SSL=1` as FALSE — every create_redis_client consumer + connected rediss:// while the Django cache built a redis:// LOCATION and + skipped its CONNECTION_POOL_KWARGS entirely. One endpoint, one process, two + TLS policies, which is the failure this module was centralised to prevent. + """ + raw, name = env_chain_named(f"{env_prefix}SSL", "REDIS_SSL") + return _parse_bool(raw, False, name) + + def url_username_from_env(env_prefix: str = "REDIS_") -> str | None: """The ACL username for a URL, in BOTH spellings that ship. @@ -564,7 +582,12 @@ def _resolve_redis_env( env_prefix: str, default_port: str = "6379", db_override: int | None = None ) -> dict[str, Any]: """Read common Redis env vars into a dict.""" - host = env_chain(f"{env_prefix}HOST", "REDIS_HOST") or "localhost" + # Stripped explicitly: env_chain returns the RAW value so a credential keeps + # any whitespace it was given, but a host must not. urlsplit() drops a + # trailing newline from a URL host while redis.Redis(host=...) keeps it, so + # a file-mounted REDIS_HOST ending in \n would fail DNS on the discrete path + # and succeed on the URL one. + host = (env_chain(f"{env_prefix}HOST", "REDIS_HOST") or "localhost").strip() port_raw, port_name = env_chain_named(f"{env_prefix}PORT", "REDIS_PORT") port = parse_port(port_raw, port_name, default_port) # BLANK MEANS UNSET, like parse_db and _parse_bool in this same module — a @@ -595,8 +618,7 @@ def _resolve_redis_env( # value but TLS did not, a blank CACHE_REDIS_SSL beside REDIS_SSL=true gave # the worker cache a real AUTH over an UNENCRYPTED socket — the credential # this same commit taught it to inherit, now on the wire in clear. - ssl_raw, ssl_name = env_chain_named(f"{env_prefix}SSL", "REDIS_SSL") - ssl = _parse_bool(ssl_raw, False, ssl_name) + ssl = resolve_ssl_enabled(env_prefix) own_url = os.getenv(f"{env_prefix}URL", "").strip() generic_url = os.getenv("REDIS_URL", "").strip() result: dict[str, Any] = { @@ -693,8 +715,14 @@ def _resolve_redis_env( # check_hostname is True while verify_mode is CERT_NONE. Decided from the # EFFECTIVE value — the URL's query string outranks the env var, and testing # the env var alone is what produced that ValueError on every connection. - raw_check_hostname = os.getenv( - f"{env_prefix}SSL_CHECK_HOSTNAME", os.getenv("REDIS_SSL_CHECK_HOSTNAME", "") + # The SAME read as resolve_ssl_check_hostname above — this one decides + # whether the value was EXPLICIT, and leaving it on the nested os.getenv + # made a blank prefixed value fall through for the value but not for the + # explicitness. On a Sentinel + TLS deployment that meant a global + # REDIS_SSL_CHECK_HOSTNAME=true was honoured on the discovery connections + # and silently dropped on the master one. + raw_check_hostname, _ = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" ) # Whether the operator ASKED for a value, as opposed to inheriting the # default. Sentinel's master plane needs to know the difference — see @@ -703,8 +731,8 @@ def _resolve_redis_env( # verification back on for Sentinel masters, which is the breakage this # distinction exists to avoid. result["ssl_check_hostname_explicit"] = ( - raw_check_hostname.strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS - ) + raw_check_hostname or "" + ).strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS if effective_cert_reqs(result["url"], result["ssl_cert_reqs"]) == "none": result["ssl_check_hostname"] = False result["ssl_check_hostname_explicit"] = False diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 3e13621980..7411369dfc 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -1281,8 +1281,15 @@ def test_url_username_from_env_honours_the_same_blank_rule(self, monkeypatch): monkeypatch.setenv("REDIS_USERNAME", "alice") assert url_username_from_env() == "alice" - def test_parse_port_treats_blank_as_unset(self): - assert parse_port(None, "REDIS_PORT", 6379) == 6379 + @pytest.mark.parametrize("raw", [None, "", " "]) + def test_parse_port_treats_blank_as_unset(self, raw, caplog): + """Quietly, too. base.py hands this the raw env value rather than + env_chain's None, so treating "" as unparseable logged an ERROR at + startup in the backend and nowhere else — for the repo's own + "leave the default" spelling.""" + with caplog.at_level(logging.ERROR): + assert parse_port(raw, "REDIS_PORT", 6379) == 6379 + assert caplog.text == "" def test_parse_port_falls_back_on_an_unparseable_value(self, caplog): with caplog.at_level(logging.ERROR): From aab18e9031fe424371d3089b8df30d04d9cc0465 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:31:32 +0000 Subject: [PATCH 08/12] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- backend/backend/settings/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 3107e7a037..cbc50a7642 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -28,8 +28,8 @@ parse_db, parse_port, resolve_ssl_cert_reqs, - resolve_ssl_enabled, resolve_ssl_check_hostname, + resolve_ssl_enabled, set_url_db_path, url_db_path, url_username_from_env, From 41463ba1561266aac12726064b273e2ac7994539 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 16:03:28 +0530 Subject: [PATCH 09/12] UN-4123 [FIX] Pin the property, not the bug: a consumer-parity sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review rounds produced the same finding four times in different clothes — a setting centralised in unstract.core with the parallel read in base.py left behind. The "@" credential heuristic, the REDIS_USERNAME spelling, blank-means-unset, and the SSL true-literals were each fixed with a test for that one setting, and each time the next round found the next one. So this pins the property instead. TestTheBackendAgreesWithCoreOnEverySharedSetting runs 19 environments an operator would actually write and asserts that where the Django cache ends up — host, port, db, username, password, TLS — is where create_redis_client in the same process ends up. A setting centralised on one side only now fails without anyone having to predict which setting it will be. It found a real divergence on its first run, and not one of the four above: the DISCRETE path cannot carry an ACL username. django-redis discards OPTIONS["USERNAME"], so with REDIS_HOST + REDIS_USER + REDIS_PASSWORD the cache authenticated as the built-in `default` while every other client in the process authenticated as the configured user — and where `default` is disabled, the cache alone failed. That is the identical bug the URL branch was fixed for, left standing on the path the module docstring calls primary. The comment beside it said this was deliberate ("auth stays password-only as the built-in `default` user ... this cache never sends one"), and a test pinned it as a "stated invariant" so a django-redis bump would be visible. Passing a key the library throws away is not an invariant worth pinning; reaching the same server as everything else is. Both branches now build the LOCATION through apply_url_credentials. The password still goes to OPTIONS["PASSWORD"] when no username is in play: that key matches Django's SafeExceptionReporterFilter and is masked in a settings dump, while LOCATION is not — so the extra exposure is paid only where django-redis leaves no alternative. Backend 57 -> 77. Three mutations verified caught by the sweep, including two it was not specifically written for. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 42 ++++-- .../tests/test_redis_settings_derivation.py | 140 ++++++++++++++++-- 2 files changed, 152 insertions(+), 30 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index cbc50a7642..981348132b 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -655,23 +655,35 @@ def filter(self, record): # So the db has to travel in the URL path, or this cache silently sits on db 0 # while every other service honours REDIS_DB: workers would RPUSH # log_history_queue to db N and the backend would LPOP an empty db 0. - # USERNAME and DB below are passed for readability and are DISCARDED by - # django-redis — they are not the mechanism for either. Auth stays password-only - # as the built-in `default` user (what a managed AUTH string is): a username - # would turn AUTH into its two-argument ACL form, and this cache never sends - # one. If a django-redis bump ever starts reading USERNAME, that becomes a real - # behaviour change rather than a silent one — which is what the assertions in - # tests/test_redis_settings_derivation.py pin. + # DB below is passed for readability and is DISCARDED by django-redis — it is + # not the mechanism. An ACL USERNAME is not passed through OPTIONS at all, + # because django-redis would discard that too: it travels in the LOCATION, + # the same route the URL branch uses. This used to say auth "stays + # password-only as the built-in `default` user ... this cache never sends + # one", which described the discrete path accurately and made it the one + # consumer that could not reach an ACL endpoint — create_redis_client in the + # same process sends the username, so where `default` is disabled the cache + # alone failed. That is the identical divergence the URL branch was fixed + # for, left standing on the path the module docstring calls primary. _cache_options = { "CLIENT_CLASS": "django_redis.client.DefaultClient", "SERIALIZER": "django_redis.serializers.json.JSONSerializer", } + _cache_username = url_username_from_env() if not _redis_url: - # Credentials and db travel IN the URL in URL mode; passing them again - # through OPTIONS risks one of them winning over the other. _cache_options["DB"] = _cache_db - _cache_options["USERNAME"] = REDIS_USER - _cache_options["PASSWORD"] = REDIS_PASSWORD + _cache_location = f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}" + if _cache_username: + # Only when a username must travel. Keeping the password in OPTIONS + # otherwise is deliberate: OPTIONS["PASSWORD"] matches Django's + # SafeExceptionReporterFilter and is masked in a settings dump, + # while LOCATION is not. Pay that exposure only where django-redis + # leaves no alternative. + _cache_location = apply_url_credentials( + _cache_location, REDIS_PASSWORD, _cache_username + ) + else: + _cache_options["PASSWORD"] = REDIS_PASSWORD else: # URL mode: the credentials go into the LOCATION, through the SAME helper # the Socket.IO URL uses. Two reasons it is not OPTIONS["PASSWORD"]: @@ -707,9 +719,8 @@ def filter(self, record): # Re-deriving the chain here is the same second copy this commit removed # for the "@" heuristic; asking core for it is what keeps the three # consumers in step by construction rather than by comment. - _redis_url = apply_url_credentials( - _redis_url, REDIS_PASSWORD, url_username_from_env() - ) + _redis_url = apply_url_credentials(_redis_url, REDIS_PASSWORD, _cache_username) + _cache_location = _redis_url # Gated on the EFFECTIVE scheme, not the flag. In URL mode TLS is carried by # the URL (and its query string), so a plaintext REDIS_URL left behind while # REDIS_SSL=true would otherwise hand ssl_cert_reqs to a plain @@ -760,8 +771,7 @@ def filter(self, record): CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": _redis_url - or f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", + "LOCATION": _cache_location, "OPTIONS": _cache_options, "KEY_FUNCTION": "utils.redis_cache.custom_key_function", } diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index c572c83288..dc1761d5ef 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -20,7 +20,7 @@ import logging import pathlib import re -from urllib.parse import urlsplit +from urllib.parse import unquote, urlsplit import pytest @@ -174,20 +174,35 @@ def test_the_password_reaches_the_cache(self): derived = _derive(REDIS_HOST="h", REDIS_PASSWORD="s3cret") assert derived["CACHES"]["default"]["OPTIONS"]["PASSWORD"] == "s3cret" - def test_db_and_username_are_passed_through_options(self): - """Pins the stated invariant so a django-redis bump is visible. - - The comment beside this code says USERNAME is deliberately not honoured — - django-redis 5.4.0 discards it, so auth stays password-only as the - built-in `default` user. That holds by accident of the pinned version: - these assertions pin what the settings SEND, so if a bump starts reading - USERNAME the change is a deliberate one rather than a surprise. + def test_an_acl_username_travels_in_the_location_not_options(self): + """This used to assert OPTIONS["USERNAME"] == "alice" and call that the + stated invariant — django-redis discards the key, so auth "stays + password-only as the built-in `default` user". + + That made the discrete path the one consumer unable to reach an ACL + endpoint: create_redis_client in the same process sends the username, so + where `default` is disabled the cache alone failed. Passing a key the + library throws away is not an invariant worth pinning; reaching the same + server as everything else is. The LOCATION is the only route django-redis + leaves, which is exactly why the URL branch already used it. """ - options = _derive(REDIS_HOST="h", REDIS_DB="3", REDIS_USER="alice")["CACHES"][ - "default" - ]["OPTIONS"] - assert options["DB"] == 3 - assert options["USERNAME"] == "alice" + derived = _derive( + REDIS_HOST="h", REDIS_DB="3", REDIS_USER="alice", REDIS_PASSWORD="pw" + ) + cache = derived["CACHES"]["default"] + assert cache["OPTIONS"]["DB"] == 3 + assert "USERNAME" not in cache["OPTIONS"] + parts = urlsplit(cache["LOCATION"]) + assert parts.username == "alice" + assert parts.password == "pw" + assert parts.hostname == "h" + + def test_no_username_keeps_the_password_out_of_the_location(self): + """OPTIONS["PASSWORD"] is masked by Django's settings filter; LOCATION is + not. Pay that exposure only where django-redis leaves no alternative.""" + cache = _derive(REDIS_HOST="h", REDIS_PASSWORD="pw")["CACHES"]["default"] + assert cache["OPTIONS"]["PASSWORD"] == "pw" + assert urlsplit(cache["LOCATION"]).password is None def test_url_mode_does_not_duplicate_credentials_into_options(self): """They travel in the URL; passing both risks one winning over the other.""" @@ -646,3 +661,100 @@ def test_the_pool_kwargs_follow_the_same_flag(self): the other does not strips the cert settings as well as the scheme.""" derived = _derive(REDIS_HOST="h", REDIS_SSL="1") assert derived["CACHES"]["default"]["OPTIONS"]["CONNECTION_POOL_KWARGS"] + + +class TestTheBackendAgreesWithCoreOnEverySharedSetting: + """A parity sweep, not another one-off. + + Four review rounds on this PR produced the same finding four times in + different clothes: a setting was centralised in unstract.core and the + parallel read in this settings module was left behind — the "@" credential + heuristic, the REDIS_USERNAME spelling, blank-means-unset, and the SSL + true-literals. Each was fixed with a test for that one setting, and the next + round found the next one. + + This pins the PROPERTY instead: for a matrix of environments, what the + Django cache ends up talking to must match what create_redis_client in the + same process would. A future setting that is centralised on one side only + fails here without anyone having to predict which setting it will be. + """ + + # Each case is what an operator actually writes, not a minimal pair. + _CASES = [ + {"REDIS_HOST": "h"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USER": "alice"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USERNAME": "alice"}, + {"REDIS_HOST": "h", "REDIS_SSL": "true"}, + {"REDIS_HOST": "h", "REDIS_SSL": "1"}, + {"REDIS_HOST": "h", "REDIS_SSL": "yes", "REDIS_SSL_CERT_REQS": "none"}, + {"REDIS_URL": "redis://h:6379/0"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_PASSWORD": "pw", "REDIS_USER": "alice"}, + { + "REDIS_URL": "redis://h:6379/0", + "REDIS_PASSWORD": "pw", + "REDIS_USERNAME": "bob", + }, + {"REDIS_URL": "redis://alice@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://:inurl@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://:@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "p@ss/w:rd"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_DB": "3", "REDIS_PASSWORD": "pw"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USER": ""}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_SSL": ""}, + ] + + @staticmethod + def _from_core(env: dict) -> dict: + """What create_redis_client would connect with, for this environment.""" + import os + + from unstract.core.cache.redis_client import create_redis_client + + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + kwargs = create_redis_client().connection_pool.connection_kwargs + cls = create_redis_client().connection_pool.connection_class.__name__ + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + return { + "host": kwargs.get("host"), + "port": kwargs.get("port"), + "db": int(kwargs.get("db") or 0), + "username": kwargs.get("username") or None, + "password": kwargs.get("password") or None, + "tls": cls == "SSLConnection", + } + + @staticmethod + def _from_cache(env: dict) -> dict: + """The same facts, read back off the Django cache LOCATION.""" + derived = _derive(**env) + location = derived["CACHES"]["default"]["LOCATION"] + parts = urlsplit(location) + options = derived["CACHES"]["default"]["OPTIONS"] + return { + "host": parts.hostname, + "port": parts.port or int(derived["REDIS_PORT"]), + "db": int((parts.path or "/0").lstrip("/") or 0), + # django-redis discards OPTIONS["USERNAME"], so the LOCATION is the + # only route for a username — that asymmetry is the whole reason the + # URL branch exists, and it is what this sweep exists to keep true. + "username": unquote(parts.username) if parts.username else None, + "password": unquote(parts.password) + if parts.password + else (options.get("PASSWORD") or None), + "tls": parts.scheme == "rediss", + } + + @pytest.mark.parametrize("env", _CASES, ids=lambda e: ",".join(sorted(e))) + def test_the_cache_and_the_client_reach_the_same_place(self, env): + assert self._from_cache(env) == self._from_core(env) From 815460ead3316a1420afb84bc6d98a5f938b9fa0 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 16:17:32 +0530 Subject: [PATCH 10/12] UN-4123 [FIX] Round 5: the sweep asks django-redis instead of imitating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 found no Critical and no High — the recurring "centralised on one side only" class has stopped on the path the parity sweep covers, confirmed against an exhaustive 2160-configuration matrix. These are the Mediums it did raise. * The sweep reimplemented django-redis's precedence. It parsed the LOCATION with urlsplit and hardcoded "userinfo wins, OPTIONS['PASSWORD'] backfills". That is correct for 5.4.0 — but it is a second copy of library behaviour inside the one test whose stated job is to survive changes it cannot predict, so a bump that reversed the precedence would leave it green while the cache diverged. It now builds the dict from a real ConnectionFactory, so the rule is never restated. (Its pools are memoised by LOCATION, so they are cleared per case; leaving that out makes three cases answer with an earlier case's pool, verified.) * An ACL username with NO password reached neither side usefully and reached them differently: apply_url_credentials returns the URL untouched when there is no password, so the cache went anonymous, while core set username=alice and redis-py packed `AUTH alice None` and raised DataError on the FIRST command — a type error from inside the library, nowhere near the values file that caused it. Redis has no one-argument ACL AUTH, so this is not a configuration that exists; it is now reported at startup and the username dropped, which also makes the two sides agree. The sweep's 19 cases paired a username with a password every time, so it could not see this; two cases added. * The db relocation warning gave the discrete-mode explanation on both paths — it fires in URL mode too, where the previous db came from the URL's own path and OPTIONS['DB'] was never the mechanism. The db numbers were right in every case; only the stated reason was wrong. Reason dropped, numbers kept. The Sentinel branch is now marked as OUTSIDE the sweep, with the four divergences from create_redis_client that live there spelled out — the "default" username default, the ignored REDIS_USERNAME spelling, no TLS at all, and a different default port. All four are pre-existing on main and this PR only replaced an int() there; changing behaviour in a branch no test in this file executes is its own ticket, not this one. Backend 77 -> 79. Two mutations verified caught. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 27 ++++++++-- .../tests/test_redis_settings_derivation.py | 52 +++++++++++++------ .../src/unstract/core/cache/redis_client.py | 19 +++++++ 3 files changed, 78 insertions(+), 20 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 981348132b..f142ac5cc0 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -563,6 +563,19 @@ def filter(self, record): REDIS_SENTINEL_MASTER_NAME = os.environ.get("REDIS_SENTINEL_MASTER_NAME", "mymaster") +# NOT COVERED by TestTheBackendAgreesWithCoreOnEverySharedSetting — _derive only +# exercises the standalone branch below, so the "cache and client reach the same +# place" property stops at this line. Four divergences from create_redis_client +# live in here and are PRE-EXISTING on main, untouched by UN-4123: +# * REDIS_USER defaults to "default" above, so this cache sends a two-argument +# ACL AUTH where core sends the one-argument form; +# * it reads REDIS_USER directly, so the REDIS_USERNAME spelling is ignored; +# * REDIS_SSL is not applied here at all — a plaintext cache against a +# TLS-only Sentinel deployment; +# * the Sentinel default port is 26379 in core and 6379 here. +# Left alone deliberately: this is the self-hosted HA path, a managed endpoint +# is a single primary and does not use it, and changing behaviour here would be +# changing code no test in this file executes. Its own ticket. if REDIS_SENTINEL_MODE: _sentinel_kwargs = {} if REDIS_PASSWORD: @@ -758,11 +771,17 @@ def filter(self, record): # counters and the dashboard caches go with it, and during a rolling # deploy old pods read db 0 while new pods read db N. Drain # log_history_queue before cutting over. + # + # The message deliberately does NOT explain where the previous db came + # from. It fires on both paths — discrete, where the cache sat on db 0 + # regardless because django-redis ignores OPTIONS['DB'], and URL mode, + # where it came from the URL's own path — and the earlier wording gave + # the discrete explanation to both. The db NUMBERS are right in every + # case; only the stated reason was wrong on the URL path. logging.getLogger(__name__).warning( - "Django cache is moving from Redis db %s to db %s (REDIS_DB). In " - "discrete mode it previously sat on db 0 regardless, because " - "django-redis ignores OPTIONS['DB']. Anything already in db %s — " - "including log_history_queue — stays there.", + "Django cache is moving from Redis db %s to db %s (REDIS_DB). " + "Anything already in db %s — including log_history_queue — stays " + "there.", _previous_db, _effective_db, _previous_db, diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index dc1761d5ef..6b4c649156 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -20,7 +20,7 @@ import logging import pathlib import re -from urllib.parse import unquote, urlsplit +from urllib.parse import urlsplit import pytest @@ -702,6 +702,11 @@ class TestTheBackendAgreesWithCoreOnEverySharedSetting: {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "pw"}, {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "p@ss/w:rd"}, {"REDIS_URL": "redis://h:6379/0", "REDIS_DB": "3", "REDIS_PASSWORD": "pw"}, + # Username with NO password: neither side can honour it, so what the + # sweep pins is that they fail the SAME way rather than one going + # anonymous while the other crashes inside redis-py. + {"REDIS_HOST": "h", "REDIS_USER": "alice"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_USERNAME": "alice"}, {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USER": ""}, {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_SSL": ""}, ] @@ -736,23 +741,38 @@ def _from_core(env: dict) -> dict: @staticmethod def _from_cache(env: dict) -> dict: - """The same facts, read back off the Django cache LOCATION.""" + """The same facts, resolved by django-redis ITSELF. + + Not by parsing the LOCATION string. django-redis's precedence — the + LOCATION's userinfo wins, OPTIONS["PASSWORD"] only backfills — is true + of 5.4.0, but hardcoding it here would be a second copy of library + behaviour inside the very test whose job is to survive changes it cannot + predict. A bump that reversed the precedence would leave this green + while the cache diverged, which is the exact failure mode this class + replaced. Asking ConnectionFactory means the rule is never restated. + """ + from django_redis.pool import ConnectionFactory + derived = _derive(**env) - location = derived["CACHES"]["default"]["LOCATION"] - parts = urlsplit(location) - options = derived["CACHES"]["default"]["OPTIONS"] + cache = derived["CACHES"]["default"] + options = { + k: v + for k, v in cache["OPTIONS"].items() + if k not in ("CLIENT_CLASS", "SERIALIZER") + } + # The factory memoises pools by LOCATION, so a stale entry from an + # earlier case would answer for this one. + ConnectionFactory._pools = {} + client = ConnectionFactory(dict(options)).connect(cache["LOCATION"]) + pool = client.connection_pool + kwargs = pool.connection_kwargs return { - "host": parts.hostname, - "port": parts.port or int(derived["REDIS_PORT"]), - "db": int((parts.path or "/0").lstrip("/") or 0), - # django-redis discards OPTIONS["USERNAME"], so the LOCATION is the - # only route for a username — that asymmetry is the whole reason the - # URL branch exists, and it is what this sweep exists to keep true. - "username": unquote(parts.username) if parts.username else None, - "password": unquote(parts.password) - if parts.password - else (options.get("PASSWORD") or None), - "tls": parts.scheme == "rediss", + "host": kwargs.get("host"), + "port": int(kwargs.get("port") or derived["REDIS_PORT"]), + "db": int(kwargs.get("db") or 0), + "username": kwargs.get("username") or None, + "password": kwargs.get("password") or None, + "tls": pool.connection_class.__name__ == "SSLConnection", } @pytest.mark.parametrize("env", _CASES, ids=lambda e: ",".join(sorted(e))) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index f67b3bc375..3b2208752d 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -602,6 +602,25 @@ def _resolve_redis_env( username = env_chain( f"{env_prefix}USER", f"{env_prefix}USERNAME", "REDIS_USER", "REDIS_USERNAME" ) + # An ACL username without a password is not a configuration Redis has. AUTH + # takes either one argument or two, so redis-py packs `AUTH alice None` and + # raises `DataError: Invalid input of type: 'NoneType'` on the FIRST command + # — a type error from inside the library, nowhere near the values file that + # caused it. Said plainly at configuration time instead, and the username is + # dropped so this client degrades to the same anonymous connection the + # Django cache already makes for this input (apply_url_credentials returns + # the URL untouched when there is no password), rather than the two + # disagreeing about a config neither can honour. + if username and not password: + logger.error( + "%sUSER=%r is set but no password is; Redis has no one-argument ACL " + "AUTH, so the username is being ignored. Set %sPASSWORD, or clear " + "the username.", + env_prefix, + username, + env_prefix, + ) + username = None prefixed_db = os.getenv(f"{env_prefix}DB", "").strip() generic_db = os.getenv("REDIS_DB", "").strip() db = ( From 9019ac13bc56677c4097f0bdf94fa01202663c9d Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 25 Sep 2026 17:11:41 +0530 Subject: [PATCH 11/12] UN-4123 [FIX] Extend the cache/client parity to Sentinel mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parity sweep stopped at the Sentinel branch, and I recorded the four divergences living there as "its own ticket". That was the wrong call twice over: a guarantee that stops at a mode boundary is a half-truth, and the reason I gave for deferring — "changing code no test in this file executes" — was not true. _derive's slice spans BOTH branches; it just needed an env selecting this one. "No test executes it" and "no test was written for it" are different statements, and only the second was ever the case. Four divergences from create_redis_client, all pre-existing on main: * THE SENTINEL PORT. The module-level REDIS_PORT defaults to 6379 — the STANDALONE port — while core's Sentinel path resolves with default_port="26379". With REDIS_PORT unset this cache looked for sentinels on the wrong port while every other client found them. The chart always sets REDIS_PORT, which is why it stayed hidden. * THE USERNAME, twice. REDIS_USER defaults to "default" at module scope, so this cache sent a two-argument ACL AUTH where core sends the one-argument form; and it read REDIS_USER directly, so the REDIS_USERNAME spelling platform-service ships was ignored. Both now come from the shared resolver, and the same value reaches SENTINEL_KWARGS, the LOCATION and the Socket.IO URL. * TLS REACHED NEITHER CONNECTION. A TLS-only Sentinel deployment got a plaintext cache while core encrypted both the discovery and the master connections. Carrying it in SENTINEL_KWARGS alone would encrypt discovery and leave the master in clear, so it goes to CONNECTION_POOL_KWARGS too — core builds both from one env dict for exactly this reason. BEHAVIOUR CHANGES for existing Sentinel deployments, stated rather than buried: an unset REDIS_PORT now resolves 26379 instead of 6379; no username is sent unless one is configured (Redis 6+ treats `AUTH default ` and `AUTH ` as the same command, and redis-py retries one-arg regardless, so this is a wire change rather than an auth change); and REDIS_SSL now applies here. Flag off is byte-identical — asserted. Backend 79 -> 87. Four mutations verified caught. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 65 +++++++--- .../tests/test_redis_settings_derivation.py | 114 ++++++++++++++++++ 2 files changed, 161 insertions(+), 18 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index f142ac5cc0..17fff429a1 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -563,32 +563,57 @@ def filter(self, record): REDIS_SENTINEL_MASTER_NAME = os.environ.get("REDIS_SENTINEL_MASTER_NAME", "mymaster") -# NOT COVERED by TestTheBackendAgreesWithCoreOnEverySharedSetting — _derive only -# exercises the standalone branch below, so the "cache and client reach the same -# place" property stops at this line. Four divergences from create_redis_client -# live in here and are PRE-EXISTING on main, untouched by UN-4123: -# * REDIS_USER defaults to "default" above, so this cache sends a two-argument -# ACL AUTH where core sends the one-argument form; -# * it reads REDIS_USER directly, so the REDIS_USERNAME spelling is ignored; -# * REDIS_SSL is not applied here at all — a plaintext cache against a -# TLS-only Sentinel deployment; -# * the Sentinel default port is 26379 in core and 6379 here. -# Left alone deliberately: this is the self-hosted HA path, a managed endpoint -# is a single primary and does not use it, and changing behaviour here would be -# changing code no test in this file executes. Its own ticket. if REDIS_SENTINEL_MODE: + # This branch used to hold four divergences from create_redis_client, all + # pre-existing. They are closed here because "the cache and the client reach + # the same place" is not a property that can stop at a mode boundary — an + # operator on Sentinel gets the same guarantee or the guarantee is a + # half-truth. TestSentinelModeAgreesWithCore covers each one. + # + # The username comes from the shared resolver, not the module-level + # REDIS_USER: that one defaults to "default", so this cache sent a + # two-argument ACL AUTH where core sends the one-argument form, and it read + # only REDIS_USER so the REDIS_USERNAME spelling platform-service ships was + # ignored entirely. + _sentinel_username = url_username_from_env() + + # 26379 — the Sentinel port — matching core's + # _resolve_redis_env(default_port="26379"). The module-level REDIS_PORT + # defaults to 6379, which is the standalone port, so an unset REDIS_PORT + # pointed this cache at the wrong port while every other client found the + # sentinels. The chart always sets it, which is why this stayed hidden. + REDIS_PORT = parse_port(os.environ.get("REDIS_PORT"), "REDIS_PORT", 26379) + _sentinel_kwargs = {} if REDIS_PASSWORD: _sentinel_kwargs["password"] = REDIS_PASSWORD - if REDIS_USER: - _sentinel_kwargs["username"] = REDIS_USER + if _sentinel_username: + _sentinel_kwargs["username"] = _sentinel_username + + # TLS reached neither the discovery connections nor the master one, so a + # TLS-only Sentinel deployment got a plaintext cache while core encrypted + # both. Core builds them from one env dict for exactly this reason; the + # same settings go to both here. + _sentinel_pool_kwargs = {} + if REDIS_SSL: + _sentinel_tls = { + "ssl": True, + "ssl_cert_reqs": REDIS_SSL_CERT_REQS, + "ssl_check_hostname": REDIS_SSL_CHECK_HOSTNAME, + } + if REDIS_SSL_CA_CERTS: + _sentinel_tls["ssl_ca_certs"] = REDIS_SSL_CA_CERTS + _sentinel_kwargs.update(_sentinel_tls) + _sentinel_pool_kwargs.update(_sentinel_tls) _redis_db = parse_db(REDIS_DB, "REDIS_") # SocketIO connection manager (Kombu Sentinel URL format) _cred_prefix = "" - if REDIS_USER and REDIS_PASSWORD: - _cred_prefix = f"{quote(REDIS_USER, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" + if _sentinel_username and REDIS_PASSWORD: + _cred_prefix = ( + f"{quote(_sentinel_username, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" + ) elif REDIS_PASSWORD: _cred_prefix = f":{quote(REDIS_PASSWORD, safe='')}@" SOCKET_IO_MANAGER_URL = ( @@ -597,7 +622,7 @@ def filter(self, record): SOCKET_IO_TRANSPORT_OPTIONS = {"master_name": REDIS_SENTINEL_MASTER_NAME} # django-redis expects username in the LOCATION URL for ACL auth - _user_prefix = f"{REDIS_USER}@" if REDIS_USER else "" + _user_prefix = f"{_sentinel_username}@" if _sentinel_username else "" CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", @@ -608,6 +633,10 @@ def filter(self, record): "CONNECTION_FACTORY": "django_redis.pool.SentinelConnectionFactory", "SENTINELS": [(REDIS_HOST, REDIS_PORT)], "SENTINEL_KWARGS": _sentinel_kwargs, + # TLS for the MASTER connection. SENTINEL_KWARGS covers only the + # discovery ones; core carries both, so a deployment with TLS on + # got an encrypted discovery and a plaintext master. + "CONNECTION_POOL_KWARGS": _sentinel_pool_kwargs, "DB": _redis_db, "PASSWORD": REDIS_PASSWORD, "SERIALIZER": "django_redis.serializers.json.JSONSerializer", diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index 6b4c649156..ed49fe6b95 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -778,3 +778,117 @@ def _from_cache(env: dict) -> dict: @pytest.mark.parametrize("env", _CASES, ids=lambda e: ",".join(sorted(e))) def test_the_cache_and_the_client_reach_the_same_place(self, env): assert self._from_cache(env) == self._from_core(env) + + +class TestSentinelModeAgreesWithCore: + """The parity property must not stop at a mode boundary. + + TestTheBackendAgreesWithCoreOnEverySharedSetting cannot cover Sentinel: its + _from_core calls create_redis_client, which in Sentinel mode performs real + discovery and blocks. So the comparison here is against the resolver rather + than a built client — _resolve_redis_env with the same default_port core's + Sentinel path passes — which is enough to pin the four facts that diverged. + + The branch was NOT untestable, which an earlier comment in base.py claimed: + _derive's slice spans both branches, so it just needed an env that selects + this one. "No test executes it" and "no test was written for it" are + different statements, and only the second was true. + """ + + @staticmethod + def _core(**env: str) -> dict: + import os + + from unstract.core.cache.redis_client import _resolve_redis_env + + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + return _resolve_redis_env("REDIS_", default_port="26379") + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + + def test_the_sentinel_port_default_matches_core(self): + """REDIS_PORT unset pointed this cache at 6379 — the STANDALONE port — + while every other client found the sentinels on 26379.""" + env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent"} + sentinels = _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"] + assert sentinels == [("sent", self._core(**env)["port"])] + assert sentinels == [("sent", 26379)] + + def test_an_explicit_port_still_wins(self): + env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent", "REDIS_PORT": "27000"} + assert _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"] == [ + ("sent", 27000) + ] + + def test_no_username_configured_means_no_username_sent(self): + """REDIS_USER defaults to "default" at module scope, so this cache sent + a two-argument ACL AUTH where core sends the one-argument form.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_PASSWORD": "pw", + } + cache = _derive(**env)["CACHES"]["default"] + assert self._core(**env)["username"] is None + assert "username" not in cache["OPTIONS"]["SENTINEL_KWARGS"] + assert urlsplit(cache["LOCATION"]).username is None + + @pytest.mark.parametrize("spelling", ["REDIS_USER", "REDIS_USERNAME"]) + def test_both_username_spellings_reach_the_sentinel_cache(self, spelling): + """platform-service ships REDIS_USERNAME; reading only REDIS_USER left + this cache authenticating as `default` while core used the ACL user.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_PASSWORD": "pw", + spelling: "alice", + } + derived = _derive(**env) + cache = derived["CACHES"]["default"] + assert self._core(**env)["username"] == "alice" + assert cache["OPTIONS"]["SENTINEL_KWARGS"]["username"] == "alice" + assert urlsplit(cache["LOCATION"]).username == "alice" + assert urlsplit(derived["SOCKET_IO_MANAGER_URL"]).username == "alice" + + def test_tls_reaches_both_the_sentinels_and_the_master(self): + """SENTINEL_KWARGS covers only discovery. Carrying TLS there alone would + leave an encrypted discovery and a plaintext master; carrying it nowhere + — which is what this branch did — leaves a plaintext cache against a + TLS-only deployment while core encrypts both.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + "REDIS_SSL_CERT_REQS": "none", + } + options = _derive(**env)["CACHES"]["default"]["OPTIONS"] + assert self._core(**env)["ssl"] is True + for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"): + assert options[where]["ssl"] is True, where + assert options[where]["ssl_cert_reqs"] == "none", where + + def test_a_ca_reaches_both_too(self): + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + "REDIS_SSL_CA_CERTS": "/etc/ssl/ca.pem", + } + options = _derive(**env)["CACHES"]["default"]["OPTIONS"] + for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"): + assert options[where]["ssl_ca_certs"] == "/etc/ssl/ca.pem", where + + def test_no_tls_configured_adds_no_tls_keys(self): + """Flag off must leave the Sentinel cache byte-identical to before.""" + options = _derive(REDIS_SENTINEL_MODE="true", REDIS_HOST="sent")["CACHES"][ + "default" + ]["OPTIONS"] + assert options["SENTINEL_KWARGS"] == {} + assert options["CONNECTION_POOL_KWARGS"] == {} From f0e02ae2b7632273c3fe167afe7a507f89c0c797 Mon Sep 17 00:00:00 2001 From: ali Date: Tue, 29 Sep 2026 09:36:31 +0530 Subject: [PATCH 12/12] UN-4123 [FIX] Sentinel's two TLS planes verify by different rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Valid review finding, and a parity bug in the commit that was meant to establish parity: 9019ac13b built one TLS dict and copied it into both SENTINEL_KWARGS and CONNECTION_POOL_KWARGS. The two planes are not the same connection. Discovery reaches REDIS_HOST, a name whose certificate can match, so it verifies like any other client. The MASTER reaches whatever `SENTINEL get-master-addr-by-name` returns, and SentinelManagedConnection assigns that address straight to self.host, which SSLConnection passes as server_hostname — an IP that no DNS SAN covers, and one that changes on failover, so pinning an IP SAN is not an answer either. core turns checking off there unless the operator asked explicitly (_tls_kwargs's sentinel_master branch). This cache did not, so it failed certificate verification against the very master create_redis_client connects to. The second half of the finding is also real: with REDIS_SSL_CERT_REQS=none the copied default of True produced CERT_NONE plus hostname checking, and the pinned redis-py 5.2.1 does NOT coerce that pair — it assigns check_hostname verbatim, so it reaches Python's ssl module, which rejects it. Verified against the installed version rather than assumed; 6.x does coerce, which is why the sibling repo's fix looks different. The standalone branch already guarded this three hundred lines below; the Sentinel branch did not. The master-plane rule is exported from core as resolve_sentinel_master_check_hostname rather than re-derived here — the same reasoning as every other shared setting in this PR, and the reason it is a new helper instead of a flag on resolve_ssl_check_hostname is that core's own call site applies the cert_reqs rule immediately afterwards and would double-apply it. Backend 87 -> 92. Two mutations verified caught. Co-Authored-By: Claude Opus 5 --- backend/backend/settings/base.py | 22 ++++-- .../tests/test_redis_settings_derivation.py | 74 +++++++++++++++++++ .../src/unstract/core/cache/redis_client.py | 26 +++++++ 3 files changed, 117 insertions(+), 5 deletions(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 17fff429a1..aa03b577ae 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -27,6 +27,7 @@ ensure_tls_query_params, parse_db, parse_port, + resolve_sentinel_master_check_hostname, resolve_ssl_cert_reqs, resolve_ssl_check_hostname, resolve_ssl_enabled, @@ -596,15 +597,26 @@ def filter(self, record): # same settings go to both here. _sentinel_pool_kwargs = {} if REDIS_SSL: - _sentinel_tls = { - "ssl": True, - "ssl_cert_reqs": REDIS_SSL_CERT_REQS, - "ssl_check_hostname": REDIS_SSL_CHECK_HOSTNAME, - } + _sentinel_tls = {"ssl": True, "ssl_cert_reqs": REDIS_SSL_CERT_REQS} if REDIS_SSL_CA_CERTS: _sentinel_tls["ssl_ca_certs"] = REDIS_SSL_CA_CERTS + + # The two planes answer by DIFFERENT rules, and copying one dict into + # both got that wrong. Discovery connects to REDIS_HOST, a name whose + # certificate can match, so it verifies like any other client. The + # MASTER connects to whatever SENTINEL get-master-addr-by-name returns — + # an IP that SentinelManagedConnection hands to SSLConnection as + # server_hostname, which no DNS SAN covers and which changes on + # failover. core turns checking off there unless the operator asked + # explicitly; this cache has to agree, or it fails verification against + # the same master that create_redis_client reaches. _sentinel_kwargs.update(_sentinel_tls) _sentinel_pool_kwargs.update(_sentinel_tls) + if REDIS_SSL_CERT_REQS != "none": + _sentinel_kwargs["ssl_check_hostname"] = REDIS_SSL_CHECK_HOSTNAME + _sentinel_pool_kwargs["ssl_check_hostname"] = ( + resolve_sentinel_master_check_hostname() + ) _redis_db = parse_db(REDIS_DB, "REDIS_") diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index ed49fe6b95..7a474d707f 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -892,3 +892,77 @@ def test_no_tls_configured_adds_no_tls_keys(self): ]["OPTIONS"] assert options["SENTINEL_KWARGS"] == {} assert options["CONNECTION_POOL_KWARGS"] == {} + + +class TestSentinelTlsPlanesDifferCorrectly: + """Discovery and master verify by DIFFERENT rules, and one dict served both. + + Discovery connects to REDIS_HOST — a name whose certificate can match — so + it verifies like any other client. The master connects to whatever + `SENTINEL get-master-addr-by-name` returns, which SentinelManagedConnection + hands to SSLConnection as `server_hostname`: an IP no DNS SAN covers, and + one that changes on failover. core turns checking off there unless asked + explicitly, so this cache must too — otherwise it fails verification against + the very master create_redis_client connects to. + """ + + @staticmethod + def _options(**env: str) -> dict: + return _derive(REDIS_SENTINEL_MODE="true", REDIS_HOST="sent", **env)["CACHES"][ + "default" + ]["OPTIONS"] + + def test_the_master_does_not_verify_the_hostname_by_default(self): + options = self._options(REDIS_SSL="true") + assert options["SENTINEL_KWARGS"]["ssl_check_hostname"] is True + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_an_explicit_request_is_honoured_on_the_master(self): + """An operator who asks for it still gets it — core says the same.""" + options = self._options(REDIS_SSL="true", REDIS_SSL_CHECK_HOSTNAME="true") + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is True + + def test_an_explicit_false_is_honoured_on_both(self): + options = self._options(REDIS_SSL="true", REDIS_SSL_CHECK_HOSTNAME="false") + assert options["SENTINEL_KWARGS"]["ssl_check_hostname"] is False + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_cert_reqs_none_turns_checking_off_on_both_planes(self): + """redis-py 5.2.1 assigns check_hostname verbatim — it does NOT coerce + the pair — so CERT_NONE with checking on reaches Python's ssl module, + which rejects it. Verified against the pinned version.""" + options = self._options(REDIS_SSL="true", REDIS_SSL_CERT_REQS="none") + assert "ssl_check_hostname" not in options["SENTINEL_KWARGS"] + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_the_master_matches_core_for_every_spelling(self): + """Pinned to the resolver core itself uses, not to a second copy.""" + import os + + from unstract.core.cache.redis_client import ( + resolve_sentinel_master_check_hostname, + ) + + for raw in (None, "true", "false", "1", "off"): + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + } + if raw is not None: + env["REDIS_SSL_CHECK_HOSTNAME"] = raw + derived = _derive(**env)["CACHES"]["default"]["OPTIONS"] + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + expected = resolve_sentinel_master_check_hostname() + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + assert derived["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] == expected, ( + raw + ) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 3b2208752d..ca6c438379 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -208,6 +208,32 @@ def resolve_ssl_cert_reqs(env_prefix: str = "REDIS_") -> str: return value +def resolve_sentinel_master_check_hostname(env_prefix: str = "REDIS_") -> bool: + """ssl_check_hostname for a Sentinel-managed MASTER connection. + + The master plane answers by a DIFFERENT rule than the discovery plane, and + exporting it is what stops a consumer re-deriving it: a master connects to + whatever `SENTINEL get-master-addr-by-name` returns, and + SentinelManagedConnection assigns that address straight to ``self.host``, + which SSLConnection passes as ``server_hostname``. It is an IP, so + verification is checked against an address no DNS SAN covers — and pinning + an IP SAN is not a workable answer, because the address changes on failover. + + So it is OFF unless the operator asked for it explicitly, matching + _tls_kwargs(sentinel_master=True). Off as well when verification itself is + off: the pinned redis-py (5.2.1) does NOT coerce that pair — it assigns + check_hostname verbatim — so CERT_NONE with checking on reaches Python's ssl + module, which rejects it. + """ + raw, _ = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" + ) + explicit = (raw or "").strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS + if not explicit or resolve_ssl_cert_reqs(env_prefix) == "none": + return False + return resolve_ssl_check_hostname(env_prefix) + + def resolve_ssl_check_hostname(env_prefix: str = "REDIS_", default: bool = True) -> bool: """{prefix}SSL_CHECK_HOSTNAME, falling back to REDIS_SSL_CHECK_HOSTNAME, then on.