fix(sdk,core): css tokenizer, override-set replay, setattribute safety, persist errors

Author: vanceingalls

bun.lock

@@ -223,10 +223,35 @@ function isSafeAttributeValue(name: string, value: string): boolean {
   return true;
 }
 
+// fallow-ignore-next-line complexity
 function patchStyleAttrString(style: string, property: string, value: string | null): string {
   const props = new Map<string, string>();
   const order: string[] = [];
-  for (const decl of style.split(";")) {
+  // Tokenize declarations robustly: values can contain ';' inside quoted strings
+  // (e.g. content: ';') and ':' inside values (data URIs, url(), etc.).
+  // Split on ';' only when outside quotes and balanced parens; the first ':' in
+  // the resulting segment is the property/value separator (property names never
+  // contain ':').
+  let i = 0;
+  while (i < style.length) {
+    let depth = 0;
+    let inSingle = false;
+    let inDouble = false;
+    const start = i;
+    while (i < style.length) {
+      const ch = style[i];
+      if (ch === "'" && !inDouble) inSingle = !inSingle;
+      else if (ch === '"' && !inSingle) inDouble = !inDouble;
+      else if (!inSingle && !inDouble) {
+        if (ch === "(") depth++;
+        else if (ch === ")") depth = Math.max(0, depth - 1);
+        else if (ch === ";" && depth === 0) break;
+      }
+      i++;
+    }
+    const decl = style.slice(start, i).trim();
+    i++; // advance past ';'
+    if (!decl) continue;
     const colon = decl.indexOf(":");
     if (colon < 0) continue;
     const key = decl.slice(0, colon).trim();

packages/core/src/studio-api/helpers/sourceMutation.ts

@@ -59,16 +59,17 @@ function parsePath(path: string): ParsedPath | null {
 
 /**
  * Replay a stored override-set onto a freshly-parsed base document (T3 init).
- * Property keys with null mean "restore base value" — a no-op on a fresh base.
- * Bare element keys with null are removal markers — the element is removed.
+ * A null value means the property was explicitly deleted — emit a remove patch
+ * so the base document matches the session state. (Removing a non-existent
+ * property is a no-op in applyOne, so this is safe against fresh-base misses.)
  */
 export function applyOverrideSet(parsed: ParsedDocument, overrides: OverrideSet): void {
   const patches: JsonPatchOp[] = [];
   for (const [key, value] of Object.entries(overrides)) {
     const path = keyToPath(key);
     if (!path) continue;
     if (value === null) {
-      if (!key.includes(".")) patches.push({ op: "remove", path });
+      patches.push({ op: "remove", path });
       continue;
     }
     patches.push({ op: "replace", path, value });

packages/sdk/src/engine/apply-patches.ts

@@ -40,19 +40,57 @@ export interface MutationResult {
 
 const EMPTY: MutationResult = { forward: [], inverse: [] };
 
-/** Ops that require the Phase 3b parser-backed engine (meriyah/css-tree). */
-const PHASE3B_OPS = new Set([
-  "setClassStyle",
-  "addGsapTween",
-  "setGsapTween",
-  "setGsapKeyframe",
-  "addGsapKeyframe",
-  "removeGsapKeyframe",
-  "removeGsapTween",
-  "addLabel",
-  "removeLabel",
+// ─── setAttribute safety ────────────────────────────────────────────────────
+
+// Composition-reserved attributes — changing these breaks element identity or
+// the core/studio data model. Reject before mutating.
+const RESERVED_ATTRS = new Set([
+  "data-hf-id",
+  "data-composition-id",
+  "data-width",
+  "data-height",
+  "data-start",
+  "data-end",
+  "data-track-index",
+  "data-hold-start",
+  "data-hold-end",
+  "data-hold-fill",
 ]);
 
+const DANGEROUS_URI_SCHEMES = /^(?:javascript|vbscript):/i;
+const DANGEROUS_DATA_URI = /^data\s*:\s*text\/html/i;
+const URI_BEARING_ATTRS = new Set([
+  "src",
+  "href",
+  "action",
+  "formaction",
+  "poster",
+  "srcset",
+  "xlink:href",
+]);
+
+function validateSetAttribute(name: string, value: string | null): void {
+  const lower = name.toLowerCase();
+  if (RESERVED_ATTRS.has(lower)) {
+    throw new Error(
+      `setAttribute: "${name}" is a reserved composition attribute and cannot be reassigned. ` +
+        `Use the appropriate typed method (setTiming, setHold, etc.) instead.`,
+    );
+  }
+  if (lower.startsWith("on")) {
+    throw new Error(
+      `setAttribute: event-handler attributes ("${name}") are not permitted — ` +
+        `they produce executable HTML that cannot be safely serialized.`,
+    );
+  }
+  if (value !== null && URI_BEARING_ATTRS.has(lower)) {
+    const trimmed = value.trim();
+    if (DANGEROUS_URI_SCHEMES.test(trimmed) || DANGEROUS_DATA_URI.test(trimmed)) {
+      throw new Error(`setAttribute: unsafe URI value for "${name}".`);
+    }
+  }
+}
+
 export class UnsupportedOpError extends Error {
   // Stable error code — part of the public API contract (F7); hosts switch on
   // err.code rather than the message.
@@ -156,7 +194,11 @@ function handleSetText(parsed: ParsedDocument, ids: HfId[], value: string): Muta
     const oldText = getOwnText(el);
     setOwnText(el, value);
     const path = textPath(id);
-    const p = scalarChange(path, oldText || null, value);
+    // getOwnText always returns string ("" for empty) — use it directly so
+    // the forward patch is always op:'replace', not op:'add'. An op:'add' on
+    // a text path is semantically wrong for external JSON-patch consumers
+    // (the path already exists; add would fail on strict appliers).
+    const p = scalarChange(path, oldText, value);
     result.forward.push(p.forward);
     result.inverse.push(p.inverse);
   }
@@ -169,6 +211,7 @@ function handleSetAttribute(
   name: string,
   value: string | null,
 ): MutationResult {
+  validateSetAttribute(name, value);
   const result: MutationResult = { forward: [], inverse: [] };
   for (const id of ids) {
     const el = findById(parsed.document, id);
@@ -387,9 +430,10 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): boolean {
       return findRoot(parsed.document) !== null;
     case "setCompositionMetadata":
       return true;
-    // Phase 3b — not implemented yet; can() must report false so callers
-    // can feature-detect instead of hitting UnsupportedOpError.
+    // Phase 3b and unknown ops — report false so callers can feature-detect.
+    // An unknown op type must never silently pass validation only to no-op
+    // or throw in applyOp (which would violate the can() contract).
     default:
-      return !PHASE3B_OPS.has(op.type);
+      return false;
   }
 }

packages/sdk/src/engine/mutate.ts

@@ -115,7 +115,9 @@ export function keyToPath(key: string): string | null {
   if (text?.[1]) return textPath(text[1]);
 
   const attr = /^([^.]+)\.attr\.(.+)$/.exec(key);
-  if (attr?.[1] && attr[2]) return attrPath(attr[1], attr[2]);
+  // pathToKey stores the RFC 6902-encoded segment verbatim; do NOT call attrPath()
+  // here (it would re-escape '~' → '~0'), just reconstruct the path directly.
+  if (attr?.[1] && attr[2]) return `/elements/${attr[1]}/attributes/${attr[2]}`;
 
   const timing = /^([^.]+)\.timing\.(start|end|trackIndex)$/.exec(key);
   if (timing?.[1]) return timingPath(timing[1], timing[2] as "start" | "end" | "trackIndex");

packages/sdk/src/engine/patches.ts

@@ -8,7 +8,7 @@
  * T3 (embedded) hosts own persistence — do not use this module.
  */
 
-import type { Composition } from "./types.js";
+import type { Composition, PersistErrorEvent } from "./types.js";
 import type { PersistAdapter } from "./adapters/types.js";
 
 export interface PersistQueueModule {
@@ -20,6 +20,8 @@ export interface PersistQueueModule {
 export interface PersistQueueOptions {
   /** Adapter path to write to. Default: "composition.html" */
   path?: string;
+  /** Called when adapter.write() rejects. */
+  onError?: (e: PersistErrorEvent) => void;
 }
 
 export function createPersistQueue(
@@ -48,8 +50,9 @@ export function createPersistQueue(
       if (disposed) return;
       try {
         await adapter.write(path, content);
-      } catch {
-        // error already surfaced via persist:error on the adapter
+      } catch (err) {
+        const message = err instanceof Error ? err.message : String(err);
+        opts.onError?.({ error: { message, cause: err } });
       }
     });
     return writeChain;

packages/sdk/src/persist-queue.ts

@@ -89,11 +89,13 @@ describe("override-set replay on open", () => {
     expect(comp.serialize()).not.toContain("subtitle");
   });
 
-  it("treats property-level null as restore-base (no-op on fresh base)", async () => {
+  it("treats property-level null as a deletion marker — removes the property from the base", async () => {
+    // Null in the override-set is emitted only from patchRemove (explicit deletion).
+    // On replay against a base that has the property set, it must be removed.
     const comp = await openComposition(BASE_HTML, {
       overrides: { "hf-title.style.color": null },
     });
-    expect(comp.getElement("hf-title")?.inlineStyles["color"]).toBe("#fff");
+    expect(comp.getElement("hf-title")?.inlineStyles["color"]).toBeUndefined();
   });
 
   it("getOverrides returns the set the session was opened with", async () => {

packages/sdk/src/session.test.ts

@@ -95,6 +95,10 @@ class CompositionImpl implements Composition {
     this.persistQueueModule = module;
   }
 
+  _fireError(e: PersistErrorEvent): void {
+    this.errorHandlers.forEach((h) => h(e));
+  }
+
   // ── Typed methods (F10 layer 1) ─────────────────────────────────────────────
 
   setStyle(id: HfId, styles: Record<string, string | null>): void {
@@ -272,9 +276,13 @@ class CompositionImpl implements Composition {
             this.batchOrigin,
             this.batchOpTypes,
           );
-          this.resetBatchState();
+          // Fire handlers before resetting batch state so that if a handler
+          // throws the patch data (batchForward/batchInverse) is still intact
+          // for callers that inspect it on error. The event was already built
+          // from a snapshot so handler re-entrancy does not corrupt the event.
           this.patchHandlers.forEach((h) => h(event));
           this.changeHandlers.forEach((h) => h());
+          this.resetBatchState();
         } else {
           if (threw && this.batchInverse.length > 0) {
             // Roll back: the dispatches inside the batch already mutated the
@@ -427,7 +435,9 @@ export async function openComposition(
     session.attachHistory(history);
 
     if (opts?.persist) {
-      const pq = createPersistQueue(session, opts.persist);
+      const pq = createPersistQueue(session, opts.persist, {
+        onError: (e) => session._fireError(e),
+      });
       session.attachPersistQueue(pq);
     }
   }