diff --git a/packages/devtools_app_shared/pubspec.yaml b/packages/devtools_app_shared/pubspec.yaml index 15b30678efa..4ed27ca8855 100644 --- a/packages/devtools_app_shared/pubspec.yaml +++ b/packages/devtools_app_shared/pubspec.yaml @@ -15,7 +15,7 @@ resolution: workspace dependencies: collection: ^1.15.0 dds_service_extensions: ^2.0.0 - devtools_shared: ^13.0.0 + devtools_shared: ^14.0.0-wip dtd: ^4.0.0 flutter: sdk: flutter diff --git a/packages/devtools_extensions/pubspec.yaml b/packages/devtools_extensions/pubspec.yaml index 7135b64b4b3..a23c14d3c5f 100644 --- a/packages/devtools_extensions/pubspec.yaml +++ b/packages/devtools_extensions/pubspec.yaml @@ -18,7 +18,7 @@ executables: dependencies: args: ^2.4.2 - devtools_shared: ^13.0.0 + devtools_shared: ^14.0.0-wip devtools_app_shared: ^0.5.1 flutter: sdk: flutter diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index cc2e5aea246..c9575eca4b6 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -3,9 +3,31 @@ Copyright 2025 The Flutter Authors 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. --> -# 13.0.2-wip +# 14.0.0-wip * The minimum Dart SDK version is bumped to 3.11.0. * The minimum Flutter SDK version is bumped to 3.41.0. +* **Breaking changes**: `LocalFileSystem`, an extension which provided some handy + helpers, has been refactored into an extension on the `file` package's + `FileSystem` abstraction. In detail: + * `fileSystem` is a new top-level constant which represents the local (real) + file system. + * `LocalFileSystem.devToolsDir()` is now a static getter, + `FileSystemExtension.devToolsDir`. + * `LocalFileSystem.maybeMoveLegacyDevToolsStore()` is now an instance method, + `FileSystemExtension.maybeMoveLegacyDevToolsStore()`. + * `LocalFileSystem.devToolsStoreLocation()` is now a static getter, + `FileSystemExtension.devToolsStoreLocation`. + * `LocalFileSystem.ensureDevToolsDirectory()` is now private. + * `LocalFileSystem.devToolsFileFromPath()` is now an instance method, + `FileSystemExtension.devToolsFileFromPath()`. + * `LocalFileSystem.devToolsFileAsJson()` is now an instance method, + `FileSystemExtension.devToolsFileAsJson()`. + * `LocalFileSystem.flutterStoreExists()` is now an instance getter, + `FileSystemExtension.flutterStoreExists`. + * `IOPersistentProperties.new` accepts a new optional `FileSystem fs` + argument. +* Update `LocalFileSystem` and `IOPersistentProperties` to use `package:file` + instead of `dart:io` to allow mocking the file system. # 13.0.1 * Handle null values for `FlutterStore.flutterClientId`. diff --git a/packages/devtools_shared/lib/src/server/devtools_store.dart b/packages/devtools_shared/lib/src/server/devtools_store.dart index 5efdf5dd57f..5df2442f1d9 100644 --- a/packages/devtools_shared/lib/src/server/devtools_store.dart +++ b/packages/devtools_shared/lib/src/server/devtools_store.dart @@ -21,10 +21,11 @@ enum DevToolsStoreKeys { /// Provides access to the local DevTools store (~/.flutter-devtools/.devtools). class DevToolsUsage { DevToolsUsage() { - LocalFileSystem.maybeMoveLegacyDevToolsStore(); + // TODO(srawlins): Accept a FileSystem parameter during tests. + fileSystem.maybeMoveLegacyDevToolsStore(); properties = IOPersistentProperties( storeName, - documentDirPath: LocalFileSystem.devToolsDir(), + documentDirPath: FileSystemExtension.devToolsDir, ); _removeLegacyKeys(); } diff --git a/packages/devtools_shared/lib/src/server/file_system.dart b/packages/devtools_shared/lib/src/server/file_system.dart index 6a2036f1de9..450790b47fd 100644 --- a/packages/devtools_shared/lib/src/server/file_system.dart +++ b/packages/devtools_shared/lib/src/server/file_system.dart @@ -3,108 +3,117 @@ // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. import 'dart:convert'; -import 'dart:io'; +import 'dart:io' as io show Platform, File; +import 'package:file/file.dart'; +import 'package:file/local.dart'; import 'package:path/path.dart' as path; import 'devtools_store.dart'; -/// A namespace local file system utlities. -extension LocalFileSystem on Never { - static String _userHomeDir() { - final envKey = Platform.operatingSystem == 'windows' ? 'APPDATA' : 'HOME'; - return Platform.environment[envKey] ?? '.'; +/// The real, local file system, which can be avoided in tests. +const FileSystem fileSystem = LocalFileSystem(); + +extension FileSystemExtension on FileSystem { + static String get _userHomeDir { + final envKey = io.Platform.operatingSystem == 'windows' + ? 'APPDATA' + : 'HOME'; + return io.Platform.environment[envKey] ?? '.'; } - /// Returns the path to the DevTools storage directory. - static String devToolsDir() { - return path.join(_userHomeDir(), '.flutter-devtools'); + /// The path to the DevTools storage directory. + static String get devToolsDir { + return path.join(_userHomeDir, '.flutter-devtools'); } - /// Moves the .devtools file to ~/.flutter-devtools/.devtools if the .devtools - /// file exists in the user's home directory. - static void maybeMoveLegacyDevToolsStore() { - final file = File(path.join(_userHomeDir(), DevToolsUsage.storeName)); - if (file.existsSync()) { - ensureDevToolsDirectory(); - file.copySync(devToolsStoreLocation()); - file.deleteSync(); + /// Moves the `.devtools` file to `~/.flutter-devtools/.devtools` if the + /// `.devtools` file exists in the user's home directory. + void maybeMoveLegacyDevToolsStore() { + final storeFile = file(path.join(_userHomeDir, DevToolsUsage.storeName)); + if (storeFile.existsSync()) { + _ensureDevToolsDirectory(); + storeFile.copySync(devToolsStoreLocation); + storeFile.deleteSync(); } } - static String devToolsStoreLocation() { - return path.join(devToolsDir(), DevToolsUsage.storeName); + static String get devToolsStoreLocation { + return path.join(devToolsDir, DevToolsUsage.storeName); } - /// Creates the ~/.flutter-devtools directory if it does not already exist. - static void ensureDevToolsDirectory() { - Directory(devToolsDir()).createSync(); + /// Creates the `~/.flutter-devtools` directory if it does not already exist. + void _ensureDevToolsDirectory() { + directory(devToolsDir).createSync(); } /// Returns a DevTools file from the given path. /// /// Only files within ~/.flutter-devtools/ can be accessed. - static File? devToolsFileFromPath(String pathFromDevToolsDir) { - if (pathFromDevToolsDir.contains('..') || - path.isAbsolute(pathFromDevToolsDir)) { + File? devToolsFileFromPath(String relativePath) { + if (relativePath.contains('..') || path.isAbsolute(relativePath)) { // The passed in path should not be able to walk up the directory tree - // outside of the ~/.flutter-devtools/ directory. It must also not be an - // absolute path: path.join() discards the base directory when its second - // argument is absolute, which would otherwise allow reading an arbitrary - // file on disk (e.g. an absolute path to a credentials .json file). + // outside of the `~/.flutter-devtools/` directory. It must also not be an + // absolute path: `path.join()` discards the base directory when its + // second argument is absolute, which would otherwise allow reading an + // arbitrary file on disk (e.g. an absolute path to a credentials `.json` + // file). return null; } - ensureDevToolsDirectory(); - final devToolsDirPath = devToolsDir(); - final file = File(path.join(devToolsDirPath, pathFromDevToolsDir)); + _ensureDevToolsDirectory(); + final targetFile = file(path.join(devToolsDir, relativePath)); // Defense in depth: ensure the resolved path is actually contained within // the DevTools directory. - if (!path.isWithin(devToolsDirPath, file.path)) { - return null; - } - if (!file.existsSync()) { - return null; - } - return file; + if (!path.isWithin(devToolsDir, targetFile.path)) return null; + if (!targetFile.existsSync()) return null; + return targetFile; } - /// Returns a DevTools file from the given path as encoded json. + /// Returns a DevTools file from the given path as encoded JSON. /// - /// Only files within ~/.flutter-devtools/ can be accessed. - static String? devToolsFileAsJson(String pathFromDevToolsDir) { - final file = devToolsFileFromPath(pathFromDevToolsDir); - if (file == null) return null; + /// Only files within `~/.flutter-devtools/` can be accessed. + String? devToolsFileAsJson(String relativePath) { + final targetFile = devToolsFileFromPath(relativePath); + if (targetFile == null) return null; - final fileName = path.basename(file.path); + final fileName = path.basename(targetFile.path); if (!fileName.endsWith('.json')) return null; - final content = file.readAsStringSync(); + final content = targetFile.readAsStringSync(); final json = jsonDecode(content) as Map; - json['lastModifiedTime'] = file.lastModifiedSync().toString(); + json['lastModifiedTime'] = targetFile.lastModifiedSync().toString(); return jsonEncode(json); } /// Whether the flutter store file exists. - static bool flutterStoreExists() { - final flutterStore = File(path.join(_userHomeDir(), '.flutter')); + bool get flutterStoreExists { + final flutterStore = file(path.join(_userHomeDir, '.flutter')); return flutterStore.existsSync(); } } class IOPersistentProperties { - IOPersistentProperties(this.name, {String? documentDirPath}) { + IOPersistentProperties( + this.name, { + String? documentDirPath, + FileSystem fs = fileSystem, + }) : _fs = fs { final fileName = name.replaceAll(' ', '_'); - documentDirPath ??= LocalFileSystem._userHomeDir(); - _file = File(path.join(documentDirPath, fileName)); + documentDirPath ??= FileSystemExtension._userHomeDir; + _file = _fs.file(path.join(documentDirPath, fileName)); if (!_file.existsSync()) { _file.createSync(recursive: true); } syncSettings(); } - IOPersistentProperties.fromFile(File file) : name = path.basename(file.path) { - _file = file; + // TODO(srawlins): This is unused in any devtools code. Even in tests. Either + // test it, if it is needed by users, or remove it. + IOPersistentProperties.fromFile(io.File file) + : name = path.basename(file.path), + _fs = file is File ? file.fileSystem : fileSystem { + _file = file is File ? file : _fs.file(file.path); if (!_file.existsSync()) { _file.createSync(recursive: true); } @@ -113,7 +122,9 @@ class IOPersistentProperties { final String name; - late File _file; + final FileSystem _fs; + + late final File _file; late Map _map; diff --git a/packages/devtools_shared/lib/src/server/handlers/_app_size.dart b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart index f7f31a5be2b..f6e61dc579f 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_app_size.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart @@ -19,7 +19,7 @@ extension _AppSizeHandler on Never { if (missingRequiredParams != null) return missingRequiredParams; final filePath = queryParams[AppSizeApi.baseAppSizeFilePropertyName]!; - final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); + final fileJson = fileSystem.devToolsFileAsJson(filePath); if (fileJson == null) { return api.badRequest('No JSON file available at $filePath.'); } @@ -39,7 +39,7 @@ extension _AppSizeHandler on Never { if (missingRequiredParams != null) return missingRequiredParams; final filePath = queryParams[AppSizeApi.testAppSizeFilePropertyName]!; - final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); + final fileJson = fileSystem.devToolsFileAsJson(filePath); if (fileJson == null) { return api.badRequest('No JSON file available at $filePath.'); } diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 3484477c1a4..b600a9457f8 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -71,9 +71,7 @@ class ServerApi { // ----- Flutter Tool GA store. ----- case apiGetFlutterGAClientId: return _encodeResponse( - LocalFileSystem.flutterStoreExists() - ? _flutterStore.flutterClientId - : '', + fileSystem.flutterStoreExists ? _flutterStore.flutterClientId : '', api: api, ); @@ -222,16 +220,16 @@ class ServerApi { : null; } - /// Accessing DevTools store file e.g., ~/.flutter-devtools/.devtools + /// Accessing DevTools store file e.g., `~/.flutter-devtools/.devtools`. static final _devToolsStore = DevToolsUsage(); - /// Accessing Flutter store file e.g., ~/.flutter + /// Accessing Flutter store file e.g., `~/.flutter`. static final _flutterStore = FlutterStore(); static DevToolsUsage get devToolsPreferences => _devToolsStore; /// Provides read and write access to DevTools options files - /// (e.g. path/to/app/root/devtools_options.yaml). + /// (e.g. `path/to/app/root/devtools_options.yaml`). static final _devToolsOptions = DevToolsOptions(); /// Logs a page view in the DevTools server. diff --git a/packages/devtools_shared/pubspec.yaml b/packages/devtools_shared/pubspec.yaml index 7d8e4298542..781cf031de1 100644 --- a/packages/devtools_shared/pubspec.yaml +++ b/packages/devtools_shared/pubspec.yaml @@ -4,7 +4,7 @@ name: devtools_shared description: Package of shared Dart structures between devtools_app, dds, and other tools. -version: 13.0.2-dev +version: 14.0.0-wip repository: https://github.com/flutter/devtools/tree/master/packages/devtools_shared @@ -18,6 +18,7 @@ dependencies: collection: ^1.15.0 dtd: ^4.0.0 extension_discovery: ^2.1.0 + file: ^7.0.0 meta: ^1.9.1 path: ^1.8.0 shelf: ^1.1.0 diff --git a/packages/devtools_shared/test/server/file_system_test.dart b/packages/devtools_shared/test/server/file_system_test.dart index 78069ec1733..b7c2a0d57b0 100644 --- a/packages/devtools_shared/test/server/file_system_test.dart +++ b/packages/devtools_shared/test/server/file_system_test.dart @@ -2,35 +2,110 @@ // 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 'package:devtools_shared/src/server/file_system.dart'; +import 'dart:convert'; + +import 'package:devtools_shared/src/server/file_system.dart' hide fileSystem; +import 'package:file/memory.dart'; +import 'package:path/path.dart' as path; import 'package:test/test.dart'; void main() { - group('LocalFileSystem.devToolsFileFromPath path validation', () { - // These inputs must be rejected before any filesystem access so that reads - // stay confined to the ~/.flutter-devtools/ directory. - - test('rejects absolute paths', () { - // path.join() discards the base directory when its second argument is - // absolute, so an absolute path would otherwise escape the DevTools - // directory and read an arbitrary file on disk. - expect(LocalFileSystem.devToolsFileFromPath('/etc/passwd'), isNull); - expect( - LocalFileSystem.devToolsFileFromPath('/absolute/path/to/file.json'), - isNull, - ); + group('LocalFileSystem.devToolsFileFromPath', () { + late MemoryFileSystem fs; + + setUp(() { + fs = MemoryFileSystem(); + fs + .directory(FileSystemExtension.devToolsDir) + .parent + .createSync(recursive: true); }); - test('rejects paths containing ".."', () { - expect(LocalFileSystem.devToolsFileFromPath('..'), isNull); - expect( - LocalFileSystem.devToolsFileFromPath('../../../etc/passwd'), - isNull, - ); - expect( - LocalFileSystem.devToolsFileFromPath('subdir/../../escape.json'), - isNull, + group('path validation', () { + // These inputs must be rejected before any filesystem access so that reads + // stay confined to the ~/.flutter-devtools/ directory. + + test('rejects absolute paths', () { + final root = path.style == .windows ? 'C:\\' : '/'; + // `path.join()` discards the base directory when its second argument is + // absolute, so an absolute path would otherwise escape the DevTools + // directory and read an arbitrary file on disk. + expect( + fs.devToolsFileFromPath(path.join('${root}etc', 'passwd')), + isNull, + ); + expect( + fs.devToolsFileFromPath( + path.join('${root}absolute', 'path', 'to', 'file.json'), + ), + isNull, + ); + }); + + test('rejects paths containing ".."', () { + expect(fs.devToolsFileFromPath('..'), isNull); + expect( + fs.devToolsFileFromPath(path.join('..', '..', '..', 'etc', 'passwd')), + isNull, + ); + expect( + fs.devToolsFileFromPath( + path.join('subdir', '..', '..', 'escape.json'), + ), + isNull, + ); + }); + }); + + test('returns file when path is valid and file exists', () { + final testFile = fs.file( + path.join(FileSystemExtension.devToolsDir, 'sub', 'file.json'), + )..createSync(recursive: true); + + final file = fs.devToolsFileFromPath(path.join('sub', 'file.json'))!; + expect(file.path, testFile.path); + }); + + test('returns null when path is valid but file does not exist', () { + final file = fs.devToolsFileFromPath( + path.join('sub', 'non_existent.json'), ); + expect(file, isNull); + }); + }); + + group('LocalFileSystem.devToolsFileAsJson', () { + late MemoryFileSystem fs; + + setUp(() { + fs = MemoryFileSystem(); + fs + .directory(FileSystemExtension.devToolsDir) + .parent + .createSync(recursive: true); + }); + + test('returns null if file does not exist', () { + expect(fs.devToolsFileAsJson('test.json'), isNull); + }); + + test('returns null if file is not json', () { + fs.file(path.join(FileSystemExtension.devToolsDir, 'test.txt')) + ..createSync(recursive: true) + ..writeAsStringSync('hello'); + expect(fs.devToolsFileAsJson('test.txt'), isNull); + }); + + test('returns json content with lastModifiedTime', () { + final file = fs.file( + path.join(FileSystemExtension.devToolsDir, 'test.json'), + )..createSync(recursive: true); + file.writeAsStringSync('{"key": "value"}'); + + final jsonStr = fs.devToolsFileAsJson('test.json')!; + final json = jsonDecode(jsonStr) as Map; + expect(json['key'], 'value'); + expect(json['lastModifiedTime'], file.lastModifiedSync().toString()); }); }); } diff --git a/packages/devtools_shared/test/server/persistent_properties_test.dart b/packages/devtools_shared/test/server/persistent_properties_test.dart index 4f78b197995..fd337ef77e9 100644 --- a/packages/devtools_shared/test/server/persistent_properties_test.dart +++ b/packages/devtools_shared/test/server/persistent_properties_test.dart @@ -3,36 +3,37 @@ // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. import 'dart:convert'; -import 'dart:io'; import 'package:devtools_shared/src/server/file_system.dart'; +import 'package:file/file.dart'; +import 'package:file/memory.dart'; +import 'package:path/path.dart' as path; import 'package:test/test.dart'; void main() { group('IOPersistentProperties', () { + late MemoryFileSystem fs; late Directory tempDir; late IOPersistentProperties properties; const storeName = 'test_store'; setUp(() { - tempDir = Directory.systemTemp.createTempSync( + fs = MemoryFileSystem(); + tempDir = fs.systemTempDirectory.createTempSync( 'persistent_properties_test', ); properties = IOPersistentProperties( storeName, documentDirPath: tempDir.path, + fs: fs, ); }); - tearDown(() { - tempDir.deleteSync(recursive: true); - }); - test('remove persists changes to disk', () { properties['key1'] = 'value1'; properties['key2'] = 'value2'; - final file = File('${tempDir.path}/$storeName'); + final file = fs.file(path.join(tempDir.path, storeName)); expect(file.existsSync(), isTrue); var content = file.readAsStringSync(); diff --git a/pubspec.lock b/pubspec.lock index cc8b58e6e9b..78ac48a916f 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -181,10 +181,10 @@ packages: dependency: transitive description: name: cross_file - sha256: "28bb3ae56f117b5aec029d702a90f57d285cd975c3c5c281eaca38dbc47c5937" + sha256: d687bec93342bf6a764a116d15c8694ebeff10e633dc28a39dd3144f7195024e url: "https://pub.dev" source: hosted - version: "0.3.5+2" + version: "0.3.5+3" crypto: dependency: transitive description: