Skip to content

fix(queue): authenticate Redis connection when user/password are configured - #247

Closed
breken-ai wants to merge 1 commit into
utopia-php:mainfrom
breken-ai:fix-queue-redis-auth-on-connect
Closed

fix(queue): authenticate Redis connection when user/password are configured#247
breken-ai wants to merge 1 commit into
utopia-php:mainfrom
breken-ai:fix-queue-redis-auth-on-connect

Conversation

@breken-ai

Copy link
Copy Markdown

Same fix as the auto-closed utopia-php/queue#87 - the queue repo is a read-only mirror, so re-filing here.

packages/queue: the Connection\Redis constructor has accepted ?user/?password since 2.0.x, but getRedis() never calls auth() - the credentials are stored and ignored. Against a password-protected Redis (requirepass or ACL), every publish/consume fails with NOAUTH.

Concrete downstream symptom: appwrite/appwrite#13554 - Appwrite's queue publisher pool passes host+port only, and even with credentials threaded through, this connection never authenticates. The app-side half is appwrite/appwrite#13588.

Fix

getRedis() calls $redis->auth() immediately after connect() when a password is configured:

  • ACL array form [$user, $password] when a username is set (phpredis >= 5.3)
  • plain password form otherwise
  • explicit null/empty-string checks (not empty()), so edge-case but valid credentials like "0" are not dropped
  • an auth failure throws RedisException inside the existing connect retry loop, so misconfiguration fails loudly with bounded retries

Prior art

utopia-php/queue#29 attempted this in January 2025 (thanks @kodejuice for the first pass) but stalled with no description and predates the constructor credentials entirely; this PR revives the approach against current main, covering the ACL username form and the retry-loop failure semantics.

Verification

Constructor signature, the getRedis() connect/retry structure, and phpredis auth() forms all confirmed in source at main (55dd5be); the monorepo file is byte-identical to the mirror's. Not run: no PHP runtime or live Redis in the author's environment - external red proof is the reporter's hourly NOAUTH log on appwrite/appwrite#13554 with every sibling pool authenticating.

Built by breken, your AI support engineer - breken.ai - this one's on us.

…igured

The constructor accepts ?user/?password but getRedis() never called
auth(), so every operation against a password-protected Redis failed
with NOAUTH. Authenticate right after connect, using the ACL array form
[user, password] when a username is set and the plain password form
otherwise (phpredis >= 5.3). Null/empty-string checks keep the guard
from treating valid edge-case credentials like "0" as unconfigured.
An auth failure throws RedisException inside the existing connect
retry loop, so misconfiguration fails loudly with bounded retries.
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The implementation appears safe to merge, with a non-blocking need for authenticated Redis integration coverage.

Fix All in Claude CodeFindings

  1. P2 Authentication Paths Lack Coverage
Fix with agent prompt
### Issue 1
packages/queue/src/Queue/Connection/Redis.php:205-209
This authentication path has no observable behavioral coverage. The existing Redis test service has authentication disabled, so the suite cannot detect regressions in either password-only authentication or the new ACL username/password form. Add integration tests against an authenticated Redis instance that verify successful publish or consume behavior with each credential form and rejection of invalid credentials. Include the `"0"` credential case highlighted by this change rather than asserting the exact `auth()` arguments.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Uses password-only authentication when no username is configured.
  • Uses Redis ACL username/password authentication when both values are configured.
  • Preserves string credentials such as "0" through explicit null and empty-string checks.
  • Closes and retries connections when authentication raises a Redis exception.
  • Behavioral integration coverage for the new authenticated connection paths is still missing.

Reviews (1) · Last reviewed commit: "fix(queue): authenticate Redis connectio..."

Comment on lines +205 to +209
if ($this->password !== null && $this->password !== '') {
// ACL form when a username is configured, plain password otherwise.
$hasUser = $this->user !== null && $this->user !== '';
$redis->auth($hasUser ? [$this->user, $this->password] : $this->password);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Authentication Paths Lack Coverage

This authentication path has no observable behavioral coverage. The existing Redis test service has authentication disabled, so the suite cannot detect regressions in either password-only authentication or the new ACL username/password form. Add integration tests against an authenticated Redis instance that verify successful publish or consume behavior with each credential form and rejection of invalid credentials. Include the "0" credential case highlighted by this change rather than asserting the exact auth() arguments.

Knowledge Base Used: Messaging, queues, and NATS

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/queue/src/Queue/Connection/Redis.php
Line: 205-209

Comment:
**Authentication Paths Lack Coverage**

This authentication path has no observable behavioral coverage. The existing Redis test service has authentication disabled, so the suite cannot detect regressions in either password-only authentication or the new ACL username/password form. Add integration tests against an authenticated Redis instance that verify successful publish or consume behavior with each credential form and rejection of invalid credentials. Include the `"0"` credential case highlighted by this change rather than asserting the exact `auth()` arguments.

**Knowledge Base Used:** [Messaging, queues, and NATS](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/monorepo/-/docs/messaging-queue-nats.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

@breken-ai

Copy link
Copy Markdown
Author

Closing as superseded - the queue-side fix landed upstream in #260 (with appwrite/appwrite#13622 on the app side). @ChiragAgg5k's version is the one to keep.

@breken-ai breken-ai closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant