Add consistent validation to protocol message parsing#77
Merged
Conversation
Assign locally-built parse results (metadata, controller, player, artwork, visualizer, and stream end/clear roles) into their destination message structs with std::move, avoiding a copy of their heap-allocated vectors and strings. Trivially copyable results are left as plain copies.
Add read_uint_field<T> and read_bool_field helpers that read optional scalar fields with a strict is<T>() check (rejecting floats and numeric strings), optional [min, max] range bounds, and warn-and-drop on malformed values. Apply them across the parser (player, artwork, command, controller, visualizer, metadata progress, hello version), clamping volume to the protocol maximum of 100. Route the metadata tri-state string/uint16 helpers through the same warn-on-malformed path. Malformed values of known fields are dropped with a log instead of silently coerced; required fields that fail validation reject the message via the existing completeness checks.
Refactor parse_color_field to read each RGB component with read_uint_field<uint8_t> instead of grabbing it as an int and manually range-checking 0-255. Since uint8_t's range is exactly 0-255, the strict type check is also the range check, and malformed components now log through the same uniform format as the other fields.
Add read_enum_field<E> that parses an optional enum from a wire string via a from_string function, warning and dropping a present non-string or an unrecognized value. Apply it to the fields whose policy is apply-if-valid-else-leave: artwork source/format, controller repeat, group playback_state, and each supported_commands element. The controller role is frozen at v1, so an unrecognized command is a non-compliant value rather than a forward-compatible one. Fields with different policies keep their handling: codec maps unknown values to the UNSUPPORTED sentinel; required command and connection_reason reject the message; visualizer types (a draft role) skip unknowns silently for forward compatibility.
This doesn't add full support where we can properly send these commands. It just adds them to the known list so the newly added enum validation fields won't fire on a newer server.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors protocol message parsing to centralize scalar/enum decoding behind helper functions that enforce stricter JSON typing and bounds checking, and updates controller command handling to recognize new seek-related commands.
Changes:
- Introduces
read_uint_field,read_bool_field, andread_enum_fieldhelpers to validate JSON types/ranges and drop malformed optional fields with warnings. - Adjusts multiple message parsers to use the new helpers and moves parsed objects into message structs to avoid copies.
- Adds
seekandseek_relativeto the controller command enum and round-trip parsing/formatting, with new tests covering stricter validation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_protocol.cpp | Adds tests for strict scalar/enum validation (range/type/float rejection) and controller command round-trip updates. |
| src/protocol.cpp | Centralizes scalar/enum parsing via helpers; applies warn-and-drop behavior and some move optimizations in message processing. |
| src/protocol_messages.h | Extends controller command string conversions to include seek / seek_relative. |
| include/sendspin/controller_role.h | Extends public controller command enum with SEEK and SEEK_RELATIVE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
466
to
474
| // Parse optional playback_state | ||
| JsonVariantConst playback_state_var = root["payload"]["playback_state"]; | ||
| if (playback_state_var.is<const char*>()) { | ||
| std::string state_str = playback_state_var.as<std::string>(); | ||
| group_msg->group.playback_state = playback_state_from_string(state_str); | ||
| } else if (!playback_state_var.isUnbound() && playback_state_var.isNull()) { | ||
| if (!playback_state_var.isUnbound() && playback_state_var.isNull()) { | ||
| // Field set to null - clear from state | ||
| group_msg->group.playback_state = std::nullopt; | ||
| } else if (auto state = read_enum_field(playback_state_var, "playback_state", | ||
| playback_state_from_string)) { | ||
| group_msg->group.playback_state = *state; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I believe this is a gap in the spec. Every client is always part of one group, so these fields should never be cleared.
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
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.
Centralizes scalar/enum field parsing in protocol.cpp behind three helpers (
read_uint_field,read_bool_field,read_enum_field) that enforce strict JSON types, range bounds, and warn-and-drop on malformed input. This replaces ad-hocas<T>()casts that silently truncated out-of-range values (e.g. volume: 300 → 44).playback_state, artworksource/format,supported_commands) drop unrecognized values instead of clearing the field.seek/seek_relativeto the controller command enum (recognition/round-trip only).New tests cover range validation, type strictness, float rejection, required-scalar rejection, and element-wise array validation.