feat: warning in screenshot/replay creation when a potentially sensit…

…ive 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
getsentry · Dec 11, 2024 · ab99a31 · ab99a31
1 parent 3cc7034
commit ab99a31
Show file tree

Hide file tree

Showing 9 changed files with 173 additions and 41 deletions.
diff --git 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
@@ -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<void> 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
@@ -27,7 +27,8 @@ class SentryPrivacyOptions {
   final _userMaskingRules = <SentryMaskingRule>[];
 
   @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<Text>(SentryMaskingDecision.mask));
       rules.add(const SentryMaskingConstantRule<EditableText>(
           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<Widget>((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
@@ -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
@@ -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
@@ -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 = <MockLogItem>[];
+
+  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
@@ -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<String> 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,15 +183,19 @@ 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', () {
       final defaultRules = [
         ...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
@@ -35,6 +35,7 @@ Future<Element> pumpTestElement(WidgetTester tester,
                       Offstage(offstage: true, child: newImage()),
                       Text(dummyText),
                       Material(child: TextFormField()),
+                      Material(child: TextField()),
                       SizedBox(
                           width: 100,
                           height: 20,