fix(iii-database): close interactive-transaction side-channel finalization bypasses#168
Merged
andersonleal merged 2 commits intoMay 20, 2026
Merged
Conversation
…ation bypasses 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.
…harness 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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
skill-check — worker0 verified, 10 skipped (no docs/).
Three for three. Nicely done. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
sergiofilhowz
approved these changes
May 20, 2026
sergiofilhowz
added a commit
that referenced
this pull request
May 20, 2026
…transaction details (#165) * chore(iii-database): update version to 0.0.4 and enhance README with transaction details - Bump version of iii-database in Cargo.lock to 0.0.4. - Revise README.md to clarify transaction handling, including new transaction functions and their usage. - Remove outdated trigger documentation related to `iii-database::query-poll`. - Add new sections for interactive transactions and prepared statements, detailing their inputs and outputs. - Introduce new skills documentation for various database operations, enhancing user guidance. * fix: fmt * chore: enhance TLS configuration and error handling in database connections - Updated README.md to clarify the behavior of `ca_cert` and introduced `trust_native` option for TLS configurations, allowing for more flexible certificate management. - Modified TlsConfig struct to include `trust_native` with a default value of true, ensuring the system trust store is extended by default. - Improved error classification in MySQL and Postgres connection pools to provide clearer feedback on connection failures, including specific hints for TLS, authentication, and network issues. - Adjusted tests to utilize the new default TlsConfig settings, ensuring consistency across database connection tests. * fix: fmt * fix: clippy * fix(iii-database): close interactive-transaction side-channel finalization bypasses (#168) * fix(iii-database): close interactive-transaction side-channel finalization bypasses 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. * test(iii-database): add bypass repros + HARNESS_MODE dispatch to e2e harness 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. --------- Co-authored-by: Anderson Leal <andersonofl@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes three side-channel-finalization bypasses in the new interactive-transaction surface added to this branch in #165. Each bypass let a caller submit SQL that the driver executes server-side as
COMMIT/ROLLBACK/BEGIN, without theTxRegistryever finding out — desynchronizing the registry from the connection's real txn state and silently breaking the atomicity contract the public API promises./* */COMMITand--\nCOMMITsurvive the first-token check — PG/MySQL/SQLite strip server-side comments before parsinghandlers/transaction_execute.rs::is_transaction_control_sql;, line comments, and block comments (postgres-style nested) iteratively before tokenizingtransactionQueryhad no transaction-control filter at all; PG/MySQL/SQLite executeCOMMIT/ROLLBACKthrough the prepared-statement pathhandlers/transaction_query.rshandle, with matching log event +INVALID_PARAMerrorSTART TRANSACTION(ANSI / MySQL spelling ofBEGIN) wasn't in the keyword set; on MySQL it implicitly commits the outer txn and opens a new untracked onehandlers/transaction_execute.rs::is_transaction_control_sqlSTARTto the two-token form alongsideSET TRANSACTION;START SLAVE/START REPLICAcorrectly pass throughThe guard is factored into a new
handlers::tx_sql_guardmodule so both handlers share one defense.Why these matter
The bypasses don't grant new capabilities — a caller can already finalize through
commitTransaction/rollbackTransaction. The harm is silent state desync: the registry thinks the txn is open, the timeout watcher later issues a no-op or error `ROLLBACK`, and subsequent `transactionExecute` / `transactionQuery` calls run on a non-transactional pinned connection while the caller still believes they're inside the txn. Atomicity guarantee gone, no error returned.Test plan
How to verify locally
```sh
cd iii-database/tests/e2e
./run-tests.sh --bypass-only # focused: ONLY the 4 bypass repros (all PASS post-fix)
./run-tests.sh # full suite + bypass repros
COMPOSE='podman-compose' ./run-tests.sh --bypass-only # rootless podman
```
The bypass repros run against real PG / MySQL / SQLite through the engine, so they exercise the full driver → handler → registry path end-to-end.
Files changed
```
iii-database/src/handlers/mod.rs | 1 +
iii-database/src/handlers/transaction_execute.rs | 146 +++++++------
iii-database/src/handlers/transaction_query.rs | 120 ++++++++++-
iii-database/src/handlers/tx_sql_guard.rs | 258 +++++++++++++++++++++++ (new)
iii-database/tests/e2e/README.md | 57 +-
iii-database/tests/e2e/run-tests.sh | 93 +-
iii-database/tests/e2e/workers/harness/src/cases-interactive-tx.ts | 83 ++
iii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.ts | 320 ++ (new)
iii-database/tests/e2e/workers/harness/src/cases.ts | 6 +
iii-database/tests/e2e/workers/harness/src/runner.ts | 76 +-
iii-database/tests/e2e/workers/harness/src/worker.ts | 18 +-
```