Cosmos improvements #448
Cosmos improvements #448
Conversation
|
@xiangyan99 Change LGTM. Might need to go through dog fooding to fix False negative use-cases |
| var cleanedQuery = query; | ||
|
|
||
| // Remove line comments (-- comment) | ||
| cleanedQuery = Regex.Replace(cleanedQuery, @"--.*?$", "", RegexOptions.Multiline); |
There was a problem hiding this comment.
.NET Regex is a NFA which is O(n!) and is subject to its own kind of denial of service attacks. I strongly recommend not using .NET Regex and writing a helper that walks the string in a single pass.
There was a problem hiding this comment.
Or at the very least specify the Timeout value -- https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.matchtimeout?view=net-9.0
There was a problem hiding this comment.
// Setting a timeout of 2 seconds
TimeSpan timeout = TimeSpan.FromSeconds(3);
Regex regex = new Regex(pattern, RegexOptions.None, timeout);
Match match = regex.Match(input);
github-project-automation
bot
moved this from Untriaged
to In Progress
in Azure MCP Server
xiangyan99
requested review from
Copilot,
hallipr,
jongio,
wbreza,
tmeschter,
msalaman,
alzimmermsft and
jairmyree
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive security validation to Cosmos DB query functionality to prevent SQL injection attacks and limit resource consumption. The changes introduce query validation that only allows SELECT statements, prevents dangerous keywords and patterns, and limits query length and result count to protect against DoS attacks.
- Added query validation in
CosmosServiceto prevent SQL injection and DoS attacks - Enhanced error handling in
ItemQueryCommandto return appropriate HTTP status codes - Added comprehensive test coverage for various malicious query patterns and edge cases
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| CosmosService.cs | Implements ValidateQuerySafety method with security checks and result limiting |
| ItemQueryCommand.cs | Updates command description and adds custom error handling for validation failures |
| ItemQueryCommandTests.cs | Adds extensive test coverage for security validation scenarios |
| CHANGELOG.md | Documents the new validation feature |
tools/Azure.Mcp.Tools.Cosmos/tests/Azure.Mcp.Tools.Cosmos.UnitTests/ItemQueryCommandTests.cs
Show resolved
Hide resolved
| @@ -85,5 +86,19 @@ public override async Task<CommandResponse> ExecuteAsync(CommandContext context, | |||
| return context.Response; | |||
| } | |||
|
|
|||
| protected override string GetErrorMessage(Exception ex) => ex switch | |||
There was a problem hiding this comment.
Is this needed if it doesn't add anything?
| private static readonly TimeSpan RegexTimeout = TimeSpan.FromSeconds(3); | ||
|
|
||
| // Static arrays for security validation - initialized once per class | ||
| private static readonly string[] DangerousKeywords = |
There was a problem hiding this comment.
Same comment as the other PR, should we move to an allow list instead of a disallow list? Seems safer.
There was a problem hiding this comment.
Yes - I would agree. A allow whitelist would help to control the workflow much better.
| var cleanedQuery = query; | ||
|
|
||
| // Remove line comments (-- comment) | ||
| cleanedQuery = Regex.Replace(cleanedQuery, @"--.*?$", "", RegexOptions.Multiline, RegexTimeout); |
There was a problem hiding this comment.
what happens when this throws RegexTimedOutException? Do we catch all exceptions higher up and handle this gracefully?
What does this PR do?
[Provide a clear, concise description of the changes]Add validation in cosmos query
[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