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: {