diff --git a/Cargo.lock b/Cargo.lock index 9cc1dfc28e..91a2344279 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6476,6 +6476,7 @@ dependencies = [ "tempfile", "thiserror", "tokio", + "unicode-normalization", "uuid", ] diff --git a/Cargo.toml b/Cargo.toml index ef879d35f2..db2ce1cdfd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -95,6 +95,7 @@ csv = "1.4" # table turns that off itself (force_no_tty). comfy-table = "8.0" unicode-width = "0.2" +unicode-normalization = "0.1.25" # No zlib feature: it pulls libz-sys, and the pure-Rust deflate keeps # the build free of another C dependency. Constant memory lets a sheet # spill its rows to a temporary file instead of holding them. diff --git a/clippy.toml b/clippy.toml index 4598cd680a..b664db485d 100644 --- a/clippy.toml +++ b/clippy.toml @@ -14,4 +14,10 @@ disallowed-types = [ disallowed-methods = [ { path = "serde_json::Value::as_object", reason = "key order is not preserved; keep JSON as JsonText" }, { path = "serde_json::Value::as_object_mut", reason = "key order is not preserved; keep JSON as JsonText" }, + # bson decodes a document by calling itself once per level, so a + # document the server nests deep enough overflows the reading + # thread's stack. A cursor's document is taken out after its depth + # is checked. + { path = "mongodb::SessionCursor::deserialize_current", reason = "decodes a document of any depth; use nested_document::current_document" }, + { path = "mongodb::Cursor::deserialize_current", reason = "decodes a document of any depth; check it as nested_document::current_document does" }, ] diff --git a/core/crates/core/src/sql_syntax/history_recording.rs b/core/crates/core/src/sql_syntax/history_recording.rs index 80cc5aab42..7416967a16 100644 --- a/core/crates/core/src/sql_syntax/history_recording.rs +++ b/core/crates/core/src/sql_syntax/history_recording.rs @@ -105,6 +105,9 @@ fn carries_credentials( /// MariaDB `EXECUTE IMMEDIATE`, PL/pgSQL `EXECUTE`. The text can reach /// them through a variable or a concatenation, so every string of such a /// script is read for the statements it holds. +/// +/// A T-SQL or MySQL script that names an `@` variable counts too, since +/// the `EXEC` or `PREPARE` that runs the variable can come in a later run. fn runs_text_as_sql(grammar: SqlGrammar, statements: &[Vec>]) -> bool { let runners: &[&str] = match grammar { SqlGrammar::MsSql => &["EXEC", "EXECUTE", "sp_executesql"], @@ -112,10 +115,32 @@ fn runs_text_as_sql(grammar: SqlGrammar, statements: &[Vec>]) SqlGrammar::PostgreSql => &["EXECUTE"], _ => return false, }; - statements.iter().flatten().any(|token| { + let names_runner = statements.iter().flatten().any(|token| { token .name() .is_some_and(|name| runners.iter().any(|runner| runner.eq_ignore_ascii_case(name))) + }); + names_runner || statements.iter().any(|tokens| names_a_variable(grammar, tokens)) +} + +/// A T-SQL `@s` word, or MySQL's `@` before a name or a quoted name. A +/// `@@` system variable holds no text the server runs later. +fn names_a_variable(grammar: SqlGrammar, tokens: &[StatementToken<'_>]) -> bool { + tokens.iter().enumerate().any(|(at, token)| match (grammar, token) { + (SqlGrammar::MsSql, StatementToken::Word(word)) => word.starts_with('@') && !word.starts_with("@@"), + (SqlGrammar::MySql, StatementToken::Punct('@')) => { + let system = |index: Option| { + index + .and_then(|index| tokens.get(index)) + .is_some_and(|token| token.is_punct('@')) + }; + !system(at.checked_sub(1)) + && !system(Some(at + 1)) + && tokens + .get(at + 1) + .is_some_and(|next| next.name().is_some() || matches!(next, StatementToken::StringLiteral(_))) + } + _ => false, }) } @@ -574,4 +599,52 @@ mod tests { assert_eq!(classify(grammar, sql), HistoryRecording::Record, "{sql}"); } } + + #[test] + fn text_stored_in_a_variable_is_read_for_statements() { + for (grammar, sql) in [ + ( + SqlGrammar::MySql, + "SET @s = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), + ( + SqlGrammar::MySql, + "SELECT @s := 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), + ( + SqlGrammar::MySql, + "SET @'s' = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", + ), + ( + SqlGrammar::MySql, + "SELECT 'CREATE USER app IDENTIFIED BY ''Hunter2!''' INTO @s", + ), + ( + SqlGrammar::MsSql, + "DECLARE @s nvarchar(max) = N'CREATE USER app WITH PASSWORD = ''Hunter2!'''", + ), + ( + SqlGrammar::MsSql, + "SELECT @s = N'CREATE USER app WITH PASSWORD = ''Hunter2!'''", + ), + ( + SqlGrammar::MsSql, + "SET @s += N'CREATE USER app WITH PASSWORD = ''Hunter2!'''", + ), + ] { + assert_eq!(classify(grammar, sql), WITHHELD, "{sql}"); + } + for grammar in [SqlGrammar::MsSql, SqlGrammar::MySql] { + for sql in ["SET @n = 5", "SELECT @n = 1"] { + assert_eq!(classify(grammar, sql), HistoryRecording::Record, "{grammar:?}: {sql}"); + } + } + for (grammar, sql) in [ + (SqlGrammar::MySql, "SET @s = 'SELECT 1'"), + (SqlGrammar::MsSql, "DECLARE @s nvarchar(max) = N'SELECT 1'"), + (SqlGrammar::MySql, "SET @@session.sql_mode = 'CREATE USER u'"), + ] { + assert_eq!(classify(grammar, sql), HistoryRecording::Record, "{sql}"); + } + } } diff --git a/core/crates/core/src/sql_syntax/script/lexer.rs b/core/crates/core/src/sql_syntax/script/lexer.rs index dcef728bad..51f55a5a86 100644 --- a/core/crates/core/src/sql_syntax/script/lexer.rs +++ b/core/crates/core/src/sql_syntax/script/lexer.rs @@ -275,16 +275,20 @@ impl<'a> Lexer<'a> { None } - /// An `E'...'` string, with every `'...'` that continues it: the - /// server reads those by the rules of the first piece, so a backslash - /// escapes in them whatever `standard_conforming_strings` says. + /// An `E'...'` string, and on PostgreSQL every `'...'` that continues + /// it: the server reads those by the rules of the first piece, so a + /// backslash escapes in them whatever `standard_conforming_strings` says. fn escape_string(&self, start: usize) -> LexedToken { let mut from = start + 2; loop { let Some(end) = self.quote_end(from, b'\'', true) else { return self.unterminated(start, OpenConstruct::StringLiteral); }; - match self.continuing_quote(end) { + let continued = match self.syntax.escape_string_continues { + true => self.continuing_quote(end), + false => None, + }; + match continued { Some(quote) => from = quote + 1, None => return self.plain(ScriptTokenKind::StringLiteral, end), } @@ -293,8 +297,8 @@ impl<'a> Lexer<'a> { /// The quote that carries on the string that closed just before /// `from`, as PostgreSQL's `quotecontinue` rule finds it: only blanks - /// and `--` comments in between, a line break among them. Before the - /// first line break only a space, tab or form feed counts as a blank. + /// and `--` comments in between, a line break among them. PostgreSQL + /// 16 takes a vertical tab for a blank; older servers refuse the text. fn continuing_quote(&self, from: usize) -> Option { let mut index = from; let mut line_broke = false; @@ -304,8 +308,7 @@ impl<'a> Lexer<'a> { line_broke = true; index += 1; } - b' ' | b'\t' | 0x0c => index += 1, - 0x0b if line_broke => index += 1, + b' ' | b'\t' | 0x0b | 0x0c => index += 1, b'-' if self.bytes.get(index + 1) == Some(&b'-') => { index += self.bytes[index..].iter().position(|&b| b == b'\n' || b == b'\r')?; } diff --git a/core/crates/core/src/sql_syntax/script/script_rules.rs b/core/crates/core/src/sql_syntax/script/script_rules.rs index 2cd7913559..5e39ea78e1 100644 --- a/core/crates/core/src/sql_syntax/script/script_rules.rs +++ b/core/crates/core/src/sql_syntax/script/script_rules.rs @@ -67,6 +67,9 @@ pub(crate) struct ScriptSyntax { pub backtick: Option, pub bracket_identifier: BracketIdentifier, pub escape_string_prefix: bool, + /// A string after a line break carries on an `E'...'` string with its + /// backslash escapes, as PostgreSQL's `quotecontinue` rule reads it. + pub escape_string_continues: bool, pub dash_comment_needs_space: bool, /// A line comment ends at a carriage return as well as a line feed, /// as PostgreSQL's scanner and SQL Server read it. Elsewhere only a @@ -118,6 +121,7 @@ const POSTGRESQL: ScriptRules = ScriptRules { backtick: None, bracket_identifier: BracketIdentifier::None, escape_string_prefix: true, + escape_string_continues: true, dash_comment_needs_space: false, line_comment_ends_at_cr: true, hash_comment: HashComment::None, @@ -144,6 +148,7 @@ const MYSQL: ScriptRules = ScriptRules { backtick: Some(quote(QuoteKind::QuotedIdentifier, BackslashPolicy::Never)), bracket_identifier: BracketIdentifier::None, escape_string_prefix: false, + escape_string_continues: false, dash_comment_needs_space: true, line_comment_ends_at_cr: false, hash_comment: HashComment::Always, @@ -170,6 +175,7 @@ const SQLITE: ScriptRules = ScriptRules { backtick: Some(quote(QuoteKind::QuotedIdentifier, BackslashPolicy::Never)), bracket_identifier: BracketIdentifier::NoDoubling, escape_string_prefix: false, + escape_string_continues: false, dash_comment_needs_space: false, line_comment_ends_at_cr: false, hash_comment: HashComment::None, @@ -196,6 +202,7 @@ const MSSQL: ScriptRules = ScriptRules { backtick: None, bracket_identifier: BracketIdentifier::Doubling, escape_string_prefix: false, + escape_string_continues: false, dash_comment_needs_space: false, line_comment_ends_at_cr: true, hash_comment: HashComment::None, @@ -222,6 +229,7 @@ const CLICKHOUSE: ScriptRules = ScriptRules { backtick: Some(quote(QuoteKind::QuotedIdentifier, BackslashPolicy::Always)), bracket_identifier: BracketIdentifier::None, escape_string_prefix: false, + escape_string_continues: false, dash_comment_needs_space: false, line_comment_ends_at_cr: false, hash_comment: HashComment::SpaceOrBang, @@ -248,6 +256,7 @@ const CQL: ScriptRules = ScriptRules { backtick: None, bracket_identifier: BracketIdentifier::None, escape_string_prefix: false, + escape_string_continues: false, dash_comment_needs_space: false, line_comment_ends_at_cr: true, hash_comment: HashComment::None, @@ -263,9 +272,9 @@ const CQL: ScriptRules = ScriptRules { }, }; -/// DuckDB's parser is PostgreSQL's scanner without its procedural -/// bodies: a string escapes only with an `E` prefix, block comments -/// nest, `$tag$` quotes, and a backtick quotes nothing. +/// DuckDB 2.0's PEG tokenizer: a string escapes only with an `E` prefix +/// and ends at its quote, with no continuation on the next line; block +/// comments nest, `$tag$` quotes, and a backtick quotes a name as `"` does. const DUCKDB: ScriptRules = ScriptRules { boundary: Boundary::Semicolon, compound: CompoundRule::None, @@ -274,9 +283,10 @@ const DUCKDB: ScriptRules = ScriptRules { syntax: ScriptSyntax { single_quote: quote(QuoteKind::StringLiteral, BackslashPolicy::Never), double_quote: quote(QuoteKind::QuotedIdentifier, BackslashPolicy::Never), - backtick: None, + backtick: Some(quote(QuoteKind::QuotedIdentifier, BackslashPolicy::Never)), bracket_identifier: BracketIdentifier::None, escape_string_prefix: true, + escape_string_continues: false, dash_comment_needs_space: false, line_comment_ends_at_cr: true, hash_comment: HashComment::None, diff --git a/core/crates/core/src/sql_syntax/state_changing_functions.rs b/core/crates/core/src/sql_syntax/state_changing_functions.rs index 4b3c5102d6..faad16f1e6 100644 --- a/core/crates/core/src/sql_syntax/state_changing_functions.rs +++ b/core/crates/core/src/sql_syntax/state_changing_functions.rs @@ -1,9 +1,13 @@ //! Built-in functions that change state from inside a SELECT, by family. //! //! A read-only transaction stops none of them on PostgreSQL 15 and -//! earlier: they signal other sessions, take locks, run SQL text on -//! another connection or write large objects. A function the user wrote -//! is beyond what the words show. +//! earlier: they signal other sessions, take locks, run SQL text, or +//! write large objects, files or catalog rows. A function the user wrote +//! is beyond what the words show. The Apple apps list the same families, +//! and `tests/fixtures/state_changing_functions.json` holds them for both +//! test suites. + +use std::borrow::Cow; const SERVER_SIGNALS: &[&str] = &[ "pg_cancel_backend", @@ -87,6 +91,7 @@ const SETTINGS_AND_STATISTICS: &[&str] = &[ "pg_stat_reset_replication_slot", "pg_stat_reset_subscription_stats", "pg_stat_statements_reset", + "pg_stat_reset_backend_stats", "pg_restore_relation_stats", "pg_restore_attribute_stats", "pg_clear_relation_stats", @@ -102,8 +107,24 @@ const LARGE_OBJECTS: &[&str] = &[ "lowrite", "lo_truncate", "lo_truncate64", +]; + +const FILES_AND_EXTENSIONS: &[&str] = &[ "lo_import", "lo_export", + "pg_file_write", + "pg_file_rename", + "pg_file_unlink", + "writefile", + "load_extension", +]; + +/// Writes to the catalog, or straight to a table's pages. +const CATALOG_AND_STORAGE: &[&str] = &[ + "pg_import_system_collations", + "heap_force_kill", + "heap_force_freeze", + "pg_truncate_visibility_map", ]; const INDEX_MAINTENANCE: &[&str] = &[ @@ -111,9 +132,13 @@ const INDEX_MAINTENANCE: &[&str] = &[ "brin_summarize_range", "brin_desummarize_range", "gin_clean_pending_list", + // SQLite's table function runs ANALYZE. + "pragma_optimize", ]; /// Functions that run SQL text: what that text does is not in the words. +/// crosstab, connectby and xpath_table paste their text arguments into +/// the SQL they run. const SQL_TEXT_RUNNERS: &[&str] = &[ "query_to_xml", "query_to_xml_and_xmlschema", @@ -123,35 +148,49 @@ const SQL_TEXT_RUNNERS: &[&str] = &[ "dblink_exec", "dblink_send_query", "dblink_open", + "openquery", + "openrowset", + "crosstab", + "crosstab2", + "crosstab3", + "crosstab4", + "connectby", + "xpath_table", ]; -const EXTENSIONS: &[&str] = &["load_extension"]; - -const FAMILIES: [&[&str]; 11] = [ - SERVER_SIGNALS, - SESSION_CONTROL, - RECOVERY_AND_BACKUP, - REPLICATION, - SEQUENCES, - ADVISORY_LOCKS, - SETTINGS_AND_STATISTICS, - LARGE_OBJECTS, - INDEX_MAINTENANCE, - SQL_TEXT_RUNNERS, - EXTENSIONS, +const FAMILIES: [(&str, &[&str]); 12] = [ + ("server_signals", SERVER_SIGNALS), + ("session_control", SESSION_CONTROL), + ("recovery_and_backup", RECOVERY_AND_BACKUP), + ("replication", REPLICATION), + ("sequences", SEQUENCES), + ("advisory_locks", ADVISORY_LOCKS), + ("settings_and_statistics", SETTINGS_AND_STATISTICS), + ("large_objects", LARGE_OBJECTS), + ("files_and_extensions", FILES_AND_EXTENSIONS), + ("catalog_and_storage", CATALOG_AND_STORAGE), + ("index_maintenance", INDEX_MAINTENANCE), + ("sql_text_runners", SQL_TEXT_RUNNERS), ]; /// Whether a call of `name` can change state, compared without regard -/// to case. +/// to case. MariaDB looks `releaſe_lock` up as `RELEASE_LOCK`, so a +/// name is compared upper-cased. pub(crate) fn changes_state(name: &str) -> bool { + let folded = match name.is_ascii() { + true => Cow::Borrowed(name), + false => Cow::Owned(name.to_uppercase()), + }; FAMILIES .iter() - .flat_map(|family| family.iter()) - .any(|known| name.eq_ignore_ascii_case(known)) + .flat_map(|(_, names)| names.iter()) + .any(|known| folded.eq_ignore_ascii_case(known)) } #[cfg(test)] mod tests { + use std::collections::BTreeMap; + use super::*; #[test] @@ -171,6 +210,12 @@ mod tests { "pg_replication_origin_create", "gin_clean_pending_list", "load_extension", + "PG_FILE_WRITE", + "heap_force_kill", + "CROSSTAB", + "pragma_optimize", + "relea\u{17f}e_lock", + "RELEA\u{17f}E_ALL_LOCK\u{17f}", ] { assert!(changes_state(name), "{name}"); } @@ -180,6 +225,8 @@ mod tests { "lo_get", "count", "dblink_build_sql_insert", + "query_to_xmlschema", + "query", ] { assert!(!changes_state(name), "{name}"); } @@ -189,7 +236,7 @@ mod tests { fn no_name_is_listed_twice() { let mut names: Vec = FAMILIES .iter() - .flat_map(|family| family.iter()) + .flat_map(|(_, family)| family.iter()) .map(|name| name.to_ascii_lowercase()) .collect(); let listed = names.len(); @@ -197,4 +244,19 @@ mod tests { names.dedup(); assert_eq!(names.len(), listed); } + + #[test] + fn the_names_are_the_ones_both_apps_share() { + let shared: BTreeMap> = + serde_json::from_str(include_str!("../../tests/fixtures/state_changing_functions.json")).unwrap(); + let listed: BTreeMap> = FAMILIES + .iter() + .map(|(family, names)| { + let mut names: Vec = names.iter().map(|name| name.to_string()).collect(); + names.sort(); + (family.to_string(), names) + }) + .collect(); + assert_eq!(listed, shared); + } } diff --git a/core/crates/core/src/sql_syntax/statement_access.rs b/core/crates/core/src/sql_syntax/statement_access.rs index f867d4c040..9e196a0603 100644 --- a/core/crates/core/src/sql_syntax/statement_access.rs +++ b/core/crates/core/src/sql_syntax/statement_access.rs @@ -37,8 +37,19 @@ const WRITE_WORDS: [&str; 15] = [ /// SQL Server needs no `;` between statements, so a batch that opens /// with SELECT can go on to any of these. They are the T-SQL statements -/// that change data, state or the server, beyond the words above. -const TSQL_WRITE_WORDS: [&str; 26] = [ +/// that change data, state or the server, beyond the words above. FETCH +/// is left out: a read's `OFFSET … FETCH NEXT` takes it. +const TSQL_WRITE_WORDS: [&str; 36] = [ + "SET", + "OPEN", + "CLOSE", + "DEALLOCATE", + "RAISERROR", + "WAITFOR", + "REVERT", + "DECLARE", + "DUMP", + "LOAD", "DENY", "BEGIN", "COMMIT", @@ -95,6 +106,12 @@ const STATEMENT_VERBS: [&str; 10] = [ "DELETE", "UPDATE", "DROP", "TRUNCATE", "SELECT", "INSERT", "MERGE", "EXEC", "EXECUTE", "ALTER", ]; +/// SQL Server also starts a statement at CREATE with no `;` before it, +/// and a CREATE INDEX or CREATE STATISTICS can carry its own WHERE. +const TSQL_STATEMENT_VERBS: [&str; 11] = [ + "DELETE", "UPDATE", "DROP", "TRUNCATE", "SELECT", "INSERT", "MERGE", "EXEC", "EXECUTE", "ALTER", "CREATE", +]; + /// Statement verbs that MySQL also has as functions: `TRUNCATE(x, 2)` /// rounds and `INSERT(s, 1, 2, 'x')` splices, both inside a read. const ALSO_FUNCTIONS: [&str; 2] = ["TRUNCATE", "INSERT"]; @@ -135,7 +152,9 @@ const TSQL_CLAUSE_WORDS: [&str; 9] = ["THEN", "FOR", "OF", "GRANT", "REVOKE", "D /// What a T-SQL DROP statement drops and an ALTER's DROP clause never /// does, so `DROP TABLE` after an ALTER with no `;` starts a statement. -const TSQL_DROPPED_OBJECTS: [&str; 24] = [ +/// ALTER LOGIN's `DROP CREDENTIAL` clause reads as one too, the stricter +/// way. +const TSQL_DROPPED_OBJECTS: [&str; 48] = [ "TABLE", "VIEW", "PROCEDURE", @@ -160,6 +179,30 @@ const TSQL_DROPPED_OBJECTS: [&str; 24] = [ "FULLTEXT", "XML", "SECURITY", + "APPLICATION", + "ASYMMETRIC", + "AVAILABILITY", + "BROKER", + "CERTIFICATE", + "CONTRACT", + "CREDENTIAL", + "CRYPTOGRAPHIC", + "ENDPOINT", + "MASTER", + "MESSAGE", + "PARTITION", + "QUEUE", + "REMOTE", + "RESOURCE", + "ROUTE", + "SEARCH", + "SENSITIVITY", + "SERVER", + "SERVICE", + "SIGNATURE", + "COUNTERSIGNATURE", + "SYMMETRIC", + "WORKLOAD", ]; /// The strictest access among the statements of `sql`. @@ -337,16 +380,31 @@ fn writes(part: &[Code<'_>], grammar: SqlGrammar) -> bool { !is_call(part, index) && (is(token.text, &WRITE_WORDS) || (grammar == SqlGrammar::MsSql && is(token.text, &TSQL_WRITE_WORDS)) + // `OPTION (USE HINT ('…'))` is a hint inside a read. + || (grammar == SqlGrammar::MsSql && token.depth == 0 && is(token.text, &["USE"])) || (is(token.text, &["SHARE"]) && follows(index, &["FOR", "KEY", "IN"])) // `NEXT VALUE FOR` takes a number from a sequence. || (is(token.text, &["VALUE"]) && follows(index, &["NEXT"])) - || calls_mutator(part, index, grammar)) + || calls_mutator(part, index, grammar) + || names_pragma_optimize(token, grammar)) } - Kind::Quoted => calls_mutator(part, index, grammar), + Kind::Quoted => calls_mutator(part, index, grammar) || names_pragma_optimize(token, grammar), + Kind::Literal => names_pragma_optimize(token, grammar), _ => false, }) } +/// SQLite runs ANALYZE wherever `pragma_optimize` names a table, called +/// or not, and takes a quoted name or a string for a table name there. +fn names_pragma_optimize(token: &Code<'_>, grammar: SqlGrammar) -> bool { + let text = token.text; + let name = match token.kind { + Kind::Literal => text.get(1..text.len().saturating_sub(1)).unwrap_or_default(), + _ => text, + }; + grammar == SqlGrammar::Sqlite && name.eq_ignore_ascii_case("pragma_optimize") +} + /// A call of a built-in that changes state, its name quoted or not. /// PostgreSQL decodes the escapes in `U&"set\0076al"` before it looks the /// name up, under an escape character `UESCAPE` may change, so a name @@ -354,12 +412,18 @@ fn writes(part: &[Code<'_>], grammar: SqlGrammar) -> bool { fn calls_mutator(part: &[Code<'_>], index: usize, grammar: SqlGrammar) -> bool { let arguments = match decodes_unicode_names(grammar) && spelled_in_escapes(part, index) { true => after_uescape(part, index + 1), - false if changes_state(part[index].text) => index + 1, + false if changes_state(part[index].text) || runs_query_text(&part[index], grammar) => index + 1, false => return false, }; part.get(arguments).is_some_and(|next| next.kind == Kind::Open) } +/// DuckDB's `query('…')` runs the SELECT in its text, which can take a +/// value from a sequence. Elsewhere `query` is any function's name. +fn runs_query_text(token: &Code<'_>, grammar: SqlGrammar) -> bool { + grammar == SqlGrammar::DuckDb && is(token.text, &["QUERY"]) +} + /// The grammars whose scanner is PostgreSQL's, which reads `U&"…"`. fn decodes_unicode_names(grammar: SqlGrammar) -> bool { matches!(grammar, SqlGrammar::PostgreSql | SqlGrammar::DuckDb) @@ -553,6 +617,10 @@ fn statement_destroys(code: &[Code<'_>], grammar: SqlGrammar) -> bool { } fn statement_starts(code: &[Code<'_>], grammar: SqlGrammar) -> Vec { + let verbs: &[&str] = match grammar { + SqlGrammar::MsSql => &TSQL_STATEMENT_VERBS, + _ => &STATEMENT_VERBS, + }; let mut starts = Vec::new(); let mut lead: Option<&str> = None; for (index, token) in code.iter().enumerate() { @@ -560,7 +628,10 @@ fn statement_starts(code: &[Code<'_>], grammar: SqlGrammar) -> Vec { _ if is_call(code, index) => false, None => token.kind == Kind::Word, Some(before) if token.is_word(&["DELETE", "UPDATE"]) && before.kind == Kind::Open => true, - Some(before) if token.depth == 0 && token.is_word(&STATEMENT_VERBS) => { + // A T-SQL DELETE or UPDATE never holds a SELECT outside + // parentheses, so one there ends it, a cursor's after FOR too. + Some(_) if grammar == SqlGrammar::MsSql && token.depth == 0 && token.is_word(&["SELECT"]) => true, + Some(before) if token.depth == 0 && token.is_word(verbs) => { let led_in = leads_in(before, &code[index..], grammar) || before.kind == Kind::Comma; let alters = lead.is_some_and(|lead| is(lead, &["ALTER"])) && token.is_word(&["DROP", "ALTER"]) @@ -595,9 +666,15 @@ fn leads_in(before: &Code<'_>, code: &[Code<'_>], grammar: SqlGrammar) -> bool { } /// A DROP at `code[0]` that names a kind of object only a DROP statement -/// takes. +/// takes. An ALTER drops an event or a column as a clause, so those two +/// count only as the statements' own `EVENT SESSION`, `EVENT +/// NOTIFICATION` and `COLUMN MASTER KEY` or `COLUMN ENCRYPTION KEY`. fn drops_an_object(code: &[Code<'_>]) -> bool { - code[0].is_word(&["DROP"]) && code.get(1).is_some_and(|kind| kind.is_word(&TSQL_DROPPED_OBJECTS)) + let word = |at: usize, set: &[&str]| code.get(at).is_some_and(|token| token.is_word(set)); + word(0, &["DROP"]) + && (word(1, &TSQL_DROPPED_OBJECTS) + || (word(1, &["EVENT"]) && word(2, &["SESSION", "NOTIFICATION"])) + || (word(1, &["COLUMN"]) && word(2, &["MASTER", "ENCRYPTION"]) && word(3, &["KEY"]))) } /// The statement that starts at `code[0]` and runs to the end of `code` @@ -1264,14 +1341,14 @@ mod tests { "SELECT E'x'\n'\\' || ' || '\\' ; COMMIT; DELETE FROM orders; --'", "SELECT e'x' -- note\r\n -- note\n\t'\\' || ' || '\\' ; COMMIT; DELETE FROM orders; --'", "SELECT E'x'\n'y'\n'\\' || ' || '\\' ; COMMIT; DELETE FROM orders; --'", + // PostgreSQL 16 and later take a vertical tab as a space. + "SELECT E'x'\x0b\n'\\' || ' || '\\' ; COMMIT; DELETE FROM orders; --'", ] { - for grammar in [SqlGrammar::PostgreSql, SqlGrammar::DuckDb] { - assert_eq!( - access(sql, grammar), - StatementAccess::Destructive, - "{grammar:?}: {sql:?}" - ); - } + assert_eq!( + access(sql, SqlGrammar::PostgreSql), + StatementAccess::Destructive, + "{sql:?}" + ); } for sql in [ "SELECT E'x'\n'\\'; DELETE FROM orders; --'", @@ -1281,6 +1358,18 @@ mod tests { } } + #[test] + fn a_duckdb_string_ends_at_its_quote_and_a_backtick_quotes_a_name() { + // DuckDB 2.0 carries no backslash escapes into a string on the + // next line, and reads a backtick as a name quote. + for sql in [ + "SELECT E'x'\n'\\'; DELETE FROM orders; --'", + "SELECT 1 AS `'`; DROP TABLE orders; --'", + ] { + assert_eq!(access(sql, SqlGrammar::DuckDb), StatementAccess::Destructive, "{sql:?}"); + } + } + #[test] fn a_unicode_blank_before_a_keyword_does_not_hide_it() { for (sql, expected) in [ @@ -1411,19 +1500,34 @@ mod tests { "SELECT pg_stat_reset_shared('bgwriter')", "SELECT pg_wal_replay_pause()", "SELECT gin_clean_pending_list('i'::regclass)", + "SELECT * FROM crosstab('SELECT pid::text, ''x'', pg_terminate_backend(pid)::text FROM pg_stat_activity') AS ct(r text, x text)", + "SELECT * FROM connectby('tree', 'id', 'parent', '1', 0) AS t(id text, parent text, level int)", + "SELECT * FROM xpath_table('id', 'doc', 'docs', '/a', 'true') AS t(id int, a text)", + "SELECT pg_import_system_collations('public'::regnamespace)", + "SELECT heap_force_kill('orders'::regclass, ARRAY['(0,1)']::tid[])", + "SELECT pg_file_write('postgresql.auto.conf', 'x', true)", ] { assert_eq!(access(sql, SqlGrammar::PostgreSql), StatementAccess::Write, "{sql}"); } - for sql in ["SELECT GET_LOCK('l', 1)", "SELECT release_all_locks()"] { + // MariaDB folds `ſ` to `s` when it looks a function up. + for sql in [ + "SELECT GET_LOCK('l', 1)", + "SELECT release_all_locks()", + "SELECT relea\u{17f}e_lock('l')", + ] { assert_eq!(access(sql, SqlGrammar::MySql), StatementAccess::Write, "{sql}"); } assert_eq!( - access( - "SELECT dblink_build_sql_insert('t', '1', 1, '{1}', '{2}')", - SqlGrammar::PostgreSql - ), - StatementAccess::Read + access("SELECT * FROM query('SELECT nextval(''s'')')", SqlGrammar::DuckDb), + StatementAccess::Write ); + for sql in [ + "SELECT dblink_build_sql_insert('t', '1', 1, '{1}', '{2}')", + "SELECT dblink FROM links", + "SELECT file, url, query(1) FROM downloads", + ] { + assert_eq!(access(sql, SqlGrammar::PostgreSql), StatementAccess::Read, "{sql}"); + } } #[test] @@ -1484,6 +1588,12 @@ mod tests { "SELECT * FROM do\nUPDATE orders SET total = 0", "ALTER TABLE t ADD x int\nDROP TABLE y", "ALTER TABLE t ADD x int\nDELETE FROM y", + "ALTER TABLE t ADD x int\nDROP PARTITION SCHEME ps", + "ALTER TABLE t ADD x int\nDROP SYMMETRIC KEY k", + "ALTER TABLE t ADD x int\nDROP EVENT SESSION s ON SERVER", + "ALTER TABLE t ADD x int\nDROP COLUMN MASTER KEY k", + "SET NOCOUNT ON\nALTER TABLE t ADD x int\nDROP TABLE y", + "SET IDENTITY_INSERT t ON\nDELETE no", ] { assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Destructive, "{sql}"); } @@ -1496,8 +1606,68 @@ mod tests { "ALTER TABLE orders DROP CONSTRAINT orders_pk", "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN DELETE", "DECLARE c CURSOR FOR SELECT id FROM orders FOR UPDATE", + "ALTER EVENT SESSION s ON SERVER DROP EVENT sqlserver.lock_deadlock", + "ALTER ROLE r DROP MEMBER u", + "ALTER TABLE t DROP COLUMN master", + ] { + assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Write, "{sql}"); + } + } + + #[test] + fn a_where_in_a_later_sql_server_statement_does_not_bound_an_earlier_one() { + for sql in [ + "UPDATE orders SET total = 0\nDECLARE c CURSOR FOR SELECT id FROM orders WHERE id = 1", + "DELETE FROM orders\nSET @c = CURSOR FOR SELECT id FROM orders WHERE id = 1", + "DELETE FROM orders\nCREATE INDEX ix ON lines (order_id) WHERE order_id > 0", + "UPDATE orders SET total = 0\nCREATE STATISTICS s ON lines (order_id) WHERE order_id > 0", + ] { + assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Destructive, "{sql}"); + } + for sql in [ + "DECLARE c CURSOR FOR SELECT id FROM orders FOR UPDATE", + "GRANT CREATE TABLE TO someone", + "GRANT SELECT, UPDATE, DELETE ON orders TO someone", + ] { + assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Write, "{sql}"); + } + } + + #[test] + fn a_sql_server_statement_after_a_select_that_sets_session_state_writes() { + for sql in [ + "SELECT 1\nSET NOCOUNT ON", + "SELECT 1\nUSE master", + "SELECT 1\nRAISERROR('x', 10, 1) WITH LOG", + "SELECT 1\nWAITFOR DELAY '00:00:05'", + "SELECT 1\nDECLARE @n int", ] { assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Write, "{sql}"); } + for sql in [ + "SELECT a FROM t OPTION (USE HINT ('FORCE_LEGACY_CARDINALITY_ESTIMATION'))", + "SELECT a FROM t ORDER BY a OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", + "SELECT a FROM t WITH (NOLOCK) WHERE id = 1", + ] { + assert_eq!(access(sql, SqlGrammar::MsSql), StatementAccess::Read, "{sql}"); + } + } + + #[test] + fn a_pragma_named_as_a_table_runs_on_sqlite() { + for sql in [ + "SELECT * FROM pragma_optimize", + "SELECT * FROM main.pragma_optimize(0x10002)", + "SELECT * FROM orders, \"pragma_optimize\"", + "SELECT * FROM pragma_optimize WHERE arg = 65538", + "SELECT * FROM [PRAGMA_OPTIMIZE]", + "SELECT * FROM 'pragma_optimize'", + ] { + assert_eq!(access(sql, SqlGrammar::Sqlite), StatementAccess::Write, "{sql}"); + } + assert_eq!( + access("SELECT * FROM pragma_table_info('orders')", SqlGrammar::Sqlite), + StatementAccess::Read + ); } } diff --git a/core/crates/core/tests/script_corpus.rs b/core/crates/core/tests/script_corpus.rs index 843cffbc63..d214e72847 100644 --- a/core/crates/core/tests/script_corpus.rs +++ b/core/crates/core/tests/script_corpus.rs @@ -74,32 +74,31 @@ fn escape_string_backslash_quote() { } /// A string that follows `E'...'` after a line break continues it, and -/// the server reads it with backslash escapes on, so `\'` does not end it. +/// PostgreSQL reads it with backslash escapes on, so `\'` does not end it. #[test] fn escape_string_continues_past_a_line_break() { - for grammar in [SqlGrammar::PostgreSql, SqlGrammar::DuckDb] { - for continued in [ - "SELECT E'a'\n'\\'; b'", - "SELECT E'a'\r'\\'; b'", - "SELECT E'a' \t\x0c-- note\n\x0b -- note\r\n '\\'; b'", - "SELECT E'a'\n'b'\n'\\'; c'", - ] { - let text = format!("{continued}; SELECT 2;"); - assert_eq!( - statements(grammar, &text), - [continued, "SELECT 2"], - "{grammar:?}: {continued:?}" - ); - } + for continued in [ + "SELECT E'a'\n'\\'; b'", + "SELECT E'a'\r'\\'; b'", + "SELECT E'a' \t\x0c-- note\n\x0b -- note\r\n '\\'; b'", + "SELECT E'a'\n'b'\n'\\'; c'", + "SELECT E'a'\x0b\n'\\'; b'", + "SELECT E'a' \x0b-- note\n'\\'; b'", + ] { + let text = format!("{continued}; SELECT 2;"); + assert_eq!( + statements(SqlGrammar::PostgreSql, &text), + [continued, "SELECT 2"], + "{continued:?}" + ); } - // Without a line break, past a block comment, past anything but a - // blank or a `--` comment, or after a vertical tab before the line - // break, the server ends the string and so must the lexer. + // Without a line break, past a block comment, or past anything but a + // blank or a `--` comment, the server ends the string and so must the + // lexer. for apart in [ "SELECT E'a' '\\'", "SELECT E'a' /* note */\n'\\'", "SELECT E'a'\n, '\\'", - "SELECT E'a'\x0b\n'\\'", ] { let text = format!("{apart}; SELECT 2;"); assert_eq!( @@ -108,6 +107,13 @@ fn escape_string_continues_past_a_line_break() { "{apart:?}" ); } + assert_eq!( + statements( + SqlGrammar::PostgreSql, + "SELECT E'x'\x0b\n'\\' || ' || '\\' ; COMMIT; DELETE FROM orders; --'" + ), + ["SELECT E'x'\x0b\n'\\' || ' || '\\'", "COMMIT", "DELETE FROM orders"] + ); assert_eq!( plan(SqlGrammar::PostgreSql, "SELECT E'a'\n'b").diagnostics(), [ScriptDiagnostic::Unterminated { @@ -117,6 +123,16 @@ fn escape_string_continues_past_a_line_break() { ); } +#[test] +fn duckdb_ends_an_escape_string_at_its_quote_whatever_follows() { + let text = "SELECT E'x'\n'\\'; DELETE FROM orders; --'"; + assert_eq!( + statements(SqlGrammar::DuckDb, text), + ["SELECT E'x'\n'\\'", "DELETE FROM orders"] + ); + assert_eq!(statements(SqlGrammar::PostgreSql, text), [text]); +} + #[test] fn a_sqlite_parameter_runs_to_its_closing_parenthesis() { for parameter in [ @@ -614,9 +630,19 @@ fn duckdb_nests_comments_and_quotes_with_dollar_tags() { } #[test] -fn duckdb_reads_a_backtick_as_no_quote() { - let text = "SELECT `a;b` FROM t;"; - assert_eq!(statements(SqlGrammar::DuckDb, text), ["SELECT `a", "b` FROM t"]); +fn duckdb_quotes_a_name_in_backticks() { + assert_eq!( + statements(SqlGrammar::DuckDb, "SELECT `a;b` FROM t;"), + ["SELECT `a;b` FROM t"] + ); + assert_eq!( + statements(SqlGrammar::DuckDb, "SELECT 1 AS `a``b`; SELECT 2;"), + ["SELECT 1 AS `a``b`", "SELECT 2"] + ); + assert_eq!( + statements(SqlGrammar::DuckDb, "SELECT 1 AS `'`; DROP TABLE orders; --'"), + ["SELECT 1 AS `'`", "DROP TABLE orders"] + ); } #[test] diff --git a/core/crates/drivers/clickhouse/src/client_factory.rs b/core/crates/drivers/clickhouse/src/client_factory.rs index 3bc0d4fda5..aa1f66eff1 100644 --- a/core/crates/drivers/clickhouse/src/client_factory.rs +++ b/core/crates/drivers/clickhouse/src/client_factory.rs @@ -104,8 +104,11 @@ pub(crate) fn base_client( clickhouse::Client::with_http_client(builder.build(https)) } }; + // A typed fetch then reads RowBinary, which names no column types, so the login never parses a type name + // the server sent, however deep it nests, nor sizes a list by the column count the server claims. let mut client = client .with_url(service_url(scheme, target)) + .with_validation(false) .with_product_info(APPLICATION_NAME, environment.client_version.clone()); for (name, value) in COMMON_SETTINGS { client = client.with_setting(name, value); diff --git a/core/crates/drivers/clickhouse/src/dialect.rs b/core/crates/drivers/clickhouse/src/dialect.rs index 1379f42e29..8d7aca130d 100644 --- a/core/crates/drivers/clickhouse/src/dialect.rs +++ b/core/crates/drivers/clickhouse/src/dialect.rs @@ -435,6 +435,8 @@ const CREDENTIAL_RULES: CredentialRules = CredentialRules { "hudi", "hudiCluster", "url", + "urlCluster", + "ytsaurus", "odbc", "jdbc", "encrypt", @@ -689,6 +691,8 @@ mod tests { "RESTORE TABLE t FROM Disk('backups', 'b.zip') SETTINGS password = 'x'", "SELECT * FROM url('http://u:p@h/f.csv', CSV)", "CREATE TABLE t (id Int32) ENGINE = URL('http://u:p@h/f.csv', CSV)", + "SELECT * FROM urlCluster('c', 'http://u:p@h/f.csv', CSV)", + "SELECT * FROM ytsaurus('http://h:80', '//tmp/t', 'oauth-token', 'JSONEachRow')", "SELECT * FROM odbc('DSN=d;UID=u;PWD=p', 't')", "SELECT * FROM jdbc('jdbc:mysql://h/?user=u&password=p', 's', 't')", "SELECT encrypt('aes-256-gcm', 'text', 'key', 'iv')", diff --git a/core/crates/drivers/clickhouse/src/endpoint.rs b/core/crates/drivers/clickhouse/src/endpoint.rs index e2246c9969..85ad5105f7 100644 --- a/core/crates/drivers/clickhouse/src/endpoint.rs +++ b/core/crates/drivers/clickhouse/src/endpoint.rs @@ -181,6 +181,7 @@ impl ClickHouseEndpoint { } } +// Read as RowBinary, which carries no names, so the fields keep the order the login query selects them in. #[derive(Debug, serde::Deserialize, clickhouse::Row)] struct Identity { version: String, @@ -197,12 +198,112 @@ fn version_numbers(version: &str) -> (u32, u32, u32) { #[cfg(test)] mod tests { + use secrecy::SecretString; + use tablepro_core::{Credentials, NetworkEndpoint, TlsConfig}; + use tablepro_net::DirectTcpTransport; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + use super::*; + /// Answers every request with HTTP 200 and `body`, and keeps the socket + /// open, so the client reads the body to its stated length. + async fn answer_every_request(listener: TcpListener, body: Vec) { + let mut held = Vec::new(); + while let Ok((mut socket, _)) = listener.accept().await { + let mut request = Vec::new(); + let mut chunk = [0; 4096]; + while !request.windows(4).any(|end| end == b"\r\n\r\n") { + match socket.read(&mut chunk).await { + Ok(0) | Err(_) => break, + Ok(read) => request.extend_from_slice(&chunk[..read]), + } + } + let head = format!("HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n", body.len()); + if socket.write_all(head.as_bytes()).await.is_ok() && socket.write_all(&body).await.is_ok() { + held.push(socket); + } + } + } + + /// The login against a loopback server whose every answer is `body`, + /// run where a runtime worker would run it, on a 2 MiB stack. + fn identify_against(body: Vec) -> Result<(), DriverError> { + let worker = std::thread::Builder::new().stack_size(2 << 20); + let handle = worker + .spawn(move || { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("a runtime"); + runtime.block_on(async move { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("a listener"); + let port = listener.local_addr().expect("an address").port(); + let answering = tokio::spawn(answer_every_request(listener, body)); + let liveness = LivenessPolicy::DESKTOP; + let address = NetworkEndpoint::new("127.0.0.1", port).expect("an endpoint"); + let target = NetworkTarget { + transport: Arc::new(DirectTcpTransport::new(address, &liveness)), + credentials: Credentials::Password { + username: "default".to_owned(), + password: SecretString::from(String::new()), + }, + database: None, + tls: TlsConfig::disabled(), + options: Default::default(), + }; + let mut endpoint = + ClickHouseEndpoint::new(&target, liveness, &ConnectEnvironment::new("test"), false) + .expect("an endpoint"); + let identity = endpoint.identify().await.map(|_| ()); + answering.abort(); + identity + }) + }) + .expect("a worker"); + handle.join().expect("the login ran to an outcome") + } + + /// RowBinaryWithNamesAndTypes header: the column count, the names, + /// then the types, each string after its LEB128 length. + fn header(name: &str, type_name: &str) -> Vec { + let mut body = vec![1]; + for text in [name, type_name] { + let mut length = text.len(); + while length >= 0x80 { + body.push((length & 0x7f) as u8 | 0x80); + length >>= 7; + } + body.push(length as u8); + body.extend_from_slice(text.as_bytes()); + } + body + } + #[test] fn a_version_splits_into_major_minor_and_patch() { assert_eq!(version_numbers("24.8.4.13"), (24, 8, 4)); assert_eq!(version_numbers("25.10.1.1"), (25, 10, 1)); assert_eq!(version_numbers("not a version"), (0, 0, 0)); } + + #[test] + fn a_login_answer_naming_a_type_nested_100000_deep_fails_the_login() { + let nested = format!("{}UInt8{}", "Array(".repeat(100_000), ")".repeat(100_000)); + + let outcome = identify_against(header("version", &nested)); + + assert!(outcome.is_err(), "{outcome:?}"); + } + + #[test] + fn a_login_answer_counting_2_to_the_62_columns_fails_the_login() { + let mut body = vec![0x80; 8]; + body.push(0x40); + body.extend_from_slice(b"\x07version"); + + let outcome = identify_against(body); + + assert!(outcome.is_err(), "{outcome:?}"); + } } diff --git a/core/crates/drivers/clickhouse/src/key_expression.rs b/core/crates/drivers/clickhouse/src/key_expression.rs index 0eb49d5dd0..056ef2176e 100644 --- a/core/crates/drivers/clickhouse/src/key_expression.rs +++ b/core/crates/drivers/clickhouse/src/key_expression.rs @@ -11,8 +11,18 @@ use sqlparser::parser::Parser; use sqlparser::tokenizer::{Token, Tokenizer}; use tablepro_core::meta::IndexKey; -/// One key per entry of the list. A list sqlparser cannot read is kept -/// whole, as the server wrote it. +/// A key the server wrote is far shorter; longer text is kept whole +/// rather than tokenized. +const MAX_KEY_LEN: usize = 4096; + +/// sqlparser prints and drops an expression by recursing once per level, +/// and every level takes a token, so this bounds the stack a worker +/// spends on one entry. A debug build takes about 12 KB a level. +const MAX_ENTRY_TOKENS: usize = 64; + +/// One key per entry of the list. A list sqlparser cannot read, or one +/// too long or too deep to read on a worker's stack, is kept whole, as +/// the server wrote it. pub(crate) fn index_keys(text: &str) -> Vec { let text = text.trim(); if text.is_empty() { @@ -28,12 +38,43 @@ pub(crate) fn index_keys(text: &str) -> Vec { } fn parse_list(text: &str) -> Option> { + if text.len() > MAX_KEY_LEN { + return None; + } let dialect = ClickHouseDialect {}; + let tokens = Tokenizer::new(&dialect, text).tokenize().ok()?; + if !entries_within(&tokens, MAX_ENTRY_TOKENS) { + return None; + } let mut parser = Parser::new(&dialect).try_with_sql(text).ok()?; let keys = parser.parse_comma_separated(Parser::parse_order_by_expr).ok()?; (parser.peek_token().token == Token::EOF).then_some(keys) } +/// Whether every entry of the list, split at commas outside any +/// bracket, holds at most `limit` tokens. +fn entries_within(tokens: &[Token], limit: usize) -> bool { + let mut nesting = 0usize; + let mut in_entry = 0; + for token in tokens { + match token { + Token::Whitespace(_) => continue, + Token::Comma if nesting == 0 => { + in_entry = 0; + continue; + } + Token::LParen | Token::LBracket | Token::LBrace => nesting += 1, + Token::RParen | Token::RBracket | Token::RBrace => nesting = nesting.saturating_sub(1), + _ => {} + } + in_entry += 1; + if in_entry > limit { + return false; + } + } + true +} + fn key(entry: OrderByExpr) -> IndexKey { let descending = matches!(entry.options.sort, Some(OrderBySort::Desc)); match entry.expr { @@ -120,6 +161,48 @@ mod tests { ); } + fn expression(sql: &str) -> IndexKey { + IndexKey::Expression { + sql: sql.to_owned(), + descending: false, + } + } + + /// Runs `check` where a runtime worker would, on the 2 MiB default stack. + fn on_a_worker_stack(check: fn()) { + let worker = std::thread::Builder::new().stack_size(2 << 20); + worker.spawn(check).unwrap().join().unwrap(); + } + + #[test] + fn a_key_of_100000_chained_terms_is_kept_whole_on_a_worker_stack() { + on_a_worker_stack(|| { + // 100,000 terms, then 2,048 in 4,095 characters, under the length limit. + for terms in [100_000, 2048] { + let text = format!("{}a", "a+".repeat(terms - 1)); + + assert_eq!(index_keys(&text), vec![expression(&text)], "{terms} terms"); + } + }); + } + + #[test] + fn an_entry_at_the_token_limit_is_read_on_a_worker_stack() { + on_a_worker_stack(|| { + let chain = format!("{}a", "a+".repeat(MAX_ENTRY_TOKENS / 2 - 1)); + let calls = format!("{}a{}", "f(".repeat(21), ")".repeat(21)); + let past_the_limit = format!("{}a", "a+".repeat(MAX_ENTRY_TOKENS / 2)); + + assert_eq!( + index_keys(&format!("{chain}, id")), + vec![expression(&["a"; MAX_ENTRY_TOKENS / 2].join(" + ")), column("id")], + "each entry counts its own tokens" + ); + assert_eq!(index_keys(&calls), vec![expression(&calls)]); + assert_eq!(index_keys(&past_the_limit), vec![expression(&past_the_limit)]); + }); + } + #[test] fn key_columns_follow_the_expression_not_the_table() { let flagged = ["id".to_owned(), "d".to_owned()]; diff --git a/core/crates/drivers/duckdb/tests/integration/open_semantics.rs b/core/crates/drivers/duckdb/tests/integration/open_semantics.rs index a8f2123866..596c5a2b91 100644 --- a/core/crates/drivers/duckdb/tests/integration/open_semantics.rs +++ b/core/crates/drivers/duckdb/tests/integration/open_semantics.rs @@ -205,11 +205,20 @@ async fn an_exported_query_cannot_write() { let committed = export("SELECT 1; COMMIT; INSERT INTO t VALUES (1)").await; let inserted = export("INSERT INTO t VALUES (2)").await; let read = export("SELECT 'COMMIT' AS word").await; + // DuckDB quotes an identifier in backticks, and a string that + // continues an E-string on the next line keeps no backslash escape. + let hidden = [ + export("SELECT 1 AS `'`; COMMIT; INSERT INTO t VALUES (3); --'").await, + export("SELECT E'x'\n'\\'; COMMIT; INSERT INTO t VALUES (3); --'").await, + ]; assert_eq!( committed.err(), Some(DriverError::ReadOnly(ReadOnlyRefusal::ClientGuard)) ); + for refused in hidden { + assert_eq!(refused.err(), Some(DriverError::ReadOnly(ReadOnlyRefusal::ClientGuard))); + } assert!( matches!( &inserted, diff --git a/core/crates/drivers/mongodb/src/execute.rs b/core/crates/drivers/mongodb/src/execute.rs index c476d3811a..f079360d7b 100644 --- a/core/crates/drivers/mongodb/src/execute.rs +++ b/core/crates/drivers/mongodb/src/execute.rs @@ -73,7 +73,7 @@ pub(crate) async fn start( }); } let first = match cursor.advance(session).await { - Ok(true) => crate::nested_document::current_document(&cursor).ok(), + Ok(true) => Some(crate::nested_document::current_document(&cursor)?), Ok(false) => None, Err(error) => return Err(endpoint.explain(&error, LossPhase::Statement).await), }; diff --git a/core/crates/drivers/mongodb/src/extended_json.rs b/core/crates/drivers/mongodb/src/extended_json.rs index f8c5e2926f..bcd251ead3 100644 --- a/core/crates/drivers/mongodb/src/extended_json.rs +++ b/core/crates/drivers/mongodb/src/extended_json.rs @@ -2,7 +2,7 @@ use std::fmt::Write; use base64::Engine; use base64::engine::general_purpose::STANDARD; -use bson::Bson; +use bson::{Bson, Document}; use chrono::{DateTime, Datelike}; /// A BSON value as extended JSON a person can read and edit: relaxed, @@ -27,18 +27,7 @@ fn write_value(value: &Bson, out: &mut String) { Bson::String(text) => write_string(text, out), Bson::Boolean(flag) => out.push_str(if *flag { "true" } else { "false" }), Bson::Null => out.push_str("null"), - Bson::Document(document) => { - out.push('{'); - for (index, (key, item)) in document.iter().enumerate() { - if index > 0 { - out.push_str(", "); - } - write_string(key, out); - out.push_str(": "); - write_value(item, out); - } - out.push('}'); - } + Bson::Document(document) => write_document(document, out), Bson::Array(items) => { out.push('['); for (index, item) in items.iter().enumerate() { @@ -75,7 +64,7 @@ fn write_value(value: &Bson, out: &mut String) { out.push_str("{\"$code\": "); write_string(&code.code, out); out.push_str(", \"$scope\": "); - write_value(&Bson::Document(code.scope.clone()), out); + write_document(&code.scope, out); out.push('}'); } Bson::Symbol(symbol) => wrapped(out, "$symbol", |out| write_string(symbol, out)), @@ -86,6 +75,19 @@ fn write_value(value: &Bson, out: &mut String) { } } +fn write_document(document: &Document, out: &mut String) { + out.push('{'); + for (index, (key, item)) in document.iter().enumerate() { + if index > 0 { + out.push_str(", "); + } + write_string(key, out); + out.push_str(": "); + write_value(item, out); + } + out.push('}'); +} + fn wrapped(out: &mut String, key: &str, inner: impl FnOnce(&mut String)) { out.push('{'); write_string(key, out); @@ -133,7 +135,7 @@ fn write_date(millis: i64, out: &mut String) { mod tests { use bson::oid::ObjectId; use bson::spec::BinarySubtype; - use bson::{Binary, Decimal128, Timestamp, doc}; + use bson::{Binary, Decimal128, JavaScriptCodeWithScope, Timestamp, doc}; use super::*; @@ -161,6 +163,26 @@ mod tests { ); } + #[test] + fn nested_javascript_scopes_keep_field_order_and_value_types() { + let inner = Bson::JavaScriptCodeWithScope(JavaScriptCodeWithScope { + code: "return \"x\"".to_owned(), + scope: doc! { "z": 2_i64, "a": [true, null] }, + }); + let outer = Bson::JavaScriptCodeWithScope(JavaScriptCodeWithScope { + code: "return inner".to_owned(), + scope: doc! { "z": 1_i32, "inner": inner, "a": "last" }, + }); + + assert_eq!( + extended_json(&outer), + concat!( + r#"{"$code": "return inner", "$scope": {"z": 1, "inner": {"$code": "return \"x\"", "#, + r#""$scope": {"z": {"$numberLong": "2"}, "a": [true, null]}}, "a": "last"}}"# + ) + ); + } + #[test] fn the_other_types_are_written_as_extended_json() { let cases = [ diff --git a/core/crates/drivers/mongodb/src/nested_document.rs b/core/crates/drivers/mongodb/src/nested_document.rs index 7b35ba1118..68781c9957 100644 --- a/core/crates/drivers/mongodb/src/nested_document.rs +++ b/core/crates/drivers/mongodb/src/nested_document.rs @@ -1,84 +1,253 @@ -//! A cursor's documents, taken out only once their nesting is known. -//! -//! bson turns raw bytes into a `Document` by calling itself once per -//! level, and writing one out and dropping it do the same, so a document -//! nested deep enough would overflow the stack of the thread that reads -//! it. - -use bson::Document; -use bson::raw::{RawBsonRef, RawDocument}; +//! Owned BSON values recurse when formatted or dropped, so cursor +//! documents need a depth bound before those operations. + +use bson::raw::{RawBsonRef, RawDocument, RawIter}; +use bson::{Bson, Document, JavaScriptCodeWithScope}; use mongodb::SessionCursor; use tablepro_core::DriverError; -/// Deeper than MongoDB stores a document (100 levels), so only a hostile -/// server sends one. +/// Leaves room above MongoDB's 100-level storage limit. const MAX_DEPTH: usize = 128; -/// The cursor's current document, refused when it nests deeper than -/// [`MAX_DEPTH`]. pub(crate) fn current_document(cursor: &SessionCursor) -> Result { - if !within_depth(cursor.current()) { - return Err(DriverError::Decode { - column: String::new(), - detail: format!("the document nests deeper than {MAX_DEPTH} levels"), - }); - } - cursor.deserialize_current().map_err(|error| DriverError::Decode { - column: String::new(), - detail: error.to_string(), - }) + decode_document(cursor.current()) } -/// Walks the levels with a stack of its own, so the walk itself stays -/// off the thread's stack. A malformed element is left for bson to -/// report. -fn within_depth(document: &RawDocument) -> bool { - let mut open = vec![document.iter_elements()]; - while let Some(elements) = open.last_mut() { - let Some(element) = elements.next() else { - open.pop(); +fn decode_document(raw: &RawDocument) -> Result { + let mut open = vec![Frame { + elements: raw.iter_elements(), + key: "", + kind: ContainerKind::Document, + values: Vec::new(), + }]; + while let Some(frame) = open.last_mut() { + let Some(element) = frame.elements.next() else { + let Some(completed) = open.pop() else { break }; + if open.is_empty() { + return Ok(completed.into_document()); + } + let key = completed.key; + if let Some(parent) = open.last_mut() { + parent.values.push((key, completed.into_value())); + } continue; }; - let nested = match element.and_then(|element| element.value()) { - Ok(RawBsonRef::Document(inner)) => inner.iter_elements(), - Ok(RawBsonRef::Array(inner)) => inner.iter_elements(), - Ok(RawBsonRef::JavaScriptCodeWithScope(code)) => code.scope.iter_elements(), - _ => continue, + let element = element.map_err(decode_error)?; + let key = element.key().as_str(); + // Borrow containers: direct BSON conversion copies their remaining + // raw bytes at each level, while Serde can exhaust a worker stack. + let (elements, kind) = match element.value().map_err(decode_error)? { + RawBsonRef::Document(document) => (document.iter_elements(), ContainerKind::Document), + RawBsonRef::Array(array) => (array.iter_elements(), ContainerKind::Array), + RawBsonRef::JavaScriptCodeWithScope(code) => (code.scope.iter_elements(), ContainerKind::Scope(code.code)), + scalar => { + frame.values.push((key, Bson::try_from(scalar).map_err(decode_error)?)); + continue; + } }; if open.len() >= MAX_DEPTH { - return false; + return Err(decode_error(format!( + "the document nests deeper than {MAX_DEPTH} levels" + ))); + } + open.push(Frame { + elements, + key, + kind, + values: Vec::new(), + }); + } + Err(decode_error("the document has no outer container")) +} + +fn decode_error(error: impl std::fmt::Display) -> DriverError { + DriverError::Decode { + column: String::new(), + detail: error.to_string(), + } +} + +struct Frame<'a> { + elements: RawIter<'a>, + key: &'a str, + kind: ContainerKind<'a>, + values: Vec<(&'a str, Bson)>, +} + +#[derive(Clone, Copy)] +enum ContainerKind<'a> { + Document, + Array, + Scope(&'a str), +} + +impl Frame<'_> { + fn into_document(self) -> Document { + self.values + .into_iter() + .map(|(key, value)| (key.to_owned(), value)) + .collect() + } + + fn into_value(self) -> Bson { + match self.kind { + ContainerKind::Document => Bson::Document(self.into_document()), + ContainerKind::Array => Bson::Array(self.values.into_iter().map(|(_, value)| value).collect()), + ContainerKind::Scope(code) => Bson::JavaScriptCodeWithScope(JavaScriptCodeWithScope { + code: code.to_owned(), + scope: self.into_document(), + }), } - open.push(nested); } - true } #[cfg(test)] mod tests { - use bson::{Bson, RawDocumentBuf, doc}; + use bson::raw::{RawArrayBuf, RawBson, RawJavaScriptCodeWithScope, cstr}; + use bson::{RawDocumentBuf, doc}; use super::*; - /// A document `levels` containers deep, the outer document included, - /// arrays and documents in turn. - fn nested(levels: usize) -> RawDocumentBuf { - let mut value = Bson::Int32(1); + #[derive(Clone, Copy)] + enum Shape { + Alternating, + Documents, + Arrays, + Scopes, + } + + fn nested_value(levels: usize, shape: Shape, mut value: RawBson) -> RawDocumentBuf { for level in 1..levels { - value = match level % 2 { - 0 => Bson::Document(doc! { "inner": value }), - _ => Bson::Array(vec![value]), + value = match (shape, level % 2) { + (Shape::Arrays, _) | (Shape::Alternating, 1) => { + let mut array = RawArrayBuf::new(); + array.push(value.as_raw_bson_ref()); + RawBson::Array(array) + } + (Shape::Scopes, _) => { + let mut scope = RawDocumentBuf::new(); + scope.append(cstr!("inner"), value.as_raw_bson_ref()); + RawBson::JavaScriptCodeWithScope(RawJavaScriptCodeWithScope { + code: "return inner".to_owned(), + scope, + }) + } + _ => { + let mut document = RawDocumentBuf::new(); + document.append(cstr!("inner"), value.as_raw_bson_ref()); + RawBson::Document(document) + } }; } - RawDocumentBuf::try_from(&doc! { "root": value }).unwrap() + let mut document = RawDocumentBuf::new(); + document.append(cstr!("root"), value.as_raw_bson_ref()); + document + } + + fn nested(levels: usize) -> RawDocumentBuf { + nested_value(levels, Shape::Alternating, RawBson::Int32(1)) } #[test] fn a_document_at_the_limit_is_read() { - assert!(within_depth(&nested(MAX_DEPTH))); + assert!(decode_document(&nested(MAX_DEPTH)).is_ok()); } #[test] fn a_document_past_the_limit_is_refused() { - assert!(!within_depth(&nested(MAX_DEPTH + 1))); + assert!(matches!( + decode_document(&nested(MAX_DEPTH + 1)), + Err(DriverError::Decode { .. }) + )); + } + + #[test] + fn a_document_at_the_limit_is_read_and_written_on_a_worker_stack() { + let raw = nested(MAX_DEPTH); + let worker = std::thread::Builder::new().stack_size(2 << 20); + let handle = worker + .spawn(move || { + let document = decode_document(&raw).unwrap(); + let written = crate::extended_json::extended_json(&Bson::Document(document)); + assert_eq!(written.matches(['{', '[']).count(), MAX_DEPTH, "{written}"); + }) + .unwrap(); + handle.join().unwrap(); + } + + #[test] + fn every_container_shape_fits_a_worker_stack_at_supported_depths() { + for levels in [100, MAX_DEPTH] { + for shape in [Shape::Documents, Shape::Arrays, Shape::Alternating, Shape::Scopes] { + let raw = nested_value(levels, shape, RawBson::Int32(1)); + std::thread::Builder::new() + .stack_size(2 << 20) + .spawn(move || { + let document = decode_document(&raw).unwrap(); + let written = crate::extended_json::extended_json(&Bson::Document(document)); + let containers = match shape { + Shape::Scopes => 1 + 2 * (levels - 1), + _ => levels, + }; + assert_eq!(written.matches(['{', '[']).count(), containers); + }) + .unwrap() + .join() + .unwrap(); + } + } + } + + #[test] + fn every_container_shape_is_refused_past_the_limit() { + for shape in [Shape::Documents, Shape::Arrays, Shape::Alternating, Shape::Scopes] { + let raw = nested_value(MAX_DEPTH + 1, shape, RawBson::Int32(1)); + assert!(matches!(decode_document(&raw), Err(DriverError::Decode { .. }))); + } + } + + #[test] + fn a_large_value_in_deep_scopes_is_read_and_written_on_a_worker_stack() { + let bytes = 1 << 20; + let raw = nested_value(MAX_DEPTH, Shape::Scopes, RawBson::String("x".repeat(bytes))); + std::thread::Builder::new() + .stack_size(2 << 20) + .spawn(move || { + let document = decode_document(&raw).unwrap(); + let written = crate::extended_json::extended_json(&Bson::Document(document)); + assert_eq!(written.bytes().filter(|byte| *byte == b'x').count(), bytes); + assert_eq!(written.matches('{').count(), 1 + 2 * (MAX_DEPTH - 1)); + }) + .unwrap() + .join() + .unwrap(); + } + + #[test] + fn decoding_keeps_field_order_and_bson_types() { + let expected = doc! { + "z": 1_i32, + "a": 2_i64, + "date": bson::DateTime::from_millis(123), + "nested": [true, null, { "last": "value" }], + }; + let raw = RawDocumentBuf::try_from(&expected).unwrap(); + let decoded = decode_document(&raw).unwrap(); + assert_eq!(decoded, expected); + assert_eq!( + decoded.keys().map(String::as_str).collect::>(), + ["z", "a", "date", "nested"] + ); + } + + #[test] + fn a_malformed_nested_value_returns_a_decode_error() { + let raw = RawDocumentBuf::try_from(&doc! { "value": 1 }).unwrap(); + let mut bytes = raw.as_bytes().to_vec(); + bytes[4] = 0x42; + let malformed = RawDocumentBuf::from_bytes(bytes).unwrap(); + let mut outer = RawDocumentBuf::new(); + outer.append(cstr!("nested"), RawBsonRef::Document(&malformed)); + assert!(matches!(decode_document(&outer), Err(DriverError::Decode { .. }))); } } diff --git a/core/crates/drivers/mongodb/src/stream_read.rs b/core/crates/drivers/mongodb/src/stream_read.rs index 4e5b4c726d..fc11685ecb 100644 --- a/core/crates/drivers/mongodb/src/stream_read.rs +++ b/core/crates/drivers/mongodb/src/stream_read.rs @@ -3,7 +3,6 @@ use std::future::Future; use std::pin::Pin; use std::sync::Arc; -use bson::Document; use mongodb::{Client, ClientSession}; use tablepro_core::sql_syntax::StatementAccess; use tablepro_core::sql_syntax::mongo::{ShellStatement, access_of, parse_statement, statement_ranges}; @@ -112,10 +111,7 @@ async fn census( loop { match cursor.advance(session).await { Ok(true) => { - let document: Document = cursor.deserialize_current().map_err(|error| DriverError::Decode { - column: String::new(), - detail: error.to_string(), - })?; + let document = crate::nested_document::current_document(&cursor)?; for key in document.keys() { if seen.insert(key.clone()) { fields.push(key.clone()); diff --git a/core/crates/drivers/mssql/src/dialect.rs b/core/crates/drivers/mssql/src/dialect.rs index d64360f879..358b8b29c7 100644 --- a/core/crates/drivers/mssql/src/dialect.rs +++ b/core/crates/drivers/mssql/src/dialect.rs @@ -499,13 +499,15 @@ const CREDENTIAL_RULES: CredentialRules = CredentialRules { "sysmail_add_account_sp", "sysmail_update_account_sp", // Replication and log shipping: the distributor_admin, Publisher, - // Subscriber, agent job and monitor passwords, as arguments or as - // a property value. + // Subscriber, agent job, snapshot FTP and monitor passwords, as + // arguments or as a property value. "sp_adddistributor", "sp_changedistributor_password", "sp_adddistributiondb", "sp_adddistpublisher", "sp_changedistpublisher", + "sp_changepublication", + "sp_changemergepublication", "sp_addsubscriber", "sp_changesubscriber", "sp_addlogreader_agent", @@ -534,7 +536,7 @@ const CREDENTIAL_RULES: CredentialRules = CredentialRules { "sp_add_log_shipping_secondary_database", "sp_change_log_shipping_secondary_database", ], - secret_parameters: &["PASSWORD", "PWD"], + secret_parameters: &["PASSWORD", "PASSWD", "PWD"], }; #[cfg(test)] @@ -868,6 +870,10 @@ mod tests { "EXEC('CREATE LOGIN l WITH PASSWORD = ''p''')", "EXEC sp_executesql N'CREATE USER u WITH PASSWORD = ''p'''", "DECLARE @s nvarchar(99) = N'CREATE LOGIN l WITH PASSWORD = ''p'''; EXEC (@s)", + "DECLARE @s nvarchar(max) = N'CREATE LOGIN app WITH PASSWORD = ''Hunter2!'''", + "EXEC sp_changepublication @publication = N'pub', @property = N'ftp_password', @value = N'Hunter2!', @force_invalidate_snapshot = 1", + "EXEC sp_changemergepublication N'pub', N'ftp_password', N'Hunter2!'", + "EXEC dbo.add_app_login @login = N'app', @passwd = N'Hunter2!'", ] { assert_eq!( dialect().history_recording(sql), @@ -888,6 +894,7 @@ mod tests { "INSERT INTO [OPENROWSET] (c) VALUES (1)", "SELECT 1\nPRINT sp_addlogin", "CREATE NONCLUSTERED INDEX ix ON dbo.sp_password (id)", + "SELECT passwd FROM dbo.users", ] { assert_eq!(dialect().history_recording(sql), HistoryRecording::Record, "{sql}"); } diff --git a/core/crates/drivers/mssql/src/read_only_guard.rs b/core/crates/drivers/mssql/src/read_only_guard.rs index 203c682661..d7778f2432 100644 --- a/core/crates/drivers/mssql/src/read_only_guard.rs +++ b/core/crates/drivers/mssql/src/read_only_guard.rs @@ -98,6 +98,8 @@ mod tests { "SET NOCOUNT ON", "SELECT 1\nDELETE FROM t", "SELECT * FROM OPENQUERY(remote, 'DELETE FROM t')", + "SHOW: UPDATE t SET a = 1 WHERE id = 1", + "SHOW: COMMIT TRANSACTION UPDATE t SET a = 1 WHERE id = 1 BEGIN TRANSACTION", ] { assert_eq!( refuse_writes(write), diff --git a/core/crates/drivers/mysql/src/connection.rs b/core/crates/drivers/mysql/src/connection.rs index 67a0996444..6c823e9ccc 100644 --- a/core/crates/drivers/mysql/src/connection.rs +++ b/core/crates/drivers/mysql/src/connection.rs @@ -176,8 +176,11 @@ impl MysqlConnection { Ok(engine.atomicity()) } + // The server ends a read-only transaction before DDL and its other + // implicit commits, so the session is held read-only as well. fn stream_limits(&self) -> SessionLimits { SessionLimits { + read_only: true, statement_timeout: None, ..SessionLimits::workspace(&self.settings) } diff --git a/core/crates/drivers/mysql/src/dialect.rs b/core/crates/drivers/mysql/src/dialect.rs index f92e26cc67..d27c77b792 100644 --- a/core/crates/drivers/mysql/src/dialect.rs +++ b/core/crates/drivers/mysql/src/dialect.rs @@ -466,6 +466,7 @@ const CREDENTIAL_RULES: CredentialRules = CredentialRules { "DES_DECRYPT", "ENCODE", "DECODE", + "VALIDATE_PASSWORD_STRENGTH", ], secret_parameters: &[], }; @@ -816,6 +817,9 @@ mod tests { "SELECT AES_DECRYPT(secret, UNHEX(SHA2('passphrase', 512))) FROM t", "SELECT DES_ENCRYPT('text', 'key'), DES_DECRYPT(secret, 'key') FROM t", "SELECT ENCODE('text', 'pass'), DECODE(secret, 'pass') FROM t", + "SELECT VALIDATE_PASSWORD_STRENGTH('Hunter2!')", + "PREPARE s FROM 'CREATE USER app IDENTIFIED BY ''p'''; EXECUTE s; DEALLOCATE PREPARE s", + "SET @s = 'CREATE USER app IDENTIFIED BY ''Hunter2!'''", ] { assert_eq!( dialect(MySqlFlavour::MySql).history_recording(sql), @@ -823,6 +827,10 @@ mod tests { "{sql}" ); } + assert_eq!( + dialect(MySqlFlavour::MariaDb).history_recording("EXECUTE IMMEDIATE 'CREATE USER app IDENTIFIED BY ''p'''"), + HistoryRecording::Withhold(tablepro_core::sql_syntax::WithholdReason::CredentialStatement) + ); } #[test] diff --git a/core/crates/drivers/mysql/tests/integration/user_read.rs b/core/crates/drivers/mysql/tests/integration/user_read.rs index 678a63e7d0..8e2fe0d7f0 100644 --- a/core/crates/drivers/mysql/tests/integration/user_read.rs +++ b/core/crates/drivers/mysql/tests/integration/user_read.rs @@ -2,7 +2,7 @@ use tablepro_core::{Connection, DriverError, ReadStatement, StreamSummary, UserSql}; -use crate::support::{CALL, EventSink, Events, FLAVOURS, connect, setup, start, within}; +use crate::support::{CALL, EventSink, Events, FLAVOURS, connect, count, setup, start, statement, within}; async fn read(connection: &dyn Connection, sql: &str) -> (Result, Events) { let events = Events::default(); @@ -44,3 +44,36 @@ async fn a_read_runs_one_statement_and_takes_any_kind_the_server_parses() { } } } + +/// The server ends the read's read-only transaction before DDL, so the +/// session itself is read-only too. The statement goes below the check +/// of user SQL, as a read the app built. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "requires docker"] +async fn a_read_on_a_writable_connection_creates_no_table() { + for flavour in FLAVOURS { + let fixture = start(flavour).await; + let connection = connect(&fixture, false).await; + let events = Events::default(); + + let outcome = connection + .stream_read( + &ReadStatement::Generated(statement("CREATE TABLE made_by_a_read (a int)")), + Box::new(EventSink::new(&events)), + within(CALL), + ) + .await; + + assert!(outcome.is_err(), "{flavour:?}: {outcome:?}"); + assert_eq!( + count( + connection.as_ref(), + "SELECT COUNT(*) FROM information_schema.TABLES \ + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'made_by_a_read'" + ) + .await, + 0, + "{flavour:?}" + ); + } +} diff --git a/core/crates/drivers/postgres/src/connection.rs b/core/crates/drivers/postgres/src/connection.rs index 569229ac35..374e3e59d5 100644 --- a/core/crates/drivers/postgres/src/connection.rs +++ b/core/crates/drivers/postgres/src/connection.rs @@ -32,7 +32,9 @@ use crate::physical::PgEndpoint; use crate::pool::{probe_timeouts, workspace_timeouts}; use crate::standalone::Standalone; use crate::stream_read::stream; -use crate::workspace_call::{Access, Preamble, ReadOp, count_op, flight, query_bound_op, read_op, statement_bound}; +use crate::workspace_call::{ + Access, BoxFuture, CATALOG_PATH, Preamble, ReadOp, count_op, flight, query_bound_op, read_op, statement_bound, +}; use crate::write_batch::{WritePlan, perform, precheck}; /// A workspace's link to one PostgreSQL server: a pool of four @@ -97,14 +99,62 @@ impl PgConnection { } } + /// Run one read of the user's data, whose names resolve through the + /// session's search_path as the user's own SQL does. + async fn read(&self, options: CallOptions, op: ReadOp) -> Result { + let begin = self.read_preamble(&options); + self.read_in(options, begin, op).await + } + + /// Run one read of the catalog, with `pg_catalog` searched first. + async fn catalog_read(&self, options: CallOptions, op: ReadOp) -> Result { + let begin = self.read_preamble(&options).with_catalog_path(); + self.read_in(options, begin, op).await + } + + // A user table can share a catalog table's name. Resolve it before + // changing the path, then retain its OID through every metadata read. + async fn catalog_relation_read(&self, options: CallOptions, relation: String, op: F) -> Result + where + T: Send + 'static, + F: for<'a> FnOnce(&'a tokio_postgres::Client, String) -> BoxFuture<'a, Result> + Send + 'static, + { + let begin = self.read_preamble(&options).with_dependent_queries(); + self.read_in( + options, + begin, + read_op(move |client| { + Box::pin(async move { + let rows = + metadata::text_rows(client, "SELECT $1::pg_catalog.regclass::pg_catalog.oid", &[&relation]) + .await?; + let row = rows + .first() + .ok_or_else(|| DriverError::Protocol("the relation lookup returned no row".to_owned()))?; + let oid: u32 = row.try_get(0).map_err(metadata::decode_error("pg_class"))?; + client + .batch_execute(CATALOG_PATH) + .await + .map_err(|error| map_error(&error))?; + op(client, oid.to_string()).await + }) + }), + ) + .await + } + /// Run one read in its own read-only transaction on a pooled /// connection, owned by a task that outlives the caller. - async fn read(&self, options: CallOptions, op: ReadOp) -> Result { + async fn read_in( + &self, + options: CallOptions, + begin: Preamble, + op: ReadOp, + ) -> Result { self.ensure_open()?; let stop = Arc::new(PgStop::new(self.endpoint.clone())); let pool = self.pool.clone(); let timeouts = workspace_timeouts(self.liveness()); - let begin = self.read_preamble(&options); let work_stop = stop.clone(); let outcome = self .calls @@ -257,18 +307,18 @@ impl Connection for PgConnection { } async fn list_databases(&self, options: CallOptions) -> Result, DriverError> { - self.read(options, read_op(|client| Box::pin(metadata::list_databases(client)))) + self.catalog_read(options, read_op(|client| Box::pin(metadata::list_databases(client)))) .await } async fn list_schemas(&self, options: CallOptions) -> Result, DriverError> { - self.read(options, read_op(|client| Box::pin(metadata::list_schemas(client)))) + self.catalog_read(options, read_op(|client| Box::pin(metadata::list_schemas(client)))) .await } async fn list_objects(&self, scope: &ObjectScope, options: CallOptions) -> Result, DriverError> { let scope = scope.clone(); - self.read( + self.catalog_read( options, read_op(move |client| Box::pin(async move { metadata::list_objects(client, &scope).await })), ) @@ -281,7 +331,7 @@ impl Connection for PgConnection { options: CallOptions, ) -> Result, DriverError> { let object = object.clone(); - self.read( + self.catalog_read( options, read_op(move |client| Box::pin(async move { metadata::fetch_object_definition(client, &object).await })), ) @@ -291,21 +341,17 @@ impl Connection for PgConnection { async fn fetch_columns(&self, table: &TableRef, options: CallOptions) -> Result, DriverError> { let relation = self.relation(table); let columns_sql = self.columns_sql(); - self.read( - options, - read_op(move |client| { - Box::pin(async move { metadata::fetch_columns(client, columns_sql, &relation).await }) - }), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_columns(client, columns_sql, &relation).await }) + }) .await } async fn fetch_indexes(&self, table: &TableRef, options: CallOptions) -> Result, DriverError> { let relation = self.relation(table); - self.read( - options, - read_op(move |client| Box::pin(async move { metadata::fetch_indexes(client, &relation).await })), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_indexes(client, &relation).await }) + }) .await } @@ -315,10 +361,9 @@ impl Connection for PgConnection { options: CallOptions, ) -> Result, DriverError> { let relation = self.relation(table); - self.read( - options, - read_op(move |client| Box::pin(async move { metadata::fetch_foreign_keys(client, &relation).await })), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_foreign_keys(client, &relation).await }) + }) .await } @@ -328,12 +373,9 @@ impl Connection for PgConnection { options: CallOptions, ) -> Result, DriverError> { let relation = self.relation(table); - self.read( - options, - read_op(move |client| { - Box::pin(async move { metadata::fetch_outbound_foreign_keys(client, &relation).await }) - }), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_outbound_foreign_keys(client, &relation).await }) + }) .await } @@ -343,12 +385,9 @@ impl Connection for PgConnection { options: CallOptions, ) -> Result, DriverError> { let relation = self.relation(table); - self.read( - options, - read_op(move |client| { - Box::pin(async move { metadata::fetch_inbound_foreign_keys(client, &relation).await }) - }), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_inbound_foreign_keys(client, &relation).await }) + }) .await } @@ -360,30 +399,25 @@ impl Connection for PgConnection { let relation = self.relation(table); let columns_sql = self.columns_sql(); let table = table.clone(); - self.read( - options, - read_op(move |client| { - Box::pin(async move { metadata::fetch_table_definition(client, columns_sql, &table, &relation).await }) - }), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::fetch_table_definition(client, columns_sql, &table, &relation).await }) + }) .await } async fn row_identity(&self, table: &TableRef, options: CallOptions) -> Result { let relation = self.relation(table); - self.read( - options, - read_op(move |client| Box::pin(async move { metadata::row_identity(client, &relation).await })), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::row_identity(client, &relation).await }) + }) .await } async fn table_estimate(&self, table: &TableRef, options: CallOptions) -> Result { let relation = self.relation(table); - self.read( - options, - read_op(move |client| Box::pin(async move { metadata::table_estimate(client, &relation).await })), - ) + self.catalog_relation_read(options, relation, move |client, relation| { + Box::pin(async move { metadata::table_estimate(client, &relation).await }) + }) .await } @@ -473,7 +507,7 @@ impl Connection for PgConnection { } async fn resolve_commit(&self, token: &CommitToken, options: CallOptions) -> Result { - self.read(options, resolve_op(token)?).await + self.catalog_read(options, resolve_op(token)?).await } async fn open_editor_session( diff --git a/core/crates/drivers/postgres/src/ddl.rs b/core/crates/drivers/postgres/src/ddl.rs index ac37814718..c02eb0cd3a 100644 --- a/core/crates/drivers/postgres/src/ddl.rs +++ b/core/crates/drivers/postgres/src/ddl.rs @@ -220,6 +220,8 @@ impl DdlDialect for PgDialect { /// the USING result, refuses a value that is too long, as an INSERT /// would. A bit string takes the cast to `varbit` only from another bit /// string: an integer has a cast to `bit(n)` and none to `varbit`. +/// `varchar` is a keyword the grammar always reads as the built-in, while +/// `bpchar` and `varbit` are names the search_path would resolve. fn unlimited_cast(original: &ColumnKind, target: &ColumnKind) -> Option { match (original, target) { ( @@ -227,14 +229,14 @@ fn unlimited_cast(original: &ColumnKind, target: &ColumnKind) -> Option ColumnKind::Text { length: TextLength::Fixed(_), }, - ) => Some("bpchar".to_owned()), + ) => Some("pg_catalog.bpchar".to_owned()), ( _, ColumnKind::Text { length: TextLength::Varying(_), }, ) => Some("varchar".to_owned()), - (ColumnKind::BitString { .. }, ColumnKind::BitString { .. }) => Some("varbit".to_owned()), + (ColumnKind::BitString { .. }, ColumnKind::BitString { .. }) => Some("pg_catalog.varbit".to_owned()), (ColumnKind::Array { element: original }, ColumnKind::Array { element: target }) => { unlimited_cast(original, target).map(|element| format!("{element}[]")) } @@ -862,8 +864,8 @@ mod tests { let using = |from: &str, to: &str| unlimited_cast(&kind(from), &kind(to)); assert_eq!(using("varchar(255)", "varchar(10)").as_deref(), Some("varchar")); - assert_eq!(using("text", "character(5)").as_deref(), Some("bpchar")); - assert_eq!(using("bit(8)", "bit(4)").as_deref(), Some("varbit")); + assert_eq!(using("text", "character(5)").as_deref(), Some("pg_catalog.bpchar")); + assert_eq!(using("bit(8)", "bit(4)").as_deref(), Some("pg_catalog.varbit")); assert_eq!(using("varchar(20)[]", "varchar(5)[]").as_deref(), Some("varchar[]")); assert_eq!(using("integer", "bit(4)"), None, "an integer has no cast to varbit"); assert_eq!(using("text", "integer"), None); diff --git a/core/crates/drivers/postgres/src/dialect.rs b/core/crates/drivers/postgres/src/dialect.rs index 6f07ebb102..826e3d898b 100644 --- a/core/crates/drivers/postgres/src/dialect.rs +++ b/core/crates/drivers/postgres/src/dialect.rs @@ -755,6 +755,8 @@ mod tests { "ALTER SYSTEM SET primary_conninfo = 'password=x'", "SELECT 1; CREATE USER u PASSWORD 'p'", "DO $$ BEGIN CREATE ROLE app LOGIN PASSWORD 'p'; END $$", + "DO $$ BEGIN EXECUTE 'CREATE ROLE app LOGIN PASSWORD ''p'''; END $$", + "DO $$ BEGIN EXECUTE format('CREATE ROLE %I LOGIN PASSWORD %L', 'app', 'p'); END $$", "DO 'BEGIN CREATE ROLE r PASSWORD ''p''; END'", "CREATE FUNCTION f() RETURNS text AS 'SELECT dblink_connect(''password=x'')' LANGUAGE sql", "SELECT pgp_sym_encrypt('data', 'passphrase')", diff --git a/core/crates/drivers/postgres/src/metadata/mod.rs b/core/crates/drivers/postgres/src/metadata/mod.rs index 4c22918fdb..bb58057803 100644 --- a/core/crates/drivers/postgres/src/metadata/mod.rs +++ b/core/crates/drivers/postgres/src/metadata/mod.rs @@ -179,6 +179,7 @@ mod tests { for (name, sql) in [ ("schemas", objects::SCHEMAS_SQL.to_owned()), ("objects", objects::OBJECTS_SQL.to_owned()), + ("databases", objects::DATABASES_SQL.to_owned()), ("function definitions", definitions::FUNCTIONS_SQL.to_owned()), ("sequence definition", definitions::SEQUENCE_SQL.to_owned()), ("trigger definition", definitions::TRIGGER_SQL.to_owned()), diff --git a/core/crates/drivers/postgres/src/metadata/objects.rs b/core/crates/drivers/postgres/src/metadata/objects.rs index e6980409a8..69e6f11117 100644 --- a/core/crates/drivers/postgres/src/metadata/objects.rs +++ b/core/crates/drivers/postgres/src/metadata/objects.rs @@ -5,16 +5,18 @@ use tokio_postgres::Client; use super::{decode_error, text_rows}; /// Schemas this session can use: no TOAST schemas, and no other -/// session's temporary schema. +/// session's temporary schema. The default is the session's first +/// schema, captured before the catalog read changes its search path. pub(super) const SCHEMAS_SQL: &str = "\ SELECT n.nspname::text, - COALESCE(n.nspname = pg_catalog.current_schema(), false), + COALESCE(n.nspname = pg_catalog.current_setting('tablepro.catalog_default_schema'), false), n.nspname = 'information_schema' OR pg_catalog.starts_with(n.nspname, 'pg_') FROM pg_catalog.pg_namespace n WHERE NOT pg_catalog.starts_with(n.nspname, 'pg_toast') AND NOT pg_catalog.pg_is_other_temp_schema(n.oid) ORDER BY n.nspname"; +/// `default` is the schema `SCHEMAS_SQL` marks as the default. pub(super) const OBJECTS_SQL: &str = "\ SELECT DISTINCT n.nspname::text, o.name, o.kind, o.owner_table FROM ( @@ -34,7 +36,7 @@ FROM ( JOIN pg_catalog.pg_namespace n ON n.oid = o.namespace WHERE NOT pg_catalog.pg_is_other_temp_schema(n.oid) AND CASE $1 - WHEN 'default' THEN n.nspname = pg_catalog.current_schema() + WHEN 'default' THEN n.nspname = pg_catalog.current_setting('tablepro.catalog_default_schema') WHEN 'schema' THEN n.nspname = $2 ELSE n.nspname <> 'information_schema' AND NOT pg_catalog.starts_with(n.nspname, 'pg_') END @@ -58,9 +60,9 @@ pub(crate) async fn list_schemas(client: &Client) -> Result, Dri /// The databases the login may connect to, the connection's own the /// default. `template0` takes no connections; `template1` and `postgres` /// are the server's own. -const DATABASES_SQL: &str = "\ +pub(super) const DATABASES_SQL: &str = "\ SELECT d.datname::text, - d.datname = current_database(), + d.datname = pg_catalog.current_database(), d.datistemplate OR d.datname = 'postgres' FROM pg_catalog.pg_database d WHERE d.datallowconn AND pg_catalog.has_database_privilege(d.datname, 'CONNECT') diff --git a/core/crates/drivers/postgres/src/physical.rs b/core/crates/drivers/postgres/src/physical.rs index 216443a778..5d7b07314d 100644 --- a/core/crates/drivers/postgres/src/physical.rs +++ b/core/crates/drivers/postgres/src/physical.rs @@ -162,7 +162,9 @@ impl PgEndpoint { /// What the server says it is, read once at connect. pub(crate) async fn server_info(client: &Client) -> Result { let messages = client - .simple_query("SELECT current_setting('server_version_num'), current_setting('server_version')") + .simple_query( + "SELECT pg_catalog.current_setting('server_version_num'), pg_catalog.current_setting('server_version')", + ) .await .map_err(|error| map_error(&error))?; let row = messages.iter().find_map(|message| match message { diff --git a/core/crates/drivers/postgres/src/workspace_call.rs b/core/crates/drivers/postgres/src/workspace_call.rs index 274266f90f..84145ba7ad 100644 --- a/core/crates/drivers/postgres/src/workspace_call.rs +++ b/core/crates/drivers/postgres/src/workspace_call.rs @@ -33,6 +33,16 @@ pub(crate) enum Access { ReadWrite, } +/// Searched first, `pg_catalog` wins the tie against a function, operator +/// or type of the same name and argument types in a later schema. The +/// session's own path stays after it, so types and defaults print as the +/// session prints them. +pub(crate) const CATALOG_PATH: &str = "SELECT pg_catalog.set_config('tablepro.catalog_default_schema', \ + COALESCE(pg_catalog.current_schema(), ''), true); \ + SELECT pg_catalog.set_config('search_path', \ + CASE WHEN pg_catalog.current_schema() IS NULL THEN 'pg_catalog, pg_temp' \ + ELSE pg_catalog.concat('pg_catalog, ', pg_catalog.current_setting('search_path'), ', pg_temp') END, true)"; + /// The transaction a workspace call runs in, with its bounds set for /// that transaction only. SET LOCAL ends with the transaction, so the /// bounds reach the server through PgBouncer's transaction pooling and @@ -44,6 +54,7 @@ pub(crate) enum Access { pub(crate) struct Preamble { sql: String, statement_timeout_ms: Option, + dependent_queries: bool, } impl Preamble { @@ -61,9 +72,25 @@ impl Preamble { Self { sql, statement_timeout_ms, + dependent_queries: false, } } + /// The transaction of a catalog read, which also puts `pg_catalog` + /// ahead of the session's search_path until it ends. + pub(crate) fn with_catalog_path(mut self) -> Self { + self.sql.push_str("; "); + self.sql.push_str(CATALOG_PATH); + self + } + + // A relation must resolve on the user's path before metadata can + // use the protected path, so its work cannot be sent in one flight. + pub(crate) fn with_dependent_queries(mut self) -> Self { + self.dependent_queries = true; + self + } + pub(crate) fn sql(&self) -> &str { &self.sql } @@ -115,6 +142,18 @@ pub(crate) fn statement_bound(settings: &WorkspaceSettings, deadline: Option(call: &PgCall, preamble: &Preamble, op: ReadOp) -> Result { let client = call.client()?; let started = Instant::now(); + if preamble.dependent_queries { + let result = match client.batch_execute(preamble.sql()).await { + Ok(()) => op(client).await, + Err(error) => Err(map_error(&error)), + }; + let end = if result.is_ok() { "COMMIT" } else { "ROLLBACK" }; + if let Err(error) = client.batch_execute(end).await { + call.taint(); + return Err(map_error(&error)); + } + return result.map_err(|error| preamble.explain(error, started)); + } let (begun, result, committed) = futures_util::join!( client.batch_execute(preamble.sql()), op(client), @@ -283,6 +322,19 @@ mod tests { assert!(sql(Access::ReadWrite, None, Duration::from_secs(10)).contains("statement_timeout = 0;")); } + #[test] + fn a_catalog_read_searches_pg_catalog_first_and_a_data_read_keeps_the_session_path() { + let data = Preamble::new( + Access::ReadOnly, + Some(Duration::from_millis(1_500)), + Duration::from_secs(10), + ); + let catalog = data.clone().with_catalog_path(); + + assert_eq!(catalog.sql(), format!("{}; {CATALOG_PATH}", data.sql())); + assert!(!data.sql().contains("search_path"), "{}", data.sql()); + } + #[tokio::test(start_paused = true)] async fn a_near_deadline_tightens_the_bound() { let settings = WorkspaceSettings::workspace(false); diff --git a/core/crates/drivers/postgres/src/write_batch.rs b/core/crates/drivers/postgres/src/write_batch.rs index 2a2cb832cf..b8251416fb 100644 --- a/core/crates/drivers/postgres/src/write_batch.rs +++ b/core/crates/drivers/postgres/src/write_batch.rs @@ -187,9 +187,9 @@ async fn count(client: &Client, probe: &Statement) -> Result { /// lost can be asked about afterwards. async fn transaction_id(client: &Client, legacy: bool) -> Result { let sql = if legacy { - "SELECT txid_current()::text" + "SELECT pg_catalog.txid_current()::pg_catalog.text" } else { - "SELECT pg_current_xact_id()::text" + "SELECT pg_catalog.pg_current_xact_id()::pg_catalog.text" }; let messages = client.simple_query(sql).await.map_err(|error| map_error(&error))?; let xid = messages diff --git a/core/crates/drivers/postgres/tests/integration/metadata.rs b/core/crates/drivers/postgres/tests/integration/metadata.rs index bd09223cd2..15f4a5eb42 100644 --- a/core/crates/drivers/postgres/tests/integration/metadata.rs +++ b/core/crates/drivers/postgres/tests/integration/metadata.rs @@ -10,10 +10,10 @@ use tablepro_core::meta::{ EditorRefusal, EngineRowId, IndexKey, ObjectInfo, ObjectKind, ObjectScope, RowCountEstimate, RowIdentity, TableRef, }; use tablepro_core::save::{ChangeSet, RowChange, RowUpdate, plan_save}; -use tablepro_core::{DriverError, Value}; +use tablepro_core::{Connection, DriverError, Value, WriteKind}; use tablepro_test_fixtures::{HbaMode, PostgresFixture, PostgresVersion}; -use crate::support::{CALL, connect, rows, run_ok, scalar, session, setup, start, text, within}; +use crate::support::{CALL, apply, connect, rows, run_ok, scalar, session, setup, start, text, within}; fn table(name: &str) -> TableRef { TableRef::new(Some("public".to_owned()), name) @@ -1013,6 +1013,14 @@ async fn alter_add_column_then_query_bound_on_all_four_pooled_connections() { } } +/// A function in `public` that fails the statement that runs it. +fn trap(name: &str, arguments: &str, returns: &str) -> String { + format!( + "CREATE FUNCTION public.{name}({arguments}) RETURNS {returns} LANGUAGE plpgsql AS \ + $$ BEGIN RAISE EXCEPTION 'public.{name} ran as %', current_user; END $$" + ) +} + /// Another user's functions and operators in `public` whose signatures /// match the catalog reads' arguments better than the built-ins do. A /// catalog read never runs one, and a table named without its schema is @@ -1022,12 +1030,6 @@ async fn alter_add_column_then_query_bound_on_all_four_pooled_connections() { async fn lookalikes_in_public_never_run_in_a_catalog_read() { let fixture = start().await; let connection = connect(&fixture, false).await; - let trap = |name: &str, arguments: &str, returns: &str| { - format!( - "CREATE FUNCTION public.{name}({arguments}) RETURNS {returns} LANGUAGE plpgsql AS \ - $$ BEGIN RAISE EXCEPTION 'public.{name} ran as %', current_user; END $$" - ) - }; let statements = [ "CREATE TYPE mood AS ENUM ('sad', 'ok')".to_owned(), "CREATE TABLE accounts (id int PRIMARY KEY, state mood)".to_owned(), @@ -1091,3 +1093,242 @@ async fn lookalikes_in_public_never_run_in_a_catalog_read() { .is_err() ); } + +/// Exact copies, in `public`, of built-ins the driver's statements use, +/// each failing the statement that runs it, and a login whose sessions +/// from now on search `public` ahead of `pg_catalog`. +fn exact_lookalikes() -> Vec { + let mut statements = Vec::new(); + for (function, operand) in [ + ("oid_equals", "oid"), + ("name_equals", "name"), + ("char_equals", "\"char\""), + ("int2_equals", "int2"), + ("text_equals", "text"), + ] { + statements.push(trap(function, &format!("{operand}, {operand}"), "boolean")); + statements.push(format!( + "CREATE OPERATOR public.= (LEFTARG = {operand}, RIGHTARG = {operand}, FUNCTION = public.{function})" + )); + } + statements.extend([ + trap("current_database", "", "name"), + trap("current_setting", "text", "text"), + trap("pg_current_xact_id", "", "xid8"), + trap("text_check", "text", "boolean"), + "CREATE DOMAIN public.text AS pg_catalog.text CHECK (public.text_check(VALUE))".to_owned(), + trap("oid_check", "oid", "boolean"), + "CREATE DOMAIN public.oid AS pg_catalog.oid CHECK (public.oid_check(VALUE))".to_owned(), + "ALTER ROLE CURRENT_USER SET search_path = public, pg_catalog".to_owned(), + ]); + statements +} + +/// Every catalog read of one table. +async fn read_table_catalog(connection: &dyn Connection, target: &TableRef) { + connection.fetch_columns(target, within(CALL)).await.expect("columns"); + connection.fetch_indexes(target, within(CALL)).await.expect("indexes"); + connection + .fetch_foreign_keys(target, within(CALL)) + .await + .expect("foreign keys"); + connection + .fetch_outbound_foreign_keys(target, within(CALL)) + .await + .expect("outbound foreign keys"); + connection + .fetch_inbound_foreign_keys(target, within(CALL)) + .await + .expect("inbound foreign keys"); + connection + .fetch_table_definition(target, within(CALL)) + .await + .expect("table definition"); + connection + .row_identity(target, within(CALL)) + .await + .expect("row identity"); + connection + .table_estimate(target, within(CALL)) + .await + .expect("row estimate"); +} + +/// The login's own search_path puts exact copies of built-ins ahead of +/// `pg_catalog`. The connect, every catalog read and a save never run +/// one, and names still print as the session prints them. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "requires docker"] +async fn exact_lookalikes_ahead_of_pg_catalog_never_run() { + let fixture = start().await; + let first = connect(&fixture, false).await; + setup( + first.as_ref(), + &[ + "CREATE TYPE mood AS ENUM ('sad', 'ok')", + "CREATE TABLE accounts (id serial PRIMARY KEY, state mood)", + "ALTER TABLE accounts ADD COLUMN next_id int8 DEFAULT 1 + nextval('accounts_id_seq')", + "CREATE TABLE orders (id int PRIMARY KEY, account int REFERENCES accounts (id))", + "CREATE INDEX orders_account ON orders (account)", + "CREATE FUNCTION stamp() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN RETURN NEW; END $$", + "CREATE TRIGGER audit BEFORE INSERT ON orders FOR EACH ROW EXECUTE FUNCTION stamp()", + ], + ) + .await; + let lookalikes = exact_lookalikes(); + let lookalikes: Vec<&str> = lookalikes.iter().map(String::as_str).collect(); + setup(first.as_ref(), &lookalikes).await; + first.close().await.unwrap(); + + let connection = connect(&fixture, false).await; + let schemas = connection.list_schemas(within(CALL)).await.unwrap(); + assert!( + schemas + .iter() + .any(|schema| schema.name == "public" && schema.is_default), + "{schemas:?}" + ); + let databases = connection.list_databases(within(CALL)).await.unwrap(); + assert!( + databases + .iter() + .any(|database| database.name == fixture.database() && database.is_default), + "{databases:?}" + ); + let objects = connection + .list_objects(&ObjectScope::DefaultSchema, within(CALL)) + .await + .unwrap(); + connection + .list_objects(&ObjectScope::AllSchemas, within(CALL)) + .await + .unwrap(); + for name in ["accounts_id_seq", "stamp", "audit"] { + let object = objects + .iter() + .find(|object| object.name == name) + .expect("a listed object"); + let definition = connection.fetch_object_definition(object, within(CALL)).await.unwrap(); + assert!(definition.is_some(), "{name}"); + } + for name in ["orders", "accounts"] { + read_table_catalog(connection.as_ref(), &table(name)).await; + read_table_catalog(connection.as_ref(), &TableRef::new(None, name)).await; + } + + let columns = connection + .fetch_columns(&TableRef::new(None, "accounts"), within(CALL)) + .await + .unwrap(); + let column = |name: &str| columns.iter().find(|column| column.name == name).expect("a column"); + let state = &column("state").column_type; + assert_eq!(state.name, "mood"); + assert_eq!( + state.catalog.as_ref().map(|catalog| catalog.cast_target.as_sql()), + Some("mood") + ); + // A serial's own default reads as auto increment, so the sequence's + // name shows in another column's default. + let next_id = &column("next_id").default; + assert!( + matches!(next_id, ColumnDefault::Expression(expression) + if expression.as_sql().contains("nextval('accounts_id_seq'::regclass)")), + "{next_id:?}" + ); + + let saved = apply( + connection.as_ref(), + WriteKind::RowChanges, + &["INSERT INTO accounts (state) VALUES ('ok')"], + ) + .await + .unwrap(); + assert_eq!(saved.rows_affected, [Some(1)]); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "requires docker"] +async fn catalog_defaults_keep_the_sessions_schema_with_an_explicit_or_empty_path() { + let fixture = start().await; + let initial = connect(&fixture, false).await; + setup(initial.as_ref(), &["CREATE TABLE public.accounts (id int PRIMARY KEY)"]).await; + initial.close().await.unwrap(); + + for (setting, expected_default) in [ + ("pg_catalog, public", Some("pg_catalog")), + ("pg_catalog", Some("pg_catalog")), + ("''", None), + ("' '", None), + ] { + let admin = connect(&fixture, false).await; + setup( + admin.as_ref(), + &[&format!("ALTER ROLE CURRENT_USER SET search_path = {setting}")], + ) + .await; + admin.close().await.unwrap(); + + let connection = connect(&fixture, false).await; + let schemas = connection.list_schemas(within(CALL)).await.unwrap(); + let defaults: Vec<&str> = schemas + .iter() + .filter(|schema| schema.is_default) + .map(|schema| schema.name.as_str()) + .collect(); + assert_eq!(defaults, expected_default.into_iter().collect::>(), "{setting}"); + let objects = connection + .list_objects(&ObjectScope::DefaultSchema, within(CALL)) + .await + .unwrap(); + match expected_default { + Some(schema) => { + assert!(!objects.is_empty(), "{setting}"); + assert!(objects.iter().all(|object| object.schema.as_deref() == Some(schema))); + } + None => assert!(objects.is_empty(), "{setting}"), + } + let columns = connection + .fetch_columns(&table("accounts"), within(CALL)) + .await + .unwrap(); + assert_eq!(columns.len(), 1, "{setting}"); + assert_eq!(columns[0].name, "id", "{setting}"); + connection.close().await.unwrap(); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "requires docker"] +async fn an_unqualified_user_table_keeps_its_identity_when_it_shares_a_catalog_name() { + let fixture = start().await; + let initial = connect(&fixture, false).await; + setup( + initial.as_ref(), + &[ + "CREATE TABLE public.pg_class (id int PRIMARY KEY, user_value text)", + "ALTER ROLE CURRENT_USER SET search_path = public, pg_catalog", + ], + ) + .await; + initial.close().await.unwrap(); + + let connection = connect(&fixture, false).await; + let target = TableRef::new(None, "pg_class"); + let columns = connection.fetch_columns(&target, within(CALL)).await.unwrap(); + let names: Vec<&str> = columns.iter().map(|column| column.name.as_str()).collect(); + assert_eq!(names, ["id", "user_value"]); + read_table_catalog(connection.as_ref(), &target).await; + let indexes = connection.fetch_indexes(&target, within(CALL)).await.unwrap(); + assert!(indexes.iter().any(|index| index.name == "pg_class_pkey")); + + for _ in 0..5 { + assert!( + connection + .fetch_columns(&TableRef::new(None, "absent_table"), within(CALL)) + .await + .is_err() + ); + let columns = connection.fetch_columns(&target, within(CALL)).await.unwrap(); + assert_eq!(columns.len(), 2); + } +} diff --git a/core/crates/drivers/sqlite/src/connection.rs b/core/crates/drivers/sqlite/src/connection.rs index b79ab83401..a0b6814523 100644 --- a/core/crates/drivers/sqlite/src/connection.rs +++ b/core/crates/drivers/sqlite/src/connection.rs @@ -246,6 +246,7 @@ async fn read_user_sql( sql: &str, sink: &mut dyn ResultSink, ) -> Result { + crate::result_stream::refuse_nul(sql)?; crate::read_guard::refuse_more_than_one(connection, sql).await?; // The count took the busy handler off. flag.install(connection, lock_timeout).await?; diff --git a/core/crates/drivers/sqlite/src/result_stream.rs b/core/crates/drivers/sqlite/src/result_stream.rs index 609c89b9a4..5dccc89731 100644 --- a/core/crates/drivers/sqlite/src/result_stream.rs +++ b/core/crates/drivers/sqlite/src/result_stream.rs @@ -88,6 +88,7 @@ pub(crate) async fn run_script( sink: &mut dyn ResultSink, halt: impl Fn() -> bool, ) -> Result { + refuse_nul(sql)?; let mut outcome = StreamOutcome::default(); for statement in split_statements(sql) { if halt() { @@ -113,6 +114,18 @@ pub(crate) async fn run_script( Ok(outcome) } +/// SQLite takes a NUL as the end of the text, and sqlx then prepares +/// what follows it again and again without moving on, so such text is +/// refused before sqlx sees it. +pub(crate) fn refuse_nul(sql: &str) -> Result<(), DriverError> { + match sql.contains('\0') { + true => Err(DriverError::Protocol( + "the statement holds a NUL character, where SQLite stops reading".to_owned(), + )), + false => Ok(()), + } +} + /// Run a read the app built, every row of it. pub(crate) async fn run_bound( connection: &mut SqlxConnection, diff --git a/core/crates/drivers/sqlite/tests/integration/sessions.rs b/core/crates/drivers/sqlite/tests/integration/sessions.rs index 4a54e8a165..7c38f68789 100644 --- a/core/crates/drivers/sqlite/tests/integration/sessions.rs +++ b/core/crates/drivers/sqlite/tests/integration/sessions.rs @@ -458,11 +458,18 @@ async fn a_read_attaches_nothing_sets_no_pragma_and_runs_one_statement() { let attach = format!("ATTACH '{}' AS other", file.path.display()); let vacuum = format!("VACUUM INTO '{copy_name}'"); let hidden = format!("SELECT $1('); VACUUM INTO '{copy_name}'; --'"); + let other = TestFile::new().await; + let pages = format!( + "SELECT $1(') AS pgno, 2 AS data WHERE 0; ATTACH DATABASE '{}' AS other; \ + SELECT pgno, hex(data) FROM sqlite_dbpage('other'); --'", + other.path.display() + ); for sql in [ vacuum.as_str(), attach.as_str(), hidden.as_str(), + pages.as_str(), "PRAGMA foreign_keys = OFF", "EXPLAIN PRAGMA hard_heap_limit = 1", "PRAGMA user_version = 7", @@ -486,3 +493,37 @@ async fn a_read_attaches_nothing_sets_no_pragma_and_runs_one_statement() { } connection.close().await.unwrap(); } + +/// SQLite takes a NUL as the end of the text, and sqlx then prepares what +/// follows it again and again without moving on. +#[tokio::test] +async fn a_statement_holding_a_nul_is_refused_at_once() { + let file = TestFile::new().await; + let connection = file.open().await; + let session = connection + .open_editor_session(EditorSessionOptions::default(), CancellationToken::new()) + .await + .unwrap(); + let collected = Collected::default(); + + let read = tokio::time::timeout( + Duration::from_secs(5), + connection.stream_read( + &ReadStatement::User(UserSql::new("SELECT 1\u{0}")), + collected.sink(), + call(), + ), + ) + .await + .expect("the read came back"); + let (summary, ran) = tokio::time::timeout(Duration::from_secs(5), run(session.as_ref(), "SELECT 1\u{0}; SELECT 2")) + .await + .expect("the run came back"); + + assert!(read.is_err(), "{read:?}"); + assert!(collected.rows().is_empty()); + assert!(matches!(summary.end, RunEnd::Failed(_)), "{:?}", summary.end); + assert!(ran.rows().is_empty(), "a statement before the NUL ran"); + session.close().await; + connection.close().await.unwrap(); +} diff --git a/core/crates/storage/Cargo.toml b/core/crates/storage/Cargo.toml index 5fbdb4fd67..4774fc2ee4 100644 --- a/core/crates/storage/Cargo.toml +++ b/core/crates/storage/Cargo.toml @@ -12,6 +12,7 @@ secrecy.workspace = true serde.workspace = true serde_json.workspace = true thiserror.workspace = true +unicode-normalization.workspace = true tokio = { workspace = true, features = ["fs"] } uuid.workspace = true sqlx = { workspace = true, features = ["migrate", "macros"] } diff --git a/core/crates/storage/src/connection_group.rs b/core/crates/storage/src/connection_group.rs index 0f8d1ea3e3..f747147f09 100644 --- a/core/crates/storage/src/connection_group.rs +++ b/core/crates/storage/src/connection_group.rs @@ -1,12 +1,14 @@ -use serde::{Deserialize, Serialize}; +use std::collections::{HashMap, HashSet}; + +use serde::{Deserialize, Deserializer, Serialize}; use uuid::Uuid; use crate::{ConnectionColor, ConnectionGroupName, GroupIcon}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "StoredGroup")] pub struct ConnectionGroup { pub id: Uuid, - #[serde(deserialize_with = "crate::connection_group_name::deserialize_required")] pub name: ConnectionGroupName, #[serde(default, skip_serializing_if = "Option::is_none")] pub parent_id: Option, @@ -19,6 +21,66 @@ pub struct ConnectionGroup { pub icon: Option, } +#[derive(Deserialize)] +struct StoredGroup { + id: Uuid, + name: String, + #[serde(default)] + parent_id: Option, + #[serde(default)] + order: u32, + #[serde(default)] + color: Option, + #[serde(default)] + icon: Option, +} + +impl TryFrom for ConnectionGroup { + type Error = crate::GroupNameError; + + fn try_from(stored: StoredGroup) -> Result { + Ok(Self { + id: stored.id, + name: ConnectionGroupName::from_stored(&stored.name).ok_or(crate::GroupNameError::Empty)?, + parent_id: stored.parent_id, + order: stored.order, + color: stored.color, + icon: stored.icon, + }) + } +} + +pub(crate) fn deserialize_list<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let stored = Vec::::deserialize(deserializer)?; + let mut recovered = Vec::new(); + let mut held: HashMap, HashSet> = HashMap::new(); + let mut groups = Vec::with_capacity(stored.len()); + for (index, raw) in stored.into_iter().enumerate() { + let recover = crate::name_rules::shown(&raw.name).is_empty(); + let group = ConnectionGroup::try_from(raw).map_err(serde::de::Error::custom)?; + if recover { + recovered.push(index); + } else { + held.entry(group.parent_id) + .or_default() + .insert(group.name.as_str().to_lowercase()); + } + groups.push(group); + } + // A recovered sibling must not match a visible group during import. + recovered.sort_unstable_by_key(|&index| groups[index].id); + for index in recovered { + let group = &mut groups[index]; + let name = + crate::name_rules::claim_recovered(group.name.as_str(), None, held.entry(group.parent_id).or_default()); + group.name = ConnectionGroupName::new(&name).map_err(serde::de::Error::custom)?; + } + Ok(groups) +} + impl ConnectionGroup { /// The macOS limit, so a tree made on either fits both. pub const MAX_DEPTH: usize = 3; diff --git a/core/crates/storage/src/connection_group_name.rs b/core/crates/storage/src/connection_group_name.rs index 709948d1c5..11b26dcc33 100644 --- a/core/crates/storage/src/connection_group_name.rs +++ b/core/crates/storage/src/connection_group_name.rs @@ -1,10 +1,10 @@ -use serde::{Deserialize, Deserializer, Serialize}; +use serde::{Deserialize, Serialize}; use crate::group_name_error::GroupNameError; use crate::name_rules::{self, NameFault}; -/// The group a saved connection is filed under, as the user typed it -/// without the spaces around it. +/// The group a saved connection is filed under, its name in the form a +/// connection's name takes. /// /// A name read from disk passes the same check as one from the dialog, /// after `from_stored` cleans it, so a name a menu or a section title @@ -17,7 +17,7 @@ impl ConnectionGroupName { // No length limit: a macOS group name has none and has to read back unchanged. pub fn new(value: &str) -> Result { match name_rules::checked(value) { - Ok(name) => Ok(Self(name.to_owned())), + Ok(name) => Ok(Self(name)), Err(NameFault::Empty) => Err(GroupNameError::Empty), Err(NameFault::ControlCharacter) => Err(GroupNameError::ControlCharacter), } @@ -38,16 +38,6 @@ impl ConnectionGroupName { } } -/// Reads a group's `name` through `from_stored`. A blank one is refused: -/// a group nobody can see cannot be told apart. -pub(crate) fn deserialize_required<'de, D>(deserializer: D) -> Result -where - D: Deserializer<'de>, -{ - let stored = String::deserialize(deserializer)?; - ConnectionGroupName::from_stored(&stored).ok_or_else(|| serde::de::Error::custom(GroupNameError::Empty)) -} - impl TryFrom for ConnectionGroupName { type Error = GroupNameError; @@ -76,6 +66,7 @@ mod tests { fn group_name_rejects_blank_and_control_characters() { assert_eq!(ConnectionGroupName::new(""), Err(GroupNameError::Empty)); assert_eq!(ConnectionGroupName::new(" \t "), Err(GroupNameError::Empty)); + assert_eq!(ConnectionGroupName::new("\u{200b}"), Err(GroupNameError::Empty)); assert_eq!( ConnectionGroupName::new("Work\nHome"), Err(GroupNameError::ControlCharacter) @@ -100,8 +91,16 @@ mod tests { ConnectionGroupName::from_stored("Work\tHome\n").map(String::from), Some(String::from("Work Home")) ); + assert_eq!( + ConnectionGroupName::from_stored("Wo\u{200b}rk\u{feff}").map(String::from), + Some(String::from("Work")) + ); assert_eq!(ConnectionGroupName::from_stored(" \t "), None); assert_eq!(ConnectionGroupName::from_stored(""), None); + assert_eq!( + ConnectionGroupName::from_stored("\u{200b}").map(String::from), + Some(String::from("\\u{200b}")) + ); } #[test] @@ -111,4 +110,12 @@ mod tests { assert_eq!(name.as_str(), "Prod"); assert_eq!(String::from(name), "Prod"); } + + #[test] + fn names_that_read_alike_are_the_same_group() { + let work = ConnectionGroupName::new("Work").expect("a name"); + + assert!(work.same_name(&ConnectionGroupName::new("wo\u{200b}rk").expect("a name"))); + assert!(!work.same_name(&ConnectionGroupName::new("Home").expect("a name"))); + } } diff --git a/core/crates/storage/src/connection_tag.rs b/core/crates/storage/src/connection_tag.rs index f91f9dee4a..99382a1c01 100644 --- a/core/crates/storage/src/connection_tag.rs +++ b/core/crates/storage/src/connection_tag.rs @@ -1,4 +1,6 @@ -use serde::{Deserialize, Serialize}; +use std::collections::HashSet; + +use serde::{Deserialize, Deserializer, Serialize}; use uuid::Uuid; use crate::{ConnectionColor, ConnectionTagName}; @@ -6,13 +8,60 @@ use crate::{ConnectionColor, ConnectionTagName}; /// A label several connections can share. Connections name it by id, so /// a rename or a new colour reaches every one of them. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "StoredTag")] pub struct ConnectionTag { pub id: Uuid, - #[serde(deserialize_with = "crate::connection_tag_name::deserialize_stored")] pub name: ConnectionTagName, pub color: ConnectionColor, } +#[derive(Deserialize)] +struct StoredTag { + id: Uuid, + name: String, + color: ConnectionColor, +} + +impl TryFrom for ConnectionTag { + type Error = crate::TagNameError; + + fn try_from(stored: StoredTag) -> Result { + Ok(Self { + id: stored.id, + name: ConnectionTagName::from_stored(&stored.name, stored.id)?, + color: stored.color, + }) + } +} + +pub(crate) fn deserialize_list<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let stored = Vec::::deserialize(deserializer)?; + let mut recovered = Vec::new(); + let mut held = HashSet::new(); + let mut tags = Vec::with_capacity(stored.len()); + for (index, raw) in stored.into_iter().enumerate() { + let recover = crate::name_rules::shown(&raw.name).is_empty(); + let tag = ConnectionTag::try_from(raw).map_err(serde::de::Error::custom)?; + if recover { + recovered.push(index); + } else { + held.insert(tag.name.key()); + } + tags.push(tag); + } + // Reserve visible names before any recovery, regardless of file order. + recovered.sort_unstable_by_key(|&index| tags[index].id); + for index in recovered { + let tag = &mut tags[index]; + let name = crate::name_rules::claim_recovered(tag.name.as_str(), Some(ConnectionTagName::MAX_CHARS), &mut held); + tag.name = ConnectionTagName::new(&name).map_err(serde::de::Error::custom)?; + } + Ok(tags) +} + impl ConnectionTag { pub fn new(name: ConnectionTagName, color: ConnectionColor) -> Self { Self { @@ -45,4 +94,80 @@ mod tests { assert!(serde_json::from_str::(json).is_err()); } + + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn a_stored_tag_with_a_truly_blank_name_is_still_refused() { + let json = serde_json::json!({ + "id": Uuid::new_v4(), + "name": " \t\n ", + "color": "teal", + }); + assert!(serde_json::from_value::(json).is_err()); + } + + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn long_invisible_tag_names_recover_by_id_and_round_trip() { + let first = Uuid::new_v4(); + let second = Uuid::new_v4(); + for (id, name) in [ + (first, "\u{200b}".repeat(32)), + (second, format!("{}\u{feff}", "\u{200b}".repeat(31))), + ] { + let tag: ConnectionTag = serde_json::from_value(serde_json::json!({ + "id": id, + "name": name, + "color": "teal", + })) + .expect("an older tag"); + assert_eq!(tag.id, id); + assert_eq!(tag.name.as_str(), id.simple().to_string()); + let written = serde_json::to_value(&tag).expect("serialize"); + assert_eq!(written["id"], id.to_string()); + assert_eq!(written["name"], id.simple().to_string()); + assert_eq!(written["color"], "teal"); + assert_eq!(serde_json::from_value::(written).expect("reread"), tag); + } + } + + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn a_recovered_tag_id_does_not_take_a_visible_name_or_exceed_the_limit() { + #[derive(Deserialize)] + struct List { + #[serde(deserialize_with = "deserialize_list")] + tags: Vec, + } + + let id = Uuid::new_v4(); + let plain = id.simple().to_string(); + let second = format!("{} (2)", &plain[..28]); + let third = format!("{} (3)", &plain[..28]); + for reverse in [false, true] { + let mut tags = vec![ + serde_json::json!({"id": id, "name": "\u{200b}".repeat(32), "color": "red"}), + serde_json::json!({"id": Uuid::new_v4(), "name": plain, "color": "blue"}), + serde_json::json!({"id": Uuid::new_v4(), "name": second, "color": "teal"}), + ]; + if reverse { + tags.reverse(); + } + let list: List = serde_json::from_value(serde_json::json!({"tags": tags})).expect("recover"); + let recovered = list.tags.iter().find(|tag| tag.id == id).expect("tag kept"); + assert_eq!(recovered.name.as_str(), third); + assert_eq!(recovered.name.as_str().chars().count(), ConnectionTagName::MAX_CHARS); + assert!(list.tags.iter().any(|tag| tag.name.as_str() == plain)); + assert!(list.tags.iter().any(|tag| tag.name.as_str() == second)); + } + } } diff --git a/core/crates/storage/src/connection_tag_name.rs b/core/crates/storage/src/connection_tag_name.rs index 307ec4f091..c996228728 100644 --- a/core/crates/storage/src/connection_tag_name.rs +++ b/core/crates/storage/src/connection_tag_name.rs @@ -1,9 +1,10 @@ -use serde::{Deserialize, Deserializer, Serialize}; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; use crate::name_rules::{self, NameFault}; use crate::tag_name_error::TagNameError; -/// A tag's name as the user typed it, without the spaces around it. +/// A tag's name as the list shows it, the form a connection's name takes. #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] #[serde(try_from = "String", into = "String")] pub struct ConnectionTagName(String); @@ -15,7 +16,7 @@ impl ConnectionTagName { pub fn new(value: &str) -> Result { match name_rules::checked(value) { Ok(name) if name.chars().count() > Self::MAX_CHARS => Err(TagNameError::TooLong { max: Self::MAX_CHARS }), - Ok(name) => Ok(Self(name.to_owned())), + Ok(name) => Ok(Self(name)), Err(NameFault::Empty) => Err(TagNameError::Empty), Err(NameFault::ControlCharacter) => Err(TagNameError::ControlCharacter), } @@ -27,26 +28,23 @@ impl ConnectionTagName { // Two tags whose names differ only in case would read as one. pub fn same_name(&self, other: &str) -> bool { - self.key() == other.trim().to_lowercase() + self.key() == name_rules::shown(other).to_lowercase() } /// The name as two tags are told apart by. pub(crate) fn key(&self) -> String { self.0.to_lowercase() } -} -// A blank name is refused: a tag nobody can see cannot be told apart. -pub(crate) fn deserialize_stored<'de, D>(deserializer: D) -> Result -where - D: Deserializer<'de>, -{ - let stored = String::deserialize(deserializer)?; - let cut: String = name_rules::cleaned(&stored) - .chars() - .take(ConnectionTagName::MAX_CHARS) - .collect(); - ConnectionTagName::new(&cut).map_err(serde::de::Error::custom) + pub(crate) fn from_stored(value: &str, id: Uuid) -> Result { + let cleaned = name_rules::cleaned(value); + // Cutting a recovered name's escapes would make distinct old + // tags look alike. Its full id fits the limit and stays stable. + if name_rules::shown(value).is_empty() && cleaned.chars().count() > Self::MAX_CHARS { + return Self::new(&id.simple().to_string()); + } + Self::new(&cleaned.chars().take(Self::MAX_CHARS).collect::()) + } } impl TryFrom for ConnectionTagName { @@ -80,6 +78,7 @@ mod tests { Ok("prod".to_owned()) ); assert_eq!(ConnectionTagName::new(" "), Err(TagNameError::Empty)); + assert_eq!(ConnectionTagName::new("\u{200b}"), Err(TagNameError::Empty)); assert_eq!( ConnectionTagName::new(&"t".repeat(ConnectionTagName::MAX_CHARS + 1)), Err(TagNameError::TooLong { @@ -87,6 +86,10 @@ mod tests { }) ); assert_eq!(ConnectionTagName::new("a\u{7}"), Err(TagNameError::ControlCharacter)); + assert_eq!( + ConnectionTagName::new("prod\u{200b}").map(String::from), + Ok("prod".to_owned()) + ); } #[test] @@ -102,24 +105,23 @@ mod tests { assert!(name.same_name("production")); assert!(name.same_name(" PRODUCTION ")); + assert!(name.same_name("produc\u{200b}tion")); assert!(!name.same_name("prod")); } - #[derive(Debug, Deserialize)] - struct Stored { - #[serde(deserialize_with = "deserialize_stored")] - name: ConnectionTagName, - } - #[test] fn a_stored_name_is_cleaned_and_a_blank_one_refused() { + let id = Uuid::new_v4(); let long = "x".repeat(ConnectionTagName::MAX_CHARS + 5); - let read: Stored = serde_json::from_str(&format!(r#"{{"name":"{long}"}}"#)).expect("a long name"); - assert_eq!(read.name.as_str(), "x".repeat(ConnectionTagName::MAX_CHARS)); + let read = ConnectionTagName::from_stored(&long, id).expect("a long name"); + assert_eq!(read.as_str(), "x".repeat(ConnectionTagName::MAX_CHARS)); + + let read = ConnectionTagName::from_stored("a\tb", id).expect("a control character"); + assert_eq!(read.as_str(), "a b"); - let read: Stored = serde_json::from_str(r#"{"name":"a\tb"}"#).expect("a control character"); - assert_eq!(read.name.as_str(), "a b"); + let read = ConnectionTagName::from_stored("production\u{200b}", id).expect("a blank"); + assert_eq!(read.as_str(), "production"); - assert!(serde_json::from_str::(r#"{"name":" \n "}"#).is_err()); + assert_eq!(ConnectionTagName::from_stored(" \n ", id), Err(TagNameError::Empty)); } } diff --git a/core/crates/storage/src/connections/legacy_groups.rs b/core/crates/storage/src/connections/legacy_groups.rs index e20d27d52e..80baeb657e 100644 --- a/core/crates/storage/src/connections/legacy_groups.rs +++ b/core/crates/storage/src/connections/legacy_groups.rs @@ -1,5 +1,5 @@ -use std::collections::HashMap; use std::collections::hash_map::Entry; +use std::collections::{BTreeSet, HashMap, HashSet}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -44,6 +44,7 @@ struct FiledById { /// listed them in. pub(super) fn group_names_to_groups(document: Value) -> Result { let list: FiledByNameList = serde_json::from_value(document).map_err(|error| error.to_string())?; + let recovered = recovered_group_names(&list.connections)?; let mut groups: Vec = Vec::new(); let mut named: HashMap = HashMap::new(); let mut connections: Vec = Vec::with_capacity(list.connections.len()); @@ -51,7 +52,14 @@ pub(super) fn group_names_to_groups(document: Value) -> Result { let group_id = connection .group .as_deref() - .and_then(ConnectionGroupName::from_stored) + .and_then(|raw| { + let name = ConnectionGroupName::from_stored(raw)?; + if crate::name_rules::shown(raw).is_empty() { + recovered.get(name.as_str()).cloned() + } else { + Some(name) + } + }) .map(|name| match named.entry(name.as_str().to_owned()) { Entry::Occupied(held) => *held.get(), Entry::Vacant(free) => { @@ -79,6 +87,32 @@ pub(super) fn group_names_to_groups(document: Value) -> Result { .map_err(|error| error.to_string()) } +// Reserve visible names before recovery, so an old invisible name cannot +// merge its connections into a group named with literal escapes. +fn recovered_group_names(connections: &[FiledByName]) -> Result, String> { + let mut held = HashSet::new(); + let mut recover = BTreeSet::new(); + for raw in connections.iter().filter_map(|connection| connection.group.as_deref()) { + let Some(name) = ConnectionGroupName::from_stored(raw) else { + continue; + }; + if crate::name_rules::shown(raw).is_empty() { + recover.insert(name.as_str().to_owned()); + } else { + held.insert(name.as_str().to_lowercase()); + } + } + recover + .into_iter() + .map(|old| { + let name = crate::name_rules::claim_recovered(&old, None, &mut held); + ConnectionGroupName::new(&name) + .map(|name| (old, name)) + .map_err(|error| error.to_string()) + }) + .collect() +} + fn number_connections(connections: &mut [FiledById]) { let mut by_group: HashMap, Vec> = HashMap::new(); for (index, connection) in connections.iter().enumerate() { diff --git a/core/crates/storage/src/connections/mod.rs b/core/crates/storage/src/connections/mod.rs index e58b4cad8c..639c0dab82 100644 --- a/core/crates/storage/src/connections/mod.rs +++ b/core/crates/storage/src/connections/mod.rs @@ -196,9 +196,17 @@ pub enum SavedSshAuth { #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] struct ConnectionsDocument { connections: Vec, - #[serde(default, skip_serializing_if = "Vec::is_empty")] + #[serde( + default, + skip_serializing_if = "Vec::is_empty", + deserialize_with = "crate::connection_group::deserialize_list" + )] groups: Vec, - #[serde(default, skip_serializing_if = "Vec::is_empty")] + #[serde( + default, + skip_serializing_if = "Vec::is_empty", + deserialize_with = "crate::connection_tag::deserialize_list" + )] tags: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] favorites: Vec, @@ -438,14 +446,17 @@ pub struct ImportEntry { pub change: ImportChange, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum ImportChange { /// A connection the list did not have, including one whose id was /// taken by a different connection and was given its own. Added, /// A connection already in the list that the file only renamed, /// recoloured, regrouped, retagged or gave another icon. - Updated, + Updated { + /// The name it had, when the import gives it another. + renamed_from: Option, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -711,7 +722,8 @@ impl ConnectionStore { /// when the import began, whatever order the file lists them in, and /// an added one never takes the file's last-opened time, so a file /// cannot pass one host off as another saved connection or sort it - /// above one. + /// above one. An updated entry whose name the file keeps keeps it, + /// even when another entry has the same one. /// /// Tags are matched by id first and then by name, so importing a /// file twice, or a file from a machine whose `production` tag has @@ -947,22 +959,29 @@ fn merge_connection( ) -> ImportEntry { match list.iter().position(|saved| saved.id == connection.id) { Some(index) if list[index].connects_like(&connection) => { - let name = unclaimed_name(list, held, &connection.name, Some(connection.id)); + // Two connections saved without a name to one server share + // one, and re-importing their export must rename neither. + let name = if connection.name == list[index].name { + connection.name + } else { + unclaimed_name(list, held, &connection.name, Some(connection.id)) + }; let order = if list[index].group_id == connection.group_id { list[index].order } else { placement::next_order(list, connection.group_id) }; let existing = &mut list[index]; - existing.name = name; + let before = std::mem::replace(&mut existing.name, name); existing.color = connection.color; existing.group_id = connection.group_id; existing.order = order; existing.tags = connection.tags; existing.icon = connection.icon; + let renamed_from = (existing.name != before).then_some(before); ImportEntry { connection: existing.clone(), - change: ImportChange::Updated, + change: ImportChange::Updated { renamed_from }, } } Some(_) => { @@ -1128,6 +1147,206 @@ mod tests { assert_eq!(list[0], connection); } + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn older_invisible_group_and_tag_names_keep_the_library_and_its_links() { + let root = TempDir::new().expect("tempdir"); + let group = Uuid::new_v4(); + let tags = [Uuid::new_v4(), Uuid::new_v4(), Uuid::new_v4()]; + let connection = SavedConnection { + group_id: Some(group), + tags: tags.to_vec(), + ..sample_connection() + }; + let document = serde_json::json!({ + "version": 3, + "connections": [connection], + "groups": [{"id": group, "name": "\u{200b}"}], + "tags": [ + {"id": tags[0], "name": "\u{feff}", "color": "teal"}, + {"id": tags[1], "name": "\u{200b}".repeat(32), "color": "red"}, + {"id": tags[2], "name": format!("{}\u{feff}", "\u{200b}".repeat(31)), "color": "blue"}, + ], + }); + seed(&root, &serde_json::to_string(&document).expect("old file")); + let opened = store(&root); + let list = opened.list_blocking().expect("load the old library"); + assert_eq!(list.connections.as_ref(), std::slice::from_ref(&connection)); + assert_eq!(list.groups.len(), 1); + assert_eq!(list.groups[0].id, group); + assert_eq!(list.groups[0].name.as_str(), "\\u{200b}"); + assert_eq!(list.tags.len(), 3); + assert_eq!(list.tags[0].name.as_str(), "\\u{feff}"); + for tag in &list.tags[1..] { + assert_eq!(tag.name.as_str(), tag.id.simple().to_string()); + } + + opened.upsert_blocking(connection.clone()).expect("save the library"); + let reread = store(&root).list_blocking().expect("read the saved library"); + assert_eq!(reread.connections.as_ref(), &[connection]); + assert_eq!(reread.groups, list.groups); + assert_eq!(reread.tags, list.tags); + } + + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn recovered_names_do_not_take_visible_names_in_either_file_order() { + let tag_ids = [Uuid::new_v4(), Uuid::new_v4()]; + let group_ids = [Uuid::new_v4(), Uuid::new_v4()]; + let connections: Vec = (0..2) + .map(|index| SavedConnection { + group_id: Some(group_ids[index]), + tags: vec![tag_ids[index]], + ..sample_connection() + }) + .collect(); + for reverse in [false, true] { + let root = TempDir::new().expect("tempdir"); + let mut tags = vec![ + serde_json::json!({"id": tag_ids[0], "name": "\\u{200b}", "color": "blue"}), + serde_json::json!({"id": tag_ids[1], "name": "\u{200b}", "color": "red"}), + ]; + let mut groups = vec![ + serde_json::json!({"id": group_ids[0], "name": "\\u{200b}", "color": "blue"}), + serde_json::json!({"id": group_ids[1], "name": "\u{200b}", "color": "red"}), + ]; + if reverse { + tags.reverse(); + groups.reverse(); + } + seed( + &root, + &serde_json::json!({"version": 3, "connections": connections, "tags": tags, "groups": groups}) + .to_string(), + ); + let opened = store(&root); + let list = opened.list_blocking().expect("read the old library"); + assert_eq!(list.tags.len(), 2); + assert_eq!(list.groups.len(), 2); + for (index, color, name) in [ + (0, crate::ConnectionColor::Blue, "\\u{200b}"), + (1, crate::ConnectionColor::Red, "\\u{200b} (2)"), + ] { + let tag = list.tags.iter().find(|tag| tag.id == tag_ids[index]).expect("tag kept"); + assert_eq!(tag.name.as_str(), name); + assert_eq!(tag.color, color); + let group = list + .groups + .iter() + .find(|group| group.id == group_ids[index]) + .expect("group kept"); + assert_eq!(group.name.as_str(), name); + assert_eq!(group.color, Some(color)); + let connection = list + .connections + .iter() + .find(|connection| connection.id == connections[index].id) + .expect("connection kept"); + assert_eq!(connection.group_id, Some(group_ids[index])); + assert_eq!(connection.tags, [tag_ids[index]]); + } + opened.upsert_blocking(list.connections[0].clone()).expect("rewrite"); + let reread = store(&root).list_blocking().expect("read the saved library"); + assert_eq!(reread.tags, list.tags); + assert_eq!(reread.groups, list.groups); + let imported_root = TempDir::new().expect("tempdir"); + let imported = store(&imported_root); + imported + .import_blocking( + Path::new("/tmp/library.json"), + &opened.export_blocking().expect("export"), + ) + .expect("import"); + let imported = imported.list_blocking().expect("read imported library"); + assert_eq!(imported.tags.len(), 2); + assert_eq!(imported.groups.len(), 2); + for connection in connections.iter() { + let saved = imported + .connections + .iter() + .find(|saved| saved.id == connection.id) + .expect("imported connection"); + assert_eq!(saved.group_id, connection.group_id); + assert_eq!(saved.tags, connection.tags); + } + } + } + + #[test] + #[expect( + clippy::disallowed_types, + reason = "old library fixtures need names the current types reject; JSON key order does not matter" + )] + fn older_group_name_upgrade_keeps_literal_escapes_separate_from_invisible_names() { + let connections = [sample_connection(), sample_connection()]; + let mut expected_groups = None; + for reverse in [false, true] { + let root = TempDir::new().expect("tempdir"); + let mut entries: Vec = connections + .iter() + .zip(["\\u{200b}", "\u{200b}"]) + .map(|(connection, group)| { + let mut entry = serde_json::to_value(connection).expect("connection"); + entry["group"] = serde_json::Value::String(group.to_owned()); + entry + }) + .collect(); + if reverse { + entries.reverse(); + } + seed( + &root, + &serde_json::json!({"version": 2, "connections": entries}).to_string(), + ); + let opened = store(&root); + let list = opened.list_blocking().expect("upgrade the library"); + assert_eq!(list.groups.len(), 2); + let group_ids: Vec<_> = connections + .iter() + .zip(["\\u{200b}", "\\u{200b} (2)"]) + .map(|(connection, name)| { + let entry = list + .connections + .iter() + .find(|entry| entry.id == connection.id) + .expect("connection kept"); + let group = list + .groups + .iter() + .find(|group| Some(group.id) == entry.group_id) + .expect("group kept"); + assert_eq!(group.name.as_str(), name); + group.id + }) + .collect(); + if let Some(expected) = &expected_groups { + assert_eq!(&group_ids, expected); + } else { + expected_groups = Some(group_ids); + } + opened.upsert_blocking(list.connections[0].clone()).expect("rewrite"); + let reread = store(&root).list_blocking().expect("read the saved library"); + assert_eq!(reread.groups, list.groups); + for entry in list.connections.iter() { + assert_eq!( + reread + .connections + .iter() + .find(|saved| saved.id == entry.id) + .expect("saved connection") + .group_id, + entry.group_id + ); + } + } + } + #[test] fn f003_truncated_file_is_unavailable_and_upsert_leaves_bytes_identical() { let root = TempDir::new().expect("tempdir"); @@ -1786,19 +2005,121 @@ mod tests { assert_eq!(second.name, "production (imported)"); } + #[test] + fn a_rename_in_the_file_does_not_free_the_name_for_an_entry_under_the_same_id() { + let root = TempDir::new().expect("tempdir"); + let store = store(&root); + let mine = saved("production"); + store.upsert_blocking(mine.clone()).expect("upsert"); + let renamed = SavedConnection { + name: "staging".to_owned(), + ..mine.clone() + }; + let elsewhere = SavedConnection { + host: "db.other.example".to_owned(), + ..mine.clone() + }; + let bytes = crate::document::encode_document(&ConnectionsDocument { + connections: vec![renamed, elsewhere], + ..ConnectionsDocument::default() + }) + .expect("encode"); + + let report = store.import_blocking(Path::new("/tmp/c.json"), &bytes).expect("import"); + + assert_eq!(report, ImportReport { added: 1, updated: 1 }); + let list = store.load_blocking().expect("load"); + let kept = list.iter().find(|saved| saved.id == mine.id).expect("the saved entry"); + assert_eq!(kept.name, "staging"); + assert_eq!(kept.host, "db.internal"); + let added = list.iter().find(|saved| saved.id != mine.id).expect("the import"); + assert_eq!(added.name, "production (imported)"); + assert_eq!(added.host, "db.other.example"); + } + + #[test] + fn an_unchanged_reimport_keeps_names_two_entries_share() { + let root = TempDir::new().expect("tempdir"); + let store = store(&root); + store.upsert_blocking(saved("postgres@localhost")).expect("upsert"); + let reports = SavedConnection { + database: "reports".to_owned(), + ..saved("postgres@localhost") + }; + store.upsert_blocking(reports).expect("upsert"); + let bytes = store.export_blocking().expect("export"); + + let preview = store + .preview_import_blocking(Path::new("/tmp/c.json"), &bytes) + .expect("preview"); + + for entry in preview.entries() { + assert_eq!(entry.change, ImportChange::Updated { renamed_from: None }); + } + assert_eq!( + store.apply_import_blocking(&preview).expect("apply"), + ImportReport { added: 0, updated: 2 } + ); + let names: Vec = store + .load_blocking() + .expect("load") + .iter() + .map(|saved| saved.name.clone()) + .collect(); + assert_eq!(names, ["postgres@localhost", "postgres@localhost"]); + } + + #[test] + fn a_preview_names_what_an_updated_entry_was_called() { + let root = TempDir::new().expect("tempdir"); + let store = store(&root); + let production = saved("production"); + let staging = saved("staging"); + store.upsert_blocking(production.clone()).expect("upsert"); + store.upsert_blocking(staging.clone()).expect("upsert"); + let renamed = SavedConnection { + name: "reporting".to_owned(), + ..production + }; + let bytes = crate::document::encode_document(&ConnectionsDocument { + connections: vec![renamed, staging], + ..ConnectionsDocument::default() + }) + .expect("encode"); + + let preview = store + .preview_import_blocking(Path::new("/tmp/c.json"), &bytes) + .expect("preview"); + + let changes: Vec = preview.entries().iter().map(|entry| entry.change.clone()).collect(); + assert_eq!( + changes, + [ + ImportChange::Updated { + renamed_from: Some("production".to_owned()) + }, + ImportChange::Updated { renamed_from: None }, + ] + ); + assert_eq!(preview.entries()[0].connection.name, "reporting"); + } + #[test] fn a_name_that_only_looks_like_a_saved_one_is_suffixed() { - for lookalike in [ - "production ", - " production", - "production\u{a0}", - "produ\u{200b}ction", - "\u{feff}production", - "production\u{2060}", + for (mine, lookalike) in [ + ("production", "production "), + ("production", " production"), + ("production", "production\u{a0}"), + ("production", "produ\u{200b}ction"), + ("production", "\u{feff}production"), + ("production", "production\u{2060}"), + ("staging db", "staging\u{a0}db"), + ("staging db", "staging\u{2007}db"), + ("Caf\u{e9} prod", "Cafe\u{301} prod"), ] { let root = TempDir::new().expect("tempdir"); let store = store(&root); - store.upsert_blocking(saved("production")).expect("upsert"); + store.upsert_blocking(saved(mine)).expect("upsert"); let imported = SavedConnection { host: "db.attacker.example".to_owned(), ..saved(lookalike) @@ -1813,7 +2134,7 @@ mod tests { let list = store.load_blocking().expect("load"); let added = list.iter().find(|saved| saved.id == imported.id).expect("the import"); - assert_eq!(added.name, "production (imported)", "{lookalike:?}"); + assert_eq!(added.name, format!("{mine} (imported)"), "{lookalike:?}"); } } @@ -2329,6 +2650,27 @@ mod tests { assert_eq!(store.load_blocking().expect("list")[0].tags, [mine.id, fresh.id]); } + #[test] + fn an_imported_tag_that_only_looks_like_one_the_list_has_becomes_that_tag() { + let root = TempDir::new().expect("tempdir"); + let store = store(&root); + let mine = store + .create_tag_blocking(tag_name("production"), crate::ConnectionColor::Red) + .expect("the tag"); + let theirs = tag("lookalike", crate::ConnectionColor::Green); + let connection = tagged("globex-prod", &[&theirs]); + let file = String::from_utf8(document(vec![connection], vec![theirs])) + .expect("utf-8") + .replace("lookalike", "production\u{200b}"); + + store + .import_blocking(Path::new("/tmp/c.json"), file.as_bytes()) + .expect("import"); + + assert_eq!(store.tags_blocking().expect("tags").to_vec(), vec![mine.clone()]); + assert_eq!(store.load_blocking().expect("list")[0].tags, [mine.id]); + } + #[test] fn an_imported_tag_with_a_known_id_takes_the_files_name_unless_another_tag_has_it() { let root = TempDir::new().expect("tempdir"); diff --git a/core/crates/storage/src/name_rules.rs b/core/crates/storage/src/name_rules.rs index 1c9da5a867..a3828d2079 100644 --- a/core/crates/storage/src/name_rules.rs +++ b/core/crates/storage/src/name_rules.rs @@ -1,5 +1,9 @@ -//! The rules a group's or a tag's name follows, shared so the two cannot -//! drift apart. +//! The rules a connection's, a group's and a tag's name follow, shared so +//! they cannot drift apart. + +use std::collections::HashSet; + +use unicode_normalization::UnicodeNormalization; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum NameFault { @@ -7,12 +11,15 @@ pub(crate) enum NameFault { ControlCharacter, } -pub(crate) fn checked(value: &str) -> Result<&str, NameFault> { - let name = value.trim(); +/// `value` as [`shown`] gives it, when it is a name a menu, a row and a +/// section title can all show. +pub(crate) fn checked(value: &str) -> Result { + let name = shown(value); if name.is_empty() { return Err(NameFault::Empty); } - if name.chars().any(char::is_control) { + // A typed name is refused for one rather than shown with a space. + if value.trim().chars().any(char::is_control) { return Err(NameFault::ControlCharacter); } Ok(name) @@ -20,21 +27,43 @@ pub(crate) fn checked(value: &str) -> Result<&str, NameFault> { // A file is never refused for what an earlier build let the user type. pub(crate) fn cleaned(value: &str) -> String { - let shown: String = value.chars().map(|c| if c.is_control() { ' ' } else { c }).collect(); - shown.trim().to_owned() + let name = shown(value); + if !name.is_empty() { + return name; + } + // Older builds accepted names made only of invisible characters. + // Keep the library readable and give those names something to show. + let old: String = value.chars().map(|c| if c.is_control() { ' ' } else { c }).collect(); + old.trim().chars().flat_map(char::escape_default).collect() } -/// A connection's name as the list shows it: control characters as -/// spaces, characters that show as nothing dropped, and no blanks around -/// it. Two names that read the same have the same form, so one cannot -/// pass for the other. +// Recovery must not take a visible name and make the library merge its owner. +pub(crate) fn claim_recovered(value: &str, max: Option, held: &mut HashSet) -> String { + let mut name = value.to_owned(); + let mut suffix = 2_usize; + while !held.insert(name.to_lowercase()) { + let tail = format!(" ({suffix})"); + let head: String = value + .chars() + .take(max.map_or(usize::MAX, |max| max.saturating_sub(tail.len()))) + .collect(); + name = format!("{head}{tail}"); + suffix += 1; + } + name +} + +/// A name as the list shows it: characters that show as nothing dropped, +/// control characters and every other space as a plain space, no blanks +/// around it, and each letter in its composed form. Two names that read +/// the same have the same form, so one cannot pass for the other. pub(crate) fn shown(value: &str) -> String { let visible: String = value .chars() .filter(|c| !tablepro_core::unicode_blank::is_default_ignorable(*c)) - .map(|c| if c.is_control() { ' ' } else { c }) + .map(|c| if c.is_control() || c.is_whitespace() { ' ' } else { c }) .collect(); - visible.trim().to_owned() + visible.trim().nfc().collect() } /// A stored name, read as [`shown`] gives it. @@ -63,15 +92,47 @@ mod tests { } #[test] - fn a_name_is_trimmed_and_checked() { - assert_eq!(checked(" Work "), Ok("Work")); + fn a_shown_name_has_one_space_and_one_form_of_each_letter() { + for lookalike in [ + "staging\u{a0}db", + "staging\u{2007}db", + "staging\u{3000}db", + "staging\u{2028}db", + ] { + assert_eq!(shown(lookalike), "staging db", "{lookalike:?}"); + } + assert_eq!(shown("Cafe\u{301} prod"), "Caf\u{e9} prod"); + assert_eq!(shown("Caf\u{e9} prod"), "Caf\u{e9} prod"); + } + + #[test] + fn a_name_is_trimmed_and_checked_in_characters() { + assert_eq!(checked(" Work "), Ok("Work".to_owned())); + assert_eq!(checked("éééé"), Ok("éééé".to_owned())); + assert_eq!(checked("e\u{301}e\u{301}e\u{301}e\u{301}"), Ok("\u{e9}".repeat(4))); assert_eq!(checked(" \t "), Err(NameFault::Empty)); + assert_eq!(checked("\u{200b}"), Err(NameFault::Empty)); assert_eq!(checked("a\nb"), Err(NameFault::ControlCharacter)); } + #[test] + fn a_checked_name_drops_what_cannot_be_seen() { + assert_eq!(checked("pr\u{200b}od\u{feff}"), Ok("prod".to_owned())); + assert_eq!(checked("a\u{a0}b"), Ok("a b".to_owned())); + } + #[test] fn a_stored_name_loses_control_characters() { assert_eq!(cleaned("Work\tHome\n"), "Work Home"); + assert_eq!(cleaned("abcdef"), "abcdef"); assert_eq!(cleaned(" \u{7} "), ""); + assert_eq!(cleaned("pro\u{200b}d\u{a0}x"), "prod x"); + } + + #[test] + fn a_stored_invisible_name_is_visible_without_accepting_a_new_one() { + assert_eq!(cleaned(" \u{200b}\u{feff} "), "\\u{200b}\\u{feff}"); + assert_eq!(checked("\u{200b}\u{feff}"), Err(NameFault::Empty)); + assert_eq!(cleaned(" \t\n "), ""); } } diff --git a/linux/changelog.d/4036.fixed.1.md b/linux/changelog.d/4036.fixed.1.md new file mode 100644 index 0000000000..7396097a63 --- /dev/null +++ b/linux/changelog.d/4036.fixed.1.md @@ -0,0 +1 @@ +PostgreSQL metadata reads could use a user-defined function or operator instead of a built-in. diff --git a/linux/changelog.d/4036.fixed.2.md b/linux/changelog.d/4036.fixed.2.md new file mode 100644 index 0000000000..340e9ec78c --- /dev/null +++ b/linux/changelog.d/4036.fixed.2.md @@ -0,0 +1 @@ +Imported connections could hide changed names, SSH servers or weaker TLS settings. diff --git a/linux/changelog.d/4036.fixed.3.md b/linux/changelog.d/4036.fixed.3.md new file mode 100644 index 0000000000..17d2d5d586 --- /dev/null +++ b/linux/changelog.d/4036.fixed.3.md @@ -0,0 +1 @@ +Connection imports could rename unchanged entries or confuse names with different Unicode spacing. diff --git a/linux/changelog.d/4036.fixed.4.md b/linux/changelog.d/4036.fixed.4.md new file mode 100644 index 0000000000..3118bf5e66 --- /dev/null +++ b/linux/changelog.d/4036.fixed.4.md @@ -0,0 +1 @@ +Some SQL containing passwords was saved in query history. diff --git a/linux/changelog.d/4036.fixed.5.md b/linux/changelog.d/4036.fixed.5.md new file mode 100644 index 0000000000..a6b12ee80f --- /dev/null +++ b/linux/changelog.d/4036.fixed.5.md @@ -0,0 +1 @@ +Unusual database replies or SQLite queries containing NUL could crash or stall a connection. diff --git a/linux/changelog.d/4036.fixed.6.md b/linux/changelog.d/4036.fixed.6.md new file mode 100644 index 0000000000..f132d01986 --- /dev/null +++ b/linux/changelog.d/4036.fixed.6.md @@ -0,0 +1 @@ +MySQL read connections could permit writes after statements that ended a transaction. diff --git a/linux/changelog.d/4036.fixed.7.md b/linux/changelog.d/4036.fixed.7.md new file mode 100644 index 0000000000..4437b0e77d --- /dev/null +++ b/linux/changelog.d/4036.fixed.7.md @@ -0,0 +1 @@ +MongoDB count queries could report zero when a server reply could not be decoded. diff --git a/linux/changelog.d/4036.fixed.md b/linux/changelog.d/4036.fixed.md new file mode 100644 index 0000000000..e9b16547c2 --- /dev/null +++ b/linux/changelog.d/4036.fixed.md @@ -0,0 +1 @@ +Some DuckDB and SQL Server statements bypassed write confirmation or split at the wrong place. diff --git a/linux/crates/app/src/ui/import_preview.rs b/linux/crates/app/src/ui/import_preview.rs index 1e6a24681b..e7735e7160 100644 --- a/linux/crates/app/src/ui/import_preview.rs +++ b/linux/crates/app/src/ui/import_preview.rs @@ -2,10 +2,11 @@ use gtk4 as gtk; use gtk4::{gio, glib}; use libadwaita as adw; use libadwaita::prelude::*; -use tablepro_core::{AiPolicy, DriverRegistry, ExternalAccess}; -use tablepro_storage::{ImportChange, ImportEntry, ImportPreview}; +use tablepro_core::{AiPolicy, DriverCapabilities, DriverRegistry, ExternalAccess, TlsMode}; +use tablepro_storage::{ImportChange, ImportEntry, ImportPreview, SavedSshConfig}; use crate::services::saved_connection_object::subtitle_for; +use crate::ui::connect_dialog::ConnectForm; pub(crate) const IMPORT: &str = "import"; const CANCEL: &str = "cancel"; @@ -64,18 +65,31 @@ struct EntryText { fn entry_text(entry: &ImportEntry, drivers: &DriverRegistry) -> EntryText { let saved = &entry.connection; - let file_based = drivers - .get(&saved.driver_id) - .is_some_and(|driver| driver.capabilities().is_file_based()); + let caps = drivers.get(&saved.driver_id).map(|driver| driver.capabilities()); + let file_based = caps.is_some_and(DriverCapabilities::is_file_based); let change = match entry.change { ImportChange::Added => crate::i18n::gettext("New"), - ImportChange::Updated => crate::i18n::gettext("Updated"), + ImportChange::Updated { .. } => crate::i18n::gettext("Updated"), }; let mut lines = vec![format!("{change} · {}", subtitle_for(saved, file_based, None))]; - // An updated entry keeps its own settings; only the list's labels - // come from the file. - if entry.change == ImportChange::Added { - lines.extend(looser_settings(saved.external_access, saved.ai_policy)); + match &entry.change { + ImportChange::Added => { + if !file_based { + lines.extend(saved.ssh.as_ref().map(ssh_hop)); + } + if let Some(caps) = caps { + let new_connection = ConnectForm::new(&saved.driver_id, caps).tls.mode; + lines.extend(looser_tls(saved.tls.mode, new_connection)); + } + lines.extend(looser_settings(saved.external_access, saved.ai_policy)); + } + // An updated entry keeps its own settings; only the list's labels + // come from the file. + ImportChange::Updated { renamed_from } => { + if let Some(name) = renamed_from.as_deref() { + lines.push(crate::i18n::gettext_f("Renamed from “{name}”", &[("name", name)])); + } + } } EntryText { title: saved.name.clone(), @@ -110,6 +124,43 @@ fn row_factory() -> gtk::SignalListItemFactory { factory } +/// The SSH server the connection goes through, after any jump hosts. +fn ssh_hop(ssh: &SavedSshConfig) -> String { + let host = match ssh.port { + Some(port) => format!("{}:{port}", ssh.host), + None => ssh.host.clone(), + }; + let target = match ssh.username.as_deref().filter(|user| !user.is_empty()) { + Some(user) => format!("{user}@{host}"), + None => host, + }; + if ssh.jump_hosts.is_empty() { + crate::i18n::gettext_f("Through SSH to {target}", &[("target", &target)]) + } else { + crate::i18n::gettext_f( + "Through SSH to {target} via {jumps}", + &[("target", &target), ("jumps", &ssh.jump_hosts.join(", "))], + ) + } +} + +/// What the file's TLS mode skips that a new connection of the same +/// driver checks. +fn looser_tls(mode: TlsMode, new_connection: TlsMode) -> Option { + if new_connection.encrypts() && !mode.encrypts() { + return Some(crate::i18n::gettext("Connects without TLS")); + } + if new_connection.verifies_certificate() && !mode.verifies_certificate() { + return Some(crate::i18n::gettext( + "Connects over TLS without checking the certificate", + )); + } + if new_connection == TlsMode::Strict && mode != TlsMode::Strict { + return Some(crate::i18n::gettext("Connects without strict encryption")); + } + None +} + /// What the file allows beyond what a new connection starts with. fn looser_settings(access: ExternalAccess, policy: AiPolicy) -> Vec { let mut lines = Vec::new(); @@ -124,6 +175,13 @@ fn looser_settings(access: ExternalAccess, policy: AiPolicy) -> Vec { #[cfg(test)] mod tests { + use std::sync::Arc; + + use tablepro_core::{AuthMode, SafeModeLevel, TlsConfig}; + use tablepro_storage::{SavedConnection, SavedSshAuth}; + use tablepro_test_support::{ConnectionScript, FakeDriver}; + use uuid::Uuid; + use super::*; #[test] @@ -132,4 +190,154 @@ mod tests { assert!(looser_settings(ExternalAccess::Blocked, AiPolicy::Never).is_empty()); assert_eq!(looser_settings(ExternalAccess::ReadWrite, AiPolicy::Allow).len(), 2); } + + #[test] + fn only_tls_looser_than_a_new_connections_is_called_out() { + assert_eq!( + looser_tls(TlsMode::Disable, TlsMode::VerifyFull).as_deref(), + Some("Connects without TLS") + ); + assert_eq!( + looser_tls(TlsMode::Require, TlsMode::VerifyFull).as_deref(), + Some("Connects over TLS without checking the certificate") + ); + assert_eq!( + looser_tls(TlsMode::Disable, TlsMode::Require).as_deref(), + Some("Connects without TLS") + ); + assert_eq!(looser_tls(TlsMode::VerifyFull, TlsMode::VerifyFull), None); + assert_eq!(looser_tls(TlsMode::Require, TlsMode::Require), None); + // A driver without TLS starts a new connection with none. + assert_eq!(looser_tls(TlsMode::Disable, TlsMode::Disable), None); + } + + #[test] + fn strict_checks_the_certificate_and_also_encrypts_the_prelogin_exchange() { + for default in [TlsMode::Disable, TlsMode::Require, TlsMode::VerifyFull, TlsMode::Strict] { + assert_eq!(looser_tls(TlsMode::Strict, default), None); + } + assert_eq!( + looser_tls(TlsMode::VerifyFull, TlsMode::Strict).as_deref(), + Some("Connects without strict encryption") + ); + assert_eq!( + looser_tls(TlsMode::Require, TlsMode::Strict).as_deref(), + Some("Connects over TLS without checking the certificate") + ); + assert_eq!( + looser_tls(TlsMode::Disable, TlsMode::Strict).as_deref(), + Some("Connects without TLS") + ); + } + + fn tunnel(port: Option, username: Option<&str>, jump_hosts: &[&str]) -> SavedSshConfig { + SavedSshConfig { + host: "bastion.example".to_owned(), + port, + username: username.map(str::to_owned), + jump_hosts: jump_hosts.iter().copied().map(str::to_owned).collect(), + auth: SavedSshAuth::Agent, + } + } + + #[test] + fn an_ssh_hop_names_the_server_and_the_jump_hosts_before_it() { + assert_eq!( + ssh_hop(&tunnel(None, Some("deploy"), &[])), + "Through SSH to deploy@bastion.example" + ); + assert_eq!(ssh_hop(&tunnel(None, None, &[])), "Through SSH to bastion.example"); + assert_eq!( + ssh_hop(&tunnel(Some(2222), Some("deploy"), &["edge.example", "gate.example"])), + "Through SSH to deploy@bastion.example:2222 via edge.example, gate.example" + ); + } + + fn drivers() -> DriverRegistry { + let mut drivers = DriverRegistry::new(); + drivers.register(Arc::new(FakeDriver::connecting( + &FakeDriver::NETWORK_CAPABILITIES, + ConnectionScript::default(), + ))); + drivers + } + + fn entry(change: ImportChange, tls: TlsConfig, ssh: Option) -> ImportEntry { + ImportEntry { + connection: SavedConnection { + id: Uuid::new_v4(), + name: "orders".to_owned(), + driver_id: "fake".to_owned(), + host: "db.internal".to_owned(), + port: 5432, + database: "orders".to_owned(), + username: "app".to_owned(), + tls, + safe_mode: SafeModeLevel::Off, + external_access: ExternalAccess::ReadOnly, + ai_policy: AiPolicy::Ask, + query_timeout_secs: None, + auth_mode: AuthMode::Password, + ssh, + last_opened_at: None, + color: None, + group_id: None, + order: 0, + tags: Vec::new(), + icon: None, + options: Default::default(), + }, + change, + } + } + + fn lines(entry: &ImportEntry) -> Vec { + entry_text(entry, &drivers()) + .subtitle + .lines() + .map(str::to_owned) + .collect() + } + + #[test] + fn an_added_entry_says_how_it_connects_beyond_its_host() { + let tunnelled = entry( + ImportChange::Added, + TlsConfig::disabled(), + Some(tunnel(None, Some("deploy"), &[])), + ); + assert_eq!( + lines(&tunnelled), + [ + "New · fake · app@db.internal:5432", + "Through SSH to deploy@bastion.example", + "Connects without TLS", + ] + ); + + let checked = entry(ImportChange::Added, TlsConfig::verify_full(), None); + assert_eq!(lines(&checked), ["New · fake · app@db.internal:5432"]); + } + + #[test] + fn an_updated_entry_says_what_it_was_called() { + let renamed = entry( + ImportChange::Updated { + renamed_from: Some("production".to_owned()), + }, + TlsConfig::disabled(), + None, + ); + assert_eq!( + lines(&renamed), + ["Updated · fake · app@db.internal:5432", "Renamed from “production”"] + ); + + let kept = entry( + ImportChange::Updated { renamed_from: None }, + TlsConfig::verify_full(), + None, + ); + assert_eq!(lines(&kept), ["Updated · fake · app@db.internal:5432"]); + } } diff --git a/linux/crates/app/src/ui/session_window/connections.rs b/linux/crates/app/src/ui/session_window/connections.rs index 4412d38be9..a75493bc83 100644 --- a/linux/crates/app/src/ui/session_window/connections.rs +++ b/linux/crates/app/src/ui/session_window/connections.rs @@ -1244,16 +1244,24 @@ mod tests { ); } - /// A file whose entry takes a saved connection's name but opens - /// another file: the dialog shows where it goes and the suffixed name - /// before anything is written, and only Import writes it. + /// A file whose entry takes a saved connection's name but connects + /// to another host without TLS: the dialog shows where it goes, that + /// it skips TLS, and the suffixed name before anything is written, + /// and only Import writes it. #[gtk4::test] fn an_import_shows_where_each_connection_goes_before_writing() { - let test = TestWindow::picker("ImportPreview"); + let test = TestWindow::with_script( + "ImportPreview", + crate::ui::session_window::tests::Script { + network: true, + ..Default::default() + }, + ); wait_for_list(&test.window); let lookalike = SavedConnection { id: Uuid::new_v4(), - database: "/srv/elsewhere.sqlite3".to_owned(), + host: "db.elsewhere.example".to_owned(), + tls: tablepro_core::TlsConfig::disabled(), ..test.saved.clone() }; let entry = serde_json::to_string(&lookalike).expect("the entry"); @@ -1277,13 +1285,9 @@ mod tests { .expect("the rows"); assert_eq!(rows.len(), 1); assert_eq!(rows[0].title(), format!("{} (imported)", test.saved.name)); - assert!( - rows[0] - .subtitle() - .is_some_and(|subtitle| subtitle.contains("elsewhere.sqlite3")), - "{:?}", - rows[0].subtitle() - ); + let subtitle = rows[0].subtitle().unwrap_or_default(); + assert!(subtitle.contains("app@db.elsewhere.example"), "{subtitle}"); + assert!(subtitle.contains("Connects without TLS"), "{subtitle}"); assert!(!listed(&test, lookalike.id), "the preview wrote the list"); respond(&alert, crate::ui::import_preview::IMPORT); diff --git a/linux/docs/drivers/postgresql.md b/linux/docs/drivers/postgresql.md index f1f6e94040..38c9ca5a06 100644 --- a/linux/docs/drivers/postgresql.md +++ b/linux/docs/drivers/postgresql.md @@ -34,7 +34,7 @@ COMMIT - Reads always run in a `READ ONLY` transaction, also on a read-write connection. - `statement_timeout` is the nearer of the call's deadline and the workspace's 60 s. `lock_timeout` is 10 s, so a blocked edit reports the lock instead of a generic timeout. - `SET LOCAL` ends with the transaction, so nothing is left on a server connection a pooler shares. -- A catalog read runs under the session's own search_path, so names print as the user's session sees them, and resolves nothing through it: every function is written `pg_catalog.`, and every operator compares two values of one built-in type, cast where they differ (`attrelid = $1::text::regclass::oid`), or is written `OPERATOR(pg_catalog.)`. A lookalike another user creates on the path can then only tie with the built-in, and `pg_catalog`, searched first, wins the tie. `bare_calls` in `metadata/mod.rs` checks every catalog statement for an unqualified call. +- A catalog read's transaction also runs `SELECT pg_catalog.set_config('search_path', ..., true)`, which puts `pg_catalog` ahead of the session's own search_path until the COMMIT. The default schema is captured before the path changes. A table name resolves to its OID on the session's original path in the same transaction, before metadata reads use the protected path. Empty paths remain valid. Every function is written `pg_catalog.`, and every operator compares two values of one built-in type, cast where they differ (`attrelid = $1::text::regclass::oid`), or is written `OPERATOR(pg_catalog.)`. A lookalike another user creates on the path can then only tie with the built-in, and `pg_catalog` wins the tie even when the session's path names it after another schema. `bare_calls` in `metadata/mod.rs` checks every catalog statement for an unqualified call. Grid reads and counts keep the session's path, as the user's own SQL does. - The sidebar lists functions, procedures, sequences and user triggers in their schema's section, a trigger with the table it fires on beside it. Two triggers of one name on different tables are both kept, and a partition's clone of its parent's trigger (`tgparentid` set) is left out. A definition comes from `pg_get_functiondef` (every overload), `pg_get_triggerdef` without pretty printing, which names the table with its schema, or for a sequence a `CREATE SEQUENCE` built from `pg_sequences` with every clause the statement takes: `UNLOGGED` or `TEMPORARY` from `relpersistence`, and `OWNED BY` the column a `pg_depend` row of type `a` names. An identity column's sequence (type `i`) is made with its table, so its text does not open in the editor. - Generated SQL runs on the unnamed statement (`query_typed_raw`, `execute_typed`) with typed parameters. Nothing is prepared under a name. - A connection whose flight did not end with an acknowledged COMMIT or ROLLBACK is taken out of the pool, because deadpool's recycle check (`RecyclingMethod::Verified`) passes inside a half-finished transaction. diff --git a/linux/docs/storage.md b/linux/docs/storage.md index 119df62fbd..ecc8a25fab 100644 --- a/linux/docs/storage.md +++ b/linux/docs/storage.md @@ -26,17 +26,18 @@ The app's data goes to the store its shape calls for. The core's `storage` crate - `external_access` is what MCP clients may do on the connection: `blocked`, `read_only` or `read_write` ([ADR 0022](decisions/0022-ai-assistant-and-mcp-server.md)). A file written before it existed reads as `read_only`, the level a new connection starts at. - `ai_policy` is what the AI assistant may send from the connection: `ask`, `allow` or `never`. A file written before it existed reads as `ask`, the policy a new connection starts at. - `query_timeout_secs` is the editor's query timeout on the connection, in seconds, with 0 for none. Left out, as in a file written before it existed, the connection follows the timeout in Preferences, and it is written back without one. -- The file's `groups` list is the library's folders, a tree ([core ADR 0006](../../core/docs/decisions/0006-connection-library.md)). Each `ConnectionGroup` has an `id`, a `name` (`ConnectionGroupName`: trimmed, not empty, of any length as on macOS, no control characters, which a read turns into spaces), a `parent_id` (none for a top-level group), an `order` among its siblings, and an optional `color` and `icon` (`GroupIcon`: one of the `ConnectionIcon` names, or the SF Symbol a macOS group wears, which Linux does not draw). Groups nest at most 3 levels. A new or moved group may not share a name with a sibling in any case; a list upgraded from version 2 can still hold two that differ only in case. A group reordered inside its own parent is checked for neither rule, so two such groups, or one a hand-edited file nests deeper than 3 levels, still reorder. +- The file's `groups` list is the library's folders, a tree ([core ADR 0006](../../core/docs/decisions/0006-connection-library.md)). Each `ConnectionGroup` has an `id`, a `name` (`ConnectionGroupName`: in a connection name's form, not empty, of any length as on macOS, no control characters, which a read turns into spaces), a `parent_id` (none for a top-level group), an `order` among its siblings, and an optional `color` and `icon` (`GroupIcon`: one of the `ConnectionIcon` names, or the SF Symbol a macOS group wears, which Linux does not draw). Groups nest at most 3 levels. A new or moved group may not share a name with a sibling in any case; a list upgraded from version 2 can still hold two that differ only in case. A group reordered inside its own parent is checked for neither rule, so two such groups, or one a hand-edited file nests deeper than 3 levels, still reorder. - A connection's `group_id` names its group, none for the top of the list, and its `order` is its place there. Every write numbers each group's groups and connections from 0, so a new entry, or one moved to another group, goes last there. `insert_after_blocking` saves a new entry right after another one, in that one's group, in one write; when that one is gone it is saved as `upsert_blocking` saves it. A read roots a group whose parent is missing or that sits on a cycle, keeps the first of two entries with one id, and moves a connection whose group is missing to the top. - `create_group_blocking`, `rename_group_blocking`, `set_group_color_blocking`, `set_group_icon_blocking`, `move_groups_blocking` (to another parent, before a sibling), `delete_group_blocking`, `move_connections_blocking` (to a group, before a connection) and `move_connections_to_group_named_blocking` change the tree. Each refuses what would break it with a `StorageError` carrying a stable code: `GroupNotFound`, `GroupExists`, `GroupNameTaken`, `GroupCycle`, `GroupTooDeep`, `ConnectionNotFound`. Deleting a group deletes the groups inside it and moves their connections to the top of the list. `GroupTree::can_place` and `GroupTree::can_create_in` apply the same parent, cycle and depth rules without writing, so the app can turn off a move or a New Subgroup the store would refuse. - `favorites` lists the favourite connections in their order. `add_favorites_blocking` adds or moves some before a given one, `remove_favorites_blocking` takes them off, and deleting a connection drops it. `ConnectionList::recent` is the opened connections, newest first, without the favourites. - The Linux app's picker and the window's tree show the groups as a tree in the library's order, empty ones too, each group's subgroups above its connections. Only the Move to Group menus and the search results name a group by its path from the top, such as `Work / Staging`. The picker also has a Favourites section, in the favourites' order, and a Recent section with the last 5 connections opened, favourites left out. New Group makes a group at the top and New Subgroup makes one inside a group, up to 3 levels. Groups and connections move by drag and drop, or with Move Up and Move Down. - Version 2 filed a connection under a `group` name. Its upgrade makes each name a top-level group whose id comes from the name, so identical names share one group and a read before the first save gives the same ids; names are cleaned as above, and a blank one is no group. Groups and each group's connections are numbered by name. -- The file's `tags` list holds every `ConnectionTag`: an `id`, a `name` (`ConnectionTagName`: trimmed, 1 to 32 characters, no control characters, cleaned on read as a group's is and cut to 32) and a `color` from the same palette as `color`. No two tags share a name in any case: `create_tag_blocking` and `update_tag_blocking` refuse one with `TagNameTaken`. +- The file's `tags` list holds every `ConnectionTag`: an `id`, a `name` (`ConnectionTagName`: in a connection name's form, 1 to 32 characters, no control characters, cleaned on read as a group's is and cut to 32) and a `color` from the same palette as `color`. No two tags share a name in any case: `create_tag_blocking` and `update_tag_blocking` refuse one with `TagNameTaken`. - A connection's `tags` are ids from that list. Every write drops an id the list lacks and a repeated one, so `delete_tag_blocking` takes the tag off every connection in the same write, and a connection saved with a tag deleted meanwhile loses it. - `icon` is one of the `ConnectionIcon` names. The app draws each with a symbolic icon from the theme (`crates/app/src/ui/icons.rs`). -- A connection's `name` is stored and compared as the list shows it: control characters as spaces, characters that show as nothing (U+200B, U+FEFF) dropped, no blanks around it, so a name that looks like a saved one is the same name. -- An import is previewed first: `preview_import_blocking` works out every entry as it will be saved and writes nothing, the app shows each one with its host, and `apply_import_blocking` writes that result, or refuses with `ImportOutdated` when the list changed in between. An imported entry never takes a name another entry holds or held before the import, whatever order the file lists them in. +- A connection's `name` is stored and compared as the list shows it: characters that show as nothing (U+200B, U+FEFF) dropped, control characters and every other space (U+00A0, U+2007) as a plain space, no blanks around it, and in Unicode NFC, so a name that looks like a saved one is the same name. Tag and group names take the same form. +- Older group and tag names made only of invisible characters read as Unicode escapes, so the library still opens. When those escapes exceed a tag's 32-character limit, its name becomes its id without hyphens. Recovery reserves visible names first and adds a number when a recovered name is taken, cutting a tag's prefix to stay within 32 characters. The ids and connection links stay the same; new invisible-only names are refused. +- An import is previewed first: `preview_import_blocking` works out every entry as it will be saved and writes nothing, the app shows each one with its host, and `apply_import_blocking` writes that result, or refuses with `ImportOutdated` when the list changed in between. For an added entry the preview also names its SSH server and a TLS mode looser than a new connection's; for an updated one, the name it had when the file renames it (`ImportChange::Updated`, `renamed_from`). An imported entry never takes a name another entry holds or held before the import, whatever order the file lists them in. An updated entry whose name the file keeps keeps it, even when another entry has the same one. - On import, a tag is matched by id, then by name in any case, and added only when neither matches, so importing a file twice leaves one tag of each name. A tag matched by id takes the file's colour, and its name unless another tag has it. A connection updated by an import takes the file's tags and icon. - On import, a group is matched by id, then by the exact name under the same parent, then by that name in any case among the groups the list had before the import, and otherwise added last in its place. So a file's own `Work` and `work` stay two groups, as a read of that file gives. A group matched by id keeps its place and takes the file's colour, icon, and name unless a sibling has it. Those renames come first, so a name one of them frees or takes decides how the rest match, whatever order the file lists them in. A group the file nests below the third level is not added: it and the groups under it map to the third-level group above them, so their connections land there, as macOS cuts the path. A connection updated by an import takes the file's group, going last there when the group changed; an added one keeps the file's order after the list's own. The file's favourites are added, and an import never removes one. - An edit, a copy or a move to another group goes through the app's `SavedConnectionEditor`, which writes the keyring first. A copy is saved with `insert_after_blocking`, right after its original, as on macOS. A refused secret stops before the list changes, and the secrets of a copy whose entry did not land are deleted again. Edits and copies run one at a time, so two windows editing one connection never leave the host of one with the password of the other. @@ -260,8 +261,9 @@ script lexer: PostgreSQL, a plain string after `DO` or `AS`. So is every string of a script that hands the server text to run (T-SQL `EXEC` and `sp_executesql`, MySQL `PREPARE ... FROM` and `EXECUTE IMMEDIATE`, - PL/pgSQL `EXECUTE`), since the text may reach them through a variable. - On SQL Server a named argument whose name holds `password` or `pwd` + PL/pgSQL `EXECUTE`) or stores a string in a T-SQL or MySQL `@` variable, + since the text may reach them through a variable, in this run or a later one. + On SQL Server a named argument whose name holds `password`, `passwd` or `pwd` withholds the entry whatever the procedure. The text is read every way the server may read it: with and without backslash escapes, and with each MySQL `/*!` comment run or skipped as each server version