fix(asyncio): reject connection URLs missing the scheme separator - #4246
fix(asyncio): reject connection URLs missing the scheme separator#4246Arunendra21 wants to merge 3 commits into
Conversation
The async parse_url used urlparse(url).scheme to validate the scheme, which accepts a URL that has a valid scheme name but no "://" separator, for example "redis:foo.bar.com:12345". In that case urlparse reports the scheme as "redis" with an empty netloc, so the function returned partial kwargs and the client silently connected to the default host instead of raising. The sync parse_url already guards against this up front and has a test for it (test_invalid_scheme_raises_error_when_double_slash_missing), but the async copy did not. This adds the same up-front check so both behave identically, and drops the now-unreachable trailing else branch. Adds the matching async test. Co-authored-by: eeshsaxena <eeshsaxena@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0debd80e0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The initial fix used a case-sensitive prefix check, which regressed async parse_url by rejecting mixed-case schemes like "REDIS://host" that urlparse() previously normalized and accepted. Normalize the URL with .lower() before the prefix check so "REDIS://", "Rediss://", and "UNIX://" are accepted again, while still requiring the "://" separator so "redis:foo" is rejected. The same change is applied to the sync parse_url so the two implementations stay consistent. Adds a test that an uppercase scheme is accepted, for both sync and async. Co-authored-by: eeshsaxena <eeshsaxena@gmail.com>
|
Good catch, thanks. Updated the check to normalize the scheme case with |
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @Arunendra21, thank you for your contribution!
Two items before merge:
- Please extend the uppercase coverage to
REDISS://(assertingSSLConnection) andUNIX://(assertingUnixDomainSocketConnection) in bothtests/test_connection_pool.pyandtests/test_asyncio/test_connection_pool.py. Case-insensitive acceptance is new for the sync parser, and the TLS variant is the one worth pinning with a test. - Please mention in the PR description that sync
from_urlnow accepts uppercase schemes — it is a released-behavior change we want captured in the release notes.
Per review, cover the TLS and unix-socket variants of the case-insensitive scheme handling in both the sync and async connection-pool URL-parsing tests, asserting SSLConnection and UnixDomainSocketConnection respectively. Co-authored-by: eeshsaxena <eeshsaxena@gmail.com>
|
Thanks @petyaslavova! Both addressed:
|
Summary
The async
parse_urlinredis/asyncio/connection.pyvalidated the URL scheme withurlparse(url).scheme, which accepts a URL that has a valid scheme name but is missing the://separator, for exampleredis:foo.bar.com:12345. In that caseurlparsereports the scheme asrediswith an empty netloc, so the function returned partial kwargs (no host/port) and the async client silently connected to the default host instead of raising.The sync
parse_urlinredis/connection.pyalready guards against this up front and has a dedicated test (test_invalid_scheme_raises_error_when_double_slash_missing), but the async copy did not, soredis.from_url("redis:foo.bar.com:12345")andredis.asyncio.from_url("redis:foo.bar.com:12345")behaved differently.Changes
redis://,rediss://, orunix://prefix raisesValueErrorwith the same message. The now-unreachable trailingelsebranch is removed, matching the structure of the sync implementation.test_invalid_scheme_raises_error_when_double_slash_missingto the async connection-pool URL-parsing tests, mirroring the existing sync test.Testing
Verified that async
parse_url/ConnectionPool.from_urlnow raises forredis:foo.bar.com:12345,localhost, andhttp://localhost, while validredis://,rediss://, andunix://URLs (including host, port, db, and querystring options) still parse exactly as before. The sync and async implementations now return matching results for these inputs.Note
Low Risk
Localized URL validation at pool creation; behavior change only for previously mis-parsed or non-canonical scheme strings.
Overview
Connection URL parsing is tightened in both
redis/connection.pyandredis/asyncio/connection.pyso invalid or ambiguous URLs fail fast with the sameValueErrormessage.parse_urlnow requires a case-insensitiveredis://,rediss://, orunix://prefix (RFC 3986 schemes). That rejects strings likeredis:foo.bar.com:12345whereurlparsewould treatredisas the scheme but leave no host—async previously could return partial kwargs and connect to defaults instead of raising. The async path is aligned with the sync guard and drops the redundant post-parseelsebranch.Tests add coverage for the missing-
://case on async (mirroring sync) and for uppercaseREDIS://,REDISS://, andUNIX://on both clients.Reviewed by Cursor Bugbot for commit 6460ba3. Bugbot is set up for automated code reviews on this repo. Configure here.
Behavior change (for release notes)
While aligning the sync and async parsers and addressing review feedback, the checks are now case-insensitive per RFC 3986. As a result the sync
redis.from_url()now also accepts uppercase schemes such asREDIS://,REDISS://, andUNIX://(theurlparse-based async parser already accepted these). This is a released-behavior change for the sync parser worth noting in the release notes; the missing-separator case (e.g.redis:foo.bar.com:12345) is still rejected.