From 3a241cf09f60c106199206bdf8fa79e450bb6316 Mon Sep 17 00:00:00 2001 From: paisley <8197966+su8su@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:30:12 +0800 Subject: [PATCH] fix: prevent plugin cleanup from deleting bundled OpenClaw runtime (#1197) --- .github/workflows/check.yml | 2 +- electron/gateway/config-sync.ts | 9 +- electron/utils/plugin-install.ts | 11 +- electron/utils/safe-fs.ts | 128 ++++++++++++++++++ .../specs/tasks/fix-windows-plugin-cleanup.md | 35 +++++ tests/unit/safe-fs.test.ts | 82 +++++++++++ 6 files changed, 257 insertions(+), 10 deletions(-) create mode 100644 electron/utils/safe-fs.ts create mode 100644 harness/specs/tasks/fix-windows-plugin-cleanup.md create mode 100644 tests/unit/safe-fs.test.ts diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 2066cff9..99d90f24 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -118,7 +118,7 @@ jobs: run: pnpm install --frozen-lockfile - name: Test Windows attachment open-with bridge - run: pnpm exec vitest run tests/unit/attachment-open-with.test.ts tests/unit/attachment-open-with-native.test.ts + run: pnpm exec vitest run tests/unit/attachment-open-with.test.ts tests/unit/attachment-open-with-native.test.ts tests/unit/safe-fs.test.ts - name: Generate extension bridge run: pnpm run ext:bridge diff --git a/electron/gateway/config-sync.ts b/electron/gateway/config-sync.ts index 7860fb46..d52f42fb 100644 --- a/electron/gateway/config-sync.ts +++ b/electron/gateway/config-sync.ts @@ -1,6 +1,6 @@ import { app } from 'electron'; import path from 'path'; -import { existsSync, readFileSync, mkdirSync, readdirSync, rmSync, symlinkSync } from 'fs'; +import { existsSync, readFileSync, mkdirSync, readdirSync, symlinkSync } from 'fs'; import { homedir } from 'os'; import { join } from 'path'; @@ -34,6 +34,7 @@ import { syncProxyConfigToOpenClaw } from '../utils/openclaw-proxy'; import { logger } from '../utils/logger'; import { prependPathEntry } from '../utils/env-path'; import { copyPluginFromNodeModules, fixupPluginManifest, cpSyncSafe, buildCandidateSources, repairTrustedOfficialPluginInstallRecords, removeTrustedOfficialPluginInstallRecord, syncTrustedOfficialPluginInstallRecord, resolvePluginNpmPackagePath } from '../utils/plugin-install'; +import { safeRmSync } from '../utils/safe-fs'; import { CLAWX_OPENAI_IMAGE_PROVIDER_KEY } from '../utils/openclaw-image-relay-constants'; import { ensureOpenClaw2026_7_1UpgradeSnapshot } from '../utils/openclaw-upgrade-snapshot'; import { stripSystemdSupervisorEnv } from './config-sync-env'; @@ -120,7 +121,7 @@ function cleanupStaleBuiltInExtensions(): void { if (existsSync(fsPath(extDir))) { logger.info(`[plugin] Removing stale built-in extension copy: ${ext}`); try { - rmSync(fsPath(extDir), { recursive: true, force: true }); + safeRmSync(fsPath(extDir)); } catch (err) { logger.warn(`[plugin] Failed to remove stale extension ${ext}:`, err); } @@ -192,7 +193,7 @@ function ensureConfiguredPluginsUpgraded(configuredChannels: string[]): boolean logger.info(`[plugin] ${isInstalled ? 'Auto-upgrading' : 'Installing'} ${channelType} plugin${isInstalled ? `: ${installedVersion} → ${sourceVersion}` : `: ${sourceVersion}`} (bundled)`); try { mkdirSync(fsPath(join(homedir(), '.openclaw', 'extensions')), { recursive: true }); - rmSync(fsPath(targetDir), { recursive: true, force: true }); + safeRmSync(fsPath(targetDir)); cpSyncSafe(bundledDir, targetDir); fixupPluginManifest(targetDir); syncTrustedOfficialPluginInstallRecord(dirName, targetDir); @@ -258,7 +259,7 @@ function cleanupUnconfiguredChannelPlugins(configuredChannels: string[]): boolea logger.info(`[plugin] Removing unconfigured channel plugin: ${channelType} (${dirName})`); try { - rmSync(fsPath(targetDir), { recursive: true, force: true }); + safeRmSync(fsPath(targetDir)); } catch (err) { logger.warn(`[plugin] Failed to remove unconfigured channel plugin ${channelType}:`, err); succeeded = false; diff --git a/electron/utils/plugin-install.ts b/electron/utils/plugin-install.ts index 704f1fe4..49ad3f11 100644 --- a/electron/utils/plugin-install.ts +++ b/electron/utils/plugin-install.ts @@ -7,12 +7,13 @@ */ import { app } from 'electron'; import path from 'node:path'; -import { existsSync, cpSync, copyFileSync, statSync, lstatSync, mkdirSync, rmSync, readFileSync, writeFileSync, readdirSync, realpathSync, symlinkSync, unlinkSync } from 'node:fs'; +import { existsSync, cpSync, copyFileSync, statSync, lstatSync, mkdirSync, readFileSync, writeFileSync, readdirSync, realpathSync, symlinkSync, unlinkSync } from 'node:fs'; import { readdir, stat, copyFile, mkdir } from 'node:fs/promises'; import { homedir } from 'node:os'; import { join } from 'node:path'; import { logger } from './logger'; import { getOpenClawResolvedDir } from './paths'; +import { safeRmSync } from './safe-fs'; import { upsertPluginInstallRecordsIntoSqlite, removePluginInstallRecordsFromSqlite, @@ -491,7 +492,7 @@ export function repairPluginOpenClawPeerLink( logger.warn(`[plugin] Cannot replace non-OpenClaw peer directory at ${linkPath}`); return false; } - rmSync(fsPath(linkPath), { recursive: true, force: true }); + safeRmSync(fsPath(linkPath)); } else { logger.warn(`[plugin] Cannot replace non-directory OpenClaw peer at ${linkPath}`); return false; @@ -664,7 +665,7 @@ export function copyPluginFromNodeModules(npmPkgPath: string, targetDir: string, } // 1. Copy plugin package itself - rmSync(fsPath(targetDir), { recursive: true, force: true }); + safeRmSync(fsPath(targetDir)); mkdirSync(fsPath(targetDir), { recursive: true }); cpSyncSafe(realPath, targetDir); @@ -764,7 +765,7 @@ export function ensurePluginInstalled( for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { mkdirSync(fsPath(extensionsRoot), { recursive: true }); - rmSync(fsPath(targetDir), { recursive: true, force: true }); + safeRmSync(fsPath(targetDir)); cpSyncSafe(sourceDir, targetDir); if (!existsSync(fsPath(join(targetDir, 'openclaw.plugin.json')))) { return { installed: false, warning: `Failed to install ${pluginLabel} plugin mirror (manifest missing).` }; @@ -778,7 +779,7 @@ export function ensurePluginInstalled( attempts.push({ attempt, ...diagnostic }); if (attempt < maxAttempts) { try { - rmSync(fsPath(targetDir), { recursive: true, force: true }); + safeRmSync(fsPath(targetDir)); } catch { // Ignore cleanup failures before retry. } diff --git a/electron/utils/safe-fs.ts b/electron/utils/safe-fs.ts new file mode 100644 index 00000000..0b8711ee --- /dev/null +++ b/electron/utils/safe-fs.ts @@ -0,0 +1,128 @@ +import { dirname, join } from 'node:path'; +import { lstatSync, readdirSync, realpathSync, rmdirSync, unlinkSync } from 'node:fs'; + +function normalizeComparablePath(input: string): string { + if (process.platform === 'win32') { + return input.replace(/\\/g, '/').toLowerCase(); + } + return input; +} + +function isPathInside(root: string, candidate: string): boolean { + const normalizedRoot = normalizeComparablePath(root); + const normalizedCandidate = normalizeComparablePath(candidate); + const rootWithSep = normalizedRoot.endsWith('/') ? normalizedRoot : `${normalizedRoot}/`; + return normalizedCandidate === normalizedRoot || normalizedCandidate.startsWith(rootWithSep); +} + +function errnoCode(error: unknown): string | undefined { + return error && typeof error === 'object' + ? (error as NodeJS.ErrnoException).code + : undefined; +} + +function resolveRealPath(input: string): string { + // Node's JavaScript realpath implementation can split a Windows namespaced + // path (\\?\C:\...) at the drive colon and try to lstat "C:". The native + // implementation accepts the same long-path form without reparsing it. + return realpathSync.native(input); +} + +function removeLinkEntry(entryPath: string): void { + // Never recursively remove a link. In particular, an NTFS junction may point + // at the bundled OpenClaw runtime outside the plugin tree. + try { + unlinkSync(entryPath); + } catch (error) { + const code = errnoCode(error); + if (code === 'ENOENT') return; + // libuv normally unlinks Windows junctions directly. Some Windows filesystems + // report directory links as EPERM/EISDIR, where a non-recursive rmdir removes + // the junction node without traversing its target. + if (process.platform === 'win32' && (code === 'EPERM' || code === 'EISDIR')) { + rmdirSync(entryPath); + return; + } + throw error; + } +} + +function removeFileEntry(entryPath: string): void { + try { + unlinkSync(entryPath); + } catch (error) { + if (errnoCode(error) !== 'ENOENT') throw error; + } +} + +function removeDirectoryEntry(entryPath: string, deletionRootRealPath: string): void { + let stat; + try { + stat = lstatSync(entryPath); + } catch (error) { + if (errnoCode(error) === 'ENOENT') return; + throw error; + } + + if (stat.isSymbolicLink()) { + removeLinkEntry(entryPath); + return; + } + + if (stat.isDirectory()) { + // Resolve before descending. If resolution fails, propagate the error rather + // than falling back to fs.rmSync(), which could follow an outbound junction. + const entryRealPath = resolveRealPath(entryPath); + if (!isPathInside(deletionRootRealPath, entryRealPath)) { + throw new Error(`Refusing to recursively delete directory outside root: ${entryPath} -> ${entryRealPath}`); + } + + for (const child of readdirSync(entryPath)) { + removeDirectoryEntry(join(entryPath, child), deletionRootRealPath); + } + rmdirSync(entryPath); + return; + } + + removeFileEntry(entryPath); +} + +/** + * Remove a file or directory tree without following outbound directory + * junctions/symlinks on Windows. Plain fs.rmSync({ recursive: true }) can + * traverse NTFS junctions (for example plugin node_modules/openclaw peers) + * and delete link targets outside the requested tree. + */ +export function safeRmSync(targetPath: string): void { + let stat; + try { + stat = lstatSync(targetPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return; + throw error; + } + + if (stat.isSymbolicLink()) { + removeLinkEntry(targetPath); + return; + } + + if (!stat.isDirectory()) { + removeFileEntry(targetPath); + return; + } + + // Fail closed when either path cannot be resolved. Falling back to recursive + // rm here would reintroduce the junction traversal this helper prevents. + const parentRealPath = resolveRealPath(dirname(targetPath)); + const deletionRootRealPath = resolveRealPath(targetPath); + if (!isPathInside(parentRealPath, deletionRootRealPath)) { + throw new Error(`Refusing to recursively delete directory outside parent: ${targetPath} -> ${deletionRootRealPath}`); + } + + for (const child of readdirSync(targetPath)) { + removeDirectoryEntry(join(targetPath, child), deletionRootRealPath); + } + + rmdirSync(targetPath); +} diff --git a/harness/specs/tasks/fix-windows-plugin-cleanup.md b/harness/specs/tasks/fix-windows-plugin-cleanup.md new file mode 100644 index 00000000..7832ec03 --- /dev/null +++ b/harness/specs/tasks/fix-windows-plugin-cleanup.md @@ -0,0 +1,35 @@ +--- +id: fix-windows-plugin-cleanup +title: Fix Windows channel plugin cleanup +scenario: plugin-lifecycle-management +taskType: plugin-lifecycle +intent: Remove unconfigured channel plugins safely when their Windows paths use the namespaced path prefix. +touchedAreas: + - .github/workflows/check.yml + - electron/gateway/config-sync.ts + - electron/utils/plugin-install.ts + - electron/utils/safe-fs.ts + - tests/unit/safe-fs.test.ts + - harness/specs/tasks/fix-windows-plugin-cleanup.md +expectedUserBehavior: + - Removing a configured channel also removes its stale plugin directory on Windows. + - Plugin cleanup never follows outbound directory links into the bundled OpenClaw runtime. +requiredProfiles: + - fast +requiredTests: + - tests/unit/safe-fs.test.ts +acceptance: + - Safe recursive removal accepts Windows namespaced paths such as `\\?\C:\Users\...\extensions\wecom`. + - Real-path validation does not reduce a namespaced drive path to `C:`. + - Outbound symlink and junction targets remain untouched. +references: + - harness/specs/scenarios/gateway-backend-communication.md + - harness/specs/scenarios/plugin-lifecycle-management.md +docs: + required: false +--- + +This task covers the Windows cleanup path used during Gateway configuration +synchronization after a channel is removed. The deletion guard must retain its +junction-safety checks while resolving namespaced paths through the native +Windows real-path implementation. diff --git a/tests/unit/safe-fs.test.ts b/tests/unit/safe-fs.test.ts new file mode 100644 index 00000000..3b418bf0 --- /dev/null +++ b/tests/unit/safe-fs.test.ts @@ -0,0 +1,82 @@ +import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, toNamespacedPath } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { existsSync } from 'node:fs'; +import { safeRmSync } from '@electron/utils/safe-fs'; + +const SYMLINK_TYPE: 'dir' | 'junction' = process.platform === 'win32' ? 'junction' : 'dir'; + +describe('safeRmSync', () => { + let root: string; + + afterEach(() => { + if (root && existsSync(root)) { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('removes a directory tree without deleting outbound symlink/junction targets', () => { + root = mkdtempSync(join(tmpdir(), 'clawx-safe-rm-')); + const bundledRuntime = join(root, 'bundled-openclaw'); + const pluginDir = join(root, 'extensions', 'openclaw-weixin'); + const peerLink = join(pluginDir, 'node_modules', 'openclaw'); + + mkdirSync(bundledRuntime, { recursive: true }); + writeFileSync(join(bundledRuntime, 'openclaw.mjs'), 'export {}'); + writeFileSync(join(bundledRuntime, 'package.json'), '{"name":"openclaw"}'); + + mkdirSync(join(pluginDir, 'node_modules'), { recursive: true }); + writeFileSync(join(pluginDir, 'openclaw.plugin.json'), '{"id":"openclaw-weixin"}'); + symlinkSync(bundledRuntime, peerLink, SYMLINK_TYPE); + + safeRmSync(pluginDir); + + expect(existsSync(pluginDir)).toBe(false); + expect(existsSync(join(bundledRuntime, 'openclaw.mjs'))).toBe(true); + expect(existsSync(join(bundledRuntime, 'package.json'))).toBe(true); + }); + + it('removes a top-level outbound directory link without deleting its target', () => { + root = mkdtempSync(join(tmpdir(), 'clawx-safe-rm-link-')); + const target = join(root, 'runtime'); + const link = join(root, 'plugin-link'); + + mkdirSync(target, { recursive: true }); + writeFileSync(join(target, 'marker.txt'), 'keep'); + symlinkSync(target, link, SYMLINK_TYPE); + + safeRmSync(link); + + expect(existsSync(link)).toBe(false); + expect(existsSync(join(target, 'marker.txt'))).toBe(true); + }); + + it('is a no-op when the path is already missing', () => { + root = mkdtempSync(join(tmpdir(), 'clawx-safe-rm-missing-')); + const missing = join(root, 'does-not-exist'); + + expect(() => safeRmSync(missing)).not.toThrow(); + }); + + it.runIf(process.platform === 'win32')('removes a directory tree through a Windows namespaced path', () => { + root = mkdtempSync(join(tmpdir(), 'clawx-safe-rm-namespaced-')); + const pluginDir = join(root, 'extensions', 'wecom'); + const namespacedPluginDir = toNamespacedPath(pluginDir); + + mkdirSync(pluginDir, { recursive: true }); + writeFileSync(join(pluginDir, 'openclaw.plugin.json'), '{"id":"wecom"}'); + + expect(namespacedPluginDir).toMatch(/^\\\\\?\\/); + expect(() => safeRmSync(namespacedPluginDir)).not.toThrow(); + expect(existsSync(pluginDir)).toBe(false); + }); + + it('runs the junction regression test in the Windows CI job', () => { + const projectRoot = join(import.meta.dirname, '..', '..'); + const workflow = readFileSync(join(projectRoot, '.github', 'workflows', 'check.yml'), 'utf8'); + const windowsJob = workflow.slice(workflow.indexOf(' build:')); + + expect(windowsJob).toContain('tests/unit/safe-fs.test.ts'); + }); +});