Harden review frame: drop popup/download rights, validate external hrefs

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`, {

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>

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.

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/);
+  }
+});