Spend MCP pre-auth IP budget only on authentication failures

[Chris0Jeky]

Closes #1368

Problem

McpAuthenticationRateLimitingMiddleware acquired an IP-scoped lease on every /mcp request — including fully authenticated ones. The pre-auth IP window default is 120/60s per client address while each key's own budget is 60/min, so multiple keys behind one egress IP (NAT/proxy/CI) shared the 120/min bucket and 429d each other before their independent per-key budgets applied — defeating the per-key isolation #1364 built. Compounding it, the standalone MCP host never wired UseForwardedHeaders, so behind a reverse proxy every client collapsed to the proxy address → one bucket for the world.

Fix

The pre-auth IP budget is now a FAILURE budget with a concurrency gate (McpAuthenticationAttemptLimiter + middleware):

1. Fast pre-check — if the client IP's failure budget is already exhausted, reject 429 before any API-key lookup (preserves the brute-force/lookup-cost protection). Uses non-consuming PartitionedRateLimiter.GetStatistics(...).CurrentAvailablePermits <= 0 inspection.
2. Concurrency gate (admission control) — the pre-check alone cannot bound concurrent pre-auth work (its consumption is post-response), so each request must also take a per-address concurrency slot (ConcurrencyLimiter, releasable leases, QueueLimit 0 → immediate 429) held for the request's duration. New setting RateLimiting:McpAuthenticationPerIpConcurrency (default 16, validated 1–10000).
3. Consume on failure only — exactly one window permit is spent (AttemptAcquire(1), no refund — window permits don't refund) only when authentication fails (401). ApiKeyMiddleware is the sole 401 source on /mcp, so a 401 unambiguously marks a failed attempt.

Successful authentications never spend IP budget → per-key isolation holds for any number of keys behind one address. Combined invariant: in-flight pre-auth work per address ≤ concurrency cap at any instant; once the failure window (120) is spent the pre-check rejects everything → failed-auth key lookups per address per window ≤ PermitLimit + concurrency cap. The existing 429 contract (application/json ApiErrorResponse, TooManyRequests, Retry-After, X-RateLimit-Policy) is unchanged; the per-key limiter (60/min, opaque key-ID partitioning from #1364) is untouched.

Retry-After note: the failure-budget pre-check 429 reports the FULL window as Retry-After — a deliberate safe over-estimate (the non-consuming inspection does not expose the exact replenishment instant). The concurrency-gate 429 reports Retry-After: 1 (a slot frees as soon as any in-flight request completes).

Forwarded headers: config-gated, default OFF. Reuses the existing ForwardedHeaders:KnownProxies/KnownNetworks convention (the co-hosted API already wires it before the limiter); this PR adds the same UseForwardedHeaders wiring to the standalone MCP host before the limiter. X-Forwarded-For is never trusted from an unknown peer, so a spoofed XFF cannot rotate buckets (pinned e2e). (Deviation from the suggested RateLimiting:TrustForwardedHeaders bool: reusing the established ForwardedHeaders section is the smaller, consistent change and is strictly more secure than a bare bool, which would trust all XFF. Reversible.)

Residual DoS tradeoff (intentional, documented): the pre-check runs before the auth outcome is known, so a valid key sharing a hostile NAT that already drained the budget still pays the 429 until the window resets — the accepted cost of shielding the key lookup from a brute-force flood. Similarly, a valid client multiplexing more than the concurrency cap of simultaneous requests through one address gets brief 429s. Operators behind a trusted proxy set ForwardedHeaders to give each real client its own bucket.

Test evidence (Release, -m:1)

Unit — McpAuthenticationRateLimitingMiddlewareTests (7 passed):

• exhausted budget rejects 429 before the auth/lookup layer (counting fake proves _next not reached) + full 429 contract
• successful requests never decrement the budget (25 successes at budget=1)
• failures consume but interleaved successes do not (budget=2, 10 successes between failures, 3rd failure 429s)
• window replenishment through the pre-check (1s window: exhaust → 429 → wait → admitted again; production 60s differs only in duration)
• concurrency bound pinned: cap=2 with a blocking auth fake — at most 2 reach the auth layer simultaneously, 3 over-cap requests 429 immediately, released slots re-admit (stable 3× consecutive runs)
• forwarded-headers OFF → XFF ignored, keyed on socket address
• forwarded-headers ON with a known proxy → keyed on the forwarded client (independent buckets per client)

Integration — McpHttpTransportApiKeyTests (48 passed): two keys behind one address with the pre-auth IP budget at its production default (120/60s — the setting under test; McpPerApiKey deliberately raised to 500 so 130 valid requests exceed the 120-permit bucket) sustain traffic without starving each other; existing ...RejectedCredentials_AreRateLimitedBeforeDatabaseLookup and ...RateLimit_IsPartitionedByApiKey still green.

Standalone e2e — StandaloneMcpHostFilteringTests (3 passed): real --mcp --transport http host — forwarded-client keying with a known proxy, XFF-spoof resistance from an unknown peer while forwarding is enabled, and the #1367 host-filtering regression.

Options validation — OptionsValidationTests (93 passed, incl. range coverage for the new concurrency setting).

• Full solution build: 0 errors.
• dotnet ef migrations has-pending-model-changes: No changes (no model change).

Docs

docs/platform/CONFIGURATION_REFERENCE.md — McpAuthenticationPerIp failure-budget semantics, the new McpAuthenticationPerIpConcurrency cap, the plain Retry-After over-estimate statement, and ForwardedHeaders coverage of the standalone MCP host with the never-trust-XFF-by-default caveat.

[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 pre-authentication rate limiter to implement a "failure budget" model, where rate limit permits are only consumed upon authentication failure (401 Unauthorized). This change prevents multiple valid clients sharing a single egress IP address from starving each other's rate limits. Additionally, the PR integrates forwarded headers support for the standalone MCP host and adds comprehensive tests and documentation updates. The feedback suggests chaining .Dispose() directly on the result of AttemptAcquire to avoid potential compiler warnings regarding the unused lease variable.

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

Self-review of this PR (author). No prior human/bot comments on the thread yet.

CRITICAL

• None.

HIGH

• None. The "spend on 401" failure signal is sound: on /mcp, ApiKeyMiddleware is the only 401 source — a valid key sets an authenticated principal and every downstream layer (per-key limiter → 429, MCP endpoint → 200/202/400/406, authorization → pass) returns non-401 — so a 401 unambiguously marks a failed attempt. Verified against the co-hosted middleware order (ApiKeyMiddleware → UseAuthentication → TokenValidationMiddleware → UseRateLimiter → UseAuthorization) and the standalone order.

MEDIUM

• M1 (test gap): the standalone MCP host (--mcp --transport http) UseForwardedHeaders wiring + reworked failure budget have no end-to-end coverage. TestWebApplicationFactory boots the co-hosted Program path, not the standalone host, so the new 4-line wiring and the 429/keying behavior over real Kestrel are unverified. The unit tests cover limiter keying generically and the co-hosted path is exercised, but the standalone wiring is inspection-only. Fixing by adding an e2e test to the existing StandaloneMcpHostFilteringTests harness (tiny budget + X-Forwarded-For keying via a known loopback proxy).

LOW

• L1 (behavior change, documented): the pre-check 429 now sets Retry-After to the full window length rather than the exact remaining time. The availability inspection (GetStatistics) does not expose the replenishment instant; the full window is a safe upper bound. The existing contract test only asserts the header is present. Keeping as-is.
• L2 (coupling, documented in code): the failure signal depends on the "sole 401 source on /mcp" invariant. A future middleware that returned 401 for an already-authenticated /mcp request would over-charge the failure budget. Documented in the middleware comment; the invariant holds today and the "401 path" is the intended design.

Bot Comments Addressed

• None present yet (fresh push; a Codex round typically follows).

Summary

0 CRITICAL, 0 HIGH, 1 MEDIUM, 2 LOW. Not merge-blocking on correctness. Fixing M1 (add standalone-host e2e coverage); L1/L2 are documented, accepted design characteristics. Fixes and evidence in a follow-up comment.

[Chris0Jeky]

Adversarial Review — Fixes Applied

Finding	Severity	Fix Commit	Verified
M1 — standalone MCP host UseForwardedHeaders wiring + failure budget had no e2e coverage	MEDIUM	c3c9d6e8	New StandaloneMcpHostFilteringTests.StandaloneMcpHttpHost_ForwardedHeaders_KeyFailureBudgetOnForwardedClient boots the real --mcp --transport http host and proves the budget keys on the X-Forwarded-For client (independent buckets per client, not the shared proxy socket). Passed 3× consecutively (deterministic via single-connection request serialization).
Gemini bot — unused using var lease in RecordFailedAttempt	MEDIUM (bot)	9707322e	Chained .Dispose() on the AttemptAcquire result. Rebuild 0 warnings/0 errors; middleware unit + transport + standalone tests green.
L1 — pre-check Retry-After is the full window (upper bound)	LOW	—	Accepted/documented: GetStatistics does not expose the replenishment instant; full window is a safe upper bound, and the contract test only asserts presence.
L2 — failure signal couples to "sole 401 source on /mcp"	LOW	—	Accepted/documented in the middleware comment; the invariant holds today and the "401 path" is the intended design.
Copilot review	—	—	Skipped by the bot (requester quota limit); nothing to address.

Test evidence (Release, -m:1, final head 9707322e)

• McpAuthenticationRateLimitingMiddlewareTests — 5 passed
• McpHttpTransportApiKeyTests — 48 passed (incl. new production-default 120/60s multi-key isolation test)
• StandaloneMcpHostFilteringTests — 2 passed (incl. new MCP /mcp pre-auth IP rate limiter throttles valid traffic and defeats per-key isolation (post-merge finding from #1364) #1368 forwarded-headers e2e)
• OptionsValidationTests — 90 passed
• Full solution build: 0 errors. dotnet ef migrations has-pending-model-changes: No changes.

All findings at every severity addressed (fixed or documented-and-accepted). CI status: pending (CodeQL/Gitleaks/guardrails running); I will not merge (weak-model rails — the gate decides).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5300ff143f

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

Consolidated Adversarial Review — Two Independent Lenses (Coordinator Adjudication)

Two independent adversarial reviews (security lens + test lens) of this PR completed and were adjudicated. Consolidated findings below; fixes follow in one batch.

Confirmed solid (traced, no action)

• 401-source completeness: on /mcp, ApiKeyMiddleware is the sole 401 source — a valid key sets an authenticated principal; the per-key limiter returns 429, the MCP endpoint 200/202/400/406, authorization passes on the principal. A 401 therefore unambiguously marks a failed authentication attempt. Verified on both the co-hosted and standalone middleware orders.
• Forwarded-headers semantics: default OFF; X-Forwarded-For honored only from ForwardedHeaders:KnownProxies/KnownNetworks; wired before the limiter in both pipelines; spoofed XFF cannot rotate buckets.
• Thread-safety: PartitionedRateLimiter partition creation and AttemptAcquire/GetStatistics are safe under concurrency; no shared mutable state added in the middleware.
• DI lifetime: McpAuthenticationAttemptLimiter remains a singleton owning its partitioned limiter; middleware resolves it per-request without capturing scoped state.
• Per-key isolation: unchanged 60/min opaque-key-ID partitioning; production-default IP-budget test proves two keys behind one address are not starved.

MEDIUM

• M2 (security): the pre-check is not admission control. Failure-budget consumption happens after await _next on the 401 path, so N concurrent invalid-key requests can all pass the non-consuming pre-check while permits are > 0 and all reach the API-key database lookup. The old acquire-on-entry design atomically hard-capped pre-lookup work at 120/window; the new design's instantaneous bound is attacker-chosen concurrency. Fix (adjudicated design): add a per-IP pre-auth concurrency limiter alongside the failure window — .NET ConcurrencyLimiter leases are releasable, unlike window permits. Partitioned per client address, new setting RateLimiting:McpAuthenticationPerIpConcurrency (default 16, QueueLimit 0 → immediate 429), acquired at /mcp pre-auth entry, released when the request completes. Combined invariant: in-flight pre-auth work per IP ≤ concurrency cap at any instant; once the failure window (120) is spent the pre-check rejects everything → total failed-auth lookups per window ≤ PermitLimit + concurrency cap. Valid multi-key NAT clients get 16 concurrent in-flight requests per address; brief 429s beyond that are the documented tradeoff. Failure-budget semantics unchanged. The previous "bounded overshoot" docstring undersold the unbounded-concurrency case and will be rewritten honestly.

LOW

• L3 (security): pre-check 429 reports the full window as Retry-After (the base path used lease metadata for exact remaining time). Accepted as the documented safe over-estimate — the availability inspection does not expose the replenishment instant. Docstring and CONFIGURATION_REFERENCE.md to state it plainly; no code change.
• L4 (security, doc nit): the comment claiming GetStatistics may return "null before creation" is wrong — it lazily creates the partition and returns full-permit statistics. Comment to be corrected; the null-safe branch stays as explicitly-labeled defensive coding.
• L5 (test): spoof resistance is not pinned on the real MCP path with forwarding ENABLED. Add a standalone e2e case sending XFF from an unknown peer while forwarded headers are enabled → assert the socket-address bucket is used. Also resolves the OFF-default-tautology critique by exercising the real Program wiring.
• L6 (test): no window-replenishment coverage through the pre-check path. Add a short-window test (exhaust → 429 → wait past window → admitted again), noting what remains untested at the production 60s window.
• L7 (test): the TOCTOU claim was untestable. With the new concurrency limiter, add a test holding M > cap concurrent invalid-key requests against a blocking auth fake, asserting (a) at most cap reach the auth layer simultaneously and (b) the excess 429 immediately — converting the claim into a pinned bound.
• L8 (precision): the PR body/test comment says the two-key test runs at "production defaults" — it raises McpPerApiKey to 500; only the IP budget (120/60s, the load-bearing setting, genuinely exceeded at 130 requests) is at its default. Wording to be corrected in the test comment and PR body.

Summary

0 CRITICAL, 1 MEDIUM (M2 — concurrency admission control), 6 LOW. M2 is the only merge-relevant gap; all seven items will be fixed or explicitly documented in the next push, with fix-evidence mapped per finding.

[Chris0Jeky]

Consolidated Review — Fixes Applied (head ed5e7434)

Finding	Severity	Fix Commit	Verified
M2 — pre-check is not admission control: N concurrent invalid-key requests could all pass while permits remained (bound = attacker-chosen concurrency)	MEDIUM	1cfebb66	Per-address ConcurrencyLimiter gate (releasable leases, QueueLimit 0 → immediate 429), new setting RateLimiting:McpAuthenticationPerIpConcurrency (default 16, validated 1–10000), acquired at /mcp pre-auth entry, released on request completion. Combined invariant documented in code and docs: in-flight pre-auth work per IP ≤ cap at any instant; failed-auth lookups per window ≤ PermitLimit + cap. The "bounded overshoot" docstring rewritten honestly (previously undersold the unbounded-concurrency case). Pinned by the new concurrency test (below).
L3 — pre-check 429 reports the full window as Retry-After	LOW	ed5e7434 (docs), 1cfebb66 (docstring)	Accepted as the documented safe over-estimate — no behavior change. The non-consuming inspection does not expose the replenishment instant. Now stated plainly in the WriteFailureBudgetRejectedAsync docstring, CONFIGURATION_REFERENCE.md, and the PR body. The concurrency-gate 429 uses Retry-After: 1 (slots free as soon as any in-flight request completes).
L4 — comment wrongly claimed GetStatistics may return "null before creation"	LOW	1cfebb66	Comment corrected: GetStatistics lazily creates the partition and reports full permits for a first-time address; the null branch is retained and explicitly labeled as defensive coding.
L5 — spoof resistance not pinned on the real MCP path with forwarding ENABLED	LOW	0b27b580	New standalone e2e StandaloneMcpHttpHost_ForwardedHeaders_IgnoreXffFromUnknownPeer: forwarding enabled, connecting peer NOT in KnownProxies → XFF ignored, budget keys on the socket address, rotated XFF 429s on the same bucket. Host-boot boilerplate extracted to a shared helper. Also resolves the OFF-default tautology critique (the middleware is in the pipeline and actively refuses the untrusted header).
L6 — no window-replenishment coverage through the pre-check	LOW	301ef362	New unit test FailureWindowReplenishment_AdmitsRequestsAgain_ThroughPreCheck (1s window: exhaust → 429 → wait → admitted, auth layer reached again). Test comment notes the production 60s window differs only in duration — the pre-check logic is window-length-agnostic.
L7 — TOCTOU claim previously untestable	LOW	301ef362	New unit test ConcurrencyGate_CapsInFlightPreAuthWork_AndRejectsExcessImmediately: cap=2, blocking auth fake, 3 over-cap requests 429 immediately without reaching auth (counter pinned at 2), released slots re-admit. Stable 3× consecutive runs.
L8 — "production defaults" claim imprecise	LOW	6dd3a4cf + PR body edit	Test comment and PR body now state precisely: only the pre-auth IP budget (120/60s — the setting under test, genuinely exceeded at 130 requests) is at its production default; McpPerApiKey is deliberately raised to 500.

Verification (Release, -m:1, head ed5e7434)

• Full solution build: 0 errors
• McpAuthenticationRateLimitingMiddlewareTests — 7 passed (was 5; +replenishment, +concurrency bound)
• McpHttpTransportApiKeyTests — 48 passed
• StandaloneMcpHostFilteringTests — 3 passed (was 2; +unknown-peer spoof resistance)
• OptionsValidationTests — 93 passed (was 90; +3 concurrency range cases)
• New concurrency test: 3× consecutive passes
• No EF model change in this batch (settings class only; has-pending-model-changes previously verified "No changes")

All adjudicated findings addressed — fixed, pinned by tests, or documented-and-accepted with rationale. CI: pending on the new push; will not merge (gate decides).

[Chris0Jeky]

Addendum — Codex review round (post-push)

Two P2 threads from chatgpt-codex-connector on head ed5e7434, both addressed:

Bot finding	Disposition
Cap concurrent failed MCP auth before lookup	Already fixed in this PR (1cfebb66, the adjudicated M2 fix): per-address ConcurrencyLimiter gate acquired before _next, default cap 16, QueueLimit 0. Failed-auth lookups per window ≤ PermitLimit + cap. Replied on the thread with the pinning test.
Throttle valid over-quota keys before auth-stage DB work	Real, out of scope for this PR (per-key limiter ordering vs ApiKeyMiddleware predates it and is a different seam). Tracked as #1384 with the fix direction; replied on the thread.

No code changes required beyond what is already pushed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed5e74349d

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

Fix Evidence — Final Cycle (head 5db6c6dc)

Updates the earlier evidence tables with the final Codex round. All review threads on this PR are now replied to and resolved (0 unresolved).

This cycle

Finding	Severity	Fix Commit	Verified
Codex — [Range] on McpAuthenticationPerIpConcurrency validated even with RateLimiting:Enabled=false (disabled config could fail startup for a limiter that is never constructed)	P2	d55ed6cb	Annotation removed; range (1–10000) enforced solely by RateLimitingSettingsValidator, which skips when disabled — same semantics as the nested policy settings. New boot-level test App_StartsSuccessfully_WhenRateLimitingDisabled_WithOutOfRangeMcpConcurrency (Enabled=false + concurrency 0 → host boots, /health/live OK) + extended validator skip test. Enabled fail-fast unchanged (RateLimitingValidator_Fails_WhenMcpAuthenticationConcurrencyIsOutOfRange, 3 range cases).
Codex — standalone --mcp --transport http never validated RateLimitingSettings; the limiter constructor only lower-clamps, so >10000 was silently accepted	P2	5db6c6dc	RateLimitingSettingsValidator now runs in standalone Program.cs BEFORE AddTaskdeckRateLimiting constructs the limiter: skipped when disabled, otherwise stderr message + exit 1 (mirroring co-hosted ValidateOnStart semantics and the standalone invalid---port convention). New e2e test StandaloneMcpHttpHost_OutOfRangeConcurrency_FailsFastWithValidationMessage: concurrency 10001 → exit 1, stderr names the setting, app-built seam never fires.

Thread state

Thread	Disposition
Gemini — unused using var lease	Fixed 9707322e; replied; resolved
Codex — cap concurrent failed auth before lookup	Pre-fixed by 1cfebb66 (concurrency gate); replied with evidence; resolved
Codex — valid over-quota key auth-stage DB work	Real, out of scope; tracked as #1384; replied; resolved
Codex — concurrency validated when disabled	Fixed d55ed6cb; replied; resolved
Codex — standalone validation before limiter construction	Fixed 5db6c6dc; replied; resolved

Verification (Release, -m:1, head 5db6c6dc)

• Full solution build: 0 errors
• OptionsValidationTests — 94 passed (+1 boot-level disabled/out-of-range test)
• StandaloneMcpHostFilteringTests — 4 passed (+1 fail-fast validation test)
• McpAuthenticationRateLimitingMiddlewareTests — 7 passed (unchanged, re-run)
• No EF model change (settings class only)

CI was fully green on ed5e7434; re-running on this head. Not merging — the gate decides.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5db6c6dc31

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

Fix Evidence — Abort-Proof Failure Consumption (head 4a91899e)

Final Codex P2 on the previous head, coordinator-adjudicated REAL and now fixed. All review threads remain replied-to and resolved (0 unresolved).

Finding	Severity	Fix Commit	Verified
Codex — aborted MCP auth failures evaded the IP budget: a client disconnecting while the 401 body was written made the response write throw and unwind past the post-_next consume, so an abort-storm repeated invalid-key lookups for free (the concurrency gate caps in-flight work, not per-window lookup count)	P2	4a91899e	ApiKeyMiddleware.WriteErrorResponse sets AuthenticationFailedItemKey in HttpContext.Items BEFORE any response write; the limiter middleware consumes the failure budget in a finally block on marker-or-401, so the charge survives client aborts (OperationCanceledException from the write). New test AbortedFailureResponseWrite_StillConsumesFailureBudget: fake marks failure + sets 401 + throws mid-write → budget decremented, follow-up request 429s at the pre-check without reaching the auth layer.

Verification (Release, -m:1, head 4a91899e)

• Full solution build: 0 errors
• McpAuthenticationRateLimitingMiddlewareTests — 8 passed (+1 abort test)
• McpHttpTransportApiKeyTests — 48 passed

Failure-budget invariants as now shipped

1. Valid requests never spend IP budget (per-key isolation, the MCP /mcp pre-auth IP rate limiter throttles valid traffic and defeats per-key isolation (post-merge finding from #1364) #1368 fix).
2. In-flight pre-auth work per address ≤ concurrency cap at any instant.
3. Every completed-or-aborted authentication failure spends exactly one window permit — abort-proof via the pre-write marker + finally.
4. Failed-auth key lookups per address per window ≤ PermitLimit + concurrency cap, regardless of client disconnect behavior.

CI re-running on 4a91899e (was fully green on the two prior heads). Not merging — the gate decides.