From ce294d6d2b963b899a8d3ba4a7cf4a917397132f Mon Sep 17 00:00:00 2001 From: John Ryan Date: Wed, 11 Nov 2026 11:30:15 -0800 Subject: [PATCH 1/2] Ensure devtoolsOptionsUri points to a served extension and prevent recursive directory creation. This ensures that `handleExtensionEnabledState` checks that `devtools_options.yaml` matches the actual path of an extension that is currently served by `ExtensionsManager`, resolving symbolic links on the parent directory and rejecting `devtools_options.yaml` if it is a symbolic link. This also ensures that `DevToolsOptions._lookupOptionsFile` does not recursively create directories or follow symbolic links if the parent directory does not exist or `devtools_options.yaml` is a symbolic link, preventing `api/extensionEnabledState` from creating directories or writing files outside served extension package roots. --- .../release_notes/NEXT_RELEASE_NOTES.md | 4 + packages/devtools_shared/CHANGELOG.md | 3 + .../src/extensions/extension_enablement.dart | 9 +- .../lib/src/extensions/extension_manager.dart | 5 + .../server/handlers/_devtools_extensions.dart | 60 +++++++++++- .../lib/src/server/server_api.dart | 2 + .../extensions/extension_enablement_test.dart | 80 ++++++++++++++++ .../server/devtools_extensions_api_test.dart | 95 +++++++++++++++++++ pubspec.lock | 12 +-- 9 files changed, 258 insertions(+), 12 deletions(-) diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index d45d7ff0f0f..9ed438b59f3 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -97,6 +97,10 @@ TODO: Remove this section if there are not any updates. the connected app's package root is registered with the Dart Tooling Daemon before available DevTools extensions are refreshed. [#10086](https://github.com/flutter/devtools/pull/10086) +* Confined DevTools extension options file access to currently served + extensions, rejected symlinked `devtools_options.yaml` files, and prevented + creating missing parent directories. + [#10088](https://github.com/flutter/devtools/pull/10088) ## Advanced developer mode updates diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index 3c8f1dff469..e8257206ff3 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -28,6 +28,9 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens 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`. +* Confine `devtoolsOptionsUri` in `api/extensionEnabledState` to currently + served extensions, reject symlinked `devtools_options.yaml` files, and prevent + recursive directory creation when looking up `devtools_options.yaml`. # 14.0.0 diff --git a/packages/devtools_shared/lib/src/extensions/extension_enablement.dart b/packages/devtools_shared/lib/src/extensions/extension_enablement.dart index d86c03ca05d..92251488584 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_enablement.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_enablement.dart @@ -124,15 +124,20 @@ $_extensionsKey: )?.writeAsStringSync(yamlEditor.toString(), flush: true); } - /// Returns the `devtools_options.yaml` file at [optionsUri]. + /// Returns the `devtools_options.yaml` file at [optionsUri], or `null` if its + /// parent directory does not exist or the file is a symbolic link. /// /// If the `devtools_options.yaml` does not already exist, it will be created /// and written with [_defaultOptions], and then returned. File? _lookupOptionsFile(Uri optionsUri) { final optionsFile = File.fromUri(optionsUri); + if (!optionsFile.parent.existsSync() || + FileSystemEntity.isLinkSync(optionsFile.path)) { + return null; + } if (!optionsFile.existsSync()) { optionsFile - ..createSync(recursive: true) + ..createSync() ..writeAsStringSync(_defaultOptions, flush: true); } return optionsFile; diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index acc0fd55258..20844df0b22 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart @@ -32,6 +32,11 @@ class ExtensionsManager { /// [serveAvailableExtensions] is called. final devtoolsExtensions = []; + /// The set of `devtools_options.yaml` file URIs for all currently served + /// DevTools extensions. + Set get servedDevToolsOptionsUris => + devtoolsExtensions.map((e) => e.devtoolsOptionsUri).toSet(); + final _extensionLocationsByIdentifier = {}; /// Returns the absolute path of the assets for the extension with identifier 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 77ab64c2399..b14c7554f09 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart @@ -78,6 +78,7 @@ extension _ExtensionsApiHandler on Never { static shelf.Response handleExtensionEnabledState( ServerApi api, Map queryParams, + ExtensionsManager extensionsManager, ) { final missingRequiredParams = ServerApi._checkRequiredParameters( [ @@ -108,13 +109,39 @@ extension _ExtensionsApiHandler on Never { final fileName = isFileUri ? p.basename(devtoolsOptionsFileUri.toFilePath()) : ''; - if (!isFileUri || fileName != 'devtools_options.yaml') { + if (!isFileUri || fileName != devtoolsOptionsFileName) { return api.badRequest( 'Invalid devtoolsOptionsUri: must be a file: URI named ' - "'devtools_options.yaml'.", + "'$devtoolsOptionsFileName'.", + ); + } + + final requestedOptionsPath = _resolveOptionsFilePath( + devtoolsOptionsFileUri, + ); + + // Make sure that this extension's `devtools_options.yaml` file is + // being served by [ExtensionsManager]. + // + // This prevents DevTools from creating missing directories on disk when + // this path points outside of the extension directory. + final isServedExtensionOptionsFile = + requestedOptionsPath != null && + extensionsManager.servedDevToolsOptionsUris.any((uriString) { + final uri = Uri.tryParse(uriString); + if (uri == null) return false; + final servedPath = _resolveOptionsFilePath(uri); + return servedPath != null && + p.equals(servedPath, requestedOptionsPath); + }); + if (!isServedExtensionOptionsFile) { + return api.badRequest( + 'Invalid devtoolsOptionsUri: must belong to a currently served ' + 'DevTools extension.', ); } + final optionsFileUri = Uri.file(requestedOptionsPath); final extensionName = queryParams[ExtensionsApi.extensionNamePropertyName]!; final extensionPackage = queryParams[ExtensionsApi.extensionPackagePropertyName]; @@ -122,7 +149,7 @@ extension _ExtensionsApiHandler on Never { final activate = queryParams[ExtensionsApi.enabledStatePropertyName]; if (activate != null) { final newState = ServerApi._devToolsOptions.setExtensionEnabledState( - devtoolsOptionsUri: devtoolsOptionsFileUri, + devtoolsOptionsUri: optionsFileUri, extensionName: extensionName, packageName: extensionPackage, enable: bool.parse(activate), @@ -131,10 +158,35 @@ extension _ExtensionsApiHandler on Never { } final activationState = ServerApi._devToolsOptions .lookupExtensionEnabledState( - devtoolsOptionsUri: devtoolsOptionsFileUri, + devtoolsOptionsUri: optionsFileUri, extensionName: extensionName, packageName: extensionPackage, ); return ServerApi._encodeResponse(activationState.name, api: api); } + + /// Resolves symbolic links on the parent directory of [uri] to return the + /// absolute path to `devtools_options.yaml`. + /// + /// Returns the actual file path to `devtools_options.yaml`, or `null` if + /// [uri] is not a local `file:` URI, its parent directory does not exist, or + /// the `devtools_options.yaml` file itself is a symbolic link. + static String? _resolveOptionsFilePath(Uri uri) { + if (uri.scheme != 'file' || uri.host.isNotEmpty) return null; + + final filePath = p.normalize(uri.toFilePath()); + if (p.basename(filePath) != devtoolsOptionsFileName) return null; + + final parentDir = Directory(p.dirname(filePath)); + if (!parentDir.existsSync()) return null; + + try { + final resolvedParent = p.normalize(parentDir.resolveSymbolicLinksSync()); + final resolvedFilePath = p.join(resolvedParent, devtoolsOptionsFileName); + if (FileSystemEntity.isLinkSync(resolvedFilePath)) return null; + return resolvedFilePath; + } on FileSystemException { + return null; + } + } } diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 0b37b3feb0b..f284d7f281f 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -18,6 +18,7 @@ import 'package:vm_service/vm_service.dart'; import '../common.dart'; import '../deeplink/deeplink_manager.dart'; import '../devtools_api.dart'; +import '../extensions/constants.dart'; import '../extensions/extension_enablement.dart'; import '../extensions/extension_manager.dart'; import '../service/service.dart'; @@ -347,6 +348,7 @@ class ServerApi { return _ExtensionsApiHandler.handleExtensionEnabledState( api, queryParams, + extensionsManager, ); // ----- deeplink api. ----- diff --git a/packages/devtools_shared/test/extensions/extension_enablement_test.dart b/packages/devtools_shared/test/extensions/extension_enablement_test.dart index db841b76b34..34472b6ebde 100644 --- a/packages/devtools_shared/test/extensions/extension_enablement_test.dart +++ b/packages/devtools_shared/test/extensions/extension_enablement_test.dart @@ -156,5 +156,85 @@ extensions: - provider: true - custom_pkg: true'''); }); + + test( + 'returns error and does not create parent directory when parent does not ' + 'exist', + () { + final missingParentDir = Directory( + p.join(tmpDir.path, 'missing_parent'), + ); + final missingOptionsUri = Uri.file( + p.join(missingParentDir.path, devtoolsOptionsFileName), + ); + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: missingOptionsUri, + extensionName: 'foo', + ), + ExtensionEnabledState.error, + ); + expect( + options.setExtensionEnabledState( + devtoolsOptionsUri: missingOptionsUri, + extensionName: 'foo', + enable: true, + ), + ExtensionEnabledState.error, + ); + expect(missingParentDir.existsSync(), isFalse); + }, + ); + + test( + 'returns error and does not follow symlink when devtools_options.yaml is ' + 'a symlink', + () { + final targetFile = File(p.join(tmpDir.path, 'target.yaml')) + ..writeAsStringSync('original'); + final brokenTargetFile = File( + p.join(tmpDir.path, 'broken_target.yaml'), + ); + + final optionsLink = Link(p.join(tmpDir.path, devtoolsOptionsFileName)) + ..createSync(targetFile.path); + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'foo', + ), + ExtensionEnabledState.error, + ); + expect( + options.setExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'foo', + enable: true, + ), + ExtensionEnabledState.error, + ); + expect(targetFile.readAsStringSync(), 'original'); + + optionsLink + ..deleteSync() + ..createSync(brokenTargetFile.path); + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'foo', + ), + ExtensionEnabledState.error, + ); + expect( + options.setExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'foo', + enable: true, + ), + ExtensionEnabledState.error, + ); + expect(brokenTargetFile.existsSync(), isFalse); + }, + ); }); } 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 899ae5cbd03..ec672bff0dc 100644 --- a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart +++ b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart @@ -716,6 +716,101 @@ void main() { } }); + test('rejects devtoolsOptionsUri in a non-existent directory and does not ' + 'create the directory', () async { + await serveExtensions(extensionsManager); + final nonExistentDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'non_existent_dir'), + ); + expect(nonExistentDir.existsSync(), isFalse); + + final invalidOptionsUri = Uri.file( + p.join(nonExistentDir.path, devtoolsOptionsFileName), + ).toString(); + final response = await sendEnabledStateRequest( + extensionName: 'drift', + enable: true, + optionsUriOverride: invalidOptionsUri, + ); + + expect(response.statusCode, HttpStatus.badRequest); + expect(nonExistentDir.existsSync(), isFalse); + }); + + test('rejects devtoolsOptionsUri in an existing directory or unserved Dart ' + 'package directory', () async { + await serveExtensions(extensionsManager); + + final nonPackageDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'non_package_dir'), + )..createSync(recursive: true); + final unservedPackageDir = Directory( + p.join(extensionTestManager.testDirectory.path, 'unserved_pkg'), + )..createSync(recursive: true); + + const unservedPubspec = ''' +name: unserved_pkg +environment: + sdk: "^3.5.0" +'''; + File( + p.join(unservedPackageDir.path, 'pubspec.yaml'), + ).writeAsStringSync(unservedPubspec); + + for (final dir in [nonPackageDir, unservedPackageDir]) { + final invalidOptionsFile = File( + p.join(dir.path, devtoolsOptionsFileName), + ); + final response = await sendEnabledStateRequest( + extensionName: 'drift', + enable: true, + optionsUriOverride: invalidOptionsFile.uri.toString(), + ); + + expect(response.statusCode, HttpStatus.badRequest); + expect(invalidOptionsFile.existsSync(), isFalse); + } + }); + + test( + 'rejects devtoolsOptionsUri when devtools_options.yaml is a symlink', + () async { + await serveExtensions(extensionsManager); + + final outsideTarget = File( + p.join(extensionTestManager.testDirectory.path, 'outside_target.txt'), + )..writeAsStringSync('unchanged'); + final brokenTarget = File( + p.join(extensionTestManager.testDirectory.path, 'broken_target.txt'), + ); + + final optionsLink = Link(optionsFile.path) + ..createSync(outsideTarget.path); + addTearDown(() { + if (FileSystemEntity.isLinkSync(optionsLink.path)) { + optionsLink.deleteSync(); + } + }); + + var response = await sendEnabledStateRequest( + extensionName: 'drift', + enable: true, + ); + expect(response.statusCode, HttpStatus.badRequest); + expect(outsideTarget.readAsStringSync(), 'unchanged'); + + optionsLink + ..deleteSync() + ..createSync(brokenTarget.path); + response = await sendEnabledStateRequest( + extensionName: 'drift', + enable: true, + ); + expect(response.statusCode, HttpStatus.badRequest); + expect(brokenTarget.existsSync(), isFalse); + }, + ); + test('accepts a valid devtools_options.yaml file: URI', () async { await serveExtensions(extensionsManager); final response = await sendEnabledStateRequest(extensionName: 'drift'); diff --git a/pubspec.lock b/pubspec.lock index 470e9668f34..a5a55b68152 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -891,26 +891,26 @@ packages: dependency: transitive description: name: test - sha256: de5d145b0afff7921e5e788a880f52d7e5f3ae24068a202f6fd3b58e4ba26323 + sha256: "44b2fccf4407bcdb5e105c064c5d6b5e2a640c87761f8b03cdce8cf532d98cfa" url: "https://pub.dev" source: hosted - version: "1.32.0" + version: "1.33.0" test_api: dependency: transitive description: name: test_api - sha256: "0a10344e901e5b2e63819567951cb6a06673ed6b84f40462188ff5a0c41f371f" + sha256: "47481c58bb92d0b1d218f0510ff5647e768f8e1256d07077a151fb520923b51d" url: "https://pub.dev" source: hosted - version: "0.7.14" + version: "0.7.15" test_core: dependency: transitive description: name: test_core - sha256: "80f3fb49087454e07e7e07c67578cfdd156c8c3a5227d8b3f47c7b2d019c2e93" + sha256: "9dec4c178bd61493e1cd2060ffe701fcb61c43da2493e99590fb1b0cc82eb4af" url: "https://pub.dev" source: hosted - version: "0.6.20" + version: "0.6.21" typed_data: dependency: transitive description: From f53221be912837c05761c37b0674dffcce9e7b26 Mon Sep 17 00:00:00 2001 From: John Ryan Date: Thu, 12 Nov 2026 09:45:00 -0800 Subject: [PATCH 2/2] Address code review comments * Iterate over extensionsManager.devtoolsExtensions. * Return `true` immediately to avoid extra calls to `_resolveOptionsFilePath`. --- .../lib/src/extensions/extension_manager.dart | 5 ----- .../src/server/handlers/_devtools_extensions.dart | 13 +++++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index 20844df0b22..acc0fd55258 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart @@ -32,11 +32,6 @@ class ExtensionsManager { /// [serveAvailableExtensions] is called. final devtoolsExtensions = []; - /// The set of `devtools_options.yaml` file URIs for all currently served - /// DevTools extensions. - Set get servedDevToolsOptionsUris => - devtoolsExtensions.map((e) => e.devtoolsOptionsUri).toSet(); - final _extensionLocationsByIdentifier = {}; /// Returns the absolute path of the assets for the extension with identifier 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 b14c7554f09..af2b3f33957 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart @@ -127,13 +127,22 @@ extension _ExtensionsApiHandler on Never { // this path points outside of the extension directory. final isServedExtensionOptionsFile = requestedOptionsPath != null && - extensionsManager.servedDevToolsOptionsUris.any((uriString) { - final uri = Uri.tryParse(uriString); + extensionsManager.devtoolsExtensions.any((ext) { + final uri = Uri.tryParse(ext.devtoolsOptionsUri); if (uri == null) return false; + if (uri.scheme == 'file' && uri.host.isEmpty) { + // If these are equal, there's no need to call + // _resolveOptionsFilePath again, so return early. + final servedPathUnresolved = p.normalize(uri.toFilePath()); + if (p.equals(servedPathUnresolved, requestedOptionsPath)) { + return true; + } + } final servedPath = _resolveOptionsFilePath(uri); return servedPath != null && p.equals(servedPath, requestedOptionsPath); }); + if (!isServedExtensionOptionsFile) { return api.badRequest( 'Invalid devtoolsOptionsUri: must belong to a currently served '