fix(#229): stop logging request data and harden AI-proxy rate-limit keying#234
fix(#229): stop logging request data and harden AI-proxy rate-limit keying#234husamql3 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughRemoves debug console logging of request data (messages, systemPrompt, conversationId) from the chat endpoint, and changes the rate-limiter's key generation logic to derive identifiers from Cloudflare-trusted headers (cf-connecting-ip, x-real-ip, x-forwarded-for) instead of the client-controlled x-api-key header. ChangesProxy Security Hardening
Estimated code review effort: 1 (Trivial) | ~5 minutes Related issues: Suggested reviewers: husamql3 PoemA rabbit hops through logs once loud, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/proxy/src/limit.ts (1)
7-13: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDrop the
X-Real-IP/X-Forwarded-Forfallbacks.CF-Connecting-IPis the trusted client-IP source here; the other headers aren’t guaranteed to represent the real visitor, so they shouldn’t decide the rate-limit key. Fall back to"anonymous"instead.🤖 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 `@packages/proxy/src/limit.ts` around lines 7 - 13, The rate-limit key selection in keyGenerator still trusts X-Real-IP and X-Forwarded-For, which should not be used here. Update the identifier resolution to rely only on cf-connecting-ip and otherwise fall back directly to "anonymous", keeping the existing keyGenerator and Context usage intact.
🤖 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.
Outside diff comments:
In `@packages/proxy/src/limit.ts`:
- Around line 7-13: The rate-limit key selection in keyGenerator still trusts
X-Real-IP and X-Forwarded-For, which should not be used here. Update the
identifier resolution to rely only on cf-connecting-ip and otherwise fall back
directly to "anonymous", keeping the existing keyGenerator and Context usage
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef72debc-9ae3-452c-abd4-b30322143ef9
📒 Files selected for processing (2)
packages/proxy/src/index.tspackages/proxy/src/limit.ts
💤 Files with no reviewable changes (1)
- packages/proxy/src/index.ts
Summary
/chatproxy handler — the handler wrote the fullmessages,systemPrompt, andconversationIdto the Worker console on every call;systemPromptcarries the connected database's schema, so this kept user schema/PII in Cloudflare Workers logs with no benefit. Now nothing about the request body is logged.x-api-keyheader that the proxy never validates, so the per-key daily cap could be sidestepped by varying that header (each value is a fresh bucket). The key now derives fromcf-connecting-ip(edge-set, unspoofable) with the same fallbacks, so the cap actually holds.Testing
bun run typecheck→ passesbun run format→ passesbun run build→ passes(The proxy package has no test suite; verified by typecheck + build + grep that the log lines and header reference are gone.)
Closes #229
Glossary
cf-connecting-ipsystemPromptSummary by CodeRabbit