diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart index b6bde825b81..c32ff13b5b1 100644 --- a/packages/flutter_tools/lib/runner.dart +++ b/packages/flutter_tools/lib/runner.dart @@ -149,11 +149,18 @@ Future _handleToolError( globals.printError('Oops; flutter has exited unexpectedly: "$error".'); try { + final BufferLogger logger = BufferLogger( + terminal: globals.terminal, + outputPreferences: globals.outputPreferences, + ); + + final DoctorText doctorText = DoctorText(logger); + final CrashDetails details = CrashDetails( command: _crashCommand(args), error: error, stackTrace: stackTrace, - doctorText: await _doctorText(), + doctorText: doctorText, ); final File file = await _createLocalCrashReport(details); await globals.crashReporter.informUser(details, file); @@ -199,7 +206,7 @@ Future _createLocalCrashReport(CrashDetails details) async { buffer.writeln('```\n${details.stackTrace}```\n'); buffer.writeln('## flutter doctor\n'); - buffer.writeln('```\n${details.doctorText}```'); + buffer.writeln('```\n${await details.doctorText.text}```'); try { crashFile.writeAsStringSync(buffer.toString()); @@ -221,22 +228,6 @@ Future _createLocalCrashReport(CrashDetails details) async { return crashFile; } -Future _doctorText() async { - try { - final BufferLogger logger = BufferLogger( - terminal: globals.terminal, - outputPreferences: globals.outputPreferences, - ); - - final Doctor doctor = Doctor(logger: logger); - await doctor.diagnose(showColor: false); - - return logger.statusText; - } on Exception catch (error, trace) { - return 'encountered exception: $error\n\n${trace.toString().trim()}\n'; - } -} - Future _exit(int code) async { // Prints the welcome message if needed. globals.flutterUsage.printWelcome(); diff --git a/packages/flutter_tools/lib/src/doctor.dart b/packages/flutter_tools/lib/src/doctor.dart index 598c33865c6..d491c4daddb 100644 --- a/packages/flutter_tools/lib/src/doctor.dart +++ b/packages/flutter_tools/lib/src/doctor.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:meta/meta.dart'; import 'package:process/process.dart'; import 'android/android_studio_validator.dart'; @@ -290,11 +291,17 @@ class Doctor { } /// Print information about the state of installed tooling. + /// + /// To exclude personally identifiable information like device names and + /// paths, set [showPii] to false. Future diagnose({ bool androidLicenses = false, bool verbose = true, bool showColor = true, AndroidLicenseValidator? androidLicenseValidator, + bool showPii = true, + List? startedValidatorTasks, + bool sendEvent = true, }) async { if (androidLicenses && androidLicenseValidator != null) { return androidLicenseValidator.runLicenseManager(); @@ -306,7 +313,7 @@ class Doctor { bool doctorResult = true; int issues = 0; - for (final ValidatorTask validatorTask in startValidatorTasks()) { + for (final ValidatorTask validatorTask in startedValidatorTasks ?? startValidatorTasks()) { final DoctorValidator validator = validatorTask.validator; final Status status = _logger.startSpinner(); ValidationResult result; @@ -334,8 +341,9 @@ class Doctor { case ValidationType.installed: break; } - - DoctorResultEvent(validator: validator, result: result).send(); + if (sendEvent) { + DoctorResultEvent(validator: validator, result: result).send(); + } final String leadingBox = showColor ? result.coloredLeadingBox : result.leadingBox; if (result.statusInfo != null) { @@ -351,7 +359,7 @@ class Doctor { int hangingIndent = 2; int indent = 4; final String indicator = showColor ? message.coloredIndicator : message.indicator; - for (final String line in '$indicator ${message.message}'.split('\n')) { + for (final String line in '$indicator ${showPii ? message.message : message.piiStrippedMessage}'.split('\n')) { _logger.printStatus(line, hangingIndent: hangingIndent, indent: indent, emphasis: true); // Only do hanging indent for the first line. hangingIndent = 0; @@ -558,3 +566,34 @@ class DeviceValidator extends DoctorValidator { } } } + +/// Wrapper for doctor to run multiple times with PII and without, running the validators only once. +class DoctorText { + DoctorText( + BufferLogger logger, { + @visibleForTesting Doctor? doctor, + }) : _doctor = doctor ?? Doctor(logger: logger), _logger = logger; + + final BufferLogger _logger; + final Doctor _doctor; + bool _sendDoctorEvent = true; + + late final Future text = _runDiagnosis(true); + late final Future piiStrippedText = _runDiagnosis(false); + + // Start the validator tasks only once. + late final List _validatorTasks = _doctor.startValidatorTasks(); + + Future _runDiagnosis(bool showPii) async { + try { + await _doctor.diagnose(showColor: false, startedValidatorTasks: _validatorTasks, showPii: showPii, sendEvent: _sendDoctorEvent); + // Do not send the doctor event a second time. + _sendDoctorEvent = false; + final String text = _logger.statusText; + _logger.clear(); + return text; + } on Exception catch (error, trace) { + return 'encountered exception: $error\n\n${trace.toString().trim()}\n'; + } + } +} diff --git a/packages/flutter_tools/lib/src/doctor_validator.dart b/packages/flutter_tools/lib/src/doctor_validator.dart index 84588557811..6530b5fd379 100644 --- a/packages/flutter_tools/lib/src/doctor_validator.dart +++ b/packages/flutter_tools/lib/src/doctor_validator.dart @@ -221,22 +221,27 @@ class ValidationMessage { /// /// The [contextUrl] may be supplied to link to external resources. This /// is displayed after the informative message in verbose modes. - const ValidationMessage(this.message, {this.contextUrl}) : type = ValidationMessageType.information; + const ValidationMessage(this.message, { this.contextUrl, String? piiStrippedMessage }) + : type = ValidationMessageType.information, piiStrippedMessage = piiStrippedMessage ?? message; /// Create a validation message with information for a failing validator. - const ValidationMessage.error(this.message) + const ValidationMessage.error(this.message, { String? piiStrippedMessage }) : type = ValidationMessageType.error, + piiStrippedMessage = piiStrippedMessage ?? message, contextUrl = null; /// Create a validation message with information for a partially failing /// validator. - const ValidationMessage.hint(this.message) + const ValidationMessage.hint(this.message, { String? piiStrippedMessage }) : type = ValidationMessageType.hint, + piiStrippedMessage = piiStrippedMessage ?? message, contextUrl = null; final ValidationMessageType type; final String? contextUrl; final String message; + /// Optional message with PII stripped, to show instead of [message]. + final String piiStrippedMessage; bool get isError => type == ValidationMessageType.error; diff --git a/packages/flutter_tools/lib/src/reporting/crash_reporting.dart b/packages/flutter_tools/lib/src/reporting/crash_reporting.dart index d6f4f94ecea..da9b4cde9ab 100644 --- a/packages/flutter_tools/lib/src/reporting/crash_reporting.dart +++ b/packages/flutter_tools/lib/src/reporting/crash_reporting.dart @@ -12,6 +12,7 @@ import '../base/io.dart'; import '../base/logger.dart'; import '../base/os.dart'; import '../base/platform.dart'; +import '../doctor.dart'; import '../project.dart'; import 'github_template.dart'; import 'reporting.dart'; @@ -49,7 +50,7 @@ class CrashDetails { final String command; final Object error; final StackTrace stackTrace; - final String doctorText; + final DoctorText doctorText; } /// Reports information about the crash to the user. @@ -92,7 +93,7 @@ class CrashReporter { details.command, details.error, details.stackTrace, - details.doctorText, + await details.doctorText.piiStrippedText, ); _logger.printStatus('$gitHubTemplateURL\n', wrap: false); } diff --git a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart index 7a535de1dfe..e99711df3ac 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart @@ -313,6 +313,14 @@ void main() { }, overrides: { Usage: () => testUsage, }); + + testUsingContext('sending events can be skipped', () async { + await FakePassingDoctor(logger).diagnose(verbose: false, sendEvent: false); + + expect(testUsage.events, isEmpty); + }, overrides: { + Usage: () => testUsage, + }); }); group('doctor with fake validators', () { @@ -456,6 +464,78 @@ void main() { '! Doctor found issues in 4 categories.\n' )); }); + + testUsingContext('validate PII can be hidden', () async { + expect(await FakePiiDoctor(logger).diagnose(showPii: false), isTrue); + expect(logger.statusText, equals( + '[✓] PII Validator\n' + ' • Does not contain PII\n' + '\n' + '• No issues found!\n' + )); + logger.clear(); + // PII shown. + expect(await FakePiiDoctor(logger).diagnose(), isTrue); + expect(logger.statusText, equals( + '[✓] PII Validator\n' + ' • Contains PII path/to/username\n' + '\n' + '• No issues found!\n' + )); + }); + }); + + group('doctor diagnosis wrapper', () { + TestUsage testUsage; + BufferLogger logger; + + setUp(() { + testUsage = TestUsage(); + logger = BufferLogger.test(); + }); + + testUsingContext('PII separated, events only sent once', () async { + final Doctor fakeDoctor = FakePiiDoctor(logger); + final DoctorText doctorText = DoctorText(logger, doctor: fakeDoctor); + const String expectedPiiText = '[✓] PII Validator\n' + ' • Contains PII path/to/username\n' + '\n' + '• No issues found!\n'; + const String expectedPiiStrippedText = + '[✓] PII Validator\n' + ' • Does not contain PII\n' + '\n' + '• No issues found!\n'; + + // Run each multiple times to make sure the logger buffer is being cleared, + // and that events are only sent once. + expect(await doctorText.text, expectedPiiText); + expect(await doctorText.text, expectedPiiText); + + expect(await doctorText.piiStrippedText, expectedPiiStrippedText); + expect(await doctorText.piiStrippedText, expectedPiiStrippedText); + + // Only one event sent. + expect(testUsage.events, [ + const TestUsageEvent( + 'doctor-result', + 'PiiValidator', + label: 'installed', + ), + ]); + }, overrides: { + Usage: () => testUsage, + }); + + testUsingContext('without PII has same text and PII-stripped text', () async { + final Doctor fakeDoctor = FakePassingDoctor(logger); + final DoctorText doctorText = DoctorText(logger, doctor: fakeDoctor); + final String piiText = await doctorText.text; + expect(piiText, isNotEmpty); + expect(piiText, await doctorText.piiStrippedText); + }, overrides: { + Usage: () => testUsage, + }); }); testUsingContext('validate non-verbose output wrapping', () async { @@ -535,7 +615,6 @@ void main() { )); }); - group('doctor with grouped validators', () { testUsingContext('validate diagnose combines validator output', () async { expect(await FakeGroupedDoctor(logger).diagnose(), isTrue); @@ -666,6 +745,9 @@ class NoOpDoctor implements Doctor { bool verbose = true, bool showColor = true, AndroidLicenseValidator androidLicenseValidator, + bool showPii = true, + List startedValidatorTasks, + bool sendEvent = true, }) async => true; @override @@ -694,6 +776,18 @@ class PassingValidator extends DoctorValidator { } } +class PiiValidator extends DoctorValidator { + PiiValidator() : super('PII Validator'); + + @override + Future validate() async { + const List messages = [ + ValidationMessage('Contains PII path/to/username', piiStrippedMessage: 'Does not contain PII'), + ]; + return const ValidationResult(ValidationType.installed, messages); + } +} + class MissingValidator extends DoctorValidator { MissingValidator() : super('Missing Validator'); @@ -840,6 +934,19 @@ class FakeQuietDoctor extends Doctor { } } +/// A doctor that passes and contains PII that can be hidden. +class FakePiiDoctor extends Doctor { + FakePiiDoctor(Logger logger) : super(logger: logger); + + List _validators; + @override + List get validators { + return _validators ??= [ + PiiValidator(), + ]; + } +} + /// A doctor with a validator that throws an exception. class FakeCrashingDoctor extends Doctor { FakeCrashingDoctor(Logger logger) : super(logger: logger); diff --git a/packages/flutter_tools/test/general.shard/analytics_test.dart b/packages/flutter_tools/test/general.shard/analytics_test.dart index 2371d30b175..75266cd66a8 100644 --- a/packages/flutter_tools/test/general.shard/analytics_test.dart +++ b/packages/flutter_tools/test/general.shard/analytics_test.dart @@ -17,6 +17,7 @@ import 'package:flutter_tools/src/commands/build.dart'; import 'package:flutter_tools/src/commands/config.dart'; import 'package:flutter_tools/src/commands/doctor.dart'; import 'package:flutter_tools/src/doctor.dart'; +import 'package:flutter_tools/src/doctor_validator.dart'; import 'package:flutter_tools/src/features.dart'; import 'package:flutter_tools/src/globals.dart' as globals; import 'package:flutter_tools/src/reporting/reporting.dart'; @@ -368,6 +369,9 @@ class FakeDoctor extends Fake implements Doctor { bool verbose = true, bool showColor = true, AndroidLicenseValidator androidLicenseValidator, + bool showPii = true, + List startedValidatorTasks, + bool sendEvent = true, }) async { return diagnoseSucceeds; } diff --git a/packages/flutter_tools/test/general.shard/crash_reporting_test.dart b/packages/flutter_tools/test/general.shard/crash_reporting_test.dart index d0c893be219..93f596fa12d 100644 --- a/packages/flutter_tools/test/general.shard/crash_reporting_test.dart +++ b/packages/flutter_tools/test/general.shard/crash_reporting_test.dart @@ -10,11 +10,13 @@ import 'package:flutter_tools/src/base/io.dart'; import 'package:flutter_tools/src/base/logger.dart'; import 'package:flutter_tools/src/base/os.dart'; import 'package:flutter_tools/src/base/platform.dart'; +import 'package:flutter_tools/src/doctor.dart'; import 'package:flutter_tools/src/project.dart'; import 'package:flutter_tools/src/reporting/crash_reporting.dart'; import 'package:flutter_tools/src/reporting/reporting.dart'; import 'package:http/http.dart'; import 'package:http/testing.dart'; +import 'package:test/fake.dart'; import '../src/common.dart'; import '../src/fake_http_client.dart'; @@ -76,7 +78,7 @@ void main() { expect(logger.traceText, contains('Crash report sent (report ID: test-report-id)')); } - testWithoutContext('CrashReporter.informUser provides basic instructions', () async { + testWithoutContext('CrashReporter.informUser provides basic instructions without PII', () async { final CrashReporter crashReporter = CrashReporter( fileSystem: fs, logger: logger, @@ -91,12 +93,16 @@ void main() { command: 'arg1 arg2 arg3', error: Exception('Dummy exception'), stackTrace: StackTrace.current, - doctorText: 'Fake doctor text'), + // Spaces are URL query encoded in the output, make it one word to make this test simpler. + doctorText: FakeDoctorText('Ignored', 'NoPIIFakeDoctorText'), + ), file, ); - expect(logger.errorText, contains('A crash report has been written to ${file.path}.')); + expect(logger.statusText, contains('NoPIIFakeDoctorText')); + expect(logger.statusText, isNot(contains('Ignored'))); expect(logger.statusText, contains('https://github.com/flutter/flutter/issues/new')); + expect(logger.errorText, contains('A crash report has been written to ${file.path}.')); }); testWithoutContext('suppress analytics', () async { @@ -379,3 +385,16 @@ class CrashingCrashReportSender extends MockClient { throw exception; }); } + +class FakeDoctorText extends Fake implements DoctorText { + FakeDoctorText(String text, String piiStrippedText) + : _text = text, _piiStrippedText = piiStrippedText; + + @override + Future get text async => _text; + final String _text; + + @override + Future get piiStrippedText async => _piiStrippedText; + final String _piiStrippedText; +} diff --git a/packages/flutter_tools/test/general.shard/github_template_test.dart b/packages/flutter_tools/test/general.shard/github_template_test.dart index 279eb9c9661..81cbd8163fc 100644 --- a/packages/flutter_tools/test/general.shard/github_template_test.dart +++ b/packages/flutter_tools/test/general.shard/github_template_test.dart @@ -308,4 +308,7 @@ class FakeError extends Error { StackTrace get stackTrace => StackTrace.fromString(''' #0 _File.open. (dart:io/file_impl.dart:366:9) #1 _rootRunUnary (dart:async/zone.dart:1141:38)'''); + + @override + String toString() => 'PII to ignore'; } diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart index 40016160318..ef5e1200e17 100644 --- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/runner_test.dart @@ -198,7 +198,7 @@ void main() { expect(sentDetails.command, 'flutter crash'); expect(sentDetails.error, 'an exception % --'); expect(sentDetails.stackTrace.toString(), contains('CrashingFlutterCommand.runCommand')); - expect(sentDetails.doctorText, contains('[✓] Flutter')); + expect(await sentDetails.doctorText.text, contains('[✓] Flutter')); }, overrides: { Platform: () => FakePlatform( environment: {