From 8b5941bb06550547e66d24313a8d965bda4ccacd Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Sun, 21 Jun 2026 18:27:34 +0300 Subject: [PATCH] Fix TOCTOU, ID collisions, redundant scans, and remove legacy theme code --- .../extensions/local_extension_registry.dart | 14 +++- lib/core/theme/theme_controller.dart | 32 +-------- lib/core/theme/theme_import_service.dart | 65 ------------------- lib/core/theme/theme_registry_service.dart | 44 +++++++++++-- test/core/theme/theme_controller_test.dart | 16 +---- .../core/theme/theme_import_service_test.dart | 29 --------- .../theme_registry_legacy_import_test.dart | 54 ++++++--------- 7 files changed, 74 insertions(+), 180 deletions(-) diff --git a/lib/core/extensions/local_extension_registry.dart b/lib/core/extensions/local_extension_registry.dart index ddf17203..95b69c1d 100644 --- a/lib/core/extensions/local_extension_registry.dart +++ b/lib/core/extensions/local_extension_registry.dart @@ -13,6 +13,7 @@ class LocalExtensionRegistry { List _manifests = []; bool _loaded = false; + Future>? _loadFuture; /// Returns an unmodifiable list of loaded manifests. List get manifests => List.unmodifiable(_manifests); @@ -20,13 +21,24 @@ class LocalExtensionRegistry { /// Reloads manifests from the disk. Future reload() async { _loaded = false; + _loadFuture = null; await load(); } /// Loads manifests from the extensions directory if not already loaded. Future> load() async { if (_loaded) return manifests; - + if (_loadFuture != null) return _loadFuture!; + + _loadFuture = _doLoad(); + try { + return await _loadFuture!; + } finally { + _loadFuture = null; + } + } + + Future> _doLoad() async { final dir = await ExtensionPaths.extensionsDirectory(); final loadedManifests = []; diff --git a/lib/core/theme/theme_controller.dart b/lib/core/theme/theme_controller.dart index f38966e4..da681b09 100644 --- a/lib/core/theme/theme_controller.dart +++ b/lib/core/theme/theme_controller.dart @@ -473,37 +473,7 @@ class ThemeController extends ChangeNotifier { return result; } - /// Parses a VS Code theme file, persists it, and activates the imported preset. - Future importThemeFromFile(String path) async { - final result = await ThemeImportService.importFromPath(path); - switch (result) { - case ThemeImportSuccess( - :final name, - :final isDark, - :final colors, - :final tokenColors, - :final storedPath, - ): - await _clearRegistrySelection(); - _importedColors = Map.unmodifiable(colors); - _importedTokenColors = List.unmodifiable(tokenColors); - _importedThemeName = name; - _preset = QueryaThemePreset.imported; - _themeMode = isDark ? ThemeMode.dark : ThemeMode.light; - await AppSettings.instance.setThemeImportedColors(colors); - await AppSettings.instance.setThemeImportName(name); - await AppSettings.instance.setThemeImportPath(storedPath); - await AppSettings.instance.setThemePreset(QueryaThemePreset.imported); - await AppSettings.instance.setThemeMode(_themeMode); - _availableThemes = _mergeBuiltinThemes( - await _registryService.loadThemeDefinitions(), - ); - _notifyThemeChanged(); - return result; - case ThemeImportFailure(): - return result; - } - } + /// Sets or clears a user override for a VS Code `colors` key. Future setWorkbenchColor(String vscodeKey, Color? value) async { diff --git a/lib/core/theme/theme_import_service.dart b/lib/core/theme/theme_import_service.dart index 8787d884..2073bc48 100644 --- a/lib/core/theme/theme_import_service.dart +++ b/lib/core/theme/theme_import_service.dart @@ -6,31 +6,6 @@ import 'package:path_provider/path_provider.dart'; import 'parser/vscode_theme_manifest.dart'; import 'theme_definition.dart'; -/// Result of importing a VS Code theme file. -sealed class ThemeImportResult { - const ThemeImportResult(); -} - -class ThemeImportSuccess extends ThemeImportResult { - const ThemeImportSuccess({ - required this.name, - required this.isDark, - required this.colors, - required this.tokenColors, - required this.storedPath, - }); - - final String name; - final bool isDark; - final Map colors; - final List tokenColors; - final String storedPath; -} - -class ThemeImportFailure extends ThemeImportResult { - const ThemeImportFailure(this.message); - final String message; -} /// Result of copying a theme file into the user themes directory. sealed class ThemeDefinitionImportResult { @@ -80,46 +55,6 @@ abstract final class ThemeImportService { /// Path to the persisted legacy import copy under app support. static Future persistedImportFile() => _storedThemeFile(); - /// Reads [sourcePath], parses JSON/JSONC, copies to app data, returns colors. - static Future importFromPath(String sourcePath) async { - try { - final source = File(sourcePath); - if (!await source.exists()) { - return const ThemeImportFailure('Theme file not found.'); - } - final raw = await source.readAsString(); - final manifest = VsCodeThemeManifest.fromJsonString(raw); - if (manifest.colors.isEmpty) { - return const ThemeImportFailure( - 'Theme file has no "colors" section to import.', - ); - } - - final storedFile = await _storedThemeFile(); - await storedFile.parent.create(recursive: true); - await storedFile.writeAsString(raw); - - final name = manifest.name?.trim().isNotEmpty == true - ? manifest.name!.trim() - : p.basenameWithoutExtension(sourcePath); - - return ThemeImportSuccess( - name: name, - isDark: manifest.isDark || !manifest.isLight, - colors: Map.unmodifiable(manifest.colors), - tokenColors: List.unmodifiable(manifest.tokenColors), - storedPath: storedFile.path, - ); - } on VsCodeThemeParseException catch (e) { - return ThemeImportFailure(e.message); - } on FormatException catch (e) { - return ThemeImportFailure(e.message); - } on IOException catch (e) { - return ThemeImportFailure(e.toString()); - } on Object catch (e) { - return ThemeImportFailure(e.toString()); - } - } /// Reloads colors from the persisted import file, if present. static Future?> loadPersistedColors() async { diff --git a/lib/core/theme/theme_registry_service.dart b/lib/core/theme/theme_registry_service.dart index 4c991311..1d66f21b 100644 --- a/lib/core/theme/theme_registry_service.dart +++ b/lib/core/theme/theme_registry_service.dart @@ -132,21 +132,46 @@ class ThemeRegistryService { await _definitionFromFile(entity, source); if (definition == null) continue; + // Resolve ID collisions for legacy themes + var logicalId = definition.id; + var idSuffix = 2; + var candidateId = logicalId; + while (LocalExtensionRegistry.instance.manifests.any( + (m) => m.type == ExtensionType.theme && m.id == candidateId)) { + candidateId = '$logicalId-$idSuffix'; + idSuffix++; + } + logicalId = candidateId; + final slug = ThemeImportService.slugifyThemeName(definition.name); var finalExtDir = Directory(p.join(extensionsDir.path, slug)); var counter = 2; - while (await finalExtDir.exists()) { + while (true) { + if (!await finalExtDir.exists()) { + try { + await finalExtDir.create(recursive: false); + break; + } on FileSystemException { + // Another async task or process claimed it, keep looping. + } + } finalExtDir = Directory(p.join(extensionsDir.path, '$slug-$counter')); counter++; } - await finalExtDir.create(recursive: true); final themeFile = File(p.join(finalExtDir.path, 'theme.json')); - await entity.copy(themeFile.path); + + if (logicalId != definition.id && definition.format == ThemeFormat.queryaCustom) { + final raw = await entity.readAsString(); + final contentToWrite = _rewriteCustomThemeId(raw, logicalId); + await themeFile.writeAsString(contentToWrite); + } else { + await entity.copy(themeFile.path); + } final manifest = ExtensionManifest( - id: definition.id, + id: logicalId, name: definition.name, version: '1.0.0', publisher: source == ThemeSource.imported ? 'Imported' : 'Unknown', @@ -262,12 +287,19 @@ class ThemeRegistryService { var finalExtDir = Directory(p.join(extensionsDir.path, preferredBaseName)); var counter = 2; - while (await finalExtDir.exists()) { + while (true) { + if (!await finalExtDir.exists()) { + try { + await finalExtDir.create(recursive: false); + break; + } on FileSystemException { + // Another async task or process claimed it, keep looping. + } + } finalExtDir = Directory( p.join(extensionsDir.path, '$preferredBaseName-$counter')); counter++; } - await finalExtDir.create(recursive: true); resolvedFile = File(p.join(finalExtDir.path, 'theme.json')); await resolvedFile.writeAsString(contentToWrite); diff --git a/test/core/theme/theme_controller_test.dart b/test/core/theme/theme_controller_test.dart index 41d8a59e..6cea1a06 100644 --- a/test/core/theme/theme_controller_test.dart +++ b/test/core/theme/theme_controller_test.dart @@ -176,21 +176,7 @@ void main() { ); }); - test('importThemeFromFile applies imported colors to activeTheme', () async { - final c = ThemeController.instance; - await c.load(); - final fixture = File('test/fixtures/themes/dark_subset.json'); - final result = await c.importThemeFromFile(fixture.path); - expect(result, isA()); - expect(c.preset, QueryaThemePreset.imported); - expect(c.hasImportedTheme, isTrue); - expect( - c.activeTheme.workbench.editorBackground, - const Color(0xFF1E1E1E), - ); - await c.resetToDefaults(); - expect(c.preset, QueryaThemePreset.queryaDark); - }); + test('setThemeAnimationEnabled persists and reset clears', () async { final c = ThemeController.instance; diff --git a/test/core/theme/theme_import_service_test.dart b/test/core/theme/theme_import_service_test.dart index e6ae274a..67069ea7 100644 --- a/test/core/theme/theme_import_service_test.dart +++ b/test/core/theme/theme_import_service_test.dart @@ -39,36 +39,7 @@ void main() { } }); - test('importFromPath parses fixture and persists copy', () async { - final fixture = File('test/fixtures/themes/dark_subset.json'); - final result = await ThemeImportService.importFromPath(fixture.path); - expect(result, isA()); - final success = result as ThemeImportSuccess; - expect(success.name, 'Fixture Dark Subset'); - expect(success.isDark, isTrue); - expect(success.colors['editor.background'], '#1e1e1e'); - final reloaded = await ThemeImportService.loadPersistedColors(); - expect(reloaded?['editor.background'], '#1e1e1e'); - }); - - test('importFromPath persists tokenColors from dracula fixture', () async { - final fixture = File('test/fixtures/themes/dracula_tokens.json'); - final result = await ThemeImportService.importFromPath(fixture.path); - expect(result, isA()); - final success = result as ThemeImportSuccess; - expect(success.tokenColors, isNotEmpty); - - final tokens = await ThemeImportService.loadPersistedTokenColors(); - expect(tokens.length, success.tokenColors.length); - expect(tokens.first.scopes, contains('comment')); - }); - - test('importFromPath returns failure for missing file', () async { - final result = - await ThemeImportService.importFromPath('/no/such/theme.json'); - expect(result, isA()); - }); test('slugifyThemeName produces filesystem-safe slug', () { expect( diff --git a/test/core/theme/theme_registry_legacy_import_test.dart b/test/core/theme/theme_registry_legacy_import_test.dart index b81258aa..575ce627 100644 --- a/test/core/theme/theme_registry_legacy_import_test.dart +++ b/test/core/theme/theme_registry_legacy_import_test.dart @@ -7,7 +7,7 @@ import 'package:path_provider_platform_interface/path_provider_platform_interfac import 'package:querya_desktop/core/storage/app_settings.dart'; import 'package:querya_desktop/core/storage/local_db.dart'; import 'package:querya_desktop/core/theme/querya_theme_preset.dart'; -import 'package:querya_desktop/core/theme/theme_controller.dart'; + import 'package:querya_desktop/core/theme/theme_definition.dart'; import 'package:querya_desktop/core/theme/theme_import_service.dart'; import 'package:querya_desktop/core/theme/theme_load_result.dart'; @@ -67,14 +67,16 @@ void main() { test('exposes legacy imported theme from persisted import settings', () async { final fixture = File('test/fixtures/themes/dark_subset.json'); - final importResult = - await ThemeImportService.importFromPath(fixture.path); - expect(importResult, isA()); - final success = importResult as ThemeImportSuccess; - - await AppSettings.instance.setThemeImportedColors(success.colors); - await AppSettings.instance.setThemeImportName(success.name); - await AppSettings.instance.setThemeImportPath(success.storedPath); + final raw = await fixture.readAsString(); + final storedFile = await ThemeImportService.persistedImportFile(); + await storedFile.parent.create(recursive: true); + await storedFile.writeAsString(raw); + + await AppSettings.instance.setThemeImportedColors({ + 'editor.background': '#1e1e1e', + }); + await AppSettings.instance.setThemeImportName('Fixture Dark Subset'); + await AppSettings.instance.setThemeImportPath(storedFile.path); await AppSettings.instance.setThemePreset(QueryaThemePreset.imported); final definitions = await registry.loadThemeDefinitions(); @@ -85,7 +87,7 @@ void main() { expect(legacy.id, ThemeImportService.legacyImportedThemeId); expect(legacy.name, 'Fixture Dark Subset'); expect(legacy.format, ThemeFormat.vscode); - expect(legacy.path, success.storedPath); + expect(legacy.path, storedFile.path); expect( definitions.where((definition) => definition.id == 'imported'), hasLength(1), @@ -94,13 +96,16 @@ void main() { test('loads legacy imported theme definition', () async { final fixture = File('test/fixtures/themes/dark_subset.json'); - final importResult = - await ThemeImportService.importFromPath(fixture.path); - final success = importResult as ThemeImportSuccess; + final raw = await fixture.readAsString(); + final storedFile = await ThemeImportService.persistedImportFile(); + await storedFile.parent.create(recursive: true); + await storedFile.writeAsString(raw); - await AppSettings.instance.setThemeImportedColors(success.colors); - await AppSettings.instance.setThemeImportName(success.name); - await AppSettings.instance.setThemeImportPath(success.storedPath); + await AppSettings.instance.setThemeImportedColors({ + 'editor.background': '#1e1e1e', + }); + await AppSettings.instance.setThemeImportName('Fixture Dark Subset'); + await AppSettings.instance.setThemeImportPath(storedFile.path); final legacy = (await registry.loadThemeDefinitions()).singleWhere( (definition) => definition.source == ThemeSource.legacyImported, @@ -134,23 +139,6 @@ void main() { expect((result as ThemeLoadFailure).message, 'Theme file not found.'); }); - test('QueryaThemePreset.imported still applies via ThemeController', - () async { - final controller = ThemeController.instance; - final fixture = File('test/fixtures/themes/dark_subset.json'); - final result = await controller.importThemeFromFile(fixture.path); - expect(result, isA()); - expect(controller.preset, QueryaThemePreset.imported); - expect(controller.hasImportedTheme, isTrue); - - final definitions = await registry.loadThemeDefinitions(); - expect( - definitions.any( - (definition) => definition.source == ThemeSource.legacyImported, - ), - isTrue, - ); - }); }); }