Conversation
ROS 2 does not check that a message is self-consistent. An OccupancyGrid whose data length disagrees with width * height, or an Image whose buffer does not match its declared size and encoding, is type-valid and is delivered to every subscriber. Scene then reshaped the buffer, and reshape raises. The raising happened inside request handlers, which is what made it matter. The debug UI answered 500 on /api/state and /api/camera for as long as the malformed message stayed the latest one, so anyone able to publish on /map could hold the interface down. The same contamination reached goal_near through the MCP path, where a resolution of zero also divided by zero. The planner half of this was already fixed by the goal_planner refactor, which rejects a non-positive resolution and a mismatched cell count. Nothing tested either check, so nothing would have noticed them going away again. message_shape states the rule once, as pure functions with no imports: what length a grid or an image of a given size and encoding must carry. Both decode sites ask it, so the rule cannot be applied at one and forgotten at the other, and it can be tested without a ROS or robonix runtime. A malformed frame is dropped with a warning rather than raised: the rest of the page keeps working and the next well-formed message recovers the view. Scene's web UI also now says at startup when it is bound to a non-loopback address, because its annotation endpoints read and write map data and carry no authentication. That is tenable on loopback and is a decision anywhere else, and an operator who did not make it deliberately should know they have. The default bind address is unchanged; web_host: 127.0.0.1 already keeps it local. Reported in #226 and #227, and the warning relates to #229.
The Scene debug UI has no authentication, and its annotation endpoints read and write map data: reaching it is enough to change what the robot believes about its world. It defaulted to every interface, so a deployment exposed that on the network without anyone deciding to. Loopback is now the default in all three places that choose it — the resolver and both launchers. A launcher that kept passing 0.0.0.0 would have made the library default meaningless, which is what the new test checks. The container runs with --network host, so the service's bind is the host's bind and nothing about port publishing changes. SCENE_WEB_HOST=0.0.0.0, or web_host in the Scene config, opens it again in one line, and the service logs a warning whenever it is bound wide so that choice stays visible. The goal-planner and message-shape tests also join the pull-request CI job. They are pure and import neither robonix_api nor the generated MCP types, so they run under the lightweight dependency set already used there. Nothing on a pull request exercised the guards against a malformed occupancy grid until now, which is how those guards came to be missing to begin with. Relates to #229.
github-actions
Bot
added
comp:scene
The two test files this branch added to CI could not be collected there: pytest prepends the directory holding a test file, not the package root, so `from scene_service import ...` only resolves when pytest runs from system/scene. CI runs it from the repository root. Every older test in that directory carries its own sys.path.insert, which is why the four already in the job pass and why test_goal_planner.py could sit in the tree without anything noticing it was never collected. A conftest does it once for the directory.
enkerewpo
marked this pull request as draft
September 13, 2026 11:59
Member
Author
|
need to be tested |
enkerewpo
marked this pull request as ready for review
September 13, 2026 12:10
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.
Fixes #226. Fixes #227. Relates to #229.
What was wrong
ROS 2 does not check that a message is self-consistent. An
OccupancyGridwhosedatalength disagrees withwidth * height, or anImagewhose buffer does not match its declared size and encoding, is type-valid and is delivered to every subscriber. Scene then reshaped the buffer, andreshaperaises.The raising happened inside request handlers, which is what made it matter. The debug UI answered 500 on
/api/stateand/api/camerafor as long as the malformed message stayed the latest one, so anyone able to publish on/mapcould hold the interface down. The same contamination reachedgoal_nearthrough the MCP path, where a resolution of zero also divided by zero.Separately, that UI has no authentication and its annotation endpoints read and write map data, and it defaulted to binding every interface.
What changed
One rule, stated once.
scene_service/message_shape.pysays what length a grid or an image of a given size and encoding must carry. It is pure and imports nothing. Both decode sites ask it, so the rule cannot be applied at one and forgotten at the other, and it can be tested without a ROS or robonix runtime.A malformed frame is dropped, not raised. The rest of the page keeps working and the next well-formed message recovers the view, with one warning naming what was discarded.
The planner half was already fixed. The
goal_plannerrefactor rejects a non-positive resolution and a mismatched cell count, so #227 no longer reproduces. Nothing tested either check, so nothing would have noticed them going away again — that is what the new tests are for.The UI defaults to loopback. All three places that choose the bind address — the resolver and both launchers — now default to
127.0.0.1. A launcher still passing0.0.0.0would have made the library default meaningless, so a test checks that too. The container runs with--network host, so the service's bind is the host's bind and nothing about port publishing changes.SCENE_WEB_HOST=0.0.0.0, orweb_hostin the Scene config, opens it again in one line, and the service logs a warning whenever it is bound wide so the choice stays visible.Tests
system/scene/tests/test_malformed_grid_inputs.py, 21 cases: the rule itself, both planner entry points, and every encoding Scene decodes.These and
test_goal_planner.pynow run in the pull-request CI job. They import neitherrobonix_apinor the generated MCP types, so the lightweight dependency set already there is enough. Nothing on a pull request exercised these guards before, which is how they came to be missing in the first place.Locally: 201 passed against a 200 baseline, with the same 11 pre-existing failures on both sides — all of them
ModuleNotFoundErrorfor workspace and codegen artifacts that are absent outside a built package.Note on #229
Authentication itself is not added here. The endpoints remain open to whoever can reach the port; what changed is that reaching it now takes a deliberate act. Whether this UI should carry real authentication is a larger question than a default, and worth deciding separately.