Skip to content

fix: rate limit MCP calls by API key (ORB-8) - #462

Open
thomasluizon wants to merge 7 commits into
mainfrom
fix/orb-8-mcp-rate-limit
Open

fix: rate limit MCP calls by API key (ORB-8)#462
thomasluizon wants to merge 7 commits into
mainfrom
fix/orb-8-mcp-rate-limit

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Aug 7, 2026

Copy link
Copy Markdown
Owner

DEGRADED: same-vendor review

Implements ORB-8.

Summary

  • Adds a distributed general MCP policy of 60 calls per minute, partitioned per API key or per stable user ID for JWT-authenticated callers.
  • Adds a distributed AI policy of 15 calls per minute for direct get_daily_summary, get_goal_review, and get_retrospective calls.
  • Applies the same AI policy when execute_agent_operation_v2 wraps get_daily_summary, get_retrospective, or review_goals.
  • Rejects limited calls before policy evaluation or tool execution with HTTP 429, Retry-After, and a JSON-RPC rate_limit_exceeded error.
  • Leaves unauthenticated handshake and notification traffic outside the limiter path.

Design choices

The ticket did not prescribe numeric budgets. The general policy uses 60 per minute because the existing tags policy uses that budget. The AI policy uses 15 per minute because the existing ai-operations and habit-suggest policies use that budget.

AI classification lives in the MCP middleware so rate limiting happens before tool execution and can return an explicit transport-level 429. Both policies use the existing database-backed DistributedRateLimitService, so counts are shared across instances.

The middleware now deserializes the envelope through the installed SDK's JsonRpcMessage, JsonRpcRequest, and CallToolRequestParams types. It does not hand-parse an assumed MCP shape. Wrapped operation IDs come from the verified params.arguments.operationId location, and their fingerprints use the inner params.arguments.arguments value, matching AgentOperationExecutor.

Validation

  • dotnet build Orbit.slnx --no-restore: 0 errors
  • dotnet test --no-build --no-restore: 5,605 passed, 0 failed
  • Focused WebApplicationExtensionsMcpTests: 31 passed, 0 failed

External interface evidence

The installed package is ModelContextProtocol.Core 1.4.1. Its local nuspec pins source commit 2b7fd35fbe58dfb9f00eae8b3393e1a7361b5e01.

Complete serializable tools/call request key set and types from that installed package:

{
  jsonrpc: string, exactly "2.0";
  id: string | signed 64-bit integer;
  method: string, exactly "tools/call";
  params: {
    name: string, required;
    arguments?: object<string, any JSON value>;
    _meta?: object<string, any JSON value>;
    task?: {
      ttl?: signed 64-bit integer milliseconds;
    };
  };
}

The SDK source-generation options use WhenWritingNull, so optional null fields above are absent rather than serialized as null. CallToolRequestParams.Arguments is the tool argument dictionary. For this repository's execute_agent_operation_v2 method, that dictionary has operationId: string, optional inner arguments: JSON object, and optional confirmationToken: string, as defined by AgentTools.ExecuteAgentOperation.

Re-derive from the installed package without credentials:

ilspycmd -p -o "$env:TEMP\mcp-core-1.4.1-decompiled" "$env:USERPROFILE\.nuget\packages\modelcontextprotocol.core\1.4.1\lib\net10.0\ModelContextProtocol.Core.dll"

Inspect the generated files ModelContextProtocol.Protocol/JsonRpcMessage.cs, JsonRpcMessageWithId.cs, JsonRpcRequest.cs, RequestId.cs, RequestParams.cs, CallToolRequestParams.cs, and McpTaskMetadata.cs. ModelContextProtocol.Client/McpClient.cs shows CallToolAsync sending method tools/call with CallToolRequestParams; ModelContextProtocol/McpSession.cs constructs the JsonRpcRequest. The positive middleware tests serialize these same installed SDK types and assert the complete emitted key set instead of hand-authoring a fixture.

The authentication partition claims remain in-repository contracts: JwtTokenService emits ClaimTypes.NameIdentifier, and ApiKeyAuthenticationHandler emits both that claim and api_key_id.

Evidence

Backend-only change. No screenshot or visible-effect evidence applies.

@thomasluizon

Copy link
Copy Markdown
Owner Author

Approach for ORB-8:

  1. Add two distributed policies in DistributedRateLimitService: a general MCP budget matching the existing 60 per minute read convention, and an AI tool budget matching the existing 15 per minute AI operation convention.
  2. Extend WebApplicationExtensions so authenticated MCP requests acquire the general bucket using the authenticated api_key_id. Tool calls named get_daily_summary or get_retrospective also acquire the stricter AI bucket before policy evaluation or tool execution.
  3. Return HTTP 429 with Retry-After and a JSON-RPC error body when either bucket rejects the call, so the client receives an explicit failure and no tool work begins.
  4. Add focused middleware tests for per-key isolation, AI rejection before the general limit, explicit error output, and unauthenticated bypass, plus service coverage for the new policy limits.

The expected files are src/Orbit.Api/Extensions/WebApplicationExtensions.cs, src/Orbit.Infrastructure/Services/DistributedRateLimitService.cs, tests/Orbit.Infrastructure.Tests/Extensions/WebApplicationExtensionsMcpTests.cs, and tests/Orbit.Infrastructure.Tests/Services/DistributedRateLimitServiceTests.cs. I am keeping AI classification in the MCP middleware instead of HabitTools.cs: a tool-level string return would mix throttling with business output and could not reliably produce an HTTP 429 before execution. ASP.NET in-process rate limiter middleware is also rejected because it would not share state across instances.

@thomasluizon
thomasluizon marked this pull request as ready for review August 7, 2026 15:07

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7cfd4d1bd3

ℹ️ 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".

Comment thread src/Orbit.Api/Extensions/WebApplicationExtensions.cs Outdated
Comment thread src/Orbit.Api/Extensions/WebApplicationExtensions.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

DEGRADED: same-vendor review

Same-vendor bias direction is known: this review can favor code from the same vendor family. Its magnitude in a real PR loop is unmeasured.

Verdict: BLOCKING

Head: 23931850ca93320e235789946090a4eaae389f51

Blocking findings

[HIGH] The one-minute MCP policies reset their full allowances every 15 seconds. blocking: true

  • dimension: 1. Correctness
  • location: src/Orbit.Infrastructure/Services/DistributedRateLimitService.cs:43
  • diff quote: ["mcp"] = new(TimeSpan.FromMinutes(1), PermitLimit: 60, SegmentCount: 4),
  • claim: Both added policies select four segments. The service computes a 15-second segment end, stores that value as the bucket's WindowEndsAtUtc, and deletes the bucket as soon as that instant arrives. Each API key therefore regains the full 60-call general allowance and 15-call AI allowance every 15 seconds, allowing 240 general calls or 60 AI calls per minute instead of the advertised 60 and 15. This defeats the security and spend-control intent of ORB-8.
  • fix: Retain and count segmented buckets for the intended full policy window, return a retry time that matches that window, and add a moving-clock regression spanning segment boundaries.
  • reference: ORB-8 acceptance criteria; rubric dimension 1; orbit-api security hard rules.

[HIGH] General MCP rate-limit errors lose the JSON-RPC request id. blocking: true

  • dimension: 1. Correctness
  • location: src/Orbit.Api/Extensions/WebApplicationExtensions.cs:535
  • diff quote: id = requestId,
  • claim: The new general limiter runs for every authenticated MCP method, but requestId comes from TryGetMcpToolCall, which returns before reading id unless the method is tools/call. When a request such as tools/list reaches the general limit, the 429 body therefore contains id: null instead of the request's id, so a JSON-RPC client cannot correlate the promised explicit error to the throttled request.
  • fix: Extract the top-level JSON-RPC id independently of tool-call classification before applying the general limit, and cover a limited non-tool request with its exact id.
  • reference: ORB-8 explicit-error acceptance criterion; rubric dimension 1.

Non-blocking follow-ups

  • ORB-258: the added tests call the helper directly, so removing the middleware invocation leaves them green.
  • ORB-259: get_retrospective has no regression proving it acquires the stricter AI policy.

N/A dimensions

    1. Contract alignment and backward compatibility: no REST DTO, controller route, shared schema, or shipped mobile contract changes. The MCP JSON-RPC correlation defect is reported under correctness.
    1. Cross-platform parity: backend-only diff.
    1. i18n: no user-facing localized strings.
    1. Design: no apps/* UI files.
    1. FEATURES.md gating: this is a backend rate-limit bugfix with no new or removed screen, tool, plan gate, platform availability, or locale behavior.

Dimensions 2, 3, 4, 6, and 11 were applicable and produced no additional findings.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a18530d641

ℹ️ 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".

Comment thread src/Orbit.Api/Extensions/WebApplicationExtensions.cs Outdated
Comment thread src/Orbit.Api/Extensions/WebApplicationExtensions.cs Outdated
@thomasluizon

Copy link
Copy Markdown
Owner Author

@codex review

@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2026

Copy link
Copy Markdown

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: d31bad003d

ℹ️ 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".

@thomasluizon

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: d31bad003d

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant