Unverified Commit 1c0eade9 authored by Jenn Magder's avatar Jenn Magder Committed by GitHub

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

parent 4df55087
......@@ -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();
......
......@@ -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';
}
}
}
......@@ -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;
......
......@@ -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);
}
......
......@@ -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);
......
......@@ -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;
}
......
......@@ -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<String> get text async => _text;
final String _text;
@override
Future<String> get piiStrippedText async => _piiStrippedText;
final String _piiStrippedText;
}
......@@ -308,4 +308,7 @@ class FakeError extends Error {
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';
}
......@@ -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: <Type, Generator>{
Platform: () => FakePlatform(
environment: <String, String>{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment