From ffa72fbb9ef8649d30d9d7fe0dab7c45c05cbc1e Mon Sep 17 00:00:00 2001 From: Stephen Brown II Date: Thu, 16 Jul 2026 17:13:18 -0400 Subject: [PATCH] [AI] fix(loot-core): replace adm-zip with fflate for zip handling (#8393) * [AI] fix(loot-core): replace adm-zip with fflate for zip handling Replaces the adm-zip dependency (which requires Node's `process`/`Buffer` and can break in browser bundle contexts) with fflate, a zero-dependency, isomorphic zip library that works identically in node and browser builds. Adds a small `zip-utils.ts` wrapper (`safeZip`/`safeUnzip`) around fflate to preserve the safety guarantees a hardened zip library provides by default, since all three call sites parse zips that may cross a trust boundary (cloud sync downloads, user-supplied budget/import files): - Rejects zip-slip/path-traversal entry names (absolute paths, `..` segments, backslashes, drive letters, NUL bytes). - Caps total archive size and per-entry uncompressed size, enforced via fflate's `filter` callback before it allocates the decompression buffer for each entry, to guard against decompression bombs. - Rejects duplicate/case-variant entry names to avoid ambiguity attacks. Updated call sites: budgetfiles/backups.ts (local backup zip write/read), cloud-storage.ts (cloud sync export/import), importers/ynab4.ts (YNAB4 budget import). * [AI] test(desktop-client): drop export/import perf e2e test Removes packages/desktop-client/e2e/export-import-performance.test.ts, added in an earlier commit to benchmark adm-zip vs fflate. Per review feedback (matt-fidd): this zip-handling logic changes rarely, and a future library swap would warrant its own fresh benchmarking rather than maintaining a ~300 line permanent regression test for it. The benchmark numbers already recorded in the PR description remain accurate for this change. * [AI] fix(loot-core): distinguish oversized-zip errors from generic parse failures cloud-storage.ts's importBuffer (shared by both cloud-sync download and the local "Import my budget -> Actual" file picker) caught every safeUnzip failure the same way and threw FileDownloadError('not-zip-file'). Since safeZip has no size limit but safeUnzip enforces one by default, a self-generated export that grows past that cap (e.g. a large db.sqlite) would fail to re-import with a misleading "this isn't a zip file" message, even though the file is perfectly valid, just larger than the cap. Catches UnsafeZipError separately and surfaces a new 'zip-too-large' reason, with matching UI messages in both places that already handle not-zip-file/invalid-zip-file/invalid-meta-file (loot-core's shared error mapper and desktop-client's local import modal), reusing the same support- link phrasing already used for invalid files. * [AI] fix(loot-core): stop faking runtime config for zip upload size limit Vite bakes process.env.ACTUAL_UPLOAD_FILE_SIZE_LIMIT_MB into the client bundle at build time, so it was never actually runtime-configurable in the browser build despite looking like sync-server's real env var. Drop the env indirection and hardcode a fixed 20MB entry cap / 60MB archive cap instead. * [AI] fix(loot-core): tighten zip entry-name matching in importBuffer * [AI] feat(loot-core): raise zip size limits to 500MB, warn on oversized/low-memory exports Matt-fidd's review on #8393 argued 20MB was too tight for real budgets and asked for (1) a more generous cap, (2) a warning when a backup is created that won't be re-importable, and (3) test coverage for zip.ts's safety checks (path traversal, size limits, duplicate entries, unsafe names, round-trip). Later review passes asked to trim the verbose comments in zip.ts and to use instead of t() for the new warning strings. - Collapse the three separate default caps into one MAX_ZIP_SIZE (500MB), since archive size, per-entry size, and total uncompressed size all shared the same value anyway. safeUnzip can run outside the browser (Electron, or Node via @actual-app/api) where the whole archive is held in memory at once, and some self-hosting providers (PikaPods) run containers with as little as 256MB RAM, so the cap stays flat rather than compounding into a multi-GB worst case. - Add exceedsSafeUnzipLimits() to zip.ts so exportBuffer() can warn upfront, using the same cap import enforces, instead of drifting. - Add #platform/server/memory (default/api/electron) exposing os.freemem() on node/electron and null in the browser, per the existing platform-conditional-export pattern. exportBuffer() warns if the uncompressed db.sqlite is bigger than currently-free memory; skipped entirely when the platform has no such API. - Surface both warnings in the Export settings screen via . - Add zip.test.ts covering round trip, path traversal, unsafe names, duplicate entries, and all three size caps. - Trim zip.ts's file-header comment per review feedback that it was too verbose to be useful. * [AI] feat: surface descriptive, translatable errors for unsafe zip imports UnsafeZipError now carries structured meta (zipReason + entryName/maxSize) instead of only a free-text English message. The meta rides on FileDownloadError('zip-too-large', ...) through the download and import-budget paths, and the desktop-client maps it to detailed, translated messages (which file, which limit) in getUnsafeZipError(); loot-core's shared getDownloadError() gets a plain-English equivalent for the headless API. Why structured meta rather than translating the message itself: i18n must not live in loot-core (see platform/client/connection/index.ts) - the backend worker has no i18next instance and doesn't know the user's locale, and a message translated at throw time would bake a locale into a string that also feeds the headless @actual-app/api and server logs, which want stable English. i18next also needs a static key plus interpolation params to extract and look up translations, so the error must cross the worker boundary as code + params (zipReason + entryName/maxSize) and be rendered with t() at display time in the client. The English Error.message is kept for logs and the API. * Update release note to include safe wrappers * [AI] fix(loot-core): require db.sqlite and metadata.json from the same archive directory importBuffer previously located db.sqlite and metadata.json independently, so an archive with a/db.sqlite and b/metadata.json would pair unrelated files, and duplicate matches were resolved by archive order. Now prefer the root-level pair; otherwise both files must come from exactly one shared directory, and anything ambiguous is rejected as invalid-zip-file. * [AI] fix(loot-core): restrict safeZip to flat entries so every name is validated safeZip only ran assertSafeEntryName on top-level keys, but fflate's Zippable type permits nested directory objects whose child names would reach zipSync unvalidated. Both callers pass flat records, so narrow the parameter to Record and let the compiler rule out nested input. --- .../adm-zip-npm-0.5.16-4556fea098.patch | 10 -- package.json | 1 - .../src/budgetfiles/budgetfilesSlice.ts | 8 +- .../modals/manager/ImportActualModal.tsx | 4 + .../src/components/settings/Export.tsx | 19 +++ packages/desktop-client/src/util/error.ts | 50 ++++++ packages/loot-core/package.json | 8 +- .../src/platform/server/memory/index.api.ts | 6 + .../platform/server/memory/index.electron.ts | 2 + .../src/platform/server/memory/index.ts | 4 + .../loot-core/src/server/budgetfiles/app.ts | 6 +- .../src/server/budgetfiles/backups.ts | 40 ++++- .../loot-core/src/server/cloud-storage.ts | 78 ++++++--- packages/loot-core/src/server/errors.ts | 4 +- .../loot-core/src/server/importers/actual.ts | 2 +- .../loot-core/src/server/importers/ynab4.ts | 33 ++-- .../loot-core/src/server/util/zip.test.ts | 149 ++++++++++++++++++ packages/loot-core/src/server/util/zip.ts | 123 +++++++++++++++ packages/loot-core/src/shared/errors.ts | 53 +++++++ .../replace-adm-zip-with-fflate.md | 6 + yarn.lock | 33 +--- 21 files changed, 556 insertions(+), 83 deletions(-) delete mode 100644 .yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch create mode 100644 packages/loot-core/src/platform/server/memory/index.api.ts create mode 100644 packages/loot-core/src/platform/server/memory/index.electron.ts create mode 100644 packages/loot-core/src/platform/server/memory/index.ts create mode 100644 packages/loot-core/src/server/util/zip.test.ts create mode 100644 packages/loot-core/src/server/util/zip.ts create mode 100644 upcoming-release-notes/replace-adm-zip-with-fflate.md diff --git a/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch b/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch deleted file mode 100644 index d7ee2e285e..0000000000 --- a/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch +++ /dev/null @@ -1,10 +0,0 @@ -diff --git a/methods/inflater.js b/methods/inflater.js -index 8769e66e82b25541aba80b1ac6429199c9a8179f..1d4402402f0e1aaf64062c1f004c3d6e6fe93e76 100644 ---- a/methods/inflater.js -+++ b/methods/inflater.js -@@ -1,4 +1,4 @@ --const version = +(process.versions ? process.versions.node : "").split(".")[0] || 0; -+const version = +(process?.versions?.node ?? "").split(".")[0] || 0; - - module.exports = function (/*Buffer*/ inbuf, /*number*/ expectedLength) { - var zlib = require("zlib"); diff --git a/package.json b/package.json index e6c581b48c..5086204989 100644 --- a/package.json +++ b/package.json @@ -112,7 +112,6 @@ "glob-hasher-linux-arm64-gnu": "1.4.2" }, "resolutions": { - "adm-zip": "patch:adm-zip@npm%3A0.5.16#~/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch", "minimatch@3.1.2": "3.1.5", "serialize-javascript": "^7.0.5", "socks": ">=2.8.3", diff --git a/packages/desktop-client/src/budgetfiles/budgetfilesSlice.ts b/packages/desktop-client/src/budgetfiles/budgetfilesSlice.ts index f52dea15b0..15ea6c4628 100644 --- a/packages/desktop-client/src/budgetfiles/budgetfilesSlice.ts +++ b/packages/desktop-client/src/budgetfiles/budgetfilesSlice.ts @@ -1,5 +1,6 @@ import { send } from '@actual-app/core/platform/client/connection'; import type { RemoteFile } from '@actual-app/core/server/cloud-storage'; +import { getUnsafeZipMeta } from '@actual-app/core/shared/errors'; import type { Budget } from '@actual-app/core/types/budget'; import type { File } from '@actual-app/core/types/file'; import type { Handlers } from '@actual-app/core/types/handlers'; @@ -12,7 +13,7 @@ import { closeModal, pushModal } from '#modals/modalsSlice'; import { loadGlobalPrefs, loadPrefs } from '#prefs/prefsSlice'; import { createAppAsyncThunk } from '#redux'; import { signOut } from '#users/usersSlice'; -import { getDownloadError, getSyncError } from '#util/error'; +import { getDownloadError, getSyncError, getUnsafeZipError } from '#util/error'; const sliceName = 'budgetfiles'; @@ -243,9 +244,10 @@ type ImportBudgetPayload = { export const importBudget = createAppAsyncThunk( `${sliceName}/importBudget`, async ({ filepath, type }: ImportBudgetPayload, { dispatch }) => { - const { error } = await send('import-budget', { filepath, type }); + const { error, meta } = await send('import-budget', { filepath, type }); if (error) { - throw new Error(error); + const zipMeta = getUnsafeZipMeta(meta); + throw new Error(zipMeta ? getUnsafeZipError(zipMeta) : error); } dispatch(closeModal()); diff --git a/packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx b/packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx index d838738a80..91724bd46a 100644 --- a/packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx +++ b/packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx @@ -39,6 +39,10 @@ export function ImportActualModal() { return t('This archive is not a valid Actual export file.'); case 'invalid-metadata-file': return t('The metadata file in the given archive is corrupted.'); + case 'zip-too-large': + return t( + 'This file is too large to import, sorry! Visit https://actualbudget.org/contact/ for support.', + ); default: return t( 'An unknown error occurred while importing. Please report this as a new issue on GitHub.', diff --git a/packages/desktop-client/src/components/settings/Export.tsx b/packages/desktop-client/src/components/settings/Export.tsx index 617a48f2fe..efc39c800f 100644 --- a/packages/desktop-client/src/components/settings/Export.tsx +++ b/packages/desktop-client/src/components/settings/Export.tsx @@ -16,12 +16,14 @@ export function ExportBudget() { const { t } = useTranslation(); const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState(null); + const [warnings, setWarnings] = useState([]); const [budgetName] = useMetadataPref('budgetName'); const [encryptKeyId] = useMetadataPref('encryptKeyId'); async function onExport() { setIsLoading(true); setError(null); + setWarnings([]); const response = await send('export-budget'); @@ -33,6 +35,7 @@ export function ExportBudget() { } if (response.data) { + setWarnings(response.warnings ?? []); void window.Actual.saveFile( response.data, `${format(new Date(), 'yyyy-MM-dd')}-${budgetName}.zip`, @@ -56,6 +59,22 @@ export function ExportBudget() { )} )} + {warnings.includes('exceeds-import-size-limit') && ( + + + This export is larger than Actual can safely re-import. You may + not be able to restore this backup. + + + )} + {warnings.includes('may-exceed-available-memory') && ( + + + This export is larger than the memory available on this device. + Restoring it here may fail. + + + )} } > diff --git a/packages/desktop-client/src/util/error.ts b/packages/desktop-client/src/util/error.ts index 53afcd7fe7..ed1bafa0dd 100644 --- a/packages/desktop-client/src/util/error.ts +++ b/packages/desktop-client/src/util/error.ts @@ -1,3 +1,5 @@ +import { getUnsafeZipMeta, toMB } from '@actual-app/core/shared/errors'; +import type { UnsafeZipMeta } from '@actual-app/core/shared/errors'; import { t } from 'i18next'; type ErrorWithMeta = { @@ -5,6 +7,45 @@ type ErrorWithMeta = { meta?: unknown; }; +export function getUnsafeZipError({ + zipReason, + entryName, + maxSize, +}: UnsafeZipMeta): string { + const maxSizeMB = toMB(maxSize); + switch (zipReason) { + case 'archive-size': + return t( + 'This file is larger than the maximum supported size of {{maxSizeMB}} MB, sorry! Visit https://actualbudget.org/contact/ for support.', + { maxSizeMB }, + ); + case 'entry-size': + return t( + 'The file "{{entryName}}" in this archive is larger than the maximum supported size of {{maxSizeMB}} MB, sorry! Visit https://actualbudget.org/contact/ for support.', + { entryName, maxSizeMB }, + ); + case 'total-size': + return t( + 'The uncompressed contents of this archive are larger than the maximum supported size of {{maxSizeMB}} MB, sorry! Visit https://actualbudget.org/contact/ for support.', + { maxSizeMB }, + ); + case 'unsafe-entry-name': + return t( + 'This archive contains an entry with an unsafe file name: "{{entryName}}".', + { entryName }, + ); + case 'duplicate-entry': + return t( + 'This archive contains more than one entry named "{{entryName}}".', + { entryName }, + ); + default: + return t( + 'This file could not be imported, sorry! Visit https://actualbudget.org/contact/ for support.', + ); + } +} + export function getUploadError({ reason, meta }: ErrorWithMeta) { switch (reason) { case 'unauthorized': @@ -88,6 +129,15 @@ export function getDownloadError({ return t( 'Downloaded file is invalid, sorry! Visit https://actualbudget.org/contact/ for support.', ); + case 'zip-too-large': { + const zipMeta = getUnsafeZipMeta(meta); + if (zipMeta) { + return getUnsafeZipError(zipMeta); + } + return t( + 'This file is too large to import, sorry! Visit https://actualbudget.org/contact/ for support.', + ); + } case 'decrypt-failure': return t( 'Unable to decrypt file {{fileName}}. To change your key, first download this file with the proper password.', diff --git a/packages/loot-core/package.json b/packages/loot-core/package.json index d6d08f6cd0..2a94d6aa9b 100644 --- a/packages/loot-core/package.json +++ b/packages/loot-core/package.json @@ -51,6 +51,11 @@ "api": "./src/platform/server/fs/path-join.api.ts", "default": "./src/platform/server/fs/path-join.ts" }, + "#platform/server/memory": { + "electron": "./src/platform/server/memory/index.electron.ts", + "api": "./src/platform/server/memory/index.api.ts", + "default": "./src/platform/server/memory/index.ts" + }, "#platform/server/sqlite": { "electron": "./src/platform/server/sqlite/index.electron.ts", "api": "./src/platform/server/sqlite/index.api.ts", @@ -195,12 +200,12 @@ "@rschedule/core": "^1.5.0", "@rschedule/standard-date-adapter": "^1.5.0", "absurd-sql": "0.0.54", - "adm-zip": "^0.5.17", "better-sqlite3": "^12.10.0", "csv-parse": "^6.2.1", "csv-stringify": "^6.7.0", "date-fns": "^4.4.0", "es-toolkit": "^1.47.1", + "fflate": "^0.8.3", "handlebars": "^4.7.9", "hyperformula": "^3.3.0", "lru-cache": "^11.5.1", @@ -211,7 +216,6 @@ "devDependencies": { "@actual-app/crdt": "workspace:*", "@actual-app/vite-plugin-peggy": "workspace:*", - "@types/adm-zip": "^0.5.8", "@types/better-sqlite3": "^7.6.13", "@types/jlongster__sql.js": "npm:@types/sql.js@latest", "@types/node": "^22.19.21", diff --git a/packages/loot-core/src/platform/server/memory/index.api.ts b/packages/loot-core/src/platform/server/memory/index.api.ts new file mode 100644 index 0000000000..e3909ec40e --- /dev/null +++ b/packages/loot-core/src/platform/server/memory/index.api.ts @@ -0,0 +1,6 @@ +import * as os from 'os'; + +import type * as T from './index'; + +export const getAvailableMemory: typeof T.getAvailableMemory = () => + os.freemem(); diff --git a/packages/loot-core/src/platform/server/memory/index.electron.ts b/packages/loot-core/src/platform/server/memory/index.electron.ts new file mode 100644 index 0000000000..9c98dddf3d --- /dev/null +++ b/packages/loot-core/src/platform/server/memory/index.electron.ts @@ -0,0 +1,2 @@ +// oxlint-disable-next-line no-restricted-imports +export * from './index.api'; diff --git a/packages/loot-core/src/platform/server/memory/index.ts b/packages/loot-core/src/platform/server/memory/index.ts new file mode 100644 index 0000000000..d49972fc5b --- /dev/null +++ b/packages/loot-core/src/platform/server/memory/index.ts @@ -0,0 +1,4 @@ +// No OS-level memory API is available in a browser context. +export function getAvailableMemory(): number | null { + return null; +} diff --git a/packages/loot-core/src/server/budgetfiles/app.ts b/packages/loot-core/src/server/budgetfiles/app.ts index da6fbce9b9..f5e50384fa 100644 --- a/packages/loot-core/src/server/budgetfiles/app.ts +++ b/packages/loot-core/src/server/budgetfiles/app.ts @@ -481,7 +481,7 @@ async function importBudget({ * to derive the budget name for some import types. */ filename?: string; -}): Promise<{ error?: string; id?: string }> { +}): Promise<{ error?: string; meta?: unknown; id?: string }> { try { let contents: Buffer; let name: string; @@ -514,8 +514,10 @@ async function importBudget({ async function exportBudget() { try { + const exported = await cloudStorage.exportBuffer(); return { - data: await cloudStorage.exportBuffer(), + data: exported?.data ?? null, + warnings: exported?.warnings ?? [], }; } catch (err) { err.message = 'Error exporting budget: ' + err.message; diff --git a/packages/loot-core/src/server/budgetfiles/backups.ts b/packages/loot-core/src/server/budgetfiles/backups.ts index f0da366ee0..5a0d9366e4 100644 --- a/packages/loot-core/src/server/budgetfiles/backups.ts +++ b/packages/loot-core/src/server/budgetfiles/backups.ts @@ -1,6 +1,5 @@ import type { Database } from '@jlongster/sql.js'; // @ts-strict-ignore -import AdmZip from 'adm-zip'; import * as dateFns from 'date-fns'; import * as connection from '#platform/server/connection'; @@ -9,6 +8,7 @@ import { logger } from '#platform/server/log'; import * as sqlite from '#platform/server/sqlite'; import * as cloudStorage from '#server/cloud-storage'; import * as prefs from '#server/prefs'; +import { safeUnzip, safeZip } from '#server/util/zip'; import * as monthUtils from '#shared/months'; // A special backup that represents the latest version of the db that @@ -137,10 +137,16 @@ export async function makeBackup(id: string) { sqlite.runQuery(db, 'DELETE FROM messages_crdt'); sqlite.runQuery(db, 'DELETE FROM messages_clock'); // Zip up the cleaned db and metadata into a single backup file - const zip = new AdmZip(); - zip.addLocalFile(tempDbPath, '', 'db.sqlite'); - zip.addLocalFile(fs.join(budgetDir, 'metadata.json')); - zip.writeZip(backupPath); + const dbContent = await fs.readFile(tempDbPath, 'binary'); + const metaContent = await fs.readFile( + fs.join(budgetDir, 'metadata.json'), + 'binary', + ); + const zipped = safeZip({ + 'db.sqlite': dbContent, + 'metadata.json': metaContent, + }); + await fs.writeFile(backupPath, zipped); } finally { if (db) { sqlite.closeDatabase(db); @@ -224,9 +230,27 @@ export async function loadBackup(id: string, backupId: string) { prefs.unloadPrefs(); - const zip = new AdmZip(fs.join(budgetDir, 'backups', backupId)); - zip.extractEntryTo('db.sqlite', budgetDir, false, true); - zip.extractEntryTo('metadata.json', budgetDir, false, true); + const zipContent = await fs.readFile( + fs.join(budgetDir, 'backups', backupId), + 'binary', + ); + + let entries: Record; + try { + entries = safeUnzip(zipContent); + } catch (e) { + logger.log(e); + throw new Error('Error reading backup zip file'); + } + if (!entries['db.sqlite'] || !entries['metadata.json']) { + throw new Error('Backup zip file is missing db.sqlite or metadata.json'); + } + + await fs.writeFile(fs.join(budgetDir, 'db.sqlite'), entries['db.sqlite']); + await fs.writeFile( + fs.join(budgetDir, 'metadata.json'), + entries['metadata.json'], + ); } } diff --git a/packages/loot-core/src/server/cloud-storage.ts b/packages/loot-core/src/server/cloud-storage.ts index 5633e03c37..c79a202db3 100644 --- a/packages/loot-core/src/server/cloud-storage.ts +++ b/packages/loot-core/src/server/cloud-storage.ts @@ -1,11 +1,11 @@ // @ts-strict-ignore -import AdmZip from 'adm-zip'; import { v4 as uuidv4 } from 'uuid'; import * as asyncStorage from '#platform/server/asyncStorage'; import { fetch } from '#platform/server/fetch'; import * as fs from '#platform/server/fs'; import { logger } from '#platform/server/log'; +import * as memory from '#platform/server/memory'; import * as sqlite from '#platform/server/sqlite'; import * as monthUtils from '#shared/months'; @@ -20,6 +20,12 @@ import { runMutator } from './mutators'; import { post } from './post'; import * as prefs from './prefs'; import { getServer } from './server-config'; +import { + exceedsSafeUnzipLimits, + safeUnzip, + safeZip, + UnsafeZipError, +} from './util/zip'; const UPLOAD_FREQUENCY_IN_DAYS = 7; @@ -145,14 +151,11 @@ export async function exportBuffer() { const budgetDir = fs.getBudgetDir(id); - // create zip - const zipped = new AdmZip(); - // We run this in a mutator even though its not mutating anything // because we are reading the sqlite file from disk. We want to make // sure that we get a valid snapshot of it so we want this to be // serialized with all other mutations. - await runMutator(async () => { + const { zipped, entries } = await runMutator(async () => { const rawDbContent = await fs.readFile( fs.join(budgetDir, 'db.sqlite'), 'binary', @@ -183,30 +186,66 @@ export async function exportBuffer() { meta.resetClock = true; const metaContent = Buffer.from(JSON.stringify(meta), 'utf8'); - zipped.addFile('db.sqlite', Buffer.from(dbContent)); - zipped.addFile('metadata.json', metaContent); + const entries = { + 'db.sqlite': Buffer.from(dbContent), + 'metadata.json': metaContent, + }; + + return { zipped: safeZip(entries), entries }; }); - return Buffer.from(zipped.toBuffer()); + const warnings: string[] = []; + if (exceedsSafeUnzipLimits(zipped, entries)) { + warnings.push('exceeds-import-size-limit'); + } + + const availableMemory = memory.getAvailableMemory(); + if ( + availableMemory != null && + entries['db.sqlite'].length > availableMemory + ) { + warnings.push('may-exceed-available-memory'); + } + + return { data: Buffer.from(zipped), warnings }; } export async function importBuffer(fileData, buffer) { - let zipped, entries; + let entries; try { - zipped = new AdmZip(buffer); - entries = zipped.getEntries(); - } catch { + entries = safeUnzip(buffer); + } catch (e) { + if (e instanceof UnsafeZipError) { + throw FileDownloadError('zip-too-large', e.meta); + } throw FileDownloadError('not-zip-file'); } - const dbEntry = entries.find(e => e.entryName.includes('db.sqlite')); - const metaEntry = entries.find(e => e.entryName.includes('metadata.json')); + const entryNames = Object.keys(entries); + const dbDirs = entryNames + .filter(name => name === 'db.sqlite' || name.endsWith('/db.sqlite')) + .map(name => name.slice(0, -'db.sqlite'.length)); + const metaDirs = entryNames + .filter(name => name === 'metadata.json' || name.endsWith('/metadata.json')) + .map(name => name.slice(0, -'metadata.json'.length)); - if (!dbEntry || !metaEntry) { + // Both files must come from the same directory: prefer the archive root, + // otherwise there must be exactly one directory containing both. + const sharedDirs = dbDirs.filter(dir => metaDirs.includes(dir)); + const dir = sharedDirs.includes('') + ? '' + : sharedDirs.length === 1 + ? sharedDirs[0] + : null; + + if (dir == null) { throw FileDownloadError('invalid-zip-file'); } - const dbContent = zipped.readFile(dbEntry); - const metaContent = zipped.readFile(metaEntry); + const entryName = dir + 'db.sqlite'; + const metaEntryName = dir + 'metadata.json'; + + const dbContent = Buffer.from(entries[entryName]); + const metaContent = Buffer.from(entries[metaEntryName]); let meta; try { @@ -254,10 +293,11 @@ export async function upload() { throw FileUploadError('unauthorized'); } - const zipContent = await exportBuffer(); - if (zipContent == null) { + const exported = await exportBuffer(); + if (exported == null) { return; } + const zipContent = exported.data; const { id, diff --git a/packages/loot-core/src/server/errors.ts b/packages/loot-core/src/server/errors.ts index e304f401e5..86ae93a37f 100644 --- a/packages/loot-core/src/server/errors.ts +++ b/packages/loot-core/src/server/errors.ts @@ -1,3 +1,5 @@ +import type { UnsafeZipMeta } from '#shared/errors'; + // TODO: normalize error types export class PostError extends Error { meta: { meta: string } | undefined; @@ -111,7 +113,7 @@ export function FileDownloadError( isMissingKey?: boolean; name?: string; id?: string; - }, + } & Partial, ) { return { type: 'FileDownloadError', reason, meta }; } diff --git a/packages/loot-core/src/server/importers/actual.ts b/packages/loot-core/src/server/importers/actual.ts index 7a5070d116..b8fca76bdb 100644 --- a/packages/loot-core/src/server/importers/actual.ts +++ b/packages/loot-core/src/server/importers/actual.ts @@ -19,7 +19,7 @@ export async function importActual(_filepath: string, buffer: Buffer) { )); } catch (e) { if (e.type === 'FileDownloadError') { - return { error: e.reason }; + return { error: e.reason, meta: e.meta }; } throw e; } diff --git a/packages/loot-core/src/server/importers/ynab4.ts b/packages/loot-core/src/server/importers/ynab4.ts index 021f5b6b87..f8dbdf2788 100644 --- a/packages/loot-core/src/server/importers/ynab4.ts +++ b/packages/loot-core/src/server/importers/ynab4.ts @@ -1,9 +1,9 @@ // @ts-strict-ignore -import AdmZip from 'adm-zip'; import { v4 as uuidv4 } from 'uuid'; import { logger } from '#platform/server/log'; import { send } from '#server/main-app'; +import { safeUnzip } from '#server/util/zip'; import * as monthUtils from '#shared/months'; import { amountToInteger, groupBy, sortByKey } from '#shared/util'; @@ -343,10 +343,13 @@ function estimateRecentness(str: string) { }, 0); } -function findLatestDevice(zipped: AdmZip, entries: AdmZip.IZipEntry[]): string { +function findLatestDevice( + zipped: Record, + entries: string[], +): string { let devices = entries .map(entry => { - const contents = zipped.readFile(entry).toString('utf8'); + const contents = Buffer.from(zipped[entry]).toString('utf8'); let data; try { @@ -411,8 +414,8 @@ export function getBudgetName(filepath) { return m[1]; } -function getFile(entries: AdmZip.IZipEntry[], path: string) { - const files = entries.filter(e => e.entryName === path); +function getFile(entries: string[], path: string) { + const files = entries.filter(e => e === path); if (files.length === 0) { throw new Error('Could not find file: ' + path); } @@ -432,28 +435,36 @@ function join(...paths: string[]): string { } export function parseFile(buffer: Buffer): YNAB4.YFull { - const zipped = new AdmZip(buffer); - const entries = zipped.getEntries(); + let zipped: Record; + try { + zipped = safeUnzip(buffer); + } catch (e) { + logger.log(e); + throw new Error('Error reading zip file'); + } + const entries = Object.keys(zipped); let root = ''; - const dirMatch = entries[0].entryName.match(/([^/]*\.ynab4)/); + const dirMatch = entries[0].match(/([^/]*\.ynab4)/); if (dirMatch) { root = dirMatch[1] + '/'; } - const metaStr = zipped.readFile(getFile(entries, root + 'Budget.ymeta')); + const metaStr = Buffer.from(zipped[getFile(entries, root + 'Budget.ymeta')]); const meta = JSON.parse(metaStr.toString('utf8')); const budgetPath = join(root, meta.relativeDataFolderName); const deviceFiles = entries.filter(e => - e.entryName.startsWith(join(budgetPath, 'devices')), + e.startsWith(join(budgetPath, 'devices')), ); const deviceGUID = findLatestDevice(zipped, deviceFiles); const yfullPath = join(budgetPath, deviceGUID, 'Budget.yfull'); let contents; try { - contents = zipped.readFile(getFile(entries, yfullPath)).toString('utf8'); + contents = Buffer.from(zipped[getFile(entries, yfullPath)]).toString( + 'utf8', + ); } catch (e) { logger.log(e); throw new Error('Error reading Budget.yfull file'); diff --git a/packages/loot-core/src/server/util/zip.test.ts b/packages/loot-core/src/server/util/zip.test.ts new file mode 100644 index 0000000000..13a07a2922 --- /dev/null +++ b/packages/loot-core/src/server/util/zip.test.ts @@ -0,0 +1,149 @@ +import { zipSync } from 'fflate'; +import { afterAll, beforeAll, describe, expect, test } from 'vitest'; + +import { + exceedsSafeUnzipLimits, + safeUnzip, + safeZip, + UnsafeZipError, +} from './zip'; + +// fflate stamps each entry with the current date in DOS format, which +// rejects the fake pre-1980 clock the test setup freezes `Date.now` to. +const testGlobal = global as unknown as { + restoreDateNow: () => void; + restoreFakeDateNow: () => void; +}; +beforeAll(() => testGlobal.restoreDateNow()); +afterAll(() => testGlobal.restoreFakeDateNow()); + +describe('safeZip / safeUnzip round trip', () => { + test('unzips exactly what was zipped', () => { + const zipped = safeZip({ + 'db.sqlite': Buffer.from('sqlite-content'), + 'metadata.json': Buffer.from('{"id":"abc"}'), + }); + + const entries = safeUnzip(zipped); + + expect(Buffer.from(entries['db.sqlite']).toString()).toBe('sqlite-content'); + expect(Buffer.from(entries['metadata.json']).toString()).toBe( + '{"id":"abc"}', + ); + }); + + test('round trips a nested entry name', () => { + const zipped = safeZip({ + 'budget/db.sqlite': Buffer.from('nested'), + }); + + const entries = safeUnzip(zipped); + + expect(Buffer.from(entries['budget/db.sqlite']).toString()).toBe('nested'); + }); +}); + +describe('unsafe entry names', () => { + test.each([ + ['path traversal', '../etc/passwd'], + ['path traversal in a nested segment', 'foo/../../etc/passwd'], + ['absolute path', '/etc/passwd'], + ['backslash', 'foo\\bar'], + ['windows drive letter', 'C:\\evil.txt'], + ['null byte', 'foo\0bar'], + ])('safeZip rejects %s', (_name, entryName) => { + expect(() => safeZip({ [entryName]: Buffer.from('x') })).toThrow( + UnsafeZipError, + ); + }); + + test.each([ + ['path traversal', '../etc/passwd'], + ['absolute path', '/etc/passwd'], + ['backslash', 'foo\\bar'], + ['windows drive letter', 'C:\\evil.txt'], + ])('safeUnzip rejects %s', (_name, entryName) => { + // Build the archive with fflate directly since safeZip would already + // reject these names before we get a chance to unzip them. + const zipped = zipSync({ [entryName]: Buffer.from('x') }); + + expect(() => safeUnzip(zipped)).toThrow(UnsafeZipError); + }); +}); + +describe('duplicate entries', () => { + test('safeUnzip rejects case-insensitive duplicate names', () => { + const zipped = zipSync({ + 'db.sqlite': Buffer.from('a'), + 'DB.SQLITE': Buffer.from('b'), + }); + + expect(() => safeUnzip(zipped)).toThrow(UnsafeZipError); + }); +}); + +describe('size limits', () => { + test('safeUnzip rejects an archive over maxArchiveSize', () => { + const zipped = safeZip({ 'db.sqlite': Buffer.from('x'.repeat(1000)) }); + + expect(() => safeUnzip(zipped, { maxArchiveSize: 10 })).toThrow( + UnsafeZipError, + ); + }); + + test('safeUnzip rejects a single entry over maxEntrySize', () => { + const zipped = safeZip({ 'db.sqlite': Buffer.from('x'.repeat(1000)) }); + + expect(() => + safeUnzip(zipped, { maxArchiveSize: 10_000, maxEntrySize: 10 }), + ).toThrow(UnsafeZipError); + }); + + test('safeUnzip rejects when total uncompressed size exceeds the cap', () => { + const zipped = safeZip({ + a: Buffer.from('x'.repeat(100)), + b: Buffer.from('x'.repeat(100)), + }); + + expect(() => + safeUnzip(zipped, { + maxArchiveSize: 10_000, + maxEntrySize: 10_000, + maxTotalUncompressedSize: 150, + }), + ).toThrow(UnsafeZipError); + }); + + test('safeUnzip accepts an archive within all limits', () => { + const zipped = safeZip({ 'db.sqlite': Buffer.from('small') }); + + expect(() => safeUnzip(zipped)).not.toThrow(); + }); +}); + +describe('exceedsSafeUnzipLimits', () => { + test('reports false for a small archive', () => { + const entries = { 'db.sqlite': Buffer.from('small') }; + const zipped = safeZip(entries); + + expect(exceedsSafeUnzipLimits(zipped, entries)).toBe(false); + }); + + test('reports true when an entry is larger than safeUnzip would allow', () => { + // Simulate an oversized entry without actually allocating 500MB+. + const bigEntry = { length: 600 * 1024 * 1024 } as unknown as Uint8Array; + const smallArchive = new Uint8Array(10); + + expect( + exceedsSafeUnzipLimits(smallArchive, { 'db.sqlite': bigEntry }), + ).toBe(true); + }); + + test('reports true when the archive itself is larger than safeUnzip would allow', () => { + const bigArchive = { + length: 2 * 1024 * 1024 * 1024, + } as unknown as Uint8Array; + + expect(exceedsSafeUnzipLimits(bigArchive, {})).toBe(true); + }); +}); diff --git a/packages/loot-core/src/server/util/zip.ts b/packages/loot-core/src/server/util/zip.ts new file mode 100644 index 0000000000..c3a9458c83 --- /dev/null +++ b/packages/loot-core/src/server/util/zip.ts @@ -0,0 +1,123 @@ +import { unzipSync, zipSync } from 'fflate'; +import type { Unzipped } from 'fflate'; + +import type { UnsafeZipMeta } from '#shared/errors'; + +// fflate does no validation itself: guard against zip-slip, decompression +// bombs, and duplicate entries. + +const MAX_ZIP_SIZE = 500 * 1024 * 1024; // 500MB; also doubles as a memory-safety cap + +export class UnsafeZipError extends Error { + readonly meta: UnsafeZipMeta; + + constructor(message: string, meta: UnsafeZipMeta) { + super(message); + this.meta = meta; + } +} + +function assertSafeEntryName(name: string) { + const isTraversal = name.split('/').some(segment => segment === '..'); + + if ( + name.includes('\0') || + name.includes('\\') || + /^[a-zA-Z]:/.test(name) || + name.startsWith('/') || + isTraversal + ) { + throw new UnsafeZipError(`Unsafe zip entry name: ${name}`, { + zipReason: 'unsafe-entry-name', + entryName: name, + }); + } +} + +type SafeUnzipOptions = { + maxArchiveSize?: number; + maxEntrySize?: number; + maxTotalUncompressedSize?: number; +}; + +export function safeUnzip( + data: Uint8Array, + { + maxArchiveSize = MAX_ZIP_SIZE, + maxEntrySize = MAX_ZIP_SIZE, + maxTotalUncompressedSize = MAX_ZIP_SIZE, + }: SafeUnzipOptions = {}, +): Unzipped { + if (data.length > maxArchiveSize) { + throw new UnsafeZipError( + `Zip archive exceeds maximum size of ${maxArchiveSize} bytes`, + { zipReason: 'archive-size', maxSize: maxArchiveSize }, + ); + } + + const seen = new Set(); + + let totalUncompressedSize = 0; + + return unzipSync(data, { + filter(file) { + assertSafeEntryName(file.name); + + if (file.originalSize > maxEntrySize) { + throw new UnsafeZipError( + `Zip entry "${file.name}" exceeds maximum size of ${maxEntrySize} bytes`, + { + zipReason: 'entry-size', + entryName: file.name, + maxSize: maxEntrySize, + }, + ); + } + + totalUncompressedSize += file.originalSize; + if (totalUncompressedSize > maxTotalUncompressedSize) { + throw new UnsafeZipError( + `Zip archive's total uncompressed size exceeds maximum of ${maxTotalUncompressedSize} bytes`, + { zipReason: 'total-size', maxSize: maxTotalUncompressedSize }, + ); + } + + const normalized = file.name.toLowerCase(); + if (seen.has(normalized)) { + throw new UnsafeZipError( + `Zip archive contains a duplicate entry: ${file.name}`, + { zipReason: 'duplicate-entry', entryName: file.name }, + ); + } + seen.add(normalized); + + return true; + }, + }); +} + +export function safeZip(files: Record): Uint8Array { + for (const name of Object.keys(files)) { + assertSafeEntryName(name); + } + return zipSync(files); +} + +export function exceedsSafeUnzipLimits( + archive: Uint8Array, + entries: Record, +): boolean { + if (archive.length > MAX_ZIP_SIZE) { + return true; + } + + let totalUncompressedSize = 0; + for (const entry of Object.values(entries)) { + if (entry.length > MAX_ZIP_SIZE) { + return true; + } + totalUncompressedSize += entry.length; + } + + return totalUncompressedSize > MAX_ZIP_SIZE; +} diff --git a/packages/loot-core/src/shared/errors.ts b/packages/loot-core/src/shared/errors.ts index 01d2e826fd..237654831e 100644 --- a/packages/loot-core/src/shared/errors.ts +++ b/packages/loot-core/src/shared/errors.ts @@ -3,6 +3,52 @@ type ErrorWithMeta = { meta?: unknown; }; +export type UnsafeZipMeta = { + zipReason: + | 'unsafe-entry-name' + | 'archive-size' + | 'entry-size' + | 'total-size' + | 'duplicate-entry'; + entryName?: string; + maxSize?: number; +}; + +export function getUnsafeZipMeta(meta?: unknown): UnsafeZipMeta | null { + if ( + meta && + typeof meta === 'object' && + 'zipReason' in meta && + typeof meta.zipReason === 'string' + ) { + return meta as UnsafeZipMeta; + } + return null; +} + +export function toMB(bytes?: number): number | null { + return bytes != null ? Math.round(bytes / (1024 * 1024)) : null; +} + +function getUnsafeZipError(meta: UnsafeZipMeta): string { + const { entryName } = meta; + const maxSizeMB = toMB(meta.maxSize); + switch (meta.zipReason) { + case 'archive-size': + return `This file is larger than the maximum supported size of ${maxSizeMB} MB, sorry! Visit https://actualbudget.org/contact/ for support.`; + case 'entry-size': + return `The file "${entryName}" in this archive is larger than the maximum supported size of ${maxSizeMB} MB, sorry! Visit https://actualbudget.org/contact/ for support.`; + case 'total-size': + return `The uncompressed contents of this archive are larger than the maximum supported size of ${maxSizeMB} MB, sorry! Visit https://actualbudget.org/contact/ for support.`; + case 'unsafe-entry-name': + return `This archive contains an entry with an unsafe file name: "${entryName}".`; + case 'duplicate-entry': + return `This archive contains more than one entry named "${entryName}".`; + default: + return 'This file could not be imported, sorry! Visit https://actualbudget.org/contact/ for support.'; + } +} + // NOTE: These error formatters are consumed by the headless `@actual-app/api` // (see `src/server/api.ts`), which has no i18n. They intentionally return // plain English strings. User-facing, translated equivalents live in the @@ -48,6 +94,13 @@ export function getDownloadError({ case 'invalid-zip-file': case 'invalid-meta-file': return 'Downloaded file is invalid, sorry! Visit https://actualbudget.org/contact/ for support.'; + case 'zip-too-large': { + const zipMeta = getUnsafeZipMeta(meta); + if (zipMeta) { + return getUnsafeZipError(zipMeta); + } + return 'This file is too large to import, sorry! Visit https://actualbudget.org/contact/ for support.'; + } case 'decrypt-failure': return ( 'Unable to decrypt file ' + diff --git a/upcoming-release-notes/replace-adm-zip-with-fflate.md b/upcoming-release-notes/replace-adm-zip-with-fflate.md new file mode 100644 index 0000000000..446801dffe --- /dev/null +++ b/upcoming-release-notes/replace-adm-zip-with-fflate.md @@ -0,0 +1,6 @@ +--- +category: Maintenance +authors: [StephenBrown2] +--- + +Replace adm-zip with fflate and safe wrappers for zip handling. diff --git a/yarn.lock b/yarn.lock index 5599fcc988..e0b1dbf4f3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -107,13 +107,11 @@ __metadata: "@jlongster/sql.js": "npm:^1.6.7" "@rschedule/core": "npm:^1.5.0" "@rschedule/standard-date-adapter": "npm:^1.5.0" - "@types/adm-zip": "npm:^0.5.8" "@types/better-sqlite3": "npm:^7.6.13" "@types/jlongster__sql.js": "npm:@types/sql.js@latest" "@types/node": "npm:^22.19.21" "@typescript/native-preview": "npm:beta" absurd-sql: "npm:0.0.54" - adm-zip: "npm:^0.5.17" assert: "npm:^2.1.0" better-sqlite3: "npm:^12.10.0" browserify-zlib: "npm:^0.2.0" @@ -126,6 +124,7 @@ __metadata: es-toolkit: "npm:^1.47.1" fake-indexeddb: "npm:^6.2.5" fast-check: "npm:^4.8.0" + fflate: "npm:^0.8.3" handlebars: "npm:^4.7.9" hyperformula: "npm:^3.3.0" jest-diff: "npm:^30.4.1" @@ -8654,15 +8653,6 @@ __metadata: languageName: node linkType: hard -"@types/adm-zip@npm:^0.5.8": - version: 0.5.8 - resolution: "@types/adm-zip@npm:0.5.8" - dependencies: - "@types/node": "npm:*" - checksum: 10/0739e5abda05f5c02b84048a02d308f77dd9e2d4e7d6ac7126830a0000b9e962a1f5795efd96d50df289333af9ae3b264adacfad9623dde66c10570ce65029d5 - languageName: node - linkType: hard - "@types/aria-query@npm:^5.0.1": version: 5.0.4 resolution: "@types/aria-query@npm:5.0.4" @@ -10514,20 +10504,6 @@ __metadata: languageName: node linkType: hard -"adm-zip@npm:0.5.16": - version: 0.5.16 - resolution: "adm-zip@npm:0.5.16" - checksum: 10/e167d1b9e60cde37334efda828fa514680af9facbd4183952f36526390e5c7da9a90ca1e6880dfd3aba7b3517f1506c6178e0dc29cd630b26b98c795f97fc599 - languageName: node - linkType: hard - -"adm-zip@patch:adm-zip@npm%3A0.5.16#~/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch": - version: 0.5.16 - resolution: "adm-zip@patch:adm-zip@npm%3A0.5.16#~/.yarn/patches/adm-zip-npm-0.5.16-4556fea098.patch::version=0.5.16&hash=f6b7d0" - checksum: 10/1a8c866a6ad1b6b6870d39468f26369cc6a43027678708cd9d40ff64274bb45b946f570293d0ac5d23a4a2b360e36168f14a3f9bc1a1cea20b8dce7ec8d8425e - languageName: node - linkType: hard - "agent-base@npm:6": version: 6.0.2 resolution: "agent-base@npm:6.0.2" @@ -15973,6 +15949,13 @@ __metadata: languageName: node linkType: hard +"fflate@npm:^0.8.3": + version: 0.8.3 + resolution: "fflate@npm:0.8.3" + checksum: 10/6ebf528dc9c56e78e715eac615b009b25dc33e15c1920b11ebba44e6d76181c647756a81a23e19247907496b93aa99928514c53090579a65109e026ac2824aa7 + languageName: node + linkType: hard + "figures@npm:^6.1.0": version: 6.1.0 resolution: "figures@npm:6.1.0"