From 00f9bc94d309c88cd8d01237da9e89b05c1d8889 Mon Sep 17 00:00:00 2001 From: Felix Angelov Date: Tue, 18 Mar 2025 12:53:13 -0500 Subject: [PATCH] fix(shorebird_cli): windows executable sanitization (#2988) --- .../lib/src/shorebird_process.dart | 18 ++++-- .../test/src/shorebird_process_test.dart | 58 ++++++++++++++++++- 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/packages/shorebird_cli/lib/src/shorebird_process.dart b/packages/shorebird_cli/lib/src/shorebird_process.dart index 64d8eae5..3a2d2572 100644 --- a/packages/shorebird_cli/lib/src/shorebird_process.dart +++ b/packages/shorebird_cli/lib/src/shorebird_process.dart @@ -6,6 +6,7 @@ import 'package:meta/meta.dart'; import 'package:scoped_deps/scoped_deps.dart'; import 'package:shorebird_cli/src/engine_config.dart'; import 'package:shorebird_cli/src/logging/logging.dart'; +import 'package:shorebird_cli/src/platform.dart'; import 'package:shorebird_cli/src/shorebird_env.dart'; /// A reference to a [ShorebirdProcess] instance. @@ -34,9 +35,6 @@ class ShorebirdProcess { Map? environment, String? workingDirectory, }) async { - logger.detail( - '''[Process.stream] $executable ${arguments.join(' ')}${workingDirectory == null ? '' : ' (in $workingDirectory)'}''', - ); final process = await start( executable, arguments, @@ -178,9 +176,21 @@ class ShorebirdProcess { required bool useVendedFlutter, }) { if (useVendedFlutter && executable == 'flutter') { - return shorebirdEnv.flutterBinaryFile.path; + return _sanitizeExecutablePath(shorebirdEnv.flutterBinaryFile.path); } + return _sanitizeExecutablePath(executable); + } + /// Sanitizes the executable path on Windows. + /// https://github.com/dart-lang/sdk/issues/37751 + String _sanitizeExecutablePath(String executable) { + if (executable.isEmpty) return executable; + if (!platform.isWindows) return executable; + if (executable.contains(' ') && !executable.contains('"')) { + // Use quoted strings to indicate where the file name ends and the arguments begin; + // otherwise, the file name is ambiguous. + return '"$executable"'; + } return executable; } diff --git a/packages/shorebird_cli/test/src/shorebird_process_test.dart b/packages/shorebird_cli/test/src/shorebird_process_test.dart index 11512a5f..a62938f8 100644 --- a/packages/shorebird_cli/test/src/shorebird_process_test.dart +++ b/packages/shorebird_cli/test/src/shorebird_process_test.dart @@ -1,12 +1,14 @@ // cspell:ignore asdfasdf -import 'dart:io'; +import 'dart:io' hide Platform; import 'package:mason_logger/mason_logger.dart'; import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as p; +import 'package:platform/platform.dart'; import 'package:scoped_deps/scoped_deps.dart'; import 'package:shorebird_cli/src/engine_config.dart'; import 'package:shorebird_cli/src/logging/logging.dart'; +import 'package:shorebird_cli/src/platform.dart'; import 'package:shorebird_cli/src/shorebird_env.dart'; import 'package:shorebird_cli/src/shorebird_process.dart'; import 'package:test/test.dart'; @@ -21,6 +23,7 @@ void main() { late EngineConfig engineConfig; late ShorebirdLogger logger; + late Platform platform; late ProcessWrapper processWrapper; late Process startProcess; late ShorebirdProcessResult runProcessResult; @@ -33,6 +36,7 @@ void main() { values: { engineConfigRef.overrideWith(() => engineConfig), loggerRef.overrideWith(() => logger), + platformRef.overrideWith(() => platform), shorebirdEnvRef.overrideWith(() => shorebirdEnv), }, ); @@ -41,6 +45,7 @@ void main() { setUp(() { engineConfig = const EngineConfig.empty(); logger = MockShorebirdLogger(); + platform = MockPlatform(); processWrapper = MockProcessWrapper(); runProcessResult = MockProcessResult(); startProcess = MockProcess(); @@ -58,6 +63,8 @@ void main() { when(() => runProcessResult.exitCode).thenReturn(ExitCode.success.code); when(() => logger.level).thenReturn(Level.info); + + when(() => platform.isWindows).thenReturn(false); }); test('ShorebirdProcessResult can be instantiated as a const', () { @@ -94,6 +101,21 @@ void main() { ).called(1); }); + test('sanitizes executable on windows', () { + when(() => platform.isWindows).thenReturn(true); + const executable = + r'C:\Program Files\Android\Android Studio\jbr\bin\java.exe'; + runWithOverrides(() => shorebirdProcess.run(executable, ['--version'])); + verify( + () => processWrapper.run( + '"$executable"', + ['--version'], + environment: any(named: 'environment'), + workingDirectory: any(named: 'workingDirectory'), + ), + ).called(1); + }); + test('replaces "flutter" with our local flutter', () async { await runWithOverrides( () => shorebirdProcess.run('flutter', [ @@ -242,6 +264,23 @@ void main() { ).called(1); }); + test('sanitizes executable on windows', () { + when(() => platform.isWindows).thenReturn(true); + const executable = + r'C:\Program Files\Android\Android Studio\jbr\bin\java.exe'; + runWithOverrides( + () => shorebirdProcess.runSync(executable, ['--version']), + ); + verify( + () => processWrapper.runSync( + '"$executable"', + ['--version'], + environment: any(named: 'environment'), + workingDirectory: any(named: 'workingDirectory'), + ), + ).called(1); + }); + test('replaces "flutter" with our local flutter', () { runWithOverrides( () => shorebirdProcess.runSync('flutter', [ @@ -417,6 +456,23 @@ void main() { ).called(1); }); + test('sanitizes executable on windows', () { + when(() => platform.isWindows).thenReturn(true); + const executable = + r'C:\Program Files\Android\Android Studio\jbr\bin\java.exe'; + runWithOverrides( + () => shorebirdProcess.start(executable, ['--version']), + ); + verify( + () => processWrapper.start( + '"$executable"', + ['--version'], + environment: any(named: 'environment'), + workingDirectory: any(named: 'workingDirectory'), + ), + ).called(1); + }); + test('replaces "flutter" with our local flutter', () async { await runWithOverrides( () => shorebirdProcess.start('flutter', ['run']),