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/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..9f683282 --- /dev/null +++ b/src/ipc/permissions.ts @@ -0,0 +1,112 @@ +/** + * 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. + * 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 { access, 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 + /** 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 { + /** Roots to walk. Each is applied to itself and to everything beneath it. */ + paths: readonly string[] + /** Mode passed straight to `chmod`. */ + perms: number + /** + * 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 +} + +/** + * 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 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, access } } = 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()) { + // 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") + + 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) + } + + 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/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 { - 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/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)) } 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() diff --git a/tests/ipc/pathsHandlers.test.ts b/tests/ipc/pathsHandlers.test.ts index e66d4e9c..7eb98545 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,33 @@ 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() + // 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/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..f823e41b 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,12 +43,13 @@ 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 }) - } + }, + // Nothing these fake trees describe is a symbolic link, so the walk never asks. + access: async (): Promise => {} } } @@ -83,8 +84,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 +93,72 @@ 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", () => { + // 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( () => 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 +167,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 +178,39 @@ 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, []) + }) + + // 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/) + }) })