From e82a3783540768077bbe647b5d88101434d915f6 Mon Sep 17 00:00:00 2001
From: Ali-Akber Saifee <ali@indydevs.org>
Date: Thu, 9 Jun 2022 11:53:30 -0700
Subject: [PATCH] Reduce scope of HELLO implementation (#129)

* Reduce scope of HELLO implementation

Only accept protover=2 as a valid argument as that is
the only thing that is supported at the moment. For all
other arguments degrade to 'unknown command'

The previous implementation creates issues for clients
expecting the presence of the HELLO command to also signal
the presence of RESP3 (as technically the command appeared in
redis at the same time as support for RESP3).

It also did not raise any errors when unsupported (or invalid)
arguments were passed to the command (such as AUTH, SETNAME)

* Fix error in test

* Remove unnecessary context in assertion

* Fix construction of unknown command error string

* Fix casing of error string
---
 src/facade/facade_types.h        |  5 +++++
 src/server/dragonfly_test.cc     | 23 ++++++++++++++---------
 src/server/server_family.cc      | 16 +++++++++++++---
 src/server/stream_family_test.cc |  2 +-
 4 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/src/facade/facade_types.h b/src/facade/facade_types.h
index 9555be9e320b..4549720a8f6b 100644
--- a/src/facade/facade_types.h
+++ b/src/facade/facade_types.h
@@ -18,6 +18,11 @@ using MutableSlice = absl::Span<char>;
 using CmdArgList = absl::Span<MutableSlice>;
 using CmdArgVec = std::vector<MutableSlice>;
 
+struct CmdArgListFormatter {
+  void operator()(std::string* out, MutableSlice arg) const {
+    out->append(absl::StrCat("`", std::string_view(arg.data(), arg.size()), "`"));
+  }
+};
 
 struct ConnectionStats {
   absl::flat_hash_map<std::string, uint64_t> err_count_map;
diff --git a/src/server/dragonfly_test.cc b/src/server/dragonfly_test.cc
index 512c2728e347..298686002179 100644
--- a/src/server/dragonfly_test.cc
+++ b/src/server/dragonfly_test.cc
@@ -295,17 +295,22 @@ TEST_F(DflyEngineTest, EvalResp) {
 }
 
 TEST_F(DflyEngineTest, Hello) {
-  auto resp_no_param = Run({"hello"});
-  ASSERT_THAT(resp_no_param, ArrLen(12));
-
-  auto resp = Run({"hello", "2"});
+  auto resp = Run({"hello"});
+  ASSERT_THAT(resp, ArrLen(12));
+  resp = Run({"hello", "2"});
   ASSERT_THAT(resp, ArrLen(12));
-  EXPECT_THAT(resp.GetVec(),
-              ElementsAre("server", "redis", "version", ArgType(RespExpr::STRING), "proto",
-                          IntArg(2), "id", ArgType(RespExpr::INT64), "mode",
-                          "standalone", "role", "master"));
 
-  EXPECT_THAT(Run({"hello", "3"}), ErrArg("ERR NOPROTO unsupported protocol"));
+  EXPECT_THAT(resp.GetVec(), ElementsAre("server", "redis", "version", ArgType(RespExpr::STRING),
+                                         "proto", IntArg(2), "id", ArgType(RespExpr::INT64), "mode",
+                                         "standalone", "role", "master"));
+
+  // These are valid arguments to HELLO, however as they are not yet supported the implementation
+  // is degraded to 'unknown command'.
+  EXPECT_THAT(Run({"hello", "3"}),
+              ErrArg("ERR unknown command 'HELLO' with args beginning with: `3`"));
+  EXPECT_THAT(
+      Run({"hello", "2", "AUTH", "uname", "pwd"}),
+      ErrArg("ERR unknown command 'HELLO' with args beginning with: `2`, `AUTH`, `uname`, `pwd`"));
 }
 
 TEST_F(DflyEngineTest, EvalSha) {
diff --git a/src/server/server_family.cc b/src/server/server_family.cc
index 58ff3d53a26f..b9b967ea268f 100644
--- a/src/server/server_family.cc
+++ b/src/server/server_family.cc
@@ -92,6 +92,11 @@ string UnknownSubCmd(string_view subcmd, string cmd) {
                       cmd, " HELP.");
 }
 
+string UnknownCmd(string cmd, CmdArgList args) {
+  return absl::StrCat("unknown command '", cmd, "' with args beginning with: ",
+                      StrJoin(args.begin(), args.end(), ", ", CmdArgListFormatter()));
+}
+
 string InferLoadFile(fs::path data_dir) {
   const auto& dbname = GetFlag(FLAGS_dbfilename);
 
@@ -953,11 +958,16 @@ void ServerFamily::Info(CmdArgList args, ConnectionContext* cntx) {
 }
 
 void ServerFamily::Hello(CmdArgList args, ConnectionContext* cntx) {
+  // Allow calling this commands with no arguments or protover=2
+  // technically that is all that is supported at the moment.
+  // For all other cases degrade to 'unknown command' so that clients
+  // checking for the existence of the command to detect if RESP3 is
+  // supported or whether authentication can be performed using HELLO
+  // will gracefully fallback to RESP2 and using the AUTH command explicitly.
   if (args.size() > 1) {
     string_view proto_version = ArgS(args, 1);
-
-    if (proto_version != "2") {
-      (*cntx)->SendError("NOPROTO unsupported protocol version");
+    if (proto_version != "2" || args.size() > 2) {
+      (*cntx)->SendError(UnknownCmd("HELLO", args.subspan(1)));
       return;
     }
   }
diff --git a/src/server/stream_family_test.cc b/src/server/stream_family_test.cc
index 21faa1d9c1a3..73485c020e97 100644
--- a/src/server/stream_family_test.cc
+++ b/src/server/stream_family_test.cc
@@ -24,7 +24,7 @@ TEST_F(StreamFamilyTest, Add) {
   auto resp = Run({"xadd", "key", "*", "field", "value"});
   ASSERT_THAT(resp, ArgType(RespExpr::STRING));
   string id = string(ToSV(resp.GetBuf()));
-  EXPECT_TRUE(id.ends_with("-0")) << id;
+  EXPECT_THAT(id, EndsWith("-0"));
 
   resp = Run({"xrange", "null", "-", "+"});
   EXPECT_THAT(resp, ArrLen(0));