fix(rate-limit): fail closed without trusted client identity - #47
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changes분산 rate limit fail-closed 처리
런타임 보안 및 배포 계약
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client as 클라이언트
participant Exchange as /exchange
participant Identifier as trustedClientIdentifier
participant RateLimit as distributedRateLimitObjectName
participant DurableObject as Durable Object
Client->>Exchange: 요청 전송
Exchange->>Identifier: CF-Connecting-IP 검증 및 정규화
Identifier-->>Exchange: canonical IP 또는 undefined
alt 유효한 식별자
Exchange->>RateLimit: 객체 이름 생성
RateLimit->>DurableObject: rate limit 조회
DurableObject-->>Exchange: rate limit 결과
Exchange-->>Client: 요청 처리 결과
else 유효하지 않은 식별자
RateLimit-->>Exchange: DistributedRateLimitUnavailable
Exchange-->>Client: 503 응답
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![noema-maintainer[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
CHANGELOG.md (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
postcss보안 수정 항목을 하나로 통합하세요.Line 43은 Line 21과 동일한
postcss버전, GHSA,overrides수정 내용을 기록합니다.security:scan세부 내용이 필요하면 Line 21에 통합하고 중복 항목을 제거하세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 43, Remove the duplicate postcss security entry at the referenced changelog location and retain a single consolidated entry with the postcss version, GHSA, overrides change, and security:scan details in the existing earlier changelog item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/distributed-rate-limiting.md`:
- Around line 89-90: Update the rollback guidance around line 87 to allow
rollback only to releases that preserve fail-closed behavior for missing or
invalid CF-Connecting-IP identities. Explicitly prohibit rollback to versions
using a shared unknown fallback bucket; otherwise retain the current entrypoint
and repair the Cloudflare route or transform configuration first.
In `@package.json`:
- Around line 42-43: Declare the Node.js minimum version as >=22 by adding the
engines.node setting in package.json, and update docs/deployment-guide.md to
state Node.js 22+ instead of 20+. Ensure both the package metadata and
deployment documentation consistently reflect the wrangler and undici runtime
requirements.
In `@src/rate-limit.ts`:
- Around line 60-67: Update trustedClientIdentifier to parse CF-Connecting-IP as
exactly one valid IPv4 or IPv6 address using a Workers-compatible validator,
rejecting malformed, out-of-range, comma-separated, and arbitrary values with
undefined before Durable Object lookup. Return a canonical normalized
representation so equivalent IPv6 spellings share the same rate-limit bucket,
and add regression tests covering valid addresses, range errors, multiple
addresses, arbitrary strings, and IPv6 normalization.
In `@test/distributed-rate-limit.test.ts`:
- Around line 247-277: Update the test “fails closed before object lookup
without a bounded Cloudflare client identity” to pass an observedNames
collection from namespaceReturning() into the runtime environment, then assert
it remains empty after all requests. Keep the existing handler assertion and
response contract checks so the test verifies neither idFromName() nor the
Durable Object fetch is invoked.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 43: Remove the duplicate postcss security entry at the referenced
changelog location and retain a single consolidated entry with the postcss
version, GHSA, overrides change, and security:scan details in the existing
earlier changelog item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d1c7eec7-1d12-41f3-b350-b29608b02d02
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
CHANGELOG.mddocs/distributed-rate-limiting.mdpackage.jsonsrc/rate-limit.tstest/distributed-rate-limit.test.ts
…% gate
The main-merge added canonicalIpv6, whose two post-parse guards cannot trigger:
a successful `new URL("http://[cand]/")` parse only occurs for a valid IPv6
literal, so the WHATWG hostname is always bracketed and its inner value always
contains ":". Empirically verified across 133,888 gate-passing candidates
(exhaustive length 1-4 over the allowed alphabet plus adversarial/random
inputs): every one of the 2,396 that parsed stayed bracketed with a ":"-bearing
inner. The bracketed-check and ":"-check guards are kept as fail-closed
backstops and marked `/* v8 ignore next */` so the restored 100% threshold
holds. Line 84's ternary is rewritten as an equivalent guard clause so its
covered happy-path return stays measured; behavior is unchanged.
Verification: npx vitest run --coverage (100% statements/branches/functions/
lines, gate green), npm run typecheck, npm test (268 pass), npm audit
(0 vulnerabilities). No change to #47 rate-limit logic, the threshold, or the
undici override/engines.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SEAGtwNR96cia2djq7XFCo
2 participants
Summary
/exchangerequests before Durable Object lookup when Cloudflare'sCF-Connecting-IPis missing, malformed, or overlongunknownfallback bucket that could let one identity-stripped traffic source exhaust the request budget for unrelated callersX-Forwarded-For/X-Real-IPdistrust and existing distributed limiter fail-closed responsesundicidependency to patched7.29.0after current advisories correctly failed the release auditCommercial-readiness impact
Cloudflare documents
CF-Connecting-IPas the edge-provided client identity, but request and managed transforms can remove it. Treating missing identity as one global bucket created an avoidable availability boundary: malformed or misconfigured traffic could consume the shared budget and deny legitimate buyers' automation. The new behavior fails closed with503andRetry-After: 1before token parsing or Durable Object invocation, making the ingress misconfiguration explicit and bounded.During verification,
npm audit --audit-level=highdetected newly publishedundiciadvisories in the locked transitive dependency. The lockfile is regenerated withundici@7.29.0, preserving a zero-high-severity release gate rather than bypassing the audit.Verification
npm run typechecknpm testnpm run security:scannpm run release:verifyReferences
CF-Connecting-IPundici@7.29.0Summary by CodeRabbit
개선 사항
/exchange요청에서 유효한 IPv4·IPv6 식별자만 허용하며 IPv6 주소를 일관되게 정규화합니다.503오류로 안전하게 처리합니다.보안
undici취약점 대응을 반영했습니다.배포