From d544b9a047c9c6560689c1403fa1ea0d034c2dfe Mon Sep 17 00:00:00 2001 From: Gabriel Fonseca Date: Tue, 27 Oct 2026 16:59:11 +0000 Subject: [PATCH 1/3] fix: remove reachable panics from untrusted remote and repo input Four sites could be crashed by input from a remote server, a fetched page, a model-generated command, or a repo's .mcp.json: - code-tools WebFetch extract_title: lowercased the body with Unicode to_lowercase and used the resulting offsets on the raw body. Characters whose lowercase form has a different byte length shifted the offsets and panicked. Use to_ascii_lowercase, which keeps offsets stable. - code-tools extract_search_hits: `after_tag[1..]` panicked on an empty or multi-byte tail after an anchor with no closing tag. Advance by one char. - code-runtime bash_validation extract_first_command: sliced the untrimmed value with an offset computed on the trimmed value, which can land inside a multi-byte character. Slice the trimmed value. - code-integrations MCP percent_decode (CCR proxy URL): sliced the str at byte offsets after '%', which panics inside a multi-byte character. Decode on bytes. - code-integrations MCP stdio read_frame: preallocated a payload buffer of the peer-declared Content-Length, which panics or aborts for huge values. Read through take() instead and return UnexpectedEof on a short stream. Each fix has a regression test. Valid input behaviour is unchanged. Co-Authored-By: Claude Haiku 5.5 --- crates/code-integrations/src/mcp/stdio.rs | 113 +++++++++++++++------ crates/code-integrations/src/mcp/utils.rs | 32 +++++- crates/code-runtime/src/bash_validation.rs | 13 ++- crates/code-tools/src/lib.rs | 53 +++++++++- 4 files changed, 173 insertions(+), 38 deletions(-) diff --git a/crates/code-integrations/src/mcp/stdio.rs b/crates/code-integrations/src/mcp/stdio.rs index d6cec65..ee39f93 100644 --- a/crates/code-integrations/src/mcp/stdio.rs +++ b/crates/code-integrations/src/mcp/stdio.rs @@ -7,7 +7,7 @@ use std::time::Duration; use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; use serde_json::Value as JsonValue; -use tokio::io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; +use tokio::io::{AsyncBufRead, AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; use tokio::process::{Child, ChildStdin, ChildStdout, Command}; use tokio::time::timeout; @@ -1206,37 +1206,7 @@ impl McpStdioProcess { } pub async fn read_frame(&mut self) -> io::Result> { - let mut content_length = None; - loop { - let mut line = String::new(); - let bytes_read = self.stdout.read_line(&mut line).await?; - if bytes_read == 0 { - return Err(io::Error::new( - io::ErrorKind::UnexpectedEof, - "MCP stdio stream closed while reading headers", - )); - } - if line == "\r\n" { - break; - } - let header = line.trim_end_matches(['\r', '\n']); - if let Some((name, value)) = header.split_once(':') { - if name.trim().eq_ignore_ascii_case("Content-Length") { - let parsed = value - .trim() - .parse::() - .map_err(|error| io::Error::new(io::ErrorKind::InvalidData, error))?; - content_length = Some(parsed); - } - } - } - - let content_length = content_length.ok_or_else(|| { - io::Error::new(io::ErrorKind::InvalidData, "missing Content-Length header") - })?; - let mut payload = vec![0_u8; content_length]; - self.stdout.read_exact(&mut payload).await?; - Ok(payload) + read_frame_from(&mut self.stdout).await } pub async fn write_jsonrpc_message(&mut self, message: &T) -> io::Result<()> { @@ -1380,6 +1350,52 @@ fn apply_env(command: &mut Command, env: &BTreeMap) { } } +/// Reads one `Content-Length` framed message. The length comes from the peer, so the payload +/// is read through `take` rather than preallocated: a huge declared length must not panic +/// (`vec!` capacity overflow) or abort on allocation. A short stream is an error. +async fn read_frame_from(reader: &mut R) -> io::Result> { + let mut content_length = None; + loop { + let mut line = String::new(); + let bytes_read = reader.read_line(&mut line).await?; + if bytes_read == 0 { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "MCP stdio stream closed while reading headers", + )); + } + if line == "\r\n" { + break; + } + let header = line.trim_end_matches(['\r', '\n']); + if let Some((name, value)) = header.split_once(':') { + if name.trim().eq_ignore_ascii_case("Content-Length") { + let parsed = value + .trim() + .parse::() + .map_err(|error| io::Error::new(io::ErrorKind::InvalidData, error))?; + content_length = Some(parsed); + } + } + } + + let content_length = content_length.ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "missing Content-Length header") + })?; + let mut payload = Vec::new(); + let read = (&mut *reader) + .take(u64::try_from(content_length).unwrap_or(u64::MAX)) + .read_to_end(&mut payload) + .await?; + if read != content_length { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "MCP stdio stream closed while reading payload", + )); + } + Ok(payload) +} + fn encode_frame(payload: &[u8]) -> Vec { let header = format!("Content-Length: {}\r\n\r\n", payload.len()); let mut framed = header.into_bytes(); @@ -1404,6 +1420,39 @@ mod tests { use std::fs; use std::io::ErrorKind; use std::os::unix::fs::PermissionsExt; + + #[test] + fn huge_declared_content_length_returns_error_instead_of_panicking() { + // A hostile peer declares a payload far larger than it sends. Preallocating the + // declared size used to panic ("capacity overflow") or abort on allocation. + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + let wire: &[u8] = b"Content-Length: 18446744073709551615\r\n\r\n{\"partial\":true}"; + let result = runtime.block_on(async { + let mut reader = tokio::io::BufReader::new(wire); + super::read_frame_from(&mut reader).await + }); + let error = result.expect_err("oversized frame must fail"); + assert_eq!(error.kind(), ErrorKind::UnexpectedEof); + } + + #[test] + fn reads_well_formed_content_length_frame() { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + let wire: &[u8] = b"Content-Length: 2\r\n\r\n{}"; + let payload = runtime + .block_on(async { + let mut reader = tokio::io::BufReader::new(wire); + super::read_frame_from(&mut reader).await + }) + .expect("frame should parse"); + assert_eq!(payload, b"{}"); + } use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; diff --git a/crates/code-integrations/src/mcp/utils.rs b/crates/code-integrations/src/mcp/utils.rs index 79a6346..dc0f784 100644 --- a/crates/code-integrations/src/mcp/utils.rs +++ b/crates/code-integrations/src/mcp/utils.rs @@ -175,6 +175,15 @@ fn collapse_underscores(value: &str) -> String { collapsed } +fn hex_nibble(byte: u8) -> Option { + match byte { + b'0'..=b'9' => Some(byte - b'0'), + b'a'..=b'f' => Some(byte - b'a' + 10), + b'A'..=b'F' => Some(byte - b'A' + 10), + _ => None, + } +} + fn percent_decode(value: &str) -> String { let bytes = value.as_bytes(); let mut decoded = Vec::with_capacity(bytes.len()); @@ -182,9 +191,12 @@ fn percent_decode(value: &str) -> String { while index < bytes.len() { match bytes[index] { b'%' if index + 2 < bytes.len() => { - let hex = &value[index + 1..index + 3]; - if let Ok(byte) = u8::from_str_radix(hex, 16) { - decoded.push(byte); + // Work on bytes, not `&value[..]`: slicing the str panics when the two bytes + // after '%' are part of a multi-byte character. + if let (Some(high), Some(low)) = + (hex_nibble(bytes[index + 1]), hex_nibble(bytes[index + 2])) + { + decoded.push((high << 4) | low); index += 3; continue; } @@ -242,6 +254,20 @@ mod tests { ); } + #[test] + fn unwrapping_ccr_proxy_url_with_multibyte_percent_sequence_does_not_panic() { + // In "%aé" the escape window after '%' ends in the middle of the 2-byte 'é'. + // Slicing the str there used to panic. The malformed escape must be kept literally. + let malformed = "https://api.anthropic.com/v2/ccr-sessions/1?mcp_url=%aé"; + assert_eq!(unwrap_ccr_proxy_url(malformed), "%aé"); + + let wrapped = "https://api.anthropic.com/v2/ccr-sessions/1?mcp_url=https%3A%2F%2Fvendor.example%2F%zz%é"; + assert_eq!( + unwrap_ccr_proxy_url(wrapped), + "https://vendor.example/%zz%é" + ); + } + #[test] fn computes_signatures_for_stdio_and_remote_servers() { let stdio = McpServerConfig::Stdio(McpStdioServerConfig { diff --git a/crates/code-runtime/src/bash_validation.rs b/crates/code-runtime/src/bash_validation.rs index f00619e..b03a94d 100644 --- a/crates/code-runtime/src/bash_validation.rs +++ b/crates/code-runtime/src/bash_validation.rs @@ -635,7 +635,10 @@ fn extract_first_command(command: &str) -> String { .all(|c| c.is_ascii_alphanumeric() || c == '_') { // Skip past the value (might be quoted). - let after_eq = &next[eq_pos + 1..]; + // `find_end_of_value` returns an offset into the *trimmed* value, so slice + // the trimmed value too. Slicing the untrimmed text with that offset can land + // inside a multi-byte character and panic. + let after_eq = next[eq_pos + 1..].trim_start(); if let Some(space) = find_end_of_value(after_eq) { remaining = &after_eq[space..]; continue; @@ -1001,4 +1004,12 @@ mod tests { fn extracts_plain_command() { assert_eq!(extract_first_command("grep -r pattern ."), "grep"); } + + #[test] + fn env_prefix_with_whitespace_and_multibyte_quoted_value_does_not_panic() { + // The quoted value contains a 2-byte character, and whitespace follows the '='. + // Slicing the untrimmed text with the trimmed offset used to panic here. + assert_eq!(extract_first_command("A= 'é' ls"), "ls"); + assert_eq!(extract_first_command("A= \"é\" ls -la"), "ls"); + } } diff --git a/crates/code-tools/src/lib.rs b/crates/code-tools/src/lib.rs index abb4660..2a863f5 100644 --- a/crates/code-tools/src/lib.rs +++ b/crates/code-tools/src/lib.rs @@ -3726,7 +3726,10 @@ fn summarize_web_fetch( fn extract_title(content: &str, raw_body: &str, content_type: &str) -> Option { if content_type.contains("html") { - let lowered = raw_body.to_lowercase(); + // ASCII lowercasing keeps byte offsets identical to `raw_body`. Full Unicode + // lowercasing can change byte lengths (e.g. 'İ'), which would make the offsets + // below land inside a multi-byte character of `raw_body` and panic. + let lowered = raw_body.to_ascii_lowercase(); if let Some(start) = lowered.find("") { let after = start + "<title>".len(); if let Some(end_rel) = lowered[after..].find("") { @@ -3821,7 +3824,7 @@ fn extract_search_hits(html: &str) -> Vec { }; let after_tag = &rest[close_tag_idx + 1..]; let Some(end_anchor_idx) = after_tag.find("") else { - remaining = &after_tag[1..]; + remaining = skip_one_char(after_tag); continue; }; let title = html_to_text(&after_tag[..end_anchor_idx]); @@ -3879,6 +3882,15 @@ fn extract_search_hits_from_generic_links(html: &str) -> Vec { hits } +/// Drops the first character of `input`. Unlike `&input[1..]`, this never panics on an +/// empty string or a multi-byte leading character in untrusted HTML. +fn skip_one_char(input: &str) -> &str { + input + .chars() + .next() + .map_or("", |ch| &input[ch.len_utf8()..]) +} + fn extract_quoted_value(input: &str) -> Option<(String, &str)> { let quote = input.chars().next()?; if quote != '"' && quote != '\'' { @@ -6738,6 +6750,43 @@ fn parse_skill_description(contents: &str) -> Option { pub mod lane_completion; +#[cfg(test)] +mod untrusted_html_panic_regression_tests { + use super::{extract_search_hits, extract_title}; + + #[test] + fn title_extraction_survives_unicode_that_changes_length_when_lowercased() { + // 'İ' (2 bytes) lowercases to 3 bytes, so full Unicode lowercasing shifts the + // offset of "" relative to the original body. Must not panic. + let body = "<html><body>İİİİ</body><TITLE>Hello & welcome"; + let title = extract_title("", body, "text/html"); + assert_eq!(title.as_deref(), Some("Hello & welcome")); + } + + #[test] + fn title_extraction_handles_unclosed_title_with_unicode_prefix() { + let body = "İno end tag"; + assert_eq!( + extract_title("fallback line", body, "text/html").as_deref(), + Some("fallback line") + ); + } + + #[test] + fn search_hit_parsing_survives_anchor_ending_at_end_of_input() { + // The anchor tag closes with '>' and then the input ends, with no "</a>". + let html = "result__a href=\"https://example.com/\">"; + assert!(extract_search_hits(html).is_empty()); + } + + #[test] + fn search_hit_parsing_survives_multibyte_char_after_anchor_tag() { + // No "</a>" follows, and the first byte after '>' starts a multi-byte char. + let html = "result__a href=\"https://example.com/\">é trailing"; + assert!(extract_search_hits(html).is_empty()); + } +} + #[cfg(test)] mod tests { use std::collections::BTreeMap; From 2b2bbf31ed2b166d85e48497fe9b6dc65eaca3a1 Mon Sep 17 00:00:00 2001 From: Gabriel Fonseca <hey@gabfon.com> Date: Tue, 27 Oct 2026 17:07:49 +0000 Subject: [PATCH 2/3] fix(bash): do not treat the word after an empty assignment as its value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With whitespace right after '=', bash makes the assignment empty and runs the next word. Trimming before measuring the value consumed that word, so the permission check saw the following argument as the command (A= rm x was reported as x). Start the value at '=' and only skip a word when the value is non-empty. Correct the earlier expectation that A= 'é' ls reports ls: bash runs 'é' there. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> --- crates/code-runtime/src/bash_validation.rs | 38 +++++++++++++++------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/crates/code-runtime/src/bash_validation.rs b/crates/code-runtime/src/bash_validation.rs index b03a94d..63d20e5 100644 --- a/crates/code-runtime/src/bash_validation.rs +++ b/crates/code-runtime/src/bash_validation.rs @@ -634,13 +634,18 @@ fn extract_first_command(command: &str) -> String { .chars() .all(|c| c.is_ascii_alphanumeric() || c == '_') { - // Skip past the value (might be quoted). - // `find_end_of_value` returns an offset into the *trimmed* value, so slice - // the trimmed value too. Slicing the untrimmed text with that offset can land - // inside a multi-byte character and panic. - let after_eq = next[eq_pos + 1..].trim_start(); - if let Some(space) = find_end_of_value(after_eq) { - remaining = &after_eq[space..]; + // The value starts immediately after `=`. If that is whitespace (or nothing), + // the assignment has an empty value and the next word is the command, so it + // must not be consumed as the value. Trimming before measuring would do that + // and hide the real command from the permission check. + let value_start = &next[eq_pos + 1..]; + if value_start.is_empty() || value_start.starts_with(char::is_whitespace) { + remaining = value_start; + continue; + } + // Skip past the value (might be quoted). Measure and slice the same string. + if let Some(space) = find_end_of_value(value_start) { + remaining = &value_start[space..]; continue; } // No space found means value goes to end of string — no actual command. @@ -1007,9 +1012,20 @@ mod tests { #[test] fn env_prefix_with_whitespace_and_multibyte_quoted_value_does_not_panic() { - // The quoted value contains a 2-byte character, and whitespace follows the '='. - // Slicing the untrimmed text with the trimmed offset used to panic here. - assert_eq!(extract_first_command("A= 'é' ls"), "ls"); - assert_eq!(extract_first_command("A= \"é\" ls -la"), "ls"); + // Whitespace right after '=' makes the assignment empty, so the next word is the + // command (bash runs `'é'` here, not `ls`). + assert_eq!(extract_first_command("A= 'é' ls"), "'é'"); + assert_eq!(extract_first_command("A= \"é\" ls -la"), "\"é\""); + } + + #[test] + fn empty_assignment_does_not_hide_the_real_command() { + // With `A=` followed by whitespace, bash runs the following word. Treating that word + // as the assignment's value would report the wrong command to the permission check. + assert_eq!(extract_first_command("A= rm x"), "rm"); + assert_eq!(extract_first_command("A= rm x"), "rm"); + assert_eq!(extract_first_command("A=1 B= rm x"), "rm"); + assert_eq!(extract_first_command("A="), ""); + assert_eq!(extract_first_command("A=1 B=2 echo hello"), "echo"); } } From a7ac712e020483c2ec10a2fee572bb0ff52734dc Mon Sep 17 00:00:00 2001 From: Gabriel Fonseca <hey@gabfon.com> Date: Tue, 27 Oct 2026 17:19:32 +0000 Subject: [PATCH 3/3] fix(bash): refuse non-bash whitespace in read-only checks The parser trimmed and split on Unicode whitespace, but bash only separates words on space, tab and newline. A command such as A=<NBSP>ls rm -rf / was reported as ls, while bash runs rm. In read-only mode, refuse any command containing other whitespace characters rather than guessing how bash will split it. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> --- crates/code-runtime/src/bash_validation.rs | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/code-runtime/src/bash_validation.rs b/crates/code-runtime/src/bash_validation.rs index 63d20e5..19387c7 100644 --- a/crates/code-runtime/src/bash_validation.rs +++ b/crates/code-runtime/src/bash_validation.rs @@ -105,6 +105,15 @@ pub fn validate_read_only(command: &str, mode: PermissionMode) -> ValidationResu return ValidationResult::Allow; } + // bash splits words only on space, tab and newline. This parser's trimming and + // splitting use Unicode whitespace, so any other whitespace character can make it + // report a different command from the one bash runs. Refuse those commands instead. + if has_non_bash_whitespace(command) { + return ValidationResult::Block { + reason: "Command contains whitespace other than space, tab, or newline and cannot be checked in read-only mode".to_string(), + }; + } + let first_command = extract_first_command(command); // Check for write commands. @@ -618,6 +627,14 @@ pub fn validate_command(command: &str, mode: PermissionMode, workspace: &Path) - // Helpers // --------------------------------------------------------------------------- +/// True when the command contains a whitespace character that bash does not treat as a +/// word separator (for example U+00A0, U+3000, `\r`, or `\x0b`). +fn has_non_bash_whitespace(command: &str) -> bool { + command + .chars() + .any(|c| c.is_whitespace() && !matches!(c, ' ' | '\t' | '\n')) +} + /// Extract the first bare command from a pipeline/chain, stripping env vars and sudo. fn extract_first_command(command: &str) -> String { let trimmed = command.trim(); @@ -1028,4 +1045,30 @@ mod tests { assert_eq!(extract_first_command("A="), ""); assert_eq!(extract_first_command("A=1 B=2 echo hello"), "echo"); } + + #[test] + fn read_only_refuses_whitespace_bash_does_not_split_on() { + // U+00A0 is whitespace to Rust but part of the word to bash. Here bash runs `rm` + // after an empty assignment, but a parser that skips the NBSP would report `ls`. + let mode = PermissionMode::ReadOnly; + for command in [ + "A=\u{a0}ls rm -rf /tmp/x", + "ls\u{3000}-la", + "A=\rls rm x", + "A=\u{b}ls rm x", + ] { + assert!( + matches!( + validate_read_only(command, mode), + ValidationResult::Block { .. } + ), + "expected a block for {command:?}" + ); + } + assert_eq!( + validate_read_only("ls -la", mode), + ValidationResult::Allow, + "plain ASCII commands are still allowed" + ); + } }