UN-4123 [FIX] A credential-free REDIS_URL no longer connects anonymously - #2299
Merged
Merged
Conversation
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://:<password>@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 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…nsumers 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 <noreply@anthropic.com>
muhammad-ali-e
marked this pull request as draft
September 25, 2026 05:07
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 <noreply@anthropic.com>
…netloc bug in it 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 <noreply@anthropic.com>
…eans unset 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 <noreply@anthropic.com>
…top stripping
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 <pw>` and
`AUTH <pw>` 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
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 <noreply@anthropic.com>
…ng it 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 <noreply@anthropic.com>
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 <pw>` and `AUTH <pw>` 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 <noreply@anthropic.com>
Contributor
Author
Review record — 5 rounds of
|
| Round | Finding | Fixed in |
|---|---|---|
| 2 | base.py read only REDIS_USER, missing the REDIS_USERNAME spelling platform-service ships — the cache authenticated as default while create_redis_client in the same process used the configured ACL user |
3ec96b81e |
| 2 | A blank {PREFIX}PASSWORD defeated the credential fallback. workers/sample.env ships CACHE_REDIS_PASSWORD= uncommented, so this PR's own documented recipe connected every worker cache anonymously |
3ec96b81e |
| 3 | Blank-means-unset was applied to credentials but not the four TLS vars. That asymmetry was worse than the original bug: a blank CACHE_REDIS_SSL beside REDIS_SSL=true sent a real AUTH over an unencrypted socket |
9893110f4 |
| 3 | env_chain stripped its return value and base.py did not, so a secret with a trailing newline gave the Socket.IO URL and the cache LOCATION different passwords |
9893110f4 |
| 4 | base.py kept its own == "true" while core moved to _parse_bool, so REDIS_SSL=1 produced TLS workers and a plaintext Django cache with no pool kwargs at all |
73b43c073 |
What broke the loop
Four rounds produced the same finding in four costumes — a setting centralised in unstract.core with the parallel read in base.py left behind. So 41463ba15 added TestTheBackendAgreesWithCoreOnEverySharedSetting: 19 realistic environments asserting that where the Django cache ends up (host, port, db, username, password, TLS) is where create_redis_client in the same process ends up. It pins the property instead of the bug.
It found a real divergence on its first run, and not one of the four above: the discrete path — which the module docstring calls the primary one — could not carry an ACL username at all.
9019ac13b then extended the same parity to Sentinel mode, closing four pre-existing divergences: the Sentinel port default (6379 vs core's 26379), the "default" username default, the ignored REDIS_USERNAME spelling, and TLS reaching neither the discovery nor the master connection.
Testing
Core 120 → 139 (276 in the dir), backend 42 → 87. Roughly 20 mutations verified caught. Two test harnesses were themselves liabilities and were fixed: _derive spliced source line-ranges with a ~415-line gap, so inserting REDIS_PASSWORD = "" at base.py:251 left all tests green while shipping a broken cache — it now asserts its own coverage.
Behaviour changes worth a reviewer's attention
- Blank now means unset for host, port, password, username and the four TLS vars.
- Sentinel mode: an unset
REDIS_PORTresolves 26379; no username is sent unless configured;REDIS_SSLnow applies. Flag-off is byte-identical, asserted. CACHES["default"]["LOCATION"]can now carry the password, which Django'sSafeExceptionReporterFilterdoes not redact — a deliberate trade, since django-redis leaves no other route for an ACL username.DEBUGmust stay off.
Known and deliberately out of scope
unstract-cloud pins CACHE_REDIS_USERNAME: "" with a comment stating the shadowing is intentional. This PR removes that mechanism, so the comment is stale — corrected separately in Zipstack/unstract-cloud#1790, which should merge after this one.
🤖 Generated with Claude Code
muhammad-ali-e
marked this pull request as ready for review
September 28, 2026 04:11
Valid review finding, and a parity bug in the commit that was meant to establish parity: 9019ac1 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 <noreply@anthropic.com>
johnyrahul
approved these changes
Sep 30, 2026
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
Author
|
@greptileai please review now The 1/5 appears to be scoped to the back-merge rather than to this PR. Recording the evidence so the re-review has it. This PR's diff is 7 files, all Redis: No image-extraction, frontend/CSP or PG-consumer file is touched. The three findings in the 1/5 rationale belong to commits that came in from The summary says so itself — "the changes since the previous review also introduce image-mode extraction and answering, frontend CI/CSP updates, and PG-consumer readiness." Those are #2210 and #2245, both already reviewed and merged on their own PRs. The strongest single indicator: "A reader can accept an incomplete page set during re-extraction" is the same finding #2210 carried as its own 4/5 rationale — "re-extraction can temporarily remove the shared page set while an active vision request is reading it" — where it was raised, discussed and resolved before merge. Also consistent with a scoping issue: the latest review posted no inline comments at all. Every inline comment on this PR is on GitHub's PR diff is Every other check is green (unit, integration, e2e, SonarCloud, pre-commit) and the PR is approved. Happy to rebase onto |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
{prefix}PASSWORDis now applied when the URL itself carries no credentials. Previously it was ignored and the client connected anonymously.sample.envrecipes show the password as a separate key rather than embedded in the URL.Why
Follow-up to #2287, which is merged. The gap was not exercised by that PR's live testing against a managed Redis: the URL-mode run embedded the password in the URL, and the discrete run had no URL at all. The defect sits in the third combination — a clean URL plus a separately supplied password — which is the one worth recommending.
It is worth recommending because a URL travels to places a password should not:
unstract.redisEndpointerror messagesComparisonErrorconditionExternalSecret.spec.target.template.dataAll 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 a URL gets printed — and this change is what makes that configuration actually work.
The failure also misdirects: the endpoint answers
NOAUTHon the first command, which reads as a broken server rather than a dropped password.How
ConnectionPool.from_urlends withkwargs.update(url_options), so an existingrediss://:<password>@hostURL behaves exactly as before — verified for both the:pw@anduser:pw@spellings.values.yamlshipsREDIS_USER: default— filling that in on its own would make redis-py sendAUTHto the in-cluster server, which has none. I wrote that regression into the first version and caught it by testing the chart's default posture, not by re-reading the diff.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
username-alone case is explicitly pinned by a test, because that is where a careless version of this change would break the default in-cluster deployment.Database Migrations
Env Config
{prefix}PASSWORDand{prefix}USERalready existed; they are now honoured in URL mode.Relevant Docs
backend/sample.env,runner/sample.env,workers/sample.env— recipes updated.Related Issues or PRs
Dependencies Versions
Notes on Testing
unstract/core243 passed, backend 35 passed,ruffclean.TestUrlModeCredentials, mutation-verified: removing the fallback fails two of them, ungating the username fails the third.:pw@in the URL wins;user:pw@in the URL wins; username rides with the password; username alone is not a credential (the regression guard); no credentials anywhere stays anonymous.Screenshots
n/a
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code