Hide PII from doctor validators for GitHub template (#96250)

Author: jmagman

packages/flutter_tools/lib/runner.dart

@@ -149,11 +149,18 @@ Future<int> _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<File> _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<File> _createLocalCrashReport(CrashDetails details) async {
   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();

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<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 @@ 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<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';
+    }
+  }
+}

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;
 

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);
   }

packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart

@@ -313,6 +313,14 @@ void main() {
     }, 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 @@ 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, <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 @@ 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<ValidatorTask> 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<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 @@ 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<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);

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<ValidatorTask> startedValidatorTasks,
+    bool sendEvent = true,
   }) async {
     return diagnoseSucceeds;
   }