From 01649d0bcd789ef930669f354a64ac68d84dec6a Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:10:12 +0200 Subject: [PATCH 1/9] refactor(backups): write installation backups as gzipped tar The backup writer was the only thing left in the launcher that needed a 7-Zip process. It now goes through the tar package the game archives are already read with, so a backup is written in process rather than by spawning a binary and parsing what it prints. The compressionLevel the config carries keeps its meaning: zlib's gzip takes the same 0 to 9 scale, so the number reaches the writer unchanged and level 0 still stores rather than deflates. Progress reporting keeps the shape the worker protocol expects. The safety walk over the source tree now also totals the bytes it sees, and each entry written moves the figure, capped a point short of the single terminal 100 the caller emits at the end. The tests drive real archives instead of a stand-in for the 7-Zip call, which is what makes the compression level and the archive's own shape assertable rather than taken on trust. --- src/domain/installations/backup.ts | 6 +- src/ipc/workers/compressWorker.ts | 5 +- src/ipc/workers/compression.ts | 123 ++++++------- tests/domain/installations/backup.test.ts | 26 +-- tests/ipc/compression.test.ts | 199 +++++++++------------- 5 files changed, 167 insertions(+), 192 deletions(-) diff --git a/src/domain/installations/backup.ts b/src/domain/installations/backup.ts index 23edc290..b82d7033 100644 --- a/src/domain/installations/backup.ts +++ b/src/domain/installations/backup.ts @@ -156,10 +156,12 @@ export async function makeInstallationBackup(ports: MakeInstallationBackupPorts, const date = ports.clock.now() // Falls back to a slice of the installation id when the name sanitises // away to nothing (e.g. "***"), so the archive never ends up as a bare - // "_.zip". + // "_.tar.gz". const cleanInstallationName = cleanFolderName(installation.name) || installation.id.slice(0, 8) const dateStamp = formatTimestampForFilename(date) - const fileName = `${cleanInstallationName}_${dateStamp}.zip` + // Backups were zips up to 1.7.0-beta.4 and the restore still reads those, by + // the extension recorded with each one. New ones are gzipped tar. + const fileName = `${cleanInstallationName}_${dateStamp}.tar.gz` const outputFolder = await ports.paths.join([backupsFolder, INSTALLATIONS_BACKUP_SUBFOLDER, cleanInstallationName]) const archivePath = await ports.paths.join([outputFolder, fileName]) diff --git a/src/ipc/workers/compressWorker.ts b/src/ipc/workers/compressWorker.ts index 2044d981..c4b2f7e8 100644 --- a/src/ipc/workers/compressWorker.ts +++ b/src/ipc/workers/compressWorker.ts @@ -3,14 +3,13 @@ import { runCompression } from "@src/ipc/workers/compression" serveTasks( async (payload, onProgress) => { - const { inputPath, outputPath, outputFileName, compressionLevel, sevenZipBin } = payload as { + const { inputPath, outputPath, outputFileName, compressionLevel } = payload as { inputPath: string outputPath: string outputFileName: string compressionLevel?: number - sevenZipBin?: string } - await runCompression({ inputPath, outputPath, outputFileName, compressionLevel, sevenZipBin, onProgress }) + await runCompression({ inputPath, outputPath, outputFileName, compressionLevel, onProgress }) }, () => "Compression failed" ) diff --git a/src/ipc/workers/compression.ts b/src/ipc/workers/compression.ts index 4e0a0743..e47335e3 100644 --- a/src/ipc/workers/compression.ts +++ b/src/ipc/workers/compression.ts @@ -5,34 +5,35 @@ * 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`. * - * The source tree is walked before 7-Zip is handed anything: a symbolic link or - * a device node in there would be followed into whatever it points at, and an - * unbounded tree would keep the worker busy indefinitely. + * Backups are written as gzipped tar through the same `tar` package the game + * archives are already read with. The source tree is walked before tar is + * handed anything: a symbolic link or a device node in there would be followed + * into whatever it points at, and an unbounded tree would keep the worker busy + * indefinitely. The walk also totals the bytes, which is what progress is + * measured against. */ -import type { EventEmitter } from "node:events" -import Seven from "node-7z" import fse from "fs-extra" import { join } from "node:path" +import * as tar from "tar" import { DEFAULT_COMPRESSION_LEVEL } from "@domain/config/defaults" const MAX_ITEMS = 100_000 -/** The `Seven.add` shape, so a test can drive the progress and end events without spawning 7-Zip. */ -export type ArchiveAdd = (archivePath: string, source: string, options: NonNullable[2]>) => EventEmitter - /** * Refuses a source tree holding anything but plain files and folders, or more * entries than the launcher ever legitimately backs up. * * @param root Folder about to be archived. * @param fileSystem Filesystem to walk, defaulting to the real one. + * @returns Total size in bytes of every plain file in the tree. * @throws When an entry is a symbolic link or a special file, or the tree is too large. */ -export function assertSafeCompressionTree(root: string, fileSystem: Pick = fse): void { +export function assertSafeCompressionTree(root: string, fileSystem: Pick = fse): number { const pending = [root] let itemCount = 0 + let totalBytes = 0 while (pending.length > 0) { const current = pending.pop() as string @@ -43,8 +44,12 @@ export function assertSafeCompressionTree(root: string, fileSystem: Pick MAX_ITEMS) throw new Error("Too many filesystem entries") if (stats.isDirectory()) { for (const child of fileSystem.readdirSync(current)) pending.push(join(current, child)) + } else { + totalBytes += stats.size } } + + return totalBytes } export interface CompressionOptions { @@ -54,12 +59,8 @@ export interface CompressionOptions { outputPath: string /** Archive file name, taken as given. */ outputFileName: string - /** 7-Zip `-mx` level, 0 to 9. */ + /** gzip level, 0 to 9. Same scale the config's compressionLevel has always carried. */ compressionLevel?: number - /** 7-Zip binary. */ - sevenZipBin?: string - /** Injected `Seven.add`, defaulting to the real one. */ - addArchive?: ArchiveAdd /** Called with 0 to 100 as the work advances, and once with 100 at the end. */ onProgress?: (progress: number) => void } @@ -68,50 +69,58 @@ export interface CompressionOptions { * Compresses one folder into one archive. * * @param options Source, destination, and how to report progress. - * @throws As a rejection, for an unsafe source or destination and for a 7-Zip - * failure alike. The caller reports "Compression failed" either way. + * @throws As a rejection, for an unsafe source or destination and for a failed + * write alike. The caller reports "Compression failed" either way. */ -export function runCompression(options: CompressionOptions): Promise { - const { inputPath, outputPath, outputFileName, compressionLevel = DEFAULT_COMPRESSION_LEVEL, sevenZipBin, addArchive = Seven.add, onProgress } = options - - return new Promise((resolvePromise, rejectPromise) => { - assertSafeCompressionTree(inputPath) - if (!fse.existsSync(inputPath) || !fse.lstatSync(inputPath).isDirectory()) throw new Error("Compression source must be a directory") - if (!fse.existsSync(outputPath)) fse.mkdirSync(outputPath, { recursive: true }) - if (fse.lstatSync(outputPath).isSymbolicLink() || !fse.lstatSync(outputPath).isDirectory()) throw new Error("Compression destination is unsafe") - - const archivePath = join(outputPath, outputFileName) - if (fse.existsSync(archivePath)) { - const archiveStats = fse.lstatSync(archivePath) - if (archiveStats.isSymbolicLink() || archiveStats.isDirectory()) throw new Error("Compression archive target is unsafe") - } - const sourceGlob = join(inputPath, "*") - - const stream = addArchive(archivePath, sourceGlob, { - $bin: sevenZipBin, - $progress: true, - recursive: true, - method: [`x=${compressionLevel}`, "mt=on"] - }) - - let lastReportedProgress = 0 - - stream.on("progress", ({ percent }: { percent: unknown }) => { - const boundedPercent = Number(percent) - // The end event below owns the single terminal 100 report. - if (Number.isFinite(boundedPercent) && boundedPercent > lastReportedProgress && boundedPercent < 100) { - lastReportedProgress = boundedPercent - onProgress?.(boundedPercent) - } - }) +export async function runCompression(options: CompressionOptions): Promise { + const { inputPath, outputPath, outputFileName, compressionLevel = DEFAULT_COMPRESSION_LEVEL, onProgress } = options + + const totalBytes = assertSafeCompressionTree(inputPath) + if (!fse.existsSync(inputPath) || !fse.lstatSync(inputPath).isDirectory()) throw new Error("Compression source must be a directory") + if (!fse.existsSync(outputPath)) fse.mkdirSync(outputPath, { recursive: true }) + if (fse.lstatSync(outputPath).isSymbolicLink() || !fse.lstatSync(outputPath).isDirectory()) throw new Error("Compression destination is unsafe") + + const archivePath = join(outputPath, outputFileName) + if (fse.existsSync(archivePath)) { + const archiveStats = fse.lstatSync(archivePath) + if (archiveStats.isSymbolicLink() || archiveStats.isDirectory()) throw new Error("Compression archive target is unsafe") + } - stream.on("end", () => { - onProgress?.(100) - resolvePromise() - }) + const entries = fse.readdirSync(inputPath) + let writtenBytes = 0 + let lastReportedProgress = 0 + + try { + await tar.create( + { + file: archivePath, + cwd: inputPath, + gzip: { level: compressionLevel }, + portable: true, + // `entry.size` is not filled in yet when this runs, and the stat behind + // the entry is the same number the walk above totalled. Folders are + // skipped for the same reason the walk skipped them: their stat size is + // the directory's own on-disk size, which is not content. + onWriteEntry: (entry) => { + writtenBytes += entry.stat?.isFile() ? entry.stat.size : 0 + if (totalBytes <= 0) return + // The terminal 100 below is the only one this ever reports, so the + // running figure is capped a point short of it. + const progress = Math.min(99, Math.floor((writtenBytes / totalBytes) * 100)) + if (progress > lastReportedProgress) { + lastReportedProgress = progress + onProgress?.(progress) + } + } + }, + // A folder with nothing in it is still a folder worth backing up, and tar + // refuses an empty list of paths. "." archives the folder itself, which + // unpacks back to an empty folder rather than to nothing at all. + entries.length > 0 ? entries : ["."] + ) + } catch { + throw new Error("Compression failed") + } - stream.on("error", () => { - rejectPromise(new Error("Compression failed")) - }) - }) + onProgress?.(100) } diff --git a/tests/domain/installations/backup.test.ts b/tests/domain/installations/backup.test.ts index 7631484e..5de9b7b6 100644 --- a/tests/domain/installations/backup.test.ts +++ b/tests/domain/installations/backup.test.ts @@ -66,7 +66,7 @@ function fakePorts(overrides: Partial = {}): MakeIn } function backup(id: string, overrides: { isDeleting?: boolean; isRestoring?: boolean } = {}): BackupRecord & { isDeleting?: boolean; isRestoring?: boolean } { - return { id, date: 1, path: `/backups/${id}.zip`, ...overrides } + return { id, date: 1, path: `/backups/${id}.tar.gz`, ...overrides } } function snapshot(overrides: Partial = {}): InstallationSnapshot { @@ -163,7 +163,7 @@ describe("makeInstallationBackup pruning", () => { assert.deepEqual(result.deletedBackupIds, ["b3", "b2"]) assert.deepEqual( trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), - ["remove:/backups/b3.zip", "deleted:b3", "remove:/backups/b2.zip", "deleted:b2"] + ["remove:/backups/b3.tar.gz", "deleted:b3", "remove:/backups/b2.tar.gz", "deleted:b2"] ) }) @@ -181,7 +181,7 @@ describe("makeInstallationBackup pruning", () => { it("stops at the first failed deletion and reports the ones already done", async () => { const installation = snapshot({ backupsLimit: 1, backups: [backup("b1"), backup("b2"), backup("b3")] }) - const ports = fakePorts({ fileSystem: fakeFileSystem({ removals: { "/backups/b2.zip": false } }) }) + const ports = fakePorts({ fileSystem: fakeFileSystem({ removals: { "/backups/b2.tar.gz": false } }) }) const result = await makeInstallationBackup(ports, { installation, backupsFolder: "/backups" }, recordingEvents()) @@ -203,7 +203,7 @@ describe("makeInstallationBackup pruning", () => { assert.deepEqual(result.deletedBackupIds, ["b2"]) assert.deepEqual( trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), - ["remove:/backups/b2.zip", "deleted:b2"] + ["remove:/backups/b2.tar.gz", "deleted:b2"] ) }) @@ -229,7 +229,7 @@ describe("makeInstallationBackup pruning", () => { assert.deepEqual(result.deletedBackupIds, ["b4", "b2"]) assert.deepEqual( trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), - ["remove:/backups/b4.zip", "deleted:b4", "remove:/backups/b2.zip", "deleted:b2"] + ["remove:/backups/b4.tar.gz", "deleted:b4", "remove:/backups/b2.tar.gz", "deleted:b2"] ) }) @@ -242,7 +242,7 @@ describe("makeInstallationBackup pruning", () => { assert.deepEqual(result.deletedBackupIds, ["b2"]) assert.deepEqual( trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), - ["remove:/backups/b2.zip", "deleted:b2"] + ["remove:/backups/b2.tar.gz", "deleted:b2"] ) }) }) @@ -253,11 +253,11 @@ describe("makeInstallationBackup archiving", () => { const result = await makeInstallationBackup(fakePorts({ archiver }), { installation: snapshot(), backupsFolder: "/backups" }) - assert.equal(requests[0]?.fileName, "My-Install-Test_2025-08-15_23-20-00.zip") + assert.equal(requests[0]?.fileName, "My-Install-Test_2025-08-15_23-20-00.tar.gz") assert.equal(requests[0]?.outputFolder, "/backups/Installations/My-Install-Test") assert.equal(requests[0]?.sourcePath, "/games/my-install") assert.equal(requests[0]?.compressionLevel, 5) - assert.equal(result.ok === true && result.backup.path, "/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.zip") + assert.equal(result.ok === true && result.backup.path, "/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.tar.gz") }) it("falls back to a slice of the installation id when the cleaned name is empty", async () => { @@ -265,9 +265,9 @@ describe("makeInstallationBackup archiving", () => { const result = await makeInstallationBackup(fakePorts({ archiver }), { installation: snapshot({ id: "installation-1", name: "***" }), backupsFolder: "/backups" }) - assert.equal(requests[0]?.fileName, "installa_2025-08-15_23-20-00.zip") + assert.equal(requests[0]?.fileName, "installa_2025-08-15_23-20-00.tar.gz") assert.equal(requests[0]?.outputFolder, "/backups/Installations/installa") - assert.equal(result.ok === true && result.backup.path, "/backups/Installations/installa/installa_2025-08-15_23-20-00.zip") + assert.equal(result.ok === true && result.backup.path, "/backups/Installations/installa/installa_2025-08-15_23-20-00.tar.gz") }) it("stamps the record with the clock time and a generated id", async () => { @@ -276,7 +276,7 @@ describe("makeInstallationBackup archiving", () => { assert.deepEqual(result.ok === true && result.backup, { id: "generated-id-1", date: FIXED_NOW, - path: "/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.zip" + path: "/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.tar.gz" }) }) @@ -287,9 +287,9 @@ describe("makeInstallationBackup archiving", () => { "exists:/games/my-install", "guard-acquire:Making and installation backup.", "started", - "remove:/backups/b1.zip", + "remove:/backups/b1.tar.gz", "deleted:b1", - "compress:/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.zip", + "compress:/backups/Installations/My-Install-Test/My-Install-Test_2025-08-15_23-20-00.tar.gz", "guard-release", "finished" ]) diff --git a/tests/ipc/compression.test.ts b/tests/ipc/compression.test.ts index 2f05c5ee..dd93d485 100644 --- a/tests/ipc/compression.test.ts +++ b/tests/ipc/compression.test.ts @@ -1,23 +1,22 @@ import assert from "node:assert/strict" -import { EventEmitter } from "node:events" -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs" +import { mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" +import { gunzipSync } from "node:zlib" import { afterEach, beforeEach, describe, it } from "vitest" +import * as tar from "tar" -import { assertSafeCompressionTree, runCompression, type ArchiveAdd } from "@src/ipc/workers/compression" +import { assertSafeCompressionTree, runCompression } from "@src/ipc/workers/compression" /** - * The backup compression, driven without spawning 7-Zip. + * The backup compression, against real archives. * - * `runCompression` takes the `Seven.add` call as a parameter, so these pin two - * separate things: the safety walk over the real source tree, which runs before - * 7-Zip is handed anything, and the invocation and message protocol, which is - * asserted on the recorded arguments instead of on an archive. + * Two separate things are pinned here: the safety walk over the source tree, + * which runs before tar is handed anything, and what the archive turns out to + * hold once it is written. * - * The walk is the half worth being fussy about. 7-Zip follows a symbolic link - * into whatever it points at, so a link inside a backup source would quietly - * pull files from outside the folder into the archive. + * The walk is the half worth being fussy about. A symbolic link inside a backup + * source would otherwise pull files from outside the folder into the archive. */ let workspace: string @@ -28,29 +27,16 @@ function workspacePath(...parts: string[]): string { return join(workspace, ...parts) } -interface Invocation { - archivePath: string - source: string - options: Record -} - -/** A stand-in for `Seven.add` that records its call and replays a scripted event sequence. */ -function fakeAdd(script: (stream: EventEmitter) => void): { add: ArchiveAdd; invocations: Invocation[] } { - const invocations: Invocation[] = [] - - const add: ArchiveAdd = (archivePath, sourceGlob, options) => { - invocations.push({ archivePath, source: sourceGlob, options: options as unknown as Record }) - const stream = new EventEmitter() - setImmediate(() => script(stream)) - return stream - } - - return { add, invocations } +/** Every entry name a written archive holds, sorted. */ +async function archiveEntryNames(archivePath: string): Promise { + const names: string[] = [] + await tar.list({ file: archivePath, onReadEntry: (entry) => void names.push(entry.path) }) + return names.sort() } const FAKE_ROOT = "/fake/root" -type FakeStats = { isSymbolicLink(): boolean; isDirectory(): boolean; isFile(): boolean } +type FakeStats = { isSymbolicLink(): boolean; isDirectory(): boolean; isFile(): boolean; size: number } type TreeFileSystem = Parameters[1] /** A filesystem that answers from the two functions given, for trees too large or too odd to build. */ @@ -63,15 +49,8 @@ function fakeStats(kind: "directory" | "file" | "other"): FakeStats { return { isSymbolicLink: (): boolean => false, isDirectory: (): boolean => kind === "directory", - isFile: (): boolean => kind === "file" - } -} - -/** Scripts a run that reports the given percentages and then ends. */ -function succeedsAt(...percentages: unknown[]): (stream: EventEmitter) => void { - return (stream) => { - for (const percent of percentages) stream.emit("progress", { percent }) - stream.emit("end") + isFile: (): boolean => kind === "file", + size: 0 } } @@ -91,8 +70,8 @@ afterEach(() => { }) describe("assertSafeCompressionTree", () => { - it("accepts a tree of plain files and folders", () => { - assert.doesNotThrow(() => assertSafeCompressionTree(source)) + it("accepts a tree of plain files and folders and totals their bytes", () => { + assert.equal(assertSafeCompressionTree(source), "elf".length + "1.22.6".length) }) it("refuses a symbolic link anywhere in the tree", () => { @@ -140,143 +119,129 @@ describe("assertSafeCompressionTree", () => { describe("runCompression", () => { it("archives the source contents, not the source folder itself", async () => { - const seven = fakeAdd(succeedsAt(30, 80)) - - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }) + await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }) - assert.equal(seven.invocations.length, 1) - assert.equal(seven.invocations[0]?.archivePath, join(output, "backup.zip")) - // The trailing glob is what keeps the wrapping folder out of the archive, - // so a restore puts the files back where they came from. - assert.equal(seven.invocations[0]?.source, join(source, "*")) + // No "installation/" prefix anywhere: the wrapping folder is what a restore + // would otherwise put back one level too deep. + assert.deepEqual(await archiveEntryNames(join(output, "backup.tar.gz")), ["Vintagestory", "assets/", "assets/version.txt"]) }) - it("passes the compression level and the binary 7-Zip is asked for", async () => { - const seven = fakeAdd(succeedsAt()) - - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", compressionLevel: 4, sevenZipBin: "/opt/7za", addArchive: seven.add }) + it("writes a gzip stream, which is what the restore reader expects", async () => { + await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }) - assert.deepEqual(seven.invocations[0]?.options, { $bin: "/opt/7za", $progress: true, recursive: true, method: ["x=4", "mt=on"] }) + const bytes = readFileSync(join(output, "backup.tar.gz")) + assert.deepEqual([bytes[0], bytes[1]], [0x1f, 0x8b]) + assert.equal(gunzipSync(bytes).length % 512, 0) }) - it("defaults to level 6 when the caller names none", async () => { - const seven = fakeAdd(succeedsAt()) - - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }) - - assert.deepEqual(seven.invocations[0]?.options.method, ["x=6", "mt=on"]) - }) + it("honours the compression level it is given", async () => { + const compressible = workspacePath("compressible") + mkdirSync(compressible) + writeFileSync(join(compressible, "log.txt"), "the same sentence over and over. ".repeat(4_000)) - it("reports progress and always ends at 100", async () => { - const progress: number[] = [] - const seven = fakeAdd(succeedsAt(10, 55, 90)) + await runCompression({ inputPath: compressible, outputPath: output, outputFileName: "fastest.tar.gz", compressionLevel: 1 }) + await runCompression({ inputPath: compressible, outputPath: output, outputFileName: "smallest.tar.gz", compressionLevel: 9 }) - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add, onProgress: (value) => progress.push(value) }) + // Level 0 stores rather than deflates, so it is the one that says outright + // that the number reaches zlib at all instead of being quietly dropped. + await runCompression({ inputPath: compressible, outputPath: output, outputFileName: "stored.tar.gz", compressionLevel: 0 }) - assert.deepEqual(progress, [10, 55, 90, 100]) + const stored = statSync(join(output, "stored.tar.gz")).size + const fastest = statSync(join(output, "fastest.tar.gz")).size + const smallest = statSync(join(output, "smallest.tar.gz")).size + assert.equal(smallest <= fastest, true) + assert.equal(fastest < stored, true) }) - it("coalesces repeated percentages and reports completion once", async () => { + it("reports progress and always ends at 100, once", async () => { + const many = workspacePath("many-files") + mkdirSync(many) + for (let index = 0; index < 200; index++) writeFileSync(join(many, `file-${index}.bin`), Buffer.alloc(4_096, index % 251)) const progress: number[] = [] - const seven = fakeAdd(succeedsAt(0, 0, 10, 10, 50, 50, 100, 100)) - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add, onProgress: (value) => progress.push(value) }) + await runCompression({ inputPath: many, outputPath: output, outputFileName: "progress.tar.gz", onProgress: (value) => progress.push(value) }) - assert.deepEqual(progress, [10, 50, 100]) + assert.equal(progress.at(-1), 100) + assert.equal(progress.filter((value) => value === 100).length, 1) + assert.equal(new Set(progress).size, progress.length) + assert.deepEqual( + [...progress].sort((left, right) => left - right), + progress + ) + assert.equal( + progress.some((value) => value > 0 && value < 100), + true + ) }) - it("ignores a progress report that goes backwards, past 100, or is not a number", async () => { + it("reports the terminal 100 even for a source with nothing in it", async () => { + const empty = workspacePath("empty") + mkdirSync(empty) const progress: number[] = [] - const seven = fakeAdd(succeedsAt(40, 20, 140, "not a number", undefined, 60)) - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add, onProgress: (value) => progress.push(value) }) + await runCompression({ inputPath: empty, outputPath: output, outputFileName: "empty.tar.gz", onProgress: (value) => progress.push(value) }) - assert.deepEqual(progress, [40, 60, 100]) - }) - - it("fails when 7-Zip errors", async () => { - const seven = fakeAdd((stream) => stream.emit("error", new Error("7-Zip exited with code 2"))) - - await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }), /Compression failed/) + assert.deepEqual(progress, [100]) }) it("creates the destination folder when it is missing", async () => { - const seven = fakeAdd(succeedsAt()) const missing = workspacePath("backups", "nested", "deeper") - await runCompression({ inputPath: source, outputPath: missing, outputFileName: "backup.zip", addArchive: seven.add }) + await runCompression({ inputPath: source, outputPath: missing, outputFileName: "backup.tar.gz" }) - assert.equal(seven.invocations[0]?.archivePath, join(missing, "backup.zip")) + assert.equal(statSync(join(missing, "backup.tar.gz")).isFile(), true) }) - it("refuses a source holding a symbolic link, without invoking 7-Zip", async () => { + it("refuses a source holding a symbolic link, without writing an archive", async () => { symlinkSync(workspacePath("backups"), join(source, "elsewhere")) - const seven = fakeAdd(succeedsAt()) - await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }), /unsafe filesystem entry/) + await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }), /unsafe filesystem entry/) - assert.deepEqual(seven.invocations, []) + assert.equal(statSync(output).isDirectory(), true) + assert.throws(() => statSync(join(output, "backup.tar.gz"))) }) it("refuses a source that is a file rather than a folder", async () => { - const seven = fakeAdd(succeedsAt()) - - await assert.rejects(runCompression({ inputPath: join(source, "Vintagestory"), outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }), /must be a directory/) - - assert.deepEqual(seven.invocations, []) + await assert.rejects(runCompression({ inputPath: join(source, "Vintagestory"), outputPath: output, outputFileName: "backup.tar.gz" }), /must be a directory/) }) it("refuses a source that does not exist", async () => { - const seven = fakeAdd(succeedsAt()) - - await assert.rejects(runCompression({ inputPath: workspacePath("gone"), outputPath: output, outputFileName: "backup.zip", addArchive: seven.add })) - - assert.deepEqual(seven.invocations, []) + await assert.rejects(runCompression({ inputPath: workspacePath("gone"), outputPath: output, outputFileName: "backup.tar.gz" })) }) it("refuses a destination that is a symbolic link", async () => { const linked = workspacePath("linked-backups") symlinkSync(output, linked) - const seven = fakeAdd(succeedsAt()) - await assert.rejects(runCompression({ inputPath: source, outputPath: linked, outputFileName: "backup.zip", addArchive: seven.add }), /destination is unsafe/) - - assert.deepEqual(seven.invocations, []) + await assert.rejects(runCompression({ inputPath: source, outputPath: linked, outputFileName: "backup.tar.gz" }), /destination is unsafe/) }) it("refuses a destination that is a file", async () => { const asFile = workspacePath("not-a-folder") writeFileSync(asFile, "") - const seven = fakeAdd(succeedsAt()) - await assert.rejects(runCompression({ inputPath: source, outputPath: asFile, outputFileName: "backup.zip", addArchive: seven.add }), /destination is unsafe/) + await assert.rejects(runCompression({ inputPath: source, outputPath: asFile, outputFileName: "backup.tar.gz" }), /destination is unsafe/) }) it("refuses to write over a folder standing where the archive would go", async () => { - mkdirSync(join(output, "backup.zip")) - const seven = fakeAdd(succeedsAt()) - - await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }), /archive target is unsafe/) + mkdirSync(join(output, "backup.tar.gz")) - assert.deepEqual(seven.invocations, []) + await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }), /archive target is unsafe/) }) it("refuses to write through a symbolic link standing where the archive would go", async () => { writeFileSync(workspacePath("someone-elses-file"), "") - symlinkSync(workspacePath("someone-elses-file"), join(output, "backup.zip")) - const seven = fakeAdd(succeedsAt()) - - await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }), /archive target is unsafe/) + symlinkSync(workspacePath("someone-elses-file"), join(output, "backup.tar.gz")) - assert.deepEqual(seven.invocations, []) + await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }), /archive target is unsafe/) + assert.equal(readFileSync(workspacePath("someone-elses-file"), "utf8"), "") }) it("overwrites a plain archive file already sitting there", async () => { - writeFileSync(join(output, "backup.zip"), "the previous backup") - const seven = fakeAdd(succeedsAt()) + writeFileSync(join(output, "backup.tar.gz"), "the previous backup") - await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.zip", addArchive: seven.add }) + await runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }) - assert.equal(seven.invocations.length, 1) + assert.deepEqual(await archiveEntryNames(join(output, "backup.tar.gz")), ["Vintagestory", "assets/", "assets/version.txt"]) }) }) From 17ce41a095405827f656ccd44f2e35551012dbbc Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:10:23 +0200 Subject: [PATCH 2/9] refactor(backups): read legacy zip backups with yauzl Every backup made up to now is a zip, and those have to keep restoring for as long as players still hold them. yauzl already reads mod archives and already reads a zip's table of contents before extraction, so the restore gets a yauzl unpacking path and the zip writer is what goes away. Two formats reach the launcher now and no others. validateArchive routes gzipped tar to the tar reader and zip to yauzl, and refuses anything else by name rather than handing it to a reader that would have to guess. The hand-written parse of 7-Zip's -slt listing text, which nothing could reach any more, goes with it. The single-wrapping-folder flattening moves from being inferred from the file extension to being asked for. It was only ever meant for the Linux game archives, and now that backups are gzipped tar too, inferring it would flatten the restore of any installation whose only entry happens to be a folder. Two committed fixtures back this: a zip backup shaped like the ones the launcher used to write, restored and compared byte for byte, and one whose entry names climb out with "..", name a drive, and give an absolute path. Nothing lands outside the destination for any of them. --- src/ipc/archiveValidation.ts | 106 ++------- src/ipc/handlers/pathsHandlers.ts | 13 +- src/ipc/workers/extractWorker.ts | 4 +- src/ipc/workers/extraction.ts | 171 +++++++++++--- src/preload/index.ts | 4 +- src/preload/preload.d.ts | 2 +- .../src/contexts/TaskManagerContext.tsx | 8 +- .../src/features/versions/adapters/install.ts | 5 +- tests/domain/installations/restore.test.ts | 2 +- tests/fixtures/build-fixtures.ts | 48 +++- tests/fixtures/hostile-backup.zip | Bin 0 -> 399 bytes tests/fixtures/legacy-backup.zip | Bin 0 -> 558 bytes tests/ipc/archiveValidation.test.ts | 37 ++- tests/ipc/extraction.test.ts | 211 +++++++++++++++--- 14 files changed, 414 insertions(+), 197 deletions(-) create mode 100644 tests/fixtures/hostile-backup.zip create mode 100644 tests/fixtures/legacy-backup.zip diff --git a/src/ipc/archiveValidation.ts b/src/ipc/archiveValidation.ts index 0ff1bead..1cbcce68 100644 --- a/src/ipc/archiveValidation.ts +++ b/src/ipc/archiveValidation.ts @@ -10,24 +10,21 @@ * only one. * * Called from inside the worker rather than from the IPC handler before it - * starts one: validateSevenZipArchive's own parse of a `7z l` listing can run - * to 100,000 entries over up to 4MB of text, real CPU work that has no - * business running on the main process's event loop when a worker thread is - * about to exist for this archive regardless. + * starts one: walking a table of contents can run to 100,000 entries, real CPU + * work that has no business running on the main process's event loop when a + * worker thread is about to exist for this archive regardless. * * Which reader runs is decided by the file name, which is exactly why the name * has to be the real one: a `.tar.gz` saved as `.zip` used to be handed to a * zip reader that could not read it, and the install died there. */ -import { spawn } from "node:child_process" import yauzl from "yauzl" import * as tar from "tar" import { isArchiveSymlink, isSafeArchiveEntry, isSafeTarEntryType, isTarGzName, MAX_ARCHIVE_ENTRY_BYTES, MAX_ARCHIVE_TOTAL_BYTES } from "./validation" const MAX_ARCHIVE_ENTRIES = 100_000 -const MAX_LISTING_BYTES = 4 * 1024 * 1024 function comparableArchiveEntry(entryName: string): string { const normalizedName = entryName.replaceAll("\\", "/") @@ -89,90 +86,11 @@ export function validateZipArchive(filePath: string): Promise { }) } -export function validateSevenZipArchive(filePath: string, sevenZipBin: string): Promise { - return new Promise((resolvePromise, rejectPromise) => { - const archiveLister = spawn(sevenZipBin, ["l", "-slt", "-bb0", filePath], { windowsHide: true }) - let output = "" - let settled = false - - const finish = (error?: Error): void => { - if (settled) return - settled = true - if (error) rejectPromise(error) - else resolvePromise() - } - - archiveLister.stdout.on("data", (chunk) => { - output += chunk.toString() - if (Buffer.byteLength(output, "utf8") > MAX_LISTING_BYTES) { - archiveLister.kill() - finish(new Error("Archive listing is too large")) - } - }) - archiveLister.stderr.on("data", () => {}) - archiveLister.on("error", (error) => finish(error)) - archiveLister.on("close", (code) => { - if (settled) return - if (code !== 0) { - finish(new Error("Archive could not be listed")) - return - } - - const records = output.split(/\r?\n\s*\r?\n/) - let entryCount = 0 - let totalUncompressedBytes = 0 - let firstRecord = true - const entryNames = new Set() - for (const record of records) { - const lines = record.split(/\r?\n/) - const pathLine = lines.find((line) => line.startsWith("Path = ")) - if (!pathLine) continue - if (firstRecord) { - firstRecord = false - continue - } - - const entryName = pathLine.slice("Path = ".length) - const sizeLine = lines.find((line) => line.startsWith("Size = ")) - const folderLine = lines.find((line) => line.startsWith("Folder = ")) - const isFolder = folderLine?.slice("Folder = ".length).trim() === "+" - const attributesLine = lines.find((line) => line.startsWith("Attributes = ")) - // Tar style listings report links in their own columns instead of the attributes one. - const linkLines = lines.filter((line) => line.startsWith("Symbolic Link = ") || line.startsWith("Hard Link = ")) - const entrySize = sizeLine ? Number(sizeLine.slice("Size = ".length)) : 0 - entryCount++ - totalUncompressedBytes += Number.isFinite(entrySize) && entrySize >= 0 ? entrySize : 0 - - if ( - entryCount > MAX_ARCHIVE_ENTRIES || - !isSafeArchiveEntry(entryName) || - entryNames.has(comparableArchiveEntry(entryName)) || - (attributesLine !== undefined && /(^|\s)L(\s|$)/.test(attributesLine)) || - linkLines.some((line) => line.slice(line.indexOf("= ") + 2).trim().length > 0) || - (!isFolder && (!sizeLine || !Number.isFinite(entrySize) || entrySize < 0)) || - entrySize > MAX_ARCHIVE_ENTRY_BYTES || - totalUncompressedBytes > MAX_ARCHIVE_TOTAL_BYTES - ) { - finish(new Error("Archive contains an unsafe entry")) - return - } - entryNames.add(comparableArchiveEntry(entryName)) - } - - finish() - }) - }) -} - /** - * Lists a gzipped tar and holds it to the same bounds as the other formats. + * Lists a gzipped tar and holds it to the same bounds as the zip reader. * - * 7-Zip is not used for this one. It would only see the gzip container and - * report the single `.tar` inside, and the bundled p7zip 16.02 cannot read the - * tars Vintage Story ships at all: their headers leave the numeric fields as - * NUL bytes, which GNU tar and node-tar accept and 7-Zip calls "Is not - * archive". The tar reader also states an entry's kind outright, so links are - * refused by type rather than by parsing an attributes column. + * The tar reader states an entry's kind outright, so links are refused by type + * rather than by reading an attributes column. */ export async function validateTarGzArchive(filePath: string): Promise { let entryCount = 0 @@ -221,12 +139,16 @@ export async function validateTarGzArchive(filePath: string): Promise { /** * Checks an archive with the reader its format actually needs. * + * Two formats reach the launcher and no others: gzipped tar, which the game + * builds ship as and which every backup is written as, and zip, which is what + * the backups made before that change still are. Anything else is refused here + * rather than handed to a reader that would have to guess at it. + * * @param filePath Archive on disk. Its name decides the reader. - * @param sevenZipBin 7-Zip binary, for the formats neither yauzl nor tar handles. * @throws When the archive cannot be read or holds an entry the launcher refuses to unpack. */ -export async function validateArchive(filePath: string, sevenZipBin: string): Promise { - if (filePath.toLowerCase().endsWith(".zip")) return validateZipArchive(filePath) +export async function validateArchive(filePath: string): Promise { if (isTarGzName(filePath)) return validateTarGzArchive(filePath) - return validateSevenZipArchive(filePath, sevenZipBin) + if (filePath.toLowerCase().endsWith(".zip")) return validateZipArchive(filePath) + throw new Error("Archive format is not supported") } diff --git a/src/ipc/handlers/pathsHandlers.ts b/src/ipc/handlers/pathsHandlers.ts index 21be8cdc..42df9e2f 100644 --- a/src/ipc/handlers/pathsHandlers.ts +++ b/src/ipc/handlers/pathsHandlers.ts @@ -1,5 +1,4 @@ import { ipcMain, app, shell } from "electron" -import { path7za } from "7zip-bin" import fse from "fs-extra" import { open } from "node:fs/promises" import type { Stats } from "node:fs" @@ -27,7 +26,6 @@ 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 sevenZipBin = app.isPackaged ? path7za.replace("app.asar", "app.asar.unpacked") : path7za const WORKER_TIMEOUTS_MS: Record = { DOWNLOAD_ON_PATH: 45 * 60 * 1_000, EXTRACT_ON_PATH: 30 * 60 * 1_000, @@ -385,17 +383,18 @@ ipcMain.handle(IPC_CHANNELS.PATHS_MANAGER.DOWNLOAD_ON_PATH, async (event, id: st return downloadedPath }) -ipcMain.handle(IPC_CHANNELS.PATHS_MANAGER.EXTRACT_ON_PATH, async (event, id: string, filePath: string, outputPath: string, deleteZip: boolean): Promise => { +ipcMain.handle(IPC_CHANNELS.PATHS_MANAGER.EXTRACT_ON_PATH, async (event, id: string, filePath: string, outputPath: string, deleteZip: boolean, unwrapSingleRootFolder = false): Promise => { assertTrustedIpcSender(event) const safeId = assertSafeTaskId(id) const safeFilePath = await assertManagedPath(filePath, "archive path") const safeOutputPath = await assertManagedPath(outputPath, "output path", { allowMissing: true }) const shouldDeleteZip = assertBoolean(deleteZip, "delete archive flag") + const shouldUnwrapSingleRootFolder = assertBoolean(unwrapSingleRootFolder, "unwrap single root folder flag") if (resolve(safeFilePath) === resolve(safeOutputPath)) throw new TypeError("Archive and output paths must differ") // validateArchive runs inside the extraction worker now (workers/extraction.ts's - // runExtraction), not here: its 7z-listing parse is real CPU work that has no business - // blocking the main process's event loop. + // runExtraction), not here: walking a table of contents is real CPU work that has no + // business blocking the main process's event loop. logMessage("info", `[back] [ipc] [ipc/handlers/pathsHandlers.ts] [EXTRACT_ON_PATH] [${safeId}] Starting a bounded extraction.`) await archiveConcurrency.run(() => { @@ -405,7 +404,7 @@ ipcMain.handle(IPC_CHANNELS.PATHS_MANAGER.EXTRACT_ON_PATH, async (event, id: str safeId, IPC_CHANNELS.PATHS_MANAGER.EXTRACT_PROGRESS, extractWorker, - { filePath: safeFilePath, outputPath: safeOutputPath, deleteZip: shouldDeleteZip, sevenZipBin }, + { filePath: safeFilePath, outputPath: safeOutputPath, deleteZip: shouldDeleteZip, unwrapSingleRootFolder: shouldUnwrapSingleRootFolder }, "EXTRACT_ON_PATH", () => true ) @@ -581,7 +580,7 @@ ipcMain.handle( safeId, IPC_CHANNELS.PATHS_MANAGER.COMPRESS_PROGRESS, compressWorker, - { inputPath: safeInputPath, outputPath: safeOutputPath, outputFileName: safeOutputFileName, compressionLevel: safeCompressionLevel, sevenZipBin }, + { inputPath: safeInputPath, outputPath: safeOutputPath, outputFileName: safeOutputFileName, compressionLevel: safeCompressionLevel }, "COMPRESS_ON_PATH", () => true ) diff --git a/src/ipc/workers/extractWorker.ts b/src/ipc/workers/extractWorker.ts index 86ba73cd..c233dbc5 100644 --- a/src/ipc/workers/extractWorker.ts +++ b/src/ipc/workers/extractWorker.ts @@ -3,8 +3,8 @@ import { runExtraction } from "@src/ipc/workers/extraction" serveTasks( async (payload, onProgress) => { - const { filePath, outputPath, deleteZip, sevenZipBin } = payload as { filePath: string; outputPath: string; deleteZip: boolean; sevenZipBin: string } - await runExtraction({ filePath, outputPath, deleteArchive: deleteZip, sevenZipBin, onProgress }) + const { filePath, outputPath, deleteZip, unwrapSingleRootFolder } = payload as { filePath: string; outputPath: string; deleteZip: boolean; unwrapSingleRootFolder: boolean } + await runExtraction({ filePath, outputPath, deleteArchive: deleteZip, unwrapSingleRootFolder, onProgress }) }, (error) => (error instanceof Error ? error.message : "Extraction failed") ) diff --git a/src/ipc/workers/extraction.ts b/src/ipc/workers/extraction.ts index 7fb320da..087ec7eb 100644 --- a/src/ipc/workers/extraction.ts +++ b/src/ipc/workers/extraction.ts @@ -9,10 +9,10 @@ * write a single byte where the launcher keeps its files. */ -import Seven from "node-7z" import fse from "fs-extra" -import { createReadStream, mkdtempSync } from "node:fs" -import { isAbsolute, join, relative, resolve, sep } from "node:path" +import yauzl from "yauzl" +import { createReadStream, createWriteStream, mkdtempSync } from "node:fs" +import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path" import { tmpdir } from "node:os" import * as tar from "tar" @@ -130,39 +130,138 @@ export function contentRoot(root: string): string { return fse.lstatSync(candidate).isDirectory() ? candidate : root } -function extractWithSevenZip(filePath: string, destination: string, sevenZipBin: string, onProgress?: (progress: number) => void): Promise { +/** + * Where one archive entry is allowed to land, or nothing. + * + * The archive's table of contents was already checked for escaping names before + * a byte was written (validateArchive, in archiveValidation.ts), and yauzl + * refuses a leading "/" or a ".." segment on its own before either. This is the + * check the writer itself makes anyway, on the resolved path rather than on the + * name, because a writer is the wrong place to trust a name that came out of a + * file someone else wrote. + */ +export function resolveEntryDestination(destination: string, entryName: string): string { + const resolvedDestination = resolve(destination) + const target = resolve(resolvedDestination, entryName.replaceAll("\\", "/")) + const relativePath = relative(resolvedDestination, target) + if (!relativePath || relativePath === ".." || relativePath.startsWith(`..${sep}`) || isAbsolute(relativePath)) throw new Error("Archive entry escaped its root") + return target +} + +/** + * Unpacks a zip. + * + * Only the backups the launcher wrote before it moved to gzipped tar arrive + * here, and they have to keep restoring for as long as players still hold them. + * Reading is yauzl's, the same reader the mod archives and the pre-extraction + * table-of-contents check already run on, so no zip writer or external process + * is needed to keep the old format readable. + * + * Progress is counted in entries rather than bytes: the entry count is the one + * total a zip states up front, per entry sizes are what the archive claims + * rather than what comes out, and a backup's entries are of a similar size. + */ +export function extractZip(filePath: string, destination: string, onProgress?: (progress: number) => void): Promise { return new Promise((resolvePromise, rejectPromise) => { - const stream = Seven.extractFull(filePath, destination, { $bin: sevenZipBin, $progress: true, recursive: true }) - let lastReportedProgress = 0 + yauzl.open(filePath, { lazyEntries: true }, (openError, zipFile) => { + if (openError || !zipFile) { + rejectPromise(new Error("Extraction failed")) + return + } - stream.on("progress", ({ percent }) => { - const boundedPercent = Number(percent) - // runExtraction emits the terminal 100 after validation and copying. Do not - // let 7-Zip publish a second terminal event before that point. - if (Number.isFinite(boundedPercent) && boundedPercent > lastReportedProgress && boundedPercent < 100) { - lastReportedProgress = boundedPercent - onProgress?.(boundedPercent) + let settled = false + let extractedEntries = 0 + let lastReportedProgress = 0 + const totalEntries = zipFile.entryCount + + const finish = (failure?: Error): void => { + if (settled) return + settled = true + try { + zipFile.close() + } catch { + // Already closed after a parse error. The outcome below is what matters. + } + if (failure) rejectPromise(failure) + else resolvePromise() } - }) - stream.on("end", () => resolvePromise()) - stream.on("error", () => rejectPromise(new Error("Extraction failed"))) + const advance = (): void => { + extractedEntries++ + if (totalEntries <= 0) return + // runExtraction emits the terminal 100 after validation and copying, so + // the running figure stops a point short of it. + const progress = Math.min(99, Math.floor((extractedEntries / totalEntries) * 100)) + if (progress > lastReportedProgress) { + lastReportedProgress = progress + onProgress?.(progress) + } + } + + zipFile.on("entry", (entry: yauzl.Entry) => { + let target: string + try { + target = resolveEntryDestination(destination, entry.fileName) + } catch (error) { + finish(error as Error) + return + } + + if (entry.fileName.endsWith("/")) { + try { + fse.ensureDirSync(target) + } catch { + finish(new Error("Extraction failed")) + return + } + advance() + zipFile.readEntry() + return + } + + zipFile.openReadStream(entry, (streamError, readStream) => { + if (streamError || !readStream) { + finish(new Error("Extraction failed")) + return + } + + let writeStream: ReturnType + try { + fse.ensureDirSync(dirname(target)) + writeStream = createWriteStream(target) + } catch { + readStream.destroy() + finish(new Error("Extraction failed")) + return + } + + readStream.on("error", () => finish(new Error("Extraction failed"))) + writeStream.on("error", () => finish(new Error("Extraction failed"))) + writeStream.on("close", () => { + if (settled) return + advance() + zipFile.readEntry() + }) + readStream.pipe(writeStream) + }) + }) + + zipFile.on("end", () => finish()) + zipFile.on("error", () => finish(new Error("Extraction failed"))) + zipFile.readEntry() + }) }) } /** - * Unpacks a gzipped tar. - * - * 7-Zip is not used here. It needs two passes for a `.tar.gz`, and the bundled - * p7zip 16.02 cannot read the tar Vintage Story ships at all: its headers leave - * the numeric fields as NUL bytes, which GNU tar and node-tar accept and 7-Zip - * rejects with "Is not archive". + * Unpacks a gzipped tar: the game builds, and every backup the launcher writes. * * Progress comes from the compressed bytes read, which is the only total known * up front. Anything that is not a plain file or folder fails the extraction - * rather than being skipped quietly. + * rather than being skipped quietly, and `preservePaths: false` is what keeps + * an absolute or climbing entry name from being written where it points. */ -function extractTarGz(filePath: string, destination: string, onProgress?: (progress: number) => void): Promise { +export function extractTarGz(filePath: string, destination: string, onProgress?: (progress: number) => void): Promise { return new Promise((resolvePromise, rejectPromise) => { const totalBytes = fse.statSync(filePath).size let readBytes = 0 @@ -220,8 +319,13 @@ export interface ExtractionOptions { outputPath: string /** Whether the archive is deleted once its contents landed. */ deleteArchive: boolean - /** 7-Zip binary, used for every format outside the tar.gz family. */ - sevenZipBin: string + /** + * Whether a single wrapping folder is stepped into before the contents are + * copied out. Asked for by the game version install, whose Linux archives + * carry everything under `vintagestory/`, and never by a backup restore, + * whose archive holds an installation's contents at the root already. + */ + unwrapSingleRootFolder?: boolean /** Called with 0 to 100 as the work advances. */ onProgress?: (progress: number) => void } @@ -234,14 +338,14 @@ export interface ExtractionOptions { * files and folders, or busts the entry and size bounds. */ export async function runExtraction(options: ExtractionOptions): Promise { - const { filePath, outputPath, deleteArchive, sevenZipBin, onProgress } = options + const { filePath, outputPath, deleteArchive, unwrapSingleRootFolder = false, onProgress } = options let temporaryRoot: string | undefined // The first of two validation gates (see archiveValidation.ts's own comment): reads the // archive's table of contents and refuses it, before a single byte is written anywhere, // if it names an entry outside its root, repeats a name, carries a link, or busts the // entry/size bounds. - await validateArchive(filePath, sevenZipBin) + await validateArchive(filePath) try { assertNoSymlinkComponents(outputPath) @@ -252,15 +356,14 @@ export async function runExtraction(options: ExtractionOptions): Promise { const extractionRoot = join(temporaryRoot, "payload") fse.ensureDirSync(extractionRoot) - const isGameArchive = isTarGzName(filePath) - if (isGameArchive) await extractTarGz(filePath, extractionRoot, onProgress) - else await extractWithSevenZip(filePath, extractionRoot, sevenZipBin, onProgress) + if (isTarGzName(filePath)) await extractTarGz(filePath, extractionRoot, onProgress) + else await extractZip(filePath, extractionRoot, onProgress) validateTree(extractionRoot) - // Only the game archives wrap their contents in a folder. The zips reaching - // here are backups the launcher wrote itself, holding an installation's + // Only the game archives wrap their contents in a folder, and only their caller + // asks for that folder to be stepped into. A backup holds an installation's // contents at the root, and a restore has to put them back exactly as they were. - copyTree(isGameArchive ? contentRoot(extractionRoot) : extractionRoot, outputPath) + copyTree(unwrapSingleRootFolder ? contentRoot(extractionRoot) : extractionRoot, outputPath) if (deleteArchive) { assertNoSymlinkComponents(filePath) diff --git a/src/preload/index.ts b/src/preload/index.ts index 47993927..7437d6e5 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -54,8 +54,8 @@ const api: BridgeAPI = { ensurePathExists: (path: string): Promise => ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.ENSURE_PATH_EXISTS, path), openPathOnFileExplorer: (path: string): Promise => ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.OPEN_PATH_ON_FILE_EXPLORER, path), downloadOnPath: (id: string, url: string, outputPath: string, fileName: string): Promise => ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.DOWNLOAD_ON_PATH, id, url, outputPath, fileName), - extractOnPath: (id: string, filePath: string, outputPath: string, deleteZip: boolean): Promise => - ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.EXTRACT_ON_PATH, id, filePath, outputPath, deleteZip), + extractOnPath: (id: string, filePath: string, outputPath: string, deleteZip: boolean, unwrapSingleRootFolder?: boolean): Promise => + ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.EXTRACT_ON_PATH, id, filePath, outputPath, deleteZip, unwrapSingleRootFolder ?? false), runInstaller: (id: string, filePath: string, outputPath: string, deleteInstaller: boolean): Promise => ipcRenderer.invoke(IPC_CHANNELS.PATHS_MANAGER.RUN_INSTALLER, id, filePath, outputPath, deleteInstaller), compressOnPath: (id: string, inputPath: string, outputPath: string, outputFileName: string, compressionLevel?: number): Promise => diff --git a/src/preload/preload.d.ts b/src/preload/preload.d.ts index 0b78b56b..d9c510a9 100644 --- a/src/preload/preload.d.ts +++ b/src/preload/preload.d.ts @@ -54,7 +54,7 @@ declare global { ensurePathExists: (path: string) => Promise openPathOnFileExplorer: (path: string) => Promise downloadOnPath: (id: string, url: string, outputPath: string, fileName: string) => Promise - extractOnPath: (id: string, filePath: string, outputPath: string, deleteZip: boolean) => Promise + extractOnPath: (id: string, filePath: string, outputPath: string, deleteZip: boolean, unwrapSingleRootFolder?: boolean) => Promise runInstaller: (id: string, filePath: string, outputPath: string, deleteInstaller: boolean) => Promise compressOnPath: (id: string, inputPath: string, outputPath: string, outputFileName: string, compressionLevel?: number) => Promise onDownloadProgress: (callback: ProgressCallback) => Unsubscribe diff --git a/src/renderer/src/contexts/TaskManagerContext.tsx b/src/renderer/src/contexts/TaskManagerContext.tsx index 4b7bdd33..1bca1451 100644 --- a/src/renderer/src/contexts/TaskManagerContext.tsx +++ b/src/renderer/src/contexts/TaskManagerContext.tsx @@ -120,7 +120,8 @@ export interface TaskContextType { filePath: string, outputPath: string, deleteZip: boolean, - onFinish: (status: boolean, error: Error | null) => void + onFinish: (status: boolean, error: Error | null) => void, + unwrapSingleRootFolder?: boolean ): Promise startInstall( name: string, @@ -270,7 +271,8 @@ export const TaskProvider = ({ children }: { children: React.ReactNode }): JSX.E filePath: string, outputPath: string, deleteZip: boolean, - onFinish: (status: boolean, error: Error | null) => void + onFinish: (status: boolean, error: Error | null) => void, + unwrapSingleRootFolder = false ): Promise { const id = crypto.randomUUID() @@ -281,7 +283,7 @@ export const TaskProvider = ({ children }: { children: React.ReactNode }): JSX.E window.api.utils.logMessage("info", `[front] [tasks] [contexts/TaskManagercontext.tsx] [TaskProvider > startExtract] [${id}] [${filePath}] Extracting...`) if (showsStart(notifications)) addNotification(t("notifications.body.extracting", { extractName: name }), "info") - const result = await window.api.pathsManager.extractOnPath(id, filePath, outputPath, deleteZip) + const result = await window.api.pathsManager.extractOnPath(id, filePath, outputPath, deleteZip, unwrapSingleRootFolder) if (!result) throw new Error("Extraction failed") diff --git a/src/renderer/src/features/versions/adapters/install.ts b/src/renderer/src/features/versions/adapters/install.ts index 06e66d37..14bb5095 100644 --- a/src/renderer/src/features/versions/adapters/install.ts +++ b/src/renderer/src/features/versions/adapters/install.ts @@ -38,8 +38,11 @@ export function createInstallPorts({ startDownload, startExtract, startInstall, ) }, unpacker: { + // The Linux game archives carry everything under a single `vintagestory/` + // folder, which is the one case the extraction steps into. A backup restore + // asks for the opposite and gets the default. extractArchive: (request, onComplete) => - startExtract(taskName, unpackDescription, "progress", request.sourcePath, request.outputFolder, true, (status, error) => onComplete({ ok: status, error: error?.message })), + startExtract(taskName, unpackDescription, "progress", request.sourcePath, request.outputFolder, true, (status, error) => onComplete({ ok: status, error: error?.message }), true), runInstaller: (request, onComplete) => startInstall(taskName, unpackDescription, "progress", request.sourcePath, request.outputFolder, true, (status, error) => onComplete({ ok: status, error: error?.message })) } diff --git a/tests/domain/installations/restore.test.ts b/tests/domain/installations/restore.test.ts index 6f8d5995..0d28a1ba 100644 --- a/tests/domain/installations/restore.test.ts +++ b/tests/domain/installations/restore.test.ts @@ -10,7 +10,7 @@ const TOKEN = "token-1" const INSTALLATION_PATH = "/games/my-install" const STAGING_PATH = `${INSTALLATION_PATH}-restoring-${TOKEN}` const REPLACED_PATH = `${INSTALLATION_PATH}-replaced-${TOKEN}` -const ARCHIVE_PATH = "/backups/my-install.zip" +const ARCHIVE_PATH = "/backups/my-install.tar.gz" /** Everything the fakes wrote down, in the order it happened. */ let trace: string[] = [] diff --git a/tests/fixtures/build-fixtures.ts b/tests/fixtures/build-fixtures.ts index 8868e10a..ad606e10 100644 --- a/tests/fixtures/build-fixtures.ts +++ b/tests/fixtures/build-fixtures.ts @@ -1,8 +1,12 @@ /** - * Hand-builds the tiny zip fixtures under tests/fixtures/ that - * tests/ipc/modScan.test.ts reads readModArchive against, so the yauzl edge - * cases in src/ipc/adapters/modScan.ts are exercised against real archive - * bytes instead of only through the domain's fakes. + * Hand-builds the tiny zip fixtures under tests/fixtures/. + * + * Most of them are what tests/ipc/modScan.test.ts reads readModArchive + * against, so the yauzl edge cases in src/ipc/adapters/modScan.ts are + * exercised against real archive bytes instead of only through the domain's + * fakes. The last two are backups: the launcher wrote its backups as zip up to + * 1.7.0-beta.4, and tests/ipc/extraction.test.ts restores them here so a + * player's old backup keeps working long after the writer went away. * * This script is not run by the test suite. It is the documentation of what * each fixture contains: run it by hand after changing it, @@ -359,3 +363,39 @@ write("zip64.zip", assembleZip64([{ name: "modinfo.json", method: METHOD_STORE, // receives the error (no End of Central Directory Record signature found), // before a single "entry" event, exercising readModArchive's openErr branch. write("not-a-zip.bin", Buffer.from("this is not a zip archive, just some bytes", "utf8")) + +// --- legacy-backup.zip -------------------------------------------------- +// A backup the launcher wrote before it moved to gzipped tar: an installation's +// contents at the root of the archive, no wrapping folder, with a folder entry +// and a zero byte file in it. tests/ipc/extraction.test.ts restores this one and +// compares the tree byte for byte. +write( + "legacy-backup.zip", + assembleZip([ + { name: "Vintagestory", method: METHOD_STORE, realBytes: Buffer.from("elf", "utf8") }, + { name: "assets/", method: METHOD_STORE, realBytes: Buffer.alloc(0) }, + { name: "assets/version-1.22.6.txt", method: METHOD_STORE, realBytes: Buffer.alloc(0) }, + { name: "Mods/", method: METHOD_STORE, realBytes: Buffer.alloc(0) }, + { name: "Mods/notes.txt", method: METHOD_DEFLATE, realBytes: Buffer.from("a mod list worth keeping\n", "utf8") } + ]) +) + +// --- hostile-backup.zip ------------------------------------------------- +// The same shape, with three entry names that point outside the folder they +// would be unpacked into: a Windows drive letter, a climb with "..", and a +// unix absolute path. None is a name any writer produces by accident, and +// nothing may be written for any of them. +// +// The drive-letter one is first on purpose. It is the one yauzl itself lets +// through (its own name validation refuses a leading "/" and any ".." segment, +// and stops the read there), so putting it first is what makes the launcher's +// own isSafeArchiveEntry check the gate that speaks, which is the check worth +// having a test hold in place. +write( + "hostile-backup.zip", + assembleZip([ + { name: "C:/escaped-drive.txt", method: METHOD_STORE, realBytes: Buffer.from("drive letter", "utf8") }, + { name: "../escaped.txt", method: METHOD_STORE, realBytes: Buffer.from("climbed out", "utf8") }, + { name: "/etc/escaped-absolute.txt", method: METHOD_STORE, realBytes: Buffer.from("absolute", "utf8") } + ]) +) diff --git a/tests/fixtures/hostile-backup.zip b/tests/fixtures/hostile-backup.zip new file mode 100644 index 0000000000000000000000000000000000000000..f8198a3d60694c368ca363c21a5a8a4341466541 GIT binary patch literal 399 zcmWIWW@Zs#fB;1XRr5~Sefrx#{G!U#oU{NG#Sl#pk{w7T78j?M z6zk(wB#BT|mReMtnV+X?sApuPXQo$DQG#0=D^QznehR{54xoS{LyXzK)zUx~$W%U< zlDzzq)MAiv-XSN>`UjpksiPODab92RWWWhOU)>XDyq_~M1b8zti7?>yG|((0fE)zq zI+2tyfI { it("accepts a gzipped tar, which is what the Linux game builds ship as", async () => { const archivePath = await makeTarGz("vs_client_linux-x64_1.22.6.tar.gz", ["vintagestory"]) - await assert.doesNotReject(async () => validateArchive(archivePath, sevenZipBin)) + await assert.doesNotReject(async () => validateArchive(archivePath)) }) it("accepts the same archive under the .tgz spelling", async () => { const archivePath = await makeTarGz("build.tgz", ["vintagestory"]) - await assert.doesNotReject(async () => validateArchive(archivePath, sevenZipBin)) + await assert.doesNotReject(async () => validateArchive(archivePath)) }) it("refuses a gzipped tar carrying a symbolic link", async () => { symlinkSync("/etc/passwd", workspacePath("source", "vintagestory", "escape.txt")) const archivePath = await makeTarGz("hostile.tar.gz", ["vintagestory"]) - await assert.rejects(validateArchive(archivePath, sevenZipBin), /unsafe entry/) + await assert.rejects(validateArchive(archivePath), /unsafe entry/) }) it("refuses a gzipped tar naming an entry outside its root", async () => { const archivePath = workspacePath("escaping.tar.gz") await tar.create({ file: archivePath, gzip: true, cwd: workspacePath("source"), portable: true, preservePaths: true }, ["../source/vintagestory/Vintagestory"]) - await assert.rejects(validateArchive(archivePath, sevenZipBin), /unsafe entry/) + await assert.rejects(validateArchive(archivePath), /unsafe entry/) }) it("refuses a file that only claims to be a gzipped tar", async () => { writeFileSync(workspacePath("liar.tar.gz"), "definitely not gzip") - await assert.rejects(validateArchive(workspacePath("liar.tar.gz"), sevenZipBin), /could not be read/) + await assert.rejects(validateArchive(workspacePath("liar.tar.gz")), /could not be read/) }) - it("still reads a zip with the zip reader", async () => { - const archivePath = workspacePath("backup.zip") - execFileSync(sevenZipBin, ["a", "-tzip", archivePath, workspacePath("source", "vintagestory")], { stdio: "ignore" }) + it("still reads a zip with the zip reader, which is what an old backup is", async () => { + await assert.doesNotReject(async () => validateArchive(join(FIXTURES, "legacy-backup.zip"))) + }) - await assert.doesNotReject(async () => validateArchive(archivePath, sevenZipBin)) + it("refuses a zip whose entries name places outside its root", async () => { + await assert.rejects(validateArchive(join(FIXTURES, "hostile-backup.zip")), /could not be read/) }) it("refuses a zip that cannot be parsed", async () => { writeFileSync(workspacePath("broken.zip"), "PK not really") - await assert.rejects(validateArchive(workspacePath("broken.zip"), sevenZipBin), /could not be read/) + await assert.rejects(validateArchive(workspacePath("broken.zip")), /could not be read/) }) - it("falls back to 7-Zip for anything else", async () => { - const archivePath = workspacePath("backup.7z") - execFileSync(sevenZipBin, ["a", "-t7z", archivePath, workspacePath("source", "vintagestory")], { stdio: "ignore" }) - - await assert.doesNotReject(async () => validateArchive(archivePath, sevenZipBin)) - await assert.rejects(validateArchive(workspacePath("source", "vintagestory", "Vintagestory"), sevenZipBin), /could not be listed/) + it("refuses anything that is neither a zip nor a gzipped tar", async () => { + // Two formats reach the launcher and no others, so a third is refused by + // name rather than handed to a reader that would have to guess at it. + await assert.rejects(validateArchive(workspacePath("source", "vintagestory", "Vintagestory")), /not supported/) + await assert.rejects(validateArchive(join(FIXTURES, "not-a-zip.bin")), /not supported/) }) }) diff --git a/tests/ipc/extraction.test.ts b/tests/ipc/extraction.test.ts index 0d6489ff..cf40e0b4 100644 --- a/tests/ipc/extraction.test.ts +++ b/tests/ipc/extraction.test.ts @@ -1,22 +1,25 @@ import assert from "node:assert/strict" -import { execFileSync } from "node:child_process" import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" -import { join } from "node:path" +import { join, relative, resolve } from "node:path" import { afterEach, beforeEach, describe, it } from "vitest" import * as tar from "tar" -import { path7za } from "7zip-bin" -import { contentRoot, runExtraction, validateTree } from "../../src/ipc/workers/extraction" +import { contentRoot, extractTarGz, extractZip, resolveEntryDestination, runExtraction, validateTree } from "../../src/ipc/workers/extraction" +import { runCompression } from "../../src/ipc/workers/compression" /** - * These build their own archives, so they run anywhere without a network. + * These build their own archives, so they run anywhere without a network. The + * two zip fixtures are the exception, and they are committed rather than built: + * the whole point of keeping a zip reader is that backups written by a version + * of the launcher nobody runs any more still restore. See + * tests/fixtures/build-fixtures.ts for what is in them. * * The one test that needs a real Vintage Story archive is opt in: point * RIFT_E2E_ARCHIVE at a downloaded game tar.gz to run it. CI never does. */ -const sevenZipBin = path7za +const FIXTURES = join(__dirname, "..", "fixtures") let workspace: string @@ -122,7 +125,7 @@ describe("runExtraction on a gzipped tar", () => { writeTree(workspacePath("source"), { vintagestory: { Vintagestory: "elf", assets: { "version-1.22.6.txt": "", "seed.json": "{}" } } }) const archivePath = await makeTarGz("vs_client_linux-x64_1.22.6.tar.gz", workspacePath("source")) - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }) assert.deepEqual(readdirSync(workspacePath("target")).sort(), ["Vintagestory", "assets"]) assert.equal(readFileSync(workspacePath("target", "Vintagestory"), "utf8"), "elf") @@ -133,7 +136,7 @@ describe("runExtraction on a gzipped tar", () => { writeTree(workspacePath("source"), { vintagestory: { Vintagestory: "elf", assets: { "version-1.22.6.txt": "" } } }) const archivePath = await makeTarGz("wrapped.tar.gz", workspacePath("source")) - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }) const marker = workspacePath("target", "assets", "version-1.22.6.txt") assert.equal(existsSync(marker), true) @@ -144,7 +147,7 @@ describe("runExtraction on a gzipped tar", () => { writeTree(workspacePath("source"), { VintagestoryServer: "elf", assets: { "version-1.22.6.txt": "" } }) const archivePath = await makeTarGz("vs_server_linux-x64_1.22.6.tar.gz", workspacePath("source")) - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }) assert.deepEqual(readdirSync(workspacePath("target")).sort(), ["VintagestoryServer", "assets"]) assert.equal(existsSync(workspacePath("target", "assets", "version-1.22.6.txt")), true) @@ -155,7 +158,7 @@ describe("runExtraction on a gzipped tar", () => { const archivePath = await makeTarGz("progress.tar.gz", workspacePath("source")) const reported: number[] = [] - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin, onProgress: (progress) => reported.push(progress) }) + await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true, onProgress: (progress) => reported.push(progress) }) assert.equal(reported.at(-1), 100) assert.equal(reported.filter((progress) => progress === 100).length, 1) @@ -171,8 +174,8 @@ describe("runExtraction on a gzipped tar", () => { const keptPath = await makeTarGz("kept.tar.gz", workspacePath("source")) const consumedPath = await makeTarGz("consumed.tar.gz", workspacePath("source")) - await runExtraction({ filePath: keptPath, outputPath: workspacePath("kept"), deleteArchive: false, sevenZipBin }) - await runExtraction({ filePath: consumedPath, outputPath: workspacePath("consumed"), deleteArchive: true, sevenZipBin }) + await runExtraction({ filePath: keptPath, outputPath: workspacePath("kept"), deleteArchive: false, unwrapSingleRootFolder: true }) + await runExtraction({ filePath: consumedPath, outputPath: workspacePath("consumed"), deleteArchive: true, unwrapSingleRootFolder: true }) assert.equal(existsSync(keptPath), true) assert.equal(existsSync(consumedPath), false) @@ -183,7 +186,7 @@ describe("runExtraction on a gzipped tar", () => { symlinkSync("/etc/passwd", workspacePath("source", "vintagestory", "escape.txt")) const archivePath = await makeTarGz("hostile.tar.gz", workspacePath("source")) - await assert.rejects(runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }), /unsafe entry/) + await assert.rejects(runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }), /unsafe entry/) assert.equal(existsSync(workspacePath("target", "escape.txt")), false) assert.equal(existsSync(workspacePath("target", "Vintagestory")), false) }) @@ -194,7 +197,10 @@ describe("runExtraction on a gzipped tar", () => { // Caught by runExtraction's own pre-extraction validateArchive call, before it even // creates the target directory, let alone extracts (and its generic "Extraction failed") // is ever reached. - await assert.rejects(runExtraction({ filePath: workspacePath("broken.tar.gz"), outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }), /Archive could not be read/) + await assert.rejects( + runExtraction({ filePath: workspacePath("broken.tar.gz"), outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }), + /Archive could not be read/ + ) assert.equal(existsSync(workspacePath("target")), false) }) @@ -203,43 +209,186 @@ describe("runExtraction on a gzipped tar", () => { writeTree(workspacePath("source"), { vintagestory: { Vintagestory: "elf" } }) const archivePath = await makeTarGz("clean.tar.gz", workspacePath("source")) - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }) assert.equal(readdirSync(tmpdir()).filter((entry) => entry.startsWith("vs-launcher-extract-")).length, before) }) }) -describe("runExtraction on a zip", () => { - it("keeps a zip's own shape, because those are the backups a restore puts back", async () => { - writeTree(workspacePath("source"), { vintagestory: { Vintagestory: "elf", assets: { "version-1.22.6.txt": "" } } }) - const archivePath = workspacePath("backup.zip") - execFileSync(sevenZipBin, ["a", "-tzip", archivePath, join(workspacePath("source"), "vintagestory")], { stdio: "ignore" }) +/** Every file in a tree, as a relative path to its bytes, so two trees can be compared outright. */ +function readTree(root: string): Record { + const files: Record = {} + const visit = (current: string): void => { + for (const name of readdirSync(current)) { + const entry = join(current, name) + if (lstatSync(entry).isDirectory()) visit(entry) + else files[relative(root, entry).replaceAll("\\", "/")] = readFileSync(entry, "base64") + } + } + visit(root) + return files +} + +describe("backup round trip", () => { + it("puts a freshly written backup back exactly as it was", async () => { + writeTree(workspacePath("installation"), { + Mods: { "carrycapacity.zip": "not really a mod", "notes.txt": "" }, + "clientsettings.json": "{}", + Saves: { "world.vcdbs": " binary-ish" } + }) + const before = readTree(workspacePath("installation")) + + await runCompression({ inputPath: workspacePath("installation"), outputPath: workspacePath("backups"), outputFileName: "backup.tar.gz" }) + await runExtraction({ filePath: workspacePath("backups", "backup.tar.gz"), outputPath: workspacePath("restored"), deleteArchive: false }) + + assert.deepEqual(readTree(workspacePath("restored")), before) + assert.deepEqual(readdirSync(workspacePath("restored")).sort(), readdirSync(workspacePath("installation")).sort()) + }) + + it("puts a backup of an empty installation back as an empty folder", async () => { + mkdirSync(workspacePath("installation"), { recursive: true }) + + await runCompression({ inputPath: workspacePath("installation"), outputPath: workspacePath("backups"), outputFileName: "backup.tar.gz" }) + await runExtraction({ filePath: workspacePath("backups", "backup.tar.gz"), outputPath: workspacePath("restored"), deleteArchive: false }) - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + assert.deepEqual(readdirSync(workspacePath("restored")), []) + }) + + it("never steps into a single folder a backup happens to hold, the way a game archive is stepped into", async () => { + // An installation whose only entry is a folder is not a wrapped archive: it + // is an installation with one folder in it, and a restore has to put that + // folder back rather than spill its contents over the installation root. + writeTree(workspacePath("installation"), { Mods: { "notes.txt": "one folder, nothing else" } }) + + await runCompression({ inputPath: workspacePath("installation"), outputPath: workspacePath("backups"), outputFileName: "backup.tar.gz" }) + await runExtraction({ filePath: workspacePath("backups", "backup.tar.gz"), outputPath: workspacePath("restored"), deleteArchive: false }) - assert.deepEqual(readdirSync(workspacePath("target")), ["vintagestory"]) - assert.equal(statSync(workspacePath("target", "vintagestory", "assets", "version-1.22.6.txt")).size, 0) + assert.deepEqual(readdirSync(workspacePath("restored")), ["Mods"]) + assert.equal(readFileSync(workspacePath("restored", "Mods", "notes.txt"), "utf8"), "one folder, nothing else") }) +}) - it("coalesces 7-Zip progress and emits one terminal 100", async () => { - const source = workspacePath("many-files") - mkdirSync(source, { recursive: true }) - for (let index = 0; index < 2_000; index++) writeFileSync(join(source, `file-${index}.bin`), Buffer.alloc(2_048, index % 251)) +describe("runExtraction on a legacy zip backup", () => { + it("restores a backup written before the launcher moved off zip", async () => { + await runExtraction({ filePath: join(FIXTURES, "legacy-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false }) + + assert.deepEqual(readTree(workspacePath("restored")), { + Vintagestory: Buffer.from("elf").toString("base64"), + "assets/version-1.22.6.txt": "", + "Mods/notes.txt": Buffer.from("a mod list worth keeping\n").toString("base64") + }) + // A folder entry lands as a folder, and the zero byte marker inside it stays zero bytes. + assert.equal(lstatSync(workspacePath("restored", "assets")).isDirectory(), true) + assert.equal(statSync(workspacePath("restored", "assets", "version-1.22.6.txt")).size, 0) + }) - const archivePath = workspacePath("progress.zip") - execFileSync(sevenZipBin, ["a", "-tzip", archivePath, source], { stdio: "ignore" }) + it("keeps a zip's own shape, because those are the backups a restore puts back", async () => { + await runExtraction({ filePath: join(FIXTURES, "legacy-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false }) + + assert.deepEqual(readdirSync(workspacePath("restored")).sort(), ["Mods", "Vintagestory", "assets"]) + }) + + it("coalesces progress and emits one terminal 100", async () => { const reported: number[] = [] - await runExtraction({ filePath: archivePath, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin, onProgress: (progress) => reported.push(progress) }) + await runExtraction({ filePath: join(FIXTURES, "legacy-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false, onProgress: (progress) => reported.push(progress) }) assert.equal(reported.at(-1), 100) assert.equal(reported.filter((progress) => progress === 100).length, 1) assert.equal(new Set(reported).size, reported.length) + assert.deepEqual( + [...reported].sort((left, right) => left - right), + reported + ) assert.equal( reported.some((progress) => progress > 0 && progress < 100), true ) }) + + it("deletes the archive when asked", async () => { + const consumedPath = workspacePath("consumed.zip") + writeFileSync(consumedPath, readFileSync(join(FIXTURES, "legacy-backup.zip"))) + + await runExtraction({ filePath: consumedPath, outputPath: workspacePath("restored"), deleteArchive: true }) + + assert.equal(existsSync(consumedPath), false) + }) + + it("refuses a zip naming entries outside the folder it unpacks into", async () => { + // The refusal comes from yauzl's own name validation, which stops the read + // at the first entry that names a drive, a "/" root or a ".." segment, so + // the launcher's table-of-contents pass reports an unreadable archive + // rather than an unsafe entry. Either way nothing is written, which is the + // property worth holding: the archive is refused before the output folder + // is even created. + await assert.rejects(runExtraction({ filePath: join(FIXTURES, "hostile-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false }), /could not be read/) + + assert.equal(existsSync(workspacePath("escaped.txt")), false) + assert.equal(existsSync(workspacePath("escaped-drive.txt")), false) + assert.equal(existsSync("/etc/escaped-absolute.txt"), false) + assert.equal(existsSync(workspacePath("restored")), false) + }) + + it("refuses an archive whose name says neither zip nor tar.gz", async () => { + await assert.rejects(runExtraction({ filePath: join(FIXTURES, "not-a-zip.bin"), outputPath: workspacePath("restored"), deleteArchive: false }), /not supported/) + }) +}) + +/** + * The unpackers' own look at an entry name, past the table-of-contents check + * that normally refuses these archives before either unpacker is reached. What + * is inside an archive is not something the launcher wrote, so both gates get + * pinned rather than only the first. + */ +describe("hostile entry names, straight at the unpackers", () => { + it("refuses to place a zip entry that climbs out, is absolute, or names a drive", () => { + const destination = workspacePath("destination", "inside") + + assert.throws(() => resolveEntryDestination(destination, "../escaped.txt"), /escaped its root/) + assert.throws(() => resolveEntryDestination(destination, "assets/../../escaped.txt"), /escaped its root/) + assert.throws(() => resolveEntryDestination(destination, "/etc/escaped-absolute.txt"), /escaped its root/) + // A backslash separator is a Windows path inside a zip, and is normalised + // before the comparison rather than taken as part of a file name. + assert.throws(() => resolveEntryDestination(destination, "..\\escaped.txt"), /escaped its root/) + assert.throws(() => resolveEntryDestination(destination, "."), /escaped its root/) + }) + + it("places an ordinary zip entry under the destination", () => { + const destination = workspacePath("destination", "inside") + + assert.equal(resolveEntryDestination(destination, "assets/version-1.22.6.txt"), join(destination, "assets", "version-1.22.6.txt")) + assert.equal(resolveEntryDestination(destination, "Mods\\notes.txt"), join(destination, "Mods", "notes.txt")) + }) + + it("writes nothing outside the destination for a zip full of escaping names", async () => { + const destination = workspacePath("destination", "inside") + mkdirSync(destination, { recursive: true }) + + await assert.rejects(extractZip(join(FIXTURES, "hostile-backup.zip"), destination)) + + assert.equal(existsSync(workspacePath("destination", "escaped.txt")), false) + assert.equal(existsSync(workspacePath("escaped.txt")), false) + assert.equal(existsSync("/etc/escaped-absolute.txt"), false) + assert.deepEqual(readdirSync(destination), []) + }) + + it("writes nothing outside the destination for a tar.gz climbing out with .. or an absolute path", async () => { + writeTree(workspacePath("source"), { "escaped.txt": "climbed out" }) + const archivePath = workspacePath("hostile-names.tar.gz") + await tar.create({ file: archivePath, gzip: true, cwd: workspacePath("source"), portable: true, preservePaths: true }, ["../source/escaped.txt", resolve(workspacePath("source", "escaped.txt"))]) + const destination = workspacePath("destination", "inside") + mkdirSync(destination, { recursive: true }) + + await extractTarGz(archivePath, destination) + + assert.equal(existsSync(workspacePath("destination", "escaped.txt")), false) + assert.equal(existsSync(workspacePath("escaped.txt")), false) + // Whatever did land is under the destination, nowhere else. + for (const landed of readdirSync(destination, { recursive: true }) as string[]) { + assert.equal(resolve(destination, landed).startsWith(`${resolve(destination)}/`), true) + } + }) }) /** @@ -253,7 +402,7 @@ describe.skipIf(!realArchive)("runExtraction on a real Vintage Story archive", ( it("puts the game executable and the version marker straight in the target folder", { timeout: 600_000 }, async () => { const version = process.env.RIFT_E2E_VERSION ?? "" - await runExtraction({ filePath: realArchive as string, outputPath: workspacePath("target"), deleteArchive: false, sevenZipBin }) + await runExtraction({ filePath: realArchive as string, outputPath: workspacePath("target"), deleteArchive: false, unwrapSingleRootFolder: true }) const landed = readdirSync(workspacePath("target")) assert.equal(landed.includes("vintagestory"), false) From 931f15b3e6435f4554950c52d902d77853f112bf Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:10:27 +0200 Subject: [PATCH 3/9] chore(deps): drop node-7z, 7zip-bin and @types/node-7z Nothing spawns 7-Zip any more, so the three packages and the six 7za binaries they ship go. That takes the bundled binaries out of app.asar.unpacked, takes the asarUnpack entry that put them there out of the builder config, and takes the executable-bit repair out of the postinstall script. The script itself stays: its other half downloads the Electron binary that a plain npm ci no longer fetches, which has nothing to do with 7-Zip. --- docs/vintage-story-quirks.md | 4 +- electron-builder.yml | 1 - package-lock.json | 76 ----------------------------------- package.json | 3 -- scripts/fix-native-deps.js | 49 +++++----------------- src/ipc/workers/workerHost.ts | 6 +-- 6 files changed, 16 insertions(+), 123 deletions(-) diff --git a/docs/vintage-story-quirks.md b/docs/vintage-story-quirks.md index 6bdebe11..f621312b 100644 --- a/docs/vintage-story-quirks.md +++ b/docs/vintage-story-quirks.md @@ -14,11 +14,11 @@ Field knowledge for anyone touching the game catalog, archive extraction, the in ## Archive -**A single wrapping folder gets flattened.** `contentRoot()` in `src/ipc/workers/extraction.ts:112-131` steps into an archive's contents only when the root holds exactly one entry and it is a directory; anything else (including an already-flat server archive) is left untouched. It is applied only to game archives, not server archives (`extraction.ts:261`: `copyTree(isGameArchive ? contentRoot(extractionRoot) : extractionRoot, outputPath)`). Covered by `tests/ipc/extraction.test.ts` ("unpacks a wrapped archive straight into the target folder" / "leaves a flat archive flat"). +**A single wrapping folder gets flattened.** `contentRoot()` in `src/ipc/workers/extraction.ts:112-131` steps into an archive's contents only when the root holds exactly one entry and it is a directory; anything else (including an already-flat server archive) is left untouched. It is applied only when the caller asks for it (`unwrapSingleRootFolder`, which the game version install passes and the backup restore does not: `copyTree(unwrapSingleRootFolder ? contentRoot(extractionRoot) : extractionRoot, outputPath)`). Covered by `tests/ipc/extraction.test.ts` ("unpacks a wrapped archive straight into the target folder" / "leaves a flat archive flat"). **Zero-size entries have to survive extraction as zero-size files, not be treated as errors or skipped.** `tests/ipc/extraction.test.ts:132-142` builds an archive containing an empty `assets/version-1.22.6.txt` (the version marker from the Catalog section above) and asserts the extracted file exists with `size === 0`. `validateTree` in `src/ipc/workers/extraction.ts` counts zero-byte files explicitly rather than special-casing them away. -**Vintage Story's tars have NUL bytes in their numeric header fields, which the bundled p7zip rejects; node-tar (and GNU tar) accept them, which is why `.tar.gz` extraction goes through the `tar` npm package instead of 7-Zip.** The reasoning is spelled out in `src/ipc/workers/extraction.ts:151-158`: "7-Zip is not used here. It needs two passes for a `.tar.gz`, and the bundled p7zip 16.02 cannot read the tar Vintage Story ships at all: its headers leave the numeric fields as NUL bytes, which GNU tar and node-tar accept and 7-Zip rejects with 'Is not archive'." Every other archive format goes through `node-7z`/`7zip-bin`; only tar.gz is routed to `tar.extract` (`extractTarGz`, same file). +**Vintage Story's tars have NUL bytes in their numeric header fields, which p7zip rejects and node-tar (and GNU tar) accept.** This is one of the reasons the launcher reads `.tar.gz` with the `tar` npm package, and it was the reason it never routed game archives through 7-Zip even while 7-Zip was still bundled. Since the launcher dropped `node-7z` and `7zip-bin` (issue #222), only two formats are read at all: gzipped tar through `tar` (`extractTarGz` in `src/ipc/workers/extraction.ts`, for game builds and for the backups the launcher writes) and zip through `yauzl` (`extractZip`, same file, for the backups made before that change). Anything else is refused by `validateArchive` in `src/ipc/archiveValidation.ts` rather than handed to a reader that would have to guess at it. ## Installer diff --git a/electron-builder.yml b/electron-builder.yml index f52ec9f0..ca3cd5ac 100644 --- a/electron-builder.yml +++ b/electron-builder.yml @@ -22,7 +22,6 @@ files: - "!node_modules/@napi-rs/lzma-*-musl{,/**/*}" asarUnpack: - resources/** - - node_modules/7zip-bin/** - node_modules/@napi-rs/lzma-*/** win: executableName: RiftLauncher diff --git a/package-lock.json b/package-lock.json index 664b9238..af9f1277 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,12 +11,10 @@ "dependencies": { "@electron-toolkit/utils": "^3.0.0", "@napi-rs/lzma": "1.5.1", - "7zip-bin": "^5.2.0", "electron-log": "^5.2.3", "electron-updater": "^6.8.9", "fs-extra": "^11.2.0", "json5": "^2.2.3", - "node-7z": "^3.0.0", "tar": "7.5.22", "write-file-atomic": "^8.0.0", "yauzl": "^3.4.0" @@ -31,7 +29,6 @@ "@testing-library/react": "^16.3.2", "@testing-library/user-event": "^14.6.4", "@types/node": "^20.14.8", - "@types/node-7z": "^2.1.11", "@types/react": "^18.3.3", "@types/react-dom": "^18.3.0", "@types/semver": "^7.5.8", @@ -3305,16 +3302,6 @@ "undici-types": "~6.21.0" } }, - "node_modules/@types/node-7z": { - "version": "2.1.11", - "resolved": "https://registry.npmjs.org/@types/node-7z/-/node-7z-2.1.11.tgz", - "integrity": "sha512-7gwLx44tqZqjyrvjkX41CWW4h7+aXrazFg/JR6N5g+R5BW1eqsNuw8SNLWrh7KcnfKhAYFiWyNb10ti5v5eCmQ==", - "dev": true, - "license": "MIT", - "dependencies": { - "@types/node": "*" - } - }, "node_modules/@types/prop-types": { "version": "15.7.15", "resolved": "https://registry.npmjs.org/@types/prop-types/-/prop-types-15.7.15.tgz", @@ -3745,12 +3732,6 @@ "node": ">=10.0.0" } }, - "node_modules/7zip-bin": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/7zip-bin/-/7zip-bin-5.2.0.tgz", - "integrity": "sha512-ukTPVhqG4jNzMro2qA9HSCSSVJN3aN7tlb+hfqYCt3ER0yWroeA2VR38MNrOHLQ/cVj+DaIMad0kFCtWWowh/A==", - "license": "MIT" - }, "node_modules/abbrev": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-4.0.0.tgz", @@ -8155,36 +8136,12 @@ "dev": true, "license": "MIT" }, - "node_modules/lodash.defaultsdeep": { - "version": "4.6.1", - "resolved": "https://registry.npmjs.org/lodash.defaultsdeep/-/lodash.defaultsdeep-4.6.1.tgz", - "integrity": "sha512-3j8wdDzYuWO3lM3Reg03MuQR957t287Rpcxp1njpEa8oDrikb+FwGdW3n+FELh/A6qib6yPit0j/pv9G/yeAqA==", - "license": "MIT" - }, - "node_modules/lodash.defaultto": { - "version": "4.14.0", - "resolved": "https://registry.npmjs.org/lodash.defaultto/-/lodash.defaultto-4.14.0.tgz", - "integrity": "sha512-G6tizqH6rg4P5j32Wy4Z3ZIip7OfG8YWWlPFzUFGcYStH1Ld0l1tWs6NevEQNEDnO1M3NZYjuHuraaFSN5WqeQ==", - "license": "MIT" - }, "node_modules/lodash.escaperegexp": { "version": "4.1.2", "resolved": "https://registry.npmjs.org/lodash.escaperegexp/-/lodash.escaperegexp-4.1.2.tgz", "integrity": "sha512-TM9YBvyC84ZxE3rgfefxUWiQKLilstD6k7PTGt6wfbtXF8ixIJLOL3VYyV/z+ZiPLsVxAsKAFVwWlWeb2Y8Yyw==", "license": "MIT" }, - "node_modules/lodash.flattendeep": { - "version": "4.4.0", - "resolved": "https://registry.npmjs.org/lodash.flattendeep/-/lodash.flattendeep-4.4.0.tgz", - "integrity": "sha512-uHaJFihxmJcEX3kT4I23ABqKKalJ/zDrDg0lsFtc1h+3uw49SIJ5beyhx5ExVRti3AvKoOJngIj7xz3oylPdWQ==", - "license": "MIT" - }, - "node_modules/lodash.isempty": { - "version": "4.4.0", - "resolved": "https://registry.npmjs.org/lodash.isempty/-/lodash.isempty-4.4.0.tgz", - "integrity": "sha512-oKMuF3xEeqDltrGMfDxAPGIVMSSRv8tbRSODbrs4KGsRRLEhrW8N8Rd4DRgB2+621hY8A8XwwrTVhXWpxFvMzg==", - "license": "MIT" - }, "node_modules/lodash.isequal": { "version": "4.5.0", "resolved": "https://registry.npmjs.org/lodash.isequal/-/lodash.isequal-4.5.0.tgz", @@ -8199,12 +8156,6 @@ "dev": true, "license": "MIT" }, - "node_modules/lodash.negate": { - "version": "3.0.2", - "resolved": "https://registry.npmjs.org/lodash.negate/-/lodash.negate-3.0.2.tgz", - "integrity": "sha512-JGJYYVslKYC0tRMm/7igfdHulCjoXjoganRNWM8AgS+RXfOvFnPkOveDhPI65F9aAypCX9QEEQoBqWf7Q6uAeA==", - "license": "MIT" - }, "node_modules/loose-envify": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/loose-envify/-/loose-envify-1.4.0.tgz", @@ -8525,24 +8476,6 @@ "dev": true, "license": "MIT" }, - "node_modules/node-7z": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/node-7z/-/node-7z-3.0.0.tgz", - "integrity": "sha512-KIznWSxIkOYO/vOgKQfJEaXd7rgoFYKZbaurainCEdMhYc7V7mRHX+qdf2HgbpQFcdJL/Q6/XOPrDLoBeTfuZA==", - "license": "ISC", - "dependencies": { - "debug": "^4.3.2", - "lodash.defaultsdeep": "^4.6.1", - "lodash.defaultto": "^4.14.0", - "lodash.flattendeep": "^4.4.0", - "lodash.isempty": "^4.4.0", - "lodash.negate": "^3.0.2", - "normalize-path": "^3.0.0" - }, - "engines": { - "node": ">=10" - } - }, "node_modules/node-abi": { "version": "4.33.0", "resolved": "https://registry.npmjs.org/node-abi/-/node-abi-4.33.0.tgz", @@ -8670,15 +8603,6 @@ "node": "^20.17.0 || >=22.9.0" } }, - "node_modules/normalize-path": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/normalize-path/-/normalize-path-3.0.0.tgz", - "integrity": "sha512-6eZs5Ls3WtCisHWp9S2GUy8dqkpGi4BVSz3GaqiE6ezub0512ESztXUwUB6C6IKbQkY2Pnb/mD4WYojCRwcwLA==", - "license": "MIT", - "engines": { - "node": ">=0.10.0" - } - }, "node_modules/normalize-url": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/normalize-url/-/normalize-url-6.1.0.tgz", diff --git a/package.json b/package.json index 9e66eb71..9a34d37c 100644 --- a/package.json +++ b/package.json @@ -34,12 +34,10 @@ "dependencies": { "@electron-toolkit/utils": "^3.0.0", "@napi-rs/lzma": "1.5.1", - "7zip-bin": "^5.2.0", "electron-log": "^5.2.3", "electron-updater": "^6.8.9", "fs-extra": "^11.2.0", "json5": "^2.2.3", - "node-7z": "^3.0.0", "tar": "7.5.22", "write-file-atomic": "^8.0.0", "yauzl": "^3.4.0" @@ -54,7 +52,6 @@ "@testing-library/react": "^16.3.2", "@testing-library/user-event": "^14.6.4", "@types/node": "^20.14.8", - "@types/node-7z": "^2.1.11", "@types/react": "^18.3.3", "@types/react-dom": "^18.3.0", "@types/semver": "^7.5.8", diff --git a/scripts/fix-native-deps.js b/scripts/fix-native-deps.js index f446406d..bc542495 100644 --- a/scripts/fix-native-deps.js +++ b/scripts/fix-native-deps.js @@ -1,49 +1,23 @@ #!/usr/bin/env node /** - * `npm ci` on this project leaves two native-tooling gaps that have nothing - * to do with the code being worked on. Both are dev/test-only: packaged - * builds are unaffected because electron-builder and electron-vite handle - * them on their own. + * `npm ci` on this project leaves one native-tooling gap that has nothing to do + * with the code being worked on. It is dev/test-only: packaged builds are + * unaffected because electron-builder and electron-vite handle it on their own. * - * 1. node_modules/7zip-bin///7za(.exe) sometimes loses its - * executable bit in transit (registries and some npm/tar combinations do - * not reliably preserve unix permissions while packing/unpacking). Any - * code path that spawns it then fails with `EACCES`. See #30. + * As of Electron 42, the `electron` package shipped its own `postinstall` + * (`node install.js`) that downloaded the platform binary; that hook was + * removed from its package.json, so a plain `npm ci` no longer fetches + * node_modules/electron/dist at all. `npm run dev` and anything that spawns + * Electron then fails until install.js is run by hand. Cheap to close here + * since this script already runs on install. * - * 2. As of Electron 42, the `electron` package shipped its own - * `postinstall` (`node install.js`) that downloaded the platform binary; - * that hook was removed from its package.json, so a plain `npm ci` no - * longer fetches node_modules/electron/dist at all. `npm run dev` and - * anything that spawns Electron then fails until install.js is run by - * hand. Cheap to close here since this script already runs on install. - * - * Both fixes are idempotent and safe to run on every `npm install`. + * The fix is idempotent and safe to run on every `npm install`. */ -const { chmodSync, existsSync, statSync } = require("node:fs") +const { existsSync } = require("node:fs") const { join } = require("node:path") const { spawnSync } = require("node:child_process") -function restoreSevenZipExecutableBit() { - // .exe needs no unix executable bit, and Windows has no such concept. - if (process.platform === "win32") return - - let path7za - try { - ;({ path7za } = require("7zip-bin")) - } catch { - return // 7zip-bin is not installed; nothing to fix. - } - - if (!path7za || !existsSync(path7za)) return - - const isExecutable = (statSync(path7za).mode & 0o111) !== 0 - if (isExecutable) return - - chmodSync(path7za, 0o755) - console.log(`[fix-native-deps] restored executable bit on ${path7za}`) -} - function ensureElectronBinaryIsDownloaded() { const electronDir = join(__dirname, "..", "node_modules", "electron") const installScript = join(electronDir, "install.js") @@ -63,5 +37,4 @@ function ensureElectronBinaryIsDownloaded() { } } -restoreSevenZipExecutableBit() ensureElectronBinaryIsDownloaded() diff --git a/src/ipc/workers/workerHost.ts b/src/ipc/workers/workerHost.ts index f7abc22d..50fa55a4 100644 --- a/src/ipc/workers/workerHost.ts +++ b/src/ipc/workers/workerHost.ts @@ -77,9 +77,9 @@ export function serveTasks(handler: TaskHandler, describeFailure: FailureDescrib } const onProgress: ProgressReporter = (progress) => { - // node-7z and the stream readers this app uses can emit one last progress event - // after their own end event. Posting it was harmless while the worker died with - // its task right after; now the worker can already be sitting idle in the pool. + // The stream readers this app uses can emit one last progress event after their + // own end event. Posting it was harmless while the worker died with its task + // right after; now the worker can already be sitting idle in the pool. if (settled) return port.postMessage({ type: "progress", token, progress }) } From a6a714f3132bda9069f7e3c48e051e475b6e2a11 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:21:41 +0200 Subject: [PATCH 4/9] fix(backups): fail a tar extraction that could not write every entry node-tar reports a per-entry write failure as a warning, skips the entry and closes the stream cleanly, so a restore that filled the disk halfway through came back looking like a whole one. runExtraction then validated only what had landed and the restore swapped the truncated tree in and deleted the copy it replaced. strict makes those failures errors, so the extraction rejects and the restore stops before it moves anything. It also covers a corrupt entry header, dropped just as quietly until now. The two other warnings it turns fatal are already refused earlier, by the entry type filter and by the table-of-contents pass. The zip reader gets the teardown the tar reader already had: a failure now unpipes and destroys the streams the entry was moving through, so the temporary folder is not removed out from under an open write handle. --- src/ipc/workers/extraction.ts | 30 ++++++++++- tests/ipc/extraction.test.ts | 97 ++++++++++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 3 deletions(-) diff --git a/src/ipc/workers/extraction.ts b/src/ipc/workers/extraction.ts index 087ec7eb..6ea2254e 100644 --- a/src/ipc/workers/extraction.ts +++ b/src/ipc/workers/extraction.ts @@ -14,6 +14,7 @@ import yauzl from "yauzl" import { createReadStream, createWriteStream, mkdtempSync } from "node:fs" import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path" import { tmpdir } from "node:os" +import type { Readable } from "node:stream" import * as tar from "tar" // Relative so the module stays importable from a plain test run, like validation.ts. @@ -173,10 +174,23 @@ export function extractZip(filePath: string, destination: string, onProgress?: ( let extractedEntries = 0 let lastReportedProgress = 0 const totalEntries = zipFile.entryCount + // The pair the entry being written is streaming through, so a failure can + // stop them. Reassigned per entry, and left pointing at the last one. + let entryReader: Readable | undefined + let entryWriter: ReturnType | undefined const finish = (failure?: Error): void => { if (settled) return settled = true + if (failure) { + // Stop both ends before the caller wipes the temporary folder, the same + // reason extractTarGz does it: an open write handle in there turns the + // removal into an EBUSY on Windows, which then replaces the real error + // and leaves the folder behind. + entryReader?.unpipe() + entryReader?.destroy() + entryWriter?.destroy() + } try { zipFile.close() } catch { @@ -225,15 +239,16 @@ export function extractZip(filePath: string, destination: string, onProgress?: ( return } + entryReader = readStream let writeStream: ReturnType try { fse.ensureDirSync(dirname(target)) writeStream = createWriteStream(target) } catch { - readStream.destroy() finish(new Error("Extraction failed")) return } + entryWriter = writeStream readStream.on("error", () => finish(new Error("Extraction failed"))) writeStream.on("error", () => finish(new Error("Extraction failed"))) @@ -260,6 +275,18 @@ export function extractZip(filePath: string, destination: string, onProgress?: ( * up front. Anything that is not a plain file or folder fails the extraction * rather than being skipped quietly, and `preservePaths: false` is what keeps * an absolute or climbing entry name from being written where it points. + * + * `strict` is what makes a failed entry a failed extraction. Left off, node-tar + * treats every per-entry write error as a warning: the entry is skipped, the + * stream still closes cleanly, and a run that hit a full disk halfway through + * comes back indistinguishable from one that unpacked everything. A restore + * believes that and deletes the installation the truncated tree replaced, so + * the archive has to fail closed here rather than downstream. It also upgrades + * the parser's own invalid-entry warning, a corrupt header whose entry is + * dropped just as quietly. Nothing else it turns fatal is reachable: an + * unsupported entry type is refused by the filter below, and an absolute name + * never gets this far, validateArchive having refused the archive by name + * before a byte was written. */ export function extractTarGz(filePath: string, destination: string, onProgress?: (progress: number) => void): Promise { return new Promise((resolvePromise, rejectPromise) => { @@ -288,6 +315,7 @@ export function extractTarGz(filePath: string, destination: string, onProgress?: const unpacker = tar.extract({ cwd: destination, preservePaths: false, + strict: true, filter: (_entryPath, entry): boolean => { if (isSafeTarEntryType("type" in entry ? entry.type : undefined)) return true unsafeEntry = new Error("Archive contains an unsafe entry") diff --git a/tests/ipc/extraction.test.ts b/tests/ipc/extraction.test.ts index cf40e0b4..32c76252 100644 --- a/tests/ipc/extraction.test.ts +++ b/tests/ipc/extraction.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict" -import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" +import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, renameSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join, relative, resolve } from "node:path" import { afterEach, beforeEach, describe, it } from "vitest" @@ -7,6 +7,8 @@ import * as tar from "tar" import { contentRoot, extractTarGz, extractZip, resolveEntryDestination, runExtraction, validateTree } from "../../src/ipc/workers/extraction" import { runCompression } from "../../src/ipc/workers/compression" +import { restoreInstallationBackup } from "../../src/domain/installations/restore" +import type { Extractor, FileSystem } from "../../src/domain/ports" /** * These build their own archives, so they run anywhere without a network. The @@ -268,6 +270,95 @@ describe("backup round trip", () => { }) }) +/** + * An archive whose second entry cannot be written, without needing a full disk + * or a permission the test runner may not be able to drop: "a" is a plain file, + * and "a/b" then asks for "a" to be a folder. The unpacker fails that one entry + * and carries on to the next, which is how a disk that fills partway through a + * multi-GB restore fails too, one entry at a time. + */ +async function makeConflictingTarGz(archiveName: string): Promise { + writeTree(workspacePath("conflict"), { a: "the file standing in the way", elsewhere: { b: "never lands" } }) + const archivePath = workspacePath(archiveName) + await tar.create( + { + file: archivePath, + cwd: workspacePath("conflict"), + gzip: true, + portable: true, + onWriteEntry: (entry) => { + if (entry.path === "elsewhere/b") entry.path = "a/b" + } + }, + ["a", "elsewhere/b"] + ) + return archivePath +} + +describe("a gzipped tar whose entries fail to land", () => { + it("fails the extraction rather than reporting a truncated tree as a whole one", async () => { + const archivePath = await makeConflictingTarGz("half-lands.tar.gz") + + await assert.rejects(runExtraction({ filePath: archivePath, outputPath: workspacePath("restored"), deleteArchive: false }), /Extraction failed/) + + // Nothing was copied out of the temporary folder, so the destination is as + // empty as it was: a partial tree is not a restore. + assert.deepEqual(readdirSync(workspacePath("restored")), []) + }) + + it("leaves the installation where it is, because the swap only happens once the extraction succeeded", async () => { + const archivePath = await makeConflictingTarGz("half-lands.tar.gz") + writeTree(workspacePath("my-install"), { "clientsettings.json": "the only copy of this" }) + + const fileSystem: FileSystem = { + exists: async (path: string): Promise => existsSync(path), + remove: async (path: string): Promise => { + rmSync(path, { recursive: true, force: true }) + return true + }, + move: async (from: string, to: string): Promise => { + renameSync(from, to) + return true + } + } + // The same shape extractWorker.ts gives the port: a rejection becomes a + // reported failure, anything else is a success. + const extractor: Extractor = { + extract: async (request, onComplete): Promise => { + try { + await runExtraction({ filePath: request.archivePath, outputPath: request.outputFolder, deleteArchive: false }) + onComplete({ ok: true }) + } catch (error) { + onComplete({ ok: false, error: (error as Error).message }) + } + } + } + + const result = await restoreInstallationBackup( + { fileSystem, extractor, ids: { newId: (): string => "token" }, closeGuard: { acquire: () => (): void => {} } }, + { + installation: { + id: "install-1", + name: "My install", + path: workspacePath("my-install"), + backupsLimit: 3, + compressionLevel: 6, + backups: [], + isBackingUp: false, + isPlaying: false, + isRestoringBackup: false + }, + backup: { id: "backup-1", date: 0, path: archivePath } + } + ) + + assert.equal(result.ok, false) + assert.equal(result.ok === false && result.reason, "extract-failed") + assert.equal(readFileSync(workspacePath("my-install", "clientsettings.json"), "utf8"), "the only copy of this") + assert.deepEqual(readdirSync(workspacePath("my-install")), ["clientsettings.json"]) + }) +}) + describe("runExtraction on a legacy zip backup", () => { it("restores a backup written before the launcher moved off zip", async () => { await runExtraction({ filePath: join(FIXTURES, "legacy-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false }) @@ -380,7 +471,9 @@ describe("hostile entry names, straight at the unpackers", () => { const destination = workspacePath("destination", "inside") mkdirSync(destination, { recursive: true }) - await extractTarGz(archivePath, destination) + // The names are refused rather than quietly dropped: an entry the unpacker + // will not place is a failed extraction, not a smaller one. + await assert.rejects(extractTarGz(archivePath, destination), /Extraction failed/) assert.equal(existsSync(workspacePath("destination", "escaped.txt")), false) assert.equal(existsSync(workspacePath("escaped.txt")), false) From 5524f39fdf618acb61330b9579beb3a609ef7ef7 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:21:54 +0200 Subject: [PATCH 5/9] fix(backups): remove the partial archive a failed compression leaves tar opens the archive as soon as it starts, so a write that failed partway through left a truncated .tar.gz in the backups folder. No backup record names it, and pruning only walks the records, so it stayed there for good and every retry added another one beside it. --- src/ipc/workers/compression.ts | 9 +++++++++ tests/ipc/compression.test.ts | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/ipc/workers/compression.ts b/src/ipc/workers/compression.ts index e47335e3..2b034799 100644 --- a/src/ipc/workers/compression.ts +++ b/src/ipc/workers/compression.ts @@ -119,6 +119,15 @@ export async function runCompression(options: CompressionOptions): Promise entries.length > 0 ? entries : ["."] ) } catch { + // tar opens the archive as soon as it starts, so a write that failed partway + // leaves a truncated file sitting in the backups folder. No backup record + // ever names it, which is exactly what pruning walks, so it would never be + // cleaned up and every retry would leave another one. + try { + fse.removeSync(archivePath) + } catch { + // Best effort. The compression failure below is the outcome that matters. + } throw new Error("Compression failed") } diff --git a/tests/ipc/compression.test.ts b/tests/ipc/compression.test.ts index dd93d485..7a3a88fb 100644 --- a/tests/ipc/compression.test.ts +++ b/tests/ipc/compression.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict" -import { mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { gunzipSync } from "node:zlib" @@ -237,6 +237,19 @@ describe("runCompression", () => { assert.equal(readFileSync(workspacePath("someone-elses-file"), "utf8"), "") }) + it.skipIf(process.platform !== "linux" || process.getuid?.() === 0)("takes the half written archive away when the write fails", async () => { + // A file the safety walk can stat but tar cannot read, so the failure lands + // after tar has already created the archive and written the first bytes. + chmodSync(join(source, "Vintagestory"), 0o000) + + await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }), /Compression failed/) + + // A failed backup leaves no record behind, and pruning only ever walks the + // records, so anything left here would stay for good and a retry would add + // one more beside it. + assert.deepEqual(readdirSync(output), []) + }) + it("overwrites a plain archive file already sitting there", async () => { writeFileSync(join(output, "backup.tar.gz"), "the previous backup") From 2aa693fc6580afddbc95aea6c723fe021cee0d78 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:32:11 +0200 Subject: [PATCH 6/9] fix(backups): refuse a source the restore reader would refuse The reader holds an archive to a 2 GiB total and refuses anything past it, so an installation over that cap compressed happily into a backup that could never be put back. The walk already had the total in hand. Refusing costs the player a failed backup. Not refusing cost them a backup they would only discover was useless on the day they needed it, plus a prune slot, since pruning runs before the archive is written and an older restorable backup had already been deleted to make room. --- src/ipc/workers/compression.ts | 10 ++++++++++ tests/ipc/compression.test.ts | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/ipc/workers/compression.ts b/src/ipc/workers/compression.ts index 2b034799..a863e879 100644 --- a/src/ipc/workers/compression.ts +++ b/src/ipc/workers/compression.ts @@ -19,6 +19,9 @@ import * as tar from "tar" import { DEFAULT_COMPRESSION_LEVEL } from "@domain/config/defaults" +// Relative so the module stays importable from a plain test run, like extraction.ts. +import { MAX_ARCHIVE_TOTAL_BYTES } from "../validation" + const MAX_ITEMS = 100_000 /** @@ -76,6 +79,13 @@ export async function runCompression(options: CompressionOptions): Promise const { inputPath, outputPath, outputFileName, compressionLevel = DEFAULT_COMPRESSION_LEVEL, onProgress } = options const totalBytes = assertSafeCompressionTree(inputPath) + // The restore reader holds an archive to this same total and refuses anything + // past it, so an installation over the cap would compress happily into a + // backup that can never be put back. Refusing here costs the player a failed + // backup; not refusing costs them a backup they only find out is useless on + // the day they need it, and one prune slot that an older, restorable backup + // used to hold. + if (totalBytes > MAX_ARCHIVE_TOTAL_BYTES) throw new Error("Compression source is too large") if (!fse.existsSync(inputPath) || !fse.lstatSync(inputPath).isDirectory()) throw new Error("Compression source must be a directory") if (!fse.existsSync(outputPath)) fse.mkdirSync(outputPath, { recursive: true }) if (fse.lstatSync(outputPath).isSymbolicLink() || !fse.lstatSync(outputPath).isDirectory()) throw new Error("Compression destination is unsafe") diff --git a/tests/ipc/compression.test.ts b/tests/ipc/compression.test.ts index 7a3a88fb..2d69da16 100644 --- a/tests/ipc/compression.test.ts +++ b/tests/ipc/compression.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict" -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs" +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, truncateSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { gunzipSync } from "node:zlib" @@ -237,6 +237,19 @@ describe("runCompression", () => { assert.equal(readFileSync(workspacePath("someone-elses-file"), "utf8"), "") }) + it.skipIf(process.platform === "win32")("refuses a source past the total the restore reader will accept", async () => { + // A sparse file: 3 GiB by every stat the walk makes, no blocks on disk. The + // point is the size the reader would read back, and that is what stat says. + const huge = join(source, "world.vcdbs") + writeFileSync(huge, "") + truncateSync(huge, 3 * 1024 * 1024 * 1024) + + await assert.rejects(runCompression({ inputPath: source, outputPath: output, outputFileName: "backup.tar.gz" }), /too large/) + + // Refused before anything was written, rather than after gigabytes of work. + assert.deepEqual(readdirSync(output), []) + }) + it.skipIf(process.platform !== "linux" || process.getuid?.() === 0)("takes the half written archive away when the write fails", async () => { // A file the safety walk can stat but tar cannot read, so the failure lands // after tar has already created the archive and written the first bytes. From 885b7277186f6a353cec415851bac43fe6b75d82 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:32:16 +0200 Subject: [PATCH 7/9] test(backups): pin the zip name gate with an archive that reaches it hostile-backup.zip never reached isSafeArchiveEntry. yauzl's own validateFileName refuses a drive letter exactly as it refuses a leading slash or a ".." segment, so all three of its names stop the read before the launcher's gate has a say, and the comment claiming otherwise was wrong. A NUL byte in the middle of a name is one yauzl has nothing to say about, so unsafe-name-backup.zip is the archive that gets there. Without it, deleting that check from validateZipArchive failed no test in the suite. --- tests/fixtures/build-fixtures.ts | 27 ++++++++++++++++++++------ tests/fixtures/unsafe-name-backup.zip | Bin 0 -> 268 bytes tests/ipc/extraction.test.ts | 10 ++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) create mode 100644 tests/fixtures/unsafe-name-backup.zip diff --git a/tests/fixtures/build-fixtures.ts b/tests/fixtures/build-fixtures.ts index ad606e10..3f694db9 100644 --- a/tests/fixtures/build-fixtures.ts +++ b/tests/fixtures/build-fixtures.ts @@ -4,7 +4,7 @@ * Most of them are what tests/ipc/modScan.test.ts reads readModArchive * against, so the yauzl edge cases in src/ipc/adapters/modScan.ts are * exercised against real archive bytes instead of only through the domain's - * fakes. The last two are backups: the launcher wrote its backups as zip up to + * fakes. The last three are backups: the launcher wrote its backups as zip up to * 1.7.0-beta.4, and tests/ipc/extraction.test.ts restores them here so a * player's old backup keeps working long after the writer went away. * @@ -386,11 +386,11 @@ write( // unix absolute path. None is a name any writer produces by accident, and // nothing may be written for any of them. // -// The drive-letter one is first on purpose. It is the one yauzl itself lets -// through (its own name validation refuses a leading "/" and any ".." segment, -// and stops the read there), so putting it first is what makes the launcher's -// own isSafeArchiveEntry check the gate that speaks, which is the check worth -// having a test hold in place. +// All three are refused by yauzl's own validateFileName, which rejects +// /^[a-zA-Z]:/ exactly as it rejects a leading "/" and any ".." segment, so the +// read stops at the first entry and the launcher reports an archive it could +// not read. Nothing here reaches isSafeArchiveEntry; unsafe-name-backup.zip +// below is the one that does. write( "hostile-backup.zip", assembleZip([ @@ -399,3 +399,18 @@ write( { name: "/etc/escaped-absolute.txt", method: METHOD_STORE, realBytes: Buffer.from("absolute", "utf8") } ]) ) + +// --- unsafe-name-backup.zip --------------------------------------------- +// A NUL byte in the middle of an entry name. yauzl's validateFileName has +// nothing to say about one, so this is the archive that reaches the launcher's +// own isSafeArchiveEntry gate in validateZipArchive, which every name in +// hostile-backup.zip is stopped short of. An ordinary entry comes first, so the +// refusal is that gate deciding rather than the archive being unreadable from +// its first byte. +write( + "unsafe-name-backup.zip", + assembleZip([ + { name: "Vintagestory", method: METHOD_STORE, realBytes: Buffer.from("elf", "utf8") }, + { name: "Mods/notes\u0000.txt", method: METHOD_STORE, realBytes: Buffer.from("a name no writer produces by accident", "utf8") } + ]) +) diff --git a/tests/fixtures/unsafe-name-backup.zip b/tests/fixtures/unsafe-name-backup.zip new file mode 100644 index 0000000000000000000000000000000000000000..22aadc914981930afc8737f7de5f1e3fa7158e5f GIT binary patch literal 268 zcmWIWW@Zs#fB;1Xd4nT&n1LJ+<^kfc%)FAs^wi>#{G!U#oU{NG#WqVd5>^70Qb;OHzvz3X1YmN|RHI6_P3y5|fiNQ&RIv u0=yZSL>O>80B9@{Kz0tgPIT8GwCMtwXl@JeW@Q5@U;@H&AYBjQFaQAE7% { assert.equal(existsSync(workspacePath("restored")), false) }) + it("refuses an entry name yauzl accepts but the launcher does not", async () => { + // The other hostile fixture is stopped by yauzl's own name validation, so + // the launcher's isSafeArchiveEntry never gets a say on it. A NUL byte in + // the middle of a name is one yauzl has nothing to say about, which makes + // this the archive that pins that gate against real bytes. + await assert.rejects(runExtraction({ filePath: join(FIXTURES, "unsafe-name-backup.zip"), outputPath: workspacePath("restored"), deleteArchive: false }), /unsafe entry/) + + assert.equal(existsSync(workspacePath("restored")), false) + }) + it("refuses an archive whose name says neither zip nor tar.gz", async () => { await assert.rejects(runExtraction({ filePath: join(FIXTURES, "not-a-zip.bin"), outputPath: workspacePath("restored"), deleteArchive: false }), /not supported/) }) From 2346851976148483a01abd036ada15d90b9ce53c Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:32:22 +0200 Subject: [PATCH 8/9] docs(backups): sweep the references the format change left behind The concurrency limiter and the worker timeout in pathsHandlers.ts were written around 7-Zip subprocesses, pathPolicy.ts still said every archive the launcher makes is a zip, isTarGzName explained itself in terms of what 7-Zip could not read, and the default compression level was described as 7-Zip's. ADR 0001 keeps its measurements. They were taken on a stated day to support a decision that is still pending, so it gets a dated note saying the 7-Zip lines no longer hold rather than an edit that would put today's tree into yesterday's argument. Also says outright, where the zip reader is, that a legacy backup's unix modes are deliberately not restored. --- docs/decisions/0001-shell-and-codebase.md | 2 ++ src/domain/config/defaults.ts | 6 ++++-- src/ipc/handlers/pathsHandlers.ts | 16 +++++++++------- src/ipc/pathPolicy.ts | 5 +++-- src/ipc/validation.ts | 4 ++-- src/ipc/workers/extraction.ts | 10 ++++++++++ 6 files changed, 30 insertions(+), 13 deletions(-) diff --git a/docs/decisions/0001-shell-and-codebase.md b/docs/decisions/0001-shell-and-codebase.md index 350cec3d..b8ab4322 100644 --- a/docs/decisions/0001-shell-and-codebase.md +++ b/docs/decisions/0001-shell-and-codebase.md @@ -14,6 +14,8 @@ So the question is no longer only "which shell". It is: Every number below was measured on 2026-08-16, on this machine, with the command named beside it. Where a number could not be measured, that is said instead of guessed. +> Note, 2026-08-29: the measurements below stand as they were taken, and the 7-Zip ones no longer describe the tree. PR #274 removed `node-7z` and `7zip-bin`, so the six `7za` binaries and the 9.4 MB `app.asar.unpacked` line are gone, a Linux `--dir` build is 9.5 MiB smaller, and the `7za l -slt` reader that Option A's cost paragraph leans on no longer exists: backups are gzipped tar read by `tar`, and the zips written before 1.7.0-beta.4 are read by `yauzl`. The test and coverage figures have moved a long way too. Nothing here is restated, because the argument was put to the deciders with the numbers of the day and rewriting it after the fact would misrepresent what was actually weighed. + ## What was measured ### The fork diff --git a/src/domain/config/defaults.ts b/src/domain/config/defaults.ts index 4f8f2236..d32f9b60 100644 --- a/src/domain/config/defaults.ts +++ b/src/domain/config/defaults.ts @@ -32,10 +32,12 @@ export const DEFAULT_CONFIG_BASE: Omit( const timeout = setTimeout( () => { // Never reused: the abandoned task is still running inside this thread (still - // holding a socket or a 7-Zip child), so its eventual message could still arrive + // holding a socket or an open archive), so its eventual message could still arrive // after some later task has been dispatched to the same worker. rejectOnce(new Error(`${operationName} timed out`), "discard") }, diff --git a/src/ipc/pathPolicy.ts b/src/ipc/pathPolicy.ts index 2cbd8e83..37f43404 100644 --- a/src/ipc/pathPolicy.ts +++ b/src/ipc/pathPolicy.ts @@ -83,8 +83,9 @@ function getLauncherFolders(): string[] { * writes mods, saves, client settings and game files under them, and an * installation the user put outside every configured folder still has to work. * A backup grants its own path only. Every archive the launcher makes is a - * single `.zip` file (see makeInstallationBackup), so a path *under* one is - * never a path the launcher meant to reach. + * single file, a `.tar.gz` now and a `.zip` for the ones written before + * 1.7.0-beta.4 (see makeInstallationBackup), so a path *under* one is never a + * path the launcher meant to reach. */ function getEntryGrants(config: ConfigType): PathGrant[] { return [ diff --git a/src/ipc/validation.ts b/src/ipc/validation.ts index b8254738..b767b47f 100644 --- a/src/ipc/validation.ts +++ b/src/ipc/validation.ts @@ -374,8 +374,8 @@ export function isRestoreWorkspaceName(installationName: string, candidateName: } /** - * True when `name` is a gzipped tar, which 7-Zip cannot unpack in one pass and, - * for the archives Vintage Story ships, cannot read at all. + * True when `name` is a gzipped tar: the game builds, and every backup written + * from 1.7.0-beta.4 on. It picks the reader, `tar` rather than `yauzl`. */ export function isTarGzName(name: unknown): boolean { return typeof name === "string" && /\.(?:tar\.gz|tgz)$/i.test(name) diff --git a/src/ipc/workers/extraction.ts b/src/ipc/workers/extraction.ts index 6ea2254e..392b8fe5 100644 --- a/src/ipc/workers/extraction.ts +++ b/src/ipc/workers/extraction.ts @@ -161,6 +161,16 @@ export function resolveEntryDestination(destination: string, entryName: string): * Progress is counted in entries rather than bytes: the entry count is the one * total a zip states up front, per entry sizes are what the archive claims * rather than what comes out, and a backup's entries are of a similar size. + * + * Unix mode bits recorded in a legacy backup are deliberately not carried over, + * unlike the tar reader, which restores what the archive holds. An installation + * folder is game data with nothing executable in it; on Linux every extraction + * is followed by a blanket chmod to 0755 (startExtract in + * TaskManagerContext.tsx), which overwrites what either reader restored; and a + * mode read out of an archive written by a tool the launcher no longer ships is + * as easily 0 as it is useful, which would leave a save file unreadable. The + * attributes are still read, for the symlink check, which is the one thing in + * them worth acting on. */ export function extractZip(filePath: string, destination: string, onProgress?: (progress: number) => void): Promise { return new Promise((resolvePromise, rejectPromise) => { From d7e6fe9282041f33b4572d86728f045f4c512513 Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:47:34 +0200 Subject: [PATCH 9/9] fix(backups): write a hard linked source as independent files tar looks a file up by dev:ino whenever its nlink is above one, and on a hit writes the second name as a Link entry pointing at the first instead of writing the bytes again. The restore validator refuses Link, so a source holding two names for one inode produced a backup that reported success and could never be put back. Handing tar a link cache that never reports a hit sends every name down the ordinary file path, so the archive carries both copies and the restore works. Refusing the source instead would have cost the backup entirely to players on deduplicating filesystems, which hand out hard links without anyone asking for one. --- src/ipc/workers/compression.ts | 27 +++++++++++++++++++++ tests/ipc/extraction.test.ts | 43 ++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/ipc/workers/compression.ts b/src/ipc/workers/compression.ts index a863e879..282306b3 100644 --- a/src/ipc/workers/compression.ts +++ b/src/ipc/workers/compression.ts @@ -24,6 +24,30 @@ import { MAX_ARCHIVE_TOTAL_BYTES } from "../validation" const MAX_ITEMS = 100_000 +/** + * The link cache tar keeps while it packs, rigged never to report a hit. + * + * tar looks a file up in here by `dev:ino` whenever its `nlink` is above one, + * and on a hit writes the second name as a `Link` entry pointing at the first + * instead of writing its bytes again. The restore reader refuses `Link`, so + * that backup reports success and can never be put back. A cache that answers + * nothing sends every name down the ordinary file path, so both names come out + * of the archive as independent copies carrying their own bytes. + * + * Deduplicating filesystems hand out hard links on their own, so a player can + * have them without ever having made one. Refusing the source instead would + * cost them backups entirely; this costs them the sharing, which was a disk + * layout detail rather than anything the installation depends on. + * + * The key type matches tar's own `LinkCacheKey`, which the package does not + * re-export from its entry point. + */ +class UnsharedLinkCache extends Map<`${number}:${number}`, string> { + get(): undefined { + return undefined + } +} + /** * Refuses a source tree holding anything but plain files and folders, or more * entries than the launcher ever legitimately backs up. @@ -107,6 +131,9 @@ export async function runCompression(options: CompressionOptions): Promise cwd: inputPath, gzip: { level: compressionLevel }, portable: true, + // Two names for one inode would otherwise become a Link entry the + // restore reader refuses. See UnsharedLinkCache above. + linkCache: new UnsharedLinkCache(), // `entry.size` is not filled in yet when this runs, and the stat behind // the entry is the same number the walk above totalled. Folders are // skipped for the same reason the walk skipped them: their stat size is diff --git a/tests/ipc/extraction.test.ts b/tests/ipc/extraction.test.ts index f01f8455..4b1c4f58 100644 --- a/tests/ipc/extraction.test.ts +++ b/tests/ipc/extraction.test.ts @@ -247,6 +247,49 @@ describe("backup round trip", () => { assert.deepEqual(readdirSync(workspacePath("restored")).sort(), readdirSync(workspacePath("installation")).sort()) }) + it("puts two names for one inode back as two ordinary files", async () => { + // A player does not have to have made a hard link to have one: a + // deduplicating filesystem hands them out on its own, and a mod copied twice + // can end up as two names over one inode. Left alone, tar writes the second + // name as a Link entry, the restore reader refuses Link, and the backup + // reports success while being impossible to put back. + writeTree(workspacePath("installation"), { Mods: { "carrycapacity.zip": "not really a mod" }, "clientsettings.json": "{}" }) + const firstName = workspacePath("installation", "Mods", "carrycapacity.zip") + const secondName = workspacePath("installation", "Mods", "carrycapacity-1.0.0.zip") + linkSync(firstName, secondName) + // The fixture is only worth anything if the two names really are one file. + assert.equal(lstatSync(firstName).ino, lstatSync(secondName).ino) + assert.equal(lstatSync(firstName).nlink, 2) + const before = readTree(workspacePath("installation")) + + await runCompression({ inputPath: workspacePath("installation"), outputPath: workspacePath("backups"), outputFileName: "backup.tar.gz" }) + + // Nothing in the archive asks the reader to link one name to another: the + // second name carries its own bytes, which is what makes the backup + // self-contained and the restore below possible at all. + const archived: { path: string; type: string; size: number }[] = [] + await tar.list({ file: workspacePath("backups", "backup.tar.gz"), onReadEntry: (entry) => void archived.push({ path: entry.path, type: entry.type, size: entry.size }) }) + assert.deepEqual( + archived.filter((entry) => entry.type !== "File" && entry.type !== "Directory"), + [] + ) + assert.deepEqual( + archived.filter((entry) => entry.path.startsWith("Mods/carrycapacity")).map((entry) => entry.size), + [16, 16] + ) + + await runExtraction({ filePath: workspacePath("backups", "backup.tar.gz"), outputPath: workspacePath("restored"), deleteArchive: false }) + + assert.deepEqual(readTree(workspacePath("restored")), before) + // Independent copies, not the sharing put back: the launcher only promises + // the bytes, and the extraction's own tree check refuses a hard link anyway. + const restoredFirst = lstatSync(workspacePath("restored", "Mods", "carrycapacity.zip")) + const restoredSecond = lstatSync(workspacePath("restored", "Mods", "carrycapacity-1.0.0.zip")) + assert.equal(restoredFirst.nlink, 1) + assert.equal(restoredSecond.nlink, 1) + assert.notEqual(restoredFirst.ino, restoredSecond.ino) + }) + it("puts a backup of an empty installation back as an empty folder", async () => { mkdirSync(workspacePath("installation"), { recursive: true })