Merge pull request #337 from skytable/fixes/empty-pass

Auth: Fix issues with incorrect auth data and corresponding CLI errors

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
 

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()?;

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!();
                 }
             }

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());

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]

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))
+    );
+}

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()