From 98633d02b751f2676edf61eec904936186b5e859 Mon Sep 17 00:00:00 2001
From: DaColdest <149835959+dacoldest@users.noreply.github.com>
Date: Wed, 19 Aug 2026 16:51:10 -0400
Subject: [PATCH] harden: drop frame popup/download rights, validate external
 hrefs

The review iframe renders artifacts that are untrusted by definition, but
its sandbox granted allow-popups and allow-downloads. Neither is needed:
the SDK posts external links to the chrome over eh:external and the chrome
opens them, and nothing downloads. Removing both narrows what a malicious
artifact can do to the reviewer.

The eh:external handler also called window.open on an unvalidated href.
The origin check is the only thing in front of it, so a compromised frame
or a misconfigured policy could land a javascript: URL there, executing on
the chrome's origin alongside the session token. Route it through the
existing normalizeHref, which already allowlists http/https/mailto/tel and
is tested against javascript:, data:, vbscript: and tab-obfuscated
variants, rather than adding a second URL-safety helper to diverge from.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 src/chrome-client.js      | 10 ++++++++--
 src/chrome.html           |  2 +-
 src/frame-policy.js       |  2 +-
 test/frame-policy.test.js | 11 +++++++++++
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/chrome-client.js b/src/chrome-client.js
index 4143b8f..955508b 100644
--- a/src/chrome-client.js
+++ b/src/chrome-client.js
@@ -6,6 +6,7 @@
  */
 import { tidy } from "./anchor-text.js";
 import { pageUrl, replacePage } from "./chrome-session.js";
+import { normalizeHref } from "./editing.js";
 import { framePolicy } from "./frame-policy.js";
 
 const $ = (id) => document.getElementById(id);
@@ -607,9 +608,14 @@ window.addEventListener("message", async (event) => {
     case "eh:scroll":
       state.scroll = { x: msg.x, y: msg.y };
       break;
-    case "eh:external":
-      window.open(msg.href, "_blank", "noopener");
+    case "eh:external": {
+      // This side is what actually calls window.open, so it re-checks the
+      // scheme rather than trusting the frame: a javascript: or data: URL
+      // arriving here would run on this origin, next to the token.
+      const external = normalizeHref(msg.href);
+      if (external) window.open(external, "_blank", "noopener");
       break;
+    }
     case "eh:navigate":
       try {
         const result = await api(`/api/session/${state.sessionId}/navigate`, {
diff --git a/src/chrome.html b/src/chrome.html
index f651a70..3bcdfe9 100644
--- a/src/chrome.html
+++ b/src/chrome.html
@@ -12,7 +12,7 @@
       <iframe
         id="frame"
         title="Artifact under review"
-        sandbox="allow-scripts allow-forms allow-modals allow-popups allow-downloads"></iframe>
+        sandbox="allow-scripts allow-forms allow-modals"></iframe>
     </main>
 
     <button type="button" id="handle" class="handle" title="Hide comments panel" aria-label="Hide comments panel">›</button>
diff --git a/src/frame-policy.js b/src/frame-policy.js
index eda9cc6..10c91d1 100644
--- a/src/frame-policy.js
+++ b/src/frame-policy.js
@@ -1,4 +1,4 @@
-const BASE_SANDBOX = "allow-scripts allow-forms allow-modals allow-popups allow-downloads";
+const BASE_SANDBOX = "allow-scripts allow-forms allow-modals";
 
 /**
  * Localhost apps need their real origin so their routing and JavaScript work.
diff --git a/test/frame-policy.test.js b/test/frame-policy.test.js
index ce14d7d..628b6fc 100644
--- a/test/frame-policy.test.js
+++ b/test/frame-policy.test.js
@@ -19,3 +19,14 @@ test("file and Markdown reviews use an opaque origin", () => {
     assert.equal(policy.targetOrigin, "*");
   }
 });
+
+test("no review grants the frame popups or downloads", () => {
+  // The SDK hands external links to the chrome over postMessage, which opens
+  // them itself, so the frame never needs allow-popups. Nothing needs
+  // allow-downloads. Untrusted artifacts get neither.
+  for (const page of [{ kind: "url" }, { kind: "file", markdown: false }, { kind: "file", markdown: true }]) {
+    const policy = framePolicy(page, artifactOrigin);
+    assert.doesNotMatch(policy.sandbox, /\ballow-popups\b/);
+    assert.doesNotMatch(policy.sandbox, /\ballow-downloads\b/);
+  }
+});