Skip to content

fix: harden frontend overlay's cssUrl injection and preview effect (PL-029/030/031) - #138

Merged
jackgranatowski merged 4 commits into
claude/pr-469-audit-rebase-ggp0e4from
claude/plugins-pr-c3-frontend-hardening
Jul 2, 2026
Merged

jackgranatowski merged 4 commits into
claude/pr-469-audit-rebase-ggp0e4from
claude/plugins-pr-c3-frontend-hardening

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Summary

Third PR from SLASHED-Plugins' technical-debt audit remediation (Wave 0, plugin-specific frontend hardening — AppOverlay.svelte/plugin-main.ts only, neither vendored).

  • PL-029 (same-origin check on cssUrl injection): plugin-main.ts set link.href = cssUrl from window.slashedApp?.cssUrl with no validation. cssUrl is normally trustworthy — server-generated via esc_url_raw() over the plugin's own asset path (class-frontend-configurator.php) — but window.slashedApp is a plain global any other script on the page (a theme, another plugin) can clobber before this module runs. Added a same-origin check (new URL(url, location.href).origin === location.origin) before using it as a stylesheet href; on failure it falls through to the existing unstyled-mount fallback rather than blocking.
  • PL-030 (typed window.slashedApp access): replaced the (window as any).slashedApp?.cssUrl as string | undefined cast with a typed local accessor. Initially tried adding a plugin-owned ambient .d.ts augmenting the global Window interface, but that conflicts with the vendored vite-env.d.ts's own Window.slashedApp declaration (TS interface-merging two incompatible shapes for the same global property broke svelte-check). Instead mirrored the exact local-intersection-type pattern the vendored persistence.ts already uses for its own window.slashedApp read (window as Window & { slashedApp?: T }) — no global declaration, no merge conflict.
  • PL-031 (debounce AppOverlay.svelte's live-preview effect): the injectLivePreview/registerPreviewDoc effect re-ran on every override change with no coalescing, unlike the framework's own PreviewPanel.svelte (SL-020, already fixed upstream). A fast-changing control (dragging a slider) could re-run it many times per animation frame even though the <style> rewrite + derived-token recompute only need to happen once per paint. Wrapped it in the same requestAnimationFrame + cancelAnimationFrame cleanup pattern as SL-020.

Verification

  • npm run check (svelte-check): 0 new errors (the one remaining error, plugin-main.ts's .ts import extension, is pre-existing and unrelated).
  • npm run build: succeeds, 216 modules.
  • npm test: 67/67 passing.
  • Manually verified isSameOrigin()'s logic (relative paths, same-origin absolute URLs, cross-origin, protocol-relative, and javascript: URLs) via a standalone script — correctly allows same-origin/relative, rejects everything else.

Type

  • fix

Checklist

  • Conventional Commit messages
  • npm test passes
  • npm run lint passes
  • npm run verify passes
  • Generated artifacts not hand-edited
  • CHANGELOG.md updated — not user-facing (internal hardening, no behavior change for the trusted/normal path)
  • Built SPA assets committed — same reasoning as PR fix: unbreak admin-app build after SLASHED codec/lucide renames #135: the committed vendored src//assets/ predate the framework audit sync (separate PR-SYNC job), and CI always rebuilds from a fresh sync regardless of what's committed here

Notes

Depends on #135 (codec/lucide rename hotfix) being present on this branch's base to build — already merged into this branch directly since #135 hadn't landed on the integration branch yet.


Generated by Claude Code

claude added 3 commits July 2, 2026 13:54
…names

SLASHED's SL-016 (codec.ts's `fa` → `generateCSS`) and SL-022
(lucide-svelte → @lucide/svelte) landed in main via #474. AppOverlay.svelte
is plugin-specific and not vendored, so it kept referencing the old names
and broke `vite build` as soon as CI's prebuild sync pulled the renamed
exports from slashed@main — failing on every open plugin PR, not just the
one that triggered it.

Swaps the import/call site to generateCSS and updates package.json/
package-lock.json to depend on @lucide/svelte instead of the deprecated
lucide-svelte package.
npm run check (svelte-check) had no equivalent of dev/build's
predev/prebuild sync hook, so it type-checked whatever was already on
disk. On a fresh checkout with the committed vendored src/ still
predating the lucide-svelte rename, that meant a stale module-resolution
error instead of an accurate check against the current framework source.
…review effect

PL-029/030: window.slashedApp is a plain global any script on the page
can clobber before plugin-main.ts runs. Add a same-origin check before
using cssUrl as a stylesheet href, and replace the (window as any) cast
with a typed local accessor (mirroring persistence.ts's own
window.slashedApp read) instead of widening to any.

PL-031: AppOverlay.svelte's live-preview effect (injectLivePreview +
registerPreviewDoc) re-ran on every override change with no coalescing,
unlike PreviewPanel.svelte's SL-020 fix. A fast-changing control
(dragging a slider) could re-run it many times per frame even though
the underlying <style> rewrite only needs to happen once per paint.
Wrapped it in the same rAF-coalescing pattern.
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3582722a-bc19-4e11-a366-06a7e8d895b3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/plugins-pr-c3-frontend-hardening

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Harden frontend overlay cssUrl injection and debounce live preview

🐞 Bug fix ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Validate injected overlay stylesheet URL is same-origin before setting .
• Debounce overlay live-preview stylesheet rewrites using requestAnimationFrame coalescing.
• Sync vendored core before svelte-check; update lucide dependency to @lucide/svelte.
Diagram

graph TD
  A{{"Host page"}} --> B{{"window.slashedApp"}} --> C["plugin-main.ts"] --> D{"Same-origin cssUrl?"}
  D -->|"yes"| E["Shadow root + CSS <link>"] --> F["AppOverlay mount"] --> G["rAF live preview"]
  D -->|"no"| F
  subgraph Legend
    direction LR
    _ext{{"External/global"}} ~~~ _proc["Code/module"] ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Pass cssUrl via data-attribute on mount element
  • ➕ Avoids relying on a clobberable global (window.slashedApp)
  • ➕ Keeps typing local to the overlay DOM boundary
  • ➕ Still allows same-origin validation if desired
  • ➖ Requires coordinated PHP/template change to set the attribute
  • ➖ Harder to share other boot fields that already live on window.slashedApp
2. Inline critical overlay CSS (no external href)
  • ➕ Eliminates URL injection surface entirely
  • ➕ Avoids flash-of-unstyled-content without async stylesheet load
  • ➖ Increases HTML payload and complicates cache-busting/versioning
  • ➖ More complex PHP rendering; harder to keep in sync with built assets
3. Stricter allowlist beyond origin (path prefix / known filename)
  • ➕ Reduces risk even under same-origin but attacker-controlled paths
  • ➕ More explicit contract for what may be loaded
  • ➖ More brittle across build pipeline changes (hashed filenames, subpaths)
  • ➖ Can block legitimate deployments with different asset paths

Recommendation: The PR’s approach is a good scope-fit: same-origin gating meaningfully mitigates the realistic threat (global clobbering) without requiring PHP/template changes, and it preserves the existing fallback behavior. If further hardening is needed later, the most robust next step is moving cssUrl transport off the global (e.g., a data-attribute on #slashed-frontend-overlay), but that’s a broader integration change than this remediation wave.

Files changed (3) +49 / -12

Bug fix (2) +47 / -11
AppOverlay.svelteDebounce live-preview injection and align imports with upstream renames +16/-9

Debounce live-preview injection and align imports with upstream renames

• Updates codec usage from fa() to generateCSS() and switches icon imports to @lucide/svelte. Wraps the live-preview injection/preview-doc registration effect in requestAnimationFrame with cancellation to coalesce rapid override changes into one per paint.

SLASHED-for-WP/admin-app/src/AppOverlay.svelte

plugin-main.tsType and validate cssUrl before stylesheet injection +31/-2

Type and validate cssUrl before stylesheet injection

• Introduces a local typed accessor for window.slashedApp.cssUrl without augmenting the global Window interface. Adds a same-origin check before using cssUrl as a stylesheet href, falling back to the existing unstyled mount path when invalid.

SLASHED-for-WP/admin-app/src/plugin-main.ts

Other (1) +2 / -1
package.jsonSync before check; migrate lucide dependency to @lucide/svelte +2/-1

Sync before check; migrate lucide dependency to @lucide/svelte

• Adds a precheck hook to run the core sync step before svelte-check so typechecking uses current vendored sources. Replaces the deprecated lucide-svelte dependency with @lucide/svelte.

SLASHED-for-WP/admin-app/package.json

@qodo-code-review

qodo-code-review Bot commented Jul 2, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 1 rule

Grey Divider


Action required

1. Lucide imports mismatch ✗ Dismissed 🐞 Bug ≡ Correctness
Description
The PR removes the lucide-svelte dependency and adds @lucide/svelte, but multiple modules still
import from 'lucide-svelte', which will fail module resolution on a clean install/build.
Code

SLASHED-for-WP/admin-app/package.json[R16-20]

  "dependencies": {
+    "@lucide/svelte": "^1.23.0",
    "fflate": "^0.8.3",
-    "lucide-svelte": "^1.0.1",
    "motion": "^12.23.24"
  },
Relevance

⭐⭐⭐ High

Team fixes clean-build breakages; lucide-svelte dependency changes were actively maintained (PR
#118).

PR-#118
PR-#126

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
package.json now installs @lucide/svelte, but existing code still imports 'lucide-svelte' (e.g.
App.svelte, PreviewPanel.svelte). Since vite.config.js defines no alias for lucide-svelte, these
imports will not resolve in a clean environment.

SLASHED-for-WP/admin-app/package.json[16-20]
SLASHED-for-WP/admin-app/src/App.svelte[1-4]
SLASHED-for-WP/admin-app/src/components/shell/PreviewPanel.svelte[1-7]
SLASHED-for-WP/admin-app/vite.config.js[15-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`package.json` now depends on `@lucide/svelte` instead of `lucide-svelte`, but the codebase still contains imports from `'lucide-svelte'`. With a fresh `npm ci`/`npm install`, those imports will not resolve and the build/check will fail.

## Issue Context
Vite config currently has no resolve.alias mapping from `lucide-svelte` to `@lucide/svelte`, so the import specifier must match the installed package name.

## Fix
Pick one:
1) Update *all* source imports from `lucide-svelte` to `@lucide/svelte`.
2) Or revert the dependency change (keep `lucide-svelte`).
3) Or add a Vite alias (and ensure TS tooling also resolves it), e.g. `resolve.alias['lucide-svelte'] = '@lucide/svelte'`.

## Fix Focus Areas
- SLASHED-for-WP/admin-app/package.json[16-20]
- SLASHED-for-WP/admin-app/src/App.svelte[1-5]
- SLASHED-for-WP/admin-app/src/components/shell/PreviewPanel.svelte[1-6]
- SLASHED-for-WP/admin-app/vite.config.js[15-26]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Effect loses overrides tracking ✓ Resolved 🐞 Bug ≡ Correctness
Description
AppOverlay.svelte wraps injectLivePreview(overrides) in requestAnimationFrame without any
synchronous read of overrides in the $effect body, so the effect may not rerun when overrides
change and the live preview can become stale after initial mount.
Code

SLASHED-for-WP/admin-app/src/AppOverlay.svelte[R110-120]

  $effect(() => {
-    injectLivePreview(overrides);
-    // registerPreviewDoc() bumps previewVersion itself (on both the
-    // first-registration and already-registered paths), so panels reading
-    // previewVersion.value re-resolve on every override change without a
-    // separate explicit bump here.
-    registerPreviewDoc(document);
+    const rafId = requestAnimationFrame(() => {
+      injectLivePreview(overrides);
+      // registerPreviewDoc() bumps previewVersion itself (on both the
+      // first-registration and already-registered paths), so panels reading
+      // previewVersion.value re-resolve on every override change without a
+      // separate explicit bump here.
+      registerPreviewDoc(document);
+    });
+    return () => cancelAnimationFrame(rafId);
  });
Relevance

⭐⭐ Medium

No history on $effect+rAF dependency tracking; overlay correctness changes often accepted (PR
#90/#91).

PR-#90
PR-#91

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new overlay effect no longer reads overrides in the synchronous $effect body (only inside
the scheduled callback), while the non-overlay app keeps reactivity by calling
injectLivePreview(overrides) synchronously inside its $effect. This indicates the overlay
version may stop reacting to overrides updates.

SLASHED-for-WP/admin-app/src/AppOverlay.svelte[98-120]
SLASHED-for-WP/admin-app/src/App.svelte[80-88]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`$effect` dependency tracking occurs on synchronous reactive reads during the effect body. In `AppOverlay.svelte`, `overrides` is only read inside the `requestAnimationFrame` callback, so the effect may not subscribe to `overrides` updates and will not re-run when overrides change.

## Issue Context
This effect is responsible for (a) applying live CSS overrides via `injectLivePreview()` and (b) making previewResolver resolve computed colors by calling `registerPreviewDoc(document)`.

## Fix
Read `overrides` synchronously inside the `$effect` and pass that snapshot into the rAF callback.

Example pattern:
```ts
$effect(() => {
 const ov = overrides; // establishes dependency
 const rafId = requestAnimationFrame(() => {
   injectLivePreview(ov);
   registerPreviewDoc(document);
 });
 return () => cancelAnimationFrame(rafId);
});
```

## Fix Focus Areas
- SLASHED-for-WP/admin-app/src/AppOverlay.svelte[110-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread SLASHED-for-WP/admin-app/src/AppOverlay.svelte
Comment thread SLASHED-for-WP/admin-app/package.json
…iew effect

Svelte 5's \$effect only tracks reactive reads that happen during its own
synchronous execution. The rAF-coalescing added for PL-031 read overrides
only inside the (later-firing) requestAnimationFrame callback, so the
effect would never re-run after the first paint -- the live preview
would silently stop reacting to override changes.

PreviewPanel.svelte's SL-020 fix (the pattern this was meant to mirror)
already captures its reactive dependencies synchronously before entering
the rAF callback for exactly this reason -- this file just missed it.
@jackgranatowski
jackgranatowski merged commit 5fe0c8a into claude/pr-469-audit-rebase-ggp0e4 Jul 2, 2026
9 checks passed
jackgranatowski pushed a commit that referenced this pull request Jul 2, 2026
Brings in the already-merged #134/#135/#136/#138 content; the only
real conflict was two independent additions to CLAUDE.md's Key
scripts section (npm run check row from #136, the playwright-admin.js
manual-only note from #139) -- kept both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants