[Audit v2 #346+#349] WS hardening: duplicate-ID reject + send_command leak

[dsarno]

Summary

Two transport-layer audit fixes in src/godot_ai/transport/websocket.py. Cribbed from abandoned PR #366 (which targeted main); this re-targets beta per the maintainer's bundle direction in #346 / #349. The #348 (errno) portion of #366 already shipped via PR #373 and is omitted here.

• [Audit v2 #2 — P1] Session hijacking via duplicate-ID handshake (silent overwrite) #346 (P1, security) — duplicate-ID handshake hijack: a second handshake with the same session_id previously overwrote both _sessions[id] and _connections[id] silently, so a local peer could brute-force the 16-bit hex suffix and steal command routing. Now rejects with WS close code 4001 + warning log naming the existing peer's pid/project. Legitimate reconnect after editor_reload_plugin is unaffected — ConnectionClosed → unregister happens before the new connect, so the slot is free.
• [Audit v2 #5 — P2] send_command leaks pending Futures when ws.send raises #349 (P2, reliability) — send_command Future leak: ws.send raising mid-call left the registered Future stranded in _pending forever. Wrapped the send + wait_for in try/finally that always pops. Happy path still pops via the response receiver, so the finally pop is a no-op there.

Test plan

• ruff check src/ tests/ — passes (the 6 unrelated unformatted files were already that way on beta)
• ruff format --check on changed files (src/godot_ai/transport/websocket.py, tests/integration/test_websocket.py) — clean
• pytest -v — 867 passed
• script/ci-check-gdscript — all GDScript files OK
• New regression tests:
  • TestDuplicateHandshake::test_duplicate_session_id_handshake_is_rejected — attack-shape regression: round-trips a command through the original WS after the duplicate is rejected to prove the routing map wasn't hijacked
  • TestDuplicateHandshake::test_reconnect_after_clean_disconnect_succeeds — guards the legitimate reconnect-after-editor_reload_plugin path
  • TestPendingFutureCleanup::test_timeout_pops_pending_entry — pins the existing timeout-pop behavior so the refactor doesn't regress it
  • TestPendingFutureCleanup::test_send_failure_pops_pending_entry — covers the new send-time-exception path that was previously leaking
• Live editor test_run skipped — diff is Python-only (transport layer), no GDScript or handler changes. PR Harden WebSocket transport: errno portability, Future leak, duplicate-ID reject #366 already live-smoked an identical diff against Godot 4.6 (1037/1040 GDScript suite green).

Deviations from "Fix shape"

None. #346's fix shape called for "reject the second handshake with a warning log naming the colliding peer" — implemented. #349's fix shape called for "try/finally pattern that pops on any non-success path" — implemented.

Cross-references

Closes #346
Closes #349
Umbrella: #343
Bundled per maintainer comment on #346.

https://claude.ai/code/session_01ERwAhFK6CEZLRigwK1iC2k

---

Generated by Claude Code

[dsarno]

Closing as duplicate of #374, which merged the identical fix to beta at 18:09:25Z (commit 84a2926). My branch was based on the pre-merge beta tip (3295ded), so when I rebased + diffed against the post-merge tip, the only differences were two comment-text variations — zero functional changes.

Background: I picked this up per the maintainer's bundle direction, which said PR #366 was closed and a beta-targeted PR was still needed. While I was working, PR #374 was opened/merged from the same source branch (claude/laughing-mendel-eVqHJ) the bundle comment told me to crib from — so the bundle direction had become stale by the time I shipped. The umbrella body in #343 already reflects #346 + #349 as merged via PR #374.

Releasing the claim on #346 and #349; will pick up the next item from the audit-v2 queue.

---

Generated by Claude Code

[Copilot]

Pull request overview

This PR hardens the WebSocket transport layer (GodotWebSocketServer) to prevent session hijacking via duplicate session_id handshakes and to eliminate a pending-Future leak when ws.send() raises during send_command. These changes directly address audit-v2 findings (#346 security, #349 reliability) and add integration tests to lock in the new behavior.

Changes:

• Reject duplicate session_id handshakes with an application-defined WS close code (4001) and a warning log that identifies the existing peer.
• Ensure send_command() always cleans up _pending[request_id] via try/finally, preventing leaked Futures on send failures / cancellations / timeouts.
• Add integration regression tests covering duplicate-handshake rejection, clean reconnect, and _pending cleanup on timeout and send failure.

Reviewed changes

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

File	Description
src/godot_ai/transport/websocket.py	Rejects duplicate session_id connections (close 4001) and wraps send_command pending tracking in try/finally to prevent leaks.
tests/integration/test_websocket.py	Adds integration tests for duplicate handshake rejection + reconnect behavior and for _pending cleanup on timeout and send exceptions.

---

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