Improve postgres query #426
Conversation
xiangyan99
requested review from
Copilot,
jongio,
vukelich,
wbreza,
conniey,
tmeschter,
sandeep-sen and
jairmyree
There was a problem hiding this comment.
Pull Request Overview
This PR improves the PostgreSQL query tool by adding comprehensive security validation and query safety features to prevent SQL injection attacks and potential DoS vulnerabilities.
- Added extensive query validation logic with dangerous keyword detection, multiple statement prevention, and query length limits
- Implemented result size limits across database operations to prevent resource exhaustion
- Enhanced test coverage with dedicated test classes for query validation and parameterized query security
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| PostgresService.cs | Added security validation logic, dangerous keyword detection, result limits, and query safety checks |
| PostgresServiceQueryValidationTests.cs | Comprehensive test suite covering dangerous queries, SQL injection attempts, and validation edge cases |
| PostgresServiceParameterizedQueryTests.cs | Tests for parameterized query security and proper handling of malicious input |
| CHANGELOG.md | Updated changelog to document the security improvements |
Comments suppressed due to low confidence (1)
tools/Azure.Mcp.Tools.Postgres/src/Services/PostgresService.cs:254
- This query uses string interpolation instead of parameterized queries, making it vulnerable to SQL injection. The table name should be parameterized using NpgsqlCommand parameters to prevent injection attacks.
var query = $"SELECT column_name, data_type FROM information_schema.columns WHERE table_name = '{table}';";
| private const int MaxResultLimit = 10000; | ||
|
|
||
| // Static arrays for security validation - initialized once per class | ||
| private static readonly string[] DangerousKeywords = |
There was a problem hiding this comment.
Why not use an ALLOW list instead of a DISALLOW list? I don't think we'll ever catch all dangerous keywords
There was a problem hiding this comment.
This implementation ported from https://github.com/microsoft/mcp/blob/main/tools/Azure.Mcp.Tools.MySql/src/Services/MySqlService.cs
My understanding is that the "allow by default" approach has more flexibility. While it may be less secure than the "disallow by default" approach, I this this trade-off is justifiable.
| // Data manipulation that could be harmful | ||
| "DROP", "DELETE", "TRUNCATE", "ALTER", "CREATE", "INSERT", "UPDATE", | ||
| // Administrative operations | ||
| "GRANT", "REVOKE", "SET", "RESET", "KILL", "SHUTDOWN", "RESTART", |
There was a problem hiding this comment.
KILL and SHUTDOWN don't exist in Postgres
| // Administrative operations | ||
| "GRANT", "REVOKE", "SET", "RESET", "KILL", "SHUTDOWN", "RESTART", | ||
| // Information disclosure | ||
| "SHOW", "EXPLAIN", "ANALYZE", |
There was a problem hiding this comment.
Postgres supports ANALYSE as a synonym to ANALYZE
| // Information disclosure | ||
| "SHOW", "EXPLAIN", "ANALYZE", | ||
| // System operations | ||
| "COPY", "\\COPY", "VACUUM", "REINDEX", |
* Establish Area pattern and update all areas * Move resources and update codeowners
What does this PR do?
[Provide a clear, concise description of the changes]Improve postgres query tool
[Any additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentation/docs/azmcp-commands.mdand/or/docs/fabric-commands.mdToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test prompts/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline