[BUG] missing response when AUTH is errored inside a transaction

[ranshid]

When we introduced module based authentication, we provided a way for the module to own the authentication and fail it in case needed. for this we do not write error reply in case the auth identify there are existing replies in the client reply list:

/* If `err` is provided, this is added as an error reply to the client.
 * Otherwise, the standard Auth error is added as a reply. */
void addAuthErrReply(client *c, robj *err) {
    if (clientHasPendingReplies(c)) return; <---------- here
    if (!err) {
        addReplyError(c, "-WRONGPASS invalid username-password pair or user is disabled.");
        return;
    }
    addReplyError(c, err->ptr);
}

However this introduces an issue as the auth might be used inside a transaction or pipeline which already has some pending replies in the client reply list.

example using valkey-cli:

multi auth no-user 123 exec

Credits to @uriyage for locating this

[enjoy-binbin]

so module auth can add reply in its callback, or not add reply and just return an err robj in its callback? And we have to figure a way to determine whether it has added a reply?

[ranshid]

so module auth can add reply in its callback, or not add reply and just return an err robj in its callback? And we have to figure a way to determine whether it has added a reply?

IIUC yes. maybe we can count on the deferred_reply_errors, but I am not sure it will cover all cases.

[enjoy-binbin]

deferred_reply_errors wont work since it is only used for module command i guess, in this auth command it does not trigger it.

Another ugly way is, check and see if c->bufpos and listLength(c->reply) changed before and after, if the value is changed that means the auth callback already add the reply...

diff --git a/src/acl.c b/src/acl.c
index c33b5d1ed..23e0737ae 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -1439,7 +1439,6 @@ int ACLCheckUserCredentials(robj *username, robj *password) {
 /* If `err` is provided, this is added as an error reply to the client.
  * Otherwise, the standard Auth error is added as a reply. */
 void addAuthErrReply(client *c, robj *err) {
-    if (clientHasPendingReplies(c)) return;
     if (!err) {
         addReplyError(c, "-WRONGPASS invalid username-password pair or user is disabled.");
         return;
@@ -3158,11 +3157,21 @@ void authCommand(client *c) {
     }
 
     robj *err = NULL;
+
+    size_t old_bufpos = c->bufpos;
+    unsigned long old_reply_length = listLength(c->reply);
     int result = ACLAuthenticateUser(c, username, password, &err);
+    size_t new_bufpos = c->bufpos;
+    unsigned long new_reply_length = listLength(c->reply);
+
     if (result == AUTH_OK) {
         addReply(c, shared.ok);
     } else if (result == AUTH_ERR) {
-        addAuthErrReply(c, err);
+        if (old_bufpos == new_bufpos && old_reply_length == new_reply_length) {
+            /* COB does not changed at all, this mean module auth callback does
+             * not added any reply. */
+            addAuthErrReply(c, err);
+        }
     }
     if (err) decrRefCount(err);
 }