sec(server): block direct IP requests bypassing Cloudflare proxy in p…#4023
Open
berkelmali wants to merge 2 commits into
Open
sec(server): block direct IP requests bypassing Cloudflare proxy in p…#4023berkelmali wants to merge 2 commits into
berkelmali wants to merge 2 commits into
Conversation
Contributor
WalkthroughAdds Cloudflare/loopback IP helpers and environment-gated middleware/upgrade guards that block HTTP requests (403) and close WebSocket connections (1002) appearing to bypass Cloudflare in Prod/Preprod. Changes span Master.ts, Worker.ts, WorkerLobbyService.ts, a new Cloudflare.ts, and unit tests. ChangesCloudflare Proxy Bypass Protection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/server/Master.ts`:
- Around line 61-70: The middleware added in Master.ts (the app.use(...) that
checks req.headers["cf-connecting-ip"]) is using a spoofable header for
security; replace this trust-with-header logic by validating the TCP source
(req.socket.remoteAddress) against an authoritative Cloudflare IP set or,
preferably, move the allow/block decision to your network layer (firewall/load
balancer) or enable Cloudflare Authenticated Origin Pulls/Tunnel so only
Cloudflare can reach the origin; keep cf-connecting-ip only for best-effort
logging, and update the middleware in Master.ts to perform a remote-address vs.
Cloudflare CIDR check (or short-circuit to rely on the infrastructure-level
check) rather than gating on the presence of cf-connecting-ip.
🪄 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
Run ID: d5613f17-9e04-4710-9c4a-19ef6b7662bb
📒 Files selected for processing (2)
src/server/Master.tssrc/server/Worker.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
…st Cloudflare CIDRs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/server/Master.ts (1)
63-86: ⚡ Quick winUse
isLoopbackIp()for the secondary loopback check.Lines 77-78 use substring matching (
clientIp.includes("127.0.0.1")) while line 66 usesisCloudflareOrLoopbackIp(). For consistency and correctness, reuse the helper function.♻️ Proposed fix
+import { isCloudflareOrLoopbackIp, isLoopbackIp } from "./Cloudflare"; -import { isCloudflareOrLoopbackIp } from "./Cloudflare";const host = req.headers.host ?? ""; const isIpHost = /^[0-9.:]+$/.test(host); - if ( - isIpHost && - !clientIp.includes("127.0.0.1") && - !clientIp.includes("::1") - ) { + if (isIpHost && !isLoopbackIp(clientIp)) { log.warn(`Bypassed Cloudflare proxy. Host header was an IP: ${host}`); return res.status(403).send("Forbidden: Direct IP access is blocked."); }🤖 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 `@src/server/Master.ts` around lines 63 - 86, In the middleware in Master.ts that blocks non-Cloudflare direct connections, replace the ad-hoc substring checks on clientIp (the two clientIp.includes("127.0.0.1") / clientIp.includes("::1") conditions) with the helper isLoopbackIp(clientIp) and use it consistently with isCloudflareOrLoopbackIp(); specifically, in the host-is-IP branch inside the app.use(...) callback, change the condition to check !isLoopbackIp(clientIp) (or combine appropriately with isCloudflareOrLoopbackIp) so loopback detection is centralized via the isLoopbackIp() helper.src/server/Worker.ts (1)
108-131: ⚡ Quick winUse
isLoopbackIp()for the secondary loopback check.Same pattern issue as in Master.ts. Lines 122-123 use substring matching while line 111 uses
isCloudflareOrLoopbackIp().♻️ Proposed fix
+import { isCloudflareOrLoopbackIp, isLoopbackIp } from "./Cloudflare"; -import { isCloudflareOrLoopbackIp } from "./Cloudflare";const host = req.headers.host ?? ""; const isIpHost = /^[0-9.:]+$/.test(host); - if ( - isIpHost && - !clientIp.includes("127.0.0.1") && - !clientIp.includes("::1") - ) { + if (isIpHost && !isLoopbackIp(clientIp)) { log.warn(`Bypassed Cloudflare proxy. Host header was an IP: ${host}`); return res.status(403).send("Forbidden: Direct IP access is blocked."); }🤖 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 `@src/server/Worker.ts` around lines 108 - 131, The middleware in the app.use block uses substring checks clientIp.includes("127.0.0.1") and clientIp.includes("::1") to detect loopback addresses; change this to call the shared helper isLoopbackIp(clientIp) (same approach used by isCloudflareOrLoopbackIp) so the check becomes !isLoopbackIp(clientIp) when deciding to block IP-host requests; update the conditional in the middleware that currently references clientIp.includes(...) to use isLoopbackIp and ensure the import/visibility of isLoopbackIp is present.src/server/WorkerLobbyService.ts (1)
106-134: ⚡ Quick winUse
isLoopbackIp()for consistency; remove unnecessary type cast.Same loopback check pattern issue. Also,
socket.remoteAddressexists onnet.Socket, so theas anycast is not needed.♻️ Proposed fix
+import { isCloudflareOrLoopbackIp, isLoopbackIp } from "./Cloudflare"; -import { isCloudflareOrLoopbackIp } from "./Cloudflare";if ( ServerEnv.env() === GameEnv.Prod || ServerEnv.env() === GameEnv.Preprod ) { - const clientIp = (socket as any).remoteAddress ?? ""; + const clientIp = socket.remoteAddress ?? ""; if (!isCloudflareOrLoopbackIp(clientIp)) { this.log.warn( `WebSocket upgrade rejected: Bypassed Cloudflare proxy. Remote IP: ${clientIp}`, ); socket.write("HTTP/1.1 403 Forbidden\r\n\r\n"); socket.destroy(); return; } const host = request.headers.host ?? ""; const isIpHost = /^[0-9.:]+$/.test(host); - if ( - isIpHost && - !clientIp.includes("127.0.0.1") && - !clientIp.includes("::1") - ) { + if (isIpHost && !isLoopbackIp(clientIp)) { this.log.warn( `WebSocket upgrade rejected: Bypassed Cloudflare proxy. Host header was an IP: ${host}`, ); socket.write("HTTP/1.1 403 Forbidden\r\n\r\n"); socket.destroy(); return; } }🤖 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 `@src/server/WorkerLobbyService.ts` around lines 106 - 134, Replace the ad-hoc loopback string checks and the unnecessary type cast: read the client IP directly from socket.remoteAddress (remove "(socket as any)" cast) and instead of testing clientIp.includes("127.0.0.1")/("::1") use the existing utility isLoopbackIp(clientIp). Keep the prior isCloudflareOrLoopbackIp(clientIp) check and the host-is-IP gate, but change the second condition to use !isLoopbackIp(clientIp) to ensure consistent loopback detection via the shared helper (referencing socket.remoteAddress, isCloudflareOrLoopbackIp, isLoopbackIp, and the host header check).src/server/Cloudflare.ts (1)
3-29: Cloudflare CIDR lists match the official published rangesThe hardcoded IPv4/IPv6 CIDR lists in
src/server/Cloudflare.tsmatch the current values from Cloudflare’s published endpoints (/ips-v4and/ips-v6), so there’s no immediate staleness issue.Optional: add a short comment with the source URLs and the last-checked date (or a lightweight refresh strategy) to prevent future drift when Cloudflare changes them.
🤖 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 `@src/server/Cloudflare.ts` around lines 3 - 29, The CIDR arrays CLOUDFLARE_IPV4_CIDRS and CLOUDFLARE_IPV6_CIDRS are hardcoded without provenance; add a short comment above these exports that lists the Cloudflare source URLs (/ips-v4 and /ips-v6), the date you last verified them, and a suggested lightweight refresh strategy (e.g., note to periodically compare against those endpoints or fetch during startup if needed) so future maintainers know where and when to update them.
🤖 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.
Nitpick comments:
In `@src/server/Cloudflare.ts`:
- Around line 3-29: The CIDR arrays CLOUDFLARE_IPV4_CIDRS and
CLOUDFLARE_IPV6_CIDRS are hardcoded without provenance; add a short comment
above these exports that lists the Cloudflare source URLs (/ips-v4 and /ips-v6),
the date you last verified them, and a suggested lightweight refresh strategy
(e.g., note to periodically compare against those endpoints or fetch during
startup if needed) so future maintainers know where and when to update them.
In `@src/server/Master.ts`:
- Around line 63-86: In the middleware in Master.ts that blocks non-Cloudflare
direct connections, replace the ad-hoc substring checks on clientIp (the two
clientIp.includes("127.0.0.1") / clientIp.includes("::1") conditions) with the
helper isLoopbackIp(clientIp) and use it consistently with
isCloudflareOrLoopbackIp(); specifically, in the host-is-IP branch inside the
app.use(...) callback, change the condition to check !isLoopbackIp(clientIp) (or
combine appropriately with isCloudflareOrLoopbackIp) so loopback detection is
centralized via the isLoopbackIp() helper.
In `@src/server/Worker.ts`:
- Around line 108-131: The middleware in the app.use block uses substring checks
clientIp.includes("127.0.0.1") and clientIp.includes("::1") to detect loopback
addresses; change this to call the shared helper isLoopbackIp(clientIp) (same
approach used by isCloudflareOrLoopbackIp) so the check becomes
!isLoopbackIp(clientIp) when deciding to block IP-host requests; update the
conditional in the middleware that currently references clientIp.includes(...)
to use isLoopbackIp and ensure the import/visibility of isLoopbackIp is present.
In `@src/server/WorkerLobbyService.ts`:
- Around line 106-134: Replace the ad-hoc loopback string checks and the
unnecessary type cast: read the client IP directly from socket.remoteAddress
(remove "(socket as any)" cast) and instead of testing
clientIp.includes("127.0.0.1")/("::1") use the existing utility
isLoopbackIp(clientIp). Keep the prior isCloudflareOrLoopbackIp(clientIp) check
and the host-is-IP gate, but change the second condition to use
!isLoopbackIp(clientIp) to ensure consistent loopback detection via the shared
helper (referencing socket.remoteAddress, isCloudflareOrLoopbackIp,
isLoopbackIp, and the host header check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdd76940-2170-4ff2-b477-860319a635e6
📒 Files selected for processing (5)
src/server/Cloudflare.tssrc/server/Master.tssrc/server/Worker.tssrc/server/WorkerLobbyService.tstests/server/Cloudflare.test.ts
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.
Description:
Prevent attackers from bypassing Cloudflare Turnstile and global rate limiting by accessing worker ports directly via server IP addresses.
Previously, individual game cluster workers hosted public HTTP/WebSocket listeners directly on public ports 3001, 3002, etc. If the server firewall is exposed or misconfigured, attackers could query the worker nodes directly on these ports (e.g.
123.45.67.89:3001), completely bypassing Cloudflare Turnstile (captchas) and Master node rate-limiting configurations.This fix secures the application layer against proxy bypasses:
Hostheader is a domain (not a raw IP) and that the incoming request contains Cloudflare proxy headers (e.g.cf-connecting-ip).Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires