[Audit v2 #345] Reject non-loopback Host/Origin (DNS-rebinding guard) by dsarno · Pull Request #375 · hi-godot/godot-ai

dsarno · 2026-05-05T17:22:09Z

Summary

Closes #345 — audit-v2 P1 finding #1 from umbrella #343.

The WebSocket server (:9500) and the streamable-HTTP transport (:8000,
including /godot-ai/status — comment-marked "small unauthenticated probe")
both bound to 127.0.0.1 but performed no Host/Origin validation. A
malicious browser tab could mount DNS rebinding: resolve
attacker.example.com to 127.0.0.1, then new WebSocket("ws://attacker.example.com:9500") — the request lands on our loopback socket, the
WS handshake registers the peer as a session, and any MCP tool becomes
drivable from a foreign origin including the write tools.

Fix

Per the issue's "Fix shape" — strict Host/Origin allowlist in a
Starlette middleware ahead of FastMCP, mirrored on the WebSocket side
via process_request.

src/godot_ai/transport/origin_guard.py (new):

is_allowed_host — accepts 127.0.0.1, localhost, [::1] with an
optional :port. Bare ::1 rejected (RFC 7230 requires bracketed IPv6
in HTTP Host headers).
is_allowed_origin — None/empty/null accepted (native clients omit
Origin; sandboxed/file:// emit null); otherwise must parse to a URL
whose hostname is loopback. Schemes outside http/https/ws/wss rejected.
LocalhostOnlyHTTPMiddleware — ASGI middleware. Rejects with HTTP 403;
passes lifespan scopes through; fails closed on duplicate-Host smuggling
(the same header appearing more than once).
make_websocket_request_guard() — process_request hook for
websockets.asyncio.server.serve(...). Uses headers.get_all so a
smuggled duplicate Host fails closed instead of tripping
MultipleValuesError and surfacing as 500.

Wiring:

transport/websocket.py: pass process_request= into serve().
server.py: outermost wrap on the HTTP app, applied to every HTTP
transport (http, streamable-http, sse) so /godot-ai/status and
the FastMCP endpoints are guarded uniformly.

Native clients keep working unchanged: the Godot plugin's WebSocketPeer
sends Host: 127.0.0.1:<port> and no Origin. Verified by live smoke.

Test plan

ruff check src/ tests/ — clean
ruff format --check src/ tests/ — clean
pytest -q — 825 passed (+45 new)
script/ci-check-gdscript — clean (no GDScript touched)
Live smoke (real GodotWebSocketServer):
- Loopback ws://127.0.0.1:<port> connect + handshake → handshake_ack ✓
- Origin: https://attacker.example.com → HTTP 403 at upgrade, no session registered ✓
Live smoke (streamable-HTTP):
- Loopback GET /godot-ai/status → 200 with full payload ✓
- Host: attacker.example.com → 403 ✓
- Loopback Host + Origin: https://attacker.example.com → 403 ✓

New tests:

tests/unit/test_origin_guard.py — 44 tests. Parametrized helper
coverage (loopback / sneaky-substring / private-IP / malformed /
smuggled-multi-value), middleware behavior including lifespan
passthrough and FastMCP state __getattr__ passthrough, and an
explicit DNS-rebinding-fingerprint test (loopback Host + browser
Origin must reject).
tests/integration/test_websocket.py::TestDnsRebindingGuard — 5 live
tests against a real websockets server: loopback succeeds, non-loopback
Host returns 403, non-loopback Origin returns 403, loopback-shaped
Origin succeeds, and a rejected request never reaches the registry.
tests/unit/test_asgi_session_diagnostics.py — structural assertions
updated for the new outer wrap; state lookup now traverses one extra
layer of __getattr__.
tests/unit/test_server_status.py — TestClient(base_url=...) set to
a loopback host so the request passes the new guard.

Deviations from the issue's "Fix shape"

The issue offers two alternatives — "HMAC handshake bound to spawned-server
PID-file secret, or strict Host/Origin allowlist in a Starlette middleware
ahead of FastMCP." I picked the second:

The named threat is DNS rebinding, which is fully mitigated by
Host/Origin validation. An HMAC token doesn't add anything for the
browser case.
Local-impostor processes that read the loopback socket are addressed
separately in [Audit v2 #2 — P1] Session hijacking via duplicate-ID handshake (silent overwrite) #346 (PR Harden WebSocket transport: errno portability, Future leak, duplicate-ID reject #366, duplicate-handshake reject) — adding a
token here would duplicate that effort without strengthening it.
Origin allowlist also closes [Audit v2 #11 — P2] /godot-ai/status HTTP endpoint is unauthenticated probe #355 (the /godot-ai/status "comment-
acknowledged" probe gap was gated on this fix), without per-launch
secret coordination between plugin and server.

The PID-file secret remains available for future use if the threat model
broadens (multi-user shared loopback, container-shared 127.0.0.1, etc).

Bundling note

#345's body says "Consider bundling with #2 (#346)." #346 already has open
PR #366, which explicitly carves #1 out: "#1 (auth/Origin gap on the WS
upgrade) — separate scope, deserves its own design discussion." So this
PR ships the carved-out scope solo. If #366 lands first I'll rebase.

Blocks

[Audit v2 #11 — P2] /godot-ai/status HTTP endpoint is unauthenticated probe #355 (audit-v2 List parameters arrive as JSON strings when called from Claude Code MCP client #11 — /godot-ai/status is unauthenticated probe) is
unblocked by this PR's HTTP middleware: the same allowlist now guards
the status route. The maintainer may want to close [Audit v2 #11 — P2] /godot-ai/status HTTP endpoint is unauthenticated probe #355 as
"addressed by [Audit v2 #1 — P1] WebSocket has no auth or Origin check (DNS-rebinding surface) #345" or queue it for further verification.

Refs umbrella #343.

Generated by Claude Code

Closes #345 — refs #343 (audit-v2 umbrella). Both transports bound to 127.0.0.1, but a malicious browser tab could mount **DNS rebinding**: resolve attacker.example.com to 127.0.0.1, then `new WebSocket("ws://attacker.example.com:9500")`. The request lands on our loopback socket carrying a non-localhost Host (and Origin), the WS handshake registered the peer as a session, and any MCP tool became drivable from a foreign origin — including write tools. The streamable-HTTP transport on :8000 had the same gap, including `/godot-ai/status` (comment-marked "small unauthenticated probe"). Per the issue's "Fix shape": **strict Host/Origin allowlist in a Starlette middleware ahead of FastMCP** plus a matching `process_request` hook on the WebSocket server. `src/godot_ai/transport/origin_guard.py` (new): - `is_allowed_host` — accepts `127.0.0.1`, `localhost`, `[::1]` with optional `:port`; case-insensitive; bare `::1` rejected (RFC 7230 requires bracketed IPv6 in Host). - `is_allowed_origin` — None / empty / `null` accepted (native clients omit Origin; sandboxed/file:// emit `null`); otherwise must parse to a URL whose hostname is loopback. Schemes outside `http/https/ws/wss` rejected. - `LocalhostOnlyHTTPMiddleware` — ASGI middleware. Rejects on first failure with HTTP 403; passes lifespan scopes through; handles duplicate-Host smuggling shapes (fail closed when the same header appears more than once). - `make_websocket_request_guard()` — `process_request` hook for `websockets.asyncio.server.serve(...)` mirroring the same logic. Uses `headers.get_all` so a smuggled duplicate Host fails closed rather than tripping `MultipleValuesError` and surfacing as 500. Wiring: - `transport/websocket.py`: pass `process_request=` to `serve()`. - `server.py`: outermost wrap on the HTTP app, applied to every HTTP transport (`http`, `streamable-http`, `sse`) so `/godot-ai/status` and the FastMCP endpoints are guarded uniformly. Native clients keep working: the Godot plugin's WebSocketPeer sends `Host: 127.0.0.1:<port>` and no Origin. Verified by smoke against a real WebSocketServer (loopback connect → `handshake_ack`; rebound Origin → 403) and against the streamable-HTTP transport (`/godot-ai/status`: loopback → 200, `Host: attacker.example.com` → 403, loopback Host + browser Origin → 403). Tests: - `tests/unit/test_origin_guard.py` (44 tests): parametrized helper coverage (loopback / sneaky-substring / IP / malformed / multiple smuggled values), middleware behavior, lifespan passthrough, `state` `__getattr__` passthrough. - `tests/integration/test_websocket.py::TestDnsRebindingGuard` (5 tests): live `websockets` server + client. Loopback succeeds, non-loopback Host returns 403, non-loopback Origin returns 403, loopback-shaped Origin succeeds, rejected request never registers a session. - `tests/unit/test_asgi_session_diagnostics.py`: structural assertions updated for the new outer wrap; FastMCP `state` lookup now traverses one extra layer. - `tests/unit/test_server_status.py`: `TestClient(base_url=...)` set to a loopback host so the request passes the new guard. Test plan: - ruff check + format: clean - pytest -q: 825 passed (+45 new) - script/ci-check-gdscript: clean (no GDScript touched) - Live smoke: real `GodotWebSocketServer` and real streamable-HTTP app, loopback succeeds, non-loopback Host/Origin → 403. Note: PR #366 (audit-v2 #2, session-hijack) closes the duplicate-handshake takeover. This PR closes the rebinding-from-browser path. Together they harden the WebSocket transport at both layers (who can connect, and what they can do once they have).

codecov · 2026-05-05T17:25:20Z

Codecov Report

❌ Patch coverage is 98.92473% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/godot_ai/transport/origin_guard.py	98.87%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Both the WebSocket ``process_request`` hook and ``LocalhostOnlyHTTPMiddleware`` implemented the same 3-step rule (count duplicate headers → check ``is_allowed_host`` → check ``is_allowed_origin`` → 403) twice, with slightly different intermediate state. Funnel both through one ``evaluate_loopback(hosts, origins) -> bool`` so a regression in one transport cannot accidentally diverge from the other. Drive-by: - Decode FORBIDDEN_BODY once at module load (FORBIDDEN_BODY_TEXT) so the WS guard doesn't decode the same bytes per rejection. - Drop redundant ``parsed.scheme.lower()`` — ``urlsplit`` already normalises the scheme per RFC 3986. No behavior change. 825 tests pass.

Copilot

Pull request overview

Adds a new Host/Origin guard around the MCP HTTP and WebSocket transports to mitigate browser-driven DNS-rebinding against the local Godot AI server, and updates tests to reflect the new transport wrapper behavior.

Changes:

Add origin_guard.py with loopback Host/Origin helpers, HTTP middleware, and a WebSocket process_request guard.
Wire the guard into the WebSocket server and wrap all HTTP transports in server.http_app().
Add/adjust unit and integration tests for Host/Origin validation, middleware layering, and status-route access under loopback hosts.

Reviewed changes

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

Show a summary per file

File	Description
`src/godot_ai/transport/origin_guard.py`	New loopback Host/Origin validation helpers plus HTTP/WebSocket guard implementations.
`src/godot_ai/transport/websocket.py`	Hooks the pre-upgrade WebSocket request guard into `websockets.serve()`.
`src/godot_ai/server.py`	Wraps HTTP transports with the new localhost-only middleware.
`tests/unit/test_origin_guard.py`	Adds unit coverage for allowed/rejected Host and Origin values and middleware behavior.
`tests/integration/test_websocket.py`	Adds live WebSocket guard coverage for accepted/rejected upgrade requests.
`tests/unit/test_asgi_session_diagnostics.py`	Updates middleware-layer assertions for the new outer wrapper.
`tests/unit/test_server_status.py`	Updates the status-route test client to use a loopback `base_url`.

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

dsarno · 2026-05-05T17:37:43Z

+    or file:// contexts send ``Origin: null``. Both are accepted; any
+    other origin must parse to a URL whose hostname is loopback.
+    """
+    if origin_header is None:
+        return True
+    value = origin_header.strip()
+    if not value or value.lower() == "null":
+        return True


Good catch — addressed in 48757ad.

is_allowed_origin now rejects null (case-insensitive, whitespace-tolerant). Native clients never produce Origin: null (they omit Origin entirely), so the tightening is invisible to the Godot plugin / FastMCP CLI / curl. New tests pin both layers:

tests/unit/test_origin_guard.py::test_origin_null_rejected (helper)

tests/unit/test_origin_guard.py::test_middleware_rejects_origin_null (HTTP middleware)

tests/integration/test_websocket.py::TestDnsRebindingGuard::test_origin_null_rejected_at_upgrade (live WS)

Generated by Claude Code

dsarno · 2026-05-05T17:37:58Z

+        ## Outermost wrap: refuse non-loopback Host/Origin (DNS-rebinding
+        ## guard, audit-v2 finding #1). Applied to every HTTP transport
+        ## including ``sse`` so ``/godot-ai/status`` and the FastMCP
+        ## endpoints are guarded uniformly.
+        return LocalhostOnlyHTTPMiddleware(app)


Right — addressed in 48757ad.

The original gate let cross-origin no-cors subresources (<img src="http://127.0.0.1:9500/godot-ai/status"> from a foreign page) through because browsers omit Origin for <img> / <script> / <link> GETs. Modern browsers do stamp every HTTP request with Sec-Fetch-Site though, and native clients never send it — that's the discriminating signal.

Added is_allowed_sec_fetch_site: rejects cross-site and same-site; accepts same-origin, none (top-level navigation: user typed URL or used a bookmark), and missing (native client). evaluate_loopback now feeds it from both transports through the same single helper, so the WS hook and HTTP middleware can't drift.

Tests:

tests/unit/test_origin_guard.py::test_sec_fetch_site_friendly_values_pass / ..._foreign_values_rejected

tests/unit/test_origin_guard.py::test_evaluate_loopback_cross_site_subresource_rejected (the exact <img> shape)

tests/unit/test_origin_guard.py::test_middleware_rejects_cross_origin_subresource

tests/integration/test_websocket.py::TestDnsRebindingGuard::test_browser_cross_origin_subresource_rejected (live WS)

#355 (status-route unauthenticated probe) is now covered by this same Sec-Fetch-Site policy in addition to the loopback Host/Origin gate.

Generated by Claude Code

…igin Two browser-side liveness/bypass shapes that the first cut of the loopback guard let through: 1. **``Origin: null``** (Copilot, origin_guard.py:85). Sandboxed ``<iframe sandbox>`` and downloaded ``file://`` pages serialize their origin as ``null``. The original guard accepted this on the theory that file:// /sandboxed contexts are "legitimate"; in practice they're exactly the bypass an attacker would use to bridge a foreign origin onto our loopback socket. **Now rejected.** Native clients never produce ``null`` (they omit Origin entirely), so this tightening is invisible to the Godot plugin / FastMCP CLI / curl. 2. **Cross-origin no-cors subresource probes** (Copilot, server.py:79). `<img src="http://127.0.0.1:9500/godot-ai/status">` from ``https://attacker.example.com`` arrives with a loopback ``Host`` and *no* ``Origin`` (browsers omit Origin for ``no-cors`` GETs of ``<img>`` / ``<script>`` / ``<link>``), so the original Host/Origin gate let it through and the page could use the 200/403 outcome as a liveness oracle. Browsers stamp every HTTP request with ``Sec-Fetch-Site`` (``cross-site`` / ``same-site`` / ``same-origin`` / ``none``). Native non-browser clients never send it. **The guard now rejects ``cross-site`` and ``same-site``** while still allowing ``none`` (top-level navigation: user typed URL, bookmark) and ``same-origin`` (loopback page fetching its own server). Missing → allow (native client), preserving the existing flow. Drive-by: trailing-dot loopback (``Host: localhost.``) now accepted. RFC 1034 valid FQDN syntax that browsers and curl can preserve through to the Host header — friction trap if rejected, no security implication either way. `evaluate_loopback` now takes an optional ``sec_fetch_sites`` list so both transports run the new policy through the same single helper — preserves the audit-v2 invariant that the WS hook and HTTP middleware cannot drift. Tests: - New parametrized helper coverage for ``is_allowed_sec_fetch_site`` (friendly + foreign + case-insensitive + whitespace), trailing-dot hosts, and explicit ``Origin: null`` rejection. - Two new ``evaluate_loopback`` cases pinning the cross-site rejection with no-Origin (the exact Copilot shape) and ``Origin: null`` with any Sec-Fetch-Site value. - Three new ``LocalhostOnlyHTTPMiddleware`` middleware cases: ``Origin: null`` rejected, cross-site subresource rejected, top-level navigation accepted. - Three new live-WS integration cases pinning the same rules end-to-end through the websockets ``process_request`` hook, plus a bracketed-IPv6 ``http://[::1]:9500`` symmetry test for the WS path. Live smoke: native loopback connect → ``handshake_ack`` ✓; ``Origin: null`` WS connect → 403 ✓; ``Sec-Fetch-Site: cross-site`` WS connect → 403 ✓; ``Origin: null`` HTTP probe → 403 ✓; cross-site HTTP probe → 403 ✓.

51 commits forward from main, headlined by two architecture audits: audit-v1 (issue #297, PRs #298-#315): scene-path ancestry guard, update/config data-loss safeguards, lifecycle reliability, characterization tests, plugin.gd extraction (McpPortResolver + McpServerLifecycleManager), state-model cleanup, McpUpdateManager extraction, Runtime Protocol deletion + DirectRuntime retype, narrowed meta-tool JSON coercion, self-update preload-alias hardening, locked FastMCP middleware order. audit-v2 (issue #343, PRs #369-#390): origin allowlist (DNS-rebinding guard, #345/#375), path-traversal guards on script_* / filesystem_* writes (#347/#369), errno.EADDRINUSE portability across all OSes (#348/#373), SessionRegistry RLock removal (#350/#370), Pydantic-validated WebSocket event payloads (#351/#378), sole-survivor auto-failover on active disconnect (#352/#379), 30s filesystem_changed watchdog during update reload (#353/#381), FAILED_MIXED self-update visibility via mixed_state in editor_state (#354/#382), 32/tick inbound packet-drain cap with spillover logging (#356/#383), error-code vocabulary enrichment (#365/#385: NODE_NOT_FOUND, PROPERTY_NOT_ON_CLASS, VALUE_OUT_OF_RANGE, MISSING_REQUIRED_PARAM cut INVALID_PARAMS sites 471 -> 97), resolve-or-error helper extraction (#364/#389: 38+ duplicate sites migrated), resource-form lint for meta-tool reads (#363/#386), LogViewer + PortPickerPanel extraction from mcp_dock.gd (#360/#390), LogViewer.tick() buffer-clear recovery (#392). Conflict resolutions (both intentional, both took beta's side): - plugin/addons/godot_ai/animation_handler.gd plus new submodules animation_presets.gd and animation_values.gd: beta's 4-domain split retained. Main's revert (#368) was CI-flake hygiene, not a structural rejection per its own commit message ("Empty commit to retrigger CI (flake on Godot tests / macOS)"). Beta's #367 is the canonical state the audit found wanted. - plugin/addons/godot_ai/clients/_toml_strategy.gd: beta's version retained. Beta handles inline comments after section headers (`[next_section] # note`) via _is_any_section_header(); main's later re-derived bare-key fix uses the simpler bracket check and would regress on commented headers. The four main-only Windows / TOML hotfixes (#302, #318, #319, #320) all landed on beta independently under different commit hashes and are content-equivalent or beta-improved; no cherry-pick was needed before this merge. Validation: - CI: green on beta tip 72b35d7 (and on PR #392 separately) across ubuntu-latest / macos-latest / windows-latest for Python tests, Godot tests, release-smoke, and game-capture-smoke. - Operator smoke (macOS, 2026-05-06): all 10 phases of the audit-v1+v2 post-landing runbook green. Report on issue #343. 47 GDScript suites + 903 Python tests passing. Phase 7 interactive self-update verified end-to-end with the local-self-update-smoke harness; plugin advanced 2.3.2 -> 2.3.3, no .ips, no _exit_tree leak, server stop/start clean. - Operator smoke (Windows 11, beta tip d5aa29f, 2026-05-06): Phase 7 self-update green (7/7; macOS-only .ips check correctly downgraded to SKIP). Path-traversal guard rejects backslash variant. Cursor client configure cycle round-trips cleanly. editor_state ping 109ms. Pre-existing pytest UnicodeDecodeError on Windows tracked separately as #397 (also on main, not introduced by this merge). # Conflicts: # test_project/tests/test_clients.gd

Copilot AI review requested due to automatic review settings May 5, 2026 17:22

Copilot started reviewing on behalf of dsarno May 5, 2026 17:22 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

dsarno merged commit 3295ded into beta May 5, 2026
11 checks passed

dsarno deleted the claude/audit-v2-345-ws-auth-origin branch May 7, 2026 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Audit v2 #345] Reject non-loopback Host/Origin (DNS-rebinding guard)#375

[Audit v2 #345] Reject non-loopback Host/Origin (DNS-rebinding guard)#375
dsarno merged 3 commits into
betafrom
claude/audit-v2-345-ws-auth-origin

dsarno commented May 5, 2026

Uh oh!

codecov Bot commented May 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

dsarno May 5, 2026

Uh oh!

dsarno May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dsarno commented May 5, 2026

Summary

Fix

Test plan

Deviations from the issue's "Fix shape"

Bundling note

Blocks

Uh oh!

codecov Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dsarno May 5, 2026

Choose a reason for hiding this comment

Uh oh!

dsarno May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 5, 2026 •

edited

Loading