diff --git a/CHANGELOG.md b/CHANGELOG.md index 43d97bcf..e7c9ba00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] ### Security +- **MCP file resources are restricted to bounded WFL sources inside the configured + workspace.** `resources/read` now canonicalizes file URIs, rejects traversal and + symlink escapes (including `.wflcfg`), caps returned source at 4 MiB, and no + longer echoes request or response bodies into diagnostic logs. +- Unsupported database URL errors no longer echo the full connection URL, + preventing embedded credentials from being disclosed in diagnostics. - Package filesystem operations now enforce the manifest's package-name rules, reject symlinked cache/install roots and targets, verify canonical directory containment before recursive deletion, and prevent archive extraction through @@ -38,6 +44,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - **Registry login supports an explicit registry address.** `wfl login [registry]` scopes a token to that HTTPS origin, mismatched logins are rejected, and `wfl logout` can recover malformed or incomplete credentials. +- **Cyclic values no longer abort the interpreter during display, diagnostics, + or isolated-module cloning.** List/object formatting now detects cycles and + caps nesting depth, while deep clones preserve cycles and shared references + inside the cloned graph. - **Subprocess policy is enforced on every process launch** (shell path and direct-exec / `with arguments` path). Previously, `shell_execution_mode` and related checks ran only when the engine believed a shell was required, so diff --git a/Cargo.lock b/Cargo.lock index 8df7368b..016739bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4050,6 +4050,7 @@ dependencies = [ "codespan-reporting", "criterion", "dhat", + "encoding_rs", "futures-util", "glob", "hkdf 0.12.4", diff --git a/Cargo.toml b/Cargo.toml index 36697113..131f4700 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,8 @@ regex = "1.13.0" log = "0.4.33" rustyline = "18.0.1" tokio = { version = "1.52.3", features = ["full"] } -reqwest = { version = "0.13.4", features = ["json"] } +reqwest = { version = "0.13.4", features = ["json", "stream"] } +encoding_rs = "0.8.35" # sqlx 0.9 split the old `runtime-tokio-rustls` feature into a separate runtime # 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"] } diff --git a/Dev diary/2026-07-16-cycle-safe-values.md b/Dev diary/2026-07-16-cycle-safe-values.md new file mode 100644 index 00000000..3c842712 --- /dev/null +++ b/Dev diary/2026-07-16-cycle-safe-values.md @@ -0,0 +1,41 @@ +# Dev Diary — Cycle-safe values (2026-07-16) + +## Context + +Lists and objects are reference-counted mutable values, so valid WFL can build +self-referential and mutually recursive graphs. Value equality already handled +those graphs, but `Display`, `Debug`, and the deep clone used for module +isolation still traversed them recursively without cycle detection. Displaying +a cyclic value (or including one in a diagnostic) could therefore exhaust the +native stack, and cloning one could do the same during isolated lookup. + +## What changed + +- `Display` and `Debug` now carry per-format traversal state. A container that + reappears on the active path renders as ``; shared acyclic values still + render normally each time they appear. +- Formatting stops after 64 nested containers and renders ``, keeping + very deep acyclic graphs comfortably below the native stack limit. +- Formatting uses `try_borrow`, so an incidental outstanding mutable borrow is + rendered as a marker instead of causing a `RefCell` panic. +- `Value::deep_clone` now memoizes list, object, and container-instance + placeholders before cloning their contents. Cycles point into the cloned + graph, shared references remain shared within that graph, and the clone stays + isolated from the source. +- Container parent links are cloned through the same memo instead of retaining + a reference into the source graph. + +## Compatibility + +Acyclic values below the depth limit retain their existing display and debug +forms. Only values that previously recursed indefinitely, exceeded the new +nesting guard, or were formatted while mutably borrowed receive marker text. + +## Tests + +- Rust-level self-cycle and list/object mutual-cycle formatting regressions. +- Deep-clone assertions for source isolation, back-reference preservation, and + shared identity. +- A depth-bound regression for acyclic nesting. +- An interpreter regression that constructs a self-referential list with WFL's + `push` statement and displays it as `[]` without aborting. diff --git a/Docs/04-advanced-features/file-io.md b/Docs/04-advanced-features/file-io.md index 2f246d28..a75a52fb 100644 --- a/Docs/04-advanced-features/file-io.md +++ b/Docs/04-advanced-features/file-io.md @@ -213,7 +213,12 @@ close file > [`reserved-keywords.md`](../reference/reserved-keywords.md) for the full list > and the always-reserved vs. contextual distinction. -Binary reads and writes are capped at 50 MB per operation as a safety limit. +Text and binary reads are capped at 50 MiB per operation by default. The cap is +enforced as bytes stream in (including for special files without a finite +metadata length), and applies to `read N bytes` before its buffer is allocated. +Set `max_file_read_size` in `.wflcfg` to tune the limit; an oversized read raises +a catchable resource-limit error. Binary byte-list writes retain their existing +50 MiB safety check. ## Directory Operations diff --git a/Docs/04-advanced-features/interoperability.md b/Docs/04-advanced-features/interoperability.md index b76bd9a6..a1abd312 100644 --- a/Docs/04-advanced-features/interoperability.md +++ b/Docs/04-advanced-features/interoperability.md @@ -92,6 +92,15 @@ Non-2xx statuses are not errors — check `resp.ok` or `resp.status` yourself. Network failures (DNS, connection refused) still raise errors you can `try`/`catch`. +Outbound responses are streamed and decoded into a bounded buffer. The +`web_server_max_response_size` setting (64 MiB by default) limits the response +body for `read content` and `read response`, both as received and after text +decoding. The limit includes chunked responses with no declared length. Outside +a `main loop`, the connection and body read share the script's remaining +`timeout_seconds`; inside a lifetime-exempt `main loop`, each request gets a +fresh timeout of that duration. Cooperative cancellation also interrupts a +request that is waiting on the remote peer. + **Note:** inside an `open url` statement the words `method`, `headers`, and `body` introduce clauses, so use different variable names there (e.g. `request_headers`, `payload`). diff --git a/Docs/04-advanced-features/subprocess-execution.md b/Docs/04-advanced-features/subprocess-execution.md index e542702f..900d4679 100644 --- a/Docs/04-advanced-features/subprocess-execution.md +++ b/Docs/04-advanced-features/subprocess-execution.md @@ -23,6 +23,13 @@ shell_execution_mode = sanitized - Policy applies to **both** the shell form and the `with arguments` form. Passing arguments is safer against injection *after* a program is allowed; it is not a bypass of the policy. +- `allowlist_only` permits direct execution only. Shell chaining, pipes, + redirects, expansion, and other shell features are blocked even when the + first command is listed. +- Name-only allowlist entries do not authorize explicit paths with the same + basename. Allow an executable path explicitly when a script must use one. +- Avoid allowlisting shells and interpreters such as `sh`, `cmd.exe`, + PowerShell, or Python: their ordinary arguments can execute additional code. See [Configuration Reference](../reference/configuration-reference.md#security-settings) for full option details. @@ -133,6 +140,18 @@ wait for read output from process proc as output_data display "Output: " with output_data ``` +Captured stdout and stderr each retain at most `max_buffer_size_bytes` raw +stream bytes (10 MiB by default) for both `execute command` and +`spawn command`. When a command produces more, WFL continues draining the +stream so the child cannot deadlock, retains only its most recent bytes, and +prints a truncation warning. Malformed UTF-8 replacement can make the returned +WFL text larger than the raw-byte count, but only by a bounded factor. +Foreground commands also observe the run's `timeout_seconds` deadline and +cooperative cancellation through both process execution and pipe draining; WFL +terminates and reaps a child that stalls past either one. A long-lived +`main loop` is exempt from the run-wide deadline, but each foreground command +inside it still receives a fresh `timeout_seconds` window. + ## Executing WFL Files In-Process `execute command` starts a separate program. To run another **WFL file** diff --git a/Docs/reference/configuration-reference.md b/Docs/reference/configuration-reference.md index 0798d33b..baac7050 100644 --- a/Docs/reference/configuration-reference.md +++ b/Docs/reference/configuration-reference.md @@ -192,7 +192,7 @@ All keys currently loaded from config files, with defaults. |---|---|---|---| | `allow_shell_execution` | bool | `false` | Master switch for all process launches | | `shell_execution_mode` | string | `forbidden` | `forbidden` / `allowlist_only` / `sanitized` / `unrestricted` | -| `allowed_shell_commands` | comma-list | *(empty)* | Program basenames allowed in `allowlist_only` mode | +| `allowed_shell_commands` | comma-list | *(empty)* | Program names or explicit paths allowed in `allowlist_only` mode | | `warn_on_shell_execution` | bool | `true` | Warn whenever a shell command runs | ### Subprocess resources @@ -211,7 +211,7 @@ All keys currently loaded from config files, with defaults. | `web_server_tls_cert_file` | path | *(none)* | Default PEM cert for bare `listen … secured` | | `web_server_tls_key_file` | path | *(none)* | Default PEM key for bare `listen … secured` | | `web_server_max_body_size` | integer ≥ 1 | `1048576` (1 MiB) | Max HTTP request body size (bytes); enforced while streaming (chunked-safe) | -| `web_server_max_response_size` | integer ≥ 1 | `67108864` (64 MiB) | Max HTTP response body size (bytes) | +| `web_server_max_response_size` | integer ≥ 1 | `67108864` (64 MiB) | Max handler or outbound HTTP response body size (bytes) | | `web_server_request_queue_bound` | integer ≥ 1 | `256` | Max queued HTTP requests before shedding with 503 | | `web_server_response_timeout_seconds` | integer ≥ 0 | `300` | Seconds to await a handler before shedding with 504; `0` disables | | `web_socket_queue_bound` | integer ≥ 1 | `1024` | Max queued frames/events per WebSocket channel before shedding | @@ -235,6 +235,7 @@ clean, catchable error instead of a crash or unbounded memory growth. | `max_pattern_steps` | integer ≥ 1 | `5000000` | Max pattern-matching transitions per match (ReDoS guard) | | `max_pattern_states` | integer ≥ 1 | `10000` | Max simultaneously-active pattern states per match | | `max_source_size` | integer ≥ 1 | `67108864` (64 MiB) | Max WFL source-file size (bytes) | +| `max_file_read_size` | integer ≥ 1 | `52428800` (50 MiB) | Max bytes buffered by one text or binary file read | The wall-clock deadline (`timeout_seconds`), request body/response ceilings, and the HTTP/WebSocket queue and connection bounds above are all part of the same @@ -248,7 +249,11 @@ budget. #### `timeout_seconds` -Maximum execution time for a WFL script in seconds. The script terminates if it exceeds this limit. +Maximum execution time for a WFL script in seconds. Outside a `main loop`, an +outbound `open url` request (connection, headers, and response body) consumes +the run's remaining time. A `main loop` remains exempt from the lifetime limit, +but each outbound request inside it gets this duration as a fresh finite timeout +so a stalled remote peer cannot wedge the server indefinitely. - **Type:** Integer (minimum: 1) - **Default:** `60` @@ -389,7 +394,7 @@ Applied to every launch, not only shell metacharacter forms. - **Default:** `forbidden` - **Options:** - `forbidden` — no process execution allowed (most secure) - - `allowlist_only` — only programs whose basename is in `allowed_shell_commands` may run + - `allowlist_only` — only direct-exec programs in `allowed_shell_commands` may run; shell features are rejected - `sanitized` — any program may run; shell features produce warnings - `unrestricted` — any program may run with shell; not recommended for production - **Example:** `shell_execution_mode = allowlist_only` @@ -411,9 +416,19 @@ allowed_shell_commands = echo, ls, git #### `allowed_shell_commands` -Comma-separated list of allowed **program basenames** when using -`allowlist_only` mode. Matching uses the basename of the program path -(`/bin/echo` matches `echo`). On Windows, comparison is case-insensitive. +Comma-separated list of allowed program names or explicit executable paths when +using `allowlist_only` mode. A name such as `echo` authorizes only a name-only +invocation resolved through the host process's `PATH`; it does not authorize +`./echo`, `/tmp/echo`, or another caller-selected path with the same basename. +Path-bearing commands require a path-bearing allowlist entry resolving to the +same executable. On Windows, comparison is case-insensitive. + +`allowlist_only` never invokes a shell. Commands containing pipes, redirects, +expansion, command chaining, or other shell features are rejected even when +their first program is allowlisted. Pass data through `with arguments`; opt in +to `sanitized` or `unrestricted` only when shell syntax is genuinely required. +Do not allowlist a shell or interpreter (`sh`, `cmd.exe`, PowerShell, Python, +and similar) unless you intend its arguments to be able to execute code. - **Type:** Comma-separated strings - **Default:** *(empty)* @@ -439,7 +454,13 @@ Maximum number of subprocesses that can run simultaneously. #### `max_buffer_size_bytes` -Maximum size of output buffers for subprocess stdout/stderr, in bytes. +Maximum number of raw stream bytes retained in each stdout/stderr output +buffer. This limit applies to both foreground `execute command` capture and +background `spawn command` capture. If a stream exceeds the limit, WFL drains +it without growing memory, keeps the most recent bytes, and emits a truncation +warning. A value of `0` discards all captured output. Converting malformed +UTF-8 to WFL text may expand the returned text, but remains a bounded multiple +of this raw-byte ceiling. - **Type:** Integer - **Default:** `10485760` (10 MiB) @@ -524,7 +545,12 @@ Because request handlers run one at a time (see [Web Servers → Limitations](.. #### `web_server_max_response_size` -Maximum HTTP response body a handler may `respond with`, in bytes. A larger response is refused (the handler gets a runtime error) rather than streaming an unbounded payload to the client. +Maximum HTTP response body size, in bytes, for both directions: content a +handler may `respond with`, and content an outbound `open url` statement may +read. A larger handler response is refused; a larger outbound response is +stopped when either its received bytes or decoded UTF-8 text reaches this +limit. This applies even when the remote server uses chunked transfer encoding +or omits `Content-Length`. - **Type:** Integer (bytes, at least 1) - **Default:** `67108864` (64 MiB) @@ -641,6 +667,18 @@ Maximum size, in bytes, of a WFL source file. A larger file is refused before it - **Default:** `67108864` (64 MiB) - **Example:** `max_source_size = 1048576` # 1 MiB +#### `max_file_read_size` + +Maximum bytes one `read content`, `read binary`, or `read N bytes` operation may +buffer. The ceiling is enforced while the file is read, so streams and special +files whose metadata has no useful length cannot grow memory without bound. A +larger read fails with a catchable resource-limit error; exact-limit reads are +accepted. + +- **Type:** Integer (bytes, at least 1) +- **Default:** `52428800` (50 MiB) +- **Example:** `max_file_read_size = 10485760` # 10 MiB + Each of these positive-integer keys rejects `0` and non-numeric values, keeping the default and logging a warning. --- diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index c9559aed..b7d56ed7 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -3284,6 +3284,7 @@ dependencies = [ "bytes", "chrono", "codespan-reporting", + "encoding_rs", "futures-util", "glob", "hkdf 0.12.4", diff --git a/src/config.rs b/src/config.rs index 66340afa..71d6609d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -53,9 +53,10 @@ pub struct WflConfig { /// server sheds new requests with a 503 instead of growing memory without /// bound. Default 256; must be at least 1. pub web_server_request_queue_bound: usize, - /// Maximum HTTP response body size in bytes. A handler that tries to send a - /// larger body is refused with a 500 rather than streaming an unbounded - /// payload. Feeds `ExecutionBudget`. Default 64 MiB. + /// Maximum HTTP response body size in bytes, for both handler responses and + /// bodies read by outbound `open url` statements. A larger body is refused + /// rather than buffered/streamed without bound. Feeds `ExecutionBudget`. + /// Default 64 MiB. pub web_server_max_response_size: usize, /// Maximum seconds the transport waits for a handler to answer an accepted /// HTTP request before shedding it with 504 and releasing its in-flight @@ -84,6 +85,9 @@ pub struct WflConfig { /// Maximum WFL source-file size in bytes. Feeds `ExecutionBudget`. /// Default 64 MiB. pub max_source_size: usize, + /// Maximum bytes a single text or binary file-read operation may buffer. + /// Feeds `ExecutionBudget`. Default 50 MiB. + pub max_file_read_size: usize, /// Maximum queued frames/events per WebSocket channel before shedding. /// Feeds `ExecutionBudget`. Default 1024; must be at least 1. pub web_socket_queue_bound: usize, @@ -198,6 +202,9 @@ impl Default for WflConfig { max_pattern_steps: 5_000_000, max_pattern_states: 10_000, max_source_size: 64 * 1024 * 1024, + // Preserve the documented per-operation binary-read ceiling and + // extend the same OOM protection to text reads. + max_file_read_size: 50 * 1024 * 1024, web_socket_queue_bound: 1_024, web_socket_max_connections: 1_024, web_socket_max_message_size: 1_048_576, @@ -858,6 +865,12 @@ fn parse_config_text(config: &mut WflConfig, text: &str, file: &Path) { "max_source_size" => { set_positive_usize(&mut config.max_source_size, "max_source_size", value, file) } + "max_file_read_size" => set_positive_usize( + &mut config.max_file_read_size, + "max_file_read_size", + value, + file, + ), "web_socket_queue_bound" => set_positive_usize( &mut config.web_socket_queue_bound, "web_socket_queue_bound", diff --git a/src/exec/budget.rs b/src/exec/budget.rs index 0498768c..8f5a4eea 100644 --- a/src/exec/budget.rs +++ b/src/exec/budget.rs @@ -27,8 +27,9 @@ //! * **Pattern transitions and active states** — //! [`ExecutionBudget::check_pattern_steps`] / //! [`ExecutionBudget::check_pattern_states`]. -//! * **Source, body, and response bytes** — +//! * **Source, file-read, body, and response bytes** — //! [`ExecutionBudget::check_source_bytes`], +//! [`ExecutionBudget::check_file_read_bytes`], //! [`ExecutionBudget::check_request_body_bytes`], //! [`ExecutionBudget::check_response_bytes`]. //! * **Pending HTTP requests** — [`ExecutionBudget::max_pending_requests`]. @@ -97,10 +98,14 @@ pub struct BudgetLimits { /// Maximum WFL source-file size in bytes. Mapped from `.wflcfg` /// `max_source_size`. pub max_source_bytes: usize, + /// Maximum bytes buffered by one text or binary file read. Mapped from + /// `.wflcfg` `max_file_read_size`. + pub max_file_read_bytes: usize, /// Maximum accepted HTTP request body size in bytes. Mapped from `.wflcfg` /// `web_server_max_body_size`. pub max_request_body_bytes: usize, - /// Maximum HTTP response body size in bytes. Mapped from `.wflcfg` + /// Maximum HTTP response body size in bytes, for both handler responses and + /// bodies read by outbound `open url` statements. Mapped from `.wflcfg` /// `web_server_max_response_size`. pub max_response_bytes: usize, /// Maximum accepted-but-unhandled HTTP requests held in the transport @@ -152,6 +157,9 @@ impl Default for BudgetLimits { max_pattern_states: 10_000, // Source size was unchecked before; 64 MiB clears any real program. max_source_bytes: 64 * 1024 * 1024, + // Preserve the file-I/O guide's existing 50 MiB per-read policy, + // now enforced for both text and binary reads while streaming. + max_file_read_bytes: 50 * 1024 * 1024, // Preserves the previous `web_server_max_body_size` default (1 MiB). max_request_body_bytes: 1_048_576, // Response size was unchecked before; 64 MiB clears any real payload. @@ -191,6 +199,7 @@ impl BudgetLimits { max_pattern_steps: config.max_pattern_steps, max_pattern_states: config.max_pattern_states, max_source_bytes: config.max_source_size, + max_file_read_bytes: config.max_file_read_size, max_request_body_bytes: config.web_server_max_body_size, max_response_bytes: config.web_server_max_response_size, max_pending_requests: config.web_server_request_queue_bound.max(1), @@ -219,6 +228,7 @@ impl BudgetLimits { max_pattern_steps: 5_000_000, max_pattern_states: 10_000, max_source_bytes: usize::MAX, + max_file_read_bytes: usize::MAX, max_request_body_bytes: usize::MAX, max_response_bytes: usize::MAX, max_pending_requests: usize::MAX, @@ -253,6 +263,8 @@ pub enum BudgetExceeded { PatternStates { limit: usize }, /// A source file exceeded the byte ceiling. SourceBytes { limit: usize, actual: usize }, + /// A text or binary file read exceeded its per-operation byte ceiling. + FileReadBytes { limit: usize, actual: usize }, /// An HTTP request body exceeded the byte ceiling. RequestBodyBytes { limit: usize, actual: usize }, /// An HTTP response body exceeded the byte ceiling. @@ -294,6 +306,9 @@ impl BudgetExceeded { BudgetExceeded::SourceBytes { limit, actual } => { format!("Source file too large: {actual} bytes (limit: {limit} bytes)") } + BudgetExceeded::FileReadBytes { limit, actual } => { + format!("File read too large: {actual} bytes (limit: {limit} bytes)") + } BudgetExceeded::RequestBodyBytes { limit, actual } => { format!("Request body too large: {actual} bytes (limit: {limit} bytes)") } @@ -612,6 +627,23 @@ impl ExecutionBudget { } } + /// The per-operation ceiling for buffered text and binary file reads. + pub fn max_file_read_bytes(&self) -> usize { + self.limits.max_file_read_bytes + } + + /// Fail if a text or binary file read exceeds its byte ceiling. + pub fn check_file_read_bytes(&self, len: usize) -> Result<(), BudgetExceeded> { + if len > self.limits.max_file_read_bytes { + Err(BudgetExceeded::FileReadBytes { + limit: self.limits.max_file_read_bytes, + actual: len, + }) + } else { + Ok(()) + } + } + /// Fail if an HTTP request body exceeds the byte ceiling. pub fn check_request_body_bytes(&self, len: usize) -> Result<(), BudgetExceeded> { if len > self.limits.max_request_body_bytes { @@ -1063,6 +1095,7 @@ mod tests { max_pattern_steps: 5, max_pattern_states: 4, max_source_bytes: 8, + max_file_read_bytes: 8, max_request_body_bytes: 8, max_response_bytes: 8, max_pending_requests: 2, @@ -1272,6 +1305,14 @@ mod tests { actual: 9 }) ); + assert!(budget.check_file_read_bytes(8).is_ok()); + assert_eq!( + budget.check_file_read_bytes(9), + Err(BudgetExceeded::FileReadBytes { + limit: 8, + actual: 9 + }) + ); assert_eq!( budget.check_request_body_bytes(100), Err(BudgetExceeded::RequestBodyBytes { @@ -1339,12 +1380,14 @@ mod tests { timeout_seconds: 42, web_server_max_body_size: 4096, web_server_request_queue_bound: 7, + max_file_read_size: 123, ..Default::default() }; let limits = BudgetLimits::from_config(&config); assert_eq!(limits.max_duration, Some(Duration::from_secs(42))); assert_eq!(limits.max_request_body_bytes, 4096); assert_eq!(limits.max_pending_requests, 7); + assert_eq!(limits.max_file_read_bytes, 123); } #[test] diff --git a/src/interpreter/bounded_buffer.rs b/src/interpreter/bounded_buffer.rs index fe6cccb1..652704f5 100644 --- a/src/interpreter/bounded_buffer.rs +++ b/src/interpreter/bounded_buffer.rs @@ -23,12 +23,20 @@ impl BoundedBuffer { /// Push bytes into the buffer. If the buffer is full, oldest bytes are dropped. pub fn push(&mut self, bytes: &[u8]) { - self.bytes_written += bytes.len(); + self.bytes_written = self.bytes_written.saturating_add(bytes.len()); + + // A zero-sized buffer means "discard all output". Without this special + // case the loop below would pop from an empty deque and then push one + // byte, violating the configured ceiling. + if self.max_size == 0 { + self.bytes_dropped = self.bytes_dropped.saturating_add(bytes.len()); + return; + } for &byte in bytes { if self.data.len() >= self.max_size { self.data.pop_front(); - self.bytes_dropped += 1; + self.bytes_dropped = self.bytes_dropped.saturating_add(1); } self.data.push_back(byte); } @@ -156,4 +164,15 @@ mod tests { assert_eq!(data.len(), 100); assert!(data.iter().all(|&b| b == b'X')); } + + #[test] + fn test_zero_sized_buffer_discards_everything() { + let mut buf = BoundedBuffer::new(0); + + buf.push(b"must not be retained"); + + assert!(buf.is_empty()); + assert_eq!(buf.stats().bytes_written, 20); + assert_eq!(buf.stats().bytes_dropped, 20); + } } diff --git a/src/interpreter/command_sanitizer.rs b/src/interpreter/command_sanitizer.rs index 55c6db27..17e3f714 100644 --- a/src/interpreter/command_sanitizer.rs +++ b/src/interpreter/command_sanitizer.rs @@ -184,8 +184,6 @@ impl CommandSanitizer { }); } - let program_base = Self::program_basename(program); - match self.config.shell_execution_mode { ShellExecutionMode::Forbidden => Ok(ValidationResult::Blocked { reason: "Subprocess execution is disabled by security policy \ @@ -193,20 +191,28 @@ impl CommandSanitizer { .to_string(), }), ShellExecutionMode::AllowlistOnly => { - if self.is_program_allowlisted(&program_base) { - if needs_shell { - Ok(ValidationResult::RequiresShell { - reason: "Command is allowlisted".to_string(), - warnings: vec!["Using shell execution (allowlisted)".to_string()], - }) - } else { - Ok(ValidationResult::Safe) - } + // An allowlist can authorize one executable, but it cannot + // safely authorize an entire shell command line. If shell + // parsing is allowed here, a command such as + // `echo safe; unlisted-program` passes the `echo` check and the + // shell executes both commands. Require the argv/direct-exec + // form in this mode; callers that intentionally need pipes, + // redirects, expansion, or chaining must opt into `sanitized` + // or `unrestricted` explicitly. + if needs_shell { + return Ok(ValidationResult::Blocked { + reason: "Shell features are not permitted in allowlist_only mode; use the direct-exec form with an explicit argument list" + .to_string(), + }); + } + + if self.is_program_allowlisted(program) { + Ok(ValidationResult::Safe) } else { Ok(ValidationResult::Blocked { reason: format!( "Program '{}' is not in the allowlist (allowed_shell_commands)", - program_base + program ), }) } @@ -251,16 +257,51 @@ impl CommandSanitizer { /// Check if a program (or command string) is in the allowlist pub fn is_allowlisted(&self, command: &str) -> bool { - let base_command = Self::program_basename(&self.get_command_base(command)); - self.is_program_allowlisted(&base_command) + if Self::contains_shell_metacharacters(command) { + return false; + } + let program = Self::parse_command(command) + .map(|(program, _)| program) + .unwrap_or_else(|_| self.get_command_base(command)); + self.is_program_allowlisted(&program) } - fn is_program_allowlisted(&self, program_base: &str) -> bool { + fn is_program_allowlisted(&self, program: &str) -> bool { + let program_has_path = Self::has_path_syntax(program); self.config.allowed_shell_commands.iter().any(|allowed| { + let allowed_has_path = Self::has_path_syntax(allowed); + + // A name-only entry delegates resolution to the trusted process + // environment's PATH. It must not also authorize a caller-selected + // executable at `./name`, `/tmp/name`, or another explicit path. + if program_has_path != allowed_has_path { + return false; + } + if program_has_path { + let Ok(program_path) = std::fs::canonicalize(program) else { + return false; + }; + let Ok(allowed_path) = std::fs::canonicalize(allowed) else { + return false; + }; + + #[cfg(windows)] + { + return program_path + .to_string_lossy() + .eq_ignore_ascii_case(&allowed_path.to_string_lossy()); + } + #[cfg(not(windows))] + { + return program_path == allowed_path; + } + } + + let program_base = Self::program_basename(program); let allowed_base = Self::program_basename(allowed); #[cfg(windows)] { - allowed_base.eq_ignore_ascii_case(program_base) + allowed_base.eq_ignore_ascii_case(&program_base) } #[cfg(not(windows))] { @@ -269,6 +310,11 @@ impl CommandSanitizer { }) } + fn has_path_syntax(program: &str) -> bool { + let trimmed = program.trim().trim_matches('"').trim_matches('\''); + trimmed.contains('/') || trimmed.contains('\\') || trimmed.as_bytes().get(1) == Some(&b':') + } + /// Extract the first whitespace-separated token from a command string fn get_command_base(&self, command: &str) -> String { command.split_whitespace().next().unwrap_or("").to_string() @@ -564,11 +610,68 @@ mod tests { .unwrap(); assert!(matches!(blocked, ValidationResult::Blocked { .. })); - // Path basename matching - let path_ok = sanitizer + // A name-only entry must not authorize a caller-selected path merely + // because the basename is the same. + let path_blocked = sanitizer .authorize_process_execution("/bin/echo", false, "/bin/echo") .unwrap(); - assert_eq!(path_ok, ValidationResult::Safe); + assert!(matches!(path_blocked, ValidationResult::Blocked { .. })); + } + + #[test] + fn test_allowlist_path_requires_the_same_executable() { + let current_executable = std::env::current_exe().expect("current test executable"); + let current_executable = current_executable + .to_str() + .expect("test executable path is UTF-8") + .to_string(); + let config = WflConfig { + allow_shell_execution: true, + shell_execution_mode: ShellExecutionMode::AllowlistOnly, + allowed_shell_commands: vec![current_executable.clone()], + ..Default::default() + }; + let sanitizer = CommandSanitizer::new(Arc::new(config)); + + assert_eq!( + sanitizer + .authorize_process_execution(¤t_executable, false, ¤t_executable) + .unwrap(), + ValidationResult::Safe + ); + let substituted = format!( + "{}/{}", + std::env::temp_dir().display(), + CommandSanitizer::program_basename(¤t_executable) + ); + let result = sanitizer + .authorize_process_execution(&substituted, false, &substituted) + .unwrap(); + assert!(matches!(result, ValidationResult::Blocked { .. })); + } + + #[test] + fn test_allowlist_only_rejects_shell_features_for_allowlisted_program() { + let config = WflConfig { + allow_shell_execution: true, + shell_execution_mode: ShellExecutionMode::AllowlistOnly, + allowed_shell_commands: vec!["echo".to_string()], + ..Default::default() + }; + let sanitizer = CommandSanitizer::new(Arc::new(config)); + + for command in [ + "echo safe; unlisted-program", + "echo safe | unlisted-program", + "echo $(unlisted-program)", + "echo safe > output.txt", + ] { + let result = sanitizer.validate_command(command).unwrap(); + assert!( + matches!(result, ValidationResult::Blocked { .. }), + "allowlist_only must reject shell-backed command {command:?}, got {result:?}" + ); + } } #[test] @@ -584,6 +687,7 @@ mod tests { assert!(sanitizer.is_allowlisted("echo hello")); assert!(sanitizer.is_allowlisted("ls -la")); assert!(!sanitizer.is_allowlisted("rm -rf /")); + assert!(!sanitizer.is_allowlisted("echo safe; rm -rf /")); } #[test] diff --git a/src/interpreter/database.rs b/src/interpreter/database.rs index 8d96fc60..482feed9 100644 --- a/src/interpreter/database.rs +++ b/src/interpreter/database.rs @@ -123,9 +123,10 @@ pub async fn connect(url: &str) -> Result { .map(DbPool::MySql) .map_err(|e| format!("Failed to connect to MariaDB/MySQL database: {e}")) } else { - Err(format!( - "Unsupported database URL '{url}'. Supported schemes: sqlite://, postgres://, postgresql://, mysql://, mariadb://" - )) + Err( + "Unsupported database URL scheme. Supported schemes: sqlite://, postgres://, postgresql://, mysql://, mariadb://" + .to_string(), + ) } } @@ -419,6 +420,21 @@ row_to_value!(mysql_row_to_value, MySqlRow, mysql_int); mod tests { use super::*; + #[tokio::test] + async fn unsupported_database_url_does_not_echo_credentials() { + const SECRET: &str = "WFL_TEST_SECRET_unsupported_password_d5e0ec"; + let url = format!("oracle://wfl:{SECRET}@database.example/app"); + + let error = match connect(&url).await { + Ok(_) => panic!("unsupported database URL unexpectedly connected"), + Err(error) => error, + }; + + assert!(error.contains("Unsupported database URL scheme")); + assert!(!error.contains(SECRET)); + assert!(!error.contains(url.as_str())); + } + #[test] fn whole_numbers_bind_as_integers() { assert!(matches!( diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 8eb53a69..77aef4a6 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1022,9 +1022,9 @@ fn expr_type(expr: &Expression) -> String { } } -use tokio::io::AsyncReadExt; use tokio::io::AsyncSeekExt; use tokio::io::AsyncWriteExt; +use tokio::io::{AsyncRead, AsyncReadExt}; use tokio::sync::Mutex; // use self::value::FutureValue; @@ -1133,6 +1133,194 @@ pub struct ProcessHandle { stderr_buffer: Arc>, } +/// Failure from a foreground `execute command`. Budget breaches stay typed so +/// the interpreter can preserve timeout/resource-limit error kinds instead of +/// flattening them into a generic subprocess error. +#[derive(Debug)] +enum ExecuteCommandError { + Budget(BudgetExceeded), + Timeout { seconds: u64 }, + Other(String), +} + +impl std::fmt::Display for ExecuteCommandError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Budget(exceeded) => std::fmt::Display::fmt(exceeded, formatter), + Self::Timeout { seconds } => { + write!( + formatter, + "Subprocess execution exceeded timeout ({seconds}s)" + ) + } + Self::Other(message) => formatter.write_str(message), + } + } +} + +/// Bytes retained from one subprocess stream plus the amount discarded after +/// the configured per-stream ceiling was reached. +struct CapturedProcessStream { + bytes: Vec, + bytes_dropped: usize, +} + +/// Abort detached pipe readers on every non-success path, including external +/// cancellation that drops the whole `execute_command` future before its +/// explicit interruption branch can run. +struct ProcessCaptureAbortGuard { + handles: [tokio::task::AbortHandle; 2], + armed: bool, +} + +impl ProcessCaptureAbortGuard { + fn new(stdout: tokio::task::AbortHandle, stderr: tokio::task::AbortHandle) -> Self { + Self { + handles: [stdout, stderr], + armed: true, + } + } + + fn abort(&mut self) { + if self.armed { + for handle in &self.handles { + handle.abort(); + } + self.armed = false; + } + } + + fn disarm(&mut self) { + self.armed = false; + } +} + +impl Drop for ProcessCaptureAbortGuard { + fn drop(&mut self) { + self.abort(); + } +} + +/// Which wall-clock rule applies to one foreground command. Long-lived +/// `main loop`s remain exempt from the run-wide deadline, but each blocking +/// subprocess operation still receives a fresh configured timeout. +#[derive(Debug, Clone, Copy)] +enum ForegroundCommandDeadline { + None, + Execution, + MainLoop { started: Instant, timeout: Duration }, +} + +const SUBPROCESS_BUDGET_POLL_INTERVAL: Duration = Duration::from_millis(10); + +/// Drain one subprocess pipe without ever retaining more than `max_size` +/// bytes. Keeping the most recent bytes matches background-process capture. +async fn capture_process_stream( + mut stream: R, + max_size: usize, +) -> io::Result +where + R: tokio::io::AsyncRead + Unpin, +{ + let mut buffer = bounded_buffer::BoundedBuffer::new(max_size); + let mut chunk = [0_u8; 8192]; + + loop { + let read = stream.read(&mut chunk).await?; + if read == 0 { + break; + } + buffer.push(&chunk[..read]); + } + + let bytes_dropped = buffer.stats().bytes_dropped; + Ok(CapturedProcessStream { + bytes: buffer.read_all(), + bytes_dropped, + }) +} + +/// Select the finite wall-clock rule for one foreground execution and reject a +/// launch when its shared budget is already expired or cancelled. +fn foreground_command_deadline( + budget: &ExecutionBudget, + configured_timeout: Duration, +) -> Result { + budget + .check_cancelled() + .map_err(ExecuteCommandError::Budget)?; + + if budget.is_deadline_exempt() { + return Ok(ForegroundCommandDeadline::MainLoop { + started: Instant::now(), + timeout: configured_timeout, + }); + } + + budget + .check_deadline() + .map_err(ExecuteCommandError::Budget)?; + if budget.limits().max_duration.is_some() { + Ok(ForegroundCommandDeadline::Execution) + } else { + Ok(ForegroundCommandDeadline::None) + } +} + +/// Wait until cancellation or the applicable deadline interrupts the complete +/// foreground operation. This monitor remains active after the direct child +/// exits because descendants can inherit its pipes and withhold EOF forever. +async fn foreground_command_interrupt( + budget: &ExecutionBudget, + deadline: ForegroundCommandDeadline, +) -> ExecuteCommandError { + loop { + if let Err(exceeded) = budget.check_cancelled() { + return ExecuteCommandError::Budget(exceeded); + } + + match deadline { + ForegroundCommandDeadline::None => {} + ForegroundCommandDeadline::Execution => { + if let Err(exceeded) = budget.check_deadline() { + return ExecuteCommandError::Budget(exceeded); + } + } + ForegroundCommandDeadline::MainLoop { started, timeout } => { + if started.elapsed() >= timeout { + return ExecuteCommandError::Timeout { + seconds: timeout.as_secs(), + }; + } + } + } + + tokio::time::sleep(SUBPROCESS_BUDGET_POLL_INTERVAL).await; + } +} + +/// Kill and reap the direct child unless it has already completed. A second +/// status check handles the normal race where it exits between inspection and +/// the kill request. +async fn terminate_foreground_child(child: &mut tokio::process::Child) -> Result<(), String> { + match child.try_wait() { + Ok(Some(_)) => return Ok(()), + Ok(None) => {} + Err(error) => return Err(format!("failed to inspect subprocess: {error}")), + } + + match child.kill().await { + Ok(()) => Ok(()), + Err(kill_error) => match child.try_wait() { + Ok(Some(_)) => Ok(()), + Ok(None) => Err(format!("failed to terminate subprocess: {kill_error}")), + Err(wait_error) => Err(format!( + "failed to terminate subprocess: {kill_error}; status check failed: {wait_error}" + )), + }, + } +} + #[allow(dead_code)] pub struct IoClient { http_client: reqwest::Client, @@ -1145,6 +1333,90 @@ pub struct IoClient { config: Arc, } +/// Errors raised while an outbound HTTP request is in flight. +/// +/// Budget failures stay structured until the interpreter can attach source +/// location and the appropriate [`ErrorKind`]. Keeping them out of strings is +/// important for `try`/`when` handlers that distinguish timeouts from resource +/// limits. +#[derive(Debug)] +enum HttpClientError { + Request(String), + Budget(BudgetExceeded), + Timeout { seconds: u64 }, +} + +impl From for HttpClientError { + fn from(exceeded: BudgetExceeded) -> Self { + Self::Budget(exceeded) + } +} + +/// Which finite wall-clock limit applies to an outbound request. +#[derive(Debug, Clone, Copy)] +enum OutboundHttpDeadline { + /// An explicitly-unlimited non-server run has no wall-clock deadline. + None, + /// Outside a `main loop`, an outbound request consumes the run's remaining + /// global execution time. + Execution { + remaining: Duration, + limit_secs: u64, + }, + /// A `main loop` is lifetime-exempt, but each individual outbound request + /// still gets a fresh finite timeout so a stalled peer cannot wedge the + /// server forever. + MainLoop { duration: Duration }, +} + +/// Polling is used because cooperative cancellation is represented by an +/// atomic flag rather than a notification primitive. This interval bounds how +/// quickly an in-flight socket operation observes `ExecutionBudget::cancel()`. +const HTTP_CANCELLATION_POLL_INTERVAL: Duration = Duration::from_millis(10); + +#[derive(Debug)] +enum FileReadError { + Io(String), + Budget(BudgetExceeded), +} + +impl std::fmt::Display for FileReadError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Io(message) => f.write_str(message), + Self::Budget(exceeded) => std::fmt::Display::fmt(exceeded, f), + } + } +} + +/// Buffer one file read under the run's file-byte ceiling. Reading through a +/// `Take` capped at `limit + 1` is intentional: metadata is not reliable for +/// special files and streams (for example `/dev/zero`), so the limit must be +/// enforced while bytes arrive rather than after an unbounded `read_to_end`. +async fn read_to_end_capped( + reader: &mut R, + budget: &ExecutionBudget, + operation: &str, +) -> Result, FileReadError> +where + R: AsyncRead + Unpin, +{ + let limit = budget.max_file_read_bytes(); + let probe_size = limit.saturating_add(1); + let probe_size_u64 = u64::try_from(probe_size).unwrap_or(u64::MAX); + let mut bytes = Vec::with_capacity(limit.min(8 * 1024)); + let mut bounded = reader.take(probe_size_u64); + bounded + .read_to_end(&mut bytes) + .await + .map_err(|e| FileReadError::Io(format!("{operation}: {e}")))?; + + budget + .check_file_read_bytes(bytes.len()) + .map_err(FileReadError::Budget)?; + Ok(bytes) +} + impl IoClient { fn new(config: Arc) -> Self { Self { @@ -1197,31 +1469,32 @@ impl IoClient { } #[allow(dead_code)] - async fn http_get(&self, url: &str) -> Result { - match self.http_client.get(url).send().await { - Ok(response) => match response.text().await { - Ok(text) => Ok(text), - Err(e) => Err(format!("Failed to read response body: {e}")), - }, - Err(e) => Err(format!("Failed to send HTTP GET request: {e}")), - } + async fn http_get( + &self, + url: &str, + budget: Arc, + ) -> Result { + let (_, _, body) = self + .send_http_request(self.http_client.get(url), "GET", budget) + .await?; + Ok(body) } #[allow(dead_code)] - async fn http_post(&self, url: &str, data: &str) -> Result { - match self - .http_client - .post(url) - .body(data.to_string()) - .send() - .await - { - Ok(response) => match response.text().await { - Ok(text) => Ok(text), - Err(e) => Err(format!("Failed to read response body: {e}")), - }, - Err(e) => Err(format!("Failed to send HTTP POST request: {e}")), - } + async fn http_post( + &self, + url: &str, + data: &str, + budget: Arc, + ) -> Result { + let (_, _, body) = self + .send_http_request( + self.http_client.post(url).body(data.to_string()), + "POST", + budget, + ) + .await?; + Ok(body) } /// Perform an HTTP request with an arbitrary method, optional headers, @@ -1233,9 +1506,10 @@ impl IoClient { url: &str, headers: &[(String, String)], body: Option, - ) -> Result<(u16, Vec<(String, String)>, String), String> { + budget: Arc, + ) -> Result<(u16, Vec<(String, String)>, String), HttpClientError> { let parsed_method = reqwest::Method::from_bytes(method.as_bytes()) - .map_err(|_| format!("Invalid HTTP method: {method}"))?; + .map_err(|_| HttpClientError::Request(format!("Invalid HTTP method: {method}")))?; let mut request = self.http_client.request(parsed_method, url); for (name, value) in headers { @@ -1245,28 +1519,244 @@ impl IoClient { request = request.body(body); } - match request.send().await { - Ok(response) => { - let status = response.status().as_u16(); - // Header names are normalized to lowercase for consistent - // access from WFL (e.g. resp.headers["content-type"]), and - // non-UTF8 values are converted lossily instead of dropped. - let response_headers = response - .headers() - .iter() - .map(|(name, value)| { - ( - name.as_str().to_ascii_lowercase(), - String::from_utf8_lossy(value.as_bytes()).into_owned(), - ) - }) - .collect(); - match response.text().await { - Ok(text) => Ok((status, response_headers, text)), - Err(e) => Err(format!("Failed to read response body: {e}")), + self.send_http_request(request, method, budget).await + } + + /// Send a request and consume its body without ever buffering more than the + /// configured response ceiling. The budget passed here is deliberately the + /// interpreter's *live* budget, not construction-time IoClient state: the + /// REPL replaces its budget for every command. + async fn send_http_request( + &self, + request: reqwest::RequestBuilder, + method: &str, + budget: Arc, + ) -> Result<(u16, Vec<(String, String)>, String), HttpClientError> { + let method = method.to_string(); + let operation_budget = Arc::clone(&budget); + let operation = async move { + use futures_util::StreamExt; + + let response = request.send().await.map_err(|e| { + HttpClientError::Request(format!("Failed to send HTTP {method} request: {e}")) + })?; + + let status = response.status().as_u16(); + // Header names are normalized to lowercase for consistent access + // from WFL (e.g. resp.headers["content-type"]), and non-UTF8 + // values are converted lossily instead of dropped. + let response_headers = response + .headers() + .iter() + .map(|(name, value)| { + ( + name.as_str().to_ascii_lowercase(), + String::from_utf8_lossy(value.as_bytes()).into_owned(), + ) + }) + .collect(); + let content_type = response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .map(str::to_owned); + + let max_response_bytes = operation_budget.limits().max_response_bytes; + if let Some(content_length) = response.content_length() { + let max_as_u64 = u64::try_from(max_response_bytes).unwrap_or(u64::MAX); + if content_length > max_as_u64 { + return Err(HttpClientError::Budget(BudgetExceeded::ResponseBytes { + limit: max_response_bytes, + actual: usize::try_from(content_length).unwrap_or(usize::MAX), + })); + } + } + + // Do not retain a full raw byte buffer and then allocate a second, + // potentially larger UTF-8 string. Decode each network chunk into + // bounded scratch space, and enforce the same ceiling on both wire + // bytes and decoded UTF-8 bytes. Invalid UTF-8 alone can expand 3x + // when replaced with U+FFFD. + let initial_capacity = response + .content_length() + .and_then(|len| usize::try_from(len).ok()) + .unwrap_or(0) + .min(max_response_bytes) + .min(64 * 1024); + let encoding = Self::http_text_encoding(content_type.as_deref()); + let mut decoder = encoding.new_decoder(); + let mut body = String::with_capacity(initial_capacity); + let mut wire_bytes = 0_usize; + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| { + HttpClientError::Request(format!("Failed to read response body: {e}")) + })?; + let actual = wire_bytes.saturating_add(chunk.len()); + if chunk.len() > max_response_bytes.saturating_sub(wire_bytes) { + return Err(HttpClientError::Budget(BudgetExceeded::ResponseBytes { + limit: max_response_bytes, + actual, + })); + } + wire_bytes = actual; + Self::decode_http_chunk( + &mut decoder, + &chunk, + false, + &mut body, + max_response_bytes, + )?; + } + Self::decode_http_chunk(&mut decoder, &[], true, &mut body, max_response_bytes)?; + + Ok((status, response_headers, body)) + }; + + // A custom live budget may deliberately have no run-wide deadline. In + // a lifetime-exempt main loop, fall back to the interpreter's + // configured `timeout_seconds` (minimum one second) so the individual + // network operation is still finite and user-configurable. + let configured_timeout = Duration::from_secs(self.config.timeout_seconds.max(1)); + Self::run_http_with_budget(budget, configured_timeout, operation).await + } + + /// Select the response encoding while preserving reqwest's text behavior: + /// honor a declared charset and default to UTF-8. + fn http_text_encoding(content_type: Option<&str>) -> &'static encoding_rs::Encoding { + let charset = content_type.and_then(|value| { + value.split(';').skip(1).find_map(|parameter| { + let (name, value) = parameter.trim().split_once('=')?; + name.trim() + .eq_ignore_ascii_case("charset") + .then(|| value.trim().trim_matches(|ch| ch == '\'' || ch == '"')) + }) + }); + charset + .and_then(|name| encoding_rs::Encoding::for_label(name.as_bytes())) + .unwrap_or(encoding_rs::UTF_8) + } + + /// Incrementally decode one response chunk into caller-owned text. The + /// decoder writes only into fixed scratch storage; the destination reserves + /// exactly the accepted addition before appending, avoiding Vec/String + /// geometric-growth spikes near the configured ceiling. + fn decode_http_chunk( + decoder: &mut encoding_rs::Decoder, + mut input: &[u8], + last: bool, + output: &mut String, + max_response_bytes: usize, + ) -> Result<(), HttpClientError> { + let mut decoded = [0_u8; 8 * 1024]; + loop { + let (result, read, written, _) = decoder.decode_to_utf8(input, &mut decoded, last); + let actual = output.len().saturating_add(written); + if written > max_response_bytes.saturating_sub(output.len()) { + return Err(HttpClientError::Budget(BudgetExceeded::ResponseBytes { + limit: max_response_bytes, + actual, + })); + } + if written > 0 { + output.try_reserve_exact(written).map_err(|error| { + HttpClientError::Request(format!( + "Failed to allocate bounded HTTP response buffer: {error}" + )) + })?; + let text = std::str::from_utf8(&decoded[..written]) + .expect("encoding_rs must emit valid UTF-8"); + output.push_str(text); + } + input = &input[read..]; + + match result { + encoding_rs::CoderResult::InputEmpty => return Ok(()), + encoding_rs::CoderResult::OutputFull => {} + } + } + } + + fn outbound_http_deadline( + budget: &ExecutionBudget, + configured_timeout: Duration, + ) -> Result { + budget.check_cancelled()?; + + if budget.is_deadline_exempt() { + return Ok(OutboundHttpDeadline::MainLoop { + duration: budget.limits().max_duration.unwrap_or(configured_timeout), + }); + } + + let Some(limit) = budget.limits().max_duration else { + return Ok(OutboundHttpDeadline::None); + }; + let Some(remaining) = limit.checked_sub(budget.elapsed()) else { + return Err(HttpClientError::Budget(BudgetExceeded::Deadline { + limit_secs: limit.as_secs(), + })); + }; + if remaining.is_zero() { + return Err(HttpClientError::Budget(BudgetExceeded::Deadline { + limit_secs: limit.as_secs(), + })); + } + Ok(OutboundHttpDeadline::Execution { + remaining, + limit_secs: limit.as_secs(), + }) + } + + /// Race the complete network operation (connect, headers, and streamed + /// body) against both cooperative cancellation and the applicable finite + /// deadline. Dropping reqwest's future closes/cancels the in-flight work. + async fn run_http_with_budget( + budget: Arc, + configured_timeout: Duration, + operation: F, + ) -> Result + where + F: std::future::Future>, + { + let deadline = Self::outbound_http_deadline(&budget, configured_timeout)?; + let timeout_duration = match deadline { + OutboundHttpDeadline::None => None, + OutboundHttpDeadline::Execution { remaining, .. } => Some(remaining), + OutboundHttpDeadline::MainLoop { duration } => Some(duration), + }; + + let cancellation_budget = Arc::clone(&budget); + let cancellation = async move { + loop { + if cancellation_budget.is_cancelled() { + break; } + tokio::time::sleep(HTTP_CANCELLATION_POLL_INTERVAL).await; + } + }; + let timeout = async move { + match timeout_duration { + Some(duration) => tokio::time::sleep(duration).await, + None => std::future::pending::<()>().await, } - Err(e) => Err(format!("Failed to send HTTP {method} request: {e}")), + }; + + tokio::pin!(operation); + tokio::pin!(cancellation); + tokio::pin!(timeout); + tokio::select! { + result = &mut operation => result, + _ = &mut cancellation => Err(HttpClientError::Budget(BudgetExceeded::Cancelled)), + _ = &mut timeout => match deadline { + OutboundHttpDeadline::Execution { limit_secs, .. } => { + Err(HttpClientError::Budget(BudgetExceeded::Deadline { limit_secs })) + } + OutboundHttpDeadline::MainLoop { duration } => { + Err(HttpClientError::Timeout { seconds: duration.as_secs() }) + } + OutboundHttpDeadline::None => unreachable!("disabled timeout cannot complete"), + }, } } @@ -1357,7 +1847,11 @@ impl IoClient { } #[allow(dead_code)] - async fn read_file(&self, handle_id: &str) -> Result { + async fn read_file( + &self, + handle_id: &str, + budget: &ExecutionBudget, + ) -> Result { let mut file_handles = self.file_handles.lock().await; if !file_handles.contains_key(handle_id) { @@ -1366,27 +1860,37 @@ impl IoClient { match self.open_file(handle_id).await { Ok(new_handle) => { // Now read from the new handle - use Box::pin to handle recursion in async fn - let future = Box::pin(self.read_file(&new_handle)); + let future = Box::pin(self.read_file(&new_handle, budget)); let result = future.await; let _ = self.close_file(&new_handle).await; return result; } - Err(e) => return Err(format!("Invalid file handle or path: {handle_id}: {e}")), + Err(e) => { + return Err(FileReadError::Io(format!( + "Invalid file handle or path: {handle_id}: {e}" + ))); + } } } let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { Ok(clone) => clone, - Err(e) => return Err(format!("Failed to clone file handle: {e}")), + Err(e) => { + return Err(FileReadError::Io(format!( + "Failed to clone file handle: {e}" + ))); + } }; drop(file_handles); - let mut contents = String::new(); - match AsyncReadExt::read_to_string(&mut file_clone, &mut contents).await { - Ok(_) => Ok(contents), - Err(e) => Err(format!("Failed to read file: {e}")), - } + let bytes = read_to_end_capped(&mut file_clone, budget, "Failed to read file").await?; + String::from_utf8(bytes).map_err(|e| { + FileReadError::Io(format!( + "Failed to read file: stream did not contain valid UTF-8: {}", + e.utf8_error() + )) + }) } /// Syncs file to disk with Windows-specific error handling. @@ -1486,16 +1990,26 @@ impl IoClient { } } - async fn read_binary(&self, handle_id: &str) -> Result, String> { + async fn read_binary( + &self, + handle_id: &str, + budget: &ExecutionBudget, + ) -> Result, FileReadError> { let mut file_handles = self.file_handles.lock().await; if !file_handles.contains_key(handle_id) { - return Err(format!("Invalid file handle: {handle_id}")); + return Err(FileReadError::Io(format!( + "Invalid file handle: {handle_id}" + ))); } let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { Ok(clone) => clone, - Err(e) => return Err(format!("Failed to clone file handle: {e}")), + Err(e) => { + return Err(FileReadError::Io(format!( + "Failed to clone file handle: {e}" + ))); + } }; drop(file_handles); @@ -1503,26 +2017,37 @@ impl IoClient { // Seek to start before reading all match AsyncSeekExt::seek(&mut file_clone, std::io::SeekFrom::Start(0)).await { Ok(_) => {} - Err(e) => return Err(format!("Failed to seek in file: {e}")), + Err(e) => return Err(FileReadError::Io(format!("Failed to seek in file: {e}"))), } - let mut contents = Vec::new(); - match AsyncReadExt::read_to_end(&mut file_clone, &mut contents).await { - Ok(_) => Ok(contents), - Err(e) => Err(format!("Failed to read binary file: {e}")), - } + read_to_end_capped(&mut file_clone, budget, "Failed to read binary file").await } - async fn read_binary_n(&self, handle_id: &str, count: usize) -> Result, String> { + async fn read_binary_n( + &self, + handle_id: &str, + count: usize, + budget: &ExecutionBudget, + ) -> Result, FileReadError> { + budget + .check_file_read_bytes(count) + .map_err(FileReadError::Budget)?; + let mut file_handles = self.file_handles.lock().await; if !file_handles.contains_key(handle_id) { - return Err(format!("Invalid file handle: {handle_id}")); + return Err(FileReadError::Io(format!( + "Invalid file handle: {handle_id}" + ))); } let mut file_clone = match file_handles.get_mut(handle_id).unwrap().1.try_clone().await { Ok(clone) => clone, - Err(e) => return Err(format!("Failed to clone file handle: {e}")), + Err(e) => { + return Err(FileReadError::Io(format!( + "Failed to clone file handle: {e}" + ))); + } }; drop(file_handles); @@ -1533,7 +2058,9 @@ impl IoClient { buf.truncate(n); Ok(buf) } - Err(e) => Err(format!("Failed to read binary bytes: {e}")), + Err(e) => Err(FileReadError::Io(format!( + "Failed to read binary bytes: {e}" + ))), } } @@ -1737,11 +2264,17 @@ impl IoClient { use_shell: bool, line: usize, column: usize, - ) -> Result<(String, String, i32), String> { + ) -> Result<(String, String, i32), ExecuteCommandError> { use crate::interpreter::command_sanitizer::CommandSanitizer; use tokio::process::Command; - let needs_shell = self.authorize_subprocess(command, args, use_shell, line, column)?; + let needs_shell = self + .authorize_subprocess(command, args, use_shell, line, column) + .map_err(ExecuteCommandError::Other)?; + + let budget = ExecutionBudget::current_or_default(); + let configured_timeout = Duration::from_secs(self.config.timeout_seconds.max(1)); + let deadline = foreground_command_deadline(&budget, configured_timeout)?; // Build the command let mut cmd = if needs_shell && (use_shell || args.is_empty()) { @@ -1762,7 +2295,7 @@ impl IoClient { } else { // Direct-exec path (still policy-gated above) let (program, parsed_args) = if args.is_empty() { - CommandSanitizer::parse_command(command)? + CommandSanitizer::parse_command(command).map_err(ExecuteCommandError::Other)? } else { ( command.to_string(), @@ -1775,15 +2308,123 @@ impl IoClient { cmd }; - // Execute the command - let output = cmd - .output() - .await - .map_err(|e| format!("Failed to execute command '{}': {}", command, e))?; + // `Command::output` accumulates both streams into unbounded Vecs and + // cannot observe WFL's cooperative cancellation while the child is + // stalled. Pipe and drain both streams concurrently under the existing + // per-stream buffer ceiling instead. `kill_on_drop` is a final safety + // net if this future itself is abandoned by its caller. + let mut child = cmd + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .kill_on_drop(true) + .spawn() + .map_err(|e| { + ExecuteCommandError::Other(format!( + "Failed to execute command '{}': {}", + command, e + )) + })?; + + let stdout_pipe = child.stdout.take().ok_or_else(|| { + ExecuteCommandError::Other("Failed to capture command stdout".to_string()) + })?; + let stderr_pipe = child.stderr.take().ok_or_else(|| { + ExecuteCommandError::Other("Failed to capture command stderr".to_string()) + })?; + let buffer_size = self.config.subprocess_config.max_buffer_size_bytes; + + let stdout_task = tokio::spawn(capture_process_stream(stdout_pipe, buffer_size)); + let stderr_task = tokio::spawn(capture_process_stream(stderr_pipe, buffer_size)); + let mut capture_abort = + ProcessCaptureAbortGuard::new(stdout_task.abort_handle(), stderr_task.abort_handle()); + + // The guarded operation includes pipe EOF, not just direct-child exit. + // A command can spawn a descendant that inherits stdout/stderr and then + // exit; without this wider guard, collectors would await that + // descendant forever even though `child.wait()` already succeeded. + let completed = { + let operation_child = &mut child; + let operation = async move { + let status = operation_child.wait().await.map_err(|error| { + ExecuteCommandError::Other(format!("Failed to wait for command: {error}")) + })?; + let (stdout_result, stderr_result) = tokio::join!(stdout_task, stderr_task); + let stdout_capture = stdout_result + .map_err(|error| { + ExecuteCommandError::Other(format!( + "Failed to join stdout collector: {error}" + )) + })? + .map_err(|error| { + ExecuteCommandError::Other(format!( + "Failed to read command stdout: {error}" + )) + })?; + let stderr_capture = stderr_result + .map_err(|error| { + ExecuteCommandError::Other(format!( + "Failed to join stderr collector: {error}" + )) + })? + .map_err(|error| { + ExecuteCommandError::Other(format!( + "Failed to read command stderr: {error}" + )) + })?; - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); - let exit_code = output.status.code().unwrap_or(-1); + Ok((status, stdout_capture, stderr_capture)) + }; + let interrupt = foreground_command_interrupt(&budget, deadline); + tokio::pin!(operation); + tokio::pin!(interrupt); + + tokio::select! { + biased; + result = &mut operation => Ok(result), + error = &mut interrupt => Err(error), + } + }; + + let (status, stdout_capture, stderr_capture) = match completed { + Ok(Ok(result)) => { + capture_abort.disarm(); + result + } + Ok(Err(interruption)) | Err(interruption) => { + // Stop retaining output immediately, then kill/reap the direct + // child if it is still alive. Closing its readers also prevents + // a chatty child from blocking forever on a full pipe. + capture_abort.abort(); + let termination = terminate_foreground_child(&mut child).await; + + if let Err(termination_error) = termination { + return Err(ExecuteCommandError::Other(format!( + "{interruption}; {termination_error}" + ))); + } + return Err(interruption); + } + }; + + if stdout_capture.bytes_dropped > 0 { + eprintln!( + "⚠️ WARNING: Command stdout exceeded max_buffer_size_bytes; \ + {} oldest byte(s) were discarded.", + stdout_capture.bytes_dropped + ); + } + if stderr_capture.bytes_dropped > 0 { + eprintln!( + "⚠️ WARNING: Command stderr exceeded max_buffer_size_bytes; \ + {} oldest byte(s) were discarded.", + stderr_capture.bytes_dropped + ); + } + + let stdout = String::from_utf8_lossy(&stdout_capture.bytes).to_string(); + let stderr = String::from_utf8_lossy(&stderr_capture.bytes).to_string(); + let exit_code = status.code().unwrap_or(-1); Ok((stdout, stderr, exit_code)) } @@ -2614,6 +3255,35 @@ impl Interpreter { RuntimeError::with_kind(exceeded.message(), line, column, kind) } + /// Attach WFL source information to an outbound HTTP error while + /// preserving structured timeout/resource-limit kinds for `try`/`when`. + fn http_client_error( + &self, + error: HttpClientError, + line: usize, + column: usize, + ) -> RuntimeError { + match error { + HttpClientError::Request(message) => RuntimeError::new(message, line, column), + HttpClientError::Budget(exceeded) => self.budget_error(exceeded, line, column), + HttpClientError::Timeout { seconds } => RuntimeError::with_kind( + format!("Outbound HTTP request exceeded timeout ({seconds}s)"), + line, + column, + ErrorKind::Timeout, + ), + } + } + + /// Preserve ordinary file I/O failures while classifying byte-ceiling + /// breaches as catchable execution-budget resource errors. + fn file_read_error(&self, error: FileReadError, line: usize, column: usize) -> RuntimeError { + match error { + FileReadError::Io(message) => RuntimeError::new(message, line, column), + FileReadError::Budget(exceeded) => self.budget_error(exceeded, line, column), + } + } + /// Map a pattern-VM error onto a `RuntimeError`. Budget breaches (step/state /// ceilings, cancellation) surface as catchable `ResourceLimit` errors so a /// ReDoS/cancellation during matching is not silently collapsed into a @@ -3902,7 +4572,7 @@ impl Interpreter { if is_file_path { match self.io_client.open_file(&path_str).await { - Ok(handle) => match self.io_client.read_file(&handle).await { + Ok(handle) => match self.io_client.read_file(&handle, &self.budget).await { Ok(content) => { match env .borrow_mut() @@ -3920,13 +4590,13 @@ impl Interpreter { } Err(e) => { let _ = self.io_client.close_file(&handle).await; - Err(RuntimeError::new(e, *line, *column)) + Err(self.file_read_error(e, *line, *column)) } }, Err(e) => Err(RuntimeError::new(e, *line, *column)), } } else { - match self.io_client.read_file(&path_str).await { + match self.io_client.read_file(&path_str, &self.budget).await { Ok(content) => { match env .borrow_mut() @@ -3936,7 +4606,7 @@ impl Interpreter { Err(msg) => Err(RuntimeError::new(msg, *line, *column)), } } - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(e) => Err(self.file_read_error(e, *line, *column)), } } } @@ -4824,31 +5494,35 @@ impl Interpreter { if is_file_path { match self.io_client.open_file(&path_str).await { - Ok(handle) => match self.io_client.read_file(&handle).await { - Ok(content) => { - match env - .borrow_mut() - .define(variable_name, Value::Text(content.into())) - { - Ok(_) => { - let _ = self.io_client.close_file(&handle).await; - Ok((Value::Null, ControlFlow::None)) - } - Err(msg) => { - let _ = self.io_client.close_file(&handle).await; - Err(RuntimeError::new(msg, *line, *column)) + Ok(handle) => { + match self.io_client.read_file(&handle, &self.budget).await { + Ok(content) => { + match env + .borrow_mut() + .define(variable_name, Value::Text(content.into())) + { + Ok(_) => { + let _ = + self.io_client.close_file(&handle).await; + Ok((Value::Null, ControlFlow::None)) + } + Err(msg) => { + let _ = + self.io_client.close_file(&handle).await; + Err(RuntimeError::new(msg, *line, *column)) + } } } + Err(e) => { + let _ = self.io_client.close_file(&handle).await; + Err(self.file_read_error(e, *line, *column)) + } } - Err(e) => { - let _ = self.io_client.close_file(&handle).await; - Err(RuntimeError::new(e, *line, *column)) - } - }, + } Err(e) => Err(RuntimeError::new(e, *line, *column)), } } else { - match self.io_client.read_file(&path_str).await { + match self.io_client.read_file(&path_str, &self.budget).await { Ok(content) => { match env .borrow_mut() @@ -4858,7 +5532,7 @@ impl Interpreter { Err(msg) => Err(RuntimeError::new(msg, *line, *column)), } } - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(e) => Err(self.file_read_error(e, *line, *column)), } } } @@ -5008,7 +5682,11 @@ impl Interpreter { } }; - match self.io_client.http_get(&url_str).await { + match self + .io_client + .http_get(&url_str, Arc::clone(&self.budget)) + .await + { Ok(body) => { match env .borrow_mut() @@ -5018,7 +5696,7 @@ impl Interpreter { Err(msg) => Err(RuntimeError::new(msg, *line, *column)), } } - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(error) => Err(self.http_client_error(error, *line, *column)), } } Statement::HttpPostStatement { @@ -5053,7 +5731,11 @@ impl Interpreter { } }; - match self.io_client.http_post(&url_str, &data_str).await { + match self + .io_client + .http_post(&url_str, &data_str, Arc::clone(&self.budget)) + .await + { Ok(body) => { match env .borrow_mut() @@ -5063,7 +5745,7 @@ impl Interpreter { Err(msg) => Err(RuntimeError::new(msg, *line, *column)), } } - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(error) => Err(self.http_client_error(error, *line, *column)), } } Statement::HttpRequestStatement { @@ -5171,7 +5853,13 @@ impl Interpreter { match self .io_client - .http_request(&method_str, &url_str, &header_list, body_str) + .http_request( + &method_str, + &url_str, + &header_list, + body_str, + Arc::clone(&self.budget), + ) .await { Ok((status, response_headers, response_body)) => { @@ -5203,7 +5891,7 @@ impl Interpreter { Err(msg) => Err(RuntimeError::new(msg, *line, *column)), } } - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(error) => Err(self.http_client_error(error, *line, *column)), } } Statement::RepeatWhileLoop { @@ -7528,19 +8216,31 @@ impl Interpreter { .io_client .execute_command(cmd_str, &args_refs, *use_shell, *line, *column) .await - .map_err(|e| { - // Determine error kind based on error message - let kind = if e.contains("program not found") - || e.contains("cannot find") - || e.contains("not recognized") - { - ErrorKind::CommandNotFound - } else if e.contains("spawn") { - ErrorKind::ProcessSpawnFailed - } else { - ErrorKind::General - }; - RuntimeError::with_kind(e, *line, *column, kind) + .map_err(|e| match e { + ExecuteCommandError::Budget(exceeded) => { + self.budget_error(exceeded, *line, *column) + } + ExecuteCommandError::Timeout { seconds } => RuntimeError::with_kind( + format!("Subprocess execution exceeded timeout ({seconds}s)"), + *line, + *column, + ErrorKind::Timeout, + ), + ExecuteCommandError::Other(message) => { + // Preserve the existing subprocess error + // classification for non-budget failures. + let kind = if message.contains("program not found") + || message.contains("cannot find") + || message.contains("not recognized") + { + ErrorKind::CommandNotFound + } else if message.contains("spawn") { + ErrorKind::ProcessSpawnFailed + } else { + ErrorKind::General + }; + RuntimeError::with_kind(message, *line, *column, kind) + } })?; // Build result object @@ -9730,9 +10430,9 @@ impl Interpreter { } }; - match self.io_client.read_file(&handle_str).await { + match self.io_client.read_file(&handle_str, &self.budget).await { Ok(content) => Ok(Value::Text(content.into())), - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(e) => Err(self.file_read_error(e, *line, *column)), } } Expression::ReadBinaryContent { @@ -9757,9 +10457,9 @@ impl Interpreter { } }; - match self.io_client.read_binary(&handle_str).await { + match self.io_client.read_binary(&handle_str, &self.budget).await { Ok(bytes) => Ok(Value::Binary(Arc::from(bytes))), - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(e) => Err(self.file_read_error(e, *line, *column)), } } Expression::ReadBinaryN { @@ -9796,20 +10496,7 @@ impl Interpreter { *column, )); } - let count = *n as usize; - // 50MB limit to prevent memory exhaustion - const MAX_READ_BYTES: usize = 50 * 1024 * 1024; - if count > MAX_READ_BYTES { - return Err(RuntimeError::new( - format!( - "Byte count {} exceeds maximum allowed ({})", - count, MAX_READ_BYTES - ), - *line, - *column, - )); - } - count + *n as usize } _ => { return Err(RuntimeError::new( @@ -9823,9 +10510,13 @@ impl Interpreter { } }; - match self.io_client.read_binary_n(&handle_str, n).await { + match self + .io_client + .read_binary_n(&handle_str, n, &self.budget) + .await + { Ok(bytes) => Ok(Value::Binary(Arc::from(bytes))), - Err(e) => Err(RuntimeError::new(e, *line, *column)), + Err(e) => Err(self.file_read_error(e, *line, *column)), } } Expression::FileSizeOf { @@ -10739,10 +11430,102 @@ mod header_lookup_tests { } } +#[cfg(test)] +mod file_read_tests { + use super::*; + use crate::exec::budget::BudgetLimits; + + fn budget_with_file_limit(limit: usize) -> ExecutionBudget { + let limits = BudgetLimits { + max_file_read_bytes: limit, + ..BudgetLimits::default() + }; + ExecutionBudget::new(limits) + } + + #[tokio::test] + async fn capped_read_stops_an_infinite_source_at_limit_plus_one() { + let budget = budget_with_file_limit(8); + let mut source = tokio::io::repeat(0xA5); + let result = tokio::time::timeout( + Duration::from_secs(1), + read_to_end_capped(&mut source, &budget, "test read"), + ) + .await + .expect("bounded read must not wait for EOF from an infinite source"); + + assert!(matches!( + result, + Err(FileReadError::Budget(BudgetExceeded::FileReadBytes { + limit: 8, + actual: 9 + })) + )); + } + + #[tokio::test] + async fn capped_read_allows_a_payload_exactly_at_the_limit() { + let budget = budget_with_file_limit(8); + let mut source = std::io::Cursor::new(b"12345678".to_vec()); + let bytes = read_to_end_capped(&mut source, &budget, "test read") + .await + .expect("an exact-limit payload is valid"); + assert_eq!(bytes, b"12345678"); + } + + #[tokio::test] + async fn text_and_binary_read_all_share_the_budget_ceiling() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("oversized.dat"); + std::fs::write(&path, b"123456789").expect("fixture"); + let path = path.to_string_lossy(); + let client = IoClient::new(Arc::new(WflConfig::default())); + let budget = budget_with_file_limit(8); + + let text_handle = client + .open_file_with_mode(&path, FileOpenMode::Read) + .await + .expect("open text fixture"); + assert!(matches!( + client.read_file(&text_handle, &budget).await, + Err(FileReadError::Budget(BudgetExceeded::FileReadBytes { .. })) + )); + client.close_file(&text_handle).await.expect("close text"); + + let binary_handle = client + .open_file_with_mode(&path, FileOpenMode::ReadBinary) + .await + .expect("open binary fixture"); + assert!(matches!( + client.read_binary(&binary_handle, &budget).await, + Err(FileReadError::Budget(BudgetExceeded::FileReadBytes { .. })) + )); + client + .close_file(&binary_handle) + .await + .expect("close binary"); + } + + #[tokio::test] + async fn explicit_binary_count_is_checked_before_allocation() { + let client = IoClient::new(Arc::new(WflConfig::default())); + let budget = budget_with_file_limit(8); + let result = client.read_binary_n("not-open", 9, &budget).await; + assert!(matches!( + result, + Err(FileReadError::Budget(BudgetExceeded::FileReadBytes { + limit: 8, + actual: 9 + })) + )); + } +} + #[cfg(test)] mod process_tests { use super::*; use crate::config::ShellExecutionMode; + use crate::exec::budget::BudgetLimits; /// Config that permits subprocesses for lifecycle tests (not the secure default). fn permissive_process_config() -> Arc { @@ -10754,6 +11537,75 @@ mod process_tests { }) } + /// Invoke one ignored helper test in a fresh copy of this test binary. This + /// is cross-platform and avoids depending on optional shell utilities in CI. + fn test_helper_command(filter: &str) -> (String, Vec) { + let executable = std::env::current_exe() + .expect("current test executable") + .to_string_lossy() + .into_owned(); + let arguments = vec![ + filter.to_string(), + "--ignored".to_string(), + "--nocapture".to_string(), + "--test-threads=1".to_string(), + ]; + (executable, arguments) + } + + #[test] + #[ignore = "subprocess fixture; invoked by foreground execution tests"] + fn subprocess_test_helper_floods_stdout_and_stderr() { + use std::io::Write as _; + + let stdout_writer = std::thread::spawn(|| { + let chunk = [b'O'; 8192]; + let mut stdout = std::io::stdout().lock(); + for _ in 0..64 { + stdout.write_all(&chunk).expect("write helper stdout"); + } + stdout.flush().expect("flush helper stdout"); + }); + let stderr_writer = std::thread::spawn(|| { + let chunk = [b'E'; 8192]; + let mut stderr = std::io::stderr().lock(); + for _ in 0..64 { + stderr.write_all(&chunk).expect("write helper stderr"); + } + stderr.flush().expect("flush helper stderr"); + }); + + stdout_writer.join().expect("stdout helper thread"); + stderr_writer.join().expect("stderr helper thread"); + } + + #[test] + #[ignore = "subprocess fixture; invoked by foreground execution tests"] + fn subprocess_test_helper_stalls() { + std::thread::sleep(Duration::from_secs(30)); + } + + #[test] + #[ignore = "subprocess fixture; invoked by foreground execution tests"] + fn subprocess_test_helper_short_stall() { + std::thread::sleep(Duration::from_secs(2)); + } + + #[test] + #[ignore = "subprocess fixture; invoked by foreground execution tests"] + // This fixture must drop the descendant handle: waiting would close the + // inherited-pipe window that the foreground cleanup regression exercises. + #[allow(clippy::zombie_processes)] + fn subprocess_test_helper_leaves_inherited_pipes_open() { + let (command, arguments) = test_helper_command("subprocess_test_helper_short_stall"); + let _descendant = std::process::Command::new(command) + .args(arguments) + .spawn() + .expect("spawn descendant that inherits stdout/stderr"); + // Drop the handle and return. The descendant deliberately outlives this + // direct child while retaining its inherited stdout/stderr handles. + } + #[tokio::test] async fn test_default_config_blocks_direct_exec() { let client = IoClient::new(Arc::new(WflConfig::default())); @@ -10766,6 +11618,7 @@ mod process_tests { result ); let err = result.unwrap_err(); + let err = err.to_string(); assert!( err.contains("security policy") || err.contains("blocked") || err.contains("disabled"), "Error should mention policy: {}", @@ -10815,6 +11668,153 @@ mod process_tests { ); } + #[tokio::test] + async fn test_execute_command_bounds_stdout_and_stderr() { + const STREAM_LIMIT: usize = 1024; + + let mut config = (*permissive_process_config()).clone(); + config.subprocess_config.max_buffer_size_bytes = STREAM_LIMIT; + let client = IoClient::new(Arc::new(config)); + let (command, arguments) = + test_helper_command("subprocess_test_helper_floods_stdout_and_stderr"); + let argument_refs: Vec<&str> = arguments.iter().map(String::as_str).collect(); + + let (stdout, stderr, exit_code) = tokio::time::timeout( + Duration::from_secs(10), + client.execute_command(&command, &argument_refs, false, 0, 0), + ) + .await + .expect("chatty helper must not deadlock") + .expect("chatty helper should execute"); + + assert_eq!(exit_code, 0); + assert!( + stdout.len() <= STREAM_LIMIT, + "stdout retained {} bytes, limit is {STREAM_LIMIT}", + stdout.len() + ); + assert!( + stderr.len() <= STREAM_LIMIT, + "stderr retained {} bytes, limit is {STREAM_LIMIT}", + stderr.len() + ); + assert!( + !stdout.is_empty(), + "the bounded stdout tail should be retained" + ); + assert!( + !stderr.is_empty(), + "the bounded stderr tail should be retained" + ); + } + + #[tokio::test] + async fn test_execute_command_kills_stalled_child_on_cancellation() { + let client = IoClient::new(permissive_process_config()); + let (command, arguments) = test_helper_command("subprocess_test_helper_stalls"); + let argument_refs: Vec<&str> = arguments.iter().map(String::as_str).collect(); + let budget = Arc::new(ExecutionBudget::unlimited()); + let cancellation_budget = Arc::clone(&budget); + + let execution = ExecutionBudget::scope( + Arc::clone(&budget), + client.execute_command(&command, &argument_refs, false, 0, 0), + ); + let cancel = async move { + tokio::time::sleep(Duration::from_millis(50)).await; + cancellation_budget.cancel(); + }; + + let (result, ()) = tokio::time::timeout(Duration::from_secs(3), async { + tokio::join!(execution, cancel) + }) + .await + .expect("cancellation must terminate and reap the stalled child"); + + assert!(matches!( + result, + Err(ExecuteCommandError::Budget(BudgetExceeded::Cancelled)) + )); + } + + #[tokio::test] + async fn test_execute_command_kills_stalled_child_at_deadline() { + let client = IoClient::new(permissive_process_config()); + let (command, arguments) = test_helper_command("subprocess_test_helper_stalls"); + let argument_refs: Vec<&str> = arguments.iter().map(String::as_str).collect(); + let mut limits = BudgetLimits::unlimited(); + limits.max_duration = Some(Duration::from_millis(100)); + let budget = Arc::new(ExecutionBudget::new(limits)); + + let result = tokio::time::timeout( + Duration::from_secs(3), + ExecutionBudget::scope( + Arc::clone(&budget), + client.execute_command(&command, &argument_refs, false, 0, 0), + ), + ) + .await + .expect("deadline must terminate and reap the stalled child"); + + assert!(matches!( + result, + Err(ExecuteCommandError::Budget(BudgetExceeded::Deadline { .. })) + )); + } + + #[tokio::test] + async fn test_execute_command_has_finite_timeout_inside_main_loop() { + let mut config = (*permissive_process_config()).clone(); + config.timeout_seconds = 1; + let config = Arc::new(config); + let client = IoClient::new(Arc::clone(&config)); + let budget = Arc::new(ExecutionBudget::from_config(&config)); + let _main_loop = budget.enter_main_loop(); + let (command, arguments) = test_helper_command("subprocess_test_helper_stalls"); + let argument_refs: Vec<&str> = arguments.iter().map(String::as_str).collect(); + + let result = tokio::time::timeout( + Duration::from_secs(3), + ExecutionBudget::scope( + Arc::clone(&budget), + client.execute_command(&command, &argument_refs, false, 0, 0), + ), + ) + .await + .expect("a main-loop command must retain a finite per-operation timeout"); + + assert!(matches!( + result, + Err(ExecuteCommandError::Timeout { seconds: 1 }) + )); + } + + #[tokio::test] + async fn test_execute_command_deadline_covers_inherited_pipe_drain() { + let client = IoClient::new(permissive_process_config()); + let (command, arguments) = + test_helper_command("subprocess_test_helper_leaves_inherited_pipes_open"); + let argument_refs: Vec<&str> = arguments.iter().map(String::as_str).collect(); + let mut limits = BudgetLimits::unlimited(); + limits.max_duration = Some(Duration::from_millis(300)); + let budget = Arc::new(ExecutionBudget::new(limits)); + + let result = tokio::time::timeout( + Duration::from_secs(3), + ExecutionBudget::scope( + Arc::clone(&budget), + client.execute_command(&command, &argument_refs, false, 0, 0), + ), + ) + .await + .expect("inherited pipes must not outlive the execution deadline"); + + assert!(matches!( + result, + Err(ExecuteCommandError::Budget(BudgetExceeded::Deadline { .. })) + )); + } + #[cfg(unix)] #[tokio::test] async fn test_spawn_and_kill_process() { diff --git a/src/interpreter/tests.rs b/src/interpreter/tests.rs index 57fb32e5..15b956e1 100644 --- a/src/interpreter/tests.rs +++ b/src/interpreter/tests.rs @@ -583,3 +583,28 @@ async fn test_header_access_case_insensitive_via_request_object() { "absent header should be nothing, got {result:?}" ); } + +/// A WFL program can insert a list into itself through `push`. Displaying that +/// value used to recurse on the native stack until the whole process aborted. +#[tokio::test] +async fn test_display_self_referential_list_is_cycle_safe() { + let source = r#" +create list items: +end list +push with items and items +display items +"#; + let tokens = lex_wfl_with_positions(source); + let mut parser = Parser::new(&tokens); + let program = parser.parse().expect("parse self-referential list program"); + let mut interpreter = Interpreter::new(); + + let output = std::rc::Rc::new(std::cell::RefCell::new(String::new())); + let result = { + let _capture = super::io_capture::push_capture(std::rc::Rc::clone(&output)); + interpreter.interpret(&program).await + }; + + result.expect("displaying a self-referential list must not abort or error"); + assert_eq!(&*output.borrow(), "[]\n"); +} diff --git a/src/interpreter/value.rs b/src/interpreter/value.rs index 7d033a6a..584d4de5 100644 --- a/src/interpreter/value.rs +++ b/src/interpreter/value.rs @@ -156,6 +156,32 @@ pub struct ActionSignature { pub column: usize, } +/// Keep value formatting comfortably below the native stack limit even when a +/// program builds a very deeply nested (but acyclic) container graph. +const MAX_VALUE_FORMAT_DEPTH: usize = 64; + +type ListStorage = RefCell>; +type ObjectStorage = RefCell>; +type ContainerInstanceStorage = RefCell; + +#[derive(Default)] +struct ValueFormatState { + active_lists: HashSet<*const ListStorage>, + active_objects: HashSet<*const ObjectStorage>, +} + +/// Memoized mutable containers created during one deep-clone operation. +/// +/// The placeholder is inserted before its contents are cloned. That both +/// breaks cycles and preserves aliases: two references to one source container +/// become two references to one cloned container. +#[derive(Default)] +struct DeepCloneMemo { + lists: HashMap<*const ListStorage, Rc>, + objects: HashMap<*const ObjectStorage, Rc>, + container_instances: HashMap<*const ContainerInstanceStorage, Rc>, +} + impl Value { pub fn type_name(&self) -> &'static str { match self { @@ -207,85 +233,171 @@ impl Value { /// Deep clone a value, creating independent copies of reference-counted containers. /// This is used for module isolation to prevent mutations from affecting parent scopes. pub fn deep_clone(&self) -> Self { + self.deep_clone_with_memo(&mut DeepCloneMemo::default()) + } + + fn deep_clone_with_memo(&self, memo: &mut DeepCloneMemo) -> Self { match self { - // For List, create a new Rc> with recursively cloned elements Value::List(list) => { - let cloned_vec = list + let source_id = Rc::as_ptr(list); + if let Some(cloned) = memo.lists.get(&source_id) { + return Value::List(Rc::clone(cloned)); + } + + let cloned = Rc::new(RefCell::new(Vec::new())); + memo.lists.insert(source_id, Rc::clone(&cloned)); + + let cloned_items = list .borrow() .iter() - .map(|v| v.deep_clone()) + .map(|value| value.deep_clone_with_memo(memo)) .collect::>(); - Value::List(Rc::new(RefCell::new(cloned_vec))) + *cloned.borrow_mut() = cloned_items; + + Value::List(cloned) } - // For Object, create a new Rc> with recursively cloned values Value::Object(obj) => { - let cloned_map = obj + let source_id = Rc::as_ptr(obj); + if let Some(cloned) = memo.objects.get(&source_id) { + return Value::Object(Rc::clone(cloned)); + } + + let cloned = Rc::new(RefCell::new(HashMap::new())); + memo.objects.insert(source_id, Rc::clone(&cloned)); + + let cloned_entries = obj .borrow() .iter() - .map(|(k, v)| (k.clone(), v.deep_clone())) + .map(|(key, value)| (key.clone(), value.deep_clone_with_memo(memo))) .collect::>(); - Value::Object(Rc::new(RefCell::new(cloned_map))) + *cloned.borrow_mut() = cloned_entries; + + Value::Object(cloned) } - // For ContainerInstance, create a new Rc> with deep cloned properties Value::ContainerInstance(instance) => { - let inst = instance.borrow(); - let cloned_properties = inst - .properties - .iter() - .map(|(k, v)| (k.clone(), v.deep_clone())) - .collect::>(); - let cloned_parent = inst.parent.as_ref().map(|p| { - // Clone the parent reference, not deep clone (to avoid infinite recursion) - Rc::clone(p) - }); - Value::ContainerInstance(Rc::new(RefCell::new(ContainerInstanceValue { - container_type: inst.container_type.clone(), - properties: cloned_properties, - parent: cloned_parent, - line: inst.line, - column: inst.column, - }))) + Value::ContainerInstance(Self::deep_clone_container_instance(instance, memo)) } // For all other types, use regular clone (they're either primitives or immutable Rc types) _ => self.clone(), } } -} -impl fmt::Debug for Value { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fn deep_clone_container_instance( + instance: &Rc>, + memo: &mut DeepCloneMemo, + ) -> Rc> { + let source_id = Rc::as_ptr(instance); + if let Some(cloned) = memo.container_instances.get(&source_id) { + return Rc::clone(cloned); + } + + let cloned = Rc::new(RefCell::new(ContainerInstanceValue { + container_type: String::new(), + properties: HashMap::new(), + parent: None, + line: 0, + column: 0, + })); + memo.container_instances + .insert(source_id, Rc::clone(&cloned)); + + let source = instance.borrow(); + let cloned_properties = source + .properties + .iter() + .map(|(key, value)| (key.clone(), value.deep_clone_with_memo(memo))) + .collect(); + let cloned_parent = source + .parent + .as_ref() + .map(|parent| Self::deep_clone_container_instance(parent, memo)); + + *cloned.borrow_mut() = ContainerInstanceValue { + container_type: source.container_type.clone(), + properties: cloned_properties, + parent: cloned_parent, + line: source.line, + column: source.column, + }; + + cloned + } + + fn fmt_debug_with_state( + &self, + f: &mut fmt::Formatter, + state: &mut ValueFormatState, + depth: usize, + ) -> fmt::Result { match self { Value::Number(n) => write!(f, "{n}"), Value::Text(s) => write!(f, "\"{s}\""), Value::Bool(b) => write!(f, "{b}"), Value::Nothing => write!(f, "nothing"), - Value::List(l) => { - let values = l.borrow(); - write!(f, "[")?; - for (i, v) in values.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; - } - write!(f, "{v:?}")?; + Value::List(list) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); } - write!(f, "]") - } - Value::Object(o) => { - let map = o.borrow(); - write!(f, "{{")?; - for (i, (k, v)) in map.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; + + let id = Rc::as_ptr(list); + if !state.active_lists.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let values = match list.try_borrow() { + Ok(values) => values, + Err(_) => return write!(f, ""), + }; + + write!(f, "[")?; + for (index, value) in values.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + value.fmt_debug_with_state(f, state, depth + 1)?; } - write!(f, "{k}: {v:?}")?; + write!(f, "]") + })(); + + state.active_lists.remove(&id); + result + } + Value::Object(obj) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); + } + + let id = Rc::as_ptr(obj); + if !state.active_objects.insert(id) { + return write!(f, ""); } - write!(f, "}}") + + let result = (|| { + let map = match obj.try_borrow() { + Ok(map) => map, + Err(_) => return write!(f, ""), + }; + + write!(f, "{{")?; + for (index, (key, value)) in map.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + write!(f, "{key}: ")?; + value.fmt_debug_with_state(f, state, depth + 1)?; + } + write!(f, "}}") + })(); + + state.active_objects.remove(&id); + result } Value::Function(func) => { write!( f, "Function({})", - func.name.as_ref().unwrap_or(&"anonymous".to_string()) + func.name.as_deref().unwrap_or("anonymous") ) } Value::NativeFunction(name, _) => write!(f, "NativeFunction({name})"), @@ -306,53 +418,89 @@ impl fmt::Debug for Value { Value::InterfaceDefinition(interface) => write!(f, "", interface.name), } } -} -impl fmt::Display for Value { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fn fmt_display_with_state( + &self, + f: &mut fmt::Formatter, + state: &mut ValueFormatState, + depth: usize, + ) -> fmt::Result { match self { Value::Number(n) => write!(f, "{n}"), Value::Text(s) => write!(f, "{s}"), Value::Bool(b) => write!(f, "{}", if *b { "yes" } else { "no" }), Value::Nothing => write!(f, "nothing"), Value::List(list) => { - let items = list.borrow(); - write!(f, "[")?; - for (i, v) in items.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; - } - write!(f, "{v}")?; + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); } - write!(f, "]") - } - Value::Object(o) => { - let map = o.borrow(); - if map.len() == 1 { - if let Some((_, value)) = map.iter().next() { - write!(f, "{value}") - } else { - write!(f, "[Object]") - } - } else if map.is_empty() { - write!(f, "[Object]") - } else { - write!(f, "{{")?; - for (i, (k, v)) in map.iter().enumerate() { - if i > 0 { + + let id = Rc::as_ptr(list); + if !state.active_lists.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let items = match list.try_borrow() { + Ok(items) => items, + Err(_) => return write!(f, ""), + }; + + write!(f, "[")?; + for (index, value) in items.iter().enumerate() { + if index > 0 { write!(f, ", ")?; } - write!(f, "{k}: {v}")?; + value.fmt_display_with_state(f, state, depth + 1)?; } - write!(f, "}}") + write!(f, "]") + })(); + + state.active_lists.remove(&id); + result + } + Value::Object(obj) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); + } + + let id = Rc::as_ptr(obj); + if !state.active_objects.insert(id) { + return write!(f, ""); } + + let result = (|| { + let map = match obj.try_borrow() { + Ok(map) => map, + Err(_) => return write!(f, ""), + }; + + if map.len() == 1 { + if let Some((_, value)) = map.iter().next() { + value.fmt_display_with_state(f, state, depth + 1) + } else { + write!(f, "[Object]") + } + } else if map.is_empty() { + write!(f, "[Object]") + } else { + write!(f, "{{")?; + for (index, (key, value)) in map.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + write!(f, "{key}: ")?; + value.fmt_display_with_state(f, state, depth + 1)?; + } + write!(f, "}}") + } + })(); + + state.active_objects.remove(&id); + result } Value::Function(func) => { - write!( - f, - "action {}", - func.name.as_ref().unwrap_or(&"anonymous".to_string()) - ) + write!(f, "action {}", func.name.as_deref().unwrap_or("anonymous")) } Value::NativeFunction(name, _) => write!(f, "native {name}"), Value::Future(_) => write!(f, "[Future]"), @@ -374,6 +522,18 @@ impl fmt::Display for Value { } } +impl fmt::Debug for Value { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.fmt_debug_with_state(f, &mut ValueFormatState::default(), 0) + } +} + +impl fmt::Display for Value { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.fmt_display_with_state(f, &mut ValueFormatState::default(), 0) + } +} + impl PartialEq for Value { fn eq(&self, other: &Self) -> bool { // Optimization: Mismatched types are never equal. diff --git a/src/lexer/tests.rs b/src/lexer/tests.rs index 92d0f677..b75a982b 100644 --- a/src/lexer/tests.rs +++ b/src/lexer/tests.rs @@ -158,6 +158,35 @@ fn test_keyword_case_sensitivity() { } } +#[test] +fn oversized_integer_literal_is_a_lex_error_instead_of_a_panic() { + use logos::Logos; + + // One above i64::MAX. The old `parse::().unwrap()` callback panicked + // here, allowing an untrusted source file to abort CLI/LSP processing. + let mut lexer = Token::lexer("9223372036854775808"); + assert!(matches!(lexer.next(), Some(Err(_)))); + assert!(lexer.next().is_none()); + + // Exercise both public collection paths as regression protection: neither + // may unwind when the malformed literal appears inside an otherwise valid + // statement. + let source = "store value as 9223372036854775808\n"; + assert!(std::panic::catch_unwind(|| lex_wfl(source)).is_ok()); + assert!(std::panic::catch_unwind(|| lex_wfl_with_positions(source)).is_ok()); +} + +#[test] +fn overflowing_float_literal_is_rejected_as_non_finite() { + use logos::Logos; + + let source = format!("{}.0", "9".repeat(400)); + let mut lexer = Token::lexer(&source); + assert!(matches!(lexer.next(), Some(Err(_)))); + assert!(lexer.next().is_none()); + assert!(std::panic::catch_unwind(|| lex_wfl_with_positions(&source)).is_ok()); +} + // Escape sequence tests #[test] fn test_parse_string_newline_escape() { diff --git a/src/lexer/token.rs b/src/lexer/token.rs index c1c6e9cf..93e4d410 100644 --- a/src/lexer/token.rs +++ b/src/lexer/token.rs @@ -460,10 +460,10 @@ pub enum Token { #[regex(r#""([^"\\]|\\.)*""#, |lex| parse_string(lex).ok())] // captures content inside quotes StringLiteral(String), - #[regex("[0-9]+\\.[0-9]+", |lex| lex.slice().parse::().unwrap())] + #[regex("[0-9]+\\.[0-9]+", parse_float_literal)] FloatLiteral(f64), - #[regex("[0-9]+", |lex| lex.slice().parse::().unwrap())] + #[regex("[0-9]+", parse_int_literal)] IntLiteral(i64), #[regex("[A-Za-z][A-Za-z0-9_]*", |lex| lex.slice().to_string())] @@ -478,6 +478,23 @@ pub enum Token { Error, } +/// Parse a decimal integer without letting attacker-controlled source panic the +/// lexer. Logos treats `None` as a normal lexing error, which the positioned +/// lexer reports with the literal's span and the parser then rejects. +fn parse_int_literal(lex: &mut logos::Lexer) -> Option { + lex.slice().parse::().ok() +} + +/// WFL numbers must be finite. Rust accepts very large decimal floats as +/// positive infinity, so reject both parse failures and non-finite results at +/// the token boundary instead of injecting an infinity value into the runtime. +fn parse_float_literal(lex: &mut logos::Lexer) -> Option { + lex.slice() + .parse::() + .ok() + .filter(|value| value.is_finite()) +} + fn parse_string(lex: &mut logos::Lexer) -> Result { let quoted = lex.slice(); // e.g. "\"Alice\"" let inner = "ed[1..quoted.len() - 1]; // strip the surrounding quotes diff --git a/src/parser/stmt/patterns.rs b/src/parser/stmt/patterns.rs index d200ea21..33c15ab2 100644 --- a/src/parser/stmt/patterns.rs +++ b/src/parser/stmt/patterns.rs @@ -27,6 +27,39 @@ fn token_to_char_class(token: &Token) -> Option { } } +/// Convert a lexer integer to the representation used by pattern quantifiers +/// without allowing signed or oversized values to wrap during conversion. +fn checked_quantifier_count(value: i64, token: &TokenWithPosition) -> Result { + u32::try_from(value).map_err(|_| { + ParseError::from_token( + format!( + "Pattern quantifier count must be between 0 and {}", + u32::MAX + ), + token, + ) + }) +} + +/// Validate a bounded quantifier before constructing its AST node. Keeping +/// this invariant at the parser boundary prevents `max - min` underflow in +/// downstream consumers, while the compiler repeats the check defensively for +/// ASTs constructed through the public Rust API. +fn checked_quantifier_range( + min: u32, + max: u32, + token: &TokenWithPosition, +) -> Result<(u32, u32), ParseError> { + if min > max { + Err(ParseError::from_token( + format!("Pattern quantifier lower bound ({min}) cannot exceed upper bound ({max})"), + token, + )) + } else { + Ok((min, max)) + } +} + pub(crate) trait PatternParser<'a>: ExprParser<'a> { fn parse_create_pattern_statement(&mut self) -> Result; fn parse_pattern_tokens(tokens: &[TokenWithPosition]) -> Result; @@ -451,6 +484,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { *i += 1; // Skip "exactly" if *i < tokens.len() { if let Token::IntLiteral(n) = tokens[*i].token { + let count = checked_quantifier_count(n, &tokens[*i])?; *i += 1; // Skip the number // Optionally consume "of" keyword @@ -461,7 +495,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { let base_element = Self::parse_pattern_element(tokens, i)?; PatternExpression::Quantified { pattern: Box::new(base_element), - quantifier: Quantifier::Exactly(n as u32), + quantifier: Quantifier::Exactly(count), } } else { return Err(ParseError::from_token( @@ -486,6 +520,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { *i += 1; // Skip "least" if *i < tokens.len() { if let Token::IntLiteral(n) = tokens[*i].token { + let count = checked_quantifier_count(n, &tokens[*i])?; *i += 1; // Skip the number // Optionally consume "of" keyword @@ -496,7 +531,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { let base_element = Self::parse_pattern_element(tokens, i)?; PatternExpression::Quantified { pattern: Box::new(base_element), - quantifier: Quantifier::AtLeast(n as u32), + quantifier: Quantifier::AtLeast(count), } } else { return Err(ParseError::from_token( @@ -515,6 +550,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { *i += 1; // Skip "most" if *i < tokens.len() { if let Token::IntLiteral(n) = tokens[*i].token { + let count = checked_quantifier_count(n, &tokens[*i])?; *i += 1; // Skip the number // Optionally consume "of" keyword @@ -525,7 +561,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { let base_element = Self::parse_pattern_element(tokens, i)?; PatternExpression::Quantified { pattern: Box::new(base_element), - quantifier: Quantifier::AtMost(n as u32), + quantifier: Quantifier::AtMost(count), } } else { return Err(ParseError::from_token( @@ -557,13 +593,18 @@ impl<'a> PatternParser<'a> for Parser<'a> { // Handle "N to M" syntax for numeric ranges Token::IntLiteral(min) => { - let min_val = *min as u32; + let min_value = *min; + let min_token_index = *i; *i += 1; // Skip the number // Check if this is a range pattern "N to M" if *i + 1 < tokens.len() && tokens[*i].token == Token::KeywordTo { + let min_val = checked_quantifier_count(min_value, &tokens[min_token_index])?; *i += 1; // Skip "to" if let Token::IntLiteral(max) = tokens[*i].token { + let max_val = checked_quantifier_count(max, &tokens[*i])?; + let (min_val, max_val) = + checked_quantifier_range(min_val, max_val, &tokens[*i])?; *i += 1; // Skip the max number // Optionally consume "of" keyword @@ -574,7 +615,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { let base_element = Self::parse_pattern_element(tokens, i)?; PatternExpression::Quantified { pattern: Box::new(base_element), - quantifier: Quantifier::Between(min_val, max as u32), + quantifier: Quantifier::Between(min_val, max_val), } } else { return Err(ParseError::from_token( @@ -584,7 +625,7 @@ impl<'a> PatternParser<'a> for Parser<'a> { } } else { // It's just a number literal, treat it as a literal pattern - PatternExpression::Literal(min.to_string()) + PatternExpression::Literal(min_value.to_string()) } } @@ -1112,10 +1153,11 @@ impl<'a> PatternParser<'a> for Parser<'a> { Token::KeywordExactly => { if *i + 1 < tokens.len() { if let Token::IntLiteral(n) = tokens[*i + 1].token { + let count = checked_quantifier_count(n, &tokens[*i + 1])?; *i += 2; Ok(PatternExpression::Quantified { pattern: Box::new(base_pattern), - quantifier: Quantifier::Exactly(n as u32), + quantifier: Quantifier::Exactly(count), }) } else { Ok(base_pattern) @@ -1129,10 +1171,13 @@ impl<'a> PatternParser<'a> for Parser<'a> { if let (Token::IntLiteral(min), Token::IntLiteral(max)) = (&tokens[*i + 1].token, &tokens[*i + 3].token) { + let min = checked_quantifier_count(*min, &tokens[*i + 1])?; + let max = checked_quantifier_count(*max, &tokens[*i + 3])?; + let (min, max) = checked_quantifier_range(min, max, &tokens[*i + 3])?; *i += 4; Ok(PatternExpression::Quantified { pattern: Box::new(base_pattern), - quantifier: Quantifier::Between(*min as u32, *max as u32), + quantifier: Quantifier::Between(min, max), }) } else { Ok(base_pattern) @@ -1145,3 +1190,18 @@ impl<'a> PatternParser<'a> for Parser<'a> { } } } + +#[cfg(test)] +mod quantifier_validation_tests { + use super::*; + + #[test] + fn checked_quantifier_count_rejects_negative_and_out_of_range_values() { + for value in [-1, i64::from(u32::MAX) + 1, i64::MAX] { + let token = TokenWithPosition::new(Token::IntLiteral(value), 1, 1, 1); + let error = checked_quantifier_count(value, &token) + .expect_err("invalid quantifier count must be rejected"); + assert!(error.message.contains("between 0 and")); + } + } +} diff --git a/src/parser/tests.rs b/src/parser/tests.rs index a8f8372a..0607d9cd 100644 --- a/src/parser/tests.rs +++ b/src/parser/tests.rs @@ -2225,3 +2225,69 @@ fn try_statement_parses_finally_and_named_error_binding() { other => panic!("expected a try statement, got {other:?}"), } } + +#[test] +fn pattern_quantifier_rejects_descending_numeric_range() { + let input = "create pattern bounded:\n 10 to 1 digit\nend pattern"; + let tokens = lex_wfl_with_positions(input); + let mut parser = Parser::new(&tokens); + + let error = parser + .parse_statement() + .expect_err("a descending pattern range must be rejected"); + + assert!( + error.message.contains("lower bound") && error.message.contains("upper bound"), + "unexpected error: {error}" + ); +} + +#[test] +fn pattern_quantifier_rejects_counts_outside_u32_range() { + for body in [ + "exactly 4294967296 digit", + "exactly 9223372036854775807 digit", + "1 to 4294967296 digit", + "digit exactly 4294967296", + "digit between 1 and 4294967296", + ] { + let input = format!("create pattern bounded:\n {body}\nend pattern"); + let tokens = lex_wfl_with_positions(&input); + let mut parser = Parser::new(&tokens); + + let error = parser + .parse_statement() + .expect_err("out-of-range quantifier must be rejected"); + assert!( + error.message.contains("between 0 and"), + "unexpected error for `{body}`: {error}" + ); + } +} + +#[test] +fn normal_pattern_quantifier_counts_remain_compatible() { + let cases = [ + ("exactly 3 digit", Quantifier::Exactly(3)), + ("2 to 6 digit", Quantifier::Between(2, 6)), + ("digit exactly 4", Quantifier::Exactly(4)), + ("digit between 1 and 5", Quantifier::Between(1, 5)), + ]; + + for (body, expected_quantifier) in cases { + let input = format!("create pattern bounded:\n {body}\nend pattern"); + let tokens = lex_wfl_with_positions(&input); + let mut parser = Parser::new(&tokens); + let statement = parser + .parse_statement() + .unwrap_or_else(|error| panic!("expected `{body}` to parse: {error}")); + + let Statement::PatternDefinition { pattern, .. } = statement else { + panic!("expected a pattern definition for `{body}`"); + }; + let PatternExpression::Quantified { quantifier, .. } = pattern else { + panic!("expected a quantified pattern for `{body}`, got {pattern:?}"); + }; + assert_eq!(quantifier, expected_quantifier, "body: `{body}`"); + } +} diff --git a/src/pattern/compiler.rs b/src/pattern/compiler.rs index 7426c579..6a8075c2 100644 --- a/src/pattern/compiler.rs +++ b/src/pattern/compiler.rs @@ -10,6 +10,14 @@ use crate::interpreter::environment::Environment; use crate::interpreter::value::Value; use crate::parser::ast::{Anchor, CharClass, PatternExpression, Quantifier}; use std::collections::HashMap; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A second line of defense beyond the VM's transition budget. Bytecode +/// compilation expands bounded quantifiers, so it must have a substantially +/// smaller ceiling of its own to keep hostile source from allocating millions +/// of instructions before the VM starts. +const MAX_COMPILED_PATTERN_INSTRUCTIONS: usize = 100_000; /// Compiler that converts PatternExpression AST into executable bytecode. /// @@ -62,6 +70,11 @@ pub struct PatternCompiler { capture_map: HashMap, /// Counter for save slots (currently unused but preserved for future use) save_counter: usize, + /// Maximum number of instructions this compilation may emit. + max_instructions: usize, + /// Instructions emitted across the root program and embedded lookbehind + /// programs. Lookbehind compilers share this counter with their parent. + emitted_instructions: Arc, } impl PatternCompiler { @@ -70,14 +83,63 @@ impl PatternCompiler { /// The compiler starts with an empty program and no capture groups. /// Each compiler instance should only be used to compile a single pattern. pub fn new() -> Self { + Self::with_instruction_limit(MAX_COMPILED_PATTERN_INSTRUCTIONS) + } + + fn with_instruction_limit(max_instructions: usize) -> Self { + Self::with_shared_instruction_limit(max_instructions, Arc::new(AtomicUsize::new(0))) + } + + fn with_shared_instruction_limit( + max_instructions: usize, + emitted_instructions: Arc, + ) -> Self { Self { program: Program::new(), capture_names: Vec::new(), capture_map: HashMap::new(), save_counter: 0, + max_instructions, + emitted_instructions, } } + fn instruction_limit_error(&self) -> PatternError { + PatternError::CompileError(format!( + "Pattern bytecode exceeds the compilation instruction limit ({})", + self.max_instructions + )) + } + + fn checked_instruction_add(&self, left: usize, right: usize) -> Result { + left.checked_add(right) + .filter(|total| *total <= self.max_instructions) + .ok_or_else(|| self.instruction_limit_error()) + } + + fn checked_instruction_mul(&self, left: usize, right: usize) -> Result { + left.checked_mul(right) + .filter(|total| *total <= self.max_instructions) + .ok_or_else(|| self.instruction_limit_error()) + } + + fn ensure_instruction_capacity(&self, additional: usize) -> Result<(), PatternError> { + self.emitted_instructions + .load(Ordering::Relaxed) + .checked_add(additional) + .filter(|total| *total <= self.max_instructions) + .map(|_| ()) + .ok_or_else(|| self.instruction_limit_error()) + } + + fn emit_instruction(&mut self, instruction: Instruction) -> Result<(), PatternError> { + self.ensure_instruction_capacity(1)?; + self.program.push(instruction); + let previous = self.emitted_instructions.fetch_add(1, Ordering::Relaxed); + debug_assert!(previous < self.max_instructions); + Ok(()) + } + /// Compile a PatternExpression AST into executable bytecode. /// /// This is the main entry point for compilation. It recursively processes @@ -110,8 +172,9 @@ impl PatternCompiler { /// # } /// ``` pub fn compile(&mut self, pattern: &PatternExpression) -> Result { + self.ensure_pattern_fits(pattern)?; self.compile_expression(pattern)?; - self.program.push(Instruction::Match); + self.emit_instruction(Instruction::Match)?; // Set metadata self.program.set_num_captures(self.capture_names.len()); @@ -152,8 +215,10 @@ impl PatternCompiler { pattern: &PatternExpression, env: &Environment, ) -> Result { - self.compile_expression_with_env(pattern, env)?; - self.program.push(Instruction::Match); + let resolved_pattern = self.resolve_list_references(pattern, env)?; + self.ensure_pattern_fits(&resolved_pattern)?; + self.compile_expression(&resolved_pattern)?; + self.emit_instruction(Instruction::Match)?; // Set metadata self.program.set_num_captures(self.capture_names.len()); @@ -173,6 +238,105 @@ impl PatternCompiler { self.capture_names.clone() } + /// Preflight bytecode growth before entering any quantifier loop. All + /// arithmetic is checked, so even multiply-nested `u32::MAX` counts become + /// a normal compile error rather than wrapping or iterating to exhaustion. + fn ensure_pattern_fits(&self, pattern: &PatternExpression) -> Result<(), PatternError> { + let body_instructions = self.estimate_pattern_instructions(pattern)?; + let total_instructions = self.checked_instruction_add(body_instructions, 1)?; // Match + self.ensure_instruction_capacity(total_instructions) + } + + fn estimate_pattern_instructions( + &self, + pattern: &PatternExpression, + ) -> Result { + match pattern { + PatternExpression::Literal(text) => Ok(if text.is_empty() { 0 } else { 1 }), + PatternExpression::CharacterClass(_) + | PatternExpression::Backreference(_) + | PatternExpression::Anchor(_) => Ok(1), + PatternExpression::ListReference(_) => Ok(0), + PatternExpression::Sequence(patterns) => { + let mut total = 0; + for pattern in patterns { + total = self.checked_instruction_add( + total, + self.estimate_pattern_instructions(pattern)?, + )?; + } + Ok(total) + } + PatternExpression::Alternative(patterns) => { + let mut total = 0; + for pattern in patterns { + total = self.checked_instruction_add( + total, + self.estimate_pattern_instructions(pattern)?, + )?; + } + let branch_instructions = + self.checked_instruction_mul(patterns.len().saturating_sub(1), 2)?; + self.checked_instruction_add(total, branch_instructions) + } + PatternExpression::Quantified { + pattern, + quantifier, + } => { + if let Quantifier::Between(min, max) = quantifier + && min > max + { + return Err(Self::invalid_range_error(*min, *max)); + } + let inner = self.estimate_pattern_instructions(pattern)?; + // Repeating an empty expression still performs one compiler + // iteration per count, so charge at least one expansion unit. + let expansion_unit = inner.max(1); + match quantifier { + Quantifier::Optional => self.checked_instruction_add(inner, 1), + Quantifier::ZeroOrMore => self.checked_instruction_add(inner, 2), + Quantifier::OneOrMore => { + let repeated = self.checked_instruction_mul(inner, 2)?; + self.checked_instruction_add(repeated, 2) + } + Quantifier::Exactly(count) => { + self.checked_instruction_mul(expansion_unit, *count as usize) + } + Quantifier::Between(min, max) => { + let repeated = + self.checked_instruction_mul(expansion_unit, *max as usize)?; + self.checked_instruction_add(repeated, (*max - *min) as usize) + } + Quantifier::AtLeast(count) => { + let repetitions = (*count as usize) + .checked_add(1) + .ok_or_else(|| self.instruction_limit_error())?; + let repeated = self.checked_instruction_mul(expansion_unit, repetitions)?; + self.checked_instruction_add(repeated, 2) + } + Quantifier::AtMost(count) => { + let one_optional = self.checked_instruction_add(inner, 1)?; + self.checked_instruction_mul(one_optional, *count as usize) + } + } + } + PatternExpression::Capture { pattern, .. } + | PatternExpression::Lookahead(pattern) + | PatternExpression::NegativeLookahead(pattern) + | PatternExpression::Lookbehind(pattern) + | PatternExpression::NegativeLookbehind(pattern) => { + let inner = self.estimate_pattern_instructions(pattern)?; + self.checked_instruction_add(inner, 2) + } + } + } + + fn invalid_range_error(min: u32, max: u32) -> PatternError { + PatternError::CompileError(format!( + "Pattern quantifier lower bound ({min}) cannot exceed upper bound ({max})" + )) + } + /// Compile a single pattern expression node recursively. /// /// This is the main dispatch method that handles different AST node types. @@ -246,38 +410,6 @@ impl PatternCompiler { Ok(()) } - /// Compile a single pattern expression node recursively with environment access. - /// - /// This is similar to `compile_expression` but allows resolving list references - /// from the provided environment. List references are resolved at compile time - /// and converted to alternative patterns. - /// - /// # Arguments - /// * `pattern` - The AST node to compile - /// * `env` - Environment containing variable definitions - /// - /// # Returns - /// * `Ok(())` - Node compiled successfully - /// * `Err(PatternError)` - Compilation failed or list not found - fn compile_expression_with_env( - &mut self, - pattern: &PatternExpression, - env: &Environment, - ) -> Result<(), PatternError> { - match pattern { - PatternExpression::ListReference(name) => { - self.compile_list_reference(name, env)?; - } - - // For all other patterns, recursively handle any nested list references - _ => { - let resolved_pattern = self.resolve_list_references(pattern, env)?; - self.compile_expression(&resolved_pattern)?; - } - } - Ok(()) - } - /// Recursively resolve list references in a pattern expression. /// /// This method traverses the pattern AST and replaces any ListReference nodes @@ -393,17 +525,6 @@ impl PatternCompiler { } } - /// Compile a list reference by resolving it from the environment. - fn compile_list_reference( - &mut self, - name: &str, - env: &Environment, - ) -> Result<(), PatternError> { - let resolved = - self.resolve_list_references(&PatternExpression::ListReference(name.to_string()), env)?; - self.compile_expression(&resolved) - } - /// Compile a literal string into matching instructions. /// /// Optimizes single characters to use `Char` instruction for efficiency. @@ -419,10 +540,10 @@ impl PatternCompiler { if text.len() == 1 { // Single character - use Char instruction let ch = text.chars().next().unwrap(); - self.program.push(Instruction::Char(ch)); + self.emit_instruction(Instruction::Char(ch))?; } else { // Multi-character string - use Literal instruction - self.program.push(Instruction::Literal(text.to_string())); + self.emit_instruction(Instruction::Literal(text.to_string()))?; } Ok(()) } @@ -449,7 +570,7 @@ impl PatternCompiler { CharClassType::UnicodeProperty(property.clone()) } }; - self.program.push(Instruction::CharClass(class_type)); + self.emit_instruction(Instruction::CharClass(class_type))?; Ok(()) } @@ -498,13 +619,13 @@ impl PatternCompiler { } else { // Not the last - emit split and compile pattern let split_addr = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; // Jump to end after this alternative succeeds let jump_addr = self.program.len(); - self.program.push(Instruction::Jump(0)); // Will be patched + self.emit_instruction(Instruction::Jump(0))?; // Will be patched jump_to_end.push(jump_addr); // Patch the split to point to the next alternative @@ -543,7 +664,7 @@ impl PatternCompiler { // L2: (continue) let split_addr = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; @@ -566,12 +687,12 @@ impl PatternCompiler { // L3: (continue) let loop_start = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; // Jump back to loop start - self.program.push(Instruction::Jump(loop_start)); + self.emit_instruction(Instruction::Jump(loop_start))?; let end_addr = self.program.len(); @@ -595,12 +716,12 @@ impl PatternCompiler { self.compile_expression(pattern)?; let loop_start = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; // Jump back to loop start - self.program.push(Instruction::Jump(loop_start)); + self.emit_instruction(Instruction::Jump(loop_start))?; let end_addr = self.program.len(); @@ -623,16 +744,20 @@ impl PatternCompiler { Quantifier::Between(min, max) => { // Between min and max: first min required, then up to (max-min) optional + if min > max { + return Err(Self::invalid_range_error(*min, *max)); + } + // Required repetitions for _ in 0..*min { self.compile_expression(pattern)?; } // Optional repetitions - let optional_count = max - min; + let optional_count = *max - *min; for _ in 0..optional_count { let split_addr = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; @@ -658,12 +783,12 @@ impl PatternCompiler { // Then zero or more (same as ZeroOrMore logic) let loop_start = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; // Jump back to loop start - self.program.push(Instruction::Jump(loop_start)); + self.emit_instruction(Instruction::Jump(loop_start))?; let end_addr = self.program.len(); @@ -681,7 +806,7 @@ impl PatternCompiler { for _ in 0..*n { let split_addr = self.program.len(); - self.program.push(Instruction::Split(0, 0)); // Will be patched + self.emit_instruction(Instruction::Split(0, 0))?; // Will be patched self.compile_expression(pattern)?; @@ -718,13 +843,13 @@ impl PatternCompiler { }; // Start capture - self.program.push(Instruction::StartCapture(capture_index)); + self.emit_instruction(Instruction::StartCapture(capture_index))?; // Compile the pattern self.compile_expression(pattern)?; // End capture - self.program.push(Instruction::EndCapture(capture_index)); + self.emit_instruction(Instruction::EndCapture(capture_index))?; Ok(()) } @@ -733,7 +858,7 @@ impl PatternCompiler { fn compile_backreference(&mut self, name: &str) -> Result<(), PatternError> { // Look up the capture index by name if let Some(&capture_index) = self.capture_map.get(name) { - self.program.push(Instruction::Backreference(capture_index)); + self.emit_instruction(Instruction::Backreference(capture_index))?; Ok(()) } else { Err(PatternError::CompileError(format!( @@ -746,10 +871,10 @@ impl PatternCompiler { fn compile_anchor(&mut self, anchor: &Anchor) -> Result<(), PatternError> { match anchor { Anchor::StartOfText => { - self.program.push(Instruction::StartAnchor); + self.emit_instruction(Instruction::StartAnchor)?; } Anchor::EndOfText => { - self.program.push(Instruction::EndAnchor); + self.emit_instruction(Instruction::EndAnchor)?; } } Ok(()) @@ -761,9 +886,9 @@ impl PatternCompiler { // 1. Begin lookahead (saves position) // 2. Compile the pattern to check // 3. End lookahead (restores position if pattern matched) - self.program.push(Instruction::BeginLookahead); + self.emit_instruction(Instruction::BeginLookahead)?; self.compile_expression(pattern)?; - self.program.push(Instruction::EndLookahead); + self.emit_instruction(Instruction::EndLookahead)?; Ok(()) } @@ -776,23 +901,26 @@ impl PatternCompiler { // 1. Begin negative lookahead (saves position) // 2. Compile the pattern to check // 3. End negative lookahead (restores position if pattern didn't match) - self.program.push(Instruction::BeginNegativeLookahead); + self.emit_instruction(Instruction::BeginNegativeLookahead)?; self.compile_expression(pattern)?; - self.program.push(Instruction::EndNegativeLookahead); + self.emit_instruction(Instruction::EndNegativeLookahead)?; Ok(()) } /// Compile a positive lookbehind fn compile_lookbehind(&mut self, pattern: &PatternExpression) -> Result<(), PatternError> { // Create a separate program for the lookbehind pattern - let mut lookbehind_compiler = PatternCompiler::new(); + let mut lookbehind_compiler = PatternCompiler::with_shared_instruction_limit( + self.max_instructions, + Arc::clone(&self.emitted_instructions), + ); lookbehind_compiler.compile_expression(pattern)?; - lookbehind_compiler.program.push(Instruction::Match); + lookbehind_compiler.emit_instruction(Instruction::Match)?; // Embed the lookbehind program in the instruction - self.program.push(Instruction::CheckLookbehind(Box::new( + self.emit_instruction(Instruction::CheckLookbehind(Box::new( lookbehind_compiler.program, - ))); + )))?; Ok(()) } @@ -803,15 +931,17 @@ impl PatternCompiler { pattern: &PatternExpression, ) -> Result<(), PatternError> { // Create a separate program for the negative lookbehind pattern - let mut lookbehind_compiler = PatternCompiler::new(); + let mut lookbehind_compiler = PatternCompiler::with_shared_instruction_limit( + self.max_instructions, + Arc::clone(&self.emitted_instructions), + ); lookbehind_compiler.compile_expression(pattern)?; - lookbehind_compiler.program.push(Instruction::Match); + lookbehind_compiler.emit_instruction(Instruction::Match)?; // Embed the lookbehind program in the instruction - self.program - .push(Instruction::CheckNegativeLookbehind(Box::new( - lookbehind_compiler.program, - ))); + self.emit_instruction(Instruction::CheckNegativeLookbehind(Box::new( + lookbehind_compiler.program, + )))?; Ok(()) } @@ -958,4 +1088,104 @@ mod tests { assert_eq!(program.instructions[2], Instruction::EndCapture(0)); assert_eq!(program.instructions[3], Instruction::Match); } + + fn quantified(pattern: PatternExpression, quantifier: Quantifier) -> PatternExpression { + PatternExpression::Quantified { + pattern: Box::new(pattern), + quantifier, + } + } + + fn assert_compile_error_without_panic(pattern: PatternExpression, expected: &str) { + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut compiler = PatternCompiler::with_instruction_limit(64); + compiler.compile(&pattern) + })); + let compile_result = result.expect("invalid quantifier must not panic"); + let error = compile_result.expect_err("invalid quantifier must not compile"); + assert!( + error.to_string().contains(expected), + "unexpected error: {error}" + ); + } + + #[test] + fn descending_between_quantifier_returns_compile_error_without_panicking() { + assert_compile_error_without_panic( + quantified( + PatternExpression::CharacterClass(CharClass::Digit), + Quantifier::Between(10, 1), + ), + "lower bound", + ); + } + + #[test] + fn huge_quantifiers_hit_compile_limit_before_expansion() { + for quantifier in [ + Quantifier::Exactly(u32::MAX), + Quantifier::Between(u32::MAX - 1, u32::MAX), + Quantifier::AtLeast(u32::MAX), + Quantifier::AtMost(u32::MAX), + ] { + assert_compile_error_without_panic( + quantified( + PatternExpression::CharacterClass(CharClass::Digit), + quantifier, + ), + "instruction limit", + ); + } + + assert_compile_error_without_panic( + quantified( + PatternExpression::Literal(String::new()), + Quantifier::Exactly(u32::MAX), + ), + "instruction limit", + ); + } + + #[test] + fn quantifier_instruction_estimate_overflow_is_a_compile_error() { + let repeated = quantified( + quantified( + quantified( + PatternExpression::CharacterClass(CharClass::Digit), + Quantifier::Exactly(u32::MAX), + ), + Quantifier::Exactly(u32::MAX), + ), + Quantifier::Exactly(u32::MAX), + ); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut compiler = PatternCompiler::with_instruction_limit(usize::MAX); + compiler.compile(&repeated) + })); + let compile_result = result.expect("instruction estimate overflow must not panic"); + let error = compile_result.expect_err("overflowing instruction estimate must be rejected"); + assert!(error.to_string().contains("instruction limit")); + } + + #[test] + fn normal_exact_and_range_quantifiers_keep_their_bytecode_shape() { + let mut exact_compiler = PatternCompiler::with_instruction_limit(64); + let exact = exact_compiler + .compile(&quantified( + PatternExpression::CharacterClass(CharClass::Digit), + Quantifier::Exactly(3), + )) + .unwrap(); + assert_eq!(exact.instructions.len(), 4); // Three classes + Match. + + let mut range_compiler = PatternCompiler::with_instruction_limit(64); + let range = range_compiler + .compile(&quantified( + PatternExpression::CharacterClass(CharClass::Digit), + Quantifier::Between(2, 4), + )) + .unwrap(); + assert_eq!(range.instructions.len(), 7); // Two required, two optional, Match. + } } diff --git a/src/stdlib/filesystem.rs b/src/stdlib/filesystem.rs index 027baca5..fe337c26 100644 --- a/src/stdlib/filesystem.rs +++ b/src/stdlib/filesystem.rs @@ -1,10 +1,12 @@ use super::helpers::{ check_arg_count, check_arg_range, expect_text, unary_path_bool_op, unary_path_string_op, }; -use crate::interpreter::error::RuntimeError; +use crate::exec::budget::ExecutionBudget; +use crate::interpreter::error::{ErrorKind, RuntimeError}; use crate::interpreter::value::Value; use std::cell::RefCell; use std::fs; +use std::io::Read; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::sync::Arc; @@ -238,8 +240,31 @@ pub fn native_count_lines(args: Vec) -> Result { )); } - let content = fs::read_to_string(path) + let budget = ExecutionBudget::current_or_default(); + let limit = budget.max_file_read_bytes(); + let probe_size = limit.saturating_add(1); + let probe_size_u64 = u64::try_from(probe_size).unwrap_or(u64::MAX); + let file = fs::File::open(path) .map_err(|e| RuntimeError::new(format!("Failed to read file '{path_str}': {e}"), 0, 0))?; + let mut bytes = Vec::with_capacity(limit.min(8 * 1024)); + file.take(probe_size_u64) + .read_to_end(&mut bytes) + .map_err(|e| RuntimeError::new(format!("Failed to read file '{path_str}': {e}"), 0, 0))?; + budget + .check_file_read_bytes(bytes.len()) + .map_err(|exceeded| { + RuntimeError::with_kind(exceeded.message(), 0, 0, ErrorKind::ResourceLimit) + })?; + let content = String::from_utf8(bytes).map_err(|e| { + RuntimeError::new( + format!( + "Failed to read file '{path_str}': stream did not contain valid UTF-8: {}", + e.utf8_error() + ), + 0, + 0, + ) + })?; // Count lines by splitting on newline characters // Handle edge case: empty file has 0 lines @@ -818,6 +843,29 @@ mod tests { assert!(result.unwrap_err().message.contains("not a file")); } + #[test] + fn test_native_count_lines_obeys_file_read_budget() { + use crate::exec::budget::BudgetLimits; + + let temp_dir = TempDir::new().unwrap(); + let test_file_path = temp_dir.path().join("too_large.txt"); + std::fs::write(&test_file_path, b"123456789").unwrap(); + + let limits = BudgetLimits { + max_file_read_bytes: 8, + ..BudgetLimits::default() + }; + let budget = Arc::new(ExecutionBudget::new(limits)); + let _guard = ExecutionBudget::enter(budget); + let result = native_count_lines(vec![Value::Text(Arc::from( + test_file_path.to_string_lossy().as_ref(), + ))]); + + let error = result.expect_err("count_lines must share the file-read ceiling"); + assert_eq!(error.kind, ErrorKind::ResourceLimit); + assert!(error.message.contains("File read too large")); + } + // Tests for path_extension #[test] fn test_native_path_extension_with_ext() { diff --git a/src/transpiler/javascript.rs b/src/transpiler/javascript.rs index bdfb0abf..79e9ba9e 100644 --- a/src/transpiler/javascript.rs +++ b/src/transpiler/javascript.rs @@ -965,7 +965,11 @@ impl JavaScriptTranspiler { result.push_str(&format!("{}super();\n", self.indent())); } result.push_str(&format!("{}this._wfl_container = true;\n", self.indent())); - result.push_str(&format!("{}this._wfl_type = '{}';\n", self.indent(), name)); + result.push_str(&format!( + "{}this._wfl_type = {};\n", + self.indent(), + js_string_literal(name) + )); // Initialize properties for prop in properties { @@ -1174,9 +1178,9 @@ impl JavaScriptTranspiler { .collect::, _>>()? .join(", "); Ok(format!( - "{}this.dispatchEvent(new CustomEvent('{}', {{ detail: [{}] }}));\n", + "{}this.dispatchEvent(new CustomEvent({}, {{ detail: [{}] }}));\n", self.indent(), - name, + js_string_literal(name), args )) } @@ -1189,10 +1193,10 @@ impl JavaScriptTranspiler { } => { let source = self.transpile_expression(event_source)?; let mut result = format!( - "{}{}.addEventListener('{}', (event) => {{\n", + "{}{}.addEventListener({}, (event) => {{\n", self.indent(), source, - event_name + js_string_literal(event_name) ); self.push_indent(); for s in handler_body { @@ -1411,9 +1415,9 @@ impl JavaScriptTranspiler { _ => signal_type, }; Ok(format!( - "{}process.on('{}', {});\n", + "{}process.on({}, {});\n", self.indent(), - signal, + js_string_literal(signal), self.sanitize_name(handler_name) )) } @@ -1439,7 +1443,7 @@ impl JavaScriptTranspiler { let mut result = format!( "{}describe({}, function() {{\n", self.indent(), - self.escape_string(description) + js_string_literal(description) ); self.push_indent(); // Transpile setup (beforeEach) @@ -1477,7 +1481,7 @@ impl JavaScriptTranspiler { let mut result = format!( "{}it({}, function() {{\n", self.indent(), - self.escape_string(description) + js_string_literal(description) ); self.push_indent(); for stmt in body { @@ -1572,7 +1576,7 @@ impl JavaScriptTranspiler { "{}expect(typeof {}).toBe({});\n", self.indent(), subject_expr, - self.escape_string(type_name) + js_string_literal(type_name) )), } } @@ -1776,17 +1780,18 @@ impl JavaScriptTranspiler { // Request variable from WaitForRequest resolves to { request, response } // We use an IIFE to cache the request variable evaluation. Ok(format!( - "(() => {{ const __req = {}; return (__req.request || __req).headers['{}']; }})()", + "(() => {{ const __req = {}; return (__req.request || __req).headers[{}]; }})()", req, - header_name.to_lowercase() + js_string_literal(&header_name.to_lowercase()) )) } Expression::CurrentTimeMilliseconds { .. } => Ok("Date.now()".to_string()), - Expression::CurrentTimeFormatted { format, .. } => { - Ok(format!("WFL.time.format(new Date(), '{}')", format)) - } + Expression::CurrentTimeFormatted { format, .. } => Ok(format!( + "WFL.time.format(new Date(), {})", + js_string_literal(format) + )), Expression::FileExists { path, .. } => { let p = self.transpile_expression(path)?; @@ -1875,7 +1880,7 @@ impl JavaScriptTranspiler { /// Transpile a literal to JavaScript fn transpile_literal(&self, lit: &Literal) -> Result { match lit { - Literal::String(s) => Ok(format!("\"{}\"", self.escape_string(s))), + Literal::String(s) => Ok(js_string_literal(s)), Literal::Integer(i) => Ok(i.to_string()), Literal::Float(f) => Ok(f.to_string()), Literal::Boolean(b) => Ok(if *b { "true" } else { "false" }.to_string()), @@ -2029,7 +2034,7 @@ impl JavaScriptTranspiler { /// Convert a simple pattern string to a regex fn pattern_to_regex(&self, pattern: &str) -> String { // For simple patterns, just escape regex special characters - format!("\"{}\"", regex_escape(pattern)) + js_string_literal(®ex_escape(pattern)) } /// Check if a list of statements contains any async operations @@ -2188,15 +2193,6 @@ impl JavaScriptTranspiler { result } - /// Escape a string for JavaScript - fn escape_string(&self, s: &str) -> String { - s.replace('\\', "\\\\") - .replace('"', "\\\"") - .replace('\n', "\\n") - .replace('\r', "\\r") - .replace('\t', "\\t") - } - /// Escape a key for JavaScript object literal fn escape_key(&self, key: &str) -> String { // Check if key is a valid identifier @@ -2210,7 +2206,7 @@ impl JavaScriptTranspiler { if is_valid_identifier { key.to_string() } else { - format!("\"{}\"", self.escape_string(key)) + js_string_literal(key) } } } @@ -2226,17 +2222,74 @@ impl Clone for JavaScriptTranspiler { } } -/// Escape special regex characters in a string +/// Encode a JavaScript string literal without allowing its contents to alter +/// the surrounding generated program. +fn js_string_literal(s: &str) -> String { + let mut result = String::with_capacity(s.len() + 2); + result.push('"'); + for c in s.chars() { + match c { + '"' => result.push_str("\\\""), + '\\' => result.push_str("\\\\"), + '\n' => result.push_str("\\n"), + '\r' => result.push_str("\\r"), + '\t' => result.push_str("\\t"), + '\u{0008}' => result.push_str("\\b"), + '\u{000C}' => result.push_str("\\f"), + '\u{2028}' => result.push_str("\\u2028"), + '\u{2029}' => result.push_str("\\u2029"), + c if c.is_control() => { + use std::fmt::Write; + write!(&mut result, "\\u{:04x}", c as u32) + .expect("writing to a String cannot fail"); + } + c => result.push(c), + } + } + result.push('"'); + result +} + +/// Escape special regex characters and regex-literal delimiters in a string. fn regex_escape(s: &str) -> String { let special_chars = [ '.', '*', '+', '?', '^', '$', '{', '}', '[', ']', '(', ')', '|', '\\', ]; let mut result = String::with_capacity(s.len() * 2); for c in s.chars() { - if special_chars.contains(&c) { - result.push('\\'); + match c { + '/' => result.push_str("\\/"), + '\n' => result.push_str("\\n"), + '\r' => result.push_str("\\r"), + '\u{2028}' => result.push_str("\\u2028"), + '\u{2029}' => result.push_str("\\u2029"), + c if special_chars.contains(&c) => { + result.push('\\'); + result.push(c); + } + c => result.push(c), } - result.push(c); } result } + +#[cfg(test)] +mod security_tests { + use super::{js_string_literal, regex_escape}; + + #[test] + fn javascript_string_literal_escapes_code_boundaries_and_line_separators() { + assert_eq!( + js_string_literal("\"');\n\r\t\u{0000}\u{2028}\u{2029}\\"), + "\"\\\"');\\n\\r\\t\\u0000\\u2028\\u2029\\\\\"" + ); + } + + #[test] + fn regex_literal_text_cannot_close_the_generated_literal() { + assert_eq!( + regex_escape("safe/;globalThis.pwned=true;//\n"), + r"safe\/;globalThis\.pwned=true;\/\/\n" + ); + } +} diff --git a/src/wfl_config/checker.rs b/src/wfl_config/checker.rs index 69c072df..29c8a3af 100644 --- a/src/wfl_config/checker.rs +++ b/src/wfl_config/checker.rs @@ -608,6 +608,12 @@ impl ConfigChecker { "Execution Budget", "Maximum WFL source-file size in bytes (default 64 MiB, min 1)", ); + int_setting( + "max_file_read_size", + "52428800", + "Execution Budget", + "Maximum bytes buffered by one text or binary file read (default 50 MiB, min 1)", + ); } Self { expected_settings } @@ -1109,7 +1115,8 @@ fn integer_min_for_key(key: &str) -> Option { | "max_execute_file_depth" | "max_pattern_steps" | "max_pattern_states" - | "max_source_size" => Some(1), + | "max_source_size" + | "max_file_read_size" => Some(1), _ => None, } } @@ -1163,6 +1170,7 @@ max_line_length = 80 "max_pattern_steps", "max_pattern_states", "max_source_size", + "max_file_read_size", "web_server_max_response_size", "web_server_response_timeout_seconds", "web_server_request_queue_bound", @@ -1182,6 +1190,7 @@ max_execute_file_depth = 6 max_pattern_steps = 250000 max_pattern_states = 5000 max_source_size = 1048576 +max_file_read_size = 2097152 web_server_max_response_size = 5242880 web_server_response_timeout_seconds = 30 web_server_request_queue_bound = 512 @@ -1219,7 +1228,10 @@ web_socket_max_queued_bytes = 33554432 let config_path = temp_dir.path().join(".wflcfg"); fs::write( &config_path, - "max_operations = -1\nmax_call_depth = 0\nmax_source_size = 4096\n", + concat!( + "max_operations = -1\nmax_call_depth = 0\n", + "max_source_size = 4096\nmax_file_read_size = 0\n" + ), ) .unwrap(); @@ -1240,6 +1252,10 @@ web_socket_max_queued_bytes = 33554432 !bad.contains("max_source_size"), "a valid max_source_size must not be flagged; issues: {issues:?}" ); + assert!( + bad.contains("max_file_read_size"), + "zero max_file_read_size must be rejected; issues: {issues:?}" + ); // The pre-existing positive-only keys the loader clamps/rejects at 0 are // now enforced by the checker too. diff --git a/tests/execution_budget_test.rs b/tests/execution_budget_test.rs index 0e34979b..1d680c0b 100644 --- a/tests/execution_budget_test.rs +++ b/tests/execution_budget_test.rs @@ -58,6 +58,7 @@ fn budget_keys_use_documented_defaults() { assert_eq!(cfg.max_pattern_steps, 5_000_000); assert_eq!(cfg.max_pattern_states, 10_000); assert_eq!(cfg.max_source_size, 64 * 1024 * 1024); + assert_eq!(cfg.max_file_read_size, 50 * 1024 * 1024); assert_eq!(cfg.web_server_max_response_size, 64 * 1024 * 1024); assert_eq!(cfg.web_socket_queue_bound, 1_024); assert_eq!(cfg.web_socket_max_connections, 1_024); @@ -72,6 +73,7 @@ fn budget_keys_accept_overrides() { max_pattern_steps = 5000\n\ max_pattern_states = 500\n\ max_source_size = 4096\n\ + max_file_read_size = 8192\n\ web_server_max_response_size = 2048\n\ web_socket_queue_bound = 32\n\ web_socket_max_connections = 16\n", @@ -82,6 +84,7 @@ fn budget_keys_accept_overrides() { assert_eq!(cfg.max_pattern_steps, 5000); assert_eq!(cfg.max_pattern_states, 500); assert_eq!(cfg.max_source_size, 4096); + assert_eq!(cfg.max_file_read_size, 8192); assert_eq!(cfg.web_server_max_response_size, 2048); assert_eq!(cfg.web_socket_queue_bound, 32); assert_eq!(cfg.web_socket_max_connections, 16); @@ -99,9 +102,11 @@ fn max_operations_zero_means_unlimited() { #[test] fn zero_and_garbage_budget_values_keep_defaults() { // The positive-integer keys reject 0 and non-numeric input, keeping defaults. - let cfg = load_with_cfg("max_call_depth = 0\nmax_pattern_states = nope\n"); + let cfg = + load_with_cfg("max_call_depth = 0\nmax_pattern_states = nope\nmax_file_read_size = 0\n"); assert_eq!(cfg.max_call_depth, 1_000); assert_eq!(cfg.max_pattern_states, 10_000); + assert_eq!(cfg.max_file_read_size, 50 * 1024 * 1024); } // --- end-to-end enforcement ------------------------------------------------ @@ -172,6 +177,42 @@ fn oversized_source_is_refused() { ); } +#[test] +fn oversized_text_file_read_is_a_resource_error() { + let binary = test_helpers::get_wfl_binary_path(); + let dir = tempfile::tempdir().expect("create temp dir"); + fs::write(dir.path().join(".wflcfg"), "max_file_read_size = 8\n").expect("cfg"); + let payload = dir.path().join("payload.txt"); + fs::write(&payload, b"123456789").expect("payload"); + let script = dir.path().join("program.wfl"); + fs::write( + &script, + format!( + concat!( + "open file at \"{}\" for reading as reader\n", + "wait for store file_text as read content from reader\n" + ), + wfl_path(&payload) + ), + ) + .expect("program"); + + let output = Command::new(binary) + .arg(&script) + .output() + .expect("run wfl binary"); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + combined.contains("File read too large: 9 bytes (limit: 8 bytes)"), + "expected the file-read ceiling diagnostic; got:\n{combined}" + ); + assert!(!output.status.success(), "an oversized read must fail"); +} + #[test] fn catching_a_recursion_limit_leaves_a_consistent_interpreter() { // A caught call-depth ResourceLimit must not corrupt the interpreter: the diff --git a/tests/http_outbound_budget_test.rs b/tests/http_outbound_budget_test.rs new file mode 100644 index 00000000..e8a1a6f5 --- /dev/null +++ b/tests/http_outbound_budget_test.rs @@ -0,0 +1,298 @@ +//! Security regressions for bounded outbound HTTP responses. +//! +//! These tests use a minimal local TCP peer so they are deterministic and do +//! not require internet access. Together they exercise all three runtime paths: +//! legacy GET, legacy POST, and the arbitrary-method/full-response statement. + +use std::sync::Arc; +use std::time::Duration; + +use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::net::{TcpListener, TcpStream}; +use tokio::sync::oneshot; + +use wfl::config::WflConfig; +use wfl::exec::budget::{BudgetLimits, ExecutionBudget}; +use wfl::interpreter::Interpreter; +use wfl::interpreter::error::ErrorKind; +use wfl::lexer::lex_wfl_with_positions; +use wfl::parser::Parser; +use wfl::parser::ast::{Expression, Literal, Program, Statement}; + +fn parse(source: &str) -> Program { + let tokens = lex_wfl_with_positions(source); + Parser::new(&tokens) + .parse() + .unwrap_or_else(|errors| panic!("WFL source should parse: {errors:?}")) +} + +async fn read_request_headers(socket: &mut TcpStream) { + let mut request = Vec::new(); + let mut chunk = [0_u8; 1024]; + loop { + let read = socket + .read(&mut chunk) + .await + .expect("read local HTTP request"); + assert!(read > 0, "client closed before completing HTTP headers"); + request.extend_from_slice(&chunk[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + return; + } + assert!( + request.len() <= 64 * 1024, + "unexpectedly large test request" + ); + } +} + +/// Spawn a one-shot HTTP peer that writes `response_prefix`. When `stall` is +/// true it keeps the connection open afterward instead of completing the body. +async fn spawn_http_peer( + response_prefix: &'static [u8], + stall: bool, +) -> ( + String, + tokio::task::JoinHandle<()>, + oneshot::Receiver>, +) { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind local HTTP peer"); + let address = listener.local_addr().expect("local HTTP peer address"); + let (response_attempted, response_attempted_rx) = oneshot::channel(); + let handle = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.expect("accept HTTP request"); + read_request_headers(&mut socket).await; + let write_result = socket + .write_all(response_prefix) + .await + .map_err(|error| error.kind()); + let _ = response_attempted.send(write_result); + if stall { + std::future::pending::<()>().await; + } + let _ = socket.shutdown().await; + }); + (format!("http://{address}"), handle, response_attempted_rx) +} + +async fn await_http_peer(server: tokio::task::JoinHandle<()>) { + tokio::time::timeout(Duration::from_secs(1), server) + .await + .expect("local HTTP peer must not hang") + .expect("local HTTP peer task must not panic"); +} + +fn assert_response_limit(errors: &[wfl::interpreter::error::RuntimeError], limit: usize) { + let error = errors.first().expect("one runtime error"); + assert_eq!(error.kind, ErrorKind::ResourceLimit); + assert!( + error.message.contains("Response body too large"), + "expected response-size diagnostic, got: {error:?}" + ); + assert!( + error.message.contains(&format!("limit: {limit} bytes")), + "diagnostic should include the configured limit: {error:?}" + ); +} + +#[tokio::test] +async fn legacy_get_rejects_oversized_content_length() { + let response = b"HTTP/1.1 200 OK\r\n\ +Content-Type: text/plain\r\n\ +Content-Length: 32\r\n\ +Connection: close\r\n\ +\r\n\ +0123456789abcdef0123456789abcdef"; + let (url, server, _response_attempted) = spawn_http_peer(response, false).await; + + let config = WflConfig { + web_server_max_response_size: 8, + ..Default::default() + }; + let mut interpreter = Interpreter::with_config(Arc::new(config)); + let program = parse(&format!( + r#"open url at "{url}" and read content as content"# + )); + + let errors = interpreter + .interpret(&program) + .await + .expect_err("advertised response above the cap must fail"); + assert_response_limit(&errors, 8); + await_http_peer(server).await; +} + +#[tokio::test] +async fn full_response_request_rejects_oversized_chunked_body() { + // The response has no Content-Length, so only incremental accounting can + // catch that the decoded body grows from five to nine bytes over an 8-byte + // cap. + let response = b"HTTP/1.1 200 OK\r\n\ +Content-Type: text/plain\r\n\ +Transfer-Encoding: chunked\r\n\ +Connection: close\r\n\ +\r\n\ +5\r\n12345\r\n\ +4\r\n6789\r\n\ +0\r\n\r\n"; + let (url, server, _response_attempted) = spawn_http_peer(response, false).await; + + let config = WflConfig { + web_server_max_response_size: 8, + ..Default::default() + }; + let mut interpreter = Interpreter::with_config(Arc::new(config)); + let program = parse(&format!( + r#"open url at "{url}" and read response as reply"# + )); + + let errors = interpreter + .interpret(&program) + .await + .expect_err("chunked response above the cap must fail"); + assert_response_limit(&errors, 8); + await_http_peer(server).await; +} + +#[tokio::test] +async fn decoded_text_cannot_expand_past_the_response_limit() { + // Four malformed UTF-8 bytes decode to four three-byte replacement + // characters. The wire body fits the four-byte limit; the decoded text + // must still be rejected before it can expand beyond that same ceiling. + let response = b"HTTP/1.1 200 OK\r\n\ +Content-Type: text/plain; charset=utf-8\r\n\ +Content-Length: 4\r\n\ +Connection: close\r\n\ +\r\n\ +\xff\xff\xff\xff"; + let (url, server, _response_attempted) = spawn_http_peer(response, false).await; + + let config = WflConfig { + web_server_max_response_size: 4, + ..Default::default() + }; + let mut interpreter = Interpreter::with_config(Arc::new(config)); + let program = parse(&format!( + r#"open url at "{url}" and read content as content"# + )); + + let errors = interpreter + .interpret(&program) + .await + .expect_err("decoded response expansion above the cap must fail"); + assert_response_limit(&errors, 4); + await_http_peer(server).await; +} + +#[tokio::test] +async fn legacy_post_body_read_observes_cooperative_cancellation() { + // Headers promise five bytes, but the peer never sends them. Before the + // fix, Response::text() could remain parked here indefinitely. + let response = b"HTTP/1.1 200 OK\r\n\ +Content-Type: text/plain\r\n\ +Content-Length: 5\r\n\ +Connection: close\r\n\ +\r\n"; + let (url, server, response_attempted) = spawn_http_peer(response, true).await; + + let mut interpreter = Interpreter::new(); + let budget = interpreter.budget(); + // Construct the legacy node directly: `with data` is no longer accepted by + // the current grammar, but embedded/previously parsed programs still reach + // the dedicated HttpPostStatement execution path. + let program = Program { + statements: vec![Statement::HttpPostStatement { + url: Expression::Literal(Literal::String(url.into()), 1, 1), + data: Expression::Literal(Literal::String("x=1".into()), 1, 1), + variable_name: "reply".to_string(), + line: 1, + column: 1, + }], + }; + let interpret = interpreter.interpret(&program); + tokio::pin!(interpret); + tokio::time::timeout(Duration::from_secs(1), async { + tokio::select! { + result = response_attempted => { + assert_eq!( + result.expect("local HTTP peer reports its response write"), + Ok(()), + "the cancellation regression must reach the stalled body read" + ); + budget.cancel(); + } + result = &mut interpret => { + panic!("request completed before the peer entered its stalled body: {result:?}"); + } + } + }) + .await + .expect("request must reach the peer's stalled response promptly"); + + let errors = tokio::time::timeout(Duration::from_secs(1), &mut interpret) + .await + .expect("in-flight POST should observe cancellation promptly") + .expect_err("cancelled POST must fail"); + let error = errors.first().expect("one runtime error"); + assert_eq!(error.kind, ErrorKind::ResourceLimit); + assert!( + error.message.contains("Execution was cancelled"), + "expected cancellation diagnostic, got: {error:?}" + ); + + server.abort(); + let _ = server.await; +} + +#[tokio::test] +async fn main_loop_gives_each_outbound_request_a_finite_timeout() { + // A main loop is deliberately exempt from the run-lifetime deadline. An + // individual outbound request inside it must still reuse that configured + // duration as a fresh per-request limit. + let response = b"HTTP/1.1 200 OK\r\n\ +Content-Type: text/plain\r\n\ +Content-Length: 5\r\n\ +Connection: close\r\n\ +\r\n"; + let (url, server, _response_attempted) = spawn_http_peer(response, true).await; + + let config = Arc::new(WflConfig::default()); + let mut interpreter = Interpreter::with_config(Arc::clone(&config)); + let program = parse(&format!( + r#" +main loop: + open url at "{url}" and read content as content + break +end loop +"# + )); + + // Install the deliberately short budget only after building the HTTP + // client and parsing the fixture. The budget's start instant covers the + // whole run, so including unrelated setup here can exhaust it before the + // interpreter enters the main loop on slower CI hosts. This test is about + // the fresh per-request deadline applied *inside* that loop. + let limits = BudgetLimits { + max_duration: Some(Duration::from_millis(250)), + ..BudgetLimits::from_config(&config) + }; + interpreter.set_budget(Arc::new(ExecutionBudget::new(limits))); + + let errors = tokio::time::timeout(Duration::from_secs(2), interpreter.interpret(&program)) + .await + .expect("main-loop request should have a finite timeout") + .expect_err("stalled main-loop request must time out"); + let error = errors.first().expect("one runtime error"); + assert_eq!(error.kind, ErrorKind::Timeout); + assert!( + error + .message + .contains("Outbound HTTP request exceeded timeout"), + "expected outbound timeout diagnostic, got: {error:?}" + ); + + server.abort(); + let _ = server.await; +} diff --git a/tests/subprocess_security_test.rs b/tests/subprocess_security_test.rs index 1a596dd4..af7513f7 100644 --- a/tests/subprocess_security_test.rs +++ b/tests/subprocess_security_test.rs @@ -43,12 +43,13 @@ allowed_shell_commands = echo warn_on_shell_execution = false "#; -// Windows has no standalone echo.exe; allowlist cmd.exe for opt-in tests. +// Windows has no standalone echo.exe. Use a non-shell executable so the +// allowlist fixture does not itself grant arbitrary `/C` command execution. #[cfg(windows)] const ALLOWLIST_PROGRAM_CONFIG: &str = r#" allow_shell_execution = true shell_execution_mode = allowlist_only -allowed_shell_commands = cmd.exe +allowed_shell_commands = where.exe warn_on_shell_execution = false "#; @@ -283,7 +284,7 @@ fn test_allowlist_only_allows_listed_program() { "#; #[cfg(windows)] let code = r#" - wait for execute command "cmd.exe" with arguments ["/C", "echo allowlisted"] as result + wait for execute command "where.exe" with arguments ["cmd.exe"] as result display result "#; @@ -293,7 +294,31 @@ fn test_allowlist_only_allows_listed_program() { "Allowlisted program should run: {:?}", result ); + #[cfg(not(windows))] assert!(result.unwrap().contains("allowlisted")); + #[cfg(windows)] + assert!(result.unwrap().to_ascii_lowercase().contains("cmd.exe")); +} + +#[test] +fn test_allowlist_only_blocks_shell_chaining_after_allowlisted_program() { + #[cfg(not(windows))] + let code = r#" + execute command "echo allowlisted; echo injected" as result + "#; + #[cfg(windows)] + let code = r#" + execute command "where.exe cmd.exe & echo injected" as result + "#; + + let result = run_wfl_with_config(code, Some(ALLOWLIST_PROGRAM_CONFIG)); + assert_blocked(result.clone(), "Shell chaining after allowlisted program"); + if let Err(err) = result { + assert!( + !err.lines().any(|line| line.trim() == "injected"), + "The unlisted chained payload must not execute: {err}" + ); + } } #[test] diff --git a/tests/transpiler_test.rs b/tests/transpiler_test.rs index ac159b9c..de042f21 100644 --- a/tests/transpiler_test.rs +++ b/tests/transpiler_test.rs @@ -632,7 +632,7 @@ fn test_wait_for_request_header_access() { // Verify header access handles the { request, response } wrapper properly assert_contains( &js, - "(() => { const __req = req; return (__req.request || __req).headers['user-agent']; })()", + "(() => { const __req = req; return (__req.request || __req).headers[\"user-agent\"]; })()", ); // Verify respond handles the { request, response } wrapper properly assert_contains( @@ -640,3 +640,43 @@ fn test_wait_for_request_header_access() { "void (() => { const __req = req; const __res = __req.response || __req; __res.writeHead(200, { 'Content-Type': 'text/html' }); __res.end(\"OK\"); })();", ); } + +#[test] +fn test_untrusted_header_name_cannot_escape_generated_javascript_string() { + let source = r#" + display header "x']; globalThis.pwned=true; // \"" of req + "#; + + let js = transpile_wfl(source).unwrap(); + assert_contains(&js, "headers[\"x']; globalthis.pwned=true; // \\\"\"]"); + assert!(!js.contains("headers['x']; globalthis.pwned=true")); +} + +#[test] +fn test_untrusted_time_format_cannot_escape_generated_javascript_string() { + let source = r#" + display current time formatted as "'); globalThis.pwned=true; // \"" + "#; + + let js = transpile_wfl(source).unwrap(); + assert_contains( + &js, + "WFL.time.format(new Date(), \"'); globalThis.pwned=true; // \\\"\")", + ); + assert!(!js.contains("format(new Date(), ''); globalThis.pwned=true")); +} + +#[test] +fn test_describe_and_test_descriptions_are_javascript_string_literals() { + let source = r#" + describe "quoted \"suite\"": + test "quoted \"case\"": + expect yes to be yes + end test + end describe + "#; + + let js = transpile_wfl(source).unwrap(); + assert_contains(&js, "describe(\"quoted \\\"suite\\\"\", function()"); + assert_contains(&js, "it(\"quoted \\\"case\\\"\", function()"); +} diff --git a/tests/value_cycle_safety.rs b/tests/value_cycle_safety.rs new file mode 100644 index 00000000..a1168a8a --- /dev/null +++ b/tests/value_cycle_safety.rs @@ -0,0 +1,93 @@ +use std::cell::RefCell; +use std::collections::HashMap; +use std::rc::Rc; +use wfl::interpreter::value::Value; + +#[test] +fn self_cycle_formats_and_deep_clones_without_recursing_forever() { + let source_list = Rc::new(RefCell::new(Vec::new())); + let source = Value::List(Rc::clone(&source_list)); + source_list.borrow_mut().push(source.clone()); + + assert_eq!(source.to_string(), "[]"); + assert_eq!(format!("{source:?}"), "[]"); + + let cloned = source.deep_clone(); + let Value::List(cloned_list) = cloned else { + panic!("a cloned list must remain a list"); + }; + assert!( + !Rc::ptr_eq(&source_list, &cloned_list), + "deep_clone must isolate the clone from its source" + ); + + let cloned_child = cloned_list.borrow()[0].clone(); + let Value::List(cloned_child_list) = cloned_child else { + panic!("the self-reference must remain a list reference"); + }; + assert!( + Rc::ptr_eq(&cloned_list, &cloned_child_list), + "the cloned self-reference must point at the cloned list" + ); +} + +#[test] +fn mutual_cycle_preserves_cycles_and_shared_identity_in_the_clone() { + let source_list = Rc::new(RefCell::new(Vec::new())); + let source_object = Rc::new(RefCell::new(HashMap::new())); + + let shared_object = Value::Object(Rc::clone(&source_object)); + source_list + .borrow_mut() + .extend([shared_object.clone(), shared_object]); + source_object + .borrow_mut() + .insert("back".to_string(), Value::List(Rc::clone(&source_list))); + let source = Value::List(Rc::clone(&source_list)); + + assert_eq!(source.to_string(), "[, ]"); + assert_eq!(format!("{source:?}"), "[{back: }, {back: }]"); + + let Value::List(cloned_list) = source.deep_clone() else { + panic!("a cloned list must remain a list"); + }; + let cloned_items = cloned_list.borrow(); + let Value::Object(first_object) = &cloned_items[0] else { + panic!("the cloned list must contain its object"); + }; + let Value::Object(second_object) = &cloned_items[1] else { + panic!("the cloned list must contain its shared object twice"); + }; + assert!( + Rc::ptr_eq(first_object, second_object), + "shared source objects must remain shared within the cloned graph" + ); + assert!( + !Rc::ptr_eq(&source_object, first_object), + "the cloned object must be independent from its source" + ); + + let cloned_back_reference = first_object + .borrow() + .get("back") + .expect("cloned object must retain its back-reference") + .clone(); + let Value::List(cloned_back_list) = cloned_back_reference else { + panic!("the cloned back-reference must remain a list"); + }; + assert!( + Rc::ptr_eq(&cloned_list, &cloned_back_list), + "the mutual cycle must point back into the cloned graph" + ); +} + +#[test] +fn formatting_stops_at_a_bounded_depth_for_acyclic_values() { + let mut value = Value::Number(1.0); + for _ in 0..128 { + value = Value::List(Rc::new(RefCell::new(vec![value]))); + } + + assert!(value.to_string().contains("")); + assert!(format!("{value:?}").contains("")); +} diff --git a/wfl-lsp/src/mcp_server.rs b/wfl-lsp/src/mcp_server.rs index 4f610298..a6d2f6e6 100644 --- a/wfl-lsp/src/mcp_server.rs +++ b/wfl-lsp/src/mcp_server.rs @@ -1,15 +1,19 @@ use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use std::fs; -use std::io::{self, BufRead, Write}; +use std::io::{self, BufRead, Read, Write}; use std::path::{Path, PathBuf}; use std::sync::Arc; +use tower_lsp::lsp_types::Url; use crate::core::WflLanguageCore; use wfl::lexer::lex_wfl_with_positions; use wfl::parser::Parser; use wfl::typechecker::TypeChecker; +/// Maximum source size returned by a single MCP resource read. +const MAX_MCP_RESOURCE_BYTES: u64 = 4 * 1024 * 1024; + /// JSON-RPC 2.0 Request #[derive(Debug, Deserialize)] struct JsonRpcRequest { @@ -934,35 +938,98 @@ impl WflMcpServer { } } - /// Handle file:///{path} resource + fn file_resource_error(id: Option, message: impl Into) -> JsonRpcResponse { + JsonRpcResponse { + jsonrpc: "2.0".to_string(), + id, + result: None, + error: Some(JsonRpcError { + code: -32602, + message: message.into(), + data: None, + }), + } + } + + fn read_bounded_text_file(path: &Path) -> Result { + let metadata = path + .metadata() + .map_err(|_| "File resource is not readable".to_string())?; + if !metadata.is_file() { + return Err("File resource is not a regular file".to_string()); + } + if metadata.len() > MAX_MCP_RESOURCE_BYTES { + return Err(format!( + "File resource exceeds the {} byte limit", + MAX_MCP_RESOURCE_BYTES + )); + } + + let mut bytes = Vec::new(); + fs::File::open(path) + .and_then(|file| { + file.take(MAX_MCP_RESOURCE_BYTES + 1) + .read_to_end(&mut bytes) + }) + .map_err(|_| "Failed to read file resource".to_string())?; + if bytes.len() as u64 > MAX_MCP_RESOURCE_BYTES { + return Err(format!( + "File resource exceeds the {} byte limit", + MAX_MCP_RESOURCE_BYTES + )); + } + String::from_utf8(bytes).map_err(|_| "File resource is not valid UTF-8".to_string()) + } + + /// Handle a workspace-owned `file:///` WFL resource. fn handle_file_resource(&self, id: Option, uri: &str) -> JsonRpcResponse { - // Extract path from file:/// URI - let path_str = uri.strip_prefix("file:///").unwrap_or(uri); - let path = Path::new(path_str); + let workspace_root = match self + .workspace_root + .as_ref() + .and_then(|root| root.canonicalize().ok()) + { + Some(root) => root, + None => return Self::file_resource_error(id, "No readable workspace root configured"), + }; - match fs::read_to_string(path) { - Ok(content) => JsonRpcResponse { - jsonrpc: "2.0".to_string(), - id, - result: Some(json!({ - "contents": [{ - "uri": uri, - "mimeType": "text/x-wfl", - "text": content - }] - })), - error: None, - }, - Err(e) => JsonRpcResponse { - jsonrpc: "2.0".to_string(), - id, - result: None, - error: Some(JsonRpcError { - code: -32603, - message: format!("Failed to read file: {}", e), - data: None, - }), - }, + let requested_path = match Url::parse(uri).ok().and_then(|url| url.to_file_path().ok()) { + Some(path) => path, + None => return Self::file_resource_error(id, "Invalid local file resource URI"), + }; + let requested_path = match requested_path.canonicalize() { + Ok(path) => path, + Err(_) => return Self::file_resource_error(id, "File resource does not exist"), + }; + + // Canonicalize both sides before comparing so `..` and symlinks cannot + // redirect a resource read outside the configured workspace. + if !requested_path.starts_with(&workspace_root) { + return Self::file_resource_error(id, "File resource is outside the workspace"); + } + if requested_path + .extension() + .and_then(|extension| extension.to_str()) + != Some("wfl") + { + return Self::file_resource_error(id, "Only WFL source files are readable resources"); + } + + let content = match Self::read_bounded_text_file(&requested_path) { + Ok(content) => content, + Err(message) => return Self::file_resource_error(id, message), + }; + + JsonRpcResponse { + jsonrpc: "2.0".to_string(), + id, + result: Some(json!({ + "contents": [{ + "uri": uri, + "mimeType": "text/x-wfl", + "text": content + }] + })), + error: None, } } @@ -994,7 +1061,7 @@ impl WflMcpServer { { let path = entry.path(); if path.extension().and_then(|s| s.to_str()) == Some("wfl") - && let Ok(content) = fs::read_to_string(&path) + && let Ok(content) = Self::read_bounded_text_file(&path) { let tokens = lex_wfl_with_positions(&content); let mut parser = Parser::new(&tokens); @@ -1044,9 +1111,30 @@ impl WflMcpServer { } }; + let workspace_root = match workspace_root.canonicalize() { + Ok(root) => root, + Err(_) => { + return Self::file_resource_error(id, "No readable workspace root configured"); + } + }; let config_path = workspace_root.join(".wflcfg"); let config_content = if config_path.exists() { - fs::read_to_string(&config_path).unwrap_or_else(|_| "{}".to_string()) + let config_path = match config_path.canonicalize() { + Ok(path) if path.starts_with(&workspace_root) => path, + Ok(_) => { + return Self::file_resource_error( + id, + "Workspace configuration resolves outside the workspace", + ); + } + Err(_) => { + return Self::file_resource_error(id, "Workspace configuration is unreadable"); + } + }; + match Self::read_bounded_text_file(&config_path) { + Ok(content) => content, + Err(message) => return Self::file_resource_error(id, message), + } } else { json!({ "message": "No .wflcfg file found in workspace", @@ -1097,7 +1185,7 @@ impl WflMcpServer { { let path = entry.path(); if path.extension().and_then(|s| s.to_str()) == Some("wfl") - && let Ok(content) = fs::read_to_string(&path) + && let Ok(content) = Self::read_bounded_text_file(&path) { let diagnostics = self.core.analyze_document(&content); if !diagnostics.is_empty() { @@ -1218,7 +1306,7 @@ pub async fn run_server() -> Result<(), Box> { continue; } - eprintln!("[MCP] Received request: {}", line); + eprintln!("[MCP] Received request"); // Parse JSON-RPC request let request: JsonRpcRequest = match serde_json::from_str(&line) { @@ -1246,7 +1334,7 @@ pub async fn run_server() -> Result<(), Box> { let response = server.process_request(request); let response_json = serde_json::to_string(&response)?; - eprintln!("[MCP] Sending response: {}", response_json); + eprintln!("[MCP] Sending response"); writeln!(stdout, "{}", response_json)?; stdout.flush()?; } @@ -1258,6 +1346,32 @@ pub async fn run_server() -> Result<(), Box> { #[cfg(test)] mod tests { use super::*; + use std::time::{SystemTime, UNIX_EPOCH}; + + struct TestWorkspace(PathBuf); + + impl TestWorkspace { + fn new(label: &str) -> Self { + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock") + .as_nanos(); + let path = std::env::temp_dir() + .join(format!("wfl-mcp-{label}-{}-{nonce}", std::process::id())); + fs::create_dir(&path).expect("test workspace"); + Self(path) + } + + fn path(&self) -> &Path { + &self.0 + } + } + + impl Drop for TestWorkspace { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } #[test] fn test_server_creation() { @@ -1330,4 +1444,124 @@ mod tests { let result = response.result.unwrap(); assert_eq!(result["isError"], true); } + + #[test] + fn file_resource_reads_workspace_wfl_source() { + let root = TestWorkspace::new("valid-resource"); + let source_path = root.path().join("program.wfl"); + fs::write(&source_path, "display \"hello\"").expect("source file"); + let uri = Url::from_file_path(&source_path) + .expect("file URI") + .to_string(); + let server = WflMcpServer::with_workspace(root.path().to_path_buf()); + + let response = server.handle_file_resource(Some(json!(10)), &uri); + + assert!(response.error.is_none()); + assert_eq!( + response.result.expect("resource")["contents"][0]["text"], + "display \"hello\"" + ); + } + + #[test] + fn file_resource_rejects_paths_outside_workspace() { + let root = TestWorkspace::new("outside-resource"); + let workspace = root.path().join("workspace"); + fs::create_dir(&workspace).expect("workspace"); + let secret_path = root.path().join("secret.wfl"); + fs::write(&secret_path, "sensitive").expect("outside file"); + let uri = Url::from_file_path(&secret_path) + .expect("file URI") + .to_string(); + let server = WflMcpServer::with_workspace(workspace); + + let response = server.handle_file_resource(Some(json!(11)), &uri); + + assert!(response.result.is_none()); + assert!( + response + .error + .expect("outside-workspace error") + .message + .contains("outside the workspace") + ); + } + + #[cfg(unix)] + #[test] + fn file_resource_rejects_symlink_escape() { + let root = TestWorkspace::new("symlink-resource"); + let workspace = root.path().join("workspace"); + fs::create_dir(&workspace).expect("workspace"); + let secret_path = root.path().join("secret.wfl"); + fs::write(&secret_path, "sensitive").expect("outside file"); + let link_path = workspace.join("linked.wfl"); + std::os::unix::fs::symlink(&secret_path, &link_path).expect("symlink"); + let uri = Url::from_file_path(&link_path) + .expect("file URI") + .to_string(); + let server = WflMcpServer::with_workspace(workspace); + + let response = server.handle_file_resource(Some(json!(12)), &uri); + + assert!(response.result.is_none()); + assert!( + response + .error + .expect("symlink error") + .message + .contains("outside the workspace") + ); + } + + #[test] + fn file_resource_rejects_oversized_source() { + let root = TestWorkspace::new("oversized-resource"); + let source_path = root.path().join("large.wfl"); + fs::write( + &source_path, + vec![b'a'; MAX_MCP_RESOURCE_BYTES as usize + 1], + ) + .expect("oversized source"); + let uri = Url::from_file_path(&source_path) + .expect("file URI") + .to_string(); + let server = WflMcpServer::with_workspace(root.path().to_path_buf()); + + let response = server.handle_file_resource(Some(json!(13)), &uri); + + assert!(response.result.is_none()); + assert!( + response + .error + .expect("oversized error") + .message + .contains("exceeds") + ); + } + + #[cfg(unix)] + #[test] + fn workspace_config_rejects_symlink_escape() { + let root = TestWorkspace::new("config-symlink"); + let workspace = root.path().join("workspace"); + fs::create_dir(&workspace).expect("workspace"); + let secret_path = root.path().join("secret.txt"); + fs::write(&secret_path, "sensitive").expect("outside file"); + std::os::unix::fs::symlink(&secret_path, workspace.join(".wflcfg")) + .expect("config symlink"); + let server = WflMcpServer::with_workspace(workspace); + + let response = server.handle_workspace_config(Some(json!(14))); + + assert!(response.result.is_none()); + assert!( + response + .error + .expect("config symlink error") + .message + .contains("outside the workspace") + ); + } }