Implement READONLY mode checks and enhance SQL query validation in Tools #64
Conversation
|
Hey noticed the TS\Node blocked problematic queries going through, but dotnet just let anything happen... this should copy the behaviour. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive security enhancements for the MSSQL MCP server, adding READONLY mode checks and extensive SQL query validation to prevent unauthorized database operations and potential security vulnerabilities.
- Adds READONLY mode environment variable support that blocks destructive operations
- Implements comprehensive SQL injection protection with keyword filtering and pattern detection
- Adds a new TestConnection tool for database connectivity validation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tools/Tools.cs | Adds IsReadOnlyMode property that checks READONLY environment variable |
| Tools/UpdateData.cs | Adds READONLY mode check to prevent UPDATE operations |
| Tools/InsertData.cs | Adds READONLY mode check to prevent INSERT operations |
| Tools/CreateTable.cs | Adds READONLY mode check to prevent CREATE TABLE operations |
| Tools/DropTable.cs | Adds READONLY mode check to prevent DROP TABLE operations |
| Tools/ReadData.cs | Implements comprehensive SQL validation, injection protection, and result sanitization |
| Tools/TestConnection.cs | New tool for testing database connectivity with connection information |
| MssqlMcp.Tests/UnitTests.cs | Adds comprehensive test coverage for READONLY mode and TestConnection functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "DELETE", "DROP", "UPDATE", "INSERT", "ALTER", "CREATE", | ||
| "TRUNCATE", "EXEC", "EXECUTE", "MERGE", "REPLACE", | ||
| "GRANT", "REVOKE", "COMMIT", "ROLLBACK", "TRANSACTION", | ||
| "BEGIN", "DECLARE", "SET", "USE", "BACKUP", |
There was a problem hiding this comment.
The keyword 'BEGIN' in the dangerous keywords list could prevent legitimate SELECT queries that use BEGIN...END blocks in subqueries or CTEs. Consider being more specific (e.g., 'BEGIN TRANSACTION') or handle this in the pattern validation instead.
| "BEGIN", "DECLARE", "SET", "USE", "BACKUP", | |
| "DECLARE", "SET", "USE", "BACKUP", |
Copilot uses AI. Check for mistakes.
|
|
||
| // Remove comments and normalize whitespace for analysis | ||
| var cleanQuery = Regex.Replace(query, @"--.*$", "", RegexOptions.Multiline) // Remove line comments | ||
| .Replace("/*", "").Replace("*/", "") // Remove block comments (simple approach) |
There was a problem hiding this comment.
The block comment removal using simple string replacement is flawed. It will remove '/' and '/' independently, which could break valid queries. For example, a query with '/* comment 1 / SELECT * FROM / comment 2 /' would become ' SELECT * FROM ' but remove both opening and closing markers incorrectly. Use proper regex pattern like @'/*.?*/' with RegexOptions.Singleline.
| .Replace("/*", "").Replace("*/", "") // Remove block comments (simple approach) | |
| ; | |
| cleanQuery = Regex.Replace(cleanQuery, @"/\*.*?\*/", "", RegexOptions.Singleline) // Remove block comments (proper approach) |
Copilot uses AI. Check for mistakes.
| { | ||
| return (false, $"Dangerous keyword '{keyword}' detected in query. Only SELECT operations are allowed."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Creating a new Regex object for each keyword in each validation call is inefficient. Consider pre-compiling all keyword regexes as static readonly Regex[] or use a single regex pattern with alternation like @'\b(DELETE|DROP|UPDATE|...)\b'.
Copilot uses AI. Check for mistakes.
| if (query.Contains("CHAR(") || query.Contains("NCHAR(") || query.Contains("ASCII(")) | ||
| { | ||
| return (false, "Character conversion functions are not allowed as they may be used for obfuscation."); | ||
| } |
There was a problem hiding this comment.
This validation is too restrictive and could prevent legitimate queries. Functions like CHAR(), NCHAR(), and ASCII() have valid use cases in SELECT queries for data formatting and analysis. Consider allowing these functions when used in a safe context rather than blanket prohibition.
| } |
Copilot uses AI. Check for mistakes.
| /// </summary> | ||
| /// <param name="data">The query result data</param> | ||
| /// <returns>Sanitized data</returns> | ||
| private List<Dictionary<string, object?>> SanitizeResult(List<Dictionary<string, object?>> data) |
There was a problem hiding this comment.
The SanitizeResult method processes all data even when no sanitization is needed. Consider adding an early check to see if any column names contain suspicious characters before processing the entire dataset.
Copilot uses AI. Check for mistakes.
|
@sitefinitysteve thank you for the PR, copilot assisted review and has left few comments PTAL. |
Oh cool feature! I'll get on it once I'm back from vacation |
@sitefinitysteve the one thing I noticed is that the code currently looks at the readonly flag and returns which is good (second level validation), in addition to this what would be required is showing only the tools that are readonly, i.e describe table, list tables and read. |
dotnet changes: