Enforce MCP per-key budget before auth-stage DB work (one budget, checked once)

[Chris0Jeky]

Summary

Closes #1384.

The MCP per-key rate limit (McpPerApiKey, 60/min per key) was an endpoint-stage policy that ran after ApiKeyMiddleware had already completed its authentication-stage database work. So a valid but over-quota key still paid, on every request before the 429: the SHA-256 hash, the ApiKeys lookup, the Users lookup, and the UpdateLastUsedAsync write. The advertised per-key quota bounded MCP endpoint work but not auth-stage database work — the only remaining bound on that path was the per-address pre-auth concurrency gate (parallelism capped, sequential throughput not).

The one-budget design (checked once, at the earliest point)

The per-key budget is now enforced inside ApiKeyMiddleware, immediately after the ApiKeys row is resolved and confirmed active and before the Users lookup and the UpdateLastUsedAsync write:

• New shared singleton McpPerApiKeyRateLimiter — a fixed-window PartitionedRateLimiter<HttpContext> on the same mcp-apikey:{keyId} partition and the same McpPerApiKey settings as the removed endpoint policy, emitting the identical 429 contract (429, Retry-After, X-RateLimit-Policy: McpPerApiKey, application/json ApiErrorResponse).
• ApiKeyMiddleware stores the key-id item, then charges exactly once; on rejection it writes the 429 and returns before the user lookup / usage write.
• The endpoint-stage RequireRateLimiting(McpPerApiKey) (and the now-dead policy + partition helper) are removed, so a request that passes the early check is never charged again — one budget, checked once, no double-charge.
• Registered in the shared AddTaskdeckRateLimiting (gated on Enabled), which both the co-hosted API and the standalone MCP host call — so both pipelines get identical enforcement (Fail closed on separator-only AllowedHosts in standalone MCP host security #1372 mirroring discipline). ApiKeyMiddleware resolves it optionally, so "rate limiting disabled = no MCP throttling" is preserved.

Pre-auth IP budget is not regressed (#1368/#1381)

A per-key rejection returns 429 and does not set AuthenticationFailedItemKey, so McpAuthenticationRateLimitingMiddleware does not spend the pre-auth IP failure budget for it. Valid keys still never touch the IP budget; only 401s do. The per-address concurrency admission gate and the abort-proof failure consumption are untouched.

Both pipelines covered

• Co-hosted: PipelineConfiguration + the McpHttpTransportApiKeyTests integration suite (full TestWebApplicationFactory pipeline).
• Standalone: Program.cs --mcp --transport http host + a new real-host e2e that seeds a valid key into the host DB and drives the per-key 429.

Tests

• ApiKeyMiddlewarePerKeyRateLimitTests (unit, real SQLite + command interceptor):
  • Over-quota valid key → 429 before the Users SELECT and before the ApiKeys UPDATE (asserted at the SQL boundary); no AuthenticationFailedItemKey; full 429 contract.
  • Under-quota valid key passes through and reaches the user lookup the over-quota path skips.
  • One budget: exactly PermitLimit (3) requests admitted, the next rejected — no double-charge.
• McpHttpTransportApiKeyTests.McpEndpoint_PerKeyBudget_AllowsExactlyPermitLimitRequests_ThenRejects — end-to-end boundary count through the real co-hosted pipeline (3 succeed, 4th 429 with the McpPerApiKey contract). The existing partition/isolation test still passes unchanged.
• StandaloneMcpHostFilteringTests.StandaloneMcpHttpHost_PerKeyBudget_RejectsOverQuotaValidKey — real standalone host, seeded key, per-key 429 with McpPerApiKey header.

Verification (exact counts)

• dotnet build backend/src/Taskdeck.Api/... — succeeded, 0 warnings.
• --filter ApiKeyMiddlewarePerKeyRateLimitTests — 3/3 passed.
• --filter McpHttpTransportApiKeyTests — 49/49 passed.
• --filter StandaloneMcpHostFilteringTests|McpAuthenticationRateLimitingMiddlewareTests|FallbackPolicyTests — 19/19 passed.
• Full Taskdeck.Api.Tests project — 2072 passed, 0 failed (4m49s).

Out-of-scope finding (not fixed here; tracked separately)

While proving the over-quota path skips the usage write, the command interceptor showed UpdateLastUsedAsync never issues its UPDATE: SetProperty(k => k.LastUsedAt /* DateTimeOffset? */, DateTimeOffset.UtcNow /* non-nullable */) fails EF Core's SQLite translation ((DateTimeOffset?)DateTimeOffset.UtcNow is not a valid SetProperty value expression), and the exception is swallowed by the method's non-critical try/catch. So ApiKey.LastUsedAt is effectively never persisted today. This is pre-existing and unrelated to the #1384 rate-limit fix; filed as a separate issue.

Intentional contract change (recorded per round-2 review, F3)

An ACTIVE key owned by an INACTIVE user now spends one per-key permit before its 401. On main, such a request died in ApiKeyMiddleware before the endpoint policy ever ran, so only the pre-auth IP failure budget was spent for it. The new behavior is stricter and consistent with this PR's goal (charge at the earliest point the key ID is known, bounding the Users lookup). The user-inactive 401 still sets AuthenticationFailedItemKey, so the IP failure budget is also still charged for it exactly as before.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gemini-code-assist]

Code Review

This pull request refactors the MCP per-API-key rate limiting by moving the enforcement of the McpPerApiKey budget from an endpoint-stage policy into ApiKeyMiddleware via a new McpPerApiKeyRateLimiter class. This optimization shields the database from unnecessary user lookups and last-used writes when a valid key is over quota. The changes also include comprehensive tests to verify this behavior in both co-hosted and standalone environments. The review feedback suggests using null-conditional operators when accessing context.RequestServices and context.Connection to prevent potential NullReferenceExceptions in testing or mock environments.

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.

[Chris0Jeky]

Adversarial Code Review

Reviewed against: double-charge/one-budget correctness, 429 contract parity, pre-auth IP budget non-regression, DI lifetime, partition-key equivalence, both-pipeline coverage.

CRITICAL

• None.

HIGH

• None. Verified: the endpoint-stage McpPerApiKey policy and its RequireRateLimiting are fully removed (no remaining refs), so the budget is charged exactly once in ApiKeyMiddleware — no double-charge. A per-key rejection returns 429 (not 401) and does not set AuthenticationFailedItemKey, so McpAuthenticationRateLimitingMiddleware never spends the pre-auth IP failure budget on it (MCP /mcp pre-auth IP rate limiter throttles valid traffic and defeats per-key isolation (post-merge finding from #1364) #1368/Spend MCP pre-auth IP budget only on authentication failures #1381 preserved). The pre-auth concurrency lease is released by its own finally even when the middleware returns early. Partition key mcp-apikey:{keyId} and the 429 contract are byte-identical to the removed policy.

MEDIUM

• M1 — ApiKeyMiddleware.cs: context.RequestServices.GetService<...>() can NRE if RequestServices is unset (bot: gemini). Real pipelines always set it, but hardening with ?. makes the middleware robust in custom-host/test contexts (skips the check safely). Will fix.
• M2 — McpPerApiKeyRateLimiter.cs: context.Connection.RemoteIpAddress fallback can NRE if Connection is null (bot: gemini). Defensive ?. on Connection. Will fix.
• M3 — Test gap: no coverage that per-key enforcement is skipped when rate limiting is disabled (limiter singleton not registered → optional resolution returns null). Adding a unit test.

LOW

• L1 — API shape: AttemptAcquire is an instance method while WriteRejectedAsync is static. Intentional (the writer needs no limiter state), documented; leaving as-is.
• L2 — By-design note (not a defect): the per-key charge now precedes the user-account active check, so a valid+active key owned by a deactivated user consumes one per-key permit before its 401. This is exactly the issue's directive ("charge before the Users lookup") and is desirable — it bounds that lookup. Called out for the record.

Bot Comments Addressed

• gemini-code-assist M1 (RequestServices null-safety) and M2 (Connection null-safety) — both accepted and fixed below.

Summary

0 CRITICAL, 0 HIGH, 3 MEDIUM (2 bot null-safety + 1 test gap), 2 LOW (1 style kept, 1 by-design note). Not merge-blocking on logic; fixing all MEDIUM findings before handoff per zero-skip policy. CI is green.

[Chris0Jeky]

Adversarial Review — Fixes Applied

Finding	Severity	Fix Commit	Verified
M1 — RequestServices.GetService NRE-safety in ApiKeyMiddleware (gemini)	MEDIUM	8db01ae6	build 0 warnings; per-key unit tests 4/4
M2 — Connection.RemoteIpAddress fallback NRE-safety in McpPerApiKeyRateLimiter (gemini)	MEDIUM	8db01ae6	build 0 warnings; MCP classes 54/54
M3 — Test gap: per-key check skipped when rate limiting disabled	MEDIUM	8db01ae6	new RateLimitingDisabled_LimiterAbsent_SkipsPerKeyCheck_AndPassesThrough passes
L1 — AttemptAcquire instance vs WriteRejectedAsync static	LOW	n/a	intentional (writer is stateless); kept + documented
L2 — per-key charge precedes user-active check	LOW	n/a	by design per #1384 ("charge before the Users lookup"); no change

Both gemini-code-assist bot findings accepted and fixed. Verification after fixes:

• dotnet build backend/src/Taskdeck.Api — succeeded, 0 warnings.
• --filter ApiKeyMiddlewarePerKeyRateLimitTests — 4/4 passed.
• --filter McpHttpTransportApiKeyTests|StandaloneMcpHostFilteringTests — 54/54 passed.
• (pre-fix) full Taskdeck.Api.Tests — 2072 passed, 0 failed.

Out-of-scope finding discovered during review filed as #1402 (UpdateLastUsedAsync never persists LastUsedAt — EF SQLite SetProperty translation fails and is swallowed).

All findings addressed. CI status: previous run GREEN; re-running on the fix push.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8844bd1a0d

ℹ️ 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]

Adversarial Review — Round 2 (consolidated, two independent lenses)

Two independent reviews were run against this PR: a security/resource-exhaustion lens and a pipeline-parity/regression lens. Verdict: zero substantive findings. Attacks attempted and refuted with code-path evidence:

• Budget bypass: no path reaches the MCP endpoint with a validated key that skips the McpPerApiKeyRateLimiter charge (single charge point in ApiKeyMiddleware, endpoint carries no rate-limiting metadata in either pipeline).
• Order of work: the charge occurs after the ApiKeys row is resolved+active and provably before the Users SELECT and the ApiKeys UPDATE (asserted at the SQL boundary by interceptor tests).
• Spend MCP pre-auth IP budget only on authentication failures #1381 preservation: a per-key 429 sets neither a 401 nor AuthenticationFailedItemKey, so the pre-auth IP failure budget is never spent for valid keys; the abort-proof finally-block consumption and the per-IP concurrency gate are untouched.
• Double/under-charge: exactly PermitLimit requests are admitted per window end-to-end (boundary-count tests in both the unit and integration suites); no second charge exists because the endpoint policy is fully removed (no RequireRateLimiting, no policy registration, no partition-helper residue).
• 429-contract identity: status, Retry-After derivation (lease metadata, min 1s), X-RateLimit-Policy: McpPerApiKey, JSON error body, and the HasStarted guard byte-match the removed endpoint policy's OnRejected.
• Pipeline parity: co-hosted and standalone both get the limiter via the shared AddTaskdeckRateLimiting and the shared ApiKeyMiddleware; the standalone host's dead UseRateLimiter() was removed with justification; parity proven by a real-host e2e test (Fail closed on separator-only AllowedHosts in standalone MCP host security #1372 discipline).
• Singleton hygiene / policy residue / test honesty: all verified clean.

Adjudicated dispositions (this round's fixes)

• F1 (LOW, fail-open smell) — FIX: ApiKeyMiddleware used context.RequestServices?.GetService<...>(); the ?. (added for a round-1 bot nit) creates a second silent-skip path — a genuinely missing provider would silently disable a security charge (fail-open). ASP.NET Core guarantees RequestServices is populated before app middleware runs, so the ?. is removed: a missing provider now throws, while the intended "limiter not registered when rate limiting is disabled" skip remains via GetService returning null.
• F2 (LOW, disposal) — FIX: AddSingleton(new McpPerApiKeyRateLimiter(...)) registers a pre-built instance, which the container never disposes (the fixed-window replenishment timer lives to process exit; in test hosts, one leaked timer per WebApplicationFactory). Switched to factory registration so the container owns disposal. The sibling McpAuthenticationAttemptLimiter registration had the identical pre-existing pattern and gets the identical one-line treatment in the same commit.
• F3 (INFO, record only — intentional contract change): an ACTIVE key owned by an INACTIVE user now spends one per-key permit before its 401. On main, that request died in ApiKeyMiddleware before the endpoint policy, so only the IP failure budget was ever spent for it. The new behavior is stricter and consistent with MCP per-key rate limit does not bound auth-stage database work for valid over-quota keys #1384's goal (bound all auth-stage work behind the earliest possible check); the user-inactive 401 still sets AuthenticationFailedItemKey, so the IP failure budget is also still charged for it exactly as before. Recorded here as a deliberate divergence, no code change.

Fix evidence follows in the next comment after the batched push.

[Chris0Jeky]

Adversarial Review Round 2 — Fixes Applied

Finding	Severity	Fix Commit	Verified
F1 — fail-open smell: ?. on RequestServices created a second silent-skip path for the per-key charge	LOW	0df118cb	removed the null-conditional (framework guarantees the provider before app middleware; missing provider now throws loudly; the intended disabled-path skip via GetService returning null is unchanged and covered by RateLimitingDisabled_LimiterAbsent_SkipsPerKeyCheck_AndPassesThrough)
F2 — instance singletons never disposed by the container (leaked replenishment timer per test host)	LOW	0df118cb	McpPerApiKeyRateLimiter AND the sibling McpAuthenticationAttemptLimiter both switched to factory registration (AddSingleton(_ => new ...)) in the same commit — the identical one-line treatment was safe. Consequence: construction is now lazy (first resolution, not registration); the standalone host's fail-fast validation still runs strictly before AddTaskdeckRateLimiting, so the ordering guarantee is unchanged — the stale "instantiates it eagerly at registration" comment in Program.cs was corrected, and StandaloneMcpHttpHost_OutOfRangeConcurrency_FailsFastWithValidationMessage still passes (in the 5/5 class run below)
F3 — active key / inactive user now spends a per-key permit before its 401	INFO	n/a (record only)	documented in the round-2 review comment AND appended to the PR body under "Intentional contract change (recorded per round-2 review, F3)"

Verification after fixes (exact counts):

• dotnet build backend/Taskdeck.sln -c Release — succeeded, 0 errors (11 pre-existing CS8603 test warnings only).
• --filter ApiKeyMiddlewarePerKeyRateLimitTests — 4/4 passed.
• --filter McpHttpTransportApiKeyTests — 49/49 passed.
• --filter StandaloneMcpHostFilteringTests — 5/5 passed.
• Full Taskdeck.Api.Tests project — 2073 passed, 0 failed (6m47s).

All round-2 findings addressed in one batched push (0df118cb).