Skip to content

Commit bc5bf8f

Browse files
scottschreckengaustclaudegithub-code-quality[bot]
authored
fix(openapi-mcp-server): DNS-pin spec fetches to prevent rebinding SSRF (#4108)
* fix(openapi-mcp-server): DNS-pin spec fetches to prevent rebinding SSRF The multi-spec (additional_specs) path validated spec_url via validate_url_for_spec() and then discarded the result, re-fetching with httpx.get(url) — a fresh DNS resolution. A rebinding server could answer a public IP during validation and 169.254.169.254 (or any internal IP) at fetch time (CWE-350 / CWE-367 TOCTOU SSRF). The primary spec URL was not validated at all. Fetch the spec by connecting only to the IP(s) validation already pinned, never re-resolving the hostname: - utils/openapi.py: add _pinned_fetch() — dials the pinned IP literal with Host + sni_hostname set to the validated hostname, follow_redirects=False with explicit 30x rejection, and a 10 MiB streamed size cap (the old loader read bodies unbounded). load_openapi_spec() gains a validated_url param and validates+pins a bare url in-function via _validate_url_sync(), so no code path fetches a spec un-pinned. SSRFError/SSRFFetchError are not retried away. - server.py: thread the ValidatedURL from validation into load_openapi_spec() for additional specs; the primary spec is validated+pinned by the loader with the --allow-* flags passed through. fastmcp's ssrf_safe_fetch is intentionally not reused: it re-resolves DNS, is HTTPS-only, caps at 5 KB, and is async (would crash the sync loader inside the running event loop). Tests cover rebinding (DNS resolved once; metadata IP never dialed), redirect refusal, mixed public/private rejection, size caps, http opt-in, and the newly-validated primary path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(openapi-mcp-server): document DNS pinning; add coverage for pinned fetch - README: replace the outdated "deploy behind an egress proxy for full DNS pinning" note with a DNS-pinning (rebinding protection) section; note the 10 MiB cap and redirect refusal; clarify --allow-insecure-http is still pinned. - CHANGELOG: add Unreleased Security entry for the DNS-rebinding TOCTOU fix. - tests: cover the new branches — scheme rejection, retry-across-pinned-IPs, all-IPs-fail, no-IPs, the running-loop sync bridge, http opt-in (no SNI), and that SSRF failures are not retried. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(openapi-mcp-server): cover _parse_spec_bytes paths for patch coverage Add unit tests for the prance-success, prance-failure→JSON, and JSON-failure→YAML branches of _parse_spec_bytes so codecov/patch reflects the new fetch/parse helpers (utils/openapi.py 89%→93%). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: pull request finding 'Explicit returns mixed with implicit (fall through) returns' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * fix: finding 'Empty except' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * fix(openapi-mcp-server): correct misplaced terminal raise in load_openapi_spec The autofix for the implicit-None fall-through inserted the terminal `raise ValueError` directly after the docstring, making it unconditional and killing every code path (url, path, validated_url) before any real logic ran. Move the terminal raise to the true fall-through point at the end of the function; the all-empty guard and the return/raise in each branch remain intact. Also fix test_path_invalid_validation, which patched validate_openapi_spec in the module where it is defined (openapi_validator) rather than where openapi.py binds it at import time. The mock never took effect, so the test failed independently of this change (fails on main too). Patch the used name to match the passing sibling test_url_invalid_spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
1 parent 95623c9 commit bc5bf8f

9 files changed

Lines changed: 687 additions & 81 deletions

File tree

‎src/openapi-mcp-server/CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
66
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
77

8+
## [Unreleased]
9+
10+
### Security
11+
- **Fixed DNS-rebinding TOCTOU SSRF in spec fetching** ([CWE-350](https://cwe.mitre.org/data/definitions/350.html) / [CWE-367](https://cwe.mitre.org/data/definitions/367.html)): `spec_url` was validated via DNS resolution and then re-fetched via a second, independent resolution, allowing a rebinding host to pass validation with a public IP and be fetched from an internal/metadata IP. Spec URLs are now fetched by connecting only to the IP(s) that passed validation (DNS resolved once; `Host`/SNI preserved), redirects are refused, and response bodies are capped at 10 MiB. The primary spec URL is now validated too (previously unchecked).
12+
13+
### Changed
14+
- Spec URL loading is DNS-pinned end to end; the previous README note about deploying behind an egress proxy for "full DNS pinning" no longer applies.
15+
816
## [1.1.0] - 2026-05-31
917

1018
### Added

‎src/openapi-mcp-server/README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -329,18 +329,18 @@ The server validates all URLs in `additional_specs` entries before fetching:
329329

330330
**Credential isolation**: Additional specs never inherit the primary API's credentials. Each entry must declare its own `auth_type`/`auth_token` or defaults to no auth.
331331

332+
**DNS pinning (rebinding protection)**: Spec URLs are validated **and fetched against the same resolved IP** — the loader connects only to the address(es) that passed validation, never re-resolving the hostname at fetch time. This closes the DNS-rebinding TOCTOU window where a hostname could resolve to a public IP during validation and to an internal/metadata IP during the fetch. This applies to both the primary spec URL and every `additional_specs` entry. Redirects are not followed (a `30x` toward an internal host is rejected), and spec bodies are capped at 10 MiB.
333+
332334
**Path traversal protection**: `spec_path` is canonicalized, restricted to spec file extensions (`.json`, `.yaml`, `.yml`), and checked against a best-effort system directory blocklist. For production deployments, use `--allowed-spec-dirs` to explicitly restrict allowed paths.
333335

334336
**Escape hatches** for development/internal use:
335337

336338
| Flag | Env Var | Effect |
337339
|------|---------|--------|
338-
| `--allow-insecure-http` | `ALLOW_INSECURE_HTTP=true` | Permits `http://` URLs |
340+
| `--allow-insecure-http` | `ALLOW_INSECURE_HTTP=true` | Permits `http://` URLs (still DNS-pinned) |
339341
| `--allow-private-networks` | `ALLOW_PRIVATE_NETWORKS=true` | Permits private/loopback IPs |
340342
| `--allowed-spec-dirs` | `ALLOWED_SPEC_DIRS=/path1:/path2` | Restricts `spec_path` to listed directories |
341343

342-
> **Note**: DNS validation confirms hostnames resolve to public IPs at check time. For environments requiring full DNS pinning, deploy behind an egress proxy or use `spec_path` with local files.
343-
344344
To report security issues, see the [Contributing Guidelines](https://github.com/awslabs/mcp/blob/main/CONTRIBUTING.md#security-issue-notifications).
345345

346346
## Docker Deployment

‎src/openapi-mcp-server/awslabs/openapi_mcp_server/server.py‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,17 @@ async def create_mcp_server_async(config: Config) -> FastMCP:
9292
logger.debug(
9393
f'Loading OpenAPI spec from URL: {config.api_spec_url} or path: {config.api_spec_path}'
9494
)
95-
openapi_spec = load_openapi_spec(url=config.api_spec_url, path=config.api_spec_path)
95+
96+
# For a URL, load_openapi_spec validates + DNS-pins internally: it
97+
# resolves once and fetches by connecting only to that pinned IP, so
98+
# there is no un-pinned fetch path for the primary spec either. Pass the
99+
# SSRF flags through so the operator's --allow-* opt-ins are honored.
100+
openapi_spec = load_openapi_spec(
101+
url=config.api_spec_url,
102+
path=config.api_spec_path,
103+
allow_http=config.allow_insecure_http,
104+
allow_private_networks=config.allow_private_networks,
105+
)
96106

97107
# Validate the OpenAPI spec
98108
if not validate_openapi_spec(openapi_spec):
@@ -300,9 +310,12 @@ def enrich_component(route: Any, component: Any) -> None:
300310
spec_url = entry.get('spec_url', '')
301311
spec_path = entry.get('spec_path', '')
302312

313+
# Capture the validated result so the fetch connects to the
314+
# pinned IP(s) — never re-resolving the hostname at fetch time.
315+
spec_validated_url = None
303316
if spec_url:
304317
try:
305-
await validate_url_for_spec(
318+
spec_validated_url = await validate_url_for_spec(
306319
spec_url,
307320
allow_http=config.allow_insecure_http,
308321
allow_private_networks=config.allow_private_networks,
@@ -326,7 +339,13 @@ def enrich_component(route: Any, component: Any) -> None:
326339

327340
logger.info(f'Loading additional spec: {extra_name}')
328341
try:
329-
extra_spec = load_openapi_spec(url=spec_url, path=spec_path)
342+
# Load using the pinned IPs from validation (no re-resolution).
343+
extra_spec = load_openapi_spec(
344+
validated_url=spec_validated_url,
345+
path=spec_path,
346+
allow_http=config.allow_insecure_http,
347+
allow_private_networks=config.allow_private_networks,
348+
)
330349
except Exception as e:
331350
logger.warning(f'Failed to load additional spec {extra_name}: {e}')
332351
continue

0 commit comments

Comments
 (0)