Repository navigation
fix(backend): IP-tier limits on the anonymous marketplace reads and gateway health - #2325
Conversation
c964fd1 to
bdceb75
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
#2325 SECURITY CLEARED at bdceb75
No blocking security findings in the complete six-file diff (rebased head), including the route-guard baseline changes. All eight named anonymous GET registrations place platformIpRateLimit ahead of handler work. Seven marketplace reads use 300/minute; gateway health uses 60/minute. The three apps reads share their router's bucket, the three marketplace-api reads share another, and official has its own bucket: this is not one combined 300-request budget across all seven routes. Existing IP-key and named-429 behavior is reused.
Own exact-archive Node 22 run: 36 tests across six suites PASS (28 committed plus eight independent arms), --runInBand --detectOpenHandles, no force exit, 15.322s. Independent arms load each actual router, preserve its registered middleware/order, and replace only the terminal handler with an effect counter. Every route accepts its full budget, then returns 429 platform_rate_limited/code rate_limited without another handler effect; a different IP still succeeds. This tests the real limiter and registrations over local Express/Supertest, not database or gateway work. Existing official-route, readiness, DB-health and route-guard tests also pass.
Removing all eight changed limiter registrations causes all eight independent arms to fail at expected 429 versus received 200. All four route files restored byte-for-byte; all six changed files match the pinned head. Changed TypeScript lint: zero errors, 25 warnings. Full diff/check clean.
The inherited Cloudflare-header trust assumption and in-memory per-process budgets remain: I did not test production ingress/header replacement, multiple backend replicas, real gateway traffic, or load capacity. The comments/PR traffic observations are operator evidence. No new security blocker follows from those unchanged deployment assumptions.
Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS
…ateway health The seven anonymous marketplace reads (apps.ts /marketplace, /featured, /:id; marketplace-api.ts /browse, /manifests/:id, /manifests/:id/forks; marketplace.ts /official) take platformIpRateLimit at 300/min/IP, and /api/health/clawdbot, which calls the agent gateway on each hit, takes 60/min/IP. Threat-model budgets, as the other IP-tier routes: the ingress log shows almost no traffic on these. The route rate-limit guard baseline drops their eight entries, and a behavioural test shows the 301st read gets the named 429. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCgCsf3UJRCPRbnNKxsECe
bdceb75 to
e034768
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
SECURITY / wake gate for #2325 (IP-tier limits on the anonymous marketplace reads and gateway health) at e034768, the head Lily named (pod message 83769). Live head check: headRefOid = e034768, OPEN, not draft, base main. Reads: the gh diff of the four route files and the new readLimit test, the platformIpRateLimit middleware (identical to main), cloudflareIpRateLimitKeyGenerator and its header comments in ipRateLimit.ts, and the trust-proxy setting at server.ts:121. Not run: the test suites and any live request. The baseline JSON and the rebase are Otto's code gate.
Verdict: PASS. All eight routes use a platform limiter keyed on the Cloudflare header, and none falls back to a default-key limiter.
The eight, checked against the diff at this head.
- apps.ts GET /marketplace, /marketplace/featured, /marketplace/:id: marketplaceReadLimit.
- health.ts GET /clawdbot: gatewayHealthLimit.
- marketplace-api.ts GET /browse, /manifests/:installableId()/forks, /manifests/:installableId(): marketplaceReadLimit.
- marketplace.ts GET /official: marketplaceReadLimit.
Both limiters come from platformIpRateLimit (middleware/platformRateLimit.ts, identical to main). That function setskeyGenerator: cloudflareIpRateLimitKeyGenerator. It does not use the default key, and no route in the diff calls rateLimit() directly.
The key. cloudflareIpRateLimitKeyGenerator (ipRateLimit.ts:49) reads CF-Connecting-IP first, and falls back to req.ip only when the header is absent or empty. Under the two assumptions its own comment records (the tunnel is the sole entrance, and the edge rejects a client-supplied CF-Connecting-IP), every external caller has the header set by Cloudflare, so the buckets are per client. Trust-proxy (server.ts:121) does not change that path.
Non-blocking, for the follow-up.
- The limiter is only as strong as assumption 2. If the edge ever forwards a client-supplied CF-Connecting-IP, a caller can rotate the header per request and get a fresh bucket each time. The probe that backs assumption 2 is dated 2026-09-23. Re-run it before relying on this for the 00:30Z deploy, or record that it is still current.
- The test added at marketplace.readLimit.test.js exercises the 300/min budget on /official, and nothing checks the key. A test that two different CF-Connecting-IP values get separate buckets, and that a request with only req.ip set is still limited, would pin the property you asked me to verify. Without it, a future change to the default key would pass CI.
- The budget is per process. The backend runs one replica (values-dev, per the docs), so the stated number is the real one. A second replica doubles the effective budget. Not a defect today.
Not covered: CI, CodeRabbit, the baseline refresh, and the live probe. The code gate for the baseline and the rebase is Otto's.
Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS
lilyshen0722
left a comment
There was a problem hiding this comment.
Otto, gate measured at e03476872710ab0bfe2e74f54e8fb0c07e24a9d4.
CODE gate: PASS. Checked out this exact head and verified the rebase and baseline against the complete route population.
Replayed the prior head bdceb7550b9740cf25dbc4d91dc25dcc61b75d09 onto the new base 14cc3e5114ff22b8acfb1cbeaff5ed349881e76a. The only textual conflict is routeRateLimitGuard.baseline.json, exactly the two competing rows in the ask. Aborted the replay and restored the reviewed head clean. git range-diff shows the baseline context change; all four production route files and the behavioral test are byte-identical between old and new heads. Excluding the baseline, both commits have stable patch ID 0e5c92d47534775ff030069e18a098462b7280f0. Comparing old and new baselines leaves only the independently fixed messages DELETE row removed.
The full scanner sees 473 registrations in 78 files, with identical route registration identities before and after this PR. Exactly the eight requested anonymous GET registrations change from unlimited to limited: apps (three), gateway health (one), marketplace-api (three), official marketplace (one). No registration loses a limiter or moves one behind auth. The current-base baseline shrinks 175 → 167, adding no entries; the resulting 167 are sorted, unique and exactly equal the remaining scan violations.
Both conflict rows must stay absent:
marketplace.ts GET /officialcarries this PR'smarketplaceReadLimit.messages.ts DELETE /:idalready carriessendMessageRateLimitbeforeauthon main. The independent change is in 933b06f / #2624, outside this PR.
Falsification: restoring those two baseline rows, while retaining sorted order, fails the stale-baseline test and names both. Removing all eight newly registered limiter arguments fails the new-violation test and names all eight. Each mutation produces 15 pass/1 fail in the 16-test guard suite; all source/baseline mutations were restored.
Node 22 commands:
npm --prefix backend test -- --runInBand routeRateLimitGuard marketplace health platformRateLimit ipRateLimit: 9 suites / 81 tests pass, including 16/16 route guard tests and the real/officialbudget check: 300 successful requests, then the named platform 429.- Full parent/head scan and baseline comparison:
node /tmp/otto-2325-probe/scan.cjs; mutation artifacts under/tmp/otto-2325-probe/. - Scoped ESLint on the four changed route files: zero errors, 25 warnings.
git diff --checkand a trial merge against current main86c6dc1d138a2899ad16cc687fa1ee4a8c65ff5c: clean. No intervening main commit touches the changed paths. Worktree clean.
No blocking code defect found. Nonblocking comment defect: marketplace.ts says its reads query the database on every hit, but /official loads a manifest and caches it (default five-minute TTL). Its real work is cached-response serialization plus cache-miss file/upstream loading. The limiter remains useful; that copied premise is inaccurate. Budgets are shared by each three-route router's limiter instance; /official has its own instance. The guard classifies middleware by source name, so the scan is not a behavioral proof of every endpoint. The behavioral endpoint and factory/key tests above complement it. No live ingress measurement was performed.
Exact-head CI has 19 successful check runs, one expected skip and zero pending/failing, including required Test & Coverage, Service Tests, E2E, chart lint and CodeQL. GitHub reports non-draft/MERGEABLE/CLEAN. coderabbit-threads is successful with no review-comment threads. CodeRabbit itself reports Review skipped, consistent with the existing @coderabbitai ignore in the public security PR body and the documented review opt-out; this is not an external code-review completion claim.
This PASS dies with the head.
Written by Otto, a Commonly agent — https://commonly.me/v2/pods/6a920d3eb08dee6c42a81bfc
GATE: PASS
Adds the platform IP tier (
middleware/platformRateLimit.ts) to the eight unauthenticated routes that had no limiter and do work on every hit:apps.tsGET /marketplace,/marketplace/featured,/marketplace/:id;marketplace-api.tsGET /browse,/manifests/:installableId(*),/manifests/:installableId(*)/forks;marketplace.tsGET /official.health.tsGET /clawdbot, which makes a request to the agent gateway on each hit. Its one caller, AgentsHub, asks once per page load.These are threat-model budgets, labelled like the other IP-tier routes. The 7-day ingress log shows almost no traffic on any of them. Each limiter is declared beside its route, as
platformRateLimit.tsasks.The route rate-limit guard baseline drops the eight entries. A behavioural test (
marketplace.readLimit.test.js) shows the 301st read from one IP gets429 platform_rate_limited. The existing health and marketplace route tests pass, andtsc:checkand eslint are clean.This came out of a check that every unauthenticated route is rate-limited. Register, login, password reset, verification, waitlist, OAuth and device auth already were. It ships after Wednesday 10-21.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MCgCsf3UJRCPRbnNKxsECe
@coderabbitai ignore