Move the CMD_PROTECTED check to after the auth check

When requirepass is enabled, we want command calls to return NOAUTH instead of ERR with the error message. Otherwise this reveals that we have disabled the configuration in the server side. Signed-off-by: Binbin <[email protected]>
valkey-io · Dec 23, 2024 · aa66479 · aa66479
1 parent d00c856
commit aa66479
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 18 deletions.
diff --git a/src/server.c b/src/server.c
@@ -4006,21 +4006,6 @@ int processCommand(client *c) {
             rejectCommandSds(c, err);
             return C_OK;
         }
-
-
-        /* Check if the command is marked as protected and the relevant configuration allows it */
-        if (c->cmd->flags & CMD_PROTECTED) {
-            if ((c->cmd->proc == debugCommand && !allowProtectedAction(server.enable_debug_cmd, c)) ||
-                (c->cmd->proc == moduleCommand && !allowProtectedAction(server.enable_module_cmd, c))) {
-                rejectCommandFormat(c,
-                                    "%s command not allowed. If the %s option is set to \"local\", "
-                                    "you can run it from a local connection, otherwise you need to set this option "
-                                    "in the configuration file, and then restart the server.",
-                                    c->cmd->proc == debugCommand ? "DEBUG" : "MODULE",
-                                    c->cmd->proc == debugCommand ? "enable-debug-command" : "enable-module-command");
-                return C_OK;
-            }
-        }
     }
 
     uint64_t cmd_flags = getCommandFlags(c);
@@ -4051,6 +4036,20 @@ int processCommand(client *c) {
         }
     }
 
+    /* Check if the command is marked as protected and the relevant configuration allows it */
+    if (c->cmd->flags & CMD_PROTECTED) {
+        if ((c->cmd->proc == debugCommand && !allowProtectedAction(server.enable_debug_cmd, c)) ||
+            (c->cmd->proc == moduleCommand && !allowProtectedAction(server.enable_module_cmd, c))) {
+            rejectCommandFormat(c,
+                                "%s command not allowed. If the %s option is set to \"local\", "
+                                "you can run it from a local connection, otherwise you need to set this option "
+                                "in the configuration file, and then restart the server.",
+                                c->cmd->proc == debugCommand ? "DEBUG" : "MODULE",
+                                c->cmd->proc == debugCommand ? "enable-debug-command" : "enable-module-command");
+            return C_OK;
+        }
+    }
+
     if (c->flag.multi && c->cmd->flags & CMD_NO_MULTI) {
         rejectCommandFormat(c, "Command not allowed inside a transaction");
         return C_OK;

diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl
@@ -991,8 +991,12 @@ start_server {tags {"introspection"}} {
     # known keywords. Might be a good idea to avoid adding tests here.
 }
 
-start_server {tags {"introspection external:skip"} overrides {enable-protected-configs {no} enable-debug-command {no}}} {
+start_server {tags {"introspection external:skip"} overrides {requirepass mypass enable-protected-configs {no} enable-debug-command {no}}} {
     test {cannot modify protected configuration - no} {
+        assert_error "NOAUTH *" {r config set dir somedir}
+        assert_error "NOAUTH *" {r DEBUG HELP}
+
+        r auth mypass
         assert_error "ERR *protected*" {r config set dir somedir}
         assert_error "ERR *DEBUG command not allowed*" {r DEBUG HELP}
     } {} {needs:debug}

diff --git a/tests/unit/moduleapi/basics.tcl b/tests/unit/moduleapi/basics.tcl
@@ -44,8 +44,11 @@ start_server {tags {"modules"}} {
     }
 }
 
-start_server {tags {"modules external:skip"} overrides {enable-module-command no}} {
+start_server {tags {"modules external:skip"} overrides {requirepass mypass enable-module-command no}} {
     test {module command disabled} {
-       assert_error "ERR *MODULE command not allowed*" {r module load $testmodule}
+        assert_error "NOAUTH *" {r module load $testmodule}
+
+        r auth mypass
+        assert_error "ERR *MODULE command not allowed*" {r module load $testmodule}
     }
 }