From 2b24bcf6537f4ee2d687eb8c9ac540deaa2b2e88 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Fri, 24 Apr 2026 03:25:54 +0900 Subject: [PATCH 1/2] fix(redis): validate CLIENT subcommand arity + distinguish string/bulk in test stub Addresses coderabbit review on f3a1903d. MAJOR: CLIENT subcommand arity (adapter/redis_compat_commands.go) `argsLen` only guarantees CLIENT has one subcommand, so malformed requests silently succeeded: CLIENT SETNAME (missing name) -> OK, no mutation CLIENT GETNAME extra (extra operand) -> name as if no operand CLIENT ID junk -> ID returned CLIENT INFO extra -> INFO returned Each subcommand now validates its exact arg count via a shared checkClientArity helper, returning `ERR wrong number of arguments for 'client|' command` on mismatch. Each case was also extracted into its own method so the switch body stays under the cyclomatic budget. MINOR: distinguish string from bulk in helloRecordingConn test stub (adapter/redis_hello_test.go) WriteString was recorded as kind="bulk", which meant a future regression swapping WriteBulkString for WriteString in the HELLO reply path would not fail any test. Now recorded as kind="string"; no existing assertions broke since all HELLO key/value writes use WriteBulk / WriteBulkString. Regression tests added: CLIENT SETNAME / GETNAME / ID / INFO arity checks (+4 tests). go test ./adapter -race -run TestClient_|TestHello_ passes; golangci-lint clean. --- adapter/redis_compat_commands.go | 75 +++++++++++++++++++++++++------- adapter/redis_hello_test.go | 72 +++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 18 deletions(-) diff --git a/adapter/redis_compat_commands.go b/adapter/redis_compat_commands.go index e44360be..fdf2f05e 100644 --- a/adapter/redis_compat_commands.go +++ b/adapter/redis_compat_commands.go @@ -206,6 +206,61 @@ func (r *RedisServer) setnx(conn redcon.Conn, cmd redcon.Command) { conn.WriteInt(1) } +// clientSubcommandArgCount is the total cmd.Args length (including +// CLIENT + subcommand) required by no-operand CLIENT subcommands +// like GETNAME / ID / INFO. +const clientSubcommandArgCount = 2 + +// checkClientArity verifies cmd.Args has exactly want elements and +// writes the standard Redis wrong-arity error otherwise. Returns +// true when the caller should stop handling (bad arity). +func checkClientArity(conn redcon.Conn, cmd redcon.Command, sub string, want int) bool { + if len(cmd.Args) == want { + return false + } + conn.WriteError("ERR wrong number of arguments for 'client|" + strings.ToLower(sub) + "' command") + return true +} + +// clientSetName handles CLIENT SETNAME. SETNAME is shared with +// HELLO's SETNAME clause; both write into the same connState.clientName +// slot so a client that uses HELLO SETNAME once and then queries +// CLIENT GETNAME gets the right answer without having to re-issue +// CLIENT SETNAME. +func clientSetName(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "SETNAME", clientSetNameMinArgs) { + return + } + state.clientName = string(cmd.Args[2]) + conn.WriteString("OK") +} + +func clientGetName(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "GETNAME", clientSubcommandArgCount) { + return + } + if state.clientName == "" { + conn.WriteNull() + return + } + conn.WriteBulkString(state.clientName) +} + +func (r *RedisServer) clientID(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "ID", clientSubcommandArgCount) { + return + } + conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice +} + +func (r *RedisServer) clientInfo(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "INFO", clientSubcommandArgCount) { + return + } + id := r.ensureConnID(state) + conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName)) +} + func (r *RedisServer) client(conn redcon.Conn, cmd redcon.Command) { sub := strings.ToUpper(string(cmd.Args[1])) state := getConnState(conn) @@ -213,25 +268,13 @@ func (r *RedisServer) client(conn redcon.Conn, cmd redcon.Command) { case "SETINFO": conn.WriteString("OK") case "SETNAME": - // SETNAME is shared with HELLO's SETNAME clause; both write into - // the same connState.clientName slot so a client that uses - // HELLO SETNAME once and then queries CLIENT GETNAME gets the - // right answer without having to re-issue CLIENT SETNAME. - if len(cmd.Args) >= clientSetNameMinArgs { - state.clientName = string(cmd.Args[2]) - } - conn.WriteString("OK") + clientSetName(conn, cmd, state) case "GETNAME": - if state.clientName == "" { - conn.WriteNull() - return - } - conn.WriteBulkString(state.clientName) + clientGetName(conn, cmd, state) case "ID": - conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice + r.clientID(conn, cmd, state) case "INFO": - id := r.ensureConnID(state) - conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName)) + r.clientInfo(conn, cmd, state) default: conn.WriteError("ERR unsupported CLIENT subcommand '" + sub + "'") } diff --git a/adapter/redis_hello_test.go b/adapter/redis_hello_test.go index 4d3341a9..9e3dc190 100644 --- a/adapter/redis_hello_test.go +++ b/adapter/redis_hello_test.go @@ -22,7 +22,7 @@ import ( // array-header) is the only way to distinguish `proto` (integer 2) // from `version` (bulk "7.0.0") or `modules` (empty array header). type helloReplyElement struct { - kind string // "bulk" | "int" | "arrayHeader" + kind string // "bulk" | "string" | "int" | "arrayHeader" | "null" str string num int64 } @@ -46,7 +46,10 @@ func (c *helloRecordingConn) Close() error { return nil } func (c *helloRecordingConn) WriteError(msg string) { c.err = msg } func (c *helloRecordingConn) WriteString(s string) { c.str = s - c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: s}) + // Recorded as a distinct "string" kind so a future regression that + // swaps WriteBulkString for WriteString (or vice versa) fails the + // wire-shape assertions instead of passing silently. + c.writes = append(c.writes, helloReplyElement{kind: "string", str: s}) } func (c *helloRecordingConn) WriteBulk(bulk []byte) { c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: string(bulk)}) @@ -419,3 +422,68 @@ elastickv_redis_requests_total{command="HELLO",node_address="10.0.0.1:50051",nod ) require.NoError(t, err) } + +func TestClient_SetName_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + state := getConnState(conn) + state.clientName = "prev" + + // CLIENT SETNAME with no value (2 args total). + r.client(conn, redcon.Command{Args: [][]byte{ + []byte("CLIENT"), []byte("SETNAME"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") + require.Equal(t, "prev", state.clientName, "malformed SETNAME must not clobber clientName") + + // CLIENT SETNAME foo bar (4 args total) — also rejected. + conn2 := &helloRecordingConn{} + state2 := getConnState(conn2) + state2.clientName = "prev" + r.client(conn2, redcon.Command{Args: [][]byte{ + []byte("CLIENT"), []byte("SETNAME"), []byte("foo"), []byte("bar"), + }}) + require.Contains(t, conn2.err, "wrong number of arguments") + require.Equal(t, "prev", state2.clientName) +} + +func TestClient_GetName_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT GETNAME extra (3 args total) — GETNAME takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte("CLIENT"), []byte("GETNAME"), []byte("extra"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} + +func TestClient_ID_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT ID junk (3 args total) — ID takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte("CLIENT"), []byte("ID"), []byte("junk"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} + +func TestClient_Info_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT INFO extra (3 args total) — INFO takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte("CLIENT"), []byte("INFO"), []byte("extra"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} From 7d283f623c76b11461f75b5b11b175bd01e1ae4f Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Sat, 25 Apr 2026 06:14:29 +0900 Subject: [PATCH 2/2] fix(redis): enforce exact CLIENT SETINFO arity + clarify constant name Copilot review on PR #608: - CLIENT SETINFO still returned OK for any arity including missing operands. That was the last CLIENT subcommand keeping the silent- success behaviour this PR was supposed to eliminate, and it diverged from the real Redis wrong-arity reply for client|setinfo. Now routed through checkClientArity with clientSetInfoArgCount=4 (CLIENT + SETINFO + attr + value), mirroring SETNAME. - Rename clientSetNameMinArgs to clientSetNameArgCount. The helper compares len(cmd.Args) == want, so the "Min" suffix was misleading and invited a future refactor that swaps the helper for a >= check and silently re-introduces the wrong-arity bug. - Replace hardcoded []byte("CLIENT") in the new arity tests with []byte(cmdClient) to stay consistent with the rest of the file and keep the command name in one place. New TestClient_SetInfo_RejectsWrongArity pins all four arity shapes (2, 3, 4, 5 args) so the SETINFO regression cannot be reintroduced. --- adapter/redis_compat_commands.go | 33 +++++++++++++++++--- adapter/redis_hello_test.go | 52 +++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/adapter/redis_compat_commands.go b/adapter/redis_compat_commands.go index fdf2f05e..245f7cdc 100644 --- a/adapter/redis_compat_commands.go +++ b/adapter/redis_compat_commands.go @@ -228,7 +228,7 @@ func checkClientArity(conn redcon.Conn, cmd redcon.Command, sub string, want int // CLIENT GETNAME gets the right answer without having to re-issue // CLIENT SETNAME. func clientSetName(conn redcon.Conn, cmd redcon.Command, state *connState) { - if checkClientArity(conn, cmd, "SETNAME", clientSetNameMinArgs) { + if checkClientArity(conn, cmd, "SETNAME", clientSetNameArgCount) { return } state.clientName = string(cmd.Args[2]) @@ -261,12 +261,24 @@ func (r *RedisServer) clientInfo(conn redcon.Conn, cmd redcon.Command, state *co conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName)) } +// clientSetInfo handles CLIENT SETINFO . elastickv does +// not persist the advertised attributes (lib-name / lib-ver, etc.), but +// it MUST still enforce exact arity — otherwise `CLIENT SETINFO` with +// no operands returns OK and masks a client bug that real Redis would +// have surfaced as a wrong-arity error. +func clientSetInfo(conn redcon.Conn, cmd redcon.Command) { + if checkClientArity(conn, cmd, "SETINFO", clientSetInfoArgCount) { + return + } + conn.WriteString("OK") +} + func (r *RedisServer) client(conn redcon.Conn, cmd redcon.Command) { sub := strings.ToUpper(string(cmd.Args[1])) state := getConnState(conn) switch sub { case "SETINFO": - conn.WriteString("OK") + clientSetInfo(conn, cmd) case "SETNAME": clientSetName(conn, cmd, state) case "GETNAME": @@ -298,9 +310,20 @@ const ( // helloReplyArrayLen is the number of elements in the flat // alternating key/value reply: 7 pairs = 14 elements. helloReplyArrayLen = 14 - // clientSetNameMinArgs is the arg count at which CLIENT SETNAME - // has its operand (CLIENT + SETNAME + name = 3). - clientSetNameMinArgs = 3 + // clientSetNameArgCount is the EXACT cmd.Args length for CLIENT + // SETNAME (CLIENT + SETNAME + name = 3). Kept as an exact-arity + // constant — not a minimum — because checkClientArity compares + // `len(cmd.Args) == want`; renaming it to *MinArgs would invite a + // future refactor that "just" swaps the helper for a >= check and + // silently re-introduces the wrong-arity silent-OK bug. + clientSetNameArgCount = 3 + // clientSetInfoArgCount is the EXACT cmd.Args length for CLIENT + // SETINFO (CLIENT + SETINFO + attr + value = 4). Real Redis + // rejects any other arity for `client|setinfo`; without this + // check we would keep returning OK for `CLIENT SETINFO` with no + // operands, matching exactly the silent-success behaviour this + // PR is supposed to eliminate for every CLIENT subcommand. + clientSetInfoArgCount = 4 ) // helloParseError is the internal signal used by parseHelloArgs to diff --git a/adapter/redis_hello_test.go b/adapter/redis_hello_test.go index 9e3dc190..5c6fd15d 100644 --- a/adapter/redis_hello_test.go +++ b/adapter/redis_hello_test.go @@ -433,7 +433,7 @@ func TestClient_SetName_RejectsWrongArity(t *testing.T) { // CLIENT SETNAME with no value (2 args total). r.client(conn, redcon.Command{Args: [][]byte{ - []byte("CLIENT"), []byte("SETNAME"), + []byte(cmdClient), []byte("SETNAME"), }}) require.Contains(t, conn.err, "wrong number of arguments") require.Equal(t, "prev", state.clientName, "malformed SETNAME must not clobber clientName") @@ -443,7 +443,7 @@ func TestClient_SetName_RejectsWrongArity(t *testing.T) { state2 := getConnState(conn2) state2.clientName = "prev" r.client(conn2, redcon.Command{Args: [][]byte{ - []byte("CLIENT"), []byte("SETNAME"), []byte("foo"), []byte("bar"), + []byte(cmdClient), []byte("SETNAME"), []byte("foo"), []byte("bar"), }}) require.Contains(t, conn2.err, "wrong number of arguments") require.Equal(t, "prev", state2.clientName) @@ -457,7 +457,7 @@ func TestClient_GetName_RejectsWrongArity(t *testing.T) { // CLIENT GETNAME extra (3 args total) — GETNAME takes no operand. r.client(conn, redcon.Command{Args: [][]byte{ - []byte("CLIENT"), []byte("GETNAME"), []byte("extra"), + []byte(cmdClient), []byte("GETNAME"), []byte("extra"), }}) require.Contains(t, conn.err, "wrong number of arguments") } @@ -470,7 +470,7 @@ func TestClient_ID_RejectsWrongArity(t *testing.T) { // CLIENT ID junk (3 args total) — ID takes no operand. r.client(conn, redcon.Command{Args: [][]byte{ - []byte("CLIENT"), []byte("ID"), []byte("junk"), + []byte(cmdClient), []byte("ID"), []byte("junk"), }}) require.Contains(t, conn.err, "wrong number of arguments") } @@ -483,7 +483,49 @@ func TestClient_Info_RejectsWrongArity(t *testing.T) { // CLIENT INFO extra (3 args total) — INFO takes no operand. r.client(conn, redcon.Command{Args: [][]byte{ - []byte("CLIENT"), []byte("INFO"), []byte("extra"), + []byte(cmdClient), []byte("INFO"), []byte("extra"), }}) require.Contains(t, conn.err, "wrong number of arguments") } + +// CLIENT SETINFO must enforce exact arity too; the prior implementation +// returned OK for any arity including zero operands, which silently +// hid client bugs that real Redis would reject as wrong-arity. Pin the +// "missing operands" and "extra operand" cases so a regression cannot +// re-introduce the silent-success behaviour. +func TestClient_SetInfo_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + + // CLIENT SETINFO with no operands (2 args total). + conn := &helloRecordingConn{} + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") + require.NotEqual(t, "OK", conn.str, + "missing-operand SETINFO must not silently return OK") + + // CLIENT SETINFO attr (3 args total) — missing value. + conn2 := &helloRecordingConn{} + r.client(conn2, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), + }}) + require.Contains(t, conn2.err, "wrong number of arguments") + + // CLIENT SETINFO attr value extra (5 args total) — extra operand. + conn3 := &helloRecordingConn{} + r.client(conn3, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"), []byte("extra"), + }}) + require.Contains(t, conn3.err, "wrong number of arguments") + + // CLIENT SETINFO attr value (4 args total) — accepted shape. + conn4 := &helloRecordingConn{} + r.client(conn4, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"), + }}) + require.Empty(t, conn4.err, "well-formed SETINFO must not produce an error") + require.Equal(t, "OK", conn4.str, "well-formed SETINFO replies OK") +}