MCP-03: HTTP transport and API key authentication#819
Conversation
Extends the --mcp flag to support HTTP transport alongside the existing stdio transport. When launched with --mcp --transport http, builds a minimal WebApplication with only the MCP endpoint and API key middleware, skipping JWT, CORS, SignalR, Swagger, and frontend middleware. Port defaults to 5001 and can be overridden with --port.
Adds create, list, and revoke subcommands under the api-key group. Create generates a tdsk_ prefixed key and outputs the plaintext once. List shows all keys for the CLI actor. Revoke accepts --name or --id to deactivate a key. Follows existing CLI patterns with JSON output.
12 tests covering create (with/without expiration), list, revoke (by name and by id), error handling for missing args, invalid inputs, nonexistent keys, and unknown subcommands. Tests use the existing CliHarness pattern running against ephemeral SQLite.
Reject unknown transport values with a clear error message instead of silently falling through to stdio. Validate that --port is in the valid range 1-65535. Found during adversarial self-review.
Adversarial Self-ReviewFixed in follow-up commit
Reviewed and accepted
Confirmed secure
|
![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 introduces a dedicated HTTP transport mode for the Model Context Protocol (MCP) server, enabling remote access via API key authentication. Key additions include a standalone MCP HTTP server mode in Program.cs, a new api-key command group in the Taskdeck CLI for managing keys (create, list, revoke), and comprehensive integration tests. Feedback focuses on security and architectural robustness: a hardcoded system username in the CLI poses a hijacking risk if public registration is enabled, and the HTTP server is currently restricted to localhost, which may hinder containerized or remote deployments. Additionally, manual service instantiation in the DI container should be replaced with standard registration to avoid brittleness, and the CLI's 'revoke by name' logic should handle potential name duplicates more safely.
|
|
||
| private async Task<Guid> GetOrCreateCliActorIdAsync() | ||
| { | ||
| const string actorUsername = "taskdeck_cli_actor"; |
There was a problem hiding this comment.
Using a hardcoded, predictable username for the CLI actor poses a security risk. If public registration is enabled, a malicious user could register an account with the username taskdeck_cli_actor before the CLI is first used. Any API keys subsequently created via the CLI would then be associated with the attacker's account, granting them access to the MCP server. Consider using a reserved system identifier or a non-registrable prefix for system actors.
backend/src/Taskdeck.Api/Program.cs
Outdated
| var mcpPort = 5001; | ||
| for (int i = 0; i < args.Length - 1; i++) | ||
| { | ||
| if (string.Equals(args[i], "--port", StringComparison.OrdinalIgnoreCase) |
There was a problem hiding this comment.
Binding to localhost hardcodes the server to only accept local connections. The PR description and comments suggest this mode is intended for cloud or remote use. In containerized environments (like Docker) or remote servers, this will prevent external access. Consider binding to http://*:{mcpPort} or allowing the binding to be configured via standard ASP.NET Core environment variables/arguments without overriding them here.
| // Infrastructure (DbContext, Repositories, UoW) | ||
| mcpHttpBuilder.Services.AddInfrastructure(mcpHttpBuilder.Configuration); | ||
|
|
||
| // Register Application services needed by MCP resources and tools. |
There was a problem hiding this comment.
Manually instantiating BoardService with new and resolving dependencies via GetRequiredService is brittle and bypasses the benefits of the DI container. If the BoardService constructor changes in the future, this code will break. Since all dependencies are already registered in the service collection, you should register the service type directly.
mcpHttpBuilder.Services.AddScoped<Taskdeck.Application.Services.BoardService>();|
|
||
| // Revoke by name: find the first active key with this name | ||
| var keys = await _apiKeyService.ListKeysAsync(userId); | ||
| var target = keys.FirstOrDefault(k => k.Name == name && k.IsActive); |
There was a problem hiding this comment.
Revoking by name using FirstOrDefault is ambiguous if a user has multiple active keys with the same name (which the ApiKey entity doesn't seem to prevent). This could lead to revoking the wrong key. It would be safer to check for duplicates and require the user to use --id if multiple keys match, or at least document that the most recent key is revoked.
There was a problem hiding this comment.
Pull request overview
Adds the remaining MCP Phase 3 deliverables by introducing a dedicated MCP HTTP server startup mode in the API host and adding CLI api-key management commands (plus docs + tests) to support remote MCP access with API key auth.
Changes:
- Add
--mcp --transport http --port <n>dedicated MCP HTTP server mode (minimal Kestrel + MCP endpoint). - Add CLI
api-key create/list/revokecommands with new integration tests. - Update status/masterplan docs to mark MCP Phase 3 as delivered.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/STATUS.md | Updates Phase 3 status entry to include dedicated HTTP mode + CLI key management. |
| docs/IMPLEMENTATION_MASTERPLAN.md | Marks MCP Phase 3 as delivered and updates dependency chain notes. |
| backend/tests/Taskdeck.Cli.Tests/ApiKeyCommandTests.cs | Adds integration tests covering api-key create/list/revoke behavior and error handling. |
| backend/src/Taskdeck.Cli/Program.cs | Registers ApiKeyService + ApiKeysCommandHandler in CLI DI container. |
| backend/src/Taskdeck.Cli/Commands/ConsoleOutput.cs | Extends CLI usage/help text with api-key commands. |
| backend/src/Taskdeck.Cli/Commands/CommandDispatcher.cs | Routes api-key command group to the new handler. |
| backend/src/Taskdeck.Cli/Commands/ApiKeysCommandHandler.cs | Implements CLI api-key create/list/revoke command logic and JSON output. |
| backend/src/Taskdeck.Api/Program.cs | Adds MCP transport selection and dedicated HTTP MCP server branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
backend/src/Taskdeck.Api/Program.cs
Outdated
| { | ||
| if (string.Equals(args[i], "--port", StringComparison.OrdinalIgnoreCase) | ||
| && int.TryParse(args[i + 1], out var parsedPort) |
There was a problem hiding this comment.
In dedicated MCP HTTP mode the server binds to http://localhost:{port}. This prevents access from outside the host/container (e.g., Docker/Kubernetes) and contradicts the “cloud/remote” use case. Bind to 0.0.0.0/* or make the host/interface configurable (and consider supporting HTTPS behind a proxy).
| mcpHttpBuilder.Services.AddScoped<Taskdeck.Application.Services.INotificationService>( | ||
| sp => sp.GetRequiredService<Taskdeck.Application.Services.NotificationService>()); | ||
|
|
||
| // HTTP identity: maps API key to user via HttpUserContextProvider. | ||
| mcpHttpBuilder.Services.AddHttpContextAccessor(); | ||
| mcpHttpBuilder.Services.AddScoped<IUserContextProvider, Taskdeck.Infrastructure.Mcp.HttpUserContextProvider>(); | ||
|
|
||
| // MCP server: HTTP transport + all resources and tools. | ||
| mcpHttpBuilder.Services.AddMcpServer() | ||
| .WithHttpTransport() | ||
| .WithResources<BoardResources>() | ||
| .WithResources<CaptureResources>() | ||
| .WithResources<ProposalResources>() | ||
| .WithTools<ReadTools>() | ||
| .WithTools<WriteTools>() | ||
| .WithTools<ProposalTools>(); | ||
|
|
||
| var mcpHttpApp = mcpHttpBuilder.Build(); | ||
|
|
||
| // Apply EF Core migrations before starting. | ||
| using (var scope = mcpHttpApp.Services.CreateScope()) | ||
| { | ||
| var dbContext = scope.ServiceProvider.GetRequiredService<Taskdeck.Infrastructure.Persistence.TaskdeckDbContext>(); | ||
| dbContext.Database.Migrate(); | ||
| } |
There was a problem hiding this comment.
The dedicated MCP HTTP server path maps MapMcp() but does not register/use rate limiting or apply the McpPerApiKey policy (unlike the normal API pipeline). As a result, the promised per-key throttling won’t run in --mcp --transport http mode. Consider wiring the same rate limiting registration + UseRateLimiter() and requiring McpPerApiKey on the mapped MCP endpoint in this mode as well.
|
|
||
| if (transport == "http") | ||
| { | ||
| // ── MCP HTTP mode ─────────────────────────────────────────────────── | ||
| // Minimal web server exposing only the MCP endpoint with API key auth. | ||
| // No controllers, no SignalR, no Swagger, no frontend — just MCP. | ||
| var mcpPort = 5001; |
There was a problem hiding this comment.
--port parsing accepts any int (including 0/negative/out-of-range), which can lead to confusing startup failures from Kestrel. Validate the port is within 1–65535 and return a usage/error message when invalid.
| var mcpHttpBuilder = WebApplication.CreateBuilder(args); | ||
| mcpHttpBuilder.WebHost.UseUrls($"http://localhost:{mcpPort}"); | ||
|
|
||
| // Infrastructure (DbContext, Repositories, UoW) | ||
| mcpHttpBuilder.Services.AddInfrastructure(mcpHttpBuilder.Configuration); | ||
|
|
||
| // Register Application services needed by MCP resources and tools. | ||
| mcpHttpBuilder.Services.AddScoped<Taskdeck.Application.Services.AuthorizationService>(); |
There was a problem hiding this comment.
BoardService is constructed with sp.GetService<IAuthorizationService>(), which can silently pass null and disable authorization scoping if DI registration changes. Prefer GetRequiredService (fail fast) or let DI construct BoardService directly now that optional params have defaults.
docs/IMPLEMENTATION_MASTERPLAN.md
Outdated
| - tool registry, policy evaluator, and first bounded template are now delivered (`#337`): `ITaskdeckTool`/`ITaskdeckToolRegistry` domain interfaces, `AgentPolicyEvaluator` with allowlist + risk-level gating, and `InboxTriageAssistant` bounded template (proposal-only, review-first default) | ||
| - LLM tool-calling architecture spike completed (`#618`); Phase 1 delivered (`#649`): read tools + orchestrator + provider tool-calling extension; `#674` delivered (OpenAI strict mode + loop detection with error-retry bypass, PR `#694`); `#677` delivered (card ID prefix resolution for chat-to-proposal continuity, PR `#695`); `#650` delivered (write tools + proposal integration, PR `#731`); `#672` delivered (double LLM call elimination, PR `#727`); `#651` delivered (Phase 3 refinements: cost tracking, `LlmToolCalling:Enabled` feature flag, `TruncateToolResult` byte budget with binary search — 17 new tests, PR `#773`); ~~`#673`~~ delivered (argument replay — `Arguments` field on `ToolCallResult`, OpenAI/Gemini replay uses real arguments, 6 new tests, PR `#770`) | ||
| - MCP server architecture spike completed (`#619`); Phase 1 delivered (`#652`/`#664`): minimal prototype with `taskdeck://boards` resource over stdio; ~~`#653`~~ delivered (full inventory — 9 resources + 11 tools, PR `#739`); remaining: `#654` (HTTP + auth), `#655` (production hardening, deferred) | ||
| - MCP server architecture spike completed (`#619`); Phase 1 delivered (`#652`/`#664`): minimal prototype with `taskdeck://boards` resource over stdio; ~~`#653`~~ delivered (full inventory — 9 resources + 11 tools, PR `#739`); ~~`#654`~~ delivered (HTTP transport + API key auth — `ApiKey` entity, `ApiKeyMiddleware`, `HttpUserContextProvider`, `ApiKeysController`, `--mcp --transport http --port` startup mode, CLI `api-key create/list/revoke`, rate limiting, 43 tests, PRs `#792`+`#654`); remaining: `#655` (production hardening, deferred) |
There was a problem hiding this comment.
This doc line cites “PRs #792+#654”, but #654 is the issue/tracker ID for MCP Phase 3, not a pull request. Update the references to the actual PR number(s) delivering the remaining work.
docs/IMPLEMENTATION_MASTERPLAN.md
Outdated
| - `#653` Phase 2: full resource + tool inventory (2-3 weeks) | ||
| - `#654` Phase 3: HTTP transport + API key auth (1-2 weeks) | ||
| - ~~`#653` Phase 2: full resource + tool inventory~~ (delivered 2026-04-04, PR `#739`) | ||
| - ~~`#654` Phase 3: HTTP transport + API key auth~~ (delivered 2026-04-08+, PRs `#792`+`#654`) |
There was a problem hiding this comment.
This line marks Phase 3 as delivered and references “PRs #792+#654”; #654 is an issue number, not a PR. Replace with the correct PR number(s) (e.g., #792 plus this PR).
![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: 85a77a0ffa
ℹ️ 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".
backend/src/Taskdeck.Api/Program.cs
Outdated
| } | ||
|
|
||
| var mcpHttpBuilder = WebApplication.CreateBuilder(args); | ||
| mcpHttpBuilder.WebHost.UseUrls($"http://localhost:{mcpPort}"); |
There was a problem hiding this comment.
The new --mcp --transport http path hard-codes UseUrls("http://localhost:{port}"), which limits the MCP endpoint to loopback only and ignores externally provided URL bindings. In cloud/container/reverse-proxy deployments, remote MCP clients cannot connect even when the port is exposed, so the dedicated HTTP mode does not satisfy its remote-access use case.
Useful? React with 👍 / 👎.
| mcpPort = parsedPort; | ||
| } | ||
|
|
||
| var mcpHttpBuilder = WebApplication.CreateBuilder(args); |
There was a problem hiding this comment.
This branch creates a fresh WebApplication builder but never adds appsettings.local.json, unlike the stdio MCP path immediately below. Any installation that keeps local overrides there (for example ConnectionStrings:DefaultConnection) will be ignored in HTTP transport mode, causing the dedicated MCP server to start against fallback/default settings and potentially the wrong database.
Useful? React with 👍 / 👎.
Security & Correctness Review FindingsSECURITY-HIGH1. Predictable CLI actor username enables key hijacking Fix: Use a well-known deterministic GUID (derived from a namespace UUID) for the CLI system actor, so it cannot be pre-empted by user registration. Also mark the actor with a non-registerable email domain pattern. P12. MCP HTTP binds to localhost only Fix: Default to 3. MCP HTTP standalone server has no rate limiting Fix: Register rate limiting services and apply the McpPerApiKey policy to the MCP endpoint. 4. Port validation silently ignores invalid values Fix: Show an error and exit when P25. MCP HTTP mode skips appsettings.local.json Fix: Add 6. GetService can return null silently Fix: Change to 7. Revoke by name is ambiguous with duplicate names Fix: When multiple active keys match, list them and ask the user to revoke by 8. Doc reference mixes PR and issue numbers Fix: Change to "PR Adversarial Review Findings9. Rate limiting partition key falls back to IP in standalone mode Fix: The rate limiting partition resolver should also check All issues will be fixed in follow-up commits. |
SECURITY-HIGH: The CLI used GetByUsernameAsync with a predictable username (taskdeck_cli_actor) to find/create the system actor. If public registration was enabled, an attacker could register with this username first, causing the CLI to create API keys under the attacker's account. Fix: Look up the CLI actor by email instead of username. The email uses the non-routable @system.taskdeck domain, which cannot be registered through the normal authentication flow (email uniqueness check blocks it). Extract shared identity constants to CliActorIdentity to keep both BoardsCommandHandler and ApiKeysCommandHandler in sync.
- Bind to 0.0.0.0 instead of localhost to support container and remote access; add --host argument for configurability - Register rate limiting services and apply McpPerApiKey policy to the standalone MCP HTTP server endpoint - Show error and exit for invalid --port values instead of silently falling back to the default - Load appsettings.local.json in HTTP mode (matches stdio mode) - Use GetRequiredService instead of GetService for IAuthorizationService to fail fast on misconfiguration
ResolveUserOrClientIdentifier only checked JWT claims, so MCP HTTP requests authenticated via API key (stored in HttpContext.Items by ApiKeyMiddleware) fell through to IP-based partitioning. This meant multiple API keys from the same IP shared a single rate limit bucket. Now checks HttpContext.Items[McpApiKeyUserId] as a fallback before defaulting to IP, ensuring per-API-key rate limiting works in both the co-hosted and standalone MCP HTTP server modes.
When multiple active API keys share the same name, revoke --name now lists all matches with their IDs and asks the user to use --id instead of silently revoking an arbitrary key.
Rename mcpHost to mcpBindHost to avoid CS0136 conflict with the mcpHost variable in the stdio mode block (top-level statement scoping treats both as same scope). Remove unused portSpecified variable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5778aef3b7
ℹ️ 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 existingActor = await _unitOfWork.Users.GetByEmailAsync(CliActorIdentity.ActorEmail); | ||
| if (existingActor is not null) |
There was a problem hiding this comment.
Looking up the CLI actor only by CliActorIdentity.ActorEmail breaks upgrades for databases that already contain the older actor (taskdeck_cli_actor with taskdeck-cli-actor@local.taskdeck). In that case this lookup returns null, then AddAsync tries to insert the same username again and hits the unique username index, causing boards create (and the same pattern in ApiKeysCommandHandler) to fail with a database exception instead of working. Add a backward-compatible fallback (e.g., lookup by legacy username/email and migrate the record) before creating a new actor.
Useful? React with 👍 / 👎.
backend/src/Taskdeck.Api/Program.cs
Outdated
| Console.Error.WriteLine($"Error: unknown transport '{transport}'. Supported values: stdio, http"); | ||
| return; |
There was a problem hiding this comment.
The new argument validation paths print an error and return, which exits Taskdeck.Api with status code 0 in top-level statements. When --transport is invalid (and similarly for invalid --port), automation will treat startup as successful even though the server never started, masking deployment/configuration failures. Set a non-zero exit code (or throw) on validation errors so callers can reliably detect startup failure.
Useful? React with 👍 / 👎.
…ew comments - Resolve merge conflicts in docs/STATUS.md and docs/IMPLEMENTATION_MASTERPLAN.md by taking main's version (MCP feature already documented in main) - Return non-zero exit code (return 1) for invalid MCP startup arguments (unknown transport, invalid port) to enable proper error detection - Return 0 from all successful exit paths for consistency with int return type
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e78eb583
ℹ️ 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".
| mcpHttpApp.UseMiddleware<Taskdeck.Api.Middleware.ApiKeyMiddleware>(); | ||
|
|
||
| // Apply rate limiting before endpoint routing. |
There was a problem hiding this comment.
In the new --mcp --transport http startup path, ApiKeyMiddleware runs before UseRateLimiter, and that middleware returns 401 immediately for missing/invalid keys. Because those rejected requests never continue down the pipeline, the McpPerApiKey limiter on MapMcp() is bypassed for unauthenticated traffic, so brute-force/abuse attempts are effectively unthrottled in this mode.
Useful? React with 👍 / 👎.
| $"Invalid --expires value: '{expiresText}'. Provide a positive number of days (e.g., 90 or 90d).", | ||
| "taskdeck api-key create --name <name> [--expires <days>]"); | ||
| } | ||
| expiresIn = TimeSpan.FromDays(days); |
There was a problem hiding this comment.
--expires is only checked for int > 0, then passed directly to TimeSpan.FromDays(days). Large values (above TimeSpan.MaxValue.TotalDays, e.g. millions of days) throw OverflowException, and this path is not caught by the current DomainException handler, so taskdeck api-key create can crash instead of returning a usage error.
Useful? React with 👍 / 👎.
Update STATUS.md with post-merge housekeeping entry, recertified test counts (4279 backend + 2245 frontend = ~6500+), and delivered status for distributed caching, SSO/OIDC/MFA, and staged rollout. Update TESTING_GUIDE.md with current test counts and new test categories (resilience, MFA/OIDC, telemetry, cache). Update IMPLEMENTATION_MASTERPLAN.md marking all expansion wave items as delivered. Extend AUTHENTICATION.md with OIDC/SSO login flow, MFA setup/verify/ recovery, API key management, and account linking endpoints. Update MANUAL_TEST_CHECKLIST.md: mark all PRs as merged, add testing tasks for error tracking (#811), MCP HTTP transport (#819), distributed caching (#805), and resilience tests (#820).
Summary
--mcp --transport http --port 5001startup option for dedicated MCP HTTP server mode (minimal Kestrel with API key middleware, no JWT/CORS/SignalR/frontend overhead)api-key create/list/revokecommands for key management from the command lineNote: The core infrastructure (ApiKey entity, ApiKeyMiddleware, HttpUserContextProvider, ApiKeysController, MapMcp(), rate limiting) was delivered in PR #792. This PR adds the remaining two deliverables from the spike: the
--transport httpstartup mode and CLI api-key management.Closes #654
Test plan