fix(studio): reject unsafe keyframe values (#1389)

2026-09-01 19:42:03 +00:00 · 2026-06-12 22:48:19 -04:00
parent ab7f69c1f5
commit 3bcab3dc29
10 changed files with 372 additions and 43 deletions
@@ -54,6 +54,10 @@
      "import": "./src/studio-api/helpers/draftMarkers.ts",
      "types": "./src/studio-api/helpers/draftMarkers.ts"
    },
+    "./studio-api/finite-mutation": {
+      "import": "./src/studio-api/helpers/finiteMutation.ts",
+      "types": "./src/studio-api/helpers/finiteMutation.ts"
+    },
    "./text": {
      "import": "./src/text/index.ts",
      "types": "./src/text/index.ts"
@@ -133,6 +137,10 @@
        "import": "./dist/studio-api/helpers/draftMarkers.js",
        "types": "./dist/studio-api/helpers/draftMarkers.d.ts"
      },
+      "./studio-api/finite-mutation": {
+        "import": "./dist/studio-api/helpers/finiteMutation.js",
+        "types": "./dist/studio-api/helpers/finiteMutation.d.ts"
+      },
      "./text": {
        "import": "./dist/text/index.js",
        "types": "./dist/text/index.d.ts"
@@ -0,0 +1,41 @@
+import { describe, expect, it } from "vitest";
+import { findUnsafeDomPatchValues, findUnsafeMutationValues } from "./finiteMutation";
+
+describe("finiteMutation", () => {
+  it("reports non-finite numbers before mutation serialization", () => {
+    expect(
+      findUnsafeMutationValues({
+        type: "set-arc-path",
+        segments: [{ curviness: Number.NaN, cp1: { x: Infinity, y: 0 } }],
+      }).map((field) => field.path),
+    ).toEqual(["body.segments[0].curviness", "body.segments[0].cp1.x"]);
+  });
+
+  it("treats null as unsafe because JSON serializes NaN and Infinity to null", () => {
+    expect(
+      findUnsafeMutationValues({
+        type: "update-property",
+        property: "x",
+        value: null,
+      }),
+    ).toEqual([{ path: "body.value", reason: "null" }]);
+  });
+
+  it("allows explicit DOM patch value removals while rejecting unsafe patch metadata", () => {
+    expect(
+      findUnsafeDomPatchValues({
+        target: { id: "title", selectorIndex: null },
+        operations: [{ type: "inline-style", property: "opacity", value: null }],
+      }),
+    ).toEqual([{ path: "body.target.selectorIndex", reason: "null" }]);
+  });
+
+  it("rejects non-finite DOM patch values before JSON serialization can turn them into null", () => {
+    expect(
+      findUnsafeDomPatchValues({
+        target: { id: "title" },
+        operations: [{ type: "inline-style", property: "left", value: Number.NaN }],
+      }),
+    ).toEqual([{ path: "body.operations[0].value", reason: "non-finite-number" }]);
+  });
+});
@@ -0,0 +1,38 @@
+export interface UnsafeMutationValue {
+  path: string;
+  reason: "non-finite-number" | "null";
+}
+
+interface FindUnsafeMutationValuesOptions {
+  allowNullPath?: (path: string) => boolean;
+}
+
+export function findUnsafeMutationValues(
+  value: unknown,
+  path = "body",
+  options: FindUnsafeMutationValuesOptions = {},
+): UnsafeMutationValue[] {
+  if (value === null) {
+    return options.allowNullPath?.(path) ? [] : [{ path, reason: "null" }];
+  }
+  if (typeof value === "number") {
+    return Number.isFinite(value) ? [] : [{ path, reason: "non-finite-number" }];
+  }
+  if (!value || typeof value !== "object") return [];
+  if (Array.isArray(value)) {
+    return value.flatMap((item, index) =>
+      findUnsafeMutationValues(item, `${path}[${index}]`, options),
+    );
+  }
+  return Object.entries(value).flatMap(([key, item]) =>
+    findUnsafeMutationValues(item, `${path}.${key}`, options),
+  );
+}
+
+const DOM_PATCH_NULL_VALUE_PATH = /^body\.operations\[\d+\]\.value$/;
+
+export function findUnsafeDomPatchValues(value: unknown): UnsafeMutationValue[] {
+  return findUnsafeMutationValues(value, "body", {
+    allowNullPath: (path) => DOM_PATCH_NULL_VALUE_PATH.test(path),
+  });
+}
@@ -19,18 +19,21 @@ function createProjectDir(): string {
 }

 describe("walkDir", () => {
-  it("hides internal HyperFrames backup files from project listings", () => {
+  it("hides internal HyperFrames files from project listings", () => {
    const projectDir = createProjectDir();
    mkdirSync(join(projectDir, ".hyperframes", "backup"), { recursive: true });
    mkdirSync(join(projectDir, ".hyperframes", "examples"), { recursive: true });
+    mkdirSync(join(projectDir, ".cache", "examples"), { recursive: true });
    mkdirSync(join(projectDir, "compositions"), { recursive: true });
    writeFileSync(join(projectDir, ".hyperframes", "backup", "snapshot.html"), "backup");
    writeFileSync(join(projectDir, ".hyperframes", "examples", "preset.html"), "preset");
+    writeFileSync(join(projectDir, ".cache", "examples", "preset.html"), "preset");
    writeFileSync(join(projectDir, "compositions", "scene.html"), "scene");

-    expect(walkDir(projectDir)).toEqual([
-      ".hyperframes/examples/preset.html",
-      "compositions/scene.html",
-    ]);
+    const files = walkDir(projectDir);
+    expect(files).toContain(".cache/examples/preset.html");
+    expect(files).toContain("compositions/scene.html");
+    expect(files).not.toContain(".hyperframes/backup/snapshot.html");
+    expect(files).not.toContain(".hyperframes/examples/preset.html");
  });
 });
@@ -7,11 +7,7 @@ export function isSafePath(base: string, resolved: string): boolean {
  return resolved.startsWith(norm) || resolved === resolve(base);
 }

-const IGNORE_DIRS = new Set([".thumbnails", "node_modules", ".git"]);
-
-function shouldIgnoreDir(rel: string): boolean {
-  return rel === ".hyperframes/backup";
-}
+const IGNORE_DIRS = new Set([".thumbnails", ".hyperframes", "node_modules", ".git"]);

 /**
 * True when any directory segment of a relative path is a dot-directory or
@@ -30,7 +26,7 @@ export function walkDir(dir: string, prefix = ""): string[] {
  const files: string[] = [];
  for (const entry of readdirSync(dir, { withFileTypes: true })) {
    const rel = prefix ? `${prefix}/${entry.name}` : entry.name;
-    if (IGNORE_DIRS.has(entry.name) || shouldIgnoreDir(rel)) continue;
+    if (IGNORE_DIRS.has(entry.name)) continue;
    if (entry.isDirectory()) {
      files.push(...walkDir(join(dir, entry.name), rel));
    } else {
@@ -239,6 +239,86 @@ tl.fromTo("#box", { opacity: 0, x: -50 }, { opacity: 1, x: 0, duration: 1.5, eas
    expect(result.parsed.animations[0].fromProperties?.x).toBe(-50);
  });

+  it("rejects serialized non-finite mutation values before writing source", async () => {
+    const projectDir = createProjectDir();
+    writeHtml(projectDir, "comp.html", FROMTO_COMP);
+    const app = new Hono();
+    registerFileRoutes(app, createAdapter(projectDir));
+
+    const anim = await getFirstAnimation(app, "comp.html");
+    const before = readFileSync(join(projectDir, "comp.html"), "utf-8");
+    const res = await app.request("http://localhost/projects/demo/gsap-mutations/comp.html", {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify({
+        type: "update-property",
+        animationId: anim.id,
+        property: "x",
+        value: Number.NaN,
+      }),
+    });
+    const payload = (await res.json()) as { error?: string; fields?: string[] };
+
+    expect(res.status).toBe(400);
+    expect(payload.error).toContain("unsafe values");
+    expect(payload.fields).toContain("body.value");
+    expect(readFileSync(join(projectDir, "comp.html"), "utf-8")).toBe(before);
+  });
+
+  it("rejects unsafe DOM patch metadata before writing source", async () => {
+    const projectDir = createProjectDir();
+    writeFileSync(join(projectDir, "index.html"), '<div id="title">Before</div>');
+    const app = new Hono();
+    registerFileRoutes(app, createAdapter(projectDir));
+
+    const response = await app.request(
+      "http://localhost/projects/demo/file-mutations/patch-element/index.html",
+      {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          target: { id: "title", selectorIndex: Number.NaN },
+          operations: [{ type: "text-content", property: "textContent", value: "After" }],
+        }),
+      },
+    );
+    const payload = (await response.json()) as { error?: string; fields?: string[] };
+
+    expect(response.status).toBe(400);
+    expect(payload.error).toContain("unsafe values");
+    expect(payload.fields).toContain("body.target.selectorIndex");
+    expect(readFileSync(join(projectDir, "index.html"), "utf-8")).toBe(
+      '<div id="title">Before</div>',
+    );
+  });
+
+  it("allows DOM patch null values used for explicit style removals", async () => {
+    const projectDir = createProjectDir();
+    writeFileSync(
+      join(projectDir, "index.html"),
+      '<div id="title" style="opacity: 1">Before</div>',
+    );
+    const app = new Hono();
+    registerFileRoutes(app, createAdapter(projectDir));
+
+    const response = await app.request(
+      "http://localhost/projects/demo/file-mutations/patch-element/index.html",
+      {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          target: { id: "title" },
+          operations: [{ type: "inline-style", property: "opacity", value: null }],
+        }),
+      },
+    );
+    const payload = (await response.json()) as { changed?: boolean; content?: string };
+
+    expect(response.status).toBe(200);
+    expect(payload.changed).toBe(true);
+    expect(payload.content).not.toContain("opacity");
+  });
+
  it("update-from-property returns 400 for a non-fromTo animation", async () => {
    const projectDir = createProjectDir();
    const TO_COMP = `<!DOCTYPE html><html><body><script data-hyperframes-gsap>
@@ -18,6 +18,11 @@ import { generateWaveformCache } from "../helpers/waveform.js";
 import { validateUploadedMediaBuffer } from "../helpers/mediaValidation.js";
 import { isSafePath } from "../helpers/safePath.js";
 import { backupPathForResponse, snapshotBeforeWrite } from "../helpers/backupJournal.js";
+import {
+  findUnsafeDomPatchValues,
+  findUnsafeMutationValues,
+  type UnsafeMutationValue,
+} from "../helpers/finiteMutation.js";
 import type { GsapAnimation } from "../../parsers/gsapSerialize.js";
 import {
  removeElementFromHtml,
@@ -116,6 +121,20 @@ function writeIfChanged(
  });
 }

+function rejectUnsafeMutationValues(
+  c: RouteContext,
+  unsafeFields: UnsafeMutationValue[],
+): Response {
+  return c.json(
+    {
+      error: "mutation contains unsafe values",
+      fields: unsafeFields.map((field) => field.path),
+      unsafeValues: unsafeFields,
+    },
+    400,
+  );
+}
+
 /**
 * Parse the request body and validate that `target` is present.
 * Returns `{ error }` if missing, or `{ target, body }` for the full parsed body.
@@ -951,6 +970,10 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
    if (!Array.isArray(parsed.body.operations) || parsed.body.operations.length === 0) {
      return c.json({ error: "target and operations required" }, 400);
    }
+    const unsafeFields = findUnsafeDomPatchValues(parsed.body);
+    if (unsafeFields.length > 0) {
+      return rejectUnsafeMutationValues(c, unsafeFields);
+    }

    let originalContent: string;
    try {
@@ -1125,6 +1148,10 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
    if (!body || !body.type) {
      return c.json({ error: "mutation type required" }, 400);
    }
+    const unsafeFields = findUnsafeMutationValues(body);
+    if (unsafeFields.length > 0) {
+      return rejectUnsafeMutationValues(c, unsafeFields);
+    }

    let html = readFileSync(res.absPath, "utf-8");
    let block = extractGsapScriptBlock(html);
@@ -1,3 +1,4 @@
+import { findUnsafeDomPatchValues } from "@hyperframes/core/studio-api/finite-mutation";
 import type { DomEditSelection } from "../components/editor/domEditingTypes";

 export const PROPERTY_DEFAULTS: Record<string, number> = {
@@ -31,3 +32,97 @@ export function ensureElementAddressable(selection: DomEditSelection): {
  el.setAttribute("id", id);
  return { selector: `#${id}`, autoId: id };
 }
+
+export class GsapMutationHttpError extends Error {
+  constructor(
+    readonly statusCode: number,
+    readonly responseBody: unknown,
+  ) {
+    super(formatGsapMutationHttpErrorMessage(statusCode, responseBody));
+    this.name = "GsapMutationHttpError";
+  }
+}
+
+function isRecord(value: unknown): value is Record<string, unknown> {
+  return typeof value === "object" && value !== null;
+}
+
+export async function readJsonResponseBody(res: Response): Promise<unknown> {
+  const contentType = res.headers.get("content-type") ?? "";
+  if (!contentType.includes("application/json")) {
+    return await res.text().catch(() => null);
+  }
+  return await res.json().catch(() => null);
+}
+
+function formatGsapMutationHttpErrorMessage(statusCode: number, body: unknown): string {
+  if (isRecord(body) && typeof body.error === "string") {
+    return body.error;
+  }
+  return `GSAP mutation failed with status ${statusCode}`;
+}
+
+export function formatGsapMutationRejectionToast(error: GsapMutationHttpError): string {
+  const body = error.responseBody;
+  if (isRecord(body)) {
+    const fields = Array.isArray(body.fields)
+      ? body.fields.filter((field): field is string => typeof field === "string")
+      : [];
+    const suffix = fields.length > 0 ? ` (${fields.join(", ")})` : "";
+    return `Couldn't save animation: ${formatGsapMutationHttpErrorMessage(
+      error.statusCode,
+      body,
+    )}${suffix}`;
+  }
+  return `Couldn't save animation: ${error.message}`;
+}
+
+interface AssignAutoIdParams {
+  projectId: string;
+  targetPath: string;
+  selection: DomEditSelection;
+  autoId: string;
+  showToast?: (message: string, tone?: "error" | "info") => void;
+}
+
+export async function assignGsapTargetAutoIdIfNeeded({
+  projectId,
+  targetPath,
+  selection,
+  autoId,
+  showToast,
+}: AssignAutoIdParams): Promise<boolean> {
+  const patchBody = {
+    target: {
+      id: selection.id,
+      hfId: selection.hfId,
+      selector: selection.selector,
+      selectorIndex: selection.selectorIndex,
+    },
+    operations: [{ type: "html-attribute", property: "id", value: autoId }],
+  };
+  const unsafePatchFields = findUnsafeDomPatchValues(patchBody);
+  if (unsafePatchFields.length > 0) {
+    showToast?.("Couldn't assign element id because the patch contains invalid values", "error");
+    return false;
+  }
+  const res = await fetch(
+    `/api/projects/${encodeURIComponent(projectId)}/file-mutations/patch-element/${encodeURIComponent(targetPath)}`,
+    {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify(patchBody),
+    },
+  );
+  if (!res.ok) {
+    showToast?.(
+      formatGsapMutationRejectionToast(
+        new GsapMutationHttpError(res.status, await readJsonResponseBody(res)),
+      ),
+      "error",
+    );
+    return false;
+  }
+  const data = (await res.json()) as { changed?: boolean };
+  return data.changed === true;
+}
@@ -1,4 +1,5 @@
 import { useCallback, useRef } from "react";
+import { findUnsafeDomPatchValues } from "@hyperframes/core/studio-api/finite-mutation";
 import { usePlayerStore } from "../player";
 import { STUDIO_GSAP_DRAG_INTERCEPT_ENABLED } from "../components/editor/manualEditingAvailability";
 import { FONT_EXT } from "../utils/mediaTypes";
@@ -38,6 +39,27 @@ import { useDomEditTextCommits } from "./useDomEditTextCommits";
 // ── Helpers ──
 type TimelineLike = { getChildren?: (nested: boolean) => Array<{ targets?: () => Element[] }> };

+function formatUnsafeFieldList(fields: Array<{ path: string }>): string {
+  return fields.map((field) => field.path).join(", ");
+}
+
+async function readErrorResponseBody(
+  response: Response,
+): Promise<{ error?: string; fields?: string[] } | null> {
+  const contentType = response.headers.get("content-type") ?? "";
+  if (!contentType.includes("application/json")) return null;
+  return (await response.json().catch(() => null)) as { error?: string; fields?: string[] } | null;
+}
+
+function formatPatchRejectionMessage(body: { error?: string; fields?: string[] } | null): string {
+  if (!body?.error) return "Couldn't save edit";
+  const fields = Array.isArray(body.fields)
+    ? body.fields.filter((field): field is string => typeof field === "string")
+    : [];
+  const suffix = fields.length > 0 ? ` (${fields.join(", ")})` : "";
+  return `Couldn't save edit: ${body.error}${suffix}`;
+}
+
 export const GSAP_CSS_FALLBACK_BLOCKED_MESSAGE =
  "This element is GSAP-animated — dragging via CSS would corrupt keyframes";

@@ -192,6 +214,13 @@ export function useDomEditCommits({
      if (options?.shouldSave && !options.shouldSave()) return;

      const patchTarget = buildDomEditPatchTarget(selection);
+      const patchBody = { target: patchTarget, operations };
+      const unsafeFields = findUnsafeDomPatchValues(patchBody);
+      if (unsafeFields.length > 0) {
+        const fields = formatUnsafeFieldList(unsafeFields);
+        showToast("Couldn't save edit because it contains invalid layout values", "error");
+        throw new Error(`DOM patch contains unsafe values: ${fields}`);
+      }

      // Mark the save timestamp before the file write so the SSE file-change
      // handler suppresses the reload even if the event arrives before the
@@ -203,10 +232,11 @@ export function useDomEditCommits({
        {
          method: "POST",
          headers: { "Content-Type": "application/json" },
-          body: JSON.stringify({ target: patchTarget, operations }),
+          body: JSON.stringify(patchBody),
        },
      );
      if (!patchResponse.ok) {
+        showToast(formatPatchRejectionMessage(await readErrorResponseBody(patchResponse)), "error");
        throw await createStudioSaveHttpError(patchResponse, `Failed to patch ${targetPath}`);
      }

@@ -266,6 +296,7 @@ export function useDomEditCommits({
      projectIdRef,
      domEditSaveTimestampRef,
      reloadPreview,
+      showToast,
    ],
  );

@@ -1,5 +1,6 @@
 import { useCallback, useEffect, useRef } from "react";
 import type { GsapAnimation, ParsedGsap } from "@hyperframes/core/gsap-parser";
+import { findUnsafeMutationValues } from "@hyperframes/core/studio-api/finite-mutation";
 import type { DomEditSelection } from "../components/editor/domEditingTypes";
 import type { EditHistoryKind } from "../utils/editHistory";
 import { applySoftReload } from "../utils/gsapSoftReload";
@@ -11,12 +12,18 @@ import {
  readKeyframeSnapshot,
  writeKeyframeCache,
 } from "./gsapKeyframeCacheHelpers";
-import { createStudioSaveHttpError } from "../utils/studioSaveDiagnostics";
 import {
  useGsapSaveFailureTelemetry,
  useSafeGsapCommitMutation,
 } from "./useSafeGsapCommitMutation";
-import { ensureElementAddressable, PROPERTY_DEFAULTS } from "./gsapScriptCommitHelpers";
+import {
+  GsapMutationHttpError,
+  assignGsapTargetAutoIdIfNeeded,
+  ensureElementAddressable,
+  formatGsapMutationRejectionToast,
+  PROPERTY_DEFAULTS,
+  readJsonResponseBody,
+} from "./gsapScriptCommitHelpers";

 interface MutationResult {
  ok: boolean;
@@ -41,7 +48,7 @@ async function mutateGsapScript(
    },
  );
  if (!res.ok) {
-    throw await createStudioSaveHttpError(res, `Failed to update GSAP in ${sourceFile}`);
+    throw new GsapMutationHttpError(res.status, await readJsonResponseBody(res));
  }
  const result = (await res.json()) as MutationResult;
  if (!result.ok) {
@@ -124,8 +131,28 @@ export function useGsapScriptCommits({
    ) => {
      const pid = projectIdRef.current;
      if (!pid) return;
+      const unsafeFields = findUnsafeMutationValues(mutation);
+      if (unsafeFields.length > 0) {
+        showToast?.(
+          "Couldn't read element layout — try again at a different playhead time",
+          "error",
+        );
+        if (options.skipReload) return;
+        throw new Error(
+          `Mutation contains unsafe values: ${unsafeFields.map((field) => field.path).join(", ")}`,
+        );
+      }
      const targetPath = selection.sourceFile || activeCompPath || "index.html";
-      const result = await mutateGsapScript(pid, targetPath, mutation);
+      let result: MutationResult;
+      try {
+        result = await mutateGsapScript(pid, targetPath, mutation);
+      } catch (error) {
+        if (error instanceof GsapMutationHttpError) {
+          showToast?.(formatGsapMutationRejectionToast(error), "error");
+        }
+        if (options.skipReload) return;
+        throw error;
+      }

      if (result.changed === false) {
        if (options.skipReload) return;
@@ -184,6 +211,7 @@ export function useGsapScriptCommits({
      reloadPreview,
      onCacheInvalidate,
      onFileContentChanged,
+      showToast,
    ],
  );

@@ -280,32 +308,14 @@ export function useGsapScriptCommits({
        const pid = projectIdRef.current;
        const targetPath = selection.sourceFile || activeCompPath || "index.html";
        if (!pid) return;
-        const res = await fetch(
-          `/api/projects/${encodeURIComponent(pid)}/file-mutations/patch-element/${encodeURIComponent(targetPath)}`,
-          {
-            method: "POST",
-            headers: { "Content-Type": "application/json" },
-            body: JSON.stringify({
-              target: {
-                id: selection.id,
-                hfId: selection.hfId,
-                selector: selection.selector,
-                selectorIndex: selection.selectorIndex,
-              },
-              operations: [{ type: "html-attribute", property: "id", value: autoId }],
-            }),
-          },
-        );
-        if (!res.ok) {
-          throw await createStudioSaveHttpError(
-            res,
-            `Failed to assign element id in ${targetPath}`,
-          );
-        }
-        const data = (await res.json()) as { changed?: boolean };
-        if (!data.changed) {
-          throw new Error(`Failed to assign element id in ${targetPath}`);
-        }
+        const assigned = await assignGsapTargetAutoIdIfNeeded({
+          projectId: pid,
+          targetPath,
+          selection,
+          autoId,
+          showToast,
+        });
+        if (!assigned) return;
      }

      const elStart = Number.parseFloat(selection.dataAttributes?.start ?? "0") || 0;
@@ -334,7 +344,7 @@ export function useGsapScriptCommits({
        { label: `Add GSAP ${method} animation` },
      );
    },
-    [commitMutation, projectIdRef, activeCompPath],
+    [commitMutation, projectIdRef, activeCompPath, showToast],
  );
  const addGsapProperty = useCallback(
    // fallow-ignore-next-line complexity