Repository navigation
fix(mcp): harden /mcp from the audit (address budget, Origin, tool contracts) - #1225
Conversation
The per-token key hashes any text after Bearer, so each forged token got a fresh 300/min window. A global limiter on the shared counter now gives each address ten tokens' worth on oauth-api endpoints, before OpenIddict looks the token up. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both MCP revisions require Origin validation against DNS rebinding. An absent Origin passes; any other value must equal the OAuth:Issuer origin. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty grades list removes a draft's grade lines; only omission was documented. A completed idempotency record is purged after 48 hours, after which a retry records again. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
openWorldHint defaults to true, so clients treated nine database reads as external calls. The gate walk now fails a read tool without OpenWorld = false. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grade names are unique per farm, not per account, so two farms' same-named grades at one sort order had no total order across read_stock pages. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r is the id's The first version passed with the tie-breaker removed: the grade query can read through the (account, farm, name) index, which orders by the random farm id. The farms are now ordered against the grade ids. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Live MCP client run at Consent. Two Owner tokens went through dynamic registration, sign-in, J. D. Origin. The SDK client ( A. Forged-bearer burst. The app was hit directly, with The budget is 300 × 10 = 3,000 per address per minute, and the 3,001st request was the first 429. B. B and C. Descriptions as served. F. The attached frame is the consent screen at 1280 px, light theme, 1:1. |
Expiry depends on a later purge sweep, so a key is not forgotten at exactly 48 hours. A write-only connection checks the current entry through a person. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PostgreSQL returns tied rows in no promised order, so the integration case cannot prove the id tie-breaker. The ordering moves to InStockOrder, and a test hands it tied rows in descending id order. The integration case now checks only that each tied row is paged once. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The all-call address budget let one busy connection, or plain valid traffic, throttle every farm behind a shared egress address. Now a presented bearer answered 401 spends the address's failure budget in the shared counter, and an address over budget is refused before authentication. Valid calls, per-token 429s and tokenless probes spend nothing. IFixedWindowCounter gains CountAsync, a read that leaves the count alone, on Redis, in process and through the resilient fallback. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The check ran inside the authenticated POST endpoint, so an unauthenticated POST got the 401 challenge and GET or HEAD got 405 whatever its Origin. Both /mcp mappings now carry the issuer's origin as metadata, and a middleware ahead of rate limiting and authentication answers a foreign Origin with 403. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The flattened-string comparison let ["https://localhost", ""] through, refused a punycode Origin against a Unicode issuer, and refused an explicit default port. A present Origin must now be one origin-only value, compared by scheme, IdnHost and Port. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reply to the Codex review at R1 (P2). Fixed in
Two costs are known and recorded. Once an address has spent its failure budget, valid tokens from it are refused too until the minute rolls over, because nothing can tell them apart before authentication. Check-then-record also lets the requests already in flight when the count crosses the limit through once per window. The R2 (P3). Fixed in R3 (P3). Fixed in R4 (P3). Fixed in R5 (P3). Fixed in Mutations. Each mutation below was applied to the committed code, its tests run, and the file restored. All 11 went red.
The targeted suite at |
|
Live MCP client rerun at Consent. Two Owner tokens went through dynamic registration, sign-in, A. Failure budget. The app was hit directly, with Valid calls spent nothing. The burst got one 401 more than the budget of 300, because 16 requests were in flight when the count crossed the limit. That is the check-then-record overshoot the PR body describes. After the burst the address is refused for the rest of the minute, valid token included, which is the accepted cost. Another address is unaffected. D. Origin, every method, before authentication. A raw 2026-07-28 J. B. These calls recorded C. The served F. The attached frame is the consent screen at 1280 px, light theme, 1:1, from this run. |
…uses writes If Redis answers the read script but refuses increments, each increment lands in the in-process fallback and alarms, while every read returned the unchanged Redis count. CountAsync now adds the fallback's live count for the same key and window. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 301-request test assumed every request landed inside one real, wall-clock-aligned minute, so a run crossing a minute boundary saw 301 401s and no 429. These tests now run on an in-process counter over a FakeTimeProvider, and a separate test advances it to show a spent address recovers when the window rolls over. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DELETE, PUT and OPTIONS on /mcp select routing's own 405 endpoint, which carried no origin metadata and skipped the check. The middleware now matches the /mcp path as routing does, ignoring case with one optional trailing slash, independent of the selected endpoint. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refusals reach the shared failure budget only when they finish, so every lookup started concurrently was admitted. oauth-api endpoints now also cap bearer requests in flight per client address at 32, with no queue, refusing the next with 429. The cap is per process on purpose, as in-flight work is each instance's own, and the #271 record classifies it. The lease is released on every outcome. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e round-2 limits The glossary said the key is forgotten after 48 hours; it is kept for at least 48 hours, after which it may be forgotten. The MCP endpoint record now names the in-flight cap and the path-keyed Origin check. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tead of hanging Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reply to the round-2 Codex review at R6 (P2). Fixed in
R7 (P2). Fixed in R3 (P3). Fixed in R8 (P3). Fixed in Docs. Mutations. Each mutation was applied to committed code, its tests run, and the file restored. All six went red:
The targeted suite at |
|
Live addendum at Origin on DELETE (R3). Failure budget, in-flight cap and the real token (R6). The app was hit directly, with The real token kept working from that address while valid traffic was all there was. The burst passed the budget of 300 by three requests that were already in flight, inside the cap of 32 in flight. After the burst the address was refused, valid token included, as the PR body describes, while another address was unaffected. The hold-open case for the in-flight cap is covered by |
…vershoot The budget is read before the permit is taken, so any number of requests can pass the check before refusals land. The cap bounds the authentication work running at once, not the total overshoot. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the sum An increment Redis applied before its reply timed out also landed in the fallback, so adding the two counts counted it twice. The larger of the two never double counts, still rises while writes fail, and undercounts by at most the smaller count. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es decide The in-flight permit covered the whole MCP request, so 32 slow tool calls from one address refused the next authentication from it. The permit now goes back right after CredentialEpochMiddleware, before authorization, body reading and tool work, with no second authentication. The request's outer using still releases it on a refusal, an exception or a cancellation. The credential-gate order guard lists the new step. Part of #789. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reply to the round-3 Codex review at R9 (P2). Fixed in
R10 (P3). Fixed in R11 (P3). Fixed in Mutations. Applied to committed code, tests run, file restored. All five went red:
The targeted suite at |
|
Live addendum at The real token kept working from that address while valid traffic was all there was, including 40 calls at a time, which is above the in-flight cap of 32. The forged burst ran two past the budget of 300 and was then refused. After it, the address was refused, valid token included, while another address and the proxy path were unaffected. A foreign-Origin DELETE still got 403 on both revisions, and DELETE without Origin got 405. |


Part of #789
Docs-impact: none — the glossary's "connected app" entry changed only its idempotency sentence, from "forgets the key after 48 hours" to "kept for at least 48 hours, after which it may be forgotten", matching the tool description. The in-app definition (
help.glossaryConnectedAppDef, en/es/tl) is left alone on the maintainer's decision, because the Help precache has no room.Fixes seven findings from the two read-only audits of the MCP server at 04dabbb, then the findings of three Codex review rounds on this PR (R1 to R11). Each finding has its own commit. The behaviour changes are A (refused bearers spend a per-address budget) and D (403 for a foreign or malformed
Origin). B and C change tool description text only. E and J tighten guards. F adds one sort key.Two changes reach operators and clients. A client address that keeps presenting refused bearers is cut off for the rest of the minute, valid tokens included. A browser page on another origin can no longer reach
/mcp. No configuration key was added, so the sim harness and the AppHost need no change (#370, #565).A. Forged bearers no longer get a fresh budget each (COD-02, F-01, review R1)
Finding.
oauth-apipartitions on a hash of any text afterBearer, before authentication. From one address, a new forged token on every request never reached a limit, and each one cost an OpenIddict token lookup.Fix. oauth-api endpoints also count refused bearers per client address, in the shared
IFixedWindowCounter(#543/#544), never in the process (#271).UseOAuthApiAddressLimitsruns ahead ofUseRateLimiterand authentication:CountAsync, and answers a JSON-RPC 429 withRetry-Afteronce the count reaches 300 in the minute.300 a minute caps forged-token lookups at five a second per address and leaves room for revoked connections to retry before their people reconnect them. The per-token budget is unchanged.
Refusals reach that budget only when they finish, so the same middleware also caps bearer authentications in flight per address at 32, with no queue (review R6). It is a
PartitionedRateLimiterconcurrency limiter, per process on purpose, because in-flight work is each instance's own. The code anddocs/decisions/271-single-serving-instance.mdrecord that classification.UseOAuthApiInFlightRelease, right afterCredentialEpochMiddleware, gives the permit back once the credential gates decide, so a valid call's body and tool work hold none (review R9). Nothing authenticates twice. The outerusingreleases it on a refusal, an exception or a cancellation.IFixedWindowCounter.CountAsyncreads without incrementing. Redis does it in one script on its own clock, and the in-process counter under its lock. The resilient decorator returns the larger of the Redis and fallback counts, because Redis may answer reads while refusing increments (review R7). The larger, not the sum, because an increment Redis applied before its reply timed out lands in both (review R10). It falls back and alarms when the read throws.The first version of this PR counted every call per address, at ten tokens' worth. The review showed that one busy connection could exhaust it for every farm on a shared address, so it is gone.
Costs. Once an address has spent its failure budget, its valid tokens are refused too until the minute rolls over, because nothing tells them apart before authentication. Check-then-record lets requests that passed the check before earlier refusals landed overshoot, by no fixed number. The cap bounds the authentication work running at once, not that total (review R11). The
ponytail:comment onFailedAuthenticationBudgetrecords that ceiling.Tests.
ForgedBearers_FromOneAddress_RunOutOfTheFailureBudget: 300 forged bearers get 401, and the 301st gets 429. This and the two count-inspection tests run on a counter over a stationaryFakeTimeProvider(review R8).SpentFailureBudget_RecoversWhenTheWindowRollsOver: a spent address gets 429, and after the clock advances one window it gets 401.BearersInFlight_FromOneAddress_AreCapped_AndAnotherAddressIsNot: authentication is held open. 32 bearers from one address are in flight, the 33rd gets 429, and another address still enters authentication. After the gate opens, a further bearer from the first address gets 401.ValidTokenCalls_SpendNoFailureBudget: five valid calls leave the count at 0, and one forged call makes it 1.PerTokenRefusals_SpendNoFailureBudget: 200, 200, 429, 429 with the count still 0.SpentFailureBudget_InTheSharedStore_RefusesBeforeAuthentication: only the shared store reports the budget spent, and a valid token gets 429 with that store'sRetry-After.AuthenticatedWorkInFlight_DoesNotHoldAuthenticationPermits: 32 authenticated requests from one address hold their tool work open, and a fresh authentication from that address gets 200.CountAsynccontract tests run in process, against a real Redis (two counters on one namespace) and through the resilient fallback. That includes a primary that reads but refuses increments, and one that applies increments and then times out.B.
grades: []is documented (F-02)Finding. An empty
gradeslist removes a draft's grade lines. The parameter description said only "Sellable eggs per grade".Fix. The description says that leaving it out keeps the draft's lines and that an empty list removes them.
Test.
OmittedGrades_KeepTheDraftsLines_AndAnEmptyListRemovesThemrecords Large 90, records again withoutgrades(the line stays), then records with[](no lines).C. The idempotency key states its retention (COD-04, review R4)
Finding. The
idempotencyKeydescription promised replay with no time limit.IdempotencyRecordPurgeSweepdeletes completed records some time after 48 hours.Fix. Description text, plus the matching glossary sentence. A successful result is kept for at least 48 hours. After that the key may be purged and a retry may record again, so the caller checks the current entry first with
cluckwork_list_daily_entries, or asks a person when the connection cannot read. Retention is unchanged.D.
/mcprefuses a foreignOrigin(COD-01, F-10, review R2 and R3)Finding. Nothing validated
Origin. MCP 2025-11-25 and 2026-07-28 both require it. The SDK leaves this check to the app: its maintainers declined an SDK Origin filter (modelcontextprotocol/csharp-sdk#1819) in favour of ASP.NET Core'sAllowedHostsand CORS (modelcontextprotocol/csharp-sdk#1554).Fix.
UseMcpOriginCheckruns ahead of rate limiting and authentication. It matches the/mcppath as routing does, ignoring case with one optional trailing slash, whatever endpoint routing selected. DELETE, PUT and OPTIONS, which select routing's own 405, are covered (review R3). A request withoutOriginpasses, because native clients send none. Otherwise the request must carry exactly one value with no path, query, fragment or credentials, sonullfails. That value's scheme, ASCII host (IdnHost) and effective port must equal the configuredOAuth:Issuer's. The request's Host plays no part.CluckworkIdentityRegistrationnow carries the issuerUriinstead of a bool.Tests.
ForeignOrigin_Gets403_BeforeAuthentication_OnEveryMethod, once per revision: a foreign Origin gets 403 on an unauthenticated POST, on GET, on DELETE and on POST to/MCP/. Without Origin, POST gets 401 and GET and DELETE get 405.ForeignOrMalformedOrigin_Gets403_AndTheIssuersOriginConnects, once per revision, with the SDK client.RequiredOrigin_ComparesSchemeAsciiHostAndEffectivePort, 18 rows. They include a Unicode issuer with a punycode header,:443against an implicit port, duplicate values, a duplicate plus an empty value, andnull.E. The free-text guard walks write replies (COD-05, F-05)
Finding.
RealTree_ReadToolRepliesReturnOnlyReviewedTextstarted from read tools only, so a note added toRecordedDailyEntrypassed green.Fix. The test, now
RealTree_ToolRepliesReturnOnlyReviewedText, walks every tool method, as the money guard does.RecordedDailyEntry.FlockNamejoins the reviewed list.F. Stock pages have a total order (COD-03, review R5)
Finding.
GetStockByGradeAsyncsorted bySortOrderthenGradeName. Grade names are unique per farm, not per account, so two farms' "Large" tied, and PostgreSQL returns tied rows in no promised order.Fix.
.ThenBy(r => r.EggGradeId), inEggLotRepository.InStockOrder.Tests.
StockOrder_BreaksANameAndSortOrderTieByIdhands the ordering two tied rows in descending id order.ReadStock_PagesTiedGradesOfTwoFarms_EachOncechecks through the tool that each tied row is paged once withlimit: 1. It makes no claim about read order.J. Read tools set
OpenWorld = false(F-07)Finding. The nine read tools left
openWorldHintat the spec defaulttrue.Fix. All nine set
OpenWorld = false.McpToolSurface.GateFindingsfails a read tool without it, and theOpenWorldReadToolfixture holds the finding.Mutations
Each mutation was applied to committed code, its tests run, and the file restored. Every one went red except the first F mutation, noted in its row.
ForgedBearers_FromOneAddress_RunOutOfTheFailureBudgetValidTokenCalls_SpendNoFailureBudget,PerTokenRefusals_SpendNoFailureBudgetForgedBearers_…,SpentFailureBudget_InTheSharedStore_RefusesBeforeAuthenticationCountAsyncusesINCRinstead ofGETCountAsync_reads_the_shared_count_without_incrementingCountAsyncreads one too manyCountAsync_ReadsWithoutIncrementing_AndResetsWithTheWindowCountAsyncrethrows instead of falling backCountAsync_WhenRedisThrows_ReadsTheFallbackAndAlarmsCountAsyncignores the fallback when Redis readsCountAsync_WhenRedisReadsButRefusesIncrements_AddsTheFallbackCountCountAsyncsums instead of taking the largerCountAsync_AfterIncrementsAppliedThenTimedOut_DoesNotDoubleCountAuthenticatedWorkInFlight_DoesNotHoldAuthenticationPermitsProgram.csCredentialEpochMiddlewareOrderTestsBearersInFlight_…BearersInFlight_…. The 33rd waited on the gate until the 30 s bound.BearersInFlight_…. The request after the gate opened got 429.BearersInFlight_…. The other address waited on the gate.if (grades is not null)becomesif (grades is { Count: > 0 })OmittedGrades_KeepTheDraftsLines_AndAnEmptyListRemovesThem.(Large, 90)stayed after[].ForeignOrigin_Gets403_BeforeAuthentication_OnEveryMethod, both revisionsHostinstead ofIdnHostForeignOrMalformedOrigin_Gets403_AndTheIssuersOriginConnects, both revisionsRecordedDailyEntrygainsstring? NoteRealTree_ToolRepliesReturnOnlyReviewedText.ThenBy(r => r.EggGradeId)removedStockOrder_BreaksANameAndSortOrderTieById,[e2b8…, 1bbd…]against[1bbd…, e2b8…]. The first, integration-only test stayed green under this mutation, which is why the ordering is now tested directly.OpenWorld = falseremoved fromcluckwork_read_stockRealTree_EveryToolCarriesARolePolicyAndAScopePolicyC changes description text only and has no test.
Live MCP client run
The first run, at
3b2c6703, is in the first live-run comment. The rerun ataf0b7397, after the review fixes, used real consent with the password step-up and is in a later comment. In summary:nulland a path also got 403. Without Origin, the same requests got 401 and 405.openWorldHint: falseon all nine read tools.gradesleft out kept two lines, and[]removed them.idempotencyKeytext is served as written.read_stockpaged five grades once each.Out of scope
F-03 (binding errors), F-04 (flock-name case), F-06 (superseded tokens), F-08 (discovery behind an untrusted proxy), F-09 (per-app keys) and COD-06 (the SDK's parse-error code) are untouched.
Local verification
At
644bca77,dotnet test tests/Cluckwork.Api.IntegrationTestsfiltered toMcp,RateLimit,OAuth,SharedState,DistributedIpand the middleware order guard passed: 445 tests, 0 failed.