mirror of
https://github.com/jcreek/CosmicClash.git
synced 2026-09-11 08:23:45 +00:00
feat(multiplayer): Phase 3 task 3.4 - input validation, rate limiting, disconnect policy
MatchSim._recv_input now validates before decoding (§3.1 steps 2-3): per-peer rolling-1s rate limiting (packet count AND byte budget, dropping over-budget packets and disconnecting after 3 consecutive over-budget seconds), and framing validation (redundancy count and payload size checked against NetCodec's own layout before unpack_input ever runs, disconnecting after 20 malformed packets). Framing has to be validated explicitly rather than relying on decode failure: StreamPeerBuffer silently zero-fills past EOF instead of erroring, a finding from Phase 2's adversarial review. networked_match.gd's _on_input_received now rejects any seq claiming to be more than 20 ticks ahead of the current server tick (§3.1 step 4) and counts (rather than silently ignoring) input from a peer with no slot, for observability. Verified with two new permanent regression tests (networked_match_smoke.gd --role=client-abuse-malformed / client-abuse-flood) that call MatchSim._recv_input directly with garbage bytes and a legitimate-but- too-frequent flood, respectively, bypassing the honest client encoder entirely - the same thing a hostile custom client sending raw ENet packets would look like. Both confirm real disconnection, not just that the server tolerates the abuse. Two bugs surfaced by getting these tests to actually pass cleanly: a GDScript lambda-capture-by-value mistake in the tests themselves (a plain `var disconnected := false` mutated inside a signal-handler lambda never became visible to the enclosing function - fixed by capturing a single-element Array instead, which is captured by reference); and a narrow real race where NetworkManager's own ping/pong reply could target a peer that a concurrent abuse-triggered disconnect had just removed from the same poll() batch, now guarded. (Passing disconnect_peer's `force` parameter as an attempted fix for a related one-off benign error was tried and reverted - it made Godot's own peer-list bookkeeping inconsistent, producing hundreds of errors instead of one; verified empirically rather than assumed.) Full regression suite, including the net-sim-latency milestone gate, re-run clean.
This commit is contained in:
@@ -39,6 +39,22 @@ func _ready() -> void:
|
||||
return
|
||||
print("SMOKE: joining ...")
|
||||
MatchNet.welcomed.connect(_on_client_welcomed)
|
||||
"client-abuse-malformed", "client-abuse-flood":
|
||||
# task 3.4's disconnect-abusive-peer paths: joins normally (so
|
||||
# it's a real connected peer, exactly like a hostile custom
|
||||
# client would be — the validation doesn't get to assume
|
||||
# anything about who's on the other end of an authenticated
|
||||
# connection), then deliberately abuses MatchSim._recv_input
|
||||
# directly rather than going through networked_match.gd's own
|
||||
# honest encoder.
|
||||
MatchNet.local_player_name = "Abuser"
|
||||
var err := NetworkManager.join("127.0.0.1", PORT)
|
||||
if err != OK:
|
||||
print("SMOKE FAIL: join() failed: %s" % error_string(err))
|
||||
get_tree().quit(1)
|
||||
return
|
||||
print("SMOKE: joining to abuse (%s) ..." % _role)
|
||||
MatchNet.welcomed.connect(_on_abuser_welcomed)
|
||||
_:
|
||||
print("SMOKE FAIL: missing or unrecognised --role=")
|
||||
get_tree().quit(1)
|
||||
@@ -69,3 +85,13 @@ func _on_client_welcomed() -> void:
|
||||
var hooks := preload("res://tests/networked_match_test_hooks.gd").new()
|
||||
get_tree().root.add_child.call_deferred(hooks)
|
||||
hooks.run_client_check.call_deferred(SETTLE_SECONDS, DRIVE_SECONDS)
|
||||
|
||||
|
||||
func _on_abuser_welcomed() -> void:
|
||||
MatchNet.welcomed.disconnect(_on_abuser_welcomed)
|
||||
var hooks := preload("res://tests/networked_match_test_hooks.gd").new()
|
||||
get_tree().root.add_child.call_deferred(hooks)
|
||||
if _role == "client-abuse-malformed":
|
||||
hooks.run_malformed_abuse_check.call_deferred()
|
||||
else:
|
||||
hooks.run_rate_limit_abuse_check.call_deferred()
|
||||
|
||||
@@ -114,3 +114,66 @@ func run_client_check(settle_seconds: float, drive_seconds: float) -> void:
|
||||
await get_tree().create_timer(0.3).timeout
|
||||
NetworkManager.shutdown()
|
||||
get_tree().quit(0 if success else 1)
|
||||
|
||||
|
||||
# task 3.4: MatchSim._recv_input must count malformed packets and disconnect
|
||||
# after MALFORMED_LIMIT_TO_DISCONNECT (20) of them. Calls the RPC directly
|
||||
# with garbage bytes rather than going through networked_match.gd's own
|
||||
# honest encoder — this IS what a hostile custom client sending raw ENet
|
||||
# packets would look like, so bypassing the normal send path is the point,
|
||||
# not a shortcut.
|
||||
func run_malformed_abuse_check() -> void:
|
||||
await get_tree().create_timer(1.0).timeout
|
||||
# A single-element Array, not a plain bool: GDScript lambdas capture
|
||||
# outer local variables BY VALUE at creation time, not by reference, so
|
||||
# `disconnected = true` inside the lambda below would silently mutate
|
||||
# only the lambda's own captured copy — invisible to this function's
|
||||
# own `disconnected` if it were a plain bool. Mutating an Array's
|
||||
# CONTENTS from inside the lambda works because the Array object
|
||||
# itself (not a copy of it) is what got captured.
|
||||
var disconnected := [false]
|
||||
NetworkManager.disconnected_from_server.connect(func() -> void: disconnected[0] = true)
|
||||
|
||||
for i in 25:
|
||||
MatchSim._recv_input.rpc_id(1, PackedByteArray([1, 2, 3])) # far too short to even hold a header
|
||||
NetworkManager.poll()
|
||||
await get_tree().physics_frame
|
||||
await get_tree().create_timer(1.0).timeout
|
||||
NetworkManager.poll()
|
||||
|
||||
print("SMOKE %s: 25 malformed packets %s" % [
|
||||
"PASS" if disconnected[0] else "FAIL",
|
||||
"resulted in disconnect" if disconnected[0] else "did NOT disconnect the abusive peer",
|
||||
])
|
||||
get_tree().quit(0 if disconnected[0] else 1)
|
||||
|
||||
|
||||
# task 3.4: MatchSim._recv_input must rate-limit and disconnect after
|
||||
# RATE_LIMIT_OVER_BUDGET_SECONDS_TO_DISCONNECT (3) consecutive seconds over
|
||||
# RATE_LIMIT_PACKETS_PER_SEC (110/s). Every packet here is individually
|
||||
# well-formed (a real NetCodec.pack_input payload) — only the SEND RATE is
|
||||
# abusive, confirming the rate limiter fires independently of the malformed-
|
||||
# packet counter, not as a side effect of it.
|
||||
func run_rate_limit_abuse_check() -> void:
|
||||
await get_tree().create_timer(1.0).timeout
|
||||
var disconnected := [false] # see run_malformed_abuse_check's comment on why not a plain bool
|
||||
NetworkManager.disconnected_from_server.connect(func() -> void: disconnected[0] = true)
|
||||
|
||||
var net_codec := preload("res://scripts/net_codec.gd")
|
||||
var ship_action_script := preload("res://scripts/ship_action.gd")
|
||||
var bytes: PackedByteArray = net_codec.pack_input(1, 0, Time.get_ticks_msec(), [ship_action_script.new()])
|
||||
|
||||
var deadline_ms := Time.get_ticks_msec() + 4000
|
||||
while Time.get_ticks_msec() < deadline_ms and not disconnected[0]:
|
||||
for i in 40: # well above 110/s once summed across a frame's worth of iterations
|
||||
MatchSim._recv_input.rpc_id(1, bytes)
|
||||
NetworkManager.poll()
|
||||
await get_tree().process_frame
|
||||
await get_tree().create_timer(0.5).timeout
|
||||
NetworkManager.poll()
|
||||
|
||||
print("SMOKE %s: sustained packet flood %s" % [
|
||||
"PASS" if disconnected[0] else "FAIL",
|
||||
"resulted in disconnect" if disconnected[0] else "did NOT disconnect the abusive peer",
|
||||
])
|
||||
get_tree().quit(0 if disconnected[0] else 1)
|
||||
|
||||
Reference in New Issue
Block a user