fix(extensions): harden GitHub extension installs

electron/main/extension-path-guard.test.ts

@@ -0,0 +1,52 @@
+import assert from 'node:assert/strict'
+import path from 'node:path'
+import test from 'node:test'
+
+async function loadGuard() {
+  return import(new URL('./extension-path-guard.ts', import.meta.url).href)
+}
+
+test('assertSafeExtensionId accepts conservative extension ids', async () => {
+  const { assertSafeExtensionId } = await loadGuard()
+  assert.equal(assertSafeExtensionId('mesh-process'), 'mesh-process')
+  assert.equal(assertSafeExtensionId('image_model.v2'), 'image_model.v2')
+  assert.equal(assertSafeExtensionId('kimodo-soma-rp'), 'kimodo-soma-rp')
+})
+
+test('assertSafeExtensionId rejects empty, traversal and absolute ids', async () => {
+  const { assertSafeExtensionId } = await loadGuard()
+  assert.throws(() => assertSafeExtensionId(''), /must not be empty/i)
+  assert.throws(() => assertSafeExtensionId('.'), /is invalid/i)
+  assert.throws(() => assertSafeExtensionId('..'), /is invalid/i)
+  assert.throws(() => assertSafeExtensionId('../escape'), /path separators/i)
+  assert.throws(() => assertSafeExtensionId('..\\escape'), /path separators/i)
+  assert.throws(() => assertSafeExtensionId('/abs/path'), /absolute path|path separators/i)
+})
+
+test('assertSafeExtensionId rejects uppercase and unsupported characters', async () => {
+  const { assertSafeExtensionId } = await loadGuard()
+  assert.throws(() => assertSafeExtensionId('Mesh-Process'), /must match/i)
+  assert.throws(() => assertSafeExtensionId('bad id'), /must match/i)
+  assert.throws(() => assertSafeExtensionId('bad:id'), /must match/i)
+})
+
+test('resolveExtensionPathWithinRoot confines paths to root', async () => {
+  const { resolveExtensionPathWithinRoot } = await loadGuard()
+  const root = path.join('/tmp', 'extensions-root')
+  assert.equal(resolveExtensionPathWithinRoot(root, 'mesh-process'), path.resolve(root, 'mesh-process'))
+  assert.throws(() => resolveExtensionPathWithinRoot(root, '../escape'), /path separators/i)
+})
+
+test('resolvePathWithinRoot rejects canonical escapes', async () => {
+  const { resolvePathWithinRoot } = await loadGuard()
+  const root = path.join('/tmp', 'extensions-root')
+  assert.equal(resolvePathWithinRoot(root, '.modly-backup-safe-123'), path.resolve(root, '.modly-backup-safe-123'))
+  assert.throws(() => resolvePathWithinRoot(root, '../escape'), /escapes root/i)
+})
+
+test('buildExtensionBackupPath stays within root and rejects unsafe ids', async () => {
+  const { buildExtensionBackupPath } = await loadGuard()
+  const root = path.join('/tmp', 'extensions-root')
+  assert.equal(buildExtensionBackupPath(root, 'mesh-process', '123'), path.resolve(root, '.modly-backup-mesh-process-123'))
+  assert.throws(() => buildExtensionBackupPath(root, '../escape', '123'), /path separators/i)
+})

electron/main/extension-path-guard.ts

@@ -0,0 +1,53 @@
+import { isAbsolute, relative, resolve as resolvePath } from 'node:path'
+
+const EXTENSION_ID_PATTERN = /^[a-z0-9][a-z0-9._-]*$/
+
+export function assertSafeExtensionId(extensionId: unknown): string {
+  if (typeof extensionId !== 'string') {
+    throw new Error('Extension id must be a string')
+  }
+
+  const trimmed = extensionId.trim()
+  if (!trimmed) {
+    throw new Error('Extension id must not be empty')
+  }
+
+  if (trimmed === '.' || trimmed === '..') {
+    throw new Error(`Extension id "${extensionId}" is invalid`)
+  }
+
+  if (isAbsolute(trimmed)) {
+    throw new Error(`Extension id "${extensionId}" must not be an absolute path`)
+  }
+
+  if (trimmed.includes('/') || trimmed.includes('\\')) {
+    throw new Error(`Extension id "${extensionId}" must not contain path separators`)
+  }
+
+  if (!EXTENSION_ID_PATTERN.test(trimmed)) {
+    throw new Error(`Extension id "${extensionId}" must match ${EXTENSION_ID_PATTERN}`)
+  }
+
+  return trimmed
+}
+
+export function resolvePathWithinRoot(rootDir: string, unsafeLeaf: string): string {
+  const resolvedRoot = resolvePath(rootDir)
+  const resolvedCandidate = resolvePath(resolvedRoot, unsafeLeaf)
+  const normalizedRelative = relative(resolvedRoot, resolvedCandidate).replace(/\\/g, '/')
+
+  if (normalizedRelative === '..' || normalizedRelative.startsWith('../') || isAbsolute(normalizedRelative)) {
+    throw new Error(`Resolved path escapes root: ${unsafeLeaf}`)
+  }
+
+  return resolvedCandidate
+}
+
+export function resolveExtensionPathWithinRoot(rootDir: string, extensionId: unknown): string {
+  return resolvePathWithinRoot(rootDir, assertSafeExtensionId(extensionId))
+}
+
+export function buildExtensionBackupPath(rootDir: string, extensionId: unknown, suffix: string): string {
+  const safeId = assertSafeExtensionId(extensionId)
+  return resolvePathWithinRoot(rootDir, `.modly-backup-${safeId}-${suffix}`)
+}

electron/main/ipc-handlers.ts

@@ -18,6 +18,7 @@ import { logger } from './logger'
 import { getProcessRunner, getPythonProcessRunner, getExtPythonExe, terminateProcessRunner, terminateAllProcessRunners } from './process-runner'
 import { getBuiltinExtensionsDir } from './builtin-sync'
 import { spawn } from 'child_process'
+import { assertSafeExtensionId, buildExtensionBackupPath, resolveExtensionPathWithinRoot } from './extension-path-guard'
 
 type WindowGetter = () => BrowserWindow | null
 
@@ -696,6 +697,8 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
       const manifest    = JSON.parse(manifestRaw) as ParsedManifest
 
       if (!manifest.id) throw new Error('manifest.json: required field "id" missing')
+      const extensionId = assertSafeExtensionId(manifest.id)
+      manifest.id = extensionId
       if (!manifest.nodes?.length) throw new Error('manifest.json: required field "nodes" missing or empty')
 
       const isProcess = manifest.type === 'process'
@@ -720,11 +723,13 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
       // 5. Copy to extensions directory (overwrite if already present)
       const extensionsDir = getSettings(app.getPath('userData')).extensionsDir
       await mkdir(extensionsDir, { recursive: true })
-      const destDir = join(extensionsDir, manifest.id)
+      const destDir = resolveExtensionPathWithinRoot(extensionsDir, extensionId)
+      const backupDir = existsSync(destDir) ? buildExtensionBackupPath(extensionsDir, extensionId, String(Date.now())) : null
 
-      if (existsSync(destDir)) {
-        terminateProcessRunner(manifest.id)
-        await rmAsync(destDir, { recursive: true, force: true })
+      try {
+      if (backupDir) {
+        terminateProcessRunner(extensionId)
+        await rename(destDir, backupDir)
       }
       await cp(extractDir, destDir, { recursive: true })
 
@@ -749,15 +754,10 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
         if (existsSync(join(destDir, 'setup.py'))) {
           emit({ step: 'setting_up', message: 'Setting up Python environment…' })
           const { sm: gpuSm, cudaVersion } = await detectGpuInfo()
-          try {
-            await runExtensionSetup(destDir, gpuSm, cudaVersion, (line) => {
-              logger.info(`[ext-setup] ${line}`)
-              emit({ step: 'setting_up', message: line })
-            })
-          } catch (err) {
-            logger.warn(`[ext-setup] setup.py failed: ${err}`)
-            emit({ step: 'setting_up', message: `Warning: setup failed — ${err}` })
-          }
+          await runExtensionSetup(destDir, gpuSm, cudaVersion, (line) => {
+            logger.info(`[ext-setup] ${line}`)
+            emit({ step: 'setting_up', message: line })
+          })
         }
       } else if (isProcess) {
         // 6b. JS process extension: npm install if package.json present
@@ -790,26 +790,33 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
         if (existsSync(join(destDir, 'setup.py'))) {
           emit({ step: 'setting_up', message: 'Setting up Python environment…' })
           const { sm: gpuSm, cudaVersion } = await detectGpuInfo()
-          try {
-            await runExtensionSetup(destDir, gpuSm, cudaVersion, (line) => {
-              logger.info(`[ext-setup] ${line}`)
-              emit({ step: 'setting_up', message: line })
-            })
-          } catch (setupErr: any) {
-            throw new Error(`Extension setup failed: ${setupErr?.message ?? setupErr}`)
-          }
+          await runExtensionSetup(destDir, gpuSm, cudaVersion, (line) => {
+            logger.info(`[ext-setup] ${line}`)
+            emit({ step: 'setting_up', message: line })
+          })
         }
 
         try {
           await axios.post(`${API_BASE_URL}/extensions/reload`, {}, { timeout: 10_000 })
         } catch { /* Python might not be running yet */ }
       }
 
-      emit({ step: 'done', extensionId: manifest.id })
+      if (backupDir) {
+        await rmAsync(backupDir, { recursive: true, force: true })
+      }
+      } catch (installErr) {
+        await rmAsync(destDir, { recursive: true, force: true }).catch(() => {})
+        if (backupDir && existsSync(backupDir)) {
+          await rename(backupDir, destDir)
+        }
+        throw installErr
+      }
+
+      emit({ step: 'done', extensionId })
 
       const trustedRepos = await fetchTrustedRepos()
-      const ext = parseExtensionManifest(manifest, manifest.id, trustedRepos)
-      return { success: true, extensionId: manifest.id, extension: ext }
+      const ext = parseExtensionManifest(manifest, extensionId, trustedRepos)
+      return { success: true, extensionId, extension: ext }
 
     } catch (err) {
       emit({ step: 'error', message: String(err) })
@@ -824,16 +831,17 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
   // Uninstall an extension — built-ins cannot be uninstalled
   ipcMain.handle('extensions:uninstall', async (_, extensionId: string) => {
     const userData      = app.getPath('userData')
-    const builtinPath   = join(getBuiltinExtensionsDir(), extensionId)
+    const safeExtensionId = assertSafeExtensionId(extensionId)
+    const builtinPath   = resolveExtensionPathWithinRoot(getBuiltinExtensionsDir(), safeExtensionId)
     if (existsSync(builtinPath)) {
-      return { success: false, error: `"${extensionId}" is a built-in extension and cannot be uninstalled.` }
+      return { success: false, error: `"${safeExtensionId}" is a built-in extension and cannot be uninstalled.` }
     }
 
     const extensionsDir = getSettings(userData).extensionsDir
-    const extPath       = join(extensionsDir, extensionId)
+    const extPath       = resolveExtensionPathWithinRoot(extensionsDir, safeExtensionId)
     try {
       // Terminate process runner if it's a process extension
-      terminateProcessRunner(extensionId)
+      terminateProcessRunner(safeExtensionId)
 
       await rmAsync(extPath, { recursive: true, force: true })
       // Hot-reload Python so it stops using the deleted model extension
@@ -849,7 +857,8 @@ export function setupIpcHandlers(pythonBridge: PythonBridge, getWindow: WindowGe
   // Re-run setup.py for a model extension (creates the venv if missing)
   ipcMain.handle('extensions:repair', async (_, extensionId: string) => {
     try {
-      const extDir = join(getSettings(app.getPath('userData')).extensionsDir, extensionId)
+      const safeExtensionId = assertSafeExtensionId(extensionId)
+      const extDir = resolveExtensionPathWithinRoot(getSettings(app.getPath('userData')).extensionsDir, safeExtensionId)
       if (!existsSync(join(extDir, 'setup.py'))) {
         return { success: false, error: 'No setup.py found for this extension' }
       }