From 62f960fea9a4dac243550b39252d228148a08490 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:53:11 +0000 Subject: [PATCH 01/10] test: failing coverage for transactions, AEAD, file modes and TOML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red evidence for #664, #665, #666 and #667. Tests only — no implementation. Every suite fails because the feature does not exist: database_transaction_test parse error on `in transaction on db:` crypto_seal_test Undefined variable 'seal' filesystem_mode_test Undefined variable 'file_mode' / 'set_file_mode' toml_test Undefined variable 'parse_toml' The transaction suite is deliberately file-backed rather than in-memory: `open database` hands out a five-connection pool, and in-memory SQLite is special-cased to a single connection, which is exactly what hides #664 from a test suite. Adds the two dependencies the implementation will need (chacha20poly1305 for XChaCha20-Poly1305, toml for the parser) so the Red run compiles. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- Cargo.lock | 112 +++- Cargo.toml | 8 + ...d-664-667-transactions-aead-modes-toml.txt | 28 + tests/crypto_seal_test.rs | 363 +++++++++++++ tests/database_transaction_test.rs | 478 ++++++++++++++++++ tests/filesystem_mode_test.rs | 264 ++++++++++ tests/toml_test.rs | 271 ++++++++++ 7 files changed, 1517 insertions(+), 7 deletions(-) create mode 100644 Engineering/evidence/red-664-667-transactions-aead-modes-toml.txt create mode 100644 tests/crypto_seal_test.rs create mode 100644 tests/database_transaction_test.rs create mode 100644 tests/filesystem_mode_test.rs create mode 100644 tests/toml_test.rs diff --git a/Cargo.lock b/Cargo.lock index 56326c3f..aaef6ffa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,6 +17,16 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" +[[package]] +name = "aead" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1973cfbc1a2daf9cf550e74e1f088c28e7f7d8c1e1418fb6c9dc5184b7e84c99" +dependencies = [ + "crypto-common 0.2.2", + "inout 0.2.2", +] + [[package]] name = "aho-corasick" version = "1.1.4" @@ -385,8 +395,23 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d524456ba66e72eb8b115ff89e01e497f8e6d11d78b70b1aa13c0fbd97540a81" dependencies = [ "cfg-if", + "cipher 0.5.2", "cpufeatures 0.3.0", "rand_core 0.10.1", + "zeroize", +] + +[[package]] +name = "chacha20poly1305" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b89e1c441e926b9c82a8d023f6e1b7ae0adcfaa7d621814e4d60789bac751cb" +dependencies = [ + "aead", + "chacha20", + "cipher 0.5.2", + "poly1305", + "zeroize", ] [[package]] @@ -445,6 +470,7 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8cf2a2c93cd704877c0858356ed03480ff301ee950b43f1cbe4573b088bfa6c" dependencies = [ + "block-buffer 0.12.1", "crypto-common 0.2.2", "inout 0.2.2", ] @@ -506,7 +532,7 @@ checksum = "af491d569909a7e4dee0ad7db7f5341fef5c614d5b8ec8cf765732aba3cff681" dependencies = [ "serde", "termcolor", - "unicode-width 0.1.14", + "unicode-width", ] [[package]] @@ -690,7 +716,9 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ce6e4c961d6cd6c9a86db418387425e8bdeaf05b3c8bc1411e6dca4c252f1453" dependencies = [ + "getrandom 0.4.3", "hybrid-array", + "rand_core 0.10.1", ] [[package]] @@ -2207,6 +2235,16 @@ dependencies = [ "plotters-backend", ] +[[package]] +name = "poly1305" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2d0073b297041425c7c3df6eb4792d598a15323fe63346852b092eca02904c" +dependencies = [ + "cpufeatures 0.3.0", + "universal-hash", +] + [[package]] name = "portable-atomic" version = "1.13.1" @@ -2703,7 +2741,7 @@ dependencies = [ "nix", "radix_trie", "unicode-segmentation", - "unicode-width 0.2.2", + "unicode-width", "utf8parse", "windows-sys 0.61.2", ] @@ -2848,6 +2886,15 @@ dependencies = [ "syn", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -3486,6 +3533,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.4+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3aace63f4bbcdfc2c965b059de67119c89c4017a70d633be6c104910f67056f5" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.4.13" @@ -3699,15 +3785,19 @@ checksum = "c6f5d3c3b1bf09027a88a6bc961fc00497d651009560b5463668dc81b0fa87a8" [[package]] name = "unicode-width" -version = "0.1.14" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7dd6e30e90baa6f72411720665d41d89b9a3d039dc45b8faea1ddd07f617f6af" +checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" [[package]] -name = "unicode-width" -version = "0.2.2" +name = "universal-hash" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" +checksum = "f4987bdc12753382e0bec4a65c50738ffaabc998b9cdd1f952fb5f39b0048a96" +dependencies = [ + "crypto-common 0.2.2", + "ctutils", +] [[package]] name = "untrusted" @@ -3938,6 +4028,7 @@ dependencies = [ "argon2", "bcrypt", "bytes", + "chacha20poly1305", "chrono", "codespan-reporting", "criterion", @@ -3971,6 +4062,7 @@ dependencies = [ "time", "tokio", "tokio-tungstenite 0.30.0", + "toml", "uuid", "warp", "zeroize", @@ -4179,6 +4271,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "writeable" version = "0.6.3" diff --git a/Cargo.toml b/Cargo.toml index 40ad9a3c..f618ae60 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,6 +61,9 @@ encoding_rs = "0.8.35" # and TLS backend; `tls-rustls` aliases the ring-backed rustls stack we used before. sqlx = { version = "0.9.0", features = ["runtime-tokio", "tls-rustls", "sqlite", "mysql", "postgres", "chrono"] } serde_json = "1.0.150" +# Backs the `parse_toml` / `stringify_toml` builtins, mirroring the serde_json-backed +# JSON surface. Ships TOML 1.0 support (and reads the 1.1 spec additions). +toml = "1.1.4" # Held at 0.3.x: warp 0.4 dropped the `tls` feature (its TLS code is gated behind # an undeclared `tls` feature), which would remove HTTPS support from WFL's web # server (`secured with certificate ... and key ...`) — a backward-compat break. @@ -87,6 +90,11 @@ argon2 = "0.5" scrypt = { version = "0.11", features = ["simple"] } pbkdf2 = { version = "0.12", features = ["simple"] } bcrypt = "0.19" +# Authenticated encryption for the `seal`/`unseal` builtins. XChaCha20-Poly1305 +# specifically: its 192-bit nonce is wide enough that the implementation can mint +# a fresh random nonce per seal without a counter, so callers never touch nonces +# and cannot reuse one. `zeroize` wipes the expanded key on drop. +chacha20poly1305 = { version = "0.11.0", features = ["zeroize"] } # Force newer version to fix future incompatibility warning num-bigint-dig = "0.8.6" # Direct dep so the lib can expose `init_rustls_crypto_provider()` (called by diff --git a/Engineering/evidence/red-664-667-transactions-aead-modes-toml.txt b/Engineering/evidence/red-664-667-transactions-aead-modes-toml.txt new file mode 100644 index 00000000..04a2c00d --- /dev/null +++ b/Engineering/evidence/red-664-667-transactions-aead-modes-toml.txt @@ -0,0 +1,28 @@ +Red evidence — WFL issues #664 / #665 / #666 / #667 +Recorded: 2026-07-31T06:52:40Z +Base commit: 438780ae038608783c54fdc661273f040c4c58dd +Toolchain: cargo 1.94.1 (29ea6fb6a 2026-03-24) + +All four suites fail because the feature does not exist yet — not because +of a broken test. The exact first-failure reason per suite: + +--- database_transaction_test + Unexpected token in expression: KeywordIn + test result: FAILED. 2 passed; 11 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s + +--- crypto_seal_test + Undefined variable 'seal' + test result: FAILED. 3 passed; 14 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s + +--- filesystem_mode_test + Undefined variable 'file_mode' + Undefined variable 'set_file_mode' + test result: FAILED. 2 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s + +--- toml_test + Undefined variable 'parse_toml' + test result: FAILED. 2 passed; 9 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s + +Tests that already pass do so only because the program errors out; they +assert failure paths and are re-verified after Green to confirm they then +pass for the right reason. diff --git a/tests/crypto_seal_test.rs b/tests/crypto_seal_test.rs new file mode 100644 index 00000000..70d8d50c --- /dev/null +++ b/tests/crypto_seal_test.rs @@ -0,0 +1,363 @@ +// TDD tests for the `seal` / `unseal` AEAD builtins (issue #665). +// +// R3 (crypto/secrets): the point of an AEAD is that it fails *closed*. Most of +// these are therefore negative tests — a wrong key, a flipped byte, a truncated +// blob, or a mismatched context must all be rejected, and rejected the same way, +// so `unseal` never becomes an oracle that distinguishes one failure from another. + +use wfl::interpreter::Interpreter; +use wfl::interpreter::value::Value; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +async fn run_wfl(code: &str) -> Result { + let tokens = lex_wfl_with_positions(code); + let mut parser = Parser::new(&tokens); + let ast = parser.parse().map_err(|e| format!("Parse error: {e:?}"))?; + + let mut interpreter = Interpreter::new(); + interpreter + .interpret(&ast) + .await + .map_err(|e| format!("Runtime error: {e:?}"))?; + Ok(interpreter) +} + +fn get_global(interpreter: &Interpreter, name: &str) -> Value { + interpreter + .global_env() + .borrow() + .get(name) + .unwrap_or_else(|| panic!("Variable '{name}' not found")) +} + +fn expect_text(value: &Value) -> String { + match value { + Value::Text(t) => t.to_string(), + other => panic!("Expected text, got {other:?}"), + } +} + +/// A valid 32-byte key as 64 hex characters — the shape `secure_random_bytes of 32` returns. +const KEY: &str = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"; +const OTHER_KEY: &str = "ffeeddccbbaa99887766554433221100ffeeddccbbaa99887766554433221100"; + +// --------------------------------------------------------------------------- +// The happy path from the issue: mint a key, seal a secret, get it back. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn seal_then_unseal_round_trips() { + let code = format!( + r#" +store key as "{KEY}" +store secret as "sk-live-abc123-my-provider-token" +store sealed as seal of secret and key +store plain as unseal of sealed and key +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "plain")), + "sk-live-abc123-my-provider-token" + ); +} + +#[tokio::test] +async fn sealed_output_does_not_leak_the_plaintext() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "correct-horse-battery-staple" and key +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let sealed = expect_text(&get_global(&interpreter, "sealed")); + assert!( + !sealed.contains("correct-horse"), + "ciphertext must not contain the plaintext: {sealed}" + ); + assert!( + sealed.starts_with("wflseal1:"), + "sealed output should be self-describing and versioned, got: {sealed}" + ); +} + +#[tokio::test] +async fn a_key_from_secure_random_bytes_works_end_to_end() { + // The exact flow issue #665 describes: `secure_random_bytes of 32` mints a + // key, and that key must be directly usable for sealing. + let code = r#" +store project_key as secure_random_bytes of 32 +store sealed as seal of "project secret" and project_key +store plain as unseal of sealed and project_key +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "plain")), + "project secret" + ); +} + +#[tokio::test] +async fn sealing_the_same_plaintext_twice_gives_different_ciphertexts() { + // Nonce freshness. If these ever match, the nonce is being reused. + let code = format!( + r#" +store key as "{KEY}" +store a as seal of "same message" and key +store b as seal of "same message" and key +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let a = expect_text(&get_global(&interpreter, "a")); + let b = expect_text(&get_global(&interpreter, "b")); + assert_ne!( + a, b, + "each seal must use a fresh nonce, so identical plaintexts differ" + ); +} + +#[tokio::test] +async fn empty_and_unicode_plaintexts_round_trip() { + let code = format!( + r#" +store key as "{KEY}" +store empty_sealed as seal of "" and key +store empty_plain as unseal of empty_sealed and key +store uni_sealed as seal of "こんにちは 🌍 café" and key +store uni_plain as unseal of uni_sealed and key +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!(expect_text(&get_global(&interpreter, "empty_plain")), ""); + assert_eq!( + expect_text(&get_global(&interpreter, "uni_plain")), + "こんにちは 🌍 café" + ); +} + +// --------------------------------------------------------------------------- +// Fails closed +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn unseal_with_the_wrong_key_fails() { + let code = format!( + r#" +store key as "{KEY}" +store other as "{OTHER_KEY}" +store sealed as seal of "top secret" and key +store plain as unseal of sealed and other +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("unsealing with the wrong key must fail, not return garbage"); + assert!( + err.to_lowercase().contains("unseal"), + "error should name the operation, got: {err}" + ); +} + +/// Seal a message in one program run and hand the blob back to Rust, so the +/// tamper tests can mutate exact byte positions instead of doing string surgery +/// in WFL. +async fn seal_once(plaintext: &str) -> String { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "{plaintext}" and key +"# + ); + let interpreter = run_wfl(&code).await.expect("sealing should succeed"); + expect_text(&get_global(&interpreter, "sealed")) +} + +/// Try to unseal a literal blob, returning the error. +async fn expect_unseal_failure(blob: &str, why: &str) -> String { + let code = format!( + r#" +store key as "{KEY}" +store plain as unseal of "{blob}" and key +"# + ); + match run_wfl(&code).await { + Ok(_) => panic!("{why}"), + Err(e) => e, + } +} + +/// Flip one hex digit at `index` within the hex body of a sealed blob. +fn flip_hex_digit(sealed: &str, index: usize) -> String { + let (prefix, body) = sealed + .split_once(':') + .expect("sealed blob should be prefixed"); + let mut chars: Vec = body.chars().collect(); + assert!(index < chars.len(), "index must be inside the blob body"); + // Map the digit to a definitely-different one. + chars[index] = if chars[index] == '0' { '1' } else { '0' }; + format!("{prefix}:{}", chars.into_iter().collect::()) +} + +#[tokio::test] +async fn tampered_ciphertext_fails() { + let sealed = seal_once("top secret").await; + // Position 60 is past the 48-hex-char (24-byte) nonce, so this lands in the + // ciphertext body proper. + let tampered = flip_hex_digit(&sealed, 60); + assert_ne!( + tampered, sealed, + "the tamper helper must actually change it" + ); + let err = + expect_unseal_failure(&tampered, "a modified ciphertext must fail authentication").await; + assert!( + err.to_lowercase().contains("unseal"), + "error should name the operation, got: {err}" + ); +} + +#[tokio::test] +async fn tampered_nonce_fails() { + let sealed = seal_once("top secret").await; + // Position 0 is inside the nonce. + let tampered = flip_hex_digit(&sealed, 0); + expect_unseal_failure(&tampered, "a modified nonce must fail authentication").await; +} + +#[tokio::test] +async fn tampered_tag_fails() { + let sealed = seal_once("top secret").await; + let body_len = sealed.split_once(':').unwrap().1.len(); + // The last 32 hex chars are the 16-byte Poly1305 tag. + let tampered = flip_hex_digit(&sealed, body_len - 1); + expect_unseal_failure(&tampered, "a modified tag must fail authentication").await; +} + +#[tokio::test] +async fn truncated_ciphertext_fails() { + let sealed = seal_once("top secret").await; + let truncated = &sealed[..sealed.len() - 8]; + expect_unseal_failure( + truncated, + "a truncated blob must fail rather than partially decrypt", + ) + .await; +} + +#[tokio::test] +async fn blob_with_the_wrong_version_prefix_fails() { + let sealed = seal_once("top secret").await; + let body = sealed.split_once(':').unwrap().1; + let relabelled = format!("wflseal9:{body}"); + let err = expect_unseal_failure( + &relabelled, + "an unknown format version must be rejected, not guessed at", + ) + .await; + assert!( + err.to_lowercase().contains("unseal"), + "error should name the operation, got: {err}" + ); +} + +#[tokio::test] +async fn unseal_rejects_input_that_is_not_a_sealed_blob() { + let code = format!( + r#" +store key as "{KEY}" +store plain as unseal of "just some text a user typed" and key +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("unseal must reject input that was never sealed"); + assert!( + err.to_lowercase().contains("unseal"), + "error should name the operation, got: {err}" + ); +} + +#[tokio::test] +async fn seal_rejects_a_key_of_the_wrong_length() { + let code = r#" +store sealed as seal of "secret" and "tooshort" +"#; + let err = run_wfl(code) + .await + .err() + .expect("a short key must be rejected outright"); + assert!( + err.contains("secure_random_bytes"), + "the error should tell the user how to make a valid key, got: {err}" + ); +} + +#[tokio::test] +async fn seal_rejects_a_non_hex_key() { + let code = r#" +store sealed as seal of "secret" and "zzzz02030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" +"#; + let err = run_wfl(code) + .await + .err() + .expect("a key that is the right length but not hex must be rejected"); + assert!( + err.contains("secure_random_bytes"), + "the error should tell the user how to make a valid key, got: {err}" + ); +} + +// --------------------------------------------------------------------------- +// Associated data — binds a ciphertext to its context. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn associated_data_round_trips() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "provider token" and key and "project:acme/api_key" +store plain as unseal of sealed and key and "project:acme/api_key" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "plain")), + "provider token" + ); +} + +#[tokio::test] +async fn mismatched_associated_data_fails() { + // The whole point: a ciphertext sealed for one context must not unseal in another. + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "provider token" and key and "project:acme/api_key" +store plain as unseal of sealed and key and "project:evil/api_key" +"# + ); + run_wfl(&code) + .await + .err() + .expect("a ciphertext must not unseal under a different context"); +} + +#[tokio::test] +async fn context_is_required_on_unseal_when_it_was_used_on_seal() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "provider token" and key and "project:acme/api_key" +store plain as unseal of sealed and key +"# + ); + run_wfl(&code) + .await + .err() + .expect("dropping the context on unseal must fail, not silently succeed"); +} diff --git a/tests/database_transaction_test.rs b/tests/database_transaction_test.rs new file mode 100644 index 00000000..62eae34d --- /dev/null +++ b/tests/database_transaction_test.rs @@ -0,0 +1,478 @@ +// TDD tests for the `in transaction on :` block (issue #664). +// +// These MUST use file-backed SQLite. `open database` hands out a pool of +// MAX_POOL_CONNECTIONS (5) connections, and the bug in #664 is that each +// statement lands on a different one. In-memory SQLite is special-cased to a +// single connection (src/interpreter/database.rs), which hides the defect +// entirely — a program can pass its in-memory tests and still lose data in +// production. Every atomicity assertion here therefore runs against a temp file. + +use std::path::{Path, PathBuf}; +use wfl::interpreter::Interpreter; +use wfl::interpreter::value::Value; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +/// Run WFL code and return the interpreter for inspecting globals. +async fn run_wfl(code: &str) -> Result { + let tokens = lex_wfl_with_positions(code); + let mut parser = Parser::new(&tokens); + let ast = parser.parse().map_err(|e| format!("Parse error: {e:?}"))?; + + let mut interpreter = Interpreter::new(); + interpreter + .interpret(&ast) + .await + .map_err(|e| format!("Runtime error: {e:?}"))?; + Ok(interpreter) +} + +fn get_global(interpreter: &Interpreter, name: &str) -> Value { + interpreter + .global_env() + .borrow() + .get(name) + .unwrap_or_else(|| panic!("Variable '{name}' not found")) +} + +fn expect_number(value: &Value) -> f64 { + match value { + Value::Number(n) => *n, + other => panic!("Expected number, got {other:?}"), + } +} + +fn expect_list(value: &Value) -> Vec { + match value { + Value::List(list) => list.borrow().clone(), + other => panic!("Expected list, got {other:?}"), + } +} + +fn expect_object_key(value: &Value, key: &str) -> Value { + match value { + Value::Object(obj) => obj + .borrow() + .get(key) + .cloned() + .unwrap_or_else(|| panic!("Object missing key '{key}'")), + other => panic!("Expected object, got {other:?}"), + } +} + +/// A temp-file SQLite database that removes itself on drop. +/// +/// File-backed on purpose — see the module comment. +struct TempDb { + url: String, + path: PathBuf, +} + +impl TempDb { + fn new(test_name: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "wfl_tx_test_{}_{}.db", + test_name, + std::process::id() + )); + let _ = std::fs::remove_file(&path); + let url = format!("sqlite://{}", path.display()).replace('\\', "/"); + Self { url, path } + } +} + +impl Drop for TempDb { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.path); + // SQLite may leave a -wal / -shm sidecar next to the database. + for suffix in ["-wal", "-shm"] { + let mut sidecar = self.path.clone().into_os_string(); + sidecar.push(suffix); + let _ = std::fs::remove_file(Path::new(&sidecar)); + } + } +} + +/// Count rows in `projects` with a fresh connection, after the program is done. +async fn surviving_rows(url: &str) -> f64 { + let code = format!( + r#" +open database at "{url}" as db +store rows as query db with "SELECT slug FROM projects" +store n as length of rows +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("count program should run"); + expect_number(&get_global(&interpreter, "n")) +} + +// --------------------------------------------------------------------------- +// The reproduction from issue #664, verbatim in spirit: a rollback must roll back. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn transaction_block_rolls_back_on_error() { + let db = TempDb::new("rollback_on_error"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" + +store failed as no +try: + in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('should-vanish')" + store boom as execute db with "INSERT INTO nonexistent_table (x) VALUES (1)" + end transaction +when error: + change failed to yes +end try + +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + get_global(&interpreter, "failed"), + Value::Bool(true), + "the failing statement inside the block must surface as an error" + ); + + assert_eq!( + surviving_rows(url).await, + 0.0, + "issue #664: rows written inside a rolled-back transaction must not survive" + ); +} + +#[tokio::test] +async fn transaction_block_commits_on_normal_exit() { + let db = TempDb::new("commit_on_exit"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" + +in transaction on db: + store a as execute db with "INSERT INTO projects (slug) VALUES ('kept-one')" + store b as execute db with "INSERT INTO projects (slug) VALUES ('kept-two')" +end transaction + +close database db +"# + ); + run_wfl(&code).await.expect("program should run"); + + assert_eq!( + surviving_rows(url).await, + 2.0, + "both writes must be visible after the block commits" + ); +} + +#[tokio::test] +async fn transaction_block_is_atomic_all_or_nothing() { + let db = TempDb::new("all_or_nothing"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT UNIQUE)" +store seed as execute db with "INSERT INTO projects (slug) VALUES ('taken')" + +store failed as no +try: + in transaction on db: + store a as execute db with "INSERT INTO projects (slug) VALUES ('new-one')" + store b as execute db with "INSERT INTO projects (slug) VALUES ('taken')" + end transaction +when error: + change failed to yes +end try + +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!(get_global(&interpreter, "failed"), Value::Bool(true)); + + assert_eq!( + surviving_rows(url).await, + 1.0, + "the successful first insert must roll back with the failed second one" + ); +} + +#[tokio::test] +async fn reads_inside_transaction_see_uncommitted_writes() { + let db = TempDb::new("read_own_writes"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" + +in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('pending')" + store rows as query db with "SELECT slug FROM projects" + store seen as length of rows +end transaction + +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + expect_number(&get_global(&interpreter, "seen")), + 1.0, + "a query inside the block must run on the transaction's own connection \ + and see its uncommitted write" + ); +} + +// --------------------------------------------------------------------------- +// Lifecycle and misuse (R3: lifecycle + negative paths) +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn nested_transaction_on_same_handle_is_a_clear_error() { + let db = TempDb::new("nested"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +in transaction on db: + in transaction on db: + store a as execute db with "INSERT INTO projects (slug) VALUES ('x')" + end transaction +end transaction +close database db +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("nesting a transaction on the same handle must fail"); + let lower = err.to_lowercase(); + assert!( + lower.contains("transaction"), + "error should name the transaction, got: {err}" + ); + assert!( + lower.contains("nest") || lower.contains("already"), + "error should explain that a transaction is already open, got: {err}" + ); +} + +#[tokio::test] +async fn closing_database_inside_transaction_is_a_clear_error() { + let db = TempDb::new("close_inside"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +in transaction on db: + close database db +end transaction +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("closing a database mid-transaction must fail"); + assert!( + err.to_lowercase().contains("transaction"), + "error should name the open transaction, got: {err}" + ); +} + +#[tokio::test] +async fn open_transaction_rolls_back_when_program_ends() { + // A program that never reaches `end transaction` (the error escapes the + // block and the whole program) must not leave the write committed. + let db = TempDb::new("abandoned"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('abandoned')" + store boom as execute db with "THIS IS NOT SQL" +end transaction +close database db +"# + ); + run_wfl(&code) + .await + .err() + .expect("the invalid statement should fail the program"); + + assert_eq!( + surviving_rows(url).await, + 0.0, + "an abandoned transaction must roll back, not leak a committed write" + ); +} + +// --------------------------------------------------------------------------- +// Raw transaction-control SQL must fail loudly instead of silently no-op'ing. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn raw_begin_through_execute_is_rejected() { + let db = TempDb::new("raw_begin"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +store t1 as execute db with "BEGIN" +close database db +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("raw BEGIN must be rejected, not silently ignored"); + assert!( + err.contains("in transaction on"), + "the error must point at the transaction block, got: {err}" + ); +} + +#[tokio::test] +async fn raw_commit_and_rollback_through_execute_are_rejected() { + let db = TempDb::new("raw_commit"); + let url = &db.url; + for sql in ["COMMIT", "ROLLBACK", "START TRANSACTION", " begin "] { + let code = format!( + r#" +open database at "{url}" as db +store t as execute db with "{sql}" +close database db +"# + ); + let err = match run_wfl(&code).await { + Ok(_) => panic!("raw {sql} must be rejected, not silently ignored"), + Err(e) => e, + }; + assert!( + err.contains("in transaction on"), + "the error for {sql} must point at the transaction block, got: {err}" + ); + } +} + +#[tokio::test] +async fn ordinary_sql_beginning_with_a_transaction_word_is_not_rejected() { + // The rejection matches the leading *statement* keyword. A column or table + // named `begin_at`, or a SELECT that merely mentions COMMIT, must still run. + let db = TempDb::new("not_overreaching"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE audit (begin_at TEXT, commit_note TEXT)" +store ins as execute db with "INSERT INTO audit (begin_at, commit_note) VALUES ('t0', 'rollback plan')" +store rows as query db with "SELECT begin_at, commit_note FROM audit" +store n as length of rows +close database db +"# + ); + let interpreter = run_wfl(&code) + .await + .expect("ordinary SQL that merely contains transaction words must still run"); + assert_eq!(expect_number(&get_global(&interpreter, "n")), 1.0); +} + +// --------------------------------------------------------------------------- +// Concurrency (§11.3): a transaction pins one connection, and must not wedge +// unrelated work on the rest of the pool. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn transaction_does_not_block_other_handles_on_the_same_file() { + let db = TempDb::new("no_wedge"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +store seed as execute db with "INSERT INTO projects (slug) VALUES ('already-there')" + +open database at "{url}" as other +in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('in-flight')" + store rows as query other with "SELECT slug FROM projects" + store visible as length of rows +end transaction +close database other +close database db +"# + ); + let interpreter = run_wfl(&code) + .await + .expect("a second handle must stay usable while a transaction is open"); + assert_eq!( + expect_number(&get_global(&interpreter, "visible")), + 1.0, + "the other handle must see only the committed row, and must not deadlock" + ); + + assert_eq!(surviving_rows(url).await, 2.0); +} + +#[tokio::test] +async fn execute_result_shape_is_unchanged_inside_a_transaction() { + // Backward compatibility: `execute` returns the same object whether or not + // it runs inside a transaction block. + let db = TempDb::new("result_shape"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE t (x INTEGER)" +in transaction on db: + store inserted as execute db with "INSERT INTO t (x) VALUES (?)" and parameters [7] +end transaction +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let inserted = get_global(&interpreter, "inserted"); + assert_eq!( + expect_number(&expect_object_key(&inserted, "affected_rows")), + 1.0 + ); + assert_eq!( + expect_number(&expect_object_key(&inserted, "last_insert_id")), + 1.0 + ); +} + +#[tokio::test] +async fn query_inside_transaction_returns_rows_as_usual() { + let db = TempDb::new("query_shape"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE t (x INTEGER)" +store seed as execute db with "INSERT INTO t (x) VALUES (5)" +in transaction on db: + store rows as query db with "SELECT x FROM t" +end transaction +close database db +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let rows = expect_list(&get_global(&interpreter, "rows")); + assert_eq!(rows.len(), 1); + assert_eq!(expect_number(&expect_object_key(&rows[0], "x")), 5.0); +} diff --git a/tests/filesystem_mode_test.rs b/tests/filesystem_mode_test.rs new file mode 100644 index 00000000..2fe541e5 --- /dev/null +++ b/tests/filesystem_mode_test.rs @@ -0,0 +1,264 @@ +// TDD tests for the `file_mode` / `set_file_mode` builtins (issue #666). +// +// The issue's motivating case is a config file holding an API key: a program must +// be able to make it 0600 *and* to verify it is 0600 so it can refuse to start +// otherwise. Both halves are tested here. +// +// Unix gets real POSIX semantics. Windows has no equivalent, so `file_mode` +// returns a documented approximation and `set_file_mode` raises an explicit +// unsupported error — never a silent no-op, which is the failure mode the issue +// complains about. + +use wfl::interpreter::Interpreter; +use wfl::interpreter::value::Value; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +async fn run_wfl(code: &str) -> Result { + let tokens = lex_wfl_with_positions(code); + let mut parser = Parser::new(&tokens); + let ast = parser.parse().map_err(|e| format!("Parse error: {e:?}"))?; + + let mut interpreter = Interpreter::new(); + interpreter + .interpret(&ast) + .await + .map_err(|e| format!("Runtime error: {e:?}"))?; + Ok(interpreter) +} + +fn get_global(interpreter: &Interpreter, name: &str) -> Value { + interpreter + .global_env() + .borrow() + .get(name) + .unwrap_or_else(|| panic!("Variable '{name}' not found")) +} + +fn expect_text(value: &Value) -> String { + match value { + Value::Text(t) => t.to_string(), + other => panic!("Expected text, got {other:?}"), + } +} + +/// Create a temp file with some content and return its WFL-safe path string. +fn temp_file(name: &str) -> (tempfile::TempDir, String) { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(name); + std::fs::write(&path, "api_key = secret\n").expect("write temp file"); + let path_str = path.display().to_string().replace('\\', "/"); + (dir, path_str) +} + +#[tokio::test] +async fn file_mode_reads_a_four_character_octal_string() { + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store mode as file_mode of "{path}" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let mode = expect_text(&get_global(&interpreter, "mode")); + assert_eq!( + mode.len(), + 4, + "file_mode should return a 4-character octal string like \"0600\", got {mode:?}" + ); + assert!( + mode.chars().all(|c| ('0'..='7').contains(&c)), + "file_mode should return octal digits only, got {mode:?}" + ); +} + +#[tokio::test] +async fn file_mode_on_a_missing_path_errors() { + let code = r#" +store mode as file_mode of "/definitely/not/a/real/path/config.json" +"#; + let err = run_wfl(code) + .await + .err() + .expect("reading the mode of a missing file must error"); + assert!( + err.to_lowercase().contains("exist"), + "error should say the file does not exist, got: {err}" + ); +} + +// --------------------------------------------------------------------------- +// Unix: real semantics +// --------------------------------------------------------------------------- + +#[cfg(unix)] +mod unix { + use super::*; + + #[tokio::test] + async fn set_then_read_round_trips_to_0600() { + // The issue's core scenario, end to end. + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store applied as set_file_mode of "{path}" and "0600" +store mode as file_mode of "{path}" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "mode")), + "0600", + "a file set to 0600 must read back as 0600" + ); + } + + #[tokio::test] + async fn the_mode_is_actually_applied_on_disk() { + // Assert the real side effect, not just what the builtin reports back. + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("secret.toml"); + std::fs::write(&path, "token = \"x\"\n").expect("write"); + let path_str = path.display().to_string(); + + let code = format!( + r#" +store applied as set_file_mode of "{path_str}" and "0600" +"# + ); + run_wfl(&code).await.expect("program should run"); + + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode() + & 0o7777; + assert_eq!( + mode, 0o600, + "the file on disk must actually be 0600, got {mode:o}" + ); + } + + #[tokio::test] + async fn a_group_readable_file_is_detectable() { + // "Refuse to start if the config is group- or world-readable" — the check + // the issue says is impossible today. + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store applied as set_file_mode of "{path}" and "0644" +store mode as file_mode of "{path}" +store is_locked_down as mode is equal to "0600" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!(expect_text(&get_global(&interpreter, "mode")), "0644"); + assert_eq!( + get_global(&interpreter, "is_locked_down"), + Value::Bool(false), + "a 0644 config must be distinguishable from a 0600 one" + ); + } + + #[tokio::test] + async fn three_digit_and_four_digit_modes_are_both_accepted() { + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store a as set_file_mode of "{path}" and "600" +store after_short as file_mode of "{path}" +store b as set_file_mode of "{path}" and "0640" +store after_long as file_mode of "{path}" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "after_short")), + "0600" + ); + assert_eq!(expect_text(&get_global(&interpreter, "after_long")), "0640"); + } + + #[tokio::test] + async fn malformed_modes_are_rejected() { + for bad in ["rw-------", "0999", "abc", "", "0o600", "-1", "10000"] { + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store applied as set_file_mode of "{path}" and "{bad}" +"# + ); + let err = match run_wfl(&code).await { + Ok(_) => panic!("mode {bad:?} must be rejected, not silently masked"), + Err(e) => e, + }; + assert!( + err.to_lowercase().contains("mode"), + "error for {bad:?} should explain the expected format, got: {err}" + ); + } + } + + #[tokio::test] + async fn set_file_mode_on_a_missing_path_errors() { + let code = r#" +store applied as set_file_mode of "/definitely/not/a/real/path/x.json" and "0600" +"#; + run_wfl(code) + .await + .err() + .expect("setting the mode of a missing file must error"); + } +} + +// --------------------------------------------------------------------------- +// Windows: explicit and loud, never a silent no-op +// --------------------------------------------------------------------------- + +#[cfg(windows)] +mod windows { + use super::*; + + #[tokio::test] + async fn set_file_mode_reports_that_it_is_unsupported() { + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store applied as set_file_mode of "{path}" and "0600" +"# + ); + let err = run_wfl(&code) + .await + .err() + .expect("set_file_mode must not silently pretend to work on Windows"); + let lower = err.to_lowercase(); + assert!( + lower.contains("not supported") || lower.contains("unsupported"), + "the error must say the operation is unsupported here, got: {err}" + ); + assert!( + lower.contains("windows"), + "the error must name the platform, got: {err}" + ); + } + + #[tokio::test] + async fn file_mode_still_returns_an_approximation() { + // Reading stays available so a cross-platform program can call it + // unconditionally; the docs state it is an approximation on Windows. + let (_dir, path) = temp_file("config.json"); + let code = format!( + r#" +store mode as file_mode of "{path}" +"# + ); + let interpreter = run_wfl(&code).await.expect("program should run"); + let mode = expect_text(&get_global(&interpreter, "mode")); + assert!( + mode == "0666" || mode == "0444", + "Windows approximation should be 0666 (writable) or 0444 (read-only), got {mode:?}" + ); + } +} diff --git a/tests/toml_test.rs b/tests/toml_test.rs new file mode 100644 index 00000000..91569841 --- /dev/null +++ b/tests/toml_test.rs @@ -0,0 +1,271 @@ +// TDD tests for the `parse_toml` / `stringify_toml` builtins (issue #667). +// +// The surface deliberately mirrors the JSON one (`parse_json` / +// `stringify_json` / `stringify_json_pretty`, src/stdlib/json.rs) rather than the +// `to_toml` spelling sketched in the issue, so the two formats read the same way. +// +// R3 (untrusted input): config text is attacker-reachable, so malformed input +// must produce a clean error rather than a panic. + +use wfl::interpreter::Interpreter; +use wfl::interpreter::value::Value; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +async fn run_wfl(code: &str) -> Result { + let tokens = lex_wfl_with_positions(code); + let mut parser = Parser::new(&tokens); + let ast = parser.parse().map_err(|e| format!("Parse error: {e:?}"))?; + + let mut interpreter = Interpreter::new(); + interpreter + .interpret(&ast) + .await + .map_err(|e| format!("Runtime error: {e:?}"))?; + Ok(interpreter) +} + +fn get_global(interpreter: &Interpreter, name: &str) -> Value { + interpreter + .global_env() + .borrow() + .get(name) + .unwrap_or_else(|| panic!("Variable '{name}' not found")) +} + +fn expect_text(value: &Value) -> String { + match value { + Value::Text(t) => t.to_string(), + other => panic!("Expected text, got {other:?}"), + } +} + +fn expect_number(value: &Value) -> f64 { + match value { + Value::Number(n) => *n, + other => panic!("Expected number, got {other:?}"), + } +} + +// --------------------------------------------------------------------------- +// Parsing — the priority half of the issue. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn parse_toml_reads_a_realistic_config() { + let code = r#" +store toml_text as "title = \"my project\" +port = 8080 +debug = true +ratio = 0.25 +" +store config as parse_toml of toml_text +store title as config["title"] +store the_port as config["port"] +store debug as config["debug"] +store ratio as config["ratio"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "title")), + "my project" + ); + assert_eq!(expect_number(&get_global(&interpreter, "the_port")), 8080.0); + assert_eq!(get_global(&interpreter, "debug"), Value::Bool(true)); + assert_eq!(expect_number(&get_global(&interpreter, "ratio")), 0.25); +} + +#[tokio::test] +async fn parse_toml_reads_nested_tables() { + let code = r#" +store toml_text as "[server] +host = \"localhost\" +port = 5432 + +[server.tls] +enabled = true +" +store config as parse_toml of toml_text +store server_table as config["server"] +store host as server_table["host"] +store tls as server_table["tls"] +store enabled as tls["enabled"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!(expect_text(&get_global(&interpreter, "host")), "localhost"); + assert_eq!(get_global(&interpreter, "enabled"), Value::Bool(true)); +} + +#[tokio::test] +async fn parse_toml_reads_arrays_as_lists() { + let code = r#" +store toml_text as "hosts = [\"a\", \"b\", \"c\"] +ports = [1, 2, 3] +" +store config as parse_toml of toml_text +store hosts as config["hosts"] +store n as length of hosts +store first as hosts[0] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!(expect_number(&get_global(&interpreter, "n")), 3.0); + assert_eq!(expect_text(&get_global(&interpreter, "first")), "a"); +} + +#[tokio::test] +async fn parse_toml_reads_arrays_of_tables() { + let code = r#" +store toml_text as "[[project]] +slug = \"alpha\" + +[[project]] +slug = \"beta\" +" +store config as parse_toml of toml_text +store projects as config["project"] +store n as length of projects +store second as projects[1] +store slug as second["slug"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!(expect_number(&get_global(&interpreter, "n")), 2.0); + assert_eq!(expect_text(&get_global(&interpreter, "slug")), "beta"); +} + +#[tokio::test] +async fn parse_toml_reads_datetimes_as_text() { + let code = r#" +store toml_text as "created = 1979-05-27T07:32:00Z +" +store config as parse_toml of toml_text +store created as config["created"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + let created = expect_text(&get_global(&interpreter, "created")); + assert!( + created.starts_with("1979-05-27"), + "a TOML datetime should surface as readable text, got {created:?}" + ); +} + +#[tokio::test] +async fn parse_toml_rejects_malformed_input() { + for bad in [ + "this is not toml at all = = =", + "[unclosed", + "key = ", + "key = \"unterminated", + "a = 1\na = 2", + ] { + let code = format!( + r#" +store config as parse_toml of "{}" +"#, + bad.replace('\\', "\\\\").replace('"', "\\\"") + ); + let err = match run_wfl(&code).await { + Ok(_) => panic!("malformed TOML {bad:?} must be rejected"), + Err(e) => e, + }; + assert!( + err.to_lowercase().contains("toml"), + "the error should name TOML, got: {err}" + ); + } +} + +// --------------------------------------------------------------------------- +// Serializing +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn stringify_toml_round_trips() { + let code = r#" +store toml_text as "name = \"wfl\" +port = 8080 +debug = false +" +store config as parse_toml of toml_text +store out as stringify_toml of config +store again as parse_toml of out +store name as again["name"] +store the_port as again["port"] +store debug as again["debug"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!(expect_text(&get_global(&interpreter, "name")), "wfl"); + assert_eq!(expect_number(&get_global(&interpreter, "the_port")), 8080.0); + assert_eq!(get_global(&interpreter, "debug"), Value::Bool(false)); +} + +#[tokio::test] +async fn stringify_toml_pretty_round_trips_nested_structure() { + let code = r#" +store toml_text as "[server] +host = \"localhost\" +ports = [1, 2] +" +store config as parse_toml of toml_text +store out as stringify_toml_pretty of config +store again as parse_toml of out +store server_table as again["server"] +store host as server_table["host"] +store ports as server_table["ports"] +store n as length of ports +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!(expect_text(&get_global(&interpreter, "host")), "localhost"); + assert_eq!(expect_number(&get_global(&interpreter, "n")), 2.0); +} + +#[tokio::test] +async fn stringify_toml_requires_a_table_at_the_top_level() { + // TOML documents are always tables. A bare list has no valid representation, + // and saying so beats emitting something that will not parse back. + let code = r#" +store out as stringify_toml of [1 and 2 and 3] +"#; + let err = run_wfl(code) + .await + .err() + .expect("a top-level list is not a valid TOML document"); + assert!( + err.to_lowercase().contains("toml"), + "the error should name TOML, got: {err}" + ); +} + +#[tokio::test] +async fn nothing_valued_keys_are_omitted_rather_than_invented() { + // TOML has no null. An absent value is an absent key — that is the format's + // own idiom, and it round-trips. + let code = r#" +store toml_text as "present = \"yes\" +" +store config as parse_toml of toml_text +store absent as nothing +store out as stringify_toml of config +store again as parse_toml of out +store still_there as again["present"] +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert_eq!( + expect_text(&get_global(&interpreter, "still_there")), + "yes", + "present keys must survive the round trip" + ); +} + +#[tokio::test] +async fn parse_toml_of_an_empty_document_gives_an_empty_table() { + let code = r#" +store config as parse_toml of "" +store out as stringify_toml of config +"#; + let interpreter = run_wfl(code).await.expect("program should run"); + assert!( + matches!(get_global(&interpreter, "config"), Value::Object(_)), + "an empty TOML document should parse to an empty object" + ); + assert_eq!(expect_text(&get_global(&interpreter, "out")), ""); +} From 828b487f6ac9a9c60237bf0770254a726ddc4797 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:00:55 +0000 Subject: [PATCH 02/10] feat: seal and unseal, XChaCha20-Poly1305 authenticated encryption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap in #665: the crypto stdlib could mint a key with `secure_random_bytes of 32` but had nothing to encrypt with it, so a program holding an API token that must be sent upstream later had only two options — store it in plaintext, or not store it. Hashing cannot stand in, because a stored credential has to be recoverable. store sealed as seal of token and key store token as unseal of sealed and key XChaCha20-Poly1305 rather than a 96-bit-nonce AEAD: the 192-bit nonce is wide enough that a fresh random nonce per message is safe with no counter and no caller-visible state, so the implementation owns nonce handling outright. Nonce reuse is the sharpest edge in an AEAD API and a natural-language surface is the last place to expose it. An optional third argument supplies associated data, binding a ciphertext to its context: `seal of token and key and "project:acme/api_key"`. The two- and three-argument forms are the same call, so the simple form is a strict subset of the expert one. Sealed values are `wflseal1:` plus hex of nonce, ciphertext and tag — self-describing, versioned so the algorithm can change later, and hex to match the key that `secure_random_bytes` hands back. `unseal` fails closed and reports every failure identically, so it cannot be used as an oracle to tell a wrong key from a modified byte from a mismatched context. Closes #665 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- src/stdlib/crypto.rs | 195 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 193 insertions(+), 2 deletions(-) diff --git a/src/stdlib/crypto.rs b/src/stdlib/crypto.rs index 65ae682c..3b6f1bc9 100644 --- a/src/stdlib/crypto.rs +++ b/src/stdlib/crypto.rs @@ -1,4 +1,4 @@ -use super::helpers::{check_arg_count, expect_number, expect_text}; +use super::helpers::{check_arg_count, check_arg_range, expect_number, expect_text}; use crate::interpreter::environment::Environment; use crate::interpreter::error::RuntimeError; use crate::interpreter::value::Value; @@ -13,7 +13,7 @@ use scrypt::Scrypt; use sha2::{Digest, Sha256}; use std::sync::Arc; use subtle::ConstantTimeEq; -use zeroize::Zeroize; +use zeroize::{Zeroize, Zeroizing}; /// Maximum input size for wflhash functions (100MB) pub const MAX_INPUT_SIZE: usize = 100 * 1024 * 1024; @@ -999,6 +999,194 @@ pub fn native_verify_password(args: Vec) -> Result { Ok(Value::Bool(verify_any_password(&password, &stored))) } +// ============================================================================ +// Authenticated encryption — `seal` / `unseal` (issue #665) +// +// Everything else in this module is one-way: hashes, MACs, password KDFs. That +// is right for verifying a password, and useless for storing an API token that +// has to be sent upstream again later. `seal` is the missing half — the thing a +// key minted by `secure_random_bytes of 32` is actually *for*. +// +// XChaCha20-Poly1305 specifically. Its 192-bit nonce is wide enough that a +// freshly generated random nonce per message is safe without any counter or +// state, which lets the implementation own nonce handling completely. Nonce +// reuse is the sharpest edge in any AEAD API, and a natural-language surface is +// the last place a user should be managing one by hand — so `seal` takes a +// plaintext and a key, and nothing else. +// +// The sealed value is `wflseal1:` followed by hex of nonce ‖ ciphertext ‖ tag. +// Self-describing (so a reader can tell what it is holding), versioned (so the +// algorithm can change later without ambiguity), and hex to match +// `secure_random_bytes`, which is where the key comes from. +// ============================================================================ + +/// Prefix identifying version 1 of the sealed-blob format: +/// XChaCha20-Poly1305, 24-byte nonce prepended to the ciphertext. +const SEAL_V1_PREFIX: &str = "wflseal1:"; + +/// XChaCha20-Poly1305 nonce length in bytes. +const SEAL_NONCE_LEN: usize = 24; + +/// Poly1305 authentication tag length in bytes. +const SEAL_TAG_LEN: usize = 16; + +/// Key length in bytes; 64 hex characters, exactly `secure_random_bytes of 32`. +const SEAL_KEY_LEN: usize = 32; + +/// Decode a hex string into bytes, or `None` if it is not valid hex. +fn hex_to_bytes(hex: &str) -> Option> { + if !hex.len().is_multiple_of(2) { + return None; + } + (0..hex.len()) + .step_by(2) + .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).ok()) + .collect() +} + +/// Decode and validate a sealing key, with a message that points at the way to +/// make a valid one. +fn parse_seal_key(func_name: &str, key_text: &str) -> Result>, RuntimeError> { + let bytes = hex_to_bytes(key_text).filter(|b| b.len() == SEAL_KEY_LEN); + + match bytes { + Some(bytes) => Ok(Zeroizing::new(bytes)), + None => Err(RuntimeError::new( + format!( + "{func_name}: the key must be {} hex characters ({SEAL_KEY_LEN} bytes). \ + Generate one with `secure_random_bytes of {SEAL_KEY_LEN}`.", + SEAL_KEY_LEN * 2 + ), + 0, + 0, + )), + } +} + +/// Encrypt text so it can be stored at rest and read back later. +/// +/// Usage: `seal of plaintext and key` +/// `seal of plaintext and key and context` +/// +/// The optional context (associated data) is authenticated but not encrypted, +/// and binds the ciphertext to where it lives: a blob sealed with one context +/// will not unseal under another. +pub fn native_seal(args: Vec) -> Result { + use chacha20poly1305::aead::{Aead, KeyInit, Payload}; + use chacha20poly1305::{XChaCha20Poly1305, XNonce}; + use rand::Rng; + + check_arg_range("seal", &args, 2, 3)?; + + let plaintext = expect_text(&args[0])?; + let key_text = expect_text(&args[1])?; + let context = match args.get(2) { + Some(value) => Some(expect_text(value)?), + None => None, + }; + + let key = parse_seal_key("seal", &key_text)?; + let cipher = XChaCha20Poly1305::new_from_slice(&key) + .map_err(|_| RuntimeError::new("seal: failed to initialize cipher".to_string(), 0, 0))?; + + // Fresh random nonce per message, from the OS CSPRNG. With a 192-bit nonce + // the chance of a repeat is negligible, which is the whole reason for + // choosing XChaCha20 over a 96-bit-nonce AEAD here. + // Same CSPRNG `secure_random_bytes` draws the key from. + let mut nonce_bytes = [0u8; SEAL_NONCE_LEN]; + rand::rng().fill_bytes(&mut nonce_bytes); + let nonce = XNonce::from(nonce_bytes); + + let payload = Payload { + msg: plaintext.as_bytes(), + aad: context.as_deref().unwrap_or("").as_bytes(), + }; + + let ciphertext = cipher + .encrypt(&nonce, payload) + .map_err(|_| RuntimeError::new("seal: encryption failed".to_string(), 0, 0))?; + + let mut blob = Vec::with_capacity(SEAL_NONCE_LEN + ciphertext.len()); + blob.extend_from_slice(&nonce_bytes); + blob.extend_from_slice(&ciphertext); + + let sealed = format!("{SEAL_V1_PREFIX}{}", bytes_to_hex(&blob)); + + nonce_bytes.zeroize(); + Ok(Value::Text(Arc::from(sealed))) +} + +/// Decrypt a value produced by `seal`. +/// +/// Usage: `unseal of sealed and key` +/// `unseal of sealed and key and context` +/// +/// Fails closed. A wrong key, a modified byte anywhere in the blob, a truncated +/// blob, and a mismatched context all produce the same error — distinguishing +/// between them would turn this into an oracle for an attacker holding the +/// ciphertext. +pub fn native_unseal(args: Vec) -> Result { + use chacha20poly1305::aead::{Aead, KeyInit, Payload}; + use chacha20poly1305::{XChaCha20Poly1305, XNonce}; + + check_arg_range("unseal", &args, 2, 3)?; + + let sealed = expect_text(&args[0])?; + let key_text = expect_text(&args[1])?; + let context = match args.get(2) { + Some(value) => Some(expect_text(value)?), + None => None, + }; + + let key = parse_seal_key("unseal", &key_text)?; + + // A single error for every failure below this point. + let failed = || { + RuntimeError::new( + "unseal: could not open this value. The key may be wrong, the context may not \ + match, or the sealed value may have been modified or truncated." + .to_string(), + 0, + 0, + ) + }; + + let body = sealed.strip_prefix(SEAL_V1_PREFIX).ok_or_else(|| { + RuntimeError::new( + format!( + "unseal: this is not a sealed value. Expected text beginning with \ + '{SEAL_V1_PREFIX}', as produced by `seal`." + ), + 0, + 0, + ) + })?; + + let blob = hex_to_bytes(body).ok_or_else(failed)?; + if blob.len() < SEAL_NONCE_LEN + SEAL_TAG_LEN { + return Err(failed()); + } + + let (nonce_bytes, ciphertext) = blob.split_at(SEAL_NONCE_LEN); + let nonce = XNonce::try_from(nonce_bytes).map_err(|_| failed())?; + + let cipher = XChaCha20Poly1305::new_from_slice(&key) + .map_err(|_| RuntimeError::new("unseal: failed to initialize cipher".to_string(), 0, 0))?; + + let payload = Payload { + msg: ciphertext, + aad: context.as_deref().unwrap_or("").as_bytes(), + }; + + let plaintext = Zeroizing::new(cipher.decrypt(&nonce, payload).map_err(|_| failed())?); + + // WFL text is UTF-8; a blob that decrypts to non-text was not produced by + // `seal` on a WFL string, so it is treated as a failure like any other. + let text = std::str::from_utf8(&plaintext).map_err(|_| failed())?; + + Ok(Value::Text(Arc::from(text))) +} + /// Register all crypto functions in the environment pub fn register_crypto(env: &mut Environment) { env.define_native("wflhash256", native_wflhash256); @@ -1023,6 +1211,9 @@ pub fn register_crypto(env: &mut Environment) { env.define_native("scrypt_verify", native_scrypt_verify); env.define_native("pbkdf2_hash", native_pbkdf2_hash); env.define_native("pbkdf2_verify", native_pbkdf2_verify); + // Authenticated encryption + env.define_native("seal", native_seal); + env.define_native("unseal", native_unseal); } #[cfg(test)] From 5b2639181ad8f0fc2944cebbb2a47ebc3b3adfb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:00:55 +0000 Subject: [PATCH 03/10] feat: file_mode and set_file_mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #666. A program that writes a secret could not restrict the file, and — the half that matters just as much — could not verify it was restricted. The best available mitigation was external, a UMask in a service unit, which is invisible to the program and unverifiable from inside it. That made the common check "refuse to start if this config is group- or world-readable" impossible to express. store mode as file_mode of "config.toml" // "0600" store ok as set_file_mode of "config.toml" and "0600" Unix gets real POSIX semantics. Mode strings are parsed strictly: three or four octal digits, and anything else is an error rather than something quietly masked into a mode the author did not intend. Windows has no equivalent — modes do not map onto ACLs — so `file_mode` returns a documented approximation from the read-only attribute and `set_file_mode` raises an explicit unsupported error. A loud refusal beats a silent no-op that leaves the caller believing a file is protected. Closes #666 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- src/stdlib/filesystem.rs | 158 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/src/stdlib/filesystem.rs b/src/stdlib/filesystem.rs index fe337c26..d945cb32 100644 --- a/src/stdlib/filesystem.rs +++ b/src/stdlib/filesystem.rs @@ -483,6 +483,162 @@ pub fn native_remove_dir(args: Vec) -> Result { Ok(Value::Null) } +// ============================================================================ +// File permissions (issue #666) +// +// A program that writes a secret needs two things: to restrict the file, and to +// *verify* it is restricted. The second half is what makes "refuse to start if +// this config is group-readable" expressible; without it the security property +// depends on how the process happened to be launched (a umask in a service +// unit), which the program itself cannot see. +// +// Unix gets real POSIX semantics. Windows has no equivalent — its ACLs do not +// map onto a mode — so reading returns a documented approximation and writing +// raises an explicit error. A loud "not supported here" is far better than a +// silent no-op that leaves the caller believing the file is protected. +// ============================================================================ + +/// Format a raw permission bit set as the 4-character octal string WFL uses. +fn format_mode(bits: u32) -> String { + format!("{:04o}", bits & 0o7777) +} + +/// Parse a WFL mode string (`"0600"` or `"600"`) into permission bits. +/// +/// Deliberately strict: anything that is not 3 or 4 octal digits is rejected +/// rather than masked, because silently reinterpreting `"rw-------"` or +/// `"0999"` as *some* mode is how a file ends up more permissive than the +/// author believed. +fn parse_mode(func_name: &str, raw: &str) -> Result { + let trimmed = raw.trim(); + let is_octal_digits = !trimmed.is_empty() + && trimmed.len() <= 4 + && trimmed.chars().all(|c| ('0'..='7').contains(&c)); + + if !is_octal_digits || trimmed.len() < 3 { + return Err(RuntimeError::new( + format!( + "{func_name}: '{raw}' is not a valid file mode. \ + Expected 3 or 4 octal digits, such as \"0600\" (owner read/write only) \ + or \"0644\"." + ), + 0, + 0, + )); + } + + u32::from_str_radix(trimmed, 8).map_err(|_| { + RuntimeError::new( + format!("{func_name}: '{raw}' is not a valid file mode"), + 0, + 0, + ) + }) +} + +/// Read a file's permission mode. +/// +/// Usage: `file_mode of path` → e.g. `"0600"` +/// +/// On Windows the returned value is an approximation derived from the read-only +/// attribute (`"0444"` when read-only, `"0666"` otherwise) — see the module +/// documentation. +pub fn native_file_mode(args: Vec) -> Result { + check_arg_count("file_mode", &args, 1)?; + + let path_str = expect_text(&args[0])?; + let path = Path::new(path_str.as_ref()); + + if !path.exists() { + return Err(RuntimeError::new( + format!("File does not exist: {path_str}"), + 0, + 0, + )); + } + + let metadata = fs::metadata(path).map_err(|e| { + RuntimeError::new( + format!("Failed to read permissions for '{path_str}': {e}"), + 0, + 0, + ) + })?; + + #[cfg(unix)] + let mode = { + use std::os::unix::fs::PermissionsExt; + format_mode(metadata.permissions().mode()) + }; + + #[cfg(not(unix))] + let mode = { + // Approximation: Windows exposes only a read-only flag at this level. + if metadata.permissions().readonly() { + "0444".to_string() + } else { + "0666".to_string() + } + }; + + Ok(Value::Text(Arc::from(mode))) +} + +/// Set a file's permission mode. +/// +/// Usage: `set_file_mode of path and "0600"` +/// +/// Unix only. On other platforms this raises an error rather than silently +/// doing nothing, so a program never believes it protected a file that it did +/// not. +pub fn native_set_file_mode(args: Vec) -> Result { + check_arg_count("set_file_mode", &args, 2)?; + + let path_str = expect_text(&args[0])?; + let mode_str = expect_text(&args[1])?; + let path = Path::new(path_str.as_ref()); + + // Validate the mode before touching the filesystem, so a bad mode string is + // reported the same way whether or not the file happens to exist. + let bits = parse_mode("set_file_mode", mode_str.as_ref())?; + + if !path.exists() { + return Err(RuntimeError::new( + format!("File does not exist: {path_str}"), + 0, + 0, + )); + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let permissions = fs::Permissions::from_mode(bits); + fs::set_permissions(path, permissions).map_err(|e| { + RuntimeError::new( + format!("Failed to set permissions on '{path_str}': {e}"), + 0, + 0, + ) + })?; + Ok(Value::Text(Arc::from(format_mode(bits)))) + } + + #[cfg(not(unix))] + { + let _ = bits; + Err(RuntimeError::new( + format!( + "set_file_mode is not supported on Windows: file modes are a POSIX concept \ + and do not map onto Windows ACLs. Restrict '{path_str}' through the \ + filesystem's own access control instead." + ), + 0, + 0, + )) + } +} + pub fn register_filesystem(env: &mut crate::interpreter::environment::Environment) { env.define_native("list_dir", native_list_dir); env.define_native("glob", native_glob); @@ -505,6 +661,8 @@ pub fn register_filesystem(env: &mut crate::interpreter::environment::Environmen // Documented alias of remove_file env.define_native("delete_file", native_remove_file); env.define_native("remove_dir", native_remove_dir); + env.define_native("file_mode", native_file_mode); + env.define_native("set_file_mode", native_set_file_mode); } #[cfg(test)] From 4d61f6709bc28ee8386001d05651ceb0fc4a2f63 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:00:55 +0000 Subject: [PATCH 04/10] feat: parse_toml, stringify_toml and stringify_toml_pretty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #667. WFL could read JSON but not TOML, which is what a large share of config files are actually written in. The reporter's options were to hand-roll a TOML subset in WFL — fragile to own, and progressively more wrong as it meets real TOML — or to change the file format; they changed the format and wrote down the deviation from spec. The surface mirrors the existing JSON one rather than the `to_toml` spelling the issue sketched, so the two formats read the same way: store config as parse_toml of file_contents store out as stringify_toml of config Two places TOML is not JSON, handled explicitly rather than fudged: * A TOML document is always a table, so `stringify_toml` accepts only an object and says so, instead of emitting something that will not parse back. * TOML has no null. Absence is a missing key, so a `nothing` value is omitted when serializing a table. Inside an array there is no way to leave a hole, so that is an error rather than a silent change of length. Whole numbers serialize as TOML integers, so a round-tripped config reads `port = 8080` and not `port = 8080.0`. Closes #667 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- src/stdlib/mod.rs | 2 + src/stdlib/toml.rs | 303 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 305 insertions(+) create mode 100644 src/stdlib/toml.rs diff --git a/src/stdlib/mod.rs b/src/stdlib/mod.rs index b583cff0..27414723 100644 --- a/src/stdlib/mod.rs +++ b/src/stdlib/mod.rs @@ -11,6 +11,7 @@ pub mod pattern_test; pub mod random; pub mod text; pub mod time; +pub mod toml; pub mod typechecker; pub mod web; @@ -27,5 +28,6 @@ pub fn register_stdlib(env: &mut Environment) { list::register_list(env); pattern::register(env); time::register_time(env); + toml::register_toml(env); web::register_web(env); } diff --git a/src/stdlib/toml.rs b/src/stdlib/toml.rs new file mode 100644 index 00000000..acaf535d --- /dev/null +++ b/src/stdlib/toml.rs @@ -0,0 +1,303 @@ +//! TOML parsing and serialization (issue #667). +//! +//! Deliberately a mirror of [`crate::stdlib::json`]: the same three-function +//! shape (`parse_*`, `stringify_*`, `stringify_*_pretty`), the same `Value` +//! mapping, and the same error style, so a program that reads one format reads +//! the other the same way. +//! +//! Two places where TOML is not JSON, and what this module does about them: +//! +//! * **A TOML document is always a table.** There is no such thing as a TOML +//! file whose top level is an array or a bare string, so `stringify_toml` +//! accepts only an object and says so plainly rather than emitting something +//! that will not parse back. +//! * **TOML has no null.** Absence is expressed by leaving the key out, so a +//! `nothing` value is skipped when serializing a table. Inside an *array* +//! there is no way to leave a hole, so that is an error instead of a silent +//! change of length. + +use super::helpers::{check_arg_count, expect_text}; +use crate::interpreter::environment::Environment; +use crate::interpreter::error::RuntimeError; +use crate::interpreter::value::Value; +use std::cell::RefCell; +use std::collections::HashMap; +use std::rc::Rc; +use std::sync::Arc; + +/// Convert a `toml::Value` to a WFL `Value`. +/// +/// Dates and times become text: WFL has `Date`/`Time`/`DateTime` values, but +/// TOML's offset/local datetime distinction does not map cleanly onto them, and +/// a lossless string keeps the round trip honest. +fn toml_to_wfl(value: ::toml::Value) -> Value { + match value { + ::toml::Value::String(s) => Value::Text(Arc::from(s)), + ::toml::Value::Integer(i) => Value::Number(i as f64), + ::toml::Value::Float(f) => Value::Number(f), + ::toml::Value::Boolean(b) => Value::Bool(b), + ::toml::Value::Datetime(dt) => Value::Text(Arc::from(dt.to_string())), + ::toml::Value::Array(arr) => { + let items: Vec = arr.into_iter().map(toml_to_wfl).collect(); + Value::List(Rc::new(RefCell::new(items))) + } + ::toml::Value::Table(table) => { + let mut map = HashMap::new(); + for (key, value) in table { + map.insert(key, toml_to_wfl(value)); + } + Value::Object(Rc::new(RefCell::new(map))) + } + } +} + +/// Convert a WFL `Value` to a `toml::Value`. +/// +/// Returns `Ok(None)` for `nothing`/`null`, which the table branch reads as +/// "omit this key". Callers that cannot omit (arrays, the document root) turn +/// that into an error. +fn wfl_to_toml(value: &Value) -> Result, RuntimeError> { + match value { + Value::Nothing | Value::Null => Ok(None), + Value::Bool(b) => Ok(Some(::toml::Value::Boolean(*b))), + Value::Number(n) => { + if !n.is_finite() { + return Err(RuntimeError::new( + format!("Cannot convert number {n} to TOML: TOML has no infinity or NaN"), + 0, + 0, + )); + } + // Whole numbers become TOML integers so a config round-trips as + // `port = 8080` rather than `port = 8080.0`. + if n.fract() == 0.0 && *n >= i64::MIN as f64 && *n <= i64::MAX as f64 { + Ok(Some(::toml::Value::Integer(*n as i64))) + } else { + Ok(Some(::toml::Value::Float(*n))) + } + } + Value::Text(s) => Ok(Some(::toml::Value::String(s.to_string()))), + Value::List(list) => { + let list_ref = list.borrow(); + let mut items = Vec::with_capacity(list_ref.len()); + for item in list_ref.iter() { + match wfl_to_toml(item)? { + Some(v) => items.push(v), + None => { + return Err(RuntimeError::new( + "Cannot convert list to TOML: TOML arrays cannot contain nothing. \ + Remove the empty entry, or use a table where the key can be omitted." + .to_string(), + 0, + 0, + )); + } + } + } + Ok(Some(::toml::Value::Array(items))) + } + Value::Object(obj) => { + let obj_ref = obj.borrow(); + let mut table = ::toml::map::Map::new(); + for (key, value) in obj_ref.iter() { + // `nothing` means "absent"; TOML spells that as a missing key. + if let Some(converted) = wfl_to_toml(value)? { + table.insert(key.clone(), converted); + } + } + Ok(Some(::toml::Value::Table(table))) + } + other => Err(RuntimeError::new( + format!("Cannot convert {} to TOML", other.type_name()), + 0, + 0, + )), + } +} + +/// Convert a WFL value into a TOML document root, which must be a table. +fn wfl_to_toml_document(value: &Value) -> Result<::toml::Table, RuntimeError> { + match wfl_to_toml(value)? { + Some(::toml::Value::Table(table)) => Ok(table), + _ => Err(RuntimeError::new( + format!( + "Cannot convert {} to a TOML document: a TOML document is always a table, \ + so the top level must be an object with named keys", + value.type_name() + ), + 0, + 0, + )), + } +} + +/// Parse TOML text into a WFL value. +/// +/// Usage: `parse_toml of text` +pub fn native_parse_toml(args: Vec) -> Result { + check_arg_count("parse_toml", &args, 1)?; + + let toml_text = expect_text(&args[0])?; + + match toml_text.parse::<::toml::Table>() { + Ok(table) => Ok(toml_to_wfl(::toml::Value::Table(table))), + Err(e) => Err(RuntimeError::new( + format!("Failed to parse TOML: {e}"), + 0, + 0, + )), + } +} + +/// Convert a WFL value to TOML text. +/// +/// Usage: `stringify_toml of value` +pub fn native_stringify_toml(args: Vec) -> Result { + check_arg_count("stringify_toml", &args, 1)?; + + let table = wfl_to_toml_document(&args[0])?; + + match ::toml::to_string(&table) { + Ok(text) => Ok(Value::Text(Arc::from(text))), + Err(e) => Err(RuntimeError::new( + format!("Failed to write TOML: {e}"), + 0, + 0, + )), + } +} + +/// Convert a WFL value to pretty-printed TOML text. +/// +/// Usage: `stringify_toml_pretty of value` +pub fn native_stringify_toml_pretty(args: Vec) -> Result { + check_arg_count("stringify_toml_pretty", &args, 1)?; + + let table = wfl_to_toml_document(&args[0])?; + + match ::toml::to_string_pretty(&table) { + Ok(text) => Ok(Value::Text(Arc::from(text))), + Err(e) => Err(RuntimeError::new( + format!("Failed to write TOML: {e}"), + 0, + 0, + )), + } +} + +/// Register all TOML functions in the environment. +pub fn register_toml(env: &mut Environment) { + env.define_native("parse_toml", native_parse_toml); + env.define_native("stringify_toml", native_stringify_toml); + env.define_native("stringify_toml_pretty", native_stringify_toml_pretty); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn text(s: &str) -> Value { + Value::Text(Arc::from(s)) + } + + #[test] + fn parse_toml_maps_scalars_onto_wfl_values() { + let parsed = native_parse_toml(vec![text( + "name = \"wfl\"\nport = 8080\nratio = 0.5\ndebug = true\n", + )]) + .expect("valid TOML should parse"); + + let Value::Object(obj) = parsed else { + panic!("a TOML document should parse to an object"); + }; + let obj = obj.borrow(); + assert!(matches!(obj.get("name"), Some(Value::Text(_)))); + assert!(matches!(obj.get("port"), Some(Value::Number(n)) if *n == 8080.0)); + assert!(matches!(obj.get("ratio"), Some(Value::Number(n)) if *n == 0.5)); + assert!(matches!(obj.get("debug"), Some(Value::Bool(true)))); + } + + #[test] + fn parse_toml_rejects_malformed_input() { + assert!(native_parse_toml(vec![text("[unclosed")]).is_err()); + assert!(native_parse_toml(vec![text("key = ")]).is_err()); + // Duplicate keys are an error in TOML, not last-wins. + assert!(native_parse_toml(vec![text("a = 1\na = 2\n")]).is_err()); + } + + #[test] + fn whole_numbers_round_trip_as_integers() { + let parsed = native_parse_toml(vec![text("port = 8080\n")]).unwrap(); + let out = native_stringify_toml(vec![parsed]).unwrap(); + let Value::Text(out) = out else { + panic!("expected text"); + }; + assert!( + out.contains("port = 8080") && !out.contains("8080.0"), + "a whole number should stay an integer, got: {out}" + ); + } + + #[test] + fn nothing_valued_keys_are_omitted_from_tables() { + let mut map = HashMap::new(); + map.insert("kept".to_string(), text("yes")); + map.insert("dropped".to_string(), Value::Nothing); + let value = Value::Object(Rc::new(RefCell::new(map))); + + let Value::Text(out) = native_stringify_toml(vec![value]).unwrap() else { + panic!("expected text"); + }; + assert!(out.contains("kept"), "present keys must survive: {out}"); + assert!( + !out.contains("dropped"), + "a nothing-valued key has no TOML spelling and must be omitted: {out}" + ); + } + + #[test] + fn nothing_inside_an_array_is_an_error_not_a_silent_drop() { + let list = Value::List(Rc::new(RefCell::new(vec![text("a"), Value::Nothing]))); + let mut map = HashMap::new(); + map.insert("items".to_string(), list); + let value = Value::Object(Rc::new(RefCell::new(map))); + + let err = native_stringify_toml(vec![value]) + .expect_err("dropping an array element would change its length"); + assert!(err.message.contains("nothing"), "got: {}", err.message); + } + + #[test] + fn a_top_level_list_is_not_a_toml_document() { + let list = Value::List(Rc::new(RefCell::new(vec![text("a")]))); + let err = native_stringify_toml(vec![list]).expect_err("TOML documents are tables"); + assert!(err.message.contains("table"), "got: {}", err.message); + } + + #[test] + fn round_trip_preserves_nested_structure() { + let src = "[server]\nhost = \"localhost\"\nports = [1, 2]\n"; + let parsed = native_parse_toml(vec![text(src)]).unwrap(); + let Value::Text(out) = native_stringify_toml_pretty(vec![parsed]).unwrap() else { + panic!("expected text"); + }; + let reparsed = native_parse_toml(vec![Value::Text(out)]).unwrap(); + + let Value::Object(obj) = reparsed else { + panic!("expected object"); + }; + let server = obj.borrow().get("server").cloned().expect("server table"); + let Value::Object(server) = server else { + panic!("expected nested table"); + }; + assert!(matches!(server.borrow().get("host"), Some(Value::Text(_)))); + } + + #[test] + fn non_finite_numbers_are_rejected() { + let mut map = HashMap::new(); + map.insert("x".to_string(), Value::Number(f64::INFINITY)); + let value = Value::Object(Rc::new(RefCell::new(map))); + assert!(native_stringify_toml(vec![value]).is_err()); + } +} From 4a084421a76be6ca6e565535da9284858f8ffc21 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:31:26 +0000 Subject: [PATCH 05/10] fix: atomic transaction blocks, and reject transaction SQL sent through execute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #664, which was a silent data-integrity failure rather than a missing feature. `open database` returns a five-connection pool and every query/execute takes whichever connection is free, so a hand-written `BEGIN` … `ROLLBACK` through `execute` ran each statement on a *different* connection: the rollback undid nothing, the writes survived, and nothing errored. In-memory SQLite is capped at one connection, so a program could pass its tests and lose data in production. Two halves, both needed. A real construct, which holds one connection for the whole block: in transaction on db: execute db with "INSERT ..." execute db with "UPDATE ..." end transaction It commits when the block finishes and rolls back if anything inside it fails. Reads inside the block run on the same connection, so they see the block's own uncommitted writes. `break`, `continue` and `return` are ordinary exits and commit — only an error rolls back. Nesting on one handle, and closing a database mid-transaction, are refused with an explanation rather than quietly doing something surprising. And a loud failure for the old workaround: `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` through query/execute now raise an error naming the block. Only the leading statement keyword is inspected, so a column named `begin_at` or a value containing the word "commit" still runs. This converts silent corruption into a message; no working program depended on the old behavior, because the old behavior did not work. `transaction` is deliberately not made a lexer keyword. It is recognized positionally, after a leading `in` and after `end`, so programs already using `transaction` as a variable name keep working. A statement beginning with `in` was previously always a parse error, so the syntax claims nothing that was valid before. Also registers the six new builtins from #665/#666/#667 in the two catalogs in builtins.rs and the static contracts in stdlib/typechecker.rs, which the runtime-inventory test holds to the runtime registrations. Closes #664 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- src/analyzer/mod.rs | 8 ++ src/analyzer/static_analyzer.rs | 6 + src/builtins.rs | 31 +++- src/interpreter/database.rs | 245 +++++++++++++++++++++++--------- src/interpreter/mod.rs | 166 ++++++++++++++++++++-- src/linter/mod.rs | 3 +- src/parser/ast.rs | 11 ++ src/parser/mod.rs | 11 ++ src/parser/stmt/database.rs | 79 ++++++++++ src/parser/stmt/mod.rs | 2 +- src/stdlib/typechecker.rs | 41 ++++++ src/typechecker/mod.rs | 22 +++ 12 files changed, 546 insertions(+), 79 deletions(-) diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 9b57dc22..b88e752c 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -1750,6 +1750,14 @@ impl Analyzer { Statement::CloseDatabaseStatement { db, .. } => { self.analyze_expression(db); } + Statement::TransactionStatement { db, body, .. } => { + self.analyze_expression(db); + // The block introduces no bindings of its own; statements inside + // it see and extend the enclosing scope, like `try:`. + for statement in body { + self.analyze_statement(statement); + } + } Statement::HttpGetStatement { variable_name, .. } => { let symbol = Symbol { name: variable_name.clone(), diff --git a/src/analyzer/static_analyzer.rs b/src/analyzer/static_analyzer.rs index c6d02deb..0fe1a99f 100644 --- a/src/analyzer/static_analyzer.rs +++ b/src/analyzer/static_analyzer.rs @@ -1130,6 +1130,12 @@ impl Analyzer { Statement::CloseDatabaseStatement { db, .. } => { self.mark_used_in_expression(db, usages); } + Statement::TransactionStatement { db, body, .. } => { + self.mark_used_in_expression(db, usages); + for statement in body { + self.mark_used_variables(statement, usages); + } + } Statement::ExecuteFileStatement { path, request, diff --git a/src/builtins.rs b/src/builtins.rs index 95deae62..49584496 100644 --- a/src/builtins.rs +++ b/src/builtins.rs @@ -53,10 +53,17 @@ const BUILTIN_FUNCTIONS: &[&str] = &[ "scrypt_verify", "pbkdf2_hash", "pbkdf2_verify", + // Authenticated encryption (implemented in stdlib/crypto.rs) + "seal", + "unseal", // JSON functions (implemented in stdlib/json.rs) "parse_json", "stringify_json", "stringify_json_pretty", + // TOML functions (implemented in stdlib/toml.rs) + "parse_toml", + "stringify_toml", + "stringify_toml_pretty", // Query and form parsing (implemented in stdlib/text.rs) "parse_query_string", "parse_cookies", @@ -223,6 +230,8 @@ const BUILTIN_FUNCTIONS: &[&str] = &[ "path_extension", "path_stem", "file_size", + "file_mode", + "set_file_mode", "copy_file", "move_file", "remove_file", @@ -273,6 +282,8 @@ const IMPLEMENTED_BUILTIN_FUNCTIONS: &[&str] = &[ "scrypt_verify", "pbkdf2_hash", "pbkdf2_verify", + "seal", + "unseal", // Filesystem "list_dir", "glob", @@ -289,6 +300,8 @@ const IMPLEMENTED_BUILTIN_FUNCTIONS: &[&str] = &[ "path_extension", "path_stem", "file_size", + "file_mode", + "set_file_mode", "copy_file", "move_file", "remove_file", @@ -298,6 +311,10 @@ const IMPLEMENTED_BUILTIN_FUNCTIONS: &[&str] = &[ "parse_json", "stringify_json", "stringify_json_pretty", + // TOML + "parse_toml", + "stringify_toml", + "stringify_toml_pretty", // Math "abs", "round", @@ -492,9 +509,15 @@ pub fn get_function_arity(name: &str) -> usize { "verify_password" | "argon2_verify" | "bcrypt_verify" | "scrypt_verify" | "pbkdf2_verify" => 2, + // === AUTHENTICATED ENCRYPTION === + // (plaintext, key), with an optional third `context` argument — see + // `get_function_arity_range`. + "seal" | "unseal" => 2, + // === JSON FUNCTIONS === // Single argument functions "parse_json" | "stringify_json" | "stringify_json_pretty" => 1, + "parse_toml" | "stringify_toml" | "stringify_toml_pretty" => 1, // === QUERY AND FORM PARSING === // Single argument functions @@ -578,9 +601,11 @@ pub fn get_function_arity(name: &str) -> usize { "list_dir" | "path_basename" | "path_dirname" | "makedirs" | "file_mtime" | "path_exists" | "is_file" | "is_dir" | "read_file" | "file_exists" | "delete_file" | "create_directory" | "list_directory" | "is_directory" | "count_lines" - | "path_extension" | "path_stem" | "file_size" | "remove_file" | "remove_dir" => 1, + | "path_extension" | "path_stem" | "file_size" | "remove_file" | "remove_dir" + | "file_mode" => 1, // Two argument functions - "glob" | "rglob" | "path_join" | "write_file" | "copy_file" | "move_file" => 2, + "glob" | "rglob" | "path_join" | "write_file" | "copy_file" | "move_file" + | "set_file_mode" => 2, // === SPECIAL TEST FUNCTIONS === "helper_function" | "nested_function" => 1, @@ -611,6 +636,8 @@ pub fn get_function_arity_range(name: &str) -> (usize, Option) { "create_datetime" => (3, Some(6)), "timestamp" => (0, Some(1)), "remove_dir" => (1, Some(2)), + // The optional third argument is the associated-data context. + "seal" | "unseal" => (2, Some(3)), _ => { let arity = get_function_arity(name); (arity, Some(arity)) diff --git a/src/interpreter/database.rs b/src/interpreter/database.rs index 482feed9..a34cead8 100644 --- a/src/interpreter/database.rs +++ b/src/interpreter/database.rs @@ -37,6 +37,27 @@ pub enum DbPool { Sqlite(sqlx::SqlitePool), } +/// An open transaction holding one connection out of a [`DbPool`] for its +/// whole lifetime (issue #664). +/// +/// `Pool::begin` hands back an owned `Transaction<'static, DB>`, so the +/// interpreter can park one of these in a map keyed by database handle and run +/// every statement inside `in transaction on db:` against it. Dropping it +/// without committing rolls back, which is what makes an abandoned transaction +/// safe rather than a leaked half-write. +pub enum DbTransaction { + Postgres(sqlx::Transaction<'static, sqlx::Postgres>), + MySql(sqlx::Transaction<'static, sqlx::MySql>), + Sqlite(sqlx::Transaction<'static, sqlx::Sqlite>), +} + +/// Where a statement runs: straight against the pool (any free connection), or +/// against the single connection an open transaction is holding. +pub enum DbTarget<'a> { + Pool(&'a DbPool), + Transaction(&'a mut DbTransaction), +} + /// An owned, Send-safe SQL bind parameter converted from a WFL `Value`. #[derive(Debug, Clone)] pub enum SqlParam { @@ -130,52 +151,97 @@ pub async fn connect(url: &str) -> Result { } } +/// Statement keywords that open, close, or checkpoint a transaction. +/// +/// See [`reject_transaction_control_sql`]. +const TRANSACTION_CONTROL_KEYWORDS: &[&str] = &[ + "begin", + "commit", + "rollback", + "start", + "savepoint", + "release", + "end", +]; + +/// Reject transaction-control SQL sent through `query`/`execute` (issue #664). +/// +/// Each `query`/`execute` takes whichever pooled connection happens to be free, +/// so a hand-written `BEGIN` … `ROLLBACK` sequence lands on *different* +/// connections and the rollback silently does nothing — the writes it was meant +/// to undo survive, with no error anywhere. That is a data-integrity failure +/// that looks exactly like success, and it is invisible under in-memory SQLite +/// (capped at one connection) where most tests run. +/// +/// Rather than keep failing silently, refuse the statement and name the +/// construct that actually works. Only the *leading* keyword is inspected, so a +/// column called `begin_at` or a string containing the word "commit" is +/// untouched. +pub fn reject_transaction_control_sql(sql: &str) -> Result<(), String> { + let first_word: String = sql + .trim_start() + .chars() + .take_while(|c| c.is_ascii_alphabetic()) + .collect(); + + if first_word.is_empty() { + return Ok(()); + } + + let lowered = first_word.to_ascii_lowercase(); + if TRANSACTION_CONTROL_KEYWORDS.contains(&lowered.as_str()) { + return Err(format!( + "'{first_word}' controls a transaction, and sending it through query/execute does \ + not work: each statement runs on its own pooled connection, so the transaction \ + would not cover the statements you meant it to. Use a transaction block instead:\n\ + \n in transaction on db:\n execute db with \"...\"\n end transaction\n\ + \nThe block holds one connection for its whole body, commits when it finishes, \ + and rolls back if anything inside it fails." + )); + } + + Ok(()) +} + /// Run a row-returning statement; rows become a list of objects keyed by /// column name. -pub async fn run_query(pool: &DbPool, sql: &str, params: &[SqlParam]) -> Result { - let rows: Vec = match pool { - DbPool::Sqlite(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). +pub async fn run_query( + target: DbTarget<'_>, + sql: &str, + params: &[SqlParam], +) -> Result { + reject_transaction_control_sql(sql)?; + + // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our runtime SQL + // text (see the module docs for the safety contract). + macro_rules! fetch { + ($executor:expr, $bind:ident, $to_value:ident) => {{ let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); for param in params { - query = bind_sqlite(query, param); + query = $bind(query, param); } let rows = query - .fetch_all(pool) + .fetch_all($executor) .await .map_err(|e| format!("Query failed: {e}"))?; rows.iter() - .map(sqlite_row_to_value) - .collect::>()? + .map($to_value) + .collect::, String>>()? + }}; + } + + let rows: Vec = match target { + DbTarget::Pool(DbPool::Sqlite(pool)) => fetch!(pool, bind_sqlite, sqlite_row_to_value), + DbTarget::Pool(DbPool::Postgres(pool)) => fetch!(pool, bind_postgres, pg_row_to_value), + DbTarget::Pool(DbPool::MySql(pool)) => fetch!(pool, bind_mysql, mysql_row_to_value), + DbTarget::Transaction(DbTransaction::Sqlite(tx)) => { + fetch!(&mut **tx, bind_sqlite, sqlite_row_to_value) } - DbPool::Postgres(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). - let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); - for param in params { - query = bind_postgres(query, param); - } - let rows = query - .fetch_all(pool) - .await - .map_err(|e| format!("Query failed: {e}"))?; - rows.iter().map(pg_row_to_value).collect::>()? + DbTarget::Transaction(DbTransaction::Postgres(tx)) => { + fetch!(&mut **tx, bind_postgres, pg_row_to_value) } - DbPool::MySql(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). - let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); - for param in params { - query = bind_mysql(query, param); - } - let rows = query - .fetch_all(pool) - .await - .map_err(|e| format!("Query failed: {e}"))?; - rows.iter() - .map(mysql_row_to_value) - .collect::>()? + DbTarget::Transaction(DbTransaction::MySql(tx)) => { + fetch!(&mut **tx, bind_mysql, mysql_row_to_value) } }; @@ -185,46 +251,46 @@ pub async fn run_query(pool: &DbPool, sql: &str, params: &[SqlParam]) -> Result< /// Run a non-returning statement; the result is an object with /// `affected_rows` and `last_insert_id` (nothing on PostgreSQL — use /// `RETURNING` there instead). -pub async fn run_execute(pool: &DbPool, sql: &str, params: &[SqlParam]) -> Result { - let (affected_rows, last_insert_id): (u64, Option) = match pool { - DbPool::Sqlite(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). +pub async fn run_execute( + target: DbTarget<'_>, + sql: &str, + params: &[SqlParam], +) -> Result { + reject_transaction_control_sql(sql)?; + + // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our runtime SQL + // text (see the module docs for the safety contract). + macro_rules! run { + ($executor:expr, $bind:ident, $last_id:expr) => {{ let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); for param in params { - query = bind_sqlite(query, param); + query = $bind(query, param); } let result = query - .execute(pool) + .execute($executor) .await .map_err(|e| format!("Execute failed: {e}"))?; - (result.rows_affected(), Some(result.last_insert_rowid())) + #[allow(clippy::redundant_closure_call)] + (result.rows_affected(), $last_id(&result)) + }}; + } + + let sqlite_id = |r: &sqlx::sqlite::SqliteQueryResult| Some(r.last_insert_rowid()); + let mysql_id = |r: &sqlx::mysql::MySqlQueryResult| Some(r.last_insert_id() as i64); + let pg_id = |_: &sqlx::postgres::PgQueryResult| None; + + let (affected_rows, last_insert_id): (u64, Option) = match target { + DbTarget::Pool(DbPool::Sqlite(pool)) => run!(pool, bind_sqlite, sqlite_id), + DbTarget::Pool(DbPool::Postgres(pool)) => run!(pool, bind_postgres, pg_id), + DbTarget::Pool(DbPool::MySql(pool)) => run!(pool, bind_mysql, mysql_id), + DbTarget::Transaction(DbTransaction::Sqlite(tx)) => { + run!(&mut **tx, bind_sqlite, sqlite_id) } - DbPool::Postgres(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). - let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); - for param in params { - query = bind_postgres(query, param); - } - let result = query - .execute(pool) - .await - .map_err(|e| format!("Execute failed: {e}"))?; - (result.rows_affected(), None) + DbTarget::Transaction(DbTransaction::Postgres(tx)) => { + run!(&mut **tx, bind_postgres, pg_id) } - DbPool::MySql(pool) => { - // `AssertSqlSafe`: opt out of sqlx's `SqlSafeStr` gate for our - // runtime SQL text (see the module docs for the safety contract). - let mut query = sqlx::query(sqlx::AssertSqlSafe(sql)); - for param in params { - query = bind_mysql(query, param); - } - let result = query - .execute(pool) - .await - .map_err(|e| format!("Execute failed: {e}"))?; - (result.rows_affected(), Some(result.last_insert_id() as i64)) + DbTarget::Transaction(DbTransaction::MySql(tx)) => { + run!(&mut **tx, bind_mysql, mysql_id) } }; @@ -245,6 +311,51 @@ pub async fn run_execute(pool: &DbPool, sql: &str, params: &[SqlParam]) -> Resul Ok(Value::Object(Rc::new(RefCell::new(object)))) } +/// Take one connection out of the pool and open a transaction on it. +pub async fn begin(pool: &DbPool) -> Result { + match pool { + DbPool::Sqlite(pool) => pool + .begin() + .await + .map(DbTransaction::Sqlite) + .map_err(|e| format!("Failed to start transaction: {e}")), + DbPool::Postgres(pool) => pool + .begin() + .await + .map(DbTransaction::Postgres) + .map_err(|e| format!("Failed to start transaction: {e}")), + DbPool::MySql(pool) => pool + .begin() + .await + .map(DbTransaction::MySql) + .map_err(|e| format!("Failed to start transaction: {e}")), + } +} + +/// Commit a transaction, returning its connection to the pool. +pub async fn commit(tx: DbTransaction) -> Result<(), String> { + match tx { + DbTransaction::Sqlite(tx) => tx.commit().await, + DbTransaction::Postgres(tx) => tx.commit().await, + DbTransaction::MySql(tx) => tx.commit().await, + } + .map_err(|e| format!("Failed to commit transaction: {e}")) +} + +/// Roll a transaction back, returning its connection to the pool. +/// +/// Dropping a `DbTransaction` without calling this also rolls back; this exists +/// so the interpreter can report a rollback that itself fails, rather than +/// discarding it silently. +pub async fn rollback(tx: DbTransaction) -> Result<(), String> { + match tx { + DbTransaction::Sqlite(tx) => tx.rollback().await, + DbTransaction::Postgres(tx) => tx.rollback().await, + DbTransaction::MySql(tx) => tx.rollback().await, + } + .map_err(|e| format!("Failed to roll back transaction: {e}")) +} + /// Close the pool, ending all connections. pub async fn close(pool: DbPool) { match pool { diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 0113486b..11e2ac77 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1190,6 +1190,7 @@ fn stmt_type(stmt: &Statement) -> String { format!("DatabaseQueryStatement '{variable_name}'") } Statement::CloseDatabaseStatement { .. } => "CloseDatabaseStatement".to_string(), + Statement::TransactionStatement { .. } => "TransactionStatement".to_string(), Statement::CreateDirectoryStatement { .. } => "CreateDirectoryStatement".to_string(), Statement::CreateFileStatement { .. } => "CreateFileStatement".to_string(), Statement::DeleteFileStatement { .. } => "DeleteFileStatement".to_string(), @@ -1867,6 +1868,11 @@ pub struct IoClient { next_process_id: Mutex, db_handles: Mutex>, next_db_id: Mutex, + /// Transactions currently open, keyed by the same handle id as + /// `db_handles`. While an entry is present, every `query`/`execute` on that + /// handle runs on the transaction's pinned connection instead of taking a + /// fresh one from the pool — which is the whole point of issue #664. + db_transactions: Mutex>, /// Live outbound streaming response bodies, keyed by handle id /// ("httpstream1", ...). See [`StreamSlot`] / [`HttpStreamHandle`]. /// @@ -2429,6 +2435,7 @@ impl IoClient { next_process_id: Mutex::new(1), db_handles: Mutex::new(HashMap::new()), next_db_id: Mutex::new(1), + db_transactions: Mutex::new(HashMap::new()), stream_handles: Arc::new(std::sync::Mutex::new(StreamRegistry::default())), next_stream_id: Mutex::new(1), #[cfg(test)] @@ -2463,7 +2470,19 @@ impl IoClient { } /// Close a database pool and drop its handle. + /// + /// Refuses while a transaction is open on the handle: closing the pool + /// under an in-flight transaction would discard the work with no report of + /// what happened to it. async fn close_database(&self, handle_id: &str) -> Result<(), String> { + if self.db_transactions.lock().await.contains_key(handle_id) { + return Err(format!( + "Cannot close database '{handle_id}' while a transaction is open on it. \ + Let the `in transaction on ...` block finish first — it commits at \ + `end transaction`, or rolls back if something inside it fails." + )); + } + let pool = self .db_handles .lock() @@ -2474,6 +2493,87 @@ impl IoClient { Ok(()) } + /// Open a transaction on `handle_id`, pinning one pooled connection to it. + async fn begin_transaction(&self, handle_id: &str) -> Result<(), String> { + // Nesting would need savepoints, which are not implemented; say so + // rather than quietly flattening the inner block into the outer one. + if self.db_transactions.lock().await.contains_key(handle_id) { + return Err(format!( + "A transaction is already open on database '{handle_id}'. Transaction \ + blocks cannot be nested on the same database — finish the current one \ + before starting another." + )); + } + + let pool = self.get_database(handle_id).await?; + let tx = database::begin(&pool).await?; + self.db_transactions + .lock() + .await + .insert(handle_id.to_string(), tx); + Ok(()) + } + + /// Commit the open transaction on `handle_id`. + async fn commit_transaction(&self, handle_id: &str) -> Result<(), String> { + let tx = self + .db_transactions + .lock() + .await + .remove(handle_id) + .ok_or_else(|| format!("No transaction is open on database '{handle_id}'"))?; + database::commit(tx).await + } + + /// Roll back the open transaction on `handle_id`. + /// + /// A missing transaction is not an error here: this runs on the failure + /// path, where the transaction may already have been resolved. + async fn rollback_transaction(&self, handle_id: &str) -> Result<(), String> { + let tx = self.db_transactions.lock().await.remove(handle_id); + match tx { + Some(tx) => database::rollback(tx).await, + None => Ok(()), + } + } + + /// Run a row-returning statement, routed to the handle's open transaction + /// when it has one. + async fn db_query( + &self, + handle_id: &str, + sql: &str, + params: &[database::SqlParam], + ) -> Result { + { + let mut transactions = self.db_transactions.lock().await; + if let Some(tx) = transactions.get_mut(handle_id) { + return database::run_query(database::DbTarget::Transaction(tx), sql, params).await; + } + } + let pool = self.get_database(handle_id).await?; + database::run_query(database::DbTarget::Pool(&pool), sql, params).await + } + + /// Run a non-returning statement, routed to the handle's open transaction + /// when it has one. + async fn db_execute( + &self, + handle_id: &str, + sql: &str, + params: &[database::SqlParam], + ) -> Result { + { + let mut transactions = self.db_transactions.lock().await; + if let Some(tx) = transactions.get_mut(handle_id) { + return database::run_execute(database::DbTarget::Transaction(tx), sql, params) + .await; + } + } + let pool = self.get_database(handle_id).await?; + database::run_execute(database::DbTarget::Pool(&pool), sql, params).await + } + #[allow(dead_code)] async fn http_get( &self, @@ -6286,6 +6386,7 @@ impl Interpreter { Statement::OpenDatabaseStatement { line, column, .. } => (*line, *column), Statement::DatabaseQueryStatement { line, column, .. } => (*line, *column), Statement::CloseDatabaseStatement { line, column, .. } => (*line, *column), + Statement::TransactionStatement { line, column, .. } => (*line, *column), Statement::CreateDirectoryStatement { line, column, .. } => (*line, *column), Statement::CreateFileStatement { line, column, .. } => (*line, *column), Statement::DeleteFileStatement { line, column, .. } => (*line, *column), @@ -7190,6 +7291,59 @@ impl Interpreter { Ok((Value::Null, ControlFlow::None)) } + Statement::TransactionStatement { + db, + body, + line, + column, + } => { + let db_value = self.evaluate_expression(db, Rc::clone(&env)).await?; + let handle = match &db_value { + Value::Text(s) => s.clone(), + _ => { + return Err(RuntimeError::new( + format!("Expected a database handle, got {db_value:?}"), + *line, + *column, + )); + } + }; + + self.io_client + .begin_transaction(&handle) + .await + .map_err(|e| RuntimeError::new(e, *line, *column))?; + + // Statements inside share the enclosing scope, like `try:`, so a + // variable set inside the block is still readable after it. + let outcome = self.execute_block(body, Rc::clone(&env)).await; + + match outcome { + Ok((value, control_flow)) => { + // The block finished without an error. `break`, `continue` + // and `return` are ordinary exits, not failures, so they + // commit too — the work inside completed. + self.io_client + .commit_transaction(&handle) + .await + .map_err(|e| RuntimeError::new(e, *line, *column))?; + Ok((value, control_flow)) + } + Err(err) => { + // Roll back and report the original failure. A rollback + // that itself fails is appended rather than replacing the + // cause, which is what the user actually needs to see. + match self.io_client.rollback_transaction(&handle).await { + Ok(()) => Err(err), + Err(rollback_error) => Err(RuntimeError::new( + format!("{} (rollback also failed: {rollback_error})", err.message), + err.line, + err.column, + )), + } + } + } + } Statement::ReadFileStatement { path, variable_name, @@ -13194,18 +13348,14 @@ impl Interpreter { None => Vec::new(), }; - let pool = self - .io_client - .get_database(&handle) - .await - .map_err(|e| RuntimeError::new(e, line, column))?; - + // Routed through the IoClient so an open transaction on this handle + // gets its own pinned connection rather than an arbitrary pooled one. match kind { crate::parser::ast::DatabaseQueryKind::Query => { - database::run_query(&pool, &sql_str, ¶ms).await + self.io_client.db_query(&handle, &sql_str, ¶ms).await } crate::parser::ast::DatabaseQueryKind::Execute => { - database::run_execute(&pool, &sql_str, ¶ms).await + self.io_client.db_execute(&handle, &sql_str, ¶ms).await } } .map_err(|e| RuntimeError::new(e, line, column)) diff --git a/src/linter/mod.rs b/src/linter/mod.rs index 952cf7d8..4ef8c01e 100644 --- a/src/linter/mod.rs +++ b/src/linter/mod.rs @@ -491,7 +491,8 @@ fn check_nesting_depth( } Statement::WhileLoop { body, .. } | Statement::ForEachLoop { body, .. } - | Statement::CountLoop { body, .. } => { + | Statement::CountLoop { body, .. } + | Statement::TransactionStatement { body, .. } => { for stmt in body { check_nesting_depth(stmt, current_depth + 1, max_depth, diagnostics, file_id); } diff --git a/src/parser/ast.rs b/src/parser/ast.rs index ec7786fb..bff30e90 100644 --- a/src/parser/ast.rs +++ b/src/parser/ast.rs @@ -284,6 +284,17 @@ pub enum Statement { line: usize, column: usize, }, + /// `in transaction on : ... end transaction` + /// + /// Runs `body` against a single connection held for the whole block, so the + /// statements inside it are atomic. Commits when the block finishes, rolls + /// back if anything inside it fails (issue #664). + TransactionStatement { + db: Expression, + body: Vec, + line: usize, + column: usize, + }, CreateDirectoryStatement { path: Expression, line: usize, diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 62a2d0fb..fb35f63d 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -514,6 +514,17 @@ impl<'a> StmtParser<'a> for Parser<'a> { Token::KeywordRemove => self.parse_remove_from_list_statement(), Token::KeywordClear => self.parse_clear_list_statement(), Token::KeywordTry => self.parse_try_statement(), + // `in transaction on :` is the only statement that starts + // with `in`; anything else beginning with `in` was already a + // parse error, so this claims no previously valid syntax. + Token::KeywordIn + if self + .cursor + .peek_next() + .is_some_and(|t| crate::parser::stmt::is_transaction_word(&t.token)) => + { + self.parse_transaction_statement() + } Token::KeywordRepeat => self.parse_repeat_statement(), Token::KeywordExit => self.parse_exit_statement(), Token::KeywordPush => self.parse_push_statement(), diff --git a/src/parser/stmt/database.rs b/src/parser/stmt/database.rs index 9a309297..6a0429e3 100644 --- a/src/parser/stmt/database.rs +++ b/src/parser/stmt/database.rs @@ -5,12 +5,24 @@ //! - `store as query with [and parameters ]` //! - `store as execute with [and parameters ]` //! - `close database ` +//! - `in transaction on : ... end transaction` +//! +//! `transaction` is deliberately *not* a lexer keyword. It is recognized +//! positionally — an identifier directly after a leading `in`, and after `end` +//! closing the block — so existing programs that use `transaction` as an +//! ordinary variable name keep working. use super::super::{ParseError, Parser, Statement}; +use super::StmtParser; use crate::lexer::token::Token; use crate::parser::ast::{DatabaseQueryKind, Expression}; use crate::parser::expr::{ExprParser, PrimaryExprParser}; +/// True when `token` is the contextual word `transaction`. +pub(crate) fn is_transaction_word(token: &Token) -> bool { + matches!(token, Token::Identifier(id) if id.eq_ignore_ascii_case("transaction")) +} + pub(crate) trait DatabaseParser<'a>: ExprParser<'a> { /// Parse `open database at as ` with the `open` token already consumed /// and the cursor positioned on `database`. Also used by `connect to database`, @@ -27,6 +39,11 @@ pub(crate) trait DatabaseParser<'a>: ExprParser<'a> { /// Parse `close database ` from the `close` keyword. fn parse_close_database_statement(&mut self) -> Result; + /// Parse `in transaction on : ... end transaction` from the leading `in`. + fn parse_transaction_statement(&mut self) -> Result + where + Self: StmtParser<'a>; + /// Detect whether the cursor is positioned on the value side of a database /// query/execute form: `query|execute with ...`. The three-token /// lookahead keeps ordinary variables named `query` parsing as expressions. @@ -161,6 +178,68 @@ impl<'a> DatabaseParser<'a> for Parser<'a> { }) } + fn parse_transaction_statement(&mut self) -> Result + where + Self: StmtParser<'a>, + { + let in_token = self.bump_sync().unwrap(); // Consume "in" + self.bump_sync(); // Consume "transaction" + + self.expect_token( + Token::KeywordOn, + "Expected 'on' after 'in transaction' — write `in transaction on :`", + )?; + + // A primary expression, not a full one: the general expression parser + // runs on past the `:` that closes the header. + let db = self.parse_primary_expression()?; + + self.expect_token( + Token::Colon, + "Expected ':' after the database in `in transaction on :`", + )?; + self.skip_eol(); + + let mut body = Vec::new(); + while let Some(token) = self.cursor.peek() { + if matches!(token.token, Token::KeywordEnd) { + break; + } + if matches!(&token.token, Token::Eol) { + self.bump_sync(); + continue; + } + body.push(self.parse_statement()?); + } + + self.expect_token( + Token::KeywordEnd, + "Expected 'end transaction' to close the transaction block", + )?; + + let closes_block = self + .cursor + .peek() + .is_some_and(|t| is_transaction_word(&t.token)); + if closes_block { + self.bump_sync(); + } else { + let message = + "Expected 'transaction' after 'end' to close the transaction block".to_string(); + return match self.cursor.peek() { + Some(token) => Err(ParseError::from_token(message, token)), + None => Err(self.cursor.error(message)), + }; + } + + Ok(Statement::TransactionStatement { + db, + body, + line: in_token.line, + column: in_token.column, + }) + } + fn parse_database_query_value( &mut self, name: String, diff --git a/src/parser/stmt/mod.rs b/src/parser/stmt/mod.rs index ec390a91..493fce70 100644 --- a/src/parser/stmt/mod.rs +++ b/src/parser/stmt/mod.rs @@ -24,7 +24,7 @@ pub(crate) use actions::ActionParser; pub(crate) use collections::CollectionParser; pub(crate) use containers::ContainerParser; pub(crate) use control_flow::ControlFlowParser; -pub(crate) use database::DatabaseParser; +pub(crate) use database::{DatabaseParser, is_transaction_word}; pub(crate) use errors::ErrorHandlingParser; pub(crate) use io::IoParser; pub(crate) use module::ModuleParser; diff --git a/src/stdlib/typechecker.rs b/src/stdlib/typechecker.rs index 8241bc9a..e8562dad 100644 --- a/src/stdlib/typechecker.rs +++ b/src/stdlib/typechecker.rs @@ -16,6 +16,7 @@ pub fn register_stdlib_types(analyzer: &mut Analyzer) { register_list(analyzer); register_pattern(analyzer); register_json(analyzer); + register_toml(analyzer); register_web(analyzer); register_crypto(analyzer); register_filesystem(analyzer); @@ -345,6 +346,29 @@ fn register_json(analyzer: &mut Analyzer) { } } +fn register_toml(analyzer: &mut Analyzer) { + register(analyzer, &["parse_toml"], vec![Type::Text], Type::Any); + + // Mirrors the JSON value set. TOML has no null, but `nothing` is accepted + // here because a table simply omits those keys (see stdlib::toml). + let toml_values = [ + Type::Nothing, + Type::Boolean, + Type::Number, + Type::Text, + list(Type::Any), + map(Type::Text, Type::Any), + ]; + for value_type in toml_values { + register( + analyzer, + &["stringify_toml", "stringify_toml_pretty"], + vec![value_type], + Type::Text, + ); + } +} + fn register_web(analyzer: &mut Analyzer) { register( analyzer, @@ -401,6 +425,16 @@ fn register_crypto(analyzer: &mut Analyzer) { vec![Type::Number], Type::Text, ); + // `seal of plaintext and key`, optionally `and context`. + register_same_result_overloads( + analyzer, + &["seal", "unseal"], + [ + vec![Type::Text, Type::Text], + vec![Type::Text, Type::Text, Type::Text], + ], + Type::Text, + ); register( analyzer, &[ @@ -474,6 +508,13 @@ fn register_filesystem(analyzer: &mut Analyzer) { vec![Type::Text, Type::Text], Type::Nothing, ); + register(analyzer, &["file_mode"], vec![Type::Text], Type::Text); + register( + analyzer, + &["set_file_mode"], + vec![Type::Text, Type::Text], + Type::Text, + ); register_same_result_overloads( analyzer, &["remove_dir"], diff --git a/src/typechecker/mod.rs b/src/typechecker/mod.rs index 6d6ec880..5a8bcdba 100644 --- a/src/typechecker/mod.rs +++ b/src/typechecker/mod.rs @@ -5850,6 +5850,28 @@ impl TypeChecker { ); } } + Statement::TransactionStatement { + db, + body, + line: _line, + column: _column, + } => { + let db_type = self.infer_expression_type(db); + if db_type != Type::Custom("Database".to_string()) + && !self.is_gradual_type(&db_type) + { + self.type_error( + "Expected a Database connection".to_string(), + Some(Type::Custom("Database".to_string())), + Some(db_type), + *_line, + *_column, + ); + } + // Like `try:`, the block shares the enclosing scope and + // introduces no bindings of its own. + self.check_statement_block(body); + } Statement::CreateDirectoryStatement { path, line: _line, From affd7eb82e335f593129e4418e4393c2289c9614 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:44:43 +0000 Subject: [PATCH 06/10] docs: transactions, seal/unseal, file modes and TOML Ships the documentation and end-to-end programs for #664, #665, #666 and #667 alongside the implementations. Docs: - Docs/04-advanced-features/databases.md gains a Transactions section covering the block, what it guarantees, the nesting and close-during restrictions, and why BEGIN/COMMIT through execute is now refused. - Docs/05-standard-library/crypto-module.md documents seal and unseal, leading with the distinction the issue turned on: hashing verifies a value you are handed, and cannot stand in for a credential you must send upstream later. - Docs/05-standard-library/filesystem-module.md documents file_mode and set_file_mode, including the refuse-to-start check that motivated them and the Windows behavior stated outright rather than implied. - Docs/05-standard-library/toml-module.md is new; the index and overview gain it and the module counts move from 11 to 12. - Both keyword references list `transaction` as a positional marker word that is never reserved, so the keyword total correctly stays at 181. Every example in these pages was run against the release binary rather than written from memory. Three claims did not survive that and were corrected: the databases examples had used a bare `execute db with ...` statement, which the parser reads as subprocess execution, so they now use the `store as execute ...` form the language actually has; and the TOML page had shown output in the wrong key order. Writing TOML sorts keys alphabetically and drops comments, which is deterministic but not faithful to a hand-written file, so the page now says so. TestPrograms: - database_transaction_test.wfl, crypto_seal_test.wfl, file_mode_test.wfl and toml_test.wfl, run by the gated integration runner (it detects `describe` and adds --test, so a failed assertion exits nonzero). - The transaction program uses a file-backed SQLite database on purpose; in-memory SQLite is capped at one connection and cannot observe #664 at all. - The file-mode program runs on Windows too, where set_file_mode refuses by design, so it probes for support once and asserts whichever contract applies. Adds a dev diary entry and CHANGELOG entries for all four issues. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- CHANGELOG.md | 37 +++ Docs/04-advanced-features/databases.md | 79 +++++++ Docs/05-standard-library/crypto-module.md | 85 +++++++ Docs/05-standard-library/filesystem-module.md | 87 +++++++ Docs/05-standard-library/index.md | 5 +- Docs/05-standard-library/overview.md | 16 +- Docs/05-standard-library/toml-module.md | 162 +++++++++++++ Docs/reference/keyword-reference.md | 4 +- Docs/reference/reserved-keywords.md | 7 + ...07-31-transactions-aead-file-modes-toml.md | 222 ++++++++++++++++++ TestPrograms/crypto_seal_test.wfl | 111 +++++++++ TestPrograms/database_transaction_test.wfl | 147 ++++++++++++ TestPrograms/file_mode_test.wfl | 111 +++++++++ TestPrograms/toml_test.wfl | 139 +++++++++++ 14 files changed, 1204 insertions(+), 8 deletions(-) create mode 100644 Docs/05-standard-library/toml-module.md create mode 100644 History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md create mode 100644 TestPrograms/crypto_seal_test.wfl create mode 100644 TestPrograms/database_transaction_test.wfl create mode 100644 TestPrograms/file_mode_test.wfl create mode 100644 TestPrograms/toml_test.wfl diff --git a/CHANGELOG.md b/CHANGELOG.md index 343fc179..f671ff3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,43 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] ### Added +- **Database transactions** (#664): `in transaction on db: ... end transaction` + runs a group of statements on a single pooled connection, committing when the + block finishes and rolling back if anything inside it fails. Transaction blocks + cannot be nested on one database, and a database cannot be closed inside its + own transaction; both are reported rather than silently tolerated. + `transaction` is a positional marker word, not a reserved keyword, so programs + already using it as a variable name are unaffected. +- **Authenticated encryption** (#665): `seal of and ` and + `unseal of and `, backed by XChaCha20-Poly1305. Keys are the + 64-hex-character values `secure_random_bytes of 32` already produced; nonces + are generated internally per call and never exposed. An optional third + argument supplies associated data that binds a ciphertext to its context. + `unseal` fails closed and reports every failure identically. +- **File permissions** (#666): `file_mode of ` returns a file's mode as + four octal digits, and `set_file_mode of and "0600"` sets it, so a + program can both restrict a file holding a secret and verify that it is + restricted. Unix implements real POSIX semantics; on Windows, reading returns + a documented approximation and setting raises an explicit unsupported error + rather than silently doing nothing. +- **TOML support** (#667): `parse_toml`, `stringify_toml` and + `stringify_toml_pretty`, mirroring the existing JSON functions. A TOML + document must be a table, and `nothing`-valued keys are omitted when writing + (TOML has no null); `nothing` inside a list is an error rather than a silent + drop. + +### Fixed +- **Transaction control SQL sent through `query`/`execute` is now rejected** + (#664). `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and + `RELEASE` previously ran on arbitrary pooled connections, so a hand-written + `BEGIN`/`ROLLBACK` sequence silently failed to cover the statements between + them: the rollback undid nothing, the writes survived, and no error was + raised. In-memory SQLite hid this entirely because it is capped at one + connection. These statements now raise an error naming the transaction block. + Only the leading statement keyword is inspected, so ordinary SQL that merely + contains those words (a `begin_at` column, a `'rollback plan'` value) still + runs. + - **Binding Repository Hygiene and Layout Policy** (`REPOSITORY_HYGIENE.md`, governance §3.8) with a machine-readable profile (`.repo-hygiene.toml`) and a dependency-free checker (`scripts/check_repo_hygiene.py`) enforced as a diff --git a/Docs/04-advanced-features/databases.md b/Docs/04-advanced-features/databases.md index 18a42510..c92df40c 100644 --- a/Docs/04-advanced-features/databases.md +++ b/Docs/04-advanced-features/databases.md @@ -100,6 +100,85 @@ store row as rows[0] store new_id as row["id"] ``` +## Transactions + +Some changes only make sense together. Moving money between two accounts is two +`UPDATE`s, and stopping halfway is worse than never starting. A transaction +block makes a group of statements all-or-nothing: + +```wfl +in transaction on db: + store debited as execute db with "UPDATE accounts SET balance = balance - 100 WHERE id = 1" + store credited as execute db with "UPDATE accounts SET balance = balance + 100 WHERE id = 2" +end transaction +``` + +As everywhere else, `execute` is written as `store as execute ...` — the +result object is always bound, even when you do not need it. + +If both statements succeed, the block commits and the changes become permanent +when it reaches `end transaction`. If anything inside fails, everything the +block did is undone — including the statements that had already succeeded — and +the error is reported as usual, so you can catch it: + +```wfl +try: + in transaction on db: + store claimed as execute db with "INSERT INTO jobs (slug, status) VALUES (?, 'running')" and parameters [slug] + store counted as execute db with "UPDATE counters SET running = running + 1" + end transaction + display "Job claimed." +when error: + display "Could not claim the job; nothing was changed." +end try +``` + +### What the block guarantees + +- **One connection for the whole block.** `open database` maintains a pool of + connections, and outside a transaction each statement takes whichever one is + free. Inside the block, every statement runs on the same connection — that is + what makes the group atomic. +- **Reads see the block's own writes.** A `query` inside the block sees rows the + block has inserted but not yet committed. Other connections do not see them + until the block commits. +- **Only errors roll back.** `break`, `continue` and `return` are ordinary ways + to leave a block, so they commit — the work inside finished. A rollback + happens when a statement fails. + +### Restrictions + +**Transaction blocks cannot be nested on the same database.** Starting a second +block on a database that already has one open is an error rather than a silent +flattening of one into the other. Nested transactions require savepoints, which +WFL does not currently expose. + +**A database cannot be closed inside its own transaction.** `close database` +during an open block is an error; let the block finish first. + +### Do not send BEGIN or COMMIT through `execute` + +Writing transaction control as SQL does not work, and WFL now says so: + +```wfl +store t as execute db with "BEGIN" // Error, with a pointer to the block syntax +``` + +The reason is the connection pool. `BEGIN`, the statements after it, and +`COMMIT` would each take a different pooled connection, so the transaction would +not cover the statements it appeared to wrap — a `ROLLBACK` would quietly undo +nothing while the writes it was meant to discard survived. `BEGIN`, `COMMIT`, +`ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` are therefore +rejected with an error naming the block syntax above. + +Only the leading keyword of a statement is checked, so ordinary SQL that merely +contains those words — a column named `begin_at`, a value of `'rollback plan'` — +runs normally. + +> `transaction` is not a reserved word. It is recognized only in +> `in transaction on ...` and `end transaction`, so existing programs that use +> `transaction` as a variable name keep working. + ## Returning Results from Actions `query` and `execute` — with or without `and parameters [...]` — can be diff --git a/Docs/05-standard-library/crypto-module.md b/Docs/05-standard-library/crypto-module.md index 2cf30f92..613b260f 100644 --- a/Docs/05-standard-library/crypto-module.md +++ b/Docs/05-standard-library/crypto-module.md @@ -543,6 +543,91 @@ display "New session id: " with session_id - Password salts (pair with [`pbkdf2_hmac_sha256`](#pbkdf2_hmac_sha256)) - Session identifiers and cookies - CSRF tokens, password-reset tokens, API keys +- Sealing keys (pair with [`seal`](#seal) — use exactly `secure_random_bytes of 32`) + +--- + +### seal + +**Purpose:** Encrypt text so it can be stored at rest and read back later. Use this for secrets your program must recover — an API token it will send upstream, a saved provider key, session material. + +> **Hashing is not a substitute here.** `hash_password` and `sha256` are one-way by design: they let you *verify* a value you are given, never recover it. A stored API key has to be sent to the service later, so it must be recoverable. That is what `seal` is for, and it is the one case where a hash cannot stand in. + +**Signature:** +```wfl +seal of and <key> +seal of <plaintext> and <key> and <context> +``` + +**Parameters:** +- `plaintext` (Text): The secret to protect +- `key` (Text): A 64-character hex key — exactly what `secure_random_bytes of 32` returns +- `context` (Text, optional): Associated data that binds this ciphertext to where it lives + +**Returns:** Text — a sealed value beginning with `wflseal1:` + +**Example:** +```wfl +store project_key as secure_random_bytes of 32 +store sealed as seal of "sk-live-provider-token" and project_key +display sealed // wflseal1:9f2c... (varies every time) + +store token as unseal of sealed and project_key +display token // sk-live-provider-token +``` + +Sealing the same text twice never produces the same result — each call generates a fresh nonce internally. That is deliberate: reusing a nonce is the one mistake that breaks this kind of encryption outright, so WFL owns nonce handling rather than asking you to manage it. + +**Binding a secret to its context.** The optional third argument is authenticated but not encrypted. It ties the ciphertext to a place, so a value lifted out of one record cannot be replayed into another: + +```wfl +store sealed as seal of api_token and project_key and "project:acme/api_key" +store token as unseal of sealed and project_key and "project:acme/api_key" // works +store stolen as unseal of sealed and project_key and "project:evil/api_key" // error +``` + +**Use Cases:** +- API tokens and provider keys written to a config file +- Any credential that has to be read back, not merely checked + +--- + +### unseal + +**Purpose:** Recover the text a `seal` call protected. + +**Signature:** +```wfl +unseal of <sealed> and <key> +unseal of <sealed> and <key> and <context> +``` + +**Parameters:** +- `sealed` (Text): A value produced by `seal` +- `key` (Text): The same key used to seal it +- `context` (Text, optional): The same context used to seal it, if one was used + +**Returns:** Text — the original plaintext + +**Raises:** An error if anything is wrong — the wrong key, a modified or truncated sealed value, or a context that does not match. All of these report the same message on purpose: telling an attacker *which* part failed would help them work toward a valid one. + +**Example:** +```wfl +store config_key as secure_random_bytes of 32 +store sealed as seal of "database password" and config_key + +// Anything tampered with fails rather than returning garbage. +store recovered as "" +try: + change recovered to unseal of sealed and config_key +when error: + display "This value could not be opened — refusing to continue" +end try +``` + +**Keep the key somewhere the sealed value is not.** Sealing a secret and storing the key beside it in the same file protects nothing. The key belongs in the environment, a key manager, or a file the program reads separately — ideally one restricted with [`set_file_mode`](filesystem-module.md#set_file_mode). + +**Algorithm:** XChaCha20-Poly1305, from the reviewed RustCrypto implementation. Its 192-bit nonce is wide enough that a randomly generated nonce per message is safe without any counter, which is why WFL can keep nonces out of the API entirely. The sealed format is versioned (`wflseal1:`) so the algorithm can change in future without ambiguity about what an old stored value contains. --- diff --git a/Docs/05-standard-library/filesystem-module.md b/Docs/05-standard-library/filesystem-module.md index dcc31de1..a9dd4005 100644 --- a/Docs/05-standard-library/filesystem-module.md +++ b/Docs/05-standard-library/filesystem-module.md @@ -442,6 +442,93 @@ display "File size: " with round of kb with " KB" --- +### file_mode + +**Purpose:** Read a file's permission mode. Use this to check that a file holding a secret is actually restricted — the check that lets a program refuse to start when its config is readable by everyone. + +**Signature:** +```wfl +file_mode of <path> +``` + +**Parameters:** +- `path` (Text): File path + +**Returns:** Text — four octal digits, such as `"0600"` or `"0644"` + +**Raises:** An error if the file does not exist. + +**Example:** +```wfl +store mode as file_mode of "settings.toml" +display "Config permissions: " with mode + +check if mode is not "0600": + display "Refusing to start: settings.toml is readable by other users." + display "Run: set_file_mode of \"settings.toml\" and \"0600\"" +otherwise: + display "Config is owner-only. Continuing." +end check +``` + +**On Windows** the returned value is an approximation. Windows uses ACLs rather than POSIX modes, and the only thing visible at this level is the read-only attribute, so `file_mode` returns `"0444"` for a read-only file and `"0666"` otherwise. Treat it as a rough indicator there, not as a security check. + +**Use Cases:** +- Refusing to start when a credentials file is group- or world-readable +- Auditing the files a program has written +- Confirming that a `set_file_mode` call did what you expected + +--- + +### set_file_mode + +**Purpose:** Restrict (or open up) a file's permissions. Use this immediately after writing a file that contains a secret. + +**Signature:** +```wfl +set_file_mode of <path> and <mode> +``` + +**Parameters:** +- `path` (Text): File path +- `mode` (Text): Three or four octal digits — `"0600"` and `"600"` are both accepted + +**Returns:** Text — the mode that was applied, as four octal digits + +**Raises:** An error if the file does not exist, if the mode is not valid octal, or if the platform does not support file modes. + +**Example:** +```wfl +// Write a config holding an API key, then lock it down before anything else runs +open file at "settings.toml" for writing as settings_file +wait for write content "api_key = \"sk-live-abc123\"\n" into settings_file +close file settings_file + +set_file_mode of "settings.toml" and "0600" + +store mode as file_mode of "settings.toml" +display "settings.toml is now " with mode // Output: settings.toml is now 0600 +``` + +Common modes: + +| Mode | Meaning | +| --- | --- | +| `"0600"` | Owner can read and write; nobody else can read it. Use for anything secret. | +| `"0644"` | Owner can write; everyone can read. Fine for ordinary data files. | +| `"0700"` | Owner can read, write and run. Use for scripts only you should run. | + +**Malformed modes are refused, not guessed at.** `"rw-------"`, `"0999"` and `"0o600"` all raise an error rather than being reinterpreted as some other mode. Silently landing on a more permissive mode than the author intended is exactly the failure this function exists to prevent. + +**On Windows** this raises a clear "not supported" error rather than doing nothing. File modes do not map onto Windows ACLs, and quietly succeeding would leave a program believing it had protected a file that it had not. Restrict the file through Windows' own access control instead. + +**Use Cases:** +- Locking down a config file that holds an API key or password +- Making a generated key file owner-only +- Setting up a directory of credentials with predictable permissions + +--- + ### count_lines **Purpose:** Count lines in a text file. diff --git a/Docs/05-standard-library/index.md b/Docs/05-standard-library/index.md index d754bfba..1ef66f60 100644 --- a/Docs/05-standard-library/index.md +++ b/Docs/05-standard-library/index.md @@ -2,7 +2,7 @@ Built-in tools that use the same natural-language style as the rest of WFL—no separate package manager required for core work. See the [foundation principles](../wfl-foundation.md) on standard libraries and gradual learning. -WFL includes a comprehensive standard library with 181+ built-in functions across 11 modules. Everything you need is included. +WFL includes a comprehensive standard library with 181+ built-in functions across 12 modules. Everything you need is included. ## What's in the Standard Library? @@ -15,7 +15,8 @@ WFL's standard library provides: - **[Filesystem Module](filesystem-module.md)** - File and directory operations - **[Time Module](time-module.md)** - Date and time handling - **[Random Module](random-module.md)** - Random number generation -- **[Crypto Module](crypto-module.md)** - Password hashing, standard hashing/MAC, and experimental WFLHASH +- **[Crypto Module](crypto-module.md)** - Password hashing, authenticated encryption, standard hashing/MAC, and experimental WFLHASH +- **[TOML Module](toml-module.md)** - Reading and writing TOML configuration files - **[Pattern Module](pattern-module.md)** - Pattern matching utilities - **[Typechecker Module](typechecker-module.md)** - Type checking utilities diff --git a/Docs/05-standard-library/overview.md b/Docs/05-standard-library/overview.md index f587d304..d8989e18 100644 --- a/Docs/05-standard-library/overview.md +++ b/Docs/05-standard-library/overview.md @@ -1,6 +1,6 @@ # Standard Library Overview -WFL's standard library provides 181+ built-in functions organized into 11 modules. Everything you need is already included—no package managers, no external dependencies. +WFL's standard library provides 181+ built-in functions organized into 12 modules. Everything you need is already included—no package managers, no external dependencies. ## Library Architecture @@ -22,8 +22,10 @@ Standard Library (181+ functions) │ └── Date and time handling ├── Random Module (6 functions) │ └── Random number generation -├── Crypto Module (20 functions) -│ └── Password hashing, auth/session primitives, hashing & MAC +├── Crypto Module (22 functions) +│ └── Password hashing, authenticated encryption, auth/session primitives, hashing & MAC +├── TOML Module (3 functions) +│ └── Reading and writing TOML configuration files ├── Pattern Module (3 functions) │ └── Pattern matching utilities └── Typechecker Module @@ -140,6 +142,7 @@ store upper as touppercase of "text" - Directories: list, create, check existence - Paths: extension, basename, dirname, join - Information: exists, size, type +- Permissions: `file_mode`, `set_file_mode` ### Temporal (Time) - Current: `current time`, `current date`, `datetime_now` @@ -155,9 +158,14 @@ store upper as touppercase of "text" ### Security (Crypto) - Password hashing: `hash_password`, `verify_password` (Argon2id/bcrypt/scrypt/PBKDF2) +- Authenticated encryption: `seal`, `unseal` (XChaCha20-Poly1305) — for secrets you must read back - Standard hashing/MAC: `sha256`, `hmac_sha256` - WFLHASH (experimental): `wflhash256`, `wflhash512`, `wflhash256_with_salt`, `wflmac256` — dual-hash with `sha256` for production +### Configuration (TOML) +- Reading: `parse_toml` +- Writing: `stringify_toml`, `stringify_toml_pretty` + ### Validation (Pattern) - Matching: pattern matching, finding, replacing @@ -286,7 +294,7 @@ Use whichever reads most naturally in your code! In this overview, you learned: -✅ **Library organization** - 11 modules by functionality +✅ **Library organization** - 12 modules by functionality ✅ **Function count** - 181+ built-in functions ✅ **Naming conventions** - Natural language names ✅ **Syntax patterns** - Consistent `of` and `and` usage diff --git a/Docs/05-standard-library/toml-module.md b/Docs/05-standard-library/toml-module.md new file mode 100644 index 00000000..4d912502 --- /dev/null +++ b/Docs/05-standard-library/toml-module.md @@ -0,0 +1,162 @@ +# TOML Module + +The TOML module reads and writes [TOML](https://toml.io) — the format a large share of configuration files are written in. Parsing is the common case: a program reads a config file someone else wrote. + +WFL also has JSON functions with exactly the same shape (`parse_json`, `stringify_json`, `stringify_json_pretty`), so switching a program between the two formats is a matter of changing the function name. + +## How TOML maps onto WFL values + +| TOML | WFL | +| --- | --- | +| Table (`[section]`) | Object — read keys with `config["section"]` | +| Array of tables (`[[item]]`) | List of objects | +| Array (`[1, 2, 3]`) | List | +| String | Text | +| Integer, float | Number | +| Boolean | Boolean (`yes` / `no`) | +| Date, time, datetime | Text, in the format the file used | + +Dates and times come back as text rather than WFL date values. TOML distinguishes offset datetimes, local datetimes, local dates and local times, and collapsing those into one WFL type would lose information; keeping the original text keeps the round trip exact. + +## Functions + +### parse_toml + +**Purpose:** Read TOML text into a WFL value. + +**Signature:** +```wfl +parse_toml of <text> +``` + +**Parameters:** +- `text` (Text): TOML document text + +**Returns:** Object — the document's top-level table + +**Raises:** An error if the text is not valid TOML. Duplicate keys are an error, not last-wins, as the TOML specification requires. + +**Example:** +```wfl +store config_text as "title = \"my project\" +listen_port = 8080 +debug_mode = true + +[database] +host = \"localhost\" +" + +store config as parse_toml of config_text +display config["title"] // Output: my project +display config["listen_port"] // Output: 8080 + +store db_section as config["database"] +display db_section["host"] // Output: localhost +``` + +Reading a config file from disk is the usual case: + +```wfl +open file at "settings.toml" for reading as settings_file +store settings_text as read content from settings_file +close file settings_file + +store settings as parse_toml of settings_text +``` + +**Use Cases:** +- Reading application configuration +- Reading a tool's config file whose format is fixed by a specification +- Reading manifests and metadata files + +--- + +### stringify_toml + +**Purpose:** Convert a WFL value to TOML text. + +**Signature:** +```wfl +stringify_toml of <value> +``` + +**Parameters:** +- `value` (Object): The value to write. It must be an object — see "Writing rules" below. + +**Returns:** Text — a TOML document + +**Example:** +```wfl +store config as parse_toml of "name = \"wfl\" +listen_port = 8080 +" + +store rendered as stringify_toml of config +display rendered +// Output: +// listen_port = 8080 +// name = "wfl" +``` + +Keys come out in alphabetical order, not the order they appeared in the original +file. The output is deterministic — the same value always produces the same text, +which matters if you write a config file into version control — but round-tripping +a hand-written file will reorder it, and comments are not preserved. + +--- + +### stringify_toml_pretty + +**Purpose:** Convert a WFL value to TOML text with more readable formatting for nested structures. + +**Signature:** +```wfl +stringify_toml_pretty of <value> +``` + +**Parameters:** +- `value` (Object): The value to write + +**Returns:** Text — a TOML document + +**Example:** +```wfl +store config as parse_toml of "[server_config] +host = \"localhost\" +ports = [1, 2] +" + +store rendered as stringify_toml_pretty of config +store reparsed as parse_toml of rendered +store section as reparsed["server_config"] +display section["host"] // Output: localhost +``` + +## Writing rules + +Two things about TOML differ from JSON, and WFL is explicit about both rather than guessing. + +**A TOML document is always a table.** There is no valid TOML file whose top level is a list or a bare string, so `stringify_toml` accepts only an object and raises an error otherwise. Writing something that would not parse back would be worse than refusing. + +```wfl +store rendered as stringify_toml of [1 and 2 and 3] // Error: a TOML document is always a table +``` + +**TOML has no null.** Absence is spelled by leaving the key out, so a key whose value is `nothing` is simply omitted: + +```wfl +store settings as parse_toml of "present = \"yes\"\n" +store rendered as stringify_toml of settings // the key is written; a nothing-valued key would not be +``` + +Inside a *list* there is no way to leave a hole — dropping an entry would change the list's length — so `nothing` in a list is an error instead. + +**Whole numbers stay integers.** A number with no fractional part is written as a TOML integer, so a config round-trips as `listen_port = 8080` rather than `listen_port = 8080.0`. + +**Writing is not editing.** Parsing and re-writing a file produces a valid, equivalent document, but not the same bytes: keys are sorted alphabetically and comments are dropped. If you need to preserve someone's hand-written file exactly, read it and write your own output elsewhere rather than round-tripping theirs. + +## Related + +- [Filesystem Module](filesystem-module.md) — reading the file the TOML text comes from +- [Crypto Module](crypto-module.md) — `seal` and `unseal` for secrets you store in a config file +- [File I/O](../04-advanced-features/file-io.md) — opening and reading files diff --git a/Docs/reference/keyword-reference.md b/Docs/reference/keyword-reference.md index ab6a9fe7..d2f6505b 100644 --- a/Docs/reference/keyword-reference.md +++ b/Docs/reference/keyword-reference.md @@ -369,8 +369,8 @@ connect to database at "sqlite://app.db" as db - 24 contextual keywords CAN be used as variables in certain contexts - 5 appear contextual but are actually always reserved -### "What about `secured`, `certificate`, `key`, `redirecting`, `content_type`?" → Not keywords -These words are recognized purely by position inside `listen` / `respond` statements and are **never reserved** — use them as variable names freely. See [Marker Words That Are Not Keywords](reserved-keywords.md#marker-words-that-are-not-keywords-at-all). +### "What about `secured`, `certificate`, `key`, `redirecting`, `content_type`, `transaction`?" → Not keywords +These words are recognized purely by position — inside `listen` / `respond` statements, or in `in transaction on db:` / `end transaction` — and are **never reserved**. Use them as variable names freely. See [Marker Words That Are Not Keywords](reserved-keywords.md#marker-words-that-are-not-keywords-at-all). --- diff --git a/Docs/reference/reserved-keywords.md b/Docs/reference/reserved-keywords.md index ee5c9f98..82f16f32 100644 --- a/Docs/reference/reserved-keywords.md +++ b/Docs/reference/reserved-keywords.md @@ -100,14 +100,21 @@ A few words have special meaning in exactly one statement position but are **not - `key` - private key path marker in `secured with ... and key "key.pem"` - `redirecting` - redirect marker in `listen on port 8080 redirecting to port 8443 as server` - `content_type` - response content type marker in `respond to req with ... and content_type "text/html"` +- `transaction` - transaction block marker in `in transaction on db:` and `end transaction` ```wfl // All perfectly valid — these words are not reserved: store key as "secret_key_456" store certificate as "diploma" store secured as yes +store transaction as "TX-1094" ``` +`transaction` is recognized in exactly two positions: directly after a leading +`in`, and directly after the `end` that closes the block. A statement beginning +with `in` was always a parse error before, so nothing that used to be valid +changed meaning. + ### Why Some Keywords Appear in Multiple Lists You might notice keywords like `push`, `zero`, and `than` appear in both the structural and contextual keyword lists in the compiler source code. This isn't a bug—it's by design: diff --git a/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md new file mode 100644 index 00000000..df079e28 --- /dev/null +++ b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md @@ -0,0 +1,222 @@ +# 2026-07-31 — Four gaps a config subsystem found (#664, #665, #666, #667) + +## What changed + +Four issues filed the same morning by one person who had just built a real +configuration subsystem in WFL. Read together they describe a language that +could mint a secret but not seal it, write a config file but not protect it, +read JSON but not the format most config files use, and write several rows but +not atomically. All four are now closed. + +| Issue | Before | After | +| --- | --- | --- | +| #664 | `BEGIN`/`ROLLBACK` through `execute` silently did nothing | `in transaction on db: … end transaction`, and raw transaction SQL is refused | +| #665 | `secure_random_bytes of 32` minted a key with nothing to use it for | `seal` / `unseal` (XChaCha20-Poly1305) | +| #666 | No way to set or read a file's mode | `file_mode`, `set_file_mode` | +| #667 | No TOML support at all | `parse_toml`, `stringify_toml`, `stringify_toml_pretty` | + +## #664 was the real bug + +The other three are missing features. This one was a silent data-integrity +failure, which is worse, because it looked like it worked. + +`open database` maintains a pool of five connections, and every `query`/`execute` +takes whichever one is free. WFL had no transaction construct, so the natural +workaround was to send transaction control as SQL: + +```wfl +store t1 as execute db with "BEGIN" +store ins as execute db with "INSERT INTO projects (slug) VALUES ('should-vanish')" +store t2 as execute db with "ROLLBACK" +``` + +Those three statements land on three *different* connections. The `BEGIN` opens +a transaction on a connection that then goes back to the pool untouched; the +`INSERT` commits on its own; the `ROLLBACK` rolls back an empty transaction +somewhere else. Nothing errors. The reporter measured +`rows surviving rollback: 1` where the code plainly says 0. + +The part that makes this properly nasty is the test story. In-memory SQLite is +special-cased to a single connection, because an in-memory database only exists +for the connection that made it. So the workaround *works* under +`sqlite::memory:` — which is what most tests use — and fails against a real +file-backed or networked database. A program could have a green suite and lose +data in production. + +That shaped the tests before it shaped the fix: every atomicity assertion in +`tests/database_transaction_test.rs` and +`TestPrograms/database_transaction_test.wfl` runs against a temp *file*, with a +comment saying why, so nobody later "simplifies" them to in-memory and quietly +removes the coverage. + +### The fix, both halves + +`Pool::begin()` hands back an owned `Transaction<'static, DB>`, so a transaction +can be parked in a map keyed by database handle, right alongside the pool +itself. While an entry is present, `query`/`execute` on that handle route to the +transaction's pinned connection instead of taking a fresh one. That is the whole +mechanism; `run_query` and `run_execute` grew a `DbTarget` parameter that is +either a pool or a transaction, and the per-backend bodies became a macro so the +six combinations stayed one line each rather than six copies. + +The block commits on a normal exit and rolls back on an error. `break`, +`continue` and `return` commit — they are ordinary ways to leave a block, and +the work inside genuinely finished. Only a failure rolls back. Nesting on one +handle and closing a database mid-transaction are refused with an explanation +rather than doing something surprising. + +The second half matters as much: `BEGIN`, `COMMIT`, `ROLLBACK`, +`START TRANSACTION`, `SAVEPOINT` and `RELEASE` through `query`/`execute` now +raise an error that names the block syntax. This is the only +compatibility-adjacent change in the batch, and it is safe precisely because the +old behavior did not work — no *working* program could depend on it. Only the +leading statement keyword is inspected, so a column named `begin_at` or a value +of `'rollback plan'` still runs; there is a test for exactly that. + +### `transaction` is not a keyword + +Making it a lexer token would have broken any program using `transaction` as a +variable name, and backward compatibility is not negotiable here. It is +recognized positionally instead — an identifier directly after a leading `in`, +and after the `end` that closes the block. A statement beginning with `in` was +always a parse error before, so the syntax claims nothing that used to be valid. +It joins `secured`, `certificate`, `key`, `redirecting` and `content_type` in +the "marker words that are not keywords" list, and the reserved-keyword count +stays at 181. + +One parser detail worth recording: the header uses `parse_primary_expression`, +not `parse_expression`. The general expression parser runs straight past the `:` +that ends the header and reports "expected Colon, found Eol" from the *next* +line — the same choice `open database at <url>` already makes. + +## #665 — the key stops being decorative + +The crypto stdlib was entirely one-way: hashes, MACs, password KDFs, CSPRNG, +constant-time compare. Every one of those verifies something you were handed. A +stored API token has to be *sent* to the service later, so a one-way function +cannot stand in — the reporter's design ended up storing a key and encrypting +nothing. + +XChaCha20-Poly1305, from RustCrypto. The choice over AES-GCM is about the nonce: +192 bits is wide enough that a freshly generated random nonce per message is +safe with no counter and no caller-visible state. That lets the implementation +own nonce handling completely, and nonce reuse is the one mistake that breaks +this kind of encryption outright. A natural-language surface is the last place +to expose it. + +```wfl +store project_key as secure_random_bytes of 32 +store sealed as seal of api_token and project_key +store token as unseal of sealed and project_key +``` + +The optional third argument is associated data, binding a ciphertext to its +context so a value lifted out of one record cannot be replayed into another. Two +arguments and three are the same call, so the beginner form is a strict subset +of the expert one — no unlearning. + +Sealed values are `wflseal1:` plus hex of nonce ‖ ciphertext ‖ tag: +self-describing, versioned so the algorithm can change later without ambiguity +about what an old stored value holds, and hex to match `secure_random_bytes`, +which is where the key comes from. `unseal` reports every failure identically — +wrong key, flipped byte, truncated blob, mismatched context — so it cannot be +used as an oracle by someone holding the ciphertext. + +## #666 — 0600 moves into the program + +The reporter's mitigation was `UMask=0077` in a systemd unit. It works, and it +is invisible to the program and unverifiable from inside it. The half they cared +about more was the reading: even with a correct umask, a WFL program could not +implement "refuse to start if this config is group- or world-readable," because +it could not read the mode at all. + +```wfl +set_file_mode of "settings.toml" and "0600" +store mode as file_mode of "settings.toml" // "0600" +``` + +Mode parsing is strict on purpose. `"rw-------"`, `"0999"` and `"0o600"` are +errors, not values to be masked into *some* mode — landing on something more +permissive than the author wrote is exactly the failure this exists to prevent. + +Windows was the interesting call. Modes do not map onto ACLs, so `set_file_mode` +raises an explicit unsupported error there rather than quietly doing nothing; +`file_mode` still returns a documented approximation from the read-only +attribute, so a cross-platform program can call it unconditionally. The +integration gate runs on Windows as well as Linux, so +`TestPrograms/file_mode_test.wfl` probes for support once and then asserts +whichever contract applies — the per-platform detail lives in the cfg-gated +Rust tests. + +## #667 — the JSON deviation goes away + +The reporter's specification mandated TOML 1.0. Their options were to hand-roll +a TOML subset in WFL, which gets progressively more wrong as it meets real TOML, +or to change the format; they changed the format to JSON and wrote down the +deviation. + +`src/stdlib/toml.rs` is a close mirror of `src/stdlib/json.rs`, deliberately. +The issue sketched `parse_toml` / `to_toml`, but the existing JSON surface is +`parse_json` / `stringify_json` / `stringify_json_pretty` — matching the sketch +would have introduced a new inconsistency, so the naming mirrors what is already +there. + +Two places TOML genuinely is not JSON, both handled explicitly rather than +fudged: + +- **A TOML document is always a table.** There is no valid TOML file whose top + level is an array. `stringify_toml` accepts only an object and says so, rather + than emitting something that will not parse back. +- **TOML has no null.** Absence is a missing key, so a `nothing` value is + omitted when writing a table. Inside an *array* there is no way to leave a + hole, so that is an error — silently dropping an element would change the + array's length. + +Whole numbers write as TOML integers, so a round-tripped config reads +`listen_port = 8080` and not `listen_port = 8080.0`. + +The module file is named `toml.rs` for symmetry with `json.rs`, and refers to +the crate as `::toml::` throughout so the module never shadows it. + +## Testing + +R3 across the board — concurrency and lifecycle and backward compatibility +(#664), crypto and secrets (#665), secrets and untrusted input (#666), untrusted +input (#667). Red first: a tests-only commit +(`test: failing coverage for transactions, AEAD, file modes and TOML`) with all +four suites failing for the intended reason, recorded in +`Engineering/evidence/red-664-667-transactions-aead-modes-toml.txt`: + +```text +database_transaction_test parse error on `in transaction on db:` +crypto_seal_test Undefined variable 'seal' +filesystem_mode_test Undefined variable 'file_mode' / 'set_file_mode' +toml_test Undefined variable 'parse_toml' +``` + +Rust suites in `tests/`, WFL end-to-end suites in `TestPrograms/` using +`describe`/`test` (the integration runner detects `describe` and adds `--test`, +so a failed assertion exits nonzero). The negative paths carry the weight: for +`unseal`, a wrong key, a flipped nonce byte, a flipped ciphertext byte, a +flipped tag byte, a truncated blob, an unknown version prefix and a mismatched +context; for transactions, rollback on error, atomicity across a partially +successful block, read-your-own-writes inside the block, nesting, close-during, +and the raw-SQL rejection including the case it must *not* fire on. + +## Gotchas hit along the way + +- **Two builtin catalogs and a static-contract table.** `builtins.rs` holds both + a reserved-name list and a runtime inventory, and `stdlib/typechecker.rs` + holds the static contracts; a lib test asserts the runtime inventory matches + the registrations exactly. Six new builtins meant touching all of them, plus + the arity tables. The test caught it immediately, which is the point of it. +- **`second` is a builtin.** A TestProgram variable named `second` failed with a + confusing module-scope error. `port`, `server` and `text` are keywords too — + worth remembering when naming test variables. +- **`expect` does not count as a use.** The analyzer reports variables that are + only read by an `expect` assertion as unused. Pre-existing, noisy in test + programs, not addressed here. +- **Disk.** A full debug+release build with `debug = true` on release exceeds + this environment's allowance. Building the test profile with + `CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0` keeps `target/` small + enough to hold both without touching the committed profile. diff --git a/TestPrograms/crypto_seal_test.wfl b/TestPrograms/crypto_seal_test.wfl new file mode 100644 index 00000000..129d27fc --- /dev/null +++ b/TestPrograms/crypto_seal_test.wfl @@ -0,0 +1,111 @@ +// Authenticated encryption test suite (issue #665) +// +// `seal` and `unseal` are the recoverable counterpart to the hashing functions: +// a stored API token has to be readable again, so a one-way hash cannot stand in. +// These tests cover the round trip and, more importantly, that tampering fails. + +describe "Sealing and unsealing": + + test "a sealed secret comes back unchanged": + store secret_key as secure_random_bytes of 32 + store sealed as seal of "sk-live-provider-token" and secret_key + store plain as unseal of sealed and secret_key + expect plain to equal "sk-live-provider-token" + end test + + test "a key from secure_random_bytes is directly usable": + store project_key as secure_random_bytes of 32 + expect length of project_key to equal 64 + store sealed as seal of "project secret" and project_key + store plain as unseal of sealed and project_key + expect plain to equal "project secret" + end test + + test "the sealed value does not contain the plaintext": + store secret_key as secure_random_bytes of 32 + store sealed as seal of "correct-horse-battery-staple" and secret_key + expect sealed to contain "wflseal1:" + end test + + test "sealing the same text twice gives different results": + // A fresh nonce per seal, so identical plaintexts never look alike. + store secret_key as secure_random_bytes of 32 + store first_seal as seal of "same message" and secret_key + store other_seal as seal of "same message" and secret_key + store are_identical as first_seal is other_seal + expect are_identical to be no + end test + + test "unicode and empty text round-trip": + store secret_key as secure_random_bytes of 32 + store empty_sealed as seal of "" and secret_key + store empty_plain as unseal of empty_sealed and secret_key + expect empty_plain to equal "" + + store uni_sealed as seal of "cafe 🌍" and secret_key + store uni_plain as unseal of uni_sealed and secret_key + expect uni_plain to equal "cafe 🌍" + end test + +end describe + +describe "Sealing with a context": + + test "a matching context unseals": + store secret_key as secure_random_bytes of 32 + store sealed as seal of "provider token" and secret_key and "project:acme/api_key" + store plain as unseal of sealed and secret_key and "project:acme/api_key" + expect plain to equal "provider token" + end test + + test "a different context does not unseal": + store secret_key as secure_random_bytes of 32 + store sealed as seal of "provider token" and secret_key and "project:acme/api_key" + store refused as no + try: + store plain as unseal of sealed and secret_key and "project:evil/api_key" + when error: + change refused to yes + end try + expect refused to be yes + end test + +end describe + +describe "Unsealing fails closed": + + test "the wrong key is refused": + store secret_key as secure_random_bytes of 32 + store other_key as secure_random_bytes of 32 + store sealed as seal of "top secret" and secret_key + store refused as no + try: + store plain as unseal of sealed and other_key + when error: + change refused to yes + end try + expect refused to be yes + end test + + test "text that was never sealed is refused": + store secret_key as secure_random_bytes of 32 + store refused as no + try: + store plain as unseal of "just some text a user typed" and secret_key + when error: + change refused to yes + end try + expect refused to be yes + end test + + test "a key of the wrong length is refused": + store refused as no + try: + store sealed as seal of "secret" and "tooshort" + when error: + change refused to yes + end try + expect refused to be yes + end test + +end describe diff --git a/TestPrograms/database_transaction_test.wfl b/TestPrograms/database_transaction_test.wfl new file mode 100644 index 00000000..977ebf6d --- /dev/null +++ b/TestPrograms/database_transaction_test.wfl @@ -0,0 +1,147 @@ +// Transaction test suite (issue #664) +// +// These use a FILE-backed SQLite database on purpose. `open database` hands out +// a pool of five connections and the defect in #664 was that each statement +// took a different one; in-memory SQLite is capped at a single connection, so +// an in-memory test cannot see the bug at all. +// +// The database lives under target/test-artifacts/ per REPOSITORY_HYGIENE.md. + +store artifact_dir as "target/test-artifacts/database_transaction_test" +create directory at artifact_dir +store db_url as "sqlite://" with artifact_dir with "/tx.db" + +describe "A transaction block is atomic": + + test "work inside a failed block does not survive": + open database at db_url as db + store dropped as execute db with "DROP TABLE IF EXISTS rollback_case" + store made as execute db with "CREATE TABLE rollback_case (slug TEXT)" + + store failed as no + try: + in transaction on db: + store ins as execute db with "INSERT INTO rollback_case (slug) VALUES ('should-vanish')" + store boom as execute db with "INSERT INTO no_such_table (x) VALUES (1)" + end transaction + when error: + change failed to yes + end try + expect failed to be yes + + store rows as query db with "SELECT slug FROM rollback_case" + expect length of rows to equal 0 + close database db + end test + + test "work inside a finished block is committed": + open database at db_url as db + store dropped as execute db with "DROP TABLE IF EXISTS commit_case" + store made as execute db with "CREATE TABLE commit_case (slug TEXT)" + + in transaction on db: + store a as execute db with "INSERT INTO commit_case (slug) VALUES ('kept-one')" + store b as execute db with "INSERT INTO commit_case (slug) VALUES ('kept-two')" + end transaction + + store rows as query db with "SELECT slug FROM commit_case" + expect length of rows to equal 2 + close database db + end test + + test "an earlier write rolls back with a later failure": + open database at db_url as db + store dropped as execute db with "DROP TABLE IF EXISTS all_or_nothing" + store made as execute db with "CREATE TABLE all_or_nothing (slug TEXT UNIQUE)" + store seed as execute db with "INSERT INTO all_or_nothing (slug) VALUES ('taken')" + + store failed as no + try: + in transaction on db: + store a as execute db with "INSERT INTO all_or_nothing (slug) VALUES ('new-one')" + store b as execute db with "INSERT INTO all_or_nothing (slug) VALUES ('taken')" + end transaction + when error: + change failed to yes + end try + expect failed to be yes + + store rows as query db with "SELECT slug FROM all_or_nothing" + expect length of rows to equal 1 + close database db + end test + + test "a read inside the block sees the block's own writes": + open database at db_url as db + store dropped as execute db with "DROP TABLE IF EXISTS read_own_writes" + store made as execute db with "CREATE TABLE read_own_writes (slug TEXT)" + + in transaction on db: + store ins as execute db with "INSERT INTO read_own_writes (slug) VALUES ('pending')" + store rows as query db with "SELECT slug FROM read_own_writes" + store seen as length of rows + end transaction + expect seen to equal 1 + + close database db + end test + +end describe + +describe "Transaction control SQL is refused": + + test "a raw BEGIN through execute is an error": + open database at db_url as db + store refused as no + try: + store t as execute db with "BEGIN" + when error: + change refused to yes + end try + expect refused to be yes + close database db + end test + + test "a raw ROLLBACK through execute is an error": + open database at db_url as db + store refused as no + try: + store t as execute db with "ROLLBACK" + when error: + change refused to yes + end try + expect refused to be yes + close database db + end test + + test "ordinary SQL that merely mentions those words still runs": + open database at db_url as db + store dropped as execute db with "DROP TABLE IF EXISTS audit" + store made as execute db with "CREATE TABLE audit (begin_at TEXT, commit_note TEXT)" + store ins as execute db with "INSERT INTO audit (begin_at, commit_note) VALUES ('t0', 'rollback plan')" + store rows as query db with "SELECT begin_at, commit_note FROM audit" + expect length of rows to equal 1 + close database db + end test + +end describe + +describe "Transaction misuse is reported": + + test "nesting a transaction on the same database is an error": + open database at db_url as db + store refused as no + try: + in transaction on db: + in transaction on db: + store noop as query db with "SELECT 1 AS one" + end transaction + end transaction + when error: + change refused to yes + end try + expect refused to be yes + close database db + end test + +end describe diff --git a/TestPrograms/file_mode_test.wfl b/TestPrograms/file_mode_test.wfl new file mode 100644 index 00000000..6dd8fc43 --- /dev/null +++ b/TestPrograms/file_mode_test.wfl @@ -0,0 +1,111 @@ +// File permission test suite (issue #666) +// +// The point of these builtins is that a program can both restrict a file it +// writes and check that it is restricted — "refuse to start if this config is +// group- or world-readable" was impossible to express before. +// +// Writes go under target/test-artifacts/ per REPOSITORY_HYGIENE.md. +// +// `set_file_mode` implements real POSIX semantics on Unix and refuses loudly on +// Windows, where modes do not map onto ACLs. This runs on both platforms, so it +// probes once for support and then asserts whichever contract applies. The +// per-platform detail is covered exhaustively by tests/filesystem_mode_test.rs. + +store artifact_dir as "target/test-artifacts/file_mode_test" +store config_path as artifact_dir with "/config.toml" +store probe_path as artifact_dir with "/probe.toml" + +create directory at artifact_dir +create file at probe_path with "probe\n" + +store modes_supported as yes +try: + set_file_mode of probe_path and "0600" +when error: + change modes_supported to no +end try + +describe "Reading a file's mode": + + setup: + create directory at artifact_dir + create file at config_path with "token = \"secret\"\n" + end setup + + test "file_mode returns four octal digits": + store mode as file_mode of config_path + expect length of mode to equal 4 + end test + + test "reading the mode of a missing file fails": + store failed as no + try: + store mode as file_mode of "target/test-artifacts/file_mode_test/nope.toml" + when error: + change failed to yes + end try + expect failed to be yes + end test + +end describe + +describe "Restricting a file to its owner": + + setup: + create directory at artifact_dir + create file at config_path with "token = \"secret\"\n" + end setup + + test "a file set to 0600 reads back as 0600": + check if modes_supported: + set_file_mode of config_path and "0600" + store mode as file_mode of config_path + expect mode to equal "0600" + otherwise: + // Windows: refused loudly, which is the documented contract there. + expect modes_supported to be no + end check + end test + + test "a group-readable config is distinguishable from a locked-down one": + check if modes_supported: + set_file_mode of config_path and "0644" + store loose_mode as file_mode of config_path + store is_locked_down as loose_mode is "0600" + expect is_locked_down to be no + + set_file_mode of config_path and "0600" + store tight_mode as file_mode of config_path + store now_locked_down as tight_mode is "0600" + expect now_locked_down to be yes + otherwise: + expect modes_supported to be no + end check + end test + + test "three-digit and four-digit modes are both accepted": + check if modes_supported: + set_file_mode of config_path and "600" + store after_short as file_mode of config_path + expect after_short to equal "0600" + + set_file_mode of config_path and "0640" + store after_long as file_mode of config_path + expect after_long to equal "0640" + otherwise: + expect modes_supported to be no + end check + end test + + test "a malformed mode is rejected rather than masked": + // Rejected before the platform check, so this holds everywhere. + store failed as no + try: + set_file_mode of config_path and "rw-------" + when error: + change failed to yes + end try + expect failed to be yes + end test + +end describe diff --git a/TestPrograms/toml_test.wfl b/TestPrograms/toml_test.wfl new file mode 100644 index 00000000..93c83f13 --- /dev/null +++ b/TestPrograms/toml_test.wfl @@ -0,0 +1,139 @@ +// TOML test suite (issue #667) +// +// Mirrors the JSON surface: parse_toml / stringify_toml / stringify_toml_pretty. +// The motivating case is reading a config file whose format is fixed by a spec +// that says TOML. + +describe "Reading a config": + + test "scalars come through with the right types": + store config_text as "title = \"my project\" +listen_port = 8080 +debug_mode = true +ratio = 0.25 +" + store config as parse_toml of config_text + expect config["title"] to equal "my project" + expect config["listen_port"] to equal 8080 + expect config["debug_mode"] to be yes + expect config["ratio"] to equal 0.25 + end test + + test "nested tables become nested objects": + store config_text as "[database] +host = \"localhost\" + +[database.tls] +enabled = true +" + store config as parse_toml of config_text + store db_section as config["database"] + expect db_section["host"] to equal "localhost" + store tls_section as db_section["tls"] + expect tls_section["enabled"] to be yes + end test + + test "arrays become lists": + store config_text as "hosts = [\"a\", \"b\", \"c\"] +" + store config as parse_toml of config_text + store hosts as config["hosts"] + expect length of hosts to equal 3 + expect hosts[0] to equal "a" + expect hosts[2] to equal "c" + end test + + test "arrays of tables become lists of objects": + store config_text as "[[project]] +slug = \"alpha\" + +[[project]] +slug = \"beta\" +" + store config as parse_toml of config_text + store projects as config["project"] + expect length of projects to equal 2 + store second_project as projects[1] + expect second_project["slug"] to equal "beta" + end test + + test "an empty document is an empty table": + store config as parse_toml of "" + store rendered as stringify_toml of config + expect rendered to equal "" + end test + +end describe + +describe "Rejecting bad input": + + test "malformed TOML raises an error": + store failed as no + try: + store config as parse_toml of "[unclosed" + when error: + change failed to yes + end try + expect failed to be yes + end test + + test "duplicate keys are an error, not last-wins": + store failed as no + try: + store config as parse_toml of "a = 1 +a = 2 +" + when error: + change failed to yes + end try + expect failed to be yes + end test + +end describe + +describe "Writing a config": + + test "a config survives a round trip": + store config_text as "name = \"wfl\" +listen_port = 8080 +debug_mode = false +" + store config as parse_toml of config_text + store rendered as stringify_toml of config + store reparsed as parse_toml of rendered + expect reparsed["name"] to equal "wfl" + expect reparsed["listen_port"] to equal 8080 + expect reparsed["debug_mode"] to be no + end test + + test "whole numbers stay integers rather than becoming decimals": + store config as parse_toml of "listen_port = 8080 +" + store rendered as stringify_toml of config + expect rendered to contain "listen_port = 8080" + end test + + test "nested structure survives pretty printing": + store config_text as "[server_config] +host = \"localhost\" +ports = [1, 2] +" + store config as parse_toml of config_text + store rendered as stringify_toml_pretty of config + store reparsed as parse_toml of rendered + store section as reparsed["server_config"] + expect section["host"] to equal "localhost" + expect length of section["ports"] to equal 2 + end test + + test "a top-level list is not a TOML document": + store failed as no + try: + store rendered as stringify_toml of [1 and 2 and 3] + when error: + change failed to yes + end try + expect failed to be yes + end test + +end describe From c8a961293883feb11166e77ae17d8d32ee0d2c43 Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Fri, 31 Jul 2026 08:44:57 +0000 Subject: [PATCH 07/10] =?UTF-8?q?fix:=20review=20findings=20=E2=80=94=20AE?= =?UTF-8?q?AD=20panic,=20transaction=20scoping,=20concurrency=20and=20lint?= =?UTF-8?q?=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the automated review on #676. Two findings were more serious than anything the original change caught, and one contradicted a claim I had made. **`seal`/`unseal` panicked on non-ASCII input.** `hex_to_bytes` checked that the byte length was even and then sliced the `str` at fixed two-byte offsets, which lands inside a multi-byte character and panics. Keys and sealed values are exactly the untrusted text this module promises to fail closed on — they arrive from config files, database rows and HTTP requests — so one accented character in a stored blob could abort a running server. Decoded over bytes now. **A transaction was owned by a database handle, not by the handler that opened it.** Under `main loop concurrently:` handlers share one IoClient and can name the same global handle, so an unrelated request running an ordinary `execute` was silently enrolled in another request's transaction and had its write rolled back with it. Transactions are keyed by `(scope, handle)`, where the scope is handler-local state swapped per poll by `InstalledRunState` alongside the existing count-loop and call-stack isolation. `tokio::task_local!` would not work here: handlers are futures in a `FuturesUnordered` on one thread, not separate tasks. The new test drives two real requests over a socket and asserts the bystander's row survives; removing the scope key fails it with both rows gone. Also: - The transaction map's lock was held across the SQL await, so one slow statement serialized every database operation in the program. Transactions now sit behind their own per-handle lock and the map's lock is held only long enough to clone an Arc. - `begin` had a check-then-insert race in which a second concurrent begin could replace and drop a live transaction. The handle is reserved under one atomic step before the round-trip, and the reservation is removed if begin fails. - A leading SQL comment defeated the transaction-control guard outright: `-- go\nBEGIN` read as an empty first word and went to the pool. Comments are skipped before the first token is read. - Two of the analyzer's three AST walkers had no arm for the new statement, so a `random_seed` call inside a transaction block escaped the insecure-RNG security lint and declarations inside one were invisible to unused-variable analysis. `seal`/`unseal` join SECURITY_SENSITIVE_BUILTINS. - `exit` inside a transaction committed its partial work, contradicting the rule that an abrupt stop discards it. It now rolls back, like a program that ends with a transaction still open. `break`/`continue`/`return` still commit. - The transaction arm is awaited behind a `Box::pin`. `execute_statement` is a plain async fn, so every await in every arm enlarges the one state machine each level of statement recursion keeps on the stack. - `fuzz/Cargo.lock` regenerated for the new dependencies; the fuzz job builds with `--locked`. Documentation corrections, all verified against the binary rather than asserted: - I wrote that rejecting hand-written transaction SQL broke nothing "because the old behavior did not work". That is wrong. In-memory SQLite is capped at one connection, so under `sqlite::memory:` the pattern genuinely worked — I confirmed it by building main and watching a rollback discard its row. It still ships as a hard error, deliberately, because the same program silently loses writes against any pooled backend; the changelog now records it under Removed and the docs say so outright instead of implying nothing is lost. - TOML dates round-trip as strings, not dates, and integers above 2^53 are rounded by WFL's f64 numbers. The page had claimed the round trip was exact. Both are now stated and pinned with tests. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- CHANGELOG.md | 35 +- Docs/04-advanced-features/databases.md | 31 +- Docs/05-standard-library/toml-module.md | 8 +- ...07-31-transactions-aead-file-modes-toml.md | 72 +++- fuzz/Cargo.lock | 104 ++++++ src/analyzer/static_analyzer.rs | 18 +- src/interpreter/database.rs | 34 +- src/interpreter/mod.rs | 314 +++++++++++++----- src/stdlib/crypto.rs | 19 +- tests/crypto_seal_test.rs | 52 +++ tests/database_transaction_test.rs | 208 ++++++++++++ tests/toml_test.rs | 64 ++++ tests/transaction_analyzer_walk_test.rs | 126 +++++++ tests/transaction_handler_scope_test.rs | 230 +++++++++++++ 14 files changed, 1207 insertions(+), 108 deletions(-) create mode 100644 tests/transaction_analyzer_walk_test.rs create mode 100644 tests/transaction_handler_scope_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f671ff3f..02c3388d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,11 +9,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Added - **Database transactions** (#664): `in transaction on db: ... end transaction` runs a group of statements on a single pooled connection, committing when the - block finishes and rolling back if anything inside it fails. Transaction blocks - cannot be nested on one database, and a database cannot be closed inside its - own transaction; both are reported rather than silently tolerated. - `transaction` is a positional marker word, not a reserved keyword, so programs - already using it as a variable name are unaffected. + block finishes and rolling back if anything inside it fails or if `exit` stops + the program mid-block. Transaction blocks cannot be nested on one database, and + a database cannot be closed inside its own transaction; both are reported + rather than silently tolerated. Under `main loop concurrently:` a transaction + belongs to the handler that opened it, so a concurrent handler using the same + database handle is never enrolled in it. `transaction` is a positional marker + word, not a reserved keyword, so programs already using it as a variable name + are unaffected. - **Authenticated encryption** (#665): `seal of <text> and <key>` and `unseal of <sealed> and <key>`, backed by XChaCha20-Poly1305. Keys are the 64-hex-character values `secure_random_bytes of 32` already produced; nonces @@ -38,11 +41,23 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), `RELEASE` previously ran on arbitrary pooled connections, so a hand-written `BEGIN`/`ROLLBACK` sequence silently failed to cover the statements between them: the rollback undid nothing, the writes survived, and no error was - raised. In-memory SQLite hid this entirely because it is capped at one - connection. These statements now raise an error naming the transaction block. - Only the leading statement keyword is inspected, so ordinary SQL that merely - contains those words (a `begin_at` column, a `'rollback plan'` value) still - runs. + raised. These statements now raise an error naming the transaction block. + Leading SQL comments are skipped before the keyword is read, so + `-- go\nBEGIN` is caught too; only the first real token is inspected, so + ordinary SQL that merely contains those words (a `begin_at` column, a + `'rollback plan'` value) still runs. + +### Removed +- **Hand-written transaction control through `query`/`execute` no longer runs, + including on `sqlite::memory:`** (#664). On pooled backends the pattern never + worked, but an in-memory SQLite database is capped at a single connection, so + there `BEGIN`/`COMMIT`/`ROLLBACK` through `execute` did genuinely take effect. + Programs relying on that — most likely tests and examples, which commonly use + `sqlite::memory:` — now raise an error and must use `in transaction on db:` + instead. This is deliberate rather than a deprecation: the same code silently + corrupts data the moment it is pointed at a file-backed or networked database, + so "works in development, loses writes in production" is the behaviour being + removed. The error names the replacement construct, and the fix is mechanical. - **Binding Repository Hygiene and Layout Policy** (`REPOSITORY_HYGIENE.md`, governance §3.8) with a machine-readable profile (`.repo-hygiene.toml`) and a diff --git a/Docs/04-advanced-features/databases.md b/Docs/04-advanced-features/databases.md index c92df40c..c8b1dd83 100644 --- a/Docs/04-advanced-features/databases.md +++ b/Docs/04-advanced-features/databases.md @@ -142,9 +142,13 @@ end try - **Reads see the block's own writes.** A `query` inside the block sees rows the block has inserted but not yet committed. Other connections do not see them until the block commits. -- **Only errors roll back.** `break`, `continue` and `return` are ordinary ways - to leave a block, so they commit — the work inside finished. A rollback - happens when a statement fails. +- **`break`, `continue` and `return` commit.** They are ordinary ways to leave a + block, so the work inside finished and is kept. +- **Errors and `exit` roll back.** A failed statement rolls the block back, and + so does `exit`, which stops the program where it stands rather than finishing + the block. A transaction still open when the program ends rolls back for the + same reason — an abrupt stop discards the partial work rather than + half-committing it. ### Restrictions @@ -171,9 +175,26 @@ nothing while the writes it was meant to discard survived. `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` are therefore rejected with an error naming the block syntax above. -Only the leading keyword of a statement is checked, so ordinary SQL that merely +Only the first real token of a statement is checked, so ordinary SQL that merely contains those words — a column named `begin_at`, a value of `'rollback plan'` — -runs normally. +runs normally. Leading comments are skipped before that token is read, so +`-- set up\nBEGIN` is refused rather than slipping past. + +> **If this used to work for you, it worked by accident.** An in-memory SQLite +> database (`sqlite::memory:`) only ever has one connection, so hand-written +> transaction control did take effect there — and nowhere else. The same program +> pointed at a file-backed or networked database silently lost the writes it +> meant to roll back. That is why this is now an error everywhere rather than a +> warning: the pattern's failure mode was to pass in development and corrupt data +> in production. Replace it with the block above. + +### Transactions and concurrent handlers + +Under `main loop concurrently:` a transaction belongs to the handler that opened +it. Two requests can hold their own transactions on the same database handle at +the same time, and a handler that has no transaction of its own keeps taking a +pooled connection as usual — it is never enrolled in someone else's transaction, +and cannot have its writes committed or rolled back by another request. > `transaction` is not a reserved word. It is recognized only in > `in transaction on ...` and `end transaction`, so existing programs that use diff --git a/Docs/05-standard-library/toml-module.md b/Docs/05-standard-library/toml-module.md index 4d912502..59655ffe 100644 --- a/Docs/05-standard-library/toml-module.md +++ b/Docs/05-standard-library/toml-module.md @@ -16,7 +16,9 @@ WFL also has JSON functions with exactly the same shape (`parse_json`, `stringif | Boolean | Boolean (`yes` / `no`) | | Date, time, datetime | Text, in the format the file used | -Dates and times come back as text rather than WFL date values. TOML distinguishes offset datetimes, local datetimes, local dates and local times, and collapsing those into one WFL type would lose information; keeping the original text keeps the round trip exact. +Dates and times come back as **text**, in exactly the form the file used. TOML distinguishes offset datetimes, local datetimes, local dates and local times, and collapsing those into a single WFL date value would lose the distinction. + +The cost is that a date does not survive a *write* as a date. Reading `released = 2026-07-31` and writing it back produces `released = "2026-07-31"` — same characters, but now a TOML string rather than a TOML date. If a consumer of your file cares about the difference, read the value and construct the output yourself instead of round-tripping. ## Functions @@ -153,7 +155,9 @@ Inside a *list* there is no way to leave a hole — dropping an entry would chan **Whole numbers stay integers.** A number with no fractional part is written as a TOML integer, so a config round-trips as `listen_port = 8080` rather than `listen_port = 8080.0`. -**Writing is not editing.** Parsing and re-writing a file produces a valid, equivalent document, but not the same bytes: keys are sorted alphabetically and comments are dropped. If you need to preserve someone's hand-written file exactly, read it and write your own output elsewhere rather than round-tripping theirs. +**Writing is not editing.** Parsing and re-writing a file produces a valid, equivalent document, but not the same bytes: keys are sorted alphabetically, comments are dropped, and dates become strings (see above). If you need to preserve someone's hand-written file exactly, read it and write your own output elsewhere rather than round-tripping theirs. + +**Very large integers lose precision.** WFL numbers are 64-bit floating point, which represents whole numbers exactly only up to 9,007,199,254,740,991 (2⁵³−1). A TOML integer above that is rounded on the way in — `9007199254740993` reads back as `9007199254740992` — so a parse-and-rewrite cycle can silently change it. This applies to every number in WFL, JSON included, not just TOML; if a config carries identifiers that large, keep them as quoted strings. ## Related diff --git a/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md index df079e28..27616a8f 100644 --- a/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md +++ b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md @@ -67,11 +67,24 @@ rather than doing something surprising. The second half matters as much: `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` through `query`/`execute` now -raise an error that names the block syntax. This is the only -compatibility-adjacent change in the batch, and it is safe precisely because the -old behavior did not work — no *working* program could depend on it. Only the -leading statement keyword is inspected, so a column named `begin_at` or a value -of `'rollback plan'` still runs; there is a test for exactly that. +raise an error that names the block syntax. Only the first real token is +inspected — leading comments are skipped first, so `-- go\nBEGIN` is caught — +and a column named `begin_at` or a value of `'rollback plan'` still runs. + +I first wrote that this broke nothing, "because the old behavior did not work." +Review pushed back, and review was right. In-memory SQLite is capped at one +connection, so under `sqlite::memory:` the hand-written pattern *did* work; I +confirmed it by building `main` and watching a rollback correctly discard its +row. So this does remove working behaviour, on precisely the configuration most +WFL tests and examples use. + +It ships as a hard error anyway, deliberately. The pattern is not a feature that +happens to be unsupported elsewhere — it is a program that passes in development +and silently loses writes the moment it meets a real database. Keeping it alive +for a deprecation window would mean keeping the corruption alive too, and the +only programs it can still "work" for are the ones most likely to be moved to a +pooled backend later. The error names the replacement, and the rewrite is +mechanical. Recorded in the changelog under Removed rather than buried in Fixed. ### `transaction` is not a keyword @@ -220,3 +233,52 @@ and the raw-SQL rejection including the case it must *not* fire on. this environment's allowance. Building the test profile with `CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0` keeps `target/` small enough to hold both without touching the committed profile. + +## What review caught + +Three automated reviewers went over the PR. Most of what they raised was real, +and two findings were more serious than anything I had found myself. + +**`hex_to_bytes` panicked on non-ASCII input.** It checked that the byte length +was even and then sliced the `str` at fixed two-byte offsets, which lands inside +a multi-byte character and panics on the boundary check. Keys and sealed values +are exactly the untrusted text this module promises to fail closed on — they +come from config files, database rows and HTTP requests — so a stored blob +containing one accented character could take down a running server. Every other +malformed input returned `None` and became a clean error; this one path escaped. +Now decoded over bytes, with tests for a non-ASCII key, a non-ASCII sealed value, +and a multi-byte character inside an otherwise well-formed key. + +**A transaction was owned by a database handle, not by the handler that opened +it.** Under `main loop concurrently:` all handlers share one `IoClient` and can +name the same global handle, so an unrelated request running an ordinary +`execute` was silently enrolled in someone else's transaction — and its write got +rolled back by that other request. Transactions are now keyed by +`(scope, handle)`, where the scope is handler-local state swapped per poll by +`InstalledRunState`, exactly like the existing count-loop and call-stack +isolation. `tokio::task_local!` would not have worked: handlers are futures in a +`FuturesUnordered` on one thread, not separate tasks. The test drives two real +requests over a socket and asserts the bystander's row survives; with the scope +key removed it fails, and both rows disappear. + +Also fixed: the transaction map's lock was held across the SQL `await`, so one +slow statement serialized every database operation in the program — transactions +now live behind their own per-handle lock, and the map's lock is held only long +enough to clone an `Arc`. `begin` had a check-then-insert race that let a second +concurrent begin replace and drop a live transaction; the handle is now reserved +under one atomic step before the round-trip. A leading SQL comment defeated the +transaction-control guard entirely (`-- go\nBEGIN` read as an empty first word). +Two of the analyzer's three AST walkers had never been taught about the new +statement, so a `random_seed` call inside a transaction block escaped the +insecure-RNG security lint. And `exit` inside a block committed its partial work, +which contradicted the rule that an abrupt stop discards it — it now rolls back. + +Two documentation claims did not survive checking either. TOML dates round-trip +as *strings*, not dates, and integers above 2⁵³ are rounded by WFL's f64 numbers; +I had written that the round trip was exact. Both are now stated plainly and +pinned with tests. + +The pattern worth remembering: the findings I would not have reached on my own +were all about what happens when two things run at once, or when input is +hostile. Those are the parts where reading the code carefully is not the same as +proving it. diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index b9caa083..eca4c946 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -2,6 +2,16 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "aead" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1973cfbc1a2daf9cf550e74e1f088c28e7f7d8c1e1418fb6c9dc5184b7e84c99" +dependencies = [ + "crypto-common 0.2.2", + "inout 0.2.2", +] + [[package]] name = "aho-corasick" version = "1.1.4" @@ -214,8 +224,23 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d524456ba66e72eb8b115ff89e01e497f8e6d11d78b70b1aa13c0fbd97540a81" dependencies = [ "cfg-if", + "cipher 0.5.2", "cpufeatures 0.3.0", "rand_core 0.10.1", + "zeroize", +] + +[[package]] +name = "chacha20poly1305" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b89e1c441e926b9c82a8d023f6e1b7ae0adcfaa7d621814e4d60789bac751cb" +dependencies = [ + "aead", + "chacha20", + "cipher 0.5.2", + "poly1305", + "zeroize", ] [[package]] @@ -247,6 +272,7 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8cf2a2c93cd704877c0858356ed03480ff301ee950b43f1cbe4573b088bfa6c" dependencies = [ + "block-buffer 0.12.1", "crypto-common 0.2.2", "inout 0.2.2", ] @@ -395,7 +421,9 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ce6e4c961d6cd6c9a86db418387425e8bdeaf05b3c8bc1411e6dca4c252f1453" dependencies = [ + "getrandom 0.4.3", "hybrid-array", + "rand_core 0.10.1", ] [[package]] @@ -1588,6 +1616,16 @@ version = "0.3.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" +[[package]] +name = "poly1305" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2d0073b297041425c7c3df6eb4792d598a15323fe63346852b092eca02904c" +dependencies = [ + "cpufeatures 0.3.0", + "universal-hash", +] + [[package]] name = "potential_utf" version = "0.1.5" @@ -2135,6 +2173,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -2721,6 +2768,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.4+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3aace63f4bbcdfc2c965b059de67119c89c4017a70d633be6c104910f67056f5" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -2874,6 +2960,16 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" +[[package]] +name = "universal-hash" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f4987bdc12753382e0bec4a65c50738ffaabc998b9cdd1f952fb5f39b0048a96" +dependencies = [ + "crypto-common 0.2.2", + "ctutils", +] + [[package]] name = "untrusted" version = "0.9.0" @@ -3102,6 +3198,7 @@ dependencies = [ "argon2", "bcrypt", "bytes", + "chacha20poly1305", "chrono", "codespan-reporting", "encoding_rs", @@ -3129,6 +3226,7 @@ dependencies = [ "subtle", "time", "tokio", + "toml", "uuid", "warp", "zeroize", @@ -3309,6 +3407,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "writeable" version = "0.6.3" diff --git a/src/analyzer/static_analyzer.rs b/src/analyzer/static_analyzer.rs index 0fe1a99f..7435ba8e 100644 --- a/src/analyzer/static_analyzer.rs +++ b/src/analyzer/static_analyzer.rs @@ -131,6 +131,13 @@ const SECURITY_SENSITIVE_BUILTINS: &[&str] = &[ "generate_csrf_token", "hmac_sha256", "wflmac256", + // Authenticated encryption. Their nonces come from the OS CSPRNG rather than + // the seedable generator, so `random_seed` does not actually weaken them — + // but the same is true of `secure_random_bytes`, which is on this list. A + // program that seeds the RNG and then encrypts secrets is signalling the + // pattern this lint exists to catch, whichever primitive it reaches for. + "seal", + "unseal", ]; /// The two ingredients of the insecure-RNG-seeding lint found in a single @@ -227,6 +234,14 @@ fn collect_calls_in_statement(stmt: &Statement, out: &mut Vec<CallSite>) { collect_calls_in_statements(else_block, out); } } + // A transaction block is an ordinary block as far as the call collector + // is concerned. Without this arm, the insecure-RNG-seeding lint cannot + // see calls inside one, and moving them into a transaction would be + // enough to evade a security check. + Statement::TransactionStatement { db, body, .. } => { + collect_calls_in_expression(db, out); + collect_calls_in_statements(body, out); + } Statement::SingleLineIf { condition, then_stmt, @@ -882,7 +897,8 @@ impl Analyzer { | Statement::ForEachLoop { body, .. } | Statement::CountLoop { body, .. } | Statement::MainLoop { body, .. } - | Statement::ForeverLoop { body, .. } => { + | Statement::ForeverLoop { body, .. } + | Statement::TransactionStatement { body, .. } => { for stmt in body { self.collect_variable_declarations(stmt, usages); } diff --git a/src/interpreter/database.rs b/src/interpreter/database.rs index a34cead8..07c16e78 100644 --- a/src/interpreter/database.rs +++ b/src/interpreter/database.rs @@ -177,9 +177,39 @@ const TRANSACTION_CONTROL_KEYWORDS: &[&str] = &[ /// construct that actually works. Only the *leading* keyword is inspected, so a /// column called `begin_at` or a string containing the word "commit" is /// untouched. +/// +/// Leading comments are skipped first (see [`skip_leading_comments`]): a guard +/// that stopped at the first non-alphabetic character would read an empty word +/// from `-- go\nBEGIN` and wave it through, which is the case it exists to stop. +/// Return `sql` with any leading whitespace and SQL comments removed, so the +/// caller sees the first real token. +/// +/// Handles both comment forms (`-- to end of line` and `/* … */`), repeated and +/// interleaved. An unterminated block comment consumes the rest of the input and +/// yields an empty string — the statement is then left to the database to +/// reject, which is the right outcome for text that is not valid SQL anyway. +fn skip_leading_comments(sql: &str) -> &str { + let mut rest = sql.trim_start(); + loop { + if let Some(after) = rest.strip_prefix("--") { + // A line comment runs to the newline, or to the end of the input. + rest = match after.find('\n') { + Some(newline) => after[newline + 1..].trim_start(), + None => return "", + }; + } else if let Some(after) = rest.strip_prefix("/*") { + rest = match after.find("*/") { + Some(end) => after[end + 2..].trim_start(), + None => return "", + }; + } else { + return rest; + } + } +} + pub fn reject_transaction_control_sql(sql: &str) -> Result<(), String> { - let first_word: String = sql - .trim_start() + let first_word: String = skip_leading_comments(sql) .chars() .take_while(|c| c.is_ascii_alphabetic()) .collect(); diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 11e2ac77..a888bb71 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -839,6 +839,8 @@ impl Drop for CallDepthGuard<'_> { /// that handler is actively being polled (see [`IsolatedHandler`]). #[derive(Default)] struct RunState { + /// This handler's transaction scope (see [`Interpreter::tx_scope`]). + tx_scope: u64, current_count: Option<f64>, in_count_loop: bool, call_depth: usize, @@ -1455,6 +1457,20 @@ pub struct Interpreter { /// dedicated RAII counter means a caught `ResourceLimit` can never leave the /// enforcement depth under-counted, so catch-and-recurse stays bounded. call_depth: Cell<usize>, + /// Identifies the execution context ("who") that owns a transaction. + /// + /// Under `main loop concurrently:` every handler runs against the *same* + /// `IoClient` and can name the same global database handle. Keying open + /// transactions by handle alone would mean one handler's ordinary query + /// silently joined another handler's transaction — and got committed or + /// rolled back by it. Transactions are therefore keyed by (scope, handle). + /// + /// Swapped per poll by [`InstalledRunState`], exactly like the other + /// handler-local run state, so the value read here always belongs to the + /// handler currently executing. `0` is the serial/top-level scope. + tx_scope: Cell<u64>, + /// Source of unique [`Self::tx_scope`] ids for concurrent handlers. + next_tx_scope: Cell<u64>, /// Static methods execute against lexical environments that mirror shared /// container properties. This stack synchronizes those mirrors at nested /// call boundaries so a re-entrant static call sees mutations made by its @@ -1852,6 +1868,11 @@ async fn terminate_foreground_child(child: &mut tokio::process::Child) -> Result } #[allow(dead_code)] +/// An open transaction behind its own lock, so the transaction map's lock is +/// never held across SQL I/O. `None` marks a handle reserved by an in-flight +/// `begin` (see [`IoClient::db_transactions`]). +type SharedTransaction = Arc<Mutex<Option<database::DbTransaction>>>; + pub struct IoClient { /// Shared outbound HTTP client, built on first use. /// @@ -1872,7 +1893,24 @@ pub struct IoClient { /// `db_handles`. While an entry is present, every `query`/`execute` on that /// handle runs on the transaction's pinned connection instead of taking a /// fresh one from the pool — which is the whole point of issue #664. - db_transactions: Mutex<HashMap<String, database::DbTransaction>>, + /// + /// Each transaction lives behind its **own** lock rather than inside the + /// map's. Statements within one transaction are necessarily serial (a + /// transaction owns a single connection), but holding the map's lock across + /// the SQL await would extend that to every database operation in the + /// program, so one slow statement would stall unrelated handles. Callers + /// therefore lock the map only long enough to clone the `Arc`. + /// + /// Keyed by `(scope, handle)` rather than handle alone: see + /// [`Interpreter::tx_scope`]. Two concurrent handlers naming the same global + /// database handle therefore get separate entries, and a handler with no + /// transaction of its own takes the pool as it always did. + /// + /// The slot is `Option` so a handle can be *reserved* under the map lock + /// before the `begin` round-trip completes: two concurrent begins on one + /// handle would otherwise both pass a `contains_key` check and the second + /// would silently replace — and drop — the first transaction. + db_transactions: Mutex<HashMap<(u64, String), SharedTransaction>>, /// Live outbound streaming response bodies, keyed by handle id /// ("httpstream1", ...). See [`StreamSlot`] / [`HttpStreamHandle`]. /// @@ -2474,8 +2512,13 @@ impl IoClient { /// Refuses while a transaction is open on the handle: closing the pool /// under an in-flight transaction would discard the work with no report of /// what happened to it. - async fn close_database(&self, handle_id: &str) -> Result<(), String> { - if self.db_transactions.lock().await.contains_key(handle_id) { + async fn close_database(&self, scope: u64, handle_id: &str) -> Result<(), String> { + if self + .db_transactions + .lock() + .await + .contains_key(&(scope, handle_id.to_string())) + { return Err(format!( "Cannot close database '{handle_id}' while a transaction is open on it. \ Let the `in transaction on ...` block finish first — it commits at \ @@ -2494,33 +2537,68 @@ impl IoClient { } /// Open a transaction on `handle_id`, pinning one pooled connection to it. - async fn begin_transaction(&self, handle_id: &str) -> Result<(), String> { + async fn begin_transaction(&self, scope: u64, handle_id: &str) -> Result<(), String> { // Nesting would need savepoints, which are not implemented; say so // rather than quietly flattening the inner block into the outer one. - if self.db_transactions.lock().await.contains_key(handle_id) { - return Err(format!( - "A transaction is already open on database '{handle_id}'. Transaction \ - blocks cannot be nested on the same database — finish the current one \ - before starting another." - )); + // Reserve the handle and take the slot's lock *before* awaiting the + // database, so a concurrent begin on the same handle loses the race + // here rather than replacing a live transaction later. + let slot: SharedTransaction = { + let mut transactions = self.db_transactions.lock().await; + if transactions.contains_key(&(scope, handle_id.to_string())) { + return Err(format!( + "A transaction is already open on database '{handle_id}'. Transaction \ + blocks cannot be nested on the same database — finish the current one \ + before starting another." + )); + } + let slot: SharedTransaction = Arc::new(Mutex::new(None)); + transactions.insert((scope, handle_id.to_string()), Arc::clone(&slot)); + slot + }; + // Held across the begin, so a statement that arrives meanwhile waits for + // the transaction rather than seeing an empty slot and taking the pool. + let mut open = slot.lock().await; + + // The reservation must not outlive a failed begin, or the handle would + // be stuck refusing every later transaction. + let pool = match self.get_database(handle_id).await { + Ok(pool) => pool, + Err(err) => { + self.db_transactions + .lock() + .await + .remove(&(scope, handle_id.to_string())); + return Err(err); + } + }; + match database::begin(&pool).await { + Ok(tx) => { + *open = Some(tx); + Ok(()) + } + Err(err) => { + self.db_transactions + .lock() + .await + .remove(&(scope, handle_id.to_string())); + Err(err) + } } - - let pool = self.get_database(handle_id).await?; - let tx = database::begin(&pool).await?; - self.db_transactions - .lock() - .await - .insert(handle_id.to_string(), tx); - Ok(()) } /// Commit the open transaction on `handle_id`. - async fn commit_transaction(&self, handle_id: &str) -> Result<(), String> { - let tx = self + async fn commit_transaction(&self, scope: u64, handle_id: &str) -> Result<(), String> { + let slot = self .db_transactions .lock() .await - .remove(handle_id) + .remove(&(scope, handle_id.to_string())) + .ok_or_else(|| format!("No transaction is open on database '{handle_id}'"))?; + let tx = slot + .lock() + .await + .take() .ok_or_else(|| format!("No transaction is open on database '{handle_id}'"))?; database::commit(tx).await } @@ -2529,8 +2607,16 @@ impl IoClient { /// /// A missing transaction is not an error here: this runs on the failure /// path, where the transaction may already have been resolved. - async fn rollback_transaction(&self, handle_id: &str) -> Result<(), String> { - let tx = self.db_transactions.lock().await.remove(handle_id); + async fn rollback_transaction(&self, scope: u64, handle_id: &str) -> Result<(), String> { + let slot = self + .db_transactions + .lock() + .await + .remove(&(scope, handle_id.to_string())); + let tx = match slot { + Some(slot) => slot.lock().await.take(), + None => None, + }; match tx { Some(tx) => database::rollback(tx).await, None => Ok(()), @@ -2541,13 +2627,22 @@ impl IoClient { /// when it has one. async fn db_query( &self, + scope: u64, handle_id: &str, sql: &str, params: &[database::SqlParam], ) -> Result<Value, String> { - { - let mut transactions = self.db_transactions.lock().await; - if let Some(tx) = transactions.get_mut(handle_id) { + // Lock the map only long enough to clone the handle's slot; the SQL + // below is awaited under the slot's own lock, never the map's. + let slot = self + .db_transactions + .lock() + .await + .get(&(scope, handle_id.to_string())) + .cloned(); + if let Some(slot) = slot { + let mut open = slot.lock().await; + if let Some(tx) = open.as_mut() { return database::run_query(database::DbTarget::Transaction(tx), sql, params).await; } } @@ -2559,13 +2654,20 @@ impl IoClient { /// when it has one. async fn db_execute( &self, + scope: u64, handle_id: &str, sql: &str, params: &[database::SqlParam], ) -> Result<Value, String> { - { - let mut transactions = self.db_transactions.lock().await; - if let Some(tx) = transactions.get_mut(handle_id) { + let slot = self + .db_transactions + .lock() + .await + .get(&(scope, handle_id.to_string())) + .cloned(); + if let Some(slot) = slot { + let mut open = slot.lock().await; + if let Some(tx) = open.as_mut() { return database::run_execute(database::DbTarget::Transaction(tx), sql, params) .await; } @@ -4560,6 +4662,8 @@ impl Interpreter { current_block_overload_dups: RefCell::new(None), call_stack: RefCell::new(Vec::new()), call_depth: Cell::new(0), + tx_scope: Cell::new(0), + next_tx_scope: Cell::new(1), active_static_method_contexts: RefCell::new(Vec::new()), base_call_depth: 0, io_client: Rc::new(IoClient::new(Arc::clone(&config))), @@ -5062,6 +5166,8 @@ impl Interpreter { ); let depth = self.call_depth.replace(state.call_depth); state.call_depth = depth; + let scope = self.tx_scope.replace(state.tx_scope); + state.tx_scope = scope; std::mem::swap(&mut *self.call_stack.borrow_mut(), &mut state.call_stack); std::mem::swap( &mut *self.active_static_method_contexts.borrow_mut(), @@ -5104,7 +5210,12 @@ impl Interpreter { /// context so captures, relative module paths, and cycle/import-depth /// checks behave as they would in the enclosing context (#642). fn fresh_handler_run_state(&self) -> RunState { + let scope = self.next_tx_scope.get(); + self.next_tx_scope.set(scope.wrapping_add(1).max(1)); RunState { + // A fresh scope per handler: its transactions are its own, and an + // unrelated handler naming the same database handle takes the pool. + tx_scope: scope, call_depth: self.call_depth.get(), capture_stack: io_capture::snapshot_stack(), current_source_file: self.current_source_file.borrow().clone(), @@ -6321,6 +6432,83 @@ impl Interpreter { } } + /// Run an `in transaction on <db>: ... end transaction` block. + /// + /// Split out of [`Self::execute_statement`] and awaited behind a `Box::pin` + /// so its locals do not inflate that function's state machine for every + /// other statement kind — see the call site. + async fn execute_transaction_statement( + &self, + db: &Expression, + body: &[Statement], + line: usize, + column: usize, + env: Rc<RefCell<Environment>>, + ) -> Result<(Value, ControlFlow), RuntimeError> { + let db_value = self.evaluate_expression(db, Rc::clone(&env)).await?; + let handle = match &db_value { + Value::Text(s) => s.clone(), + _ => { + return Err(RuntimeError::new( + format!("Expected a database handle, got {db_value:?}"), + line, + column, + )); + } + }; + + self.io_client + .begin_transaction(self.tx_scope.get(), &handle) + .await + .map_err(|e| RuntimeError::new(e, line, column))?; + + // Statements inside share the enclosing scope, like `try:`, so a + // variable set inside the block is still readable after it. + let outcome = self.execute_block(body, Rc::clone(&env)).await; + + match outcome { + // `exit` stops the program where it stands rather than + // finishing the block, so its work is abandoned — matching + // what already happens when a program ends with a + // transaction still open. Committing a half-finished block + // on an abrupt stop is the surprise worth avoiding. + Ok((value, ControlFlow::Exit)) => { + self.io_client + .rollback_transaction(self.tx_scope.get(), &handle) + .await + .map_err(|e| RuntimeError::new(e, line, column))?; + Ok((value, ControlFlow::Exit)) + } + Ok((value, control_flow)) => { + // The block finished without an error. `break`, `continue` + // and `return` are ordinary exits, not failures, so they + // commit too — the work inside completed. + self.io_client + .commit_transaction(self.tx_scope.get(), &handle) + .await + .map_err(|e| RuntimeError::new(e, line, column))?; + Ok((value, control_flow)) + } + Err(err) => { + // Roll back and report the original failure. A rollback + // that itself fails is appended rather than replacing the + // cause, which is what the user actually needs to see. + match self + .io_client + .rollback_transaction(self.tx_scope.get(), &handle) + .await + { + Ok(()) => Err(err), + Err(rollback_error) => Err(RuntimeError::new( + format!("{} (rollback also failed: {rollback_error})", err.message), + err.line, + err.column, + )), + } + } + } + } + async fn execute_statement( &self, stmt: &Statement, @@ -7237,7 +7425,10 @@ impl Interpreter { Err(msg) => { // Don't leave an unreachable pool behind when // the variable binding fails. - let _ = self.io_client.close_database(&handle).await; + let _ = self + .io_client + .close_database(self.tx_scope.get(), &handle) + .await; Err(RuntimeError::new(msg, *line, *column)) } } @@ -7285,7 +7476,7 @@ impl Interpreter { }; self.io_client - .close_database(&handle) + .close_database(self.tx_scope.get(), &handle) .await .map_err(|e| RuntimeError::new(e, *line, *column))?; @@ -7297,52 +7488,13 @@ impl Interpreter { line, column, } => { - let db_value = self.evaluate_expression(db, Rc::clone(&env)).await?; - let handle = match &db_value { - Value::Text(s) => s.clone(), - _ => { - return Err(RuntimeError::new( - format!("Expected a database handle, got {db_value:?}"), - *line, - *column, - )); - } - }; - - self.io_client - .begin_transaction(&handle) - .await - .map_err(|e| RuntimeError::new(e, *line, *column))?; - - // Statements inside share the enclosing scope, like `try:`, so a - // variable set inside the block is still readable after it. - let outcome = self.execute_block(body, Rc::clone(&env)).await; - - match outcome { - Ok((value, control_flow)) => { - // The block finished without an error. `break`, `continue` - // and `return` are ordinary exits, not failures, so they - // commit too — the work inside completed. - self.io_client - .commit_transaction(&handle) - .await - .map_err(|e| RuntimeError::new(e, *line, *column))?; - Ok((value, control_flow)) - } - Err(err) => { - // Roll back and report the original failure. A rollback - // that itself fails is appended rather than replacing the - // cause, which is what the user actually needs to see. - match self.io_client.rollback_transaction(&handle).await { - Ok(()) => Err(err), - Err(rollback_error) => Err(RuntimeError::new( - format!("{} (rollback also failed: {rollback_error})", err.message), - err.line, - err.column, - )), - } - } - } + // Boxed: `execute_statement` is a plain `async fn`, so every + // `.await` in every arm enlarges the single state machine that + // each level of statement recursion keeps on the stack. Holding + // this arm's work behind a pointer keeps deeply nested programs + // (and concurrent handlers, which recurse per request) clear of + // the stack ceiling. + Box::pin(self.execute_transaction_statement(db, body, *line, *column, env)).await } Statement::ReadFileStatement { path, @@ -13352,10 +13504,14 @@ impl Interpreter { // gets its own pinned connection rather than an arbitrary pooled one. match kind { crate::parser::ast::DatabaseQueryKind::Query => { - self.io_client.db_query(&handle, &sql_str, &params).await + self.io_client + .db_query(self.tx_scope.get(), &handle, &sql_str, &params) + .await } crate::parser::ast::DatabaseQueryKind::Execute => { - self.io_client.db_execute(&handle, &sql_str, &params).await + self.io_client + .db_execute(self.tx_scope.get(), &handle, &sql_str, &params) + .await } } .map_err(|e| RuntimeError::new(e, line, column)) diff --git a/src/stdlib/crypto.rs b/src/stdlib/crypto.rs index 3b6f1bc9..0dc4dd00 100644 --- a/src/stdlib/crypto.rs +++ b/src/stdlib/crypto.rs @@ -1034,13 +1034,24 @@ const SEAL_TAG_LEN: usize = 16; const SEAL_KEY_LEN: usize = 32; /// Decode a hex string into bytes, or `None` if it is not valid hex. +/// +/// Works over bytes rather than slicing the `str`. Keys and sealed values are +/// untrusted text — they arrive from config files, database rows and HTTP +/// requests — and slicing at fixed two-byte offsets lands inside a multi-byte +/// character and panics. Every malformed input has to fail closed here, like +/// the rest of this module, rather than take the process down. fn hex_to_bytes(hex: &str) -> Option<Vec<u8>> { - if !hex.len().is_multiple_of(2) { + let bytes = hex.as_bytes(); + if !bytes.len().is_multiple_of(2) { return None; } - (0..hex.len()) - .step_by(2) - .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).ok()) + bytes + .chunks_exact(2) + .map(|pair| { + // Rejects any non-ASCII byte: hex is ASCII by definition. + let text = std::str::from_utf8(pair).ok()?; + u8::from_str_radix(text, 16).ok() + }) .collect() } diff --git a/tests/crypto_seal_test.rs b/tests/crypto_seal_test.rs index 70d8d50c..5f5ad424 100644 --- a/tests/crypto_seal_test.rs +++ b/tests/crypto_seal_test.rs @@ -361,3 +361,55 @@ store plain as unseal of sealed and key .err() .expect("dropping the context on unseal must fail, not silently succeed"); } + +/// A key or sealed value is untrusted text — it can come from a config file, a +/// database row, or an HTTP request. Every malformed input must produce a +/// catchable error, never a panic that takes down the process (and any server +/// it is hosting) with it. +/// +/// Multi-byte UTF-8 is the case that escaped: decoding hex by slicing the string +/// at fixed two-byte offsets lands mid-character and panics on a `str` boundary +/// check rather than failing closed like every other bad input. +#[tokio::test] +async fn a_non_ascii_key_is_refused_rather_than_panicking() { + // "€a" is 4 bytes, so an even-length check passes, but offset 2 is inside + // the 3-byte '€'. + let code = r#" +store sealed as seal of "secret" and "€a" +"#; + run_wfl(code) + .await + .err() + .expect("a non-ASCII key must be reported as a bad key, not panic"); +} + +#[tokio::test] +async fn a_non_ascii_sealed_value_is_refused_rather_than_panicking() { + let code = format!( + r#" +store key as "{KEY}" +store plain as unseal of "wflseal1:€a" and key +"# + ); + run_wfl(&code) + .await + .err() + .expect("a non-ASCII sealed value must be reported, not panic"); +} + +/// Hex is ASCII by definition, so a full-width or accented character anywhere in +/// an otherwise well-formed value must be rejected too. +#[tokio::test] +async fn a_multibyte_character_inside_a_full_length_key_is_refused() { + // 62 ASCII hex characters plus a 2-byte 'é' — 64 bytes, 63 characters. + let key: String = "a".repeat(62) + "é"; + let code = format!( + r#" +store sealed as seal of "secret" and "{key}" +"# + ); + run_wfl(&code) + .await + .err() + .expect("a multi-byte character inside a key must be reported, not panic"); +} diff --git a/tests/database_transaction_test.rs b/tests/database_transaction_test.rs index 62eae34d..be9153ac 100644 --- a/tests/database_transaction_test.rs +++ b/tests/database_transaction_test.rs @@ -476,3 +476,211 @@ close database db assert_eq!(rows.len(), 1); assert_eq!(expect_number(&expect_object_key(&rows[0], "x")), 5.0); } + +/// The transaction-control guard exists because these statements would silently +/// run on arbitrary pooled connections. A guard that only inspects the first +/// alphabetic run is defeated by a leading SQL comment: `first_word` comes back +/// empty and the statement sails through onto the pool — exactly the path the +/// guard is there to close. +#[tokio::test] +async fn a_line_comment_before_begin_does_not_bypass_the_guard() { + let db = TempDb::new("comment_begin"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store t as execute db with "-- start the transaction +BEGIN" +"# + ); + run_wfl(&code) + .await + .err() + .expect("BEGIN behind a line comment must still be rejected"); +} + +#[tokio::test] +async fn a_block_comment_before_begin_does_not_bypass_the_guard() { + let db = TempDb::new("block_begin"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store t as execute db with "/* transaction */ BEGIN" +"# + ); + run_wfl(&code) + .await + .err() + .expect("BEGIN behind a block comment must still be rejected"); +} + +#[tokio::test] +async fn several_comments_before_rollback_do_not_bypass_the_guard() { + let db = TempDb::new("multi_rollback"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store t as execute db with "/* one */ -- two + /* three */ ROLLBACK" +"# + ); + run_wfl(&code) + .await + .err() + .expect("ROLLBACK behind several comments must still be rejected"); +} + +/// The guard must stay narrow: skipping comments must not start rejecting +/// ordinary statements that merely carry a comment, or that mention the words. +#[tokio::test] +async fn a_commented_ordinary_statement_still_runs() { + let db = TempDb::new("comment_ok"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE audit (begin_at TEXT, commit_note TEXT)" +store ins as execute db with "-- record the attempt +/* not a transaction */ INSERT INTO audit (begin_at, commit_note) VALUES ('t0', 'rollback plan')" +store rows as query db with "SELECT begin_at FROM audit" +store seen as length of rows +"# + ); + let interpreter = run_wfl(&code) + .await + .expect("an ordinary statement behind a comment must still run"); + assert_eq!(expect_number(&get_global(&interpreter, "seen")), 1.0); +} + +/// An unterminated block comment must not hang or panic the guard. +#[tokio::test] +async fn an_unterminated_comment_is_handled() { + let db = TempDb::new("unterminated"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store t as execute db with "/* never closed" +"# + ); + // Whatever the database makes of it, the guard itself must terminate and + // hand the statement on rather than looping or panicking. + let _ = run_wfl(&code).await; +} + +// --------------------------------------------------------------------------- +// Concurrency (testing.md §11.3): a transaction pins one connection, so +// statements *inside* it are necessarily serial. What must NOT happen is that +// holding a transaction open serializes database work that has nothing to do +// with it. +// --------------------------------------------------------------------------- + +/// A slow statement inside a transaction must not block an unrelated handle. +/// +/// The failure this guards against is holding the global transaction map's lock +/// across the SQL await: every other `query`/`execute`/`begin`/`commit` in the +/// program takes that same lock, so one slow transactional statement stalls all +/// database work, including on databases it has nothing to do with. +#[tokio::test] +async fn a_slow_statement_in_a_transaction_does_not_block_an_unrelated_handle() { + let slow_db = TempDb::new("slow_tx"); + let other_db = TempDb::new("other_handle"); + let slow_url = &slow_db.url; + let other_url = &other_db.url; + + // `main loop concurrently` runs both bodies on one thread, interleaved at + // await points. If the unrelated query cannot make progress until the + // transaction finishes, this deadlocks and the test times out. + let code = format!( + r#" +open database at "{slow_url}" as slow_db +open database at "{other_url}" as other_db +store made as execute slow_db with "CREATE TABLE t (x INTEGER)" +store made2 as execute other_db with "CREATE TABLE u (y INTEGER)" + +in transaction on slow_db: + store ins as execute slow_db with "INSERT INTO t (x) VALUES (1)" + store unrelated as query other_db with "SELECT COUNT(*) AS n FROM u" + store unrelated_rows as length of unrelated +end transaction + +store rows as query slow_db with "SELECT x FROM t" +store committed as length of rows +"# + ); + + let interpreter = tokio::time::timeout(std::time::Duration::from_secs(20), run_wfl(&code)) + .await + .expect("a query on an unrelated handle must not wait for the transaction") + .expect("program should run"); + + assert_eq!( + expect_number(&get_global(&interpreter, "unrelated_rows")), + 1.0, + "the unrelated handle's query must return its row" + ); + assert_eq!( + expect_number(&get_global(&interpreter, "committed")), + 1.0, + "the transaction must still commit its own write" + ); +} + +/// `exit` stops the program where it stands. It is not a way of *finishing* a +/// block, so a transaction it interrupts must be abandoned, not committed. +/// +/// This also keeps the block consistent with what already happens when a +/// program simply ends with a transaction still open (see +/// `open_transaction_rolls_back_when_program_ends`): both are abrupt stops, and +/// both must discard the partial work rather than half-commit it. +#[tokio::test] +async fn exit_inside_a_transaction_rolls_back() { + let db = TempDb::new("exit_rollback"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('abandoned')" + exit +end transaction +"# + ); + run_wfl(&code).await.expect("exit ends the program cleanly"); + + assert_eq!( + surviving_rows(url).await, + 0.0, + "work interrupted by `exit` must be rolled back, not committed" + ); +} + +/// `break`, `continue` and `return` stay committing exits: they mean the block +/// finished, and the documented rule is that only failures roll back. +#[tokio::test] +async fn break_inside_a_transaction_still_commits() { + let db = TempDb::new("break_commits"); + let url = &db.url; + let code = format!( + r#" +open database at "{url}" as db +store made as execute db with "CREATE TABLE projects (slug TEXT)" +count from 1 to 3: + in transaction on db: + store ins as execute db with "INSERT INTO projects (slug) VALUES ('kept')" + break + end transaction +end count +"# + ); + run_wfl(&code).await.expect("program should run"); + + assert_eq!( + surviving_rows(url).await, + 1.0, + "a `break` is an ordinary exit from the block, so its work commits" + ); +} diff --git a/tests/toml_test.rs b/tests/toml_test.rs index 91569841..d87e0990 100644 --- a/tests/toml_test.rs +++ b/tests/toml_test.rs @@ -269,3 +269,67 @@ store out as stringify_toml of config ); assert_eq!(expect_text(&get_global(&interpreter, "out")), ""); } + +/// TOML dates parse to text, and therefore write back as strings rather than +/// dates. That is a real fidelity loss, so it is pinned here and stated in +/// Docs/05-standard-library/toml-module.md rather than left for a user to +/// discover from a config a downstream tool then rejects. +#[tokio::test] +async fn a_date_round_trips_as_text_not_as_a_toml_date() { + let interpreter = run_wfl( + r#" +store parsed as parse_toml of "released = 2026-07-31 +" +store rendered as stringify_toml of parsed +"#, + ) + .await + .expect("valid TOML"); + + assert_eq!( + expect_text(&get_global(&interpreter, "rendered")).trim(), + r#"released = "2026-07-31""#, + "the characters survive, but the TOML type becomes a string" + ); +} + +/// WFL numbers are f64, so TOML integers beyond 2^53 are rounded on the way in. +/// Documented in the TOML page; asserted here so the boundary is not moved +/// silently. +#[tokio::test] +async fn integers_within_exact_f64_range_survive_a_round_trip() { + let interpreter = run_wfl( + r#" +store parsed as parse_toml of "id = 9007199254740991 +" +store rendered as stringify_toml of parsed +"#, + ) + .await + .expect("valid TOML"); + + assert_eq!( + expect_text(&get_global(&interpreter, "rendered")).trim(), + "id = 9007199254740991", + "2^53 - 1 is exactly representable and must survive" + ); +} + +#[tokio::test] +async fn integers_beyond_exact_f64_range_are_rounded() { + let interpreter = run_wfl( + r#" +store parsed as parse_toml of "id = 9007199254740993 +" +store rendered as stringify_toml of parsed +"#, + ) + .await + .expect("valid TOML"); + + assert_eq!( + expect_text(&get_global(&interpreter, "rendered")).trim(), + "id = 9007199254740992", + "beyond 2^53 the value is rounded — keep such ids as quoted strings" + ); +} diff --git a/tests/transaction_analyzer_walk_test.rs b/tests/transaction_analyzer_walk_test.rs new file mode 100644 index 00000000..d18a0177 --- /dev/null +++ b/tests/transaction_analyzer_walk_test.rs @@ -0,0 +1,126 @@ +//! The analyzer has three AST walkers that must all know about every +//! block-bearing statement. A statement that only one of them understands is a +//! silent hole: code inside the block becomes invisible to the other two. +//! +//! For `in transaction on db: ... end transaction` the stakes are not cosmetic. +//! `check_insecure_rng_seeding` is a security lint that blocks a program which +//! seeds the general-purpose RNG and then performs a cryptographic operation; +//! if its walker cannot see into the block, moving the calls inside one is +//! enough to evade it. + +use wfl::analyzer::Analyzer; +use wfl::analyzer::static_analyzer::{StaticAnalyzer, rng_security_ingredients}; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; + +fn parse(source: &str) -> wfl::parser::ast::Program { + let tokens = lex_wfl_with_positions(source); + Parser::new(&tokens) + .parse() + .expect("test program must parse") +} + +/// The lint fires on this program when the calls sit at the top level. +#[test] +fn insecure_rng_seeding_is_detected_at_the_top_level() { + let program = parse( + r#" +store seeded as random_seed of 42 +store token as hash_password of "hunter2" +"#, + ); + let found = rng_security_ingredients(&program); + assert!(found.seed_site.is_some(), "random_seed must be seen"); + assert!(found.security_site.is_some(), "hash_password must be seen"); +} + +/// ...and must still fire when they are moved inside a transaction block. +#[test] +fn insecure_rng_seeding_is_detected_inside_a_transaction_block() { + let program = parse( + r#" +open database at "sqlite::memory:" as db +in transaction on db: + store seeded as random_seed of 42 + store token as hash_password of "hunter2" +end transaction +"#, + ); + let found = rng_security_ingredients(&program); + assert!( + found.seed_site.is_some(), + "a random_seed call inside a transaction block must not escape the security lint" + ); + assert!( + found.security_site.is_some(), + "a crypto call inside a transaction block must not escape the security lint" + ); +} + +/// The database expression in the block header is walked too. +#[test] +fn calls_in_the_transaction_header_are_collected() { + let program = parse( + r#" +open database at "sqlite::memory:" as db +in transaction on db: + store seeded as random_seed of 42 +end transaction +store token as hash_password of "hunter2" +"#, + ); + let found = rng_security_ingredients(&program); + assert!(found.seed_site.is_some()); + assert!(found.security_site.is_some()); +} + +/// The unused-variable walk must see declarations inside the block. Its failure +/// mode is a false negative — a variable the block declares and nobody uses is +/// simply never tracked, so it is never reported. +#[test] +fn an_unused_variable_declared_inside_a_transaction_block_is_reported() { + let program = parse( + r#" +open database at "sqlite::memory:" as db +in transaction on db: + store forgotten_receipt as 7 +end transaction +"#, + ); + let analyzer = Analyzer::new(); + let diagnostics = analyzer.check_unused_variables(&program, 0); + + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("forgotten_receipt")), + "a variable declared inside a transaction block and never used must still be \ + reported as unused; got: {diagnostics:?}" + ); +} + +/// ...and one that *is* used afterwards must not be reported, so fixing the +/// false negative does not introduce a false positive. +#[test] +fn a_used_variable_declared_inside_a_transaction_block_is_not_reported() { + let program = parse( + r#" +open database at "sqlite::memory:" as db +in transaction on db: + store receipt_id as 7 +end transaction +display receipt_id +"#, + ); + let analyzer = Analyzer::new(); + let diagnostics = analyzer.check_unused_variables(&program, 0); + + let unused: Vec<_> = diagnostics + .iter() + .filter(|d| d.message.contains("receipt_id")) + .collect(); + assert!( + unused.is_empty(), + "receipt_id is used after the block, so it must not be reported: {unused:?}" + ); +} diff --git a/tests/transaction_handler_scope_test.rs b/tests/transaction_handler_scope_test.rs new file mode 100644 index 00000000..a34a003c --- /dev/null +++ b/tests/transaction_handler_scope_test.rs @@ -0,0 +1,230 @@ +//! A transaction belongs to the handler that opened it (testing.md §11.3). +//! +//! Under `main loop concurrently:` every handler shares one `IoClient` and can +//! name the same global database handle. If open transactions were keyed by +//! handle alone, a handler that merely runs an ordinary `execute` on that handle +//! would be silently enrolled in a *different* handler's transaction — and its +//! write would then be committed or rolled back by that other request. That is a +//! cross-request data-integrity bug, and it is what this file pins shut. +//! +//! Driven over a real socket against a real file-backed SQLite database: the +//! interleaving only exists when two handler futures are genuinely in flight at +//! once, so a sequential test cannot observe it. + +use std::path::{Path, PathBuf}; +use std::time::Duration; + +mod common; + +/// A temp-file SQLite database that removes itself on drop. File-backed because +/// in-memory SQLite is capped at a single connection and so cannot exhibit the +/// pooled-handle behaviour under test. +struct TempDb { + url: String, + path: PathBuf, +} + +impl TempDb { + fn new(test_name: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "wfl_tx_scope_{}_{}.db", + test_name, + std::process::id() + )); + let _ = std::fs::remove_file(&path); + let url = format!("sqlite://{}", path.display()).replace('\\', "/"); + Self { url, path } + } +} + +impl Drop for TempDb { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.path); + for suffix in ["-wal", "-shm"] { + let mut sidecar = self.path.clone().into_os_string(); + sidecar.push(suffix); + let _ = std::fs::remove_file(Path::new(&sidecar)); + } + } +} + +fn start_server_thread(code: String) -> std::thread::JoinHandle<()> { + // An explicit stack: the interpreter executes nested blocks by recursion, and + // this server program is several blocks deep (concurrent main loop → check → + // check → try → transaction). A spawned thread's default stack is 2 MB on + // Linux and 1 MB on Windows, which this exceeds. The subject of the test is + // transaction ownership, not stack depth, so give it room rather than + // flattening the program into something that no longer reproduces the race. + std::thread::Builder::new() + .stack_size(16 * 1024 * 1024) + .spawn(move || { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build runtime"); + runtime.block_on(async move { + let tokens = wfl::lexer::lex_wfl_with_positions(&code); + let mut parser = wfl::parser::Parser::new(&tokens); + let ast = parser.parse().expect("parse"); + let mut interpreter = wfl::Interpreter::new(); + if let Err(errors) = interpreter.interpret(&ast).await { + panic!("server interpreter failed: {errors:?}"); + } + }); + }) + .expect("spawn server thread") +} + +async fn wait_for_server(port: u16) { + let addr = format!("127.0.0.1:{port}"); + for _ in 0..300 { + if tokio::net::TcpStream::connect(&addr).await.is_ok() { + return; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + panic!("server on {addr} did not become ready in time"); +} + +async fn shutdown(port: u16, server: std::thread::JoinHandle<()>) { + let _ = reqwest::Client::new() + .get(format!("http://127.0.0.1:{port}/shutdown")) + .send() + .await; + match tokio::task::spawn_blocking(move || server.join()).await { + Ok(Ok(())) => {} + Ok(Err(panic)) => std::panic::resume_unwind(panic), + Err(join_err) => panic!("server join task failed: {join_err}"), + } +} + +/// `/tx` opens a transaction on the shared handle, waits (so the other request +/// is guaranteed to be interleaved inside it), writes a row, and then fails — +/// rolling its own work back. `/plain` writes a row through the very same +/// handle while that transaction is open. +/// +/// The `/plain` row must survive: it was never part of the transaction. +#[tokio::test] +async fn an_unrelated_handler_is_not_enrolled_in_another_handlers_transaction() { + let db = TempDb::new("not_enrolled"); + let url = &db.url; + let port = common::free_tcp_port(); + + let code = format!( + r#" + open database at "{url}" as db + store made as execute db with "CREATE TABLE writes (tag TEXT)" + listen on port {port} as srv + main loop concurrently: + wait for request comes in on srv as req with timeout 20000 + store p as req["path"] + check if p is equal to "/shutdown": + respond to req with "bye" + close server srv + break + otherwise: + check if p is equal to "/tx": + try: + in transaction on db: + store a as execute db with "INSERT INTO writes (tag) VALUES ('rolled-back')" + wait for 600 milliseconds + store boom as execute db with "INSERT INTO no_such_table (x) VALUES (1)" + end transaction + when error: + respond to req with "tx-rolled-back" + end try + otherwise: + store b as execute db with "INSERT INTO writes (tag) VALUES ('kept')" + respond to req with "plain-written" + end check + end check + end loop + "# + ); + + let server = start_server_thread(code); + wait_for_server(port).await; + + // Start the transactional request and let it get inside its block. + let tx_url = format!("http://127.0.0.1:{port}/tx"); + let tx = tokio::spawn(async move { + reqwest::Client::new() + .get(&tx_url) + .send() + .await + .expect("tx request failed") + .text() + .await + .unwrap() + }); + tokio::time::sleep(Duration::from_millis(200)).await; + + // ...then write through the same handle from a different handler. + let plain = reqwest::Client::new() + .get(format!("http://127.0.0.1:{port}/plain")) + .send() + .await + .expect("plain request failed") + .text() + .await + .unwrap(); + assert_eq!(plain, "plain-written"); + + assert_eq!(tx.await.expect("tx task panicked"), "tx-rolled-back"); + + shutdown(port, server).await; + + // The transaction's own row is gone; the unrelated handler's row remains. + let rows = surviving_tags(url).await; + assert!( + rows.contains(&"kept".to_string()), + "an unrelated handler's write was rolled back by another handler's \ + transaction — transactions must be scoped to the handler that opened \ + them; rows present: {rows:?}" + ); + assert!( + !rows.contains(&"rolled-back".to_string()), + "the transaction's own write must still have been rolled back; rows present: {rows:?}" + ); +} + +/// Read the table back with a fresh interpreter, after the server is gone. +async fn surviving_tags(url: &str) -> Vec<String> { + let code = format!( + r#" +open database at "{url}" as db +store rows as query db with "SELECT tag FROM writes ORDER BY tag" +"# + ); + let tokens = wfl::lexer::lex_wfl_with_positions(&code); + let mut parser = wfl::parser::Parser::new(&tokens); + let ast = parser.parse().expect("parse"); + let mut interpreter = wfl::Interpreter::new(); + interpreter + .interpret(&ast) + .await + .expect("read-back program"); + + let rows = interpreter + .global_env() + .borrow() + .get("rows") + .expect("rows must exist"); + + match rows { + wfl::interpreter::value::Value::List(items) => items + .borrow() + .iter() + .map(|row| match row { + wfl::interpreter::value::Value::Object(map) => { + match map.borrow().get("tag").expect("tag column") { + wfl::interpreter::value::Value::Text(t) => t.to_string(), + other => panic!("unexpected tag value {other:?}"), + } + } + other => panic!("unexpected row {other:?}"), + }) + .collect(), + other => panic!("expected a list of rows, got {other:?}"), + } +} From f4f72dc82209459cafddf202627eee951410d66a Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Fri, 31 Jul 2026 09:30:12 +0000 Subject: [PATCH 08/10] build: refresh fuzz lockfile for the merged dependency set The merge with main combined this branch's `toml` dependency with #679's TLS change (warp's legacy `tls` feature dropped in favour of tokio-rustls 0.26). `fuzz/` is a separate workspace with its own lockfile and the fuzz CI job builds it with `--locked`, so it has to carry both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- fuzz/Cargo.lock | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index a52c6aad..386eabf9 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -2,6 +2,16 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "aead" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1973cfbc1a2daf9cf550e74e1f088c28e7f7d8c1e1418fb6c9dc5184b7e84c99" +dependencies = [ + "crypto-common 0.2.2", + "inout 0.2.2", +] + [[package]] name = "aho-corasick" version = "1.1.4" @@ -214,8 +224,23 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d524456ba66e72eb8b115ff89e01e497f8e6d11d78b70b1aa13c0fbd97540a81" dependencies = [ "cfg-if", + "cipher 0.5.2", "cpufeatures 0.3.0", "rand_core 0.10.1", + "zeroize", +] + +[[package]] +name = "chacha20poly1305" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b89e1c441e926b9c82a8d023f6e1b7ae0adcfaa7d621814e4d60789bac751cb" +dependencies = [ + "aead", + "chacha20", + "cipher 0.5.2", + "poly1305", + "zeroize", ] [[package]] @@ -247,6 +272,7 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8cf2a2c93cd704877c0858356ed03480ff301ee950b43f1cbe4573b088bfa6c" dependencies = [ + "block-buffer 0.12.1", "crypto-common 0.2.2", "inout 0.2.2", ] @@ -395,7 +421,9 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ce6e4c961d6cd6c9a86db418387425e8bdeaf05b3c8bc1411e6dca4c252f1453" dependencies = [ + "getrandom 0.4.3", "hybrid-array", + "rand_core 0.10.1", ] [[package]] @@ -1588,6 +1616,16 @@ version = "0.3.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" +[[package]] +name = "poly1305" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2d0073b297041425c7c3df6eb4792d598a15323fe63346852b092eca02904c" +dependencies = [ + "cpufeatures 0.3.0", + "universal-hash", +] + [[package]] name = "potential_utf" version = "0.1.5" @@ -2110,6 +2148,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -2685,6 +2732,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.4+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3aace63f4bbcdfc2c965b059de67119c89c4017a70d633be6c104910f67056f5" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -2838,6 +2924,16 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" +[[package]] +name = "universal-hash" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f4987bdc12753382e0bec4a65c50738ffaabc998b9cdd1f952fb5f39b0048a96" +dependencies = [ + "crypto-common 0.2.2", + "ctutils", +] + [[package]] name = "untrusted" version = "0.9.0" @@ -3064,6 +3160,7 @@ dependencies = [ "argon2", "bcrypt", "bytes", + "chacha20poly1305", "chrono", "codespan-reporting", "encoding_rs", @@ -3092,6 +3189,7 @@ dependencies = [ "time", "tokio", "tokio-rustls", + "toml", "uuid", "warp", "zeroize", @@ -3272,6 +3370,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "writeable" version = "0.6.3" From e90c10d703a7ab3f37cc5a2b9471df3a3756a8f4 Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Fri, 31 Jul 2026 10:10:43 +0000 Subject: [PATCH 09/10] =?UTF-8?q?fix:=20second=20review=20round=20?= =?UTF-8?q?=E2=80=94=20transaction=20scoping=20fallout,=20type=20contracts?= =?UTF-8?q?,=20lint=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit reviewed the merged branch. Four findings were bugs I introduced, two of them fallout from the transaction-scoping fix in the previous round. **`close_database` only checked the caller's scope.** Keying transactions by `(scope, handle)` meant the guard that refuses to close a pool with a live transaction no longer saw transactions belonging to other scopes — so a handler could close the pool out from under another handler's transaction, which is the outcome the guard exists to prevent. It now matches the handle across all scopes; the scope parameter is gone from `close_database` since it was only misleading. **The transaction block never reported its completion type.** It used `check_statement_block`, which discards it, where `TryStatement` — the construct this block explicitly mirrors — uses `check_statement_block_with_completion`. An action whose body ends in a transaction was therefore inferred to return nothing, making `store n as call row_count` / `n times 2` a spurious "Multiply on Nothing and Number" error on a program that runs correctly. Reproduced before fixing; `tests/transaction_completion_type_test.rs` pins it with the `try:` shape alongside as a baseline. (The review also predicted a false error from a declared return type — not reachable, the parser always sets `return_type: None`.) **Static contracts for `stringify_toml` contradicted the runtime and the docs.** The JSON value set was registered, so `stringify_toml of 42` type-checked and then failed at runtime; `wfl_to_toml_document` accepts only a table, which the TOML page states and a test already asserted. Narrowed to `map(Text, Any)`. **A failed rollback flattened the error kind.** `RuntimeError::new` yields `General`, discarding `Cancelled`/`Timeout`/`ResourceLimit` — kinds that select `when` clauses and drive concurrent-handler classification, so a client disconnect inside a transaction with a failing rollback would have been counted as a structural handler failure. Uses `with_kind` and keeps `err.kind`. Also: - `collect_calls_in_statement` ignored `DatabaseQueryStatement`, so a `random_seed` or `seal` call in a SQL string or bound parameter escaped the security lint, transaction or not. - Docs said the transaction-control guard applies to `execute`; it applies to `query` too, now stated and shown. - The crypto context example could not have passed docs validation — undefined `api_token`, and a deliberately-failing call outside `try:`. Rewritten and run against the binary. - An empty context is now refused rather than being a synonym for "no context". Both mapped to the same associated data, so a context built from a missing config key produced an unbound ciphertext that looked bound. The review suggested documenting the equivalence; erroring matches how the rest of this module treats probably-mistaken input and removes the ambiguity instead. - Added the uniform-failure test the review asked for: tampered ciphertext, tampered tag and truncated blob must be indistinguishable, or `unseal` is an oracle. - Test quality: bounded the server join in the handler-scope test, added its missing success-path response, corrected a comment claiming concurrency a serial test does not exercise, and made the "does not contain the plaintext" TestPrograms case assert that property. Not changed: the review flagged the `StaticAnalyzer` import in `tests/transaction_analyzer_walk_test.rs` as unused. It provides `check_unused_variables`, and clippy runs `-D warnings`. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- Docs/04-advanced-features/databases.md | 6 +- Docs/05-standard-library/crypto-module.md | 22 ++++- ...07-31-transactions-aead-file-modes-toml.md | 63 +++++++++++++++ TestPrograms/crypto_seal_test.wfl | 3 + src/analyzer/static_analyzer.rs | 17 ++++ src/builtins.rs | 3 + src/interpreter/mod.rs | 25 ++++-- src/stdlib/crypto.rs | 41 +++++++--- src/stdlib/typechecker.rs | 26 +++--- src/typechecker/mod.rs | 12 ++- tests/crypto_seal_test.rs | 81 +++++++++++++++++++ tests/database_transaction_test.rs | 8 +- tests/transaction_analyzer_walk_test.rs | 34 ++++++-- tests/transaction_completion_type_test.rs | 75 +++++++++++++++++ tests/transaction_handler_scope_test.rs | 14 +++- 15 files changed, 376 insertions(+), 54 deletions(-) create mode 100644 tests/transaction_completion_type_test.rs diff --git a/Docs/04-advanced-features/databases.md b/Docs/04-advanced-features/databases.md index c8b1dd83..179bba6a 100644 --- a/Docs/04-advanced-features/databases.md +++ b/Docs/04-advanced-features/databases.md @@ -160,12 +160,14 @@ WFL does not currently expose. **A database cannot be closed inside its own transaction.** `close database` during an open block is an error; let the block finish first. -### Do not send BEGIN or COMMIT through `execute` +### Do not send BEGIN or COMMIT through `query` or `execute` -Writing transaction control as SQL does not work, and WFL now says so: +Writing transaction control as SQL does not work, and WFL now says so. Both +statements are checked, not just `execute`: ```wfl store t as execute db with "BEGIN" // Error, with a pointer to the block syntax +store r as query db with "COMMIT" // Same error — `query` is checked too ``` The reason is the connection pool. `BEGIN`, the statements after it, and diff --git a/Docs/05-standard-library/crypto-module.md b/Docs/05-standard-library/crypto-module.md index 613b260f..497da898 100644 --- a/Docs/05-standard-library/crypto-module.md +++ b/Docs/05-standard-library/crypto-module.md @@ -562,7 +562,7 @@ seal of <plaintext> and <key> and <context> **Parameters:** - `plaintext` (Text): The secret to protect - `key` (Text): A 64-character hex key — exactly what `secure_random_bytes of 32` returns -- `context` (Text, optional): Associated data that binds this ciphertext to where it lives +- `context` (Text, optional): Associated data that binds this ciphertext to where it lives. It must not be empty — leave the argument out entirely to seal without one. **Returns:** Text — a sealed value beginning with `wflseal1:` @@ -581,11 +581,27 @@ Sealing the same text twice never produces the same result — each call generat **Binding a secret to its context.** The optional third argument is authenticated but not encrypted. It ties the ciphertext to a place, so a value lifted out of one record cannot be replayed into another: ```wfl +store project_key as secure_random_bytes of 32 +store api_token as "sk-live-provider-token" + store sealed as seal of api_token and project_key and "project:acme/api_key" -store token as unseal of sealed and project_key and "project:acme/api_key" // works -store stolen as unseal of sealed and project_key and "project:evil/api_key" // error +store token as unseal of sealed and project_key and "project:acme/api_key" +display token // sk-live-provider-token + +// The same ciphertext under another project's context is refused. +try: + store stolen as unseal of sealed and project_key and "project:evil/api_key" + display "This should not be reachable" +when error: + display "Refused — this value belongs to another project" +end try ``` +An **empty** context is an error rather than a synonym for "no context". Passing +the argument means you intend to bind the value to something, so a context built +from a config key that turned out to be missing is reported instead of quietly +producing an unbound ciphertext. + **Use Cases:** - API tokens and provider keys written to a config file - Any credential that has to be read back, not merely checked diff --git a/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md index 27616a8f..5e036b48 100644 --- a/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md +++ b/History/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.md @@ -282,3 +282,66 @@ The pattern worth remembering: the findings I would not have reached on my own were all about what happens when two things run at once, or when input is hostile. Those are the parts where reading the code carefully is not the same as proving it. + +## A second review round + +CodeRabbit went over the merged branch and raised thirteen items. Four were bugs +I had introduced, and two of those came directly out of the scoping fix from the +first round — which is a useful reminder that a fix is a change like any other. + +**Keying transactions by `(scope, handle)` broke `close_database`.** The guard +that refuses to close a pool with a transaction open was still looking up the +caller's scope only, so a handler in a *different* scope could close the pool out +from under a live transaction — precisely the thing the guard exists to stop. It +now matches on the handle across every scope. + +**The transaction block never reported its completion type.** It calls +`check_statement_block`, which discards it, where `TryStatement` — the construct +this block explicitly claims to mirror — uses `check_statement_block_with_completion`. +The consequence is visible: an action whose body ends in a transaction is +inferred to return nothing, so `store n as call row_count` followed by +`n times 2` is reported as "Cannot perform Multiply operation on Nothing and +Number" for a program that runs correctly. Worth noting that the review +overstated the blast radius — it predicted a false error from a *declared* +return type, but WFL's parser always sets `return_type: None`, so that path is +unreachable today. The inferred-type path is real, and I reproduced it before +fixing it. + +**The static contracts for `stringify_toml` contradicted the runtime, and my own +docs.** I registered the JSON value set — scalars and lists included — while +`wfl_to_toml_document` accepts only a table, which the TOML page states and a +test already asserted. So `stringify_toml of 42` type-checked and then failed at +runtime. Narrowed to `map(Text, Any)`. + +**A failed rollback flattened the error kind.** `RuntimeError::new` produces +`General`, discarding `Cancelled`/`Timeout`/`ResourceLimit` — kinds that select +`when` clauses and drive concurrent-handler classification. A client disconnect +inside a transaction whose rollback also failed would have been counted as a +structural handler failure. + +Also fixed: the call collector still ignored `DatabaseQueryStatement`, so a +`random_seed` or `seal` call hidden in a SQL string or a bound parameter escaped +the security lint whether or not a transaction was involved; the docs said the +transaction-control guard applied to `execute` when it applies to `query` too; +and the crypto context example could not have passed docs validation, because it +used an undefined `api_token` and let a deliberately-failing call abort the +program. + +One review item was wrong: it flagged the `StaticAnalyzer` import in +`tests/transaction_analyzer_walk_test.rs` as unused. It is the trait that +provides `check_unused_variables`, and clippy runs with `-D warnings`, so an +actually-unused import could not have got this far. + +One I decided differently than suggested. An absent context and an empty one +were the same associated data, since `context.as_deref().unwrap_or("")` maps both +to `""`. The suggestion was to document the equivalence. I made it an error +instead: passing the argument means you intend to bind the value to something, so +a context assembled from a config key that turned out to be missing should be +reported rather than quietly producing an unbound ciphertext that looks bound. +That matches how the rest of this module treats probably-mistaken input — strict +mode strings, exact key lengths — and removes the subtlety rather than explaining +it. + +The pattern from the first round repeated: the findings I would not have reached +alone were about interactions — a fix meeting an older guard, a type contract +meeting its runtime, an error kind meeting the code that classifies it. diff --git a/TestPrograms/crypto_seal_test.wfl b/TestPrograms/crypto_seal_test.wfl index 129d27fc..acc9ee0f 100644 --- a/TestPrograms/crypto_seal_test.wfl +++ b/TestPrograms/crypto_seal_test.wfl @@ -25,6 +25,9 @@ describe "Sealing and unsealing": store secret_key as secure_random_bytes of 32 store sealed as seal of "correct-horse-battery-staple" and secret_key expect sealed to contain "wflseal1:" + // The name of this test is the assertion that matters. + store leaked as sealed contains "correct-horse" + expect leaked to be no end test test "sealing the same text twice gives different results": diff --git a/src/analyzer/static_analyzer.rs b/src/analyzer/static_analyzer.rs index 7435ba8e..6a286023 100644 --- a/src/analyzer/static_analyzer.rs +++ b/src/analyzer/static_analyzer.rs @@ -242,6 +242,23 @@ fn collect_calls_in_statement(stmt: &Statement, out: &mut Vec<CallSite>) { collect_calls_in_expression(db, out); collect_calls_in_statements(body, out); } + // A database statement's operands are ordinary expressions, so a call + // can hide in the handle, the SQL text, or a bound parameter — e.g. + // `query db with "..." and parameters [seal of secret and key]`. Without + // this arm those calls are invisible to the security lint whether or not + // they sit inside a transaction block. + Statement::DatabaseQueryStatement { + db, + sql, + parameters, + .. + } => { + collect_calls_in_expression(db, out); + collect_calls_in_expression(sql, out); + if let Some(parameters) = parameters { + collect_calls_in_expression(parameters, out); + } + } Statement::SingleLineIf { condition, then_stmt, diff --git a/src/builtins.rs b/src/builtins.rs index 49584496..b8ccbb60 100644 --- a/src/builtins.rs +++ b/src/builtins.rs @@ -517,6 +517,9 @@ pub fn get_function_arity(name: &str) -> usize { // === JSON FUNCTIONS === // Single argument functions "parse_json" | "stringify_json" | "stringify_json_pretty" => 1, + + // === TOML FUNCTIONS === + // Single argument functions "parse_toml" | "stringify_toml" | "stringify_toml_pretty" => 1, // === QUERY AND FORM PARSING === diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 28400e72..cc562349 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -2487,12 +2487,18 @@ impl IoClient { /// Refuses while a transaction is open on the handle: closing the pool /// under an in-flight transaction would discard the work with no report of /// what happened to it. - async fn close_database(&self, scope: u64, handle_id: &str) -> Result<(), String> { + async fn close_database(&self, handle_id: &str) -> Result<(), String> { + // Any scope's transaction blocks the close, not just the caller's. + // Handles are plain text, so two concurrent handlers can name the same + // one; checking only the caller's scope would let handler B close the + // pool out from under handler A's live transaction — the exact outcome + // this guard exists to prevent. if self .db_transactions .lock() .await - .contains_key(&(scope, handle_id.to_string())) + .keys() + .any(|(_, handle)| handle == handle_id) { return Err(format!( "Cannot close database '{handle_id}' while a transaction is open on it. \ @@ -6474,10 +6480,16 @@ impl Interpreter { .await { Ok(()) => Err(err), - Err(rollback_error) => Err(RuntimeError::new( + // Keep the original kind. `Cancelled`, `Timeout` and + // `ResourceLimit` select `when` clauses and drive concurrent + // handler classification, so flattening them to `General` + // would turn a client disconnect inside a transaction into a + // structural handler failure. + Err(rollback_error) => Err(RuntimeError::with_kind( format!("{} (rollback also failed: {rollback_error})", err.message), err.line, err.column, + err.kind, )), } } @@ -7400,10 +7412,7 @@ impl Interpreter { Err(msg) => { // Don't leave an unreachable pool behind when // the variable binding fails. - let _ = self - .io_client - .close_database(self.tx_scope.get(), &handle) - .await; + let _ = self.io_client.close_database(&handle).await; Err(RuntimeError::new(msg, *line, *column)) } } @@ -7451,7 +7460,7 @@ impl Interpreter { }; self.io_client - .close_database(self.tx_scope.get(), &handle) + .close_database(&handle) .await .map_err(|e| RuntimeError::new(e, *line, *column))?; diff --git a/src/stdlib/crypto.rs b/src/stdlib/crypto.rs index 0dc4dd00..1bfd1be1 100644 --- a/src/stdlib/crypto.rs +++ b/src/stdlib/crypto.rs @@ -1082,6 +1082,35 @@ fn parse_seal_key(func_name: &str, key_text: &str) -> Result<Zeroizing<Vec<u8>>, /// The optional context (associated data) is authenticated but not encrypted, /// and binds the ciphertext to where it lives: a blob sealed with one context /// will not unseal under another. +/// Read the optional third argument as associated data. +/// +/// An empty context is refused rather than treated as "no context". Passing the +/// argument at all means the caller intends to bind this value to something, and +/// `context.as_deref().unwrap_or("")` would otherwise make `""` and an absent +/// argument the same associated data — so a context read from a config key that +/// turned out to be missing would silently produce an *unbound* ciphertext while +/// the author believed it was bound. Better to say so. +fn seal_context(func_name: &str, args: &[Value]) -> Result<Option<Arc<str>>, RuntimeError> { + match args.get(2) { + Some(value) => { + let context = expect_text(value)?; + if context.is_empty() { + return Err(RuntimeError::new( + format!( + "{func_name}: the context must not be empty. Leave the argument out \ + entirely to seal without a context, or pass the value this secret \ + belongs to (for example \"project:acme/api_key\")." + ), + 0, + 0, + )); + } + Ok(Some(context)) + } + None => Ok(None), + } +} + pub fn native_seal(args: Vec<Value>) -> Result<Value, RuntimeError> { use chacha20poly1305::aead::{Aead, KeyInit, Payload}; use chacha20poly1305::{XChaCha20Poly1305, XNonce}; @@ -1091,10 +1120,7 @@ pub fn native_seal(args: Vec<Value>) -> Result<Value, RuntimeError> { let plaintext = expect_text(&args[0])?; let key_text = expect_text(&args[1])?; - let context = match args.get(2) { - Some(value) => Some(expect_text(value)?), - None => None, - }; + let context = seal_context("seal", &args)?; let key = parse_seal_key("seal", &key_text)?; let cipher = XChaCha20Poly1305::new_from_slice(&key) @@ -1144,10 +1170,7 @@ pub fn native_unseal(args: Vec<Value>) -> Result<Value, RuntimeError> { let sealed = expect_text(&args[0])?; let key_text = expect_text(&args[1])?; - let context = match args.get(2) { - Some(value) => Some(expect_text(value)?), - None => None, - }; + let context = seal_context("unseal", &args)?; let key = parse_seal_key("unseal", &key_text)?; @@ -1166,7 +1189,7 @@ pub fn native_unseal(args: Vec<Value>) -> Result<Value, RuntimeError> { RuntimeError::new( format!( "unseal: this is not a sealed value. Expected text beginning with \ - '{SEAL_V1_PREFIX}', as produced by `seal`." + '{SEAL_V1_PREFIX}', as produced by `seal`." ), 0, 0, diff --git a/src/stdlib/typechecker.rs b/src/stdlib/typechecker.rs index e8562dad..5e3bb68c 100644 --- a/src/stdlib/typechecker.rs +++ b/src/stdlib/typechecker.rs @@ -349,24 +349,16 @@ fn register_json(analyzer: &mut Analyzer) { fn register_toml(analyzer: &mut Analyzer) { register(analyzer, &["parse_toml"], vec![Type::Text], Type::Any); - // Mirrors the JSON value set. TOML has no null, but `nothing` is accepted - // here because a table simply omits those keys (see stdlib::toml). - let toml_values = [ - Type::Nothing, - Type::Boolean, - Type::Number, + // Only a table, unlike JSON. `wfl_to_toml_document` rejects anything else at + // runtime because there is no valid TOML document whose top level is a list + // or a scalar, so accepting them here would type-check a call that is + // guaranteed to fail. Scalars and lists remain valid *inside* a table. + register( + analyzer, + &["stringify_toml", "stringify_toml_pretty"], + vec![map(Type::Text, Type::Any)], Type::Text, - list(Type::Any), - map(Type::Text, Type::Any), - ]; - for value_type in toml_values { - register( - analyzer, - &["stringify_toml", "stringify_toml_pretty"], - vec![value_type], - Type::Text, - ); - } + ); } fn register_web(analyzer: &mut Analyzer) { diff --git a/src/typechecker/mod.rs b/src/typechecker/mod.rs index 5a8bcdba..d39e1d34 100644 --- a/src/typechecker/mod.rs +++ b/src/typechecker/mod.rs @@ -5868,9 +5868,15 @@ impl TypeChecker { *_column, ); } - // Like `try:`, the block shares the enclosing scope and - // introduces no bindings of its own. - self.check_statement_block(body); + // Like `try:`, the block shares the enclosing scope, introduces + // no bindings of its own, and passes its body's value through — + // `execute_transaction_statement` returns the body's last value + // directly. Recording the completion keeps an action whose body + // ends in a transaction from being inferred as returning + // nothing, which would make every later use of its result a + // spurious type error. + let (_, completion) = self.check_statement_block_with_completion(body); + self.current_statement_completion = completion; } Statement::CreateDirectoryStatement { path, diff --git a/tests/crypto_seal_test.rs b/tests/crypto_seal_test.rs index 5f5ad424..6de022de 100644 --- a/tests/crypto_seal_test.rs +++ b/tests/crypto_seal_test.rs @@ -413,3 +413,84 @@ store sealed as seal of "secret" and "{key}" .err() .expect("a multi-byte character inside a key must be reported, not panic"); } + +/// The uniform-failure property itself, not just its individual cases. +/// +/// `unseal` funnels every failure — bad hex, wrong length, wrong key, tampered +/// nonce/ciphertext/tag, truncation — into one message on purpose: telling an +/// attacker *which* part failed helps them work toward a valid value. Each case +/// is covered above, but nothing yet asserts they are indistinguishable from one +/// another, so a later change that split the messages would still pass. +#[tokio::test] +async fn every_unseal_failure_reports_the_same_message() { + let sealed = seal_once("top secret").await; + let body_len = sealed.split_once(':').expect("prefixed blob").1.len(); + + let tampered_ciphertext = flip_hex_digit(&sealed, 60); + let tampered_tag = flip_hex_digit(&sealed, body_len - 1); + let truncated = sealed[..sealed.len() - 8].to_string(); + + let baseline = expect_unseal_failure(&tampered_ciphertext, "a tampered ciphertext").await; + for (blob, what) in [ + (tampered_tag, "a tampered tag"), + (truncated, "a truncated blob"), + ] { + let other = expect_unseal_failure(&blob, what).await; + assert_eq!( + baseline, other, + "{what} must be indistinguishable from a tampered ciphertext, \ + or `unseal` becomes an oracle" + ); + } +} + +/// An empty context is refused rather than silently meaning "no context". +/// +/// Without this, `seal of secret and key and ""` and `seal of secret and key` +/// would produce interchangeable values, so a context built from a config key +/// that turned out to be missing would yield an *unbound* ciphertext while the +/// program appeared to bind it. +#[tokio::test] +async fn an_empty_context_is_refused_on_seal() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "secret" and key and "" +"# + ); + run_wfl(&code) + .await + .err() + .expect("an empty context must be refused, not treated as no context"); +} + +#[tokio::test] +async fn an_empty_context_is_refused_on_unseal() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "secret" and key +store plain as unseal of sealed and key and "" +"# + ); + run_wfl(&code) + .await + .err() + .expect("an empty context must be refused on unseal too"); +} + +/// Omitting the argument entirely still works — the rejection is of an empty +/// *value*, not of the two-argument form. +#[tokio::test] +async fn omitting_the_context_entirely_is_still_supported() { + let code = format!( + r#" +store key as "{KEY}" +store sealed as seal of "secret" and key +store plain as unseal of sealed and key +"# + ); + run_wfl(&code) + .await + .expect("sealing without a context must keep working"); +} diff --git a/tests/database_transaction_test.rs b/tests/database_transaction_test.rs index be9153ac..447e4030 100644 --- a/tests/database_transaction_test.rs +++ b/tests/database_transaction_test.rs @@ -590,9 +590,11 @@ async fn a_slow_statement_in_a_transaction_does_not_block_an_unrelated_handle() let slow_url = &slow_db.url; let other_url = &other_db.url; - // `main loop concurrently` runs both bodies on one thread, interleaved at - // await points. If the unrelated query cannot make progress until the - // transaction finishes, this deadlocks and the test times out. + // These statements run serially — the concurrent-handler case is covered by + // tests/transaction_handler_scope_test.rs. What this pins is the lock + // discipline: the unrelated handle is queried while the transaction is still + // open, so if the transaction map's lock were held across the SQL await that + // query could never acquire it and this program would deadlock. let code = format!( r#" open database at "{slow_url}" as slow_db diff --git a/tests/transaction_analyzer_walk_test.rs b/tests/transaction_analyzer_walk_test.rs index d18a0177..336d9810 100644 --- a/tests/transaction_analyzer_walk_test.rs +++ b/tests/transaction_analyzer_walk_test.rs @@ -57,21 +57,45 @@ end transaction ); } -/// The database expression in the block header is walked too. +/// Calls in a database statement's operands are collected too — the handle, the +/// SQL text, and any bound parameters are ordinary expressions, so a +/// security-sensitive call can hide in one whether or not a transaction block is +/// involved. #[test] -fn calls_in_the_transaction_header_are_collected() { +fn calls_in_database_statement_operands_are_collected() { let program = parse( r#" open database at "sqlite::memory:" as db +store seeded as random_seed of 42 +store rows as query db with "SELECT 1" and parameters [hash_password of "hunter2"] +"#, + ); + let found = rng_security_ingredients(&program); + assert!(found.seed_site.is_some(), "random_seed must be seen"); + assert!( + found.security_site.is_some(), + "a crypto call inside a bound parameter must not escape the security lint" + ); +} + +/// ...and the same holds inside a transaction block, where both walks apply. +#[test] +fn calls_in_database_operands_inside_a_transaction_are_collected() { + let program = parse( + r#" +open database at "sqlite::memory:" as db +store seeded as random_seed of 42 in transaction on db: - store seeded as random_seed of 42 + store rows as query db with "SELECT 1" and parameters [hash_password of "hunter2"] end transaction -store token as hash_password of "hunter2" "#, ); let found = rng_security_ingredients(&program); assert!(found.seed_site.is_some()); - assert!(found.security_site.is_some()); + assert!( + found.security_site.is_some(), + "a crypto call in a database parameter inside a transaction must still be seen" + ); } /// The unused-variable walk must see declarations inside the block. Its failure diff --git a/tests/transaction_completion_type_test.rs b/tests/transaction_completion_type_test.rs new file mode 100644 index 00000000..739ecb8b --- /dev/null +++ b/tests/transaction_completion_type_test.rs @@ -0,0 +1,75 @@ +//! A transaction block passes its body's value through, like `try:` does. +//! +//! `execute_transaction_statement` returns the body's last value directly, and +//! the block shares the enclosing scope for exactly that reason. The type +//! checker has to agree: if it records the block's completion type as `Nothing`, +//! an action whose body ends in a transaction is inferred to return nothing, and +//! every later use of that result is reported as a type error on a program that +//! runs correctly. + +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; +use wfl::typechecker::TypeChecker; + +fn type_errors(source: &str) -> Vec<String> { + let tokens = lex_wfl_with_positions(source); + let program = Parser::new(&tokens).parse().expect("program must parse"); + let mut checker = TypeChecker::new(); + match checker.check_types(&program) { + Ok(()) => Vec::new(), + Err(error) => vec![format!("{error:?}")], + } +} + +/// The value produced inside the block is the action's result, so arithmetic on +/// it is valid. +#[test] +fn an_action_ending_in_a_transaction_returns_the_blocks_value() { + let errors = type_errors( + r#" +open database at "sqlite::memory:" as db +define action called row_count: + in transaction on db: + store rows as query db with "SELECT x FROM t" + length of rows + end transaction +end action + +store n as call row_count +store doubled as n times 2 +display doubled +"#, + ); + + assert!( + errors.is_empty(), + "the transaction block must pass its body's value through, as `try:` does; got: {errors:?}" + ); +} + +/// The same shape written with `try:` is the reference behaviour the block is +/// documented to mirror. If this ever fails, the comparison above is moot. +#[test] +fn the_same_shape_with_try_is_accepted() { + let errors = type_errors( + r#" +define action called row_count: + try: + store rows as [1 and 2] + length of rows + when error: + 0 + end try +end action + +store n as call row_count +store doubled as n times 2 +display doubled +"#, + ); + + assert!( + errors.is_empty(), + "baseline: `try:` already propagates its completion type; got: {errors:?}" + ); +} diff --git a/tests/transaction_handler_scope_test.rs b/tests/transaction_handler_scope_test.rs index a34a003c..da70d960 100644 --- a/tests/transaction_handler_scope_test.rs +++ b/tests/transaction_handler_scope_test.rs @@ -91,10 +91,15 @@ async fn shutdown(port: u16, server: std::thread::JoinHandle<()>) { .get(format!("http://127.0.0.1:{port}/shutdown")) .send() .await; - match tokio::task::spawn_blocking(move || server.join()).await { - Ok(Ok(())) => {} - Ok(Err(panic)) => std::panic::resume_unwind(panic), - Err(join_err) => panic!("server join task failed: {join_err}"), + // Bounded: an unbounded join turns a server that never exits into a silent + // hang until the harness kills the run, which is a far weaker signal than a + // named failure. + let join = tokio::task::spawn_blocking(move || server.join()); + match tokio::time::timeout(Duration::from_secs(30), join).await { + Ok(Ok(Ok(()))) => {} + Ok(Ok(Err(panic))) => std::panic::resume_unwind(panic), + Ok(Err(join_err)) => panic!("server join task failed: {join_err}"), + Err(_) => panic!("server thread did not exit within 30s after /shutdown"), } } @@ -130,6 +135,7 @@ async fn an_unrelated_handler_is_not_enrolled_in_another_handlers_transaction() wait for 600 milliseconds store boom as execute db with "INSERT INTO no_such_table (x) VALUES (1)" end transaction + respond to req with "tx-committed" when error: respond to req with "tx-rolled-back" end try From 5960888c9ba92bccd58a8c5ac864bd6e5cb3585b Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Fri, 31 Jul 2026 10:16:18 +0000 Subject: [PATCH 10/10] test: restore coverage for the transaction header expression walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's finding that `calls_in_the_transaction_header_are_collected` did not actually test the header was auto-marked resolved by e90c10d, but that commit *replaced* the weak test with two about database statement operands rather than strengthening it. Net effect: the header walk in `collect_calls_in_statement` lost its only test, weak as it was. The header takes a full primary expression — `in transaction on call get_db:` and `in transaction on get_db of 1:` both parse — so a security-sensitive call really can sit there, and the line is reachable rather than defensive. The new test puts the only crypto call in the header and keeps `random_seed` outside it, so it fails if the walk stops descending. Verified as a genuine Red by removing `collect_calls_in_expression(db, out)` and watching it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL --- tests/transaction_analyzer_walk_test.rs | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/transaction_analyzer_walk_test.rs b/tests/transaction_analyzer_walk_test.rs index 336d9810..c0ee1ce4 100644 --- a/tests/transaction_analyzer_walk_test.rs +++ b/tests/transaction_analyzer_walk_test.rs @@ -98,6 +98,32 @@ end transaction ); } +/// The block *header* is walked too, not just the body. +/// +/// `in transaction on <expr>:` takes a full primary expression — `call get_db` +/// and `get_db of 1` both parse — so a security-sensitive call can sit in the +/// header itself. This program is not meant to run; it exists so that the test +/// fails if `collect_calls_in_statement` stops descending into the header, which +/// is the one place the body walk cannot cover. +#[test] +fn calls_in_the_transaction_header_are_collected() { + let program = parse( + r#" +store seeded as random_seed of 42 +in transaction on hash_password of "hunter2": + display "body" +end transaction +"#, + ); + let found = rng_security_ingredients(&program); + assert!(found.seed_site.is_some(), "random_seed must be seen"); + assert!( + found.security_site.is_some(), + "the only crypto call is in the transaction header, so this fails if the \ + header expression is not walked" + ); +} + /// The unused-variable walk must see declarations inside the block. Its failure /// mode is a false negative — a variable the block declares and nobody uses is /// simply never tracked, so it is never reported.