Fix/redis ssl

[Prashant-r3x]

No description provided.

[shreyas-lyzr]

Overall: clean, focused addition. cache_broker_url fills a real gap for Celery and other URL-based consumers, the docstring is thorough, and the error handling is clear. Three items worth addressing — one is a security default that should be flipped, the others are a correctness nuance and a missing input guard.

Inline comments below.

[shreyas-lyzr]

Defaulting ssl_cert_reqs to CERT_NONE silently disables certificate verification for all cloud providers. A misconfigured caller gets an encrypted-but-unverified connection with no warning — a classic man-in-the-middle window.

Consider defaulting to CERT_REQUIRED, which is the safe baseline for ElastiCache and Azure Redis in production. Callers who genuinely need CERT_NONE (dev clusters with self-signed certs) should opt in explicitly.

Suggested change

	ssl_cert_reqs: str = "CERT_NONE",
	ssl_cert_reqs: str = "CERT_REQUIRED",

[shreyas-lyzr]

For the plain redis provider, a non-empty password produces redis://:password@host:6379/0 — colon prefix, no username. This is spec-valid and most clients handle it, but it differs from the redis://default:password@... form expected by Redis 6+ ACL-based auth. Worth a brief comment clarifying the intent, or switching to the explicit form to avoid silent auth failures against ACL-enabled clusters.

[shreyas-lyzr]

ssl_cert_reqs is a free-form string with no validation. An invalid value (e.g. CERT_INVALID) gets silently embedded in the URL and only surfaces as a confusing error from the Redis client at connection time, far from the call site. Suggest validating eagerly:

Suggested change

	f"?ssl_cert_reqs={ssl_cert_reqs}"
	_VALID_CERT_REQS = {"CERT_NONE", "CERT_OPTIONAL", "CERT_REQUIRED"}
	if ssl_cert_reqs not in _VALID_CERT_REQS:
	raise ValueError(
	f"ssl_cert_reqs must be one of {sorted(_VALID_CERT_REQS)}, got {ssl_cert_reqs!r}"
	)
	return (
	f"rediss://:{password}@{host}:{port}/{db}"
	f"?ssl_cert_reqs={ssl_cert_reqs}"
	)

[shreyas-lyzr]

Overall this is a clean, well-scoped addition — the new cache_broker_url helper fills a real gap (Celery and other URL-driven consumers cannot use the CacheBackend object directly). A few correctness and usability issues worth addressing.

---

1. Password is URL-encoded without escaping (correctness bug)

The auth segment is built with a plain f-string:

auth = f"default:{password}@" if password else ""

If password contains characters that are special in a URI — @, :, /, ?, #, % — the resulting URL will be malformed and authentication will silently fail or raise a confusing error at connection time.

Suggestion: use urllib.parse.quote(password, safe='') (stdlib, no new dependency) before interpolating the password.

---

2. ssl_cert_reqs validation only runs on cloud paths — plain "redis" silently ignores a bad value

The ssl_cert_reqs check is only inside the elasticache/azure_redis branch. A caller who passes ssl_cert_reqs="CERT_NONE" with provider="redis" gets no error; the value is silently dropped. Move the validation before the provider if-chain so it applies universally.

---

3. ssl_cert_reqs query param may not be honored by Celery

Celery's Redis transport parses rediss:// URLs but does not forward arbitrary query parameters to the redis-py connection. The ssl_cert_reqs value in the URL will be silently ignored when used as a Celery broker URL. The docstring already calls out Celery as the primary consumer — worth an explicit note that callers may need to set ssl_options separately on the Celery app config when they need non-default cert verification.

---

4. Minor: no validation on db

A negative integer or non-integer for db produces a syntactically valid URL that Redis will reject at connect time with a cryptic error. A if not isinstance(db, int) or db < 0: raise ValueError(...) guard is consistent with the existing validation pattern and surfaces the mistake early.

[shreyas-lyzr]

Password characters that are special in a URI (:, @, /, ?, #, %) will silently corrupt the URL here. Use urllib.parse.quote(password, safe='') to percent-encode before interpolating:

Suggested change

	return f"redis://{auth}{host}:{port}/{db}"
	from urllib.parse import quote
	auth = f"default:{quote(password, safe='')}@" if password else ""

[shreyas-lyzr]

This ssl_cert_reqs validation is only reached for elasticache/azure_redis. A caller using provider="redis" with a bad ssl_cert_reqs value gets no error and the param is silently dropped. Move the validation above the if provider == "redis" check so it applies to all code paths.

[shreyas-lyzr]

Worth noting in the docstring that ssl_cert_reqs as a URL query param is not forwarded by Celery's Redis transport — callers still need to configure ssl_options on the Celery app separately when using this URL as a broker.

[shreyas-lyzr]

Three things worth addressing before merge, one of which is a correctness issue. Inline comments below.

[shreyas-lyzr]

The ssl_cert_reqs query parameter is silently ignored by Celery's Redis transport, but the function still encodes it into the URL for elasticache and azure_redis providers. This creates a false sense of security: a caller that passes ssl_cert_reqs='CERT_NONE' thinking they are relaxing certificate verification for Celery will have no effect at all. The docstring mentions this, but only in a 'Notes' block that is easy to miss. Consider raising a warning (or at minimum making it more prominent inline near the parameter docs) so callers aren't surprised when cert verification is not what they set.

Also worth noting: redis-py itself does respect ssl_cert_reqs as a query param when parsing a rediss:// URL, so for direct redis-py usage the parameter is effective — it is only Celery that ignores it. Clarifying which consumer is affected would help.

[shreyas-lyzr]

The provider == 'redis' branch returns a plain redis:// URL regardless of what ssl_cert_reqs was passed. But self-hosted Redis can also run with TLS (rediss://). There is no way for a caller to get a TLS URL for self-hosted Redis from this function — they would have to fall back to StandaloneRedisBackend.from_url directly. If intentional, worth documenting explicitly. If not, consider accepting an ssl: bool = False param for the 'redis' provider, mirroring what StandaloneRedisBackend.from_credentials does.

[shreyas-lyzr]

The scan abstract method declares the return type as tuple[int, list[bytes]], but the concrete _RedisMixin.scan implementation returns whatever aioredis.Redis.scan() gives back. In recent versions of redis-py, scan() returns tuple[int, list[bytes]] for the default decode mode, but tuple[int, list[str]] when decode_responses=True is set on the client. If any backend is created with decode_responses=True (none appear to be currently, but it is an easy mistake), the return type contract would be silently violated and downstream callers expecting bytes would get str.

Consider adding a note to the abstract method doc, or asserting in _RedisMixin.scan that the client is not in decode_responses mode, or widening the return type annotation to tuple[int, list[bytes | str]].

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[SahilShetLYZR]

LGTM