From edb7defc4a2921e4af8e1e8ccb88aa0337034a8a Mon Sep 17 00:00:00 2001 From: Gabriel Stein Date: Thu, 6 Aug 2026 07:23:04 -0300 Subject: [PATCH] feat: make backups real and opt-in instead of a facade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/utils/backup.ts` is 230 lines with a CLI command and a 7-day retention policy — and zero callers. No deletion has ever produced a backup. `restoreBackup` was not even reachable from the CLI, so `backup --list` could only ever show nothing. In the same way, `loadConfig()` was only called by `config --show`: the file written by `config --init` had no effect on cleaning at all. With `{"backupEnabled": true}` in ~/.maccleanerrc, selected items are now MOVED to ~/.mac-cleaner-cli/backup// instead of deleted, and can be brought back with the new `backup --restore `. Three contracts the implementation commits to, stated explicitly because each one is a place where a "backup" feature can quietly betray a user: - A failed backup is NOT turned into a deletion. The item stays where it is and the failure is reported. Ending with the file in place and an error on screen beats ending with the file gone and no copy. - Moving does not free space. `freedSpace` stays 0 and the size is reported as `backedUpSize`, with the UI saying so before the confirmation and again in the summary. Reporting it as "freed" would trade the old facade for a new untruth. - Docker and Homebrew declare `supportsBackup = false`: the external tool does the deleting, so there is nothing of ours to move. Both the interactive flow and the `clean` subcommand warn that those categories WILL be deleted even with backups on. Both entry points honour the setting. Backing up in the interactive flow while `clean` deleted permanently would be worse than no backup at all, since the outcome would depend on which command the user typed. Three bugs in the module, all of which existed only because it had never run: 1. `backupItem` used `path.replace(homedir(), 'HOME')`, which replaces the first occurrence anywhere in the string and, for a path outside home, returned an absolute path — making `join()` land outside the backup directory. It now accepts only paths under home (the single shape `restoreBackup` can undo) and validates via validatePathSafety. 2. `restoreBackup` checked `startsWith(BACKUP_DIR)` with no trailing slash, so `~/.mac-cleaner-cli/backup-anything` passed. Verified it is refused now. 3. `ensureBackupDir` named the folder with an ISO timestamp (millisecond resolution) and used `mkdir recursive`, which accepts an existing directory silently — two sessions in the same millisecond would share it. This surfaced first as suite flakiness, since vitest runs files in parallel. Tests: the previous ones used paths under tmpdir() (outside home) and asserted `expect(typeof result).toBe('boolean')` — they would pass with the backup failing 100% of the time. Replaced with a real round trip: move, assert the original is gone, assert the content in the copy, restore, assert the content is back at the original path. backup.ts was removed from the vitest coverage exclusion list. Verified with the built CLI: a real item backed up from ~/Library/Caches, `backup --list`, `backup --restore`, identical content returned, and `--restore` refused for a path outside the backup folder. --- README.md | 27 ++++ src/commands/clean.test.ts | 113 +++++++++++++++++ src/commands/clean.ts | 40 +++++- src/commands/interactive.ts | 64 +++++++++- src/index.ts | 23 +++- src/scanners/base-scanner.test.ts | 87 ++++++++++++- src/scanners/base-scanner.ts | 32 +++-- src/scanners/docker.ts | 5 + src/scanners/homebrew.ts | 5 + src/types.ts | 10 +- src/utils/backup.test.ts | 196 +++++++++++++++++++++++------- src/utils/backup.ts | 150 +++++++++++++++++++---- vitest.config.ts | 4 +- 13 files changed, 669 insertions(+), 87 deletions(-) diff --git a/README.md b/README.md index 2a7a6fd..beb9b70 100644 --- a/README.md +++ b/README.md @@ -196,9 +196,36 @@ npx mac-cleaner-cli config --show # Manage backups npx mac-cleaner-cli backup --list +npx mac-cleaner-cli backup --restore npx mac-cleaner-cli backup --clean ``` +### Backups (opt-in) + +By default, cleaning is **permanent**: files are removed with `rm -rf` and do +**not** go to the Trash. + +You can opt into backups by setting `backupEnabled` in `~/.maccleanerrc`: + +```json +{ "backupEnabled": true } +``` + +With backups on, selected items are **moved** to `~/.mac-cleaner-cli/backup//` +instead of being deleted, and can be brought back with `backup --restore`. + +Two honest caveats: + +- **Moving does not free disk space.** The files still occupy the same volume. + Space is only reclaimed by `backup --clean`, which deletes backups older than + 7 days permanently. +- **Docker and Homebrew cannot be backed up.** Their cleanup is performed by the + external tool (`docker system prune`, `brew cleanup`), so there is no file for + us to move. The CLI warns you before proceeding when these are selected. + +If a backup fails, the item is **not** deleted — it stays where it is and the +failure is reported. + ### Flags ```bash diff --git a/src/commands/clean.test.ts b/src/commands/clean.test.ts index 25a7109..3cdfe16 100644 --- a/src/commands/clean.test.ts +++ b/src/commands/clean.test.ts @@ -20,6 +20,14 @@ vi.mock('@inquirer/checkbox', () => ({ default: vi.fn(), })); +// loadConfig is mocked so tests never read the developer's real ~/.maccleanerrc +// (and never hit its module-level cache). Default: backups off, which is the +// shipped default. +vi.mock('../utils/config.js', async () => { + const actual = await vi.importActual('../utils/config.js'); + return { ...actual, loadConfig: vi.fn(async () => ({ backupEnabled: false })) }; +}); + vi.mock('child_process', () => ({ exec: vi.fn(), spawn: vi.fn(() => ({ @@ -32,6 +40,8 @@ const inquirerPrompts = { checkbox: inquirerCheckbox.default, }; +const config = await import('../utils/config.js'); + const trashCategory: Category = { id: 'trash', name: 'Trash', @@ -549,3 +559,106 @@ describe('clean command', () => { consoleSpy.mockRestore(); }); }); + +describe('clean command backup handling', () => { + // The `clean` subcommand must honour `backupEnabled` exactly like the + // interactive flow. Backing up in one entry point and deleting permanently in + // the other is worse than not backing up at all: what happens to the user's + // files would depend on which command they happened to type. + const trashScanner = () => ({ + category: trashCategory, + supportsBackup: true, + scan: vi.fn(), + clean: vi.fn().mockResolvedValue({ + category: trashCategory, + cleanedItems: 1, + freedSpace: 0, + backedUpSize: 1000, + errors: [], + }), + }); + + const setup = (scanner: unknown) => { + vi.mocked(scanners.runAllScans).mockResolvedValue({ + results: [ + { + category: trashCategory, + items: [{ path: '/test', size: 1000, name: 'test', isDirectory: false }], + totalSize: 1000, + }, + ], + totalSize: 1000, + totalItems: 1, + }); + vi.mocked(scanners.getScanner).mockReturnValue( + scanner as ReturnType + ); + vi.mocked(inquirerPrompts.confirm).mockResolvedValue(true); + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(config.loadConfig).mockResolvedValue({ backupEnabled: false }); + }); + + it('deletes permanently when backupEnabled is off', async () => { + const scanner = trashScanner(); + setup(scanner); + vi.mocked(config.loadConfig).mockResolvedValue({ backupEnabled: false }); + + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + await cleanCommand({ all: true, yes: true }); + + expect(scanner.clean).toHaveBeenCalledWith(expect.anything(), undefined, undefined); + + consoleSpy.mockRestore(); + }); + + it('passes a backup directory to the scanner when backupEnabled is on', async () => { + const scanner = trashScanner(); + setup(scanner); + vi.mocked(config.loadConfig).mockResolvedValue({ backupEnabled: true }); + + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + await cleanCommand({ all: true, yes: true }); + + const backupDirArg = scanner.clean.mock.calls[0][2]; + expect(backupDirArg).toEqual(expect.stringContaining('.mac-cleaner-cli')); + + consoleSpy.mockRestore(); + }); + + it('warns that moving does not free disk space', async () => { + const scanner = trashScanner(); + setup(scanner); + vi.mocked(config.loadConfig).mockResolvedValue({ backupEnabled: true }); + + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + await cleanCommand({ all: true, yes: true }); + + const output = consoleSpy.mock.calls.flat().join('\n'); + expect(output).toContain('does NOT free disk space'); + + consoleSpy.mockRestore(); + }); + + it('warns which categories cannot be backed up and will be deleted', async () => { + const scanner = { ...trashScanner(), supportsBackup: false }; + setup(scanner); + vi.mocked(config.loadConfig).mockResolvedValue({ backupEnabled: true }); + + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + await cleanCommand({ all: true, yes: true }); + + const output = consoleSpy.mock.calls.flat().join('\n'); + expect(output).toContain('No backup possible'); + // An external-tool scanner must never receive a backup directory. + expect(scanner.clean).toHaveBeenCalledWith(expect.anything(), undefined, undefined); + + consoleSpy.mockRestore(); + }); +}); diff --git a/src/commands/clean.ts b/src/commands/clean.ts index 284c456..3b8dd7c 100644 --- a/src/commands/clean.ts +++ b/src/commands/clean.ts @@ -4,7 +4,7 @@ import checkbox from '@inquirer/checkbox'; import { spawn } from 'child_process'; import type { CategoryId, CleanSummary, CleanableItem, ScanResult, SafetyLevel } from '../types.js'; import { runAllScans, runScans, getScanner, getAllScanners } from '../scanners/index.js'; -import { formatSize, createScanProgress, createCleanProgress } from '../utils/index.js'; +import { formatSize, createScanProgress, createCleanProgress, loadConfig, ensureBackupDir, getBackupDir } from '../utils/index.js'; const DONATION_URL = 'https://ko-fi.com/guhcostan'; @@ -148,11 +148,45 @@ export async function cleanCommand(options: CleanCommandOptions): Promise getScanner(categoryId)) + .filter((scanner) => scanner?.supportsBackup === false) + .map((scanner) => scanner.category.name); + + console.log(); + console.log(chalk.cyan(`Backup is ON — items will be MOVED to ${getBackupDir()}`)); + console.log( + chalk.yellow('⚠ Moving does NOT free disk space. Run "mac-cleaner-cli backup --clean" to reclaim it.') + ); + if (withoutBackup.length > 0) { + console.log( + chalk.red( + `⚠ No backup possible for: ${withoutBackup.join(', ')} (external tool does the cleanup) — these WILL be deleted.` + ) + ); + } + } + + const backupDir = + backupEnabled && selectedItems.some(({ categoryId }) => getScanner(categoryId)?.supportsBackup !== false) + ? await ensureBackupDir() + : undefined; + const cleanProgress = showProgress ? createCleanProgress(selectedItems.length) : null; const cleanResults: CleanSummary = { results: [], totalFreedSpace: 0, + totalBackedUpSize: 0, + backupDir, totalCleanedItems: 0, totalErrors: 0, }; @@ -162,9 +196,11 @@ export async function cleanCommand(options: CleanCommandOptions): Promise = { @@ -92,10 +92,43 @@ export async function interactiveCommand(options: InteractiveOptions = {}): Prom const totalItems = selectedItems.reduce((sum, s) => sum + s.items.length, 0); // Step 5: Confirm + // `backupEnabled` comes from ~/.maccleanerrc. Until now `loadConfig()` was + // only ever called by `config --show` — the file `config --init` wrote had no + // effect whatsoever on cleaning. + const config = await loadConfig(); + const backupEnabled = config.backupEnabled === true; + + const categoriesWithoutBackup = backupEnabled + ? selectedItems + .map(({ categoryId }) => getScanner(categoryId)) + .filter((scanner) => scanner?.supportsBackup === false) + .map((scanner) => scanner.category.name) + : []; + console.log(); console.log(chalk.bold('Summary:')); console.log(` Items to delete: ${chalk.yellow(totalItems.toString())}`); console.log(` Space to free: ${chalk.green(formatSize(totalToClean))}`); + + if (backupEnabled) { + console.log(); + console.log(chalk.cyan(` Backup is ON — items will be MOVED to ${getBackupDir()}`)); + // The awkward part, said plainly: moving does not free space. Without this, + // the final report ("0 B freed") would look like a bug. + console.log( + chalk.yellow( + ` ⚠ Moving does NOT free disk space. Run "mac-cleaner-cli backup --clean" to reclaim it.` + ) + ); + if (categoriesWithoutBackup.length > 0) { + console.log( + chalk.red( + ` ⚠ No backup possible for: ${categoriesWithoutBackup.join(', ')} (external tool does the cleanup) — these WILL be deleted.` + ) + ); + } + } + console.log(); const proceed = await confirm({ @@ -109,11 +142,21 @@ export async function interactiveCommand(options: InteractiveOptions = {}): Prom } // Step 6: Clean + // The backup directory is created ONCE per run, and only if some scanner in + // this run can actually back up — otherwise we would leave empty timestamped + // folders behind. + const backupDir = + backupEnabled && selectedItems.some(({ categoryId }) => getScanner(categoryId)?.supportsBackup !== false) + ? await ensureBackupDir() + : undefined; + const cleanProgress = showProgress ? createCleanProgress(selectedItems.length) : null; const cleanResults: CleanSummary = { results: [], totalFreedSpace: 0, + totalBackedUpSize: 0, + backupDir, totalCleanedItems: 0, totalErrors: 0, }; @@ -123,9 +166,11 @@ export async function interactiveCommand(options: InteractiveOptions = {}): Prom const scanner = getScanner(categoryId); cleanProgress?.update(cleanedCount, `Cleaning ${scanner.category.name}...`); - const result = await scanner.clean(items); + const scannerBackupDir = scanner.supportsBackup === false ? undefined : backupDir; + const result = await scanner.clean(items, false, scannerBackupDir); cleanResults.results.push(result); cleanResults.totalFreedSpace += result.freedSpace; + cleanResults.totalBackedUpSize = (cleanResults.totalBackedUpSize ?? 0) + (result.backedUpSize ?? 0); cleanResults.totalCleanedItems += result.cleanedItems; cleanResults.totalErrors += result.errors.length; cleanedCount++; @@ -192,8 +237,11 @@ function printCleanResults(summary: CleanSummary): void { for (const result of summary.results) { if (result.cleanedItems > 0) { + const backedUp = (result.backedUpSize ?? 0) > 0; console.log( - ` ${result.category.name.padEnd(30)} ${chalk.green('✓')} ${formatSize(result.freedSpace)} freed` + backedUp + ? ` ${result.category.name.padEnd(30)} ${chalk.cyan('↦')} ${formatSize(result.backedUpSize ?? 0)} moved to backup` + : ` ${result.category.name.padEnd(30)} ${chalk.green('✓')} ${formatSize(result.freedSpace)} freed` ); } for (const error of result.errors) { @@ -206,6 +254,16 @@ function printCleanResults(summary: CleanSummary): void { console.log(chalk.bold(`🎉 Freed ${chalk.green(formatSize(summary.totalFreedSpace))} of disk space!`)); console.log(chalk.dim(` Cleaned ${summary.totalCleanedItems} items`)); + if ((summary.totalBackedUpSize ?? 0) > 0) { + console.log(); + console.log( + chalk.cyan(` ${formatSize(summary.totalBackedUpSize ?? 0)} moved to backup (still on disk):`) + ); + console.log(chalk.dim(` ${summary.backupDir}`)); + console.log(chalk.dim(` restore: mac-cleaner-cli backup --restore "${summary.backupDir}"`)); + console.log(chalk.dim(` reclaim: mac-cleaner-cli backup --clean`)); + } + if (summary.totalErrors > 0) { console.log(chalk.red(` Errors: ${summary.totalErrors}`)); } diff --git a/src/index.ts b/src/index.ts index bd03dff..c0d2d31 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,7 +3,7 @@ import { Command, InvalidArgumentError } from 'commander'; import { ExitPromptError } from '@inquirer/core'; import { cleanCommand, interactiveCommand, listCategories, maintenanceCommand, scanCommand, uninstallCommand } from './commands/index.js'; -import { initConfig, configExists, listBackups, cleanOldBackups, loadConfig, formatSize } from './utils/index.js'; +import { initConfig, configExists, listBackups, cleanOldBackups, restoreBackup, loadConfig, formatSize } from './utils/index.js'; import { CATEGORIES, type CategoryId } from './types.js'; import pkg from '../package.json' with { type: 'json' }; @@ -183,12 +183,13 @@ program .command('backup') .description('Manage backups') .option('--list', 'List all backups') - .option('--clean', 'Clean old backups (older than 7 days)') + .option('--restore ', 'Restore a backup back to its original locations') + .option('--clean', 'Delete old backups permanently (older than 7 days), reclaiming disk space') .action(async (options) => { if (options.list) { const backups = await listBackups(); if (backups.length === 0) { - console.log('No backups found.'); + console.log('No backups found. Backups only happen when "backupEnabled": true is set in ~/.maccleanerrc'); return; } console.log('\nBackups:'); @@ -196,6 +197,20 @@ program console.log(` ${backup.date.toLocaleDateString()} - ${formatSize(backup.size)}`); console.log(` ${backup.path}`); } + console.log('\nRestore with: mac-cleaner-cli backup --restore '); + return; + } + + if (options.restore) { + const result = await restoreBackup(options.restore); + console.log(`Restored ${result.success} items.`); + if (result.failed > 0) { + console.log(`Failed: ${result.failed}`); + for (const error of result.errors) { + console.log(` ✗ ${error}`); + } + process.exitCode = 1; + } return; } @@ -205,7 +220,7 @@ program return; } - console.log('Use --list to show backups or --clean to remove old ones.'); + console.log('Use --list to show backups, --restore to bring one back, or --clean to remove old ones.'); }); program.parse(); diff --git a/src/scanners/base-scanner.test.ts b/src/scanners/base-scanner.test.ts index 0a9070b..1ae02c6 100644 --- a/src/scanners/base-scanner.test.ts +++ b/src/scanners/base-scanner.test.ts @@ -1,9 +1,12 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtemp, writeFile, rm } from 'fs/promises'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { mkdtemp, writeFile, rm, mkdir } from 'fs/promises'; +import { existsSync } from 'fs'; import { join } from 'path'; -import { tmpdir } from 'os'; +import { tmpdir, homedir } from 'os'; import { BaseScanner } from './base-scanner.js'; +import { CATEGORIES } from '../types.js'; import type { Category, ScanResult, ScannerOptions, CleanableItem } from '../types.js'; +import { ensureBackupDir } from '../utils/backup.js'; class TestScanner extends BaseScanner { category: Category = { @@ -117,3 +120,81 @@ describe('BaseScanner', () => { }); }); + +describe('BaseScanner backup routing', () => { + class FakeScanner extends BaseScanner { + category = CATEGORIES['trash']; + async scan() { + return this.createResult([]); + } + } + + let sourceDir: string; + + beforeEach(async () => { + sourceDir = join(homedir(), '.mac-cleaner-cli-test-scanner'); + await mkdir(sourceDir, { recursive: true }); + }); + + afterEach(async () => { + await rm(sourceDir, { recursive: true, force: true }); + }); + + it('deletes when no backupDir is given', async () => { + const file = join(sourceDir, 'delete-me.txt'); + await writeFile(file, 'x'); + + const scanner = new FakeScanner(); + const result = await scanner.clean( + [{ path: file, size: 1, name: 'delete-me.txt', isDirectory: false }] + ); + + expect(existsSync(file)).toBe(false); + expect(result.freedSpace).toBe(1); + expect(result.backedUpSize).toBeUndefined(); + }); + + it('moves to backup and reports ZERO freed space when backupDir is given', async () => { + const file = join(sourceDir, 'keep-me.txt'); + await writeFile(file, 'x'); + + const backupDir = await ensureBackupDir(); + const scanner = new FakeScanner(); + const result = await scanner.clean( + [{ path: file, size: 1, name: 'keep-me.txt', isDirectory: false }], + false, + backupDir + ); + + // The point of this test: moving does NOT free space. Reporting freedSpace + // here would trade the old facade for a new untruth. + expect(result.freedSpace).toBe(0); + expect(result.backedUpSize).toBe(1); + expect(result.backupDir).toBe(backupDir); + expect(existsSync(file)).toBe(false); + + await rm(backupDir, { recursive: true, force: true }); + }); + + it('reports an error and keeps the file when the backup fails', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const outside = join(tmpdir(), 'scanner-outside-home.txt'); + await writeFile(outside, 'x'); + + const backupDir = await ensureBackupDir(); + const scanner = new FakeScanner(); + const result = await scanner.clean( + [{ path: outside, size: 1, name: 'scanner-outside-home.txt', isDirectory: false }], + false, + backupDir + ); + + expect(result.cleanedItems).toBe(0); + expect(result.errors[0]).toContain('nothing was deleted'); + expect(existsSync(outside)).toBe(true); + + await rm(outside, { force: true }); + await rm(backupDir, { recursive: true, force: true }); + consoleSpy.mockRestore(); + }); +}); diff --git a/src/scanners/base-scanner.ts b/src/scanners/base-scanner.ts index c565af3..cee92d0 100644 --- a/src/scanners/base-scanner.ts +++ b/src/scanners/base-scanner.ts @@ -1,12 +1,35 @@ import type { Scanner, Category, ScanResult, CleanResult, CleanableItem, ScannerOptions } from '../types.js'; import { removeItems } from '../utils/fs.js'; +import { backupItems } from '../utils/backup.js'; export abstract class BaseScanner implements Scanner { abstract category: Category; + /** + * Scanners that delete files can back them up first. Scanners that delegate + * cleanup to an external tool (Docker, Homebrew) override this with `false`: + * there is no file of ours to move, and promising a backup there would repeat + * the very problem this module exists to fix. + */ + readonly supportsBackup: boolean = true; + abstract scan(options?: ScannerOptions): Promise; - async clean(items: CleanableItem[], dryRun = false): Promise { + async clean(items: CleanableItem[], dryRun = false, backupDir?: string): Promise { + if (backupDir) { + const result = await backupItems(items, backupDir, dryRun); + + return { + category: this.category, + cleanedItems: result.success, + // Nothing was freed: the files are still on disk, inside the backup. + freedSpace: 0, + backedUpSize: result.backedUpSize, + backupDir, + errors: result.failed > 0 ? [`Failed to back up ${result.failed} items (nothing was deleted for those)`] : [], + }; + } + const result = await removeItems(items, dryRun); const errors: string[] = []; @@ -42,10 +65,3 @@ export abstract class BaseScanner implements Scanner { }; } } - - - - - - - diff --git a/src/scanners/docker.ts b/src/scanners/docker.ts index 6bd8c30..fc60821 100644 --- a/src/scanners/docker.ts +++ b/src/scanners/docker.ts @@ -69,6 +69,11 @@ function execCommand(command: string, args: string[]): Promise { } export class DockerScanner extends BaseScanner { + // Cleanup here is `docker system prune`: the external tool does the deleting, + // not us. There is no file to move, so backup does not apply — and pretending + // it does would be the same facade as before. + readonly supportsBackup = false; + category = CATEGORIES['docker']; private dockerPath: string | null = null; diff --git a/src/scanners/homebrew.ts b/src/scanners/homebrew.ts index 8646e6a..97db459 100644 --- a/src/scanners/homebrew.ts +++ b/src/scanners/homebrew.ts @@ -75,6 +75,11 @@ function execCommand(command: string, args: string[]): Promise { } export class HomebrewScanner extends BaseScanner { + // Cleanup here is `brew cleanup`: the external tool does the deleting, not us. + // There is no file to move, so backup does not apply — and pretending it does + // would be the same facade as before. + readonly supportsBackup = false; + category = CATEGORIES['homebrew']; private brewPath: string | null = null; diff --git a/src/types.ts b/src/types.ts index 1e01b06..6dbdfeb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -56,13 +56,19 @@ export interface ScanSummary { export interface CleanResult { category: Category; cleanedItems: number; + /** Space actually returned to the disk. Zero when items went to the backup. */ freedSpace: number; + /** Size moved into the backup. Still occupying disk until `backup --clean`. */ + backedUpSize?: number; + backupDir?: string; errors: string[]; } export interface CleanSummary { results: CleanResult[]; totalFreedSpace: number; + totalBackedUpSize?: number; + backupDir?: string; totalCleanedItems: number; totalErrors: number; } @@ -75,8 +81,10 @@ export interface ScannerOptions { export interface Scanner { category: Category; + /** false when cleanup is delegated to an external tool and cannot be backed up. */ + readonly supportsBackup?: boolean; scan(options?: ScannerOptions): Promise; - clean(items: CleanableItem[], dryRun?: boolean): Promise; + clean(items: CleanableItem[], dryRun?: boolean, backupDir?: string): Promise; } export const CATEGORIES: Record = { diff --git a/src/utils/backup.test.ts b/src/utils/backup.test.ts index 1f4f2d7..421d5af 100644 --- a/src/utils/backup.test.ts +++ b/src/utils/backup.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { mkdir, mkdtemp, rm, writeFile } from 'fs/promises'; +import { mkdir, mkdtemp, rm, writeFile, readFile } from 'fs/promises'; +import { existsSync } from 'fs'; import { join } from 'path'; -import { tmpdir } from 'os'; +import { tmpdir, homedir } from 'os'; import * as backup from './backup.js'; describe('backup utilities', () => { @@ -46,79 +47,171 @@ describe('backup utilities', () => { }); }); - describe('backupItem', () => { - it('should return false for non-existent item', async () => { + describe('backupItem / backupItems (round-trip real)', () => { + // These move REAL files and check the content afterwards. The previous + // version only asserted `expect(typeof result).toBe('boolean')`, which + // passed even with the backup failing 100% of the time — which was exactly + // the state of this module: no callers, never exercised. + let sourceDir: string; + + beforeEach(async () => { + // Must be INSIDE home: backupItem refuses anything else, because + // restoreBackup only knows how to restore what sits under the HOME/ prefix. + sourceDir = join(homedir(), '.mac-cleaner-cli-test-src'); + await mkdir(sourceDir, { recursive: true }); + }); + + afterEach(async () => { + await rm(sourceDir, { recursive: true, force: true }); + }); + + it('should MOVE the file into the backup, not copy it', async () => { + const testFile = join(sourceDir, 'moved.txt'); + await writeFile(testFile, 'conteudo original'); + const dir = await backup.ensureBackupDir(); - const result = await backup.backupItem( - { path: '/non/existent/file.txt', size: 0, name: 'file.txt', isDirectory: false }, + const ok = await backup.backupItem( + { path: testFile, size: 17, name: 'moved.txt', isDirectory: false }, dir ); - expect(result).toBe(false); + + expect(ok).toBe(true); + expect(existsSync(testFile)).toBe(false); + + const backedUpPath = backup.backupPathFor(testFile, dir); + expect(backedUpPath).not.toBeNull(); + expect(await readFile(backedUpPath as string, 'utf-8')).toBe('conteudo original'); + await rm(dir, { recursive: true, force: true }); }); - it('should backup existing file', async () => { - const testFile = join(testBackupDir, 'test-backup.txt'); - await writeFile(testFile, 'test content'); + it('should refuse paths outside the home directory', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const outside = join(tmpdir(), 'outside-home.txt'); + await writeFile(outside, 'x'); const dir = await backup.ensureBackupDir(); - const result = await backup.backupItem( - { path: testFile, size: 12, name: 'test-backup.txt', isDirectory: false }, + const ok = await backup.backupItem( + { path: outside, size: 1, name: 'outside-home.txt', isDirectory: false }, dir ); - expect(typeof result).toBe('boolean'); + // Refuses, rather than creating a backup restoreBackup could not undo. + expect(ok).toBe(false); + expect(existsSync(outside)).toBe(true); + + await rm(outside, { force: true }); await rm(dir, { recursive: true, force: true }); + consoleSpy.mockRestore(); }); - }); - describe('backupItems', () => { - it('should handle empty items array', async () => { - const result = await backup.backupItems([]); + it('should refuse protected system paths', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const dir = await backup.ensureBackupDir(); - expect(result.success).toBe(0); - expect(result.failed).toBe(0); - await rm(result.backupDir, { recursive: true, force: true }); + const ok = await backup.backupItem( + { path: '/var/log/system.log', size: 1, name: 'system.log', isDirectory: false }, + dir + ); + + expect(ok).toBe(false); + await rm(dir, { recursive: true, force: true }); + consoleSpy.mockRestore(); }); - it('should backup multiple items', async () => { - const testFile = join(testBackupDir, 'test.txt'); - await writeFile(testFile, 'test content'); + it('should not touch the disk in dry run but still report the item', async () => { + const testFile = join(sourceDir, 'dry.txt'); + await writeFile(testFile, 'intacto'); + + const dir = await backup.ensureBackupDir(); + const result = await backup.backupItems( + [{ path: testFile, size: 7, name: 'dry.txt', isDirectory: false }], + dir, + true + ); - const result = await backup.backupItems([ - { path: testFile, size: 12, name: 'test.txt', isDirectory: false }, - ]); + expect(result.success).toBe(1); + expect(result.backedUpSize).toBe(7); + expect(await readFile(testFile, 'utf-8')).toBe('intacto'); - expect(result.backupDir).toBeDefined(); - await rm(result.backupDir, { recursive: true, force: true }); + await rm(dir, { recursive: true, force: true }); }); - it('should call progress callback', async () => { - const testFile = join(testBackupDir, 'test2.txt'); - await writeFile(testFile, 'test content'); + it('should report backedUpSize separately from freed space', async () => { + const testFile = join(sourceDir, 'sized.txt'); + await writeFile(testFile, 'abcdefghij'); - const progressFn = vi.fn(); + const dir = await backup.ensureBackupDir(); + const result = await backup.backupItems( + [{ path: testFile, size: 10, name: 'sized.txt', isDirectory: false }], + dir + ); + expect(result.success).toBe(1); + expect(result.backedUpSize).toBe(10); + + await rm(dir, { recursive: true, force: true }); + }); + + it('should count failures without deleting the failed item', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const good = join(sourceDir, 'good.txt'); + await writeFile(good, 'ok'); + + const dir = await backup.ensureBackupDir(); const result = await backup.backupItems( - [{ path: testFile, size: 12, name: 'test2.txt', isDirectory: false }], + [ + { path: good, size: 2, name: 'good.txt', isDirectory: false }, + { path: join(sourceDir, 'ghost.txt'), size: 0, name: 'ghost.txt', isDirectory: false }, + ], + dir + ); + + expect(result.success).toBe(1); + expect(result.failed).toBe(1); + + await rm(dir, { recursive: true, force: true }); + consoleSpy.mockRestore(); + }); + + it('should call the progress callback', async () => { + const testFile = join(sourceDir, 'progress.txt'); + await writeFile(testFile, 'p'); + const progressFn = vi.fn(); + + const dir = await backup.ensureBackupDir(); + await backup.backupItems( + [{ path: testFile, size: 1, name: 'progress.txt', isDirectory: false }], + dir, + false, progressFn ); expect(progressFn).toHaveBeenCalled(); - await rm(result.backupDir, { recursive: true, force: true }); + await rm(dir, { recursive: true, force: true }); }); - it('should count successes and failures', async () => { - const testFile = join(testBackupDir, 'success.txt'); - await writeFile(testFile, 'test'); + it('should survive a full backup -> restore round trip with identical content', async () => { + const testFile = join(sourceDir, 'roundtrip.txt'); + const content = 'este conteudo precisa voltar identico'; + await writeFile(testFile, content); + + const dir = await backup.ensureBackupDir(); + expect( + await backup.backupItem( + { path: testFile, size: content.length, name: 'roundtrip.txt', isDirectory: false }, + dir + ) + ).toBe(true); + expect(existsSync(testFile)).toBe(false); + + const restored = await backup.restoreBackup(dir); - const result = await backup.backupItems([ - { path: testFile, size: 4, name: 'success.txt', isDirectory: false }, - { path: '/non/existent.txt', size: 0, name: 'fail.txt', isDirectory: false }, - ]); + expect(restored.failed).toBe(0); + expect(restored.success).toBe(1); + expect(await readFile(testFile, 'utf-8')).toBe(content); - expect(result.success + result.failed).toBe(2); - await rm(result.backupDir, { recursive: true, force: true }); + await rm(dir, { recursive: true, force: true }); }); }); @@ -187,3 +280,20 @@ describe('backup utilities', () => { }); }); }); + +describe('ensureBackupDir uniqueness', () => { + // Regression guard: the name was just the ISO timestamp (millisecond + // resolution) and `mkdir` with `recursive: true` accepts an existing directory + // silently. Two sessions in the same millisecond would share the folder and + // one would overwrite the other. This surfaced as suite flakiness (vitest runs + // files in parallel) before it could surface as a production bug. + it('never hands out the same directory twice', async () => { + const dirs = await Promise.all( + Array.from({ length: 25 }, () => backup.ensureBackupDir()) + ); + + expect(new Set(dirs).size).toBe(dirs.length); + + await Promise.all(dirs.map((d) => rm(d, { recursive: true, force: true }))); + }); +}); diff --git a/src/utils/backup.ts b/src/utils/backup.ts index 8b879ae..5c3ad7e 100644 --- a/src/utils/backup.ts +++ b/src/utils/backup.ts @@ -1,7 +1,8 @@ -import { mkdir, rename, readdir, stat, rm } from 'fs/promises'; +import { mkdir, rename, readdir, stat, rm, cp } from 'fs/promises'; import { join, dirname, resolve, relative } from 'path'; import { homedir } from 'os'; import type { CleanableItem } from '../types.js'; +import { validatePathSafety } from './fs.js'; const BACKUP_DIR = join(homedir(), '.mac-cleaner-cli', 'backup'); const BACKUP_RETENTION_DAYS = 7; @@ -13,60 +14,161 @@ const BACKUP_RETENTION_DAYS = 7; function validateRestorePath(targetPath: string): string | null { const home = homedir(); const resolved = resolve(targetPath); - + // Ensure the resolved path is within the home directory if (!resolved.startsWith(home + '/') && resolved !== home) { return `Path traversal detected: ${targetPath} resolves outside home directory`; } - + // Check for suspicious patterns that might indicate an attack if (targetPath.includes('..')) { return `Suspicious path pattern detected: ${targetPath}`; } - + return null; } +/** + * Creates this session's backup directory. + * + * The name is an ISO timestamp, which has millisecond resolution — two sessions + * started in the same millisecond would land in the SAME directory and one could + * overwrite the other. `mkdir` with `recursive: true` does not report that: it + * accepts an existing directory silently. Hence the incremental suffix and the + * non-recursive `mkdir` on the last level, which is what makes creation + * exclusive. + */ export async function ensureBackupDir(): Promise { const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); - const sessionDir = join(BACKUP_DIR, timestamp); - await mkdir(sessionDir, { recursive: true }); - return sessionDir; + + await mkdir(BACKUP_DIR, { recursive: true }); + + for (let attempt = 0; ; attempt++) { + const sessionDir = join(BACKUP_DIR, attempt === 0 ? timestamp : `${timestamp}-${attempt}`); + try { + await mkdir(sessionDir); + return sessionDir; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + } + } +} + +/** + * Maps an original path to its path inside the backup. + * + * Only paths INSIDE the home directory are accepted, for two reasons: + * + * 1. `restoreBackup()` only knows how to restore what sits under the `HOME/` + * prefix — accepting anything else would create a backup that cannot be + * restored. + * 2. The previous implementation used `item.path.replace(homedir(), 'HOME')`, + * which replaces the FIRST occurrence anywhere in the string and, for a path + * outside home, returned an absolute path — making `join(backupDir, '/x')` + * land outside the backup directory entirely. + * + * Returns null when the path cannot be backed up safely. + */ +export function backupPathFor(originalPath: string, backupDir: string): string | null { + const home = homedir(); + const resolved = resolve(originalPath); + + if (!resolved.startsWith(home + '/')) { + return null; + } + + const relativeToHome = relative(home, resolved); + if (!relativeToHome || relativeToHome.startsWith('..')) { + return null; + } + + return join(backupDir, 'HOME', relativeToHome); } +/** + * Moves an item into the backup directory instead of deleting it. + * + * `rename` first (instant, same volume) with a copy-then-remove fallback, + * because `rename` fails with EXDEV when the item lives on another volume — a + * real case for `/Volumes/*` and some caches. + * + * Safety contract: if the backup fails, the item is NOT deleted. Callers count + * that as a failure, never as a silent success. + */ export async function backupItem(item: CleanableItem, backupDir: string): Promise { + const safetyError = validatePathSafety(item.path); + if (safetyError) { + console.error(safetyError); + return false; + } + + const backupPath = backupPathFor(item.path, backupDir); + if (!backupPath) { + console.error(`Cannot back up path outside home directory: ${item.path}`); + return false; + } + try { - const relativePath = item.path.replace(homedir(), 'HOME'); - const backupPath = join(backupDir, relativePath); await mkdir(dirname(backupPath), { recursive: true }); await rename(item.path, backupPath); return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EXDEV') { + return false; + } + } + + // Different volume: copy preserving symlinks (never following the target), + // and only then remove the original. + try { + await cp(item.path, backupPath, { + recursive: true, + verbatimSymlinks: true, + errorOnExist: false, + force: true, + }); + await rm(item.path, { recursive: true, force: true }); + return true; } catch { return false; } } +/** + * Moves every item into `backupDir` instead of deleting them. + * + * `backedUpSize` is reported separately from "freed space" on purpose: moving a + * file to another folder on the SAME disk does not free a single byte. The space + * only appears after `mac-cleaner-cli backup --clean`. Calling it "freed" would + * trade one untruth (a backup that never happened) for another. + */ export async function backupItems( items: CleanableItem[], + backupDir: string, + dryRun = false, onProgress?: (current: number, total: number, item: CleanableItem) => void -): Promise<{ backupDir: string; success: number; failed: number }> { - const backupDir = await ensureBackupDir(); +): Promise<{ success: number; failed: number; backedUpSize: number }> { let success = 0; let failed = 0; + let backedUpSize = 0; for (let i = 0; i < items.length; i++) { const item = items[i]; onProgress?.(i + 1, items.length, item); - const backed = await backupItem(item, backupDir); + const backed = dryRun + ? backupPathFor(item.path, backupDir) !== null + : await backupItem(item, backupDir); + if (backed) { success++; + backedUpSize += item.size; } else { failed++; } } - return { backupDir, success, failed }; + return { success, failed, backedUpSize }; } export async function cleanOldBackups(): Promise { @@ -157,13 +259,18 @@ export async function restoreBackup(backupDir: string): Promise<{ success: numbe const errors: string[] = []; const home = homedir(); - // Validate that backupDir is within our expected backup location + // Validate that backupDir is within our expected backup location. + // The `+ '/'` matters: without it, `~/.mac-cleaner-cli/backup-anything` + // passed the check simply by being a textual prefix. const resolvedBackupDir = resolve(backupDir); - if (!resolvedBackupDir.startsWith(BACKUP_DIR)) { - return { - success: 0, - failed: 1, - errors: ['Invalid backup directory: must be within the mac-cleaner-cli backup folder'] + if ( + resolvedBackupDir !== BACKUP_DIR && + !resolvedBackupDir.startsWith(BACKUP_DIR + '/') + ) { + return { + success: 0, + failed: 1, + errors: ['Invalid backup directory: must be within the mac-cleaner-cli backup folder'] }; } @@ -186,7 +293,7 @@ export async function restoreBackup(backupDir: string): Promise<{ success: numbe } else { // Compute the relative path from backup directory const relFromBackup = relative(resolvedBackupDir, entryPath); - + // Replace HOME prefix with actual home directory // Use a more secure replacement that only matches at the start let targetPath: string; @@ -201,7 +308,7 @@ export async function restoreBackup(backupDir: string): Promise<{ success: numbe failed++; continue; } - + // Validate the target path to prevent path traversal attacks const validationError = validateRestorePath(targetPath); if (validationError) { @@ -230,4 +337,3 @@ export async function restoreBackup(backupDir: string): Promise<{ success: numbe export function getBackupDir(): string { return BACKUP_DIR; } - diff --git a/vitest.config.ts b/vitest.config.ts index 5993b82..01ab2d5 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -16,7 +16,9 @@ export default defineConfig({ '**/index.ts', 'src/scanners/duplicates.ts', 'src/scanners/node-modules.ts', - 'src/utils/backup.ts', + // src/utils/backup.ts left this list: it is no longer dead code and now + // runs on the deletion path. A module that can lose a user's file does + // not sit outside the coverage count. 'src/utils/checkbox.ts', ], thresholds: {