audit - #305
Merged
Merged
audit#305
Conversation
ConversionContext::default() carries a hardcoded 1920x1080 viewport. Every caller that knows the video's real dimensions was free to forget it, and two did: both geometry-validation call sites resolve vw/vh against the phantom default while the render path builds a real context. On a 1080x1920 vertical render that inverts the two axes inside the validator, so the mandatory pre-delivery gate reports overflow that never happens and misses overflow that does. for_viewport() is the single constructor those callers now share, so the validator and the renderer cannot drift apart again. The private helper that already did this in the render path (engine/render/scene.rs) is superseded by it. Rejected: making default() unavailable outside tests. Roughly forty call sites in the test suites legitimately want an arbitrary viewport, and churning them would bury the fix.
A newer stable clippy adds `chunks_exact_to_as_chunks`, which fires on every `chunks_exact` call whose size is a literal. The workspace has 32 of them across 20 files, so the clippy job fails on every branch — main included. Its last run, from before this work existed, was already red for the same reason. The replacement is clippy's own suggestion, `as_chunks::<N>().0.iter()`. It is not a pure substitution: the iterator now yields `&[T; N]` rather than `&[T]`, so two sites needed real edits — an assertion comparing against an array literal, and a predicate taking `&[u8]`. The array type is the better one anyway; it is what makes the chunk length visible to the compiler. Completeness is checkable rather than argued: no `chunks_exact` with a literal size remains in the workspace, so the lint has nothing left to fire on. The local toolchain predates this lint, so that grep is the verification, not a local clippy run.
…antier/audit-2026-09
…#223) bleed is derived only from node.css.filter (FilterFn::Blur / DropShadow in filter_bleed, line 610). It ignores the node's own box_shadow, which step 5 paints *inside* this layer (line 411-418) at layout.x + dx - spread, width + spread*2 — i.e. outside the border-box — and it ignores the whole subtree painted at steps 9-10, which with the default overflow: visible may legitimately extend past the parent box (absolutely-positioned children, a child's own scale/pulse transform, a child glow, marquee, which CLAUDE.md explicitly documents as "exempté (leur rôle est de bleed)"). The file's own comment on filter_bleed (line 605-609) states the rule: "a *too-tight* one would silently clip filter bleed, trading a perf bug for a correctness one". Concretely: a card with box-shadow: 0 20px 40px rgba(0,0,0,.5) loses its shadow the moment any fade_in drives opacity below 1.0, and regains it on the frame opacity reaches 1.0 — a visible pop mid-entrance. The repo already treats this exact invariant as load-bearing for overflow: hidden (test overflow_hidden_does_not_clip_own_outset_box_shadow, line 2909); the opacity layer violates it for the same shadow. Fix: Compute the layer bounds from the union of: the border-box, the filter bleed, the outset box_shadow extents (|offset| + blur*1.5 + spread per shadow), and — when overflow is visible — the descendants' layout union. Alternatively move the outset box-shadow painting outside the opacity layer and multiply its paint alpha by opacity, and fall back to an unbounded layer when overflow: visible and children exist. Refs #220
ConversionContext::default() carries LengthContext { viewport_width: 1920.0,
viewport_height: 1080.0, .. } (rustmotion-core/src/css/units.rs:66-74), and
taffy_bridge::size_to_dim/lp_to_lp/lp_to_lp_auto resolve every vw/vh/rem/em
against it (taffy_bridge.rs:335-338). The renderer does NOT:
render_with_new_pipeline_iter (engine/render/scene.rs:554-558) and
render_scene_hits (scene.rs:792) both pass viewport_conversion_context(vw,
vh), whose own doc comment calls the 1920×1080 default a 78% error on a
1080×1920 vertical video. So on any non-1920×1080 scenario the mandatory
rustmotion validate gate checks a layout the renderer never produces: width:
"50vw" is measured as 960px by the validator and painted as 540px, 50vh as
540px vs 960px. The file header promises the opposite ("so the geometry it
checks matches what the renderer will actually paint"). The same file builds
a correct LengthContext with the real viewport at line 477 for its
transform-origin math, so the omission is confined to the layout pass. Both
the resting walk (:180) and the --strict-anim walk (:1347) are affected.
Fix: Make viewport_conversion_context public (or move it next to run_layout
in rustmotion-core) and call it from both geometry.rs sites:
run_layout(&built.root, viewport_f,
&viewport_conversion_context(viewport_f.0, viewport_f.1)). Better: stop
exposing ConversionContext at the run_layout signature at all — derive it
from the viewport argument inside run_layout, which makes the wrong context
unrepresentable.
Refs #220
When render_frame_task fails mid-encode (line 613 sets pipe_error and breaks), the code drops stdin and then *waits* for ffmpeg. ffmpeg sees a clean EOF on pipe:0, finalizes the frames it already received, writes the moov atom and exits 0. rustmotion then returns Err, but output_path now holds a structurally valid MP4 containing only the first N frames. Because ffmpeg_args emits -y (line 194), this also silently destroys a previously- good render at the same path. A user scripting rustmotion render who checks only file existence (or whose CI publishes the artifact) ships a truncated video. No code path anywhere in src/cli or src/encode removes the output on failure — only test code calls remove_file on outputs. Fix: On the pipe_error path, call child.kill() and child.wait() before returning, and let _ = std::fs::remove_file(output_path); on both the pipe_error and !status.success() branches. Better still, have ffmpeg write to a sibling scratch path and fs::rename onto output_path only after a clean exit — the same promote-on-success discipline video_audio.rs::partial_wav_path already applies to cached WAVs. Refs #220
Triggering scenario: a scenario with one gif component, rendered to MP4. encode_with_ffmpeg (ffmpeg.rs:582) dispatches the FIRST chunk of batch_size = rayon::current_num_threads() * 2 frames — all frames 0..N of the same scene — to par_iter() at once. Every one of those workers calls Gif::paint_content, every one misses the still-empty gif_cache, and every one runs decode_composed_frames independently. Unlike icons and videos, GIFs are NOT preloaded (preload.rs only walks Component::Icon and Component::Video), so the first decode genuinely happens inside the parallel section. Each decode is itself unbounded: decode_composed_frames pushes one full-canvas RGBA copy per GIF frame with no cap (gif.rs:141). A 600-frame 800x600 GIF is 1.15 GB per decode; on a 16-core machine (batch_size 32) that is ~37 GB attempted concurrently, then 31 of the 32 results are thrown away when insert overwrites. The process OOM-kills before frame 33. Fix: Decode GIFs in the preload pass alongside icons and videos (preload.rs), so the parallel section only ever hits a warm cache. If lazy decode must stay, gate it on a per-key single-flight primitive (e.g. DashMap<String, Arc<OnceLock<GifData>>> — insert the empty cell under the shard lock, then let exactly one worker fill it) and cap total decoded frames per source. Refs #220
tile_spacing returns one scalar that line 830 applies to both axes, and the
Heropattern arm never reads d.height. The function's own doc comment states
the invariant it relies on — "since every tiled pattern is exactly periodic
on spacing, translating by any offset congruent mod spacing produces byte-
identical pixels" — which is false for any non-square pattern. I counted the
table programmatically: 41 of the 87 entries in crates/rustmotion-
core/src/engine/heropatterns.rs have width != height (architect 100x199,
aztec 32x64, bamboo 16x32, bank_note 100x20, death_star 80x105, yyy
60x96...). Concrete case: {"animated-background":{"preset":"heropattern","he
ropattern":{"pattern":"aztec"},"direction":"down","speed":60}}. The shader
tiles vertically on 64px but the offset wraps at 32px, so every 32px of
travel the pattern snaps by half a period — a periodic jolt through the
whole scene. The .max(20.0) clamp breaks it a second way for the 8 patterns
narrower than 20px: bamboo (16 wide) wraps at 20, so even horizontal scroll
jumps 4px per wrap. draw_world_bg_with_parallax reuses the same tile_spacing
for the camera-pan modulo (line 89), so world views inherit it.
Fix: Make tile_spacing return a (f32, f32) period and have the Heropattern
arm emit (d.width * cfg.scale, d.height * cfg.scale); wrap raw_x on the x
period and raw_y on the y period at line 830, and do the same for the camera
modulo at line 89. Drop the .max(20.0) floor or replace it with a multiple-
of-period ceiling (period * (20.0 / period).ceil()) so the clamp preserves
periodicity.
Refs #220
…252) Executed: <scene duration="2" freeze_at="1" world-position="0,0" animated- background="halo"> transpiles to a scene carrying only duration and layout — three documented scene features (CLAUDE.md documents all three) are discarded with no warning and validate exits 0. Same on the root: <rustmotion codec="prores" durationn="5"> drops both. Same on containers/text: <div gap="32" width="400" id="x"> keeps nothing but style/anim (element.rs:101-112 and :92-100 never iterate attrs). This is the exact failure mode check_component_attrs exists to prevent, but that check runs on the already-transpiled ResolvedScenario (validate_attrs.rs:79 check_component_attrs(scenario: &ResolvedScenario)) and only over scene.children — an attribute the transpiler never emitted is invisible to it, so the workspace's only unknown-attribute net has a blind spot precisely over the HTML front-end's own attribute surface. rm-* custom elements are the asymmetric exception: they forward everything and do get caught. Fix: After reading the known keys in scene_to_value and html_to_scenario_value, diff against the consumed set and return a named error (with did-you-mean, mirroring validate_attrs) for the remainder — class/id/data-* can be allowlisted as deliberately inert. Do the same for TagKind::Container/TagKind::Text. Separately, extend <scene>'s allowlist to cover freeze_at, world-position and animated-background, which are otherwise unreachable from HTML. Refs #220
write_prop/write_root_field/write_content capture m.raw (the ENTIRE document) at edit time, then 250 ms later serialize that snapshot over the whole file. If the notify watcher reloads the model from an agent/editor write during that window, the payload's raw is already stale and the flush wipes the agent's change completely — not just the edited pointer. write_and_note then registers the result in the self-write ledger, so app/mod.rs:141 makes the watcher skip the resulting event: memory holds the agent's version, disk holds the studio's stale version, and nothing ever reconciles them. No error is shown (m.write_error = None on success). This is the crate's headline workflow (baseline/diff review of agent edits), so the race is routine, not exotic. Fix: Before flushing, re-read the file and verify it still matches what the payload was derived from (hash the source at snapshot time, compare at write time); on mismatch, re-apply the single pointer mutation to the CURRENT disk content instead of replaying the stale whole-document snapshot, or surface a conflict in write_error. Do not register a self-write for a flush whose base no longer matches the disk. Refs #220
I read ureq 3.2.0's impl Default for Timeouts (config.rs:898-911): global, per_call, resolve, connect, send_request, send_body, recv_response and recv_body are all None; only await_100 is set (1 s). None of the four call sites configures one — assets.rs:141 (Iconify), google_fonts.rs:189 and google_fonts.rs:217 (Google Fonts CSS + TTF), rustmotion/src/include.rs:221 (arbitrary remote include). A host that accepts the TCP connection and then stalls, or trickles one byte per minute, blocks indefinitely; ureq's 10 MB body cap bounds bytes, not time. The icon path is the worst case: icon.rs:55 can reach fetch_icon_svg from inside paint_content, i.e. on a render worker thread, so one stalled connection wedges a render that has no user at the keyboard (CI, --frames a-b distributed segments). Combined with the remote- include finding, a scenario chooses the host that stalls. Fix: Build one shared ureq::Agent with Config::builder().timeout_global(Some (Duration::from_secs(20))).timeout_connect(Some(Duration::from_secs(5))) (plus https_only(true)) and route all four call sites through it, instead of the bare ureq::get(...) free function which always uses the default (untimed) agent. Refs #220
font_size_px_or goes through Length::px() → px_or_warn, which returns 0.0 for any relative unit; style.rs:1601 asserts this explicitly (assert_eq!(s.font_size_px_or(48.0), 0.0) for "15.6vw"). Five live call sites remain in apply_intrinsic_overrides — List (1498), Callout (1643), Tooltip (1663), PillNav (1688), Marquee (1716) — while every corresponding painter was migrated to the context-aware resolver in the "lot B, wave S" pass (marquee.rs:82 self.style.font_size_px_ctx(&crate::intrinsic::font_size_ctx(...), self.font_size), list.rs:88 the same). So a marquee with "font-size": "2rem" and no explicit style.height gets apply_default_size(css, 800.0, 0.0) → the (false,false) branch writes css.height = Px(0.0) → the node is skipped at paint time and the marquee is invisible; list (1498-1503) collapses to height 0 the same way; callout/tooltip/pill_nav get boxes measured at font- size 0 while the painter draws at the real size, so the ink overflows its own box. intrinsic.rs:27-46 states this wave's goal was to replace font_size_px_or at every site; box_builder was missed, and it is also the one file that does not use the module's own measure_time_font_size_ctx helper (intrinsic.rs:87). Fix: Replace the five font_size_px_or(...) calls in apply_intrinsic_overrides with font_size_px_ctx(&crate::intrinsic::measure_time_font_size_ctx(0.0), ...), the same helper every *Intrinsic in intrinsic.rs already uses. Add a regression test asserting a marquee/list with "font-size": "2rem" and no explicit height lays out with positive height. Refs #220
build() (layout_pass.rs:122-154) hands the *same* ConversionContext to to_taffy_style for every node in the tree and never re-derives it from the node's own (cascaded) font-size, and the only production builder keeps ..LengthContext::default() → font_size: 16.0 (scene.rs:29-37). So padding: "1em", gap: "0.5em", width: "10em", top: "2em" all resolve against 16px regardless of the element's actual font size: on a 48px-font card, padding: "1em" reserves 16px instead of 48px. Unlike the context-free .px() accessors — which were deliberately made to warn loudly for exactly this class of drop (units.rs:240-256) — this path is silent, so the wrong geometry reaches both the renderer and the validator with no signal. Fix: Derive a per-node LengthContext while building the taffy tree: resolve the node's own font-size (post-cascade) to px and pass it as ctx.length.font_size to its own to_taffy_style call and to its children's. Until that exists, emit the same one-shot warning order already uses (taffy_bridge.rs:142-151) whenever an em reaches this bridge. Refs #220
taffy 0.10.1 (`compute/leaf.rs`) subtracts `content_box_inset` (padding+border) from `available_space` before calling the measure fn, but passes `known_dimensions` — the outer border-box size — untouched. The two arguments are therefore in different coordinate spaces, and neither `IntrinsicMeasure`'s trait doc (box_tree.rs:114-122) nor this closure says so. `TextIntrinsic::measure` (rustmotion-components/src/intrinsic.rs:224) prefers `known.0` as its wrap width, while `Text::paint` wraps at `layout.content_box()` width (legacy_dispatch.rs:145-155). A `text` with horizontal padding is therefore measured at a wider line width than it is painted at: fewer wrapped lines, a reserved box one line too short, and the text spills out of its own box. The geometry validator re-measures through the same intrinsic, so it signs the overflow off. intrinsic.rs:258-265 explicitly asserts the opposite ("taffy hands a leaf its own known/available height already padding/border-subtracted") — true for `available`, false for `known`.
Refs #220
The closed-form underdamped step response is `1 - e^(-zeta*omega*t) * [cos(omega_d*t) + (zeta*omega/omega_d)*sin(omega_d*t)]`. The code feeds `zeta*omega*t/omega_d` into `sin()` instead of `omega_d*t` — the `t` was multiplied into the coefficient instead of the phase. I verified this numerically against a 400k-step ODE integration of `m*x'' + c*x' + k*x = k`: the correct formula matches the integrator to 5 decimals, the repo formula does not. With the shipped defaults (damping 15, stiffness 100, mass 1, zeta = 0.75), at the first 60fps frame after the start (t = 0.0167 s) the solver reports 0.10416 of the travel where the physics gives 0.01282 — 8x. Initial velocity is 6.21/s instead of 0, so the element visibly snaps on frame 1 rather than easing out of rest, and the overshoot peak is wrong too (1.0194 vs 1.0284). This affects every spring path in the engine: `EasingType::Spring` keyframes, the `bounce_in` preset (kf_anim_spring, zeta = 0.6, animator.rs:2092), `elastic_in` (kf_anim_spring_underdamped, zeta = 0.274, animator.rs:2106), and every author-supplied `spring` override applied through `apply_spring_to_motion`. `spring_settle_time`/`spring_rest_time` (what `rustmotion info` reports and what the `spring.duration` remap divides by) are derived from the same wrong curve. Refs #220
…286) `apply_transform_chain` resolves `translate`/`translateX`/`translateY` lengths through this context. The paint pass resolves the very same functions through `LengthContext { ..., parent_size: box_layout.width.max(height), font_size: node.css.font_size_px_or(16.0), ... }` and then splits it into per-axis `length_ctx_x`/`length_ctx_y` (crates/rustmotion-core/src/engine/paint_pass.rs:229-250, used at :953-962). Two divergences: (a) `em` — a `font-size: 96px` text with `transform: "translateX(-10em)"` is moved -960px by the renderer but only -160px by the validator, so a component at x=200 that renders at x=-760 (fully off-frame) validates clean; (b) percentage translate — the validator uses `max(w, h)` on both axes, the exact mistake paint_pass.rs:236-242 calls out in a comment ("never max(width, height) on both axes"), which over-estimates on the short axis and produces false positives. Note the `--strict-anim` path funnels animated transforms through this same function (geometry.rs:1482), so it inherits both errors. Refs #220
`check_unwrappable_text` — the only check left for a nowrap node — compares width only and always emits `Axis::X` (geometry.rs:781-793). So the Y axis is unchecked for any `white-space: nowrap|pre` text. `{"type":"text","content":"Hi","style":{"font-size":"120px","white-space":"nowrap","height":"40px"}}` has a single line ~156px tall painted out of a 40px content box and validates clean; delete the `white-space` key and the identical fixture is correctly reported as `ContentOverflowsBox`/`Axis::Y` (proved by the existing test `wrapped_text_taller_than_its_fixed_height_card_is_flagged`, geometry.rs:2806). The stated rationale only justifies skipping the *wrap-dependent* width re-measure; a nowrap node's height is exactly one `line_height` and needs no wrapping to compute.
Refs #220
`bbox.h` is `layout.height`, the border box. `LegacyPaintDispatcher::dispatch` treats only `Component::Codeblock(_)` as self-padding (`let is_self_padding = matches!(child.component, Component::Codeblock(_));`); every other painter, terminal included, receives a `BoxLayout` whose height is `layout.content_box()`'s `ch`. So a `terminal` with `auto_scroll: false` and `style.padding: "32px"` has 64px less drawable height than the check credits it with: content needing 500px in a 520px border box (456px content box) overflows by 44px and is reported clean. The codeblock arm just above is correct, because codeblock really is handed the border box — so the two arms of the same violation kind need different boxes and currently use the same one. Refs #220
`rustmotion_components::box_builder::component_kind` (box_builder.rs:2090-2154) is public and already imported elsewhere in the same crate (`crates/rustmotion/src/engine/render/scene.rs:744`). geometry.rs re-implements it as a private 60-arm copy. Both must be edited for every new component, and both already carry the same two label bugs: `QrCode(_) => "qrcode"` while the serde tag is `qr_code`, and `Container(_) => "container"` while the serde tag is `div`. Those strings are user-facing — they are the `component:` field of every `GeometryViolation` and drive the hint branches at geometry.rs:686 and :1591 — so a violation on a `qr_code` reports a type name that does not exist in the schema. Exhaustiveness protects against a *missing* arm but not against the two copies drifting on a rename, which is exactly how the labels got out of sync with serde in the first place. Refs #220
`remaining` never reaches 0.5 when it starts at 0.0 (0.0/0.5 == 0.0) or at any negative value (it diverges toward -inf). The loop never terminates and pushes a fresh `String` on every iteration, so rustmotion hangs while consuming memory until the OOM killer fires. Reached from a plain scenario JSON: `{"type":"video","src":"clip.mp4","playback_rate":0}` — `volume` defaults to 1.0 (`crates/rustmotion-components/src/video.rs:14` `fn default_volume() -> f32 { 1.0 }`), so `collect_videos_in_child` collects it (`v.volume > 0.0`, line 128), `collect_video_audio_tracks` calls `extract_audio_to_wav(..., occ.playback_rate)` (line 421), which calls `build_atempo_filter(0.0)` at line 333. `playback_rate` is an unvalidated `Option<f64>` (`crates/rustmotion-components/src/video.rs:26`); a repo-wide grep shows no validator constrains it. Both the ffmpeg render path (`encode_with_ffmpeg_hw_impl` line 432) and the native path (`mux.rs:34`) reach it.
Refs #220
Triggering scenario: a scenario JSON declaring a `video` component with `style.width/height` of 1920x1080 inside a 60 s scene at 30 fps. `.output()` (line 234) collects ffmpeg's entire rawvideo stdout into one `Vec<u8>`: 1800 frames x 8.3 MB = 15 GB. `data` then stays borrowed for the whole copy loop, so the per-frame `.to_vec()` copies build a second 15 GB structure alongside it — 30 GB peak before `output.stdout` is dropped. The result is inserted into the process-global `VIDEO_FRAME_CACHE` (assets.rs:168), which has no eviction and no `clear` function at all. There is no size, duration, or resolution cap anywhere on this path, and the inputs are entirely scenario-controlled. Under `--watch` this compounds: changing the component's `style.width` mints a new `"src:WxH"` key while the old multi-GB entry is never freed. Secondary hazard on the same line: `width * height * 4` is plain u32 arithmetic, so a declared 65536x16384 wraps to 0 and `data.len() / frame_size` panics with a divide-by-zero. Refs #220
Triggering scenario: `rustmotion render deck.json -o out.mp4 --watch` left open for an afternoon, with an `image` component pointing at `logo.png`. The author re-exports `logo.png`. `ASSET_CACHE` is keyed on the path alone (image.rs:41-54) with no mtime or size component, and in the incremental branch `clear_asset_cache()` only runs when the video config hash changes (resolution/fps) — editing scene content never clears it. Every subsequent re-render silently re-uses the stale decoded bitmap; the author sees the old logo indefinitely with nothing on stderr. The adjacent audio path solved exactly this and documents why (`encode/audio_analysis.rs:29-39`: fingerprint on len+mtime because 'someone re-exports a mix while the studio is open') — the image/GIF/video caches never got the same treatment. Memory side: `clear_asset_cache` touches only `ASSET_CACHE`; `GIF_CACHE`, `VIDEO_FRAME_CACHE` and `AUDIO_ANALYSIS_CACHE` have no clear function at all, so a long `--watch` session that touches several asset variants accumulates every decode for the process lifetime. Refs #220
Two parsers for the same syntax have drifted. Executed: `style="padding: 24px 48px; margin: 0 auto; grid-template-columns: repeat(3, 1fr)"` transpiles to `"padding":"24px 48px"`, `"margin":"0 auto"`, `"grid-template-columns":["repeat(3,","1fr)"]`. All three deserialize successfully — `Edges::Uniform(LengthPercentage::String)` and `GridTrack::Length(LengthPercentage::String)` are untagged string variants — and then core's `parse_length` (rustmotion-core/src/css/units.rs:125) only understands a single number+unit token, so `parse_length_or_warn` (units.rs:262) falls back to `Px(0.0)`: zero padding, zero margin, two zero-width grid tracks. The only signal is an `eprintln!` warning; `validate` exits 0 and the render is wrong. Per-side padding is entirely unexpressible from HTML (`padding-top:32` is rejected by `CssStyle`'s `deny_unknown_fields`, `padding:32px 48px` silently becomes 0), and the skill rule html-css-mental-model.md:97 only documents the JSON object form, which inline `style=""` cannot produce. Refs #220
…#290) `render_frame` panics on failure (`.expect("render frame")`, `.expect("rgba matches dimensions")`, `.expect("encode jpeg")`). Because the panic happens in the scoped child thread and is consumed by `join().unwrap_or_default()`, the `catch_unwind` wrapper in `view.rs::serve_or_render` never fires — so `fail_ledger().record_failure(key)` is unreachable on that path and the whole retry-budget mechanism is dead code for the on-demand asset handler. Instead the empty `Vec` is treated as a successful render: `frame_cache().insert(key, rendered.clone(), ...)` caches zero bytes, and the responder replies `200 image/jpeg` with an empty body. The canvas shows a permanently broken image for that (generation, frame, scale) and the cache guarantees it is never re-attempted. (The prefetch worker path calls `render_frame` directly and is correctly fenced.) Refs #220
`src` is the `video` component's scenario field, unfiltered. `info.rs:385-391` documents this explicitly ("video's ffmpeg-backed path only reaches one incidentally by handing the string straight to ffmpeg's own demuxer") — so the network reach is acknowledged as accidental, not designed. ffmpeg is invoked without `-protocol_whitelist`, which means the scenario selects from ffmpeg's entire built-in protocol set: `src: "http://169.254.169.254/latest/meta-data/"` or `http://127.0.0.1:8500/...` makes the renderer issue that request (SSRF from an LLM-authored scenario, and a reliable internal port-scan oracle via the exit status printed at preload.rs:252-258). The same unfiltered string reaches `preload.rs:221-222` and `encode/video_audio.rs:326-327`. Secondary risk: playlist/subtitle demuxers that dereference nested `file:` URLs would render local file content into the output video. There is no timeout on the subprocess either, so a slow remote URL stalls the render.
Refs #220
box_builder.rs:496 calls `rustmotion_core::css::cascade::inherit_from(parent_css, &mut css)` and paint_pass.rs:479 passes `&node.css` into `dispatch`. `LegacyPaintDispatcher` (the only real `PaintDispatcher`; the other is `NoopDispatcher`) binds it to `_css` and drops it, and `Painter::paint_content` (painter.rs:60-66) has no `CssStyle` parameter at all — so a painter structurally cannot see the cascaded style. Every painter resolves typography from its own declared struct field instead (text.rs:523 `self.style.color_str_or("#FFFFFF")`). All twelve properties `inherit_from` propagates (color, font-family, font-size, font-weight, font-style, line-height, letter-spacing, text-align, white-space, overflow-wrap, visibility, text-decoration) are paint-only: taffy_bridge.rs reads none of them (its own header says "Paint properties (color, background, transform...)" are excluded), paint_pass.rs contains zero references to `visibility`/`text_decoration`/`text_align`/`font_family`/`white_space`, and `component_intrinsic` (box_builder.rs:639) builds measurers from `&child.component`, not from the cascaded `css`. Net effect: `{"type":"card", "style":{"color":"#ff0000"}}` wrapping a `text` with no colour of its own renders white, not red — the documented CLAUDE.md feature "cascade.rs — Héritage color/font-* parent → enfant" has no observable effect. The regression test `card_color_cascades_to_text_child_with_no_color_of_its_own` (box_builder.rs:3066) passes because it only asserts on the intermediate `text_box.css.color` field, never on painted output.
Refs #220
For `angle_deg = 180` (the default, CSS "to bottom"): rad = 0, sin_a = 0, cos_a = -1, so p0 = (cx, cy + h/2) = bottom edge and p1 = top edge. Skia places `colors[0]` at p0, so the first stop lands at the *bottom* where CSS puts it at the top. Same 180-degree offset at every angle (90 -> first stop on the right, CSS says left). The repo's own skill doc contradicts the rendered result: `.claude/skills/rustmotion/rules/glassmorphism.md:114` prescribes `"gradient-border": { "colors": ["#FFFFFF50", "#FFFFFF08"], "width": 1.5, "angle": 180 }` and describes it at line 118 as "la lumière qui accroche le haut de la carte" — but this function puts the bright stop at the bottom. `paint_gradient_border` (line 1375) shares the function, so both `background` gradients and `gradient-border` are affected. Note the engine also carries a second, incompatible convention for `Fill::Gradient` in rustmotion-components/src/shape.rs:78 (angle 0 = left-to-right, math convention).
Refs #220
`spring_value` is reached per animated property, per node, per frame (`box_builder.rs:538` -> `resolve_props_for_effects` -> `resolve_animation_value_full`, animator.rs:997). When `SpringConfig::duration` is set, each of those calls runs a full coarse scan plus 40 bisection steps, with `steps` clamped to [2000, 20000]. For the shipped defaults (stiffness 100, mass 1 -> omega 10, period 0.628 s) `desired_steps` is 2292, so one animated property costs ~2333 `exp`/`sin`/`cos` evaluations per frame to recompute a value that depends only on (damping, stiffness, mass, rest_threshold) — completely invariant across frames. A 30 s/60 fps render with 20 nodes carrying two spring-duration properties each burns roughly 165 million redundant solver evaluations. The function's own doc comment acknowledges it is "called on every `spring_value` sample when `duration` is set". Refs #220
`bbox` here is `raw_bbox = bbox_of(layout)` (geometry.rs:272, 376-383), i.e. `layout.width` — the BORDER box. But `LegacyPaintDispatcher::dispatch` (crates/rustmotion-components/src/legacy_dispatch.rs) hands every non-codeblock painter a synthetic `BoxLayout { width: cw, height: ch, border/padding: zero }` taken from `layout.content_box()` and translates the canvas to the content-box origin. So `Text::paint` wraps and draws inside `cw`, not `layout.width`. A `white-space: nowrap` text with `style.padding: "0 24px"` in a 300px box has `cw = 252`; a natural line width of 280px paints 28px past the box's right edge, yet `280 > 300` is false and nothing is reported. The sibling check `check_content_overflows_box` (geometry.rs:848) does this correctly via `layout.content_box()` — the two checks disagree about which box text lives in, and the flagship `unwrappable_text_overflow` is the one that is wrong. The gap equals `padding.left + padding.right + border.left + border.right`.
Refs #220
`decoder.width()/height()` come straight from the GIF logical-screen descriptor (2 bytes each, max 65535). A ~200-byte crafted GIF declaring 65535×65535 makes line 129 request a single 17.2 GB allocation before a single pixel is decoded. Worse, the `while` loop then pushes a *full-canvas clone* per animation frame (line 141) with no frame-count limit — a 4000×4000 canvas with 500 frames is 32 TB of `Vec<u8>` — and the result is inserted into the global `gif_cache()` (`renderer/assets.rs:32`) which is never evicted. A scenario whose `gif` src points at such a file aborts the render process (Rust allocation failure = abort, not a catchable error), which is a denial of service on any batch/CI renderer fed third-party scenarios or media. Refs #220
`resolve_name_template` walks the template as raw bytes and casts each one to `char`, which in Rust is a Latin-1 reinterpretation of the byte value. Substituted `{field}` values are fine (they go through `push_str` at line 71); the literal text around them is not. I confirmed the behaviour by compiling the same loop standalone: `résumé-{id}.mp4` yields `résumé-VAL.mp4` and `动画-{id}.mp4` yields `å¨ç»-VAL.mp4`. So `rustmotion batch --name-template "résumé-{id}.mp4"` writes files literally named `résumé-abc.mp4` to disk, and the same corruption lands in the `[ok] <path>` progress lines and in every failure message (`item N: ...`). A localisation batch — the exact use case `{lang}/{id}.mp4` in the module doc is built for — is where a non-ASCII template is most likely. None of the five tests in `name_template_tests` (lines 448-495) uses a non-ASCII template.
Refs #220
Every non-decorative node is timed by `PaintWindow::contains` — `self.end.is_none_or(|e| t < e)` (rustmotion-core/src/engine/box_tree.rs:43), a half-open window — and gets its effects from `box_builder::effective_effects` (legacy_dispatch.rs:110), which folds in `timeline` steps and `style.transition` keyframes. This second path uses an inclusive `time > e` and calls `a.animation_effects()` raw. `Particle` declares `timeline: Vec<TimelineStep>` (particle.rs:57), so a particle's timeline steps and transition keyframes are silently dropped, and its `end_at` keeps it visible for one extra frame — but only in a `world` view, since this is the sole call site (scene.rs:1130). The same particle in a `slide` view behaves correctly. A component whose behaviour depends on which view type contains it is invisible to tests written against either one. Refs #220
#250) `cfg.color` is a free-form `String` on `HeropatternConfig` (crates/rustmotion-core/src/schema/background.rs:216, `#[serde(default = "default_hero_color")]`) with no validation anywhere — `validate_animated_bg` (crates/rustmotion/src/include.rs:267-273) checks only that the pattern *name* resolves. The template lands inside a double-quoted attribute (`fill="{{color}}"`), so a colour containing `"` closes the attribute and injects arbitrary markup into the document usvg then parses. usvg 0.46's `Options::default()` keeps the default `ImageHrefResolver`, whose string resolver I read at usvg-0.46.0/src/parser/image.rs:85-112: it calls `std::fs::read(&path)` on any href that `path.exists()` accepts — with `resources_dir: None` an absolute path resolves to itself — so an injected `<image href="/absolute/path.png">` is decoded and painted into the frame. The impact is bounded (a scenario can already read a local image legitimately via `{"type":"image","src":"/abs/path.png"}`, and there is no network resolver), so this is a hardening gap rather than a privilege escalation. The quieter half is availability: any malformed splice makes `Tree::from_data` fail and line 601 `return`s, so the background silently does not render with no diagnostic at all. Refs #220
The `TagKind::Text` arm never calls `element_to_value` on its children, so the whole guard layer (`StyleElementUnsupported`, `UnsupportedNativeElement`, `TagKind::Ignored`) is unreachable below `p/span/h1..h6/strong/em/label`. Executed against the real crate: `<p>Real <script>var secret = 1; alert(2);</script></p>` transpiles to `{"content":"Real var secret = 1; alert(2);","type":"text"}` — the JS source is rendered on screen; `<h1>Title<style>h1{color:#0f0}</style></h1>` gives `{"content":"Titleh1{color:#0f0}"}`; `<p>cap<img src="hero.png"></p>`, `<h1>t<svg>…</svg></h1>` and `<span>n=<rm-counter from="0" to="100"></rm-counter></span>` drop the img/svg/counter entirely and exit 0. This is a live regression of "constat 1" and "constat 3", which tests/silent_loss_regressions.rs asserts are fixed — the fix only ever covered the `Container` path.
Refs #220
All four write-back entry points (`set_inline_style`, `set_text_content`, `set_attribute`, `remove_inline_style`) re-serialize only the `<rustmotion>` element, and rustmotion-studio writes that string straight over the source file (editor/inspector.rs:2168 -> `write_and_note(path, &updated)`). Executed: a 281-byte document with `<!DOCTYPE html>`, `<html><head><meta charset="utf-8"></head><body>`, a leading `<!-- authored by hand -->` comment and a trailing comment becomes 150 bytes — doctype, head, meta, body and both outer comments gone — after one font-size tweak in the inspector. The transpiler happily accepts such documents (`find_element` is a document-wide DFS), so this is a supported authoring form being destroyed. The docstring's "formatting is normalized" does not cover deleting the surrounding document. Secondarily, `serialize_element` (lib.rs:488-497) does `let _ = serialize(...)` and `String::from_utf8(buf).unwrap_or_default()`, so a serializer failure would write an empty file rather than surface an error. Refs #220
`MAX_EXPANSION_DEPTH` bounds how deeply directives may nest, not how many nodes they produce, and nesting one `for-each` inside another's `template` is explicitly supported (`use_template_can_contain_a_nested_for_each`, expand.rs:1005). Each nesting level costs exactly +1 depth (resolve_entry recurses with `depth + 1` per produced node, while `walk_children` forwards `depth` unchanged), so up to 64 levels are legal and the output is the product of the array lengths. Four nested levels of 50 elements is 6.25M nodes from a file under 1 KB; eight levels of 10 is 10^8. Each iteration does a full `Value` deep-clone of the template plus a `substitute` walk, so the process hangs and OOMs during `expand_directives` — before validation, before any render, and with no diagnostic. An LLM-authored or third-party scenario reaches this through the documented public syntax; `--no-validate` is not needed. Refs #220
Any scenario handed to `rustmotion render`/`validate` can make the user's machine issue GETs to hosts of the author's choosing, with no host/scheme allowlist, no redirect policy, no timeout set on the call, and no `--offline` switch to refuse. On a developer laptop or a CI runner this reaches loopback services, RFC1918 addresses and cloud link-local metadata (169.254.169.254), and it doubles as a silent beacon that fires merely from *validating* a file (validation::load goes through the same `resolve_includes`). Fetching remote includes is clearly deliberate design, so the finding is the absence of any control around it, not the feature. Refs #220
`TableIntrinsic::compute_width` (intrinsic.rs:911-949) measures every header and every cell, keeps a per-column max (`col_widths[i] = col_widths[i].max(w + cell_padding * 2.0)`) and returns the **sum**. The painter then throws that per-column distribution away and gives each column `total_w / col_count`. For an uneven table (e.g. headers `["ID", "Description of the incident"]`, natural widths 60px and 400px → intrinsic 460px) the painter allocates 230px per column; the 400px column's text is drawn from `align_text_x` at a position computed against a 230px cell and there is no per-cell clip (table.rs:203-262 draws each cell with `draw_text_with_fallback` and only advances `col_x += cw`), so the text overlaps the neighbouring column or spills past the table's right edge. With `column_align: "right"` it spills backwards into the previous column. The measurer's own doc comment (intrinsic.rs:884-888) asserts the opposite: "Natural size formula (matches the painter exactly) ... otherwise each column gets `max(header_text_width + 2 × cell_padding, min_col_width)`" — the painter implements no such rule, and `compute_width` has no `min_col_width` either. Refs #220
`optimistic::commit` bumps `generation` on EVERY mutation (one per `oninput` event), and `use_hot_reload` polls it every 250 ms, so a slider drag on a scenario that has audio calls `audio::prepare` ~4x/second. `prepare` spawns a brand-new `std::thread` that runs `mix_audio_tracks` over the whole scenario — documented in audio.rs:158 as "seconds on a long scenario" — decoding and resampling every track from scratch even though a font-size edit cannot have changed the audio. `MIX_TOKEN` only discards the RESULT; nothing cancels the work. Dozens of concurrent decode threads pile up, each holding a full interleaved f32 PCM buffer (~23 MB for 60 s stereo), fighting the 2-6 prefetch render workers for cores during exactly the interaction that most needs to stay smooth. Refs #220
…266) The first pass at making `cascade::inherit_from` reach painters special-cased `Component::Text` inside `LegacyPaintDispatcher::dispatch`. That left the other seventeen components that read `color`/`font-*` off their own un-cascaded `self.style` still blind to inherited typography — `gradient_text`, `caption`, `rich_text`, `badge`, `callout`, `counter`, `divider`, `icon`, `kbd`, `list`, `marquee`, `notification`, `number_wheel`, `pill_nav`, `table`, `terminal`, `tooltip`. Worse than the missing coverage, a per-variant `match` arm means the cascade applies to exactly the variants someone remembered to list: the next component added would silently miss it. That is the same defect class the audit reported elsewhere in this crate, reintroduced by the fix for it. Refs #220
The interleaved samples appended to `all_samples` come from `decoded.spec()`, but the returned `(sample_rate, channels)` come from the container header with `unwrap_or` fallbacks. These diverge for real files: symphonia's ADTS AAC reader only sets `params.with_channels(channels)` when `header.channels` is `Some` (symphonia-codec-aac-0.5.5/src/adts.rs:139-141) — an ADTS stream with `channel_config == 0` leaves `codec_params.channels == None`, so rustmotion assumes 2 while the decoder actually emits mono. `to_stereo(&samples, 2)` (line 205) then passes the mono buffer through untouched, so the track is half its real length and plays at double speed; `mix_audio_tracks_segment` sizes everything from that, and the muxed audio desyncs from video with no error anywhere. `audio_analysis.rs:143-149` divides by the same wrong `channels` for the waveform/spectrum envelope, so the on-screen visualisation agrees with the broken mix. Refs #220
The path is fully derived from PID plus a counter starting at 0, in a shared, world-writable directory. `create_dir_all` succeeds on an already-existing directory (or a symlink to one) and `fs::write` follows symlinks, so a local attacker who pre-creates `/tmp/rustmotion_audio_<pid>_0/audio.raw` as a symlink gets an arbitrary file truncated and overwritten with PCM bytes under the rendering user's identity; PID space is small enough to pre-seed exhaustively. The same class exists in `video_audio.rs:258`, `std::env::temp_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash))`, where `hash` comes from `DefaultHasher::new()` (SipHash seeded with fixed keys 0,0 — deterministic across processes and machines), so the path is exactly predictable. There, `if wav_path.exists() { return Some(wav_path) }` (line 298) means a pre-planted file is used verbatim as the render's audio, and the `.partial.wav` sibling is handed to `ffmpeg -y` (line 339-340), which follows a symlink at that path.
Refs #220
Every structural claim in this block is false. Verified: there is no root `src/` (`ls src` → No such file or directory, the tree is `crates/*/src`); there is no `layout/` directory (`ls crates/rustmotion-core/src/layout` → No such file or directory — taffy replaced it); the `Widget` trait does not exist (`grep -rn "Widget" crates/` returns exactly two hits, both comments, one of which is `crates/rustmotion-core/src/traits/painter.rs:1`: "`Painter` trait — replaces the old `Widget::render`"); the pipeline is box_tree → layout_pass → paint_pass, not "measure → layout → paint"; and the `Component` enum in `crates/rustmotion-components/src/lib.rs:355-418` has 60 variants, not 51 (CLAUDE.md's "Composants disponibles (60)" is the correct count). The "CLI Reference" section (README.md:88-104) documents only `rustmotion render` and 9 of its flags, while `crates/rustmotion/src/cli/mod.rs` declares 11 subcommands (Render, Concat, Still, Captions, Validate, Batch, Schema, Info, Skills, Completions, plus nested) — `validate`, the gate CLAUDE.md makes mandatory before delivering any scenario, is entirely absent, as are `--watch`, `--frames`, `--var`, `--props`, `--strict-anim`. The branch HEAD (`4d54504 fix(crates): ship the README with every published crate`) adds `readme = "../../README.md"` to all four crate manifests, so this document becomes the front page of rustmotion, rustmotion-core, rustmotion-components and rustmotion-html on crates.io and docs.rs. A contributor following it looks for `src/components/` and implements `Widget`. Refs #220
The heading (line 40) and list (line 44) name `shape, image, icon, svg, video, gif, callout, chart, comparison, dot_map, gauge, heatmap, lottie, marquee, mockup, pill_nav, skeleton, sparkline, stat, stepper, tag_cloud, tooltip, treemap` as having no size source, "verified against ... `component_intrinsic` and `apply_intrinsic_overrides`". box_builder.rs:1605-1955 now gives every one of those a default size — the block's own comment cites this exact issue ("#126 / W3: the 23 components with no size source") — and box_builder.rs:2883 asserts each gets a positive box. This file is the LLM-facing generation guide shipped inside the published crate: it makes the model add redundant explicit `width`/`height` everywhere and, worse, line 48-50 tells it to suspect this list first when a component doesn't render, sending debugging down a dead path. The doc even claims `stat` in a flex row produces a blank frame, which the `Stat(_) => apply_default_size(css, 280.0, 180.0)` arm (box_builder.rs:1817-1825) was written specifically to fix.
Refs #220
…on (#304) The 55 findings were merged one squashed commit at a time. Several of them landed during the first merge attempts, before a defect in the carving mechanism was found and corrected, so they carried a snapshot taken while their agent was still polishing rather than that agent's final state. The gap was real, not cosmetic: 200 lines of regression tests missing from one file, 141 from another, 40 from a third. The branch compiled and its remaining tests passed, which is exactly why this needed comparing against something rather than trusting a green run. This takes every path the audit owns from chantier/audit-2026-09-full, the single commit of the whole remediation tree on which fmt, the full test suite and clippy were run, then re-applies the constant-size chunks_exact replacement to the five files that needed it, since that branch predates the lint fix. The two files still differing from it afterwards differ only by that same lint fix, which they now carry and it did not. Verified on this branch: cargo check --workspace --all-targets, cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --all --check. Refs #220
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.
No description provided.