Repository: skytable/skytable (language: rust)

Issue:
CLI: cli error: client error. connection setup error: handshake error code 5 always occurs
**Description of bug**
Every time I run ./skysh (not added to path), after entering the password, it returns 
`cli error: client error. connection setup error: handshake error code 5`
 The server log says 
`[2024-03-21T22:25:58Z ERROR skyd::engine::net] failed to handshake with client`

If I put in no password, however, the server log says 
```
thread 'server' panicked at server/src/engine/core/system_db.rs:84:58:
called `Result::unwrap()` on an `Err` value: BadPassword
```
And the client error is 
`cli error: client error. io error: failed to fill whole buffer`

**Steps to reproduce**
Steps to reproduce the behavior:
<!--
For example:
1. Run `skyd`
2. Run `./skysh` and enter ANY password that is valid, including the correct one
-->

**Expected behavior**
I expected that skysh would successfully connect.

**Meta**
 - **Release tag:**
sky-bundle-v0.8.0-x86_64-macos.zip
 - **Branch:**
 Release?
- **Commit ID:**
?
- **Operating system:**
MacOS 14.1 Beta (23B5067a) (Sonoma)
**Additional context**
I am very confused - the benchmark, sky-bench works.

Feature: Print response codes/strings to stderr
Firstly thank you kindly for this project and consideration of this feature request!


What I would like to propose is printing all response codes and strings to `stderr` for `skysh` utility (and/or other utils)
That would in my opinion simplify parsing and working with CLI utility in scripts or in command line.

**Rationale is this example:**
This would parse also response `(Okay)` but if this fail I will never know until I see full `stdout`.
**Example:** `skysh -e "lskeys" |grep Ok` 

If we want to handle more use cases, we talked about `--output=stderr` that could change behaviour of printing based on this option.
Also simple `-q --quite` flag could mitigate all reponses if we only want data output to `stdout`.


Discussion / hints:
Can you see if it works with `--password <your password>`?
We actually had a related issue (#323) but since this is on macOS it may not exactly be related. With regard to the panic on the server side, that's a bug and I'll get it fixed soon
It seems to work with `--password`.
I'll test this locally and try to find a fix. Sorry for the trouble!
Possible pathways and expected behavior:
1. You enter an incorrect password, you get HS error 5
2. You enter a correct password, you get authenticated
3. You enter an empty (essentially an incorrect one) password, you get HS error 5

The last case did not have the expected outcome due to a server-side bug (a fairly trivial one) which has been fixed on the active branch and will be released into the next version (0.8.1).

Can you confirm if cases 1 and 2 behave as expected?
Let's look into the specifics. So, we're going to print resp strings/codes to `stderr` instead of printing it to `stdout`. This is a breaking change, but is fine since we're moving closer to 0.8 which will introduce multiple breaking changes. Customization of behavior should also be allowed to enable users to adjust output to their specific workflow.
+1. Nominated
I would like to work on this. So far, I have identified the location where the error is being logged. By changing it to `eprintln`, we can print that into stderr. https://github.com/skytable/skytable/blob/b83e42af260d61cdd26bac9d59f78e48a3710cbb/cli/src/resp.rs#L47

I want to discuss the configuration options that you would like to provide to the users. As mentioned in the first comment, we can have a `--output` flag, which can take values `stderr` and `stdout`, or we can just add the `-q --quite` flag. If we don't want this to be a breaking change, then we can make `stderr` opt-in instead. I would like to hear your thoughts.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f02ab680..e236e454 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,8 +10,14 @@ All changes in this project will be noted in this file.
 
 ### Fixes
 
-- Fixed migration from v1 SE (released with v0.8.0-beta) to v2 SE (released in v0.8.0)
-- Fixed health reporting
+- **Server**:
+  - Fixed migration from v1 SE (released with v0.8.0-beta) to v2 SE (released in v0.8.0)
+  - Fixed health reporting
+  - Fixed a connection crash (not a server-wide crash) at the pre-connection stage when authentication data
+    was sent incorrectly
+- **CLI**:
+  - Fixed `--eval` output. All server errors are now correctly written to `stderr`
+  - Guard against empty passwords
 
 ## Version 0.8.0
 
diff --git a/cli/src/args.rs b/cli/src/args.rs
index dbd6eedb..23a23eb3 100644
--- a/cli/src/args.rs
+++ b/cli/src/args.rs
@@ -148,12 +148,12 @@ pub fn parse() -> CliResult<Task> {
         }
     };
     let password = match args.remove("--password") {
-        Some(p) => p,
+        Some(p) => check_password(p, "cli arguments")?,
         None => {
             // let us check the environment variable to see if anything was set
             match env::var(env_vars::SKYDB_PASSWORD) {
-                Ok(v) => v,
-                Err(_) => read_password("Enter password: ")?,
+                Ok(v) => check_password(v, "env")?,
+                Err(_) => check_password(read_password("Enter password: ")?, "env")?,
             }
         }
     };
@@ -169,6 +169,16 @@ pub fn parse() -> CliResult<Task> {
     }
 }
 
+fn check_password(p: String, source: &str) -> CliResult<String> {
+    if p.is_empty() {
+        return Err(CliError::ArgsErr(format!(
+            "password value cannot be empty (currently set via {source})"
+        )));
+    } else {
+        Ok(p)
+    }
+}
+
 fn read_password(prompt: &str) -> Result<String, std::io::Error> {
     print!("{prompt}");
     io::stdout().flush()?;
diff --git a/cli/src/resp.rs b/cli/src/resp.rs
index 4027fc1c..2c199648 100644
--- a/cli/src/resp.rs
+++ b/cli/src/resp.rs
@@ -40,33 +40,44 @@ macro_rules! pprint {
     }
 }
 
-pub fn format_response(resp: Response, print_special: bool, pretty_format: bool) -> bool {
+pub fn format_response(resp: Response, print_special: bool, in_repl: bool) -> bool {
     match resp {
-        Response::Empty => pprint!(pretty_format, "(Okay)".cyan()),
+        Response::Empty => {
+            if in_repl {
+                println!("{}", "(Okay)".cyan())
+            }
+            // for empty responses outside the repl, it's equivalent to an exit 0, so we don't output anything to stdout or stderr
+        }
         Response::Error(e) => {
-            println!("{}", format!("(server error code: {e})").red());
+            if in_repl {
+                println!("{}", format!("(server error code: {e})").red());
+            } else {
+                // outside the repl, just write the error code to stderr. note, the query was technically "successful" because the server received it
+                // and responded to it. but on the application end, it was not. so, no need for a nonzero exit code
+                eprintln!("{e}");
+            }
             return false;
         }
         Response::Value(v) => {
-            print_value(v, print_special, pretty_format);
+            print_value(v, print_special, in_repl);
             println!();
         }
         Response::Row(r) => {
-            print_row(r, pretty_format);
+            print_row(r, in_repl);
             println!();
         }
         Response::Rows(rows) => {
             if rows.is_empty() {
-                pprint!(pretty_format, "[0 rows returned]".grey().italic());
+                pprint!(in_repl, "[0 rows returned]".grey().italic());
             } else {
                 for (i, row) in rows.into_iter().enumerate().map(|(i, r)| (i + 1, r)) {
-                    if pretty_format {
+                    if in_repl {
                         let fmt = format!("({i})").grey().italic();
                         print!("{fmt}")
                     } else {
                         print!("({i})")
                     }
-                    print_row(row, pretty_format);
+                    print_row(row, in_repl);
                     println!();
                 }
             }
diff --git a/server/src/engine/config.rs b/server/src/engine/config.rs
index 33ba576d..7d518abe 100644
--- a/server/src/engine/config.rs
+++ b/server/src/engine/config.rs
@@ -359,7 +359,7 @@ pub enum ConfigSource {
 impl ConfigSource {
     fn as_str(&self) -> &'static str {
         match self {
-            ConfigSource::Cli => "CLI",
+            ConfigSource::Cli => "command-line arguments",
             ConfigSource::Env => "ENV",
             ConfigSource::File => "config file",
         }
@@ -530,8 +530,8 @@ fn arg_decode_auth<CS: ConfigurationSource>(
         return Err(ConfigError::with_src(
             CS::SOURCE,
             ConfigErrorKind::ErrorString(format!(
-                "to enable auth, you must provide values for {}",
-                CS::KEY_AUTH_DRIVER,
+                "to enable password auth, you must provide a value for '{}'",
+                CS::KEY_AUTH_ROOT_PASSWORD,
             )),
         )
         .into());


Test patch:
diff --git a/server/src/engine/ql/tests/lexer_tests.rs b/server/src/engine/ql/tests/lexer_tests.rs
index 8446cf15..17ddee5e 100644
--- a/server/src/engine/ql/tests/lexer_tests.rs
+++ b/server/src/engine/ql/tests/lexer_tests.rs
@@ -294,26 +294,26 @@ fn safe_query_float() {
 
 #[test]
 fn safe_query_binary() {
-    let (query, query_window) = make_safe_query(b"?", SFQ_BINARY);
-    let binary = lex_secure(&query, query_window).unwrap();
-    assert_eq!(
-        binary,
-        vec![Token::Lit(Lit::new_bin(
-            "cringe😃😄😁😆😅😂🤣😊😸😺".as_bytes()
-        ))]
-    );
+    for (test_payload_string, expected) in [
+        (b"\x050\n".as_slice(), b"".as_slice()),
+        (SFQ_BINARY, "cringe😃😄😁😆😅😂🤣😊😸😺".as_bytes()),
+    ] {
+        let (query, query_window) = make_safe_query(b"?", test_payload_string);
+        let binary = lex_secure(&query, query_window).unwrap();
+        assert_eq!(binary, vec![Token::Lit(Lit::new_bin(expected))]);
+    }
 }
 
 #[test]
 fn safe_query_string() {
-    let (query, query_window) = make_safe_query(b"?", SFQ_STRING);
-    let binary = lex_secure(&query, query_window).unwrap();
-    assert_eq!(
-        binary,
-        vec![Token::Lit(Lit::new_string(
-            "cringe😃😄😁😆😅😂🤣😊😸😺".to_owned().into()
-        ))]
-    );
+    for (test_payload_string, expected) in [
+        (b"\x060\n".as_slice(), ""),
+        (SFQ_STRING, "cringe😃😄😁😆😅😂🤣😊😸😺"),
+    ] {
+        let (query, query_window) = make_safe_query(b"?", test_payload_string);
+        let binary = lex_secure(&query, query_window).unwrap();
+        assert_eq!(binary, vec![Token::Lit(Lit::new_str(expected))]);
+    }
 }
 
 #[test]
diff --git a/server/src/engine/tests/client_misc/sec/mod.rs b/server/src/engine/tests/client_misc/sec/mod.rs
index be1fa0bb..ff3df078 100644
--- a/server/src/engine/tests/client_misc/sec/mod.rs
+++ b/server/src/engine/tests/client_misc/sec/mod.rs
@@ -31,7 +31,10 @@ mod dml_sec;
 use {
     crate::engine::error::QueryError,
     sky_macros::dbtest,
-    skytable::{error::Error, query},
+    skytable::{
+        error::{ConnectionSetupError, Error},
+        query,
+    },
 };
 
 const INVALID_SYNTAX_ERR: u16 = QueryError::QLInvalidSyntax.value_u8() as u16;
@@ -52,3 +55,30 @@ fn deny_unknown_tokens() {
         );
     }
 }
+
+#[dbtest(username = "root", password = "")]
+fn ensure_empty_password_returns_hs_error_5() {
+    let db = db_connect!();
+    assert_err_eq!(
+        db,
+        Error::ConnectionSetupErr(ConnectionSetupError::HandshakeError(5))
+    );
+}
+
+#[dbtest(username = "", password = "1234567890")]
+fn ensure_empty_username_returns_hs_error_5() {
+    let db = db_connect!();
+    assert_err_eq!(
+        db,
+        Error::ConnectionSetupErr(ConnectionSetupError::HandshakeError(5))
+    );
+}
+
+#[dbtest(username = "", password = "")]
+fn ensure_empty_username_and_password_returns_hs_error_5() {
+    let db = db_connect!();
+    assert_err_eq!(
+        db,
+        Error::ConnectionSetupErr(ConnectionSetupError::HandshakeError(5))
+    );
+}
diff --git a/sky-macros/src/dbtest.rs b/sky-macros/src/dbtest.rs
index 68cfef90..77eab96e 100644
--- a/sky-macros/src/dbtest.rs
+++ b/sky-macros/src/dbtest.rs
@@ -241,6 +241,14 @@ pub fn dbtest(attrs: TokenStream, item: TokenStream) -> TokenStream {
             block = quote! {
                 #block
                 /// Get a Skyhash connection the database (defined by [`sky_macros::dbtest`])
+                #[allow(unused_macros)]
+                macro_rules! db_connect {
+                    () => {{
+                        skytable::Config::new(__DBTEST_HOST, __DBTEST_PORT, __DBTEST_USER, __DBTEST_PASS).connect()
+                    }}
+                }
+                /// Get a Skyhash connection the database (defined by [`sky_macros::dbtest`])
+                #[allow(unused_macros)]
                 macro_rules! db {
                     () => {{
                         skytable::Config::new(__DBTEST_HOST, __DBTEST_PORT, __DBTEST_USER, __DBTEST_PASS).connect().unwrap()