From d393f76c3dd999000b605b388c23a32bc127bdd5 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:29:08 +0200 Subject: [PATCH 1/6] Index the logger by mode instead of switching on it The five modes in ErrorTypes are electron-log's own method names, so the switch spelled out the mapping it was indexing. The optional call keeps the old default branch: an unknown mode reads as an absent property and the line is dropped rather than throwing. The redaction is unchanged. --- src/utils/logManager.ts | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/src/utils/logManager.ts b/src/utils/logManager.ts index 47f59b48..c63c0af4 100644 --- a/src/utils/logManager.ts +++ b/src/utils/logManager.ts @@ -14,26 +14,11 @@ export function getErrorMessage(error: unknown): string { return redactSensitiveText(error.message) } +/** + * The five modes are electron-log's own method names, so the mode indexes the logger directly. + * The optional call is the old `default:` branch: a mode that slipped past the type reads as an + * absent property and the line is dropped, rather than throwing inside a log call. + */ export function logMessage(mode: ErrorTypes, message: string): void { - const safeMessage = redactSensitiveText(message) - - switch (mode) { - case "error": - Logger.error(safeMessage) - break - case "warn": - Logger.warn(safeMessage) - break - case "info": - Logger.info(safeMessage) - break - case "debug": - Logger.debug(safeMessage) - break - case "verbose": - Logger.verbose(safeMessage) - break - default: - break - } + Logger[mode]?.(redactSensitiveText(message)) } From fa84ac9a425dc9642f52315a69ebf9471058acb8 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:29:32 +0200 Subject: [PATCH 2/6] Collapse the duplicated gameVersionId arms in validateGameInstallation Two of the four arms were the same expression, left over from the fix that made an invalid id throw instead of being dropped. One undefined test decides whether the key is written, and the value is null or whatever assertString accepts. The three outcomes the boundary owes its callers are unchanged. --- src/ipc/validation.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/ipc/validation.ts b/src/ipc/validation.ts index fde6582b..8de7630f 100644 --- a/src/ipc/validation.ts +++ b/src/ipc/validation.ts @@ -300,13 +300,10 @@ export function validateGameInstallation(value: unknown): Pick Date: Sun, 20 Sep 2026 13:30:30 +0200 Subject: [PATCH 3/6] Write the normalized config straight out instead of re-cleaning it writeConfig ran the whole document through JSON.parse(JSON.stringify(...)) to drop underscore keys, which cost a second full serialise of installations, versions, backups, icons and accounts on every save. The only value that ever reaches the writer is a normalizeConfig result, and that builds a fixed object literal field by field, so the renderer's session-only markers are already gone by then. The guard predates the normaliser. The configManager test now asserts the invariant on normalizeConfig as well as on the file, so it fails where the invariant actually lives. --- src/config/configManager.ts | 15 ++++----------- tests/ipc/configManager.test.ts | 9 ++++++++- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/config/configManager.ts b/src/config/configManager.ts index b238705e..00a0d154 100644 --- a/src/config/configManager.ts +++ b/src/config/configManager.ts @@ -47,16 +47,6 @@ let configWriteQueue: Promise = Promise.resolve() let pendingConfig: ConfigType | null = null let scheduledConfigWrite: Promise | null = null -async function writeConfig(normalizedConfig: ConfigType): Promise { - const cleanedConfig = JSON.parse( - JSON.stringify(normalizedConfig, (key, value) => { - return key.startsWith("_") ? undefined : value - }) - ) - - await writeJsonAtomic(configPath, cleanedConfig) -} - function scheduleConfigWrite(): Promise { // Compared against null rather than tested for truthiness: the question is whether a write is already scheduled, not whether a promise is truthy (it always is). if (scheduledConfigWrite !== null) return scheduledConfigWrite @@ -67,7 +57,10 @@ function scheduleConfigWrite(): Promise { while (pendingConfig) { const nextConfig = pendingConfig pendingConfig = null - await writeConfig(nextConfig) + // Written as it stands: the only thing that ever reaches here is a normalizeConfig result, + // and that builds a fixed literal field by field, so the renderer's session-only markers + // (`_notifiedModUpdatesInstallations`, `_backgroundRevision`) are already gone. + await writeJsonAtomic(configPath, nextConfig) } }) diff --git a/tests/ipc/configManager.test.ts b/tests/ipc/configManager.test.ts index fd8517d9..697cbd96 100644 --- a/tests/ipc/configManager.test.ts +++ b/tests/ipc/configManager.test.ts @@ -825,10 +825,17 @@ describe("saveConfig and flushConfigWrites", () => { assert.equal(result, true, "the write itself landed; only its own best-effort cleanup failed") }) + /** + * The normaliser is what drops these, not the writer: it builds a fixed literal field by field, + * so a key it does not name cannot come out the other side. Asserted on both, because the day + * `normalizeConfig` grows a spread of its input is the day a session-only marker reaches disk. + */ it("strips underscore-prefixed session-only fields before writing to disk", async () => { - const { saveConfig, flushConfigWrites } = await freshConfigManager() + const { saveConfig, flushConfigWrites, normalizeConfig } = await freshConfigManager() const withSessionField = { ...minimalConfig(), _notifiedModUpdatesInstallations: ["install-1"] } + assert.equal("_notifiedModUpdatesInstallations" in normalizeConfig(withSessionField), false) + await saveConfig(withSessionField) await flushConfigWrites() From a839025abfe8ca87dd2094f2931ad78b56b2d71b Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:38:36 +0200 Subject: [PATCH 4/6] Walk the permissions tree on the main thread instead of in a worker CHANGE_PERMS spun a worker thread to run existsSync/lstat/readdir/chmod over a folder tree. That is I/O, not CPU: the other four workers stream or decode and belong in a thread, this one paid for a worker script, a ?modulePath import, two table entries (one of them 0, with a comment saying pooling bought nothing) and the whole message protocol for one call fired once per Linux install. changePermissions is now async over node:fs/promises, which satisfies the filesystem port as it stands, so the delegating nodeFileSystem object goes with it. The existsSync test in front of each lstat is gone too: a stat the filesystem refuses is the same skip with no window between the two answers. The worker's 10 minute bound comes back as an AbortSignal.timeout the walk checks before each entry, which stops the walk rather than orphaning a thread, and the handler still reports the same two error texts the worker reported. The symlink refusal and the 100,000 entry cap are untouched. permissions.ts moves out of src/ipc/workers/ since no worker runs it now. --- src/ipc/handlers/pathsHandlers.ts | 39 +++++++++++---- src/ipc/permissions.ts | 75 ++++++++++++++++++++++++++++ src/ipc/workers/changePermsWorker.ts | 10 ---- src/ipc/workers/permissions.ts | 75 ---------------------------- src/ipc/workers/workerHost.ts | 8 +-- tests/ipc/pathsHandlers.test.ts | 33 ++++++++---- tests/ipc/pathsHandlersWin32.test.ts | 3 +- tests/ipc/permissions.test.ts | 58 ++++++++++++--------- 8 files changed, 165 insertions(+), 136 deletions(-) create mode 100644 src/ipc/permissions.ts delete mode 100644 src/ipc/workers/changePermsWorker.ts delete mode 100644 src/ipc/workers/permissions.ts diff --git a/src/ipc/handlers/pathsHandlers.ts b/src/ipc/handlers/pathsHandlers.ts index a60e627c..10d4c8af 100644 --- a/src/ipc/handlers/pathsHandlers.ts +++ b/src/ipc/handlers/pathsHandlers.ts @@ -15,6 +15,7 @@ import type { WorkerDisposition } from "@src/ipc/workerManager" import { ConcurrencyLimiter } from "@domain/concurrencyLimiter" import { assertAllowedDownloadUrl, assertBoolean, assertInteger, assertPath, assertSafeFileName, assertSafeTaskId, comparablePath, isRecord, MAX_CUSTOM_ICON_BYTES } from "@src/ipc/validation" import { assertManagedDeletionPath, assertManagedPath } from "@src/ipc/pathPolicy" +import { changePermissions } from "@src/ipc/permissions" import { getConfig } from "@src/config/configManager" import { assertVerifiedArtifact, getTrustedDownloadHash, recordVerifiedArtifact } from "@src/ipc/artifactVerification" import { getCachedOptimumManifest, getTrustedOverlayHash } from "@src/ipc/optimumManifest" @@ -25,14 +26,12 @@ import { DEFAULT_COMPRESSION_LEVEL } from "@domain/config/defaults" import compressWorker from "@src/ipc/workers/compressWorker?modulePath" import extractWorker from "@src/ipc/workers/extractWorker?modulePath" import innoExtractWorker from "@src/ipc/workers/innoExtractWorker?modulePath" -import changePermsWorker from "@src/ipc/workers/changePermsWorker?modulePath" import downloadWorkerPath from "@src/ipc/workers/downloadWorker?modulePath" const WORKER_TIMEOUTS_MS: Record = { DOWNLOAD_ON_PATH: 45 * 60 * 1_000, EXTRACT_ON_PATH: 30 * 60 * 1_000, COMPRESS_ON_PATH: 30 * 60 * 1_000, - CHANGE_PERMS: 10 * 60 * 1_000, // Reading the payload out of the Windows installer. Measured at 41 seconds // for the 598 MB installer of 1.22.6 on a developer machine, so this leaves // room for a slow disk without leaving a stuck worker running for an hour. @@ -69,6 +68,15 @@ const EXTRACT_INSTALLER_PAYLOAD = true */ const RUN_INSTALLER_TIMEOUT_MS = 15 * 60 * 1_000 +/** + * Same standing as RUN_INSTALLER_TIMEOUT_MS: CHANGE_PERMS walks the tree on this thread + * rather than in a worker, so it is not the pool's timeout that bounds it. The value is the + * one the table used to carry. The entry cap in changePermissions bounds how many entries + * the walk may touch; this bounds how long it may take to touch them, which is the case a + * filesystem that stops answering produces. + */ +const CHANGE_PERMS_TIMEOUT_MS = 10 * 60 * 1_000 + /** * Nothing capped how many DOWNLOAD_ON_PATH/EXTRACT_ON_PATH/COMPRESS_ON_PATH calls the * renderer could fire at once (a mod update-all, say): each one spun up its own worker @@ -114,19 +122,18 @@ app.on("before-quit", () => { * compression share a two-slot archive lane but use different worker scripts, so one idle * worker per archive operation keeps the combined idle count within that shared limit. * - * CHANGE_PERMS and RUN_INSTALLER are 0 on purpose. Both run once per install with no burst - * behind them, so pooling either would buy one saved worker spawn per game install and pay - * for it with a resident idle isolate. They still go through the pooled protocol below so - * there is only one worker protocol in the app; 0 just means every release terminates, - * exactly like before this file started pooling anything. RUN_INSTALLER joining the archive - * lane leaves those sums alone for the same reason: at 0 it never contributes an idle worker - * to weigh against the two-slot limit. + * RUN_INSTALLER is 0 on purpose. It runs once per install with no burst behind it, so + * pooling it would buy one saved worker spawn per game install and pay for it with a + * resident idle isolate. It still goes through the pooled protocol below so there is only + * one worker protocol in the app; 0 just means every release terminates, exactly like + * before this file started pooling anything. RUN_INSTALLER joining the archive lane leaves + * those sums alone for the same reason: at 0 it never contributes an idle worker to weigh + * against the two-slot limit. */ const WORKER_POOL_MAX_IDLE: Record = { DOWNLOAD_ON_PATH: DOWNLOAD_CONCURRENCY_LIMIT, EXTRACT_ON_PATH: 1, COMPRESS_ON_PATH: 1, - CHANGE_PERMS: 0, RUN_INSTALLER: 0 } @@ -657,8 +664,18 @@ ipcMain.handle(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS, async (event, paths: str const safePaths = await Promise.all(paths.map((pathValue) => assertManagedPath(pathValue, "permissions path"))) const safePerms = assertInteger(perms, "permissions", 0, 0o777) + const timeout = AbortSignal.timeout(CHANGE_PERMS_TIMEOUT_MS) + + try { + await changePermissions({ paths: safePaths, perms: safePerms, signal: timeout }) + } catch (err) { + // Same two texts the pooled worker reported, so the renderer's extract task reads a + // refusal exactly as it did. The reason behind them used to be dropped on the way out + // of the worker; at debug it is there for whoever reads the log afterwards. + logMessage("debug", `[back] [ipc] [ipc/handlers/pathsHandlers.ts] [CHANGE_PERMS] ${getErrorMessage(err)}`) + throw new Error(timeout.aborted ? "CHANGE_PERMS timed out" : "Changing permissions failed") + } - await runTrackedWorker(event, "permissions", undefined, changePermsWorker, { paths: safePaths, perms: safePerms }, "CHANGE_PERMS", () => true) return true }) diff --git a/src/ipc/permissions.ts b/src/ipc/permissions.ts new file mode 100644 index 00000000..e1fcb8a7 --- /dev/null +++ b/src/ipc/permissions.ts @@ -0,0 +1,75 @@ +/** + * Applying a permission bit to a folder tree. + * + * This exists for the Linux game folder, whose executables come out of the + * archive without the execute bit. A symbolic link inside the tree stops the + * whole run rather than being followed: chmod resolves links, so following one + * would apply the launcher's bits to a file outside the folder the user picked. + * + * The walk is I/O and nothing else, so it runs on the main thread's event loop + * rather than in a worker: every step is an awaited syscall, and none of them + * holds the loop. Nothing here touches Electron. + */ + +import { chmod, lstat, readdir } from "node:fs/promises" +import { join } from "node:path" + +const MAX_ITEMS = 100_000 + +/** The slice of the filesystem this walk needs, so a test can stand in a fake tree. `node:fs/promises` satisfies it as it is. */ +export interface PermissionsFileSystem { + lstat(path: string): Promise<{ isSymbolicLink(): boolean; isDirectory(): boolean; isFile(): boolean }> + readdir(path: string): Promise + chmod(path: string, mode: number): Promise +} + +export interface ChangePermissionsOptions { + /** Roots to walk. Each is applied to itself and to everything beneath it. */ + paths: readonly string[] + /** Mode passed straight to `chmod`. */ + perms: number + /** Stops the walk before its next entry. The caller's bound on a tree that never ends. */ + signal?: AbortSignal + /** Filesystem to act on, defaulting to the real one. */ + fileSystem?: PermissionsFileSystem +} + +/** + * Applies `perms` to every path given and to everything under it. + * + * A path that cannot be stat'd is skipped, which is what lets a caller pass the + * union of the paths a Linux install might use without checking each one first. + * + * @param options Roots, mode, abort signal, and the filesystem to act on. + * @throws On a symbolic link, on an entry that is neither a file nor a folder, + * once the tree passes the entry cap, and on an aborted signal. Nothing is + * rolled back: the caller reports the failure and the bits already applied stay + * applied. + */ +export async function changePermissions(options: ChangePermissionsOptions): Promise { + const { paths, perms, signal, fileSystem = { lstat, readdir, chmod } } = options + let itemCount = 0 + + const visit = async (path: string): Promise => { + signal?.throwIfAborted() + + // Anything the filesystem will not describe is skipped rather than refused, missing paths + // included: the caller passes the union of the paths a Linux install might use and lets + // the walk sort out which of them are there. + const stats = await fileSystem.lstat(path).catch(() => null) + if (!stats) return + + if (stats.isSymbolicLink()) throw new Error("Symbolic links are not allowed") + itemCount++ + if (itemCount > MAX_ITEMS) throw new Error("Too many filesystem entries") + + if (stats.isDirectory()) { + for (const item of await fileSystem.readdir(path)) await visit(join(path, item)) + } + + if (!stats.isDirectory() && !stats.isFile()) throw new Error("Unsupported filesystem entry") + await fileSystem.chmod(path, perms) + } + + for (const path of paths) await visit(path) +} diff --git a/src/ipc/workers/changePermsWorker.ts b/src/ipc/workers/changePermsWorker.ts deleted file mode 100644 index a629aca7..00000000 --- a/src/ipc/workers/changePermsWorker.ts +++ /dev/null @@ -1,10 +0,0 @@ -import { serveTasks } from "@src/ipc/workers/workerHost" -import { changePermissions } from "@src/ipc/workers/permissions" - -serveTasks( - (payload) => { - const { paths, perms } = payload as { paths: readonly string[]; perms: number } - changePermissions({ paths, perms }) - }, - () => "Changing permissions failed" -) diff --git a/src/ipc/workers/permissions.ts b/src/ipc/workers/permissions.ts deleted file mode 100644 index 463415f8..00000000 --- a/src/ipc/workers/permissions.ts +++ /dev/null @@ -1,75 +0,0 @@ -/** - * Applying a permission bit to a folder tree, without the worker plumbing. - * - * The worker thread is a shim over this module, the same split extraction.ts - * and innoExtraction.ts already use, so the same code can be driven from a test - * or a script. Nothing here touches Electron or `worker_threads`. - * - * This exists for the Linux game folder, whose executables come out of the - * archive without the execute bit. A symbolic link inside the tree stops the - * whole run rather than being followed: chmod resolves links, so following one - * would apply the launcher's bits to a file outside the folder the user picked. - */ - -import fse from "fs-extra" -import { join } from "node:path" - -const MAX_ITEMS = 100_000 - -/** The slice of the filesystem this walk needs, so a test can stand in a fake tree. */ -export interface PermissionsFileSystem { - existsSync(path: string): boolean - lstatSync(path: string): { isSymbolicLink(): boolean; isDirectory(): boolean; isFile(): boolean } - readdirSync(path: string): string[] - chmodSync(path: string, mode: number): void -} - -const nodeFileSystem: PermissionsFileSystem = { - existsSync: (path) => fse.existsSync(path), - lstatSync: (path) => fse.lstatSync(path), - readdirSync: (path) => fse.readdirSync(path), - chmodSync: (path, mode) => fse.chmodSync(path, mode) -} - -export interface ChangePermissionsOptions { - /** Roots to walk. Each is applied to itself and to everything beneath it. */ - paths: readonly string[] - /** Mode passed straight to `chmod`. */ - perms: number - /** Filesystem to act on, defaulting to the real one. */ - fileSystem?: PermissionsFileSystem -} - -/** - * Applies `perms` to every path given and to everything under it. - * - * A path that does not exist is skipped, which is what lets a caller pass the - * union of the paths a Linux install might use without checking each one first. - * - * @param options Roots, mode, and the filesystem to act on. - * @throws On a symbolic link, on an entry that is neither a file nor a folder, - * and once the tree passes the entry cap. Nothing is rolled back: the caller - * reports the failure and the bits already applied stay applied. - */ -export function changePermissions(options: ChangePermissionsOptions): void { - const { paths, perms, fileSystem = nodeFileSystem } = options - let itemCount = 0 - - const visit = (path: string): void => { - if (!fileSystem.existsSync(path)) return - - const stats = fileSystem.lstatSync(path) - if (stats.isSymbolicLink()) throw new Error("Symbolic links are not allowed") - itemCount++ - if (itemCount > MAX_ITEMS) throw new Error("Too many filesystem entries") - - if (stats.isDirectory()) { - for (const item of fileSystem.readdirSync(path)) visit(join(path, item)) - } - - if (!stats.isDirectory() && !stats.isFile()) throw new Error("Unsupported filesystem entry") - fileSystem.chmodSync(path, perms) - } - - for (const path of paths) visit(path) -} diff --git a/src/ipc/workers/workerHost.ts b/src/ipc/workers/workerHost.ts index 50fa55a4..929fce0f 100644 --- a/src/ipc/workers/workerHost.ts +++ b/src/ipc/workers/workerHost.ts @@ -3,11 +3,11 @@ * `workerData` and lets the thread fall idle so the parent can terminate it: it waits on * `parentPort` for one task at a time and stays alive for the next one. * - * All five worker shims are this function plus a handler, so the protocol lives in exactly + * All four worker shims are this function plus a handler, so the protocol lives in exactly * one place on this side of the port: the token echo that lets the parent tell a live * task's messages from an abandoned one's, the guard that drops a progress report arriving - * after its own task already settled, and the promise-or-value handling that lets the - * synchronous permissions handler share a shape with the four asynchronous ones. + * after its own task already settled, and the promise-or-value handling that accepts a + * handler that answers without awaiting anything. * * Attaching the "message" listener is also what keeps the thread alive between tasks: a * started MessagePort refs the worker's event loop. @@ -25,7 +25,7 @@ export type TaskHandler = (payload: Record, onProgress: Progres /** * Turns a rejection into the message text the parent sees. Per worker on purpose: - * extraction forwards its own reason, the other four report a fixed one, matching what + * extraction forwards its own reason, the other three report a fixed one, matching what * each already did before this file existed. */ export type FailureDescriber = (error: unknown) => string diff --git a/tests/ipc/pathsHandlers.test.ts b/tests/ipc/pathsHandlers.test.ts index e66d4e9c..0af29f6e 100644 --- a/tests/ipc/pathsHandlers.test.ts +++ b/tests/ipc/pathsHandlers.test.ts @@ -1,7 +1,7 @@ import assert from "node:assert/strict" import { EventEmitter } from "node:events" import { execFileSync } from "node:child_process" -import { copyFileSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs" +import { copyFileSync, mkdirSync, mkdtempSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join, resolve as resolvePath, sep } from "node:path" import { afterEach, beforeEach, describe, it, vi } from "vitest" @@ -33,7 +33,7 @@ import { MAX_CUSTOM_ICON_BYTES } from "@src/ipc/validation" * worker_thread or child_process. That includes `runTrackedWorker`'s own * message-handling logic (progress validation, "finished"/"error"/unknown * message shapes, the worker's own "error" event), which DOWNLOAD_ON_PATH/ - * EXTRACT_ON_PATH/COMPRESS_ON_PATH/CHANGE_PERMS all funnel through: + * EXTRACT_ON_PATH/COMPRESS_ON_PATH all funnel through: * `@src/ipc/workerManager` is mocked so `acquireWorker` hands back a lease * wrapping a plain `EventEmitter` a test drives directly instead of a real * `worker_threads.Worker`, which is what runTrackedWorker only ever calls @@ -52,7 +52,6 @@ import { MAX_CUSTOM_ICON_BYTES } from "@src/ipc/validation" vi.mock("@src/ipc/workers/compressWorker?modulePath", () => ({ default: "compressWorker-path" })) vi.mock("@src/ipc/workers/extractWorker?modulePath", () => ({ default: "extractWorker-path" })) vi.mock("@src/ipc/workers/innoExtractWorker?modulePath", () => ({ default: "innoExtractWorker-path" })) -vi.mock("@src/ipc/workers/changePermsWorker?modulePath", () => ({ default: "changePermsWorker-path" })) vi.mock("@src/ipc/workers/downloadWorker?modulePath", () => ({ default: "downloadWorker-path" })) vi.mock("@src/ipc/workerManager", () => ({ @@ -1005,17 +1004,29 @@ describe("COMPRESS_ON_PATH: runTrackedWorker via a fake worker", () => { }) // Same Linux-only early return: on Windows the handler resolves false without -// ever starting a worker, so the fake worker this waits for never arrives. -describe.skipIf(process.platform === "win32")("CHANGE_PERMS: runTrackedWorker via a fake worker", () => { - it("resolves true once the worker finishes", async () => { +// walking anything, so the mode these read back would mean nothing. +describe.skipIf(process.platform === "win32")("CHANGE_PERMS: the walk itself", () => { + it("resolves true once the tree has been walked, and applies the mode", async () => { const event = await createTrustedEvent() - const workerPromise = nextTrackedWorker() - const resultPromise = handler>(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [managedFolder], 0o755) + const target = join(managedFolder, "Vintagestory") + writeFileSync(target, "elf", { mode: 0o600 }) - const worker = await workerPromise - worker.emit("message", { type: "finished" }) + assert.equal(await handler>(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [managedFolder], 0o755), true) - assert.equal(await resultPromise, true) + assert.equal(statSync(target).mode & 0o777, 0o755) + + const { acquireWorker } = await import("@src/ipc/workerManager") + assert.equal(vi.mocked(acquireWorker).mock.calls.length, 0, "the walk runs on this thread, not in a worker") + }) + + // The refusal is changePermissions', and permissions.test.ts pins it there. What this + // adds is the text the renderer's extract task sees, which the pooled worker used to fix + // and the handler now fixes in its place. + it("reports a refusal under one fixed message rather than the reason behind it", async () => { + const event = await createTrustedEvent() + symlinkSync(join(temporaryRoot, "outsider.txt"), join(managedFolder, "shortcut")) + + await assert.rejects(() => handler(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [managedFolder], 0o755), /^Error: Changing permissions failed$/) }) }) diff --git a/tests/ipc/pathsHandlersWin32.test.ts b/tests/ipc/pathsHandlersWin32.test.ts index 68ff1467..8f645068 100644 --- a/tests/ipc/pathsHandlersWin32.test.ts +++ b/tests/ipc/pathsHandlersWin32.test.ts @@ -42,7 +42,6 @@ import { CURRENT_CONFIG_SCHEMA } from "@domain/config/migrations" vi.mock("@src/ipc/workers/compressWorker?modulePath", () => ({ default: "compressWorker-path" })) vi.mock("@src/ipc/workers/extractWorker?modulePath", () => ({ default: "extractWorker-path" })) vi.mock("@src/ipc/workers/innoExtractWorker?modulePath", () => ({ default: "innoExtractWorker-path" })) -vi.mock("@src/ipc/workers/changePermsWorker?modulePath", () => ({ default: "changePermsWorker-path" })) vi.mock("@src/ipc/workers/downloadWorker?modulePath", () => ({ default: "downloadWorker-path" })) vi.mock("@src/ipc/workerManager", () => ({ @@ -543,7 +542,7 @@ describe("RUN_INSTALLER on win32: format-refused falls back to spawning the inst }) describe("CHANGE_PERMS on win32", () => { - it("returns false before any worker runs, since the permissions worker is Linux-only", async () => { + it("returns false before anything is walked, since POSIX mode bits are Linux-only", async () => { const event = await createTrustedEvent() const result = await handler>(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [managedFolder], 0o755) assert.equal(result, false) diff --git a/tests/ipc/permissions.test.ts b/tests/ipc/permissions.test.ts index fc19f4a1..cb0e6230 100644 --- a/tests/ipc/permissions.test.ts +++ b/tests/ipc/permissions.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from "node:os" import { join } from "node:path" import { afterEach, beforeEach, describe, it } from "vitest" -import { changePermissions, type PermissionsFileSystem } from "@src/ipc/workers/permissions" +import { changePermissions, type PermissionsFileSystem } from "@src/ipc/permissions" /** * The Linux permission pass, run against a real temporary tree. @@ -43,10 +43,9 @@ function fakeTree(lstat: (path: string) => FakeStats, readdir: (path: string) => return { chmodCalls, - existsSync: (): boolean => true, - lstatSync: lstat, - readdirSync: readdir, - chmodSync: (path, mode): void => { + lstat: async (path): Promise => lstat(path), + readdir: async (path): Promise => readdir(path), + chmod: async (path, mode): Promise => { chmodCalls.push({ path, mode }) } } @@ -83,8 +82,8 @@ describe("changePermissions", () => { // POSIX mode bits like 0o755 or 0o600, so the mode a real tree ends up with // has nothing to do with what changePermissions asked for. These read the // mode back off disk, so they only mean anything on a POSIX filesystem. - it.skipIf(process.platform === "win32")("applies the mode to the root, its files and everything nested under it", () => { - changePermissions({ paths: [installation], perms: 0o755 }) + it.skipIf(process.platform === "win32")("applies the mode to the root, its files and everything nested under it", async () => { + await changePermissions({ paths: [installation], perms: 0o755 }) assert.equal(modeOf(installation), 0o755) assert.equal(modeOf(installation, "Vintagestory"), 0o755) @@ -92,59 +91,59 @@ describe("changePermissions", () => { assert.equal(modeOf(installation, "assets", "version.txt"), 0o755) }) - it.skipIf(process.platform === "win32")("applies the mode to a single file given directly", () => { - changePermissions({ paths: [join(installation, "Vintagestory")], perms: 0o750 }) + it.skipIf(process.platform === "win32")("applies the mode to a single file given directly", async () => { + await changePermissions({ paths: [join(installation, "Vintagestory")], perms: 0o750 }) assert.equal(modeOf(installation, "Vintagestory"), 0o750) assert.equal(modeOf(installation, "assets", "version.txt"), 0o600) }) - it.skipIf(process.platform === "win32")("walks every root it is given", () => { + it.skipIf(process.platform === "win32")("walks every root it is given", async () => { const second = workspacePath("data") mkdirSync(second) writeFileSync(join(second, "clientsettings.json"), "{}", { mode: 0o600 }) - changePermissions({ paths: [installation, second], perms: 0o755 }) + await changePermissions({ paths: [installation, second], perms: 0o755 }) assert.equal(modeOf(installation, "Vintagestory"), 0o755) assert.equal(modeOf(second, "clientsettings.json"), 0o755) }) - it.skipIf(process.platform === "win32")("skips a path that is not there", () => { - assert.doesNotThrow(() => changePermissions({ paths: [workspacePath("never-installed"), installation], perms: 0o755 })) + it.skipIf(process.platform === "win32")("skips a path that is not there", async () => { + await assert.doesNotReject(() => changePermissions({ paths: [workspacePath("never-installed"), installation], perms: 0o755 })) assert.equal(modeOf(installation, "Vintagestory"), 0o755) }) - it.skipIf(process.platform === "win32")("does nothing at all when given no paths", () => { - changePermissions({ paths: [], perms: 0o755 }) + it.skipIf(process.platform === "win32")("does nothing at all when given no paths", async () => { + await changePermissions({ paths: [], perms: 0o755 }) assert.equal(modeOf(installation, "Vintagestory"), 0o600) }) - it.skipIf(process.platform === "win32")("refuses a symbolic link rather than applying the mode to what it points at", () => { + it.skipIf(process.platform === "win32")("refuses a symbolic link rather than applying the mode to what it points at", async () => { const outsider = workspacePath("outsider.txt") writeFileSync(outsider, "not the launcher's file", { mode: 0o600 }) symlinkSync(outsider, join(installation, "shortcut")) - assert.throws(() => changePermissions({ paths: [installation], perms: 0o777 }), /Symbolic links are not allowed/) + await assert.rejects(() => changePermissions({ paths: [installation], perms: 0o777 }), /Symbolic links are not allowed/) assert.equal(modeOf(outsider), 0o600) }) - it("refuses an entry that is neither a file nor a folder", () => { + it("refuses an entry that is neither a file nor a folder", async () => { const socket = fakeStats("other") const fileSystem = fakeTree( () => socket, () => [] ) - assert.throws(() => changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }), /Unsupported filesystem entry/) + await assert.rejects(() => changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }), /Unsupported filesystem entry/) assert.deepEqual(fileSystem.chmodCalls, []) }) - it("refuses a tree with more entries than the cap allows", () => { + it("refuses a tree with more entries than the cap allows", async () => { // A real tree of 100 001 entries costs more to build than the whole suite // costs to run, so the walk is pointed at a fake one that claims to hold them. const children = Array.from({ length: 100_001 }, (_, index) => `child-${index}`) @@ -153,10 +152,10 @@ describe("changePermissions", () => { () => children ) - assert.throws(() => changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }), /Too many filesystem entries/) + await assert.rejects(() => changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }), /Too many filesystem entries/) }) - it("descends before it touches the folder it descended into", () => { + it("descends before it touches the folder it descended into", async () => { // The order matters on a folder whose current mode would stop the walk: the // children are reached with the permissions the extraction left behind. const fileSystem = fakeTree( @@ -164,11 +163,24 @@ describe("changePermissions", () => { () => ["Vintagestory"] ) - changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }) + await changePermissions({ paths: [FAKE_ROOT], perms: 0o755, fileSystem }) assert.deepEqual(fileSystem.chmodCalls, [ { path: join(FAKE_ROOT, "Vintagestory"), mode: 0o755 }, { path: FAKE_ROOT, mode: 0o755 } ]) }) + + // The caller's bound on a filesystem that stops answering. The check sits before each + // entry, so an aborted walk stops rather than running to the end of the tree. + it("stops on an aborted signal, leaving the entries it has not reached alone", async () => { + const fileSystem = fakeTree( + (path) => (path === FAKE_ROOT ? FAKE_DIRECTORY : FAKE_FILE), + () => ["Vintagestory", "assets"] + ) + + await assert.rejects(() => changePermissions({ paths: [FAKE_ROOT], perms: 0o755, signal: AbortSignal.abort(), fileSystem })) + + assert.deepEqual(fileSystem.chmodCalls, []) + }) }) From c89a5c14ac7d18ab9a25d511efff032495715ecc Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:10:02 +0200 Subject: [PATCH 5/6] Skip a symbolic link with nothing reachable behind it The walk used to open each entry with existsSync before lstat. existsSync resolves links, so a link whose target could not be reached answered false and the entry was skipped. Replacing both with a single lstat changed that: lstat does not follow links, so the link itself stats fine and the walk refuses it. The class is every link existsSync could not resolve, a dangling target, a loop, a target behind a folder with no execute bit. The handler turns that refusal into "Changing permissions failed", and the renderer's extract task fails on it by design, so a Linux install into a folder holding one stale link failed where it used to complete. The link is now tested with access, which follows links the way existsSync did, and refused only when something is there for chmod to resolve it to. Nothing is applied on either branch, so the gap between the two answers leaves nothing for the tree to change under. The handler test that pinned the refusal built its link over a file it never wrote, so it was passing on the dangling arm rather than the one it names. It now writes the target first, the way the case in permissions.test.ts always did. --- src/ipc/permissions.ts | 31 ++++++++++++++++++++++++------- tests/ipc/pathsHandlers.test.ts | 6 +++++- tests/ipc/permissions.test.ts | 17 ++++++++++++++++- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/ipc/permissions.ts b/src/ipc/permissions.ts index e1fcb8a7..c62ab0ec 100644 --- a/src/ipc/permissions.ts +++ b/src/ipc/permissions.ts @@ -5,13 +5,14 @@ * archive without the execute bit. A symbolic link inside the tree stops the * whole run rather than being followed: chmod resolves links, so following one * would apply the launcher's bits to a file outside the folder the user picked. + * A link that resolves to nothing has no such file behind it and is skipped. * * The walk is I/O and nothing else, so it runs on the main thread's event loop * rather than in a worker: every step is an awaited syscall, and none of them * holds the loop. Nothing here touches Electron. */ -import { chmod, lstat, readdir } from "node:fs/promises" +import { access, chmod, lstat, readdir } from "node:fs/promises" import { join } from "node:path" const MAX_ITEMS = 100_000 @@ -21,6 +22,8 @@ export interface PermissionsFileSystem { lstat(path: string): Promise<{ isSymbolicLink(): boolean; isDirectory(): boolean; isFile(): boolean }> readdir(path: string): Promise chmod(path: string, mode: number): Promise + /** Follows links, unlike `lstat`. Refusing is what tells a link with nothing on the other end apart from a live one. */ + access(path: string): Promise } export interface ChangePermissionsOptions { @@ -41,13 +44,13 @@ export interface ChangePermissionsOptions { * union of the paths a Linux install might use without checking each one first. * * @param options Roots, mode, abort signal, and the filesystem to act on. - * @throws On a symbolic link, on an entry that is neither a file nor a folder, - * once the tree passes the entry cap, and on an aborted signal. Nothing is - * rolled back: the caller reports the failure and the bits already applied stay - * applied. + * @throws On a symbolic link whose target can be reached, on an entry that is + * neither a file nor a folder, once the tree passes the entry cap, and on an + * aborted signal. Nothing is rolled back: the caller reports the failure and + * the bits already applied stay applied. */ export async function changePermissions(options: ChangePermissionsOptions): Promise { - const { paths, perms, signal, fileSystem = { lstat, readdir, chmod } } = options + const { paths, perms, signal, fileSystem = { lstat, readdir, chmod, access } } = options let itemCount = 0 const visit = async (path: string): Promise => { @@ -59,7 +62,21 @@ export async function changePermissions(options: ChangePermissionsOptions): Prom const stats = await fileSystem.lstat(path).catch(() => null) if (!stats) return - if (stats.isSymbolicLink()) throw new Error("Symbolic links are not allowed") + if (stats.isSymbolicLink()) { + // A link with nothing reachable on the other end is skipped rather than refused: a + // dangling target, a loop, a target behind a folder with no execute bit. The rule this + // guard exists for is about what chmod would resolve the link to, and there is nothing + // to resolve it to here, so a Linux install into a folder holding one stale link + // completes the way it always has. Neither answer applies a bit, so the gap between + // this call and the lstat above leaves nothing for the tree to change under. + const targetReachable = await fileSystem.access(path).then( + () => true, + () => false + ) + if (targetReachable) throw new Error("Symbolic links are not allowed") + return + } + itemCount++ if (itemCount > MAX_ITEMS) throw new Error("Too many filesystem entries") diff --git a/tests/ipc/pathsHandlers.test.ts b/tests/ipc/pathsHandlers.test.ts index 0af29f6e..7eb98545 100644 --- a/tests/ipc/pathsHandlers.test.ts +++ b/tests/ipc/pathsHandlers.test.ts @@ -1024,7 +1024,11 @@ describe.skipIf(process.platform === "win32")("CHANGE_PERMS: the walk itself", ( // and the handler now fixes in its place. it("reports a refusal under one fixed message rather than the reason behind it", async () => { const event = await createTrustedEvent() - symlinkSync(join(temporaryRoot, "outsider.txt"), join(managedFolder, "shortcut")) + // The target has to be there: a link pointing at nothing is skipped rather than refused, + // so without this the walk would finish and there would be no refusal to report. + const outsider = join(temporaryRoot, "outsider.txt") + writeFileSync(outsider, "not the launcher's file", { mode: 0o600 }) + symlinkSync(outsider, join(managedFolder, "shortcut")) await assert.rejects(() => handler(IPC_CHANNELS.PATHS_MANAGER.CHANGE_PERMS)(event, [managedFolder], 0o755), /^Error: Changing permissions failed$/) }) diff --git a/tests/ipc/permissions.test.ts b/tests/ipc/permissions.test.ts index cb0e6230..1b7e45ff 100644 --- a/tests/ipc/permissions.test.ts +++ b/tests/ipc/permissions.test.ts @@ -47,7 +47,9 @@ function fakeTree(lstat: (path: string) => FakeStats, readdir: (path: string) => readdir: async (path): Promise => readdir(path), chmod: async (path, mode): Promise => { chmodCalls.push({ path, mode }) - } + }, + // Nothing these fake trees describe is a symbolic link, so the walk never asks. + access: async (): Promise => {} } } @@ -131,6 +133,19 @@ describe("changePermissions", () => { assert.equal(modeOf(outsider), 0o600) }) + // The other half of that rule. A link pointing at nothing has no target for chmod to + // resolve it to, so it is skipped the way a missing path is, and the install it sits in + // finishes. A stale link left behind in a game folder is ordinary, and failing the whole + // extract task over one would be a refusal the player cannot act on. + it.skipIf(process.platform === "win32")("skips a symbolic link whose target cannot be reached, and walks the rest", async () => { + symlinkSync(workspacePath("never-written.txt"), join(installation, "dangling")) + + await assert.doesNotReject(() => changePermissions({ paths: [installation], perms: 0o755 })) + + assert.equal(modeOf(installation, "Vintagestory"), 0o755) + assert.equal(modeOf(installation, "assets", "version.txt"), 0o755) + }) + it("refuses an entry that is neither a file nor a folder", async () => { const socket = fakeStats("other") const fileSystem = fakeTree( From 0cac633e97950ddb60a437d8008a027f28603891 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:10:12 +0200 Subject: [PATCH 6/6] Race the abort signal against the walk, not only between entries The ten minute bound was only ever observed at throwIfAborted, which runs before each entry. A syscall that never settles never reaches the next check, so the returned promise stayed pending for good and the handler never rejected. That is the one case the bound is named for: a hard NFS or FUSE mount that goes away leaves the thread in uninterruptible sleep, and the walk never comes back to look at the signal. The bound the worker carried did not depend on the worker's state. It was a timer on the main thread, which is why the thread it gave up on was discarded rather than reused. Racing the signal against the walk restores that: the call settles when the signal fires, whatever the walk is doing, and the walk is abandoned in the same way. The check between entries stays. It is what stops a walk still making progress from touching the rest of the tree. --- src/ipc/permissions.ts | 24 ++++++++++++++++++++++-- tests/ipc/permissions.test.ts | 15 +++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/ipc/permissions.ts b/src/ipc/permissions.ts index c62ab0ec..9f683282 100644 --- a/src/ipc/permissions.ts +++ b/src/ipc/permissions.ts @@ -31,7 +31,11 @@ export interface ChangePermissionsOptions { paths: readonly string[] /** Mode passed straight to `chmod`. */ perms: number - /** Stops the walk before its next entry. The caller's bound on a tree that never ends. */ + /** + * The caller's bound on a tree that never ends. Checked before each entry, so a walk still + * making progress stops without touching the rest of the tree, and raced against the walk + * as a whole, so one syscall that never answers still settles the call. + */ signal?: AbortSignal /** Filesystem to act on, defaulting to the real one. */ fileSystem?: PermissionsFileSystem @@ -88,5 +92,21 @@ export async function changePermissions(options: ChangePermissionsOptions): Prom await fileSystem.chmod(path, perms) } - for (const path of paths) await visit(path) + const walk = (async (): Promise => { + for (const path of paths) await visit(path) + })() + + if (!signal) return walk + + // The check at the top of visit stops a walk that is still making syscalls. It never runs + // again once a single syscall stops answering, which is what a hard NFS or FUSE mount that + // goes away leaves behind, so the signal has to settle this call on its own as well. + // The walk is then abandoned rather than stopped, the same standing the worker thread this + // replaced had: its timeout fired on the main thread too, and the thread it gave up on was + // discarded rather than waited for. + const aborted = new Promise((_, reject) => { + signal.addEventListener("abort", () => reject(signal.reason), { once: true }) + }) + + return Promise.race([walk, aborted]) } diff --git a/tests/ipc/permissions.test.ts b/tests/ipc/permissions.test.ts index 1b7e45ff..f823e41b 100644 --- a/tests/ipc/permissions.test.ts +++ b/tests/ipc/permissions.test.ts @@ -198,4 +198,19 @@ describe("changePermissions", () => { assert.deepEqual(fileSystem.chmodCalls, []) }) + + // And the case the check between entries cannot reach: a hard NFS or FUSE mount that goes + // away leaves a syscall that never answers, so the walk never gets back to the check. The + // bound has to hold there too, or the handler's promise stays pending and the renderer's + // extract task shows as running for good. + it("rejects on the signal even when a syscall never answers", async () => { + const wedged: PermissionsFileSystem = { + lstat: () => new Promise(() => {}), + readdir: async (): Promise => [], + chmod: async (): Promise => {}, + access: async (): Promise => {} + } + + await assert.rejects(() => changePermissions({ paths: ["/mnt/wedged"], perms: 0o755, signal: AbortSignal.timeout(20), fileSystem: wedged }), /aborted due to timeout/) + }) })