fix(gst): check property values against the spec instead of panicking

[wagenet]

The bug

A flow definition names both a property and its value, and properties.rs handed whatever arrived straight to GLib whenever it did not recognise the property's type:

} else {
    // Try i64, might work
    element.set_property(prop_name, *v);
}
...
} else {
    // Property not found, try anyway
    element.set_property(prop_name, *v);
}

GLib's setters report every kind of misuse by panicking, so:

POST /api/flows        {"id":"src0","element_type":"videotestsrc","properties":{"pattern":1}}
POST /api/flows/{id}/start

panicked at backend/src/gst/pipeline/properties.rs:140:33:
property 'pattern' ... (expected: 'GstVideoTestSrcPattern', got: 'gint64')

Every arm had that shape, not just Int. PropertyValue::String was no safer — it went through set_property_from_str, which is find_property(..).unwrap() followed by Value::deserialize_with_pspec(..).unwrap(). So four distinct ways a request body reached a panic:

what the client sends	what GLib does
a value of the wrong type	validate_property_type panics on the type check
a property the element does not have	find_property().unwrap() panics
a property that is read-only or construct-only	validate_property_type panics on the flags check
a value the spec would clamp (num-buffers: -5, an out-of-range enum integer)	g_param_value_validate reports the clamp, glib-rs panics because it happened

The last row is the easy one to miss: a type check alone does not catch it.

Is it a denial of service?

Not a process kill, but worse than a bad error message.

• No panic = "abort" in any profile and no CatchPanicLayer on the router, so the panic unwinds.
• axum-server 0.8 serves each accepted connection in a tokio::spawned task, so tokio catches the unwind at the task boundary. The worker thread and the process survive.
• The client gets a dropped connection and no HTTP response.
• start_flow's error path never runs. On a clean error it calls teardown_flow; on a panic it does not, so the half-built pipeline and whatever the blocks registered on the way are leaked. Every repeat of the request leaks again.

Reachable by anything that can talk to the API — authenticated when auth is configured, unauthenticated otherwise.

The fix

Resolve the GParamSpec first and convert against it (checked_property_value), returning PipelineError::InvalidProperty for every row of that table.

No value shape that applied before stops applying. Enums, flags and the ranged numeric types have typed conversions, so an integer names an enum or flags member as well as its nick string does: {"pattern": 1} is as good as {"pattern": "snow"}, and one that names no real member is a clean error listing the alternatives:

{"error": "Failed to start flow",
 "details": "Invalid property value for src0.pattern: 99999 is not a valid GstVideoTestSrcPattern value (valid: 0=smpte, 1=snow, 2=black, ...)"}

Everything else — a boolean written as 0/1, an integer for a GstFraction, caps, structures — goes to GStreamer's own deserializer in its text form, which is the conversion set_property_from_str performed, minus its unwrap. That matters most for pad properties: linking.rs only logs one it cannot set, so a rejection there would start the flow misconfigured rather than report anything.

What the deserializer returns is checked against the spec too. gst_value_deserialize parses text into the property's type but never consults the GParamSpec, so {"num-buffers": "-5"} came back happily and was caught only by the catch_unwind, while {"num-buffers": -5} gave a clean message. Both now say:

Invalid property value for src0.num-buffers: Value -5 is out of range for gint (-1..=2147483647)

The same check covers a flags string with bits the class does not define (rtspsrc protocols=0xff0 deserializes fine and GLib then rejects it).

The set itself is still wrapped in catch_unwind. A GParamSpec subclass can carry validation of its own that it only reports by panicking, and this module's guarantee should not rest on having enumerated every one; gst/discovery.rs already uses catch_unwind the same way. With the range check in place it is a backstop rather than the thing an ordinary request runs into.

start_flow now answers 400 instead of 500 for a property the client got wrong, matching the live property-update path, and 404 for an unknown flow id — that case returned InvalidFlow("Flow not found: ..."), which the handler mapped to 500 while openapi.json advertised 404. It gets a PipelineError::FlowNotFound of its own rather than widening InvalidFlow, which also carries block-expansion failures that are not 404s. openapi.json is regenerated for the added 400 response.

Left alone

get_element_property has a dead "GEnum" match arm — an element's enum property reports its own GType name (GstVideoTestSrcPattern), never "GEnum", so enum properties fall through to "Unsupported property type" and are silently dropped from GET .../properties. The pad version of the same function gets it right (value_type().is_a(glib::Type::ENUM)). It is a read-path bug with no panic in it, and fixing it changes what the API returns, so it belongs in its own PR.

The other InvalidFlow("Flow not found: ...") sites (block property updates, and the live-update handlers) keep their current status mapping. This PR only touches the handler whose mapping it is already changing.

Tests

Both layers call the real module.

backend/tests/property_type_panic_test.rs — 6 tests driving PipelineManager::new the way start_flow does: out-of-range enum integer, in-range enum integer accepted (and the nick form still working), unparseable string for a gint property, an integer below the spec minimum, an unknown property, plus a control that a valid flow still builds, starts and stops.

backend/src/gst/pipeline/properties.rs — 16 unit tests on checked_property_value itself, asserting the exact glib::Value type and contents: the enum member, gint range, whole vs. fractional float, gdouble built from an integer (the mixer path), a gboolean built from 0/1, a gboolean pad property on an audiomixer sink_%u pad, "-5" rejected for num-buffers, and stray bits rejected for rtspsrc protocols.

Reverting the fix fails the tests

With properties.rs restored to its pre-fix state and the test file kept, 5 of the 6 fail and the control (valid_properties_still_build_and_start) passes, so they are not merely asserting "everything fails now":

thread 'out_of_range_enum_integer_is_rejected' panicked at
  backend/src/gst/pipeline/properties.rs:140:33

That is the exact line from the report.

Dropping just the post-deserialize range check (the check_against_spec call) fails integer_property_rejects_an_out_of_range_string and flags_property_rejects_stray_bits_in_a_string, and leaves the other 14 green — verified this way rather than assumed.

CI

videotestsrc, audiomixer and fakesink come from gstreamer1.0-plugins-base and rtspsrc from gstreamer1.0-plugins-good, both already in the package list of every test job in .github/workflows/ci.yml, so these run in CI rather than skipping. The test uses the repo's STROM_REQUIRE_GST_PLUGINS guard, read through strom_types::env::var_opt so a blank value means unset.

What was actually run (macOS, debug)

• cargo test over the whole backend: 620 passed, 0 failed, 1 ignored (the ignored one is pre-existing in jitterbuffer_mute_test)
• cargo test --test property_type_panic_test: 6 passed
• cargo test --lib gst::pipeline::properties: 16 passed
• cargo test --test openapi_test: passes, and openapi.json needed no further change
• cargo clippy --all-targets --features efp,nvidia -- -D warnings, cargo clippy --workspace --all-targets -- -D warnings: clean
• cargo fmt --all -- --check: clean
• Against a fresh headless server: an unknown flow id returns 404; {"pattern": 99999}, {"num-buffers": "-5"}, {"num-buffers": -5} and an unknown property each return 400 (both spellings of -5 with the same message); {"pattern": 1}, {"is-live": 1} and pad_properties: {"sink_0": {"mute": 1}} on an audiomixer each start 200. No panics in the log and the server stayed up.

Rebased onto current main.

Not run: the Linux and Windows CI jobs, and anything needing hardware this machine does not have.

---

Authored by Claude (Claude Code).

🤖 Generated with Claude Code

[wagenet]

This all seems reasonable to me.

[srperens]

Verdict: Approve — correct diagnosis of a real DoS-adjacent bug (panic mid-start_flow skips teardown_flow, leaking the half-built pipeline on every repeat), fix is at the right layer, and both new test layers genuinely exercise the changed code.

Claims

Claim	Verdict	Evidence
Old code panicked via GLib on unwritable/wrong-type/out-of-range property values	CONFIRMED	Pre-fix set_property (properties.rs:20, replaced) called element.set_property(prop_name, *v) unconditionally in the _ => and "not found" arms with no spec check first — set_property panics on type/flags/range mismatch per glib-rs
Panic previously skipped teardown_flow, now a returned Err runs it	CONFIRMED	set_checked_value (properties.rs, new) returns Result instead of panicking; state.rs:864 start_flow propagates PipelineError, and construction failure paths call self.teardown_flow(...) at state.rs:941/1029/1071 — a normal Err return (not an unwind) reaches those call sites
catch_unwind/AssertUnwindSafe around a GLib setter matches existing repo pattern, not a new risk class	CONFIRMED	backend/src/gst/discovery.rs uses the identical std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {...})) shape at 25+ call sites already
start_flow now answers 400 (not 500) specifically for InvalidProperty/PropertyNotMutable	CONFIRMED	backend/src/api/flows.rs:573-579, match e { PipelineError::InvalidProperty{..} | PipelineError::PropertyNotMutable{..} => StatusCode::BAD_REQUEST, _ => StatusCode::INTERNAL_SERVER_ERROR }
New tests exercise the real fix and would fail if reverted	CONFIRMED	property_type_panic_test.rs drives PipelineManager::new (the start_flow path); guarded by STROM_REQUIRE_GST_PLUGINS=1 (set in ci.yml:132,596) which turns a missing-videotestsrc skip into a hard failure rather than a silent pass

Diagnosis — root cause matches: every arm of the old type-dispatch match ended in an unchecked set_property/set_property_from_str call, so any of (missing property, read-only/construct-only, wrong type, out-of-range/clamped value) reached GLib unchecked. The fix resolves the GParamSpec once and converts against it (checked_property_value), covering the full class the PR's own table lists, not just the reported pattern case — enum-as-integer, flags-as-bitmask, and ranged numeric types all get spec-checked before any GLib call. The residual catch_unwind is correctly framed as a backstop for a GParamSpec subclass with private validation, not the primary mechanism.

Radius — SHARED: set_property/set_pad_property are the only property-setting paths for both the flow-creation (start_flow) and live-update code paths, both are touched, both get equivalent checked-conversion + catch_unwind treatment. No strom-types or WebSocket contract change. get_element_property's dead "GEnum" arm is explicitly left alone with a stated reason — correct call, since fixing a read path changes API output and belongs in its own PR.

Tests & CI — Build (Linux x86_64/ARM64), Check (Linux), Check & Build (WASM), API Contract Check all pass at ccac5661. cargo test --package strom (which includes the new property_type_panic_test.rs and the 12 properties.rs unit tests) runs inside Check (Linux) per ci.yml:128-129. Build (macOS)/Build (Windows) skip per the usual gate; this diff has no platform-cfg code, so that's not a coverage gap here.

Confidence: HIGH

[srperens]

Verdict: Request changes — the element path is sound and the panic it closes is real, but the pad path drops a value shape it used to accept, and that failure only logs, so the flow starts misconfigured instead of erroring.

Requested changes

1. Route a non-string value for a non-numeric target through to_string() + deserialize_with_pspec instead of rejecting it — backend/src/gst/pipeline/properties.rs:938 — _ => Err(format!(. That is what the removed pad.set_property_from_str(prop_name, &value_str) did for every value shape, so a boolean pad property regresses: GStreamer accepts 1 for one (gst-launch-1.0 audiotestsrc num-buffers=5 ! audiomixer name=m sink_0::mute=1 ! fakesink ran clean on 1.28.6), so pad_properties: {"sink_0": {"mute": 1}} used to apply and now returns Err — which backend/src/gst/pipeline/linking.rs:620 — "Failed to set pad property {}:{}:{}: {}", only logs before continuing. Same for an integer on a GstFraction pad property.

2. Range-check the value deserialize_with_pspec returns — backend/src/gst/pipeline/properties.rs:835 — return glib::Value::deserialize_with_pspec(v, pspec). {"num-buffers": "-5"} deserializes, reaches GLib and is saved only by the catch_unwind: I get a 400 detailed GLib rejected the value (see the log for details) plus a panic on stderr, where {"num-buffers": -5} gives Value -5 is out of range for gint (-1..=2147483647). Two spellings of one value should not differ, and it leaves catch_unwind the belt-and-braces its doc comment describes rather than load-bearing for an ordinary request.

3. Use strom_types::env::var_opt in the skip guard — backend/tests/property_type_panic_test.rs:40 — std::env::var("STROM_REQUIRE_GST_PLUGINS").is_err(),. Presence-only semantics on a STROM_ variable is what types/src/env.rs exists to forbid, and the three sibling guards already use var_opt.

4. Optional and pre-existing, but this diff is this handler's status mapping: add an InvalidFlow → 404 arm. backend/src/state.rs:877 — error!("Flow not found: {}", id); precedes it, so an unknown id answers 500 while openapi.json advertises 404.

Claims

Claim	Verdict	Evidence
Pad properties take the same path, existing behaviour kept	CONTRADICTED	requested change 1
Nothing reaches GLib unchecked against the spec	CONTRADICTED	requested change 2
start_flow answers 400, not 500, for a property the client got wrong	CONFIRMED	headless run on this branch: {"pattern": 99999} → 400 naming the valid enum values, zero panics; the same request against main's properties.rs → no HTTP response at all, panic on tokio-rt-worker, no teardown line
The new test fails if the fix is reverted	CONFIRMED	main's properties.rs with this test file: 5 of 6 fail, incl. num-buffers: -5 on the clamp path a type check misses
The new tests execute in CI rather than skipping	CONFIRMED	.github/workflows/ci.yml:132 — STROM_REQUIRE_GST_PLUGINS: 1

Diagnosis — Right layer: the spec is resolved before the value is built, not a panic hook or a catch at the handler. Coverage is BOUNDED to properties named in a flow definition and in the live update path.

Radius — SHARED: every flow start reaches set_property through add_element, and every live property update calls the same function. No signature outside properties.rs changes.

Tests & CI — Green at ccac566. On this branch with main merged: cargo test --workspace (657 pass), cargo clippy --all-targets --workspace, cargo fmt --check, --test openapi_test, and STROM_REQUIRE_GST_PLUGINS=1 --test property_type_panic_test (6/6 executed, none skipped). No platform code, so no macOS/Windows dispatch needed.

Confidence: HIGH

Superseded by a later re-review at the same head (review 5101543080, verdict Request changes): the pad-property path silently drops values it used to accept and an out-of-range numeric string reaches GLib unchecked. An older Approve must not stand beside a current Request changes verdict per protocol v3 (scripts/agent/PROTOCOL.md). Dismissing per protocol, not new disagreement.

[wagenet]

All four addressed, on d9b67b5 (rebased onto current main).

1. Non-string value for a non-numeric target — the _ arm now runs deserialized_value(pspec, &text_form(prop_value), target_name) instead of returning Err, and the Bool arm falls through to it rather than erroring when the target is not gboolean. So pad_properties: {"sink_0": {"mute": 1}} applies again, as does an integer for a GstFraction. Live against audiomixer: both {"mute": 1} and {"mute": true} start 200 and log Set pad property mix:sink_0:mute. {"name": 5} on a gchararray also goes back to being accepted as "5" — same rule, and the test that asserted a rejection is now a_number_names_a_string_property.

2. Range-check what the deserializer returns — deserialized_value runs check_against_spec on the result: the flags mask, and min/max for each ranged numeric type. {"num-buffers": "-5"} and {"num-buffers": -5} now both give Value -5 is out of range for gint (-1..=2147483647), with no panic on stderr. The flags mask check moved here too, so the string spelling is covered as well — rtspsrc protocols=0xff0 deserializes fine and GLib rejects it, which the new flags_property_rejects_stray_bits_in_a_string pins. Enums need no check at this layer: gst_value_deserialize_enum resolves through the enum class and fails on a name, nick or integer that is not a member.

Dropping just the check_against_spec call fails those two tests and leaves the other 14 green, so they guard the check rather than the surrounding conversion.

3. var_opt in the skip guard — done, matching the three siblings.

4. InvalidFlow → 404 — taken, but as a PipelineError::FlowNotFound used at the start_flow lookup rather than by widening InvalidFlow, which also carries block-expansion failures that should stay 400/500. An unknown id now returns 404 {"error":"Flow not found",...}. The other InvalidFlow("Flow not found: ...") sites keep their current mapping — this PR only touches the handler whose mapping it was already changing.

Full run on macOS: 620 passed / 0 failed / 1 ignored (pre-existing), clippy clean on both feature sets, fmt clean, openapi.json needed no further change. Test counts and the live-server results are in the updated PR body.

Posted by Claude (Claude Code).

[srperens]

This supersedes my CHANGES_REQUESTED review of ccac566 — verdict changes to Approve. All four requested changes are present at the new head and each is backed by a test that targets exactly the gap I found.

Claims

Claim	Verdict	Evidence
Pad/element properties whose value doesn't match String/enum/flags/ranged-numeric now go through the deserializer instead of erroring (req. change 1)	CONFIRMED	backend/src/gst/pipeline/properties.rs:920 — _ => deserialized_value(pspec, &text_form(prop_value), target_name), — and backend/src/gst/pipeline/properties.rs:1268 — fn pad_boolean_property_accepts_an_integer() { exercises exactly the sink_0::mute=1 case I cited
Every deserialized value is range-checked, closing the two-spellings-of--5 gap (req. change 2)	CONFIRMED	backend/src/gst/pipeline/properties.rs:939 — check_against_spec(pspec, &value, target_name)?; runs unconditionally inside deserialized_value, the sole path checked_property_value uses for anything not Bool/enum/flags/ranged-numeric; backend/src/gst/pipeline/properties.rs:1285 — fn integer_property_rejects_an_out_of_range_string() { is the String-spelling twin of the existing Int one
Skip guard uses strom_types::env::var_opt, not presence-only env::var (req. change 3)	CONFIRMED	backend/tests/property_type_panic_test.rs:38 — strom_types::env::var_opt("STROM_REQUIRE_GST_PLUGINS").is_none(),
Unknown flow id on start_flow now answers 404, not 500 (req. change 4)	CONFIRMED	backend/src/state.rs:878 — return Err(PipelineError::FlowNotFound(id.to_string())); and backend/src/api/flows.rs:576 — PipelineError::FlowNotFound(_) => (StatusCode::NOT_FOUND, "Flow not found"),

Diagnosis — Unchanged from my first pass: checked_property_value resolves the GParamSpec before any GLib call, covering the full class (missing property, read-only/construct-only, wrong type, out-of-range/clamped). The fixes in this round close the two paths that still reached GLib unchecked or rejected a previously-valid shape — coverage is now ABSOLUTE for the property-setting surface this PR touches (element and pad properties, both construction and live-update).

Radius — SHARED, and both call sites get identical treatment: backend/src/gst/pipeline/properties.rs:20 — pub(super) fn set_property( (used by construction.rs:367 at flow start, and by update_element_property for the live-update path) and backend/src/gst/pipeline/properties.rs:560 — pub(super) fn set_pad_property( (used by linking.rs:617) both convert through the same checked_property_value — so there is one conversion function, not two that could drift. No strom-types/WebSocket contract change; openapi.json's new 400 response is registered and API Contract Check is green.

Tests & CI — Build (Linux x86_64/ARM64), Check (Linux), Check & Build (WASM), API Contract Check all pass at 5eb9cc0. STROM_REQUIRE_GST_PLUGINS=1 (.github/workflows/ci.yml:132) turns the test file's plugin-missing skip into a hard failure, so the new tests execute rather than skip. Build (macOS)/Build (Windows) skip as usual; no platform-cfg code in this diff, so that's not a coverage gap here.

No CLAUDE.md violation found (no new #[allow(dead_code)], no emoji in log macros).

Confidence: HIGH

Superseded by re-review at 5eb9cc0 — the CHANGES_REQUESTED review targeted an older head (ccac566); all four requested changes are addressed and the verdict is now Approve. Dismissing because the protocol requires this, not because the original review was wrong.