MCP: per-client rate limiting + durable operator quota hook for public tools#1633
Merged
Conversation
…tables (#1575) * Mirror loadedFromSource onto the request context so it is observable with plain-id gets (#1571) The cache-disposition flag was only set on the RequestTarget of the get; with a plain string id the static dispatch mints an internal target the caller never sees, so Context.loadedFromSource (declared but never assigned) always read undefined. Now every site that sets the flag on the target mirrors it onto the context via a shared helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Set cache disposition on cache hits in the loadAsInstance=false get path and ensureLoaded; document exact stale semantics Review findings (Gemini): the loadAsInstance=false get path and ensureLoaded() set the flag on source fetch but never wrote false on cache hits, leaving a stale true on shared contexts; and with allowStaleWhileRevalidate the flag reads false (the cache-hit overwrite wins over the synchronous true at fetch start), while staleIfError fallbacks read true — jsdoc now states both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Guard primitive targets in setLoadedFromSource Instance-API gets on loadAsInstance=false tables can pass a primitive id as the target; assigning a property on a primitive throws in strict mode (Codex review). Context mirroring still applies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(resources): remove dead wasLoadedFromSource() stub (#1576) wasLoadedFromSource() was declared on ResourceInterface and documented as reporting cache disposition, but its only implementation was the base-class stub that always returned undefined — no subclass ever overrode it, and get() returns a plain RecordObject with no instance methods anyway. We've standardized on context.loadedFromSource / target.loadedFromSource (this PR) as the supported way to observe cache disposition, so remove the misleading interface declaration and stub. Non-breaking: the method never returned a meaningful value. The only in-repo callers (OCSP/CRL cert verification) invoke it via `(x as any).?.()` on a plain record, so they were already reading undefined and are unaffected. Docs: HarperFast/documentation#563. Closes #1576. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Kyle Bernhardy <kyle.bernhardy@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kris Zyp <kriszyp@gmail.com>
Component authors can now expose arbitrary text/blob content under author-chosen URIs on the application profile, parallel to mcpTools (#622) and mcpPrompts: entries declare a fixed uri or an RFC-6570-style uriTemplate ({name} one segment, {+name} cross-segment), served by an instance method dispatched on the LIVE registry class, with optional per-parameter completion values and resources list_changed support. Exported-Resource descriptors now list under harper+rest:// (the spec reserves https:// for web-fetchable resources); legacy http(s):// URIs still read and subscribe. Reserved schemes are rejected in mcpResources declarations so authors cannot shadow the built-in surfaces. Also fixes a latent registration gap: component entry loading is asynchronous past the boot awaits, so tableless components' custom mcpTools/mcpPrompts/mcpResources could register after the initial walk with no schema event to refresh it. Resources gains a monotonic registrationVersion and the MCP transport rebuilds lazily per request when the registry moved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…om scheme
Codex final review: the lazy per-request rebuild could change the tool
set without emitting notifications/tools/list_changed (only prompts and
resources were notified), and the reserved-scheme guard could be
bypassed with a parameterized scheme position like {scheme}://{+path}.
The notifier per-session diffs, so no-op rebuilds stay silent; schemes
must now be literal and non-reserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e a legacy URI
Gemini bot review: docs:///{name}/{name} silently overwrote the first
captured value; and the scheme back-compat test requested harper+rest
for both cases (an overzealous scheme-migration sed), so the legacy
http(s) path was untested.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
guessAppHttpUrlPrefix appended the raw HTTP_PORT/HTTP_SECUREPORT config value as if it were a bare port, producing authorities like harper+rest://127.0.0.9:127.0.0.9:9926 when the port was configured as a bind address (host:port). Extract just the port. Pre-existing (legacy https:// descriptors were equally malformed); surfaced by runtime verification of #1609 and rolled into this PR at the user's request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Custom resources impose no auth of their own (listing is per-profile, reads delegate to the author's method — parity with mcpTools' visibleTo: () => true). The new integration case runs a full no-Authorization-header session end-to-end; the registry doc no longer implies a login gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a durable, operator-pluggable quota hook and per-client rate limiting for MCP tool calls to prevent session-cycling abuse, alongside platform-specific port-sharing adjustments and replication catch-up fixes for blobs. Feedback on the changes highlights a potential runtime TypeError in resources/Table.ts when assigning properties directly to a potentially primitive or undefined target object, and suggests making the misconfiguration warning flag in components/mcp/quota.ts profile-specific to improve observability.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. |
…1610) Session-scoped buckets are trivially cycled by an anonymous client (initialize, call, drop session, repeat) and reset on restart — no cost control for public unauthenticated tools. Two additions, both opt-in: - Per-client-identity token buckets (rateLimit.ts): keyed on client identity (socket IP, or the first value of a config-named trusted header for proxied deployments), surviving session cycling. Config perClientPerSecond/perClientBurst per profile, default off; burst floors at one whole token so fractional sustained rates still admit; per-profile config now cached with a 10s TTL to keep the per-call identity resolution off the env-lookup path; startup warning when an identity header is configured (spoofing trap). - Durable quota hook (quota.ts): mcp.<profile>.quota.resource names an exported Resource whose static method (default allowMcpCall) is called per admitted tools/call with {identity, tool, user, profile, sessionId}; deny with {allowed:false, message?, retryAfterSeconds?} surfacing as kind:'quota_exceeded'. Fail-closed on misconfiguration or hook errors (#1422 precedent); dispatches on the live registry class; runs after the in-memory admit so throttled clients cannot spam a table-backed hook. Stacked on feat/mcp-custom-resources-1609 for the lazy registry freshness (#1613) that makes custom-tool fixtures deterministic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rebase onto the #1613 branch had replayed the intervening v5.1 release commits (including the #1575 revert) as head-side commits, polluting the PR diff; the branch now carries only this feature's commit. The misconfiguration warn-once flag is per profile so a second misconfigured profile still logs (gemini bot). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kylebernhardy
force-pushed
the
feat/mcp-identity-quota-1610
branch
from
531cbfb to
594ff6d
Compare
Member
|
Reviewed via Claude review-queue (full review: harper-1633-594ff6d.md). Approving — the per-client token bucket (keyed on identity, not session, so it survives session-cycling abuse) and the fail-closed durable quota hook are solid, and the hot path stays cheap/lock-free. One item for a fast-follow: the durable-quota reference implementation ( Companion docs (documentation#571) carry the same "one-token floor" style caveat gap worth syncing once that lands. |
kriszyp
approved these changes
Jul 7, 2026
kriszyp
left a comment
kriszyp
left a comment
Member
There was a problem hiding this comment.
Approving — solid design, TOCTOU race in the reference quota fixture flagged as a fast-follow in the comment above.
The reference fixture's get-then-put counter is the shape operators will copy, and it can undercount under concurrent calls for the same identity. RACE-SAFETY note in quota.ts (transaction / CAS / native atomic increment) plus an honest caveat on the fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Fast-follow addressed in 6d48990 (this PR, doc-only) rather than deferred, since the docs PR was about to ship the same copyable shape:
Chose the doc-warning option over demonstrating an atomic pattern in the fixture: a "safe example" that gets Harper's transaction conflict semantics subtly wrong would be worse than an honest warning — happy to do a follow-up that adds a verified atomic-counter example if you'd like one. Comment generated by an LLM (Claude Fable 5). |
…ta-1610-merge-v51 # Conflicts: # components/mcp/rateLimit.ts # components/mcp/tools/application.ts
Matches this repo's no-restricted-imports convention already followed by the other MCP integration test files. ok/strictEqual/deepStrictEqual are strict-mode in both modules, so this is behavior-preserving.
kylebernhardy
pushed a commit
to HarperFast/documentation
that referenced
this pull request
Jul 8, 2026
…per#1633) Companion to HarperFast/harper#1633 (fixes HarperFast/harper#1610): rateLimit.perClientPerSecond/perClientBurst/identityHeader and the mcp.<profile>.quota.* hook with the counter-table example. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1610. Stacked on #1613 (base =
feat/mcp-custom-resources-1609, for the lazy registry-freshness fix that custom-tool fixtures need); will retarget tov5.1automatically when #1613 merges — only the last commit is this PR's diff until then.Summary
The existing
tools/callbuckets key on session — an anonymous client evades them by cycling sessions (initialize→ call → drop → repeat) — and are in-memory per worker, so nothing survives a restart. For a public, cost-bearing tool (the issue's LLM-backedanswer), that's no cost control at all. Two opt-in additions:1. Per-client-identity buckets (
rateLimit.ts) —mcp.<profile>.rateLimit.perClientPerSecond/perClientBurst(default off) key a token bucket on client identity: socket IP (adapters now passrequest.ip→NormRequest.clientIp), or the first value of a config-named trusted header (identityHeader) for proxied deployments. Survives session cycling; pruned on the existing idle sweep; denials surface as the existingkind: 'rate_limited'shape withscope: 'per_client'.2. Durable quota hook (
quota.ts, new) —mcp.<profile>.quota.resourcenames an exported Resource; its static method (defaultallowMcpCall) runs per admittedtools/callwith{identity, tool, user, profile, sessionId}and can deny with{allowed: false, message?, retryAfterSeconds?}→kind: 'quota_exceeded'. The reporter's per-IP daily counter table drops straight in (the integration fixture is exactly that). Fail-closed on misconfiguration or hook errors (#1422 precedent); dispatches on the live registry class; runs after the in-memory admit so throttled clients can't spam a table-backed hook.Where to look
quota.ts): a broken hook denies all tool calls on the profile rather than silently disabling cost protection. Deliberate; flagging for confirmation since it can brick a tool surface on an operator bug (warn-once log names the misconfiguration).identityHeaderspoofing: client-controlled unless the proxy strips it — doc'd on the config key and a startup warning fires when configured. An alternative (trusted-proxy depth) was left out for KISS.configFordid 6–7 env lookups per session-create only). Config edits take effect within seconds instead of immediately — acceptable?0.1/s= "6/minute") with omitted burst floor at one whole token, else the bucket could never admit (cross-model review catch).Testing
15 new unit tests (bucket isolation/session-cycling/refill/fractional floor/identity resolution; quota hook allow/deny/fail-closed/live-class) + 3 integration tests on a real instance: the full abuse ladder (fresh session per call: 3×ok → 3×
quota_exceededwith author message →per_clientdenials), and the counter row visible over REST. Also verified live at runtime: the quota survives a full process restart (attacker still denied; in-memory buckets reset) and socket-IP fallback works with no header. All 452 MCP unit + 35 MCP integration tests pass;test:unit:resources979 passing.Generated by an LLM (Claude Fable 5); cross-reviewed by Codex (fractional-burst dead-bucket caught and fixed) and Gemini (hot-path config caching + spoof warning, both applied).
Docs: companion documentation PR — HarperFast/documentation#571.