From 1497c0a697d07441c45850fa4f55ad3b1dd9d1b1 Mon Sep 17 00:00:00 2001 From: Anderson Leal Date: Wed, 20 May 2026 11:00:37 -0300 Subject: [PATCH 1/2] fix(iii-database): close interactive-transaction side-channel finalization bypasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `transactionExecute` / `transactionQuery` handlers held a server-side `BEGIN ... COMMIT` open under a UUID and routed caller-supplied SQL onto that pinned connection. Three ways the caller could finalize the txn out-of-band, desynchronizing `TxRegistry` from the conn's real txn state: 1. `is_transaction_control_sql` only inspected the first whitespace- delimited token. `/* */COMMIT` and `--\nCOMMIT` both reached the driver because PG/MySQL strip comments server-side before parsing. 2. `transactionQuery` had no `is_transaction_control_sql` guard at all — PG/MySQL/SQLite all execute `COMMIT`/`ROLLBACK`/`BEGIN` through the prepared-statement path that `run_prepared` uses. 3. The keyword set missed `START TRANSACTION` (ANSI / MySQL spelling of `BEGIN`). On MySQL, `START TRANSACTION` inside an existing txn implicitly commits the outer one and opens a new (untracked) one. Fix: - Move `is_transaction_control_sql` into a dedicated `handlers::tx_sql_guard` module so both handlers share one defense. - Strip leading whitespace, `;`, line comments (`-- ...\n`), and block comments (`/* ... */`, postgres-style nested) *before* tokenizing. The strip is iterative — `/* a */ /* b */COMMIT` is caught too. Byte scanning is sound because the comment markers are pure ASCII. - Add the two-token `START TRANSACTION` form (alongside `SET TRANSACTION`). `START SLAVE` / `START REPLICA` correctly pass through. - Wire the guard into `transactionQuery::handle`, mirroring the existing log event and `INVALID_PARAM` error shape used by `transactionExecute`. Tests: - 11 unit tests in `tx_sql_guard.rs` covering plain keywords, comment prefixes (block / line / chained / nested), START TRANSACTION variants, comment-only / unterminated inputs, comment-prefixed DML pass-through, and UTF-8 safety. - 6 new handler-level rejection tests (3 in each handler) asserting the rejection fires before the registry is touched and the error names the right finalization handler. - 179 total iii-database lib tests pass. --- iii-database/src/handlers/mod.rs | 1 + .../src/handlers/transaction_execute.rs | 146 ++++++---- .../src/handlers/transaction_query.rs | 120 +++++++- iii-database/src/handlers/tx_sql_guard.rs | 258 ++++++++++++++++++ 4 files changed, 465 insertions(+), 60 deletions(-) create mode 100644 iii-database/src/handlers/tx_sql_guard.rs diff --git a/iii-database/src/handlers/mod.rs b/iii-database/src/handlers/mod.rs index 9fd587e0..53d9a5d6 100644 --- a/iii-database/src/handlers/mod.rs +++ b/iii-database/src/handlers/mod.rs @@ -20,6 +20,7 @@ pub mod run_statement; pub mod transaction; pub mod transaction_execute; pub mod transaction_query; +mod tx_sql_guard; pub(crate) use query::rows_to_objects as query_rows_to_objects; diff --git a/iii-database/src/handlers/transaction_execute.rs b/iii-database/src/handlers/transaction_execute.rs index f73a8191..dbd351e3 100644 --- a/iii-database/src/handlers/transaction_execute.rs +++ b/iii-database/src/handlers/transaction_execute.rs @@ -1,10 +1,14 @@ //! `iii-database::transactionExecute` — write SQL inside an interactive //! transaction. Same response envelope as `iii-database::execute`. Bare //! `BEGIN` / `COMMIT` / `ROLLBACK` / `END` / `SAVEPOINT` / `RELEASE` / -//! `SET TRANSACTION` statements are rejected with `INVALID_PARAM` so -//! callers cannot side-channel finalization through SQL — they must use -//! the dedicated `commitTransaction` / `rollbackTransaction` handlers. +//! `SET TRANSACTION` / `START TRANSACTION` statements are rejected with +//! `INVALID_PARAM` so callers cannot side-channel finalization through SQL — +//! they must use the dedicated `commitTransaction` / `rollbackTransaction` +//! handlers. The detection runs *after* stripping leading SQL comments and +//! `;`, so `/* */COMMIT` and `--\nCOMMIT` are rejected too. See +//! [`super::tx_sql_guard`] for the full coverage and rationale. +use super::tx_sql_guard::is_transaction_control_sql; use super::AppState; use crate::driver; use crate::error::DbError; @@ -33,34 +37,6 @@ pub struct TxExecuteResp { pub returned_rows: Vec>, } -/// First-token check for transaction-control SQL. We compare against the -/// uppercased trimmed first whitespace-delimited token (with any trailing -/// `;` stripped); this matches `BEGIN`, `COMMIT`, `ROLLBACK`, `END`, -/// `SAVEPOINT`, `RELEASE`, and `SET TRANSACTION ...` regardless of -/// surrounding whitespace and trailing arguments. Bare `SET` (not -/// `SET TRANSACTION`) is left alone — callers may legitimately set session -/// variables inside a transaction. -fn is_transaction_control_sql(sql: &str) -> bool { - let trimmed = sql.trim_start_matches(|c: char| c.is_whitespace() || c == ';'); - let mut tokens = trimmed.split_whitespace(); - let Some(first) = tokens.next() else { - return false; - }; - // `COMMIT;` / `end;` / `ROLLBACK ;` all reach here as the first token - // including the trailing `;`. Strip it before matching so the same - // keyword set covers terminated and unterminated forms. - let first = first.trim_end_matches(';'); - let upper = first.to_ascii_uppercase(); - match upper.as_str() { - "BEGIN" | "COMMIT" | "ROLLBACK" | "END" | "SAVEPOINT" | "RELEASE" => true, - "SET" => tokens - .next() - .map(|t| t.eq_ignore_ascii_case("TRANSACTION")) - .unwrap_or(false), - _ => false, - } -} - pub async fn handle(state: &AppState, req: TxExecuteReq) -> Result { if req.sql.trim().is_empty() { return Err(err_to_str(DbError::DriverError { @@ -83,7 +59,8 @@ pub async fn handle(state: &AppState, req: TxExecuteReq) -> Result Result { if req.sql.trim().is_empty() { - return Err(err_to_str(crate::error::DbError::DriverError { + return Err(err_to_str(DbError::DriverError { driver: "(tx)".into(), code: None, message: "empty SQL".into(), failed_index: None, })); } + if is_transaction_control_sql(&req.sql) { + // Mirror the transactionExecute rejection. The defense is symmetric: + // PG/MySQL/SQLite all execute COMMIT/ROLLBACK/etc. through the + // prepared-statement path that `run_prepared` uses, so without this + // guard the query handler is the easiest finalization side-channel. + state.log.warn( + "db_tx_finalization_via_sql_rejected", + Some(json!({ + "db.transaction.id": req.transaction_id, + "db.operation": "QUERY", + "db.statement": req.sql.trim(), + })), + ); + return Err(err_to_str(DbError::InvalidParam { + index: 0, + reason: "transaction-control SQL (BEGIN/START TRANSACTION/COMMIT/ROLLBACK/\ + SAVEPOINT/RELEASE/END/SET TRANSACTION, including comment-prefixed forms) \ + is not allowed in transactionQuery; use commitTransaction or \ + rollbackTransaction" + .into(), + })); + } let params = JsonParam::from_json_slice(&req.params).map_err(err_to_str)?; let lock = match state.transactions.lock(&req.transaction_id).await { @@ -167,4 +199,90 @@ mod tests { .unwrap_err(); assert!(err.contains("empty SQL"), "got: {err}"); } + + /// transactionQuery must reject bare COMMIT/ROLLBACK/etc. with the same + /// `INVALID_PARAM` rationale as transactionExecute. Without this, a + /// caller can finalize the txn through the query handler — the registry + /// keeps the entry around and subsequent calls run on a now-non-txn conn + /// until the timeout watcher sweeps. + #[tokio::test(flavor = "multi_thread")] + async fn rejects_commit_via_query() { + let st = state(); + let begin = crate::handlers::begin_transaction::handle( + &st, + serde_json::from_value(json!({ "db": "primary" })).unwrap(), + ) + .await + .unwrap(); + let err = handle( + &st, + req(json!({ "transaction_id": begin.transaction.id, "sql": "COMMIT" })), + ) + .await + .unwrap_err(); + assert!(err.contains("INVALID_PARAM"), "got: {err}"); + assert!(err.contains("commitTransaction"), "got: {err}"); + } + + #[tokio::test(flavor = "multi_thread")] + async fn rejects_rollback_via_query() { + let st = state(); + let begin = crate::handlers::begin_transaction::handle( + &st, + serde_json::from_value(json!({ "db": "primary" })).unwrap(), + ) + .await + .unwrap(); + let err = handle( + &st, + req(json!({ "transaction_id": begin.transaction.id, "sql": "ROLLBACK" })), + ) + .await + .unwrap_err(); + assert!(err.contains("INVALID_PARAM"), "got: {err}"); + assert!(err.contains("rollbackTransaction"), "got: {err}"); + } + + /// Comment-prefixed bypass via the query path. + #[tokio::test(flavor = "multi_thread")] + async fn rejects_block_comment_prefixed_commit_via_query() { + let st = state(); + let begin = crate::handlers::begin_transaction::handle( + &st, + serde_json::from_value(json!({ "db": "primary" })).unwrap(), + ) + .await + .unwrap(); + let err = handle( + &st, + req(json!({ + "transaction_id": begin.transaction.id, + "sql": "/* sneak */COMMIT", + })), + ) + .await + .unwrap_err(); + assert!(err.contains("INVALID_PARAM"), "got: {err}"); + } + + #[tokio::test(flavor = "multi_thread")] + async fn rejects_start_transaction_via_query() { + let st = state(); + let begin = crate::handlers::begin_transaction::handle( + &st, + serde_json::from_value(json!({ "db": "primary" })).unwrap(), + ) + .await + .unwrap(); + let err = handle( + &st, + req(json!({ + "transaction_id": begin.transaction.id, + "sql": "START TRANSACTION", + })), + ) + .await + .unwrap_err(); + assert!(err.contains("INVALID_PARAM"), "got: {err}"); + } } diff --git a/iii-database/src/handlers/tx_sql_guard.rs b/iii-database/src/handlers/tx_sql_guard.rs new file mode 100644 index 00000000..4b2b4614 --- /dev/null +++ b/iii-database/src/handlers/tx_sql_guard.rs @@ -0,0 +1,258 @@ +//! First-line defense against side-channel transaction finalization. +//! +//! `transactionExecute` and `transactionQuery` accept caller-supplied SQL and +//! run it on a connection that the worker holds inside `BEGIN ... COMMIT`. +//! If a caller submits raw `COMMIT` / `ROLLBACK` / `BEGIN` / `SAVEPOINT` / +//! `SET TRANSACTION` / `START TRANSACTION`, the driver executes it server- +//! side — finalizing or restarting the transaction without the `TxRegistry` +//! ever finding out. The next handler call then operates on a pinned conn +//! whose txn state no longer matches the registry's view, silently breaking +//! the atomicity contract. +//! +//! [`is_transaction_control_sql`] returns `true` for any SQL whose first +//! non-comment token is a transaction-control keyword, letting handlers +//! reject with `INVALID_PARAM` before the registry lookup. +//! +//! Coverage: +//! - Plain keyword forms: `BEGIN`, `COMMIT`, `ROLLBACK`, `END`, +//! `SAVEPOINT`, `RELEASE`, two-token forms `SET TRANSACTION ...` and +//! `START TRANSACTION ...` (ANSI / MySQL spelling of `BEGIN`). +//! - Comment-prefixed forms — `/* */COMMIT`, `-- foo\nCOMMIT`, +//! `/* a */ /* b */COMMIT`, and any chain of leading whitespace, `;`, +//! line comments, and block comments (including nested block comments +//! for postgres parity). +//! +//! Why nested block comments: postgres parses `/* /* nest */ */` as a single +//! comment. mysql and sqlite don't, but if we over-strip on those drivers +//! the worst case is a false-positive rejection — far better than letting a +//! finalization SQL through on postgres. + +/// Returns `true` if the first non-comment, non-whitespace, non-`;` token +/// of `sql` is a transaction-control keyword. The two-token forms +/// `SET TRANSACTION ...` and `START TRANSACTION ...` are matched only when +/// the second token is exactly `TRANSACTION` (case-insensitive) — bare +/// `SET LOCAL ...` (legitimate per-tx session settings) and MySQL +/// replication's `START SLAVE` / `START REPLICA` are left alone. +pub(super) fn is_transaction_control_sql(sql: &str) -> bool { + let trimmed = strip_leading_noise(sql); + let mut tokens = trimmed.split_whitespace(); + let Some(first) = tokens.next() else { + return false; + }; + // `COMMIT;` / `end;` reach here including the trailing `;`. Strip it so + // terminated and unterminated forms share one branch. + let first = first.trim_end_matches(';'); + let upper = first.to_ascii_uppercase(); + match upper.as_str() { + "BEGIN" | "COMMIT" | "ROLLBACK" | "END" | "SAVEPOINT" | "RELEASE" => true, + "SET" | "START" => tokens + .next() + .map(|t| t.trim_end_matches(';').eq_ignore_ascii_case("TRANSACTION")) + .unwrap_or(false), + _ => false, + } +} + +/// Strip any prefix of leading whitespace, `;`, line comments (`-- ...\n`), +/// and block comments (`/* ... */`, with nesting supported) so the returned +/// slice begins at the first character of the first real token. Returns an +/// empty slice when the input is comment-only or contains an unterminated +/// comment — the caller treats both as "no command present". +fn strip_leading_noise(sql: &str) -> &str { + let mut s = sql; + loop { + let trimmed = s.trim_start_matches(|c: char| c.is_whitespace() || c == ';'); + if let Some(after) = trimmed.strip_prefix("--") { + // Line comment ends at the next newline; if no newline exists, + // the comment swallows the rest of the input. + match after.find('\n') { + Some(pos) => s = &after[pos + 1..], + None => return "", + } + } else if let Some(after) = trimmed.strip_prefix("/*") { + // Block comment with postgres-style nesting. If the comment is + // unterminated, consume the rest — the driver would error on it + // regardless, and we'd rather over-strip than risk a bypass. + match strip_block_comment_body(after) { + Some(rest) => s = rest, + None => return "", + } + } else { + return trimmed; + } + } +} + +/// Consume the body of a block comment whose opening `/*` was already +/// stripped, returning the remainder of the input after the matching `*/`. +/// Handles nested `/* /* */ */`. Returns `None` when unterminated. +/// +/// Byte-level scanning is sound here because `/` (0x2F) and `*` (0x2A) are +/// both ASCII and never appear as continuation bytes in valid UTF-8 +/// (continuation bytes are 0x80..0xBF). A returned slice always begins +/// immediately after the matched `*/` — i.e. on a char boundary — because +/// `/` is a 1-byte ASCII char. +fn strip_block_comment_body(s: &str) -> Option<&str> { + let bytes = s.as_bytes(); + let mut depth: usize = 1; + let mut i = 0; + while i + 1 < bytes.len() { + if bytes[i] == b'/' && bytes[i + 1] == b'*' { + depth += 1; + i += 2; + } else if bytes[i] == b'*' && bytes[i + 1] == b'/' { + depth -= 1; + i += 2; + if depth == 0 { + return Some(&s[i..]); + } + } else { + i += 1; + } + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + // --- Existing keyword coverage (no regression) ------------------------- + + #[test] + fn flags_plain_finalization_keywords() { + assert!(is_transaction_control_sql("COMMIT")); + assert!(is_transaction_control_sql(" rollback ")); + assert!(is_transaction_control_sql("ROLLBACK TO SAVEPOINT foo")); + assert!(is_transaction_control_sql( + "BEGIN ISOLATION LEVEL SERIALIZABLE" + )); + assert!(is_transaction_control_sql("end;")); + assert!(is_transaction_control_sql("SAVEPOINT s1")); + assert!(is_transaction_control_sql("RELEASE SAVEPOINT s1")); + assert!(is_transaction_control_sql("SET TRANSACTION READ ONLY")); + assert!(is_transaction_control_sql( + "set transaction isolation level serializable" + )); + } + + #[test] + fn passes_normal_dml() { + assert!(!is_transaction_control_sql("INSERT INTO t (n) VALUES (1)")); + assert!(!is_transaction_control_sql("UPDATE t SET n = 2")); + assert!(!is_transaction_control_sql("DELETE FROM t")); + assert!(!is_transaction_control_sql("SELECT 1")); + // Bare `SET` for per-tx session settings — second token isn't TRANSACTION. + assert!(!is_transaction_control_sql("SET LOCAL search_path = foo")); + assert!(!is_transaction_control_sql("SET search_path TO foo")); + } + + // --- Finding #3: START TRANSACTION (MySQL/ANSI spelling) --------------- + + #[test] + fn flags_start_transaction() { + assert!(is_transaction_control_sql("START TRANSACTION")); + assert!(is_transaction_control_sql("start transaction")); + assert!(is_transaction_control_sql( + "START TRANSACTION ISOLATION LEVEL SERIALIZABLE" + )); + assert!(is_transaction_control_sql("start transaction;")); + assert!(is_transaction_control_sql("START\tTRANSACTION")); + } + + #[test] + fn passes_start_other_than_transaction() { + // MySQL replication ops use START with a different second token — + // not finalization, must pass through. + assert!(!is_transaction_control_sql("START SLAVE")); + assert!(!is_transaction_control_sql("START REPLICA SQL_THREAD")); + // Bare `START` with nothing after isn't a real statement, but it + // also isn't transaction-control. + assert!(!is_transaction_control_sql("START")); + } + + // --- Finding #1: comment-prefixed bypass -------------------------------- + + #[test] + fn flags_block_comment_prefixed_finalization() { + assert!(is_transaction_control_sql("/* sneak */COMMIT")); + assert!(is_transaction_control_sql("/* sneak */ COMMIT")); + assert!(is_transaction_control_sql("/**/ COMMIT")); + assert!(is_transaction_control_sql("/*\n multi\n line\n*/ ROLLBACK")); + // Chained block comments. + assert!(is_transaction_control_sql("/* a *//* b */ COMMIT")); + assert!(is_transaction_control_sql("/* a */ /* b */ /* c */ COMMIT")); + // Mixed with whitespace and `;`. + assert!(is_transaction_control_sql( + "; /* foo */ ;; /* bar */COMMIT" + )); + // Postgres-style nested block comments must strip completely. + assert!(is_transaction_control_sql( + "/* outer /* inner */ outer */ COMMIT" + )); + } + + #[test] + fn flags_line_comment_prefixed_finalization() { + assert!(is_transaction_control_sql("-- sneak\nCOMMIT")); + assert!(is_transaction_control_sql("-- foo\n -- bar\nROLLBACK")); + // Mixed line + block comments. + assert!(is_transaction_control_sql("-- foo\n/* bar */ COMMIT")); + assert!(is_transaction_control_sql("/* bar */-- foo\nCOMMIT")); + } + + #[test] + fn flags_comment_prefixed_start_transaction() { + // Combine finding #1 and finding #3: a comment-prefixed + // START TRANSACTION must also be rejected (otherwise MySQL would + // implicitly commit the outer txn). + assert!(is_transaction_control_sql("/* */ START TRANSACTION")); + assert!(is_transaction_control_sql("-- skip\nSTART TRANSACTION")); + } + + #[test] + fn passes_comment_only_or_unterminated() { + // A SQL string that's *only* comments has no command — driver will + // no-op or error; we don't need to reject. + assert!(!is_transaction_control_sql("/* just a comment */")); + assert!(!is_transaction_control_sql("-- end-of-line comment\n")); + assert!(!is_transaction_control_sql("-- no newline")); + // Unterminated block comment: strip consumes the rest, no token left. + assert!(!is_transaction_control_sql("/* unterminated COMMIT")); + assert!(!is_transaction_control_sql("")); + assert!(!is_transaction_control_sql(" ")); + assert!(!is_transaction_control_sql(";;;;")); + } + + #[test] + fn passes_comment_prefixed_dml() { + // Stripping must not turn benign comment-prefixed DML into a flag. + assert!(!is_transaction_control_sql("/* note */ SELECT 1")); + assert!(!is_transaction_control_sql( + "-- note\nINSERT INTO t VALUES (1)" + )); + assert!(!is_transaction_control_sql( + "/* a */ /* b */ UPDATE t SET n=1" + )); + } + + // --- UTF-8 safety ------------------------------------------------------- + + #[test] + fn handles_utf8_inside_comments() { + // Multi-byte characters inside the comment body must not break + // byte-level scanning. The string after `/* … */` is just `COMMIT`. + assert!(is_transaction_control_sql("/* 漢字 🔥 مرحبا */ COMMIT")); + assert!(is_transaction_control_sql("-- 漢字 emoji 🚀\nROLLBACK")); + } + + #[test] + fn handles_utf8_in_statement_body() { + // UTF-8 chars *after* the keyword must also survive (regression + // safety net for the byte-scan implementation). + assert!(!is_transaction_control_sql("SELECT '漢字 🔥' AS n")); + assert!(!is_transaction_control_sql( + "INSERT INTO t VALUES ('مرحبا')" + )); + } +} From b4ef390e5d417f9049f50f6297157071dfca5189 Mon Sep 17 00:00:00 2001 From: Anderson Leal Date: Wed, 20 May 2026 11:01:00 -0300 Subject: [PATCH 2/2] test(iii-database): add bypass repros + HARNESS_MODE dispatch to e2e harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New file `cases-tx-control-bypass.ts` (320 lines) — 4 cases that each attempt one of the side-channel finalization shapes the previous commit closes, then *prove* the desync if accepted by counting outside-tx rows that shouldn't be visible. Post-fix all 4 PASS; pre-fix at least one FAILs with a clear `BYPASS CONFIRMED [#N]` message naming the leaked row count. | # | Bypass | Drivers | |---|---|---| | 1 | `/* */COMMIT` via `transactionExecute` | all 3 | | 1b | `--\nCOMMIT` via `transactionExecute` | pg + mysql | | 2 | `COMMIT` via `transactionQuery` | all 3 | | 3 | `START TRANSACTION` via `transactionExecute` | mysql only | Harness dispatch: - `RunnerMode = 'full' | 'no-bypass' | 'bypass-only'` (exported), passed via `HARNESS_MODE` env. `bypass-only` skips SCHEMA_RESET since each bypass case owns its scratch table. - `run-tests.sh` adds `--bypass-only` / `--no-bypass` flags and a `COMPOSE` env override defaulting to `docker compose` (preserves CI behavior). For `podman-compose`, the script auto-switches to a `podman inspect .State.Health.Status` poll loop because podman-compose 1.x doesn't implement compose v2's `--wait`. - `SCHEMA_RESET` now defensively drops `outbox` + `__iii_cursors` (vestiges of the removed query-poll trigger that survive `--keep` data volumes). - `cases-interactive-tx.ts` gets two additional regression cases asserting comment-prefixed and START-form rejection on both `transactionExecute` and `transactionQuery` paths. README documents the new flags, the bypass-repro table mapping each repro to its `/review` finding, and the `COMPOSE` override for rootless podman. --- iii-database/tests/e2e/README.md | 57 +++- iii-database/tests/e2e/run-tests.sh | 93 ++++- .../e2e/workers/harness/package-lock.json | 2 + .../harness/src/cases-interactive-tx.ts | 83 +++++ .../harness/src/cases-tx-control-bypass.ts | 320 ++++++++++++++++++ .../tests/e2e/workers/harness/src/cases.ts | 6 + .../tests/e2e/workers/harness/src/runner.ts | 76 +++-- .../tests/e2e/workers/harness/src/worker.ts | 18 +- 8 files changed, 609 insertions(+), 46 deletions(-) create mode 100644 iii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.ts diff --git a/iii-database/tests/e2e/README.md b/iii-database/tests/e2e/README.md index 16da94e3..d710ca09 100644 --- a/iii-database/tests/e2e/README.md +++ b/iii-database/tests/e2e/README.md @@ -1,7 +1,11 @@ # iii-database worker — end-to-end harness -Self-asserting smoke harness for the `iii-database` worker. Validates the 5 -core RPC functions and the `row-change` slot/publication derivation contract +Self-asserting smoke harness for the `iii-database` worker. Validates the +function surface (query / execute / prepareStatement / runStatement / +transaction), the **interactive-transaction** surface (begin / +transactionQuery / transactionExecute / commit / rollback), the +`row-change` slot/publication derivation contract, and the side-channel- +finalization repros from the `/review` of branch `feat/database-and-skills` against real **SQLite**, **PostgreSQL 16**, and **MySQL 8.4** with one command. @@ -9,7 +13,8 @@ Runs locally and in CI (`.github/workflows/iii-database-e2e.yml`). ## Prerequisites -- Docker (for the postgres + mysql containers) +- Docker (for the postgres + mysql containers). Rootless podman works too — + see the `COMPOSE` env override below. - Rust toolchain (`cargo` on `$PATH`) - Node.js 20+ (`npm` on `$PATH`) - The iii engine on `$PATH`. Install with: @@ -22,20 +27,26 @@ Runs locally and in CI (`.github/workflows/iii-database-e2e.yml`). ## Run ```sh -./run-tests.sh +./run-tests.sh # full suite + bypass repros +./run-tests.sh --bypass-only # ONLY the 4 side-channel-finalization repros +./run-tests.sh --no-bypass # full suite without the bypass repros (use + # when the worker hasn't shipped the fix yet) ``` Builds the worker (`cargo build --release --bin iii-database`), brings up -the docker stack with `wal_level=logical`, starts the engine, and runs ~90 -assertions across all 3 drivers. Exits 0 on PASS, 1 on any FAIL. +the docker stack with `wal_level=logical`, starts the engine, and runs the +selected case groups across all 3 drivers. Exits 0 on PASS, 1 on any FAIL. ## Flags | Flag | Effect | |---|---| -| `--keep` | Leave docker stack up after the run for debugging | +| `--keep` | Leave the compose stack up after the run for debugging | | `--no-build` | Skip the cargo build step | +| `--with-cargo-test` | Also run `cargo test --all-features` with live DB URLs (CI uses this) | | `--filter=` | Run only one driver | +| `--bypass-only` | Run only the side-channel-finalization bypass repros (4 cases × 3 drivers, gated per case) | +| `--no-bypass` | Run the full suite without the bypass repros | ## Env overrides @@ -48,9 +59,34 @@ overridden: | `III_BIN` | `$(command -v iii)` then `$HOME/.local/bin/iii` | Engine binary | | `WORKER_BIN_TARGET` | `$WORKER_SRC/target/release/iii-database` | Built worker | | `WORKER_BIN_LINK` | `$HOME/.iii/workers/iii-database` | Symlink the engine reads | +| `COMPOSE` | `docker compose` | Compose command. Set to `podman-compose` for rootless podman; the script auto-switches its healthcheck strategy to `podman inspect` (since podman-compose 1.x doesn't implement compose v2's `--wait`). | +| `HARNESS_MODE` | `full` | `full` / `no-bypass` / `bypass-only`. Set by the flags above; you usually don't need to set this directly. | | `HARNESS_TIMEOUT` | `180` | Seconds to wait for the test sentinel | | `HEALTH_TIMEOUT` | `60` | Seconds to wait for db healthchecks | +## Bypass repros (`--bypass-only`) + +The bypass-repro cases live in +`workers/harness/src/cases-tx-control-bypass.ts` and demonstrate the three +ways the `is_transaction_control_sql` filter in +`src/handlers/transaction_execute.rs` was bypassable before the fix in this +branch. Each case stages a row inside an interactive transaction, attempts +the bypass, and — if the worker accepts the SQL — proves the desync by +counting outside-txn rows that shouldn't be visible. PASS = defense holds; +FAIL = bypass confirmed (the failure message names the exact mode and the +leaked-row count). + +| # | Bypass | Drivers | Maps to | +|---|---|---|---| +| 1 | `/* */COMMIT` via `transactionExecute` | all 3 | finding #1 (block-comment strip) | +| 1b | `--\nCOMMIT` via `transactionExecute` | pg + mysql | finding #1 (line-comment strip) | +| 2 | `COMMIT` via `transactionQuery` | all 3 | finding #2 (missing guard on query path) | +| 3 | `START TRANSACTION` via `transactionExecute` | mysql only | finding #3 (`START TRANSACTION` keyword) | + +On the post-fix worker all four PASS. On a pre-fix worker each FAILing case +prints the desync evidence (e.g. `BYPASS CONFIRMED [#1]: '/* */COMMIT' was +accepted; outside-tx COUNT=1`). + ## Layout | File | Role | @@ -59,6 +95,8 @@ overridden: | `docker-compose.yml` | Postgres (wal_level=logical) + MySQL with healthchecks | | `config.yaml` | Engine config (queue, observability, iii-database, harness) | | `workers/harness/` | TypeScript smoke-test worker (runs as a host process) | +| `workers/harness/src/cases-interactive-tx.ts` | Interactive-transaction lifecycle cases | +| `workers/harness/src/cases-tx-control-bypass.ts` | Side-channel-finalization repros | | `reports/report.json` | Per-case results (latest run) | ## CI @@ -77,4 +115,7 @@ the same docker compose stack used locally, and shells out to - **`iii engine binary missing`**: install with the script above. - **Sentinel timeout**: tail `reports/harness-*.log` for the harness output. - **Docker daemon not running**: start Docker Desktop (or `colima start`) - and re-run. + and re-run. Or use rootless podman: `COMPOSE='podman-compose' ./run-tests.sh`. +- **Bypass cases FAIL**: expected on a worker that hasn't shipped the + side-channel-finalization fix. Re-run with `--no-bypass` to confirm the + rest of the suite is clean. diff --git a/iii-database/tests/e2e/run-tests.sh b/iii-database/tests/e2e/run-tests.sh index e5601372..99632298 100755 --- a/iii-database/tests/e2e/run-tests.sh +++ b/iii-database/tests/e2e/run-tests.sh @@ -12,6 +12,12 @@ III_BIN="${III_BIN:-$(command -v iii 2>/dev/null || echo "$HOME/.local/bin/iii") WORKER_BIN_TARGET="${WORKER_BIN_TARGET:-$WORKER_SRC/target/release/iii-database}" WORKER_BIN_LINK="${WORKER_BIN_LINK:-$HOME/.iii/workers/iii-database}" +# Container runtime. Defaults to `docker compose` (compose v2; what CI runs). +# Override to `podman-compose` for rootless podman on dev laptops; the script +# falls back to a healthcheck poll because podman-compose 1.x doesn't +# implement compose v2's `up --wait` flag. +COMPOSE="${COMPOSE:-docker compose}" + REPORT_PATH="$ROOT_DIR/reports/report.json" TS=$(date +%Y%m%d-%H%M%S) ENGINE_LOG="$ROOT_DIR/reports/engine-$TS.log" @@ -23,6 +29,7 @@ KEEP=0 NO_BUILD=0 WITH_CARGO_TEST=0 FILTER="" +MODE="full" for arg in "$@"; do case "$arg" in @@ -30,23 +37,34 @@ for arg in "$@"; do --no-build) NO_BUILD=1 ;; --with-cargo-test) WITH_CARGO_TEST=1 ;; --filter=*) FILTER="${arg#--filter=}" ;; + --bypass-only) MODE="bypass-only" ;; + --no-bypass) MODE="no-bypass" ;; -h|--help) cat <] +Usage: $0 [--keep] [--no-build] [--with-cargo-test] + [--filter=] [--bypass-only|--no-bypass] - --keep Leave docker compose stack running after the run. + --keep Leave the compose stack running after the run. --no-build Skip cargo build of the iii-database worker. --with-cargo-test Run \`cargo test --all-features\` after compose is healthy with TEST_POSTGRES_URL and TEST_MYSQL_URL pointing at the - docker stack — exercises gated driver/pool tests with - real DBs. CI uses this; local dev usually doesn't need it. + stack — exercises gated driver/pool tests with real DBs. + CI uses this; local dev usually doesn't need it. --filter=KEY Run only one driver (default: all 3). + --bypass-only Run ONLY the side-channel-finalization bypass repros. + Use this to focus on the /review findings: post-fix all + 4 cases must PASS; pre-fix at least one FAILs. + --no-bypass Run the full suite WITHOUT the bypass repros. Useful + when running against a worker that hasn't shipped the + fix yet so the rest of the suite reports cleanly. Env overrides: WORKER_SRC Path to the database worker crate (default: ../..). III_BIN Path to the iii engine binary (default: \$(command -v iii) or \$HOME/.local/bin/iii). WORKER_BIN_TARGET Path to the built worker binary (default: \$WORKER_SRC/target/release/iii-database). WORKER_BIN_LINK Path to the symlink the engine reads (default: \$HOME/.iii/workers/iii-database). + COMPOSE Compose command (default: 'docker compose'; set to + 'podman-compose' for rootless podman). HARNESS_TIMEOUT Seconds to wait for the harness sentinel (default: 180). HEALTH_TIMEOUT Seconds to wait for postgres/mysql healthchecks (default: 60). EOF @@ -69,7 +87,7 @@ cleanup() { wait "$ENGINE_PID" 2>/dev/null || true fi if [[ "$KEEP" -eq 0 ]]; then - (cd "$ROOT_DIR" && docker compose down -v >/dev/null 2>&1) || true + (cd "$ROOT_DIR" && $COMPOSE down -v >/dev/null 2>&1) || true fi exit "$code" } @@ -100,14 +118,58 @@ if [[ ! -x "$III_BIN" ]]; then exit 1 fi -# 4. Bring up postgres + mysql and wait for healthchecks via compose's --wait -# (compose v2 native; exits non-zero if any service fails to become healthy -# within HEALTH_TIMEOUT). Beats parsing `compose ps --format json` with regex. -echo "[run-tests] docker compose up -d --wait (timeout=${HEALTH_TIMEOUT}s)" -if ! (cd "$ROOT_DIR" && docker compose up -d --wait --wait-timeout "$HEALTH_TIMEOUT"); then - echo "[run-tests] FATAL: services did not become healthy within ${HEALTH_TIMEOUT}s" >&2 - (cd "$ROOT_DIR" && docker compose logs --tail 40) >&2 - exit 1 +# 4. Bring up postgres + mysql. +# +# docker compose v2 supports `up -d --wait --wait-timeout N`, which exits +# non-zero if any service fails to become healthy within the budget. That's +# the happy path and what CI uses. podman-compose 1.x doesn't implement +# `--wait`, so we detect the runtime and fall back to a healthcheck poll. +if [[ "$COMPOSE" == "docker compose" || "$COMPOSE" == "docker-compose" ]]; then + echo "[run-tests] $COMPOSE up -d --wait (timeout=${HEALTH_TIMEOUT}s)" + if ! (cd "$ROOT_DIR" && $COMPOSE up -d --wait --wait-timeout "$HEALTH_TIMEOUT"); then + echo "[run-tests] FATAL: services did not become healthy within ${HEALTH_TIMEOUT}s" >&2 + (cd "$ROOT_DIR" && $COMPOSE logs --tail 40) >&2 + exit 1 + fi +else + # Fallback for podman-compose et al. Poll `podman inspect` (or `docker + # inspect`) for the `.State.Health.Status` field — works on both runtimes + # because Healthcheck status is an OCI-standard field. + echo "[run-tests] $COMPOSE up -d" + (cd "$ROOT_DIR" && $COMPOSE up -d) + + # Auto-detect the runtime CLI from the compose command. `podman-compose` + # → `podman`; everything else → `docker`. + INSPECT_BIN="docker" + if [[ "$COMPOSE" == "podman-compose" ]]; then + INSPECT_BIN="podman" + fi + project=$(basename "$ROOT_DIR") + echo "[run-tests] waiting up to ${HEALTH_TIMEOUT}s for postgres + mysql to be healthy ($INSPECT_BIN inspect)" + deadline=$(( $(date +%s) + HEALTH_TIMEOUT )) + while :; do + # Containers are named {_|-}{_|-}1 depending on the + # compose version — accept either separator. + pg_name=$("$INSPECT_BIN" ps --format '{{.Names}}' | grep -E "${project}[-_]postgres([-_]1)?$" | head -1 || true) + my_name=$("$INSPECT_BIN" ps --format '{{.Names}}' | grep -E "${project}[-_]mysql([-_]1)?$" | head -1 || true) + pg_status="" + my_status="" + if [[ -n "$pg_name" ]]; then + pg_status=$("$INSPECT_BIN" inspect "$pg_name" --format '{{.State.Health.Status}}' 2>/dev/null || echo "") + fi + if [[ -n "$my_name" ]]; then + my_status=$("$INSPECT_BIN" inspect "$my_name" --format '{{.State.Health.Status}}' 2>/dev/null || echo "") + fi + if [[ "$pg_status" == "healthy" && "$my_status" == "healthy" ]]; then + break + fi + if (( $(date +%s) > deadline )); then + echo "[run-tests] FATAL: services did not become healthy within ${HEALTH_TIMEOUT}s (pg=$pg_status mysql=$my_status)" >&2 + (cd "$ROOT_DIR" && $COMPOSE logs --tail 40) >&2 + exit 1 + fi + sleep 1 + done fi echo "[run-tests] both services healthy" @@ -128,7 +190,7 @@ if [[ "$WITH_CARGO_TEST" -eq 1 ]]; then fi # 6. Reset SQLite file -rm -f "$ROOT_DIR/data/test.sqlite" +rm -f "$ROOT_DIR/data/test.sqlite" "$ROOT_DIR/data/iii.db" # 7. Install harness deps if needed if [[ ! -d "$ROOT_DIR/workers/harness/node_modules" ]]; then @@ -169,13 +231,14 @@ done echo "[run-tests] engine listening" # 10. Launch the harness as a host node process -echo "[run-tests] starting harness" +echo "[run-tests] starting harness (mode=$MODE)" HARNESS_ENV=() if [[ -n "$FILTER" ]]; then HARNESS_ENV+=("HARNESS_FILTER=$FILTER") fi HARNESS_ENV+=("III_URL=ws://127.0.0.1:49134") HARNESS_ENV+=("HARNESS_REPORT_PATH=$REPORT_PATH") +HARNESS_ENV+=("HARNESS_MODE=$MODE") ( cd "$ROOT_DIR/workers/harness" && env "${HARNESS_ENV[@]}" npm run --silent dev ) > "$HARNESS_LOG" 2>&1 & HARNESS_PID=$! diff --git a/iii-database/tests/e2e/workers/harness/package-lock.json b/iii-database/tests/e2e/workers/harness/package-lock.json index 36f4a7d8..3cd1ffdd 100644 --- a/iii-database/tests/e2e/workers/harness/package-lock.json +++ b/iii-database/tests/e2e/workers/harness/package-lock.json @@ -464,6 +464,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/api/-/api-1.9.1.tgz", "integrity": "sha512-gLyJlPHPZYdAk1JENA9LeHejZe1Ti77/pTeFm/nMXmQH/HFZlcS/O2XJB+L8fkbrNSqhdtlvjBVjxwUYanNH5Q==", "license": "Apache-2.0", + "peer": true, "engines": { "node": ">=8.0.0" } @@ -784,6 +785,7 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.16.0.tgz", "integrity": "sha512-UVJyE9MttOsBQIDKw1skb9nAwQuR5wuGD3+82K6JgJlm/Y+KI92oNsMNGZCYdDsVtRHSak0pcV5Dno5+4jh9sw==", "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, diff --git a/iii-database/tests/e2e/workers/harness/src/cases-interactive-tx.ts b/iii-database/tests/e2e/workers/harness/src/cases-interactive-tx.ts index 0d63f468..17e540c4 100644 --- a/iii-database/tests/e2e/workers/harness/src/cases-interactive-tx.ts +++ b/iii-database/tests/e2e/workers/harness/src/cases-interactive-tx.ts @@ -133,6 +133,89 @@ export const INTERACTIVE_TX_CASES: TestCase[] = [ } }, }, + { + // Regression for the three side-channel-finalization bypasses identified + // in pre-landing review: `/* */COMMIT`, `--\nCOMMIT`, and `START + // TRANSACTION`. Without the strip-leading-comments fix and the + // START-keyword addition, all three slipped past `is_transaction_control_sql` + // and reached the driver, desyncing the registry from the conn's txn state. + name: 'transactionExecute rejects comment-prefixed and START-form finalization', + async run({ driver, call, expectError }) { + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + try { + await expectError( + () => + call('iii-database::transactionExecute', { + transaction_id: id, + sql: '/* sneak */COMMIT', + }), + 'INVALID_PARAM', + ); + await expectError( + () => + call('iii-database::transactionExecute', { + transaction_id: id, + sql: '-- sneak\nCOMMIT', + }), + 'INVALID_PARAM', + ); + await expectError( + () => + call('iii-database::transactionExecute', { + transaction_id: id, + sql: 'START TRANSACTION', + }), + 'INVALID_PARAM', + ); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* ignore */} + } + }, + }, + { + // Regression: `transactionQuery` previously had no transaction-control + // filter at all. PG/MySQL/SQLite all execute COMMIT through the + // prepared-statement path used by `run_prepared`. Must now reject. + name: 'transactionQuery rejects COMMIT/ROLLBACK and comment-prefixed forms', + async run({ driver, call, expectError }) { + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + try { + await expectError( + () => call('iii-database::transactionQuery', { transaction_id: id, sql: 'COMMIT' }), + 'INVALID_PARAM', + ); + await expectError( + () => + call('iii-database::transactionQuery', { transaction_id: id, sql: 'ROLLBACK' }), + 'INVALID_PARAM', + ); + await expectError( + () => + call('iii-database::transactionQuery', { + transaction_id: id, + sql: '/* sneak */COMMIT', + }), + 'INVALID_PARAM', + ); + await expectError( + () => + call('iii-database::transactionQuery', { + transaction_id: id, + sql: 'START TRANSACTION', + }), + 'INVALID_PARAM', + ); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* ignore */} + } + }, + }, { name: 'interactive tx beginTransaction unknown isolation returns INVALID_PARAM', async run({ driver, call, expectError }) { diff --git a/iii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.ts b/iii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.ts new file mode 100644 index 00000000..73b265e9 --- /dev/null +++ b/iii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.ts @@ -0,0 +1,320 @@ +import type { TestCase } from './cases.ts'; +import { expect, expectEqual } from './cases.ts'; + +/** + * Side-channel finalization bypass repros. + * + * Each case demonstrates a separate way the + * `transactionExecute`/`transactionQuery` defense in + * `iii-database/src/handlers/transaction_execute.rs::is_transaction_control_sql` + * can be bypassed to finalize an interactive transaction without going through + * `commitTransaction` / `rollbackTransaction`. + * + * Each case is structured as: + * - drive the worker through a sequence that SHOULD be rejected with + * `INVALID_PARAM` (per the docstring on `transactionExecute`) + * - if the worker rejects → PASS (defense holds, fix shipped) + * - if the worker accepts AND the transaction-registry/conn state desyncs + * → FAIL with a label describing the observed bypass + * + * Run with: `./run-tests.sh --bypass-only` + * + * Findings (see /review output on branch feat/database-and-skills): + * 1. comment-prefix in transactionExecute (handlers/transaction_execute.rs:42-63) + * 2. transactionQuery missing the guard (handlers/transaction_query.rs) + * 3. `START TRANSACTION` not in keyword set (handlers/transaction_execute.rs:55, MySQL-specific) + * + * These cases will FAIL until the fix is shipped. After the fix, they MUST + * pass on every driver they apply to. + */ +export const TX_CONTROL_BYPASS_CASES: TestCase[] = [ + { + // Finding #1: `/* ... */COMMIT` — the first whitespace-delimited token is + // `/*`, never matches the COMMIT keyword, the SQL is passed to the driver, + // and PG/MySQL strip server-side comments and execute the COMMIT. SQLite + // does the same. The registry still tracks the txn handle; subsequent + // calls see TRANSACTION_NOT_FOUND only after the watcher sweep removes + // the entry (1+ second later), or — worse — succeed against a non-tx conn + // until the watcher fires. + name: 'bypass #1: transactionExecute /* */COMMIT must be rejected', + async run({ driver, dialect, call }) { + const ph1 = dialect.placeholder(1); + const table = 'bypass_comment_commit'; + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + await call('iii-database::execute', { + db: driver, + sql: `CREATE TABLE ${table} (id ${dialect.idColumnDDL()}, n INT NOT NULL)`, + }); + + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + + try { + // Stage a row inside the txn. Visible from inside, not yet committed. + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: `INSERT INTO ${table} (n) VALUES (${ph1})`, + params: [1], + }); + + // The bypass attempt. The defense MUST reject this — the user must + // be told to use `commitTransaction`. + let rejected = false; + let acceptedErr: unknown = null; + try { + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: '/* sneak */COMMIT', + }); + } catch (e: any) { + const msg = e?.message ?? String(e); + if (msg.includes('INVALID_PARAM')) { + rejected = true; + } else { + acceptedErr = e; + } + } + + if (!rejected) { + // The worker accepted the SQL. Now prove it desynced the state. + // Read from outside the txn — if the COMMIT landed at the driver + // level, the staged row is visible from a fresh connection. + const outside = await call('iii-database::query', { + db: driver, + sql: `SELECT COUNT(*) AS c FROM ${table}`, + }); + const leakedCount = Number(outside.rows[0].c); + throw new Error( + `BYPASS CONFIRMED [#1 comment-prefix]: '/* */COMMIT' was accepted ` + + `by transactionExecute; outside-tx COUNT=${leakedCount} ` + + `(>0 means the side-channel COMMIT landed at the driver, ` + + `desyncing the registry from the connection state)` + + (acceptedErr ? ` — non-INVALID_PARAM error: ${acceptedErr}` : ''), + ); + } + expect(rejected, 'transactionExecute must reject /* */COMMIT with INVALID_PARAM'); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* may already be gone if the bypass succeeded */} + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + } + }, + }, + { + // Finding #2: `transactionQuery` has NO transaction-control filter at all. + // PG/MySQL/SQLite all happily execute COMMIT through the prepared-statement + // path that `run_prepared` uses. Once COMMIT lands, the registry still + // holds the entry — subsequent calls operate on a now-non-transactional + // pinned connection until the watcher sweeps. + name: 'bypass #2: transactionQuery COMMIT must be rejected', + async run({ driver, dialect, call }) { + const ph1 = dialect.placeholder(1); + const table = 'bypass_query_commit'; + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + await call('iii-database::execute', { + db: driver, + sql: `CREATE TABLE ${table} (id ${dialect.idColumnDDL()}, n INT NOT NULL)`, + }); + + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + + try { + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: `INSERT INTO ${table} (n) VALUES (${ph1})`, + params: [2], + }); + + let rejected = false; + let acceptedErr: unknown = null; + try { + await call('iii-database::transactionQuery', { + transaction_id: id, + sql: 'COMMIT', + }); + } catch (e: any) { + const msg = e?.message ?? String(e); + if (msg.includes('INVALID_PARAM')) { + rejected = true; + } else { + acceptedErr = e; + } + } + + if (!rejected) { + const outside = await call('iii-database::query', { + db: driver, + sql: `SELECT COUNT(*) AS c FROM ${table}`, + }); + const leakedCount = Number(outside.rows[0].c); + throw new Error( + `BYPASS CONFIRMED [#2 transactionQuery]: 'COMMIT' was accepted by ` + + `transactionQuery (no is_transaction_control_sql guard exists ` + + `on the query path); outside-tx COUNT=${leakedCount} ` + + `(>0 means the side-channel COMMIT landed at the driver)` + + (acceptedErr ? ` — non-INVALID_PARAM error: ${acceptedErr}` : ''), + ); + } + expect(rejected, 'transactionQuery must reject COMMIT with INVALID_PARAM'); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* ignore */} + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + } + }, + }, + { + // Finding #3: `START TRANSACTION` is the MySQL/ANSI SQL-92 spelling of + // `BEGIN`. The keyword set in `is_transaction_control_sql` only catches + // `BEGIN`, not `START`. On MySQL, `START TRANSACTION` while inside an + // existing txn implicitly COMMITs the in-progress one and opens a new + // one — directly breaking the atomicity contract. + // + // On Postgres, `START TRANSACTION` inside a txn issues a NOTICE and + // no-ops, so the worst case is "user is mildly confused" rather than + // "registry desyncs"; we still expect the worker to reject for + // consistency with how `BEGIN` is handled. Gated to mysql_db where the + // observable atomicity violation lives. + name: 'bypass #3: transactionExecute START TRANSACTION must be rejected (mysql)', + applies: ['mysql_db'], + async run({ driver, dialect, call }) { + const ph1 = dialect.placeholder(1); + const table = 'bypass_start_transaction'; + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + await call('iii-database::execute', { + db: driver, + sql: `CREATE TABLE ${table} (id ${dialect.idColumnDDL()}, n INT NOT NULL)`, + }); + + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + + try { + // Stage a row in the OUTER txn that — on a healthy worker — would be + // discarded when we `rollbackTransaction` below. If `START TRANSACTION` + // is accepted, MySQL implicitly COMMITs that row before opening the + // new (untracked) txn, leaking the row past the eventual rollback. + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: `INSERT INTO ${table} (n) VALUES (${ph1})`, + params: [3], + }); + + let rejected = false; + let acceptedErr: unknown = null; + try { + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: 'START TRANSACTION', + }); + } catch (e: any) { + const msg = e?.message ?? String(e); + if (msg.includes('INVALID_PARAM')) { + rejected = true; + } else { + acceptedErr = e; + } + } + + if (!rejected) { + // Roll back the (now-second) txn the user thinks they're in. + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* ignore */} + + const outside = await call('iii-database::query', { + db: driver, + sql: `SELECT COUNT(*) AS c FROM ${table}`, + }); + const leakedCount = Number(outside.rows[0].c); + throw new Error( + `BYPASS CONFIRMED [#3 START TRANSACTION on MySQL]: ` + + `'START TRANSACTION' was accepted by transactionExecute; ` + + `outside-tx COUNT=${leakedCount} after a rollback that should ` + + `have undone the staged INSERT (>0 means MySQL implicitly ` + + `COMMITed the outer txn when START TRANSACTION ran)` + + (acceptedErr ? ` — non-INVALID_PARAM error: ${acceptedErr}` : ''), + ); + } + expect(rejected, 'transactionExecute must reject START TRANSACTION with INVALID_PARAM'); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* may already be gone */} + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + } + }, + }, + { + // Extra repro: `--COMMIT` (line-comment-prefixed) — same shape as #1 but + // using `--` instead of `/* */`. The first token after `trim_start_matches` + // is `--COMMIT`, doesn't match the keyword set, falls through. Postgres + // strips line comments server-side and executes COMMIT. + name: 'bypass #1b: transactionExecute --COMMIT (line-comment) must be rejected', + // Skipped on sqlite_db: rusqlite's prepare on a sql string consisting of + // only `--text\n…COMMIT` may interpret the line-comment + COMMIT as a + // single statement OR raise its multi-statement guard; behavior is less + // predictable. PG/MySQL both reliably strip the line comment. + applies: ['pg_db', 'mysql_db'], + async run({ driver, dialect, call }) { + const ph1 = dialect.placeholder(1); + const table = 'bypass_line_comment'; + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + await call('iii-database::execute', { + db: driver, + sql: `CREATE TABLE ${table} (id ${dialect.idColumnDDL()}, n INT NOT NULL)`, + }); + + const begin = await call('iii-database::beginTransaction', { db: driver }); + const id = begin.transaction.id; + + try { + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: `INSERT INTO ${table} (n) VALUES (${ph1})`, + params: [4], + }); + + let rejected = false; + let acceptedErr: unknown = null; + try { + // Newline is required after `--` so the server sees COMMIT as a + // statement; otherwise the comment swallows the rest of the line. + await call('iii-database::transactionExecute', { + transaction_id: id, + sql: '-- sneak\nCOMMIT', + }); + } catch (e: any) { + const msg = e?.message ?? String(e); + if (msg.includes('INVALID_PARAM')) { + rejected = true; + } else { + acceptedErr = e; + } + } + + if (!rejected) { + const outside = await call('iii-database::query', { + db: driver, + sql: `SELECT COUNT(*) AS c FROM ${table}`, + }); + const leakedCount = Number(outside.rows[0].c); + throw new Error( + `BYPASS CONFIRMED [#1b line-comment]: '-- sneak\\nCOMMIT' was ` + + `accepted by transactionExecute; outside-tx COUNT=${leakedCount}` + + (acceptedErr ? ` — non-INVALID_PARAM error: ${acceptedErr}` : ''), + ); + } + expectEqual(rejected, true, 'transactionExecute must reject --COMMIT'); + } finally { + try { + await call('iii-database::rollbackTransaction', { transaction_id: id }); + } catch {/* ignore */} + await call('iii-database::execute', { db: driver, sql: `DROP TABLE IF EXISTS ${table}` }); + } + }, + }, +]; diff --git a/iii-database/tests/e2e/workers/harness/src/cases.ts b/iii-database/tests/e2e/workers/harness/src/cases.ts index 1da2c82a..1712ac00 100644 --- a/iii-database/tests/e2e/workers/harness/src/cases.ts +++ b/iii-database/tests/e2e/workers/harness/src/cases.ts @@ -37,6 +37,12 @@ export function expect(cond: boolean, msg: string): asserts cond { export const SCHEMA_RESET: TestCase = { name: 'schema-reset', async run({ driver, dialect, call }) { + // `outbox` and `__iii_cursors` are vestigial from the query-poll trigger + // surface that was removed on feat/database-and-skills. Drop defensively + // so re-runs against a stale data volume (docker / podman named volumes + // preserved across `--keep`) still come up clean. + await call('iii-database::execute', { db: driver, sql: 'DROP TABLE IF EXISTS outbox' }); + await call('iii-database::execute', { db: driver, sql: 'DROP TABLE IF EXISTS __iii_cursors' }); await call('iii-database::execute', { db: driver, sql: 'DROP TABLE IF EXISTS t' }); await call('iii-database::execute', { db: driver, diff --git a/iii-database/tests/e2e/workers/harness/src/runner.ts b/iii-database/tests/e2e/workers/harness/src/runner.ts index 62ed5ca7..cfe6c790 100644 --- a/iii-database/tests/e2e/workers/harness/src/runner.ts +++ b/iii-database/tests/e2e/workers/harness/src/runner.ts @@ -14,6 +14,7 @@ import { TRANSACTION_EDGE_CASES } from './cases-transaction.ts'; import { INTERACTIVE_TX_CASES } from './cases-interactive-tx.ts'; import { CONCURRENCY_CASES } from './cases-concurrency.ts'; import { ROW_CHANGE_CASES } from './cases-row-change.ts'; +import { TX_CONTROL_BYPASS_CASES } from './cases-tx-control-bypass.ts'; interface CaseResult { driver: DriverKey; @@ -23,10 +24,24 @@ interface CaseResult { duration_ms: number; } +/** + * Harness mode controls which case groups run: + * + * - `full` — full suite + bypass repros (default; this is what CI runs) + * - `no-bypass` — full suite WITHOUT the bypass repros. Useful when running + * against a worker that hasn't shipped the side-channel- + * finalization fix yet, so the rest of the suite still + * reports clean instead of being drowned in bypass FAILs. + * - `bypass-only` — ONLY the bypass repros. Use this to focus on the + * /review findings (post-fix it must be all-PASS). + */ +export type RunnerMode = 'full' | 'no-bypass' | 'bypass-only'; + export interface RunnerOptions { iii: ISdk; reportPath: string; filterDriver?: DriverKey; + mode?: RunnerMode; } export class Runner { @@ -89,6 +104,7 @@ export class Runner { } async runAll(): Promise<{ pass: number; total: number; results: CaseResult[] }> { + const mode: RunnerMode = this.opts.mode ?? 'full'; const drivers: DriverKey[] = this.opts.filterDriver ? [this.opts.filterDriver] : [...DRIVER_KEYS]; // Wait for the database worker to be reachable on the first driver before kicking off. await this.waitForDatabaseWorker(drivers[0]); @@ -116,29 +132,49 @@ export class Runner { return r; }; + // The full suite (`full` and `no-bypass`) needs the shared `t` table from + // SCHEMA_RESET. `bypass-only` doesn't, since every bypass case manages its + // own scratch table — skip the reset to avoid letting a stale schema from + // a prior run block the focused repro. + const includeFullSuite = mode === 'full' || mode === 'no-bypass'; + const includeBypassRepros = mode === 'full' || mode === 'bypass-only'; + for (const driver of drivers) { - // Always run the schema reset; not a counted case but failures abort this driver. - const reset = record(await this.runCase(driver, SCHEMA_RESET)); - if (reset.status === 'FAIL') continue; + if (includeFullSuite) { + // Always run the schema reset; not a counted case but failures abort this driver. + const reset = record(await this.runCase(driver, SCHEMA_RESET)); + if (reset.status === 'FAIL') continue; + + // Function suite. + for (const c of FUNCTION_CASES) { + record(await this.runCase(driver, c)); + } - // Function suite. - for (const c of FUNCTION_CASES) { - record(await this.runCase(driver, c)); + // Boundary, protocol, transaction-edge, interactive-tx, concurrency, + // row-change cases. Each test is self-contained (creates and drops + // its own scratch tables / replication slots) so order doesn't matter. + for (const c of [ + ...BOUNDARY_CASES, + ...PROTOCOL_CASES, + ...TRANSACTION_EDGE_CASES, + ...INTERACTIVE_TX_CASES, + ...CONCURRENCY_CASES, + ...ROW_CHANGE_CASES, + ]) { + if (!matchesDriver(driver, c)) continue; + record(await this.runCase(driver, c)); + } } - // Boundary, protocol, transaction-edge, concurrency, row-change cases. - // Each test is self-contained (creates and drops its own scratch tables - // / replication slots) so order doesn't matter. - for (const c of [ - ...BOUNDARY_CASES, - ...PROTOCOL_CASES, - ...TRANSACTION_EDGE_CASES, - ...INTERACTIVE_TX_CASES, - ...CONCURRENCY_CASES, - ...ROW_CHANGE_CASES, - ]) { - if (!matchesDriver(driver, c)) continue; - record(await this.runCase(driver, c)); + if (includeBypassRepros) { + // Side-channel-finalization repros are kept as their own group so + // they can be re-run in isolation via `--bypass-only` and appear + // together in the per-case summary as the documented coverage for + // the /review findings on this branch. + for (const c of TX_CONTROL_BYPASS_CASES) { + if (!matchesDriver(driver, c)) continue; + record(await this.runCase(driver, c)); + } } } @@ -148,7 +184,7 @@ export class Runner { mkdirSync(resolve(this.opts.reportPath, '..'), { recursive: true }); writeFileSync( this.opts.reportPath, - JSON.stringify({ pass, total: counted.length, results }, null, 2), + JSON.stringify({ pass, total: counted.length, results, mode }, null, 2), ); return { pass, total: counted.length, results }; diff --git a/iii-database/tests/e2e/workers/harness/src/worker.ts b/iii-database/tests/e2e/workers/harness/src/worker.ts index aeac6eec..27bf5664 100644 --- a/iii-database/tests/e2e/workers/harness/src/worker.ts +++ b/iii-database/tests/e2e/workers/harness/src/worker.ts @@ -1,17 +1,29 @@ import { registerWorker, Logger } from 'iii-sdk'; import { resolve } from 'node:path'; -import { Runner } from './runner.ts'; +import { Runner, type RunnerMode } from './runner.ts'; import type { DriverKey } from './dialect.ts'; const URL = process.env.III_URL ?? 'ws://127.0.0.1:49134'; const REPORT_PATH = resolve(process.env.HARNESS_REPORT_PATH ?? './reports/report.json'); const FILTER = process.env.HARNESS_FILTER as DriverKey | undefined; +// `HARNESS_MODE` controls which case groups run. Validated here so a typo in +// run-tests.sh becomes "ran the full suite" rather than silently disabling +// the bypass repros that CI relies on. +const MODE_ENV = (process.env.HARNESS_MODE ?? 'full') as string; +const MODE: RunnerMode = + MODE_ENV === 'bypass-only' || MODE_ENV === 'no-bypass' ? MODE_ENV : 'full'; + const iii = registerWorker(URL); const logger = new Logger(); -const runner = new Runner({ iii, reportPath: REPORT_PATH, filterDriver: FILTER }); +const runner = new Runner({ iii, reportPath: REPORT_PATH, filterDriver: FILTER, mode: MODE }); -logger.info('harness: registered, kicking off suite', { url: URL, filter: FILTER ?? 'all', reportPath: REPORT_PATH }); +logger.info('harness: registered, kicking off suite', { + url: URL, + filter: FILTER ?? 'all', + mode: MODE, + reportPath: REPORT_PATH, +}); (async () => { // ANSI colors only when stdout is a TTY — run-tests.sh redirects to a log file,