Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThis PR changes the MCP default transport from stdio to HTTP, adds CORS and transport feature flags, exposes MCP over HTTP via a new controller and client example, refactors McpServer to implement IMcpRequestHandler, updates DI/Startup for HTTP/CORS and conditional stdio hosting, and switches MCP listening port to 8080. Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Controller as McpController
participant Handler as IMcpRequestHandler\n(McpServer)
participant Server as McpServer\n(Internal Logic)
participant Logger as Logger
Client->>Controller: POST /mcp (JSON-RPC request)
Controller->>Logger: Log incoming request (redacted)
Controller->>Handler: HandleRequestAsync(requestJson, ctx)
Handler->>Server: Deserialize & dispatch to tools
Server->>Server: Execute MCP operation (initialize/list/call)
Server-->>Handler: Return JSON-RPC response string
Handler-->>Controller: Response string
Controller->>Logger: Log response (redacted)
Controller-->>Client: HTTP 200 + JSON-RPC response
Note over Controller: JsonException -> RPC Parse error (-32700)\nOther exceptions -> Internal error (-32603)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (1)
15-27:⚠️ Potential issue | 🟠 Major
Dictionary<string, ToolDefinition>is not thread-safe for concurrent HTTP requests.With the HTTP transport,
HandleRequestAsyncwill be called concurrently from multiple request threads.Dictionary<TKey, TValue>is not safe for concurrent reads if there's any possibility of concurrent writes. Tools are registered at startup viaAddTool, so in practice this may be read-only at runtime—but there's no enforcement.Consider switching to
ConcurrentDictionaryor making_toolsanIReadOnlyDictionaryafter initialization to make the thread-safety guarantee explicit.Option: Use ConcurrentDictionary
- private readonly Dictionary<string, ToolDefinition> _tools; + private readonly ConcurrentDictionary<string, ToolDefinition> _tools;- _tools = new Dictionary<string, ToolDefinition>(); + _tools = new ConcurrentDictionary<string, ToolDefinition>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs` around lines 15 - 27, The _tools field in McpServer is a mutable Dictionary<string, ToolDefinition> that can be accessed concurrently by HandleRequestAsync; replace it with a thread-safe collection or make it immutable after startup: either change the field to ConcurrentDictionary<string, ToolDefinition> and update AddTool to use TryAdd (and reads to index/safe methods), or populate a Dictionary during construction/registration then expose it as an IReadOnlyDictionary<string, ToolDefinition> (or call AsReadOnly/ToDictionary and assign to a readonly field) so concurrent request handlers only perform safe read operations; update references to _tools, the constructor, and the AddTool method accordingly to reflect the chosen approach.Web/Resgrid.Web.Mcp/Startup.cs (1)
137-148:⚠️ Potential issue | 🟠 MajorCORS middleware is placed before
UseRouting— move it betweenUseRoutingandUseEndpoints.ASP.NET Core requires
UseCorsto be called afterUseRoutingand beforeUseEndpointsfor CORS policies to integrate with endpoint routing. The current order prevents the CORS middleware from accessing endpoint metadata, which can cause CORS headers to be missing on responses.🔧 Proposed fix
- // Enable CORS if configured - if (McpConfig.EnableCors) - { - app.UseCors("McpCorsPolicy"); - } - app.UseRouting(); + // Enable CORS if configured + if (McpConfig.EnableCors) + { + app.UseCors("McpCorsPolicy"); + } + app.UseEndpoints(endpoints => { endpoints.MapControllers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/Startup.cs` around lines 137 - 148, The CORS middleware is registered before routing, preventing endpoint metadata access; when McpConfig.EnableCors is true, move the app.UseCors("McpCorsPolicy") call so it runs after app.UseRouting() and before app.UseEndpoints(...). Update the Startup.cs sequence to call app.UseRouting(); then app.UseCors("McpCorsPolicy"); then app.UseEndpoints(endpoints => { endpoints.MapControllers(); }); so the UseCors, UseRouting, and UseEndpoints ordering is correct.
🧹 Nitpick comments (4)
Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs (1)
102-125: Redundant catch block —HandleRequestAsync(string, …)already catches all exceptions.
HandleRequestAsync(string, CancellationToken)(Line 44-68) wraps deserialization and handling in a try/catch that returns a JSON-RPC error on any exception. This means the catch block here (Lines 108-125) is now unreachable under normal operation. It's harmless as defense-in-depth but the duplicated error-response construction adds maintenance burden.Consider removing the inner try/catch or simplifying it to just log and re-throw if a deeper invariant is desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs` around lines 102 - 125, The outer try/catch that surrounds the call to HandleRequestAsync in McpServer.cs is redundant because HandleRequestAsync(string, CancellationToken) already catches exceptions and returns a JSON-RPC error; remove the outer try/catch (the block that builds errorResponse, serializes it and writes it to Console.Out) so the code simply awaits HandleRequestAsync(line, cancellationToken), writes its result and flushes, or alternatively replace the outer catch with a minimal logging-only handler that rethrows to preserve invariant — reference the HandleRequestAsync method name and the surrounding Console.Out.WriteLineAsync/FlushAsync calls when locating the code to change.Web/Resgrid.Web.Mcp/ModelContextProtocol/IMcpRequestHandler.cs (1)
4-18: Namespace diverges from project convention.This interface and
McpServerlive underModelContextProtocol.Serverwhile the rest of the project usesResgrid.Web.Mcp.*. If the intent is to keep the protocol layer reusable/decoupled, that's fine—but it's worth a brief comment or README note explaining the choice so future contributors don't "fix" it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/IMcpRequestHandler.cs` around lines 4 - 18, The namespace ModelContextProtocol.Server for IMcpRequestHandler (and McpServer) diverges from the project's Resgrid.Web.Mcp.* convention; either move the types into the Resgrid.Web.Mcp.* namespace to match the rest of the project (update namespace declarations for IMcpRequestHandler and McpServer) or, if you intend to keep a reusable/decoupled protocol layer, add a short code comment above the IMcpRequestHandler declaration and a brief README note in the Web/Mcp area documenting the deliberate namespace choice and rationale so future contributors won't "fix" it unintentionally.Web/Resgrid.Web.Mcp/Examples/McpHttpClient.cs (1)
105-115:JsonDocumentinstances returned to callers are never disposed.
JsonDocumentisIDisposableand callers (includingRunExampleAsync) never dispose the returned instances. Since this is example/reference code, consider adding a note or wrapping usages inusingto model correct lifetime management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/Examples/McpHttpClient.cs` around lines 105 - 115, SendRequestAsync currently returns a JsonDocument (which implements IDisposable) but neither SendRequestAsync nor its callers (e.g., RunExampleAsync) dispose it; fix by either changing SendRequestAsync to return a non-disposable representation (e.g., string or JsonElement) or ensure every caller wraps the JsonDocument in a using/using declaration and disposes it. Update the SendRequestAsync method signature and return type if choosing non-disposable (adjust call sites), or update RunExampleAsync and any other callers to use using (or using var) around the JsonDocument returned by SendRequestAsync so the IDisposable is properly disposed.Core/Resgrid.Config/McpConfig.cs (1)
22-42: Redundant transport configuration and permissive CORS default.Two observations:
The
Transportstring field and theEnableHttpTransport/EnableStdioTransportbooleans can drift out of sync (e.g.,Transport = "stdio"butEnableHttpTransport = true). Consider deriving one from the other, or removingTransportin favor of the explicit booleans.
CorsAllowedOrigins = "*"allows all origins by default. For a service handling CAD/dispatch data, a more restrictive default (or at least a log warning when*is active) would reduce the risk of unintended cross-origin access in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Config/McpConfig.cs` around lines 22 - 42, McpConfig currently has a redundant Transport string that can drift from the explicit booleans EnableHttpTransport and EnableStdioTransport, and CorsAllowedOrigins defaults to "*" which is overly permissive; fix by choosing one authoritative source: either remove the Transport field and rely solely on EnableHttpTransport/EnableStdioTransport (delete Transport and update any callers to use the booleans), or make Transport authoritative and derive the booleans from it (implement logic in the class to set EnableHttpTransport = Transport == "http" and EnableStdioTransport = Transport == "stdio" and mark the booleans readonly or private setters), and change CorsAllowedOrigins default to a restrictive value (e.g., empty string) and add a startup-time warning/log when CorsAllowedOrigins == "*" to surface the risk; update any config parsing code that populates McpConfig to match the chosen approach and adjust tests accordingly, referencing McpConfig.Transport, McpConfig.EnableHttpTransport, McpConfig.EnableStdioTransport, and McpConfig.CorsAllowedOrigins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 60-65: The current debug logs call _logger.LogDebug with the full
requestBody and response which may contain PII (e.g., username/password); change
the logging to avoid logging full bodies by either logging only metadata (e.g.,
request type, correlation id) or by passing a redacted version of
requestBody/response to _logger.LogDebug. Implement or call a redaction helper
(e.g., RedactSensitiveFields(requestBody) and RedactSensitiveFields(response))
before logging, ensure fields like "username", "password", "token", "ssn" are
masked or removed, and keep the call to
_mcpHandler.HandleRequestAsync(requestBody, cancellationToken) unchanged;
replace the two _logger.LogDebug lines to log the sanitized/redacted payloads or
limited metadata only.
- Around line 70-84: The JsonException catch in McpController is effectively
unreachable because _mcpHandler.HandleRequestAsync handles deserialization
errors itself, and returning ex.Message in the response leaks internals; remove
or simplify this catch: either delete the JsonException catch block in
McpController or change it to return a generic JSON-RPC parse error without
including ex.Message (omit the data field or set it to a generic string), while
leaving _mcpHandler.HandleRequestAsync as the source of detailed error handling;
reference the JsonException catch in McpController and the call to
_mcpHandler.HandleRequestAsync when making this change.
In `@Web/Resgrid.Web.Mcp/Startup.cs`:
- Around line 76-84: The CORS origin list from McpConfig.CorsAllowedOrigins is
split but not trimmed, so entries with surrounding whitespace will fail
matching; update the origin parsing in the Startup.cs CORS configuration block
to Trim() each split entry and filter out any empty results before passing to
builder.WithOrigins (i.e., replace the current Split(...) usage with a pipeline
that splits, selects s.Trim(), and excludes empty strings) so
builder.WithOrigins receives clean origin strings.
---
Outside diff comments:
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs`:
- Around line 15-27: The _tools field in McpServer is a mutable
Dictionary<string, ToolDefinition> that can be accessed concurrently by
HandleRequestAsync; replace it with a thread-safe collection or make it
immutable after startup: either change the field to ConcurrentDictionary<string,
ToolDefinition> and update AddTool to use TryAdd (and reads to index/safe
methods), or populate a Dictionary during construction/registration then expose
it as an IReadOnlyDictionary<string, ToolDefinition> (or call
AsReadOnly/ToDictionary and assign to a readonly field) so concurrent request
handlers only perform safe read operations; update references to _tools, the
constructor, and the AddTool method accordingly to reflect the chosen approach.
In `@Web/Resgrid.Web.Mcp/Startup.cs`:
- Around line 137-148: The CORS middleware is registered before routing,
preventing endpoint metadata access; when McpConfig.EnableCors is true, move the
app.UseCors("McpCorsPolicy") call so it runs after app.UseRouting() and before
app.UseEndpoints(...). Update the Startup.cs sequence to call app.UseRouting();
then app.UseCors("McpCorsPolicy"); then app.UseEndpoints(endpoints => {
endpoints.MapControllers(); }); so the UseCors, UseRouting, and UseEndpoints
ordering is correct.
---
Nitpick comments:
In `@Core/Resgrid.Config/McpConfig.cs`:
- Around line 22-42: McpConfig currently has a redundant Transport string that
can drift from the explicit booleans EnableHttpTransport and
EnableStdioTransport, and CorsAllowedOrigins defaults to "*" which is overly
permissive; fix by choosing one authoritative source: either remove the
Transport field and rely solely on EnableHttpTransport/EnableStdioTransport
(delete Transport and update any callers to use the booleans), or make Transport
authoritative and derive the booleans from it (implement logic in the class to
set EnableHttpTransport = Transport == "http" and EnableStdioTransport =
Transport == "stdio" and mark the booleans readonly or private setters), and
change CorsAllowedOrigins default to a restrictive value (e.g., empty string)
and add a startup-time warning/log when CorsAllowedOrigins == "*" to surface the
risk; update any config parsing code that populates McpConfig to match the
chosen approach and adjust tests accordingly, referencing McpConfig.Transport,
McpConfig.EnableHttpTransport, McpConfig.EnableStdioTransport, and
McpConfig.CorsAllowedOrigins.
In `@Web/Resgrid.Web.Mcp/Examples/McpHttpClient.cs`:
- Around line 105-115: SendRequestAsync currently returns a JsonDocument (which
implements IDisposable) but neither SendRequestAsync nor its callers (e.g.,
RunExampleAsync) dispose it; fix by either changing SendRequestAsync to return a
non-disposable representation (e.g., string or JsonElement) or ensure every
caller wraps the JsonDocument in a using/using declaration and disposes it.
Update the SendRequestAsync method signature and return type if choosing
non-disposable (adjust call sites), or update RunExampleAsync and any other
callers to use using (or using var) around the JsonDocument returned by
SendRequestAsync so the IDisposable is properly disposed.
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/IMcpRequestHandler.cs`:
- Around line 4-18: The namespace ModelContextProtocol.Server for
IMcpRequestHandler (and McpServer) diverges from the project's Resgrid.Web.Mcp.*
convention; either move the types into the Resgrid.Web.Mcp.* namespace to match
the rest of the project (update namespace declarations for IMcpRequestHandler
and McpServer) or, if you intend to keep a reusable/decoupled protocol layer,
add a short code comment above the IMcpRequestHandler declaration and a brief
README note in the Web/Mcp area documenting the deliberate namespace choice and
rationale so future contributors won't "fix" it unintentionally.
In `@Web/Resgrid.Web.Mcp/ModelContextProtocol/McpServer.cs`:
- Around line 102-125: The outer try/catch that surrounds the call to
HandleRequestAsync in McpServer.cs is redundant because
HandleRequestAsync(string, CancellationToken) already catches exceptions and
returns a JSON-RPC error; remove the outer try/catch (the block that builds
errorResponse, serializes it and writes it to Console.Out) so the code simply
awaits HandleRequestAsync(line, cancellationToken), writes its result and
flushes, or alternatively replace the outer catch with a minimal logging-only
handler that rethrows to preserve invariant — reference the HandleRequestAsync
method name and the surrounding Console.Out.WriteLineAsync/FlushAsync calls when
locating the code to change.
Comment on lines
+70
to
+84
| catch (JsonException ex) | ||
| { | ||
| _logger.LogError(ex, "Failed to parse JSON-RPC request"); | ||
| return BadRequest(new | ||
| { | ||
| jsonrpc = "2.0", | ||
| id = (object)null, | ||
| error = new | ||
| { | ||
| code = -32700, | ||
| message = "Parse error", | ||
| data = ex.Message | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
JsonException catch is likely dead code; ex.Message leaks internals to client.
Two issues in this block:
-
The controller passes the raw string to
_mcpHandler.HandleRequestAsync, which internally catches its ownJsonSerializer.Deserializeexceptions and returns a JSON-RPC error string. AJsonExceptionwon't propagate here under normal flow—making this catch effectively unreachable. -
If it were reached,
data = ex.Message(Line 81) exposes internal error details to the caller. Omitdataor use a generic message.
Suggested fix for the data leak
error = new
{
code = -32700,
- message = "Parse error",
- data = ex.Message
+ message = "Parse error"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs` around lines 70 - 84, The
JsonException catch in McpController is effectively unreachable because
_mcpHandler.HandleRequestAsync handles deserialization errors itself, and
returning ex.Message in the response leaks internals; remove or simplify this
catch: either delete the JsonException catch block in McpController or change it
to return a generic JSON-RPC parse error without including ex.Message (omit the
data field or set it to a generic string), while leaving
_mcpHandler.HandleRequestAsync as the source of detailed error handling;
reference the JsonException catch in McpController and the call to
_mcpHandler.HandleRequestAsync when making this change.
Comment on lines
+76
to
+84
| { | ||
| // Specific origins (production) | ||
| var origins = McpConfig.CorsAllowedOrigins.Split(',', StringSplitOptions.RemoveEmptyEntries); | ||
| builder | ||
| .WithOrigins(origins) | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader() | ||
| .AllowCredentials(); | ||
| } |
Contributor
There was a problem hiding this comment.
Origins are not trimmed after splitting — whitespace in config will cause silent CORS failures.
If CorsAllowedOrigins contains "http://a.com, http://b.com", the second entry becomes " http://b.com" (leading space), and CORS matching will silently fail for that origin.
🔧 Proposed fix
- var origins = McpConfig.CorsAllowedOrigins.Split(',', StringSplitOptions.RemoveEmptyEntries);
+ var origins = McpConfig.CorsAllowedOrigins
+ .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)
+ .ToArray();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| // Specific origins (production) | |
| var origins = McpConfig.CorsAllowedOrigins.Split(',', StringSplitOptions.RemoveEmptyEntries); | |
| builder | |
| .WithOrigins(origins) | |
| .AllowAnyMethod() | |
| .AllowAnyHeader() | |
| .AllowCredentials(); | |
| } | |
| { | |
| // Specific origins (production) | |
| var origins = McpConfig.CorsAllowedOrigins | |
| .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) | |
| .ToArray(); | |
| builder | |
| .WithOrigins(origins) | |
| .AllowAnyMethod() | |
| .AllowAnyHeader() | |
| .AllowCredentials(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Mcp/Startup.cs` around lines 76 - 84, The CORS origin list
from McpConfig.CorsAllowedOrigins is split but not trimmed, so entries with
surrounding whitespace will fail matching; update the origin parsing in the
Startup.cs CORS configuration block to Trim() each split entry and filter out
any empty results before passing to builder.WithOrigins (i.e., replace the
current Split(...) usage with a pipeline that splits, selects s.Trim(), and
excludes empty strings) so builder.WithOrigins receives clean origin strings.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [HttpPost] | ||
| [Consumes("application/json")] | ||
| [Produces("application/json")] | ||
| public async Task<IActionResult> HandleRequest(CancellationToken cancellationToken) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
In general, to fix missing CSRF validation in ASP.NET Core MVC/Web API controllers, add the [ValidateAntiForgeryToken] attribute (or [AutoValidateAntiforgeryToken] globally) to all state-changing POST actions that are intended to be invoked from a browser with cookie-based authentication. The corresponding views or clients must include a valid anti-forgery token in their POSTs (e.g., via @Html.AntiForgeryToken() in Razor or the RequestVerificationToken header/body).
For this controller, the minimal fix that satisfies CodeQL without changing behavior elsewhere is to decorate HandleRequest with [ValidateAntiForgeryToken]. This ensures that if the endpoint is used from a browser with cookie-based auth, the standard ASP.NET Core antiforgery system will enforce that a valid token is provided. Token-based or non-cookie clients will need to send the antiforgery token as well (usually via header), but that’s outside the shown snippet and cannot be changed here.
Concretely:
- File to edit:
Web/Resgrid.Web.Mcp/Controllers/McpController.cs. - In the
HandleRequestaction (line 38), add the[ValidateAntiForgeryToken]attribute above the method signature, keeping all existing attributes ([HttpPost],[Consumes],[Produces]) unchanged. - No new methods are needed;
ValidateAntiForgeryTokenis part ofMicrosoft.AspNetCore.Mvc, already imported on line 7, so no new imports or dependencies are required.
Suggested changeset
1
Web/Resgrid.Web.Mcp/Controllers/McpController.cs
| @@ -35,6 +35,7 @@ | ||
| [HttpPost] | ||
| [Consumes("application/json")] | ||
| [Produces("application/json")] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<IActionResult> HandleRequest(CancellationToken cancellationToken) | ||
| { | ||
| try |
Copilot is powered by AI and may make mistakes. Always verify output.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 27318225 | Triggered | Username Password | 23829fd | Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.Test.cs | View secret |
| 27318226 | Triggered | Generic Password | 23829fd | Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.Test.cs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.cs (1)
96-142: Consider simplifying with LINQ and addingusingdirectives for readability.The fully-qualified
System.Collections.Generic.List<System.Collections.Generic.KeyValuePair<...>>on Line 101 is verbose. Adding ausing System.Collections.Generic;at the top and using LINQAny()for the sensitive check would make this more idiomatic.♻️ Suggested simplification
using System.Text.Json; using System.Text.Json.Nodes; +using System.Collections.Generic; +using System.Linq;private static void RedactNode(JsonNode node) { if (node is JsonObject jsonObject) { - // Create a list of properties to avoid modifying while iterating - var propertiesToProcess = new System.Collections.Generic.List<System.Collections.Generic.KeyValuePair<string, JsonNode>>(); - foreach (var property in jsonObject) - { - propertiesToProcess.Add(new System.Collections.Generic.KeyValuePair<string, JsonNode>(property.Key, property.Value)); - } + var propertiesToProcess = jsonObject.ToList(); foreach (var property in propertiesToProcess) { var propertyName = property.Key.ToLowerInvariant(); - // Check if this property name matches a sensitive field - var isSensitive = false; - foreach (var sensitiveField in SensitiveFields) - { - if (propertyName.Contains(sensitiveField)) - { - isSensitive = true; - break; - } - } + var isSensitive = SensitiveFields.Any(f => propertyName.Contains(f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.cs` around lines 96 - 142, Refactor RedactNode to simplify types and the sensitive-field check: add "using System.Collections.Generic;" and "using System.Linq;" at the top, replace the fully-qualified List<KeyValuePair<string, JsonNode>> with List<KeyValuePair<string, JsonNode>> or simply call jsonObject.ToList() to capture properties, and replace the manual loop that sets isSensitive with a LINQ Any() against SensitiveFields (e.g., SensitiveFields.Any(sf => propertyName.Contains(sf))). Keep existing behavior of setting jsonObject[property.Key] = RedactedValue and recursing via RedactNode for non-sensitive values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 43-44: The current code in McpController uses new
StreamReader(Request.Body) and await reader.ReadToEndAsync(), which can allocate
unbounded memory for large request bodies; update the controller to enforce a
size cap by either (A) adding a [RequestSizeLimit(<bytes>)] attribute to the
action or controller or (B) checking HttpContext.Request.ContentLength (and
rejecting if > limit) before reading, or (C) reading the stream in a bounded
fashion (e.g., read into a fixed-size buffer up to the limit and stop/throw if
exceeded) instead of ReadToEndAsync; reference McpController, Request.Body,
reader.ReadToEndAsync and implement one of these protections so the request body
cannot grow unbounded.
- Around line 46-71: The closing brace for the if
(string.IsNullOrWhiteSpace(requestBody)) block is mis-indented, making the
subsequent logic (calls to SensitiveDataRedactor.RedactSensitiveFields,
_logger.LogDebug, _mcpHandler.HandleRequestAsync and return Content) appear
outside the try/method scope; move the closing brace and align the following
lines so they remain inside the try block and at the same indentation level as
the if statement, ensuring the null-body early return (with _logger.LogWarning
and BadRequest) ends the method and the redaction, logging, handling
(SensitiveDataRedactor.RedactSensitiveFields, _logger.LogDebug,
_mcpHandler.HandleRequestAsync) and final return Content(response,
"application/json") are clearly nested under the try scope.
In `@Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.Test.cs`:
- Around line 10-48: SensitiveDataRedactorTests is not a real test (it uses
Console.WriteLine, has no assertions, and lives in production), so either move
it into a proper test project with assertions or remove it; specifically, remove
or relocate the class SensitiveDataRedactorTests and its RunTests method from
the production assembly and implement real unit tests in the test project (e.g.,
using xUnit/NUnit) that call SensitiveDataRedactor.RedactSensitiveFields and use
Assert methods to verify redaction (for example assert redacted output contains
"***REDACTED***" and does not contain original secrets) so regressions can fail
CI instead of only printing to console.
---
Duplicate comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 73-87: In the catch (JsonException ex) block in McpController
(where _logger.LogError is called and a BadRequest JSON-RPC response is
returned), stop returning ex.Message to the client; either remove the error.data
property entirely or replace it with a non-sensitive generic string (e.g.,
"Invalid JSON" or "Parse error"). Keep the existing _logger.LogError(ex, ...) so
internal details are still recorded server-side but ensure the JSON-RPC response
created in the return BadRequest(...) does not include ex.Message or any
internal exception details.
---
Nitpick comments:
In `@Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.cs`:
- Around line 96-142: Refactor RedactNode to simplify types and the
sensitive-field check: add "using System.Collections.Generic;" and "using
System.Linq;" at the top, replace the fully-qualified List<KeyValuePair<string,
JsonNode>> with List<KeyValuePair<string, JsonNode>> or simply call
jsonObject.ToList() to capture properties, and replace the manual loop that sets
isSensitive with a LINQ Any() against SensitiveFields (e.g.,
SensitiveFields.Any(sf => propertyName.Contains(sf))). Keep existing behavior of
setting jsonObject[property.Key] = RedactedValue and recursing via RedactNode
for non-sensitive values.
Comment on lines
+43
to
+44
| using var reader = new StreamReader(Request.Body); | ||
| var requestBody = await reader.ReadToEndAsync(); |
Contributor
There was a problem hiding this comment.
ReadToEndAsync without size limit allows unbounded memory allocation.
A malicious or buggy client can send an arbitrarily large body, causing the server to allocate unbounded memory. Consider enforcing a request body size limit (e.g., via [RequestSizeLimit] attribute or middleware), or reading with a capped buffer.
🛡️ Example: add size limit attribute
[HttpPost]
[Consumes("application/json")]
[Produces("application/json")]
+ [RequestSizeLimit(1_048_576)] // 1 MB limit
public async Task<IActionResult> HandleRequest(CancellationToken cancellationToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs` around lines 43 - 44, The
current code in McpController uses new StreamReader(Request.Body) and await
reader.ReadToEndAsync(), which can allocate unbounded memory for large request
bodies; update the controller to enforce a size cap by either (A) adding a
[RequestSizeLimit(<bytes>)] attribute to the action or controller or (B)
checking HttpContext.Request.ContentLength (and rejecting if > limit) before
reading, or (C) reading the stream in a bounded fashion (e.g., read into a
fixed-size buffer up to the limit and stop/throw if exceeded) instead of
ReadToEndAsync; reference McpController, Request.Body, reader.ReadToEndAsync and
implement one of these protections so the request body cannot grow unbounded.
Comment on lines
+46
to
+71
| if (string.IsNullOrWhiteSpace(requestBody)) | ||
| { | ||
| _logger.LogWarning("Received empty request body"); | ||
| return BadRequest(new | ||
| { | ||
| jsonrpc = "2.0", | ||
| id = (object)null, | ||
| error = new | ||
| { | ||
| code = -32600, | ||
| message = "Invalid Request: Empty request body" | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| var redactedRequest = SensitiveDataRedactor.RedactSensitiveFields(requestBody); | ||
| _logger.LogDebug("Received MCP request: {Request}", redactedRequest); | ||
|
|
||
| // Process the request through the MCP handler | ||
| var response = await _mcpHandler.HandleRequestAsync(requestBody, cancellationToken); | ||
|
|
||
| var redactedResponse = SensitiveDataRedactor.RedactSensitiveFields(response); | ||
| _logger.LogDebug("Sending MCP response: {Response}", redactedResponse); | ||
|
|
||
| // Return the JSON-RPC response | ||
| return Content(response, "application/json"); |
Contributor
There was a problem hiding this comment.
Misleading indentation — the if block's closing brace and subsequent lines are mis-indented.
Line 59 closes the if block (opened at Line 47), but it's indented at the same level as the try block. Lines 61–71 are inside the try but indented as if they're at method scope. This makes the control flow confusing to read and maintain.
♻️ Fix indentation
try
{
// Read the raw request body
using var reader = new StreamReader(Request.Body);
var requestBody = await reader.ReadToEndAsync();
if (string.IsNullOrWhiteSpace(requestBody))
{
_logger.LogWarning("Received empty request body");
return BadRequest(new
{
jsonrpc = "2.0",
id = (object)null,
error = new
{
code = -32600,
message = "Invalid Request: Empty request body"
}
});
- }
+ }
- var redactedRequest = SensitiveDataRedactor.RedactSensitiveFields(requestBody);
- _logger.LogDebug("Received MCP request: {Request}", redactedRequest);
+ var redactedRequest = SensitiveDataRedactor.RedactSensitiveFields(requestBody);
+ _logger.LogDebug("Received MCP request: {Request}", redactedRequest);
- // Process the request through the MCP handler
- var response = await _mcpHandler.HandleRequestAsync(requestBody, cancellationToken);
+ // Process the request through the MCP handler
+ var response = await _mcpHandler.HandleRequestAsync(requestBody, cancellationToken);
- var redactedResponse = SensitiveDataRedactor.RedactSensitiveFields(response);
- _logger.LogDebug("Sending MCP response: {Response}", redactedResponse);
+ var redactedResponse = SensitiveDataRedactor.RedactSensitiveFields(response);
+ _logger.LogDebug("Sending MCP response: {Response}", redactedResponse);
- // Return the JSON-RPC response
+ // Return the JSON-RPC response
return Content(response, "application/json");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs` around lines 46 - 71, The
closing brace for the if (string.IsNullOrWhiteSpace(requestBody)) block is
mis-indented, making the subsequent logic (calls to
SensitiveDataRedactor.RedactSensitiveFields, _logger.LogDebug,
_mcpHandler.HandleRequestAsync and return Content) appear outside the try/method
scope; move the closing brace and align the following lines so they remain
inside the try block and at the same indentation level as the if statement,
ensuring the null-body early return (with _logger.LogWarning and BadRequest)
ends the method and the redaction, logging, handling
(SensitiveDataRedactor.RedactSensitiveFields, _logger.LogDebug,
_mcpHandler.HandleRequestAsync) and final return Content(response,
"application/json") are clearly nested under the try scope.
Comment on lines
+10
to
+48
| public static class SensitiveDataRedactorTests | ||
| { | ||
| public static void RunTests() | ||
| { | ||
| Console.WriteLine("Testing SensitiveDataRedactor...\n"); | ||
|
|
||
| // Test 1: Redact password | ||
| var jsonWithPassword = @"{""username"":""john.doe"",""password"":""secret123""}"; | ||
| var redacted1 = SensitiveDataRedactor.RedactSensitiveFields(jsonWithPassword); | ||
| Console.WriteLine($"Original: {jsonWithPassword}"); | ||
| Console.WriteLine($"Redacted: {redacted1}\n"); | ||
|
|
||
| // Test 2: Redact token and apikey | ||
| var jsonWithToken = @"{""token"":""Bearer abc123"",""apikey"":""xyz789"",""data"":""safe data""}"; | ||
| var redacted2 = SensitiveDataRedactor.RedactSensitiveFields(jsonWithToken); | ||
| Console.WriteLine($"Original: {jsonWithToken}"); | ||
| Console.WriteLine($"Redacted: {redacted2}\n"); | ||
|
|
||
| // Test 3: Nested JSON with sensitive fields | ||
| var nestedJson = @"{""user"":{""username"":""jane"",""password"":""pass456""},""sessionToken"":""token123""}"; | ||
| var redacted3 = SensitiveDataRedactor.RedactSensitiveFields(nestedJson); | ||
| Console.WriteLine($"Original: {nestedJson}"); | ||
| Console.WriteLine($"Redacted: {redacted3}\n"); | ||
|
|
||
| // Test 4: JSON-RPC request with params containing sensitive data | ||
| var jsonRpcRequest = @"{""jsonrpc"":""2.0"",""method"":""authenticate"",""params"":{""username"":""admin"",""password"":""admin123""},""id"":1}"; | ||
| var redacted4 = SensitiveDataRedactor.RedactSensitiveFields(jsonRpcRequest); | ||
| Console.WriteLine($"Original: {jsonRpcRequest}"); | ||
| Console.WriteLine($"Redacted: {redacted4}\n"); | ||
|
|
||
| // Test 5: Invalid JSON (should return metadata only) | ||
| var invalidJson = "not valid json {"; | ||
| var redacted5 = SensitiveDataRedactor.RedactSensitiveFields(invalidJson); | ||
| Console.WriteLine($"Original: {invalidJson}"); | ||
| Console.WriteLine($"Redacted: {redacted5}\n"); | ||
|
|
||
| Console.WriteLine("All tests completed!"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
These are not real tests — no assertions, no test framework, ships in production.
This class uses Console.WriteLine with no assertions, so it can never fail or catch regressions. It also lives in the production project (Web/Resgrid.Web.Mcp) rather than a test project, meaning it ships with the deployed artifact.
Consider either:
- Moving this to a proper test project and using xUnit/NUnit with
Assertcalls, or - Removing it entirely if it was only used for manual verification during development.
Example with xUnit:
[Fact]
public void RedactSensitiveFields_RedactsPassword()
{
var input = @"{""username"":""john.doe"",""password"":""secret123""}";
var result = SensitiveDataRedactor.RedactSensitiveFields(input);
Assert.Contains("***REDACTED***", result);
Assert.DoesNotContain("secret123", result);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.Test.cs` around
lines 10 - 48, SensitiveDataRedactorTests is not a real test (it uses
Console.WriteLine, has no assertions, and lives in production), so either move
it into a proper test project with assertions or remove it; specifically, remove
or relocate the class SensitiveDataRedactorTests and its RunTests method from
the production assembly and implement real unit tests in the test project (e.g.,
using xUnit/NUnit) that call SensitiveDataRedactor.RedactSensitiveFields and use
Assert methods to verify redaction (for example assert redacted output contains
"***REDACTED***" and does not contain original secrets) so regressions can fail
CI instead of only printing to console.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Configuration
Tests & Examples