fix: remove vulnerable legacy TLS dependency chain - #679
Merged
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+273
to
+291
| impl Accept for SecuredIncoming { | ||
| type Conn = SecuredStream; | ||
| type Error = io::Error; | ||
|
|
||
| fn poll_accept( | ||
| self: Pin<&mut Self>, | ||
| context: &mut Context<'_>, | ||
| ) -> Poll<Option<Result<Self::Conn, Self::Error>>> { | ||
| let this = self.get_mut(); | ||
| match ready!(Pin::new(&mut this.incoming).poll_accept(context)) { | ||
| Some(Ok(stream)) => Poll::Ready(Some(Ok(SecuredStream::new( | ||
| stream, | ||
| Arc::clone(&this.config), | ||
| )))), | ||
| Some(Err(error)) => Poll::Ready(Some(Err(error))), | ||
| None => Poll::Ready(None), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🟨 Secured listener has no TLS handshake timeout or connection cap
Each accepted TCP connection on the secured listener spawns a Hyper task whose TLS handshake is driven lazily (SecuredStream::poll_read at src/interpreter/tls.rs:48-67) with no handshake deadline and no application-level cap on concurrent pre-HTTP connections. A client that opens many sockets and never sends a ClientHello keeps tasks, sockets, and per-connection buffers alive indefinitely until the server is closed, so exhaustion is bounded only by OS file-descriptor limits.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe PR replaces Warp’s legacy TLS feature with a custom Tokio-Rustls transport. It adds asynchronous handshakes, tracked connection cancellation, peer-address propagation, TLS configuration validation, expanded integration tests, and dependency remediation records. TLS transport remediation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TLSClient
participant SecuredIncoming
participant SecuredStream
participant HyperServer
TLSClient->>SecuredIncoming: connect to secure listener
SecuredIncoming->>SecuredStream: accept TCP connection
SecuredStream->>TLSClient: negotiate Rustls TLS and ALPN
SecuredStream->>HyperServer: provide established stream and peer address
HyperServer->>TLSClient: serve HTTP/2 or HTTP/1.1 request
HyperServer->>SecuredIncoming: close server
SecuredIncoming->>SecuredStream: cancel active and stalled connections
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/web_server_tls_test.rs (2)
607-614: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm the reported ephemeral port is the actual listener.
The assertion proves the port is non-zero. It does not prove the listener bound that port. A connect to the reported port would prove the reported value and the bound socket agree.
♻️ Proposed addition
assert_ne!( actual_port, 0, "A port-zero TLS listener must report the actual ephemeral port" ); + tokio::time::timeout( + Duration::from_secs(2), + tokio::net::TcpStream::connect(("127.0.0.1", actual_port)), + ) + .await + .expect("Connecting to the reported ephemeral port timed out") + .expect("The reported ephemeral port must accept connections");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/web_server_tls_test.rs` around lines 607 - 614, Extend the TLS server port test after parsing actual_port to connect to the reported address and assert the connection succeeds, proving it matches the listener’s bound ephemeral port. Reuse the existing server address/setup symbols and retain the non-zero assertion.
449-495: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that no listener was bound when credentials are rejected.
Both cases prove the error message. Neither proves the socket was never bound. The PR objective states that invalid certificate/key pairs are rejected before binding, so make that observable.
Bind the port after each failing case. A successful bind proves the listener never claimed the port.
♻️ Proposed addition
assert!( malformed_message.contains("contains no private key"), "Malformed-key error should explain the PEM requirement, got: {malformed_message}" ); + std::net::TcpListener::bind(("127.0.0.1", 8222)) + .expect("A rejected credential pair must not leave port 8222 bound");assert!( mismatch_message.contains(&cert_path) && mismatch_message.contains(&unrelated_key_path) && mismatch_message.contains("not a valid pair"), "Certificate/key mismatch should identify both invalid inputs, got: {mismatch_message}" ); + std::net::TcpListener::bind(("127.0.0.1", 8223)) + .expect("A rejected credential pair must not leave port 8223 bound");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/web_server_tls_test.rs` around lines 449 - 495, The test test_tls_configuration_rejects_malformed_and_mismatched_keys only verifies error messages but never confirms the listener port was never bound when credentials fail validation. After both malformed_result and mismatch_result assertions, add a check that attempts to bind to the same port (8222 and 8223 respectively, e.g. via std::net::TcpListener::bind) and asserts the bind succeeds, proving no listener was left claiming the port from the failed interpret call. Add this verification after each of the two failure scenarios in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@History/dev-diary/2026/2026-07-31-dependabot-tls-remediation.md`:
- Around line 80-99: Rename the “broader Green verification” section to
“Verification results.” Update the verification summary to identify the failing
npm lint check and pre-existing extension assertion as unresolved or explicitly
accepted failures, and state the release decision without claiming full
validation or Green status. Keep required failures visible rather than skipping,
retrying, or reclassifying them as successful checks.
- Around line 43-46: Update the TLS connection handling described by the
migration to enforce both a timeout for stalled pre-HTTP handshakes and a
configurable maximum number of in-flight handshakes, releasing capacity on
completion, failure, or expiry; add tests covering timeout expiry and subsequent
capacity recovery. If these controls are intentionally deferred, replace the
current scope statement with an explicit security risk acceptance naming an
owner and remediation deadline.
---
Nitpick comments:
In `@tests/web_server_tls_test.rs`:
- Around line 607-614: Extend the TLS server port test after parsing actual_port
to connect to the reported address and assert the connection succeeds, proving
it matches the listener’s bound ephemeral port. Reuse the existing server
address/setup symbols and retain the non-zero assertion.
- Around line 449-495: The test
test_tls_configuration_rejects_malformed_and_mismatched_keys only verifies error
messages but never confirms the listener port was never bound when credentials
fail validation. After both malformed_result and mismatch_result assertions, add
a check that attempts to bind to the same port (8222 and 8223 respectively, e.g.
via std::net::TcpListener::bind) and asserts the bind succeeds, proving no
listener was left claiming the port from the failed interpret call. Add this
verification after each of the two failure scenarios in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ed83aaf-e2fa-4a92-ad3b-0a4b1ecf5b2b
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockfuzz/Cargo.lockis excluded by!**/*.lockvscode-extension/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
Cargo.tomlEngineering/plans/2026-07-31-dependabot-remediation.mdHistory/dev-diary/2026/2026-07-31-dependabot-tls-remediation.mdsrc/interpreter/mod.rssrc/interpreter/tls.rstests/web_server_tls_test.rsvscode-extension/package.json
Comment on lines
+43
to
+46
| The migration does not add a separate application-level cap for pre-HTTP TLS | ||
| connections. The tracker retains only currently active Hyper tasks and remains | ||
| bounded by the operating system's accepted-connection resources; adding a | ||
| configurable handshake limit or timeout is a separate hardening change. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound stalled TLS handshakes before accepting this R3 change.
Lines 43-46 state that pre-HTTP TLS connections have no timeout or application-level limit. A client can open many sockets and withhold ClientHello, consuming file descriptors, task memory, and accepted-connection capacity until shutdown. Add a handshake timeout and a bounded in-flight handshake count, with tests for expiry and recovery. If this control remains out of scope, record an explicit security risk acceptance with an owner and deadline.
As per coding guidelines, asynchronous web and lifecycle changes require timeout and resource-limit coverage and failure-path tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@History/dev-diary/2026/2026-07-31-dependabot-tls-remediation.md` around lines
43 - 46, Update the TLS connection handling described by the migration to
enforce both a timeout for stalled pre-HTTP handshakes and a configurable
maximum number of in-flight handshakes, releasing capacity on completion,
failure, or expiry; add tests covering timeout expiry and subsequent capacity
recovery. If these controls are intentionally deferred, replace the current
scope statement with an explicit security risk acceptance naming an owner and
remediation deadline.
Source: Coding guidelines
Comment on lines
+80
to
+99
| The broader Green verification was: | ||
|
|
||
| - `cargo fmt --all -- --check`; | ||
| - `cargo clippy --all-targets --all-features -- -D warnings`; | ||
| - `cargo test --all -j 2` (the unrestricted parallel compile exceeded this | ||
| Windows host's paging-file limit; two jobs completed the same suite); | ||
| - `cargo build --release -j 2`; | ||
| - `cargo check --manifest-path fuzz/Cargo.toml --all-targets`; | ||
| - 130 ordinary `TestPrograms` cases with the integration runner's timeout, | ||
| expected-failure, `--test`, exclusion, and `CI-SKIP` rules; | ||
| - `scripts/run_web_tests.ps1` (2/2; its optional OpenSSL-generated TLS fixture | ||
| was skipped because OpenSSL is unavailable, while the Rust TLS target ran | ||
| all 13 real-socket cases); | ||
| - `python scripts/validate_docs_examples.py` (19/19); | ||
| - `npm ci --ignore-scripts` and `npm run compile`. | ||
|
|
||
| The extension's existing `npm run lint` command cannot find an ESLint | ||
| configuration or a matching `src` target, so its `pretest` stops before the | ||
| test runner. Invoking the test runner directly produced 24 passing, 2 pending, | ||
| and one pre-existing Windows line-ending assertion failure. Static repository |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not label failed checks as Green.
Line 80 says “broader Green verification,” but Line 96 reports that npm run lint fails because no ESLint configuration exists, and Lines 98-99 report a failing extension assertion. Rename this section to Verification results, list these failures as unresolved or explicitly accepted pre-existing failures, and state the release decision. Do not claim full validation while required checks fail.
As per coding guidelines, required tests must remain failures and must not be skipped, retried, or presented as Green.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@History/dev-diary/2026/2026-07-31-dependabot-tls-remediation.md` around lines
80 - 99, Rename the “broader Green verification” section to “Verification
results.” Update the verification summary to identify the failing npm lint check
and pre-existing extension assertion as unresolved or explicitly accepted
failures, and state the release decision without claiming full validation or
Green status. Keep required failures visible rather than skipping, retrying, or
reclassifying them as successful checks.
Source: Coding guidelines
logbie
pushed a commit
that referenced
this pull request
Jul 31, 2026
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
logbie
added a commit
that referenced
this pull request
Jul 31, 2026
* test: failing coverage for transactions, AEAD, file modes and TOML 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * feat: seal and unseal, XChaCha20-Poly1305 authenticated encryption 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * feat: file_mode and set_file_mode 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * feat: parse_toml, stringify_toml and stringify_toml_pretty 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * fix: atomic transaction blocks, and reject transaction SQL sent through execute 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * 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 <name> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL * fix: review findings — AEAD panic, transaction scoping, concurrency and lint gaps 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 * 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 * fix: second review round — transaction scoping fallout, type contracts, lint gaps 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 * test: restore coverage for the transaction header expression walk 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
js-yaml4.2.0 to 4.3.0 and the affected nestedbrace-expansion5.0.7 copy to 5.0.9.This addresses all ten Dependabot alerts open on 2026-07-31: root Cargo alerts 40/41/49/59, fuzz Cargo alerts 60/61/62/63, and npm alerts 67/68. GitHub will close them only after this reaches the default branch and Dependabot rescans.
Impact
WFL syntax and the public behavior of plain HTTP, secured HTTP, redirects, standalone WebSockets, request
client_ip, port-zero binding, andclose serverare preserved. Invalid certificate/key pairs are now rejected before binding. Closing a secured server now also cancels accepted idle and ClientHello-stalled connections after the existing 50 ms response-flush allowance.Test evidence
tests/web_server_tls_test.rscovers occupied ports, malformed and mismatched keys, TLS 1.2/1.3, HTTP/2 ALPN plus HTTP/1.1 fallback, peer IP, stalled-handshake isolation, established/stalled connection cancellation, immediate rebind, and actual port-zero reporting.src/interpreter/tls.rscovers panic-safe task unregistration.2aa19de2produced 11 passing TLS cases and a failing certificate/key-mismatch case because the legacy listener accepted the pair. Commit0a90e770added the lifecycle regression; before tracked cancellation it failed withEstablished TLS connection remained open after close server. The panic-cleanup unit initially retained one dead abort handle (left: 1, expected0) before the RAII guard.cargo test --lib panicking_tracked_task_is_unregistered -- --nocapture --test-threads=1;cargo test --all --test web_server_tls_test -- --test-threads=1— 13/13.cargo test --all -j 2;cargo build --release -j 2;cargo check --manifest-path fuzz/Cargo.toml --all-targets -j 2;cargo fmt --all -- --check;cargo clippy --all-targets --all-features -- -D warnings.scripts/run_web_tests.ps1passed 2/2; forced docs validation passed 21/21 examples.cargo tree --locked -i rustls-webpki@0.102.8correctly report no matching package; both resolve only 0.103.13 through Rustls 0.23.42.npm ci --ignore-scripts,npm run compile, andnpm ls js-yaml brace-expansion --allpass withjs-yaml4.3.0 and the affected nestedbrace-expansion5.0.9.eslint srccommand has no usable lint target/config and blocksnpm test; direct VS Code tests are 24 passing, 2 pending, with one pre-existing Windows CRLF assertion failure. Working-tree hygiene passed clean in the disposable worktree after commit; a root-worktree hygiene scan sees only the two preserved user-owned staged.worktreesentries.npm auditremains nonzero for broader legacy dev-tooling nodes that were not open alerts on the requested Dependabot dashboard.Implementation details and chronology are recorded in
Engineering/plans/2026-07-31-dependabot-remediation.mdandHistory/dev-diary/2026/2026-07-31-dependabot-tls-remediation.md.Posted by the WFL repo warden (automated maintainer pass).
Summary by CodeRabbit
Bug Fixes
Security
Documentation