feat: Implement CORS and Origin validation for enhanced security#24
feat: Implement CORS and Origin validation for enhanced security#24MartinKolarik merged 15 commits intomasterfrom
Conversation
eyepokes
commented
Nov 21, 2025
eyepokes
commented
- Added validateOrigin and getCorsOptions functions to handle Origin header validation and CORS configuration.
- Integrated Origin validation in MCP request handling to prevent DNS rebinding attacks.
- Updated existing tools to include structured output and detailed descriptions.
- Refactored tools registration to use registerTool method with input and output schemas.
- Enhanced help and guide tools with structured content.
- Created unit tests for security utilities, covering valid and invalid origin scenarios.
- Updated OAuth flow tests to register a client with the OAuth provider.
- Updated dependencies
- Added validateOrigin and getCorsOptions functions to handle Origin header validation and CORS configuration. - Integrated Origin validation in MCP request handling to prevent DNS rebinding attacks. - Updated existing tools to include structured output and detailed descriptions. - Refactored tools registration to use registerTool method with input and output schemas. - Enhanced help and guide tools with structured content. - Created unit tests for security utilities, covering valid and invalid origin scenarios. - Updated OAuth flow tests to register a client with the OAuth provider. - Updated dependencies
WalkthroughAdds CORS configuration and a new security module that validates Host and Origin headers, resolves per-request CORS options, and exposes matching helpers. Re-exports the security module from the public lib surface. Replaces per-tool registrations with a unified registerTool API exposing richer metadata, input/output schemas, and structuredContent; updates MCP, SSE, and API-token transports to apply Host/Origin validation and per-request CORS options and reworks routing for early security checks. Adds unit tests for security utilities, updates integration tests to include Host headers, bumps package and manifest versions, and exposes gemini-extension.json via two GET endpoints. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/integration/oauth-flow.test.ts (1)
214-249: Consider cleanup of registered test clients.The test creates a client entry in
OAUTH_KVbut doesn't explicitly clean it up after the test completes. While the KV entry may expire naturally or be cleaned up by test infrastructure, explicitly deleting test data ensures test isolation and prevents potential side effects.Consider adding cleanup in an afterEach or at the end of the test:
await env.OAUTH_KV.put(`oauth_state_${testState}`, JSON.stringify(stateData), { expirationTtl: 600, }); // Now call the callback with valid code and state const response = await SELF.fetch( `http://localhost/auth/callback?code=${testCode}&state=${testState}`, { method: "GET", redirect: "manual" }, ); // ... assertions ... // Verify state was deleted from KV const deletedState = await env.OAUTH_KV.get(`oauth_state_${testState}`); expect(deletedState).toBeNull(); + + // Cleanup: Delete registered test client + await env.OAUTH_KV.delete(`client:${clientId}`); });src/mcp/tools.ts (1)
649-650: Consider omitting raw API response from output.The limits tool includes the raw API response in the text output. While this may be useful for debugging, it could expose internal API details or sensitive information that shouldn't be shown to end users.
Consider removing or making the raw API response optional:
- // Add the raw API response to the output - textOutput += `\nAPI Response:\n${JSON.stringify(limits, null, 2)}\n\n`; - // Format parsed datasrc/index.ts (2)
42-61: Tighten tool schemas and make structuredContent more predictable for clientsThere are a few rough edges around schemas and structuredContent that will make MCP clients’ lives harder:
For
getMeasurement, the success path returnsstructuredContent: { measurement }, but the “not found” path (Lines 77‑84) returns onlycontent. Given the declaredoutputSchema.measurement, MCP clients may reasonably assume structuredContent is always present and shaped according to the schema. Consider always returning structuredContent with a stable shape (e.g.,found: boolean,measurement: object | null, optionalerror), and updateoutputSchemaaccordingly so callers can branch onfoundinstead of parsing the human text.Across
compareLocations,help, andauthStatus,inputSchemais{}andoutputSchemais a plain object whose values areztypes. Depending on howregisterToolis typed, this may mean you’re not getting any runtime validation or type inference (vs.z.object({ ... })). It’s worth double‑checking thatregisterToolactually consumes these as intended; otherwise, consider switching to explicit Zod objects (includingz.object({}).strict()for no‑arg tools) so schemas are unambiguous.In
authStatus,statusis declared asz.string()(Lines 261‑264) but only ever set to"Authenticated"/"Not authenticated"(Lines 268‑275). Narrowing that field toz.enum(["Authenticated", "Not authenticated"])(and keepingmessageas the human‑readable string) would make automated clients less brittle and prevent accidental new status strings from slipping in unnoticed.If you want a more concrete direction, I can propose a diff that standardizes
getMeasurement’s output into a{ found, measurement, error? }shape and switches these schemas toz.object(...)while aligning the handlers.Also applies to: 63-85, 89-146, 149-248, 251-293
366-380: Consider adding Host header validation alongside Origin checks for stronger DNS rebinding defenseYou’re validating
Originfor MCP and API‑token requests (Lines 366‑380 and 406‑418), which is good, but current best practice for DNS rebinding is to combine Origin checks with Host validation:
- An attacker who can influence DNS might still be able to hit the worker through an unexpected hostname while supplying an allowed Origin value.
- Validating that
Host/:authoritymatches an expected hostname (or an allowlist) closes off that class of attack and aligns with the “Origin + Host” recommendation from recent MCP security guidance (Based on learnings).A minimal pattern would be:
const url = new URL(req.url); const expectedHost = url.host; // or derive from config/env const hostHeader = req.headers.get("Host") ?? req.headers.get(":authority") ?? expectedHost; if (hostHeader !== expectedHost /* or not in an allowlist */) { return new Response("Forbidden: Invalid Host", { status: 403 }); }You can factor this into a shared helper alongside
validateOriginso bothhandleMcpRequestandhandleAPITokenRequestapply the same Host policy.Also applies to: 406-418
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.json(1 hunks)src/app.ts(1 hunks)src/config/constants.ts(1 hunks)src/index.ts(9 hunks)src/lib/index.ts(1 hunks)src/lib/security.ts(1 hunks)src/mcp/tools.ts(12 hunks)test/integration/oauth-flow.test.ts(1 hunks)test/unit/lib/security.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.tssrc/app.tstest/integration/oauth-flow.test.tssrc/mcp/tools.tssrc/lib/security.tssrc/config/constants.tstest/unit/lib/security.test.tssrc/index.ts
🧬 Code graph analysis (5)
test/integration/oauth-flow.test.ts (1)
worker-configuration.d.ts (1)
env(7789-7789)
src/mcp/tools.ts (3)
src/index.ts (1)
getToken(337-340)src/api/client.ts (2)
getLocations(118-137)getRateLimits(145-164)src/auth/token-manager.ts (1)
maskToken(87-93)
src/lib/security.ts (1)
src/config/constants.ts (1)
CORS_CONFIG(106-132)
test/unit/lib/security.test.ts (2)
src/lib/security.ts (2)
validateOrigin(31-55)getCorsOptions(67-75)src/config/constants.ts (1)
CORS_CONFIG(106-132)
src/index.ts (2)
src/lib/security.ts (2)
validateOrigin(31-55)getCorsOptions(67-75)src/config/constants.ts (1)
MCP_CONFIG(37-49)
🪛 GitHub Actions: Tests
test/integration/oauth-flow.test.ts
[error] 1-1: SyntaxError: The requested module 'ajv' does not provide an export named 'Ajv'.
🔍 Remote MCP DeepWiki
The repository doesn't have indexed DeepWiki documentation yet. Let me search for best practices and standards related to the technologies and patterns in this PR.Based on the comprehensive research I've conducted, here is relevant context to inform the PR review:
Key Security Best Practices for Origin Validation & DNS Rebinding Protection
When publishing software that holds security critical information or takes privileged actions, developers should check that the origin header matches the host or an allowlist to protect against DNS rebinding attacks. The PR's validateOrigin function implements this principle by returning false for falsy origins and validating against an allowlist.
To make a secure validator for cross site requests, combine exactMatch for second level domains and endsWith for subdomains, and never add the null origin to allowed domains. The test suite appears to cover these edge cases, including null origin handling.
In addition to checking the Origin header, resource authors are strongly encouraged to check the Host header to ensure it matches the server's hostname—this provides protection against DNS rebinding attacks. The review should verify whether Host header validation is also implemented alongside Origin validation.
MCP Tool Registration & Structured Output Standards
Structured content is returned as a JSON object in the structuredContent field of a result; for backwards compatibility, tools returning structured content SHOULD also return the serialized JSON in a TextContent block. The PR updates tools to include structured content alongside textual responses, which aligns with MCP standards.
Tools may provide an output schema for validation of structured results; if provided, servers MUST provide structured results conforming to the schema, and clients SHOULD validate against it. The PR appears to add explicit inputSchema and outputSchema to tools, which supports this requirement.
A comprehensive testing strategy for MCP tools should cover functional, integration, security, and performance testing. The PR includes unit tests for security utilities and OAuth flow tests—additional integration tests could be valuable.
OAuth 2.0 Protected Resource Metadata Endpoint (RFC 9728)
By default, the well-known URI string used for OAuth 2.0 Protected Resource Metadata is /.well-known/oauth-protected-resource. The PR adds this endpoint correctly.
MCP servers MUST implement OAuth 2.0 Protected Resource Metadata (RFC9728), and MCP clients MUST use it for authorization server discovery. MCP servers MUST include the authorization_servers field containing at least one authorization server in the Protected Resource Metadata document. The PR's implementation should be verified against RFC 9728 requirements.
Metadata endpoints must be served over HTTPS to prevent tampering or man-in-the-middle attacks. This is a security requirement for the new endpoint.
Security Considerations for MCP Implementation
Recent research found that nearly all MCP servers exposed to the internet lacked authentication, meaning anyone could access internal tool listings and potentially exfiltrate sensitive data. The Origin validation in this PR helps mitigate this by restricting access to allowed origins.
Security researchers identified multiple outstanding security issues with MCP, including prompt injection, tool permissions where combining tools can exfiltrate files, and lookalike tools that can silently replace trusted ones. The structured content and explicit schema definitions in this PR may help surface tool capabilities more clearly for security review.
Summary of Review Focus Areas
- Origin/Host validation completeness: Verify Host header validation complements Origin validation
- HTTPS requirement: Confirm the
.well-known/oauth-protected-resourceendpoint enforces HTTPS - RFC 9728 compliance: Validate the metadata endpoint content matches requirements (authorization_servers, scopes_supported, etc.)
- Backwards compatibility: Ensure structured content additions include TextContent for compatibility
- Null origin handling: Confirm null origins are explicitly rejected in validation logic
- Test coverage: Assessment of security and integration test coverage for the new features
[::web_search::],,,
🔇 Additional comments (4)
src/lib/security.ts (1)
67-75: No changes needed – comma-separated format is supported by the MCP SDK.The MCP SDK uses standard Node CORS middleware which accepts CORS list fields (including origin) as either an array or comma-separated string. The current implementation returning
CORS_CONFIG.ALLOWED_ORIGINS.join(",")is valid and fully compatible with the transport layer.package.json (2)
23-23: Review comment verified: No issues found.The WebCrypto APIs used in your code (
crypto.getRandomValues()andcrypto.subtle.digest()) are stable and fully compatible with @types/node v24, which provides access to the Web Crypto API through globalThis.crypto or require('node:crypto').webcrypto. The @types/node major version must match your environment's Node.js major version, so v24.10.1 is appropriate for Node.js v24. The breaking changes between Node.js 22 and 24 affected other modules but not the WebCrypto subsystem you're using. The version jump from v22 to v24 is standard: v23 was a temporary "Current" release cycle before v24 became the next major version.
26-26: Marked is a devDependency and not used in the codebase—breaking API changes do not affect this project.While marked v16 requires Node.js 20.0.0+ and v17 includes list rendering API changes and checkbox token modifications, the codebase does not import or use the marked library. Since marked appears only in
devDependenciesand is not referenced in the application code, these breaking changes have no impact on the project's compatibility or functionality.Likely an incorrect or invalid review comment.
test/integration/oauth-flow.test.ts (1)
1-1: Review comment is incorrect and should be dismissed.The review claims a SyntaxError related to ajv imports at line 1 of the test file, but line 1 contains only a JSDoc comment start (
/**), and no problematic ajv imports exist anywhere in the codebase. The error described would only occur if code attempted to useimport { Ajv } from 'ajv', which is not present. There is no evidence of the claimed pipeline failure.Likely an incorrect or invalid review comment.
- Updated dependencies in package.json for @modelcontextprotocol/sdk and agents. - Refactored app.ts to include Host header validation alongside Origin validation to prevent DNS rebinding attacks. - Implemented validateHost function in security.ts to check allowed hostnames and prevent malicious requests. - Updated handleMcpRequest and handleAPITokenRequest functions to validate Host header. - Added integration tests for Host header validation in mcp-tools.test.ts and oauth-flow.test.ts. - Expanded unit tests for validateHost function in security.test.ts to cover various scenarios and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/integration/mcp-tools.test.ts (1)
159-171: Consider adding negative test cases for Host/Origin validation.The integration tests verify the happy path with valid
Host: "localhost"headers, but don't test the security behavior when invalid headers are provided. Consider adding test cases that verify 403 Forbidden responses are returned for:
- Missing Host header
- Invalid Host header (e.g.,
Host: "evil.com")- Invalid Origin header (e.g.,
Origin: "https://attacker.com")This would provide end-to-end verification that the DNS rebinding protection works as intended.
test/integration/oauth-flow.test.ts (1)
246-322: Clean up registered test client from KV store.The test registers a client in KV (line 257) but doesn't clean it up afterward. While the state is deleted (line 320), the client registration persists. Consider adding cleanup in an
afterEachor at the end of the test:// After line 322 // Clean up registered client await env.OAUTH_KV.delete(`client:${clientId}`);src/index.ts (1)
366-393: Consider extracting duplicate validation logic.The Host and Origin validation code (lines 366-393 and 420-447) is duplicated between
handleMcpRequestandhandleAPITokenRequest. Consider extracting to a helper function:function validateSecurityHeaders(req: Request): Response | null { const host = req.headers.get("Host"); if (!validateHost(host)) { console.error(`[Security] Rejected request with invalid Host header: ${host}`); return new Response("Forbidden: Invalid Host", { status: 403, headers: { "Content-Type": "text/plain" }, }); } const origin = req.headers.get("Origin"); if (origin && !validateOrigin(origin)) { console.error(`[Security] Rejected request with invalid Origin header: ${origin}`); return new Response("Forbidden: Invalid Origin", { status: 403, headers: { "Content-Type": "text/plain" }, }); } return null; // validation passed }Then use it in both handlers:
const securityError = validateSecurityHeaders(req); if (securityError) return securityError;Also applies to: 420-447
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(1 hunks)src/app.ts(2 hunks)src/index.ts(9 hunks)src/lib/security.ts(1 hunks)test/integration/mcp-tools.test.ts(3 hunks)test/integration/oauth-flow.test.ts(13 hunks)test/unit/lib/security.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/app.ts
🧰 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:
test/integration/mcp-tools.test.tssrc/index.tstest/unit/lib/security.test.tstest/integration/oauth-flow.test.tssrc/lib/security.ts
🧬 Code graph analysis (4)
src/index.ts (2)
src/lib/security.ts (3)
validateHost(57-68)validateOrigin(93-117)getCorsOptions(129-137)src/config/constants.ts (1)
MCP_CONFIG(37-49)
test/unit/lib/security.test.ts (2)
src/lib/security.ts (3)
validateOrigin(93-117)getCorsOptions(129-137)validateHost(57-68)src/config/constants.ts (1)
CORS_CONFIG(106-132)
test/integration/oauth-flow.test.ts (1)
worker-configuration.d.ts (1)
env(7789-7789)
src/lib/security.ts (1)
src/config/constants.ts (1)
CORS_CONFIG(106-132)
🔍 Remote MCP DeepWiki
Summary of additional documentation/context found via DeepWiki
- DeepWiki repository wiki not found / not indexed for jsdelivr/globalping-mcp-server. No documentation, best-practice pages, or excerpts were retrievable from DeepWiki for this repo.,,
Suggested focus areas for the PR review (based on repo-docs absence and PR changes)
- Confirm Host header validation is exercised everywhere Origin validation is applied (SSE/MCP and API token paths).
- Verify /.well-known/oauth-protected-resource enforces HTTPS and RFC 9728 fields (authorization_servers, scopes_supported, resource_documentation).
- Ensure structuredContent matches the declared outputSchema for each tool and textual responses remain for backwards compatibility.
- Check unit/integration tests cover DNS rebinding, null Origin handling, and Host header port-stripping edge cases.
🔇 Additional comments (3)
test/unit/lib/security.test.ts (1)
1-319: LGTM!Comprehensive test coverage for the security utilities with proper attention to edge cases and DNS rebinding attack vectors.
src/lib/security.ts (1)
57-117: LGTM!The validation logic correctly implements DNS rebinding protection with proper normalization and whitelist checking.
src/index.ts (1)
42-293: LGTM!The tool registration refactor with schemas and structured content improves API clarity while maintaining backward compatibility.
…hostnames in development
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/security.ts (1)
17-31: Consider filtering out empty hostnames from protocol-only schemes.Protocol-only entries like
"vscode://"and"claude://"inALLOWED_ORIGINSmay parse to URLs with empty hostnames (url.hostname === ""), which would then be added to thehostnamesset. WhilevalidateHostsafely rejects empty/falsy hosts, adding empty strings to the set is semantically odd.Consider applying this check after line 22:
if (origin.includes("://")) { const url = new URL(origin); - hostnames.add(url.hostname.toLowerCase()); + if (url.hostname) { + hostnames.add(url.hostname.toLowerCase()); + } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/security.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/security.ts
🧬 Code graph analysis (1)
src/lib/security.ts (1)
src/config/constants.ts (1)
CORS_CONFIG(106-132)
🔍 Remote MCP DeepWiki
Summary of additional context found relevant to the PR review:
- DeepWiki repository wiki not found / not indexed for this repo; no wiki content available to consult.
Relevant notes drawn from the PR context and suggested review focus (no further DeepWiki docs available):
- Ensure Host header validation is applied to all paths where Origin validation is used (SSE/MCP and API token routes).
- Confirm the /.well-known/oauth-protected-resource endpoint enforces HTTPS and returns RFC 9728 fields: authorization_servers, scopes_supported, resource_documentation.
- Verify structuredContent payloads conform to each tool's declared outputSchema and that textual content remains for backwards compatibility.
- Check unit and integration tests exercise DNS rebinding defenses, null/empty Origin handling, and Host header port-stripping/casing edge cases.
…rovider - fix port-stripping logic
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app.ts(1 hunks)src/index.ts(9 hunks)src/lib/security.ts(1 hunks)test/unit/lib/security.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/lib/security.test.ts
🧰 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/index.tssrc/app.tssrc/lib/security.ts
🧬 Code graph analysis (2)
src/index.ts (2)
src/lib/security.ts (3)
validateHost(53-64)validateOrigin(89-119)getCorsOptionsForRequest(156-169)src/config/constants.ts (1)
MCP_CONFIG(37-49)
src/lib/security.ts (1)
src/config/constants.ts (1)
CORS_CONFIG(106-132)
🔍 Remote MCP DeepWiki
Summary of additional context relevant to reviewing PR #24 (sources listed below):
Key places to check in this PR
- Constants & config: verify new CORS_CONFIG aligns with existing GLOBALPING/OAUTH/MCP constants and routes in src/config/constants.ts and wrangler.jsonc (Durable Object & OAUTH_KV bindings) — worker env uses GLOBALPING_CLIENT_ID, globalping_mcp_object, OAUTH_KV.,
- Authentication routing: main fetch handler prioritizes API token path (isAPITokenRequest → handleAPITokenRequest) else OAuthProvider.fetch; ensure Host/Origin validation added in both API-token and OAuth paths so token-based requests and MCP/SSE transport both enforce Host/Origin.
- Token management & sanitization: sanitizeToken / isValidAPIToken used when creating ctx.props for API-token requests — confirm Host/Origin checks do not bypass token validation.
- MCP transport wiring: handleMcpRequest routes /sse and /mcp to GlobalpingMCP.serveSSE / serve — confirm cors options from new security module are applied for both SSE and streamable HTTP flows.
- GlobalpingMCP state/OAuth methods: setOAuthState/getOAuthState/removeOAuthData persist to DO state and OAUTH_KV; ensure new Origin checks don't break OAuth flow (authorize/callback, PKCE, KV state lifecycle).
- Tool registration & structured output: tools are registered in init() and via registerGlobalpingTools; PR changes to registerTool/registerTool metadata must keep backward-compatible textual content + new structuredContent matching declared outputSchema. Verify outputSchema/structuredContent shape for getMeasurement, compareLocations, help, authStatus and for core tools.
- Tests added/modified: unit tests for security utilities (origin/host/CORS resolution) and integration tests updated to include Host headers and dynamic OAuth client registration. Ensure new tests cover localhost variants, port stripping, and DNS rebinding cases referenced by PR.
- API client behavior on auth failures: API client sets agent.setIsAuthenticated(false) on 401/403; confirm Origin/Host validation returns appropriate HTTP 403 where intended (not 401) and that auth state handling remains consistent.
- Deployment/infra: wrangler.jsonc defines Durable Object and OAUTH_KV; confirm any new CORS behavior is compatible with smart placement and Observability settings.
Minimal concrete review checklist
- Confirm src/lib/security.ts exports used (getCorsOptions/getCorsOptionsForRequest/getMatchingOrigin/validateHost/validateOrigin) and that handleMcpRequest and handleAPITokenRequest call validateHost/validateOrigin where request has Host/Origin headers.
- Confirm MCP and SSE transports receive and forward per-request cors options (getCorsOptionsForRequest) and responses include Access-Control-Allow-Origin per getMatchingOrigin logic.
- Verify unit tests in test/unit/lib/security.test.ts cover:
- exact whitelist matches
- localhost/127.0.0.1 and IPv6 ::1 with ports
- DNS rebinding/hostname edge cases
- getCorsOptionsForRequest wildcard fallback behavior
- Verify integration tests (test/integration/oauth-flow.test.ts, mcp-tools.test.ts) include Host header, and OAuth KV client registration changes still pass callback flow (state storage, deletion).
- Validate tool structuredContent conforms to declared inputSchema/outputSchema and retains legacy content text blocks for backwards compatibility.
- Check package.json dependency bumps (hono, @cloudflare/workers-oauth-provider, types) and ensure TypeScript/biome/vitest configs remain compatible.
Sources used
- DeepWiki repo structure and pages (full documentation pages) — DeepWiki_read_wiki_structure, DeepWiki_read_wiki_contents,
🔇 Additional comments (1)
src/lib/security.ts (1)
134-142: Clarify the intended use case forgetCorsOptions()—it's exported API but not used internally.The function is exported as part of the public API and has comprehensive test coverage validating that
originis an array. However, it's not used anywhere in the production codebase—onlygetCorsOptionsForRequest()is called internally (4 call sites insrc/index.ts).This suggests
getCorsOptions()may serve a different purpose than the MCP SDK integration (which uses the request-specific variant). Either document its intended use case for external consumers, or if it's not needed, remove it along with its test suite.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/config/constants.ts(1 hunks)src/lib/security.ts(1 hunks)test/unit/lib/security.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/constants.ts
- src/lib/security.ts
🧰 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:
test/unit/lib/security.test.ts
🧬 Code graph analysis (1)
test/unit/lib/security.test.ts (2)
src/lib/security.ts (5)
validateOrigin(104-134)getCorsOptions(149-157)getMatchingOrigin(198-224)getCorsOptionsForRequest(171-184)validateHost(54-79)src/config/constants.ts (1)
CORS_CONFIG(106-135)
🔍 Remote MCP DeepWiki
Summary of additional repo documentation and relevant facts for reviewing PR #24
High-level sources used:
- DeepWiki repository structure and pages: , full docs content:
Key facts directly relevant to this PR (concise, actionable)
-
Where CORS/Origin/Host rules belong and how they're applied
- Constants and routing config live in src/config/constants.ts and wrangler.jsonc; CORS/global origin rules are centralized in constants used by request handlers and OAuth provider.
- OAuth endpoints and MCP routes (/authorize, /auth/callback, /sse, /mcp) are routed in src/index.ts via OAuthProvider and handleMcpRequest — origin/host validation must be enforced in both API token and OAuth paths.
-
Security utilities integration points
- OAuth flow (src/app.ts) relies on KV (OAUTH_KV) for PKCE/state; Durable Object GlobalpingMCP provides set/get OAuth state and removeOAuthData. validateOrigin/validateHost should not break KV/DurableObject flows.
- The OAuthProvider wiring in main fetch checks isAPITokenRequest first then uses OAuthProvider.fetch; changes must preserve that decision order.
-
MCP Durable Object and per-request props usage
- GlobalpingMCP Durable Object exposes getToolContext(), getToken(), setIsAuthenticated(), setOAuthState/getOAuthState/removeOAuthData and stores measurement cache in state.measurements. Any CORS/origin behavior must still allow ctx.props to be set for API-token requests and be accessible to tools.
-
Tool registration/structured output changes
- Tools are registered in init(): registerGlobalpingTools(this, tokenProvider) plus inline registrations for getMeasurement, compareLocations, help, authStatus. The PR’s change to use registerTool/registerTool-like metadata and structuredContent must remain compatible with existing tool names and the getToolContext token-provider pattern.
-
Transport/CORS interaction points to verify in review
- handleMcpRequest routes /sse and /mcp to GlobalpingMCP.serveSSE / serve — these are the exact places to ensure getCorsOptionsForRequest(req) is applied for both SSE and streamable-HTTP transports (both API-token and OAuth paths).
- API-token path (handleAPITokenRequest) constructs ctx.props and then calls agent.serve/serveSSE — confirm Host/Origin checks are applied before props are set or before serve called to avoid bypass.
-
Tests and expected coverage relevant to PR
- There are existing unit tests for URL validation and a dedicated test/unit/lib/security.test.ts described (covers validateOrigin/validateHost/getCorsOptions/getMatchingOrigin/getCorsOptionsForRequest with many edge cases: localhost variants, ports, DNS rebinding, homograph, port stripping). Ensure new security code aligns with these tests and PR's tests run against same cases.
- Integration tests (test/integration/oauth-flow.test.ts and test/integration/mcp-tools.test.ts) already include Host header adjustments; verify PR preserves OAuth state lifecycle and that added origin/host checks in fetch paths don't break these integration flows.
-
Deployment and env implications
- wrangler.jsonc binds Durable Object globalping_mcp_object and OAUTH_KV; CORS changes must be compatible with smart placement and Cloudflare edge behavior. Secrets and KV binding names used by OAuth flows remain unchanged.
-
Dependency/compat checks to consider
- package.json updates include workers & TypeScript bumps; ensure nodejs_compat and MCP SDK expectations (McpServer.registerTool API vs previous server.tool) are consistent with the refactor.
Files / locations reviewers should inspect (explicit)
- src/lib/security.ts (new exports: validateHost, validateOrigin, getCorsOptions, getCorsOptionsForRequest, getMatchingOrigin)
- src/config/constants.ts (CORS_ALLOWED origins, EXCEPTION_HOSTS / STANDARD_PROTOCOLS)
- src/index.ts: fetch handler, handleMcpRequest, handleAPITokenRequest, where Host/Origin checks and corsOptions must be applied
- src/mcp/tools.ts and registerGlobalpingTools (tool registration / token provider usage)
- src/lib/index.ts export changes (re-export security)
- test/unit/lib/security.test.ts and test/integration/oauth-flow.test.ts, test/integration/mcp-tools.test.ts (new/updated tests)
- wrangler.jsonc (Durable Object & KV bindings)
Concrete review checklist items (map to repo docs above)
- Confirm validateHost/validateOrigin are used in both handleMcpRequest and handleAPITokenRequest paths (no bypass).
- Confirm per-request CORS options (getCorsOptionsForRequest) are passed into both MCP and SSE transports in both OAuth and API-token flows.
- Confirm Host header validation occurs before any authenticated ctx.props are trusted (so invalid Host returns 403 and does not allow prop injection).
- Verify unit tests in test/unit/lib/security.test.ts cover the same origin/host variants asserted in docs (localhost IPv4/IPv6/ports, exception hosts, magic client schemes) and integration tests include Host headers and dynamic OAuth client registration.
- Confirm tool registration metadata (registerTool/registerTool signature) preserves existing tool names/handler contracts and that token provider still returns sanitizeToken(this.getToken()).
- Check package dependency bumps (hono, workers-oauth-provider, @types/node, typescript) do not conflict with nodejs_compat and MCP SDK usage per wrangler.jsonc settings.
…xes in Host validation
- Removed `workers-mcp` from devDependencies. - Updated `@modelcontextprotocol/sdk` from `^1.18.2` to `^1.20.2`. - Updated `agents` from `^0.2.6` to `^0.2.21`.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
README.md (1)
49-113: Normalize heading levels for client integration sectionsJumping from the
## Installationheading directly to#### Gemini CLI(Line 49) violates MD001 and makes the Gemini/Claude/Cursor sections look hierarchically nested when they’re really peers.Consider promoting all four tool headings to H3 so they’re siblings under Installation:
-#### Gemini CLI +### Gemini CLI ... -#### Claude Desktop App +### Claude Desktop App ... -#### Anthropic Claude API (via Console) +### Anthropic Claude API (via Console) ... -#### Cursor +### CursorThis keeps the structure consistent and resolves the markdownlint warning.
gemini-extension.json (1)
3-12: Minimize drift between manifest instructions and server/README docsThe
instructionstext here now overlaps heavily with guidance insrc/index.ts(McpServerinstructions) and the README. Keeping three copies in sync will be error‑prone over time.Consider either:
- Moving the shared instructions into a single constant (e.g., in a config module) and importing it both here and in
src/index.ts, or- Adding a brief comment in both places noting that they must be kept in sync with the README.
This will make future updates to usage guidance less likely to diverge.
src/app.ts (1)
10-59: Add caching headers for static Gemini extension manifestThe Gemini extension manifest is static content, but the new routes at
/gemini-extension.jsonand/.well-known/gemini-extension.jsondon’t send any caching headers. That can cause unnecessary repeated fetches from clients or intermediaries.Consider something like:
-app.get("/gemini-extension.json", (c) => { - return c.json(geminiExtension); -}); +app.get("/gemini-extension.json", (c) => { + c.header("Cache-Control", "public, max-age=3600"); + return c.json(geminiExtension); +}); -app.get("/.well-known/gemini-extension.json", (c) => { - return c.json(geminiExtension); -}); +app.get("/.well-known/gemini-extension.json", (c) => { + c.header("Cache-Control", "public, max-age=3600"); + return c.json(geminiExtension); +});Adjust the TTL as appropriate for how often you expect the manifest to change.
src/index.ts (2)
19-37: Avoid duplicating user instructions across MCP server, manifest, and READMEThe
instructionsblock given toMcpServerlargely duplicates the guidance ingemini-extension.jsonand in the README. Maintaining three separate copies will make it easy for them to diverge (e.g., when adding/removing tools or changing recommended usage).Consider moving this text into a shared constant (e.g., in
src/config/constants.ts) and reusing it both here and in the extension manifest, or at least adding a short comment noting that all locations must be kept in sync.
386-468: Deduplicate Host/Origin validation between MCP and API‑token handlers
handleMcpRequest(Lines 386–413) andhandleAPITokenRequest(Lines 440–467) both implement nearly identical Host and Origin validation logic and 403 error responses.To reduce the risk of subtle divergence over time (e.g., one path gaining extra logging or behavior), it would be cleaner to extract this into a shared helper, for example:
function validateRequestHeaders(req: Request, contextLabel: string): Response | null { const host = req.headers.get("Host"); if (!validateHost(host)) { console.error(`[Security] Rejected ${contextLabel} request with invalid Host header: ${host}`); return new Response("Forbidden: Invalid Host", { status: 403, headers: { "Content-Type": "text/plain" }, }); } const origin = req.headers.get("Origin"); if (origin && !validateOrigin(origin)) { console.error(`[Security] Rejected ${contextLabel} request with invalid Origin header: ${origin}`); return new Response("Forbidden: Invalid Origin", { status: 403, headers: { "Content-Type": "text/plain" }, }); } return null; }And then call it at the top of both handlers:
const headerError = validateRequestHeaders(req, "MCP"); if (headerError) return headerError;This keeps the security checks consistent and easier to evolve.
src/mcp/tools.ts (2)
10-30: Clarify error result shape fromhandleToolExecutionvs declaredoutputSchema
handleToolExecutionreturns a custom{ content, isError: true }object when an exception is thrown, but all tools declare concreteoutputSchemafields (e.g.,measurementId,rateLimit, etc.). That means error responses won’t conform to the declared output schema.If the MCP SDK/tooling enforces
outputSchemaat runtime, this mismatch could be confusing for clients that assume outputs always match the schema.Two options to tighten this up:
- Either extend the declared output schemas to model a discriminated union like
{ error?: string; ... }, and populate that in the error path, or- Let exceptions bubble up so the MCP layer’s built‑in error handling kicks in, instead of returning an ad‑hoc
{ isError: true }payload.Without that, clients relying strictly on
outputSchemamay have to special‑case error handling.Check how
registerToolandoutputSchemaare used in your current MCP SDK version to confirm whether it expects all responses to validate against the schema or allows an out‑of‑band error shape.
549-727: Consider trimming or structuring verbose text inlocationsandlimitsoutputsBoth
locationsandlimitstools now return:
- A large human‑readable
textOutputstring, and- A well‑structured
structuredContentobject.For clients that primarily consume
structuredContent, the long text blocks (including full JSON dumps and potentially very long location lists) may be unnecessary noise and increase token/response size.You might consider:
- Keeping the detailed data only in
structuredContentand shortening the textual summary, or- Making the verbose sections conditional (e.g., only when a flag is set), if your client ecosystem sometimes uses only
content.This isn’t a correctness issue, but tightening the text output can improve UX and reduce bandwidth for MCP clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)gemini-extension.json(1 hunks)package.json(2 hunks)src/app.ts(2 hunks)src/index.ts(11 hunks)src/mcp/helpers.ts(4 hunks)src/mcp/tools.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/app.tssrc/mcp/tools.tssrc/mcp/helpers.tssrc/index.ts
🧬 Code graph analysis (2)
src/mcp/tools.ts (4)
src/index.ts (2)
GlobalpingMCP(13-378)getToken(357-360)src/mcp/helpers.ts (2)
parseLocations(10-18)formatMeasurementSummary(93-284)src/api/client.ts (3)
runMeasurement(89-110)getLocations(118-137)getRateLimits(145-164)src/auth/token-manager.ts (1)
maskToken(87-93)
src/index.ts (4)
src/types/globalping.ts (1)
GlobalpingEnv(23-27)src/types/app.ts (2)
State(11-16)Props(1-8)src/config/constants.ts (1)
MCP_CONFIG(37-49)src/lib/security.ts (3)
validateHost(54-89)validateOrigin(114-145)getCorsOptionsForRequest(182-195)
🪛 GitHub Actions: Tests
src/mcp/tools.ts
[error] 285-285: src/mcp/tools.ts(285,12): error TS2339: Property 'target' does not exist on type '{ trace?: boolean | undefined; locations?: string | string[] | undefined; limit?: number | undefined; queryType?: "MX" | "A" | "AAAA" | "ANY" | "CNAME" | "DNSKEY" | "DS" | "HTTPS" | "NS" | ... 7 more ... | undefined; resolver?: string | undefined; }'.
🪛 GitHub Check: test
src/mcp/tools.ts
[failure] 383-383:
Property 'target' does not exist on type '{ locations?: string | string[] | undefined; limit?: number | undefined; packets?: number | undefined; protocol?: "ICMP" | "TCP" | "UDP" | undefined; port?: number | undefined; }'.
[failure] 285-285:
Property 'target' does not exist on type '{ trace?: boolean | undefined; locations?: string | string[] | undefined; limit?: number | undefined; queryType?: "MX" | "A" | "AAAA" | "ANY" | "CNAME" | "DNSKEY" | "DS" | "HTTPS" | "NS" | ... 7 more ... | undefined; resolver?: string | undefined; }'.
🪛 markdownlint-cli2 (0.18.1)
README.md
49-49: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🔍 Remote MCP DeepWiki
Summary of additional repository context relevant to reviewing PR #24
-
Security/CORS surface
- CORS and allowed-origins are defined centrally in src/config/constants.ts and used by URL/host/origin validators; standard protocols and EXCEPTION_HOSTS are defined there.
- The repo includes URL/Origin/Host validation utilities (getAllowedHostnames / validateHost / validateOrigin / getMatchingOrigin / getCorsOptionsForRequest) and unit tests covering many cases (localhost, IPv6, exact whitelist, MCP client schemes). See Security / URL validation docs and test descriptions.
-
Request routing & where validation must happen
- fetch() routes: isAPITokenRequest → handleAPITokenRequest (sets ctx.props) else OAuthProvider.fetch; MCP endpoints (/sse, /mcp) routed to handleMcpRequest which calls GlobalpingMCP.serve()/serveSSE(). Ensure Host/Origin validation is applied at this entrypoint before ctx.props is set for API-token flows to avoid bypass.
- handleAPITokenRequest builds ctx.props and then calls agent.serve/serveSSE; OAuthProvider is configured with apiHandler: handleMcpRequest.
-
Durable Object (GlobalpingMCP) responsibilities
- GlobalpingMCP.init() registers tools (registerGlobalpingTools + direct tools), exposes getToolContext/getToken/setOAuthState/getOAuthState/removeOAuthData and caches measurements in state.measurements. Token provider passed into registerGlobalpingTools sanitizes tokens. Changes to registration API (this.server.tool → this.server.registerTool) must preserve this behavior.
-
Tool registration & structured metadata
- Tools are registered during Durable Object init; core tools are registered via registerGlobalpingTools(this, getToken) and utility tools (getMeasurement, compareLocations, help, authStatus) are registered inline. New registerTool API must keep token provider, input/output schemas, and structuredContent behavior for clients.
-
Tests exercising changes
- New/updated tests: unit tests for security utilities (extensive origin/host/CORS cases) and integration tests updating Host header handling and dynamic OAuth client registration in KV. CI should run both unit and workers integration tests.
-
OAuth / KV / Durable Object bindings
- OAuth flow stores PKCE/state in OAUTH_KV with 10-minute TTL; GlobalpingMCP.removeOAuthData cleans grant: and token: keys from OAUTH_KV. Wrangler bindings: durable object globalping_mcp_object and OAUTH_KV configured in wrangler.jsonc. Ensure origin/host checks align with deployment domain (mcp.globalping.dev) and tests that set Host headers.
Files/areas to verify in PR
- src/lib/security.ts: correctness of getMatchingOrigin, validateOrigin, validateHost and behavior when Origin header is absent (should produce "*").
- src/config/constants.ts: ALLOWED_ORIGINS, STANDARD_PROTOCOLS, EXCEPTION_HOSTS used by validators.
- src/index.ts: confirm Host header validation occurs before ctx.props is set in API-token path and that corsOptions are passed to serve()/serveSSE().
- src/mcp/tools.ts and registerGlobalpingTools usage: ensure registerTool signature change preserves token-provider behavior and structuredContent/output schemas.
- test/unit/lib/security.test.ts & test/integration/*: ensure tests exercise Host+Origin+CORS behavior and updated tests include Host header values matching ALLOWED_ORIGINS.
Sources used
- DeepWiki repository structure and docs listing (architecture, authentication, MCP server, config, tests)
- DeepWiki full contents pages (Architecture, Authentication, MCP Server, OAuth flow, Token Management, Tool Registration, Security/URL validation, Testing, Wrangler config)
🔇 Additional comments (1)
package.json (1)
20-39: Verify compatibility of upgraded runtime and tooling dependenciesSeveral dependencies here have nontrivial version bumps (
@cloudflare/workers-oauth-provider,@modelcontextprotocol/sdk,agents,hono,marked, TypeScript, Node types). It’s easy for subtle behavior changes (OAuth flows, MCP server behavior, markdown rendering) to slip in.Please double‑check:
- That all existing integration and workers tests cover these paths and pass after the upgrade.
- That any
markedconsumers still render as expected (no API/signature changes being relied on).- That the new OAuth provider and MCP SDK versions don’t require additional config beyond what’s already in the repo.
If helpful, you can skim each library’s changelog for the versions you’re jumping across and confirm nothing breaking is being ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mcp/tools.ts (1)
10-30: Consider logging errors for observability.The error handler silently swallows exceptions. Adding a console.error or structured log would help diagnose production issues.
async function handleToolExecution( operation: () => Promise<any>, errorMessagePrefix: string ) { try { return await operation(); } catch (error: any) { + console.error(`${errorMessagePrefix}:`, error); return { content: [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp/tools.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/mcp/tools.ts
🧬 Code graph analysis (1)
src/mcp/tools.ts (4)
src/index.ts (2)
GlobalpingMCP(13-378)getToken(357-360)src/mcp/helpers.ts (2)
parseLocations(10-18)formatMeasurementSummary(93-284)src/api/client.ts (3)
runMeasurement(89-110)getLocations(118-137)getRateLimits(145-164)src/auth/token-manager.ts (1)
maskToken(87-93)
🔍 Remote MCP DeepWiki
Relevant additional facts from repository docs (useful for reviewing PR #24):
-
Docs confirm centralized CORS/origin/host/security config lives under src/config/constants.ts and URL/host/origin validation + helpers are documented in Security/URL validation pages — verify CORS_CONFIG changes align with these constants and docs.,
-
Entrypoint and routing: fetch() in src/index.ts routes API-token requests (isAPITokenRequest → handleAPITokenRequest) before delegating to OAuthProvider; MCP transports (/sse, /mcp) are routed to GlobalpingMCP. Ensure Host validation is applied at this entrypoint for API-token and OAuth flows.
-
GlobalpingMCP Durable Object:
- init() registers tools (registerGlobalpingTools + inline tools: getMeasurement, compareLocations, help, authStatus). Confirm registerTool API change preserves token-provider behavior.
- Durable Object binding name: globalping_mcp_object (wrangler.jsonc). Confirm code uses same binding constant.
-
OAuth/KV details:
- OAuth flow stores StateData in OAUTH_KV with 10min TTL using key oauth_state_{state}; callback deletes state (one-time use). Tests and app.ts reflect this — check test updates that register dynamic client in KV.
-
URL trust rules and tests:
- isTrustedRedirectUri composed of isLocalhost, isDeepLink, isExceptionHost; EXCEPTION_HOSTS list documented (playground.ai.cloudflare.com, mcp.docker.com, etc.). Unit tests cover many origin/host cases — validate new validateOrigin/getMatchingOrigin behavior against these rules.
-
API client & token handling:
- src/api/client.ts enforces validateToken, sets agent.setIsAuthenticated(false) on 401/403; registerGlobalpingTools uses token getter sanitizeToken(this.getToken()) — ensure new CORS/Origin checks don't prevent token propagation.
-
Testing & CI notes:
- Unit tests (test/unit/) include security tests for origin/host/CORS; integration tests updated to include Host headers and dynamic OAuth client registration. CI uses Vitest and workers pool — ensure tests pass with the changes.
-
Deployment/config:
- wrangler.jsonc shows durable object binding, OAUTH_KV binding, routes (mcp.globalping.dev), nodejs_compat flag — verify new Host/Origin validations align with configured domain and any exception hosts used by clients (Claude/Cursor/Gemini).
Sources consulted: DeepWiki repository docs and structure pages.,
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/mcp/helpers.ts (1)
138-141: Guard against non-numericlossbefore calling.toFixed(2)The check
testResult.stats.loss !== undefineddoesn't guard againstnullor non-numeric values, which will cause.toFixed(2)to throw at runtime.Apply this diff to align with the defensive checks used for min/avg/max:
-const loss = - testResult.stats.loss !== undefined - ? testResult.stats.loss.toFixed(2) - : "N/A"; +const rawLoss = testResult.stats.loss; +const loss = + typeof rawLoss === "number" ? rawLoss.toFixed(2) : "N/A";src/mcp/tools.ts (1)
553-560: Fix typo in Locations tool description textThe Locations tool description still says “instead simplify provide the location…”, which reads awkwardly.
- "Retrieve the list of available Globalping probe locations. Use this tool to find specific countries, cities, or ASNs to use as the 'locations' argument in other measurement tools. Avoid using this tool unless absolutely necessary, instead simplify provide the location you need to the tools above, the field is smart and will auto select the right probes.", + "Retrieve the list of available Globalping probe locations. Use this tool to find specific countries, cities, or ASNs to use as the 'locations' argument in other measurement tools. Avoid using this tool unless absolutely necessary, instead simply provide the location you need to the tools above, the field is smart and will auto select the right probes.",
🧹 Nitpick comments (4)
src/app.ts (1)
52-59: Add origin validation and CORS headers to Gemini extension manifest endpointsThese public metadata endpoints serve client configuration but lack origin validation and CORS headers. Browser-based MCP clients (like the Gemini extension) may need cross-origin access to fetch this manifest.
Apply the same security pattern used for other MCP endpoints (validateOrigin + getCorsOptions) to ensure only allowed origins can access the manifest and that proper CORS headers are returned.
Consider applying this pattern:
app.get("/gemini-extension.json", (c) => { const origin = c.req.header("Origin"); if (origin && !validateOrigin(origin)) { return c.text("Forbidden", 403); } const response = c.json(geminiExtension); if (origin) { const corsOptions = getCorsOptionsForRequest(c.req.raw); response.headers.set("Access-Control-Allow-Origin", corsOptions.origin); response.headers.set("Access-Control-Allow-Methods", corsOptions.methods.join(", ")); response.headers.set("Access-Control-Allow-Headers", corsOptions.headers.join(", ")); } return response; });Apply the same pattern to the
/.well-known/gemini-extension.jsonendpoint.test/unit/lib/security.test.ts (1)
430-467: Add coverage for malformed IPv6 hosts with junk after]Current invalid IPv6 tests cover missing brackets and incomplete forms, but not cases like
"[::1]evil.com"or"[::1]foo:443". With the current implementation, these are easy to accidentally treat as valid localhost once you only look up to the closing bracket.To lock in stricter behavior and support the tightened parsing in
validateHost, extend the “invalid hosts”/“malformed IPv6 addresses” section with explicit cases such as:it("should reject malformed IPv6 addresses", () => { // Missing brackets, incomplete addresses, etc. expect(validateHost("::1")).toBe(false); expect(validateHost("::1:8080")).toBe(false); expect(validateHost("[::1")).toBe(false); expect(validateHost("::1]")).toBe(false); expect(validateHost("[:1]")).toBe(false); + // Garbage after closing bracket should not be accepted + expect(validateHost("[::1]evil.com")).toBe(false); + expect(validateHost("[::1]foo:443")).toBe(false); });This will keep Host validation robust against non-RFC IPv6 host syntaxes and ensure tests match the stricter normalization logic.
src/index.ts (2)
61-105: MakegetMeasurement’s structured output consistent with its schema
outputSchemaalways declares ameasurementobject, but in the cache-miss path you return onlycontentwith nostructuredContent. That makes it harder for clients to rely on the schema shape.Consider always returning
structuredContent, e.g. with a nullable or unknown measurement:- outputSchema: { - measurement: z.object({}), - }, + outputSchema: { + measurement: z.unknown().nullable(), + }, @@ - if (this.state.measurements[id]) { - const measurement = this.state.measurements[id]; + if (this.state.measurements[id]) { + const measurement = this.state.measurements[id]; return { content: [ @@ - structuredContent: { measurement }, + structuredContent: { measurement }, }; } - return { - content: [ + return { + content: [ { type: "text", text: "Measurement not found in cache. Use one of the measurement tools (ping, traceroute, dns, mtr, http) to generate a new measurement.", }, - ], + ], + structuredContent: { measurement: null }, };This keeps the contract predictable for downstream consumers parsing
structuredContent.
386-413: Deduplicate Host/Origin validation between MCP and API-token paths
handleMcpRequestandhandleAPITokenRequesteach reimplement the same Host and Origin validation logic and 403 responses. Keeping these in sync long term is brittle.Consider extracting a small helper, e.g.:
function enforceRequestSecurity(req: Request, contextLabel: string): Response | null { const host = req.headers.get("Host"); if (!validateHost(host)) { console.error(`[Security] Rejected ${contextLabel} request with invalid Host header: ${host}`); return new Response("Forbidden: Invalid Host", { status: 403, headers: { "Content-Type": "text/plain" } }); } const origin = req.headers.get("Origin"); if (origin && !validateOrigin(origin)) { console.error(`[Security] Rejected ${contextLabel} request with invalid Origin header: ${origin}`); return new Response("Forbidden: Invalid Origin", { status: 403, headers: { "Content-Type": "text/plain" } }); } return null; }and calling it from both handlers before branching on
pathname. That reduces duplication and avoids divergence in future security tweaks.Also applies to: 440-467
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.md(1 hunks)gemini-extension.json(1 hunks)package.json(2 hunks)src/app.ts(2 hunks)src/config/constants.ts(1 hunks)src/index.ts(11 hunks)src/lib/index.ts(1 hunks)src/lib/security.ts(1 hunks)src/mcp/helpers.ts(4 hunks)src/mcp/tools.ts(1 hunks)test/integration/mcp-tools.test.ts(3 hunks)test/integration/oauth-flow.test.ts(13 hunks)test/unit/lib/security.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/mcp/helpers.tstest/integration/oauth-flow.test.tssrc/config/constants.tssrc/lib/index.tstest/integration/mcp-tools.test.tssrc/index.tssrc/lib/security.tstest/unit/lib/security.test.tssrc/mcp/tools.tssrc/app.ts
🧠 Learnings (1)
📚 Learning: 2025-10-05T23:19:04.251Z
Learnt from: eyepokes
Repo: jsdelivr/globalping-mcp-server PR: 18
File: src/config/constants.ts:64-82
Timestamp: 2025-10-05T23:19:04.251Z
Learning: In the Globalping MCP OAuth redirect validation, STANDARD_PROTOCOLS is an exception list used with inverted logic: protocols NOT in this set are treated as deep links and allowed by default, while standard web protocols (including javascript:, chrome:, etc.) require explicit whitelisting via EXCEPTION_HOSTS. This prevents open redirects to arbitrary web URLs while allowing legitimate OAuth app deep link flows.
Applied to files:
src/lib/security.ts
🪛 markdownlint-cli2 (0.18.1)
README.md
49-49: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🔍 Remote MCP DeepWiki
Relevant repository documentation & facts found (useful when reviewing PR #24):
-
CORS / security config lives under src/config/constants.ts and URL/host/origin validation helpers are documented in Security/URL validation pages; entrypoint fetch() in src/index.ts routes API-token requests before OAuth and should be where Host/Origin validation is applied.,
-
GlobalpingMCP Durable Object:
- init() registers tools (registerGlobalpingTools + inline metadata tools) and uses a token provider closure (sanitizeToken(this.getToken())). Changes to tool registration/API (registerTool) must preserve that token-provider pattern.
-
OAuth flow:
- /authorize stores StateData in OAUTH_KV (10min TTL) with PKCE; /auth/callback retrieves and deletes state, exchanges code, introspects user, then uses OAuthProvider.completeAuthorization() and either auto-redirects (trusted URIs) or renders manualRedirectPage() for untrusted origins. validateOrigin/isTrustedRedirectUri logic is exercised here. Tests cover many redirect/host cases.
-
Token handling & API-token path:
- isAPITokenRequest() distinguishes API tokens (32-char alphanumeric) vs OAuth tokens; handleAPITokenRequest() builds ctx.props with accessToken =
Bearer ${token}which GlobalpingMCP.getToken() and sanitizeToken() rely on. Any CORS/Origin/Host validation applied at fetch() must still allow API-token flow to populate ctx.props.
- isAPITokenRequest() distinguishes API tokens (32-char alphanumeric) vs OAuth tokens; handleAPITokenRequest() builds ctx.props with accessToken =
-
URL/host trust rules and tests:
- isLocalhost/isDeepLink/isExceptionHost and isTrustedRedirectUri exist and are heavily unit-tested (many local/IPv6/hostname edge cases). PR changes to validateOrigin/getMatchingOrigin must align with these documented rules.
-
MCP transport endpoints and routing:
- /sse and /mcp handled in src/index.ts via handleMcpRequest → GlobalpingMCP.serveSSE()/serve(); CORS and Origin behavior must be applied consistently for both transports and for API-token vs OAuth flows.
-
Tests & CI:
- Unit tests (test/unit/) include comprehensive security tests for origin/host/CORS; integration tests updated to add Host headers and dynamic OAuth client registration. The repo uses Vitest and @cloudflare/vitest-pool-workers; updates to security behavior must preserve tests or update them accordingly.
-
Client integrations & manifests:
- gemini-extension.json and README describe clients (Gemini, Claude, Cursor) using the MCP endpoints; gemini-extension enables OAuth. Changes adding Origin/CORS validation can affect client registration flows (especially Gemini OAuth and deep-link handling).
Tools used:
- DeepWiki_read_wiki_structure
- DeepWiki_read_wiki_contents
🔇 Additional comments (6)
src/lib/index.ts (1)
3-3: LGTM!The security utilities are now part of the public API surface, enabling consumers to use origin/host validation and CORS helpers.
test/integration/mcp-tools.test.ts (1)
163-163: LGTM!Tests properly include Host headers to align with the new Host validation security requirements.
Also applies to: 240-240, 270-270
gemini-extension.json (1)
3-11: LGTM!Manifest updates properly document OAuth enablement and provide clear usage instructions for AI clients, synchronized with the 1.1.0 version bump.
src/config/constants.ts (1)
111-126: Port handling is correctly implementedThe origin validation logic in
src/lib/security.tsalready handles port variations for localhost origins. ThevalidateOrigin()function uses a two-stage check: exact match first, then for localhost/127.0.0.1/::1, it extracts the base origin (without port) and validates against that. The example comments in the code confirm this works as intended (validateOrigin("http://localhost:3000") // true).package.json (2)
22-28: Manual verification required: Tests must pass with these dependency updatesThe sandbox environment cannot execute
npm testto verify compatibility with the updated devDependencies. You must confirm locally that:
npm test(unit and integration tests) passes completelynpm run cf-typegencompletes without TypeScript errors from@types/nodev24.10.1- No runtime errors occur with
markedv17.0.0 (note: this version has breaking changes in list/checkbox token handling, though it appears unused in the codebase)Additionally, clarify why
patch-packagewas added if no patches are currently configured (nopatches/directory exists).
33-38: No breaking API changes found in @cloudflare/workers-oauth-provider 0.1.0The release notes only document a minor feature addition (audience validation for OAuth tokens per RFC 7519) with no API breaking changes. The upgrade from 0.0.11 → 0.1.0 maintains backward compatibility—existing calls to
parseAuthRequest()andcompleteAuthorization()remain unaffected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp/helpers.ts (1)
135-137: Consider using numeric type checks for min/avg/max to match aggregate logic.For aggregate stats (lines 57-65), you guard with
typeof stats.min === "number", but here you use!== undefined. If the API returnsnullfor these fields, the current check will render "null" in the output instead of "N/A", whereasundefinedcorrectly falls through.For consistency with the aggregate pattern and to handle both
nullandundefineduniformly:-const min = testResult.stats.min !== undefined ? testResult.stats.min : "N/A"; -const avg = testResult.stats.avg !== undefined ? testResult.stats.avg : "N/A"; -const max = testResult.stats.max !== undefined ? testResult.stats.max : "N/A"; +const min = typeof testResult.stats.min === "number" ? testResult.stats.min : "N/A"; +const avg = typeof testResult.stats.avg === "number" ? testResult.stats.avg : "N/A"; +const max = typeof testResult.stats.max === "number" ? testResult.stats.max : "N/A";This aligns with the defensive approach already applied to
lossat lines 139-140.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app.ts(2 hunks)src/lib/security.ts(1 hunks)src/mcp/helpers.ts(4 hunks)src/mcp/tools.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app.ts
- src/lib/security.ts
🧰 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/helpers.tssrc/mcp/tools.ts
🧬 Code graph analysis (1)
src/mcp/tools.ts (4)
src/index.ts (2)
GlobalpingMCP(13-378)getToken(357-360)src/mcp/helpers.ts (2)
parseLocations(10-18)formatMeasurementSummary(93-284)src/api/client.ts (3)
runMeasurement(89-110)getLocations(118-137)getRateLimits(145-164)src/auth/token-manager.ts (1)
maskToken(87-93)
🔍 Remote MCP DeepWiki
Relevant repository documentation & facts found (concise — items important to reviewing PR #24):
-
Main entrypoint & routing
- fetch() in src/index.ts routes API-token requests first, otherwise delegates to OAuthProvider which uses handleMcpRequest for /sse and /mcp — Host/Origin validation must be applied here to both API-token and OAuth flows.
-
Authentication & token handling
- Two auth paths: API token (32‑char alphanumeric) vs OAuth (PKCE). API-token requests build ctx.props.accessToken =
Bearer ${token}and bypass OAuth. Any CORS/Origin checks added in fetch() must preserve API-token path behavior so ctx.props is still populated.
- Two auth paths: API token (32‑char alphanumeric) vs OAuth (PKCE). API-token requests build ctx.props.accessToken =
-
OAuth flow & KV usage
- /authorize stores StateData (codeVerifier, codeChallenge, state) in OAUTH_KV with 10‑min TTL; /auth/callback retrieves and deletes it, exchanges code, introspects user, and calls completeAuthorization(). Changes to origin/host/CORS must not break StateData usage or redirect trust logic (trusted localhost/deep links → auto redirect; untrusted HTTPS → manual redirect page).
-
URL/Host validation & trust rules
- There are existing URL/host validation helpers (isLocalhost/isDeepLink/isExceptionHost/isTrustedRedirectUri) and comprehensive unit tests. New validateHost/validateOrigin/getMatchingOrigin must align with those rules to avoid regressions. Tests for URL validation exist and cover many edge cases.
-
MCP Durable Object and tools
- GlobalpingMCP is a Durable Object that registers tools during init(); tools rely on getToken() → sanitizeToken() and receive props via getToolContext(). CORS/Origin changes that affect MCP transport must still allow retrieval of ctx.props and the token-provider closure used by registerGlobalpingTools.
-
CORS / per-request options impact surface
- MCP supports both SSE and Streamable HTTP transports at /sse and /mcp. getCorsOptionsForRequest should return a single Access‑Control‑Allow‑Origin value (exact origin or "*") usable by both SSE and streamable HTTP responses. Ensure SSE handling (text/event-stream) sets headers correctly.
-
Tests & CI
- Unit tests and integration tests exist (Vitest + @cloudflare/vitest-pool-workers). The PR added new unit tests for security utilities and updated integration tests to include Host header usage — verify CI runs these and that the new validation passes all existing cases.
-
Deployment & bindings
- Durable Object binding name is globalping_mcp_object; KV binding is OAUTH_KV; config and routes are in wrangler.jsonc — changes that affect behavior at runtime (CORS, host validation) must be compatible with these bindings and with smart placement/Workers environment.
-
Client/manifest considerations
- gemini-extension.json and README mention clients (Gemini, Claude, Cursor) using /mcp and OAuth; stricter Origin validation may impact Gemini extension oauth.enabled flows or clients that rely on deep-link or localhost behavior — ensure allowed origins include intended client origins or provide clear config.
Sources used:
- DeepWiki (repository docs & code overview) — DeepWiki_read_wiki_structure, DeepWiki_read_wiki_contents,
3 participants
