diff --git a/lib/core/extensions/local_extension_installer.dart b/lib/core/extensions/local_extension_installer.dart index e3578431..f2ac7112 100644 --- a/lib/core/extensions/local_extension_installer.dart +++ b/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//`, 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 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); } diff --git a/lib/core/extensions/local_extension_registry.dart b/lib/core/extensions/local_extension_registry.dart index 23ac974c..5c73a84c 100644 --- a/lib/core/extensions/local_extension_registry.dart +++ b/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 { diff --git a/lib/core/market/http_marketplace_repository.dart b/lib/core/market/http_marketplace_repository.dart index 0d2bb4c1..b5074e21 100644 --- a/lib/core/market/http_marketplace_repository.dart +++ b/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(); diff --git a/test/core/extensions/local_extension_installer_test.dart b/test/core/extensions/local_extension_installer_test.dart index 0e27688a..a70f72e4 100644 --- a/test/core/extensions/local_extension_installer_test.dart +++ b/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 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()), + ); + + 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; + expect(kept['version'], '1.0.0'); + final leftovers = tempDir + .listSync() + .whereType() + .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); + }); }); }