Skip to content

feat: warning in screenshot/replay creation when a potentially sensitive widget is not masked #2375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 11, 2024
Merged
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
20 changes: 13 additions & 7 deletions flutter/lib/src/screenshot/recorder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,26 @@
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,

Check warning on line 67 in flutter/lib/src/screenshot/recorder.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/screenshot/recorder.dart#L67

Added line #L67 was not covered by tests
"SentryScreenshotWidget is not attached, skipping capture.");
_warningLogged = true;
}
return;
Expand Down Expand Up @@ -112,9 +119,9 @@
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);
Expand All @@ -125,8 +132,7 @@
picture.dispose();
}
} catch (e, stackTrace) {
options.logger(
SentryLevel.error, "$logName: failed to capture screenshot.",
_log(SentryLevel.error, "failed to capture screenshot.",

Check warning on line 135 in flutter/lib/src/screenshot/recorder.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/screenshot/recorder.dart#L135

Added line #L135 was not covered by tests
exception: e, stackTrace: stackTrace);
if (options.automatedTestMode) {
rethrow;
Expand Down
36 changes: 35 additions & 1 deletion flutter/lib/src/sentry_privacy_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
39 changes: 30 additions & 9 deletions flutter/test/mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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});
}
24 changes: 16 additions & 8 deletions flutter/test/screenshot/masking_config_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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',
''))
Expand All @@ -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)'
]);
});

Expand All @@ -148,6 +149,7 @@ void main() async {
expect(rulesAsStrings(sut), [
...alwaysEnabledRules,
'$SentryMaskingConstantRule<$Image>(mask)',
'$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)'
]);
});

Expand All @@ -159,6 +161,7 @@ void main() async {
expect(rulesAsStrings(sut), [
...alwaysEnabledRules,
'$SentryMaskingCustomRule<$Image>(Closure: (Element, Widget) => SentryMaskingDecision)',
'$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)'
]);
});

Expand All @@ -171,6 +174,7 @@ void main() async {
...alwaysEnabledRules,
'$SentryMaskingConstantRule<$Text>(mask)',
'$SentryMaskingConstantRule<$EditableText>(mask)',
'$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)'
]);
});

Expand All @@ -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();
Expand Down
1 change: 1 addition & 0 deletions flutter/test/screenshot/test_widget.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading