Skip to content

feat: isolate run button#1952

Open
dsanders11 wants to merge 1 commit into
mainfrom
refactor/isolate-zee-run-button-now
Open

feat: isolate run button#1952
dsanders11 wants to merge 1 commit into
mainfrom
refactor/isolate-zee-run-button-now

Conversation

@dsanders11
Copy link
Copy Markdown
Member

Isolates the run button to a cross-origin iframe to ensure it's rendered as an out-of-process iframe (OOPIF), and is isolated in a separate process to protect against a compromised renderer. This prevents the renderer from being able to programmatically run fiddles, and only the main process or user interaction can start them.

Assisted-by: Claude Opus 4.7
@dsanders11 dsanders11 requested a review from ckerr May 22, 2026 05:23
@dsanders11 dsanders11 requested review from a team and codebytere as code owners May 22, 2026 05:23
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 87.45% (-0.9%) from 88.301% — refactor/isolate-zee-run-button-now into main

Comment thread src/preload/preload.ts
Comment on lines +85 to +89
if (location.protocol === ISOLATED_ACTIONS_PROTOCOL) {
setupIsolatedActionsGlobal();
} else {
await setupFiddleGlobal();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an isMainFrame check here, exposing only isolated if we're not the main frame

Suggested change
if (location.protocol === ISOLATED_ACTIONS_PROTOCOL) {
setupIsolatedActionsGlobal();
} else {
await setupFiddleGlobal();
}
if (isMainAppFrame()) {
await setupFiddleGlobal();
} else {
setupIsolatedActionsGlobal();
}

Comment on lines +88 to +92
try {
return new URL(frame.url).protocol === `${ISOLATED_ACTIONS_SCHEME}:`;
} catch {
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to gate on more than just the protocol? For example we could check the host and pathname too. That would limit the "run fiddle" permissions a little more tightly.

Not sure if I'm overthinking this, open to push back on this. If you're onboard, other related changes inline with (limit-run-permissions-to-run-button) notation.

Suggested change
try {
return new URL(frame.url).protocol === `${ISOLATED_ACTIONS_SCHEME}:`;
} catch {
return false;
}
return isIsolatedRunButtonDocumentUrl(frame.url);

with

const RUN_BUTTON_ENTRY_PATH_PREFIX = `/${RUN_BUTTON_ENTRY_NAME}/`;

export function isIsolatedRunButtonDocumentUrl(rawUrl: string): boolean {
  try {
    const url = new URL(rawUrl);
    return (
      url.protocol === `${ISOLATED_ACTIONS_SCHEME}:` &&
      url.host === ISOLATED_ACTIONS_RUN_BUTTON_HOST &&
      url.pathname === '/'
    );
  } catch {
    return false;
  }
}

Comment on lines +55 to +59
// Refuse path traversal — anything with `..` could escape the
// upstream output directory once we resolve it.
if (pathname.includes('..')) {
return new Response(null, { status: 400 });
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't safeguard against encoded dots. Not sure what the risk level is here, but there are safer ways to test for this, eg decodeURIComponent + path.posix.normalize(decoded) + normalized.startsWith(entryDirSuffix)

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.

3 participants