From 6c5ab543c0b6b2644d18bc0d6ede5a6c1904f2df Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 24 Feb 2026 15:05:09 +0000 Subject: [PATCH] refactor: tighten external-link policy and window.open guard --- scripts/check-no-raw-window-open.mjs | 85 +++++++++++++++---- test/scripts/check-no-raw-window-open.test.ts | 39 +++++++++ ui/src/ui/app-render.ts | 5 +- ui/src/ui/external-link.test.ts | 18 ++++ ui/src/ui/external-link.ts | 19 +++++ ui/src/ui/test-helpers/app-mount.ts | 3 +- .../ui/views/chat-image-open.browser.test.ts | 1 - ui/src/ui/views/overview.ts | 21 ++--- 8 files changed, 162 insertions(+), 29 deletions(-) create mode 100644 test/scripts/check-no-raw-window-open.test.ts create mode 100644 ui/src/ui/external-link.test.ts create mode 100644 ui/src/ui/external-link.ts diff --git a/scripts/check-no-raw-window-open.mjs b/scripts/check-no-raw-window-open.mjs index 55334549ba1..930bfe60a61 100644 --- a/scripts/check-no-raw-window-open.mjs +++ b/scripts/check-no-raw-window-open.mjs @@ -3,6 +3,7 @@ import { promises as fs } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import ts from "typescript"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const uiSourceDir = path.join(repoRoot, "ui", "src", "ui"); @@ -39,20 +40,67 @@ async function collectTypeScriptFiles(dir) { return out; } -function lineNumberAt(content, index) { - let lines = 1; - for (let i = 0; i < index; i++) { - if (content.charCodeAt(i) === 10) { - lines++; +function unwrapExpression(expression) { + let current = expression; + while (true) { + if (ts.isParenthesizedExpression(current)) { + current = current.expression; + continue; } + if (ts.isAsExpression(current) || ts.isTypeAssertionExpression(current)) { + current = current.expression; + continue; + } + if (ts.isNonNullExpression(current)) { + current = current.expression; + continue; + } + return current; } +} + +function asPropertyAccess(expression) { + if (ts.isPropertyAccessExpression(expression)) { + return expression; + } + if (typeof ts.isPropertyAccessChain === "function" && ts.isPropertyAccessChain(expression)) { + return expression; + } + return null; +} + +function isRawWindowOpenCall(expression) { + const propertyAccess = asPropertyAccess(unwrapExpression(expression)); + if (!propertyAccess || propertyAccess.name.text !== "open") { + return false; + } + + const receiver = unwrapExpression(propertyAccess.expression); + return ( + ts.isIdentifier(receiver) && (receiver.text === "window" || receiver.text === "globalThis") + ); +} + +export function findRawWindowOpenLines(content, fileName = "source.ts") { + const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.Latest, true); + const lines = []; + + const visit = (node) => { + if (ts.isCallExpression(node) && isRawWindowOpenCall(node.expression)) { + const line = + sourceFile.getLineAndCharacterOfPosition(node.expression.getStart(sourceFile)).line + 1; + lines.push(line); + } + ts.forEachChild(node, visit); + }; + + visit(sourceFile); return lines; } -async function main() { +export async function main() { const files = await collectTypeScriptFiles(uiSourceDir); const violations = []; - const rawWindowOpenRe = /\bwindow\s*\.\s*open\s*\(/g; for (const filePath of files) { if (allowedCallsites.has(filePath)) { @@ -60,12 +108,9 @@ async function main() { } const content = await fs.readFile(filePath, "utf8"); - let match = rawWindowOpenRe.exec(content); - while (match) { - const line = lineNumberAt(content, match.index); + for (const line of findRawWindowOpenLines(content, filePath)) { const relPath = path.relative(repoRoot, filePath); violations.push(`${relPath}:${line}`); - match = rawWindowOpenRe.exec(content); } } @@ -81,7 +126,17 @@ async function main() { process.exit(1); } -main().catch((error) => { - console.error(error); - process.exit(1); -}); +const isDirectExecution = (() => { + const entry = process.argv[1]; + if (!entry) { + return false; + } + return path.resolve(entry) === fileURLToPath(import.meta.url); +})(); + +if (isDirectExecution) { + main().catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/test/scripts/check-no-raw-window-open.test.ts b/test/scripts/check-no-raw-window-open.test.ts new file mode 100644 index 00000000000..543c4b79793 --- /dev/null +++ b/test/scripts/check-no-raw-window-open.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; +import { findRawWindowOpenLines } from "../../scripts/check-no-raw-window-open.mjs"; + +describe("check-no-raw-window-open", () => { + it("finds direct window.open calls", () => { + const source = ` + function openDocs() { + window.open("https://docs.openclaw.ai"); + } + `; + expect(findRawWindowOpenLines(source)).toEqual([3]); + }); + + it("finds globalThis.open calls", () => { + const source = ` + function openDocs() { + globalThis.open("https://docs.openclaw.ai"); + } + `; + expect(findRawWindowOpenLines(source)).toEqual([3]); + }); + + it("ignores mentions in strings and comments", () => { + const source = ` + // window.open("https://example.com") + const text = "window.open('https://example.com')"; + `; + expect(findRawWindowOpenLines(source)).toEqual([]); + }); + + it("handles parenthesized and asserted window references", () => { + const source = ` + const openRef = (window as Window).open; + openRef("https://example.com"); + (window as Window).open("https://example.com"); + `; + expect(findRawWindowOpenLines(source)).toEqual([4]); + }); +}); diff --git a/ui/src/ui/app-render.ts b/ui/src/ui/app-render.ts index 487ba0bbc53..55eeaedd7e0 100644 --- a/ui/src/ui/app-render.ts +++ b/ui/src/ui/app-render.ts @@ -62,6 +62,7 @@ import { updateSkillEdit, updateSkillEnabled, } from "./controllers/skills.ts"; +import { buildExternalLinkRel, EXTERNAL_LINK_TARGET } from "./external-link.ts"; import { icons } from "./icons.ts"; import { normalizeBasePath, TAB_GROUPS, subtitleForTab, titleForTab } from "./navigation.ts"; import { renderAgents } from "./views/agents.ts"; @@ -289,8 +290,8 @@ export function renderApp(state: AppViewState) { diff --git a/ui/src/ui/external-link.test.ts b/ui/src/ui/external-link.test.ts new file mode 100644 index 00000000000..3c46c7faa30 --- /dev/null +++ b/ui/src/ui/external-link.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { buildExternalLinkRel } from "./external-link.ts"; + +describe("buildExternalLinkRel", () => { + it("always includes required security tokens", () => { + expect(buildExternalLinkRel()).toBe("noopener noreferrer"); + }); + + it("preserves extra rel tokens while deduping required ones", () => { + expect(buildExternalLinkRel("noreferrer nofollow NOOPENER")).toBe( + "noopener noreferrer nofollow", + ); + }); + + it("ignores whitespace-only rel input", () => { + expect(buildExternalLinkRel(" ")).toBe("noopener noreferrer"); + }); +}); diff --git a/ui/src/ui/external-link.ts b/ui/src/ui/external-link.ts new file mode 100644 index 00000000000..fc7ff5ef7e2 --- /dev/null +++ b/ui/src/ui/external-link.ts @@ -0,0 +1,19 @@ +const REQUIRED_EXTERNAL_REL_TOKENS = ["noopener", "noreferrer"] as const; + +export const EXTERNAL_LINK_TARGET = "_blank"; + +export function buildExternalLinkRel(currentRel?: string): string { + const extraTokens: string[] = []; + const seen = new Set(REQUIRED_EXTERNAL_REL_TOKENS); + + for (const rawToken of (currentRel ?? "").split(/\s+/)) { + const token = rawToken.trim().toLowerCase(); + if (!token || seen.has(token)) { + continue; + } + seen.add(token); + extraTokens.push(token); + } + + return [...REQUIRED_EXTERNAL_REL_TOKENS, ...extraTokens].join(" "); +} diff --git a/ui/src/ui/test-helpers/app-mount.ts b/ui/src/ui/test-helpers/app-mount.ts index f64c9da6dd6..d6fda9475c4 100644 --- a/ui/src/ui/test-helpers/app-mount.ts +++ b/ui/src/ui/test-helpers/app-mount.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach } from "vitest"; -import { OpenClawApp } from "../app.ts"; +import "../app.ts"; +import type { OpenClawApp } from "../app.ts"; export function mountApp(pathname: string) { window.history.replaceState({}, "", pathname); diff --git a/ui/src/ui/views/chat-image-open.browser.test.ts b/ui/src/ui/views/chat-image-open.browser.test.ts index 768c5968100..60e6df26554 100644 --- a/ui/src/ui/views/chat-image-open.browser.test.ts +++ b/ui/src/ui/views/chat-image-open.browser.test.ts @@ -1,5 +1,4 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import "../app.ts"; import { mountApp, registerAppMountHooks } from "../test-helpers/app-mount.ts"; registerAppMountHooks(); diff --git a/ui/src/ui/views/overview.ts b/ui/src/ui/views/overview.ts index 56889c0f6d5..3c341df473b 100644 --- a/ui/src/ui/views/overview.ts +++ b/ui/src/ui/views/overview.ts @@ -1,6 +1,7 @@ import { html } from "lit"; import { ConnectErrorDetailCodes } from "../../../../src/gateway/protocol/connect-error-details.js"; import { t, i18n, type Locale } from "../../i18n/index.ts"; +import { buildExternalLinkRel, EXTERNAL_LINK_TARGET } from "../external-link.ts"; import { formatRelativeTimestamp, formatDurationHuman } from "../format.ts"; import type { GatewayHelloOk } from "../gateway.ts"; import { formatNextRun } from "../presenter.ts"; @@ -59,8 +60,8 @@ export function renderOverview(props: OverviewProps) { Docs: Device pairing @@ -116,8 +117,8 @@ export function renderOverview(props: OverviewProps) { Docs: Control UI auth @@ -132,8 +133,8 @@ export function renderOverview(props: OverviewProps) { Docs: Control UI auth @@ -171,8 +172,8 @@ export function renderOverview(props: OverviewProps) { Docs: Tailscale Serve @@ -180,8 +181,8 @@ export function renderOverview(props: OverviewProps) { Docs: Insecure HTTP