Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches MCP hosting from stdio to HTTP: adds an HTTP JSON-RPC controller and client, IMcpRequestHandler contract, McpServer.HandleRequestAsync, CORS and port 8080 config, sensitive-data redaction and tests, optional stdio hosting flag, and new MCP-related config flags and defaults. Changes
Sequence DiagramsequenceDiagram
actor Client as HTTP Client
participant Controller as McpController
participant Handler as IMcpRequestHandler
participant Server as McpServer
participant Tools as McpToolRegistry
Client->>Controller: POST /mcp/handle (JSON-RPC)
activate Controller
Controller->>Controller: Validate size (<1MB) & redact request for logs
Controller->>Handler: HandleRequestAsync(requestJson)
activate Handler
Handler->>Server: HandleRequestAsync(requestJson)
activate Server
Server->>Server: Deserialize & route method
Server->>Tools: Invoke tool/provider
alt success
Tools-->>Server: result
Server->>Server: Serialize JSON-RPC response
else internal error
Server->>Server: Create JSON-RPC error (-32603)
end
Server-->>Handler: responseJson
deactivate Server
Handler-->>Controller: responseJson
deactivate Handler
Controller->>Controller: Redact response for logs
Controller-->>Client: JSON-RPC response
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 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
+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.
Web/Resgrid.Web.Mcp/Infrastructure/SensitiveDataRedactor.Test.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Tests/Resgrid.Tests/Web/Mcp/SensitiveDataRedactorTests.cs (1)
61-76: Line 73:Does.Not.Contain("admin")may be fragile if "admin" appears elsewhere.Currently "admin" only appears as the username value, so this works. But if the method name were ever changed to contain "admin" (e.g.,
"method":"admin_authenticate"), this assertion would break. Consider checking the specific value more precisely, though this is a low-risk nit for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Mcp/SensitiveDataRedactorTests.cs` around lines 61 - 76, The assertion IsTooBroad: in the test RedactSensitiveFields_ShouldRedactJsonRpcRequest (SensitiveDataRedactor.RedactSensitiveFields) the check Assert.That(redacted, Does.Not.Contain("admin")) is fragile; change it to assert the specific key/value is redacted instead (for example assert the redacted string does not contain the exact substring "\"username\":\"admin\"" or use a regex/JSON parse to verify that the "username" property value equals RedactedValue), so the method name or other occurrences of "admin" won't cause false failures while still verifying the username was redacted.Web/Resgrid.Web.Mcp/Controllers/McpController.cs (1)
125-140: GET health-check endpoint looks fine.Returns useful connectivity metadata. Consider whether the hardcoded
versionstring ("1.0.0") should be sourced from assembly metadata to avoid drift, but this is a minor nit for a later iteration.🤖 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 125 - 140, The Get method in McpController returns a hardcoded version string ("1.0.0"); update it to read the application assembly version instead to avoid drift by replacing the literal in IActionResult Get() with a runtime lookup (e.g., using typeof(McpController).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion or Assembly.GetEntryAssembly()?.GetName().Version.ToString()) and return that value in the anonymous object so the service/version field reflects the assembly metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/Resgrid.Tests/Resgrid.Tests.csproj`:
- Line 1: The file starts with two UTF-8 BOM characters before the Project XML
(<Project Sdk="Microsoft.NET.Sdk">), causing MSBuild to fail; open the project
file, remove the extra BOM so there is at most one (or save as UTF-8 without
BOM), then re-save the file ensuring your editor/IDE writes only a single UTF-8
BOM (or none) so the XML parser accepts the document.
In `@Tests/Resgrid.Tests/Web/Mcp/SensitiveDataRedactorTests.cs`:
- Around line 153-189: The test currently asserts that the redacted string
Does.Not.Contain("secret") which fails because "secret" is also a JSON property
name; update RedactSensitiveFields_ShouldRedactCommonSensitiveFieldNames to
deserialize the result of
SensitiveDataRedactor.RedactSensitiveFields(jsonWithVariousSensitiveFields) (or
parse it into a JObject) and assert that the sensitive properties (e.g.
"secret", "password", "token", "ssn", "apikey", "api_key", "authorization",
"auth", "credentials", "credit_card"/"creditcard", "cvv", "pin") have values
equal to RedactedValue (or that jObject["secret"].Value<string>() ==
RedactedValue) instead of using Does.Not.Contain substring checks.
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 45-65: Add the Kestrel-level guard by applying the
[RequestSizeLimit] attribute to the McpController action (or the McpController
class) using the same MaxRequestBodySize constant so chunked requests are
rejected before buffering; keep the existing Request.ContentLength manual check
for the friendly JSON-RPC 413 response, and ensure you import the
Microsoft.AspNetCore.Mvc namespace so the [RequestSizeLimit(MaxRequestBodySize)]
attribute is recognized.
---
Duplicate comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 67-92: The block handling an empty requestBody is misindented
causing the closing brace for the if to be aligned incorrectly and making the
following statements (redactedRequest, _logger.LogDebug,
_mcpHandler.HandleRequestAsync, redactedResponse, and return Content(...))
appear outside the intended scope; fix by re-indenting so the closing brace that
ends the if aligns with its opening and ensure the subsequent statements remain
inside the same try/block scope (adjust indentation around requestBody check and
the lines referencing SensitiveDataRedactor.RedactSensitiveFields, _logger,
_mcpHandler.HandleRequestAsync, and return Content(response, "application/json")
accordingly).
- Around line 94-108: The JsonException catch block in McpController (the catch
(JsonException ex) handling where _logger.LogError(ex, "Failed to parse JSON-RPC
request") returns BadRequest with jsonrpc/id/error) is currently returning
ex.Message in the error.data field which leaks internals; remove the data
property from the anonymous error object (or replace it with a generic string
like "Parse error") so the BadRequest payload only contains jsonrpc, id and
error with code -32700 and a non-sensitive message, leaving the logging of ex
intact via _logger.LogError.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Web/Mcp/SensitiveDataRedactorTests.cs`:
- Around line 61-76: The assertion IsTooBroad: in the test
RedactSensitiveFields_ShouldRedactJsonRpcRequest
(SensitiveDataRedactor.RedactSensitiveFields) the check Assert.That(redacted,
Does.Not.Contain("admin")) is fragile; change it to assert the specific
key/value is redacted instead (for example assert the redacted string does not
contain the exact substring "\"username\":\"admin\"" or use a regex/JSON parse
to verify that the "username" property value equals RedactedValue), so the
method name or other occurrences of "admin" won't cause false failures while
still verifying the username was redacted.
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 125-140: The Get method in McpController returns a hardcoded
version string ("1.0.0"); update it to read the application assembly version
instead to avoid drift by replacing the literal in IActionResult Get() with a
runtime lookup (e.g., using
typeof(McpController).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion
or Assembly.GetEntryAssembly()?.GetName().Version.ToString()) and return that
value in the anonymous object so the service/version field reflects the assembly
metadata.
Comment on lines
+153
to
+189
| [Test] | ||
| public void RedactSensitiveFields_ShouldRedactCommonSensitiveFieldNames() | ||
| { | ||
| // Arrange | ||
| var jsonWithVariousSensitiveFields = @"{ | ||
| ""password"":""secret"", | ||
| ""token"":""token123"", | ||
| ""ssn"":""123-45-6789"", | ||
| ""apikey"":""key123"", | ||
| ""api_key"":""key456"", | ||
| ""secret"":""secret789"", | ||
| ""authorization"":""Bearer xyz"", | ||
| ""auth"":""auth123"", | ||
| ""credentials"":""creds"", | ||
| ""credit_card"":""4111111111111111"", | ||
| ""creditcard"":""4111111111111111"", | ||
| ""cvv"":""123"", | ||
| ""pin"":""1234"" | ||
| }"; | ||
|
|
||
| // Act | ||
| var redacted = SensitiveDataRedactor.RedactSensitiveFields(jsonWithVariousSensitiveFields); | ||
|
|
||
| // Assert | ||
| Assert.That(redacted, Does.Contain(RedactedValue), "Should contain redaction marker"); | ||
| Assert.That(redacted, Does.Not.Contain("secret"), "Should not contain 'secret' value"); | ||
| Assert.That(redacted, Does.Not.Contain("token123"), "Should not contain token value"); | ||
| Assert.That(redacted, Does.Not.Contain("123-45-6789"), "Should not contain SSN"); | ||
| Assert.That(redacted, Does.Not.Contain("key123"), "Should not contain apikey value"); | ||
| Assert.That(redacted, Does.Not.Contain("key456"), "Should not contain api_key value"); | ||
| Assert.That(redacted, Does.Not.Contain("secret789"), "Should not contain secret value"); | ||
| Assert.That(redacted, Does.Not.Contain("Bearer xyz"), "Should not contain authorization value"); | ||
| Assert.That(redacted, Does.Not.Contain("auth123"), "Should not contain auth value"); | ||
| Assert.That(redacted, Does.Not.Contain("creds"), "Should not contain credentials value"); | ||
| Assert.That(redacted, Does.Not.Contain("4111111111111111"), "Should not contain credit card number"); | ||
| Assert.That(redacted, Does.Not.Contain("1234"), "Should not contain PIN"); | ||
| } |
Contributor
There was a problem hiding this comment.
Line 178 assertion will fail — "secret" is also a JSON property name in the output.
The redactor replaces values of sensitive fields but keeps the property names. The redacted output will contain "secret":"***REDACTED***", so Does.Not.Contain("secret") fails as a substring match against the key.
Use a more targeted assertion that checks the value was replaced rather than doing a blanket substring search for a word that also appears as a key name.
🐛 Proposed fix
- Assert.That(redacted, Does.Not.Contain("secret"), "Should not contain 'secret' value");
+ Assert.That(redacted, Does.Not.Contain(@"""secret"""), "Should not contain unredacted 'secret' value");Alternatively, deserialize the redacted JSON and assert the property value directly equals "***REDACTED***" — this avoids fragile substring checks entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Web/Mcp/SensitiveDataRedactorTests.cs` around lines 153 -
189, The test currently asserts that the redacted string
Does.Not.Contain("secret") which fails because "secret" is also a JSON property
name; update RedactSensitiveFields_ShouldRedactCommonSensitiveFieldNames to
deserialize the result of
SensitiveDataRedactor.RedactSensitiveFields(jsonWithVariousSensitiveFields) (or
parse it into a JObject) and assert that the sensitive properties (e.g.
"secret", "password", "token", "ssn", "apikey", "api_key", "authorization",
"auth", "credentials", "credit_card"/"creditcard", "cvv", "pin") have values
equal to RedactedValue (or that jObject["secret"].Value<string>() ==
RedactedValue) instead of using Does.Not.Contain substring checks.
| [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 2 days ago
In general, to fix missing CSRF validation on an ASP.NET Core MVC POST action, you add the [ValidateAntiForgeryToken] attribute (or a global filter) so that the framework checks a token submitted with the request. This requires that clients include an anti-forgery token (e.g., via form fields or headers) generated by the application. For endpoints that are intentionally not CSRF-protected (e.g., pure APIs using bearer tokens), you would instead add [IgnoreAntiforgeryToken] to document that choice and, if necessary, configure other protections.
For this specific code, the least invasive way to fix the reported issue without changing existing logic is to decorate the HandleRequest method with [ValidateAntiForgeryToken] in addition to the existing [HttpPost], [Consumes], [Produces], and [RequestSizeLimit] attributes. ASP.NET Core MVC already has ValidateAntiForgeryTokenAttribute in the Microsoft.AspNetCore.Mvc namespace, which is included at the top of this file, so no new imports are needed. The change is localized to the action declaration around line 38–42 in Web/Resgrid.Web.Mcp/Controllers/McpController.cs. No other methods need to be altered.
Suggested changeset
1
Web/Resgrid.Web.Mcp/Controllers/McpController.cs
| @@ -36,6 +36,7 @@ | ||
| /// <param name="cancellationToken">Cancellation token</param> | ||
| /// <returns>JSON-RPC response</returns> | ||
| [HttpPost] | ||
| [ValidateAntiForgeryToken] | ||
| [Consumes("application/json")] | ||
| [Produces("application/json")] | ||
| [RequestSizeLimit(MaxRequestBodySize)] |
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Web/Resgrid.Web.Mcp/Controllers/McpController.cs (1)
130-141: Hardcodedversion = "1.0.0"will silently go stale.Consider reflecting the actual assembly version at runtime to keep this endpoint accurate after upgrades.
♻️ Proposed fix
+using System.Reflection; [HttpGet] public IActionResult Get() { + var version = Assembly.GetExecutingAssembly() + .GetCustomAttribute<AssemblyInformationalVersionAttribute>() + ?.InformationalVersion ?? "unknown"; return Ok(new { service = "Resgrid MCP Server", - version = "1.0.0", + version, protocol = "Model Context Protocol", transport = "HTTP", endpoint = "/mcp", method = "POST" }); }🤖 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 130 - 141, Replace the hardcoded version string in McpController.Get() with the assembly's runtime version: update the Get() method to retrieve the current assembly version (e.g., use typeof(McpController).Assembly.GetName().Version or FileVersionInfo.GetVersionInfo(typeof(McpController).Assembly.Location).ProductVersion), guard against null and call ToString() as needed, then use that value for the version property in the returned object so the endpoint reflects the actual assembly version at runtime.
🤖 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`:
- Line 66: Pass the incoming CancellationToken into the StreamReader read call
so reads are abortable: change the call that sets requestBody (the await
reader.ReadToEndAsync()) to use the CancellationToken overload (await
reader.ReadToEndAsync(cancellationToken)); ensure the surrounding action or
method exposes/receives a CancellationToken parameter (or uses
HttpContext.RequestAborted) and propagate that token into the ReadToEndAsync
call so client disconnects/timeouts cancel the body read.
- Around line 52-61: The 413 response currently uses the JSON-RPC error code
-32600 (Invalid Request); change it to an implementation-defined server error in
the -32000..-32099 range (e.g., -32001) so callers can distinguish a
payload-too-large from other invalid-request cases. Update the error.code in the
block that returns StatusCode(StatusCodes.Status413PayloadTooLarge, ...) to use
the new code (reference MaxRequestBodySize and the 413 response block in
McpController), and ensure the empty-body branch retains or uses a different
code so the two paths are distinct.
- Around line 38-42: The POST endpoint method HandleRequest and the health-check
Get endpoint lack authorization, leaving them publicly accessible; fix by
applying [Authorize] to the HandleRequest method (and the Get health-check
action) or by registering JWT Bearer authentication and enabling
app.UseAuthentication() and app.UseAuthorization() with a global authorization
policy in your startup/Program configuration (register Authentication in
ConfigureServices and enforce Authorization globally or add the attribute to the
controller/actions); locate the HandleRequest method and the Get health-check
action in McpController and update them or update the service registration in
Startup.cs/Program.cs accordingly.
---
Duplicate comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 68-93: The closing brace for the if
(string.IsNullOrWhiteSpace(requestBody)) block is mis-indented causing the
subsequent lines (calls to SensitiveDataRedactor.RedactSensitiveFields,
_logger.LogDebug, _mcpHandler.HandleRequestAsync, redaction of response and
return Content) to appear outside the try-block; fix by moving the closing brace
so all lines from var redactedRequest =
SensitiveDataRedactor.RedactSensitiveFields(requestBody); through return
Content(response, "application/json"); are indented inside the try scope (align
their indentation with the if-block body) to make control flow visually correct
and consistent.
- Around line 95-109: The JsonException catch in McpController.cs currently
returns ex.Message which leaks internals; either remove the entire catch block
if _mcpHandler.HandleRequestAsync already handles deserialization errors, or
keep the catch but stop exposing exception details by replacing data =
ex.Message with data = null (or a generic string like "Parse error") while still
calling _logger.LogError(ex, "..."); update the catch that references
JsonException ex and the returned anonymous JSON payload (jsonrpc/id/error)
accordingly.
---
Nitpick comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 130-141: Replace the hardcoded version string in
McpController.Get() with the assembly's runtime version: update the Get() method
to retrieve the current assembly version (e.g., use
typeof(McpController).Assembly.GetName().Version or
FileVersionInfo.GetVersionInfo(typeof(McpController).Assembly.Location).ProductVersion),
guard against null and call ToString() as needed, then use that value for the
version property in the returned object so the endpoint reflects the actual
assembly version at runtime.
Comment on lines
+38
to
+42
| [HttpPost] | ||
| [Consumes("application/json")] | ||
| [Produces("application/json")] | ||
| [RequestSizeLimit(MaxRequestBodySize)] | ||
| public async Task<IActionResult> HandleRequest(CancellationToken cancellationToken) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine McpController.cs
find . -name "McpController.cs" -type fRepository: Resgrid/Core
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Once we have the path, let's get the full controller content
cat -n ./Web/Resgrid.Web.Mcp/Controllers/McpController.csRepository: Resgrid/Core
Length of output: 4909
🏁 Script executed:
#!/bin/bash
# Look for Program.cs or Startup.cs in the web project
find ./Web/Resgrid.Web.Mcp -name "Program.cs" -o -name "Startup.cs" | head -5Repository: Resgrid/Core
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for [Authorize] attributes in the controller and related files
rg -n "\[Authorize" --type=cs Web/Resgrid.Web.Mcp/Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for authorization policy configuration
rg -n "RequireAuthorization|FallbackPolicy|DefaultPolicy|AddAuthorization" --type=cs Web/Resgrid.Web.Mcp/Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the project file for JWT dependency
find ./Web/Resgrid.Web.Mcp -name "*.csproj" -exec cat {} \; | grep -i "jwtbearer\|authentication" -B2 -A2Repository: Resgrid/Core
Length of output: 498
🏁 Script executed:
#!/bin/bash
# Check Program.cs
cat -n ./Web/Resgrid.Web.Mcp/Program.csRepository: Resgrid/Core
Length of output: 3728
🏁 Script executed:
#!/bin/bash
# Check Startup.cs
cat -n ./Web/Resgrid.Web.Mcp/Startup.csRepository: Resgrid/Core
Length of output: 6032
Apply [Authorize] to MCP endpoint or configure authentication policy.
The HandleRequest POST method (lines 38-42) and Get health check endpoint lack authorization. No global authorization policy is configured in Startup.cs or Program.cs, leaving these endpoints publicly accessible. Despite Microsoft.AspNetCore.Authentication.JwtBearer being a listed dependency, authentication is not registered in the service configuration.
If authentication is required, apply the attribute:
Suggested fix
[HttpPost]
[Consumes("application/json")]
[Produces("application/json")]
[RequestSizeLimit(MaxRequestBodySize)]
+ [Authorize]
public async Task<IActionResult> HandleRequest(CancellationToken cancellationToken)Alternatively, configure authentication and authorization middleware in Startup.cs and enable a global policy via app.UseAuthentication() and app.UseAuthorization().
🤖 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 38 - 42, The
POST endpoint method HandleRequest and the health-check Get endpoint lack
authorization, leaving them publicly accessible; fix by applying [Authorize] to
the HandleRequest method (and the Get health-check action) or by registering JWT
Bearer authentication and enabling app.UseAuthentication() and
app.UseAuthorization() with a global authorization policy in your
startup/Program configuration (register Authentication in ConfigureServices and
enforce Authorization globally or add the attribute to the controller/actions);
locate the HandleRequest method and the Get health-check action in McpController
and update them or update the service registration in Startup.cs/Program.cs
accordingly.
Comment on lines
+52
to
+61
| return StatusCode(StatusCodes.Status413PayloadTooLarge, new | ||
| { | ||
| jsonrpc = "2.0", | ||
| id = (object)null, | ||
| error = new | ||
| { | ||
| code = -32600, | ||
| message = $"Request body too large. Maximum size is {MaxRequestBodySize} bytes." | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Use a custom server error code for the 413 case; -32600 misrepresents the failure.
JSON-RPC 2.0 reserves -32600 for "Invalid Request" (malformed or incomplete Request object). A payload-size limit violation is a distinct server-side constraint, better expressed as an implementation-defined code in the -32000–-32099 range. Currently both the empty-body and the too-large paths return -32600, which prevents callers from distinguishing them.
♻️ Proposed fix
+ private const int JsonRpcPayloadTooLarge = -32001; // server-defined: request too large
// ...
return StatusCode(StatusCodes.Status413PayloadTooLarge, new
{
jsonrpc = "2.0",
id = (object)null,
error = new
{
- code = -32600,
+ code = JsonRpcPayloadTooLarge,
message = $"Request body too large. Maximum size is {MaxRequestBodySize} bytes."
}
});📝 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
| return StatusCode(StatusCodes.Status413PayloadTooLarge, new | |
| { | |
| jsonrpc = "2.0", | |
| id = (object)null, | |
| error = new | |
| { | |
| code = -32600, | |
| message = $"Request body too large. Maximum size is {MaxRequestBodySize} bytes." | |
| } | |
| }); | |
| return StatusCode(StatusCodes.Status413PayloadTooLarge, new | |
| { | |
| jsonrpc = "2.0", | |
| id = (object)null, | |
| error = new | |
| { | |
| code = JsonRpcPayloadTooLarge, | |
| message = $"Request body too large. Maximum size is {MaxRequestBodySize} bytes." | |
| } | |
| }); |
🤖 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 52 - 61, The
413 response currently uses the JSON-RPC error code -32600 (Invalid Request);
change it to an implementation-defined server error in the -32000..-32099 range
(e.g., -32001) so callers can distinguish a payload-too-large from other
invalid-request cases. Update the error.code in the block that returns
StatusCode(StatusCodes.Status413PayloadTooLarge, ...) to use the new code
(reference MaxRequestBodySize and the 413 response block in McpController), and
ensure the empty-body branch retains or uses a different code so the two paths
are distinct.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Web/Resgrid.Web.Mcp/Controllers/McpController.cs (1)
130-141: Hard-codedversion = "1.0.0"in the health-check response.Consider reading the version from the assembly to keep it in sync automatically, avoiding stale values after future releases.
♻️ Proposed fix
- version = "1.0.0", + version = typeof(McpController).Assembly.GetName().Version?.ToString() ?? "unknown",🤖 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 130 - 141, The Get() action in McpController currently returns a hard-coded version string; update it to read the assembly version dynamically (e.g., via typeof(McpController).Assembly or Assembly.GetExecutingAssembly()) and use the assembly's informational/file/version attribute (AssemblyInformationalVersion or AssemblyVersion) to populate the version field in the anonymous object returned by Get(), so the health-check reflects the actual build version.
🤖 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`:
- Line 9: Rename the project-local IMcpRequestHandler interface to live in a
project-specific namespace (e.g., change namespace in IMcpRequestHandler.cs to
Resgrid.Web.Mcp.ModelContextProtocol) and keep the interface name unchanged;
then update McpController.cs to replace the using ModelContextProtocol.Server;
with using Resgrid.Web.Mcp.ModelContextProtocol; (and update any other
references/imports in the project to the new namespace) so the controller
compiles against the project-local IMcpRequestHandler instead of the external
SDK namespace.
---
Duplicate comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 38-42: The POST endpoint method HandleRequest and the Get
health-check endpoint are missing authorization; add the [Authorize] attribute
to the HandleRequest method (and to the Get health-check controller action) to
enforce authentication, and ensure JWT Bearer authentication is configured in
startup (e.g., services.AddAuthentication(...).AddJwtBearer(...) and
app.UseAuthentication()/UseAuthorization()); if the endpoints must remain
public, add a clear comment above the methods explaining why they're
intentionally open and document expected security controls.
- Around line 95-109: The catch block for JsonException in McpController.cs is
dead (McpServer.HandleRequestAsync already catches deserialization exceptions)
and exposing ex.Message leaks internal details; remove the specific catch
(JsonException ex) entirely or, if you prefer to keep explicit handling, change
it to a generic catch that omits internal error text (e.g., return the same
JSON-RPC error object but with no data or with a generic message), referencing
the catch block around JsonException and the McpServer.HandleRequestAsync
deserialization handling to ensure you don't duplicate error handling.
- Around line 52-62: The JSON-RPC error code used in the 413 response in
McpController (the returned anonymous object with jsonrpc/id/error) is
incorrect; change the error.code from -32600 to an implementation-defined code
in the reserved server error range (for example -32001) so callers can
distinguish a payload-too-large condition from an "Invalid Request"; update the
error.message if desired but primarily replace the numeric code in the
StatusCode(StatusCodes.Status413PayloadTooLarge, new { jsonrpc = "2.0", id =
(object)null, error = new { code = /* replace -32600 */ -32001, message =
$"Request body too large. Maximum size is {MaxRequestBodySize} bytes." } })
return in McpController.cs.
- Around line 64-93: The block under the try has inconsistent indentation: the
using var reader = new StreamReader(Request.Body), the redaction/logging lines
(SensitiveDataRedactor.RedactSensitiveFields and _logger.LogDebug), the call to
_mcpHandler.HandleRequestAsync, and the return Content(response,
"application/json") should all be at the same indent level inside the try so the
using scope and control flow are clear; fix by reindenting those statements to
the same level (so reader is declared and disposed for the entire try block
section), ensuring the guard that returns BadRequest stays properly nested, and
verify the scoped disposal of reader still encloses the read and subsequent
processing (_mcpHandler.HandleRequestAsync and the final return
Content(response, "application/json")).
---
Nitpick comments:
In `@Web/Resgrid.Web.Mcp/Controllers/McpController.cs`:
- Around line 130-141: The Get() action in McpController currently returns a
hard-coded version string; update it to read the assembly version dynamically
(e.g., via typeof(McpController).Assembly or Assembly.GetExecutingAssembly())
and use the assembly's informational/file/version attribute
(AssemblyInformationalVersion or AssemblyVersion) to populate the version field
in the anonymous object returned by Get(), so the health-check reflects the
actual build version.
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.Extensions.Logging; | ||
| using ModelContextProtocol.Server; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
fd "IMcpRequestHandler.cs" --exec head -30 {}Repository: Resgrid/Core
Length of output: 665
Move IMcpRequestHandler to a project-specific namespace.
The IMcpRequestHandler interface is declared in the external SDK's ModelContextProtocol.Server namespace, which is a namespace pollution pattern and violates C# design conventions. Project-local interfaces should use the project's own namespace hierarchy (e.g., Resgrid.Web.Mcp.ModelContextProtocol). This prevents future naming collisions if the official SDK introduces its own IMcpRequestHandler. Update the namespace in IMcpRequestHandler.cs and adjust the corresponding using directive in McpController.cs.
🤖 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` at line 9, Rename the
project-local IMcpRequestHandler interface to live in a project-specific
namespace (e.g., change namespace in IMcpRequestHandler.cs to
Resgrid.Web.Mcp.ModelContextProtocol) and keep the interface name unchanged;
then update McpController.cs to replace the using ModelContextProtocol.Server;
with using Resgrid.Web.Mcp.ModelContextProtocol; (and update any other
references/imports in the project to the new namespace) so the controller
compiles against the project-local IMcpRequestHandler instead of the external
SDK namespace.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
Infrastructure
Tests