fix(skills): clear two Snyk Fails and harden the network + supply-chain surface

[WaterrrForever]

What

Resolve the security-audit findings surfaced on the published skills (skills.sh — Gen / Socket / Snyk) with no change to any skill's runtime behaviour: two Snyk Fails cleared, plus targeted network / supply-chain hardening. Also folds in one unrelated routing-doc clarification (called out separately below).

Why

The skills.sh audit flagged two skills as Snyk Fail and several as Warn. The two Fails are SAST pattern matches — not runtime-exploitable in our offline render context, but they block adoption in Snyk-gated environments and read badly on an official catalog:

• media-use — an execSync call built from an interpolated command string (CWE-78).
• music-to-video — element.innerHTML = <var> in two frame templates (CWE-79).

The remaining flagged items are real-but-legitimate capabilities (network, subprocess, runtime fetches). Where a capability had an avoidable sharp edge we hardened it; where it's inherent we left it (see Not in scope).

Changes (security)

Skill	Change	Finding addressed
media-use	resolve.test.mjs runs resolve.mjs via execFileSync with an argv array, instead of an interpolated execSync command string	command injection (CWE-78) — Snyk Fail
music-to-video	dynamic innerHTML → setSvg() that sanitizes with a default-deny allow-list (cleanSvg): DOMParser builds the tree, then only inert drawing elements + presentation attributes survive — <script>, <image>/<use>/<foreignObject>/<a>/<animate>, every on* handler and href/xlink:href/style are stripped (root included) before nodes enter the document. Trusted icons render byte-identically.	DOM-XSS (CWE-79) — Snyk Fail
pr-to-video	fetch-people-avatars.mjs refuses any avatar URL that isn't https on a GitHub avatar host, and only writes under the project dir	SSRF (CWE-918) + path traversal
embedded-captions	pin uvx --from whisperx==3.8.6 (override via $WHISPERX_VERSION)	runtime "latest" dependency fetch
embedded-captions, music-to-video, faceless-explainer, pr-to-video, product-launch-video (8 tags)	add SRI integrity + crossorigin to the render-time CDN GSAP <script>	unpinned external script (tamper-evidence)
hyperframes-animation, hyperframes-creative	document package-loader's defense-in-depth and note the installLine strings are display-only	npm-bootstrap clarity for reviewers

Also in this PR (non-security)

• hyperframes/SKILL.md — routing-rule clarification, not a Snyk fix. Sharpens the product-vs-site decision: the split becomes "is the site selling a product?" → /product-launch-video (promo; default for any commercial URL) vs /website-to-video (tour of a non-commercial site). Updates the workflow table, the disambiguation bullet, and both workflows' Input/Output blurbs. Flagged separately per REVIEW_DISCIPLINE rule 3 (per Rames' review).

Test

• media-use: resolve (12/12), probe injection regression (1/1), manifest (19/19) pass.
• pr-to-video: avatar host-allowlist checks pass — accepts GitHub avatar hosts; rejects other hosts, non-https, suffix-spoofs, and the cloud-metadata endpoint.
• music-to-video SVG sanitizer — verified in headless Chrome (both templates' helper): the prior importNode-only shape fired ["script","onload"]; the new setSvg fires [], the trusted icon still renders, and the output has 0 danger nodes / 0 on* attributes.
• All changed JS passes node --check; both templates' inline <script> blocks compile; bunx oxfmt --check clean.

Reviewer notes

• Manifest hash bumps (Rames' flag initial code #2): against current main, skills-manifest.json bumps only the 9 skills actually modified here — hyperframes-core / slideshow are not among them. The bumps you saw were main's own fix: storyboard-angle review follow-ups (M1 bg-on-clip, B3 slideshow, parser guard, CLI fixes) #1791 (which edited slideshow/SKILL.md) surfaced by merging main into this branch against the pre-fix: storyboard-angle review follow-ups (M1 bg-on-clip, B3 slideshow, parser guard, CLI fixes) #1791 base, not changes from this PR. The generator is deterministic (verified by a double-run producing identical output).

Not in scope

• Capability-surface Warns that stem from necessary features are left as-is (e.g. embedded-captions' subprocess + model-download surface, hyperframes-media's credential + network use). Hardening them would remove the feature.
• product-launch-video / website-to-video Snyk Warns: not pinned to a precise sink from static reading — targeting them cleanly needs the actual snyk code test output.
• GSAP stays on the (now SRI-pinned) CDN; fully offline rendering would require vendoring it locally — a separate change.

[miga-heygen]

Review: fix(skills) — clear Snyk Fails + supply-chain hardening

SHA: 57cf9c1

Clean, focused security-hardening PR that addresses real audit findings without changing runtime behavior. The changes are well-scoped and the PR description does a great job of categorizing what was addressed, what was hardened, and what was deliberately left alone.

Verdict: LGTM — no blockers

This is solid work. Here are my notes by area:

---

media-use: execSync → execFileSync (CWE-78 fix)

The resolveCmd() string-builder is replaced with runResolve() using an argv array — exactly the right fix. Each argument is passed as a discrete array element, eliminating shell metacharacter injection. The helper defaults cwd and encoding so call sites are cleaner too. Well done.

music-to-video: innerHTML → setSvg() (CWE-79 fix)

The setSvg() helper uses DOMParser with image/svg+xml + importNode, falling back to textContent for non-SVG input. This is the textbook safe-DOM pattern. One subtlety worth noting (not a blocker):

• DOMParser.parseFromString("invalid", "image/svg+xml") produces a <parsererror> document, not null. The root.nodeName.toLowerCase() === "svg" check catches this correctly — parsererror won't match "svg", so it falls through to the textContent fallback. Good.

Both templates (intro-kinetic-cascade and logo-split-lockup-pulse) define their own copy of setSvg() inline. That is fine for frame templates that need to be self-contained — no shared module to import in an HTML <script> block. The duplication is inherent to the medium.

pr-to-video: SSRF + path-traversal guards

The isAllowedAvatarUrl() function is well-structured:

• Protocol check (https: only) blocks file://, http://, cloud-metadata http://169.254.169.254.
• Host allowlist: avatars.githubusercontent.com, github.com, www.github.com — all GitHub-controlled.
• The host.endsWith(".githubusercontent.com") wildcard is safe because *.githubusercontent.com is a GitHub-owned zone; a domain like evil-githubusercontent.com (no dot prefix) would NOT match the endsWith check.

The isUnderProject() path guard using resolve() + startsWith(projectDir + sep) is the standard Node pattern for directory containment. The sep suffix prevents projectDir + "other" from matching — correct.

Nice touch keeping the script's always-exit-0 contract intact while adding the guards.

embedded-captions: whisperx version pin

Pinning whisperx==3.8.6 with $WHISPERX_VERSION override is a good balance of reproducibility and flexibility. The uvx --from syntax is the right mechanism.

SRI on GSAP CDN tags

All 8 <script> tags now carry the same integrity + crossorigin attribute. The hash is consistent across all occurrences. This gives tamper-evidence for the CDN-fetched GSAP script — a meaningful hardening even though full offline vendoring is deferred.

package-loader documentation

The defense-in-depth comment block is accurate and useful for reviewers scanning for npm-bootstrap patterns. The installLine display-only clarification preempts the obvious "are these executed?" question.

SKILL.md routing clarification

The product-vs-site disambiguation is clearer now — "is the site selling a product?" is a better decision heuristic than the previous "markets a specific product" phrasing.

---

CI

Format, File size, CodeQL, Skills manifest, Skills tests all pass. Other checks skipped (no core package changes detected). No issues.

Summary

Every change is narrowly targeted at a specific finding, the behavioral contract of each script is preserved, and the hardening choices are proportionate. Ship it.

— Miga

[jrusso1020]

Posting before any other reviewer arrives. Verified each security claim against the diff; two scope-clarity flags surfaced beyond the body.

✅ Security fixes — verified semantically equivalent

CWE-78 (command injection) — media-use/scripts/resolve.test.mjs

Old shape: execSync(node skills/media-use/scripts/resolve.mjs ${args}, …) where args was a template-interpolated string containing untrusted-looking values like ${tmp} (a temp-dir name).

New shape: execFileSync(process.execPath, [RESOLVE_CLI, ...args], …) via a runResolve() wrapper — argv array, no shell. Each token is a separate argv entry; ${tmp} (or any value containing spaces / shell metacharacters) can no longer break out into shell parsing.

Verified all 8 call sites converted (grep -c "execSync\|runResolve" of the changed file). Test semantics preserved: each call's argument list is the deterministic argv expansion of the prior interpolated string. The --help, missing required args, and stub fail cases still exercise the same exit-code paths. Solid CWE-78 fix.

CWE-79 (XSS via innerHTML) — intro-kinetic-cascade/index.html + logo-split-lockup-pulse/index.html

Old shape (4 sinks across the two templates): el.innerHTML = <var> where <var> is library SVG markup or template-config-provided custom inline SVG.

New shape: setSvg(el, markup) helper that:

1. textContent = "" (clear)
2. new DOMParser().parseFromString(String(markup), "image/svg+xml").documentElement
3. If root.nodeName.toLowerCase() === "svg" → el.appendChild(document.importNode(root, true)) — preserves the SVG verbatim as a parsed DOM tree
4. Otherwise → el.textContent = String(markup) — safe fallback for non-SVG content

This is the right shape: DOMParser with image/svg+xml doesn't execute scripts on parse, and the nodeName === 'svg' gate prevents an attacker from sneaking a non-SVG root past the check. The document.importNode(..., true) deep-clone preserves all SVG attributes and nested elements identically to what innerHTML would have rendered for trusted input. Rendering parity verified for both library icons (ICONS[iconSpec]) and custom-inline-SVG branches.

Identical setSvg() helper duplicated across both templates — minor code duplication, but each template ships as a standalone artifact so the duplication is intentional / unavoidable.

✅ Supply-chain hardening — verified defensive, no success-path change

package-loader.mjs (+10 in each of hyperframes-animation + hyperframes-creative)

Pure header-comment additions (zero code change). Documents the existing defense-in-depth:

• Specs version-pinned via assertPinnedPackageSpecs (no floating latest)
• npm install --ignore-scripts (package lifecycle scripts skipped)
• --no-save into a throwaway tmp dir (host project untouched)
• Interactive y/N prompt or explicit $HYPERFRAMES_SKILL_BOOTSTRAP_DEPS=1 opt-in
• npm spawned with argv array (no shell)
• installLine strings are display-only, never executed

Surfacing this in code comments is good archaeological practice — makes the hardening claims visible to anyone reading the loader without grepping for them. ✓

fetch-people-avatars.mjs (+39, -1) — new SSRF + path-traversal guards

Two new defensive functions added BEFORE the existing fetch logic:

1. isAllowedAvatarUrl(u): HTTPS-only check + host allowlist (avatars.githubusercontent.com, github.com, www.github.com, any *.githubusercontent.com). Rejects non-HTTPS protocols. Crafted people.json carrying http://internal:8080/x or file:///etc/x now soft-fails with "not a GitHub avatar URL" rather than letting fetch reach internal hosts.
2. isUnderProject(p): resolve(p) === projectDir || resolve(p).startsWith(projectDir + sep). Rejects any avatar path that would write outside the project dir (e.g. ../../etc/...). Crafted avatarFile: "../../../tmp/x.png" soft-fails with "avatar path escapes the project dir."

Both gated checks happen BEFORE the network fetch + before the file write. On rejection: mark avatarFetched=false, log a single-line note, return "fail" (the existing soft-fail path — softExit ensures the overall script always exits 0). Existing happy path (https://avatars.githubusercontent.com/u/123?v=4 → fetch → write to assets/<login>.png) is untouched. ✓

Other hardening — verified

• transcribe.cjs: whisperx==3.8.6 pin (with $WHISPERX_VERSION env override) instead of --from whisperx. Determinism + supply-chain — reproducible build vs floating-latest. ✓
• GSAP SRI (make-theme.cjs + 5× assemble-index.mjs / captions.mjs touches): added integrity="sha384-sG0Hv1tP1lZCk9KQmrIbY/XNwi+OY84GQqhMscbnsoBFqAz8KNCil1kvfL3Hbbk2" + crossorigin="anonymous" to gsap@3.14.2/dist/gsap.min.js script tags. Identical hash across all 6 touch points (correct — they all reference the same URL). Defends against CDN compromise. ✓

⚠️ Two body-vs-diff scope flags (non-blocking)

The body's framing — "Snyk fixes + network + supply-chain hardening" + "no change to any skill's behaviour" — is accurate for the security work, but two diffs are outside that scope and unmentioned:

1. skills/hyperframes/SKILL.md +17/-17 — routing-rule prose rewrite

The workflow cheat-sheet rows for /product-launch-video and /website-to-video get a substantive prose rewrite ("Marketing / launching / promoting a product" → "Selling a product (SaaS, app, company / product site)", "Turning a general website into a video" → "Showing a site itself — a tour / showcase built from the site's own screenshots"). The disambiguation paragraph is rewritten with a new "ask one thing: is the site selling a product?" framing.

This is a real routing-behavior change for the orchestrator skill (/hyperframes), not security hardening. It belongs to a routing-clarity follow-up, not the Snyk-clear PR. Worth either:

• Splitting into a separate PR, or
• Adding a row to the body's "Findings addressed" table calling out the routing rule refinement

The change itself reads well; the issue is body-vs-diff scope per REVIEW_DISCIPLINE rule 3.

2. skills-manifest.json — two unexpected hash bumps

The body says the manifest is "auto-regenerated." Cross-checked which entries got new hashes vs which skills had files in the diff:

Skill	Files in diff?	Hash bumped?
embedded-captions	✓ (transcribe, make-theme)	✓
faceless-explainer	✓	✓
hyperframes	✓ (SKILL.md)	✓
hyperframes-animation	✓	✓
hyperframes-core	no files	✓ unexpected
hyperframes-creative	✓	✓
media-use	✓	✓
music-to-video	✓	✓
pr-to-video	✓	✓
product-launch-video	✓	✓
slideshow	no files	✓ unexpected

hyperframes-core and slideshow get new hashes despite no files in the diff. Possible explanations: (a) the manifest generator hashes some shared/cross-skill resource that changed; (b) stale-master regen drift carried in via rebase; (c) generator nondeterminism. Worth a one-line answer in the body so future archaeology + Snyk auditors don't waste time chasing it. Not blocking.

CI

32/32 deduped green (39 raw — 7 superseded filtered per the rollup-dedup gotcha). BLOCKED is the awaiting-review gate.

Verdict

COMMENT only — external author, routing through @U08E7PV788Z (James) for any stamp/merge call per protocol. All security claims verified semantically equivalent + defensive guards genuinely defensive. Two body-vs-diff scope flags (routing rewrite + unexpected manifest bumps) worth a body update but not blocking.

— Jerrai (pr-review)

[miguel-heygen]

Blocking issue: the innerHTML replacement does not actually neutralize active SVG payloads.

The new setSvg() helper parses variable-provided markup with DOMParser(..., "image/svg+xml") and appends document.importNode(root, true) when the root is <svg> (intro-kinetic-cascade/index.html:241-248, logo-split-lockup-pulse/index.html:366-373). That removes the innerHTML sink, but it is not sanitization. In Chrome, an imported SVG appended this way still executes active content.

I verified with a local headless Chrome probe using the same helper shape: this payload resulted in data-hit="svg-script,image-onerror,svg-onload" on <body> after append:

<svg xmlns="http://www.w3.org/2000/svg" onload="window.hit('svg-onload')">
  <script>window.hit('svg-script')</script>
  <image href="x" onerror="window.hit('image-onerror')"></image>
</svg>

This matters because these paths still accept SVG from variables (vars.leftMark, vars.rightMark, and vars.icon custom SVG), not only from the hardcoded built-ins. So the PR currently changes the shape of the Snyk sink but does not clear the CWE-79 class.

Suggested fixes:

• safest/minimal: only allow the built-in SVG strings/default constants; treat variable-provided custom SVG as text unless explicitly trusted, or
• sanitize the parsed SVG before append with a strict allowlist: drop <script>, <foreignObject>, animation/link/scriptable elements, all on* attributes, and unsafe href/xlink:href/URL-bearing values (javascript:, remote/data URLs unless intentionally allowed). Add a regression test/probe for <script>, onload, and onerror so the security claim is pinned.

Other checks: the GSAP SRI hash matches the CDN bytes, the execFileSync(argv) change is the right CWE-78 fix, and the avatar path/host guards are directionally sound. Rames' manifest/routing notes are worth documenting, but this SVG execution issue is the blocker.

Verdict: REQUEST_CHANGES
— Magi

[WaterrrForever]

Thanks all — pushed 67d95f71 addressing the review.

Re: Magi (requested changes) — blocker fixed, you were right

The prior setSvg() only moved the sink (innerHTML → DOMParser + importNode); it did not sanitize, so active SVG content still executed on insertion. I reproduced your finding and then verified the fix in headless Chrome (same helper, both templates):

	old setSvg (importNode-only)	new setSvg (sanitizing)
fired handlers	["script","onload"]	[]
trusted icon renders	—	✅ svg>path present
danger nodes in output	—	0
on* attrs in output	—	0

setSvg() now runs a default-deny cleanSvg() over the parsed tree before it touches the document: only an allow-list of inert drawing elements (svg/g/path/line/rect/circle/ellipse/defs/linearGradient/stop/text/…) and presentation attributes (d/fill/stroke/viewBox/transform/…) survives. Everything else — <script>, <image>, <use>, <foreignObject>, <a>, <animate>, every on* handler, and href/xlink:href/style — is stripped, including on the root node. Non-SVG / malformed input falls back to textContent. Trusted content (the bundled icon library + default spark/cloud marks) renders byte-identically.

Re: Rames — both notes addressed

1. Routing rewrite — agreed it's not a Snyk fix. Documented explicitly in the PR description under "Also in this PR (non-security)"; it's an isolated commit (docs(skills): clarify product-launch-video vs website-to-video routing).
2. Manifest hash bumps — ran it down: against current main, the manifest bumps only the 9 skills actually modified here; hyperframes-core / slideshow are not among them. What you saw was main's own fix: storyboard-angle review follow-ups (M1 bg-on-clip, B3 slideshow, parser guard, CLI fixes) #1791 (it edited slideshow/SKILL.md) surfaced by merging main into this branch against the pre-fix: storyboard-angle review follow-ups (M1 bg-on-clip, B3 slideshow, parser guard, CLI fixes) #1791 base — not changes from this PR. Generator is deterministic (double-run = identical output). No archaeology needed.

Re: Miga — thanks for the pass.

CI is green again after the re-fix (Format, CodeQL, Skills manifest/tests). Ready for re-review whenever you have a moment.

[jrusso1020]

R2 verification at 67d95f71 — Miao went deeper than my R1's praise of setSvg() and shipped a proper allowlist sanitizer. Concurring with the "real CWE-79 fix" framing in the commit message — my R1 was too lenient.

Where R1 fell short — for the record

My R1 said the original setSvg(DOMParser parse + importNode) was "the right shape" because DOMParser doesn't execute scripts during parse. That's true but insufficient — importNode into the live document then would fire <script> elements, <image href="javascript:…">, on* event handlers, and SMIL <animate> indirect-script paths. Custom user-supplied SVG could smuggle active content past the original setSvg. Real CWE-79 mitigation needs both parse-safety and tree-sanitization.

R2 fix shape — verified proper sanitization

The new setSvg() adds two safeguards beyond R1:

if (!root || root.nodeName.toLowerCase() !== "svg" || doc.getElementsByTagName("parsererror").length) {
  el.textContent = String(markup); return;
}
cleanSvg(root);
el.appendChild(document.importNode(root, true));

1. Parsererror check — catches malformed-XML input that DOMParser silently turns into a <parsererror> document. Without this check the original setSvg would have called nodeName.toLowerCase() !== "svg" correctly but a malformed input that LOOKS like SVG enough to parse-to-svg-root could still slip through.

2. cleanSvg(root) recursive allowlist scrub — runs BEFORE importNode. Strict allowlists:

   • SVG_OK_TAGS: only inert drawing elements (svg/g/path/line/polyline/polygon/rect/circle/ellipse/defs/lineargradient/radialgradient/stop/clippath/title/desc/text/tspan). Notably ABSENT: <script>, <foreignObject>, <image>, <use>, <iframe>, <a>, <animate>, <animateTransform>. Each of those is a known SVG-XSS vector — <foreignObject> can embed HTML, <use href="data:image/svg+xml…"> can pull cross-document content, <animate> SMIL can trigger script-style behavior.
   • SVG_OK_ATTRS: only presentation attributes (viewbox/xmlns/width/height/fill/stroke/d/x/y/cx/cy/r/transform/etc.). Notably ABSENT: all on* handlers, href/xlink:href (no javascript: URL sneaks), style (no CSS-expression smuggling).
   • Recurses depth-first; removes any element not in allowlist, removes any attribute not in allowlist. Text nodes preserved.

Identical sanitizer duplicated across both templates (intro-kinetic-cascade + logo-split-lockup-pulse) — same minor template-duplication note from R1 still applies but each template ships standalone so it's intentional.

Behavior preservation — verified for legitimate input

For the documented use cases (library icons + custom-inline-SVG configs):

• Library icons (ICONS[iconSpec]) — produce simple <svg> markup with <path d="..." fill="..."/> shapes. Every tag + attribute in those library icons IS in the allowlist (svg, path, fill, d, viewbox, width, height, stroke*, transform). Rendering preserved.
• Custom inline SVG (iconSpec.startsWith("<svg")) — for template-config-provided SVGs containing only drawing markup (no script, no foreignObject), all elements/attributes survive the scrub. Active-content elements/attributes get scrubbed (the whole point).
• Non-SVG / malformed input — falls to el.textContent = String(markup). Safe text fallback.

document.importNode(root, true) deep-clone after sanitization preserves the cleaned tree exactly as it'll render.

What remains from my R1

Both my R1 body-vs-diff flags are still in the diff (this R2 commit only touched the SVG sanitizer):

1. skills/hyperframes/SKILL.md +17/-17 routing-rule rewrite — still in the PR, still not mentioned in the body's "Findings addressed" table. Real orchestrator behavior change, not security.
2. Unexpected manifest hashes for hyperframes-core + slideshow — still in skills-manifest.json despite no files in either skill changing. R2 only adjusted music-to-video (which DID change). The two anomalous bumps from R1 persist; worth a one-line explanation in the body.

Both remain non-blocking, just clarity for archaeology.

CI: 31/31 deduped green (59 raw — 28 superseded filtered).

Verdict

R2 cleanly resolves the genuine CWE-79 gap I missed in R1. Concurring with the commit message's "real CWE-79 fix" framing. Still COMMENT (external author, stamp routing through @U08E7PV788Z).

— Jerrai (pr-review)

[miga-heygen]

Re-review at 67d95f7 (was 57cf9c15)

New commit since last review

67d95f7 — fix(skills): sanitize SVG in music-to-video templates (real CWE-79 fix)

This directly addresses the blocking issue from Miguel's review: the previous setSvg() swapped the sink (innerHTML to DOMParser + importNode) but never sanitized the parsed tree, leaving DOM-XSS fully exploitable via crafted SVGs in template variables (vars.icon, leftMark, rightMark).

What changed

Both music-to-video templates (intro-kinetic-cascade and logo-split-lockup-pulse) now run a cleanSvg() sanitizer before any parsed SVG enters the live document. The approach is default-deny with explicit allowlists:

• SVG_OK_TAGS — 18 inert drawing elements (svg, g, path, rect, circle, defs, lineargradient, stop, clippath, text, tspan, etc.). Everything else (<script>, <image>, <use>, <foreignObject>, <a>, <animate>, <set>) is removed.
• SVG_OK_ATTRS — 35 presentation/geometry attributes (d, fill, stroke, viewBox, transform, class, id, etc.). All on* handlers, href, xlink:href, and style are stripped.
• Parse-error guard — doc.getElementsByTagName("parsererror").length check catches malformed SVG and falls back to textContent.
• Non-element, non-text nodes (comments, processing instructions) are also removed.

Verification

The commit message confirms headless Chrome testing:

• Old setSvg fired both onload and <script> payloads
• New setSvg fires neither, while trusted bundled icons render identically

Assessment

The allowlist is well-scoped — covers the full set of drawing primitives the templates actually use (paths, circles, rects, gradients, clip-paths, text) without opening any active-content vectors. The transform attribute is the only one that could theoretically be a concern, but SVG transform is purely geometric (translate/rotate/scale/skew/matrix) — no execution surface there.

One minor observation (non-blocking): the cleanSvg + allowlist definitions are duplicated across both templates. If a third music-to-video template lands, worth extracting to a shared snippet. Not a blocker — these are standalone HTML compositions and the duplication is self-contained.

CI

All required checks pass at this SHA. skills-manifest.json hash bump for music-to-video is consistent.

Verdict: LGTM — the CWE-79 finding is properly resolved. The blocking review feedback has been addressed.

— Miga

[miguel-heygen]

Re-reviewed at 67d95f71; R2 resolves my blocker.

The previous DOMParser + importNode change was parse-safe but not insertion-safe. The new helper now sanitizes before import with a default-deny allowlist: active elements (<script>, <image>, <use>, <foreignObject>, <a>, <animate>), all on* handlers, href / xlink:href, and style are stripped before nodes enter the live document, while inert drawing tags and presentation geometry survive.

I re-ran the headless-Chrome proof against the R2 sanitizer shape: a valid active SVG payload produced data-danger-nodes=0, data-on-attrs=0, data-href-attrs=0, data-style-attrs=0, no execution marker, and preserved the benign <rect>. A trusted gradient/path SVG also still rendered (path=1, stop=1).

The other hardening remains sound: execFileSync(argv) fixes the shell-injection shape, the avatar URL/path guards are defensive, and the GSAP SRI additions are consistent. Rames' routing/body and manifest archaeology notes are now documented/non-blocking.

Approved.
— Magi

[miga-heygen]

Re-review: fix(skills): clear two Snyk Fails and harden the network + supply-chain surface

What changed: Only a merge-forward of main into the branch (8985b3e), with conflicts limited to skills-manifest.json hash entries (auto-generated). No new feature commits, no code changes beyond merge resolution.

Previous concerns — all resolved:

• SVG sanitizer blocker (Magi's CHANGES_REQUESTED) → fixed in 67d95f7 with cleanSvg() default-deny allowlists
• Routing-rule prose rewrite scope → documented in PR body
• Manifest hash bumps → explained as surfaced from main's #1791, verified by double-run

SVG sanitizer re-verified (unchanged since approval):

• 18 inert drawing tags allowlisted; all active-content vectors stripped
• 35 presentation/geometry attributes allowlisted; all on* handlers, href/xlink:href, style stripped
• Root node attributes also scrubbed
• Non-SVG/malformed input falls back to safe textContent

CI: All relevant checks pass (format, file size, CodeQL, skills manifest, skills tests, semantic PR title).

Verdict: Still LGTM — Same state as Magi's approval + clean merge-forward. Ready to merge.

— Miga

[miguel-heygen]

Re-reviewed current head 8985b3e9. The new head is a merge-forward only; the SVG sanitizer fix from 67d95f71 is unchanged, the routing-scope and manifest-hash notes are documented, and required checks are green. I also removed the generated-by footer from the PR body before stamping.

Verdict: APPROVE
Reasoning: Prior blocker is resolved, current delta is metadata/merge-forward only, and CI is clean.