From c5d4e936d5970bffc6497dc0e32ff10a4cf62f2b Mon Sep 17 00:00:00 2001 From: Abeautifulsnow Date: Wed, 14 Oct 2026 14:07:20 +0800 Subject: [PATCH] fix(oracle): prove built-in function queries read-only --- crates/dbx-core/src/query/mod.rs | 122 +++++- .../dbx-sql-core/src/query_execution_sql.rs | 261 ++++++------ crates/dbx-sql-core/src/sql_risk.rs | 372 +++++++++++++++++- 3 files changed, 625 insertions(+), 130 deletions(-) diff --git a/crates/dbx-core/src/query/mod.rs b/crates/dbx-core/src/query/mod.rs index c5a744c92d..0210302f6f 100644 --- a/crates/dbx-core/src/query/mod.rs +++ b/crates/dbx-core/src/query/mod.rs @@ -6083,9 +6083,11 @@ fn mysql_error_is_syntax_error(error: &mysql_async::Error) -> bool { /// manual-transaction UX (#7122 Oracle, #9018 MySQL/PostgreSQL, ... GaussDB). /// The user-facing classification SQL is split with the same dialect-aware /// splitter as the execution SQL and paired by count/position; any mismatch is -/// fail-closed (no markers). Oracle/OceanBase-Oracle keep the lexical -/// classifier, the PostgreSQL family (MySQL is separate) use the strict -/// `sql_risk` proof; every other dialect is unproven. +/// fail-closed (no markers). Oracle/OceanBase-Oracle reach the same AST proof as +/// the PostgreSQL family through `is_oracle_proven_read_only_statement`, which +/// keeps the Oracle-specific lexical guard rails (see `sql_risk`); MySQL and the +/// PostgreSQL family use the strict `sql_risk` proof directly; every other +/// dialect is unproven. fn classify_manual_transaction_statements( database_type: Option, execution_statement_count: usize, @@ -12168,11 +12170,26 @@ for line in sys.stdin: assert!(classify_manual_transaction_statements(Some(DatabaseType::Mysql), 1, None).is_empty()); assert!(classify_manual_transaction_statements(Some(DatabaseType::Doris), 1, Some("SELECT 1")).is_empty()); assert!(classify_manual_transaction_statements(None, 1, Some("SELECT 1")).is_empty()); - // Oracle keeps its lexical classifier and its pairing behavior. + // Oracle/OceanBase-Oracle share the AST proof and keep the pairing + // behavior; built-in functions are now provable (#11788) while the + // Oracle guard rails (dblink, sequence pseudocolumns, set operators) + // still fail the proof. assert_eq!( classify_manual_transaction_statements(Some(DatabaseType::Oracle), 1, Some("SELECT * FROM EMP")), vec![true] ); + assert_eq!( + classify_manual_transaction_statements(Some(DatabaseType::Oracle), 1, Some("SELECT LENGTH(1) FROM dual")), + vec![true] + ); + assert_eq!( + classify_manual_transaction_statements( + Some(DatabaseType::OceanbaseOracle), + 2, + Some("SELECT COUNT(*) FROM EMP; SELECT * FROM EMP@LINK") + ), + vec![true, false] + ); assert_eq!( classify_manual_transaction_statements(Some(DatabaseType::OceanbaseOracle), 1, Some("DELETE FROM EMP")), vec![false] @@ -12477,6 +12494,103 @@ for line in sys.stdin: let _ = std::fs::remove_dir_all(dir); } + /// #11788: the Oracle manual-transaction marker must follow the user's SQL + /// through the real execution path — `execute_in_manual_transaction_with_options` + /// → statement/classification pairing → per-statement marker — because that + /// marker is what the frontend aggregates into `tab.txnPossiblyDirty` and the + /// toolbar's Commit/Rollback visibility. The fake agent echoes each statement, + /// so this needs no live database. It does NOT cover the Java driver or a real + /// Oracle server, and it does not render the toolbar; the two-way real-Oracle + /// acceptance run is still outstanding. + #[tokio::test] + async fn oracle_manual_transaction_marks_only_proven_read_only_statements() { + async fn markers(state: &AppState, txn_session_id: &str, sql: &str) -> Vec { + execute_in_manual_transaction_with_options( + state, + txn_session_id, + sql, + "ORCL", + None, + ManualTransactionExecutionOptions { classification_sql: Some(sql.to_string()), ..Default::default() }, + ) + .await + .expect("statement executes through the manual transaction session") + .into_iter() + .map(|result| result.manual_transaction_proven_read_only) + .collect() + } + + let (state, txn_session_id, dir) = manual_transaction_test_state(DatabaseType::Oracle).await; + + // Proven reads: the toolbar must stay clean, including the built-in + // functions and non-call parentheses the old blanket `(` rule rejected. + for sql in [ + "SELECT LENGTH(1) FROM dual", + "SELECT COUNT(*) FROM EMP", + "SELECT UPPER(name) FROM EMP", + "SELECT NVL(name, 'x') FROM EMP", + "SELECT * FROM EMP WHERE id IN (1, 2, 3)", + "SELECT CURRENT_TIME FROM dual", + ] { + assert_eq!(markers(&state, &txn_session_id, sql).await, vec![true], "expected proven: {sql}"); + } + + // Unproven statements must still dirty the session. + for sql in [ + "SELECT my_udf(id) FROM EMP", + "SELECT DBMS_LOCK.SLEEP(1) FROM dual", + "SELECT * FROM EMP@LINK", + "SELECT seq.NEXTVAL FROM dual", + "SELECT * FROM EMP FOR UPDATE", + "UPDATE EMP SET name = 'x'", + ] { + assert_eq!(markers(&state, &txn_session_id, sql).await, vec![false], "expected unproven: {sql}"); + } + + // A mixed script is classified per statement and paired by position, so + // one unproven statement keeps the session dirty. + assert_eq!( + markers(&state, &txn_session_id, "SELECT COUNT(*) FROM EMP; SELECT * FROM EMP@LINK").await, + vec![true, false] + ); + + // The marker is the frontend contract: present in JSON only when proven. + let proven = execute_in_manual_transaction_with_options( + &state, + &txn_session_id, + "SELECT UPPER(name) FROM EMP", + "ORCL", + None, + ManualTransactionExecutionOptions { + classification_sql: Some("SELECT UPPER(name) FROM EMP".to_string()), + ..Default::default() + }, + ) + .await + .expect("proven statement executes"); + assert_eq!( + serde_json::to_value(&proven[0]).unwrap()["manual_transaction_proven_read_only"], + serde_json::json!(true) + ); + let unproven = execute_in_manual_transaction_with_options( + &state, + &txn_session_id, + "SELECT my_udf(id) FROM EMP", + "ORCL", + None, + ManualTransactionExecutionOptions { + classification_sql: Some("SELECT my_udf(id) FROM EMP".to_string()), + ..Default::default() + }, + ) + .await + .expect("unproven statement executes"); + assert!(serde_json::to_value(&unproven[0]).unwrap().get("manual_transaction_proven_read_only").is_none()); + + assert!(rollback_manual_transaction(&state, &txn_session_id).await.is_ok()); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn agent_close_query_session_params_include_session() { let params = agent_close_query_session_params("session-1"); diff --git a/crates/dbx-sql-core/src/query_execution_sql.rs b/crates/dbx-sql-core/src/query_execution_sql.rs index edbee10f5b..92488ba0a7 100644 --- a/crates/dbx-sql-core/src/query_execution_sql.rs +++ b/crates/dbx-sql-core/src/query_execution_sql.rs @@ -847,7 +847,7 @@ pub fn has_dialect_specific_write(sql: &str, database_type: DatabaseType) -> boo fn has_dialect_specific_write_statement(sql: &str, database_type: DatabaseType) -> bool { if is_mysql_compatible_database(database_type) { - let (cleaned, has_executable_comment) = strip_sql_comments_and_literals_with_metadata(sql, true); + let (cleaned, has_executable_comment) = strip_sql_comments_and_literals_with_metadata(sql, true, true); return has_executable_comment || contains_keyword_sequence(&cleaned, "INTO", "OUTFILE") || contains_keyword_sequence(&cleaned, "INTO", "DUMPFILE"); @@ -892,7 +892,7 @@ fn is_write_sql_statement( ) -> bool { // 1. Strip comments and string literals let (cleaned, has_mysql_executable_comment) = - strip_sql_comments_and_literals_with_metadata(sql, detect_mysql_executable_comments); + strip_sql_comments_and_literals_with_metadata(sql, detect_mysql_executable_comments, true); // MySQL/MariaDB executable comments may contain arbitrary SQL, including // writes that are not represented by the outer statement (for example, // INTO OUTFILE inside a SELECT). Treat them as writes rather than @@ -1008,7 +1008,10 @@ fn is_safe_read_pragma(upper_stripped: &str) -> bool { false } -fn starts_with_keyword(upper: &str, keyword: &str) -> bool { +/// True when `upper` starts with `keyword` followed by a non-alphanumeric byte +/// (or nothing). `upper` must already be ASCII-uppercased. Shared with the +/// Oracle manual-transaction proof in [`crate::sql_risk`]. +pub(crate) fn starts_with_keyword(upper: &str, keyword: &str) -> bool { upper.starts_with(keyword) && (upper.len() == keyword.len() || !upper.as_bytes()[keyword.len()].is_ascii_alphanumeric()) } @@ -1054,94 +1057,31 @@ pub fn check_read_only(sql: &str, connection_name: &str, database_type: Database Ok(()) } -/// Oracle-only, fail-closed classifier for manual-transaction UX. Returns true -/// only for an ordinary top-level read that DBX can prove harmless: a single -/// top-level `SELECT` (not `WITH`) that after Oracle-aware comment/literal -/// stripping contains no row-locking, transaction-control, DDL/DCL, `CALL`/ -/// `EXEC`, PL/SQL block, `EXECUTE IMMEDIATE`, database-link, sequence -/// pseudocolumn (`NEXTVAL`/`CURRVAL`), function-invocation, or subquery -/// syntax. Any lexical ambiguity is **not** proven read-only; the classifier -/// deliberately whitelists no functions because user/package function side -/// effects cannot be proven and would create an unbounded compatibility -/// surface. +/// Oracle-only, fail-closed classifier for manual-transaction UX: true only for +/// a top-level read DBX can prove harmless. +/// +/// This is the public Oracle entry point; the rule set lives in +/// [`crate::sql_risk::prove_read_only_for_database`] (see +/// `prove_oracle_read_only_statement` there) and combines the Oracle lexical +/// guard rails — no `WITH`, no row locking, no transaction control, no DDL/DCL, +/// no `CALL`/`EXEC`, no PL/SQL block, no `EXECUTE IMMEDIATE`, no database link +/// (`@`), no sequence pseudocolumn (`NEXTVAL`/`CURRVAL`), no `SELECT ... INTO` — +/// with an `OracleDialect` AST proof that every function call is on the reviewed +/// built-in whitelist. +/// +/// Before #11788 the guard rails included "reject any `(`", which rejected every +/// ordinary query that used a built-in function (`SELECT LENGTH(1) FROM dual`) +/// and could not tell a call from a datatype argument +/// (`CAST(a AS VARCHAR2(10))`) or an `IN (1, 2)` list. pub fn is_oracle_proven_read_only_statement(sql: &str) -> bool { - let cleaned = strip_sql_comments_and_literals(sql); - let trimmed = cleaned.trim_start(); - if trimmed.is_empty() { - return false; - } - let upper = trimmed.to_uppercase(); - // Single top-level SELECT only (WITH is excluded because data-modifying - // CTEs can hide writes in a row-returning path). - if !starts_with_keyword(&upper, "SELECT") { - return false; - } - - let lower = cleaned.to_lowercase(); - - // Function invocation or subquery syntax: any '(' after literal stripping. - // This deliberately excludes SELECT upper(name), COUNT(*), package - // functions, and subqueries DBX cannot prove harmless. - if lower.contains('(') { - return false; - } - - // Database link: '@' cannot appear in an unquoted Oracle identifier, so any - // remaining '@' after literal stripping is a remote-object reference. - if lower.contains('@') { - return false; - } - - // Word-boundary forbidden keywords. These are Oracle keywords/reserved - // words, so they cannot be legitimate unquoted identifiers inside a simple - // read; a quoted occurrence is already stripped to spaces above. - const FORBIDDEN_WORDS: &[&str] = &[ - // row locking - "for", - "update", - "lock", - // transaction control - "begin", - "commit", - "rollback", - "savepoint", - // DDL/DCL - "create", - "alter", - "drop", - "truncate", - "rename", - "grant", - "revoke", - "comment", - "purge", - "audit", - "noaudit", - "analyze", - "flashback", - "merge", - "insert", - "delete", - // set operators (a UNION is not a single top-level SELECT) - "union", - "minus", - "intersect", - // stored procedures / dynamic SQL / PL/SQL blocks - "call", - "exec", - "execute", - "declare", - // sequence pseudocolumns - "nextval", - "currval", - // SELECT ... INTO is PL/SQL-only in Oracle - "into", - ]; - - !FORBIDDEN_WORDS.iter().any(|word| contains_word(&lower, word)) + crate::sql_risk::prove_read_only_for_database(sql, DatabaseType::Oracle) + == crate::sql_risk::ReadProof::ProvenReadOnly } -fn contains_word(source: &str, word: &str) -> bool { +/// Match a whole word (`word` bounded by non-identifier bytes) in a +/// pre-lowercased, comment/literal-stripped source. Shared with the Oracle +/// manual-transaction proof in [`crate::sql_risk`]. +pub(crate) fn contains_word(source: &str, word: &str) -> bool { let bytes = source.as_bytes(); let word_bytes = word.as_bytes(); if word_bytes.is_empty() || bytes.len() < word_bytes.len() { @@ -1222,10 +1162,28 @@ pub fn strip_sql_comments(sql: &str) -> String { } pub fn strip_sql_comments_and_literals(sql: &str) -> String { - strip_sql_comments_and_literals_with_metadata(sql, false).0 + strip_sql_comments_and_literals_with_metadata(sql, false, true).0 } -fn strip_sql_comments_and_literals_with_metadata(sql: &str, detect_mysql_executable_comments: bool) -> (String, bool) { +/// [`strip_sql_comments_and_literals`] with `#` handled as a **code character** +/// instead of a line-comment start. +/// +/// `#` only starts a comment in the MySQL family; Oracle (and OceanBase-Oracle) +/// allow `#` inside unquoted identifiers, so stripping from `#` to end of line +/// hides real code from the safety scanners. `sql::SqlDialectProfile:: +/// supports_hash_line_comments` models the same distinction for statement +/// splitting. Only the Oracle manual-transaction proof opts in here; every other +/// caller keeps the MySQL-conservative default, so no other dialect's +/// classification changes. +pub(crate) fn strip_sql_comments_and_literals_keeping_hash(sql: &str) -> String { + strip_sql_comments_and_literals_with_metadata(sql, false, false).0 +} + +fn strip_sql_comments_and_literals_with_metadata( + sql: &str, + detect_mysql_executable_comments: bool, + hash_line_comments: bool, +) -> (String, bool) { let mut output = String::with_capacity(sql.len()); let mut chars = sql.chars().peekable(); let mut in_line_comment = false; @@ -1323,7 +1281,10 @@ fn strip_sql_comments_and_literals_with_metadata(sql: &str, detect_mysql_executa in_line_comment = true; continue; } - if ch == '#' { + // `#` starts a line comment only in the MySQL family; elsewhere it is a + // code character (Oracle allows it inside identifiers), so callers that + // pass `hash_line_comments = false` must keep the text visible. + if hash_line_comments && ch == '#' { in_line_comment = true; continue; } @@ -1535,19 +1496,24 @@ mod tests { #[test] fn oracle_proven_read_only_accepts_simple_top_level_selects() { - assert!(is_oracle_proven_read_only_statement("SELECT * FROM table")); - assert!(is_oracle_proven_read_only_statement("select id, name from users")); - assert!(is_oracle_proven_read_only_statement("SELECT * FROM users WHERE id = 1")); - assert!(is_oracle_proven_read_only_statement("SELECT * FROM users ORDER BY id")); - assert!(is_oracle_proven_read_only_statement("SELECT * FROM users FETCH FIRST 10 ROWS ONLY")); - assert!(is_oracle_proven_read_only_statement("SELECT 1")); - assert!(is_oracle_proven_read_only_statement("SELECT ROWNUM FROM dual")); - assert!(is_oracle_proven_read_only_statement("SELECT * FROM users")); - // Comment/literal stripping hides keywords that are not part of the SQL. - assert!(is_oracle_proven_read_only_statement("-- comment\nSELECT * FROM users")); - assert!(is_oracle_proven_read_only_statement("SELECT 'update' FROM dual")); - assert!(is_oracle_proven_read_only_statement("SELECT \"select\" FROM t")); - assert!(is_oracle_proven_read_only_statement("SELECT * FROM user_update")); + for sql in [ + "SELECT * FROM users", + "select id, name from users", + "SELECT * FROM users WHERE id = 1", + "SELECT * FROM users ORDER BY id", + "SELECT * FROM users FETCH FIRST 10 ROWS ONLY", + "SELECT * FROM users ORDER BY id OFFSET 5 ROWS FETCH NEXT 10 ROWS ONLY", + "SELECT 1", + "SELECT ROWNUM FROM dual", + "SELECT * FROM users;", + // Comment/literal stripping hides keywords that are not part of the SQL. + "-- comment\nSELECT * FROM users", + "SELECT 'update' FROM dual", + "SELECT \"select\" FROM t", + "SELECT * FROM user_update", + ] { + assert!(is_oracle_proven_read_only_statement(sql), "expected proven: {sql}"); + } } #[test] @@ -1596,28 +1562,89 @@ mod tests { } } + /// #11788: the old blanket "reject any `(`" rule rejected every ordinary + /// read that used a built-in function. Whitelisted calls, non-call + /// parentheses (`IN` lists, grouping, datatype arguments) and subqueries are + /// proven now; the AST walk is what tells a call from a datatype argument. #[test] - fn oracle_proven_read_only_rejects_functions_and_subqueries() { + fn oracle_proven_read_only_accepts_whitelisted_functions_and_parenthesised_reads() { for sql in [ - "SELECT upper(name) FROM users", - "SELECT count(*) FROM users", - "SELECT pkg.fn() FROM dual", - "SELECT * FROM users WHERE id IN (SELECT id FROM orders)", - "SELECT * FROM (SELECT * FROM users)", + "SELECT LENGTH(1) FROM dual", + "SELECT COUNT(*) FROM users", + "SELECT UPPER(name) FROM users", "SELECT NVL(name, 'x') FROM users", + "SELECT TO_CHAR(SYSDATE, 'YYYY-MM-DD') FROM dual", + "SELECT SUBSTR(name, 1, 3) || DECODE(a, 1, 'x', 'y') FROM users", + "SELECT MIN(a), MAX(a), AVG(a), SUM(a) FROM t", + "SELECT COUNT(*) FROM t GROUP BY a HAVING COUNT(*) > 1", + "SELECT * FROM t WHERE id IN (1, 2, 3)", + "SELECT (a + b) * c FROM t", + "SELECT * FROM t WHERE (a = 1 OR b = 2)", + "SELECT CAST(a AS VARCHAR2(10)) FROM t", + "SELECT CAST(a AS NUMBER(10,2)) FROM t", + "SELECT * FROM (SELECT * FROM users)", + "SELECT * FROM users WHERE id IN (SELECT id FROM orders)", + // Read-only keyword forms sqlparser models as function calls must + // stay proven, or the fix would regress them. + "SELECT CURRENT_DATE FROM dual", + "SELECT CURRENT_TIME FROM dual", + "SELECT CURRENT_TIMESTAMP FROM dual", + "SELECT SYSTIMESTAMP, USER, LEVEL FROM dual", + "SELECT LEVEL, a FROM t START WITH id = 1 CONNECT BY PRIOR id = pid", + "SELECT /*+ FULL(t) */ COUNT(*) FROM t WHERE a = :1", + ] { + assert!(is_oracle_proven_read_only_statement(sql), "expected proven: {sql}"); + } + } + + #[test] + fn oracle_proven_read_only_rejects_unknown_functions_and_qualified_calls() { + for sql in [ + // Not on the reviewed built-in whitelist. + "SELECT my_udf(a) FROM t", + "SELECT my_pkg.fn(a) FROM t", + "SELECT DBMS_LOCK.SLEEP(1) FROM dual", + "SELECT SYS_CONTEXT('USERENV', 'SESSION_USER') FROM dual", + "SELECT SYS_GUID() FROM dual", + "SELECT XMLAGG(x) FROM t", + "SELECT LISTAGG(a, ',') WITHIN GROUP (ORDER BY a) FROM t", + // Window and table functions stay Unproven by the visitor. + "SELECT ROW_NUMBER() OVER (ORDER BY a) FROM t", + "SELECT * FROM TABLE(my_func())", ] { assert!(!is_oracle_proven_read_only_statement(sql), "expected rejected: {sql}"); } } #[test] - fn oracle_proven_read_only_rejects_empty_and_ambiguous_input() { - assert!(!is_oracle_proven_read_only_statement("")); - assert!(!is_oracle_proven_read_only_statement(" ")); - assert!(!is_oracle_proven_read_only_statement("-- only a comment")); - assert!(!is_oracle_proven_read_only_statement("SELECT * FROM t WHERE a IN (1, 2)")); - assert!(!is_oracle_proven_read_only_statement("SELECT * FROM t WHERE b = 'x' AND c IN (3)")); - assert!(!is_oracle_proven_read_only_statement("SELECT 1 UNION SELECT 2")); + fn oracle_proven_read_only_rejects_empty_and_multi_statement_input() { + for sql in [ + "", + " ", + "-- only a comment", + "SELECT 1 UNION SELECT 2", + "SELECT 1; SELECT 2", + "SELECT a FROM t; SELECT b FROM u", + // Parse failure is judged by the lexical guard rails alone. Call-shaped + // syntax stays Unproven because no AST can vouch for the call. + "SELECT * FROM t AS OF TIMESTAMP TO_TIMESTAMP('2020-01-01', 'YYYY-MM-DD')", + ] { + assert!(!is_oracle_proven_read_only_statement(sql), "expected rejected: {sql}"); + } + } + + /// Oracle-only syntax sqlparser cannot parse keeps the verdict it had before + /// #11788 (no `(`, no `@`, no forbidden word) instead of losing the bit: the + /// pre-change classifier never parsed the statement either. + #[test] + fn oracle_proven_read_only_keeps_pre_change_verdict_for_unparseable_oracle_syntax() { + for sql in [ + "SELECT * FROM t AS OF TIMESTAMP SYSTIMESTAMP - INTERVAL '1' HOUR", + "SELECT * FROM t AS OF SCN 12345", + "SELECT * FROM t VERSIONS BETWEEN TIMESTAMP SYSTIMESTAMP - INTERVAL '1' HOUR AND SYSTIMESTAMP", + ] { + assert!(is_oracle_proven_read_only_statement(sql), "expected proven: {sql}"); + } } #[test] diff --git a/crates/dbx-sql-core/src/sql_risk.rs b/crates/dbx-sql-core/src/sql_risk.rs index 34718fa6ee..e496ab03b7 100644 --- a/crates/dbx-sql-core/src/sql_risk.rs +++ b/crates/dbx-sql-core/src/sql_risk.rs @@ -6,7 +6,8 @@ use sqlparser::ast::{ Statement, TableFactor, UnaryOperator, Value, Visit, Visitor, }; use sqlparser::dialect::{ - ClickHouseDialect, DuckDbDialect, GenericDialect, MsSqlDialect, MySqlDialect, PostgreSqlDialect, SQLiteDialect, + ClickHouseDialect, DuckDbDialect, GenericDialect, MsSqlDialect, MySqlDialect, OracleDialect, PostgreSqlDialect, + SQLiteDialect, }; use sqlparser::parser::Parser; use sqlparser::tokenizer::{Token, Tokenizer}; @@ -1132,22 +1133,232 @@ const POSTGRES_PROOF_SAFE_FUNCTIONS: &[&str] = &[ "version", ]; +/// Pure built-in functions accepted by the Oracle proof. Same fail-closed +/// contract as `MYSQL_PROOF_SAFE_FUNCTIONS`: a call to anything not listed is +/// Unproven. Only ever grows by review. +/// +/// `current_date`, `current_time`, `current_timestamp`, `localtime` and +/// `localtimestamp` are listed even though Oracle writes them without +/// parentheses: sqlparser routes exactly these five keywords through +/// `parse_time_functions` (`parser/mod.rs`), so they arrive as function calls. +/// Omitting one would reject statements the previous lexical classifier still +/// proved (`SELECT CURRENT_TIME FROM dual`). `sysdate`/`systimestamp`/`rownum`/ +/// `level`/`user` parse as ordinary expressions and need no entry — as do +/// `current_user`/`session_user`, which sqlparser only models as calls for +/// PostgreSQL/Generic. +/// +/// Deliberately absent: package or schema-qualified calls (`dbms_*`, `utl_*`, +/// `pkg.fn`), sequence pseudocolumns, `sys_context`, `sys_guid`, `xmlagg` and +/// ordered-set aggregates (`listagg ... WITHIN GROUP`) — each stays Unproven +/// until reviewed. +const ORACLE_PROOF_SAFE_FUNCTIONS: &[&str] = &[ + "abs", + "add_months", + "ascii", + "avg", + "bitand", + "ceil", + "chr", + "coalesce", + "concat", + "count", + "current_date", + "current_time", + "current_timestamp", + "decode", + "exp", + "floor", + "greatest", + "initcap", + "instr", + "last_day", + "least", + "length", + "lengthb", + "ln", + "localtime", + "localtimestamp", + "log", + "lower", + "lpad", + "ltrim", + "max", + "min", + "mod", + "months_between", + "next_day", + "nullif", + "nvl", + "nvl2", + "power", + "remainder", + "replace", + "round", + "rpad", + "rtrim", + "sign", + "sqrt", + "stddev", + "substr", + "substrb", + "sum", + "to_char", + "to_date", + "to_number", + "to_timestamp", + "translate", + "trim", + "trunc", + "upper", + "variance", +]; + /// Prove that a single SQL statement is, by DBX's strict heuristic, an -/// ordinary read. Only MySQL and the PostgreSQL family (PostgreSQL, GaussDB, -/// openGauss) take part in this proof; every other database type is Unproven so -/// its manual-transaction toolbar keeps the legacy behavior. Oracle keeps its -/// own lexical classifier (`is_oracle_proven_read_only_statement`) — do not -/// reroute it here. +/// ordinary read. MySQL, the PostgreSQL family (PostgreSQL, GaussDB, openGauss) +/// and Oracle/OceanBase-Oracle take part in this proof; every other database +/// type is Unproven so its manual-transaction toolbar keeps the legacy behavior. +/// +/// Oracle used to keep a purely lexical classifier in `query_execution_sql` +/// that rejected *any* `(` after comment/literal stripping, which is why +/// `SELECT LENGTH(1) FROM dual` showed commit/rollback buttons (#11788). It now +/// shares this proof's mechanism, keeping the Oracle-specific lexical guard +/// rails in `prove_oracle_read_only_statement` — see the comment there for why +/// those guards are load-bearing and cannot be replaced by the AST walk. pub fn prove_read_only_for_database(sql: &str, database_type: DatabaseType) -> ReadProof { match database_type { DatabaseType::Mysql => prove_read_only_statement(sql, "mysql", MYSQL_PROOF_SAFE_FUNCTIONS), DatabaseType::Postgres | DatabaseType::Gaussdb | DatabaseType::OpenGauss => { prove_read_only_statement(sql, "postgres", POSTGRES_PROOF_SAFE_FUNCTIONS) } + DatabaseType::Oracle | DatabaseType::OceanbaseOracle => prove_oracle_read_only_statement(sql), _ => ReadProof::Unproven, } } +/// Oracle keywords/reserved words that make a statement unprovable. These are +/// Oracle keywords, so they cannot be legitimate unquoted identifiers inside a +/// simple read; a quoted occurrence is already stripped to spaces first. +/// +/// Most entries are redundant now that the statement is parsed (a write parses +/// as a non-`Query` statement and `query_is_write_capable` sees locking clauses +/// and `SELECT ... INTO`), but they are kept verbatim: dropping one would widen +/// what DBX calls a proven read, which is a deliberate decision per keyword, not +/// a side effect of moving to the AST. +const ORACLE_PROOF_FORBIDDEN_WORDS: &[&str] = &[ + // row locking + "for", + "update", + "lock", + // transaction control + "begin", + "commit", + "rollback", + "savepoint", + // DDL/DCL + "create", + "alter", + "drop", + "truncate", + "rename", + "grant", + "revoke", + "comment", + "purge", + "audit", + "noaudit", + "analyze", + "flashback", + "merge", + "insert", + "delete", + // set operators (a UNION is not a single top-level SELECT) + "union", + "minus", + "intersect", + // stored procedures / dynamic SQL / PL/SQL blocks + "call", + "exec", + "execute", + "declare", + // sequence pseudocolumns + "nextval", + "currval", + // SELECT ... INTO is PL/SQL-only in Oracle + "into", +]; + +/// Oracle has no keyword fallback either: the lexical guard rails below must +/// hold *and* the statement must parse as exactly one `Statement::Query` under +/// `OracleDialect`, with no disallowed function call. A statement that does not +/// parse at all is judged by the guard rails alone (see the parse-failure branch). +/// +/// The guard rails are load-bearing, not belt-and-braces: sqlparser happily +/// parses `SELECT * FROM t@dblink` and `SELECT seq.NEXTVAL FROM dual` as plain +/// queries, so without them a remote read and a sequence bump would be stamped +/// as proven read-only. +/// +/// The dialect is resolved here rather than through `resolve_dialect`: the +/// shared resolver also feeds `classify_sql_risk_for_database`, +/// `is_dangerous_sql_for_database` and the MCP `USE` check, and changing Oracle's +/// dialect there would move Oracle's risk tiers — out of scope for a +/// toolbar-metadata fix. `OracleDialect` (not `GenericDialect`, today's fallback) +/// is what parses `(+)` outer joins and the rest of the Oracle corpus. +fn prove_oracle_read_only_statement(sql: &str) -> ReadProof { + use crate::query_execution_sql::{ + contains_word, starts_with_keyword, strip_sql_comments_and_literals_keeping_hash, + }; + + // Oracle's own comment rules: `#` is an identifier character, not a comment + // start (mysql-only). With the shared `#`-blind stripper, + // `SELECT a#b FROM t@dblink` lost everything after the `#` and was stamped + // proven — the AST cannot catch it, because `t@dblink` parses as an ordinary + // table reference. + let cleaned = strip_sql_comments_and_literals_keeping_hash(sql); + let trimmed = cleaned.trim_start(); + if trimmed.is_empty() { + return ReadProof::Unproven; + } + // Single top-level SELECT only (WITH is excluded because data-modifying CTEs + // can hide writes in a row-returning path). + if !starts_with_keyword(&trimmed.to_uppercase(), "SELECT") { + return ReadProof::Unproven; + } + // Multi-statement text is Unproven on both paths below, so the parse-failure + // fallback cannot prove a whole script from its first statement. The count + // comes from the dialect-aware splitter, not from + // `has_extra_statement_after_semicolon`: the latter's lexer treats `#` as a + // line comment (the MySQL rule) while Oracle allows `#` inside identifiers, + // so a `#` would hide a following statement from it. The splitter models that + // per dialect (`sql::SqlDialectProfile::supports_hash_line_comments`). + if crate::sql::split_sql_statements_for_database(sql, DatabaseType::Oracle).len() != 1 { + return ReadProof::Unproven; + } + let lower = cleaned.to_lowercase(); + // Database link: '@' cannot appear in an unquoted Oracle identifier, so any + // remaining '@' after literal stripping is a remote-object reference. + if lower.contains('@') { + return ReadProof::Unproven; + } + if ORACLE_PROOF_FORBIDDEN_WORDS.iter().any(|word| contains_word(&lower, word)) { + return ReadProof::Unproven; + } + + let Ok(statements) = Parser::parse_sql(&OracleDialect {}, sql) else { + // Oracle-only syntax the parser cannot handle (`AS OF TIMESTAMP` / + // `AS OF SCN` flashback reads, `VERSIONS BETWEEN`) must not lose a + // capability it had before #11788. With no AST there is no function list + // to inspect, so fall back to the pre-#11788 rule: proven only while + // nothing call-shaped is visible at all. This never proves more than the + // old classifier did, and it is the only reason a statement without an + // AST can still be proven. + return if lower.contains('(') { ReadProof::Unproven } else { ReadProof::ProvenReadOnly }; + }; + let [statement] = statements.as_slice() else { + return ReadProof::Unproven; + }; + prove_statement(statement, ORACLE_PROOF_SAFE_FUNCTIONS) +} + fn prove_read_only_statement(sql: &str, dialect: &str, allowed_functions: &[&str]) -> ReadProof { let database_type = if dialect == "mysql" { DatabaseType::Mysql } else { DatabaseType::Postgres }; // Lexical rejections that must not depend on parser support: dialect-specific @@ -1397,13 +1608,156 @@ mod tests { #[test] fn proof_only_applies_to_enabled_dialects() { - // Oracle keeps its own lexical classifier; the gate in query.rs must - // never route it through the generic proof. - assert_eq!(prove_read_only_for_database("SELECT 1", DatabaseType::Oracle), ReadProof::Unproven); + // Oracle/OceanBase-Oracle share this proof since #11788; their + // Oracle-specific guard rails live in `prove_oracle_read_only_statement`. + assert_eq!(prove_read_only_for_database("SELECT 1", DatabaseType::Oracle), ReadProof::ProvenReadOnly); + assert_eq!( + prove_read_only_for_database("SELECT LENGTH(1) FROM dual", DatabaseType::OceanbaseOracle), + ReadProof::ProvenReadOnly + ); + assert_eq!( + prove_read_only_for_database("SELECT * FROM t@dblink", DatabaseType::OceanbaseOracle), + ReadProof::Unproven + ); // Family members without manual-transaction UI stay unproven in v1. assert_eq!(prove_read_only_for_database("SELECT 1", DatabaseType::Doris), ReadProof::Unproven); } + /// The Oracle guard rails are not belt-and-braces: each statement below + /// parses as a single ordinary `Query` under `OracleDialect`, so the AST + /// walk alone would call it ProvenReadOnly. Removing a guard rail (the `@` + /// scan, the word-boundary keyword scan) silently widens what DBX calls a + /// proven read, which is why they are asserted here rather than only through + /// the public classifier. + #[test] + fn oracle_proof_keeps_the_guard_rails_the_ast_cannot_see() { + for sql in [ + // Remote objects: '@' cannot appear in an unquoted Oracle identifier. + "SELECT * FROM t@dblink", + "SELECT * FROM DUAL@dblink", + // Sequence pseudocolumns: parsed as a compound identifier, not a call. + "SELECT seq.NEXTVAL FROM dual", + "SELECT seq.CURRVAL FROM dual", + // Retained restrictions that the AST itself would accept. + "WITH cte AS (SELECT 1 AS a FROM dual) SELECT * FROM cte", + "SELECT 1 FROM dual UNION SELECT 2 FROM dual", + "SELECT 1 FROM dual MINUS SELECT 2 FROM dual", + // A write hidden behind a row-returning shape. + "SELECT * FROM t FOR UPDATE", + "SELECT COUNT(*) INTO v FROM t", + "SELECT 1; SELECT 2", + ] { + assert_eq!( + prove_read_only_for_database(sql, DatabaseType::Oracle), + ReadProof::Unproven, + "expected unproven: {sql}" + ); + } + } + + #[test] + fn oracle_proof_accepts_reviewed_builtins_only() { + for sql in [ + "SELECT LENGTH(1) FROM dual", + "SELECT COUNT(*) FROM users", + "SELECT UPPER(name) FROM users", + "SELECT NVL(name, 'x') FROM users", + "SELECT TO_CHAR(SYSDATE, 'YYYY-MM-DD') FROM dual", + "SELECT * FROM t WHERE id IN (1, 2, 3)", + "SELECT * FROM t WHERE (a = 1 OR b = 2)", + "SELECT CAST(a AS VARCHAR2(10)) FROM t", + "SELECT * FROM (SELECT * FROM users)", + // No-paren forms that must not regress: SYSDATE/ROWNUM/LEVEL parse as + // ordinary expressions, while sqlparser models these five as calls. + "SELECT SYSDATE, ROWNUM, USER FROM dual", + "SELECT CURRENT_DATE FROM dual", + "SELECT CURRENT_TIME FROM dual", + "SELECT CURRENT_TIMESTAMP FROM dual", + "SELECT LOCALTIME FROM dual", + "SELECT LOCALTIMESTAMP FROM dual", + // Precision form: sqlparser models it as a call too. + "SELECT CURRENT_TIME(3) FROM dual", + // Parser-invisible Oracle syntax keeps the pre-#11788 verdict via the + // parse-failure fallback (it contains no call-shaped syntax). + "SELECT * FROM t AS OF TIMESTAMP SYSTIMESTAMP - INTERVAL '1' HOUR", + "SELECT * FROM t VERSIONS BETWEEN TIMESTAMP SYSTIMESTAMP - INTERVAL '1' HOUR AND SYSTIMESTAMP", + ] { + for database_type in [DatabaseType::Oracle, DatabaseType::OceanbaseOracle] { + assert_eq!( + prove_read_only_for_database(sql, database_type), + ReadProof::ProvenReadOnly, + "expected proven for {database_type:?}: {sql}" + ); + } + } + + for sql in [ + "SELECT my_udf(a) FROM t", + "SELECT my_pkg.fn(a) FROM t", + "SELECT SYS_CONTEXT('USERENV', 'SESSION_USER') FROM dual", + "SELECT SYS_GUID() FROM dual", + "SELECT LISTAGG(a, ',') WITHIN GROUP (ORDER BY a) FROM t", + "SELECT ROW_NUMBER() OVER (ORDER BY a) FROM t", + "SELECT * FROM TABLE(my_func())", + ] { + assert_eq!( + prove_read_only_for_database(sql, DatabaseType::Oracle), + ReadProof::Unproven, + "expected unproven: {sql}" + ); + } + } + + /// `#` is inside Oracle identifiers, so a statement hidden behind one must not + /// become provable. Two rails are needed: the shared stripper used to treat + /// `#` as a line comment (mysql-only rule) and hide the rest of the line, so + /// the proof strips with `strip_sql_comments_and_literals_keeping_hash`, and + /// the multi-statement rail uses the dialect-aware splitter. Without the first + /// one `SELECT a#b FROM t@dblink` was proven (the AST cannot catch a database + /// link — `t@dblink` parses as an ordinary table reference); without the + /// second, `SELECT a#b; DELETE FROM t` was proven. The pre-#11788 classifier + /// proved all of these. + #[test] + fn oracle_proof_rejects_statements_hidden_behind_a_hash_identifier() { + for sql in [ + // `@` database link hidden behind `#` + "SELECT a#b FROM t@link", + "SELECT a#b FROM dual@dblink", + // multi-statement, where the dialect-aware splitter is the rail + "SELECT a#b; EXECUTE IMMEDIATE 'DROP TABLE t'", + "SELECT a#b; DELETE FROM t", + "SELECT 1 #; COMMIT", + "SELECT 'x' # y; DELETE FROM t", + "SELECT a#b WHERE 1 = 1; BEGIN NULL; END;", + "SELECT a#b; BEGIN DELETE FROM t; END;", + // the AST covers these even with the `#` text visible + "SELECT a#b FROM t WHERE x = 1 FOR UPDATE", + "SELECT a#b INTO v FROM t", + // Deliberate conservative outcome: with `#` visible, an identifier + // whose `#`-suffix spells a forbidden word no longer matches as an + // identifier boundary, so the statement is Unproven rather than + // proven. Safe direction; the statement is unusual either way. + "SELECT x#for FROM t", + ] { + assert_eq!( + prove_read_only_for_database(sql, DatabaseType::Oracle), + ReadProof::Unproven, + "expected unproven: {sql}" + ); + } + // Single statements with `#` in an identifier stay readable, exactly as + // the splitter sees them. + assert_eq!( + prove_read_only_for_database("SELECT a#b FROM t WHERE c = 1", DatabaseType::Oracle), + ReadProof::ProvenReadOnly + ); + assert_eq!(prove_read_only_for_database("SELECT * FROM t#1", DatabaseType::Oracle), ReadProof::ProvenReadOnly); + assert_eq!( + prove_read_only_for_database("SELECT COUNT(*) FROM emp#1", DatabaseType::Oracle), + ReadProof::ProvenReadOnly + ); + } + #[test] fn classify_select_statements() { assert_eq!(classify_sql_risk("SELECT * FROM users", "postgres").unwrap(), SqlRisk::ReadOnly);