Skip to content
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

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

Merged
merged 11 commits into from
Dec 11, 2024
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
19 changes: 12 additions & 7 deletions flutter/lib/src/screenshot/recorder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,25 @@
final privacyOptions = isReplayRecorder
? options.experimental.privacyForReplay
: options.experimental.privacyForScreenshots;
final maskingConfig = privacyOptions?.buildMaskingConfig();
final maskingConfig = privacyOptions?.buildMaskingConfig(_log);
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 66 in flutter/lib/src/screenshot/recorder.dart

View check run for this annotation

Codecov / codecov/patch

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

Added line #L66 was not covered by tests
"SentryScreenshotWidget is not attached, skipping capture.");
_warningLogged = true;
}
return;
Expand Down Expand Up @@ -112,9 +118,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 +131,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 134 in flutter/lib/src/screenshot/recorder.dart

View check run for this annotation

Codecov / codecov/patch

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

Added line #L134 was not covered by tests
exception: e, stackTrace: stackTrace);
if (options.automatedTestMode) {
rethrow;
Expand Down
30 changes: 29 additions & 1 deletion flutter/lib/src/sentry_privacy_options.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:flutter/foundation.dart';
import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';
import 'package:meta/meta.dart';
Expand Down Expand Up @@ -27,7 +28,7 @@ class SentryPrivacyOptions {
final _userMaskingRules = <SentryMaskingRule>[];

@internal
SentryMaskingConfig buildMaskingConfig() {
SentryMaskingConfig buildMaskingConfig(SentryLogger logger) {
// First, we collect rules defined by the user (so they're applied first).
final rules = _userMaskingRules.toList();

Expand All @@ -50,12 +51,39 @@ 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 (kDebugMode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt of using options.platformChecker.isDebugMode()?

but doesn't really make a difference I think

only matters if we add an additional test case where we assert that the warning isn't added for release builds, then we can mock it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've made the changes and added test cases for non-debug builds

rules.add(
SentryMaskingCustomRule<Widget>((Element element, Widget widget) {
final type = widget.runtimeType.toString();
final regexp = 'video|webview|password|pinput|camera|chart';
if (RegExp(regexp, caseSensitive: false).hasMatch(type)) {
final optionsName = 'options.experimental.privacy';
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
23 changes: 23 additions & 0 deletions flutter/test/mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,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});
}
23 changes: 15 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,13 @@ void main() async {

group('$SentryReplayOptions.buildMaskingConfig()', () {
List<String> rulesAsStrings(SentryPrivacyOptions options) {
final config = options.buildMaskingConfig();
final config = options.buildMaskingConfig(MockLogger().call);
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 +135,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 +148,7 @@ void main() async {
expect(rulesAsStrings(sut), [
...alwaysEnabledRules,
'$SentryMaskingConstantRule<$Image>(mask)',
'$SentryMaskingCustomRule<$Widget>(Closure: ($Element, $Widget) => $SentryMaskingDecision)'
]);
});

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

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

Expand All @@ -179,15 +182,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
35 changes: 31 additions & 4 deletions flutter/test/screenshot/widget_filter_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -14,6 +15,7 @@ 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,
Expand All @@ -23,7 +25,8 @@ void main() async {
final replayOptions = SentryPrivacyOptions();
replayOptions.maskAllImages = redactImages;
replayOptions.maskAllText = redactText;
return WidgetFilter(replayOptions.buildMaskingConfig(),
logger.clear();
return WidgetFilter(replayOptions.buildMaskingConfig(logger.call),
(level, message, {exception, logger, stackTrace}) {});
};

Expand All @@ -39,7 +42,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 {
Expand Down Expand Up @@ -73,12 +76,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');
});
});

Expand Down Expand Up @@ -215,6 +219,25 @@ void main() async {
expect(sut.items.length, 1);
expect(boundsRect(sut.items[0]), '344x248');
});

testWidgets('warns on sensitive widgets', (tester) async {
final sut = createSut(redactText: true);
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();
expect(
logMessages,
anyElement(contains(
'name matches widgets that should usually be masked because they may contain sensitive data')));
});
}

class TestAssetBundle extends CachingAssetBundle {
Expand All @@ -223,3 +246,7 @@ class TestAssetBundle extends CachingAssetBundle {
return ByteData(0);
}
}

class CustomPasswordWidget extends Column {
const CustomPasswordWidget({super.key});
}
Loading