Dont allow private ips #28
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
globalping-mcp-server | ff4f90f | Nov 29 2025, 03:47 PM |
WalkthroughAdds a new target-validation module that detects localhost, loopback, private IPv4/IPv6, and link-local addresses and exposes an Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/index.ts (1)
1-4: Be deliberate about exposing all target-validation helpers via the public barrelRe-exporting
./target-validationfrom the main lib index makes all helpers (e.g.isPrivateIPv4,isPrivateIPv6, etc.) part of your public API surface. If you only intendisPublicTargetto be a supported entry point, consider exporting just that (or re-exporting named symbols explicitly) to avoid committing to the stability of all the lower-level helpers and reduce the chance of external callers depending on internal details.test/unit/lib/target-validation.test.ts (1)
210-239: Augment tests for localhost FQDNs and IPv4‑mapped IPv6 edge casesGiven the goal of hardening against private/localhost targets, the current tests are missing a few important cases:
localhost.and*.localhost.(FQDNs with a trailing dot) should be treated as localhost domains if you adopt the normalization suggested inisLocalhostDomain.- IPv4‑mapped IPv6 literals such as
::ffff:10.0.0.1and0:0:0:0:0:ffff:192.168.1.1should be explicitly rejected when the embedded IPv4 part is private, and accepted when it’s public.You can extend the suite along these lines:
describe("isLocalhostDomain", () => { @@ it("should not detect empty or non-localhost strings", () => { expect(isLocalhostDomain("")).toBe(false); expect(isLocalhostDomain("example.com")).toBe(false); expect(isLocalhostDomain("google.com")).toBe(false); }); + + it("should detect localhost FQDNs with trailing dot", () => { + expect(isLocalhostDomain("localhost.")).toBe(true); + expect(isLocalhostDomain("app.localhost.")).toBe(true); + }); }); @@ describe("isPublicTarget", () => { @@ describe("localhost domains", () => { @@ it("should reject *.localhost", () => { const result = isPublicTarget("app.localhost"); expect(result.valid).toBe(false); expect(result.reason).toContain("localhost domain"); }); + + it("should reject localhost with trailing dot", () => { + const result = isPublicTarget("localhost."); + expect(result.valid).toBe(false); + expect(result.reason).toContain("localhost domain"); + }); + + it("should reject *.localhost with trailing dot", () => { + const result = isPublicTarget("app.localhost."); + expect(result.valid).toBe(false); + expect(result.reason).toContain("localhost domain"); + }); }); @@ + describe("IPv4-mapped IPv6 addresses", () => { + it("should reject IPv4-mapped private IPv4 addresses", () => { + const result = isPublicTarget("::ffff:10.0.0.1"); + expect(result.valid).toBe(false); + expect(result.reason).toContain("private IPv4"); + }); + + it("should accept IPv4-mapped public IPv4 addresses", () => { + expect(isPublicTarget("::ffff:1.1.1.1").valid).toBe(true); + }); + });These cases will lock in the intended behavior and protect against regressions as the validation logic evolves.
Also applies to: 241-405
src/mcp/tools.ts (2)
45-53: Avoid repeating the same target-validation/error block in every toolThe
isPublicTargetcheck plus the long error message is duplicated in each tool handler. This makes it easy for behavior or wording to drift if one call site is updated but others are not, and increases the chance that a new tool forgets to add the validation at all.You could centralize this into a small helper in this module, e.g.:
function assertPublicTarget(target: string) { const validation = isPublicTarget(target); if (!validation.valid) { throw new Error( `Invalid target: ${validation.reason}. Globalping only supports public endpoints. Private IP addresses (RFC1918), localhost, and link-local addresses are not allowed.`, ); } }Then each tool handler only needs:
assertPublicTarget(target);This keeps the behavior and messaging consistent and makes future policy changes (e.g., expanding blocked ranges) a single edit instead of five.
Also applies to: 78-85, 141-149, 181-188, 244-250, 305-312, 371-380, 415-422, 479-485, 531-538
244-289: Custom DNSresolverparameter currently bypasses the public-target validationIn the DNS tool, only
targetis validated withisPublicTarget, whileresolveris passed straight through intomeasurementOptions:measurementOptions: { query: { type: queryType || "A" }, resolver, trace: trace || false, }If the goal is “no private IPs / localhost / link-local anywhere in a measurement”, this lets callers still point probes at private or localhost resolvers (e.g.,
192.168.0.1,fc00::1,localhost) even though the main target is constrained.If resolvers are also supposed to be public-only, consider validating them too, e.g.:
async ({ target, locations, limit, queryType, resolver, trace }) => { return handleToolExecution(async () => { // Validate target is public - const validation = isPublicTarget(target); - if (!validation.valid) { - throw new Error( - `Invalid target: ${validation.reason}. Globalping only supports public endpoints. Private IP addresses (RFC1918), localhost, and link-local addresses are not allowed.`, - ); - } + const validation = isPublicTarget(target); + if (!validation.valid) { + throw new Error( + `Invalid target: ${validation.reason}. Globalping only supports public endpoints. Private IP addresses (RFC1918), localhost, and link-local addresses are not allowed.`, + ); + } + + if (resolver) { + const resolverValidation = isPublicTarget(resolver); + if (!resolverValidation.valid) { + throw new Error( + `Invalid resolver: ${resolverValidation.reason}. Globalping only supports public resolvers. Private IP addresses (RFC1918), localhost, and link-local addresses are not allowed.`, + ); + } + }If, on the other hand, private resolvers are intentionally allowed (e.g., to hit operator-controlled infrastructure that isn’t globally routable), it would be good to clarify that exception in the description and docs so users don’t assume the “public-only” rule applies uniformly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/index.ts(1 hunks)src/lib/target-validation.ts(1 hunks)src/mcp/tools.ts(11 hunks)test/unit/lib/target-validation.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/lib/index.tstest/unit/lib/target-validation.test.tssrc/mcp/tools.tssrc/lib/target-validation.ts
🧬 Code graph analysis (2)
test/unit/lib/target-validation.test.ts (1)
src/lib/target-validation.ts (7)
isPrivateIPv4(16-51)isPrivateIPv6(62-77)isLoopbackIPv4(86-96)isLoopbackIPv6(105-110)isLinkLocalIPv4(119-136)isLocalhostDomain(145-148)isPublicTarget(157-226)
src/mcp/tools.ts (1)
src/lib/target-validation.ts (1)
isPublicTarget(157-226)
🔍 Remote MCP
I have successfully processed the comprehensive pull request documentation. Let me provide you with relevant context about the changes in this PR:
PR Context Summary
Pull Request: "Dont allow private ips" (#28)
This PR introduces critical validation to prevent private IP addresses from being used in network measurements through the Globalping MCP Server. Based on the extensive documentation, here are the key insights:
Changes Overview
The PR adds three major components:
-
Target Validation Module (
src/lib/target-validation.ts)- Functions to detect private IPv4/IPv6, loopback, and link-local addresses
- Localhost domain detection for various formats (localhost, .localhost, 127.0.0.1, ::1)
- Public target validation that returns
{ valid: boolean, reason?: string }with specific failure reasons
-
Integration into MCP Tools (
src/mcp/tools.ts)- Runtime validation on every Globalping tool registration to enforce public-only endpoints
- Error responses when private IPs/localhost/link-local addresses are detected
- Updated tool descriptions to explicitly note public-only restrictions
-
Comprehensive Test Coverage (
test/unit/lib/target-validation.test.ts)- RFC1918 private range validation (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)
- IPv6 unique local and link-local address detection
- Localhost and .localhost TLD handling
- Boundary cases and mixed format handling
Security Rationale
The PR addresses MCPcat's logged instances of users attempting to run tests against LAN IPs (10.0.0.x), which represents a significant security concern. Private IP testing could expose internal network infrastructure or be used maliciously.
Integration Points
According to the documentation, the validation integrates at multiple layers:
- [::Authentication::] OAuth and API token flows remain unchanged; validation happens post-authentication
- [::MCP Server::] Validation occurs before token retrieval and measurement execution in the request flow
- [::API Client::] The sanitized token is still used for all Globalping API calls to public targets
Code Quality Standards
The codebase enforces:
- Biome-based formatting (4-space indent, 100-char line width)
- Comprehensive TypeScript strict mode
- Automated import organization
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
| export function isLocalhostDomain(target: string): boolean { | ||
| const lower = target.toLowerCase(); | ||
| return lower === "localhost" || lower.endsWith(".localhost"); | ||
| } |
There was a problem hiding this comment.
Localhost FQDNs and IPv4‑mapped IPv6 addresses currently bypass the “public target” guard
Two notable edge cases slip through isPublicTarget right now:
-
Localhost with a trailing root dot
Domains such as"localhost."and"app.localhost."are standard FQDN representations of the localhost namespace, butisLocalhostDomainonly matches"localhost"and*.localhostwithout a trailing dot. These inputs are therefore treated as public and accepted. -
IPv4‑mapped IPv6 literals with private IPv4 parts
Addresses like::ffff:10.0.0.1or0:0:0:0:0:ffff:192.168.1.1are IPv6 forms of private IPv4 addresses. The current logic only checks IPv6 ULA/link‑local prefixes (fc,fd,fe8–feb) and does not look for IPv4‑mapped forms, so these will be classified as public even though they conceptually target the same private space you’re trying to block.
If the underlying Globalping stack accepts IPv4‑mapped IPv6 addresses (common in many networking stacks), this creates a straightforward way to sidestep the private‑IP restriction.
You can tighten both cases with small changes:
-export function isLocalhostDomain(target: string): boolean {
- const lower = target.toLowerCase();
- return lower === "localhost" || lower.endsWith(".localhost");
-}
+export function isLocalhostDomain(target: string): boolean {
+ const lower = target.toLowerCase();
+
+ // Normalize a trailing root dot (e.g., "localhost.", "app.localhost.")
+ const normalized = lower.endsWith(".") ? lower.slice(0, -1) : lower;
+
+ return normalized === "localhost" || normalized.endsWith(".localhost");
+}And in isPublicTarget, normalize IPv4‑mapped IPv6 into the IPv4 path so the existing private/loopback/link‑local checks apply:
// Try to determine if it's an IP address
// IPv6 addresses may be in brackets
const ipToCheck = cleanTarget.replace(/^\[|\]$/g, "");
- // Check if it looks like an IPv4 address (contains only digits and dots)
- const ipv4Pattern = /^[\d.]+$/;
- if (ipv4Pattern.test(ipToCheck)) {
+ // Handle IPv4-mapped IPv6 like ::ffff:10.0.0.1 or 0:0:0:0:0:ffff:10.0.0.1
+ const ipv4MappedMatch = ipToCheck.match(
+ /^(?:\:\:ffff:|0:0:0:0:0:ffff:)(\d+\.\d+\.\d+\.\d+)$/i,
+ );
+ const ipv4Candidate = ipv4MappedMatch ? ipv4MappedMatch[1] : ipToCheck;
+
+ // Check if it looks like an IPv4 address (contains only digits and dots)
+ const ipv4Pattern = /^[\d.]+$/;
+ if (ipv4Pattern.test(ipv4Candidate)) {
// Check IPv4 loopback
- if (isLoopbackIPv4(ipToCheck)) {
+ if (isLoopbackIPv4(ipv4Candidate)) {
return {
valid: false,
- reason: `${ipToCheck} is a loopback address (127.0.0.0/8)`,
+ reason: `${ipv4Candidate} is a loopback address (127.0.0.0/8)`,
};
}
// Check IPv4 private ranges
- if (isPrivateIPv4(ipToCheck)) {
+ if (isPrivateIPv4(ipv4Candidate)) {
return {
valid: false,
- reason: `${ipToCheck} is a private IPv4 address (RFC1918)`,
+ reason: `${ipv4Candidate} is a private IPv4 address (RFC1918)`,
};
}
// Check IPv4 link-local
- if (isLinkLocalIPv4(ipToCheck)) {
+ if (isLinkLocalIPv4(ipv4Candidate)) {
return {
valid: false,
- reason: `${ipToCheck} is a link-local address (169.254.0.0/16)`,
+ reason: `${ipv4Candidate} is a link-local address (169.254.0.0/16)`,
};
}
}Separately, if target can ever include a port (e.g., "10.0.0.1:80"), that form will currently bypass the IPv4 checks because of the colon and be treated as IPv6‑ish, eventually classified as public. Either enforce a “host only” invariant at all call sites or strip an IPv4‑style host:port before running this logic to avoid that loophole as well.
Also applies to: 157-225
🤖 Prompt for AI Agents
In src/lib/target-validation.ts around lines 145–148 (and also affecting
157–225), isLocalhostDomain and isPublicTarget miss FQDNs with a trailing dot
(e.g., "localhost." or "app.localhost.") and fail to detect IPv4‑mapped IPv6
literals (e.g., "::ffff:10.0.0.1"), and host:port forms can confuse IPv4 checks;
fix by normalizing input: trim trailing dot from domain names before matching
localhost and subdomains, strip a trailing ":port" when present (only for
IPv4-style host:port), detect and extract IPv4 from IPv4‑mapped IPv6 addresses
(map ::ffff:a.b.c.d to a.b.c.d) and then reuse the existing IPv4
private/loopback checks so these cases are properly classified as non-public.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp/tools.ts (1)
75-81: Validation correctly implemented and consistently applied.The target validation is properly placed before token retrieval across all measurement tools, preventing invalid requests early with clear error messages. The implementation correctly blocks private IPs, localhost, and link-local addresses as intended.
To reduce duplication, consider extracting the validation logic into a helper function:
/** * Validate that a target is a public endpoint * @throws Error if target is not public */ function validatePublicTarget(target: string): void { const validation = isPublicTarget(target); if (!validation.valid) { throw new Error( `Invalid target: ${validation.reason}. Globalping only supports public endpoints. Private IP addresses (RFC1918), localhost, and link-local addresses are not allowed.`, ); } }Then replace each validation block with:
validatePublicTarget(target);This would centralize the error message and make future updates easier, though the current implementation is perfectly acceptable.
Also applies to: 178-184, 302-308, 412-418, 528-534
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp/tools.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/mcp/tools.ts
🧬 Code graph analysis (1)
src/mcp/tools.ts (1)
src/lib/target-validation.ts (1)
isPublicTarget(157-226)
🔍 Remote MCP DeepWiki
Summary of additional repo context relevant to reviewing PR #28 ("Dont allow private ips"):
-
Project architecture and where tool/validation integrates
- Cloudflare Worker + Durable Object MCP agent (GlobalpingMCP) registers tools and routes /mcp and /sse; tools call Globalping API via src/api/client.ts.
-
Where to check integrations/usage of isPublicTarget
- Tool registration/handlers live under src/mcp and are invoked by GlobalpingMCP.init(); API client (src/api/client.ts) and tool wrappers call create/runMeasurement. PR changes should be reviewed in src/mcp/tools.ts and GlobalpingMCP tool registration paths.
-
OAuth / token flow does validation before routing to Durable Object
- Main fetch handler detects API token vs OAuth and routes to handleAPITokenRequest or OAuthProvider → handleMcpRequest → GlobalpingMCP. New target validation in tools.ts will reject targets before token use. Review interaction with isAPITokenRequest/handleAPITokenRequest.
-
URL / redirect validation and existing helpers
- Repo contains URL validation utilities (isTrustedRedirectUri, isLocalhost, isDeepLink, exception hosts) and tests — relevant for consistency with new target-localhost/domain checks.
-
Tests and test strategy
- Unit and integration test setup: Vitest unit tests for lib functions and workers integration tests. New unit tests were added (test/unit/lib/target-validation.test.ts) — run unit suite and Workers integration suite per repo's test scripts.
-
Config and deployment implications
- Worker routes, KV (OAUTH_KV) and Durable Object binding names are defined in wrangler.jsonc; ensure no runtime binding/name changes required by PR.,
Files/areas to focus review on (quick checklist):
- src/lib/target-validation.ts and exported API surface (ensure edge cases, bracketed IPv6, trimming, clear reasons).
- src/mcp/tools.ts modifications where isPublicTarget is used — confirm errors returned before any Globalping API calls and messages match docs/UI.
- test/unit/lib/target-validation.test.ts — run locally; verify coverage for RFC1918 ranges, IPv6 ULA/link-local, localhost variants, bracket handling, and reason strings.
- Integration points: handleAPITokenRequest and handleMcpRequest flow (src/index.ts) to ensure props/token logic unaffected.
- URL validation/exception host logic overlap to avoid conflicting trust rules (src/lib/url-validation.ts area covered in docs).
- CI/test configuration (vitest unit + workers integration) to ensure added tests will run in pipeline.
Sources used:
- Repo documentation and pages (architecture, MCP server, API client, auth, tooling, testing) — DeepWiki read of repository docs and contents.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (1)
src/mcp/tools.ts (1)
9-9: LGTM!The import is correctly placed and the validation function is properly integrated into all measurement tools.
MCPcat already logged users having issues with their agents trying to run tests against LAN ips like 10.0.0.x.