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 { } }; 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 { } } +fn check_password(p: String, source: &str) -> CliResult { + 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 { 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( 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()); 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()