Default SimulationServer to loopback-only, add test coverage - #284
Open
jaisinha77777 wants to merge 1 commit into
Open
Default SimulationServer to loopback-only, add test coverage#284jaisinha77777 wants to merge 1 commit into
jaisinha77777 wants to merge 1 commit into
Conversation
SimulationServer binds with socketserver.ThreadingTCPServer(('', port),
...), where an empty host string means "all interfaces" -- so by
default this server is reachable from any other machine on the
network, not just the local one running the simulation.
That matters because every endpoint is unauthenticated. In particular,
POST /cmd/set_component_state lets a caller overwrite arbitrary
component state on any entity in the running simulation, and
/play, /pause, /step, /stop give full remote control over execution --
all with Access-Control-Allow-Origin: * and no credential check.
Binding to all interfaces by default means anyone else on the same
network segment can reach these.
Adds a host constructor parameter, defaulting to '127.0.0.1'
(loopback-only), matching the secure-by-default convention used by
comparable local dev servers (Jupyter, TensorBoard, Flask's dev
server). Passing host='0.0.0.0' explicitly still allows opting into
LAN/remote access for anyone who actually wants that.
Also adds simulation_server_test.py, which had no coverage: default
host, host/port/bound_port properties, broadcast_step/
broadcast_entity_info behavior (including dropping full SSE queues),
and the actual HTTP endpoints exercised end-to-end against a live
server on an OS-assigned port.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
`concordia/utils/simulation_server.py` (a debug/visualization HTTP server for stepping through a running simulation) starts with:
An empty host string binds to all network interfaces, not just loopback. That's a real exposure here specifically because every endpoint is unauthenticated:
None of that is gated by any credential or token. Binding to `0.0.0.0` by default means anyone else on the same network segment (same WiFi, same cloud VPC, same container network) can reach all of this, not just the person running the simulation on their own machine.
(Side note: I found this while adding test coverage — this module currently has zero test files, part of the ongoing #205 effort. It also isn't imported anywhere else in the repo, so nothing currently instantiates it with the old default; this is a library-level fix to what it does when someone does use it.)
Fix
Adds a `host` constructor parameter, defaulting to `'127.0.0.1'` (loopback-only) — matching the secure-by-default convention used by comparable local dev servers (Jupyter, TensorBoard, Flask's/Django's dev servers). Anyone who actually wants LAN/remote access can still opt in explicitly with `host='0.0.0.0'`.
Also added `host`, `port`, and `bound_port` properties (the last exposes the OS-assigned port when starting with `port=0`, used by the new tests).
Test plan
New `concordia/utils/simulation_server_test.py` (previously no coverage at all):
`python -m pytest concordia/utils/simulation_server_test.py -q` → 18 passed.
`python -m pyink --check` on both changed/new files → clean.