Commit 3295ded
* Reject non-loopback Host/Origin on WS + HTTP transports (audit-v2 #1)
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).
* Simplify: extract evaluate_loopback so WS + HTTP guards can't drift
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.
* Address Copilot review: reject Origin: null + Sec-Fetch-Site cross-origin
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 ✓.
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent 39366fd commit 3295ded
7 files changed
Lines changed: 853 additions & 6 deletions
File tree
- src/godot_ai
- transport
- tests
- integration
- unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
51 | 51 | | |
52 | 52 | | |
53 | 53 | | |
| 54 | + | |
54 | 55 | | |
55 | 56 | | |
56 | 57 | | |
| |||
70 | 71 | | |
71 | 72 | | |
72 | 73 | | |
73 | | - | |
74 | | - | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
75 | 80 | | |
76 | 81 | | |
77 | 82 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| 17 | + | |
17 | 18 | | |
18 | 19 | | |
19 | 20 | | |
| |||
37 | 38 | | |
38 | 39 | | |
39 | 40 | | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
40 | 45 | | |
41 | 46 | | |
42 | 47 | | |
| |||
0 commit comments