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 @@
+ sandbox="allow-scripts allow-forms allow-modals">
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/);
+ }
+});