Skip to content

fix(core): treat unparsable timing attributes as missing instead of NaN#1206

Open
calcarazgre646 wants to merge 2 commits into
heygen-com:mainfrom
calcarazgre646:fix/core-timing-nan-validation
Open

fix(core): treat unparsable timing attributes as missing instead of NaN#1206
calcarazgre646 wants to merge 2 commits into
heygen-com:mainfrom
calcarazgre646:fix/core-timing-nan-validation

Conversation

@calcarazgre646
Copy link
Copy Markdown
Contributor

Problem

Element-level timing attributes go through parseFloat unguarded in the timing compiler and the HTML parser, so a typo like data-start="abc" propagates as NaN with no signal to the user:

  • compileTag (timingCompiler.ts) computes data-end as start + parseFloat(durationStr) and serializes the result into the compiled HTML: the output literally contains data-end="NaN". The same unguarded reads exist in injectDurations, extractResolvedMedia (start/mediaStart; duration was already guarded), clampDurations, and the unresolved-div scan.
  • parseHtml (htmlParser.ts) computes duration = Math.max(0, parseFloat(dataEnd) - start) — and Math.max(0, NaN) is NaN, so a garbage data-end lands in the timeline model as a NaN duration. updateElementInHtml writes data-end="NaN" back into the document when data-start is garbage.

Neither lint nor compile reports anything: the linter has no numeric check for element timing attributes, while composition-level duration is already validated (validateCompositionHtml errors on non-finite data-composition-duration, and extractCompositionMetadata guards with isFinite). Element timing attrs missed the same policy.

Change

Unparsable is treated as missing, which all of these paths already know how to handle:

  • New parseTimingAttr(value, fallback) helper in timingCompiler.ts (the same policy extractResolvedMedia already applies to data-duration), used at every timing-attribute read in both files.
  • compileTag additionally normalizes the compiled output: an unparsable data-start is rewritten to 0, and unparsable data-end/data-duration are stripped so the element takes the existing recompute-or-resolver path. Downstream consumers (parser, runtime) never see the garbage values.
  • parseHtml falls back to its existing defaults (start 0, the 5s default duration).

No behavior change for valid inputs.

Tests

  • Compiler: unparsable data-start normalized to 0 with data-end computed from it; unparsable data-duration dropped and the element marked unresolved; unparsable data-end stripped and recomputed; unparsable data-media-start resolved to 0; direct parseTimingAttr cases (including "Infinity"). All assert the compiled output contains no NaN.
  • Parser: unparsable data-start parses as 0; unparsable data-end falls back to the 5s default instead of NaN; updateElementInHtml recomputes data-end from 0 instead of writing NaN.
  • Full core suite: 1175 passed. bun run build green.

A typo like data-start="abc" flowed through parseFloat unguarded in
both the timing compiler and the HTML parser. The compiler serialized
it into the compiled output as data-end="NaN"; the parser turned a
garbage data-end into a NaN duration via Math.max(0, NaN), which is
NaN. Neither lint nor compile surfaced anything to the user.

compileTag now normalizes an unparsable data-start to 0 in the output,
strips unparsable data-end/data-duration so they take the existing
missing-attribute paths (recompute or resolver), and the parser falls
back to its own defaults. Same policy extractResolvedMedia already
applied to data-duration, and the same validation the composition-level
duration already gets in extractCompositionMetadata.
Comment thread packages/core/src/compiler/timingCompiler.ts Fixed
Comment thread packages/core/src/compiler/timingCompiler.ts Fixed
CodeQL flags the \s* prefix as polynomial backtracking on
attacker-controlled input (js/polynomial-redos). A single \s is
enough: the attribute always has whitespace before it inside a tag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants