diff --git a/package-lock.json b/package-lock.json index a1e477da..e00a8ce4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "riftlauncher", - "version": "1.7.0-beta.7", + "version": "1.7.0-beta.8", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "riftlauncher", - "version": "1.7.0-beta.7", + "version": "1.7.0-beta.8", "hasInstallScript": true, "dependencies": { "@electron-toolkit/utils": "^3.0.0", diff --git a/package.json b/package.json index 26c5a0c9..adc42222 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "riftlauncher", - "version": "1.7.0-beta.7", + "version": "1.7.0-beta.8", "description": "RiftLauncher for Vintage Story", "overrides": { "js-yaml": "4.3.1", diff --git a/src/domain/mods/installedFilters.ts b/src/domain/mods/installedFilters.ts index ee0401df..259ff399 100644 --- a/src/domain/mods/installedFilters.ts +++ b/src/domain/mods/installedFilters.ts @@ -24,6 +24,32 @@ export const NO_INSTALLED_MOD_FILTERS: InstalledModFilters = { author: "", tags: /** The shape a ModDB game-version release tag has: a dotted number, never a leading "v". */ const GAME_VERSION_TAG = /^\d+(\.\d+)+/ +/** + * Keeps only the string entries of a list handed to us by the mod database or a modinfo. + * + * The API's documented shape says `tags` is a list of strings. The live detail endpoint returns + * `["Cosmetics", "Crafting", "Storage", null]` for Vanilla Variants (#370). Every reader below folds + * case or tests a prefix, both of which throw on `null`, and one throw during the first render blanks + * the whole page. So nothing in this module trusts the shape: whatever is not a string is dropped, and + * a field that is not a list at all reads as empty. + */ +function textEntries(values: unknown): string[] { + return Array.isArray(values) ? values.filter((value): value is string => typeof value === "string") : [] +} + +/** + * The releases the mod database lists for a mod, keeping only the entries that are objects. + * + * Guarding the outer field is not enough: `{ releases: [null] }` passes an array check and then + * `release.tags` throws on the entry, the same render crash this module exists to rule out. So each + * entry is checked too, and a missing or malformed field reads as no releases at all. + */ +function releasesOf(mod: InstalledModType): readonly { tags?: unknown }[] { + const releases: unknown = mod._mod?.releases + if (!Array.isArray(releases)) return [] + return releases.filter((release): release is { tags?: unknown } => typeof release === "object" && release !== null) +} + /** * Deduplicates ignoring case, keeping the first spelling seen. * @@ -42,7 +68,7 @@ function uniqueIgnoringCase(values: readonly string[]): string[] { /** Every author across the scan, A to Z. Read from the local modinfo, so this axis needs no network. */ export function installedModAuthors(mods: readonly InstalledModType[]): string[] { - return uniqueIgnoringCase(mods.flatMap((mod) => mod.authors ?? [])).sort((a, b) => a.localeCompare(b)) + return uniqueIgnoringCase(mods.flatMap((mod) => textEntries(mod.authors))).sort((a, b) => a.localeCompare(b)) } /** @@ -52,7 +78,7 @@ export function installedModAuthors(mods: readonly InstalledModType[]): string[] * modinfo.ts shows, so this list is empty for a folder the ModDB cannot answer for. */ export function installedModTags(mods: readonly InstalledModType[]): string[] { - return uniqueIgnoringCase(mods.flatMap((mod) => mod._mod?.tags ?? [])).sort((a, b) => a.localeCompare(b)) + return uniqueIgnoringCase(mods.flatMap((mod) => textEntries(mod._mod?.tags))).sort((a, b) => a.localeCompare(b)) } /** @@ -63,7 +89,7 @@ export function installedModTags(mods: readonly InstalledModType[]): string[] { * are the category tags the other dropdown offers. */ export function installedModGameVersions(mods: readonly InstalledModType[]): string[] { - const tags = mods.flatMap((mod) => (mod._mod?.releases ?? []).flatMap((release) => release.tags ?? [])) + const tags = mods.flatMap((mod) => releasesOf(mod).flatMap((release) => textEntries(release.tags))) return Array.from(new Set(tags.filter((tag) => GAME_VERSION_TAG.test(tag)))).sort((a, b) => compareVersions(b, a)) } @@ -71,13 +97,13 @@ export function installedModGameVersions(mods: readonly InstalledModType[]): str function matchesAuthor(mod: InstalledModType, author: string): boolean { if (author === "") return true const wanted = author.toLowerCase() - return (mod.authors ?? []).some((name) => name.toLowerCase() === wanted) + return textEntries(mod.authors).some((name) => name.toLowerCase() === wanted) } /** Every selected tag has to be present, so picking a second tag narrows the list rather than widening it. */ function matchesTags(mod: InstalledModType, tags: readonly string[]): boolean { if (tags.length < 1) return true - const modTags = (mod._mod?.tags ?? []).map((tag) => tag.toLowerCase()) + const modTags = textEntries(mod._mod?.tags).map((tag) => tag.toLowerCase()) return tags.every((tag) => modTags.includes(tag.toLowerCase())) } @@ -91,7 +117,7 @@ function matchesTags(mod: InstalledModType, tags: readonly string[]): boolean { */ function matchesGameVersion(mod: InstalledModType, gameVersion: string): boolean { if (gameVersion === "") return true - return (mod._mod?.releases ?? []).some((release) => evaluateModCompatibility(release.tags ?? [], gameVersion) !== "undeclared") + return releasesOf(mod).some((release) => evaluateModCompatibility(textEntries(release.tags), gameVersion) !== "undeclared") } /** All three axes at once. A mod clears every one of them, so each pick narrows what is left. */ diff --git a/src/domain/mods/moddb.ts b/src/domain/mods/moddb.ts index 07732647..8fb5afa0 100644 --- a/src/domain/mods/moddb.ts +++ b/src/domain/mods/moddb.ts @@ -120,9 +120,32 @@ export function parseModListResponse(rawText: string): ModDbResponse { modid: number name: string + /** Always a list of strings, whatever the API sent: see {@link readModDetail}. */ + tags: string[] + /** Always a list of objects, each with string `modversion` and string-only `tags`: see {@link readRelease}. */ + releases: Record[] logofile?: string } +/** + * Cleans one entry of a detail's `releases` array. + * + * Every reader downstream, the update scan, the release list, the modpack planner, treats a release + * as an object with a string `modversion` and a `tags` array of strings, because that is what the + * ModDB documents. It is not what the ModDB always sends: a listing has been seen with a `null` in + * the array and with `null` under `tags`, which is what took the Manage Mods page down in beta.7. + * + * `modversion` is coerced to an empty string rather than dropping the release, because + * {@link newestReleaseFileId} reads `releases[0]` to build the download URL for the newest file: + * removing an entry here would silently shift which file "install newest" picks. Every reader + * already treats a falsy `modversion` as unusable, so an empty one costs that release nothing but + * the version comparisons it could never have taken part in anyway. + */ +function readRelease(value: unknown): Record | undefined { + if (!isRecord(value)) return undefined + return { ...value, tags: cleanStrings(value["tags"]), modversion: typeof value["modversion"] === "string" ? value["modversion"] : "" } +} + function readModDetail(value: unknown): ModDbModDetail | undefined { if (!isRecord(value)) return undefined @@ -132,9 +155,20 @@ function readModDetail(value: unknown): ModDbModDetail | undefined { // The install popup maps `releases` directly, so a detail without the array would crash it // instead of reaching its failure state. Unlike modid and name this is not a field the API is // known to always send, hence the explicit check rather than trusting the shape. - if (!Array.isArray(value["releases"])) return undefined - - return { ...value, modid, name, ...(typeof value["logofile"] === "string" ? { logofile: value["logofile"] } : {}) } + const releases = value["releases"] + if (!Array.isArray(releases)) return undefined + + return { + ...value, + modid, + name, + // Mod-level tags carry the same `null` the Vanilla Variants listing shipped in beta.7. The + // installed filters tolerate it on their own since #371, cleaning it here makes that tolerance + // a second line rather than the only one. + tags: cleanStrings(value["tags"]), + releases: releases.map(readRelease).filter((release): release is Record => release !== undefined), + ...(typeof value["logofile"] === "string" ? { logofile: value["logofile"] } : {}) + } } /** @@ -155,10 +189,7 @@ export function parseModDetailResponse(rawText: string): ModDbResponse 0 ? fileId : undefined } diff --git a/tests/domain/mods/installedFilters.test.ts b/tests/domain/mods/installedFilters.test.ts index 8585c0cf..d906fcda 100644 --- a/tests/domain/mods/installedFilters.test.ts +++ b/tests/domain/mods/installedFilters.test.ts @@ -167,3 +167,52 @@ describe("matching one installed mod against the filters", () => { assert.equal(hasActiveInstalledModFilters(filters({ gameVersion: "1.20.0" })), true) }) }) + +describe("real mod database shapes (#370)", () => { + // The detail endpoint returns exactly this for Vanilla Variants: a null among the category tags. + // The documented shape says strings only, and the launcher used to believe it. + const nullTag = ["Cosmetics", "Crafting", "Storage", null] as unknown as string[] + const vanillaVariants = aMod("VanillaVariants", { _mod: aDetail(nullTag, [["1.21.0", "1.21.1"]]) }) + + it("collects category tags past a null in the list instead of throwing", () => { + assert.deepEqual(installedModTags([vanillaVariants]), ["Cosmetics", "Crafting", "Storage"]) + }) + + it("still matches the tags that are real when a null sits beside them", () => { + assert.equal(matchesInstalledModFilters(vanillaVariants, { ...NO_INSTALLED_MOD_FILTERS, tags: ["storage"] }), true) + assert.equal(matchesInstalledModFilters(vanillaVariants, { ...NO_INSTALLED_MOD_FILTERS, tags: ["Food"] }), false) + }) + + it("collects game versions past a null in a release's tags", () => { + const releaseWithNull = [["1.21.0", null, "1.21.1"]] as unknown as string[][] + const mod = aMod("Odd", { _mod: aDetail(["Other"], releaseWithNull) }) + assert.deepEqual(installedModGameVersions([mod]), ["1.21.1", "1.21.0"]) + assert.equal(matchesInstalledModFilters(mod, { ...NO_INSTALLED_MOD_FILTERS, gameVersion: "1.21.0" }), true) + }) + + it("skips a null entry inside an otherwise valid releases list", () => { + // { releases: [null, {...}] } passes an array check and used to throw on `release.tags`. + const detail = aDetail(["Other"], [["1.21.0"]]) + const mod = aMod("Odd", { _mod: { ...detail, releases: [null, ...detail.releases] as unknown as typeof detail.releases } }) + assert.deepEqual(installedModGameVersions([mod]), ["1.21.0"]) + assert.equal(matchesInstalledModFilters(mod, { ...NO_INSTALLED_MOD_FILTERS, gameVersion: "1.21.0" }), true) + }) + + it("ignores an author entry that is not a string", () => { + const mod = aMod("Odd", { authors: ["Ann", null, 42, "Bob"] as unknown as string[] }) + assert.deepEqual(installedModAuthors([mod]), ["Ann", "Bob"]) + assert.equal(matchesInstalledModFilters(mod, { ...NO_INSTALLED_MOD_FILTERS, author: "bob" }), true) + }) + + it("reads a tags field that is not a list as no tags, and releases that are not a list as no releases", () => { + const detail = aDetail([], []) + // A string rather than null on purpose: `null ?? []` would read as empty by accident, and the + // guard has to hold for anything that is not a list, not only for the absent case. + const mod = aMod("Odd", { _mod: { ...detail, tags: "Cosmetics" as unknown as string[], releases: "1.21.0" as unknown as typeof detail.releases } }) + assert.deepEqual(installedModTags([mod]), []) + assert.deepEqual(installedModGameVersions([mod]), []) + assert.equal(matchesInstalledModFilters(mod, { ...NO_INSTALLED_MOD_FILTERS, tags: ["cosmetics"] }), false) + assert.equal(matchesInstalledModFilters(mod, { ...NO_INSTALLED_MOD_FILTERS, gameVersion: "1.21.0" }), false) + assert.equal(filterInstalledMods([mod], NO_INSTALLED_MOD_FILTERS).length, 1) + }) +}) diff --git a/tests/domain/mods/moddb.test.ts b/tests/domain/mods/moddb.test.ts index e1564921..bb4f5557 100644 --- a/tests/domain/mods/moddb.test.ts +++ b/tests/domain/mods/moddb.test.ts @@ -113,7 +113,7 @@ describe("parseModDetailResponse", () => { assert.equal(result.payload.modid, 1783) assert.equal(result.payload.name, "Config lib") assert.equal(result.payload.logofile, "https://moddbcdn.vintagestory.at/config.png") - assert.deepEqual(result.payload["releases"], [{ releaseid: 1, modversion: "1.12.0" }]) + assert.deepEqual(result.payload["releases"], [{ releaseid: 1, modversion: "1.12.0", tags: [] }]) }) it("names a mod field that is not an object as malformed", () => { @@ -142,6 +142,72 @@ describe("parseModDetailResponse", () => { }) }) +/** + * Issue #370: the ModDB served Vanilla Variants with a `null` among its tags, and the first render + * of Manage Mods died on it. The hotfix taught the installed filters to survive that. These pin the + * same shapes at the boundary instead, so that no reader downstream has to know about them. + */ +describe("parseModDetailResponse: shapes the ModDB actually sends", () => { + function detailOf(mod: unknown): ModDbModDetail { + const result = parseModDetailResponse(JSON.stringify({ statuscode: "200", mod })) + if (!result.ok) throw new Error("unreachable") + return result.payload + } + + it("keeps a null out of the mod's own tags, the Vanilla Variants payload that took the page down", () => { + const detail = detailOf({ modid: 1, name: "Vanilla Variants", tags: ["Cosmetics", "Crafting", "Storage", null], releases: [] }) + assert.deepEqual(detail.tags, ["Cosmetics", "Crafting", "Storage"]) + }) + + it("reads a missing or wrongly typed mod tags field as no tags at all", () => { + assert.deepEqual(detailOf({ modid: 1, name: "No Tags", releases: [] }).tags, []) + assert.deepEqual(detailOf({ modid: 1, name: "Odd Tags", tags: "Storage", releases: [] }).tags, []) + }) + + it("drops a release that is not an object, which every consumer dereferences unchecked", () => { + const detail = detailOf({ modid: 1, name: "Nulled", releases: [null, { releaseid: 2, modversion: "1.1.0" }, "not a release"] }) + assert.deepEqual( + detail.releases.map((release) => release["releaseid"]), + [2] + ) + }) + + it("keeps a null out of a release's tags, which reach evaluateModCompatibility raw", () => { + const detail = detailOf({ + modid: 1, + name: "Tagged", + releases: [ + { modversion: "1.1.0", tags: ["1.21.0", null] }, + { modversion: "1.0.0", tags: null } + ] + }) + assert.deepEqual( + detail.releases.map((release) => release["tags"]), + [["1.21.0"], []] + ) + }) + + it("reads a release modversion that is not a string as an empty one, rather than dropping the release", () => { + // Dropping it would shift releases[0], which newestReleaseFileId turns into the download URL. + const detail = detailOf({ modid: 1, name: "Versionless", releases: [{ fileid: 42, modversion: null }] }) + assert.equal(detail.releases[0]?.["modversion"], "") + assert.equal(newestReleaseFileId(detail), 42) + }) + + it("carries every other release field through untouched", () => { + const detail = detailOf({ modid: 1, name: "Whole", releases: [{ releaseid: 9, fileid: 42, mainfile: "https://mods.example/a.zip", modidstr: "a", changelog: "

hi

" }] }) + assert.deepEqual(detail.releases[0], { + releaseid: 9, + fileid: 42, + mainfile: "https://mods.example/a.zip", + modidstr: "a", + changelog: "

hi

", + modversion: "", + tags: [] + }) + }) +}) + describe("newestReleaseFileId", () => { function detail(releases: unknown[]): ModDbModDetail { const result = parseModDetailResponse(JSON.stringify({ statuscode: "200", mod: { modid: 11016, name: "RiftLauncher", releases } })) diff --git a/tests/renderer-dom/manageMods.test.tsx b/tests/renderer-dom/manageMods.test.tsx index a55dcc1a..f68ab49d 100644 --- a/tests/renderer-dom/manageMods.test.tsx +++ b/tests/renderer-dom/manageMods.test.tsx @@ -1016,3 +1016,66 @@ describe("ManageMods: filtering the installed Mods", { timeout: 20000 }, () => { expect(screen.getByRole("button", { name: "VS Version" })).toBeTruthy() }) }) + +/** + * Issue #370, and Zaldaryon's review of #374 on top of it. Beta.7 shipped a blank Manage Mods page + * because the ModDB served Vanilla Variants with a `null` among its tags. The hotfix taught the + * installed filters to read around that, but `useGetCompleteInstalledMods` still walks the raw + * `releases` array first, so the page died before the filters ever ran. + * + * The payload below is deliberately raw and deliberately ugly: it goes through the real + * `parseModDetailResponse` on its way in, which is where the shape is now fixed. + */ +describe("ManageMods: a ModDB payload with nulls in it", () => { + const ALPHA_MALFORMED = JSON.stringify({ + statuscode: "200", + mod: { + modid: 1, + assetid: 101, + name: "Alpha Mod", + // The Vanilla Variants tag list, verbatim. + tags: ["Cosmetics", "Crafting", "Storage", null], + releases: [ + // A hole in the array: `release.modversion` on this one is what threw. + null, + { releaseid: 14, mainfile: "https://mods.example/alpha-1.3.0.zip", filename: "alpha-1.3.0.zip", fileid: 14, tags: null, modidstr: "alpha", modversion: "1.3.0" }, + { releaseid: 13, mainfile: "https://mods.example/alpha-1.2.0.zip", filename: "alpha-1.2.0.zip", fileid: 13, tags: ["1.20.0", null], modidstr: "alpha", modversion: "1.2.0" }, + { releaseid: 12, mainfile: "https://mods.example/alpha-1.1.0.zip", filename: "alpha-1.1.0.zip", fileid: 12, tags: ["1.20.0"], modidstr: "alpha", modversion: null } + ] + } + }) + + function queryMalformedModDb(url: string): Promise { + if (url.endsWith("/mod/alpha")) return Promise.resolve(ALPHA_MALFORMED) + return queryModDb(url) + } + + it("finishes loading and lists the Mod instead of taking the page down", async () => { + renderManageMods({ netManager: { queryURL: vi.fn(queryMalformedModDb) } }) + + // The page renders at all, which is the whole of #370. + expect(await screen.findByText("Alpha Mod", {}, { timeout: 3000 })).toBeTruthy() + expect(screen.getByText("Beta Mod")).toBeTruthy() + expect(screen.getByText("Gamma Mod")).toBeTruthy() + expect(screen.getByText("broken.zip")).toBeTruthy() + }) + + it("still reads the update off the releases that survived the cleaning", async () => { + const user = userEvent.setup() + renderManageMods({ + netManager: { queryURL: vi.fn(queryMalformedModDb) }, + modsManager: { getInstalledMods: vi.fn(async () => ({ mods: [aModScan().mods[0] as InstalledModType], errors: [] })) }, + pathsManager: { deletePath: vi.fn(async () => true), downloadOnPath: vi.fn(async () => "/games/a/Mods/alpha-1.2.0.zip") } + }) + + await screen.findByText("Alpha Mod", {}, { timeout: 3000 }) + expect(screen.getByText("Mods with updates")).toBeTruthy() + + await user.click(screen.getByText("Update all").closest("button") as HTMLElement) + + // 1.3.0 is newer still, but its tags came in as `null`: undeclared, so the offer is 1.2.0, the + // newest release actually tagged for this Installation once its own null tag was dropped. + const summary = await screen.findByRole("dialog", {}, { timeout: 3000 }) + expect(within(summary).getByText("v1.2.0")).toBeTruthy() + }) +}) diff --git a/tests/security-boundaries.test.ts b/tests/security-boundaries.test.ts index b8b303bd..9198d059 100644 --- a/tests/security-boundaries.test.ts +++ b/tests/security-boundaries.test.ts @@ -3,6 +3,7 @@ import { readdirSync, readFileSync } from "node:fs" import { resolve } from "node:path" import { pathToFileURL } from "node:url" import { describe, it } from "vitest" +import * as ts from "typescript" import { parseLegacyAccount, parseLoginAccount } from "../src/domain/account/credentials" import { isAllowedRendererUrl, parseSafeEnvironment, validateGameInstallation, validateGameVersion } from "../src/ipc/validation" @@ -100,6 +101,7 @@ describe("process and navigation boundaries", () => { const MAIN_SOURCE = readFileSync(resolve(__dirname, "../src/main/index.ts"), "utf8") const PRELOAD_SOURCE = readFileSync(resolve(__dirname, "../src/preload/index.ts"), "utf8") const RENDERER_HTML = readFileSync(resolve(__dirname, "../src/renderer/index.html"), "utf8") +const MAIN_AST = ts.createSourceFile("src/main/index.ts", MAIN_SOURCE, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS) describe("startup network boundaries", () => { it("keeps both session calls that stop the spellcheck dictionary download", () => { @@ -130,14 +132,78 @@ describe("startup network boundaries", () => { }) }) +/** + * The renderer CSP is one long attribute, so a substring match on it is a poor + * pin: "script-src 'self'" is still present after someone appends a host to the + * source list. Parsing the attribute into directives means the assertion below + * compares the value the browser will actually apply, and it survives a + * reordering or a reflow of the attribute. + */ +function rendererCspDirectives(html = RENDERER_HTML): Map { + const activeMetaTags = html + .replace(//g, "") + .match(/]*>/gi) + ?.map((tag) => { + const attributes = new Map() + for (const match of tag.matchAll(/\b([:\w-]+)\s*=\s*(["'])(.*?)\2/gi)) { + const rawName = match[1] ?? "" + const value = match[3] ?? "" + attributes.set(rawName.toLowerCase(), value) + } + return attributes + }) + .filter((attributes) => attributes.get("http-equiv")?.toLowerCase() === "content-security-policy") + .map((attributes) => attributes.get("content")) + .filter((policy): policy is string => policy !== undefined) + + if (activeMetaTags?.length !== 1) { + throw new Error(`Expected exactly one active Content-Security-Policy meta tag, found ${activeMetaTags?.length ?? 0}`) + } + + const policy = activeMetaTags[0] + if (policy === undefined) throw new Error("Content-Security-Policy meta tag has no content attribute") + + const directives = new Map() + for (const directive of policy + .split(";") + .map((entry) => entry.trim()) + .filter(Boolean)) { + const [rawName = "", ...sources] = directive.split(/\s+/) + const name = rawName.toLowerCase() + if (directives.has(name)) throw new Error(`Duplicate CSP directive: ${name}`) + directives.set(name, sources.join(" ")) + } + return directives +} + describe("renderer document boundaries", () => { + it("keeps every renderer script source local", () => { + const directives = rendererCspDirectives() + + assert.equal(directives.get("script-src"), "'self'", `renderer script-src is now ${JSON.stringify(directives.get("script-src"))}; the renderer must not run script from anywhere but the bundle`) + assert.equal(directives.get("default-src"), "'self'", `renderer default-src is now ${JSON.stringify(directives.get("default-src"))}; it is the fallback every directive that goes missing lands on`) + assert.equal(directives.get("object-src"), "'none'", "renderer object-src stopped blocking plugin content") + assert.equal(directives.get("base-uri"), "'none'", "renderer base-uri stopped blocking a rewritten document base") + }) + + it("rejects duplicate CSP directives instead of hiding the first policy", () => { + const duplicatePolicy = '' + assert.throws(() => rendererCspDirectives(duplicatePolicy), /Duplicate CSP directive: script-src/) + + const caseVariantPolicy = '' + assert.throws(() => rendererCspDirectives(caseVariantPolicy), /Duplicate CSP directive: script-src/) + }) + + it("reads one active CSP meta tag and ignores comments", () => { + const commentedPolicy = '' + assert.equal(rendererCspDirectives(commentedPolicy).get("script-src"), "'self'") + + const multiplePolicies = '' + assert.throws(() => rendererCspDirectives(multiplePolicies), /Expected exactly one active Content-Security-Policy meta tag, found 2/) + }) + it("does not allow framed content in the renderer CSP", () => { - assert.match(RENDERER_HTML, /frame-src 'none'/) - // Match the full directive up to its semicolon or end-of-string so that - // frame-src 'none' https://evil.example does not slip through: when a - // source list holds 'none' alongside other expressions, browsers ignore - // the 'none' and honour the rest. - assert.doesNotMatch(RENDERER_HTML, /frame-src\s+'none'\s*[^';"\n]/) + assert.equal(rendererCspDirectives().get("frame-src"), "'none'") }) it("exposes the preload bridge only in the main frame", () => { @@ -158,16 +224,558 @@ describe("renderer document boundaries", () => { }) }) -function mainHandlerSource(startMarker: string, endMarker: string): string { - const start = MAIN_SOURCE.indexOf(startMarker) - if (start === -1) throw new Error(`Could not find main-process handler: ${startMarker}`) +function findNodes(root: ts.Node, predicate: (node: ts.Node) => node is T): T[] { + const matches: T[] = [] - const end = MAIN_SOURCE.indexOf(endMarker, start + startMarker.length) - if (end === -1) throw new Error(`Could not find end of main-process handler: ${startMarker}`) + function visit(node: ts.Node): void { + if (predicate(node)) matches.push(node) + ts.forEachChild(node, visit) + } - return MAIN_SOURCE.slice(start, end) + visit(root) + return matches +} + +function findCreateWindow(): ts.FunctionDeclaration { + const functions = findNodes(MAIN_AST, (node): node is ts.FunctionDeclaration => ts.isFunctionDeclaration(node) && node.name?.text === "createWindow") + if (functions.length !== 1 || functions[0]?.body === undefined) throw new Error(`Expected one createWindow function with a body, found ${functions.length}`) + return functions[0] +} + +function sourceAst(source: string, fileName: string): ts.SourceFile { + return ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS) +} + +function propertyName(name: ts.PropertyName | undefined): string | undefined { + if (name === undefined) return undefined + if (ts.isIdentifier(name) || ts.isStringLiteral(name) || ts.isNumericLiteral(name)) return name.text + return undefined +} + +function uniqueProperty(object: ts.ObjectLiteralExpression, name: string): ts.PropertyAssignment { + const matches = object.properties.filter((property): property is ts.PropertyAssignment => ts.isPropertyAssignment(property) && propertyName(property.name) === name) + if (matches.length !== 1 || matches[0] === undefined) throw new Error(`Expected exactly one ${name} property, found ${matches.length}`) + return matches[0] +} + +function functionHandler(call: ts.CallExpression, method: string): ts.ArrowFunction { + if (!ts.isPropertyAccessExpression(call.expression) || call.expression.name.text !== method) throw new Error(`Expected a ${method} call`) + const handler = call.arguments[0] + if (handler === undefined || !ts.isArrowFunction(handler)) throw new Error(`${method} must receive an arrow-function handler`) + return handler +} + +function directCallStatements(body: ts.Block): ts.CallExpression[] { + return body.statements + .filter((statement): statement is ts.ExpressionStatement => ts.isExpressionStatement(statement)) + .map((statement) => statement.expression) + .filter((expression): expression is ts.CallExpression => ts.isCallExpression(expression)) } +function isMainWindowWebContentsCall(call: ts.CallExpression, method: string): boolean { + if (!ts.isPropertyAccessExpression(call.expression) || call.expression.name.text !== method) return false + const receiver = call.expression.expression + return ts.isPropertyAccessExpression(receiver) && receiver.name.text === "webContents" && ts.isIdentifier(receiver.expression) && receiver.expression.text === "mainWindow" +} + +function webContentsHandler(ast: ts.SourceFile, method: string): ts.ArrowFunction { + const createWindow = findNodes(ast, (node): node is ts.FunctionDeclaration => ts.isFunctionDeclaration(node) && node.name?.text === "createWindow")[0] + if (createWindow?.body === undefined) throw new Error("Expected a createWindow function with a body") + + const calls = directCallStatements(createWindow.body).filter((call) => isMainWindowWebContentsCall(call, method)) + + if (calls.length !== 1 || calls[0] === undefined) throw new Error(`Expected exactly one direct mainWindow.webContents.${method} registration, found ${calls.length}`) + return functionHandler(calls[0], method) +} + +function unwrapParentheses(node: ts.Expression): ts.Expression { + return ts.isParenthesizedExpression(node) ? unwrapParentheses(node.expression) : node +} + +/** + * Whether the statement can hand control out of the handler body on any path: + * a return, a throw, or a break or continue, wherever it sits inside the + * statement. Bodies of nested functions are skipped, because a return in there + * leaves that function, not this handler. + * + * This is the conservative half of a choice. Deciding that `if (x) return` only + * leaves on one path, and that the statements after it still run on the other, + * needs a real control-flow walk where the denial has to dominate every exit. + * The shipped handlers in src/main/index.ts do not need that: the only + * statement any of them runs before its denial is the try/catch in the + * window-open handler, which contains no return, throw, break or continue. So + * anything that can leave early stops the scan, and a handler that grows a + * genuine early exit has to reflow rather than argue with the pin. + */ +function canLeaveTheBody(statement: ts.Statement): boolean { + let found = false + + function visit(node: ts.Node): void { + if (found || ts.isFunctionLike(node)) return + if (ts.isReturnStatement(node) || ts.isThrowStatement(node) || ts.isBreakStatement(node) || ts.isContinueStatement(node)) { + found = true + return + } + ts.forEachChild(node, visit) + } + + visit(statement) + return found +} + +/** + * The direct statements of a handler body the runtime always reaches: the + * statements of the block itself, stopping after the first one that can leave + * it on any path. A statement nested in an if, a ternary, a try, a loop or + * another function is not in this list, and neither is anything written after a + * conditional return or throw. That is the whole point. findNodes walks the + * entire subtree, so a defense pinned with it alone is satisfied by a call the + * runtime can skip, which is what happened here: `if (false) callback(false)` + * matched, and so did `if (x) return; callback(false)`. + */ +function reachableStatements(body: ts.ConciseBody): ts.Statement[] { + if (!ts.isBlock(body)) return [] + const reachable: ts.Statement[] = [] + for (const statement of body.statements) { + reachable.push(statement) + if (canLeaveTheBody(statement)) break + } + return reachable +} + +/** + * An expression a handler evaluates directly, with the way it is written kept + * alongside it: `x` in `() => x` and `return x` produce the handler's answer, + * while `x;` on its own line evaluates and throws the value away. Telling those + * apart is what stops a bare `false` expression statement from reading as a + * denial. + */ +interface DirectExpression { + expression: ts.Expression + isAnswer: boolean +} + +/** + * The expressions a handler always evaluates: its expression body, or the + * expressions of the reachable direct statements of its block body. Both shapes + * matter because the real permission handlers are one-expression arrows. + */ +function directExpressions(body: ts.ConciseBody): DirectExpression[] { + if (!ts.isBlock(body)) return [{ expression: unwrapParentheses(body), isAnswer: true }] + const expressions: DirectExpression[] = [] + for (const statement of reachableStatements(body)) { + if (ts.isExpressionStatement(statement)) expressions.push({ expression: unwrapParentheses(statement.expression), isAnswer: false }) + else if (ts.isReturnStatement(statement) && statement.expression !== undefined) expressions.push({ expression: unwrapParentheses(statement.expression), isAnswer: true }) + } + return expressions +} + +function isCallToPreventDefault(node: ts.Statement, parameter: string): boolean { + if (!ts.isExpressionStatement(node) || !ts.isCallExpression(node.expression)) return false + const call = node.expression + return ( + ts.isPropertyAccessExpression(call.expression) && + call.expression.name.text === "preventDefault" && + call.arguments.length === 0 && + ts.isIdentifier(call.expression.expression) && + call.expression.expression.text === parameter + ) +} + +function onlyStatement(consequent: ts.Statement): ts.Statement | undefined { + if (ts.isBlock(consequent)) return consequent.statements.length === 1 ? consequent.statements[0] : undefined + return consequent +} + +function isRejectedUrlCondition(condition: ts.Expression, urlArgument: string): boolean { + const expression = unwrapParentheses(condition) + if (!ts.isPrefixUnaryExpression(expression) || expression.operator !== ts.SyntaxKind.ExclamationToken) return false + const call = unwrapParentheses(expression.operand) + return ( + ts.isCallExpression(call) && call.arguments.length === 1 && ts.isIdentifier(call.expression) && call.expression.text === "isAllowedMainFrameUrl" && call.arguments[0]?.getText() === urlArgument + ) +} + +function assertNavigationHandler(source: string, eventName: string): void { + const ast = sourceAst(source, `fixture-${eventName}.ts`) + const createWindow = findNodes(ast, (node): node is ts.FunctionDeclaration => ts.isFunctionDeclaration(node) && node.name?.text === "createWindow")[0] + if (createWindow?.body === undefined) throw new Error("Expected a createWindow fixture") + + const registrations = directCallStatements(createWindow.body).filter((call) => { + if (!isMainWindowWebContentsCall(call, "on")) return false + return call.arguments[0]?.getText(ast) === JSON.stringify(eventName) + }) + if (registrations.length !== 1 || registrations[0] === undefined) throw new Error(`Expected one direct ${eventName} registration`) + + const handler = registrations[0].arguments[1] + if (handler === undefined || !ts.isArrowFunction(handler) || !ts.isBlock(handler.body)) throw new Error(`${eventName} must use a block-bodied arrow-function handler`) + + const guard = reachableStatements(handler.body).filter(ts.isIfStatement) + if (guard.length !== 1 || guard[0] === undefined || guard[0].elseStatement !== undefined) throw new Error(`${eventName} must have exactly one direct guard without an alternate branch`) + const ifStatement = guard[0] + const consequent = onlyStatement(ifStatement.thenStatement) + if (consequent === undefined) throw new Error(`${eventName} guard must have exactly one executable statement`) + + if (eventName === "will-frame-navigate") { + const detailsParameter = handler.parameters[0]?.name + if (detailsParameter === undefined || !ts.isIdentifier(detailsParameter)) throw new Error(`${eventName} must expose navigation details`) + const details = detailsParameter.text + const condition = unwrapParentheses(ifStatement.expression) + if (!ts.isBinaryExpression(condition) || condition.operatorToken.kind !== ts.SyntaxKind.AmpersandAmpersandToken) throw new Error(`${eventName} must check the main frame before the URL`) + const left = unwrapParentheses(condition.left) + const right = unwrapParentheses(condition.right) + if ( + !ts.isPropertyAccessExpression(left) || + left.name.text !== "isMainFrame" || + !ts.isIdentifier(left.expression) || + left.expression.text !== details || + !isRejectedUrlCondition(right, `${details}.url`) + ) + throw new Error(`${eventName} must reject unsafe main-frame URLs`) + if (!isCallToPreventDefault(consequent, details)) throw new Error(`${eventName} must prevent the rejected navigation`) + return + } + + const eventParameter = handler.parameters[0]?.name + const urlParameter = handler.parameters[1]?.name + if (eventParameter === undefined || !ts.isIdentifier(eventParameter) || urlParameter === undefined || !ts.isIdentifier(urlParameter)) + throw new Error(`${eventName} must expose event and URL parameters`) + if (!isRejectedUrlCondition(ifStatement.expression, urlParameter.text) || !isCallToPreventDefault(consequent, eventParameter.text)) throw new Error(`${eventName} must prevent a rejected URL`) +} + +/** + * Reads the boolean webPreferences the main window is built with as values + * rather than as text. A substring pin on "sandbox: true" would also be + * satisfied by a comment mentioning it, and it says nothing when the flag flips; + * this fails with the flag that changed and its new value. Comment lines and + * non-boolean options (preload, icon) do not match, so they are simply skipped. + */ +function mainWindowFlags(): Map { + const createWindow = findCreateWindow() + if (createWindow.body === undefined) throw new Error("Expected createWindow to have a body") + const windows = findNodes(createWindow.body, (node): node is ts.NewExpression => ts.isNewExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === "BrowserWindow") + if (windows.length !== 1 || windows[0] === undefined) throw new Error(`Expected exactly one BrowserWindow construction, found ${windows.length}`) + + const window = windows[0] + const options = window.arguments?.[0] + if (options === undefined || !ts.isObjectLiteralExpression(options)) throw new Error("BrowserWindow must receive an options object") + const preferences = uniqueProperty(options, "webPreferences").initializer + if (!ts.isObjectLiteralExpression(preferences)) throw new Error("BrowserWindow webPreferences must be an object") + + const flags = new Map() + for (const name of ["sandbox", "nodeIntegration", "contextIsolation"]) { + const value = uniqueProperty(preferences, name).initializer + if (value.kind !== ts.SyntaxKind.TrueKeyword && value.kind !== ts.SyntaxKind.FalseKeyword) throw new Error(`${name} must be a boolean literal`) + flags.set(name, value.kind === ts.SyntaxKind.TrueKeyword) + } + return flags +} + +function sessionHandler(ast: ts.SourceFile, method: string): ts.ArrowFunction { + const calls = findNodes(ast, (node): node is ts.CallExpression => { + if (!ts.isCallExpression(node) || !ts.isPropertyAccessExpression(node.expression) || node.expression.name.text !== method) return false + const session = node.expression.expression + return ts.isPropertyAccessExpression(session) && session.name.text === "defaultSession" && ts.isIdentifier(session.expression) && session.expression.text === "session" + }) + if (calls.length !== 1 || calls[0] === undefined) throw new Error(`Expected exactly one session.defaultSession.${method} call, found ${calls.length}`) + return functionHandler(calls[0], method) +} + +function assertPermissionRequestHandlerDenies(source: string): void { + const ast = sourceAst(source, "permission-request-fixture.ts") + const handler = sessionHandler(ast, "setPermissionRequestHandler") + const callbackParameter = handler.parameters[2]?.name + if (callbackParameter === undefined || !ts.isIdentifier(callbackParameter)) throw new Error("The permission request callback must be the third handler parameter") + + const name = callbackParameter.text + const isDenial = (expression: ts.Expression): boolean => + ts.isCallExpression(expression) && + ts.isIdentifier(expression.expression) && + expression.expression.text === name && + expression.arguments.length === 1 && + expression.arguments[0]?.kind === ts.SyntaxKind.FalseKeyword + + const callbackCalls = findNodes(handler.body, (node): node is ts.CallExpression => { + if (!ts.isCallExpression(node) || !ts.isIdentifier(node.expression) || node.expression.text !== name) return false + return true + }) + if (callbackCalls.length === 0) throw new Error(`setPermissionRequestHandler: permission request callback ${name} is not called`) + for (const callbackCall of callbackCalls) { + if (!isDenial(callbackCall)) throw new Error(`setPermissionRequestHandler: permission request callback ${name} must be called with false`) + } + // The denial here is the call itself, so only a call expression counts: a + // bare `false`, or a mention of the callback that never calls it, answers + // nothing. + if (!directExpressions(handler.body).some((direct) => isDenial(direct.expression))) + throw new Error( + `setPermissionRequestHandler: permission request callback ${name} must be called with false as a reachable direct statement of the handler body, not from a branch, a ternary, a try, a loop, a nested function or after an earlier exit` + ) +} + +function assertPermissionCheckHandlerDenies(source: string): void { + const ast = sourceAst(source, "permission-check-fixture.ts") + const handler = sessionHandler(ast, "setPermissionCheckHandler") + const returns = findNodes(handler.body, ts.isReturnStatement) + if (returns.length > 1) throw new Error(`setPermissionCheckHandler: permission check handler must return false and nothing else, found ${returns.length} return statements`) + // Electron reads the value this handler produces, so only the expression body + // of `() => false` or a reachable `return false` denies. `{ false }` returns + // undefined and `{ false; return true }` grants, and both used to pass. + if (!directExpressions(handler.body).some((direct) => direct.isAnswer && direct.expression.kind === ts.SyntaxKind.FalseKeyword)) + throw new Error( + "setPermissionCheckHandler: permission check handler must return false, as the expression body of the handler or as a reachable direct statement of the handler body, not from a branch, a ternary, a try, a loop, a nested function, an expression statement that discards it or after an earlier exit" + ) +} + +function assertWindowOpenHandlerDenies(source: string): void { + const ast = sourceAst(source, "window-open-fixture.ts") + const handler = webContentsHandler(ast, "setWindowOpenHandler") + // One object return only, wherever it sits, so a second one cannot allow the + // window before the denial the pin reads. + const returns = findNodes(handler.body, ts.isReturnStatement) + if (returns.length > 1) throw new Error(`setWindowOpenHandler: window open handler must have exactly one object return, found ${returns.length} return statements`) + + // The denial is the value the handler answers with, so a bare + // `({ action: "deny" })` expression statement is not one. + const directDenial = directExpressions(handler.body).find((direct) => direct.isAnswer && ts.isObjectLiteralExpression(direct.expression)) + const nestedReturn = returns[0]?.expression + const object = directDenial?.expression ?? (nestedReturn !== undefined && ts.isObjectLiteralExpression(nestedReturn) ? nestedReturn : undefined) + if (object === undefined || !ts.isObjectLiteralExpression(object)) throw new Error("setWindowOpenHandler: window open handler must have exactly one object return") + + const action = uniqueProperty(object, "action").initializer + if (!ts.isStringLiteral(action) || action.text !== "deny") throw new Error("setWindowOpenHandler: window open handler must return action deny") + if (directDenial === undefined) + throw new Error( + "setWindowOpenHandler: window open handler must return action deny, as the expression body of the handler or as a reachable direct statement of the handler body, not from a branch, a ternary, a try, a loop, a nested function or after an earlier exit" + ) +} + +/** + * The renderer defenses the main process holds on its own side of the bridge. + * None of them can be exercised for real here: index.ts bootstraps Electron on + * import, and the only harness that drives a rendered window is the packaged CDP + * run under tests/e2e, which is started by hand from a workflow rather than by + * vitest. The assertions therefore parse the TypeScript structure without + * importing the Electron bootstrap. This ignores comments and dead text while + * requiring the security calls to appear in the executable handler bodies. + */ +describe("main process renderer defenses", () => { + it("builds the main window with the renderer locked out of node", () => { + const flags = mainWindowFlags() + + assert.equal(flags.get("sandbox"), true, `the main window webPreferences set sandbox: ${flags.get("sandbox")}, which drops the renderer out of the OS sandbox`) + assert.equal(flags.get("nodeIntegration"), false, `the main window webPreferences set nodeIntegration: ${flags.get("nodeIntegration")}, which hands the renderer require()`) + assert.equal(flags.get("contextIsolation"), true, `the main window webPreferences set contextIsolation: ${flags.get("contextIsolation")}, which puts the preload bridge in the page's own world`) + }) + + it("blocks any main-frame navigation the renderer policy rejects", () => { + // will-navigate alone is not the boundary: a redirect and a frame + // navigation reach the same window through their own events, so all three + // guards are pinned together. + for (const event of ["will-navigate", "will-redirect"]) { + assert.doesNotThrow(() => assertNavigationHandler(MAIN_SOURCE, event), `the ${event} guard no longer prevents a URL the renderer policy rejects`) + } + }) + + it("requires will-frame-navigate to block an unsafe main-frame URL", () => { + assert.doesNotThrow(() => assertNavigationHandler(MAIN_SOURCE, "will-frame-navigate"), "the will-frame-navigate guard no longer blocks unsafe main-frame URLs") + }) + + it("rejects navigation guards hidden in comments, dead branches, or weakened conditions", () => { + const commentOnly = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { /* if (!isAllowedMainFrameUrl(url)) event.preventDefault() */ }) }` + const deadBranch = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { if (false) { if (!isAllowedMainFrameUrl(url)) event.preventDefault() } }) }` + const weakenedCondition = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { if (!isAllowedMainFrameUrl(url) && false) event.preventDefault() }) }` + const frameWithoutMainFrameCheck = `function createWindow() { mainWindow.webContents.on("will-frame-navigate", (details) => { if (!isAllowedMainFrameUrl(details.url)) details.preventDefault() }) }` + // The guard sits at the top level of the body, so a dead branch around it is + // caught, but an early return in front of it left it unreachable and green. + const unreachableGuard = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { return; if (!isAllowedMainFrameUrl(url)) event.preventDefault() }) }` + const guardInsideTry = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { try { if (!isAllowedMainFrameUrl(url)) event.preventDefault() } catch {} }) }` + + assert.throws(() => assertNavigationHandler(commentOnly, "will-navigate")) + assert.throws(() => assertNavigationHandler(deadBranch, "will-navigate")) + assert.throws(() => assertNavigationHandler(weakenedCondition, "will-navigate")) + assert.throws(() => assertNavigationHandler(frameWithoutMainFrameCheck, "will-frame-navigate")) + assert.throws(() => assertNavigationHandler(unreachableGuard, "will-navigate"), /exactly one direct guard/) + assert.throws(() => assertNavigationHandler(guardInsideTry, "will-navigate"), /exactly one direct guard/) + }) + + it("denies every window the renderer asks Electron to open", () => { + assert.doesNotThrow(() => assertWindowOpenHandlerDenies(MAIN_SOURCE), "the window open handler stopped denying renderer-created windows") + }) + + it("rejects an early allow before the final window-open denial", () => { + const weakenedSource = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { if (details.url) return { action: "allow" }; return { action: "deny" } }) }` + + assert.throws(() => assertWindowOpenHandlerDenies(weakenedSource), /exactly one object return/) + }) + + it("refuses every renderer permission request", () => { + assertPermissionRequestHandlerDenies(MAIN_SOURCE) + assertPermissionCheckHandlerDenies(MAIN_SOURCE) + }) + + it("binds permission denial to the third callback parameter", () => { + const fourParameterHandler = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback, extra) => callback(false))" + assert.doesNotThrow(() => assertPermissionRequestHandlerDenies(fourParameterHandler)) + }) + + it("rejects permission callbacks hidden by comments, dead branches, or another false call", () => { + const commentOnly = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { /* callback(false) */ })" + const deadBranch = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { if (false) callback(true) })" + const anotherFalseCall = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { log(false); callback(true) })" + const weakCheck = "session.defaultSession.setPermissionCheckHandler(() => true)" + + assert.throws(() => assertPermissionRequestHandlerDenies(commentOnly), /is not called/) + assert.throws(() => assertPermissionRequestHandlerDenies(deadBranch), /must be called with false/) + assert.throws(() => assertPermissionRequestHandlerDenies(anotherFalseCall), /must be called with false/) + assert.throws(() => assertPermissionCheckHandlerDenies(weakCheck), /must return false/) + }) + + // A denial the runtime can step over is not a denial. Everything below calls + // the callback with false, or returns false, somewhere in the handler, and + // every one of them leaves a permission request unanswered or answered later + // by something else. + it("rejects a permission request denial the handler can skip", () => { + const deadBranch = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { if (false) callback(false) })" + const insideTry = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { try { callback(false) } catch {} })" + const ternaryWithAllow = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { granted ? callback(false) : callback(true) })" + const ternaryDenial = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { granted ? callback(false) : callback(false) })" + const nestedFunction = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { const deny = () => callback(false) })" + const emptyBody = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => {})" + + assert.throws(() => assertPermissionRequestHandlerDenies(deadBranch), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(insideTry), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(ternaryWithAllow), /must be called with false/) + assert.throws(() => assertPermissionRequestHandlerDenies(ternaryDenial), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(nestedFunction), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(emptyBody), /is not called/) + }) + + it("rejects a permission check or window-open denial the handler can skip", () => { + const checkInBranch = "session.defaultSession.setPermissionCheckHandler(() => { if (false) return false })" + const checkInTry = "session.defaultSession.setPermissionCheckHandler(() => { try { return false } catch {} })" + const checkTernary = "session.defaultSession.setPermissionCheckHandler(() => (granted ? false : false))" + const checkNestedFunction = "session.defaultSession.setPermissionCheckHandler(() => { const deny = () => false })" + const checkAfterAllow = "session.defaultSession.setPermissionCheckHandler(() => { if (granted) return true; return false })" + const checkEmptyBody = "session.defaultSession.setPermissionCheckHandler(() => {})" + const openInBranch = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { if (details.url) { return { action: "deny" } } }) }` + const openInTry = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { try { return { action: "deny" } } catch {} }) }` + const openUnreachable = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { throw new Error("unreachable"); return { action: "deny" } }) }` + const openEmptyBody = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => {}) }` + + assert.throws(() => assertPermissionCheckHandlerDenies(checkInBranch), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkInTry), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkTernary), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkNestedFunction), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkAfterAllow), /found 2 return statements/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkEmptyBody), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openInBranch), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openInTry), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openUnreachable), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openEmptyBody), /exactly one object return/) + }) + + // Evaluating a value and throwing it away is not answering with it. Both of + // these used to satisfy the contract: the first hands Electron undefined, the + // second hands it true, and Electron reads what the handler returns. + it("rejects a denial written as an expression statement that discards it", () => { + const checkDiscarded = "session.defaultSession.setPermissionCheckHandler(() => { false })" + const checkDiscardedThenAllow = "session.defaultSession.setPermissionCheckHandler(() => { false; return true })" + const requestBareFalse = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { false })" + const requestCallbackUnused = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { callback })" + const openDiscarded = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { ({ action: "deny" }) }) }` + + assert.throws(() => assertPermissionCheckHandlerDenies(checkDiscarded), /must return false/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkDiscardedThenAllow), /must return false/) + assert.throws(() => assertPermissionRequestHandlerDenies(requestBareFalse), /is not called/) + assert.throws(() => assertPermissionRequestHandlerDenies(requestCallbackUnused), /is not called/) + assert.throws(() => assertWindowOpenHandlerDenies(openDiscarded), /exactly one object return/) + }) + + // A denial written after something that can leave the handler runs on some + // paths and not others. reachableStatements used to stop only at an exit + // written directly in the block, so every shape below reached its denial in + // the pin and skipped it at runtime. + it("rejects a denial reached only when an earlier statement does not leave the handler", () => { + const requestAfterConditionalReturn = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { if (granted) return; callback(false) })" + const requestAfterConditionalThrow = 'session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { if (granted) throw new Error("no"); callback(false) })' + const requestAfterTryExit = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { try { return } catch {} callback(false) })" + const requestAfterLoopExit = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => { for (const p of pending) { return } callback(false) })" + + const checkAfterConditionalThrow = 'session.defaultSession.setPermissionCheckHandler(() => { if (granted) throw new Error("no"); return false })' + const checkAfterTryExit = 'session.defaultSession.setPermissionCheckHandler(() => { try { throw new Error("no") } catch {} return false })' + const checkAfterLoopExit = "session.defaultSession.setPermissionCheckHandler(() => { for (const p of pending) { return true } return false })" + + const navAfterConditionalReturn = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { if (trusted) return; if (!isAllowedMainFrameUrl(url)) event.preventDefault() }) }` + const navAfterConditionalThrow = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { if (trusted) throw new Error("no"); if (!isAllowedMainFrameUrl(url)) event.preventDefault() }) }` + const navAfterTryExit = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { try { return } catch {} if (!isAllowedMainFrameUrl(url)) event.preventDefault() }) }` + const navAfterLoopExit = `function createWindow() { mainWindow.webContents.on("will-navigate", (event, url) => { for (const frame of frames) { return } if (!isAllowedMainFrameUrl(url)) event.preventDefault() }) }` + + const openAfterConditionalThrow = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { if (details.url) throw new Error("no"); return { action: "deny" } }) }` + const openAfterTryExit = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { try { throw new Error("no") } catch {} return { action: "deny" } }) }` + const openAfterLoopExit = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { for (const feature of details.features) { throw new Error("no") } return { action: "deny" } }) }` + // A conditional return before the denial puts a second return in the + // handler, which the one-object-return rule rejects first. + const openAfterConditionalReturn = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => { if (details.url) return; return { action: "deny" } }) }` + + assert.throws(() => assertPermissionRequestHandlerDenies(requestAfterConditionalReturn), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(requestAfterConditionalThrow), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(requestAfterTryExit), /direct statement of the handler body/) + assert.throws(() => assertPermissionRequestHandlerDenies(requestAfterLoopExit), /direct statement of the handler body/) + + assert.throws(() => assertPermissionCheckHandlerDenies(checkAfterConditionalThrow), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkAfterTryExit), /direct statement of the handler body/) + assert.throws(() => assertPermissionCheckHandlerDenies(checkAfterLoopExit), /found 2 return statements/) + + assert.throws(() => assertNavigationHandler(navAfterConditionalReturn, "will-navigate"), /must prevent a rejected URL/) + assert.throws(() => assertNavigationHandler(navAfterConditionalThrow, "will-navigate"), /must prevent a rejected URL/) + assert.throws(() => assertNavigationHandler(navAfterTryExit, "will-navigate"), /exactly one direct guard/) + assert.throws(() => assertNavigationHandler(navAfterLoopExit, "will-navigate"), /exactly one direct guard/) + + assert.throws(() => assertWindowOpenHandlerDenies(openAfterConditionalThrow), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openAfterTryExit), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openAfterLoopExit), /direct statement of the handler body/) + assert.throws(() => assertWindowOpenHandlerDenies(openAfterConditionalReturn), /exactly one object return/) + }) + + // The rule is about where the denial sits, not about how it is spelled, so a + // rename and a reflow of the real shapes stay accepted. + it("accepts the shipped denials after a rename and a reformat", () => { + const renamedRequest = `session.defaultSession.setPermissionRequestHandler((contents, requested, respond) => { + respond(false) + })` + const renamedCheck = `session.defaultSession.setPermissionCheckHandler(() => { + return false + })` + const renamedOpen = `function createWindow() { + mainWindow.webContents.setWindowOpenHandler((request) => { + try { + const safeUrl = assertAllowedBrowserUrl(request.url) + void shell.openExternal(safeUrl.toString()) + } catch { + logMessage("warn", "Blocked an unsafe external window URL.") + } + return { action: "deny" } + }) + }` + + assert.doesNotThrow(() => assertPermissionRequestHandlerDenies(renamedRequest)) + assert.doesNotThrow(() => assertPermissionCheckHandlerDenies(renamedCheck)) + assert.doesNotThrow(() => assertWindowOpenHandlerDenies(renamedOpen)) + }) + + // An expression body answers with its value, so it is the same denial written + // shorter, and the check handler already ships that way. + it("accepts an expression-bodied denial in each handler", () => { + const expressionRequest = "session.defaultSession.setPermissionRequestHandler((_webContents, _permission, callback) => callback(false))" + const expressionCheck = "session.defaultSession.setPermissionCheckHandler(() => false)" + const expressionOpen = `function createWindow() { mainWindow.webContents.setWindowOpenHandler((details) => ({ action: "deny" })) }` + + assert.doesNotThrow(() => assertPermissionRequestHandlerDenies(expressionRequest)) + assert.doesNotThrow(() => assertPermissionCheckHandlerDenies(expressionCheck)) + assert.doesNotThrow(() => assertWindowOpenHandlerDenies(expressionOpen)) + }) +}) + describe("main process protocol boundary wiring", () => { // index.ts bootstraps Electron on import, so keep this contract scoped to each inline handler. it("routes app and custom icon requests through containment before file checks", () => { @@ -199,6 +807,16 @@ describe("main process protocol boundary wiring", () => { }) }) +function mainHandlerSource(startMarker: string, endMarker: string): string { + const start = MAIN_SOURCE.indexOf(startMarker) + if (start === -1) throw new Error(`Could not find main-process handler: ${startMarker}`) + + const end = MAIN_SOURCE.indexOf(endMarker, start + startMarker.length) + if (end === -1) throw new Error(`Could not find end of main-process handler: ${startMarker}`) + + return MAIN_SOURCE.slice(start, end) +} + /** * Two renderer trees are held off the preload bridge. Shared components reach the host only * through a feature they were handed, and the mods feature keeps its bridge calls in