fix private mcp server dns resolution

[j4ys0n]

No description provided.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR delivers two independent fixes: (1) a SSRF bypass allowlist for private-IP MCP hostnames via EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST, enabling internal/private MCP servers to be registered without triggering the IP block; and (2) a hardcoded server-name allowlist (google-workspace) that maps unsupported_grant_type OAuth refresh errors to a McpReauthRequiredError instead of a generic failure. Both changes include test coverage.

Architecture & Organization

• The OAUTH_REFRESH_UNSUPPORTED_GRANT_REAUTH_SERVER_NAMES set in oauthTokens.ts is a hardcoded, in-process allowlist of server names with non-standard OAuth behavior. This pattern will require code changes every time a new provider with this quirk is onboarded, and there is no documentation or comment explaining the broader design intent (e.g., whether this list is expected to grow, or if a config-driven approach is planned).
• The getTrustedPrivateHostnameAllowlist() function in ssrf.ts reads process.env on every call to validateExternalMcpUrl. For a hot path, this is a minor inefficiency, but more importantly it means the allowlist is re-parsed per request rather than cached at startup — which is actually useful for tests but worth noting operationally.

Key Changes & Positives

• ssrf.ts: isHostnameAllowlisted correctly supports both exact-match and subdomain-wildcard (.example.com) patterns, which is a flexible and safe design for an allowlist. 🟢
• oauthTokens.ts: Surfacing unsupported_grant_type as a McpReauthRequiredError with a clear user-facing message prevents silent failures and gives clients actionable guidance. 🟢
• Tests in mcp-external-auth.spec.ts properly restore process.env and mock state, avoiding test pollution. 🟢

Potential Issues & Recommendations

1. Issue / Risk: isHostnameAllowlisted in ssrf.ts does not normalize the input hostname to lowercase before comparison, but the allowlist entries are lowercased via .toLowerCase().

   • Impact: A hostname like GoogleMCP.MissionSquad.AI would not match an allowlist entry of googlemcp.missionsquad.ai, potentially causing unexpected SSRF blocks for allowlisted hosts on case-sensitive inputs.
   • Recommendation: Apply .toLowerCase() to hostname at the top of isHostnameAllowlisted (or at the call site before passing in).
   • Status: 🟡 Needs review

2. Issue / Risk: The subdomain wildcard check hostname.endsWith(entry) where entry starts with . (e.g., .missionsquad.ai) would also match evil.notmissionsquad.ai if the suffix happens to align — but more concretely, it would match xmissionsquad.ai only if the entry were .missionsquad.ai and the hostname were x.missionsquad.ai, which is correct. However, hostname.endsWith('.missionsquad.ai') would also match evil.sub.missionsquad.ai, which may or may not be intended.

   • Impact: Overly broad allowlisting if operators use wildcard entries expecting only one level of subdomain to match.
   • Recommendation: Document the wildcard semantics in the env var description (README or inline comment) so operators understand .example.com matches all depths of subdomains.
   • Status: 🟡 Needs review

3. Issue / Risk: OAUTH_REFRESH_UNSUPPORTED_GRANT_REAUTH_SERVER_NAMES in oauthTokens.ts is a module-level constant with no escape hatch — if google-workspace fixes their token endpoint, or if another server needs the same treatment, the only path is a code change and redeploy.

   • Impact: Operational inflexibility; the list cannot be extended without a release.
   • Recommendation: Consider reading additional server names from an env var (e.g., OAUTH_REFRESH_UNSUPPORTED_GRANT_SERVERS) merged with the hardcoded set, or at minimum add a comment explaining why this is intentionally hardcoded.
   • Status: 🟡 Needs review

Security & Privacy

• The SSRF allowlist bypass is gated entirely on an env var (EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST). If this env var is misconfigured or accidentally set too broadly (e.g., . or empty-string entries), it could allow SSRF to internal infrastructure. The current filter((entry) => entry.length > 0) guards against empty strings, but a single . entry would match every hostname via endsWith('.') — which is always false for valid hostnames, so this edge case is actually safe. Still, operators should be warned in documentation that this env var is security-sensitive.
• DNS lookup still occurs even for allowlisted hostnames (dns.lookup is called before the if (!allowlistedHostname) guard in validateExternalMcpUrl). This is correct behavior — the URL still needs to resolve — but the resolved IPs are not logged or audited for allowlisted hosts, which may be a gap for security observability.

Tests

• The test 'allows trusted MCP hostnames that resolve to private IPs when explicitly allowlisted' only tests exact-match allowlisting. A test covering the subdomain wildcard path (.missionsquad.ai matching googlemcp.missionsquad.ai) would improve confidence in the isHostnameAllowlisted logic.

Approval Recommendation

Approve with caveats

• Lowercase-normalize the hostname argument in isHostnameAllowlisted to match the lowercased allowlist entries.
• Add a comment or README note documenting that EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST is security-sensitive and that wildcard entries (.example.com) match all subdomain depths.
• Consider adding a test for the subdomain wildcard matching path in isHostnameAllowlisted.
• (Optional) Add a comment in oauthTokens.ts explaining why OAUTH_REFRESH_UNSUPPORTED_GRANT_REAUTH_SERVER_NAMES is hardcoded rather than config-driven.

[Copilot]

Pull request overview

This PR updates external MCP SSRF validation to support explicitly allowlisting hostnames that resolve to private IPs (to unblock private MCP deployments), and improves OAuth refresh error handling for the google-workspace integration.

Changes:

• Add EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST-driven bypass for private-IP DNS resolution checks in validateExternalMcpUrl.
• Map unsupported_grant_type refresh failures to reauth_required for google-workspace.
• Add Jest coverage for the new SSRF allowlist behavior and the new OAuth refresh mapping; bump package version.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
test/mcp-external-auth.spec.ts	Adds tests for private-IP DNS allowlisting and google-workspace refresh error mapping.
src/utils/ssrf.ts	Implements hostname allowlist for private-IP DNS resolutions in external MCP URL validation.
src/services/oauthTokens.ts	Treats unsupported_grant_type refresh failures as reauth-required for google-workspace.
package.json	Bumps package version to 1.11.8.

Comments suppressed due to low confidence (1)

test/mcp-external-auth.spec.ts:221

• This test mutates process.env.EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST and restores it only after the assertion. If the expectation fails, the env var and dns.lookup spy may not be restored, causing cross-test pollution. Wrap the body in try/finally (restore env + mockRestore) or move restoration into afterEach.

    const previousAllowlist = process.env.EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST
    process.env.EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST = 'googlemcp.missionsquad.ai'
    const lookupSpy = jest.spyOn(dns, 'lookup').mockImplementation(async () =>
      [{ address: '10.1.253.40', family: 4 }] as any
    )

    await expect(validateExternalMcpUrl('https://googlemcp.missionsquad.ai/mcp')).resolves.toMatchObject({
      hostname: 'googlemcp.missionsquad.ai',
      pathname: '/mcp'
    })

    if (previousAllowlist === undefined) {
      delete process.env.EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST
    } else {
      process.env.EXTERNAL_MCP_PRIVATE_HOST_ALLOWLIST = previousAllowlist
    }
    lookupSpy.mockRestore()
  })

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.