From 3cc70348b5b03f969cd1a8aa265ad80f3695f06c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 10 Dec 2024 23:28:54 +0100 Subject: [PATCH 1/2] build(deps): bump ruby/setup-ruby from 1.202.0 to 1.203.0 (#2470) Bumps [ruby/setup-ruby](https://github.com/ruby/setup-ruby) from 1.202.0 to 1.203.0. - [Release notes](https://github.com/ruby/setup-ruby/releases) - [Changelog](https://github.com/ruby/setup-ruby/blob/master/release.rb) - [Commits](https://github.com/ruby/setup-ruby/compare/a2bbe5b1b236842c1cb7dd11e8e3b51e0a616acc...2a18b06812b0e15bb916e1df298d3e740422c47e) --- updated-dependencies: - dependency-name: ruby/setup-ruby dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giancarlo Buenaflor --- .github/workflows/testflight.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/testflight.yml b/.github/workflows/testflight.yml index a4547016e..a73a2a496 100644 --- a/.github/workflows/testflight.yml +++ b/.github/workflows/testflight.yml @@ -16,7 +16,7 @@ jobs: - uses: actions/checkout@v4 - uses: subosito/flutter-action@f2c4f6686ca8e8d6e6d0f28410eeef506ed66aff # pin@v2.18.0 - run: xcodes select 15.0.1 - - uses: ruby/setup-ruby@a2bbe5b1b236842c1cb7dd11e8e3b51e0a616acc # pin@v1.202.0 + - uses: ruby/setup-ruby@2a18b06812b0e15bb916e1df298d3e740422c47e # pin@v1.203.0 with: ruby-version: '2.7.5' bundler-cache: true From ab99a31f02c555d16a22a2404065a895cce1653b Mon Sep 17 00:00:00 2001 From: Ivan Dlugos <6349682+vaind@users.noreply.github.com> Date: Wed, 11 Dec 2024 10:55:17 +0100 Subject: [PATCH 2/2] feat: warning in screenshot/replay creation when a potentially sensitive widget is not masked (#2375) * feat: fail screenshot/replay creation when a potentially sensitive widget is not masked * fix tests for web * improve text * warn instead of throwin an exception * chore: changelog * test: add debug/profile/release mode tests * test assertion --- CHANGELOG.md | 4 ++ flutter/lib/src/screenshot/recorder.dart | 20 +++--- flutter/lib/src/sentry_privacy_options.dart | 36 ++++++++++- ...flutter_enricher_event_processor_test.dart | 24 ++++++-- .../debug_print_integration_test.dart | 5 +- flutter/test/mocks.dart | 39 +++++++++--- .../test/screenshot/masking_config_test.dart | 24 +++++--- flutter/test/screenshot/test_widget.dart | 1 + .../test/screenshot/widget_filter_test.dart | 61 ++++++++++++++++--- 9 files changed, 173 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cad2f811e..d07743001 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Enhancements + +- Warning (in a debug build) if a potentially sensitive widget is not masked or unmasked explicitly ([#2375](https://github.com/getsentry/sentry-dart/pull/2375)) + ### Dependencies - Bump Native SDK from v0.7.15 to v0.7.16 ([#2465](https://github.com/getsentry/sentry-dart/pull/2465)) diff --git a/flutter/lib/src/screenshot/recorder.dart b/flutter/lib/src/screenshot/recorder.dart index 2fe86c6b6..4595a9c4c 100644 --- a/flutter/lib/src/screenshot/recorder.dart +++ b/flutter/lib/src/screenshot/recorder.dart @@ -46,19 +46,26 @@ class ScreenshotRecorder { final privacyOptions = isReplayRecorder ? options.experimental.privacyForReplay : options.experimental.privacyForScreenshots; - final maskingConfig = privacyOptions?.buildMaskingConfig(); + final maskingConfig = + privacyOptions?.buildMaskingConfig(_log, options.platformChecker); if (maskingConfig != null && maskingConfig.length > 0) { _widgetFilter = WidgetFilter(maskingConfig, options.logger); } } + void _log(SentryLevel level, String message, + {String? logger, Object? exception, StackTrace? stackTrace}) { + options.logger(level, '$logName: $message', + logger: logger, exception: exception, stackTrace: stackTrace); + } + Future capture(ScreenshotRecorderCallback callback) async { final context = sentryScreenshotWidgetGlobalKey.currentContext; final renderObject = context?.findRenderObject() as RenderRepaintBoundary?; if (context == null || renderObject == null) { if (!_warningLogged) { - options.logger(SentryLevel.warning, - "$logName: SentryScreenshotWidget is not attached, skipping capture."); + _log(SentryLevel.warning, + "SentryScreenshotWidget is not attached, skipping capture."); _warningLogged = true; } return; @@ -112,9 +119,9 @@ class ScreenshotRecorder { try { final finalImage = await picture.toImage( (srcWidth * pixelRatio).round(), (srcHeight * pixelRatio).round()); - options.logger( + _log( SentryLevel.debug, - "$logName: captured a screenshot in ${watch.elapsedMilliseconds}" + "captured a screenshot in ${watch.elapsedMilliseconds}" " ms ($blockingTime ms blocking)."); try { await callback(finalImage); @@ -125,8 +132,7 @@ class ScreenshotRecorder { picture.dispose(); } } catch (e, stackTrace) { - options.logger( - SentryLevel.error, "$logName: failed to capture screenshot.", + _log(SentryLevel.error, "failed to capture screenshot.", exception: e, stackTrace: stackTrace); if (options.automatedTestMode) { rethrow; diff --git a/flutter/lib/src/sentry_privacy_options.dart b/flutter/lib/src/sentry_privacy_options.dart index 7873116b9..29bd644ea 100644 --- a/flutter/lib/src/sentry_privacy_options.dart +++ b/flutter/lib/src/sentry_privacy_options.dart @@ -27,7 +27,8 @@ class SentryPrivacyOptions { final _userMaskingRules = []; @internal - SentryMaskingConfig buildMaskingConfig() { + SentryMaskingConfig buildMaskingConfig( + SentryLogger logger, PlatformChecker platform) { // First, we collect rules defined by the user (so they're applied first). final rules = _userMaskingRules.toList(); @@ -50,12 +51,45 @@ class SentryPrivacyOptions { assert(!maskAssetImages, "maskAssetImages can't be true if maskAllImages is false"); } + if (maskAllText) { rules.add( const SentryMaskingConstantRule(SentryMaskingDecision.mask)); rules.add(const SentryMaskingConstantRule( SentryMaskingDecision.mask)); } + + // In Debug mode, check if users explicitly mask (or unmask) widgets that + // look like they should be masked, e.g. Videos, WebViews, etc. + if (platform.isDebugMode()) { + final regexp = RegExp('video|webview|password|pinput|camera|chart', + caseSensitive: false); + + // Note: the following line just makes sure if the option is renamed, + // someone will notice that there is a string that needs updating too. + SentryFlutterOptions().experimental.privacy; + final optionsName = 'options.experimental.privacy'; + + rules.add( + SentryMaskingCustomRule((Element element, Widget widget) { + final type = widget.runtimeType.toString(); + if (regexp.hasMatch(type)) { + logger( + SentryLevel.warning, + 'Widget "$widget" name matches widgets that should usually be ' + 'masked because they may contain sensitive data. Because this ' + 'widget comes from a third-party plugin or your code, Sentry ' + "doesn't recognize it and can't reliably mask it in release " + 'builds (due to obfuscation). ' + 'Please mask it explicitly using $optionsName.mask<$type>(). ' + 'If you want to silence this warning and keep the widget ' + 'visible in captures, you can use $optionsName.unmask<$type>(). ' + 'Note: the RegExp matched is: $regexp (case insensitive).'); + } + return SentryMaskingDecision.continueProcessing; + })); + } + return SentryMaskingConfig(rules); } diff --git a/flutter/test/event_processor/flutter_enricher_event_processor_test.dart b/flutter/test/event_processor/flutter_enricher_event_processor_test.dart index 0873bd3b2..6f82a6e6b 100644 --- a/flutter/test/event_processor/flutter_enricher_event_processor_test.dart +++ b/flutter/test/event_processor/flutter_enricher_event_processor_test.dart @@ -222,12 +222,24 @@ void main() { testWidgets('adds correct flutter runtime', (WidgetTester tester) async { final checkerMap = { - MockPlatformChecker(isWebValue: false, isDebug: true): 'Dart VM', - MockPlatformChecker(isWebValue: false, isProfile: true): 'Dart AOT', - MockPlatformChecker(isWebValue: false, isRelease: true): 'Dart AOT', - MockPlatformChecker(isWebValue: true, isDebug: true): 'dartdevc', - MockPlatformChecker(isWebValue: true, isProfile: true): 'dart2js', - MockPlatformChecker(isWebValue: true, isRelease: true): 'dart2js', + MockPlatformChecker( + isWebValue: false, + buildMode: MockPlatformCheckerBuildMode.debug): 'Dart VM', + MockPlatformChecker( + isWebValue: false, + buildMode: MockPlatformCheckerBuildMode.profile): 'Dart AOT', + MockPlatformChecker( + isWebValue: false, + buildMode: MockPlatformCheckerBuildMode.release): 'Dart AOT', + MockPlatformChecker( + isWebValue: true, + buildMode: MockPlatformCheckerBuildMode.debug): 'dartdevc', + MockPlatformChecker( + isWebValue: true, + buildMode: MockPlatformCheckerBuildMode.profile): 'dart2js', + MockPlatformChecker( + isWebValue: true, + buildMode: MockPlatformCheckerBuildMode.release): 'dart2js', }; for (var pair in checkerMap.entries) { diff --git a/flutter/test/integrations/debug_print_integration_test.dart b/flutter/test/integrations/debug_print_integration_test.dart index 67359ade7..a5580d635 100644 --- a/flutter/test/integrations/debug_print_integration_test.dart +++ b/flutter/test/integrations/debug_print_integration_test.dart @@ -94,7 +94,10 @@ class Fixture { bool debug = false, bool enablePrintBreadcrumbs = true, }) { - return defaultTestOptions(MockPlatformChecker(isDebug: debug)) + return defaultTestOptions(MockPlatformChecker( + buildMode: debug + ? MockPlatformCheckerBuildMode.debug + : MockPlatformCheckerBuildMode.release)) ..enablePrintBreadcrumbs = enablePrintBreadcrumbs; } diff --git a/flutter/test/mocks.dart b/flutter/test/mocks.dart index 60eb12dc2..66e05b68e 100644 --- a/flutter/test/mocks.dart +++ b/flutter/test/mocks.dart @@ -99,18 +99,14 @@ class MockPlatform with NoSuchMethodProvider implements Platform { class MockPlatformChecker with NoSuchMethodProvider implements PlatformChecker { MockPlatformChecker({ - this.isDebug = false, - this.isProfile = false, - this.isRelease = false, + this.buildMode = MockPlatformCheckerBuildMode.debug, this.isWebValue = false, this.hasNativeIntegration = false, this.isRoot = true, Platform? mockPlatform, }) : _mockPlatform = mockPlatform ?? MockPlatform(''); - final bool isDebug; - final bool isProfile; - final bool isRelease; + final MockPlatformCheckerBuildMode buildMode; final bool isWebValue; final bool isRoot; final Platform _mockPlatform; @@ -119,13 +115,13 @@ class MockPlatformChecker with NoSuchMethodProvider implements PlatformChecker { bool hasNativeIntegration = false; @override - bool isDebugMode() => isDebug; + bool isDebugMode() => buildMode == MockPlatformCheckerBuildMode.debug; @override - bool isProfileMode() => isProfile; + bool isProfileMode() => buildMode == MockPlatformCheckerBuildMode.profile; @override - bool isReleaseMode() => isRelease; + bool isReleaseMode() => buildMode == MockPlatformCheckerBuildMode.release; @override bool get isRootZone => isRoot; @@ -137,6 +133,8 @@ class MockPlatformChecker with NoSuchMethodProvider implements PlatformChecker { Platform get platform => _mockPlatform; } +enum MockPlatformCheckerBuildMode { debug, profile, release } + // Does nothing or returns default values. // Useful for when a Hub needs to be passed but is not used. class NoOpHub with NoSuchMethodProvider implements Hub { @@ -237,3 +235,26 @@ class FunctionEventProcessor implements EventProcessor { return applyFunction(event, hint); } } + +class MockLogger { + final items = []; + + void call(SentryLevel level, String message, + {String? logger, Object? exception, StackTrace? stackTrace}) { + items.add(MockLogItem(level, message, + logger: logger, exception: exception, stackTrace: stackTrace)); + } + + void clear() => items.clear(); +} + +class MockLogItem { + final SentryLevel level; + final String message; + final String? logger; + final Object? exception; + final StackTrace? stackTrace; + + const MockLogItem(this.level, this.message, + {this.logger, this.exception, this.stackTrace}); +} diff --git a/flutter/test/screenshot/masking_config_test.dart b/flutter/test/screenshot/masking_config_test.dart index 90333c5fe..23cef2ddb 100644 --- a/flutter/test/screenshot/masking_config_test.dart +++ b/flutter/test/screenshot/masking_config_test.dart @@ -3,6 +3,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:sentry_flutter/sentry_flutter.dart'; import 'package:sentry_flutter/src/screenshot/masking_config.dart'; +import '../mocks.dart'; import 'test_widget.dart'; void main() async { @@ -115,15 +116,14 @@ void main() async { group('$SentryReplayOptions.buildMaskingConfig()', () { List rulesAsStrings(SentryPrivacyOptions options) { - final config = options.buildMaskingConfig(); + final config = + options.buildMaskingConfig(MockLogger().call, PlatformChecker()); return config.rules .map((rule) => rule.toString()) // These normalize the string on VM & js & wasm: .map((str) => str.replaceAll( - RegExp( - r"SentryMaskingDecision from:? [fF]unction '?_maskImagesExceptAssets[@(].*", - dotAll: true), - 'SentryMaskingDecision)')) + RegExp(r"=> SentryMaskingDecision from:? .*", dotAll: true), + '=> SentryMaskingDecision)')) .map((str) => str.replaceAll( ' from: (element, widget) => masking_config.SentryMaskingDecision.mask', '')) @@ -136,7 +136,8 @@ void main() async { ...alwaysEnabledRules, '$SentryMaskingCustomRule<$Image>(Closure: (Element, Widget) => SentryMaskingDecision)', '$SentryMaskingConstantRule<$Text>(mask)', - '$SentryMaskingConstantRule<$EditableText>(mask)' + '$SentryMaskingConstantRule<$EditableText>(mask)', + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' ]); }); @@ -148,6 +149,7 @@ void main() async { expect(rulesAsStrings(sut), [ ...alwaysEnabledRules, '$SentryMaskingConstantRule<$Image>(mask)', + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' ]); }); @@ -159,6 +161,7 @@ void main() async { expect(rulesAsStrings(sut), [ ...alwaysEnabledRules, '$SentryMaskingCustomRule<$Image>(Closure: (Element, Widget) => SentryMaskingDecision)', + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' ]); }); @@ -171,6 +174,7 @@ void main() async { ...alwaysEnabledRules, '$SentryMaskingConstantRule<$Text>(mask)', '$SentryMaskingConstantRule<$EditableText>(mask)', + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' ]); }); @@ -179,7 +183,10 @@ void main() async { ..maskAllText = false ..maskAllImages = false ..maskAssetImages = false; - expect(rulesAsStrings(sut), alwaysEnabledRules); + expect(rulesAsStrings(sut), [ + ...alwaysEnabledRules, + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' + ]); }); group('user rules', () { @@ -187,7 +194,8 @@ void main() async { ...alwaysEnabledRules, '$SentryMaskingCustomRule<$Image>(Closure: (Element, Widget) => SentryMaskingDecision)', '$SentryMaskingConstantRule<$Text>(mask)', - '$SentryMaskingConstantRule<$EditableText>(mask)' + '$SentryMaskingConstantRule<$EditableText>(mask)', + '$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)' ]; test('mask() takes precedence', () { final sut = SentryPrivacyOptions(); diff --git a/flutter/test/screenshot/test_widget.dart b/flutter/test/screenshot/test_widget.dart index 2e8d257a2..7011dc610 100644 --- a/flutter/test/screenshot/test_widget.dart +++ b/flutter/test/screenshot/test_widget.dart @@ -35,6 +35,7 @@ Future pumpTestElement(WidgetTester tester, Offstage(offstage: true, child: newImage()), Text(dummyText), Material(child: TextFormField()), + Material(child: TextField()), SizedBox( width: 100, height: 20, diff --git a/flutter/test/screenshot/widget_filter_test.dart b/flutter/test/screenshot/widget_filter_test.dart index 7342e920e..703ff31d2 100644 --- a/flutter/test/screenshot/widget_filter_test.dart +++ b/flutter/test/screenshot/widget_filter_test.dart @@ -4,6 +4,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:sentry_flutter/sentry_flutter.dart'; import 'package:sentry_flutter/src/screenshot/widget_filter.dart'; +import '../mocks.dart'; import 'test_widget.dart'; // Note: these tests predate existance of `SentryMaskingConfig` which now @@ -14,17 +15,23 @@ void main() async { const defaultBounds = Rect.fromLTRB(0, 0, 1000, 1000); final rootBundle = TestAssetBundle(); final otherBundle = TestAssetBundle(); + final logger = MockLogger(); final colorScheme = WidgetFilterColorScheme( defaultMask: Colors.white, defaultTextMask: Colors.green, background: Colors.red); - final createSut = ({bool redactImages = false, bool redactText = false}) { - final replayOptions = SentryPrivacyOptions(); - replayOptions.maskAllImages = redactImages; - replayOptions.maskAllText = redactText; - return WidgetFilter(replayOptions.buildMaskingConfig(), - (level, message, {exception, logger, stackTrace}) {}); + final createSut = ( + {bool redactImages = false, + bool redactText = false, + PlatformChecker? platformChecker}) { + final privacyOptions = SentryPrivacyOptions() + ..maskAllImages = redactImages + ..maskAllText = redactText; + logger.clear(); + final maskingConfig = privacyOptions.buildMaskingConfig( + logger.call, platformChecker ?? PlatformChecker()); + return WidgetFilter(maskingConfig, logger.call); }; boundsRect(WidgetFilterItem item) => @@ -39,7 +46,7 @@ void main() async { pixelRatio: 1.0, bounds: defaultBounds, colorScheme: colorScheme); - expect(sut.items.length, 5); + expect(sut.items.length, 6); }); testWidgets('does not redact text when disabled', (tester) async { @@ -73,12 +80,13 @@ void main() async { pixelRatio: 1.0, bounds: defaultBounds, colorScheme: colorScheme); - expect(sut.items.length, 5); + expect(sut.items.length, 6); expect(boundsRect(sut.items[0]), '624x48'); expect(boundsRect(sut.items[1]), '169x20'); expect(boundsRect(sut.items[2]), '800x192'); expect(boundsRect(sut.items[3]), '800x24'); - expect(boundsRect(sut.items[4]), '50x20'); + expect(boundsRect(sut.items[4]), '800x24'); + expect(boundsRect(sut.items[5]), '50x20'); }); }); @@ -215,6 +223,37 @@ void main() async { expect(sut.items.length, 1); expect(boundsRect(sut.items[0]), '344x248'); }); + + group('warning on sensitive widgets', () { + assert(MockPlatformCheckerBuildMode.values.length == 3); + for (final buildMode in MockPlatformCheckerBuildMode.values) { + testWidgets(buildMode.name, (tester) async { + final sut = createSut( + redactText: true, + platformChecker: MockPlatformChecker(buildMode: buildMode)); + final element = + await pumpTestElement(tester, children: [CustomPasswordWidget()]); + sut.obscure( + context: element, + pixelRatio: 1.0, + bounds: defaultBounds, + colorScheme: colorScheme); + final logMessages = logger.items + .where((item) => item.level == SentryLevel.warning) + .map((item) => item.message) + .toList(); + + if (buildMode == MockPlatformCheckerBuildMode.debug) { + expect( + logMessages, + anyElement(contains( + 'name matches widgets that should usually be masked because they may contain sensitive data'))); + } else { + expect(logMessages, isEmpty); + } + }); + } + }); } class TestAssetBundle extends CachingAssetBundle { @@ -223,3 +262,7 @@ class TestAssetBundle extends CachingAssetBundle { return ByteData(0); } } + +class CustomPasswordWidget extends Column { + const CustomPasswordWidget({super.key}); +}