From 255bdb486ad358263e8eed9420331d699d634507 Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Sat, 17 Oct 2026 05:38:22 +0300 Subject: [PATCH] fix(extensions): validate the manifest id before using it as a folder (#1560) ExtensionManifest.fromJson read the id unchecked, and the installers and uninstall joined it onto the extensions directory and deleted the result recursively, so "id": "../x" reached outside it. - fromJson rejects an id that is not letters, digits, '.', '_' or '-' starting with a letter or digit, contains '..', is a Windows device name or is longer than 128 characters. - ExtensionPaths.extensionDirectory(root, id) is the one way to get an extension's folder and refuses anything not strictly inside root; the local installer, the HTTP and mock marketplaces and both uninstalls use it. --- CHANGELOG.md | 1 + lib/core/extensions/extension_paths.dart | 12 ++++ .../extensions/local_extension_installer.dart | 2 +- .../extensions/models/extension_manifest.dart | 20 +++++- .../market/http_marketplace_repository.dart | 4 +- lib/core/market/marketplace_repository.dart | 4 +- .../local_extension_installer_test.dart | 30 +++++++++ test/core/market/extension_manifest_test.dart | 64 +++++++++++++++++++ 8 files changed, 131 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8f44a50..673c8454 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Extension manifest `id` is checked before it becomes a folder** — a sideloaded `.qext` with `"id": "../some/dir"` made the installer recursively delete and overwrite a folder outside the extensions directory. The id must now be letters, digits, `.`, `_` or `-` (starting with a letter or digit, no `..`), and every install / uninstall refuses a folder that is not inside the extensions directory (#1560). - **`querya-ext-tester` sends a `--connect` password with `system.injectCredentials`, like the app** — a driver that reads its password only from the injected secret (the ClickHouse driver) failed authentication under the tester because the password was in the `db.connect` payload only. - **Demo Marketplace themes were named `${manifest.name}`** — the generated theme.json now carries the real name (the demo catalog is only used with `--dart-define=QUERYA_MARKETPLACE_MOCK=true`). - **In-app update of a package install (AUR, `.deb`, `.rpm`)** — the updater no longer downloads a zip and quits when the app lives in `/opt/querya-desktop` (owned by root): the update dialog says how to update it (`yay -Syu querya-desktop`, `apt`, `dnf`) with a *Copy update command* button, or offers the release page for any other folder the user cannot write. If an in-place replace fails anyway, the app is started again and the reason is logged to `~/.local/state/querya-update.log`, instead of staying closed. diff --git a/lib/core/extensions/extension_paths.dart b/lib/core/extensions/extension_paths.dart index a06ba1c4..b863620b 100644 --- a/lib/core/extensions/extension_paths.dart +++ b/lib/core/extensions/extension_paths.dart @@ -2,6 +2,7 @@ import 'dart:io'; import 'package:flutter/foundation.dart'; import 'package:path/path.dart' as p; +import 'package:querya_desktop/core/extensions/models/extension_manifest.dart'; import 'package:querya_desktop/core/storage/app_data_root.dart'; /// Centralizes extension file locations. @@ -30,6 +31,17 @@ abstract final class ExtensionPaths { return Directory(p.join(home, '.querya', _extensionsSegment)); } + /// `/`. Throws [ArgumentError] when [id] is not a single valid + /// folder name inside [root]: callers delete this folder recursively, so a + /// manifest id like `../x` must never reach them (#1560). + static Directory extensionDirectory(Directory root, String id) { + final dir = Directory(p.join(root.path, id)); + if (!ExtensionManifest.isValidId(id) || !p.isWithin(root.path, dir.path)) { + throw ArgumentError.value(id, 'id', 'not a valid extension id'); + } + return dir; + } + /// Creates the extensions directory if it doesn't exist. static Future ensureExtensionsDirectory() async { final dir = await extensionsDirectory(); diff --git a/lib/core/extensions/local_extension_installer.dart b/lib/core/extensions/local_extension_installer.dart index 1afaafa6..d1642333 100644 --- a/lib/core/extensions/local_extension_installer.dart +++ b/lib/core/extensions/local_extension_installer.dart @@ -124,7 +124,7 @@ class LocalExtensionInstaller { await _stopSessionsForExtension(manifest.id); final root = await _extensionsDirectory(); - final extDir = Directory(p.join(root.path, manifest.id)); + final extDir = ExtensionPaths.extensionDirectory(root, manifest.id); if (await extDir.exists()) { await extDir.delete(recursive: true); } diff --git a/lib/core/extensions/models/extension_manifest.dart b/lib/core/extensions/models/extension_manifest.dart index 5d2f0356..853c0b73 100644 --- a/lib/core/extensions/models/extension_manifest.dart +++ b/lib/core/extensions/models/extension_manifest.dart @@ -2,6 +2,7 @@ import 'dart:io'; import 'package:path/path.dart' as p; +import '../../security/archive_path_guard.dart'; import '../../theme/theme_definition.dart'; import 'extension_contributions.dart'; import 'extension_type.dart'; @@ -16,6 +17,16 @@ class ExtensionManifest { /// and docs/extension-registry.md. static const currentManifestVersion = 1; + static final _idPattern = RegExp(r'^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$'); + + /// Whether [id] can name the extension's folder: letters, digits, `.`, `_` + /// and `-`, starting with a letter or digit, no `..` and no device names. + /// Installs and uninstalls delete that folder recursively (#1560). + static bool isValidId(String id) => + _idPattern.hasMatch(id) && + !id.contains('..') && + isArchiveEntryNameSafe(id); + /// `manifestVersion` of `manifest.json`: the format of the file itself, apart /// from the extension's own [version] and the host range in [engines]. final int manifestVersion; @@ -122,10 +133,17 @@ class ExtensionManifest { factory ExtensionManifest.fromJson(Map json, {String? installPath}) { final rawFormat = json['manifestVersion']; + final id = json['id']; + if (id is! String || !isValidId(id)) { + throw FormatException( + '"id" must be letters, digits, ".", "_" or "-" ' + '(starting with a letter or digit): $id', + ); + } return ExtensionManifest( // Not a whole number >= 1: the validator reports it; here it reads as 1. manifestVersion: rawFormat is int && rawFormat >= 1 ? rawFormat : 1, - id: json['id'] as String, + id: id, name: json['name'] as String, version: json['version'] as String? ?? '0.0.0', publisher: json['publisher'] as String? ?? diff --git a/lib/core/market/http_marketplace_repository.dart b/lib/core/market/http_marketplace_repository.dart index 164674ea..17dba8aa 100644 --- a/lib/core/market/http_marketplace_repository.dart +++ b/lib/core/market/http_marketplace_repository.dart @@ -203,7 +203,7 @@ class HttpMarketplaceRepository implements MarketplaceRepository { } final dir = await ExtensionPaths.extensionsDirectory(); - final extDir = Directory(p.join(dir.path, manifest.id)); + final extDir = ExtensionPaths.extensionDirectory(dir, manifest.id); if (!await extDir.exists()) { await extDir.create(recursive: true); } @@ -277,7 +277,7 @@ class HttpMarketplaceRepository implements MarketplaceRepository { } } else { final dir = await ExtensionPaths.extensionsDirectory(); - final extDir = Directory(p.join(dir.path, extensionId)); + final extDir = ExtensionPaths.extensionDirectory(dir, extensionId); if (await extDir.exists()) { await extDir.delete(recursive: true); } diff --git a/lib/core/market/marketplace_repository.dart b/lib/core/market/marketplace_repository.dart index ab49033c..41b9287d 100644 --- a/lib/core/market/marketplace_repository.dart +++ b/lib/core/market/marketplace_repository.dart @@ -209,7 +209,7 @@ class MockMarketplaceRepository implements MarketplaceRepository { // Persist to local extensions directory so LocalExtensionRegistry discovers it final dir = await ExtensionPaths.extensionsDirectory(); - final extDir = Directory(p.join(dir.path, manifest.id)); + final extDir = ExtensionPaths.extensionDirectory(dir, manifest.id); if (!await extDir.exists()) { await extDir.create(recursive: true); } @@ -326,7 +326,7 @@ class MockMarketplaceRepository implements MarketplaceRepository { } else { // Fallback: check standard extensions directory by ID final dir = await ExtensionPaths.extensionsDirectory(); - final extDir = Directory(p.join(dir.path, extensionId)); + final extDir = ExtensionPaths.extensionDirectory(dir, extensionId); if (await extDir.exists()) { await extDir.delete(recursive: true); } diff --git a/test/core/extensions/local_extension_installer_test.dart b/test/core/extensions/local_extension_installer_test.dart index 81bc6247..0e27688a 100644 --- a/test/core/extensions/local_extension_installer_test.dart +++ b/test/core/extensions/local_extension_installer_test.dart @@ -117,6 +117,36 @@ void main() { expect(await Directory(p.join(tempDir.path, 'test.evil')).exists(), isFalse); }); + test('rejects a manifest id outside the extensions folder (#1560)', + () async { + final extensionsDir = Directory(p.join(tempDir.path, 'extensions')) + ..createSync(); + ExtensionPaths.mockExtensionsDirectory = extensionsDir; + final victim = File(p.join(tempDir.path, 'victim', 'keep.txt')) + ..createSync(recursive: true); + + final archive = Archive() + ..addFile(_jsonFile('manifest.json', { + 'id': '../victim', + 'name': 'Evil', + 'version': '1.0.0', + 'publisher': 'Test', + 'type': 'theme', + 'engines': {'querya_desktop': '*'}, + })); + + final zip = await _writeZip(tempDir, archive, 'evil-id.zip'); + await expectLater( + LocalExtensionInstaller().installFromArchive(zip), + throwsA(isA().having( + (e) => e.message, + 'message', + contains('Invalid manifest.json'), + )), + ); + expect(victim.existsSync(), isTrue); + }); + test('rejects preview database drivers without process sandbox', () async { final archive = Archive() ..addFile(_jsonFile('manifest.json', { diff --git a/test/core/market/extension_manifest_test.dart b/test/core/market/extension_manifest_test.dart index e4f8840d..b7dfd207 100644 --- a/test/core/market/extension_manifest_test.dart +++ b/test/core/market/extension_manifest_test.dart @@ -1,4 +1,8 @@ +import 'dart:io'; + import 'package:flutter_test/flutter_test.dart'; +import 'package:path/path.dart' as p; +import 'package:querya_desktop/core/extensions/extension_paths.dart'; import 'package:querya_desktop/core/market/extension_manifest.dart'; import 'package:querya_desktop/core/theme/theme_definition.dart'; import 'package:querya_desktop/core/theme/theme_metadata.dart'; @@ -61,5 +65,65 @@ void main() { expect(manifest.sha256Checksum, 'ff00aa11'); expect(manifest.downloadUrl, isEmpty); }); + + group('id (#1560)', () { + Map json(Object? id) => + {'id': id, 'name': 'X', 'type': 'theme'}; + + test('accepts letters, digits, dots, dashes and underscores', () { + for (final id in [ + 'acme.redis-driver', + 'queryahub.clickhouse-driver', + 'my_theme', + '42', + ]) { + expect(ExtensionManifest.fromJson(json(id)).id, id); + } + }); + + test('rejects ids that are not a single folder name', () { + for (final id in [ + '../x', + '..', + '.', + 'a/b', + r'..\x', + r'a\b', + '/etc', + r'C:\x', + '', + ' ', + '.hidden', + 'a..b', + 'CON', + 'nul.txt', + 'a\x00b', + 'x' * 129, + null, + 42, + ]) { + expect( + () => ExtensionManifest.fromJson(json(id)), + throwsFormatException, + reason: 'id: $id', + ); + } + }); + + test('extensionDirectory refuses an unsafe id', () { + final root = Directory.systemTemp; + expect( + ExtensionPaths.extensionDirectory(root, 'acme.theme').path, + p.join(root.path, 'acme.theme'), + ); + for (final id in ['../x', '', '.', 'a/b']) { + expect( + () => ExtensionPaths.extensionDirectory(root, id), + throwsArgumentError, + reason: 'id: $id', + ); + } + }); + }); }); }