Add security utilities and implementation guide for Excel Tables (ListObjects)

[Copilot]

Identified during architecture review of Issue #24: Excel Tables implementation requires security hardening against path traversal, injection attacks, DoS vectors, and COM memory leaks.

Security Utilities

TableNameValidator - Prevents injection via Excel table names:

using Sbroenne.ExcelMcp.Core.Security;

// Blocks spaces, reserved names (Print_Area), cell references (A1), 
// formula injection (=SUM), invalid chars, 255+ char names
string safe = TableNameValidator.ValidateTableName(tableName);

RangeValidator - Prevents DoS from oversized ranges:

// Default max: 1M cells. Prevents integer overflow and excessive memory use
RangeValidator.ValidateRange(rangeObj);
RangeValidator.ValidateRangeAddress("A1:XFD1048576"); // throws if malformed

Implementation Guide

docs/EXCEL-TABLES-SECURITY-GUIDE.md provides patterns for Issue #24 implementers:

• Path traversal prevention: PathValidator.ValidateExistingFile() usage
• Null safety: HeaderRowRange and DataBodyRange can be null (header-only or no-data tables)
• COM cleanup: Unlist() invalidates references - must release immediately to prevent leaks
• Security checklist: Pre-merge verification for all 4 critical issues

Test Coverage

• 40 tests: TableNameValidator (injection attempts, reserved names, cell ref patterns)
• 35 tests: RangeValidator (DoS limits, dimension validation, overflow prevention)

Impact

• Zero changes to existing code - pure additive
• Blocks Issue Add support for creating Excel Tables programmatically #24 until security patterns are followed
• SECURITY.md updated with new validation features

Original prompt

---

This section details on the original issue you should resolve

<issue_title>🔴 CRITICAL: Security and Robustness Fixes Required Before Excel Tables Implementation</issue_title>
<issue_description>## 🚨 CRITICAL ISSUES - MUST FIX BEFORE ISSUE #24

These critical security and robustness issues were identified during senior architect review of the Excel Tables implementation plan (Issue #24). They must be addressed before proceeding with Excel Tables development.

Severity: 🔴 CRITICAL
Priority: P0 - Blocker
Blocks: Issue #24 (Excel Tables Implementation)

---

Issue 1: Path Traversal Vulnerability 🔴

Problem

TableCommands implementation does not use PathValidator for file path validation, exposing path traversal vulnerability.

Current Code (Vulnerable)

public TableListResult List(string filePath)
{
    if (!File.Exists(filePath))
        return new TableListResult { /* error */ };
    
    // No path validation - VULNERABLE!
}

Required Fix

using Sbroenne.ExcelMcp.Core.Security;

public TableListResult List(string filePath)
{
    // CRITICAL: Validate path to prevent traversal attacks
    filePath = PathValidator.ValidateExistingFile(filePath, nameof(filePath));
    
    var result = new TableListResult { FilePath = filePath, Action = "list-tables" };
    // Rest of implementation...
}

Impact

• Security Risk: HIGH
• Attack Vector: Path traversal (e.g., ../../etc/passwd, C:\Windows\System32\config\SAM)
• Affected Methods: List(), Create(), Rename(), Delete(), GetInfo()

Acceptance Criteria

• ✅ All 5 TableCommands methods use PathValidator.ValidateExistingFile()
• ✅ PathValidator called FIRST before any file operations
• ✅ Unit tests verify path validation (invalid paths rejected)

---

Issue 2: Null Reference - HeaderRowRange/DataBodyRange 🔴

Problem

HeaderRowRange and DataBodyRange can be NULL when tables have no headers or no data, causing NullReferenceException.

Current Code (Vulnerable)

headerRowRange = listObject.HeaderRowRange;  // CAN BE NULL!
dataBodyRange = listObject.DataBodyRange;    // CAN BE NULL!

var tableInfo = new TableInfo
{
    DataRowCount = dataBodyRange.Rows.Count,  // CRASH if null!
    ColumnCount = headerRowRange.Columns.Count  // CRASH if null!
};

Required Fix

// Safe access with null checks
headerRowRange = listObject.ShowHeaders ? listObject.HeaderRowRange : null;
dataBodyRange = listObject.DataBodyRange;  // Can legitimately be null

var tableInfo = new TableInfo
{
    DataRowCount = dataBodyRange?.Rows.Count ?? 0,  // Safe
    ColumnCount = headerRowRange?.Columns.Count ?? 0  // Safe
};

// Get column names only if header exists
if (headerRowRange != null && listObject.ShowHeaders)
{
    dynamic? columns = listObject.ListColumns;
    try
    {
        for (int k = 1; k <= columns.Count; k++)
        {
            dynamic? column = null;
            try
            {
                column = columns.Item(k);
                tableInfo.ColumnNames.Add(column.Name);
            }
            finally
            {
                ComUtilities.Release(ref column);
            }
        }
    }
    finally
    {
        ComUtilities.Release(ref columns);
    }
}

Impact

• Reliability Risk: HIGH
• Failure Mode: NullReferenceException crashes application
• Affected Methods: List(), GetInfo()

Acceptance Criteria

• ✅ All HeaderRowRange accesses have null checks
• ✅ All DataBodyRange accesses have null checks
• ✅ Integration tests with tables having no data (header-only tables)
• ✅ Integration tests with tables having ShowHeaders=false

---

Issue 3: COM Cleanup After Unlist() 🔴

Problem

ListObject.Unlist() removes the table but doesn't release the COM reference, causing memory leaks and potential use-after-free bugs.

Current Code (Vulnerable)

// Delete operation
listObject.Unlist();  // Removes table but listObject still points to dead COM object

// Later code might try to access listObject - CRASH or leak!

Required Fix

// Delete operation with proper cleanup
try
{
    listObject = listObjects.Item(j);
    if (listObject.Name == tableName)
    {
        // Step 1: Unlist (converts to range)
        listObject.Unlist();
        
        // Step 2: CRITICAL - Release COM reference
        ComUtilities.Release(ref listObject);
        
        // Step 3: Explicit null to prevent use-after-free
        listObject = null;
        
        found = true;
        break;
    }
}
finally
{
    // Final cleanup (handles case where Unlist() not called)
    if (listObject != null)
    {
        ComUtilities.Release(ref listObject);
    }
}

Impact

• Reliability Risk: MEDIUM-HIGH
• Failure Mode: Excel.exe process le...

• Fixes 🔴 CRITICAL: Security and Robustness Fixes Required Before Excel Tables Implementation #27

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

To fix this problem, refactor the catch (Exception ex) block in ValidateRange so that only specific, anticipated exceptions are caught. In this context, the anticipated exceptions seem to be COMException (interop failure) and potentially ArgumentException (argument validation).

• Since the method already catches and rethrows ArgumentException, the only "foreseeable" exception for interacting with the COM object would likely be COMException from Office interop. The generic catch-all is unnecessary unless particular runtime exceptions need handling (which should be enumerated individually if so).
• Thus, simply remove the catch (Exception ex) block. This allows unanticipated exceptions to propagate naturally, improving diagnosability.
• No new imports or definitions are needed.

To fix the issue, you should replace the generic catch (Exception ex) clause with catch clauses targeting specific exception types likely to occur when accessing dynamic COM properties. These could include COMException (when Excel interop fails), InvalidCastException (bad type on dynamic), and optionally RuntimeBinderException (from the C# dynamic runtime). Only these specific errors should be caught, to avoid hiding unexpected bugs. Any other exceptions will propagate and can be handled at a higher level if needed.

Edit only the catch clause in the file src/ExcelMcp.Core/Security/RangeValidator.cs within the TryValidateRange method (lines 114–117). You also need to add using Microsoft.CSharp.RuntimeBinder; if it is not already present at the top of the file, since you may now reference RuntimeBinderException explicitly.

---

To fix the problem, the foreach loop on line 146 should be replaced by iteration over a filtered sequence that expresses the validation check as a filter predicate. Specifically, the code should iterate over only those characters that do not match the "allowed" character set, i.e., those where !char.IsLetterOrDigit(c) && c != ':' && c != '$' && c != '!' && c != '_' && c != '.'. For those, throw the exception, as before.

• Change lines 146-155: Replace the foreach (char c in rangeAddress) loop together with the if-statement, with a LINQ .Where(...) loop.
• Add a using System.Linq; import at the top unless already present, so that .Where works.
• Otherwise, keep all logic the same ( still throws exception for the first invalid character ).
• No changes needed elsewhere. Maintain all trim, length check, etc.

---

To fix the issue, rewrite the loop to explicitly filter invalid characters using .Where(...). This reduces the nesting, makes the intent more apparent, and adheres to LINQ idioms. Specifically, replace the foreach (char c in tableName) { if (invalid) throw ...; } loop with a LINQ .Where filter selecting invalid characters, then iterate over those (if any) and throw on the first found. In this case, since the code should throw on the first invalid character, we can use .FirstOrDefault() (or .First()) to get any invalid char if one exists, or proceed if none is found.

File to change: src/ExcelMcp.Core/Security/TableNameValidator.cs
Region: Lines 80–89

Implementation plan:

1. Replace the loop with:
   • Use var invalidChar = tableName.FirstOrDefault(c => !char.IsLetterOrDigit(c) && c != '_' && c != '.');
   • If invalidChar != '\0', throw the ArgumentException (if the string is never empty, that's safe; otherwise check for tableName.Any()).
2. Alternatively, if the string (possibly empty) could have '\0' as a char, use .FirstOrDefault(...) with a nullable or other check.
3. No new imports needed; using System.Linq; is already present.

---