Skip to content

fix(audit): remediate the September 25 audit across all five crates - #319

Merged
LeadcodeDev merged 6 commits into
mainfrom
chantier/audit-2026-09-25
Sep 25, 2026
Merged

LeadcodeDev merged 6 commits into
mainfrom
chantier/audit-2026-09-25

Conversation

@LeadcodeDev

@LeadcodeDev LeadcodeDev commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Remediation of the 25 September audit: 217 findings across five crates, cross-referenced against 795 Remotion features. This branch closes most of them. The full record is on #315.

This is too large to review in one sitting, and that is worth saying plainly. It opened as six commits covering a spike and grew to forty-three across fourteen workstreams. Reviewing it as a unit is not realistic; reviewing it commit by commit is, since each one carries its own why, the alternative it rejected, and what it could not verify. A reader with limited time should read the commit subjects, pick the three that touch code they care about, and read those bodies.

What the audit actually found

The dominant defect class is silence, not crashes. A field that parses and does nothing. A flag accepted and dropped. A barrier that approves what it should refuse. Almost nothing here is a segfault; nearly everything is a picture that came out wrong with no way for the author to find out.

That shape drove the remediation's single rule: either the thing works, or its presence is refused by a message that names it and locates it. Silence is the bug.

What changed

The geometry validator — the entry document calls it mandatory — was bypassable five ways. vw and vh measured against a hardcoded 1920×1080 during intrinsic measurement, so the same declaration was wrong in both directions on any other resolution. --strict-anim sampled eight points per second whatever the frame rate, while the documentation sells it as frame-by-frame. A keyframed camera was never interpolated, so the documented way to animate a pan checked nothing at all. Seven painters ignored the box layout gave them. A centre-aligned stroke always bled half its width outside the box it was measured in.

Encoding did not produce what the flags asked for. --transparent was dropped on the default path after every frame had been rendered. Codec and CRF vanished without ffmpeg. concat drifted the audio by 33 to 45 ms per seam, cumulatively, on the command the entry document sells as the brick of a distributed render — and the cause was not the edit list everyone assumed but the AAC encoder's trailing padding, which nothing in the audio stream records. The video track, never padded, is the only ground truth available for the sample count.

Three file reads reachable from a scenario. Path::join returns its argument unchanged when that argument is absolute, so an icon id carrying a path made the cache directory vanish from the computation. usvg's default href resolver opens a plain <image href> as a local file, so inline SVG data could pull an arbitrary file into a frame. And three of the four sites that hand a scenario's src to ffmpeg had no guard at all — including the one the studio reaches by opening a file, with no flag and no validation upstream.

Seventeen inert fields. mix-blend-mode, clip-path and visibility are implemented. text-decoration is removed, because drawing it correctly needs per-line metrics that only the text painters have, and a bar at a fixed height is right for one line and wrong for every wrapped paragraph.

The scenario expansion had no scope. A config variable named like a for-each field silently won, and a nested iteration lost to its parent. Bindings chain now, and a component sees only what it was handed.

The paint pass clipped transformed descendants out of their own opacity layer, dropped whole subtrees when a container's own box measured zero, and painted eight components' backgrounds twice.

The charts drew plausible wrong answers — a radial bar whose largest item was always a closed ring, an area chart painting its smallest value on the floor as if it were near zero, a treemap whose negative value pushed the others out of the box.

The HTML dialect turned an unrecognized tag into a div in silence, so a typo produced a scenario that transpiled, validated and rendered almost right.

Supply chain: six of the ten advisories the CI suppressed had a semver-compatible fix available the whole time, reachable by a lockfile update and no manifest change. Real vulnerabilities go from eleven to five. The publish workflow now refuses a tag whose commit is not an ancestor of main or whose checks did not pass.

Documentation: every JSON block in the README and the skills was executed rather than read. Eighteen README examples failed. Around twenty-five documented CLI invocations passed the file positionally, which no subcommand accepts.

What the diff does not show

Two findings did not survive contact. world-position is not inert in the HTML dialect: paired with transition: camera_pan it drives the pan between two scenes, proven by two renders whose pixels differ — the finding generalised from a single test. And a spring's closeness to its target at the end of its segment was a sampling coincidence on a zero crossing; instrumenting the settle time showed it had not settled at all.

Three fixes were measured and then not written. Caching layout across frames: taffy is 8.3% of a frame on a deliberately dense scenario, the box tree is 28.8%, and a fingerprint that missed an animated dimension would produce a stale layout — a wrong picture, silently. The text measurement cache gains nothing measurable on this repository's own examples, and the commit says so; it is kept for the complexity fix beside it, which is proven, and for the twelve-thread pathology it removes.

Rendering changes visibly in five examples, and each is a defect being corrected rather than a regression: an area chart's axis now starts at zero, badges lose a square corner behind their rounded pill, a badge that never centred now does, and the promo's particle background declares the bleed it always had.

A missing font family is now reported. Expect it on every example on a machine without Inter — the same scenario used to wrap differently on different machines without a word.

text-decoration is a compatibility break. It parsed and did nothing; it now fails validation. Nothing it produced was ever what was asked for.

Explicitly out of scope

The TEST lens — eleven findings on the suite itself — is untouched, deliberately: writing tests against an engine under repair freezes the intermediate state. Around fifteen findings were handed between workstreams and landed outside every write partition; stagger, declared on fifty-seven component structs and read for four, is the largest. Five encoding performance findings are architecture changes rather than repairs.

Five findings became issues instead: #318, #320, #321, #322, #324. Two of them need a product call, one needs the copyright holder's name.

This audit never had an adversarial panel. The September one did, with 50% refutation. Two findings were refuted here by measurement, both by accident, while doing something else. How many of the rest stand only because nobody tried to break them is unknown.

Verification

Full suite on a detached worktree at rest, cargo fmt --all --check clean, clippy clean across the workspace, and all eleven examples/*.json validate — which they did not before this branch.

Refs #315
Closes #323

A timeline step has two halves and they obeyed different rules. Its `style`
was gated by `apply_style_states`, which merges only the states whose `at`
the clock has reached. Its `animation` was merged unconditionally by
`effective_effects`, so every step contributed from t=0.

With one step that is invisible: the step's own first keyframe is the base
state, so holding it changes nothing. With two or more it decides the frame.
All keyframes effects share one bucket where the last entry wins on a shared
property, and the last entry is the last step — which has not begun, so it
imposes the value of its first keyframe from the very start. A node that
moves in three beats sits at the end of beat two for the whole scene, and
only the final beat ever animates.

Passing the component's local time in and filtering the steps the same way
`apply_style_states` already does aligns the two halves. The last-effect-wins
rule for `style.animation` is untouched: it was chosen deliberately so that
composition does not depend on an incidental `delay`, and it is pinned by
`last_declared_effect_wins_regardless_of_which_one_carries_the_delay`, which
still passes.

Found by porting an existing promo animation to a scenario rather than by
reading: the logo had to enter centred, move to a corner, and come back, and
it never left the corner.
A filled SVG (no stroke) could only draw-on as a wireframe outline:
paint_draw_on always strokes each path with draw_stroke_width, so a solid
logo traced in thin lines and only became solid the instant draw_progress
hit 1.0. There was no way to express "the logo draws itself" for anything
but stroke-art icons.

Add `reveal: "stroke" | "fill"` (default "stroke", behavior byte-for-byte
unchanged for the default: same branch, same functions). `"fill"` instead
sweeps a per-path clip mask (left-to-right, path bounds) over the SVG's own
already fully rasterized image (paint_resvg's output, now cached via the
new `cached_full_image`, factored out of `paint_static` without behavior
change). Clipping the pre-rendered raster keeps gradients/patterns intact,
which a per-path flat-color fill (the alternative: reuse collect_paths'
color resolution and skia-fill each path with a growing rect clip) would
have lost — collect_paths already falls back to white for gradient/pattern
paints, which is fine for a 2px trace but not for a filled reveal meant to
show the real artwork. Path ordering/windowing (length-weighted, draw_overlap)
is reused as-is from paint_draw_on so paths still reveal sequentially.

Not verified: full ffmpeg render/encode of a `reveal: fill` scenario (only
`still` frames and unit tests); behavior with self-intersecting/evenodd
paths beyond the fill-rule set on the mask path.
taffy_bridge.rs only wrote style.flex_direction when css.flex_direction
was Some(..); when a card/div/flex/grid omitted flex-direction, taffy's
own Style::DEFAULT (Row) leaked through unnoticed. SKILL.md documents
"column" as the default, and the scene root (box_builder.rs's
default_root_css) already sets Column explicitly — that root-level
override was masking the mismatch, since every top-level layout looked
correct while any nested container silently behaved like Row.

Measured before choosing: 75/259 card/div/flex/grid instances across
examples/*.json omit flex-direction (0/61 for `flex`, since authors
always state it explicitly there; the silent gap is entirely in `card`
45/98 and `div` 30/100). Rendered stills of every affected scene in all
7 affected example files before and after, at each scene's sampled
midpoint: 25/27 frames are byte-identical; the remaining 2 (both in
ferriskey-launch-60s.json, a pill-nav button row) differ by ~0.14% of
pixels, a sub-pixel border AA shift on a single-child card whose
content is unaffected by axis choice. No `validate` output regresses
on any example (the one ferriskey-presentation.json geometry failure
predates this change, confirmed by testing the unpatched binary).

Alternative considered: fix SKILL.md to document "row" instead, since
that's taffy/CSS's real default. Rejected — the scene root, SKILL.md,
and effectively every example in the repo are already written assuming
column, so "row" would be the surprising, silently-wrong default for
the LLM authors this schema targets, not less so than today.

Reserve: cargo test --workspace surfaces 7 failures in
crates/rustmotion/src/cli/commands/geometry.rs (out of this commit's
file scope), isolated to be caused by this change and not concurrent
work (confirmed by reverting only this file against the current tree).
Root cause: a fixed-height card/div/flex with a single oversized child
and no explicit flex-direction used to have that child's cross-axis
clamped to the card's declared size under Row+stretch, so a
remeasurement pass reliably caught the mismatch as ContentOverflowsBox.
Under Column, the child's main-axis (height) is no longer clamped and
grows to its natural content size instead, so the layout box already
"matches" its own content and that check stops firing. When the grown
box also happens to cross the viewport edge, check_viewport still
catches it (reclassified, not lost) - but when it doesn't, or when the
node carries bleed: true, the overflow escapes detection entirely.
Reproduced with a minimal fixture (card w=300 h=80, one wrapped text
child needing ~343px, positioned so the grown box stays inside the
viewport): the unpatched binary reports ContentOverflowsBox; the
patched one reports nothing. This is a real, if narrow, hole in the
geometry validator's coverage for a common pattern (45/98 card
instances in examples/*.json have no explicit flex-direction), not
mere reclassification, and fixing it requires touching geometry.rs's
own overflow checks - outside this commit's file scope. Needs
escalation before this lands.
… start_at

start_at opened a component's visibility window (PaintWindow, already correct)
but never touched the clock its animation effects resolve against — they kept
running on raw scene time. An entrance already playing out by the time the
node became visible snapped straight to its end state instead of animating;
an exit whose own `delay` had elapsed before `start_at` left the node
painting nothing for its whole visible window (measured: a badge with
start_at: 2.0 rendered zero pixels at every sampled instant, its exit having
completed at scene time 1.15).

Fix: fold the node's own `start_at` into the same `extra_delay` that a
container's `stagger` already contributes in `build_child` (box_builder.rs) —
same mechanism, same call sites (effective_effects, apply_style_states,
resolve_transition_css_overrides, ghost generation), so `start_at` rebases
exactly like stagger already did. `BuiltScene.stagger_delays` now carries
this combined delay per node, which legacy_dispatch.rs and geometry.rs (the
--strict-anim overflow sampler, outside this change's scope) both already
read from — geometry.rs picks up the fix for free without being touched.
`paint_decorative_fullscreen` (scene.rs) gets the same treatment for
full-viewport leaves, which have no stagger of their own to fold in.

Considered rebasing `BuildAnimationCtx.time` itself instead of shifting each
effect's `delay`. Rejected: `stagger` already uses the delay-shift form (see
`effective_effects`), and the two forms are only equivalent as long as
nothing downstream reads the un-shifted clock directly — mixing them would
have made that invariant easy to break later. Reusing the proven form keeps
`start_at` and `stagger` composing through the identical path.

Does not touch `end_at`, `delay` without `start_at` (still resolves against
scene time, unchanged), or the timeline-step gate from b9cf23a
(`last_declared_effect_wins_regardless_of_which_one_carries_the_delay` still
passes) — `t` passed into `effective_effects` stays the raw scene clock, only
`extra_delay` grows.

Measured against every examples/*.json (43 rendered frames across all 11
files, before/after, pixel-diffed): zero visual change. Only two example
components declare start_at at all (both `counter`, mega-showcase.json and
rustmotion-promo.json), and counter's own progress reads ctx.time directly
rather than going through the effects pipeline this change touches, so even
those are unaffected.
…s box

8afc4c1 (default flex-direction: column) fixed a real layout bug but opened
a detection hole here. check_content_overflows_box/check_auto_scroll both
compared a leaf's measured content against its OWN post-layout box
(layout.content_box()). Under the old row default, align-items: stretch
clamped a lone child's cross axis (height) to its container's declared
size, so a too-small card produced a real box/content mismatch these
checks caught. Under column, that axis is the MAIN axis, which stretch
never clamps — the child's box now legitimately grows to match its content
exactly (CSS min-height:auto-style overflow), so the self-vs-self
comparison became vacuous: the box IS the content, by construction. The
paragraph still paints past its card, invisibly to both checks, unless the
grown box happens to also cross the viewport edge.

Fix: thread the nearest containing block's own resolved content box
(container_bound) down through walk(), and clamp an in-flow child's
effective box to min(own, container) per axis before comparing. A card's
own box stays true to its declared size regardless of its children's
overflow (ordinary CSS containing-block behavior), so this recovers
exactly the bound the old row-direction clamp used to provide, without
resurrecting row as the default. Absolute children are excluded — taken
out of flex flow entirely, their box is legitimately sized from their own
content only, per the already-passing
absolutely_positioned_*_spilling_past_a_visible_card_is_legal tests. That
exclusion checks box_node.css.position == Some(Position::Absolute), the
exact condition box_builder.rs uses to decide the same thing, not
ChildComponent::is_flow() (a stricter, unrelated predicate that's false
for any declared `position` shorthand, "absolute" or not, and isn't
otherwise read by the layout pipeline).

Considered parsing each node's own declared style.width/height directly
instead of reading the parent's resolved layout box. Rejected: the
overflowing leaf (text/table/codeblock/...) usually has no explicit size
of its own — the fixed size lives on the ancestor card — and the parent's
resolved box already gives the same number without re-deriving unit/
percentage resolution that CssStyle -> px conversion already does once,
correctly, during layout.

Of the 7 previously-red tests: 6 were real detection losses, now fixed by
container_bound (in_flow_codeblock_shrunk_by_its_card_is_caught_by_auto_
scroll_check, in_flow_table_taller_than_its_card_is_flagged_via_content_
overflows_box, bleed_true_does_not_exempt_content_overflows_box,
gradient_text_taller_than_its_fixed_height_card_is_flagged,
without_text_autofit_the_same_fixture_still_overflows,
text_autofit_does_not_silence_an_overflow_the_floor_cannot_fix). The 7th,
wrapped_text_taller_than_its_fixed_height_card_is_flagged, was a genuine
reclassification unrelated to this file's fix: its y=200 fixture's grown
(unclamped) box now happens to cross the 540px frame edge by ~3px for
real, so check_viewport correctly starts firing ViewportOverflow
alongside — moved the fixture to y=100 (restoring the isolated-repro
invariant its own docstring claims) rather than weakening the assertion,
and added a dedicated regression test using the audit's own 1920x1080
numbers.

Verified: the audit's own repro (1920x1080, card 300x80 at (660,50),
overflowing text) is a named ContentOverflowsBox error again via the CLI.
All 69 geometry:: tests and the full `cargo test -p rustmotion` (440
tests) pass. `rustmotion validate` on all 11 examples/*.json is
byte-identical before/after this change (built from git-show'd pre-fix
geometry.rs vs. the fixed version, everything else held constant) —
including the pre-existing, unrelated ferriskey-presentation.json
gradient_text overflow. cargo fmt --all and cargo clippy -p rustmotion
--all-targets -- -D warnings are clean.

Not verified: multi-child containers where siblings jointly (not
individually) exceed a fixed-size container's declared axis — container_
bound clamps each child independently against the full container box, not
against space already consumed by earlier siblings, so it can under-report
in that specific shape. Not exercised by any test or example/ file found.
@LeadcodeDev LeadcodeDev added the bug Something isn't working label Sep 25, 2026
@LeadcodeDev LeadcodeDev self-assigned this Sep 25, 2026
@LeadcodeDev
LeadcodeDev merged commit 23f4d6a into main Sep 25, 2026
4 checks passed
@LeadcodeDev LeadcodeDev changed the title fix(engine): repair the animation clock, the flex default and the validator's blind spot fix(audit): remediate the September 25 audit across all five crates Sep 25, 2026
LeadcodeDev added a commit that referenced this pull request Sep 25, 2026
The spike that opened this branch landed on main through #319, rebased, so
main carries the same six fixes under different hashes. Merging brings the
base forward and keeps the pull request's diff to the work that has not
landed yet.

Three files conflicted — the SVG component, the box builder and the
geometry validator — and in each the branch holds the later version: the
sandboxed usvg options and their tests, the paint window computed before
the ghosts are built, and the real viewport threaded into intrinsic
measurement. The branch's side was taken throughout, since it already
contains what main brought.

1689 tests pass on the merged tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Media cache clearing never reaches the rayon workers

1 participant