From 589ac048e0dae9e4877c8e43aa7227f98c85569a Mon Sep 17 00:00:00 2001
From: Jasper <jasper@spiral.xyz>
Date: Thu, 13 Aug 2026 02:41:14 +0000
Subject: [PATCH] fix(security): bound XLSX range reads (#11190)

Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
---
 .../src/computercontroller/xlsx_tool.rs       | 87 ++++++++++++++++++-
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/crates/goose-mcp/src/computercontroller/xlsx_tool.rs b/crates/goose-mcp/src/computercontroller/xlsx_tool.rs
index 4e7b79415..8f6592fc6 100644
--- a/crates/goose-mcp/src/computercontroller/xlsx_tool.rs
+++ b/crates/goose-mcp/src/computercontroller/xlsx_tool.rs
@@ -3,6 +3,10 @@ use serde::{Deserialize, Serialize};
 use std::path::Path;
 use umya_spreadsheet::{structs::Workbook, Worksheet};
 
+const MAX_EXCEL_ROWS: u32 = 1_048_576;
+const MAX_EXCEL_COLUMNS: u32 = 16_384;
+const MAX_RANGE_CELLS: u64 = 100_000;
+
 #[derive(Debug, Serialize, Deserialize)]
 pub struct WorksheetInfo {
     name: String,
@@ -98,11 +102,13 @@ impl XlsxTool {
 
     pub fn get_range(&self, worksheet: &Worksheet, range: &str) -> Result<RangeData> {
         let (start_row, start_col, end_row, end_col) = parse_range(range)?;
-        let mut values = Vec::new();
+        let (row_count, column_count) =
+            validate_range_bounds(start_row, start_col, end_row, end_col)?;
+        let mut values = Vec::with_capacity(row_count as usize);
 
         // Iterate through rows first, then columns
         for row_idx in start_row..=end_row {
-            let mut row_values = Vec::new();
+            let mut row_values = Vec::with_capacity(column_count as usize);
             for col_idx in start_col..=end_col {
                 let cell_value = if let Some(cell) = worksheet.cell((col_idx, row_idx)) {
                     CellValue {
@@ -217,6 +223,44 @@ fn parse_range(range: &str) -> Result<(u32, u32, u32, u32)> {
     Ok((start.0, start.1, end.0, end.1))
 }
 
+fn validate_range_bounds(
+    start_row: u32,
+    start_col: u32,
+    end_row: u32,
+    end_col: u32,
+) -> Result<(u32, u32)> {
+    anyhow::ensure!(
+        (1..=MAX_EXCEL_ROWS).contains(&start_row) && (1..=MAX_EXCEL_ROWS).contains(&end_row),
+        "Row must be between 1 and {MAX_EXCEL_ROWS}"
+    );
+    anyhow::ensure!(
+        (1..=MAX_EXCEL_COLUMNS).contains(&start_col) && (1..=MAX_EXCEL_COLUMNS).contains(&end_col),
+        "Column must be between 1 and {MAX_EXCEL_COLUMNS}"
+    );
+    anyhow::ensure!(
+        start_row <= end_row && start_col <= end_col,
+        "Range start must not follow range end"
+    );
+
+    let row_count = end_row
+        .checked_sub(start_row)
+        .and_then(|span| span.checked_add(1))
+        .context("Row span overflow")?;
+    let column_count = end_col
+        .checked_sub(start_col)
+        .and_then(|span| span.checked_add(1))
+        .context("Column span overflow")?;
+    let cell_count = u64::from(row_count)
+        .checked_mul(u64::from(column_count))
+        .context("Range area overflow")?;
+    anyhow::ensure!(
+        cell_count <= MAX_RANGE_CELLS,
+        "Range contains {cell_count} cells; maximum is {MAX_RANGE_CELLS}"
+    );
+
+    Ok((row_count, column_count))
+}
+
 fn parse_cell_reference(reference: &str) -> Result<(u32, u32)> {
     // Parse Excel cell reference (e.g., "A1") and return (row, column) to match umya_spreadsheet's expectation
     let mut col_str = String::new();
@@ -249,7 +293,11 @@ fn column_letter_to_number(column: &str) -> Result<u32> {
         if !c.is_ascii_alphabetic() {
             anyhow::bail!("Invalid column letter");
         }
-        result = result * 26 + (c.to_ascii_uppercase() as u32 - 'A' as u32 + 1);
+        let digit = c.to_ascii_uppercase() as u32 - 'A' as u32 + 1;
+        result = result
+            .checked_mul(26)
+            .and_then(|value| value.checked_add(digit))
+            .context("Column number out of range")?;
     }
     Ok(result)
 }
@@ -296,6 +344,39 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_range_budget_boundaries() -> Result<()> {
+        assert_eq!(validate_range_bounds(1, 1, 100_000, 1)?, (100_000, 1));
+        assert!(validate_range_bounds(1, 1, 100_001, 1).is_err());
+        assert!(validate_range_bounds(1, 1, MAX_EXCEL_ROWS, MAX_EXCEL_COLUMNS).is_err());
+        Ok(())
+    }
+
+    #[test]
+    fn test_get_range_rejects_oversized_area_before_materialization() -> Result<()> {
+        let xlsx = XlsxTool::new(get_test_file())?;
+        let worksheet = xlsx.get_worksheet_by_index(0)?;
+        assert!(xlsx.get_range(worksheet, "A1:A100001").is_err());
+        Ok(())
+    }
+
+    #[test]
+    fn test_range_rejects_invalid_excel_coordinates_and_ordering() {
+        assert!(parse_range("A0:A1")
+            .and_then(|range| validate_range_bounds(range.0, range.1, range.2, range.3))
+            .is_err());
+        assert!(parse_range("A1048577:A1048577")
+            .and_then(|range| validate_range_bounds(range.0, range.1, range.2, range.3))
+            .is_err());
+        assert!(parse_range("XFE1:XFE1")
+            .and_then(|range| validate_range_bounds(range.0, range.1, range.2, range.3))
+            .is_err());
+        assert!(parse_range("B2:A1")
+            .and_then(|range| validate_range_bounds(range.0, range.1, range.2, range.3))
+            .is_err());
+        assert!(column_letter_to_number("ZZZZZZZZZZ").is_err());
+    }
+
     #[test]
     fn test_find_in_worksheet() -> Result<()> {
         let xlsx = XlsxTool::new(get_test_file())?;