Hide PII from doctor validators for GitHub template (#96250)
diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart
index b6bde82..c32ff13 100644
--- a/packages/flutter_tools/lib/runner.dart
+++ b/packages/flutter_tools/lib/runner.dart
@@ -149,11 +149,18 @@
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 @@
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 @@
return crashFile;
}
-Future<String> _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<int> _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 598c338..d491c4d 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 @@
}
/// Print information about the state of installed tooling.
+ ///
+ /// To exclude personally identifiable information like device names and
+ /// paths, set [showPii] to false.
Future<bool> diagnose({
bool androidLicenses = false,
bool verbose = true,
bool showColor = true,
AndroidLicenseValidator? androidLicenseValidator,
+ bool showPii = true,
+ List<ValidatorTask>? startedValidatorTasks,
+ bool sendEvent = true,
}) async {
if (androidLicenses && androidLicenseValidator != null) {
return androidLicenseValidator.runLicenseManager();
@@ -306,7 +313,7 @@
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 @@
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 @@
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 @@
}
}
}
+
+/// 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<String> text = _runDiagnosis(true);
+ late final Future<String> piiStrippedText = _runDiagnosis(false);
+
+ // Start the validator tasks only once.
+ late final List<ValidatorTask> _validatorTasks = _doctor.startValidatorTasks();
+
+ Future<String> _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 8458855..6530b5f 100644
--- a/packages/flutter_tools/lib/src/doctor_validator.dart
+++ b/packages/flutter_tools/lib/src/doctor_validator.dart
@@ -221,22 +221,27 @@
///
/// 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 d6f4f94..da9b4cd 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/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 @@
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 @@
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 7a535de..e99711d 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 @@
}, overrides: <Type, Generator>{
Usage: () => testUsage,
});
+
+ testUsingContext('sending events can be skipped', () async {
+ await FakePassingDoctor(logger).diagnose(verbose: false, sendEvent: false);
+
+ expect(testUsage.events, isEmpty);
+ }, overrides: <Type, Generator>{
+ Usage: () => testUsage,
+ });
});
group('doctor with fake validators', () {
@@ -456,6 +464,78 @@
'! 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, <TestUsageEvent>[
+ const TestUsageEvent(
+ 'doctor-result',
+ 'PiiValidator',
+ label: 'installed',
+ ),
+ ]);
+ }, overrides: <Type, Generator>{
+ 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: <Type, Generator>{
+ Usage: () => testUsage,
+ });
});
testUsingContext('validate non-verbose output wrapping', () async {
@@ -535,7 +615,6 @@
));
});
-
group('doctor with grouped validators', () {
testUsingContext('validate diagnose combines validator output', () async {
expect(await FakeGroupedDoctor(logger).diagnose(), isTrue);
@@ -666,6 +745,9 @@
bool verbose = true,
bool showColor = true,
AndroidLicenseValidator androidLicenseValidator,
+ bool showPii = true,
+ List<ValidatorTask> startedValidatorTasks,
+ bool sendEvent = true,
}) async => true;
@override
@@ -694,6 +776,18 @@
}
}
+class PiiValidator extends DoctorValidator {
+ PiiValidator() : super('PII Validator');
+
+ @override
+ Future<ValidationResult> validate() async {
+ const List<ValidationMessage> messages = <ValidationMessage>[
+ 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 @@
}
}
+/// A doctor that passes and contains PII that can be hidden.
+class FakePiiDoctor extends Doctor {
+ FakePiiDoctor(Logger logger) : super(logger: logger);
+
+ List<DoctorValidator> _validators;
+ @override
+ List<DoctorValidator> get validators {
+ return _validators ??= <DoctorValidator>[
+ 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 2371d30..75266cd 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/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 @@
bool verbose = true,
bool showColor = true,
AndroidLicenseValidator androidLicenseValidator,
+ bool showPii = true,
+ List<ValidatorTask> 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 d0c893b..93f596f 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/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 @@
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 @@
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 @@
throw exception;
});
}
+
+class FakeDoctorText extends Fake implements DoctorText {
+ FakeDoctorText(String text, String piiStrippedText)
+ : _text = text, _piiStrippedText = piiStrippedText;
+
+ @override
+ Future<String> get text async => _text;
+ final String _text;
+
+ @override
+ Future<String> 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 279eb9c..81cbd81 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 @@
StackTrace get stackTrace => StackTrace.fromString('''
#0 _File.open.<anonymous closure> (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 4001616..ef5e120 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 @@
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: <Type, Generator>{
Platform: () => FakePlatform(
environment: <String, String>{
