diff --git a/pkg/telemetry/lib/crash_reporting.dart b/pkg/telemetry/lib/crash_reporting.dart index 5ec776f49a9..63be674d109 100644 --- a/pkg/telemetry/lib/crash_reporting.dart +++ b/pkg/telemetry/lib/crash_reporting.dart @@ -9,6 +9,7 @@ import 'dart:math' as math; import 'package:http/http.dart' as http; import 'package:meta/meta.dart'; import 'package:stack_trace/stack_trace.dart'; +import 'package:telemetry/src/pii_regexp.dart'; import 'src/utils.dart'; @@ -129,7 +130,7 @@ class CrashReportSender { fields['osVersion'] = Platform.operatingSystemVersion; fields['type'] = _dartTypeId; fields['error_runtime_type'] = '${error.runtimeType}'; - fields['error_message'] = '$error'; + fields['error_message'] = filterPiiFromErrorMessage('$error'); // Optional comments. if (comment != null) { diff --git a/pkg/telemetry/lib/src/pii_regexp.dart b/pkg/telemetry/lib/src/pii_regexp.dart new file mode 100644 index 00000000000..42846960854 --- /dev/null +++ b/pkg/telemetry/lib/src/pii_regexp.dart @@ -0,0 +1,77 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// This library gives some backstops to make sure that arbitrary string +/// exceptions generated by something that might be sent to the crash reporting +/// framework do not contain PII. The regular expressions here are +/// intentionally broad to make sure they get everything, possibly at the +/// expense of readability for the error message. Use care when reducing +/// their scope. +library pii_regexp; + +/// Contain an ordered list of regular expressions to be applied in sequence +/// with [applyTo] to a string, with any matches being substituted with +/// [substitution]. While a single regular expression could be +/// used in place of this class, this is intending to make the regular +/// expressions being applied more comprehensible to non-expert regular +/// expression authors and to make it very simple to add or remove individual +/// cases. +class _RegExpList { + /// This list should be usually be ordered from more specific to more general + /// cases in the event the regular expressions overlap. + final List _regExps; + final String substitution; + + _RegExpList(List uncompiledRegexps, this.substitution) + : _regExps = uncompiledRegexps.map((s) => RegExp(s)).toList(); + + String applyTo(String input) => _regExps.fold( + input, (previousInput, r) => previousInput.replaceAll(r, substitution)); +} + +/// An ordered list of regular expressions to be substituted out and replaced +/// with a generic '' string. To be applied in sequence. +/// The regular expressions here intentionally eat all characters after a +/// path is identified, with the hope of catching all potential complex +/// pathnames and assuming that most error messages will have any paths last. +/// For example: `throw Exception('error in: $fileName)` +final _piiPathRegexps = _RegExpList([ + // C:\Some\Things\Including Spaces\Too + r'\w:\\.*', + // \\Windows\Shares\Look\Like This + r'\\\\\S+\\.*', + // ..\StartsWithDots\Windows Path + r'[.]+\\+\S*\\.*', + // A\Relative\Windows Path + r'\b\S+\\.*', + // ../StartsWithDots/Unix path + r'[.]+/\S*/.*', + // /AUnix/path + r'/(\S|[.])+/.*', + // a/unix\ relative path/possibly-containing-strange-characters + r'\b(\S|[.])+/.*', +], ''); + +/// An ordered list of regular expressions to be substituted out and replaced +/// with a generic '' string. To be applied after +/// all paths from [_piiPathRegexps] are applied. This is somewhat less +/// all-encompassing than the path finder, above, just to try and grab things +/// that could be file names but without being quite so greedy or depending +/// on the presence of path separators. +final _piiFileRegexps = _RegExpList([ + // foo.dart + // something.exe + // code.js + r'\b\S+[.]\w{1,4}\b', +], ''); + +/// Attempt to remove things that look like they could be PII from the +/// given [message]. +/// Returns the filtered string. +/// Not suitable for pre-scrubbed strings that intentionally include things +/// that look like filenames. +String filterPiiFromErrorMessage(String message) => [ + _piiPathRegexps, + _piiFileRegexps, + ].fold(message, (previousMessage, r) => r.applyTo(previousMessage)); diff --git a/pkg/telemetry/test/crash_reporting_test.dart b/pkg/telemetry/test/crash_reporting_test.dart index ef6bc6fe71b..c2515bae79e 100644 --- a/pkg/telemetry/test/crash_reporting_test.dart +++ b/pkg/telemetry/test/crash_reporting_test.dart @@ -43,6 +43,18 @@ void main() { expect(body, contains('test-error')); }); + test('hits pii filters', () async { + CrashReportSender sender = CrashReportSender.prod( + analytics.trackingId, shouldSend, + httpClient: mockClient); + + await sender.sendReport('filter this: filename.exe', StackTrace.current); + + String body = utf8.decode(request.bodyBytes); + expect(body, contains('String')); // error.runtimeType + expect(body, contains('filter this: ')); + }); + test('reportsSent', () async { CrashReportSender sender = CrashReportSender.prod( analytics.trackingId, shouldSend, diff --git a/pkg/telemetry/test/pii_regexp_test.dart b/pkg/telemetry/test/pii_regexp_test.dart new file mode 100644 index 00000000000..eb88a91c767 --- /dev/null +++ b/pkg/telemetry/test/pii_regexp_test.dart @@ -0,0 +1,69 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:telemetry/src/pii_regexp.dart'; +import 'package:test/test.dart'; + +void main() { + group('filterPiiFromErrorMessage', () { + test('filters paths greedily', () { + expect(filterPiiFromErrorMessage(r'a message: C:\My\Special Path'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage( + r'a message: C:\My\Overly\..\Complicated\Special Path'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage(r'a message: ..\A\Relative\Windows Path'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage( + r'a message: A\Relatively\..\Complicated\Windows Path'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage(r'a message: \\My\Windows\Network Share'), + equals('a message: ')); + expect(filterPiiFromErrorMessage(r'a message: Q:\I Have A Lot of Drives'), + equals('a message: ')); + expect(filterPiiFromErrorMessage(r'a message: R:\'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage(r'a message: /Users/macuser/Desktop/dart/'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage( + r'a message: ../relative_path/to_some/dart'), + equals('a message: ')); + expect( + filterPiiFromErrorMessage( + r'a message: embedded_dots/./../to_some/dart'), + equals('a message: ')); + }); + test('filters some complex examples derived from real exceptions', () { + // Real world examples may not actually be perfectly readable due to the + // greediness of the matchers, but that's the tradeoff for making sure we + // get everything. + expect( + filterPiiFromErrorMessage( + r'Could not resolve "package:an_important_package/an_important_package.dart" in C:\Users\AUser\dart\files\foo.dart'), + equals('Could not resolve "')); + }); + test('filters filenames without path separators without being greedy', () { + expect(filterPiiFromErrorMessage(r'mine.dart: a special file'), + equals(': a special file')); + expect(filterPiiFromErrorMessage(r'foo.js: a special file'), + equals(': a special file')); + expect(filterPiiFromErrorMessage(r'something.exe: a special file'), + equals(': a special file')); + }); + test('does not filter things that should not match', () { + expect(filterPiiFromErrorMessage('User ran a Commodore 64 executable'), + equals('User ran a Commodore 64 executable')); + expect( + filterPiiFromErrorMessage( + 'franchiseError: somehow Palpatine returned'), + equals('franchiseError: somehow Palpatine returned')); + }); + }); +}