diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index 61d8d705d40..d49d5c4df61 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -19,6 +19,10 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens `DASH__SUPPRESS_ANALYTICS=true` is now passed on instead of being reset to `false`. * Require `package:unified_analytics` `^8.4.0`. +* Reject `file://` URIs with non-empty hosts (UNC paths), absolute, missing, or + non-directory `buildLocation` paths, and symbolic links resolving outside the + extension build or package root in `ExtensionsManager`, and store valid + resolved paths in `DevToolsExtensionConfig.extensionAssetsPath`. # 14.0.0 diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index 90da58f2f21..acc0fd55258 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. +import 'dart:io'; + import 'package:collection/collection.dart'; import 'package:dtd/dtd.dart'; import 'package:extension_discovery/extension_discovery.dart'; @@ -149,6 +151,16 @@ class ExtensionsManager { for (final extension in extensions) { final config = extension.config; + if (extension.rootUri.scheme != 'file' || + extension.rootUri.host.isNotEmpty) { + parsingErrors.writeln( + 'Ignoring extension from package "${extension.package}": invalid ' + 'package root URI "${extension.rootUri}" must be a file:// URI ' + 'with an empty host.', + ); + continue; + } + // TODO(https://github.com/dart-lang/pub/issues/4042): make this check // more robust. final isPubliclyHosted = @@ -161,6 +173,21 @@ class ExtensionsManager { final relativeExtensionLocation = config['buildLocation'] as String? ?? 'build'; + // Ensure that `relativeExtensionLocation is not absolute to prevent + // arbitrary access to the filesystem. + // + // For example, path.join(a, b, c, d) discards a, b, and c if d is + // absolute. + if (path.isAbsolute(relativeExtensionLocation) || + path.posix.isAbsolute(relativeExtensionLocation) || + path.windows.isAbsolute(relativeExtensionLocation)) { + parsingErrors.writeln( + 'Ignoring extension from package "${extension.package}": invalid ' + 'buildLocation "$relativeExtensionLocation" must be a relative path.', + ); + continue; + } + final packageRoot = path.normalize(extension.rootUri.toFilePath()); final location = path.normalize( path.join( @@ -185,11 +212,23 @@ class ExtensionsManager { continue; } + final (:resolvedLocation, :error) = _validateAndResolveExtensionLocation( + packageRootPath: packageRoot, + locationPath: location, + relativeExtensionLocation: relativeExtensionLocation, + ); + if (resolvedLocation == null) { + parsingErrors.writeln( + 'Ignoring extension from package "${extension.package}": $error', + ); + continue; + } + try { final extensionConfig = DevToolsExtensionConfig.parse({ ...config, DevToolsExtensionConfig.packageNameKey: extension.package, - DevToolsExtensionConfig.extensionAssetsPathKey: location, + DevToolsExtensionConfig.extensionAssetsPathKey: resolvedLocation, // The [packageConfigPath] will look like // 'pkg/.dart_tool/package_config.json' so we will store the // devtools_options.yaml file at the 'pkg' root: @@ -210,9 +249,128 @@ class ExtensionsManager { } } + /// Validates that [packageRootPath] and [locationPath] exist as directories + /// and that [locationPath] and all entries within it do not resolve via + /// symbolic links outside [packageRootPath] or [locationPath]. + /// + /// This prevents a package from registering a non-existent or non-directory + /// build location, or using symlinks in its extension directory to expose + /// arbitrary files or directories on the host filesystem. + /// + /// Returns a record containing either the canonical `resolvedLocation` on + /// success, or an `error` description using [relativeExtensionLocation] if + /// validation fails. + ({String? resolvedLocation, String? error}) + _validateAndResolveExtensionLocation({ + required String packageRootPath, + required String locationPath, + required String relativeExtensionLocation, + }) { + if (FileSystemEntity.isLinkSync(locationPath) && + !Directory(locationPath).existsSync()) { + return ( + resolvedLocation: null, + error: + 'invalid buildLocation "$relativeExtensionLocation" is a broken ' + 'or non-directory symlink.', + ); + } + + if (!Directory(packageRootPath).existsSync()) { + return ( + resolvedLocation: null, + error: 'package root does not exist or is not a directory.', + ); + } + + if (!Directory(locationPath).existsSync()) { + return ( + resolvedLocation: null, + error: + 'invalid buildLocation "$relativeExtensionLocation" does not ' + 'exist or is not a directory.', + ); + } + + final String resolvedPackageRoot; + final String resolvedLocation; + try { + resolvedPackageRoot = path.normalize( + Directory(packageRootPath).resolveSymbolicLinksSync(), + ); + resolvedLocation = path.normalize( + Directory(locationPath).resolveSymbolicLinksSync(), + ); + } on FileSystemException catch (e) { + return ( + resolvedLocation: null, + error: + 'failed to resolve symbolic links for buildLocation ' + '"$relativeExtensionLocation" ($e).', + ); + } + + if (!path.isWithin(resolvedPackageRoot, resolvedLocation)) { + return ( + resolvedLocation: null, + error: + 'invalid buildLocation "$relativeExtensionLocation" resolves ' + 'outside package root.', + ); + } + + final List entries; + try { + entries = Directory( + resolvedLocation, + ).listSync(recursive: true, followLinks: false); + } on FileSystemException catch (e) { + return ( + resolvedLocation: null, + error: + 'failed to inspect extension build directory ' + '"$relativeExtensionLocation" ($e).', + ); + } + + for (final entry in entries) { + // Because listSync was called with followLinks: false on + // resolvedLocation, any non-Link entry physically resides within + // resolvedLocation. + if (entry is! Link) continue; + String? resolvedTargetPath; + try { + resolvedTargetPath = path.normalize(entry.resolveSymbolicLinksSync()); + } on FileSystemException { + // Leave resolvedTargetPath as null so broken or unresolvable symlinks + // are rejected below. + } + if (resolvedTargetPath == null || + (!path.isWithin(resolvedLocation, resolvedTargetPath) && + !path.equals(resolvedLocation, resolvedTargetPath))) { + return ( + resolvedLocation: null, + error: + 'extension build directory contains a symlink pointing ' + 'outside the build directory ("${entry.path}").', + ); + } + } + return (resolvedLocation: resolvedLocation, error: null); + } + void _assertUriFormat(String? uriString) { - if (uriString != null && !uriString.startsWith('file://')) { - throw ArgumentError.value(uriString, 'must be a file:// URI String'); + if (uriString == null) return; + final uri = Uri.tryParse(uriString); + if (!uriString.startsWith('file://') || + uri == null || + uri.scheme != 'file' || + uri.host.isNotEmpty) { + throw ArgumentError.value( + uriString, + 'uriString', + 'must be a file:// URI String with an empty host', + ); } } diff --git a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart index 035ccaa4600..77ab64c2399 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart @@ -23,6 +23,18 @@ extension _ExtensionsApiHandler on Never { final logs = []; final rootFileUriString = queryParams[ExtensionsApi.packageRootUriPropertyName]; + if (rootFileUriString != null && rootFileUriString.isNotEmpty) { + final rootFileUri = Uri.tryParse(rootFileUriString); + if (!rootFileUriString.startsWith('file://') || + rootFileUri == null || + rootFileUri.scheme != 'file' || + rootFileUri.host.isNotEmpty) { + return api.badRequest( + 'Invalid ${ExtensionsApi.packageRootUriPropertyName}: must be a ' + 'file:// URI with an empty host.', + ); + } + } final result = {}; /// Helper to return a success response with all available extensions diff --git a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart index ca0d9059dd1..70a138acf32 100644 --- a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart +++ b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart @@ -177,6 +177,483 @@ void main() { expect(error, contains('Fake exception for test')); }, ); + + test( + 'rejects packageRootUri with non-empty host (Windows UNC path)', + () async { + await initializeTestDirectory(); + const uncUri = 'file://remotehost/share/app'; + final request = Request( + 'post', + Uri( + scheme: 'https', + host: 'localhost', + path: ExtensionsApi.apiServeAvailableExtensions, + queryParameters: {ExtensionsApi.packageRootUriPropertyName: uncUri}, + ), + ); + final response = await ServerApi.handle( + request, + extensionsManager: extensionsManager, + deeplinkManager: FakeDeeplinkManager(), + dtd: dtd!.info, + ); + expect(response.statusCode, HttpStatus.badRequest); + expect(extensionsManager.devtoolsExtensions, isEmpty); + + await expectLater( + extensionsManager.serveAvailableExtensions(uncUri, [], null), + throwsArgumentError, + ); + }, + ); + + test('rejects extension config.yaml with absolute buildLocation', () async { + await initializeTestDirectory(); + final posixAbsConfigFile = File( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'config.yaml', + ), + ); + posixAbsConfigFile.writeAsStringSync( + '${staticExtension2Package.configYamlContent}\nbuildLocation: /tmp\n', + flush: true, + ); + + final windowsAbsConfigFile = File( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + newerStaticExtension1Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'config.yaml', + ), + ); + windowsAbsConfigFile.writeAsStringSync( + '${newerStaticExtension1Package.configYamlContent}\n' + r'buildLocation: C:\' + '\n', + flush: true, + ); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => e.packageName == staticExtension2Package.packageName, + ), + isEmpty, + ); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => + e.packageName == newerStaticExtension1Package.packageName && + e.version == newerStaticExtension1Package.version, + ), + isEmpty, + ); + + final parsedResponse = json.decode(await response.readAsString()) as Map; + final warning = + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName] + as String; + expect( + warning, + contains('Encountered errors while parsing extension config.yaml'), + ); + expect(warning, contains('must be a relative path')); + }); + + test( + 'rejects extension with symlink pointing outside build directory', + () async { + await initializeTestDirectory(); + final secretFile = File( + p.join(extensionTestManager.testDirectory.path, 'secret.txt'), + )..writeAsStringSync('top secret', flush: true); + + final extensionBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + Link( + p.join(extensionBuildDir.path, 'secret_link.txt'), + ).createSync(secretFile.path); + + final outsideBuildDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'outside_build'), + )..createSync(recursive: true); + final newerBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + newerStaticExtension1Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + newerBuildDir.deleteSync(recursive: true); + Link(newerBuildDir.path).createSync(outsideBuildDir.path); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => e.packageName == staticExtension2Package.packageName, + ), + isEmpty, + ); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => + e.packageName == newerStaticExtension1Package.packageName && + e.version == newerStaticExtension1Package.version, + ), + isEmpty, + ); + + final parsedResponse = + json.decode(await response.readAsString()) as Map; + final warning = + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName] + as String; + expect( + warning, + contains('Encountered errors while parsing extension config.yaml'), + ); + expect( + warning, + contains('contains a symlink pointing outside the build directory'), + ); + expect(warning, contains('resolves outside package root')); + }, + ); + + test('rejects extension with broken or outside directory symlink inside ' + 'build directory', () async { + await initializeTestDirectory(); + final extensionBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + Link( + p.join(extensionBuildDir.path, 'broken_link.txt'), + ).createSync(p.join(extensionBuildDir.path, 'does_not_exist.txt')); + + final outsideDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'outside_dir'), + )..createSync(recursive: true); + final newerBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + newerStaticExtension1Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + Link( + p.join(newerBuildDir.path, 'outside_dir_link'), + ).createSync(outsideDir.path); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => e.packageName == staticExtension2Package.packageName, + ), + isEmpty, + ); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => + e.packageName == newerStaticExtension1Package.packageName && + e.version == newerStaticExtension1Package.version, + ), + isEmpty, + ); + + final parsedResponse = json.decode(await response.readAsString()) as Map; + final warning = + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName] + as String; + expect( + warning, + contains('contains a symlink pointing outside the build directory'), + ); + }); + + test( + 'allows extension with symlinks pointing within build directory', + () async { + await initializeTestDirectory(); + final extensionBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + final indexFile = File(p.join(extensionBuildDir.path, 'index.html')) + ..writeAsStringSync('', flush: true); + final assetsDir = Directory(p.join(extensionBuildDir.path, 'assets')) + ..createSync(recursive: true); + File( + p.join(assetsDir.path, 'data.json'), + ).writeAsStringSync('{}', flush: true); + + Link( + p.join(extensionBuildDir.path, 'alias.html'), + ).createSync(indexFile.path); + Link( + p.join(extensionBuildDir.path, 'assets_alias'), + ).createSync(assetsDir.path); + Link( + p.join(extensionBuildDir.path, 'self_link'), + ).createSync(extensionBuildDir.path); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + final parsedResponse = + json.decode(await response.readAsString()) as Map; + expect( + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName], + isNull, + ); + _verifyAllExtensions(extensionsManager); + }, + ); + + test( + 'rejects extension with non-existent or non-directory buildLocation', + () async { + await initializeTestDirectory(); + final missingBuildDir = Directory( + p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ), + ); + missingBuildDir.deleteSync(recursive: true); + + final regularFileBuildPath = p.join( + extensionTestManager.testDirectory.path, + 'extensions', + newerStaticExtension1Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ); + Directory(regularFileBuildPath).deleteSync(recursive: true); + File( + regularFileBuildPath, + ).writeAsStringSync('not a directory', flush: true); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => e.packageName == staticExtension2Package.packageName, + ), + isEmpty, + ); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => + e.packageName == newerStaticExtension1Package.packageName && + e.version == newerStaticExtension1Package.version, + ), + isEmpty, + ); + + final parsedResponse = + json.decode(await response.readAsString()) as Map; + final warning = + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName] + as String; + expect( + warning, + contains('Encountered errors while parsing extension config.yaml'), + ); + expect( + warning, + contains( + 'Ignoring extension from package ' + '"${staticExtension2Package.packageName}": invalid buildLocation ' + '"build" does not exist or is not a directory.', + ), + ); + expect( + warning, + contains( + 'Ignoring extension from package ' + '"${newerStaticExtension1Package.packageName}": invalid ' + 'buildLocation "build" does not exist or is not a directory.', + ), + ); + }, + ); + + test( + 'rejects extension with broken or non-directory symlink buildLocation', + () async { + await initializeTestDirectory(); + final brokenSymlinkBuildPath = p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + 'extension', + 'devtools', + 'build', + ); + Directory(brokenSymlinkBuildPath).deleteSync(recursive: true); + Link(brokenSymlinkBuildPath).createSync( + p.join(extensionTestManager.testDirectory.path, 'does_not_exist'), + ); + + final newerDevToolsDir = p.join( + extensionTestManager.testDirectory.path, + 'extensions', + newerStaticExtension1Package.relativePathFromExtensions, + 'extension', + 'devtools', + ); + final fileSymlinkBuildPath = p.join(newerDevToolsDir, 'build'); + Directory(fileSymlinkBuildPath).deleteSync(recursive: true); + Link( + fileSymlinkBuildPath, + ).createSync(p.join(newerDevToolsDir, 'config.yaml')); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => e.packageName == staticExtension2Package.packageName, + ), + isEmpty, + ); + expect( + extensionsManager.devtoolsExtensions.where( + (e) => + e.packageName == newerStaticExtension1Package.packageName && + e.version == newerStaticExtension1Package.version, + ), + isEmpty, + ); + + final parsedResponse = + json.decode(await response.readAsString()) as Map; + final warning = + parsedResponse[ExtensionsApi.extensionsResultWarningPropertyName] + as String; + expect( + warning, + contains('Encountered errors while parsing extension config.yaml'), + ); + expect( + warning, + contains( + 'Ignoring extension from package ' + '"${staticExtension2Package.packageName}": invalid buildLocation ' + '"build" is a broken or non-directory symlink.', + ), + ); + expect( + warning, + contains( + 'Ignoring extension from package ' + '"${newerStaticExtension1Package.packageName}": invalid ' + 'buildLocation "build" is a broken or non-directory symlink.', + ), + ); + }, + ); + + test( + 'stores canonical resolvedLocation and prevents post-discovery symlink ' + 'manipulation', + () async { + await initializeTestDirectory(); + final packageRoot = p.join( + extensionTestManager.testDirectory.path, + 'extensions', + staticExtension2Package.relativePathFromExtensions, + ); + final customBuildDir = Directory(p.join(packageRoot, 'custom_build')) + ..createSync(recursive: true); + File( + p.join(customBuildDir.path, 'index.html'), + ).writeAsStringSync('', flush: true); + + final symlinkBuildPath = p.join( + packageRoot, + 'extension', + 'devtools', + 'build', + ); + Directory(symlinkBuildPath).deleteSync(recursive: true); + Link(symlinkBuildPath).createSync(customBuildDir.path); + + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + + final registered = extensionsManager.devtoolsExtensions + .firstWhereOrNull( + (e) => e.packageName == staticExtension2Package.packageName, + ); + expect(registered, isNotNull); + + final canonicalBuildPath = p.normalize( + customBuildDir.resolveSymbolicLinksSync(), + ); + expect(registered!.extensionAssetsPath, canonicalBuildPath); + expect( + extensionsManager.lookupLocationFor(registered.identifier), + canonicalBuildPath, + ); + + // Mutating the symlink after discovery should not affect the stored or + // looked-up canonical assets path. + final outsideDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'outside_build'), + )..createSync(recursive: true); + Link(symlinkBuildPath).deleteSync(); + Link(symlinkBuildPath).createSync(outsideDir.path); + + expect(registered.extensionAssetsPath, canonicalBuildPath); + expect( + extensionsManager.lookupLocationFor(registered.identifier), + canonicalBuildPath, + ); + }, + ); }); group(ExtensionsApi.apiExtensionEnabledState, () { @@ -471,6 +948,10 @@ void _verifyExtension( ), ); } + expect( + ext.extensionAssetsPath, + p.normalize(Directory(ext.extensionAssetsPath).resolveSymbolicLinksSync()), + ); expect( ext.devtoolsOptionsUri, endsWith(