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

packages/core/src/compiler/timingCompiler.test.ts

@@ -4,6 +4,7 @@ import {
   injectDurations,
   extractResolvedMedia,
   clampDurations,
+  parseTimingAttr,
 } from "./timingCompiler.js";
 
 describe("compileTimingAttrs", () => {
@@ -185,3 +186,63 @@ describe("clampDurations", () => {
     expect(result).toContain('data-end="7"');
   });
 });
+
+// ── Unparsable timing attributes ──
+//
+// A typo like data-start="abc" must not reach the compiled output: it would
+// otherwise be serialized as data-end="NaN" and parsed downstream as a NaN
+// duration. Unparsable values are treated as missing (data-start → 0,
+// data-duration/data-end → resolver path), mirroring the Number.isFinite
+// guard extractResolvedMedia already applies to data-duration.
+
+describe("unparsable timing attributes", () => {
+  it("normalizes an unparsable data-start to 0 and computes data-end from it", () => {
+    const html = '<video id="v1" src="a.mp4" data-start="abc" data-duration="5">';
+    const { html: compiled, unresolved } = compileTimingAttrs(html);
+
+    expect(compiled).toContain('data-start="0"');
+    expect(compiled).toContain('data-end="5"');
+    expect(compiled).not.toContain("NaN");
+    expect(unresolved).toHaveLength(0);
+  });
+
+  it("drops an unparsable data-duration and marks the element unresolved", () => {
+    const html = '<video id="v1" src="a.mp4" data-start="1" data-duration="oops">';
+    const { html: compiled, unresolved } = compileTimingAttrs(html);
+
+    expect(compiled).not.toContain("data-duration");
+    expect(compiled).not.toContain("NaN");
+    expect(unresolved).toHaveLength(1);
+    expect(unresolved[0].start).toBe(1);
+  });
+
+  it("strips an unparsable data-end and recomputes it from data-duration", () => {
+    const html = '<video id="v1" src="a.mp4" data-start="2" data-duration="3" data-end="oops">';
+    const { html: compiled, unresolved } = compileTimingAttrs(html);
+
+    expect(compiled).toContain('data-end="5"');
+    expect(compiled).not.toContain("oops");
+    expect(unresolved).toHaveLength(0);
+  });
+
+  it("treats an unparsable data-media-start as 0 on unresolved elements", () => {
+    const html = '<video id="v1" src="a.mp4" data-start="1" data-media-start="bad">';
+    const { unresolved } = compileTimingAttrs(html);
+
+    expect(unresolved).toHaveLength(1);
+    expect(unresolved[0].mediaStart).toBe(0);
+  });
+});
+
+describe("parseTimingAttr", () => {
+  it("parses finite values", () => {
+    expect(parseTimingAttr("2.5", 0)).toBe(2.5);
+    expect(parseTimingAttr("0", 7)).toBe(0);
+  });
+
+  it("falls back on null, unparsable, and non-finite values", () => {
+    expect(parseTimingAttr(null, 3)).toBe(3);
+    expect(parseTimingAttr("abc", 3)).toBe(3);
+    expect(parseTimingAttr("Infinity", 3)).toBe(3);
+  });
+});

packages/core/src/compiler/timingCompiler.ts

@@ -70,6 +70,20 @@ function injectAttr(tag: string, attr: string, value: string): string {
   return tag.replace(/>$/, ` ${attr}="${value}">`);
 }
 
+/**
+ * Parse a timing attribute (data-start, data-end, data-media-start),
+ * treating unparsable values as `fallback`. Without the guard a typo like
+ * data-start="abc" flows through as NaN and is serialized back into the
+ * compiled HTML as data-end="NaN", which the parser then turns into a NaN
+ * duration. Same policy extractResolvedMedia already applies to
+ * data-duration.
+ */
+export function parseTimingAttr(value: string | null, fallback: number): number {
+  if (value === null) return fallback;
+  const parsed = parseFloat(value);
+  return Number.isFinite(parsed) ? parsed : fallback;
+}
+
 // ── Core compilation ─────────────────────────────────────────────────────
 
 function compileTag(
@@ -91,15 +105,37 @@ function compileTag(
     result = injectAttr(result, "data-hf-auto-start", "");
     startStr = "0";
   }
-  const start = parseFloat(startStr);
-  const mediaStartStr = getAttr(result, "data-media-start");
-  const mediaStart = mediaStartStr ? parseFloat(mediaStartStr) : 0;
+  let start = parseFloat(startStr);
+  if (!Number.isFinite(start)) {
+    // Unparsable data-start: normalize the attribute to 0 in the output so
+    // downstream consumers (parser, runtime) never see the garbage value.
+    result = result.replace(/data-start=["'][^"']*["']/, 'data-start="0"');
+    start = 0;
+  }
+  const mediaStart = parseTimingAttr(getAttr(result, "data-media-start"), 0);
+
+  // Strip an unparsable data-end so the normal missing-data-end flow below
+  // recomputes or resolves it instead of leaving the garbage value in place.
+  // Single \s (not \s*): the attribute always has whitespace before it
+  // inside a tag, and an unbounded prefix backtracks polynomially on
+  // attacker-controlled input (CodeQL js/polynomial-redos).
+  const dataEndStr = getAttr(result, "data-end");
+  if (dataEndStr !== null && !Number.isFinite(parseFloat(dataEndStr))) {
+    result = result.replace(/\sdata-end=["'][^"']*["']/, "");
+  }
 
   // 1. Compute data-end from data-start + data-duration
   if (!hasAttr(result, "data-end")) {
     const durationStr = getAttr(result, "data-duration");
-    if (durationStr !== null) {
-      const end = start + parseFloat(durationStr);
+    const duration = durationStr !== null ? parseFloat(durationStr) : NaN;
+    if (durationStr !== null && !Number.isFinite(duration)) {
+      // Unparsable data-duration: drop it and fall through to the
+      // unresolved path so the caller's resolver supplies the real one.
+      // Single \s for the same reason as the data-end strip above.
+      result = result.replace(/\sdata-duration=["'][^"']*["']/, "");
+    }
+    if (Number.isFinite(duration)) {
+      const end = start + duration;
       result = injectAttr(result, "data-end", String(end));
     } else if (id) {
       // No data-duration: mark as unresolved so caller can provide it
@@ -165,7 +201,7 @@ export function compileTimingAttrs(html: string): CompilationResult {
       unresolved.push({
         id,
         tagName: "div",
-        start: startStr ? parseFloat(startStr) : 0,
+        start: parseTimingAttr(startStr, 0),
         mediaStart: 0,
         compositionSrc: compositionSrc ?? undefined,
       });
@@ -199,8 +235,7 @@ export function injectDurations(html: string, resolutions: ResolvedDuration[]):
 
       // Add data-end if missing
       if (!hasAttr(result, "data-end")) {
-        const startStr = getAttr(result, "data-start");
-        const start = startStr ? parseFloat(startStr) : 0;
+        const start = parseTimingAttr(getAttr(result, "data-start"), 0);
         result = injectAttr(result, "data-end", String(start + duration));
       }
 
@@ -241,9 +276,9 @@ export function extractResolvedMedia(html: string): ResolvedMediaElement[] {
       id,
       tagName: isVideo ? "video" : "audio",
       src: getAttr(tag, "src") ?? undefined,
-      start: startStr !== null ? parseFloat(startStr) : 0,
+      start: parseTimingAttr(startStr, 0),
       duration,
-      mediaStart: mediaStartStr ? parseFloat(mediaStartStr) : 0,
+      mediaStart: parseTimingAttr(mediaStartStr, 0),
       loop: hasAttr(tag, "loop"),
     });
   }
@@ -265,8 +300,7 @@ export function clampDurations(html: string, clamps: ResolvedDuration[]): string
       tag = tag.replace(/data-duration=["'][^"']*["']/, `data-duration="${duration}"`);
 
       // Recompute data-end from data-start + clamped duration
-      const startStr = getAttr(tag, "data-start");
-      const start = startStr ? parseFloat(startStr) : 0;
+      const start = parseTimingAttr(getAttr(tag, "data-start"), 0);
       tag = tag.replace(/data-end=["'][^"']*["']/, `data-end="${start + duration}"`);
 
       return tag;

packages/core/src/parsers/htmlParser.test.ts

@@ -640,3 +640,50 @@ describe("extractCompositionMetadata", () => {
     expect(meta.variables[1].type).toBe("number");
   });
 });
+
+describe("parseHtml unparsable timing attributes", () => {
+  it("treats an unparsable data-start as 0", () => {
+    const html = `
+      <html>
+      <body>
+        <div id="stage">
+          <div id="t1" data-start="abc" data-end="3"><div>x</div></div>
+        </div>
+      </body>
+      </html>
+    `;
+    const result = parseHtml(html);
+
+    expect(result.elements[0].startTime).toBe(0);
+    expect(result.elements[0].duration).toBe(3);
+  });
+
+  it("falls back to the 5s default duration for an unparsable data-end", () => {
+    const html = `
+      <html>
+      <body>
+        <div id="stage">
+          <div id="t1" data-start="1" data-end="oops"><div>x</div></div>
+        </div>
+      </body>
+      </html>
+    `;
+    const result = parseHtml(html);
+
+    // Math.max(0, NaN) is NaN — without the guard this propagated a NaN
+    // duration into the timeline model.
+    expect(result.elements[0].startTime).toBe(1);
+    expect(result.elements[0].duration).toBe(5);
+  });
+});
+
+describe("updateElementInHtml with unparsable data-start", () => {
+  it("recomputes data-end from 0 instead of writing NaN", () => {
+    const html =
+      '<html><body><div id="stage"><div id="t1" data-start="abc" data-end="9"><div>x</div></div></div></body></html>';
+    const updated = updateElementInHtml(html, "t1", { duration: 4 });
+
+    expect(updated).toContain('data-end="4"');
+    expect(updated).not.toContain("NaN");
+  });
+});

packages/core/src/parsers/htmlParser.ts

@@ -11,6 +11,7 @@ import type {
   CompositionVariable,
 } from "../core.types";
 import { validateCompositionGsap } from "./gsapSerialize";
+import { parseTimingAttr } from "../compiler/timingCompiler";
 import type { ValidationResult } from "../core.types";
 
 const MEDIA_TYPES = new Set<string>(["video", "image", "audio"]);
@@ -180,15 +181,12 @@ export function parseHtml(html: string): ParsedHtml {
     const type = getElementType(el);
     if (!type) return;
 
-    const start = parseFloat(el.getAttribute("data-start") || "0");
-    const dataEnd = el.getAttribute("data-end");
-
-    let duration: number;
-    if (dataEnd) {
-      duration = Math.max(0, parseFloat(dataEnd) - start);
-    } else {
-      duration = 5;
-    }
+    const start = parseTimingAttr(el.getAttribute("data-start"), 0);
+    // Missing and unparsable data-end both fall back to the 5s default —
+    // Math.max(0, NaN) is NaN, so a garbage data-end would otherwise
+    // propagate a NaN duration into the timeline model.
+    const end = parseTimingAttr(el.getAttribute("data-end"), start + 5);
+    const duration = Math.max(0, end - start);
 
     const id = el.id || `element-${++idCounter}`;
     const name = getElementName(el);
@@ -520,7 +518,7 @@ export function updateElementInHtml(
   }
 
   if (updates.duration !== undefined) {
-    const start = parseFloat(el.getAttribute("data-start") || "0");
+    const start = parseTimingAttr(el.getAttribute("data-start"), 0);
     el.setAttribute("data-end", String(start + updates.duration));
     el.removeAttribute("data-duration"); // Clean up legacy
   }