Repository navigation
feat(mcp): enforce per-tool rate limits for MCP roles - #1544
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thanks @Coding-Professional, this follows what we agreed on #983. I went through the diff and it looks good: every query is scoped to the workspace, the new endpoint is admin only and the DTO is tight. Two small things before you mark it ready:
The CodeQL note about hashing Mark the PR ready when CI is green and I'll take it from there. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
helpcodeai
left a comment
helpcodeai
left a comment
There was a problem hiding this comment.
Thanks @Coding-Professional, both points are done well:
- The "has any limit" check is cached per workspace for 5 seconds, so workspaces without limits skip the membership and grant queries.
- A full in-memory store now lets the call through and logs a warning.
The safer setToolAccess is a good extra: it checks the role's workspace and keeps existing grants and their limits.
CI is green. Since this adds a migration, I'll merge it right after today's release, so it ships on its own in the next one.
…ode-ai#983) Any member can create MCP API keys, so a budget per key name let a limited user multiply it by creating more keys. The counter is now keyed on workspace, tool and user, shared by sessions and all API keys.
|
@Coding-Professional, one more thing came up when I tested this locally against a real PostgreSQL and Redis. I pushed a small fix to your branch (292c7ef), so you don't need to do anything. The counter was keyed on the API-key name, the same identity as the principal limiter. Any member can create MCP API keys, so a user with 10 calls per minute could create five keys and make 50. The counter is now keyed on workspace, tool and user. A user has one budget per tool, shared by their sessions and all of their keys. The service spec, the endpoint description and the API reference say so too. Everything else held up in my tests:
I'll merge once CI is green on the new commit. |
Summary
Add optional per-principal minute/hour limits to role tool grants, as agreed in #983, so admins can budget calls to metered APIs and database tools. This PR covers the migration and backend; the role editor UI is a separate follow-up.
Changes
callsPerMinuteandcallsPerHourfields with positive-value SQL CHECK constraints documented in Prisma.isErrorwith the reached limit and retry time.Type
Testing
Full CI before the latest main sync, on
22c9dfed: Backend CI: 31,820 passed, 1,862 skipped; 1,036 suites passed. Lint, typecheck, build and backend/files-worker boot checks passed. CLI test/build passed on Linux, macOS and Windows; frontend build and E2E passed. CodeQL reports no new alerts. Docker image build and boot/health check passed.Latest main sync: merged upstream
3ac1d4ebinto396f00a1without conflicts (two adapter description corrections). Frompackages/backend,npm test -- --runInBand --runTestsByPath src/adapters/catalog.spec.ts src/adapters/de/weclapp.spec.ts src/adapters/de/weclapp.write-tools.spec.ts src/adapters/de/weclapp.live.spec.ts src/adapters/intl/dynamics365-business-central.live.spec.tspassed: 5 suites, 15,529 tests passed, 5 credential-gated live tests skipped. From the root,node scripts/validate-adapters.mjspassed with the same 1,440 existing warnings;node scripts/generate-llms-txt.mjs --checkandgit diff --checkpassed. CI is rerunning on the new merge commit.Local checks used Windows, Node 24.17.0, PostgreSQL 17 and Redis 7 in throwaway Docker containers. Database/Redis URLs were provided through
DATABASE_URL,AUDIT_TEST_DATABASE_URLandTOOL_RATE_LIMIT_TEST_REDIS_URL.From
packages/backend:npx prisma generate,npx prisma validate,npx prisma migrate deploy: passed; all 70 migrations applied to an empty database.npm run lint:check,npx tsc --noEmit -p tsconfig.json,npm run build: passed after the review fixes; lint reports 11 existing warnings.npx jest --runInBand --runTestsByPath src/roles/tool-rate-limits.spec.ts src/roles/roles.service.spec.ts src/roles/tool-rate-limits.postgres.spec.ts src/mcp-server/tool-rate-limit.service.spec.ts src/mcp-server/tool-rate-limit.redis.spec.ts src/mcp-server/dynamic-mcp-tools.rate-limit.spec.ts: 6 suites, 111 tests passed against real PostgreSQL/Redis. Covers cache query counts, concurrent cold probes, invalidation, TTL, tenant isolation, capacity fail-open, atomic admission, expiry, retained grants, cache hits and nested calls.npx jest --runInBand --runTestsByPath src/mcp-server/tool-rate-limit.service.spec.ts: 10 passed after adding warning-throttle assertions.npx jest --runInBand --runTestsByPath src/connectors/engines/database.engine.ssrf.spec.ts: 8 passed after syncing the deterministic upstream DNS test (Make the allowlisted DB host SSRF test independent of DNS #1547)./healthreturned HTTP 200 in both. Processes and test containers were stopped afterward.From the root:
npm ci --no-audit --no-fund: passed.npm test -w packages/cli: 23 passed, 3 unavailable-shell skips, with Git Bash on PATH.npm run build -w packages/cli: passed.node scripts/validate-adapters.mjs: passed, with 1,440 existing non-blocking warnings.node scripts/adapter-count.mjs --check,node scripts/check-runtime-deps.mjs,node scripts/generate-cli-operations.mjs --check,node scripts/generate-llms-txt.mjs --check,git diff --check: passed.node --test scripts/generate-llms-txt.test.mjs: 11 passed, 4 Windows file-symlink privilege skips.Jest reports a worker teardown warning, also present before these review fixes; the suite exits successfully. The full backend suite is verified in CI. A duplicate local full-suite run was stopped after an earlier full CI run passed; the local results above are the focused runs.
Checklist
Related Issues
Relates to #983. Backend scope approved in #983 (comment); the UI remains a follow-up.