Map and authenticate MCP HTTP at /mcp - #1364
Conversation
|
Exact-head local gate for
Two independent adversarial reviews are running. Please review this exact head as well. @codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the Model Context Protocol (MCP) HTTP transport security boundary by consolidating standalone and co-hosted endpoints under a single /mcp route, partitioning rate limits by opaque API key IDs, and enforcing loopback-only defaults for standalone hosts. Comprehensive integration tests and documentation updates have been added to support these changes. Feedback on the changes suggests adding defensive null checks for configuration and extension method parameters, trimming host header values before wildcard validation, and making API key ID resolution in the rate limiter more robust by handling non-Guid types gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42711bce2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent adversarial security/correctness review
Reviewed exact commit: aaff46955199fb32b90124fe8c08ec663b4323f4
Verdict: changes required; do not merge this head.
I completed my source conclusions before the mandatory final discussion refresh. That refresh revealed public bot threads matching the first two findings below; I independently verified the third thread against the actual CORS policy as part of the required zero-skip discussion pass.
MEDIUM
-
Standalone Host filtering does not reject every ASP.NET any-host value.
ApplyStandaloneMcpHostSecurityonly replaces a blank value or the exact string*. ASP.NET HostFiltering treats0.0.0.0and[::]as top-level wildcards too, and one wildcard inside a semicolon-delimited list (for examplelocalhost;*) disables filtering for all non-empty hosts. Those values are preserved here, contradicting the fail-closed standalone guarantee. Split/trim the entries, reject/replace any of*,0.0.0.0, or[::], and cover each form plus mixed lists in tests. Existing thread: #1364 (comment) and the broader exact-value thread now on the PR. -
Missing/malformed/nonexistent/revoked-key attempts bypass rate limiting. Both hosts place
ApiKeyMiddlewarebeforeUseRateLimiter(), and every rejected credential returns without calling_next. The endpoint policy and the new IP fallback branch therefore never execute for failed authentication; a public/tunnel deployment can force unbounded API-key database lookups without ever receiving 429. Add an IP-scoped pre-auth limiter (or equivalent two-stage throttling) and a regression showing repeated invalid-key requests are throttled while valid keys retain independent opaque-ID partitions. Existing thread: #1364 (comment). -
The co-hosted
/mcproute is cross-origin enabled despite the documented boundary.UseCors("AllowFrontend")is global and its policy explicitly allows credentials,Authorization, and all MCP HTTP methods for every configured frontend origin. Thus a hosted deployment withCors:AllowedOriginspermits browser JavaScript from those origins to call/mcp, contrary to “Cross-origin browser MCP is not enabled.” Give MCP an explicit no-CORS/restricted endpoint policy or exclude it from the global frontend policy, then add a preflight regression. Existing thread: #1364 (comment).
Existing discussion disposition
- All 6 current inline threads are unresolved and must be addressed before merge.
- The Gemini suggestion to accept arbitrary
HttpContext.Itemsvalues viaToString()is not a correctness improvement:ApiKeyMiddlewareis the sole writer and stores the entityGuid; retaining the type check fails safely if that contract is broken. - The null-guard suggestions concern internal calls whose receivers are constructed non-null. The Host-trimming portion is directionally valid but insufficient because ASP.NET recognizes additional any-host values and wildcard list entries.
Verified clean areas
- One shared explicit
MapMcp("/mcp")mapping is used by standalone and co-hosted hosts;StartsWithSegments("/mcp")protects all methods/subpaths while/mcpfoois excluded. - Valid keys populate both user identity and the opaque key ID before authorization/rate partitioning; missing, malformed, nonexistent, revoked, expired, and inactive-user paths fail closed.
- MCP SDK 1.4.1 defaults to the current HTTP request execution context for handlers, so
IHttpContextAccessorobserves the credential revalidated on each request; the scoped board-resource integration proves separate users see only their own boards. - No raw API key or MCP session ID was added to docs, telemetry, or rate-limit keys; the checked-in client example remains environment-backed.
- Stdio remains on its original transport/provider path.
Evidence
- Guarded clean worktree and exact-head/base diff inspected.
dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~McpHttpTransportApiKeyTests|FullyQualifiedName~FallbackPolicyTests|FullyQualifiedName~ApiKeyMiddlewareTests|FullyQualifiedName~RateLimiting"— 56 passed, 0 failed on this head.- CI snapshot at review time: 23 passing, 2 API Integration jobs still in progress, 8 path-gated jobs skipped.
- A separate ad-hoc runtime probe for rejected-request throttling was attempted but not treated as evidence: the external harness hit concurrent test-binary locks, then WebApplicationFactory content-root discovery. The middleware control flow itself is conclusive; no repository files were changed.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent adversarial protocol/test/operations review
Reviewed exact head: aaff46955199fb32b90124fe8c08ec663b4323f4
Verdict: changes required; do not merge this head.
I completed the source/protocol pass before the mandatory final discussion refresh. That refresh exposed six live threads; I then independently verified every thread against this exact head.
MEDIUM
-
Rejected credentials bypass the advertised MCP rate limit. In both hosts
ApiKeyMiddlewareruns beforeUseRateLimiter(), and every missing/malformed/nonexistent/revoked/expired/inactive-user branch returns without calling_next. Those attempts never reach the endpoint limiter or its IP fallback, so an exposed/tunnelled host permits unbounded key hashing/database lookups and never returns 429. Add a pre-auth IP budget (or equivalent two-stage scheme) and prove repeated invalid attempts throttle while valid keys retain per-key partitions. Existing thread: #1364 (comment). -
Standalone Host hardening accepts ASP.NET any-host equivalents. The helper replaces only blank or exact
*;0.0.0.0,[::], whitespace variants, and semicolon lists containing a wildcard survive. ASP.NET treats those as allowing every non-empty Host, contradicting the documented fail-closed boundary. Split/trim entries, reject every any-host form, and test single plus mixed-list cases. Existing threads: #1364 (comment) and #1364 (comment). -
Co-hosted
/mcpis cross-origin enabled despite the README claim. GlobalUseCors(AllowFrontend)runs before MCP auth and permits credentials,Authorization, and MCP HTTP methods for configured frontend origins. Browser JavaScript at an allowed hosted frontend origin can therefore call/mcp; “Cross-origin browser MCP is not enabled” is false. Exclude/mcpfrom that policy or attach an explicit restricted/no-CORS policy and add a preflight regression. Existing thread: #1364 (comment). -
The required expired-key HTTP path remains false-green. Issue #1338 explicitly requires a real expired key against
/mcp, and the test-class summary claims expired-key coverage, but the suite has no expired-key MCP test.CreateApiKey_WithExpiration_SetsExpiresAtonly creates a future expiry; the domain suite also never provesIsActivetransitions false after expiry. Add a deterministic persisted expired key and assert the real route returns the same generic 401 contract.
LOW
mcp-http.example.jsonis presented as client-neutral although its interpolation is Claude Code-specific. README introduces Claude Code and Cursor, then tells “the client” to use this file.${VAR}and${VAR:-default}are explicitly Claude Code.mcp.jsonsyntax (https://code.claude.com/docs/en/mcp); Cursor documents a separatemcp.jsonclient surface (https://docs.cursor.com/context/model-context-protocol). Label this example Claude Code-only and provide a validated Cursor variant/instructions, or replace it with explicit placeholders and client-specific secret-safe expansion guidance.
Existing-thread disposition
- All 6 inline threads are unresolved and must be replied to/fixed before merge.
- Keep the strict
Guidcontract forMcpApiKeyId; the middleware is the sole writer, and accepting arbitraryToString()values weakens the invariant. - The two null-guard suggestions are internal-call style, not runtime defects. The Host-trimming suggestion is subsumed by finding 2 and is insufficient alone.
Exact-head proof
- Focused MCP HTTP suite: 30/30 passed; API-key domain suite: 11/11 passed.
- Fresh standalone/shared-SQLite replay: missing key
401; initialize200with session ID; initialized202; board resource200and contained the REST-created board;/404; hostile Host400under the default guard. mcp-http.example.jsonparses;git diff --checkpasses; worktree remains clean.- Telemetry assertion is now correlated to the actual request via unique
X-Request-Id. Shared rate-limit tests cover the JSON 429 body; the MCP test correctly proves key partitioning, but not rejected-attempt throttling. - CI snapshot: every completed exact-head job passes; required E2E Smoke is still pending, so merge state remains
UNSTABLE.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent protocol/test/operations re-review
Reviewed exact head: c292ceeb82fb472adc27014252301ccce02394e2
Verdict: one LOW test-contract finding remains; do not merge until it is fixed and the live threads are dispositioned.
LOW
- The new pre-authentication 429 regression does not lock the bespoke JSON error contract.
McpEndpoint_RejectedCredentials_AreRateLimitedBeforeDatabaseLookupinbackend/tests/Taskdeck.Api.Tests/McpHttpTransportApiKeyTests.csasserts 429,Retry-After, andX-RateLimit-Policy, but never reads the response body or content type. This limiter uses its ownMcpAuthenticationAttemptLimiter.WriteRejectedAsyncimplementation rather than the already-covered sharedRateLimiterOptions.OnRejectedpath, soApiErrorResponse.errorCode, its non-empty message, or JSON content type can regress while the test remains green. Parse the payload and assertTooManyRequestsplus a non-empty message (and preferably JSON content type), matching the repository's existingAssertThrottleContractAsyncpattern.
Repaired boundaries independently verified
- Expired key: the test creates a real key through REST, backdates its persisted
ExpiresAt, then reaches the actual/mcpmiddleware path and gets 401 without a session ID. - Two-stage throttling: both hosts place the client-address limiter before API-key parsing/database lookup, while valid requests continue to the separate opaque-key endpoint limiter. The focused tests prove invalid-key 401 then pre-auth 429 and two keys owned by one user retain independent budgets.
- CORS: the mapped MCP endpoint carries
DisableCorsAttribute; the preflight regression proves no allow-origin, credentials, methods, or headers are emitted. - Host filtering: blank, exact/mixed
*, whitespace,0.0.0.0, and[::]forms all collapse to the loopback allowlist; explicit hosts remain unchanged. - Client example/docs: the example is renamed
mcp-claude-code-http.example.json; README labels${VAR}/${VAR:-default}as Claude Code-specific and gives separate secret-safe guidance for Cursor/other clients. STATUS, MASTERPLAN, TESTING_GUIDE, and CONFIGURATION_REFERENCE match the repaired behavior.
Evidence on this head
- Focused API slice: 43/43 passed (38 MCP HTTP + 5 rate-limit validator tests).
- API-key domain slice: 11/11 passed.
- Real standalone launch with
AllowedHosts=localhost;*and one pre-auth permit: hostile Host400; first missing-key attempt401; second attempt429; root404. The live 429 wasapplication/json; charset=utf-8, policyMcpAuthenticationPerIp, positiveRetry-After, and body{errorCode:TooManyRequests,message:...}. - Docs governance, golden principles, renamed example JSON parse,
git diff --check, and clean-worktree checks passed.
Live PR state
- All six existing inline threads were re-read. Their underlying code concerns are now fixed or invalidated by the exact producer contract, but every thread is still unresolved and needs an evidence-backed reply/resolution before merge.
- The only conversation comment still records stale
42711bceevidence; post a fresh exact-head repair/evidence mapping. - CI snapshot: 23 passing, 9 path-gated skipped, 2 API Integration jobs still in progress; merge state remains
UNSTABLE.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Exact-head repaired security re-review
Reviewed commit: c292ceeb82fb472adc27014252301ccce02394e2
Code verdict: no further findings in the repaired security scope.
I independently re-traced both host pipelines and ran focused evidence for the three prior security findings:
-
Standalone Host filtering now fails closed for ASP.NET's complete any-host set. The helper splits semicolon lists with trimming and replaces the configuration when any entry is
*,0.0.0.0, or[::]; blank values are also replaced. The theory covers blank/whitespace, each any-host form, and mixed lists, while a specific host is preserved. I cross-checked the exact framework wildcard set against ASP.NET 8 HostFiltering/HostString behavior rather than assuming string equivalence: https://github.com/dotnet/aspnetcore/blob/v8.0.24/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs and https://github.com/dotnet/aspnetcore/blob/v8.0.24/src/Http/Http.Abstractions/src/HostString.cs. -
The pre-authentication IP limiter now precedes every rejected-key return in both pipelines.
McpAuthenticationRateLimitingMiddlewareruns beforeApiKeyMiddlewarein co-hosted and standalone HTTP, so missing/malformed/nonexistent/revoked/expired/inactive-user attempts cannot reach an unbounded early return. Its regression proves a nonexistent key receives 401 then 429 with the pre-auth policy header. Valid keys continue throughUseRateLimiter(); the independent-key regression proves the endpoint policy still partitions by opaque key ID and returnsMcpPerApiKeyon exhaustion. -
/mcpexplicitly disables inherited credentialed CORS. The shared endpoint mapping attachesDisableCorsAttribute, so both hosts use the same route metadata. The allowed-frontend-origin preflight regression proves no allow-origin, allow-credentials, allow-methods, or allow-headers response is emitted.
All six live-thread dispositions
- Strict
Guidhandling forMcpApiKeyIdremains correct:ApiKeyMiddlewareis the sole writer, and failing closed on a broken internal type contract is safer than accepting arbitraryToString()values. - The configuration and endpoint extension null guards are now present.
- The Host trimming/full-wildcard-list concern is fixed.
- The broader any-host thread is outdated by the repair and fixed.
- The no-CORS and rejected-credential throttling concerns are fixed.
The mandatory final refresh still shows all 6 threads unresolved with no fix-evidence replies. That is a review-policy blocker even though the code is clean; reply with finding → repair commit/evidence and resolve each thread before merge.
Evidence
dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~McpHttpTransportApiKeyTests|FullyQualifiedName~OptionsValidationTests"— 128 passed, 0 failed, 0 skipped.git diff --check aaff4695..HEAD— passed.- Exact-head CI snapshot: 24 passing, 1 pending (
API Integration / API Integration (windows-latest)), 9 path-gated skips. Do not merge until the remaining exact-head job is green. - Worktree remained clean; no edits or merge performed.
Repair evidence on exact head
|
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Exact-head test-only delta re-review
Reviewed commit: 202736f23529d9d014b208c707dca2954ee1b5bf
Verdict: no findings in the c292ceeb..202736f2 delta.
The new commit changes only McpHttpTransportApiKeyTests.cs: it adds the existing API error-contract imports and strengthens McpEndpoint_RejectedCredentials_AreRateLimitedBeforeDatabaseLookup to prove the pre-auth 429 response is JSON, deserializes as ApiErrorResponse, carries ErrorCodes.TooManyRequests, and has a non-empty message. Those assertions match McpAuthenticationAttemptLimiter.WriteRejectedAsync and complement the already-asserted status, Retry-After, and X-RateLimit-Policy values. The production implementation is unchanged, so the clean security conclusions from c292ceeb remain valid.
Evidence
git diff --check c292ceeb82fb472adc27014252301ccce02394e2..202736f23529d9d014b208c707dca2954ee1b5bf— passed.- Exact strengthened test, built and run from this SHA with an isolated artifacts directory — 1 passed, 0 failed, 0 skipped.
- The first attempt using the shared Release output did not reach tests because another active
testhostheld the DLLs; I did not stop it. The isolated-output rerun is the verification result. - Final discussion refresh: all 6/6 original threads now have evidence replies and are resolved; I read each disposition and found no remaining unaddressed comment.
- Exact-head CI snapshot: 17 passing, 7 pending, 10 path-gated/skipped. Do not merge until every required exact-head job is green.
- Review worktree remained clean; no edits or merge performed.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Final exact-head protocol/test/operations re-review
Reviewed exact head: 202736f23529d9d014b208c707dca2954ee1b5bf
Verdict: no findings. The prior LOW test-contract finding is resolved.
The c292ceeb..202736f2 delta is test-only and directly locks the bespoke pre-authentication 429 contract: application/json, deserialization as the shared ApiErrorResponse, ErrorCodes.TooManyRequests, and a non-empty message, in addition to the existing status, Retry-After, and X-RateLimit-Policy assertions. Those checks match McpAuthenticationAttemptLimiter.WriteRejectedAsync. Production source is unchanged, so the previously verified repaired security/protocol conclusions remain valid.
Exact-head evidence
- Full
McpHttpTransportApiKeyTestsclass: 38 passed, 0 failed, including the strengthened rejection-throttle test. git diff --check ccd7bf93721c07171099383c77bfb508c23fbf48..202736f23529d9d014b208c707dca2954ee1b5bfpassed; review worktree is clean.- The first default-output test invocation did not reach test execution because another active
testhostheld the shared Release DLLs. I did not terminate that process; the same exact-head suite was rebuilt and passed using an isolatedBaseOutputPath. This is an environmental output collision, not a product/test failure. - Final mandatory discussion refresh: 6/6 original inline threads have direct dispositions and are resolved; the fresh repair-evidence conversation comment covers every prior finding. No new actionable comment was present.
- CI snapshot at posting: 21 successful, 4 pending, 9 path-gated/skipped, 0 failed; merge state remains
UNSTABLE. Do not merge until every required exact-head check is green.
No files were edited and no merge was performed.
Final merge gate ? exact head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202736f235
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var containsAnyHost = allowedHosts? | ||
| .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) | ||
| .Any(host => host is "*" or "0.0.0.0" or "[::]") == true; | ||
| if (string.IsNullOrWhiteSpace(allowedHosts) || containsAnyHost) |
There was a problem hiding this comment.
When standalone MCP is exposed with --host and AllowedHosts is accidentally set to only separators such as ; or ;;, this check does not treat it as blank and leaves the value unchanged. ASP.NET's host-filtering setup then splits the value into zero hosts and falls back to *, so hostile Host headers are accepted even though this helper is supposed to fail closed for blank/any-host config. Parse the tokens once and replace when the token list is empty.
Useful? React with 👍 / 👎.
| // Bound all authentication attempts by client address before parsing a key or | ||
| // querying the database. Valid requests also reach the later per-key policy. | ||
| if (mcpRateLimitingSettings.Enabled) | ||
| { | ||
| mcpHttpApp.UseMiddleware<Taskdeck.Api.Middleware.McpAuthenticationRateLimitingMiddleware>(); |
There was a problem hiding this comment.
If standalone HTTP is put behind a reverse proxy or tunnel, this branch never runs UseForwardedHeaders before the new pre-auth limiter, so RemoteIpAddress remains the proxy address. In that deployment every external client shares one McpAuthenticationPerIp bucket, letting a noisy or unauthenticated client 429 valid MCP users before their per-key budgets are considered; wire the same trusted forwarded-header handling used by the co-hosted pipeline before this middleware.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| using var lease = await limiter.AcquireAsync(context); |
There was a problem hiding this comment.
Because this middleware acquires the McpAuthenticationPerIp lease for every /mcp request before ApiKeyMiddleware can prove the key is valid, successful traffic still consumes the shared IP auth bucket. With three API keys behind one NAT/proxy each staying under its 60/min per-key limit, the aggregate 120/min default is exhausted and later valid calls get 429 before the opaque-key policy runs. Gate this limiter to rejected/missing credentials or otherwise exempt validated keys so valid keys keep independent budgets.
Useful? React with 👍 / 👎.
Summary
/mcproute/mcpVerification
dotnet build backend/Taskdeck.sln -c Release -m:1— 0 errors; 12 pre-existing test warningsdotnet test backend/Taskdeck.sln -c Release -m:1 --no-buildon exact202736f2— 7,222 passed, 0 failed, 1 known INV-09 skipTooManyRequestscontractResidual boundaries
AllowedHostsand TLS terminationCloses #1338