Fall back to RESP2 when HELLO is not supported by server/proxy#4090
Fall back to RESP2 when HELLO is not supported by server/proxy#4090yangbodong22011 wants to merge 2 commits into
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
from
10806a7 to
0fc4c80
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
2 times, most recently
from
b9cec38 to
0753043
Compare
|
Hey @yangbodong22011, thank you for the contribution! I’ll review it shortly. Meanwhile, can you please check Cursor’s comment and the test failures? There are also a few things we need to be careful about when changing the protocol configuration at this point in the flow: what happens if the parser is Having dedicated unit tests for those cases would be very helpful. |
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
from
0753043 to
915ac57
Compare
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
from
915ac57 to
4d21d3a
Compare
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
from
4d21d3a to
07e0652
Compare
When the default protocol is RESP3 (8.0+), servers or proxies that do not support HELLO will now gracefully fall back to RESP2 + standalone AUTH instead of raising an exception. Key design decisions: - Only fall back for "unknown command" or "NOPROTO" errors; other errors (WRONGPASS, NOAUTH, etc.) propagate immediately. - Only auto-fallback when protocol was NOT explicitly set by the user and maintenance notifications are not force-enabled (enabled=True), since those features depend on RESP3 push channels. - Extract _do_resp2_auth() helper to avoid duplicating the AUTH + Redis <6.0 single-arg fallback logic (issue redis#1274). - The helper re-fetches credentials from the provider (not the HELLO-mutated auth_args with injected "default"), saving one round-trip for password-only configurations. - Emit a WARNING log with the actual protocol version attempted. Closes redis#4089 🤖 Generated with [Qoder][https://qoder.com]
yangbodong22011
force-pushed
the
fix/hello-fallback-resp2
branch
from
07e0652 to
e1a72da
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit e1a72da. Configure here.
| self.maint_notifications_config | ||
| and self.maint_notifications_config.enabled is True | ||
| ): | ||
| raise error |
There was a problem hiding this comment.
Fallback doesn't guard against client-side caching configuration
Medium Severity
_fallback_to_resp2 checks for maint_notifications_config (which requires RESP3 push notifications) before allowing the downgrade, but doesn't check for client-side caching configuration. Client-side caching relies on RESP3 push notifications for key invalidation tracking. Silently falling back to RESP2 when caching is configured would cause the cache to never receive invalidation messages, leading to stale data being served indefinitely without any error or warning.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e1a72da. Configure here.
The async RESP3 branches stored the HELLO response in a local variable `response` but never assigned it to `self.handshake_metadata`. This caused downstream code (CacheProxyConnection.connect()) to hit AttributeError when accessing handshake_metadata.get(...) on None. Changes: - Async branch 1 & 3: assign read_response() to self.handshake_metadata - Add self.handshake_metadata = None in async __init__ (sync had it) - Add proto version validation to sync branch 1 (was only on branch 3) - Add proto validation to async branch 3 for full symmetry - Tests: set driver_info=None to isolate HELLO/AUTH mock flow - Add 2 regression tests for success-path metadata assignment 🤖 Generated with [Qoder][https://qoder.com]
2 participants
Summary
Fixes #4089
redis-py 8.0 defaults to RESP3, which sends a
HELLOcommand during connection handshake. If the server (Redis < 6.0) or proxy doesn't supportHELLO, the connection fails withResponseError: unknown command— there is no fallback.This PR adds a
try/except ResponseErroraround theHELLOresponse in both sync and async connection paths. On failure, it gracefully falls back to RESP2 with standaloneAUTH, matching the behavior of Lettuce, Jedis, and go-redis.Changes
redis/connection.py—on_connect_check_health(): catchResponseErrorafterHELLO, revert parser to RESP2, issue standaloneAUTHredis/asyncio/connection.py— same fix for the async pathBoth branch 1 (RESP3 + auth) and branch 3 (RESP3 + no auth) are covered.
Testing
Manually verified against a proxy that rejects
HELLOwithERR unknown command. Connection now succeeds with automatic RESP2 fallback instead of raising.Note
Medium Risk
Changes connection handshake and authentication for all default-RESP3 clients; behavior is gated on error text and explicit protocol/maint settings, but misclassified errors could still affect connect success.
Overview
When the default RESP3 connect path sends HELLO and the server or proxy rejects it (unknown command / NOPROTO), sync and async connections now downgrade to RESP2 instead of failing outright. Shared helpers
_fallback_to_resp2and_do_resp2_authcentralize parser/protocol downgrade, optional standalone AUTH with fresh credentials (avoiding HELLO’s injecteddefaultusername), and a warning unless the user setprotocol=3explicitly or maintenance notifications are enabled.Successful HELLO responses are stored in
handshake_metadataon both paths; after fallback it is set to{}so downstream code does not seeNone. New tests intests/test_hello_fallback.pycover auth branches, NOPROTO, no-fallback cases, and logging.Reviewed by Cursor Bugbot for commit 9ce4953. Bugbot is set up for automated code reviews on this repo. Configure here.