fix(extensions): a failed upgrade no longer deletes the working extension (#1562)

lib/core/extensions/local_extension_installer.dart

@@ -119,56 +119,104 @@ class LocalExtensionInstaller {
 
     onProgress?.call(0.4);
 
+    final extDir = await installArchive(
+      archive: archive,
+      manifest: manifest,
+      onProgress: onProgress,
+    );
+    return ExtensionManifest.fromJson(
+      {
+        ...manifest.toJson(),
+        // installPath is not part of toJson; reload will set it.
+      },
+      installPath: extDir.path,
+    );
+  }
+
+  /// Extracts [archive] into a hidden sibling of `extensions/<id>/`, validates
+  /// it there and only then swaps it in, so a failed install or upgrade leaves
+  /// the previous version untouched (#1562). Shared by local and marketplace
+  /// installs; [manifest] is what gets written as `manifest.json`.
+  Future<Directory> installArchive({
+    required Archive archive,
+    required ExtensionManifest manifest,
+    void Function(double progress)? onProgress,
+  }) async {
     // A running driver process for this extension holds its executable/DLL
-    // open; on Windows that mandatory lock makes the overwrite below fail
-    // with Access is denied (#891). Stop every live session first.
+    // open; on Windows that mandatory lock makes the swap below fail with
+    // Access is denied (#891). Stop every live session first.
     await _stopSessionsForExtension(manifest.id);
 
     final root = await _extensionsDirectory();
     final extDir = ExtensionPaths.extensionDirectory(root, manifest.id);
-    if (await extDir.exists()) {
-      await extDir.delete(recursive: true);
-    }
-    await extDir.create(recursive: true);
+    // Dot-prefixed: the registry skips them and an extension id cannot start
+    // with a dot, so they never collide with or shadow a real install.
+    final staging = Directory(p.join(root.path, '.${manifest.id}.installing'));
+    final backup = Directory(p.join(root.path, '.${manifest.id}.backup'));
 
+    var swapped = false;
     try {
+      if (await staging.exists()) await staging.delete(recursive: true);
+      // A backup left by a crashed swap is the last good install: put it back
+      // rather than letting this attempt delete it.
+      if (await backup.exists()) {
+        if (await extDir.exists()) {
+          await backup.delete(recursive: true);
+        } else {
+          await backup.rename(extDir.path);
+        }
+      }
+      await staging.create(recursive: true);
+
       await _extractArchive(
         archive: archive,
-        destDir: extDir,
-        stripPrefix: stripPrefix,
+        destDir: staging,
+        stripPrefix: _commonRootPrefix(archive),
       );
       onProgress?.call(0.85);
 
       ExtensionSupport.validateDriverPackage(
         manifest: manifest,
-        installDir: extDir,
+        installDir: staging,
       );
       await ExtensionSupport.ensureDriverExecutables(
         manifest: manifest,
-        installDir: extDir,
+        installDir: staging,
       );
 
       // Ensure canonical manifest on disk (pretty-printed, with install metadata).
-      final manifestFile = File(p.join(extDir.path, 'manifest.json'));
+      final manifestFile = File(p.join(staging.path, 'manifest.json'));
       const encoder = JsonEncoder.withIndent('  ');
       await manifestFile.writeAsString(
         encoder.convert(manifest.toJson()),
       );
 
+      final hadPrevious = await extDir.exists();
+      if (hadPrevious) await extDir.rename(backup.path);
+      try {
+        await staging.rename(extDir.path);
+        swapped = true;
+      } catch (_) {
+        if (hadPrevious && !await extDir.exists()) {
+          await backup.rename(extDir.path);
+        }
+        rethrow;
+      }
+      if (hadPrevious) {
+        // The new version is in place; a leftover backup is only clutter.
+        try {
+          await backup.delete(recursive: true);
+        } catch (_) {}
+      }
+
       await _reloadRegistry();
       onProgress?.call(1.0);
-      return ExtensionManifest.fromJson(
-        {
-          ...manifest.toJson(),
-          // installPath is not part of toJson; reload will set it.
-        },
-        installPath: extDir.path,
-      );
+      return extDir;
     } catch (e) {
-      // Best-effort rollback so half-installed packages do not linger.
+      // The previous install was never touched; drop only the half-built one.
       try {
-        if (await extDir.exists()) {
-          await extDir.delete(recursive: true);
+        if (!swapped && await staging.exists()) {
+          await staging.delete(recursive: true);
         }
       } catch (_) {}
       if (e is MarketplaceException) rethrow;
@@ -239,6 +287,14 @@ class LocalExtensionInstaller {
     for (final file in archive) {
       var filename = file.name.replaceAll('\\', '/');
       if (filename.startsWith('__MACOSX/')) continue;
+      // Judge the name as the archive wrote it: a shared `../` root would
+      // otherwise be stripped away together with the prefix.
+      if (!isArchiveEntryNameSafe(filename)) {
+        throw MarketplaceException(
+          'Security violation: Path traversal detected in archive entry '
+          '"${file.name}"',
+        );
+      }
       if (stripPrefix.isNotEmpty && filename.startsWith(stripPrefix)) {
         filename = filename.substring(stripPrefix.length);
       }

lib/core/extensions/local_extension_registry.dart

@@ -50,7 +50,7 @@ class LocalExtensionRegistry {
       // Use list() rather than listSync() to prevent blocking the UI
       final entities = await dir.list().toList();
       for (final entity in entities) {
-        if (entity is Directory) {
+        if (entity is Directory && !p.basename(entity.path).startsWith('.')) {
           final manifestFile = File(p.join(entity.path, 'manifest.json'));
           if (await manifestFile.exists()) {
             try {

lib/core/market/http_marketplace_repository.dart

@@ -10,10 +10,10 @@ import 'package:querya_desktop/core/extensions/extension_driver_session.dart';
 import 'package:querya_desktop/core/extensions/extension_support.dart';
 import 'package:querya_desktop/core/extensions/extension_paths.dart';
 import 'package:querya_desktop/core/extensions/sandbox/sandbox_policy.dart';
+import 'package:querya_desktop/core/extensions/local_extension_installer.dart';
 import 'package:querya_desktop/core/extensions/local_extension_registry.dart';
 import 'package:querya_desktop/core/extensions/models/extension_manifest.dart';
 import 'package:querya_desktop/core/extensions/models/extension_type.dart';
-import 'package:querya_desktop/core/security/archive_path_guard.dart';
 import 'package:querya_desktop/core/security/safe_zip_extractor.dart';
 import 'package:querya_desktop/core/updater/sha256_checksums.dart';
 import 'marketplace_download_policy.dart';
@@ -202,61 +202,14 @@ class HttpMarketplaceRepository implements MarketplaceRepository {
         throw MarketplaceException(error.message);
       }
 
-      final dir = await ExtensionPaths.extensionsDirectory();
-      final extDir = ExtensionPaths.extensionDirectory(dir, manifest.id);
-      if (!await extDir.exists()) {
-        await extDir.create(recursive: true);
-      }
-
-      final extDirPath = p.normalize(extDir.path);
-
-      for (final file in archive) {
-        final filename = file.name;
-        // Check for Path Traversal attempts
-        if (!isArchiveEntryNameSafe(filename)) {
-          throw MarketplaceException('Security violation: Path traversal detected in archive entry "$filename"');
-        }
-
-        final targetPath = p.normalize(p.join(extDirPath, filename));
-        if (!isArchiveExtractPathWithinRoot(extDirPath, targetPath)) {
-          throw MarketplaceException('Security violation: Extraction path out of bounds "$filename"');
-        }
-
-        if (file.isFile) {
-          final outFile = File(targetPath);
-          await outFile.create(recursive: true);
-          final Uint8List bytes;
-          try {
-            bytes = SafeZipExtractor.readEntry(file);
-          } on SafeZipException catch (error) {
-            throw MarketplaceException(error.message);
-          }
-          await outFile.writeAsBytes(bytes);
-          file.clear();
-        } else {
-          await Directory(targetPath).create(recursive: true);
-        }
-      }
-
-      onProgress?.call(0.95);
-
-      ExtensionSupport.validateDriverPackage(
+      // Steps 4-5: extract aside, validate, swap in, write manifest.json and
+      // reload the registry. Same path as local installs, so a failed upgrade
+      // keeps the working version and removed files do not linger (#1562).
+      await LocalExtensionInstaller().installArchive(
+        archive: archive,
         manifest: manifest,
-        installDir: extDir,
+        onProgress: (value) => onProgress?.call(0.85 + value * 0.15),
       );
-      await ExtensionSupport.ensureDriverExecutables(
-        manifest: manifest,
-        installDir: extDir,
-      );
-
-      // Step 4: Write/Update manifest.json in the extension directory
-      final manifestFile = File(p.join(extDir.path, 'manifest.json'));
-      const encoder = JsonEncoder.withIndent('  ');
-      await manifestFile.writeAsString(encoder.convert(manifest.toJson()));
-
-      // Step 5: Reload local extension registry
-      await LocalExtensionRegistry.instance.reload();
-      onProgress?.call(1.0);
     } finally {
       if (await archiveFile.exists()) {
         await archiveFile.delete();

test/core/extensions/local_extension_installer_test.dart

@@ -342,5 +342,76 @@ void main() {
       expect(extDirExistedWhenStopped, isTrue);
       expect(await File(p.join(extDir.path, 'stale.txt')).exists(), isFalse);
     });
+
+    Map<String, Object?> themeManifest(String version) => {
+          'id': 'test.upgrade',
+          'name': 'Upgrade',
+          'version': version,
+          'publisher': 'Test',
+          'type': 'theme',
+          'engines': {'querya_desktop': '*'},
+        };
+
+    test('a failed upgrade keeps the working version (#1562)', () async {
+      final v1 = await _writeZip(
+        tempDir,
+        Archive()
+          ..addFile(_jsonFile('manifest.json', themeManifest('1.0.0')))
+          ..addFile(ArchiveFile('old.txt', 3, utf8.encode('old'))),
+        'v1.zip',
+      );
+      await LocalExtensionInstaller().installFromArchive(v1);
+
+      // The new version fails during extraction, after the old one was
+      // already on disk.
+      final broken = await _writeZip(
+        tempDir,
+        Archive()
+          ..addFile(_jsonFile('manifest.json', themeManifest('2.0.0')))
+          ..addFile(ArchiveFile('../evil.txt', 4, utf8.encode('evil'))),
+        'v2.zip',
+      );
+      await expectLater(
+        LocalExtensionInstaller().installFromArchive(broken),
+        throwsA(isA<MarketplaceException>()),
+      );
+
+      final extDir = Directory(p.join(tempDir.path, 'test.upgrade'));
+      expect(await File(p.join(extDir.path, 'old.txt')).exists(), isTrue);
+      final kept = jsonDecode(
+        await File(p.join(extDir.path, 'manifest.json')).readAsString(),
+      ) as Map<String, dynamic>;
+      expect(kept['version'], '1.0.0');
+      final leftovers = tempDir
+          .listSync()
+          .whereType<Directory>()
+          .where((d) => p.basename(d.path).startsWith('.'));
+      expect(leftovers, isEmpty);
+    });
+
+    test('an upgrade replaces the old files instead of mixing versions (#1562)',
+        () async {
+      final v1 = await _writeZip(
+        tempDir,
+        Archive()
+          ..addFile(_jsonFile('manifest.json', themeManifest('1.0.0')))
+          ..addFile(ArchiveFile('old.txt', 3, utf8.encode('old'))),
+        'v1.zip',
+      );
+      await LocalExtensionInstaller().installFromArchive(v1);
+
+      final v2 = await _writeZip(
+        tempDir,
+        Archive()
+          ..addFile(_jsonFile('manifest.json', themeManifest('2.0.0')))
+          ..addFile(ArchiveFile('new.txt', 3, utf8.encode('new'))),
+        'v2.zip',
+      );
+      await LocalExtensionInstaller().installFromArchive(v2);
+
+      final extDir = Directory(p.join(tempDir.path, 'test.upgrade'));
+      expect(await File(p.join(extDir.path, 'new.txt')).exists(), isTrue);
+      expect(await File(p.join(extDir.path, 'old.txt')).exists(), isFalse);
+    });
   });
 }