operator: rate-limit the diagnostics that fan out to third parties - #665
Merged
Conversation
…hird parties Closes the one open code-scanning alert (CodeQL js/missing-rate-limiting, high, #81 on /__operator/ledger-sync.json). The route was already operator-authed and already cached its RPC fan-out for 15s, and someone had written the alert id into a comment as if that settled it. It does not: a cache bounds how often we hit an UPSTREAM, auth bounds WHO can call, and neither bounds how much work one caller can queue against this process. Those are three different guarantees and only two were present. Applied to BOTH expensive operator routes, not only the flagged one. discovery-gap.json reaches the on-chain economy snapshot and has the identical shape; the scanner found one instance and the defect is the class. That distinction is the recurring theme of today's work. Budget is deliberately generous (30/min, 300/hour): these are human-driven diagnostics, and a limit that locks an operator out of their own dashboard mid-incident would be worse than the abuse it prevents. Verified the bound does not spill onto the cheap operator endpoints. 4 assertions added to scripts/test-operator-auth.js (34 total). Mutations killed: removing the limiter from the flagged route, and from the unflagged sibling. Note on the implementation: the first version called clientIp(req), which does not exist in this file. It passed `node --check` and would have thrown on the first request - the same call-before-definition slip as earlier today, caught the same way, by running it rather than trusting the syntax check. It now derives the ip exactly as the MCP transport limiter does, honouring the app's trust-proxy setting so Railway's proxy is not treated as the client. Scanning status at this commit: 0 open Dependabot alerts, 0 open secret-scanning alerts, and this was the last open code-scanning alert. Co-Authored-By: Claude Opus 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
1 participant
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.
Closes the last open code-scanning alert (CodeQL
js/missing-rate-limiting, high, #81 on/__operator/ledger-sync.json).Why the existing mitigations were not enough
The route was already operator-authed and already cached its RPC fan-out for 15s, and the alert id had been written into a comment as if that settled it. It does not — these are three different guarantees and only two were present:
Applied to the class, not the instance
discovery-gap.jsonreaches the on-chain economy snapshot and has the identical shape. CodeQL flagged only its sibling; both are now bounded.Budget is deliberately generous (30/min, 300/hour) — these are human-driven diagnostics, and a limit that locks an operator out of their own dashboard mid-incident would be worse than the abuse it prevents. Verified the bound does not spill onto the cheap operator endpoints.
Ordering
The limiter runs after
operatorAuthed, which matters: an unauthenticated caller still gets404and can never elicit a429, so the new status is not a route-existence oracle and an attacker cannot poison an operator's bucket.Verification
404, authed200, and a 35-request burst produced exactly 5 ×429against a 30/min budgetscripts/test-operator-auth.js(34 total), in CI/__operatormiddleware already setsno-store, private+Varyon the 429 path, and the new bucket shares no table with the credential-guessing limiterImplementation note
The first version called
clientIp(req), which does not exist in this file. It passednode --checkand would have thrown on the first request. It now derives the IP exactly as the MCP transport limiter does, honouring the app's trust-proxy setting.Scanning status: 0 open Dependabot alerts, 0 open secret-scanning alerts.